Feature: new sponsor cart grid - #278
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds expandable detail rows to ChangesFormItemTable expandable rows refactor
Formik input prop support
Release version bump
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/mui/formik-inputs/mui-formik-checkbox.js (1)
24-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheckbox has no visual required indicator, unlike its sibling inputs in this PR.
MuiFormikSelectV2shows required viaFormControl required(asterisk through context), andMuiFormikDatepicker/MuiFormikTimepickerappend" *"to the label whenrequiredis true. Here,requiredjust flows through...propsonto<Checkbox>(valid HTML attribute, no visual effect), andFormControlLabeldoesn't support arequiredprop, so a required checkbox field (e.g.ItemTableField'sCheckBoxcase) renders with no required cue at all.💡 Suggested fix for parity with sibling inputs
-const MuiFormikCheckbox = ({ name, label, margin = "normal", ...props }) => { +const MuiFormikCheckbox = ({ name, label, margin = "normal", required, ...props }) => { const [field, meta] = useField({ name, type: "checkbox" }); + const displayLabel = required ? `${label} *` : label; return ( <FormControl fullWidth margin={margin} error={meta.touched && Boolean(meta.error)} > <FormControlLabel control={ <Checkbox name={name} {...field} checked={field.value} + required={required} // eslint-disable-next-line react/jsx-props-no-spreading {...props} /> } - label={label} + label={displayLabel} />🤖 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/formik-inputs/mui-formik-checkbox.js` around lines 24 - 30, The required state is not rendered visually for MuiFormikCheckbox because the `required` prop is only passed through `...props` to `<Checkbox>` and `FormControlLabel` does not display it. Update `MuiFormikCheckbox` so the label or container reflects `required` the same way as `MuiFormikSelectV2` and the date/time pickers do, using the `required` prop from the component props and ensuring the `ItemTableField` checkbox case shows a visible required cue.
🤖 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/formik-inputs/mui-formik-checkbox.js`:
- Around line 24-30: The required state is not rendered visually for
MuiFormikCheckbox because the `required` prop is only passed through `...props`
to `<Checkbox>` and `FormControlLabel` does not display it. Update
`MuiFormikCheckbox` so the label or container reflects `required` the same way
as `MuiFormikSelectV2` and the date/time pickers do, using the `required` prop
from the component props and ensuring the `ItemTableField` checkbox case shows a
visible required cue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23ae8d9a-8b61-4627-a054-626803fd0f1c
📒 Files selected for processing (11)
package.jsonsrc/components/index.jssrc/components/mui/FormItemTable/__tests__/FormItemTable.test.jssrc/components/mui/FormItemTable/components/ExpandedRowContent.jssrc/components/mui/FormItemTable/components/ItemTableField.jssrc/components/mui/FormItemTable/index.jssrc/components/mui/formik-inputs/mui-formik-checkbox.jssrc/components/mui/formik-inputs/mui-formik-datepicker.jssrc/components/mui/formik-inputs/mui-formik-select-v2.jssrc/components/mui/formik-inputs/mui-formik-timepicker.jssrc/i18n/en.json
There was a problem hiding this comment.
Pull request overview
Adds an expandable “Details” experience to the sponsor cart FormItemTable, moving per-item meta fields and notes into a collapsible row area and updating several Formik/MUI input wrappers to better support required indicators and label behavior.
Changes:
- Introduces per-row expand/collapse UI with a new “Details” column and expanded-row content container.
- Improves form field rendering for required state and label shrinking across select/date/time/checkbox inputs.
- Updates i18n strings, exports, tests, and bumps package version.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/i18n/en.json | Adds “Details” translation key for the new table column. |
| src/components/mui/FormItemTable/index.js | Reworks table layout to support expandable rows and details status icon; updates props to use touched/errors. |
| src/components/mui/FormItemTable/components/ItemTableField.js | Enhances per-field props (required/shrink) and normalizes option values to strings. |
| src/components/mui/FormItemTable/components/ExpandedRowContent.js | New component rendering extra/meta fields + inline notes in the expanded area. |
| src/components/mui/FormItemTable/tests/FormItemTable.test.js | Updates tests to reflect new table structure and details behavior. |
| src/components/mui/formik-inputs/mui-formik-timepicker.js | Adds required label handling and allows external slotProps passthrough. |
| src/components/mui/formik-inputs/mui-formik-select-v2.js | Adds required support and forces InputLabel to shrink. |
| src/components/mui/formik-inputs/mui-formik-datepicker.js | Adds required label handling and external slotProps passthrough. |
| src/components/mui/formik-inputs/mui-formik-checkbox.js | Makes checkbox margin configurable (supports compact layouts). |
| src/components/index.js | Exposes ExpandedRowContent from the library entrypoint. |
| package.json | Bumps package version to 5.0.38-beta.2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@santipalenque please review
|
done @smarcet |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/mui/FormItemTable/index.js (3)
63-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
touchedbefore indexing it.Line 82 can throw when
errorsis populated buttouchedis still undefined/null. The component already treatstouchedas nullable elsewhere, so this path should too.Proposed fix
useEffect(() => { - if (!errors || Object.keys(errors).length === 0) return; + if (!errors || !touched || Object.keys(errors).length === 0) return; setOpenRows((prev) => { const updates = {}; data.forEach((row) => {🤖 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/FormItemTable/index.js` around lines 63 - 83, The `useEffect` in `FormItemTable` indexes `touched[key]` even though `touched` can be null/undefined, which can throw when errors exist before touched is initialized. Update the `hasVisibleError` check to guard `touched` before reading from it, matching the nullable handling used elsewhere in this component. Keep the fix local to the `useEffect` path that builds `expandedKeys` and evaluates visible errors.
202-276: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the controlled details state on both toggle buttons.
Both buttons toggle the details region, but Line 210 and Line 266 only provide a static label. Add
aria-expandedandaria-controlsso assistive tech can report the current state and target.Proposed fix
const disabled = !isItemAvailable(row, currentApplicableRate); const isOpen = !!openRows[row.form_item_id]; + const detailsId = `form-item-${row.form_item_id}-details`; ... size="small" aria-label="Toggle row details" + aria-expanded={isOpen} + aria-controls={detailsId} onClick={() => toggleRow(row.form_item_id)} ... size="small" aria-label="Toggle row details" + aria-expanded={isOpen} + aria-controls={detailsId} onClick={() => toggleRow(row.form_item_id)} ... - <TableCell colSpan={totalColumns} sx={{ padding: 0 }}> + <TableCell id={detailsId} colSpan={totalColumns} sx={{ padding: 0 }}>🤖 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/FormItemTable/index.js` around lines 202 - 276, The two row toggle IconButton controls in FormItemTable currently only have a static aria-label, so update both the expand/collapse button and the info button to expose the controlled details state. Use the existing isOpen and toggleRow(row.form_item_id) logic to add aria-expanded and a shared aria-controls value that points to the corresponding ExpandedRowContent/Collapse region, so assistive tech can announce the current state and target.
55-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
datain the auto-expand effect dependencies.The effect builds
expandedKeysfromdata/extraColumns, but only reruns onerrorsandtouched. If rows change while validation state already exists, newly invalid rows can stay collapsed.🤖 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/FormItemTable/index.js` around lines 55 - 88, The auto-expand logic in the useEffect inside FormItemTable only depends on errors and touched, but it also derives expandedKeys from data and extraColumns. Add the missing data dependency (and anything derived from it if needed, such as extraColumns) so the effect reruns when rows change and newly invalid rows are opened correctly.
🤖 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.
Outside diff comments:
In `@src/components/mui/FormItemTable/index.js`:
- Around line 63-83: The `useEffect` in `FormItemTable` indexes `touched[key]`
even though `touched` can be null/undefined, which can throw when errors exist
before touched is initialized. Update the `hasVisibleError` check to guard
`touched` before reading from it, matching the nullable handling used elsewhere
in this component. Keep the fix local to the `useEffect` path that builds
`expandedKeys` and evaluates visible errors.
- Around line 202-276: The two row toggle IconButton controls in FormItemTable
currently only have a static aria-label, so update both the expand/collapse
button and the info button to expose the controlled details state. Use the
existing isOpen and toggleRow(row.form_item_id) logic to add aria-expanded and a
shared aria-controls value that points to the corresponding
ExpandedRowContent/Collapse region, so assistive tech can announce the current
state and target.
- Around line 55-88: The auto-expand logic in the useEffect inside FormItemTable
only depends on errors and touched, but it also derives expandedKeys from data
and extraColumns. Add the missing data dependency (and anything derived from it
if needed, such as extraColumns) so the effect reruns when rows change and newly
invalid rows are opened correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd5e230e-e687-4869-8e45-14d2487f7cff
📒 Files selected for processing (3)
package.jsonsrc/components/mui/FormItemTable/components/ItemTableField.jssrc/components/mui/FormItemTable/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/components/mui/FormItemTable/components/ItemTableField.js
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@santipalenque please review
#278 (comment)
|
@santipalenque const hasItemLevel = (row.meta_fields ?? []).some(
(mf) => mf.class_field === SPONSOR_FORMS_METAFIELD_CLASS.ITEM && mf.type === "Quantity"
);
return hasFormLevel || hasItemLevel;But So for a row that has an Item-class Since only Form-class fields should drive the row quantity, I'd just drop the Could we add a regression test for this in describe("hasDrivingQuantityField", () => {
test("returns false for a row whose only Quantity field is Item-class", () => {
const row = {
form_item_id: 1,
meta_fields: [
{ type_id: 1, class_field: "Item", type: "Quantity" }
]
};
expect(hasDrivingQuantityField(row, [])).toBe(false);
});
test("returns true when a Form-class Quantity field exists", () => {
const row = { form_item_id: 1, meta_fields: [] };
const extraColumns = [{ type_id: 1, class_field: "Form", type: "Quantity" }];
expect(hasDrivingQuantityField(row, extraColumns)).toBe(true);
});
});(note: |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@santipalenque please review #278 (comment)
7b4c2b3 to
da94665
Compare
https://app.clickup.com/t/9014802374/86ban7rdx
Summary by CodeRabbit