fixes for uncontrolled to controlled error and other small console warns #1220
Conversation
WalkthroughThe changes update Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccordionItem
participant CollapsiblePrimitiveRoot
User->>CollapsiblePrimitiveRoot: Triggers open/close (click, keyboard)
CollapsiblePrimitiveRoot->>AccordionItem: Calls onOpenChange(newState)
AccordionItem->>AccordionItem: setIsOpen(newState)
sequenceDiagram
participant User
participant DisclosureItem
participant CollapsiblePrimitiveRoot
User->>CollapsiblePrimitiveRoot: Triggers open/close (click, keyboard)
CollapsiblePrimitiveRoot->>DisclosureItem: Calls onOpenChange(newState)
DisclosureItem->>DisclosureItem: setIsOpen(newState)
Estimated code review effort3 (30–60 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/Accordion/fragments/AccordionItem.tsx(1 hunks)src/components/ui/Accordion/tests/Accordion.test.tsx(0 hunks)src/components/ui/Callout/fragments/CalloutRoot.tsx(1 hunks)src/components/ui/Disclosure/fragments/DisclosureItem.tsx(1 hunks)src/core/primitives/Collapsible/tests/Collapsible.test.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/Accordion/tests/Accordion.test.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/components/ui/Callout/fragments/CalloutRoot.tsx (2)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
src/components/ui/Accordion/fragments/AccordionItem.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
src/components/ui/Disclosure/fragments/DisclosureItem.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/Collapsible/tests/Collapsible.test.tsx (2)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
🧬 Code Graph Analysis (1)
src/components/ui/Callout/fragments/CalloutRoot.tsx (1)
src/core/hooks/createDataAttribute/index.ts (1)
useCreateDataAttribute(10-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/components/ui/Callout/fragments/CalloutRoot.tsx (1)
23-23: Good normalization of data attribute prefix to lowercase.This change standardizes the data attribute prefix from
'Callout'to'callout', which will generate consistent lowercase data attributes (e.g.,data-callout-variantinstead ofdata-Callout-variant). This appears to be part of a broader consistency effort across the component library.src/core/primitives/Collapsible/tests/Collapsible.test.tsx (1)
21-24: Excellent improvement to test event-driven behavior.The test now simulates actual user interaction by clicking the trigger element instead of manually rerendering with props. This better reflects real usage patterns and validates that the collapsible primitive properly handles user events and updates its data-state attribute accordingly.
Also applies to: 28-28
| @@ -38,6 +38,7 @@ const AccordionItem: React.FC<AccordionItemProps> = ({ children, value, classNam | |||
| <AccordionItemContext.Provider value={{ itemValue, setItemValue, disabled }}> | |||
| <CollapsiblePrimitive.Root | |||
| open={isOpen} | |||
| onOpenChange={setIsOpen} | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify state synchronization between collapsible primitive and accordion context.
Adding onOpenChange={setIsOpen} creates a two-way binding with the collapsible primitive, but there's a potential conflict with the useEffect on lines 24-26 that syncs isOpen with activeItems from the accordion context.
When a user triggers the collapsible to open/close, onOpenChange will update the local isOpen state, but if the accordion context's activeItems doesn't reflect this change, the useEffect might override the user's action.
🏁 Script executed:
#!/bin/bash
# Description: Check how AccordionRoot manages activeItems state and if it responds to individual item changes
# Expected: Find onValueChange or similar handlers in AccordionRoot that update activeItems
ast-grep --pattern 'const AccordionRoot = $_' -A 50Length of output: 4072
🏁 Script executed:
#!/bin/bash
# Inspect AccordionItem around onOpenChange and state management
rg -n -C5 "AccordionItem" -n src/components/ui/Accordion/fragments/AccordionItem.tsx
rg -n -C5 "onOpenChange" src/components/ui/Accordion/fragments/AccordionItem.tsxLength of output: 2320
🏁 Script executed:
#!/bin/bash
# Check for useEffect syncing isOpen with activeItems in AccordionItem
rg -n "useEffect" -C5 src/components/ui/Accordion/fragments/AccordionItem.tsxLength of output: 1123
Ensure AccordionItem updates accordion context instead of only local state
The onOpenChange={setIsOpen} handler only updates the local isOpen flag, but never calls setActiveItems on the parent AccordionContext. As a result, your useEffect that syncs isOpen from activeItems will immediately override any local toggle. You need to:
• Destructure setActiveItems and openMultiple from AccordionContext.
• Replace the local setIsOpen callback with a handler that adds or removes itemValue from activeItems.
Affected locations in src/components/ui/Accordion/fragments/AccordionItem.tsx:
- Line 21: Update context destructuring.
- Lines 40–41: Replace
onOpenChange={setIsOpen}.
Suggested diff:
--- a/src/components/ui/Accordion/fragments/AccordionItem.tsx
+++ b/src/components/ui/Accordion/fragments/AccordionItem.tsx
@@ 21,1 +21,1 @@
- const { rootClass, activeItems, transitionDuration, transitionTimingFunction } = useContext(AccordionContext);
+ const { rootClass, activeItems, setActiveItems, openMultiple, transitionDuration, transitionTimingFunction } = useContext(AccordionContext);
@@ 39,2 +39,11 @@
<CollapsiblePrimitive.Root
- open={isOpen}
- onOpenChange={setIsOpen}
+ open={isOpen}
+ onOpenChange={(open) => {
+ if (open) {
+ setActiveItems(openMultiple
+ ? [...activeItems, itemValue]
+ : [itemValue]
+ );
+ } else {
+ setActiveItems(activeItems.filter((v) => v !== itemValue));
+ }
+ }}This ensures toggles update the accordion’s activeItems, and your useEffect will correctly derive isOpen from that shared state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onOpenChange={setIsOpen} | |
| // Line 21: include setActiveItems and openMultiple from context | |
| const { | |
| rootClass, | |
| activeItems, | |
| setActiveItems, | |
| openMultiple, | |
| transitionDuration, | |
| transitionTimingFunction, | |
| } = useContext(AccordionContext); | |
| // … | |
| <CollapsiblePrimitive.Root | |
| open={isOpen} | |
| onOpenChange={(open) => { | |
| if (open) { | |
| setActiveItems( | |
| openMultiple | |
| ? [...activeItems, itemValue] | |
| : [itemValue] | |
| ); | |
| } else { | |
| setActiveItems( | |
| activeItems.filter((v) => v !== itemValue) | |
| ); | |
| } | |
| }} | |
| > | |
| {/* … */} | |
| </CollapsiblePrimitive.Root> |
🤖 Prompt for AI Agents
In src/components/ui/Accordion/fragments/AccordionItem.tsx around lines 21 and
40-41, the onOpenChange handler currently only updates local isOpen state and
does not update the accordion context's activeItems. To fix this, destructure
setActiveItems and openMultiple from AccordionContext at line 21, then replace
onOpenChange={setIsOpen} with a handler that updates activeItems by adding or
removing itemValue accordingly. This will ensure the accordion context state is
updated and the useEffect syncing isOpen from activeItems works correctly.
| @@ -32,6 +32,7 @@ const DisclosureItem = ({ children, className = '', value }:DisclosureItemProps) | |||
|
|
|||
| <CollapsiblePrimitive.Root | |||
| open={isOpen} | |||
| onOpenChange={setIsOpen} | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify state synchronization between collapsible primitive and disclosure context.
Similar to the AccordionItem, adding onOpenChange={setIsOpen} creates a potential conflict with the useEffect on lines 20-22 that syncs isOpen with activeItem from the disclosure context.
When the collapsible primitive changes state, onOpenChange will update the local isOpen state, but if the disclosure context's activeItem doesn't reflect this change, the useEffect might override the user's action.
🏁 Script executed:
#!/bin/bash
# Description: Check how DisclosureRoot manages activeItem state and if it responds to individual item changes
# Expected: Find state management logic in DisclosureRoot that updates activeItem
ast-grep --pattern 'const DisclosureRoot = $_' -A 50Length of output: 3378
🏁 Script executed:
#!/bin/bash
# Inspect DisclosureItem and AccordionItem implementations for state sync patterns
# Locate and display DisclosureItem.tsx
echo "=== DisclosureItem.tsx ==="
fd DisclosureItem.tsx -t f
cat src/components/ui/Disclosure/fragments/DisclosureItem.tsx
# Locate and display AccordionItem.tsx for comparison
echo -e "\n=== AccordionItem.tsx ==="
fd AccordionItem.tsx -t f
cat src/components/ui/Accordion/fragments/AccordionItem.tsxLength of output: 4404
🏁 Script executed:
#!/bin/bash
# Inspect DisclosureTrigger and DisclosureContent implementations for context interactions
echo "=== DisclosureTrigger.tsx ==="
fd DisclosureTrigger.tsx -t f
cat src/components/ui/Disclosure/fragments/DisclosureTrigger.tsx
echo -e "\n=== DisclosureContent.tsx ==="
fd DisclosureContent.tsx -t f
cat src/components/ui/Disclosure/fragments/DisclosureContent.tsxLength of output: 2932
Remove local onOpenChange in favor of context-driven state
The onOpenChange={setIsOpen} on line 35 in DisclosureItem creates a one-way update to the local isOpen flag—but the disclosure context’s activeItem never changes, so your useEffect immediately resets isOpen to match activeItem. That conflicts with user actions. Since the trigger already calls setActiveItem, you should drive open/closed purely from context:
• File: src/components/ui/Disclosure/fragments/DisclosureItem.tsx
– Remove the onOpenChange={setIsOpen} prop on <CollapsiblePrimitive.Root>
– (Optionally) drop the isOpen/setIsOpen local state entirely and read open={activeItem === itemValue} directly
Suggested diff snippet:
--- a/src/components/ui/Disclosure/fragments/DisclosureItem.tsx
+++ b/src/components/ui/Disclosure/fragments/DisclosureItem.tsx
@@ -33,7 +33,7 @@
<CollapsiblePrimitive.Root
- open={isOpen}
- onOpenChange={setIsOpen}
+ open={activeItem === itemValue}
asChild
>🤖 Prompt for AI Agents
In src/components/ui/Disclosure/fragments/DisclosureItem.tsx at line 35, remove
the onOpenChange={setIsOpen} prop from the CollapsiblePrimitive.Root component
to avoid conflicting local state updates. Instead, eliminate the local isOpen
and setIsOpen state variables and directly control the open state by comparing
activeItem from context with itemValue, passing open={activeItem === itemValue}
to CollapsiblePrimitive.Root.
| @@ -69,7 +69,6 @@ describe('Accordion Component', () => { | |||
| test('displays content when item is focused and enter is pressed', () => { | |||
| render(<TestAccordion />); | |||
| const item1Trigger = screen.getByText('Item 1'); | |||
| item1Trigger.focus(); | |||
There was a problem hiding this comment.
why? this doesnt make sense
There was a problem hiding this comment.
making it focus gives error from rovingFocus in tests
making it focused doesnt test anything. The test makes it focused and then fires click event.
There was a problem hiding this comment.
This is asking you to wrap the test inside an act function
There was a problem hiding this comment.
i'll make the changes
closes #1187
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor