The rate limiter was broken, and autosuggest would have made it obvious
limiter = Limiter(key_func=get_remote_address) reads request.client.host. 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 60/minute bucket per route.
Measured against staging before the fix: 70 concurrent requests to /api/schools returned exactly 60 × 200 and 10 × 429, from one machine. One person typing "st marys primary" produces 6–8 debounced requests, so eight concurrent searchers would have 429'd the site.
Now keyed on CF-Connecting-IP. 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 — slowapi cannot express one, because 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. That is in the spec's Risks, not papered over.
/api/suggest
A dedicated endpoint, not a mode of /api/schools — that path filters and sorts 25,000 pandas rows per query while holding the GIL, which is affordable once per search and not once per keystroke. Every field a suggestion needs is already in the Typesense index.
A test asserts the distinction directly: it makes load_school_dataraise and requires the endpoint to answer anyway.
Short query, no matches, or Typesense down → 200 with {"suggestions": []}. The keystroke path never errors on ordinary input.
No DataFrame fallback, deliberately: the 25,000-row substring scan is exactly what this exists to avoid.
Cached (s-maxage=3600 + ETag). Prefix queries repeat enormously; without this every keystroke pays full price.
120/min per client — typing is legitimately bursty.
Not flagged, so it can be smoke-tested while the UI is still dark.
The combobox
Full ARIA — role="combobox", aria-expanded, aria-activedescendant, listbox/option — with arrow/Enter/Escape handling. Enter with no active option still submits the free-text search exactly as today, and that has its own test.
AbortController per keystroke. Correctness, not economy: without it a slow response for "st" lands after the fast one for "st marys" and replaces a correct list with a stale one.
onMouseDown, not onClick — the input's blur closes the list and blur fires first, the classic bug where a dropdown works by keyboard and is dead to the mouse.
Suppressed once the value parses as a postcode; the box takes a name or a postcode.
Local authority on every row, because there are many "St Mary's".
Three things the plan got wrong, fixed during execution
CSS tokens were invented. The plan used --color-surface, --color-border etc. The real tokens are --bg-card, --border, --text-muted, --bg-secondary, --shadow-soft — all five redefined in the dark theme. The guessed names would have silently fallen back to hardcoded light values and broken dark mode.
.omniBoxContainer had no position: relative, so the absolutely-positioned dropdown would have anchored to the page. Added.
jest.setup.js needed no change here, but the @jest-environment node guard added in the flags PR is what lets these suites coexist.
Reproduced CI's exact path on Python 3.12 — pip install -r requirements.txt, import smoke test, suite green
Ran the UI journeys against staging: the flag-off journey passes (and asserts the existing search still works), the flag-on journey correctly skips
To switch it on
Create school_autosuggest in Unleash — exact name, Release type — and enable it under development for staging. Same shape as admission_distance.
School name suggestions in the main search box, behind the `school_autosuggest` flag — plus the rate-limit fix that had to come first.
**Autosuggest is off until toggled in Unleash.** The rate-limit change is **not** flagged and applies immediately on merge.
Spec: `docs/superpowers/specs/2026-08-26-school-autosuggest-design.md` · Plan: `docs/superpowers/plans/2026-08-26-school-autosuggest.md`
## The rate limiter was broken, and autosuggest would have made it obvious
`limiter = Limiter(key_func=get_remote_address)` reads `request.client.host`. 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 60/minute bucket per route**.
Measured against staging before the fix: 70 concurrent requests to `/api/schools` returned exactly **60 × 200 and 10 × 429**, from one machine. One person typing "st marys primary" produces 6–8 debounced requests, so eight concurrent searchers would have 429'd the site.
Now keyed on `CF-Connecting-IP`. 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 — slowapi cannot express one, because `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. That is in the spec's Risks, not papered over.
## `/api/suggest`
A dedicated endpoint, not a mode of `/api/schools` — that path filters and sorts 25,000 pandas rows per query while holding the GIL, which is affordable once per search and not once per keystroke. Every field a suggestion needs is already in the Typesense index.
A test asserts the distinction directly: it makes `load_school_data` **raise** and requires the endpoint to answer anyway.
- Short query, no matches, or Typesense down → `200` with `{"suggestions": []}`. The keystroke path never errors on ordinary input.
- **No DataFrame fallback**, deliberately: the 25,000-row substring scan is exactly what this exists to avoid.
- Cached (`s-maxage=3600` + ETag). Prefix queries repeat enormously; without this every keystroke pays full price.
- 120/min per client — typing is legitimately bursty.
- Not flagged, so it can be smoke-tested while the UI is still dark.
## The combobox
Full ARIA — `role="combobox"`, `aria-expanded`, `aria-activedescendant`, listbox/option — with arrow/Enter/Escape handling. **Enter with no active option still submits the free-text search exactly as today**, and that has its own test.
- `AbortController` per keystroke. Correctness, not economy: without it a slow response for "st" lands after the fast one for "st marys" and replaces a correct list with a stale one.
- `onMouseDown`, not `onClick` — the input's blur closes the list and blur fires first, the classic bug where a dropdown works by keyboard and is dead to the mouse.
- Suppressed once the value parses as a postcode; the box takes a name *or* a postcode.
- Local authority on every row, because there are many "St Mary's".
## Three things the plan got wrong, fixed during execution
1. **CSS tokens were invented.** The plan used `--color-surface`, `--color-border` etc. The real tokens are `--bg-card`, `--border`, `--text-muted`, `--bg-secondary`, `--shadow-soft` — all five redefined in the dark theme. The guessed names would have silently fallen back to hardcoded light values and broken dark mode.
2. **`.omniBoxContainer` had no `position: relative`**, so the absolutely-positioned dropdown would have anchored to the page. Added.
3. **`jest.setup.js`** needed no change here, but the `@jest-environment node` guard added in the flags PR is what lets these suites coexist.
## Testing
- Backend **150**, frontend **284**, `tsc` clean, `next build` green, **98** E2E collected (was 93)
- Reproduced CI's exact path on Python 3.12 — `pip install -r requirements.txt`, import smoke test, suite green
- Ran the UI journeys against staging: the flag-off journey passes (and asserts the existing search still works), the flag-on journey correctly skips
## To switch it on
Create `school_autosuggest` in Unleash — exact name, Release type — and enable it under **development** for staging. Same shape as `admission_distance`.
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
This PR adds a Typesense-backed /api/suggest autosuggest endpoint (flagged UI, unflagged endpoint) and, as a prerequisite, fixes the rate limiter to key on the real caller instead of the shared Next-proxy IP. The change is well-tested (backend and frontend), fails safe on Typesense errors, and preserves existing search behavior; two edge-case gaps are worth a look before merge but neither blocks it.
🟡 Minor
backend/app.py: client_key() trusts the CF-Connecting-IP header unconditionally, with no verification that the request actually passed through Cloudflare's edge (no Authenticated Origin Pulls / source-IP allowlist). The accompanying design doc claims only the X-Forwarded-For fallback is forgeable if Cloudflare is bypassed, but CF-Connecting-IP is equally forgeable in that scenario since nothing here distinguishes an edge-set header from an attacker-set one — so a caller that reaches the origin directly can rotate this header per request to get an unlimited number of rate-limit buckets, fully defeating per-client limits on every endpoint (including the DataFrame-heavy /api/schools).
backend/data_loader.py: suggest_schools_typesense()'s docstring promises it 'never raises', but only the Typesense search() call is wrapped in try/except; the result-processing loop (line 137, int(doc.get('urn', 0))) runs outside it. A document with a missing or non-numeric urn field would raise an uncaught ValueError/TypeError, turning the keystroke path into a 500 instead of the intended 200 with an empty list.
## 🤖 AI Code Review (Claude Code)
This PR adds a Typesense-backed /api/suggest autosuggest endpoint (flagged UI, unflagged endpoint) and, as a prerequisite, fixes the rate limiter to key on the real caller instead of the shared Next-proxy IP. The change is well-tested (backend and frontend), fails safe on Typesense errors, and preserves existing search behavior; two edge-case gaps are worth a look before merge but neither blocks it.
### 🟡 Minor
- **backend/app.py**: client_key() trusts the CF-Connecting-IP header unconditionally, with no verification that the request actually passed through Cloudflare's edge (no Authenticated Origin Pulls / source-IP allowlist). The accompanying design doc claims only the X-Forwarded-For fallback is forgeable if Cloudflare is bypassed, but CF-Connecting-IP is equally forgeable in that scenario since nothing here distinguishes an edge-set header from an attacker-set one — so a caller that reaches the origin directly can rotate this header per request to get an unlimited number of rate-limit buckets, fully defeating per-client limits on every endpoint (including the DataFrame-heavy /api/schools).
- **backend/data_loader.py**: suggest_schools_typesense()'s docstring promises it 'never raises', but only the Typesense search() call is wrapped in try/except; the result-processing loop (line 137, int(doc.get('urn', 0))) runs outside it. A document with a missing or non-numeric urn field would raise an uncaught ValueError/TypeError, turning the keystroke path into a 500 instead of the intended 200 with an empty list.
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
tudor
merged commit d8ccb5b733 into main2026-08-26 19:58:58 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
School name suggestions in the main search box, behind the
school_autosuggestflag — plus the rate-limit fix that had to come first.Autosuggest is off until toggled in Unleash. The rate-limit change is not flagged and applies immediately on merge.
Spec:
docs/superpowers/specs/2026-08-26-school-autosuggest-design.md· Plan:docs/superpowers/plans/2026-08-26-school-autosuggest.mdThe rate limiter was broken, and autosuggest would have made it obvious
limiter = Limiter(key_func=get_remote_address)readsrequest.client.host. 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 60/minute bucket per route.Measured against staging before the fix: 70 concurrent requests to
/api/schoolsreturned exactly 60 × 200 and 10 × 429, from one machine. One person typing "st marys primary" produces 6–8 debounced requests, so eight concurrent searchers would have 429'd the site.Now keyed on
CF-Connecting-IP. Cloudflare fronts both environments and overwrites any client-supplied value, which a parsedX-Forwarded-Forchain 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 — slowapi cannot express one, because
default_limitsandapplication_limitsare both keyed bykey_func(per-client, not global) and the latter needsSlowAPIMiddleware, which this app does not install. That is in the spec's Risks, not papered over./api/suggestA dedicated endpoint, not a mode of
/api/schools— that path filters and sorts 25,000 pandas rows per query while holding the GIL, which is affordable once per search and not once per keystroke. Every field a suggestion needs is already in the Typesense index.A test asserts the distinction directly: it makes
load_school_dataraise and requires the endpoint to answer anyway.200with{"suggestions": []}. The keystroke path never errors on ordinary input.s-maxage=3600+ ETag). Prefix queries repeat enormously; without this every keystroke pays full price.The combobox
Full ARIA —
role="combobox",aria-expanded,aria-activedescendant, listbox/option — with arrow/Enter/Escape handling. Enter with no active option still submits the free-text search exactly as today, and that has its own test.AbortControllerper keystroke. Correctness, not economy: without it a slow response for "st" lands after the fast one for "st marys" and replaces a correct list with a stale one.onMouseDown, notonClick— the input's blur closes the list and blur fires first, the classic bug where a dropdown works by keyboard and is dead to the mouse.Three things the plan got wrong, fixed during execution
--color-surface,--color-borderetc. The real tokens are--bg-card,--border,--text-muted,--bg-secondary,--shadow-soft— all five redefined in the dark theme. The guessed names would have silently fallen back to hardcoded light values and broken dark mode..omniBoxContainerhad noposition: relative, so the absolutely-positioned dropdown would have anchored to the page. Added.jest.setup.jsneeded no change here, but the@jest-environment nodeguard added in the flags PR is what lets these suites coexist.Testing
tscclean,next buildgreen, 98 E2E collected (was 93)pip install -r requirements.txt, import smoke test, suite greenTo switch it on
Create
school_autosuggestin Unleash — exact name, Release type — and enable it under development for staging. Same shape asadmission_distance.🤖 AI Code Review (Claude Code)
This PR adds a Typesense-backed /api/suggest autosuggest endpoint (flagged UI, unflagged endpoint) and, as a prerequisite, fixes the rate limiter to key on the real caller instead of the shared Next-proxy IP. The change is well-tested (backend and frontend), fails safe on Typesense errors, and preserves existing search behavior; two edge-case gaps are worth a look before merge but neither blocks it.
🟡 Minor