Fix/redux state simplification#599
Conversation
Enable scrolling with mouse wheel (up/down) to move the bottom grid left/right. Support double-finger swipe (left/right) to move the grid accordingly. Ensure that the currently open/selected image is always centered in the bottom grid.
…that in the Chronological Gallery. The image viewer now accepts a custom list of images and starting index, enabling context-aware viewing (e.g., from filtered or grouped galleries). Redux state and selectors were updated to support a separate viewerImages array, and related components were refactored to use this new approach. Unused utilities and props were removed for clarity.
Optimized chronological index lookup in ChronologicalGallery by introducing a memoized image index map
…/redux-state-simplification
…te related components
WalkthroughBackend adds optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Tauri
participant FileExplorer
User->>Frontend: View tagged images
Frontend->>Backend: GET /images/?tagged=true
Backend-->>Frontend: Filtered image list
Frontend->>Frontend: Store in Redux (setImages)
Frontend->>Frontend: Display in ChronologicalGallery
User->>Frontend: Click image
Frontend->>Frontend: Calculate chronological index
Frontend->>Frontend: Dispatch setCurrentViewIndex
Frontend->>Frontend: Update MediaView current image
User->>Frontend: Click "Open folder" button
Frontend->>Tauri: revealItemInDir(path)
Tauri->>FileExplorer: Show file in explorer
FileExplorer-->>User: File explorer opens
User->>Frontend: Scroll thumbnails
Frontend->>Frontend: Auto-center active thumbnail
Frontend->>Frontend: Smooth horizontal scroll
sequenceDiagram
participant Old Flow
participant New Flow
rect rgb(200, 220, 255)
Note over Old Flow: Image Navigation (Removed)
Old Flow->>Old Flow: ImageCard receives imageIndex
Old Flow->>Old Flow: Dispatch nextImage/previousImage
Old Flow->>Old Flow: MediaView manages navigation state
end
rect rgb(200, 255, 220)
Note over New Flow: Image Navigation (New)
New Flow->>New Flow: ChronologicalGallery computes index locally
New Flow->>New Flow: Dispatch setCurrentViewIndex directly
New Flow->>New Flow: ImageCard uses onClick callback
New Flow->>New Flow: MediaView derives from state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
|
1 similar 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: 3
🧹 Nitpick comments (3)
frontend/src/layout/layout.tsx (1)
9-15: LGTM! Clean navigation-based state cleanup.The useEffect correctly clears search state when navigating away from
/home. The logic is straightforward and maintains proper state isolation between routes.Optional: The
dispatchdependency can be safely removed from the dependency array as React guarantees its stability:- }, [location, dispatch]); + }, [location]);frontend/src/pages/Home/Home.tsx (1)
28-32: Consider simplifying the queryFn wrapper.Since
fetchAllImages()is being called without arguments, you can pass the function reference directly instead of wrapping it:const { data, isLoading, isSuccess, isError } = usePictoQuery({ queryKey: ['images'], - queryFn: () => fetchAllImages(), + queryFn: fetchAllImages, enabled: !isSearchActive, });frontend/src/components/Media/MediaView.tsx (1)
81-89: Improve error logging to include error details.The error logging on Line 86 uses
console.log(err)followed by a genericconsole.errormessage. Consider consolidating these to provide more useful debugging information:const handleOpenFolder = async () => { if (!currentImage?.path) return; try { await revealItemInDir(currentImage.path); } catch (err) { - console.log(err); - console.error('Failed to open folder.'); + console.error('Failed to open folder:', err); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
backend/app/database/images.py(2 hunks)backend/app/routes/images.py(2 hunks)docs/backend/backend_python/openapi.json(3 hunks)frontend/package.json(1 hunks)frontend/src-tauri/Cargo.toml(1 hunks)frontend/src-tauri/capabilities/migrated.json(1 hunks)frontend/src-tauri/src/main.rs(1 hunks)frontend/src/api/api-functions/images.ts(1 hunks)frontend/src/components/Dialog/FaceSearchDialog.tsx(3 hunks)frontend/src/components/Media/ChronologicalGallery.tsx(4 hunks)frontend/src/components/Media/ImageCard.tsx(2 hunks)frontend/src/components/Media/MediaThumbnails.tsx(2 hunks)frontend/src/components/Media/MediaView.tsx(4 hunks)frontend/src/components/Media/MediaViewControls.tsx(1 hunks)frontend/src/components/WebCam/WebCamComponent.tsx(3 hunks)frontend/src/features/imageSelectors.ts(1 hunks)frontend/src/features/imageSlice.ts(1 hunks)frontend/src/features/searchSlice.ts(1 hunks)frontend/src/layout/layout.tsx(1 hunks)frontend/src/pages/AITagging/AITagging.tsx(3 hunks)frontend/src/pages/Home/Home.tsx(4 hunks)frontend/src/pages/PersonImages/PersonImages.tsx(2 hunks)frontend/src/types/Media.ts(0 hunks)frontend/src/utils/dateUtils.ts(0 hunks)
💤 Files with no reviewable changes (2)
- frontend/src/types/Media.ts
- frontend/src/utils/dateUtils.ts
🧰 Additional context used
🧬 Code graph analysis (11)
frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
frontend/src/features/imageSlice.ts (1)
setImages(18-20)
frontend/src/api/api-functions/images.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
imagesEndpoints(1-3)
frontend/src/layout/layout.tsx (1)
frontend/src/features/searchSlice.ts (1)
clearSearch(21-24)
frontend/src/pages/PersonImages/PersonImages.tsx (2)
frontend/src/features/imageSlice.ts (1)
setCurrentViewIndex(22-34)frontend/src/components/Media/MediaView.tsx (1)
MediaView(26-218)
backend/app/routes/images.py (1)
backend/app/database/images.py (1)
db_get_all_images(122-210)
frontend/src/components/WebCam/WebCamComponent.tsx (1)
frontend/src/features/imageSlice.ts (1)
setImages(18-20)
frontend/src/components/Media/ImageCard.tsx (1)
frontend/src/types/Media.ts (1)
Image(13-22)
frontend/src/pages/AITagging/AITagging.tsx (4)
frontend/src/features/imageSelectors.ts (1)
selectImages(5-7)frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/components/Media/MediaView.tsx (1)
MediaView(26-218)
frontend/src/pages/Home/Home.tsx (3)
frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/components/Media/ChronologicalGallery.tsx (1)
ChronologicalGallery(23-180)frontend/src/components/Media/MediaView.tsx (1)
MediaView(26-218)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(19-109)frontend/src/features/imageSlice.ts (1)
setCurrentViewIndex(22-34)
frontend/src/components/Media/MediaView.tsx (3)
frontend/src/types/Media.ts (1)
MediaViewProps(34-37)frontend/src/features/imageSelectors.ts (1)
selectImages(5-7)frontend/src/features/imageSlice.ts (1)
setCurrentViewIndex(22-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
🔇 Additional comments (20)
frontend/package.json (1)
45-45: LGTM! Dependency addition follows project conventions.The Tauri opener plugin dependency is correctly placed alphabetically and uses caret versioning consistent with other Tauri plugins in the project.
frontend/src-tauri/Cargo.toml (1)
37-37: LGTM! Rust dependency follows existing patterns.The tauri-plugin-opener dependency uses exact versioning consistent with other Tauri plugins in the manifest. While the frontend uses
^2.5.2and the backend uses2.5.2, this version drift is acceptable and already present with other plugins in the codebase.frontend/src-tauri/src/main.rs (1)
11-11: LGTM! Plugin initialization follows the established pattern.The opener plugin is correctly initialized in the Tauri builder chain using the same pattern as other plugins. The placement at the beginning of the plugin chain is appropriate as it has no dependencies on other plugins.
frontend/src-tauri/capabilities/migrated.json (1)
139-139: Verification complete: opener plugin is properly invoked and permission is justified.The frontend code actively uses the opener plugin's
revealItemInDirfunction infrontend/src/components/Media/MediaView.tsx(imported at line 3, called at line 84). The backend plugin is initialized infrontend/src-tauri/src/main.rsat line 11, and dependencies are declared in bothpackage.jsonandCargo.toml. The permissionopener:allow-reveal-item-in-diris necessary and correctly configured.backend/app/database/images.py (2)
122-132: LGTM! Clear API enhancement with backward compatibility.The optional
taggedparameter is well-documented and maintains backward compatibility with existing calls. The docstring clearly explains the three filtering modes.
137-160: LGTM! Safe parameterized query with conditional filtering.The dynamic query construction correctly uses parameterized queries to prevent SQL injection. The conditional WHERE clause is only added when filtering is needed, and the parameter list is properly managed.
frontend/src/pages/PersonImages/PersonImages.tsx (2)
118-118: LGTM! Proper integration of centralized image viewing.The
onClickhandler correctly dispatchessetCurrentViewIndexto update the global state, enablingMediaViewto access the current image index from Redux.
124-124: LGTM! Consistent with Redux-based MediaView.
MediaViewno longer receives animagesprop and instead reads images directly from Redux state viaselectImages, as shown in the relevant code snippets. This centralization eliminates prop drilling and aligns with the PR's state simplification goals.frontend/src/api/api-functions/images.ts (1)
5-12: LGTM! Clean optional parameter handling.The function correctly handles the optional
taggedparameter, only including it in query params when explicitly provided. This aligns with the backend API's optional filtering capability.backend/app/routes/images.py (1)
46-52: LGTM! Clean FastAPI query parameter integration.The optional
taggedquery parameter is properly defined using FastAPI'sQuerywith a clear description, and correctly passed to the database layer. The implementation maintains backward compatibility while enabling filtered queries.docs/backend/backend_python/openapi.json (1)
836-886: LGTM! OpenAPI spec properly reflects backend changes.The OpenAPI specification correctly documents:
- The new optional
taggedquery parameter with proper type definition (boolean or null)- Added 422 validation error response for query parameter validation
- Adjusted error response structure for consistency
These changes accurately reflect the backend API enhancements.
frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
14-14: LGTM! Consistent with Redux state simplification.The change from
setResultstosetImagesaligns with the PR's goal of simplifying Redux state management. Verification confirms all components previously consuming search results fromsearchSlicenow read fromimageSlice:
selectImagesselector is properly defined infrontend/src/features/imageSelectors.ts- Components using it:
PersonImages.tsx,Home.tsx,AITagging.tsx,MediaView.tsx- No remaining references to
setResultsor old search result selectors found- Image data successfully consolidated into a single source of truth
frontend/src/features/imageSlice.ts (2)
22-34: LGTM! Cleaner bounds checking with local alias.The use of
imageListas a local alias improves readability, and the bounds checking logic correctly handles both the closed state (-1) and valid image indices (0 to length-1).
46-47: Removed Redux actions are not referenced in the codebase.Verification confirms that all previously exported actions (
addImages,nextImage,previousImage,updateImage,removeImage,setError) have been cleanly removed with no remaining imports or usages. The four exported actions (setImages,setCurrentViewIndex,closeImageView,clearImages) are actively used throughout the codebase.frontend/src/pages/Home/Home.tsx (2)
60-63: LGTM! Title logic simplified.The title now consistently uses
images.lengthfor both search results and the gallery, which aligns well with the Redux state simplification.
95-95: LGTM! MediaView API simplified.MediaView now derives images internally via selectors instead of receiving them as props. This is a cleaner pattern that reduces prop drilling and aligns with the Redux state simplification goals.
frontend/src/components/Media/ImageCard.tsx (1)
5-5: Excellent refactor! Redux coupling removed from presentational component.Moving the Redux dispatch logic out of
ImageCardand replacing it with anonClickcallback prop is a great architectural improvement. This makes the component more reusable and easier to test.Also applies to: 41-41
frontend/src/components/Media/MediaView.tsx (2)
3-9: LGTM! MediaView now self-contained with selector-based data.The refactor to derive images from
selectImagesinstead of props is a significant improvement. MediaView is now more self-contained and aligns perfectly with the Redux state simplification goals.Also applies to: 26-26, 30-30
53-65: LGTM! Navigation logic correctly updated.The navigation handlers now use
setCurrentViewIndexwith proper bounds checking. The index-based approach is cleaner than separatenextImage/previousImageactions and maintains correct navigation semantics.frontend/src/features/imageSelectors.ts (1)
1-16: Verify that removed selectors are no longer referenced.The selector API has been significantly reduced to three essential selectors:
selectImages,selectCurrentViewIndex, andselectIsImageViewOpen. According to the PR summary, the following selectors were removed:selectTotalImages,selectImagesError,selectCurrentImage,selectImageById,selectImagesByFolderId,selectTaggedImages,selectUntaggedImages, andselectImagesWithTags.Multiple searches of the codebase produced no results for these removed selectors. However, this absence of evidence is inconclusive—the selectors may be referenced in ways the search tools didn't capture, or the sandbox environment may have limited file coverage. Please manually verify that these removed selectors have been replaced or removed throughout the codebase, particularly in:
- Component files importing from imageSelectors
- Redux hooks or selectors that depend on the removed ones
- Test files
- Any dynamic or string-based references
| const chronologicallySortedImages = useMemo(() => { | ||
| return sortedGrouped.flatMap(({ months }) => | ||
| months.flatMap(([, imgs]) => imgs), | ||
| ); | ||
| }, [sortedGrouped]); | ||
|
|
||
| const imageIndexMap = useMemo(() => { | ||
| const map = new Map<string, number>(); | ||
| chronologicallySortedImages.forEach((img, idx) => { | ||
| map.set(img.id, idx); | ||
| }); | ||
| return map; | ||
| }, [chronologicallySortedImages]); | ||
|
|
There was a problem hiding this comment.
Fix image index mapping to match Redux order
imageIndexMap is built from chronologicallySortedImages, but the Redux store keeps images in their original order (the same images array you pass into this component). If the API returns images in a different order than your chronological grouping (e.g., unsorted or ascending), the computed chronologicalIndex no longer matches the indices in state.images.images. As a result, clicking a thumbnail can open the wrong image in MediaView. This is a correctness blocker.
Build the map directly from the images array so it stays aligned with the store:
- const chronologicallySortedImages = useMemo(() => {
- return sortedGrouped.flatMap(({ months }) =>
- months.flatMap(([, imgs]) => imgs),
- );
- }, [sortedGrouped]);
-
- const imageIndexMap = useMemo(() => {
- const map = new Map<string, number>();
- chronologicallySortedImages.forEach((img, idx) => {
- map.set(img.id, idx);
- });
- return map;
- }, [chronologicallySortedImages]);
+ const imageIndexMap = useMemo(() => {
+ const map = new Map<string, number>();
+ images.forEach((img, idx) => {
+ map.set(img.id, idx);
+ });
+ return map;
+ }, [images]);Remove the now-unused chronologicallySortedImages block as part of this change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chronologicallySortedImages = useMemo(() => { | |
| return sortedGrouped.flatMap(({ months }) => | |
| months.flatMap(([, imgs]) => imgs), | |
| ); | |
| }, [sortedGrouped]); | |
| const imageIndexMap = useMemo(() => { | |
| const map = new Map<string, number>(); | |
| chronologicallySortedImages.forEach((img, idx) => { | |
| map.set(img.id, idx); | |
| }); | |
| return map; | |
| }, [chronologicallySortedImages]); | |
| const imageIndexMap = useMemo(() => { | |
| const map = new Map<string, number>(); | |
| images.forEach((img, idx) => { | |
| map.set(img.id, idx); | |
| }); | |
| return map; | |
| }, [images]); |
🤖 Prompt for AI Agents
In frontend/src/components/Media/ChronologicalGallery.tsx around lines 52 to 65,
the imageIndexMap is currently built from chronologicallySortedImages which can
differ from the Redux/store image ordering causing incorrect indices; instead,
remove the chronologicallySortedImages block and construct imageIndexMap
directly from the original images array passed into the component (iterate
images and map img.id -> index) so the map aligns with state.images.images;
update any hooks/dependencies to use images for the memo and drop the unused
chronologicallySortedImages variable.
| interface ImageCardViewProps { | ||
| image: Image; | ||
| imageIndex: number; | ||
| className?: string; | ||
| isSelected?: boolean; | ||
| showTags?: boolean; | ||
| onClick?: () => void; | ||
| imageIndex?: number; | ||
| } | ||
|
|
||
| export function ImageCard({ | ||
| image, | ||
| imageIndex, | ||
| className, | ||
| isSelected = false, | ||
| showTags = true, | ||
| }: ImageCardProps) { | ||
| const dispatch = useDispatch(); | ||
| onClick, | ||
| }: ImageCardViewProps) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused imageIndex prop.
The imageIndex prop is declared in the interface but never used within the component. This is dead code and should be removed to keep the API clean.
interface ImageCardViewProps {
image: Image;
className?: string;
isSelected?: boolean;
showTags?: boolean;
onClick?: () => void;
- imageIndex?: number;
}
export function ImageCard({
image,
className,
isSelected = false,
showTags = true,
onClick,
}: ImageCardViewProps) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface ImageCardViewProps { | |
| image: Image; | |
| imageIndex: number; | |
| className?: string; | |
| isSelected?: boolean; | |
| showTags?: boolean; | |
| onClick?: () => void; | |
| imageIndex?: number; | |
| } | |
| export function ImageCard({ | |
| image, | |
| imageIndex, | |
| className, | |
| isSelected = false, | |
| showTags = true, | |
| }: ImageCardProps) { | |
| const dispatch = useDispatch(); | |
| onClick, | |
| }: ImageCardViewProps) { | |
| interface ImageCardViewProps { | |
| image: Image; | |
| className?: string; | |
| isSelected?: boolean; | |
| showTags?: boolean; | |
| onClick?: () => void; | |
| } | |
| export function ImageCard({ | |
| image, | |
| className, | |
| isSelected = false, | |
| showTags = true, | |
| onClick, | |
| }: ImageCardViewProps) { |
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageCard.tsx around lines 10 to 25, the
ImageCardViewProps interface declares an unused imageIndex prop; remove
imageIndex from the interface and from the component's destructured props so the
prop is no longer accepted or referenced, then run TypeScript/ESLint to ensure
no remaining references and update any callers if they relied on imageIndex.
| onToggleInfo: () => void; | ||
| onToggleFavorite: () => void; | ||
| onOpenFolder: () => void; | ||
| onOpenFolder: () => Promise<void>; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all call sites handle the async signature.
The onOpenFolder signature changed from synchronous to async. Ensure all components passing this callback properly handle the Promise (e.g., using async/await or .then()).
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of MediaViewControls and verify onOpenFolder is handled correctly
# Search for components using MediaViewControls and check onOpenFolder prop
rg -nP -A5 -B2 '<MediaViewControls' --type=tsx --type=ts
# Search for onOpenFolder implementations
rg -nP 'onOpenFolder.*=.*\(' --type=tsx --type=tsLength of output: 55
🏁 Script executed:
#!/bin/bash
# Find all usages of MediaViewControls component
echo "=== MediaViewControls usages ==="
rg -n '<MediaViewControls' -A5 -B2 frontend/src
echo -e "\n=== onOpenFolder prop assignments ==="
rg -n 'onOpenFolder\s*=' -A3 -B3 frontend/src
echo -e "\n=== onOpenFolder function definitions/implementations ==="
rg -n 'onOpenFolder\s*[:=].*=>' -A2 frontend/src
echo -e "\n=== MediaViewControls component definition ==="
rg -n 'export.*MediaViewControls\|interface.*MediaViewControls' -A20 frontend/srcLength of output: 2405
🏁 Script executed:
#!/bin/bash
# Find handleOpenFolder definition and implementation
echo "=== handleOpenFolder definition in MediaView.tsx ==="
rg -n 'handleOpenFolder' -A5 -B2 frontend/src/components/Media/MediaView.tsx
# Find where onOpenFolder is called/invoked
echo -e "\n=== onOpenFolder invocations in MediaViewControls ==="
rg -n 'onOpenFolder\(' -A3 -B3 frontend/src/components/Media/MediaViewControls.tsx
# Get full MediaViewControls component to see implementation
echo -e "\n=== Full MediaViewControls.tsx content ==="
cat frontend/src/components/Media/MediaViewControls.tsxLength of output: 3924
Fix Promise handling in onOpenFolder button click.
The onOpenFolder callback is invoked via onClick={onOpenFolder} in MediaViewControls.tsx (line 46) without awaiting the returned Promise. This silently ignores errors and prevents proper async flow control. Wrap the call to handle the Promise:
onClick={async () => {
try {
await onOpenFolder();
} catch (err) {
console.error('Failed to open folder:', err);
}
}}
🤖 Prompt for AI Agents
In frontend/src/components/Media/MediaViewControls.tsx around line 8, the
onOpenFolder prop is declared as returning a Promise but the click handler
invokes it directly (onClick={onOpenFolder}) which ignores rejections; change
the click handler to an async wrapper that awaits onOpenFolder and catches
errors (use try/catch to log or surface the error) so Promise rejections are
handled and async flow is preserved.
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores