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." ``` ---