fix: sponsor form default values and some style changes#839
Open
santipalenque wants to merge 2 commits intomasterfrom
Open
fix: sponsor form default values and some style changes#839santipalenque wants to merge 2 commits intomasterfrom
santipalenque wants to merge 2 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Formik-integrated MUI Select (MuiFormikSelectV2), updates several Formik/MUI input components to accept labels and small sizing, adjusts table/modal layout spacing, introduces default-selection logic and broader validation for sponsor meta fields, and adds a Jest maxWorkers setting plus a test mock. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (6)
src/components/mui/FormItemTable/__tests__/FormItemTable.test.js (1)
204-217: Make themui-formik-select-v2mock follow Formik state.The real component is Formik-backed, but this stub is static. That means the tests can stay green even if defaulted select values stop rendering or updating correctly. Please bind
value/onChange/onBlurhere, or use the real select in the default-value coverage.♻️ Suggested mock shape
-jest.mock("../../formik-inputs/mui-formik-select-v2", () => ({ - __esModule: true, - default: ({ name, label, options }) => ( - <select data-testid={`select-${name}`} name={name}> - <option value="">{label}</option> - {options && - options.map((opt) => ( - <option key={opt.value} value={opt.value}> - {opt.label} - </option> - ))} - </select> - ) -})); +jest.mock("../../formik-inputs/mui-formik-select-v2", () => { + const { useField } = require("formik"); + return { + __esModule: true, + default: ({ name, label, options }) => { + const [field] = useField(name); + return ( + <select + data-testid={`select-${name}`} + name={name} + value={field.value ?? ""} + onChange={field.onChange} + onBlur={field.onBlur} + > + <option value="">{label}</option> + {(options || []).map((opt) => ( + <option key={opt.value} value={opt.value}> + {opt.label} + </option> + ))} + </select> + ); + } + }; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/FormItemTable/__tests__/FormItemTable.test.js` around lines 204 - 217, The mock for the mui-formik-select-v2 is static and must follow Formik state: update the mocked default export to accept and use Formik's field props (e.g., accept a prop named "field" or destructure { field, form, name, label, options }) and bind value, onChange and onBlur from that field onto the rendered <select> (keep data-testid={`select-${name}`} and options rendering), so tests exercise Formik-driven value updates; ensure the mock reads field.value and calls field.onChange/field.onBlur (or spreads {...field}) so it mirrors the real Formik-backed component behavior.src/components/mui/formik-inputs/mui-formik-timepicker.js (2)
33-38: Conflicting label props may cause confusion.The
TimePickercomponent receiveslabel={timeZone}directly (line 33), whileslotProps.textField.labelreceives the newlabelprop (line 38). In MUI X v6+, the TimePicker's ownlabelprop is deprecated/ignored in favor ofslotProps.textField.label. This results intimeZonebeing ignored while only thelabelprop from line 38 is displayed.If the intent is to show
timeZoneas a label, remove the redundant line 33 or consolidate the logic.♻️ Suggested fix: remove redundant label prop
<TimePicker value={field.value} onChange={helpers.setValue} minTime={minTime} maxTime={maxTime} timezone={timeZone} - label={timeZone} views={["hours", "minutes"]} slotProps={{ textField: { name, 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-timepicker.js` around lines 33 - 38, The TimePicker in mui-formik-timepicker.js currently passes both label={timeZone} to the TimePicker and slotProps.textField.label={label}, causing the TimePicker to ignore timeZone; remove the redundant top-level label={timeZone} prop (or replace it with the intended value) so only slotProps.textField.label is used, ensuring the displayed label comes from slotProps.textField.label (update references to timeZone or label accordingly in the TimePicker and keep name, views, and slotProps.textField intact).
59-61: PropTypes are incomplete.The component accepts
label,minTime,maxTime, andtimeZoneprops but onlynameis declared in PropTypes. Consider adding the missing prop type declarations for better documentation and runtime validation.📝 Suggested PropTypes additions
MuiFormikTimepicker.propTypes = { - name: PropTypes.string.isRequired + name: PropTypes.string.isRequired, + label: PropTypes.string, + minTime: PropTypes.object, + maxTime: PropTypes.object, + timeZone: 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 59 - 61, MuiFormikTimepicker's PropTypes only declare name; add declarations for the other accepted props: declare label as PropTypes.string, minTime and maxTime as PropTypes.instanceOf(Date) (or PropTypes.oneOfType([PropTypes.instanceOf(Date), PropTypes.string]) if strings are allowed), and timeZone as PropTypes.string; keep name as PropTypes.string.isRequired and add any optional defaultProps if applicable to reflect defaults used by the component.src/components/mui/formik-inputs/mui-formik-dropdown-checkbox.js (3)
100-102: Missing PropTypes declaration.The component has no PropTypes defined. Consider adding them for consistency with other components in this codebase and for better documentation.
📝 Suggested PropTypes
+MuiFormikDropdownCheckbox.propTypes = { + name: PropTypes.string.isRequired, + label: PropTypes.string, + options: PropTypes.arrayOf( + PropTypes.shape({ + label: PropTypes.string, + value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]) + }) + ).isRequired, + placeholder: PropTypes.string +}; + export default MuiFormikDropdownCheckbox;Note: You'll need to import
PropTypesat the top of the file.🤖 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-dropdown-checkbox.js` around lines 100 - 102, Add a PropTypes declaration for the MuiFormikDropdownCheckbox component and import PropTypes at the top of the file; specifically define MuiFormikDropdownCheckbox.propTypes with expected props used in the component (e.g., name, label, options (array), value, onChange (func), disabled (bool), required (bool), multiple (bool), helperText (string), fullWidth (bool) and any formik-specific props like field and form as objects) so the component's API is documented and type-checked at runtime.
82-85: Variable shadowing:labelin map callback shadows component prop.The destructured
labelin the.map(({ label }) => label)shadows the outerlabelprop. While this works correctly here, it can cause confusion or bugs if the code is modified later.♻️ Suggested fix: rename inner variable
const selectedNames = options .filter(({ value }) => selected?.includes(value)) - .map(({ label }) => label); + .map(({ label: optLabel }) => optLabel); return selectedNames.join(", ");🤖 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-dropdown-checkbox.js` around lines 82 - 85, The map callback inside the selectedNames computation shadows the component prop named "label"; update the map in the selectedNames logic (the options.filter(...).map(...) chain) to use a different identifier (e.g., map(({ label: optionLabel }) => optionLabel) or map(option => option.label)) so the inner variable no longer conflicts with the outer "label" prop and avoid future confusion or bugs.
93-97: Same shadowing issue in MenuItem mapping.The
labeldestructured from option objects also shadows the component'slabelprop.♻️ Suggested fix: rename inner variable
- {options.map(({ label, value }) => ( + {options.map(({ label: optLabel, value }) => ( <MenuItem key={`ckbx-ddl-${value}`} value={value}> <Checkbox checked={field.value?.includes(value)} /> - <ListItemText primary={label} /> + <ListItemText primary={optLabel} /> </MenuItem> ))}🤖 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-dropdown-checkbox.js` around lines 93 - 97, The mapped destructuring in the MenuItem loop shadows the component prop named label; rename the inner variable (e.g., change ({ label, value }) => to ({ label: optionLabel, value }) =>) and update the Checkbox/ListItemText usage to use optionLabel (ListItemText primary={optionLabel}) and the key if it referenced label, so no local variable shadows the component's label prop inside the MenuItem mapping in the mui-formik-dropdown-checkbox component.
🤖 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/formik-inputs/mui-formik-select-v2.js`:
- Around line 30-33: In renderValue inside the MUI Formik select component
(mui-formik-select-v2.js), the current falsy check `if (!selected)` wrongly
treats valid values like 0 or "" as empty; change the condition to explicitly
check for null/undefined (e.g., `selected === null || selected === undefined`)
and also handle empty array for multi-selects (`Array.isArray(selected) &&
selected.length === 0`) before returning the placeholder (finalPlaceholder) so
valid falsy selections render correctly.
- Line 20: The InputLabel in the MuiFormikSelectV2 component can overlap the
placeholder because it lacks the shrink prop; update the InputLabel rendering
(the element that uses id={`${name}-label`}) to include shrink (e.g.,
<InputLabel shrink id={`${name}-label`}> or shrink={true}) so the label is
forced to the shrunk position when using Select with displayEmpty.
In `@src/pages/sponsors/sponsor-cart-tab/components/edit-form/index.js`:
- Around line 42-45: The CheckBoxList branch currently picks a single default id
via find(), but CheckBoxList is multi-value; change the defaultVal to an array
of ids by replacing find() with filter(...).map(v => v.id) so defaults remain
array-shaped, then return item.current_value || defaultVal || [] (or explicitly
default to [] if needed); also ensure this aligns with the getYupValidation
handling of CheckBoxList.
---
Nitpick comments:
In `@src/components/mui/formik-inputs/mui-formik-dropdown-checkbox.js`:
- Around line 100-102: Add a PropTypes declaration for the
MuiFormikDropdownCheckbox component and import PropTypes at the top of the file;
specifically define MuiFormikDropdownCheckbox.propTypes with expected props used
in the component (e.g., name, label, options (array), value, onChange (func),
disabled (bool), required (bool), multiple (bool), helperText (string),
fullWidth (bool) and any formik-specific props like field and form as objects)
so the component's API is documented and type-checked at runtime.
- Around line 82-85: The map callback inside the selectedNames computation
shadows the component prop named "label"; update the map in the selectedNames
logic (the options.filter(...).map(...) chain) to use a different identifier
(e.g., map(({ label: optionLabel }) => optionLabel) or map(option =>
option.label)) so the inner variable no longer conflicts with the outer "label"
prop and avoid future confusion or bugs.
- Around line 93-97: The mapped destructuring in the MenuItem loop shadows the
component prop named label; rename the inner variable (e.g., change ({ label,
value }) => to ({ label: optionLabel, value }) =>) and update the
Checkbox/ListItemText usage to use optionLabel (ListItemText
primary={optionLabel}) and the key if it referenced label, so no local variable
shadows the component's label prop inside the MenuItem mapping in the
mui-formik-dropdown-checkbox component.
In `@src/components/mui/formik-inputs/mui-formik-timepicker.js`:
- Around line 33-38: The TimePicker in mui-formik-timepicker.js currently passes
both label={timeZone} to the TimePicker and slotProps.textField.label={label},
causing the TimePicker to ignore timeZone; remove the redundant top-level
label={timeZone} prop (or replace it with the intended value) so only
slotProps.textField.label is used, ensuring the displayed label comes from
slotProps.textField.label (update references to timeZone or label accordingly in
the TimePicker and keep name, views, and slotProps.textField intact).
- Around line 59-61: MuiFormikTimepicker's PropTypes only declare name; add
declarations for the other accepted props: declare label as PropTypes.string,
minTime and maxTime as PropTypes.instanceOf(Date) (or
PropTypes.oneOfType([PropTypes.instanceOf(Date), PropTypes.string]) if strings
are allowed), and timeZone as PropTypes.string; keep name as
PropTypes.string.isRequired and add any optional defaultProps if applicable to
reflect defaults used by the component.
In `@src/components/mui/FormItemTable/__tests__/FormItemTable.test.js`:
- Around line 204-217: The mock for the mui-formik-select-v2 is static and must
follow Formik state: update the mocked default export to accept and use Formik's
field props (e.g., accept a prop named "field" or destructure { field, form,
name, label, options }) and bind value, onChange and onBlur from that field onto
the rendered <select> (keep data-testid={`select-${name}`} and options
rendering), so tests exercise Formik-driven value updates; ensure the mock reads
field.value and calls field.onChange/field.onBlur (or spreads {...field}) so it
mirrors the real Formik-backed component behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fd48fce-c517-4cee-b6dd-07234904c533
📒 Files selected for processing (12)
package.jsonsrc/components/mui/FormItemTable/__tests__/FormItemTable.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.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-cart-tab/components/edit-form/index.js
|
|
||
| return ( | ||
| <FormControl fullWidth error={meta.touched && Boolean(meta.error)}> | ||
| {label && <InputLabel id={`${name}-label`}>{label}</InputLabel>} |
There was a problem hiding this comment.
Missing shrink prop on InputLabel causes label overlap with placeholder.
When using displayEmpty on Select, the InputLabel should have shrink prop to prevent the label from overlapping the placeholder text. The MuiFormikDropdownCheckbox component correctly uses shrink but this component doesn't.
🐛 Suggested fix
- {label && <InputLabel id={`${name}-label`}>{label}</InputLabel>}
+ {label && (
+ <InputLabel shrink id={`${name}-label`}>
+ {label}
+ </InputLabel>
+ )}📝 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.
Suggested change
| {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/formik-inputs/mui-formik-select-v2.js` at line 20, The
InputLabel in the MuiFormikSelectV2 component can overlap the placeholder
because it lacks the shrink prop; update the InputLabel rendering (the element
that uses id={`${name}-label`}) to include shrink (e.g., <InputLabel shrink
id={`${name}-label`}> or shrink={true}) so the label is forced to the shrunk
position when using Select with displayEmpty.
Comment on lines
+30
to
+33
| renderValue={(selected) => { | ||
| if (!selected) { | ||
| return <em>{finalPlaceholder}</em>; | ||
| } |
There was a problem hiding this comment.
Falsy value check may incorrectly show placeholder for valid selections.
The condition !selected will treat 0, "", or other falsy values as "no selection" and display the placeholder. If any option can have 0 or an empty string as its value, the selected option won't display correctly.
🐛 Suggested fix: use explicit null/undefined check
renderValue={(selected) => {
- if (!selected) {
+ if (selected === null || selected === undefined) {
return <em>{finalPlaceholder}</em>;
}📝 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.
Suggested change
| renderValue={(selected) => { | |
| if (!selected) { | |
| return <em>{finalPlaceholder}</em>; | |
| } | |
| renderValue={(selected) => { | |
| if (selected === null || selected === undefined) { | |
| return <em>{finalPlaceholder}</em>; | |
| } |
🤖 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 -
33, In renderValue inside the MUI Formik select component
(mui-formik-select-v2.js), the current falsy check `if (!selected)` wrongly
treats valid values like 0 or "" as empty; change the condition to explicitly
check for null/undefined (e.g., `selected === null || selected === undefined`)
and also handle empty array for multi-selects (`Array.isArray(selected) &&
selected.length === 0`) before returning the placeholder (finalPlaceholder) so
valid falsy selections render correctly.
Comment on lines
+42
to
+45
| case "CheckBoxList": { | ||
| const defaultVal = item.values.find((v) => v.is_default)?.id; | ||
| return item.current_value || defaultVal || []; | ||
| } |
There was a problem hiding this comment.
Keep CheckBoxList defaults array-shaped.
CheckBoxList is treated as a multi-value field here and in getYupValidation, but this fallback returns a single id from find(). That drops additional defaults and can start the form in an invalid shape as soon as a default exists.
🐛 Suggested fix
case "CheckBoxList": {
- const defaultVal = item.values.find((v) => v.is_default)?.id;
- return item.current_value || defaultVal || [];
+ if (Array.isArray(item.current_value)) return item.current_value;
+ return (
+ item.values?.filter((v) => v.is_default)?.map((v) => v.id) || []
+ );
}📝 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.
Suggested change
| case "CheckBoxList": { | |
| const defaultVal = item.values.find((v) => v.is_default)?.id; | |
| return item.current_value || defaultVal || []; | |
| } | |
| case "CheckBoxList": { | |
| if (Array.isArray(item.current_value)) return item.current_value; | |
| return ( | |
| item.values?.filter((v) => v.is_default)?.map((v) => v.id) || [] | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors/sponsor-cart-tab/components/edit-form/index.js` around
lines 42 - 45, The CheckBoxList branch currently picks a single default id via
find(), but CheckBoxList is multi-value; change the defaultVal to an array of
ids by replacing find() with filter(...).map(v => v.id) so defaults remain
array-shaped, then return item.current_value || defaultVal || [] (or explicitly
default to [] if needed); also ensure this aligns with the getYupValidation
handling of CheckBoxList.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/86b8k7d1g
Summary by CodeRabbit
New Features
Style
Chores
Tests