From 786ec80dd4de4a3cb674a89e35b3b5e461639289 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:14:32 +0100 Subject: [PATCH] feat(seo): split the sitemap into a per-family index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 — unlike on a , where it would be a claim we cannot support. 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. Confirmed by the build output, which lists /sitemaps/[...parts] as a dynamic route. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 113 +++++++++++++++----- backend/tests/test_sitemap.py | 112 ++++++++++++++++--- e2e/tests/journeys.spec.ts | 51 +++++++-- nextjs-app/app/sitemap.xml/route.ts | 28 +---- nextjs-app/app/sitemaps/[...parts]/route.ts | 24 +++++ nextjs-app/lib/sitemapProxy.ts | 29 +++++ 6 files changed, 284 insertions(+), 73 deletions(-) create mode 100644 nextjs-app/app/sitemaps/[...parts]/route.ts create mode 100644 nextjs-app/lib/sitemapProxy.ts diff --git a/backend/app.py b/backend/app.py index 678fabc..2b22d81 100644 --- a/backend/app.py +++ b/backend/app.py @@ -7,6 +7,7 @@ Uses real data from UK Government Compare School Performance downloads. import hashlib import re from contextlib import asynccontextmanager +from datetime import datetime, timezone from typing import Optional import numpy as np @@ -53,8 +54,9 @@ PHASE_GROUPS: dict[str, set[str]] = { BASE_URL = "https://www.schoolcompare.co.uk" MAX_SLUG_LENGTH = 60 -# In-memory sitemap cache -_sitemap_xml: str | None = None +# 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 def _slugify(text: str) -> str: @@ -135,16 +137,65 @@ def _school_sitemap_rows(df) -> list[str]: return rows -def build_sitemap() -> str: - """Generate sitemap XML from in-memory school data. Returns the XML string.""" +# 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 + +# 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 name.""" 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) + children: dict[str, str] = { + "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): + 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}{SITEMAP_CHILD_PREFIX}/{name}" + f"{generated}" + for name in children + ] + index = "\n".join([ + '', + '', + *index_rows, + "", + ]) + return {**children, "sitemap.xml": index} + + +def build_sitemap() -> str: + """The sitemap index. Kept for `lifespan` and the admin endpoint.""" + return build_sitemaps()["sitemap.xml"] def clean_filter_values(series: pd.Series) -> list[str]: @@ -322,7 +373,7 @@ def validate_postcode(postcode: Optional[str]) -> Optional[str]: @asynccontextmanager async def lifespan(app: FastAPI): """Application lifespan - startup and shutdown events.""" - global _sitemap_xml + global _sitemaps print("Loading school data from marts...") df = load_school_data() if df.empty: @@ -332,9 +383,9 @@ async def lifespan(app: FastAPI): # Pre-compute the latest-year snapshot so the first search request is fast await asyncio.to_thread(load_latest_school_data) try: - _sitemap_xml = build_sitemap() - n = _sitemap_xml.count("") - print(f"Sitemap built: {n} URLs.") + _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}") @@ -1125,16 +1176,28 @@ async def robots_txt(): return FileResponse(settings.frontend_dir / "robots.txt", media_type="text/plain") -@app.get("/sitemap.xml") -async def sitemap_xml(): - """Serve sitemap.xml for search engine indexing.""" - global _sitemap_xml - if _sitemap_xml is None: +def _serve_sitemap(name: str) -> Response: + global _sitemaps + if _sitemaps is None: try: - _sitemap_xml = build_sitemap() + _sitemaps = build_sitemaps() except Exception as e: raise HTTPException(status_code=503, detail=f"Sitemap unavailable: {e}") - return Response(content=_sitemap_xml, media_type="application/xml") + if name not in _sitemaps: + raise HTTPException(status_code=404, detail="No such sitemap") + return Response(content=_sitemaps[name], media_type="application/xml") + + +@app.get("/sitemap.xml") +async def sitemap_xml(): + """Serve the sitemap index.""" + return _serve_sitemap("sitemap.xml") + + +@app.get("/sitemaps/{name}") +async def sitemap_child(name: str): + """Serve a child sitemap (static.xml, or schools-N.xml).""" + return _serve_sitemap(name) @app.post("/api/admin/regenerate-sitemap") @@ -1144,10 +1207,10 @@ async def regenerate_sitemap( _: bool = Depends(verify_admin_api_key), ): """Rebuild and cache the sitemap from current school data. Called by Airflow after data updates.""" - global _sitemap_xml - _sitemap_xml = build_sitemap() - n = _sitemap_xml.count("") - return {"status": "ok", "urls": n} + global _sitemaps + _sitemaps = build_sitemaps() + n = sum(x.count("") for x in _sitemaps.values()) + return {"status": "ok", "urls": n, "sitemaps": len(_sitemaps)} # Mount static files directly (must be after all routes to avoid catching API calls) diff --git a/backend/tests/test_sitemap.py b/backend/tests/test_sitemap.py index 7564cb1..eb030b1 100644 --- a/backend/tests/test_sitemap.py +++ b/backend/tests/test_sitemap.py @@ -32,32 +32,56 @@ def _schools_df() -> pd.DataFrame: @pytest.fixture() def sitemap(monkeypatch) -> str: + """The sitemap index.""" 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): +@pytest.fixture() +def schools_child(monkeypatch) -> str: + """The first school child sitemap, where school URLs actually live.""" + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemaps()["schools-1.xml"] + + +@pytest.fixture() +def static_child(monkeypatch) -> str: + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemaps()["static.xml"] + + +def test_every_loc_uses_the_www_host(sitemaps): # 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 + # Checked across every file, index included, not just one. + for name, xml in sitemaps.items(): + assert "https://www.schoolcompare.co.uk" in xml, name + assert "https://schoolcompare.co.uk" not in xml, name -def test_school_with_results_is_listed(sitemap): - assert "/school/100001-alpha-primary" in sitemap +def test_school_with_results_is_listed(schools_child): + assert "/school/100001-alpha-primary" in schools_child -def test_school_with_no_results_and_no_ofsted_is_omitted(sitemap): +def test_school_with_no_results_and_no_ofsted_is_omitted(schools_child): # 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 + # + # Asserted against the child, not the index: the index carries no school + # URLs at all, so it would pass this trivially and prove nothing. + assert "/school/100002" not in schools_child -def test_no_invented_priority_or_changefreq(sitemap): +def test_no_invented_priority_or_changefreq(sitemaps): # Google ignores both. They were noise dressed as signal. - assert "" not in sitemap - assert "" not in sitemap + for name, xml in sitemaps.items(): + assert "" not in xml, name + assert "" not in xml, name def test_ofsted_date_becomes_lastmod(monkeypatch): @@ -70,7 +94,7 @@ def test_ofsted_date_becomes_lastmod(monkeypatch): return base monkeypatch.setattr(app_module, "load_school_data", _df) - xml = app_module.build_sitemap() + xml = app_module.build_sitemaps()["schools-1.xml"] assert "2024-03-14" in xml @@ -85,10 +109,70 @@ def test_no_lastmod_invented_when_date_unknown(monkeypatch): return df monkeypatch.setattr(app_module, "load_school_data", _df) - xml = app_module.build_sitemap() + # The child only. The index legitimately carries a lastmod, because there + # it means "when this sitemap file changed", which we do know. + xml = app_module.build_sitemaps()["schools-1.xml"] assert "" not in xml -def test_static_routes_are_listed(sitemap): +def test_static_routes_are_listed(static_child): for path in ("/", "/rankings", "/compare", "/admissions"): - assert f"https://www.schoolcompare.co.uk{path}" in sitemap + assert f"https://www.schoolcompare.co.uk{path}" in static_child + + +@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_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["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["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["schools-1.xml"].count("") == 10_000 + assert maps["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 " { +async function sitemapChildren(page: Page): Promise { const res = await page.request.get('/sitemap.xml'); expect(res.ok()).toBeTruthy(); - const xml = await res.text(); + const index = await res.text(); + expect(index).toContain('([^<]+)<\/loc>/g)].map((m) => m[1]); +} - const urlCount = (xml.match(//g) ?? []).length; - expect(urlCount, 'sitemap looks empty or truncated').toBeGreaterThan(1000); +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(''); - // 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'); + const locs = await sitemapChildren(page); + expect(locs.length).toBeGreaterThanOrEqual(2); + + for (const loc of locs) { + 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(' { + const locs = await sitemapChildren(page); + + 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 or changefreq', async ({ page }) => { + const [first] = await sitemapChildren(page); + expect(first).toBeTruthy(); + + 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(''); }); /* diff --git a/nextjs-app/app/sitemap.xml/route.ts b/nextjs-app/app/sitemap.xml/route.ts index acb504d..9cfab89 100644 --- a/nextjs-app/app/sitemap.xml/route.ts +++ b/nextjs-app/app/sitemap.xml/route.ts @@ -1,32 +1,8 @@ -/** - * Runtime proxy for /sitemap.xml → the FastAPI backend's generated sitemap. - * - * 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 here. - */ - -import { NextResponse } from 'next/server'; +import { proxySitemap } from '@/lib/sitemapProxy'; export const dynamic = 'force-dynamic'; export const runtime = 'nodejs'; -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 GET() { - let upstream: Response; - try { - upstream = await fetch(`${backendOrigin()}/sitemap.xml`, { 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' }, - }); + return proxySitemap('/sitemap.xml'); } diff --git a/nextjs-app/app/sitemaps/[...parts]/route.ts b/nextjs-app/app/sitemaps/[...parts]/route.ts new file mode 100644 index 0000000..c540b82 --- /dev/null +++ b/nextjs-app/app/sitemaps/[...parts]/route.ts @@ -0,0 +1,24 @@ +import { NextResponse } from 'next/server'; +import { proxySitemap } from '@/lib/sitemapProxy'; + +export const dynamic = 'force-dynamic'; +export const runtime = 'nodejs'; + +/** + * 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$/; + +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(`/sitemaps/${name}`); +} diff --git a/nextjs-app/lib/sitemapProxy.ts b/nextjs-app/lib/sitemapProxy.ts new file mode 100644 index 0000000..dab6359 --- /dev/null +++ b/nextjs-app/lib/sitemapProxy.ts @@ -0,0 +1,29 @@ +/** + * 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 children under /sitemaps/, 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(path: string): Promise { + let upstream: Response; + try { + upstream = await fetch(`${backendOrigin()}${path}`, { 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' }, + }); +}