[all components] Reduce animation completion work - #5535
Conversation
commit: |
Bundle size
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@claude review |
|
The review finished without producing a report. 🤖 Review generated with Claude Code · Opus 5 (High) · |
00b3f4f to
b1766ea
Compare
flaviendelangle
left a comment
There was a problem hiding this comment.
The direction make a lot of sense to me
I think Simplification 2 is over-engineered and can be dismissed.
PR review
One narrow behavior regression is reproducible, but nothing merge-blocking. A temporary Chromium test passed on the base commit and failed on the PR head. The existing focused tests, TypeScript, ESLint, and Prettier checks otherwise passed.
Bugs (1)
1. 🟠 Batching can detach an indicator rechecked by a sibling cleanup
Location: packages/react/src/checkbox/indicator/CheckboxIndicator.tsx:39
useOpenChangeComplete({
batch: true,
open: rendered,
ref: indicatorRef,
onComplete() {
if (!rendered) {
setMounted(false);
}
},
});The callbacks run before any of their state updates commit. If removing indicator A invokes a forwarded ref callback or layout-effect cleanup that rechecks controlled indicator B, B has already queued setMounted(false) using its stale closed state.
Previously, A committed first. Its cleanup reopened B before B's completion ran, aborting B's pending unmount. The same mechanism affects Radio and the Menu checkbox and radio indicators.
Failure scenario: Two indicators finish closing together. A's ref cleanup rechecks B. On this branch, B's forwarded ref receives null and then the element again, restarting its enter transition and potentially losing focus. The same Chromium test keeps B continuously mounted on the base commit.
Fix: Keep per-callback commits for components whose unmount runs consumer code, or redesign batching so later callbacks are revalidated after earlier commit effects. Add this ref-cleanup case as a regression test.
Simplifications (2)
1. 🟡 Skip enter-side watchers for batched indicators
Location: packages/react/src/checkbox/indicator/CheckboxIndicator.tsx:39
useOpenChangeComplete({
batch: true,
open: rendered,
// ...
onComplete() {
if (!rendered) {
setMounted(false);
}
},
});When an indicator opens, its completion handler cannot do anything. The hook still waits for starting styles, calls getAnimations() on every element, creates promises, and schedules a no-op flush. The Avatar change already avoids this work for the same reason.
Failure scenario: Programmatically checking or selecting 100 animated items performs 100 unnecessary animation queries. This leaves the enter-side cost that the PR's Avatar benchmark identifies as expensive.
Fix: Pass enabled: !rendered, with the equivalent closed-state condition at the other five indicator call sites. Add an assertion that getAnimations() is not called while entering.
2. 🟡 Keep the opt-in queue out of non-batched component bundles
Location: packages/react/src/internals/useAnimationsFinished.ts:8
let pendingCallbacks: Array<() => void> | null = null;
function flushBeforePaint(fn: () => void) {
// ...
}Only six indicator callers enable batching, but the queue and its branch now ship with every consumer of useAnimationsFinished. A filtered bundle measurement showed the non-batched Dialog entry increasing from 62,514 to 62,744 parsed bytes and from 20,932 to 21,017 gzip bytes.
Failure scenario: Applications importing Dialog, Popover, or another default-path component pay for queue code they cannot use.
Fix: Extract the scheduler into an indicator-only module or hook and pass it into the shared animation waiter. Keep the default path free of a static import of the batching implementation.
Verdict
Approve after nits - one narrow batching regression and two efficiency cleanups remain.
🤖 Review generated with Codex
This seems to be unreachable in any realistic scenario and the failure mode doesn't seem severe, so ignoring here Applied simplification No. 1 before the review already |
Fixes #5481
When multiple animation watchers finish together, each currently runs its own
flushSync. Queue callbacks that become ready in the same microtask and flush them once before paint, while continuing to respect aborted callbacks.Avatar.Imagenow only watches exit animations because its completion callback only performs work on exit.Performance
Synthetic Chromium benchmark with a 1 ms CSS animation:
The shared queue retains one callback record until the next microtask. Avatar enter animations no longer call
getAnimations().