diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index b9261e5..65fcaa0 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -239,6 +239,31 @@ test('comparing two secondary schools renders the secondary sections', async ({ await expect(page.getByText(/No primary schools in your comparison/)).toHaveCount(0); }); +test('opening a different compare link after a previous comparison still renders', async ({ page }) => { + // Regression: the first visit stores a basket in localStorage; opening a + // link for a DIFFERENT school set then raced a stale fetch for the stored + // basket against the new SSR data, blanking every section (including the + // trends chart) until a hard refresh. + const [s0, s1] = await twoSecondaryUrns(page); + const [p0, p1] = await twoPrimaryUrns(page); + + await page.goto(`/compare?urns=${s0},${s1}`); + await expect(page.getByRole('heading', { name: 'At a glance' }).first()).toBeVisible({ + timeout: 15_000, + }); + + await page.goto(`/compare?urns=${p0},${p1}`); + await expect(page.getByRole('heading', { name: 'At a glance' }).first()).toBeVisible({ + timeout: 15_000, + }); + // Give any straggling stale response time to land, then confirm the new + // comparison is still on screen. + await page.waitForTimeout(1500); + await expect(page.getByRole('heading', { name: 'At a glance' }).first()).toBeVisible(); + await expect(page.getByRole('heading', { name: 'Explore trends' }).first()).toBeVisible(); + await expect(page.locator(`a[href*="${p0}"]`).first()).toBeVisible(); +}); + test('compare chart on mobile shows school chips with tap-to-focus', async ({ page }) => { await page.setViewportSize({ width: 390, height: 844 }); diff --git a/nextjs-app/__tests__/components/ComparisonView.staleFetch.test.tsx b/nextjs-app/__tests__/components/ComparisonView.staleFetch.test.tsx new file mode 100644 index 0000000..08262e2 --- /dev/null +++ b/nextjs-app/__tests__/components/ComparisonView.staleFetch.test.tsx @@ -0,0 +1,107 @@ +/** + * Regression: opening a compare link while a DIFFERENT basket is stored must + * not blank the page. + * + * The basket hydrates from localStorage first, which can fire a fetch for the + * OLD school set; the URL-seed effect then replaces the basket with the URL's + * schools (already covered by SSR data, so no new fetch). When the stale + * response for the old set finally lands, it must not clobber the fresh SSR + * data — that left every section (including the trends chart) empty until a + * hard refresh. + */ + +import { act, 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, + }; +} + +// The visitor's previously stored basket (a different school entirely). +const STORED_SCHOOL = school(900, 'Old Stored School'); + +// The comparison the URL (and SSR) actually asked for. +const URL_DATA = { + '100': data(100, 'Alpha Primary'), + '200': data(200, 'Beta Primary'), +}; + +beforeEach(() => { + fetchComparison.mockReset(); + localStorage.clear(); +}); + +test('a stale fetch for the previously stored basket does not clobber the URL comparison', async () => { + localStorage.setItem('selectedSchools', JSON.stringify([STORED_SCHOOL])); + + const pending: Array<(v: unknown) => void> = []; + fetchComparison.mockImplementation(() => new Promise((resolve) => pending.push(resolve))); + + render( + + + , + ); + + // The URL's schools render from SSR data once the basket is reseeded. + await waitFor(() => { + expect(screen.getByRole('heading', { name: 'At a glance' })).toBeInTheDocument(); + }); + expect(screen.getAllByText('Alpha Primary').length).toBeGreaterThan(0); + + // The transient stored-basket fetch (for school 900) resolves LATE, after + // the basket has moved on to the URL's schools. + await act(async () => { + for (const resolve of pending) { + resolve({ + comparison: { '900': data(900, 'Old Stored School') }, + national_averages: { year: 202425, primary: {}, secondary: {}, by_year: [] }, + benchmarks: undefined, + }); + } + }); + + // The page must still show the URL comparison — not go blank. + expect(screen.getByRole('heading', { name: 'At a glance' })).toBeInTheDocument(); + expect(screen.getAllByText('Alpha Primary').length).toBeGreaterThan(0); +}); diff --git a/nextjs-app/components/ComparisonView.tsx b/nextjs-app/components/ComparisonView.tsx index e1e2c0e..5f5b163 100644 --- a/nextjs-app/components/ComparisonView.tsx +++ b/nextjs-app/components/ComparisonView.tsx @@ -85,7 +85,9 @@ export function ComparisonView({ replaceSchools(urlSchools); } } - }, [isInitialized]); // eslint-disable-line react-hooks/exhaustive-deps + // Re-seed when a client-side navigation lands on a different ?urns= set + // (initialUrns/initialData are new props on the same component instance). + }, [isInitialized, initialUrns.join(',')]); // eslint-disable-line react-hooks/exhaustive-deps const urnKey = selectedSchools.map((s) => s.urn).join(','); @@ -128,9 +130,18 @@ export function ComparisonView({ const covered = urnKey.split(',').every((urn) => have[urn] != null); if (covered) return; + // Guard against out-of-order responses: while the basket hydrates from + // localStorage it can transiently hold a DIFFERENT school set than the + // URL, firing a fetch for schools the user is no longer comparing. That + // stale response must not replace data for the current set — replacing + // it blanked every section until a hard refresh. We (a) drop responses + // from superseded effect runs and (b) merge rather than replace, so data + // for the current schools always survives. + let cancelled = false; fetchComparison(urnKey, { cache: 'no-store' }) .then((data) => { - setComparisonData(data.comparison); + if (cancelled) return; + setComparisonData((prev) => ({ ...(prev ?? {}), ...data.comparison })); setNationalAverages(data.national_averages); setBenchmarks(data.benchmarks); }) @@ -140,6 +151,9 @@ export function ComparisonView({ // destroy a working comparison the user is looking at. console.error('Failed to fetch comparison:', err); }); + return () => { + cancelled = true; + }; }, [urnKey, isInitialized]); const primarySchools = selectedSchools.filter((school) => {