Skip to content

feat(content-drive): bulk file upload, and unify how work in progress and outcomes are reported (#37166) - #37378

Draft
zJaaal wants to merge 28 commits into
mainfrom
37166-bulk-file-upload-frontend-impl
Draft

feat(content-drive): bulk file upload, and unify how work in progress and outcomes are reported (#37166)#37378
zJaaal wants to merge 28 commits into
mainfrom
37166-bulk-file-upload-frontend-impl

Conversation

@zJaaal

@zJaaal zJaaal commented Sep 3, 2026

Copy link
Copy Markdown
Member

Frontend half of #37166: the bulk file upload, and the unification of how Content Drive reports work in progress and outcomes.

The upload is not in the diff yet, but it is no longer blocked. The client/server contract is agreed and committed as specs/37166-bulk-file-upload/contracts/bulk-upload-api.md on @dario-daza's branch, and all six items in contracts/client-requirements.md are answered there. The upload lands in this PR next. Everything else is here and reviewable now.

Draft for a review environment. The behaviour here is best judged by using it rather than reading it: four of the defects fixed below were found by running the product while the suite was green.

What this does

Content Drive announced "this is running" four different ways depending on which operation you triggered, and reported outcomes in three. This makes it one of each.

The pattern

The row's own state is the report. Both halves of it — that work is happening, and that it finished. A row dims and goes non-interactive while an operation runs on it, then updates when the run settles. That change is the outcome; the author watched it happen.

This is the design decision behind everything below, and it is what makes the rest fall out:

In-flight state goes to the rows, not to a notification and not by blanking the grid. A workflow action fired from a row's context menu used to replace the entire listing with skeleton rows, hiding the very row you acted on and looking exactly like a page load. Upload and drag-and-drop move announced themselves with a toast that competed with the outcome that followed. Lock and Unlock from the row menu said nothing at all, while the identical Lock from the Workflow Center reported normally.

The toolbar indicator is for runs the rows cannot speak for. Today that is nothing, so it stays quiet. It is what will report the bulk upload, whose content does not exist until the run creates it and therefore has no rows to mark.

Notifications are reserved for what the rows cannot say:

Reported by
Work in progress on rows the rows, dimmed
Work with no rows (upload) the toolbar indicator
Clean success nothing — the rows already showed it
A shortfall: partial, total failure, refusal, counts that do not add up a notification
Success that leaves no visible trace (Add to Bundle, Push Publish) a notification
An outcome arriving after the author moved on a notification

Eight success toasts are gone. The three exceptions are there because in each case the outcome genuinely appears nowhere else — and silence on a shortfall would recreate #37166's original defect, the author believing it worked.

Two defects found while building it

  • Selecting one language version of a contentlet selected them all. dot-folder-list-view took its identifier default for dataKey because the shell never passed one. The codebase already assumed inode — the search service backfills folder inodes with a comment saying the table keys on it.
  • Add to Bundle wrote its failures to a debug log and nowhere else, so a bundle that failed to save looked identical to one that saved. Both dialogs also signalled success only by closing, which reads as having given up.

Worth a reviewer's attention

  • withActionExecution changed shape: one slot → a registry keyed by operation:targets. The old single slot had a documented hazard where one action settling wiped another's indicator and reopened its guard. The guard is now scoped per operation and items, so an upload will not freeze the portlet for minutes.
  • libs/ui and apps/dotcms-ui are touched. dot-folder-list-view gains a busyRows input; the two shared dialogs gain an optional callback in their data. Omitting it preserves today's behaviour exactly, so the nine other consumers of the bundle dialog and every portlet that opens push publish are unaffected.
  • Quiet reload is opt-in. A reload only skips the skeleton where rows are already marked busy. Loud is the default, because the skeleton is otherwise the only signal anything is happening.
  • The failure reason set is now the wire vocabulary, not a parallel one. DISALLOWED_TYPE became DISALLOWED_FILE_TYPE to match the contract's results[].reason, and its message key followed. That set was written before the contract existed and deliberately declared itself client-only; five of its six names matched anyway. Left alone it would have failed quietly rather than loudly: messageKeyForFailureReason falls back to UNCLASSIFIED for anything unrecognised, so the obvious implementation would have rendered generic copy for a rejected file type with nothing complaining, which is the hole FR-036 exists to close.

Proposed Changes

  • Report in-flight state on the toolbar indicator for every operation that outlives its trigger: bulk and single workflow actions, Lock/Unlock from both surfaces, drag-and-drop move, Delete Folder, Add to Bundle, Push Publish.
  • Mark the individual rows an operation is running on, keyed by inode.
  • Replace the single-slot execution state with a keyed run registry; scope the repeat-fire guard to operation and items.
  • Unify outcome copy on the Workflow Center's wording, and route Add to Bundle and Push Publish through the same outcome pipeline instead of new copy.
  • Reload quietly when rows are already marked, so the table does not blank straight after them.
  • Fix dataKey so language versions of a contentlet select independently.
  • Make Add to Bundle and Push Publish report at all, via an optional callback the caller supplies.
  • Absorb the agreed contract into the frontend spec, and settle what it left open: a folder-scoped, deferrable reload (FR-044, FR-043), the two-phase upload/run indicator (FR-041), a beforeunload guard while the upload is in flight (FR-042), and announcing by whether a run was backgrounded rather than by whether rows changed (FR-028a) — which also repairs FR-028, whose "every outcome MUST be announced" forbade the silence this PR already ships.

Checklist

  • Tests — 36 suites / 1475 in portlets-content-drive, 1534 in ui, plus the dotcms-ui push-publish suite. Includes a characterisation spec for withActionExecution, which had no co-located tests before this.
  • Translations — all new copy in Language.properties; obsolete keys removed only after proving them unreferenced across core-web, legacy JSP/VTL and Java.
  • Security Implications Contemplated — content-supplied titles reach the indicator through an [innerHTML] binding and are escaped; the permission gate on every route into an upload is unchanged and covered by tests. No new endpoints.

Additional Info

Still to land here: the bulk upload itself, no longer blocked. The contract answers all six items: failedCount (not failCount); results[].key as the generic item key; and the collision branch, flagged by duplicateSubmission so a successful retry is reportable as the success it is rather than as a batch whose files all collided. Submission is POST /api/v1/assets/_bulkupload, multipart, answered 202 with entity.jobId and entity.statusUrl; completion arrives as BULK_UPLOAD_COMPLETED carrying counts and per-file results.

The server half is spec-only so far, so the client will be built against the contract and mocked until that Java lands. Worth agreeing who exercises the seam first.

The unification landed first deliberately: it needs nothing from the contract, so it could be built and reviewed while that conversation is open. It is also what the upload will report through — the toolbar indicator is the only surface an upload has, since the contentlets do not exist until the run creates them and there are no rows to mark.

Also deliberately unchanged: bulk reindex keeps its own reporting (documented opt-out), and the folder create/edit dialog keeps its modal spinner, since the author is looking at it.

block-editor fails 37 tests on main already; verified identical with these changes stashed.

Screenshots

To be added from the review environment.

This PR fixes: #37166

zJaaal and others added 17 commits September 3, 2026 13:06
…t requirements (#37166)

The two planning artifacts this repo commits. plan.md, research.md, tasks.md,
quickstart.md and checklists/ stay gitignored as process.

data-model.md carries the client-side shapes, and two decisions a future reader
cannot get from the spec: the closed six-reason failure set that needs product
copy, and the `unrecognised` terminal state, which is what any state the client
does not know resolves to. That state renders as an error and never a success,
and it is what makes deferring cancellation to #33331 safe.

contracts/client-requirements.md is INPUT to the cross-team agreement, not the
agreement. It lists the six open items blocking frontend Phase 3 and states what
the client needs behaviourally. The agreed contract belongs in
specs/37166-bulk-file-upload/contracts/, owned by the half that defines the
outcome shape - written there and not in either plan.md, because those are
gitignored and the agreement would not survive the branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…stry rework (#37166)

This store feature had no test of any kind, and #37166 Phases 4-5 rewrite its
core state: the single `actionExecution` slot becomes a keyed registry so an
upload cannot block a selection action for minutes. Rewriting untested shared
state is how the Action Center regresses quietly, so this defines what must
survive first. 22 tests, 84.8% statement coverage.

Passing is the point. This describes what the code does today; it is a safety
net, not a Red gate.

Two behaviours are pinned deliberately because they read as incidental and are
not:

- Counts always come from the response, never from the number of items
  submitted. Every endpoint here answers 200 with per-item failures inside, so
  substituting `inodes.length` would report a refusal as a success.
- Refresh deliberately does NOT set `actionExecution`. That field drives the
  "Applying ..." indicator and locks the Action Center, and neither fits a job
  running for minutes. A rework that "tidies" this by treating every action
  alike would regress it.

Also adds scripts/audit-message-key.sh, which proves a Language.properties key
is unreferenced across core-web, legacy JSP/VTL and Java before it is deleted.
Language.properties is shared well beyond the client, so a core-web-only grep
is not sufficient evidence.

Note for whoever touches the sibling feature specs: they hand-roll an
initialState that has gone stale - it still carries a `totalItems` field the
state no longer has, and omits `pagination.page`, which it now requires. Jest
transpiles without typechecking so they stay green while being wrong. This
file sources its defaults from shared/constants instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…37166)

User Story 6 of the frontend half. Content Drive reported outcomes three
different ways: some named the action and the item, some said "Workflow
Executed" and left the author guessing, and two showed the server's raw error
text. This makes them all follow the Workflow Center's shape.

The copy for the missing half already existed and had never been connected:
`content-drive.toast.workflow-executed-detail` was defined at
Language.properties:7243 and referenced nowhere.

What changes for the author:

- A workflow action fired from a row's context menu now says which action ran
  and on what, on success AND on failure. It said "Workflow Executed" with no
  detail, and on failure named neither.
- Creating a folder no longer shows the untranslated literal "Success" as its
  summary, and names the folder.
- Editing a folder gains a detail naming the folder; it had none at all.
- Lock and unlock failures name the item, as their success counterparts
  already did.
- Both folder failures show product copy instead of the server's raw
  `error.message`. The raw detail moves to the log, which is now the only route
  to it, so the comment says so.

Also adds the per-file upload failure copy - one message per reason in the
closed set - and the pure mapping behind it. A reason without copy is a hole
the author sees, so the mapping is a `Record` over the union rather than a
switch: adding a seventh reason without copy then fails to compile instead of
rendering a blank.

The reason union is the client's own, not the wire format. The server's codes
belong to the submission contract, which is still being agreed, so translating
them is a later step - which is precisely why this story could ship first.

Deletes `content-drive.toast.workflow-in-progress` and its `-detail`, orphaned
and superseded by the toolbar indicator. Both were proven unreferenced across
core-web, legacy JSP/VTL and Java before removal.

Tests first throughout: 36 suites / 1443 tests green, lint clean, and `tsc`
shows 92 pre-existing errors before and after - zero introduced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iding the grid (#37166)

First half of User Story 3. Content Drive announced "this is running" three
different ways depending on what you triggered; two of them are now one.

**The context menu no longer blanks the listing.** Firing a workflow action on
one right-clicked row set the whole grid to LOADING, which replaced every row
with skeletons - hiding the very row the author acted on, and looking exactly
like an ordinary page load. It now reports on the toolbar indicator and the
listing stays rendered. The listing's own loading state goes back to meaning
"the listing is being fetched" and nothing else.

**Upload and drag-and-drop move no longer raise a "starting" notification.**
Those said only that something had begun, which the indicator says better and
without stacking up over the outcome that follows.

The indicator gains two things:

- **A target label**, so a single-item run reads "Applying Publish to My Page"
  rather than "to 1 item(s)" - which told the author nothing they did not
  already know. Escaped like the action name, and it matters more here: an
  action name comes from the backend, a title is typed by an author.
- **An optional position.** `processed` is deliberately optional: absent means
  the run does not report progress, which renders as activity without a
  position, never as zero. Progress readback is the backend's largest piece of
  hidden work, so this degrades rather than assuming a number exists. A
  reported `0` is a position and stays distinct from absence.

Progress updates sit outside the polite live region (`aria-live="off"`): the
region announces that work started and that it finished, but a fifty-file batch
must not speak fifty times on the way.

Deletes the four now-orphaned move keys. `content-drive.file-upload-in-progress`
and its detail are deliberately KEPT even though Content Drive no longer uses
them: the AssetPicker still references them and has the same first-file-only
defect, tracked as #37370. The key audit caught that before the deletion, which
is what it exists for.

`setActionExecution` is transitional and says so - when the single slot becomes
a keyed registry it is replaced by registering and settling a run by id.

36 suites / 1454 tests green, lint clean, tsc unchanged at 92 pre-existing
errors before and after.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
User Story 7. `actionExecution` was a single slot guarded globally: any run in
flight refused every other one. That was tolerable when actions took a second.
It stops being tolerable the moment one of them is a bulk upload running for
minutes, because the guard becomes a freeze - the feature would have made the
portlet worse, not better.

Runs are now a registry keyed by a client-allocated id, and the guard is scoped
to *this operation over these items* (FR-016) rather than to the portlet. Firing
Publish twice on the same row is still refused; locking a row while an upload
runs is not.

The id is allocated client-side rather than taken from a server handle on
purpose: the window a double-click has to fire twice is exactly the window
before any handle has come back, so a server id would leave it unguarded.

Three computeds keep every existing consumer reading unchanged:

- `actionExecution` names the single run when there is exactly one, and is
  undefined when there are several - naming one of several arbitrarily is worse
  than naming none, so the indicator falls back to a count (FR-017).
- `activeRunCount` is that count.
- `busyRows` is every inode any in-flight run touches, keyed by inode rather
  than identifier because the language filter is multi-select and one identifier
  can legitimately occupy several rows.

`setActionExecution`, the transitional setter from the previous commit, is
replaced by `startExternalRun` / `endExternalRun` for the two callers that own
their own service call, the context menu and the drag-and-drop move.

It also removes a hazard rather than working around it: `reportRefreshCompleted`
carried a careful note about not settling through `onSettled` because doing so
would wipe a *different* action's slot. Keying runs by id makes that impossible
by construction.

Correction to the previous commit's premise: `withActionExecution` was not
untested. It has 53 tests in `dot-content-drive.store.spec.ts:1648`, which I
missed when I said it had none. Those tests are what caught every behaviour
change here.

Two deliberate behaviour changes, marked as such in the specs that assert them:
a different action may now start while one is in flight, and the same operation
over *different* items is allowed.

36 suites / 1458 tests green, lint clean, tsc unchanged at 92.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#37166)

Closes a gap the registry commit opened. `actionExecution` is deliberately
undefined with more than one run, because naming one of several arbitrarily is
worse than naming none. But the indicator keyed its visibility off that same
value, so it vanished exactly when the most was happening: two runs meant no
indicator at all.

Visibility now keys off `activeRunCount`, and the label collapses to
"<b>N</b> operations running…" once no single run can be named (FR-017).

36 suites / 1460 tests green, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The toolbar says *what* is running and on how many items. It never said *which*
ones. Fine for three items, useless for forty: the author fires Publish over a
selection and then cannot tell which rows are affected, which have settled, or
whether the row they are about to right-click is one of them.

`dot-folder-list-view` gains a `busyRows` input, mirroring the `lockedByOthers`
input already in the same file and keyed the same way. The store's run registry
already exposes `busyRows`, so this is wiring rather than new machinery.

**Keyed by inode, not identifier.** The language filter is multi-select, so one
identifier legitimately occupies several rows, and marking by identifier would
mark siblings that nothing is happening to.

**A busy row is marked, not replaced.** Explicitly not a skeleton: swapping it
for placeholders repeats, one row at a time, the exact bug this feature removes
- the row the author wants to watch is the one that disappears. It stays
readable, dims, becomes non-interactive, and cannot be selected into a second
action while the first is still running on it. The marker carries
`aria-live="off"` so forty rows do not announce the same news forty times; the
toolbar indicator remains the one polite live region for a run.

Also fixes a selection defect the same keying question caused. The shell never
passed `dataKey`, so the table took the component's `identifier` default and
**selecting one language version of a contentlet selected them all**. The
codebase already assumed inode - the search service backfills folder inodes with
a comment saying the table keys on it - the shell simply never said so. It does
now.

36 suites / 1460 tests in the portlet, 212 in the list view, lint clean in both.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The busy marker replaced the thumbnail's *container* rather than its contents.
That container is the title cell's first grid column and it is sized (4.5rem),
so removing it collapsed the column: the spinner floated into the wrong place
and the row visibly lost its shape. Reported from the running instance.

The marker now swaps what is *inside* the box, leaving the box itself alone, so
the grid keeps its three columns whatever the row is doing.

The regression test took two attempts and the first was worthless. Counting the
title cell's children passes against the broken version too, because it still
rendered exactly one element there - just an unsized one. The test now asserts
the container itself survives with the marker nested inside it, and was verified
to fail against the broken template before being kept.

213 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y marked (#37166)

Reported from the running instance: a second load right after the first. The row
marks appear, the action settles, and then the whole grid blanks to a skeleton
while it refetches - so the author sees loading, then loading again, and the
table jumps.

`loadItems` takes a `quiet` option that refetches without putting the listing
into LOADING, and the three call sites that settle an action after marking rows
now use it: the Action Center's bulk fire, the drag-and-drop move, and the
context-menu workflow action.

**Opt-in, not the default.** The skeleton is not noise: it is the only signal an
author has that anything is happening. Suppressing it everywhere would leave a
folder rename or a side-panel save looking like nothing happened until rows
silently changed underneath. It is redundant in exactly one case - when the
affected rows already carry busy marks - and the caller is what knows that. The
store cannot decide for itself: the run registry is composed after these methods
and is not reachable from `loadItems`.

Upload deliberately stays loud. It creates rows rather than acting on them, so
there is nothing to mark and nothing to be redundant with.

It is still a real refetch, which is what keeps the outcome filter-correct: a row
the action archived or unpublished simply is not in the new result. An optimistic
in-place update could never manage that, because the client cannot know whether
the new state still matches an active filter.

Five tests, with the request held in flight so they measure the listing *during*
the reload rather than after it settles.

36 suites / 1465 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rive (#37166)

Regression from the quiet reload, reported from the running instance: after
firing a workflow action, the row's context menu still offered the actions for
the step the item was on *before* it ran.

The menu is memoized per row, and the memo was dropped by an effect watching for
a LOADING status. That held while every reload blanked the listing. A reload
that settles an action is now quiet and never sets LOADING, so the cache
outlived the data it was built from.

Keying the memo by inode does not save it, which is worth knowing before someone
tries: publishing does not change the working inode, so the same key comes back
pointing at a stale menu.

The effect now also tracks `items`. That is the honest trigger - the rows
arriving is exactly the moment what the menu was built from stopped being true -
and it is agnostic to how the reload was performed, so a future caller cannot
reintroduce this by choosing quiet.

Both signals are read before the guard, or the guard would drop `items` as a
dependency and the effect would never re-run for it.

The regression test was verified to fail against the un-fixed effect before
being kept.

36 suites / 1466 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…37166)

Both dialogs signalled success by closing. From the author's side that reads as
the dialog having given up, not as the work having happened - "basically
non-responding", as reported.

Add to Bundle was worse than silent on failure: the error went to
`loggerService.debug(result.errorMessages)` and nowhere else, so a bundle that
failed to save looked identical to one that saved, except the dialog stayed
open with no explanation. It now reports both outcomes. Push Publish already
showed its errors inside the dialog, so only its success needed saying, and the
copy says *queued* rather than done, because that is what push publish is.

Fixed in the dialogs rather than in Content Drive on purpose. Checked first:
none of the ten places rendering `dot-add-to-bundle` reports anything - every
one binds only `(cancel)` - and the portlets that open the push-publish dialog
(locales, plugins, experiments) call `open()` and walk away. The silence is
universal, so there was no existing pattern to match and every consumer gains
from it: pages, categories, experiments, plugins, rules, content types, and
Content Drive.

`MessageService` is injected **optionally** in both. PrimeNG's is not an
app-level provider in dotcms-ui, and the push-publish dialog is opened globally,
so requiring it would crash the dialog wherever no toast host exists. Missing
provider degrades to the old silent close rather than to an error.

Also adds `content-drive.context-menu.move`, which I referenced for the move
indicator's label and never defined - a drag-and-drop move would have shown the
raw key. Found by a repo-wide sweep for referenced-but-undefined keys.

portlets-content-drive 1466 tests, ui 1534, dotcms-ui push-publish suite green,
lint clean in all three.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… Publish (#37166)

Replaces the previous approach, which did not work and was in the wrong place.

**Why it did not work.** I had both dialogs raise their own toast, injecting
`MessageService` optionally so a missing provider could not crash a globally
opened dialog. In Content Drive that provider did not resolve from the dialog's
injector, so the optional inject returned null and the toast silently did
nothing - the form still just cleared. A failure mode that hides itself is worse
than the silence it was meant to fix.

**Why it was the wrong place anyway.** Nine other consumers render
`dot-add-to-bundle` and several portlets open the push-publish dialog. Giving a
shared dialog a messaging concern it cannot satisfy everywhere it is used means
it either crashes somewhere or, as here, quietly does nothing.

Both dialogs now take an optional callback as data and invoke it; the caller
supplies it and owns its own copy. `DotPushPublishDialogData` gains `onSuccess`,
and `dot-add-to-bundle` gains an `onAdded` input carrying the bundle. Omitting
either keeps today's behaviour exactly, so the other consumers are untouched.

Content Drive supplies both and reports from the shell and the context menu,
where a toast host demonstrably exists. Push publish copy says *queued*, because
that is what it is.

The dialogs themselves are back to their original code apart from the one line
each that invokes the callback.

1466 tests in the portlet, 1534 in ui, push-publish dialog suite green, lint
clean in all three.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orkflow Center's outcome (#37166)

Reuses the messaging rather than copying it. The two operations now publish an
`actionExecutionResult` and the shell's existing effect renders them, which is
the same path the Workflow Center already uses for the same two actions.

The previous commit gave the context menu its own toast and its own copy. That
fixed the silence but recreated the problem this feature exists to remove: one
operation reading two different ways depending on which surface fired it, and
two sets of strings to keep in step. They would have drifted the first time
either was edited.

Publishing a result instead means the wording, the severity, the partial-outcome
handling and the reload all come from one place. The labels are the very keys
the Workflow Center labels these actions with - `Remote-Publish` and
`content-drive.action-center.add-to-bundle` - not lookalikes.

Deletes the six bespoke keys the previous commit added; nothing references them
now, checked with a repo-wide sweep.

The dialogs keep their optional callbacks and are otherwise untouched, so the
nine other consumers of the bundle dialog and every portlet that opens push
publish are unaffected.

1466 tests, lint clean across ui, dotcms-ui and the portlet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last operation in the portlet that said nothing while working, and the one
where it mattered most: `FolderAPI.delete` is recursive over the whole subtree,
so it is the slowest thing here, and it reported nothing between the confirm
dialog closing and its toast arriving.

It now registers a run like every other operation: the toolbar names it and the
folder's own row is marked busy, so the author can see which folder is going.
Cleared on both the success and the failure path - a failed delete that left the
indicator up would report work that had stopped.

Targeted by `identifier` rather than `inode`, deliberately:
`DotContentDriveActionableFolder` carries no inode, and a folder row's inode is
backfilled to equal its identifier by the search service, so this is the key the
grid marks busy by.

The reload is quiet for the same reason as the others: the row is already
marked, so a skeleton would be a second load straight after the first.

Its outcome copy is left alone. It already names the folder and says its
contents went with it, which is better than the shared "<action> ran on N
item(s)" for something recursive and irreversible - and unlike Add to Bundle and
Push Publish, Delete Folder exists on one surface only, so there is no second
wording to drift against.

36 suites / 1469 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#37166)

A run is now stored under `operation:targets` rather than a minted
`OPERATION-<timestamp>-<random>`. It has to be unique and this already is: a
second run with the same key is exactly what the guard refuses, so a collision
cannot arise. Callers hold something readable instead of an opaque token, and
`Date.now()` plus `Math.random()` leaves the file.

**The change quietly weakened the guard, and the tests did not catch it.** All
1469 passed after the swap. Publish on [a,b] and Publish on [a] are *different*
keys, so the second slipped through and would have acted on row `a` twice at
once - the exact thing the guard exists to stop. Busy rows are non-interactive
in the UI, but a disabled control is an affordance, not a lock on the store.

`isRunning` now checks two things: the exact key, and whether any target is
already being touched by any run. The overlap half is *stronger* than the
operation-scoped check it replaces, since it refuses any run over an item
another run already has, which is what the row marks already tell the author.
The key half is what still covers a run with no item targets, such as an upload.

So the lookup-instead-of-scan saving does not materialise. What the natural key
actually buys is readability and one less piece of invented machinery, which is
worth having on its own.

Adds the overlap test, written after the swap because nothing existing covered
partial overlap.

36 suites / 1470 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…un (#37166)

The last place the inconsistency survived. Firing Lock from the Workflow Center
registered a run and reported it; firing the identical Lock from the row's
context menu registered nothing and stayed silent until its toast. Same action,
two behaviours, decided by which surface you started from - which is exactly
what this feature exists to remove.

Both now register a run, so the toolbar names them and the row is marked busy,
and the repeat-fire guard covers them like everything else. Settled on all four
branches: lock success, lock failure, unlock success, unlock failure. A failed
lock that left the indicator up would report work that had stopped.

The request stays this component's own. The Workflow Center locks through the
default workflow action and this locks through the contentlet service, which are
different endpoints; only the *reporting* is shared. Their outcome copy is also
left alone, since it already names the item ("Locked {0}") and there is no
second wording to drift against.

With this, every Content Drive operation that outlives its trigger goes through
the registry, except two deliberate exclusions: reindex, which opts out with a
documented reason, and folder create/edit, which is a modal that owns its own
spinner while the author is looking at it.

36 suites / 1474 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…7166)

Reported from the running instance. Routing lock/unlock through the registry
marked their rows busy but left their reloads loud, so the marks appeared and
the whole grid then blanked to a skeleton - a second load straight after the
first.

Both reload quietly now, the same rule the other four context-menu operations
already follow.

**No test caught it**, which is the part worth noting: 1474 passed with the blink
present, because nothing asserted *how* the reload was performed, only that it
happened. Adds that assertion.

Swept the rest of the portlet for the same shape. The three remaining loud
reloads are correct and stay: a side-panel save, an upload, and folder
create/edit all mark no rows, so the skeleton is the only signal the author has.
Loud is the right default; quiet is the exception that needs a reason.

36 suites / 1475 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @zJaaal's task in 51s —— View job


SDK Compatibility Check

  • Read docs/core/SDK_BREAKING_CHANGE_CATEGORIES.md
  • Diff origin/main...HEAD (439be9d...aa6ee25)
  • Evaluate diff against every category (G-1/G-2/G-3, R-1, U-1/U-2, H-1)
  • Post finding only if breaking

Result: No SDK-breaking change detected.

All 29 changed files are confined to:

  • core-web/libs/portlets/dot-content-drive/** (portlet-local components, store, withActionExecution, failure-reasons.ts, models.ts)
  • core-web/libs/ui/** (dot-folder-list-view, dot-add-to-bundle) and apps/dotcms-ui/** (dot-push-publish-dialog) — admin UI only
  • core-web/libs/dotcms-models/** — portlet/dialog-local models (dot-content-drive.model.ts, dot-push-publish-dialog-data.model.ts), not the SDK's public REST/GraphQL types
  • dotCMS/src/main/webapp/WEB-INF/messages/Language.properties and specs/37166-bulk-file-upload-frontend/** — copy/spec docs

None of these touch the surfaces the reference document scopes the check to:

  • No changes to page-api.ts / GraphQL query building (graphql.page, graphql.content) — G-1/G-2/G-3 N/A
  • No changes to /api/v1/page/*, /api/v1/content, or /api/v1/nav response shapes — R-1 N/A
  • No changes to events.ts (__DOTCMS_UVE_EVENT__) or DotCMSUVEAction payloads — U-1/U-2 N/A
  • No changes to SdkVersionWebInterceptor, X-DotCMS-Version/X-DotCMS-Min-SDK, or compareVersions()/sdk-compatibility.ts — H-1 N/A

This matches the doc's own calibration for non-breaking changes: admin-UI-only and portlet-internal changes with no wire-format impact on @dotcms/client, @dotcms/react, @dotcms/angular, or @dotcms/uve.

Per instructions, no PR comment or SDK Breaking Change label was added since no breaking change was found.

@zJaaal zJaaal changed the title feat(content-drive): unify how work in progress and outcomes are reported (#37166) feat(content-drive): bulk file upload, and unify how work in progress and outcomes are reported (#37166) Sep 3, 2026
zJaaal and others added 3 commits September 3, 2026 15:56
…ws (#37166)

The PM's call, and it removes most of the remaining noise: a notification saying
"Published" while the row visibly publishes in front of the author repeats what
they are already looking at.

**Success is now silent** for every operation whose outcome the listing shows:
workflow actions from both surfaces, lock, unlock, move, upload, folder create,
folder rename, delete folder, and a clean bulk run.

**Three exceptions, each because the outcome is genuinely invisible:**

- **A shortfall always speaks.** Partial failure, total failure, permission
  refusal, counts that do not add up. Those numbers and their causes appear
  nowhere else, and it is the case the author has to act on. Going silent here
  would recreate #37166's original defect - the author believing it worked.
- **Success that leaves no trace speaks**, via `confirmSuccess`: Add to Bundle
  and Push Publish change nothing in the grid, so silence there is the
  "non-responding" complaint all over again.
- **Backgrounded outcomes speak.** They arrive unprompted, minutes after the
  author moved on, and with a dialog open the grid does not even reload, so
  nothing on screen reflects them.

Only the *notification* is suppressed. The grid still reloads and dialogs still
close - those are how the author sees the outcome, so dropping them too would
replace a redundant message with no feedback at all.

Download keeps its message: a browser download starting is not visible in the
listing at all.

The partial-move test now expects zero success toasts and one error. That is the
rule working: a row that *stayed* is indistinguishable from one nobody tried to
move, which is exactly why failures still speak.

36 suites / 1470 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eak for (#37166)

Scoped rather than deleted. For an operation on rows the toolbar line is now
duplication: the rows dim in front of the author and say the same thing. But it
is not dead weight - an upload has no rows to mark, because the content does not
exist until the run creates it, so the indicator is the only surface it has.

So it speaks only for runs with nothing to mark. Today that is none of them, and
the indicator is quiet; when upload lands it is exactly what reports it.

**`toolbarRun` and `toolbarRunCount` are new signals, not a narrowing of
`actionExecution`.** Narrowing that one was the first attempt and it broke eight
tests for the right reason: `actionExecution` is the general "is something
running" signal and the Action Center reads it to gate itself. Overloading it
with a presentation rule conflated two questions - *is work happening* and
*where should it be shown*. They are now separate, and the toolbar reads the
second.

36 suites / 1471 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@zJaaal zJaaal added the PR: docker image Build & push a per-PR test image to dotcms/dotcms-test label Sep 3, 2026
… Center too (#37166)

Regression from the silence change. `confirmSuccess` was set on the two context-
menu paths but not on the store path the Workflow Center uses, so bundling from
the row menu confirmed and bundling from the Workflow Center went quiet - the
same action reading two ways again, which is what this feature exists to remove.

Set on `fireLegacyServletBulk` instead, which is the one place both consumers go
through. Neither Add to Bundle nor Push Publish changes anything in the listing,
so their success has to be said out loud; the row-based operations stay silent
precisely because their rows do change.

Fourteen result assertions widened to `objectContaining`, since the outcome now
carries the extra field.

**A note on how that nearly went wrong.** The first attempt rewrote those
assertions with a regex that opened `expect.objectContaining({` without closing
it. Jest ran the suite and reported 1472 passing anyway - its transform was
lenient enough to accept the unbalanced parentheses. `tsc` found 14 errors
immediately. Reverted and redone with balanced replacements; the two files are
back to their pre-existing baseline of 11 errors, adding none.

Jest passing is not evidence the file is valid.

36 suites / 1472 tests, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🐳 PR Docker test image

Latest build for commit 3530f62 pushed to dotcms/dotcms-test:

docker pull dotcms/dotcms-test:pr-37378-37166-bulk-file-upload-frontend-impl
docker pull dotcms/dotcms-test:pr-37378-37166-bulk-file-upload-frontend-impl_3530f62

…d reporting (#37166)

The submission format is no longer open. It is agreed and written down in
specs/37166-bulk-file-upload/contracts/bulk-upload-api.md, which both halves
reference, so the frontend spec's "Not settled at this altitude" note now
points at it instead of asking planning to produce it. FR-038 names the
field it asked for (totalSizeBytes), and FR-037 with C-002a name the flag
they depended on (duplicateSubmission).

Four contract items the backend added are restated from the consumer's side.
C-001a records what the one-call shape costs this half: no per-file upload
progress and no retry of a single file, so a dropped connection means
resending the batch. C-001a1 records that every guarantee begins at the
handle - while content is still being sent there is no run, so a request
that dies is neither recorded nor notified. C-001b records what it buys:
the staging clock starts server-side immediately before the batch is
created, which is what makes STAGED_CONTENT_UNAVAILABLE a rare safety net
rather than a routine outcome. C-006a records that the completion signal is
emitted only after search-index visibility resolves, which is the ordering
FR-025's single refresh silently relied on.

contracts/client-requirements.md moves from INPUT to ANSWERED, with all six
open items closed and a section for the four things the contract adds that
this half did not ask for.

Three requirements the flow needed and did not have. FR-041 splits a batch
into its two phases, since they carry different guarantees and only one of
them is safe to walk away from; FR-041a notes the upload phase can report a
real position from the browser, because C-001a rules out per-file progress
and not progress. FR-042 requires warning before unload while the upload is
in flight, that being the only moment the interface can intervene in a loss
that is otherwise silent. FR-043 allows and requires deferring the refresh,
because today's refetch clears the selection unconditionally and a run
settling mid-task would take it from the author.

FR-044 scopes the reload to the folder the run actually changed: the
destination for an upload, and the source or the destination for a move,
where the source is the case a destination-only rule would miss because the
visible change there is a removal. Announcing is deliberately not governed
by it. FR-028a states that separately and by the right test - whether the
author was there to watch. A backgrounded run always announces, since it may
settle minutes later or with its reload deferred; a synchronous row action
may stay silent where its rows visibly change, which is the behaviour
d051ae1 shipped. That also repairs FR-028, which required every outcome to
be announced and so forbade what already shipped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@zJaaal zJaaal added the PR : dotbot review Trigger dotbot AI code review on this PR label Sep 7, 2026
this.#store.reportExternalResult({
// `Remote-Publish` is the key the Workflow Center labels this action with
// (`action-center.ts`, PUSH_PUBLISH quick action). Same key, same wording.
actionName: this.#dotMessageService.get('Remote-Publish'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [P1] dot-folder-list-context-menu.component.ts:696 confirm silent Push Publish

Current code:

successCount: 1,
skippedCount: 0,
failCount: 0

Problem: Clean success without confirmSuccess stays silent, no toast shown.

Fix:

successCount: 1,
skippedCount: 0,
failCount: 0,
confirmSuccess: true

UNCLASSIFIED: 'content-drive.upload.failure.unclassified'
};

const isKnownReason = (reason: string): reason is DotContentDriveFailureReason =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [P2] failure-reasons.ts:40 use hasOwnProperty for reason check

Current code:

const isKnownReason = (reason: string): reason is DotContentDriveFailureReason =>
    reason in MESSAGE_KEY_BY_REASON;

Problem: in also matches prototype keys, returning non-string values.

Fix:

const isKnownReason = (reason: string): reason is DotContentDriveFailureReason =>
    Object.prototype.hasOwnProperty.call(MESSAGE_KEY_BY_REASON, reason);

// `Remote-Publish` is the key the Workflow Center labels this action with
// (`action-center.ts`, PUSH_PUBLISH quick action). Same key, same wording.
actionName: this.#dotMessageService.get('Remote-Publish'),
successCount: 1,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dot-folder-list-context-menu.component.ts:702 Push Publish success stays completely silent

Current code:

                        successCount: 1,
                        skippedCount: 0,
                        failCount: 0
                    })

Problem: Clean success without confirmSuccess is suppressed by the shell's new announce gate, so the closed dialog leaves no visible feedback.

Fix:

                        successCount: 1,
                        skippedCount: 0,
                        failCount: 0,
                        confirmSuccess: true
                    })

JSON.stringify(this.setBundleData())
);
this.form.reset();
this.onAdded?.(this.setBundleData());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dot-add-to-bundle.component.ts:148 onAdded receives empty bundle after form.reset()

Current code:

                        this.form.reset();
                        this.onAdded?.(this.setBundleData());

Problem: form.reset() blanks addBundle first, so setBundleData() returns an empty {id: '', name: ''} bundle.

Fix:

                        const bundle = this.setBundleData();
                        this.form.reset();
                        this.onAdded?.(bundle);

zJaaal and others added 2 commits September 7, 2026 12:56
…act (#37166)

The client's failure-reason set was written before the submission contract
existed, and said so: its docblock declared the union "the client's, not the
wire format" and deferred translating server codes to the upload work. Five of
the six names it chose match the contract anyway. DISALLOWED_TYPE does not -
the contract's code is DISALLOWED_FILE_TYPE.

Left alone, that lands as a silent hole rather than an error.
messageKeyForFailureReason takes a bare string and falls back to UNCLASSIFIED
for anything it does not recognise, which is the right soft landing for a
reason the server adds later, but it means the obvious implementation - passing
results[].reason straight in - would render the generic copy for a rejected
file type and nothing would complain. That is the exact hole FR-036 exists to
close.

So the union becomes the wire codes rather than a parallel vocabulary, and the
docblock's "not settled" paragraph is replaced by a pointer to the contract
that now settles it. The alternative was a translation map whose entries almost
all send a name to itself, with the one renaming entry being precisely where
the two halves would drift apart again.

The message key follows the constant to disallowed-file-type. Every sibling key
in Language.properties is the kebab-case of its reason code, so this one had
become the only break in that convention. Renamed in place, so no key is left
defined-and-unreferenced for T098's audit to find.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hared models (#37166)

The reason codes are a wire vocabulary now that they match the submission
contract, so they belong with the other bulk wire types in
dot-content-drive.model.ts rather than in a portlet's utils. The forcing
constraint is the dependency direction: T064 puts the run-following capability
in libs/data-access, and a service there cannot import a type out of a portlet.
Leaving the union where it was would have meant either a duplicate declaration
or an import that Nx should refuse.

What moved is only the vocabulary. Resolving a reason to product copy stays in
the portlet, because copy is the portlet's business - the split follows what
each half actually owns.

Renamed while moving, since the names no longer describe where they live:
CONTENT_DRIVE_FAILURE_REASONS becomes DOT_BULK_UPLOAD_FAILURE_REASONS, and
DotContentDriveFailureReason becomes DotBulkUploadFailureReason. These reasons
are specific to uploading a file - over size, disallowed type, name collision -
so they are named for the batch operation rather than for the portlet that
happens to render them. FR-032's genericity requirement is about the outcome
envelope (counts and results), not about this set.

The spec now imports the real vocabulary instead of restating it. A set
declared twice is a set that can disagree with itself, and the risk the file
exists to guard is a reason arriving with no copy behind it: iterating the
contract's own list means adding a member fails the build here until someone
writes its copy.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

dotbot code review:

  • Reviewer: meta/muse-spark-1.3 (medium)
  • Overall: patch is correct
  • New findings this run: 0
  • Prior unresolved dotbot findings still relevant: 0
  • Active findings total: 0

No new live defects identified in this review run; prior add-to-bundle concern could not be verified as live in current patch and is not carried forward without verbatim evidence.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

reviewed by dotbot · meta/muse-spark-1.3 · medium

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

dotbot code review:

  • Reviewer: ~z-ai/glm-latest (medium)
  • Overall: patch is incorrect
  • New findings this run: 0
  • Prior unresolved dotbot findings still relevant: 1
  • Active findings total: 1

No new actionable bugs were found in the current changes, but 1 prior unresolved dotbot finding still applies, so the patch remains incorrect.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

reviewed by dotbot · ~z-ai/glm-latest · medium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Frontend PR changes Angular/TypeScript frontend code PR: docker image Build & push a per-PR test image to dotcms/dotcms-test PR : dotbot review Trigger dotbot AI code review on this PR

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Content Drive: bulk file upload (multi-file selection uploads only the first file)

1 participant