fix: Redux State Isolation - Prevent Gallery Pollution from Cluster View#761
fix: Redux State Isolation - Prevent Gallery Pollution from Cluster View#761Jitmisra wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
…iew (AOSSIE-Org#706) Problem: - Navigating to person/cluster view overwrites global image state - Returning to Home shows only cluster images instead of full gallery - Required hard refresh to restore gallery Solution: - Add 'source' field to imageSlice to track where images came from - Add setGalleryImages, setClusterImages, setSearchImages actions - Home page detects stale data (source !== 'gallery') and auto-refetches - PersonImages uses setClusterImages to mark source as 'cluster' Benefits over alternative approaches: - No duplicate state (single source of truth) - Minimal code changes (4 files modified) - Extensible for future views (albums, tags, etc.) - Auto-recovery without manual cleanup Tests: 23 unit tests added and passing Closes AOSSIE-Org#706
github-actions
bot
added
the
needs-triage
WalkthroughThe PR introduces image source tracking to Redux state via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 3
🧹 Nitpick comments (5)
frontend/src/features/imageSelectors.ts (1)
3-15: Use a type-only import forImageSourceto avoid runtime import coupling.In TS, prefer:
-import { ImageSource } from './imageSlice'; +import type { ImageSource } from './imageSlice';frontend/src/pages/PersonImages/PersonImages.tsx (1)
41-47: Avoidany+ avoid shadowingimagesin the success branch.Right now
const res: anyandconst images = ...(Line 41-42) makes this easier to misuse. Consider:- const res: any = data?.data; - const images = (res?.images || []) as Image[]; + const res = data?.data as { images?: Image[]; cluster_name?: string } | undefined; + const clusterImages = res?.images ?? []; // Use setClusterImages to mark source as 'cluster' // This allows Home page to detect stale data and refetch - dispatch(setClusterImages(images)); + dispatch(setClusterImages(clusterImages));frontend/src/features/__tests__/imageSlice.test.ts (1)
129-140: Prefer asserting viaselectNeedsGalleryRefresh(avoids duplicating logic in tests).This test hardcodes
source !== 'gallery'; it’ll drift if the selector changes (e.g.,nullhandling). Consider importing and asserting the selector instead.frontend/src/features/__tests__/imageSelectors.test.ts (1)
11-24: Tighten mock typing: avoidany[]increateMockState.If feasible, use
Image[](or minimal{id: string}[]) to keep selector tests from accepting invalid shapes silently.frontend/src/features/imageSlice.ts (1)
8-8: Minor:ImageSourceincludes'album'but there’s nosetAlbumImagesyet.Either add
setAlbumImagesfor completeness, or remove'album'until it’s supported (to avoid “dead” source values showing up viaselectNeedsGalleryRefresh).Also applies to: 47-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/features/__tests__/imageSelectors.test.ts(1 hunks)frontend/src/features/__tests__/imageSlice.test.ts(1 hunks)frontend/src/features/imageSelectors.ts(2 hunks)frontend/src/features/imageSlice.ts(2 hunks)frontend/src/pages/Home/Home.tsx(3 hunks)frontend/src/pages/PersonImages/PersonImages.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/features/__tests__/imageSelectors.test.ts (3)
frontend/src/features/imageSlice.ts (1)
ImageSource(8-8)frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/features/imageSelectors.ts (5)
selectImages(6-8)selectCurrentViewIndex(10-11)selectImageSource(13-14)selectIsImageViewOpen(17-20)selectNeedsGalleryRefresh(26-29)
frontend/src/features/imageSlice.ts (1)
frontend/src/types/Media.ts (1)
Image(13-23)
frontend/src/features/imageSelectors.ts (2)
frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/features/imageSlice.ts (1)
ImageSource(8-8)
frontend/src/pages/PersonImages/PersonImages.tsx (1)
frontend/src/features/imageSlice.ts (1)
setClusterImages(42-45)
🔇 Additional comments (5)
frontend/src/pages/PersonImages/PersonImages.tsx (1)
6-6: Correct action for isolation:setClusterImagesinstead of overwriting gallery state.frontend/src/features/__tests__/imageSlice.test.ts (1)
25-79: Solid reducer coverage for newsourcefield + actions.frontend/src/pages/Home/Home.tsx (1)
9-13:setGalleryImageswiring looks correct and matches the new source-tracking model.Also applies to: 54-59
frontend/src/features/__tests__/imageSelectors.test.ts (1)
25-120: Selector test coverage is comprehensive for the new “source/refresh” behavior.frontend/src/features/imageSlice.ts (1)
4-21: Source-tracking model is a clean, minimal fix for state pollution.Also applies to: 35-52
| /** | ||
| * Returns true if the current images are NOT from the gallery | ||
| * Used to trigger refetch when returning to Home page | ||
| */ | ||
| export const selectNeedsGalleryRefresh = createSelector( | ||
| [selectImageSource], | ||
| (source) => source !== null && source !== 'gallery', | ||
| ); |
There was a problem hiding this comment.
Selector logic is good; docstring doesn’t match behavior (“cluster/search” vs includes “album”).
selectNeedsGalleryRefresh returns true for any non-null non-gallery source (including 'album'). Consider updating the comment to match the implementation (or constrain the logic if album shouldn’t trigger refresh).
🤖 Prompt for AI Agents
In frontend/src/features/imageSelectors.ts around lines 22 to 29, the docstring
claims the selector returns true only when images are NOT from the gallery
("cluster/search"), but the implementation returns true for any non-null,
non-'gallery' source (including 'album'); either update the comment to describe
that it triggers refresh for any non-gallery source (e.g., cluster, search,
album) or change the predicate to explicitly exclude album as well (e.g., source
!== null && source !== 'gallery' && source !== 'album') depending on intended
behavior so the comment and logic are consistent.
| /** | ||
| * Sets images with source tracking | ||
| * @deprecated Use setGalleryImages or setClusterImages instead for clarity | ||
| */ | ||
| setImages(state, action: PayloadAction<Image[]>) { | ||
| state.images = action.payload; | ||
| }, | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the imageSlice.ts file to understand the full context
cat -n frontend/src/features/imageSlice.ts | head -80Repository: AOSSIE-Org/PictoPy
Length of output: 2688
🏁 Script executed:
# Search for setImages usage in the codebase
rg -n --type=ts --type=tsx "\bsetImages\s*\(" frontend/src -S -A 2 -B 2Repository: AOSSIE-Org/PictoPy
Length of output: 89
🏁 Script executed:
# Search for dispatch of setImages
rg -n --type=ts --type=tsx "dispatch\(\s*setImages" frontend/src -S -A 1 -B 1Repository: AOSSIE-Org/PictoPy
Length of output: 89
🏁 Script executed:
# Look for how 'source' field is used in the state and refresh detection
rg -n --type=ts --type=tsx "\bsource\b" frontend/src/features/imageSlice.ts -A 2 -B 2Repository: AOSSIE-Org/PictoPy
Length of output: 89
🏁 Script executed:
# Search for setImages usage with corrected flags
rg -n "setImages\s*\(" frontend/src -A 2 -B 2Repository: AOSSIE-Org/PictoPy
Length of output: 3309
🏁 Script executed:
# Search for dispatch of setImages
rg -n "dispatch\(\s*setImages" frontend/src -A 1 -B 1Repository: AOSSIE-Org/PictoPy
Length of output: 1274
🏁 Script executed:
# Look for how 'source' field is used in refresh detection
rg -n "source.*gallery\|gallery.*source" frontend/src -A 2 -B 2Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Check Home component for refresh detection logic
fd Home frontend/src -type fRepository: AOSSIE-Org/PictoPy
Length of output: 232
🏁 Script executed:
# Find Home component
fd -e tsx -e ts Home frontend/srcRepository: AOSSIE-Org/PictoPy
Length of output: 94
🏁 Script executed:
# Look at the Home component(s)
rg -n "source\s*===.*gallery\|gallery.*source" frontend/src -A 3 -B 3Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Check the context of each setImages call to understand what they should be
cat -n frontend/src/pages/Home/MyFav.tsx | head -60Repository: AOSSIE-Org/PictoPy
Length of output: 2574
🏁 Script executed:
# Check AITagging context
cat -n frontend/src/pages/AITagging/AITagging.tsx | head -50Repository: AOSSIE-Org/PictoPy
Length of output: 2287
🏁 Script executed:
# Check WebCam context
cat -n frontend/src/components/WebCam/WebCamComponent.tsx | head -65Repository: AOSSIE-Org/PictoPy
Length of output: 2624
🏁 Script executed:
# Look at Home.tsx for any refresh/source-related logic
cat -n frontend/src/pages/Home/Home.tsx | head -100Repository: AOSSIE-Org/PictoPy
Length of output: 3733
🏁 Script executed:
# Search for any refresh or skip logic based on source
rg -n "source\s*===" frontend/src -A 2 -B 2Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Check if there's any conditional logic based on source in selectors or effects
rg -n "source" frontend/src/features/imageSelectors.ts -A 2 -B 2Repository: AOSSIE-Org/PictoPy
Length of output: 344
setImages (legacy) actively pollutes source field and breaks refresh detection—must be fixed or fully replaced.
Found 5 active dispatches of setImages across MyFav, AITagging, WebCamComponent, and FaceSearchDialog. The reducer does not update source, allowing stale values to persist. This directly breaks selectNeedsGalleryRefresh (imageSelectors.ts, line 28), which relies on source to trigger gallery refetch in Home:
selectNeedsGalleryRefreshreturnssource !== null && source !== 'gallery'- If a non-gallery flow (AITagging, face search) calls
setImageswithout resettingsource, the next Home navigation may skip necessary refresh.
The test file (imageSlice.test.ts, lines 52–60) explicitly documents this behavior as intentional—setImages preserves the existing source value.
Recommended fix: Reset source in setImages to force Home to recognize the state as non-gallery:
setImages(state, action: PayloadAction<Image[]>) {
state.images = action.payload;
+ state.source = null; // unknown/legacy: forces Home refresh detection
},Alternatively, migrate all 5 call sites to use setGalleryImages, setClusterImages, or setSearchImages (depending on context) and deprecate setImages entirely.
📝 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.
| /** | |
| * Sets images with source tracking | |
| * @deprecated Use setGalleryImages or setClusterImages instead for clarity | |
| */ | |
| setImages(state, action: PayloadAction<Image[]>) { | |
| state.images = action.payload; | |
| }, | |
| /** | |
| * Sets images with source tracking | |
| * @deprecated Use setGalleryImages or setClusterImages instead for clarity | |
| */ | |
| setImages(state, action: PayloadAction<Image[]>) { | |
| state.images = action.payload; | |
| state.source = null; // unknown/legacy: forces Home refresh detection | |
| }, |
🤖 Prompt for AI Agents
In frontend/src/features/imageSlice.ts around lines 27–34, the legacy reducer
setImages only replaces images and preserves the existing source, which lets
stale source values persist and breaks selectNeedsGalleryRefresh; change
setImages to also set state.source to a clear non-gallery marker (e.g.,
state.source = 'non-gallery' or another agreed non-'gallery' string) so Home
will detect a non-gallery flow and trigger refresh, and as an alternative
follow-up migrate the five call sites to the explicit
setGalleryImages/setClusterImages/setSearchImages reducers and deprecate
setImages.
| const { data, isLoading, isSuccess, isError, error, refetch } = usePictoQuery( | ||
| { | ||
| queryKey: ['images'], | ||
| queryFn: () => fetchAllImages(), | ||
| enabled: !isSearchActive, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find routing configuration to understand if Home stays mounted across navigation
find frontend/src -type f \( -name "*.tsx" -o -name "*.ts" \) | head -20
rg -n "Routes|createBrowserRouter|<Route" frontend/src --type=ts --type=tsx -A 3 | head -50Repository: AOSSIE-Org/PictoPy
Length of output: 1004
🏁 Script executed:
#!/bin/bash
# Examine the Home.tsx file to see current implementation
wc -l frontend/src/pages/Home/Home.tsx
cat -n frontend/src/pages/Home/Home.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 3769
🏁 Script executed:
#!/bin/bash
# Find usePictoQuery hook definition
fd -e ts -e tsx | xargs rg -l "usePictoQuery"
rg -n "export.*usePictoQuery|const usePictoQuery" frontend/src --type=ts --type=tsx -B 2 -A 15Repository: AOSSIE-Org/PictoPy
Length of output: 919
🏁 Script executed:
#!/bin/bash
# Read the usePictoQuery hook implementation
cat -n frontend/src/hooks/useQueryExtension.tsRepository: AOSSIE-Org/PictoPy
Length of output: 3519
🏁 Script executed:
#!/bin/bash
# Check App.tsx for routing setup
cat -n frontend/src/App.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 1528
🏁 Script executed:
#!/bin/bash
# Find and examine AppRoutes
find frontend/src -name "*AppRoutes*" -o -name "*routes*" | grep -E "\.(tsx|ts)$"Repository: AOSSIE-Org/PictoPy
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Read the routes file
fd -e ts -e tsx "routes" frontend/src | head -5Repository: AOSSIE-Org/PictoPy
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Read AppRoutes component
cat -n frontend/src/routes/AppRoutes.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 1500
🏁 Script executed:
#!/bin/bash
# Read routes constants
cat -n frontend/src/constants/routes.tsRepository: AOSSIE-Org/PictoPy
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Find where needsRefresh is set in redux
rg -n "setNeedsGalleryRefresh|selectNeedsGalleryRefresh" frontend/src -A 3 -B 1Repository: AOSSIE-Org/PictoPy
Length of output: 5102
🏁 Script executed:
#!/antml
# Find the imageSlice to understand needsRefresh
fd -e ts -e tsx "imageSlice" frontend/srcRepository: AOSSIE-Org/PictoPy
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Read imageSlice to understand setGalleryImages and source tracking
cat -n frontend/src/features/imageSlice.ts | head -100Repository: AOSSIE-Org/PictoPy
Length of output: 2926
🏁 Script executed:
#!/antml
# Also check imageSelectors to see full implementation
cat -n frontend/src/features/imageSelectors.tsRepository: AOSSIE-Org/PictoPy
Length of output: 1102
🏁 Script executed:
#!/antml
# Check if there's any documentation or comments about React Query behavior
rg -n "queryKey\|deduplic\|staleTime\|gcTime" frontend/src/hooks/useQueryExtension.ts frontend/src/config/ -A 2Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
#!/antml
# Look for QueryClient configuration
fd -e ts -e tsx "QueryClient" frontend/src/config/Repository: AOSSIE-Org/PictoPy
Length of output: 105
🏁 Script executed:
#!/an/bash
# Read QueryClientProvider to understand React Query configuration
cat -n frontend/src/config/QueryClientProvider.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 529
Guard against redundant/double fetches: the "needsRefresh" effect can refetch on top of the initial query fetch.
When Home returns to the active route, usePictoQuery with enabled: !isSearchActive will fetch, and the useEffect can then trigger an additional refetch() while needsRefresh is still true. React Query's default deduplication window won't prevent both requests from executing.
A low-impact fix is to refetch only on a false -> true transition of needsRefresh (prevents redundant refetch when Home component remounts):
export const Home = () => {
const dispatch = useDispatch();
const images = useSelector(selectImages);
const needsRefresh = useSelector(selectNeedsGalleryRefresh);
+ const prevNeedsRefresh = useRef(needsRefresh);
const scrollableRef = useRef<HTMLDivElement>(null);
const [monthMarkers, setMonthMarkers] = useState<MonthMarker[]>([]);
const searchState = useSelector((state: RootState) => state.search);
const isSearchActive = searchState.active;
const { data, isLoading, isSuccess, isError, error, refetch } = usePictoQuery(
{
queryKey: ['images'],
queryFn: () => fetchAllImages(),
enabled: !isSearchActive,
},
);
// Refetch gallery images if returning from cluster/search view
useEffect(() => {
- if (!isSearchActive && needsRefresh) {
+ if (!isSearchActive && needsRefresh && !prevNeedsRefresh.current) {
refetch();
}
+ prevNeedsRefresh.current = needsRefresh;
}, [needsRefresh, isSearchActive, refetch]);Also applies to: 47-53
🤖 Prompt for AI Agents
In frontend/src/pages/Home/Home.tsx around lines 29-35 (and similarly at lines
47-53), the effect that calls refetch() can fire redundantly when the component
mounts because usePictoQuery already fetches; change the effect to only call
refetch when needsRefresh transitions from false to true by tracking the
previous needsRefresh value (e.g., use a ref or previous state) and performing
refetch only if prevNeedsRefresh === false and needsRefresh === true; update
both effect locations accordingly and clear/update the prev flag after refetch
so subsequent mounts won't trigger a duplicate fetch.
|
teamname bigbull |
|
The issue's author wanted to work on it. So by the project rules, we assign the issue to them first. |
2 participants
#706
fix: Redux State Isolation - Prevent Gallery Pollution from Cluster View
🎯 Resolves Issue #706
Fixes Redux state pollution where viewing a person's face cluster overwrites the global image gallery, causing incorrect Home page display after navigation.
🐛 Problem
Current Bug Behavior
Root Cause
File:
frontend/src/pages/PersonImages/PersonImages.tsxThis overwrites the global Redux
imagesSlicewhich is shared by Home page, Search, and other views.🔧 Solution: Source Tracking
Added a
sourcefield to track where images came from. Home page detects stale data and auto-refetches.Architecture
State Flow
💻 Implementation
New State Structure
New Actions
Auto-Recovery in Home
✅ Files Changed
frontend/src/features/imageSlice.tssourcefield + typed actionsfrontend/src/features/imageSelectors.tsselectNeedsGalleryRefreshfrontend/src/pages/Home/Home.tsxfrontend/src/pages/PersonImages/PersonImages.tsxsetClusterImagesfrontend/src/features/__tests__/imageSlice.test.tsfrontend/src/features/__tests__/imageSelectors.test.ts🧪 Testing
Unit Tests: 23 Passing
Manual Test Steps
📋 Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.