-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: bubble set performance workaround #7208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v5
Are you sure you want to change the base?
fix: bubble set performance workaround #7208
Conversation
/gemini review |
There was a problem hiding this 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.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
};
optimize bubble set path update with deferred execution logic