Skip to content

fix(ui): re-scope bulkUpload onSuccess by field path to fix hasMany upload collision - #17560

Open
firstarriving wants to merge 1 commit into
payloadcms:mainfrom
firstarriving:fix/bulk-upload-onsuccess-path-collision
Open

fix(ui): re-scope bulkUpload onSuccess by field path to fix hasMany upload collision#17560
firstarriving wants to merge 1 commit into
payloadcms:mainfrom
firstarriving:fix/bulk-upload-onsuccess-path-collision

Conversation

@firstarriving

Copy link
Copy Markdown

What

BulkUploadProvider (packages/ui/src/elements/BulkUpload/index.tsx) currently holds a single onSuccess callback shared across every mounted upload field. Each UploadInput reactively re-registers it via setOnSuccess(onUploadSuccess) in a useEffect keyed on its own value/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 hasMany upload 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 local value, 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 onSuccess map 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/initialForms additions visible in the current main) reverted the shape of onSuccess/setOnSuccess back down to a single shared callback, with no per-path map and no currentActivePath. This PR reintroduces that pattern against the current provider shape.

Fix

  • BulkUploadContext.setOnSuccess takes (path, onSuccess) and stores callbacks in a Record<string, onSuccess> map instead of a single value.
  • Added currentActivePath / setCurrentActivePath to the context, set imperatively the moment a field opens the bulk upload modal (UploadInput.onLocalFileSelection, and the list view's openBulkUpload) — so the field that was actually clicked owns the next success callback, independent of any sibling field's render timing.
  • BulkUploadProvider's onSuccess dispatcher looks up the map by currentActivePath instead of calling a single stored function.
  • currentActivePath is cleared on modal close alongside the other per-session state.

Repro

  1. A collection/global with a top-level hasMany upload field, and a nested array field whose rows each contain their own hasMany upload field (mirrors this repo's own packages/ui/src/fields/Upload reuse pattern — any schema with two hasMany upload fields mounted on the same edit view reproduces this).
  2. Add at least one row to the array field so its upload field mounts alongside the top-level one.
  3. Use "Create New" on the top-level field's dropzone and upload a file.
  4. The Media doc is created, but the new file does not appear in the top-level field — it merges into the array row's field state instead (or is dropped if that field's effect hasn't captured a stale value closure).

Notes for reviewers

  • I wasn't able to run the full monorepo test suite in this environment (sandboxed, no ability to install/build the whole repo) — the diff is a straightforward reapplication of the pattern from fix(ui): prevent many upload fields overwriting bulkUpload onSuccess #10189 onto the current provider shape, but please have CI/e2e confirm.
  • Happy to add a specific e2e/int test under test/fields covering two sibling hasMany upload fields if that's wanted — didn't want to guess at the preferred test harness placement without more context.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant