diagram: make the canvas viewport local during gestures#734
Conversation
Pull the pan/zoom/pinch/momentum arithmetic out of Canvas.tsx into a new pure module drawing/viewport.ts (the functional core): wheel pan/zoom, zoom-around-cursor, pinch zoom, momentum friction, release-velocity estimation, and resize. Canvas keeps the DOM-bound work -- screen->canvas mapping, the rAF loop, and React state -- and calls these pure transforms. This is a behavior-preserving refactor (viewport updates still round-trip to the controller per frame); it exists to make the upcoming live-viewport change test-driven, since the math can now be unit-tested without jsdom. Part of the issue #707 work.
Replace the offset-only `movingCanvasOffset` local state with a unified
`liveViewport {x, y, zoom}`. Offset and zoom now both resolve from the live
viewport while a gesture is in flight (via getCanvasOffset/getCanvasZoom) and
from props.view otherwise. This gives pinch and wheel-zoom a local home for
the in-progress zoom, which they previously lacked -- the prerequisite for
making those paths fully local.
No behavior change: drag-pan still commits on pointer-up, and since a pan
never changes zoom the live zoom equals props.view.zoom throughout.
Part of the issue #707 work.
The momentum rAF loop now writes the local live viewport each frame instead of round-tripping every frame through the controller (the per-frame engine serialize/re-parse that issue #707 is about). A coasted pan commits exactly once, at the coast's natural end; startMomentumAnimation returns whether a coast actually started so pointer-up commits immediately only for a stationary release. The two paths are mutually exclusive, so a gesture commits once. Also anchor each drag-pan against the offset captured at press time (refs.panBaseOffset) rather than props.view.viewBox, so a pan that interrupts an in-flight coast (whose offset is not yet committed) continues from the on-screen position instead of snapping back to the last committed viewBox. Adds deterministic clock/rAF control to the gesture harness (velocity estimation and the coast both read the clock) plus a live-transform accessor, and covers: single-commit-on-settle, live transform during the coast, and the interrupted-coast -> pan continuity. Part of the issue #707 work.
Wheel and trackpad pan/zoom now update the local live viewport on each event and (re)arm a 200ms trailing debounce instead of round-tripping to the controller per event -- the per-frame engine serialize/re-parse that issue #707 targets. The single commit fires once the scroll settles. A new pointer-down cancels the pending debounce so it can't fire mid-gesture; the pan/pinch it starts inherits the live offset and commits the combined result. The unmount cleanup cancels (does not flush) the timer so no commit lands on an unmounting host. Adds a native-wheel dispatch helper to the harness and covers, under Jest fake timers, wheel pan/zoom single-commit-on-settle, burst re-arming, and a wheel interrupting a momentum coast (no stray commit, continues from the coasted offset). Part of the issue #707 work.
handlePinchMove now writes the local live viewport each move instead of round-tripping to the controller per move; the single commit fires when the second finger lifts. Pinch start anchors its fixed reference (initial zoom and the model point under the fingers) against the live viewport rather than props.view, so a pinch that follows an uncommitted pan keeps its place, and it no longer clears the live viewport on entry. Updates the pinch test to assert the live transform zooms during the move with no commit, and exactly one commit on exit. Part of the issue #707 work.
handleSvgResize now branches on whether a viewport gesture is in flight: when the live viewport is set it folds the re-centering offset shift into it (no commit), so the gesture's own settle commit carries the new size; when idle it commits immediately as before. Committing during a gesture would register as an external view change and snap the gesture. Resize is also gated to non-embedded mode (embedded draws to tight element bounds and ignores viewBox). Makes the harness ResizeObserver triggerable (jsdom never fires real ones) and covers idle-resize-commits and resize-mid-pan-folds-without-snapping. Part of the issue #707 work.
While a gesture owns the live viewport, props.view is expected to stay put (a gesture never commits mid-flight). If its offset/zoom value nonetheless changes -- centerVariable, module navigation, or an undo that restored a different viewport -- that external view must win. A new effect compares props.view by value against a baseline tracked while idle and, when a change is seen with the live viewport still set, drops the live viewport and cancels any pending momentum/wheel commit so the abandoned gesture leaves no stale commit. A self-commit clears the live viewport in the same React commit as its optimistic props.view update, so it is never misread as external; comparing by value (not snapshot identity) ignores a content-equal republished view. Adds a setViewport helper to the harness and covers the centerVariable/ navigation race against a pending wheel commit. Part of the issue #707 work.
Adds the issue #707 invariants to src/diagram/CLAUDE.md (live-viewport ownership + single settle commit, and the external-view override) and a test asserting embedded mode is viewport-inert -- wheel and resize never set a live viewport, commit, or produce a content transform (embedded draws to a tight viewBox attribute). Also adds a Pull Requests section to the root CLAUDE.md: PR descriptions must carry a GitHub 'Fixes #<n>' closing keyword for each resolved issue (kept in the PR body, not commit messages, so the auto-close fires once on merge). Part of the issue #707 work.
![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: 00a047d4e8
ℹ️ 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".
| // external view change and snap the gesture (see the external-override | ||
| // effect). | ||
| const adjusted = resizeViewBox(live, dWidth, dHeight, contentRect.width, contentRect.height); | ||
| setLiveViewport({ x: adjusted.x, y: adjusted.y, zoom: live.zoom }); |
There was a problem hiding this comment.
When the canvas resizes during an active viewport gesture, this branch computes the resized width/height but stores only x, y, and zoom in liveViewport. The later commitLiveViewport() rebuilds the committed viewBox from props.view.viewBox, so the resize is settled with the old dimensions. In split-pane/detail-panel resize while panning, pinching, or before wheel debounce fires, the controller keeps stale viewport dimensions, which affects subsequent centering/zoom calculations that read view.viewBox.width/height.
Useful? React with 👍 / 👎.
| // the next viewport commit. viewBox is presentational and not independently | ||
| // saved, so this is benign.) | ||
| stopMomentumAnimation(); | ||
| cancelWheelCommit(); |
There was a problem hiding this comment.
If a user wheel-pans/zooms and then clicks an element before the 200 ms debounce fires, this unconditionally cancels the only scheduled settle commit. A normal click/selection path does not call commitLiveViewport(), so onViewBoxChange never reaches the controller until some later viewport gesture; the screen stays moved via local state, but persistence and any host logic reading props.view keep using the old viewport.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #734 +/- ##
=======================================
Coverage 90.60% 90.60%
=======================================
Files 224 224
Lines 136247 136247
=======================================
Hits 123447 123447
Misses 12800 12800 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code review for PR #734[P2] Resize during a gesture: the fold is overwritten by subsequent gesture mathFile:
Overall correctness verdictThe patch is functionally correct for the primary issue #707 goal (single-commit-per-gesture) and the tests cover all the main paths. The resize-fold issue above is a real but edge-case correctness gap; it does not block merge, but the intended fold behavior will not survive past the next move/frame. |
…ommits Three fixes from PR #734 review: - commitLiveViewport sources viewBox width/height from the live svgSize rather than props.view.viewBox, so a resize that fires mid-gesture settles with the current dimensions instead of stale ones (centerVariable and friends read view.viewBox.width/height). - A mid-gesture resize no longer folds a re-centering offset shift into the live viewport. That shift was immediately discarded by the next pan move / momentum frame (which recompute the offset from their press-time anchor), and shifting the offset out from under an active gesture is wrong anyway. The gesture now keeps full control of the offset; only svgSize updates, and the settle commit carries the new size. An idle resize still re-centers and commits immediately. - Generalize the wheel trailing-debounce into a guarded 'deferred commit' for an ORPHANED live viewport and also arm it when a press interrupts a momentum coast. handlePointerDown no longer cancels it. Its callback commits only when no pan/pinch/coast is active, so a plain click between a wheel scroll (or a coast) and the timer persists the viewport instead of stranding it in local state, with no double commit. Before this, a click interrupting a coast (or a pending wheel) left props.view stale until the next viewport gesture. Tests: resize-during-pan now asserts the offset is untouched, the pan continues from its anchor after the resize, and the commit carries the new dimensions; new tests cover a plain click persisting a pending wheel offset and an interrupted coast. Part of the issue #707 work.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be3ef71f03
ℹ️ 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 { zoom: newZoom, changed } = wheelZoom(zoom, e.deltaY); | ||
| if (!changed) { | ||
| return; |
There was a problem hiding this comment.
When a momentum coast is active and the user sends a ctrl/meta wheel zoom that is already clamped at MIN_ZOOM or MAX_ZOOM, handleNativeWheel has just stopped the momentum loop, but this early return skips scheduleDeferredCommit(). That leaves the coasted liveViewport with no remaining settle path, so the visible pan can be lost instead of being sent to onViewBoxChange; schedule a deferred commit (or avoid stopping the coast) when a clamped zoom event interrupts an existing live viewport.
Useful? React with 👍 / 👎.
|
[P3] Wheel-zoom-at-limit during a momentum coast can leave the live viewport orphaned In Overall correctness verdict: correct. The PR is well-tested and the recently-addressed review feedback resolves the more significant prior gaps (mid-resize offset corruption, orphaned-wheel state). The remaining finding above is a narrow edge case that doesn't block correctness for the common paths. |
A ctrl/meta wheel zoom already clamped at MIN/MAX_ZOOM is a no-op, but handleNativeWheel had already stopped any momentum coast before handleNativeWheelZoom returned early -- leaving the coasted live viewport with no settle path, so the visible pan could be lost. Centralize coast interruption in interruptCoast(): it stops the coast and, when one was actually running, arms the deferred commit. handlePointerDown and handleNativeWheel both route through it, so the orphaned offset always has a settle path (re-armed by a moving wheel/pan, skipped when a pan/pinch/coast takes over, or fired by the timer for a click or a clamped no-op wheel) -- and no caller that stops a coast can silently drop it. Part of the issue #707 work.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdbfa71dd2
ℹ️ 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".
| stopMomentumAnimation(); | ||
| cancelDeferredCommit(); | ||
| setLiveViewport(undefined); |
There was a problem hiding this comment.
When an external view change arrives during an active drag-pan or pinch, this effect only clears liveViewport; it leaves interaction, panBaseOffset, and the pinch reference data intact. If the user keeps moving the pointer after a center/navigation/undo update, handleMovingCanvas or handlePinchMove will recreate liveViewport from the stale gesture baseline and the later pointer-up can commit that abandoned gesture over the external view, so the external viewport does not actually win in active pointer gestures.
Useful? React with 👍 / 👎.
Code reviewI reviewed the diff (10 commits, ~1.5k additions) covering the canvas viewport refactor for issue #707. The design is well-considered, the pure/imperative split (drawing/viewport.ts vs Canvas.tsx) is clean, and the test coverage spans the meaningful interaction sequences (single-commit-on-settle, interrupted-coast-continuity, burst re-arm, mid-gesture resize folding, external view override, embedded inertness, plain-click-doesn't-strand-pending-commit, and the clamped-zoom + interrupted-coast settle path that motivated I traced the key state transitions and timer interactions, including:
The math in viewport.ts (wheelPanOffset / wheelZoom / zoomAroundPoint / pinchOffset / friction model) is internally consistent and matches the prior in-line code. I did not identify any discrete actionable bugs to flag. Overall correctness verdict: correct. |
The external-view override effect cleared liveViewport but left the in-flight pointer gesture's state (interaction mode, panBaseOffset, pinch reference, activePointers) intact. If an external view change (centerVariable, navigation, undo) arrived mid drag-pan or pinch and the user kept moving, handleMovingCanvas / handlePinchMove would recreate liveViewport from the now-stale press-time anchor and the pointer-up would commit that abandoned gesture back over the external view -- so the external view did not actually win. When the override fires and a viewport pointer gesture (panning or pinching) is still in progress, also reset the interaction to idle and drop the pointer anchors (mouseDownPoint/panBaseOffset/pointerId/activePointers). That makes the continued move a no-op and the release a clean no-commit, so the external view wins. Non-viewport gestures don't touch liveViewport and are left alone. Part of the issue #707 work.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
|
Review verdict I reviewed the changes against the issue 707 goal (one settle-time commit per gesture) and walked through the gesture interactions (pan, momentum, wheel pan/zoom, pinch, resize) plus the external-view override path. The architecture is clean -- pure math in viewport.ts, lifecycle in the shell -- and the single-commit contract is rigorously enforced across every gesture tail: pan release with no momentum (commitLiveViewport in handlePointerCancel), momentum natural end (commitLiveViewport in animateMomentum), wheel/trackpad (scheduleDeferredCommit 200ms trailing debounce re-armed per event, guarded callback skips when a viewport gesture is now active), and pinch exit (commitLiveViewport in handlePointerCancel). interruptCoast correctly arms a deferred commit only when something actually was coasting, so a non-viewport press between a coast and the timer still persists the viewport. I traced several potentially risky scenarios: an orphaned wheel timer firing while a different gesture is now in flight (guarded by viewportGestureActive), a coast interrupted by another viewport gesture vs by a click (correctly differentiated; the new gesture inherits via panBaseOffset or the pinch reference reads, a click lets the deferred commit fire), mid-gesture resize (only updates svgSize; commit reads the new dimensions at settle), external view change during pan/pinch (drops the live viewport AND neutralizes the in-flight gesture by resetting interaction to idle and clearing pointer anchors, so a continued move/release cannot re-write the live viewport). The StrictMode mount/unmount/mount cycle is covered by the symmetric cleanup that cancels the deferred timer and momentum animation. Test coverage is thorough -- the new viewport.test.ts covers the pure math, and the reconciler-level gesture tests assert both the single committed viewBox AND that the live g transform updates in between, including interrupted-coast continuity, wheel-zoom clamped-to-no-op interrupting a coast, the abandoned-gesture override semantics, and resize-during-pan. No findings. Overall correctness verdict: Correct. |
1 participant
Summary
Implements Option C for issue #707: the canvas now owns its viewport (offset+zoom) locally during a gesture and notifies the controller (
onViewBoxChange->queueViewUpdate) exactly once, on settle, instead of once per animation frame / wheel event. This removes the full engine round-trip --applyPatch+serializeProtobuf+serializeJson(includeStdlib)+projectFromJson+preserveLiveView-- that previously ran on every pan/zoom frame.Drag-pan already committed once on release; this generalizes that pattern to every viewport path (momentum, wheel/trackpad pan+zoom, pinch, resize) and unifies them behind a single local
liveViewport {x, y, zoom}.Fixes #707.
What changed
drawing/viewport.ts(functional core): wheel pan/zoom, zoom-around-cursor, pinch, momentum friction, release-velocity, resize -- unit-tested without jsdom.Canvas.tsxholds offset+zoom in localliveViewportand renders from it;getCanvasOffset()/getCanvasZoom()resolve from it when set, elseprops.view. OnecommitLiveViewport()is the settle-time commit for all paths:refs.panBaseOffsetso a pan interrupting a coast doesn't snap back to the last committed viewBox.props.viewchange (centerVariable / navigation / undo) observed mid-gesture overrides the gesture (drops the live viewport, cancels pending commits).Testing
tests/viewport.test.tscovers the pure math.onViewBoxChangeexactly once (on settle) and the rendered<g transform>updates live in between -- across pan, momentum (single-commit + interrupted-coast continuity), wheel pan/zoom (debounce + burst re-arm + momentum interruption), pinch (commit on exit), resize (fold vs commit), embedded inertness, and the external-view override.Notes / follow-up
The controller is unchanged:
queueViewUpdate's rebuild still runs once per gesture. Issue #707's "Option B" (skipping the full project rebuild for view-only updates) remains a possible follow-up but is out of scope here.