test(scroll-sync): cover the split-view mapping by running it - #442
Merged
Conversation
Scroll sync had 23 assertions and no coverage. The defect that set the sync threshold to a value which disabled sync outright left every one of them green, because they read Editor.svelte as text; both files were deleted in #433, so the behaviour is currently untested. The cause is structural: `node --test` cannot import a `.svelte` file, so the only thing a test can do to logic living inside one is grep it. The two functions that decide where the other pane scrolls to are pure arithmetic — `getScrollSyncPositionFromPixels` and `getScrollTopForSyncPosition` read nothing but their arguments — so they can simply leave the component, the same move that produced `documentSession` and `windowSession`. They were also duplicated. Both functions plus their `clampScrollRatio` helper existed byte for byte in Editor.svelte and MarkdownViewer.svelte: two private copies of one formula, invisible to each other and to the compiler. Both call sites now import `src/lib/utils/scrollSync.ts`, whose body is the moved code verbatim apart from `export` (md5 of all three blocks, de-indented: 31ab3110). scripts/scrollSync.test.ts imports and runs it. The cases are stated as a mapping between two panes with different proportions — the editor spends 20% of its scroll range on front matter, the preview 10% — because that asymmetry is the reason the position carries a section instead of a single ratio. 13 injected defects, 13 caught, including both spellings of "sync does nothing". 540 -> 552 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 3, 2026
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.
Scroll sync is the miss from the coverage audit that is easiest to close, because the part of it that can be wrong is arithmetic.
The injected defect set the sync threshold to a value that disabled sync entirely.
previewScrollSync.test.ts(23 assertions) andscrollSyncInput.test.tsstayed green through it. Both read a.sveltefile as text, so what they detect is that a particular line was edited — the first was deleted in #433 for exactly that. The behaviour is currently untested.That is not a property of those two files.
node --testcannot import.svelte, so 14,581 lines across 21 components have no executable coverage at all, and the only thing a test can do to logic inside one is grep it. The fix that works is the one that produceddocumentSessionandwindowSession: move the logic out. This is one function pair's worth of that.What moved, and the check that it moved unchanged
getScrollSyncPositionFromPixelsandgetScrollTopForSyncPositionread nothing but their arguments — no DOM, no Monaco, no component state, no closure over anything but the privateclampScrollRatiobeside them. They are pure arithmetic and leave the component with no wrapper and no seam.They were also duplicated. Both functions and
clampScrollRatioexisted byte for byte inEditor.svelte(editor side) andMarkdownViewer.svelte(preview side) — two private copies of one formula, each invisible to the other and to the compiler. Nothing imports a private function, so the two could have drifted silently; a fix applied to one pane's copy would have left the other pane mapping by the old formula, and split view would have desynchronised in one direction only. That is the bug classsingleImplementationConvention.test.tsexists for, and it could not see this one because a private identifier is not a legal marker there.Both call sites now import
src/lib/utils/scrollSync.ts. The diff on both components is a deletion plus an import;Editor.svelte'seditorReadyeffect from #423 is untouched.The move is byte-identical, not asserted to be. Taking the two pre-change blocks out of
git show HEAD:, removing the module'sexportkeywords and re-indenting it by one tab:What the mapping is for
The two panes are not proportional to each other. Front matter is ordinary text lines in the editor and a fixed-height panel in the preview, so the same document gives them different total scroll ranges and a different share of that range spent on front matter. The tests use 1000px total / 200px front matter for the editor and 600px / 60px for the preview — 20% against 10%.
A single
scrollTop / scrollMaxratio drifts under that. At the moment the editor finishes scrolling past its front matter,(200 / 1000) * 600 = 120pxin the preview is already 60px into the body. Splitting the range atfrontMatterEndand carrying a(section, ratio)pair instead maps front matter onto front matter, body onto body, and the boundary onto the boundary — 60, exactly.That number is the first assertion in the file, with the naive 120 computed alongside it so the test says why 60 is the answer.
Tests
scripts/scrollSync.test.ts, 12 tests, all of them calling the functions. NoreadFileSync, no regex over source.frontMatterEnddropped from the body arithmetic</<=at the section splitfrontMatterEndof 0scrollMaxof 0frontMatterEnd >= scrollMax, zero body rangeTwo of those are worth stating in full, because they are not restatements of the implementation.
The round trip is exact, including where the mapping looks degenerate. For a
scrollTopon either side of the boundary,position → pixelsreturns the same pixel andpixels → positionreturns the same position. It holds even when the body range is zero (frontMatterEnd >= scrollMax), because a body ratio of 0 resolves tofrontMatterEnd, which in that pane is the bottom of the scroll range.The boundary label can only be asserted as a label. At
scrollTop === frontMatterEnd,frontmatterat ratio 1 andbodyat ratio 0 resolve to the same pixel in every pane, so flipping<to<=is invisible in the target scrollTop. Assertingsection === 'body'there is not pedantry about the return shape: it is what stops a one-pixel scroll from jumping back to a ratio of 1.Mutation check
Each defect injected into
src/lib/utils/scrollSync.tson top of this change, the suite run, the file restored. 12 tests in the suite.frontMatterEnddroppedfrontMatterEnddropped from the reverse mapping<becomes<=frontMatterEndnot capped atscrollMaxscrollTopnot clamped to the scroll range13 injected, 13 caught. The first two rows are the two ways to write "sync does nothing", which is the defect the deleted tests could not fail.
Messages name the measurement, not the line:
The first pass had a fourteenth entry that came back green. It was a bad patch, not a gap — the injected early return sat behind
if (false && …)and never executed. Rewritten to actually fire, it is row 1.npm test540 → 552 passing,npm run check633 files / 0 errors,npm run buildclean.Not covered
revealFoldsAround/foldToggleForinFindBar.svelte— the same extraction, and the closest of the three.foldToggleFor(container, root)is already parameterised on the DOM it touches;revealFoldsAroundcloses only overmarkdownBodyand a module constant, so passing the root in makes it pure over a DOM interface. The behaviour worth pinning is an ordering claim — outermost fold opened first, so a nested one is on screen when its own toggle fires — andfoldMeasurementBatching.test.ts's recording stand-in already records exactly that kind of ordered event stream. Needs a stand-in forclassList/closest/matches/dispatchEventand aCSS.escapeshim, so a larger fixture than this PR's, but the same shape.exportAsPdfinexport.ts— importable already, so no extraction; the blocker is different in kind. It callsinvokeand the dialog plugin'ssaveas module-level imports, so a test needs thewindow.__TAURI_INTERNALS__stub thatcheckedReadMigration.test.tsandreopenDirtyDocument.test.tsalready use, plus a seam forsave. The claims are small and real: non-Windows invokesprint_pdfand never opens a dialog, Windows opens one, and a cancelled dialog must invoke nothing.macosPdfExport.test.tscovers the command-name half by grep and nothing covers the branch.reloadFromDisk's dirty-state guard inMarkdownViewer.svelte— not the same clean extraction: it has no arithmetic core, and what matters is the sequencing (canCloseTabresolves the buffer, thenloadMarkdownruns withdiscardUnsavedBuffer: trueto cover typing during the await). A predicate lifted out of it would not carry that. The realistic route is thedocumentSessionone —canCloseTabandloadMarkdownare both already indocumentSession.svelte.ts, which several test files drive for real against a stubbed backend, so moving the reload beside them makes it testable without a new technique. That is a behaviour-moving change with its own risk, not a mechanical lift, and belongs in its own PR.singleImplementationConvention.test.tscould take a row pinninggetScrollSyncPositionFromPixelstoscrollSync.ts, and now that the name is exported it would be a legal marker. Not added here: it is a source-text assertion, this PR's claim is that executable tests are what catch defects, and the two should not be argued in the same change.getEditorContentScrollMax,getEditorFrontMatterScrollEnd,getPreviewFrontMatterScrollEnd) still live in components, still read Monaco and the DOM, and are still uncovered. A wrongfrontMatterEndwould produce a correct mapping of a wrong number, and nothing here would notice.🤖 Generated with Claude Code