feat(annotator): the document — UUID-keyed, immutable, and a selection that survives undo because it is not in it (#40) - #115
Merged
Conversation
…n that survives undo because it is not in it (#40) `types.ts` ended with the question this task answers: a locally drawn annotation has no server `id`, so how does a draft carry identity between the first click and the response? And #39's every acceptance criterion is phrased against "the document", which did not exist — `commandLog.ts` undid opaque closures over nothing. `AnnotationDocument` is an asset, the schema its labels are judged against, and `ReadonlyMap<string, Annotation>`. A Map because an array means somebody eventually addresses an annotation by index — the epic's named original sin — and because insertion order is the draw order. Every operation returns a new document, which is what makes #39's undo a pointer swap. Not named `Document`: that is a host global, and @types/react declares a stand-in for it, so a core type by that name would shadow it in every adapter. ## Selection is filtered on read, never pruned on write Two decisions, and acceptance criterion 1 falls out of both. Selection lives *beside* the document, so it is never in a command's snapshot — were it a document field, Ctrl+Z after a click would undo the click. And `selectedAnnotations` resolves ids against the document at the moment it is asked, so deleting a selected annotation stops yielding it and **undoing that delete yields it again, still selected, with no bookkeeping anywhere**. An eager prune would have to be undone in step with the command log; this design has no such coordination and needs none. Surviving reordering and edits is the same mechanism. The cost is a set that can carry stale ids — bounded by the session, never wrong because a uuid v4 is not reused. `compactSelection` is offered, and a test states out loud that calling it is what forfeits the undo behaviour above. ## Draft identity, and no rebase A drawn annotation gets a client-minted uuid v4 in the ordinary `id` and a provisional `schema_version` from the schema in hand. `toAnnotationCreate` drops both, so neither guess travels — which is exactly what makes inventing them safe. There is deliberately no `rebaseAnnotationId`: the service mints its own id, and swapping it would move a live selection and a whole undo history under the user to fix a mismatch only the host can see. ## What it refuses, and what it does not Its own invariants only — duplicate id, unknown id, an annotation from another asset. It does not re-check class↔geometry agreement, and there is a proof in this repository that it must not: the round-trip fixture's four annotations all say `label_class: "sign"`, whose class declares `bbox`, while three carry a polygon or a tag. That is valid data the kernel produced. A document enforcing that rule could not load its own fixture — asserted as a test. ## The boundary forced the id port `crypto.randomUUID()` cannot be named inside `src/core/` — #112's gate, one task later, verified as `TS2304`. So `core/ids.ts` declares `IdFactory` and `adapters/ids.ts` implements it, which is the kernel's own hexagonal shape and makes every test deterministic with an injected counter instead of a stubbed global. ## The schema arrives as typed input, proved not asserted `scripts/export_wire_fixtures.py` now exports the three inputs a document is built from — asset, schema, annotations — plus `attribute_kinds`. A hand-written TypeScript schema fixture would have been the second spelling this whole arrangement exists to prevent. The schema carries one class per carryable geometry (so #43-#45 inherit something to draw with) and every optional field in both states. That brought a fourth wire rule: **strictness follows the round-trip, not the type.** Annotations are exact-keyed because the editor hands them back; the schema and the asset are input-only, so `allowExactKeys` applies the wire's own defaults for an absent optional key while still refusing an unknown one. And `parseAssetDescriptor` refuses a null `width`/`height` — `AssetOut` declares both nullable for pre-pipeline rows, and geometry here is native pixels, so an unmeasured asset has no frame to be native to. `tsconfig.build.json` now also excludes `src/**/_*.ts`, the `tests/server/_flow.py` convention, which is what keeps the new `_fixture.ts` harness and its `node:fs` import out of the shipped engine and out of the boundary gate. Ledger, M4's four files: FORMAT_VERSION 11, VERSION 0.0.1.dev0, `openapi.json` and the generated client both byte-identical. No route, service, model or error touched. 86 vitest tests, up from 30; 1924 Python, up from 1919; no new dependency, and the package still ships zero runtime dependencies. Closes #40
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 #40. Third task of M4 (epic #38), position 3 of 14. It lands before #39 because all three of that issue's acceptance criteria are phrased against "the document" — which did not exist.
What was missing
core/state/commandLog.tsexecuted and undid opaque closures over nothing, andcore/types.tsended its own docstring with the question:The document
A
ReadonlyMap, not an array and not aRecord: an array means somebody eventually addresses an annotation by index — the epic's named "original sin" from v1 — while a Map gives O(1) lookup by id and insertion-ordered iteration, which is the draw order. Every operation returns a new document and mutates nothing, which is what lets #39 snapshot by reference and makes undo a pointer swap.Not named
Document. That is a host global, and@types/react/global.d.tsdeclares a stand-in for it, so a core type by that name would shadow it in every adapter and every consumer — the class of confusion #112 exists to prevent.Selection is filtered on READ, never pruned on write
Two decisions, and acceptance criterion 1 falls out of both.
Ctrl+Zafter a click would undo the click, and the log would fill with entries for looking at things. The kernel has no notion of selection either.selectedAnnotationsresolves ids against the document at the moment it is asked. So deleting a selected annotation stops it being yielded, and undoing that delete yields it again, still selected, with no bookkeeping anywhere. An eager prune would have to be undone in step with the command log — coordination this design does not have and does not need.Surviving reordering and edits is the same mechanism: it keys on ids, so it cannot notice iteration order, and
replaceAnnotationkeeps an annotation's place (Map.seton a present key does not move it — pinned by a test, because an editor where editing changes z-order fights its user).The cost is a set that may carry stale ids: bounded by the session, invisible, never wrong because a uuid v4 is not reused.
compactSelectionis offered for a host that wants the hygiene, and a test states out loud that calling it is exactly what forfeits the undo behaviour above — so the trade-off is not discovered by surprise.The undo/redo tests drive the real
CommandLog, not a simulation, because the claim is about how the two pieces sit together. #39 will replace the ad-hoc closures with real commands; the asserted behaviour is what it has to keep.Draft identity, and why there is no rebase
id— the document's key, the selection's key, and what a renderer keys elements by.schema_versionis provisional, from the schema in hand (which is why the schema mirror keepsversion).toAnnotationCreatedrops both, so neither guess ever travels — precisely what makes inventing them safe.rebaseAnnotationId. The service mints its own id on POST, so swapping the local one would move a live selection and a whole undo history out from under the user, to fix a mismatch only the host can see. The engine is the session's source of truth; a host correlates with aMap<localId, AnnotationCreate>where it builds the payload. That is the epic's "annotations leave by events" read literally.What it refuses, and the proof that it must not refuse more
Its own invariants only: a duplicate id, an unknown id on replace/delete, an annotation belonging to another asset.
It does not re-check class↔geometry agreement, required attributes, or geometry bounds — those have an owner in the kernel and
wire.ts's rule 2 already argued the copies would drift. There is a concrete proof in this repository: the round-trip fixture's four annotations all carrylabel_class: "sign", whose class declaresbbox, while three of them carry a polygon or a tag. That is valid wire data the kernel produced. A document enforcing that rule could not load its own fixture — asserted as a test rather than left as prose. What it offers instead isclassNamed, so the tools (#43–#45) refuse at draw time where a user can be told.#112's boundary forced the id port, one task later
crypto.randomUUID()cannot be named insidesrc/core/— verified asTS2304: Cannot find name 'crypto'against the committedtsconfig.core.json. Socore/ids.tsdeclarestype IdFactory = () => stringandadapters/ids.tsimplements it withcrypto.randomUUID(). That is the kernel's own hexagonal shape, and it makes every test deterministic with an injected counter rather than a stubbed global. The boundary being productive, not obstructive.The schema arrives as typed input, and it is proved rather than asserted
scripts/export_wire_fixtures.pynow exports the three inputs a document is built from —asset,schema,annotations— plus anattribute_kindsvocabulary list ongeometry_types' terms. A hand-written TypeScript schema fixture would have been the second spelling this whole arrangement exists to prevent.The exported schema carries one class per carryable geometry (so #43–#45 each inherit something to draw with) and every optional field in both states — a populated class with a
selectattribute carrying options and a default, one withcolor: nulland no attributes, and one whose attribute has every optional at its default. A mirror only ever handed values leaves the null branch unparsed.LabelClass.coloralready existed in the kernel, so "classes with colors" needed no client-side invention.A fourth wire rule: strictness follows the round-trip, not the type
Annotations are exact-keyed because the editor hands them back — a key it dropped is a field the kernel wrote and the editor erased. The schema and the asset are input-only, so that argument does not transfer, and
color/attributes/required/options/defaultall carry defaults which #27's rule emits as optional.allowExactKeysapplies the wire's own defaults for an absent optional key while still refusing an unknown one — a second function, deliberately, rather thanrequireExactKeysloosened.parseAssetDescriptorrefuses an unmeasured assetAssetOut.width/heightareint | Nonebecause a pre-pipeline row has never been probed. There is no honest default: geometry here is native pixels and never normalized, so an asset with no known frame has nothing to be native to, and guessing would produce coordinates that are in range and uniformly wrong.AssetDescriptor's docstring names the same trapget_asset_imagepublishes four numbers to avoid — the screen↔image transform is #47's, and the document only ever holds native pixels.tsconfig.build.jsonalso excludessrc/**/_*.tsThe
tests/server/_flow.pyconvention, arriving on the TypeScript side for the new_fixture.tsloader. It is what keeps anode:fsimport out of the shipped engine and out of the headless boundary's type gate, which inherits this exclude list — verified with--listFiles: the core project sees eight source files and no harness.Ledger
M4's four files:
FORMAT_VERSION11,VERSION0.0.1.dev0,openapi.jsonandfrontend/ui-core/src/generated/api.tsboth byte-identical. No route, service, model or error touched — the only Python edits are the fixture exporter and its gate.tests/fixtures/wire_annotations.jsonregenerated (committed artifact, gated both ways). 86 vitest tests, up from 30; 1924 Python, up from 1919; no new dependency, and the package still ships zero runtime dependencies.Acceptance criteria
selection.test.tsdriving the realCommandLog, including the delete→undo restoration and a full unwind of a three-command run.document.test.ts's "built from the wire, and back again" block loads the kernel's own fixture throughdocumentFromWireand serializes every annotation back to the bytes it came from, all three variants included.