fix(react): correct effect dependency lifecycles - #884
Merged
Conversation
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.
Problem
Inline suppressions hid 86
react-hooks/exhaustive-depsdiagnostics across the audited React surface. In this category's files, several were genuine lifecycle bugs: WebSocket option changes were ignored, asynchronous conversation and branch fetches captured stale snapshots, listeners/callbacks could keep old props, and mount-only effects mixed true ownership constraints with accidental omissions.This PR owns 67 active diagnostics plus 3 stale directives in its non-overlapping file set. Companion low-risk and state-sync PRs own the remaining 19 active diagnostics.
Solution
useEffectEventwhere resubscription would be wrong.-- reasonto every one.undefinedoptions no longer overwrite client defaults.set-state-in-effectcases narrowly: WSProvider publishes an externally owned disposable client, and the realtime capability probe clears state before its async result.Potential risks
onBeforeReconnectchanges. Parents that change callback identity every render should stabilize that callback.Retained exhaustive-deps suppressions
ComposerInput/index,InlineRenameInput,VirtualizedListBase,XtermOutput,PokerTableWindow.CodeMirror/Diffconstruction effects.CloudSessionReferencePreview,CloudSessionShareDialog, both resolved-model label hooks, threeFileSidebarprojections,ShellSidebar,useSubmissionsData,useTeamRuntimeRoster,useFileMode, and threeNavigationMenuhooks.useCloudOrgPanelState,useEventStoreSelector,useAsyncData,useCallbackRefEffect, anduseScrollToBottom.Verification
pnpm exec eslint $(git diff --name-only origin/develop...HEAD --diff-filter=ACM | rg '\.(ts|tsx)$') --report-unused-disable-directivesrg -l 'eslint-(disable|disable-next-line|disable-line).*react-hooks/exhaustive-deps' src | xargs pnpm exec eslint --no-inline-config --rule 'react-hooks/exhaustive-deps: warn' --format json— 47 visible diagnostics: 28 intentional/reasoned here, 19 assigned to companion PRs.pnpm typecheckpnpm exec vitest run src/api/realtime/websocket/WSProvider.test.ts— 2 tests passed.git diff --check origin/develop...HEADPerformance lifecycle evidence
optionsobject; regression test covers equal-object and changed-value renders.Performance verdict: pass for lifecycle/churn review and focused automated coverage. No live Tauri profiling claim is made in this PR.