fix(navigation): wire the forms section anchors, drop the dead presentation set by BigSimmo · Pull Request #1697 · BigSimmo/Database

BigSimmo · 2026-08-07T13:55:30Z

Summary

Closes /issues #256 for its two remaining routes. Both claimed an information-page section set and drew no navigation at all: informationPageSectionDefinitions takes the route before the mode branch, AvailableInformationPageNavigation then drops every section whose targetIds are absent from the DOM and returns null when all are dropped — silently, with no error and no failing test. Specifiers and formulation were fixed in #1647; these were the two suspected remainders, now confirmed and handled differently because they are different problems.

Forms — wire the anchors. form-detail-page.tsx rendered zero id attributes, so all six declared sections resolved to nothing. Four map 1:1 onto sections that already existed and needed only an id. The two breakpoint-variant pairs reuse the wrappers already present on the mobile side and take single-child wrappers on the desktop side, so no component signature changes. form-source-verification-desktop anchors the source card rather than spanning it plus the two RailCards below — those are its siblings in the same scrollable rail, so landing there brings them into view without restructuring the aside.
Differentials presentations — delete the set. Three of its six sections declared a -mobile targetId that no render could ever satisfy: ReviewPanels renders twice (tablet and xl), not three times, so satisfying them would have meant inventing markup nobody asked for. The page already owns navigation at every width — MobileTabs below xl, and an always-visible aria-label="Differential review sidebar" aside at xl showing every panel at once — so hasLocalInformationPageNavigation now says that directly. Behaviour-preserving: the route drew nothing before and draws nothing now, minus the dead declaration. Without that entry it would instead have fallen through to the differentials mode bar on a submitted search, which is a surface nobody asked for.

The browser spec is the point, not a formality. The source-text binding guard added alongside it matches id="…" and cannot see whether an anchor sits in a branch that never renders; jsdom applies no Tailwind, so a DOM test cannot tell a -mobile target from its -desktop twin either. Only a real browser at a real width can — and it earned its place immediately: the first run failed because /forms/form-1, the placeholder the unit tests use for pure predicates, is not a real catalogue slug, so nothing rendered while every source-level assertion passed.

Registering that spec required both allowlists in playwright.config.ts — the global testMatch and the per-project productionSpecPattern. Adding it to only one leaves a spec that silently never runs, which is the same dead-config shape this PR exists to remove.

RAG impact: no retrieval behaviour change — navigation and layout only.

Verification

npm run verify:pr-local

Verification not run: aborts at check:installed-lock-parity (installed @playwright/test 1.62.0 vs locked 1.62.1). Not resolved with npm ci, and here that is not merely a preference — see the environment note below. Components run directly instead:

npm run lint — exit 0.
npm run typecheck — clean.
npm run test — Test Files 1 failed | 519 passed (520), Tests 1 failed | 5529 passed | 4 skipped (5534). The single failure is tests/pr-handoff-stop.test.ts, a shell-hook test untouched by this diff and confirmed pre-existing on this base by a stashed re-run.
npm run check:gate-manifest and npm run check:ci-scope — pass with the new spec registered.
ui-forms-section-nav + ui-accessibility — 18 passed. The forms spec asserts the nav renders on a real record (/forms/transport-crisis-form) with all six links, and that at 390px and 1280px exactly one of each -mobile/-desktop pair is visible — never both, never neither.
npm run format — run, no changes.

Mutation-checked: removing a single id from form-detail-page.tsx turns the binding guard red. The browser spec was also observed failing when the nav was genuinely absent (the wrong-slug run), so neither assertion passes vacuously — which matters, because a source assertion matching zero occurrences is exactly how #256 survived this long.

Environment note for reviewers. This container runs Node 24.13.0, and main's current lockfile requires ≥24.15 (jsdom@30), so npm ci refuses outright. src/lib/tailwind-merge.ts arrived on main in #1678 and its dependency was therefore not installed here. Rather than bypass the engine check, I materialised tailwind-merge@3.6.0 alone from its tarball into node_modules — no change to package.json or the lockfile — which is what let the typecheck and full suite above actually run. Flagging it so the numbers are read for what they are: a real run on this base, on a container whose dependency set could not be installed the normal way.

Risk and rollout

Risk: Low and contained. Forms gains navigation it declared but never drew; differentials presentations loses a declaration that never drew anything. No shared component signature changes.
Rollback: Single revert.
Provider or production effects: None.

Clinical Governance Preflight

Not applicable, verified against scripts/pr-policy.mjs rather than assumed: neither src/components/forms/form-detail-page.tsx nor src/components/page-secondary-navigation.tsx matches clinicalRiskPatterns — the presentation clause requires auth/permission/privacy/security/upload/download/patient in the path — and nothing matches ragRankingPatterns. Section anchors and layout wiring only; no ingestion, answer generation, ranking, source rendering, document access or privacy behaviour is touched.

Notes

Opened on its own branch rather than claude/handover-review-nlhuln: that branch had been picked up by another session mid-task and now carries an unrelated open PR (#1685) for outstanding-issues/ledger work. My commit had landed on top of those docs commits as a result, so this branch is a clean cherry-pick of the single fix onto current main — no duplicated docs commits, and what was verified is exactly what is under review.

Generated by Claude Code

…tation set Two clinical routes claimed an information-page section set and drew no navigation at all. `informationPageSectionDefinitions` takes the route before the mode branch, then `AvailableInformationPageNavigation` drops every section whose targetIds are absent from the DOM and returns null when all are dropped — silently, with no error and no failing test. That is /issues #256; specifiers and formulation were fixed in #1647, and these were the two suspected remainders, both now confirmed. Forms — wire the anchors. form-detail-page.tsx rendered zero `id` attributes, so all six declared sections resolved to nothing. Four map 1:1 onto sections that already existed and only needed an id. The two breakpoint-variant pairs use the wrappers already present on the mobile side, and single-child wrappers on the desktop side, so no component signature changes. `-desktop` for source/verification anchors the source card rather than spanning it and the two RailCards below: those are its siblings in the same scrollable rail, so landing there brings them into view without restructuring the aside. Differentials presentations — delete the set. Three of its six sections declared a `-mobile` targetId that no render could ever satisfy: ReviewPanels renders twice (tablet and xl), not three times, so faking them would have meant inventing markup nobody asked for. The page already owns navigation at every width — MobileTabs below xl, and an always-visible "Differential review sidebar" aside at xl showing every panel at once — so `hasLocalInformationPageNavigation` now says so directly. Behaviour-preserving: the route drew nothing before and draws nothing now, minus the dead declaration. Without that entry the route would have fallen through to the differentials mode bar on a submitted search, which is a surface nobody asked for. The browser spec is the point, not a formality. The source-text binding guard added alongside it matches `id="…"` and cannot see whether an anchor sits in a branch that never renders; jsdom applies no Tailwind, so a DOM test cannot tell a `-mobile` target from its `-desktop` twin either. Only a real browser at a real width can, and it earned its place immediately: the first run failed because `/forms/form-1` — the placeholder the unit tests use for pure predicates — is not a real catalogue slug, so nothing rendered while every source assertion passed. Registering the spec needed both allowlists in playwright.config.ts: the global `testMatch` and the per-project `productionSpecPattern`. Adding it to only one leaves a spec that silently never runs, which is the same dead-config shape being fixed here. Verification: lint exit 0; typecheck clean; check:gate-manifest and check:ci-scope pass with the new spec registered; full offline suite 518/519 files, the single failure (tests/pr-handoff-stop.test.ts) re-confirmed pre-existing on this base by a stashed re-run; ui-accessibility 16 passed; ui-forms-section-nav 2 passed against a real record at 390px and 1280px, with exactly one of each variant pair visible per width. The binding guard was mutation-checked — removing a single id turns it red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE

chatgpt-codex-connector · 2026-08-07T13:55:36Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

supabase · 2026-08-07T13:55:37Z

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

coderabbitai · 2026-08-07T13:55:39Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 17 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0e9dd26f-ea9e-459c-8973-88fbca760217

📥 Commits

Reviewing files that changed from the base of the PR and between 31cb5eb and 4c19050.

📒 Files selected for processing (6)

docs/branch-review-ledger.md
playwright.config.ts
src/components/forms/form-detail-page.tsx
src/components/page-secondary-navigation.tsx
tests/page-secondary-navigation.dom.test.tsx
tests/ui-forms-section-nav.spec.ts

_{Comment @coderabbitai help to get the list of available commands.}

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE

Copilot

Pull request overview

This PR closes /issues #256 by ensuring the Forms detail route actually renders the anchor ids that PageSecondaryNavigation’s section registry declares, and by removing the dead section-definition set for the Differentials presentation workflow (which owns its own navigation at all widths). It also adds a Playwright regression spec and updates the existing unit-level guards so the failure mode can’t silently return.

Changes:

Wire all six Forms “On this page” section anchors by adding the missing id attributes in FormDetailPage.
Delete the unused differentials presentation section definition set and explicitly mark that route family as locally-owned navigation (so PageSecondaryNavigation doesn’t claim it).
Add a browser-level Playwright spec for Forms section navigation and register it in playwright.config.ts; adjust the unit tests accordingly.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/ui-forms-section-nav.spec.ts	New Playwright spec proving Forms “On this page” renders on a real catalogue slug and breakpoint-variant anchors resolve to exactly one visible target per pair.
tests/page-secondary-navigation.dom.test.tsx	Updates DOM/unit guards: removes the differential presentations section-set expectations, asserts presentations now return `[]` + are locally owned, and adds a source-binding guard for Forms ids.
src/components/page-secondary-navigation.tsx	Removes the dead `differentials/presentations` section definitions and marks the route family as locally-owned navigation.
src/components/forms/form-detail-page.tsx	Adds the missing `id` anchors for all declared Forms section targets (including breakpoint-specific pairs).
playwright.config.ts	Registers the new `ui-forms-section-nav` spec in both the global matcher and production project matcher so it actually runs.
docs/branch-review-ledger.md	Appends a ledger row documenting the review scope and verification evidence for this PR.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…lready require jsdom@30 requires Node ^24.15.0, but engines.node said "24.x" and every gate compared majors only. A container on 24.13.0 therefore passed check:runtime and the preinstall hook, then failed `npm ci` with an opaque EBADENGINE for a transitive package. That blocked local verification on PRs #1611, #1697, #1705 and #1740, each worked around by hand. - package.json: engines.node "24.x" -> ">=24.15.0 <25", declaring the floor the dependency tree already imposes. This cannot newly break any environment that currently installs, since jsdom enforces the same floor during `npm ci`. - check-node-engine.cjs (npm preinstall): compare the full version against that range instead of the major, so the failure names the real requirement. Kept import-free — the Dockerfile COPYs this file alone before `npm ci` — with the range restated as a literal and pinned to package.json by test. - check-runtime.ts: add NODE_MINIMUM_VERSION and reject a matching major below it. This gate fronts verify:cheap, :ui, :release and :pr-local. - session-start.sh: provision 24.19.0, and make the install guard floor-aware. The old major-only test meant a cached container already on 24.13 skipped the install entirely, so bumping the pin alone would not have upgraded it. - check-codex-cloud-setup.mjs: accept the range form, still requiring its major to track .node-version. Closes #285. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZHZjz6kX8Ly1qBvvCg29C

…e claiming "0 commits ahead" when it isn't (#2117) * fix(claude): make the session-start hook runnable and give the agent config teeth `.claude/hooks/session-start.sh` was committed as mode 100644 while both its siblings were 100755, and it is the only hook registered by bare path rather than through `bash`. Its whole body is gated on CLAUDE_CODE_REMOTE=true, so the one environment it does any work in is a Linux web container — exactly where a non-executable checkout cannot run. It is also the script that provisions the Node 24 the engine floor requires, after npm ci EBADENGINE blocked PRs #1611, #1697, #1705 and #1740. The defect was invisible locally: the primary workstation is a Windows ReFS Dev Drive with core.fileMode=false, so git ignores filesystem permission bits and a local `chmod +x` is a silent no-op. Only `git update-index --chmod=+x` can fix it. Fixed three ways so it cannot recur: the index mode, a `bash "..."` registration that stops the mode being load-bearing, and a contract test. Also in this change: - .claude/settings.json gains a permissions block. AGENTS.md's provider confirmation boundary was prose-only; this encodes it as deny/ask rules, including deny on reading .env* (a staging key leaked on 2026-08-18) and on the Supabase MCP write tools. The repo already learned that prose does not hold here — see the comment in pr-handoff-stop.sh. - check-base-freshness.mjs now emits its stale-base warning on stdout as hook JSON. Every human-readable branch used console.error, and Claude Code injects only stdout into context, so the tripwire never reached the agent. The origin/main fetch also gains a 10s timeout so a hung remote cannot burn the whole SessionStart budget. - clean-worktree.mjs gains list-only `--merged` and `--squashed`. Nothing reclaimed merged worktrees, so 49 accumulated, ~19 GB of duplicated node_modules on a 50 GB Dev Drive. Ancestor detection alone finds 2 of 49 because this repo squash-merges; the patch-id test finds 9. A `confidence:` line distinguishes proven from inferred, because the two are not the same claim and one candidate had 2 of 21 files still differing. - Explicit hook timeouts, a PreCompact hook that asks for /issues capture while the context still exists, and a push format guard that only fires where the .githooks pre-push guard is not wired. Removal stays a separate opt-in throughout; `runWorktreeCleanup()` is byte identical, so verify:preflight is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(tests): stop the session-start hook test failing on every Windows run `tests/session-start-hook.test.ts` asserted that the written env file contains `join(home, ".node24", …)`. On Windows `home` comes from mkdtempSync(tmpdir()) as `C:\Users\…\AppData\Local\Temp\session-start-home-XXXX`, while the hook runs under Git Bash and writes the POSIX view of the same directory, `/tmp/ session-start-home-XXXX`. The assertion therefore failed on every Windows run regardless of the diff under test. That is worse than a red test: it is a red test everyone learns to ignore. It fails inside `npm run test`, which is the last step of `verify:pr-local`, so the whole gate goes red locally for every change and the only way to use it is to decide which failures do not count. Compare the path tail instead. The unique mkdtemp basename still pins the assertion to this test's own HOME, so it loses no strength, and it now holds on both platforms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the Claude Code environment review for PR #2113 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(claude): pin the permission boundary and make the PreCompact hook answerable Closes the four items left open when PR #2113 was opened, each with the smallest fix that actually resolves it rather than restating it. 1. The claim that no `allow` rule can reach a provider-backed script was asserted in review and never measured. It is now a test: 36 provider-backed scripts, derived from script names rather than hand-listed, each asserted unreachable through `allow` and covered by an explicit `ask`. Mutation-checked — injecting a broad `Bash(npm run check:*)` allow rule turns 4 of them red. 2. Hook registrations are pinned to invoke through an interpreter rather than a bare path, and to carry an explicit timeout. Bare-path registration is what made the `session-start.sh` mode bug reachable; the mutation check confirms both guards bite. 3. The PreCompact hook's known limit could not be closed by reading code — the installed CLI ships a compiled binary with no inspectable bundle, so whether the platform injects its stdout into model context is not determinable here. Instead of leaving that permanently unverified, the hook now appends one line per firing to a log under the git dir. After the next compaction the log distinguishes "hook never ran" from "hook ran but its output went nowhere", and both answers are actionable. The log lives outside the worktree so it can never be staged. 4. The `newtask` skill said "~40 worktrees". It was 48 on 2026-08-18 and reached 50 during one session, so the number now carries the reason it drifts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(issues): queue the five follow-ups this session could not close Each of these is blocked on something outside the repo, so capturing them is the smallest fix that resolves them — the alternative is that they die with the session context. - P1 rec: PR churn has exhausted both review bots' budgets, so PR #2113 landed with zero automated review and subsequent PRs will too. AGENTS.md already measured the CI half of this cost; this is the second bill and the worse one. Needs a decision on whether the bundling rule gets a gate. - P2 task: nine landed worktrees, ~4.5 GB on a 51%-full Dev Drive, deliberately not removed — re-verification immediately before deletion showed two held unmerged commits despite the scan reporting none minutes earlier, and a third had been switched branches mid-scan by a live session. - P3 task: confirm D:\.npm-cache is a registered Dev Drive trusted cache; fsutil needs elevation and the non-elevated registry fallback reads empty. - P3 task: read the PreCompact hook's own log after the next compaction to settle whether its output reaches model context. The log lives under the worktree's git dir, so check it before cleaning that worktree up. - P3 task: confirm on a real web session that session-start.sh now runs. The mode bug was proven; the failure it would cause on Linux was not, because no container was available. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(clean-worktree): stop the candidate listing claiming "0 commits ahead" when it isn't Every `--squashed` candidate printed "0 commits ahead" while being genuinely ahead of origin/main — by 11, 2, 1 commits on the real fleet. A squash-merged branch keeps its original commits forever, so it stays ahead permanently; what is zero is the count of UNLANDED commits, which is a different claim. The line was therefore stating something a reader could disprove with one `git rev-list`, on a tool whose entire job is to be trusted before a deletion. It now reports both numbers: "11 ahead of origin/main, 0 unlanded commits". Also corrects the comment above the ahead check, which described it as belt-and-braces against the merge test. That is true in ancestor mode, where it is the real gate. In squash mode it is not a second opinion at all — gitAheadUnlandedCount returns 0 for any branch the squash test just accepted, so the check is satisfied by construction and can only fire on a candidate that was already skipped. Keeping it is correct; describing it as independent evidence was not. The raw count is reporting only and never gates, so no candidate set changes. Verified against the live fleet: same 9 candidates before and after, worktree count unchanged, --remove not run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the follow-up review for PR #2117 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

docs(ledger): record the #256 section-nav fix review (PR #1697)

8a548b2

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE

BigSimmo requested a lite review from Copilot August 7, 2026 13:59

Copilot started reviewing on behalf of BigSimmo August 7, 2026 14:00 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

merge(main): sync PR #1697 with origin/main

2b99db1

BigSimmo enabled auto-merge (squash) August 7, 2026 14:15

BigSimmo disabled auto-merge August 7, 2026 14:24

Merge branch 'main' into claude/issues-256-section-nav-clean

4c19050

BigSimmo enabled auto-merge (squash) August 7, 2026 14:29

BigSimmo merged commit d32dd54 into main Aug 7, 2026
24 of 25 checks passed

BigSimmo deleted the claude/issues-256-section-nav-clean branch August 7, 2026 14:41

BigSimmo mentioned this pull request Aug 7, 2026

fix(ci): pin Lighthouse Chromium and stop advisory Chrome-drift fails #1703

Closed

3 tasks

BigSimmo mentioned this pull request Aug 9, 2026

fix(runtime): declare and enforce the Node 24.15 floor dependencies already require #1771

Merged

BigSimmo mentioned this pull request Aug 18, 2026

fix(claude): make the session-start hook runnable, and give the agent config teeth #2113

Merged

Navigation Menu

fix(navigation): wire the forms section anchors, drop the dead presentation set - #1697

fix(navigation): wire the forms section anchors, drop the dead presentation set#1697
BigSimmo merged 4 commits into
mainfrom
claude/issues-256-section-nav-clean

BigSimmo commented Aug 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 7, 2026

Uh oh!

supabase Bot commented Aug 7, 2026

Uh oh!

coderabbitai Bot commented Aug 7, 2026 •

edited

Loading

Review limit reached

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

BigSimmo commented Aug 7, 2026

Summary

Verification

Risk and rollout

Clinical Governance Preflight

Notes

Uh oh!

chatgpt-codex-connector Bot commented Aug 7, 2026

Uh oh!

supabase Bot commented Aug 7, 2026

Uh oh!

coderabbitai Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderabbitai Bot commented Aug 7, 2026 •

edited

Loading