[code-infra] Enable mui/no-floating-cleanup and fix flagged leaks#22872
[code-infra] Enable mui/no-floating-cleanup and fix flagged leaks#22872romgrk wants to merge 3 commits into
Conversation
Adopt the new `mui/no-floating-cleanup` rule (mui/mui-public#1538), which flags call statements whose returned cleanup/unsubscribe is discarded, and resolve everything it reports. - Bump `@mui/internal-code-infra` to 0.0.4-canary.66 (first version with the rule); enable it on `packages/*/src` via `projectService`. - Real leaks fixed: - Return the unsubscribe from effects (`MoreEventsPopover`, `useKeepGroupedColumnsHidden`). - Register the scheduler stores' dev-assertion subscriptions on their `DisposableStack`. - Migrate the Tree View store to `useDisposable`; its subscriptions, `EventManager` and `TimeoutManager` are now torn down on unmount. The mount-time side effect (lazy-loading init) moves from the old `disposeEffect` to a dedicated `mountEffect`. - Intentional non-leak discards annotated with `void` (d3 builder, callback-ref forwarding, `markChatLayoutPane` in-place mutation). - Excluded `x-codemod` (AST transforms) and the vendored `base-ui-copy`. - Two Data Grid cases left as `void` + `FIXME` pending a behavior decision (see inline comments): `useGridRowSelection` no-op effect and `useGridRegisterStrategyProcessor` discarded unregister. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // discards it (previously `runIfRowSelectionIsEnabled(removeOutdatedSelection)`, | ||
| // which invoked it). Restoring the call changes selection behavior, so it needs | ||
| // a decision; `void` preserves current behavior. See #20668. | ||
| void runIf(props.rowSelection, removeOutdatedSelection); |
There was a problem hiding this comment.
no-floating-cleanup flagged this. runIf is curried — runIf(props.rowSelection, removeOutdatedSelection) builds a handler and returns it, so as a bare statement the effect is a no-op. It regressed in #20668, which replaced runIfRowSelectionIsEnabled(removeOutdatedSelection) (that one invoked the fn).
Restoring the call (if (props.rowSelection) removeOutdatedSelection()) makes rowSelection.DataGridPro › "should auto select parents when controlling row selection model" expect an extra onRowSelectionModelChange (4 vs 3) — so whether the effect should run is a behavior decision. Left as void to preserve current behavior; needs a maintainer call on whether to restore it (and update the test) or drop the dead effect.
There was a problem hiding this comment.
Resolved in 1d7dba4: deleted the dead effect. Since runIf is curried it has been a no-op since #20668, so removing it is behavior-identical and drops the dead code. removeOutdatedSelection stays wired to the filteredRowsSet event. (If prop-change re-pruning is actually wanted, that is a separate, deliberate fix that also needs to suppress the redundant onRowSelectionModelChange — happy to open an issue.)
Deploy previewhttps://deploy-preview-22872--material-ui-x.netlify.app/ Bundle size
PerformanceTotal duration: 1,878.17 ms -20.65 ms(-1.1%) | Renders: 63 (+0) No significant changes — details Check out the code infra dashboard for more information about this PR. |
romgrk
added
internal
`pnpm dedupe` converges @mui/utils and @typescript-eslint/parser to single versions, unblocked by the @mui/internal-code-infra canary.66 bump. Fixes the `test_static` "`pnpm dedupe` was run?" check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- useGridRowSelection: delete the dead effect. `runIf` is curried, so the statement has been a no-op since mui#20668; removing it is behavior-identical and drops the confusing dead code. (removeOutdatedSelection is still wired to the `filteredRowsSet` event.) - useGridRegisterStrategyProcessor: keep the discard, reword to a NOTE. The unregister fn is intentionally dropped — a strategy processor must outlive the registering component (applyStrategyProcessor throws if the active strategy's processor is missing) and the cache is reclaimed with the grid api, so it is not a leak. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Enables the new
mui/no-floating-cleanuprule (mui/mui-public#1538) on package sources and resolves everything it flags. The rule reports call statements whose returned cleanup/unsubscribe function is discarded — usually a leaked subscription.Changes
@mui/internal-code-infrato0.0.4-canary.66(first version with the rule); enable it onpackages/*/srcusingprojectService.MoreEventsPopover,useKeepGroupedColumnsHidden.DisposableStack.useDisposable— its subscriptions,EventManager, andTimeoutManagerare now torn down on unmount. The mount-time side effect (lazy-loading init) moves from the olddisposeEffectinto a dedicatedmountEffect.void: d3 scale builder, callback-ref forwarding,markChatLayoutPane(mutates in place and returns the component).x-codemod(jscodeshift AST transforms, no subscriptions) and the vendoredbase-ui-copy.useGridRowSelection— deleted a dead effect (runIfis curried, so it has been a no-op since [DataGridPremium] HandleisRowSelectable()checks for the rows missing due tokeepNonExistentRowsSelected#20668; removal is behavior-identical).useGridRegisterStrategyProcessor— by-design discard (a strategy processor must outlive the registering component; not a leak), documented with aNOTE.Notes
eslint .runs clean with no OOM at the CI 4 GB cap.projectServiceis ~7% faster thanprojecthere; the dominant cost is whole-repo type-checking (see review thread for follow-up speed ideas).🤖 Generated with Claude Code