hub-client: drop redundant history() walk in attribution incremental path (#237)

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.

Author: shikokuchuo

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<TDoc>): DocHandle<unknown> {
+  const view: ViewableHandle = {
+    history: () => A.topoHistoryTraversal(doc).map(h => encodeHeads([h])),
+    metadata: () => undefined,
+    doc: () => doc,
+  };
+  return view as unknown as DocHandle<unknown>;
+}
+
+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<TDoc>({ text: '' }, { actor: aliceActor });
+    let bob = A.load<TDoc>(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<TDoc>({ text: '' }, { actor: aliceActor });
+    let bob = A.load<TDoc>(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<TDoc>({ 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);
+  });
+});

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<unknown>;
-  const newChanges = getChanges(state._workDoc, doc);
-  const changeByHash = new Map<string, Change>();
-  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<typeof decodeHeads>[0]);
-  let lastHeads: unknown[] = state.processedHeads;
   let workDoc: Doc<unknown> = state._workDoc;
-
-  for (let i = state.processedHistoryIndex; i < history.length; i++) {
-    const currHeads = history[i];
-    const decodedCurr = decodeHeads(currHeads as Parameters<typeof decodeHeads>[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,
   };
 }