[Vis Tools] Fix memory leaks in the Map Tool #5922
Conversation
Summary of ChangesHello @juliawu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and performance of the Map Tool by systematically implementing request cancellation for all its data fetching operations. By leveraging the AbortController API, the application now proactively terminates pending network requests when they are no longer needed, such as when a user navigates away from a component or triggers a new data fetch. This prevents unnecessary state updates on unmounted components and reduces resource consumption, ultimately leading to a more robust and responsive user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces AbortController to cancel in-flight API requests, effectively addressing potential memory leaks and race conditions in the Map Tool and statistical variable hierarchy components. The implementation is solid, using correct patterns for both class components and functional components with hooks. I have a couple of suggestions to further improve the robustness of the cancellation logic.
| private onSearchSelectionChange(selection: string): void { | ||
| this.getPath(selection).then((path) => { | ||
| const searchSelectionCleared = | ||
| !_.isEmpty(this.state.focusPath) && _.isEmpty(path); | ||
| this.setState({ | ||
| focus: selection, | ||
| focusPath: path, | ||
| searchSelectionCleared, | ||
| expandedPath: searchSelectionCleared ? this.state.focusPath : [], | ||
| this.getPath(selection, this._abortController?.signal) | ||
| .then((path) => { | ||
| const searchSelectionCleared = | ||
| !_.isEmpty(this.state.focusPath) && _.isEmpty(path); | ||
| this.setState({ | ||
| focus: selection, | ||
| focusPath: path, | ||
| searchSelectionCleared, | ||
| expandedPath: searchSelectionCleared ? this.state.focusPath : [], | ||
| }); | ||
| this.togglePath(selection, path, /*isSearchSelection=*/ true); | ||
| }) | ||
| .catch((error) => { | ||
| // Ignore abort errors | ||
| if (!axios.isCancel(error) && error.name !== "AbortError") { | ||
| console.error(error); | ||
| } | ||
| }); | ||
| this.togglePath(selection, path, /*isSearchSelection=*/ true); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The _abortController is managed by fetchData and is designed to cancel requests on re-renders or component unmount. Reusing it here for search selection changes can lead to issues. For example, if a user makes two search selections quickly, the request for the first selection won't be cancelled. Also, a fetchData call could unintentionally cancel a search request.
It would be better to use a separate AbortController for search-related requests to manage their lifecycle independently. You could introduce a _searchAbortController class property, create a new controller in this method (aborting the previous one if it exists), and also abort it in componentWillUnmount.
There was a problem hiding this comment.
Good bot. Added separate AbortControllers for data vs search fetches.
| if (!axios.isCancel(error) && error.name !== "AbortError") { | ||
| console.error(error); | ||
| } |
There was a problem hiding this comment.
For consistency with other error handling in this PR, it's better to check for cancellation errors affirmatively and return, rather than negatively. This makes the intent clearer and aligns with the pattern used elsewhere in the changes.
if (axios.isCancel(error) || error.name === "AbortError") {
return;
}
console.error(error);
There was a problem hiding this comment.
Good bot. Standardized to using affirmative cancellation error checks throughout.
1 participant
No description provided.