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