Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a new Formik-bound select component (MuiFormikSelectV2), updates ItemTableField to use it, re-exports FormItemTable helpers and subcomponents, refines label/size/placeholder handling across several form inputs, introduces small layout tweaks, adds Jest mocks for the new select, and adjusts package version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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)
src/components/mui/formik-inputs/mui-formik-timepicker.js (1)
40-45:⚠️ Potential issue | 🔴 CriticalBug:
label={timeZone}onTimePickeroverrides the newlabelprop.Line 40 passes
timeZoneas theTimePicker's top-levellabel, which takes precedence overslotProps.textField.labelset on line 45. As a result, the newlabelprop introduced by this PR will not be rendered — users will see the timezone string as the field label instead. This looks like a pre-existing copy/paste bug that this PR now makes observable.🐛 Proposed fix
timezone={timeZone} - label={timeZone} views={["hours", "minutes"]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-timepicker.js` around lines 40 - 45, The TimePicker currently sets a top-level label using label={timeZone}, which overrides the intended field label passed via the new label prop; update the TimePicker usage in mui-formik-timepicker.js (the TimePicker component instance) to remove or replace label={timeZone} so the field label comes from slotProps.textField.label (the variable name `label`) — for example, delete the top-level label prop or change it to label={label} and render `timeZone` elsewhere (e.g., in slotProps.textField.InputProps.endAdornment or helperText) so `slotProps.textField.label` is not ignored.
🧹 Nitpick comments (4)
src/components/mui/formik-inputs/mui-formik-timepicker.js (1)
67-69: Declarelabel(and other accepted props) inpropTypes.The component now destructures
label,minTime,maxTime,timeZone, anddisabled, butpropTypesonly listsname. For consistency withMuiFormikDatepicker(seesrc/components/mui/formik-inputs/mui-formik-datepicker.jslines 68-72) and to surface misuse at dev time, at minimum addlabel.♻️ Proposed change
MuiFormikTimepicker.propTypes = { - name: PropTypes.string.isRequired + name: PropTypes.string.isRequired, + label: PropTypes.string, + minTime: PropTypes.any, + maxTime: PropTypes.any, + timeZone: PropTypes.string, + disabled: PropTypes.bool };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-timepicker.js` around lines 67 - 69, Add the missing propTypes for MuiFormikTimepicker: update the MuiFormikTimepicker.propTypes declaration to include label (string), and also declare the other accepted props that are destructured (minTime, maxTime, timeZone, disabled) with appropriate PropTypes (e.g., string or instanceOf(Date) for times as used in MuiFormikDatepicker, and bool for disabled) so that MuiFormikTimepicker, like MuiFormikDatepicker, surfaces misuse at dev time and matches the component's destructured props.src/components/mui/formik-inputs/mui-formik-select-v2.js (3)
21-29: Spread{...rest}before{...field}to protect Formik bindings.With the current order, any
value,onChange,onBlur, ornamepassed by callers via...restwill silently override the Formik-provided handlers fromuseField, decoupling the input from form state. Putting{...rest}first ensures Formik always wins.♻️ Proposed change
<Select name={name} label={label} labelId={`${name}-label`} displayEmpty // eslint-disable-next-line react/jsx-props-no-spreading - {...field} - // eslint-disable-next-line react/jsx-props-no-spreading {...rest} + // eslint-disable-next-line react/jsx-props-no-spreading + {...field}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-select-v2.js` around lines 21 - 29, The Select component spreads {...rest} after {...field} which allows caller props in rest to override Formik bindings (value/onChange/onBlur/name); change the prop order so that {...rest} is spread before {...field} (i.e., render <Select name={name} label={label} labelId={`${name}-label`} displayEmpty {...rest} {...field} />) to ensure the useField-provided handlers and value always take precedence over caller-supplied props.
53-56: Incomplete propTypes.
labelandplaceholderare used by the component but not declared inpropTypes. Consider adding them for consistency and lint/DX benefits.MuiFormikSelectV2.propTypes = { name: PropTypes.string.isRequired, - options: PropTypes.array.isRequired + options: PropTypes.array.isRequired, + label: PropTypes.string, + placeholder: PropTypes.string };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-select-v2.js` around lines 53 - 56, MuiFormikSelectV2.propTypes is missing declarations for props used by the component; add PropTypes entries for label and placeholder to match usage in MuiFormikSelectV2 (e.g., declare label: PropTypes.string and placeholder: PropTypes.string) alongside the existing name and options to satisfy linting and improve DX.
30-38: Placeholder also shows for falsy non-empty values.
if (!selected)treats0and other falsy values as "no selection". Ifoptionsever includes a numeric0value (or boolean), the placeholder will render over a legitimate selection. Consider an explicit check:♻️ Proposed change
- renderValue={(selected) => { - if (!selected) { + renderValue={(selected) => { + if (selected === "" || selected === undefined || selected === null) { return <em>{finalPlaceholder}</em>; } const selectedOption = options.find( ({ value }) => value === selected ); return selectedOption ? selectedOption.label : ""; }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-select-v2.js` around lines 30 - 38, In the renderValue function of the MUI Formik select component (renderValue in mui-formik-select-v2.js) replace the falsy check `if (!selected)` with an explicit null/undefined check (e.g., `if (selected === null || selected === undefined)` or `if (selected == null)`) so numeric 0 or boolean false selections are treated as valid; keep the subsequent lookup using `options.find(({ value }) => value === selected)` and return `selectedOption ? selectedOption.label : ""` as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/FormItemTable/components/ItemTableField.js`:
- Around line 75-79: The InputLabel in the MuiFormikSelectV2 component is
missing the shrink prop which causes the floating label to overlap when
displayEmpty is used; open the MuiFormikSelectV2 implementation (symbol:
MuiFormikSelectV2) and add shrink to the label handling — either set
InputLabelProps={{ shrink: true }} on the underlying MUI Select/SelectField or
pass shrink directly to the InputLabel element (symbol: InputLabel) so that
displayEmpty remains enabled but the label is forced to shrink; update any props
forwarding (e.g., InputLabelProps) so callers like ItemTableField using
<MuiFormikSelectV2 ... /> inherit this behavior.
---
Outside diff comments:
In `@src/components/mui/formik-inputs/mui-formik-timepicker.js`:
- Around line 40-45: The TimePicker currently sets a top-level label using
label={timeZone}, which overrides the intended field label passed via the new
label prop; update the TimePicker usage in mui-formik-timepicker.js (the
TimePicker component instance) to remove or replace label={timeZone} so the
field label comes from slotProps.textField.label (the variable name `label`) —
for example, delete the top-level label prop or change it to label={label} and
render `timeZone` elsewhere (e.g., in
slotProps.textField.InputProps.endAdornment or helperText) so
`slotProps.textField.label` is not ignored.
---
Nitpick comments:
In `@src/components/mui/formik-inputs/mui-formik-select-v2.js`:
- Around line 21-29: The Select component spreads {...rest} after {...field}
which allows caller props in rest to override Formik bindings
(value/onChange/onBlur/name); change the prop order so that {...rest} is spread
before {...field} (i.e., render <Select name={name} label={label}
labelId={`${name}-label`} displayEmpty {...rest} {...field} />) to ensure the
useField-provided handlers and value always take precedence over caller-supplied
props.
- Around line 53-56: MuiFormikSelectV2.propTypes is missing declarations for
props used by the component; add PropTypes entries for label and placeholder to
match usage in MuiFormikSelectV2 (e.g., declare label: PropTypes.string and
placeholder: PropTypes.string) alongside the existing name and options to
satisfy linting and improve DX.
- Around line 30-38: In the renderValue function of the MUI Formik select
component (renderValue in mui-formik-select-v2.js) replace the falsy check `if
(!selected)` with an explicit null/undefined check (e.g., `if (selected === null
|| selected === undefined)` or `if (selected == null)`) so numeric 0 or boolean
false selections are treated as valid; keep the subsequent lookup using
`options.find(({ value }) => value === selected)` and return `selectedOption ?
selectedOption.label : ""` as before.
In `@src/components/mui/formik-inputs/mui-formik-timepicker.js`:
- Around line 67-69: Add the missing propTypes for MuiFormikTimepicker: update
the MuiFormikTimepicker.propTypes declaration to include label (string), and
also declare the other accepted props that are destructured (minTime, maxTime,
timeZone, disabled) with appropriate PropTypes (e.g., string or instanceOf(Date)
for times as used in MuiFormikDatepicker, and bool for disabled) so that
MuiFormikTimepicker, like MuiFormikDatepicker, surfaces misuse at dev time and
matches the component's destructured props.
🪄 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: e7ee8513-ebc6-409a-80b6-0fb17861fb7b
📒 Files selected for processing (12)
package.jsonsrc/components/index.jssrc/components/mui/FormItemTable/__tests__/FormItemTable.test.jssrc/components/mui/FormItemTable/__tests__/ItemTableField.test.jssrc/components/mui/FormItemTable/components/ItemTableField.jssrc/components/mui/FormItemTable/index.jssrc/components/mui/ItemSettingsModal/index.jssrc/components/mui/formik-inputs/mui-formik-datepicker.jssrc/components/mui/formik-inputs/mui-formik-dropdown-checkbox.jssrc/components/mui/formik-inputs/mui-formik-dropdown-radio.jssrc/components/mui/formik-inputs/mui-formik-select-v2.jssrc/components/mui/formik-inputs/mui-formik-timepicker.js
| <MuiFormikSelectV2 | ||
| {...commonProps} | ||
| size="small" | ||
| options={field.values.map((v) => ({ value: v.id, label: v.value }))} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For MUI v6 Select with displayEmpty and InputLabel, does the label need to be forced shrink to avoid overlap with the empty placeholder?
💡 Result:
Yes, in MUI v6, when using Select with displayEmpty={true} and an InputLabel, the label needs to be forced to shrink (via shrink={true} on InputLabel) to avoid overlapping with the empty placeholder value. The official Select API documentation explicitly warns: "When using this prop, make sure the label doesn't overlap with the empty displayed value. The label should either be hidden or forced to a shrunk state." For outlined variant, you may also need notched={true} on the Select to properly notch the border.
Citations:
- 1: https://mui.com/material-ui/api/select/
- 2: https://v6.mui.com/material-ui/api/select
- 3: [material-ui][Select]
displayEmpty=truerenders label over value mui/material-ui#42148
🏁 Script executed:
# First, find and read the ItemTableField.js file around lines 75-79
fd -t f "ItemTableField.js" | head -5Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 145
🏁 Script executed:
# Also search for the mui-formik-select-v2.js file
fd -t f "mui-formik-select-v2.js" | head -5Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 140
🏁 Script executed:
# Check for MUI version in package.json
fd -t f "package.json" -x grep -l "\"@mui" {} \;Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 98
🏁 Script executed:
# Read ItemTableField.js around lines 75-79
cat -n src/components/mui/FormItemTable/components/ItemTableField.js | sed -n '70,85p'Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 598
🏁 Script executed:
# Read the entire mui-formik-select-v2.js file
wc -l src/components/mui/formik-inputs/mui-formik-select-v2.jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 143
🏁 Script executed:
# Check MUI version in package.json
grep -A 2 "@mui/material" package.json | head -10Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 300
🏁 Script executed:
# Read the full mui-formik-select-v2.js file
cat -n src/components/mui/formik-inputs/mui-formik-select-v2.jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 2129
🏁 Script executed:
# Search for other occurrences of shrink in dropdown/select related files
rg -i "shrink" src/components/mui/formik-inputs/ -A 2 -B 2Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 4092
🏁 Script executed:
# Search for displayEmpty usage
rg "displayEmpty" src/components/mui/Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 696
Add shrink prop to InputLabel in MuiFormikSelectV2 to prevent label/placeholder overlap.
The component uses displayEmpty (line 25 of mui-formik-select-v2.js) but the InputLabel lacks the shrink prop, which can cause the floating label to overlap the empty placeholder. Other dropdown components in this codebase (mui-formik-select.js, mui-formik-dropdown-radio.js, mui-formik-dropdown-checkbox.js) already use shrink, and MUI v6 documentation explicitly requires this to prevent overlap when displayEmpty is set.
Fix in src/components/mui/formik-inputs/mui-formik-select-v2.js
- {label && <InputLabel id={`${name}-label`}>{label}</InputLabel>}
+ {label && <InputLabel shrink id={`${name}-label`}>{label}</InputLabel>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/FormItemTable/components/ItemTableField.js` around lines
75 - 79, The InputLabel in the MuiFormikSelectV2 component is missing the shrink
prop which causes the floating label to overlap when displayEmpty is used; open
the MuiFormikSelectV2 implementation (symbol: MuiFormikSelectV2) and add shrink
to the label handling — either set InputLabelProps={{ shrink: true }} on the
underlying MUI Select/SelectField or pass shrink directly to the InputLabel
element (symbol: InputLabel) so that displayEmpty remains enabled but the label
is forced to shrink; update any props forwarding (e.g., InputLabelProps) so
callers like ItemTableField using <MuiFormikSelectV2 ... /> inherit this
behavior.
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...rest} | ||
| renderValue={(selected) => { | ||
| if (!selected) { |
There was a problem hiding this comment.
@santipalenque breaks for option value 0:
The component is used in ItemTableField.js for the "ComboBox" case:
options={field.values.map((v) => ({ value: v.id, label: v.value }))}v.id comes from server-side form field values. If any field value has id: 0, the user selects it, Formik stores 0, then renderValue receives 0, evaluates !0 as true, and renders the placeholder
instead of the option label. The selection is silently swallowed visually , the value IS stored in Formik state, but the user sees a placeholder as if nothing was selected.
The fix is to replace the falsy check with an explicit check for "no meaningful selection":
if (selected == null || selected === "") {This correctly treats 0, false, or any other legitimate option value as a real selection, while still showing the placeholder for null, undefined, and empty string.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@santipalenque please review
santipalenque
force-pushed
the
feature/mui-components-improvements
branch
from
3fbad38 to
dd663d8
Compare
|
done @smarcet |
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)
src/components/mui/formik-inputs/mui-formik-timepicker.js (1)
40-40:⚠️ Potential issue | 🟠 MajorRemove stale
label={timeZone}on the TimePicker.The outer
TimePickerstill setslabel={timeZone}, which appears to be a leftover bug now that a properlabelprop is wired throughslotProps.textField.label(Line 45). PassingtimeZoneas a label will either conflict with or override the intended label on the underlying text field, depending on how MUI merges slotProps.🐛 Proposed fix
timezone={timeZone} - label={timeZone} views={["hours", "minutes"]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-timepicker.js` at line 40, The TimePicker component still passes label={timeZone}, which conflicts with the proper label provided via slotProps.textField.label; remove the stale label prop from the TimePicker JSX (the label={timeZone} instance) so the component uses the wired slotProps.textField.label instead and avoid overriding the intended label; search for the TimePicker usage in mui-formik-timepicker.js and delete the label prop referencing timeZone.
🧹 Nitpick comments (3)
src/components/mui/formik-inputs/mui-formik-select-v2.js (2)
21-45: Spread order can clobber Formik wiring.
{...rest}is spread after{...field}, so anyvalue/onChange/onBlurpassed by a consumer will silently override Formik's bindings. If that's not intentional, spreadrestbeforefieldso Formik props win.Proposed fix
- // eslint-disable-next-line react/jsx-props-no-spreading - {...field} - // eslint-disable-next-line react/jsx-props-no-spreading - {...rest} + // eslint-disable-next-line react/jsx-props-no-spreading + {...rest} + // eslint-disable-next-line react/jsx-props-no-spreading + {...field}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-select-v2.js` around lines 21 - 45, The Select component currently spreads {...field} then {...rest}, which allows consumer props (value/onChange/onBlur) in rest to override Formik's bindings; change the spread order so {...rest} is applied before {...field} (i.e., spread rest first, then field) in the Select element (refer to the Select JSX where {...field} and {...rest} are used) to ensure Formik's value/onChange/onBlur from field take precedence.
53-56: ExpandpropTypes.
labelandplaceholderare part of the public API but not declared.optionscould also specify item shape.Proposed fix
MuiFormikSelectV2.propTypes = { name: PropTypes.string.isRequired, - options: PropTypes.array.isRequired + label: PropTypes.string, + placeholder: PropTypes.string, + options: PropTypes.arrayOf( + PropTypes.shape({ + value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, + label: PropTypes.node.isRequired + }) + ).isRequired };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-select-v2.js` around lines 53 - 56, Update MuiFormikSelectV2.propTypes to declare the full public API: add PropTypes.string for label and placeholder, and refine options to PropTypes.arrayOf(PropTypes.shape({ value: PropTypes.any.isRequired, label: PropTypes.string.isRequired })). Ensure you reference MuiFormikSelectV2.propTypes and keep name as PropTypes.string.isRequired; keep existing options required if intended or change to optional accordingly.src/components/mui/formik-inputs/mui-formik-timepicker.js (1)
67-69: AddlabeltopropTypes.The new
labelprop is not declared inpropTypes.Proposed fix
MuiFormikTimepicker.propTypes = { - name: PropTypes.string.isRequired + name: PropTypes.string.isRequired, + label: PropTypes.string };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/formik-inputs/mui-formik-timepicker.js` around lines 67 - 69, The component MuiFormikTimepicker is missing a propTypes declaration for the new label prop; update MuiFormikTimepicker.propTypes to include label: PropTypes.string (or PropTypes.node if labels can be JSX) and mark it as required only if the component expects it (e.g., label: PropTypes.string.isRequired) so the label prop is validated like name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/FormItemTable/__tests__/ItemTableField.test.js`:
- Around line 83-89: The mock for "mui-formik-select-v2" is using the same
data-testid as the other select mock, causing ambiguous queries; update the
jest.mock for mui-formik-select-v2 (the default mock component defined in the
jest.mock call for "mui-formik-select-v2") to render a unique attribute such as
data-testid="select-v2" (and keep data-name the same), then update the ComboBox
test assertion/title that expects the v2 select (the test referenced as the
ComboBox test) to query for "select-v2" (and rename the test title if it refers
to v2) so queries like getByTestId/getAllByTestId unambiguously target the v2
mock.
---
Outside diff comments:
In `@src/components/mui/formik-inputs/mui-formik-timepicker.js`:
- Line 40: The TimePicker component still passes label={timeZone}, which
conflicts with the proper label provided via slotProps.textField.label; remove
the stale label prop from the TimePicker JSX (the label={timeZone} instance) so
the component uses the wired slotProps.textField.label instead and avoid
overriding the intended label; search for the TimePicker usage in
mui-formik-timepicker.js and delete the label prop referencing timeZone.
---
Nitpick comments:
In `@src/components/mui/formik-inputs/mui-formik-select-v2.js`:
- Around line 21-45: The Select component currently spreads {...field} then
{...rest}, which allows consumer props (value/onChange/onBlur) in rest to
override Formik's bindings; change the spread order so {...rest} is applied
before {...field} (i.e., spread rest first, then field) in the Select element
(refer to the Select JSX where {...field} and {...rest} are used) to ensure
Formik's value/onChange/onBlur from field take precedence.
- Around line 53-56: Update MuiFormikSelectV2.propTypes to declare the full
public API: add PropTypes.string for label and placeholder, and refine options
to PropTypes.arrayOf(PropTypes.shape({ value: PropTypes.any.isRequired, label:
PropTypes.string.isRequired })). Ensure you reference
MuiFormikSelectV2.propTypes and keep name as PropTypes.string.isRequired; keep
existing options required if intended or change to optional accordingly.
In `@src/components/mui/formik-inputs/mui-formik-timepicker.js`:
- Around line 67-69: The component MuiFormikTimepicker is missing a propTypes
declaration for the new label prop; update MuiFormikTimepicker.propTypes to
include label: PropTypes.string (or PropTypes.node if labels can be JSX) and
mark it as required only if the component expects it (e.g., label:
PropTypes.string.isRequired) so the label prop is validated like name.
🪄 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: 0064f71e-47f3-48c2-9c10-a747f7a186bd
📒 Files selected for processing (12)
package.jsonsrc/components/index.jssrc/components/mui/FormItemTable/__tests__/FormItemTable.test.jssrc/components/mui/FormItemTable/__tests__/ItemTableField.test.jssrc/components/mui/FormItemTable/components/ItemTableField.jssrc/components/mui/FormItemTable/index.jssrc/components/mui/ItemSettingsModal/index.jssrc/components/mui/formik-inputs/mui-formik-datepicker.jssrc/components/mui/formik-inputs/mui-formik-dropdown-checkbox.jssrc/components/mui/formik-inputs/mui-formik-dropdown-radio.jssrc/components/mui/formik-inputs/mui-formik-select-v2.jssrc/components/mui/formik-inputs/mui-formik-timepicker.js
✅ Files skipped from review due to trivial changes (4)
- package.json
- src/components/mui/FormItemTable/tests/FormItemTable.test.js
- src/components/mui/formik-inputs/mui-formik-datepicker.js
- src/components/index.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/mui/ItemSettingsModal/index.js
- src/components/mui/FormItemTable/components/ItemTableField.js
- src/components/mui/FormItemTable/index.js
- src/components/mui/formik-inputs/mui-formik-dropdown-checkbox.js
- src/components/mui/formik-inputs/mui-formik-dropdown-radio.js
| jest.mock("../../formik-inputs/mui-formik-select-v2", () => { | ||
| const React = require("react"); | ||
| return { | ||
| __esModule: true, | ||
| default: ({ name }) => <div data-testid="select" data-name={name} /> | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Duplicate data-testid="select" collides with the existing mock.
Both mui-formik-select (Line 76) and mui-formik-select-v2 (Line 87) now render data-testid="select". Any test that renders both (or queries with getAllByTestId) becomes ambiguous, and the ComboBox test at Line 138 no longer clearly targets v2. Use a distinct id such as "select-v2" and update the corresponding assertion/title.
Proposed fix
jest.mock("../../formik-inputs/mui-formik-select-v2", () => {
const React = require("react");
return {
__esModule: true,
- default: ({ name }) => <div data-testid="select" data-name={name} />
+ default: ({ name }) => <div data-testid="select-v2" data-name={name} />
};
});And update the ComboBox test:
- test("renders MuiFormikSelect for ComboBox type", () => {
+ test("renders MuiFormikSelectV2 for ComboBox type", () => {
renderField({ type: "ComboBox" });
- expect(screen.getByTestId("select")).toBeInTheDocument();
+ expect(screen.getByTestId("select-v2")).toBeInTheDocument();
});📝 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.
| jest.mock("../../formik-inputs/mui-formik-select-v2", () => { | |
| const React = require("react"); | |
| return { | |
| __esModule: true, | |
| default: ({ name }) => <div data-testid="select" data-name={name} /> | |
| }; | |
| }); | |
| jest.mock("../../formik-inputs/mui-formik-select-v2", () => { | |
| const React = require("react"); | |
| return { | |
| __esModule: true, | |
| default: ({ name }) => <div data-testid="select-v2" data-name={name} /> | |
| }; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/FormItemTable/__tests__/ItemTableField.test.js` around
lines 83 - 89, The mock for "mui-formik-select-v2" is using the same data-testid
as the other select mock, causing ambiguous queries; update the jest.mock for
mui-formik-select-v2 (the default mock component defined in the jest.mock call
for "mui-formik-select-v2") to render a unique attribute such as
data-testid="select-v2" (and keep data-name the same), then update the ComboBox
test assertion/title that expects the v2 select (the test referenced as the
ComboBox test) to query for "select-v2" (and rename the test title if it refers
to v2) so queries like getByTestId/getAllByTestId unambiguously target the v2
mock.
santipalenque
force-pushed
the
feature/mui-components-improvements
branch
from
d8bef51 to
f1cd183
Compare
2 participants
https://app.clickup.com/t/86b8k7d1g
Summary by CodeRabbit
New Features
Improvements
Tests
Chores