test(CheckboxGroup): controlled/uncontrolled value switch (#1825)#2081
test(CheckboxGroup): controlled/uncontrolled value switch (#1825)#2081kotAPI wants to merge 1 commit into
Conversation
|
📝 WalkthroughWalkthroughA new test file is added for ChangesCheckboxGroup Controlled Switch Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/components/ui/CheckboxGroup/tests/CheckboxGroup.controlledSwitch.test.tsx`:
- Around line 46-60: The controlled-to-uncontrolled test currently remounts a
new tree with unmount() and render(), so it does not verify switching the same
CheckboxGroup instance from value to defaultValue. Update the test in
CheckboxGroup.controlledSwitch.test.tsx to use rerender() on the existing render
result when moving from the controlled props to the uncontrolled props, and keep
the assertions against CheckboxGroup behavior (and the group helper) aligned
with that same-instance transition; if you intend to keep remounting, rename the
test to reflect that instead.
🪄 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: 5f63702a-b43f-4a73-a948-09dfe83ffb51
📒 Files selected for processing (1)
src/components/ui/CheckboxGroup/tests/CheckboxGroup.controlledSwitch.test.tsx
| test('switches from controlled value to uncontrolled defaultValue', async() => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| const { unmount } = render(group({ value: ['banana'], onValueChange: () => {} })); | ||
|
|
||
| expect(screen.getByRole('checkbox', { name: 'Banana' })).toHaveAttribute('aria-checked', 'true'); | ||
| unmount(); | ||
|
|
||
| render(group({ defaultValue: ['apple'] })); | ||
|
|
||
| expect(screen.getByRole('checkbox', { name: 'Apple' })).toHaveAttribute('aria-checked', 'true'); | ||
|
|
||
| await user.click(screen.getByRole('checkbox', { name: 'Banana' })); | ||
| expect(screen.getByRole('checkbox', { name: 'Banana' })).toHaveAttribute('aria-checked', 'true'); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how CheckboxGroup.Root handles controlled vs uncontrolled value, and any switch handling/warnings.
fd -t f 'CheckboxGroup' src/components/ui/CheckboxGroup --exec echo {}
ast-grep outline src/components/ui/CheckboxGroup/CheckboxGroup.tsx 2>/dev/null
rg -nP -C3 '(defaultValue|value|onValueChange|isControlled|useControll)' src/components/ui/CheckboxGroupRepository: rad-ui/ui
Length of output: 21808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the component implementation and the exact test file.
ast-grep outline src/components/ui/CheckboxGroup/fragments/CheckboxGroupRoot.tsx
printf '\n--- CheckboxGroupRoot.tsx ---\n'
sed -n '1,220p' src/components/ui/CheckboxGroup/fragments/CheckboxGroupRoot.tsx
printf '\n--- CheckboxGroup.controlledSwitch.test.tsx ---\n'
sed -n '1,220p' src/components/ui/CheckboxGroup/tests/CheckboxGroup.controlledSwitch.test.tsx
# Search for any explicit controlled/uncontrolled switch handling or docs in the repo.
printf '\n--- switch-related search ---\n'
rg -n "controlled.*uncontrolled|uncontrolled.*controlled|defaultValue|value=.*defaultValue|switches from controlled|switches from uncontrolled" src/components/ui/CheckboxGroup src | sed -n '1,220p'Repository: rad-ui/ui
Length of output: 32419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the component implementation and the exact test file.
ast-grep outline src/components/ui/CheckboxGroup/fragments/CheckboxGroupRoot.tsx
printf '\n--- CheckboxGroupRoot.tsx ---\n'
sed -n '1,220p' src/components/ui/CheckboxGroup/fragments/CheckboxGroupRoot.tsx
printf '\n--- CheckboxGroup.controlledSwitch.test.tsx ---\n'
sed -n '1,220p' src/components/ui/CheckboxGroup/tests/CheckboxGroup.controlledSwitch.test.tsx
# Search for any explicit controlled/uncontrolled switch handling or docs in the repo.
printf '\n--- switch-related search ---\n'
rg -n "controlled.*uncontrolled|uncontrolled.*controlled|defaultValue|value=.*defaultValue|switches from controlled|switches from uncontrolled" src/components/ui/CheckboxGroup src | sed -n '1,220p'Repository: rad-ui/ui
Length of output: 32467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/components/ui/CheckboxGroup/fragments/CheckboxGroupRoot.tsxRepository: rad-ui/ui
Length of output: 1911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- useControllableState ---\n'
sed -n '1,220p' src/core/hooks/useControllableState/index.tsx
printf '\n--- CheckboxGroupPrimitiveRoot ---\n'
sed -n '1,220p' src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveRoot.tsx
printf '\n--- CheckboxGroupPrimitive tests around switching ---\n'
sed -n '1,240p' src/core/primitives/CheckboxGroup/tests/CheckboxGroupPrimitive.test.tsxRepository: rad-ui/ui
Length of output: 14950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- useControllableState ---\n'
sed -n '1,220p' src/core/hooks/useControllableState/index.tsx
printf '\n--- CheckboxGroupPrimitiveRoot ---\n'
sed -n '1,220p' src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveRoot.tsx
printf '\n--- CheckboxGroupPrimitive tests around switching ---\n'
sed -n '1,240p' src/core/primitives/CheckboxGroup/tests/CheckboxGroupPrimitive.test.tsxRepository: rad-ui/ui
Length of output: 14950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- useControllableState ---\n'
sed -n '1,220p' src/core/hooks/useControllableState/index.tsx
printf '\n--- CheckboxGroupPrimitiveRoot ---\n'
sed -n '1,220p' src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveRoot.tsx
printf '\n--- CheckboxGroupPrimitive tests around switching ---\n'
sed -n '1,240p' src/core/primitives/CheckboxGroup/tests/CheckboxGroupPrimitive.test.tsxRepository: rad-ui/ui
Length of output: 14950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- useControllableState ---\n'
sed -n '1,220p' src/core/hooks/useControllableState/index.tsx
printf '\n--- CheckboxGroupPrimitiveRoot ---\n'
sed -n '1,220p' src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveRoot.tsx
printf '\n--- CheckboxGroupPrimitive tests around switching ---\n'
sed -n '1,240p' src/core/primitives/CheckboxGroup/tests/CheckboxGroupPrimitive.test.tsxRepository: rad-ui/ui
Length of output: 14950
Use rerender for the controlled→uncontrolled case. unmount() + render() mounts a fresh uncontrolled tree, so this doesn’t cover a same-instance switch and misses the behavior this test name implies. If remounting is intentional, rename the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/ui/CheckboxGroup/tests/CheckboxGroup.controlledSwitch.test.tsx`
around lines 46 - 60, The controlled-to-uncontrolled test currently remounts a
new tree with unmount() and render(), so it does not verify switching the same
CheckboxGroup instance from value to defaultValue. Update the test in
CheckboxGroup.controlledSwitch.test.tsx to use rerender() on the existing render
result when moving from the controlled props to the uncontrolled props, and keep
the assertions against CheckboxGroup behavior (and the group helper) aligned
with that same-instance transition; if you intend to keep remounting, rename the
test to reflect that instead.
Code reviewLGTM. Matches project patterns for portal Theme refs, Floating UI prop merge, controlled-switch/lazy-mount/RTL tests, or focused bug fixes. No changes requested. |
1 participant
Summary
add controlled/uncontrolled value switch regression tests
Test plan
Related to #1825
Summary by CodeRabbit
CheckboxGroupswitching between uncontrolled and controlled states.