docs(tabs): say which of the three scroll-position fields to use - #443
Merged
Conversation
`Tab` records the reading position three times - `scrollTop`, `scrollPercentage`, `anchorLine` - with nothing saying why one concept needs three fields or which one a caller should read. They are three increasingly degraded answers to the same question, tried in order until one resolves: a source line, a fraction of the scrollable range, then raw pixels. Each survives a different kind of change, and each is written by a different subset of the three writers, so the most recently written is not the most precise. One block above `scrollPercentage`/`anchorLine`, plus a one-line pointer on `scrollTop`, which sits apart from them in the interface. Comments only. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tabrecords the reading position three times —scrollTop,scrollPercentage,anchorLine— and says nothing about why one concept needs three fields or which one a caller should read. They are not interchangeable, they are not kept in step with each other, and the names do not tell you which is authoritative.Comments only.
npm test559/559,npm run check636 files / 0 errors,npm run buildclean, all measured on5fd9b8f.They are a fallback cascade, not three copies
Re-activating a tab tries them most-precise-first and stops at the first that resolves —
MarkdownViewer.svelte:1131-1163:The editor runs the same idea with a different first entry and one fewer step —
editorViewState, thenanchorLineviarevealLineNearTop, thenscrollPercentage(components/Editor.svelte:290-311). It never readsscrollTop:grep -n scrollTop src/lib/components/Editor.sveltereturns one line,e.scrollTopChanged.anchorLinescrollPercentagescrollTopThe history is the clearest statement of why each was added.
scrollTopis the original (bf04daa, "cursor/scroll persists between tabs").scrollPercentagearrived asedfb555, "roughly match viewer-editor scroll location" — a ratio, because the two panes have different heights.anchorLineis #420 (d9d5447), whose first sentence is the failure the ratio produced: "Switching tabs and coming back landed on a rough percentage of the document instead of where you left off."anchorLineis a source line both ways: the preview pins it atPREVIEW_ANCHOR_OFFSET = 60px(utils/previewAnchor.ts) and the editor at two lines below the viewport top (Editor.svelte:640writesstartLine + 2,:296restoresanchorLine - 2). That is why it is the only one that means anything across a pane swap — it names a place in the text, not a place on screen.Split-view scroll sync does not use any of the three. It is live DOM→DOM through
utils/scrollSync.ts(#442) and reads the elements' ownscrollTop, not the tab's.The part that is worth the comment
Three writers, each updating a different subset:
scrollTopscrollPercentageanchorLineMarkdownViewer.svelte:1281-1295):1274-1278)Editor.svelte:626-643)So the most recently written field is not the most precise one, and one of them can be current while its neighbours are stale. That is the sentence a reader needs, and it is not derivable from the names.
scrollTopalso has a second, unrelated consumer —isScrolled = scrollTop > 0(MarkdownViewer.svelte:252) drives the title bar's.scrolledshadow (TitleBar.svelte:344) — which is why the programmatic branch keeps it up to date at all.Also worth knowing and not written down anywhere: assigning any of the three scrolls nothing. The preview restore is an
$effectkeyed onactiveTabIdandmarkdownBodywith its bodyuntracked, so it runs on tab activation and body remount, not on write.All three documented, in one block
One block above
scrollPercentage/anchorLine, plus a one-line pointer onscrollTop.scrollTopsits five fields away from the other two in the interface and is not moved next to them — reordering an interface is a bigger diff than the comment it would tidy.Does not fit cleanly — reported, not fixed
navigate()resets the one field the cascade consults last. Following a Markdown link reuses the tab, andtabs.svelte.ts:786ends that with:scrollPercentageandanchorLineare left holding the previous document's values, and it isanchorLinethe restore reads first. ThescrollTop = 0branch is reachable only when the anchor fails to resolve andscrollPercentageis 0 — so for any tab that had been scrolled before the user clicked the link, the reset cannot take effect.The consequence is deferred rather than immediate: the restore effect does not re-run on same-tab navigation (
activeTabIdis unchanged, and the<article bind:this={markdownBody}>atMarkdownViewer.svelte:3299is not inside a block that remounts on a path change), so nothing jumps at the moment of navigation. It is the next activation of that tab that resolves the old document'sanchorLineagainst the new document and lands at whatever block now occupies that line number.I have evidenced this from the code, not reproduced it in the running app. The fix is a behaviour change — clearing all three, or moving the reset into a single "new document in this tab" helper — and belongs in its own PR.
Secondary, same shape: the programmatic-scroll early return at
MarkdownViewer.svelte:1274-1278skipsscrollPercentage/anchorLineas a side effect of the feedback-loop guard it shares a branch with (thereturnalso skipssyncEditorToPreviewScroll). So in split view, scrolling the editor moves the preview but leaves the tab's anchor where the last user preview scroll put it. Whether that is intended is not recoverable from the code, so the comment states the mechanism and not a rationale.Not traced
scrollHistory/scrollFuture(MarkdownViewer.svelte:2561-2587) — the in-preview scroll back/forward stack. Component-local, not onTab, and confirmed only to the extent that it setsisProgrammaticScrolland so lands in the row above.editorViewStatebeyond its position in the editor's cascade. It is Monaco's opaque blob and carries selection and folding as well as scroll, which makes it a different kind of thing from these three.collapsedHeadersfold state applied in the same activation.🤖 Generated with Claude Code