fix(Accordion): sync defaultExpanded with shared accordion state - #2756
Open
Devil1716 wants to merge 1 commit into
Open
fix(Accordion): sync defaultExpanded with shared accordion state#2756Devil1716 wants to merge 1 commit into
Devil1716 wants to merge 1 commit into
Conversation
Seed defaultExpanded=true into context items and always drive Disclosure from isExpanded so items can toggle and exclusive open/close works. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Reviewer's GuideEnsures Accordion items using defaultExpanded=true stay synchronized with the shared accordion state by always controlling Disclosure via accordion state, seeding default-expanded items into the shared items collection, and adding a regression test for exclusive-open behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/components/Accordion/__tests__/Accordion.test.tsx" line_range="294-303" />
<code_context>
+ test('AccordionItem with defaultExpanded=true closes when another item opens', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary test for non-exclusive (multiple-open) accordions with defaultExpanded items
Consider adding a sibling test for the non-exclusive Accordion case (e.g. when `multiple` is enabled). In that scenario, a `defaultExpanded` item should remain open when another item opens and stay consistent with the shared state. This will help catch regressions where the seeding logic incorrectly forces items closed for multiple-open accordions.
Suggested implementation:
```typescript
expect(content2.className).toContain('g-disclosure__content_visible');
});
test('AccordionItem with defaultExpanded=true remains open in multiple mode when another item opens', async () => {
const user = userEvent.setup();
render(
<Accordion multiple>
<Accordion.Item qa="item-1" summary="Item 1" defaultExpanded>
Content 1
</Accordion.Item>
<Accordion.Item qa="item-2" summary="Item 2">
Content 2
</Accordion.Item>
</Accordion>,
);
const item1Summary = screen.getByText('Item 1');
const item2Summary = screen.getByText('Item 2');
const content1 = screen.getByText('Content 1');
const content2 = screen.getByText('Content 2');
// defaultExpanded item should start open
expect(content1.className).toContain('g-disclosure__content_visible');
expect(content2.className).not.toContain('g-disclosure__content_visible');
// opening another item in multiple mode should NOT close the defaultExpanded item
await user.click(item2Summary);
expect(content1.className).toContain('g-disclosure__content_visible');
expect(content2.className).toContain('g-disclosure__content_visible');
});
test('AccordionItem with defaultExpanded=true closes when another item opens', async () => {
const user = userEvent.setup();
render(
<Accordion>
<Accordion.Item qa="item-1" summary="Item 1" defaultExpanded>
Content 1
</Accordion.Item>
<Accordion.Item qa="item-2" summary="Item 2">
Content 2
</Accordion.Item>
```
This change assumes that `screen` from `@testing-library/react` is already imported in this test file, and that `"g-disclosure__content_visible"` is the canonical class used elsewhere in the tests to assert visibility.
If the existing tests use different query helpers (e.g. `render` return values instead of `screen`) or different visibility checks, mirror those conventions in the new test (e.g. replace `screen.getByText` with the existing pattern for locating summaries and content nodes).
</issue_to_address>
### Comment 2
<location path="src/components/Accordion/AccordionItem/AccordionItem.tsx" line_range="121" />
<code_context>
+ }
+ }, [isControlledItem, defaultExpanded, itemValue, items, updateItems]);
const isExpanded = React.useMemo(() => {
if (isControlledItem) {
return expanded;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a shared helper to determine whether an accordion item is open and reuse it in both the seeding effect and `isExpanded` to simplify the control flow.
You can keep the new behavior but reduce the cognitive load by centralizing the “is this item open?” logic and reusing it across the effect and `isExpanded`. That removes the repeated array/scalar branching and makes the seeding flow easier to follow.
For example:
```ts
function isItemOpen(
items: string | string[] | undefined,
itemValue: string,
): boolean {
if (items == null) return false;
return Array.isArray(items) ? items.includes(itemValue) : items === itemValue;
}
function useAccordionItemState({
expanded,
defaultExpanded,
value,
onUpdate,
}: {
expanded?: boolean;
defaultExpanded?: boolean;
value?: string;
onUpdate?: (next: boolean) => void;
}) {
const id = useUniqId();
const {items, updateItems} = useAccordion();
const isControlledItem = expanded !== undefined;
const itemValue = value ?? id;
const seededDefaultExpanded = React.useRef(false);
React.useLayoutEffect(() => {
if (seededDefaultExpanded.current || isControlledItem || defaultExpanded !== true) {
return;
}
seededDefaultExpanded.current = true;
if (!isItemOpen(items, itemValue)) {
updateItems(itemValue);
}
}, [isControlledItem, defaultExpanded, itemValue, items, updateItems]);
const isExpanded = React.useMemo(() => {
if (isControlledItem) {
return expanded;
}
const openInContext = isItemOpen(items, itemValue);
if (openInContext) {
return true;
}
// Honor defaultExpanded on the first paint before the seed effect runs.
if (defaultExpanded === true && !seededDefaultExpanded.current) {
return true;
}
return false;
}, [isControlledItem, expanded, items, itemValue, defaultExpanded]);
const handleUpdate = React.useCallback(
(next: boolean) => {
onUpdate?.(next);
if (!isControlledItem) {
updateItems(itemValue);
}
},
[onUpdate, isControlledItem, updateItems, itemValue],
);
return {id, isExpanded, handleUpdate};
}
```
This keeps:
- The one-time seeding of `defaultExpanded` into the shared accordion state.
- The first-paint `defaultExpanded` behavior.
- The controlled vs uncontrolled distinction.
But it:
- Removes duplicated `Array.isArray(items)` / scalar checks.
- Makes the seeding effect and `isExpanded` both clearly rely on the same `isItemOpen` definition, reducing branching and mental overhead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+294
to
+303
| test('AccordionItem with defaultExpanded=true closes when another item opens', async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| render( | ||
| <Accordion> | ||
| <Accordion.Item qa="item-1" summary="Item 1" defaultExpanded> | ||
| Content 1 | ||
| </Accordion.Item> | ||
| <Accordion.Item qa="item-2" summary="Item 2"> | ||
| Content 2 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a complementary test for non-exclusive (multiple-open) accordions with defaultExpanded items
Consider adding a sibling test for the non-exclusive Accordion case (e.g. when multiple is enabled). In that scenario, a defaultExpanded item should remain open when another item opens and stay consistent with the shared state. This will help catch regressions where the seeding logic incorrectly forces items closed for multiple-open accordions.
Suggested implementation:
expect(content2.className).toContain('g-disclosure__content_visible');
});
test('AccordionItem with defaultExpanded=true remains open in multiple mode when another item opens', async () => {
const user = userEvent.setup();
render(
<Accordion multiple>
<Accordion.Item qa="item-1" summary="Item 1" defaultExpanded>
Content 1
</Accordion.Item>
<Accordion.Item qa="item-2" summary="Item 2">
Content 2
</Accordion.Item>
</Accordion>,
);
const item1Summary = screen.getByText('Item 1');
const item2Summary = screen.getByText('Item 2');
const content1 = screen.getByText('Content 1');
const content2 = screen.getByText('Content 2');
// defaultExpanded item should start open
expect(content1.className).toContain('g-disclosure__content_visible');
expect(content2.className).not.toContain('g-disclosure__content_visible');
// opening another item in multiple mode should NOT close the defaultExpanded item
await user.click(item2Summary);
expect(content1.className).toContain('g-disclosure__content_visible');
expect(content2.className).toContain('g-disclosure__content_visible');
});
test('AccordionItem with defaultExpanded=true closes when another item opens', async () => {
const user = userEvent.setup();
render(
<Accordion>
<Accordion.Item qa="item-1" summary="Item 1" defaultExpanded>
Content 1
</Accordion.Item>
<Accordion.Item qa="item-2" summary="Item 2">
Content 2
</Accordion.Item>This change assumes that screen from @testing-library/react is already imported in this test file, and that "g-disclosure__content_visible" is the canonical class used elsewhere in the tests to assert visibility.
If the existing tests use different query helpers (e.g. render return values instead of screen) or different visibility checks, mirror those conventions in the new test (e.g. replace screen.getByText with the existing pattern for locating summaries and content nodes).
| } | ||
| }, [isControlledItem, defaultExpanded, itemValue, items, updateItems]); | ||
|
|
||
| const isExpanded = React.useMemo(() => { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting a shared helper to determine whether an accordion item is open and reuse it in both the seeding effect and isExpanded to simplify the control flow.
You can keep the new behavior but reduce the cognitive load by centralizing the “is this item open?” logic and reusing it across the effect and isExpanded. That removes the repeated array/scalar branching and makes the seeding flow easier to follow.
For example:
function isItemOpen(
items: string | string[] | undefined,
itemValue: string,
): boolean {
if (items == null) return false;
return Array.isArray(items) ? items.includes(itemValue) : items === itemValue;
}
function useAccordionItemState({
expanded,
defaultExpanded,
value,
onUpdate,
}: {
expanded?: boolean;
defaultExpanded?: boolean;
value?: string;
onUpdate?: (next: boolean) => void;
}) {
const id = useUniqId();
const {items, updateItems} = useAccordion();
const isControlledItem = expanded !== undefined;
const itemValue = value ?? id;
const seededDefaultExpanded = React.useRef(false);
React.useLayoutEffect(() => {
if (seededDefaultExpanded.current || isControlledItem || defaultExpanded !== true) {
return;
}
seededDefaultExpanded.current = true;
if (!isItemOpen(items, itemValue)) {
updateItems(itemValue);
}
}, [isControlledItem, defaultExpanded, itemValue, items, updateItems]);
const isExpanded = React.useMemo(() => {
if (isControlledItem) {
return expanded;
}
const openInContext = isItemOpen(items, itemValue);
if (openInContext) {
return true;
}
// Honor defaultExpanded on the first paint before the seed effect runs.
if (defaultExpanded === true && !seededDefaultExpanded.current) {
return true;
}
return false;
}, [isControlledItem, expanded, items, itemValue, defaultExpanded]);
const handleUpdate = React.useCallback(
(next: boolean) => {
onUpdate?.(next);
if (!isControlledItem) {
updateItems(itemValue);
}
},
[onUpdate, isControlledItem, updateItems, itemValue],
);
return {id, isExpanded, handleUpdate};
}This keeps:
- The one-time seeding of
defaultExpandedinto the shared accordion state. - The first-paint
defaultExpandedbehavior. - The controlled vs uncontrolled distinction.
But it:
- Removes duplicated
Array.isArray(items)/ scalar checks. - Makes the seeding effect and
isExpandedboth clearly rely on the sameisItemOpendefinition, reducing branching and mental overhead.
Contributor
|
Preview is ready. |
Contributor
|
🎭 Component Tests Report is ready. |
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
defaultExpanded={true}into shared accordionitemsso Disclosure stays controlled by accordion stateexpanded={isExpanded}(stop leaving Disclosure uncontrolled whendefaultExpandedis true)defaultExpandeditem closes when another item opens, and can reopenCloses #2718
Test plan
defaultExpanded(existing + new exclusive-open test)Summary by Sourcery
Synchronize AccordionItem defaultExpanded behavior with shared accordion state so items remain controlled and exclusive, and add coverage for the default-expanded exclusive-open scenario.
Bug Fixes:
Tests: