fix(dashboard): prevent duplicate items in return create form#15348
Conversation
🦋 Changeset detectedLatest commit: 57f8799 The changes in this PR will be included in the next version bump. This PR includes changesets to release 79 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@Metbcy is attempting to deploy a commit to the medusajs Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for your contribution, @Metbcy! Could you please fill in the PR description following our pull request template? We need the Testing section completed so we can review your PR efficiently. The What, Why, and How sections look great — we just need to know how these changes have been tested (or how a reviewer can reproduce and verify the fix). Once you've updated the description, another review will be triggered automatically. Thanks! Triggered by: new PR opened |
|
Thank you for the update, @Metbcy! After reviewing the latest commit, we still need one thing addressed before we can move forward: Required changes:
Concerns: The changes in Could you clarify whether this is intentional? If so, please add it to the What section of the PR description so reviewers understand the scope. Alternatively, consider moving it to a separate PR — bundling unrelated fixes makes reviews harder and complicates reverting if needed. The core fix in Triggered by: new commit pushed |
|
Thank you for the update, @Metbcy! The Testing section is now complete — that concern is resolved. One item from the previous review still needs to be addressed: Required changes:
The fix in Triggered by: PR description updated |
Metbcy
force-pushed
the
fix/return-create-duplicate-items
branch
from
851039d to
02f83e8
Compare
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Thank you for the update, @Metbcy! After reviewing the latest commit, both previously flagged concerns are resolved:
The core fix looks correct: initialising After an initial review, this PR looks good to us. Here's a summary: ✅ PR template is complete A team member will do a final review before this is merged. We appreciate your patience! Triggered by: new commit pushed |
shahednasser
left a comment
shahednasser
left a comment
There was a problem hiding this comment.
Thank you for opening this PR, but it doesn't fix the original issue. I can still reproduce it based on this comment. You should look into the selectedItems array, as mentioned in the OS bot comment
… after save Filter selectedItems against current form items before calling addReturnItem so already-present ordli IDs are not re-posted, and reset selectedItems after save so stale checks cannot leak between dialog opens.
|
Good catch @shahednasser, you're right that the original fix was at the wrong layer. The duplicates come from Pushed 5d61e8d:
Repro from #14936 (alunark's steps) no longer fires for me. |
|
Thank you for the update, @Metbcy! The new approach moves the fix from the After re-review, this PR still looks good to us: ✅ PR template is complete A team member will do the final review before this is merged. We appreciate your patience! Triggered by: new commit pushed |
NicolasGorga
left a comment
NicolasGorga
left a comment
There was a problem hiding this comment.
Hey @Metbcy thanks for the contribution, but I can still reproduce the issue. What I suggest is that we disable the checkbox for any already selected return item in the add return item table. This way, you use said table to select the items you want, which are added with the default 1 quantity amount- at this point, you can update the quantities from the main return modal. If the user then removes the item from there, they will be able to reselect it in the table.
I suggest the following:
// in /packages/admin/dashboard/src/routes/orders/order-create-return/components/add-return-items-table/add-return-items-table.tsx
// add this variable
const alreadyAdded = useMemo(() => new Set(selectedItems), [selectedItems])
...
const updater: OnChangeFn<RowSelectionState> = (fn) => {
const newState: RowSelectionState =
typeof fn === "function" ? fn(rowSelection) : fn
setRowSelection(newState)
onSelectionChange(Object.keys(newState).filter((id) => !alreadyAdded.has(id))) // add here to emit only new selections
}
...
const { table } = useDataTable({
data: queriedItems as AdminOrderLineItem[],
columns: columns,
count: queriedItems.length,
enablePagination: true,
getRowId: (row) => row.id,
pageSize: PAGE_SIZE,
enableRowSelection: (row) => {
return getReturnableQuantity(row.original) > 0 && !alreadyAdded.has(row.original.id) // add this second condition to disable already selected ones
},
rowSelection: {
state: rowSelection,
updater,
},
})With this, you can remove the deduplication in the file you updated and just keep the selectedItems = [] clearing (left comment in the file)
| const existingItemIds = new Set(items.map((i) => i.item_id)) | ||
| const newItems = selectedItems.filter((id) => !existingItemIds.has(id)) | ||
|
|
||
| if (newItems.length) { | ||
| addReturnItem({ | ||
| items: newItems.map((id) => ({ | ||
| id, | ||
| quantity: 1, | ||
| })), | ||
| }) | ||
| } |
There was a problem hiding this comment.
With what I suggested in the review, this could be rollbacked to what we previously had as it would be redundant. Keep the update in L301
Move dedup from the form into the add-return-items-table per NicolasGorga's review: rows already in the return are now disabled in the table (enableRowSelection guard), and the updater only emits genuinely new ids. Rolls back the existingItemIds filter in return-create-form since the table can no longer emit duplicates. The selectedItems = [] reset after save is kept so stale selections don't leak between dialog opens.
|
Thanks @NicolasGorga, makes sense — disabling at the table is the right layer, the user gets a clear signal instead of a silent dedup. Pushed a58d62f:
|
|
Thanks for iterating on this, @Metbcy! The new commit moves the dedup from the form layer into
After re-review, this PR still looks good to us: ✅ PR template is complete A team member will do the final review before this is merged. We appreciate your patience! Triggered by: new commit pushed |
3 participants
Summary
What -- What changes are introduced in this PR?
Prevents duplicate return line items from being created when clicking "Add items" multiple times in the return create form.
Why -- Why are these changes relevant or necessary?
Fixes #14936. The
useEffectthat syncspreviewItemsto form fields usesitems.findIndex()to check for existing entries. However, sinceitems(fromuseFieldArray) does not update mid-render, callingappend()multiple times in the same effect for the same item results in duplicates becausefindIndexkeeps returning -1 for newly appended items.How -- How have these changes been implemented?
Added a local
Set<string>(appendedIds) inside the effect to track item IDs that have already been appended during the current iteration. Before callingappend(), the code now checks both the existingitemsarray and theappendedIdsset, preventing duplicates.Testing -- How have these changes been tested, or how can the reviewer test the feature?
Checklist
Additional Context
The fix is minimal and contained to the
useEffectinreturn-create-form.tsx. No API or backend changes needed since this is purely a frontend form state issue.