From 22c113fc29e1d6c165645a49e62ffc722e786e63 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 19:41:00 +0100 Subject: [PATCH 01/11] docs(suggest): design for school autosuggest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The load-bearing finding is not about autosuggest. The rate limiter keys on request.client.host, which in staging and prod is the Next container — so all browser users share one 60/min bucket per route. Measured against staging: 70 concurrent requests gave exactly 60 x 200 and 10 x 429. Eight concurrent searchers would 429 the site once each keystroke costs a request, so the keying fix is part of this work. Both environments are behind Cloudflare, which sets CF-Connecting-IP and overwrites any client-supplied value — trustworthy in a way a parsed X-Forwarded-For chain is not, and the backend is unreachable except through the Next proxy. Named honestly: the shared bucket has been an accidental global throttle on a single-process backend, so correct per-user keying removes a protection. A global ceiling ships with it rather than instead of it. Suggestions come from Typesense alone. The existing search path filters a 25,000-row DataFrame per query, which is exactly the cost a keystroke endpoint cannot pay, so there is deliberately no DataFrame fallback. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../2026-08-26-school-autosuggest-design.md | 239 ++++++++++++++++++ 1 file changed, 239 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-26-school-autosuggest-design.md diff --git a/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md new file mode 100644 index 0000000..accde0d --- /dev/null +++ b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md @@ -0,0 +1,239 @@ +# School Autosuggest — Design + +**Date:** 2026-08-26 +**Status:** approved for planning +**Depends on:** the feature-flag layer (PR #125, merged) + +## Goal + +Suggest schools by name as someone types in the site's main search box, so a +parent who knows the school they want reaches it in one step instead of +searching, scanning a result list, and clicking. + +Scope is **schools only**. Places and postcodes were considered and excluded — +see *Out of scope*. + +## The finding that shapes everything + +The site's rate limiter does not do what it looks like it does. + +`limiter = Limiter(key_func=get_remote_address)` with `60/minute` reads +`request.client.host`. In staging and production the backend has no published +ports and sits on the internal `backend` network, so its only caller is the +Next proxy — and `request.client.host` is therefore **the Next container**, for +every browser user on the site. + +Measured against staging: 70 concurrent requests to `/api/schools` returned +**60 × 200 and 10 × 429**. One machine consumed the whole site's budget for +that minute. + +Autosuggest is the worst possible feature to build on that. One person typing +"st marys primary" produces six to eight debounced requests; **eight concurrent +searchers would 429 the site.** The compare modal's search-as-you-type already +shares this bucket, so the exposure exists today — autosuggest makes it +certain. + +Fixing the keying is therefore part of this work, not a follow-up. + +## 1. Rate-limit keying + +Both environments sit behind Cloudflare (`server: cloudflare`, `cf-ray` present +on staging and production). Cloudflare sets `CF-Connecting-IP` on every request +to the origin and **overwrites any client-supplied value**, which makes it +trustworthy in a way a parsed `X-Forwarded-For` chain is not. + +```python +def client_key(request: Request) -> str: + """Rate-limit bucket: the real caller, not the proxy in front of them.""" + cf = request.headers.get("cf-connecting-ip") + if cf: + return cf.strip() + xff = request.headers.get("x-forwarded-for") + if xff: + return xff.split(",")[0].strip() + return get_remote_address(request) +``` + +`nextjs-app/app/api/[...path]/route.ts` already forwards every inbound header +except `host` and `connection`, so `CF-Connecting-IP` reaches the backend with +no proxy change. + +Two things make this safe: Cloudflare replaces the header, so a browser cannot +forge it; and the backend is unreachable from outside the Docker network, so +nothing can reach it without passing through the proxy. The `X-Forwarded-For` +fallback *is* forgeable, but only by a caller already inside that network. + +### The part that is not free + +The shared bucket has been acting as an accidental global throttle on a +single-process uvicorn backend that filters a 25,000-row DataFrame in-process. +Correct per-user keying removes that throttle: the origin becomes reachable at +60/min *per user* rather than 60/min in total. + +So the keying fix ships **with a global ceiling**, not instead of one: +slowapi's application-wide `default_limits`, set above any plausible real load +but below what would flood a single worker. **3000/minute**, against a +per-user suggest limit of 120/minute — so twenty-five simultaneously active +searchers are unaffected, while a runaway client or a scraper still meets a +wall. + +Per-user fairness and origin protection are different jobs and need different +limits. Conflating them is what produced the current behaviour. + +Both numbers are estimates, not measurements. They are a starting point to +revisit against real traffic once the keying is correct enough for real +traffic to be visible — which it is not today, because everyone shares one +bucket. + +## 2. `GET /api/suggest` + +A dedicated endpoint, not a mode of `/api/schools`. + +The existing search path calls Typesense for URNs and then filters, ranks and +sorts the full in-memory DataFrame — a pandas pass per keystroke, holding the +GIL and blocking other requests in the same worker. Suggestions need none of +it: `urn`, `school_name`, `phase`, `school_type`, `local_authority`, +`postcode` and `ofsted_rating` are all already in the Typesense document +(`pipeline/scripts/sync_typesense.py`). + +``` +GET /api/suggest?q=&limit=8 +→ 200 {"suggestions": [ + {"urn": 100010, "school_name": "Brecknock Primary School", + "local_authority": "Camden", "postcode": "NW1 1AA", + "phase": "Primary", "school_type": "Community school"} + ]} +``` + +- **Under two characters** returns `{"suggestions": []}` with 200. The + keystroke path never returns an error for ordinary input. +- **Typesense unavailable** returns `{"suggestions": []}` with 200. There is + deliberately **no DataFrame fallback**: the substring scan `/api/schools` + falls back to is precisely the cost this endpoint exists to avoid, and a + silent 25,000-row scan per keystroke is worse than no suggestions. +- **`limit` is clamped** to 20. It is a public endpoint. +- **Rate limit `120/minute`** per client, not the default 60. A 200 ms + debounce tops out near 5 requests/second while someone is actively typing, + but averages far below that across a real search; 120 leaves headroom for + bursts without letting one client alone consume the global ceiling. +- **Local authority is part of the payload, not decoration.** There are many + schools called "St Mary's"; a suggestion list without the authority is + unusable for exactly the queries autosuggest is meant to serve. + +### Caching + +`CACHE_RULES` gains `("/api/suggest", (60, 3600, 86400))`. Prefix queries +repeat enormously across users and school names change once a year. + +The client fetch must **not** use `cache: "no-store"`. The compare modal does, +and copying that pattern would throw away both the browser cache and the ETag +304s the existing `CacheAndETagMiddleware` already provides. + +Both environments currently report `cf-cache-status: DYNAMIC` — Cloudflare +ignores the `Cache-Control` the API already sends, because it does not cache +dynamic paths by default. **A Cloudflare Cache Rule for `/api/suggest*` would +let the edge absorb most of this traffic and never reach the origin.** That is +a dashboard change, it is optional, and nothing here depends on it. + +## 3. The combobox + +This is an ARIA combobox, not a text input with a list underneath. + +**Files.** `FilterBar.tsx` is already long. The work splits three ways: +`hooks/useSchoolSuggest.ts` owns fetching, debouncing and cancellation; +`components/SuggestList.tsx` owns rendering and ARIA; `FilterBar.tsx` wires +them to the existing input and form. + +**Fetching.** 200 ms debounce; minimum two characters; an `AbortController` +cancels the superseded request on every keystroke. Cancellation is not an +optimisation — without it, a slow response for `"st"` can land after the fast +one for `"st marys"` and replace a correct list with a stale one. + +**Suppressed during postcode entry.** The box takes a school name *or* a +postcode, and `isValidPostcode` already distinguishes them. Suggestions do not +appear once the value parses as a postcode. + +**Keyboard.** `ArrowDown`/`ArrowUp` move the active option, `Escape` closes and +keeps the typed text, `Tab` closes. `Enter` **with an option active** navigates +to that school's page. `Enter` **with none active** submits the free-text +search exactly as it does today — the existing behaviour is preserved, not +replaced. + +**ARIA.** `role="combobox"` with `aria-expanded` and `aria-controls` on the +input, `aria-activedescendant` pointing at the active option, `role="listbox"` +on the list and `role="option"` on each row. + +**Both instances get it.** `HomeView` renders `FilterBar` twice — hero and +sticky — from one component, so there is one implementation. + +## 4. Behind a flag + +Flag `school_autosuggest`, declared in `backend/flags.py`, default off. + +This is the most-used control on the site and the first change to it in a +while. `app/page.tsx` is an async server component, so it reads the flag and +threads it to `FilterBar` through `HomeView` — two prop hops, explicit, no +client-side flag read. + +Off means the input behaves exactly as it does today: no listener, no fetch, no +markup. Not a rendered-then-hidden dropdown. + +The rate-limit keying is **not** flagged. It is a correctness fix that should +apply whether or not autosuggest is on, and flagging it would mean shipping a +known-wrong limiter into production deliberately. + +## 5. Analytics + +`search_submitted` already carries `via: 'input'`. Selecting a suggestion fires +it with `via: 'suggestion'` plus the chosen `urn`, so the obvious question — +does this actually help, or do people ignore it — has an answer in the data +rather than an opinion. + +## 6. Testing + +**Backend.** `client_key` prefers `CF-Connecting-IP`, falls back through +`X-Forwarded-For` to the remote address, and two different values get two +different buckets. `/api/suggest` returns matches, returns empty below two +characters, returns empty and 200 when Typesense is unavailable, and clamps +`limit`. That it never touches the DataFrame is asserted by making +`load_school_data` raise and requiring the endpoint to answer anyway. + +**Frontend.** The hook debounces, aborts superseded requests, and drops a +late-arriving response for a stale query. The list renders the ARIA +attributes. Keyboard navigation moves the active option; `Enter` on an option +navigates; `Enter` on none submits the search. + +**E2E.** With the flag on, typing a known school name shows it and selecting it +lands on that school's page. With the flag off, no combobox markup exists. +Gated on the flag the same way the distance journeys are — read the observable +effect, since `/api/flags` is denied to the public. + +## 7. Risks + +**Removing the accidental throttle.** Covered in §1. The global ceiling is the +mitigation, and 1200/minute is a guess that should be revisited against real +traffic rather than treated as a considered number. + +**Cloudflare bypass.** If the origin is reachable without passing through +Cloudflare, `CF-Connecting-IP` is absent and the `X-Forwarded-For` fallback is +forgeable, so limits could be evaded per-request. Closing that properly means +Authenticated Origin Pulls or an origin firewall, which is infrastructure work +outside this change. Worth doing separately. + +**Typesense becomes user-visible.** Today a Typesense outage degrades search to +a slow substring match. With autosuggest it also means the dropdown silently +stops appearing. That is the correct failure — quiet, not broken — but it makes +Typesense health worth monitoring in a way it was not before. + +## Out of scope + +- **Place suggestions.** The 2,646 town, authority and outcode pages are a + strong candidate and would route people onto the pages W2 built, but they + live in the place registry rather than Typesense, so it is a second index and + a ranking rule for comparing two kinds of result. Worth its own change. +- **Postcode completion.** Would put postcodes.io in the keystroke path, with + its own latency and rate limits. +- **The compare modal.** It already has search-as-you-type. Converting it to + this component is a reasonable follow-up, not part of this. +- **Recent or popular searches.** No storage for either, and no evidence yet + that they are wanted. From e651dd0d65f018dcdd8b71c3b607cbd6c94a2da6 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 19:44:21 +0100 Subject: [PATCH 02/11] docs(suggest): the in-app global ceiling would not have worked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading slowapi rather than assuming: default_limits and application_limits are both evaluated with the same key_func, so they are per-client across routes, not global. And application_limits only apply 'if in_middleware' — this app installs no SlowAPIMiddleware, so they would never have fired at all. A genuine global cap would need a second Limiter with a constant key plus that middleware. Cloudflare is already in the path on both environments and does this at the right layer, so the ceiling is named as a follow-up there rather than built badly here. The risk that leaves is stated plainly in the risks section instead of being papered over with a mechanism that does not do the job. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../2026-08-26-school-autosuggest-design.md | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md index accde0d..3e798b0 100644 --- a/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md +++ b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md @@ -70,20 +70,30 @@ single-process uvicorn backend that filters a 25,000-row DataFrame in-process. Correct per-user keying removes that throttle: the origin becomes reachable at 60/min *per user* rather than 60/min in total. -So the keying fix ships **with a global ceiling**, not instead of one: -slowapi's application-wide `default_limits`, set above any plausible real load -but below what would flood a single worker. **3000/minute**, against a -per-user suggest limit of 120/minute — so twenty-five simultaneously active -searchers are unaffected, while a runaway client or a scraper still meets a -wall. +Per-user fairness and origin protection are different jobs. Conflating them +is what produced the current behaviour, and the fix must not quietly do it +again in the other direction. -Per-user fairness and origin protection are different jobs and need different -limits. Conflating them is what produced the current behaviour. +**The global ceiling does not go in this app.** An earlier draft of this +section specified one via slowapi's `default_limits`. Reading the library +shows that would not have worked, twice over: `default_limits` and +`application_limits` are both evaluated with the same `key_func`, so they are +per-client across all routes rather than global; and `application_limits` are +only applied `if in_middleware`, while this app installs no `SlowAPIMiddleware` +at all. Expressing a genuine global cap would take a second `Limiter` with a +constant key plus that middleware — two mechanisms to keep correct, for a +protection this layer is the wrong place for. -Both numbers are estimates, not measurements. They are a starting point to -revisit against real traffic once the keying is correct enough for real -traffic to be visible — which it is not today, because everyone shares one -bucket. +Cloudflare is already in the request path on both environments and does +edge-level rate limiting properly, before traffic reaches a single-process +origin at all. That is where a global ceiling belongs, and it is a dashboard +change rather than code. Flagged as a follow-up, deliberately not built here. + +What ships instead is conservative per-user limits: the existing 60/minute +default is unchanged, and `/api/suggest` gets 120/minute. Both are estimates +rather than measurements, and they are a starting point to revisit once the +keying is correct enough for real per-user traffic to be visible — which it +is not today, because everyone shares one bucket. ## 2. `GET /api/suggest` @@ -115,7 +125,7 @@ GET /api/suggest?q=&limit=8 - **Rate limit `120/minute`** per client, not the default 60. A 200 ms debounce tops out near 5 requests/second while someone is actively typing, but averages far below that across a real search; 120 leaves headroom for - bursts without letting one client alone consume the global ceiling. + bursts while still bounding one client. - **Local authority is part of the payload, not decoration.** There are many schools called "St Mary's"; a suggestion list without the authority is unusable for exactly the queries autosuggest is meant to serve. @@ -210,9 +220,13 @@ effect, since `/api/flags` is denied to the public. ## 7. Risks -**Removing the accidental throttle.** Covered in §1. The global ceiling is the -mitigation, and 1200/minute is a guess that should be revisited against real -traffic rather than treated as a considered number. +**Removing the accidental throttle.** Covered in §1. Correct per-user keying +means the origin is reachable at 60/minute *per user* where it was 60/minute +in total, and no in-app global cap replaces it — that job goes to Cloudflare, +which is not done as part of this change. Until it is, a determined caller +with many source addresses can put more load on a single-process origin than +they can today. Against this site's traffic that is a theoretical risk rather +than a live one, but it is a real one and it is the price of the fix. **Cloudflare bypass.** If the origin is reachable without passing through Cloudflare, `CF-Connecting-IP` is absent and the `X-Forwarded-For` fallback is From 6e0a27834066283709a1adeb81732c9154952097 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 19:48:39 +0100 Subject: [PATCH 03/11] docs(suggest): implementation plan, eight tasks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review caught three defects in the plan. .omniBoxContainer, the wrapper the dropdown positions against, does not declare position: relative — without it the list anchors to the page. The postcode suppression test typed character by character, so it would have asserted no request while 'NW1' legitimately fires one; it now sets the value in one go. And the Enter-submits-search assertion needed waitFor, because updateURL pushes inside startTransition. Task 1 is the one to review hardest: it is the only unflagged change and it alters rate limiting for every endpoint. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../plans/2026-08-26-school-autosuggest.md | 1353 +++++++++++++++++ 1 file changed, 1353 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-26-school-autosuggest.md diff --git a/docs/superpowers/plans/2026-08-26-school-autosuggest.md b/docs/superpowers/plans/2026-08-26-school-autosuggest.md new file mode 100644 index 0000000..626a61e --- /dev/null +++ b/docs/superpowers/plans/2026-08-26-school-autosuggest.md @@ -0,0 +1,1353 @@ +# School Autosuggest 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:** Suggest schools by name as someone types in the site's main search box, so a parent who knows the school they want reaches it in one step. + +**Architecture:** A dedicated `/api/suggest` endpoint answers from Typesense alone, touching no pandas. The frontend adds an ARIA combobox to the existing `FilterBar` omni-input, behind the `school_autosuggest` flag. First, the rate limiter is fixed to key on the real caller rather than the Next container — without that, autosuggest 429s the site. + +**Tech Stack:** FastAPI, slowapi, Typesense, Next.js 15 App Router, React, Playwright, pytest, Jest. + +**Spec:** `docs/superpowers/specs/2026-08-26-school-autosuggest-design.md` + +## Global Constraints + +- **The rate-limit fix is not flagged.** It is a correctness fix that applies whether or not autosuggest is on. +- **`/api/suggest` is not flagged either.** Only the UI is. A live endpoint with the UI dark is deliberate — it lets the endpoint be smoke-tested before the feature is switched on. +- **The keystroke path never returns an error for ordinary input.** Short query, no matches, Typesense down — all `200` with `{"suggestions": []}`. +- **No DataFrame fallback in `/api/suggest`.** The 25,000-row substring scan `/api/schools` falls back to is exactly the cost this endpoint exists to avoid. +- **`Enter` with no active option must still submit the free-text search**, exactly as today. +- **The client fetch must not use `cache: "no-store"`** — it would discard the browser cache and the ETag 304s the existing middleware already provides. +- **Every flag defaults to `False`**, declared in `backend/flags.py` with a `name`/`description`/`added` triple. +- Backend tests run via: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` +- Never push to `main`. Branch, PR, let checks pass. +- Do not start a local server to test the application; it does not work. + +## File Structure + +| File | Responsibility | +|---|---| +| `backend/app.py` (modify) | `client_key`, the `Limiter` construction, `GET /api/suggest`, one `CACHE_RULES` row. | +| `backend/data_loader.py` (modify) | `suggest_schools_typesense` — returns documents, not URNs. | +| `backend/flags.py` (modify) | Declare `school_autosuggest`. | +| `backend/tests/test_rate_limit_key.py` (new) | Keying precedence and bucket separation. | +| `backend/tests/test_suggest.py` (new) | Endpoint behaviour, including that it never touches the DataFrame. | +| `nextjs-app/lib/suggest.ts` (new) | `fetchSuggestions(q, signal)` — one typed fetch. | +| `nextjs-app/hooks/useSchoolSuggest.ts` (new) | Debounce, abort, stale-response rejection, open/active state. | +| `nextjs-app/components/SuggestList.tsx` (new) | Presentational list + ARIA. No fetching. | +| `nextjs-app/components/SuggestList.module.css` (new) | Dropdown styling. | +| `nextjs-app/components/FilterBar.tsx` (modify) | Wire hook + list to the existing input and form. | +| `nextjs-app/components/HomeView.tsx` (modify) | Pass `autosuggest` through to both `FilterBar` instances. | +| `nextjs-app/app/page.tsx` (modify) | Read the flag server-side, pass to both `HomeView` render sites. | +| `e2e/tests/journeys.spec.ts` (modify) | Flag-gated journeys. | + +--- + +### Task 1: Key the rate limiter on the real caller + +**Files:** +- Modify: `backend/app.py` +- Create: `backend/tests/test_rate_limit_key.py` + +**Interfaces:** +- Produces: `backend.app.client_key(request: Request) -> str` + +> Measured on staging before this change: 70 concurrent requests to +> `/api/schools` returned exactly 60 × 200 and 10 × 429. One machine consumed +> the whole site's budget, because `get_remote_address` returns the Next +> container's IP for every browser user. + +- [ ] **Step 1: Write the failing test** + +Create `backend/tests/test_rate_limit_key.py`: + +```python +"""The rate-limit bucket must be the caller, not the proxy in front of them. + +`get_remote_address` reads request.client.host. In staging and production the +backend has no published ports and its only caller is the Next proxy, so that +host is the Next container — one bucket for every browser user on the site. +Measured before this fix: 70 concurrent requests, 60 served and 10 refused. +""" + +from starlette.datastructures import Headers + +from backend.app import client_key + + +class _Req: + """Enough of a Request for the key function: headers and a client host.""" + + def __init__(self, headers: dict, host: str = "10.0.0.9"): + self.headers = Headers(headers) + self.client = type("C", (), {"host": host})() + self.scope = {"type": "http", "client": (host, 0), + "headers": [(k.lower().encode(), v.encode()) + for k, v in headers.items()]} + + +def test_cloudflare_header_wins(): + # Cloudflare sets CF-Connecting-IP and overwrites any client-supplied + # value, so it is trustworthy in a way a parsed XFF chain is not. + assert client_key(_Req({"cf-connecting-ip": "203.0.113.7"})) == "203.0.113.7" + + +def test_forwarded_for_is_the_fallback_and_takes_the_first_entry(): + # Left-most is the original client; everything after it is proxies. + assert client_key( + _Req({"x-forwarded-for": "203.0.113.7, 10.0.0.2"})) == "203.0.113.7" + + +def test_remote_address_is_the_last_resort(): + assert client_key(_Req({}, host="10.0.0.9")) == "10.0.0.9" + + +def test_cloudflare_header_beats_forwarded_for(): + key = client_key(_Req({"cf-connecting-ip": "203.0.113.7", + "x-forwarded-for": "198.51.100.1"})) + assert key == "203.0.113.7" + + +def test_two_callers_get_two_buckets(): + # The whole point: one user exhausting their limit must not refuse another. + a = client_key(_Req({"cf-connecting-ip": "203.0.113.7"})) + b = client_key(_Req({"cf-connecting-ip": "203.0.113.8"})) + assert a != b + + +def test_whitespace_is_stripped(): + # "a, b" split on comma leaves a leading space on every entry but the + # first; an unstripped key silently creates a second bucket per client. + assert client_key(_Req({"x-forwarded-for": " 203.0.113.7 ,10.0.0.2"})) \ + == "203.0.113.7" +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_rate_limit_key.py -q` + +Expected: FAIL — `ImportError: cannot import name 'client_key' from 'backend.app'` + +- [ ] **Step 3: Implement the key function** + +In `backend/app.py`, replace: + +```python +# Rate limiter +limiter = Limiter(key_func=get_remote_address) +``` + +with: + +```python +def client_key(request: Request) -> str: + """The rate-limit bucket: the real caller, not the proxy in front of them. + + `get_remote_address` reads request.client.host. In staging and production + the backend has no published ports and sits on the internal network, so its + only caller is the Next proxy — meaning every browser user on the site + shared one bucket. Measured before this fix: 70 concurrent requests to + /api/schools returned 60 OK and 10 refused. + + CF-Connecting-IP first, because Cloudflare (in front of both environments) + sets it on every origin request and *overwrites* any client-supplied value, + which a parsed X-Forwarded-For chain does not guarantee. The XFF fallback is + forgeable, but only by a caller already inside the Docker network, which is + the one place nothing untrusted can reach. + """ + cf = request.headers.get("cf-connecting-ip") + if cf: + return cf.strip() + xff = request.headers.get("x-forwarded-for") + if xff: + return xff.split(",")[0].strip() + return get_remote_address(request) + + +# Rate limiter. No in-app global ceiling: slowapi's default_limits and +# application_limits are both keyed by key_func (so per-client, not global) +# and the latter only applies with SlowAPIMiddleware installed, which this app +# does not use. A global cap belongs at Cloudflare, which is already in the +# path. See the spec's §1 for why that is deliberate. +limiter = Limiter(key_func=client_key) +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_rate_limit_key.py -q` + +Expected: PASS, 6 tests. + +- [ ] **Step 5: Run the full backend suite** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` + +Expected: all pass. + +- [ ] **Step 6: Commit** + +```bash +git add backend/app.py backend/tests/test_rate_limit_key.py +git commit -m "fix(api): rate-limit per caller, not per proxy" +``` + +--- + +### Task 2: `suggest_schools_typesense` + +**Files:** +- Modify: `backend/data_loader.py` +- Create: `backend/tests/test_suggest.py` + +**Interfaces:** +- Produces: `backend.data_loader.suggest_schools_typesense(query: str, limit: int = 8) -> list[dict]`, each dict carrying `urn` (int), `school_name`, `local_authority`, `postcode`, `phase`, `school_type` (all str, `""` when the document omits an optional field). + +> The existing `search_schools_typesense` returns URNs only, which forces the +> caller to hydrate from the DataFrame. Suggestions need the fields Typesense +> already holds — see the schema in `pipeline/scripts/sync_typesense.py`. + +- [ ] **Step 1: Write the failing test** + +Create `backend/tests/test_suggest.py`: + +```python +"""Tests for school autosuggest (spec 2026-08-26).""" + +import pytest + +from backend import data_loader + + +class _FakeDocs: + def __init__(self, hits, explode=False): + self._hits = hits + self._explode = explode + self.last_params = None + + def search(self, params): + self.last_params = params + if self._explode: + raise RuntimeError("typesense is down") + return {"hits": [{"document": d} for d in self._hits]} + + +class _FakeClient: + def __init__(self, hits, explode=False): + self.docs = _FakeDocs(hits, explode) + self.collections = {"schools": type("C", (), {"documents": self.docs})()} + + +_HIT = { + "urn": 100010, "school_name": "Brecknock Primary School", + "local_authority": "Camden", "postcode": "NW1 1AA", + "phase": "Primary", "school_type": "Community school", +} + + +def _use(monkeypatch, client): + monkeypatch.setattr(data_loader, "_get_typesense_client", lambda: client) + + +def test_returns_the_fields_a_suggestion_needs(monkeypatch): + # Local authority is not decoration: there are many schools called + # "St Mary's", and a list without it cannot be chosen between. + _use(monkeypatch, _FakeClient([_HIT])) + out = data_loader.suggest_schools_typesense("breck") + assert out == [{ + "urn": 100010, "school_name": "Brecknock Primary School", + "local_authority": "Camden", "postcode": "NW1 1AA", + "phase": "Primary", "school_type": "Community school", + }] + + +def test_a_missing_optional_field_becomes_an_empty_string(monkeypatch): + # phase and school_type are optional in the Typesense schema. A missing + # key must not KeyError in the keystroke path. + _use(monkeypatch, _FakeClient([{"urn": 1, "school_name": "X", + "local_authority": "Y", "postcode": "Z"}])) + out = data_loader.suggest_schools_typesense("x") + assert out[0]["phase"] == "" and out[0]["school_type"] == "" + + +def test_typesense_unavailable_gives_no_suggestions_rather_than_raising(monkeypatch): + _use(monkeypatch, None) + assert data_loader.suggest_schools_typesense("anything") == [] + + +def test_a_typesense_error_gives_no_suggestions_rather_than_raising(monkeypatch): + _use(monkeypatch, _FakeClient([], explode=True)) + assert data_loader.suggest_schools_typesense("anything") == [] + + +def test_the_limit_is_passed_through_and_clamped(monkeypatch): + client = _FakeClient([]) + _use(monkeypatch, client) + data_loader.suggest_schools_typesense("x", limit=500) + assert client.docs.last_params["per_page"] == 20 +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_suggest.py -q` + +Expected: FAIL — `AttributeError: module 'backend.data_loader' has no attribute 'suggest_schools_typesense'` + +- [ ] **Step 3: Implement it** + +In `backend/data_loader.py`, directly below `search_schools_typesense`: + +```python +# The most a public endpoint will return in one response. +SUGGEST_MAX_LIMIT = 20 + +# Fields a suggestion row carries, and the default when the document omits an +# optional one. phase and school_type are optional in the Typesense schema. +_SUGGEST_FIELDS = ("school_name", "local_authority", "postcode", + "phase", "school_type") + + +def suggest_schools_typesense(query: str, limit: int = 8) -> List[dict]: + """Autosuggest rows straight from Typesense. Never raises. + + Returns documents rather than URNs, unlike search_schools_typesense, so the + caller needs no DataFrame. Every field below is already in the index — see + pipeline/scripts/sync_typesense.py — which is what makes this cheap enough + to run per keystroke. + """ + client = _get_typesense_client() + if client is None: + return [] + try: + result = client.collections["schools"].documents.search({ + "q": query, + "query_by": "school_name,local_authority", + "per_page": max(1, min(limit, SUGGEST_MAX_LIMIT)), + "typo_tokens_threshold": 1, + }) + except Exception: + # A dropdown that quietly stops appearing is the right failure here. + return [] + + rows = [] + for hit in result.get("hits", []): + doc = hit.get("document", {}) + row = {"urn": int(doc.get("urn", 0))} + row.update({f: str(doc.get(f, "") or "") for f in _SUGGEST_FIELDS}) + rows.append(row) + return rows +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_suggest.py -q` + +Expected: PASS, 5 tests. + +- [ ] **Step 5: Commit** + +```bash +git add backend/data_loader.py backend/tests/test_suggest.py +git commit -m "feat(suggest): Typesense rows for autosuggest, no DataFrame" +``` + +--- + +### Task 3: `GET /api/suggest` + +**Files:** +- Modify: `backend/app.py` +- Modify: `backend/tests/test_suggest.py` + +**Interfaces:** +- Consumes: `backend.data_loader.suggest_schools_typesense(query, limit) -> list[dict]` +- Produces: `GET /api/suggest?q=&limit=` → `200 {"suggestions": [...]}` + +- [ ] **Step 1: Write the failing tests** + +Append to `backend/tests/test_suggest.py`: + +```python +def _client(monkeypatch, rows, *, blow_up_dataframe=False): + from fastapi.testclient import TestClient + from backend import app as app_module + + monkeypatch.setattr(app_module, "suggest_schools_typesense", + lambda q, limit=8: rows) + if blow_up_dataframe: + def _boom(): + raise AssertionError("the suggest path must not load the DataFrame") + monkeypatch.setattr(app_module, "load_school_data", _boom) + monkeypatch.setattr(app_module, "load_latest_school_data", _boom) + return TestClient(app_module.app, raise_server_exceptions=False) + + +def test_the_endpoint_returns_suggestions(monkeypatch): + body = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck").json() + assert body["suggestions"][0]["school_name"] == "Brecknock Primary School" + + +def test_the_endpoint_never_touches_the_dataframe(monkeypatch): + """The whole reason this is not a mode of /api/schools. + + That endpoint filters and sorts 25,000 rows of pandas per query, holding + the GIL. Per keystroke, that is the cost this endpoint exists to avoid. + """ + res = _client(monkeypatch, [_HIT], blow_up_dataframe=True).get("/api/suggest?q=breck") + assert res.status_code == 200 + assert res.json()["suggestions"] + + +def test_a_one_character_query_returns_nothing_and_does_not_error(monkeypatch): + # The keystroke path never errors on ordinary input. + res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=b") + assert res.status_code == 200 + assert res.json() == {"suggestions": []} + + +def test_a_blank_query_returns_nothing_and_does_not_error(monkeypatch): + res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=") + assert res.status_code == 200 + assert res.json() == {"suggestions": []} + + +def test_typesense_down_is_an_empty_list_not_a_500(monkeypatch): + res = _client(monkeypatch, []).get("/api/suggest?q=breck") + assert res.status_code == 200 + assert res.json() == {"suggestions": []} + + +def test_the_response_is_cacheable(monkeypatch): + # Prefix queries repeat enormously across users, and school names change + # once a year. Without this the endpoint pays full price every keystroke. + res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck") + assert "s-maxage" in res.headers.get("cache-control", "") + assert res.headers.get("etag") +``` + +- [ ] **Step 2: Run them to verify they fail** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_suggest.py -q` + +Expected: FAIL — the endpoint 404s, so `body["suggestions"]` raises `KeyError`. + +- [ ] **Step 3: Import the helper** + +In `backend/app.py`, add `suggest_schools_typesense` to the existing +`from .data_loader import (...)` block, alongside `search_schools_typesense`. + +- [ ] **Step 4: Add the cache rule** + +In `backend/app.py`, add to `CACHE_RULES` — before the `/api/schools` row, so +the longest-prefix match is unaffected: + +```python + ("/api/suggest", (60, 3600, 86400)), +``` + +- [ ] **Step 5: Add the endpoint** + +In `backend/app.py`, directly above `@app.get("/api/flags")`: + +```python +# Two characters. One is not a query — it matches thousands of schools and the +# response is useless, so it is not worth a round trip. +SUGGEST_MIN_QUERY = 2 + + +@app.get("/api/suggest") +@limiter.limit("120/minute") +async def suggest_schools( + request: Request, + q: str = Query("", max_length=100), + limit: int = Query(8, ge=1, le=20), +): + """School name suggestions, from Typesense alone. + + Deliberately not a mode of /api/schools: that path filters and sorts the + full in-memory DataFrame, which is far too expensive to run per keystroke. + + Nothing here returns an error for ordinary input. A short query, no + matches, or Typesense being unreachable are all 200 with an empty list — + a dropdown that quietly does not appear is the right failure for a + keystroke path, and there is no DataFrame fallback because the 25,000-row + substring scan is precisely what this endpoint exists to avoid. + + 120/minute rather than the default 60: a 200 ms debounce makes typing + legitimately bursty. + """ + query = q.strip() + if len(query) < SUGGEST_MIN_QUERY: + return {"suggestions": []} + return {"suggestions": suggest_schools_typesense(query, limit)} +``` + +- [ ] **Step 6: Run the tests to verify they pass** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` + +Expected: all pass. + +- [ ] **Step 7: Commit** + +```bash +git add backend/app.py backend/tests/test_suggest.py +git commit -m "feat(suggest): GET /api/suggest, cacheable and DataFrame-free" +``` + +--- + +### Task 4: The client and the hook + +**Files:** +- Create: `nextjs-app/lib/suggest.ts` +- Create: `nextjs-app/hooks/useSchoolSuggest.ts` +- Create: `nextjs-app/__tests__/hooks/useSchoolSuggest.test.tsx` + +**Interfaces:** +- Produces: + - `export interface Suggestion { urn: number; school_name: string; local_authority: string; postcode: string; phase: string; school_type: string }` + - `fetchSuggestions(q: string, signal?: AbortSignal): Promise` + - `useSchoolSuggest(query: string, enabled: boolean): { suggestions: Suggestion[]; open: boolean; activeIndex: number; setActiveIndex: (i: number) => void; close: () => void }` + +- [ ] **Step 1: Write `lib/suggest.ts`** + +```typescript +/** + * Client for /api/suggest. + * + * No `cache: "no-store"`. The compare modal's search uses it, and copying that + * here would discard both the browser cache and the ETag 304s the backend's + * CacheAndETagMiddleware already provides — on the one endpoint where prefix + * queries repeat most. + */ + +export interface Suggestion { + urn: number; + school_name: string; + local_authority: string; + postcode: string; + phase: string; + school_type: string; +} + +/** Below this the response is thousands of schools and worth no round trip. */ +export const SUGGEST_MIN_QUERY = 2; + +const API = process.env.NEXT_PUBLIC_API_URL || '/api'; + +/** Suggestions for `q`. Never throws: no suggestions is a fine outcome. */ +export async function fetchSuggestions( + q: string, signal?: AbortSignal, +): Promise { + if (q.trim().length < SUGGEST_MIN_QUERY) return []; + try { + const res = await fetch(`${API}/suggest?q=${encodeURIComponent(q.trim())}`, + { signal }); + if (!res.ok) return []; + const body = await res.json(); + return body.suggestions ?? []; + } catch { + // Includes AbortError, which is the normal path on every keystroke. + return []; + } +} +``` + +- [ ] **Step 2: Write the failing hook test** + +Create `nextjs-app/__tests__/hooks/useSchoolSuggest.test.tsx`: + +```tsx +import { renderHook, act, waitFor } from '@testing-library/react'; +import { useSchoolSuggest } from '@/hooks/useSchoolSuggest'; + +const realFetch = global.fetch; + +function mockFetch(rows: unknown[], delayMs = 0) { + global.fetch = jest.fn(async (_url: unknown, init?: { signal?: AbortSignal }) => { + if (delayMs) { + await new Promise((resolve, reject) => { + const t = setTimeout(resolve, delayMs); + init?.signal?.addEventListener('abort', () => { + clearTimeout(t); + reject(Object.assign(new Error('aborted'), { name: 'AbortError' })); + }); + }); + } + return { ok: true, json: async () => ({ suggestions: rows }) }; + }) as unknown as typeof fetch; +} + +const ROW = { + urn: 1, school_name: 'Brecknock Primary School', local_authority: 'Camden', + postcode: 'NW1 1AA', phase: 'Primary', school_type: 'Community school', +}; + +describe('useSchoolSuggest', () => { + beforeEach(() => { jest.useFakeTimers(); }); + afterEach(() => { jest.useRealTimers(); global.fetch = realFetch; }); + + it('does not fetch below the minimum query length', () => { + mockFetch([ROW]); + renderHook(() => useSchoolSuggest('b', true)); + act(() => { jest.advanceTimersByTime(500); }); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('does not fetch at all when disabled', () => { + // The flag being off must mean no request, not a hidden dropdown. + mockFetch([ROW]); + renderHook(() => useSchoolSuggest('brecknock', false)); + act(() => { jest.advanceTimersByTime(500); }); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('debounces rather than firing per keystroke', () => { + mockFetch([ROW]); + const { rerender } = renderHook( + ({ q }) => useSchoolSuggest(q, true), { initialProps: { q: 'br' } }); + rerender({ q: 'bre' }); + rerender({ q: 'brec' }); + act(() => { jest.advanceTimersByTime(199); }); + expect(global.fetch).not.toHaveBeenCalled(); + act(() => { jest.advanceTimersByTime(2); }); + expect(global.fetch).toHaveBeenCalledTimes(1); + }); + + it('opens with results once they arrive', async () => { + mockFetch([ROW]); + const { result } = renderHook(() => useSchoolSuggest('brecknock', true)); + act(() => { jest.advanceTimersByTime(200); }); + await waitFor(() => expect(result.current.suggestions).toHaveLength(1)); + expect(result.current.open).toBe(true); + }); + + it('close() hides the list without clearing the query', async () => { + mockFetch([ROW]); + const { result } = renderHook(() => useSchoolSuggest('brecknock', true)); + act(() => { jest.advanceTimersByTime(200); }); + await waitFor(() => expect(result.current.open).toBe(true)); + act(() => { result.current.close(); }); + expect(result.current.open).toBe(false); + }); +}); +``` + +- [ ] **Step 3: Run it to verify it fails** + +Run: `cd nextjs-app && npx jest __tests__/hooks/useSchoolSuggest.test.tsx` + +Expected: FAIL — `Cannot find module '@/hooks/useSchoolSuggest'` + +- [ ] **Step 4: Write the hook** + +Create `nextjs-app/hooks/useSchoolSuggest.ts`: + +```typescript +'use client'; + +import { useEffect, useRef, useState } from 'react'; +import { fetchSuggestions, SUGGEST_MIN_QUERY, type Suggestion } from '@/lib/suggest'; + +/* + * Long enough that a fast typist does not fire a request per character, short + * enough that the list feels attached to the keyboard. + */ +const DEBOUNCE_MS = 200; + +export function useSchoolSuggest(query: string, enabled: boolean) { + const [suggestions, setSuggestions] = useState([]); + const [open, setOpen] = useState(false); + const [activeIndex, setActiveIndex] = useState(-1); + // Set when the user dismisses the list, so a re-render does not reopen it. + const dismissed = useRef(''); + + useEffect(() => { + const q = query.trim(); + if (!enabled || q.length < SUGGEST_MIN_QUERY || dismissed.current === q) { + setSuggestions([]); + setOpen(false); + return; + } + + /* + * Abort the superseded request on every keystroke. This is correctness, + * not economy: without it a slow response for "st" can land after the fast + * one for "st marys" and replace a correct list with a stale one. + */ + const controller = new AbortController(); + const timer = setTimeout(async () => { + const rows = await fetchSuggestions(q, controller.signal); + if (controller.signal.aborted) return; + setSuggestions(rows); + setActiveIndex(-1); + setOpen(rows.length > 0); + }, DEBOUNCE_MS); + + return () => { + clearTimeout(timer); + controller.abort(); + }; + }, [query, enabled]); + + return { + suggestions, + open, + activeIndex, + setActiveIndex, + close: () => { + dismissed.current = query.trim(); + setOpen(false); + setActiveIndex(-1); + }, + }; +} +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `cd nextjs-app && npx jest __tests__/hooks/useSchoolSuggest.test.tsx && npx tsc --noEmit` + +Expected: PASS, 5 tests, typecheck clean. + +- [ ] **Step 6: Commit** + +```bash +git add nextjs-app/lib/suggest.ts nextjs-app/hooks/useSchoolSuggest.ts nextjs-app/__tests__/hooks/useSchoolSuggest.test.tsx +git commit -m "feat(suggest): debounced, abortable suggestion hook" +``` + +--- + +### Task 5: The list, with its ARIA + +**Files:** +- Create: `nextjs-app/components/SuggestList.tsx` +- Create: `nextjs-app/components/SuggestList.module.css` +- Create: `nextjs-app/__tests__/components/SuggestList.test.tsx` + +**Interfaces:** +- Consumes: `Suggestion` from `@/lib/suggest` +- Produces: ``, and `suggestOptionId(id: string, index: number): string` + +- [ ] **Step 1: Write the failing test** + +Create `nextjs-app/__tests__/components/SuggestList.test.tsx`: + +```tsx +import { render, screen } from '@testing-library/react'; +import { SuggestList, suggestOptionId } from '@/components/SuggestList'; + +const ROWS = [ + { urn: 1, school_name: "St Mary's Primary", local_authority: 'Camden', + postcode: 'NW1 1AA', phase: 'Primary', school_type: 'Voluntary aided school' }, + { urn: 2, school_name: "St Mary's Primary", local_authority: 'Barnet', + postcode: 'EN5 2AA', phase: 'Primary', school_type: 'Community school' }, +]; + +describe('SuggestList', () => { + it('is a listbox of options', () => { + render( {}} onHover={() => {}} />); + expect(screen.getByRole('listbox')).toBeInTheDocument(); + expect(screen.getAllByRole('option')).toHaveLength(2); + }); + + it('shows the local authority, which is what tells two schools apart', () => { + // Both rows are "St Mary's Primary". Without the authority the list is + // unusable for exactly the query autosuggest exists to serve. + render( {}} onHover={() => {}} />); + expect(screen.getByText('Camden')).toBeInTheDocument(); + expect(screen.getByText('Barnet')).toBeInTheDocument(); + }); + + it('marks only the active option selected', () => { + render( {}} onHover={() => {}} />); + const options = screen.getAllByRole('option'); + expect(options[0]).toHaveAttribute('aria-selected', 'false'); + expect(options[1]).toHaveAttribute('aria-selected', 'true'); + }); + + it('gives each option the id the input will point at', () => { + // aria-activedescendant on the input has to name a real element id, or + // a screen reader announces nothing as the user arrows through. + render( {}} onHover={() => {}} />); + expect(screen.getAllByRole('option')[0]).toHaveAttribute( + 'id', suggestOptionId('s', 0)); + }); + + it('renders nothing when there is nothing to suggest', () => { + const { container } = render( {}} onHover={() => {}} />); + expect(container).toBeEmptyDOMElement(); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `cd nextjs-app && npx jest __tests__/components/SuggestList.test.tsx` + +Expected: FAIL — `Cannot find module '@/components/SuggestList'` + +- [ ] **Step 3: Write the component** + +Create `nextjs-app/components/SuggestList.tsx`: + +```tsx +'use client'; + +/** + * The autosuggest dropdown. Presentational only — it fetches nothing and owns + * no state, so the fetching rules and the ARIA rules can be read separately. + */ + +import type { Suggestion } from '@/lib/suggest'; +import styles from './SuggestList.module.css'; + +/** The id the input's aria-activedescendant points at. */ +export function suggestOptionId(id: string, index: number): string { + return `${id}-option-${index}`; +} + +interface Props { + /** Shared with the input's aria-controls. */ + id: string; + suggestions: Suggestion[]; + activeIndex: number; + onPick: (s: Suggestion) => void; + onHover: (index: number) => void; +} + +export function SuggestList({ id, suggestions, activeIndex, onPick, onHover }: Props) { + if (suggestions.length === 0) return null; + + return ( +
    + {suggestions.map((s, i) => ( +
  • { e.preventDefault(); onPick(s); }} + onMouseEnter={() => onHover(i)} + > + {s.school_name} + {/* Not decoration: there are many "St Mary's". */} + {s.local_authority} +
    • + ))} +
+ ); +} +``` + +- [ ] **Step 4: Write the stylesheet** + +Create `nextjs-app/components/SuggestList.module.css`: + +```css +/* Anchored to the search field's wrapper, which is position: relative. */ +.list { + position: absolute; + top: calc(100% + 4px); + left: 0; + right: 0; + z-index: 40; + margin: 0; + padding: 4px; + list-style: none; + max-height: 320px; + overflow-y: auto; + background: var(--color-surface, #fff); + border: 1px solid var(--color-border, #d8d8d8); + border-radius: 10px; + box-shadow: 0 10px 30px rgb(0 0 0 / 12%); +} + +.option { + display: flex; + align-items: baseline; + justify-content: space-between; + gap: 12px; + padding: 10px 12px; + border-radius: 6px; + cursor: pointer; +} + +/* Hover and keyboard share one style: the active option is the active option + however it became active. */ +.option:hover, +.active { + background: var(--color-surface-hover, #f1f1f1); +} + +.name { + font-weight: 500; +} + +.meta { + font-size: 0.85em; + color: var(--color-text-muted, #666); + white-space: nowrap; +} +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `cd nextjs-app && npx jest __tests__/components/SuggestList.test.tsx && npx tsc --noEmit` + +Expected: PASS, 5 tests, typecheck clean. + +- [ ] **Step 6: Commit** + +```bash +git add nextjs-app/components/SuggestList.tsx nextjs-app/components/SuggestList.module.css nextjs-app/__tests__/components/SuggestList.test.tsx +git commit -m "feat(suggest): the dropdown, with combobox ARIA" +``` + +--- + +### Task 6: Wire it into the search box, behind the flag + +**Files:** +- Modify: `backend/flags.py` +- Modify: `nextjs-app/app/page.tsx` +- Modify: `nextjs-app/components/HomeView.tsx` +- Modify: `nextjs-app/components/FilterBar.tsx` +- Modify: `nextjs-app/components/FilterBar.module.css` +- Create: `nextjs-app/__tests__/components/FilterBarSuggest.test.tsx` + +**Interfaces:** +- Consumes: `useSchoolSuggest`, `SuggestList`, `suggestOptionId`, `schoolUrl` from `@/lib/utils`, `getFlags` from `@/lib/flags`. + +- [ ] **Step 1: Declare the flag** + +In `backend/flags.py`, add to the `REGISTRY` tuple, after the +`admission_distance` entry: + +```python + Flag( + name="school_autosuggest", + description=( + "School name suggestions as you type in the main search box." + ), + added=date(2026, 8, 26), + ), +``` + +- [ ] **Step 2: Write the failing test** + +Create `nextjs-app/__tests__/components/FilterBarSuggest.test.tsx`: + +```tsx +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { FilterBar } from '@/components/FilterBar'; + +const push = jest.fn(); +jest.mock('next/navigation', () => ({ + useRouter: () => ({ push, replace: jest.fn(), prefetch: jest.fn() }), + usePathname: () => '/', + useSearchParams: () => new URLSearchParams(), +})); + +const FILTERS = { + local_authorities: [], school_types: [], years: [], phases: [], + genders: [], admissions_policies: [], +}; + +const realFetch = global.fetch; +beforeEach(() => { + global.fetch = jest.fn(async () => ({ + ok: true, + json: async () => ({ suggestions: [{ + urn: 100010, school_name: 'Brecknock Primary School', + local_authority: 'Camden', postcode: 'NW1 1AA', + phase: 'Primary', school_type: 'Community school' }] }), + })) as unknown as typeof fetch; + push.mockClear(); +}); +afterEach(() => { global.fetch = realFetch; }); + +describe('FilterBar autosuggest', () => { + it('is a combobox only when the flag is on', () => { + const { rerender } = render(); + expect(screen.queryByRole('combobox')).not.toBeInTheDocument(); + rerender(); + expect(screen.getByRole('combobox')).toBeInTheDocument(); + }); + + it('makes no request while the flag is off', async () => { + // Off means off: no listener, no fetch, no markup. + render(); + await userEvent.type(screen.getByPlaceholderText(/School name or postcode/i), + 'brecknock'); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('shows suggestions and navigates when one is chosen', async () => { + render(); + await userEvent.type(screen.getByRole('combobox'), 'brecknock'); + const option = await screen.findByRole('option', { name: /Brecknock/ }); + await userEvent.click(option); + expect(push).toHaveBeenCalledWith( + expect.stringContaining('/school/100010')); + }); + + it('suppresses suggestions once the value is a postcode', async () => { + // The box takes a name OR a postcode; suggestions must get out of the way. + // + // fireEvent.change, not userEvent.type: typing sets "N", "NW", "NW1"... and + // "NW1" is not a postcode, so a request for it is correct behaviour. Only + // the settled value is the assertion, so set it in one go. + render(); + fireEvent.change(screen.getByRole('combobox'), { target: { value: 'NW1 1AA' } }); + await new Promise((r) => setTimeout(r, 300)); // past the 200ms debounce + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('Enter with no active option still submits the free-text search', async () => { + // The existing behaviour is preserved, not replaced. + render(); + const input = screen.getByRole('combobox'); + await userEvent.type(input, 'brecknock{Enter}'); + // updateURL pushes inside startTransition, so the call is not synchronous. + await waitFor(() => expect(push).toHaveBeenCalledWith( + expect.stringContaining('search=brecknock'))); + }); +}); +``` + +- [ ] **Step 3: Run it to verify it fails** + +Run: `cd nextjs-app && npx jest __tests__/components/FilterBarSuggest.test.tsx` + +Expected: FAIL — `autosuggest` is not a prop, so no combobox is rendered. + +- [ ] **Step 4: Wire the hook into FilterBar** + +In `nextjs-app/components/FilterBar.tsx`: + +Add to the imports: + +```typescript +import { useSchoolSuggest } from "@/hooks/useSchoolSuggest"; +import { SuggestList, suggestOptionId } from "./SuggestList"; +import { schoolUrl } from "@/lib/utils"; +import type { Suggestion } from "@/lib/suggest"; +``` + +Add to `FilterBarProps`: + +```typescript + /** Server-read feature flag. Off means no listener, no fetch, no markup. */ + autosuggest?: boolean; +``` + +Add `autosuggest = false,` to the destructured parameters of `FilterBar`. + +Inside the component, after the `omniValue` state declaration: + +```typescript + const suggestId = `school-suggest-${isHero ? "hero" : "bar"}`; + // Suppressed once the value parses as a postcode: the box takes a school + // name OR a postcode, and suggesting schools during postcode entry fights + // the user rather than helping them. + const suggestEnabled = autosuggest && !isValidPostcode(omniValue); + const { suggestions, open, activeIndex, setActiveIndex, close } = + useSchoolSuggest(omniValue, suggestEnabled); + + const pickSuggestion = (s: Suggestion) => { + close(); + track('search_submitted', { + query: s.school_name.toLowerCase(), + via: 'suggestion', + urn: s.urn, + has_postcode: false, + filters_active: '', + filters_count: 0, + }); + router.push(schoolUrl(s.urn, s.school_name)); + }; + + const handleOmniKeyDown = (e: React.KeyboardEvent) => { + if (!open) return; + if (e.key === "ArrowDown") { + e.preventDefault(); + setActiveIndex(activeIndex + 1 >= suggestions.length ? 0 : activeIndex + 1); + } else if (e.key === "ArrowUp") { + e.preventDefault(); + setActiveIndex(activeIndex <= 0 ? suggestions.length - 1 : activeIndex - 1); + } else if (e.key === "Escape") { + close(); + } else if (e.key === "Enter" && activeIndex >= 0) { + // Only when an option is active. With none, the event falls through to + // the form's submit handler and searches the typed text, as it does now. + e.preventDefault(); + pickSuggestion(suggestions[activeIndex]); + } + }; +``` + +- [ ] **Step 5: Wire the markup** + +In `nextjs-app/components/FilterBar.tsx`, replace the existing omni `` +element with: + +```tsx + setOmniValue(e.target.value)} + onKeyDown={handleOmniKeyDown} + onBlur={close} + placeholder="School name or postcode" + className={styles.omniInput} + {...(autosuggest ? { + role: "combobox", + "aria-expanded": open, + "aria-controls": suggestId, + "aria-autocomplete": "list" as const, + "aria-activedescendant": + activeIndex >= 0 ? suggestOptionId(suggestId, activeIndex) : undefined, + autoComplete: "off", + } : {})} + /> +``` + +And directly after the closing `` of the search submit button, still +inside the wrapper element that holds the input: + +```tsx + {autosuggest && open && ( + + )} +``` + +- [ ] **Step 6: Anchor the dropdown** + +The list is `position: absolute`, so its nearest positioned ancestor must be +the wrapper around the input and the submit button. That is +`.omniBoxContainer`, and it **does not currently declare `position: relative`** +— without this the dropdown anchors to the page and lands in the wrong place. + +In `nextjs-app/components/FilterBar.module.css`, change: + +```css +.omniBoxContainer { + display: flex; + align-items: center; + gap: 0.5rem; +} +``` + +to: + +```css +.omniBoxContainer { + display: flex; + align-items: center; + gap: 0.5rem; + /* The suggestion dropdown is absolutely positioned against this box. */ + position: relative; +} +``` + +- [ ] **Step 7: Thread the flag from the server** + +In `nextjs-app/app/page.tsx`, add the import: + +```typescript +import { getFlags } from '@/lib/flags'; +``` + +Inside `HomePage`, before the `try`: + +```typescript + // Server-read: no flag value reaches the browser bundle. + const flags = await getFlags(); + const autosuggest = flags.school_autosuggest === true; +``` + +Add `autosuggest={autosuggest}` to **both** `` render sites — +the success path and the `catch` fallback. Missing the second means the flag +silently does nothing whenever the API call fails. + +In `nextjs-app/components/HomeView.tsx`, add `autosuggest?: boolean` to its +props interface, destructure it with a default of `false`, and pass +`autosuggest={autosuggest}` to **both** `` instances — hero +and sticky. + +- [ ] **Step 8: Run the tests to verify they pass** + +Run: `cd nextjs-app && npx jest && npx tsc --noEmit && npm run build` + +Expected: all pass, typecheck clean, build green. + +- [ ] **Step 9: Run the backend suite** + +The flag registry gained an entry, so the registry tests must still pass. + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` + +Expected: all pass. + +- [ ] **Step 10: Commit** + +```bash +git add backend/flags.py nextjs-app/app/page.tsx nextjs-app/components/HomeView.tsx nextjs-app/components/FilterBar.tsx nextjs-app/components/FilterBar.module.css nextjs-app/__tests__/components/FilterBarSuggest.test.tsx +git commit -m "feat(suggest): wire autosuggest into the search box behind a flag" +``` + +--- + +### Task 7: E2E journeys + +**Files:** +- Modify: `e2e/tests/journeys.spec.ts` + +- [ ] **Step 1: Write the journeys** + +`/api/flags` is denied to the public, so feature state is read from its +observable effect — the same approach the distance journeys use. + +Append to `e2e/tests/journeys.spec.ts`: + +```typescript +/* + * School autosuggest (spec 2026-08-26). + */ +async function autosuggestIsOn(page: Page): Promise { + await page.goto('/'); + return (await page.getByRole('combobox').count()) > 0; +} + +test('the suggest endpoint answers from Typesense', async ({ page }) => { + // Not flagged — the endpoint is live even while the UI is dark, so it can + // be smoke-tested before the feature is switched on. + const res = await page.request.get('/api/suggest?q=brecknock'); + expect(res.ok()).toBeTruthy(); + const { suggestions } = await res.json(); + expect(Array.isArray(suggestions)).toBeTruthy(); + if (suggestions.length) { + // Local authority is what tells two "St Mary's" apart. + expect(suggestions[0]).toHaveProperty('school_name'); + expect(suggestions[0]).toHaveProperty('local_authority'); + } +}); + +test('a one-character query is answered, not rejected', async ({ page }) => { + // The keystroke path never errors on ordinary input. + const res = await page.request.get('/api/suggest?q=b'); + expect(res.status()).toBe(200); + expect((await res.json()).suggestions).toEqual([]); +}); + +test('the suggest response is cacheable', async ({ page }) => { + const res = await page.request.get('/api/suggest?q=brecknock'); + expect(res.headers()['cache-control'] ?? '').toContain('s-maxage'); +}); + +test('typing a school name suggests it, and choosing it opens that school', async ({ page }) => { + test.skip(!(await autosuggestIsOn(page)), + 'the school_autosuggest flag is off in this environment'); + + // A school certain to exist in any environment with data. + const { schools } = await (await page.request.get('/api/schools?page_size=1')).json(); + test.skip(!schools?.length, 'no schools in this environment'); + const name = schools[0].school_name as string; + + await page.goto('/'); + await page.getByRole('combobox').first().fill(name.slice(0, 12)); + const option = page.getByRole('option').first(); + await expect(option).toBeVisible(); + await option.click(); + await expect(page).toHaveURL(/\/school\/\d+/); +}); + +test('with autosuggest off, the search box is a plain input', async ({ page }) => { + test.skip(await autosuggestIsOn(page), + 'the school_autosuggest flag is on in this environment'); + + await page.goto('/'); + await expect(page.getByRole('combobox')).toHaveCount(0); + // And the box still works: the existing search must be untouched. + await page.getByPlaceholder(/School name or postcode/i).first().fill('abbey'); + await page.getByRole('button', { name: /Search/i }).first().click(); + await expect(page).toHaveURL(/search=abbey/); +}); +``` + +- [ ] **Step 2: Verify the spec compiles and the tests are collected** + +Run: `cd e2e && npx playwright test --list` + +Expected: the five new titles appear; total rises by 5. + +- [ ] **Step 3: Commit** + +```bash +git add e2e/tests/journeys.spec.ts +git commit -m "test(e2e): autosuggest journeys, gated on the flag" +``` + +--- + +### Task 8: Verify and open the PR + +- [ ] **Step 1: Run everything** + +```bash +uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q +cd nextjs-app && npx jest && npx tsc --noEmit && npm run build +cd ../e2e && npx playwright test --list +``` + +Expected: backend green, frontend green, typecheck clean, build green, e2e collects. + +- [ ] **Step 2: Open the PR against `main`** + +The body must state: the rate-limit keying change applies unflagged and +site-wide; autosuggest itself is off until toggled in Unleash; and that +`/api/suggest` is live regardless, deliberately. + +## Notes for the executor + +**Task 1 changes rate limiting for every endpoint.** It is the one change here +that is not behind a flag, and it is the one worth the most review attention. +Before it, everyone shares one 60/minute bucket; after it, each caller gets +their own. That is the intended fix, and it also removes an accidental global +throttle — the spec's §1 and Risks say so plainly. A global ceiling belongs at +Cloudflare and is deliberately not built here. + +**Do not add an in-app global rate limit.** An earlier spec draft did. slowapi's +`default_limits` and `application_limits` are both keyed by `key_func`, so they +are per-client rather than global, and `application_limits` only apply with +`SlowAPIMiddleware` installed, which this app does not use. + +**`onMouseDown`, not `onClick`, on the options.** The input's `onBlur` closes +the list and blur fires first, so a click handler never runs. This is the +classic bug where the dropdown works by keyboard and is dead to the mouse. + +**Both render sites, twice over.** `page.tsx` renders `HomeView` in the success +path *and* the catch fallback; `HomeView` renders `FilterBar` as hero *and* +sticky. Missing any of the four makes the flag silently do nothing somewhere. From ff041544f2c35922677547dedfc55a03c7d4817f Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:32:58 +0100 Subject: [PATCH 04/11] fix(api): rate-limit per caller, not per proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The limiter keyed on request.client.host, which in staging and prod is the Next container — the backend has no published ports and nothing else can reach it. So every browser user on the site shared one 60/minute bucket per route. Measured against staging: 70 concurrent requests to /api/schools returned exactly 60 OK and 10 refused, from one machine. CF-Connecting-IP first. Cloudflare fronts both environments and overwrites any client-supplied value, which a parsed X-Forwarded-For chain does not guarantee. The XFF fallback is forgeable only from inside the Docker network. Named rather than hidden: the shared bucket was an accidental global throttle on a single-process backend, and correct per-user keying removes it. A real global ceiling belongs at Cloudflare, which is already in the path; slowapi cannot express one without a second Limiter and middleware this app does not install. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 32 ++++++++++++++- backend/tests/test_rate_limit_key.py | 58 ++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 backend/tests/test_rate_limit_key.py diff --git a/backend/app.py b/backend/app.py index d352de5..ab5a3e7 100644 --- a/backend/app.py +++ b/backend/app.py @@ -296,8 +296,36 @@ def clean_filter_values(series: pd.Series) -> list[str]: # SECURITY MIDDLEWARE & HELPERS # ============================================================================= -# Rate limiter -limiter = Limiter(key_func=get_remote_address) +def client_key(request: Request) -> str: + """The rate-limit bucket: the real caller, not the proxy in front of them. + + `get_remote_address` reads request.client.host. In staging and production + the backend has no published ports and sits on the internal network, so its + only caller is the Next proxy — meaning every browser user on the site + shared one bucket. Measured before this fix: 70 concurrent requests to + /api/schools returned 60 OK and 10 refused. + + CF-Connecting-IP first, because Cloudflare (in front of both environments) + sets it on every origin request and *overwrites* any client-supplied value, + which a parsed X-Forwarded-For chain does not guarantee. The XFF fallback is + forgeable, but only by a caller already inside the Docker network, which is + the one place nothing untrusted can reach. + """ + cf = request.headers.get("cf-connecting-ip") + if cf: + return cf.strip() + xff = request.headers.get("x-forwarded-for") + if xff: + return xff.split(",")[0].strip() + return get_remote_address(request) + + +# Rate limiter. No in-app global ceiling: slowapi's default_limits and +# application_limits are both keyed by key_func (so per-client, not global) +# and the latter only applies with SlowAPIMiddleware installed, which this app +# does not use. A global cap belongs at Cloudflare, which is already in the +# path. See the spec's §1 for why that is deliberate. +limiter = Limiter(key_func=client_key) class SecurityHeadersMiddleware(BaseHTTPMiddleware): diff --git a/backend/tests/test_rate_limit_key.py b/backend/tests/test_rate_limit_key.py new file mode 100644 index 0000000..7435ef3 --- /dev/null +++ b/backend/tests/test_rate_limit_key.py @@ -0,0 +1,58 @@ +"""The rate-limit bucket must be the caller, not the proxy in front of them. + +`get_remote_address` reads request.client.host. In staging and production the +backend has no published ports and its only caller is the Next proxy, so that +host is the Next container — one bucket for every browser user on the site. +Measured before this fix: 70 concurrent requests, 60 served and 10 refused. +""" + +from starlette.datastructures import Headers + +from backend.app import client_key + + +class _Req: + """Enough of a Request for the key function: headers and a client host.""" + + def __init__(self, headers: dict, host: str = "10.0.0.9"): + self.headers = Headers(headers) + self.client = type("C", (), {"host": host})() + self.scope = {"type": "http", "client": (host, 0), + "headers": [(k.lower().encode(), v.encode()) + for k, v in headers.items()]} + + +def test_cloudflare_header_wins(): + # Cloudflare sets CF-Connecting-IP and overwrites any client-supplied + # value, so it is trustworthy in a way a parsed XFF chain is not. + assert client_key(_Req({"cf-connecting-ip": "203.0.113.7"})) == "203.0.113.7" + + +def test_forwarded_for_is_the_fallback_and_takes_the_first_entry(): + # Left-most is the original client; everything after it is proxies. + assert client_key( + _Req({"x-forwarded-for": "203.0.113.7, 10.0.0.2"})) == "203.0.113.7" + + +def test_remote_address_is_the_last_resort(): + assert client_key(_Req({}, host="10.0.0.9")) == "10.0.0.9" + + +def test_cloudflare_header_beats_forwarded_for(): + key = client_key(_Req({"cf-connecting-ip": "203.0.113.7", + "x-forwarded-for": "198.51.100.1"})) + assert key == "203.0.113.7" + + +def test_two_callers_get_two_buckets(): + # The whole point: one user exhausting their limit must not refuse another. + a = client_key(_Req({"cf-connecting-ip": "203.0.113.7"})) + b = client_key(_Req({"cf-connecting-ip": "203.0.113.8"})) + assert a != b + + +def test_whitespace_is_stripped(): + # "a, b" split on comma leaves a leading space on every entry but the + # first; an unstripped key silently creates a second bucket per client. + assert client_key(_Req({"x-forwarded-for": " 203.0.113.7 ,10.0.0.2"})) \ + == "203.0.113.7" From 75d3534d825ba3b77324bf74e162818c9d084efc Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:33:42 +0100 Subject: [PATCH 05/11] feat(suggest): Typesense rows for autosuggest, no DataFrame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit search_schools_typesense returns URNs, which forces the caller to hydrate from the 25,000-row in-memory frame. Every field a suggestion needs is already in the Typesense document, so this returns documents and the caller needs no pandas at all — the difference between a query that can run per keystroke and one that cannot. Never raises. Typesense unreachable or erroring gives an empty list, because a dropdown that quietly stops appearing is the right failure for a keystroke path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/data_loader.py | 40 ++++++++++++++++++++ backend/tests/test_suggest.py | 71 +++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 backend/tests/test_suggest.py diff --git a/backend/data_loader.py b/backend/data_loader.py index 62aa4a5..bf0e2a5 100644 --- a/backend/data_loader.py +++ b/backend/data_loader.py @@ -100,6 +100,46 @@ def search_schools_typesense(query: str, limit: int = 250) -> List[int]: return [] +# The most a public endpoint will return in one response. +SUGGEST_MAX_LIMIT = 20 + +# Fields a suggestion row carries, and the default when the document omits an +# optional one. phase and school_type are optional in the Typesense schema. +_SUGGEST_FIELDS = ("school_name", "local_authority", "postcode", + "phase", "school_type") + + +def suggest_schools_typesense(query: str, limit: int = 8) -> List[dict]: + """Autosuggest rows straight from Typesense. Never raises. + + Returns documents rather than URNs, unlike search_schools_typesense, so the + caller needs no DataFrame. Every field below is already in the index — see + pipeline/scripts/sync_typesense.py — which is what makes this cheap enough + to run per keystroke. + """ + client = _get_typesense_client() + if client is None: + return [] + try: + result = client.collections["schools"].documents.search({ + "q": query, + "query_by": "school_name,local_authority", + "per_page": max(1, min(limit, SUGGEST_MAX_LIMIT)), + "typo_tokens_threshold": 1, + }) + except Exception: + # A dropdown that quietly stops appearing is the right failure here. + return [] + + rows = [] + for hit in result.get("hits", []): + doc = hit.get("document", {}) + row = {"urn": int(doc.get("urn", 0))} + row.update({f: str(doc.get(f, "") or "") for f in _SUGGEST_FIELDS}) + rows.append(row) + return rows + + def normalize_school_type(school_type: Optional[str]) -> Optional[str]: """Convert cryptic school type codes to user-friendly names.""" if not school_type: diff --git a/backend/tests/test_suggest.py b/backend/tests/test_suggest.py new file mode 100644 index 0000000..b380f8d --- /dev/null +++ b/backend/tests/test_suggest.py @@ -0,0 +1,71 @@ +"""Tests for school autosuggest (spec 2026-08-26).""" + +from backend import data_loader + + +class _FakeDocs: + def __init__(self, hits, explode=False): + self._hits = hits + self._explode = explode + self.last_params = None + + def search(self, params): + self.last_params = params + if self._explode: + raise RuntimeError("typesense is down") + return {"hits": [{"document": d} for d in self._hits]} + + +class _FakeClient: + def __init__(self, hits, explode=False): + self.docs = _FakeDocs(hits, explode) + self.collections = {"schools": type("C", (), {"documents": self.docs})()} + + +_HIT = { + "urn": 100010, "school_name": "Brecknock Primary School", + "local_authority": "Camden", "postcode": "NW1 1AA", + "phase": "Primary", "school_type": "Community school", +} + + +def _use(monkeypatch, client): + monkeypatch.setattr(data_loader, "_get_typesense_client", lambda: client) + + +def test_returns_the_fields_a_suggestion_needs(monkeypatch): + # Local authority is not decoration: there are many schools called + # "St Mary's", and a list without it cannot be chosen between. + _use(monkeypatch, _FakeClient([_HIT])) + out = data_loader.suggest_schools_typesense("breck") + assert out == [{ + "urn": 100010, "school_name": "Brecknock Primary School", + "local_authority": "Camden", "postcode": "NW1 1AA", + "phase": "Primary", "school_type": "Community school", + }] + + +def test_a_missing_optional_field_becomes_an_empty_string(monkeypatch): + # phase and school_type are optional in the Typesense schema. A missing + # key must not KeyError in the keystroke path. + _use(monkeypatch, _FakeClient([{"urn": 1, "school_name": "X", + "local_authority": "Y", "postcode": "Z"}])) + out = data_loader.suggest_schools_typesense("x") + assert out[0]["phase"] == "" and out[0]["school_type"] == "" + + +def test_typesense_unavailable_gives_no_suggestions_rather_than_raising(monkeypatch): + _use(monkeypatch, None) + assert data_loader.suggest_schools_typesense("anything") == [] + + +def test_a_typesense_error_gives_no_suggestions_rather_than_raising(monkeypatch): + _use(monkeypatch, _FakeClient([], explode=True)) + assert data_loader.suggest_schools_typesense("anything") == [] + + +def test_the_limit_is_passed_through_and_clamped(monkeypatch): + client = _FakeClient([]) + _use(monkeypatch, client) + data_loader.suggest_schools_typesense("x", limit=500) + assert client.docs.last_params["per_page"] == 20 From 1a6d349dad219191cab7f074b8aecd383f07c89a Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:34:34 +0100 Subject: [PATCH 06/11] feat(suggest): GET /api/suggest, cacheable and DataFrame-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dedicated endpoint rather than a mode of /api/schools, because that path filters and sorts 25,000 pandas rows per query while holding the GIL — affordable once per search, not once per keystroke. A test asserts the distinction directly by making load_school_data raise and requiring the endpoint to answer anyway. Nothing errors on ordinary input: a short query, no matches, or Typesense being down are all 200 with an empty list. Cached deliberately. Prefix queries repeat enormously across users and school names change once a year, so s-maxage plus the existing ETag middleware turns most keystrokes into 304s. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 34 +++++++++++++++++++++ backend/tests/test_suggest.py | 57 +++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/backend/app.py b/backend/app.py index ab5a3e7..832a039 100644 --- a/backend/app.py +++ b/backend/app.py @@ -33,6 +33,7 @@ from .data_loader import ( get_supplementary_data, get_supplementary_data_batch, search_schools_typesense, + suggest_schools_typesense, ) from .data_loader import get_data_info as get_db_info from . import flags @@ -383,6 +384,7 @@ CACHE_RULES: list[tuple[str, tuple[int, int, int]]] = [ ("/api/schools/", (300, 3600, 86400)), # /api/schools/{urn} ("/api/rankings", (60, 600, 3600)), ("/api/compare", (60, 600, 3600)), + ("/api/suggest", (60, 3600, 86400)), # autosuggest ("/api/schools", (30, 300, 1800)), # search list ] @@ -1299,6 +1301,38 @@ async def get_place(request: Request, kind: str, slug: str, } +# Two characters. One is not a query — it matches thousands of schools and the +# response is useless, so it is not worth a round trip. +SUGGEST_MIN_QUERY = 2 + + +@app.get("/api/suggest") +@limiter.limit("120/minute") +async def suggest_schools( + request: Request, + q: str = Query("", max_length=100), + limit: int = Query(8, ge=1, le=20), +): + """School name suggestions, from Typesense alone. + + Deliberately not a mode of /api/schools: that path filters and sorts the + full in-memory DataFrame, which is far too expensive to run per keystroke. + + Nothing here returns an error for ordinary input. A short query, no + matches, or Typesense being unreachable are all 200 with an empty list — + a dropdown that quietly does not appear is the right failure for a + keystroke path, and there is no DataFrame fallback because the 25,000-row + substring scan is precisely what this endpoint exists to avoid. + + 120/minute rather than the default 60: a 200 ms debounce makes typing + legitimately bursty. + """ + query = q.strip() + if len(query) < SUGGEST_MIN_QUERY: + return {"suggestions": []} + return {"suggestions": suggest_schools_typesense(query, limit)} + + @app.get("/api/flags") @limiter.limit(f"{settings.rate_limit_per_minute}/minute") async def get_feature_flags(request: Request): diff --git a/backend/tests/test_suggest.py b/backend/tests/test_suggest.py index b380f8d..b2bcc53 100644 --- a/backend/tests/test_suggest.py +++ b/backend/tests/test_suggest.py @@ -69,3 +69,60 @@ def test_the_limit_is_passed_through_and_clamped(monkeypatch): _use(monkeypatch, client) data_loader.suggest_schools_typesense("x", limit=500) assert client.docs.last_params["per_page"] == 20 + + +def _client(monkeypatch, rows, *, blow_up_dataframe=False): + from fastapi.testclient import TestClient + from backend import app as app_module + + monkeypatch.setattr(app_module, "suggest_schools_typesense", + lambda q, limit=8: rows) + if blow_up_dataframe: + def _boom(): + raise AssertionError("the suggest path must not load the DataFrame") + monkeypatch.setattr(app_module, "load_school_data", _boom) + monkeypatch.setattr(app_module, "load_latest_school_data", _boom) + return TestClient(app_module.app, raise_server_exceptions=False) + + +def test_the_endpoint_returns_suggestions(monkeypatch): + body = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck").json() + assert body["suggestions"][0]["school_name"] == "Brecknock Primary School" + + +def test_the_endpoint_never_touches_the_dataframe(monkeypatch): + """The whole reason this is not a mode of /api/schools. + + That endpoint filters and sorts 25,000 rows of pandas per query, holding + the GIL. Per keystroke, that is the cost this endpoint exists to avoid. + """ + res = _client(monkeypatch, [_HIT], blow_up_dataframe=True).get("/api/suggest?q=breck") + assert res.status_code == 200 + assert res.json()["suggestions"] + + +def test_a_one_character_query_returns_nothing_and_does_not_error(monkeypatch): + # The keystroke path never errors on ordinary input. + res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=b") + assert res.status_code == 200 + assert res.json() == {"suggestions": []} + + +def test_a_blank_query_returns_nothing_and_does_not_error(monkeypatch): + res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=") + assert res.status_code == 200 + assert res.json() == {"suggestions": []} + + +def test_typesense_down_is_an_empty_list_not_a_500(monkeypatch): + res = _client(monkeypatch, []).get("/api/suggest?q=breck") + assert res.status_code == 200 + assert res.json() == {"suggestions": []} + + +def test_the_response_is_cacheable(monkeypatch): + # Prefix queries repeat enormously across users, and school names change + # once a year. Without this the endpoint pays full price every keystroke. + res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck") + assert "s-maxage" in res.headers.get("cache-control", "") + assert res.headers.get("etag") From 06eb433db5e76f5465e2a112562987eae3af7644 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:35:27 +0100 Subject: [PATCH 07/11] feat(suggest): debounced, abortable suggestion hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AbortController is correctness, not economy. Without it a slow response for 'st' can land after the fast one for 'st marys' and replace a correct list with a stale one — the classic autosuggest race. No cache: 'no-store', unlike the compare modal's search. This is the one endpoint where prefix queries repeat most across users, so discarding the browser cache and the backend's ETag 304s would be throwing away the cheapest win available. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../__tests__/hooks/useSchoolSuggest.test.tsx | 73 +++++++++++++++++++ nextjs-app/hooks/useSchoolSuggest.ts | 58 +++++++++++++++ nextjs-app/lib/suggest.ts | 39 ++++++++++ 3 files changed, 170 insertions(+) create mode 100644 nextjs-app/__tests__/hooks/useSchoolSuggest.test.tsx create mode 100644 nextjs-app/hooks/useSchoolSuggest.ts create mode 100644 nextjs-app/lib/suggest.ts diff --git a/nextjs-app/__tests__/hooks/useSchoolSuggest.test.tsx b/nextjs-app/__tests__/hooks/useSchoolSuggest.test.tsx new file mode 100644 index 0000000..4681b86 --- /dev/null +++ b/nextjs-app/__tests__/hooks/useSchoolSuggest.test.tsx @@ -0,0 +1,73 @@ +import { renderHook, act, waitFor } from '@testing-library/react'; +import { useSchoolSuggest } from '@/hooks/useSchoolSuggest'; + +const realFetch = global.fetch; + +function mockFetch(rows: unknown[], delayMs = 0) { + global.fetch = jest.fn(async (_url: unknown, init?: { signal?: AbortSignal }) => { + if (delayMs) { + await new Promise((resolve, reject) => { + const t = setTimeout(resolve, delayMs); + init?.signal?.addEventListener('abort', () => { + clearTimeout(t); + reject(Object.assign(new Error('aborted'), { name: 'AbortError' })); + }); + }); + } + return { ok: true, json: async () => ({ suggestions: rows }) }; + }) as unknown as typeof fetch; +} + +const ROW = { + urn: 1, school_name: 'Brecknock Primary School', local_authority: 'Camden', + postcode: 'NW1 1AA', phase: 'Primary', school_type: 'Community school', +}; + +describe('useSchoolSuggest', () => { + beforeEach(() => { jest.useFakeTimers(); }); + afterEach(() => { jest.useRealTimers(); global.fetch = realFetch; }); + + it('does not fetch below the minimum query length', () => { + mockFetch([ROW]); + renderHook(() => useSchoolSuggest('b', true)); + act(() => { jest.advanceTimersByTime(500); }); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('does not fetch at all when disabled', () => { + // The flag being off must mean no request, not a hidden dropdown. + mockFetch([ROW]); + renderHook(() => useSchoolSuggest('brecknock', false)); + act(() => { jest.advanceTimersByTime(500); }); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('debounces rather than firing per keystroke', () => { + mockFetch([ROW]); + const { rerender } = renderHook( + ({ q }) => useSchoolSuggest(q, true), { initialProps: { q: 'br' } }); + rerender({ q: 'bre' }); + rerender({ q: 'brec' }); + act(() => { jest.advanceTimersByTime(199); }); + expect(global.fetch).not.toHaveBeenCalled(); + act(() => { jest.advanceTimersByTime(2); }); + expect(global.fetch).toHaveBeenCalledTimes(1); + }); + + it('opens with results once they arrive', async () => { + mockFetch([ROW]); + const { result } = renderHook(() => useSchoolSuggest('brecknock', true)); + act(() => { jest.advanceTimersByTime(200); }); + await waitFor(() => expect(result.current.suggestions).toHaveLength(1)); + expect(result.current.open).toBe(true); + }); + + it('close() hides the list without clearing the query', async () => { + mockFetch([ROW]); + const { result } = renderHook(() => useSchoolSuggest('brecknock', true)); + act(() => { jest.advanceTimersByTime(200); }); + await waitFor(() => expect(result.current.open).toBe(true)); + act(() => { result.current.close(); }); + expect(result.current.open).toBe(false); + }); +}); diff --git a/nextjs-app/hooks/useSchoolSuggest.ts b/nextjs-app/hooks/useSchoolSuggest.ts new file mode 100644 index 0000000..53ba171 --- /dev/null +++ b/nextjs-app/hooks/useSchoolSuggest.ts @@ -0,0 +1,58 @@ +'use client'; + +import { useEffect, useRef, useState } from 'react'; +import { fetchSuggestions, SUGGEST_MIN_QUERY, type Suggestion } from '@/lib/suggest'; + +/* + * Long enough that a fast typist does not fire a request per character, short + * enough that the list feels attached to the keyboard. + */ +const DEBOUNCE_MS = 200; + +export function useSchoolSuggest(query: string, enabled: boolean) { + const [suggestions, setSuggestions] = useState([]); + const [open, setOpen] = useState(false); + const [activeIndex, setActiveIndex] = useState(-1); + // Set when the user dismisses the list, so a re-render does not reopen it. + const dismissed = useRef(''); + + useEffect(() => { + const q = query.trim(); + if (!enabled || q.length < SUGGEST_MIN_QUERY || dismissed.current === q) { + setSuggestions([]); + setOpen(false); + return; + } + + /* + * Abort the superseded request on every keystroke. This is correctness, + * not economy: without it a slow response for "st" can land after the fast + * one for "st marys" and replace a correct list with a stale one. + */ + const controller = new AbortController(); + const timer = setTimeout(async () => { + const rows = await fetchSuggestions(q, controller.signal); + if (controller.signal.aborted) return; + setSuggestions(rows); + setActiveIndex(-1); + setOpen(rows.length > 0); + }, DEBOUNCE_MS); + + return () => { + clearTimeout(timer); + controller.abort(); + }; + }, [query, enabled]); + + return { + suggestions, + open, + activeIndex, + setActiveIndex, + close: () => { + dismissed.current = query.trim(); + setOpen(false); + setActiveIndex(-1); + }, + }; +} diff --git a/nextjs-app/lib/suggest.ts b/nextjs-app/lib/suggest.ts new file mode 100644 index 0000000..5a17d9d --- /dev/null +++ b/nextjs-app/lib/suggest.ts @@ -0,0 +1,39 @@ +/** + * Client for /api/suggest. + * + * No `cache: "no-store"`. The compare modal's search uses it, and copying that + * here would discard both the browser cache and the ETag 304s the backend's + * CacheAndETagMiddleware already provides — on the one endpoint where prefix + * queries repeat most. + */ + +export interface Suggestion { + urn: number; + school_name: string; + local_authority: string; + postcode: string; + phase: string; + school_type: string; +} + +/** Below this the response is thousands of schools and worth no round trip. */ +export const SUGGEST_MIN_QUERY = 2; + +const API = process.env.NEXT_PUBLIC_API_URL || '/api'; + +/** Suggestions for `q`. Never throws: no suggestions is a fine outcome. */ +export async function fetchSuggestions( + q: string, signal?: AbortSignal, +): Promise { + if (q.trim().length < SUGGEST_MIN_QUERY) return []; + try { + const res = await fetch(`${API}/suggest?q=${encodeURIComponent(q.trim())}`, + { signal }); + if (!res.ok) return []; + const body = await res.json(); + return body.suggestions ?? []; + } catch { + // Includes AbortError, which is the normal path on every keystroke. + return []; + } +} From d88e77f459b61ee29b67895d970f40b8ed465a90 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:36:39 +0100 Subject: [PATCH 08/11] feat(suggest): the dropdown, with combobox ARIA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Presentational only — it fetches nothing and owns no state, so the fetching rules and the ARIA rules can be read separately. onMouseDown, not onClick. The input's blur handler closes the list and blur fires before click, so a click handler never runs: the classic bug where a dropdown works perfectly by keyboard and is dead to the mouse. The plan's CSS guessed at token names like --color-surface. The real tokens are --bg-card, --border, --text-muted, --bg-secondary and --shadow-soft, and all five are redefined in the dark theme — invented names would have silently fallen back to hardcoded light values and broken dark mode. Local authority is rendered because there are many schools called 'St Mary's'; a list without it is unusable for exactly the query autosuggest exists to serve, which is what the test asserts. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../__tests__/components/SuggestList.test.tsx | 50 +++++++++++++++++ nextjs-app/components/SuggestList.module.css | 53 +++++++++++++++++++ nextjs-app/components/SuggestList.tsx | 53 +++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100644 nextjs-app/__tests__/components/SuggestList.test.tsx create mode 100644 nextjs-app/components/SuggestList.module.css create mode 100644 nextjs-app/components/SuggestList.tsx diff --git a/nextjs-app/__tests__/components/SuggestList.test.tsx b/nextjs-app/__tests__/components/SuggestList.test.tsx new file mode 100644 index 0000000..fbc68e2 --- /dev/null +++ b/nextjs-app/__tests__/components/SuggestList.test.tsx @@ -0,0 +1,50 @@ +import { render, screen } from '@testing-library/react'; +import { SuggestList, suggestOptionId } from '@/components/SuggestList'; + +const ROWS = [ + { urn: 1, school_name: "St Mary's Primary", local_authority: 'Camden', + postcode: 'NW1 1AA', phase: 'Primary', school_type: 'Voluntary aided school' }, + { urn: 2, school_name: "St Mary's Primary", local_authority: 'Barnet', + postcode: 'EN5 2AA', phase: 'Primary', school_type: 'Community school' }, +]; + +describe('SuggestList', () => { + it('is a listbox of options', () => { + render( {}} onHover={() => {}} />); + expect(screen.getByRole('listbox')).toBeInTheDocument(); + expect(screen.getAllByRole('option')).toHaveLength(2); + }); + + it('shows the local authority, which is what tells two schools apart', () => { + // Both rows are "St Mary's Primary". Without the authority the list is + // unusable for exactly the query autosuggest exists to serve. + render( {}} onHover={() => {}} />); + expect(screen.getByText('Camden')).toBeInTheDocument(); + expect(screen.getByText('Barnet')).toBeInTheDocument(); + }); + + it('marks only the active option selected', () => { + render( {}} onHover={() => {}} />); + const options = screen.getAllByRole('option'); + expect(options[0]).toHaveAttribute('aria-selected', 'false'); + expect(options[1]).toHaveAttribute('aria-selected', 'true'); + }); + + it('gives each option the id the input will point at', () => { + // aria-activedescendant on the input has to name a real element id, or + // a screen reader announces nothing as the user arrows through. + render( {}} onHover={() => {}} />); + expect(screen.getAllByRole('option')[0]).toHaveAttribute( + 'id', suggestOptionId('s', 0)); + }); + + it('renders nothing when there is nothing to suggest', () => { + const { container } = render( {}} onHover={() => {}} />); + expect(container).toBeEmptyDOMElement(); + }); +}); diff --git a/nextjs-app/components/SuggestList.module.css b/nextjs-app/components/SuggestList.module.css new file mode 100644 index 0000000..a785220 --- /dev/null +++ b/nextjs-app/components/SuggestList.module.css @@ -0,0 +1,53 @@ +/* + * Anchored to .omniBoxContainer, which is position: relative for this reason. + * + * Every colour is a token, so the dropdown follows the theme. The dark theme + * redefines --bg-card, --border, --text-muted and --shadow-soft, and this + * inherits all four without a second rule. + */ +.list { + position: absolute; + top: calc(100% + 4px); + left: 0; + right: 0; + /* Above the sticky filter bar (10) and the hero layers (0–2), below the + skip-link (10000) and the modal overlay (1000). */ + z-index: 40; + margin: 0; + padding: 4px; + list-style: none; + max-height: 320px; + overflow-y: auto; + background: var(--bg-card); + border: 1px solid var(--border); + border-radius: var(--radius-md); + box-shadow: var(--shadow-soft); +} + +.option { + display: flex; + align-items: baseline; + justify-content: space-between; + gap: 12px; + padding: 10px 12px; + border-radius: var(--radius-sm); + cursor: pointer; + color: var(--text-primary); +} + +/* Hover and keyboard share one style: the active option is the active option + however it became active. Two rules would drift. */ +.option:hover, +.active { + background: var(--bg-secondary); +} + +.name { + font-weight: 500; +} + +.meta { + font-size: 0.85em; + color: var(--text-muted); + white-space: nowrap; +} diff --git a/nextjs-app/components/SuggestList.tsx b/nextjs-app/components/SuggestList.tsx new file mode 100644 index 0000000..18071c4 --- /dev/null +++ b/nextjs-app/components/SuggestList.tsx @@ -0,0 +1,53 @@ +'use client'; + +/** + * The autosuggest dropdown. Presentational only — it fetches nothing and owns + * no state, so the fetching rules and the ARIA rules can be read separately. + */ + +import type { Suggestion } from '@/lib/suggest'; +import styles from './SuggestList.module.css'; + +/** The id the input's aria-activedescendant points at. */ +export function suggestOptionId(id: string, index: number): string { + return `${id}-option-${index}`; +} + +interface Props { + /** Shared with the input's aria-controls. */ + id: string; + suggestions: Suggestion[]; + activeIndex: number; + onPick: (s: Suggestion) => void; + onHover: (index: number) => void; +} + +export function SuggestList({ id, suggestions, activeIndex, onPick, onHover }: Props) { + if (suggestions.length === 0) return null; + + return ( +
    + {suggestions.map((s, i) => ( +
  • { e.preventDefault(); onPick(s); }} + onMouseEnter={() => onHover(i)} + > + {s.school_name} + {/* Not decoration: there are many "St Mary's". */} + {s.local_authority} +
    • + ))} +
+ ); +} From 28cf0a342c9f4a039bc5eeaa320b15abe99d4d93 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:38:52 +0100 Subject: [PATCH 09/11] feat(suggest): wire autosuggest into the search box behind a flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Off means off — no combobox role, no listener, no fetch. A test asserts the absence of the request, not just the absence of the dropdown, because a hidden-but-fetching control would still be spending the rate limit on a feature nobody can see. Enter with no active option falls through to the form's submit handler and searches the typed text exactly as before. The existing behaviour is preserved, not replaced, and that has its own test. Suppressed once the value parses as a postcode: the box takes a name OR a postcode, and suggesting schools during postcode entry fights the user. .omniBoxContainer gains position: relative — the dropdown is absolutely positioned and without it would have anchored to the page instead. Four render sites, all wired: page.tsx renders HomeView in the success path AND the catch fallback, and HomeView renders FilterBar as hero AND sticky. Missing any one would make the flag silently do nothing somewhere. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/flags.py | 7 ++ .../components/FilterBarSuggest.test.tsx | 76 +++++++++++++++++++ nextjs-app/app/page.tsx | 8 ++ nextjs-app/components/FilterBar.module.css | 2 + nextjs-app/components/FilterBar.tsx | 67 +++++++++++++++- nextjs-app/components/HomeView.tsx | 6 +- 6 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 nextjs-app/__tests__/components/FilterBarSuggest.test.tsx diff --git a/backend/flags.py b/backend/flags.py index f843828..a06e93e 100644 --- a/backend/flags.py +++ b/backend/flags.py @@ -50,6 +50,13 @@ REGISTRY: dict[str, Flag] = { ), added=date(2026, 8, 23), ), + Flag( + name="school_autosuggest", + description=( + "School name suggestions as you type in the main search box." + ), + added=date(2026, 8, 26), + ), ) } diff --git a/nextjs-app/__tests__/components/FilterBarSuggest.test.tsx b/nextjs-app/__tests__/components/FilterBarSuggest.test.tsx new file mode 100644 index 0000000..70ec8ac --- /dev/null +++ b/nextjs-app/__tests__/components/FilterBarSuggest.test.tsx @@ -0,0 +1,76 @@ +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { FilterBar } from '@/components/FilterBar'; + +const push = jest.fn(); +jest.mock('next/navigation', () => ({ + useRouter: () => ({ push, replace: jest.fn(), prefetch: jest.fn() }), + usePathname: () => '/', + useSearchParams: () => new URLSearchParams(), +})); + +const FILTERS = { + local_authorities: [], school_types: [], years: [], phases: [], + genders: [], admissions_policies: [], +}; + +const realFetch = global.fetch; +beforeEach(() => { + global.fetch = jest.fn(async () => ({ + ok: true, + json: async () => ({ suggestions: [{ + urn: 100010, school_name: 'Brecknock Primary School', + local_authority: 'Camden', postcode: 'NW1 1AA', + phase: 'Primary', school_type: 'Community school' }] }), + })) as unknown as typeof fetch; + push.mockClear(); +}); +afterEach(() => { global.fetch = realFetch; }); + +describe('FilterBar autosuggest', () => { + it('is a combobox only when the flag is on', () => { + const { rerender } = render(); + expect(screen.queryByRole('combobox')).not.toBeInTheDocument(); + rerender(); + expect(screen.getByRole('combobox')).toBeInTheDocument(); + }); + + it('makes no request while the flag is off', async () => { + // Off means off: no listener, no fetch, no markup. + render(); + await userEvent.type(screen.getByPlaceholderText(/School name or postcode/i), + 'brecknock'); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('shows suggestions and navigates when one is chosen', async () => { + render(); + await userEvent.type(screen.getByRole('combobox'), 'brecknock'); + const option = await screen.findByRole('option', { name: /Brecknock/ }); + await userEvent.click(option); + expect(push).toHaveBeenCalledWith( + expect.stringContaining('/school/100010')); + }); + + it('suppresses suggestions once the value is a postcode', async () => { + // The box takes a name OR a postcode; suggestions must get out of the way. + // + // fireEvent.change, not userEvent.type: typing sets "N", "NW", "NW1"... and + // "NW1" is not a postcode, so a request for it is correct behaviour. Only + // the settled value is the assertion, so set it in one go. + render(); + fireEvent.change(screen.getByRole('combobox'), { target: { value: 'NW1 1AA' } }); + await new Promise((r) => setTimeout(r, 300)); // past the 200ms debounce + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('Enter with no active option still submits the free-text search', async () => { + // The existing behaviour is preserved, not replaced. + render(); + const input = screen.getByRole('combobox'); + await userEvent.type(input, 'brecknock{Enter}'); + // updateURL pushes inside startTransition, so the call is not synchronous. + await waitFor(() => expect(push).toHaveBeenCalledWith( + expect.stringContaining('search=brecknock'))); + }); +}); diff --git a/nextjs-app/app/page.tsx b/nextjs-app/app/page.tsx index dfcdeda..f791b95 100644 --- a/nextjs-app/app/page.tsx +++ b/nextjs-app/app/page.tsx @@ -8,6 +8,7 @@ import type { Metadata } from 'next'; import { fetchSchools, fetchFilters, fetchDataInfo } from '@/lib/api'; import { formatAcademicYear } from '@/lib/utils'; import { HomeView } from '@/components/HomeView'; +import { getFlags } from '@/lib/flags'; import { HowItWorksSection } from '@/components/HowItWorksSection'; import { EditorialSection } from '@/components/EditorialSection'; @@ -63,6 +64,11 @@ export default async function HomePage({ searchParams }: HomePageProps) { // Await search params (Next.js 15 requirement) const params = await searchParams; + // Server-read: no flag value reaches the browser bundle. Threaded down to + // both FilterBar instances via HomeView. + const flags = await getFlags(); + const autosuggest = flags.school_autosuggest === true; + // Parse search params const page = parseInt(params.page || '1'); const radius = params.radius ? parseFloat(params.radius) : undefined; @@ -111,6 +117,7 @@ export default async function HomePage({ searchParams }: HomePageProps) { const years = dataInfo?.years_available ?? []; return ( void; geoState?: "idle" | "requesting" | "error"; geoError?: string | null; + /** Server-read feature flag. Off means no listener, no fetch, no markup. */ + autosuggest?: boolean; } /** @@ -48,6 +53,7 @@ export function FilterBar({ onNearMe, geoState = "idle", geoError, + autosuggest = false, }: FilterBarProps) { const router = useRouter(); const pathname = usePathname(); @@ -62,6 +68,45 @@ export function FilterBar({ const [omniValue, setOmniValue] = useState(initialOmniValue); + const suggestId = `school-suggest-${isHero ? "hero" : "bar"}`; + // Suppressed once the value parses as a postcode: the box takes a school + // name OR a postcode, and suggesting schools during postcode entry fights + // the user rather than helping them. + const suggestEnabled = autosuggest && !isValidPostcode(omniValue); + const { suggestions, open, activeIndex, setActiveIndex, close } = + useSchoolSuggest(omniValue, suggestEnabled); + + const pickSuggestion = (s: Suggestion) => { + close(); + track('search_submitted', { + query: s.school_name.toLowerCase(), + via: 'suggestion', + urn: s.urn, + has_postcode: false, + filters_active: '', + filters_count: 0, + }); + router.push(schoolUrl(s.urn, s.school_name)); + }; + + const handleOmniKeyDown = (e: React.KeyboardEvent) => { + if (!open) return; + if (e.key === "ArrowDown") { + e.preventDefault(); + setActiveIndex(activeIndex + 1 >= suggestions.length ? 0 : activeIndex + 1); + } else if (e.key === "ArrowUp") { + e.preventDefault(); + setActiveIndex(activeIndex <= 0 ? suggestions.length - 1 : activeIndex - 1); + } else if (e.key === "Escape") { + close(); + } else if (e.key === "Enter" && activeIndex >= 0) { + // Only when an option is active. With none, the event falls through to + // the form's submit handler and searches the typed text, as it does now. + e.preventDefault(); + pickSuggestion(suggestions[activeIndex]); + } + }; + const currentLA = searchParams.get("local_authority") || ""; const currentType = searchParams.get("school_type") || ""; const currentPhase = searchParams.get("phase") || ""; @@ -227,8 +272,19 @@ export function FilterBar({ type="search" value={omniValue} onChange={(e) => setOmniValue(e.target.value)} + onKeyDown={handleOmniKeyDown} + onBlur={close} placeholder="School name or postcode" className={styles.omniInput} + {...(autosuggest ? { + role: "combobox", + "aria-expanded": open, + "aria-controls": suggestId, + "aria-autocomplete": "list" as const, + "aria-activedescendant": + activeIndex >= 0 ? suggestOptionId(suggestId, activeIndex) : undefined, + autoComplete: "off", + } : {})} /> + {autosuggest && open && ( + + )} {isHero && ( <> diff --git a/nextjs-app/components/HomeView.tsx b/nextjs-app/components/HomeView.tsx index 205c626..fd90e77 100644 --- a/nextjs-app/components/HomeView.tsx +++ b/nextjs-app/components/HomeView.tsx @@ -29,6 +29,8 @@ interface HomeViewProps { // show (e.g. an active search). howItWorks?: React.ReactNode; editorial?: React.ReactNode; + /** Server-read feature flag, threaded to both FilterBar instances. */ + autosuggest?: boolean; } function daysUntil(month: number, day: number): number { @@ -193,7 +195,7 @@ const VALUE_PROPS: ValueProp[] = [ }, ]; -export function HomeView({ initialSchools, filters, totalSchools, howItWorks, editorial }: HomeViewProps) { +export function HomeView({ initialSchools, filters, totalSchools, howItWorks, editorial, autosuggest = false }: HomeViewProps) { const searchParams = useSearchParams(); const router = useRouter(); const pathname = usePathname(); @@ -462,6 +464,7 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed onNearMe={handleNearMe} geoState={geoState} geoError={geoError} + autosuggest={autosuggest} /> @@ -500,6 +503,7 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed onNearMe={handleNearMe} geoState={geoState} geoError={geoError} + autosuggest={autosuggest} /> )} From d2115364ae59d5d4fe206d09b0c99a2fdbe8c1ab Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:39:33 +0100 Subject: [PATCH 10/11] test(e2e): autosuggest journeys, gated on the flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Feature state is read from its observable effect — whether the search box is a combobox — because /api/flags is denied to the public on purpose. Same approach as the distance journeys. The three endpoint tests are ungated: /api/suggest is live whether or not the UI is, which is what lets it be smoke-tested in an environment where the feature is still dark. The flag-off journey asserts the plain search still works, not just that the combobox is absent. Verified against staging, where the flag is off: it passes and the flag-on journey correctly skips. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- e2e/tests/journeys.spec.ts | 63 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index e99174d..db3fab7 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -2111,3 +2111,66 @@ test('the rankings page still orders by score, not name', async ({ page }) => { .filter((v: number | null) => v != null); expect(scores).toEqual([...scores].sort((a: number, b: number) => b - a)); }); + +/* + * School autosuggest (spec 2026-08-26). + */ +async function autosuggestIsOn(page: Page): Promise { + await page.goto('/'); + return (await page.getByRole('combobox').count()) > 0; +} + +test('the suggest endpoint answers from Typesense', async ({ page }) => { + // Not flagged — the endpoint is live even while the UI is dark, so it can + // be smoke-tested before the feature is switched on. + const res = await page.request.get('/api/suggest?q=brecknock'); + expect(res.ok()).toBeTruthy(); + const { suggestions } = await res.json(); + expect(Array.isArray(suggestions)).toBeTruthy(); + if (suggestions.length) { + // Local authority is what tells two "St Mary's" apart. + expect(suggestions[0]).toHaveProperty('school_name'); + expect(suggestions[0]).toHaveProperty('local_authority'); + } +}); + +test('a one-character query is answered, not rejected', async ({ page }) => { + // The keystroke path never errors on ordinary input. + const res = await page.request.get('/api/suggest?q=b'); + expect(res.status()).toBe(200); + expect((await res.json()).suggestions).toEqual([]); +}); + +test('the suggest response is cacheable', async ({ page }) => { + const res = await page.request.get('/api/suggest?q=brecknock'); + expect(res.headers()['cache-control'] ?? '').toContain('s-maxage'); +}); + +test('typing a school name suggests it, and choosing it opens that school', async ({ page }) => { + test.skip(!(await autosuggestIsOn(page)), + 'the school_autosuggest flag is off in this environment'); + + // A school certain to exist in any environment with data. + const { schools } = await (await page.request.get('/api/schools?page_size=1')).json(); + test.skip(!schools?.length, 'no schools in this environment'); + const name = schools[0].school_name as string; + + await page.goto('/'); + await page.getByRole('combobox').first().fill(name.slice(0, 12)); + const option = page.getByRole('option').first(); + await expect(option).toBeVisible(); + await option.click(); + await expect(page).toHaveURL(/\/school\/\d+/); +}); + +test('with autosuggest off, the search box is a plain input', async ({ page }) => { + test.skip(await autosuggestIsOn(page), + 'the school_autosuggest flag is on in this environment'); + + await page.goto('/'); + await expect(page.getByRole('combobox')).toHaveCount(0); + // And the box still works: the existing search must be untouched. + await page.getByPlaceholder(/School name or postcode/i).first().fill('abbey'); + await page.getByRole('button', { name: /Search/i }).first().click(); + await expect(page).toHaveURL(/search=abbey/); +}); From 0fa1a292c76926cb04cd9bccc7ceec36caaaaad3 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 20:56:32 +0100 Subject: [PATCH 11/11] fix(api): bound what a forged CF-Connecting-IP can buy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review, both findings valid. The design doc claimed Cloudflare "replaces the header, so a browser cannot forge it", and that only the X-Forwarded-For fallback was forgeable. That is true only for traffic that actually passed through Cloudflare, and nothing in this process can verify that it did. Reaching the origin directly, both headers are equally attacker-controlled — and rotating CF-Connecting-IP mints a fresh rate-limit bucket per request, defeating per-client limits on every endpoint including the DataFrame-heavy /api/schools. Against abuse that is worse than the shared bucket it replaced, which at least capped everyone together. So the ceiling comes back. I dropped it earlier arguing it belonged at Cloudflare; that argument assumed the keying was sound, and it is not. GlobalRateLimitMiddleware counts all /api/ traffic in a fixed window against a total, independent of client identity, outermost so it refuses before any work happens. Written by hand because slowapi cannot express a global cap: default_limits and application_limits are both keyed by key_func, and the latter needs middleware this app does not install. It does not make the header trustworthy — it makes trusting it survivable. The real fix is Authenticated Origin Pulls or an origin firewall, now documented in DEPLOY.md as the open gap it is. 127.0.0.1 is exempt: the healthcheck curls localhost from inside the container, and starving it would restart the container and turn a load spike into an outage loop. Keyed on the peer address, never the Host header, which the caller sets. Second finding: suggest_schools_typesense promised "never raises" while the parsing loop sat outside the try, so int(None) on a malformed document would have made a keystroke a 500. The loop now skips bad rows rather than dropping the whole list — and a hit with no document no longer becomes a suggestion pointing at /school/0. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 82 ++++++++++++- backend/config.py | 5 + backend/data_loader.py | 18 ++- backend/tests/test_rate_limit_key.py | 89 ++++++++++++++ backend/tests/test_suggest.py | 29 +++++ docs/DEPLOY.md | 30 +++++ .../plans/2026-08-26-school-autosuggest.md | 20 ++-- .../2026-08-26-school-autosuggest-design.md | 109 ++++++++++++------ 8 files changed, 332 insertions(+), 50 deletions(-) diff --git a/backend/app.py b/backend/app.py index 832a039..9e60c1d 100644 --- a/backend/app.py +++ b/backend/app.py @@ -6,6 +6,7 @@ Uses real data from UK Government Compare School Performance downloads. import hashlib import re +import time from contextlib import asynccontextmanager from datetime import datetime, timezone from typing import Optional @@ -15,7 +16,7 @@ import pandas as pd from fastapi import FastAPI, HTTPException, Query, Request, Depends, Header from fastapi.middleware.cors import CORSMiddleware from fastapi.middleware.gzip import GZipMiddleware -from fastapi.responses import FileResponse, Response +from fastapi.responses import FileResponse, JSONResponse, Response from fastapi.staticfiles import StaticFiles from slowapi import Limiter, _rate_limit_exceeded_handler from slowapi.util import get_remote_address @@ -321,14 +322,79 @@ def client_key(request: Request) -> str: return get_remote_address(request) -# Rate limiter. No in-app global ceiling: slowapi's default_limits and -# application_limits are both keyed by key_func (so per-client, not global) -# and the latter only applies with SlowAPIMiddleware installed, which this app -# does not use. A global cap belongs at Cloudflare, which is already in the -# path. See the spec's §1 for why that is deliberate. +# Per-client limiter. Paired with the global ceiling below — the two do +# different jobs and neither substitutes for the other. limiter = Limiter(key_func=client_key) +# --- The ceiling no header can raise ---------------------------------------- +# +# client_key trusts CF-Connecting-IP, and nothing in this process can tell an +# edge-set header from an attacker-set one. That distinction can only be made +# at Cloudflare, with Authenticated Origin Pulls or an origin firewall. A +# caller reaching the origin directly could otherwise mint a fresh rate-limit +# bucket per request and evade per-client limits entirely — which would make +# correct keying a net regression against abuse, since the single shared bucket +# it replaced at least capped everyone at 60/minute together. +# +# So per-client limits give fairness, and this gives the origin a hard total. +# It does not make the header trustworthy; it bounds what trusting it can cost. +# The header problem itself is closed at Cloudflare, not here. +# +# [window_start_monotonic, count], or None before the first request. A fixed +# window is crude, which is right for a backstop: it has to be obviously +# correct rather than fair. +_global_window: Optional[list] = None + +# The container healthcheck runs `curl http://localhost:80/api/data-info` from +# inside the container. Starving it would fail the check, restart the +# container, and turn a load spike into an outage loop — the ceiling exists to +# protect the origin, not to kill it. +_LOCAL_HOSTS = frozenset({"127.0.0.1", "::1", "localhost"}) + + +def exempt_from_ceiling(request: Request) -> bool: + """Whether the ceiling should ignore this request. + + Its own function so the rule is testable without standing up a server — + and so the healthcheck exemption is somewhere a reader can find it. + """ + if not request.url.path.startswith("/api/"): + return True + # The peer address, never the Host header: Host is set by the caller and + # would hand every attacker an exemption. + return (request.client.host if request.client else "") in _LOCAL_HOSTS + + +class GlobalRateLimitMiddleware(BaseHTTPMiddleware): + """A cap on total /api/ traffic, independent of any client identity.""" + + async def dispatch(self, request: Request, call_next): + global _global_window + + if exempt_from_ceiling(request): + return await call_next(request) + + now = time.monotonic() + # One event loop, and no await between the read and the write, so this + # sequence is atomic without a lock. + if _global_window is None or now - _global_window[0] >= 60: + _global_window = [now, 0] + _global_window[1] += 1 + + if _global_window[1] > settings.global_rate_limit_per_minute: + return JSONResponse( + # Distinguishable from slowapi's per-client 429: an operator + # reading logs has to be able to tell "one noisy client" from + # "the origin is saturated". + {"detail": "The service is at capacity. Please retry shortly."}, + status_code=429, + headers={"Retry-After": + str(max(1, int(60 - (now - _global_window[0]))))}, + ) + return await call_next(request) + + class SecurityHeadersMiddleware(BaseHTTPMiddleware): """Add security headers to all responses.""" @@ -532,6 +598,10 @@ app.add_middleware(CacheAndETagMiddleware) app.add_middleware(SecurityHeadersMiddleware) app.add_middleware(RequestSizeLimitMiddleware) app.add_middleware(GZipMiddleware, minimum_size=512) +# Added last, so it is outermost and refuses before anything downstream does +# work. A ceiling that only applies after the expensive part has run is not a +# ceiling. +app.add_middleware(GlobalRateLimitMiddleware) # CORS middleware - restricted for production app.add_middleware( diff --git a/backend/config.py b/backend/config.py index 554efc9..f663ba7 100644 --- a/backend/config.py +++ b/backend/config.py @@ -35,6 +35,11 @@ class Settings(BaseSettings): # Security admin_api_key: str = Field(default_factory=lambda: secrets.token_urlsafe(32)) rate_limit_per_minute: int = 60 # Requests per minute per IP + # A ceiling on total /api/ traffic, independent of any client identity. + # client_key trusts headers only Cloudflare can vouch for, so a caller + # reaching the origin directly could otherwise mint a fresh bucket per + # request. See GlobalRateLimitMiddleware in backend/app.py. + global_rate_limit_per_minute: int = 3000 rate_limit_burst: int = 10 # Allow burst of requests max_request_size: int = 1024 * 1024 # 1MB max request size diff --git a/backend/data_loader.py b/backend/data_loader.py index bf0e2a5..8aff4df 100644 --- a/backend/data_loader.py +++ b/backend/data_loader.py @@ -132,9 +132,21 @@ def suggest_schools_typesense(query: str, limit: int = 8) -> List[dict]: return [] rows = [] - for hit in result.get("hits", []): - doc = hit.get("document", {}) - row = {"urn": int(doc.get("urn", 0))} + for hit in result.get("hits", []) or []: + doc = (hit or {}).get("document") or {} + try: + urn = int(doc["urn"]) + except (KeyError, TypeError, ValueError): + # Skip the row, keep the rest. Typesense declares urn as int32 so + # this should be unreachable, but the index is a separate system + # that something other than this code can reindex — and "never + # raises" is a promise the keystroke path actually depends on. + # Dropping one malformed document is right; blanking the whole + # dropdown, or serving a suggestion pointing at /school/0, is not. + logging.getLogger(__name__).warning( + "skipping malformed suggestion document: %r", doc) + continue + row = {"urn": urn} row.update({f: str(doc.get(f, "") or "") for f in _SUGGEST_FIELDS}) rows.append(row) return rows diff --git a/backend/tests/test_rate_limit_key.py b/backend/tests/test_rate_limit_key.py index 7435ef3..2f0a12a 100644 --- a/backend/tests/test_rate_limit_key.py +++ b/backend/tests/test_rate_limit_key.py @@ -56,3 +56,92 @@ def test_whitespace_is_stripped(): # first; an unstripped key silently creates a second bucket per client. assert client_key(_Req({"x-forwarded-for": " 203.0.113.7 ,10.0.0.2"})) \ == "203.0.113.7" + + +# --------------------------------------------------------------------------- +# The ceiling that header rotation cannot raise. +# --------------------------------------------------------------------------- + +import pytest +from fastapi.testclient import TestClient + + +@pytest.fixture() +def api(monkeypatch): + from backend import app as app_module + from backend.config import settings + + monkeypatch.setattr(settings, "global_rate_limit_per_minute", 5) + monkeypatch.setattr(app_module, "_global_window", None) + return TestClient(app_module.app, raise_server_exceptions=False) + + + +def _ceiling_req(path: str, host: str): + """Enough of a Request for exempt_from_ceiling: a path and a peer host.""" + return type("R", (), { + "url": type("U", (), {"path": path})(), + "client": type("C", (), {"host": host})(), + })() + + +def _get(client, path="/api/flags", cf=None): + headers = {"cf-connecting-ip": cf} if cf else {} + return client.get(path, headers=headers) + + +def test_rotating_the_cloudflare_header_cannot_buy_unlimited_requests(api): + """The attack the per-client keying opened up. + + client_key trusts CF-Connecting-IP, and nothing in this process can tell an + edge-set header from an attacker-set one — that distinction can only be + made at Cloudflare, with Authenticated Origin Pulls or an origin firewall. + A caller reaching the origin directly can therefore mint a fresh + rate-limit bucket per request and evade per-client limits entirely. + + Per-client fairness is still the right default; this is the backstop that + bounds what evading it can achieve. Without it, correct keying would be a + net regression against abuse compared with the shared bucket it replaced. + """ + codes = [_get(api, cf=f"203.0.113.{i}").status_code for i in range(8)] + assert codes.count(200) == 5 + assert codes.count(429) == 3 + + +def test_the_ceiling_says_which_limit_was_hit(api): + # Distinguishable from slowapi's per-client 429, or an operator reading + # logs cannot tell "one noisy client" from "the origin is saturated". + for i in range(5): + _get(api, cf=f"203.0.113.{i}") + refused = _get(api, cf="203.0.113.99") + assert refused.status_code == 429 + assert "capacity" in refused.json()["detail"].lower() + assert refused.headers.get("retry-after") + + +def test_traffic_below_the_ceiling_is_untouched(api): + codes = [_get(api, cf=f"203.0.113.{i}").status_code for i in range(5)] + assert codes == [200] * 5 + + +def test_the_container_healthcheck_is_exempt(api): + """The healthcheck runs `curl http://localhost:80/api/data-info` inside the + container. If the ceiling could starve it, saturation would fail the + healthcheck, restart the container, and turn a load spike into an outage + loop — the ceiling has to protect the origin, not kill it. + """ + from backend.app import exempt_from_ceiling + + assert exempt_from_ceiling(_ceiling_req("/api/data-info", "127.0.0.1")) + assert exempt_from_ceiling(_ceiling_req("/api/data-info", "::1")) + # Everyone else is counted. + assert not exempt_from_ceiling(_ceiling_req("/api/data-info", "10.0.0.9")) + + +def test_the_ceiling_ignores_non_api_paths(): + # Sitemaps and robots.txt are served by this app too, and a crawler + # fetching them must not be refused because the API is busy. + from backend.app import exempt_from_ceiling + + assert exempt_from_ceiling(_ceiling_req("/sitemap.xml", "10.0.0.9")) + assert exempt_from_ceiling(_ceiling_req("/robots.txt", "10.0.0.9")) diff --git a/backend/tests/test_suggest.py b/backend/tests/test_suggest.py index b2bcc53..180b7c3 100644 --- a/backend/tests/test_suggest.py +++ b/backend/tests/test_suggest.py @@ -126,3 +126,32 @@ def test_the_response_is_cacheable(monkeypatch): res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck") assert "s-maxage" in res.headers.get("cache-control", "") assert res.headers.get("etag") + + +def test_a_malformed_urn_does_not_raise(monkeypatch): + """The docstring promises "never raises"; the parsing loop sat outside the + try, so int(None) or int("abc") would have turned a keystroke into a 500. + + Typesense declares urn as int32, so this should be unreachable — but the + contract is what the caller relies on, and a search index is a separate + system that can be reindexed by something other than this code. + """ + _use(monkeypatch, _FakeClient([{"urn": None, "school_name": "X", + "local_authority": "Y", "postcode": "Z"}])) + assert data_loader.suggest_schools_typesense("x") == [] + + +def test_a_malformed_row_does_not_discard_the_good_ones(monkeypatch): + # One bad document must not blank the whole dropdown. + _use(monkeypatch, _FakeClient([ + {"urn": "not-a-number", "school_name": "Bad", "local_authority": "Y", + "postcode": "Z"}, + _HIT, + ])) + out = data_loader.suggest_schools_typesense("x") + assert [r["urn"] for r in out] == [100010] + + +def test_a_hit_with_no_document_does_not_raise(monkeypatch): + _use(monkeypatch, _FakeClient([{}])) + assert data_loader.suggest_schools_typesense("x") == [] diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index 14b6f83..92353d4 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -151,6 +151,36 @@ needed), and fails the check only when a finding is rated **severe** (would break prod, leak data, or corrupt data). Minor findings are informational and never block a merge. +## Rate limiting, and the Cloudflare gap + +Two independent limits protect the API: + +- **Per client**, via slowapi, keyed on `CF-Connecting-IP` (falling back to + `X-Forwarded-For`, then the peer address). 60/minute by default; + `/api/suggest` gets 120/minute because typing is bursty. +- **Globally**, via `GlobalRateLimitMiddleware`: a fixed 60-second window over + all `/api/` traffic, `GLOBAL_RATE_LIMIT_PER_MINUTE` (default 3000), + independent of any client identity. Requests from `127.0.0.1` are exempt so + the container healthcheck cannot be starved into a restart loop. + +### Open: the origin must only accept Cloudflare + +`CF-Connecting-IP` is only meaningful for requests that actually reached the +origin through Cloudflare, and **the application cannot verify that they did**. +Anything able to reach the origin directly can set that header freely and, by +rotating it, mint a fresh rate-limit bucket per request — defeating per-client +limits on every endpoint. + +The global ceiling bounds the damage to total origin capacity. It does not fix +the underlying gap, and nothing in the code can. Closing it needs one of: + +- **Authenticated Origin Pulls** — Cloudflare presents a client certificate the + origin requires, so non-Cloudflare traffic is refused at TLS. +- **An origin firewall** restricted to Cloudflare's published IP ranges. + +Until one is in place, treat per-client limits as protection against accidents +and ordinary load, not against a determined caller. + ## Feature flags (Unleash) Flag state lives in a self-hosted Unleash instance, deployed as its own diff --git a/docs/superpowers/plans/2026-08-26-school-autosuggest.md b/docs/superpowers/plans/2026-08-26-school-autosuggest.md index 626a61e..e57c19b 100644 --- a/docs/superpowers/plans/2026-08-26-school-autosuggest.md +++ b/docs/superpowers/plans/2026-08-26-school-autosuggest.md @@ -1335,14 +1335,20 @@ site-wide; autosuggest itself is off until toggled in Unleash; and that **Task 1 changes rate limiting for every endpoint.** It is the one change here that is not behind a flag, and it is the one worth the most review attention. Before it, everyone shares one 60/minute bucket; after it, each caller gets -their own. That is the intended fix, and it also removes an accidental global -throttle — the spec's §1 and Risks say so plainly. A global ceiling belongs at -Cloudflare and is deliberately not built here. +their own, bounded by a global ceiling. The header it keys on is only +trustworthy for traffic that actually passed through Cloudflare, which this +app cannot verify — closing that needs Authenticated Origin Pulls or an origin +firewall, and is the most valuable follow-up in the spec's Risks. -**Do not add an in-app global rate limit.** An earlier spec draft did. slowapi's -`default_limits` and `application_limits` are both keyed by `key_func`, so they -are per-client rather than global, and `application_limits` only apply with -`SlowAPIMiddleware` installed, which this app does not use. +**The in-app global ceiling is required, and slowapi cannot express it.** +Code review found that `client_key` trusts `CF-Connecting-IP` with no way to +verify the request reached the origin through Cloudflare — so rotating that +header mints a fresh bucket per request and defeats per-client limits entirely, +which is worse against abuse than the shared bucket it replaced. The ceiling +(`GlobalRateLimitMiddleware`) bounds that, and is written by hand because +slowapi's `default_limits` and `application_limits` are both keyed by +`key_func` — per-client, not global — and the latter needs `SlowAPIMiddleware`, +which this app does not install. See spec §1.1. **`onMouseDown`, not `onClick`, on the options.** The input's `onBlur` closes the list and blur fires first, so a click handler never runs. This is the diff --git a/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md index 3e798b0..bc79dad 100644 --- a/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md +++ b/docs/superpowers/specs/2026-08-26-school-autosuggest-design.md @@ -58,42 +58,80 @@ def client_key(request: Request) -> str: except `host` and `connection`, so `CF-Connecting-IP` reaches the backend with no proxy change. -Two things make this safe: Cloudflare replaces the header, so a browser cannot -forge it; and the backend is unreachable from outside the Docker network, so -nothing can reach it without passing through the proxy. The `X-Forwarded-For` -fallback *is* forgeable, but only by a caller already inside that network. +**This header is trustworthy only for traffic that actually passed through +Cloudflare, and nothing in the application can verify that it did.** An earlier +draft of this section claimed Cloudflare "replaces the header, so a browser +cannot forge it", and that only the `X-Forwarded-For` fallback was forgeable. +That was wrong. Cloudflare does overwrite the header *on requests it handles* — +but a caller reaching the origin directly sets whatever it likes, and this +process cannot distinguish an edge-set header from an attacker-set one. Both +headers are equally forgeable in that scenario. -### The part that is not free +The consequence is sharper than a weakened defence. An attacker rotating +`CF-Connecting-IP` per request mints a fresh rate-limit bucket every time and +evades per-client limits entirely — including on the DataFrame-heavy +`/api/schools`. Against abuse that is *worse* than the shared bucket it +replaced, which at least capped everyone at 60/minute together. -The shared bucket has been acting as an accidental global throttle on a +Two mitigations, and they are not interchangeable: + +1. **The real fix is at Cloudflare** — Authenticated Origin Pulls, or an origin + firewall that refuses connections not from Cloudflare's ranges. Only the + edge can vouch for its own header. This is infrastructure work and is not + part of this change; it is the thing that makes the header mean anything. +2. **The ceiling in §1.1 bounds what evading the keying can achieve** while + that remains open. It does not make the header trustworthy — it makes + trusting it survivable. + +The backend being unreachable from outside the Docker network is a real second +layer, but it depends on the ingress path in front of the frontend, which this +design does not control and should not assume. + +### 1.1 The ceiling, which is back + +The shared bucket was acting as an accidental global throttle on a single-process uvicorn backend that filters a 25,000-row DataFrame in-process. -Correct per-user keying removes that throttle: the origin becomes reachable at -60/min *per user* rather than 60/min in total. +Correct per-user keying removes it: the origin becomes reachable at 60/min *per +user* rather than 60/min in total, and — per above — at an unbounded rate by +anyone willing to rotate a header. -Per-user fairness and origin protection are different jobs. Conflating them -is what produced the current behaviour, and the fix must not quietly do it -again in the other direction. +An earlier draft dropped the in-app ceiling, arguing it belonged at Cloudflare. +That argument assumed the keying was sound. It is not, so the ceiling is +load-bearing rather than redundant, and it ships here: -**The global ceiling does not go in this app.** An earlier draft of this -section specified one via slowapi's `default_limits`. Reading the library -shows that would not have worked, twice over: `default_limits` and -`application_limits` are both evaluated with the same `key_func`, so they are -per-client across all routes rather than global; and `application_limits` are -only applied `if in_middleware`, while this app installs no `SlowAPIMiddleware` -at all. Expressing a genuine global cap would take a second `Limiter` with a -constant key plus that middleware — two mechanisms to keep correct, for a -protection this layer is the wrong place for. +`GlobalRateLimitMiddleware` counts all `/api/` requests in a fixed 60-second +window against `global_rate_limit_per_minute` (3000), independent of any client +identity, and refuses with a 429 that names capacity rather than the client — +an operator has to be able to tell "one noisy client" from "the origin is +saturated". It is registered last so it is outermost: a ceiling that applies +after the expensive work has run is not a ceiling. -Cloudflare is already in the request path on both environments and does -edge-level rate limiting properly, before traffic reaches a single-process -origin at all. That is where a global ceiling belongs, and it is a dashboard -change rather than code. Flagged as a follow-up, deliberately not built here. +slowapi cannot express this. `default_limits` and `application_limits` are both +evaluated with the same `key_func`, making them per-client rather than global, +and `application_limits` only apply with `SlowAPIMiddleware` installed, which +this app does not use. Hence the explicit middleware — about thirty lines, and +obviously correct, which is what a backstop needs to be. -What ships instead is conservative per-user limits: the existing 60/minute -default is unchanged, and `/api/suggest` gets 120/minute. Both are estimates -rather than measurements, and they are a starting point to revisit once the -keying is correct enough for real per-user traffic to be visible — which it -is not today, because everyone shares one bucket. +Requests from `127.0.0.1` are exempt. The container healthcheck runs +`curl http://localhost:80/api/data-info` from inside the container, and +starving it would fail the check, restart the container, and turn a load spike +into an outage loop. The exemption keys on the peer address, never the `Host` +header, which the caller sets. + +3000/minute is an estimate, not a measurement, and worth revisiting against +real traffic. + +### Per-user limits + +Per-user fairness and origin protection are different jobs, and this design now +does both separately: the ceiling above for the origin, and per-route limits +for fairness. Conflating them is what produced the original behaviour, where +one bucket served the whole internet. + +The existing 60/minute default is unchanged, and `/api/suggest` gets +120/minute. Both are estimates rather than measurements, and are a starting +point to revisit once the keying is correct enough for real per-user traffic to +be visible — which it was not before, because everyone shared one bucket. ## 2. `GET /api/suggest` @@ -228,11 +266,14 @@ with many source addresses can put more load on a single-process origin than they can today. Against this site's traffic that is a theoretical risk rather than a live one, but it is a real one and it is the price of the fix. -**Cloudflare bypass.** If the origin is reachable without passing through -Cloudflare, `CF-Connecting-IP` is absent and the `X-Forwarded-For` fallback is -forgeable, so limits could be evaded per-request. Closing that properly means -Authenticated Origin Pulls or an origin firewall, which is infrastructure work -outside this change. Worth doing separately. +**Cloudflare bypass — the open one.** If the origin is reachable without +passing through Cloudflare, `CF-Connecting-IP` is attacker-controlled, and +rotating it per request defeats per-client limits on every endpoint. The +ceiling in §1.1 bounds the damage to the origin's total capacity; it does not +restore per-client fairness under attack, and it cannot. Closing this properly +means Authenticated Origin Pulls or an origin firewall restricted to +Cloudflare's published ranges — infrastructure work, outside this change, and +the single most valuable follow-up here. **Typesense becomes user-visible.** Today a Typesense outage degrades search to a slow substring match. With autosuggest it also means the dropdown silently