Compare commits

...
Author SHA1 Message Date
TudorandClaude Fable 5 090d5f7bec ci: speed up Frontend Typecheck + Tests; cancel superseded PR runs
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m34s
PR Checks / Backend Smoke (pull_request) Successful in 7s
PR Checks / Build Backend (no push) (pull_request) Successful in 10s
PR Checks / Build Frontend (no push) (pull_request) Successful in 46s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 9s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 9s
The job does ~3s of real work (typecheck 1.4s + jest 1.3s) but installs
452 MB / 460 packages every run. Two changes:

- Cache nextjs-app/node_modules keyed on the lockfile hash (OS + node
  major pinned) and skip npm ci entirely on a hit — deps change rarely,
  so most PR pushes now do zero install. On miss, npm ci runs with
  --prefer-offline --no-audit --no-fund.
- Workflow-level concurrency with cancel-in-progress: a new commit (or an
  empty re-trigger) aborts the previous run instead of stacking a second
  full matrix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
2026-07-14 22:54:32 +01:00
tudor abc03a0dd3 Merge pull request 'fix(compare): blank page on refresh + remove dead per-page comparison fetch' (#37) from fix/compare-refresh-and-fetch into main
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 13s
Stage (build -> staging -> E2E gate) / Build Frontend (Next.js) (push) Successful in 53s
Stage (build -> staging -> E2E gate) / Build Pipeline (Meltano + dbt + Airflow) (push) Successful in 13s
Stage (build -> staging -> E2E gate) / Deploy to Staging (push) Successful in 1s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Failing after 1m9s
Reviewed-on: #37
2026-07-14 21:39:46 +00:00
TudorandClaude Fable 5 43a2c4a6bc fix(compare): show SSR data on refresh; drop dead per-page comparison fetch
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 9m42s
PR Checks / Backend Smoke (pull_request) Successful in 7s
PR Checks / Build Backend (no push) (pull_request) Successful in 10s
PR Checks / Build Frontend (no push) (pull_request) Successful in 49s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 9s
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
2026-07-14 22:34:20 +01:00
4 changed files with 131 additions and 63 deletions
+20 -3
View File
@@ -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
@@ -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();
});
+20 -19
View File
@@ -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' => {
+9 -41
View File
@@ -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();
}