feat: Add pagination support for image gallery API #1067
Conversation
📝 WalkthroughWalkthroughThis PR implements full pagination support across the stack: backend database layer adds LIMIT/OFFSET queries with pagination metadata, API routes accept page/limit parameters with dual response modes, and the frontend integrates infinite scroll via IntersectionObserver while maintaining backward compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Gallery as ChronologicalGallery<br/>(IntersectionObserver)
participant Page as Home/AITagging<br/>Page Component
participant Redux as Redux State
participant API as fetchPaginatedImages
participant Backend as Backend API<br/>(GET /images)
participant DB as Database
User->>User: Scrolls near bottom
Gallery->>Gallery: Observer detects sentinel<br/>in viewport
rect rgba(100, 150, 200, 0.5)
Note over Gallery,Page: Check conditions
alt hasMore && !isLoadingMore
Gallery->>Page: Trigger onLoadMore()
Page->>Redux: setLoadingMore(true)
Page->>API: fetchPaginatedImages(nextPage, limit)
else Cannot load
Gallery->>Gallery: Ignore (loading or no more)
end
end
rect rgba(150, 100, 200, 0.5)
Note over API,DB: Fetch next page
API->>Backend: GET /images?page=2&limit=50
Backend->>DB: db_get_all_images(page=2, limit=50)
DB->>Backend: Query results + pagination metadata
Backend->>API: PaginatedImagesResponse
end
rect rgba(100, 200, 150, 0.5)
Note over API,Page: Update state
API->>Page: Return data + pagination
Page->>Redux: appendImages(newImages)
Page->>Redux: setPagination(updatedMetadata)
Page->>Redux: setLoadingMore(false)
Redux->>Gallery: State update triggers re-render
Gallery->>Gallery: Display new images
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/database/images.py (1)
164-263: Apply SQL LIMIT/OFFSET before joining tags to avoid loading full dataset into memory.The current code executes a query without LIMIT/OFFSET (line 58), materializes all matching images with
cursor.fetchall()(line 60), then applies pagination in Python (line 110). For galleries with thousands of images, this loads the entire result set—multiplied by tag count due to the LEFT JOIN—into memory before slicing, defeating pagination's purpose and causing unnecessary memory/performance issues.Use LIMIT/OFFSET in the base image query (before joining tags) via a CTE, as proposed. SQLite has supported WITH clauses since version 3.8.3 (released 2014), so compatibility is not a concern.
🤖 Fix all issues with AI agents
In `@backend/app/routes/images.py`:
- Around line 68-133: The route currently allows partial pagination by accepting
only page or only limit and then calling db_get_all_images which can return all
rows; add validation at the start of the handler to reject mismatched pagination
params: if (page is None) != (limit is None) raise an HTTPException(400) (or
return a 400 response) with a clear message like "Both 'page' and 'limit' must
be provided together or neither"; keep existing behavior when both are None
(return GetAllImagesResponse) and when both are present call db_get_all_images
and return PaginatedImagesResponse as before. Ensure you reference the existing
parameter names page, limit and functions db_get_all_images,
GetAllImagesResponse and PaginatedImagesResponse when implementing the check.
🧹 Nitpick comments (1)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
121-151: Usedisconnect()instead ofunobserve()for cleaner observer cleanup.For component unmount scenarios where the entire observer is no longer needed,
disconnect()is the spec-aligned pattern and avoids managing individual target references. Since this observer handles a single element and is torn down when the component unmounts,disconnect()is the appropriate choice.♻️ Suggested refactor
- return () => { - if (currentRef) { - observer.unobserve(currentRef); - } - }; + return () => { + observer.disconnect(); + };
| tagged: Optional[bool] = Query(None, description="Filter images by tagged status"), | ||
| page: Optional[int] = Query(None, ge=1, description="Page number (1-indexed)"), | ||
| limit: Optional[int] = Query( | ||
| None, ge=1, le=100, description="Number of images per page (max 100)" | ||
| ), | ||
| ): | ||
| """Get all images from the database.""" | ||
| """ | ||
| Get all images from the database. | ||
| - If `page` and `limit` are provided, returns paginated results. | ||
| - If `page` and `limit` are not provided, returns all images (backward compatible). | ||
| """ | ||
| try: | ||
| # Get all images with tags from database (single query with optional filter) | ||
| images = db_get_all_images(tagged=tagged) | ||
|
|
||
| # Convert to response format | ||
| image_data = [ | ||
| ImageData( | ||
| id=image["id"], | ||
| path=image["path"], | ||
| folder_id=image["folder_id"], | ||
| thumbnailPath=image["thumbnailPath"], | ||
| metadata=image_util_parse_metadata(image["metadata"]), | ||
| isTagged=image["isTagged"], | ||
| isFavourite=image.get("isFavourite", False), | ||
| tags=image["tags"], | ||
| # Get images with optional pagination | ||
| result = db_get_all_images(tagged=tagged, page=page, limit=limit) | ||
|
|
||
| # Check if paginated result | ||
| if page is not None and limit is not None: | ||
| images = result["images"] | ||
| pagination = result["pagination"] | ||
|
|
||
| # Convert to response format | ||
| image_data = [ | ||
| ImageData( | ||
| id=image["id"], | ||
| path=image["path"], | ||
| folder_id=image["folder_id"], | ||
| thumbnailPath=image["thumbnailPath"], | ||
| metadata=image_util_parse_metadata(image["metadata"]), | ||
| isTagged=image["isTagged"], | ||
| isFavourite=image.get("isFavourite", False), | ||
| tags=image["tags"], | ||
| ) | ||
| for image in images | ||
| ] | ||
|
|
||
| return PaginatedImagesResponse( | ||
| success=True, | ||
| message=f"Successfully retrieved {len(image_data)} images (page {pagination['page']} of {pagination['total_pages']})", | ||
| data=image_data, | ||
| pagination=PaginationInfo(**pagination), | ||
| ) | ||
| else: | ||
| # Non-paginated response (backward compatible) | ||
| images = result | ||
|
|
||
| # Convert to response format | ||
| image_data = [ | ||
| ImageData( | ||
| id=image["id"], | ||
| path=image["path"], | ||
| folder_id=image["folder_id"], | ||
| thumbnailPath=image["thumbnailPath"], | ||
| metadata=image_util_parse_metadata(image["metadata"]), | ||
| isTagged=image["isTagged"], | ||
| isFavourite=image.get("isFavourite", False), | ||
| tags=image["tags"], | ||
| ) | ||
| for image in images | ||
| ] | ||
|
|
||
| return GetAllImagesResponse( | ||
| success=True, | ||
| message=f"Successfully retrieved {len(image_data)} images", | ||
| data=image_data, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the file
find . -name "images.py" -path "*/routes/*" | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 92
🏁 Script executed:
# Search for the db_get_all_images function definition
rg "def db_get_all_images" -A 30 -t pyRepository: AOSSIE-Org/PictoPy
Length of output: 2254
🏁 Script executed:
# Check the structure of the backend directory
ls -la backend/app/routes/ 2>/dev/null | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 551
🏁 Script executed:
# Read the complete db_get_all_images function
cat -n backend/app/database/images.py | grep -A 80 "def db_get_all_images"Repository: AOSSIE-Org/PictoPy
Length of output: 3287
🏁 Script executed:
# Read the complete get_all_images function in routes
cat -n backend/app/routes/images.py | grep -A 80 "def get_all_images"Repository: AOSSIE-Org/PictoPy
Length of output: 3613
🏁 Script executed:
# Get the rest of db_get_all_images function
cat -n backend/app/database/images.py | sed -n '142,250p'Repository: AOSSIE-Org/PictoPy
Length of output: 4576
🏁 Script executed:
# Get the rest of db_get_all_images function
cat -n backend/app/database/images.py | sed -n '250,280p'Repository: AOSSIE-Org/PictoPy
Length of output: 1209
Guard against partial pagination params to avoid unexpected full fetches.
If a client sends only page or only limit without both parameters, the function silently falls back to returning all images. This defeats the purpose of pagination and can reintroduce the large-response risk. Both page and limit should be required together or explicitly default; partial pagination parameters should not be silently accepted.
Consider rejecting requests with mismatched parameters:
Suggested validation
def get_all_images(
tagged: Optional[bool] = Query(None, description="Filter images by tagged status"),
page: Optional[int] = Query(None, ge=1, description="Page number (1-indexed)"),
limit: Optional[int] = Query(
None, ge=1, le=100, description="Number of images per page (max 100)"
),
):
"""
Get all images from the database.
- If `page` and `limit` are provided, returns paginated results.
- If `page` and `limit` are not provided, returns all images (backward compatible).
"""
try:
+ if (page is None) ^ (limit is None):
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail="Both 'page' and 'limit' must be provided together.",
+ )
# Get images with optional pagination
result = db_get_all_images(tagged=tagged, page=page, limit=limit)🤖 Prompt for AI Agents
In `@backend/app/routes/images.py` around lines 68 - 133, The route currently
allows partial pagination by accepting only page or only limit and then calling
db_get_all_images which can return all rows; add validation at the start of the
handler to reject mismatched pagination params: if (page is None) != (limit is
None) raise an HTTPException(400) (or return a 400 response) with a clear
message like "Both 'page' and 'limit' must be provided together or neither";
keep existing behavior when both are None (return GetAllImagesResponse) and when
both are present call db_get_all_images and return PaginatedImagesResponse as
before. Ensure you reference the existing parameter names page, limit and
functions db_get_all_images, GetAllImagesResponse and PaginatedImagesResponse
when implementing the check.
1 participant
Summary
Implements pagination for the image gallery API to improve performance with large photo collections.
Changes
Backend
db_get_all_images()to supportpageandlimitparametersPaginationInforesponse modelFrontend
PaginationInfoandPaginatedAPIResponsetypesfetchPaginatedImages()functionAPI Usage
GET /images/?page=1&limit=50 # First 50 images
GET /images/?page=2&limit=50 # Next 50 images
GET /images/?page=1&limit=50&tagged=true # Paginated tagged images
Testing
Closes #1065
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.