fix: add new v2 versions of draggable components #285
Open
tomrndom wants to merge 4 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds new "V2" React components: ChangesAdditionalInput/MetaFieldValues V2
MuiTableSortableV2 sortable table
Module exports and webpack wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SortableRow
participant MuiTableSortableV2
participant onReorder
User->>SortableRow: drag row
SortableRow->>MuiTableSortableV2: handleDragEnd(event)
MuiTableSortableV2->>MuiTableSortableV2: arrayMove(data)
MuiTableSortableV2->>MuiTableSortableV2: update updateOrderKey per row
MuiTableSortableV2->>onReorder: onReorder(reorderedData, movedOrder)
sequenceDiagram
participant User
participant MetaFieldValuesV2
participant ConfirmDialog
participant Formik
User->>MetaFieldValuesV2: click remove icon
MetaFieldValuesV2->>ConfirmDialog: showConfirmDialog
ConfirmDialog-->>MetaFieldValuesV2: confirmed
MetaFieldValuesV2->>MetaFieldValuesV2: onMetaFieldTypeValueDeleted(entityId, fieldId, valueId)
MetaFieldValuesV2->>Formik: setFieldValue(updated values)
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
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
Comment on lines
+74
to
+100
| 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", | ||
| confirmButtonColor: "#DD6B55", | ||
| confirmButtonText: T.translate("general.yes_delete") | ||
| }); | ||
|
|
||
| if (!isConfirmed) return; | ||
|
|
||
| const removeValueFromFields = () => { | ||
| const newFields = [...metaFields]; | ||
| newFields[fieldIndex].values = newFields[fieldIndex].values.filter( | ||
| (_, index) => index !== valueIndex | ||
| ); | ||
| setFieldValue(baseName, newFields); | ||
| }; | ||
|
|
||
| if (field.id && metaFieldValue.id && onMetaFieldTypeValueDeleted) { | ||
| onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( | ||
| () => removeValueFromFields() | ||
| ); | ||
| } else { | ||
| removeValueFromFields(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file ==\n'
git ls-files 'src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js'
wc -l src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js
sed -n '1,220p' src/components/mui/formik-inputs/additional-input/meta-field-values-v2.js | cat -n
printf '\n== showConfirmDialog ==\n'
git ls-files 'src' | rg 'showConfirmDialog\.js$'
wc -l $(git ls-files 'src' | rg 'showConfirmDialog\.js$')
sed -n '1,220p' $(git ls-files 'src' | rg 'showConfirmDialog\.js$') | cat -n
printf '\n== sibling component ==\n'
git ls-files 'src/components/mui/formik-inputs/additional-input' | rg 'AdditionalInputListV2'
sed -n '1,220p' src/components/mui/formik-inputs/additional-input/AdditionalInputListV2.js | cat -nRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 13290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== find sibling handler ==\n'
rg -n "handleRemove|onMetaFieldTypeValueDeleted|showConfirmDialog" src/components/mui/formik-inputs/additional-input -S
printf '\n== locate AdditionalInputListV2 file ==\n'
fd -a 'AdditionalInputListV2.*' src/components/mui/formik-inputs/additional-input
printf '\n== inspect matching files ==\n'
for f in $(fd -a 'AdditionalInputListV2.*' src/components/mui/formik-inputs/additional-input); do
echo "--- $f"
wc -l "$f"
sed -n '1,260p' "$f" | cat -n
doneRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 3479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== additional-input-list-v2.js ==\n'
wc -l src/components/mui/formik-inputs/additional-input/additional-input-list-v2.js
sed -n '1,220p' src/components/mui/formik-inputs/additional-input/additional-input-list-v2.js | cat -n
printf '\n== additional-input-list.js ==\n'
wc -l src/components/mui/formik-inputs/additional-input/additional-input-list.js
sed -n '1,220p' src/components/mui/formik-inputs/additional-input/additional-input-list.js | cat -n
printf '\n== meta-field-values.js ==\n'
wc -l src/components/mui/formik-inputs/additional-input/meta-field-values.js
sed -n '1,220p' src/components/mui/formik-inputs/additional-input/meta-field-values.js | cat -nRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 17165
Handle the API failure path and pass the confirm icon prop.
onMetaFieldTypeValueDeleted(...).then(...)needs a.catch(...); otherwise a delete API rejection becomes an unhandled promise rejection.showConfirmDialogexpectsiconType, nottype, so the warning icon is ignored.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 89-89: Avoid using the initial state variable in setState
Context: setFieldValue(baseName, newFields)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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 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.
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.
Comment on lines
+300
to
+306
| <TablePagination | ||
| component="div" | ||
| count={totalRows} | ||
| rowsPerPageOptions={customPerPageOptions} | ||
| rowsPerPage={perPage} | ||
| page={currentPage - 1} | ||
| onPageChange={handleChangePage} |
Contributor
Author
There was a problem hiding this comment.
update with suggestion on count prop count={totalRows ?? 0}, but I don't think removing onPageChange function not applies here
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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
New Features
Tests
Chores