Uh oh!
There was an error while loading. Please reload this page.
fix(headless): correct panel measurement and tabs indicator timing - #8897
Merged
Conversation
- Accordion.Panel / Collapsible.Panel: reset flex/grid alignment before reading scrollHeight/scrollWidth so non-default alignment can't truncate the measured content size (new resetLayoutStyles util). - Accordion.Panel / Collapsible.Panel: drop the pinned pixel size once the open animation settles (panel flows at auto) and re-pin a measured value when closing starts so exit transitions still animate. - Tabs.Indicator: measure in useLayoutEffect (before paint) to remove a one-frame stale position on tab change. Track the list element as context state so the indicator's layout effect can read it (a child layout effect runs before the parent ref attaches).
🦋 Changeset detectedLatest commit: b33c1db The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a ChangesPanel Height Measurement & Animation Lifecycle
Tabs List Ref → State Migration
Sequence Diagram(s)sequenceDiagram
participant ResizeObserver
participant AccordionPanel
participant resetLayoutStyles
participant useAnimationsFinished
ResizeObserver->>AccordionPanel: observe size changes
AccordionPanel->>resetLayoutStyles: call with panel element
resetLayoutStyles->>resetLayoutStyles: capture align styles + priorities
resetLayoutStyles->>AccordionPanel: force align to initial !important
AccordionPanel->>AccordionPanel: read scrollHeight
AccordionPanel->>resetLayoutStyles: schedule restore on next frame
AccordionPanel->>AccordionPanel: set height = px value
Note over AccordionPanel,useAnimationsFinished: Open animation in progress
useAnimationsFinished->>AccordionPanel: animation finished
AccordionPanel->>AccordionPanel: clear height (undefined)
Note over AccordionPanel,useAnimationsFinished: User closes panel
AccordionPanel->>resetLayoutStyles: call with panel element (re-pin)
resetLayoutStyles->>AccordionPanel: force align to initial !important
AccordionPanel->>AccordionPanel: set height = px value
AccordionPanel->>AccordionPanel: play close animation
sequenceDiagram
participant TabsRoot
participant TabsContext
participant TabsList
participant TabsIndicator
TabsRoot->>TabsRoot: useState(listElement)
TabsRoot->>TabsContext: provide listElement + setListElement
TabsList->>TabsContext: consume setListElement
TabsList->>TabsList: pass setListElement to Composite ref
Note over TabsList,TabsIndicator: Tab list element mounted
TabsList->>TabsContext: setListElement(composite element)
TabsContext->>TabsIndicator: listElement updated
TabsIndicator->>TabsIndicator: useLayoutEffect observes listElement
TabsIndicator->>TabsIndicator: measure active tab bounding rect
TabsIndicator->>TabsIndicator: update indicator position sync
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/eslint-plugin@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/headless/src/primitives/accordion/accordion.test.tsx (1)
194-238: ⚡ Quick winAdd a close-transition test for re-pinning height.
The updated suite validates open-path pin/clear behavior, but it still doesn’t assert the new close-path contract (re-pin measured height when closing starts). A focused test here would lock down the exact behavior this PR adds.
💡 Suggested test shape
+ it('re-pins --cl-accordion-panel-height when close transition starts', async () => {+ const user = userEvent.setup();+ renderAccordion({ defaultValue: ['item1'] });++ const original = (Element.prototype as { getAnimations?: unknown }).getAnimations;+ (Element.prototype as { getAnimations?: unknown }).getAnimations = () => [+ { finished: new Promise<void>(() => {}) },+ ];+ try {+ await user.click(screen.getByRole('button', { name: 'Section 1' }));+ const panel = document.querySelector('[data-cl-slot="accordion-panel"]') as HTMLElement;+ expect(panel.getAttribute('style') ?? '').toContain('--cl-accordion-panel-height');+ } finally {+ if (original) {+ (Element.prototype as { getAnimations?: unknown }).getAnimations = original;+ } else {+ delete (Element.prototype as { getAnimations?: unknown }).getAnimations;+ }+ }+ });As per coding guidelines,
**/*.{test,spec}.{js,ts,jsx,tsx}should “Implement comprehensive testing including unit, integration, and E2E tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/headless/src/primitives/accordion/accordion.test.tsx` around lines 194 - 238, The animation lifecycle test suite validates the open-path behavior for height pinning and clearing, but is missing a test case for the close-path behavior. Add a new test within the 'animation lifecycle' describe block that validates the height re-pinning behavior when closing starts. This test should open an accordion panel (using defaultValue), then simulate closing it and verify that the --cl-accordion-panel-height CSS variable is re-pinned to the measured pixel height during the close animation phase, similar to the pattern used in the existing 'pins --cl-accordion-panel-height while the open animation is in progress' test case.Source: Coding guidelines
packages/headless/src/primitives/collapsible/collapsible.test.tsx (1)
142-189: ⚡ Quick winAdd a close-transition test that asserts both dimensions are re-pinned.
This suite now covers open-path pin/clear behavior, but it doesn’t directly test the new close-path re-pin logic (
--collapsible-panel-height/--collapsible-panel-widthwhen closing starts). Adding that case will better protect this PR’s core behavior.💡 Suggested test shape
+ it('re-pins height/width vars when close transition starts', async () => {+ const user = userEvent.setup();+ renderCollapsible({ defaultOpen: true });++ const original = (Element.prototype as { getAnimations?: unknown }).getAnimations;+ (Element.prototype as { getAnimations?: unknown }).getAnimations = () => [+ { finished: new Promise<void>(() => {}) },+ ];+ try {+ await user.click(screen.getByRole('button', { name: 'Toggle' }));+ const panel = document.querySelector('[data-cl-slot="collapsible-panel"]') as HTMLElement;+ const style = panel.getAttribute('style') ?? '';+ expect(style).toContain('--collapsible-panel-height');+ expect(style).toContain('--collapsible-panel-width');+ } finally {+ if (original) {+ (Element.prototype as { getAnimations?: unknown }).getAnimations = original;+ } else {+ delete (Element.prototype as { getAnimations?: unknown }).getAnimations;+ }+ }+ });As per coding guidelines,
**/*.{test,spec}.{js,ts,jsx,tsx}should “Implement comprehensive testing including unit, integration, and E2E tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/headless/src/primitives/collapsible/collapsible.test.tsx` around lines 142 - 189, Add a new test case in the 'animation lifecycle' describe block that verifies the close-transition re-pin behavior. The test should render the collapsible in an open state (using defaultOpen: true), then trigger the close action, and assert that the --collapsible-panel-height and --collapsible-panel-width CSS variables are re-pinned on the panel element (with the data-cl-slot="collapsible-panel" selector) during the closing animation. This mirrors the existing "pins --collapsible-panel-height/width while the open animation is in progress" test but for the close path, ensuring that both dimensions are preserved while the close animation is in progress before being cleared once the animation completes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/headless/src/utils/reset-layout-styles.ts`:
- Around line 22-40: The originalLayoutStyles object only captures the property
values but not their priority flags (the !important declaration). When restoring
in the restore() function, this causes properties that originally had !important
to be re-applied without it, changing layout behavior. Modify the code to
capture both the value and priority using element.style.getPropertyPriority(key)
for each key in originalLayoutStyles, then update the restore() function to pass
the captured priority as the third parameter to element.style.setProperty() when
re-applying the values, ensuring the !important flag is preserved if it was
originally present.
---
Nitpick comments:
In `@packages/headless/src/primitives/accordion/accordion.test.tsx`:
- Around line 194-238: The animation lifecycle test suite validates the
open-path behavior for height pinning and clearing, but is missing a test case
for the close-path behavior. Add a new test within the 'animation lifecycle'
describe block that validates the height re-pinning behavior when closing
starts. This test should open an accordion panel (using defaultValue), then
simulate closing it and verify that the --cl-accordion-panel-height CSS variable
is re-pinned to the measured pixel height during the close animation phase,
similar to the pattern used in the existing 'pins --cl-accordion-panel-height
while the open animation is in progress' test case.
In `@packages/headless/src/primitives/collapsible/collapsible.test.tsx`:
- Around line 142-189: Add a new test case in the 'animation lifecycle' describe
block that verifies the close-transition re-pin behavior. The test should render
the collapsible in an open state (using defaultOpen: true), then trigger the
close action, and assert that the --collapsible-panel-height and
--collapsible-panel-width CSS variables are re-pinned on the panel element (with
the data-cl-slot="collapsible-panel" selector) during the closing animation.
This mirrors the existing "pins --collapsible-panel-height/width while the open
animation is in progress" test but for the close path, ensuring that both
dimensions are preserved while the close animation is in progress before being
cleared once the animation completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3c30d6de-3161-4e3c-a8d1-3ac274415bf5
📒 Files selected for processing (10)
packages/headless/src/primitives/accordion/accordion-panel.tsxpackages/headless/src/primitives/accordion/accordion.test.tsxpackages/headless/src/primitives/collapsible/collapsible-panel.tsxpackages/headless/src/primitives/collapsible/collapsible.test.tsxpackages/headless/src/primitives/tabs/tabs-context.tspackages/headless/src/primitives/tabs/tabs-indicator.tsxpackages/headless/src/primitives/tabs/tabs-list.tsxpackages/headless/src/primitives/tabs/tabs-root.tsxpackages/headless/src/utils/index.tspackages/headless/src/utils/reset-layout-styles.ts
Uh oh!
There was an error while loading. Please reload this page.
kylemac
approved these changes
Jun 18, 2026
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Three correctness fixes in
@clerk/headlessmeasurement primitives, identified by comparing against base-ui'suseCollapsiblePanel.1. Reset flex/grid alignment before measuring
Accordion.PanelandCollapsible.PanelreadscrollHeight/scrollWidthdirectly. Withdisplay: flex/gridand non-defaultjustify-content/align-items/align-content/justify-items, alignment can make the scroll size report smaller than the true content size. A newresetLayoutStylesutil forces those properties toinitialbefore measuring and restores them on the next frame.2. Flow at
autowhen idle, re-pin on closeOnce the open animation settles, the panels now drop the measured pixel size (CSS vars omitted →
height: auto) so content flows naturally, and re-pin a measured value when the close transition starts so exit animations still have a concrete value to animate from.3.
Tabs.Indicatormeasures before paintSwitched the indicator measurement from
useEffecttouseLayoutEffectto remove a one-frame stale indicator position on tab change. This required tracking the tab-list element as context state (not auseRef): a child's layout effect runs before its parent's ref is attached, so the prioruseEffectonly worked by accident (passive effects run after all refs attach). Same pattern base-ui uses.Testing
@clerk/headlessunit suite passes (404 tests), typecheck clean.Summary by CodeRabbit
Summary
Bug Fixes
Tests
Refactor