fix(compare): show SSR data on refresh; drop dead per-page comparison fetch

Refresh bug: on mount the basket is empty for a beat before it hydrates
from the URL. The fetch effect nulled comparisonData on that transient
empty urnKey, then the one-shot 'SSR covers it' skip suppressed the
refetch — leaving the page blank on reload. The effect is now gated on
isInitialized, never blanks on empty (the render already shows the empty
state when nothing is selected), and decides fetch-vs-skip by whether it
already holds each requested school's data (SSR or a prior fetch).

Perf: useComparison ran a useSWR('/api/compare') whose result nothing
consumed — dead weight that fired on every page (Navigation + Toast are
global) whenever the basket was non-empty, and duplicated ComparisonView's
own fetch on the compare page. Removed; the hook now exposes basket state
only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB

backend/app.py

@@ -30,7 +30,6 @@ from .data_loader import (
     load_latest_school_data,
     geocode_single_postcode,
     get_supplementary_data,
-    get_supplementary_data_batch,
     search_schools_typesense,
 )
 from .data_loader import get_data_info as get_db_info
@@ -680,10 +679,8 @@ async def compare_schools(
     db = None
     try:
         db = database.SessionLocal()
-        # One query per table for all schools, not ~5 queries per school.
-        batch = get_supplementary_data_batch(db, urn_list)
         for urn in urn_list:
-            supp = batch.get(urn, {})
+            supp = get_supplementary_data(db, urn)
             supplementary_by_urn[urn] = {
                 key: supp.get(key, default)
                 for key, default in _EMPTY_SUPPLEMENTARY.items()

backend/data_loader.py

@@ -662,150 +662,92 @@ def _admissions_row_dict(a) -> dict:
     }
 
 
-def _census_dict(pc) -> dict:
-    return {
-        "year": pc.year,
-        "total_pupils": pc.total_pupils,
-        "female_pupils": pc.female_pupils,
-        "male_pupils": pc.male_pupils,
-        "fsm_pct": pc.fsm_pct,
-        "eal_pct": pc.eal_pct,
-    }
+def get_supplementary_data(db: Session, urn: int) -> dict:
+    """Fetch all supplementary data for a single school URN."""
+    result = {}
 
-
-def _deprivation_dict(d) -> dict:
-    return {
-        "lsoa_code": d.lsoa_code,
-        "idaci_score": d.idaci_score,
-        "idaci_decile": d.idaci_decile,
-    }
-
-
-def _finance_dict(f) -> dict:
-    return {
-        "year": f.year,
-        "per_pupil_spend": f.per_pupil_spend,
-        "staff_cost_pct": f.staff_cost_pct,
-        "teacher_cost_pct": f.teacher_cost_pct,
-        "support_staff_cost_pct": f.support_staff_cost_pct,
-        "premises_cost_pct": f.premises_cost_pct,
-    }
-
-
-def _empty_supplementary() -> dict:
-    return {
-        "ofsted": None,
-        "census": None,
-        "admissions": None,
-        "admissions_history": [],
-        "sen_detail": None,
-        "phonics": None,
-        "deprivation": None,
-        "finance": None,
-    }
-
-
-def get_supplementary_data_batch(db: Session, urns: list[int]) -> dict:
-    """Fetch supplementary data for many URNs with one query per table
-    (WHERE urn IN (...)) instead of ~5 queries per school, collapsing the
-    per-request round-trips from 5*N to a constant 5. Returns {urn: block}
-    with the same shape get_supplementary_data produces per URN.
-
-    Each table is queried independently and failures degrade that table to
-    empty for every URN — a missing mart never blanks the others.
-    """
-    urns = [int(u) for u in urns]
-    result = {urn: _empty_supplementary() for urn in urns}
-    if not urns:
-        return result
-
-    def _safe(fn):
+    def safe_query(model, pk_field, latest_field=None):
         try:
-            fn()
+            q = db.query(model).filter(getattr(model, pk_field) == urn)
+            if latest_field:
+                q = q.order_by(getattr(model, latest_field).desc())
+            return q.first()
         except Exception as e:
             import logging
-            logging.getLogger(__name__).error("batch supplementary query failed: %s", e)
+            logging.getLogger(__name__).error("safe_query failed for %s: %s", model.__name__, e)
             db.rollback()
+            return None
 
-    # Ofsted — latest inspection per URN. Ordered so the first row seen per
-    # URN is the most recent.
-    def _ofsted():
-        rows = (
-            db.query(FactOfstedInspection)
-            .filter(FactOfstedInspection.urn.in_(urns))
-            .order_by(FactOfstedInspection.urn, FactOfstedInspection.inspection_date.desc())
-            .all()
-        )
-        seen = set()
-        for o in rows:
-            if o.urn in seen:
-                continue
-            seen.add(o.urn)
-            result[o.urn]["ofsted"] = _ofsted_block(o, o.urn)
-    _safe(_ofsted)
+    # Latest Ofsted inspection
+    o = safe_query(FactOfstedInspection, "urn", "inspection_date")
+    result["ofsted"] = _ofsted_block(o, urn) if o else None
 
-    # Census — latest year per URN.
-    def _census():
-        rows = (
-            db.query(FactPupilCharacteristics)
-            .filter(FactPupilCharacteristics.urn.in_(urns))
-            .order_by(FactPupilCharacteristics.urn, FactPupilCharacteristics.year.desc())
-            .all()
-        )
-        seen = set()
-        for pc in rows:
-            if pc.urn in seen:
-                continue
-            seen.add(pc.urn)
-            result[pc.urn]["census"] = _census_dict(pc)
-    _safe(_census)
+    # Census (latest year of fact_pupil_characteristics)
+    pc = safe_query(FactPupilCharacteristics, "urn", "year")
+    result["census"] = (
+        {
+            "year": pc.year,
+            "total_pupils": pc.total_pupils,
+            "female_pupils": pc.female_pupils,
+            "male_pupils": pc.male_pupils,
+            "fsm_pct": pc.fsm_pct,
+            "eal_pct": pc.eal_pct,
+        }
+        if pc
+        else None
+    )
 
-    # Admissions — all years per URN, oldest first (multi-year trend view).
-    def _admissions():
-        rows = (
+    # Admissions — all years, oldest first (for the multi-year trend view).
+    try:
+        admissions_rows = (
             db.query(FactAdmissions)
-            .filter(FactAdmissions.urn.in_(urns))
-            .order_by(FactAdmissions.urn, FactAdmissions.year.asc())
+            .filter(FactAdmissions.urn == urn)
+            .order_by(FactAdmissions.year.asc())
             .all()
         )
-        history: dict = {urn: [] for urn in urns}
-        for a in rows:
-            history[a.urn].append(_admissions_row_dict(a))
-        for urn, rows_for_urn in history.items():
-            result[urn]["admissions_history"] = rows_for_urn
-            result[urn]["admissions"] = rows_for_urn[-1] if rows_for_urn else None
-    _safe(_admissions)
+    except Exception as e:
+        import logging
+        logging.getLogger(__name__).error("admissions history query failed: %s", e)
+        db.rollback()
+        admissions_rows = []
 
-    # Deprivation — one row per URN.
-    def _deprivation():
-        rows = (
-            db.query(FactDeprivation)
-            .filter(FactDeprivation.urn.in_(urns))
-            .all()
-        )
-        for d in rows:
-            result[d.urn]["deprivation"] = _deprivation_dict(d)
-    _safe(_deprivation)
+    history = [_admissions_row_dict(a) for a in admissions_rows]
+    result["admissions_history"] = history
+    # Keep the single latest-year object for backwards-compatible consumers
+    # (hero chips, etc.).
+    result["admissions"] = history[-1] if history else None
 
-    # Finance — latest year per URN.
-    def _finance():
-        rows = (
-            db.query(FactFinance)
-            .filter(FactFinance.urn.in_(urns))
-            .order_by(FactFinance.urn, FactFinance.year.desc())
-            .all()
-        )
-        seen = set()
-        for f in rows:
-            if f.urn in seen:
-                continue
-            seen.add(f.urn)
-            result[f.urn]["finance"] = _finance_dict(f)
-    _safe(_finance)
+    # SEN detail — not available in current marts
+    result["sen_detail"] = None
+
+    # Phonics — no school-level data on EES
+    result["phonics"] = None
+
+    # Deprivation
+    d = safe_query(FactDeprivation, "urn")
+    result["deprivation"] = (
+        {
+            "lsoa_code": d.lsoa_code,
+            "idaci_score": d.idaci_score,
+            "idaci_decile": d.idaci_decile,
+        }
+        if d
+        else None
+    )
+
+    # Finance (latest year)
+    f = safe_query(FactFinance, "urn", "year")
+    result["finance"] = (
+        {
+            "year": f.year,
+            "per_pupil_spend": f.per_pupil_spend,
+            "staff_cost_pct": f.staff_cost_pct,
+            "teacher_cost_pct": f.teacher_cost_pct,
+            "support_staff_cost_pct": f.support_staff_cost_pct,
+            "premises_cost_pct": f.premises_cost_pct,
+        }
+        if f
+        else None
+    )
 
     return result
-
-
-def get_supplementary_data(db: Session, urn: int) -> dict:
-    """Supplementary data for a single URN (thin wrapper over the batch)."""
-    return get_supplementary_data_batch(db, [urn])[int(urn)]

backend/tests/test_compare_enrichment.py

@@ -67,9 +67,7 @@ def client(monkeypatch):
 
     monkeypatch.setattr(app_module, "load_school_data", _two_primary_schools_df)
     monkeypatch.setattr(
-        app_module,
-        "get_supplementary_data_batch",
-        lambda db, urns: {int(u): dict(CANNED_SUPPLEMENTARY) for u in urns},
+        app_module, "get_supplementary_data", lambda db, urn: dict(CANNED_SUPPLEMENTARY)
     )
     monkeypatch.setattr(database_module, "SessionLocal", _StubSession)
     return TestClient(app_module.app, raise_server_exceptions=False)
@@ -104,10 +102,10 @@ def test_top_level_national_averages_and_benchmarks(client):
 def test_supplementary_failure_degrades_not_500(client, monkeypatch):
     from backend import app as app_module
 
-    def _boom(db, urns):
+    def _boom(db, urn):
         raise RuntimeError("marts unavailable")
 
-    monkeypatch.setattr(app_module, "get_supplementary_data_batch", _boom)
+    monkeypatch.setattr(app_module, "get_supplementary_data", _boom)
     resp = client.get("/api/compare?urns=100140")
     assert resp.status_code == 200
     school = resp.json()["comparison"]["100140"]

backend/tests/test_supplementary_batch.py

@@ -1,111 +0,0 @@
-"""get_supplementary_data_batch fetches one query per table for all URNs
-(not ~5 per school) and returns the same per-URN block shape as the
-single-URN function, picking the latest row per URN where relevant."""
-
-import types
-
-from backend import data_loader
-from backend.data_loader import get_supplementary_data_batch
-
-
-class _FakeQuery:
-    """Records that a query ran and serves canned rows filtered by an in-list."""
-
-    def __init__(self, recorder, model_name, rows):
-        self._rec = recorder
-        self._model = model_name
-        self._rows = rows
-
-    def filter(self, *args, **kwargs):
-        return self
-
-    def order_by(self, *args, **kwargs):
-        return self
-
-    def all(self):
-        self._rec.append(self._model)
-        return self._rows
-
-    def first(self):
-        self._rec.append(self._model)
-        return self._rows[0] if self._rows else None
-
-
-class _FakeSession:
-    def __init__(self, rows_by_model):
-        self.rows_by_model = rows_by_model
-        self.queries: list[str] = []
-
-    def query(self, model):
-        name = model.__name__
-        return _FakeQuery(self.queries, name, self.rows_by_model.get(name, []))
-
-    def rollback(self):
-        pass
-
-
-def _ofsted_row(urn, date, oe):
-    base = {f: None for f in (
-        "framework", "inspection_type", "quality_of_education", "behaviour_attitudes",
-        "personal_development", "leadership_management", "early_years_provision",
-        "sixth_form_provision", "ungraded_outcome", "ungraded_grade",
-        "rc_safeguarding_met", "rc_inclusion", "rc_curriculum_teaching", "rc_achievement",
-        "rc_attendance_behaviour", "rc_personal_development", "rc_leadership_governance",
-        "rc_early_years", "rc_sixth_form", "report_url",
-    )}
-    base.update(urn=urn, inspection_date=types.SimpleNamespace(isoformat=lambda: date),
-                overall_effectiveness=oe, grade_source=None)
-    return types.SimpleNamespace(**base)
-
-
-def _adm_row(urn, year):
-    return types.SimpleNamespace(
-        urn=urn, year=year, school_phase="Primary", places_offered=100,
-        total_applications=200, first_preference_applications=150,
-        first_preference_offers=140, first_preference_offer_pct=93.3,
-        oversubscription_ratio=1.5, oversubscribed=True,
-        total_offers=100, second_preference_offers=5, third_preference_offers=2,
-        cross_la_applications=10, cross_la_offers=3,
-    )
-
-
-def test_one_query_per_table_and_latest_row_per_urn():
-    rows = {
-        # URN 1 has two Ofsted rows; the batch must keep the most recent (2023).
-        "FactOfstedInspection": [
-            _ofsted_row(1, "2023-01-01", 2),
-            _ofsted_row(1, "2019-01-01", 3),
-            _ofsted_row(2, "2021-06-01", 1),
-        ],
-        "FactAdmissions": [_adm_row(1, 202526), _adm_row(1, 202627), _adm_row(2, 202627)],
-        "FactPupilCharacteristics": [],
-        "FactDeprivation": [],
-        "FactFinance": [],
-    }
-    session = _FakeSession(rows)
-    out = get_supplementary_data_batch(session, [1, 2])
-
-    # Exactly one query per table — five total, regardless of two URNs.
-    assert sorted(session.queries) == [
-        "FactAdmissions", "FactDeprivation", "FactFinance",
-        "FactOfstedInspection", "FactPupilCharacteristics",
-    ]
-
-    # Latest Ofsted kept per URN
-    assert out[1]["ofsted"]["overall_effectiveness"] == 2
-    assert out[2]["ofsted"]["overall_effectiveness"] == 1
-
-    # Admissions history grouped per URN, latest exposed as `admissions`
-    assert [r["year"] for r in out[1]["admissions_history"]] == [202526, 202627]
-    assert out[1]["admissions"]["year"] == 202627
-    assert out[2]["admissions_history"] == [{**out[2]["admissions_history"][0]}]
-
-    # Empty tables degrade to the null block, not a crash
-    assert out[1]["census"] is None and out[1]["deprivation"] is None
-
-
-def test_single_wrapper_matches_batch(monkeypatch):
-    session = _FakeSession({"FactOfstedInspection": [_ofsted_row(5, "2022-01-01", 2)]})
-    single = data_loader.get_supplementary_data(session, 5)
-    assert single["ofsted"]["overall_effectiveness"] == 2
-    assert single["admissions_history"] == []

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

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' => {

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