feat(services): adopt default in-page nav on service detail - #1767
Closed
BigSimmo wants to merge 2 commits into
Closed
feat(services): adopt default in-page nav on service detail#1767BigSimmo wants to merge 2 commits into
BigSimmo wants to merge 2 commits into
Conversation
Replace the shell pill rail and breadcrumb row on /services/[slug] with the shared DocumentViewer-style in-page nav: back control, section track, and ellipsis actions for Save/Close. Keep the in-body h1 and mark the route as locally owned so the header addon slot stays single-occupant. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Contributor
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Contributor
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #9427 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo added a commit
that referenced
this pull request
Aug 9, 2026
…ree items (#1773) * docs: pin PageSection ownership to the nav-header sibling, capture three 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 * docs: fix the placeholder-pattern reference, wire the duplicate-work 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 * docs: address PR review — fix the quarantine sequencing and the ledger-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 --------- Co-authored-by: Claude <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/services/[slug]onto sharedInPageNavHeader(back, title + active section, ellipsis actions, weighted track, phone collapse portal).InformationPageBreadcrumbs(Services › title) and move Save/Close into the header actions sheet; keep the large in-body<h1>.Clinical Governance Preflight
Test plan
service-detail-page,page-secondary-navigation,mode-nav-addon-slot,in-page-nav-headerDOM testsnpm run test— 541 passed (5799 tests, 4 skipped)npm run verify:phone-chrome—84 passed(ui-tools.spec.ts) and7 passed(ui-phone-scroll-page-owned.spec.ts)RAG impact: no retrieval behaviour change — services detail chrome/navigation only.