Skip to content

mockups(record-pages): three breadcrumb header directions - #1769

Merged
BigSimmo merged 13 commits into
mainfrom
claude/breadcrumb-header-mockups-cei6lw
Aug 9, 2026
Merged

mockups(record-pages): three breadcrumb header directions#1769
BigSimmo merged 13 commits into
mainfrom
claude/breadcrumb-header-mockups-cei6lw

Conversation

@BigSimmo

@BigSimmoBigSimmo commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Adds /mockups/breadcrumb-header, a design-scratch study of three sticky header directions for record pages that use InformationPageBreadcrumbs and have no in-page section index — factsheets, services, forms, DSM, specifiers, formulation, medications. InPageNavHeader stays the documented default for in-page navigation; it is the wrong shape for these eight pages, because with no sections its title disclosure opens a sheet listing one item and its weighted track renders one full-width segment. What those pages actually carry is a breadcrumb row followed by a wrapping toolbar, and on a 390 px phone the factsheet detail page spends three bands of chrome before the first line of content with none of it sticky, so the way back scrolls away with it.
  • The three directions keep the document header's row grammar (back control, title, ellipsis actions, one scroll owner) and replace the section machinery: 01 Crumb rail is identity and return only with every action in the sheet; 02 Action rail adds one promoted primary action pill, icon-only on a phone and labelled from sm; 03 Crumb rail + mode adds a segmented view mode in the exact slot the document header gives DocumentSectionTrack, which is what the factsheet reading level actually is. All three portal into #phone-header-collapse-addon-slot below sm so they hide with the universal chrome under a single collapse owner, stick from sm, keep the page's <h1> in the hero so the header title stays a <span>, and share one actions sheet.
  • Suppresses shared mockup chrome for the route in src/app/mockups/mockups-layout-client.tsx, because each frame draws its own universal phone header and the shared header would read as a second real header over the comparison.
  • Regenerates docs/site-map.md for the new route and records the study in mockups/README.md.
  • No component is extracted and no production page is changed. This is scratch for design review.

Verification

  • npm run verify:pr-local

Verification not run: verify:pr-local was not run in this container. The diff is design-scratch plus a generated doc, and the container's node_modules was missing tailwind-merge, so npm run typecheck failed identically on a clean tree before any of these changes. Repairing that one package let the real gates run, and the smallest checks covering this diff were run directly instead:

  • npm run typecheck — clean, no output after the two script banners (it previously failed with src/lib/tailwind-merge.ts(1,37): error TS2307, pre-existing on a clean tree).
  • npx eslint on the three changed source files — no findings.
  • npx prettier --check on all five changed files — All matched files use Prettier code style!
  • npm run sitemap:check — passed, no drift.
  • npx vitest run tests/site-map.test.ts tests/production-mockup-boundary.test.ts tests/env-mockups-gate.test.ts tests/docs-inventory.test.ts tests/route-reachability.test.tsTest Files 5 passed (5), Tests 23 passed (23).

UI verification not run: the changed routes are design-scratch that 404 in production, and no shared UI foundation, production route, or shell component was touched. The one non-mockup-shaped edit is a pathname conditional inside the mockups layout client, which is itself mockup-only.

  • npm run verify:ui when UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changed
  • npm run verify:release before release or handoff confidence claims

Risk and rollout

  • Risk: Very low. src/app/mockups/** and *-mockups.tsx are design-scratch that 404 in production, and the only shared file touched is the mockups layout client, whose change is guarded by an exact pathname match on the new route. Mockups are exempt from the wiring and reachability gates but are still compiled and still count toward check:bundle-budget's repo-wide total, so the realistic failure mode for this PR is Build on bundle budget rather than any runtime behaviour.
  • Rollback: git revert the single commit. Nothing depends on the new route, and no production module imports the new component.
  • Provider or production effects: None.

Notes

  • InformationPageBreadcrumbs currently has eight call sites, which is the reuse this study is sized against. If a direction is adopted, the recommendation in the study is to build the 02 shape and let 01 and 03 fall out of omitting or adding one prop, rather than shipping three components.
  • The moment one of these pages grows real sections it should adopt InPageNavHeader instead of extending this header — no section index, no measurement hook, no weighted track belongs here.

Generated by Claude Code

Summary by CodeRabbit

  • New Features

    • Factsheet detail pages now use a consistent breadcrumb header with back navigation, save and PDF actions, reading-level controls, and responsive view modes.
    • Header actions and navigation adapt to mobile layouts.
    • Reading-level options update factsheet content where multiple levels are available.
  • Bug Fixes

    • Improved factsheet save-state persistence and action-sheet behavior.
    • Adjusted sidebar positioning to remain aligned with the updated header.
  • Documentation

    • Documented breadcrumb header behavior, supported page patterns, design-system adoption, and related mockup guidance.

Record pages that use InformationPageBreadcrumbs have no in-page section
index, so the repository's default in-page navigation header is the wrong
shape for them: its title disclosure opens a sheet listing one item and
its weighted track renders one full-width segment. What those pages do
have is a breadcrumb row followed by a wrapping toolbar — on a 390px
phone the factsheet detail page spends three bands of chrome before the
first line of content, and none of it is sticky.
Adds /mockups/breadcrumb-header, a design-scratch study of three headers
that keep the document header's row grammar (back, title, ellipsis, one
scroll owner) and drop the section machinery:
01 Crumb rail identity and return only; actions in the sheet
02 Action rail + one promoted primary action pill
03 Crumb rail + mode + a segmented view mode in the track slot
All three portal into the phone collapse slot below sm and stick from
sm, keep the page's h1 in the hero, and share one actions sheet. No
component is extracted yet — this is scratch for design review.
Shared mockup chrome is suppressed for the route because each frame
draws its own universal phone header.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBAt8pJVz2TxMEUJeWEdUy
@supabase

supabaseBot commented Aug 9, 2026

Copy link
Copy Markdown

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

coderabbitaiBot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: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 @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: 0d6204d8-ca56-4a2e-95dc-7b280463e4b5

📥 Commits

Reviewing files that changed from the base of the PR and between bcf2071 and 0f5d863.

📒 Files selected for processing (1)
  • docs/branch-review-ledger.md
📝 Walkthrough

Walkthrough

InPageNavHeader now supports sectionless breadcrumb layouts with optional back labels, primary actions, and view-mode controls. FactsheetDetailPage adopts this header for navigation, reading levels, saving, and PDF access. Tests, route ownership, and documentation were updated.

Changes

Factsheet breadcrumb header

Layer / File(s)Summary
Sectionless header contract and behavior
src/components/in-page-nav/in-page-nav-header.tsx, tests/in-page-nav-header.dom.test.tsx
InPageNavHeader accepts optional section navigation, back-label visibility, primary actions, and segmented view modes. Tests cover rendering, accessibility, activation, focus order, and omission states.
Factsheet detail header integration
src/components/factsheets/factsheet-detail-page.tsx, src/components/mode-nav/header-addon-slot.ts, tests/factsheet-detail-header.dom.test.tsx, tests/factsheet-save.dom.test.tsx, tests/helpers/phone-scroll.ts, tests/mode-nav-addon-slot.dom.test.tsx
Factsheet details use the shared header for navigation, reading levels, saving, and PDF download. Route ownership, sticky offsets, persistence behavior, storage failures, and phone navigation coverage were updated.
Header adoption documentation and records
docs/search-chrome-behaviour.md, mockups/README.md, docs/branch-review-ledger.md, docs/design-system/COMPONENTS.md, docs/design-system/adoption-manifest.json
Documentation records the breadcrumb shape, factsheet adoption, mockup study, review history, and updated design-system adoption data.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
participant Browser
participant FactsheetDetailPage
participant InPageNavHeader
participant FactsheetActions
Browser->>FactsheetDetailPage: open factsheet detail
FactsheetDetailPage->>InPageNavHeader: render breadcrumb header
InPageNavHeader->>FactsheetActions: expose reading-level, save, and PDF actions
FactsheetActions-->>FactsheetDetailPage: update content or save state
Loading

Possibly related PRs

Suggested labels:codex

Suggested reviewers:cursoragent

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check nameStatusExplanationResolution
Title check⚠️ WarningThe title describes a mockup study, but the changeset primarily updates the production factsheet header, shared navigation component, and related tests.Retitle the PR to describe the factsheet detail header and InPageNavHeader changes.
Description check⚠️ WarningThe description focuses on a mockup route and claims no production component changed, but the changeset includes substantial production header and test updates.Rewrite the description to document the factsheet header implementation, API changes, tests, verification, and associated documentation updates.
Docstring Coverage⚠️ WarningDocstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/breadcrumb-header-mockups-cei6lw

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

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/components/breadcrumb-header-mockups.tsx`:
- Line 422: Replace the arbitrary width, height, radius, and max-width utilities
in the mockup frame elements around the affected JSX with semantic Tailwind
token utilities. Define the corresponding mockup frame and content-width tokens
in globals.css using the existing `@theme` conventions, then reference those
tokens at the elements near the affected mockup sections instead of hardcoded
values.
🪄 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: 3577687e-eebd-4f6c-b7a0-b57c659d3142

📥 Commits

Reviewing files that changed from the base of the PR and between 883e725 and 003d3cd.

📒 Files selected for processing (6)
  • docs/branch-review-ledger.md
  • docs/site-map.md
  • mockups/README.md
  • src/app/mockups/breadcrumb-header/page.tsx
  • src/app/mockups/mockups-layout-client.tsx
  • src/components/breadcrumb-header-mockups.tsx

Comment threadsrc/components/breadcrumb-header-mockups.tsx Outdated

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:8effa5abe7

ℹ️ 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".

Comment threadsrc/components/breadcrumb-header-mockups.tsx Outdated
Comment threadsrc/components/breadcrumb-header-mockups.tsx Outdated
@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch claude/breadcrumb-header-mockups-cei6lw at starting commit 003d3cd; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:claude/breadcrumb-header-mockups-cei6lw, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit:003d3cdc4c

ℹ️ 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".

The factsheet detail page carried a breadcrumb row plus a toolbar that
wrapped to three bands on a phone — roughly 250px before the first line
of content — and none of it stuck, so the way back scrolled away with
it. This replaces that with the header direction chosen from the
/mockups/breadcrumb-header study.
Extends InPageNavHeader rather than adding a third header copy: it
already branched to a plain title when a page had no sections, and
usePageSectionWeights observes nothing for an empty list, so the
breadcrumb shape costs those pages no measurement. Four optional props,
every default preserving today's behaviour so the differentials adopter
is unchanged:
sections? omit to drop the disclosure, section sheet and track
primaryAction one promoted action, Button variant="secondary"
mode a page-level view mode via the shared SegmentedControl
showBackLabel false keeps the arrow alone so the title owns the row
The ellipsis drops to a flat face only when a primary action is present:
`secondary` is stylistically identical to its bordered face, so beside
one the row would read as two equal buttons.
Reading level rides the `mode` slot — it is how the page renders, not
where you are in it — and only `medRich` sheets (1 of 8) get it. Below
`sm` it wraps to its own full-width band; from `sm` it is inline and
costs no extra height. Measured on /factsheets/sertraline: 131px phone,
75px from sm, 65px with no mode.
`layout="fit"` there is load-bearing. `equal` gives each segment an 8rem
floor sized for a full-width group, but `sm:w-auto` makes the group
shrink-to-fit and its intrinsic width comes from the labels — 171px
against 268px of segments, which overflowed under the primary action at
700-834px.
Also: registers the factsheet detail routes as header-addon-slot
claimants, moves the save-failure live region with Save, re-offsets the
lg sidebar to clear both headers, and adds the route to the phone-scroll
matrix — no browser test visited a factsheet detail route before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBAt8pJVz2TxMEUJeWEdUy
…der-mockups-cei6lw
# Conflicts:
#	src/components/in-page-nav/in-page-nav-header.tsx
#	src/components/mode-nav/header-addon-slot.ts
#	tests/in-page-nav-header.dom.test.tsx
#	tests/mode-nav-addon-slot.dom.test.tsx
@github-actions

Copy link
Copy Markdown
Contributor

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

  • Buildneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
  • PR requiredneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

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.

check:bundle-budget totals every built chunk, mockups included, and main
sits at ~+9.4% against a 10% tolerance. The study's two scratch chunks
(~9.8 KiB gzip) alone took the repo to +10.1% and failed Build — the
same failure PR #1580 hit, at the same number.
Measured on this branch: main alone 308 chunks / 1538.9 KiB (+9.42%,
passing); with the study 310 / 1548.7 KiB (+10.1%, failing); without it
309 / 1542.4 KiB (+9.67%, passing). The implementation itself adds no
new route chunk — it edits existing components.
Direction 02 has shipped, so the runnable route had already served its
purpose. A design-scratch route that 404s in production is the wrong
thing to spend the last of that headroom on, and raising the baseline
would have settled the open #13/#252 question — whether scratch should
count toward this budget at all — by default, in the direction of "raise
the ceiling". mockups/README.md keeps the three directions and says why
the route went, so the alternatives stay recoverable from history.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBAt8pJVz2TxMEUJeWEdUy
@BigSimmo
BigSimmo marked this pull request as draft August 9, 2026 04:08
@BigSimmo
BigSimmo marked this pull request as ready for review August 9, 2026 04:10
@BigSimmo
BigSimmo requested a balanced review from CopilotAugust 9, 2026 04:10
Comment threadsrc/components/in-page-nav/in-page-nav-header.tsx

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

The diff extends InPageNavHeader with a sectionless breadcrumb variant and adopts it on production factsheet pages, retiring the original mockup study.

Changes:

  • Adds optional primary-action, view-mode, and compact-back slots.
  • Migrates factsheet actions and reading-level controls into the sticky header.
  • Adds behavioral tests and updates design-system/navigation documentation.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
FileDescription
src/components/in-page-nav/in-page-nav-header.tsxAdds the breadcrumb header variant.
src/components/factsheets/factsheet-detail-page.tsxAdopts the header on factsheet details.
src/components/mode-nav/header-addon-slot.tsRegisters factsheet detail routes as header owners.
tests/in-page-nav-header.dom.test.tsxTests the new header options.
tests/factsheet-detail-header.dom.test.tsxTests factsheet header behavior.
tests/factsheet-save.dom.test.tsxUpdates save tests for the actions sheet.
tests/mode-nav-addon-slot.dom.test.tsxAdds the factsheet header claimant.
tests/helpers/phone-scroll.tsAdds factsheets to phone-collapse coverage.
docs/search-chrome-behaviour.mdDocuments the breadcrumb shape.
mockups/README.mdRecords the study outcome and retirement.
docs/design-system/COMPONENTS.mdUpdates generated usage counts.
docs/design-system/adoption-manifest.jsonRecords new component adoption.
docs/branch-review-ledger.mdAdds review records but also reorders historical rows.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadsrc/components/factsheets/factsheet-detail-page.tsx
Comment threaddocs/branch-review-ledger.md Outdated

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:38804f5ce1

ℹ️ 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".

Comment threaddocs/branch-review-ledger.md Outdated
Comment threadsrc/components/in-page-nav/in-page-nav-header.tsx Outdated
Comment threadsrc/components/factsheets/factsheet-detail-page.tsx Outdated
BigSimmoand others added 2 commits August 9, 2026 12:22
The merge created a ledger that deleted existing rows from their
original positions and re-added them elsewhere, breaking the
append-only invariant. Restore the exact main-branch ledger state
and append only the two new records for this branch
(claude/breadcrumb-header-mockups-cei6lw)."
Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com>

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/search-chrome-behaviour.md (1)

95-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Clarify factsheet route ownership.

Lines 95-100 still state that factsheets use their own pattern. Lines 145-149 state that factsheet detail pages now use InPageNavHeader and require route registration. Clarify the scope or remove factsheets from the exception list to prevent contradictory guidance and future duplicate header ownership.

🤖 Prompt for 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.
In `@docs/search-chrome-behaviour.md` around lines 95 - 100, Update the factsheets
wording in the “Still on their own patterns” exception list to match the
documented detail-page behavior: either scope it only to routes that still use
the separate pattern or remove factsheets if all relevant routes now use
InPageNavHeader with route registration. Keep the guidance consistent with the
factsheet ownership requirements described later in the document.
🧹 Nitpick comments (1)
src/components/factsheets/factsheet-detail-page.tsx (1)

350-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use named layout tokens for the sticky offset.

Line 350 adds 4.75rem and 1rem as hardcoded design values. Define named Tailwind 4 @theme tokens in src/app/globals.css, then reference them in this calculation. Keep --inpage-sticky-header-height as the live measured override.

As per coding guidelines, use Tailwind 4 @theme tokens in src/app/globals.css rather than introducing hardcoded design values.

🤖 Prompt for 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.
In `@src/components/factsheets/factsheet-detail-page.tsx` at line 350, Replace the
hardcoded 4.75rem and 1rem values in the aside element’s lg:top calculation with
named Tailwind 4 theme tokens defined in globals.css. Preserve
--inpage-sticky-header-height as the live measured override and keep the
existing sticky offset calculation structure.

Source: Coding guidelines

🤖 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.
Outside diff comments:
In `@docs/search-chrome-behaviour.md`:
- Around line 95-100: Update the factsheets wording in the “Still on their own
patterns” exception list to match the documented detail-page behavior: either
scope it only to routes that still use the separate pattern or remove factsheets
if all relevant routes now use InPageNavHeader with route registration. Keep the
guidance consistent with the factsheet ownership requirements described later in
the document.
---
Nitpick comments:
In `@src/components/factsheets/factsheet-detail-page.tsx`:
- Line 350: Replace the hardcoded 4.75rem and 1rem values in the aside element’s
lg:top calculation with named Tailwind 4 theme tokens defined in globals.css.
Preserve --inpage-sticky-header-height as the live measured override and keep
the existing sticky offset calculation structure.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a906cedc-e531-49bc-9694-b7e40849e4e6

📥 Commits

Reviewing files that changed from the base of the PR and between 003d3cd and be8d8ac.

📒 Files selected for processing (13)
  • docs/branch-review-ledger.md
  • docs/design-system/COMPONENTS.md
  • docs/design-system/adoption-manifest.json
  • docs/search-chrome-behaviour.md
  • mockups/README.md
  • src/components/factsheets/factsheet-detail-page.tsx
  • src/components/in-page-nav/in-page-nav-header.tsx
  • src/components/mode-nav/header-addon-slot.ts
  • tests/factsheet-detail-header.dom.test.tsx
  • tests/factsheet-save.dom.test.tsx
  • tests/helpers/phone-scroll.ts
  • tests/in-page-nav-header.dom.test.tsx
  • tests/mode-nav-addon-slot.dom.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • mockups/README.md
  • docs/branch-review-ledger.md

@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why.

Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker.

If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop.

Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them.

Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head.

Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol.

Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it.

Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted.

Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval.

Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed.

One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1.

Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me.

@cursor

cursorBot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

PR #1769 worked end-to-end from GitHub tip be8d8aca → now f2db535c.

Snapshot

  • Merge: was GitHub DIRTY; merge-tree was clean (behind-but-clean). Synced origin/main once → MERGEABLE, ahead 11 / behind 0.
  • CI: Build, Static PR, Production UI critical green; Production UI shards (1–3) still in progress. mergeStateStatus: BLOCKED waiting on those.

Fixed & pushed

  • Discriminated sections / onSelectSection contract
  • Phone tab order (mode after verbs in DOM)
  • Save-failure live region inside the open actions sheet
  • Search-chrome docs: factsheet detail adopted, not an exception
  • Adoption manifest refresh for the new save test

Dispositioned

  • Outdated mockup/ledger threads (route retired / already fixed in be8d8aca)
  • Sticky-token nit skipped (single fallback calc)

Left for you

  • Open Copilot thread: PR title/body still say mockup-only — gh pr edit denied for this integration. Please retitle/update the description.
  • Ledger row for this head is local only (not pushed — avoids a ledger-only tip).
  • Summary comment posted on the PR.

verify:cheap (543 files / 5823 tests) and verify:pr-local (incl. build) both green. Merge left to you.

Open in WebOpen in Cursor

cursoragentand others added 3 commits August 9, 2026 05:57
Behind-but-clean sync: merge-tree vs origin/main was clean; GitHub DIRTY was
staleness after main advanced to 5bd0eee (#1770).
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…ve status
Require onSelectSection whenever sections are provided so section clicks cannot
no-op. Put mode after the verb controls in DOM so phone focus matches the
painted rows, and keep the save/failure live region inside the open actions
sheet. Align search-chrome docs with factsheet detail adoption.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
The denied-storage sheet coverage imports the shared Button/Sheet path through
FactsheetDetailPage, so the adoption inventory must list that test owner.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
@cursor

cursorBot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

PR #1769 babysit summary

Tip:f2db535cc47652afc94f27ef84598c79fae6d605 on claude/breadcrumb-header-mockups-cei6lw
Base:origin/main @ 5bd0eeed — ahead 11 / behind 0
Merge-tree: clean (git merge-tree --write-tree exit 0). GitHub mergeable: MERGEABLE (was DIRTY from behind-but-clean staleness; synced once).

Sync

Fixed (pushed)

FindingChange
Sentry: sections without onSelectSection silently no-opsDiscriminated InPageNavHeaderPropssections requires onSelectSection
Codex P2: phone tab order vs painted rowsMode after verbs in DOM; sm:order-* for desktop; tab-order test
Codex P2: save failure live region behind inert SheetLive region moved inside actions sheet; denied-storage test
Docs: factsheets still listed as exceptionFactsheet detail listed as adopted; exception list no longer includes factsheets
Adoption manifest drift after save testRegenerated docs/design-system/adoption-manifest.json

Dispositioned (no code / outdated)

FindingReason
CodeRabbit / Codex mockup frame tokens, 320px width, scroll reading-levelMockup route retired; file gone
Copilot + Codex P1 ledger rewriteAlready fixed in be8d8aca; append-only restored
CodeRabbit sticky 4.75rem/1rem token nitSkipped — measured-header CSS fallback + standard gap; single call site

Threads

  • Resolved: Sentry, tab-order, save-in-sheet, mockup threads, ledger threads (inline replies forbidden for this integration — dispositions recorded here).
  • Left open: Copilot PR metadata — title/body still describe mockup-only work. PR title/body update denied (Resource not accessible by integration). Human needed: retitle to feat(record-pages): breadcrumb InPageNavHeader on factsheet detail and replace Summary/Verification/Risk with the production factsheet adoption text (classifyPullRequestFiles: clinicalRisk false, ragRanking false, ui true).

Required CI on tip (snapshot)

  • Green: Change scope, Static PR, Safety/config, Unit coverage, Build, Production UI critical, PR policy, PR mergeability, Semgrep, Gitleaks
  • In progress: Production UI (1)(2)(3), Lighthouse advisory

Local gates (decisive lines)

  • Focused Vitest — Test Files 4 passed (4) / Tests 37 passed (37)
  • npm run verify:cheapTest Files 543 passed (543) / Tests 5823 passed | 4 skipped
  • npm run verify:pr-localfailed: (none); includes lint, typecheck, test, build, rag fixtures

Residual risks

  • Shared InPageNavHeader change affects sectioned adopters — covered by header DOM tests + typecheck; browser proof relies on CI Production UI.
  • PR title/body still stale until a human updates them (open Copilot thread).
  • Review ledger row for this head is local only (not pushed — avoiding a ledger-only tip).

Merge left to you.

@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

@cursoragent Unblock the current open PR. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why.

Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP; do not discard dirty work; do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge. No provider-backed gates without approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put blocker dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker and avoid cancelling in-flight required CI unless the push clears that blocker.

If the PR is already merged or closed: confirm outcome, post one summary, and stop.

Primary job: clear mergeability and required-CI blockers on this tip. Do a full product review only if needed to explain or fix a blocker. Stay light otherwise—no broad verification suites or Playwright unless needed to prove one concrete unblock fix.

Snapshot the GitHub head: tip, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, and unresolved threads that block merge or required CI. Missing checks while dirty are not green. Tip moved → re-snapshot.

Ledger-lookup against that GitHub head under an unblock/fix scope for this PR. If already handled at this head for unblock work, merge-tree clean, required checks green, and no blocking threads: summarize, comment, stop.

Unblock once: real conflict → merge origin/main with the smallest correct resolution (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. skip-branch-sync and similar labels do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it.

Fix only what blocks required checks or mergeability on this tip. Ignore advisory red unless it reveals a real required failure. Prefer the smallest safe fix; if a bot/autofix commit caused the break, prefer revert. Prove with the smallest targeted check only. Format and commit before push.

Push only blocker-fix commits. Append ledger for the final GitHub head under the unblock scope. Never push a ledger-only tip.

One PR comment: tip, sync/merge-tree, what blocked and what you fixed or dispositioned, required CI status, residual risks; merge left to me.

Stop when merge-tree is clean on the current GitHub tip, no blocking threads remain for merge/required CI, and required checks are green or clearly in progress.

@cursor

cursorBot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

PR #1769 · tip bcf20715 (remote had already moved past our prior f2db535c)

Snapshot

Unblock actions

  • No code push — nothing failing required checks on this tip
  • Resolved the Copilot title/body thread so merge isn’t blocked by required_review_thread_resolution
  • Could not update title/body (403 / body not agent-managed) — please retitle and refresh the description yourself
  • Ledger for this unblock head left local / unpushed

Required CI

In progress: Static PR, Build, Unit coverage, Production UI critical (plus refreshed PR policy / mergeability). Semgrep / Gitleaks / Safety already green.

Merge left to you.

Open in WebOpen in Cursor

@cursor

cursorBot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

PR #1769 unblock

Tip:bcf20715d23ca90666d97c9993b44e1c7b9c0642 (remote tip; local was behind after a main sync already on the branch)
Merge-tree vs origin/main: clean · ahead 12 / behind 0 · GitHub mergeable: MERGEABLE

What blocked

  1. Earlier: behind-but-clean / dirty mergeability — already cleared by the tip’s merge of origin/main (Document viewer Phase 3: page virtualization, rail windowing, keyboard reading mode, and the first canvas gate #1772 Document viewer Phase 3). No further sync needed.
  2. Review thread resolution (ruleset):required_review_thread_resolution: true. Sole open thread was Copilot’s “PR title/body still describe mockup-only work.”

What I did

  • No code push (nothing failing required checks on this tip; auto-merge not armed).
  • Resolved the Copilot metadata thread so conversation resolution no longer blocks merge. Agent cannot edit PR title/body (403 Resource not accessible by integration / ManagePullRequest body not agent-managed). Please retitle to e.g. feat(record-pages): breadcrumb InPageNavHeader on factsheet detail and refresh Summary/Verification/Risk to the production factsheet adoption (PR policy was already green on the prior tip).
  • Unresolved actionable review threads: 0.
  • Ledger rows for prior babysit + this unblock head: local only (not pushed).

Required CI (this tip)

  • Green so far: Change scope, Safety/config, Semgrep, Gitleaks, GitGuardian
  • In progress: Static PR checks, Build, Unit coverage, Production UI critical, Lighthouse (advisory), and refreshed PR policy / PR mergeability runs
  • Aggregate PR required not settled yet — waiting on the in-progress jobs above

Residual

  • Stale PR title/body until you edit them (content already shipped; policy was green).
  • No code blocker identified on bcf20715.

Merge left to you.

@BigSimmo
BigSimmo enabled auto-merge (squash) August 9, 2026 06:38
@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

@cursoragent resolve merge conflict

@cursor

cursorBot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor

@BigSimmo
BigSimmo merged commit af85cbc into mainAug 9, 2026
26 checks passed
@BigSimmo
BigSimmo deleted the claude/breadcrumb-header-mockups-cei6lw branch August 9, 2026 06:59
Copilot stopped work on behalf of BigSimmo due to an error August 12, 2026 11:03
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@BigSimmo@claude@cursoragent