Align the source editor with the clicked block in both previews - #577
Open
gordonwoodhull wants to merge 5 commits into
Open
Align the source editor with the clicked block in both previews#577gordonwoodhull wants to merge 5 commits into
gordonwoodhull wants to merge 5 commits into
Conversation
Clicking a block in the q2-preview did nothing to the Monaco source editor, while the HTML preview scrolled. Root cause, confirmed in a real browser: `Q2PreviewIframe` drove preview->editor sync off a `click` listener, but q2-preview activates block editing on `pointerup`, and activation replaces the clicked element's subtree with a synthetic edit region. Chromium dispatches no `click` at all when the `pointerup` target was detached during `pointerup`, so the listener never ran for the one gesture that mattered. Fixed by listening for capture-phase `pointerup` and resolving the clicked element's nearest `[data-loc]` ancestor to a source line. Capture phase runs before the app's bubble-phase activation handler, while the target is still attached. The reveal is deliberately scroll-only -- no cursor move, no focus steal -- because the same click opens an inline editor *inside the preview*, and pulling focus to Monaco would break the gesture that click just started. `setPosition` would additionally feed editor->preview sync and bounce. The HTML preview's own path is left untouched. `lineForClickTarget` returns null rather than a line when the target is inside the active edit region, when the nearest located ancestor is a `<section>`, and when nothing resolves -- so a caret move inside an open editor does not yank the editor away. Worth recording: the plan's own pinned API specified narrowing the argument with `instanceof Element`. That is false for every real click -- the listener runs in the parent frame's realm against a target from the sandboxed iframe's realm, and each realm has its own `Element` constructor. Thirteen green jsdom rows passed against a guard that returned null in production; only the real-browser row caught it. The shipped guard is duck-typed, and row U1f reproduces the cross-realm case at the jsdom tier so a revert reddens a fast test. Plan, decisions and the full test-seam spec: claude-notes/plans/2026-08-21-preview-click-to-editor-scroll.md
None of these could have been caught by the test suite as written. Each came from a step beyond "the tests pass". The reveal was silently overwritten about 50ms after landing correctly. revealEditorLine never set the isSyncingRef flag, and by design it never takes focus, so both of syncPreviewToEditor's feedback-loop guards were unarmed: any real preview scroll inside the debounce window replaced the correct reveal with a scroll-ratio-derived position. Measured in a live browser: Monaco correctly at lines 149-189 (containing the clicked line 171) at t=1ms; a genuine 6px preview scroll at t=6ms as a rich-text toolbar mounted and reflowed the page; Monaco at 174-211 by t=83ms, with no second pointerup. Not specific to callouts -- any post-click reflow does this to any block. Fixed with the same isSyncingRef/300ms idiom the file already used elsewhere. The browser row guarding it asserts, as an explicit precondition, that the reflow it depends on actually fired, so it cannot quietly pass if the fixture ever stops reflowing. Clicking included content revealed a wrong line. lineForClickTarget parsed the fileId out of data-loc and never checked it, so clicking content from an included file revealed that file's line number as though it were a line in the file you had open -- a real, editable, wrong line. The plan claimed such clicks were left inert; they were not. Now a fourth null case, with a same-file control so the guard cannot be satisfied by rejecting everything. The guard against caret-move clicks was unbound. Deleting it reddened neither of its two tests: the unit fixture nested the edit region directly inside a located section, so a separate section guard returned null anyway and the test could not tell a working implementation from a broken one. Both now discriminate, proven in both directions. That last one surfaced in a six-hunk fail-on-revert pass over the finished test seam, which also established that dropping the capture-phase argument does not redden the browser test -- a detached paragraph still carries its own data-loc, and closest() called on it finds itself. The unit test's listener-registration assertion is therefore the only thing binding capture phase; don't delete it on the theory that the browser tier covers it.
Clicking a block revealed its source line centred in the editor. Now it aligns:
the block's first line of code lands at the same on-screen height as the block
you clicked, so the two panes line up across the split.
scrollTop = getTopForLineNumber(line) - (hostY - editorTop)
clamped to the editor's scroll range. Near the start or end of a document the
clamp wins and the panes cannot line up exactly; that is geometry, not a defect.
hostY is the clicked block's top edge in host-page coordinates. Two coordinate
spaces are involved: the block's rect is relative to the iframe's viewport, so
the iframe element's own offset in the host page has to be added. Omitting that
term misaligns by exactly the height of whatever chrome sits above the preview,
which is the symptom to look for. Only the browser test can catch it -- jsdom has
no layout -- and its 6px tolerance is set by measurement: reverting to the old
centring call misses by about 13px.
Alignment is unconditional, where the previous call did nothing when the line was
already visible. Alignment is a claim about where a line sits, so "on screen
somewhere" isn't good enough.
Click-align is deliberately not gated by the scroll-sync toggle, unlike the three
ambient-sync paths, so turning that toggle off leaves click-align as the only
coupling between the panes. A test pins this, because adding the missing gate
looks like a tidy consistency fix.
Plan and decisions: claude-notes/plans/2026-08-22-click-align-editor-y.md
…ncludes The HTML preview reaches the editor by a different route than q2-preview, and it already did more: its selection handler runs on selectionchange with no collapsed-selection guard, so a plain click already moved the cursor, centred the view, and took focus. Only the centring is replaced here. The cursor move and the focus steal stay -- this preview has no inline editor of its own to protect, which is the only reason q2-preview avoids them. The alignment anchor is the clicked span, not its containing block. The source location already reported is span- and column-precise, so anchoring to a coarser block's top would desync from the very line being reported -- on a paragraph that wraps, by a lot. The browser test therefore measures the span under the click, and uses a deliberately wrapping fixture, since a single-line paragraph could not tell span-anchored from block-anchored. revealEditorLine is threaded from useScrollSync into useSelectionSync rather than reimplemented or extracted into a shared helper. That is load-bearing, not stylistic: it brackets useScrollSync's own isSyncingRef, which is the flag syncPreviewToEditor reads, so the reveal-then-overwrite race is closed on this path too. A shared helper called from useSelectionSync would set that hook's unrelated flag and leave the race wide open. Don't decouple the hooks. Also fixes a pre-existing bug in the same function: it built a Monaco range without checking fileId, so selecting text inside included content moved the caret and focus to an unrelated line of the file you had open. That is worse than the q2-side equivalent, which only produced a wrong scroll -- here you could type into the wrong place.
Replaces the five per-commit entries the two-commit workflow produced while this was built. Their hashes did not survive squashing, and an entry cannot carry the hash of the commit it lives in, so the consolidated entry lands here in its own commit -- the same reason the two-commit workflow exists. Written for someone using the preview rather than reviewing the diff, since this renders in the About tab.
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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
2 participants
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.
Clicking a block in the q2-preview never moved the Monaco source editor. Clicking in the HTML preview moved it, but centered the line vertically. Both now align: the clicked block's first source line lands at the same on-screen Y position as the block itself, so the panes read side by side.
17 files, ~3050 insertions. TypeScript only — no Rust or Cargo files changed, which is why
cargo xtask verifyisn't part of this.Why q2-preview did nothing
Q2PreviewIframedrove preview→editor sync off aclicklistener on the iframe document. But q2-preview activates block editing onpointerup, and activation replaces the clicked element's subtree with a synthetic edit region — and Chromium dispatches noclickat all when thepointeruptarget was detached duringpointerup. So the handler never ran for the one gesture that mattered.Fixed by listening for capture-phase
pointerup, which runs before the app's bubble-phase activation handler while the target is still attached, and resolving the nearest[data-loc]ancestor to a source line.The alignment
clamped to the editor's scroll range. Near a document's start or end the clamp wins and the panes can't line up exactly — geometry, not a bug.
hostYcrosses two coordinate spaces: the clicked element's rect is relative to the iframe's viewport, so the iframe element's own offset in the host page is added. Dropping that term misaligns by exactly the height of the chrome above the preview. jsdom has no layout, so only the browser row can catch it; its 6px tolerance is set by measurement — reverting to the old centring call misses by ~13px.Two deliberate asymmetries between the previews:
setPositionwould feed editor→preview sync and bounce.Five defects the tests as written could not have caught
Worth reviewing in their own right — each came from a step beyond "the suite is green".
instanceof Element, which is false for every real click: the listener runs in the parent frame's realm against a target from the sandboxed iframe's realm, and each realm has its ownElement. Thirteen green jsdom rows passed against a guard that returnednullin production. Only the real-browser row caught it. The shipped guard is duck-typed, and a new row reproduces the cross-realm case at the jsdom tier so a revert reddens a fast test.revealEditorLinenever setisSyncingRef, and by design never takes focus — so both ofsyncPreviewToEditor's feedback guards were unarmed, and any post-click reflow replaced the reveal with a ratio-derived position. Measured: correct at t=1 ms, a 6px preview scroll at t=6 ms as a toolbar mounted, wrong by t=83 ms, with no secondpointerup.fileIdclicks weren't inert as the plan claimed — they revealed the included file's line number as a line in the open file.<section>, so a separate guard returnednullanyway and the row couldn't tell a working implementation from a broken one. Found by a six-hunk fail-on-revert pass over the finished seam.What to look at
scrollSyncDom.ts—lineForClickTarget's fournullcases and the duck-typed narrowing. Please don't "tidy" the latter back toinstanceof.useScrollSync.ts—revealEditorLineis threaded intouseSelectionSyncrather than extracted into a shared helper. That's load-bearing: it bracketsuseScrollSync's ownisSyncingRef, the flagsyncPreviewToEditorreads. A shared helper would set the other hook's unrelated flag and reopen (2) on the HTML path.Q2PreviewIframe.tsx/MorphIframe.tsx— the two-coordinate-spacehostYcomputation, in both previews.Tests
22 jsdom rows and 7 Playwright rows, each bound to a named production hunk. Every hunk was reverted and confirmed to redden its rows. Two rows are pinned against plausible-looking "fixes": one asserts click-align is not gated by the scroll-sync toggle (adding the gate looks tidy and would break it), and the browser race row asserts as a precondition that the reflow it depends on actually fired, so it can't quietly go vacuous.
hub-client1001 unit / 112 integration / 131 wasm ·preview-renderer549 unit / 590 integration ·typecheck:testsclean ·build:allclean · Playwright 7/7.One pre-existing unrelated failure:
custom-componentsEquation > appends \tag{N}under the pinned katex 0.18.1 — filed as bd-s36g9dav, fails identically onmain(invisible there only because a stalenode_modulespins katex 0.17.0).Deferred, deliberately
{{< include >}}shortcode's own line. Investigated; recommends a client-side scan over a producer-side Rust/wire change. Inert today, not wrong.local-prod-server.mjsdies on an unhandledECONNRESET; hit it twice while demoing.Plans:
claude-notes/plans/2026-08-21-preview-click-to-editor-scroll.md,claude-notes/plans/2026-08-22-click-align-editor-y.md. Both record the false premises they contained, so the next reader doesn't re-derive them.