Second pre-push review, run as an independent auditor against git diff origin/main...HEAD. Round 1 returned Needs fixes: six documentation/structure majors and four functional minors, no regressions. Commit 2e55f087 claims all ten. This review verifies each one individually, audits the two mechanisms that commit introduced (armsReassert and the quiet re-assert), and re-runs the affected suites. Findings only — no code was changed.
TOCEntryRow and the sheet's .entries mode are deleted outright; and all four test gaps are closed with positive controls.prism-scroll.js:635. A setSectionState push whose only changed id is the target's own composite cannot re-assert, so a clamped scroll to a collapsed heading with no collapsed ancestor stays clamped. One line: add element.getAttribute("data-prism-section-id") to the chain, plus a fixture beside collapsedHeadingExpansionReassertsTarget.docs/agent-notes/typography-font-settings.md:136 and docs/accessibility-review.md:65 cite prism/Views/TOCEntryRow.swift, with line numbers. Created by this commit. The historical spec references are fine; these two describe current state.prism-scroll.js:549 still says a push re-asserts only when it touched an ANCESTOR; the same commit added the descendant clause three functions down. The per-function comments are right; the block a reader hits first is not.armsReassert survives the recovery replay correctly. WebDocumentStateSnapshot.apply is the only writer of either half and writes both in the same case arm; coalescedCommands is the only reader and emits both; nothing rebuilds the snapshot piecewise; no production path compares the command by equality. The property the field exists to guarantee is structural, not conventional.navigationTarget is deadline-checked on every read, so a target outliving its window is unreachable rather than stale. revealPending is correctly untouched: an older reveal was already cleared by the announcing initial scroll, and a newer one delivered after the re-assert on the same hook announces for itself and takes the claim.latestSnapshot.scrollTargetBlockID. Nothing was silently disarmed.Needs fixes
All ten round-1 items are genuinely resolved — not merely touched. The six majors are substantive rewrites and every functional fix is pinned by a test that fails without it. Both new mechanisms hold up under audit: armsReassert survives the WebContent recovery replay correctly (one write site, one read site, written and replayed as a pair, so a restore can never be promoted into an arming navigation), and the announce:false re-assert leaves no stale revealPending and no dangling navigation claim. Verification is real: both platforms build with zero compiler warnings, SwiftLint is clean, the WebKit isolation guard passes, and 209 tests across 15 suites executed and passed — including TOCNavigationDetailsIndexTests, the T-1662 index-space suite over the exact producer this branch changes, which the report's own run command omits.
What holds it back is one verified gap in the mechanism this ticket is about, and it is a gap the documentation says is not there. The relevance chain is [domID] + heading ancestors + <details> ancestors, plus the new descendant test. domID is a DOM id, which only ever matches a setDetailsState push; collapsibleHeadingAncestorIds walks strictly j < targetIndex, so it excludes the target. The target's own data-prism-section-id composite — the id space setSectionState actually speaks — is therefore in the chain nowhere. TOC-select a collapsed heading with no collapsed ancestor and expandSectionAndAncestors changes exactly one id, its own: no intersection, no re-assert, and a scroll that landed first stays clamped. That is the same failure mode the mechanism exists for, and the same ancestor-only asymmetry round 3 correctly fixed for descendant <details> — reached by the same reasoning, one case earlier. It is one line plus one fixture.
It is not a regression: that path behaves exactly as it did before this branch, because the mechanism is new. What makes it a major rather than a minor is that both the agent note and Decision 14 describe the chain as starting with the target, which a future reader will take to mean the target's own collapse is covered. Round 1's second major was an agent note stating an invariant the code did not have; this is the same defect one level down. Three one-line tidy-ups ride alongside it — see the must-fix list. None is architectural, and a fourth round should be a short one.
64984b96 fix(toc): list headings inside details blocks and open them on selection 64b17823 fix(toc): make details-nested navigation immune to bridge command order a1b95f6c fix(toc): make navigation re-assert ancestor-aware (T-1928 round-2 review) 2e55f087 fix(toc): address T-1928 round-3 pre-push review Prism's table of contents was missing every heading written inside a collapsible <details> block, so those headings could not be jumped to. That was fixed over three commits. Two earlier reviews found problems; a fourth commit set out to fix all ten at once. This review checks that each was really fixed, and that fixing them did not break anything else.
One case the page still cannot recover from. If you collapse a heading and then pick it from the contents list, the page is told "this one heading changed" — and the code that decides whether to re-do the scroll does not recognise the target's own name in that message. It recognises the names of everything around it, and (since the last commit) everything inside it, but not the target itself. So the scroll can stop short and stay short. It behaved this way before the fix too, so nothing got worse; but the notes written for the next person say this case is covered, and it is not.
SidebarSectionRow.isRowExpanded replaces isCollapsed as the accessibility value's source, branching per row kind. Dimming stays on document collapse, recorded as Quick Decision Q3 — deliberately two different facts.performScrollToBlock(domID, announce). Injection order is bridge, scroll, theme, media, notes, search, so on the shared notifySectionVisibilityChanged the re-assert runs before prism-search's deferred-reveal listener; an announcing re-assert cleared revealPending out from under a newer reveal. Re-asserts now pass false.changedIdsAffectTarget gained isDescendantDetailsID: a changed id matching {domID}-d as a prefix now counts, because the clamping failure is relieved by any expansion that grows the document, including one inside the target.OutboundBridgeCommand.scrollToBlock gained armsReassert: Bool = true, passed false by restoreScroll and carried through WebDocumentStateSnapshot. pointerdown joined the cancel set; the key map became Object.create(null).TOCEntryRow.swift, the sheet's .entries mode and TOCEntryRowTests are gone; the sheet now wraps SidebarContentsView directly, so both surfaces share one row and Req 8.2's indent lands on both at once.Two id spaces feed one intersection test. setSectionState ships MarkdownSection composites matched against data-prism-section-id; setDetailsState ships BlockDOMID DOM ids matched against section.id. The chain is [domID] (DOM space) + collapsibleHeadingAncestorIds (composite space, strictly ancestors) + detailsAncestorSectionIds (DOM space). Round 1 read the exclusion of the target's own composite as deliberate and correct — and on reachability grounds it is, since collapsing the heading you navigated to does not hide that heading. But the mechanism covers a second failure mode, clamping, and there the exclusion is wrong for exactly the reason the descendant case was: expanding the target's own section grows the document below it. expandSectionAndAncestors removes the target's id first and its ancestors second, so a target with no collapsed ancestor produces a single-element diff that matches nothing.
Going quiet also stops a re-assert dropping the typography reflow anchor — the third listener on notifyExplicitNavigation, and the one the comment does not mention. Narrow, because only applyTypography captures an anchor and the initial navigation already dropped the old one; the trade is right, the comment claiming a re-assert "leaves every other authority exactly as it found it" is what overstates.
M1 report — rewritten across all four commits, dedicated "Page changes" section, Affected Files complete against git diff --stat. Residual: the run command omits TOCNavigationDetailsIndexTests, and the header says "11 tests" for a 12-test suite. M2 agent note — four bounds, the ancestor gate named as the fourth and the easiest to lose, diffIdSets located at the only place holding previous state, cancel set correct including pointerdown. M3 decision entry — Decision 14, format-conformant against rules/references/decision-log-format.md: ISO date, valid status, Context/Decision/Rationale, four alternatives with rejection reasons, both consequence polarities, Impact, ---, 6-word title, no non-standard H3. Q1–Q3 sit correctly above Decision 1 on an independent Q sequence; Q3 is the borderline tier call (it does name an alternative and a user-visible consequence) but is defensible. M4 citation — spec path named with the wrong-spec trap called out inline, 8.2's indent implemented at 12 pt matching Decision 10's per-level value, 8.3 marked PARTIALLY covered with T-1924 named. M5 dead code — deleted, not shared from; no symbol survives in sources, tests, UI tests or the pbxproj, and the UI tests match on label BEGINSWITH 'Heading level', which the surviving row still produces. M6 test gaps — all four closed: collapsedHeadingExpansionReassertsTarget drives the setSectionState half and would fail if collapsibleHeadingAncestorIds returned [], both negative tests end in a positive control, and cancellation has a phase-1 control of its own.
emitHeading puts both ids on a heading section: id="b-{hash}-{occ}" and data-prism-section-id="{hash}-{sourceIndex}". setSectionState reads the latter (prism-theme.js:155) and its changedIds are therefore composites; setDetailsState reads section.id (prism-theme.js:212) and its changedIds are DOM ids. navigationTargetAncestorIds returns [domID].concat(collapsibleHeadingAncestorIds(element)).concat(detailsAncestorSectionIds(element)); the first is DOM space, the second walks j < targetIndex and so cannot contain the target, the third is DOM space and returns [] for every target production reaches today. The target's own composite appears nowhere. SectionCollapseManager.expandSectionAndAncestors removes sectionId then subtracts expandAncestors(of:), so with no collapsed ancestor the diff is exactly [ownComposite]. applySectionVisibility hides following siblings but not the collapsed heading itself, so the target is rendered and performScrollToBlock takes no T-1944 fallback — the failure is purely the clamp, which is the second of the two the header comment names. Pre-existing, since the mechanism is new; the defect is that the note and Decision 14 both describe the chain as beginning with the target.
armsReassert replay pathStructurally sound. WebDocumentStateSnapshot.apply is the sole writer of both scrollTargetBlockID and scrollTargetArmsReassert and writes them in the same case arm; coalescedCommands is the sole reader and emits them together; nothing resets either independently; latestSnapshot is never rebuilt piecewise; the default true is only reachable when the id is nil, when no command is emitted. One weak justification, not a defect: the field's comment calls the replay "inert in practice, since coalescedCommands orders the scroll LAST and no display-state push follows it", but dispatch(_:) sends every command in its own fire-and-forget Task, so arrival order is precisely what T-2028 says it is not. The outcome is benign — a re-assert after a recovery lands on the target the replay just scrolled to — but the reasoning leans on the guarantee this ticket exists to work around.
announce:false auditThree listeners hang off notifyExplicitNavigation. navigationTarget (scroll.js:707) is deliberately not cleared by a re-assert and is deadline-checked on every read, so a dangling target is unreachable rather than stale, and the deadline is genuinely never extended. revealPending (search.js:359) is correctly untouched: an older reveal was cleared by the announcing initial scroll, and a newer one delivered later on the same hook calls notifyExplicitNavigation itself at search.js:393/413 and takes the claim — newest intent wins in both directions. reflowAnchor (scroll.js:448) is the residual: captureReflowAnchor fires only from applyTypography (prism-theme.js:94-98 is explicit that section and details pushes must not re-anchor), so the hole needs a typography push with a real variable change landing inside the 1 s window after the initial navigation dropped the old anchor, whereupon closeReflowWindow would restore the pre-re-assert position. Very narrow; worth naming in the comment.
changedIds truthiness is untested — [] is truthy so an empty diff evaluates the guard and is correctly suppressed, undefined skips it and fails open, and both halves turn on a JS subtlety one refactor away from inverting. pointerdown, the cancel-set member this commit added, is not exercised (only wheel), though dispatchReaderInput is already parameterised by event type. detailsAncestorSectionIds remains unreachable in production and now says so in its own comment, which is the better resolution than deleting it.
Fully implemented: nested headings listed and navigable on both surfaces; the compositeId index-space fix, now consumed and pinned by a second-details-block fixture and by DetailsExpansionCoordinator.registerDetails' matching path format; ordering immunity for the details path including the descendant case; Req 8.2's chevron and extra indent on both surfaces; the VoiceOver value for both row kinds that have a disclosure. Partially implemented: Req 8.3 (expansion exact, scroll lands on the outermost enclosing <details>, acknowledged and tracked as T-1924); ordering immunity for the plain collapsed-heading path (the chain gap above). Correctly out of scope: T-2028's native command ordering; a shared composite-id constructor across the twelve sites that still spell the format by hand.
prism/Resources/WebRenderer/prism-scroll.js
Why it matters. This is the round-2 gate, the round-3 descendant fix, and the one place a bug remains. The chain mixes MarkdownSection composites and BlockDOMID DOM ids on purpose — block hashes are dash-free hex, so they cannot collide — and each command's changed ids can only match their own half. But the target's own entry is contributed only in DOM space, and collapsibleHeadingAncestorIds walks strictly j < targetIndex, so no setSectionState push about the target itself can ever match.
prism/ViewModels/WebDocumentController.swift
Why it matters. The round-1 finding was that restoreScroll issues the identical scrollToBlock an explicit navigation does, so opening a document armed 1 s of re-assert for a position nobody chose. The fix splits them with a defaulted enum payload — which puts a second value beside scrollTargetBlockID in the recovery snapshot, and a snapshot pair that can drift is how a restore gets silently promoted into an arming navigation on the page after a crash.
What to look at. WebBridgeContract.swift:240-251, WebDocumentController.swift:639, :1529-1538, :1567-1576, :1601-1608
prism/Resources/WebRenderer/prism-scroll.js
Why it matters. The most consequential of the four functional fixes, because it changes what a re-assert does to three subsystems at once. notifyExplicitNavigation means "a new position authority has taken over" and its listeners drop deferred intent on that basis. Injection order puts prism-scroll's listener before prism-search's on the shared hook, so an announcing re-assert cleared revealPending out from under a reveal that was NEWER than the target it was re-scrolling to.
What to look at. prism-scroll.js:474-507, :692-705; prism-search.js:359, :393, :480-487
prism/Resources/WebRenderer/prism-scroll.js
Why it matters. The round-2 gate asked whether a push touched the target's ANCESTORS. The clamping failure is relieved by any expansion that grows the document, including one inside the target: nested <details> with the outer already open yields changedIds = [inner] against a chain of [outer, ...headings], so the clamp stood. Production-reachable, not theoretical — DetailsExpansionCoordinator.registerDetails recurses with domID: "{parent}-d{childIndex}", and navigateToHeading expands both ancestors.
What to look at. prism-scroll.js:643-686; DetailsExpansionCoordinator.swift:186-206; DocumentSession.swift:424-427
prism/Views/TableOfContentsSheet.swift
Why it matters. Round 1's fifth major was that the PR copy-pasted from unreachable code rather than deleting it, leaving comments pointing at code no user could reach. The resolution went further than asked: the sheet's flat .entries mode went with the row, and TableOfContentsSheet is now a 70-line wrapper around SidebarContentsView. Req 8.2's extra indent lands on both surfaces because there is only one place left to put it.
What to look at. TableOfContentsSheet.swift (+32 / -105), TOCEntryRow.swift (-172), TOCEntryRowTests.swift (-171), SidebarContentsView.swift:163-196
prismTests/WebRendering/WebReloadNavigationClaimTests.swift
Why it matters. Adding a payload to scrollToBlock is exactly the change that turns !pendingCommands.contains(.scrollToBlock(domID: x)) from a real assertion into a guaranteed pass, because the restore now carries armsReassert: false while the test spells the defaulted true. Three suites were converted to match on the id alone, each with a comment naming the trap; sweeping the remaining sites confirms they compare default-to-default or assert through latestSnapshot.scrollTargetBlockID.
What to look at. WebReloadNavigationClaimTests.swift:190-200, WebSearchScrollOwnershipTests.swift:433-446, WebFragmentNavigationPrecedenceTests.swift:143-149
Four alternatives with rejection reasons — fixing T-2028 natively, widening onSectionVisibilityChanged, re-asserting on any push (the first shipped version), and comparing payload membership instead of a diff — plus four positive and four negative consequences. The negatives are honest: T-2028 stays open, there is now a second time-bounded scroll authority in the page, the chain mixes two id spaces, and NAVIGATION_REASSERT_MS has no derivation. Conforms to rules/references/decision-log-format.md on every checklist item.
The table sits directly under the file header and before Decision 1, on an independent Q sequence. Q1 (the 1 s window) has no nameable alternative and says so. Q2 (delete rather than share) has an obvious winner. Q3 — dimming follows document collapse while the accessibility value follows the disclosure — does name an alternative and does have a user-visible consequence, so a full entry would also have been defensible; keeping it in the table is a judgement call, not a misapplication of the rule.
The non-standard ### Notes H3 is folded into Context, the title is down to seven words, and Impact now lists the JS files, both ViewModel files and the agent note. The residue is duplication rather than deviation: that Impact list restates Decision 14's own, and Q1's rationale restates Decision 14's fourth negative consequence nearly word for word.
Recorded as Q3 and restated at SidebarContentsView.swift:151-156. Right for both row kinds that have a disclosure. The leaf case is where it strains: isRowExpanded returns true unconditionally for a childless node, so a top-level heading with no sub-headings whose section is collapsed announces "Expanded" while rendering at 0.5 opacity — where before this commit it announced "Collapsed".
The default keeps every existing call site and test spelling unchanged and confines the opt-out to restoreScroll. There are exactly two production constructors, so what the default actually buys is unedited test call sites — and three of those had to be hand-fixed anyway, one of which would have failed vacuously rather than loudly. It also makes the riskier behaviour, arming, the implicit one.
Only applyTypography raises notifyReflowImminent — setSectionState and setDetailsState deliberately do not, per prism-theme.js:94-98 — so the anchor can only be live during a re-assert if a typography push captured a fresh one inside the window. The trade is right; the comment claiming a re-assert "leaves every other authority exactly as it found it" is the part that overstates.
| Severity | Area | Finding | Resolution |
|---|---|---|---|
| major | prism/Resources/WebRenderer/prism-scroll.js:635 (navigationTargetAncestorIds) | The target's own heading-section composite is in the relevance chain nowhere, so a setSectionState push about the target itself can never re-assert. The chain is [domID] + collapsibleHeadingAncestorIds + detailsAncestorSectionIds. domID is a DOM id (b-{hash}-{occ}) and only ever matches setDetailsState, which keys on section.id (prism-theme.js:212); collapsibleHeadingAncestorIds walks strictly j < targetIndex so it excludes the element itself; detailsAncestorSectionIds is DOM space and returns [] for every target production reaches. But setSectionState's changedIds are data-prism-section-id composites (prism-theme.js:155, emitted at BlockHTMLEmitter.swift:410). Reachable path: TOC-select a collapsed heading that has no collapsed ancestor — SectionCollapseManager.expandSectionAndAncestors removes that id and then subtracts ancestors, so changedIds is exactly [ownComposite]. A collapsed heading is not itself hidden (applySectionVisibility pushes its level AFTER the hidden test), so there is no T-1944 fallback and the target resolves fine; the failure is purely the CLAMP, the second of the two the header comment at :525-527 names. Expanding the target's own section grows the document below it exactly as a descendant <details> does — this is the same ancestor-only asymmetry round 3 correctly fixed via isDescendantDetailsID, reached one case earlier. Not a regression (the path behaves as it did pre-branch, because the mechanism is new); what makes it major is that both docs/agent-notes/webview-rendering-status.md and Decision 14 describe the chain as '[target] + ...', which a future reader will read as covering the target's own collapse. Round 1's second major was an agent note asserting an invariant the code did not have. | Add element.getAttribute('data-prism-section-id') to navigationTargetAncestorIds, and add a fixture beside collapsedHeadingExpansionReassertsTarget: a trailing heading collapsed with no collapsed ancestor, scroll clamped, then setSectionState([]) must re-assert. One line plus one test. |
| minor | docs/agent-notes/typography-font-settings.md:136, docs/accessibility-review.md:65 | Deleting prism/Views/TOCEntryRow.swift left two CURRENT-STATE documents naming it with line numbers. The typography note lists it as a chrome surface 'currently using .chromeFont'; the accessibility review cites TOCEntryRow.swift:101-119 for macOS TOC font sizes. Both now describe a file that does not exist, and both describe behaviour that moved to SidebarContentsView.TOCSectionRow (whose ramp is three cases, not six). This is round 1's second major repeated at a smaller scale, and created by this commit. The historical spec references (specs/table-of-contents/design.md, specs/ui-improvements/design.md, specs/collapsible-headers/design.md, specs/a11y-semantic-chrome-fonts/*) are a different matter — those are records of what was decided then and may name deleted files. | Repoint both lines at prism/Views/SidebarContentsView.swift, where TOCSectionRow now owns styleForLevel/weightForLevel and the chevron. Two lines. |
| minor | prism/Resources/WebRenderer/prism-scroll.js:549-551 | The 'Bounded on FOUR sides' summary — the comment a reader reaches first, and the one the agent note points at — still says 'a display-state push only re-asserts when it touched an ANCESTOR of the pending target (below)'. The same commit added isDescendantDetailsID three functions below, so the chain now also accepts <details> descendants. The per-function comments at :643 and :676 are correct and thorough; only the summary was left behind them. | Amend the fourth bullet to 'an ancestor of, or a <details> nested inside, the pending target' — and, with the major above fixed, 'the target's own section' too. |
| minor | prism/Views/SidebarContentsView.swift:127-131, :195 | The VoiceOver fix moved a smaller inaccuracy onto a different row kind. isRowExpanded returns true unconditionally for any node with no children, so a TOP-LEVEL heading with no sub-headings whose document section IS collapsed now announces 'Expanded'; before this commit the value came from isCollapsed and it announced 'Collapsed'. The row still renders at 0.5 opacity, so for that row the visible cue and the spoken value now contradict each other. The doc comment shows it was considered ('A row with no children has no disclosure at all; true keeps it out of the collapsed-looking state') — but a row with no disclosure arguably should carry no accessibilityValue at all, which is a stronger answer than announcing a state it does not have. | Apply .accessibilityValue only when node.children is non-empty; or, if a leaf's document-collapse state is worth announcing, fold isCollapsed back into that branch so the value and the opacity agree. |
| minor | prism/Services/TOCCoordinator.swift:176-232 vs prism/Services/DetailsExpansionCoordinator.swift:170-235 | The <details> ancestor path is computed twice, by hand, and the two must agree exactly or openDetails(matching:) resolves nothing. Both walk blocks producing '{block.id}-{topLevelIndex}' at depth 0 and '{path}/{childIndex}' nested — TOCCoordinator in buildTOCEntries, DetailsExpansionCoordinator in traverseForAncestors/registerDetails. That divergence IS T-1928's root cause: TOCCoordinator's depth-0 spelling had drifted to runningIndex. This commit restores agreement by editing one side and adding a comment explaining the two index spaces; it does not make the pair structurally incapable of drifting again, and nothing fails if they diverge because the symptom is a silent lookup miss. | Follow-up ticket rather than this PR: have TOCEntry.detailsAncestorIds read from DetailsExpansionCoordinator.ancestorMap, or derive both from one shared path walker, so there is one producer of the format and a miss is impossible rather than merely tested against. |
| minor | prism/Models/TOCNode.swift:79-128 vs prism/Services/MarkdownSectionBuilder.swift:18-70 | TOCNode.build duplicates MarkdownSectionBuilder.build: the same heading-level stack, the same 'pop while last.level >= candidate.level', the same mutable class node frozen into a value tree. TOCNode adds exactly one predicate — the detailsAncestorIds prefix rule — and inherits none of the other's hardening (MarkdownSectionBuilder freezes iteratively with an explicit post-order traversal to avoid stack growth; TOCNode.freeze recurses, justified by depth <= 6, which is correct but is a second answer to a question already answered). Decision 13 argues convincingly for two MODELS; it does not argue for two BUILDERS, and the distinction matters because the shared part is the part with the subtle loop. | Follow-up: a shared heading-hierarchy builder parameterised on (level, canParent), with MarkdownSectionBuilder and TOCNode supplying their own predicate and node type. |
| minor | prism/ViewModels/WebBridgeContract.swift:251 | case scrollToBlock(domID: String, armsReassert: Bool = true) — a default value on an enum case payload. It is not a trap for exhaustive matching (case .scrollToBlock(let id, _) still binds both) but it is one for EQUALITY: .scrollToBlock(domID: x) now means 'the arming variant', so every ==, contains(_:) and firstIndex(of:) against the plain spelling silently stops matching a restore. Three such sites had to be hand-fixed in this commit, one of which would have failed vacuously rather than loudly. There are exactly two production constructors (WebDocumentController.swift:602 and :639), so the default buys nothing but unedited test call sites — while making the riskier behaviour, arming, the implicit one. | Drop the default and have both production sites state intent, or lift the pair into a named ScrollTarget type so the value cannot be half-specified. Either removes the equality footgun for good. |
| minor | prism/ViewModels/WebDocumentController.swift:1529-1538 | Two issues on one field. (1) scrollTargetArmsReassert is only meaningful when scrollTargetBlockID != nil, yet it defaults to true, participates in Equatable in that meaningless state, and has to be reasoned about separately — a var scrollTarget: (id: String, armsReassert: Bool)? (or a two-field struct) would make the invariant structural and delete the dead default. (2) Its comment says the replay is 'Inert in practice, since coalescedCommands orders the scroll LAST and no display-state push follows it to re-assert on' — but dispatch(_:) sends every command in its own fire-and-forget Task, so arrival order at the page is exactly what T-2028 says it is not. The behaviour is fine either way (a re-assert lands on the target the replay just scrolled to), so this is a justification problem, not a defect: the field is mechanically correct and the pair provably cannot desynchronise. | Group the pair into one optional, and replace 'inert' with the accurate reason: arming on the replay is harmless because the re-assert lands on the target the replay just scrolled to, not because ordering rules out a following push. |
| minor | specs/table-of-contents/design.md, specs/collapsible-sections/tasks.md:165-206 | Round 1's documentation minor was skipped without comment, and this PR is the evidence that part of it is now demonstrably false. design.md still maps '3.1-3.8 | TOCEntryRow view with styling', documents struct TOCEntryRow at :132-142, describes DocumentSession caching tocEntries at :58, and shows a File Structure at :402 with TOCEntryRow.swift and no TOCNode.swift — seven places, in the design document for the feature whose decision log this PR writes two entries to. Separately, specs/collapsible-sections/tasks.md still ticks tasks 15-19, including 17 ('Add additional indentation for details headings', delivered for the first time by THIS commit) and 18 ('Implement TOC navigation with expansion', never reachable from either surface). Neither the report nor Decision 13/14 records that those ticks were wrong. | At minimum: point table-of-contents/design.md at TOCNode and at collapsible-sections Req 8.x as the owning requirement, and un-tick tasks 17 and 18. A tasks file claiming delivered work that the ticket exists to deliver is worse than an out-of-date design document. |
| minor | prism/Resources/WebRenderer/prism-scroll.js:474-485 vs :448 | Going quiet also stops a re-assert dropping the typography reflow anchor — the third listener on notifyExplicitNavigation and the one the comment does not mention. Reachability is narrow: captureReflowAnchor fires only from applyTypography (prism-theme.js:94-98 is explicit that section and details pushes must NOT re-anchor), and the initial navigation still announces and drops whatever anchor existed. The hole needs a typography push with a real variable change landing inside the 1 s window AFTER the navigation — a Dynamic Type or text-size change within a second of a TOC selection — whereupon closeReflowWindow restores the pre-re-assert position and undoes the re-assert. The trade is the right one; the claim that a re-assert 'leaves every other authority exactly as it found it' is what overstates, since one authority can now outlive it. | Name the reflow anchor in the :474 comment as the deliberate exception, or drop it explicitly on a re-assert — unlike revealPending it is not deferred intent, it is a position measured for a layout that has since been replaced. |
| minor | specs/bugfixes/toc-details-headings/report.md:280-296 | The Run command selects TOCDetailsHeadingsTests, TOCProductionWiringTests, TOCCoordinatorTests, TOCEntryTests and WebDetailsNavigationOrderingTests — but not TOCNavigationDetailsIndexTests, the T-1662 index-space suite that drives the real toc.tocEntries producer and is the closest existing coverage of the runningIndex -> blockIndex composite fix this branch makes. The ticket's own reproduction recipe omits the suite most exposed to its riskiest line. (I ran it: 58/58 green alongside TOCProductionWiringTests, TOCEntryTests and DetailsExpansionCoordinatorTests.) | Add -only-testing:prismTests/TOCNavigationDetailsIndexTests and -only-testing:prismTests/DetailsExpansionCoordinatorTests to the run command. |
| nit | prism/Services/TOCCoordinator.swift:83-86 | tocTree opens with a bare `_ = cachedBlocks` and a five-line comment explaining that deleting it would silently stop the TOC re-rendering after a re-parse. Correct, but fragile-by-comment: the line has no local effect and is exactly what a tidy-up removes. `let entries = tocEntries` at the top registers the same dependency through the sibling getter, is self-evidently not deletable, and costs one array retain on a cache hit. The @ObservationIgnored decision itself is right and well argued. | Replace the discard with `let entries = tocEntries` and build from `entries`. |
| nit | prism/Models/TOCEntry.swift:15-20, :105-121, :130-149 | Three carried items in one file. (1) Two initializers are now near-identical — six parameters each, bodies differing only in detailsAncestorIds = [] — and the first has no production caller at all (TOCCoordinator.swift:193 uses the second; everything else is previews and tests). (2) scrollTargetId and topLevelBlockIndex remain two correlated optional trailing parameters that default independently via ?? blockIndex, so a caller can still pass one without the other; the #Preview that got this wrong was fixed (SidebarContentsView.swift:248-262 now passes topLevelBlockIndex: 1) but the shape that allowed it was not. (3) The 'Requirements covered:' block still mixes 3.1/3.2/4.1 (table-of-contents) with 8.1/8.4 (collapsible-sections) and names no spec path — the exact ambiguity fixed thoroughly in TOCNode.swift next door. | Collapse to one init with detailsAncestorIds: [String] = [], make the correlated pair non-optional or a single value type, and qualify both requirement groups with their spec path. |
| nit | prism/Services/TOCCoordinator.swift:176 and eleven other sites | Carried from round 1, half-addressed. The in-loop duplication is gone — topLevelComposite is hoisted and spelled once per iteration, which is the 'free version' the round-1 finding offered and exactly what would have prevented T-1928. The cross-file duplication that made the defect possible remains: '{blockId}-{sourceIndex}' is still hand-spelled at MarkdownSection.swift:26/:170/:235/:239/:242/:247/:305, DetailsExpansionCoordinator.swift:173, TOCCoordinator.swift:176, BlockHTMLEmitter.swift:410, SearchMatch.swift:42, SearchOverlaySheet.swift:242, DocumentSession.swift:770 and RelocationEngine.swift:323 — with no shared constructor, even though BlockDOMID.restoreDOMID already parses the format. | Follow-up ticket: BlockDOMID.compositeID(blockID:sourceIndex:) plus the matching parse, with MarkdownSection.id as the one definition, and route every site through it. |
| nit | prismTests/WebRendering/WebDetailsNavigationOrderingTests.swift:286-345, :463-494 | Two test-shape observations. (1) pointerdown — the cancel-set member this commit ADDED, and the one that answers the chevron / <summary> case the round-1 finding was about — is not exercised; readerInputCancelsThePendingReassert dispatches 'wheel' only, and dispatchReaderInput is already parameterised by event type, so a second phase costs one line. NAVIGATION_CANCEL_KEYS / keydown and the changedIds truthiness contract ([] truthy and correctly suppressing, undefined failing open) are likewise untested although the code comments treat both as load-bearing. (2) The positive controls are the right fix for round 1's vacuity finding, but they sit at the END of an already tight budget — unrelatedDetailsPushDoesNotReassertTarget spends five WebKit round-trips between arming the 1 s window and the control that proves it was still armed. Under contention the suite now fails loudly instead of passing vacuously, which is the right direction, but the timing dependency was made visible rather than removed. | Parameterise the cancellation test over ['wheel', 'pointerdown', 'touchmove']. Optionally add a debug-only seam over NAVIGATION_REASSERT_MS so the negative tests assert the gate without racing the deadline. |
| nit | specs/table-of-contents/decision_log.md:7, :310, :344, :512-525 | Four small decision-log residues. Decision 13's rebuilt Impact enumerates Decision 14's files, duplicating Decision 14's own Impact at :650-659 and letting D13 claim scope it does not own; Q1's Rationale restates Decision 14's fourth Negative consequence nearly word for word. Separately, Decision 10 (12 pt per level) survives only as the new detailsIndent constant now that level indentation is structural DisclosureGroup nesting, and Decision 11 (macOS arrow-key navigation within the TOC sheet) has no implementation left — its @FocusState went with the deleted .entries mode. That one is fair: the .entries mode was already unreachable on origin/main (CompactDocumentLayout used the structure: init), so this commit deleted a corpse rather than shipping behaviour; but both decisions are still marked accepted. | Cross-reference instead of copying in D13's Impact and Q1's Rationale, and either supersede Decisions 10/11 or note in Decision 13's Consequences what the compact sheet's presentation now is. |
| nit | prism/Models/DocumentSession.swift:424-427; prism-scroll.js:486, :692, :667 | Four small shape items, none a defect. navigateToHeading loops expansionCoordinator.expand over every element of detailsAncestorIds, but expand already unions ancestorMap[blockId] itself, so expand(blockId: detailsAncestorIds.last) does the whole chain — the loop is O(depth^2) and hides that dependency. performScrollToBlock(domID, false) is a bare boolean at the call site and reads as nothing without opening the definition. reassertNavigationTarget treats a missing changedIds as fail-OPEN while registerCommand('scrollToBlock') treats a missing armsReassert as fail-CLOSED; both are documented individually, neither says why they differ. And idsIntersect is O(a*b) against a chain recomputed by a full querySelectorAll plus a linear target scan on every qualifying push, when the heading structure is static for a page load and the chain could be computed once at arm time beside navigationDeadline. | All optional. The fail-open/fail-closed asymmetry is the one worth a sentence, since it is the kind of thing a later reader 'harmonises'. |
| nit | specs/bugfixes/toc-details-headings/report.md:210; prismTests/TOCDetailsHeadingsTests.swift:378, :387, :404 | The report says TOCDetailsHeadingsTests has '11 tests'; it has 12 @Test cases, and the bullet list directly beneath names all 12. Separately, TOCProductionWiringTests asserts on raw source substrings ('nodes: session.toc.tocTree', 'DisclosureGroup(isExpanded: $isNestedExpanded)'). The file is honest that this is not a proof, and the T-1943 precedent for pinning production wiring is sound, but these break on a line wrap inside the call rather than on the wiring changing. | Change 11 to 12. Optionally match the wiring assertions on a whitespace-tolerant regex. |
Click to expand.
diff --git a/prism/Resources/WebRenderer/prism-scroll.js b/prism/Resources/WebRenderer/prism-scroll.jsindex 53dcd214..fb31210b 100644--- a/prism/Resources/WebRenderer/prism-scroll.js+++ b/prism/Resources/WebRenderer/prism-scroll.js@@ -471,17 +471,28 @@ return null; } - bridge.registerCommand("scrollToBlock", function (payload) {- if (!payload || !payload.domID) { return null; }- var element = document.getElementById(payload.domID);- if (!element) { return null; }+ // `announce` (T-1928) is false for a RE-ASSERT of the target already in+ // flight, and true for every genuinely new navigation. The difference is+ // not cosmetic: `notifyExplicitNavigation` means "a NEW position authority+ // has taken over", and its listeners drop deferred intent on that basis —+ // prism-search.js clears `revealPending`, so a reveal waiting on a section+ // expansion is discarded with no recovery. Injection order is bridge,+ // scroll, theme, media, notes, search, so on a shared+ // `notifySectionVisibilityChanged` the re-assert here runs BEFORE the+ // search listener reads `revealPending` — a re-assert that announced would+ // silently swallow a reveal that is NEWER than the target it is re-scroll-+ // ing to. A re-assert is the SAME intent landing again, not a new one, so+ // it stays quiet and leaves every other authority exactly as it found it.+ function performScrollToBlock(domID, announce) {+ var element = document.getElementById(domID);+ if (!element) { return false; } // scrollIntoView on a display:none element does NOTHING, so a restore // (or navigation) into a since-collapsed block or the hidden // frontmatter carrier used to silently lose the position (T-1944). // Land on the nearest rendered section instead. if (!bridge.isSectionRendered(element)) { element = nearestRenderedSection(element);- if (!element) { return null; }+ if (!element) { return false; } } // This scroll is the newest position authority (TOC/fragment/note // target, or the stored-position restore): tell listeners so deferred@@ -489,10 +500,268 @@ // is discarded instead of firing later and clobbering it (T-1944). // Notified only when the scroll actually executes: a target that // cannot be resolved establishes no new position.- bridge.notifyExplicitNavigation();+ if (announce) { bridge.notifyExplicitNavigation(); } beginProgrammaticScroll(); element.scrollIntoView({ block: "start", behavior: "auto" }); return true;+ }++ // ---- Navigation target re-assertion (T-1928) -------------------------+ //+ // Selecting a TOC heading that lives inside a closed <details> reaches this+ // page as up to THREE independent bridge commands — setSectionState,+ // setDetailsState and scrollToBlock. WebDocumentController.dispatch sends+ // each in its own fire-and-forget `Task { await page.callJavaScript(…) }`,+ // so nothing awaits the state commands before the scroll and their arrival+ // order here is unspecified (the open T-2028 race). No user-reachable path+ // depended on that order before T-1928 — a details-nested heading was simply+ // not listed — so this is the first one that does.+ //+ // Landing the scroll FIRST is not harmless, in two distinct ways:+ // - Under a collapsed heading the target section is display:none, so the+ // scroll resolves to the nearest RENDERED section. That fallback is right+ // for a stale stored position (T-1944) and wrong for a navigation whose+ // expansion is still in flight.+ // - A target near the document end is clamped by the CURRENT document+ // height. Opening the <details> afterwards makes the document taller but+ // does not move the reader, leaving the target short of the viewport top.+ //+ // Rather than ordering the commands natively (T-2028's job, and a much+ // larger change), the page makes the order IRRELEVANT: the navigation target+ // is remembered for a short window and re-asserted whenever a display-state+ // command applies inside it. Whichever of the three lands last, the reader+ // ends up on the target.+ //+ // Only an EXPLICIT navigation arms the window (`payload.armsReassert`); the+ // stored-position restore issues the same command and must not, because+ // nobody chose that position.+ //+ // Bounded on FOUR sides, so a stale intent can never fire:+ // - a fixed deadline measured from the ORIGINAL scroll and never extended+ // by a re-assert (a re-assert is not new intent, so it must not buy more+ // time — otherwise a stream of state pushes could hold it open forever);+ // - any explicit navigation drops it, exactly as it drops the reflow anchor+ // and prism-search.js's deferred reveal: the newest authority wins;+ // - a scroll the page did not initiate drops it too, as does any direct+ // reader input (wheel/touchmove/pointerdown/navigation key) — that is+ // the reader moving themselves, and they outrank a target still+ // settling;+ // - a display-state push only re-asserts when it touched an ANCESTOR of+ // the pending target (below) — a push about something else entirely is+ // not evidence the target needs re-scrolling.+ //+ // ROUND-2 RESIDUAL (post-review, still T-1928): the original version+ // re-asserted on ANY setSectionState/setDetailsState push while the window+ // was open, with no way to tell a push that belongs to the pending+ // navigation from one that doesn't. Two concrete failures followed:+ // (a) the reader toggles a DIFFERENT, unrelated <details> — that toggle+ // round-trips to native and echoes back as its own setDetailsState,+ // which re-triggered the old unconditional re-assert and yanked the+ // reader back to the earlier TOC target;+ // (b) two TOC navigations fired within the ~1s window — the second+ // navigation's state commands (which don't call+ // notifyExplicitNavigation; only scrollToBlock/scrollToEdge/+ // scrollByPage/search-reveal do) could land before its OWN+ // scrollToBlock, and the still-live first target got re-asserted in+ // between, producing a transient jump backwards.+ // Both are fixed the same way: `changedIds` (the ids whose collapsed/+ // expanded membership actually flipped this push — see prism-theme.js's+ // diffIdSets) is compared against the pending target's ancestor chain,+ // computed fresh from the DOM. A push whose changed ids share nothing+ // with that chain cannot affect whether the target is reachable or+ // clamped, so it is ignored.+ var NAVIGATION_REASSERT_MS = 1000;+ var navigationTarget = null;+ var navigationDeadline = 0;++ // The section ids in the heading level-stack spanning `element`'s position+ // — the SAME stack `applySectionVisibility` (prism-theme.js) walks to+ // decide what a collapsed heading hides — EXCLUDING `element` itself.+ // These are the `setSectionState` collapsedIDs that could hide `element`+ // if collapsed.+ function collapsibleHeadingAncestorIds(element) {+ var sections = document.querySelectorAll("main[data-prism-document] section[data-prism-block-id]");+ var targetIndex = -1;+ for (var i = 0; i < sections.length; i++) {+ if (sections[i] === element) { targetIndex = i; break; }+ }+ if (targetIndex < 0) { return []; }+ var stack = [];+ for (var j = 0; j < targetIndex; j++) {+ var section = sections[j];+ var levelAttr = section.getAttribute("data-prism-heading-level");+ if (!levelAttr) { continue; }+ var level = parseInt(levelAttr, 10);+ while (stack.length && stack[stack.length - 1].level >= level) { stack.pop(); }+ var sectionId = section.getAttribute("data-prism-section-id");+ if (sectionId) { stack.push({ level: level, id: sectionId }); }+ }+ var ids = [];+ for (var k = 0; k < stack.length; k++) { ids.push(stack[k].id); }+ return ids;+ }++ // The section ids of every <details> ancestor CONTAINING `element` (not+ // `element` itself) — the `setDetailsState` expandedIDs that gate whether+ // `element` is laid out at all. Mirrors+ // DetailsExpansionCoordinator.detailsAncestorIds on the native side.+ //+ // DEFENSIVE, and returns [] for every target production reaches today:+ // every scrollToBlock domID is resolved by BlockDOMID.navigationDOMID+ // against the TOP-LEVEL blocks array, and a top-level section is a direct+ // child of <main data-prism-document>, so `element.parentElement` is that+ // <main>, `main.closest("details")` is null, and the loop exits on its+ // first iteration. It is kept because the id space it walks is real — a+ // nested <details> DOES emit its own section (id "{domID}-d{childIndex}",+ // BlockHTMLEmitter.emitDetails) — so the day T-1924 makes a nested block a+ // navigation target, the chain is already correct.+ function detailsAncestorSectionIds(element) {+ var ids = [];+ var node = element.parentElement;+ while (node) {+ var details = node.closest("details");+ if (!details) { break; }+ var section = details.closest("section[data-prism-block-id]");+ if (section && section.id) { ids.push(section.id); }+ node = details.parentElement;+ }+ return ids;+ }++ // Every id whose collapse/expand state can affect whether `domID` is+ // reachable or correctly positioned: the target's OWN id (a details-block+ // target's own open/closed state is what un-clamps IT — T-1928's original+ // fixture), plus its collapsible-heading and <details> ancestors above.+ function navigationTargetAncestorIds(domID) {+ var element = document.getElementById(domID);+ if (!element) { return []; }+ return [domID]+ .concat(collapsibleHeadingAncestorIds(element))+ .concat(detailsAncestorSectionIds(element));+ }++ // Whether `changedId` names a <details> section nested INSIDE the target.+ //+ // The ancestor chain alone is not sufficient, because the second failure+ // this mechanism exists to fix — a target near the document end clamped by+ // the CURRENT document height — is relieved by any expansion that makes the+ // document taller, INCLUDING one inside the target itself. Concretely:+ // nested <details> (outer "b-X-0", inner "b-X-0-d0"), a TOC heading inside+ // the inner one, outer already open. navigateToHeading flips only the inner,+ // so changedIds is ["b-X-0-d0"] while the chain is ["b-X-0", …headings] —+ // no intersection, and the clamped scroll would stand.+ //+ // Matched by id prefix rather than DOM containment because it must work in+ // BOTH id spaces the chain mixes: a DOM id resolves through+ // getElementById, but a setSectionState composite ("{hash}-{sourceIndex}")+ // is not an element id at all and would silently resolve to nothing. The+ // nested form is exactly `{parentDOMID}-d{childIndex}` at every depth+ // (BlockHTMLEmitter.emitDetails), and a top-level DOM id is+ // `b-{hex}-{occurrence}` — no `-d` segment — so the prefix cannot collide+ // with a sibling or an unrelated block.+ function isDescendantDetailsID(changedId, domID) {+ return changedId.length > domID.length+ && changedId.indexOf(domID + "-d") === 0;+ }++ function idsIntersect(a, b) {+ for (var i = 0; i < a.length; i++) {+ for (var j = 0; j < b.length; j++) {+ if (a[i] === b[j]) { return true; }+ }+ }+ return false;+ }++ // Whether this push can have changed whether `domID` is reachable or+ // correctly positioned: it touched the target, an ancestor of it, or a+ // <details> nested inside it.+ function changedIdsAffectTarget(changedIds, domID) {+ var chain = navigationTargetAncestorIds(domID);+ if (idsIntersect(changedIds, chain)) { return true; }+ for (var i = 0; i < changedIds.length; i++) {+ if (isDescendantDetailsID(changedIds[i], domID)) { return true; }+ }+ return false;+ }++ // `changedIds` is omitted by callers that have no such list to offer+ // (there are none today, but a future caller failing open — re-asserting+ // rather than silently going stale — is the safer default than a missing+ // argument silently suppressing every re-assert).+ function reassertNavigationTarget(changedIds) {+ if (!navigationTarget) { return; }+ var domID = navigationTarget;+ var deadline = navigationDeadline;+ if (Date.now() > deadline) { navigationTarget = null; return; }+ if (changedIds && !changedIdsAffectTarget(changedIds, domID)) { return; }+ // Scrolls WITHOUT announcing an explicit navigation (see+ // performScrollToBlock): a re-assert is the same intent landing again,+ // so it neither clears its own target nor discards anyone else's+ // deferred position intent. `navigationTarget` and `navigationDeadline`+ // are therefore left exactly as they were — a second state command in+ // the same window still re-asserts, and the deadline is never extended.+ performScrollToBlock(domID, false);+ }++ bridge.onExplicitNavigation(function () { navigationTarget = null; });+ bridge.onSectionVisibilityChanged(reassertNavigationTarget);+ bridge.onDetailsStateApplied(reassertNavigationTarget);++ window.addEventListener("scroll", function () {+ if (!scrollReportsSuppressed()) { navigationTarget = null; }+ }, { passive: true });++ // Unambiguous user-intent cancellation (T-1928 round-2 residual,+ // part (c)). The generic `scroll` listener above is gated on+ // !scrollReportsSuppressed() because "scroll" fires for BOTH a+ // programmatic scrollIntoView/scrollTo AND a genuine user scroll, and the+ // 600ms post-programmatic-scroll suppression window exists so the page's+ // own settle doesn't read as the reader cancelling their own navigation.+ // But that same gate hides a genuine scroll-away if the reader moves fast+ // enough — a wheel flick or arrow key within 600ms of the scroll landing+ // is real intent, not an echo of it. wheel/touchmove/keydown never fire+ // for a scripted scroll, only for actual input devices, so they are NOT+ // gated: any one of them is unconditional evidence the reader is scrolling+ // themselves, and outranks a still-open re-assert window immediately.+ //+ // `pointerdown` is in the set for the same reason, and covers the case the+ // scroll events cannot: a mouse click on a collapse chevron or a <summary>+ // is equally unambiguous reader intent, and it moves the document without+ // any input-device scroll happening at all. `Object.create(null)` for the+ // key map so no KeyboardEvent.key can match through Object.prototype —+ // none does today ("constructor" is not a key value), but the guard is+ // structural rather than incidental.+ var NAVIGATION_CANCEL_KEYS = Object.create(null);+ ["ArrowUp", "ArrowDown", "ArrowLeft", "ArrowRight",+ "PageUp", "PageDown", "Home", "End", " "].forEach(function (key) {+ NAVIGATION_CANCEL_KEYS[key] = true;+ });+ function cancelNavigationTarget() { navigationTarget = null; }+ window.addEventListener("wheel", cancelNavigationTarget, { passive: true });+ window.addEventListener("touchmove", cancelNavigationTarget, { passive: true });+ window.addEventListener("pointerdown", cancelNavigationTarget, { passive: true });+ window.addEventListener("keydown", function (event) {+ if (NAVIGATION_CANCEL_KEYS[event.key]) { cancelNavigationTarget(); }+ });++ // `armsReassert` is false for the stored-position RESTORE+ // (WebDocumentController.restoreScroll) and true for an explicit+ // navigation (TOC entry, #fragment, note target). Only a navigation the+ // reader asked for may claim a second, time-bounded scroll authority:+ // opening a document would otherwise arm 1s of re-assert for a position+ // nobody chose, and a chevron or <summary> click in that second would pull+ // the reader back to it.+ bridge.registerCommand("scrollToBlock", function (payload) {+ if (!payload || !payload.domID) { return null; }+ if (!performScrollToBlock(payload.domID, true)) { return null; }+ if (payload.armsReassert) {+ navigationTarget = payload.domID;+ navigationDeadline = Date.now() + NAVIGATION_REASSERT_MS;+ } else {+ navigationTarget = null;+ }+ return true; }); bridge.registerCommand("scrollToEdge", function (payload) {
diff --git a/prism/Resources/WebRenderer/prism-theme.js b/prism/Resources/WebRenderer/prism-theme.jsindex 1d875c84..fd62cc13 100644--- a/prism/Resources/WebRenderer/prism-theme.js+++ b/prism/Resources/WebRenderer/prism-theme.js@@ -18,6 +18,25 @@ var root = document.documentElement; + // The ids present in exactly one of `previous`/`next` (a Set-shaped object+ // keyed by id, as built by setSectionState/setDetailsState below) — i.e.+ // what actually flipped between two applications of the same command, not+ // the full id list either one carries. Both commands re-send the WHOLE+ // current state on every push, so membership alone cannot tell a real+ // change from a redundant re-affirmation; only the diff can (T-1928+ // round-2 residual — see prism-scroll.js's reassertNavigationTarget).+ function diffIdSets(previous, next) {+ var changed = [];+ var id;+ for (id in previous) {+ if (Object.prototype.hasOwnProperty.call(previous, id) && !next[id]) { changed.push(id); }+ }+ for (id in next) {+ if (Object.prototype.hasOwnProperty.call(next, id) && !previous[id]) { changed.push(id); }+ }+ return changed;+ }+ // ---- Theme (Req 1.3) ------------------------------------------------- // applyTheme sets data-prism-theme (selecting a :root variable block) and // overlays any explicit variable overrides, then re-themes in-page mermaid.@@ -124,10 +143,13 @@ // against each heading section's data-prism-section-id. A collapsed heading hides the // following sibling sections until the next heading of the same/higher level — the DOM // is flat (each block is its own section), so the span is computed from heading levels.+ var previousCollapsedIds = {}; bridge.registerCommand("setSectionState", function (payload) { var collapsed = (payload && payload.collapsedIDs) || []; var set = {}; collapsed.forEach(function (id) { set[id] = true; });+ var changedIds = diffIdSets(previousCollapsedIds, set);+ previousCollapsedIds = set; var sections = document.querySelectorAll("main[data-prism-document] section[data-prism-block-id]"); for (var i = 0; i < sections.length; i++) { var sectionId = sections[i].getAttribute("data-prism-section-id");@@ -143,7 +165,7 @@ applySectionVisibility(sections); // Section layout changed: let geometric readers re-derive (T-1944 — // prism-search.js re-windows and delivers a deferred reveal here).- bridge.notifySectionVisibilityChanged();+ bridge.notifySectionVisibilityChanged(changedIds); return true; }); @@ -177,10 +199,13 @@ } // ---- Details expansion state (Req 1.6 replay) ------------------------+ var previousExpandedIds = {}; bridge.registerCommand("setDetailsState", function (payload) { var expanded = (payload && payload.expandedIDs) || []; var set = {}; expanded.forEach(function (id) { set[id] = true; });+ var changedIds = diffIdSets(previousExpandedIds, set);+ previousExpandedIds = set; var details = document.querySelectorAll("details"); for (var i = 0; i < details.length; i++) { var section = details[i].closest("section[data-prism-block-id]");@@ -188,6 +213,10 @@ if (id && set[id]) { details[i].open = true; } else if (id) { details[i].open = false; } }+ // Document height changed without a scroll event and without any section+ // becoming (un)hidden: let prism-scroll.js re-assert a navigation target+ // whose scroll may have landed before this command (T-1928).+ bridge.notifyDetailsStateApplied(changedIds); return true; });
diff --git a/prism/Resources/WebRenderer/prism-bridge.js b/prism/Resources/WebRenderer/prism-bridge.jsindex 99c6e745..d5e8e900 100644--- a/prism/Resources/WebRenderer/prism-bridge.js+++ b/prism/Resources/WebRenderer/prism-bridge.js@@ -131,13 +131,22 @@ // arrival order between feature scripts is unspecified, so state that must // survive an ordering race (a search reveal into a section whose expansion // push has not landed yet) retries here instead of assuming an order.+ //+ // `changedIds` (T-1928 round-2 residual) is the array of heading section+ // ids whose collapsed/expanded membership actually FLIPPED versus the last+ // application — not the full `collapsedIDs` payload, which restates every+ // id on every push whether or not it moved. prism-theme.js computes the+ // diff; passing the diff (not the raw payload) is what lets+ // prism-scroll.js's re-assert tell "this push touched my target's ancestor+ // chain" from "this push was for something else entirely" (see+ // reassertNavigationTarget in prism-scroll.js). var sectionVisibilityListeners = []; function onSectionVisibilityChanged(listener) { sectionVisibilityListeners.push(listener); }- function notifySectionVisibilityChanged() {+ function notifySectionVisibilityChanged(changedIds) { for (var i = 0; i < sectionVisibilityListeners.length; i++) {- sectionVisibilityListeners[i]();+ sectionVisibilityListeners[i](changedIds); } } @@ -158,6 +167,32 @@ } } + // Details-state hook (T-1928). prism-theme.js notifies after setDetailsState+ // has forced every <details> open or closed. Opening a disclosure changes the+ // laid-out height of the document with NO scroll event and NO section+ // visibility change, so a listener that has to re-derive geometry after it+ // (prism-scroll.js re-asserting a navigation target) has nothing else to hang+ // off.+ //+ // Deliberately NOT folded into notifySectionVisibilityChanged. That hook means+ // "a heading collapse changed which sections are laid out", and its only other+ // listener — prism-search.js's re-windowing and deferred-reveal delivery — is+ // scoped to exactly that. Widening it would smuggle a search behaviour change+ // into a scroll fix.+ //+ // `changedIds` (T-1928 round-2 residual): the details-block section ids+ // whose open/closed membership flipped versus the last `setDetailsState`+ // application — see the matching note on notifySectionVisibilityChanged.+ var detailsStateListeners = [];+ function onDetailsStateApplied(listener) {+ detailsStateListeners.push(listener);+ }+ function notifyDetailsStateApplied(changedIds) {+ for (var i = 0; i < detailsStateListeners.length; i++) {+ detailsStateListeners[i](changedIds);+ }+ }+ // Reflow-imminent hook (T-1965). prism-theme.js notifies IMMEDIATELY BEFORE a // command whose variables reflow the document lands (today: applyTypography — // font size, in-app scale, body family). Listeners (prism-scroll.js) capture@@ -236,6 +271,8 @@ notifySectionVisibilityChanged: notifySectionVisibilityChanged, onExplicitNavigation: onExplicitNavigation, notifyExplicitNavigation: notifyExplicitNavigation,+ onDetailsStateApplied: onDetailsStateApplied,+ notifyDetailsStateApplied: notifyDetailsStateApplied, onReflowImminent: onReflowImminent, notifyReflowImminent: notifyReflowImminent, // Generation helpers exposed for feature scripts and tests.
diff --git a/prism/ViewModels/WebBridgeContract.swift b/prism/ViewModels/WebBridgeContract.swiftindex 3d75f9bf..c984c05c 100644--- a/prism/ViewModels/WebBridgeContract.swift+++ b/prism/ViewModels/WebBridgeContract.swift@@ -237,7 +237,18 @@ enum OutboundBridgeCommand: Equatable, Sendable { case setNoteIndicators(json: String) case setInlineNotes(json: String) case setSectionState(collapsedIDs: [String])- case scrollToBlock(domID: String)+ /// Scrolls a block to the viewport top.+ ///+ /// `armsReassert` (T-1928) says whether the page may remember this target+ /// for its 1 s navigation re-assert window: true for an EXPLICIT navigation+ /// (TOC entry, `#fragment`, note target), false for the stored-position+ /// restore, which issues the identical command for a position the reader+ /// never chose — arming it there would let a chevron or `<summary>` click+ /// in the first second of a document being opened pull the reader back to+ /// it. Defaulted, so the arming case (every `scrollTo` caller, and every+ /// existing test) keeps the plain spelling and only `restoreScroll` opts+ /// out.+ case scrollToBlock(domID: String, armsReassert: Bool = true) case scrollToEdge(edge: ScrollEdge) case scrollByPage(direction: ScrollDirection)
diff --git a/prism/ViewModels/WebDocumentController.swift b/prism/ViewModels/WebDocumentController.swiftindex 1b62f2df..a79b9f4c 100644--- a/prism/ViewModels/WebDocumentController.swift+++ b/prism/ViewModels/WebDocumentController.swift@@ -464,7 +464,7 @@ final class WebDocumentController { // between the two dispatches. Keep the id check: the moment a third // producer appears, or flushing stops being synchronous, an unconditional // clear silently drops the navigation. Do not "simplify" it away.- if case .scrollToBlock(let domID) = command, domID == undeliveredNavigationTarget {+ if case .scrollToBlock(let domID, _) = command, domID == undeliveredNavigationTarget { undeliveredNavigationTarget = nil } // Same release rule for the search-reveal claim (T-1918): the reveal is on@@ -525,8 +525,8 @@ final class WebDocumentController { return ["notes": json] case .setSectionState(let collapsedIDs): return ["collapsedIDs": collapsedIDs]- case .scrollToBlock(let domID):- return ["domID": domID]+ case .scrollToBlock(let domID, let armsReassert):+ return ["domID": domID, "armsReassert": armsReassert] case .scrollToEdge(let edge): return ["edge": edge.rawValue] case .scrollByPage(let direction):@@ -636,7 +636,7 @@ final class WebDocumentController { Self.logger.debug("Restore scroll skipped: search reveal still undelivered") return }- send(.scrollToBlock(domID: blockID))+ send(.scrollToBlock(domID: blockID, armsReassert: false)) } func scrollToEdge(_ edge: ScrollEdge) {@@ -1529,6 +1529,14 @@ struct WebDocumentStateSnapshot: Equatable, Sendable { /// The last scroll target to restore after layout settles (Req 2.2). var scrollTargetBlockID: String? + /// Whether that target arms the page's navigation re-assert window+ /// (T-1928). Carried alongside the id so a replay re-issues the command the+ /// session actually made rather than silently promoting a stored-position+ /// restore into an arming navigation. Inert in practice, since+ /// `coalescedCommands` orders the scroll LAST and no display-state push+ /// follows it to re-assert on.+ var scrollTargetArmsReassert = true+ /// Folds a command into the snapshot, overwriting any previous value of the /// same kind. Transient page-scroll commands (scrollByPage, scrollToEdge) are /// not part of restorable state and are not retained.@@ -1559,8 +1567,12 @@ struct WebDocumentStateSnapshot: Equatable, Sendable { inlineNotesJSON = json case .setSectionState(let collapsedIDs): sectionCollapsedIDs = collapsedIDs- case .scrollToBlock(let domID):+ case .scrollToBlock(let domID, let armsReassert): scrollTargetBlockID = domID+ // Carried so a WebContent-recovery replay re-issues the command+ // the session actually made, rather than silently promoting a+ // restore into an arming navigation (T-1928).+ scrollTargetArmsReassert = armsReassert case .scrollToEdge, .scrollByPage: break }@@ -1589,7 +1601,11 @@ struct WebDocumentStateSnapshot: Equatable, Sendable { if let searchStateJSON { commands.append(.setSearchState(json: searchStateJSON, reveal: false)) }- if let scrollTargetBlockID { commands.append(.scrollToBlock(domID: scrollTargetBlockID)) }+ if let scrollTargetBlockID {+ commands.append(+ .scrollToBlock(domID: scrollTargetBlockID, armsReassert: scrollTargetArmsReassert)+ )+ } return commands } }
diff --git a/prism/Models/TOCNode.swift b/prism/Models/TOCNode.swiftnew file mode 100644index 00000000..09577397--- /dev/null+++ b/prism/Models/TOCNode.swift@@ -0,0 +1,128 @@+//+// TOCNode.swift+// prism+//+// Created by Claude on 22/8/2026.+//++import Foundation++/// A node in the hierarchical table of contents rendered by both TOC surfaces.+///+/// `TOCNode` is the SINGLE production TOC model (T-1928). It is built from+/// `TOCCoordinator.tocEntries`, which is the only producer that descends into+/// `<details>` children, so headings nested inside a collapsed disclosure are+/// present here even though they have no `MarkdownSection` of their own.+///+/// It deliberately does NOT replace `DocumentStructure`. That model answers+/// containment questions — which blocks belong to a heading, what a block's+/// heading path is, what is hidden when a section collapses — and every one of+/// those answers is keyed on a TOP-LEVEL `sourceIndex`. A heading inside+/// `<details>` has no top-level index, so folding it into `MarkdownSection`+/// would mean inventing one and corrupting `visibleBlocks`,+/// `headingPathByBlockId`, note relocation, and `expandSectionsContaining`.+/// The TOC needs a navigation model, not a containment model, so it gets its+/// own.+///+/// Requirements 8.1–8.4 (`specs/collapsible-sections/requirements.md` — NOT+/// `specs/table-of-contents/requirements.md`, whose section 8 is Performance+/// and has no 8.4):+/// - 8.1: headings inside `<details>` appear in the TOC — covered.+/// - 8.2: a chevron indicator AND one additional level of indentation —+/// covered by `SidebarContentsView.TOCSectionRow` (level indentation itself+/// is structural, one `DisclosureGroup` per level).+/// - 8.3: selecting such a heading expands the section and scrolls to the+/// heading — PARTIALLY covered. The expansion is exact; the scroll lands on+/// the OUTERMOST enclosing `<details>` rather than the heading, because+/// `TOCEntry.scrollTargetId` resolves to the outermost block's anchor. The+/// emitter does give a nested `<details>` its own anchor+/// (`"{domID}-d{childIndex}"`), so a deeper target is constructible; doing so+/// needs the shared `BlockDOMID.nestedDetailsID` helper and is tracked by+/// T-1924.+/// - 8.4: the heading→`<details>`-ancestor mapping — covered by+/// `TOCEntry.detailsAncestorIds` and `DetailsExpansionCoordinator`.+struct TOCNode: Identifiable, Equatable {+ /// The flat entry this node wraps.+ let entry: TOCEntry++ /// Child nodes, deeper in heading level and inside the same (or a deeper)+ /// `<details>` path.+ let children: [TOCNode]++ var id: String { entry.id }+ var level: Int { entry.level }+ var text: String { entry.text }+ var isInsideDetails: Bool { entry.isInsideDetails }++ /// Composite id of the document section this heading opens, or `nil` when+ /// the heading lives inside a `<details>` block.+ ///+ /// Only top-level headings own a `MarkdownSection`, so only they can be+ /// collapsed from the TOC. For a top-level heading this is exactly+ /// `MarkdownSection.id` — both are `"{headingBlockId}-{topLevelIndex}"`.+ var collapsibleSectionId: String? {+ isInsideDetails ? nil : entry.scrollTargetId+ }++ /// Builds the heading hierarchy from flat, document-ordered entries.+ ///+ /// Nesting follows heading level, with one extra rule that keeps+ /// `<details>` content from escaping its block: a candidate parent must sit+ /// on the same `<details>` path as the child, i.e. its `detailsAncestorIds`+ /// must be a PREFIX of the child's. Without that rule a heading inside a+ /// disclosure would adopt the top-level headings that follow the closing+ /// `</details>` (they are outside it), and two sibling `<details>` blocks+ /// with identically-levelled headings would nest into each other.+ ///+ /// - Parameter entries: Document-ordered entries from `TOCCoordinator`.+ /// - Returns: Root nodes in document order.+ static func build(from entries: [TOCEntry]) -> [TOCNode] {+ guard !entries.isEmpty else { return [] }++ var roots: [MutableNode] = []+ var stack: [MutableNode] = []+ stack.reserveCapacity(8)++ for entry in entries {+ while let candidate = stack.last, !candidate.canParent(entry) {+ stack.removeLast()+ }++ let node = MutableNode(entry: entry)+ if let parent = stack.last {+ parent.children.append(node)+ } else {+ roots.append(node)+ }+ stack.append(node)+ }++ return roots.map { $0.freeze() }+ }+}++/// Mutable builder node.+///+/// `freeze()` recurses, which is safe here in a way it is not for+/// `MarkdownSection`: every child is strictly deeper in heading level than its+/// parent, so the tree can never be deeper than the number of distinct heading+/// levels.+private final class MutableNode {+ let entry: TOCEntry+ var children: [MutableNode] = []++ init(entry: TOCEntry) {+ self.entry = entry+ }++ /// Whether this node may parent `candidate`.+ func canParent(_ candidate: TOCEntry) -> Bool {+ candidate.level > entry.level+ && entry.detailsAncestorIds.count <= candidate.detailsAncestorIds.count+ && candidate.detailsAncestorIds.starts(with: entry.detailsAncestorIds)+ }++ func freeze() -> TOCNode {+ TOCNode(entry: entry, children: children.map { $0.freeze() })+ }+}
diff --git a/prism/Models/TOCEntry.swift b/prism/Models/TOCEntry.swiftindex 6c4200ac..8752abfc 100644--- a/prism/Models/TOCEntry.swift+++ b/prism/Models/TOCEntry.swift@@ -34,6 +34,20 @@ struct TOCEntry: Identifiable, Equatable { /// Position index in the parsed blocks array. let blockIndex: Int + /// Index of the enclosing TOP-LEVEL block in `parsedBlocks`.+ ///+ /// For a heading outside `<details>` this is the heading's own top-level+ /// index. For a heading nested inside `<details>` it is the index of the+ /// outermost enclosing details block — the same block `scrollTargetId`+ /// names. It is deliberately NOT `blockIndex`, which is the flattened+ /// counter that also advances through nested children (see+ /// `scrollTargetId`).+ ///+ /// Used to expand any collapsed heading section that would otherwise hide+ /// the navigation target, via+ /// `SectionCollapseManager.expandSectionsContaining(blockAtIndex:)`.+ let topLevelBlockIndex: Int+ /// IDs of ancestor details blocks containing this heading. /// /// Empty for top-level headings not inside details.@@ -88,13 +102,21 @@ struct TOCEntry: Identifiable, Equatable { /// `scrollTargetId` defaults to `id`, which is correct for any document /// with no `<details>` block ahead of the heading (the two index schemes /// agree there). `TOCCoordinator` passes it explicitly.- init(id: String, level: Int, text: String, blockIndex: Int, scrollTargetId: String? = nil) {+ init(+ id: String,+ level: Int,+ text: String,+ blockIndex: Int,+ scrollTargetId: String? = nil,+ topLevelBlockIndex: Int? = nil+ ) { self.id = id self.level = level self.text = text self.blockIndex = blockIndex self.detailsAncestorIds = [] self.scrollTargetId = scrollTargetId ?? id+ self.topLevelBlockIndex = topLevelBlockIndex ?? blockIndex } /// Creates a TOC entry with optional ancestor tracking.@@ -113,7 +135,8 @@ struct TOCEntry: Identifiable, Equatable { text: String, blockIndex: Int, detailsAncestorIds: [String],- scrollTargetId: String? = nil+ scrollTargetId: String? = nil,+ topLevelBlockIndex: Int? = nil ) { self.id = id self.level = level@@ -121,5 +144,6 @@ struct TOCEntry: Identifiable, Equatable { self.blockIndex = blockIndex self.detailsAncestorIds = detailsAncestorIds self.scrollTargetId = scrollTargetId ?? id+ self.topLevelBlockIndex = topLevelBlockIndex ?? blockIndex } }
diff --git a/prism/Models/DocumentSession.swift b/prism/Models/DocumentSession.swiftindex e49218ff..b7531c0f 100644--- a/prism/Models/DocumentSession.swift+++ b/prism/Models/DocumentSession.swift@@ -399,13 +399,42 @@ final class DocumentSession: Identifiable { slugCounts[baseSlug] = count + 1 if dedupedSlug == normalized {- sections.expandSectionAndAncestors(for: entry.scrollId)- pendingAnchorScroll = entry.scrollId+ navigateToHeading(entry) return } } } + /// Reveals and scrolls to a heading selected from the table of contents+ /// (compact sheet, regular sidebar) or matched by a fragment link.+ ///+ /// The single selection path for the one production TOC model (T-1928).+ /// Three things have to happen before the scroll, in this order:+ ///+ /// 1. Every enclosing `<details>` is opened through+ /// `DetailsExpansionCoordinator`. Its `openDetailsDOMIDs` set is what+ /// `WebDocumentStateSynchronizer` pushes as `setDetailsState`, so a+ /// heading inside a closed disclosure is unreachable without this — it+ /// is exactly the omission that made nested TOC entries useless.+ /// 2. Any collapsed heading section hiding the target is expanded. A+ /// top-level heading owns its section and expands by id; a nested+ /// heading has none, so the section containing its enclosing+ /// `<details>` block is expanded by top-level index instead.+ /// 3. Only then is the scroll target published.+ func navigateToHeading(_ entry: TOCEntry) {+ for ancestorId in entry.detailsAncestorIds {+ expansionCoordinator.expand(blockId: ancestorId)+ }++ if entry.isInsideDetails {+ sections.expandSectionsContaining(blockAtIndex: entry.topLevelBlockIndex)+ } else {+ sections.expandSectionAndAncestors(for: entry.scrollId)+ }++ pendingAnchorScroll = entry.scrollId+ }+ /// Converts heading text to a GitHub-flavored anchor slug. /// /// Rules: lowercase, spaces → hyphens, keep alphanumerics, hyphens, and underscores.
diff --git a/prism/Services/TOCCoordinator.swift b/prism/Services/TOCCoordinator.swiftindex 8a9be409..0a8b3b2a 100644--- a/prism/Services/TOCCoordinator.swift+++ b/prism/Services/TOCCoordinator.swift@@ -26,22 +26,44 @@ final class TOCCoordinator { // MARK: - State private var cachedBlocks: [MarkdownBlock] = []- private var _cachedTOCEntries: [TOCEntry]?++ /// Lazily-populated derived caches.+ ///+ /// Both are `@ObservationIgnored`, which is load-bearing now that a VIEW+ /// BODY reads `tocTree` (T-1928: `RegularDocumentLayout` and+ /// `CompactDocumentLayout`). As observed storage they were written from a+ /// getter during view evaluation, so the miss invalidated the very body+ /// that was reading it — converging on the second pass rather than looping,+ /// but costing an extra evaluation of the whole document layout per parse+ /// revision, and leaving a mutate-during-evaluation trap on the main+ /// layout.+ ///+ /// Ignoring them means the dependency has to come from somewhere else, so+ /// both getters read `cachedBlocks` (which IS observed, and is what+ /// `invalidateAndUpdate` writes) on EVERY access, cache hit included.+ /// Without that read a hit would register no dependency at all and a later+ /// re-parse would not re-render the TOC.+ @ObservationIgnored private var _cachedTOCEntries: [TOCEntry]?+ @ObservationIgnored private var _cachedTOCTree: [TOCNode]? // MARK: - Computed /// TOC entries extracted from the current parsed blocks. ///- /// Lazily built on first access after invalidation. Subsequent accesses- /// return the cached result until `invalidateAndUpdate(blocks:)` is called.+ /// Lazily built on first access after invalidation (Req 3.4). Subsequent+ /// accesses return the cached result until `invalidateAndUpdate(blocks:)`+ /// is called. var tocEntries: [TOCEntry] {+ // Registers the Observation dependency on every access — see the note+ // on `_cachedTOCEntries`. Copying the array is a retain, not a copy.+ let blocks = cachedBlocks if let cached = _cachedTOCEntries { return cached } var entries: [TOCEntry] = [] var runningIndex = 0 buildTOCEntries(- from: cachedBlocks,+ from: blocks, into: &entries, runningIndex: &runningIndex, ancestorDetailIds: [],@@ -51,6 +73,25 @@ final class TOCCoordinator { return entries } + /// The hierarchical table of contents rendered by BOTH production TOC+ /// surfaces — the compact `TableOfContentsSheet` and the regular+ /// `TOCSidebar` (T-1928).+ ///+ /// Derived from `tocEntries`, so it includes headings nested inside+ /// `<details>` blocks. Cached alongside the entries and invalidated with+ /// them.+ var tocTree: [TOCNode] {+ // Same dependency registration as `tocEntries` — a cache hit here+ // never reaches that getter, so it must read `cachedBlocks` itself.+ _ = cachedBlocks+ if let cached = _cachedTOCTree {+ return cached+ }+ let tree = TOCNode.build(from: tocEntries)+ _cachedTOCTree = tree+ return tree+ }+ /// Number of headings in the document, for accessibility hints. var headingCount: Int { tocEntries.count } @@ -66,6 +107,7 @@ final class TOCCoordinator { func invalidateAndUpdate(blocks: [MarkdownBlock]) { cachedBlocks = blocks _cachedTOCEntries = nil+ _cachedTOCTree = nil } // MARK: - Private@@ -99,28 +141,52 @@ final class TOCCoordinator { /// 0. Nested headings have no section id of their own (the emitter gives /// non-details children none), so they navigate to the details block that /// contains them — the nearest ancestor with a DOM anchor.+ /// - Parameter topLevelIndex: the top-level `parsedBlocks` index that+ /// `topLevelAnchor` was built from, threaded down for the same reason and+ /// surfaced as `TOCEntry.topLevelBlockIndex`. Kept as its own parameter+ /// rather than parsed back out of the composite, which would depend on+ /// the block-id format never containing a `-`. private func buildTOCEntries( from blocks: [MarkdownBlock], into entries: inout [TOCEntry], runningIndex: inout Int, ancestorDetailIds: [String], parentCompositeId: String?,- topLevelAnchor: String? = nil+ topLevelAnchor: String? = nil,+ topLevelIndex: Int? = nil ) { for (blockIndex, block) in blocks.enumerated() {- // Build the composite ID matching the view hierarchy scheme+ // Build the composite ID matching the view hierarchy scheme.+ //+ // At depth 0 this MUST use the top-level `blockIndex`, not+ // `runningIndex` — it is the same index-space confusion T-1662 fixed+ // for `scrollTargetId`, and it survived here because `compositeId`+ // is only consumed as a `<details>` ancestor path. That path is+ // matched against `DetailsExpansionCoordinator.domIDByDetailsPath`,+ // which `buildAncestorMap` keys on `"{block.id}-{topLevelIndex}"`.+ // The two agree until the first `<details>` block's children advance+ // `runningIndex` past `blockIndex`; from the SECOND `<details>`+ // block onward the ancestor id matched nothing, `openDetails` found+ // no DOM id, and selecting a heading inside it opened nothing+ // (T-1928).+ //+ // Spelled ONCE per iteration and reused below: the defect above was+ // two independent spellings of the same composite three lines+ // apart, one of which drifted.+ let topLevelComposite = "\(block.id)-\(blockIndex)" let compositeId: String if let parentId = parentCompositeId { compositeId = "\(parentId)/\(blockIndex)" } else {- compositeId = "\(block.id)-\(runningIndex)"+ compositeId = topLevelComposite } // Resolvable target: this block's own top-level composite at depth 0, // otherwise the enclosing TOP-LEVEL block's — note this is threaded // unchanged through deeper nesting, so a heading several `<details>` // levels down targets the outermost one, not its immediate parent // (see `TOCEntry.scrollTargetId`).- let resolvableTarget = topLevelAnchor ?? "\(block.id)-\(blockIndex)"+ let resolvableTarget = topLevelAnchor ?? topLevelComposite+ let resolvableIndex = topLevelIndex ?? blockIndex switch block { case .heading(let level, let text):@@ -133,7 +199,8 @@ final class TOCCoordinator { text: FootnoteStripping.strip(HTMLCommentStripping.strip(text)), blockIndex: runningIndex, detailsAncestorIds: ancestorDetailIds,- scrollTargetId: resolvableTarget+ scrollTargetId: resolvableTarget,+ topLevelBlockIndex: resolvableIndex ) entries.append(entry) runningIndex += 1@@ -155,7 +222,8 @@ final class TOCCoordinator { runningIndex: &runningIndex, ancestorDetailIds: newAncestorPath, parentCompositeId: compositeId,- topLevelAnchor: resolvableTarget+ topLevelAnchor: resolvableTarget,+ topLevelIndex: resolvableIndex ) default:
diff --git a/prism/Views/SidebarContentsView.swift b/prism/Views/SidebarContentsView.swiftindex 557e9592..69895b41 100644--- a/prism/Views/SidebarContentsView.swift+++ b/prism/Views/SidebarContentsView.swift@@ -8,27 +8,32 @@ import SwiftUI /// Hierarchical table of contents for sidebar and compact TOC sheet.+///+/// Renders `TOCCoordinator.tocTree`, the single production TOC model (T-1928),+/// so headings nested inside `<details>` blocks are listed alongside top-level+/// ones. Only top-level headings own a document section, so only they offer+/// collapse; a details-nested row navigates and nothing more. struct SidebarContentsView: View {- /// Hierarchical section structure for the current document.- let structure: DocumentStructure+ /// Heading hierarchy for the current document.+ let nodes: [TOCNode] /// Shared collapse state from the active document session. let collapsedSectionIds: Set<String> /// Called when a heading row is selected for navigation.- let onNavigate: (String) -> Void+ let onNavigate: (TOCNode) -> Void /// Called when a section collapse state is toggled. let onToggleCollapse: (String) -> Void var body: some View {- if structure.sections.isEmpty {+ if nodes.isEmpty { emptyState } else { List {- ForEach(structure.sections) { section in+ ForEach(nodes) { node in SidebarSectionRow(- section: section,+ node: node, collapsedSectionIds: collapsedSectionIds, onNavigate: onNavigate, onToggleCollapse: onToggleCollapse@@ -53,40 +58,84 @@ struct SidebarContentsView: View { } private struct SidebarSectionRow: View {- let section: MarkdownSection+ let node: TOCNode let collapsedSectionIds: Set<String>- let onNavigate: (String) -> Void+ let onNavigate: (TOCNode) -> Void let onToggleCollapse: (String) -> Void + /// Expansion for a details-nested row, which owns no document section.+ ///+ /// Starts EXPANDED, matching every top-level row: those bind to+ /// `collapsedSectionIds`, which is empty on a fresh document. An unbound+ /// `DisclosureGroup(content:label:)` starts COLLAPSED, so a `<details>`+ /// block holding `## Outer` / `### Inner` listed `Outer` and hid `Inner`+ /// behind a tap nothing else in the TOC asks for.+ ///+ /// Deliberately local rather than an entry in `collapsedSectionIds`: that+ /// set is `MarkdownSection` collapse state — pushed to the page as+ /// `setSectionState` and read by `visibleBlocks` and note relocation. A+ /// nested heading has no section id to put in it, and its only available+ /// key (the enclosing `<details>` block's composite) is shared by every+ /// sibling heading in that block and would be published as a collapsed+ /// document section. Collapsing a TOC sublist here is a list affordance,+ /// not document state.+ @State private var isNestedExpanded = true+ var body: some View {- if section.children.isEmpty {+ if node.children.isEmpty { headingButton+ } else if let sectionId = node.collapsibleSectionId {+ DisclosureGroup(isExpanded: isExpandedBinding(for: sectionId)) {+ childRows+ } label: {+ headingButton+ } } else {- DisclosureGroup(isExpanded: isExpandedBinding) {- ForEach(section.children) { child in- SidebarSectionRow(- section: child,- collapsedSectionIds: collapsedSectionIds,- onNavigate: onNavigate,- onToggleCollapse: onToggleCollapse- )- }+ DisclosureGroup(isExpanded: $isNestedExpanded) {+ childRows } label: { headingButton } } } + private var childRows: some View {+ ForEach(node.children) { child in+ SidebarSectionRow(+ node: child,+ collapsedSectionIds: collapsedSectionIds,+ onNavigate: onNavigate,+ onToggleCollapse: onToggleCollapse+ )+ }+ }+ private var isCollapsed: Bool {- collapsedSectionIds.contains(section.id)+ guard let sectionId = node.collapsibleSectionId else { return false }+ return collapsedSectionIds.contains(sectionId) } - private var isExpandedBinding: Binding<Bool> {+ /// Whether this row's disclosure is open, for BOTH row kinds.+ ///+ /// A top-level row's disclosure is bound to `collapsedSectionIds`, so+ /// `!isCollapsed` describes it. A details-nested row owns no section, so+ /// `isCollapsed` is unconditionally `false` for it and describes nothing —+ /// its disclosure is `$isNestedExpanded`. Reading the accessibility value+ /// off `isCollapsed` alone therefore announced "Expanded" for a nested row+ /// the reader had collapsed. A row with no children has no disclosure at+ /// all; `true` keeps it out of the collapsed-looking state.+ private var isRowExpanded: Bool {+ if node.children.isEmpty { return true }+ if node.collapsibleSectionId == nil { return isNestedExpanded }+ return !isCollapsed+ }++ private func isExpandedBinding(for sectionId: String) -> Binding<Bool> { Binding( get: { !isCollapsed }, set: { shouldBeExpanded in if shouldBeExpanded == isCollapsed {- onToggleCollapse(section.id)+ onToggleCollapse(sectionId) } } )@@ -94,35 +143,72 @@ private struct SidebarSectionRow: View { private var headingButton: some View { Button {- onNavigate(section.id)+ onNavigate(node) } label: {- TOCSectionRow(section: section, isCollapsed: isCollapsed)+ TOCSectionRow(node: node, isExpanded: isRowExpanded) } .buttonStyle(.plain)+ // Dimming stays on DOCUMENT collapse, not on `isRowExpanded`: it means+ // "the blocks under this heading are hidden in the document", which a+ // collapsed TOC sublist (a list affordance on a details-nested row)+ // does not do. The accessibility value describes the disclosure the+ // reader operates; the opacity describes the document. .opacity(isCollapsed ? 0.5 : 1.0) } } /// Sidebar row label for a section heading.+///+/// Requirements 8.1, 8.2 (`specs/collapsible-sections/requirements.md`). private struct TOCSectionRow: View {- let section: MarkdownSection- let isCollapsed: Bool+ let node: TOCNode++ /// Whether this row's disclosure is open — see+ /// `SidebarSectionRow.isRowExpanded`, which is the only correct source for+ /// both row kinds.+ let isExpanded: Bool++ @Environment(\.themeColors) private var colors++ /// Extra leading inset for a details-nested row (Req 8.2). Level+ /// indentation itself is structural here (each level is a+ /// `DisclosureGroup`), so this is the ONE additional level the requirement+ /// asks for, and nothing else.+ private static let detailsIndent: CGFloat = 12 var body: some View { HStack(spacing: 4) {- Text(section.heading.textContent)- .chromeFont(Self.styleForLevel(headingLevel), weight: Self.weightForLevel(headingLevel))+ // Indicator for headings that live inside a `<details>` block+ // (Req 8.2).+ if node.isInsideDetails {+ Image(systemName: "chevron.right")+ .chromeFont(.caption2)+ .foregroundColor(colors.textTertiary)+ }++ Text(displayText)+ .chromeFont(Self.styleForLevel(node.level), weight: Self.weightForLevel(node.level)) .lineLimit(2) }- .accessibilityLabel(LocalizedStringKey("Heading level \(headingLevel): \(section.heading.textContent)"))- .accessibilityValue(isCollapsed ? LocalizedStringKey("Collapsed") : LocalizedStringKey("Expanded"))+ .padding(.leading, node.isInsideDetails ? Self.detailsIndent : 0)+ .accessibilityLabel(accessibilityLabel)+ .accessibilityValue(isExpanded ? LocalizedStringKey("Expanded") : LocalizedStringKey("Collapsed")) } - private var headingLevel: Int {- if case .heading(let level, _) = section.heading {- return level+ private var displayText: String {+ node.text.isEmpty ? String(localized: "(Empty heading)") : node.text+ }++ /// Accessibility label including details context when applicable+ /// (Req 8.1/8.2).+ private var accessibilityLabel: String {+ let headingText = node.text.isEmpty ? String(localized: "Empty heading") : node.text+ let baseLabel = String(localized: "Heading level \(node.level): \(headingText)")++ if node.isInsideDetails {+ return String(localized: "Inside collapsible section, \(baseLabel)") }- return 1+ return baseLabel } private static func styleForLevel(_ level: Int) -> Font.TextStyle {@@ -143,17 +229,40 @@ private struct TOCSectionRow: View { } #Preview("With Sections") {- let sections = MarkdownSectionBuilder.build(+ let nodes = TOCNode.build(+ from: [+ TOCEntry(id: "a-0", level: 1, text: "Intro", blockIndex: 0),+ TOCEntry(id: "b-2", level: 2, text: "Child", blockIndex: 2)+ ]+ )++ SidebarContentsView(+ nodes: nodes,+ collapsedSectionIds: [],+ onNavigate: { _ in },+ onToggleCollapse: { _ in }+ )+ .frame(width: 320, height: 420)+}++#Preview("Inside Details") {+ let nodes = TOCNode.build( from: [- .heading(level: 1, text: "Intro"),- .paragraph(markdown: "Body"),- .heading(level: 2, text: "Child"),- .paragraph(markdown: "Child body")+ TOCEntry(id: "a-0", level: 1, text: "Intro", blockIndex: 0),+ TOCEntry(+ id: "b-2",+ level: 2,+ text: "Heading Inside Details",+ blockIndex: 2,+ detailsAncestorIds: ["details-1"],+ scrollTargetId: "details-1",+ topLevelBlockIndex: 1+ ) ] ) SidebarContentsView(- structure: sections,+ nodes: nodes, collapsedSectionIds: [], onNavigate: { _ in }, onToggleCollapse: { _ in }@@ -163,7 +272,7 @@ private struct TOCSectionRow: View { #Preview("Empty") { SidebarContentsView(- structure: DocumentStructure(preamble: [], sections: [], headingPathByBlockId: [:]),+ nodes: [], collapsedSectionIds: [], onNavigate: { _ in }, onToggleCollapse: { _ in }
diff --git a/prism/Views/TableOfContentsSheet.swift b/prism/Views/TableOfContentsSheet.swiftindex 96f6a347..3babc2ba 100644--- a/prism/Views/TableOfContentsSheet.swift+++ b/prism/Views/TableOfContentsSheet.swift@@ -8,133 +8,60 @@ import SwiftUI /// Sheet displaying headings for document navigation.+///+/// Renders `TOCCoordinator.tocTree` through `SidebarContentsView`, the single+/// production TOC model (T-1928), so the compact sheet and the regular sidebar+/// list and announce headings identically. struct TableOfContentsSheet: View {- private enum Mode {- case entries(entries: [TOCEntry], onNavigate: (String) -> Void)- case structure(- structure: DocumentStructure,- collapsedSectionIds: Set<String>,- onNavigate: (String) -> Void,- onToggleCollapse: (String) -> Void- )- }-- private let mode: Mode+ let nodes: [TOCNode]+ let collapsedSectionIds: Set<String>+ let onNavigate: (TOCNode) -> Void+ let onToggleCollapse: (String) -> Void let onDismiss: () -> Void - /// Focus state for legacy flat entry mode keyboard navigation.- @FocusState private var focusedEntryId: String?- @Environment(\.themeColors) private var colors - init(- entries: [TOCEntry],- onNavigate: @escaping (String) -> Void,- onDismiss: @escaping () -> Void- ) {- self.mode = .entries(entries: entries, onNavigate: onNavigate)- self.onDismiss = onDismiss- }-- init(- structure: DocumentStructure,- collapsedSectionIds: Set<String>,- onNavigate: @escaping (String) -> Void,- onToggleCollapse: @escaping (String) -> Void,- onDismiss: @escaping () -> Void- ) {- self.mode = .structure(- structure: structure,- collapsedSectionIds: collapsedSectionIds,- onNavigate: onNavigate,- onToggleCollapse: onToggleCollapse- )- self.onDismiss = onDismiss- }- var body: some View { NavigationStack {- content- .scrollContentBackground(.hidden)- .background(colors.background)- .navigationTitle("Table of Contents")- #if os(iOS)- .navigationBarTitleDisplayMode(.inline)- #elseif os(macOS)- .frame(minWidth: 360, minHeight: 400, maxHeight: 600)- #endif- .toolbar {- ToolbarItem(placement: .cancellationAction) {- Button {- onDismiss()- } label: {- Image(systemName: "xmark")- }- .accessibilityLabel(LocalizedStringKey("Close"))- .keyboardShortcut(.escape, modifiers: [])- }- }- }- .onAppear {- #if os(macOS)- if case .entries(let entries, _) = mode {- focusedEntryId = entries.first?.id- }- #endif- }- }-- @ViewBuilder- private var content: some View {- switch mode {- case .entries(let entries, let onNavigate):- List(entries) { entry in- Button {- onNavigate(entry.scrollId)- } label: {- TOCEntryRow(entry: entry)- }- .buttonStyle(.plain)- .focused($focusedEntryId, equals: entry.id)- .listRowBackground(Color.clear)- }- .listStyle(.plain)-- case .structure(let structure, let collapsedSectionIds, let onNavigate, let onToggleCollapse): SidebarContentsView(- structure: structure,+ nodes: nodes, collapsedSectionIds: collapsedSectionIds, onNavigate: onNavigate, onToggleCollapse: onToggleCollapse )+ .scrollContentBackground(.hidden)+ .background(colors.background)+ .navigationTitle("Table of Contents")+ #if os(iOS)+ .navigationBarTitleDisplayMode(.inline)+ #elseif os(macOS)+ .frame(minWidth: 360, minHeight: 400, maxHeight: 600)+ #endif+ .toolbar {+ ToolbarItem(placement: .cancellationAction) {+ Button {+ onDismiss()+ } label: {+ Image(systemName: "xmark")+ }+ .accessibilityLabel(LocalizedStringKey("Close"))+ .keyboardShortcut(.escape, modifiers: [])+ }+ } } } } -#Preview("Legacy Entries") {- TableOfContentsSheet(- entries: [- TOCEntry(id: "1", level: 1, text: "Introduction", blockIndex: 0),- TOCEntry(id: "2", level: 2, text: "Getting Started", blockIndex: 1),- TOCEntry(id: "3", level: 3, text: "Installation", blockIndex: 2)- ],- onNavigate: { _ in },- onDismiss: {}- )-}- #Preview("Section Tree") {- let structure = MarkdownSectionBuilder.build(+ let nodes = TOCNode.build( from: [- .heading(level: 1, text: "Introduction"),- .paragraph(markdown: "Body"),- .heading(level: 2, text: "Child"),- .paragraph(markdown: "Body 2")+ TOCEntry(id: "a-0", level: 1, text: "Introduction", blockIndex: 0),+ TOCEntry(id: "b-2", level: 2, text: "Child", blockIndex: 2) ] ) TableOfContentsSheet(- structure: structure,+ nodes: nodes, collapsedSectionIds: [], onNavigate: { _ in }, onToggleCollapse: { _ in },
diff --git a/prism/Views/TOCEntryRow.swift b/prism/Views/TOCEntryRow.swiftdeleted file mode 100644index 4ddae0fd..00000000--- a/prism/Views/TOCEntryRow.swift+++ /dev/null@@ -1,172 +0,0 @@-//-// TOCEntryRow.swift-// prism-//-// Created by Claude on 14/1/2026.-//--import SwiftUI--/// Renders a single heading entry in the table of contents with appropriate styling.-///-/// Displays the heading text with indentation based on level and font size-/// appropriate to the heading hierarchy. For headings inside `<details>` elements,-/// displays a chevron.right indicator prefix and adds extra indentation.-///-/// Supports accessibility features including full text announcement for truncated headings.-///-/// Requirements covered:-/// - 3.3-3.4: Indentation based on level (12pt per level)-/// - 3.5: Scaled font sizes for hierarchy-/// - 3.6: Single line with truncation-/// - 3.7: Theme color support-/// - 3.8: RTL layout support (via SwiftUI automatic mirroring)-/// - 6.5: Accessibility label with level and full text-/// - 6.7: Full text announced for truncated headings-/// - 8.2: Chevron indicator and extra indentation for details headings-struct TOCEntryRow: View {- let entry: TOCEntry-- @Environment(\.themeColors) private var colors-- /// Indentation per heading level (Req 3.4).- private static let indentPerLevel: CGFloat = 12.0-- var body: some View {- HStack(spacing: 4) {- // Show chevron indicator for headings inside details sections- if entry.isInsideDetails {- Image(systemName: "chevron.right")- .chromeFont(.caption2)- .foregroundColor(colors.textTertiary)- }-- Text(displayText)- .chromeFont(Self.styleForLevel(entry.level), weight: Self.weightForLevel(entry.level))- .foregroundColor(colors.textPrimary)- .lineLimit(1)- .truncationMode(.tail)- }- .padding(.leading, indentation)- .frame(maxWidth: .infinity, alignment: .leading)- .accessibilityLabel(accessibilityLabel)- .accessibilityAddTraits(.isButton)- }-- // MARK: - Display Text-- /// Display text with placeholder for empty headings.- private var displayText: String {- entry.text.isEmpty ? String(localized: "(Empty heading)") : entry.text- }-- // MARK: - Indentation-- /// Calculates leading indentation based on heading level.- ///- /// H1 has no indent, H2 has 12pt, H3 has 24pt, etc.- /// For headings inside details sections, adds one additional level of indentation.- ///- /// Requirements:- /// - 3.3-3.4: 12 points per level, H1 at base- /// - 8.2: Additional level indentation for details headings- private var indentation: CGFloat {- let baseLevels = entry.level - 1- let extraLevel = entry.isInsideDetails ? 1 : 0- return CGFloat(baseLevels + extraLevel) * Self.indentPerLevel- }-- // MARK: - Accessibility-- /// Accessibility label including details context when applicable.- private var accessibilityLabel: String {- let headingText = entry.text.isEmpty ? String(localized: "Empty heading") : entry.text- let baseLabel = String(localized: "Heading level \(entry.level): \(headingText)")-- if entry.isInsideDetails {- return String(localized: "Inside collapsible section, \(baseLabel)")- }- return baseLabel- }-- // MARK: - Font Styling-- /// Maps a heading level to the chrome `Font.TextStyle` driving its size.- /// H6+ uses `.caption2` rather than `.caption` so the macOS baseline lands- /// on 11pt (matching the previous hardcoded size) instead of 12pt.- static func styleForLevel(_ level: Int) -> Font.TextStyle {- switch level {- case 1: return .title3- case 2: return .headline- case 3: return .subheadline- case 4: return .callout- case 5: return .footnote- default: return .caption2- }- }-- /// Visual hierarchy weight per heading level (Requirement 3.5).- static func weightForLevel(_ level: Int) -> Font.Weight {- switch level {- case 1: return .semibold- case 2: return .medium- case 3: return .medium- default: return .regular- }- }-}--#Preview("All Levels") {- List {- TOCEntryRow(entry: TOCEntry(id: "1", level: 1, text: "Introduction", blockIndex: 0))- TOCEntryRow(entry: TOCEntry(id: "2", level: 2, text: "Getting Started", blockIndex: 1))- TOCEntryRow(entry: TOCEntry(id: "3", level: 3, text: "Installation", blockIndex: 2))- TOCEntryRow(entry: TOCEntry(id: "4", level: 4, text: "Prerequisites", blockIndex: 3))- TOCEntryRow(entry: TOCEntry(id: "5", level: 5, text: "System Requirements", blockIndex: 4))- TOCEntryRow(entry: TOCEntry(id: "6", level: 6, text: "Optional Dependencies", blockIndex: 5))- }-}--#Preview("Inside Details") {- List {- TOCEntryRow(entry: TOCEntry(id: "1", level: 1, text: "Main Section", blockIndex: 0))- TOCEntryRow(entry: TOCEntry(- id: "2",- level: 2,- text: "Heading Inside Details",- blockIndex: 1,- detailsAncestorIds: ["details-1"]- ))- TOCEntryRow(entry: TOCEntry(- id: "3",- level: 3,- text: "Nested Details Heading",- blockIndex: 2,- detailsAncestorIds: ["details-1", "details-2"]- ))- TOCEntryRow(entry: TOCEntry(id: "4", level: 2, text: "Regular Heading", blockIndex: 3))- }-}--#Preview("Long Text") {- List {- TOCEntryRow(entry: TOCEntry(- id: "1",- level: 1,- text: "This is a very long heading that should be truncated with an ellipsis when it exceeds the available width",- blockIndex: 0- ))- TOCEntryRow(entry: TOCEntry(- id: "2",- level: 3,- text: "Another long heading at level 3 that demonstrates indentation combined with truncation behavior",- blockIndex: 1- ))- }-}--#Preview("Empty Heading") {- List {- TOCEntryRow(entry: TOCEntry(id: "1", level: 2, text: "", blockIndex: 0))- }-}
diff --git a/prism/Views/TOCSidebar.swift b/prism/Views/TOCSidebar.swiftindex 3344666c..dbf722fb 100644--- a/prism/Views/TOCSidebar.swift+++ b/prism/Views/TOCSidebar.swift@@ -7,16 +7,16 @@ import SwiftUI /// Thin wrapper around `SidebarContentsView` with sidebar styling. struct TOCSidebar: View {- let structure: DocumentStructure+ let nodes: [TOCNode] let collapsedSectionIds: Set<String>- let onNavigate: (String) -> Void+ let onNavigate: (TOCNode) -> Void let onToggleCollapse: (String) -> Void @Environment(\.themeColors) private var colors var body: some View { SidebarContentsView(- structure: structure,+ nodes: nodes, collapsedSectionIds: collapsedSectionIds, onNavigate: onNavigate, onToggleCollapse: onToggleCollapse@@ -26,15 +26,15 @@ struct TOCSidebar: View { } #Preview {- let structure = MarkdownSectionBuilder.build(+ let nodes = TOCNode.build( from: [- .heading(level: 1, text: "Heading"),- .heading(level: 2, text: "Child")+ TOCEntry(id: "a-0", level: 1, text: "Heading", blockIndex: 0),+ TOCEntry(id: "b-1", level: 2, text: "Child", blockIndex: 1) ] ) TOCSidebar(- structure: structure,+ nodes: nodes, collapsedSectionIds: [], onNavigate: { _ in }, onToggleCollapse: { _ in }
diff --git a/prism/Views/CompactDocumentLayout.swift b/prism/Views/CompactDocumentLayout.swiftindex 8011dac9..9113a9ab 100644--- a/prism/Views/CompactDocumentLayout.swift+++ b/prism/Views/CompactDocumentLayout.swift@@ -179,13 +179,14 @@ struct CompactDocumentLayout: View { // Table of Contents sheet .sheet(isPresented: $showTOCSheet) { TableOfContentsSheet(- structure: session.documentStructure,+ nodes: session.toc.tocTree, collapsedSectionIds: session.sections.collapsedSectionIds,- onNavigate: { sectionId in- session.sections.expandSectionAndAncestors(for: sectionId)- // Consumed by WebDocumentStateSynchronizer (same path as- // fragment links).- session.pendingAnchorScroll = sectionId+ onNavigate: { node in+ // Opens any enclosing `<details>`, expands any collapsed+ // section, then publishes the scroll target — consumed by+ // WebDocumentStateSynchronizer (same path as fragment+ // links).+ session.navigateToHeading(node.entry) showTOCSheet = false }, onToggleCollapse: { sectionId in
diff --git a/prism/Views/RegularDocumentLayout.swift b/prism/Views/RegularDocumentLayout.swiftindex 55984892..49f9f17b 100644--- a/prism/Views/RegularDocumentLayout.swift+++ b/prism/Views/RegularDocumentLayout.swift@@ -225,13 +225,14 @@ struct RegularDocumentLayout: View { // Left sidebar (TOC) -- Req 1.1 if showLeftSidebar { TOCSidebar(- structure: session.documentStructure,+ nodes: session.toc.tocTree, collapsedSectionIds: session.sections.collapsedSectionIds,- onNavigate: { sectionId in- session.sections.expandSectionAndAncestors(for: sectionId)- // Consumed by WebDocumentStateSynchronizer (same path- // as fragment links).- session.pendingAnchorScroll = sectionId+ onNavigate: { node in+ // Opens any enclosing `<details>`, expands any collapsed+ // section, then publishes the scroll target — consumed+ // by WebDocumentStateSynchronizer (same path as+ // fragment links).+ session.navigateToHeading(node.entry) }, onToggleCollapse: { sectionId in toggleSection(sectionId)
diff --git a/prismTests/WebRendering/WebDetailsNavigationOrderingTests.swift b/prismTests/WebRendering/WebDetailsNavigationOrderingTests.swiftnew file mode 100644index 00000000..e8f540e7--- /dev/null+++ b/prismTests/WebRendering/WebDetailsNavigationOrderingTests.swift@@ -0,0 +1,517 @@+//+// WebDetailsNavigationOrderingTests.swift+// prismTests+//+// T-1928 regression tests for the ordering hazard the details-aware TOC+// introduced.+//+// Selecting a heading inside a closed `<details>` mutates three pieces of+// native state in one synchronous call (`DocumentSession.navigateToHeading`),+// which `WebDocumentStateSynchronizer.dispatch` reads in one pass and+// `WebDocumentController.dispatch(_:)` then sends as three INDEPENDENT+// fire-and-forget `Task { await page.callJavaScript(…) }` calls. Nothing awaits+// the state commands before the scroll, so their arrival order at the page is+// unspecified — the open T-2028 race. Before T-1928 a details-nested heading+// was not listed anywhere, so no user-reachable path depended on that order;+// this one does.+//+// The fix is on the page side (`prism-scroll.js`): a navigation target is+// remembered for a short window and re-asserted whenever a display-state+// command applies inside it, so the order stops mattering. These tests drive a+// real `WebPage` with the bundled `document.css` injected — the clamping this+// turns on is geometry, so the real cascade has to govern it — and dispatch the+// commands in the LOSING order on purpose.+//++import Foundation+import Testing+import WebKit+@testable import prism++@MainActor+@Suite("TOC navigation into a closed <details> survives command reordering (T-1928)")+struct WebDetailsNavigationOrderingTests {++ /// A document whose LAST block is a closed `<details>` holding a heading and+ /// enough content to be taller than the viewport.+ ///+ /// The shape is what makes the ordering observable. With the disclosure+ /// closed the document ends just past the summary line, so scrolling the+ /// details block to the viewport top is clamped by the document height;+ /// opening it afterwards makes the document taller without moving the+ /// reader, leaving the target short of the top. Opened first, the same+ /// scroll lands exactly.+ private static func blocks() -> [MarkdownBlock] {+ var blocks: [MarkdownBlock] = [.heading(level: 1, text: "Document")]+ for index in 0..<150 {+ blocks.append(.paragraph(markdown: "Lead paragraph \(index) with running text for height."))+ }+ var children: [MarkdownBlock] = [.heading(level: 2, text: "Nested Heading")]+ for index in 0..<150 {+ children.append(.paragraph(markdown: "Nested paragraph \(index) with running text for height."))+ }+ blocks.append(+ .details(summary: "Collapsed", children: children, isOpenByDefault: false, depth: 0)+ )+ return blocks+ }++ /// `blocks()` plus one extra, UNRELATED closed `<details>` block near the+ /// document top (index 1). Used by the round-2 residual tests below: the+ /// trailing details block (index `.last`) stays the clamped navigation+ /// target, and the leading one gives them a second, unrelated details id+ /// to toggle/open without touching the target's ancestor chain.+ private static func blocksWithLeadingDetails() -> [MarkdownBlock] {+ var blocks: [MarkdownBlock] = Self.blocks()+ blocks.insert(+ .details(+ summary: "Other",+ children: [.heading(level: 2, text: "Other Heading"), .paragraph(markdown: "Unrelated content.")],+ isOpenByDefault: false,+ depth: 0+ ),+ at: 1+ )+ return blocks+ }++ /// `blocks()` with the trailing disclosure holding a SECOND, nested+ /// `<details>` instead of the heading directly. The nested block emits its+ /// own section, id `"{outerDOMID}-d0"` (`BlockHTMLEmitter.emitDetails`), so+ /// opening it is a display-state change INSIDE the navigation target rather+ /// than above it.+ private static func blocksWithNestedDetails() -> [MarkdownBlock] {+ var blocks: [MarkdownBlock] = [.heading(level: 1, text: "Document")]+ for index in 0..<150 {+ blocks.append(.paragraph(markdown: "Lead paragraph \(index) with running text for height."))+ }+ var innerChildren: [MarkdownBlock] = [.heading(level: 2, text: "Nested Heading")]+ for index in 0..<150 {+ innerChildren.append(.paragraph(markdown: "Nested paragraph \(index) with running text."))+ }+ let inner = MarkdownBlock.details(+ summary: "Inner", children: innerChildren, isOpenByDefault: false, depth: 1+ )+ blocks.append(+ .details(summary: "Outer", children: [inner], isOpenByDefault: false, depth: 0)+ )+ return blocks+ }++ /// A document whose FIRST block is a heading owning everything after it, so+ /// collapsing that heading hides the navigation target. Used for the+ /// `setSectionState` half of the ancestor gate.+ private static func blocksUnderOneHeading() -> [MarkdownBlock] {+ var blocks: [MarkdownBlock] = [.heading(level: 1, text: "Only Heading")]+ for index in 0..<200 {+ blocks.append(.paragraph(markdown: "Body paragraph \(index) with running text for height."))+ }+ return blocks+ }++ /// The DOM id of the trailing `<details>` block — the scroll target a nested+ /// heading resolves to (`TOCEntry.scrollTargetId`).+ private func detailsDOMID(_ blocks: [MarkdownBlock]) throws -> String {+ try #require(BlockDOMID.map(blocks: blocks).last?.domID)+ }++ /// The composite section id `setSectionState` matches against+ /// `data-prism-section-id` — `MarkdownSection.id`, `"{blockID}-{index}"`.+ private func sectionID(_ blocks: [MarkdownBlock], _ index: Int) -> String {+ "\(blocks[index].id)-\(index)"+ }++ /// Scrolls the viewport and returns the resulting `scrollY` in ONE+ /// round-trip. Every step between arming a navigation target and observing+ /// it has to fit inside `NAVIGATION_REASSERT_MS` (1 s), so these tests+ /// deliberately spend as few round-trips as possible in that window.+ private func scrollByAndReport(+ _ harness: WebDocumentLiveHarness, _ delta: Double+ ) async throws -> Double {+ let result = try await harness.page.callJavaScript(+ "window.scrollBy({ top: delta, behavior: 'auto' }); return window.scrollY;",+ arguments: ["delta": delta],+ contentWorld: harness.bridgeWorld+ )+ return try #require(result as? Double)+ }++ /// Dispatches a synthetic reader input on `window`, the same event the+ /// cancellation listeners are registered for.+ private func dispatchReaderInput(+ _ harness: WebDocumentLiveHarness, _ type: String+ ) async throws {+ _ = try await harness.page.callJavaScript(+ "window.dispatchEvent(new Event(type)); return null;",+ arguments: ["type": type],+ contentWorld: harness.bridgeWorld+ )+ }++ private func sectionTop(+ _ harness: WebDocumentLiveHarness, _ domID: String+ ) async throws -> Double {+ let result = try await harness.page.callJavaScript(+ "var el = document.getElementById(id);"+ + " return el ? el.getBoundingClientRect().top : null;",+ arguments: ["id": domID],+ contentWorld: harness.bridgeWorld+ )+ return try #require(result as? Double)+ }++ /// `window.scrollY` — unlike `sectionTop`, unaffected by a reflow that+ /// changes a target's on-page position without actually scrolling+ /// anything (e.g. an unrelated `<details>` earlier in the document+ /// opening and pushing everything below it down). The round-2 residual+ /// tests need to tell "the document reflowed" from "a reassert scrolled+ /// the viewport", and only `scrollY` answers that.+ private func scrollY(_ harness: WebDocumentLiveHarness) async throws -> Double {+ let result = try await harness.page.callJavaScript(+ "return window.scrollY;",+ contentWorld: harness.bridgeWorld+ )+ return try #require(result as? Double)+ }++ /// Polls until the target's top settles at the viewport top, or gives up and+ /// returns the last reading for the failure message.+ private func settledTop(+ _ harness: WebDocumentLiveHarness, _ domID: String+ ) async throws -> Double {+ var top = try await sectionTop(harness, domID)+ for _ in 0..<20 {+ if abs(top) <= 4 { return top }+ try await Task.sleep(for: .milliseconds(50))+ top = try await sectionTop(harness, domID)+ }+ return top+ }++ private func isOpen(+ _ harness: WebDocumentLiveHarness, _ domID: String+ ) async throws -> Bool {+ let result = try await harness.page.callJavaScript(+ "var el = document.getElementById(id);"+ + " var d = el ? el.querySelector('details') : null;"+ + " return !!(d && d.open);",+ arguments: ["id": domID],+ contentWorld: harness.bridgeWorld+ )+ return (result as? Bool) ?? false+ }++ // MARK: - Tests++ @Test("A scroll dispatched BEFORE the details-open command still lands on the target")+ func scrollDispatchedBeforeDetailsOpenStillLands() async throws {+ let blocks = Self.blocks()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let domID = try detailsDOMID(blocks)++ #expect(try await isOpen(harness, domID) == false, "the fixture's <details> should start closed")++ // The losing order, on purpose: the scroll first, the state command+ // after.+ try await harness.send(.scrollToBlock(domID: domID))++ // The fixture has to be one where the order MATTERS, or the test proves+ // nothing: with the disclosure closed the document is too short for the+ // target to reach the viewport top, so this scroll is clamped.+ let clamped = try await sectionTop(harness, domID)+ #expect(+ clamped > 1,+ "fixture is not order-sensitive — the closed-details scroll was not clamped (top \(clamped))"+ )++ try await harness.send(.setDetailsState(expandedIDs: [domID]))++ #expect(try await isOpen(harness, domID))+ let settled = try await settledTop(harness, domID)+ #expect(+ abs(settled) <= 4,+ "the scroll landed before the disclosure opened and was never re-asserted (top \(settled))"+ )+ }++ @Test("The winning order lands on the target too")+ func detailsOpenBeforeScrollLands() async throws {+ let blocks = Self.blocks()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let domID = try detailsDOMID(blocks)++ try await harness.send(.setDetailsState(expandedIDs: [domID]))+ try await harness.send(.scrollToBlock(domID: domID))++ #expect(try await isOpen(harness, domID))+ let settled = try await settledTop(harness, domID)+ #expect(abs(settled) <= 4, "the target did not end at the viewport top (top \(settled))")+ }++ // The bound on the re-assertion. It exists so a state command that arrives+ // late cannot silently become a scroll authority: past the window — and past+ // any scroll the reader made themselves — a display-state push must leave the+ // viewport exactly where it is.+ @Test("A late display-state push does not drag the reader back to a finished navigation")+ func lateStatePushDoesNotClobberTheReader() async throws {+ let blocks = Self.blocks()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let domID = try detailsDOMID(blocks)++ try await harness.send(.setDetailsState(expandedIDs: [domID]))+ try await harness.send(.scrollToBlock(domID: domID))+ _ = try await settledTop(harness, domID)++ // Past the programmatic-scroll suppression window and the re-assert+ // deadline both, so the reader's own scroll is the only authority left.+ try await Task.sleep(for: .milliseconds(1200))+ _ = try await harness.page.callJavaScript(+ "window.scrollBy({ top: 300, behavior: 'auto' }); return null;",+ contentWorld: harness.bridgeWorld+ )+ try await harness.send(.setDetailsState(expandedIDs: [domID]))++ let top = try await sectionTop(harness, domID)+ #expect(top < -100, "a late state push re-asserted a finished navigation (top \(top))")+ }++ // ---- Round-2 review residual: reassertNavigationTarget must be selective ----+ //+ // The original re-assert fired on ANY setSectionState/setDetailsState push+ // while its window was open, with no way to tell a push that belongs to the+ // pending navigation from one that doesn't. Both tests below drive that gap+ // directly: an unrelated display-state push must leave the viewport exactly+ // where the reader put it, not snap back to an earlier target.++ @Test("A state push for an unrelated <details> block does not re-assert a still-pending navigation")+ func unrelatedDetailsPushDoesNotReassertTarget() async throws {+ let blocks = Self.blocksWithLeadingDetails()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let mapped = BlockDOMID.map(blocks: blocks)+ let unrelatedDomID = mapped[1].domID+ let domID = try detailsDOMID(blocks)++ #expect(try await isOpen(harness, unrelatedDomID) == false)+ #expect(try await isOpen(harness, domID) == false)++ // TOC navigation into the closed trailing details — clamped, exactly+ // as in the losing-order test above.+ try await harness.send(.scrollToBlock(domID: domID))+ let clamped = try await sectionTop(harness, domID)+ #expect(+ clamped > 1,+ "fixture is not order-sensitive — the closed-details scroll was not clamped (top \(clamped))"+ )++ // The reader scrolls away to look at something else, INSIDE the+ // programmatic-scroll suppression window (600ms) so the generic+ // `scroll` listener — a separate cancellation path — does not itself+ // clear the pending target. This isolates the ancestor-relevance gate.+ let moved = try await scrollByAndReport(harness, 300)++ // The reader toggles a DIFFERENT, unrelated <details> open elsewhere.+ // Native echoes the full expanded-id set back: the unrelated block is+ // newly open, the target is unchanged (still closed — its own open+ // push has not landed). Nothing about the target's ancestor chain+ // changed, so the still-pending navigation must not be re-asserted.+ //+ // Compared on `scrollY`, not the target's rect: the unrelated+ // <details> sits BEFORE the target, so opening it legitimately moves+ // the target's on-page position by reflow alone, with no scroll+ // involved. `scrollY` only moves if a reassert actually fired.+ try await harness.send(.setDetailsState(expandedIDs: [unrelatedDomID]))++ let after = try await scrollY(harness)+ #expect(+ abs(after - moved) <= 4,+ "an unrelated details push re-asserted the earlier navigation target (moved to \(moved), after \(after))"+ )++ // POSITIVE CONTROL, in the same fixture and the same window. Without+ // it the assertion above passes for any reason the target stopped+ // being live — an expired deadline, the generic scroll listener having+ // cleared it — none of which is the gate under test. A push that DOES+ // touch the target's chain (its own open state) must re-assert here,+ // which is only possible if the target was still armed a moment ago.+ try await harness.send(.setDetailsState(expandedIDs: [unrelatedDomID, domID]))++ let settled = try await settledTop(harness, domID)+ #expect(+ abs(settled) <= 4,+ "the target was not still armed, so the negative assertion above proves nothing (top \(settled))"+ )+ #expect(try await isOpen(harness, unrelatedDomID))+ #expect(try await isOpen(harness, domID))+ }++ @Test("A second navigation's state pushes arriving first do not transiently re-scroll to the earlier target")+ func secondNavigationStatePushesDoNotReassertFirstTarget() async throws {+ let blocks = Self.blocksWithLeadingDetails()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let mapped = BlockDOMID.map(blocks: blocks)+ let secondDomID = mapped[1].domID+ let firstDomID = try detailsDOMID(blocks)++ // Navigation 1: a TOC selection targets the closed trailing details —+ // clamped.+ try await harness.send(.scrollToBlock(domID: firstDomID))+ let clamped = try await sectionTop(harness, firstDomID)+ #expect(clamped > 1, "fixture is not order-sensitive for the first target (top \(clamped))")++ let moved = try await scrollByAndReport(harness, 300)++ // Navigation 2 (a second TOC selection) targets the leading details.+ // Its own bridge commands are independent fire-and-forget tasks, so+ // its state push can arrive BEFORE its own scrollToBlock — the T-2028+ // ordering race. navigationTarget is still the FIRST target here (its+ // own scrollToBlock has not run yet), so this must not re-assert it.+ //+ // Compared on `scrollY`: the second target sits BEFORE the first, so+ // opening it legitimately reflows the first target's rect with no+ // scroll involved — only `scrollY` moving is evidence of a reassert.+ try await harness.send(.setDetailsState(expandedIDs: [secondDomID]))+ let afterNav2State = try await scrollY(harness)+ #expect(+ abs(afterNav2State - moved) <= 4,+ "nav2's state push transiently re-scrolled to the first target (moved to \(moved), after nav2 state \(afterNav2State))"+ )++ // POSITIVE CONTROL: the first target must still be armed, or the+ // assertion above is satisfied by an expired window rather than by the+ // gate. A push that also flips the FIRST target's own state re-asserts+ // it.+ try await harness.send(.setDetailsState(expandedIDs: [secondDomID, firstDomID]))+ let firstSettled = try await settledTop(harness, firstDomID)+ #expect(+ abs(firstSettled) <= 4,+ "the first target was not still armed, so the negative assertion above proves nothing (top \(firstSettled))"+ )++ // Navigation 2's own scroll now lands and correctly becomes the target.+ try await harness.send(.scrollToBlock(domID: secondDomID))+ let settled = try await settledTop(harness, secondDomID)+ #expect(abs(settled) <= 4, "the second navigation did not land on its own target (top \(settled))")+ }++ // ---- Round-3 review residual: the gate's other half, and its bounds ----++ @Test("Expanding the collapsed HEADING section hiding the target re-asserts the navigation")+ func collapsedHeadingExpansionReassertsTarget() async throws {+ let blocks = Self.blocksUnderOneHeading()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let mapped = BlockDOMID.map(blocks: blocks)+ let targetDomID = mapped[150].domID+ let headingSectionID = sectionID(blocks, 0)++ // The whole document below `# Only Heading` is collapsed, so the target+ // is display:none: the scroll resolves to the nearest RENDERED section+ // (T-1944) and the reader ends up at the top, nowhere near it.+ try await harness.send(.setSectionState(collapsedIDs: [headingSectionID]))+ try await harness.send(.scrollToBlock(domID: targetDomID))+ let clampedY = try await scrollY(harness)+ #expect(+ clampedY <= 4,+ "fixture is not order-sensitive — the collapsed-section scroll did not fall back to the top (scrollY \(clampedY))"+ )++ // The expansion push that belongs to the SAME navigation lands second.+ // Its changed id is the target's collapsible-heading ancestor, which is+ // the `setSectionState` half of the gate — the level-stack walk in+ // `collapsibleHeadingAncestorIds`, and the ONLY thing that can match+ // here: the target's own id is not in this push's id space at all.+ try await harness.send(.setSectionState(collapsedIDs: []))++ let settled = try await settledTop(harness, targetDomID)+ #expect(+ abs(settled) <= 4,+ "the section expansion did not re-assert the pending navigation (top \(settled))"+ )+ let settledY = try await scrollY(harness)+ #expect(settledY > 100, "the viewport never moved off the top (scrollY \(settledY))")+ }++ @Test("Opening a <details> nested INSIDE the target re-asserts it, because it un-clamps the target")+ func descendantDetailsExpansionReassertsTarget() async throws {+ let blocks = Self.blocksWithNestedDetails()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let outerDomID = try detailsDOMID(blocks)+ // The nested block's own section id, as the emitter forms it.+ let innerDomID = "\(outerDomID)-d0"++ // The outer disclosure is ALREADY open — so the navigation flips only+ // the inner one, and the changed id is a DESCENDANT of the target, not+ // an ancestor of it. The document is still short (the inner disclosure+ // holds all the height), so the scroll is clamped.+ try await harness.send(.setDetailsState(expandedIDs: [outerDomID]))+ try await harness.send(.scrollToBlock(domID: outerDomID))+ let clamped = try await sectionTop(harness, outerDomID)+ #expect(+ clamped > 1,+ "fixture is not order-sensitive — the nested-details scroll was not clamped (top \(clamped))"+ )++ try await harness.send(.setDetailsState(expandedIDs: [outerDomID, innerDomID]))++ #expect(try await isOpen(harness, innerDomID), "the inner disclosure did not open")+ let settled = try await settledTop(harness, outerDomID)+ #expect(+ abs(settled) <= 4,+ "an expansion inside the target did not relieve the clamp (top \(settled))"+ )+ }++ @Test("A reader's own input cancels the pending re-assert outright")+ func readerInputCancelsThePendingReassert() async throws {+ let blocks = Self.blocks()++ // Phase 1 — the same fixture WITHOUT the input, so phase 2's negative+ // assertion cannot pass because the mechanism never worked here.+ let armed = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let armedID = try detailsDOMID(blocks)+ try await armed.send(.scrollToBlock(domID: armedID))+ #expect(try await sectionTop(armed, armedID) > 1)+ try await armed.send(.setDetailsState(expandedIDs: [armedID]))+ let armedSettled = try await settledTop(armed, armedID)+ #expect(abs(armedSettled) <= 4, "control: the re-assert did not fire at all (top \(armedSettled))")++ // Phase 2 — identical, except the reader flicks the wheel first. That+ // is unambiguous intent and is NOT gated on the programmatic-scroll+ // suppression window, so the still-open re-assert window is dropped+ // immediately and the state push must leave the viewport alone.+ let cancelled = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let cancelledID = try detailsDOMID(blocks)+ try await cancelled.send(.scrollToBlock(domID: cancelledID))+ let before = try await scrollY(cancelled)+ try await dispatchReaderInput(cancelled, "wheel")+ try await cancelled.send(.setDetailsState(expandedIDs: [cancelledID]))++ #expect(try await isOpen(cancelled, cancelledID), "the state push itself did not land")+ let after = try await scrollY(cancelled)+ #expect(+ abs(after - before) <= 4,+ "a wheel flick did not cancel the pending re-assert (scrollY \(before) → \(after))"+ )+ }++ @Test("A stored-position restore does not arm the re-assert window")+ func storedPositionRestoreDoesNotArmTheWindow() async throws {+ let blocks = Self.blocks()+ let harness = try await WebDocumentLiveHarness.makeStyled(blocks: blocks)+ let domID = try detailsDOMID(blocks)++ // The same command an explicit navigation sends, but issued by+ // `WebDocumentController.restoreScroll` for a position the reader never+ // chose (T-1928). It scrolls — and claims no re-assert authority.+ try await harness.send(.scrollToBlock(domID: domID, armsReassert: false))+ let before = try await scrollY(harness)++ try await harness.send(.setDetailsState(expandedIDs: [domID]))++ #expect(try await isOpen(harness, domID), "the state push itself did not land")+ let after = try await scrollY(harness)+ #expect(+ abs(after - before) <= 4,+ "a stored-position restore armed the re-assert window (scrollY \(before) → \(after))"+ )+ }+}
diff --git a/prismTests/TOCDetailsHeadingsTests.swift b/prismTests/TOCDetailsHeadingsTests.swiftnew file mode 100644index 00000000..429c0bd1--- /dev/null+++ b/prismTests/TOCDetailsHeadingsTests.swift@@ -0,0 +1,406 @@+//+// TOCDetailsHeadingsTests.swift+// prismTests+//+// Regression tests for T-1928: the shipped compact and regular tables of+// contents omitted every heading nested inside a `<details>` block.+//+// Both surfaces were built from `DocumentSession.documentStructure`, and+// `MarkdownSectionBuilder.build(from:)` only scans top-level `parsedBlocks` —+// a `.details` block is ordinary direct content and its children are never+// traversed. `TOCCoordinator` did descend into details, but nothing in+// production consumed its output, so `TOCEntryTests` only ever verified an+// unused path.+//+// These tests go through the production model (`session.toc.tocTree`) and the+// production selection path (`session.navigateToHeading`), so wiring either of+// them back to a details-blind source fails here.+//+// Deliberately parse real markdown rather than hand-assembling blocks: the+// ancestor map and the details DOM ids are built by `parseContent()`, and a+// hand-set `parsedBlocks` skips that.+//++import Foundation+import Testing+@testable import prism++@MainActor+@Suite("TOC includes headings inside <details> (T-1928)")+struct TOCDetailsHeadingsTests {++ /// A closed `<details>` block holding a heading, with a top-level heading+ /// before it and another after it.+ private static let markdown = """+ # Intro++ Intro paragraph.++ <details>+ <summary>Collapsed</summary>++ ## Nested Heading++ Nested paragraph.++ </details>++ ## Later Heading++ Trailing paragraph.+ """++ private static func makeSession() async -> DocumentSession {+ let session = DocumentSession(clipboardContent: markdown)+ await session.parseContent()+ return session+ }++ /// Top-level index of the document's `<details>` block.+ private static func detailsIndex(in session: DocumentSession) -> Int? {+ session.parsedBlocks.firstIndex { block in+ if case .details = block { return true }+ return false+ }+ }++ /// Flattens the tree in document order for shape assertions.+ private static func flatten(_ nodes: [TOCNode]) -> [TOCNode] {+ nodes.flatMap { [$0] + flatten($0.children) }+ }++ // MARK: - The bug++ @Test("Production TOC tree lists the heading inside a closed <details>")+ func treeIncludesNestedHeading() async {+ let session = await Self.makeSession()++ let texts = Self.flatten(session.toc.tocTree).map(\.text)++ #expect(texts.contains("Nested Heading"))+ #expect(texts == ["Intro", "Nested Heading", "Later Heading"])+ }++ @Test("The details-blind section model still omits it, which is why the TOC has its own model")+ func sectionStructureStillOmitsNestedHeading() async {+ let session = await Self.makeSession()++ var headings: [String] = []+ var stack = session.documentStructure.sections+ while let section = stack.popLast() {+ if case .heading(_, let text) = section.heading { headings.append(text) }+ stack.append(contentsOf: section.children)+ }++ // `MarkdownSection` is keyed on a top-level `sourceIndex` that a nested+ // heading does not have. Documenting the split so a future change does+ // not "fix" this by inventing one.+ #expect(!headings.contains("Nested Heading"))+ }++ @Test("Nested heading node carries its details ancestors and cannot be collapsed")+ func nestedNodeShape() async throws {+ let session = await Self.makeSession()++ let nestedNode = try #require(+ Self.flatten(session.toc.tocTree).first { $0.text == "Nested Heading" }+ )++ #expect(nestedNode.isInsideDetails)+ #expect(nestedNode.entry.detailsAncestorIds.count == 1)+ // No `MarkdownSection` of its own, so the TOC must not offer collapse.+ #expect(nestedNode.collapsibleSectionId == nil)++ // Its scroll target is the enclosing top-level `<details>` block.+ let index = try #require(Self.detailsIndex(in: session))+ #expect(nestedNode.entry.scrollId == "\(session.parsedBlocks[index].id)-\(index)")+ #expect(nestedNode.entry.topLevelBlockIndex == index)+ }++ @Test("A top-level heading node stays collapsible and matches its MarkdownSection id")+ func topLevelNodeStaysCollapsible() async throws {+ let session = await Self.makeSession()++ let intro = try #require(session.toc.tocTree.first)++ #expect(intro.text == "Intro")+ #expect(!intro.isInsideDetails)+ #expect(intro.collapsibleSectionId == session.documentStructure.sections.first?.id)+ }++ // MARK: - Selection++ @Test("Selecting the nested heading opens its enclosing <details> before scrolling")+ func selectingNestedHeadingOpensDetails() async throws {+ let session = await Self.makeSession()++ let index = try #require(Self.detailsIndex(in: session))+ let detailsDOMID = BlockDOMID.map(blocks: session.parsedBlocks)[index].domID++ // The block is closed by default, so nothing has opened it yet.+ #expect(!session.expansionCoordinator.openDetailsDOMIDs.contains(detailsDOMID))++ let nestedNode = try #require(+ Self.flatten(session.toc.tocTree).first { $0.text == "Nested Heading" }+ )++ session.navigateToHeading(nestedNode.entry)++ // `openDetailsDOMIDs` is what WebDocumentStateSynchronizer pushes as+ // `setDetailsState`; without this the heading stays hidden in the page.+ #expect(session.expansionCoordinator.openDetailsDOMIDs.contains(detailsDOMID))+ #expect(session.pendingAnchorScroll == nestedNode.entry.scrollId)+ }++ @Test("Selecting a top-level heading does not open any <details>")+ func selectingTopLevelHeadingLeavesDetailsClosed() async throws {+ let session = await Self.makeSession()++ let before = session.expansionCoordinator.openDetailsDOMIDs+ let laterNode = try #require(+ Self.flatten(session.toc.tocTree).first { $0.text == "Later Heading" }+ )++ session.navigateToHeading(laterNode.entry)++ #expect(session.expansionCoordinator.openDetailsDOMIDs == before)+ #expect(session.pendingAnchorScroll == laterNode.entry.scrollId)+ }++ @Test("Selecting a heading inside the SECOND <details> block also opens it")+ func selectingHeadingInSecondDetailsOpensIt() async throws {+ // The `<details>` ancestor path is matched against the map+ // `DetailsExpansionCoordinator.buildAncestorMap` keys on the TOP-LEVEL+ // index. `TOCCoordinator` built it from the FLATTENED counter instead,+ // which agrees only until the first `<details>` block's children have+ // advanced it — so every block from the second one on resolved to+ // nothing and opened nothing. Same index-space confusion as T-1662.+ let session = DocumentSession(clipboardContent: """+ # Intro++ <details>+ <summary>First</summary>++ ## In First++ Body one.++ </details>++ <details>+ <summary>Second</summary>++ ## In Second++ Body two.++ </details>+ """)+ await session.parseContent()++ let detailsIndices = session.parsedBlocks.indices.filter { index in+ if case .details = session.parsedBlocks[index] { return true }+ return false+ }+ #expect(detailsIndices.count == 2)+ let secondIndex = try #require(detailsIndices.last)+ let secondDOMID = BlockDOMID.map(blocks: session.parsedBlocks)[secondIndex].domID++ let nestedNode = try #require(+ Self.flatten(session.toc.tocTree).first { $0.text == "In Second" }+ )++ session.navigateToHeading(nestedNode.entry)++ #expect(session.expansionCoordinator.openDetailsDOMIDs.contains(secondDOMID))+ #expect(nestedNode.entry.detailsAncestorIds == ["\(session.parsedBlocks[secondIndex].id)-\(secondIndex)"])+ }++ @Test("A fragment link to a nested heading opens its enclosing <details>")+ func fragmentNavigationOpensDetails() async throws {+ let session = await Self.makeSession()++ let index = try #require(Self.detailsIndex(in: session))+ let detailsDOMID = BlockDOMID.map(blocks: session.parsedBlocks)[index].domID++ session.scrollToAnchor("nested-heading")++ #expect(session.expansionCoordinator.openDetailsDOMIDs.contains(detailsDOMID))+ }++ // MARK: - Tree shape++ @Test("Headings after a </details> are not nested under the details heading")+ func headingAfterDetailsIsASibling() async {+ let session = await Self.makeSession()++ let roots = session.toc.tocTree+ #expect(roots.count == 1)+ #expect(roots.first?.text == "Intro")++ // Both `## Nested Heading` and `## Later Heading` hang off `# Intro`.+ // The nested one must not adopt the heading that follows the closing+ // `</details>`, which is outside the block entirely.+ let children = roots.first?.children ?? []+ #expect(children.map(\.text) == ["Nested Heading", "Later Heading"])+ #expect(children.first?.children.isEmpty == true)+ }++ @Test("Two heading levels inside one <details> nest, and neither offers collapse")+ func nestedHeadingLevelsInsideOneDetails() async throws {+ let session = DocumentSession(clipboardContent: """+ # Intro++ <details>+ <summary>Collapsed</summary>++ ## Outer++ Body.++ ### Inner++ More body.++ </details>+ """)+ await session.parseContent()++ let intro = try #require(session.toc.tocTree.first)+ #expect(intro.children.map(\.text) == ["Outer"])+ let outer = try #require(intro.children.first)+ #expect(outer.children.map(\.text) == ["Inner"])+ let inner = try #require(outer.children.first)++ // Both live inside the same `<details>`, so neither owns a+ // `MarkdownSection` and neither may offer document collapse. `Inner` is+ // therefore reachable only through `Outer`'s TOC disclosure — which is+ // why that disclosure must start EXPANDED (`SidebarContentsView`+ // .isNestedExpanded). An unbound `DisclosureGroup` starts collapsed and+ // hid `Inner` behind a tap nothing else in the TOC asks for.+ #expect(outer.isInsideDetails)+ #expect(outer.collapsibleSectionId == nil)+ #expect(inner.isInsideDetails)+ #expect(inner.collapsibleSectionId == nil)+ #expect(inner.entry.detailsAncestorIds == outer.entry.detailsAncestorIds)++ // And selecting the deeper one still opens the enclosing block.+ let index = try #require(Self.detailsIndex(in: session))+ let detailsDOMID = BlockDOMID.map(blocks: session.parsedBlocks)[index].domID+ session.navigateToHeading(inner.entry)+ #expect(session.expansionCoordinator.openDetailsDOMIDs.contains(detailsDOMID))+ #expect(session.pendingAnchorScroll == inner.entry.scrollId)+ }++ @Test("Sibling <details> blocks do not nest into each other")+ func siblingDetailsDoNotNest() {+ let entries = [+ TOCEntry(+ id: "a-1", level: 2, text: "In First", blockIndex: 1,+ detailsAncestorIds: ["d1-0"], scrollTargetId: "d1-0", topLevelBlockIndex: 0+ ),+ TOCEntry(+ id: "b-3", level: 3, text: "In Second", blockIndex: 3,+ detailsAncestorIds: ["d2-1"], scrollTargetId: "d2-1", topLevelBlockIndex: 1+ )+ ]++ let tree = TOCNode.build(from: entries)++ // Level alone would make "In Second" (h3) a child of "In First" (h2),+ // but they live in different `<details>` blocks.+ #expect(tree.count == 2)+ #expect(tree.allSatisfy { $0.children.isEmpty })+ }++ @Test("Without <details> the tree hierarchy matches the section tree")+ func hierarchyMatchesSectionTreeWithoutDetails() async {+ let session = DocumentSession(clipboardContent: """+ # One++ ## Two++ ### Three++ ## Four++ # Five+ """)+ await session.parseContent()++ let roots = session.toc.tocTree+ #expect(roots.map(\.text) == ["One", "Five"])+ #expect(roots.first?.children.map(\.text) == ["Two", "Four"])+ #expect(roots.first?.children.first?.children.map(\.text) == ["Three"])++ // Same shape the section tree produces, so no document without details+ // changes appearance.+ let sections = session.documentStructure.sections+ #expect(sections.map(\.heading.textContent) == roots.map(\.text))+ #expect(+ sections.first?.children.map(\.heading.textContent)+ == roots.first?.children.map(\.text)+ )+ }+}++/// Pins the production WIRING, which a direct-invocation test cannot see.+///+/// `navigateToHeading` can be unit-tested all day while a layout quietly does+/// `session.pendingAnchorScroll = id` inline again — which is precisely the+/// state this bug was found in, and the same class of gap that let the+/// WebContent-termination wiring sit dead through the whole T-1542 cutover+/// (T-1943). The model itself is compiler-enforced (both TOC surfaces now take+/// `[TOCNode]` and nothing else), so only the selection call needs pinning.+///+/// Textual, so it is not a proof: a layout could route through a helper this+/// scan cannot follow. It closes the gap that actually occurred.+@Suite("Both TOC surfaces are wired to the shared selection path (T-1928)")+struct TOCProductionWiringTests {++ private static func source(of file: String) throws -> String {+ let root = try #require(+ ProductionSourceScan.productionSourceRoot(+ sentinel: "Views/CompactDocumentLayout.swift"+ ),+ "Could not locate the prism/ source tree from #filePath."+ )+ return try String(+ contentsOf: root.appendingPathComponent(file), encoding: .utf8+ )+ }++ @Test("The compact TOC sheet is fed the tree and selects through navigateToHeading")+ func compactLayoutWiring() throws {+ let source = try Self.source(of: "Views/CompactDocumentLayout.swift")++ #expect(source.contains("TableOfContentsSheet("))+ #expect(source.contains("nodes: session.toc.tocTree"))+ #expect(source.contains("session.navigateToHeading("))+ }++ @Test("The regular TOC sidebar is fed the tree and selects through navigateToHeading")+ func regularLayoutWiring() throws {+ let source = try Self.source(of: "Views/RegularDocumentLayout.swift")++ #expect(source.contains("TOCSidebar("))+ #expect(source.contains("nodes: session.toc.tocTree"))+ #expect(source.contains("session.navigateToHeading("))+ }++ /// The details-nested row's disclosure must be expansion-bound.+ ///+ /// `DisclosureGroup(content:label:)` with no `isExpanded:` binding starts+ /// COLLAPSED, while every top-level row binds to `collapsedSectionIds` —+ /// empty on a fresh document — and starts expanded. A `<details>` block with+ /// two heading levels then listed the outer heading and hid the inner one.+ /// Nothing about the default is observable from the model, so this is the+ /// only place it can be pinned.+ @Test("The details-nested TOC disclosure is expansion-bound, so it cannot start collapsed")+ func nestedDisclosureIsExpansionBound() throws {+ let source = try Self.source(of: "Views/SidebarContentsView.swift")++ #expect(!source.contains("DisclosureGroup {"))+ #expect(source.contains("DisclosureGroup(isExpanded: $isNestedExpanded)"))+ }+}
diff --git a/prismTests/TOCEntryRowTests.swift b/prismTests/TOCEntryRowTests.swiftdeleted file mode 100644index 79e504da..00000000--- a/prismTests/TOCEntryRowTests.swift+++ /dev/null@@ -1,171 +0,0 @@-//-// TOCEntryRowTests.swift-// prismTests-//-// Created by Claude on 14/1/2026.-//--import SwiftUI-import Testing-@testable import prism--/// Tests for TOCEntryRow styling and display.-///-/// These tests verify the view's styling calculations. Since SwiftUI views-/// don't expose computed values directly, we test the underlying logic-/// by instantiating the view and checking its behavior through previews-/// or by extracting testable logic into separate functions.-///-/// Requirements covered:-/// - 3.3-3.4: Indentation calculation (12pt per level)-/// - 3.5: Font selection for hierarchy-/// - Empty heading placeholder text-@Suite("TOCEntryRow Styling")-struct TOCEntryRowStylingTests {-- // MARK: - Indentation Tests-- @Test("H1 has zero indentation")- func h1HasZeroIndentation() {- let indent = calculateIndentation(for: 1)- #expect(indent == 0.0)- }-- @Test("H2 has 12pt indentation")- func h2Has12ptIndentation() {- let indent = calculateIndentation(for: 2)- #expect(indent == 12.0)- }-- @Test("H3 has 24pt indentation")- func h3Has24ptIndentation() {- let indent = calculateIndentation(for: 3)- #expect(indent == 24.0)- }-- @Test("H4 has 36pt indentation")- func h4Has36ptIndentation() {- let indent = calculateIndentation(for: 4)- #expect(indent == 36.0)- }-- @Test("H5 has 48pt indentation")- func h5Has48ptIndentation() {- let indent = calculateIndentation(for: 5)- #expect(indent == 48.0)- }-- @Test("H6 has 60pt indentation")- func h6Has60ptIndentation() {- let indent = calculateIndentation(for: 6)- #expect(indent == 60.0)- }-- @Test("Indentation formula is (level - 1) * 12")- func indentationFormulaCorrect() {- for level in 1...6 {- let expected = CGFloat(level - 1) * 12.0- let actual = calculateIndentation(for: level)- #expect(actual == expected, "Level \(level) should have \(expected)pt indent")- }- }-- // MARK: - Font Selection Tests-- @Test("H1 uses title3 style")- func h1UsesTitle3Style() {- #expect(TOCEntryRow.styleForLevel(1) == .title3)- }-- @Test("H2 uses headline style")- func h2UsesHeadlineStyle() {- #expect(TOCEntryRow.styleForLevel(2) == .headline)- }-- @Test("H3 uses subheadline style")- func h3UsesSubheadlineStyle() {- #expect(TOCEntryRow.styleForLevel(3) == .subheadline)- }-- @Test("H4 uses callout style")- func h4UsesCalloutStyle() {- #expect(TOCEntryRow.styleForLevel(4) == .callout)- }-- @Test("H5 uses footnote style")- func h5UsesFootnoteStyle() {- #expect(TOCEntryRow.styleForLevel(5) == .footnote)- }-- @Test("H6 uses caption2 style")- func h6UsesCaption2Style() {- #expect(TOCEntryRow.styleForLevel(6) == .caption2)- }-- // MARK: - Font Weight Tests-- @Test("H1 uses semibold weight")- func h1UsesSemiboldWeight() {- #expect(TOCEntryRow.weightForLevel(1) == .semibold)- }-- @Test("H2 uses medium weight")- func h2UsesMediumWeight() {- #expect(TOCEntryRow.weightForLevel(2) == .medium)- }-- @Test("H3 uses medium weight")- func h3UsesMediumWeight() {- #expect(TOCEntryRow.weightForLevel(3) == .medium)- }-- @Test("H4 uses regular weight")- func h4UsesRegularWeight() {- #expect(TOCEntryRow.weightForLevel(4) == .regular)- }-- @Test("H5 uses regular weight")- func h5UsesRegularWeight() {- #expect(TOCEntryRow.weightForLevel(5) == .regular)- }-- @Test("H6 uses regular weight")- func h6UsesRegularWeight() {- #expect(TOCEntryRow.weightForLevel(6) == .regular)- }-- // MARK: - Empty Heading Placeholder Tests-- @Test("Empty heading text shows placeholder")- func emptyHeadingShowsPlaceholder() {- let displayText = displayTextForHeading("")- #expect(displayText == "(Empty heading)")- }-- @Test("Non-empty heading text is preserved")- func nonEmptyHeadingPreserved() {- let displayText = displayTextForHeading("Introduction")- #expect(displayText == "Introduction")- }-- @Test("Whitespace-only heading shows placeholder")- func whitespaceOnlyHeadingShowsPlaceholder() {- // The TOCEntry text comes from the parser which handles whitespace,- // but if empty string reaches the row, it should show placeholder- let displayText = displayTextForHeading("")- #expect(displayText == "(Empty heading)")- }-- // MARK: - Helper Functions-- /// Calculates indentation matching TOCEntryRow logic.- /// Formula: (level - 1) * 12.0 points- private func calculateIndentation(for level: Int) -> CGFloat {- CGFloat(level - 1) * 12.0- }-- /// Returns display text matching TOCEntryRow logic.- private func displayTextForHeading(_ text: String) -> String {- text.isEmpty ? "(Empty heading)" : text- }-}
diff --git a/prismTests/WebRendering/WebReloadNavigationClaimTests.swift b/prismTests/WebRendering/WebReloadNavigationClaimTests.swiftindex 31acc57b..8e973cca 100644--- a/prismTests/WebRendering/WebReloadNavigationClaimTests.swift+++ b/prismTests/WebRendering/WebReloadNavigationClaimTests.swift@@ -190,7 +190,13 @@ struct WebReloadNavigationClaimTests { "the requested heading must outrank the saved reading position on the reloaded revision" ) #expect(- !controller.pendingCommands.contains(.scrollToBlock(domID: savedBlockID)),+ !controller.pendingCommands.contains {+ // Matched on the id alone: the restore carries+ // `armsReassert: false` (T-1928), so a whole-value comparison+ // against the defaulted spelling would pass vacuously.+ if case .scrollToBlock(let domID, _) = $0 { return domID == savedBlockID }+ return false+ }, "no queued scroll may still land on the saved position" ) }
diff --git a/prismTests/WebRendering/WebSearchScrollOwnershipTests.swift b/prismTests/WebRendering/WebSearchScrollOwnershipTests.swiftindex 5f3de6a3..48884ba2 100644--- a/prismTests/WebRendering/WebSearchScrollOwnershipTests.swift+++ b/prismTests/WebRendering/WebSearchScrollOwnershipTests.swift@@ -433,7 +433,14 @@ struct WebSearchScrollOwnershipTests { #expect(commands.last == .setSearchState( json: "{\"query\":\"n\",\"blocks\":{}}", reveal: true ))- if let scrollIndex = commands.firstIndex(of: .scrollToBlock(domID: "b-saved-0")) {+ // Matched on the id alone: the replayed restore carries+ // `armsReassert: false` (T-1928), and this `if let` would otherwise+ // drop the assertion entirely rather than fail.+ let scrollIndex = commands.firstIndex {+ if case .scrollToBlock(let domID, _) = $0 { return domID == "b-saved-0" }+ return false+ }+ if let scrollIndex { #expect(scrollIndex < commands.count - 1) } }
diff --git a/prismTests/WebRendering/WebFragmentNavigationPrecedenceTests.swift b/prismTests/WebRendering/WebFragmentNavigationPrecedenceTests.swiftindex 3c110266..5fe173eb 100644--- a/prismTests/WebRendering/WebFragmentNavigationPrecedenceTests.swift+++ b/prismTests/WebRendering/WebFragmentNavigationPrecedenceTests.swift@@ -143,7 +143,7 @@ struct WebFragmentNavigationPrecedenceTests { ) #expect( !assembly.controller.pendingCommands.contains {- if case .scrollToBlock(let domID) = $0 { return domID == savedBlockID }+ if case .scrollToBlock(let domID, _) = $0 { return domID == savedBlockID } return false }, "no queued scroll may still land on the saved position"
diff --git a/specs/table-of-contents/decision_log.md b/specs/table-of-contents/decision_log.mdindex e6b0c012..064e73a2 100644--- a/specs/table-of-contents/decision_log.md+++ b/specs/table-of-contents/decision_log.md@@ -1,5 +1,13 @@ # Decision Log: Table of Contents +## Quick Decisions++| ID | Date | Decision | Rationale |+|----|------|----------|-----------|+| Q1 | 2026-08-23 | `NAVIGATION_REASSERT_MS = 1000` for the navigation re-assert window (Decision 14) | No derivation available: it must outlive the dispatch skew between three independent `Task { await page.callJavaScript(…) }` calls and be short enough that a reader action inside it is rare. 1 s satisfies both by inspection; neither bound is measurable today |+| Q2 | 2026-08-23 | Delete `TOCEntryRow` and the TOC sheet's flat `.entries` mode rather than sharing their helpers | Both were already unreachable once Decision 13 made `TOCNode` the only production model; sharing helpers with dead code leaves the dead code |+| Q3 | 2026-08-23 | The TOC row's dimming stays bound to DOCUMENT collapse, while its accessibility value follows the disclosure the reader operates | They are different facts: a collapsed TOC sublist on a details-nested row hides no document content, so dimming it would misreport |+ ## Decision 1: Button Placement in Toolbar **Date**: 2026-01-14@@ -398,3 +406,256 @@ Proper internationalization is essential for global users. SwiftUI handles most - May require testing with RTL content ---++## Decision 13: One Production TOC Model, a Navigation Tree++**Date**: 2026-08-22+**Status**: accepted++### Context++Two heading models coexisted. `MarkdownSectionBuilder.build(from:)` produced+`DocumentStructure`, a containment tree of `MarkdownSection` keyed on top-level+`sourceIndex`, and both shipped TOC surfaces — `TableOfContentsSheet` on iPhone+and `TOCSidebar` on iPad/macOS — rendered it. That builder only scans top-level+`parsedBlocks`, so a `.details` block is ordinary direct content and the+headings inside it were never traversed: they were absent from the TOC and+unreachable from it (T-1928).++`TOCCoordinator` produced the other model, a flat `[TOCEntry]` that DOES descend+into `<details>` and records `detailsAncestorIds` and a resolvable+`scrollTargetId`. Nothing in production consumed it except fragment lookup, so+`TOCEntryTests` had been verifying an unused path for months.++Wiring the expansion path exposed a latent defect in that producer: a top-level+`<details>` block's `compositeId` — the value that becomes a+`detailsAncestorIds` element — was built from the flattened `runningIndex`+rather than the top-level `blockIndex`, so from the second `<details>` block+onward it matched nothing in `DetailsExpansionCoordinator.domIDByDetailsPath`.+It is the same index-space confusion T-1662 fixed for `scrollTargetId`, and it+survived because no production code consumed the ancestor ids. It is fixed as+part of this decision, since this decision is what starts consuming them.++### Decision++`TOCCoordinator` owns the single production TOC model. It gains `tocTree`, a+`[TOCNode]` hierarchy built from its existing details-aware entries, and both+TOC surfaces render that instead of `DocumentStructure`. `DocumentStructure` is+left exactly as it was and keeps its containment role. Selection routes through+one method, `DocumentSession.navigateToHeading(_:)`, which opens the enclosing+`<details>` blocks through `DetailsExpansionCoordinator`, expands any collapsed+heading section hiding the target, and only then publishes the scroll target.++### Rationale++The alternative the ticket offered — making `MarkdownSectionBuilder`+details-aware — fails on the identity of `MarkdownSection`. Every question that+model answers is keyed on a top-level `sourceIndex`: `visibleBlocks`,+`headingPathByBlockId`, `noteCount`, note relocation, `sectionId(forBlockId:)`,+`expandSectionsContaining(blockAtIndex:)`. A heading inside `<details>` has no+top-level index, so folding it in means inventing one, and every one of those+consumers then indexes notes, heading paths, and collapse state against a+position that does not exist. Nor is the containment answer even wanted: a+heading inside a disclosure does not own the blocks that follow the closing+`</details>`.++The TOC asks a narrower question — what to list, and where selecting it should+go — so it gets its own model. Building the tree from `TOCEntry` also retires+the second model rather than adding a third: the entries were already produced,+already correct about details, and already tested.++Hierarchy is by heading level with one extra rule: a candidate parent's+`detailsAncestorIds` must be a PREFIX of the child's. Without it a heading+inside a disclosure would adopt the top-level headings that follow the closing+tag, and two sibling `<details>` blocks with identically-levelled headings would+nest into each other.++### Alternatives Considered++- **Extend `MarkdownSection` with nested-details headings**: Add+ `detailsAncestorIds` and an explicit navigation target to the section model -+ Rejected because nested headings have no top-level `sourceIndex` and the+ model's other five consumers all key on one; every nested section would need+ an exclusion flag threaded through `visibleBlocks`, the heading-path index,+ `allSectionIds`, and collapse resolution.+- **Render the flat `[TOCEntry]` list in both surfaces**: Use+ `TOCCoordinator.tocEntries` directly via the sheet's legacy entries mode -+ Rejected because it drops the disclosure hierarchy and section collapse the+ sidebar provides; rebuilding those over a flat list reimplements+ `DocumentStructure`.+- **Leave both models and only fix selection**: Rejected because the headings+ are not listed at all — there is nothing to select.++### Consequences++**Positive:**+- Headings inside `<details>` appear in both TOC surfaces and navigate correctly+- One selection path, so the compact sheet, the regular sidebar, and fragment+ links cannot drift+- `DocumentStructure` is untouched, so notes, relocation, and collapse carry no+ risk from this change+- A row now shows the stripped heading text (`FootnoteStripping` ++ `HTMLCommentStripping`) that the flat entries always carried; the section tree+ showed raw heading source++**Negative:**+- Two heading models still exist in the codebase, now with clearly separate+ roles (navigation vs. containment) rather than an accidental overlap+- A details-nested heading offers no collapse control in the TOC, which reads as+ an inconsistency until you know it owns no section+- A nested heading still scrolls to the OUTERMOST enclosing `<details>` rather+ than its immediate parent; that limitation is `TOCEntry.scrollTargetId`'s and+ is tracked by T-1924++### Impact++`prism/Models/TOCNode.swift` (new), `prism/Models/TOCEntry.swift`,+`prism/Services/TOCCoordinator.swift`, `prism/Models/DocumentSession.swift`,+`prism/Views/SidebarContentsView.swift`, `prism/Views/TOCSidebar.swift`,+`prism/Views/TableOfContentsSheet.swift`,+`prism/Views/CompactDocumentLayout.swift`,+`prism/Views/RegularDocumentLayout.swift`,+`prism/Views/TOCEntryRow.swift` (deleted, with `prismTests/TOCEntryRowTests.swift`),+and — for the page-side half recorded as Decision 14 —+`prism/Resources/WebRenderer/prism-scroll.js`,+`prism/Resources/WebRenderer/prism-bridge.js`,+`prism/Resources/WebRenderer/prism-theme.js`,+`prism/ViewModels/WebBridgeContract.swift`,+`prism/ViewModels/WebDocumentController.swift`, and+`docs/agent-notes/webview-rendering-status.md`.++---++## Decision 14: Page-Side Ordering Immunity for Details Navigation++**Date**: 2026-08-23+**Status**: accepted++### Context++Decision 13 made a heading inside a `<details>` block selectable from the TOC.+Selecting one mutates three pieces of native state in a single synchronous call+(`DocumentSession.navigateToHeading`): the details open-set, the collapsed+heading sections, and the scroll target. `WebDocumentStateSynchronizer` reads+them in one coalesced pass, but `WebDocumentController.dispatch(_:)` sends each+resulting command in its own fire-and-forget `Task { await+page.callJavaScript(…) }`. Nothing awaits the state pushes before the scroll, so+their arrival order at the page is unspecified — the open **T-2028** race.++Before T-1928 no user-reachable path depended on that order, because a+details-nested heading was not listed anywhere. This one does, and landing the+scroll first fails in two distinct ways: under a collapsed heading the target is+`display:none`, so the scroll resolves to the nearest RENDERED section (the+T-1944 fallback, right for a stale stored position and wrong for a navigation+whose expansion is still in flight); and a target near the document end is+clamped by the CURRENT document height, so opening the `<details>` afterwards+makes the document taller without moving the reader.++Two further questions came with it. `setDetailsState` changes the laid-out+height of the document with no scroll event and no section-visibility change, so+a listener that must re-derive geometry after it has nothing to hang off — it+needs a hook. And both display-state commands re-send their WHOLE current state+on every push, so a listener cannot tell a real change from a re-affirmation.++### Decision++Do not order the commands natively. Make this one path order-INSENSITIVE on the+page side instead: `prism-scroll.js` remembers an arming `scrollToBlock` target+for `NAVIGATION_REASSERT_MS` (1 s) and re-asserts the scroll whenever a+display-state push lands inside that window AND the ids that actually flipped+touch the target's chain.++`setDetailsState` gets its own bridge hook, `onDetailsStateApplied`, rather than+widening `onSectionVisibilityChanged`. `prism-theme.js` computes the changed-id+diff (`diffIdSets`) at the point that owns the previous state and passes it+through both hooks. T-2028 stays open.++### Rationale++Ordering the commands natively is the correct fix and a much larger one: it+means giving `WebDocumentController.dispatch` a sequencing discipline that every+command and every recovery/replay path must then honour, for a race that affects+one known path today. Re-asserting at the consumer makes the order irrelevant+for that path at a fraction of the surface, and it is idempotent — the target is+already where the re-assert would put it whenever the order was fine.++What makes a second scroll authority in the page tolerable is that it is bounded+on four sides: a deadline measured from the ORIGINAL scroll and never extended+by a re-assert; any explicit navigation; any scroll the page did not initiate,+or any direct reader input (`wheel`/`touchmove`/`pointerdown`/navigation key,+none of them gated on the programmatic-scroll suppression window); and the+changed-id relevance gate. Two supporting rules follow from the same principle+that a re-assert is not new intent: it does not call `notifyExplicitNavigation`+(injection order puts it before prism-search.js's deferred-reveal listener on+the shared hook, so announcing would discard a NEWER search reveal), and only an+explicit navigation arms the window at all — `scrollToBlock` carries+`armsReassert`, false for the stored-position restore.++The dedicated hook is a scoping decision, not a stylistic one.+`onSectionVisibilityChanged` means "a heading collapse changed which sections+are laid out", and its only other listener is prism-search.js's re-windowing and+deferred-reveal delivery. Widening it to also mean "a disclosure opened" would+smuggle a search behaviour change into a scroll fix, with the search side having+no way to tell the two causes apart.++### Alternatives Considered++- **Fix T-2028: order the commands natively**: Await the state pushes before+ dispatching the scroll, or send one composite command per user action -+ Rejected for this ticket's scope: it changes the dispatch contract for every+ command and every replay/recovery path, and it is a larger and riskier change+ than the bug warrants. It remains the right eventual fix; the agent note says+ so explicitly, and any NEW order-dependent path still needs it.+- **Widen `onSectionVisibilityChanged` to also fire for `setDetailsState`**:+ Reuse the existing hook instead of adding one - Rejected because its only+ other listener is search's deferred-reveal delivery, which would then fire on+ disclosure toggles it has no business reacting to, with no way to+ discriminate. A hook whose meaning is widened silently changes every existing+ listener.+- **Re-assert on ANY display-state push in the window** (the first shipped+ version): Rejected after review - both commands restate their whole state, so+ an unrelated `<details>` toggle echoing back re-triggered the re-assert and+ yanked the reader to an earlier target, and a second navigation's state push+ landing before its own scroll re-asserted the first target. The changed-id+ diff is what distinguishes them, and it can only be computed where the+ previous state lives.+- **Have the re-assert compare payload membership instead of a diff**: Rejected+ for the same reason - membership is identical between a real change and a+ re-affirmation.++### Consequences++**Positive:**+- A TOC selection into a closed `<details>` lands correctly whichever of the+ three commands arrives last, without touching the dispatch contract+- The mechanism is idempotent and time-bounded, so the no-race case is unchanged+- The dedicated hook keeps the search listener's meaning intact+- The changed-id diff is reusable by any future listener that needs to know what+ a state push actually moved++**Negative:**+- T-2028 stays open, and a future order-dependent path gets no protection from+ this — it needs its own immunity or the native fix+- There is now a second, time-bounded scroll authority in the page, whose+ correctness rests entirely on four bounds that a later edit could relax+ without any test noticing the intent+- The relevance chain mixes two id spaces (`setSectionState` composites and+ `setDetailsState` DOM ids). It is safe because block hashes are dash-free hex,+ so a composite can never begin `b-`, but that is a property of the id formats+ rather than of the code, and it is easy to break from a distance+- `NAVIGATION_REASSERT_MS = 1000` is a judgement call with no derivation: it has+ to outlive the dispatch skew between three independent tasks and be short+ enough that a reader action inside it is rare++### Impact++`prism/Resources/WebRenderer/prism-scroll.js` (the re-assert, the chain, the+cancellation listeners), `prism/Resources/WebRenderer/prism-bridge.js` (the+`onDetailsStateApplied` hook and the `changedIds` argument),+`prism/Resources/WebRenderer/prism-theme.js` (`diffIdSets` and both push sites),+`prism/ViewModels/WebBridgeContract.swift` and+`prism/ViewModels/WebDocumentController.swift` (the `armsReassert` flag),+`prismTests/WebRendering/WebDetailsNavigationOrderingTests.swift`, and+`docs/agent-notes/webview-rendering-status.md`.++---
diff --git a/specs/bugfixes/toc-details-headings/report.md b/specs/bugfixes/toc-details-headings/report.mdnew file mode 100644index 00000000..ddde0f48--- /dev/null+++ b/specs/bugfixes/toc-details-headings/report.md@@ -0,0 +1,382 @@+# Bugfix Report: Production TOC Omits Headings Inside Details Blocks++**Date:** 2026-08-23+**Status:** Fixed+**Ticket:** T-1928++This report covers the whole change, which landed in three commits: the native+TOC model (`64984b96`), the page-side ordering immunity the new navigation path+needed (`64b17823`), and the ancestor-relevance gate that immunity needed after+review (`a1b95f6c`), plus the round-3 review fixes.++## Description of the Issue++Any markdown heading written inside a `<details>` block was missing from the+table of contents on every platform. Because the TOC is the only list of the+document's headings, there was no way to navigate to such a heading at all.++**Reproduction steps:**+1. Open a document containing a closed `<details>` block with a markdown heading+ inside it.+2. Open the compact TOC sheet (iPhone) or the regular left TOC sidebar+ (iPad/macOS).+3. Observe the nested heading is absent.++**Impact:** Medium. Navigation-only; no data loss. Affects every document that+uses collapsible sections with headings inside them, on both layouts. A document+whose headings live *entirely* inside `<details>` showed "No Headings".++## Investigation Summary++- **Symptoms examined:** Nested headings absent from both TOC surfaces;+ `TOCEntryTests` nevertheless asserted they were produced correctly.+- **Code inspected:** `MarkdownSectionBuilder`, `MarkdownSection` /+ `DocumentStructure`, `TOCCoordinator`, `DetailsExpansionCoordinator`,+ `SectionCollapseManager`, `SidebarContentsView`, `TOCSidebar`,+ `TableOfContentsSheet`, `CompactDocumentLayout`, `RegularDocumentLayout`,+ `DocumentSession`, and — once selection worked natively but did not land in+ the page — `WebDocumentStateSynchronizer`, `WebDocumentController.dispatch`,+ `prism-scroll.js`, `prism-theme.js`, `prism-bridge.js`.+- **Hypotheses tested and ruled out:**+ - *The parser drops nested headings* — ruled out: `MarkdownBlockParser`+ produces `.details(children:)` with the heading intact, and+ `TOCCoordinator.tocEntries` finds it.+ - *The views filter them out* — ruled out: `SidebarContentsView` renders+ whatever `DocumentStructure.sections` contains; the tree never had them.+ - *A DOM-id resolution failure hides the row* — ruled out: the row is never+ constructed, so nothing gets as far as resolving an id.++## Discovered Root Cause++Two heading models existed and production consumed the wrong one.++`MarkdownSectionBuilder.build(from:)` walks only the top level of+`parsedBlocks`. A `.details` block matches the `else if let current =+sectionStack.last` branch and is appended as ordinary direct content; its+`children` are never traversed. Both TOC surfaces render the resulting+`DocumentStructure` (`CompactDocumentLayout` -> `TableOfContentsSheet`,+`RegularDocumentLayout` -> `TOCSidebar`), so no nested heading could ever appear.++`TOCCoordinator.tocEntries` — which does recurse into `<details>`, records+`detailsAncestorIds`, and computes a resolvable `scrollTargetId` — was consumed+only by `DocumentSession.scrollToAnchor` (fragment links) and by tests. Its+details-handling code, and the tests covering it, were verifying a path no user+could reach.++**Defect type:** Missing traversal, compounded by duplicate models with the+unused one being the correct one.++**Why it occurred:** `DocumentStructure` was introduced later, for section+collapse and note heading-paths, and the TOC surfaces were migrated onto it+because it already carried the hierarchy the sidebar needed. The migration+inherited the containment model's top-level-only assumption, and the tests that+would have caught it were pointed at the model that was left behind.++**Contributing factors:** `TOCEntryTests` gave false confidence — the assertions+read as production coverage of nested details but exercised an unused producer.++### Two further defects the fix uncovered++1. **A latent index-space defect in the producer** (native).+ `buildTOCEntries` built a top-level `<details>` block's `compositeId` from+ the FLATTENED `runningIndex` rather than the top-level `blockIndex`. That id+ is the `detailsAncestorIds` element, matched against+ `DetailsExpansionCoordinator.domIDByDetailsPath`, which `buildAncestorMap`+ keys on `"{block.id}-{topLevelIndex}"`. The two counters agree only until the+ first `<details>` block's children advance `runningIndex` past `blockIndex`,+ so from the SECOND `<details>` block onward the ancestor id matched nothing,+ `openDetails(matching:)` fell through, and expansion did nothing. The same+ confusion T-1662 fixed for `scrollTargetId`; it survived because nothing in+ production consumed the ancestor ids.++2. **The commands that carry the selection are not ordered** (page).+ Selecting a nested heading mutates three pieces of native state in one+ synchronous call. `WebDocumentController.dispatch(_:)` sends each resulting+ command in its own fire-and-forget `Task { await page.callJavaScript(...) }`,+ so nothing awaits `setSectionState`/`setDetailsState` before `scrollToBlock`+ — the open **T-2028** race. Landing the scroll first fails two ways: under a+ collapsed heading the target is `display:none` and the scroll resolves to the+ nearest RENDERED section (the T-1944 fallback), and a target near the+ document end is clamped by the CURRENT document height, so opening the+ `<details>` afterwards makes the document taller without moving the reader.++## Resolution for the Issue++Two decisions, both recorded in `specs/table-of-contents/decision_log.md`:++- **Decision 13** — one production TOC model: a navigation tree owned by+ `TOCCoordinator`, rather than making `MarkdownSectionBuilder` details-aware.+- **Decision 14** — page-side ordering immunity for the new navigation path,+ rather than fixing T-2028 natively. T-2028 stays open.++### Native changes++- `prism/Models/TOCNode.swift` (new) — `TOCNode` hierarchy plus the pure+ `build(from:)` that assembles it from flat entries. Nesting is by heading+ level with the added rule that a parent's `detailsAncestorIds` must be a+ prefix of the child's, so `<details>` content cannot adopt the headings that+ follow its closing tag and sibling `<details>` blocks cannot nest into each+ other.+- `prism/Models/TOCEntry.swift` — added `topLevelBlockIndex`, the top-level+ `parsedBlocks` index of the enclosing block, so a nested selection can expand+ the collapsed heading section hiding it. Kept as its own field rather than+ parsed back out of the composite id, which would depend on block ids never+ containing a `-`.+- `prism/Services/TOCCoordinator.swift` — exposes and caches `tocTree`,+ invalidated with `tocEntries`; threads `topLevelIndex` alongside the existing+ `topLevelAnchor`; fixes the `runningIndex`/`blockIndex` defect above; spells+ the top-level composite ONCE per loop iteration (the defect was two+ independent spellings three lines apart, one of which drifted); and marks both+ derived caches `@ObservationIgnored`, reading `cachedBlocks` on every access+ instead, now that a VIEW BODY reads `tocTree` and the old shape wrote observed+ storage during view evaluation.+- `prism/Models/DocumentSession.swift` — new `navigateToHeading(_:)`, the single+ selection path: open the enclosing `<details>` via+ `DetailsExpansionCoordinator`, expand the collapsed heading section (by+ section id for a top-level heading, by top-level block index for a nested+ one), then publish `pendingAnchorScroll`. `scrollToAnchor` routes through it+ too.+- `prism/Views/SidebarContentsView.swift` — renders `[TOCNode]`; carries the+ details chevron, one additional level of indentation (Req 8.2) and the "Inside+ collapsible section" accessibility prefix; a details-nested node with children+ uses a `@State`-bound `DisclosureGroup` because it owns no `MarkdownSection`+ to collapse, and that binding must start EXPANDED. The accessibility value is+ derived from `isRowExpanded`, which is the only expression correct for both+ row kinds — `isCollapsed` is unconditionally false for a nested row, so a+ collapsed one used to announce "Expanded".+- `prism/Views/TOCSidebar.swift`, `TableOfContentsSheet.swift` — consume+ `[TOCNode]`. The sheet's flat `.entries` mode, its `@FocusState`, and+ `prism/Views/TOCEntryRow.swift` (with `prismTests/TOCEntryRowTests.swift`)+ were deleted: nothing could reach them once the tree became the only model.+- `prism/Views/CompactDocumentLayout.swift`, `RegularDocumentLayout.swift` —+ pass `session.toc.tocTree` and call `session.navigateToHeading(node.entry)`.+- `prism/ViewModels/WebBridgeContract.swift`,+ `prism/ViewModels/WebDocumentController.swift` — `scrollToBlock` gains+ `armsReassert` (defaulted true). `restoreScroll` sends it false, so a+ stored-position restore scrolls without claiming the page's re-assert window;+ the snapshot carries the flag so a recovery replay re-issues the command the+ session actually made.++### Page changes++- `prism/Resources/WebRenderer/prism-scroll.js` — the re-assert. An arming+ `scrollToBlock` target is remembered for `NAVIGATION_REASSERT_MS` (1 s) and+ re-scrolled whenever a display-state push lands in that window and touches the+ target's chain. Bounded on four sides: a deadline measured from the ORIGINAL+ scroll and never extended by a re-assert; any explicit navigation; any scroll+ the page did not initiate, plus `wheel`/`touchmove`/`pointerdown`/navigation+ keys (ungated by the 600 ms programmatic-scroll suppression, because none of+ them fires for a scripted scroll); and the changed-id relevance gate. The+ chain is `[target]` + collapsible-heading ancestors + `<details>` ancestors ++ `<details>` DESCENDANTS of the target — descendants included because the+ clamping failure is relieved by any expansion that grows the document,+ including one INSIDE the target. A re-assert scrolls WITHOUT announcing+ (`performScrollToBlock(domID, false)`): injection order is bridge, scroll,+ theme, media, notes, search, so an announcing re-assert cleared+ prism-search.js's `revealPending` before that listener could read it, dropping+ a newer deferred reveal.+- `prism/Resources/WebRenderer/prism-bridge.js` — new `onDetailsStateApplied` /+ `notifyDetailsStateApplied` hook, and a `changedIds` argument on the existing+ section-visibility hook. Deliberately NOT folded into+ `onSectionVisibilityChanged`, whose only other listener is prism-search.js's+ re-windowing and deferred-reveal delivery.+- `prism/Resources/WebRenderer/prism-theme.js` — `diffIdSets`, plus the previous+ id set retained by both `setSectionState` and `setDetailsState`. Both commands+ restate their WHOLE current state on every push, so membership alone cannot+ distinguish a real change from a re-affirmation; the diff has to be computed+ where the previous state lives.++**Approach rationale:** `MarkdownSection` is keyed on a top-level `sourceIndex`+that a nested heading does not have, and five consumers depend on that key+(`visibleBlocks`, `headingPathByBlockId`, `noteCount`, note relocation,+`expandSectionsContaining`). Making the section builder details-aware means+inventing an index for nested headings and threading an exclusion flag through+all of them. The TOC asks a narrower question, so it gets a navigation model;+`DocumentStructure` is untouched. On the page side, ordering the commands+natively is the correct fix and a much larger one — it changes the dispatch+contract for every command and every replay path — so this path is made+order-insensitive instead, idempotently and under four bounds.++**Alternatives considered:** the full lists, with rejection reasons, are+Decision 13 and Decision 14. In short: extending `MarkdownSection` (corrupts the+containment key), rendering the flat `[TOCEntry]` list (drops hierarchy and+collapse), fixing T-2028 natively (correct, much larger, still open), widening+`onSectionVisibilityChanged` (changes search behaviour silently), and+re-asserting on any push in the window (the first shipped version — an unrelated+disclosure toggle yanked the reader back).++## Regression Tests++### `prismTests/TOCDetailsHeadingsTests.swift`++Suite `TOC includes headings inside <details> (T-1928)` — 11 tests, all driven+through `session.toc.tocTree` and `session.navigateToHeading`, over markdown put+through a real `parseContent()`:++- `treeIncludesNestedHeading` — the production model lists a heading inside a+ closed `<details>`.+- `sectionStructureStillOmitsNestedHeading` — pins the deliberate split: the+ containment model still omits it, so nobody "fixes" this by inventing a+ `sourceIndex`.+- `nestedNodeShape` / `topLevelNodeStaysCollapsible` — nested nodes carry their+ ancestors, target the enclosing `<details>` block, and offer no collapse;+ top-level nodes still match their `MarkdownSection.id`.+- `selectingNestedHeadingOpensDetails` — selection puts the details block's DOM+ id into `DetailsExpansionCoordinator.openDetailsDOMIDs` (what the state+ synchronizer pushes as `setDetailsState`) before publishing the scroll target.+- `selectingTopLevelHeadingLeavesDetailsClosed` — no over-expansion.+- `selectingHeadingInSecondDetailsOpensIt` — a document with two `<details>`+ blocks; pins the flattened-vs-top-level index fix. Deliberately the SECOND+ block, because the two counters agree on the first.+- `fragmentNavigationOpensDetails` — the fragment path gets the same treatment.+- `headingAfterDetailsIsASibling` / `siblingDetailsDoNotNest` /+ `nestedHeadingLevelsInsideOneDetails` /+ `hierarchyMatchesSectionTreeWithoutDetails` — tree shape, including that a+ document without `<details>` is grouped exactly as the section tree grouped+ it.++Suite `Both TOC surfaces are wired to the shared selection path (T-1928)` — 3+tests. Textual scans of `CompactDocumentLayout`, `RegularDocumentLayout` and+`SidebarContentsView`. A direct-invocation test cannot see a layout that quietly+inlines `session.pendingAnchorScroll = id` again — which is the state this bug+was found in — nor that the nested disclosure is expansion-bound, which is+observable nowhere else.++### `prismTests/WebRendering/WebDetailsNavigationOrderingTests.swift`++Suite `TOC navigation into a closed <details> survives command reordering+(T-1928)` — 9 tests over a real `WebPage` with the bundled `document.css`+injected (the failure is geometry, so the real cascade has to govern it),+dispatching the commands in the LOSING order on purpose. Every fixture asserts+its own order-sensitivity first — that the scroll really was clamped, or really+did fall back to the top — so none can pass vacuously.++- `scrollDispatchedBeforeDetailsOpenStillLands` — the losing order still lands.+- `detailsOpenBeforeScrollLands` — the winning order is unaffected.+- `lateStatePushDoesNotClobberTheReader` — past the deadline and past the+ reader's own scroll, a state push moves nothing.+- `unrelatedDetailsPushDoesNotReassertTarget` — a push for a DIFFERENT+ `<details>` does not re-assert a still-pending navigation; ends with a+ positive control (a push that DOES touch the target re-asserts) proving the+ target was still armed, so the negative assertion is not satisfied by an+ expired window.+- `secondNavigationStatePushesDoNotReassertFirstTarget` — a second navigation's+ state push arriving before its own scroll does not transiently re-scroll to+ the first target; same positive control, then the second navigation lands.+- `collapsedHeadingExpansionReassertsTarget` — the `setSectionState` half of the+ gate: the target is hidden under a collapsed heading, and the expansion push+ that belongs to the same navigation re-asserts it. The only thing that can+ match here is `collapsibleHeadingAncestorIds`, the level-stack walk.+- `descendantDetailsExpansionReassertsTarget` — a nested `<details>` opening+ INSIDE the target relieves the clamp; the changed id is a descendant, not an+ ancestor.+- `readerInputCancelsThePendingReassert` — two phases in one fixture: the+ re-assert fires without the input, and does not fire after a `wheel` event.+- `storedPositionRestoreDoesNotArmTheWindow` — `armsReassert: false` scrolls+ without claiming the window.++### Run command++```+xcodebuild build-for-testing -project prism.xcodeproj -scheme prism \+ -destination 'platform=macOS' -testPlan prism -derivedDataPath ./DerivedData++xcodebuild test-without-building -project prism.xcodeproj -scheme prism \+ -destination 'platform=macOS' -testPlan prism \+ -only-test-configuration "en (base)" -parallel-testing-worker-count 1 \+ -derivedDataPath ./DerivedData \+ -resultBundlePath /tmp/T-1928.xcresult \+ -only-testing:prismTests/TOCDetailsHeadingsTests \+ -only-testing:prismTests/TOCProductionWiringTests \+ -only-testing:prismTests/TOCCoordinatorTests \+ -only-testing:prismTests/TOCEntryTests \+ -only-testing:prismTests/WebDetailsNavigationOrderingTests++Tools/check-test-results.sh /tmp/T-1928.xcresult+```++`-derivedDataPath ./DerivedData` on BOTH steps: the Makefile builds there, and a+raw `xcodebuild` otherwise uses the global derived data, so the two steps do not+share products. `Tools/check-test-results.sh` is the verdict, not the exit code.++## Affected Files++| File | Change |+|------|--------|+| `prism/Models/TOCNode.swift` | New navigation-tree model and builder |+| `prism/Models/TOCEntry.swift` | Added `topLevelBlockIndex` |+| `prism/Services/TOCCoordinator.swift` | Cached `tocTree`; thread top-level index; composite-id fix; `@ObservationIgnored` caches |+| `prism/Models/DocumentSession.swift` | `navigateToHeading(_:)`; `scrollToAnchor` routed through it |+| `prism/Views/SidebarContentsView.swift` | Renders `[TOCNode]`; details indicator + indent; non-collapsible nested rows; expansion-accurate a11y value |+| `prism/Views/TOCSidebar.swift` | Signature change to `[TOCNode]` |+| `prism/Views/TableOfContentsSheet.swift` | Tree-only; flat `.entries` mode deleted |+| `prism/Views/TOCEntryRow.swift` | **Deleted** — unreachable once the tree became the only model |+| `prism/Views/CompactDocumentLayout.swift` | Feeds `tocTree`; selection via `navigateToHeading` |+| `prism/Views/RegularDocumentLayout.swift` | Feeds `tocTree`; selection via `navigateToHeading` |+| `prism/ViewModels/WebBridgeContract.swift` | `scrollToBlock` gains `armsReassert` |+| `prism/ViewModels/WebDocumentController.swift` | Restore sends `armsReassert: false`; snapshot carries it |+| `prism/Resources/WebRenderer/prism-scroll.js` | Navigation re-assert, ancestor/descendant chain, cancellation listeners |+| `prism/Resources/WebRenderer/prism-bridge.js` | `onDetailsStateApplied` hook; `changedIds` on the section hook |+| `prism/Resources/WebRenderer/prism-theme.js` | `diffIdSets`; both push sites carry the diff |+| `prismTests/TOCDetailsHeadingsTests.swift` | New regression suite + production-wiring pins |+| `prismTests/WebRendering/WebDetailsNavigationOrderingTests.swift` | New live-WebKit ordering suite |+| `prismTests/TOCEntryRowTests.swift` | **Deleted** with the view it covered |+| `prismTests/WebRendering/WebFragmentNavigationPrecedenceTests.swift`, `WebReloadNavigationClaimTests.swift`, `WebSearchScrollOwnershipTests.swift` | Match `scrollToBlock` on the id alone, so `armsReassert` cannot make an assertion vacuous |+| `specs/table-of-contents/decision_log.md` | Decisions 13 and 14, plus Quick Decisions Q1-Q3 |+| `docs/agent-notes/webview-rendering-status.md` | T-2028 / re-assert note, four bounds |+| `CHANGELOG.md` | Unreleased -> Fixed |++## Verification++**Automated** (counts in the PR body):+- `prismTests/TOCDetailsHeadingsTests`, `TOCProductionWiringTests`,+ `TOCCoordinatorTests`, `TOCEntryTests`,+ `WebRendering/WebDetailsNavigationOrderingTests` — all pass, verified through+ `Tools/check-test-results.sh` rather than the exit code.+- `make build-macos`, `make build-ios` — succeed, zero warnings.+- `make lint` — 0 violations.+- `make verify-test-isolation` — passes; every WebKit-touching test here is+ `async` in a `@MainActor` suite.+- The full suite was NOT run: several `xcodebuild` runs share this machine, so+ validation was restricted to targeted classes.++**Mutation checks** (each applied, run, and reverted):+- Reverting `CompactDocumentLayout`/`RegularDocumentLayout` to the+ `documentStructure` source, or `navigateToHeading` to the old+ `expandSectionAndAncestors` + `pendingAnchorScroll` pair — turns+ `TOCDetailsHeadingsTests` red.+- Stubbing BOTH `collapsibleHeadingAncestorIds` and `detailsAncestorSectionIds`+ to `[]` — turns `collapsedHeadingExpansionReassertsTarget` red. (Before the+ round-3 tests it turned nothing red: the only positive test matched on the+ target's own id.)+- Removing the descendant clause from `changedIdsAffectTarget` — turns+ `descendantDetailsExpansionReassertsTarget` red.+- Ignoring `payload.armsReassert` and always arming — turns+ `storedPositionRestoreDoesNotArmTheWindow` red.++## Prevention++- A model that is produced but not consumed is not covered, however many tests+ point at it. Tests for a user-facing behaviour should enter through the+ production assembly (`session.toc.tocTree`, `session.navigateToHeading`), not+ through the producer that happens to be convenient.+- When two counters agree for the first element of a sequence, a single-element+ fixture proves nothing about either. The composite-id regression test+ deliberately uses the SECOND `<details>` block.+- A negative test needs a positive control in the same fixture, or it passes for+ every reason the thing under test stopped happening — including the ones the+ test is not about.+- When two models describe the same thing, state which is authoritative for+ which question in the type's own documentation — `TOCNode` and+ `DocumentStructure` now each say so.++## Related++- T-2028 — bridge commands belonging to one user action are not ordered against+ each other. Still open; this fix makes one path immune rather than fixing it.+- T-1662 — the index-space divergence `TOCEntry.scrollTargetId` documents+- T-1924 — nested `<details>` targeting the outermost block rather than the+ immediate parent (still open; unchanged by this fix, and the reason Req 8.3 is+ recorded as partially covered)+- T-1930 — search reveal inside nested details; separate ticket, also touches+ `DetailsExpansionCoordinator`
diff --git a/docs/agent-notes/webview-rendering-status.md b/docs/agent-notes/webview-rendering-status.mdindex 8412051e..a03d566a 100644--- a/docs/agent-notes/webview-rendering-status.md+++ b/docs/agent-notes/webview-rendering-status.md@@ -260,3 +260,17 @@ take down unrelated applications' web content. Always PID-verify, never pattern- - **Swift 6 warnings under `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor`**: pure value-types/statics/globals/value-type-conformances read from nonisolated contexts just need `nonisolated`; an existential stored `nonisolated let` needs its protocol `: Sendable`. Catalog "declared in catalog but no source reference found" = orphaned keys — remove from `Localizable.xcstrings`, verifying exactly-orphaned via parsed-JSON set difference vs the warning list (don't trust a `git diff | grep` of the xcstrings — it mis-reports). - **The macOS test daemon wedges hard under machine contention this session** — `make test-quick`/`test-locales`/full-suite runs hang in test-START or finalization repeatedly. `pkill -9 -f "xctest|testmanagerd|test-without-building"` to clear; run targeted `-only-testing:prismTests/<Class>` (build-for-testing + test-without-building); if even that hangs, the per-test results are written to the xcresult **Staging** session logs during execution (readable before finalization), and behaviour-neutral refactors can lean on the clean build + lint. **When several worktrees run tests concurrently** (parallel bug-blitz agents), don't `pkill` the daemon blindly — check `ps` for other `xcodebuild test`/`testmanagerd` processes first; a wedge on your own bundle is more often a `resultBundlePath` collision or genuine contention than a global wedge, and a blind kill takes down siblings' runs too. A run whose xcresult never got past `Staging/` (no top-level summary) is the wedge signature — `Tools/check-test-results.sh` reports `could not read a test summary` for it; just retry with a fresh `-resultBundlePath`. - **T-1676 ("footnote refs inside HTML comments leak visible badges") does not reproduce** — investigated 2026-08-17. The ticket's own description cites `BlockHTMLEmitter.swift:712-748` scanning the whole inline source and substituting badges before `InlineHTMLRenderer` parses it; that architecture was replaced by the `FootnoteReferenceScanner` pre-pass (T-1716/T-1945, merged 2026-07-27), and the search-side half it also cites (`SearchStateFeeder`/`MarkdownBlock` not honouring the comment-visibility gate) was closed by T-1365 and T-1713 (2026-07-29). The ticket was filed 2026-07-06, before all three. Verified against current code with 15+ shapes driven through the real two-parse pipeline (`prismTests/WebRendering/FootnoteHTMLCommentIntegrationTests.swift`), including the ticket's own literal repro, multi-line comments, duplicate identifiers split live/hidden, and both `showHTMLComments` states — none leak a badge. Two real but orthogonal findings surfaced during the sweep, neither of which is a badge leak: (1) a comment written with extra dashes (`<!--- text --->`) is not valid CommonMark comment syntax (content ends in `-` right before `-->`), so it is plain Text from the very first parse — smart punctuation then renders the extra dashes as an em/en-dash — and a live reference in it badges correctly *because it is not actually inside a comment*; this is correct parser behaviour, not a defect. (2) CommonMark's HTML-block grammar is line-quantised: a paragraph whose reformatted `textContent` starts with `<!--` (e.g. after `stripEmptyAnchors` deletes everything before it) reparses, on the `InlineHTMLRenderer`'s per-block `Document(parsing:)` call, as a top-level `HTMLBlock` rather than inline content — the exact `visitHTMLBlock`-gap class already tracked as **T-2206** — and separately, at the FIRST full-document parse, a comment that IS a line's entire content still swallows same-line trailing text into that HTML block (CommonMark's end condition is "line contains `-->`", not "stop at `-->`"), which `MarkdownBlockParser` then classifies as mixed content and falls back to `.html(content:)` with the comment stripped and the trailing text — including any footnote reference in it — rendered as literal, unprocessed text (under-rendering, the opposite of a leak). Neither is T-1676's reported symptom; don't re-investigate them under this ticket if it resurfaces.++- **Bridge commands that belong to ONE user action are not ordered against each other.** `WebDocumentStateSynchronizer.dispatch` reads every state domain in one coalesced pass, but `WebDocumentController.dispatch(_:)` sends each command in its own fire-and-forget `Task { await page.callJavaScript(…) }` — nothing awaits the state pushes before a scroll. That is the open **T-2028** race, and it is unfixed: T-1928 did NOT order the commands. It made one path immune instead, on the page side. `prism-scroll.js` remembers an ARMING `scrollToBlock` target for 1s (`NAVIGATION_REASSERT_MS`) and re-asserts it whenever `setSectionState` or `setDetailsState` applies inside that window, so a TOC selection of a heading inside a closed `<details>` lands correctly whichever command arrives last. Two distinct failures it covers: a `display:none` target resolving to the T-1944 nearest-rendered fallback, and a scroll near the document end being CLAMPED by a height a later `<details>` open increases.++ The re-assert is bounded on **four** sides, and those bounds are the whole reason a second scroll authority in the page is tolerable — do not relax them:+ 1. a fixed deadline measured from the ORIGINAL scroll and never extended by a re-assert;+ 2. any `notifyExplicitNavigation` (a genuinely NEW navigation) drops it;+ 3. any scroll the page did not initiate drops it, as does any direct reader input — `wheel`/`touchmove`/`pointerdown`/a navigation key, none of which is gated on the 600ms programmatic-scroll suppression window, because none of them fires for a scripted scroll;+ 4. **the ancestor-relevance gate** — a display-state push re-asserts only when the ids that actually FLIPPED touch the target's chain. That fourth bound is the round-2 fix and the easiest to lose: both commands re-send their WHOLE current state on every push, so membership alone cannot tell a real change from a re-affirmation. `prism-theme.js`'s `diffIdSets` computes the changed set at the only place that holds the previous state, and the hooks forward it. Without it, toggling an unrelated `<details>` yanked the reader back to an earlier target, and two navigations inside 1s jumped backwards transiently.++ The chain the gate matches against is `[target] + collapsible-heading ancestors + <details> ancestors + <details> DESCENDANTS of the target`. The descendants half is not symmetry for its own sake: the clamping failure is relieved by ANY expansion that grows the document, including one INSIDE the target (nested `<details>`, outer already open — the changed id is `{targetDOMID}-d{index}`, which is a descendant, not an ancestor). It mixes two id spaces on purpose and safely: `setSectionState` ships `MarkdownSection` composites (`{hash}-{sourceIndex}`), `setDetailsState` ships DOM ids (`b-{hash}-{occurrence}`), block hashes are dash-free hex so a composite can never begin `b-`, and each command's ids can only ever need to match their own half. The chain deliberately EXCLUDES the target's own composite — collapsing the heading you navigated to does not hide that heading.++ Two more properties that read as details and are not: a re-assert does **not** announce (`performScrollToBlock(domID, false)`) — it is the same intent landing again, and injection order is bridge, scroll, theme, media, notes, search, so an announcing re-assert ran BEFORE prism-search.js's deferred-reveal listener on the same hook and cleared `revealPending` out from under a NEWER search reveal. And only an EXPLICIT navigation arms the window at all: `scrollToBlock` carries `armsReassert`, false for `WebDocumentController.restoreScroll`, because a stored-position restore is a position the reader never chose and a `<summary>` click in the first second of opening a document must not be answered by yanking them to it.++ `setDetailsState` gets its own bridge hook (`onDetailsStateApplied`) rather than reusing `onSectionVisibilityChanged`, whose only other listener is prism-search.js's re-window/deferred-reveal delivery. Pinned by `WebDetailsNavigationOrderingTests`, whose fixtures are deliberately order-sensitive (each asserts the scroll was clamped, or fell back to the top, BEFORE the state push lands) and whose two negative tests each end in a positive control proving the target was still armed. **Any NEW path whose correctness depends on command order still needs T-2028**, or its own immunity.
diff --git a/CHANGELOG.md b/CHANGELOG.mdindex bb517458..e7576823 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Headings written inside a collapsible section now appear in the table of contents, on iPhone and on iPad/Mac, and choosing one opens the section it lives in before scrolling to it (T-1928). The contents list was built from a model that only ever looked at the top level of the document, so a heading inside a `<details>` block was missing from it entirely and there was no way to navigate to it — even though a separate, unused model in the app had been collecting those headings all along. A nested heading is now listed under whichever heading precedes it, marked with the same chevron the app already uses elsewhere for collapsible content, and is not itself collapsible from the contents list: the collapsible section it sits in is the thing that opens and closes. Following a link to a nested heading's anchor opens its section too, which it previously did not. A document with no collapsible sections is grouped and ordered exactly as before, with two deliberate improvements that also reach it: a heading containing a footnote marker or an HTML comment now lists with those stripped out, matching what the iPhone sheet always showed, and a heading with no text at all now reads "(Empty heading)" instead of appearing as a blank row. Fixing the navigation also uncovered a second problem in the same area, which is fixed here too: from the second collapsible section in a document onwards, the app was identifying those sections by a position that shifts as it counts through the contents of earlier ones, so it could not find them to open. Nothing had noticed because nothing had ever asked it to open one this way. Choosing a heading also no longer competes with the position the app restores when you reopen a document: only a heading you chose yourself is held briefly and re-applied once the section it lives in has opened, and any scroll, wheel flick, key press or click of your own cancels that immediately. - An image that is tiny as a file but enormous as a picture can no longer exhaust memory or terminate Prism, whether it comes from the web, from a file beside the document, or written directly into the markdown (T-2132, T-2149, T-2151, T-1867). A picture is stored compressed, and a plain-coloured one compresses at about a thousand to one — so a 400 KB download can be a 20,000 by 20,000 image that needs about 400 MB the moment anything tries to display it, and four times that if it is in colour. Prism's limits were all written on the wrong side of that: a 50 MB cap on the download said nothing about the picture inside it, and the 2 MB cap on a local SVG was applied only after the whole file had already been read, so a very large one could freeze the app on its way to being refused. The limits that did exist covered only images fetched from the web; the same image referenced from a file next to your document, or embedded inline in the markdown, went straight to the renderer unchecked. Prism now reads the picture's dimensions from its header — a few bytes, before anything is decoded — and decides from that. An ordinary image is displayed as before. A very large one referenced from the web or from a file is scaled down to fit. One beyond any reasonable size is refused outright and shows the usual "Image failed to load" placeholder, rather than being handed to a decoder that would have to build the whole thing first. How large a picture is now also accounts for how much detail each dot of it carries: most pictures store one byte per colour, but some store two or four, and Prism previously assumed the smaller size for all of them and so under-counted the deep ones by half or three quarters. One consequence you may see: a very large deep-colour photograph that used to display at full size is now scaled down, because its true size was always above the limit and is now measured as such. Files are now read up to their limit instead of read whole and then measured — including the copy Prism keeps of a document you have not saved yet, which is restored when the app reopens. How much decoding happens at once is limited by how much memory those pictures actually need rather than by how many of them there are, so a page full of large images no longer overruns while appearing to stay within its bounds. Two things behave differently, both deliberately. An image whose file does not say how big it is, or what kind of dots it stores, now shows the "Image failed to load" placeholder instead of being displayed — there is no way to know what it would cost until it has already cost it. And an image written directly into the markdown is treated more strictly than the same image kept in a file beside the document: it is either small enough to display as it is or refused, never scaled down. That difference is about memory rather than effort. Scaling a picture that is written into the markdown means rebuilding it and writing the smaller version back into the page, where it then stays for as long as the document is open — which costs more memory, for longer, than not showing it. A picture in a file has somewhere else to keep its smaller version, so it can be scaled instead of refused. Animated images are unaffected in either case: they play as before, however many frames they have. - A verification scan that starts during the app's initial entitlement bootstrap can no longer publish a stale result while a newer scan is still in flight (T-2152). While `entitlementState` was still `.loading`, any scan's result was accepted regardless of whether a more recent scan — for example one started right after `AppStore.sync()` — was still reading the world; the older scan finishing first could briefly flip the paywall to locked (or unlocked) ahead of the newer, more current answer. An older result that arrives while a newer scan is still outstanding is now held back rather than published. If the newer scan goes on to answer, its fresher result is published and the held-back one is simply dropped; if instead it is cancelled without ever answering, the held-back result is released, so a cancelled scan cannot leave the paywall stranded on `.loading`. The trade is that the brief loading state now ends when the last overlapping scan answers rather than the first, so it can last marginally longer; every control it gates is disabled meanwhile, so nothing silently does nothing. - Saving a pasted document to a file no longer disturbs whatever document you opened next (T-2213). A save finishes in two parts: the file is written straight away, but the document only becomes that file once its notes have been moved across, and on a slow iCloud connection that second part can still be running after you have closed the document or opened another one. When it finished late, it acted on the document then on screen instead of the one it had saved: the pasted text of that other document was deleted from the place Prism keeps unsaved documents — so it could no longer be recovered after a relaunch — its entry in Recent Files was labelled with the wrong document's title, and an action you had queued behind its own Save prompt could run without you confirming it. A save that failed to move its notes also raised an alert naming a file you were no longer looking at. Each of these now belongs to the document that was actually saved, and the document on screen is left alone. Its Recent Files entry is labelled with its own title rather than the other document's. Where that other document had itself started saving in the meantime, the late save no longer takes over the shortcut that document had prepared for its own file, which can leave the saved file without a Recent Files entry of its own. The file is saved either way, and can be opened from the Files app.
Collapse the last major heading of a long document from the page, scroll to the top, then pick that heading from the TOC. The scroll should stop short of the top and stay there after the section expands. If it lands correctly, the dispatch happened to win the race — retry, or drive it directly with setSectionState([id]), scrollToBlock(domID), setSectionState([]) in the live harness, which is the losing order the suite already knows how to produce.
Confirmed and expected. git merge-tree HEAD origin/main reports exactly one conflict, in CHANGELOG.md. Every other file — including DocumentSession.swift, which main also touched — auto-merges. The merge queue resolves it; nothing to do pre-push.
Two runs, both through Tools/check-test-results.sh: 151 tests across 11 suites (WebDetailsNavigationOrderingTests, TOCDetailsHeadingsTests, TOCCoordinatorTests and the eight scroll/navigation suites the enum widening touches), then 58 across four more (TOCProductionWiringTests, TOCNavigationDetailsIndexTests, TOCEntryTests, DetailsExpansionCoordinatorTests). 209 executed, 209 passed, 0 skipped, both bundles reporting result=Passed, and every named suite appears in the log. Full suite not run, per this review's constraints. make lint 0 violations across 556 files, make verify-test-isolation OK, make build-macos and make build-ios both succeeded with zero compiler warnings.
Worth one pass with VoiceOver on a document shaped # A / paragraphs / # B, with A collapsed from the page: the row renders dimmed and announces "Expanded". Both TOC surfaces share the row now, so the behaviour is identical on iPhone and on iPad/Mac.
Round 1's first nit is resolved by documentation rather than deletion, which is the better call: prism-scroll.js:609-617 now explains that every scrollToBlock domID resolves against the top-level blocks array, so main.closest("details") is null and the loop exits on its first iteration — always — and that the function is kept for the day T-1924 makes a nested block a navigation target. Re-read it when T-1924 lands.
The field's comment calls it inert. It is not inert — the replayed commands are independent tasks, so a display-state push can land after the replayed scroll and re-assert it. That outcome is right, but it means the recovery path is relying on the fix rather than sidestepping it, which is a different claim from the one the comment makes.