feat(editor): add shift-based angle snapping for line drawing and anchor dragging #14244
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds angle-snapping: new SnapOverlay.snapDragAngle snaps drag vectors to 15° increments when Shift is held; InteractivityManager now instantiates and uses SnapOverlay across move/rotate/resize flows; tests and an index re-export added. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant IM as InteractivityManager
participant SO as SnapOverlay
participant Gfx as GfxRenderer
User->>IM: pointer down (startPoint)
User->>IM: pointer move (currentPoint, Shift)
IM->>SO: snapDragAngle(startPoint, currentPoint, Shift)
SO-->>IM: snappedPoint
IM->>Gfx: apply drag delta (snappedPoint - startPoint)
Gfx-->>IM: render/update
User->>IM: pointer up (endPoint)
IM->>SO: snapDragAngle(startPoint, endPoint, Shift)
SO-->>IM: finalSnappedPoint
IM->>Gfx: finalize drag with snapped delta
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts:
- Around line 54-84: The snapDragAngle method is implemented but never used;
integrate it into the pointer/drag handlers that compute drag points so
SHIFT-driven 15° snapping actually occurs, e.g., call snapDragAngle(startPoint,
currentPoint, isShiftPressed) from the line-drawing pointer move/drag handler
and the anchor-drag handler where currentPoint is computed and isShiftPressed
comes from keyboard state; add a unit test for SnapOverlay.snapDragAngle
covering several angles and zero-distance, and add an e2e test that simulates
holding Shift while dragging a line/anchor to assert the resulting angle is
snapped to 15° increments.
🧹 Nitpick comments (1)
blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts (1)
54-84: Tighten numeric guard + avoid recreating the interval constant per callThe snapping logic is good, but I’d recommend (1) guarding on
distancewith a small epsilon (instead ofdx === 0 && dy === 0), (2) fixing the “division by zero” comment, and (3) hoisting the interval to astatic readonly(or module constant) for reuse. Also consider whether returningcurrentPointby reference (when Shift isn’t pressed) is intended vs always returning a newPointfor consistency.Proposed diff
export class SnapOverlay extends Overlay { static override overlayName: string = 'snap-manager'; + private static readonly ANGLE_SNAP_INTERVAL_RAD = Math.PI / 12; // 15 degrees + private static readonly SNAP_EPSILON = 1e-6; + private _skippedElements: Set<GfxModel> = new Set(); @@ snapDragAngle(startPoint: Point, currentPoint: Point, isShiftPressed: boolean): Point { if (!isShiftPressed) { return currentPoint; } - const ANGLE_SNAP_INTERVAL_RAD = Math.PI / 12; // 15 degrees in radians - const dx = currentPoint.x - startPoint.x; const dy = currentPoint.y - startPoint.y; - if (dx === 0 && dy === 0) { - return currentPoint; // Avoid division by zero when points are identical - } + const distance = Math.hypot(dx, dy); + if (distance < SnapOverlay.SNAP_EPSILON) { + return currentPoint; // No effective movement; avoid snapping jitter + } const currentAngle = Math.atan2(dy, dx); - const snappedAngle = Math.round(currentAngle / ANGLE_SNAP_INTERVAL_RAD) * ANGLE_SNAP_INTERVAL_RAD; - const distance = Math.sqrt(dx * dx + dy * dy); + const interval = SnapOverlay.ANGLE_SNAP_INTERVAL_RAD; + const snappedAngle = Math.round(currentAngle / interval) * interval; const snappedX = startPoint.x + distance * Math.cos(snappedAngle); const snappedY = startPoint.y + distance * Math.sin(snappedAngle); return new Point(snappedX, snappedY); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts
🧰 Additional context used
🧬 Code graph analysis (1)
blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts (1)
tests/blocksuite/e2e/utils/actions/edgeless.ts (1)
Point(53-53)
| /** | ||
| * Snaps a current point to an angle interval relative to a start point if the shift key is pressed. | ||
| * | ||
| * @param startPoint The starting point of the drag. | ||
| * @param currentPoint The current point of the drag. | ||
| * @param isShiftPressed True if the SHIFT key is currently pressed, false otherwise. | ||
| * @returns The snapped point if shift is pressed, otherwise the original currentPoint. | ||
| */ | ||
| snapDragAngle(startPoint: Point, currentPoint: Point, isShiftPressed: boolean): Point { | ||
| if (!isShiftPressed) { | ||
| return currentPoint; | ||
| } | ||
|
|
||
| const ANGLE_SNAP_INTERVAL_RAD = Math.PI / 12; // 15 degrees in radians | ||
|
|
||
| const dx = currentPoint.x - startPoint.x; | ||
| const dy = currentPoint.y - startPoint.y; | ||
|
|
||
| if (dx === 0 && dy === 0) { | ||
| return currentPoint; // Avoid division by zero when points are identical | ||
| } | ||
|
|
||
| const currentAngle = Math.atan2(dy, dx); | ||
| const snappedAngle = Math.round(currentAngle / ANGLE_SNAP_INTERVAL_RAD) * ANGLE_SNAP_INTERVAL_RAD; | ||
| const distance = Math.sqrt(dx * dx + dy * dy); | ||
|
|
||
| const snappedX = startPoint.x + distance * Math.cos(snappedAngle); | ||
| const snappedY = startPoint.y + distance * Math.sin(snappedAngle); | ||
|
|
||
| return new Point(snappedX, snappedY); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find call sites for the new API
echo "=== Call sites for snapDragAngle ==="
rg -n --hidden --no-ignore -S '\.snapDragAngle\s*\(' || echo "No call sites found"
# 2) Find where Shift-modifier state is read in pointer/drag handling
echo -e "\n=== Shift-modifier handling in pointer/src ==="
rg -n --hidden --no-ignore -S '\b(shiftKey|isShiftPressed|modifiers)\b' blocksuite/affine/gfx/pointer/src || echo "No modifier handling found"
# 3) Look for test files
echo -e "\n=== Test files for snap ==="
find . -name "*snap*test*" -o -name "*snap*spec*" | head -20Repository: toeverything/AFFiNE
Length of output: 768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check e2e snap tests
echo "=== E2E snap tests ==="
cat ./tests/blocksuite/e2e/edgeless/snap.spec.ts | head -100
echo -e "\n=== Integration test snap tests ==="
cat ./blocksuite/integration-test/src/__tests__/main/snapshot.spec.ts | grep -A5 -B5 "snap" | head -50
# Search for any drag handling that might need to call snapDragAngle
echo -e "\n=== Drag handling in pointer/src ==="
rg -n -S '\b(drag|snap)\b' blocksuite/affine/gfx/pointer/src --type ts | head -20
# Check if snapDragAngle is referenced in any test file
echo -e "\n=== References to snapDragAngle in tests ==="
rg -n 'snapDragAngle' . --type ts || echo "No test references found"Repository: toeverything/AFFiNE
Length of output: 3851
Method is not integrated—add integration point and tests before merge
The snapDragAngle method exists but has no call sites in the codebase. Shift-key modifier wiring must be added to the line-drawing and/or anchor-drag flows that should trigger 15° angle snapping. Additionally, at least one unit test (for the method logic) and one e2e test (for the shift+drag behavior) must be added.
🤖 Prompt for AI Agents
In @blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts around lines 54 - 84,
The snapDragAngle method is implemented but never used; integrate it into the
pointer/drag handlers that compute drag points so SHIFT-driven 15° snapping
actually occurs, e.g., call snapDragAngle(startPoint, currentPoint,
isShiftPressed) from the line-drawing pointer move/drag handler and the
anchor-drag handler where currentPoint is computed and isShiftPressed comes from
keyboard state; add a unit test for SnapOverlay.snapDragAngle covering several
angles and zero-distance, and add an e2e test that simulates holding Shift while
dragging a line/anchor to assert the resulting angle is snapped to 15°
increments.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
392-471: Fix snapped-move vs unsnapped-end mismatch (can cause jump on pointerup).While Shift is pressed you snap
dx/dyduringonDragMove, butonDragEndstill commitsdx/dyfrom the raw pointer end position. If the last move frame was snapped, the committed position can differ from what the user saw during drag. Also, when snapped,dragLastPosstays raw whiledx/dyare snapped, which is internally inconsistent for consumers.Proposed fix (compute an “effective” drag position for both move and end)
const onDragMove = (event: PointerEvent) => { - dragLastPos = Point.from( - this.gfx.viewport.toModelCoordFromClientCoord([event.x, event.y]) - ); - - let dx = dragLastPos.x - internal.dragStartPos.x; - let dy = dragLastPos.y - internal.dragStartPos.y; - - const isShiftPressed = this.keyboard.shiftKey$.peek(); - if (isShiftPressed) { - const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, true); - dx = snappedPoint.x - internal.dragStartPos.x; - dy = snappedPoint.y - internal.dragStartPos.y; - } + const rawPos = Point.from( + this.gfx.viewport.toModelCoordFromClientCoord([event.x, event.y]) + ); + const isShiftPressed = this.keyboard.shiftKey$.peek(); + const effectivePos = isShiftPressed + ? this.snapOverlay.snapDragAngle(internal.dragStartPos, rawPos, true) + : rawPos; + + dragLastPos = effectivePos; + const dx = effectivePos.x - internal.dragStartPos.x; + const dy = effectivePos.y - internal.dragStartPos.y; const moveContext: ExtensionDragMoveContext = { ...internal, event, dragLastPos, dx, dy, }; @@ const onDragEnd = (event: PointerEvent) => { @@ - dragLastPos = Point.from( - this.gfx.viewport.toModelCoordFromClientCoord([event.x, event.y]) - ); + const rawPos = Point.from( + this.gfx.viewport.toModelCoordFromClientCoord([event.x, event.y]) + ); + const isShiftPressed = this.keyboard.shiftKey$.peek(); + const effectivePos = isShiftPressed + ? this.snapOverlay.snapDragAngle(internal.dragStartPos, rawPos, true) + : rawPos; + + dragLastPos = effectivePos; const endContext: ExtensionDragEndContext = { ...internal, event, dragLastPos, - dx: dragLastPos.x - internal.dragStartPos.x, - dy: dragLastPos.y - internal.dragStartPos.y, + dx: effectivePos.x - internal.dragStartPos.x, + dy: effectivePos.y - internal.dragStartPos.y, };(Also, since
viewportWatcherreplaysonDragMove(lastEvent), ensurelastEventis updated insideonDragMoveif the intent is to reapply the latest pointer location after viewport changes.)
🤖 Fix all issues with AI agents
In @blocksuite/framework/std/src/gfx/interactivity/manager.ts:
- Line 5: The file imports SnapOverlay via a deep path which breaks package
boundaries; remove the deep import in
blocksuite/framework/std/src/gfx/interactivity/manager.ts and either (A)
re-export SnapOverlay from the public API of @blocksuite/affine-gfx-pointer (add
an export for ./snap or include it in the main export so you can import {
SnapOverlay } from '@blocksuite/affine-gfx-pointer'), or (B) move the
snap-related logic into @blocksuite/std and use a local SnapOverlay
implementation, or (C) register SnapOverlay as an extension similarly to
PointerViewExtension and obtain it via the extension registry instead of
importing it directly; update references to SnapOverlay in the
InteractivityManager to use the chosen public API/extension/local
implementation.
🧹 Nitpick comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
65-70: MakesnapOverlayreadonly.
snapOverlayis assigned only in the constructor and never reassigned, so it should bereadonly. WhileSnapOverlayinherits adispose()method from its parentOverlayclass, it does not currently attach listeners or allocate resources requiring cleanup. However, for consistency with the extension lifecycle pattern, consider callingthis.snapOverlay.dispose()inInteractivityManager.unmounted()if resource management needs become necessary in the future.Proposed change
- private snapOverlay: SnapOverlay; + private readonly snapOverlay: SnapOverlay;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @blocksuite/affine/gfx/pointer/src/__tests__/snap-overlay.unit.spec.ts:
- Around line 67-76: The cardinal-direction unit tests for
snapOverlay.snapDragAngle assert truncated coordinates that contradict the
function's distance-preserving behavior; update the expectations in the tests
that call snapOverlay.snapDragAngle (the 0°, 90°, and 180° cases referencing
startPoint/currentPoint) to assert coordinates that preserve the original
distance (use the computed radius ≈ Math.hypot(10,2) and assert x/y near ±radius
or 0 accordingly, e.g., ~10.198 on the axis) instead of fixed integers like 10
or 0 so they match the distance preservation validated by the existing distance
test.
🧹 Nitpick comments (3)
blocksuite/affine/gfx/pointer/src/__tests__/snap-overlay.unit.spec.ts (3)
46-51: Clarify the test and verify distance preservation.The trigonometric expression
10 * Math.tan(Math.PI / 4 + Math.PI / 100)is complex and could be clearer. Consider extracting it to a named constant or adding a comment explaining it represents a point near 45°.Additionally, similar to the 0° test, verify that the expected snapped point
(10, 10)matches the implementation's distance-preserving behavior. The original distance issqrt(100 + (10*tan(46.8°))^2) ≈ 14.55, so the snapped point should be approximately(10.29, 10.29)to preserve distance.
78-83: Consider improving test clarity.The test correctly verifies snapping to -15° (345°), but the inline trigonometric calculations make it harder to read. Consider extracting the angle and radius to named constants:
const targetAngle = -Math.PI / 12; // -15 degrees const offsetAngle = -Math.PI / 100; // small offset for testing snap behavior const radius = 10; const inputAngle = targetAngle + offsetAngle; const currentPoint = new Point( radius * Math.cos(inputAngle), radius * Math.sin(inputAngle) );This would make the test's intent clearer without changing its behavior.
22-84: Consider expanding test coverage for other 15° increments.The test suite covers key angles (0°, 45°, 90°, 180°, -15°), but since the PR implements snapping to 15° increments, consider adding tests for other multiples like 15°, 30°, 60°, 75°, 120°, 135°, etc. This would provide more comprehensive verification that the snapping works correctly across all angle ranges.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/affine/gfx/pointer/src/__tests__/snap-overlay.unit.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
blocksuite/affine/gfx/pointer/src/__tests__/snap-overlay.unit.spec.ts (3)
blocksuite/affine/gfx/pointer/src/snap/snap-manager.ts (1)
snapOverlay(16-20)blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts (1)
SnapOverlay(39-848)tests/blocksuite/e2e/utils/actions/edgeless.ts (1)
Point(53-53)
🔇 Additional comments (6)
blocksuite/affine/gfx/pointer/src/__tests__/snap-overlay.unit.spec.ts (6)
1-3: LGTM! Imports are appropriate.The imports correctly bring in the necessary types and classes for testing the SnapOverlay angle-snapping functionality.
25-30: LGTM! Correctly tests the no-snap case.The test properly verifies that when Shift is not pressed, the current point is returned unchanged.
32-37: LGTM! Good edge case coverage.This test correctly handles the edge case where start and current points are identical, preventing potential division-by-zero issues in angle calculations.
53-58: LGTM, but verify distance preservation consistency.The 90° vertical snap test is well-structured. However, like the horizontal test, ensure the expected values align with the distance-preserving behavior of
snapDragAngle. Original distance:sqrt(4 + 100) ≈ 10.198, expected snapped point:(0, 10)with distance10.
60-65: LGTM, but verify distance preservation consistency.The 180° horizontal-left snap test is correct in structure. Similar to other cardinal direction tests, verify the expected snapped point
(-10, 0)is consistent with distance preservation. Original distance:sqrt(100 + 4) ≈ 10.198, snapped distance:10.
9-20: The mock GfxController is sufficient for testingsnapDragAnglebut incomplete for broader usage.The
snapDragAnglemethod doesn't access anyGfxControllerproperties—it operates purely on thestartPoint,currentPoint, andisShiftPressedparameters. However, otherSnapOverlaymethods usethis.gfx.viewport.zoom,this.gfx.viewport.viewportBounds, andthis.gfx.grid.search(). The mock omitsviewport.viewportBounds, which is used in alignment calculations. Theas GfxControllertype cast masks this incompleteness. Consider either expanding the mock to include all properties used bySnapOverlay, or documenting that the mock is scoped only to testsnapDragAngle.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @blocksuite/affine/gfx/pointer/src/index.ts:
- Line 1: The package entry (index.ts) currently only re-exports snap-overlay;
add a re-export for the tools module so existing consumers of EmptyTool and
PanTool still work: update index.ts to export the tools module (i.e., add an
export * from './tools'; alongside export * from './snap/snap-overlay';) so
EmptyTool and PanTool (and their types) are publicly available again.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/affine/gfx/pointer/src/index.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## canary #14244 +/- ##
===========================================
+ Coverage 56.14% 74.86% +18.72%
===========================================
Files 2809 495 -2314
Lines 147733 72341 -75392
Branches 22228 4746 -17482
===========================================
- Hits 82943 54159 -28784
+ Misses 62144 18060 -44084
+ Partials 2646 122 -2524
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
400-408: Consider simplifying the redundant boolean parameter.The
snapDragAnglemethod is only called whenisShiftPressedistrue(Line 404), yet the third parameter is hardcoded totrue(Line 405). Consider one of these approaches:
- Pass the
isShiftPressedvariable directly:snapDragAngle(internal.dragStartPos, dragLastPos, isShiftPressed)- Remove the conditional and always call
snapDragAngle, letting it handle the shift key state internally- If the API requires the boolean, this is fine but slightly redundant
♻️ Proposed simplification
-const isShiftPressed = this.keyboard.shiftKey$.peek(); -if (isShiftPressed) { - const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, true); +if (this.keyboard.shiftKey$.peek()) { + const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek()); dx = snappedPoint.x - internal.dragStartPos.x; dy = snappedPoint.y - internal.dragStartPos.y; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
blocksuite/affine/gfx/pointer/src/__tests__/snap-overlay.unit.spec.tsblocksuite/affine/gfx/pointer/src/index.tsblocksuite/framework/std/src/gfx/interactivity/manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/gfx/pointer/src/tests/snap-overlay.unit.spec.ts
🔇 Additional comments (5)
blocksuite/affine/gfx/pointer/src/index.ts (1)
2-2: LGTM!The re-export cleanly exposes the
SnapOverlayAPI, making it available for use in the interactivity manager.blocksuite/framework/std/src/gfx/interactivity/manager.ts (4)
5-5: LGTM!The import correctly references the newly exported
SnapOverlayAPI.
65-65: LGTM!The private field declaration is appropriate for managing the snap overlay instance.
67-70: LGTM!The constructor properly initializes the
SnapOverlayinstance with the GFX controller. The use of rest parameters withConstructorParameters<typeof GfxExtension>provides better type safety and flexibility.
449-450: Verify that drag end intentionally skips angle snapping.The drag end logic (Lines 449-450) calculates
dxanddydirectly from the raw positions without applying angle snapping, unlike the drag move logic (Lines 400-408). This means the final committed position won't respect the 15° angle snap even if Shift was held at release.Confirm whether this is intentional behavior (e.g., to allow users to release Shift before pointerup for fine-tuning) or if the drag end should also apply snapping when Shift is pressed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
449-450: Apply snapping in drag end to match preview position.The
onDragEndfunction calculatesdxanddywithout applying angle snapping, even when Shift is held at the time of thepointerupevent. This causes the element to jump from the snapped preview position (shown during drag) to an unsnapped final position when the user releases the mouse while still holding Shift.This creates an inconsistent and jarring user experience that undermines the precision benefit of angle snapping.
🔧 Proposed fix
Apply the same snapping logic in
onDragEndto ensure the final position matches the preview:dragLastPos = Point.from( this.gfx.viewport.toModelCoordFromClientCoord([event.x, event.y]) ); + + const snappedPoint = this.snapOverlay.snapDragAngle( + internal.dragStartPos, + dragLastPos, + this.keyboard.shiftKey$.peek() + ); + const endContext: ExtensionDragEndContext = { ...internal, event, dragLastPos, - dx: dragLastPos.x - internal.dragStartPos.x, - dy: dragLastPos.y - internal.dragStartPos.y, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, };
🧹 Nitpick comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
400-408: Remove redundant shift key check.The code evaluates
this.keyboard.shiftKey$.peek()twice: once in theifcondition (line 404) and again when passing it tosnapDragAngle(line 405). According to the SnapOverlay implementation,snapDragAnglereturns the originalcurrentPointwhenisShiftPressedis false, making the outerifcheck unnecessary.♻️ Suggested simplification
Option 1 (recommended): Remove the if check and always call snapDragAngle
- let dx = dragLastPos.x - internal.dragStartPos.x; - let dy = dragLastPos.y - internal.dragStartPos.y; - - - if (this.keyboard.shiftKey$.peek()) { - const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek()); - dx = snappedPoint.x - internal.dragStartPos.x; - dy = snappedPoint.y - internal.dragStartPos.y; - } + const snappedPoint = this.snapOverlay.snapDragAngle( + internal.dragStartPos, + dragLastPos, + this.keyboard.shiftKey$.peek() + ); + const dx = snappedPoint.x - internal.dragStartPos.x; + const dy = snappedPoint.y - internal.dragStartPos.y;Option 2: Pass
truedirectly since we know shift is pressedlet dx = dragLastPos.x - internal.dragStartPos.x; let dy = dragLastPos.y - internal.dragStartPos.y; - - + if (this.keyboard.shiftKey$.peek()) { - const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek()); + const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, true); dx = snappedPoint.x - internal.dragStartPos.x; dy = snappedPoint.y - internal.dragStartPos.y; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
blocksuite/affine/gfx/pointer/src/index.tsblocksuite/framework/std/src/gfx/interactivity/manager.ts
🧰 Additional context used
🧬 Code graph analysis (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)
blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts (1)
SnapOverlay(39-848)blocksuite/framework/std/src/gfx/interactivity/index.ts (1)
ExtensionDragMoveContext(15-15)
🔇 Additional comments (2)
blocksuite/affine/gfx/pointer/src/index.ts (1)
1-2: LGTM! Clean public API exposure.The new snap-overlay export properly exposes the SnapOverlay functionality for consumption by other modules (as seen in InteractivityManager). The structure is clean and maintains consistency with the existing export pattern.
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
5-5: LGTM! Clean SnapOverlay integration.The import and initialization follow proper patterns. The constructor correctly instantiates SnapOverlay with the gfx context, and the type-safe constructor signature using
ConstructorParameters<typeof GfxExtension>is a good practice.Also applies to: 65-70
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 400-407: Remove the stray leading "+" tokens introduced in the
call to snapOverlay.snapDragAngle — locate the expression using
snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos,
this.keyboard.shiftKey$.peek()) and remove any leading "+" characters before the
arguments so the call parses correctly; ensure the subsequent dx and dy
calculations using internal.dragStartPos.x/y remain unchanged and properly
indented.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 399-411: The final drag end handler (onDragEnd) computes dx/dy
from dragLastPos without applying the same angle snapping used during
onDragMove, causing a visual jump when Shift is held; fix by calling
this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos,
this.keyboard.shiftKey$.peek()) inside onDragEnd, compute dx/dy from the
returned snappedPoint (snappedPoint.x - internal.dragStartPos.x, snappedPoint.y
- internal.dragStartPos.y), and use those values when building the
ExtensionDragEndContext so the committed end position matches the snapped
preview.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 65-70: snapOverlay is created once in the constructor while
canvasEventHandler is re-initialized in mounted(), which can leave SnapOverlay's
state stale across remounts; in mounted() either re-instantiate this.snapOverlay
= new SnapOverlay(this.gfx) (matching canvasEventHandler) or call
this.snapOverlay.clear() to reset its internal fields (_skippedElements,
_referenceBounds, _distributedAlignLines, _intraGraphicAlignLines), and ensure
you also clear or destroy snapOverlay in unmounted() to avoid carrying state
between mounts.
🧹 Nitpick comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
441-452: Snapping consistency is now correct; minor indentation inconsistency.The snapping logic in
onDragEndnow matchesonDragMove, addressing the previously flagged issue. However, lines 445–452 appear to use 5-space indentation while the equivalent block inonDragMove(lines 399–405) uses 6 spaces. Consider aligning for consistency.♻️ Suggested fix
- const snappedPoint = this.snapOverlay.snapDragAngle( - internal.dragStartPos, - dragLastPos, - this.keyboard.shiftKey$.peek() - ); - const endContext: ExtensionDragEndContext = { + const snappedPoint = this.snapOverlay.snapDragAngle( + internal.dragStartPos, + dragLastPos, + this.keyboard.shiftKey$.peek() + ); + const endContext: ExtensionDragEndContext = { ...internal, event, dragLastPos, - dx: snappedPoint.x - internal.dragStartPos.x, - dy: snappedPoint.y - internal.dragStartPos.y, - }; + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, + };
Make snapOverlay optional and initialize it in mounted.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
64-94: Critical: Multiple issues in lifecycle methods will cause runtime failures.
Undefined
CanvasEventHandler(line 73):CanvasEventHandleris not imported—onlyGfxViewEventManageris imported at line 27. This will throw aReferenceErrorat runtime.Duplicate
unmounted()methods (lines 76–82 and 89–94): The class definesunmounted()twice. TypeScript/JavaScript will only use the second definition (lines 89–94), making the first one (lines 76–82) dead code. The cleanup logic forsnapOverlayandcanvasEventHandlerwill never execute.Type mismatch (line 81):
canvasEventHandleris declared as a non-optional field at line 64, but line 81 attempts to assignundefined.🐛 Proposed fix: Consolidate lifecycle methods and fix the constructor reference
- private canvasEventHandler = new GfxViewEventManager(this.gfx); + private canvasEventHandler?: GfxViewEventManager; private snapOverlay?: SnapOverlay; constructor(...args: ConstructorParameters<typeof GfxExtension>) { super(...args); } mounted() { this.snapOverlay = new SnapOverlay(this.gfx); - this.canvasEventHandler = new CanvasEventHandler(this.gfx, this.snapOverlay); + this.canvasEventHandler = new GfxViewEventManager(this.gfx); } - unmounted() { - this.snapOverlay?.clear(); - this.snapOverlay = undefined; - - this.canvasEventHandler?.destroy(); - this.canvasEventHandler = undefined; - } - - // ... other code ... - override unmounted(): void { + this.snapOverlay?.clear(); + this.snapOverlay = undefined; + this.canvasEventHandler?.destroy(); + this.canvasEventHandler = undefined; + this._disposable.dispose(); this.interactExtensions.forEach(ext => { ext.unmounted(); }); }Note: If
CanvasEventHandleris a new class that should acceptsnapOverlay, ensure it is imported and that the constructor signature matches.
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 446-457: Guard against a possibly undefined snapOverlay in the
drag end handler by checking snapOverlay before calling snapDragAngle (e.g., use
optional chaining or an early return) so that when this.snapOverlay is undefined
you do not call this.snapOverlay.snapDragAngle; update the construction of
endContext (symbols: snapOverlay, snapDragAngle, internal, dragLastPos,
endContext) to compute dx/dy using the snappedPoint only if available (fall back
to dragLastPos - internal.dragStartPos otherwise), and fix the inconsistent
indentation around the closing parenthesis and the endContext block to match the
surrounding code style.
- Around line 404-410: Guard against undefined snapOverlay in handleElementMove
by checking it before calling snapDragAngle: either return early or assert
snapOverlay exists; replace direct access to this.snapOverlay.snapDragAngle with
a safe check (e.g., if (!this.snapOverlay) return or throw) or use optional
chaining and a fallback path that computes dx/dy without snapping. Update
references around snapOverlay, snapDragAngle, internal.dragStartPos, and
dragLastPos so dx/dy are always computed safely even when mounted() hasn't run.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
71-94: Critical: Duplicateunmounted()methods cause resource leak.There are two
unmounted()methods defined in this class:
- Lines 76-82: Clears
snapOverlayand destroyscanvasEventHandler- Lines 89-94: Disposes
_disposableand unmounts extensions (withoverridekeyword)The second method completely shadows the first, meaning the cleanup logic for
snapOverlayandcanvasEventHandlerwill never execute, causing memory leaks and stale state.Additionally, line 73 references
CanvasEventHandlerwhich is not imported—onlyGfxViewEventManageris imported (line 27).🐛 Proposed fix: Merge the two unmounted methods and fix the class reference
mounted() { this.snapOverlay = new SnapOverlay(this.gfx); - this.canvasEventHandler = new CanvasEventHandler(this.gfx, this.snapOverlay); + this.canvasEventHandler = new GfxViewEventManager(this.gfx); } - - unmounted() { - this.snapOverlay?.clear(); - this.snapOverlay = undefined; - - this.canvasEventHandler?.destroy(); - this.canvasEventHandler = undefined; - } - // ... other code ... - override unmounted(): void { + this.snapOverlay?.clear(); + this.snapOverlay = undefined; + this._disposable.dispose(); this.interactExtensions.forEach(ext => { ext.unmounted(); }); }Note: If
GfxViewEventManagerneeds thesnapOverlayreference, you'll need to update its constructor signature or pass it via a different mechanism.
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 64-65: The field canvasEventHandler is declared non-optional but
elsewhere (during unmount/cleanup) is assigned undefined, causing a TypeScript
type error; make canvasEventHandler optional or nullable (e.g.,
canvasEventHandler?: GfxViewEventManager or canvasEventHandler:
GfxViewEventManager | undefined) and remove the inline instantiation so that you
initialize it in mounted() (new GfxViewEventManager(this.gfx)) and set it to
undefined during unmount/cleanup, ensuring all uses guard for its presence.
♻️ Duplicate comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
404-410: Guard against undefinedsnapOverlayin drag move handler.
snapOverlayis declared as optional (line 65), but line 404 accesses it directly without optional chaining. The drag end handler (line 446) correctly uses?.but the drag move handler doesn't.🔧 Suggested fix: Add optional chaining with fallback
- const snappedPoint = this.snapOverlay.snapDragAngle( + const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() - ); + ) ?? dragLastPos; const dx = snappedPoint.x - internal.dragStartPos.x; const dy = snappedPoint.y - internal.dragStartPos.y;
🧹 Nitpick comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
446-460: Redundant null-coalescing onfinalPoint.Line 450 already guarantees
snappedPointis non-nullish via?? dragLastPos. Line 452'ssnappedPoint ?? dragLastPosis therefore redundant and adds confusion.♻️ Suggested simplification
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() ) ?? dragLastPos; - const finalPoint = snappedPoint ?? dragLastPos; - const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, };
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
127-128: Guard against undefinedcanvasEventHandleraccess.
canvasEventHandleris declared as optional (?), but line 128 accesses it directly without optional chaining. IfdispatchEventis invoked beforemounted(), this will throw a runtime error.🔧 Suggested fix
const handledByView = - this.canvasEventHandler.dispatch(eventName, evt) ?? false; + this.canvasEventHandler?.dispatch(eventName, evt) ?? false;
♻️ Duplicate comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
401-407: Guard against undefinedsnapOverlayin drag move handler.
snapOverlayis declared as optional (?), but line 401 accesses it directly without optional chaining. The drag end handler (line 443) correctly usesthis.snapOverlay?.snapDragAngle(...) ?? dragLastPos, but the drag move handler does not. This inconsistency could cause a runtime error ifonDragMoveis invoked beforemounted().🔧 Suggested fix: Add optional chaining with fallback
- const snappedPoint = this.snapOverlay.snapDragAngle( + const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() - ); + ) ?? dragLastPos; const dx = snappedPoint.x - internal.dragStartPos.x; const dy = snappedPoint.y - internal.dragStartPos.y;
🧹 Nitpick comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
443-457: Remove redundantfinalPointvariable.
snappedPointat line 443-447 already has a fallback:this.snapOverlay?.snapDragAngle(...) ?? dragLastPos. Therefore,snappedPointis guaranteed to be defined, makingconst finalPoint = snappedPoint ?? dragLastPosat line 449 redundant—the?? dragLastPosbranch will never execute.Also note the inconsistent indentation at line 457 (extra leading space on the closing brace).
♻️ Suggested cleanup
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() ) ?? dragLastPos; - - const finalPoint = snappedPoint ?? dragLastPos; - const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, - }; + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, + };
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 400-406: handleElementMove uses this.snapOverlay without guarding
for undefined; change the logic in handleElementMove so it calls
this.snapOverlay?.snapDragAngle(internal.dragStartPos, dragLastPos,
this.keyboard.shiftKey$.peek()) and if that returns undefined (i.e. snapOverlay
is absent) fall back to using dragLastPos as the snappedPoint before computing
dx/dy. This mirrors the null-safe pattern used in onDragEnd and prevents a
TypeError when snapOverlay is not set; update references around snappedPoint,
dx, and dy in handleElementMove accordingly.
🧹 Nitpick comments (2)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)
71-76: Minor: Extra blank lines aftermounted()method.Lines 75-76 have consecutive blank lines which is inconsistent with the rest of the file's style.
♻️ Suggested fix
mounted() { this.snapOverlay = new SnapOverlay(this.gfx); this.canvasEventHandler = new GfxViewEventManager(this.gfx); } - - + activeInteraction$ = signal<null | {
442-456: Redundant nullish coalescing and indentation inconsistency.
Line 448 (
const finalPoint = snappedPoint ?? dragLastPos;) is redundant -snappedPointis already guaranteed to be non-nullish from line 446's fallback?? dragLastPos.Indentation is inconsistent around lines 446 and 456.
♻️ Suggested cleanup
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() - ) ?? dragLastPos; - - const finalPoint = snappedPoint ?? dragLastPos; - + ) ?? dragLastPos; + const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, - }; + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, + };
Add optional chaining to snapDragAngle call.
|
Hi @darkskygit could you please take a look and let me know if this works as expected? |
darkskygit
changed the title
|
Does this behave as intended on your side? Cuz when I tried to press ⇧ Shift when creating/ modify an arrow (tried all three types) In here, and with adding some So this doesn't seem to be the correct place to put (Or maybe this is because I ran the client without server, tho that would change such behavior is unlikely) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 947-958: The snapping branch computes scaleX/scaleY using
snappedPos relative to originalBound as if the handle were bottom-right,
ignoring handleSign; update the shift-key branch in the resize handler (where
snappedPos is set via snapOverlay.snapDragAngle) to compute scaleX and scaleY
using the handle direction (handleSign or xSign/ySign) the same way
ResizeController.getScaleFromDelta does—i.e., derive delta based on snappedPos
adjusted by the handle sign (or divide by originalBound.w/h multiplied by sign,
falling back to 1 when sign is 0) so snapped scales match non-snapped behavior
for all handles.
🧹 Nitpick comments (3)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (3)
442-456: Redundant null coalescing onfinalPoint.Line 448 (
const finalPoint = snappedPoint ?? dragLastPos) is redundant becausesnappedPointis already guaranteed to be non-nullish from the null coalescing on line 446. The?? dragLastPosfallback already ensuressnappedPointis either the snap result ordragLastPos.♻️ Suggested simplification
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() ) ?? dragLastPos; - const finalPoint = snappedPoint ?? dragLastPos; - const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, };
983-1001: Static analysis:forEachcallback should not return a value.Per Biome lint, the
forEachcallback at line 985 implicitly returns the result ofext.onResizeStart?.(...). While this doesn't cause runtime issues (forEach ignores return values), it's cleaner to use an explicit block body.♻️ Suggested fix
- extensionHandlers.forEach(ext => - ext.onResizeStart?.({ elements, handle, handlePos, handleSign }) - ); + extensionHandlers.forEach(ext => { + ext.onResizeStart?.({ elements, handle, handlePos, handleSign }); + });
1028-1033: Static analysis: SameforEachreturn value issue.Same pattern as line 985 - the callback implicitly returns a value that
forEachignores.♻️ Suggested fix
- extensionHandlers.forEach(ext => - ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }) - ); + extensionHandlers.forEach(ext => { + ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }); + });
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 946-958: The Shift-resize branch references undefined variables
and uses world-space coords incorrectly: replace xSign/ySign with handleSign.x
and handleSign.y, use snappedPos instead of currentPos, and compute scale
factors by converting snappedPos into the same coordinate space used by
getScaleFromDelta (i.e., derive the delta in local/handle space) then call
getScaleFromDelta to obtain scaleX/scaleY; also compute the snap anchor from
handleSign (the opposite corner of the handle, not always originalBound.topLeft)
and pass that anchor into snapDragAngle/getScaleFromDelta so the fixed corner
matches the snap origin. Ensure references: snapOverlay.snapDragAngle,
handleSign, snappedPos, getScaleFromDelta, originalBound are updated
accordingly.
🧹 Nitpick comments (2)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)
442-456: Redundant null coalescing onfinalPoint.
snappedPointis already assigned with a fallback todragLastPoson lines 442-446, so it will never be nullish. The?? dragLastPoson line 448 is dead code.♻️ Suggested simplification
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() ) ?? dragLastPos; - const finalPoint = snappedPoint ?? dragLastPos; - const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, };
983-988: forEach callback implicitly returns a value (flagged by static analysis).The arrow function
ext => ext.onResizeStart?.(...)implicitly returns the result of the optional call. While harmless, this triggers Biome'suseIterableCallbackReturnlint rule. Wrap in braces to suppress.♻️ Suggested fix
extensionHandlers.forEach(ext => { - ext.onResizeStart?.({ elements, handle, handlePos, handleSign }) + ext.onResizeStart?.({ elements, handle, handlePos, handleSign }); });The same pattern applies to line 1030 (
onResizeEnd).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
396-413:⚠️ Potential issue | 🟡 MinorConfirmed mismatch:
dragLastPosshould be snapped inonDragMovefor consistency withonDragEnd.The
onDragMovehandler passes rawdragLastPosto downstream handlers while providing snappeddx/dy. However, theonDragEndhandler (line 448–453) already setsdragLastPosto the snappedfinalPoint. This inconsistency means handlers likemind-map-drag.ts(line 86) andframe-highlight-manager.ts(line 49) receive different coordinate systems between move and end events, breaking expected snapping behavior.Set
dragLastPostosnappedPointinonDragMoveto align withonDragEnd:Suggested fix
dragLastPos = Point.from( this.gfx.viewport.toModelCoordFromClientCoord([event.x, event.y]) ); const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, - dragLastPos, + dragLastPos, this.keyboard.shiftKey$.peek() ) ?? dragLastPos; + dragLastPos = snappedPoint; const dx = snappedPoint.x - internal.dragStartPos.x; const dy = snappedPoint.y - internal.dragStartPos.y; const moveContext: ExtensionDragMoveContext = { ...internal, event, - dragLastPos, + dragLastPos: snappedPoint, dx, dy, };
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 985-987: The forEach callback is using a concise arrow body that
implicitly returns the optional call (e.g., extensionHandlers.forEach(ext =>
ext.onResizeStart?.({...}))), which Biome flags; change these to statement-only
arrow bodies by adding braces and using an explicit statement (e.g.,
extensionHandlers.forEach(ext => { ext.onResizeStart?.({...}); });) for the
occurrence that calls ext.onResizeStart and the other similar
extensionHandlers.forEach callback that invokes the other resize handler so no
implicit return remains.
- Around line 559-568: The rotation update sends an inconsistent payload because
currentAngle is snapped but delta uses the raw payload.delta; change the delta
passed to options.onRotateUpdate to be computed from the snapped angle (e.g.,
adjustedDelta = snappedAngle - initialRotate) so that options.onRotateUpdate is
called with { currentAngle: snappedAngle, delta: adjustedDelta }, keeping the
existing snap computation using initialRotate, snappedAngle, payload.delta and
the snapOverlay?.snapRotateAngle and keyboard.shiftKey$.peek() logic intact.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
396-431:⚠️ Potential issue | 🔴 CriticalSnapping implementation won't apply to anchor endpoint dragging.
The snapping logic in
onDragMoveapplies only to elements managed by this handler. However, connector endpoint dragging is handled separately inconnector-handle.tsthrough directpointermovelisteners, completely bypassing this manager's drag infrastructure.The connector-handle component has no reference to
snapOverlayor any angle-snapping logic. If the PR goal is to add shift-based angle snapping for "anchor dragging," the implementation must be integrated into the connector-handle's pointer event handlers, not the element move manager.
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 946-958: The resize snapping uses the wrong anchor and coordinate
space: replace the use of originalBound.topLeft when calling snapDragAngle with
the correct fixed-corner anchor computed in getCoordsTransform (use the same
anchor logic based on the handle type/handleSign), and after receiving
snappedPos (world coordinates) convert it to the local space before computing
scale (use the same toLocal/local conversion pattern as getScaleFromDelta).
Ensure scaleX/scaleY calculations use the local delta relative to the chosen
anchor (and respect handleSign) rather than dividing world coords by
originalBound dimensions.
🧹 Nitpick comments (2)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)
442-456: Redundant null coalescing and inconsistent indentation.Line 448 (
const finalPoint = snappedPoint ?? dragLastPos) is redundant sincesnappedPointon line 446 already has?? dragLastPosas a fallback, making it impossible forsnappedPointto be nullish.Additionally, the indentation at lines 446 and 456 is inconsistent with the surrounding code.
♻️ Suggested cleanup
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() - ) ?? dragLastPos; - - const finalPoint = snappedPoint ?? dragLastPos; - + ) ?? dragLastPos; + const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, - }; + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, + };
1030-1032: Inconsistent indentation in forEach callback.Lines 1031-1032 have inconsistent indentation compared to the surrounding code. The closing brace should align with the opening of the forEach.
♻️ Suggested fix
extensionHandlers.forEach(ext => { - ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }); - }); + ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }); + });
Refactor snapping logic to use fixed-corner anchor for resizing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
341-341:⚠️ Potential issue | 🟠 MajorThe snapping feature is integrated in the wrong code path and will not apply to anchor dragging.
Verification confirms the architectural concern: connector anchor dragging uses a separate event handling system in
connector-handle.tsthat completely bypasseshandleElementMove. The anchor drag handler (lines 86-96 ofconnector-handle.ts) directly listens to pointermove events and updates the connector without invoking the element movement system where the snapping logic was added.The shift-based angle snapping needs to be integrated into the connector anchor drag handler, not into
handleElementMove. As currently implemented, the feature will only snap during regular element dragging on the canvas, not when dragging connector anchors—which contradicts the PR objective of adding snapping for "dragging existing anchors."
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 962-975: handleElementResize calls toLocal(snappedPos) and
toLocal(anchorWorld) but toLocal is undefined; fix by extending the resize
callback to receive the coordinate-transform helper from the ResizeController:
add a toLocal function parameter to the onResizeMove callback signature (and its
type), have ResizeController (in resize/manager.ts) pass its local
toWorld/toLocal closure when invoking onResizeMove, and then replace the direct
toLocal references in handleElementResize with the provided callback parameter;
also update all call sites and types where onResizeMove is invoked so the
transform is supplied.
🧹 Nitpick comments (1)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)
442-456: Remove redundant null coalescing operator.
snappedPointis already assigned with a fallback (?? dragLastPos) on line 446, so it can never benullorundefined. The subsequentconst finalPoint = snappedPoint ?? dragLastPos;on line 448 is redundant dead code.♻️ Proposed fix
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() - ) ?? dragLastPos; - - const finalPoint = snappedPoint ?? dragLastPos; - + ) ?? dragLastPos; + const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, - }; + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, + };
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 938-983: The onResizeMove callback references matrix but doesn't
receive it; update the ResizeController to pass the transform matrix into
onResizeMove (add matrix to the destructured params in onResizeMove) and update
all call sites that invoke onResizeMove to supply the same matrix used by
onResizeUpdate (or compute it there and forward it), then adjust the callback
type/interface so matrix is typed (e.g., Matrix or TransformMatrix) and the
snapping code (snapOverlay.snapDragAngle, invMatrix.apply, etc.) uses that
passed-in matrix instead of a missing variable.
🧹 Nitpick comments (2)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)
442-456: Remove redundant null coalescing.
snappedPointalready has a fallback todragLastPoson line 446, so it can never benullorundefined. ThefinalPointassignment on line 448 is redundant and adds unnecessary complexity.♻️ Suggested simplification
const snappedPoint = this.snapOverlay?.snapDragAngle( internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek() ) ?? dragLastPos; - const finalPoint = snappedPoint ?? dragLastPos; - const endContext: ExtensionDragEndContext = { ...internal, event, - dragLastPos: finalPoint, - dx: finalPoint.x - internal.dragStartPos.x, - dy: finalPoint.y - internal.dragStartPos.y, + dragLastPos: snappedPoint, + dx: snappedPoint.x - internal.dragStartPos.x, + dy: snappedPoint.y - internal.dragStartPos.y, };
1053-1057: Inconsistent indentation.Lines 1056-1057 have extra leading whitespace compared to the surrounding code structure. This is a minor formatting issue.
🎨 Suggested fix
extensionHandlers.forEach(ext => { - ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }); - }); + ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }); + });
| onResizeMove: ({ | ||
| scaleX, | ||
| scaleY, | ||
| originalBound, | ||
| handleSign, | ||
| handlePos, | ||
| currentHandlePos, | ||
| lockRatio, | ||
| }) => { | ||
| let snappedPos = currentHandlePos; | ||
| if (this.keyboard.shiftKey$.peek()) { | ||
| // Snap resize handle if Shift is held | ||
| // Compute fixed-corner anchor (same logic as getCoordsTransform) | ||
| const anchorWorld = new Point( | ||
| handleSign.x > 0 ? originalBound.left : originalBound.right, | ||
| handleSign.y > 0 ? originalBound.top : originalBound.bottom | ||
| ); | ||
|
|
||
| // Snap in world space using the correct anchor | ||
| snappedPos = | ||
| this.snapOverlay?.snapDragAngle(anchorWorld, currentHandlePos, true) ?? | ||
| currentHandlePos; | ||
|
|
||
| // Convert snapped position to local space | ||
| // Convert world-space points into local space using the resize transform | ||
| const invMatrix = matrix.clone().invert(); | ||
|
|
||
| const snappedLocal = Point.from( | ||
| invMatrix.apply([snappedPos.x, snappedPos.y]) | ||
| ); | ||
|
|
||
| const anchorLocal = Point.from( | ||
| invMatrix.apply([anchorWorld.x, anchorWorld.y]) | ||
| ); | ||
|
|
||
| const deltaLocal = snappedLocal.sub(anchorLocal); | ||
| // Compute scale using local delta (same as getScaleFromDelta) | ||
| scaleX = handleSign.x | ||
| ? (deltaLocal.x / originalBound.w) * handleSign.x | ||
| : 1; | ||
|
|
||
| scaleY = handleSign.y | ||
| ? (deltaLocal.y / originalBound.h) * handleSign.y | ||
| : 1; | ||
|
|
||
| if (extHandlers) { | ||
| handlers.push(extHandlers); | ||
| } | ||
|
|
||
| return handlers; | ||
| }, | ||
| [] as ActionContextMap['elementResize']['returnType'][] | ||
| ); | ||
| let lockRatio = false; | ||
|
|
||
| viewConfigMap.forEach(config => { | ||
| const { lockRatio: lockRatioConfig } = config.constraint; | ||
|
|
||
| lockRatio = | ||
| lockRatio || | ||
| lockRatioConfig === true || | ||
| (Array.isArray(lockRatioConfig) && lockRatioConfig.includes(handle)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and find the manager.ts file
find . -name "manager.ts" | grep interactivityRepository: toeverything/AFFiNE
Length of output: 189
🏁 Script executed:
# Search for onResizeMove invocation and the ResizeController definition
rg -n "onResizeMove" blocksuite/framework/std/src/gfx/interactivity/ -A 5 -B 2Repository: toeverything/AFFiNE
Length of output: 7760
🏁 Script executed:
# Check the ResizeController interface/type definition
rg -n "interface ResizeController|type ResizeController" blocksuite/framework/std/src/gfx/interactivity/ -A 30Repository: toeverything/AFFiNE
Length of output: 45
🏁 Script executed:
# Read the exact code around lines 938-983 to see destructuring and matrix usage
sed -n '920,1000p' blocksuite/framework/std/src/gfx/interactivity/manager.tsRepository: toeverything/AFFiNE
Length of output: 2526
🏁 Script executed:
# Also check if matrix is defined in the enclosing scope
sed -n '900,940p' blocksuite/framework/std/src/gfx/interactivity/manager.tsRepository: toeverything/AFFiNE
Length of output: 1192
🏁 Script executed:
# Look for where matrix is defined in this function - check broader context
sed -n '870,940p' blocksuite/framework/std/src/gfx/interactivity/manager.tsRepository: toeverything/AFFiNE
Length of output: 1938
🏁 Script executed:
# Also look at the full function signature to understand the context
sed -n '850,920p' blocksuite/framework/std/src/gfx/interactivity/manager.tsRepository: toeverything/AFFiNE
Length of output: 2021
🏁 Script executed:
# Search for where matrix is defined in the handleElementResize function
sed -n '880,1050p' blocksuite/framework/std/src/gfx/interactivity/manager.ts | grep -n "matrix"Repository: toeverything/AFFiNE
Length of output: 222
🏁 Script executed:
# Get a larger context to see if matrix is defined anywhere in handleElementResize
sed -n '880,1050p' blocksuite/framework/std/src/gfx/interactivity/manager.tsRepository: toeverything/AFFiNE
Length of output: 5317
matrix is undefined in onResizeMove callback — this will throw a ReferenceError at runtime.
The onResizeMove callback at lines 938-946 destructures { scaleX, scaleY, originalBound, handleSign, handlePos, currentHandlePos, lockRatio }, but line 963 references matrix which is not included in these parameters.
When a user holds Shift during resize, this code path executes and will crash with ReferenceError: matrix is not defined.
The matrix is available in the onResizeUpdate callback (line 1029), but not in onResizeMove. You need to either:
- Extend the
onResizeMovecallback parameters inResizeControllerto include the transformation matrix, or - Move the snapping logic to
onResizeUpdatewhere matrix is available, or - Recompute the transformation inline using
originalBoundand element rotation.
🤖 Prompt for AI Agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts` around lines 938 -
983, The onResizeMove callback references matrix but doesn't receive it; update
the ResizeController to pass the transform matrix into onResizeMove (add matrix
to the destructured params in onResizeMove) and update all call sites that
invoke onResizeMove to supply the same matrix used by onResizeUpdate (or compute
it there and forward it), then adjust the callback type/interface so matrix is
typed (e.g., Matrix or TransformMatrix) and the snapping code
(snapOverlay.snapDragAngle, invMatrix.apply, etc.) uses that passed-in matrix
instead of a missing variable.
Add null check for matrix before applying inverse transformation.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts`:
- Around line 400-412: The moveContext currently contains an unsnapped
dragLastPos while dx/dy are calculated from snappedPoint, causing inconsistency;
update the construction of moveContext so that dragLastPos is the snappedPoint
(i.e., set dragLastPos: snappedPoint) and, if raw pointer is needed by
consumers, add a rawDragLastPos field preserving the original dragLastPos;
adjust any consumers of ExtensionDragMoveContext that expect the old semantics
accordingly (refer to snapOverlay.snapDragAngle, dragStartPos, dx/dy, and
moveContext).
- Around line 442-448: snappedPoint already defaults to dragLastPos via the
nullish coalescing in the call to snapDragAngle, so the extra "?? dragLastPos"
when assigning finalPoint is redundant; update the code in manager.ts to remove
the dead coalescing (e.g., set finalPoint = snappedPoint or eliminate finalPoint
and use snappedPoint directly), referencing snappedPoint, finalPoint,
snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos,
this.keyboard.shiftKey$.peek()), and dragLastPos to locate and simplify the
assignment.
- Around line 962-967: The null-check for matrix is unreachable because
matrix.clone().invert() is called first; move the guard so you check `if
(!matrix) return { scaleX, scaleY };` before calling `matrix.clone().invert()`,
then compute `const invMatrix = matrix.clone().invert();` only after the guard
(or remove the guard entirely if callers guarantee a non-null `matrix`); update
any subsequent uses of `invMatrix` accordingly to ensure no operations run on a
null matrix.
🧹 Nitpick comments (3)
blocksuite/framework/std/src/gfx/interactivity/manager.ts (3)
67-69: Unnecessary constructor.This constructor only calls
super(...args)with no additional logic. TypeScript will generate this automatically.♻️ Suggested removal
- constructor(...args: ConstructorParameters<typeof GfxExtension>) { - super(...args); - }
517-612: Inconsistent indentation inhandleElementRotate.The method body is indented at 1-space/2-space from the class body instead of the standard indentation level used by other methods (e.g.,
handleElementMove,handleElementResize). This appears to be an accidental shift, making the code harder to read.
1059-1061: Inconsistent indentation inonResizeEndcallback.Lines 1060-1061 use mismatched indentation (extra leading space on 1060, reduced on 1061) compared to surrounding code.
🧹 Fix indentation
extensionHandlers.forEach(ext => { - ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }); - }); + ext.onResizeEnd?.({ elements, handle, handlePos, handleSign }); + });
| const snappedPoint = this.snapOverlay?.snapDragAngle( | ||
| internal.dragStartPos, | ||
| dragLastPos, | ||
| this.keyboard.shiftKey$.peek() | ||
| ) ?? dragLastPos; | ||
| const dx = snappedPoint.x - internal.dragStartPos.x; | ||
| const dy = snappedPoint.y - internal.dragStartPos.y; | ||
| const moveContext: ExtensionDragMoveContext = { | ||
| ...internal, | ||
| event, | ||
| dragLastPos, | ||
| dx: dragLastPos.x - internal.dragStartPos.x, | ||
| dy: dragLastPos.y - internal.dragStartPos.y, | ||
| dx, | ||
| dy, |
There was a problem hiding this comment.
dragLastPos in moveContext is unsnapped while dx/dy are snapped — potential consumer confusion.
moveContext.dragLastPos (Line 410) is the raw pointer position, but dx/dy (Lines 411-412) are computed from the snapped point. A consumer relying on both would see inconsistent data — e.g., dragStartPos + dx/dy ≠ dragLastPos. Consider either passing snappedPoint as dragLastPos, or documenting that dragLastPos is always the raw position.
🤖 Prompt for AI Agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts` around lines 400 -
412, The moveContext currently contains an unsnapped dragLastPos while dx/dy are
calculated from snappedPoint, causing inconsistency; update the construction of
moveContext so that dragLastPos is the snappedPoint (i.e., set dragLastPos:
snappedPoint) and, if raw pointer is needed by consumers, add a rawDragLastPos
field preserving the original dragLastPos; adjust any consumers of
ExtensionDragMoveContext that expect the old semantics accordingly (refer to
snapOverlay.snapDragAngle, dragStartPos, dx/dy, and moveContext).
| const snappedPoint = this.snapOverlay?.snapDragAngle( | ||
| internal.dragStartPos, | ||
| dragLastPos, | ||
| this.keyboard.shiftKey$.peek() | ||
| ) ?? dragLastPos; | ||
|
|
||
| const finalPoint = snappedPoint ?? dragLastPos; |
There was a problem hiding this comment.
Redundant null coalescing on finalPoint.
snappedPoint on Line 442-446 already falls back to dragLastPos via ?? dragLastPos, so it is guaranteed non-nullish. The ?? dragLastPos on Line 448 is dead code and finalPoint will always equal snappedPoint.
🧹 Proposed simplification
const snappedPoint = this.snapOverlay?.snapDragAngle(
internal.dragStartPos,
dragLastPos,
this.keyboard.shiftKey$.peek()
) ?? dragLastPos;
- const finalPoint = snappedPoint ?? dragLastPos;
-
const endContext: ExtensionDragEndContext = {
...internal,
event,
- dragLastPos: finalPoint,
- dx: finalPoint.x - internal.dragStartPos.x,
- dy: finalPoint.y - internal.dragStartPos.y,
+ dragLastPos: snappedPoint,
+ dx: snappedPoint.x - internal.dragStartPos.x,
+ dy: snappedPoint.y - internal.dragStartPos.y,
};🤖 Prompt for AI Agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts` around lines 442 -
448, snappedPoint already defaults to dragLastPos via the nullish coalescing in
the call to snapDragAngle, so the extra "?? dragLastPos" when assigning
finalPoint is redundant; update the code in manager.ts to remove the dead
coalescing (e.g., set finalPoint = snappedPoint or eliminate finalPoint and use
snappedPoint directly), referencing snappedPoint, finalPoint,
snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos,
this.keyboard.shiftKey$.peek()), and dragLastPos to locate and simplify the
assignment.
| // Convert snapped position to local space | ||
| // Convert world-space points into local space using the resize transform | ||
| const invMatrix = matrix.clone().invert(); | ||
| if (!matrix) { | ||
| return { scaleX, scaleY }; | ||
| } |
There was a problem hiding this comment.
Null guard for matrix is unreachable — will crash before it's checked.
Line 964 calls matrix.clone().invert() unconditionally. If matrix is nullish, this line throws a TypeError before the guard on Line 965 is ever evaluated. The if (!matrix) block is dead code.
Either move the guard before the .clone() call, or remove it if matrix is guaranteed by the caller.
🐛 Proposed fix — guard before use
- const invMatrix = matrix.clone().invert();
- if (!matrix) {
- return { scaleX, scaleY };
- }
+ if (!matrix) {
+ return { scaleX, scaleY };
+ }
+ const invMatrix = matrix.clone().invert();🤖 Prompt for AI Agents
In `@blocksuite/framework/std/src/gfx/interactivity/manager.ts` around lines 962 -
967, The null-check for matrix is unreachable because matrix.clone().invert() is
called first; move the guard so you check `if (!matrix) return { scaleX, scaleY
};` before calling `matrix.clone().invert()`, then compute `const invMatrix =
matrix.clone().invert();` only after the guard (or remove the guard entirely if
callers guarantee a non-null `matrix`); update any subsequent uses of
`invMatrix` accordingly to ensure no operations run on a null matrix.
2 participants
This PR adds ⇧ Shift angle snapping when drawing new lines/arrows and when dragging existing anchors.
While Shift is held, drag directions snap to 15° increments (matching tldraw behavior). Releasing Shift restores freeform movement.
This improves precision and consistency when creating and editing linear geometry.
Summary by CodeRabbit
New Features
Tests
Chores