fix(FR-2852): resolve react-doctor error-level findings#7325
Draft
yomybaby wants to merge 1 commit into
Draft
Conversation
Member
Author
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses React Doctor error-level findings across the WebUI and backend.ai-ui package by adding missing useEffect cleanups for timers/listeners, adding missing alt text for an image, and suppressing known false-positives for react-router’s useLocation() values in dependency arrays.
Changes:
- Added proper cleanup functions for
setTimeoutandaddEventListenerregistrations to prevent leaks and post-unmount side effects. - Added
alt="Run on Backend.AI"to the notebook badge image to satisfyjsx-a11y/alt-text. - Added targeted
react-doctor/no-mutable-in-depssuppressions foruseLocation()-derived deps.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| react/src/pages/StartPage.tsx | Suppresses react-doctor/no-mutable-in-deps for location.search derived from useLocation(). |
| react/src/components/ServiceValidationView.tsx | Tracks validation timeout via ref and clears it on unmount. |
| react/src/components/PluginLoader.tsx | Suppresses react-doctor/no-mutable-in-deps for location.pathname derived from useLocation(). |
| react/src/components/NetworkStatusBanner.tsx | Adds cleanup removing custom document event listeners. |
| react/src/components/MainLayout/WebUISider.tsx | Suppresses react-doctor/no-mutable-in-deps for location.pathname derived from useLocation(). |
| react/src/components/MainLayout/MainLayout.tsx | Suppresses react-doctor/no-mutable-in-deps for location.pathname derived from useLocation(). |
| react/src/components/InputNumberWithSlider.tsx | Captures timeout id and clears it in effect cleanup. |
| react/src/components/ImportNotebookForm.tsx | Adds alt text to the Backend.AI badge <img>. |
| react/src/components/DefaultProviders.tsx | Captures and clears the i18n initialization timeout in effect cleanup. |
| packages/backend.ai-ui/src/components/BAIFetchKeyButton.tsx | Introduces timeout ref to manage “min loading display time” behavior. |
| packages/backend.ai-ui/src/components/BAIDynamicUnitInputNumberWithSlider.tsx | Captures timeout id and clears it in effect cleanup. |
| packages/backend.ai-ui/src/components/BAIDynamicStepInputNumber.tsx | Captures timeout id and clears it in effect cleanup. |
Comment on lines
+80
to
84
| turnOffTimeoutRef.current = setTimeout(() => { | ||
| setDisplayLoading(false); | ||
| turnOffTimeoutRef.current = null; | ||
| }, remainingTime); | ||
| }; |
Comment on lines
+218
to
+222
| return () => { | ||
| if (validationTimeoutRef.current !== null) { | ||
| clearTimeout(validationTimeoutRef.current); | ||
| validationTimeoutRef.current = null; | ||
| } |
Comment on lines
+87
to
89
| // `location` is from react-router useLocation() — pathname/search are reactive across navigations. | ||
| // react-doctor-disable-next-line react-doctor/no-mutable-in-deps | ||
| }, [location.search, queryParams.type]); |
Comment on lines
+362
to
364
| // `location` is from react-router useLocation() — pathname is reactive across navigations. | ||
| // react-doctor-disable-next-line react-doctor/no-mutable-in-deps | ||
| }, [location.pathname]); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #7323 (FR-2852)
Summary
react-doctor/effect-needs-cleanuperrors by returning proper cleanup functions forsetTimeout/addEventListenerregistrations:react/src/components/InputNumberWithSlider.tsxreact/src/components/NetworkStatusBanner.tsx(both event listeners)react/src/components/ServiceValidationView.tsx(track timeout via ref since it is created inside an async.then)react/src/components/DefaultProviders.tsxpackages/backend.ai-ui/src/components/BAIDynamicStepInputNumber.tsxpackages/backend.ai-ui/src/components/BAIDynamicUnitInputNumberWithSlider.tsxpackages/backend.ai-ui/src/components/BAIFetchKeyButton.tsx(track in-flight turn-offsetTimeoutvia ref so re-runs cancel any pending hide)jsx-a11y/alt-texterror: addalt="Run on Backend.AI"to the badge<img>inreact/src/components/ImportNotebookForm.tsx.react-doctor/no-mutable-in-depsfalse positives. The rule treats anylocation.*in deps as a mutable global, butlocationhere comes fromreact-routeruseLocation(), which is reactive across navigations. Inlinereact-doctor-disable-next-linekeeps the rule active for genuineref.current/window.locationmistakes.react/src/pages/StartPage.tsxreact/src/components/MainLayout/MainLayout.tsxreact/src/components/MainLayout/WebUISider.tsxreact/src/components/PluginLoader.tsxNo behavior changes for the 4
useLocationdeps — they were already reactive. Cleanup additions remove real listener / timer leaks.Test plan
bash scripts/verify.sh— Relay / Lint / Format pass; no new TypeScript errors (pre-existing failures inbackend.ai-clientandDeleteForeverVFolderModalV2.tsxare unrelated).npx -y react-doctor@latest .—effect-needs-cleanup,no-mutable-in-deps, andjsx-a11y/alt-texterrors all resolved.WebUISider).ServiceValidationView, unmount during validation, confirm no "CannotValidateNow" toast fires post-unmount.