From 550b556a82e07cd0f9eb49efbf4214dcb5b698c0 Mon Sep 17 00:00:00 2001 From: shikokuchuo <53399081+shikokuchuo@users.noreply.github.com> Date: Fri, 27 Mar 2026 10:45:52 -0700 Subject: [PATCH 1/2] fix(hub-client): replace `updateText` with positional splice for CRDT sync Eliminates race condition where concurrent remote edits were lost because `updateText` diffed against stale Automerge state. Monaco's `IModelContentChange` events now map directly to Automerge splice operations --- .../plans/2026-03-26-operation-based-sync.md | 260 +++++++++++++++ crates/quarto-hub/Cargo.toml | 5 +- crates/quarto-hub/src/automerge_api_tests.rs | 144 ++++++++ hub-client/src/App.tsx | 16 +- hub-client/src/components/Editor.tsx | 50 +-- .../src/components/render/PreviewRouter.tsx | 6 +- .../src/components/render/ReactPreview.tsx | 8 +- hub-client/src/services/automergeSync.ts | 13 +- hub-client/src/test-utils/mockSyncClient.ts | 18 + .../src/applyEditorOperations.test.ts | 315 ++++++++++++++++++ ts-packages/quarto-sync-client/src/client.ts | 29 +- ts-packages/quarto-sync-client/src/index.ts | 1 + ts-packages/quarto-sync-client/src/types.ts | 20 ++ 13 files changed, 842 insertions(+), 43 deletions(-) create mode 100644 claude-notes/plans/2026-03-26-operation-based-sync.md create mode 100644 ts-packages/quarto-sync-client/src/applyEditorOperations.test.ts diff --git a/claude-notes/plans/2026-03-26-operation-based-sync.md b/claude-notes/plans/2026-03-26-operation-based-sync.md new file mode 100644 index 00000000..9a01b42c --- /dev/null +++ b/claude-notes/plans/2026-03-26-operation-based-sync.md @@ -0,0 +1,260 @@ +# Operation-Based Sync: Fix Concurrent Edit Race Condition + +**Issue:** quarto-dev/q2#74 — hub: edits can get lost +**Related PR:** #80 (partial fix, merged 2026-03-25) + +## Overview + +The hub-client uses `updateText()` to sync editor content to Automerge. `updateText` computes a full-text diff between the current Automerge document state and the content string passed to it. When remote edits merge into the Automerge document between the time Monaco's content is read and the time `updateText` executes, the diff deletes the remote content — because Monaco doesn't have it yet. + +**Fix:** Replace full-text `updateText()` with positional `splice()` operations derived from Monaco's `IModelContentChange` events. Each keystroke/edit produces exact `{rangeOffset, rangeLength, text}` triples that map directly to Automerge `splice(doc, ['text'], offset, deleteCount, insertText)`. Since Automerge's CRDT handles concurrent positional splices natively, no full-text diff is needed and the race window is eliminated. + +## Architecture + +### Current flow (broken under concurrency) + +``` +User types → Monaco onChange(value, event) + → handleEditorChange(value) + → onContentChange(path, value) // full content string + → updateFileContent(path, value) // in client.ts + → handle.change(doc => updateText(doc, ['text'], value)) + // ↑ full-text diff against + // current Automerge state +``` + +### New flow (operation-based) + +``` +Any local edit (typing, drag-drop, paste, find-replace, undo/redo) + → Monaco onChange(value, event) + → handleEditorChange(value, event) + → setContent(value) // React state for preview + → onContentOperations(path, event.changes) // NEW: positional ops + → applyEditorOperations(path, changes) // in client.ts + → handle.change(doc => { + for (change of changes) { + splice(doc, ['text'], change.rangeOffset, + change.rangeLength, change.text) + } + }) + +ReactPreview AST rewrite (e.g., slide reorder) + → setContent(newQmd) // prop = handleContentRewrite + → editorRef.executeEdits('ast-rewrite', fullRangeEdit) + → Monaco onChange(value, event) // fires synchronously + → (same splice path as above) +``` + +All local edits — typed, pasted, drag-dropped, find-replaced, or AST-rewritten — flow through the same `onChange` → splice path. The explicit `onContentChange` callback and `updateFileContent` full-text path are removed from Editor entirely. `updateFileContent` remains only for replay mode (`useReplayMode.ts:159`), which operates outside the Editor component. + +### Key design decisions + +1. **Single unified path for all local edits** — Every local edit (including drag-drop `executeEdits`) triggers Monaco's `onChange`, which fires `handleEditorChange`. This calls `setContent(value)` for React state and `onContentOperations(path, event.changes)` for the CRDT. No edit needs a separate sync call. + +2. **Remove `onContentChange` prop from Editor** — The old `onContentChange` callback carried full content strings for `updateText`. With all local edits routed through splice, this prop is no longer needed. Drag-drop code (lines 765, 861) no longer explicitly calls `onContentChange` after `executeEdits` — Monaco's `onChange` handles it. + +3. **Remove `handleContentChange` from App.tsx** — With `onContentChange` gone, `handleContentChange` (which called `updateFileContent`) is removed. `handleContentOperations` (which calls `applyEditorOperations`) replaces it. + +4. **Route ReactPreview AST writes through Monaco** — `ReactPreview.tsx:212` calls `setContent(newQmd)` after AST modifications via `incrementalWriteQmd`. Rather than calling `updateFileContent` (which has the same `updateText` race condition), we route through Monaco: `handleContentRewrite` applies the new content to the Monaco editor via `executeEdits('ast-rewrite', ...)`, which fires `onChange`, which flows through the splice path. This: + - **Eliminates the race**: Monaco's model is always up-to-date with remote edits, so the splice is diffed against the correct base state. + - **Eliminates a wiring gap**: No need to pipe `updateFileContent` into Editor — `handleContentRewrite` only needs `editorRef`, which Editor already has. + - **Preserves undo**: `executeEdits` pushes onto Monaco's undo stack, so Ctrl+Z reverses an AST rewrite. + - **Unifies all mutation paths**: Every content change flows through Monaco → `onChange` → splice. + +5. **Keep `updateFileContent` only for replay mode** — `useReplayMode.ts:159` calls `updateFileContent` directly to set replay snapshots. This operates outside the Editor component and doesn't go through Monaco's `onChange`. It is the sole remaining consumer of `updateFileContent` from the Editor's perspective. + +6. **Changes are applied in a single `handle.change()` call** — Monaco batches changes (e.g., multi-cursor, find-replace). All changes from one event go into one Automerge transaction. + +7. **Changes are ordered end-to-beginning** — Monaco documents: "The changes are ordered from the end of the document to the beginning, so they should be safe to apply in sequence." For `splice`, we need to apply them in this same order (end-first) so earlier offsets remain valid. + +## Work Items + +### Phase 0: UTF-16 encoding alignment (DONE) + +Both the JS/WASM client and the Rust server must agree on text index encoding. Monaco and the Automerge WASM build both use UTF-16 code units (JavaScript's native string encoding). The Rust server previously defaulted to `UnicodeCodePoint`, which disagrees for non-BMP characters (emoji etc.) — each emoji is 1 code point but 2 UTF-16 code units. + +- [x] **0.1** Enable `utf16-indexing` feature on `automerge` dependency in `quarto-hub/Cargo.toml` +- [x] **0.2** Add `test_text_encoding_is_utf16` — assert `TextEncoding::platform_default() == Utf16CodeUnit` +- [x] **0.3** Add `test_splice_text_with_emoji` — verify `splice_text` offsets are correct after a non-BMP character (🎉 = 2 UTF-16 code units) +- [x] **0.4** Add `test_splice_text_delete_emoji` — verify deleting an emoji requires `deleteCount=2` +- [x] **0.5** Add `test_splice_text_replace_after_multiple_emoji` — verify offsets accumulate correctly with 3 consecutive emoji +- [x] **0.6** Add `test_update_text_preserves_emoji_in_concurrent_edits` — concurrent splices around emoji survive merge +- [x] **0.7** Add `test_splice_text_with_mixed_bmp_and_non_bmp` — mix of CJK (1 code unit) and emoji (2 code units) +- [x] **0.8** Verify all 172 existing `quarto-hub` tests still pass with the new feature flag + +### Phase 1: Tests (TDD — write tests before implementation) + +- [x] **1.1** Add `EditorContentChange` type to `types.ts` (mirrors Monaco's `IModelContentChange` shape: `{ rangeOffset: number; rangeLength: number; text: string }`) +- [x] **1.2** Write unit test for `applyEditorOperations` in client — single insert, single delete, replace, multi-change batch, multi-cursor (N changes at arbitrary positions verifying end-to-beginning ordering) (test will not compile yet — that's expected) +- [ ] **1.3** Write integration test simulating concurrent edits: local splice + remote merge should not lose remote content (deferred: requires real Automerge; covered by Rust-side tests in Phase 0) +- [x] **1.4** Write test with non-BMP characters (emoji) in `applyEditorOperations` — verify UTF-16 offsets from Monaco map correctly through Automerge `splice` +- [x] **1.5** Write test that `applyEditorOperations` with an empty changes array is a no-op (no Automerge transaction created) + +### Phase 2: Add `splice` import and new client API + +- [x] **2.1** Add `splice` import to `client.ts` (from `@automerge/automerge-repo` — verified exported) +- [x] **2.2** Add `applyEditorOperations(path, changes)` function to `client.ts` that applies positional splice operations +- [x] **2.3** Export `applyEditorOperations` from the sync client's public API (add to the returned object from `createSyncClient`) +- [x] **2.4** Export `applyEditorOperations` from `automergeSync.ts` service layer +- [x] **2.5** Run Phase 1 tests — verify they pass (all 9 tests pass) + +### Phase 3: Wire Editor to use operations and remove old path + +- [x] **3.1** Add `onContentOperations` prop to Editor component (`(path: string, changes: EditorContentChange[]) => void`) +- [x] **3.2** Update `handleEditorChange` in Editor.tsx to accept the `IModelContentChangedEvent` (second arg from Monaco's `onChange`) and call `onContentOperations` with `event.changes` +- [x] **3.3** Remove `onContentChange` prop from Editor component entirely +- [x] **3.4** Remove the post-`executeEdits` sync blocks from drag-drop code (lines ~760–766 and ~857–862): remove the `getValue()`, `setContent()`, and `onContentChange()` calls. Also remove `onContentChange` from the `useCallback` dependency arrays at lines ~796 and ~869. Monaco's `onChange` now handles both React state and CRDT sync via the splice path. +- [x] **3.5** Route ReactPreview AST writes through Monaco: introduce `handleContentRewrite` in Editor that applies new content via `diffToMonacoEdits` → `executeEdits('ast-rewrite', ...)`. This fires `onChange`, which flows through the splice path — no direct access to `updateFileContent` needed. Pass `handleContentRewrite` as the `onContentRewrite` prop to `PreviewRouter` instead of passing `handleEditorChange` as `setContent`. +- [x] **3.6** Add `handleContentOperations` callback in App.tsx that calls `applyEditorOperations` +- [x] **3.7** Remove `handleContentChange` and `onContentChange` prop from App.tsx — no longer needed +- [x] **3.8** Pass `onContentOperations={handleContentOperations}` to Editor +- [x] **3.9** Verify replay mode (`useReplayMode.ts:159`) still works — it calls `updateFileContent` directly, which remains available (sole remaining consumer outside Editor) +- [ ] **3.10** Verify ReactPreview AST writes (`ReactPreview.tsx:212`) still work — `onContentRewrite` now routes through `handleContentRewrite` → `executeEdits` → `onChange` → splice. Test that undo (Ctrl+Z) reverses an AST rewrite. (manual testing required) + +### Phase 4: Verification + +- [x] **4.1** `cargo build --workspace` passes +- [x] **4.2** `cargo nextest run --workspace` passes (6893 passed) +- [x] **4.3** `cd hub-client && npm run build` passes +- [x] **4.4** `cd hub-client && npm run test:ci` — unit tests (389 passed), integration (12 passed), WASM (48 passed, 4 failed pre-existing in formatDetection.wasm.test.ts — unrelated) +- [ ] **4.5** Manual testing: open two browser tabs, type rapidly in both, verify no content loss +- [ ] **4.6** Manual testing: drag-drop a file into the editor, verify content is inserted correctly +- [ ] **4.7** Manual testing: ReactPreview AST rewrite (e.g., drag-reorder slides) while another tab is typing — verify no content loss +- [ ] **4.8** Manual testing: after a ReactPreview AST rewrite, Ctrl+Z undoes the change + +## File Change Summary + +| File | Change | +|------|--------| +| `crates/quarto-hub/Cargo.toml` | Enable `utf16-indexing` feature on automerge dependency **(DONE)** | +| `crates/quarto-hub/src/automerge_api_tests.rs` | Add UTF-16 encoding verification and splice tests with emoji **(DONE)** | +| `ts-packages/quarto-sync-client/src/types.ts` | Add `EditorContentChange` type | +| `ts-packages/quarto-sync-client/src/client.ts` | Add `splice` import, add `applyEditorOperations` function, export it from return object | +| `ts-packages/quarto-sync-client/src/index.ts` | Export `EditorContentChange` type and `applyEditorOperations` | +| `hub-client/src/services/automergeSync.ts` | Add `applyEditorOperations` wrapper, export `EditorContentChange` type | +| `hub-client/src/components/Editor.tsx` | Replace `onContentChange` prop with `onContentOperations`, import `EditorContentChange` type, update `handleEditorChange` to accept `IModelContentChangedEvent` second arg, remove post-`executeEdits` sync blocks from drag-drop, add `handleContentRewrite` that routes through `executeEdits` → splice for PreviewRouter's `onContentRewrite` | +| `hub-client/src/App.tsx` | Add `handleContentOperations` callback (imports `applyEditorOperations`), remove `handleContentChange` and `updateFileContent` import, replace `onContentChange` prop with `onContentOperations` | +| `hub-client/src/test-utils/mockSyncClient.ts` | Add `applyEditorOperations` mock | +| `hub-client/src/services/automergeSync.test.ts` | Add tests for `applyEditorOperations` wrapper (if applicable) | + +## Detailed Implementation Notes + +### `applyEditorOperations` in client.ts + +```typescript +function applyEditorOperations(path: string, changes: EditorContentChange[]): void { + if (changes.length === 0) return; // no-op guard (edge case 9) + + const handle = state.fileHandles.get(path); + if (!handle) { + console.warn(`No handle found for file: ${path}`); + return; + } + + handle.change(doc => { + // Monaco orders changes end-to-beginning, so earlier offsets stay valid. + for (const change of changes) { + splice(doc, ['text'], change.rangeOffset, change.rangeLength, change.text); + } + }); +} +``` + +### Monaco `onChange` signature + +The `@monaco-editor/react` `onChange` prop already provides the event: + +```typescript +type OnChange = (value: string | undefined, ev: editor.IModelContentChangedEvent) => void; +``` + +So `handleEditorChange` just needs to accept the second argument: + +```typescript +const handleEditorChange = (value: string | undefined, event: Monaco.editor.IModelContentChangedEvent) => { + if (replayActiveRef.current) return; + if (applyingRemoteRef.current) return; + + if (value !== undefined && currentFile) { + setContent(value); // React state for preview + onContentOperations(currentFile.path, event.changes); // CRDT operations + } +}; +``` + +### Why drag-drop needs no special handling + +Drag-drop calls `executeEdits()`, which modifies the Monaco model synchronously. This fires Monaco's `onChange` synchronously during `executeEdits`, which triggers `handleEditorChange` → `setContent` + `onContentOperations`. By the time `executeEdits` returns, both React state and the CRDT are already updated. The old explicit post-`executeEdits` sync code (`getValue()` → `setContent()` → `onContentChange()`) is therefore redundant and removed. + +### Why `updateFileContent` still exists + +Only one consumer remains after the refactor: + +**Replay mode** (`useReplayMode.ts:159`) calls `updateFileContent` directly to set file content from replay snapshots. This operates outside the Editor component and doesn't go through Monaco's `onChange`. It is the sole remaining consumer. + +ReactPreview AST writes no longer use `updateFileContent` — they route through Monaco (see below). + +### `handleContentRewrite` in Editor.tsx + +ReactPreview AST writes (e.g., drag-reordering slides) produce a new QMD string via `incrementalWriteQmd`. Rather than calling `updateFileContent` directly (which has the same `updateText` race condition as the original bug), the new `handleContentRewrite` routes through Monaco: + +```typescript +// Route AST rewrites through Monaco → onChange → splice path. +// Uses diffToMonacoEdits (already in the codebase) to compute minimal edits, +// so concurrent remote edits in unchanged regions merge cleanly via CRDT. +// Also preserves undo history (executeEdits pushes onto the undo stack). +const handleContentRewrite = useCallback((newContent: string) => { + if (!editorRef.current || !currentFile) return; + const model = editorRef.current.getModel(); + if (!model) return; + + const oldContent = model.getValue(); + const edits = diffToMonacoEdits(oldContent, newContent); + if (edits.length > 0) { + editorRef.current.executeEdits('ast-rewrite', edits); + } + // onChange fires synchronously → handleEditorChange → setContent + onContentOperations + // Each edit becomes a targeted splice, not a full-document replacement. +}, [currentFile]); +``` + +This replaces the previous `setContent={handleEditorChange}` prop on `PreviewRouter` (line 1086) with `onContentRewrite={handleContentRewrite}`. Rename the prop from `setContent` to `onContentRewrite` in the `PreviewRouter` → `ReactPreview` chain to avoid confusion with the React state setter `setContent` used in `handleEditorChange`. The signature remains `(content: string) => void`. + +**Why this is better than `updateFileContent`:** + +1. **No race condition** — Monaco's model is always synchronized with remote edits (applied via `executeEdits('remote-sync', ...)`). The `onChange` event diffs against this up-to-date state, not against potentially-stale Automerge document state. +2. **No wiring gap** — `handleContentRewrite` only needs `editorRef` (already available in Editor), not `updateFileContent` (which lives in `client.ts` and would require a new prop). +3. **Undo support** — `executeEdits` pushes onto Monaco's undo stack. Users can Ctrl+Z to reverse an AST rewrite (e.g., undo a slide reorder). +4. **Single mutation path** — All content changes (typing, drag-drop, AST rewrites) flow through Monaco → `onChange` → splice. +5. **Fine-grained CRDT merges** — `diffToMonacoEdits` (already used in Editor.tsx for remote sync) computes minimal edits via `fast-diff`. Monaco's `onChange` then produces targeted changes that become targeted Automerge splices. Concurrent remote edits in unchanged regions merge cleanly, unlike a full-document `splice(0, fullLen, newContent)` which would displace them. + +### Edge cases + +1. **Undo/redo**: Monaco fires `onChange` for undo/redo with the same `IModelContentChange` format. These map naturally to splice operations. No special handling needed. + +2. **Find-replace**: Monaco batches all replacements into a single `onChange` event with multiple changes. The batch goes into one `handle.change()` transaction. The end-to-beginning ordering ensures correctness. + +3. **Paste**: Large pastes are a single change with `rangeLength` (selection replaced) and `text` (pasted content). Maps directly to a single splice. + +4. **Remote edits arriving during local edit**: The Automerge CRDT handles concurrent splice operations correctly — that's its core design. Two users inserting at the same position will both see both insertions (in a deterministic order). No content is lost. + +5. **`applyingRemoteRef` guard**: When remote edits are applied via `executeEdits('remote-sync', ...)`, Monaco fires `onChange`. The `applyingRemoteRef` check in `handleEditorChange` prevents these from being sent back to Automerge as local operations. This guard already exists and works correctly. + +6. **Drag-drop via `executeEdits`**: `executeEdits` modifies the Monaco model synchronously, which fires `onChange` synchronously. The splice path in `handleEditorChange` handles both React state and CRDT sync. No explicit post-`executeEdits` sync is needed. This unifies all local edits through one code path. + +7. **ReactPreview AST writes**: `ReactPreview` calls `setContent(newQmd)` after converting a modified AST back to QMD text. This routes through `handleContentRewrite`, which uses `diffToMonacoEdits` (already in the codebase) to compute minimal edits, then applies them via `executeEdits('ast-rewrite', ...)` → Monaco `onChange` → splice path. Because the edits are fine-grained (not a full-document replacement), concurrent remote edits in unchanged regions merge cleanly at the CRDT level. The `applyingRemoteRef` guard is `false` during AST writes (they are local edits), so the splice operations are correctly sent to Automerge. + +8. **Non-BMP characters (emoji, supplementary CJK, etc.)**: Monaco's `rangeOffset`/`rangeLength` count UTF-16 code units (JavaScript's native string encoding). Automerge's `splice` must use the same encoding. The WASM build of Automerge enables `utf16-indexing` by default. The Rust server now also enables `utf16-indexing` via Cargo feature flag (Phase 0). Both sides agree: emoji like 🎉 (U+1F389) count as 2 units, BMP characters count as 1. This is verified by tests in `automerge_api_tests.rs`. + +9. **Empty changes array**: Monaco could theoretically fire `onChange` with an empty changes array. Guard with `if (changes.length === 0) return;` in `applyEditorOperations` to avoid a no-op Automerge transaction. + +## Risk Assessment + +**Low risk:** The core change is small — we're replacing one Automerge API (`updateText`, full-text diff) with another (`splice`, positional). Both are first-class Automerge APIs. The rest is plumbing. + +**Simplification benefit:** By routing all local edits (including drag-drop) through the splice path, we *reduce* the API surface — `onContentChange` prop and `handleContentChange` are removed entirely. Fewer code paths means fewer places for bugs. + +**Regression risk:** Replay mode continues using `updateFileContent`/`updateText`, which is low risk (it operates outside the Editor and isn't a concurrent-typing scenario). ReactPreview AST writes now route through Monaco → `diffToMonacoEdits` → fine-grained `executeEdits` → splice, eliminating the residual `updateText` race, gaining undo support, and preserving fine-grained CRDT merges for concurrent edits. + +**Correctness depends on:** Monaco's documented guarantee that `changes` are ordered end-to-beginning and that `rangeOffset`/`rangeLength` are relative to the document state *before* any changes in the batch are applied. This is well-documented behavior. diff --git a/crates/quarto-hub/Cargo.toml b/crates/quarto-hub/Cargo.toml index 169eb35f..9e410822 100644 --- a/crates/quarto-hub/Cargo.toml +++ b/crates/quarto-hub/Cargo.toml @@ -42,7 +42,10 @@ cookie = "0.18" time = "0.3" # Automerge (via samod for JS compatibility) -automerge = "0.7" +# utf16-indexing: use UTF-16 code units for text indices, matching the JS/WASM client. +# Without this, the Rust default is UnicodeCodePoint, which disagrees with JS on +# non-BMP characters (emoji etc.) — each emoji is 1 code point but 2 UTF-16 code units. +automerge = { version = "0.7", features = ["utf16-indexing"] } samod = { git = "https://github.com/quarto-dev/samod.git", branch = "q2", features = ["tokio", "axum", "tungstenite"] } # Async streams (for samod event handling) diff --git a/crates/quarto-hub/src/automerge_api_tests.rs b/crates/quarto-hub/src/automerge_api_tests.rs index 0e9bf414..553b66c1 100644 --- a/crates/quarto-hub/src/automerge_api_tests.rs +++ b/crates/quarto-hub/src/automerge_api_tests.rs @@ -407,4 +407,148 @@ mod tests { // Result should have filesystem's changes assert_eq!(read_text(&doc), "Modified by filesystem"); } + + // ========================================================================= + // UTF-16 encoding verification + // + // The JS/WASM client uses UTF-16 code units for text indices (JavaScript's + // native string encoding). The Rust server must use the same encoding so + // that positional splice operations produce consistent CRDT element IDs. + // These tests verify that the `utf16-indexing` feature flag is active and + // that splice_text works correctly with non-BMP characters (emoji, etc.). + // ========================================================================= + + #[test] + fn test_text_encoding_is_utf16() { + // Verify the feature flag is active: platform_default() should be Utf16CodeUnit + assert_eq!( + automerge::TextEncoding::platform_default(), + automerge::TextEncoding::Utf16CodeUnit, + "automerge must be compiled with utf16-indexing feature for JS client compatibility" + ); + } + + #[test] + fn test_splice_text_with_emoji() { + // 🎉 (U+1F389) is a non-BMP character: + // - 1 Unicode code point + // - 2 UTF-16 code units (surrogate pair: 0xD83C 0xDF89) + // - 4 UTF-8 bytes + // If encoding is wrong, offsets after emoji will be off. + let mut doc = create_doc_with_text("Hello 🎉 world"); + let (_, text_obj) = doc.get(ROOT, "text").unwrap().unwrap(); + + // "Hello 🎉 world" + // UTF-16 offsets: H=0 e=1 l=2 l=3 o=4 ' '=5 🎉=6,7 ' '=8 w=9 o=10 r=11 l=12 d=13 + // length in UTF-16 code units = 14 + + // Verify length uses UTF-16 code units + assert_eq!(doc.length(&text_obj), 14); + + // splice_text at UTF-16 offset 8 (the space after 🎉) to insert "!" + doc.transact::<_, _, automerge::AutomergeError>(|tx| { + tx.splice_text(&text_obj, 8, 0, "!")?; + Ok(()) + }) + .unwrap(); + + assert_eq!(read_text(&doc), "Hello 🎉! world"); + } + + #[test] + fn test_splice_text_delete_emoji() { + // Deleting an emoji requires deleteCount=2 in UTF-16 + let mut doc = create_doc_with_text("A🎉B"); + let (_, text_obj) = doc.get(ROOT, "text").unwrap().unwrap(); + + // UTF-16 offsets: A=0 🎉=1,2 B=3 → length=4 + assert_eq!(doc.length(&text_obj), 4); + + // Delete the emoji (2 UTF-16 code units starting at offset 1) + doc.transact::<_, _, automerge::AutomergeError>(|tx| { + tx.splice_text(&text_obj, 1, 2, "")?; + Ok(()) + }) + .unwrap(); + + assert_eq!(read_text(&doc), "AB"); + } + + #[test] + fn test_splice_text_replace_after_multiple_emoji() { + // Multiple emoji shift offsets more dramatically + let mut doc = create_doc_with_text("🌍🎉🚀end"); + let (_, text_obj) = doc.get(ROOT, "text").unwrap().unwrap(); + + // UTF-16 offsets: 🌍=0,1 🎉=2,3 🚀=4,5 e=6 n=7 d=8 → length=9 + assert_eq!(doc.length(&text_obj), 9); + + // Replace "end" (3 chars at offset 6) with "fin" + doc.transact::<_, _, automerge::AutomergeError>(|tx| { + tx.splice_text(&text_obj, 6, 3, "fin")?; + Ok(()) + }) + .unwrap(); + + assert_eq!(read_text(&doc), "🌍🎉🚀fin"); + } + + #[test] + fn test_update_text_preserves_emoji_in_concurrent_edits() { + // Simulate the scenario that motivated operation-based sync: + // Two peers editing a document containing emoji concurrently. + let mut doc = create_doc_with_text("Hello 🎉 world"); + let checkpoint = doc.get_heads(); + + // Peer A: insert " beautiful" after 🎉 (at UTF-16 offset 8) + let (_, text_obj) = doc.get(ROOT, "text").unwrap().unwrap(); + doc.transact::<_, _, automerge::AutomergeError>(|tx| { + tx.splice_text(&text_obj, 8, 0, " beautiful")?; + Ok(()) + }) + .unwrap(); + + // Peer B (forked at checkpoint): append "!" + let mut forked = doc.fork_at(&checkpoint).unwrap(); + let (_, text_obj_b) = forked.get(ROOT, "text").unwrap().unwrap(); + let forked_len = forked.length(&text_obj_b); + forked + .transact::<_, _, automerge::AutomergeError>(|tx| { + tx.splice_text(&text_obj_b, forked_len, 0, "!")?; + Ok(()) + }) + .unwrap(); + + // Merge — both edits should survive + doc.merge(&mut forked).unwrap(); + let result = read_text(&doc); + + // Both the " beautiful" insertion and the trailing "!" should be present. + // The emoji must be intact (not split or corrupted). + assert!(result.contains("🎉"), "emoji must survive merge"); + assert!(result.contains("beautiful"), "peer A's edit must survive"); + assert!(result.ends_with('!'), "peer B's edit must survive"); + } + + #[test] + fn test_splice_text_with_mixed_bmp_and_non_bmp() { + // Mix of BMP (CJK) and non-BMP (emoji) characters + let mut doc = create_doc_with_text("你好🌍世界"); + let (_, text_obj) = doc.get(ROOT, "text").unwrap().unwrap(); + + // UTF-16: 你=0 好=1 🌍=2,3 世=4 界=5 → length=6 + // (CJK characters are in the BMP, so 1 code unit each) + assert_eq!(doc.length(&text_obj), 6); + + // Insert between 🌍 and 世 (offset 4) + doc.transact::<_, _, automerge::AutomergeError>(|tx| { + tx.splice_text(&text_obj, 4, 0, "🎉")?; + Ok(()) + }) + .unwrap(); + + assert_eq!(read_text(&doc), "你好🌍🎉世界"); + // New length: 你=0 好=1 🌍=2,3 🎉=4,5 世=6 界=7 → 8 + assert_eq!(doc.length(&text_obj), 8); + } } diff --git a/hub-client/src/App.tsx b/hub-client/src/App.tsx index 901dcf09..511fc3df 100644 --- a/hub-client/src/App.tsx +++ b/hub-client/src/App.tsx @@ -10,9 +10,10 @@ import { disconnect, setSyncHandlers, getFileContent, - updateFileContent, + applyEditorOperations, createNewProject, type ActorIdentity, + type EditorContentChange, } from './services/automergeSync'; import type { ProjectFile } from './services/wasmRenderer'; import * as projectStorage from './services/projectStorage'; @@ -377,15 +378,8 @@ function App() { navigateToProjectSelector({ replace: true }); }, [navigateToProjectSelector]); - const handleContentChange = useCallback((path: string, content: string) => { - // updateFileContent fires handle.change(), which synchronously triggers the - // 'change' event on the DocHandle. The registered changeHandler calls - // callbacks.onFileChanged with the true merged Automerge document state, - // which propagates to setFileContents via onFileContent. Setting fileContents - // directly here with the raw editor content would overwrite that merged state, - // causing concurrent remote edits to be silently deleted by subsequent - // updateText calls that diff against the stale Monaco content. - updateFileContent(path, content); + const handleContentOperations = useCallback((path: string, changes: EditorContentChange[]) => { + applyEditorOperations(path, changes); }, []); const handleProjectCreated = useCallback(async ( @@ -496,7 +490,7 @@ function App() { files={files} fileContents={fileContents} onDisconnect={handleDisconnect} - onContentChange={handleContentChange} + onContentOperations={handleContentOperations} route={route} onNavigateToFile={(filePath, options) => { navigateToFile(project.id, filePath, options); diff --git a/hub-client/src/components/Editor.tsx b/hub-client/src/components/Editor.tsx index db8102b3..68a5d77f 100644 --- a/hub-client/src/components/Editor.tsx +++ b/hub-client/src/components/Editor.tsx @@ -11,6 +11,7 @@ import { deleteFile, renameFile, exportProjectAsZip, + type EditorContentChange, } from '../services/automergeSync'; import { vfsAddFile, isWasmReady } from '../services/wasmRenderer'; import type { Diagnostic } from '../types/diagnostic'; @@ -45,7 +46,7 @@ interface Props { files: FileEntry[]; fileContents: Map; onDisconnect: () => void; - onContentChange: (path: string, content: string) => void; + onContentOperations: (path: string, changes: EditorContentChange[]) => void; /** Current route from URL */ route: Route; /** Callback to update URL when file changes */ @@ -101,7 +102,7 @@ function selectDefaultFile(files: FileEntry[]): FileEntry | null { return files[0]; } -export default function Editor({ project, files, fileContents, onDisconnect, onContentChange, route, onNavigateToFile, identities, isOnline }: Props) { +export default function Editor({ project, files, fileContents, onDisconnect, onContentOperations, route, onNavigateToFile, identities, isOnline }: Props) { // View mode for pane sizing const { viewMode } = useViewMode(); @@ -510,7 +511,7 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC } }, [route, files, fileContents, currentFile]); - const handleEditorChange = (value: string | undefined) => { + const handleEditorChange = (value: string | undefined, event: Monaco.editor.IModelContentChangedEvent) => { // Skip changes during replay mode. Use the ref (always current) rather than // the closure value (can be stale between setState and re-render). if (replayActiveRef.current) return; @@ -519,11 +520,26 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC if (value !== undefined && currentFile) { setContent(value); - onContentChange(currentFile.path, value); - // Thumbnail regeneration will be triggered by handleAstChange when preview finishes + onContentOperations(currentFile.path, event.changes); } }; + // Route AST rewrites through Monaco → onChange → splice path. + // Uses diffToMonacoEdits to compute minimal edits, so concurrent remote + // edits in unchanged regions merge cleanly via CRDT. Also preserves undo. + const handleContentRewrite = useCallback((newContent: string) => { + if (!editorRef.current || !currentFile) return; + const model = editorRef.current.getModel(); + if (!model) return; + + const oldContent = model.getValue(); + const edits = diffToMonacoEdits(oldContent, newContent); + if (edits.length > 0) { + editorRef.current.executeEdits('ast-rewrite', edits); + } + // onChange fires synchronously → handleEditorChange → setContent + onContentOperations + }, [currentFile]); + // Configure Monaco before mount (for TypeScript diagnostics) const handleBeforeMount = (monaco: typeof Monaco) => { // Disable TypeScript diagnostics to avoid noisy errors in TSX/TS files @@ -757,13 +773,8 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC text: markdown, forceMoveMarkers: true, }]); - - // Update local content state to match - const newContent = editorRef.current.getValue(); - setContent(newContent); - if (currentFile) { - onContentChange(currentFile.path, newContent); - } + // Monaco's onChange fires synchronously from executeEdits, + // updating both React state and CRDT via the splice path. } return; // Internal drag handled, don't process as external } catch { @@ -793,7 +804,7 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC setPendingUploadFiles(files); setShowNewFileDialog(true); } - }, [currentFile, onContentChange]); + }, [currentFile]); // Cleanup editor drag-drop listeners and Monaco providers on unmount useEffect(() => { @@ -850,23 +861,18 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC text: markdown, forceMoveMarkers: true, }]); + // Monaco's onChange fires synchronously from executeEdits, + // updating both React state and CRDT via the splice path. // Clear the pending position after insertion pendingDropPositionRef.current = null; - - // Update local content state to match - const newContent = editorRef.current.getValue(); - setContent(newContent); - if (currentFile) { - onContentChange(currentFile.path, newContent); - } } } catch (err) { console.error('Failed to upload file:', err); // Clear pending position on error too pendingDropPositionRef.current = null; } - }, [handleCreateTextFile, currentFile, onContentChange]); + }, [handleCreateTextFile, currentFile]); // Handle deleting a file const handleDeleteFile = useCallback((file: FileEntry) => { @@ -1083,7 +1089,7 @@ export default function Editor({ project, files, fileContents, onDisconnect, onC currentSlideIndex={currentSlideIndex} onSlideChange={handleSlideChange} onFormatChange={handleFormatChange} - setContent={handleEditorChange} + onContentRewrite={handleContentRewrite} /> diff --git a/hub-client/src/components/render/PreviewRouter.tsx b/hub-client/src/components/render/PreviewRouter.tsx index 5cf39848..3fd7085a 100644 --- a/hub-client/src/components/render/PreviewRouter.tsx +++ b/hub-client/src/components/render/PreviewRouter.tsx @@ -28,7 +28,7 @@ interface PreviewRouterProps { currentSlideIndex?: number; onSlideChange?: (slideIndex: number) => void; onFormatChange?: (format: string | null) => void; - setContent: (content: string) => void; + onContentRewrite: (content: string) => void; } /** @@ -114,7 +114,7 @@ export default function PreviewRouter(props: PreviewRouterProps) { } // Render the appropriate preview component with shared WASM error banner - const { onRegisterScrollToLine, onRegisterSetScrollRatio, onFormatChange, setContent, fileContents, ...commonProps } = props; + const { onRegisterScrollToLine, onRegisterSetScrollRatio, onFormatChange, onContentRewrite, fileContents, ...commonProps } = props; return (
@@ -124,7 +124,7 @@ export default function PreviewRouter(props: PreviewRouterProps) { )}
{reactFormat ? ( - + ) : ( )} diff --git a/hub-client/src/components/render/ReactPreview.tsx b/hub-client/src/components/render/ReactPreview.tsx index 4a1fe4b9..3192e2b5 100644 --- a/hub-client/src/components/render/ReactPreview.tsx +++ b/hub-client/src/components/render/ReactPreview.tsx @@ -35,7 +35,7 @@ interface PreviewProps { onAstChange?: (astJson: string | null) => void; currentSlideIndex?: number; onSlideChange?: (slideIndex: number) => void; - setContent: (content: string) => void; + onContentRewrite: (content: string) => void; format: string; // 'q2-slides' or 'q2-debug' } @@ -105,7 +105,7 @@ export default function ReactPreview({ onAstChange, currentSlideIndex, onSlideChange, - setContent, + onContentRewrite, format, }: PreviewProps) { // Preview state machine for error handling @@ -209,11 +209,11 @@ export default function ReactPreview({ const handleSetAst = useCallback((newAst: any) => { try { const newQmd = incrementalWriteQmd(content, newAst); - setContent(newQmd); + onContentRewrite(newQmd); } catch (err) { console.error('Failed to write AST back to QMD:', err); } - }, [content, setContent]); + }, [content, onContentRewrite]); return (
diff --git a/hub-client/src/services/automergeSync.ts b/hub-client/src/services/automergeSync.ts index 786e9641..e7bc9a96 100644 --- a/hub-client/src/services/automergeSync.ts +++ b/hub-client/src/services/automergeSync.ts @@ -11,6 +11,7 @@ import { type SyncClient, type SyncClientCallbacks, type Patch, + type EditorContentChange, type FileEntry, type ActorIdentity, type CreateBinaryFileResult, @@ -22,7 +23,7 @@ import { import { vfsAddFile, vfsAddBinaryFile, vfsRemoveFile, vfsClear, initWasm } from './wasmRenderer'; // Re-export types for use in other components -export type { Patch, FileEntry, ActorIdentity, CreateBinaryFileResult, CreateProjectOptions, CreateProjectResult }; +export type { Patch, EditorContentChange, FileEntry, ActorIdentity, CreateBinaryFileResult, CreateProjectOptions, CreateProjectResult }; // Event handlers for state changes type FilesChangeHandler = (files: FileEntry[]) => void; @@ -157,6 +158,16 @@ export function updateFileContent(path: string, content: string): void { // VFS is updated via callback } +/** + * Apply positional editor operations (splice-based) to a text file. + * Each change maps directly to an Automerge splice, avoiding the + * full-text diff race in updateText. + */ +export function applyEditorOperations(path: string, changes: EditorContentChange[]): void { + ensureClient().applyEditorOperations(path, changes); + // VFS is updated via callback (change handler fires onFileChanged) +} + /** * Create a new text file in the project. */ diff --git a/hub-client/src/test-utils/mockSyncClient.ts b/hub-client/src/test-utils/mockSyncClient.ts index b81f0237..aff3fe6c 100644 --- a/hub-client/src/test-utils/mockSyncClient.ts +++ b/hub-client/src/test-utils/mockSyncClient.ts @@ -12,6 +12,7 @@ import type { Patch } from '@automerge/automerge-repo'; import type { SyncClientCallbacks, + EditorContentChange, TextFilePayload, BinaryFilePayload, FilePayload, @@ -166,6 +167,23 @@ export function createMockSyncClient( callbacks.onFileChanged(path, content, []); }, + applyEditorOperations(path: string, changes: EditorContentChange[]): void { + if (changes.length === 0) return; + const existing = files.get(path); + if (!existing || existing.type !== 'text') { + throw new Error(`Cannot update non-text file: ${path}`); + } + // Apply changes end-to-beginning (matching real splice behavior) + let text = existing.text; + for (const change of changes) { + const before = text.slice(0, change.rangeOffset); + const after = text.slice(change.rangeOffset + change.rangeLength); + text = before + change.text + after; + } + files.set(path, { type: 'text', text }); + callbacks.onFileChanged(path, text, []); + }, + async createFile(path: string, content: string = ''): Promise { const payload: TextFilePayload = { type: 'text', text: content }; files.set(path, payload); diff --git a/ts-packages/quarto-sync-client/src/applyEditorOperations.test.ts b/ts-packages/quarto-sync-client/src/applyEditorOperations.test.ts new file mode 100644 index 00000000..792758bb --- /dev/null +++ b/ts-packages/quarto-sync-client/src/applyEditorOperations.test.ts @@ -0,0 +1,315 @@ +/** + * Tests for applyEditorOperations — positional splice-based text editing. + * + * TDD: these tests are written BEFORE the implementation. + * They verify that applyEditorOperations correctly translates Monaco's + * IModelContentChange events into Automerge splice operations. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { DocumentId } from '@automerge/automerge-repo'; +import type { EditorContentChange } from './types.js'; + +// ── Mocks ────────────────────────────────────────────────────────────── + +// Track splice calls for verification +const spliceSpy = vi.fn(); + +vi.mock('@automerge/automerge', () => ({ + clone: vi.fn((doc: unknown) => structuredClone(doc)), + from: vi.fn((val: unknown) => structuredClone(val)), + save: vi.fn(() => new Uint8Array(0)), +})); + +vi.mock('@automerge/automerge-repo-network-websocket', () => ({ + BrowserWebSocketClientAdapter: vi.fn(), +})); + +vi.mock('@automerge/automerge-repo-storage-indexeddb', () => ({ + IndexedDBStorageAdapter: vi.fn(), +})); + +vi.mock('@quarto/quarto-automerge-schema', async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + setIdentity: vi.fn(), + }; +}); + +// Mock Repo + splice +vi.mock('@automerge/automerge-repo', async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + Repo: vi.fn(), + splice: (...args: unknown[]) => { + spliceSpy(...args); + }, + generateAutomergeUrl: original.generateAutomergeUrl, + parseAutomergeUrl: original.parseAutomergeUrl, + }; +}); + +import { Repo } from '@automerge/automerge-repo'; +import { createSyncClient } from './client.js'; +import type { SyncClientCallbacks } from './types.js'; +import type { IndexDocument, TextDocumentContent } from '@quarto/quarto-automerge-schema'; + +// ── Helpers ──────────────────────────────────────────────────────────── + +function createMockHandle(initialDoc: T) { + let current = structuredClone(initialDoc); + const changeListeners: Array<(args: { doc: T; patches: never[] }) => void> = []; + const handle = { + documentId: 'mock-doc-id' as DocumentId, + doc: () => current, + change: (fn: (d: T) => void) => { + const draft = structuredClone(current); + fn(draft); + current = draft; + // Fire change listeners so subscriptions propagate + for (const cb of changeListeners) { + cb({ doc: current, patches: [] }); + } + }, + update: (fn: (d: T) => T) => { + current = fn(structuredClone(current)); + }, + on: vi.fn((event: string, cb: (args: { doc: T; patches: never[] }) => void) => { + if (event === 'change') { + changeListeners.push(cb); + } + }), + off: vi.fn(), + whenReady: () => Promise.resolve(), + }; + return { handle, getDoc: () => current }; +} + +function noopCallbacks(): SyncClientCallbacks { + return { + onFileAdded: vi.fn(), + onFileChanged: vi.fn(), + onBinaryChanged: vi.fn(), + onFileRemoved: vi.fn(), + onFilesChange: vi.fn(), + onIdentitiesChange: vi.fn(), + onConnectionChange: vi.fn(), + onError: vi.fn(), + }; +} + +/** + * Set up a connected client with a file handle for 'test.qmd'. + * Returns the client and a getter for the file document. + */ +async function setupClientWithFile(initialText: string) { + const indexDoc: IndexDocument = { + files: { 'test.qmd': 'file-doc-id' as DocumentId }, + version: 1, + identities: {}, + }; + const { handle: indexHandle } = createMockHandle(indexDoc); + + const fileDoc: TextDocumentContent = { text: initialText }; + const { handle: fileHandle, getDoc } = createMockHandle(fileDoc); + + const mockNetworkSubsystem = { on: vi.fn(), off: vi.fn() }; + + vi.mocked(Repo).mockImplementation(function (this: unknown) { + Object.assign(this as Record, { + find: vi.fn().mockImplementation((url: string) => { + // Return fileHandle for file doc lookups, indexHandle for index + if (url.includes('file-doc-id')) return Promise.resolve(fileHandle); + return Promise.resolve(indexHandle); + }), + import: vi.fn().mockReturnValue(indexHandle), + create: vi.fn().mockReturnValue(fileHandle), + networkSubsystem: mockNetworkSubsystem, + }); + return this as Repo; + } as unknown as typeof Repo); + + const cbs = noopCallbacks(); + const client = createSyncClient(cbs); + + await client.connect('ws://localhost:9999', 'mock-doc-id', 'actor-1', 'Test', '#000'); + + return { client, getDoc, cbs }; +} + +// ── Tests ────────────────────────────────────────────────────────────── + +describe('applyEditorOperations', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + // 1.2: Single insert + it('applies a single insert at a position', async () => { + const { client } = await setupClientWithFile('Hello world'); + + const changes: EditorContentChange[] = [ + { rangeOffset: 5, rangeLength: 0, text: ',' }, + ]; + + client.applyEditorOperations('test.qmd', changes); + + expect(spliceSpy).toHaveBeenCalledTimes(1); + expect(spliceSpy).toHaveBeenCalledWith( + expect.anything(), // doc + ['text'], // path + 5, // offset + 0, // deleteCount + ',', // insertText + ); + }); + + // 1.2: Single delete + it('applies a single delete', async () => { + const { client } = await setupClientWithFile('Hello world'); + + const changes: EditorContentChange[] = [ + { rangeOffset: 5, rangeLength: 1, text: '' }, + ]; + + client.applyEditorOperations('test.qmd', changes); + + expect(spliceSpy).toHaveBeenCalledTimes(1); + expect(spliceSpy).toHaveBeenCalledWith( + expect.anything(), + ['text'], + 5, + 1, + '', + ); + }); + + // 1.2: Replace + it('applies a replace operation', async () => { + const { client } = await setupClientWithFile('Hello world'); + + const changes: EditorContentChange[] = [ + { rangeOffset: 6, rangeLength: 5, text: 'there' }, + ]; + + client.applyEditorOperations('test.qmd', changes); + + expect(spliceSpy).toHaveBeenCalledTimes(1); + expect(spliceSpy).toHaveBeenCalledWith( + expect.anything(), + ['text'], + 6, + 5, + 'there', + ); + }); + + // 1.2: Multi-change batch (e.g., find-replace) + it('applies all changes from a batch in a single transaction', async () => { + const { client } = await setupClientWithFile('aaa bbb ccc'); + + // Replace all 3-letter words (changes ordered end-to-beginning by Monaco) + const changes: EditorContentChange[] = [ + { rangeOffset: 8, rangeLength: 3, text: 'ZZZ' }, + { rangeOffset: 4, rangeLength: 3, text: 'YYY' }, + { rangeOffset: 0, rangeLength: 3, text: 'XXX' }, + ]; + + client.applyEditorOperations('test.qmd', changes); + + // All splices should be in one transaction (spliceSpy called 3 times) + expect(spliceSpy).toHaveBeenCalledTimes(3); + + // Verify end-to-beginning order is preserved + expect(spliceSpy).toHaveBeenNthCalledWith(1, expect.anything(), ['text'], 8, 3, 'ZZZ'); + expect(spliceSpy).toHaveBeenNthCalledWith(2, expect.anything(), ['text'], 4, 3, 'YYY'); + expect(spliceSpy).toHaveBeenNthCalledWith(3, expect.anything(), ['text'], 0, 3, 'XXX'); + }); + + // 1.2: Multi-cursor (N changes at arbitrary positions) + it('handles multi-cursor edits at arbitrary positions', async () => { + const { client } = await setupClientWithFile('line1\nline2\nline3'); + + // Multi-cursor inserts "X" at start of each line (end-to-beginning) + const changes: EditorContentChange[] = [ + { rangeOffset: 12, rangeLength: 0, text: 'X' }, + { rangeOffset: 6, rangeLength: 0, text: 'X' }, + { rangeOffset: 0, rangeLength: 0, text: 'X' }, + ]; + + client.applyEditorOperations('test.qmd', changes); + + expect(spliceSpy).toHaveBeenCalledTimes(3); + expect(spliceSpy).toHaveBeenNthCalledWith(1, expect.anything(), ['text'], 12, 0, 'X'); + expect(spliceSpy).toHaveBeenNthCalledWith(2, expect.anything(), ['text'], 6, 0, 'X'); + expect(spliceSpy).toHaveBeenNthCalledWith(3, expect.anything(), ['text'], 0, 0, 'X'); + }); + + // 1.4: Non-BMP characters (emoji) — UTF-16 offsets + it('handles non-BMP characters with correct UTF-16 offsets', async () => { + // '🎉' is U+1F389, which is 2 UTF-16 code units (surrogate pair) + const { client } = await setupClientWithFile('🎉 hello'); + + // Insert " world" after "hello" (offset: 🎉=2 + space=1 + hello=5 = 8) + const changes: EditorContentChange[] = [ + { rangeOffset: 8, rangeLength: 0, text: ' world' }, + ]; + + client.applyEditorOperations('test.qmd', changes); + + expect(spliceSpy).toHaveBeenCalledWith( + expect.anything(), + ['text'], + 8, + 0, + ' world', + ); + }); + + // 1.4: Delete after emoji + it('handles deletion after emoji with correct UTF-16 offsets', async () => { + const { client } = await setupClientWithFile('🎉🎊 test'); + + // Delete " test" (offset: 🎉=2 + 🎊=2 = 4, length: 5) + const changes: EditorContentChange[] = [ + { rangeOffset: 4, rangeLength: 5, text: '' }, + ]; + + client.applyEditorOperations('test.qmd', changes); + + expect(spliceSpy).toHaveBeenCalledWith( + expect.anything(), + ['text'], + 4, + 5, + '', + ); + }); + + // 1.5: Empty changes array is a no-op + it('does nothing for an empty changes array', async () => { + const { client } = await setupClientWithFile('Hello world'); + + client.applyEditorOperations('test.qmd', []); + + // splice should never be called — no Automerge transaction created + expect(spliceSpy).not.toHaveBeenCalled(); + }); + + // Edge case: no handle for path + it('warns and returns when no handle exists for path', async () => { + const { client } = await setupClientWithFile('Hello'); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + client.applyEditorOperations('nonexistent.qmd', [ + { rangeOffset: 0, rangeLength: 0, text: 'X' }, + ]); + + expect(spliceSpy).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('nonexistent.qmd')); + + warnSpy.mockRestore(); + }); +}); diff --git a/ts-packages/quarto-sync-client/src/client.ts b/ts-packages/quarto-sync-client/src/client.ts index da4c73f1..77aa2cd3 100644 --- a/ts-packages/quarto-sync-client/src/client.ts +++ b/ts-packages/quarto-sync-client/src/client.ts @@ -6,7 +6,7 @@ * them to provide their own storage/VFS implementation. */ -import { Repo, DocHandle, updateText, generateAutomergeUrl, parseAutomergeUrl } from '@automerge/automerge-repo'; +import { Repo, DocHandle, updateText, splice, generateAutomergeUrl, parseAutomergeUrl } from '@automerge/automerge-repo'; import type { DocumentId, Patch } from '@automerge/automerge-repo'; import { clone as automergeClone, from as automergeFrom, save as automergeSerialize } from '@automerge/automerge'; import { BrowserWebSocketClientAdapter } from '@automerge/automerge-repo-network-websocket'; @@ -29,6 +29,7 @@ import { } from '@quarto/quarto-automerge-schema'; import type { + EditorContentChange, SyncClientCallbacks, ASTOptions, CreateBinaryFileResult, @@ -514,6 +515,31 @@ export function createSyncClient(callbacks: SyncClientCallbacks, astOptions?: AS // remote edits to be silently deleted on the next updateFileContent call. } + /** + * Apply positional editor operations (splice-based) to a text file. + * + * Each change maps directly to an Automerge `splice` call, avoiding the + * full-text diff race condition in `updateText`. Changes must be ordered + * end-to-beginning (Monaco's documented ordering) so earlier offsets + * remain valid as later changes are applied. + */ + function applyEditorOperations(path: string, changes: EditorContentChange[]): void { + if (changes.length === 0) return; + + const handle = state.fileHandles.get(path); + if (!handle) { + console.warn(`No handle found for file: ${path}`); + return; + } + + handle.change((doc) => { + const textDoc = doc as unknown as { text: string }; + for (const change of changes) { + splice(textDoc, ['text'], change.rangeOffset, change.rangeLength, change.text); + } + }); + } + /** * Create a new text file. */ @@ -882,6 +908,7 @@ export function createSyncClient(callbacks: SyncClientCallbacks, astOptions?: AS getFileContent, getBinaryFileContent, updateFileContent, + applyEditorOperations, updateFileAst, getFileAst, createFile, diff --git a/ts-packages/quarto-sync-client/src/index.ts b/ts-packages/quarto-sync-client/src/index.ts index e875df51..d09e1211 100644 --- a/ts-packages/quarto-sync-client/src/index.ts +++ b/ts-packages/quarto-sync-client/src/index.ts @@ -29,6 +29,7 @@ export { // Export sync client types export type { Patch, + EditorContentChange, TextFilePayload, BinaryFilePayload, FilePayload, diff --git a/ts-packages/quarto-sync-client/src/types.ts b/ts-packages/quarto-sync-client/src/types.ts index 2bfe1e0b..b6f1265f 100644 --- a/ts-packages/quarto-sync-client/src/types.ts +++ b/ts-packages/quarto-sync-client/src/types.ts @@ -8,6 +8,26 @@ import type { FileEntry, ActorIdentity } from '@quarto/quarto-automerge-schema'; // Re-export Patch for consumers export type { Patch }; +// ============================================================================ +// Editor Operation Types +// ============================================================================ + +/** + * Mirrors Monaco's `IModelContentChange` shape for positional text operations. + * Used by `applyEditorOperations` to apply splice-based edits to Automerge. + * + * `rangeOffset` and `rangeLength` are in UTF-16 code units (JavaScript's native + * string encoding), matching both Monaco and Automerge's WASM build. + */ +export interface EditorContentChange { + /** The offset of the range that got replaced (UTF-16 code units). */ + rangeOffset: number; + /** The length of the range that got replaced (UTF-16 code units). */ + rangeLength: number; + /** The new text for the range. */ + text: string; +} + // ============================================================================ // File Payload Types (discriminated union) // ============================================================================ From 19a9ec65afa8410ea16026e9a876a5d5e10966c5 Mon Sep 17 00:00:00 2001 From: shikokuchuo <53399081+shikokuchuo@users.noreply.github.com> Date: Sat, 28 Mar 2026 10:12:50 -0700 Subject: [PATCH 2/2] Add e2e regression test --- .../src/concurrent-editing.test.ts | 327 ++++++++++++++++++ 1 file changed, 327 insertions(+) create mode 100644 ts-packages/sync-test-harness/src/concurrent-editing.test.ts diff --git a/ts-packages/sync-test-harness/src/concurrent-editing.test.ts b/ts-packages/sync-test-harness/src/concurrent-editing.test.ts new file mode 100644 index 00000000..381a779e --- /dev/null +++ b/ts-packages/sync-test-harness/src/concurrent-editing.test.ts @@ -0,0 +1,327 @@ +/** + * Concurrent editing integration tests. + * + * Two real sync clients connected to the same hub server edit the same + * document concurrently. Verifies that: + * + * 1. applyEditorOperations (splice) preserves both peers' edits + * 2. updateFileContent with stale state destroys the remote peer's edit + * + * This is the end-to-end regression test for the splice-based sync fix. + */ + +// Polyfill IndexedDB for Node.js (required by automerge-repo-storage-indexeddb) +import 'fake-indexeddb/auto'; + +import { describe, test, beforeAll, afterAll, expect } from 'vitest'; +import { + createSyncClient, + type SyncClientCallbacks, + type FilePayload, + type Patch, +} from '@quarto/quarto-sync-client'; +import { startHubServer, type ServerHandle } from './server-manager.js'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +interface TrackedClient { + client: ReturnType; + files: Map; + errors: Error[]; + /** Resolves when onFileAdded or onFileChanged delivers content for `path`. */ + waitForFile(path: string, timeoutMs?: number): Promise; + /** Resolves when onFileChanged fires for `path` (ignores onFileAdded). */ + waitForChange(path: string, timeoutMs?: number): Promise; + /** Poll getFileContent until `predicate` passes or timeout. */ + waitForContent(path: string, predicate: (text: string) => boolean, timeoutMs?: number): Promise; +} + +function createTrackedClient(): TrackedClient { + const files = new Map(); + const errors: Error[] = []; + const fileWaiters = new Map void>>(); + const changeWaiters = new Map void>>(); + + function notifyWaiters(map: Map void>>, path: string, text: string) { + const waiters = map.get(path); + if (waiters) { + for (const resolve of waiters) resolve(text); + map.delete(path); + } + } + + const callbacks: SyncClientCallbacks = { + onFileAdded(path: string, file: FilePayload) { + if (file.type === 'text') { + files.set(path, file.text); + notifyWaiters(fileWaiters, path, file.text); + } + }, + onFileChanged(path: string, text: string, _patches: Patch[]) { + files.set(path, text); + notifyWaiters(fileWaiters, path, text); + notifyWaiters(changeWaiters, path, text); + }, + onBinaryChanged() {}, + onFileRemoved(path: string) { + files.delete(path); + }, + onFilesChange() {}, + onConnectionChange() {}, + onError(error: Error) { + errors.push(error); + }, + }; + + const client = createSyncClient(callbacks); + + function makeWaiter(map: Map void>>, path: string, timeoutMs: number): Promise { + // If already available (for fileWaiters), resolve immediately + if (map === fileWaiters && files.has(path)) { + return Promise.resolve(files.get(path)!); + } + return new Promise((resolve, reject) => { + const timeout = setTimeout( + () => reject(new Error(`Timed out waiting for ${path}`)), + timeoutMs, + ); + const waiters = map.get(path) ?? []; + waiters.push((text) => { + clearTimeout(timeout); + resolve(text); + }); + map.set(path, waiters); + }); + } + + function waitForContent(path: string, predicate: (text: string) => boolean, timeoutMs = 5000): Promise { + // Check immediately + const current = client.getFileContent(path); + if (current !== null && predicate(current)) return Promise.resolve(current); + + return new Promise((resolve, reject) => { + const deadline = Date.now() + timeoutMs; + const interval = setInterval(() => { + const text = client.getFileContent(path); + if (text !== null && predicate(text)) { + clearInterval(interval); + resolve(text); + } else if (Date.now() > deadline) { + clearInterval(interval); + reject(new Error(`Timed out waiting for content predicate on ${path} (last: ${text})`)); + } + }, 50); + }); + } + + return { + client, + files, + errors, + waitForFile: (path, timeoutMs = 5000) => makeWaiter(fileWaiters, path, timeoutMs), + waitForChange: (path, timeoutMs = 5000) => makeWaiter(changeWaiters, path, timeoutMs), + waitForContent, + }; +} + +// --------------------------------------------------------------------------- +// Test suites — separated so the "bug demonstration" suite (updateFileContent +// only) is fully independent from the "fix verification" suite +// (applyEditorOperations). This avoids IndexedDB cross-contamination when one +// suite fails because the fix is missing. +// --------------------------------------------------------------------------- + +const HUB_PORT = 18_300; +let server: ServerHandle; + +beforeAll(async () => { + server = await startHubServer({ port: HUB_PORT }); +}, 120_000); + +afterAll(async () => { + await server?.stop(); +}); + +// ========================================================================= +// Suite 1: Demonstrate the updateFileContent race condition (the bug). +// Uses only the old API — passes with or without the splice fix. +// ========================================================================= + +describe('updateFileContent race condition', () => { + test('stale content destroys remote edit', async () => { + const clientA = createTrackedClient(); + const result = await clientA.client.createNewProject({ + syncServer: server.url, + files: [{ path: 'doc.qmd', content: 'Hello world', contentType: 'text' }], + }); + + const clientB = createTrackedClient(); + await clientB.client.connect(server.url, result.indexDocId); + await clientB.waitForFile('doc.qmd'); + + // Client A inserts "REMOTE " at the start + const changePromise = clientB.waitForChange('doc.qmd'); + clientA.client.updateFileContent('doc.qmd', 'REMOTE Hello world'); + await changePromise; + + expect(clientB.files.get('doc.qmd')).toBe('REMOTE Hello world'); + + // Client B's editor was showing stale "Hello world" and user typed "!". + // With updateFileContent, it sends the full stale text + keystroke: + const changePromiseA = clientA.waitForChange('doc.qmd'); + clientB.client.updateFileContent('doc.qmd', 'Hello world!'); + await changePromiseA; + + // Wait for convergence + await clientA.waitForContent('doc.qmd', (t) => !t.includes('REMOTE')); + + const textA = clientA.client.getFileContent('doc.qmd'); + const textB = clientB.client.getFileContent('doc.qmd'); + + // updateText diffed "REMOTE Hello world" against "Hello world!" and + // decided "REMOTE " should be deleted. This is the bug. + expect(textA).not.toContain('REMOTE'); + expect(textB).not.toContain('REMOTE'); + + await clientA.client.disconnect(); + await clientB.client.disconnect(); + }); +}); + +// ========================================================================= +// Suite 2: Verify that applyEditorOperations (splice) fixes the race. +// These tests fail without the splice fix (applyEditorOperations doesn't +// exist), serving as the regression tests. +// ========================================================================= + +describe('applyEditorOperations (splice fix)', () => { + test('propagates to a second client', async () => { + const clientA = createTrackedClient(); + const result = await clientA.client.createNewProject({ + syncServer: server.url, + files: [{ path: 'doc.qmd', content: 'Hello world', contentType: 'text' }], + }); + + const clientB = createTrackedClient(); + await clientB.client.connect(server.url, result.indexDocId); + await clientB.waitForFile('doc.qmd'); + + expect(clientB.files.get('doc.qmd')).toBe('Hello world'); + + const changePromise = clientB.waitForChange('doc.qmd'); + clientA.client.applyEditorOperations('doc.qmd', [ + { rangeOffset: 5, rangeLength: 0, text: ', beautiful' }, + ]); + + const receivedText = await changePromise; + expect(receivedText).toBe('Hello, beautiful world'); + + await clientA.client.disconnect(); + await clientB.client.disconnect(); + }); + + test('concurrent edits from both clients preserve all text', async () => { + const clientA = createTrackedClient(); + const result = await clientA.client.createNewProject({ + syncServer: server.url, + files: [{ path: 'doc.qmd', content: 'Hello world', contentType: 'text' }], + }); + + const clientB = createTrackedClient(); + await clientB.client.connect(server.url, result.indexDocId); + await clientB.waitForFile('doc.qmd'); + + expect(clientB.files.get('doc.qmd')).toBe('Hello world'); + + // Both clients edit concurrently + clientA.client.applyEditorOperations('doc.qmd', [ + { rangeOffset: 5, rangeLength: 0, text: ', beautiful' }, + ]); + clientB.client.applyEditorOperations('doc.qmd', [ + { rangeOffset: 11, rangeLength: 0, text: '!' }, + ]); + + // Wait for both clients to see both edits + const hasBoth = (t: string) => t.includes(', beautiful') && t.includes('!'); + const [textA, textB] = await Promise.all([ + clientA.waitForContent('doc.qmd', hasBoth), + clientB.waitForContent('doc.qmd', hasBoth), + ]); + + expect(textA).toBe(textB); + + await clientA.client.disconnect(); + await clientB.client.disconnect(); + }); + + test('stale offset preserves remote edit (unlike updateFileContent)', async () => { + const clientA = createTrackedClient(); + const result = await clientA.client.createNewProject({ + syncServer: server.url, + files: [{ path: 'doc.qmd', content: 'Hello world', contentType: 'text' }], + }); + + const clientB = createTrackedClient(); + await clientB.client.connect(server.url, result.indexDocId); + await clientB.waitForFile('doc.qmd'); + + // Client A inserts "REMOTE " at the start + const changePromise = clientB.waitForChange('doc.qmd'); + clientA.client.applyEditorOperations('doc.qmd', [ + { rangeOffset: 0, rangeLength: 0, text: 'REMOTE ' }, + ]); + await changePromise; + + expect(clientB.files.get('doc.qmd')).toBe('REMOTE Hello world'); + + // Client B's editor was stale. Splice inserts "!" at offset 11. + clientB.client.applyEditorOperations('doc.qmd', [ + { rangeOffset: 11, rangeLength: 0, text: '!' }, + ]); + + // Wait for A to see the "!" + const textA = await clientA.waitForContent('doc.qmd', (t) => t.includes('!')); + const textB = clientB.client.getFileContent('doc.qmd'); + + // The "!" lands at offset 11 inside the CRDT ("REMOTE Hello world"), + // splitting "Hello" into "Hell!o" — positionally wrong, but no text is + // destroyed. The key property: every character from both peers survives. + expect(textA).toContain('REMOTE'); + expect(textA).toContain('world'); + expect(textA).toContain('!'); + expect(textA).toHaveLength('REMOTE Hello world'.length + 1); + expect(textB).toBe(textA); // converged + + await clientA.client.disconnect(); + await clientB.client.disconnect(); + }); + + test('batch splice operations propagate correctly', async () => { + const clientA = createTrackedClient(); + const result = await clientA.client.createNewProject({ + syncServer: server.url, + files: [{ path: 'doc.qmd', content: 'foo bar foo baz foo', contentType: 'text' }], + }); + + const clientB = createTrackedClient(); + await clientB.client.connect(server.url, result.indexDocId); + await clientB.waitForFile('doc.qmd'); + + clientA.client.applyEditorOperations('doc.qmd', [ + { rangeOffset: 16, rangeLength: 3, text: 'qux' }, + { rangeOffset: 8, rangeLength: 3, text: 'qux' }, + { rangeOffset: 0, rangeLength: 3, text: 'qux' }, + ]); + + const textB = await clientB.waitForContent( + 'doc.qmd', + (t) => !t.includes('foo'), + ); + expect(textB).toBe('qux bar qux baz qux'); + + await clientA.client.disconnect(); + await clientB.client.disconnect(); + }); +});