feat(viewer): wire "save 2D drawing markup into model" into the UI

[BIMvoice]

Closes #4153

Stack (three deep — merge LAST)

#4160 (write side: IfcAnnotation builders in @ifc-lite/create) → #4167 (read side: readDrawingMarkupFromParseResult, now including af5e65b5c's reader-side placement fix — see below) → this PR (UI wiring). This PR's base branch is fix-4153-drawing2d-markup-ifc-read (#4167's head), which already includes #4160. Please merge #4160 and #4167 first; this one should merge last, retargeted to main once its base has landed.

Restacked 2026-09-08. This branch was previously pinned 4 commits behind #4167's head and did not include af5e65b5c ("invert the annotation placement chain and plan-Y negation when reading drawing markup") — the reader-side fix that makes the round trip below actually correct on a non-identity storey placement. This push merges #4167's current head in, so this branch now carries that fix; the section below describing what round-trips has been corrected to match.

No CI runs on this branch. test.yml triggers only on pull_request: branches: [main], and this branch targets #4167's branch, not main. Every check below was run locally.

What this adds

• "Save Markup to Model" — a toolbar button (+ overflow-menu item in the narrow layout) in the 2D drawing panel. It resolves the active model's spatial anchor and root 3D representation context, then calls addDrawingMarkupToStore with the current measurements/area annotations/text notes/revision clouds.
• Restore-on-load — when a model is parsed, its tagged IfcAnnotation entities are read back into Drawing2DState via the existing readDrawingMarkupFromParseResult, reusing the shared symbolic-annotation parse cache (no second WASM walk).

Design decisions

Is it actually "saved"? No — saveDrawingMarkupToModel writes into the model's StoreEditor overlay only, the same overlay ExportChangesButton already reads from via markModelsDirty. Nothing touches disk. The button's tooltip and every toast say so explicitly: "Save drawing markup into the model (overlay only — Export Changes writes it to a file)", and a successful save reads "Added N markup annotation(s) to the model — use Export Changes to save it to a file."

Idempotence. removeDrawingMarkupFromStore sweeps every overlay-only IfcAnnotation tagged with a DRAWING_MARKUP_OBJECTTYPE value (plus everything only it references — placement, polyline/points, representation) before every save, mirroring emit-spatial-zones.ts's removeSpatialZones sweep-and-replace. Pressing Save N times leaves exactly one copy of the current markup. Saving an empty drawing when a previous save exists clears it (rather than refusing with "nothing to save").

The double-restore question (#4159). #4159 restores markup from localStorage, keyed by content hash, and is a separate, actively-restructured branch this PR does not touch or build on. Since both this restore and #4159's write the four flat arrays as a full overwrite (never append), duplication is structurally impossible regardless of ordering — the only open question is which source wins when both would populate the same model. I picked never overwrite: tryRestoreDrawingMarkup checks the arrays are still empty immediately before writing (checked twice — once up front, once again right before setState, since resolving the parse can take a tick), and backs off permanently for that model-session if something already populated them. Whichever source runs first wins; this restore never clobbers an existing value. I could not make the ordering itself deterministic against #4159 without touching its files, which the brief said to avoid — this is the defensible rule available without that, and is documented in useDrawingMarkupRestoreOnLoad.ts's module doc for whoever finishes the resolution once both PRs are in the same tree.

A bug this surfaced along the way

dataStore.entities.getObjectType() — the parser's batched columnar cache — only returns a value for entities the "lite" fast-parse path chose to batch-extract ObjectType for (Group family, geometry, type objects). IfcAnnotation lands in the "other relevant products" bucket, which is only batch-extracted for GlobalId+Name. Reading ObjectType off a lite-parsed IfcAnnotation this way silently returns '' every time — caught by a restore test against a real parsed fixture. Fixed by reading ObjectType via EntityExtractor off the source bytes instead (readBaseObjectType in drawing-markup-restore.ts), the same convention resolve-anchor.ts already uses for other root attributes it can't trust the columnar cache for.

A full-suite run also caught a second issue: mounting useDrawingMarkupRestoreOnLoad unconditionally reached an existing component test (measure-parity.test.tsx) whose fixture seeds a deliberately partial IfcDataStore stub as the active model — constructing a StoreEditor (which walks entityIndex.byId) during a pure read crashed it. Fixed by splitting drawing-markup-context.ts into a read-only context (getDrawingMarkupReadContext, no StoreEditor) used by restore, and the full read+write context used by save — plus a defensive try/catch around restoreDrawingMarkupFromModel so an unexpected shape degrades to "not restored yet" rather than crashing the panel.

Local verification (substituting for the absent CI)

• pnpm typecheck — 0 errors, all 1767 test files across 48 packages in the typecheck program.
• pnpm lint (oxlint) — 0 errors; 5 pre-existing warnings elsewhere in the repo, unrelated to this change.
• node scripts/check-module-size.mjs — OK, 0 new files over 400 lines (Section2DPanel.tsx grew by 9 net lines, from 1356 to 1365, against its recorded budget of 1367 — 2 lines of headroom left, budget untouched; Drawing2DCanvas.tsx, at 0 headroom, was not touched at all).
• node scripts/check-test-wiring.mjs — OK.
• node scripts/check-source-text-assertions.mjs — OK, 0 new.
• npx turbo test --filter=@ifc-lite/viewer (full suite, no cache, run on the restacked branch): 7174 pass, 0 fail, 6 pre-existing skips (7180 total across 1584 suites, 42/42 tasks) — one suite and one test more than the pre-restack count, from af5e65b5c's added drawing-markup-read-real-roundtrip.test.ts. Also ran directly and in isolation: 1 pass, 0 fail.

Mutation testing (each mutation grep/diff-confirmed applied to the file, then reverted and re-confirmed clean):

• Broke saveDrawingMarkupToModel's write call (short-circuit return before addDrawingMarkupToStore) → 4 of 7 save-side tests went RED (both "writes annotations" and "twice doesn't duplicate" suites).
• Broke buildDrawingMarkupMetaLookup's ObjectType extraction (forced readBaseObjectType to always return null) → 3 of 9 restore-side tests went RED across the two new suites; re-run together with the save mutation after the StoreEditor refactor showed 7 of the combined 16 new tests RED, confirming both wirings are still covered post-refactor.

Tests added

• apps/viewer/src/lib/drawing2d-markup/drawing-markup-save.test.ts — real IfcParser + StepExporter fixture: writes the expected tagged IfcAnnotation (test 1), survives export/re-parse, replaces rather than duplicates on a second save (test 4), clears on an empty re-save, and refuses cleanly for a missing model / model with no storey.
• apps/viewer/src/lib/drawing2d-markup/drawing-markup-restore.test.ts — buildDrawingMarkupMetaLookup against a real base-file fixture (not overlay-authored, unlike the existing drawing-markup-read.test.ts fixtures) surfacing ObjectType/Qto_IfcLiteMarkup correctly (test 2), an untagged annotation with real geometry is skipped (test 5), a model with no tagged annotations restores nothing without throwing (test 3), and restoreDrawingMarkupFromModel returns null (not empty) before a parse has landed.
• apps/viewer/src/hooks/useDrawingMarkupRestoreOnLoad.test.ts — tryRestoreDrawingMarkup called bare (no act(), since production calls it from a raw store subscription and a parse-cache callback, not a React event handler): never overwrites already-populated fields, backs off permanently once attempted, and (via a faked WASM worker reply, same technique useSymbolicAnnotations.frameRace.test.ts uses) restores from a real parse without duplicating on a repeated call.

Not done here / known limits

• Round-tripping (save → export → reopen → restore) recovers the authored coordinates and scalars, and depends on feat(viewer): translate tagged IfcAnnotation markup back into typed Drawing2DState (read side) #4167's reader-side fix. The writer (drawing-markup.ts) anchors each IfcAnnotation under the real storey placement and writes the caller's (x, y) verbatim — it does no section-cut reprojection, which drawing-markup.ts's own docs call out as a UI/wiring concern deliberately left to this action. On its own, reading those points back verbatim would only round-trip correctly when the storey chain is the identity. feat(viewer): translate tagged IfcAnnotation markup back into typed Drawing2DState (read side) #4167's af5e65b5c reader-side fix (toAuthoredPoint in drawing-markup-read-placement.ts) composes the annotation's own placement chain and inverts the symbolic parser's plan-Y negation to recover the authored local point — that is what makes the round trip correct on a real, non-trivially-placed model, and this branch now includes it (see the restack note above).
• What is still NOT true, with the reader fix or without it: the saved annotation's real-world 3D position/orientation does not track the drawing's actual section cut. Placing the markup at its true cut-plane pose in a general IFC viewer does not work, yet — see the writer's own header comment on this.
• Rotation/tilt coverage is not exercised for this feature. The reader's placement-inversion math (toAuthoredPoint) handles translation, any in-plane (about-Z) rotation, and a non-degenerate out-of-plane tilt by derivation, but the only real end-to-end round-trip test for drawing markup (drawing-markup-read-real-roundtrip.test.ts) runs against building-architecture.ifc, whose storey placement is translation-only — its own doc comment says so. The underlying placement-composition rotation math (composeWorldPlacement) is covered separately, but only by synthetic single-axis fixtures in worldPlacement.test.ts, not by a real rotated model, and not in combination with drawing markup at all. No fixture anywhere in this stack combines a non-identity rotation with a non-metre length unit. A rotated-chain and/or non-metre-unit round-trip fixture for drawing markup specifically would be a useful follow-up.
• DXF-underlay persistence (also mentioned in 2D drawing markup is lost on reload: no client-side persistence for measurements, annotations, clouds or DXF underlays #4153) is out of scope here.
• The double-restore ordering against fix(viewer): persist 2D drawing markup across a reload #4159 is defensible (never-overwrite) but not deterministic; worth a follow-up once both PRs share a tree.

Comment claiming this scope: #4153 (comment)

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: b4cc5888-e927-4686-8297-4a7159dc6090

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Claude review - no findings for 65db3499e

Reviewed this diff and found nothing to flag.

[BIMvoice]

Adversarial review came back clean — two follow-ups worth recording, neither blocking. Verified by execution (built the real packages, drove save → sweep → save → StepExporter.export()): zero dangling references, export carries only the second save's entities. No entity-deletion, data-loss, or export-corruption defect found.

1. Double-restore can discard the newer of two sources. tryRestoreDrawingMarkup adds the model to restoredForModel even on the branch where the markup arrays were already non-empty at entry. Since both restore paths (embedded IFC markup, and #4159's localStorage) do a full-array overwrite rather than a merge, whichever wins is nondeterministic and the loser is silently discarded with no re-check. Narrow — it needs a file this feature already saved into that also has a matching stored entry — and honestly documented in the module header today. Worth revisiting once #4159 lands and the two restores can actually collide, since only then is it reachable. Related: restoredForModel is a module-level Set with no retry path, so once a model is marked attempted while its fields were non-empty, embedded markup stays unloadable for the rest of the session — no data loss, but no recovery either.

2. A mid-batch throw can leave the overlay half-swept. saveDrawingMarkupToModel sweeps the previous batch before calling addDrawingMarkupToStore, and only calls markModelsDirty after that succeeds. If the add threw partway — e.g. a malformed polygon hitting addPolygonAreaMarkupToStore's points.length < 3 guard, not reachable from the current UI's invariants but not statically prevented — the overlay would hold the old batch swept and the new one partially written, with markModelsDirty never firing. The button's catch does show an honest error toast rather than failing silently, and the next save self-heals because the sweep finds remnants by tag regardless of when they were created. Low severity; a defensive fix would make sweep-and-add transactional.

Neither reproduced as a live defect under the current UI's invariants.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated
ifc-lite-dev	Ignored		Sep 8, 2026 4:12pm UTC
ifc-lite-viewer-embed	Ignored		Sep 8, 2026 4:12pm UTC

[github-actions]

Claude review - no findings for e663a6445

Reviewed this diff and found nothing to flag.

[BIMvoice]

Re-triggering (empty commit e663a644) didn't move the Issue queue check, and it won't: this branch's base, fix-4153-drawing2d-markup-ifc-read, predates fd63ca7c (#4161), which taught check-issue-queue.mjs to accept an honest line-start Refs #N against an open, ready issue — git merge-base --is-ancestor fd63ca7c fix-4153-drawing2d-markup-ifc-read reports the fix absent from that branch. CI checks out refs/pull/4170/merge, which merges this PR's head against that base, not against main — so the job runs the pre-#4161 script no matter what's pushed here. Its log confirms it: Closing issue references: (none), i.e. it never looks at the PR body's Refs #4153 at all, old or new commits alike. We pushed the empty commit expecting a re-evaluation against main's script and that was a misdiagnosis on our part.

Two ways to clear it: merge current main into fix-4153-drawing2d-markup-ifc-read so the branch carries fd63ca7c (which changes a branch you may prefer to review as-is), or apply unqueued directly to this PR. We'd request unqueued as the lower-risk option, but happy to do the rebase instead if you'd rather.

For context, this is a process red rather than a code one — #4153 is open and ready (applied by you), so the merged gate on main would pass this PR outright. The underlying stack (#4160 → #4167 → #4170) still merges cleanly onto current main, and #4160 was just re-verified: every pre-existing entity in a real 444-entity model came back byte-identical after export (checked=444 mismatches=0), and ifcopenshell.validate was clean on the original file, the annotated export, and a doubly-annotated export. Worth noting plainly: #4170 gets no CI at all since its base isn't main, so that evidence is local runs and review, not a green pipeline.

[BIMvoice]

Restacked onto #4167's current head (2026-09-08).

This branch was pinned 4 commits behind fix-4153-drawing2d-markup-ifc-read's head and did not include af5e65b5c ("invert the annotation placement chain and plan-Y negation when reading drawing markup") — the reader-side fix for #4153's coordinate bug. Without it, a save → export → reopen round trip only recovered the authored coordinates when the target storey's placement chain was the identity; on a real, non-trivially-placed model it came back mirrored/offset.

What changed here:

• Merged upstream/fix-4153-drawing2d-markup-ifc-read (current head, including af5e65b5c) into this branch — a merge, not a rebase, since the branch is pushed and the maintainer may have based work on it. No conflicts.
• Verified git merge-base --is-ancestor af5e65b5c HEAD now exits 0.
• Cherry-picked the corrected header comment on drawing-markup-save.ts (previously claimed the round trip "works" unconditionally and that the reader reads points "straight off the parsed geometry" — both false; the reader composes a placement chain and inverts a Y-negation). Re-verified the corrected comment line-by-line against the current toAuthoredPoint implementation.
• Ran the real round-trip test (drawing-markup-read-real-roundtrip.test.ts) directly on this restacked branch: 1 pass, 0 fail.
• Full viewer suite on the restacked branch: 7174 pass, 0 fail, 6 pre-existing skips (7180 total, 1584 suites, 42/42 tasks) — one suite/test more than the previous count, from the merged-in round-trip test.
• node scripts/check-module-size.mjs: OK, 0 new files over budget. Section2DPanel.tsx (allowlisted, tightest headroom in this stack) unaffected by this restack — still 1365/1367.
• Rewrote the PR body: the "Round-tripping ... works" line was true only once this restack landed, so it now explains the dependency on feat(viewer): translate tagged IfcAnnotation markup back into typed Drawing2DState (read side) #4167's reader-side fix, and adds an honest note that rotation/tilt handling has no real-fixture test coverage in this stack (the only real round-trip fixture, building-architecture.ifc, has a translation-only storey placement; no fixture combines a non-identity rotation with a non-metre unit).
• Added Closes #4153 — the issue carries the ready label. feat(create): translate 2D drawing markup into tagged IfcAnnotation (write side) (#4153) #4160 and feat(viewer): translate tagged IfcAnnotation markup back into typed Drawing2DState (read side) #4167 still need unqueued from a maintainer (a self-applied label is auto-stripped) since this is the top of the stack.

Pushed to upstream/fix-4153-drawing2d-markup-ui at 179d63d1e. No merge, close, label, or assignment changes made.

[github-actions]

Claude review - no findings for 179d63d1e

Reviewed this diff and found nothing to flag.