fix: Mouse listerner leaks

[benfrontify]

No description provided.

[Copilot]

Pull request overview

This PR aims to fix mouse listener leaks around the image “stage” and container-operator lifecycle in figma-block, primarily by introducing explicit teardown paths and invoking them during React effect cleanup.

Changes:

• Ensure ImageStage registers a stable mousemove handler and provides destroy() to remove it.
• In useImageStage, destroy ImageStage on effect cleanup and destroy/reset ContainerOperator instances on resize/unmount.
• Add a destroy() method to the ContainerOperator base type to support cleanup calls.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
packages/figma-block/src/hooks/useImageStage.ts	Adds teardown for ImageStage and ContainerOperator during resize/unmount to address leaks.
packages/figma-block/src/components/ImageStage.ts	Fixes mousemove listener binding and adds destroy() for proper event listener removal.
packages/figma-block/src/components/ContainerOperator/ContainerOperator.ts	Introduces a destroy() method on the base operator API.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ragi96]

destroy() is never overridden in VectorContainerOperator, so the operator cleanup added in this PR is currently a no-op — and that's where the remaining listener leaks are.

VectorContainerOperator.ts:25-26 adds mouseover/mousedown listeners to imageContainer.node with inline .bind(this) — the same un-removable pattern this PR fixes in ImageStage. The ResizeObserver callback creates a new operator on every resize against the same persistent DOM node, so listeners accumulate and every stale operator instance is retained by its closures.

Additionally, the drag mousemove listener on document is only removed by mouseup on the container node (VectorContainerOperator.ts:71-75). Releasing the mouse outside the container, or replacing the operator mid-drag, leaves a document-level mousemove handler attached forever.

Suggested override in VectorContainerOperator.ts:

private readonly onMouseOverListener = () => this.onMouseOver();
private readonly onMouseDownListener = (event: MouseEvent) => this.onMouseDown(event);

// in the constructor, replace lines 25-26 with:
imageContainer.node.addEventListener('mouseover', this.onMouseOverListener);
imageContainer.node.addEventListener('mousedown', this.onMouseDownListener);

public override destroy(): void {
    this.imageContainer.node.removeEventListener('mouseover', this.onMouseOverListener);
    this.imageContainer.node.removeEventListener('mousedown', this.onMouseDownListener);
    this.imageContainer.node.removeEventListener('mouseup', this.mouseUpListener);
    document.removeEventListener('mousemove', this.mouseMoveListener);
}

[ragi96]

The updated revision looks much better, the VectorContainerOperator.destroy() override, the stored listener references, and the tests address the main leak concerns. Two changes I'd like to see before merging, plus a follow-up ticket:

1. Fix the coordinate mismatch — the inside-check is still wrong when the page is scrolled, and the new scroll/resize invalidation can't fix it. MouseProperties.getCurrentPosition returns pageX/pageY (document-relative, includes scroll), but getBoundingClientRect() is viewport-relative. With the page scrolled down by S, pageY = clientY + S, so pageY < boundaries.bottom fails even with a freshly recomputed rect. The new spec doesn't catch this because it dispatches events at scroll position 0, where pageX === clientX.

2. Remove the document-level listener instead of managing it. isMouseInsideImageStage has exactly one consumer — VectorContainerOperator.onMouseMove, which only runs during a drag (attached on mousedown, removed on mouseup). The dirty flag, the scroll/resize listeners, the three removals in destroy(), and their tests all exist to keep an always-on document-wide mousemove listener alive for a cursor tweak during drags. Computing the value on demand deletes all of that machinery and fixes point 1 in the same move:

// ImageStage — drop isMouseInsideImageStage, the mousemove/scroll/resize listeners, and the dirty flag
public isPointInside({ x, y }: Point): boolean {
    const rect = this.imageStage.getBoundingClientRect();
    return x > rect.left && x < rect.right && y > rect.top && y < rect.bottom;
}

// VectorContainerOperator.onMouseMove
if (this.imageStage.isPointInside({ x: event.clientX, y: event.clientY })) {
    this.imageContainer.changeMouseCursor(Cursor.GRABBING);
}

getBoundingClientRect() then runs only during drags (cheap), ImageStage.destroy() shrinks or disappears entirely, and there's nothing always-on left to leak — which also means the three new ImageStage specs reduce to one. The hook-side cleanup and the VectorContainerOperator.destroy() work stay as they are; that part is solid.

(If there's a consumer of isMouseInsideImageStage I'm missing, let me know and we can revisit.)

Follow-up (separate ticket, not this PR): migrate the drag in VectorContainerOperator to Pointer Events + setPointerCapture. That removes its remaining document-level drag listener, fixes the "mouseup outside the container leaves the listener attached" edge case, and adds touch/pen support for free — after it, nothing in this feature attaches document/window listeners at all.