fix: clear values array on additional input type change#248
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughThe PR adds type-change handling to the AdditionalInput form component. When a field type switches from an options-capable type (e.g., CheckBoxList) to a non-options type, the component now clears the associated values array. Tests verify both the clearing behavior and value preservation when switching between options-based types. ChangesType-change handler and validation
🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/mui/__tests__/additional-input.test.js (1)
219-305: 💤 Low valueReduce duplication between the two
handleTypeChangetests.The two test cases share an identical
TestWrapper,itemWithValues, and render setup; only the option clicked and expected count differ. Consider hoistingTestWrapperand a small helper (e.g.,renderWithValuesCount(initialItem)) to thedescribescope to cut ~40 lines and make the behavioral diff between cases obvious.🤖 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/mui/__tests__/additional-input.test.js` around lines 219 - 305, Hoist the duplicated setup inside the describe("handleTypeChange") by extracting the repeated itemWithValues object, the TestWrapper component (which uses useFormikContext and renders AdditionalInput plus the values-count div), and the Formik render logic into shared helpers—e.g., define const itemWithValues = ... once, create a renderWithValuesCount(initialItem) helper that mounts <Formik><Form><TestWrapper/></Form></Formik>, and reuse those in each test; keep each test only performing the specific userEvent clicks and final expect (referencing TestWrapper, itemWithValues, renderWithValuesCount, and AdditionalInput to locate the code).
🤖 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.
Nitpick comments:
In `@src/components/mui/__tests__/additional-input.test.js`:
- Around line 219-305: Hoist the duplicated setup inside the
describe("handleTypeChange") by extracting the repeated itemWithValues object,
the TestWrapper component (which uses useFormikContext and renders
AdditionalInput plus the values-count div), and the Formik render logic into
shared helpers—e.g., define const itemWithValues = ... once, create a
renderWithValuesCount(initialItem) helper that mounts
<Formik><Form><TestWrapper/></Form></Formik>, and reuse those in each test; keep
each test only performing the specific userEvent clicks and final expect
(referencing TestWrapper, itemWithValues, renderWithValuesCount, and
AdditionalInput to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 932d88da-54fb-43ec-98b9-f1aedd2920ee
📒 Files selected for processing (2)
src/components/mui/__tests__/additional-input.test.jssrc/components/mui/formik-inputs/additional-input/additional-input.js
ref: https://app.clickup.com/t/86ba1rkqu
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests