Skip to content

issues: rewrite #287 from the full phase 3 survey #1764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BigSimmo wants to merge 2 commits into from
Closed

issues: rewrite #287 from the full phase 3 survey #1764

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ecc2f7a
issues: rewrite #287 from the full phase 3 survey
claude Aug 9, 2026
55ba14f
Merge branch 'main' into claude/inpage-nav-info-pages-v8rhnd
BigSimmo Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/outstanding-issues.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ removed after current-main verification; it is not missing recommended work.
| #284 | P3 | issue | tests/pr-handoff-stop.test.ts fails whenever the suite runs as root | **Outcome:** 'npm run test' is green in a root container, so a real failure is not hidden behind a known one. **Detail:** 'pr-handoff-stop hook > emits handoff context only when the marker file exists' expects markerExists('sess-readonly') to be false — it makes the marker directory read-only and asserts the hook could not write there. Root ignores the permission bits, so the write succeeds and the assertion fails. Reproduced on an unmodified bc33d41 checkout as well as on the viewer-optimisation branch, so it is environment-dependent, not a regression. Cost is that every full-suite run in a root container reports '1 failed', which trains readers to skim past the failure count. **Next:** skip the case when 'process.getuid?.() === 0' with an explicit reason, or drop privileges for that assertion. **Stop:** do not delete the coverage — the read-only case is the point of the test on a normal user account. | session 2026-08-08 full-suite runs; reproduced on bc33d41 | 2026-08-08 |
| #285 | P2 | issue | Fresh remote/Cloud containers cannot run npm ci — shipped Node 24.13.0 is below the ^24.15.0 floor that main's jsdom@30.0.1 now requires | Observed 2026-08-08 in a Claude Code web container while syncing PR #1730. npm ci --include=dev aborts with EBADENGINE on jsdom@30.0.1 (needs Node ^22.22.2, ^24.15.0 or >=26); the container ships v24.13.0, so node_modules stays stale and the pre-push static guard then fails typecheck on the missing tailwind-merge added by #1678. Worked around by nvm install 24.19.0 plus a PATH prefix (nvm use alone does not stick — system node shadows it). Next action: raise the engines.node floor in package.json to >=24.15 so the mismatch fails loudly at the declared contract, and provision a compatible Node in the remote/Cloud setup path so a fresh container is not blocked at first install. | session 2026-08-08 (PR #1730) | 2026-08-08 |
| #286 | P2 | task | PR 2 of the in-page nav series - convert the six pill-rail information pages onto InPageNavHeader | **Outcome:** the six routes still drawing the shell-owned pill rail use the documented default in-page navigation template, and the rail behind them is deleted. **Detail:** PR 1 landed as 2806d5e (#1740), extracting InPageNavHeader plus PageSection/toDocumentSections and usePageSectionWeights into src/components/in-page-nav/, and converting differential-detail-page.tsx as proof. Remaining routes: /services/[slug], /forms/[slug], /specifiers/[slug] (record and reference), /formulation/[slug], /dsm/diagnoses/[slug], /dsm/diagnoses/[slug]/differentials. Settled with the owner: replace the rail at every width, drop the InformationPageBreadcrumbs row but keep the large in-body h1, move each record's actions into the ellipsis sheet. **Next:** FIRST widen InPageNavHeader actions from the render prop (close) => ReactNode to ReactNode or ((close) => ReactNode) and close both sheets on pathname change - four of the seven components in scope are Server Components (specifier-record-page, specifier-reference-page, formulation-mechanism-page, dsm-diagnosis-page carry no use client) and React refuses to pass a function across that boundary, so all four fail to build against the current signature. Then add use-resolved-page-sections and use-in-page-section-nav, generalise useDocumentChromeMetrics for an --inpage-anchor-offset, add scroll-mt to every anchor (information-page sections carry none today), convert each page, then delete informationPageSectionDefinitions and the section kind in secondary-navigation.tsx. Two DSM routes declare anchors nothing renders - see #256. Full brief: artifact 77de9f5b-d195-409d-8156-2e4b41cd2f45. **Stop:** do not convert DocumentViewer in this PR (its chrome-metric property names are contract-pinned by tests/header-scroll-hide-contract.test.ts:110-113), and do not verify anchors by grepping for id= - assert against the rendered DOM per route. | session 2026-08-08; PR #1740 (2806d5e) | 2026-08-08 |
| #287 | P2 | task | PR 3 of the in-page nav series - the three locally-owned routes each need a decision, not just a conversion | **Outcome:** every information page uses the documented in-page navigation template, or has a recorded reason not to. **Detail:** the last three routes each own a different bespoke pattern, and none is a mechanical port. (1) /medications/[slug] - SectionTabs at medication-record-page.tsx:183 SWAPS CONTENT rather than scrolling: sectionsByTab[activeTab] at :391 filters record.sections by type, so a different set mounts per tab. The InPageNavHeader track is scroll-spy over anchors that all exist at once, so adopting it means either driving tab state from the track (the track stops meaning where am I on the page) or flattening to one scrolling page - a real behaviour change to a clinical record, and a product call. (2) /differentials/presentations/[slug] - MobileTabs at differential-presentation-workflow-page.tsx plus the xl review sidebar; the old `differentialPresentationSections` shell set is gone and the route is locally owned (`page-secondary-navigation.tsx`). Remaining work is the product decision to adopt `InPageNavHeader` (or keep the tab/sidebar model with a recorded reason), not resurrecting deleted section targetIds. (3) /factsheets/[slug] - the On this page list at factsheet-detail-page.tsx:365-371 is li text with no link, button or handler, and the sections themselves carry no ids at all (:214, :267, :313, :447, :453, :471); the tail is data-driven via factsheet.sections.map keyed on section.heading (:538), so anchor ids must be generated deterministically from headings and that generator becomes the contract the section list depends on. Therapy Compass is deliberately excluded from the whole series - ModeNav is a different multi-route pattern. **Next:** decide the medications tab model first (owner decision, blocks planning); decide whether presentations keep MobileTabs/sidebar or adopt InPageNavHeader; choose the factsheets heading-to-id scheme. Then convert. **Stop:** do not port medications mechanically - swapping the tablist for a scroll track silently changes what a clinician sees on a medication record. | session 2026-08-08; follows #286 | 2026-08-08 |
| #287 | P2 | task | PR 3 of the in-page nav series - the three locally-owned routes each need a decision, not just a conversion | **Outcome:** every information page uses the documented in-page navigation template, or has a recorded reason not to. **Detail:** the last three routes each own a different bespoke pattern. Fully surveyed 2026-08-08 (all three components and their tests read end to end); this supersedes the initial sketch. (1) /medications/[slug] - SectionTabs (medication-record-page.tsx:175-181; tabs summary/dosing/safety/more) SWAPS CONTENT: sectionsByTab at :378-389 buckets record.sections by type, activeTab is local useState at :376 with no URL sync, and only activeSections is mapped into the single role=tabpanel at :444-464, so non-active sections have NO DOM nodes. OWNER DECISION 2026-08-08: keep tab-swapping and drive it from the header track. That makes this the CHEAPEST of the three and unblocks it - differential-detail-page.tsx:1041-1066 is an exact precedent (also panel-swap, maps activeId onto tab state, passes explicit weights because there is nothing on screen to measure, per page-section-index.ts:31-36). Sections carry no id in the data (MedicationSection = title/type/rows, src/lib/medications.ts:20-24, confirmed across all 328 snapshot records), so build the four PageSections from the tab ids. The page has NO page-level actions and no role=status notice, so omit actions entirely. Watch two data facts: the safe type in safetyTypes occurs zero times, and spec/sel/comp appear in 7/1/1 records, so the more tab is empty for most drugs. Placement needs width=bleed or mounting outside InformationPageShell (:504), and testId medication-page-<slug> is load-bearing for ui-smoke.spec.ts:129. Needs NOTHING from #286 - buildable against merged PR 1 alone. (2) /differentials/presentations/[slug] - the shell set is gone and the route is locally owned; remaining work is a product decision, not a bug fix. This is the HARDEST of the three. MobileTabs (:595-630) is a CROSS-ROUTE link strip: Overview/Map/Related navigate to the first candidate diagnosis, only Compare is a self-link, so those three destinations must move into the actions sheet or be dropped - they are not sections. Blockers: the page is a Server Component (no use client) so it needs converting or a client wrapper; four panels take only {workflow} and each render 2-3 times across breakpoints, so ids must be threaded as props (SafetySnapshot :288, HighestUrgencyPanel :372, ReviewPanel :397, SourceStatusPanel :454); ReviewPanels has NO phone render path (:753 tablet, :759 desktop only), which is why three of the old sixteen targetIds were unbuildable - do not repeat that; and five pinned assertions must change deliberately (four MobileTabs strings at tests/mobile-interaction-regressions.test.ts:54-70, which encode a past regression, plus tests/mode-nav-addon-slot.dom.test.tsx:46 asserting the route does not own the slot). Leave the phone footer and the main className alone - both contract-pinned. Fold in one real a11y gap: CopyAfterReviewButton signals success only by swapping its own label, with no live region anywhere in the feature. (3) /factsheets/[slug] - there are 17 section elements, not the 6 first listed, and NONE carries an id (the only DOM id in the 854-line file is unrelated). The On this page list at :364-373 is li text with no link, button or handler, and it escapes require-button-wiring because it is not a button. Its source tocFor (factsheets-data.ts:750-770) is a hand-maintained per-kind list, wrong in both directions: it names four blocks that are divs not sections (:431, :512, :602, :719), mislabels three, and omits two real sections (:267, :313). Build the list from what renders, not from tocFor; note :267 is conditional on moreInTopic.length. Only medLite has data-driven sections (2 of 8 sheets, 6 total, factsheets-data.ts:83-87 = {heading, body}, no id/slug), and headings are unique today but UNASSERTED - escitalopram's What it is / What to expect are byte-identical to static therapy headings at :628/:660 - so ship a uniqueness assertion with the generator. Moving Save into the sheet breaks tests/factsheet-save.dom.test.tsx:17-25 and orphans the sr-only live region at :158, which must stay outside the sheet. Do NOT rename or extract FactsheetPrintSheet (:733): design-system-contract-utils.test.ts:372-386 fails closed without it. Back destination changes deliberately from the breadcrumb's /factsheets/search to appModeHomeHref(factsheets). Therapy Compass stays excluded - ModeNav is a different multi-route pattern. **Next:** recommended order medications, then factsheets, then presentations as its own PR - cheapest and safest first, now that presentations is known not to carry a live bug. Factsheets and presentations need #286's scroll-spy hooks and anchor-offset generalisation; medications does not. Full brief: the PR 3 handover artifact published 2026-08-08. **Stop:** do not re-add a section set for the presentations route - tests/page-secondary-navigation.dom.test.tsx:78-90 forbids it. | session 2026-08-08; follows #286 | 2026-08-08 |

@coderabbitai coderabbitai Bot Aug 9, 2026

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate heading IDs at the rendered-page boundary.

Line 329 places the uniqueness assertion in the generator, but it also identifies static headings in FactsheetDetailPage at lines 628 and 660. factsheets-data.ts cannot validate those headings or collisions between static and medLite sections. Validate the final per-factsheet section list in the shared ID builder or a rendered-DOM test.

🤖 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/outstanding-issues.md` at line 329, Move heading-ID uniqueness
validation from the data generator to the final per-factsheet ID-building path
used by FactsheetDetailPage, so it includes both static headings and
medLite-generated sections. Ensure collisions across all rendered sections fail
validation, using a rendered-DOM test only if no shared ID builder exists.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make the generated factsheet list navigable.

Line 329 records that the current On this page entries are plain <li> text with no link, button, or handler. The next step only replaces tocFor with rendered sections. Require each entry to target its generated section ID with an anchor or equivalent keyboard-accessible control. Otherwise, adding section IDs does not complete factsheet navigation.

🤖 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/outstanding-issues.md` at line 329, Update the factsheets page’s
generated “On this page” list to render each entry as a keyboard-accessible
navigation control targeting the corresponding generated section ID, rather than
plain li text. Keep the list derived from rendered sections instead of tocFor,
and ensure each target uses the same IDs assigned by the section generator.

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Synchronize #287's execution-queue entry

When a session selects #287 from the recommended execution queue, line 167 still says After #286; medications needs an owner product call and instructs the reader to decide the tab model, while this rewritten row says the owner decision is complete and medications needs nothing from #286. Because this ledger declares task order and dependencies canonical, the contradictory entry can unnecessarily block the medication conversion or reopen an already-settled decision; update the queue row alongside this detail.

Useful? React with 👍 / 👎.

| #288 | P3 | rec | Decide whether DocumentViewer adopts the template it was extracted from, or the partial adoption is recorded as final | **Outcome:** the in-page navigation template has one deliberate owner story rather than an unexplained gap. **Detail:** PR 1 (2806d5e, #1740) extracted the header from DocumentViewer.tsx and differential-detail-page.tsx, which held it near-verbatim twice, into src/components/in-page-nav/InPageNavHeader. differential-detail-page was converted onto it; DocumentViewer was deliberately NOT, because its own useDocumentSectionSpy and useDocumentChromeMetrics wiring and its CSS custom-property names (--document-anchor-offset, --document-sticky-header-height, [data-document-sticky-header]) are pinned verbatim by tests/header-scroll-hide-contract.test.ts:110-113. Once #286 and #287 land, the template is adopted on every information page EXCEPT DocumentViewer. `docs/search-chrome-behaviour.md` (Default in-page navigation template) already records that DocumentViewer keeps its own header copy because it owns the page h1, uses edge-glass-header, and is pinned by visual baselines — so the gap is documented, not overlooked. #286 generalises chrome metrics for information pages only; it does not close DocumentViewer convergence. **Next:** owner decision only — convert DocumentViewer later (leaving pinned `--document-*` property names untouched per tests/header-scroll-hide-contract.test.ts:110-113), or explicitly mark the documented non-adoption as the final end state in this ledger when the series closes. **Stop:** do not rename or repoint the pinned document CSS custom properties to unify them with the information-page ones - the contract test pins those exact strings and the document route is the highest-traffic surface in the app. | session 2026-08-08; PR #1740 | 2026-08-08 |

## Resolved / archive
Expand Down
Loading