From c85b57f724264434d00ce01d62d0a4e5148c22ee Mon Sep 17 00:00:00 2001 From: tanmaysachann Date: Sat, 13 Jun 2026 23:00:10 +0530 Subject: [PATCH 1/4] fix(person-images): prevent flash of previous page's images (#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. --- .../src/pages/PersonImages/PersonImages.tsx | 20 +++- .../src/pages/__tests__/PersonImages.test.tsx | 102 ++++++++++++++++++ 2 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 frontend/src/pages/__tests__/PersonImages.test.tsx diff --git a/frontend/src/pages/PersonImages/PersonImages.tsx b/frontend/src/pages/PersonImages/PersonImages.tsx index ea646b8ad..4dc128934 100644 --- a/frontend/src/pages/PersonImages/PersonImages.tsx +++ b/frontend/src/pages/PersonImages/PersonImages.tsx @@ -22,6 +22,11 @@ export const PersonImages = () => { const images = useSelector(selectImages); const [clusterName, setClusterName] = useState('random_name'); const [isEditing, setIsEditing] = useState(false); + // Which cluster the shared `images` slice currently holds. Keyed on clusterId + // (not `isLoading`) so it also covers person A -> person B when B is cached. + const [loadedClusterId, setLoadedClusterId] = useState( + undefined, + ); const { data, isLoading, isSuccess, isError } = usePictoQuery({ queryKey: ['person-images', clusterId], queryFn: async () => fetchClusterImages({ clusterId: clusterId || '' }), @@ -42,9 +47,18 @@ export const PersonImages = () => { const images = (res?.images || []) as Image[]; dispatch(setImages(images)); setClusterName(res?.cluster_name || 'random_name'); + setLoadedClusterId(clusterId); dispatch(hideLoader()); } - }, [data, isSuccess, isError, isLoading, dispatch]); + }, [data, isSuccess, isError, isLoading, dispatch, clusterId]); + + // 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. + const personImages = + loadedClusterId === clusterId + ? images + : (((data?.data as any)?.images as Image[]) ?? []); const handleEditName = () => { setClusterName(clusterName); @@ -109,7 +123,7 @@ export const PersonImages = () => {

{clusterName}

- {images.map((image, index) => ( + {personImages.map((image, index) => ( {
{/* Media Viewer Modal */} - {isImageViewOpen && } + {isImageViewOpen && } ); }; diff --git a/frontend/src/pages/__tests__/PersonImages.test.tsx b/frontend/src/pages/__tests__/PersonImages.test.tsx new file mode 100644 index 000000000..f1370a639 --- /dev/null +++ b/frontend/src/pages/__tests__/PersonImages.test.tsx @@ -0,0 +1,102 @@ +import { render, screen, waitFor } from '@/test-utils'; +import { Routes, Route } from 'react-router'; +import { PersonImages } from '@/pages/PersonImages/PersonImages'; +import type { Image } from '@/types/Media'; +import * as apiFunctions from '@/api/api-functions'; + +// ImageCard resolves thumbnails through Tauri's convertFileSrc; return the path +// unchanged so we can assert which person's images are in the DOM by their src. +jest.mock('@tauri-apps/api/core', () => ({ + invoke: jest.fn().mockResolvedValue(null), + convertFileSrc: (path: string) => path, +})); + +jest.mock('@/api/api-functions', () => ({ + fetchClusterImages: jest.fn(), + renameCluster: jest.fn(), +})); + +const fetchClusterImages = apiFunctions.fetchClusterImages as jest.Mock; + +const makeImage = (id: string, path: string): Image => ({ + id, + path, + thumbnailPath: path, + folder_id: 'folder', + isTagged: true, + isFavourite: false, + tags: [], +}); + +// "Person A" stands in for whatever the previous page (e.g. the Home gallery) +// left in the shared `images` slice; "Person B" is the cluster we navigate to. +const personAImages = [ + makeImage('a1', '/personA/1.jpg'), + makeImage('a2', '/personA/2.jpg'), +]; +const personBImages = [ + makeImage('b1', '/personB/1.jpg'), + makeImage('b2', '/personB/2.jpg'), +]; + +const renderPerson = (clusterId: string) => + render( + + } /> + , + { + // Pre-seed the shared slice with the previous page's images. This is the + // exact condition that produced the flash described in issue #1315. + preloadedState: { + images: { images: personAImages, currentViewIndex: -1 }, + }, + initialRoutes: [`/person/${clusterId}`], + }, + ); + +const imageSrcs = (container: HTMLElement) => + Array.from(container.querySelectorAll('img')).map((img) => + img.getAttribute('src'), + ); + +const hasPersonA = (container: HTMLElement) => + imageSrcs(container).some((src) => src?.startsWith('/personA/')); +const hasPersonB = (container: HTMLElement) => + imageSrcs(container).some((src) => src?.startsWith('/personB/')); + +beforeEach(() => { + fetchClusterImages.mockReset(); + fetchClusterImages.mockImplementation(async ({ clusterId }) => ({ + success: true, + data: { + images: clusterId === 'B' ? personBImages : personAImages, + cluster_name: clusterId === 'B' ? 'Bob' : 'Alice', + }, + })); +}); + +describe('PersonImages (issue #1315: no flash of the previous page)', () => { + test('never paints the stale slice images, on the first frame or after', async () => { + const { container } = renderPerson('B'); + + // First frame: before the cluster query resolves, the shared slice still + // holds Person A's images. The grid must not paint them, otherwise the user + // sees the flash. (This assertion fails against the pre-fix code.) + expect(hasPersonA(container)).toBe(false); + + // Let the query resolve, then confirm Person A never appears at any point. + await waitFor(() => { + expect(hasPersonB(container)).toBe(true); + }); + expect(hasPersonA(container)).toBe(false); + }); + + test("renders the navigated cluster's images and name once loaded", async () => { + const { container } = renderPerson('B'); + + expect(await screen.findByText('Bob')).toBeInTheDocument(); + await waitFor(() => { + expect(hasPersonB(container)).toBe(true); + }); + }); +}); From 1eebcba6f9ff55e6b4f84b2875bc3dd8dea89994 Mon Sep 17 00:00:00 2001 From: tanmaysachann Date: Sun, 14 Jun 2026 00:23:14 +0530 Subject: [PATCH 2/4] fix(person-images): guard undefined clusterId and drop the any cast 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. --- frontend/src/pages/PersonImages/PersonImages.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/pages/PersonImages/PersonImages.tsx b/frontend/src/pages/PersonImages/PersonImages.tsx index 4dc128934..4484f9be0 100644 --- a/frontend/src/pages/PersonImages/PersonImages.tsx +++ b/frontend/src/pages/PersonImages/PersonImages.tsx @@ -56,9 +56,9 @@ export const PersonImages = () => { // 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. const personImages = - loadedClusterId === clusterId + clusterId !== undefined && loadedClusterId === clusterId ? images - : (((data?.data as any)?.images as Image[]) ?? []); + : ((data?.data as { images?: Image[] })?.images ?? []); const handleEditName = () => { setClusterName(clusterName); From 88187a7c0f38be4d775053bcc392133969247e08 Mon Sep 17 00:00:00 2001 From: tanmaysachann Date: Mon, 15 Jun 2026 20:25:05 +0530 Subject: [PATCH 3/4] refactor: clean up verbose comments --- frontend/src/pages/PersonImages/PersonImages.tsx | 7 ++----- frontend/src/pages/__tests__/PersonImages.test.tsx | 9 --------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/frontend/src/pages/PersonImages/PersonImages.tsx b/frontend/src/pages/PersonImages/PersonImages.tsx index 4484f9be0..68d4d8c99 100644 --- a/frontend/src/pages/PersonImages/PersonImages.tsx +++ b/frontend/src/pages/PersonImages/PersonImages.tsx @@ -22,8 +22,7 @@ export const PersonImages = () => { const images = useSelector(selectImages); const [clusterName, setClusterName] = useState('random_name'); const [isEditing, setIsEditing] = useState(false); - // Which cluster the shared `images` slice currently holds. Keyed on clusterId - // (not `isLoading`) so it also covers person A -> person B when B is cached. + const [loadedClusterId, setLoadedClusterId] = useState( undefined, ); @@ -52,9 +51,7 @@ export const PersonImages = () => { } }, [data, isSuccess, isError, isLoading, dispatch, clusterId]); - // 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. + // Fallback to query data until Redux slice syncs for this cluster (#1315) const personImages = clusterId !== undefined && loadedClusterId === clusterId ? images diff --git a/frontend/src/pages/__tests__/PersonImages.test.tsx b/frontend/src/pages/__tests__/PersonImages.test.tsx index f1370a639..0da5d1a54 100644 --- a/frontend/src/pages/__tests__/PersonImages.test.tsx +++ b/frontend/src/pages/__tests__/PersonImages.test.tsx @@ -4,8 +4,6 @@ import { PersonImages } from '@/pages/PersonImages/PersonImages'; import type { Image } from '@/types/Media'; import * as apiFunctions from '@/api/api-functions'; -// ImageCard resolves thumbnails through Tauri's convertFileSrc; return the path -// unchanged so we can assert which person's images are in the DOM by their src. jest.mock('@tauri-apps/api/core', () => ({ invoke: jest.fn().mockResolvedValue(null), convertFileSrc: (path: string) => path, @@ -28,8 +26,6 @@ const makeImage = (id: string, path: string): Image => ({ tags: [], }); -// "Person A" stands in for whatever the previous page (e.g. the Home gallery) -// left in the shared `images` slice; "Person B" is the cluster we navigate to. const personAImages = [ makeImage('a1', '/personA/1.jpg'), makeImage('a2', '/personA/2.jpg'), @@ -45,8 +41,6 @@ const renderPerson = (clusterId: string) => } /> , { - // Pre-seed the shared slice with the previous page's images. This is the - // exact condition that produced the flash described in issue #1315. preloadedState: { images: { images: personAImages, currentViewIndex: -1 }, }, @@ -79,9 +73,6 @@ describe('PersonImages (issue #1315: no flash of the previous page)', () => { test('never paints the stale slice images, on the first frame or after', async () => { const { container } = renderPerson('B'); - // First frame: before the cluster query resolves, the shared slice still - // holds Person A's images. The grid must not paint them, otherwise the user - // sees the flash. (This assertion fails against the pre-fix code.) expect(hasPersonA(container)).toBe(false); // Let the query resolve, then confirm Person A never appears at any point. From 0fe7282d575526534229e0e5e5d2d33231b5195c Mon Sep 17 00:00:00 2001 From: tanmaysachann Date: Tue, 16 Jun 2026 02:07:34 +0530 Subject: [PATCH 4/4] fix(person-images): prevent stale heading flash --- .../src/pages/PersonImages/PersonImages.tsx | 9 +- .../src/pages/__tests__/PersonImages.test.tsx | 93 +++++++++++++++++-- 2 files changed, 90 insertions(+), 12 deletions(-) diff --git a/frontend/src/pages/PersonImages/PersonImages.tsx b/frontend/src/pages/PersonImages/PersonImages.tsx index 68d4d8c99..d03698028 100644 --- a/frontend/src/pages/PersonImages/PersonImages.tsx +++ b/frontend/src/pages/PersonImages/PersonImages.tsx @@ -22,7 +22,7 @@ export const PersonImages = () => { const images = useSelector(selectImages); const [clusterName, setClusterName] = useState('random_name'); const [isEditing, setIsEditing] = useState(false); - + const [loadedClusterId, setLoadedClusterId] = useState( undefined, ); @@ -56,6 +56,11 @@ export const PersonImages = () => { clusterId !== undefined && loadedClusterId === clusterId ? images : ((data?.data as { images?: Image[] })?.images ?? []); + const displayName = + clusterId !== undefined && loadedClusterId === clusterId + ? clusterName + : ((data?.data as { cluster_name?: string })?.cluster_name ?? + 'random_name'); const handleEditName = () => { setClusterName(clusterName); @@ -118,7 +123,7 @@ export const PersonImages = () => { )} -

{clusterName}

+

{displayName}

{personImages.map((image, index) => ( +const CommitProbe = ({ + onCommit, +}: { + onCommit?: (headingText: string | null) => void; +}) => { + useLayoutEffect(() => { + onCommit?.(document.querySelector('h1')?.textContent ?? null); + }); + + return null; +}; + +const PersonImagesWithNavigation = ({ + onCommit, +}: { + onCommit?: (headingText: string | null) => void; +}) => { + const navigate = useNavigate(); + + return ( + <> + + + + + ); +}; + +const renderPerson = ( + clusterId: string, + options: { + withNavigation?: boolean; + onCommit?: (headingText: string | null) => void; + } = {}, +) => render( - } /> + + ) : ( + + ) + } + /> , { preloadedState: { @@ -70,21 +116,48 @@ beforeEach(() => { }); describe('PersonImages (issue #1315: no flash of the previous page)', () => { - test('never paints the stale slice images, on the first frame or after', async () => { - const { container } = renderPerson('B'); - - expect(hasPersonA(container)).toBe(false); + test('never paints stale images or heading when navigating to a cached person', async () => { + const committedHeadings: Array = []; + const { container, queryClient } = renderPerson('A', { + withNavigation: true, + onCommit: (headingText) => committedHeadings.push(headingText), + }); - // Let the query resolve, then confirm Person A never appears at any point. await waitFor(() => { - expect(hasPersonB(container)).toBe(true); + expect( + screen.getByRole('heading', { name: 'Alice' }), + ).toBeInTheDocument(); + expect(hasPersonA(container)).toBe(true); + }); + + queryClient.setQueryData(['person-images', 'B'], { + success: true, + data: { + images: personBImages, + cluster_name: 'Bob', + }, }); + + committedHeadings.length = 0; + fireEvent.click(screen.getByRole('button', { name: 'Go to Person B' })); + + expect(committedHeadings).not.toContain('Alice'); + expect( + screen.queryByRole('heading', { name: 'Alice' }), + ).not.toBeInTheDocument(); + expect(screen.getByRole('heading', { name: 'Bob' })).toBeInTheDocument(); expect(hasPersonA(container)).toBe(false); + expect(hasPersonB(container)).toBe(true); }); test("renders the navigated cluster's images and name once loaded", async () => { const { container } = renderPerson('B'); + expect( + screen.queryByRole('heading', { name: 'Alice' }), + ).not.toBeInTheDocument(); + expect(hasPersonA(container)).toBe(false); + expect(await screen.findByText('Bob')).toBeInTheDocument(); await waitFor(() => { expect(hasPersonB(container)).toBe(true);