Compare commits

...
Author SHA1 Message Date
TudorandClaude Opus 5 d1a8596208 feat(analytics): measure the location layer, and stop calling it direct
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 11s
PR Checks / Build Frontend (no push) (pull_request) Successful in 44s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 40s
The location pages were only half-tracked. Umami counts a pageview for
each of the ~3,900 URLs automatically, but nothing else: components/
places contained no track() call, and place_viewed was not even a
declared event name.

The part that mattered was worse than a gap. getNavigationSource mapped
a same-origin referrer to a funnel source and had no case for /schools/,
so every school view arriving through the location layer fell through to
'direct' — the bucket you read as "typed the URL, no referrer". W2's
whole purpose is funnelling search traffic onto school pages, so the one
measurement that says whether it worked was reporting the wrong answer,
and reporting it confidently. Verified live against staging: expected
"place", received "direct".

/schools/ is checked before /school/. They differ by one letter and mean
different things — the location layer versus a single school — and a
prefix test in the wrong order silently merges them.

place_viewed carries kind, slug, phase and school_count. kind is the
reason it exists: whether to keep investing in these pages turns on
which sort earns engagement, and a pageview cannot say, because all four
families share the /schools/ prefix and only the registry knows which is
which. It is a client component because PlaceView is a server component;
one line in PlaceView covers all four families, since they all render
through it.

Both E2E journeys were verified failing against staging first — one
because place_viewed does not exist there, the other on the exact
"place" vs "direct" mismatch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-27 08:23:21 +01:00
tudor a3c09d9b67 Merge pull request 'fix(suggest): the dropdown reopened on top of the search results' (#131) from fix/suggest-reopens-over-results 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 50s
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) Successful in 1m37s
Reviewed-on: #131
2026-08-26 21:07:55 +00:00
tudor a7f4c86464 Merge pull request 'fix(search): the mobile hero search was indented by a card's padding' (#130) from fix/mobile-hero-search into main
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 12s
Stage (build -> staging -> E2E gate) / Build Frontend (Next.js) (push) Successful in 50s
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 5m50s
Reviewed-on: #130
2026-08-26 20:51:20 +00:00
TudorandClaude Opus 5 0804566736 fix(test): drop a committed scratch probe, and close a hole in the guard
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 11s
PR Checks / Build Frontend (no push) (pull_request) Successful in 44s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 9s
Code review, all three findings valid.

e2e/tests/__m.spec.ts was a throwaway probe used to measure the mobile
hero geometry. It asserts nothing, so it could never fail; it carried a
leftover `pick('form').constructor === Object ? null : null` that is
null either way and throws if no form matches; and it should never have
been committed. Deleted.

It survived because `rm -f e2e/tests/__m.spec.ts` ran with the shell
already inside e2e/, so the path resolved to e2e/e2e/tests/... — which
does not exist, and rm -f is silent about that. `git add -A` then swept
it in. I checked `git diff --stat` before committing, which lists only
tracked modifications and never shows an untracked file; `git status
--short` would have.

The scoping guard compared the last line of a rule's prelude against the
literal '.filterBar', so a regression written as a selector list —
`.filterBar, .other { padding }`, or the same split across two lines —
would have walked straight past the test meant to catch it. Selectors
are now split on commas and matched individually, and comments are
stripped first so a brace inside one cannot desynchronise the parse.

Verified against all three shapes: bare, inline comma list, and
multi-line comma list. Each is caught; each passes again once reverted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-26 21:49:48 +01:00
TudorandClaude Opus 5 55363cbd18 fix(suggest): the dropdown reopened on top of the search results
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m2s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 11s
PR Checks / Build Frontend (no push) (pull_request) Successful in 44s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 55s
Three staging-gate failures, two of them one real bug.

After a search, the results-page bar still holds the term in its input,
so on every render the query was >= 2 characters and the suggestion list
opened again — on top of the very results the search had just produced.
Playwright reported it as "<li role=option ...> intercepts pointer
events" while trying to click the first result; a reader would simply
have found their first result unclickable. Both the school-detail and
hero-map journeys failed on it, and neither is about autosuggest.

Suggestions now answer typing, not the mere presence of a value:
`hasTyped` gates the hook, is set on change, and is cleared when a
search is submitted or a suggestion is chosen. A pre-filled input makes
no request and shows no list.

Third failure was my test, not the product. An unphased place page
renders one table per phase, and an all-through school legitimately
appears in both — so the page's school links were never one alphabetical
run. The assertion collected them all together and only passed because
no town it picked had held an all-through school. When the data gave
Abbots Langley one, Breakspeare School appeared in the primary table and
again in the secondary, and the test failed on correct behaviour. It now
checks each table separately, and passes against the data that broke it.

Guards: a jest test that a pre-filled input neither fetches nor opens
(verified by reverting — it is the only one that fails), and an E2E
journey that submits a search and then requires the first result to be
clickable, which is the reader-facing version of the same thing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-26 21:38:16 +01:00
tudor 868eb344f5 Merge pull request 'fix(map): the hero map's fade to the header was hardcoded white' (#129) from fix/dark-map-fade 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 51s
Stage (build -> staging -> E2E gate) / Build Pipeline (Meltano + dbt + Airflow) (push) Successful in 12s
Stage (build -> staging -> E2E gate) / Deploy to Staging (push) Successful in 0s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Failing after 5m49s
Reviewed-on: #129
2026-08-26 20:32:45 +00:00
TudorandClaude Opus 5 0b15497c09 fix(search): the mobile hero search was indented by a card's padding
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 11s
PR Checks / Build Frontend (no push) (pull_request) Successful in 44s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 1m3s
Measured at 390px: the headline and lede sit at x=34, while the search
box, the hint and the location link all sat at x=48 and the field was
28px narrower than the copy above it.

The 14px came from `@media (max-width: 768px) { .filterBar { padding:
0.875rem } }`. That rule is for the results filter bar, which is a card
— background, border, shadow — and needs inner padding. The hero search
is not a card: .heroMode strips all of it, padding included.

Both selectors are specificity (0,1,0), so source order decides, and
.heroMode only wins because it is declared right after .filterBar. A
bare .filterBar rule inside a media query comes later and silently wins
instead. The two rules directly below this one in the same block were
already written as `.filterBar:not(.heroMode)`; this one was missed.

Scoping it aligns the search box, hint and location link to the same
left edge as the headline and gives the field back its 28px.

The location link also carried its own 6px of button padding, so its
label started further right than the hint even once the boxes agreed.
Pulled back with a negative margin, which keeps the tap target.

The guard is a stylesheet test: the failure is a plausible-looking
layout rather than a broken one, so nothing short of measuring or
looking would catch it. Verified by reverting: it names ".filterBar sets
padding".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-26 21:32:41 +01:00
tudor d55f6cce23 Merge pull request 'fix(suggest): let the dropdown out of the hero panel' (#128) from fix/hero-dropdown-clipping 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 50s
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 5m50s
Reviewed-on: #128
2026-08-26 20:23:33 +00:00
TudorandClaude Opus 5 3236efa846 fix(map): the hero map's fade to the header was hardcoded white
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m7s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 10s
PR Checks / Build Frontend (no push) (pull_request) Successful in 44s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 49s
The fade between the map band and the school header ramped through
rgba(255,255,255,...) and landed on var(--bg-card). In the light theme
that is white into white and invisible, as designed. In the dark theme
it climbed to 95% WHITE and then met a near-black card, putting a bright
band across the full width exactly where the map should dissolve into
the title.

Fading to the colour the gradient lands on is the whole trick, and it
only works if that colour is a token — so --bg-card-rgb now exists in
both theme blocks, matching the --hero-ground-rgb precedent.

Two more defects in the same file, same cause, found while in there:

The controls floating over the map paired a hardcoded white background
with color: var(--text-primary), which resolves to #E9EEF0 in dark —
near-white text on a near-white button. These deliberately do NOT follow
the theme, because the map tiles are light in both, so the ink is now
literal too and says why. A themed token is the wrong tool for a surface
that never changes.

The loading skeleton swept 50% white across var(--bg-secondary), which
is a bright flash every 1.4s on a dark page. It now sweeps toward the
card colour, a shade lighter than the ground in both themes.

The guard is a stylesheet test rather than a render test, because the
bug is invisible in the theme it was written for. Verified by reverting
each fix in turn: it names .fade and .openHint exactly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-26 21:20:57 +01:00
TudorandClaude Opus 5 d5a6db289d fix(suggest): let the dropdown out of the hero panel
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 11s
PR Checks / Build Frontend (no push) (pull_request) Successful in 45s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 54s
.heroPanel had overflow: hidden to clip its artwork and scrim to the
rounded corners. It clipped the suggestion dropdown too. Measured on
staging with the flag on: the list runs 482 to 802, the panel ends at
624 — so 178px of 320 was cut off, about half the options, with nothing
on screen to say anything was missing.

The two things that actually needed clipping now round themselves:
.heroArt gets border-radius: inherit plus its own overflow, and the
::before scrim inherits the radius. Below 860px the artwork is a band
flush with the top of the panel rather than a layer covering it, so it
takes the top two corners only — inheriting all four would leave it
floating with rounded corners against the copy.

Nothing else depended on the panel clipping: .valueProps below it is
entirely static, so a positioned dropdown paints above it without a
z-index fight.

The regression test asserts the LAST option is the element actually
painted at its own coordinates. toBeVisible() would not have caught
this — it checks for a non-empty box and visibility, and an ancestor's
overflow clips neither. elementFromPoint catches clipping and occlusion
alike.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-26 21:10:32 +01:00
tudor d8ccb5b733 Merge pull request 'feat(suggest): school autosuggest, and the rate-limit fix it needed first' (#127) from feat/school-autosuggest into main
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 20s
Stage (build -> staging -> E2E gate) / Build Frontend (Next.js) (push) Successful in 51s
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 5m49s
Reviewed-on: #127
2026-08-26 19:58:57 +00:00
tudor c3f044bd65 Merge pull request 'docs(flags): Unleash does not create flags by itself' (#126) from fix/flags-runbook into main
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 44s
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 2m10s
Stage (build -> staging -> E2E gate) / Deploy to Staging (push) Successful in 1s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Failing after 1m46s
Reviewed-on: #126
2026-08-26 19:48:58 +00:00
TudorandClaude Opus 5 59265f78b6 docs(flags): Unleash does not create flags by itself
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m6s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 35s
PR Checks / Build Frontend (no push) (pull_request) Successful in 46s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 1m15s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 35s
The runbook said a flag 'appears in the Unleash UI after the backend has
evaluated it once'. That is wrong. SDKs read definitions from the server
and never register anything, and metrics for an unknown flag are
discarded — so a declared flag is evaluated on every request, stays
False forever, and never shows up until someone creates it by hand.

Found the way these things usually are: staging had been running the
flag code for a while and the UI was still empty.

Also names the environment trap while here — each stack's token is
scoped to one environment, so toggling the other does nothing visible
and looks like the flag is broken.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-26 20:11:45 +01:00
15 changed files with 655 additions and 21 deletions

No files matched your search

+22 -3
View File
@@ -206,11 +206,30 @@ registry is orphaned and nothing reads it.
variable, and set `UNLEASH_URL` to `http://<UNLEASH_IP>:4242/api`.
5. Redeploy the application stacks.
### Adding a flag to Unleash
**Unleash does not create flags by itself.** The SDK reads definitions from the
server and never registers anything, and metrics for a flag the server has
never heard of are discarded. So a flag declared in `backend/flags.py` will be
evaluated on every request, stay `False` forever, and never appear in the UI
until someone creates it there by hand.
For each flag in the registry, create one in Unleash with:
- **Name** — character for character what `backend/flags.py` declares.
snake_case, no hyphens or spaces. A typo produces a flag that looks correct
in the UI and is read by nothing.
- **Type** — Release. No strategies, constraints or variants: these are plain
on/off switches, by design.
### Turning a feature on
Toggle the flag in the environment you want. Flags appear in the Unleash UI
after the backend has evaluated them once, so a newly declared flag shows up
shortly after the deploy that introduced it.
Toggle the flag in the environment matching the stack you mean: **development**
for staging, **production** for prod. The token in each stack is scoped to one
environment, so toggling the other one has no visible effect.
The SDK refreshes every 15 seconds, so the API reflects the change almost at
once; the pages follow on their own schedule, below.
A flip reaches school pages within about five minutes and place pages within
the hour. Next's ISR does the propagating — it revalidates a route at the
+144 -5
View File
@@ -2094,12 +2094,32 @@ test('a place page lists its schools alphabetically', async ({ page }) => {
expect(town).toBeTruthy();
await page.goto(`/schools/${town.slug}`);
const names = await page.locator('a[href^="/school/"]').allTextContents();
expect(names.length).toBeGreaterThan(1);
const sorted = [...names].sort((a, b) =>
a.toLowerCase().localeCompare(b.toLowerCase()));
expect(names).toEqual(sorted);
/*
* Per table, not per page.
*
* An unphased place page renders one table per phase, and an all-through
* school legitimately appears in both — so the page's school links are not
* one alphabetical run and never were. This assertion used to collect them
* all together and only passed because no town it picked happened to hold an
* all-through school; when the data gave Abbots Langley one, Breakspeare
* School showed up in the primary table and again in the secondary, and the
* test failed on correct behaviour.
*/
const tables = page.locator('table');
const tableCount = await tables.count();
expect(tableCount).toBeGreaterThan(0);
let checked = 0;
for (let i = 0; i < tableCount; i++) {
const names = await tables.nth(i).locator('a[href^="/school/"]').allTextContents();
if (names.length < 2) continue; // a one-row table says nothing about order
const sorted = [...names].sort((a, b) =>
a.toLowerCase().localeCompare(b.toLowerCase()));
expect(names, `table ${i + 1} is not alphabetical`).toEqual(sorted);
checked++;
}
expect(checked, 'no table had enough rows to check the ordering').toBeGreaterThan(0);
});
test('the rankings page still orders by score, not name', async ({ page }) => {
@@ -2112,6 +2132,66 @@ test('the rankings page still orders by score, not name', async ({ page }) => {
expect(scores).toEqual([...scores].sort((a: number, b: number) => b - a));
});
/*
* Analytics on the location layer.
*
* Umami counts a pageview for every one of these URLs already. What it cannot
* say is which *kind* of location page earns engagement, because all four
* families share the /schools/ prefix — and that is the question that decides
* whether to keep investing in them.
*/
/** Capture Umami events, with the real script blocked so it cannot clobber
* the stub. Must be called before the first navigation. */
async function captureEvents(page: Page) {
const events: Array<{ name: string; data: Record<string, unknown> }> = [];
await page.route('**/analytics.schoolcompare.co.uk/**', (route) => route.abort());
await page.exposeFunction('__capture',
(name: string, data: Record<string, unknown>) => { events.push({ name, data }); });
await page.addInitScript(() => {
(window as unknown as { umami: unknown }).umami = {
track: (name: string, data: unknown) =>
(window as unknown as { __capture: (n: string, d: unknown) => void })
.__capture(name, data),
};
});
return events;
}
test('a location page reports which kind of place it is', async ({ page }) => {
const events = await captureEvents(page);
const place = await firstPlaceOfKind(page, 'authority');
await page.goto(`/schools/authority/${place.slug}`);
await expect.poll(() => events.find((e) => e.name === 'place_viewed'),
{ timeout: 10_000 }).toBeTruthy();
const event = events.find((e) => e.name === 'place_viewed')!;
expect(event.data.kind).toBe('authority');
expect(event.data.slug).toBe(place.slug);
expect(event.data.phase).toBe('all');
});
test('a school reached from a location page is attributed to it, not to direct', async ({ page }) => {
/*
* The defect this was written for. getNavigationSource had no case for
* /schools/, so every school view that came through the location layer was
* filed as 'direct' — the bucket you read as "typed the URL". The one
* measurement that says whether ~3,900 SEO pages work was reporting the
* wrong answer, confidently.
*/
const events = await captureEvents(page);
const place = await firstPlaceOfKind(page, 'town');
await page.goto(`/schools/${place.slug}`);
await page.locator('a[href^="/school/"]').first().click();
await page.waitForURL(/\/school\//);
await expect.poll(() => events.find((e) => e.name === 'school_viewed'),
{ timeout: 10_000 }).toBeTruthy();
expect(events.find((e) => e.name === 'school_viewed')!.data.from).toBe('place');
});
/*
* School autosuggest (spec 2026-08-26).
*/
@@ -2163,6 +2243,65 @@ test('typing a school name suggests it, and choosing it opens that school', asyn
await expect(page).toHaveURL(/\/school\/\d+/);
});
test('the whole dropdown is reachable, not clipped by the hero', async ({ page }) => {
/*
* The hero panel had overflow: hidden to clip its artwork to the rounded
* corners, and it clipped the dropdown too — 320px of list against 145px of
* panel below the input, so roughly half was cut off with nothing to say so.
*
* toBeVisible() does not catch this: it checks the box is non-empty and not
* visibility:hidden, and an ancestor's overflow clips neither. The invariant
* that does catch it is that the LAST option is the thing actually painted
* at its own coordinates — which fails for clipping and for occlusion alike.
*/
test.skip(!(await autosuggestIsOn(page)),
'the school_autosuggest flag is off in this environment');
const { schools } = await (await page.request.get('/api/schools?page_size=1')).json();
test.skip(!schools?.length, 'no schools in this environment');
await page.goto('/');
await page.getByRole('combobox').first().fill(
(schools[0].school_name as string).slice(0, 6));
const options = page.getByRole('option');
await expect(options.first()).toBeVisible();
const count = await options.count();
const painted = await options.nth(count - 1).evaluate((el) => {
const r = el.getBoundingClientRect();
const hit = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2);
return { inside: el.contains(hit) || el === hit, bottom: Math.round(r.bottom) };
});
expect(painted.inside,
`the last option is not painted at its own coordinates (bottom ${painted.bottom}) `
+ '— an ancestor is clipping or covering the dropdown').toBeTruthy();
});
test('the dropdown does not survive into the results it produced', async ({ page }) => {
/*
* The bug that took the staging gate down, and it was not a test problem:
* after a search the results-page bar still holds the term, so the dropdown
* reopened on top of the results and swallowed the click on the first one.
* Playwright reported it as "<li role=option> intercepts pointer events"; a
* reader would simply have found their first result unclickable.
*/
test.skip(!(await autosuggestIsOn(page)),
'the school_autosuggest flag is off in this environment');
await page.goto('/');
await page.getByRole('combobox').first().fill('school');
await expect(page.getByRole('option').first()).toBeVisible();
await page.getByRole('button', { name: /Search/i }).first().click();
await page.waitForURL(/search=school/);
await expect(page.getByRole('listbox')).toHaveCount(0);
// And the results underneath are actually reachable, which is the point.
await page.locator('a[href^="/school/"]').first().click({ timeout: 15_000 });
await expect(page).toHaveURL(/\/school\//);
});
test('with autosuggest off, the search box is a plain input', async ({ page }) => {
test.skip(await autosuggestIsOn(page),
'the school_autosuggest flag is on in this environment');
@@ -3,10 +3,11 @@ import userEvent from '@testing-library/user-event';
import { FilterBar } from '@/components/FilterBar';
const push = jest.fn();
let searchParams = new URLSearchParams();
jest.mock('next/navigation', () => ({
useRouter: () => ({ push, replace: jest.fn(), prefetch: jest.fn() }),
usePathname: () => '/',
useSearchParams: () => new URLSearchParams(),
useSearchParams: () => searchParams,
}));
const FILTERS = {
@@ -24,6 +25,7 @@ beforeEach(() => {
phase: 'Primary', school_type: 'Community school' }] }),
})) as unknown as typeof fetch;
push.mockClear();
searchParams = new URLSearchParams();
});
afterEach(() => { global.fetch = realFetch; });
@@ -74,3 +76,35 @@ describe('FilterBar autosuggest', () => {
expect.stringContaining('search=brecknock')));
});
});
describe('FilterBar autosuggest does not reopen over results', () => {
it('stays shut when the input arrives pre-filled from the URL', async () => {
/*
* The results-page bar renders with the search term already in the input.
* Opening on that would drop the dropdown on top of the results the search
* just produced — which is exactly what happened: the first result became
* unclickable, because the list sat over it and swallowed the pointer.
*
* Suggestions answer typing, not the presence of a value.
*/
searchParams = new URLSearchParams('search=brecknock');
render(<FilterBar filters={FILTERS} autosuggest />);
expect(screen.getByRole('combobox')).toHaveValue('brecknock');
await new Promise((r) => setTimeout(r, 300)); // past the 200ms debounce
expect(global.fetch).not.toHaveBeenCalled();
expect(screen.queryByRole('listbox')).not.toBeInTheDocument();
});
it('closes the dropdown when the search is submitted', async () => {
render(<FilterBar filters={FILTERS} autosuggest />);
const input = screen.getByRole('combobox');
await userEvent.type(input, 'brecknock');
expect(await screen.findByRole('listbox')).toBeInTheDocument();
await userEvent.type(input, '{Enter}');
await waitFor(() =>
expect(screen.queryByRole('listbox')).not.toBeInTheDocument());
});
});
@@ -0,0 +1,45 @@
import { render } from '@testing-library/react';
import { TrackPlaceView } from '@/components/places/TrackPlaceView';
const trackMock = jest.fn();
jest.mock('@/lib/analytics', () => ({
track: (...args: unknown[]) => trackMock(...args),
getNavigationSource: () => 'search',
}));
describe('TrackPlaceView', () => {
beforeEach(() => trackMock.mockClear());
it('reports which kind of location page was viewed', () => {
/*
* `kind` is the reason this event exists. Whether to keep investing in the
* location layer turns on which *sort* of page earns engagement — towns,
* authorities or postcode districts — and a bare pageview cannot say,
* because all four families share the /schools/ prefix.
*/
render(<TrackPlaceView kind="authority" slug="kent" count={412} />);
expect(trackMock).toHaveBeenCalledWith('place_viewed', {
kind: 'authority', slug: 'kent', phase: 'all',
school_count: 412, from: 'search',
});
});
it('names the phase when the page is a phase variant', () => {
render(<TrackPlaceView kind="town" slug="brentwood" count={29} phase="primary" />);
expect(trackMock).toHaveBeenCalledWith('place_viewed',
expect.objectContaining({ phase: 'primary' }));
});
it('fires once, not once per render', () => {
const { rerender } = render(
<TrackPlaceView kind="town" slug="brentwood" count={29} />);
rerender(<TrackPlaceView kind="town" slug="brentwood" count={29} />);
expect(trackMock).toHaveBeenCalledTimes(1);
});
it('renders nothing', () => {
const { container } = render(
<TrackPlaceView kind="town" slug="brentwood" count={29} />);
expect(container).toBeEmptyDOMElement();
});
});
@@ -0,0 +1,78 @@
import fs from 'fs';
import path from 'path';
/**
* Guards against light-theme-only CSS.
*
* The site themes entirely through tokens redefined under
* `@media (prefers-color-scheme: dark)`. A hardcoded colour therefore does not
* fail loudly — it renders perfectly in the theme it was written for and
* quietly wrongly in the other, which nobody sees unless they happen to be in
* dark mode when they look.
*
* Both rules below are drawn from real defects in SchoolHeroMap.module.css,
* found by eye rather than by any test:
*
* - the map's fade to the header ramped through hardcoded white and landed on
* `var(--bg-card)`. Invisible in light; a bright band across the full width
* of a near-black card in dark.
* - the controls floating over the map paired a hardcoded white background
* with `color: var(--text-primary)`, which resolves to #E9EEF0 in dark —
* near-white text on a near-white button.
*/
const COMPONENTS = path.join(__dirname, '..', '..', 'components');
function stylesheets(dir: string): string[] {
return fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) return stylesheets(full);
return entry.name.endsWith('.module.css') ? [full] : [];
});
}
/** Innermost `selector { body }` pairs. Nested at-rules never match as rules,
* because their body contains braces. */
function rules(css: string): Array<{ selector: string; body: string }> {
return Array.from(css.matchAll(/([^{}]+)\{([^{}]*)\}/g), (m) => ({
selector: m[1].trim().split('\n').pop()!.trim(),
body: m[2],
}));
}
const HARDCODED_WHITE_BG = /background[^;]*(?:255,\s*255,\s*255|#fff\b|#ffffff\b)/i;
const THEMED_COLOR = /(?:^|[^-])color:\s*var\(--/;
const files = stylesheets(COMPONENTS);
describe('dark-theme safety', () => {
it('finds stylesheets to check', () => {
expect(files.length).toBeGreaterThan(0);
});
it('never pairs a hardcoded white background with a themed text colour', () => {
const offenders = files.flatMap((file) =>
rules(fs.readFileSync(file, 'utf8'))
.filter((r) => HARDCODED_WHITE_BG.test(r.body) && THEMED_COLOR.test(r.body))
.map((r) => `${path.relative(COMPONENTS, file)} ${r.selector}`));
// Either the surface follows the theme and so should the text, or it does
// not and the text must be literal too. Mixing them is how near-white text
// ends up on a near-white button.
expect(offenders).toEqual([]);
});
it('never fades to a themed colour through a hardcoded one', () => {
const offenders = files.flatMap((file) =>
rules(fs.readFileSync(file, 'utf8'))
.filter((r) => /linear-gradient/.test(r.body)
&& /var\(--bg-(card|primary|secondary)\)/.test(r.body)
&& /255,\s*255,\s*255|#fff\b/i.test(r.body))
.map((r) => `${path.relative(COMPONENTS, file)} ${r.selector}`));
// A gradient that lands on a token has to be made of that token, or the
// ramp and its destination disagree in one theme. Use the matching
// `--*-rgb` token for the transparent stops.
expect(offenders).toEqual([]);
});
});
@@ -0,0 +1,108 @@
import fs from 'fs';
import path from 'path';
/**
* The hero search and the results filter bar are the same component in two
* costumes. `.filterBar` is the card — background, border, shadow, padding —
* and `.heroMode` strips all of it so the search sits directly on the hero
* panel.
*
* Both selectors have specificity (0,1,0), so **source order decides**, and
* `.heroMode` only wins because it is declared immediately after. Any later
* bare `.filterBar` rule — which in practice means one inside a media query —
* silently wins instead, and the hero grows a card's padding back.
*
* That is exactly what happened: `@media (max-width: 768px) { .filterBar {
* padding: 0.875rem } }` re-added 14px in hero mode, indenting the search box,
* the hint and the location link 14px past the headline above them and costing
* the search field 28px of width on a 390px screen. The two rules directly
* below it in the same block were correctly written as
* `.filterBar:not(.heroMode)`; this one was missed, and nothing caught it
* because the result is a plausible-looking layout rather than a broken one.
*/
const CSS = path.join(__dirname, '..', '..', 'components', 'FilterBar.module.css');
/** Properties `.heroMode` resets. A later bare `.filterBar` rule setting any
* of these puts the card back on the hero. */
const RESET_BY_HERO_MODE = [
'background', 'border', 'border-radius', 'box-shadow', 'padding',
];
/**
* Comments are stripped before anything is parsed.
*
* A `{` or `}` inside a comment would otherwise desynchronise the brace walk
* below and the rule regex alike, and the selector text captured for each rule
* would carry the preceding comment along with it.
*/
function withoutComments(css: string): string {
return css.replace(/\/\*[\s\S]*?\*\//g, '');
}
/**
* The individual selectors in a rule's prelude.
*
* Split on commas, because a selector list is a list: `.filterBar, .other { }`
* applies to `.filterBar` just as surely as `.filterBar { }` does, and an
* earlier version of this guard compared the whole prelude against the literal
* string '.filterBar' — so writing the regression as a comma list, or across
* two lines, would have walked straight past it.
*/
function selectorsOf(prelude: string): string[] {
return prelude.split(',').map((sel) => sel.trim().replace(/\s+/g, ' '))
.filter(Boolean);
}
function mediaQueryBodies(css: string): string[] {
const bodies: string[] = [];
const re = /@media[^{]*\{/g;
let m: RegExpExecArray | null;
while ((m = re.exec(css)) !== null) {
// Walk braces from the opening one to find this at-rule's whole body.
let depth = 1;
let i = m.index + m[0].length;
const start = i;
while (i < css.length && depth > 0) {
if (css[i] === '{') depth++;
else if (css[i] === '}') depth--;
i++;
}
bodies.push(css.slice(start, i - 1));
}
return bodies;
}
describe('FilterBar hero-mode scoping', () => {
const css = withoutComments(fs.readFileSync(CSS, 'utf8'));
it('confirms heroMode still resets the card, which is what makes this matter', () => {
const hero = css.match(/\.heroMode\s*\{([^}]*)\}/);
expect(hero).not.toBeNull();
expect(hero![1]).toMatch(/padding:\s*0/);
});
it('never re-applies card styling to the hero from inside a media query', () => {
const offenders: string[] = [];
for (const body of mediaQueryBodies(css)) {
for (const rule of body.matchAll(/([^{}]+)\{([^{}]*)\}/g)) {
// Only a *bare* .filterBar is dangerous, and it is dangerous wherever
// it appears in a selector list. Scoped variants
// (`.filterBar:not(.heroMode)`) and descendants are fine.
const selectors = selectorsOf(rule[1]);
if (!selectors.includes('.filterBar')) continue;
for (const prop of RESET_BY_HERO_MODE) {
if (new RegExp(`(^|[;\\s])${prop}\\s*:`).test(rule[2])) {
offenders.push(`${rule[1].trim()} sets ${prop}`);
}
}
}
}
// Fix by scoping the rule as `.filterBar:not(.heroMode)`, the way the
// neighbouring rules in the same block already are.
expect(offenders).toEqual([]);
});
});
@@ -0,0 +1,64 @@
import { getNavigationSource } from '@/lib/analytics';
/** jsdom's document.referrer is read-only; redefining it is the way in. */
function referrer(url: string) {
Object.defineProperty(document, 'referrer', { value: url, configurable: true });
}
const ORIGIN = 'http://localhost';
describe('getNavigationSource', () => {
afterEach(() => referrer(''));
it('attributes a visit from a location page to the place layer', () => {
/*
* The one this was added for.
*
* W2 published ~3,900 location pages whose entire purpose is to funnel
* search traffic onto school pages. Before this case existed they fell
* through to 'direct' — so the location layer's contribution was not
* merely missing from the funnel, it was being counted in the bucket you
* read as "typed the URL". The measurement that decides whether W2 worked
* was confidently reporting the wrong answer.
*/
referrer(`${ORIGIN}/schools/barnet`);
expect(getNavigationSource()).toBe('place');
});
it.each([
['/schools/authority/kent', 'authority'],
['/schools/near/sw11', 'outcode'],
['/schools/brentwood/primary', 'phase variant'],
])('covers %s (%s)', (path) => {
referrer(`${ORIGIN}${path}`);
expect(getNavigationSource()).toBe('place');
});
it('still calls a school page "detail", one character away', () => {
// /school/ and /schools/ differ by one letter and mean different things.
// A prefix test written in the wrong order silently merges them.
referrer(`${ORIGIN}/school/100010-brecknock-primary-school`);
expect(getNavigationSource()).toBe('detail');
});
it.each([
['/', 'search'],
['/rankings', 'rankings'],
['/compare?urns=1,2', 'compare'],
])('leaves %s attributed as %s', (path, expected) => {
referrer(`${ORIGIN}${path}`);
expect(getNavigationSource()).toBe(expected);
});
it('treats an external referrer as direct', () => {
// Umami records the real referrer on the pageview; this field is only
// about internal navigation.
referrer('https://www.google.com/search?q=schools+in+barnet');
expect(getNavigationSource()).toBe('direct');
});
it('treats no referrer as direct', () => {
referrer('');
expect(getNavigationSource()).toBe('direct');
});
});
+4
View File
@@ -28,6 +28,9 @@
--bg-primary: #FAFAF8; /* Warm White */
--bg-secondary: #F5EFE6; /* Sand — hero panels, sunken rows */
--bg-card: #FFFFFF;
/* For gradients that have to fade to the card colour. A hardcoded white
ramp reads as a bright band against a dark card. */
--bg-card-rgb: 255, 255, 255;
--surface-inverse: #0F766E;
/* ── Ink ────────────────────────────────────────────────────────── */
@@ -234,6 +237,7 @@
--bg-primary: #111A20;
--bg-secondary: #16222A;
--bg-card: #18242C;
--bg-card-rgb: 24, 36, 44;
--surface-inverse: #E9EEF0;
--text-primary: #E9EEF0;
+19 -1
View File
@@ -413,7 +413,17 @@
/* ── Narrow ───────────────────────────────────────────────────────── */
@media (max-width: 768px) {
.filterBar {
/*
* Scoped, like the two rules below it.
*
* The results filter bar is a card — background, border, shadow — and needs
* inner padding. The hero's search is not a card: .heroMode zeroes the
* padding, border and background so the search sits directly on the panel.
* Unscoped, this rule put 14px back, which indented the search box, the hint
* and the location link 14px past the headline they sit under, and cost the
* search field 28px of width on a 390px screen.
*/
.filterBar:not(.heroMode) {
padding: 0.875rem;
}
@@ -457,6 +467,14 @@
align-items: flex-start;
}
/* Optical alignment: the button's own 6px of padding is what makes its
label start further right than the hint above it, even once both boxes
share a left edge. Pulling the padding back off lines the text up while
keeping the tap target. */
.heroMode .nearMeBtn {
margin-left: -0.375rem;
}
.geoError {
text-align: left;
}
+19 -2
View File
@@ -69,14 +69,28 @@ export function FilterBar({
const [omniValue, setOmniValue] = useState(initialOmniValue);
const suggestId = `school-suggest-${isHero ? "hero" : "bar"}`;
/*
* Suggestions answer typing, not the mere presence of a value.
*
* Without this the results-page bar reopened the dropdown over the results:
* after a search the input still holds the term, so on every render the
* query was >= 2 characters and the list opened again — on top of the very
* results the search had just produced, swallowing the click on the first
* one. The E2E gate caught it as "<li role=option> intercepts pointer
* events", but a reader would just have found the page unclickable.
*/
const [hasTyped, setHasTyped] = useState(false);
// Suppressed once the value parses as a postcode: the box takes a school
// name OR a postcode, and suggesting schools during postcode entry fights
// the user rather than helping them.
const suggestEnabled = autosuggest && !isValidPostcode(omniValue);
const suggestEnabled = autosuggest && hasTyped && !isValidPostcode(omniValue);
const { suggestions, open, activeIndex, setActiveIndex, close } =
useSchoolSuggest(omniValue, suggestEnabled);
const pickSuggestion = (s: Suggestion) => {
setHasTyped(false);
close();
track('search_submitted', {
query: s.school_name.toLowerCase(),
@@ -169,6 +183,9 @@ export function FilterBar({
const handleSearchSubmit = (e: React.FormEvent) => {
e.preventDefault();
// The search has been made; the suggestions that led to it are spent.
setHasTyped(false);
close();
if (!omniValue.trim()) {
updateURL({ search: "", postcode: "", radius: "" });
return;
@@ -271,7 +288,7 @@ export function FilterBar({
ref={inputRef}
type="search"
value={omniValue}
onChange={(e) => setOmniValue(e.target.value)}
onChange={(e) => { setOmniValue(e.target.value); setHasTyped(true); }}
onKeyDown={handleOmniKeyDown}
onBlur={close}
placeholder="School name or postcode"
+23 -1
View File
@@ -90,7 +90,18 @@
isolation: isolate;
background: var(--hero-ground);
border-radius: var(--radius-xl);
overflow: hidden;
/*
* Deliberately NOT overflow: hidden.
*
* It used to be, to clip the artwork and the scrim to the rounded corners —
* and it also clipped the search box's suggestion dropdown, which is 320px
* tall against 145px of panel below the input. Roughly half the list was cut
* off with no indication anything was missing.
*
* The two things that actually needed clipping round themselves instead, so
* the panel can let a dropdown out. Anything absolutely positioned inside
* this panel and taller than the space below it depends on this.
*/
}
.heroContent {
@@ -107,6 +118,10 @@
position: absolute;
inset: 0;
z-index: 0;
/* Rounds itself, because the panel no longer clips it. inset: 0 makes this
exactly the panel's own corners. */
border-radius: inherit;
overflow: hidden;
}
.heroArt picture,
@@ -143,6 +158,9 @@
inset: 0;
z-index: 1;
pointer-events: none;
/* Same reason as .heroArt: the panel stopped clipping, so the scrim keeps
its own corners rather than squaring off over the panel's. */
border-radius: inherit;
background: linear-gradient(
to right,
var(--hero-ground) 0%,
@@ -331,6 +349,10 @@
position: static;
order: -1;
height: 13rem;
/* Top corners only. Here the artwork is a band flush with the top of the
panel, not a layer covering it — inheriting all four would leave it
floating with rounded bottom corners against the copy below. */
border-radius: var(--radius-xl) var(--radius-xl) 0 0;
}
/* The band crop puts the schoolhouse at 73% across — reported by
scripts/build-hero-images.js, which derives it from the crop box rather
+31 -7
View File
@@ -47,7 +47,13 @@
width: 100%;
height: 100%;
background:
linear-gradient(100deg, rgba(255, 255, 255, 0) 40%, rgba(255, 255, 255, .5) 50%, rgba(255, 255, 255, 0) 60%) var(--bg-secondary);
/* Sweeps toward the card colour, which is a shade lighter than this
ground in both themes. Hardcoded white was a bright flash across a
dark page every 1.4s while the tiles loaded. */
linear-gradient(100deg,
rgba(var(--bg-card-rgb), 0) 40%,
rgba(var(--bg-card-rgb), .5) 50%,
rgba(var(--bg-card-rgb), 0) 60%) var(--bg-secondary);
background-size: 200% 100%;
animation: shimmer 1.4s infinite;
}
@@ -76,6 +82,15 @@
justify-content: center;
}
/*
* Controls that float ON the map.
*
* The map tiles are light in both themes, so these deliberately do NOT follow
* the theme — they follow the map. The literal ink below is the point: paired
* with a hardcoded white background, `color: var(--text-primary)` resolved to
* #E9EEF0 in the dark theme and put near-white text on a near-white button.
* A themed token is the wrong tool for a surface that never changes.
*/
.openHint {
display: inline-flex;
align-items: center;
@@ -85,7 +100,8 @@
border-radius: 999px;
font-size: 13px;
font-weight: 600;
color: var(--text-primary);
/* See "Controls that float ON the map" above. */
color: #1C2731;
background: rgba(255, 255, 255, .85);
-webkit-backdrop-filter: blur(6px);
backdrop-filter: blur(6px);
@@ -113,11 +129,18 @@
on top of the blend. */
z-index: 450;
pointer-events: none;
/* The card colour, not white.
This ramp was hardcoded white and ended at var(--bg-card). In the light
theme that is white into white and invisible, as intended. In the dark
theme it climbed to 95% WHITE and then met a near-black card — a bright
band across the full width, right where the map is supposed to dissolve
into the header. Fading to the same colour the gradient lands on is the
whole trick, and it only works if that colour is a token. */
background: linear-gradient(to bottom,
rgba(255, 255, 255, 0) 0%,
rgba(255, 255, 255, .35) 35%,
rgba(255, 255, 255, .75) 62%,
rgba(255, 255, 255, .95) 82%,
rgba(var(--bg-card-rgb), 0) 0%,
rgba(var(--bg-card-rgb), .35) 35%,
rgba(var(--bg-card-rgb), .75) 62%,
rgba(var(--bg-card-rgb), .95) 82%,
var(--bg-card) 100%);
}
@@ -134,7 +157,8 @@
border: none;
border-radius: 8px;
background: rgba(255, 255, 255, .92);
color: var(--text-primary);
/* See "Controls that float ON the map" above. */
color: #1C2731;
cursor: pointer;
box-shadow: 0 2px 10px rgba(var(--shadow-rgb), .2);
}
@@ -15,6 +15,7 @@ import { placeUrl, authoritySlug } from '@/lib/places';
import type { School } from '@/lib/types';
import { schoolUrl } from '@/lib/utils';
import { absoluteUrl } from '@/lib/site';
import { TrackPlaceView } from './TrackPlaceView';
import styles from './PlaceView.module.css';
interface Props {
@@ -165,6 +166,11 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
return (
<div className={styles.container}>
{/* One line, and all four place families are measured, because they all
render through this component. */}
<TrackPlaceView kind={place.kind} slug={place.slug}
count={place.count} phase={phase} />
<script
type="application/ld+json"
dangerouslySetInnerHTML={{ __html: JSON.stringify(jsonLd) }}
@@ -0,0 +1,47 @@
'use client';
/**
* Fires `place_viewed` once per location page.
*
* A separate client component because PlaceView is a server component and
* cannot call into the browser. It renders nothing — its whole job is the
* effect, which keeps the page itself server-rendered.
*
* Umami already counts a pageview for every one of these URLs, so this is not
* about traffic. It is about `kind`: whether to keep investing in the location
* layer turns on which *sort* of page earns engagement — towns, authorities,
* London localities or postcode districts — and a pageview cannot say, because
* all four families share the /schools/ prefix and only the registry knows
* which is which.
*/
import { useEffect } from 'react';
import { track, getNavigationSource } from '@/lib/analytics';
interface Props {
kind: string;
slug: string;
count: number;
phase?: 'primary' | 'secondary';
}
export function TrackPlaceView({ kind, slug, count, phase }: Props) {
useEffect(() => {
track('place_viewed', {
kind,
slug,
// "all" rather than omitting it, so the unphased page is a value in the
// same field rather than a gap that has to be interpreted.
phase: phase ?? 'all',
school_count: count,
// Internal navigation only. An arrival from Google reads as 'direct'
// here; Umami's own pageview referrer is where external attribution
// lives, and these pages exist to be arrived at externally.
from: getNavigationSource(),
});
// Once per place, not once per render.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [kind, slug, phase]);
return null;
}
+10 -1
View File
@@ -19,6 +19,7 @@ export type EventName =
| 'empty_results'
// Engagement
| 'school_viewed'
| 'place_viewed'
| 'section_nav_used'
| 'chart_metric_changed'
| 'metric_compared_in_rankings'
@@ -56,7 +57,10 @@ export function track(name: EventName, data?: Payload): void {
* Categorise where the user navigated from, for funnel attribution
* (mostly used on school_viewed). Only checks same-origin referrers.
*/
export function getNavigationSource(): 'search' | 'rankings' | 'compare' | 'detail' | 'direct' {
export type NavigationSource =
'search' | 'rankings' | 'compare' | 'detail' | 'place' | 'direct';
export function getNavigationSource(): NavigationSource {
if (typeof window === 'undefined' || !document.referrer) return 'direct';
try {
const ref = new URL(document.referrer);
@@ -65,6 +69,11 @@ export function getNavigationSource(): 'search' | 'rankings' | 'compare' | 'deta
if (p === '/' || p === '') return 'search';
if (p.startsWith('/rankings')) return 'rankings';
if (p.startsWith('/compare')) return 'compare';
// `/schools/` before `/school/`: they differ by one letter and mean
// different things — the location layer versus a single school. Checked
// first so the narrower-looking prefix cannot shadow it if either string
// is ever edited.
if (p.startsWith('/schools/')) return 'place';
if (p.startsWith('/school/')) return 'detail';
return 'direct';
} catch {