fix(compare): blank page on refresh + remove dead per-page comparison fetch #37

Merged
tudor merged 1 commits from fix/compare-refresh-and-fetch into main 2026-07-14 21:39:47 +00:00
3 changed files with 111 additions and 60 deletions
@@ -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 }); router.replace(newUrl, { scroll: false });
}, [urnKey, selectedMetric, pathname, searchParams, router]); }, [urnKey, selectedMetric, pathname, searchParams, router]);
// Fetch only when the school set changes. The very first run is skipped // Fetch when the school set changes, but only for schools we don't already
// when the SSR payload already covers the current set — no double-fetch // have data for. This skips the refetch of SSR-rendered data on load AND
// of data the server just rendered. // avoids a network call when a school is merely removed. A ref holds the
const firstFetchRef = useRef(true); // latest data so the effect can read it without re-running on every fetch.
useEffect(() => { //
if (!urnKey) { // Correctness note: we must NOT null the data on a transient empty urnKey.
setComparisonData(null); // On mount the basket is empty for a beat before it hydrates from the URL,
setNationalAverages(undefined); // and blanking here (then skipping the refetch because SSR "covers" the set)
setBenchmarks(undefined); // was leaving the page empty on refresh. The render already shows the empty
return; // 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) { useEffect(() => {
firstFetchRef.current = false; if (!isInitialized || !urnKey) return;
const ssrUrns = new Set(Object.keys(initialData ?? {}));
const covered = urnKey.split(',').every((urn) => ssrUrns.has(urn)); const have = comparisonDataRef.current ?? {};
if (covered && ssrUrns.size > 0) return; const covered = urnKey.split(',').every((urn) => have[urn] != null);
} if (covered) return;
fetchComparison(urnKey, { cache: 'no-store' }) fetchComparison(urnKey, { cache: 'no-store' })
.then((data) => { .then((data) => {
@@ -138,8 +140,7 @@ export function ComparisonView({
// destroy a working comparison the user is looking at. // destroy a working comparison the user is looking at.
console.error('Failed to fetch comparison:', err); console.error('Failed to fetch comparison:', err);
}); });
// eslint-disable-next-line react-hooks/exhaustive-deps }, [urnKey, isInitialized]);
}, [urnKey]);
// Classify schools by phase using comparison data // Classify schools by phase using comparison data
const classifySchool = (school: School): 'primary' | 'secondary' => { const classifySchool = (school: School): 'primary' | 'secondary' => {
+9 -41
View File
@@ -1,50 +1,18 @@
/** /**
* Custom hook for managing school comparison state * Custom hook for managing school comparison state.
* Uses shared context for real-time updates across components *
* 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'; 'use client';
import useSWR from 'swr';
import { fetcher } from '@/lib/api';
import { useComparisonContext } from '@/context/ComparisonContext'; import { useComparisonContext } from '@/context/ComparisonContext';
import type { ComparisonResponse } from '@/lib/types';
export function useComparison() { export function useComparison() {
const { return useComparisonContext();
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,
};
} }