Skip to content

Route every document mutation through a commit() helper#64

Merged
kmatzen merged 1 commit into
mainfrom
commit-helper-refactor
Jul 25, 2026
Merged

Route every document mutation through a commit() helper#64
kmatzen merged 1 commit into
mainfrom
commit-helper-refactor

Conversation

@kmatzen

@kmatzen kmatzen commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Completes #54. Stacked on #58 — that PR fixed the instance, this removes the bug class. Base is fix-toggle-node-history, so as with #59 and #60 there will be no CI until #58 merges and GitHub retargets this.

The problem

The history ritual — slice off redo entries, push a deep clone, advance the index — was open-coded at seventeen sites:

const newHistory = state.history.slice(0, state.historyIndex + 1);
newHistory.push(cloneTree(newTree));
set({ tree: newTree, history: newHistory, historyIndex: newHistory.length - 1 });

toggleNode simply omitted it. That's the bug #58 fixed: tree and history[historyIndex] diverged, so an undo discarded one more edit than the user asked for. Seventeen copies of the same four lines is an invitation to write an eighteenth that forgets.

The change

function commit(state, tree, extra = {}): Partial<ModelerState> {
  const history = state.history.slice(0, state.historyIndex + 1);
  history.push(tree ? cloneTree(tree) : null);
  return { ...extra, tree, history, historyIndex: history.length - 1 };
}

Every mutating action becomes set(commit(get(), newTree, { ...view state })). extra carries selection and expansion, so no action touches history or historyIndex directly — which is exactly how toggleNode came to skip them.

addNodeFromData already had a local commit() doing this plus its own expansion bookkeeping — the duplication had been noticed once and solved in one place. I renamed it place() and had it delegate to the module helper; keeping the name would have shadowed it and silently left that action on the old path.

Verification

check result
tsc --noEmit exit 0
store suite 28/28 pass
newHistory references remaining 0

The 23 pre-existing tests are the real check here. Behaviour should be identical, and they are what would catch drift. The refactor was applied by a script performing 25 exact-anchor replacements, each asserted present before substitution — so a missed site fails loudly rather than silently. But anchors only prove I edited what I intended, not that semantics are preserved; the tests are what establish that.

No discrimination check applies — this is a pure refactor with no behaviour change to detect.

One sharp edge

commit spreads extra before the history fields, so a caller passing tree or history inside extra would have it silently overridden. The ordering is deliberate — history bookkeeping should win — and no current caller does it, but it is worth knowing before someone tries.

🤖 Generated with Claude Code

@kmatzen
kmatzen force-pushed the fix-toggle-node-history branch from 4a8e0dd to 201b479 Compare July 25, 2026 16:38
@kmatzen
kmatzen force-pushed the commit-helper-refactor branch from 22b16fb to 2967fa2 Compare July 25, 2026 16:38
@kmatzen
kmatzen force-pushed the fix-toggle-node-history branch from 201b479 to 620f30e Compare July 25, 2026 19:20
@kmatzen
kmatzen changed the base branch from fix-toggle-node-history to main July 25, 2026 19:22
Completes #54. #58 fixed the instance; this removes the bug class.

The history ritual -- slice off redo entries, push a deep clone, advance the
index -- was open-coded at seventeen sites. toggleNode simply omitted it,
which is the bug #58 fixed: `tree` and `history[historyIndex]` diverged, so an
undo discarded one more edit than the user asked for. Seventeen copies of the
same four lines is an invitation to write a sixteenth that forgets.

commit(state, tree, extra) now builds the whole state patch. `extra` carries
whatever view state the action also sets (selection, expansion), so no action
touches history or historyIndex directly.

addNodeFromData already had a local commit() doing this plus its own expansion
bookkeeping -- the duplication had been noticed once and solved in one place.
Renamed to place() and delegated to the module helper, since keeping the name
would have shadowed it and silently left that action on the old path.

Behaviour is unchanged: all 28 store tests pass, including the 23 that predate
this work and the 5 added with #58.

One sharp edge worth knowing: commit spreads `extra` before the history
fields, so a caller passing `tree` or `history` in `extra` has it overridden.
That ordering is deliberate -- history bookkeeping should win -- and no
current caller does it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kmatzen
kmatzen force-pushed the commit-helper-refactor branch from 2967fa2 to 805fdbc Compare July 25, 2026 19:22
@github-actions

Copy link
Copy Markdown

🚀 Preview deployed: https://commit-helper-refactor.sinter.pages.dev

(updates on every push to this PR)

@kmatzen
kmatzen merged commit 4737458 into main Jul 25, 2026
5 checks passed
@kmatzen
kmatzen deleted the commit-helper-refactor branch July 25, 2026 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant