perf(detail): render school detail sections on the server #84
@@ -0,0 +1,232 @@
|
||||
# Detail pages: server/client split
|
||||
|
||||
**Date:** 2026-07-30
|
||||
**Status:** Approved, ready for implementation planning
|
||||
**Branch:** `perf/server-client-split`
|
||||
|
||||
## Problem
|
||||
|
||||
Every page of the site ships ~172 KB gzipped of JavaScript before any
|
||||
page-specific code loads. The 404 page costs that much; `/admissions`, a static
|
||||
content page with no data, costs 177 KB. A well-tuned Next.js app baseline is
|
||||
90-110 KB.
|
||||
|
||||
The cause is that nearly the whole site is a client component.
|
||||
`SchoolDetailView.tsx` is 65 KB of source marked `'use client'`;
|
||||
`SecondarySchoolDetailView.tsx` is 46 KB. Both are overwhelmingly presentational
|
||||
markup driven by props the server already fetched. Every stat tile, table row and
|
||||
Ofsted badge is serialised into the RSC payload *and* shipped again as JavaScript
|
||||
to hydrate.
|
||||
|
||||
Measured on a production build of `cab7b4f` (Next 16.1.6, Turbopack):
|
||||
|
||||
| Metric | Value |
|
||||
| --- | --- |
|
||||
| Baseline JS, every page | 172 KB gz across 8 chunks |
|
||||
| `/admissions` (no data) | 177 KB gz |
|
||||
| Chart.js chunk | 64 KB gz (correctly lazy) |
|
||||
| Leaflet chunk | 41 KB gz (correctly lazy) |
|
||||
| CSS | ~30 KB gz, route-split |
|
||||
|
||||
**Scope of this document.** The table above characterises the whole problem; this
|
||||
design addresses one part of it — the route-level client JS on school detail
|
||||
pages, the site's highest-traffic and most SEO-critical route. The shared
|
||||
baseline and the other routes are separate work, tracked elsewhere.
|
||||
|
||||
## What makes this tractable
|
||||
|
||||
Two properties of the existing code make an aggressive split low-risk.
|
||||
|
||||
**The section bodies are almost entirely static.** Across ~750 lines of sections
|
||||
in `SchoolDetailView` (lines 596-1350), the only interactivity is five charts
|
||||
(already `dynamic({ ssr: false })` islands), fourteen `<MetricTooltip>`
|
||||
instances (already a small client leaf), and two buttons driving the admissions
|
||||
year/trend toggle. Everything else renders props.
|
||||
|
||||
**The scroll-spy is already decoupled.** `SchoolDetailView.tsx:283` locates
|
||||
sections with `document.getElementById(id)`, and `navItems` (lines 258-267) is
|
||||
computed purely from data. The navigation does not care which component renders
|
||||
the sections, so server-rendered children work unchanged.
|
||||
|
||||
There is also a precedent to follow: `components/compare/` is already decomposed
|
||||
this way — one file per section plus a `sectionShared.tsx` of primitives.
|
||||
|
||||
## Architecture
|
||||
|
||||
A server component imported by a client component becomes a client component. So
|
||||
the sections cannot be children of a client `SchoolDetailView`. They are composed
|
||||
in `page.tsx` and passed *through* the client shell as `children` — the pattern
|
||||
already used at `app/page.tsx:98-99`.
|
||||
|
||||
The header and navigation sit above the sections, and the sections are plain
|
||||
siblings, so a single `children` slot suffices. Named slots were considered and
|
||||
rejected: more ceremony, no benefit when sections render in sequence.
|
||||
|
||||
```
|
||||
app/school/[slug]/page.tsx server; fetches details + nationalAverages
|
||||
│ computes isSecondary / isAllThrough / navItems
|
||||
└── <SchoolDetailShell schoolInfo navItems> 'use client' — the only large client file
|
||||
│ back link · header + details reveal · hero map · compare CTA
|
||||
│ sticky section nav · scroll spy · jump sheet
|
||||
└── children: <PrimarySchoolSections/> | <SecondarySchoolSections/> server
|
||||
└── components/school/ all server, mirroring components/compare/
|
||||
OfstedSection · ResultsSection · AdmissionsSection
|
||||
InclusionSection · HistorySection · SchoolLifeSection
|
||||
LocalAreaSection · FinancesSection · WellbeingSection
|
||||
sectionShared.tsx Section/Card/Row primitives
|
||||
```
|
||||
|
||||
Phase branching stays out of the route file: two thin server components
|
||||
(`PrimarySchoolSections`, `SecondarySchoolSections`) return the section sequence,
|
||||
and `page.tsx` picks one.
|
||||
|
||||
### Client leaves inside server sections
|
||||
|
||||
Importing a client component into a server component is the legal direction, so
|
||||
these stay client and are used from server sections unchanged:
|
||||
|
||||
- `MetricTooltip` / `InfoPopover` — exist, unchanged
|
||||
- `PerformanceChart`, `SatsChart`, `AdmissionsTrendChart` — exist, stay `dynamic({ ssr: false })`
|
||||
- `AdmissionsViewToggle` — **new**, ~25 lines; receives server-rendered year-view
|
||||
and trend-view as two props and swaps between them
|
||||
- `DeltaChip`, `SpecialSchoolNote` — already server-compatible (no `'use client'`)
|
||||
|
||||
### What stays in the client shell, and why
|
||||
|
||||
The shell is genuinely interactive throughout: `router.back()`, the details
|
||||
reveal, `IntersectionObserver` hero-CTA tracking, the nav overflow fade, the
|
||||
Escape-to-close sheet, and the comparison context. The goal is not to shrink the
|
||||
shell but to stop the other ~750 lines being attached to it.
|
||||
|
||||
### Section sharing
|
||||
|
||||
| Shared by both views | Primary only | Secondary only |
|
||||
| --- | --- | --- |
|
||||
| Ofsted, Admissions, History, Finances, and the shell | Results (KS2), Inclusion, School Life, Local Area | Wellbeing |
|
||||
|
||||
All-through schools render both the KS2 and KS4 section sets. The shared library
|
||||
makes this natural rather than duplicated.
|
||||
|
||||
### Prerequisite: national averages move server-side
|
||||
|
||||
`SchoolDetailView.tsx:173-179` fetches `/api/national-averages` in a `useEffect`,
|
||||
and `primaryAvg` / `secondaryAvg` feed the England-comparison deltas across most
|
||||
sections. Those sections cannot be server components until that data arrives as a
|
||||
prop. `SecondarySchoolDetailView.tsx:91-93` has the same fetch.
|
||||
|
||||
`page.tsx` will fetch it in parallel with the school details (the endpoint is
|
||||
backend-cached for an hour) and pass `nationalAvg` down. This is a prerequisite,
|
||||
not an optional extra. It independently removes a client round-trip per detail
|
||||
page and puts the deltas in the initial HTML instead of popping in after
|
||||
hydration.
|
||||
|
||||
### Layout
|
||||
|
||||
`Footer` is already a server component — no change needed. `Navigation` genuinely
|
||||
requires client (`usePathname` plus the comparison count) and stays as it is.
|
||||
This item is much smaller than the original review estimated.
|
||||
|
||||
### Unused dependency
|
||||
|
||||
`hooks/useSchools.ts`, `useFilters.ts`, `useMetrics.ts` and `useSchoolDetails.ts`
|
||||
are imported by nothing. They are the only consumers of `swr`. All four files and
|
||||
the `swr` dependency are deleted.
|
||||
|
||||
## Testing
|
||||
|
||||
`SchoolDetailView` will not exist as a single component afterwards, so
|
||||
characterization tests must not import it directly. A single render helper
|
||||
absorbs the change:
|
||||
|
||||
```
|
||||
__tests__/support/renderSchoolDetail.tsx the only file that changes at the split
|
||||
before: render(<SchoolDetailView {...props} />)
|
||||
after: render(<SchoolDetailShell {...}><PrimarySchoolSections {...} /></SchoolDetailShell>)
|
||||
```
|
||||
|
||||
Every assertion imports that helper and never changes. Assertions passing
|
||||
untouched across the refactor is the proof that behaviour is preserved.
|
||||
|
||||
**Coverage:** Ofsted panel (rating, date, report link); KS2 results rows and
|
||||
England deltas; KS4 Attainment 8, Progress 8 and EBacc; admissions Q&A and the
|
||||
year/trend toggle; inclusion and wellbeing figures; the history table; finances;
|
||||
and the conditional `navItems` set.
|
||||
|
||||
**Fixtures:** four shapes — primary, secondary, all-through, and special school.
|
||||
The special-school path drops the England comparison via `isSpecialSchool()` and
|
||||
is the site of a known past regression (PR #70), so it gets explicit coverage.
|
||||
|
||||
**Environment gaps to close in `jest.setup.js`:** jsdom provides neither
|
||||
`IntersectionObserver` (used twice in each view) nor `window.scrollTo`. Chart.js
|
||||
needs a canvas, so the chart components are mocked. `next/jest` already handles
|
||||
CSS modules, and `next/navigation` is already globally mocked. Section components
|
||||
are synchronous functions, so React Testing Library renders both the server and
|
||||
client pieces in jsdom without special handling.
|
||||
|
||||
**E2E:** `e2e/journeys.spec.ts` gains detail-page assertions. CLAUDE.md requires
|
||||
e2e updates in the same PR as user-facing changes.
|
||||
|
||||
## Commit sequence
|
||||
|
||||
One PR, but the diff is large, so it is structured to stay reviewable and
|
||||
bisectable:
|
||||
|
||||
1. `test:` jest.setup additions (IntersectionObserver, scrollTo), chart mocks, fixtures
|
||||
2. `test:` characterization tests against the current components — all green before anything moves
|
||||
3. `refactor:` server-side `nationalAverages` in `page.tsx`; drop both `useEffect` fetches
|
||||
4. `refactor:` extract `components/school/sectionShared.tsx` and the server sections (pure moves, no logic edits)
|
||||
5. `refactor:` add `SchoolDetailShell` and `AdmissionsViewToggle`; rewire `page.tsx`; delete the two old views
|
||||
6. `chore:` delete the four unused SWR hooks; drop `swr` from `package.json`
|
||||
7. `test:` extend `e2e/journeys.spec.ts` with detail-page assertions
|
||||
|
||||
Commits 4 and 5 move the bytes; commits 1 and 2 are the safety net that makes
|
||||
them safe. Commit 6 lands late so it cannot confuse a bisect.
|
||||
|
||||
## Verification
|
||||
|
||||
Before the work is claimed complete:
|
||||
|
||||
- `npm run typecheck`, `npm test` and `npm run build` all green
|
||||
- characterization tests passing **unmodified** from commit 2
|
||||
- before/after gzipped JS per route recorded in the PR description, measured the
|
||||
same way as the figures in the Problem section
|
||||
|
||||
## Success criteria
|
||||
|
||||
- `/school/[slug]` route-specific JS drops to roughly 40-60 KB gz, from a client
|
||||
tree currently built out of a 65 KB and a 46 KB source file
|
||||
- No visual or behavioural change on any of the four school shapes
|
||||
- One fewer client API round-trip per detail page
|
||||
|
||||
**This change does not reduce the 172 KB baseline.** That baseline is react-dom,
|
||||
the Next.js runtime, and the layout's client components (`Navigation`,
|
||||
`ComparisonProvider`, `ComparisonToast`) — none of which this design touches, as
|
||||
`Navigation` legitimately needs `usePathname` and the comparison count. The
|
||||
baseline is a separate piece of work; it is cited above only to establish why
|
||||
route-level client JS matters. Reporting must state the two numbers separately so
|
||||
the unchanged baseline is not read as a failure of this change.
|
||||
|
||||
Budget enforcement in CI was considered and deliberately deferred: measurement is
|
||||
recorded in the PR, but no new CI gate is added in this change.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- No visual redesign or copy changes to any section
|
||||
- No change to `Navigation`, `Footer`, or any other route
|
||||
- No CI bundle-budget gate
|
||||
- Bugs found mid-move are recorded, not fixed inline
|
||||
|
||||
## Risks
|
||||
|
||||
**DOM drift.** Moving ~750 lines of JSX risks subtle markup changes that CSS
|
||||
modules depend on. Commits 4 and 5 are strict moves and the section CSS moves
|
||||
with them; any styling change is out of scope.
|
||||
|
||||
**All-through schools.** The trickiest case: `isAllThrough` gates KS2 content
|
||||
back on for a secondary-phase school (`app/school/[slug]/page.tsx:150-154`). It
|
||||
gets a dedicated fixture.
|
||||
|
||||
**Large diff.** Accepted deliberately in favour of designing the shared section
|
||||
library against both consumers at once. Mitigated by the commit sequence above.
|
||||
|
||||
**`swr` removal** is safe — nothing imports those hooks.
|
||||
Reference in New Issue
Block a user