refactor: update Accordion component to use string values and enhance context API #1326
Conversation
… context API - Changed activeItems and itemValue types from (number | string) to string for consistency. - Introduced new properties in AccordionContext: type, collapsible, disabled, dir, forceMount, and hiddenUntilFound. - Updated AccordionItem and AccordionContent to accommodate new context properties. - Enhanced AccordionTrigger logic to respect collapsible behavior. - Adjusted stories and tests to reflect changes in value handling and ensure compatibility with Radix UI.
WalkthroughThis change refactors the Accordion UI component system to use string-only item values, replacing previous support for numeric or mixed types. It introduces new props for configuration (such as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccordionRoot
participant AccordionContext
participant AccordionItem
participant AccordionItemContext
participant AccordionContent
participant AccordionTrigger
User->>AccordionRoot: Renders with props (type, collapsible, etc.)
AccordionRoot->>AccordionContext: Provides context (activeItems, type, etc.)
AccordionRoot->>AccordionItem: Renders with value (string)
AccordionItem->>AccordionItemContext: Provides itemValue (string), disabled
AccordionTrigger->>AccordionContext: Reads type, collapsible, setActiveItems
User->>AccordionTrigger: Clicks trigger
AccordionTrigger->>AccordionContext: setActiveItems (toggles item)
AccordionContent->>AccordionContext: Reads forceMount, hiddenUntilFound, open state
AccordionContent-->>User: Renders content based on open state and forceMount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mrkazmi333
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
src/components/ui/Accordion/fragments/AccordionItem.tsx (1)
46-46: No-op setter inAccordionItemContextIf
setItemValueis no longer used anywhere, consider removing it from the context type to simplify the API. If kept for compatibility, leaving a no-op is fine.src/components/ui/Accordion/contexts/AccordionContext.tsx (1)
5-7: Consider supporting functional updates insetActiveItemsTo avoid stale closures and ease incremental updates in consumers, broaden the setter signature:
- setActiveItems: (items: string[]) => void; + setActiveItems: (items: string[] | ((prev: string[]) => string[])) => void;The default can remain a no-op; providers can accept both forms.
src/components/ui/Accordion/stories/Accordion.stories.tsx (2)
101-107: Prefertype="multiple"over deprecatedopenMultiplein storiesYou already added
TypeMultiple. Consider removing or markingOpenMultipleas deprecated in the docs to avoid API confusion.
1-5: Unused import:ButtonIf not used in any story here, remove it to keep stories clean.
src/components/ui/Accordion/fragments/AccordionRoot.tsx (2)
83-91: Collapsible safeguard is good; consider messaging levelThe guard preventing close-all when
type='single'andcollapsible=falseis correct. You might switch theconsole.warnto a development-only warning or standardize via your logger to avoid noisy consoles in production.
117-123: Optionally expose disabled state on the root for styling/accessibilityAdd
data-disabled(and optionallyaria-disabled) to advertise the disabled state at the root:<Primitive.div className={clsx(`${rootClass}-root`)} ref={accordionRef} asChild={asChild} dir={dir} data-orientation={orientation} data-type={actualType} + data-disabled={disabled ? '' : undefined} + aria-disabled={disabled || undefined} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/ui/Accordion/contexts/AccordionContext.tsx(2 hunks)src/components/ui/Accordion/contexts/AccordionItemContext.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionContent.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionItem.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionTrigger.tsx(2 hunks)src/components/ui/Accordion/stories/Accordion.stories.tsx(2 hunks)src/components/ui/Accordion/stories/AccordionRootVisualTests.stories.tsx(4 hunks)src/components/ui/Accordion/tests/Accordion.test.tsx(3 hunks)src/components/ui/Skeleton/stories/Skeleton.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2025-04-07T04:38:34.864Z
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.
Applied to files:
src/components/ui/Accordion/tests/Accordion.test.tsxsrc/components/ui/Accordion/stories/Accordion.stories.tsxsrc/components/ui/Accordion/fragments/AccordionRoot.tsxsrc/components/ui/Accordion/fragments/AccordionItem.tsx
📚 Learning: 2024-12-12T08:34:33.079Z
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`.
Applied to files:
src/components/ui/Accordion/stories/Accordion.stories.tsxsrc/components/ui/Accordion/fragments/AccordionTrigger.tsx
🧬 Code Graph Analysis (4)
src/components/ui/Accordion/stories/AccordionRootVisualTests.stories.tsx (1)
docs/app/docs/components/accordion/docs/anatomy.tsx (1)
Accordion(3-14)
src/components/ui/Skeleton/stories/Skeleton.stories.tsx (1)
docs/app/docs/components/accordion/docs/anatomy.tsx (1)
Accordion(3-14)
src/components/ui/Accordion/fragments/AccordionContent.tsx (2)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1)
AccordionContext(18-31)src/components/ui/Accordion/contexts/AccordionItemContext.tsx (1)
AccordionItemContext(9-13)
src/components/ui/Accordion/fragments/AccordionItem.tsx (2)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1)
AccordionContext(18-31)src/components/ui/Accordion/contexts/AccordionItemContext.tsx (1)
AccordionItemContext(9-13)
🔇 Additional comments (18)
src/components/ui/Accordion/stories/AccordionRootVisualTests.stories.tsx (1)
13-13: LGTM! Consistent string value format for Accordion items.The changes consistently apply string-based
valueprops using the"item-{number}"pattern, which aligns perfectly with the broader refactor to standardize Accordion item values as strings only.Also applies to: 34-34, 54-54, 62-62, 81-81, 89-89
src/components/ui/Skeleton/stories/Skeleton.stories.tsx (1)
111-111: LGTM! Proper dynamic string value generation.The template literal syntax correctly generates string-based values in the
"item-{index}"format, maintaining consistency with the Accordion component's refactor to string-only item values.src/components/ui/Accordion/contexts/AccordionItemContext.tsx (2)
4-5: LGTM! Type refinement aligns with string-only refactor.The changes correctly narrow the
itemValuetype fromnumber | stringtostringand update thesetItemValuefunction signature accordingly, supporting the broader refactor to standardize item values as strings.
10-10: LGTM! Consistent default value.The default
itemValueis correctly updated to an empty string, maintaining type consistency with the string-only refactor.src/components/ui/Accordion/tests/Accordion.test.tsx (4)
21-21: LGTM! Dynamic string value generation in tests.The template literal correctly generates string-based values for test items, maintaining consistency with the Accordion component's string-only refactor.
113-117: LGTM! Proper controlled mode adaptation.The state type is correctly narrowed to
string[]and the handler function properly normalizes theonValueChangecallback input, ensuring compatibility with the updated Accordion API that uses string-only values.
122-124: LGTM! String values in controlled mode test.The button handlers correctly use string values (
'item-1','item-0','item-2') that align with the string-only refactor and maintain test functionality.
156-156: LGTM! Default value test updated correctly.The
defaultValueprop correctly uses string array format with['item-2'], maintaining consistency with the string-only item value refactor.src/components/ui/Accordion/fragments/AccordionTrigger.tsx (5)
11-16: LGTM! Exported props type improves reusability.Exporting the
AccordionTriggerPropstype enhances type reusability across the codebase and follows TypeScript best practices.
24-34: LGTM! Enhanced context integration with proper disabled state handling.The extraction of additional context values (
type,collapsible,disabled) provides more granular control. The combinedisDisabledlogic correctly handles both root-level and item-level disabled states, ensuring comprehensive disable functionality.
37-37: LGTM! Defensive programming with early exit.The early return when
isDisabledis true prevents unnecessary processing and ensures the component respects disabled states properly.
41-58: LGTM! Improved toggle logic with semantic type checking.The transition from
openMultipleboolean totype === 'multiple'is more semantic and aligns with the enhanced context API. The collapsible constraint check (lines 50-53) properly prevents unwanted item closing whencollapsibleis false, improving user experience consistency.
63-71: LGTM! Enhanced accessibility and unified disabled handling.The unified
isDisabledflag usage across both theCollapsiblePrimitive.Triggerand button element ensures consistent behavior. The enhancedaria-controlswith fallback toitemValuewhenindexis not provided improves robustness. The newdata-stateattribute provides valuable state information for styling and testing purposes.src/components/ui/Accordion/fragments/AccordionItem.tsx (1)
13-13: String-only, requiredvaluelooks goodAligns with the refactor to string identifiers and Radix compatibility.
src/components/ui/Accordion/fragments/AccordionContent.tsx (1)
33-38: Render gating looks correct
forceMount || rootForceMount || isOpenis a clear and predictable rule, withhiddenUntilFoundhandled separately.src/components/ui/Accordion/contexts/AccordionContext.tsx (1)
25-30: Defaults align with the new APINew properties (
type,collapsible,disabled,dir,forceMount,hiddenUntilFound) and defaults look consistent.src/components/ui/Accordion/stories/Accordion.stories.tsx (1)
73-73: Value as string key LGTMUsing
value={item-${index}}aligns with the string-only refactor.src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
62-65: Back-compat mapping reads well
openMultiple->type,keepMounted->forceMount. Clear and safe defaults.
…sibility - Added useRef and useEffect hooks in AccordionContent to manage the hidden attribute for better accessibility. - Simplified state management in AccordionItem by removing unnecessary useState and useEffect hooks. - Updated onValueChange type in AccordionRoot to accept undefined, improving type safety. - Adjusted AccordionItem to directly use activeItems for determining open state.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
77-83: Correctly mapping internal array state to publiconValueChangeshapeThis adapts single mode to emit
string | undefinedand multiple to emitstring[]. Matches Radix expectations.
🧹 Nitpick comments (6)
src/components/ui/Accordion/fragments/AccordionRoot.tsx (6)
31-33: Broadenvaluetype to allow explicitundefinedin controlled single modeWith strictNullChecks, passing
value={undefined}is not assignable tostring | string[]. Allowing explicitundefinedimproves DX for controlled single/collapsible usage.Apply this diff:
- value?: string | string[]; + value?: string | string[] | undefined;Optionally, consider a discriminated union for stronger typing:
- Single: value?: string; defaultValue?: string; onValueChange?: (v: string | undefined) => void
- Multiple: value?: string[]; defaultValue?: string[]; onValueChange?: (v: string[]) => void
63-65: Emit deprecation warnings when legacy props are used (and when both new and legacy are supplied)Helps consumers migrate and avoids silent precedence.
Apply this diff:
const actualType = openMultiple !== undefined ? (openMultiple ? 'multiple' : 'single') : type; const actualForceMount = keepMounted !== undefined ? keepMounted : forceMount; + if (process.env.NODE_ENV !== 'production') { + if (openMultiple !== undefined) { + console.warn('Accordion: `openMultiple` is deprecated; use `type="multiple"` instead.'); + } + if (keepMounted !== undefined) { + console.warn('Accordion: `keepMounted` is deprecated; use `forceMount` instead.'); + } + if (openMultiple !== undefined && type !== undefined) { + console.warn('Accordion: Both `openMultiple` and `type` provided; `openMultiple` takes precedence.'); + } + if (keepMounted !== undefined && forceMount !== undefined) { + console.warn('Accordion: Both `keepMounted` and `forceMount` provided; `keepMounted` takes precedence.'); + } + }
67-75: Memoize processed values to avoid new array identities each renderDeriving arrays like
[value]on every render can cause unnecessary work downstream (especially ifuseControllableStateshallow-compares). Memoize byvalue/defaultValueandactualType.Apply this diff:
- const processedValue = value !== undefined - ? (actualType === 'multiple' - ? (Array.isArray(value) ? value : [value]) - : (Array.isArray(value) ? (value.length > 0 ? [value[0]] : []) : [value])) - : undefined; + const processedValue = React.useMemo(() => { + if (value === undefined) return undefined; + if (actualType === 'multiple') { + return Array.isArray(value) ? value : [value]; + } + // single + return Array.isArray(value) ? (value.length > 0 ? [value[0]] : []) : [value]; + }, [value, actualType]); - const processedDefaultValue = actualType === 'multiple' - ? (Array.isArray(defaultValue) ? defaultValue : [defaultValue]) - : (Array.isArray(defaultValue) ? (defaultValue.length > 0 ? [defaultValue[0]] : []) : [defaultValue]); + const processedDefaultValue = React.useMemo(() => { + if (actualType === 'multiple') { + return Array.isArray(defaultValue) ? defaultValue : [defaultValue]; + } + // single + return Array.isArray(defaultValue) ? (defaultValue.length > 0 ? [defaultValue[0]] : []) : [defaultValue]; + }, [defaultValue, actualType]);Also update the import:
// Line 2 import React, { useRef, useMemo } from 'react';
85-93: StabilizehandleValueChangeidentity and gate console warning to dev only
- Wrap in
useCallbackto avoid re-renders in consumers subscribing to context.- Avoid console noise in production.
Apply this diff:
- const handleValueChange = (newValue: string[]) => { - if (actualType === 'single' && !collapsible && newValue.length === 0) { - // Prevent closing all items when collapsible is false - console.warn('Accordion: Cannot close all items when collapsible is false'); - return; - } - setActiveItems(newValue); - }; + const handleValueChange = React.useCallback((newValue: string[]) => { + if (actualType === 'single' && !collapsible && newValue.length === 0) { + // Prevent closing all items when collapsible is false + if (process.env.NODE_ENV !== 'production') { + console.warn('Accordion: Cannot close all items when collapsible is false'); + } + return; + } + setActiveItems(newValue); + }, [actualType, collapsible, setActiveItems]);And update the import:
// Line 2 import React, { useRef, useCallback } from 'react';
84-85: Clamp internal state when switching from multiple → single at runtimeIf
typechanges after mount (stories often toggle),activeItemsmay carry multiple entries whilesingleexpects at most one. Clamp to the first item.Apply this diff:
+ + React.useEffect(() => { + if (actualType === 'single' && activeItems.length > 1) { + setActiveItems([activeItems[0]]); + } + }, [actualType, activeItems, setActiveItems]);Please confirm whether runtime
typetoggling is supported in this library. If not, documenting it as a static prop would also be acceptable.
111-125: Accessibility/data attributes are good; consider addingdata-disabledanddata-collapsibleThese extra flags can simplify styling and automated tests.
Example:
- data-orientation={orientation} - data-type={actualType} + data-orientation={orientation} + data-type={actualType} + data-disabled={disabled ? '' : undefined} + data-collapsible={collapsible ? '' : undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Accordion/fragments/AccordionContent.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionItem.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/Accordion/fragments/AccordionContent.tsx
- src/components/ui/Accordion/fragments/AccordionItem.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-04-07T04:38:34.864Z
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.
Applied to files:
src/components/ui/Accordion/fragments/AccordionRoot.tsx
🔇 Additional comments (2)
src/components/ui/Accordion/fragments/AccordionRoot.tsx (2)
21-26: New context props and API surface look solidThe added props (type, collapsible, disabled, dir, forceMount, hiddenUntilFound) align with Radix semantics and give consumers needed control. Defaults are sensible.
36-51: Sane defaults and backward-compat-friendly surfaceDefault values (orientation, loop, dir, etc.) are consistent with context and typical Accordion behavior. Using
defaultValue = []ensures closed-by-default in single mode.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests