Merge pull request 'fix(compare): trends chart was squashed to ~150px — give it a real height' (#41) from fix/trends-chart-height into main

Reviewed-on: #41

.gitea/workflows/pr-checks.yml

@@ -5,6 +5,13 @@ on:
     branches:
       - main
 
+# Cancel superseded runs: pushing a new commit to a PR (or an empty
+# re-trigger) aborts the previous still-running checks instead of running
+# a second full matrix alongside them.
+concurrency:
+  group: pr-checks-${{ gitea.event.pull_request.number }}
+  cancel-in-progress: true
+
 env:
   REGISTRY: privaterepo.sitaru.org
   BACKEND_IMAGE_NAME: ${{ gitea.repository }}-backend
@@ -23,12 +30,22 @@ jobs:
         uses: actions/setup-node@v4
         with:
           node-version: 22
-          cache: npm
-          cache-dependency-path: nextjs-app/package-lock.json
+
+      # Cache the resolved node_modules (452 MB / 460 packages) keyed on the
+      # lockfile. On a hit — the common case, since deps change rarely — the
+      # whole `npm ci` step is skipped, not just its download phase. The key
+      # pins OS + node major so we never restore incompatible native binaries.
+      - name: Cache node_modules
+        id: node-modules-cache
+        uses: actions/cache@v4
+        with:
+          path: nextjs-app/node_modules
+          key: nextjs-node-modules-${{ runner.os }}-node22-${{ hashFiles('nextjs-app/package-lock.json') }}
 
       - name: Install dependencies
+        if: steps.node-modules-cache.outputs.cache-hit != 'true'
         working-directory: nextjs-app
-        run: npm ci
+        run: npm ci --prefer-offline --no-audit --no-fund
 
       - name: Typecheck
         working-directory: nextjs-app

backend/app.py

@@ -30,6 +30,7 @@ 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
@@ -679,8 +680,10 @@ 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 = get_supplementary_data(db, urn)
+            supp = batch.get(urn, {})
             supplementary_by_urn[urn] = {
                 key: supp.get(key, default)
                 for key, default in _EMPTY_SUPPLEMENTARY.items()

backend/data_loader.py

@@ -662,30 +662,8 @@ def _admissions_row_dict(a) -> dict:
     }
 
 
-def get_supplementary_data(db: Session, urn: int) -> dict:
-    """Fetch all supplementary data for a single school URN."""
-    result = {}
-
-    def safe_query(model, pk_field, latest_field=None):
-        try:
-            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("safe_query failed for %s: %s", model.__name__, e)
-            db.rollback()
-            return None
-
-    # Latest Ofsted inspection
-    o = safe_query(FactOfstedInspection, "urn", "inspection_date")
-    result["ofsted"] = _ofsted_block(o, urn) if o else None
-
-    # Census (latest year of fact_pupil_characteristics)
-    pc = safe_query(FactPupilCharacteristics, "urn", "year")
-    result["census"] = (
-        {
+def _census_dict(pc) -> dict:
+    return {
         "year": pc.year,
         "total_pupils": pc.total_pupils,
         "female_pupils": pc.female_pupils,
@@ -693,52 +671,18 @@ def get_supplementary_data(db: Session, urn: int) -> dict:
         "fsm_pct": pc.fsm_pct,
         "eal_pct": pc.eal_pct,
     }
-        if pc
-        else None
-    )
 
-    # Admissions — all years, oldest first (for the multi-year trend view).
-    try:
-        admissions_rows = (
-            db.query(FactAdmissions)
-            .filter(FactAdmissions.urn == urn)
-            .order_by(FactAdmissions.year.asc())
-            .all()
-        )
-    except Exception as e:
-        import logging
-        logging.getLogger(__name__).error("admissions history query failed: %s", e)
-        db.rollback()
-        admissions_rows = []
 
-    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
-
-    # 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"] = (
-        {
+def _deprivation_dict(d) -> dict:
+    return {
         "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"] = (
-        {
+
+def _finance_dict(f) -> dict:
+    return {
         "year": f.year,
         "per_pupil_spend": f.per_pupil_spend,
         "staff_cost_pct": f.staff_cost_pct,
@@ -746,8 +690,122 @@ def get_supplementary_data(db: Session, urn: int) -> dict:
         "support_staff_cost_pct": f.support_staff_cost_pct,
         "premises_cost_pct": f.premises_cost_pct,
     }
-        if f
-        else None
+
+
+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):
+        try:
+            fn()
+        except Exception as e:
+            import logging
+            logging.getLogger(__name__).error("batch supplementary query failed: %s", e)
+            db.rollback()
+
+    # 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)
+
+    # 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)
+
+    # Admissions — all years per URN, oldest first (multi-year trend view).
+    def _admissions():
+        rows = (
+            db.query(FactAdmissions)
+            .filter(FactAdmissions.urn.in_(urns))
+            .order_by(FactAdmissions.urn, 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)
+
+    # 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)
+
+    # 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)
 
     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,7 +67,9 @@ def client(monkeypatch):
 
     monkeypatch.setattr(app_module, "load_school_data", _two_primary_schools_df)
     monkeypatch.setattr(
-        app_module, "get_supplementary_data", lambda db, urn: dict(CANNED_SUPPLEMENTARY)
+        app_module,
+        "get_supplementary_data_batch",
+        lambda db, urns: {int(u): dict(CANNED_SUPPLEMENTARY) for u in urns},
     )
     monkeypatch.setattr(database_module, "SessionLocal", _StubSession)
     return TestClient(app_module.app, raise_server_exceptions=False)
@@ -102,10 +104,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, urn):
+    def _boom(db, urns):
         raise RuntimeError("marts unavailable")
 
-    monkeypatch.setattr(app_module, "get_supplementary_data", _boom)
+    monkeypatch.setattr(app_module, "get_supplementary_data_batch", _boom)
     resp = client.get("/api/compare?urns=100140")
     assert resp.status_code == 200
     school = resp.json()["comparison"]["100140"]

backend/tests/test_supplementary_batch.py

@@ -0,0 +1,111 @@
+"""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"] == []

e2e/tests/journeys.spec.ts

@@ -19,6 +19,27 @@ function schoolLinks(page: Page) {
   return page.locator('a[href^="/school/"]');
 }
 
+/**
+ * Two URNs guaranteed to be pure-primary (same phase). The compare page's
+ * phase tabs split all-through schools (which carry KS4 data) onto the
+ * secondary tab, so picking two arbitrary "primary" search hits can land
+ * them on different tabs where only the active one renders. Selecting via
+ * the API by exact phase keeps both on the same tab. Data-invariant: uses
+ * whatever primaries the environment holds.
+ */
+async function twoPrimaryUrns(page: Page): Promise<[string, string]> {
+  const res = await page.request.get('/api/schools?search=primary&per_page=50');
+  expect(res.ok()).toBeTruthy();
+  const body = await res.json();
+  const urns: string[] = (body.schools ?? [])
+    .filter((s: { phase?: string; rwm_expected_pct?: number | null }) =>
+      s.phase === 'Primary' && s.rwm_expected_pct != null,
+    )
+    .map((s: { urn: number }) => String(s.urn));
+  expect(urns.length).toBeGreaterThanOrEqual(2);
+  return [urns[0], urns[1]];
+}
+
 test('home page loads with hero search', async ({ page }) => {
   await page.goto('/');
   await expect(page.locator('h1').first()).toBeVisible();
@@ -139,19 +160,13 @@ test('results map fullscreen falls back to an overlay on iOS', async ({ page })
 });
 
 test('comparing two schools shows the parent-first sections side by side', async ({ page }) => {
-  // Collect two school URNs from search results, then load the share URL
-  await searchByName(page, 'primary');
-  await expect(schoolLinks(page).first()).toBeVisible({ timeout: 15_000 });
-  const hrefs = await schoolLinks(page).evaluateAll((links) =>
-    links.map((l) => (l as HTMLAnchorElement).getAttribute('href') || '')
-  );
-  const urns = [...new Set(hrefs.map((h) => h.match(/\/school\/(\d+)/)?.[1]).filter(Boolean))];
-  expect(urns.length).toBeGreaterThanOrEqual(2);
+  // Two same-phase (pure primary) schools so both stay on one tab.
+  const [urn0, urn1] = await twoPrimaryUrns(page);
 
-  await page.goto(`/compare?urns=${urns[0]},${urns[1]}`);
+  await page.goto(`/compare?urns=${urn0},${urn1}`);
   // Both schools' detail links should render in the comparison view
-  await expect(page.locator(`a[href*="${urns[0]}"]`).first()).toBeVisible({ timeout: 15_000 });
-  await expect(page.locator(`a[href*="${urns[1]}"]`).first()).toBeVisible();
+  await expect(page.locator(`a[href*="${urn0}"]`).first()).toBeVisible({ timeout: 15_000 });
+  await expect(page.locator(`a[href*="${urn1}"]`).first()).toBeVisible();
 
   // The parent-first sections render in order (data-invariant: headings only)
   for (const heading of [
@@ -213,7 +228,12 @@ test('compare chart on mobile shows school chips with tap-to-focus', async ({ pa
   expect(bodyOverflowsX).toBe(false);
 
   // The trends chart still renders (inside the Explore trends section)…
-  await expect(page.locator('canvas:visible').first()).toBeVisible({ timeout: 15_000 });
+  const chartCanvas = page.locator('canvas:visible').first();
+  await expect(chartCanvas).toBeVisible({ timeout: 15_000 });
+  // …at a real height, not the squashed ~150px Chart.js fallback that
+  // appears when the container lacks a definite height.
+  const chartBox = await chartCanvas.boundingBox();
+  expect(chartBox && chartBox.height).toBeGreaterThan(220);
 
   // …with the mobile chart legend chips and tap-to-focus behaviour intact.
   const chipGroup = page.getByRole('group', { name: /highlight a school/i });

nextjs-app/components/compare/TrendsExplorer.module.css

@@ -60,8 +60,20 @@
   margin: 0 0 1rem;
 }
 
+/* ComparisonChart runs Chart.js with maintainAspectRatio:false, so it fills
+   its container's height — which must be *definite*. A min-height alone does
+   not resolve the chart wrapper's height:100%, leaving Chart.js to fall back
+   to its ~150px default (a squashed sliver). Give it a real height. */
 .chartBox {
-  min-height: 320px;
+  height: 420px;
+}
+
+@media (max-width: 640px) {
+  /* Taller on mobile: the mobile-only school chips sit above the canvas and
+     wrap to two rows for 3+ schools, so the plot keeps a usable height. */
+  .chartBox {
+    height: 360px;
+  }
 }
 
 .tableWrapper {