diagram: interactive-editing burndown (Cancel, cloud drags, offscreen open, sim specs, selection state)#834
Merged
Merged
Conversation
The variable details inspector saves the equation/units/notes fields on blur, which is load-bearing: a canvas-driven edit blurs the panel editor to commit its in-progress text. But clicking Cancel or pressing Escape blurred the focused editor first, so the edit committed before the discard ran. handleEquationCancel also only reset React state, while the Slate document is uncontrolled after mount, so the visible editor kept showing the abandoned text. Blur now commits only when focus actually leaves the panel (relatedTarget outside the card), so tabbing or clicking toward the panel's own Cancel/Save/Delete buttons or another field no longer save-on-blur; a blur to the canvas or nowhere still commits. A within-panel blur also keeps the raw equation editor open (rather than collapsing to the last-committed preview) so a pending edit stays visible. The action buttons additionally preventDefault on pointer-down to keep focus on the editor for mouse/touch, since macOS does not focus a button on click and a plain blur would otherwise fire before the click. Escape now discards on all three fields, and the shared discard path resets the live Slate documents (not just React state) so the visible content reverts. Because the editors are withHistory, the discard also clears each editor's undo/redo history -- otherwise a single Cmd+Z would invert the revert and resurrect the abandoned text, which a later blur would then commit.
When a cloud endpoint drag crosses the perpendicular threshold and UpdateCloudAndFlow rebuilds the flow as an L, the valve was placed with clampValveToClosestSegment against the fixed pre-drag valve position. As the L deepened the closest segment flipped from the cloud-adjacent segment onto the perpendicular riser and the clamp pinned the valve to the riser's stock-adjacent end -- a ~100px same-topology teleport of the valve and its label onto the stock's corner every frame, which the commit path (reattachEndpoint routes through the same branch) then persisted. The valve now lands on the cloud-adjacent segment -- the segment the drag is actively growing, which tracks the cursor continuously -- mirroring the Z-shape precedent of landing the valve on the segment the drag created. New growEndpointDrag continuity tests sweep perpendicular drags (sink/source/vertical/reverse) as small steps re-routed from the original flow, asserting bounded per-step valve movement, no segment flip-flop, and an interior final valve. The source-cloud case's topology-change bound is temporarily 30px because the pre-existing adjustFlows valve-mirroring defect (#832) leaks into that one step.
adjustFlows' 2-point valve tail re-placed the valve as min(otherEnd, movedEnd) plus an absolute scaled offset, which is only idempotent when the fixed end is the smaller coordinate (sink drags). Grabbing a SOURCE cloud reflected an off-center valve across the segment midpoint on the very first frame -- a ~22px visible jump of the valve and label before the drag even moved. Replace the mirror math with signed fraction preservation anchored to the fixed end, per axis (the same idea preserveValveFraction uses for multi-segment flows): frac = clamp01((valve-other)/(origMoved-other)), newCoord = other + frac*(newMoved-other). The oldLen==0 branch keeps the #818 divide-by-zero guard, and the degenerate-creation sub-case (oldLen==0, newLen!=0) centers the valve to match the old fallback; the #720 self-loop early-return is untouched. Sink-side behavior is algebraically identical for in-segment valves, and the reviewer verified the one perpendicular-axis difference (valve exactly on otherEnd no longer jumps to the midpoint) is strictly more continuous. With the reflection gone, the source-cloud straight-to-L continuity hop drops from ~24px to ~8px, so the temporary 30px test bound tightens to the 15px default.
Several element-mutation paths (delete, create, flow attach, link attach) cleared state.selection directly, leaving showDetails at 'variable' over an empty selection -- inconsistent state that only the render guard in getVariableDetails masked, and (via handleSearchChange) a no-match search could strand a details panel the same way. Issue #529's other half, the onSelectionChanged host notification, was already covered by the post-commit selection effect from the React refactor. Route those four mutation paths through one pure helper, selectionStatePatch(selection, currentShowDetails): an emptied selection closes the selection-tied 'variable' panel and resets the details tab, but deliberately preserves an open model-level 'errors' panel so deleting a variable mid-error-triage does not close the error list. handleSelection's empty-click/Escape dismiss and the navigation handlers keep their broader explicit close-all: clicking empty canvas is a dismiss gesture, and navigation closes panels even for a non-empty restored selection.
A saved viewBox/zoom can leave a model's diagram entirely outside the visible canvas, so opening the project greets the user with a blank surface and no hint of where the model went. On mount -- once per Canvas instance, after the first real svg measurement, when no gesture owns the viewport, and never in embedded mode (viewport-inert) -- the Canvas now measures the visible fraction of the diagram and, below a 10% threshold, re-centers the diagram at the current zoom through the same onViewBoxChange path a settle commit uses (queueViewUpdate: view refresh, no undo entry). The visibility metric intersects the diagram's screen-space bbox with the viewport and normalizes by the SMALLER of the two areas: for models smaller than the viewport that is fraction-of-diagram-visible, while for models larger than the viewport it is fraction-of-viewport-filled, so a large or zoomed-in model that fills the screen reads fully visible and a legitimately saved framing is never yanked to bbox-center. Module navigation reuses the Canvas instance and manages its own viewport, so the once-per-mount latch deliberately does not re-run on drill-in.
UpdateCloudAndFlow's reroute branch moved the cloud endpoint by only the perpendicular component of the drag, pinning its parallel coordinate to the original position. Because each live-drag frame re-routes from the original flow, a drag that had already traveled along the flow's axis snapped the cloud back the instant the perpendicular component crossed the bend threshold -- a one-frame teleport of 100px or more, which the commit path then persisted. The four near-identical per-endpoint/per-axis sub-branches collapse into one: the cloud endpoint moves by the full drag delta on both axes while the corner stays a riser on the fixed end's coordinate, so the segments remain orthogonal and pure-perpendicular drags are unchanged (verified point-for-point against the old branches). Dragging the cloud exactly back onto the fixed end's axis now degenerates the L to a straight flow, so the points are normalized before valve placement to drop the zero-length corner. The straight (axis-constrained) branch already applied the full parallel delta, so the two regimes now agree at the threshold and the parallel coordinate is continuous across the bend. The perpendicular catch-up hop at the bend itself is pre-existing axis-pinning behavior and is strictly smaller than before.
Each character typed in the drawer's start/stop/dt/time-units fields
fired an engine patch, recorded an undo entry (typing a four-digit year
evicted most of the 5-deep undo buffer), and scheduled a save. Worse,
intermediate input states patched garbage into the model: clearing a
field to retype patched Number('') === 0.
Each field now holds a local draft while focused and commits once on
blur or Enter, through a pure resolver that rejects empty/non-finite
numbers, non-positive dt, and unchanged values (start<stop stays the
engine's job; time units is a free string, empty allowed). Escape
reverts the field to the model value without committing and without
dismissing the drawer. An engine refresh updates idle fields but never
clobbers a focused draft; a just-committed draft stays visible until
the model prop catches up, with a ref guard making the commit
idempotent if a sibling-field focus re-blurs the field during that lag.
Closing the drawer commits a pending edit via the Drawer's
focus-restoration on close (it CSS-hides rather than unmounts, so the
blur is what fires). The drawer's per-field onChange props collapse
into one onSimSpecCommit(field, value).
The full-payload read-modify-write in applySimSpecChange (a second
field committing while the first's round-trip is in flight could carry
stale values) predates this change and is now tracked separately.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
=======================================
Coverage 90.87% 90.87%
=======================================
Files 224 224
Lines 143133 143133
=======================================
+ Hits 130070 130072 +2
+ Misses 13063 13061 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c011e15e17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+401
to
+405
| const focusLeftPanel = (e: React.FocusEvent): boolean => { | ||
| const next = e.relatedTarget as Node | null; | ||
| const card = cardRef.current; | ||
| return !next || !card || !card.contains(next); | ||
| }; |
There was a problem hiding this comment.
When a pending equation/units/docs edit blurs to the "Lookup Function" tab, relatedTarget is a tab trigger inside cardRef, so this returns false and skips handleEquationSave; the tab switch then unmounts the editors. If the user selects another variable or navigates away without returning to the Equation tab and pressing Save, the edit is dropped even though leaving the field used to commit it. Treat tab changes as a leave/commit path, or save before changing activeTab.
Useful? React with 👍 / 👎.
|
Review Reviewed the seven fixes across Editor.tsx, ModelPropertiesDrawer.tsx, VariableDetails.tsx, Canvas.tsx, Flow.tsx, viewport.ts, and sim-spec-draft.ts, plus the accompanying test coverage. Every fix traces through cleanly:
No blocking issues found. Overall correctness: Correct. |
Codex review of the Cancel-discard change caught a hole the intra-panel blur gate opened: clicking to the Lookup Function tab blurs the equation editor toward a target inside the card, so the blur deliberately does not commit -- but the tab switch then unmounts the editors, and the Lookup tab renders no Save/Cancel, so the pending edit sits stranded and invisible until the next keyed remount (selecting another variable, any content edit) silently drops it. Before the Cancel fix that blur committed, so tab switching never lost an edit. Treat a tab change as leaving the editing context: handleTabChange now commits pending edits (and collapses back to the preview when no error pins the editor open) before switching, matching blur-away semantics.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ReviewI reviewed the seven fixes across VariableDetails (Cancel/Escape discard), Flow.tsx (cloud drag valve placement, valve fraction preservation, along-axis travel preservation), Editor.tsx ( I didn't find any correctness bugs introduced by this PR. A few spots I looked at closely:
Overall correctness verdict: correct. No blocking issues found. |
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.
Seven fixes for users interactively creating and editing models, picked from the #733 backlog index plus three issues discovered (and fixed) along the way. Each fix was implemented by a dedicated agent, adversarially reviewed by an independent agent, and iterated until the review had no material findings; each landed as its own commit.
Fixes #86
Fixes #53
Fixes #52
Fixes #55
Fixes #529
Fixes #832
What changed
59c780fd). All three details-panel editors save on blur, and clicking Cancel blurred first — so Cancel saved the edit it was discarding. Blur now commits only when focus leaves the panel; Cancel/Save/Delete preventDefault on pointer-down; Escape discards on all three fields; the discard resets the live Slate documents and clears their undo history so Cmd+Z can't resurrect the abandoned text (found by the adversarial review). The load-bearing canvas-blur-commit is regression-tested. Browser-verified on the real macOS mouse path.b8031d52, diagram: adjustFlows valve-mirroring is non-idempotent for off-center valves on the min-coordinate endpoint #83291718548, follow-upd4199189). Browser-reproduced: dragging a cloud perpendicular so the flow bends into an L teleported the valve+label onto the stock corner (~100px, every frame) and committed it there. Root cause was closest-segment clamping against the fixed pre-drag valve; the valve now rides the segment the drag is growing. The review of that fix surfaced two more defects in the same code, both fixed here rather than deferred: grabbing a source cloud reflected an off-center valve ~22px on the first frame (diagram: adjustFlows valve-mirroring is non-idempotent for off-center valves on the min-coordinate endpoint #832, the old mirror math was only idempotent for sink drags — replaced with signed fraction preservation), and a diagonal drag that traveled along-axis before bending snapped the cloud back ~100-165px at the bend (the reroute discarded accumulated parallel travel — it now applies the full delta). Continuity is pinned by step-sweep tests (same-topology steps <12px, topology-change hop <15px, measured 8px).a780ee96). Once per mount, after the first real svg measurement, idle-only, never in embedded mode; commits through the no-undo viewport path. The adversarial review caught that the first-draft visibility metric would have yanked every large/zoomed-in model to center on open (fraction-of-bbox can never reach the threshold when the bbox dwarfs the viewport); the metric now normalizes by the smaller of bbox and viewport area, so a viewport full of diagram reads fully visible.370200b5). Most of diagram: selection-mutating Editor handlers bypass handleSelection, skipping showDetails reset and onSelectionChanged #529 was already fixed by the React refactor (onSelectionChangedfires from a post-commit effect for every path). The residue: delete/create/flow-attach/link-attach (and a no-match search) leftshowDetails: 'variable'over an empty selection. They now route through one pure helper. Decision: the model-level errors panel is deliberately exempt — deleting a variable mid-error-triage keeps the error list open; empty-canvas click/Escape keep their broader dismiss-everything behavior.c011e15e). Every keystroke in start/stop/dt/time-units fired an engine patch, an undo entry (typing "1900" evicted most of the 5-deep undo buffer), and a save — and clearing a field patchedNumber('') === 0into the model. Fields now hold a local draft and commit once on blur/Enter through a pure resolver that rejects empty/non-finite values and non-positive dt; Escape reverts the field without dismissing the drawer. Browser-verified.Issues handled without code
applySimSpecChange's full-payload read-modify-write can lose a concurrent sim-specs commit. Pre-existing, sub-second window, not human-reachable today; tracked instead of hotfixed.Verification
Every fix has fail-then-pass unit coverage (+~1,050 net test lines; the diagram suite grew from 1,395 to 1,475 tests). #86, #53, #110, and #55 were additionally verified in a real browser against the dev stack; #52's mount path is covered by integration tests with real resize sequencing plus the reviewer's code-trace (synthetic pointer events can't drive the canvas viewport, so no browser E2E for that one). Pre-commit hooks (Rust + TS + Python) passed for every commit.
One known cosmetic behavior deliberately left alone: the perpendicular catch-up hop when a straight flow first bends (the flow is axis-pinned below the bend threshold, so the arrowhead catches up at the bend). It predates this work, is strictly smaller than before, and smoothing it is a reroute-threshold redesign rather than a bug fix.