fix(chip,filter-chip): forward HTML attributes onto root element#769
fix(chip,filter-chip): forward HTML attributes onto root element#769rohilsurana merged 3 commits intomainfrom
Conversation
Chip selectively picked props (`onClick`, `role`, `data-state`, etc.) and did not spread the rest, so consumers couldn't pass `style`, `data-*`, `aria-*`, `id`, mouse events, or any other native span attribute. FilterChip already spread `...props` at runtime but its props interface didn't extend any HTMLAttributes type, so the same attributes were rejected by TypeScript. Both components now extend `ComponentProps<'span' | 'div'>` and forward all unspecified attributes to the root element. Existing prop API is preserved: `ariaLabel` (legacy camelCase) still wins over the native `aria-label`, the `disabled` gate on `onClick` still applies, and `role` still defaults to `status` on Chip. Closes part of #674. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates the Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 53 minutes and 53 seconds.Comment |
|
Closing for now — keeping changes local for further testing. |
…rd HTML attributes
![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 (2)
packages/raystack/components/filter-chip/filter-chip.tsx (1)
160-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
{...props}placed after accessibility-critical attrs — user props silently win.Because
roleandaria-labelare not destructured out ofFilterChipProps, they remain in...props. In JSX, later props override earlier ones, so a consumer who passesrole='region'oraria-label='custom'will silently clobberrole='group'and the computedFilter by ${label}label.data-variantis similarly overridable.
Chipavoids this by spreading{...props}first and setting its controlled attributes after.FilterChipshould follow the same pattern:🛡️ Proposed fix — mirror the Chip spread order
return ( <Flex align='center' ref={ref} - className={chip({ variant, className })} - role='group' - aria-label={`Filter by ${label}`} - data-variant={variant} {...props} + className={chip({ variant, className })} + role='group' + aria-label={`Filter by ${label}`} + data-variant={variant} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/filter-chip/filter-chip.tsx` around lines 160 - 168, The FilterChip component currently spreads {...props} after accessibility attributes so consumer props can overwrite role, aria-label and data-variant; update the JSX in the FilterChip (the Flex element in filter-chip.tsx) to spread {...props} first and then set controlled attributes (role='group', aria-label={`Filter by ${label}`}, data-variant={variant}), or alternatively destructure role and aria-label out of FilterChipProps to prevent them being included in ...props and ensure the component-controlled values (in the FilterChip/Flex render) always override consumer-supplied values.packages/raystack/components/chip/chip.tsx (1)
30-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemoving
ariaLabelis a breaking change for existing consumers.The previous
ChipAPI acceptedariaLabel?: stringas a custom prop. This PR replaces it entirely with the native'aria-label'attribute without providing any backward-compatible alias or deprecation path. A consumer who today writes<Chip ariaLabel="..." />will silently drop the label at runtime (TypeScript will also produce a type error once they upgrade to this version).The playground and docs were updated in this PR, but external package consumers won't know to migrate.
A low-effort, backward-compatible fix is to keep
ariaLabelas a deprecated alias that feeds the same internal variable:🔄 Proposed backward-compatible fix
type ChipProps = ComponentProps<'span'> & VariantProps<typeof chip> & { trailingIcon?: ReactNode; leadingIcon?: ReactNode; isDismissible?: boolean; children: ReactNode; onDismiss?: () => void; disabled?: boolean; + /** `@deprecated` Use the native `aria-label` attribute instead. */ + ariaLabel?: string; };export const Chip = ({ ... - 'aria-label': ariaLabel, + 'aria-label': ariaLabelNative, + ariaLabel: ariaLabelLegacy, ...props }: ChipProps) => { + const ariaLabel = ariaLabelNative ?? ariaLabelLegacy;Alternatively, if this is intentionally a breaking change (semver-major), it should be called out clearly in the changelog/migration notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/chip/chip.tsx` around lines 30 - 54, The PR removed the legacy ariaLabel prop causing a breaking change; restore backward-compatibility by adding ariaLabel?: string to ChipProps and accept ariaLabel in the Chip parameter list alongside the native 'aria-label' destructure, then compute a single value (e.g., const resolvedAriaLabel = ariaLabel ?? ariaLabelNative) and pass that as the element's 'aria-label' attribute; reference the ChipProps type, the Chip component function, the existing 'aria-label' destructured identifier (currently named ariaLabel in the code), and add a short deprecation comment on ariaLabel to indicate it will be removed in a future major release.
🤖 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/raystack/components/chip/__tests__/chip.test.tsx`:
- Around line 231-236: The test "accepts native aria-label attribute" is
redundant because it duplicates the assertion path of the existing "uses custom
aria-label when provided" test; remove the entire it block named "accepts native
aria-label attribute" from the chip.test.tsx file (the render(<Chip
aria-label='...'>...); + expect(...toHaveAttribute('aria-label', ...)) block)
and keep the distinct "uses custom aria-label when provided" and the override
test that checks precedence (both referenced in the same test suite).
---
Outside diff comments:
In `@packages/raystack/components/chip/chip.tsx`:
- Around line 30-54: The PR removed the legacy ariaLabel prop causing a breaking
change; restore backward-compatibility by adding ariaLabel?: string to ChipProps
and accept ariaLabel in the Chip parameter list alongside the native
'aria-label' destructure, then compute a single value (e.g., const
resolvedAriaLabel = ariaLabel ?? ariaLabelNative) and pass that as the element's
'aria-label' attribute; reference the ChipProps type, the Chip component
function, the existing 'aria-label' destructured identifier (currently named
ariaLabel in the code), and add a short deprecation comment on ariaLabel to
indicate it will be removed in a future major release.
In `@packages/raystack/components/filter-chip/filter-chip.tsx`:
- Around line 160-168: The FilterChip component currently spreads {...props}
after accessibility attributes so consumer props can overwrite role, aria-label
and data-variant; update the JSX in the FilterChip (the Flex element in
filter-chip.tsx) to spread {...props} first and then set controlled attributes
(role='group', aria-label={`Filter by ${label}`}, data-variant={variant}), or
alternatively destructure role and aria-label out of FilterChipProps to prevent
them being included in ...props and ensure the component-controlled values (in
the FilterChip/Flex render) always override consumer-supplied values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71336f27-2570-475b-8003-d1b54be89d53
📒 Files selected for processing (7)
apps/www/src/components/playground/chip-examples.tsxapps/www/src/content/docs/components/chip/demo.tsapps/www/src/content/docs/components/chip/props.tspackages/raystack/components/chip/__tests__/chip.test.tsxpackages/raystack/components/chip/chip.tsxpackages/raystack/components/filter-chip/__tests__/filter-chip.test.tsxpackages/raystack/components/filter-chip/filter-chip.tsx
| it('accepts native aria-label attribute', () => { | ||
| render(<Chip aria-label='Native Label'>String Child</Chip>); | ||
|
|
||
| const chip = screen.getByRole('status'); | ||
| expect(chip).toHaveAttribute('aria-label', 'Native Label'); | ||
| }); |
There was a problem hiding this comment.
Redundant test — identical assertion path to the test at Line 217.
"accepts native aria-label attribute" (lines 231-236) and "uses custom aria-label when provided" (lines 217-222) both render <Chip aria-label='...' /> and assert toHaveAttribute('aria-label', ...). They exercise the exact same code path; the only variation is the label/children string.
The "override" test at lines 224-229 is distinct (string child + explicit aria-label — checks precedence) and should be kept.
🗑️ Proposed removal
- it('accepts native aria-label attribute', () => {
- render(<Chip aria-label='Native Label'>String Child</Chip>);
-
- const chip = screen.getByRole('status');
- expect(chip).toHaveAttribute('aria-label', 'Native Label');
- });📝 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.
| it('accepts native aria-label attribute', () => { | |
| render(<Chip aria-label='Native Label'>String Child</Chip>); | |
| const chip = screen.getByRole('status'); | |
| expect(chip).toHaveAttribute('aria-label', 'Native Label'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/chip/__tests__/chip.test.tsx` around lines 231 -
236, The test "accepts native aria-label attribute" is redundant because it
duplicates the assertion path of the existing "uses custom aria-label when
provided" test; remove the entire it block named "accepts native aria-label
attribute" from the chip.test.tsx file (the render(<Chip aria-label='...'>...);
+ expect(...toHaveAttribute('aria-label', ...)) block) and keep the distinct
"uses custom aria-label when provided" and the override test that checks
precedence (both referenced in the same test suite).
3 participants
Summary
Closes the remaining gap from #674 — covers Chip (#605) and FilterChip (#616). The other components listed in #674 already extend the appropriate
HTMLAttributestype and spread...rest(verified againstmainata7e6705).onClick,role,data-state,disabled,ariaLabeland dropped everything else, so consumers couldn't passstyle,id,data-*,aria-*, mouse events, etc....propsonto the root<Flex>at runtime, butFilterChipPropsdidn't extend any HTMLAttributes type — so TypeScript rejected those props at the call site.Both now extend
ComponentProps<'span'>/ComponentProps<'div'>and the Chip implementation spreads the rest props onto the root element. Existing prop API is preserved:ariaLabel(legacy camelCase) still wins over the nativearia-labeldisabledstill gatesonClickand setsdata-disabledrolestill defaults to'status'on Chipdata-stateis still passed through (now via the spread instead of an explicit prop)Components NOT changed (already compliant)
ComponentProps<'div'>and spreadsImgHTMLAttributesComponentProps<'button'>and spreadsuseRender.ComponentProps<'h2'>+mergePropsComponentProps<typeof Flex>IconButtonPropsComponentProps<'span'>ComponentProps<'span'>If maintainers agree, #674 can be closed once this lands.
Test plan
pnpm test— full suite passes (1684 tests; +4 new covering arbitrary attribute forwarding on Chip and FilterChip)pnpm build— typecheck cleanariaLabel,role,disabled, dismiss, and click behavior covered by existing Chip tests still passstyle,data-*,id,onMouseEnter, etc.🤖 Generated with Claude Code