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(); }