Merge pull request 'fix(compare): align sticky school chips with their data columns on desktop' (#51) from fix/compare-final-review-mustfix into main

Reviewed-on: #51

e2e/tests/journeys.spec.ts

@@ -204,8 +204,6 @@ 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();
@@ -239,31 +237,6 @@ 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,7 +72,5 @@ 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

@@ -1,107 +0,0 @@
-/**
- * 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,16 +148,10 @@
   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 caption occupies the rail; chips flow into the school columns. */
+   The first chip starts after the empty label rail. */
 @media (min-width: 761px) {
   .schoolBar {
     display: grid;
@@ -170,29 +164,8 @@
     min-width: 0;
   }
 
-  .barCaption {
+  .schoolChip:first-child {
-    grid-column: 1;
+    grid-column: 2;
-    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,9 +85,7 @@ export function ComparisonView({
         replaceSchools(urlSchools);
       }
     }
-    // Re-seed when a client-side navigation lands on a different ?urns= set
+  }, [isInitialized]); // eslint-disable-line react-hooks/exhaustive-deps
-    // (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(',');
 
@@ -130,18 +128,8 @@ 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);
@@ -152,9 +140,6 @@ 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) => {
@@ -365,14 +350,6 @@ 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}