fix: wire up three unreachable UI affordances - #33
Merged
Conversation
Confidence Score: 5/5The PR appears safe to merge. Both previously reported focus-state failures are fixed, and no blocking failure remains. Reviews (5): Last reviewed commit: "fix(mindmap): clear the mind-map chrome ..." | Re-trigger Greptile |
Two mind-map capabilities were fully implemented in the model but had no way to
reach them from the UI. Both read as bugs rather than missing features, so both
are now wired.
1. Cross-links could be created but never removed. The overlay has a "Link to
node" button and the node layer renders the links, but nothing called
unlinkNodes — so a link could only be undone through undo history, and never
at all once the session moved on. Cross-links are now selectable: an invisible
12px-wide hit-line makes the 1.5px dotted line comfortably clickable, the
selected link turns blue, and it can be removed with Delete/Backspace or the
× that appears at its midpoint.
Selection details that matter:
- A cross-link is not a node, so it can't live in store.state.selection (which
holds node ids). It gets its own single slot, mindmapUi.selectedCrosslinkId.
- Node and link selection are mutually exclusive: selectNode() clears the link,
and Delete only targets a link when no node is selected — so Delete never has
two plausible targets.
- Escape deselects a link, and pressing empty canvas clears it, so its × never
lingers over a map the user has moved on from.
- Clicking a link while "click a target node" is armed is ignored, so it can't
steal the cross-link-creation click.
2. Focus mode (isolate one branch, dimming the rest to 12%) had no activator,
though MindMapNodeLayer already read mindmapUi.focusId and did the dimming.
There is now a "Focus this branch" toggle in the contextual toolbar.
The way OUT is deliberately a separate always-visible pill at the top of the
screen, NOT that toolbar button: the contextual toolbar only renders while a
node is selected, so clicking empty canvas while focused would otherwise hide
the only exit and strand the user in a dimmed map.
Tests: 109 vitest pass (102 before; 7 new in stores/mindmapUi.test.js covering
selection exclusivity, Delete routing in both directions, undo as one step, and
that focus always toggles off). One test documents an existing good behaviour the
work surfaced: deleteSubtree already prunes cross-links touching a removed node,
so deleting an endpoint leaves no dangling link.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drag-a-tool-from-the-palette-onto-the-canvas was half-built. The receiving side has always been live — DiagramCanvas binds dragover/drop, and useShapeCreation.onCanvasDrop reads the payload and places the shape at the drop point (or a two-endpoint line for connector types) — but no palette tile ever called startPaletteDrag, so nothing produced a payload and the gesture silently did nothing. The three tile groups in the shapes popover (Recent, Shapes, Lines & connectors) are now draggable and wired to it. Two details: - The popover closes on dragEND, not dragstart. Closing it on dragstart unmounts the element being dragged, which cancels the drag in some browsers. - startPaletteDrag also arms draw mode, so a drag released outside the canvas leaves the tool ready to draw — the same end state as clicking the tile, rather than a gesture that appears to do nothing. DATA_TRANSFER_KEY is now exported so a test can pin the producer/consumer contract. That is the failure mode worth guarding: the two halves live in different files and agree only on that string, and a mismatch makes the whole gesture a silent no-op — exactly the bug being fixed here. Tests: 114 vitest pass (109 before; 5 new in composables/useShapeCreation.test.js). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Caught in review of the previous commit. mindmapUi.focusId can outlive the node it names — delete the focused node, undo past its creation, switch documents, or take a collaborator's delete — and subtreeIds() returns [id] for an id that isn't in the model. So the "focused subtree" ended up containing nothing real and the dim test inverted: EVERY remaining node dimmed to 12% while the banner still claimed a focus. The exit pill made it recoverable, but it looked broken. Focus is now read through focusedNodeId(model), which reports the id only while that node still exists. Both consumers use it — the node layer for dimming and the overlay for the banner/button state — so a stale id is simply inert. The guard is deliberately pure: it does not clear focusId, because it is called from computeds and mutating reactive state during evaluation invites feedback loops. Turning focus off explicitly remains toggleFocus's job. A consequence worth noting: undoing the delete brings the node back and focus resumes, which is the right outcome for undo. Tests: 119 vitest pass (114 before; 5 new covering delete, undo, a null/empty model and the never-focused case). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vibhavkatre
force-pushed
the
feat/wire-mindmap-affordances
branch
from
July 29, 2026 09:52
1e20701 to
810dbfb
Compare
Follow-on to the previous commit, also from review. Rendering now ignores a focusId whose node is gone, but toggleFocus still branched on the raw flag — so with stale state the UI showed "Focus this branch" while the toggle thought focus was ON. The user's first click merely cleared state that was already inert and appeared to do nothing; a second click was needed to actually focus. toggleFocus now decides from focusedNodeId(store.state.mindmap), the same guarded value the button and banner display, so the control and its label always agree. Tests: 145 vitest pass (144 before; 1 new asserting that after deleting the focused node, a single toggle focuses the newly selected node). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third review round on the same state, and the root cause rather than another symptom. editorUi is created fresh per editor, but mindmapUi is a module singleton that outlives a document — and every field it holds is a node id. Node ids come from a per-session counter, so two documents created in different sessions both contain ids like 'm2'. A focus (or a half-armed cross-link, or a delete confirm) left behind in one map therefore PASSES the existence guard added in the previous commit and silently attaches to an unrelated node that happens to share that id in the next map — reopening a document could restore a branch focus the user never asked for, dimming the rest. resetMindmapUi() now clears all five fields, and EditorShell calls it both at setup and in the watcher that loads a late-arriving or swapped document (the editor route has no :key, so setup does not necessarily re-run per diagram). The existence guard still earns its place: it covers staleness WITHIN a document (delete, undo), where no reset fires. Tests: 146 vitest pass (145 before; 1 new that builds a second document containing the leaked id — the same-session store counter never repeats ids, so the test has to construct the collision the way persistence produces it). beforeEach now uses resetMindmapUi, so the tests share the editor's own reset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Follow-up to the cleanup audit (#24). That audit found code that exists and works but has no way to reach it from the UI — these read as bugs to a user, not as missing features, so this PR wires all three.
1. Mind-map cross-links could be created but never deleted
The overlay has a "Link to node" button and the node layer renders the links, but nothing ever called
unlinkNodes. A cross-link could only be undone via undo history — and once the session moved on, not at all.Cross-links are now selectable and removable (Delete/Backspace, or the × at the link's midpoint). An invisible 12px hit-line makes the 1.5px dotted line comfortably clickable, and the selected link turns blue.
Selection details that matter:
store.state.selection(which holds node ids). It gets its own slot,mindmapUi.selectedCrosslinkId.selectNode()clears the link, and Delete only targets a link when no node is selected. Delete never has two plausible targets.2. Mind-map focus mode had no activator
Isolating one branch (dimming the rest to 12%) was fully implemented —
MindMapNodeLayeralready readmindmapUi.focusId— but nothing could turn it on. There's now a "Focus this branch" toggle in the contextual toolbar.The way out is deliberately a separate always-visible pill at the top of the screen, not that toolbar button: the contextual toolbar only renders while a node is selected, so clicking empty canvas while focused would otherwise hide the only exit and strand the user in a dimmed map.
3. Palette tiles couldn't be dragged onto the canvas
The receiving side was always live —
DiagramCanvasbinds dragover/drop andonCanvasDropplaces the shape at the drop point (or a two-endpoint line for connectors) — but no tile calledstartPaletteDrag, so nothing produced a payload and the gesture silently did nothing. All three tile groups (Recent, Shapes, Lines & connectors) are now draggable.Two details:
startPaletteDragalso arms draw mode, so releasing outside the canvas leaves the tool ready, matching what clicking the tile does.DATA_TRANSFER_KEYis now exported so a test can pin the producer/consumer contract. That's the failure mode worth guarding: the halves live in different files, agree only on that string, and a mismatch makes the gesture a silent no-op — exactly the bug being fixed.Note on the fourth item
The audit also flagged
duplicate_diagramas unwired. On closer inspection that was a misclassification — Duplicate already works from the tile menu, implemented frontend-side via the list resource (TileGrid.duplicate), the same way trash and delete are. The backend endpoint is therefore superseded, not missing, and is removed in #26 alongside the others.Testing
stores/mindmapUi.test.js, 5 incomposables/useShapeCreation.test.js)deleteSubtreealready prunes cross-links touching a removed node, so deleting an endpoint leaves no dangling linkyarn buildcleanNot yet verified in a live browser — local Playwright driving is blocked on site credentials.
🤖 Generated with Claude Code