From 5b945c308d9b3516993dfea1977ff226d7b345bf Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 10 Jul 2026 23:36:15 +0200 Subject: [PATCH] fix(spa): episode anchor nav must handle in-app popstate + not re-scroll on refetch (#220 review) Adversarial review of #220 found in-grid episode card clicks never scrolled/highlighted: navigateToPath() (routing.ts) does pushState + a synthetic popstate, not a real hash change, so the anchor effect's hashchange-only listener never fired for same-pathname navigation. Now listens to both hashchange and popstate. Also: track the last anchor value actually scrolled to so a refetch/pagination that recreates the items array (anchor unchanged) doesn't hijack scroll position; document the known CHILD_PAGE_SIZE deep-link limitation (parity with the Blazor fragment link); and fix the MediaPosterCard/shell.css comments that described the highlight ring as "temporary" when only its glow pulse fades, not the ring itself. Co-Authored-By: Claude Fable 5 --- web/src/media/MediaPosterCard.tsx | 5 +- web/src/screens/MediaDetailScreen.test.tsx | 92 ++++++++++++++++++++++ web/src/screens/MediaDetailScreen.tsx | 35 ++++++-- web/src/shell.css | 8 +- 4 files changed, 130 insertions(+), 10 deletions(-) diff --git a/web/src/media/MediaPosterCard.tsx b/web/src/media/MediaPosterCard.tsx index e00fa271b..9ae7b152a 100644 --- a/web/src/media/MediaPosterCard.tsx +++ b/web/src/media/MediaPosterCard.tsx @@ -25,7 +25,10 @@ export function MediaPosterCard({ actions?: ReactNode; // Stable DOM id (e.g. `episode-{id}`) so callers can deep-link/scroll to this card. id?: string; - // Applies a temporary visual highlight, e.g. when this card is the deep-link target. + // Applies a ring highlight while this card is the deep-link target (e.g. the current + // `#episode-{id}` hash) — persists for as long as the anchor matches this card, with only the + // ring's outer glow pulse fading shortly after mount (see .ctv-media-card-highlighted in + // shell.css). highlighted?: boolean; }) { const hue = hueOf(item.title); diff --git a/web/src/screens/MediaDetailScreen.test.tsx b/web/src/screens/MediaDetailScreen.test.tsx index e3f92bdb4..ed20d30bd 100644 --- a/web/src/screens/MediaDetailScreen.test.tsx +++ b/web/src/screens/MediaDetailScreen.test.tsx @@ -1,5 +1,6 @@ import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, describe, expect, it, vi } from 'vitest'; +import { navigateToPath } from '../routing'; import { ArtistDetailScreen, MovieDetailScreen, @@ -217,4 +218,95 @@ describe('media detail screens', () => { window.location.hash = ''; }); + + it('updates the anchor and scrolls on same-pathname in-app navigation (synthetic popstate, review #220)', async () => { + vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => { + const url = input.toString(); + if (url.startsWith('/api/library/browse')) { + return Promise.resolve( + jsonResponse({ + page: [ + { artwork: '', collectionType: 'Episode', id: 90, mediaType: 'Episode', title: 'Pilot' }, + { artwork: '', collectionType: 'Episode', id: 91, mediaType: 'Episode', title: 'Second' } + ], + totalCount: 2 + }) + ); + } + if (url === '/api/collections' || url === '/api/playlists/groups' || url === '/api/schedules') { + return Promise.resolve(jsonResponse([])); + } + return Promise.resolve(jsonResponse(season)); + }); + + const scrollIntoView = vi.fn(); + Element.prototype.scrollIntoView = scrollIntoView; + window.location.hash = ''; + + render(); + await waitFor(() => expect(screen.getByText('Second')).toBeInTheDocument()); + expect(scrollIntoView).not.toHaveBeenCalled(); + + // `routing.ts`'s navigateToPath is what App.tsx uses for in-app clicks (e.g. an episode card + // inside this same season grid): pushState + a synthetic `popstate`, not a real hash change, + // so `hashchange` alone would never fire. This exercises that same-pathname path directly. + navigateToPath('/app/media/seasons/8#episode-91'); + + await waitFor(() => expect(scrollIntoView).toHaveBeenCalledTimes(1)); + const target = document.getElementById('episode-91'); + expect(target?.className).toContain('ctv-media-card-highlighted'); + + window.location.hash = ''; + }); + + it('does not re-scroll on refetch/pagination when the anchor is unchanged (review #220)', async () => { + vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => { + const url = input.toString(); + if (url.startsWith('/api/library/browse')) { + const params = new URL(url, 'http://localhost').searchParams; + const pageNum = params.get('pageNum'); + if (pageNum === '1') { + return Promise.resolve( + jsonResponse({ + page: [{ artwork: '', collectionType: 'Episode', id: 92, mediaType: 'Episode', title: 'Third' }], + totalCount: 61 + }) + ); + } + return Promise.resolve( + jsonResponse({ + page: [ + { artwork: '', collectionType: 'Episode', id: 90, mediaType: 'Episode', title: 'Pilot' }, + { artwork: '', collectionType: 'Episode', id: 91, mediaType: 'Episode', title: 'Second' } + ], + totalCount: 61 + }) + ); + } + if (url === '/api/collections' || url === '/api/playlists/groups' || url === '/api/schedules') { + return Promise.resolve(jsonResponse([])); + } + return Promise.resolve(jsonResponse(season)); + }); + + const scrollIntoView = vi.fn(); + Element.prototype.scrollIntoView = scrollIntoView; + window.location.hash = '#episode-91'; + + render(); + await waitFor(() => expect(screen.getByText('Second')).toBeInTheDocument()); + await waitFor(() => expect(scrollIntoView).toHaveBeenCalledTimes(1)); + + // Page away (target leaves the DOM) and back (a fresh `items` array is fetched, but the + // anchor value itself never changed) — must not re-trigger the scroll. + fireEvent.click(screen.getByTitle('Next page')); + await waitFor(() => expect(screen.getByText('Third')).toBeInTheDocument()); + + fireEvent.click(screen.getByTitle('Previous page')); + await waitFor(() => expect(screen.getByText('Second')).toBeInTheDocument()); + + expect(scrollIntoView).toHaveBeenCalledTimes(1); + + window.location.hash = ''; + }); }); diff --git a/web/src/screens/MediaDetailScreen.tsx b/web/src/screens/MediaDetailScreen.tsx index f80ce857c..1e87154fc 100644 --- a/web/src/screens/MediaDetailScreen.tsx +++ b/web/src/screens/MediaDetailScreen.tsx @@ -214,6 +214,9 @@ function ChildGrid({ const [anchorId, setAnchorId] = useState(() => window.location.hash.slice(1) || null); const activeRef = useRef(true); const seqRef = useRef(0); + // The anchor value we last actually scrolled to, so a refetch/pagination that recreates + // `items` (but leaves the hash unchanged) doesn't hijack the user's scroll position. + const scrolledAnchorRef = useRef(null); useEffect(() => { activeRef.current = true; @@ -223,20 +226,40 @@ function ChildGrid({ }, []); // Deep-link support (#220): a season detail's episode grid can be opened with an - // `#episode-{id}` hash (from search/browse cards, or Blazor-parity links). Track hash changes - // so re-navigating to the same season with a different anchor still scrolls/highlights. + // `#episode-{id}` hash (from search/browse cards, or Blazor-parity links). The SPA's own + // in-app navigation (routing.ts navigateToPath) uses `history.pushState` + a synthetic + // `popstate` dispatch rather than a real hash change — real browser hash navigation + // (address bar, back/forward across a hash-only change) fires `hashchange` instead — so both + // events must be handled to catch in-grid episode card clicks (same season pathname, new + // fragment) as well as deep links that remount this screen. useEffect(() => { - const onHashChange = () => setAnchorId(window.location.hash.slice(1) || null); - window.addEventListener('hashchange', onHashChange); - return () => window.removeEventListener('hashchange', onHashChange); + const onHashNav = () => setAnchorId(window.location.hash.slice(1) || null); + window.addEventListener('hashchange', onHashNav); + window.addEventListener('popstate', onHashNav); + return () => { + window.removeEventListener('hashchange', onHashNav); + window.removeEventListener('popstate', onHashNav); + }; }, []); useEffect(() => { if (status !== 'success' || mediaType !== 'Episode' || !anchorId) { return; } + // One-shot per anchor value: only scroll the first time we see this anchor resolve + // successfully, so a later refetch/pagination (which recreates `items`) doesn't re-jump the + // page back to it. Known limitation shared with the Blazor fragment link this replaces: the + // anchor only resolves against the currently-loaded page (CHILD_PAGE_SIZE) — a target beyond + // page 1 won't be found (and so won't scroll) until that page is loaded. + if (scrolledAnchorRef.current === anchorId) { + return; + } const target = document.getElementById(anchorId); - target?.scrollIntoView({ behavior: 'smooth', block: 'center' }); + if (!target) { + return; + } + target.scrollIntoView({ behavior: 'smooth', block: 'center' }); + scrolledAnchorRef.current = anchorId; }, [status, mediaType, anchorId, items]); const load = useCallback(() => { diff --git a/web/src/shell.css b/web/src/shell.css index 7a48e3ae2..f1c624fe0 100644 --- a/web/src/shell.css +++ b/web/src/shell.css @@ -2529,14 +2529,16 @@ body { box-shadow: 0 0 0 1px var(--action-primary); } -/* Deep-link target highlight (#220), e.g. `#episode-{id}` from search/browse episode cards. */ +/* Deep-link target highlight (#220), e.g. `#episode-{id}` from search/browse episode cards. + The ring itself (border + 2px box-shadow) persists for as long as the hash names this card; + only the outer glow pulse fades out shortly after mount. */ .ctv-media-card-highlighted { border-color: var(--action-primary); box-shadow: 0 0 0 2px var(--action-primary); - animation: ctv-media-card-highlight-fade 2400ms ease-out 1; + animation: ctv-media-card-highlight-pulse 2400ms ease-out 1; } -@keyframes ctv-media-card-highlight-fade { +@keyframes ctv-media-card-highlight-pulse { 0% { box-shadow: 0 0 0 4px var(--action-primary); }