Conversation
commit: |
Bundle size
Deploy previewhttps://deploy-preview-4565--base-ui.netlify.app/ 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. |
f5e3f52 to
b30e454
Compare
b30e454 to
056a53b
Compare
atomiks
left a comment
There was a problem hiding this comment.
Codex Review (GPT-5.4)
This is primarily a refactor PR with secondary bug-fix, tests, and docs scope. I reviewed the full branch against a freshly fetched master; the panel-logic consolidation is moving in the right direction, but there is one remaining public-surface bug in the new animation-suppression path.
1. Bugs / Issues
1. 🔴 Inline style animations bypass the new mount/activity suppression (blocking)
The refactor now suppresses initial-open / Activity-resume keyframe animations by returning style: { animationName: 'none' } from useCollapsiblePanel, but that value is merged before the consumer style prop. Because Collapsible.Panel and Accordion.Panel both pass the hook result through useRenderElement, a consumer can override the suppression with a supported public API such as style={{ animationName: 'panel-slide-down', animationDuration: '100ms' }}.
packages/react/src/collapsible/panel/useCollapsiblePanel.ts
props: {
hidden,
id: idParam,
style: {
animationName: shouldPreventOpenAnimation ? 'none' : undefined,
},
},packages/react/src/collapsible/panel/CollapsiblePanel.tsx
props: [
props,
{
style: {
[CollapsiblePanelCssVars.collapsiblePanelHeight as string]:
height === undefined ? 'auto' : `${height}px`,
[CollapsiblePanelCssVars.collapsiblePanelWidth as string]:
width === undefined ? 'auto' : `${width}px`,
},
},
elementProps,
],I verified this with SSR spot checks on both Collapsible.Panel and Accordion.Panel: initially-open markup still rendered animation-name: panel-slide-down when the animation was supplied via style, so the issue-904 fix and the new React.Activity suppression are incomplete for a real styling path in an unstyled library. The new tests only cover class-based animations, so this regression is currently unguarded.
Fix: Apply the suppression after consumer styles have been resolved/merged, or reintroduce an imperative post-merge override on the element, and add an inline-style regression case for both Collapsible.Panel and Accordion.Panel.
Behavior Preservation Assessment
Moving mount/unmount, measurement, and motion ownership into useCollapsiblePanel does make the runtime easier to reason about, and I did a second pass over the touched collapsible/accordion runtime files, docs, and experiments after reading the full diff. Outside the style-merge bug above, I did not find another concrete correctness, React lifecycle, or accessibility issue from code reading.
Test Coverage Assessment
The added CollapsiblePanel tests are pointed at the right behaviors: initial-open keyframes, close measurement, beforematch, and React.Activity resume paths. I also ran pnpm typescript, pnpm test:jsdom CollapsiblePanel --no-watch, pnpm test:chromium CollapsiblePanel --no-watch, pnpm test:jsdom AccordionPanel --no-watch, and pnpm test:chromium AccordionPanel --no-watch; they all passed. The missing piece is coverage for animation supplied through the public style prop, which is exactly where the remaining bug lives.
Recommendation
Request changes 🔴
The refactor is close, but the new motion suppression can still be bypassed through a supported public styling path on both Collapsible and Accordion, which leaves the core fix incomplete. Once that path is protected and covered by a regression test, I’d be comfortable approving.
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review: [collapsible] Refactor panel logic (#4565)
Author: mj12albert
Branch: refactor/collapsible -> master
Stats: +1647 / -1291 across 25 files
Overview
This PR is a significant refactor that moves panel mount/unmount, dimension tracking, and motion handling from useCollapsibleRoot into useCollapsiblePanel. The root hook becomes very thin — it only manages open state, transition status, and the trigger handler. The panel hook now owns:
- Animation type detection (
css-transition|css-animation|none) - Dimension measurement and caching
- Mount/unmount lifecycle
React.Activityanimation suppressionbeforematch/hiddenUntilFoundhandling
It also fixes #904, adds React.Activity support to prevent replaying open animations, and adds comprehensive Chromium-only tests.
Strengths
-
Much cleaner separation of concerns. The root no longer needs
panelRef,abortControllerRef,animationTypeRef,transitionDimensionRef,visible,setVisible,setDimensions,setHiddenUntilFound,setKeepMounted,runOnceAnimationsFinish,height,width. The context object shrinks dramatically, which reduces re-render scope. -
Well-structured helper functions.
setTemporaryStyle,resetLayoutStyles,scheduleRestore,cleanup,getDimensions,getAnimationType,hasNonZeroDurationare well-extracted, well-documented, and pure. This is a big improvement over the inline imperative code that was previously scattered across multipleuseIsoLayoutEffectcallbacks. -
Thorough test coverage. The new tests cover CSS transitions, CSS animations, SSR animation suppression, and four
React.Activityscenarios (transition + animation, initially open + user opened). These are all Chromium-only (skipIf(isJSDOM)) which is appropriate since they depend on real layout. -
Good experiment cleanup. Shared accordion/collapsible experiment components are extracted into
_components/directories, eliminating massive CSS and markup duplication across experiment pages.
Issues & Suggestions
1. Typo in comment
In useCollapsiblePanel.ts, the comment on renderedDimensions has a typo:
// These 2 refs are also safe to read in render, bothhold the last committed
Should be both hold.
2. enabled parameter on useOpenChangeComplete
The hook is now called with an enabled field:
useOpenChangeComplete({
enabled: open && mounted && panelTransitionStatus === 'idle',
open: true,
ref: panelRef,
onComplete() { ... },
});This parameter doesn't appear in the current useOpenChangeComplete interface (at least not in the published @base-ui/react version). Is there a companion PR adding this parameter, or is this expected to land together? Without enabled, the hook would ignore an unknown property and always run.
3. shouldRender includes open
The new shouldRender logic is:
const shouldRender = keepMounted || hiddenUntilFound || mounted || open;Previously it was keepMounted || hiddenUntilFound || mounted. The addition of || open makes sense for the first render cycle before mounted is set, but worth confirming this doesn't cause a flash of content for initially-closed panels (the hidden prop should handle it, but the DOM element will exist briefly).
4. getAnimationType called every layout effect
Previously animationTypeRef was set once and never changed. Now getAnimationType(panel, shouldPreventOpenAnimation) is called on every useIsoLayoutEffect run (whenever mounted, open, transitionStatus, or shouldPreventOpenAnimation changes). The getComputedStyle call inside is not free. Consider whether caching the result when shouldPreventOpenAnimation is false would help, or confirm that the frequency is acceptable given that these effects only fire on open/close cycles.
5. lastMeasuredDimensionsRef never cleared
The ref caches the last measured dimensions indefinitely. While this is intentional for CSS animation close paths (to restore a pixel value when dimensions resets to auto), it means a stale measurement could be used if panel content changes between open/close cycles. This might be acceptable since it only affects the start of the close animation, but worth documenting.
6. Removed hidden from AccordionItem state
The AccordionItem state no longer includes hidden: !collapsible.mounted. This is a public API change for AccordionItem.State consumers who might be using state.hidden in render callbacks or className functions. Confirm this is documented as a breaking change or that hidden was never part of the public type.
7. Two useOpenChangeComplete calls
The hook is now called twice: once for the open-complete path and once for the close-complete path. Previously there was a single call. This is fine architecturally but doubles the MutationObserver / event listener overhead. Given these only activate when enabled is true, the overhead should be negligible.
Risk Assessment
-
Medium risk — This is a large refactor of core animation/transition logic that touches both Collapsible and Accordion (via shared hooks). The comprehensive test suite mitigates this well, but real-world edge cases (interrupted animations, rapid toggling,
prefers-reduced-motion, etc.) are worth manual testing via the experiment links. -
The
React.Activitysupport usesReact.useEffectcleanup to detect Activity teardown, which is the correct approach per React 19 semantics.
Summary
This is a well-executed refactor that significantly simplifies the root hook and consolidates panel logic where it belongs. The code is cleaner, more testable, and better documented than what it replaces. The main items to address are the typo, confirming the enabled parameter on useOpenChangeComplete, and verifying the hidden state removal from AccordionItem isn't a breaking change.
I think this is a breaking change |
8d6686f to
3ef5c3d
Compare
|
Fixed in 3ef5c3d |
| // Wait for `[data-ending-style]` to be applied. | ||
| attributeObserver = new MutationObserver((mutationList) => { |
There was a problem hiding this comment.
Not sure if removing this will cause a regression. It watched for DOM mutations to avoid flake issues, specifically: #3101. Does the new logic avoid flakes like this?
There was a problem hiding this comment.
useAnimationsFinished actually also waits, to avoid flakes. I found that rAF, double rAF, setTimeout etc are all async hacks that aren't fully reliable in all scenarios
There was a problem hiding this comment.
I've added back this part, now it needs useAnimationsFinished + 1 RAF because the timing changed, codex explanation:
the old fix installed its
MutationObserverearlier in the close lifecycle, beforedata-ending-stylelanded. The current refactor’s close-completion effect starts later, whenpanelTransitionStatus === 'ending', which often means the ending-style render has already committed.
There was a problem hiding this comment.
I think it actually isn't necessary with your previous (new) logic.
The observer is not needed for the Base UI-controlled data-ending-style timing. This effect only runs when panelTransitionStatus === 'ending', and that same value is passed into panelState.transitionStatus, which renders data-ending-style via transitionStatusMapping. Since this is a passive useEffect, React has already committed that render before this effect runs, so the attribute is already present when the observer would be created. The old #3101 observer was installed earlier in the close lifecycle; this one is installed after the ending-style commit. Keep the RAF before useAnimationsFinished for Chrome’s transition registration timing, but remove the observer.
I had it prove it with tests as well:
Old logic:
- close handling started earlier
- so there was a real window where the close-completion machinery could start before
data-ending-stylehad landed - that’s what the old
MutationObserverwas protecting
New logic:
- close handling starts later, after the panel is already in the
'ending'phase - with the observer removed, the Chromium timing tests I added still pass
- and the new instrumentation shows the actual close animation check sees
data-ending-stylealready present
So the regression that justified the old MO does not reproduce under the new lifecycle. The extra RAF still matters; the MO does not look justified anymore.
e10aa78 to
be38c93
Compare
Experiments:
useCollapsiblePanelnow owns panel mount/unmount, dimension tracking, and motion handling completely.useCollapsibleRootno longer touches any of that and is now very thin. Also:Fixes [collapsible] With CSS animations there is a flash on loaded when the panel is initially open #904
Added support for
React.Activityso already-open panels do not replay their open animation when Activity hides and reveals a collapsibleAdded tests
I have followed (at least) the PR section of the contributing guide.