feat(annotator): the command log the document was built for — undo is a pointer swap, and redo never re-runs (#39) - #116
Merged
Conversation
… a pointer swap, and redo never re-runs (#39) The bootstrap's log executed and undid opaque closures over nothing. #40 landed the immutable document it was supposed to hold, so a command is now a one-way transformation and the log keeps the document that was there before it. That closes a live trap rather than merely tidying one: the scaffold called `execute()` again on redo, and a create-command mints a client-side uuid v4, so redo produced a *different* annotation — orphaning the selection that keys on the first id. `apply` now runs exactly once, ever. - Grouping is `composeCommands`, not a begin/commit protocol: a command is already an arbitrary document transformation, and an open transaction spanning pointer events is a state Escape can leave dangling. - `AnnotatorStore` holds the document, its history, the selection beside it and a staging area, behind one subscription with a cached snapshot — the identity `useSyncExternalStore` requires. A drag stages and renders; only pointer-up commits, so the committed document is the same object across the whole gesture. - A throwing subscriber starves none of the others and is not swallowed: core has no logger, so the failures come back as one AggregateError. No Python, no new dependency; openapi.json and the generated client byte-identical. 120 vitest tests, up from 86.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #39. Fourth task of M4 (epic #38), position 4 of 14. It lands after #40 because all three of this issue's acceptance criteria are phrased against "the document", which that task defined.
What was there
core/state/commandLog.tswas the bootstrap scaffold:execute/undo/redoover opaque closures that captured whatever they liked, with one no-op test. #40'sselection.test.tsdrove it through an ad-hocediting()helper and said in a comment that #39 would replace it. This cashes that cheque, and #40's document docstring's too:The trap this closes, which was live rather than hypothetical
redo()calledcommand.execute()again. A drawn annotation gets a client-minted uuid v4 from #40'sIdFactory, so a create-command that mints inside its own body produces a different annotation on redo — leaving the selection (which keys on the first id) pointing at one that no longer exists, and a host's local-id correlation map holding a key nothing matches. Nothing in the repository does this yet, which is exactly why it had to be designed out now instead of found at #43.A command is therefore one-way:
The log entry keeps
beforeandafter; undo and redo are pointer swaps.applynever runs twice, so a command may close over an id factory, a clock or the pointer position without any of it having to be reproducible.test_...redo replays a snapshot and never re-runs the commandasserts the redone annotation carries the same id and thatapplyran exactly once.The rejected alternative, and the single case that decides it
An
apply/invertpair retains no document per entry. But inverting a delete has to restore the annotation's place in the draw order, andaddAnnotationappends — so an inverting delete would have to capture the removed annotations and their positions (a partial snapshot), plus a positional-insert operation the document does not have, to save one pointer. Draw-order equality after a full unwind is asserted in the property test precisely because it is the assertion that design would fail.What the snapshots cost, stated rather than discovered
Consecutive entries share a document (
entries[i].after === entries[i+1].before), so N commands retain N+1. The annotations are shared by reference — only theMapspine is copied — so the cost is ~N × (annotation count) pointer slots: a thousand commands over two hundred annotations is a few megabytes, bounded by the session. No cap is implemented, because nothing has asked for one; the remedies (a bound on entry count, or structural sharing) are named in the docstring so that stays a choice.Grouping is composition, not a protocol
A command is already an arbitrary document transformation, so a group is function composition threaded through the document. It is exception-safe for free:
executeassigns and appends only afterapplyreturns, so a member throwing part of the way through leaves both the document and the log untouched — asserted.Deliberately no
begin()/commit()/abort()pair. An open transaction spanning pointer events is a state an Escape, a thrown error or a lost pointer capture can leave dangling, and nothing needs one: transient drag state lives outside the log entirely. #44's "a whole polygon drawing session is one undo step" is served the same way — the pending points are #42's interaction state, and closing the polygon is oneaddAnnotationCommand.Related, and it falls out of the same shape: a command that returns the document it was given is not history.
removeAnnotations([])reaches this without anybody writing a deliberate no-op, and an undo step that visibly does nothing is the worst thing a history can hold.The store, and criterion 3
AnnotatorStoreholds the document, its history, the selection beside it and the drag that has not happened yet, behind one subscription.stage(project)holds a preview outside the log;renderedis what a canvas paints and follows the pointer, whiledocumentis untouched andcanUndodoes not move.commit(label)turns it into exactly one entry;discard()into none. The criterion-3 test drives ten pointer-moves and asserts the committed document is the same object (toBe) at the end as at the start — reference identity, not resemblance. That is what kills v1's re-render-per-pointer-move.select()notifies and leavescanUndowhere it was, and undoing a delete restores the annotation to the selection for free.Two things designed for a consumer that does not exist yet
The snapshot's identity is load-bearing. #47's React adapter will read this through
useSyncExternalStore, which callsgetSnapshotevery render and compares withObject.is— a store building a fresh object per call re-renders forever. So the snapshot is cached and rebuilt only after something actually changed, pinned by a test, one task before the failure mode would land in somebody else's component.A throwing subscriber starves none of the others, and is not swallowed. The listener set is copied before it is walked (so unsubscribing from inside a listener is safe), failures are collected, and they come back as one
AggregateErroronce everyone has had their turn. This is the kernelInProcessEventBus's log-and-continue posture adapted to a layer that has no logger —consoleis not nameable insidesrc/core/— and in a browser a silently dropped exception is worse than a loud one.Acceptance criteria
store.property.test.ts, six seeds × forty steps of adds, replacements, deletes, nested groups, staged drags and mid-run undo/redo. Two details make it non-vacuous: the expectation is built separately from the same ids and never handed to the store (comparing against the object the run started from would pass by construction under a snapshot history), and it compares an ordered array, because a history restoring the right annotations in the wrong order satisfies a set comparison.commandLog.test.ts's "a group is one step" block, and again inside every property run.toBeassertion described above.The invariant the property test corrected
The first draft asserted that a full redo returns the document the run ended on. It fails on seed 42: a run whose last step was an undo is sitting inside the log, not at its head. The honest invariant is that a full rewind reaches the head of the history, which the test now names and walks to explicitly.
A hand-written PRNG rather than fast-check
fast-checkwould bring shrinking, which is the real thing a property-testing library buys — and it would be the annotator's first test dependency, in a package that ships zero. Sixteen lines ofmulberry32in_random.tsswept over a fixed seed list gets the coverage; what it does not get is a minimal counterexample, so a failure arrives at full length with its seed in the test name and is replayed by running that one case. The trade is recorded in the harness's docstring rather than left implicit. The suite was run five consecutive times, 120/120 each time._random.tsand the new_sample.tscarry the_prefix #40 established, sotsconfig.build.jsonalready excludes them from the shipped engine and from the boundary's type gate, which inherits that list. Verified with--listFiles: the core project compiles ten source files —store.tsandcommands.tsamong them — and neither harness.Ledger
M4's four files:
FORMAT_VERSION11,VERSION0.0.1.dev0,openapi.jsonandfrontend/ui-core/src/generated/api.tsboth byte-identical. No Python was touched at all. No new dependency, runtime or dev; the package still ships zero runtime dependencies. 120 vitest tests, up from 86.AggregateErrorunderlib: ["ES2022"]withtypes: []was verified against the committedtsconfig.core.json, not assumed.Next: #41 — the 2D geometry port, the one M4 task takeable in parallel.