From b670b436c33fdf9784f6485e82e2c6a07082b49b Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 9 Aug 2026 06:26:27 -0700 Subject: [PATCH] fix(site-builder): stop CodeEditor fallback insert from being lost on CodeMirror mount insertAtCursor's textarea-fallback branch advanced lastEmittedRef to the post-insertion value. The mount effect's dynamic import() closes over `value` as of initial render, so if CodeMirror finishes loading after a fallback-mode insertion, it mounts with the pre-insertion doc. The only repair mechanism -- the value-sync effect -- is gated on `value !== lastEmittedRef.current`, so advancing that ref made the gate see them as already equal and skip the repair, silently dropping the insertion. Also clamps caretOffset to [0, text.length] defensively, and adds coverage for a non-empty-selection replace and for the fallback-to-CodeMirror-mount transition itself (using the real @codemirror/* packages, no mocks). Co-Authored-By: Claude Opus 5 (1M context) --- craft/src/ui/CodeEditor.test.tsx | 68 ++++++++++++++++++++++++++++++++ craft/src/ui/CodeEditor.tsx | 29 +++++++++++--- 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/craft/src/ui/CodeEditor.test.tsx b/craft/src/ui/CodeEditor.test.tsx index 7fa5b8d..e4134d8 100644 --- a/craft/src/ui/CodeEditor.test.tsx +++ b/craft/src/ui/CodeEditor.test.tsx @@ -122,4 +122,72 @@ describe('CodeEditor imperative handle (textarea fallback mode)', () => { }); expect(ta.selectionStart).toBe(3); }); + + test('insertAtCursor replaces a non-empty selection (not just an empty caret)', () => { + const onChange = vi.fn(); + const ref = React.createRef(); + render(); + + const ta = container.querySelector('[data-testid="code-editor-fallback"]') as HTMLTextAreaElement; + // "
old
" -- select "old" (indices 5-8). + ta.selectionStart = 5; + ta.selectionEnd = 8; + + act(() => { + ref.current!.insertAtCursor('new'); + }); + + expect(onChange).toHaveBeenCalledWith('
new
'); + }); +}); + +describe('CodeEditor imperative handle: fallback insert survives a later CodeMirror mount', () => { + // Regression test for a data-loss bug: insertAtCursor's textarea-fallback + // branch used to advance lastEmittedRef to the post-insertion value. The + // mount effect's dynamic import() closes over `value` as of the initial + // render, so if CodeMirror finishes loading *after* a fallback-mode + // insertion, it mounts with the pre-insertion doc. The only thing that + // repairs that is the value-sync effect, which is gated on `value !== + // lastEmittedRef.current` -- advancing lastEmittedRef made that gate see + // them as already equal and skip the repair, silently dropping the + // insertion. This test drives the component through that exact sequence + // using the real @codemirror/* packages (no mocks, no fake timers) to + // prove the fix holds. + test('insertAtCursor in fallback mode is not lost once CodeMirror mounts', async () => { + const handleRef = React.createRef(); + function Harness() { + const [value, setValue] = React.useState('
'); + return ; + } + render(); + + // First tick: the dynamic import() chain is always async, so this is + // still the textarea fallback (see the file-level comment above). + const ta = container.querySelector('[data-testid="code-editor-fallback"]') as HTMLTextAreaElement; + expect(ta).not.toBeNull(); + ta.selectionStart = 5; + ta.selectionEnd = 5; + act(() => { + handleRef.current!.insertAtCursor('

'); + }); + + // Let the real dynamic import() of @codemirror/* actually resolve and + // the view mount (real elapsed time, not mocked/faked). + for (let i = 0; i < 5; i += 1) { + // eslint-disable-next-line no-await-in-loop + await act(async () => { + await new Promise((resolve) => { setTimeout(resolve, 50); }); + }); + } + + // Confirm CodeMirror actually mounted (fallback textarea gone, replaced + // by the CodeMirror root) -- otherwise this assertion would trivially + // pass by reading back the fallback textarea's own value and wouldn't + // exercise the bug at all. + expect(container.querySelector('[data-testid="code-editor-fallback"]')).toBeNull(); + + // The value-sync effect must have pushed the post-insertion value into + // the freshly-mounted doc -- the insertion must not have been dropped. + expect(handleRef.current!.getValue()).toBe('

'); + }); }); diff --git a/craft/src/ui/CodeEditor.tsx b/craft/src/ui/CodeEditor.tsx index d3c16b1..71cc197 100644 --- a/craft/src/ui/CodeEditor.tsx +++ b/craft/src/ui/CodeEditor.tsx @@ -109,6 +109,15 @@ function loadCodeMirror(): Promise { return cmModulesPromise; } +// Guards against a caller passing a `caretOffset` outside [0, text.length] +// (e.g. a stale offset computed against different snippet text), which +// would otherwise let `insertAtCursor` compute a caret position past the +// text it just inserted. +function clampCaretOffset(caretOffset: number | undefined, textLength: number): number { + if (caretOffset === undefined) return textLength; + return Math.min(Math.max(caretOffset, 0), textLength); +} + function languageExtension(mods: CmModules, language: CodeEditorLanguage) { switch (language) { case 'css': @@ -246,7 +255,7 @@ export const CodeEditor = forwardRef(function const view = viewRef.current; if (view) { const { from, to } = view.state.selection.main; - const caret = from + (caretOffset ?? text.length); + const caret = from + clampCaretOffset(caretOffset, text.length); view.dispatch({ changes: { from, to, insert: text }, selection: { anchor: caret }, @@ -254,15 +263,25 @@ export const CodeEditor = forwardRef(function view.focus(); return; } - // Textarea fallback: CodeMirror never mounted (still loading, or its - // lazy chunks failed). The toolbar must keep working either way. + // Textarea fallback: CodeMirror never mounted yet (still loading, or + // its lazy chunks failed). The toolbar must keep working either way. + // + // Deliberately do NOT touch lastEmittedRef here -- same as the plain + // textarea onChange handler below, which never touches it either. + // The mount effect's dynamic import() closes over `value` at the time + // it started, so CodeMirror can finish loading with a stale doc if it + // resolves after this insertion. The only thing that catches that is + // the value-sync effect above, which is gated on `value !== + // lastEmittedRef.current`. If this insertion advanced lastEmittedRef + // to `next`, that effect would see value === lastEmittedRef.current + // once the parent re-renders and silently skip pushing the insertion + // into the freshly-mounted (stale) doc -- losing it for good. const ta = textareaRef.current; if (!ta) return; const from = ta.selectionStart ?? ta.value.length; const to = ta.selectionEnd ?? from; const next = ta.value.slice(0, from) + text + ta.value.slice(to); - const caret = from + (caretOffset ?? text.length); - lastEmittedRef.current = next; + const caret = from + clampCaretOffset(caretOffset, text.length); onChangeRef.current(next); ta.value = next; ta.selectionStart = caret;