From cc93715c03da3a2bb3e6acd722e3a9a1b53dcc79 Mon Sep 17 00:00:00 2001 From: robonen Date: Tue, 11 Aug 2026 03:04:12 +0700 Subject: [PATCH] fix(writekit): attr coercion stops erasing data, validate runs, undo coalesces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes driven by building a real consumer (cyrille studio) on 0.0.1, each pinned by tests: - `coerceAttrs` treated the spec as a whitelist: any attribute a block definition did not declare was silently deleted by the first `normalizeDocument` pass — and since normalization runs on load and consumers autosave, the erasure wrote itself back to storage. Coercion now fills defaults and keeps unknown keys verbatim. Parse rules build attrs explicitly, so pasted markup cannot smuggle keys through this path; the CRDT never calls coercion, so replica semantics are unchanged. - `AttrSpec.validate` was consulted only by `validateDocument`, which nothing in the library calls — it looked like enforcement and was inert. A provided value failing `validate` now falls back to the declared default, deterministically (CRDT-safe given one spec) and loudly in dev. - Undo recorded one entry per transaction — one keystroke per Ctrl+Z, and 200 keystrokes evicted the entire earlier history. Plain typing in one block now coalesces within a 500ms window by concatenation, which preserves the replay invariant (`inverted` stays in application order, replayed reversed), counts as ONE entry against maxSize, and never merges across blocks, structural changes, or a foreign transaction (remote setDoc, undo/redo, selection-only moves interrupt the chain). `coalesceMs: 0` opts out. Also: `component` in a block definition may now be a lazy loader (`() => import('./Card.vue')`) — a registry imported for its schema (codecs, tests, server-side normalizers) then carries no view graph; the view wraps the loader in a cached async component on first render. Co-Authored-By: Claude Fable 5 --- vue/writekit/src/registry/define-block.ts | 10 +- .../src/schema/__test__/coerce.test.ts | 76 ++++++++++ vue/writekit/src/schema/schema.ts | 50 +++++-- .../src/state/__test__/history.test.ts | 132 ++++++++++++++++++ vue/writekit/src/state/history.ts | 79 ++++++++++- vue/writekit/src/state/writekit.ts | 5 + vue/writekit/src/view/BlockView.vue | 30 +++- 7 files changed, 367 insertions(+), 15 deletions(-) create mode 100644 vue/writekit/src/schema/__test__/coerce.test.ts create mode 100644 vue/writekit/src/state/__test__/history.test.ts diff --git a/vue/writekit/src/registry/define-block.ts b/vue/writekit/src/registry/define-block.ts index d395745..538cd1a 100644 --- a/vue/writekit/src/registry/define-block.ts +++ b/vue/writekit/src/registry/define-block.ts @@ -4,6 +4,9 @@ import type { NodeSpec } from '../schema'; import type { CommandFactory } from '../state/command'; import type { InputRuleSpec } from './input-rule'; +/** A lazy block component: resolved by the view on first render. */ +export type BlockComponentLoader = () => Promise; + /** Props passed to an atom/void block's Vue `component`. */ export interface BlockComponentProps { /** The block's model node (read its `attrs`). */ @@ -36,11 +39,16 @@ export interface BlockBehavior { * A block definition: schema contribution + behavior + an opaque Vue component. * Non-view layers treat `component` as an opaque value; only the view resolves * it. The type is `Component` purely for authoring ergonomics (type-only import). + * + * `component` may be a lazy loader (`() => import('./Card.vue')`): a registry + * imported for its SCHEMA — a codec, a test, a server-side normalizer — then + * carries no view graph at all, and the view resolves the loader on first + * render exactly like any async component. */ export interface BlockDefinition { readonly type: string; readonly spec: NodeSpec; - readonly component?: Component; + readonly component?: Component | BlockComponentLoader; readonly meta?: BlockMeta; readonly behavior?: BlockBehavior; readonly commands?: Record; diff --git a/vue/writekit/src/schema/__test__/coerce.test.ts b/vue/writekit/src/schema/__test__/coerce.test.ts new file mode 100644 index 0000000..4bd2f60 --- /dev/null +++ b/vue/writekit/src/schema/__test__/coerce.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it, vi } from 'vitest'; +import { normalizeDocument } from '../normalize'; +import { createSchema } from '../schema'; + +const schema = createSchema({ + nodes: new Map([ + ['paragraph', { + content: { kind: 'text' as const }, + attrs: { + condition: { default: null }, + level: { default: 1, validate: (v: unknown) => typeof v === 'number' && v >= 1 && v <= 6 }, + }, + }], + ['bare', { content: { kind: 'atom' as const } }], + ]), + marks: new Map(), +}); + +describe('attr coercion', () => { + it('keeps unknown attributes instead of erasing them', () => { + // Coercion is not a whitelist: a document must round-trip through an + // editor whose schema does not know every field — dropping them silently + // erased consumer data, and the loss was autosaved before anyone saw it. + const attrs = schema.coerceAttrs('paragraph', { + condition: { op: 'flag', key: 'met' }, + futureField: 'still here', + }); + + expect(attrs.futureField).toBe('still here'); + expect(attrs.condition).toEqual({ op: 'flag', key: 'met' }); + expect(attrs.level).toBe(1); + }); + + it('keeps attrs even when the spec declares none', () => { + expect(schema.coerceAttrs('bare', { anything: 1 })).toEqual({ anything: 1 }); + }); + + it('runs validate and falls back to the default on a rejected value', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + // `validate` looked like enforcement and never ran; an out-of-range level + // normalized cleanly and rendered . + const attrs = schema.coerceAttrs('paragraph', { level: 99 }); + + expect(attrs.level).toBe(1); + expect(warn).toHaveBeenCalledOnce(); + warn.mockRestore(); + }); + + it('accepts a value validate approves', () => { + expect(schema.coerceAttrs('paragraph', { level: 3 }).level).toBe(3); + }); + + it('is idempotent — a second pass changes nothing', () => { + const once = schema.coerceAttrs('paragraph', { level: 2, custom: [1, 2] }); + const twice = schema.coerceAttrs('paragraph', once); + + expect(twice).toEqual(once); + }); + + it('carries unknown attrs through normalizeDocument', () => { + const doc = { + content: [{ + id: 'b1', + type: 'paragraph', + attrs: { condition: { op: 'flag', key: 'met' }, futureField: true }, + content: [{ text: 'hi', marks: [] }], + }], + }; + + const normalized = normalizeDocument(doc as never, schema); + + expect(normalized.content[0]!.attrs.futureField).toBe(true); + expect(normalized.content[0]!.attrs.condition).toEqual({ op: 'flag', key: 'met' }); + }); +}); diff --git a/vue/writekit/src/schema/schema.ts b/vue/writekit/src/schema/schema.ts index 3abaade..0175918 100644 --- a/vue/writekit/src/schema/schema.ts +++ b/vue/writekit/src/schema/schema.ts @@ -14,27 +14,61 @@ export interface Schema { markSpec: (type: string) => MarkSpec | undefined; /** Default attrs for a block type (all defaults applied). */ defaultAttrs: (type: string) => Attrs; - /** Fill defaults and drop unknown keys for a block type. */ + /** Fill defaults, run `validate`, keep unknown keys for a block type. */ coerceAttrs: (type: string, attrs?: Attrs) => Attrs; /** Default attrs for a mark type. */ defaultMarkAttrs: (type: string) => Attrs; - /** Fill defaults and drop unknown keys for a mark type. */ + /** Fill defaults, run `validate`, keep unknown keys for a mark type. */ coerceMarkAttrs: (type: string, attrs?: Attrs) => Attrs; } +/** + * Coercion fills defaults and enforces `validate`; it is NOT a whitelist. + * + * Unknown keys pass through verbatim: a document round-tripping through the + * editor must never lose fields this schema version does not know about — + * dropping them silently erased consumer data (a `condition` attribute the + * spec forgot to declare disappeared on the first normalization pass and the + * loss was autosaved). Parse rules build attrs explicitly, so pasted markup + * cannot smuggle arbitrary keys through this path. + * + * A provided value failing its `validate` falls back to the declared default: + * deterministic for CRDT replicas (given one spec), loud in dev, and never a + * silently-kept invalid value. + */ function coerceWithSpec(spec: AttrsSpec | undefined, attrs?: Attrs): Attrs { - if (!spec) - return {}; + if (!spec) { + return attrs ? { ...attrs } : {}; + } const result: Record = {}; + if (attrs) { + for (const key in attrs) { + if (attrs[key] !== undefined && !(key in spec)) + result[key] = attrs[key]!; + } + } + for (const key in spec) { + const attr = spec[key]!; const provided = attrs?.[key]; - if (provided !== undefined) - result[key] = provided; - else if (spec[key]!.default !== undefined) - result[key] = spec[key]!.default!; + if (provided !== undefined) { + if (attr.validate && !attr.validate(provided)) { + if (__DEV__) + console.warn(`[writekit] Attr "${key}" rejected by validate(); falling back to its default.`, provided); + + if (attr.default !== undefined) + result[key] = attr.default; + } + else { + result[key] = provided; + } + } + else if (attr.default !== undefined) { + result[key] = attr.default; + } } return result; diff --git a/vue/writekit/src/state/__test__/history.test.ts b/vue/writekit/src/state/__test__/history.test.ts new file mode 100644 index 0000000..8c1db13 --- /dev/null +++ b/vue/writekit/src/state/__test__/history.test.ts @@ -0,0 +1,132 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { HistoryEntry } from '../history'; +import type { Step } from '../step'; +import { createHistory } from '../history'; + +const caret = { type: 'text', anchor: { blockId: 'b1', offset: 0 }, focus: { blockId: 'b1', offset: 0 } } as never; + +function typing(blockId: string, text: string): HistoryEntry { + return { + steps: [{ type: 'insertInline', blockId, offset: 0, content: [{ text, marks: [] }] } as Step], + inverted: [{ type: 'deleteText', blockId, from: 0, to: text.length } as Step], + selectionBefore: caret, + selectionAfter: caret, + }; +} + +function structural(blockId: string): HistoryEntry { + return { + steps: [{ type: 'removeBlock', blockId } as Step], + inverted: [{ type: 'insertBlock', node: { id: blockId, type: 'paragraph', attrs: {}, content: [] }, index: 0 } as never], + selectionBefore: caret, + selectionAfter: caret, + }; +} + +beforeEach(() => vi.useFakeTimers()); +afterEach(() => vi.useRealTimers()); + +describe('history coalescing', () => { + it('merges a typing burst in one block into one undo press', () => { + const history = createHistory(); + + for (const ch of ['h', 'e', 'l', 'l', 'o']) { + history.record(typing('b1', ch)); + vi.advanceTimersByTime(100); + } + + const entry = history.undo()!; + + expect(entry.steps).toHaveLength(5); + expect(history.canUndo()).toBe(false); + }); + + it('keeps the replay order: later keystrokes undo first', () => { + const history = createHistory(); + + history.record(typing('b1', 'a')); + history.record(typing('b1', 'b')); + + const entry = history.undo()!; + + // `inverted` stays in application order; undo replays it reversed, so the + // inverse of "b" must sit AFTER the inverse of "a". + expect(entry.inverted.map(step => (step as { to: number }).to)).toEqual([1, 1]); + expect(entry.steps.map(step => (step as { content: Array<{ text: string }> }).content[0]!.text)).toEqual(['a', 'b']); + }); + + it('starts a new group after the time window', () => { + const history = createHistory({ coalesceMs: 500 }); + + history.record(typing('b1', 'a')); + vi.advanceTimersByTime(600); + history.record(typing('b1', 'b')); + + history.undo(); + + expect(history.canUndo()).toBe(true); + }); + + it('never merges across blocks', () => { + const history = createHistory(); + + history.record(typing('b1', 'a')); + history.record(typing('b2', 'b')); + + history.undo(); + + expect(history.canUndo()).toBe(true); + }); + + it('never merges structural changes', () => { + const history = createHistory(); + + history.record(typing('b1', 'a')); + history.record(structural('b1')); + history.record(typing('b1', 'b')); + + expect(history.undo()!.steps).toHaveLength(1); + expect(history.undo()!.steps).toHaveLength(1); + expect(history.undo()!.steps).toHaveLength(1); + }); + + it('breaks the chain on interrupt — a foreign transaction is a boundary', () => { + // A remote setDoc or an undo between keystrokes must not be spliced into + // one undo press with them. + const history = createHistory(); + + history.record(typing('b1', 'a')); + history.interrupt(); + history.record(typing('b1', 'b')); + + history.undo(); + + expect(history.canUndo()).toBe(true); + }); + + it('counts groups, not keystrokes, against maxSize', () => { + const history = createHistory({ maxSize: 2 }); + + // Two bursts of three keystrokes: two groups — both must survive. + for (const ch of ['a', 'b', 'c']) + history.record(typing('b1', ch)); + vi.advanceTimersByTime(1000); + for (const ch of ['d', 'e', 'f']) + history.record(typing('b1', ch)); + + expect(history.undo()!.steps).toHaveLength(3); + expect(history.undo()!.steps).toHaveLength(3); + expect(history.canUndo()).toBe(false); + }); + + it('can be disabled outright', () => { + const history = createHistory({ coalesceMs: 0 }); + + history.record(typing('b1', 'a')); + history.record(typing('b1', 'b')); + + history.undo(); + + expect(history.canUndo()).toBe(true); + }); +}); diff --git a/vue/writekit/src/state/history.ts b/vue/writekit/src/state/history.ts index 395e11e..cddfd3d 100644 --- a/vue/writekit/src/state/history.ts +++ b/vue/writekit/src/state/history.ts @@ -15,6 +15,14 @@ export interface HistoryEntry { export interface HistoryOptions { /** Maximum number of undo entries to retain (default 200). */ readonly maxSize?: number; + /** + * Coalesce a new entry into the previous one when both are plain typing in + * the same block and land within this window (ms). One keystroke per + * transaction otherwise makes Ctrl+Z a character-by-character crawl, and a + * short paragraph evicts the whole earlier history through `maxSize`. + * `0` disables coalescing. @default 500 + */ + readonly coalesceMs?: number; } /** @@ -26,6 +34,13 @@ export interface HistoryOptions { export interface History { /** Record a new edit, clearing the redo stack. */ record: (entry: HistoryEntry) => void; + /** + * Break the coalescing chain: the next recorded entry starts its own group. + * Called for anything that lands between recordings (a remote change, an + * undo/redo, a selection jump) — merging across such a boundary would splice + * foreign state into one undo press. + */ + interrupt: () => void; /** Pop the latest undo entry (and push it onto the redo stack). */ undo: () => HistoryEntry | undefined; /** Pop the latest redo entry (and push it back onto the undo stack). */ @@ -35,18 +50,75 @@ export interface History { clear: () => void; } +/** Plain typing: text-only steps confined to a single block. */ +function typingBlockOf(steps: readonly Step[]): string | null { + let block: string | null = null; + + for (const step of steps) { + if (step.type !== 'insertInline' && step.type !== 'deleteText' && step.type !== 'replaceInline') + return null; + + if (block === null) + block = step.blockId; + else if (block !== step.blockId) + return null; + } + + return block; +} + export function createHistory(options: HistoryOptions = {}): History { const maxSize = options.maxSize ?? 200; + const coalesceMs = options.coalesceMs ?? 500; const undoStack: HistoryEntry[] = []; const redoStack: HistoryEntry[] = []; + let lastRecordAt = 0; + let lastTypingBlock: string | null = null; + + /** + * Concatenation preserves the replay invariant: `inverted` is stored in + * application order and replayed reversed, so a merged entry undoes the + * later keystrokes first — exactly as separate entries would, in one press. + */ + function coalesce(top: HistoryEntry, entry: HistoryEntry): HistoryEntry { + return { + steps: [...top.steps, ...entry.steps], + inverted: [...top.inverted, ...entry.inverted], + selectionBefore: top.selectionBefore, + selectionAfter: entry.selectionAfter, + }; + } + return { record(entry) { - undoStack.push(entry); - if (undoStack.length > maxSize) - undoStack.shift(); + const now = Date.now(); + const block = typingBlockOf(entry.steps); + const top = undoStack[undoStack.length - 1]; + + const mergeable + = coalesceMs > 0 + && top !== undefined + && block !== null + && block === lastTypingBlock + && now - lastRecordAt <= coalesceMs; + + if (mergeable) { + undoStack[undoStack.length - 1] = coalesce(top, entry); + } + else { + undoStack.push(entry); + if (undoStack.length > maxSize) + undoStack.shift(); + } + + lastRecordAt = now; + lastTypingBlock = block; redoStack.length = 0; }, + interrupt() { + lastTypingBlock = null; + }, undo() { const entry = undoStack.pop(); if (entry) @@ -64,6 +136,7 @@ export function createHistory(options: HistoryOptions = {}): History { clear() { undoStack.length = 0; redoStack.length = 0; + lastTypingBlock = null; }, }; } diff --git a/vue/writekit/src/state/writekit.ts b/vue/writekit/src/state/writekit.ts index 44ab7f0..72c9195 100644 --- a/vue/writekit/src/state/writekit.ts +++ b/vue/writekit/src/state/writekit.ts @@ -63,6 +63,11 @@ export function createWritekit(options: CreateWritekitOptions): Writekit { selectionAfter: next.selection, }); } + else { + // Anything that lands between recordings — a remote setDoc, undo/redo, a + // selection-only move — is a boundary the coalescer must not merge over. + history.interrupt(); + } bus.emit('transaction', tr, next, prev); if (next.doc !== prev.doc) diff --git a/vue/writekit/src/view/BlockView.vue b/vue/writekit/src/view/BlockView.vue index c63d5ab..33e21c2 100644 --- a/vue/writekit/src/view/BlockView.vue +++ b/vue/writekit/src/view/BlockView.vue @@ -3,8 +3,9 @@ import type { Attrs, Node } from '../model';