From 297e88dc0385382e6fd30a7970276c2527dfc137 Mon Sep 17 00:00:00 2001 From: Johannes Date: Sat, 18 Jul 2026 10:56:56 +0200 Subject: [PATCH 1/2] Close the typing batch window on undo and redo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A batched apply right after an undo read history[history_index] while last_batch_started was still open. After an undo that slot is stale, or gone entirely (history_index may be -1 once the redo tail is truncated), which crashed with "TypeError: Cannot read properties of undefined (reading 'ops')" — reproducible by typing, pressing undo, and typing again within one second. When the slot still existed, new typing was silently merged into a history entry it didn't belong to, corrupting undo granularity. Undo and redo now close the batch window, so typing after history navigation always starts a fresh entry. Also aligns the batching comments with BATCH_WINDOW_MS (1 second); they still said 2 seconds. Co-Authored-By: Claude Fable 5 --- src/lib/Session.svelte.ts | 11 +++++-- src/test/batch_window.svelte.test.ts | 45 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 src/test/batch_window.svelte.test.ts diff --git a/src/lib/Session.svelte.ts b/src/lib/Session.svelte.ts index 784efa40..3f3dfdd1 100644 --- a/src/lib/Session.svelte.ts +++ b/src/lib/Session.svelte.ts @@ -256,7 +256,7 @@ export default class Session { /** * Applies a transaction to the document. - * Auto-batches history entries with debounced behavior (max one entry per 2 seconds) when batch is true. + * Auto-batches history entries with debounced behavior (max one entry per second) when batch is true. * * @param transaction - The transaction to apply * @param options - Optional configuration (batch: whether to allow batching with previous transaction) @@ -283,7 +283,7 @@ export default class Session { now - this.last_batch_started < BATCH_WINDOW_MS; if (should_batch) { - // Append to existing history entry (within 2s of batch start) + // Append to existing history entry (within the batch window) const last_entry = this.history[this.history_index]; last_entry.ops.push(...transaction.ops); last_entry.inverse_ops.push(...transaction.inverse_ops); @@ -291,7 +291,7 @@ export default class Session { // Trigger update this.history = [...this.history]; } else { - // Create new history entry (more than 2s since batch started, or first edit, or batch not requested) + // Create new history entry (batch window elapsed, first edit, or batch not requested) this.history = [ ...this.history, { @@ -330,6 +330,9 @@ export default class Session { this.doc = doc; this.selection = change.selection_before; this.history_index = this.history_index - 1; + // History navigation ends any open typing batch, so the next batched + // apply starts a fresh entry instead of appending to a stale one. + this.last_batch_started = undefined; return this; } @@ -345,6 +348,8 @@ export default class Session { }); this.doc = doc; this.selection = change.selection_after; + // Redo ends any open typing batch, matching undo(). + this.last_batch_started = undefined; return this; } diff --git a/src/test/batch_window.svelte.test.ts b/src/test/batch_window.svelte.test.ts new file mode 100644 index 00000000..863ae45c --- /dev/null +++ b/src/test/batch_window.svelte.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect } from 'vitest'; +import create_test_session from './create_test_session.js'; + +function set_title_caret(session: ReturnType, offset: number) { + session.selection = { + type: 'text', + path: ['story_1', 'title'], + anchor_offset: offset, + focus_offset: offset + }; +} + +function type_char(session: ReturnType, char: string) { + const tr = session.tr; + tr.insert_text(char); + session.apply(tr, { batch: true }); +} + +describe('batch window after undo/redo', () => { + it('does not crash when a batched apply follows an undo within the batch window', () => { + const session = create_test_session(); + set_title_caret(session, 11); + + type_char(session, 'a'); + session.undo(); + type_char(session, 'b'); + + expect(session.get<{ content: string }>(['story_1', 'title']).content).toBe('First storyb'); + expect(session.history.length).toBe(1); + }); + + it('starts a new entry when typing continues after a redo', () => { + const session = create_test_session(); + set_title_caret(session, 11); + + type_char(session, 'a'); + session.undo(); + session.redo(); + type_char(session, 'b'); + + // 'b' must not merge into the redone 'a' entry. + expect(session.get<{ content: string }>(['story_1', 'title']).content).toBe('First storyab'); + expect(session.history.length).toBe(2); + }); +}); From be7975d19b5f4c7e4e46f4736135b7220aaab1fd Mon Sep 17 00:00:00 2001 From: Johannes Date: Sat, 18 Jul 2026 10:59:13 +0200 Subject: [PATCH 2/2] Route all doc mutations through one commit funnel with change notifications apply(), undo() and redo() now commit through a single private _commit() which swaps the doc and notifies subscribers registered via session.on_change(). Undo and redo previously mutated session.doc directly, so any observer of document changes (autosave, op logs, sync) would silently miss them. on_change listeners receive { ops, inverse_ops, origin } after the doc swap; op arrays are shallow-copied so history batching cannot mutate a delivered change record. Ops-less commits (selection-only transactions) do not notify, matching the existing doc-swap gate. Listener exceptions are caught and logged: a faulty observer cannot abort the commit and desync the recorded history from the already-swapped doc. The ChangeEvent type is exported from the package index. Test helpers shared by the op-engine suites live in src/test/op_engine_helpers.ts. Co-Authored-By: Claude Fable 5 --- src/lib/Session.svelte.ts | 108 +++++++++++++++++--- src/lib/index.ts | 1 + src/test/commit_funnel.svelte.test.ts | 136 ++++++++++++++++++++++++++ src/test/op_engine_helpers.ts | 24 +++++ 4 files changed, 254 insertions(+), 15 deletions(-) create mode 100644 src/test/commit_funnel.svelte.test.ts create mode 100644 src/test/op_engine_helpers.ts diff --git a/src/lib/Session.svelte.ts b/src/lib/Session.svelte.ts index 3f3dfdd1..82307845 100644 --- a/src/lib/Session.svelte.ts +++ b/src/lib/Session.svelte.ts @@ -22,6 +22,7 @@ import type { SelectedAttachment } from './doc_utils.js'; import type { NodeId, DocumentPath, + DocumentOperation, Selection, Attachment, NodeKind, @@ -47,6 +48,16 @@ type HistoryEntry = { selection_after: Selection | null; }; +/** A committed document change, as delivered to on_change listeners. */ +export type ChangeEvent = { + /** The ops that were applied to the document, in application order */ + ops: DocumentOperation[]; + /** Ops that revert this change when applied in reverse order */ + inverse_ops: DocumentOperation[]; + /** What kind of commit produced the change */ + origin: 'transaction' | 'undo' | 'redo'; +}; + export default class Session { #selection: Selection | null = $state.raw(null); @@ -65,6 +76,10 @@ export default class Session { commands: CommandRegistry = $state.raw({}); keymap: Keymap = $state.raw({}); + // Change listeners registered via on_change(). Deliberately not reactive + // state: notification happens explicitly in _commit(). + #change_listeners: Array<(change: ChangeEvent) => void> = []; + // Reactive helpers for UI state can_undo = $derived(this.history_index >= 0); can_redo = $derived(this.history_index < this.history.length - 1); @@ -254,6 +269,63 @@ export default class Session { return new Transaction(this.schema, this.doc, this.selection, this.config); } + /** + * Subscribes to committed document changes. + * + * The listener fires after every commit that changed the document: + * transactions, undo and redo all pass through the same funnel. It + * receives the applied ops, their inverse and the origin of the change. + * Ops-less commits (e.g. selection-only transactions) do not notify. + * + * Listeners fire synchronously right after the doc swap; history and + * selection bookkeeping of the surrounding commit may not have run yet. + * Exceptions thrown by a listener are caught and logged, so a faulty + * observer cannot corrupt that bookkeeping. + * + * This is the integration point for hosts that persist, sync or observe + * documents (autosave, op logs, devtools) without forking Session. + * + * @returns Unsubscribe function + */ + on_change(listener: (change: ChangeEvent) => void): () => void { + this.#change_listeners.push(listener); + return () => { + const index = this.#change_listeners.indexOf(listener); + if (index >= 0) this.#change_listeners.splice(index, 1); + }; + } + + /** + * Commits a new document state and notifies change listeners. + * + * Every mutation of session.doc (apply, undo, redo) goes through this + * single funnel, so observers see every change, including undo and redo. + * + * Only swaps the doc when something changed: the incoming doc is always + * a fresh object, and assigning it for an ops-less commit (e.g. a + * selection-only transaction) would needlessly invalidate every + * doc-dependent derived in the component tree. + */ + private _commit(next_doc: Document, { ops, inverse_ops, origin }: ChangeEvent): void { + if (ops.length === 0) return; + this.doc = next_doc; + if (this.#change_listeners.length === 0) return; + // Shallow-copy the op arrays: history batching mutates them in place + // after the fact (a batched apply pushes follow-up ops into the same + // history entry), and listeners must see a stable record. + const change = { ops: [...ops], inverse_ops: [...inverse_ops], origin }; + for (const listener of [...this.#change_listeners]) { + // A throwing listener must not abort the commit: history and + // selection bookkeeping run after notification, and skipping them + // would desync the recorded history from the already-swapped doc. + try { + listener(change); + } catch (error) { + console.error('on_change listener failed', error); + } + } + } + /** * Applies a transaction to the document. * Auto-batches history entries with debounced behavior (max one entry per second) when batch is true. @@ -263,13 +335,11 @@ export default class Session { */ apply(transaction: Transaction, { batch = false }: { batch?: boolean } = {}): this { this.validate_transaction_result(transaction); - // Only swap the doc when something changed. The transaction draft is - // always a fresh object, and assigning it for an ops-less transaction - // (e.g. selection-only) would needlessly invalidate every doc-dependent - // derived in the component tree. - if (transaction.ops.length > 0) { - this.doc = transaction.doc; - } + this._commit(transaction.doc, { + ops: transaction.ops, + inverse_ops: transaction.inverse_ops, + origin: 'transaction' + }); // Make sure selection gets a new reference (is rerendered) this.selection = structuredClone(transaction.selection); if (this.history_index < this.history.length - 1) { @@ -321,13 +391,17 @@ export default class Session { // One draft for the whole change set — ops mutate the draft with // node-level copy-on-write instead of copying the nodes map per op. const doc = create_document_draft(this.doc); - change.inverse_ops - .slice() - .reverse() - .forEach((op) => { - apply_op_to_draft(doc, op); - }); - this.doc = doc; + const undo_ops = change.inverse_ops.slice().reverse(); + undo_ops.forEach((op) => { + apply_op_to_draft(doc, op); + }); + // Reversing the original ops yields the inverse of this undo commit: + // applying them in reverse order (= original order) redoes the change. + this._commit(doc, { + ops: undo_ops, + inverse_ops: change.ops.slice().reverse(), + origin: 'undo' + }); this.selection = change.selection_before; this.history_index = this.history_index - 1; // History navigation ends any open typing batch, so the next batched @@ -346,7 +420,11 @@ export default class Session { change.ops.forEach((op) => { apply_op_to_draft(doc, op); }); - this.doc = doc; + this._commit(doc, { + ops: change.ops, + inverse_ops: change.inverse_ops, + origin: 'redo' + }); this.selection = change.selection_after; // Redo ends any open typing batch, matching undo(). this.last_batch_started = undefined; diff --git a/src/lib/index.ts b/src/lib/index.ts index 8f13170a..e77cae52 100644 --- a/src/lib/index.ts +++ b/src/lib/index.ts @@ -59,6 +59,7 @@ export type { } from './types.js'; export type { SelectedAttachment } from './doc_utils.js'; export type { Keymap } from './KeyMapper.svelte.js'; +export type { ChangeEvent } from './Session.svelte.js'; // Document utilities export { diff --git a/src/test/commit_funnel.svelte.test.ts b/src/test/commit_funnel.svelte.test.ts new file mode 100644 index 00000000..c73bc0c7 --- /dev/null +++ b/src/test/commit_funnel.svelte.test.ts @@ -0,0 +1,136 @@ +import { describe, it, expect } from 'vitest'; +import create_test_session from './create_test_session.js'; +import type { ChangeEvent } from '../lib/Session.svelte.js'; +import { set_button_content } from './op_engine_helpers.js'; + +describe('commit funnel + on_change', () => { + it('notifies on transaction commits with the applied ops', () => { + const session = create_test_session(); + const events: ChangeEvent[] = []; + session.on_change((change) => events.push(change)); + + set_button_content(session, 'Changed'); + + expect(events.length).toBe(1); + expect(events[0].origin).toBe('transaction'); + expect(events[0].ops).toEqual([ + ['set', ['button_1', 'content'], { content: 'Changed', marks: [], annotations: [] }] + ]); + expect(events[0].inverse_ops).toEqual([ + ['set', ['button_1', 'content'], { content: 'Get started', marks: [], annotations: [] }] + ]); + }); + + it('fires after the document was swapped, so listeners observe the new state', () => { + const session = create_test_session(); + let observed_content: string | null = null; + session.on_change(() => { + observed_content = session.get<{ content: string }>(['button_1', 'content']).content; + }); + + set_button_content(session, 'Changed'); + + expect(observed_content).toBe('Changed'); + }); + + it('does not notify for ops-less transactions', () => { + const session = create_test_session(); + const events: ChangeEvent[] = []; + session.on_change((change) => events.push(change)); + + const tr = session.tr; + tr.set_selection({ + type: 'text', + path: ['page_1', 'body', 0, 'title'], + anchor_offset: 0, + focus_offset: 0 + }); + session.apply(tr); + + expect(events.length).toBe(0); + }); + + it('notifies on undo and redo with invertible op pairs', () => { + const session = create_test_session(); + const events: ChangeEvent[] = []; + set_button_content(session, 'Changed'); + session.on_change((change) => events.push(change)); + + session.undo(); + expect(session.get<{ content: string }>(['button_1', 'content']).content).toBe('Get started'); + expect(events.length).toBe(1); + expect(events[0].origin).toBe('undo'); + expect(events[0].ops).toEqual([ + ['set', ['button_1', 'content'], { content: 'Get started', marks: [], annotations: [] }] + ]); + expect(events[0].inverse_ops).toEqual([ + ['set', ['button_1', 'content'], { content: 'Changed', marks: [], annotations: [] }] + ]); + + session.redo(); + expect(session.get<{ content: string }>(['button_1', 'content']).content).toBe('Changed'); + expect(events.length).toBe(2); + expect(events[1].origin).toBe('redo'); + expect(events[1].ops).toEqual([ + ['set', ['button_1', 'content'], { content: 'Changed', marks: [], annotations: [] }] + ]); + }); + + it('notifies once per apply, also when batching merges history entries', () => { + const session = create_test_session(); + const events: ChangeEvent[] = []; + session.on_change((change) => events.push(change)); + + set_button_content(session, 'One', { batch: true }); + set_button_content(session, 'Two', { batch: true }); + + expect(events.length).toBe(2); + expect(session.history.length).toBe(1); + }); + + it('keeps delivered ops stable when later applies batch into the same history entry', () => { + const session = create_test_session(); + const events: ChangeEvent[] = []; + session.on_change((change) => events.push(change)); + + set_button_content(session, 'One', { batch: true }); + const first_ops_count = events[0].ops.length; + set_button_content(session, 'Two', { batch: true }); + + // The first history entry was mutated by batching, but the delivered + // change record must not grow retroactively. + expect(events[0].ops.length).toBe(first_ops_count); + }); + + it('supports unsubscribing', () => { + const session = create_test_session(); + const events: ChangeEvent[] = []; + const unsubscribe = session.on_change((change) => events.push(change)); + + set_button_content(session, 'One'); + unsubscribe(); + set_button_content(session, 'Two'); + + expect(events.length).toBe(1); + }); + + it('isolates listener exceptions from commit bookkeeping', () => { + const session = create_test_session(); + const seen: ChangeEvent[] = []; + session.on_change(() => { + throw new Error('listener boom'); + }); + session.on_change((change) => seen.push(change)); + + expect(() => set_button_content(session, 'Changed')).not.toThrow(); + + // The doc changed, later listeners still ran, and the history entry + // was recorded despite the throwing listener. + expect(session.get<{ content: string }>(['button_1', 'content']).content).toBe('Changed'); + expect(seen.length).toBe(1); + expect(session.history.length).toBe(1); + + session.undo(); + expect(session.get<{ content: string }>(['button_1', 'content']).content).toBe('Get started'); + }); +}); diff --git a/src/test/op_engine_helpers.ts b/src/test/op_engine_helpers.ts new file mode 100644 index 00000000..91f92f6f --- /dev/null +++ b/src/test/op_engine_helpers.ts @@ -0,0 +1,24 @@ +/** + * Shared helpers for the op-engine test suites (commit funnel, splice ops, + * ingest, history). They operate on the fixture document from + * create_test_session.js. + */ + +import type create_test_session from './create_test_session.js'; +import type { Text } from '../lib/types.js'; + +type TestSession = ReturnType; + +export function text_value(content: string): Text { + return { content, marks: [], annotations: [] }; +} + +export function set_button_content( + session: TestSession, + text: string, + { batch = false }: { batch?: boolean } = {} +): void { + const tr = session.tr; + tr.set(['button_1', 'content'], text_value(text)); + session.apply(tr, { batch }); +}