fix(ui): load the diagram engine lazily, and keep the canvas controls out of the diagram; ui 0.41.1 - #1562
Merged
Conversation
The document's fence block needs the rendered diagram's intrinsic size to size its inline box, and nothing else of the canvas. Importing it from DiagramCanvas dragged the zoom/pan surface, its viewport hook and lucide into every closure that touched a diagram block. DiagramCanvas re-exports it, so components/diagram/DiagramCanvas and the barrel still resolve it.
The pane is CodeMirror, and a viewer that can never open one -- every fence in a Plannotator document today, since no host passes onSave -- carried the whole editor anyway. React.lazy behind the existing hasPane && sourceOpen condition; the Suspense fallback is a box with the pane's own class list rather than null, so the split it opens into is already the right size and never collapses back onto the canvas. A host that imports DiagramViewer directly still gets a working pane; it arrives one chunk later.
The full-size popout is only ever reached by pressing Expand. React.lazy with a null fallback: the overlay has simply not opened yet, so nothing in the document flow moves. The block's pending state (the source fence under "Rendering diagram…", data-mermaid-pending and all) moves to its own dependency-free module, because it is about to become the Suspense fallback the document shows while the block's own chunk loads -- one pending state for both waits.
Viewer imported MermaidBlock and GraphvizBlock statically, so every host that reads a document shipped the renderer slot, the canvas, the comment overlay, the finders and the projection whether or not the document had a fence. Both blocks are now React.lazy, one chunk each over a shared DiagramBlock chunk. The Suspense fallback is the block's own pending state in the block's own boxes (DiagramBlockPending), so the source fence paints with the document and the two waits -- the block chunk, then the engine -- read as one.
Bundles Viewer with the chunking bundler the portal build uses and walks the entry chunk's static imports only (rollup reports imports and dynamicImports separately, so a lazy edge is a chunk boundary by construction). Asserts the entry closure reaches no CodeMirror, Source pane, popout, canvas or projection, that each of those is still reachable off-entry, and that the fence's pending state stays in the entry chunk. Single-file builds inline everything and can never show this regression, which is why it needs its own check.
…on the part behind it The zoom strip, the composer and the popout header are painted over the canvas and are not in the svg, so the elementsFromPoint walk (node, then edge, then cluster) stepped straight past them to whatever part sat underneath: pressing Zoom out over a node opened the composer on that node, and a press on the strip could start a pan. A pointer event whose composed path contains a control surface now resolves no target, opens no composer and starts no pan. Controls are marked with data-diagram-control; buttons, toolbars, inputs, the composer and the source pane count without marking. Released over a control after a press that began on the canvas is handled too: targetUnder answering null would otherwise read as 'comment on the whole diagram'.
Lazy diagram engine and the canvas-control fix. Core stays at 0.25.4: nothing under packages/core moved, so 0.41.1 publishes alone. Every export named in the 0.41.0 notes still resolves from the same path; the diagram barrel additionally exports the new pending components and the control predicate.
Making MermaidBlock / GraphvizBlock lazy opens a window in which the document has painted and the draft has restored but no diagram exists in the DOM. A comment on a diagram part has no text to fall back on, so anything that drops or mis-reports it in that window is data loss. The test mounts Viewer (real lazy edges) beside AnnotationPanel with the row already seeded, holds the engine open on a gated runtime loader, and asserts the panel lists the row, nothing reports it unanchored while the block is still pending, and the badge and mark appear once the engine arrives -- no second restore pass, no reload. Negative control: making Viewer's "no diagram in this document" report key on what has MOUNTED rather than on the parse fails it on the unanchored assertion.
…m row
A comment on a whole diagram (or a label-less part) carries an empty
originalText, and one posted by an agent that named no fence carries
blockId ''. That is exactly the shape the text restore pass treats as
unrestorable: findTextInDOM('') matches nothing and the blockId names
nothing. The diagramAnchor skip has to come first, or such a comment
returns from a reload wearing the "Unanchored" chip while the diagram
overlay is showing it perfectly well.
Negative control: dropping the skip reports both diagram rows as attempted.
A report of diagram comments lost across a reload did not hold -- the probe behind it never answered the "Draft Recovered" modal and then counted an un-restored session -- but the window it pointed at is real and is widened by the lazy block wrappers. Name the three properties that keep it safe and the tests that now pin each of them.
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
1 participant
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.
@plannotator/ui0.41.1. Two fixes over 0.41.0, no API change: every export named in the HANDOFF's "Diagram engine (0.41.0)" section still resolves from the same path, and a host upgrading changes nothing.1. The diagram engine rode every document read
Reported from Workspaces (a chunked build). Chain, verified here before touching anything:
So any host that statically imports
Viewershipped the canvas, the overlay, the finders, the popout and CodeMirror on every document read, including markdown with no diagram — and since no host passesonSavefor a fence, an editor that can never open. (The reporter's suggested one-lazy-around-DiagramPopoutfix is not sufficient: the inlineDiagramViewerstill reachesDiagramSourcePane.)Three edges are now
React.lazy:Viewer→MermaidBlock/GraphvizBlock, one chunk each over a sharedDiagramBlockchunk (chosen over one sharedDiagramBlockchunk so each wrapper keeps itsReact.memocomparator and its test hooks; the no-diagram closure is identical either way). Suspense fallback is the block's own pending state — the source fence under "Rendering diagram…",data-mermaid-pendingandrole="status"intact — lifted intocomponents/diagram/DiagramPending.tsxand rendered inside the same boxes, so nothing flashes and nothing jumps.DiagramBlock→DiagramPopout, mounted when Expand is pressed, fallbacknull.DiagramViewer→DiagramSourcePane, mounted only when the pane opens, fallback a box carrying the pane's own class list (nevernull, which would collapse the split).svgContentSizemoved to its own dependency-free module (it was dragging the whole canvas intoDiagramBlock) and is re-exported fromDiagramCanvasand the barrel, so both published paths are unchanged. Every other static importer was checked; the barrel is the only other one and it is not inViewer's closure.2. The canvas controls commented on the part behind them (owner report)
The canvas resolves a click over everything under the pointer (
elementsFromPoint, node → edge → cluster) because the edge hit layer sits above the nodes. The chrome painted over the canvas is not in the svg, so the walk skipped it (wrapper.contains(el)is false for a control) and continued to the part underneath: pressing Zoom out over a node opened the composer on that node, and a press on the strip could start a pan.A pointer event whose composed path contains a control surface now resolves no target, opens no composer and starts no pan (
components/diagram/diagramControls.ts). Controls are markeddata-diagram-control(the zoom strip, the popout header);button,[role=toolbar], inputs, the composer and the source pane count without marking. Release-over-a-control after a press that began on the canvas is handled too —targetUnderansweringnullwould otherwise read as "comment on the whole diagram".Numbers
Viewer's document-read closure (the thing the report is about — the entry chunk plus its static imports, minified, gzip -9):Portal build (
apps/portal, the repo's one chunked build). Its entry already carries CodeMirror for the markdown editor, so it only sheds the diagram half:New lazy chunks (portal, raw / gzip):
DiagramBlock62,566 / 21,194 ·DiagramSourcePane4,348 / 1,878 ·DiagramPopout1,502 / 835 ·MermaidBlock571 / 320 ·GraphvizBlock506 / 302.Single-file builds are byte-neutral (they inline everything, which is why they never showed the regression):
apps/hook/dist/review.html18,280,891 → 18,280,891, byte-identical;apps/hook/dist/index.html24,661,766 → 24,664,307 (+2,541 B, +0.01%).The test, and that it fails on main
packages/ui/components/Viewer.diagramClosure.test.tsbundlescomponents/Viewer.tsxwith the chunking bundler the portal build uses (Vite/rollup,write: false) and walks the entry chunk's static imports only — rollup reportsimportsanddynamicImportsseparately, so a lazy edge is a chunk boundary by construction rather than by parsing. It asserts the entry closure contains none of@codemirror/,cm-editor,DiagramSourcePane,DiagramPopout,data-diagram-canvas,getScreenCTM; that each of those is reachable off-entry (a lazy edge that loads nothing would pass the first half and break every diagram); and that the fence's pending state stays in the entry chunk. It runs in the plainbun testsweep (not DOM-gated), so no allowlist entry is needed.On
origin/main's sources with this test file added: 7 fail, 2 pass.On the branch: 9 pass, 0 fail.
The control fix has its own DOM test in
DiagramViewer.test.tsx(a press at a point where a node sits under the zoom strip opens nothing; 1 px outside it the node opens the composer). Without the guard it failsExpected: 0 / Received: 1; with it, 20 pass in that file.Verify
bun run typecheck— cleanbun test packages/ui/utils packages/core packages/shared— 1968 pass, 0 failDOM_TESTS=1 bun test --isolate packages/ui/components packages/ui/hooks packages/editor— 1014 pass, 0 failbun test scripts/dom-test-allowlist.test.ts,bun test tests/entry-assets.test.ts(after a fresh build) — passbun run --cwd packages/ui smoke:package— "Verified @plannotator/ui@0.41.1 packs, resolves …, and installs externally with @plannotator/core@0.25.4"build:viewer+check:manifest— in syncHeadless Chromium on the built plan editor (
scratchpad/diagram-engine/fixture.md: two Mermaid fences + one dot fence), single-file build and the same App served as a chunked build (portaldistwith/apiproxied to the annotate server):[data-diagram-pending]0 after render, in both builds;On Reviewer (node U) · line 9;elementsFromPointthere reportsbuttontopmost, exactly the shape the unit test stubs);DiagramPopout-*.jsis requested only at that moment;diagram chunks requested: [];["MermaidBlock-*.js","DiagramBlock-*.js","GraphvizBlock-*.js"]— andDiagramSourcePane-*.jsnever, because no host passesonSave.Withdrawn: "diagram comments lost on reload"
An earlier revision of this PR reported a pre-existing data-loss bug — a diagram comment not surviving a reload,
after reload: {"badges":0,"marks":0,"cards":0}on this branch and onorigin/main. That report was wrong, and it was my probe, not the app.lazyprobe/reload.mjsonly clicked through first-run dialogs (Continue|Got it|Start reviewing|Done); the "Draft Recovered" modal answers toRestore/Dismiss, so it was never answered and the probe then counted an un-restored session. Its own screenshot shows the modal still open over the document, reading "Found 1 annotation from just now."Driven end to end headless on the built plan editor (
scratchpad/diagram-engine/fixture.md, isolatedPLANNOTATOR_DATA_DIR) — comment on flowchart nodeD, comment on sequence messageH->>S, reload, Restore, 10/10:flowchart/node/D,sequence/edge/msg-1);Diagram node Approve? (D), line 9andDiagram edge plan (stdin JSON) (msg-1), line 24.The same probe with the one-line fix applied (wait for the modal, then answer it) flips from
{"badges":0,"marks":0,"cards":0,"draftModalOpen":true}to{"badges":1,"marks":1,"cards":1,"draftModalOpen":false}against an unchanged binary — the whole delta was the un-answered modal. The #1560 parity driver's reload step was fixed the same way: it now answers the modal, waits for it to detach, and asserts the card count and the badge count explicitly instead of letting awaitForthrow out of the run.The mount-order window the lazy wrappers open is real, so it was proved rather than assumed: on the chunked portal build with
MermaidBlock/GraphvizBlock/DiagramBlockchunks delayed 8 s and Restore clicked while all three fences were still[data-diagram-pending], the card is present immediately (cards: 1, badges: 0, pending: 3) and the badge arrives with the chunk (cards: 1, badges: 1, marks: 1), chip-free throughout.Two tests pin it (no product change was needed):
components/Viewer.diagramLazyRestore.test.tsx(new, allowlisted) —Viewer+AnnotationPanelwith the row seeded before the engine arrives, engine held open on a gated runtime loader: the row is listed, nothing reports it unanchored while the block is pending, the badge and mark appear on arrival. Negative control: keyingViewer's "no diagram in this document" report on what has MOUNTED rather than on the parse fails it.hooks/useAnnotationHighlighter.diagramSkip.test.tsx— extended with the shape that has no quote and noblockId(a whole-diagram or label-less anchor, or an agent-posted row), which is exactly what the text pass would otherwise call unrestorable. Negative control: dropping thediagramAnchorskip fails both cases.For hosts
Nothing to change.
DiagramViewerimported directly still gives a working viewer — its pane simply arrives one chunk later. The diagram barrel additionally exportsDiagramPending/DiagramInlineSource/DiagramBlockPendingandisDiagramControlEvent/DIAGRAM_CONTROL_SELECTOR; host chrome drawn inside the canvas should carrydata-diagram-control. Core is unchanged at0.25.4, so 0.41.1 publishes alone.