fix: add new v2 versions of draggable components #285
Open
tomrndom wants to merge 14 commits into
Open
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:
📝 WalkthroughWalkthroughAdds shared Formik-backed V1/V2 metafield editors and a new draggable, sortable MUI table. The change includes confirmation flows, validation, pagination, build wiring, and Jest coverage for editor and table interactions. ChangesAdditionalInput and MetaFieldValues V2
MuiTableSortableV2 sortable table
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AdditionalInput
participant MetaFieldValuesV2
participant Formik
User->>AdditionalInput: render V1 or V2 entry point
AdditionalInput->>MetaFieldValuesV2: inject V2 values component
User->>MetaFieldValuesV2: add, reorder, edit, or remove value
MetaFieldValuesV2->>Formik: setFieldValue(updated values)
sequenceDiagram
participant User
participant SortableRow
participant MuiTableSortableV2
participant onReorder
User->>SortableRow: drag row
SortableRow->>MuiTableSortableV2: report drag end
MuiTableSortableV2->>onReorder: send reordered rows
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 |
…tiful-dnd for @dnd-kit/sortable Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
fix/react-dnd-v2-components
branch
from
July 8, 2026 18:30
2cb07b0 to
5c3ebac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/components/mui/formik-inputs/additional-input/additional-input-list-v2.js (1)
50-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSame
type/iconTypemismatch as MetaFieldValuesV2
showConfirmDialogdestructuresiconType, nottype. The warning icon won't be displayed.🔧 Proposed fix
const isConfirmed = await showConfirmDialog({ title: T.translate("general.are_you_sure"), text: `${T.translate("additional_inputs.delete_warning")} ${ item.name }`, - type: "warning", + iconType: "warning", confirmButtonColor: "`#DD6B55`", confirmButtonText: T.translate("general.yes_delete") });🤖 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/additional-input/additional-input-list-v2.js` around lines 50 - 59, The confirm dialog call in handleRemove uses the wrong option name, so the warning icon is not passed through to showConfirmDialog. Update the dialog config in additional-input-list-v2.js to use iconType instead of type, matching the existing destructuring used by showConfirmDialog and the similar MetaFieldValuesV2 flow.
🧹 Nitpick comments (3)
src/components/mui/formik-inputs/additional-input/additional-input-list-v2.js (1)
72-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo user feedback on API deletion failure
The
.catch()handler logs the error but provides no UI feedback. The user sees no indication that the deletion failed and the item remains in the list with no explanation.💡 Suggested improvement
onDelete(entityId, item.id) .then(() => removeFromUI()) - .catch((err) => console.error("Error deleting field from API", err)); + .catch((err) => { + console.error("Error deleting field from API", err); + // Add user-facing error feedback, e.g.: + // showConfirmDialog({ title: T.translate("general.error"), text: T.translate("additional_inputs.delete_error"), iconType: "error" }); + });🤖 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/additional-input/additional-input-list-v2.js` around lines 72 - 75, The deletion flow in additional-input-list-v2.js only logs API failures in the onDelete promise chain and gives no user-facing feedback. Update the handler around the item.id / onDelete path so the .catch() also surfaces the failure in the UI, using the existing removeFromUI and error handling logic as the reference point. Ensure the user is informed that the delete failed and the item remains in place with an explanatory message or equivalent UI state.src/components/mui/__tests__/mui-table-sortable-v2.test.js (2)
145-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests rely on fragile button index selection.
screen.getAllByRole("button")[1]depends on the exact render order of buttons. If a column is added, made sortable, or an action column is reordered, these tests will silently click the wrong button. Consider addingaria-labelordata-testidattributes to the edit/delete/reorder buttons in the component and selecting by those.🤖 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__/mui-table-sortable-v2.test.js` around lines 145 - 165, The tests in mui-table-sortable-v2 are using fragile button index selection through getAllByRole("button")[1], which can break when the render order changes. Update the component’s edit/delete/reorder buttons to expose stable selectors such as aria-label or data-testid, then change the affected tests to target those specific controls instead of relying on array positions. Use the MuiTableSortableV2 action button rendering and the existing onEdit, onDelete, and showConfirmDialog flows to locate the right elements.
119-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMissing test coverage for core features: drag-end reordering, sort, and per-page change.
The dnd-kit mocks render children directly, so
handleDragEnd(the core reordering logic) is never exercised. Additionally, there are no tests for:
onSortcallback when clicking a sortable column headeronPerPageChangewhen changing rows per page (the mock has a "change-rows" button but no test uses it)- Delete cancellation (
showConfirmDialogreturnsfalse→onDeleteshould not be called)idKeyprop with a non-"id"key🧪 Suggested additional tests
test("calls onSort when sortable column header is clicked", async () => { const onSort = jest.fn(); setup({ onSort }); const sortButton = screen.getByText("Name").closest("button"); await userEvent.click(sortButton); expect(onSort).toHaveBeenCalledWith("name", -1); }); test("does not call onDelete when delete is cancelled", async () => { const onDelete = jest.fn(); showConfirmDialog.mockResolvedValueOnce(false); setup({ onDelete }); const buttons = screen.getAllByRole("button"); await userEvent.click(buttons[1]); await new Promise((r) => setTimeout(r, 0)); expect(showConfirmDialog).toHaveBeenCalled(); expect(onDelete).not.toHaveBeenCalled(); }); test("calls onPerPageChange when rows per page is changed", async () => { const onPerPageChange = jest.fn(); setup({ onPerPageChange }); await userEvent.click(screen.getByLabelText("change-rows")); expect(onPerPageChange).toHaveBeenCalledWith(10); });🤖 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__/mui-table-sortable-v2.test.js` around lines 119 - 182, Add missing coverage in MuiTableSortableV2 tests for the core interactions that aren’t exercised by the current dnd-kit mock setup: verify handleDragEnd/reordering via the component’s drag behavior, assert onSort fires when clicking the sortable header in MuiTableSortableV2, add a test for onPerPageChange using the change-rows control, cover delete cancellation by mocking showConfirmDialog to return false and ensuring onDelete is not called, and add a case for idKey with a non-id field so the row action handlers still target the correct item.
🤖 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/index.js`:
- Around line 142-144: The export stub name is inconsistent with the actual
component/module name, which can cause import confusion when enabled. Update the
stub in the components index to use the same identifier as the module and test,
`MuiTableSortableV2`, and keep the related exports for `MuiAdditionalInputV2`
and `MuiAdditionalInputListV2` aligned with their existing names.
In `@src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js`:
- Around line 41-45: The handlers in meta-field-values-v2.js are mutating Formik
state in place even though they make shallow copies first. Update onReorder,
handleDefaultChange, and removeValueFromFields to create fully immutable updates
by cloning the affected field object at fieldIndex and any nested value objects
before changing values, then pass the new structure to setFieldValue. Use the
existing symbols onReorder, handleDefaultChange, removeValueFromFields, and
metaFields to locate and rewrite the state updates without mutating shared
objects.
- Around line 47-54: `handleAddValue` in `meta-field-values-v2.js` is appending
new value objects without an `order`, which leaves them unsorted until a drag
occurs. Update the new object added in `handleAddValue` to include a computed
`order` based on the current field’s existing values (for example, the next
index in that array), and keep the structure consistent with the value objects
used by `DragNDropList` so the initial sort can work deterministically.
- Around line 74-100: The delete flow in handleRemoveValue needs two fixes: pass
the confirmation icon using showConfirmDialog’s expected iconType option instead
of type so the warning icon appears, and add a catch path to the
onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id) promise
before calling removeValueFromFields. If the API call fails, do not remove the
value and handle the rejection gracefully to avoid an unhandled promise
rejection.
In `@src/components/mui/sortable-table/mui-table-sortable-v2.js`:
- Around line 136-153: handleDelete is hardcoded to delete with item.id, which
breaks consumers that pass a different identifier via the idKey prop. Update the
delete flow in mui-table-sortable-v2.js so handleDelete uses item[idKey] when
calling onDelete, keeping it consistent with the existing idKey-based lookup
used elsewhere in the sortable table component.
- Around line 285-291: The empty-state row in mui-table-sortable-v2 does not
span the extra action columns added by onEdit, onDelete, or onReorder, so the
no-items message only covers the data columns. Update the TableCell colSpan in
the empty-state branch to include the base columns plus any enabled action
columns, using the same flags and rendering logic used by the table header in
mui-table-sortable-v2 so the width stays aligned.
- Around line 106-134: `handleDragEnd` in `mui-table-sortable-v2` is mutating
the original `data` items when updating `updateOrderKey`, so switch to creating
copied row objects before reordering and updating order values, rather than
writing into the existing references returned by `arrayMove`. Also make the
moved-item lookup consistent by using `idKey` everywhere in `handleDragEnd`
instead of hardcoding `movedItem.id`, so `newOrder` resolves correctly when the
row id field is not named `id`.
In `@src/components/mui/sortable-table/sortable-row.js`:
- Around line 34-49: The dragging styles in SortableRow are overriding the
dnd-kit transform by setting a separate scale value inside the isDragging
spread. Update the existing transform handling in sortable-row.js so the row
keeps the CSS.Transform.toString(transform) translation and also applies the
scale effect in the same transform string when isDragging is true, instead of
replacing it with only scale(1.01).
---
Duplicate comments:
In
`@src/components/mui/formik-inputs/additional-input/additional-input-list-v2.js`:
- Around line 50-59: The confirm dialog call in handleRemove uses the wrong
option name, so the warning icon is not passed through to showConfirmDialog.
Update the dialog config in additional-input-list-v2.js to use iconType instead
of type, matching the existing destructuring used by showConfirmDialog and the
similar MetaFieldValuesV2 flow.
---
Nitpick comments:
In `@src/components/mui/__tests__/mui-table-sortable-v2.test.js`:
- Around line 145-165: The tests in mui-table-sortable-v2 are using fragile
button index selection through getAllByRole("button")[1], which can break when
the render order changes. Update the component’s edit/delete/reorder buttons to
expose stable selectors such as aria-label or data-testid, then change the
affected tests to target those specific controls instead of relying on array
positions. Use the MuiTableSortableV2 action button rendering and the existing
onEdit, onDelete, and showConfirmDialog flows to locate the right elements.
- Around line 119-182: Add missing coverage in MuiTableSortableV2 tests for the
core interactions that aren’t exercised by the current dnd-kit mock setup:
verify handleDragEnd/reordering via the component’s drag behavior, assert onSort
fires when clicking the sortable header in MuiTableSortableV2, add a test for
onPerPageChange using the change-rows control, cover delete cancellation by
mocking showConfirmDialog to return false and ensuring onDelete is not called,
and add a case for idKey with a non-id field so the row action handlers still
target the correct item.
In
`@src/components/mui/formik-inputs/additional-input/additional-input-list-v2.js`:
- Around line 72-75: The deletion flow in additional-input-list-v2.js only logs
API failures in the onDelete promise chain and gives no user-facing feedback.
Update the handler around the item.id / onDelete path so the .catch() also
surfaces the failure in the UI, using the existing removeFromUI and error
handling logic as the reference point. Ensure the user is informed that the
delete failed and the item remains in place with an explanatory message or
equivalent UI state.
🪄 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: 13c13ff4-c52f-4976-a261-ae17ee5cdacc
📒 Files selected for processing (10)
src/components/index.jssrc/components/mui/__tests__/additional-input-v2.test.jssrc/components/mui/__tests__/meta-field-values-v2.test.jssrc/components/mui/__tests__/mui-table-sortable-v2.test.jssrc/components/mui/formik-inputs/additional-input/additional-input-list-v2.jssrc/components/mui/formik-inputs/additional-input/additional-input-v2.jssrc/components/mui/formik-inputs/additional-input/meta-field-values-v2.jssrc/components/mui/sortable-table/mui-table-sortable-v2.jssrc/components/mui/sortable-table/sortable-row.jswebpack.common.js
There was a problem hiding this comment.
Pull request overview
This PR introduces “V2” variants of draggable MUI components that use @dnd-kit/* (instead of react-beautiful-dnd), and wires them into the build as new entrypoints while adding Jest coverage for the new variants.
Changes:
- Add new webpack entrypoints for
sortable-table-v2andadditional-input(-list)-v2. - Implement
MuiTableSortableV2+SortableRowusing@dnd-kit/core/@dnd-kit/sortable. - Implement V2 additional-input meta-field value management using the shared
DragNDropList, plus new tests for the V2 components.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.common.js | Adds new build entrypoints for the V2 components. |
| src/components/mui/sortable-table/sortable-row.js | New dnd-kit powered sortable row wrapper for MUI tables. |
| src/components/mui/sortable-table/mui-table-sortable-v2.js | New sortable table implementation using dnd-kit + MUI pagination/actions. |
| src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js | New meta-field values editor with drag reordering via DragNDropList. |
| src/components/mui/formik-inputs/additional-input/additional-input-v2.js | New additional-input V2 component integrating meta-field values management. |
| src/components/mui/formik-inputs/additional-input/additional-input-list-v2.js | New list wrapper for additional-input V2 items and delete/add flows. |
| src/components/mui/tests/mui-table-sortable-v2.test.js | Tests for V2 sortable table rendering + actions + pagination. |
| src/components/mui/tests/meta-field-values-v2.test.js | Tests for meta-field values V2 behavior (add/remove/default). |
| src/components/mui/tests/additional-input-v2.test.js | Integration-style tests between AdditionalInputV2 and MetaFieldValuesV2. |
| src/components/index.js | Adds commented-out export hints for the new V2 components. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
santipalenque
requested changes
Jul 13, 2026
…component on additional input Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js (1)
84-111: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle the API failure path and pass the confirm icon prop.
This is a friendly reminder that the issues identified in a previous review for the deletion flow have not been addressed:
- Pass the confirmation icon using
showConfirmDialog’s expectediconTypeoption instead oftypeso the warning icon appears.- Add a
.catch()block to theonMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id)promise before callingremoveValueFromFields. If the API call fails, the application should handle the rejection gracefully to avoid an unhandled promise rejection.🔧 Proposed fix for the delete flow
const handleRemoveValue = async (metaFieldValue, valueIndex) => { const isConfirmed = await showConfirmDialog({ title: T.translate("general.are_you_sure"), text: T.translate("meta_fields.delete_value_warning"), - type: "warning", + iconType: "warning", confirmButtonColor: "`#DD6B55`", confirmButtonText: T.translate("general.yes_delete") }); if (!isConfirmed) return; const removeValueFromFields = () => { const newFields = metaFields.map((f, i) => i === fieldIndex ? { ...f, values: f.values.filter((_, index) => index !== valueIndex) } : f ); setFieldValue(baseName, newFields); }; if (field.id && metaFieldValue.id && onMetaFieldTypeValueDeleted) { - onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( - () => removeValueFromFields() - ); + onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id) + .then(() => removeValueFromFields()) + .catch((err) => console.error("Failed to delete value", err)); } else { removeValueFromFields(); } };🤖 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/additional-input/meta-field-values-v2.js` around lines 84 - 111, Update handleRemoveValue to pass the warning icon through showConfirmDialog’s iconType option instead of type, and add rejection handling to the onMetaFieldTypeValueDeleted promise chain before removeValueFromFields. Ensure API failures are handled gracefully without removing the value or producing an unhandled promise rejection.
🧹 Nitpick comments (1)
src/components/mui/sortable-table-v2/mui-table-sortable-v2.js (1)
174-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding
sortDirectionon the headerTableCellfor proper ARIA sort semantics.MUI's recommended pattern pairs
TableSortLabelwithsortDirectionon the enclosingTableCell(sortDirection={sortCol === col.columnKey ? (sortDir === -1 ? "desc" : "asc") : false}) so screen readers announce the active sort column/direction viaaria-sort. Currently only the visually-hidden text conveys this.🤖 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/sortable-table-v2/mui-table-sortable-v2.js` around lines 174 - 192, Update the enclosing header TableCell in the sortable column rendering to provide sortDirection based on sortCol and sortDir: use "desc" or "asc" for the active column and false otherwise, while preserving the existing TableSortLabel behavior.
🤖 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.
Duplicate comments:
In `@src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js`:
- Around line 84-111: Update handleRemoveValue to pass the warning icon through
showConfirmDialog’s iconType option instead of type, and add rejection handling
to the onMetaFieldTypeValueDeleted promise chain before removeValueFromFields.
Ensure API failures are handled gracefully without removing the value or
producing an unhandled promise rejection.
---
Nitpick comments:
In `@src/components/mui/sortable-table-v2/mui-table-sortable-v2.js`:
- Around line 174-192: Update the enclosing header TableCell in the sortable
column rendering to provide sortDirection based on sortCol and sortDir: use
"desc" or "asc" for the active column and false otherwise, while preserving the
existing TableSortLabel behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8dd9d4f-b9cc-41df-85c0-c19bb79f451e
📒 Files selected for processing (11)
src/components/index.jssrc/components/mui/__tests__/additional-input.test.jssrc/components/mui/__tests__/meta-field-values-v2.test.jssrc/components/mui/__tests__/mui-table-sortable-v2.test.jssrc/components/mui/formik-inputs/additional-input/additional-input-list.jssrc/components/mui/formik-inputs/additional-input/additional-input.jssrc/components/mui/formik-inputs/additional-input/meta-field-values-v2.jssrc/components/mui/sortable-table-v2/mui-table-sortable-v2.jssrc/components/mui/sortable-table-v2/sortable-row.jssrc/components/mui/sortable-table-v2/styles.module.lesswebpack.common.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/index.js
- webpack.common.js
- src/components/mui/tests/meta-field-values-v2.test.js
smarcet
reviewed
Jul 14, 2026
smarcet
reviewed
Jul 14, 2026
smarcet
reviewed
Jul 14, 2026
smarcet
reviewed
Jul 14, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/mui/__tests__/meta-field-values-v2.test.js (2)
397-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer semantic queries over test IDs and DOM traversal.
Using
getByRolewith the accessible name is more resilient and idiomatic in React Testing Library than querying an SVG icon by test ID and calling.closest("button"). The component already providesaria-label="remove value"on the remove buttons.♻️ Proposed refactor
- const closeButton = screen.getAllByTestId("CloseIcon")[0].closest("button"); + const closeButton = screen.getAllByRole("button", { name: /remove value/i })[0]; await userEvent.click(closeButton); await waitFor(() => expect(mockOnDelete).toHaveBeenCalled()); // today: silently stays at 2 items, no error surfaced - expect(screen.getAllByTestId("CloseIcon")).toHaveLength(2); + expect(screen.getAllByRole("button", { name: /remove value/i })).toHaveLength(2);🤖 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__/meta-field-values-v2.test.js` around lines 397 - 404, Update the test’s close-button lookup to use a semantic role query for the button with accessible name “remove value,” replacing the CloseIcon test ID lookup and closest("button") traversal. Keep the existing click, mockOnDelete assertion, and remaining-item count expectations unchanged.
186-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the order assertion explicit and remove stale debugging comments.
Asserting the exact expected array (e.g.,
[1, 3, 4]) is more robust than checking for uniqueness, and thefails today:comment appears to be a leftover from debugging prior to fixing the component.♻️ Proposed refactor
await waitFor(() => { const orders = latestValues.meta_fields[0].values.map((v) => v.order); - expect(new Set(orders).size).toBe(orders.length); // fails today: [1, 3, 3] + expect(orders).toEqual([1, 3, 4]); });🤖 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__/meta-field-values-v2.test.js` around lines 186 - 188, Update the order assertion in the meta-field values test to compare orders against the exact expected array [1, 3, 4] rather than only checking Set uniqueness, and remove the stale “fails today” debugging comment.
🤖 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__/meta-field-values-v2.test.js`:
- Around line 397-404: Update the test’s close-button lookup to use a semantic
role query for the button with accessible name “remove value,” replacing the
CloseIcon test ID lookup and closest("button") traversal. Keep the existing
click, mockOnDelete assertion, and remaining-item count expectations unchanged.
- Around line 186-188: Update the order assertion in the meta-field values test
to compare orders against the exact expected array [1, 3, 4] rather than only
checking Set uniqueness, and remove the stale “fails today” debugging comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dffd1b3-9b26-4c80-8047-0692b1e06743
📒 Files selected for processing (6)
src/components/index.jssrc/components/mui/__tests__/meta-field-values-v2.test.jssrc/components/mui/__tests__/mui-table-sortable-v2.test.jssrc/components/mui/formik-inputs/additional-input/meta-field-values-v2.jssrc/components/mui/sortable-table-v2/mui-table-sortable-v2.jswebpack.common.js
💤 Files with no reviewable changes (1)
- webpack.common.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/index.js
- src/components/mui/tests/mui-table-sortable-v2.test.js
- src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js
- src/components/mui/sortable-table-v2/mui-table-sortable-v2.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
smarcet
reviewed
Jul 21, 2026
smarcet
reviewed
Jul 21, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/components/mui/__tests__/additional-input-list-core.test.js (1)
209-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't assert the UI actually updates after successful delete.
This only checks
mockOnDeletewas called with the right args, not thatremoveFromUIran afterward (e.g.field-countdecreasing). Other tests in this suite do assert the post-removal count; adding the same here would close the gap for this specific path.🤖 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-list-core.test.js` around lines 209 - 239, The test “calls API and removes from UI when item has id” should also verify the UI updates after a successful deletion. After awaiting the delete action, assert that the field-count element decreases from 1 to 0, while retaining the existing mockOnDelete argument assertion.src/components/mui/formik-inputs/additional-input/additional-input-list-core.js (2)
20-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
_keygeneration is duplicated and collision-prone.
\draft_${Date.now()}`is repeated at three call sites (init effect, add, and remove-to-empty).Date.now()has millisecond resolution, so two drafts created in the same tick (e.g. rapid clicks, or the initial effect firing alongside a synchronous add) can produce identical_keyvalues, which are used as the ReactkeyforAdditionalInputComponent` (Line 113) and can cause incorrect reconciliation.♻️ Suggested helper
+let draftCounter = 0; +const createDraftMetaField = () => ({ + ...DEFAULT_META_FIELD, + _key: `draft_${Date.now()}_${draftCounter++}` +});Then replace each
{ ...DEFAULT_META_FIELD, _key: \draft_${Date.now()}` }withcreateDraftMetaField()`.Also applies to: 40-46, 48-53, 68-73
🤖 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/additional-input/additional-input-list-core.js` around lines 20 - 27, Introduce a createDraftMetaField helper near DEFAULT_META_FIELD that returns a fresh meta field with a collision-resistant unique _key, then replace the duplicated draft_${Date.now()} constructions in the initialization effect, add flow, and remove-to-empty flow with this helper.
109-125: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
areMetafieldsIncomplete()is recomputed for every row.The result doesn't depend on
itemIdx, yet it's invoked once per item inside.map(Line 121), making this O(n) redundant calls per render for an n-item list. Hoist it once above the map.♻️ Suggested fix
+ const isAddDisabled = areMetafieldsIncomplete(); + return ( <> {metaFields.map((item, itemIdx) => ( <AdditionalInputComponent key={item.id || item._key} item={item} itemIdx={itemIdx} baseName={name} onAdd={handleAddItem} onDelete={handleRemove} onDeleteValue={onDeleteValue} entityId={entityId} - isAddDisabled={areMetafieldsIncomplete()} + isAddDisabled={isAddDisabled} /> ))} </> );🤖 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/additional-input/additional-input-list-core.js` around lines 109 - 125, Compute areMetafieldsIncomplete() once before the metaFields.map call in the component’s render flow, store the result in a local value, and pass that value to each AdditionalInputComponent via isAddDisabled instead of invoking the function per row.
🤖 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/mui/__tests__/additional-input-list-core.test.js`:
- Around line 193-207: Update the confirmation-dialog assertion in the “shows
confirmation dialog when delete is clicked” test to expect the corrected
iconType property instead of type, matching the caller behavior in
additional-input-list-core.js. Keep the existing warning value and other dialog
assertions unchanged.
In
`@src/components/mui/formik-inputs/additional-input/additional-input-list-core.js`:
- Around line 77-84: Update the deletion handler around onDelete and
removeFromUI so rejected API deletions surface a user-visible error through the
component’s existing toast or alert mechanism, while preserving the current
behavior of removing the item only after successful deletion. Replace or
supplement the console.error path in the catch handler without changing the
local-only deletion branch.
- Around line 55-64: Replace the unsupported type option in handleRemove’s
showConfirmDialog call with the iconType option set to "warning", ensuring the
warning icon is forwarded to GlobalConfirmDialog and ConfirmDialog.
---
Nitpick comments:
In `@src/components/mui/__tests__/additional-input-list-core.test.js`:
- Around line 209-239: The test “calls API and removes from UI when item has id”
should also verify the UI updates after a successful deletion. After awaiting
the delete action, assert that the field-count element decreases from 1 to 0,
while retaining the existing mockOnDelete argument assertion.
In
`@src/components/mui/formik-inputs/additional-input/additional-input-list-core.js`:
- Around line 20-27: Introduce a createDraftMetaField helper near
DEFAULT_META_FIELD that returns a fresh meta field with a collision-resistant
unique _key, then replace the duplicated draft_${Date.now()} constructions in
the initialization effect, add flow, and remove-to-empty flow with this helper.
- Around line 109-125: Compute areMetafieldsIncomplete() once before the
metaFields.map call in the component’s render flow, store the result in a local
value, and pass that value to each AdditionalInputComponent via isAddDisabled
instead of invoking the function per row.
🪄 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: 736ab949-eeba-4579-b2bb-90ed2111e49a
📒 Files selected for processing (17)
src/components/index.jssrc/components/mui/__tests__/additional-input-core.test.jssrc/components/mui/__tests__/additional-input-list-core.test.jssrc/components/mui/__tests__/additional-input-list-v2.test.jssrc/components/mui/__tests__/additional-input-list.test.jssrc/components/mui/__tests__/additional-input-v2.test.jssrc/components/mui/__tests__/additional-input.test.jssrc/components/mui/__tests__/meta-field-values-v2.test.jssrc/components/mui/formik-inputs/additional-input/additional-input-core.jssrc/components/mui/formik-inputs/additional-input/additional-input-list-core.jssrc/components/mui/formik-inputs/additional-input/additional-input-list-v2.jssrc/components/mui/formik-inputs/additional-input/additional-input-list.jssrc/components/mui/formik-inputs/additional-input/additional-input-v2.jssrc/components/mui/formik-inputs/additional-input/additional-input.jssrc/components/mui/formik-inputs/additional-input/meta-field-values-v2.jssrc/components/mui/sortable-table-v2/mui-table-sortable-v2.jswebpack.common.js
🚧 Files skipped from review as they are similar to previous changes (4)
- webpack.common.js
- src/components/mui/sortable-table-v2/mui-table-sortable-v2.js
- src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js
- src/components/mui/tests/meta-field-values-v2.test.js
Comment on lines
+77
to
+84
| if (item.id && onDelete) { | ||
| onDelete(entityId, item.id) | ||
| .then(() => removeFromUI()) | ||
| .catch((err) => console.error("Error deleting field from API", err)); | ||
| } else { | ||
| removeFromUI(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
API delete failure is swallowed with only console.error.
If onDelete rejects, removeFromUI never runs, but nothing informs the user their deletion failed — from their perspective the click did nothing. Consider surfacing the error (toast/alert) so a failed deletion isn't silently ignored.
🤖 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/additional-input/additional-input-list-core.js`
around lines 77 - 84, Update the deletion handler around onDelete and
removeFromUI so rejected API deletions surface a user-visible error through the
component’s existing toast or alert mechanism, while preserving the current
behavior of removing the item only after successful deletion. Replace or
supplement the console.error path in the catch handler without changing the
local-only deletion branch.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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/mui/__tests__/meta-field-values.test.js`:
- Line 238: Update the relevant showConfirmDialog assertion in the
meta-field-values test to use the producer’s iconType key instead of typeIcon,
preserving the expected "warning" value.
🪄 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: 85193202-002e-4967-b548-19f0ddb0bf21
📒 Files selected for processing (4)
src/components/mui/__tests__/additional-input-list-core.test.jssrc/components/mui/__tests__/meta-field-values.test.jssrc/components/mui/formik-inputs/additional-input/additional-input-list-core.jssrc/components/mui/formik-inputs/additional-input/meta-field-values.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/mui/formik-inputs/additional-input/additional-input-list-core.js
- src/components/mui/tests/additional-input-list-core.test.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
smarcet
reviewed
Jul 22, 2026
smarcet
reviewed
Jul 22, 2026
…e tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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
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.
ref: https://app.clickup.com/t/9014802374/86b94xa4t
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
MuiTableSortableV2with sortable headers, drag-and-drop row reordering, pagination, and edit/delete actions (with confirmation).AdditionalInputV2,AdditionalInputListV2, andMetaFieldValuesV2.nameorvalue.