feat: Implement infinite scroll pagination for images #620
Conversation
- Add server-side pagination to /images endpoint with limit/offset params - Implement infinite scroll with TanStack Query in Home and AI Tagging pages - Add pagination configuration constants for consistency - Improve loader UX with minimum display time - Implement optimistic updates for AI tagging toggle - Fix SQL injection vulnerability in image queries using CTE - Add real-time query refetching for folder operations - Refactor to eliminate magic numbers and verbose comments Backend: - New pagination config with MAX_PAGE_SIZE=1000, DEFAULT=50 - CTE-based queries for security and performance - Comprehensive test suite for pagination edge cases - Updated table creation order respecting foreign keys Frontend: - Infinite scroll at 80% threshold - 50 images per page with smooth loading - Optimistic UI updates for instant feedback - Centralized retry/stale time configuration - Clean, maintainable code structure
|
|
WalkthroughAdds offset/limit pagination end-to-end: backend pagination config and CTE-based DB query returning images + total, route changes with validation and error responses, frontend infinite-scroll integration and pagination constants, tests and OpenAPI updates, plus minor .gitignore and table-init ordering tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FE as Frontend (Home / AITagging)
participant API as Backend API (/images)
participant DB as Database
rect "#e8f0ff"
note over U,FE: Initial page load (offset=0)
U->>FE: Visit page
FE->>API: GET /images?limit=50&offset=0
API->>DB: db_get_all_images(limit, offset)
DB->>DB: CTE computes total_count and selects paginated IDs
DB-->>API: {images:[...], total:N}
API-->>FE: {success, data:{images,total,limit,offset}}
FE->>U: Render first page
end
rect "#e8ffe8"
note over U,FE: Infinite scroll -> fetch next page
U->>FE: Scroll near bottom
FE->>FE: compute nextOffset = offset + limit
FE->>API: GET /images?limit=50&offset=50
API->>DB: db_get_all_images(limit, offset)
DB-->>API: {images:[...], total:N}
API-->>FE: Next page data
FE->>U: Append images
end
rect "#fff4dd"
note over FE: Stop when offset+limit >= total
FE->>FE: getNextPageParam() returns undefined
FE->>U: No more fetches
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/hooks/useFolderOperations.tsx (1)
93-119: Rollback optimistic folder toggles on failure.
onMutatepushes an optimisticsetFoldersupdate, but there’s no rollback path. If either mutation fails, the Redux store stays in the optimistic state until some unrelated refetch happens, so the UI shows the wrong tagging status. TanStack recommends returning the previous snapshot fromonMutateand restoring it inonErrorfor optimistic updates.(tanstack.com)Apply a change like:
const enableAITaggingMutation = usePictoMutation({ mutationFn: async (folder_id: string) => enableAITagging({ folder_ids: [folder_id] }), autoInvalidateTags: ['folders', 'images'], - onMutate: async (folder_id: string) => { - const updatedFolders = folders.map(f => - f.folder_id === folder_id ? { ...f, AI_Tagging: true } : f - ); - dispatch(setFolders(updatedFolders)); - }, + onMutate: async (folder_id: string) => { + const previousFolders = folders; + const updatedFolders = previousFolders.map((f) => + f.folder_id === folder_id ? { ...f, AI_Tagging: true } : f, + ); + dispatch(setFolders(updatedFolders)); + return { previousFolders }; + }, + onError: (_err, _folderId, context) => { + if (context?.previousFolders) { + dispatch(setFolders(context.previousFolders)); + } + }, }); const disableAITaggingMutation = usePictoMutation({ mutationFn: async (folder_id: string) => disableAITagging({ folder_ids: [folder_id] }), autoInvalidateTags: ['folders', 'images'], - onMutate: async (folder_id: string) => { - const updatedFolders = folders.map(f => - f.folder_id === folder_id ? { ...f, AI_Tagging: false } : f - ); - dispatch(setFolders(updatedFolders)); - }, + onMutate: async (folder_id: string) => { + const previousFolders = folders; + const updatedFolders = previousFolders.map((f) => + f.folder_id === folder_id ? { ...f, AI_Tagging: false } : f, + ); + dispatch(setFolders(updatedFolders)); + return { previousFolders }; + }, + onError: (_err, _folderId, context) => { + if (context?.previousFolders) { + dispatch(setFolders(context.previousFolders)); + } + }, });backend/app/routes/images.py (1)
88-96: Remove double metadata parsing to avoid 500s.
db_get_all_imagesnow parses the metadata before returning it (see backend/app/database/images.py lines 121-222), soimage["metadata"]is already a dict. Running it throughimage_util_parse_metadataagain causes aTypeError: the JSON object must be str, bytes or bytearray, not dict, turning every GET /images into a 500. Please return the value directly and drop the redundant import.Apply this diff:
-from app.utils.images import image_util_parse_metadata ... - metadata=image_util_parse_metadata(image["metadata"]), + metadata=image["metadata"],
🧹 Nitpick comments (1)
frontend/src/components/Loader/GlobalLoader.tsx (1)
12-36: Use timeout typing that works in browser builds
NodeJS.Timeoutrequires Node typings; in a browser-only TypeScript build this namespace is undefined, which can break compilation. Switching toReturnType<typeof setTimeout>keeps the code portable without changing behavior.- const timerRef = useRef<NodeJS.Timeout | null>(null); + const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.gitignore(1 hunks)backend/app/config/pagination.py(1 hunks)backend/app/database/images.py(1 hunks)backend/app/routes/images.py(3 hunks)backend/main.py(1 hunks)backend/tests/test_images.py(1 hunks)docs/backend/backend_python/openapi.json(4 hunks)frontend/src/api/api-functions/images.ts(1 hunks)frontend/src/components/Loader/GlobalLoader.tsx(1 hunks)frontend/src/config/pagination.ts(1 hunks)frontend/src/hooks/useFolderOperations.tsx(5 hunks)frontend/src/hooks/useQueryExtension.ts(4 hunks)frontend/src/pages/AITagging/AITagging.tsx(2 hunks)frontend/src/pages/Home/Home.tsx(2 hunks)frontend/src/types/API.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
frontend/src/hooks/useQueryExtension.ts (1)
frontend/src/config/pagination.ts (2)
DEFAULT_RETRY_COUNT(8-8)DEFAULT_RETRY_DELAY(9-9)
frontend/src/pages/AITagging/AITagging.tsx (6)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(10-14)ChronologicalGallery(25-187)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(11-31)frontend/src/config/pagination.ts (5)
IMAGES_PER_PAGE(5-5)DEFAULT_STALE_TIME(11-11)DEFAULT_RETRY_COUNT(8-8)DEFAULT_RETRY_DELAY(9-9)SCROLL_THRESHOLD(6-6)frontend/src/types/Media.ts (1)
Image(13-22)frontend/src/features/imageSlice.ts (1)
setImages(18-20)frontend/src/features/loaderSlice.ts (2)
showLoader(17-20)hideLoader(21-24)
frontend/src/components/Loader/GlobalLoader.tsx (2)
frontend/src/types/loading.ts (1)
GlobalLoaderProps(1-4)frontend/src/config/pagination.ts (1)
MIN_LOADER_DISPLAY_TIME(7-7)
frontend/src/pages/Home/Home.tsx (9)
frontend/src/features/imageSelectors.ts (1)
selectImages(5-7)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(10-14)ChronologicalGallery(25-187)frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(11-31)frontend/src/config/pagination.ts (5)
IMAGES_PER_PAGE(5-5)DEFAULT_STALE_TIME(11-11)DEFAULT_RETRY_COUNT(8-8)DEFAULT_RETRY_DELAY(9-9)SCROLL_THRESHOLD(6-6)frontend/src/types/Media.ts (1)
Image(13-22)frontend/src/features/imageSlice.ts (1)
setImages(18-20)frontend/src/features/loaderSlice.ts (2)
showLoader(17-20)hideLoader(21-24)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
backend/tests/test_images.py (4)
backend/app/database/images.py (3)
db_create_images_table(52-85)db_bulk_insert_images(88-119)db_insert_image_classes_batch(302-333)backend/app/database/folders.py (1)
db_create_folders_table(15-36)backend/app/database/yolo_mapping.py (1)
db_create_YOLO_classes_table(6-35)backend/tests/test_folders.py (1)
test_db(20-33)
backend/main.py (7)
backend/app/database/yolo_mapping.py (1)
db_create_YOLO_classes_table(6-35)backend/app/database/face_clusters.py (1)
db_create_clusters_table(21-39)backend/app/database/faces.py (1)
db_create_faces_table(29-52)backend/app/database/folders.py (1)
db_create_folders_table(15-36)backend/app/database/albums.py (2)
db_create_albums_table(7-26)db_create_album_images_table(29-48)backend/app/database/images.py (1)
db_create_images_table(52-85)backend/app/database/metadata.py (1)
db_create_metadata_table(8-30)
frontend/src/api/api-functions/images.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-11)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
imagesEndpoints(1-3)
frontend/src/hooks/useFolderOperations.tsx (2)
frontend/src/config/pagination.ts (2)
TAGGING_STATUS_POLL_INTERVAL(10-10)DEFAULT_RETRY_COUNT(8-8)frontend/src/features/folderSlice.ts (1)
setFolders(21-23)
backend/app/database/images.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
backend/app/config/pagination.py (1)
frontend/src/config/pagination.ts (1)
DEFAULT_RETRY_COUNT(8-8)
backend/app/routes/images.py (2)
backend/app/schemas/images.py (1)
ErrorResponse(59-62)backend/app/database/images.py (1)
db_get_all_images(122-223)
🔇 Additional comments (1)
.gitignore (1)
33-33: Appropriate additions for ML/model file handling and Python env isolation.The three new entries follow existing .gitignore conventions and best practices:
.venv/complements the existingvenv/entry (line 32), covering both common naming conventions for Python virtual environments.*.onnxprevents large ML model files (in ONNX format, as used for AI tagging) from bloating the repository.backend/models/centralizes the ignore rule for the model artifacts directory, aligning with the PR's ML feature integration.Also applies to: 36-37
Fix two critical issues identified during code review: 1. Optimistic Updates Rollback (frontend) - Added rollback logic to useFolderOperations mutations - enableAITaggingMutation now saves previousFolders and restores on error - disableAITaggingMutation now saves previousFolders and restores on error - Ensures UI state consistency when mutations fail 2. Remove Double Metadata Parsing (backend) - Removed redundant image_util_parse_metadata call in routes/images.py - Metadata now parsed only once in db_get_all_images() - Eliminates unnecessary overhead and potential errors - Metadata correctly returned as dict from database layer 3. Fix Table Creation Order (backend) - Reordered table creation to respect foreign key dependencies - images table now created before faces, album_images, and metadata - Prevents FK constraint violations when PRAGMA foreign_keys = ON - Applied fix to main.py and test fixtures (conftest.py, test_images.py) 4. Fix Query Refetch Logic (frontend) - Changed autoInvalidateTags to iterate and refetch each tag separately - Prevents passing entire array as single composite queryKey - Ensures proper cache invalidation for each query Testing: - Frontend: TypeScript compiles with 0 errors, all Jest tests pass (6/6) - Backend: 19/20 pagination tests pass, 2/2 metadata tests pass - Added test_metadata_fix.py to verify metadata structure - Fixed test_images.py fixture to properly create test database Files modified: - backend/app/routes/images.py (remove double parsing) - backend/main.py (fix table order) - backend/tests/conftest.py (fix table order) - backend/tests/test_images.py (fix test fixtures) - backend/tests/test_metadata_fix.py (new verification tests) - frontend/src/hooks/useFolderOperations.tsx (add rollback) - frontend/src/hooks/useQueryExtension.ts (fix refetch iteration)
|
Fixed issues suggested by CodeRabbit: added rollback for optimistic folder toggles and removed double metadata parsing in /images. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/tests/test_metadata_fix.py (2)
11-31: Consider adding explicit test data setup.The test only exercises assertions when
result["total"] > 0(line 18). If the database is empty or has been cleared by other tests, the critical assertions on lines 23-30 won't run, and the test passes vacuously without validating metadata structure.Consider adding explicit test data seeding similar to
sample_images_datafixture inbackend/tests/test_images.py, or use a dedicated fixture that ensures at least one image exists before running assertions.
33-62: Consider adding explicit test data setup.Similar to the database test above, this test only exercises assertions when
data["total"] > 0(line 42). The critical metadata structure validations on lines 50-61 won't run if the database is empty.Consider using a fixture like
sample_images_datafrombackend/tests/test_images.pyto guarantee test data exists, ensuring consistent test coverage.backend/tests/test_images.py (1)
100-107: Consider extracting the metadata JSON into a constant or helper.The hard-coded metadata JSON string on line 105 is verbose and repeated in
test_single_image_dataset(line 375). Extracting this into a module-level constant or helper function would improve maintainability and reduce duplication.Example:
# At module level DEFAULT_TEST_METADATA = '{"name": "test.jpg", "date_created": "2024-01-01", "width": 1920, "height": 1080, "file_size": 1024, "item_type": "image", "file_location": "/test"}' # In fixture "metadata": DEFAULT_TEST_METADATA,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/routes/images.py(3 hunks)backend/main.py(1 hunks)backend/tests/conftest.py(1 hunks)backend/tests/test_images.py(1 hunks)backend/tests/test_metadata_fix.py(1 hunks)frontend/src/hooks/useFolderOperations.tsx(5 hunks)frontend/src/hooks/useQueryExtension.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/src/hooks/useQueryExtension.ts (1)
frontend/src/config/pagination.ts (2)
DEFAULT_RETRY_COUNT(8-8)DEFAULT_RETRY_DELAY(9-9)
backend/main.py (7)
backend/app/database/yolo_mapping.py (1)
db_create_YOLO_classes_table(6-35)backend/app/database/face_clusters.py (1)
db_create_clusters_table(21-39)backend/app/database/folders.py (1)
db_create_folders_table(15-36)backend/app/database/albums.py (2)
db_create_albums_table(7-26)db_create_album_images_table(29-48)backend/app/database/images.py (1)
db_create_images_table(52-85)backend/app/database/faces.py (1)
db_create_faces_table(29-52)backend/app/database/metadata.py (1)
db_create_metadata_table(8-30)
backend/tests/test_metadata_fix.py (2)
backend/tests/test_images.py (2)
app(68-72)client(76-78)backend/app/database/images.py (1)
db_get_all_images(122-223)
frontend/src/hooks/useFolderOperations.tsx (2)
frontend/src/config/pagination.ts (2)
TAGGING_STATUS_POLL_INTERVAL(10-10)DEFAULT_RETRY_COUNT(8-8)frontend/src/features/folderSlice.ts (1)
setFolders(21-23)
backend/app/routes/images.py (2)
backend/app/schemas/images.py (1)
ErrorResponse(59-62)backend/app/database/images.py (1)
db_get_all_images(122-223)
backend/tests/conftest.py (6)
backend/app/database/face_clusters.py (1)
db_create_clusters_table(21-39)backend/app/database/folders.py (1)
db_create_folders_table(15-36)backend/app/database/albums.py (2)
db_create_albums_table(7-26)db_create_album_images_table(29-48)backend/app/database/images.py (1)
db_create_images_table(52-85)backend/app/database/faces.py (1)
db_create_faces_table(29-52)backend/app/database/metadata.py (1)
db_create_metadata_table(8-30)
backend/tests/test_images.py (5)
backend/tests/test_folders.py (2)
test_db(20-33)client(76-78)backend/app/database/images.py (3)
db_create_images_table(52-85)db_bulk_insert_images(88-119)db_insert_image_classes_batch(302-333)backend/app/database/folders.py (1)
db_create_folders_table(15-36)backend/app/database/yolo_mapping.py (1)
db_create_YOLO_classes_table(6-35)backend/tests/test_metadata_fix.py (1)
client(65-73)
🔇 Additional comments (26)
frontend/src/hooks/useQueryExtension.ts (4)
18-18: LGTM: Centralized configuration constants.Importing retry defaults from a dedicated config module improves maintainability and ensures consistent retry behavior across the application.
46-47: LGTM: Configuration-driven retry behavior.Using the imported constants eliminates magic numbers and ensures consistency with application-wide retry policies.
63-69: LGTM: Per-tag refetch correctly implemented.The iteration over
autoInvalidateTagsnow properly refetches each query independently, resolving the composite key issue flagged in the previous review. The guard condition ensures safe handling of empty or undefined arrays.
97-98: LGTM: Consistent retry configuration.Query defaults mirror the mutation retry configuration, ensuring uniform behavior across all Picto operations.
frontend/src/hooks/useFolderOperations.tsx (7)
15-18: LGTM: Centralized polling and retry constants.Importing configuration constants ensures consistent behavior and simplifies future adjustments to polling intervals and retry logic.
32-33: LGTM: Configuration-driven polling intervals.Using
TAGGING_STATUS_POLL_INTERVALfor bothstaleTimeandrefetchIntervalensures consistent polling behavior and makes it easy to adjust polling frequency application-wide.
36-38: LGTM: Appropriate retry and refetch settings for polling query.Disabling
retryOnMountandrefetchOnWindowFocuswhile using the centralized retry count is appropriate for a background polling query, preventing unnecessary network calls while maintaining retry resilience during active polling.
57-62: LGTM: Ensures Redux stays synchronized with query data.This effect complements the
useMutationFeedbackcallback by ensuring the Redux store reflects query data changes on all refetches, not just the initial load, maintaining consistency after cache invalidations.
92-105: LGTM: Well-implemented optimistic updates with proper rollback.The optimistic update pattern correctly:
- Preserves previous state for rollback
- Immediately reflects UI changes
- Safely rolls back on error using context
- Invalidates both 'folders' and 'images' caches since AI tagging affects image metadata
120-133: LGTM: Consistent optimistic update pattern for disable operation.The disable mutation mirrors the enable pattern with appropriate inverse logic (
AI_Tagging: false), maintaining consistency and properly invalidating dependent caches.
148-148: LGTM: Proper cache invalidation for folder deletion.Adding 'images' to
autoInvalidateTagsensures the images cache updates when a folder is deleted, maintaining data consistency without the complexity of optimistic delete (which is appropriate given the destructive nature of the operation).backend/main.py (1)
47-57: LGTM! Table creation order now respects foreign key dependencies.The reordering correctly addresses the critical issue flagged in the previous review. The new sequence creates base tables first (YOLO_classes, clusters, folders, albums), followed by images, and finally dependent tables (faces, album_images, metadata) that reference images. This prevents FK constraint initialization failures.
backend/tests/conftest.py (2)
25-31: LGTM! Table creation order now matches the fixed sequence in main.py.The reordering correctly establishes foreign key dependencies, creating images (line 28) before dependent tables faces, album_images, and metadata. The added comments clearly document the dependency relationships, improving maintainability.
34-35: Good practice: re-raising exceptions halts test execution on setup failure.The explicit
raiseensures that if table creation fails, the test suite stops immediately rather than proceeding with an incomplete database schema, which would cause confusing downstream failures.backend/tests/test_images.py (6)
16-64: Test isolation setup is comprehensive and follows good practices.The fixture correctly:
- Creates an isolated temporary database per test
- Overrides the global DATABASE_PATH
- Reloads modules to pick up the new path
- Creates required tables in dependency order
- Provides cleanup on teardown
130-145: LGTM! Test validates non-paginated retrieval behavior.The test correctly verifies that fetching all images without pagination parameters returns all 100 images with
limitandoffsetset toNonein the response.
170-181: Excellent pagination consistency check.This test ensures that paginated results don't overlap across pages by verifying that the sets of image IDs are disjoint. This is a critical property of correct pagination implementation.
278-301: Comprehensive validation error coverage.The test suite properly validates that invalid pagination parameters (zero, negative, or exceeding maximum values) return appropriate HTTP error codes (422 for validation errors).
302-306: No issues found. The test is correct.The validation logic correctly returns HTTP 400 when offset (9,999,999) exceeds MAX_OFFSET_VALUE (1,000,000), with limit present. The test expectation aligns with the implementation.
42-45: No issues found. The minimal table creation is sufficient.Verification confirms that all test methods exclusively use the
/imagesendpoint, which only callsdb_get_all_images. This function performs JOINs on only three tables:images,image_classes, andmappings—all of which are created and populated in the test setup. The tables mentioned in your concern (clusters, albums, faces, album_images) are not referenced by any test method or required by the images endpoint functionality. The current table setup is appropriate and complete for the test suite.backend/app/routes/images.py (6)
5-9: LGTM! Pagination constants properly centralized.Importing MAX_PAGE_SIZE, MAX_OFFSET_VALUE, and MIN_PAGE_SIZE from the config module promotes maintainability and consistency across the application.
43-45: LGTM! Response model extended with pagination metadata.Adding
total,limit, andoffsetfields to the response allows clients to implement proper pagination UI (e.g., page numbers, "load more" buttons).
57-64: Query parameter validation is well-structured.Using FastAPI's
Querywithgeandleconstraints provides automatic validation and generates proper OpenAPI documentation. The constraints (MIN_PAGE_SIZE ≤ limit ≤ MAX_PAGE_SIZE, offset ≥ 0) are appropriate for pagination.
82-84: LGTM! Database call properly uses pagination parameters.The call to
db_get_all_images(tagged=tagged, limit=limit, offset=offset)correctly passes all parameters, and extractingimagesandtotalfrom the result dict aligns with the updated database function signature.
92-92: LGTM! Metadata handling simplified.Using
image["metadata"]directly is correct sincedb_get_all_imagesalready parses the metadata usingimage_util_parse_metadata(as seen in backend/app/database/images.py lines 194-195). This eliminates double-parsing that was flagged in the test_metadata_fix.py tests.
108-109: Good practice: preserving HTTP status codes for validation errors.Re-raising
HTTPExceptionbefore the generic exception handler ensures that 400 Bad Request errors (from offset validation) aren't incorrectly converted to 500 Internal Server Error.
Change offset validation to check offset independently rather than requiring both limit and offset to be provided. Before: if limit is not None and offset is not None and offset > MAX_OFFSET_VALUE After: if offset is not None and offset > MAX_OFFSET_VALUE This ensures that requests with a large offset but no limit are correctly rejected, preventing potential performance issues. Testing: - Large offset without limit: correctly rejected (400) - Large offset with limit: correctly rejected (400) - Valid offset without limit: correctly accepted (200) - Valid offset with limit: correctly accepted (200) - All existing tests still pass (19/20) Fixes potential bypass of offset validation when limit is not specified.
|
@mohi-devhub Please resolve conflicts. |
- Integrated favorites feature (isFavourite field) from main - Preserved infinite scroll pagination (50 images/page) - Resolved conflicts in images.py: added isFavourite to SELECT query - Resolved conflicts in Home.tsx: combined useInfiniteQuery with useMutationFeedback - New features from main: togglefav API, MyFav page, favorites UI
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/routes/images.py (1)
144-151: toggle_favourite is incompatible with new db_get_all_images return type
db_get_all_images()now returns a dict{"images": [...], "total": ...}, but this route still treats it as an iterable of image dicts. As written:image = next( (img for img in db_get_all_images() if img["id"] == image_id), None ) ... "isFavourite": image.get("isFavourite", False),will:
- iterate over dict keys instead of images, so
img["id"]will fail, and- call
.getonNoneif no image is found, raising an AttributeError.This is a runtime‑breaking bug whenever the endpoint is hit.
A minimal fix that respects the new API would be:
- image = next( - (img for img in db_get_all_images() if img["id"] == image_id), None - ) + result = db_get_all_images() + image = next( + (img for img in result.get("images", []) if img["id"] == image_id), + None, + ) + if image is None: + raise HTTPException( + status_code=404, + detail="Image not found after toggling favourite", + )Longer term, a dedicated
db_get_image_by_idhelper would avoid loading all images just to return the updated flag.
🧹 Nitpick comments (4)
frontend/src/pages/Home/Home.tsx (1)
32-59: Well-structured infinite query configuration.The
useInfiniteQuerysetup correctly implements offset-based pagination:
pageParamdrives the offset for each fetchgetNextPageParamproperly calculates whether more pages exist (nextOffset < total)- Query is disabled during search mode to avoid conflicts
Optional refinement for line 56:
The
placeholderData: (previousData) => previousDataconfiguration is redundant for infinite queries, as TanStack Query already preserves previous pages during refetches by default. Consider removing it for simplicity:initialPageParam: 0, enabled: !isSearchActive, staleTime: DEFAULT_STALE_TIME, - placeholderData: (previousData) => previousData, retry: DEFAULT_RETRY_COUNT, retryDelay: DEFAULT_RETRY_DELAY,docs/backend/backend_python/openapi.json (1)
833-915: Pagination query parameters and 400 error contract look consistentThe
limit/offsetquery params and their integer bounds, plus the new400response usingapp__schemas__images__ErrorResponse, align well with the backend route and pagination constraints. If the backend applies a default page size whenlimitis omitted, consider adding a"default"value in thelimitschema to make that explicit in the docs.backend/app/database/images.py (1)
123-239: db_get_all_images pagination semantics and error handlingThe CTE‑based pagination and tag aggregation look solid, and returning
{"images": images, "total": total_count}is a good fit for the route.A couple of points to consider:
offsetonly takes effect whenlimitis notNoneand> 0. If callers ever sendoffset>0withlimit=None, they’ll silently get results from the start. Either:
- enforce
offsetbeing used only together withlimitat the route layer, or- support offset‑only by emitting a
LIMIT -1 OFFSET ?(or similar) whenlimitisNonebutoffsetis provided.- On exceptions you log and return
{"images": [], "total": 0}. That prevents the API from surfacing genuine DB failures (the route will think “0 images” is a valid result). Consider letting exceptions propagate so the route can return a 500, or at least distinguishing between “no data” and “error” in the return structure.If you want stricter typing, you could also introduce a small
TypedDictfor the return value instead of a baredict.backend/app/routes/images.py (1)
53-75: Query parameter validation is good; consider explicit default limit semanticsUsing
MIN_PAGE_SIZE/MAX_PAGE_SIZEandge=0foroffsetgives you strong input validation. However, whenlimitisNonethe route passes that through to the DB, which results in an unbounded query andlimit=Nonein the response. If the goal is to always paginate and avoid fetching all images, consider applying aDEFAULT_PAGE_SIZEwhenlimitis omitted and reflecting that in both the response and OpenAPI docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/database/images.py(2 hunks)backend/app/routes/images.py(3 hunks)docs/backend/backend_python/openapi.json(4 hunks)frontend/src/pages/Home/Home.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/pages/Home/Home.tsx (7)
frontend/src/features/imageSelectors.ts (1)
selectImages(5-7)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(10-14)ChronologicalGallery(25-187)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(11-31)frontend/src/config/pagination.ts (5)
IMAGES_PER_PAGE(5-5)DEFAULT_STALE_TIME(11-11)DEFAULT_RETRY_COUNT(8-8)DEFAULT_RETRY_DELAY(9-9)SCROLL_THRESHOLD(6-6)frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/features/imageSlice.ts (1)
setImages(18-20)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
backend/app/database/images.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
backend/app/routes/images.py (2)
backend/app/schemas/images.py (1)
ErrorResponse(59-62)backend/app/database/images.py (1)
db_get_all_images(123-240)
🔇 Additional comments (12)
frontend/src/pages/Home/Home.tsx (7)
1-21: LGTM! Clean imports for infinite scroll implementation.The new imports are well-organized and appropriate:
- React hooks (
useCallback,useMemo) for performance optimization- TanStack Query's
useInfiniteQueryfor pagination- Centralized pagination configuration constants
61-78: Excellent scroll-based pagination implementation.The scroll handler correctly:
- Guards against unnecessary fetches with
hasNextPageand!isFetchingNextPagechecks- Calculates scroll percentage accurately:
(scrollTop + clientHeight) / scrollHeight- Triggers
fetchNextPage()at 80% scroll threshold- Properly attaches and cleans up the event listener
85-89: LGTM! Redux synchronization preserves global state.This effect correctly synchronizes query results to Redux when not in search mode. While this component uses
allImagesdirectly for rendering, maintaining Redux state ensures other components can access the image list through Redux selectors.
91-93: LGTM! Clean separation of data sources.The
imagesmemo elegantly switches between:
- Redux images during face search (preserving search results)
- Query-fetched images during normal browsing
This maintains compatibility with existing search functionality while enabling infinite scroll for the gallery.
95-103: LGTM! Properly scoped loading feedback.The mutation feedback correctly shows loading state only during the initial gallery load when not in search mode (
isLoading && !isSearchActive). This prevents:
- Showing loading UI during search (which has its own feedback)
- Showing loading for subsequent page fetches (handled by the "Loading more images..." indicator on line 127-129)
105-109: LGTM! Dynamic title reflects UI state.The memoized title correctly displays:
- Search result count when actively searching with matches
- Default "Image Gallery" title otherwise
117-134: LGTM! Clean rendering with loading states.The conditional rendering properly handles:
- Gallery display with images, title, and timeline integration
- Inline loading indicator during subsequent page fetches (
isFetchingNextPage)- Empty state when no images exist
The fragment grouping of the gallery and loading indicator maintains clean JSX structure.
docs/backend/backend_python/openapi.json (1)
1891-1940: Extending GetAllImagesResponse with pagination metadata is backwards‑compatibleAdding
total,limit, andoffsetas optional fields preserves existing clients (success/message/data still required) while exposing enough information for infinite scroll. This shape matches the backend response and frontend expectations.backend/app/routes/images.py (4)
44-50: Response model pagination fields match backend and docsAdding
total,limit, andoffsettoGetAllImagesResponsemirrors the DB return shape and OpenAPI schema, and keeping them optional avoids breaking existing consumers.
77-85: Offset upper‑bound validation correctly rejects oversize offsetsThe independent
offset > MAX_OFFSET_VALUEcheck ensures large offsets are rejected with a structured 400ErrorResponse, which closes the earlier gap where offset could bypass validation.
87-112: Integration with db_get_all_images and pagination metadata looks correctUnpacking
result["images"]andresult["total"], constructingImageData(reusing the already‑parsedmetadata), and returningtotal,limit, andoffsetin the response aligns well with the DB contract and OpenAPI schema. The success message that includes both the page size and total count is also helpful for clients.
114-124: 500‑error mapping is consistent with ErrorResponse modelRe‑raising
HTTPExceptionand mapping unexpected exceptions to a 500 with a serializedErrorResponsekeeps error handling predictable and matches the documented 500 schema for this route.
- Convert APIResponse interface to generic type APIResponse<T> - Add default type parameter (T = any) for backward compatibility - Type fetchAllImages return as APIResponse<Image[]> - Remove unsafe type assertion (page?.data as Image[]) in Home.tsx - TypeScript now correctly infers Image[] type from API response - Improves type safety and IDE IntelliSense across codebase
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/pages/Home/Home.tsx (1)
79-82: Type safety issue persists: APIResponse.data field remains loosely typed.This is related to the previous review comment about unsafe type assertion. Line 81 uses
page?.data ?? []which assumesdatais anImage[], but theAPIResponseinterface definesdataas{ [key: string]: any; }according to past reviews.The proper fix is to:
- Make
APIResponsegeneric (e.g.,APIResponse<T>) with properly typeddata: T, OR- Create a specific
PaginatedResponse<T>interface withdata: T[],total: number,offset: number,limit: numberThen update
fetchAllImagesto return the properly typed response so line 81 can safely usepage.datawithout the nullish coalescing workaround.
🧹 Nitpick comments (2)
frontend/src/pages/Home/Home.tsx (2)
54-54: ReconsiderstaleTime: 0for better performance.Setting
staleTime: DEFAULT_STALE_TIME(which is 0) marks data as immediately stale, potentially triggering unnecessary refetches when components remount or the query refocuses.For image gallery data that doesn't change frequently, consider using a longer
staleTime(e.g., 5 minutes:5 * 60 * 1000) to reduce server load and improve perceived performance.Apply this diff to use a more appropriate staleTime:
- staleTime: DEFAULT_STALE_TIME, + staleTime: 5 * 60 * 1000, // 5 minutesOr update
DEFAULT_STALE_TIMEinfrontend/src/config/pagination.tsif this should be a global default.
84-88: Consider optimizing Redux updates during infinite scroll.The effect dispatches
setImages(allImages)every timeallImageschanges, which happens on every new page load during infinite scroll. This could cause frequent Redux updates and potential performance overhead, especially if multiple components subscribe to the images state.Consider whether Redux updates are needed on every page load, or only:
- On initial load
- When switching from search mode back to normal mode
- When explicitly refreshing the gallery
If frequent updates are necessary for other components, this is acceptable. Otherwise, consider debouncing or limiting updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/api/api-functions/images.ts(1 hunks)frontend/src/pages/Home/Home.tsx(2 hunks)frontend/src/types/API.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/api/api-functions/images.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/Home/Home.tsx (7)
frontend/src/features/imageSelectors.ts (1)
selectImages(5-7)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(10-14)ChronologicalGallery(25-187)frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(12-32)frontend/src/config/pagination.ts (5)
IMAGES_PER_PAGE(5-5)DEFAULT_STALE_TIME(11-11)DEFAULT_RETRY_COUNT(8-8)DEFAULT_RETRY_DELAY(9-9)SCROLL_THRESHOLD(6-6)frontend/src/features/imageSlice.ts (1)
setImages(18-20)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
🔇 Additional comments (9)
frontend/src/types/API.ts (2)
1-2: LGTM! Excellent type safety improvement.Making
APIResponsegeneric withdata?: Tenables strongly-typed API responses (e.g.,APIResponse<Image[]>), providing better IDE support and compile-time safety while maintaining backward compatibility with theanydefault.
6-8: LGTM! Pagination metadata properly typed.The optional pagination fields (
total,limit,offset) are correctly typed and align with the backend pagination implementation described in the PR objectives.frontend/src/pages/Home/Home.tsx (7)
1-20: LGTM: Imports are well-organized and appropriate.The addition of
useCallback,useMemo,useInfiniteQuery, and pagination constants properly supports the infinite scroll implementation. Renaming the selector result toreduxImages(line 25) clearly distinguishes Redux state from query-derived images.
60-77: LGTM: Scroll detection logic is well-implemented.The scroll handler correctly:
- Guards against missing refs, exhausted pages, and concurrent fetches (line 61)
- Calculates scroll progress accurately (lines 63-64)
- Triggers pagination at the configured threshold (line 66)
- Properly attaches and cleans up the event listener (lines 71-77)
The
useCallbackdependencies are complete and accurate.
90-92: LGTM: Image selection logic correctly handles search vs. normal mode.The memoized selector appropriately chooses
reduxImagesduring active search orallImagesduring normal browsing, with correct dependencies.
94-102: LGTM: Mutation feedback configuration is appropriate.The loading state logic
isPending: isLoading && !isSearchActive(line 95) is correct. Since the query is disabled during search mode (line 53:enabled: !isSearchActive),isLoadingshould be false anyway, making the!isSearchActivecheck redundant but harmless as a defensive guard.The feedback messages appropriately hide success notifications and show relevant error messages.
104-108: LGTM: Title logic appropriately reflects search state.The memoized title correctly displays search result count when active, or the default gallery title otherwise, with proper dependencies.
117-130: LGTM: Rendering logic properly handles loading states.The rendering correctly:
- Displays
ChronologicalGallerywith all required props includingscrollContainerReffor offset calculations- Shows a loading indicator only during
isFetchingNextPage(lines 125-129)- Falls back to
EmptyGalleryStatewhen no images are presentThe fragment wrapper appropriately contains both the gallery and loading indicator.
44-51: Remove this review comment - pagination metadata is properly typed in APIResponse.Verification confirms the pagination metadata fields (
total,offset,limit) ARE properly typed in theAPIResponseinterface as optional numbers. ThefetchAllImagesfunction returnsPromise<APIResponse<Image[]>>, and thegetNextPageParamlogic correctly accesses these fields with optional chaining, matching the interface definition exactly. No typing issue exists here.Likely an incorrect or invalid review comment.
|
@rahulharpal1603 Have fixed the issue, please check |
CRITICAL BUG FIX - App launch failure on macOS arm64 Root cause: Infinite recursion in onboarding step components causing the app to freeze/crash on launch when localStorage has saved state. Issues fixed: 1. AvatarSelectionStep - useEffect missing deps, re-dispatching on every render 2. FolderSetupStep - useEffect missing deps, re-dispatching on every render 3. ThemeSelectionStep - useEffect missing deps, re-dispatching on every render 4. ServerCheck - missing dispatch/stepIndex dependencies 5. InitialSteps - missing navigate dependency Solution: - Added hasMarkedCompleted flag to prevent re-dispatching markCompleted - Added proper useEffect dependencies (dispatch, stepIndex, navigate) - Prevents infinite loop: useEffect -> dispatch -> state change -> re-render -> useEffect This fixes issue AOSSIE-Org#645 where the bundled app fails to launch on macOS Apple Silicon due to infinite recursion in the login/onboarding page. Tested scenarios: - Fresh install (no localStorage) - Returning user (localStorage has avatar/name/theme/folder) - Step-by-step completion without infinite loops
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/OnboardingSteps/ServerCheck.tsx (1)
68-69: LGTM! Dependency array correctly updated.Adding
dispatchandstepIndexto the dependency array correctly addresses potential stale closure issues and aligns with React Hook rules. Since the query success states are stable once true andmarkCompletedis idempotent, repeated dispatches are not a concern.Optional consistency note: Other onboarding steps (ThemeSelectionStep, AvatarSelectionStep, FolderSetupStep) use a
hasMarkedCompletedguard to prevent duplicate completion dispatches. While not necessary here due to stable query states, adding a similar guard would provide defensive consistency across all onboarding components:export const ServerCheck: React.FC<ServerCheckProps> = ({ stepIndex }) => { const dispatch = useDispatch(); + const [hasMarkedCompleted, setHasMarkedCompleted] = React.useState(false); // ... queries ... useEffect(() => { // ... loading and error handling ... if (mainBackendSuccess && syncMicroserviceSuccess) { dispatch(hideLoader()); - dispatch(markCompleted(stepIndex)); + if (!hasMarkedCompleted) { + setHasMarkedCompleted(true); + dispatch(markCompleted(stepIndex)); + } } - }, [/* deps */]); + }, [/* deps, hasMarkedCompleted */]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/ServerCheck.tsx(1 hunks)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx(1 hunks)frontend/src/pages/InitialSteps/InitialSteps.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
frontend/src/features/onboardingSlice.ts (1)
markCompleted(31-42)
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
frontend/src/features/onboardingSlice.ts (1)
markCompleted(31-42)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
frontend/src/features/onboardingSlice.ts (1)
markCompleted(31-42)
🔇 Additional comments (7)
frontend/src/pages/InitialSteps/InitialSteps.tsx (1)
16-16: LGTM! Effect dependencies correctly updated.Adding
navigateto the dependency array correctly follows React Hook rules and prevents potential stale closure issues.frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)
32-32: LGTM! Completion guard pattern correctly implemented.The
hasMarkedCompletedstate prevents duplicatemarkCompleteddispatches when the effect re-runs, which is essential since effects execute even when the component returnsnull(line 52-53).
35-39: LGTM! Effect logic and dependencies correctly implemented.The guard condition and dependency array are correct:
!hasMarkedCompletedprevents duplicate dispatches across re-renderslocalStorage.getItem('themeChosen')enables auto-completion when returning to an already-completed step- Dependencies (
hasMarkedCompleted,dispatch,stepIndex) are complete per React Hook rulesfrontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (2)
35-35: LGTM! Completion guard correctly added.The
hasMarkedCompletedstate follows the same pattern as ThemeSelectionStep, preventing duplicate completion dispatches.
38-42: LGTM! Effect implementation matches established pattern.The guard condition and dependency array correctly implement auto-completion for already-completed steps while preventing duplicate dispatches. Dependencies are complete per React Hook rules.
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (2)
32-32: LGTM! Completion guard correctly added.The
hasMarkedCompletedstate prevents duplicate completion dispatches, maintaining consistency with the other onboarding steps.
35-39: LGTM! Effect correctly implements auto-completion with guard.The implementation matches the established pattern across onboarding steps:
- Guard prevents duplicate
markCompleteddispatches- Enables auto-completion when
folderChosenflag exists in localStorage- Dependencies (
hasMarkedCompleted,dispatch,stepIndex) are complete per React Hook rules
2 participants
Implement Infinite Scroll Pagination & Secure Image Queries
📝 Overview
This PR implements an efficient infinite scroll pagination system across the application.
Critically, it also resolves a major SQL injection vulnerability found in the image retrieval queries by refactoring them to use parameterized Common Table Expressions (CTEs).
✨ Key Feature: Infinite Scroll
useInfiniteQuery) and the Intersection Observer API for efficient scroll detection.🔐 Security Improvements (Critical Fix)
limitandoffset) are now properly validated and sanitized on the backend.🔧 Backend & Performance
limit/offsetpagination system.MAX_PAGE_SIZEof 1000.DEFAULT=50,MAX_OFFSET=1M).🧹 Code Quality & Refactoring
🧪 Testing
Fixes #619
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.