Merge pull request 'fix(compare): stale-basket fetch blanking comparisons; rail caption' (#52) from fix/compare-final-review-mustfix into main

Reviewed-on: #52

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

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

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(
+    <ComparisonProvider>
+      <ComparisonView
+        initialData={URL_DATA}
+        initialNationalAverages={{
+          year: 202425,
+          primary: { rwm_expected_pct: 62 },
+          secondary: {},
+          by_year: [],
+        }}
+        initialBenchmarks={undefined}
+        initialUrns={[100, 200]}
+        metrics={[]}
+        selectedMetric="rwm_expected_pct"
+      />
+    </ComparisonProvider>,
+  );
+
+  // 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);
+});

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 {
+  .barCaption {
-    grid-column: 2;
+    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);
   }
 }
 

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,8 +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 — 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(data.comparison);
         setNationalAverages(data.national_averages);
         setBenchmarks(data.benchmarks);
@@ -140,6 +152,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) => {
@@ -350,6 +365,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). */}
+            <div className={styles.barCaption}>
+              <span className={styles.barCaptionEyebrow}>Comparing</span>
+              <span className={styles.barCaptionCount}>
+                {activeSchools.length} {comparePhase} school
+                {activeSchools.length === 1 ? '' : 's'}
+              </span>
+            </div>
             {activeSchools.map((school, index) => (
               <div
                 key={school.urn}