From 7650b16f62c693b70e95583c8f1092905d9edce4 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 21:46:28 +0100 Subject: [PATCH 1/3] feat(data): publish England only, dropping Welsh and overseas establishments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GIAS ships the whole UK plus overseas and offshore establishments. None of them carry comparable DfE performance data — Wales does not publish on the English measures at all — so every one of these pages rendered with null results, null Ofsted and null phase. There were 2,036 of them: 1,569 Welsh, 123 offshore (Jersey, Guernsey, Isle of Man, Gibraltar), 316 British schools overseas and 28 service children's schools. All 2,036 were being submitted to search engines, alongside 29 local authorities that existed in the filters purely to list them. Filter at the mart boundary rather than the view layer. dim_school and dim_location both exclude TypeOfEstablishment in {25, 26, 30, 37}, listed once as vars.non_england_school_type_codes. Everything downstream reads those two marts — search, the school page, /api/filters, rankings, Typesense and build_sitemap() — so one filter removes them from the site and the sitemap together, and Typesense drops them on its next rebuild since it recreates the collection and swaps the alias rather than upserting in place. coalesce rather than a bare NOT IN: a null type code would make the predicate null and drop the row silently, and an unknown type is not grounds for exclusion. No establishment has a null type today, but a future GIAS refresh could ship one and the loss would be invisible. assert_england_only_schools guards both directions: no excluded type survives in dim_school, and dim_location holds no URN dim_school lacks — the API inner-joins them, so the two filters drifting apart would silently shrink the corpus. Corpus goes from 27,229 schools to 25,193, and the authority list from 182 to 153. The 1,569 Welsh URLs now 404. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../specs/2026-08-20-seo-programme-design.md | 309 ++++++++++++++++++ e2e/tests/journeys.spec.ts | 101 ++++++ pipeline/transform/dbt_project.yml | 13 + .../transform/models/marts/dim_location.sql | 6 +- .../transform/models/marts/dim_school.sql | 5 + .../tests/assert_england_only_schools.sql | 28 ++ 6 files changed, 461 insertions(+), 1 deletion(-) create mode 100644 docs/superpowers/specs/2026-08-20-seo-programme-design.md create mode 100644 pipeline/transform/tests/assert_england_only_schools.sql diff --git a/docs/superpowers/specs/2026-08-20-seo-programme-design.md b/docs/superpowers/specs/2026-08-20-seo-programme-design.md new file mode 100644 index 0000000..1986897 --- /dev/null +++ b/docs/superpowers/specs/2026-08-20-seo-programme-design.md @@ -0,0 +1,309 @@ +# SEO Programme — Design + +Date: 2026-08-20 +Status: awaiting review + +## Problem + +schoolcompare ranks second for "school compare" — an exact match for the +brand and the domain. It ranks poorly for "compare schools", "school +comparison" and "schools near me". The first is a naming artefact and +transfers to nothing; the rest are the queries that actually carry parent +demand. + +The cause is structural, not editorial. The site publishes five route +families: + + / /compare /rankings /admissions /school/[slug] + +Location intent has no landing page at all. Every competitor outranking us +on those queries wins with programmatic location pages: + +| Competitor | URL pattern | +|---------------|--------------------------------------------| +| School Guide | `/best-schools-in/manchester` | +| Locrating | `/the-best-primary-schools-in-Manchester_…`| +| FindMySchool | `/best-primary-schools/manchester` | +| Snobe | `/best-primary-schools/manchester` | +| School Atlas | `/guides/best-primary-schools-manchester` | + +"Schools near me" is a local-intent query. Google resolves it against the +user's coordinates and serves pages that are *about a place*. A national +homepage cannot win it. No title or description change fixes this; only +pages Google can localise will. + +## Baseline (measured 2026-08-20, production API) + +| Measure | Value | +|--------------------------------------------|---------| +| Unique schools | 27,229 | +| URLs in sitemap.xml | 27,232 | +| Schools with 2024/25 performance data | 21,266 | +| Schools with **no** current performance data| ~5,963 (22%) | +| Welsh establishments (all metrics null) | 1,569 | +| Overseas / offshore establishments | 467 | +| Static URLs in sitemap | 3 | +| Routes setting a canonical | 1 of 5 | + +Three findings from that table drive the plan. + +**We submit ~6,000 thin pages to Google.** `build_sitemap()` +(`backend/app.py:73`) enumerates every URN regardless of whether the school +has any data. Welsh establishments return `school_type: "Welsh +establishment"` with every performance metric, Ofsted grade and phase field +null. Overseas and offshore establishments ("BFPO Overseas Establishments", +"Gibraltar Overseas Establishments", "Jersey Offshore Establishments") are +in the local-authority list too. At 22% of the submitted corpus this is a +site-wide quality signal problem and a crawl-budget waste, not a rounding +error. + +W1 item 4 removes 2,036 of those — every non-England establishment — taking +the corpus to 25,193. The 3,927 that remain are English schools with no +current data: mostly newly opened, special, nursery or alternative provision. +Those are a template problem, not a corpus problem, and item 5 handles them +separately. + +**The homepage is its own competitor.** `app/page.tsx` accepts eleven search +params (`search`, `local_authority`, `school_type`, `phase`, `page`, +`postcode`, `radius`, `sort`, `gender`, `admissions_policy`, +`has_sixth_form`) and sets no canonical. Every filter combination is a +crawlable near-duplicate of the single page we are asking to rank for +"compare schools". + +**School pages are near-orphans.** Reachable from the sitemap and from site +search, but almost nothing links to them contextually, so they accrue no +internal authority. + +Also noted: the sitemap emits invented `priority` values and no `lastmod`. +Google ignores `priority` and `changefreq` entirely; `lastmod` is the field +it does read, and we omit it. + +## Keyword clusters + +Ranked by judgement of UK parent search behaviour and by the competitive +SERP evidence above. Google Search Console is connected, so cluster +priorities are to be re-derived from measured impressions before build +starts (see Workstream 0). + +**C1 — Head "compare" terms.** compare schools · school comparison · school +comparison tool · compare school performance · compare primary schools · +compare secondary schools · compare two schools + +**C2 — League tables and rankings.** primary school league tables · +secondary school league tables · school league tables 2026 · SATs results by +school · GCSE results by school · KS2 league tables · Progress 8 rankings · +best primary schools in [town] · top 10 primary schools in [LA] + +**C3 — Local / near me.** schools near me · primary schools near me · +secondary schools near me · best schools near me · good schools near me · +schools in [town] · primary schools in [LA] · schools near [postcode] · +[postcode] school catchment + +**C4 — Individual school long tail.** [school] ofsted · [school] SATs +results · [school] catchment area · [school] reviews · [school] URN + +**C5 — Admissions.** primary school admissions 2027 · national offer day +2027 · school application deadline · school admissions appeal · +oversubscription criteria · distance criteria school admissions · didn't get +first choice school · school admissions [LA] + +**C6 — Metric explainers.** what is a good SATs score · what is Progress 8 · +what is Attainment 8 · expected standard KS2 meaning · scaled score +explained · Ofsted grades explained · Ofsted report cards · pupil premium +explained + +**C7 — Head to head.** [school A] vs [school B] · academy vs community +school · grammar school vs comprehensive · faith school vs community school + +## Workstreams + +### W0 — Measure before touching anything + +Export a Google Search Console baseline: impressions, clicks, average +position and CTR by query and by page, for the trailing 16 months. Bucket +queries into C1–C7. This sets the counterfactual — without it, no later +claim about lift is defensible, because school-search traffic is strongly +seasonal (results day in December, offer day in March/April). + +Re-rank C1–C7 against measured impressions and adjust the sequence below if +the data disagrees with the judgement calls. + +### W1 — Crawl hygiene and index sanity + +Cheap, and it unblocks everything after it. Adding 5,000 pages on top of a +corpus that is 22% thin would compound the existing problem. + +1. Canonical on every route. `/`, `/rankings`, `/compare` and `/admissions` + currently set none. +2. The homepage canonicalises to `/` regardless of search params. +3. `/compare?urns=…` gets `noindex, follow` — it is an unbounded parameter + space with no standalone value. +4. **England only — DONE.** Wales, the Crown Dependencies, Gibraltar and the + service/overseas schools are removed from the corpus at the mart boundary, + not hidden at the view layer. `dim_school` and `dim_location` both exclude + `TypeOfEstablishment` in {25, 26, 30, 37} — Offshore schools, Service + children's education, Welsh establishment, British schools overseas — + listed once as `vars.non_england_school_type_codes` in `dbt_project.yml`. + That removes 2,036 establishments and 29 local authorities, and because + `build_sitemap()` reads the same marts, it drops them from the sitemap in + the same stroke. `assert_england_only_schools` fails the pipeline if a GIAS + refresh reintroduces them or if the two models drift apart. +5. Prune the remaining thin pages: exclude any school with no performance data + **and** no Ofsted record. Distinct from item 4 — these are English schools + with nothing yet to show, so the fix may be a better template rather than + removal. +6. Rebuild the sitemap as a sitemap **index**: one child per page family, + real `lastmod` from the data-load timestamp, `priority` and `changefreq` + dropped. + +### W2 — The location layer + +The dominant lever. `dim_location` already carries `town`, `county`, +`local_authority_name`, `parliamentary_constituency`, `latitude`, +`longitude` and `postcode`, so no new ingestion is required. + +Routes: + + /schools/[la] e.g. /schools/manchester + /schools/[la]/primary + /schools/[la]/secondary + /best-primary-schools/[town] + /best-secondary-schools/[town] + /schools/near/[outcode] e.g. /schools/near/m20 + /schools/near-me geolocating hub + +**Thin-page threshold: generate a town or outcode page only where at least +five schools have current performance data.** Below that, 301 to the parent +LA page. This is the single most important constraint in the workstream — +it is what separates a location layer from index bloat. + +Each page must earn its place with content a parent would actually use, not +a template shell: + +- H1 matching the query intent ("Best primary schools in Manchester") +- Counts framed usefully: "137 primary schools, 9 rated Outstanding" +- A ranked table of the top 20 on the headline metric +- Local average against the England average +- Ofsted grade distribution +- Map +- Links to neighbouring towns and to the parent LA +- An FAQ block (feeds `FAQPage` in W4) +- Links to every school page in scope — this is what de-orphans W1's corpus + +Sizing estimate: ~150 usable LAs × 3 ≈ 450; towns clearing the threshold +≈ 1,200 × 2 ≈ 2,400; outcodes ≈ 2,300. Roughly **5,000 new pages**, +comfortably inside a sitemap index and well under the per-file 50,000 limit. + +### W3 — Make rankings indexable + +`/rankings` is driven entirely by query params, so Google indexes +approximately one page where there should be hundreds. + + /rankings/[phase]/[metric] + /rankings/[phase]/[metric]/[la] + +The interactive filter UI stays; its state moves into real paths. Param +forms canonicalise to the clean path. This is the direct play for C2. + +### W4 — Structured data and internal linking + +- Replace the bare `EducationalOrganization` on school pages with `School`, + and populate it properly. +- `BreadcrumbList` site-wide. +- `ItemList` on every rankings and location page. +- `FAQPage` on admissions and on location pages. +- New school-page modules: "Other schools in [town]", "Nearby schools", + "Compare with similar schools". Each links out to W2 and W3 pages, which + is what circulates authority instead of stranding it. + +Explicitly **not** doing `Dataset` or `AggregateRating` — no review corpus +exists, and fabricating one would be both useless and a policy violation. + +### W5 — Admissions expansion + +One static page currently carries an entire cluster. + + /admissions/[la] per-authority deadlines and offer day + /admissions/appeals + /admissions/national-offer-day + +`school admissions [LA]` is high-intent and highly seasonal; per-authority +pages are the natural unit. + +### W6 — Explainer content + + /guides/progress-8 + /guides/attainment-8 + /guides/sats-scaled-scores + /guides/ofsted-grades + /guides/expected-standard + +Each links into the corresponding W3 rankings page. Cheap to build, and it +is what gives the metric vocabulary enough topical weight to support C1–C4. + +### W7 — Head-to-head pages + + /compare/[school-a]-vs-[school-b] + +C7 is uncontested and native to the product. It is also the easiest way to +destroy everything W1 fixes: 27,229 schools generate 370 million pairs. +**Curated pairs only** — same town, both with current data, both with real +search demand — capped in the low thousands. Gated behind evidence that W2 +is indexing cleanly. + +### W8 — Metadata rewrite for C1 + +Current homepage title is `schoolcompare | Compare every school in England`, +which spends the most valuable position on the brand. Rewrite the homepage, +rankings and compare titles and descriptions around C1 phrasing. Small +change, and the cheapest item in the programme. + +## Sequencing + + W0 → W1 → W2 → W3 → W4 → W5 → W6 → (W7 if W2 indexes cleanly) + +W1 before W2 is not negotiable: adding pages to a corpus that is 22% thin +compounds the problem rather than diluting it. + +This spec is a programme, not a single implementation plan. Each workstream +gets its own plan and its own PR; W2 will likely need several. Only W0 and W1 +are ready to plan against today — the rest should be re-read after W0's +Search Console baseline lands, because that data may reorder them. + +## Testing + +Per CLAUDE.md, user-facing behaviour changes extend the `e2e/` journeys in +the same PR. Each workstream adds: + +- W1: canonical present and correct on every route; `/compare?urns=` carries + `noindex`; sitemap excludes a known dataless URN. +- W2: a known LA, town and outcode page renders with the expected school + count; a below-threshold town redirects to its LA. +- W3: a clean rankings path renders; the param form canonicalises to it. +- W4: JSON-LD parses and validates against the declared types. + +## Risks + +**Index bloat.** The failure mode of every programmatic SEO programme. The +five-school threshold, the W1 prune and the W7 gate are the three controls. + +**Helpful-content exposure.** Google's stance on templated location pages +has hardened. The mitigation is that each page carries genuinely local +computed data — real counts, real distributions, real local-vs-national +comparison — rather than a name substituted into boilerplate. + +**Build cost.** School pages already use ISR with a 7-day revalidate and +`PRERENDER_SCHOOLS` gating full prerender. 5,000 more routes need the same +treatment; full static generation of 32,000 pages is likely impractical in +CI. + +**Seasonality.** Results day and offer day dominate the traffic curve. +Judging the programme on a mid-summer window would misread it in either +direction. W0's baseline must be year-on-year, not month-on-month. + +## Open questions + +1. Catchment areas are Locrating's moat and a strong C3 driver + (`[postcode] school catchment`). `fact_admissions` carries admission + distances. Is deriving approximate catchment a later workstream, or out + of scope? diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 9c18dc8..c1c8a8b 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1449,3 +1449,104 @@ test('no single section dominates the height of a school page', async ({ page }) + sections.map((s) => `${s.id}=${s.h}`).join(', '), ).toBeLessThan(2.5); }); + +/* + * England-only corpus. + * + * GIAS ships the whole UK plus overseas and offshore establishments, none of + * which carry comparable DfE performance data. dim_school and dim_location + * exclude them (vars.non_england_school_type_codes in dbt_project.yml), which + * keeps them out of the site, the filter lists and the sitemap together. + * + * These assert the published surface, not the warehouse: the dbt test + * assert_england_only_schools guards the marts, and these guard what the + * environment actually serves once the marts have been rebuilt. + */ + +const WELSH_AUTHORITIES = [ + 'Blaenau Gwent', 'Bridgend', 'Caerphilly', 'Cardiff', 'Carmarthenshire', + 'Ceredigion', 'Conwy', 'Denbighshire', 'Flintshire', 'Gwynedd', + 'Isle of Anglesey', 'Merthyr Tydfil', 'Monmouthshire', 'Neath Port Talbot', + 'Newport', 'Pembrokeshire', 'Powys', 'Rhondda Cynon Taf', 'Swansea', + 'Torfaen', 'Vale of Glamorgan', 'Wrexham', +]; + +const NON_ENGLAND_AUTHORITIES = [ + 'BFPO Overseas Establishments', 'Fieldwork Overseas Establishments', + 'Gibraltar Overseas Establishments', 'Guernsey Offshore Establishments', + 'Isle of Man Offshore Establishments', 'Jersey Offshore Establishments', + 'Scotland Offshore Establishments', +]; + +const NON_ENGLAND_TYPES = [ + 'Welsh establishment', 'Offshore schools', + "Service children's education", 'British schools overseas', +]; + +test('the local authority filter offers no Welsh or overseas authority', async ({ page }) => { + const res = await page.request.get('/api/filters'); + expect(res.ok()).toBeTruthy(); + const { local_authorities: las } = await res.json(); + + expect(Array.isArray(las)).toBeTruthy(); + // Guards against the list being empty, which would pass the check below + // for the wrong reason. + expect(las.length).toBeGreaterThan(100); + + const leaked = [...WELSH_AUTHORITIES, ...NON_ENGLAND_AUTHORITIES] + .filter((la) => las.includes(la)); + expect(leaked, `non-England authorities still offered: ${leaked.join(', ')}`) + .toEqual([]); +}); + +test('the school type filter offers no non-England establishment type', async ({ page }) => { + const res = await page.request.get('/api/filters'); + expect(res.ok()).toBeTruthy(); + const { school_types: types } = await res.json(); + + expect(Array.isArray(types)).toBeTruthy(); + expect(types.length).toBeGreaterThan(10); + + const leaked = NON_ENGLAND_TYPES.filter((t) => types.includes(t)); + expect(leaked, `non-England types still offered: ${leaked.join(', ')}`) + .toEqual([]); +}); + +test('searching a Welsh authority by name returns no schools', async ({ page }) => { + // Cardiff held 144 Welsh establishments and nothing else, so the authority + // should now be absent from the corpus entirely rather than merely thinned. + const res = await page.request.get('/api/schools?local_authority=Cardiff&page_size=1'); + expect(res.ok()).toBeTruthy(); + const body = await res.json(); + expect(body.total ?? (body.schools ?? []).length).toBe(0); +}); + +test('a Welsh school URL 404s while an English one still resolves', async ({ page }) => { + // Paired on purpose: the Welsh assertion alone would also pass if the whole + // site were down, which is the failure this test most needs to distinguish. + const english = await page.request.get('/api/schools?search=primary&per_page=1'); + expect(english.ok()).toBeTruthy(); + const [first] = (await english.json()).schools ?? []; + expect(first, 'no English school available to compare against').toBeTruthy(); + + const good = await page.goto(`/school/${first.urn}-x`); + expect(good?.status(), 'an English school should still resolve').toBeLessThan(400); + + // Adamsdown Primary School, Cardiff — a Welsh establishment (URN 401559). + const welsh = await page.goto('/school/401559-adamsdown-primary-school'); + expect(welsh?.status(), 'a Welsh school should no longer resolve').toBe(404); +}); + +test('the sitemap submits no Welsh or overseas school', async ({ page }) => { + const res = await page.request.get('/sitemap.xml'); + expect(res.ok()).toBeTruthy(); + const xml = await res.text(); + + const urlCount = (xml.match(//g) ?? []).length; + expect(urlCount, 'sitemap looks empty or truncated').toBeGreaterThan(1000); + + // 401559 (Cardiff) and 402426 (ACT Schools, Cardiff) were both submitted + // before the England-only filter landed. + expect(xml).not.toContain('/school/401559'); + expect(xml).not.toContain('/school/402426'); +}); diff --git a/pipeline/transform/dbt_project.yml b/pipeline/transform/dbt_project.yml index ba23641..ff62b6f 100644 --- a/pipeline/transform/dbt_project.yml +++ b/pipeline/transform/dbt_project.yml @@ -11,6 +11,19 @@ seed-paths: ["seeds"] target-path: "target" clean-targets: ["target", "dbt_packages"] +# schoolcompare publishes England only. GIAS ships the whole UK plus overseas +# and offshore establishments, none of which have comparable DfE performance +# data — Wales does not publish on the English measures at all. Excluding them +# at the mart boundary keeps them out of the site, the filter lists and the +# sitemap together. Codes are TypeOfEstablishment, see seeds/gias_code_names.csv. +# 25 = Offshore schools (Jersey, Guernsey, Isle of Man, Gibraltar) +# 26 = Service children's education (BFPO, Fieldwork Overseas) +# 30 = Welsh establishment +# 37 = British schools overseas +# dim_school, dim_location and assert_england_only_schools all read this list. +vars: + non_england_school_type_codes: [25, 26, 30, 37] + models: school_compare: staging: diff --git a/pipeline/transform/models/marts/dim_location.sql b/pipeline/transform/models/marts/dim_location.sql index b05e8a7..3495b14 100644 --- a/pipeline/transform/models/marts/dim_location.sql +++ b/pipeline/transform/models/marts/dim_location.sql @@ -31,5 +31,9 @@ select else null end as longitude from {{ ref('stg_gias_establishments') }} s --- Must match dim_school's status filter exactly (the API inner-joins the two). +-- Must match dim_school's status and England filters exactly (the API +-- inner-joins the two). where s.status_code in (1, 3) +-- coalesce, not a bare NOT IN: a null type code would make the predicate +-- null and drop the row silently. Unknown type is not grounds for exclusion. +and coalesce(s.school_type_code, -1) not in ({{ var('non_england_school_type_codes') | join(', ') }}) diff --git a/pipeline/transform/models/marts/dim_school.sql b/pipeline/transform/models/marts/dim_school.sql index 86dfa2e..bcb246c 100644 --- a/pipeline/transform/models/marts/dim_school.sql +++ b/pipeline/transform/models/marts/dim_school.sql @@ -91,3 +91,8 @@ left join {{ ref('int_ofsted_latest') }} o on s.urn = o.urn -- 1 = Open; 3 = Open, but proposed to close (still operating; drops out when -- GIAS flips to Closed — marts fully rebuild each run). where s.status_code in (1, 3) +-- England only. dim_location must apply this filter identically (the API +-- inner-joins the two). See vars in dbt_project.yml for what the codes are. +-- coalesce, not a bare NOT IN: a null type code would make the predicate +-- null and drop the row silently. Unknown type is not grounds for exclusion. +and coalesce(s.school_type_code, -1) not in ({{ var('non_england_school_type_codes') | join(', ') }}) diff --git a/pipeline/transform/tests/assert_england_only_schools.sql b/pipeline/transform/tests/assert_england_only_schools.sql new file mode 100644 index 0000000..0356e29 --- /dev/null +++ b/pipeline/transform/tests/assert_england_only_schools.sql @@ -0,0 +1,28 @@ +-- Custom test: the published corpus is England only. +-- +-- GIAS ships the whole UK plus overseas and offshore establishments. None of +-- them carry comparable DfE performance data, so dim_school and dim_location +-- filter them out (see vars.non_england_school_type_codes in dbt_project.yml). +-- This test is the guard: a GIAS refresh that reintroduces them, or an edit +-- that drops the filter from one of the two models, fails the pipeline rather +-- than quietly republishing ~2,000 dataless pages to the site and the sitemap. + +select + urn, + school_name, + school_type_code +from {{ ref('dim_school') }} +where school_type_code in ({{ var('non_england_school_type_codes') | join(', ') }}) + +union all + +-- dim_location is inner-joined to dim_school by the API, so it must filter +-- identically. Anything here that dim_school does not have means the two +-- models have drifted apart. +select + l.urn, + null::text as school_name, + null::integer as school_type_code +from {{ ref('dim_location') }} l +left join {{ ref('dim_school') }} s on l.urn = s.urn +where s.urn is null -- 2.54.0 From e6048c9ca60a1e6e3d8ea9464e5f96bdc3a9dbfd Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:05:00 +0100 Subject: [PATCH 2/3] docs(seo): implementation plan for W1, crawl hygiene and sitemap Five tasks: one canonical host, canonicals on every route, noindex on parameterised comparisons, a sitemap that drops dataless schools and invented priorities, and a per-family sitemap index. Planning turned up a fault the spec had missed: the apex 301s to www, but metadataBase, the school-page canonical, robots.txt's Sitemap: line and every sitemap named the apex. Task 1 fixes it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../plans/2026-08-20-w1-crawl-hygiene.md | 1233 +++++++++++++++++ 1 file changed, 1233 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md diff --git a/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md b/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md new file mode 100644 index 0000000..7e3c269 --- /dev/null +++ b/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md @@ -0,0 +1,1233 @@ +# W1: Crawl Hygiene and Sitemap — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make every page declare one correct canonical URL, stop the homepage +competing with itself across eleven search params, and turn the sitemap from a +flat 27k-URL dump into a per-family index carrying honest `lastmod`. + +**Architecture:** Two independent halves. The frontend half is Next.js +`Metadata` — a single shared canonical-host constant, `alternates.canonical` on +every route, and `generateMetadata` on `/compare` so a parameterised comparison +goes `noindex`. The backend half is `build_sitemap()` in `backend/app.py`, +which grows a dataless-school filter and splits into a sitemap index with +child sitemaps, proxied through Next by a catch-all route. + +**Tech Stack:** Next.js 15 App Router (`Metadata` / `generateMetadata`), Jest + +jsdom for frontend unit tests, FastAPI + pandas for the sitemap, pytest with +`monkeypatch`-injected DataFrames for backend tests, Playwright for the e2e gate. + +**Spec:** `docs/superpowers/specs/2026-08-20-seo-programme-design.md` (workstream W1) + +## Global Constraints + +- **Canonical host is `https://www.schoolcompare.co.uk`.** The apex 301s to + `www` at Cloudflare (verified 2026-08-20). Every canonical, `metadataBase`, + sitemap `` and `robots.txt` `Sitemap:` line must use `www` so no + canonical points at a redirect. +- **Prerequisite:** branch `feat/england-only-corpus` must be merged first. + This plan's sitemap counts assume the England-only corpus of 25,193 schools. +- **Never push to `main`.** Feature branch and a PR, per `CLAUDE.md`. +- **User-facing behaviour changes extend `e2e/tests/journeys.spec.ts` in the + same PR** — the journeys gate staging and promotion. +- **Backend test command** (there is no local pytest; this builds an isolated env): + ```bash + uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests -q + ``` +- **Frontend test command:** `cd nextjs-app && npm test` +- **Do not start a local server to test** (`CLAUDE.md`). Backend behaviour is + tested through `TestClient`, frontend through Jest, integration through the + staging e2e run. + +--- + +### Task 1: One canonical host, used everywhere + +Today `metadataBase`, the school-page canonical, the backend `BASE_URL` and +`robots.ts` all say `https://schoolcompare.co.uk`, which 301s to `www`. A +canonical that points at a redirect is a wasted signal. This task introduces +one constant and routes every producer of an absolute URL through it. + +**Files:** +- Create: `nextjs-app/lib/site.ts` +- Create: `nextjs-app/__tests__/lib/site.test.ts` +- Modify: `nextjs-app/app/layout.tsx` (the `metadataBase` and `openGraph.url` keys) +- Modify: `nextjs-app/app/robots.ts` (the `sitemap` key) +- Modify: `nextjs-app/app/school/[slug]/page.tsx` (the two hardcoded + `https://schoolcompare.co.uk` template strings in `generateMetadata`) +- Modify: `backend/app.py:51` (`BASE_URL`) +- Test: `backend/tests/test_sitemap.py` (created here, extended in Tasks 4–5) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `SITE_URL: string` and `absoluteUrl(path: string): string` from + `@/lib/site`. Tasks 2 and 3 import both. `backend.app.BASE_URL` keeps its + name and type (`str`) so nothing else in `app.py` changes. + +- [ ] **Step 1: Write the failing test for the site-URL helper** + +Create `nextjs-app/__tests__/lib/site.test.ts`: + +```typescript +import { SITE_URL, absoluteUrl } from '@/lib/site'; + +describe('SITE_URL', () => { + it('is the www host, which is the one that serves a 200', () => { + // The apex 301s to www at Cloudflare. A canonical pointing at a redirect + // is a wasted signal, so every absolute URL we emit must already be www. + expect(SITE_URL).toBe('https://www.schoolcompare.co.uk'); + }); + + it('has no trailing slash, so joins never double up', () => { + expect(SITE_URL.endsWith('/')).toBe(false); + }); +}); + +describe('absoluteUrl', () => { + it('joins a rooted path', () => { + expect(absoluteUrl('/rankings')).toBe('https://www.schoolcompare.co.uk/rankings'); + }); + + it('joins a path missing its leading slash', () => { + expect(absoluteUrl('rankings')).toBe('https://www.schoolcompare.co.uk/rankings'); + }); + + it('maps the site root to a bare trailing slash', () => { + expect(absoluteUrl('/')).toBe('https://www.schoolcompare.co.uk/'); + }); +}); +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `cd nextjs-app && npm test -- __tests__/lib/site.test.ts` +Expected: FAIL — `Cannot find module '@/lib/site'`. + +- [ ] **Step 3: Write the helper** + +Create `nextjs-app/lib/site.ts`: + +```typescript +/** + * The one place the site's absolute origin is written down. + * + * The apex domain 301s to www at Cloudflare, so www is the host that actually + * serves a 200. Canonicals, og:url, sitemap entries and the robots.txt + * Sitemap: line must all agree with it — a canonical pointing at a redirect + * makes Google resolve the hop before it can consolidate the signal. + * + * backend/app.py holds the same value as BASE_URL for the sitemap. The two are + * asserted against each other by the e2e journeys rather than shared at build + * time, because the backend and frontend ship as separate images. + */ +export const SITE_URL = 'https://www.schoolcompare.co.uk'; + +/** Absolute URL for a site-relative path. Tolerates a missing leading slash. */ +export function absoluteUrl(path: string): string { + const rooted = path.startsWith('/') ? path : `/${path}`; + return `${SITE_URL}${rooted}`; +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `cd nextjs-app && npm test -- __tests__/lib/site.test.ts` +Expected: PASS (5 tests). + +- [ ] **Step 5: Point the frontend's absolute URLs at the helper** + +In `nextjs-app/app/layout.tsx`, add the import and replace the two hardcoded hosts: + +```typescript +import { SITE_URL } from '@/lib/site'; +``` + +```typescript + metadataBase: new URL(SITE_URL), +``` + +```typescript + openGraph: { + type: 'website', + title: 'schoolcompare | Compare School Performance', + description: 'Compare primary and secondary school SATs and GCSE performance across England', + url: SITE_URL, + siteName: 'schoolcompare', + }, +``` + +In `nextjs-app/app/robots.ts`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript + sitemap: absoluteUrl('/sitemap.xml'), +``` + +In `nextjs-app/app/school/[slug]/page.tsx`, add the import and replace both +occurrences inside `generateMetadata`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript + url: absoluteUrl(canonicalPath), +``` + +```typescript + alternates: { + canonical: absoluteUrl(canonicalPath), + }, +``` + +- [ ] **Step 6: Point the backend's BASE_URL at the same host** + +In `backend/app.py`, replace line 51: + +```python +# Must match SITE_URL in nextjs-app/lib/site.ts. The apex 301s to www, and a +# sitemap that redirects wastes a crawl on every URL it lists. +BASE_URL = "https://www.schoolcompare.co.uk" +``` + +- [ ] **Step 7: Write the backend test pinning the host** + +Create `backend/tests/test_sitemap.py`: + +```python +"""Tests for sitemap generation (spec 2026-08-20, workstream W1). + +The sitemap is built from the in-memory school DataFrame, so these inject a +small frame via monkeypatch rather than touching a database. +""" + +import numpy as np +import pandas as pd +import pytest + + +def _schools_df() -> pd.DataFrame: + """Two schools: one with results, one with neither results nor Ofsted.""" + base = { + "local_authority": "Testshire", + "school_type": "Academy", + "phase": "Primary", + "year": 202425, + "ofsted_date": None, + } + return pd.DataFrame( + [ + {**base, "urn": 100001, "school_name": "Alpha Primary", + "rwm_expected_pct": 62.0, "attainment_8_score": np.nan, + "ofsted_grade": 2.0}, + {**base, "urn": 100002, "school_name": "Ghost Primary", + "rwm_expected_pct": np.nan, "attainment_8_score": np.nan, + "ofsted_grade": np.nan}, + ] + ) + + +@pytest.fixture() +def sitemap(monkeypatch) -> str: + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemap() + + +def test_every_loc_uses_the_www_host(sitemap): + # The apex 301s to www. A that redirects burns a crawl per URL. + assert "https://www.schoolcompare.co.uk" in sitemap + assert "https://schoolcompare.co.uk" not in sitemap +``` + +- [ ] **Step 8: Run both suites to verify they pass** + +Run: +```bash +cd nextjs-app && npm test -- __tests__/lib/site.test.ts && cd .. +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: frontend 5 passed; backend 1 passed. + +Note: `test_every_loc_uses_the_www_host` asserts absence of the apex string. +Because `https://www.schoolcompare.co.uk` contains `schoolcompare.co.uk` but +not `https://schoolcompare.co.uk`, the second assertion is precise as written. + +- [ ] **Step 9: Commit** + +```bash +git add nextjs-app/lib/site.ts nextjs-app/__tests__/lib/site.test.ts \ + nextjs-app/app/layout.tsx nextjs-app/app/robots.ts \ + "nextjs-app/app/school/[slug]/page.tsx" backend/app.py \ + backend/tests/test_sitemap.py +git commit -m "fix(seo): canonicalise on the www host, which is the one that serves 200 + +The apex 301s to www at Cloudflare, but metadataBase, the school-page +canonical, robots.txt's Sitemap: line and the sitemap's own entries all +named the apex. Every one of those pointed Google at a redirect." +``` + +--- + +### Task 2: A canonical on every route, and one homepage instead of eleven params + +`app/page.tsx` accepts eleven search params and sets no canonical, so every +filter combination is a crawlable near-duplicate of the page we most want to +rank. `/rankings` and `/admissions` set no canonical either. + +**Files:** +- Modify: `nextjs-app/app/page.tsx` (the `metadata` export) +- Modify: `nextjs-app/app/rankings/page.tsx` (the `metadata` export) +- Modify: `nextjs-app/app/admissions/page.tsx` (the `metadata` export) +- Test: `nextjs-app/__tests__/app/metadata.test.ts` (created here, extended in Task 3) + +**Interfaces:** +- Consumes: `absoluteUrl` from `@/lib/site` (Task 1). +- Produces: nothing later tasks import. Task 3 adds cases to the same test file. + +- [ ] **Step 1: Write the failing tests** + +Create `nextjs-app/__tests__/app/metadata.test.ts`: + +```typescript +import { metadata as homeMetadata } from '@/app/page'; +import { metadata as rankingsMetadata } from '@/app/rankings/page'; +import { metadata as admissionsMetadata } from '@/app/admissions/page'; + +describe('canonical URLs', () => { + it('the homepage canonicalises to the bare root', () => { + // page.tsx reads eleven search params. Without this, every filter + // combination is a crawlable near-duplicate of the one page we want to + // rank for "compare schools". + expect(homeMetadata.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/'); + }); + + it('rankings canonicalises to the bare path', () => { + expect(rankingsMetadata.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/rankings'); + }); + + it('admissions canonicalises to the bare path', () => { + expect(admissionsMetadata.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/admissions'); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: FAIL — all three receive `undefined`. + +- [ ] **Step 3: Add the canonicals** + +In `nextjs-app/app/page.tsx`, add the import and extend the metadata export: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript +export const metadata: Metadata = { + title: { absolute: 'schoolcompare | Compare every school in England' }, + description: 'Search and compare school performance across England', + // This page reads eleven search params. They filter a result set; they do + // not make a new document. Collapsing every combination onto "/" stops the + // homepage competing with itself for its own head terms. + alternates: { canonical: absoluteUrl('/') }, +}; +``` + +In `nextjs-app/app/rankings/page.tsx`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript +export const metadata: Metadata = { + title: 'School Rankings', + description: 'Top-ranked schools by SATs and GCSE performance across England', + keywords: 'school rankings, top schools, best schools, KS2 rankings, KS4 rankings, school league tables', + // Param forms (?metric=&local_authority=&year=&phase=) collapse here for + // now. W3 replaces them with real indexable paths. + alternates: { canonical: absoluteUrl('/rankings') }, +}; +``` + +In `nextjs-app/app/admissions/page.tsx`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript +export const metadata: Metadata = { + title: 'School Admissions Guide', + description: + 'Understand the Primary and Secondary school admissions process in England, with live countdowns to every key deadline and National Offer Day.', + alternates: { canonical: absoluteUrl('/admissions') }, +}; +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: PASS (3 tests). + +- [ ] **Step 5: Add the e2e assertion** + +Append to `e2e/tests/journeys.spec.ts`: + +```typescript +/* + * Canonical URLs (spec 2026-08-20, W1). + * + * Every indexable route declares exactly one canonical, on the www host, with + * no query string. The homepage's eleven search params filter a result set + * rather than making a new document, so they all collapse onto "/". + */ +const CANONICAL_ROUTES: Array<[string, string]> = [ + ['/', 'https://www.schoolcompare.co.uk/'], + ['/rankings', 'https://www.schoolcompare.co.uk/rankings'], + ['/admissions', 'https://www.schoolcompare.co.uk/admissions'], +]; + +for (const [path, expected] of CANONICAL_ROUTES) { + test(`${path} declares exactly one canonical, on the www host`, async ({ page }) => { + await page.goto(path); + const hrefs = await page.locator('link[rel="canonical"]').evaluateAll( + (els) => els.map((e) => e.getAttribute('href'))); + expect(hrefs, `${path} should declare one canonical`).toHaveLength(1); + expect(hrefs[0]).toBe(expected); + }); +} + +test('a filtered homepage still canonicalises to the bare root', async ({ page }) => { + await page.goto('/?search=primary&phase=primary&sort=name&page=2'); + const href = await page.locator('link[rel="canonical"]').first() + .getAttribute('href'); + expect(href).toBe('https://www.schoolcompare.co.uk/'); +}); + +test('a school page canonicalises to its own slug on the www host', async ({ page }) => { + const res = await page.request.get('/api/schools?search=primary&per_page=1'); + expect(res.ok()).toBeTruthy(); + const [first] = (await res.json()).schools ?? []; + expect(first, 'no school available').toBeTruthy(); + + await page.goto(`/school/${first.urn}-x`); + const href = await page.locator('link[rel="canonical"]').first() + .getAttribute('href'); + expect(href).toMatch(/^https:\/\/www\.schoolcompare\.co\.uk\/school\/\d+-/); +}); +``` + +- [ ] **Step 6: Verify the e2e file still parses** + +Run: `cd e2e && npx playwright test --list` +Expected: the five new tests appear in the listing; total rises by 5. + +- [ ] **Step 7: Commit** + +```bash +git add nextjs-app/app/page.tsx nextjs-app/app/rankings/page.tsx \ + nextjs-app/app/admissions/page.tsx \ + nextjs-app/__tests__/app/metadata.test.ts e2e/tests/journeys.spec.ts +git commit -m "fix(seo): declare a canonical on every route + +The homepage read eleven search params and declared no canonical, so every +filter combination was a crawlable near-duplicate of the page we most want to +rank. Rankings and admissions declared none either." +``` + +--- + +### Task 3: A parameterised comparison is not a document + +`/compare?urns=…` is an unbounded parameter space — 25,193 schools make ~317 +million pairs, before triples. Bare `/compare` stays indexable: it is the +landing page for "compare schools", the head term in cluster C1. + +**Files:** +- Modify: `nextjs-app/app/compare/page.tsx` (replace the static `metadata` + export with `generateMetadata`) +- Test: `nextjs-app/__tests__/app/metadata.test.ts` (extend) + +**Interfaces:** +- Consumes: `absoluteUrl` from `@/lib/site` (Task 1). +- Produces: `generateMetadata({ searchParams }): Promise` exported + from `app/compare/page.tsx`, replacing the `metadata` const. `searchParams` + has the same `Promise<{ urns?: string; metric?: string }>` shape the default + export already declares as `ComparePageProps`. + +- [ ] **Step 1: Write the failing tests** + +Append to `nextjs-app/__tests__/app/metadata.test.ts`: + +```typescript +import { generateMetadata as compareMetadata } from '@/app/compare/page'; + +describe('/compare indexability', () => { + it('the bare compare page is indexable and canonical to itself', async () => { + // This is the landing page for the "compare schools" head term. + const meta = await compareMetadata({ searchParams: Promise.resolve({}) }); + expect(meta.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/compare'); + expect(meta.robots).toBeUndefined(); + }); + + it('a comparison of specific schools is noindex, follow', async () => { + // ~317 million pairs before triples. Indexing the parameter space would + // swamp everything else in the corpus. + const meta = await compareMetadata({ + searchParams: Promise.resolve({ urns: '100001,100002' }), + }); + expect(meta.robots).toEqual({ index: false, follow: true }); + }); + + it('a parameterised comparison still canonicalises to the bare path', async () => { + // follow:true plus a canonical means the outbound links to each school + // page still pass value even though this URL is not indexed. + const meta = await compareMetadata({ + searchParams: Promise.resolve({ urns: '100001,100002' }), + }); + expect(meta.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/compare'); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: FAIL — `compareMetadata is not a function`. + +- [ ] **Step 3: Replace the static metadata with generateMetadata** + +In `nextjs-app/app/compare/page.tsx`, add the import: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +Delete the whole `export const metadata: Metadata = { … };` block and put this +in its place: + +```typescript +/** + * Indexability depends on the query string, so this cannot be a static export. + * + * Bare /compare is the landing page for the "compare schools" head term and + * stays indexable. /compare?urns=… is an unbounded parameter space — 25,193 + * schools make ~317 million pairs — so it goes noindex. It stays `follow` and + * keeps a canonical to the bare path, so the links out to each school page + * still count. + */ +export async function generateMetadata( + { searchParams }: ComparePageProps, +): Promise { + const { urns } = await searchParams; + + const base: Metadata = { + title: 'Compare Schools', + description: + 'Compare schools in England side by side — Ofsted inspections, KS2 and GCSE results against the England average, admissions odds and school community.', + keywords: + 'school comparison, compare schools, Ofsted comparison, school admissions, KS2 comparison, primary school performance', + alternates: { canonical: absoluteUrl('/compare') }, + }; + + if (!urns) return base; + + return { ...base, robots: { index: false, follow: true } }; +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: PASS (6 tests — 3 from Task 2, 3 from this task). + +- [ ] **Step 5: Add the e2e assertion** + +Append to `e2e/tests/journeys.spec.ts`: + +```typescript +test('a bare /compare is indexable, a parameterised one is not', async ({ page }) => { + await page.goto('/compare'); + await expect(page.locator('meta[name="robots"]')).toHaveCount(0); + + const [a, b] = await twoPrimaryUrns(page); + await page.goto(`/compare?urns=${a},${b}`); + const robots = await page.locator('meta[name="robots"]').first() + .getAttribute('content'); + expect(robots).toContain('noindex'); + expect(robots).toContain('follow'); + + // noindex but follow: the links out to each school page still count, so the + // canonical must still be present and point at the bare path. + const canonical = await page.locator('link[rel="canonical"]').first() + .getAttribute('href'); + expect(canonical).toBe('https://www.schoolcompare.co.uk/compare'); +}); +``` + +- [ ] **Step 6: Verify the e2e file parses** + +Run: `cd e2e && npx playwright test --list` +Expected: the new test appears; total rises by 1. + +- [ ] **Step 7: Commit** + +```bash +git add nextjs-app/app/compare/page.tsx \ + nextjs-app/__tests__/app/metadata.test.ts e2e/tests/journeys.spec.ts +git commit -m "fix(seo): noindex parameterised comparisons, keep bare /compare + +25,193 schools make ~317 million pairs. The bare page stays indexable as the +landing page for the head term; the parameter space goes noindex, follow so +its outbound links still count." +``` + +--- + +### Task 4: Drop dataless schools and invented priorities from the sitemap + +`build_sitemap()` lists every URN and attaches `priority` and `changefreq`, +both of which Google ignores, while omitting `lastmod`, which it reads. After +the England-only change, 3,927 of 25,193 schools still have no results and no +Ofsted — nothing for a search result to say. + +`lastmod` is set from each school's `ofsted_date` where one exists, and +omitted otherwise. An always-`now` `lastmod` is a claim Google learns to +distrust; an absent one honestly means "unknown". + +**Files:** +- Modify: `backend/app.py:73-108` (`build_sitemap`) +- Test: `backend/tests/test_sitemap.py` (extend) + +**Interfaces:** +- Consumes: `BASE_URL` (Task 1). +- Produces: `build_sitemap()` keeps its `() -> str` signature. Task 5 changes + what it returns; the helper `_school_sitemap_rows(df) -> list[str]` added + here is what Task 5 reuses. + +- [ ] **Step 1: Write the failing tests** + +Append to `backend/tests/test_sitemap.py`: + +```python +def test_school_with_results_is_listed(sitemap): + assert "/school/100001-alpha-primary" in sitemap + + +def test_school_with_no_results_and_no_ofsted_is_omitted(sitemap): + # Nothing for a search result to say about it. Submitting it spends crawl + # budget and drags the corpus-wide quality signal down. + assert "/school/100002" not in sitemap + + +def test_no_invented_priority_or_changefreq(sitemap): + # Google ignores both. They were noise dressed as signal. + assert "" not in sitemap + assert "" not in sitemap + + +def test_ofsted_date_becomes_lastmod(monkeypatch): + from backend import app as app_module + import datetime + + def _df(): + base = _schools_df() + base.loc[base["urn"] == 100001, "ofsted_date"] = datetime.date(2024, 3, 14) + return base + + monkeypatch.setattr(app_module, "load_school_data", _df) + xml = app_module.build_sitemap() + assert "2024-03-14" in xml + + +def test_no_lastmod_invented_when_date_unknown(monkeypatch): + # An always-now lastmod is a claim Google learns to distrust. Absent + # honestly means unknown. + from backend import app as app_module + + def _df(): + df = _schools_df() + df["ofsted_date"] = None + return df + + monkeypatch.setattr(app_module, "load_school_data", _df) + xml = app_module.build_sitemap() + assert "" not in xml + + +def test_static_routes_are_listed(sitemap): + for path in ("/", "/rankings", "/compare", "/admissions"): + assert f"https://www.schoolcompare.co.uk{path}" in sitemap +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: FAIL — the dataless school is still listed, `` is present, +`/admissions` is missing, and no `` is emitted. + +- [ ] **Step 3: Rewrite build_sitemap** + +Replace `build_sitemap` in `backend/app.py` (currently lines 73-108) with: + +```python +# Routes worth submitting that are not a school page. /admissions was missing +# from the sitemap entirely despite being a static, indexable guide. +STATIC_SITEMAP_PATHS = ("/", "/rankings", "/compare", "/admissions") + + +def _has_publishable_data(row) -> bool: + """True when a school page has something a search result could state. + + A school with no results in any year and no Ofsted grade renders an empty + page. Submitting it spends crawl budget and drags the corpus-wide quality + signal down, so it stays out of the sitemap. The page itself still resolves + for anyone who has the URL. + """ + for field in ("rwm_expected_pct", "attainment_8_score", "ofsted_grade"): + value = row.get(field) + if value is not None and not pd.isna(value): + return True + return False + + +def _url_element(loc: str, lastmod: str | None = None) -> str: + """One entry. No priority or changefreq — Google ignores both.""" + body = f"{loc}" + if lastmod: + body += f"{lastmod}" + return f" {body}" + + +def _school_sitemap_rows(df) -> list[str]: + """A element per school that has something to show, URN order. + + lastmod comes from the school's Ofsted date where there is one and is + omitted otherwise. An always-now lastmod is a claim Google learns to + distrust; an absent one honestly means "unknown". + """ + if df.empty or "urn" not in df.columns or "school_name" not in df.columns: + return [] + + rows: list[str] = [] + seen: set[int] = set() + + # Latest row per URN first, so a school's most recent Ofsted date wins. + ordered = df.sort_values("year", ascending=False) if "year" in df.columns else df + + for _, row in ordered.iterrows(): + urn = int(row["urn"]) + if urn in seen: + continue + seen.add(urn) + if not _has_publishable_data(row): + continue + + lastmod = None + ofsted_date = row.get("ofsted_date") + if ofsted_date is not None and not pd.isna(ofsted_date): + lastmod = pd.Timestamp(ofsted_date).date().isoformat() + + rows.append(_url_element( + BASE_URL + _school_url(urn, str(row["school_name"])), lastmod)) + + return rows + + +def build_sitemap() -> str: + """Generate sitemap XML from in-memory school data. Returns the XML string.""" + df = load_school_data() + + lines = ['', + ''] + lines.extend(_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS) + lines.extend(_school_sitemap_rows(df)) + lines.append("") + return "\n".join(lines) +``` + +Confirm `pandas` is imported in `backend/app.py` as `pd`. If it is not, add +`import pandas as pd` alongside the other imports at the top of the file. + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: PASS (7 tests). + +- [ ] **Step 5: Run the whole backend suite for regressions** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests -q +``` +Expected: PASS. Baseline before this plan was 54 passed; expect 61. + +- [ ] **Step 6: Commit** + +```bash +git add backend/app.py backend/tests/test_sitemap.py +git commit -m "fix(seo): submit only school pages that have something to show + +Drops the 3,927 schools with neither results nor an Ofsted grade, adds +/admissions which was never listed, replaces the invented priority and +changefreq with a lastmod taken from each school's Ofsted date." +``` + +--- + +### Task 5: Split the sitemap into a per-family index + +One flat file cannot tell you which page family Google is failing to index. +Search Console reports coverage per submitted sitemap, so splitting by family +is what makes W2's location pages measurable when they land. 25,193 URLs is +still under the 50,000 per-file limit, so this is for the diagnostics, not the +size. + +`/sitemap.xml` becomes the index. Children are `/sitemap-static.xml` and +`/sitemap-schools-{n}.xml`, 10,000 URLs each. + +**Files:** +- Modify: `backend/app.py` (`build_sitemap`, the `_sitemap_xml` cache, the + `/sitemap.xml` route, `/api/admin/regenerate-sitemap`) +- Create: `nextjs-app/lib/sitemapProxy.ts` +- Create: `nextjs-app/app/sitemap-[...parts]/route.ts` +- Modify: `nextjs-app/app/sitemap.xml/route.ts` (body moves to the shared proxy) +- Test: `backend/tests/test_sitemap.py` (extend) + +**Interfaces:** +- Consumes: `_school_sitemap_rows`, `_url_element`, `STATIC_SITEMAP_PATHS` (Task 4). +- Produces: `build_sitemaps() -> dict[str, str]` mapping a filename + (`"sitemap.xml"`, `"sitemap-static.xml"`, `"sitemap-schools-1.xml"`) to its + XML. The module-level cache `_sitemap_xml: str | None` is replaced by + `_sitemaps: dict[str, str] | None`. `build_sitemap()` is kept as a thin + wrapper returning `build_sitemaps()["sitemap.xml"]` so Task 4's tests and + the startup path in `lifespan` keep working unchanged. + +- [ ] **Step 1: Write the failing tests** + +Append to `backend/tests/test_sitemap.py`: + +```python +@pytest.fixture() +def sitemaps(monkeypatch) -> dict: + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemaps() + + +def test_index_lists_each_child(sitemaps): + index = sitemaps["sitemap.xml"] + assert " entries only; mixing in is invalid. + assert "" not in sitemaps["sitemap.xml"] + + +def test_static_child_holds_the_static_routes(sitemaps): + static = sitemaps["sitemap-static.xml"] + for path in ("/", "/rankings", "/compare", "/admissions"): + assert f"https://www.schoolcompare.co.uk{path}" in static + + +def test_school_child_holds_the_schools(sitemaps): + assert "/school/100001-alpha-primary" in sitemaps["sitemap-schools-1.xml"] + + +def test_children_are_chunked_under_the_limit(monkeypatch): + # Sitemaps cap at 50,000 URLs per file. Chunk at 10,000 so a child stays + # small enough to eyeball in Search Console. + from backend import app as app_module + import pandas as _pd + + rows = [ + {"urn": 200000 + i, "school_name": f"School {i}", "year": 202425, + "rwm_expected_pct": 60.0, "attainment_8_score": None, + "ofsted_grade": 2.0, "ofsted_date": None} + for i in range(10_001) + ] + monkeypatch.setattr(app_module, "load_school_data", lambda: _pd.DataFrame(rows)) + + maps = app_module.build_sitemaps() + assert maps["sitemap-schools-1.xml"].count("") == 10_000 + assert maps["sitemap-schools-2.xml"].count("") == 1 + + +def test_build_sitemap_still_returns_the_index(sitemap): + # lifespan and the admin endpoint call build_sitemap(); keep it working. + assert " XML. Populated on startup and by the +# admin regenerate endpoint after a pipeline run. +_sitemaps: dict[str, str] | None = None +``` + +Add below `build_sitemap`'s helpers from Task 4: + +```python +# Sitemaps cap at 50,000 URLs per file. 10,000 keeps a child small enough to +# scan by eye in Search Console, which is the point of splitting at all: +# coverage is reported per submitted sitemap, so one file per page family is +# what makes an indexation problem attributable to a family. +SITEMAP_CHUNK_SIZE = 10_000 + + +def build_sitemaps() -> dict[str, str]: + """Build the sitemap index and every child, keyed by filename.""" + df = load_school_data() + + def _urlset(rows: list[str]) -> str: + return "\n".join([ + '', + '', + *rows, + "", + ]) + + maps: dict[str, str] = { + "sitemap-static.xml": _urlset( + [_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS]), + } + + school_rows = _school_sitemap_rows(df) + # Always emit at least one school child, so the index shape is stable even + # on an empty database. + chunks = [school_rows[i:i + SITEMAP_CHUNK_SIZE] + for i in range(0, len(school_rows), SITEMAP_CHUNK_SIZE)] or [[]] + for n, chunk in enumerate(chunks, start=1): + maps[f"sitemap-schools-{n}.xml"] = _urlset(chunk) + + # On a sitemap index, lastmod means "when this sitemap file last changed", + # so generation time is the correct value here — unlike on a , where + # it would be a claim about content we cannot support. + generated = datetime.now(timezone.utc).date().isoformat() + index_rows = [ + f" {BASE_URL}/{name}" + f"{generated}" + for name in maps + ] + maps["sitemap.xml"] = "\n".join([ + '', + '', + *index_rows, + "", + ]) + return maps + + +def build_sitemap() -> str: + """The sitemap index. Kept for `lifespan` and the admin endpoint.""" + return build_sitemaps()["sitemap.xml"] +``` + +Delete the old `build_sitemap` body from Task 4 (the one assembling a single +``); `_url_element`, `_has_publishable_data`, `_school_sitemap_rows` +and `STATIC_SITEMAP_PATHS` all stay. + +Ensure `from datetime import datetime, timezone` is imported at the top of +`backend/app.py`; add it if absent. + +Replace the `/sitemap.xml` route and add the child route: + +```python +def _serve_sitemap(filename: str) -> Response: + global _sitemaps + if _sitemaps is None: + try: + _sitemaps = build_sitemaps() + except Exception as e: + raise HTTPException(status_code=503, detail=f"Sitemap unavailable: {e}") + if filename not in _sitemaps: + raise HTTPException(status_code=404, detail="No such sitemap") + return Response(content=_sitemaps[filename], media_type="application/xml") + + +@app.get("/sitemap.xml") +async def sitemap_xml(): + """Serve the sitemap index.""" + return _serve_sitemap("sitemap.xml") + + +@app.get("/sitemap-{name}.xml") +async def sitemap_child(name: str): + """Serve a child sitemap (static, or schools-N).""" + return _serve_sitemap(f"sitemap-{name}.xml") +``` + +Update `/api/admin/regenerate-sitemap`: + +```python +@app.post("/api/admin/regenerate-sitemap") +@limiter.limit("10/minute") +async def regenerate_sitemap( + request: Request, + _: bool = Depends(verify_admin_api_key), +): + """Rebuild and cache the sitemaps from current school data. Called by Airflow after data updates.""" + global _sitemaps + _sitemaps = build_sitemaps() + n = sum(x.count("") for x in _sitemaps.values()) + return {"status": "ok", "urls": n, "sitemaps": len(_sitemaps)} +``` + +In `lifespan`, replace the sitemap block: + +```python + try: + _sitemaps = build_sitemaps() + n = sum(x.count("") for x in _sitemaps.values()) + print(f"Sitemaps built: {len(_sitemaps)} files, {n} URLs.") + except Exception as e: + print(f"Warning: sitemap build failed on startup: {e}") +``` + +and change its `global _sitemap_xml` declaration to `global _sitemaps`. + +Update the six Task 4 tests to read from the right child. In +`backend/tests/test_sitemap.py`, change `test_school_with_results_is_listed`, +`test_school_with_no_results_and_no_ofsted_is_omitted`, +`test_ofsted_date_becomes_lastmod` and `test_no_lastmod_invented_when_date_unknown` +to assert against `build_sitemaps()["sitemap-schools-1.xml"]`, and +`test_static_routes_are_listed` against `build_sitemaps()["sitemap-static.xml"]`. +`test_no_invented_priority_or_changefreq` and `test_every_loc_uses_the_www_host` +should assert across every value in `build_sitemaps()`. + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: PASS (13 tests). + +- [ ] **Step 5: Replace the Next proxy with a catch-all** + +The index and the children need the same proxy, and a catch-all cannot match +`/sitemap.xml` itself, so both routes stay and share one handler. Extract it to +`nextjs-app/lib/sitemapProxy.ts`: + +```typescript +/** + * Runtime proxy for the sitemap family → the FastAPI backend. + * + * Like the /api/* proxy, this reads FASTAPI_URL at request time rather than + * baking the backend host into the build, so one image works in every + * environment. robots.ts points crawlers at /sitemap.xml, which is the index; + * the index names the children, which land on the same proxy. + */ +import { NextResponse } from 'next/server'; + +function backendOrigin(): string { + const base = process.env.FASTAPI_URL || process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8000/api'; + return base.replace(/\/api$/, ''); +} + +export async function proxySitemap(filename: string): Promise { + let upstream: Response; + try { + upstream = await fetch(`${backendOrigin()}/${filename}`, { cache: 'no-store' }); + } catch { + return new NextResponse('Sitemap temporarily unavailable', { status: 502 }); + } + + const body = await upstream.text(); + return new NextResponse(body, { + status: upstream.status, + headers: { 'content-type': upstream.headers.get('content-type') || 'application/xml' }, + }); +} +``` + +Replace the body of `nextjs-app/app/sitemap.xml/route.ts` with: + +```typescript +import { proxySitemap } from '@/lib/sitemapProxy'; + +export const dynamic = 'force-dynamic'; +export const runtime = 'nodejs'; + +export async function GET() { + return proxySitemap('sitemap.xml'); +} +``` + +Create `nextjs-app/app/sitemap-[...parts]/route.ts`: + +```typescript +import { NextResponse } from 'next/server'; +import { proxySitemap } from '@/lib/sitemapProxy'; + +export const dynamic = 'force-dynamic'; +export const runtime = 'nodejs'; + +/** + * Children are /sitemap-static.xml and /sitemap-schools-{n}.xml. The segment + * pattern is validated here rather than passed through, so this route cannot + * be used to reach arbitrary backend paths. + */ +const CHILD = /^(static|schools-\d+)\.xml$/; + +export async function GET( + _request: Request, + { params }: { params: Promise<{ parts: string[] }> }, +) { + const { parts } = await params; + const name = parts.join('/'); + if (!CHILD.test(name)) { + return new NextResponse('Not found', { status: 404 }); + } + return proxySitemap(`sitemap-${name}`); +} +``` + +- [ ] **Step 6: Add the e2e assertions** + +Replace the `the sitemap submits no Welsh or overseas school` test in +`e2e/tests/journeys.spec.ts` with this block, which keeps its assertions and +follows the index to its children: + +```typescript +test('the sitemap index names children that all resolve', async ({ page }) => { + const res = await page.request.get('/sitemap.xml'); + expect(res.ok()).toBeTruthy(); + const index = await res.text(); + + expect(index).toContain(' entries only; mixing in is invalid. + expect(index).not.toContain(''); + + const locs = [...index.matchAll(/([^<]+)<\/loc>/g)].map((m) => m[1]); + expect(locs.length).toBeGreaterThanOrEqual(2); + + for (const loc of locs) { + expect(loc.startsWith('https://www.schoolcompare.co.uk/')).toBeTruthy(); + const child = await page.request.get(new URL(loc).pathname); + expect(child.ok(), `${loc} should resolve`).toBeTruthy(); + expect(await child.text()).toContain(' { + const index = await (await page.request.get('/sitemap.xml')).text(); + const locs = [...index.matchAll(/([^<]+)<\/loc>/g)].map((m) => m[1]); + + let total = 0; + for (const loc of locs) { + const xml = await (await page.request.get(new URL(loc).pathname)).text(); + total += (xml.match(//g) ?? []).length; + // 401559 (Adamsdown, Cardiff) and 402426 (ACT Schools, Cardiff) were both + // submitted before the England-only filter landed. + expect(xml).not.toContain('/school/401559'); + expect(xml).not.toContain('/school/402426'); + } + expect(total, 'sitemap looks empty or truncated').toBeGreaterThan(1000); +}); + +test('the sitemap invents no priority, changefreq, or lastmod it cannot support', async ({ page }) => { + const index = await (await page.request.get('/sitemap.xml')).text(); + const firstChild = index.match(/([^<]+)<\/loc>/)?.[1]; + expect(firstChild).toBeTruthy(); + + const xml = await (await page.request.get(new URL(firstChild!).pathname)).text(); + expect(xml).not.toContain(''); + expect(xml).not.toContain(''); +}); +``` + +- [ ] **Step 7: Verify the e2e file parses and the whole backend suite is green** + +Run: +```bash +cd e2e && npx playwright test --list && cd .. +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests -q +cd nextjs-app && npm test && npx next build --no-lint +``` +Expected: e2e listing parses; backend suite green; Jest green; Next build +succeeds with both sitemap routes present. + +- [ ] **Step 8: Commit** + +```bash +git add backend/app.py backend/tests/test_sitemap.py \ + nextjs-app/lib/sitemapProxy.ts nextjs-app/app/sitemap.xml/route.ts \ + "nextjs-app/app/sitemap-[...parts]/route.ts" e2e/tests/journeys.spec.ts +git commit -m "feat(seo): split the sitemap into a per-family index + +Search Console reports coverage per submitted sitemap, so one file per page +family is what will make W2's location pages measurable when they land. The +index's lastmod is generation time, which is the correct semantic there." +``` + +--- + +## After the plan + +1. Open a PR from the feature branch. Do not merge until the England-only PR + is in `main`, or the sitemap counts in the tests will not hold. +2. **Staging needs an Airflow run between the deploy and the e2e gate.** The + England-only filter is a dbt change and staging's marts must be rebuilt + before the journeys run, or the England-only tests fail and block promotion. +3. After staging is verified, resubmit `/sitemap.xml` in Search Console. The + old flat sitemap should be removed from the property so its coverage report + does not compete with the index's. +4. W0's Search Console baseline export should be captured **before** this + lands, so the before/after comparison has a clean cut. + +## Not in this plan + +W1 item 5 in the spec — a better template for the 3,927 English schools with +no data — is deliberately excluded. Task 4 stops submitting them, which is the +crawl-hygiene half. Giving them something worth showing is a product change, +not a crawl fix, and belongs with its own design. -- 2.54.0 From 69f220124499d9e77e8f78a803536853493dc3c1 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:09:00 +0100 Subject: [PATCH 3/3] docs(seo): correct W1 plan's sitemap child routes Next only treats a whole bracketed path segment as dynamic, so the planned app/sitemap-[...parts]/route.ts would have been read as a literal static folder and never matched. Children move under /sitemaps/. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../plans/2026-08-20-w1-crawl-hygiene.md | 194 ++++++++++-------- 1 file changed, 108 insertions(+), 86 deletions(-) diff --git a/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md b/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md index 7e3c269..a6b09e8 100644 --- a/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md +++ b/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md @@ -807,25 +807,33 @@ is what makes W2's location pages measurable when they land. 25,193 URLs is still under the 50,000 per-file limit, so this is for the diagnostics, not the size. -`/sitemap.xml` becomes the index. Children are `/sitemap-static.xml` and -`/sitemap-schools-{n}.xml`, 10,000 URLs each. +`/sitemap.xml` becomes the index. Children live under `/sitemaps/` — +`/sitemaps/static.xml` and `/sitemaps/schools-{n}.xml`, 10,000 URLs each. + +**Why the children sit in their own directory:** Next.js only treats a path +segment as dynamic when the whole segment is bracketed. Verified in Next's +own router source — `UrlNode._insert` only reads a segment as dynamic if it +`startsWith('[') && endsWith(']')`, so a folder named `sitemap-[...parts]` +would be inserted as a *static* segment and never match. Putting the children +under `/sitemaps/` gives a clean `app/sitemaps/[...parts]/route.ts`. **Files:** - Modify: `backend/app.py` (`build_sitemap`, the `_sitemap_xml` cache, the - `/sitemap.xml` route, `/api/admin/regenerate-sitemap`) + `/sitemap.xml` route, `/api/admin/regenerate-sitemap`, `lifespan`) - Create: `nextjs-app/lib/sitemapProxy.ts` -- Create: `nextjs-app/app/sitemap-[...parts]/route.ts` +- Create: `nextjs-app/app/sitemaps/[...parts]/route.ts` - Modify: `nextjs-app/app/sitemap.xml/route.ts` (body moves to the shared proxy) - Test: `backend/tests/test_sitemap.py` (extend) **Interfaces:** - Consumes: `_school_sitemap_rows`, `_url_element`, `STATIC_SITEMAP_PATHS` (Task 4). -- Produces: `build_sitemaps() -> dict[str, str]` mapping a filename - (`"sitemap.xml"`, `"sitemap-static.xml"`, `"sitemap-schools-1.xml"`) to its - XML. The module-level cache `_sitemap_xml: str | None` is replaced by +- Produces: `build_sitemaps() -> dict[str, str]` mapping a key + (`"sitemap.xml"`, `"static.xml"`, `"schools-1.xml"`) to its XML. The keys of + the children are bare filenames; the index prefixes them with `/sitemaps/`. + The module-level cache `_sitemap_xml: str | None` is replaced by `_sitemaps: dict[str, str] | None`. `build_sitemap()` is kept as a thin - wrapper returning `build_sitemaps()["sitemap.xml"]` so Task 4's tests and - the startup path in `lifespan` keep working unchanged. + wrapper returning `build_sitemaps()["sitemap.xml"]` so `lifespan` and the + admin endpoint keep working unchanged. - [ ] **Step 1: Write the failing tests** @@ -843,8 +851,8 @@ def sitemaps(monkeypatch) -> dict: def test_index_lists_each_child(sitemaps): index = sitemaps["sitemap.xml"] assert "" not in sitemaps["sitemap.xml"] +def test_index_does_not_list_itself(sitemaps): + assert "https://www.schoolcompare.co.uk/sitemap.xml" not in sitemaps["sitemap.xml"] + + def test_static_child_holds_the_static_routes(sitemaps): - static = sitemaps["sitemap-static.xml"] + static = sitemaps["static.xml"] for path in ("/", "/rankings", "/compare", "/admissions"): assert f"https://www.schoolcompare.co.uk{path}" in static def test_school_child_holds_the_schools(sitemaps): - assert "/school/100001-alpha-primary" in sitemaps["sitemap-schools-1.xml"] + assert "/school/100001-alpha-primary" in sitemaps["schools-1.xml"] def test_children_are_chunked_under_the_limit(monkeypatch): @@ -877,8 +889,8 @@ def test_children_are_chunked_under_the_limit(monkeypatch): monkeypatch.setattr(app_module, "load_school_data", lambda: _pd.DataFrame(rows)) maps = app_module.build_sitemaps() - assert maps["sitemap-schools-1.xml"].count("") == 10_000 - assert maps["sitemap-schools-2.xml"].count("") == 1 + assert maps["schools-1.xml"].count("") == 10_000 + assert maps["schools-2.xml"].count("") == 1 def test_build_sitemap_still_returns_the_index(sitemap): @@ -895,24 +907,21 @@ uv run --quiet --with-requirements requirements.txt --with pytest \ ``` Expected: FAIL — `build_sitemaps` is not defined. -Note: `test_no_invented_priority_or_changefreq`, `test_static_routes_are_listed`, -`test_school_with_results_is_listed`, `test_school_with_no_results_and_no_ofsted_is_omitted`, -`test_ofsted_date_becomes_lastmod` and `test_no_lastmod_invented_when_date_unknown` -from Task 4 assert against `build_sitemap()`, which now returns the index and -no longer contains school URLs. Step 3 updates them to read the relevant child -from `build_sitemaps()`. +Note: the Task 4 tests assert against `build_sitemap()`, which now returns the +index and no longer contains school URLs. Step 3 updates them to read the +relevant child from `build_sitemaps()`. - [ ] **Step 3: Implement the index** In `backend/app.py`, replace the module-level cache declaration: ```python -# In-memory sitemap cache: filename -> XML. Populated on startup and by the -# admin regenerate endpoint after a pipeline run. +# In-memory sitemap cache: name -> XML. Populated on startup and by the admin +# regenerate endpoint after a pipeline run. _sitemaps: dict[str, str] | None = None ``` -Add below `build_sitemap`'s helpers from Task 4: +Add below Task 4's helpers: ```python # Sitemaps cap at 50,000 URLs per file. 10,000 keeps a child small enough to @@ -921,21 +930,27 @@ Add below `build_sitemap`'s helpers from Task 4: # what makes an indexation problem attributable to a family. SITEMAP_CHUNK_SIZE = 10_000 +# Children are served under /sitemaps/ because Next.js only treats a whole +# bracketed path segment as dynamic — a route folder named "sitemap-[...parts]" +# is read as a literal static segment and never matches. +SITEMAP_CHILD_PREFIX = "/sitemaps" + + +def _urlset(rows: list[str]) -> str: + return "\n".join([ + '', + '', + *rows, + "", + ]) + def build_sitemaps() -> dict[str, str]: - """Build the sitemap index and every child, keyed by filename.""" + """Build the sitemap index and every child, keyed by name.""" df = load_school_data() - def _urlset(rows: list[str]) -> str: - return "\n".join([ - '', - '', - *rows, - "", - ]) - - maps: dict[str, str] = { - "sitemap-static.xml": _urlset( + children: dict[str, str] = { + "static.xml": _urlset( [_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS]), } @@ -945,24 +960,24 @@ def build_sitemaps() -> dict[str, str]: chunks = [school_rows[i:i + SITEMAP_CHUNK_SIZE] for i in range(0, len(school_rows), SITEMAP_CHUNK_SIZE)] or [[]] for n, chunk in enumerate(chunks, start=1): - maps[f"sitemap-schools-{n}.xml"] = _urlset(chunk) + children[f"schools-{n}.xml"] = _urlset(chunk) # On a sitemap index, lastmod means "when this sitemap file last changed", # so generation time is the correct value here — unlike on a , where # it would be a claim about content we cannot support. generated = datetime.now(timezone.utc).date().isoformat() index_rows = [ - f" {BASE_URL}/{name}" + f" {BASE_URL}{SITEMAP_CHILD_PREFIX}/{name}" f"{generated}" - for name in maps + for name in children ] - maps["sitemap.xml"] = "\n".join([ + index = "\n".join([ '', '', *index_rows, "", ]) - return maps + return {**children, "sitemap.xml": index} def build_sitemap() -> str: @@ -970,9 +985,8 @@ def build_sitemap() -> str: return build_sitemaps()["sitemap.xml"] ``` -Delete the old `build_sitemap` body from Task 4 (the one assembling a single -``); `_url_element`, `_has_publishable_data`, `_school_sitemap_rows` -and `STATIC_SITEMAP_PATHS` all stay. +Delete Task 4's single-`` `build_sitemap` body; `_url_element`, +`_has_publishable_data`, `_school_sitemap_rows` and `STATIC_SITEMAP_PATHS` stay. Ensure `from datetime import datetime, timezone` is imported at the top of `backend/app.py`; add it if absent. @@ -980,16 +994,16 @@ Ensure `from datetime import datetime, timezone` is imported at the top of Replace the `/sitemap.xml` route and add the child route: ```python -def _serve_sitemap(filename: str) -> Response: +def _serve_sitemap(name: str) -> Response: global _sitemaps if _sitemaps is None: try: _sitemaps = build_sitemaps() except Exception as e: raise HTTPException(status_code=503, detail=f"Sitemap unavailable: {e}") - if filename not in _sitemaps: + if name not in _sitemaps: raise HTTPException(status_code=404, detail="No such sitemap") - return Response(content=_sitemaps[filename], media_type="application/xml") + return Response(content=_sitemaps[name], media_type="application/xml") @app.get("/sitemap.xml") @@ -998,10 +1012,10 @@ async def sitemap_xml(): return _serve_sitemap("sitemap.xml") -@app.get("/sitemap-{name}.xml") +@app.get("/sitemaps/{name}") async def sitemap_child(name: str): - """Serve a child sitemap (static, or schools-N).""" - return _serve_sitemap(f"sitemap-{name}.xml") + """Serve a child sitemap (static.xml, or schools-N.xml).""" + return _serve_sitemap(name) ``` Update `/api/admin/regenerate-sitemap`: @@ -1020,7 +1034,8 @@ async def regenerate_sitemap( return {"status": "ok", "urls": n, "sitemaps": len(_sitemaps)} ``` -In `lifespan`, replace the sitemap block: +In `lifespan`, change `global _sitemap_xml` to `global _sitemaps` and replace +the sitemap block: ```python try: @@ -1031,14 +1046,12 @@ In `lifespan`, replace the sitemap block: print(f"Warning: sitemap build failed on startup: {e}") ``` -and change its `global _sitemap_xml` declaration to `global _sitemaps`. - -Update the six Task 4 tests to read from the right child. In -`backend/tests/test_sitemap.py`, change `test_school_with_results_is_listed`, +Update the Task 4 tests to read the right child: change +`test_school_with_results_is_listed`, `test_school_with_no_results_and_no_ofsted_is_omitted`, `test_ofsted_date_becomes_lastmod` and `test_no_lastmod_invented_when_date_unknown` -to assert against `build_sitemaps()["sitemap-schools-1.xml"]`, and -`test_static_routes_are_listed` against `build_sitemaps()["sitemap-static.xml"]`. +to assert against `build_sitemaps()["schools-1.xml"]`, and +`test_static_routes_are_listed` against `build_sitemaps()["static.xml"]`. `test_no_invented_priority_or_changefreq` and `test_every_loc_uses_the_www_host` should assert across every value in `build_sitemaps()`. @@ -1049,12 +1062,12 @@ Run: uv run --quiet --with-requirements requirements.txt --with pytest \ --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q ``` -Expected: PASS (13 tests). +Expected: PASS (14 tests). -- [ ] **Step 5: Replace the Next proxy with a catch-all** +- [ ] **Step 5: Share one proxy between the index route and the child route** -The index and the children need the same proxy, and a catch-all cannot match -`/sitemap.xml` itself, so both routes stay and share one handler. Extract it to +The index and the children need the same proxy, and the catch-all cannot match +`/sitemap.xml` itself, so both routes stay and share one handler. Create `nextjs-app/lib/sitemapProxy.ts`: ```typescript @@ -1064,7 +1077,7 @@ The index and the children need the same proxy, and a catch-all cannot match * Like the /api/* proxy, this reads FASTAPI_URL at request time rather than * baking the backend host into the build, so one image works in every * environment. robots.ts points crawlers at /sitemap.xml, which is the index; - * the index names the children, which land on the same proxy. + * the index names children under /sitemaps/, which land on the same proxy. */ import { NextResponse } from 'next/server'; @@ -1073,10 +1086,10 @@ function backendOrigin(): string { return base.replace(/\/api$/, ''); } -export async function proxySitemap(filename: string): Promise { +export async function proxySitemap(path: string): Promise { let upstream: Response; try { - upstream = await fetch(`${backendOrigin()}/${filename}`, { cache: 'no-store' }); + upstream = await fetch(`${backendOrigin()}${path}`, { cache: 'no-store' }); } catch { return new NextResponse('Sitemap temporarily unavailable', { status: 502 }); } @@ -1098,11 +1111,11 @@ export const dynamic = 'force-dynamic'; export const runtime = 'nodejs'; export async function GET() { - return proxySitemap('sitemap.xml'); + return proxySitemap('/sitemap.xml'); } ``` -Create `nextjs-app/app/sitemap-[...parts]/route.ts`: +Create `nextjs-app/app/sitemaps/[...parts]/route.ts`: ```typescript import { NextResponse } from 'next/server'; @@ -1112,9 +1125,9 @@ export const dynamic = 'force-dynamic'; export const runtime = 'nodejs'; /** - * Children are /sitemap-static.xml and /sitemap-schools-{n}.xml. The segment - * pattern is validated here rather than passed through, so this route cannot - * be used to reach arbitrary backend paths. + * Children are /sitemaps/static.xml and /sitemaps/schools-{n}.xml. The name is + * validated here rather than passed through, so this route cannot be used to + * reach arbitrary backend paths. */ const CHILD = /^(static|schools-\d+)\.xml$/; @@ -1127,31 +1140,35 @@ export async function GET( if (!CHILD.test(name)) { return new NextResponse('Not found', { status: 404 }); } - return proxySitemap(`sitemap-${name}`); + return proxySitemap(`/sitemaps/${name}`); } ``` -- [ ] **Step 6: Add the e2e assertions** +- [ ] **Step 6: Update the e2e assertions** Replace the `the sitemap submits no Welsh or overseas school` test in `e2e/tests/journeys.spec.ts` with this block, which keeps its assertions and follows the index to its children: ```typescript -test('the sitemap index names children that all resolve', async ({ page }) => { +async function sitemapChildren(page: Page): Promise { const res = await page.request.get('/sitemap.xml'); expect(res.ok()).toBeTruthy(); const index = await res.text(); - expect(index).toContain('([^<]+)<\/loc>/g)].map((m) => m[1]); +} + +test('the sitemap index names children that all resolve', async ({ page }) => { + const index = await (await page.request.get('/sitemap.xml')).text(); // An index holds entries only; mixing in is invalid. expect(index).not.toContain(''); - const locs = [...index.matchAll(/([^<]+)<\/loc>/g)].map((m) => m[1]); + const locs = await sitemapChildren(page); expect(locs.length).toBeGreaterThanOrEqual(2); for (const loc of locs) { - expect(loc.startsWith('https://www.schoolcompare.co.uk/')).toBeTruthy(); + expect(loc.startsWith('https://www.schoolcompare.co.uk/sitemaps/')).toBeTruthy(); const child = await page.request.get(new URL(loc).pathname); expect(child.ok(), `${loc} should resolve`).toBeTruthy(); expect(await child.text()).toContain(' { }); test('the sitemap submits no Welsh or overseas school', async ({ page }) => { - const index = await (await page.request.get('/sitemap.xml')).text(); - const locs = [...index.matchAll(/([^<]+)<\/loc>/g)].map((m) => m[1]); + const locs = await sitemapChildren(page); let total = 0; for (const loc of locs) { @@ -1174,18 +1190,18 @@ test('the sitemap submits no Welsh or overseas school', async ({ page }) => { expect(total, 'sitemap looks empty or truncated').toBeGreaterThan(1000); }); -test('the sitemap invents no priority, changefreq, or lastmod it cannot support', async ({ page }) => { - const index = await (await page.request.get('/sitemap.xml')).text(); - const firstChild = index.match(/([^<]+)<\/loc>/)?.[1]; - expect(firstChild).toBeTruthy(); +test('the sitemap invents no priority or changefreq', async ({ page }) => { + const [first] = await sitemapChildren(page); + expect(first).toBeTruthy(); - const xml = await (await page.request.get(new URL(firstChild!).pathname)).text(); + const xml = await (await page.request.get(new URL(first).pathname)).text(); + // Google ignores both. They were noise dressed as signal. expect(xml).not.toContain(''); expect(xml).not.toContain(''); }); ``` -- [ ] **Step 7: Verify the e2e file parses and the whole backend suite is green** +- [ ] **Step 7: Verify everything is green** Run: ```bash @@ -1194,20 +1210,26 @@ uv run --quiet --with-requirements requirements.txt --with pytest \ --with "httpx==0.27.0" python -m pytest backend/tests -q cd nextjs-app && npm test && npx next build --no-lint ``` -Expected: e2e listing parses; backend suite green; Jest green; Next build -succeeds with both sitemap routes present. +Expected: e2e listing parses; backend suite green; Jest green; the Next build +succeeds and lists both `/sitemap.xml` and `/sitemaps/[...parts]` as routes. +The build output is the check that the dynamic segment resolves — a folder +Next reads as static would simply not appear as a dynamic route. - [ ] **Step 8: Commit** ```bash git add backend/app.py backend/tests/test_sitemap.py \ nextjs-app/lib/sitemapProxy.ts nextjs-app/app/sitemap.xml/route.ts \ - "nextjs-app/app/sitemap-[...parts]/route.ts" e2e/tests/journeys.spec.ts + "nextjs-app/app/sitemaps/[...parts]/route.ts" e2e/tests/journeys.spec.ts git commit -m "feat(seo): split the sitemap into a per-family index Search Console reports coverage per submitted sitemap, so one file per page family is what will make W2's location pages measurable when they land. The -index's lastmod is generation time, which is the correct semantic there." +index's lastmod is generation time, which is the correct semantic there. + +Children sit under /sitemaps/ because Next only treats a whole bracketed path +segment as dynamic; a route folder named sitemap-[...parts] would be read as a +literal static segment and never match." ``` --- -- 2.54.0