Add bidirectional click-to-navigate between editor and preview#1526
Open
anuragkatyal wants to merge 6 commits into
Open
Add bidirectional click-to-navigate between editor and preview#1526anuragkatyal wants to merge 6 commits into
anuragkatyal wants to merge 6 commits into
Conversation
Clicking a rendered element moves the editor's cursor to (and centers) its source location; moving the editor's cursor scrolls the preview to follow, debounced against active typing. Works in both the VS Code extension's preview panel and DoenetEditor's built-in CodeMirror editor, since both share the same DocViewer-level mechanism: the core now includes each component's source position in its renderer instructions, and DocViewer maintains an id-to-position map (built from that stream) powering a single delegated click handler and a scrollToSourceOffset prop, with no changes needed in any individual renderer component. Also fixes @doenet/codemirror's library build, whose Vite config pointed lib.entry at CodeMirror.tsx instead of index.ts, silently dropping any runtime export added to index.ts from the published bundle (invisible until now since the only prior index.ts exports were type-only). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
The extension's language-server.test.ts mocks the vscode module by hand; it was missing window.onDidChangeTextEditorSelection, along with Range, Selection, TextEditorRevealType, and TextEditorSelectionChangeKind, all newly used by click-to-navigate's editor<->preview wiring in setupPreviewWindow. CI caught this: activate() crashed immediately on window.onDidChangeTextEditorSelection not being a function. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Review-cycle fixes for click-to-navigate: - EditorViewer: a preview click's programmatic cursor move no longer echoes back into the viewer. The old comment claimed DocViewer's offset dedup made the round-trip a no-op, but that only held when re-clicking the same element — any other click would, 120ms later, scroll the preview to re-center the clicked element under the user's pointer. Now suppressed with a flag mirroring the VS Code extension's suppressNextSelectionEcho (reliably consumed, since CodeMirror reports every transaction that carries a selection). - VS Code extension: skip the selection assignment (and the suppression flag) when the cursor is already at the clicked position — an identical assignment fires no selection-change event, so the flag would stay latched and swallow the next real cursor move. - DocViewer: verify a scroll candidate is actually in the DOM before choosing it. positionByDomId is only ever added to, so ids recorded from an earlier version of the source (or hidden components) linger; previously such a stale entry could win the best-match search and make the scroll silently do nothing. - Tests: abstract the duplicated click-reports-range / cursor-moves-to-source test bodies into helpers; fix an inaccurate offset comment. - Cleanup: drop the now-unused TextEditorSelectionChangeKind mock; use a type-only SourcePosition import in doenetml.tsx. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YTSdaxqmE3mYeBVeFQmFi5
Two robustness fixes from a follow-up review pass: - EditorViewer: clamp the clicked position's offset to the editor's current document length before dispatching the cursor move. Rendered positions index into the last-compiled source (viewerDoenetML), but the editor only recompiles on an explicit update, so after deleting text a click on a late-in-document element could dispatch a selection past doc.length — CodeMirror throws a RangeError, and the suppression flag set just before the dispatch would stay latched. - DocViewer: recordPositions now only records instructions whose position carries numeric start/end offsets (and whose id is actually a string). Point.offset is optional in the underlying DAST types (unist Point / Rust Option<usize>), while everything reading the map — the scroll-target search and both click paths — does arithmetic on the offsets; validating at the single entry point makes the map's SourcePosition value type honest instead of a hopeful cast. Both click-to-navigate Cypress specs (8 tests) pass against a fresh build. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YTSdaxqmE3mYeBVeFQmFi5
positionByDomId was only ever added to for the lifetime of a DocViewer instance, so in an editing session (EditorViewer / VS Code preview, where DocViewer stays mounted across recompiles) entries from every prior version of the source accumulated without bound, and a click could report offsets recorded from an earlier compile if the current instruction for that id happened to lack usable offsets. Clearing the map in the document-reset path bounds it to the current document; recordPositions repopulates it as the new core's instructions arrive. Also tighten the changeset's description of the @doenet/codemirror lib.entry fix: the package isn't npm-published itself, its built bundle is what @doenet/doenetml consumes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YTSdaxqmE3mYeBVeFQmFi5
The comment on DocViewer's scroll-to-source-offset effect claimed the fallback picks the element whose range *starts* closest to the offset, but the code measures distance to the nearest range boundary (start or end). Align the comment with the actual behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YTSdaxqmE3mYeBVeFQmFi5
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.
Summary
Adds bidirectional click-to-navigate between the DoenetML source editor and the rendered preview, in both surfaces that pair an editor with a live viewer:
DoenetEditor's built-in CodeMirror editor (the reusable editor+viewer component — e.g. for sites embedding it, such as doenet.org's activity editor)Clicking a rendered element moves the editor's cursor to (and centers) its source location. Moving the editor's cursor scrolls the preview to follow, debounced (~120ms) so it doesn't fight active typing.
How it works
RendererInstructionBuildernow includes each component's sourceposition(line/column/character-offset range) in the renderer instructions already sent to the UI — one additive field, no protocol changes.DocViewer(shared by both surfaces): maintains anid → positionmap built from that stream, with a single delegated click listener on the preview root and ascrollToSourceOffsetprop that scrolls the matching element into view. Only instructions whose position carries numeric start/end offsets are recorded (offsetis optional in the underlying DAST types, and every consumer of the map does offset arithmetic), and scroll candidates are verified to actually be in the DOM before they're chosen, so stale map entries (components not currently rendered, e.g. hidden or removed by an update) can't capture the scroll and make it silently do nothing. The map is cleared whenever the document itself resets (new source/attempt/variant), so offsets recorded for one version of the source are never carried into another and the map can't grow without bound across recompiles in an editing session. No changes needed in any of the ~97 individual renderer components.revealPositionmessage on click → extension callseditor.revealRange(centered) + sets the cursor; the extension listens toonDidChangeTextEditorSelectionand posts a debouncedcursorMovedmessage with the offset. An explicit suppression flag keeps the extension's own programmatic selection change from echoing back into the preview (akind-based filter can't do this — directeditor.selectionassignment fires withkind: undefined); when the cursor is already at the clicked position the assignment is skipped entirely, since an identical assignment fires no event and would leave the flag latched.EditorViewer(powersDoenetEditor): aCodeMirroreditorViewRefgives the viewer's click handler direct access to the underlyingEditorViewto dispatch a centered selection change (EditorView.scrollIntoView(pos, { y: "center" })); the existing per-keystrokeonCursorChangecallback is extended to also (debounced) drive the viewer'sscrollToSourceOffset. A suppression flag mirroring the VS Code extension's keeps a preview click's programmatic cursor move from echoing back and re-centering the clicked element under the user's pointer. Since rendered positions index into the last-compiled source while the editor may hold newer, shorter text (the viewer only recompiles on an explicit update), the clicked offset is clamped to the editor's current document length so the dispatch can't throw a selection-out-of-range error.Bonus fix
Found and fixed a real, separate bug in
@doenet/codemirror's build: its Vite config pointedlib.entryatCodeMirror.tsxinstead ofindex.ts, so anything exported only fromindex.tswas silently dropped from the runtime bundle (types still worked, since those are bundled separately — this went unnoticed because the only priorindex.ts-only exports were type-only).Known limitation
Content produced via
<copy>collapses to the<copy>tag's own source range rather than mapping to the exact nested element, since copied content doesn't retain per-element source positions independent of the tag that produced it.Test plan
tsc --noEmitclean across all touched packages (codemirror,doenetml,doenetml-worker-javascript,vscode-extension)DocViewer/clickToNavigate.cy.js,EditorViewer/editorViewerClickToNavigate.cy.js), 8 tests total, all passing — covering exact source-offset reporting on click, scroll-to-offset, staying correct across repeated interactions, and the editor centering the target line (not landing it at an edge)editorViewer.cy.jsfailures were reproduced identically at baseline (change fully reverted, clean rebuild) — pre-existing, unrelated to this PRdoenetmldev playground (DoenetEditor, both directions, including the centering fix)@doenet/vscode-extensionVitest suite passing🤖 Generated with Claude Code