From 6cc95f86bf8ed86e68f3113162548c6dae092229 Mon Sep 17 00:00:00 2001 From: shikokuchuo <53399081+shikokuchuo@users.noreply.github.com> Date: Thu, 28 May 2026 11:21:34 +0100 Subject: [PATCH] hub-client: drop redundant history() walk in attribution incremental path updateRunListAttribution was iterating handle.history() to decide which changes to replay, even though getChanges(workDoc, doc) already returns exactly the new changes in causal order. The history walk added an O(N) topoHistoryTraversal call per debounce cycle (N = total changes in the doc) plus K decodeHeads + Map lookups, all to recompute information getChanges already provides. processedHeads / processedHistoryIndex remain in the state for the cold-start path's contract but are now pure bookkeeping in the incremental path. Adds an invariant test: incremental update must equal a from-scratch buildRunListAttribution on the merged final doc. --- .../src/services/attribution-runs.test.ts | 95 +++++++++++++++++++ hub-client/src/services/attribution-runs.ts | 46 ++++----- 2 files changed, 114 insertions(+), 27 deletions(-) diff --git a/hub-client/src/services/attribution-runs.test.ts b/hub-client/src/services/attribution-runs.test.ts index 3f35673a..ff5f155f 100644 --- a/hub-client/src/services/attribution-runs.test.ts +++ b/hub-client/src/services/attribution-runs.test.ts @@ -11,11 +11,18 @@ */ import { describe, it, expect } from 'vitest'; +import { next as A } from '@automerge/automerge'; +import type { Doc } from '@automerge/automerge'; +import { encodeHeads } from '@automerge/automerge-repo'; +import type { DocHandle } from '@automerge/automerge-repo'; import { buildCharToByteMap, + buildRunListAttribution, runsCharToByteOffsets, + updateRunListAttribution, type AttributionRun, + type ViewableHandle, } from './attribution-runs'; describe('buildCharToByteMap', () => { @@ -75,3 +82,91 @@ describe('runsCharToByteOffsets', () => { expect(out).toEqual(runs); }); }); + +// --------------------------------------------------------------------------- +// Incremental ≡ from-scratch invariant +// --------------------------------------------------------------------------- + +// Anchor for the incremental path: whatever shortcut `updateRunListAttribution` +// uses to skip work, the final run list must agree character-for-character with +// what `buildRunListAttribution` produces from `init()` on the same final doc. +// If a future refactor breaks this — including via something subtle at the +// Automerge boundary (history-traversal ordering, getChanges semantics, etc.) +// — this is the test that should catch it. + +interface TDoc { text: string } + +function fakeHandle(doc: Doc): DocHandle { + const view: ViewableHandle = { + history: () => A.topoHistoryTraversal(doc).map(h => encodeHeads([h])), + metadata: () => undefined, + doc: () => doc, + }; + return view as unknown as DocHandle; +} + +describe('updateRunListAttribution invariant', () => { + it('matches a from-scratch rebuild after a concurrent merge', async () => { + const aliceActor = 'f'.repeat(32); + const bobActor = '0'.repeat(32); + + let alice = A.from({ text: '' }, { actor: aliceActor }); + let bob = A.load(A.save(alice), { actor: bobActor }); + + alice = A.change(alice, d => A.splice(d, ['text'], 0, 0, 'Hello')); + alice = A.change(alice, d => A.splice(d, ['text'], 5, 0, ' World')); + alice = A.change(alice, d => A.splice(d, ['text'], 11, 0, '!')); + + const stateBefore = await buildRunListAttribution(fakeHandle(alice), 'text'); + expect(stateBefore).toBeTruthy(); + + bob = A.change(bob, d => A.splice(d, ['text'], 0, 0, 'X')); + bob = A.change(bob, d => A.splice(d, ['text'], 1, 0, 'Y')); + bob = A.change(bob, d => A.splice(d, ['text'], 2, 0, 'Z')); + alice = A.merge(alice, bob); + + const incremental = updateRunListAttribution(stateBefore!, fakeHandle(alice), 'text'); + const fromScratch = await buildRunListAttribution(fakeHandle(alice), 'text'); + + expect(fromScratch).toBeTruthy(); + expect(incremental.runs).toEqual(fromScratch!.runs); + }); + + it('matches a from-scratch rebuild across interleaved local and merged remote edits', async () => { + const aliceActor = 'f'.repeat(32); + const bobActor = '0'.repeat(32); + + let alice = A.from({ text: '' }, { actor: aliceActor }); + let bob = A.load(A.save(alice), { actor: bobActor }); + + alice = A.change(alice, d => A.splice(d, ['text'], 0, 0, 'A1')); + const stateBefore = await buildRunListAttribution(fakeHandle(alice), 'text'); + expect(stateBefore).toBeTruthy(); + + bob = A.change(bob, d => A.splice(d, ['text'], 0, 0, 'B1')); + bob = A.change(bob, d => A.splice(d, ['text'], 2, 0, 'B2')); + alice = A.merge(alice, bob); + alice = A.change(alice, d => A.splice(d, ['text'], 0, 0, 'A2')); + alice = A.change(alice, d => A.splice(d, ['text'], 0, 0, 'A3')); + + const incremental = updateRunListAttribution(stateBefore!, fakeHandle(alice), 'text'); + const fromScratch = await buildRunListAttribution(fakeHandle(alice), 'text'); + + expect(fromScratch).toBeTruthy(); + expect(incremental.runs).toEqual(fromScratch!.runs); + }); + + it('returns state unchanged when no new changes are present', async () => { + const aliceActor = 'f'.repeat(32); + let alice = A.from({ text: '' }, { actor: aliceActor }); + alice = A.change(alice, d => A.splice(d, ['text'], 0, 0, 'hello')); + + const stateBefore = await buildRunListAttribution(fakeHandle(alice), 'text'); + expect(stateBefore).toBeTruthy(); + + // No edits between build and update — the incremental path should + // short-circuit and return the same runs. + const incremental = updateRunListAttribution(stateBefore!, fakeHandle(alice), 'text'); + expect(incremental.runs).toEqual(stateBefore!.runs); + }); +}); diff --git a/hub-client/src/services/attribution-runs.ts b/hub-client/src/services/attribution-runs.ts index a86ce3ec..569258b9 100644 --- a/hub-client/src/services/attribution-runs.ts +++ b/hub-client/src/services/attribution-runs.ts @@ -26,6 +26,7 @@ import { decodeChange, getAllChanges, getChanges, + getHeads, init, } from '@automerge/automerge'; import type { Change, Doc, Patch } from '@automerge/automerge'; @@ -350,44 +351,35 @@ export function updateRunListAttribution( textFieldName: string, ): RunListAttribution { const viewable = handle as unknown as ViewableHandle; - const history = viewable.history(); - if (!history) throw new HistoryCompactedError(); - if (state.processedHistoryIndex > history.length) throw new HistoryCompactedError(); if (!state._workDoc) throw new HistoryCompactedError(); - if (state.processedHistoryIndex === history.length) { - return state; - } - - // Pull just the new changes (since workDoc's heads), index by hash. const doc = viewable.doc() as Doc; - const newChanges = getChanges(state._workDoc, doc); - const changeByHash = new Map(); - for (const c of newChanges) { - changeByHash.set(decodeChange(c).hash, c); + let newChanges: Change[]; + try { + newChanges = getChanges(state._workDoc, doc); + } catch { + // `getChanges` throws if `oldState` has changes not in `newState`. + // Production state machine never hits that — `_workDoc` only ever + // advances by applying changes from `doc` — but guard against + // hand-constructed state in tests and surface as a cold-rebuild signal. + throw new HistoryCompactedError(); } + if (newChanges.length === 0) return state; const runs = state.runs.map(r => ({ ...r })); - let prevHeads = decodeHeads(state.processedHeads as Parameters[0]); - let lastHeads: unknown[] = state.processedHeads; let workDoc: Doc = state._workDoc; - - for (let i = state.processedHistoryIndex; i < history.length; i++) { - const currHeads = history[i]; - const decodedCurr = decodeHeads(currHeads as Parameters[0]); - const newHash = newChangeHashAt(prevHeads, decodedCurr); - const change = newHash ? changeByHash.get(newHash) : undefined; - if (change) { - workDoc = replayChange(workDoc, change, textFieldName, runs); - } - prevHeads = decodedCurr; - lastHeads = Array.isArray(currHeads) ? currHeads : [currHeads]; + for (const change of newChanges) { + workDoc = replayChange(workDoc, change, textFieldName, runs); } + // `processedHeads` / `processedHistoryIndex` are bookkeeping in the + // incremental path — neither is read here. Updating them keeps the state + // truthful for the cold-start path and for any future consumer. + const history = viewable.history(); return { runs, - processedHeads: lastHeads as unknown[], - processedHistoryIndex: history.length, + processedHeads: getHeads(workDoc), + processedHistoryIndex: history?.length ?? state.processedHistoryIndex, _workDoc: workDoc, }; }