From 5ddc7314fd5cc6fcbf78aa764a79654931e02195 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:10:07 +0100 Subject: [PATCH 1/5] 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. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 4 ++- backend/tests/test_sitemap.py | 44 +++++++++++++++++++++++++++ nextjs-app/__tests__/lib/site.test.ts | 27 ++++++++++++++++ nextjs-app/app/layout.tsx | 5 +-- nextjs-app/app/robots.ts | 3 +- nextjs-app/app/school/[slug]/page.tsx | 5 +-- nextjs-app/lib/site.ts | 19 ++++++++++++ 7 files changed, 101 insertions(+), 6 deletions(-) create mode 100644 backend/tests/test_sitemap.py create mode 100644 nextjs-app/__tests__/lib/site.test.ts create mode 100644 nextjs-app/lib/site.ts diff --git a/backend/app.py b/backend/app.py index 20d9570..45a7772 100644 --- a/backend/app.py +++ b/backend/app.py @@ -48,7 +48,9 @@ PHASE_GROUPS: dict[str, set[str]] = { "all-through": {"all-through"}, } -BASE_URL = "https://schoolcompare.co.uk" +# 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" MAX_SLUG_LENGTH = 60 # In-memory sitemap cache diff --git a/backend/tests/test_sitemap.py b/backend/tests/test_sitemap.py new file mode 100644 index 0000000..430db84 --- /dev/null +++ b/backend/tests/test_sitemap.py @@ -0,0 +1,44 @@ +"""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 diff --git a/nextjs-app/__tests__/lib/site.test.ts b/nextjs-app/__tests__/lib/site.test.ts new file mode 100644 index 0000000..2003959 --- /dev/null +++ b/nextjs-app/__tests__/lib/site.test.ts @@ -0,0 +1,27 @@ +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/'); + }); +}); diff --git a/nextjs-app/app/layout.tsx b/nextjs-app/app/layout.tsx index ebf9b67..20eb8d7 100644 --- a/nextjs-app/app/layout.tsx +++ b/nextjs-app/app/layout.tsx @@ -5,6 +5,7 @@ import { Navigation } from '@/components/Navigation'; import { Footer } from '@/components/Footer'; import { ComparisonToast } from '@/components/ComparisonToast'; import { ComparisonProvider } from '@/context/ComparisonProvider'; +import { SITE_URL } from '@/lib/site'; import './globals.css'; // Manrope carries headings and key messaging — the guideline's "friendly, @@ -57,12 +58,12 @@ export const metadata: Metadata = { // No `icons` key on purpose: setting it here would override the file // conventions. app/icon.svg and app/apple-icon.tsx are the source, and // app/opengraph-image.tsx supplies og:image and twitter:image. - metadataBase: new URL('https://schoolcompare.co.uk'), + metadataBase: new URL(SITE_URL), openGraph: { type: 'website', title: 'schoolcompare | Compare School Performance', description: 'Compare primary and secondary school SATs and GCSE performance across England', - url: 'https://schoolcompare.co.uk', + url: SITE_URL, siteName: 'schoolcompare', }, twitter: { diff --git a/nextjs-app/app/robots.ts b/nextjs-app/app/robots.ts index 8ab3bbc..e2cb38e 100644 --- a/nextjs-app/app/robots.ts +++ b/nextjs-app/app/robots.ts @@ -4,6 +4,7 @@ */ import { MetadataRoute } from 'next'; +import { absoluteUrl } from '@/lib/site'; export default function robots(): MetadataRoute.Robots { return { @@ -14,6 +15,6 @@ export default function robots(): MetadataRoute.Robots { disallow: ['/api/', '/_next/'], }, ], - sitemap: 'https://schoolcompare.co.uk/sitemap.xml', + sitemap: absoluteUrl('/sitemap.xml'), }; } diff --git a/nextjs-app/app/school/[slug]/page.tsx b/nextjs-app/app/school/[slug]/page.tsx index db2c229..4e9f6aa 100644 --- a/nextjs-app/app/school/[slug]/page.tsx +++ b/nextjs-app/app/school/[slug]/page.tsx @@ -15,6 +15,7 @@ import { } from '@/lib/schoolSections'; import { parseSchoolSlug, schoolUrl } from '@/lib/utils'; import type { NationalAverages } from '@/lib/types'; +import { absoluteUrl } from '@/lib/site'; import type { Metadata } from 'next'; /** @@ -97,7 +98,7 @@ export async function generateMetadata({ params }: SchoolPageProps): Promise 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}`; +} From 51ce2d637383ec9f7b92aeaf82e48ab3538d1a4d Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:10:50 +0100 Subject: [PATCH 2/5] 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. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- e2e/tests/journeys.spec.ts | 42 +++++++++++++++++++++++ nextjs-app/__tests__/app/metadata.test.ts | 23 +++++++++++++ nextjs-app/app/admissions/page.tsx | 2 ++ nextjs-app/app/page.tsx | 5 +++ nextjs-app/app/rankings/page.tsx | 4 +++ 5 files changed, 76 insertions(+) create mode 100644 nextjs-app/__tests__/app/metadata.test.ts diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 2909136..44a83f5 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1600,3 +1600,45 @@ test('the sitemap submits no Welsh or overseas school', async ({ page }) => { expect(xml).not.toContain('/school/401559'); expect(xml).not.toContain('/school/402426'); }); + +/* + * 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+-/); +}); diff --git a/nextjs-app/__tests__/app/metadata.test.ts b/nextjs-app/__tests__/app/metadata.test.ts new file mode 100644 index 0000000..7d283a3 --- /dev/null +++ b/nextjs-app/__tests__/app/metadata.test.ts @@ -0,0 +1,23 @@ +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'); + }); +}); diff --git a/nextjs-app/app/admissions/page.tsx b/nextjs-app/app/admissions/page.tsx index c4cad97..e3d2a67 100644 --- a/nextjs-app/app/admissions/page.tsx +++ b/nextjs-app/app/admissions/page.tsx @@ -1,3 +1,4 @@ +import { absoluteUrl } from '@/lib/site'; import type { Metadata } from 'next'; import { AdmissionsView } from '@/components/AdmissionsView'; @@ -7,6 +8,7 @@ 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') }, }; export default function AdmissionsPage() { diff --git a/nextjs-app/app/page.tsx b/nextjs-app/app/page.tsx index e9d89b6..750ca6e 100644 --- a/nextjs-app/app/page.tsx +++ b/nextjs-app/app/page.tsx @@ -3,6 +3,7 @@ * Main landing page with school search and browsing */ +import { absoluteUrl } from '@/lib/site'; import type { Metadata } from 'next'; import { fetchSchools, fetchFilters, fetchDataInfo } from '@/lib/api'; import { formatAcademicYear } from '@/lib/utils'; @@ -35,6 +36,10 @@ interface HomePageProps { 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('/') }, }; // The page reads searchParams, which makes rendering dynamic by default. diff --git a/nextjs-app/app/rankings/page.tsx b/nextjs-app/app/rankings/page.tsx index 6078cd3..708893a 100644 --- a/nextjs-app/app/rankings/page.tsx +++ b/nextjs-app/app/rankings/page.tsx @@ -5,6 +5,7 @@ import { fetchRankings, fetchFilters, fetchMetrics } from '@/lib/api'; import { RankingsView } from '@/components/RankingsView'; +import { absoluteUrl } from '@/lib/site'; import type { Metadata } from 'next'; interface RankingsPageProps { @@ -20,6 +21,9 @@ 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') }, }; // Dynamic via searchParams; remove force-dynamic so internal data fetches From 3816b92d06aea09278573744a784976a59eba78d Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:11:34 +0100 Subject: [PATCH 3/5] 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. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- e2e/tests/journeys.spec.ts | 18 ++++++++++++ nextjs-app/__tests__/app/metadata.test.ts | 30 +++++++++++++++++++ nextjs-app/app/compare/page.tsx | 35 ++++++++++++++++++----- 3 files changed, 76 insertions(+), 7 deletions(-) diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 44a83f5..ec4074b 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1642,3 +1642,21 @@ test('a school page canonicalises to its own slug on the www host', async ({ pag .getAttribute('href'); expect(href).toMatch(/^https:\/\/www\.schoolcompare\.co\.uk\/school\/\d+-/); }); + +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'); +}); diff --git a/nextjs-app/__tests__/app/metadata.test.ts b/nextjs-app/__tests__/app/metadata.test.ts index 7d283a3..e173201 100644 --- a/nextjs-app/__tests__/app/metadata.test.ts +++ b/nextjs-app/__tests__/app/metadata.test.ts @@ -1,6 +1,7 @@ import { metadata as homeMetadata } from '@/app/page'; import { metadata as rankingsMetadata } from '@/app/rankings/page'; import { metadata as admissionsMetadata } from '@/app/admissions/page'; +import { generateMetadata as compareMetadata } from '@/app/compare/page'; describe('canonical URLs', () => { it('the homepage canonicalises to the bare root', () => { @@ -21,3 +22,32 @@ describe('canonical URLs', () => { .toBe('https://www.schoolcompare.co.uk/admissions'); }); }); + +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'); + }); +}); diff --git a/nextjs-app/app/compare/page.tsx b/nextjs-app/app/compare/page.tsx index 0b2b4e4..2d8fb22 100644 --- a/nextjs-app/app/compare/page.tsx +++ b/nextjs-app/app/compare/page.tsx @@ -5,6 +5,7 @@ import { fetchComparison, fetchMetrics } from '@/lib/api'; import { ComparisonView } from '@/components/ComparisonView'; +import { absoluteUrl } from '@/lib/site'; import type { Metadata } from 'next'; interface ComparePageProps { @@ -14,13 +15,33 @@ interface ComparePageProps { }>; } -export const metadata: 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', -}; +/** + * 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 } }; +} // Dynamic via searchParams; remove force-dynamic so internal data fetches // can still use Next.js's per-call revalidate cache. From 24f3cb4c65f533a4929ec4242a04ebede1b04da5 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:12:20 +0100 Subject: [PATCH 4/5] fix(seo): submit only school pages that have something to show Drops the 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. lastmod is omitted where no date is known rather than defaulted to now. An always-now lastmod is a claim Google learns to distrust; absent honestly means unknown. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 94 ++++++++++++++++++++++++----------- backend/tests/test_sitemap.py | 50 +++++++++++++++++++ 2 files changed, 115 insertions(+), 29 deletions(-) diff --git a/backend/app.py b/backend/app.py index 45a7772..678fabc 100644 --- a/backend/app.py +++ b/backend/app.py @@ -72,41 +72,77 @@ def _school_url(urn: int, school_name: str) -> str: return f"/school/{urn}-{slug}" +# 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. + + 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() - static_urls = [ - (BASE_URL + "/", "daily", "1.0"), - (BASE_URL + "/rankings", "weekly", "0.8"), - (BASE_URL + "/compare", "weekly", "0.8"), - ] - lines = ['', ''] - - for url, freq, priority in static_urls: - lines.append( - f" {url}" - f"{freq}" - f"{priority}" - ) - - if not df.empty and "urn" in df.columns and "school_name" in df.columns: - seen = set() - for _, row in df[["urn", "school_name"]].drop_duplicates(subset="urn").iterrows(): - urn = int(row["urn"]) - name = str(row["school_name"]) - if urn in seen: - continue - seen.add(urn) - path = _school_url(urn, name) - lines.append( - f" {BASE_URL}{path}" - f"monthly" - f"0.6" - ) - + 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) diff --git a/backend/tests/test_sitemap.py b/backend/tests/test_sitemap.py index 430db84..7564cb1 100644 --- a/backend/tests/test_sitemap.py +++ b/backend/tests/test_sitemap.py @@ -42,3 +42,53 @@ 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 + + +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 From 2208ad93c1ed8ec980498336dde28a23612e9ae7 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:14:32 +0100 Subject: [PATCH 5/5] 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' }, + }); +}