Conversation
Includes trailing newline fix for package.json. Made-with: Cursor
🦋 Changeset detectedLatest commit: 5a393d4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
📝 WalkthroughWalkthroughThis PR introduces a root-level Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccordionRoot
participant AccordionContext
participant AccordionItem
participant AccordionTrigger
User->>AccordionRoot: Render with disabled=true & orientation="horizontal"
AccordionRoot->>AccordionContext: Provide {disabled: true, orientation: "horizontal"}
AccordionRoot->>AccordionRoot: Set data-orientation="horizontal"
AccordionContext->>AccordionItem: Read {disabled: true, orientation: "horizontal"}
AccordionItem->>AccordionItem: Calculate effectiveDisabled = true
AccordionItem->>AccordionItem: Set data-disabled=true, data-orientation="horizontal"
AccordionContext->>AccordionTrigger: Read {disabled: true, orientation: "horizontal"}
AccordionTrigger->>AccordionTrigger: Set data-orientation="horizontal"
User->>AccordionTrigger: Click to toggle
AccordionTrigger->>AccordionItem: Attempt onOpenChange
AccordionItem->>AccordionItem: Block transition (effectiveDisabled=true)
AccordionItem->>User: No state change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage improved or stayed the same. Great job! Run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
210-221:⚠️ Potential issue | 🟡 MinorOverlapping lint-staged patterns may run ESLint twice on some files.
Files in
src/,e2e/,scripts/,test-utils/, or.storybook/directories matching*.{js,jsx,ts,tsx}will match both patterns (lines 211 and 215), causing ESLint to run twice on the same files. Consider consolidating:♻️ Suggested consolidation
"lint-staged": { "*.{js,jsx,ts,tsx}": [ "eslint --fix", "jest --findRelatedTests --passWithNoTests" ], - "{src,e2e,scripts,test-utils,.storybook}/**/*.{js,jsx,ts,tsx}": [ - "eslint --fix" - ], "*.{css,scss,sass}": [ "stylelint --fix" ] },Alternatively, if the directory-specific pattern exists for a reason (e.g., different options), exclude those directories from the first pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 210 - 221, The lint-staged config currently runs ESLint twice because the global pattern "*.{js,jsx,ts,tsx}" overlaps with the directory-specific pattern "{src,e2e,scripts,test-utils,.storybook}/**/*.{js,jsx,ts,tsx}"; fix this by consolidating or excluding: either remove the global pattern and keep only the directory-specific entry, or keep the global pattern and change it to exclude those directories (e.g., "!src/**", "!e2e/**", "!scripts/**", "!test-utils/**", "!.storybook/**") so ESLint only executes once; edit the "lint-staged" block and adjust the two patterns accordingly.
🧹 Nitpick comments (3)
src/components/ui/Accordion/tests/Accordion.a11y.test.tsx (1)
76-94: Good coverage for orientation propagation, but DOM traversal is fragile.The test at lines 86-87 uses
trigger.closest('h3')andheader?.parentElementto find the item element. This assumes a specific DOM structure and may break silently if the component hierarchy changes.Consider using
data-testidattributes on the item/header for more robust element selection, or at minimum add a comment explaining the expected DOM structure.💡 Suggested improvement for robustness
- const header = trigger.closest('h3'); - const item = header?.parentElement; + // DOM structure: Item > Header(h3) > Trigger(button) + const header = trigger.closest('h3'); + const item = header?.closest('[data-orientation]');Or consider adding
data-testid="accordion-item"toAccordionItemfor more reliable selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Accordion/tests/Accordion.a11y.test.tsx` around lines 76 - 94, The test's DOM traversal is fragile because it uses trigger.closest('h3') and header?.parentElement to find the item; update the component and test to use a stable test id instead: add a data-testid (e.g. data-testid="accordion-item") to the AccordionItem (or TestAccordion's rendered item) and then replace the fragile traversal in the test with const item = screen.getByTestId('accordion-item') (keep assertions on header/trigger/content), or alternatively add a brief comment above the traversal explaining the required DOM shape if you prefer not to add test ids; reference TestAccordion, AccordionItem, trigger and header when making the change.src/components/ui/Accordion/fragments/AccordionHeader.tsx (1)
18-21: Consumer-provideddata-orientationon child element will override accordion's value.Based on the
mergePropsimplementation (seesrc/core/utils/mergeProps.ts:49),childPropsare spread last, giving them precedence. UnlikeclassName(which is concatenated) and event handlers (which are composed),data-orientationhas no special handling—so a consumer's value silently wins.This is consistent with the "child wins" philosophy of
mergeProps, but consider whether this is the intended behavior for the accordion's orientation attribute. If the accordion's orientation should always take precedence, you'd need to either:
- Add special handling in
mergePropsfor data attributes, or- Spread the
data-orientationafter{...merged}in thecloneElementcall.If the current behavior (consumer override wins) is acceptable, no change is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Accordion/fragments/AccordionHeader.tsx` around lines 18 - 21, The current mergeProps usage in AccordionHeader (variable merged created by mergeProps({ className: mergedClass, 'data-orientation': orientation }, child.props)) allows a consumer-provided data-orientation in child.props to override the accordion's orientation; to ensure the accordion's orientation always wins, either update mergeProps to special-case data attributes (so mergeProps({ 'data-orientation': ... }, child.props) preserves the first value) or, more locally and simply, change the cloneElement call that uses merged to spread the accordion's data-orientation after the merged props (i.e., apply { ...merged, 'data-orientation': orientation } when calling React.cloneElement) so the AccordionHeader's orientation takes precedence over child.props.eslint.config.mjs (1)
67-71: Use'detect'for React version.Hardcoding
'18.2.0'requires manual updates when React is upgraded. Using'detect'auto-reads the version frompackage.json.♻️ Proposed fix
settings: { react: { - version: '18.2.0', + version: 'detect', }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.mjs` around lines 67 - 71, The React version in the ESLint config is hardcoded (settings.react.version = '18.2.0'); change it to 'detect' so the linter automatically reads the project React version from package.json—update the settings.react.version entry to use the string 'detect' (refer to the settings.react.version key in the config).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.mjs`:
- Line 112: Remove the deprecated eslint rule 'jsx-a11y/accessible-emoji' from
the config: locate the rule entry "'jsx-a11y/accessible-emoji': 'warn'," in
eslint.config.mjs (the rule list) and delete that line so the linter stops
referencing a rule removed in eslint-plugin-jsx-a11y v6.4.0; no replacement is
needed because the rule is deprecated and has no effect with ^6.10.2.
---
Outside diff comments:
In `@package.json`:
- Around line 210-221: The lint-staged config currently runs ESLint twice
because the global pattern "*.{js,jsx,ts,tsx}" overlaps with the
directory-specific pattern
"{src,e2e,scripts,test-utils,.storybook}/**/*.{js,jsx,ts,tsx}"; fix this by
consolidating or excluding: either remove the global pattern and keep only the
directory-specific entry, or keep the global pattern and change it to exclude
those directories (e.g., "!src/**", "!e2e/**", "!scripts/**", "!test-utils/**",
"!.storybook/**") so ESLint only executes once; edit the "lint-staged" block and
adjust the two patterns accordingly.
---
Nitpick comments:
In `@eslint.config.mjs`:
- Around line 67-71: The React version in the ESLint config is hardcoded
(settings.react.version = '18.2.0'); change it to 'detect' so the linter
automatically reads the project React version from package.json—update the
settings.react.version entry to use the string 'detect' (refer to the
settings.react.version key in the config).
In `@src/components/ui/Accordion/fragments/AccordionHeader.tsx`:
- Around line 18-21: The current mergeProps usage in AccordionHeader (variable
merged created by mergeProps({ className: mergedClass, 'data-orientation':
orientation }, child.props)) allows a consumer-provided data-orientation in
child.props to override the accordion's orientation; to ensure the accordion's
orientation always wins, either update mergeProps to special-case data
attributes (so mergeProps({ 'data-orientation': ... }, child.props) preserves
the first value) or, more locally and simply, change the cloneElement call that
uses merged to spread the accordion's data-orientation after the merged props
(i.e., apply { ...merged, 'data-orientation': orientation } when calling
React.cloneElement) so the AccordionHeader's orientation takes precedence over
child.props.
In `@src/components/ui/Accordion/tests/Accordion.a11y.test.tsx`:
- Around line 76-94: The test's DOM traversal is fragile because it uses
trigger.closest('h3') and header?.parentElement to find the item; update the
component and test to use a stable test id instead: add a data-testid (e.g.
data-testid="accordion-item") to the AccordionItem (or TestAccordion's rendered
item) and then replace the fragile traversal in the test with const item =
screen.getByTestId('accordion-item') (keep assertions on
header/trigger/content), or alternatively add a brief comment above the
traversal explaining the required DOM shape if you prefer not to add test ids;
reference TestAccordion, AccordionItem, trigger and header when making the
change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76e7130e-e5ac-4da9-b2ee-08fd0c5fd3e2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.changeset/accordion-orientation-disabled.md.eslintignore.eslintrc.cjs.husky/pre-commit.storybook/preview.jseslint.config.mjspackage.jsonsrc/components/ui/Accordion/contexts/AccordionContext.tsxsrc/components/ui/Accordion/fragments/AccordionContent.tsxsrc/components/ui/Accordion/fragments/AccordionHeader.tsxsrc/components/ui/Accordion/fragments/AccordionItem.tsxsrc/components/ui/Accordion/fragments/AccordionRoot.tsxsrc/components/ui/Accordion/fragments/AccordionTrigger.tsxsrc/components/ui/Accordion/tests/Accordion.a11y.test.tsx
💤 Files with no reviewable changes (3)
- .husky/pre-commit
- .eslintignore
- .eslintrc.cjs
| semi: ['warn', 'always'], | ||
| 'jsx-a11y/alt-text': 'warn', | ||
| 'jsx-a11y/anchor-is-valid': 'warn', | ||
| 'jsx-a11y/accessible-emoji': 'warn', |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When was jsx-a11y/accessible-emoji rule deprecated in eslint-plugin-jsx-a11y?
💡 Result:
The jsx-a11y/accessible-emoji rule was first deprecated in eslint-plugin-jsx-a11y version 6.4.0, released on 2020-10-26. It was marked as deprecated via commit dac6864 ("Deprecate the accessible-emoji rule"), included in v6.4.0. It was fully deprecated (removed from recommended/strict configs and README updated) in PR #782, merged on 2021-04-06.
Citations:
- 1: Fully deprecate accessible-emoji rule jsx-eslint/eslint-plugin-jsx-a11y#782
- 2: CHANGELOG update for v6.4.0 jsx-eslint/eslint-plugin-jsx-a11y#753
- 3: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/CHANGELOG.md
Remove deprecated jsx-a11y/accessible-emoji rule.
This rule was deprecated in eslint-plugin-jsx-a11y v6.4.0 (2020-10-26) since emoji are now well-supported by screen readers. With ^6.10.2 installed, this rule has no effect.
🧹 Proposed fix
'jsx-a11y/alt-text': 'warn',
'jsx-a11y/anchor-is-valid': 'warn',
- 'jsx-a11y/accessible-emoji': 'warn',
'jsx-a11y/anchor-has-content': 'warn',📝 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.
| 'jsx-a11y/accessible-emoji': 'warn', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.mjs` at line 112, Remove the deprecated eslint rule
'jsx-a11y/accessible-emoji' from the config: locate the rule entry
"'jsx-a11y/accessible-emoji': 'warn'," in eslint.config.mjs (the rule list) and
delete that line so the linter stops referencing a rule removed in
eslint-plugin-jsx-a11y v6.4.0; no replacement is needed because the rule is
deprecated and has no effect with ^6.10.2.
1 participant
Summary by CodeRabbit
New Features
disabledprop to disable all accordion items at oncedata-orientationattributes throughout accordion for consistent styling hooksTests
Chores