The compare basket was capped at 5 schools in total. It is now capped at 5 primary and 5 secondary schools, so up to 10 in all, matching the compare page's phase tabs.
All-through, special (phase "Not applicable") and unknown-phase schools count against both groups. The compare page assigns tabs from results data that isn't known at add time, so this is what guarantees no tab exceeds the five-slot chart palette and point styles.
Search modal disables only schools whose group is full ("Primary full" / "Secondary full") and says which group is full.
Rankings rows now carry the phase of the tab they're ranked under (they had none).
Shared /compare?urns=… links are trimmed per group, in order.
Backend /api/compare already allowed 10 URNs; no API change.
Copy (SEO)
/compare title: "School Comparison Tool: Primary and Secondary Schools"; description names both phases.
Homepage value prop: "Up to five primary and five secondary schools side by side…"
How it works, Compare card: rewritten around "primary and secondary school performance", KS2 SATs, GCSE Attainment 8 and Ofsted ratings.
Tests
tsc --noEmit clean; Jest 56 suites / 482 tests pass.
E2E: the landing page states the real comparison limit now expects "five primary and five secondary schools". It runs against staging post-merge, so it isn't provable in PR checks.
## What
The compare basket was capped at **5 schools in total**. It is now capped at **5 primary and 5 secondary schools**, so up to 10 in all, matching the compare page's phase tabs.
## How
- `lib/compareLogic.ts`: `MAX_PER_GROUP`, `compareGroups`, `fullGroupFor`, `fitToGroupLimits` (pure, unit-tested).
- **All-through, special (phase "Not applicable") and unknown-phase schools count against both groups.** The compare page assigns tabs from results data that isn't known at add time, so this is what guarantees no tab exceeds the five-slot chart palette and point styles.
- Search modal disables only schools whose group is full ("Primary full" / "Secondary full") and says which group is full.
- Rankings rows now carry the phase of the tab they're ranked under (they had none).
- Shared `/compare?urns=…` links are trimmed per group, in order.
- Backend `/api/compare` already allowed 10 URNs; no API change.
## Copy (SEO)
- `/compare` title: "School Comparison Tool: Primary and Secondary Schools"; description names both phases.
- Homepage value prop: "Up to five primary and five secondary schools side by side…"
- How it works, Compare card: rewritten around "primary and secondary school performance", KS2 SATs, GCSE Attainment 8 and Ofsted ratings.
## Tests
- `tsc --noEmit` clean; Jest 56 suites / 482 tests pass.
- E2E: `the landing page states the real comparison limit` now expects "five primary and five secondary schools". It runs against staging post-merge, so it isn't provable in PR checks.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
A parent choosing a primary and a secondary school at once hit the old
cap of five total. The basket now holds up to five primary and five
secondary schools (ten in all), matching the compare page's phase tabs.
Schools that could land in either tab (all-through, special schools with
phase "Not applicable", unknown phase) count against both groups, so no
tab ever exceeds the five-slot chart palette and point styles.
- lib/compareLogic: compareGroups, fullGroupFor, fitToGroupLimits
- search modal disables only the full group and says which one
- rankings rows carry the phase of the tab they are ranked under
- shared ?urns= links are trimmed per group
- copy: compare metadata, homepage value prop, How it works card now
name primary and secondary schools (also better for search intent)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replaces the flat five-school comparison basket with a limit of five per phase group (primary and secondary). The new logic lives in lib/compareLogic.ts, and the provider, search modal, rankings, copy and tests are updated to match. The logic and tests look internally consistent, and I found no blocking problems in the diff.
🟡 Minor
nextjs-app/context/ComparisonContext.tsx: canAddMore is removed from the context type and replaced by fullGroupFor. The diff shows only SchoolSearchModal being updated. Any other consumer, such as the useComparison hook, school-page compare buttons or the compare page, would fail the type-check or break at runtime. Grep for remaining canAddMore usages before merging.
nextjs-app/lib/compareLogic.ts: Schools with a missing or unknown phase count against both groups. Entries already in users' localStorage without a phase, or added from paths that don't set one, will eat a slot in each group. A basket can therefore show 'full' earlier than expected. This is the deliberate conservative design, but stored data isn't migrated.
nextjs-app/components/HomeView.tsx: The edited doc comment has awkwardly nested parentheses: '(now MAX_PER_GROUP = 5 ...) (and the card ...'. This is cosmetic only.
## 🤖 AI Code Review (Claude Code)
Replaces the flat five-school comparison basket with a limit of five per phase group (primary and secondary). The new logic lives in `lib/compareLogic.ts`, and the provider, search modal, rankings, copy and tests are updated to match. The logic and tests look internally consistent, and I found no blocking problems in the diff.
### 🟡 Minor
- **nextjs-app/context/ComparisonContext.tsx**: `canAddMore` is removed from the context type and replaced by `fullGroupFor`. The diff shows only `SchoolSearchModal` being updated. Any other consumer, such as the `useComparison` hook, school-page compare buttons or the compare page, would fail the type-check or break at runtime. Grep for remaining `canAddMore` usages before merging.
- **nextjs-app/lib/compareLogic.ts**: Schools with a missing or unknown phase count against both groups. Entries already in users' localStorage without a phase, or added from paths that don't set one, will eat a slot in each group. A basket can therefore show 'full' earlier than expected. This is the deliberate conservative design, but stored data isn't migrated.
- **nextjs-app/components/HomeView.tsx**: The edited doc comment has awkwardly nested parentheses: '(now MAX_PER_GROUP = 5 ...) (and the card ...'. This is cosmetic only.
Review of the per-phase limit found entries reaching the basket with no
phase, and a phase-less entry holds a slot in both groups:
- Nearby-school cards added without one. The API now returns each
peer's own phase (its pool is a phase group, so an all-through school
can sit beside a primary); the button passes it through, and an older
API simply leaves the conservative both-groups count in place.
- Baskets saved before this change were never migrated. The compare
page now backfills missing phases from the data it already fetches,
and never overwrites a phase the basket has.
- "16 plus" counted against both groups; it is secondary, as the API's
PHASE_GROUPS files it.
Also rewraps the HomeView doc comment the previous commit left awkward.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
canAddMore: no remaining usages (grep is clean; tsc passes). Every add path was checked, and that turned up the next item.
Phase-less entries: fixed at the source rather than accepted.
Nearby-school cards were adding schools without a phase (a live path, not just stale data). The backend nearby payload now includes each peer's own phase, with a test. The frontend treats the field as optional, so a frontend deployed ahead of the API falls back to the conservative both-groups count.
Stored baskets: /compare backfills missing phases from the data it already fetches (backfillPhases, which never overwrites an existing phase).
16 plus now counts as secondary, matching the backend PHASE_GROUPS.
HomeView comment: rewrapped.
Verified: backend pytest 231 passed; tsc clean; Jest 57 suites / 486 tests passed, including a new provider suite (__tests__/context/ComparisonProvider.test.tsx).
Note: this PR now touches backend/nearby_schools.py, so the API image changes too. Staging needs both images deployed before the nearby phase is live.
Review follow-up (587cfe3):
- **`canAddMore`**: no remaining usages (grep is clean; `tsc` passes). Every add path was checked, and that turned up the next item.
- **Phase-less entries**: fixed at the source rather than accepted.
- Nearby-school cards were adding schools **without a phase** (a live path, not just stale data). The backend nearby payload now includes each peer's own `phase`, with a test. The frontend treats the field as optional, so a frontend deployed ahead of the API falls back to the conservative both-groups count.
- Stored baskets: `/compare` backfills missing phases from the data it already fetches (`backfillPhases`, which never overwrites an existing phase).
- `16 plus` now counts as secondary, matching the backend `PHASE_GROUPS`.
- **HomeView comment**: rewrapped.
Verified: backend pytest 231 passed; `tsc` clean; Jest 57 suites / 486 tests passed, including a new provider suite (`__tests__/context/ComparisonProvider.test.tsx`).
Note: this PR now touches `backend/nearby_schools.py`, so the API image changes too. Staging needs both images deployed before the nearby phase is live.
The compare basket limit changes from five overall to five per phase group (primary and secondary). Schools with an unknown or all-through phase count against both groups. The change adds a per-peer phase field to the nearby-schools API, a phase backfill for old stored baskets, updated copy and tests. The logic looks consistent and well covered, and I found no blocking problems.
🟡 Minor
nextjs-app/context/ComparisonProvider.tsx: alert() is called inside the setSelectedSchools updater function. Updaters should be pure, and React StrictMode in development can invoke them twice, which would show duplicate alerts. This pattern already existed, so it is not a regression, but the new message is still emitted from the updater.
## 🤖 AI Code Review (Claude Code)
The compare basket limit changes from five overall to five per phase group (primary and secondary). Schools with an unknown or all-through phase count against both groups. The change adds a per-peer `phase` field to the nearby-schools API, a phase backfill for old stored baskets, updated copy and tests. The logic looks consistent and well covered, and I found no blocking problems.
### 🟡 Minor
- **nextjs-app/context/ComparisonProvider.tsx**: `alert()` is called inside the `setSelectedSchools` updater function. Updaters should be pure, and React StrictMode in development can invoke them twice, which would show duplicate alerts. This pattern already existed, so it is not a regression, but the new message is still emitted from the updater.
tudor
merged commit b34feb8e98 into main2026-09-30 11:23:08 +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.
What
The compare basket was capped at 5 schools in total. It is now capped at 5 primary and 5 secondary schools, so up to 10 in all, matching the compare page's phase tabs.
How
lib/compareLogic.ts:MAX_PER_GROUP,compareGroups,fullGroupFor,fitToGroupLimits(pure, unit-tested)./compare?urns=…links are trimmed per group, in order./api/comparealready allowed 10 URNs; no API change.Copy (SEO)
/comparetitle: "School Comparison Tool: Primary and Secondary Schools"; description names both phases.Tests
tsc --noEmitclean; Jest 56 suites / 482 tests pass.the landing page states the real comparison limitnow expects "five primary and five secondary schools". It runs against staging post-merge, so it isn't provable in PR checks.🤖 Generated with Claude Code
🤖 AI Code Review (Claude Code)
Replaces the flat five-school comparison basket with a limit of five per phase group (primary and secondary). The new logic lives in
lib/compareLogic.ts, and the provider, search modal, rankings, copy and tests are updated to match. The logic and tests look internally consistent, and I found no blocking problems in the diff.🟡 Minor
canAddMoreis removed from the context type and replaced byfullGroupFor. The diff shows onlySchoolSearchModalbeing updated. Any other consumer, such as theuseComparisonhook, school-page compare buttons or the compare page, would fail the type-check or break at runtime. Grep for remainingcanAddMoreusages before merging.Review follow-up (
587cfe3):canAddMore: no remaining usages (grep is clean;tscpasses). Every add path was checked, and that turned up the next item.phase, with a test. The frontend treats the field as optional, so a frontend deployed ahead of the API falls back to the conservative both-groups count./comparebackfills missing phases from the data it already fetches (backfillPhases, which never overwrites an existing phase).16 plusnow counts as secondary, matching the backendPHASE_GROUPS.Verified: backend pytest 231 passed;
tscclean; Jest 57 suites / 486 tests passed, including a new provider suite (__tests__/context/ComparisonProvider.test.tsx).Note: this PR now touches
backend/nearby_schools.py, so the API image changes too. Staging needs both images deployed before the nearby phase is live.🤖 AI Code Review (Claude Code)
The compare basket limit changes from five overall to five per phase group (primary and secondary). Schools with an unknown or all-through phase count against both groups. The change adds a per-peer
phasefield to the nearby-schools API, a phase backfill for old stored baskets, updated copy and tests. The logic looks consistent and well covered, and I found no blocking problems.🟡 Minor
alert()is called inside thesetSelectedSchoolsupdater function. Updaters should be pure, and React StrictMode in development can invoke them twice, which would show duplicate alerts. This pattern already existed, so it is not a regression, but the new message is still emitted from the updater.