From 6e0a27834066283709a1adeb81732c9154952097 Mon Sep 17 00:00:00 2001 From: Tudor Date: Wed, 26 Aug 2026 19:48:39 +0100 Subject: [PATCH] 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.