From 99b769ca9edfaf1ee9f5425a2c618b8bb5a73467 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 16 Jul 2026 21:27:25 +0100 Subject: [PATCH 1/3] feat(compare): fill the sticky bar's label rail with a comparison caption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'COMPARING / 3 primary schools' — 0.72rem uppercase eyebrow over a 0.95rem semibold count, sized to sit alongside the 0.92rem chip names without dominating. Desktop only; mobile pills unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB --- e2e/tests/journeys.spec.ts | 2 ++ .../components/ComparisonView.phase.test.tsx | 2 ++ .../components/ComparisonView.module.css | 33 +++++++++++++++++-- nextjs-app/components/ComparisonView.tsx | 8 +++++ 4 files changed, 42 insertions(+), 3 deletions(-) diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 7b01cf7..b9261e5 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -204,6 +204,8 @@ test('comparing two schools shows the parent-first sections side by side', async .locator('[aria-label="Schools in this comparison"]') .evaluate((el) => getComputedStyle(el).gridTemplateColumns); expect(barTemplate).toMatch(/^200px /); + // ...and its label rail carries the comparison caption. + await expect(page.getByText(/^\d+ (primary|secondary) schools?$/)).toBeVisible(); // Ofsted linkout goes to the school's provider page, never a report deep-link const ofstedLink = page.getByRole('link', { name: /Ofsted page/i }).first(); diff --git a/nextjs-app/__tests__/components/ComparisonView.phase.test.tsx b/nextjs-app/__tests__/components/ComparisonView.phase.test.tsx index 4fd5716..1bf7938 100644 --- a/nextjs-app/__tests__/components/ComparisonView.phase.test.tsx +++ b/nextjs-app/__tests__/components/ComparisonView.phase.test.tsx @@ -72,5 +72,7 @@ test('an all-secondary comparison renders the sections, not an empty primary tab }); expect(screen.getAllByText('Gamma High').length).toBeGreaterThan(0); expect(screen.queryByText(/No primary schools in your comparison/)).toBeNull(); + // The sticky bar's rail caption reflects the active phase and count. + expect(screen.getByText('2 secondary schools')).toBeInTheDocument(); expect(fetchComparison).not.toHaveBeenCalled(); }); diff --git a/nextjs-app/components/ComparisonView.module.css b/nextjs-app/components/ComparisonView.module.css index d4de534..932b132 100644 --- a/nextjs-app/components/ComparisonView.module.css +++ b/nextjs-app/components/ComparisonView.module.css @@ -148,10 +148,16 @@ text-overflow: ellipsis; } +/* Caption filling the label rail on desktop ("Comparing / 3 primary + schools"). Hidden on mobile, where the bar is a row of compact pills. */ +.barCaption { + display: none; +} + /* Desktop (matches the sections' 761px breakpoint): the bar adopts the same grid template as compareSections' .grid — a 200px row-label rail plus one column per school — so each chip sits exactly over the column it labels. - The first chip starts after the empty label rail. */ + The caption occupies the rail; chips flow into the school columns. */ @media (min-width: 761px) { .schoolBar { display: grid; @@ -164,8 +170,29 @@ min-width: 0; } - .schoolChip:first-child { - grid-column: 2; + .barCaption { + grid-column: 1; + display: flex; + flex-direction: column; + justify-content: center; + gap: 0.1rem; + padding-right: 0.5rem; + min-width: 0; + } + + .barCaptionEyebrow { + font-size: 0.72rem; + font-weight: 600; + letter-spacing: 0.06em; + text-transform: uppercase; + color: var(--text-muted, #6d685f); + } + + .barCaptionCount { + font-size: 0.95rem; + font-weight: 600; + line-height: 1.3; + color: var(--text-primary, #1a1612); } } diff --git a/nextjs-app/components/ComparisonView.tsx b/nextjs-app/components/ComparisonView.tsx index 1733a5e..e1e2c0e 100644 --- a/nextjs-app/components/ComparisonView.tsx +++ b/nextjs-app/components/ComparisonView.tsx @@ -350,6 +350,14 @@ export function ComparisonView({ style={{ '--school-count': activeSchools.length } as CSSProperties} aria-label="Schools in this comparison" > + {/* Fills the 200px label rail on desktop (hidden on mobile). */} +
+ Comparing + + {activeSchools.length} {comparePhase} school + {activeSchools.length === 1 ? '' : 's'} + +
{activeSchools.map((school, index) => (
Date: Fri, 17 Jul 2026 07:16:04 +0100 Subject: [PATCH 2/3] fix(compare): stale basket fetch no longer blanks a freshly opened comparison Opening a compare link while localStorage held a different basket raced a fetch for the OLD school set against the URL's SSR data; the stale response replaced comparisonData, so no active school had data and every section (including the trends chart) vanished until a hard refresh. Responses from superseded effect runs are now dropped, successful ones merge instead of replace, and the URL-seed effect re-runs when a client-side navigation changes ?urns=. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB --- e2e/tests/journeys.spec.ts | 25 ++++ .../ComparisonView.staleFetch.test.tsx | 107 ++++++++++++++++++ nextjs-app/components/ComparisonView.tsx | 18 ++- 3 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 nextjs-app/__tests__/components/ComparisonView.staleFetch.test.tsx 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) => { From 80176cac4db4820e76ea2a156c7a2974bee2f203 Mon Sep 17 00:00:00 2001 From: Tudor Date: Fri, 17 Jul 2026 07:27:10 +0100 Subject: [PATCH 3/3] =?UTF-8?q?fix(compare):=20drop=20the=20merge-on-fetch?= =?UTF-8?q?=20=E2=80=94=20cancellation=20alone=20fixes=20the=20race?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding: merging left a re-added school's old entry in the map forever, so the covered check served stale data and the map grew unboundedly. The cancelled flag already discards superseded responses, which is the actual race fix; replacing keeps the map bounded to the current selection and guarantees fresh refetches. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB --- nextjs-app/components/ComparisonView.tsx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/nextjs-app/components/ComparisonView.tsx b/nextjs-app/components/ComparisonView.tsx index 5f5b163..b3973af 100644 --- a/nextjs-app/components/ComparisonView.tsx +++ b/nextjs-app/components/ComparisonView.tsx @@ -133,15 +133,16 @@ export function ComparisonView({ // 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. + // stale response must not replace data for the current set — it blanked + // every section until a hard refresh. Cleanup marks the run cancelled + // when urnKey moves on, so only the current selection's response is + // applied (replacing the map keeps it bounded and guarantees a re-added + // school is refetched fresh rather than served a lingering old entry). let cancelled = false; fetchComparison(urnKey, { cache: 'no-store' }) .then((data) => { if (cancelled) return; - setComparisonData((prev) => ({ ...(prev ?? {}), ...data.comparison })); + setComparisonData(data.comparison); setNationalAverages(data.national_averages); setBenchmarks(data.benchmarks); })