diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c71eb93..9eca1b65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to Quoll are documented here. - Pasting text, tables, or rich content together with files now inserts the text and accepted image links together in one undoable step. Captions survive rejected files, and image links keep their clipboard order even when replies arrive out of order. - Undoing while a paste is waiting for images preserves Redo and prevents late image replies from changing the document. Moving the caret while waiting keeps the paste at its original target without moving you back. +- If an internal error interrupts the editor right after a change, that change is still saved. If it cannot be queued, an error banner appears instead of the change being lost silently. ## 0.1.86 — 2026-11-03 diff --git a/src/webview/cm/edit-sync.ts b/src/webview/cm/edit-sync.ts index 3613633d..1a1b22b4 100644 --- a/src/webview/cm/edit-sync.ts +++ b/src/webview/cm/edit-sync.ts @@ -15,7 +15,7 @@ // // Driven by the shell's synchronous post-commit dispatch (editor.ts + // shell.ts): -// - onLocalChange from the CM updateListener (debounced post). +// - onLocalChange from the editor's dispatchTransactions (debounced post). // - onHostSnapshot from applyDocument (records the snapshot's metadata and // judges the in-flight Edit against it, demoting one the host did not apply // into the replay buffer — never touches editInFlight, never drains). @@ -160,7 +160,8 @@ export type EditSyncOptions = { * The implementation must apply it as a host reseed, not as a user edit * (non-history, and not read back through `onLocalChange`). * MUST NEVER RUN INSIDE A CODEMIRROR UPDATE: it dispatches on the view. The - * only drain reachable from the update listener is the one driven by + * only drain reachable from a local edit's hand-off (`noteLocalChange` in + * the editor's `dispatchTransactions`) is the one driven by * `local-edit-attempt`, and that dispatch is preceded by `discardBuffer()`, * so the drain finds no buffer and never gets here. * NOT called while a debounce timer is live: the view then holds a keystroke @@ -213,8 +214,15 @@ export type EditSyncOptions = { export type SettleOutcome = "settled" | "unsynced" | "cancelled"; export type EditSync = { - /** Editor content changed locally (CM updateListener docChanged). */ + /** Editor content changed locally (a committed change, from the editor's + * `dispatchTransactions`). */ onLocalChange: () => void; + /** A committed local change could NOT be scheduled (the editor's + * `onLocalChange` hand-off threw). Hold the view's bytes as the replay + * buffer, so `viewHoldsUnackedEdit` keeps a same-lineage Document from + * reseeding over them and the next open-gate drain posts them. Same + * readonly rule as `trySend`: under `canWrite` false nothing is held. */ + retainLocalChange: () => void; /** A host Document arrived. Records its metadata — the version + canWrite * edit-sync echoes on the next Edit, and the identity pair — and JUDGES the * in-flight Edit against it (below). Returns `true` while that Edit still @@ -1130,6 +1138,11 @@ export function createEditSync(opts: EditSyncOptions): EditSync { // post), and then no reducer commit follows to wake a waiter. rejudgeSettleWaiters(); }), + retainLocalChange: () => { + if (canWrite) { + buffered = stampHeld(opts.getDoc()); + } + }, onHostSnapshot: (d) => { const transition = isIdentityTransition(d.externalEpoch, d.epochGeneration); // Generation-aware acceptance ordering (S3b): version order is meaningful diff --git a/src/webview/editor.ts b/src/webview/editor.ts index e6a87185..f8f0fa38 100644 --- a/src/webview/editor.ts +++ b/src/webview/editor.ts @@ -484,9 +484,81 @@ export function mountEditor(opts: EditorOptions): EditorHandle { pendingCaret = null; }; + // A committed local change → edit-sync. Called from `dispatchTransactions`, + // NOT an updateListener: CodeMirror commits the state before its DOM phase and + // wraps each listener in its own try/catch, so a DOM throw skips the listeners + // and a listener throw is logged and swallowed — both leave the committed edit + // unscheduled with no sign it never reached the host (SYNC-01). Any throw here + // becomes a send failure: the banner says nothing is being saved, and the + // next keystroke's local-edit-attempt below is the retry. + function noteLocalChange(): void { + try { + // Send-failure recovery: when the prior Edit could not be sent, + // the next user keystroke is their retry intent. Two ops in + // STRICT order before scheduling the debounced flush: + // 1. discardBuffer — the buffered bytes precede this keystroke; the + // synchronous onReducerCommit → replayIfNeeded chain + // that fires INSIDE the local-edit-attempt dispatch + // below would otherwise post them again and the + // banner would flicker. ORDER IS LOAD-BEARING — twice + // over: that drain runs INSIDE this call, and + // with a buffer in hand it could reach `showHeld`, which + // dispatches a reseed on the view — right after the very + // update that is delivering the user's keystroke. An + // empty buffer keeps the drain out of that step. + // 2. dispatch(local-edit-attempt) — clears the gate via + // the reducer; drain sees empty buffer (step 1) → no-op. + // Then onLocalChange's debounce reads FRESH getDoc() — which + // includes the keystroke that was just committed — + // and ships a retry on the next timer tick. The order + // regression is pinned by the "synchronous drain does NOT + // replay stale buffered bytes" test in editor.test.ts. + if (opts.getState().sendFailure !== null) { + sync.discardBuffer(); + opts.dispatch({ type: "local-edit-attempt" }); + } + sync.onLocalChange(); + } catch (err) { + console.error("[quoll] a committed edit could not be scheduled for sending", err); + const detail = err instanceof Error ? err.message : String(err); + try { + opts.dispatch({ + type: "send-failure", + message: `an edit could not be scheduled for sending: ${detail}`, + }); + } catch (dispatchErr) { + console.error("[quoll] send-failure dispatch itself failed", dispatchErr); + } + // AFTER the send-failure dispatch: its drain finds the gate closed, so + // the retained bytes are not posted from inside this update. Without the + // hold, a same-lineage Document (an earlier Edit's ack) finds nothing + // un-acked, reseeds over the unsent change, and its reducer arm clears + // the banner — the edit vanishes unseen. + try { + sync.retainLocalChange(); + } catch (retainErr) { + console.error("[quoll] retaining the unscheduled edit failed", retainErr); + } + } + } + const viewCreateStart = QUOLL_PERF ? perfNow() : 0; const view: EditorView = new EditorView({ parent: mount, + // `finally`, so a DOM-phase throw still hands the committed edit to + // edit-sync before it propagates to the dispatcher. Doc identity, not + // `docChanged`: a throw before the commit leaves the doc as it was, and + // there is then nothing to send. + dispatchTransactions(trs, v) { + const before = v.state.doc; + try { + v.update(trs); + } finally { + if (v.state.doc !== before && !seeding) { + noteLocalChange(); + } + } + }, state: EditorState.create({ doc: "", extensions: [ @@ -503,7 +575,7 @@ export function mountEditor(opts: EditorOptions): EditorHandle { // In-editor find & replace over the raw-Markdown text canonical // (@codemirror/search core module). `search({ top: true })` mounts the // panel at the top (VS Code-like). A replace is an ordinary transaction, - // so it rides the normal updateListener → edit-sync → host write-lock → + // so it rides the normal dispatchTransactions → edit-sync → host write-lock → // verified-write pipeline. In read-only // mode CM's replace commands return false and the panel omits its replace // UI (both guard on EditorState.readOnly, set when canWrite=false), so @@ -900,33 +972,6 @@ export function mountEditor(opts: EditorOptions): EditorHandle { ]), EditorView.updateListener.of((u) => { const updateStart = QUOLL_PERF ? perfNow() : 0; - if (u.docChanged && !seeding) { - // Send-failure recovery: when the prior Edit could not be sent, - // the next user keystroke is their retry intent. Two ops in - // STRICT order before scheduling the debounced flush: - // 1. discardBuffer — the buffered bytes precede this keystroke; the - // synchronous onReducerCommit → replayIfNeeded chain - // that fires INSIDE the local-edit-attempt dispatch - // below would otherwise post them again and the - // banner would flicker. ORDER IS LOAD-BEARING — twice - // over: that drain runs INSIDE this update listener, and - // with a buffer in hand it could reach `showHeld`, which - // dispatches a reseed on the view — from within the very - // update that is delivering the user's keystroke. An - // empty buffer keeps the drain out of that step. - // 2. dispatch(local-edit-attempt) — clears the gate via - // the reducer; drain sees empty buffer (step 1) → no-op. - // Then onLocalChange's debounce reads FRESH getDoc() — which - // includes the keystroke that just fired this updateListener — - // and ships a retry on the next timer tick. The order - // regression is pinned by the "synchronous drain does NOT - // replay stale buffered bytes" test above. - if (opts.getState().sendFailure !== null) { - sync.discardBuffer(); - opts.dispatch({ type: "local-edit-attempt" }); - } - sync.onLocalChange(); - } // Caret handoff: report the caret on any selection change (debounced — // see scheduleCaretReport). Suppressed during host seeds (`seeding` — // the reseed restores a selection, not a user move) and during @@ -1043,8 +1088,8 @@ export function mountEditor(opts: EditorOptions): EditorHandle { }); } finally { // try/finally: if dispatch throws, seeding must NOT stay stuck - // true — a stuck guard permanently suppresses the updateListener's - // echo-Edit detection. + // true — a stuck guard permanently suppresses noteLocalChange's + // hand-off of local edits to edit-sync. seeding = false; } } @@ -1056,8 +1101,8 @@ export function mountEditor(opts: EditorOptions): EditorHandle { // that strips a folded heading's OWN marker can orphan its fold entirely (no // canonical range left to clamp to). Only meaningful when the reseed actually // changed the document (reseedChange !== null). This is a SEPARATE, display-only - // dispatch carrying NO `changes` — byte-identical round-trip, and a no-op for the - // updateListener's `docChanged`-gated edit-sync — so it behaves exactly like a + // dispatch carrying NO `changes` — byte-identical round-trip, and a no-op for + // dispatchTransactions' doc-identity-gated edit-sync — so it behaves exactly like a // user fold/unfold (hence NOT annotated as a hostDocumentReseed; it rewrites no // document bytes). addToHistory.of(false) keeps the clamp out of undo. // reconcileReseedFolds self-guards on a complete parse tree. The edited-span diff --git a/test/webview/editor-unsent-edit-faults.test.ts b/test/webview/editor-unsent-edit-faults.test.ts new file mode 100644 index 00000000..7fad195e --- /dev/null +++ b/test/webview/editor-unsent-edit-faults.test.ts @@ -0,0 +1,204 @@ +// @vitest-environment happy-dom +import { StateEffect } from "@codemirror/state"; +import { Decoration, EditorView, WidgetType } from "@codemirror/view"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { PROTOCOL_VERSION } from "../../src/shared/protocol.js"; +import { type EditorHandle, mountEditor } from "../../src/webview/editor.js"; +import { type Action, initialState, type WebviewState } from "../../src/webview/state.js"; + +// SYNC-01: CodeMirror commits the new state BEFORE it touches the DOM, and runs +// each update listener inside its own try/catch. So a throw from the DOM phase +// skips every listener, and a throw from inside the edit-sync listener is logged +// and swallowed — either way the committed edit is in `view.state` but nothing +// schedules it, and the user gets no sign it never reached the host. These pin +// the contract for any dispatch: the edit is scheduled, or it is surfaced. + +const postMessage = vi.fn(); +const editPosts = () => + postMessage.mock.calls + .map((c) => c[0] as { type?: string; content?: string }) + .filter((m) => m?.type === "edit"); + +vi.mock("../../src/webview/host.js", () => ({ + getHost: () => ({ postMessage }), + subscribeToHost: () => () => {}, + readPersistedState: () => ({}), + patchPersistedState: () => {}, +})); + +// Fault-injection seam for the listener arm: wraps the real edit-sync so one +// `onLocalChange` call can be made to throw. +const syncFault = { throwOnLocalChange: false }; +vi.mock("../../src/webview/cm/edit-sync.js", async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + createEditSync: (...args: Parameters) => { + const sync = real.createEditSync(...args); + return { + ...sync, + onLocalChange: () => { + if (syncFault.throwOnLocalChange) { + syncFault.throwOnLocalChange = false; + throw new Error("onLocalChange exploded"); + } + sync.onLocalChange(); + }, + }; + }, + }; +}); + +const SNAPSHOT = { + content: "seed", + eol: "\n" as const, + canWrite: true, + docVersion: 1, + externalEpoch: 0, + epochGeneration: 1, + settledEditId: 0, +}; + +let container: HTMLElement; +const mounted: EditorHandle[] = []; + +beforeEach(() => { + vi.useFakeTimers(); + postMessage.mockReset(); + syncFault.throwOnLocalChange = false; + container = document.createElement("div"); + document.body.appendChild(container); +}); + +afterEach(() => { + for (const handle of mounted.splice(0)) { + handle.dispose(); + } + container.remove(); + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +function mount() { + let state: WebviewState = { ...initialState, ready: true, docVersion: 1, canWrite: true }; + const actions: Action[] = []; + const handle = mountEditor({ + parent: container, + nonce: "test-nonce", + getState: () => state, + dispatch: (action) => { + actions.push(action); + if (action.type === "send-failure") { + state = { ...state, sendFailure: action.message }; + } else if (action.type === "local-edit-attempt") { + state = { ...state, sendFailure: null }; + } + }, + }); + mounted.push(handle); + handle.applyDocument(SNAPSHOT); + const view = EditorView.findFromDOM(container.querySelector(".quoll-editor") as HTMLElement); + if (!view) { + throw new Error("EditorView not found via findFromDOM"); + } + // A host Document as the shell delivers it: applyDocument, then the reducer's + // `document` arm (which clears the send-failure gate), then the drain. + const hostDocument = (snapshot: typeof SNAPSHOT) => { + handle.applyDocument(snapshot); + state = { ...state, docVersion: snapshot.docVersion, sendFailure: null }; + handle.onReducerCommit(false); + }; + return { handle, view, actions, hostDocument }; +} + +// A widget whose DOM build throws: CodeMirror calls toDOM unwrapped inside its +// DOM update, after the state is already committed. +class ExplodingWidget extends WidgetType { + toDOM(): HTMLElement { + throw new Error("toDOM exploded"); + } +} + +describe("editor — a committed edit is never silently unsent (SYNC-01)", () => { + it("still schedules the edit when the DOM update throws", () => { + const { view } = mount(); + let explode = false; + view.dispatch({ + effects: StateEffect.appendConfig.of( + EditorView.decorations.of(() => + explode + ? Decoration.set(Decoration.widget({ widget: new ExplodingWidget() }).range(0)) + : Decoration.none + ) + ), + }); + vi.spyOn(console, "error").mockImplementation(() => {}); + explode = true; + expect(() => view.dispatch({ changes: { from: 4, insert: "x" } })).toThrow("toDOM exploded"); + explode = false; + // The state was committed before the throw… + expect(view.state.doc.toString()).toBe("seedx"); + vi.advanceTimersByTime(300); + // …and those bytes go out on the normal debounce. + expect(editPosts()).toEqual([ + expect.objectContaining({ protocol: PROTOCOL_VERSION, type: "edit", content: "seedx" }), + ]); + }); + + it("surfaces the edit as unsent when edit-sync's scheduling throws, and the next keystroke retries", () => { + const { view, actions } = mount(); + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + syncFault.throwOnLocalChange = true; + view.dispatch({ changes: { from: 4, insert: "x" } }); + vi.advanceTimersByTime(300); + expect(editPosts()).toEqual([]); + const failure = actions.find((a) => a.type === "send-failure"); + expect(failure).toEqual({ + type: "send-failure", + message: expect.stringContaining("onLocalChange exploded"), + }); + expect( + consoleSpy.mock.calls.some( + ([first]) => typeof first === "string" && first.startsWith("[quoll]") + ) + ).toBe(true); + // The retry contract of every send failure: the next keystroke clears the + // gate and ships the WHOLE document, the unsent change included. + view.dispatch({ changes: { from: 5, insert: "y" } }); + vi.advanceTimersByTime(300); + expect(editPosts().map((m) => m.content)).toEqual(["seedxy"]); + }); + + it("keeps the unscheduled edit when a same-lineage Document arrives before the next keystroke, and posts it", () => { + const { view, hostDocument } = mount(); + vi.spyOn(console, "error").mockImplementation(() => {}); + syncFault.throwOnLocalChange = true; + view.dispatch({ changes: { from: 4, insert: "x" } }); + vi.advanceTimersByTime(300); + expect(editPosts()).toEqual([]); + // Same epoch and generation, a newer version, the host's old bytes: a + // Document of the lineage the unsent change was typed on. Its reducer arm + // clears the banner, so a reseed here would lose the edit unseen. + hostDocument({ ...SNAPSHOT, docVersion: 2 }); + expect(view.state.doc.toString()).toBe("seedx"); + expect(editPosts().map((m) => m.content)).toEqual(["seedx"]); + }); + + it("does not retain an unscheduled programmatic edit while read-only, so write access returning cannot post it", () => { + const { view, hostDocument } = mount(); + vi.spyOn(console, "error").mockImplementation(() => {}); + // Read-only, same lineage. Programmatic dispatch is not stopped by the + // readOnly facet, so a change can still be committed here. + hostDocument({ ...SNAPSHOT, docVersion: 2, canWrite: false }); + syncFault.throwOnLocalChange = true; + view.dispatch({ changes: { from: 4, insert: "x" } }); + vi.advanceTimersByTime(300); + expect(editPosts()).toEqual([]); + // Write access returns on the same lineage with the host's bytes. Nothing was + // retained, so the reseed wins and no Edit for the readonly change goes out. + hostDocument({ ...SNAPSHOT, docVersion: 3, canWrite: true }); + vi.advanceTimersByTime(300); + expect(view.state.doc.toString()).toBe("seed"); + expect(editPosts()).toEqual([]); + }); +});