diagram: flow-editing audit hardening (rename corruption, name-edit UX, routing robustness)#823
Merged
Conversation
The SpeedDial tool palette regressed two deselect gestures the editor once had: re-clicking the active tool no longer toggled it off, and closing the palette via the FAB left the tool selected. On load no tool is selected, and both gestures should return to that state. handleDialClick lost its selectedTool clearing in 5191a9b (a SpeedDial UX commit); restore it so closing the palette deselects the active tool. The tool-select handlers set the tool unconditionally; make them toggle so re-clicking the active tool deselects it. Selecting a different tool still switches normally.
Creating a flow by dragging its sink onto a stock attached the sink to the stock's center coordinates with a midpoint valve and no orthogonal routing (71752f3), so the arrowhead sat behind the stock and the flow could be diagonal. That commit was working around an earlier zero-delta route that collapsed the sink onto the source column. Route the in-creation sink the same way an existing flow's endpoint reattaches (reattachEndpoint): place the target at the old sink point and move it to the stock via UpdateCloudAndFlow, which picks the axis from the drag direction, aligns the sink to the source's axis, and clips it to the stock face. The source stays fixed and the valve lands on the segment. A flow sink only ever targets a stock (isValidTarget gates the canvas to stocks), so the dead cloud-target write-back is dropped.
Dragging out a flow with the flow tool showed nothing while the mouse moved: the tool stages a degenerate flow (both points at the press point) and records the drag only as moveDelta, and applyGroupMovement translated that degenerate cloud-to-cloud flow rigidly, so it stayed zero-length and invisible until release. Derive the preview in deriveRenderState instead: a new pure helper, growInCreationFlow, routes the sink toward the cursor (over empty space) or onto a hovered stock's edge, keeping the source fixed and the flow orthogonal -- the same routing computeFlowAttachment uses on release, so the preview matches the committed result. applyGroupMovement's UpdateFlow also drags the source cloud to the cursor (its cloud-to-cloud case translates both clouds), so the source cloud is restored to its staged position after the override. Also fix a latent degenerate-axis bug in adjustFlows: with both points coincident the perpendicular component is zeroed upstream, and the signed 'd.x > d.y' axis pick then collapsed to 'd.x > 0' / '0 > d.y' -- correct for rightward/downward drags but leaving leftward/upward ones degenerate. Compare magnitudes instead. This guard runs only for fully degenerate (in-creation) flows, so non-degenerate routing is unchanged; the fix covers both the live preview and the commit path.
LAYOUT.md still described flows as 2-point-only and used Immutable.js List types removed in 406a700; the routing code has supported multi-segment flows, perpendicular-drag L conversion, interior segment drags, and same-side flow spreading for some time. The new audit doc records the June 2026 deep audit of the interactive flow-editing path (rename/stale-view corruption chain, name-editor commit semantics, orientation-classification robustness, group-movement duplication) and the decisions behind the fixes that follow in this branch, so the PR has a durable rationale.
preserveLiveView unconditionally keeps the active model's live view on every updateProject rebuild, protecting newer optimistic pans/moves from older engine snapshots. But handleRename ships its view change as an upsertView op inside the rename patch rather than through updateView, so the live view it was 'protected' by was the stale pre-rename one: the rename applied correctly in the engine and the save, while the UI kept showing the old name -- and the next geometry edit round-tripped that stale view back into the engine, persisting a model/view divergence (a view element whose variable no longer exists; observed live as an editor-crashing corrupted project). applyPatchOrReportError now mirrors any primary-view upsertView op for the active model into the live project via the same optimistic step updateView uses, so an explicit view op -- which is newer user intent than the live view by construction -- wins the merge. updateView and queueViewUpdate call engine.applyPatch directly, so their own view patches are not double-applied.
Plain Enter in the inline name editor previously inserted a newline (only ctrl/shift/alt+Enter committed), so the universal 'type name, press Enter' interaction staged a trailing newline that a later click-away committed; handleRename's single-occurrence newline escape then canonicalized such names into garbage idents (drain_). Enter and NumpadEnter now commit, shift+Enter inserts the line break (labels stay multi-line capable), and Escape still cancels. Committed names are sanitized (per-line trim, blank lines dropped); an empty result is treated as cancel, which for a just-created flow keeps the existing delete-on-cancel behavior. The create path now encodes line breaks to the stored backslash-n form exactly like the rename path, and the rename path encodes every newline instead of only the first. The Editor's global keydown gains Delete/Backspace (delete the current selection; guarded by isEditableElement, readOnlyMode, and stdlib models) and Escape (disarm the active creation tool, else clear the selection). This is the only delete affordance for unnamed elements (clouds) -- the details panel is the sole other one and unnamed elements never open it. The details panel itself now degrades gracefully when the selected view element's variable is missing from the model (a divergent/corrupted project): previously a render-time getOrThrow took down the entire editor via the ErrorBoundary, which also made the corrupt element unremovable since the panel hosts the delete button. Together with keyboard delete this makes such projects repairable in the UI.
Routing decisions used exact coordinate equality (p1.y === p2.y) to classify a segment as horizontal or vertical. Pre-fix creation bugs persisted flows with a few pixels of cross-axis drift (a live specimen drifts 5.5px), and imported models can carry the same; under exact checks such a visually-horizontal flow classified as VERTICAL, so a stock drag routed a wrong-way L (down from the source cloud, into the stock's left edge, valve and label stacked on the cloud), a parallel endpoint drag read as perpendicular and rerouted into a bogus L, and adjustFlows updated only the wrong coordinate, visually detaching the flow from its dragged endpoint. computeFlowRoute, getFlowAttachmentInfo, UpdateCloudAndFlow, and adjustFlows now classify by the segment's dominant axis (ties count as horizontal). Exact equality remains the definition of orthogonal for rendering and normalizeFlowPoints; dominance only breaks the classification tie so near-axis data routes the way it looks. One group-movement expectation moved with the change: a genuinely diagonal (77.5px by 50px) flow now attaches its L via the bottom edge (perpendicular to its dominant horizontal) instead of the right edge the misclassification used to pick.
A flow drawn out of a stock stages its source point at the press-time stock CENTER, and computeFlowAttachment routed only the dragged sink -- so the persisted source endpoint sat hidden under the stock body, violating the documented edge-attachment rule until the next stock drag re-pinned it (observed in live data: both endpoints of a stock's flows attached at its center). pinSourceToStockEdge moves only the dominant-axis coordinate of the source point onto the edge facing the sink (a straight flow stays straight) and re-clamps the valve to the shortened segment. It is applied once, at creation commit, after the sink is routed. The routing deliberately avoids UpdateCloudAndFlow's zero-delta straight path, whose cloud-branch valve math mirrors an off-center valve around the segment midpoint (part of the adjustFlows duplication flagged in the audit).
Behavior-preserving P3 cleanup of the interactive flow-editing code, per docs/dev/flow-editing-audit-2026-06.md. No runtime behavior changes; the diagram jest suite passes identically (plus new characterization tests). group-movement.ts: the cloud "straight-flow to L when moved diagonally" conversion appeared three times -- in routeCloudEndpointFlow and twice inside routeUnselectedFlows' near-duplicate source/sink loops. The two loops are now one helper parameterized on which end is the moved endpoint, reusing routeCloudEndpointFlow. Their per-endpoint `cloudIsSelected` guard was always true by construction (the maps are keyed only by selected endpoints), so the dead branch is removed. The repeated "propose valve at flow - delta, clamp to path" block is extracted as clampDraggedValve. Dead weight removed: the deprecated Point2D alias (unused outside the file), preProcessSelectedFlows' always-empty sideEffects return, the `hasSelectedEndpoints` expression that reduced to `selection.size > 0`, and routeUnselectedFlows' redundant second element iterable (every caller passed the same values twice, and it built two identical maps). Flow.tsx: the recurring getSegments -> findClosestSegment -> clampToSegment valve re-clamp is extracted as clampValveToClosestSegment and applied at the clean sites (both straight-to-L reroute branches plus computeFlowRoute and the segment-move path). The straight-to-L corner construction in UpdateCloudAndFlow vs UpdateFlow is intentionally NOT unified: the two model different endpoints (single cloud with a cloud var vs one-or-both clouds with a clouds array) and return different shapes, so forcing a shared corner builder would obscure both without removing real duplication. The adjustFlows FIXME is replaced with a comment explaining that the isCloud and non-cloud valve formulas differ deliberately (absolute mirror vs signed offset) and must not be merged; the non-cloud branch is noted as currently unreachable (sole caller passes isCloud=true; tracked in #822). Creation sentinel UIDs (inCreation/faux target/cloud) had duplicate definitions in Canvas.tsx and flow-attach.ts with a "keep in sync" comment and a pinning test. They now live in one module, drawing/creation-sentinels.ts, re-exported by both so every import path keeps resolving; the sync test asserts against the shared source.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #823 +/- ##
=======================================
Coverage 90.87% 90.87%
=======================================
Files 224 224
Lines 143133 143133
=======================================
Hits 130072 130072
Misses 13061 13061 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Reviewed the diff end-to-end (Editor keydown handlers, Canvas name-edit + growing preview, EditableLabel commit rules, common encoders, flow-attach create/reattach path, adjustFlows/UpdateCloudAndFlow orientation classification, group-movement.ts dedup, project-controller.adoptPatchedViews, and the accompanying tests). I traced the sink-pinning arithmetic (degenerate-source degenerate-sink case, leftward/upward variants, and the stock-source pinSourceToStockEdge follow-up), the growInCreationFlow preview against zero-delta and dominant-vertical inputs, and the adopt-patched-view interaction with preserveLiveView + refresh. I couldn't produce a scenario in which the changes regress correctness or hit a case not already covered by the added tests. No blocking findings. Overall correctness verdict: correct. |
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.
Deep audit + hardening of the interactive flow-editing path in
src/diagram, driven by a full read of the routing/attach/group-movement code and live exploration against the dev server. The audit doc (docs/dev/flow-editing-audit-2026-06.md) records every finding and decision; the highlights:The corruption chain (found live, reproduced, root-caused, fixed)
Renaming an existing element could silently corrupt a project:
handleRename's single-occurrence\nescape canonicalized into garbage idents (a livedrain_variable in bpowers/fooz).handleRenameis the only handler that patches the view via an in-patchupsertViewop, andpreserveLiveViewclobbered the renamed view with the stale live one on refresh. The rename looked like a silent no-op -- and any later geometry edit round-tripped the stale view back into the engine, persisting a model/view divergence.getOrThrowingetDetails) -- and since the details panel was the only delete affordance and there was no keyboard delete, the corrupt element was unremovable.Fixes: the controller now mirrors patch-carried
upsertViewops into the live project (same optimistic stepupdateViewuses); Enter commits / shift+Enter inserts a line break; committed names are sanitized (empty result = cancel);getDetailsdegrades gracefully; Delete/Backspace delete the selection and Escape disarms the tool / clears the selection. Each stage has regression tests, and the whole chain was re-verified live (including using the new keyboard delete to repair the corrupted fooz project).Geometry robustness
y === ychecks, so slightly-diagonal legacy flows (pre-fix creations drift a few px; imports can too) classified as the wrong axis -- stock drags routed wrong-way Ls with the valve stacked on the source cloud, and parallel endpoint drags read as perpendicular. Classification now uses the segment's dominant axis; exact equality remains the definition of orthogonal for rendering/normalization.Cleanup
group-movement.ts: the straight-flow-to-L cloud conversion existed three times (two copies inside always-true dead conditionals); source/sink loops unified; dead parameters/returns removed.drawing/creation-sentinels.ts) instead of a "must stay in sync" comment.adjustFlows' FIXME replaced with characterization tests plus a comment explaining why the two valve-fraction branches must not be merged (they differ semantically for off-center/sign-flip cases); the dead non-cloud branch is tracked in diagram: remove dead non-cloud branch and isCloud param from adjustFlows (Flow.tsx) #822.LAYOUT.mdcaught up with reality (multi-segment flows, readonly arrays, flow spreading, cloud retraction).Deferred (filed)
stockFlowViewToJsonhas two divergent implementationsadjustFlowsNote:
bpowers/foozwas repaired in the UI during verification; the orphaneddrain_model variable (referenced frompop.outflows) is pre-existing residue that view-level deletion cannot remove.Generated with Claude Code
https://claude.ai/code/session_01SuFkUBDzqaWtwgPEzVDKRm