Skip to content

Docs front door + page chrome: reference console (#273) - #275

Merged
simpros merged 3 commits into
mainfrom
gh-273-docs-design-pass
Oct 2, 2026
Merged

simpros merged 3 commits into
mainfrom
gh-273-docs-design-pass

Conversation

@simpros

@simpros simpros commented Oct 2, 2026

Copy link
Copy Markdown
Owner

#273

Why the change

Rebuild the published docs chrome and front door in the real manifest-driven pipeline as the approved "reference console" stance: dense terminal-manual pages with a slim top bar, grouped sidebar, scroll-spy TOC rail, labelled copyable code, and a spec-sheet front door.

Special things to note

  • Sweep result: no page overflow, no sub-32px target, no clipped text at 360/390/768/1280 on the front door, docs/index.html, getting-started and cli-reference; sticky table headers verified stuck; drawer (10 links, CSS-only), copy confirmation and scroll-spy exercised in the browser; full-page captures at 375px and 1280px checked locally.
  • Motion review (vendored review-animations, emil-design-eng): state changes only — drawer, disclosures, copy .done, TOC .active — at 150–180ms on a strong ease-out curve, prefers-reduced-motion gated. One noted deviation: the FAQ disclosure height eases via grid-template-rows, the only way to animate a <details> height; short, occasional, and gated.
  • No assemble.ts change was needed: the sidebar model, manifest titles/descriptions and all docs/*.md are untouched. .design/ and BRIEF-273.md stay untracked.

Change outline

Page chrome, before → after:

 <body>
+  <input checkbox drawer state> (sibling of header + shell)
-  <header class="site"> brand only
+  <header class="topbar"> brand · Docs-menu trigger · GitHub
-  <div class="wrap"><div class="layout">
+  <div class="shell">
     <nav class="docs-sidebar"> (group kickers, aria-current rail)
     <main> crumb + TOC-inline-<details> + body </main>
+    <aside class="rail"> TOC + scroll-spy </aside>

Component responsibilities:

docs/site/
├── shell.ts      # topbar, static grouped nav, crumb, dual TOC, drawer order, spy script
├── theme.css     # console stance: 1-col → 236px+1fr (860) → +210px rail (1120)
├── codeblock.ts  # lang + source-file header, "copied" confirm, $ -stripping copy
├── markdown.ts   # no printed `#` anchors, tables own their .tablewrap
└── marketing.html# spec-sheet hero, glance card, markup lifecycle, spec table,
                   # 3-step strip, titled prompt card, <details> FAQ

Data flow for the code header: fence meta ```yaml .sprout.yaml → fileFromMeta → <span class="codeblock-file">; the prompt block always labels text · prompt + onboarding-prompt.md.

Closes #273

Slim top bar, grouped sidebar with drawer fallback, mono breadcrumb, dual TOC with scroll-spy, labelled code blocks, table scroll containers, spec-sheet front door. Tests updated to the new structure.
@simpros

simpros commented Oct 2, 2026

Copy link
Copy Markdown
Owner Author

🧊 Thermo-nuclear review (round 1) — REQUESTCHANGES · 80ebfa6d

  • Findings
    1. Dead disclosure state left behind (open on SidebarGroup) — structural regression + obvious code-judo
    1. codeBlockFigure infers prompt chrome from an aria-label string — string sentinel as a type
    1. Invisible, focusable drawer checkbox on desktop — a11y regression
    1. TOC renders h3/h4 levels but the scroll spy only tracks h2
    1. Unscoped .toc selectors collide between the docs rail and the marketing hero
    1. Newly added CSS with no producer
  • Residual risks
  • Unverifiable from here
Full report (click to expand)

VERDICT: REQUEST CHANGES

Findings

1. Dead disclosure state left behind (open on SidebarGroup) — structural regression + obvious code-judo

docs/site/shell.ts:45-49 still declares open: boolean; docs/site/assemble.ts:144-152 still computes currentGroup solely to fill it (open: currentGroup ? currentGroup === group.name : index === 0); docs/site/assemble.ts:130-136 still documents "the reader's group renders open … wrong group open"; docs/site/grouped-nav.test.ts:100-114 still asserts it. renderSidebar (shell.ts:116-139) no longer reads open at all — it renders every group as a flat gname + list. So the MR replaced disclosures with sections but left the entire disclosure-era model, its comments, and its tests in place.

Remedy: delete open from SidebarGroup, drop currentGroup/the group-name lookup from docsSidebar (keep only the current membership validation the test exercises), fix the comments, and delete the open assertions in the test. This is the "delete a whole concept" move: fewer fields, fewer passing-but-meaningless tests, no stale invariant.

2. codeBlockFigure infers prompt chrome from an aria-label string — string sentinel as a type

docs/site/codeblock.ts:49-50:

const langLabel = copyLabel === PROMPT_COPY_LABEL ? PROMPT_LANG_LABEL : shown;
const fileLabel = file ?? (copyLabel === PROMPT_COPY_LABEL ? PROMPT_SOURCE_FILE : undefined);

A copy-button accessibility label is now the discriminant that rewrites the displayed language and injects a filename. Any caller passing the same label for a non-text fence silently gets text · prompt / onboarding-prompt.md; changing copy wording silently changes header content. fileFromMeta (codeblock.ts:30-37) compounds it by sniffing freeform fence meta with token.includes(".") || token.includes("/"). Positional optionality is also growing (lang, code, copyLabel, file).

Remedy: make the header explicit and dumb. Have codeBlockExtension and promptFigure compute { langLabel, file, copyLabel } (or a discriminated prompt: true) and let codeBlockFigure only format. Drop the includes(".")/includes("/") sniff in favor of the caller that knows the fence contract, and collapse the .filter().find() into one find.

3. Invisible, focusable drawer checkbox on desktop — a11y regression

docs/site/theme.css:98-107. .sidebar-state is a clipped 0×0 input at all widths, but .drawer-toggle is display:none until @media (max-width: 859px) (theme.css:632). The focus-ring rule targets the hidden label (.sidebar-state:focus-visible ~ .topbar .drawer-toggle), so on desktop tab order lands on a 0×0 control with no visible focus indicator and no effect. The code this replaced explicitly avoided this ("desktop keyboard users never land on an invisible control").

Remedy: hide the checkbox on desktop (.sidebar-state { display: none } in the base sheet) and re-enable both input and label together inside the narrow media query, where the :checked selectors already live.

4. TOC renders h3/h4 levels but the scroll spy only tracks h2

docs/site/shell.ts:158-166 emits every level > 1 (lvl-2 … lvl-4), and theme.css:251-252 spends rules indenting .lvl-3/.lvl-4. But docs/site/shell.ts:223 queries main h2[id] only. On pages with subheadings (operator-deploy has 9 ###), every lvl-3 link is permanently inert — no .active, no reading-position feedback. This is half a feature: either query all heading levels (main :is(h2,h3,h4)[id], keeping the "last one above the threshold" rule) or filter the TOC to level 2 and delete the level CSS. Pick one; don't ship the indentation without the tracking.

5. Unscoped .toc selectors collide between the docs rail and the marketing hero

docs/site/theme.css:231-252 styles .toc a as the sticky rail (flex, min-height 34, transparent left border used as the active indicator, transition, .active), and theme.css:210/216 adds a generic nav.toc a override. But docs/site/marketing.html:27 reuses <nav class="toc"> for the front-door link list, so the rail's chrome (border-left, min-height, border-radius, font-size: 12.5px, transition, active state) bleeds into a completely different context, and the nav.toc a patch exists only to fight that collision.

Remedy: scope the rail rules under .rail .toc (or rename the marketing nav to .hero-toc) so the two surfaces stop sharing a selector by accident.

6. Newly added CSS with no producer

docs/site/theme.css:593-600 (footer .finner) and docs/site/theme.css:387 (figure.codeblock pre.tall) have no matching markup: siteFooter emits <footer id="docs"><p>… with no .finner, and codeBlockFigure never adds tall. Delete both (or wire the intended markup); dead rules are exactly the kind of incidental complexity this pass should not be adding.

Residual risks

  • Desktop phantom focus stop: tab from the top of any page ≥860px wide and observe focus landing with no visible ring (finding 3) — probe by tabbing and checking document.activeElement / :focus-visible styling.
  • Inert sub-heading nav: on docs/operator-deploy.html, scroll through the h3 sections and confirm no .lvl-3 link ever gets .active (finding 4).
  • Front-door nav drift: compare the marketing hero .toc link styling before/after any future rail change; the rail rules leak onto it today (finding 5).
  • Table double-wrap: wrapTables only recognizes the exact literal TABLEWRAP_OPEN immediately preceding a table; a markdown-authored raw-HTML wrapper with any attribute or whitespace would be double-wrapped — probe by rendering a doc containing a hand-wrapped <table>.
  • $-stripping edge case: copyValue strips prompts only when every non-empty line starts with $ ; a copied block mixing $ command with output keeps the $. No current corpus block starts with $, so this branch is unexercised in production — probe with a mixed command/output fence.

Unverifiable from here

Rendered layout/a11y behavior in a browser, whether the test suite actually passes (no bun in this checkout), and whether @tanstack/markdown still emits heading ids under headingAnchors: false (asserted by tests, not run). HEAD was verified at 80ebfa6dd4ba47245f3e0a3e73540949fe888c10; it did not move.


Automated agent review (opencode) · worktree /home/sim/.herdr/worktrees/sprout/gh-273-docs-design-pass · subsequent rounds will reply in this thread.

Drop dead sidebar disclosure state (open/currentGroup), make codeblock
header explicit instead of copy-label-sniffed, hide drawer checkbox on
desktop, track h2-h4 in scroll spy, scope rail TOC selectors off the
front-door nav, delete dead CSS rules.
@simpros

simpros commented Oct 2, 2026

Copy link
Copy Markdown
Owner Author

🧊 Thermo-nuclear review (round 2) — REQUESTCHANGES · f9793735

    1. fileFromMeta reinvents the parser's node.file and misreads valid fence meta — codeblock.ts:31-34 (used :73)
    1. copyValue decides what gets pasted by sniffing content shape at click time — codeblock.ts:108-115
    1. withInlineToc splices another module's HTML by substring search — shell.ts:199-205 (used :285)
    1. wrapTables carries a dead idempotence branch and lookup arithmetic — markdown.ts:80-89
    1. "The one place the prompt header lives" is actually two places — codeblock.ts:74-79 and :89-95
  • Notes (not blockers)
  • Residual risks
  • Unverifiable from here
Full report (click to expand)

VERDICT: REQUEST CHANGES

Reviewed git diff origin/main...HEAD at HEAD f9793735bd1ecb0665464f952ede95b424fe8fd8 (matches the brief; branch gh-273-docs-design-pass). bun test docs/site/ (84 pass), bun run docs:typecheck, bun run docs:check are all green. The findings below are structural, not gate failures.

1. fileFromMeta reinvents the parser's node.file and misreads valid fence meta — codeblock.ts:31-34 (used :73)

The library this repo already depends on parses file=/title= into CodeBlockNode.file (node_modules/@tanstack/markdown/dist/types.d.ts:38, parser.js:322,331), and CodeBlockNode is exactly what the extension receives after node.type === "code". Instead the diff hand-rolls a second fence-meta grammar: "first token that is not prompt is the file".

That grammar is wrong against the parser's own meta contract. A fence written ```ts {1,3} yields meta === "{1,3}", highlightLines [1,2,3], no file — but fileFromMeta("{1,3}") returns "{1,3}" and the header will render <span class="codeblock-file">{1,3}</span>. Same for ```bash lines=1-3 → lines=1-3, and framework=react → framework=react. The comment "The only other meta token the fence contract allows is the prompt tag" is simply false for the parser this file consumes.

Remedy (deletes code, not adds it): have the corpus write ```yaml file=.sprout.yaml, read node.file in codeBlockExtension, and delete fileFromMeta plus its test block. The bare-filename convention buys nothing the parser does not already do and competes with it. If a bare token must be supported, it has to reject key=value and {...} tokens explicitly — but preferring the parser's field is the simpler, sound path.

2. copyValue decides what gets pasted by sniffing content shape at click time — codeblock.ts:108-115

Every non-empty line starting $ silently causes the copied text to differ from the displayed text. This is runtime magic keyed off string shape, in direct tension with the same PR's deliberate move to explicitly-passed CodeBlockHeader data (codeblock.ts:36-43) — the header was made explicit, then copy semantics were made implicit. It also doesn't match the approved direction (BRIEF-273.md / .design/002-console-reference/README.md: each command row gets its own copy affordance), and today no docs/*.md block contains $ , so this ships as speculative behavior that is only unit-tested against a stub.

Remedy: make it data, not inference. Either a fence-meta token (```sh copy=commands) carried into a data-copy attribute, or per-row copy buttons as the mockup specifies. A block whose lines merely happen to start with $ should copy verbatim unless the markup says otherwise.

3. withInlineToc splices another module's HTML by substring search — shell.ts:199-205 (used :285)

The shell reaches into the markdown-rendered body and inserts <details> after the first literal </h1>. That makes the shell depend on an unstated invariant of markdown.ts's output (allowHtml: true means a raw <h1> in any page could be the match; a page without an h1 falls back to a silently different placement). This is a boundary leak and exactly the kind of incidental-control-flow coupling the rubric flags.

Remedy: give the placement an explicit seam instead of string surgery — have renderMarkdown (which already owns the AST and knows the first heading node) return the body with a slot for the inline TOC, or accept the card and insert after the first heading node there. The shell should never parse body HTML to decide where chrome goes.

4. wrapTables carries a dead idempotence branch and lookup arithmetic — markdown.ts:80-89

renderDocumentBody calls wrapTables exactly once (markdown.ts:64); nothing in the repo calls it on already-wrapped HTML. The check full.slice(Math.max(0, offset - TABLEWRAP_OPEN.length), offset) === TABLEWRAP_OPEN exists only to satisfy the expect(wrapTables(body)).toBe(body) test the same diff added. It is non-obvious incidental complexity guarding an invariant with no production caller. (The non-greedy <table[\s\S]*?</table> also cannot wrap nested tables; not in the corpus today, but it is a latent sharp edge on top of the magic.)

Remedy: drop the idempotence branch, stop exporting the helper, and delete the test that pins a contract nothing depends on. If double-wrapping is a real requirement, encode it in the regex (optional wrapper group) rather than offset arithmetic.

5. "The one place the prompt header lives" is actually two places — codeblock.ts:74-79 and :89-95

CodeBlockHeader was introduced precisely so the prompt header is explicit data, but the same three-field literal (copyLabel: PROMPT_COPY_LABEL, langLabel: PROMPT_LANG_LABEL, file: PROMPT_SOURCE_FILE) is written in both codeBlockExtension.renderHtml and promptFigure, and a test asserts the two stay byte-identical by hand.

Remedy: one const PROMPT_HEADER: CodeBlockHeader = { … }; promptFigure uses it directly and the extension spreads it with the parsed-file override ({ ...PROMPT_HEADER, file: node.file ?? PROMPT_HEADER.file }). Then the comment is true and the test no longer exists to police a copy-paste.

Notes (not blockers)

  • No file crosses 1000 lines after this change (theme.css 713, shell.ts 298, marketing.html 175) — no decomposition concern.
  • Stale comments referencing the removed .wrap: docs/site/test-fixture.ts:18 and docs/site/assemble.test.ts:505-506.
  • .tablewrap overflow-auto (markdown.ts:80, marketing.html:89) references an undefined overflow-auto class; .tablewrap already sets overflow: auto. Harmless but dead.
  • scrollSpyScript tracks h2,h3,h4 while tocItems emits links for every level > 1 (h5/h6 included), so a future h5/h6 TOC entry can never become .active. Low impact today.

Residual risks

  1. A fence using supported meta ({1,3}, framework=, lines=) renders that token as a source filename via fileFromMeta. Probe: grep the assembled _site/docs/*.html for class="codeblock-file"> and assert every value is a path, never {, framework=, or lines=.
  2. copyValue rewrites clipboard content on any block whose non-empty lines all start $ . Probe: in the browser, copy each text/sh block and diff the clipboard against code.textContent.
  3. withInlineToc misplaces or drops the card when bodyHtml does not start with the page </h1> (first-match substring). Probe: every assembled docs page should contain exactly one .toc-inline and its offset should sit immediately after the first </h1>.
  4. The scroll-spy can leave no link active (headings/links mismatch, or page-bottom clamp). Probe: scroll cli-reference.html to the bottom and assert exactly one .toc a.active / .toc-inline a.active whose href matches the last tracked heading.
  5. Drawer regression on mobile if the checkbox/header/shell sibling order or stacking changes. Probe: at 375 px with JS disabled, toggle .drawer-toggle and assert .docs-sidebar becomes visible with no horizontal page scroll.

Unverifiable from here

Rendered appearance, responsive behavior at 360/390/768/1280, real clipboard and scroll-spy behavior, and screen-reader output could not be checked from a checkout — only the static HTML/CSS/TS and the test suite were inspected.

@simpros

simpros commented Oct 2, 2026

Copy link
Copy Markdown
Owner Author

Comment-removal second opinion (independent of the reviewer above): 4 removed comment block(s) look like they carried information the code cannot express — concentrated in the files below. Report-only: decide per file whether the reason should be rewritten rather than dropped.

  • docs/site/grouped-nav.test.ts — 2 block(s), e.g. // Hero keeps both entry points: docs first, agent path second.
  • docs/site/assemble.test.ts — 1 block(s), e.g. // The shell owns .wrap and .layout: exactly one per page, never nested.
  • docs/site/assemble.ts — 1 block(s), e.g. // artifact path being written, never hand-set per call. The reader's group

Heuristic, not a verdict: a missing flag is not proof that nothing was lost.

@simpros

simpros commented Oct 2, 2026

Copy link
Copy Markdown
Owner Author

🧊 Thermo-nuclear review (round 3) — REQUESTCHANGES · f9793735

  • Findings
    1. The footer lost its page container — clear layout regression
    1. The heading anchor affordance was deleted, but the comment claims CSS renders it
    1. Mobile table card-ification is generic, but its labels are a one-off — and it hides headers from AT
    1. withInlineToc does HTML-shape surgery on the rendered body
    1. wrapTables idempotency is bespoke string arithmetic, and overflow-auto is dead
    1. The prompt header is constructed twice
    1. Scroll-spy constants are decoupled from the CSS they track
  • Residual risks
  • Unverifiable from here
Full report (click to expand)

VERDICT: REQUEST CHANGES

I reviewed git diff origin/main...HEAD at HEAD f9793735bd1ecb0665464f952ede95b424fe8fd8 (matches the brief). All 84 docs/site tests and docs:typecheck pass, so the findings below are structural/behavioral, not test failures.

Findings

1. The footer lost its page container — clear layout regression

docs/site/shell.ts:288-289 (closes .shell, then appends siteFooter) and docs/site/theme.css:590-596.

The old chrome had .wrap { max-width: var(--max); margin: 0 auto; padding: 0 1.25rem 4rem; } around header + layout + footer, so the footer content was inset and width-capped. This PR deletes .wrap, makes .shell the only container (theme.css:107-114, max-width + padding: 0 clamp(14px,3vw,28px)), but renderShell emits the footer outside .shell, and the new footer rule has no max-width and no horizontal padding. On every viewport the footer text is flush against the viewport edge while the topbar/main are inset by clamp(14px,3vw,28px) — most visible at 375px and at ≥1280px where .shell is centered but footer is not. The brief's item 7 asks for a legible footer, not a misaligned strip.

Remedy: reintroduce one container rule for header+shell+footer (e.g. a .container/.wrap class), or at minimum give footer max-width: var(--max); margin-inline: auto; padding-inline: clamp(14px,3vw,28px). If the full-bleed border-top is wanted, put the border on an outer element and the padding on the inner content.

2. The heading anchor affordance was deleted, but the comment claims CSS renders it

docs/site/markdown.ts:64-73 and docs/site/theme.css (no matching rule).

headingAnchors: false removes the anchor-heading element, and the old theme's h1:hover .anchor-heading … { opacity: 1 } rules were deleted with it. The new comment ("the anchor affordance lives in CSS hover/focus, not as a literal character in the markup") describes CSS that does not exist anywhere in theme.css, and the brief's item 3 explicitly requires "an anchor affordance, not a character". As shipped there is no per-heading deep link at all: headings have ids, but nothing on the page exposes them, and the code comment asserts the opposite of what the code does.

Remedy: restore a CSS-only affordance (an extension that appends a focusable/hover-revealed link styled via ::after, so no literal # sits in the markup), or delete the claim and the test expectation and accept the lost affordance explicitly. Do not leave the comment asserting a feature that isn't there.

3. Mobile table card-ification is generic, but its labels are a one-off — and it hides headers from AT

docs/site/theme.css:681-708.

table { display: block } plus thead { display: block; height: 0 } and thead tr, thead th { display: none } applies to every table ≤640px. But the only "labelled rows" are table.spectable (:697-707). A 3-column table like docs/cli-reference.html ("Command | Flags | Purpose", with empty first cells for continuation rows) loses its headers and renders as unlabelled cards; the display: none on thead tr/th also removes the header cells from the accessibility tree, contradicting the comment "The thead stays in the DOM at zero height". The brief's item 6 asks for stacked labelled rows.

Remedy: pick one policy per table shape. Either scope card-ification to .spectable and leave the docs tables in their .tablewrap horizontal scroll, or emit data-label on every td from the header row at render time and use td::before { content: attr(data-label) }, keeping header cells visually hidden but AT-readable (clip/sr-only) rather than display:none.

4. withInlineToc does HTML-shape surgery on the rendered body

docs/site/shell.ts:195-205, called at shell.ts:285.

Splicing the card after the literal first </h1> is exactly the "narrow edge-case handling in the middle of a flow" the rubric flags. With allowHtml: true, a raw-HTML block or a future code sample containing </h1> silently misplaces the card; a page without an </h1> silently moves it to the very top. The shell already receives the heading model (opts.toc) and is the single chrome owner, so it can do better.

Remedy: have renderMarkdown/renderDocumentBody emit a stable sentinel (e.g. a comment node) after the first rendered heading, and have the shell replace the sentinel; or render the inline card where the headings are known. Any of these deletes the string search.

5. wrapTables idempotency is bespoke string arithmetic, and overflow-auto is dead

docs/site/markdown.ts:80-89.

The full.slice(offset - TABLEWRAP_OPEN.length, offset) === TABLEWRAP_OPEN lookbehind exists to make a second call idempotent (and to satisfy markdown.test.ts:82), but the only production call site (renderDocumentBody) runs once; the branch is untestable-in-practice cleverness that also couples correctness to the exact class string. Meanwhile the wrapper emits class="tablewrap overflow-auto" while theme.css styles only .tablewrap — overflow-auto has no rule (the old table { overflow-x:auto } was removed, and .tablewrap already sets overflow:auto). It is dead markup pinned by tests. This also runs against the module's own header claim of not "regexing rendered HTML".

Remedy: either wrap at the AST/extension layer (consistent with codeBlockExtension) or make wrapTables private, drop the idempotency lookbehind, and drop overflow-auto from the wrapper (update the two tests that pin it). If genuinely wrapped raw-HTML tables must be respected, detect via an explicit marker rather than a 38-character lookbehind.

6. The prompt header is constructed twice

docs/site/codeblock.ts:74-79 and codeblock.ts:89-95 build the identical { copyLabel: PROMPT_COPY_LABEL, langLabel: PROMPT_LANG_LABEL, file: file ?? PROMPT_SOURCE_FILE }. The comment at :87-88 calls promptFigure "the one place the prompt header lives", but it is two. Extract promptHeader(file?: string): CodeBlockHeader and call it from both promptFigure and codeBlockExtension.renderHtml.

7. Scroll-spy constants are decoupled from the CSS they track

docs/site/shell.ts:223,231.

heads queries h2,h3,h4 while tocItems (shell.ts:159) includes every level > 1 (h5/h6 get a TOC link that can never become active), and the activation line is a hardcoded 96 while the CSS sticky/scroll-margin value is 74px (theme.css:190,196,606). Derive the tracked ids from the rendered TOC links (or share one constant) so the two cannot drift.

Residual risks

  • Footer flush/misalignment ships visible. Probe: open the assembled docs/getting-started.html at 1280px and 390px and compare the footer text left edge with main; it will not align.
  • Mobile docs tables lose column meaning and header announcement. Probe: docs/cli-reference.html at 390px — inspect the 3-column ci table with VoiceOver/NVDA; headers are display:none.
  • No heading deep-link at all. Probe: hover/focus a h2 at 1280px; confirm no # affordance and no focusable anchor appears (confirms Finding 2 is live, not just a stale comment).
  • Scroll-spy highlight lags the sticky line. Probe: scroll docs/cli-reference.html; the active rail link flips at ~96px instead of the 74px sticky offset, and any h5+ link never activates.
  • wrapTables double-wrap on real HTML. Probe: add a table preceded by whitespace or inside a raw-HTML wrapper in a scratch page; assert exactly one .tablewrap per table.

Unverifiable from here

I could not visually compare the assembled site against the approved .design/002-console-reference/ mockups/shots, nor verify the viewport sweep (no overflow, ≥32px targets, no clipped text at 360/390/768/1280) or real browser rendering of backdrop-filter, sticky rail, drawer, and prefers-reduced-motion; those require a browser session.

Rounds 2-3 structural findings (round 1 already landed in f979373):
- codeblock: read filename from the parser's own node.file, delete
  fileFromMeta; getting-started manifest fence now uses file=.sprout.yaml
- copy: gate $-stripping on an explicit copy=commands fence tag carried
  as data-copy; untagged blocks copy verbatim
- prompt header: single promptHeader() factory shared by the extension
  and promptFigure
- markdown/shell: inline TOC placed via an explicit INLINE_TOC_SLOT the
  renderer emits, filled by substitution in the shell
- wrapTables: private, no idempotence lookbehind, no dead overflow-auto
  class (markup, stylesheet, and tests agree on .tablewrap)
- headings: real anchor affordance via headingAnchors object option plus
  CSS-revealed # (no literal in markup); footer content shares the
  shell's max width and gutters
- mobile tables: card-ification scoped to .spectable with clip-hidden
  (AT-readable) thead; docs tables keep .tablewrap scroll
- scroll-spy: tracked headings derive from rendered TOC links, flip line
  74px matching scroll-margin-top
@simpros

simpros commented Oct 2, 2026

Copy link
Copy Markdown
Owner Author

🧊 Thermo-nuclear review (round 4) — APPROVE · 5dd3d0a2

  • Findings
    1. The copy=commands pipeline has no caller anywhere in the corpus — delete the transform rather than ship it speculatively
    1. Heading anchors are exposed and focusable but have no accessible name
    1. fillInlineTocSlot carries an unexercised fallback
  • Residual risks
  • Unverifiable from here
Full report (click to expand)

VERDICT: APPROVE

Reviewed git diff origin/main...HEAD at HEAD 5dd3d0a29f513309f94aa2f8dbc655616e4763d0 (matches the brief). bun test docs/site/ (87 pass), bun run docs:typecheck, and bun run docs:check are all green. No changed file crosses 1000 lines. The round-3 blockers (footer inset, heading affordance, sentinel placement, prompt-header duplication, scroll-spy drift, table card scoping) are genuinely resolved, and the new code is factored into small pure helpers rather than pasted into the shell. The findings below are non-blocking; they are the remaining places where complexity could still be deleted or a contract tightened.

Findings

1. The copy=commands pipeline has no caller anywhere in the corpus — delete the transform rather than ship it speculatively

docs/site/codeblock.ts:31-35 (COPY_COMMANDS_META), :64 (data-copy), :82-85 (extension branch), :119-127 (copyValue), plus the three tests in codeblock.test.ts:176-203 and the getAttribute stub at :141.

grep '^\$ ' docs/*.md returns nothing, and all 31 bash/sh fences in docs/ are written as bare commands, not $ command. So the whole opt-in path — meta constant, rendered attribute, client transform, and its tests — is production machinery tested only against a hand-built stub DOM. This is exactly the "temporary branching likely to become permanent debt" the rubric flags, and there is a clean deletion: copying verbatim is already the corpus's real behaviour.

Remedy: either wire it to a real block (tag one bash fence that actually uses $ ) so it has an executable contract, or drop copy=commands, copyValue, the data-copy branch, and their tests in a follow-up. If the decision is "keep for future authors", say so in the file comment; right now the comment reads as if command blocks exist.

2. Heading anchors are exposed and focusable but have no accessible name

docs/site/markdown.ts:72-77 and docs/site/theme.css:202-213.

The renderer now emits <a href="#id" aria-hidden="false" class="heading-anchor" tabindex="0"></a> (asserted at markdown.test.ts:70). The # is CSS ::after content, so the element has no text; aria-hidden="false" is a no-op that signals the intent was "expose this". Every heading on every docs page therefore adds an unnamed focusable link, and the CSS comment's claim that keyboard readers use the "On this page" links is contradicted by the tab stops. The library only supports content (a literal character in markup, which round 3 rejected) as a name source — it has no aria-label hook.

Remedy: pick one policy. Simplest is the library defaults (ariaHidden: true, tabIndex: -1) and let the # be a hover affordance for pointer users, with the TOC as the keyboard deep-link path. If a focusable, named link is required, it needs post-processing or a custom heading renderer, not the empty anchor.

3. fillInlineTocSlot carries an unexercised fallback

docs/site/shell.ts:203-207, specifically the if (!bodyHtml.includes(INLINE_TOC_SLOT)) branch at :205.

The function handles three cases, but the third (non-empty inline card + a body with no slot) is unreachable: markdown bodies always carry exactly one slot and marketing/index bodies pass toc: [], so the card is always empty for them. The comment admits this ("which no call site does today"). Dead defensive branching in the one place that exists to remove magic.

Remedy: delete the fallback and let substitution be the single rule (bodyHtml.split(INLINE_TOC_SLOT).join(inline) already covers both live cases), or add the call site that needs it. Either way the comment stops documenting an invariant no code enforces.

Residual risks

  • Unnamed heading anchors ship visible to AT. Probe: in a screen reader, Tab from the h1 and confirm each subsequent stop announces only the URL / "link" with no name on every docs page.
  • copy=commands semantics are load-bearing the day it is first used. The guard silently copies verbatim when any non-empty line lacks a $ prefix (multi-line continuations, trailing output). Probe: tag a real block with copy=commands and diff the clipboard against code.textContent.
  • Inline card misplacement on a future page whose first </h1> is not the page title (raw-HTML <h1> under allowHtml). Probe: every assembled _site/docs/*.html should contain exactly one .toc-inline, positioned immediately after the first </h1>.
  • Footer alignment is still 28px off at ≥1320px. footer p is max-width-centered inside a full-bleed footer while .shell content is inset by its own padding (theme.css:107-114, 610-617). Probe: at 1600px compare the footer text left edge with main's.
  • Scroll-spy active link at the page bottom. Headings are now derived from rendered links (good), but the bottom clamp picks the last de-duped id. Probe: scroll cli-reference.html to the end and assert exactly one .toc a.active matching the last heading.

Unverifiable from here

Rendered appearance, responsive layout at 360/390/768/1280, real clipboard and scroll-spy behaviour, drawer operation with JS disabled, and screen-reader output could not be checked from a checkout — only the static HTML/CSS/TS and the green test/typecheck/check suite were inspected.

@simpros

simpros commented Oct 2, 2026

Copy link
Copy Markdown
Owner Author

Comment-removal second opinion (independent of the reviewer above): 6 removed comment block(s) look like they carried information the code cannot express — concentrated in the files below. Report-only: decide per file whether the reason should be rewritten rather than dropped.

  • docs/site/grouped-nav.test.ts — 2 block(s), e.g. // Hero keeps both entry points: docs first, agent path second.
  • docs/site/markdown.ts — 2 block(s), e.g. // TOC, and the code block component all read the same tree instead of
  • docs/site/assemble.test.ts — 1 block(s), e.g. // The shell owns .wrap and .layout: exactly one per page, never nested.
  • docs/site/assemble.ts — 1 block(s), e.g. // artifact path being written, never hand-set per call. The reader's group

Heuristic, not a verdict: a missing flag is not proof that nothing was lost.

@simpros
simpros merged commit 7e4ea7c into main Oct 2, 2026
2 checks passed
@simpros
simpros deleted the gh-273-docs-design-pass branch October 8, 2026 07:04
Sign up for free to 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.

Docs front door + page chrome: design pass with the emil-design-eng skill

1 participant