Skip to content

Commit a6fefb7

Browse files
authored
Fix first keystroke lost after selection in Monaco editor (#106)
* Fix first keystroke lost after selection in Monaco editor * Simplify * Fix RTL selection bug
1 parent 7e77f71 commit a6fefb7

4 files changed

Lines changed: 217 additions & 29 deletions

File tree

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
# Fix: First keystroke lost after selection in Monaco editor
2+
3+
## Overview
4+
5+
Two separate bugs caused the first keystroke to be lost after making a selection in the hub-client Monaco editor:
6+
7+
1. **Intermittent (any selection direction)**: The `handleEditorChange` callback was recreated on every render, causing `@monaco-editor/react` to re-subscribe its `onDidChangeModelContent` listener via `useEffect` on every render. Keystrokes landing between paint and effect execution could be dropped.
8+
9+
2. **Deterministic (backward/RTL selections only)**: On some platforms, the browser's hidden textarea input pipeline silently drops the first character typed into a backward selection. Monaco receives the `keyDown` event but the `input` event never fires on the hidden textarea, so no model change occurs.
10+
11+
## Bug 1: Unstable onChange callback
12+
13+
### Root Cause
14+
15+
`@monaco-editor/react` v4.7.0 manages its `onDidChangeModelContent` listener inside a `useEffect([isReady, onChange])`. Because `handleEditorChange` was not memoized, it received a new function reference on every React render, causing the library to dispose and re-subscribe its listener on every render. Keystrokes landing in the window between paint and effect execution could be silently dropped.
16+
17+
A secondary issue: the `options` object passed to `<MonacoEditor>` was defined inline in JSX, creating a new reference every render, causing unnecessary `editor.updateOptions()` calls.
18+
19+
### Fix
20+
21+
- **`useAutomergeSync.ts`**: Added `currentFileRef` (a `useRef` that mirrors `currentFile`). Wrapped `handleEditorChange` in `useCallback([onContentOperations])` — stable across all renders since `onContentOperations` is itself `useCallback([], [])`.
22+
- **`Editor.tsx`**: Promoted the inline `options` to a module-level `const editorOptions` (fully static, no `useMemo` needed).
23+
24+
### Tests
25+
26+
Two regression tests in `useAutomergeSync.test.ts`:
27+
1. **Stable identity across re-renders** — asserts `handleEditorChange` is `===` across re-renders with different `fileContents`.
28+
2. **Ref picks up file switches** — asserts the stable callback routes changes to the new file path after a file switch.
29+
30+
## Bug 2: Backward selection drops first keystroke
31+
32+
### Root Cause
33+
34+
On some platforms (confirmed on macOS), when the user makes a backward (RTL) selection in Monaco and types a character, the browser's input pipeline silently drops the keystroke. Diagnosis via instrumentation confirmed:
35+
- `editor.onKeyDown` fires (`code=KeyS, hasSelection=true, selDir=RTL`)
36+
- `model.onDidChangeContent` does NOT fire
37+
- The `@monaco-editor/react` `onChange` callback is never called
38+
39+
The issue is at the browser/OS level: Monaco's hidden textarea has its selection set to represent the backward selection, and the platform's input method system fails to process the first keystroke in this state. This was confirmed by ruling out all application-level causes (selection sync, presence, reconciliation) via targeted disabling.
40+
41+
### Fix
42+
43+
**`Editor.tsx`**: Added an `editor.onKeyDown` handler in `handleEditorMount` that normalizes backward selections to forward on any printable keyDown. When a printable character key is pressed with an RTL selection active, the handler calls `editor.setSelection()` to flip the selection to LTR (same highlighted range, cursor moves to end). This allows the browser's input pipeline to process the character correctly.
44+
45+
```typescript
46+
editor.onKeyDown((e) => {
47+
const sel = editor.getSelection();
48+
if (!sel || sel.isEmpty() || sel.getDirection() === 0) return;
49+
const key = e.browserEvent.key;
50+
if (!key || key.length !== 1) return;
51+
editor.setSelection({
52+
selectionStartLineNumber: sel.startLineNumber,
53+
selectionStartColumn: sel.startColumn,
54+
positionLineNumber: sel.endLineNumber,
55+
positionColumn: sel.endColumn,
56+
});
57+
});
58+
```
59+
60+
## Analysis
61+
62+
### What was ruled out
63+
64+
- **Automerge sync path**: Fully synchronous. `getFileContent()` always matches `model.getValue()` for local edits.
65+
- **Reconciliation effect**: Always a no-op for local edits (reads live Automerge state, not stale closure).
66+
- **Selection sync (`useSelectionSync`)**: Disabled entirely during debugging — backward selection bug persisted.
67+
- **Presence hook**: `onDidChangeCursorSelection` handler only sends presence data, doesn't modify editor.
68+
- **`@monaco-editor/react` controlled mode**: We use `defaultValue` (uncontrolled), library's value-sync code is inert.
69+
- **Preview iframe stealing focus**: `editor.focus()` after `preview.setSelection()` didn't fix the issue.
70+
- **MorphIframe `selectionchange` feedback loop**: Suppressing programmatic `selectionchange` events didn't fix the issue.
71+
72+
### Diagnostic approach for Bug 2
73+
74+
Instrumentation was added at three levels:
75+
1. **Sync layer** (`handleEditorChange`): logged all calls including dropped ones (replay/remote guards). Result: no log at all for backward selections → callback never invoked.
76+
2. **Monaco events** (`editor.onKeyDown`, `model.onDidChangeContent`): `keyDown` fired but `modelChange` did not → keystroke received by Monaco but never applied to model.
77+
3. **Selection sync disabled**: Bug persisted → not caused by selection sync.
78+
79+
This narrowed the cause to the browser's input pipeline between Monaco's `keyDown` handler and the hidden textarea's `input` event.
80+
81+
### Key files
82+
83+
| File | Role |
84+
|------|------|
85+
| `hub-client/src/hooks/useAutomergeSync.ts` | Bidirectional Automerge ↔ Monaco sync |
86+
| `hub-client/src/hooks/useAutomergeSync.test.ts` | Regression tests for callback stability |
87+
| `hub-client/src/components/Editor.tsx` | Main editor component, RTL selection workaround |
88+
89+
## Changes
90+
91+
- [x] Diagnose and fix intermittent keystroke loss (unstable onChange callback)
92+
- [x] Diagnose and fix deterministic backward selection keystroke loss (RTL normalization)
93+
- [x] Add regression tests for callback stability
94+
- [x] Simplification pass (comment trimming, useMemo → module constant)
95+
- [x] Remove debug instrumentation
96+
- [x] Revert unsuccessful fixes (MorphIframe settingSelectionRef, editor.focus in useSelectionSync)
97+
- [x] Verify TypeScript compiles cleanly
98+
- [x] Verify all 398 hub-client tests pass

hub-client/src/components/Editor.tsx

Lines changed: 47 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,31 @@ function getLanguageForFile(filePath: string): string {
8787
}
8888
}
8989

90+
// Stable options object — defined at module level so @monaco-editor/react
91+
// never sees a new reference and skips its internal updateOptions() effect.
92+
const editorOptions = {
93+
minimap: { enabled: false },
94+
fontSize: 14,
95+
lineNumbers: 'on' as const,
96+
wordWrap: 'on' as const,
97+
padding: { top: 16 },
98+
scrollBeyondLastLine: false,
99+
// Disable paste-as to prevent snippet expansion (e.g., URLs from browser
100+
// address bar being pasted with $0 appended). See quarto-dev/kyoto#3.
101+
pasteAs: { enabled: false },
102+
fixedOverflowWidgets: true,
103+
// Prefer showing hover below the line — prevents diagnostic popups near
104+
// the top of the editor from overlapping the navbar.
105+
hover: { above: false },
106+
quickSuggestions: false,
107+
suggestOnTriggerCharacters: false,
108+
wordBasedSuggestions: 'off' as const,
109+
acceptSuggestionOnEnter: 'off' as const,
110+
acceptSuggestionOnCommitCharacter: false,
111+
suggest: { showWords: false, showSnippets: false },
112+
inlineSuggest: { enabled: false },
113+
};
114+
90115
// Select the best default file: prefer index.qmd, then first .qmd, then first file
91116
function selectDefaultFile(files: FileEntry[]): FileEntry | null {
92117
if (files.length === 0) return null;
@@ -524,6 +549,27 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC
524549
editorHasFocusRef.current = false;
525550
});
526551

552+
// Workaround: normalize backward (RTL) selections to forward (LTR) on
553+
// any printable keyDown. On some platforms the browser's input pipeline
554+
// silently drops the first character typed into a backward selection
555+
// because the hidden textarea's selection state confuses the OS input
556+
// method. Flipping to LTR keeps the same highlighted range but places
557+
// the cursor at the end, which the input method handles reliably.
558+
editor.onKeyDown((e) => {
559+
const sel = editor.getSelection();
560+
if (!sel || sel.isEmpty() || sel.getDirection() === 0) return; // LTR or no selection
561+
// Only intervene for printable characters (single char from e.browserEvent.key)
562+
const key = e.browserEvent.key;
563+
if (!key || key.length !== 1) return;
564+
// Flip to LTR (same range, cursor moves to end)
565+
editor.setSelection({
566+
selectionStartLineNumber: sel.startLineNumber,
567+
selectionStartColumn: sel.startColumn,
568+
positionLineNumber: sel.endLineNumber,
569+
positionColumn: sel.endColumn,
570+
});
571+
});
572+
527573
// Track cursor position changes for slide navigation
528574
editor.onDidChangeCursorPosition((e) => {
529575
// Get the cursor line (0-based in Monaco)
@@ -956,31 +1002,7 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC
9561002
beforeMount={handleBeforeMount}
9571003
onChange={handleEditorChange}
9581004
onMount={handleEditorMount}
959-
options={{
960-
minimap: { enabled: false },
961-
fontSize: 14,
962-
lineNumbers: 'on',
963-
wordWrap: 'on',
964-
padding: { top: 16 },
965-
scrollBeyondLastLine: false,
966-
// Disable paste-as to prevent snippet expansion (e.g., URLs from browser
967-
// address bar being pasted with $0 appended). See quarto-dev/kyoto#3.
968-
pasteAs: { enabled: false },
969-
// Move hover/diagnostic widgets to a fixed container outside the editor's
970-
// overflow:hidden boundary, preventing them from being clipped by the navbar.
971-
fixedOverflowWidgets: true,
972-
// Prefer showing hover below the line. This prevents diagnostic popups near
973-
// the top of the editor from overlapping the navbar.
974-
hover: { above: false },
975-
// Disable aggressive autocomplete/suggestions
976-
quickSuggestions: false,
977-
suggestOnTriggerCharacters: false,
978-
wordBasedSuggestions: 'off',
979-
acceptSuggestionOnEnter: 'off',
980-
acceptSuggestionOnCommitCharacter: false,
981-
suggest: { showWords: false, showSnippets: false },
982-
inlineSuggest: { enabled: false },
983-
}}
1005+
options={editorOptions}
9841006
/>
9851007
</div>
9861008
</div>

hub-client/src/hooks/useAutomergeSync.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,66 @@ describe('useAutomergeSync', () => {
315315

316316
expect(onContentOperations).not.toHaveBeenCalled();
317317
});
318+
319+
it('should maintain stable identity across re-renders (prevents listener churn)', () => {
320+
// Regression: unstable handleEditorChange caused @monaco-editor/react to
321+
// dispose and re-subscribe its onDidChangeModelContent listener on every
322+
// render, which could race with keystrokes and drop the first character
323+
// typed after a selection.
324+
const onContentOperations = vi.fn();
325+
const { result, rerender } = renderHook(
326+
(props) => useAutomergeSync(props),
327+
{ initialProps: defaultOptions({ onContentOperations }) }
328+
);
329+
330+
const first = result.current.handleEditorChange;
331+
332+
// Re-render with a different fileContents map (simulates remote edit to
333+
// another file, presence state update, or any other unrelated state change).
334+
rerender(defaultOptions({
335+
onContentOperations,
336+
fileContents: new Map([['test.qmd', '# Hello'], ['other.qmd', '# Other']]),
337+
}));
338+
339+
expect(result.current.handleEditorChange).toBe(first);
340+
341+
// Re-render again with yet another map.
342+
rerender(defaultOptions({
343+
onContentOperations,
344+
fileContents: new Map([['test.qmd', '# Hello changed']]),
345+
}));
346+
347+
expect(result.current.handleEditorChange).toBe(first);
348+
});
349+
350+
it('should read latest currentFile via ref after re-render', () => {
351+
// Ensures the stable callback picks up file switches through the ref,
352+
// not through a stale closure.
353+
const onContentOperations = vi.fn();
354+
const file1: FileEntry = { path: 'file1.qmd' };
355+
const file2: FileEntry = { path: 'file2.qmd' };
356+
357+
const { result, rerender } = renderHook(
358+
(props) => useAutomergeSync(props),
359+
{ initialProps: defaultOptions({ currentFile: file1, onContentOperations }) }
360+
);
361+
362+
const stableCallback = result.current.handleEditorChange;
363+
364+
// Switch to a different file
365+
rerender(defaultOptions({ currentFile: file2, onContentOperations }));
366+
367+
// Identity must not change
368+
expect(result.current.handleEditorChange).toBe(stableCallback);
369+
370+
// But the callback must use the NEW file path
371+
const mockEvent = { changes: [{ rangeOffset: 0, rangeLength: 0, text: 'x' }] };
372+
act(() => {
373+
result.current.handleEditorChange('x', mockEvent as never);
374+
});
375+
376+
expect(onContentOperations).toHaveBeenCalledWith('file2.qmd', mockEvent.changes);
377+
});
318378
});
319379

320380
describe('handleContentRewrite', () => {

hub-client/src/hooks/useAutomergeSync.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,11 @@ export function useAutomergeSync({
7272
const applyingRemoteRef = useRef(false);
7373
const editorRef = useRef<Monaco.editor.IStandaloneCodeEditor | null>(null);
7474

75+
// Keep a ref to currentFile so the stable handleEditorChange callback
76+
// always reads the latest value without needing it as a dependency.
77+
const currentFileRef = useRef(currentFile);
78+
currentFileRef.current = currentFile;
79+
7580
const onEditorMount = useCallback((editor: Monaco.editor.IStandaloneCodeEditor) => {
7681
editorRef.current = editor;
7782
}, []);
@@ -144,15 +149,18 @@ export function useAutomergeSync({
144149
}, [currentFile, fileContents, replayIsActive]);
145150

146151
// ── Monaco → Automerge ───────────────────────────────────────────────
147-
const handleEditorChange = (value: string | undefined, event: Monaco.editor.IModelContentChangedEvent) => {
152+
//
153+
// Stable callback: uses refs so the identity never changes, preventing
154+
// @monaco-editor/react from re-subscribing its listener every render.
155+
const handleEditorChange = useCallback((value: string | undefined, event: Monaco.editor.IModelContentChangedEvent) => {
148156
if (replayActiveRef.current) return;
149157
if (applyingRemoteRef.current) return;
150158

151-
if (value !== undefined && currentFile) {
159+
if (value !== undefined && currentFileRef.current) {
152160
setContent(value);
153-
onContentOperations(currentFile.path, event.changes);
161+
onContentOperations(currentFileRef.current.path, event.changes);
154162
}
155-
};
163+
}, [onContentOperations]);
156164

157165
// ── AST rewrite (through Monaco → onChange → splice path) ────────────
158166
const handleContentRewrite = useCallback((newContent: string) => {

0 commit comments

Comments
 (0)