Skip to content

[collapsible] Refactor panel logic#4565

Open
mj12albert wants to merge 10 commits intomui:masterfrom
mj12albert:refactor/collapsible
Open

[collapsible] Refactor panel logic#4565
mj12albert wants to merge 10 commits intomui:masterfrom
mj12albert:refactor/collapsible

Conversation

@mj12albert
Copy link
Copy Markdown
Member

@mj12albert mj12albert commented Apr 10, 2026

Experiments:

useCollapsiblePanel now owns panel mount/unmount, dimension tracking, and motion handling completely. useCollapsibleRoot no longer touches any of that and is now very thin. Also:

@mj12albert mj12albert added the component: collapsible Changes related to the collapsible component. label Apr 10, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 10, 2026

commit: be38c93

@code-infra-dashboard
Copy link
Copy Markdown

code-infra-dashboard bot commented Apr 10, 2026

Bundle size

Bundle Parsed size Gzip size
@base-ui/react ▼-1.25KB(-0.27%) ▼-172B(-0.12%)

Details of bundle changes

Deploy preview

https://deploy-preview-4565--base-ui.netlify.app/


Check out the code infra dashboard for more information about this PR.

@netlify
Copy link
Copy Markdown

netlify bot commented Apr 10, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit be38c93
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/69d913fc776da100085466f9
😎 Deploy Preview https://deploy-preview-4565--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@mj12albert mj12albert force-pushed the refactor/collapsible branch 2 times, most recently from f5e3f52 to b30e454 Compare April 10, 2026 09:47
@mj12albert mj12albert force-pushed the refactor/collapsible branch from b30e454 to 056a53b Compare April 10, 2026 10:05
@mj12albert mj12albert changed the title [collapsible] Refactor internals [collapsible] Refactor panel logic Apr 10, 2026
@mj12albert mj12albert marked this pull request as ready for review April 10, 2026 10:09
Copy link
Copy Markdown
Contributor

@atomiks atomiks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Activity animation suppression
  • beforematch / hiddenUntilFound handling

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, hasNonZeroDuration are well-extracted, well-documented, and pure. This is a big improvement over the inline imperative code that was previously scattered across multiple useIsoLayoutEffect callbacks.

  • Thorough test coverage. The new tests cover CSS transitions, CSS animations, SSR animation suppression, and four React.Activity scenarios (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.Activity support uses React.useEffect cleanup 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.

@flaviendelangle
Copy link
Copy Markdown
Member

and verifying the hidden state removal from AccordionItem isn't a breaking change.

I think this is a breaking change

@mj12albert mj12albert force-pushed the refactor/collapsible branch from 8d6686f to 3ef5c3d Compare April 10, 2026 12:33
@mj12albert
Copy link
Copy Markdown
Member Author

@flaviendelangle

  • enabled parameter on useOpenChangeComplete: it exists 😅
  • shouldRender includes open: it's expected and there's no flash, it's needed because open === true && mounted === false is possible
  • lastMeasuredDimensionsRef never cleared: added code comments explaining the niche use-case of this
  • hidden removed from AccordionItem state: added back, but the condition now includes !isOpen &&
  • getAnimationType runs on every layout-effect pass: previously it was set once and forgotten assuming nobody will realistically change between animations/transitions within the lifetime of the component. However this apparently also breaks live prefers-reduced-motion changes (e.g. a setting that toggles "Reduce motion"), so I think it's worth supporting this at the cost of calling getComputedStyle effectively on every open change. Minimizing these calls without breaking the reduced motion case gets verbose and might cost the bundle size reduction gained in this refactor 😪

@mj12albert
Copy link
Copy Markdown
Member Author

mj12albert commented Apr 10, 2026

Inline style animations bypass the new mount/activity suppression

Fixed in 3ef5c3d

Comment on lines -223 to -224
// Wait for `[data-ending-style]` to be applied.
attributeObserver = new MutationObserver((mutationList) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added back this part, now it needs useAnimationsFinished + 1 RAF because the timing changed, codex explanation:

the old fix installed its MutationObserver earlier in the close lifecycle, before data-ending-style landed. The current refactor’s close-completion effect starts later, when panelTransitionStatus === 'ending', which often means the ending-style render has already committed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-style had landed
  • that’s what the old MutationObserver was 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-style already 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.

@mj12albert mj12albert force-pushed the refactor/collapsible branch from e10aa78 to be38c93 Compare April 10, 2026 15:15
@mj12albert mj12albert requested a review from atomiks April 11, 2026 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: collapsible Changes related to the collapsible component.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[collapsible] With CSS animations there is a flash on loaded when the panel is initially open

3 participants