Repository navigation
Conversation
…operation DragService.startDrag accepted a `component` option but never passed it to dragListener, so the GraphLayer never captured events for it. GraphComponent.onDrag passes `component: this`, so components dragged with onDrag lost the capture they had before DragService was introduced (#198): hover moved to the components under the cursor and the mouseup was dispatched to whatever was under it. startDrag now passes the component on: it is captured when the drag starts and released when the drag ends or is cancelled. DragService.cancel() only cancelled a component drag. An operation started with startDrag() could only be stopped through stopDragListening(), which the package does not export. cancel() now cancels the current drag of either kind: the operation gets onCancel (or onEnd when onCancel is not provided) with the last event and coordinates, the cursor, auto-panning and the captured component are released, and the rest of the gesture is ignored. An operation that has not crossed the drag threshold is dropped without callbacks, as a component drag is. DragService forgets the operation once it ends or is cancelled, so a repeated cancel() does nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThe PR fixes custom drag operations by forwarding component capture into the drag listener and by tracking active File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Preview is ready. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/services/drag/DragService.ts" line_range="424" />
<code_context>
const {
document: doc,
cursor,
+ component,
autopanning = true,
stopOnMouseLeave,
</code_context>
<issue_to_address>
**Drag survives graph unmount**
When the graph unmounts while a `startDrag()` operation is active, `DragService.destroy()` does not cancel `currentOperationEmitter`, so its document listeners and callbacks remain active; when the operation captured a component, `GraphLayer` also retains that target. Later pointer events still reach the unmounted graph.
Cancel the active operation in `destroy()` before completing cleanup.
Also at `src/services/drag/DragService.ts:436`.
</issue_to_address>
### Comment 2
<location path="src/services/drag/DragService.ts" line_range="436" />
<code_context>
+ }
+ };
+
+ this.currentOperationEmitter = emitter;
+ return emitter
.on(EVENTS.DRAG_START, (event: MouseEvent) => {
</code_context>
<issue_to_address>
**Cancelled drags keep running**
When a second `startDrag()` begins before the first operation ends, `startDrag()` overwrites `currentOperationEmitter` without stopping the earlier emitter, so cancellation stops only the newer operation and the earlier callbacks can still run through mouseup. For component-backed operations, the later operation also replaces the graph layer’s capture, and either cleanup can release capture while the other operation is still running.
Stop the currently tracked operation before replacing it with the new emitter.
Also at `src/services/drag/DragService.ts:424`.
</issue_to_address>
### Comment 3
<location path="src/services/drag/DragService.ts" line_range="424" />
<code_context>
const {
document: doc,
cursor,
+ component,
autopanning = true,
stopOnMouseLeave,
</code_context>
<issue_to_address>
**Capture survives a throwing start callback**
When a component-backed operation’s `onStart` callback throws, `dragListener` captures the component before emitting `DRAG_START`; if the operation’s `onStart` callback throws, setup exits before the move listener is installed and before drag cleanup runs. The component remains captured and drag resources stay active after the exception.
Guard drag-start setup so an exception from a start callback releases capture and other drag resources before propagating.
</issue_to_address>
### Comment 4
<location path="src/services/drag/DragService.test.ts" line_range="319-320" />
<code_context>
+
+ expect(callbacks.onCancel).toHaveBeenCalledTimes(1);
+ expect(callbacks.onCancel).toHaveBeenCalledWith(lastMove, world(graph, 20, 30));
+ expect(graph.getCursorLayer().isAuto()).toBe(true);
+ expect(graph.cameraService.getCameraState().autoPanningEnabled).toBe(false);
+
+ const updatesBeforeCancel = callbacks.onUpdate.mock.calls.length;
</code_context>
<issue_to_address>
**Default states mask cleanup regressions**
When `startDrag()` fails to activate the cursor or auto-panning before cancellation, in `calls onCancel once with the last event and coordinates and ignores the rest of the gesture`, the cursor and auto-panning assertions pass if `startDrag()` never activates either resource: the cursor starts in auto mode and auto-panning starts disabled. The test therefore does not catch a regression that leaves these resources unmanaged.
Assert that the cursor is locked and auto-panning is enabled before calling `cancel()`.
</issue_to_address>Sourcery assessment
Approval pending. 4 findings to address first.
Blocking findings: src/services/drag/DragService.ts:424, src/services/drag/DragService.ts:436, src/services/drag/DragService.ts:424, src/services/drag/DragService.test.ts:320
| const emitter = dragListener(targetDocument, { | ||
| graph: this.graph, | ||
| dragCursor: cursor, | ||
| component, |
There was a problem hiding this comment.
🟡 Medium · Drag survives graph unmount
When the graph unmounts while a startDrag() operation is active, DragService.destroy() does not cancel currentOperationEmitter, so its document listeners and callbacks remain active; when the operation captured a component, GraphLayer also retains that target. Later pointer events still reach the unmounted graph.
Cancel the active operation in destroy() before completing cleanup.
Also at src/services/drag/DragService.ts:436.
Prompt for AI agents
In `src/services/drag/DragService.ts` at line 424:
**Drag survives graph unmount**
When the graph unmounts while a `startDrag()` operation is active, `DragService.destroy()` does not cancel `currentOperationEmitter`, so its document listeners and callbacks remain active; when the operation captured a component, `GraphLayer` also retains that target. Later pointer events still reach the unmounted graph.
Cancel the active operation in `destroy()` before completing cleanup.
Also at `src/services/drag/DragService.ts:436`.| } | ||
| }; | ||
|
|
||
| this.currentOperationEmitter = emitter; |
There was a problem hiding this comment.
🟡 Medium · Cancelled drags keep running
When a second startDrag() begins before the first operation ends, startDrag() overwrites currentOperationEmitter without stopping the earlier emitter, so cancellation stops only the newer operation and the earlier callbacks can still run through mouseup. For component-backed operations, the later operation also replaces the graph layer’s capture, and either cleanup can release capture while the other operation is still running.
Stop the currently tracked operation before replacing it with the new emitter.
Also at src/services/drag/DragService.ts:424.
Prompt for AI agents
In `src/services/drag/DragService.ts` at line 436:
**Cancelled drags keep running**
When a second `startDrag()` begins before the first operation ends, `startDrag()` overwrites `currentOperationEmitter` without stopping the earlier emitter, so cancellation stops only the newer operation and the earlier callbacks can still run through mouseup. For component-backed operations, the later operation also replaces the graph layer’s capture, and either cleanup can release capture while the other operation is still running.
Stop the currently tracked operation before replacing it with the new emitter.
Also at `src/services/drag/DragService.ts:424`.| const emitter = dragListener(targetDocument, { | ||
| graph: this.graph, | ||
| dragCursor: cursor, | ||
| component, |
There was a problem hiding this comment.
🟡 Medium · Capture survives a throwing start callback
When a component-backed operation’s onStart callback throws, dragListener captures the component before emitting DRAG_START; if the operation’s onStart callback throws, setup exits before the move listener is installed and before drag cleanup runs. The component remains captured and drag resources stay active after the exception.
Guard drag-start setup so an exception from a start callback releases capture and other drag resources before propagating.
Prompt for AI agents
In `src/services/drag/DragService.ts` at line 424:
**Capture survives a throwing start callback**
When a component-backed operation’s `onStart` callback throws, `dragListener` captures the component before emitting `DRAG_START`; if the operation’s `onStart` callback throws, setup exits before the move listener is installed and before drag cleanup runs. The component remains captured and drag resources stay active after the exception.
Guard drag-start setup so an exception from a start callback releases capture and other drag resources before propagating.
Comment on lines
+319
to
+320
| expect(graph.getCursorLayer().isAuto()).toBe(true); | ||
| expect(graph.cameraService.getCameraState().autoPanningEnabled).toBe(false); |
There was a problem hiding this comment.
🟡 Medium · Default states mask cleanup regressions
When startDrag() fails to activate the cursor or auto-panning before cancellation, in calls onCancel once with the last event and coordinates and ignores the rest of the gesture, the cursor and auto-panning assertions pass if startDrag() never activates either resource: the cursor starts in auto mode and auto-panning starts disabled. The test therefore does not catch a regression that leaves these resources unmanaged.
Assert that the cursor is locked and auto-panning is enabled before calling cancel().
Prompt for AI agents
In `src/services/drag/DragService.test.ts` at lines 319-320:
**Default states mask cleanup regressions**
When `startDrag()` fails to activate the cursor or auto-panning before cancellation, in `calls onCancel once with the last event and coordinates and ignores the rest of the gesture`, the cursor and auto-panning assertions pass if `startDrag()` never activates either resource: the cursor starts in auto mode and auto-panning starts disabled. The test therefore does not catch a regression that leaves these resources unmanaged.
Assert that the cursor is locked and auto-panning is enabled before calling `cancel()`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DragService.startDrag()acceptsDragOperationOptions.componentbut never passes it todragListener, so theGraphLayernever captures events for it.GraphComponent.onDrag()passescomponent: this, so sinceDragServicewas introduced (feat: introduce DragSystem #198, 1.10.0) a component dragged withonDrag()no longer keeps the pointer: hover moves to the components under the cursor, and the mouseup is dispatched to whatever is under it.DragService.cancel()cancels only a component drag. An operation started withstartDrag()(ConnectionLayer,NewBlockLayer,PortConnectionLayer,SelectionLayer, the minimap,GraphComponent.onDrag()) could only be stopped withstopDragListening(), which the package does not export, so there was no public way to cancel it, e.g. on Escape.Fix
startDrag()passescomponenttodragListener. The component is captured withGraphLayer.captureEvents()when the drag starts and released when it ends or is cancelled (cancel(), window blur, hidden page, a new press).DragServicekeeps the currentstartDrag()operation and forgets it once the operation ends or is cancelled.cancel()cancels the current drag of either kind: the component drag and the operation. A participant that throws does not leave the other one running.DragService,cancel(),startDrag(),DragOperationOptions.component,DragOperationCallbacks.onCancel, the cancel events ofConnectionLayerandNewBlockLayer, anddocs/system/drag-system.mdare updated. They no longer point tostopDragListening().Behaviour change
GraphComponent.onDrag(), or passed ascomponenttostartDrag(), keeps the pointer capture again while the drag runs: other components get nomouseenter/mouseleave, and the mouseup goes to the dragged component. This is the behaviour before 1.10.0 and whatdocs/components/canvas-graph-component.mddescribes. Built-in layers do not passcomponentand are not affected.graph.dragService.cancel()also cancels astartDrag()operation. The operation getsonCancel, oronEndwhen it has noonCancel(as on window blur), with the last event and coordinates; the cursor, auto-panning and the captured component are released, and the rest of the gesture is ignored. For the built-in layers this firesconnection-create-cancel,block-add-cancel-from-shadowandport-connection-cancel, drops the selection rectangle and stops the minimap drag;GraphComponent.onDrag()getsonDrop. An operation that has not crossed the drag threshold is dropped without callbacks, as a component drag is. A repeatedcancel()does nothing.startDrag()and the existing ways to cancel (blur,visibilitychange, a new press) are unchanged.Tests
src/services/drag/DragService.test.ts:cancel(), blur, a hidden page and a new press, including throughGraphComponent.onDrag();cancel()during an operation callsonCancel, oronEndwithout it, once with the last event and coordinates, and ignores the rest of the gesture;cancel()before the threshold drops the operation silently;cancel(), or one after the operation ended, does nothing;main.e2e/tests/drag/start-drag-operation.spec.ts:graph.dragService.cancel()cancels aConnectionLayerdrag and no connection is created.main.npm run lint,npm run typecheck, all unit tests and the Playwright e2e suite pass.🤖 Generated with Claude Code
Summary by Sourcery
Restore component pointer capture during drags and make
DragService.cancel()stop both component drags and custom drag operations.New Features:
DragService.cancel()to cancel custom drag operations started withstartDrag(), including consistent lifecycle callbacks and cleanup.startDrag()so its pointer events remain captured throughout the drag.Bug Fixes:
GraphComponent.onDrag()and component-awarestartDrag()operations, preventing hover and mouseup events from moving to underlying components.Enhancements:
Documentation:
graph.dragService.cancel()and component event capture.Tests: