fix(person-images): prevent flash of previous page's images (#1315)#1321
Conversation
…rg#1315) Scope the grid to the active cluster so the shared `images` slice left by the previous page can no longer paint before the person's photos load. Track which cluster the slice holds via `loadedClusterId`; until it syncs, render from the `['person-images', clusterId]` query data, which is always scoped to the active id. Reading the slice once synced keeps the optimistic favourite toggle working. Adds a regression test that fails when the previous page's images render.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPersonImages now prevents stale image flash during cluster navigation by tracking which cluster's images are loaded in Redux via ChangesPrevent stale images when navigating between person clusters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
bug
![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
🧹 Nitpick comments (1)
frontend/src/pages/PersonImages/PersonImages.tsx (1)
64-65: 💤 Low valueRemove redundant assignment.
setClusterName(clusterName)sets the state to its current value, which is redundant.♻️ Proposed simplification
const handleEditName = () => { - setClusterName(clusterName); setIsEditing(true); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/PersonImages/PersonImages.tsx` around lines 64 - 65, Remove the redundant state call setClusterName(clusterName) inside the handler and only call setIsEditing(true); if the intent was to update the cluster name to a different value, replace the current argument with the new value (e.g., setClusterName(newValue))—otherwise delete the setClusterName(clusterName) line so only setIsEditing(true) remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/pages/PersonImages/PersonImages.tsx`:
- Around line 55-61: The code uses a triple-cast to any when reading images from
data; replace that with an explicit BackendRes<{ images: Image[] }> typed
access. Change the expression for personImages to use (data as BackendRes<{
images: Image[] }> | undefined)?.data?.images ?? [] so you avoid "any" and
preserve types for images, leaving loadedClusterId, clusterId and images logic
unchanged; add an import or reference to BackendRes and Image if not already
present.
- Line 50: The component is calling setLoadedClusterId with clusterId from
useParams which can be undefined; update the logic in PersonImages (where
clusterId and setLoadedClusterId are used) to guard against undefined by
returning early if clusterId is undefined (or at least do not call
setLoadedClusterId when clusterId is undefined) so the derived check that picks
between Redux images and query data doesn't mistakenly treat a stale
loadedClusterId; ensure the early-return or conditional prevents running the
downstream image-selection logic when clusterId is missing.
---
Nitpick comments:
In `@frontend/src/pages/PersonImages/PersonImages.tsx`:
- Around line 64-65: Remove the redundant state call setClusterName(clusterName)
inside the handler and only call setIsEditing(true); if the intent was to update
the cluster name to a different value, replace the current argument with the new
value (e.g., setClusterName(newValue))—otherwise delete the
setClusterName(clusterName) line so only setIsEditing(true) remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6bba0b1-9ef7-4259-8319-f80129c377e5
📒 Files selected for processing (2)
frontend/src/pages/PersonImages/PersonImages.tsxfrontend/src/pages/__tests__/PersonImages.test.tsx
Addresses CodeRabbit review on the PR: only treat the slice as synced when clusterId is defined, and type the query-data fallback explicitly instead of casting through any.
|
Re: the nitpick about the redundant |
There was a problem hiding this comment.
Pull request overview
This PR fixes a UI flash in the PersonImages page where images from the previously visited page could briefly render while the target person’s cluster images are still loading (due to a shared images Redux slice being reused across pages).
Changes:
- Track which
clusterIdthe sharedimagesslice currently represents (loadedClusterId) and only render from the slice once it matches the active route param. - While the slice is still “out of sync”, render images from the cluster-scoped React Query response (
['person-images', clusterId]) to prevent stale images from ever painting. - Add a regression test that pre-seeds the shared slice with previous-page images and asserts they never appear when navigating to a different cluster.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
frontend/src/pages/PersonImages/PersonImages.tsx |
Adds cluster-scoping logic to prevent stale Redux-slice images from rendering during navigation/loading. |
frontend/src/pages/__tests__/PersonImages.test.tsx |
Adds a regression test to ensure previous-page images never paint when opening a person cluster. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -109,7 +123,7 @@ export const PersonImages = () => { | |||
| </div> | |||
| <h1 className="mb-6 text-2xl font-bold">{clusterName}</h1> | |||
rohan-pandeyy
left a comment
rohan-pandeyy
left a comment
There was a problem hiding this comment.
Please try to avoid leaving messy comments in the codebase
| // Until the slice is synced for this cluster it still holds the previous | ||
| // page's images (issue #1315). Fall back to the cluster-scoped query data for | ||
| // that window; read the slice once synced so favourite toggles still work. |
There was a problem hiding this comment.
Please try to avoid leaving messy comments in the codebase. Comments are definitely appreciated, but it’s best to keep them concise and clean. Single-line comments are preferred, and at most two lines in rare cases.
There was a problem hiding this comment.
Addressed your feedback, I've removed the verbose comments and kept only a concise single-line one. Please take a look!
|
Image gating with
Should just be the same trick you already used for const displayName = then render Also the test doesn't really catch this yet — it checks that "Bob" shows up but not that "Alice" never flashes first (and since it mounts fresh, clusterName starts as random_name anyway). A case that actually navigates A→B and asserts the old name never paints would cover it. |
|
@VanshajPoonia Updated, thanks. I gated the heading with the same |
|
LGTM 👍 Pulled the latest and went through it properly - the heading's gated with the same loadedClusterId check as the images now, so on a cached A→B jump it shows the right name from the first frame instead of the old one. Using the layout-effect probe to record the Couple of optional things, nothing blocking:
Nice work, this fixes what I flagged. |
|
Thanks for your contribution @tanmaysachann, |
|
Great experience working on this |
|
Thank you for the review and for merging! Great learning experience :) |
When a face cluster is opened from the Navbar search panel,
PersonImagesbriefly rendered the previous page's images before the person's photos loaded, producing the flash described in the issue. This PR scopes the grid to the active cluster so the stale images can never paint.Root cause: the original "redirect to
/ai-tagging" hypothesis did not match the code;PersonImageshas no such redirect (it only navigates on the user-clicked "Back to AI Tagging" button). The real cause is the sharedimagesRedux slice: the grid renders fromselectImages, which still holds whatever the previous page left there untilfetchClusterImagesresolves andsetImagesruns. For the first frame(s) that stale set is on screen. Thanks to @VanshajPoonia for independently confirming this and flagging the cached-navigation case.The fix (scoped to
PersonImages.tsx):loadedClusterIdmarker, set alongsidesetImageswhen the query succeeds.clusterId, read the slice (so the optimistic favourite toggle keeps working); during the brief window before it syncs, fall back to the['person-images', clusterId]React Query data, which is already scoped to the active id and can only ever be the correct person's photos.This does not rely on a loading flag, so it also covers person A to person B when B is already cached (
isLoadingisfalseimmediately while the slice still holds A for a tick), which was @VanshajPoonia's review point.Addressed Issues:
Fixes #1315
Screenshots/Recordings:
This is a sub-frame rendering flash, so it is covered by an automated regression test rather than a recording.
PersonImages.test.tsxpre-seeds the shared slice with a previous page's images, navigates to a different cluster, and asserts the previous images never paint (on the first frame and after the query resolves). The test fails against the pre-fix code and passes with the fix.Additional Notes:
Reading the slice once synced (rather than reading query data everywhere) is deliberate: the favourite/heart toggle updates the slice, and its
['images']cache invalidation does not match the['person-images', id]key, so reading query data everywhere would fix the flash but stop the heart icon from updating. The hybrid keeps both correct. Scope is limited toPersonImages.tsxand its test. Verified locally: full frontend suite (133 tests),tsc, and ESLint all pass.AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
have tested the code locally and I am responsible for it.
I have used the following AI models and tools: Claude Code (Anthropic Claude). I used it to investigate the root cause, draft the fix in
PersonImages.tsx, and write the regression test. I reviewed every line, verified the diagnosis against the code myself, and confirmed the full frontend test suite,tsc, and ESLint pass locally before submitting.Checklist
the project maintainers there
review otherwise.
Summary by CodeRabbit