feat(content-drive): bulk file upload, and unify how work in progress and outcomes are reported (#37166) - #37378
feat(content-drive): bulk file upload, and unify how work in progress and outcomes are reported (#37166)#37378zJaaal wants to merge 28 commits into
Conversation
…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 finished @zJaaal's task in 51s —— View job SDK Compatibility Check
Result: No SDK-breaking change detected. All 29 changed files are confined to:
None of these touch the surfaces the reference document scopes the check to:
This matches the doc's own calibration for non-breaking changes: admin-UI-only and portlet-internal changes with no wire-format impact on Per instructions, no PR comment or |
….com/dotCMS/core into 37166-bulk-file-upload-frontend-impl
…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>
… 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>
🐳 PR Docker test imageLatest build for commit 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>
….com/dotCMS/core into 37166-bulk-file-upload-frontend-impl
| 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'), |
There was a problem hiding this comment.
🔴 [P1] dot-folder-list-context-menu.component.ts:696 confirm silent Push Publish
Current code:
successCount: 1,
skippedCount: 0,
failCount: 0Problem: 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 => |
There was a problem hiding this comment.
🟡 [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, |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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);…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>
|
dotbot code review:
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 |
|
dotbot code review:
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 |
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.mdon @dario-daza's branch, and all six items incontracts/client-requirements.mdare 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:
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
dot-folder-list-viewtook itsidentifierdefault fordataKeybecause the shell never passed one. The codebase already assumedinode— the search service backfills folder inodes with a comment saying the table keys on it.Worth a reviewer's attention
withActionExecutionchanged shape: one slot → a registry keyed byoperation: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/uiandapps/dotcms-uiare touched.dot-folder-list-viewgains abusyRowsinput; 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.DISALLOWED_TYPEbecameDISALLOWED_FILE_TYPEto match the contract'sresults[].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:messageKeyForFailureReasonfalls back toUNCLASSIFIEDfor 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
dataKeyso language versions of a contentlet select independently.beforeunloadguard 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
portlets-content-drive, 1534 inui, plus thedotcms-uipush-publish suite. Includes a characterisation spec forwithActionExecution, which had no co-located tests before this.Language.properties; obsolete keys removed only after proving them unreferenced across core-web, legacy JSP/VTL and Java.[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(notfailCount);results[].keyas the generic item key; and the collision branch, flagged byduplicateSubmissionso a successful retry is reportable as the success it is rather than as a batch whose files all collided. Submission isPOST /api/v1/assets/_bulkupload, multipart, answered202withentity.jobIdandentity.statusUrl; completion arrives asBULK_UPLOAD_COMPLETEDcarrying 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-editorfails 37 tests onmainalready; verified identical with these changes stashed.Screenshots
To be added from the review environment.
This PR fixes: #37166