Skip to content

Conversation

@Mnikley
Copy link
Contributor

@Mnikley Mnikley commented Jun 10, 2025

optimize bubble set path update with deferred execution logic

@hustcc hustcc requested a review from Aarebecca July 29, 2025 03:02
@Aarebecca Aarebecca changed the title Hotfix/bubble set performance workaround fix: bubble set performance workaround Aug 1, 2025
@Aarebecca
Copy link
Contributor

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request optimizes bubble set path updates with deferred execution logic. The current approach has a critical logical flaw that can lead to missed updates and a potential memory leak from an unmanaged setTimeout. The review focuses on correcting these issues by suggesting a more robust and standard debounce pattern, which simplifies the logic, fixes the bug, and addresses the potential leak.

Comment on lines 131 to 148
private updateBubbleSetsPath = (event: ElementLifeCycleEvent) => {
if (!this.shallRunUpdateBubbleSetsPath) {
if (!this.pendingRunUpdateBubbleSetsPath) {
this.pendingRunUpdateBubbleSetsPath = true;
setTimeout(() => {
this.shallRunUpdateBubbleSetsPath = true;
this.updateBubbleSetsPath(event);
});
}
return;
}

this.pendingRunUpdateBubbleSetsPath = false;
this.shallRunUpdateBubbleSetsPath = false;

if (!this.shape) return;
const id = idOf(event.data);
if (![...this.options.members, ...this.options.avoidMembers].includes(id)) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The deferred execution logic is flawed and can lead to missed updates and memory leaks.

The event object from the first call is captured and used in the deferred execution. If the first call is for a non-member element, and a subsequent call for a member element happens before the timeout, the update for the member will be skipped. This is because the deferred check on line 146 will use the captured non-member event and fail.

The setTimeout is not cleared if the plugin is destroyed. This can lead to errors when the callback tries to access properties of a destroyed object.

Refactor this to a simpler and more robust debounce pattern. Check if the event is for a member element upfront, use a single timeout ID to manage the debounced call, and clear the timeout in a destroy() method.

  private updateBubbleSetsPath = (event: ElementLifeCycleEvent) => {
    if (!this.shape) return;
    const id = idOf(event.data);
    if (![...this.options.members, ...this.options.avoidMembers].includes(id)) return;

    if (this.updatePathTimeout) {
      clearTimeout(this.updatePathTimeout);
    }

    this.updatePathTimeout = setTimeout(() => {
      if (!this.shape) return;
      // The actual update logic, which was previously after the check, should be placed here.
      // For example: this.drawBubbleSets();
    }, 0);
  };

@Aarebecca Aarebecca force-pushed the hotfix/bubble-set-performance-workaround branch from 1b6fc18 to 40643c5 Compare November 27, 2025 07:24
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.53%. Comparing base (ef197c8) to head (40643c5).
⚠️ Report is 1 commits behind head on v5.

Files with missing lines Patch % Lines
packages/g6/src/plugins/bubble-sets.ts 80.76% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##               v5    #7208      +/-   ##
==========================================
- Coverage   95.59%   95.53%   -0.06%     
==========================================
  Files         188      188              
  Lines        9888     9907      +19     
  Branches     2134     2142       +8     
==========================================
+ Hits         9452     9465      +13     
- Misses        436      442       +6     
Flag Coverage Δ
g6 95.53% <80.76%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/g6/src/plugins/bubble-sets.ts 95.03% <80.76%> (-3.57%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Aarebecca Aarebecca merged commit e8d95bc into antvis:v5 Nov 27, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants