From 7b41218e6e141154b1b08fdf5762f59f9a041830 Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 15 Sep 2026 10:17:18 +0100 Subject: [PATCH] fix(web): show an outage as an outage, and drop superseded fetches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The home page caught every fetch failure and rendered its empty state, so a backend outage looked like a site with no schools in it. School pages turned any error into notFound(), which told visitors — and crawlers — that a real school had ceased to exist. Place fetches did the same by returning [] and null. Failures now reach a retryable error boundary; only a genuine 404 still calls notFound(). "Load more" and the map fetch resolved against whatever state existed when they returned, so results from an abandoned search appended themselves to the new ones. Each fetch now carries an AbortController and checks that its search scope is still current before touching state. The map only records its cache key on success, so a failed load retries instead of pinning the stale marker set. Jest ignored .next/, whose build output otherwise shadowed real suites. Co-Authored-By: Claude Opus 5 --- .../__tests__/app/dataFailures.test.tsx | 51 ++++++++ .../components/HomeView.staleFetch.test.tsx | 81 +++++++++++++ nextjs-app/app/(frontend)/error.tsx | 12 ++ nextjs-app/app/(frontend)/page.tsx | 111 +++++++----------- .../app/(frontend)/school/[slug]/page.tsx | 6 +- nextjs-app/components/HomeView.tsx | 42 +++++-- nextjs-app/jest.config.cjs | 1 + nextjs-app/lib/places.ts | 7 +- 8 files changed, 231 insertions(+), 80 deletions(-) create mode 100644 nextjs-app/__tests__/app/dataFailures.test.tsx create mode 100644 nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx create mode 100644 nextjs-app/app/(frontend)/error.tsx diff --git a/nextjs-app/__tests__/app/dataFailures.test.tsx b/nextjs-app/__tests__/app/dataFailures.test.tsx new file mode 100644 index 0000000..95ba9d4 --- /dev/null +++ b/nextjs-app/__tests__/app/dataFailures.test.tsx @@ -0,0 +1,51 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import HomePage from '@/app/(frontend)/page'; +import SchoolPage from '@/app/(frontend)/school/[slug]/page'; +import ErrorPage from '@/app/(frontend)/error'; +import { APIFetchError, fetchSchools, fetchFilters, fetchSchoolDetails } from '@/lib/api'; +import { fetchPlace, fetchPlaces } from '@/lib/places'; + +jest.mock('@/lib/api', () => ({ + ...jest.requireActual('@/lib/api'), + fetchSchools: jest.fn(), + fetchSchoolDetails: jest.fn(), + fetchFilters: jest.fn(async () => ({})), + fetchDataInfo: jest.fn(async () => null), + fetchNationalAverages: jest.fn(async () => null), +})); +jest.mock('@/lib/flags', () => ({ getFlags: jest.fn(async () => ({})) })); +jest.mock('next/navigation', () => ({ + notFound: () => { throw new Error('NEXT_NOT_FOUND'); }, + redirect: jest.fn(), +})); +const realFetch = global.fetch; +afterEach(() => { global.fetch = realFetch; jest.clearAllMocks(); }); + +test('school outages propagate; only a real 404 becomes not found', async () => { + const request = { params: Promise.resolve({ slug: '100001-school' }) }; + const outage = new APIFetchError('unavailable', 503); + jest.mocked(fetchSchoolDetails).mockRejectedValueOnce(outage); + await expect(SchoolPage(request)).rejects.toBe(outage); + jest.mocked(fetchSchoolDetails).mockRejectedValueOnce(new APIFetchError('missing', 404)); + await expect(SchoolPage(request)).rejects.toThrow('NEXT_NOT_FOUND'); +}); + +test('homepage search failure is not returned as an empty successful page', async () => { + jest.mocked(fetchSchools).mockRejectedValueOnce(new APIFetchError('unavailable', 503)); + await expect(HomePage({ searchParams: Promise.resolve({ search: 'school' }) })).rejects.toThrow('unavailable'); +}); + +test('a place is absent only on 404; other failures propagate', async () => { + global.fetch = jest.fn().mockResolvedValue({ ok: false, status: 404 }); + await expect(fetchPlace('town', 'example')).resolves.toBeNull(); + jest.mocked(global.fetch).mockResolvedValue({ ok: false, status: 503 } as Response); + await expect(fetchPlace('town', 'example')).rejects.toMatchObject({ status: 503 }); + await expect(fetchPlaces()).rejects.toMatchObject({ status: 503 }); +}); + +test('the error boundary offers a retry without showing an empty search', () => { + const reset = jest.fn(); + render(); + fireEvent.click(screen.getByRole('button', { name: 'Try again' })); + expect(reset).toHaveBeenCalledTimes(1); +}); diff --git a/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx b/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx new file mode 100644 index 0000000..4b35b3f --- /dev/null +++ b/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx @@ -0,0 +1,81 @@ +import { act, fireEvent, render, screen } from '@testing-library/react'; +import { HomeView } from '@/components/HomeView'; +import { fetchSchools } from '@/lib/api'; +import { primaryFixture } from '../support/schoolFixtures'; +import type { SchoolsResponse, School } from '@/lib/types'; + +let params = new URLSearchParams('postcode=SW1A+1AA'); +jest.mock('next/navigation', () => ({ + useSearchParams: () => params, + usePathname: () => '/', + useRouter: () => ({ push: jest.fn(), replace: jest.fn() }), +})); +jest.mock('@/context/ComparisonContext', () => ({ + useComparisonContext: () => ({ addSchool: jest.fn(), removeSchool: jest.fn(), selectedSchools: [] }), +})); +jest.mock('@/lib/api', () => ({ + fetchSchools: jest.fn(), + fetchNationalAverages: jest.fn(async () => ({})), + fetchLAaverages: jest.fn(async () => ({ secondary: { attainment_8_by_la: {} } })), +})); +jest.mock('@/components/FilterBar', () => ({ FilterBar: () => null })); +jest.mock('@/components/SchoolRow', () => ({ SchoolRow: ({ school }: {school: School}) =>
{school.school_name}
})); +jest.mock('@/components/SchoolMap', () => ({ SchoolMap: ({ schools }: {schools: School[]}) =>
{schools.map(s => s.school_name).join(',')}
})); + +const filters = { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; +function response(name: string): SchoolsResponse { + return { schools: [{ ...primaryFixture.schoolInfo, school_name: name }], + total: 2, page: 1, page_size: 1, total_pages: 2 }; +} +function deferred() { + let resolve!: (value: SchoolsResponse) => void; + let reject!: (error: Error) => void; + const promise = new Promise((yes, no) => { resolve = yes; reject = no; }); + return { promise, resolve, reject }; +} +beforeEach(() => { + params = new URLSearchParams('postcode=SW1A+1AA'); + jest.mocked(fetchSchools).mockReset(); +}); + +test('load-more results from an old search are discarded, even after returning to it', async () => { + const pending = deferred(); + jest.mocked(fetchSchools).mockReturnValueOnce(pending.promise); + const view = render(); + fireEvent.click(screen.getByRole('button', { name: 'Load more schools' })); + const signal = jest.mocked(fetchSchools).mock.calls[0][1]?.signal; + params = new URLSearchParams('postcode=SW2+1AA'); + view.rerender(); + expect(signal?.aborted).toBe(true); + params = new URLSearchParams('postcode=SW1A+1AA'); + view.rerender(); + await act(async () => pending.resolve(response('Stale append'))); + expect(screen.queryByText('Stale append')).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Load more schools' })).toBeEnabled(); +}); + +test('an older map response cannot overwrite the current search', async () => { + const first = deferred(), second = deferred(); + jest.mocked(fetchSchools).mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise); + const initial = response('Initial A'); + const view = render(); + fireEvent.click(screen.getByRole('button', { name: 'Map' })); + params = new URLSearchParams('postcode=SW2+1AA'); + view.rerender(); + await act(async () => second.resolve(response('Current map'))); + await act(async () => first.resolve(response('Stale map'))); + expect(screen.getByTestId('map')).toHaveTextContent('Current map'); + expect(screen.getByTestId('map')).not.toHaveTextContent('Stale map'); +}); + +test('failed map requests can be retried by reopening the map', async () => { + const pending = deferred(); + jest.mocked(fetchSchools).mockReturnValueOnce(pending.promise).mockResolvedValue(response('Retry result')); + render(); + fireEvent.click(screen.getByRole('button', { name: 'Map' })); + await act(async () => pending.reject(new Error('offline'))); + fireEvent.click(screen.getByRole('button', { name: 'List' })); + await act(async () => fireEvent.click(screen.getByRole('button', { name: 'Map' }))); + expect(fetchSchools).toHaveBeenCalledTimes(2); + expect(screen.getByTestId('map')).toHaveTextContent('Retry result'); +}); diff --git a/nextjs-app/app/(frontend)/error.tsx b/nextjs-app/app/(frontend)/error.tsx new file mode 100644 index 0000000..134d2f2 --- /dev/null +++ b/nextjs-app/app/(frontend)/error.tsx @@ -0,0 +1,12 @@ +'use client'; + +export default function ErrorPage({ reset }: { error: Error & { digest?: string }; reset: () => void }) { + return ( +
+

We couldn’t load this page

+

School information is temporarily unavailable. Please try again.

+ +

Return to school search

+
+ ); +} diff --git a/nextjs-app/app/(frontend)/page.tsx b/nextjs-app/app/(frontend)/page.tsx index f791b95..f3fd365 100644 --- a/nextjs-app/app/(frontend)/page.tsx +++ b/nextjs-app/app/(frontend)/page.tsx @@ -85,73 +85,50 @@ export default async function HomePage({ searchParams }: HomePageProps) { params.has_sixth_form ); - // Fetch data on server with error handling - try { - const [filtersData, dataInfo] = await Promise.all([fetchFilters(), fetchDataInfo().catch(() => null)]); + // Failures propagate to the retryable error boundary. + const [filtersData, dataInfo] = await Promise.all([fetchFilters(), fetchDataInfo().catch(() => null)]); - // Only fetch schools if there are search parameters - let schoolsData; - if (hasSearchParams) { - schoolsData = await fetchSchools({ - search: params.search, - local_authority: params.local_authority, - school_type: params.school_type, - phase: params.phase, - postcode: params.postcode, - radius, - page, - page_size: 50, - gender: params.gender, - admissions_policy: params.admissions_policy, - has_sixth_form: params.has_sixth_form, - }); - } else { - // Empty state by default - schoolsData = { schools: [], page: 1, page_size: 50, total: 0, total_pages: 0 }; - } - - const resolvedFilters = filtersData || { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; - // `unique_schools`, not `total_schools` — the latter is not a field this - // endpoint returns, and reading it silently yielded null on every request. - const total = dataInfo?.unique_schools ?? null; - const years = dataInfo?.years_available ?? []; - return ( - } - editorial={hasSearchParams ? null : ( - - )} - /> - ); - } catch (error) { - console.error('Error fetching data for home page:', error); - - const emptyFilters = { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; - return ( - } - editorial={hasSearchParams ? null : ( - - )} - /> - ); + // Only fetch schools if there are search parameters + let schoolsData; + if (hasSearchParams) { + schoolsData = await fetchSchools({ + search: params.search, + local_authority: params.local_authority, + school_type: params.school_type, + phase: params.phase, + postcode: params.postcode, + radius, + page, + page_size: 50, + gender: params.gender, + admissions_policy: params.admissions_policy, + has_sixth_form: params.has_sixth_form, + }); + } else { + // Empty state by default + schoolsData = { schools: [], page: 1, page_size: 50, total: 0, total_pages: 0 }; } + + const resolvedFilters = filtersData || { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; + // `unique_schools`, not `total_schools` — the latter is not a field this + // endpoint returns, and reading it silently yielded null on every request. + const total = dataInfo?.unique_schools ?? null; + const years = dataInfo?.years_available ?? []; + return ( + } + editorial={hasSearchParams ? null : ( + + )} + /> + ); } diff --git a/nextjs-app/app/(frontend)/school/[slug]/page.tsx b/nextjs-app/app/(frontend)/school/[slug]/page.tsx index 9b3649f..fe0f4af 100644 --- a/nextjs-app/app/(frontend)/school/[slug]/page.tsx +++ b/nextjs-app/app/(frontend)/school/[slug]/page.tsx @@ -4,7 +4,7 @@ * URL format: /school/138267-school-name-here */ -import { fetchSchoolDetails, fetchSchools, fetchNationalAverages } from '@/lib/api'; +import { APIFetchError, fetchSchoolDetails, fetchSchools, fetchNationalAverages } from '@/lib/api'; import { notFound, redirect } from 'next/navigation'; import { SchoolDetailShell } from '@/components/school/SchoolDetailShell'; import { NearbyPlaces } from '@/components/school/NearbyPlaces'; @@ -146,8 +146,8 @@ export default async function SchoolPage({ params }: SchoolPageProps) { fetchNationalAverages().catch(() => null), ]); } catch (error) { - console.error(`Failed to fetch school ${urn}:`, error); - notFound(); + if (error instanceof APIFetchError && error.status === 404) notFound(); + throw error; } const { school_info, yearly_data, absence_data, ofsted, census, admissions, admissions_history, admission_distance, deprivation, finance, destinations } = data; diff --git a/nextjs-app/components/HomeView.tsx b/nextjs-app/components/HomeView.tsx index fd90e77..66632f4 100644 --- a/nextjs-app/components/HomeView.tsx +++ b/nextjs-app/components/HomeView.tsx @@ -213,6 +213,16 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed const [isLoadingMap, setIsLoadingMap] = useState(false); const prevSearchParamsRef = useRef(searchParams.toString()); const mapParamsRef = useRef(''); + const loadMoreController = useRef(null); + // Identity changes even for A → B → A, so an old A response stays stale. + const searchScope = useRef({ key: searchParams.toString() }); + if (searchScope.current.key !== searchParams.toString()) { + searchScope.current = { key: searchParams.toString() }; + } + useEffect(() => { + setIsLoadingMore(false); + return () => { loadMoreController.current?.abort(); }; + }, [searchParams]); const [geoState, setGeoState] = useState<'idle' | 'requesting' | 'error'>('idle'); const [geoError, setGeoError] = useState(null); /* @@ -274,17 +284,27 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed if (resultsView !== 'map' || !isLocationSearch) return; const paramsKey = searchParams.toString(); if (paramsKey === mapParamsRef.current) return; - mapParamsRef.current = paramsKey; + const controller = new AbortController(); + const scope = searchScope.current; + const current = () => !controller.signal.aborted && searchScope.current === scope; setIsLoadingMap(true); const params: Record = {}; searchParams.forEach((value, key) => { params[key] = value; }); params.page = 1; params.page_size = 500; - fetchSchools(params, { cache: 'no-store' }) - .then(r => setMapSchools(r.schools)) - .catch(() => setMapSchools(initialSchools.schools)) - .finally(() => setIsLoadingMap(false)); - }, [resultsView, searchParams]); + fetchSchools(params, { cache: 'no-store', signal: controller.signal }) + .then(r => { + if (!current()) return; + mapParamsRef.current = paramsKey; + setMapSchools(r.schools); + }) + .catch(() => { + if (current()) setMapSchools(initialSchools.schools); + // No cache marker on failure: opening the map again retries. + }) + .finally(() => { if (current()) setIsLoadingMap(false); }); + return () => controller.abort(); + }, [resultsView, searchParams, initialSchools.schools]); // Fetch LA averages when secondary or mixed schools are visible useEffect(() => { @@ -305,19 +325,25 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed if (isLoadingMore || !hasMore) return; track('results_load_more', { next_page: currentPage + 1 }); setIsLoadingMore(true); + const scope = searchScope.current; + const controller = new AbortController(); + loadMoreController.current?.abort(); + loadMoreController.current = controller; + const current = () => !controller.signal.aborted && searchScope.current === scope; try { const params: Record = {}; searchParams.forEach((value, key) => { params[key] = value; }); params.page = currentPage + 1; params.page_size = initialSchools.page_size; - const response = await fetchSchools(params, { cache: 'no-store' }); + const response = await fetchSchools(params, { cache: 'no-store', signal: controller.signal }); + if (!current()) return; setAllSchools(prev => [...prev, ...response.schools]); setCurrentPage(response.page); setHasMore(response.page < response.total_pages); } catch { // silently ignore } finally { - setIsLoadingMore(false); + if (current()) setIsLoadingMore(false); } }; diff --git a/nextjs-app/jest.config.cjs b/nextjs-app/jest.config.cjs index 4bb2bb1..37fee00 100644 --- a/nextjs-app/jest.config.cjs +++ b/nextjs-app/jest.config.cjs @@ -9,6 +9,7 @@ const createJestConfig = nextJest({ const customJestConfig = { setupFilesAfterEnv: ['/jest.setup.js'], testEnvironment: 'jest-environment-jsdom', + modulePathIgnorePatterns: ['/.next/'], moduleNameMapper: { '^@/(.*)$': '/$1', }, diff --git a/nextjs-app/lib/places.ts b/nextjs-app/lib/places.ts index 29529f4..143fb7b 100644 --- a/nextjs-app/lib/places.ts +++ b/nextjs-app/lib/places.ts @@ -1,3 +1,5 @@ +import { APIFetchError } from './api'; + /** * Client for the places API. * @@ -69,7 +71,7 @@ const API = process.env.FASTAPI_URL || process.env.NEXT_PUBLIC_API_URL export async function fetchPlaces(): Promise { const res = await fetch(`${API}/places`, { next: { revalidate: 604800 } }); - if (!res.ok) return []; + if (!res.ok) throw new APIFetchError("Unable to load places", res.status); return (await res.json()).places ?? []; } @@ -79,6 +81,7 @@ export async function fetchPlace( const q = phase ? `?phase=${encodeURIComponent(phase)}` : ''; const res = await fetch(`${API}/places/${kind}/${slug}${q}`, { next: { revalidate: 604800 } }); - if (!res.ok) return null; + if (res.status === 404) return null; + if (!res.ok) throw new APIFetchError("Unable to load place", res.status); return res.json(); }