Skip to content

fix(preview): restore the scroll position to the block it was taken from - #420

Merged
PathGao merged 1 commit into
masterfrom
fix/preview-scroll-anchors
Aug 3, 2026
Merged

fix(preview): restore the scroll position to the block it was taken from#420
PathGao merged 1 commit into
masterfrom
fix/preview-scroll-anchors

Conversation

@PathGao

@PathGao PathGao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Switch tabs and come back, and the preview lands on a rough percentage of the document rather than where you left off.

Why

The line-based restore walks only markdownBody.children, but processMarkdownHtml moves everything after a heading into a .foldable-content-wrapper — a JS-created element with no data-sourcepos.

What is actually left at the top level, measured on real output rather than assumed: the shallowest-level headings, plus whatever precedes the first one.

flat h2 document      955 top-level children, 478 with data-sourcepos
nested h1>h2>h3       319 top-level children, 160 with data-sourcepos

So a saved anchor resolved only when it happened to fall on one of those heading lines. Everything else fell through to the percentage fallback.

(The audit note that started this said the top level retains roughly four elements. That is wrong, and the correct mechanism matters for the fix — the surviving elements are the headings, which is exactly what the old loop was hitting.)

Hit rate, before and after

Same fixture, same 300 evenly-sampled anchor lines, both algorithms run over the same post-processMarkdownHtml DOM. The pre-fix loop is restated verbatim in the test as a permanent control:

10 000-line document, 6 202 annotated elements before after
flat, 477 h2 23 / 300 — 7.7 % 300 / 300
nested h1>h2>h3, 159 h1 7 / 300 — 2.3 % 300 / 300

The fix descends rather than widening the query

An element without a source position is treated as a transparent container whose span comes from its first and last annotated descendants. Two obvious alternatives were tried first and rejected on concrete grounds:

Giving the wrapper its first child's range does not fix it — that range covers only the first block, so every line deeper in the section still misses. The union-span variant does work at the top level, but it poisons the capture side: the wrapper precedes its own contents in document order, so it would win the distance === 0 early break and coarsen every anchor that gets written down.

A flat querySelectorAll('[data-sourcepos]') deep scan is what makes getPreviewScrollAnchor cost 18–41 ms per scroll event — a layout read per element — and it has two correctness holes: it returns the outermost container rather than the narrowest, and it reads offsets inside height: 0; overflow: hidden collapsed folds, where children still report their un-collapsed positions and would scroll to the wrong place.

The descent gets the narrowest block, stops at a collapsed fold (returning the zero-height wrapper, which sits at the correct offset just below its heading), and reads layout exactly once.

Effect on the per-scroll cost

Not fixed, and deliberately not widened. getPreviewScrollAnchor still does its flat scan on every scroll event. Two things did improve:

  • The restore path does 1 layout read instead of ~9 000. Traversal alone, measured in the DOM shim over the 10k fixture: descent 0.31 ms/lookup vs flat scan 1.26 ms/lookup — and that excludes layout, which is where the browser's 18–41 ms actually goes.
  • A future fix for the scroll-event cost can reuse the same descent shape keyed on pixels instead of lines.

parseSourceposLineRange was duplicated between the component and the new module; it is now imported. The magic 60 on both the capture and restore sides is now one PREVIEW_ANCHOR_OFFSET.

Tests

findAnchorElement regressed to the pre-fix top-level scan, tests unchanged 4 of 8 fail, and the measured rate collapses to exactly the control — 23/300 (7.7%) and 7/300 (2.3%), actual: 23, expected: 300
Module and wiring stashed suite fails to load
Final 8 / 8
npm run check   437 files, 0 errors
npm test        517 / 517
cargo test      131 / 131

Not covered

  • An anchor on a blank line, or stale after an edit → returns null and falls through to the existing percentage restore. Unchanged; the capture side only ever emits lines inside a block range.
  • offsetTop is not measured against the scroll container. .markdown-body has no position, so offsets are relative to a positioned ancestor above it. Kept on purpose — capture compares offsetTop to scrollTop + 60 and restore sets scrollTop = offsetTop − 60, so the constant cancels in the round trip, and switching only the restore side to getBoundingClientRect would have introduced a real error. But .markdown-body pre and .markdown-body .footnotes li are position: relative, so a match resolved inside a code block or a footnote measures against that ancestor. Pre-existing on the capture side; not addressed here.
  • No browser round-trip test. Resolution is measured over real processMarkdownHtml output via the existing DOM shim and the pixel maths is tested as a pure function, but capture→restore end to end is reasoned, not measured.
  • The fixture is comrak-shaped, not comrak-produced (the JS side has no renderer; the shape comes from the recorded convert_markdown output in renderProtocolFixtures.ts). Tables, footnotes, blockquote callouts and front-matter panels are not in it.
  • The sibling scan is linear in the sections preceding the match (~720 childNodes expansions per lookup on the 10k fixture). No binary search: sibling range monotonicity is not structurally guaranteed, and restore is low-frequency.

🤖 Generated with Claude Code

Switching tabs and coming back landed on a rough percentage of the
document instead of where you left off. The line-based restore was
looking only at the preview body's direct children, and
`processMarkdownHtml` moves everything after a heading into a
`.foldable-content-wrapper` that carries no source position. What is
left at the top level is the shallowest-level headings and whatever
precedes the first one, so a saved anchor resolved only when it happened
to fall on one of those lines.

Measured over the real `processMarkdownHtml` output for a 10 000-line
document with 6 202 annotated elements, 300 evenly sampled anchors:

  flat, 477 h2       23 / 300 =  7.7%  ->  300 / 300
  nested h1>h2>h3     7 / 300 =  2.3%  ->  300 / 300

The fix descends instead of widening the query. An element without a
source position is treated as a transparent container whose span comes
from its first and last annotated descendants, so the search reaches the
narrowest block that contains the line, stops at a collapsed fold rather
than reading offsets inside `height: 0` where children still report
un-collapsed positions, and reads layout exactly once.

Giving the wrapper its first child's range does not work - that range
covers only the first block, so every line deeper in a section still
misses - and the union-span variant would poison the capture side, where
the wrapper precedes its own contents in document order and would win the
`distance === 0` break, coarsening every anchor that is written down. A
flat deep query is what makes `getPreviewScrollAnchor` cost 18-41ms per
scroll event, and it returns the outermost container rather than the
narrowest one.

`parseSourceposLineRange` was duplicated between the component and the
new module, and the magic 60 on both sides is now one constant.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant