Skip to content

feat(annotator): one dialog session is one version, a pin that answers, and a grouped ledger (WS4) (#368) - #379

Merged
JArmandoAnaya merged 5 commits into
mainfrom
feat/368-dialog-sessions
Aug 6, 2026
Merged

feat(annotator): one dialog session is one version, a pin that answers, and a grouped ledger (WS4) (#368)#379
JArmandoAnaya merged 5 commits into
mainfrom
feat/368-dialog-sessions

Conversation

@JArmandoAnaya

Copy link
Copy Markdown
Contributor

WS4 of the annotation workspace redesign, and the last of the four. Closes #368.

Three deliverables and one prefill, plus a defect the tests exposed.

What changed

  • AddClassDialog is a session. Create and add another (⌘↵) banks the class and clears the form; the primary publishes everything banked plus whatever is still in the form, as one schema version. Banked classes show as removable chips, the auto-written description names them all (Added classes "cone", "barrier" and "crossing" from the annotation view), and the primary says how many it will publish. The collision check now covers the session as well as the published version, because create_version judges the whole contract and a 409 after the save is the worst place to learn about a name typed twice — the two cases get different sentences, since one means "pick the class that exists" and the other means "you already added that".

    Cancelling with classes banked asks, and the ask lives in the close handler rather than on the Cancel button: Radix routes Escape and the overlay through onOpenChange, so a guard the button owned would protect one of three ways out. It is the only question this dialog asks — everything a session holds lives in the browser, so closing loses exactly what somebody typed and nothing else.

    Unchanged, deliberately: the save → publish → repin order and its test, the F23 canRepin preflight, the two-word button (Add class / Publish without re-pinning), the partial-failure table in the module docstring, and the DESTRUCTIVE_SCHEMA_CHANGE refusal that names the Schema tab. runAddClass took declared: LabelClassBody and now takes added: readonly LabelClassBody[]; nothing else about it moved.

  • The v{n} badge answers the question it raises. Pressing it opens a panel saying whether the batch's pin is still the project's current version and, when it is not, what arrived since — the kernel's own words for each change, the same payload the Schema tab's ledger renders. Nothing is fetched until it is opened: useActiveSchema by its enabled, the comparison by being handed null bounds. That is the rule e2e/annotate.spec.ts pins from the other side — a page that read the active version on arrival would be one refactor from offering classes the API then refuses.

    A hand-built disclosure rather than Radix's Popover, for the reason Combobox.tsx already writes down: a Popover owns focus on open and restores it on close, and the annotator reads the keyboard off its own root.

  • The version history groups what the annotator published. Consecutive versions whose provenance is annotation collapse into one expandable row — v2–v3, how many, when the run ended, and the contract it left behind — so the curated milestones somebody opened the ledger to read are not buried under a run of Added class "cone" from the annotation view. curated and null always render individually; a run of one is not a run. Expanding gives back exactly the rows a flat table would have had, with the same data-testids.

    The rule is screens/schemaHistory.ts, a pure function with its own boundary tests, because every way it goes wrong is at an edge and none of them is a thing a rendering test isolates.

  • The create row's typed name reaches the dialog. ClassField has handed it over since WS2 and the page dropped it, because AddClassDialog had nowhere to put it. The tool strip's + still opens empty — that press means "I want a class", not a particular one.

Found in scope, and fixed

#233's "you are now drawing with the class you just made" had never worked. activeClass was Workspace state, Workspace is keyed on the asset, and JobScreen returns LoadingState whenever any of its four queries is pending — while usePinnedSchema's query key names the version. So the re-pin at the end of the chain moved the key, the page fell through to the loading state, the component holding the freshly-armed class unmounted, and the class field read Select again a moment later. No error, no refusal, nothing on screen to notice.

The state moved up to JobScreen, beside the clipboard, which lives there for the same reason one frame over (#123). Deliberate consequence: the drawing class now also survives moving to the next frame, where it used to reset — which is what somebody labelling one class across a clip wants, and is the same scope a paste already has. It still stops at the job's edge.

Also fixed, because it blocked this PR's own deliverable: the class field's create row was spread in only when onOpenGallery was supplied. Nothing about knowing where the gallery is bears on whether a class can be created, and the tool strip's + was never gated on it — so the row was absent for exactly the callers whose + still worked, and the prefill would have been unreachable behind it.

Deviations, stated rather than silent

  1. The grouping landed in ProjectScreen's VersionHistory table, not in SchemaEditor's VersionNavigator. Annotation workspace redesign — class system, top bar, objects panel #368 §WS4 names the latter; decision 7 says "collapses consecutive annotation runs into one expandable row" and the issue's own screenshot note points at "the flat Version history table (Version / Published / Why / Classes, one row per version)". VersionNavigator is a <Select> whose items are options, not rows, and it is the reader — one version at a time with its diff — where every version must stay individually selectable; grouping it would remove the ability to read one. The ledger is what decision 7 describes, so that is what grouped. The rule is exported from its own module, so pointing a second surface at it is one import.
  2. no-repin-notice's subject is the whole session, not the form field. The mechanism — canRepin, the two sentences, the testid, the trigger — is untouched; but by the time somebody presses, the field is usually empty and the classes are banked, so “this class” would have named nothing at all.
  3. frontend/app/cycle/cycle.spec.ts is in the diff for four lines: the pin popover is the one WS4 surface with a real project behind it, and the demo has none. The session flow stays in ui-core, where the assertions are about the request body that actually leaves.

Test plan

621 ui-core vitest (up from 566), 820 annotator unchanged, 198 e2e unchanged, the real-server cycle.

Full local gate, in stages under the harness's ~10-minute ceiling. Every exit code:

Stage Exit
pytest tests/kernel 0
pytest tests/server tests/cli tests/mcp 0
pytest tests/architecture tests/examples tests/formats tests/jobs tests/packaging tests/scripts tests/test_versioning.py 0
ruff check . / ruff format --check . 0 / 0
mypy src/visionset — 133 files 0
lint-imports — 3 contracts kept 0
pnpm -r build 0
pnpm -r lint 0
pnpm -r test 0
pnpm test:scripts 0
check.sh generated 0
check.sh browsere2e (chromium) 198 in 2.2m, browser cycle, real server in 30s 0

The pytest split was derived from ls tests/ at run time. No Python source changed; the backend stages are the regression check.

Twenty-three mutations, each on an anchor asserted unique before and after, each reverted by its exact inverse. Twenty turned a named test red on the first attempt. Three did not, and each was a real hole in a test rather than a mutation not worth making — all three are fixed in their own commits and re-verified:

# Mutation Caught by
1 banking replaces instead of appending survived → the test banked once, where replace and append publish the same two names. Now banks twice (8f4a70d)
2 the primary publishes only the form, dropping the session publishes the banked classes when the form is empty
3 cancel discards without asking asks before it discards them
4 the ask moves onto the Cancel button asks on Escape too, which is the route a button guard walks past
5 the session is not checked for collisions refuses a name already banked in this session
6 the note names only the last class publishes the banked classes and the form's own
7 the prefill is dropped starts from what the create row was typed with
8 the badge fetches the active version on arrival asks for nothing about the active version until it is opened
9 the comparison is enabled whether or not the popover is open survived → no test ever closed it, and after one opening the active version is cached. New test invalidates over a closed popover (9cb3410)
10 the last class is not armed arms the last class written and names it
11 the first class is armed instead of the last that one
12 the drawing class goes back to being per-asset keeps the drawing class when the next frame opens
13 the create row opens the dialog empty again opens the dialog on the name the class field's create row was typed with
14 the tail run is never flushed closes a run that reaches the end of the list
15 a run of one collapses survived → the shape helper rendered a one-element run identically to a single row. Brackets added (411e071)
16 null provenance is read as annotation never joins a run with a version that recorded no provenance
17 the ledger renders flat again collapses the run and leaves the milestones alone
18 expanding a run renders nothing gives back every row when it is expanded
19 the run summarises its oldest version survived → every version in the fixture declared the same classes. Fixture differentiated (63222e1)
20 the F23 notice names only the form field names every class of the session
21 a banked class cannot be removed lets a banked class be taken back out
22 the primary gates on the form, not on what it publishes publishes the banked classes when the form is empty
23 ⌘Enter does nothing banks on ⌘Enter, so a session is typed without leaving the keyboard
24 in chromium: the popover ignores Escape the cycle's own pin-popover assertion — one browser repeat, because focus and key handling are what a hand-built disclosure is for

The mutation harness itself needed two fixes mid-run, and both are worth writing down: grep -cF with an embedded newline counts lines, not occurrences, so a multi-line anchor reports a bogus uniqueness count; and a harness that aborts between applying and reverting leaves the mutation in the tree for the next one to stack on — which happened once here, was caught by a git diff --quiet after the revert, and is why every run now asserts a clean tree before it starts.

No CI job was added, renamed or removed, so the main ruleset is untouched. No Python, no migration, no wire change — openapi.json and frontend/ui-core/src/generated/ are byte-identical.

…rs back (WS4)

WS4 of the annotation workspace redesign — cf. #368.

- `AddClassDialog` accumulates: `Create and add another` (⌘↵) banks a class and
  clears the form, the primary publishes the whole session as one schema version
  under one auto-written description naming them all. Cancelling with classes
  banked asks, and asks on Escape and the overlay too, because everything a
  session holds lives in the browser. The F23 `canRepin` preflight, the
  save→publish→repin order and the partial-failure table are unchanged.
- The class field's create row carries the typed name into the dialog. WS2 had
  handed it over already; the page dropped it because the dialog had nowhere to
  put it. The row is also no longer gated on `onOpenGallery`, which had made it
  absent for exactly the callers whose tool-strip `+` still worked.
- The pinned `v{n}` badge opens a disclosure: whether the batch's version is
  still the project's current one and, if not, what arrived since. Both reads are
  gated on it being open, so opening a job still asks for no `/schema`.
- The project's version history collapses consecutive `annotation`-provenance
  versions into one expandable row. `curated` and null always render
  individually; a run of one is not a run. The rule is a pure function with its
  own boundary tests.

Found in scope and fixed: the drawing class was `Workspace` state, and
`usePinnedSchema`'s query key names the version — so a re-pin sent the page
through `LoadingState`, unmounting the component that held the class somebody had
just created. #233's `activateClass(declared.name)` had therefore never worked.
It now lives in `JobScreen` beside the clipboard, so it also survives moving to
the next frame.
A session that replaced rather than appended published the same two names when
only one class had been banked, so the test could not fail on it. Caught by the
mutation pass, not by review.
…uld not see

A comparison left enabled while the popover is closed keeps refetching over a
cached active version, and the earlier tests never closed it — so removing the
`open &&` on the bounds turned nothing red. A disabled query ignores an
invalidation; an enabled one answers it.
The shape helper rendered a one-element run as `v2`, identical to an ungrouped
version, so lowering RUN_MINIMUM to 1 turned no assertion red. The brackets are
what make the two kinds of row distinguishable.
… of them

Every version in the fixture declared the same contract, so a run summarising
its oldest member instead of its newest read identically.
@JArmandoAnaya
JArmandoAnaya enabled auto-merge (squash) August 6, 2026 02:51
@JArmandoAnaya
JArmandoAnaya merged commit ed3b8cb into main Aug 6, 2026
14 checks passed
@JArmandoAnaya
JArmandoAnaya deleted the feat/368-dialog-sessions branch August 6, 2026 02:57
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