fix(ui): re-scope bulkUpload onSuccess by field path to fix hasMany upload collision - #17560
Open
firstarriving wants to merge 1 commit into
Open
Conversation
…pload collision BulkUploadProvider held a single onSuccess callback shared by every mounted upload field. Any hasMany upload field's UploadInput reactively re-registered it via setOnSuccess() whenever that field's own value changed, so whichever field's effect last ran "won" ownership of the next upload — not necessarily the field whose Create New button was actually clicked. On a page with more than one hasMany upload field open at once (e.g. a top-level gallery plus per-row galleries inside an array field), uploading through Create New could silently land in a different field's state, making it look like the upload never appended anywhere. This is the same defect class reported in payloadcms#10177 and fixed for the list-view bulk upload in payloadcms#10189 (path-keyed onSuccess map + explicit "current active path"), but a later refactor of this provider (Drawer -> Modal rename, plus the parentID/initialForms additions) reverted back to the single-callback shape. This reintroduces the path-keyed map and adds setCurrentActivePath, called imperatively at the moment a field opens the upload modal, so success always resolves against the field that was actually clicked regardless of mount/render order of its siblings. Repro: a collection with a top-level hasMany upload field and a nested array field whose rows each have their own hasMany upload field (e.g. Payload's own docs example of a "gallery" reused inside a "sections" array). With at least one array row present, using Create New on the top-level field silently fails to append once another upload field's effect has run more recently.
firstarriving
requested review from
AlessioGr,
JarrodMFlesch and
jacobsfletch
as code owners
July 30, 2026 04:01
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.
What
BulkUploadProvider(packages/ui/src/elements/BulkUpload/index.tsx) currently holds a singleonSuccesscallback shared across every mounted upload field. EachUploadInputreactively re-registers it viasetOnSuccess(onUploadSuccess)in auseEffectkeyed on its ownvalue/path— so whichever field's effect last ran "wins" ownership of the next upload's success handler, regardless of which field's "Create New" button the user actually clicked.On any page with more than one
hasManyupload field mounted at once — e.g. a top-level gallery field plus a nested array field whose rows each have their own gallery field — using "Create New" on one field can silently resolve against a different field's closure. The new Media doc gets created successfully, but it's merged into the wrong field's localvalue, so from the field you actually clicked, the upload appears to just vanish.Why this looks like a regression
This is the same defect class as #10177, which #10189 fixed for exactly this reason — introducing a path-keyed
onSuccessmap plus an explicit "current active path" set at click time, rather than relying on effect mount/render order. That fix landed in v3.12.0.A later refactor of this provider (the Drawer → Modal rename, plus the
parentID/initialFormsadditions visible in the currentmain) reverted the shape ofonSuccess/setOnSuccessback down to a single shared callback, with no per-path map and nocurrentActivePath. This PR reintroduces that pattern against the current provider shape.Fix
BulkUploadContext.setOnSuccesstakes(path, onSuccess)and stores callbacks in aRecord<string, onSuccess>map instead of a single value.currentActivePath/setCurrentActivePathto the context, set imperatively the moment a field opens the bulk upload modal (UploadInput.onLocalFileSelection, and the list view'sopenBulkUpload) — so the field that was actually clicked owns the next success callback, independent of any sibling field's render timing.BulkUploadProvider'sonSuccessdispatcher looks up the map bycurrentActivePathinstead of calling a single stored function.currentActivePathis cleared on modal close alongside the other per-session state.Repro
hasManyupload field, and a nestedarrayfield whose rows each contain their ownhasManyupload field (mirrors this repo's ownpackages/ui/src/fields/Uploadreuse pattern — any schema with twohasManyupload fields mounted on the same edit view reproduces this).valueclosure).Notes for reviewers
test/fieldscovering two siblinghasManyupload fields if that's wanted — didn't want to guess at the preferred test harness placement without more context.