fix: fix memory leaks in destroy methods by redfish4ktc · Pull Request #1023 · maxGraph/maxGraph

redfish4ktc · 2026-03-13T16:02:13Z

PR Checklist

Addresses an existing open issue: closes [BUG] Memory leak in PanningManager: some listeners are not destroyed #994. If not, explain why (minor changes, etc.).
You have discussed this issue with the maintainers of maxGraph, and you are assigned to the issue.
The scope of the PR is sufficiently narrow to be examined in a single session. A PR covering several issues must be split into separate PRs. Do not create a large PR, otherwise it cannot be reviewed and you will be asked to split it later or the PR will be closed.
I have added tests to prove my fix is effective or my feature works. This can be done in the form of automatic tests in packages/core/_tests_ or a new or altered Storybook story in packages/html/stories (an existing story may also demonstrate the change).
I have provided screenshot/videos to demonstrate the change. If no releavant, explain why. No visual changes
I have added or edited necessary documentation, or no docs changes are needed. No documentation changes
The PR title follows the "Conventional Commits" guidelines.

Overview

Changes:
- Fix AbstractGraph.destroy() to iterate plugins Map correctly and set null container reference
- Fix PanningHandler.onDestroy() to call panningManager.destroy()
- Fix PanningManager.destroy() to call stop() to clear interval timer
- Fix RubberBandHandler.onDestroy() to remove gestureHandler listener
- Fix SelectionCellsHandler.onDestroy() to remove listener from selectionModel
- Add EventSource.destroy() to clear eventListeners, call super.destroy() in children classes
- Add destroy tests for all modified classes

Notes

The scope of this PR is wider than what is described in #994.
I did a systematic search on destroy methods and look for missing resources releasing.

Summary by CodeRabbit

Tests
- Added broad test coverage verifying event listeners and plugins are cleaned up during component teardown to reduce memory leaks and improve stability.
Refactor
- Standardized destruction flows across UI, graph, and plugin components to ensure base-class cleanup runs, stop active timers/processes, and explicitly remove gesture/panning and other listeners for more reliable teardown.

- Fix AbstractGraph.destroy() to iterate plugins Map correctly and set null container reference - Fix PanningHandler.onDestroy() to call panningManager.destroy() - Fix PanningManager.destroy() to call stop() to clear interval timer - Fix RubberBandHandler.onDestroy() to remove gestureHandler listener - Fix SelectionCellsHandler.onDestroy() to remove listener from selectionModel - Add EventSource.destroy() to clear eventListeners, call super.destroy() in children classes - Add destroy tests for all modified classes

coderabbitai · 2026-03-13T16:02:39Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d563b5ac-57d3-40af-8d90-4fdbea30cd40

📥 Commits

Reviewing files that changed from the base of the PR and between 462075e and 722b97f.

📒 Files selected for processing (1)

packages/core/__tests__/view/BaseGraph.test.ts

Walkthrough

Adds an explicit destruction propagation and listener cleanup path: new EventSource.destroy(), many destroy/onDestroy methods marked override and now call super.destroy(), AbstractGraph plugin iteration/container nulling adjusted, and numerous tests added to validate destroy behavior. (No API removals; a new EventSource.destroy was added.)

Changes

Cohort / File(s)	Summary
Test Coverage for Destroy Methods `packages/core/__tests__/editor/Editor.test.ts`, `packages/core/__tests__/gui/MaxPopupMenu.test.ts`, `packages/core/__tests__/gui/MaxToolbar.test.ts`, `packages/core/__tests__/gui/MaxWindow.test.ts`, `packages/core/__tests__/view/GraphView.test.ts`, `packages/core/__tests__/view/cell/CellMarker.test.ts`, `packages/core/__tests__/view/layout/LayoutManager.test.ts`, `packages/core/__tests__/view/layout/SwimlaneManager.test.ts`, `packages/core/__tests__/view/event/EventSource.test.ts`, `packages/core/__tests__/view/other/AutoSaveManager.test.ts`	Added unit tests asserting that destroy()/onDestroy() clear event listeners and null references for multiple UI and view components.
Plugin and Graph Destroy Tests `packages/core/__tests__/view/BaseGraph.test.ts`, `packages/core/__tests__/view/cell/CellTracker.test.ts`, `packages/core/__tests__/view/plugin/PanningHandler.test.ts`, `packages/core/__tests__/view/plugin/RubberBandHandler.test.ts`, `packages/core/__tests__/view/plugin/SelectionCellsHandler.test.ts`	Added tests validating plugin onDestroy invocation, listener removal across models/views, handler cleanup, idempotency, and plugin-related teardown.
Test Utilities `packages/core/__tests__/utils.ts`	Added exported `hasListener()` test helper to check for listener presence in eventListeners arrays.
Editor & GUI Destroy Overrides `packages/core/src/editor/Editor.ts`, `packages/core/src/gui/MaxPopupMenu.ts`, `packages/core/src/gui/MaxToolbar.ts`, `packages/core/src/gui/MaxWindow.ts`	Changed `destroy()` signatures to `override destroy()` and added `super.destroy()` calls after local cleanup.
Core Graph Destruction Logic `packages/core/src/view/AbstractGraph.ts`, `packages/core/src/view/GraphView.ts`	AbstractGraph: switched plugin iteration to `this.plugins.forEach(...)`, nulls `this.container`, documents destroy behavior, and calls `super.destroy()`; GraphView now calls `super.destroy()` after local cleanup.
Event Source Base Destruction `packages/core/src/view/event/EventSource.ts`	Added public `destroy()` that resets `eventListeners` to an empty array and updated related JSDoc. Subclasses are expected to call `super.destroy()`.
Layout & Swimlane Destroy Overrides `packages/core/src/view/layout/LayoutManager.ts`, `packages/core/src/view/layout/SwimlaneManager.ts`	Marked `destroy()` as `override` and call `super.destroy()` after nullifying graph references.
Cell & Manager Destroy Overrides `packages/core/src/view/cell/CellMarker.ts`, `packages/core/src/view/other/AutoSaveManager.ts`	Marked `destroy()` as `override`, invoke `super.destroy()` and ensure component-specific teardown (e.g., highlight.destroy()).
Panning & Plugin Cleanup `packages/core/src/view/other/PanningManager.ts`, `packages/core/src/view/plugin/PanningHandler.ts`, `packages/core/src/view/plugin/RubberBandHandler.ts`, `packages/core/src/view/plugin/SelectionCellsHandler.ts`	PanningManager now calls `stop()` before removing listeners; PanningHandler destroys its panningManager and calls superclass cleanup; RubberBandHandler now removes `gestureHandler` on destroy; SelectionCellsHandler removes `refreshHandler` from the selectionModel and calls `super.destroy()`.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Destroy Caller
  participant Instance as ComponentInstance
  participant EventSrc as EventSource
  participant Plugins as PluginsCollection
  participant Super as BaseClass

  Caller->>Instance: destroy()
  Instance->>Instance: local cleanup (DOM refs, timers, managers)
  Instance->>EventSrc: EventSource.destroy() (clears eventListeners)
  Instance->>Plugins: plugins.forEach(p => p.onDestroy())
  Instance->>Super: super.destroy()
  Super->>Super: base cleanup

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

chore(typescript): always use explicit override #921: Similar additions of override modifiers and super.destroy() calls across lifecycle methods.
feat: let efficiently not load default plugins and style builtins #776: Overlapping changes to destruction lifecycle in EventSource/AbstractGraph/Graph components.
refactor(type)!: make AbstractGraph.getPlugin return T | undefined #842: Related AbstractGraph plugin handling/iteration changes that affect plugin teardown.

Suggested labels

refactor

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely summarizes the main change: fixing memory leaks in destroy methods across multiple classes.
Description check	✅ Passed	The PR description is complete with all required checklist items checked, clear explanation of changes made, and proper reference to the linked issue `#994`.
Linked Issues check	✅ Passed	The PR comprehensively addresses the memory leak issue `#994` by fixing PanningManager destruction and extending the fix systematically across other classes with similar issues.
Out of Scope Changes check	✅ Passed	While the PR scope extends beyond issue `#994`, all changes are directly related to memory leak fixes in destroy methods, making them well-justified as part of systematic resource cleanup improvements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

Generate coding plan for human review comments

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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)

packages/core/src/view/GraphView.ts (1)

2363-2401: ⚠️ Potential issue | 🟡 Minor

Consider moving super.destroy() outside the conditional block.

The super.destroy() call is currently inside the if (root && root.parentNode) block. If this condition is false (e.g., the canvas is already detached or null), the base class cleanup won't execute and event listeners in EventSource.eventListeners will remain, potentially causing memory leaks.

Suggested fix

   override destroy(): void {
     let root: SVGElement | HTMLElement | null = null;

     if (this.canvas && this.canvas instanceof SVGElement) {
       root = this.canvas.ownerSVGElement as SVGElement;
     }

     if (!root) {
       root = this.canvas;
     }

     if (root && root.parentNode) {
       this.clear(this.currentRoot, true);
       InternalEvent.removeGestureListeners(
         document,
         null,
         this.moveHandler,
         this.endHandler
       );
       InternalEvent.release(this.graph.container);
       root.parentNode.removeChild(root);

       this.moveHandler = null;
       this.endHandler = null;

       // `@ts-expect-error` Can be null when destroyed.
       this.canvas = null;
       // `@ts-expect-error` Can be null when destroyed.
       this.backgroundPane = null;
       // `@ts-expect-error` Can be null when destroyed.
       this.drawPane = null;
       // `@ts-expect-error` Can be null when destroyed.
       this.overlayPane = null;
       // `@ts-expect-error` Can be null when destroyed.
       this.decoratorPane = null;
-
-      super.destroy();
     }
+
+    super.destroy();
   }

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9feb3b8e-62d2-4e78-90c4-f46b23b704d9

📥 Commits

Reviewing files that changed from the base of the PR and between b113cce and 462075e.

📒 Files selected for processing (8)

packages/core/__tests__/view/BaseGraph.test.ts
packages/core/src/gui/MaxToolbar.ts
packages/core/src/view/AbstractGraph.ts
packages/core/src/view/GraphView.ts
packages/core/src/view/cell/CellMarker.ts
packages/core/src/view/event/EventSource.ts
packages/core/src/view/layout/LayoutManager.ts
packages/core/src/view/layout/SwimlaneManager.ts

🚧 Files skipped from review as they are similar to previous changes (1)

packages/core/src/view/AbstractGraph.ts

sonarqubecloud · 2026-03-18T16:04:25Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

redfish4ktc requested a review from tbouffard March 13, 2026 16:02

redfish4ktc added the bug Something isn't working label Mar 13, 2026

tbouffard reviewed Mar 13, 2026

View reviewed changes

Comment thread packages/core/__tests__/view/BaseGraph.test.ts Outdated

This comment was marked as resolved.

Sign in to view

test: mark property of a test fixture as readonly [skip ci]

b113cce

This comment was marked as resolved.

Sign in to view

redfish4ktc added 3 commits March 18, 2026 16:19

test: add missing import

3c826d9

EventSource: add explicit return type

c2f45e0

MaxToolbar: free the menu property on destroy

462075e

coderabbitai Bot reviewed Mar 18, 2026

View reviewed changes

Comment thread packages/core/__tests__/view/BaseGraph.test.ts

BaseGraph.test.ts: improve test

722b97f

redfish4ktc merged commit 24357a7 into main Mar 19, 2026
14 checks passed

redfish4ktc deleted the fix/994-destroy_panningManager_when_destroy_PanningHandler branch March 19, 2026 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: fix memory leaks in destroy methods#1023

fix: fix memory leaks in destroy methods#1023
redfish4ktc merged 6 commits into
mainfrom
fix/994-destroy_panningManager_when_destroy_PanningHandler

redfish4ktc commented Mar 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 13, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented Mar 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

redfish4ktc commented Mar 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Overview

Notes

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented Mar 18, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

redfish4ktc commented Mar 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 13, 2026 •

edited

Loading