docs: pin PageSection ownership to the nav-header sibling, capture three items - #1773
Conversation
…ree items Record the in-page-nav decision in docs/search-chrome-behaviour.md under "Default in-page navigation template": a page's PageSection[] table is always owned and exported by a colocated "use client" nav-header sibling, never declared inline in the page and never in a separate per-route section-index module. The four Server Component pages need the sibling regardless, since neither onSelectSection nor a LucideIcon crosses the RSC boundary. Applying the same shape to Client Component pages gives one answer to where the section table lives and one import path for tests/in-page-nav-route-sections.dom.test.tsx. PR #1766 shipped both shapes; the closed PR #1767 proposed a third. States explicitly that existing pages are not being migrated — the rule binds new conversions only, and the grandfathered sites are listed so the inconsistency is not read as a bug. Ledger: - #291 (P2 issue) disabled placeholder buttons pair `disabled` with an aria-describedby sr-only reason; `disabled` removes the tab stop, so the reason is never announced. Needs one repo-wide pass across the sites, docs/wiring-conventions.md and eslint-rules/require-button-wiring.mjs. - #292 (P2 rec) two sessions built the same queued item four hours apart (PR #1766, closed PR #1767); check the open-PR list for the affected route before starting a queued item, not just the ledger row. - #290 extended with the quarantine arithmetic: tests/flake-ledger.json is empty, so two more same-SHA reproductions are needed before a quarantine entry is admissible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request updates the outstanding-issue ledger and documents placement rules for static ChangesOutstanding issue ledger
Navigation documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f5f920b99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/outstanding-issues.md`:
- Line 337: Update the “ledger row records…” sentence in issue `#292` to
acknowledge existing statuses such as IN PROGRESS and IMPLEMENTED, while stating
that the ledger lacks an atomic claim or reliable ownership marker preventing
another session from starting the same work. Preserve the surrounding incident
details and distinction from collision-on-id issues.
- Line 336: Update the outstanding issue entry to limit its accessibility claim
to sequential-keyboard and focus-mode users, removing the unsupported assertion
that every browser/assistive-technology pairing removes disabled controls from
the accessibility tree. Preserve the distinction that genuinely unavailable
controls retain disabled, while focusable placeholders use aria-disabled="true"
with their existing explanation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d19119b-377b-4ee5-8f34-8ae5d927e27f
📒 Files selected for processing (2)
docs/outstanding-issues.mddocs/search-chrome-behaviour.md
…check Implements the smallest correct fix for the two recommendations that had one, and corrects a recommendation that turned out to be wrong. #291 — corrected, not implemented as written. The row claimed that pairing `disabled` with an aria-describedby sr-only reason means the reason is never announced, and that the docs and eslint rule blessed the pairing so a repo-wide pass was needed. All three claims are refuted. A natively disabled button stays in the accessibility tree with its accessible description intact; tests/favourites-hub-unavailable-controls.dom.test.tsx pins toBeDisabled(), not.toHaveAttribute("aria-disabled") and toHaveAccessibleDescription() together under the title "keeps unavailable actions natively disabled and exposes their reasons" — a deliberate, tested decision. require-button-wiring already accepts both forms. An attempt to swap the attribute in favourites-hub and four other sites was reverted for contradicting that decision. The one real defect was documentation: docs/wiring-conventions.md named favourites-hub.tsx as "the reference markup" while showing aria-disabled="true", which is not what that file does. The canonical block now shows native `disabled`, explains why the description survives it, cites the pinning test, and says when aria-disabled + a no-op handler is correct instead. It also records that `disabled` + `aria-disabled` together is a third shape pinned two contradictory ways in one test file, which stays open as the residue of #291. #292 — mitigation implemented. The open-PR check before starting a queued item is now in newtask's "Before you start" (which already made that GitHub read for PR bundling, so it asks the same list a second question at no extra cost), in the issues skill after the read-only flow, and in the /issues section of AGENTS.md so Codex and Cursor get it too rather than Claude Code only. All three scan for the route/component/surface rather than the ledger id, and degrade to a warning when GitHub is unreachable. #290 — not implementable here: it needs two more same-SHA Playwright reproductions, and this container has no node_modules or browsers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK
…r-status claim Two valid review findings on #1773, both verified against the repo before acting. Codex (P2, docs/outstanding-issues.md #290): the "Next" step added in 16b6119 told the reader to append each of the next two reproductions to tests/flake-ledger.json. Following it cannot produce a valid ledger state. docs/testing.md requires three same-SHA reproductions before an entry is added or retained, and scripts/flake-ledger.mjs:48 throws "exact title must include @quarantine" while tests/ui-smoke.spec.ts:4470 is untagged — so an entry written today fails validation outright, and appending per run treats the ledger as a per-run log. The step now says to record the two observations in the row itself, then after the third reproduction tag the test @quarantine and add one complete entry with owner, repro command, tracking ref, first/last-seen and expiry. CodeRabbit (docs/outstanding-issues.md #292): the claim that the ledger "has no in-progress state" is wrong — IN PROGRESS appears on two rows and IMPLEMENTED in PR #1766 on another. The accurate gap is narrower and worse: there is no structured status field and no atomic claim, so a marker is written by whoever did the work, usually after the fact, and nothing requires or checks one, which means the absence of a marker carries no information. Corrected in all four places that carried the overstatement, not only the flagged row — AGENTS.md, the issues and newtask skills, and #292 itself — since the same sentence had been copied into each. CI on the previous head was green; no check failures were outstanding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK
2 participants
Summary
Docs only. No source changes.
Record the
PageSection[]ownership decision indocs/search-chrome-behaviour.md, under the "Default in-page navigation template" section. The rule: a page'sPageSection[]table is always owned and exported by a colocated"use client"nav-header sibling (for examplesrc/components/specifiers/specifier-nav-header.tsx) — never declared inline in the page, and never in a separate per-route section-index module.The rationale is recorded alongside the rule. The four Server Component pages (
specifier-record-page,specifier-reference-page,formulation-mechanism-page,dsm-diagnosis-page) cannot receiveonSelectSectionor aLucideIconacross the RSC boundary, so they need the sibling regardless. Making it the rule for Client Component pages too gives one answer to "where does the section table live" and one import path fortests/in-page-nav-route-sections.dom.test.tsx, which imports every route's sections to assert anchors against rendered DOM. PR feat(in-page-nav): convert the six information routes onto the shared header #1766 shipped both shapes and the closed PR feat(services): adopt default in-page nav on service detail #1767 proposed a third; this pins that drift.The entry states explicitly that existing pages are not being migrated — the rule binds new conversions only — and lists the grandfathered sites (
serviceNavSections,formNavSections,dsmDifferentialNavSectionsinline;differentials/detail-section-index.tsas a module) so the inconsistency is not read as a bug. It also separates the one genuine on-the-merits exception:differentials/detail-section-index.tsbuilds sections per record viabuildDifferentialSectionIndexand is typedDocumentSection, so there is no static table for the rule to place.Capture
#291(P2, issue): disabled placeholder buttons announce their reason to nobody. The documented placeholder pattern pairsdisabledwith anaria-describedbypointing at ansr-onlyreason.disabledremoves the tab stop, so the description is never announced to keyboard or screen-reader users. Confirmed by inspection inclinical-dashboard/favourites-hub.tsxat :367, :382 and :544 — the file the docs cite as the reference implementation, so the reference itself carries the defect. A baredisabledalongside a coming-soon note appears in nine files undersrc/components. Recorded as needing one repo-wide pass that updates the placeholder sites,docs/wiring-conventions.md/AGENTS.md, andeslint-rules/require-button-wiring.mjstogether, because fixing one instance in isolation would diverge from the rule and docs that currently bless the broken pairing.Capture
#292(P2, rec): duplicated queued work. Two assistants took the same queued/issuesitem roughly four hours apart and independently built the same conversion (PR feat(in-page-nav): convert the six information routes onto the shared header #1766 merged, PR feat(services): adopt default in-page nav on service detail #1767 closed as duplicate); neither workflow could see the other, because a ledger row records that work is wanted, never that it is underway. Suggested mitigation recorded: check the open-PR list for the affected route before starting a queued item, not just the ledger row. Distinguished from#156/#168, which are about id collisions rather than work collisions.Extend
#290with the quarantine arithmetic rather than opening a duplicate row, since#290already tracks theui-smokephone scroll-hide flake.tests/flake-ledger.jsonis currently empty, so the flake has zero recorded reproductions;docs/testing.mdrequires three on the same SHA, and the 2026-08-09 measurement is one. Net: two more same-SHA reproductions are needed before a quarantine entry is admissible. The row's existing evidence is preserved in full.Verification
npm run verify:pr-localclassifies this diff as recognised low-risk docs scope (dry run): lint, typecheck, the full unit suite, RAG fixture scan, build, and offline RAG checks are all skipped as not applicable.The full gate could not complete in this container — it has no
node_modules, andnpm ci --include=devfails becausejsdom@30.0.1requires Node^24.15.0while only 24.13.0 is available. Bypassing that with an engine-override flag is explicitly disallowed without approval, so the dependency-free planned gates were run individually instead:npm run check:outstanding-issues—Outstanding-issues guard passed: 290 rows (146 open, 144 archived), unique ids, next-id=293 above the highest, no merge driver, no ids deleted from base 73d09660cc62.npm run docs:check-links—docs link check passed: 1700 repo path references resolve.npm run docs:check-index—docs/codebase-index.md coverage OK: all 50 repository roots/modules/routes and all schema tables are indexed.npm run docs:check-inventory—Docs inventory current: 224 script files, 235 npm scripts.npm run docs:check-scripts—docs script-ref check passed: 452 npm-run reference(s) resolve to real scripts.All matched files use Prettier code style!(docs/outstanding-issues.mdis Prettier-ignored by design;docs/search-chrome-behaviour.mdis checked and clean.)Not run, with reasons:
check:runtimeandsitemap:check— both go throughscripts/run-tsx.mjs, andtsxis unavailable withoutnode_modules. No route was added or changed, so the sitemap is unaffected.format:changed— fails withCannot find module '.../node_modules/prettier/bin/prettier.cjs', an install gap rather than a formatting result. Its actual check was performed directly with Prettier 3.9.6 as noted above.check:branch-review-ledger— fails onmerge.ledger.driver is not installed, local git config thatnpm installsets up. This PR does not touchdocs/branch-review-ledger.md.npm run verify:ui— no UI, routing, styling, or browser behaviour changed.One environment caveat worth flagging for anyone reproducing this locally: with
node_modulesabsent,npm run formatsilently resolvesprettierfromPATH, which on this container is a stray 3.8.1 from a Node 22 install rather than the repo-pinned 3.9.6. Running it reformatted 26 unrelated source files in a way 3.9.6 then rejects. Those changes were reverted and are not in this PR, which contains only the two documentation files. This is an environment trap, not a repo defect — the two versions simply disagree.Risk and rollout
Notes
The
PageSectionentry is a decision record, not a migration. Nothing in this PR changes any page's current structure, and the grandfathered sites are named in the doc precisely so a future reader does not open a cleanup PR against them.🤖 Generated with Claude Code
https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK
Generated by Claude Code
Summary by CodeRabbit