diagram: editor bug burndown (caret mapping, Cmd+Enter, dblclick rename, connector sync, cloud drag)#831
Conversation
Clicking the fraction bar or the denominator of a KaTeX-rendered division (e.g. population/average_lifespan) placed the editing caret at a garbage offset, while numerator clicks worked. The click handler resolved the annotated source span via Element.closest, an ancestor-only walk: KaTeX renders a fraction's operand rows as absolutely-positioned .vlist wrappers plus a .frac-line, none of which carry data-eqnloc, so those clicks resolved to the whole-fraction span and interpolated coarsely across the entire equation. Replace ancestry lookup with geometry: gather every annotated span under the preview and pick the smallest box containing the click (a new pure chooseSpanForClick in equation-caret.ts, nearest-box fallback for padding clicks), then map within the chosen span as before. This generalizes to any nested expression (fractions, superscripts, calls) because the click resolves to the operand the pixel actually sits in. A dead-center fraction-bar click maps into an adjacent operand rather than exactly at the '/' -- the bar is geometrically ambiguous and carries no oploc annotation, so that is the principled behavior.
Cmd+Enter did nothing in the equation and documentation editors while Shift+Enter inserted a line break. The chord falls through both of Slate's input paths: modern browsers emit no beforeinput event for a modifier+Enter chord, and the legacy keydown fallback's is-hotkey bindings require unnamed modifiers to be absent, so meta/ctrl+Enter matches neither the split-block nor soft-break binding. Add a pure isNewlineChord predicate (keyboard-shortcuts.ts) and route matching chords through Editor.insertSoftBreak -- the same operation Shift+Enter triggers, so serialization and undo behavior are identical. Ctrl+Enter is treated the same as Cmd+Enter so the gesture works on non-mac keyboards; Alt combinations are left to the platform. The units editor (semantically single-line) and the canvas name editor (Enter and Ctrl+Enter commit there) deliberately do not get the chord.
Double-clicking a variable's name label failed to open the inline name editor in two ways. Deterministically: a label dblclick on an already-selected element routed into the deferred-single-select path, whose name-edit resolution only ran on pointer-up -- but dblclick is a terminal MouseEvent with no pointerId, so the resolver's pointer-id guard could never pass and the editor never opened (the union's deferredIsText field was unreachable). Flakily: Label had no click-vs-drag threshold, so the 1-2px wobble of a physical double-click started a label drag and captured the pointer, which suppresses the native dblclick in real browsers. Open the editor synchronously in handleSetSelection when the request comes from a label double-click (collapsing selection to that element, same rendering dependency the unselected path always used), delete the now-dead deferredIsText plumbing from the interaction union, and gate label drags (and their pointer capture) behind the same 5-screen-pixel threshold the rest of the canvas uses.
Equations are the source of truth for causal dependencies; connectors are their visual representation, and nothing kept them consistent: an equation could reference a variable with no connector drawn from it, or a connector could linger after the equation stopped using its source. Flag both on the dependent variable, with the existing warning-dot idiom on the canvas and explanatory text in the variable details panel. The check is a pure core (connector-sync.ts computeConnectorErrors) fed by the engine's existing Model.getIncomingLinks -- no new FFI. getIncomingLinks rather than getLinks is load-bearing: causal edges add structural flow->stock integration edges (which no connector represents) and omit a stock's initial-equation deps, so it would produce false positives and negatives; incoming links already exclude builtins/TIME, structural stock relationships, and dotted module outputs. Notable semantics: only variables whose dependency is actually drawable on the view are flagged; modules are exempt as targets (input wiring lives in the references table) and as connector sources; aliases resolve one hop; stocks are checked against their initial equation. The annotation is display-only -- saves serialize from the engine, and new tests pin that toJson never emits it. Warnings never block simulation or pin the equation editor open.
Dragging an existing flow's cloud endpoint moved the valve live but left the flow's line and arrowhead stale until pointer-up, which then rerouted and visibly snapped. The preview path (applyGroupMovement -> UpdateFlow) and the commit path (computeFlowAttachment -> UpdateCloudAndFlow) used different geometry: UpdateFlow's along-axis fallback only moves the valve. Flow creation never had this problem because it routes its live preview through UpdateCloudAndFlow. Unify on the creation codepath: generalize growInCreationFlow into growEndpointDrag (pure, in flow-attach.ts), and give deriveRenderState a branch mirroring the in-creation one that reroutes the flow and dragged cloud live -- cloud follows the cursor over empty space, pins to the stock edge over a stock. Preview and commit now call UpdateCloudAndFlow with the same delta, so release is snap-free by construction. The non-dragged endpoint's cloud is restored to its committed position during the drag because UpdateFlow's cloud-to-cloud path translates both clouds. UpdateFlow's valve/Z-routing math is deliberately left alone (still used by stock-endpoint and stock-move drags); dragging a stock-attached endpoint retains the old stale-until-release behavior, tracked as issue 830.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #831 +/- ##
==========================================
- Coverage 90.87% 90.87% -0.01%
==========================================
Files 224 224
Lines 143133 143133
==========================================
- Hits 130072 130069 -3
- Misses 13061 13064 +3 ☔ 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: 524a4fe853
ℹ️ 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".
| } | ||
|
|
||
| return project; | ||
| return this.attachConnectorErrors(project); |
There was a problem hiding this comment.
When updateProject() rebuilds after a view edit, it calls updateVariableErrors() before preserveLiveView() replaces the active view with the newest optimistic one. Because connector warnings are now attached here from view.elements, a second in-flight connector/view edit can leave connectorErrors computed against the stale engine view but rendered on the preserved live view, so users can see missing/stale-connector warnings that no longer match the diagram. Compute these annotations after the live-view merge, or pass the final view into the check.
Useful? React with 👍 / 👎.
| if (start && Math.hypot(e.clientX - start.x, e.clientY - start.y) < ClickDragThresholdPx) { | ||
| return; | ||
| } | ||
| inMove.current = true; | ||
| (e.target as Element).setPointerCapture(e.pointerId); |
There was a problem hiding this comment.
For mouse label drags that move a few pixels and leave the <text> before crossing the 5px threshold, this branch returns without pointer capture, so later pointermove/pointerup events go to whatever is under the cursor instead of this Label; the drag never calls onLabelDrag and the label cannot be moved. Capture on pointerdown or before the sub-threshold return while still delaying the onLabelDrag callback until the threshold is crossed.
Useful? React with 👍 / 👎.
Code reviewReviewed the caret-mapping / Cmd+Enter / dblclick-rename / connector-sync / cloud-drag fixes and their supporting refactors. No blocking bugs found. Notes on things I looked at closely:
Overall correctness: CORRECT. The changes are well-scoped, the removed |
Review feedback on the connector-sync work: updateProject annotated connector drift from the engine-serialized view and only afterwards swapped in the newer optimistic live view via preserveLiveView, so a connector the user just drew could transiently flag as missing (and a just-deleted one could transiently escape the stale check) because the annotations described a view the user was no longer looking at. Stop tail-calling attachConnectorErrors from updateVariableErrors and invoke it explicitly at each rebuild site on the view that path actually renders: after preserveLiveView (and projectAttachData) in updateProject, and directly on the opened project in the initial-open and undo-reopen paths, where no newer live view exists.
Review feedback on the label drag threshold: capture was acquired only once movement crossed the 5px threshold, on the element under the pointer at move time. A grip near the edge of a short label can leave the text's bounding box while still sub-threshold, after which no pointermove reaches the label at all -- the drag can never start from that grip point. Capture on pointerdown instead (on currentTarget, since the press may land on a child tspan), keeping the threshold gate on actually starting the drag. The load-bearing part of the double-click reliability fix is unchanged: sub-threshold wobble still fires no onLabelDrag, so there is no selection or node churn between the two clicks, and capture lives on the same node as the dblclick handler so click targeting is unaffected. Verified in a real browser: double-click rename stays reliable (selected and unselected variables, repeated) and an edge grip drags the label.
|
@codex review |
ReviewI reviewed all 5 fixes and their follow-up commits (caret placement on rendered fractions, Cmd/Ctrl+Enter newline chord, double-click rename reliability, connector/equation sync warnings, live cloud-endpoint drag preview). Each fix is well-scoped, has a clear root-cause explanation in its commit message, and comes with regression tests that fail against the pre-fix code. Notable strengths:
I did not find bugs to flag. The remaining considerations (e.g. tail-of- Overall correctness verdict: Correct. The patch is well-tested, the design decisions are documented in commit messages and CLAUDE.md, and existing behavior is preserved for the scoped-out cases (stock-endpoint drag, units editor plain-Enter, modules as connector targets). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3fa00415a
ℹ️ 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".
| // after preserveLiveView has swapped in the (possibly newer) live view -- | ||
| // see attachConnectorErrors. Runs after projectAttachData too, so it | ||
| // preserves the attached series when it rewrites the active model's vars. | ||
| activeProject = await this.attachConnectorErrors(activeProject); |
There was a problem hiding this comment.
When a user drills into a module model, no project rebuild runs: drillIntoModule() only switches modelName and calls refreshCachedErrors(). Since connector annotations are attached only from the rebuild/open paths such as this updateProject() call, variables in the newly active child model never receive connectorErrors on first navigation, so missing/stale connector warnings are absent until a later pan/edit forces updateProject(). Trigger attachConnectorErrors (or an equivalent annotation refresh) after changing the active model.
Useful? React with 👍 / 👎.
Review feedback: drilling into a module only flips the controller's modelName and refreshes the error panel cache -- no project rebuild runs -- so the child model's variables never received connector warnings on first navigation. Verifying the finding surfaced a deeper instance of the same mechanism: equation/unit error dots were missing after drill-in too, because refreshCachedErrors never re-annotates project variables and the model-scoped annotation passes only run from the rebuild/open paths. Add refreshActiveModelAnnotations, which re-runs both annotation passes (updateVariableErrors + attachConnectorErrors) on the current project for the newly-active model and commits the result only if neither the project nor modelName moved while its engine round-trips were in flight -- a rebuild or further navigation landing mid-flight has already produced a fresher project and must not be clobbered. Drill-in calls it in place of the bare panel refresh; back-navigation keeps relying on its queueViewUpdate rebuild (which already re-annotates), with the direct pass only as a no-stored-view fallback.
|
@codex review |
Automated reviewReviewed all five fixes end-to-end (caret placement, Cmd/Ctrl+Enter, double-click rename, connector-sync, cloud-endpoint drag) plus the follow-up commits. The functional-core / imperative-shell split is clean; the ordering rationale for A few small notes rather than blocking findings:
No blocking issues found. Overall correctness verdict: CORRECT |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d92da0a21
ℹ️ 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".
| const variable = model.variables.get(el.ident); | ||
| if (!variable || variable.type === 'module' || seen.has(el.ident)) { | ||
| continue; |
There was a problem hiding this comment.
When a target variable already has equation/compile errors, this still queues it for getIncomingLinks. For invalid/no-AST equations the engine reports no dependencies, so any existing connector into that variable is then surfaced as a staleConnector even though the equation is simply unparsable; users fixing a syntax error would get bogus connector warnings alongside the real equation error. Skip targets whose dependencies are not authoritative, such as variables with fatal equation errors.
Useful? React with 👍 / 👎.
Review feedback: a variable whose equation fails to parse was still queued for the connector consistency check. The engine reports no dependencies for a broken AST, so every inbound connector surfaced as a stale-connector warning (and every real dependency as missing) on top of the actual equation error the user was already fixing. Skip such targets at collection time: their dependency list is not authoritative, and the equation error is the actionable signal. This relies on updateVariableErrors annotating errors before attachConnectorErrors runs, which every caller guarantees. Unit-only errors do not skip -- the AST parses fine there, so dependencies stay authoritative. Empty equations carry an EmptyEquation error, so an inbound connector drawn to a not-yet-written variable reads as a forward declaration rather than stale noise; the all-empty blank sketch was already exempted via hasNoEquations.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8ae42d58d
ℹ️ 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".
| async attachConnectorErrors(project: Project): Promise<Project> { | ||
| const engine = this.engine; | ||
| const modelName = this.modelName; | ||
| if (!engine || isStdlibModel(modelName)) { |
There was a problem hiding this comment.
When every variable only has EmptyEquation, updateVariableErrors() returns { ...project, hasNoEquations: true } before annotating variable.errors; this method then still collects those variables, getIncomingLinks() reports empty dependency lists, and any connector drawn in a brand-new sketch is shown as a stale connector. In the all-empty starter-model case, add project.hasNoEquations to this early return before fetching dependencies so users don't get connector warnings while drafting equations.
Useful? React with 👍 / 👎.
Code reviewSolid, well-scoped work with strong test coverage across five separate fixes. Below are review findings. [P2]
|
Review feedback: when every variable has only an empty equation, updateVariableErrors takes its hasNoEquations branch and deliberately skips annotating variable.errors (blank-sketch suppression), so the per-variable fatal-error skip added in the previous commit never triggers. getIncomingLinks then reports no dependencies for every variable and each connector drawn in a brand-new sketch surfaced as stale -- a wall of warnings exactly where the editor suppresses them elsewhere during initial layout. Add project.hasNoEquations to attachConnectorErrors's early return, before any engine fetch. The previous commit's comment claimed this case was already guarded; that claim was wrong (the hasNoEquations check it pointed at lives in recalculateStatus) -- the comments now distinguish the two suppression paths accurately: per-variable errors skip for a mixed model's not-yet-written variable, hasNoEquations early return for the all-empty starter model.
|
@codex review |
ReviewReviewed all five fixes across the caret mapping, Cmd/Ctrl+Enter, double-click rename, connector-sync, and cloud-drag preview commits (plus the four adversarial-review follow-ups). Overall correctness: correct. No findings. Spot-checks that held up:
|
|
Codex Review: Didn't find any major issues. Nice work! 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". |
1 participant
Five user-reported editor bugs, fixed one commit each. Each fix was implemented and then adversarially reviewed (independent review pass verifying root cause, running tests against pre-fix code, and probing for regressions) before landing.
1. Caret placement on rendered fractions (04e588a)
Clicking the fraction bar or denominator of a KaTeX-rendered division placed the caret at a garbage offset. Root cause: the click handler resolved the annotated source span via
Element.closest(ancestor-only), but KaTeX renders fraction operands inside unannotated.vlist/.frac-linelayout chrome, so those clicks resolved to the whole-fraction span and interpolated across the entire equation. Now all annotated spans are gathered and the smallest box containing the click wins (purechooseSpanForClick), generalizing to any nested expression.Design note: a dead-center fraction-bar click maps into an adjacent operand rather than exactly at the
/-- the bar carries no annotation and is geometrically ambiguous, so that is the principled behavior.2. Cmd+Enter inserts a newline (f714c2c)
Cmd+Enter did nothing in the equation/documentation editors: modifier+Enter chords produce no
beforeinputevent, and Slate's legacy keydown fallback (is-hotkey) requires unnamed modifiers to be absent. NewisNewlineChordroutes Cmd+Enter throughEditor.insertSoftBreak-- the identical operation Shift+Enter triggers.Design notes: Ctrl+Enter is treated the same so the gesture works on non-mac keyboards (nothing else binds it in those fields). The units editor (semantically single-line) and the canvas name editor (Enter/Ctrl+Enter commit there) deliberately do not get the chord. Discovered along the way: the units editor already accepts plain-Enter newlines into a single-line field -- filed as #829 rather than scope-creeping here.
3. Reliable double-click rename (1942bdf)
Two root causes. Deterministic: a label double-click on an already-selected variable routed into the deferred-single-select path whose name-edit resolution only ran on pointer-up -- unreachable for a terminal
dblclickevent, so the editor never opened (the interaction union'sdeferredIsTextfield was provably dead code, now removed). Flaky: labels had no click-vs-drag threshold, so 1-2px of double-click wobble started a label drag and captured the pointer, suppressing the nativedblclick. Labels now use the same 5-screen-pixel threshold as the rest of the canvas.4. Connector/equation sync warnings (3791f79)
New feature: variables now show the existing warning-dot (plus explanatory text in the details panel) when the sketch and equations disagree -- an equation references a variable with no connector drawn from it, or a connector's source is no longer used by the equation.
Design decisions:
Model.getIncomingLinks(no new FFI). UsinggetLinks/causal edges instead would be wrong: those add structural flow-to-stock integration edges (which no connector represents) and omit stock initial-equation deps.5. Live flow updates while dragging a cloud (524a4fe)
Dragging an existing flow's cloud endpoint moved the valve live but left the line/arrowhead stale until release, which then snapped. The preview used
UpdateFlow(valve-centric) while the commit usedUpdateCloudAndFlow. Fixed by unifying on the flow-creation codepath, per the reported expectation:growInCreationFlowis generalized intogrowEndpointDrag, and the canvas previews existing-endpoint drags through it -- preview and commit now share geometry, so release is snap-free by construction.Design notes:
UpdateFlow's valve/Z-routing math is deliberately not unified (it still serves stock-endpoint and stock-move drags, and past attempts to merge the two broke routing). Dragging a stock-attached endpoint retains the old stale-until-release preview -- scoped out and filed as #830.Verification
Every commit passed the full pre-commit gamut (Rust fmt/clippy/tests, TS lint/tsc/build/tests, pysimlin). Diagram suite grew from 1322 to 1376 tests; each fix's new tests were verified to fail against the pre-fix code. Browser verification of the caret fix was done against the real KaTeX DOM pipeline; the dev-server API returns 401 to the automation browser session, so the remaining fixes rely on the gesture-suite tests driving real pointer/keyboard events through the full component tree -- worth a quick manual pass on /bpowers/population (fraction clicks, Cmd+Enter, double-click rename, connector warnings, cloud drags).
Generated with Claude Code
Session: https://claude.ai/code/session_01591tEkjitjUfd5wrtSmr6u