Bug Fix: Prevent PersonImages from polluting global image state#713
Bug Fix: Prevent PersonImages from polluting global image state#713VasuS609 wants to merge 4 commits intoAOSSIE-Org:mainfrom
Conversation
- Use local useState instead of Redux dispatch(setImages) - Add loading spinner and empty state for better UX - Fixes bug where Home gallery shows only person's images after navigation - Preserves full gallery state when navigating back to Home
|
|
|
Warning Rate limit exceeded@VasuS609 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughPersonImages now stores fetched images in local component state ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/pages/PersonImages/PersonImages.tsx (3)
37-38: Add user-facing error message.The error case only hides the loader but doesn't inform users why images failed to load. This creates a poor user experience when API calls fail.
Consider adding an error state message similar to the empty state:
} else if (isError) { dispatch(hideLoader());Then in the render section (after line 123):
) : isError ? ( <div className="flex items-center justify-center py-12"> <p className="text-muted-foreground"> Failed to load images. Please try again. </p> </div>
48-50: Remove redundant state update.Line 49 sets
clusterNameto its current value, which is a no-op.Apply this diff:
const handleEditName = () => { - setClusterName(clusterName); setIsEditing(true); };
53-57: Remove redundant state update.Line 54 sets
clusterNameto its current value, which is a no-op.Apply this diff:
const handleSaveName = () => { - setClusterName(clusterName); renameClusterMutate(clusterName); setIsEditing(false); };
🧹 Nitpick comments (2)
frontend/src/pages/PersonImages/PersonImages.tsx (2)
40-40: Avoidanytype for better type safety.The response should be properly typed instead of using
any.Apply this diff:
- const res: any = data?.data; + const res = data?.data as { images?: Image[]; cluster_name?: string };
111-135: Excellent loading and empty state UX improvements!The conditional rendering correctly handles loading, empty, and populated states with appropriate user feedback.
Optional: The loading spinner (lines 114) could be extracted to a reusable component if this pattern appears elsewhere in the codebase, but this is a low-priority optimization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/PersonImages/PersonImages.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/PersonImages/PersonImages.tsx (3)
frontend/src/types/Media.ts (1)
Image(13-22)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-111)frontend/src/components/Media/MediaView.tsx (1)
MediaView(26-200)
🔇 Additional comments (4)
frontend/src/pages/PersonImages/PersonImages.tsx (4)
7-7: LGTM!Import changes correctly remove Redux image state dependencies while retaining
selectIsImageViewOpenfor modal visibility.
21-21: LGTM!Local state declaration properly isolates person-specific images from global Redux state, addressing the core bug.
42-42: LGTM!Correctly updates local component state instead of dispatching to Redux, which fixes the state pollution bug.
138-138: LGTM!MediaView correctly receives
personImageswhich matches the indices used by ImageCard components, ensuring proper image navigation in the viewer.
- Remove redundant setClusterName calls - Add error state UI for failed image loads - Replace 'any' type with proper interface
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/pages/PersonImages/PersonImages.tsx (2)
34-46: Tighten loader effect to avoid potential stuck loader edge casesRight now the loader handling is:
showLoaderwhenisLoadingis true.hideLoaderonly whenisErroror whenisSuccess && data?.data.If the query ever reaches
isSuccess === truebutdata?.datais falsy (e.g., API returns an unexpected shape ornull),hideLoaderwill never be dispatched and the global loader could remain visible indefinitely. Also, if the component unmounts while loading, there’s no cleanup to ensure the loader is cleared, depending on howloaderSliceis implemented.Consider restructuring so that:
showLoaderis only called whileisLoadingis true.hideLoaderis always called when!isLoading(regardless of success/error/data shape).- Any
dataparsing happens after that.For example:
useEffect(() => { - if (isLoading) { - dispatch(showLoader('Loading images')); - } else if (isError) { - dispatch(hideLoader()); - } else if (isSuccess && data?.data) { - const res = data.data as { images?: Image[]; cluster_name?: string }; - const images = res.images || []; - setPersonImages(images); - setClusterName(res.cluster_name || 'random_name'); - dispatch(hideLoader()); - } -}, [data, isSuccess, isError, isLoading, dispatch]); + if (isLoading) { + dispatch(showLoader('Loading images')); + return; + } + + // Not loading anymore: ensure loader is hidden regardless of success/error/data shape + dispatch(hideLoader()); + + if (isSuccess && data?.data) { + const res = data.data as { images?: Image[]; cluster_name?: string }; + const images = res.images || []; + setPersonImages(images); + setClusterName(res.cluster_name || 'random_name'); + } +}, [data, isSuccess, isLoading, dispatch]);You can keep
isErrorin the deps if your linter enforces it, but it’s no longer needed in the branching logic.
109-151: Retry via full page reload is heavy; prefer refetching the queryThe new loading / error / empty states look good and clearly scoped to this page. One concern: the Retry button currently uses
window.location.reload(), which is heavy-handed and loses in-memory UI state.If
usePictoQueryexposes arefetch(or similar) function, consider wiring Retry to that instead:const { data, isLoading, isSuccess, isError, refetch } = usePictoQuery({ ... }); // ... <Button variant="outline" onClick={() => refetch()} className="mt-4" > Retry </Button>This keeps the retry localized to the data fetch without a full app reload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/PersonImages/PersonImages.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/PersonImages/PersonImages.tsx (3)
frontend/src/types/Media.ts (1)
Image(13-22)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-111)frontend/src/components/Media/MediaView.tsx (1)
MediaView(26-200)
🔇 Additional comments (1)
frontend/src/pages/PersonImages/PersonImages.tsx (1)
21-22: LocalpersonImagesstate + MediaView wiring looks correctMoving person-specific images into
personImagesand passing that into<MediaView images={personImages} />cleanly fixes the global image-state pollution while keeping the viewer behavior consistent withImageCardandMediaViewexpectations (index-based navigation over the provided array). No issues here.Also applies to: 39-43, 154-155
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/pages/PersonImages/PersonImages.tsx (2)
39-45: Ensure global loader is always hidden once the query settles
hideLoaderis only called whenisErroris true or whenisSuccess && data?.datais truthy. If the query succeeds butdata?.datais unexpectedly falsy, the global loader would remain shown indefinitely even though the inline UI moves on to the empty state.Consider restructuring the effect so the loader is hidden whenever the query leaves the loading state, independent of the
datashape, e.g.:useEffect(() => { - if (isLoading) { - dispatch(showLoader('Loading images')); - } else if (isError) { - dispatch(hideLoader()); - } else if (isSuccess && data?.data) { - const res = data.data as { images?: Image[]; cluster_name?: string }; - const images = res.images || []; - setPersonImages(images); - setClusterName(res.cluster_name || 'random_name'); - dispatch(hideLoader()); - } + if (isLoading) { + dispatch(showLoader('Loading images')); + return; + } + + // Query has settled (success or error) — always hide the global loader + dispatch(hideLoader()); + + if (isSuccess && data?.data) { + const res = data.data as { images?: Image[]; cluster_name?: string }; + const images = res.images || []; + setPersonImages(images); + setClusterName(res.cluster_name || 'random_name'); + } }, [data, isSuccess, isError, isLoading, dispatch]);This keeps the UX robust even if the backend occasionally returns an unexpected payload.
109-151: Use query refetch instead of full page reload for Retry and review duplicate loadersThe new loading/error/empty/grid flow looks good and reads clearly, but there are two small UX nits:
- The Retry button currently calls
window.location.reload(), which tears down the whole SPA and reloads everything. IfusePictoQueryexposes arefetchfunction, prefer wiring Retry to that instead so only this query re-runs:- const { data, isLoading, isSuccess, isError } = usePictoQuery({ + const { data, isLoading, isSuccess, isError, refetch } = usePictoQuery({ queryKey: ['person-images', clusterId], queryFn: async () => fetchClusterImages({ clusterId: clusterId || '' }), }); ... - <Button - variant="outline" - onClick={() => window.location.reload()} - className="mt-4" - > + <Button + variant="outline" + onClick={() => refetch()} + className="mt-4" + > Retry </Button>
- You now have both the global loader (via
showLoader/hideLoader) and this inline spinner controlled byisLoading. If the global loader is visible over this page, users may see two loaders at once. It may be worth standardizing on either the global overlay or the inline spinner for this screen, depending on project UX conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/PersonImages/PersonImages.tsx(4 hunks)
⏰ 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 (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
🔇 Additional comments (2)
frontend/src/pages/PersonImages/PersonImages.tsx (2)
21-27: Local personImages state cleanly isolates person gallery from global storeUsing
useState<Image[]>([])forpersonImagesand driving the grid from this local state (rather than Redux) aligns with the PR goal of preventing global image pollution and keeps the person gallery scoped to this page. No issues spotted with this change.
7-7: MediaView wiring correctly switches to local personImages while reusing global open stateKeeping
isImageViewOpenin Redux viaselectIsImageViewOpenbut passingimages={personImages}into<MediaView>ensures the viewer opens using the same locally-fetched set rendered in this grid, without mutating the global images slice. This is a clean way to fix the original gallery pollution bug while preserving existing open/close behavior.Also applies to: 20-21, 154-154
1 participant
fixe:#706
Problem
PersonImages component was overwriting global Redux image state, causing the Home page gallery to display only the last viewed person's images instead of the full gallery.
Reproduction Steps
Solution
dispatch(setImages)to localuseState(personImages)Changes
src/pages/PersonImages/PersonImages.tsx- Use local state + improved loading/empty statesTesting
Home shows full gallery
Person page shows only their images
Navigating back preserves full gallery
Loading state displays during fetch
Empty state shows when no images found
##fixes:#706
Summary by CodeRabbit
Refactor
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.