Conversation
|
|
WalkthroughThis PR refactors the MediaView component to receive image data via props rather than Redux selectors. ChronologicalGallery conditionally renders MediaView with images. Some pages remove MediaView integration, while others update calls to pass the images prop. Changes
Sequence DiagramsequenceDiagram
participant Gallery as ChronologicalGallery
participant Redux as Redux Store
participant MediaView as MediaView Component
Note over Gallery,MediaView: New Flow (Prop-based)
Gallery->>Redux: selectIsImageViewOpen
Redux-->>Gallery: boolean (isImageViewOpen)
alt isImageViewOpen
Gallery->>Gallery: Pass chronologicallySortedImages<br/>to MediaView via prop
Gallery->>MediaView: images={chronologicallySortedImages}
MediaView->>MediaView: Derive totalImages &<br/>currentImage from prop
end
Note over MediaView: MediaView no longer<br/>directly queries Redux<br/>for image data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaView.tsx (1)
23-35: Implementation is safe, but consider resetting currentViewIndex on images change.The refactoring correctly shifts from Redux selector to prop-based images, with defensive bounds checking that prevents index errors.
However, there's an architectural coupling:
currentViewIndexremains global Redux state whileimagesare now passed per-component. If a user opens MediaView on one page (e.g., index 7 of 10 images), closes it, then navigates to another page with fewer images (e.g., 3 images), the stale index 7 would be out of bounds. The defensive checks at lines 31-34 and 131 prevent crashes by returning null, but MediaView won't display when expected.Consider resetting
currentViewIndexwhen theimagesarray changes to avoid stale index issues:+useEffect(() => { + // Reset view index if current index is out of bounds for new images array + if (currentViewIndex >= images.length) { + dispatch(setCurrentViewIndex(-1)); + } +}, [images, currentViewIndex, dispatch]);Alternatively, reset on component mount:
+useEffect(() => { + // Reset view on unmount to prevent stale indices + return () => { + dispatch(setCurrentViewIndex(-1)); + }; +}, [dispatch]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/Media/ChronologicalGallery.tsx(3 hunks)frontend/src/components/Media/MediaView.tsx(2 hunks)frontend/src/pages/AITagging/AITagging.tsx(1 hunks)frontend/src/pages/Home/Home.tsx(1 hunks)frontend/src/pages/PersonImages/PersonImages.tsx(1 hunks)frontend/src/types/Media.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/Media/MediaView.tsx (1)
frontend/src/types/Media.ts (1)
MediaViewProps(34-38)
frontend/src/pages/PersonImages/PersonImages.tsx (1)
frontend/src/components/Media/MediaView.tsx (1)
MediaView(23-214)
frontend/src/components/Media/ChronologicalGallery.tsx (4)
frontend/src/features/imageSelectors.ts (1)
selectIsImageViewOpen(13-16)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 (1)
MediaView(23-214)
⏰ 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 (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
🔇 Additional comments (5)
frontend/src/types/Media.ts (1)
37-37: LGTM! Type definition aligns with the refactoring.The addition of
images: Image[]toMediaViewPropscorrectly supports the architectural change to pass image data via props rather than Redux selectors.frontend/src/pages/PersonImages/PersonImages.tsx (1)
124-124: LGTM! Correctly passes images to MediaView.The images prop aligns with the array used for iteration and index calculation, preventing index mismatches.
frontend/src/pages/AITagging/AITagging.tsx (1)
7-7: LGTM! MediaView functionality preserved through ChronologicalGallery.The removal of direct MediaView usage is appropriate since ChronologicalGallery now handles MediaView rendering internally, centralizing the viewer logic.
frontend/src/components/Media/ChronologicalGallery.tsx (1)
184-184: MediaView rendering centralized correctly.The conditional rendering of MediaView with
chronologicallySortedImagesaligns properly with the index calculations viaimageIndexMap, preventing index errors.Note: If multiple
ChronologicalGalleryinstances were rendered simultaneously on the same page, each would render a MediaView (full-screen modal) whenisImageViewOpenis true, which could cause issues. Currently, each page renders only one instance, so this isn't a concern in practice.frontend/src/pages/Home/Home.tsx (1)
11-11: LGTM! Consistent refactoring pattern.Removal of direct MediaView usage is appropriate as ChronologicalGallery now manages MediaView rendering, maintaining functionality while centralizing the viewer logic.
1 participant
Fix errors caused due to #599
Summary by CodeRabbit