From 43a2c4a6bc539b621f31655aec05ef319a25f343 Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 14 Jul 2026 22:34:20 +0100 Subject: [PATCH] fix(compare): show SSR data on refresh; drop dead per-page comparison fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refresh bug: on mount the basket is empty for a beat before it hydrates from the URL. The fetch effect nulled comparisonData on that transient empty urnKey, then the one-shot 'SSR covers it' skip suppressed the refetch — leaving the page blank on reload. The effect is now gated on isInitialized, never blanks on empty (the render already shows the empty state when nothing is selected), and decides fetch-vs-skip by whether it already holds each requested school's data (SSR or a prior fetch). Perf: useComparison ran a useSWR('/api/compare') whose result nothing consumed — dead weight that fired on every page (Navigation + Toast are global) whenever the basket was non-empty, and duplicated ComparisonView's own fetch on the compare page. Removed; the hook now exposes basket state only. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB --- .../ComparisonView.refresh.test.tsx | 82 +++++++++++++++++++ nextjs-app/components/ComparisonView.tsx | 39 ++++----- nextjs-app/hooks/useComparison.ts | 50 ++--------- 3 files changed, 111 insertions(+), 60 deletions(-) create mode 100644 nextjs-app/__tests__/components/ComparisonView.refresh.test.tsx diff --git a/nextjs-app/__tests__/components/ComparisonView.refresh.test.tsx b/nextjs-app/__tests__/components/ComparisonView.refresh.test.tsx new file mode 100644 index 0000000..90bdae9 --- /dev/null +++ b/nextjs-app/__tests__/components/ComparisonView.refresh.test.tsx @@ -0,0 +1,82 @@ +/** + * Regression: on refresh, the compare page must show the SSR-rendered data. + * + * The basket hydrates from the URL a beat after mount (selectedSchools is + * empty for the first render), so the fetch effect must not blank the + * SSR payload during that window — and must not refetch data the server + * already provided. + */ + +import { render, screen, waitFor } from '@testing-library/react'; + +import { ComparisonView } from '@/components/ComparisonView'; +import { ComparisonProvider } from '@/context/ComparisonProvider'; +import type { ComparisonData, School } from '@/lib/types'; + +const fetchComparison = jest.fn(); +jest.mock('@/lib/api', () => ({ + fetchComparison: (...args: unknown[]) => fetchComparison(...args), +})); +jest.mock('@/lib/analytics', () => ({ track: jest.fn() })); + +function school(urn: number, name: string): School { + return { + urn, + school_name: name, + local_authority: 'Testshire', + school_type: 'Community school', + rwm_expected_pct: 80, + phase: 'Primary', + } as School; +} + +function data(urn: number, name: string): ComparisonData { + return { + school_info: school(urn, name), + yearly_data: [{ year: 202425, rwm_expected_pct: 80 }] as ComparisonData['yearly_data'], + ofsted: null, + census: null, + admissions: null, + admissions_history: [], + deprivation: null, + }; +} + +const INITIAL_DATA = { + '100': data(100, 'Alpha Primary'), + '200': data(200, 'Beta Primary'), +}; + +beforeEach(() => { + fetchComparison.mockReset(); +}); + +test('renders SSR data on refresh without wiping it or refetching', async () => { + render( + + + , + ); + + // Both SSR-provided schools appear (data was not blanked during hydration) + await waitFor(() => { + expect(screen.getAllByText('Alpha Primary').length).toBeGreaterThan(0); + }); + expect(screen.getAllByText('Beta Primary').length).toBeGreaterThan(0); + expect(screen.getByRole('heading', { name: 'At a glance' })).toBeInTheDocument(); + + // …and the client never refetched data the server already rendered. + expect(fetchComparison).not.toHaveBeenCalled(); +}); diff --git a/nextjs-app/components/ComparisonView.tsx b/nextjs-app/components/ComparisonView.tsx index 418fb33..dc013fd 100644 --- a/nextjs-app/components/ComparisonView.tsx +++ b/nextjs-app/components/ComparisonView.tsx @@ -107,24 +107,26 @@ export function ComparisonView({ router.replace(newUrl, { scroll: false }); }, [urnKey, selectedMetric, pathname, searchParams, router]); - // Fetch only when the school set changes. The very first run is skipped - // when the SSR payload already covers the current set — no double-fetch - // of data the server just rendered. - const firstFetchRef = useRef(true); - useEffect(() => { - if (!urnKey) { - setComparisonData(null); - setNationalAverages(undefined); - setBenchmarks(undefined); - return; - } + // Fetch when the school set changes, but only for schools we don't already + // have data for. This skips the refetch of SSR-rendered data on load AND + // avoids a network call when a school is merely removed. A ref holds the + // latest data so the effect can read it without re-running on every fetch. + // + // Correctness note: we must NOT null the data on a transient empty urnKey. + // On mount the basket is empty for a beat before it hydrates from the URL, + // and blanking here (then skipping the refetch because SSR "covers" the set) + // was leaving the page empty on refresh. The render already shows the empty + // state whenever `selectedSchools` is empty, so stale data for deselected + // schools is harmless — it's simply unused. + const comparisonDataRef = useRef(comparisonData); + comparisonDataRef.current = comparisonData; - if (firstFetchRef.current) { - firstFetchRef.current = false; - const ssrUrns = new Set(Object.keys(initialData ?? {})); - const covered = urnKey.split(',').every((urn) => ssrUrns.has(urn)); - if (covered && ssrUrns.size > 0) return; - } + useEffect(() => { + if (!isInitialized || !urnKey) return; + + const have = comparisonDataRef.current ?? {}; + const covered = urnKey.split(',').every((urn) => have[urn] != null); + if (covered) return; fetchComparison(urnKey, { cache: 'no-store' }) .then((data) => { @@ -138,8 +140,7 @@ export function ComparisonView({ // destroy a working comparison the user is looking at. console.error('Failed to fetch comparison:', err); }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [urnKey]); + }, [urnKey, isInitialized]); // Classify schools by phase using comparison data const classifySchool = (school: School): 'primary' | 'secondary' => { diff --git a/nextjs-app/hooks/useComparison.ts b/nextjs-app/hooks/useComparison.ts index f7ed256..68913e8 100644 --- a/nextjs-app/hooks/useComparison.ts +++ b/nextjs-app/hooks/useComparison.ts @@ -1,50 +1,18 @@ /** - * Custom hook for managing school comparison state - * Uses shared context for real-time updates across components + * Custom hook for managing school comparison state. + * + * This hook is mounted on every page via the global Navigation and + * ComparisonToast, so it must stay cheap — it exposes basket state only. + * The compare page fetches `/api/compare` itself (ComparisonView); nothing + * ever read the comparison payload from here, so the previous per-page SWR + * fetch (which fired on every page whenever the basket was non-empty) was + * dead weight and has been removed. */ 'use client'; -import useSWR from 'swr'; -import { fetcher } from '@/lib/api'; import { useComparisonContext } from '@/context/ComparisonContext'; -import type { ComparisonResponse } from '@/lib/types'; export function useComparison() { - const { - selectedSchools, - addSchool, - removeSchool, - replaceSchools, - clearAll, - isSelected, - canAddMore, - isInitialized, - } = useComparisonContext(); - - // Fetch comparison data for selected schools - const urns = selectedSchools.map((s) => s.urn).join(','); - const { data, error, isLoading, mutate } = useSWR( - selectedSchools.length > 0 ? `/compare?urns=${urns}` : null, - fetcher, - { - revalidateOnFocus: false, - dedupingInterval: 10000, - } - ); - - return { - selectedSchools, - comparisonData: data?.comparison, - isLoading, - error, - addSchool, - removeSchool, - replaceSchools, - clearAll, - isSelected, - canAddMore, - isInitialized, - mutate, - }; + return useComparisonContext(); }