1. Blank compare page / empty trends chart (the bug you reported)
Opening a compare link while localStorage held a different basket raced a fetch for the old school set against the URL's SSR data. The stale response replaced comparisonData, leaving the active schools with no data — every section, including the Explore Trends chart, vanished until a hard refresh. (Reproduced live on staging: chips and phase tabs rendered, comparisonData held the previous URL's schools, and no fetch ever fired for the new set.)
Fix: responses from superseded effect runs are dropped (cleanup flag), successful responses merge into existing data instead of replacing it, and the URL-seed effect re-runs when a client-side navigation changes ?urns=. Covered by a new Jest regression test that fails on the old code, plus a new e2e journey that opens one comparison and then a different compare link.
2. Sticky-bar rail caption
The label rail freed up by the chip-alignment fix now carries "COMPARING / 3 primary schools" (phase-aware, pluralised) — 0.72rem uppercase eyebrow over a 0.95rem semibold count, sized against the 0.92rem chip names. Desktop only; mobile pills unchanged.
Two follow-ups to the compare-screen work:
## 1. Blank compare page / empty trends chart (the bug you reported)
Opening a compare link while localStorage held a **different** basket raced a fetch for the old school set against the URL's SSR data. The stale response replaced `comparisonData`, leaving the active schools with no data — every section, including the Explore Trends chart, vanished until a hard refresh. (Reproduced live on staging: chips and phase tabs rendered, `comparisonData` held the previous URL's schools, and no fetch ever fired for the new set.)
Fix: responses from superseded effect runs are dropped (cleanup flag), successful responses **merge** into existing data instead of replacing it, and the URL-seed effect re-runs when a client-side navigation changes `?urns=`. Covered by a new Jest regression test that fails on the old code, plus a new e2e journey that opens one comparison and then a different compare link.
## 2. Sticky-bar rail caption
The label rail freed up by the chip-alignment fix now carries "COMPARING / 3 primary schools" (phase-aware, pluralised) — 0.72rem uppercase eyebrow over a 0.95rem semibold count, sized against the 0.92rem chip names. Desktop only; mobile pills unchanged.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
'COMPARING / 3 primary schools' — 0.72rem uppercase eyebrow over a
0.95rem semibold count, sized to sit alongside the 0.92rem chip names
without dominating. Desktop only; mobile pills unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
Opening a compare link while localStorage held a different basket raced a
fetch for the OLD school set against the URL's SSR data; the stale
response replaced comparisonData, so no active school had data and every
section (including the trends chart) vanished until a hard refresh.
Responses from superseded effect runs are now dropped, successful ones
merge instead of replace, and the URL-seed effect re-runs when a
client-side navigation changes ?urns=.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
This PR fixes a real regression where a stale localStorage-basket fetch could clobber a freshly-loaded comparison, and adds a caption to the sticky bar's label rail plus matching tests. The cancellation-flag guard correctly fixes the described race, but the accompanying switch from replacing to merging comparisonData introduces a smaller, new staleness issue.
🟡 Minor
nextjs-app/components/ComparisonView.tsx: The fetch resolve handler now merges into comparisonData ({...prev, ...data.comparison}) instead of replacing it. Previously a successful fetch replaced the whole map with only the current selection's data, so removing then re-adding the same school later in a session guaranteed a fresh refetch. Now the old entry for that URN lingers indefinitely, so the pre-existing 'covered' check (have[urn] != null) will consider it up to date and skip refetching, potentially showing stale Ofsted/attainment data for a re-added school. This also grows comparisonData unboundedly over a long session. The cancelled flag added in this same diff already prevents the specific stale-response race the tests target, making the merge-vs-replace change an unnecessary side effect with a downside rather than a required part of the fix.
## 🤖 AI Code Review (Claude Code)
This PR fixes a real regression where a stale localStorage-basket fetch could clobber a freshly-loaded comparison, and adds a caption to the sticky bar's label rail plus matching tests. The cancellation-flag guard correctly fixes the described race, but the accompanying switch from replacing to merging comparisonData introduces a smaller, new staleness issue.
### 🟡 Minor
- **nextjs-app/components/ComparisonView.tsx**: The fetch resolve handler now merges into comparisonData (`{...prev, ...data.comparison}`) instead of replacing it. Previously a successful fetch replaced the whole map with only the current selection's data, so removing then re-adding the same school later in a session guaranteed a fresh refetch. Now the old entry for that URN lingers indefinitely, so the pre-existing 'covered' check (`have[urn] != null`) will consider it up to date and skip refetching, potentially showing stale Ofsted/attainment data for a re-added school. This also grows comparisonData unboundedly over a long session. The `cancelled` flag added in this same diff already prevents the specific stale-response race the tests target, making the merge-vs-replace change an unnecessary side effect with a downside rather than a required part of the fix.
Review finding: merging left a re-added school's old entry in the map
forever, so the covered check served stale data and the map grew
unboundedly. The cancelled flag already discards superseded responses,
which is the actual race fix; replacing keeps the map bounded to the
current selection and guarantees fresh refetches.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
tudor
merged commit 163b501be6 into main2026-07-17 06:28:28 +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.
Two follow-ups to the compare-screen work:
1. Blank compare page / empty trends chart (the bug you reported)
Opening a compare link while localStorage held a different basket raced a fetch for the old school set against the URL's SSR data. The stale response replaced
comparisonData, leaving the active schools with no data — every section, including the Explore Trends chart, vanished until a hard refresh. (Reproduced live on staging: chips and phase tabs rendered,comparisonDataheld the previous URL's schools, and no fetch ever fired for the new set.)Fix: responses from superseded effect runs are dropped (cleanup flag), successful responses merge into existing data instead of replacing it, and the URL-seed effect re-runs when a client-side navigation changes
?urns=. Covered by a new Jest regression test that fails on the old code, plus a new e2e journey that opens one comparison and then a different compare link.2. Sticky-bar rail caption
The label rail freed up by the chip-alignment fix now carries "COMPARING / 3 primary schools" (phase-aware, pluralised) — 0.72rem uppercase eyebrow over a 0.95rem semibold count, sized against the 0.92rem chip names. Desktop only; mobile pills unchanged.
🤖 Generated with Claude Code
https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
🤖 AI Code Review (Claude Code)
This PR fixes a real regression where a stale localStorage-basket fetch could clobber a freshly-loaded comparison, and adds a caption to the sticky bar's label rail plus matching tests. The cancellation-flag guard correctly fixes the described race, but the accompanying switch from replacing to merging comparisonData introduces a smaller, new staleness issue.
🟡 Minor
{...prev, ...data.comparison}) instead of replacing it. Previously a successful fetch replaced the whole map with only the current selection's data, so removing then re-adding the same school later in a session guaranteed a fresh refetch. Now the old entry for that URN lingers indefinitely, so the pre-existing 'covered' check (have[urn] != null) will consider it up to date and skip refetching, potentially showing stale Ofsted/attainment data for a re-added school. This also grows comparisonData unboundedly over a long session. Thecancelledflag added in this same diff already prevents the specific stale-response race the tests target, making the merge-vs-replace change an unnecessary side effect with a downside rather than a required part of the fix.