fix: Collapsible content never renders in controlled mode#8777
Open
ofried-acutis wants to merge 2 commits intomakeplane:previewfrom
Open
fix: Collapsible content never renders in controlled mode#8777ofried-acutis wants to merge 2 commits intomakeplane:previewfrom
ofried-acutis wants to merge 2 commits intomakeplane:previewfrom
Conversation
The Collapsible component uses headlessui's Disclosure and Transition components, but they prevent content from rendering in controlled mode. The Disclosure.Button has its own internal toggle state that conflicts with the controlled isOpen prop, and the Transition grid-rows animation doesn't produce a visible expand/collapse effect. This affects sub-work items on issue detail pages: clicking the toggle arrow changes the arrow direction but the content never appears. Replace Disclosure/Transition with a plain div/button and simple conditional rendering. Steps to reproduce: 1. Create an issue with sub-work items 2. Open the issue detail page 3. Click the sub-work items toggle arrow 4. Content never appears despite arrow toggling
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced Headless UI Disclosure/Transition with plain HTML elements and conditional rendering; adjusted local open-state initialization to explicitly distinguish Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/collapsible/collapsible.tsx (1)
23-23: State initialization fix correctly handles controlled vs uncontrolled modes.The change from
isOpen || defaultOpen ? true : falsetoisOpen !== undefined ? isOpen : !!defaultOpenproperly distinguishes betweenisOpen={false}(controlled, closed) andisOpen={undefined}(uncontrolled, defers todefaultOpen).However, the PropEl wrapper at
packages/propel/src/collapsible/collapsible.tsx:50usescontrolledIsOpen || defaultOpen || false, which has the same bug pattern—treatingcontrolledIsOpen={false}as falsy and falling back todefaultOpen. Apply the same fix there to ensure consistent behavior across both packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/collapsible/collapsible.tsx` at line 23, PropEl's controlled state fallback incorrectly treats controlledIsOpen={false} as falsy and falls back to defaultOpen; update the logic in the PropEl wrapper (the variable/prop named controlledIsOpen) to mirror the other component: use a ternary that checks controlledIsOpen !== undefined ? controlledIsOpen : !!defaultOpen so a false explicit prop is honored and undefined falls back to defaultOpen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/collapsible/collapsible.tsx`:
- Around line 42-49: The button is missing the accessibility attribute
aria-expanded; update the rendered button in the collapsible component to
include aria-expanded={localIsOpen} so assistive tech knows the open/closed
state—modify the JSX for the button element (the one using buttonRef,
className={buttonClassName}, onClick={handleOnClick}, and rendering {title}) to
add aria-expanded based on the localIsOpen state.
---
Nitpick comments:
In `@packages/ui/src/collapsible/collapsible.tsx`:
- Line 23: PropEl's controlled state fallback incorrectly treats
controlledIsOpen={false} as falsy and falls back to defaultOpen; update the
logic in the PropEl wrapper (the variable/prop named controlledIsOpen) to mirror
the other component: use a ternary that checks controlledIsOpen !== undefined ?
controlledIsOpen : !!defaultOpen so a false explicit prop is honored and
undefined falls back to defaultOpen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4a2fd56-48ff-4459-92e9-74b3b3d58ffc
📒 Files selected for processing (1)
packages/ui/src/collapsible/collapsible.tsx
Author
|
So my pull request looks good? Or no?
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: coderabbitai[bot] ***@***.***>
Sent: Thursday, March 19, 2026 7:02:59 PM
To: makeplane/plane ***@***.***>
Cc: Oliver Fried ***@***.***>; Author ***@***.***>
Subject: Re: [makeplane/plane] fix: Collapsible content never renders in controlled mode (PR #8777)
@coderabbitai[bot] commented on this pull request.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/collapsible/collapsible.tsx (1)
23-23: State initialization fix correctly handles controlled vs uncontrolled modes.
The change from isOpen || defaultOpen ? true : false to isOpen !== undefined ? isOpen : !!defaultOpen properly distinguishes between isOpen={false} (controlled, closed) and isOpen={undefined} (uncontrolled, defers to defaultOpen).
However, the PropEl wrapper at packages/propel/src/collapsible/collapsible.tsx:50 uses controlledIsOpen || defaultOpen || false, which has the same bug pattern—treating controlledIsOpen={false} as falsy and falling back to defaultOpen. Apply the same fix there to ensure consistent behavior across both packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In ***@***.***/ui/src/collapsible/collapsible.tsx` at line 23, PropEl's controlled
state fallback incorrectly treats controlledIsOpen={false} as falsy and falls
back to defaultOpen; update the logic in the PropEl wrapper (the variable/prop
named controlledIsOpen) to mirror the other component: use a ternary that checks
controlledIsOpen !== undefined ? controlledIsOpen : !!defaultOpen so a false
explicit prop is honored and undefined falls back to defaultOpen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In ***@***.***/ui/src/collapsible/collapsible.tsx`:
- Around line 42-49: The button is missing the accessibility attribute
aria-expanded; update the rendered button in the collapsible component to
include aria-expanded={localIsOpen} so assistive tech knows the open/closed
state—modify the JSX for the button element (the one using buttonRef,
className={buttonClassName}, onClick={handleOnClick}, and rendering {title}) to
add aria-expanded based on the localIsOpen state.
---
Nitpick comments:
In ***@***.***/ui/src/collapsible/collapsible.tsx`:
- Line 23: PropEl's controlled state fallback incorrectly treats
controlledIsOpen={false} as falsy and falls back to defaultOpen; update the
logic in the PropEl wrapper (the variable/prop named controlledIsOpen) to mirror
the other component: use a ternary that checks controlledIsOpen !== undefined ?
controlledIsOpen : !!defaultOpen so a false explicit prop is honored and
undefined falls back to defaultOpen.
________________________________
ℹ️ Review info ⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4a2fd56-48ff-4459-92e9-74b3b3d58ffc
📥 Commits
Reviewing files that changed from the base of the PR and between 1faf06c<1faf06c> and fe1301a<fe1301a>.
📒 Files selected for processing (1)
* packages/ui/src/collapsible/collapsible.tsx
________________________________
In packages/ui/src/collapsible/collapsible.tsx<#8777 (comment)>:
+ <button ref={buttonRef} type="button" className={buttonClassName} onClick={handleOnClick}>
{title}
- </Disclosure.Button>
- <Transition
- show={localIsOpen}
- enter="transition-all duration-300 ease-in-out"
- enterFrom="grid-rows-[0fr] opacity-0"
- enterTo="grid-rows-[1fr] opacity-100"
- leave="transition-all duration-300 ease-in-out"
- leaveFrom="grid-rows-[1fr] opacity-100"
- leaveTo="grid-rows-[0fr] opacity-0"
- className="grid overflow-hidden"
- >
- <Disclosure.Panel static className="min-h-0">
+ </button>
+ {localIsOpen && (
+ <div className="min-h-0">
{children}
- </Disclosure.Panel>
- </Transition>
- </Disclosure>
+ </div>
+ )}
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
However, there are two items flagged in the review that should be addressed before this is fully ready: 1. Since - <button ref={buttonRef} type="button" className={buttonClassName} onClick={handleOnClick}>
+ <button ref={buttonRef} type="button" className={buttonClassName} onClick={handleOnClick} aria-expanded={localIsOpen}>
{title}
</button>2. 🧹 Same bug in the PropEl wrapper (nitpick)
- controlledIsOpen || defaultOpen || false
+ controlledIsOpen !== undefined ? controlledIsOpen : !!defaultOpenSummary: ✅ The main fix is correct. Address the two items above (especially |
Author
|
Can you make the changes yourself ?
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: coderabbitai[bot] ***@***.***>
Sent: Thursday, March 19, 2026 7:13:31 PM
To: makeplane/plane ***@***.***>
Cc: Oliver Fried ***@***.***>; Author ***@***.***>
Subject: Re: [makeplane/plane] fix: Collapsible content never renders in controlled mode (PR #8777)
[https://avatars.githubusercontent.com/in/347564?s=20&v=4]coderabbitai[bot] left a comment (makeplane/plane#8777)<#8777 (comment)>
Tip
For best results, initiate chat on the files or code changes.
@ofried-acutis — the core fix is solid! Replacing the headlessui Disclosure/Transition with a plain <button>/<div> and conditional rendering directly resolves the root cause of the controlled-mode bug, and the state initialization fix (isOpen !== undefined ? isOpen : !!defaultOpen) correctly handles isOpen={false} vs isOpen={undefined}.
However, there are two items flagged in the review that should be addressed before this is fully ready:
________________________________
1.
________________________________
2. 🧹 Same bug in the PropEl wrapper (nitpick)
packages/propel/src/collapsible/collapsible.tsx (line 50) appears to use controlledIsOpen || defaultOpen || false, which has the same issue your fix addresses — controlledIsOpen={false} is treated as falsy and incorrectly falls back to defaultOpen. The same ternary pattern should be applied there:
- controlledIsOpen || defaultOpen || false
+ controlledIsOpen !== undefined ? controlledIsOpen : !!defaultOpen
________________________________
Summary: ✅ The main fix is correct. Address the two items above (especially aria-expanded for accessibility) and the PR should be good to merge.
—
Reply to this email directly, view it on GitHub<#8777?email_source=notifications&email_token=BQ3NZMSXC5AMIOX7QTP7B634RQ2EXA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMBZGIZDMOJTGU4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4092269358>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BQ3NZMSBP6FEE3NJUQOVAGL4RQ2EXAVCNFSM6AAAAACWYIHTLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DAOJSGI3DSMZVHA>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
- Add aria-expanded={localIsOpen} to button for accessibility
- Fix same controlled state bug in propel collapsible wrapper
(controlledIsOpen={false} was treated as falsy, falling back
to defaultOpen)
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
The
Collapsiblecomponent inpackages/uinever shows its content when used in controlled mode (isOpenprop). This breaks sub-work items on issue detail pages — clicking the toggle arrow changes the arrow direction but the content never appears.Root Cause
Two issues in the current implementation:
Disclosure.Buttonfrom headlessui has its own internal toggle state that conflicts with the controlledisOpenprop — the internal state andlocalIsOpenget out of syncTransitionwithgrid-rows-[0fr]/grid-rows-[1fr]animation classes doesn't produce a visible expand/collapse effectFix
Replace
Disclosure/Transitionfrom headlessui with a plaindiv/buttonand simple conditional rendering ({localIsOpen && ...}).Steps to Reproduce
Test Plan
isOpenprop) still worksSummary by CodeRabbit