Conversation
30 tasks
rjvelazco
added this pull request to stack #37613
September 17, 2026 20:32
rjvelazco
changed the title
Contributor
|
Claude finished @rjvelazco's task in 4m 34s —— View job Review — bulk folder delete frontend (#37612)Reviewed against New Issues
Notes (non-blocking)
The M-3 rollback note in the earlier automated comment concerns the base branch's · |
github-actions
Bot
added
the
Area : Frontend
Contributor
|
test |
Contributor
|
Pull Request Unsafe to Rollback!!!
No database migrations, Elasticsearch mapping changes, or structural data-model changes were found in this PR -- the scan-limit/time-budget rewrite in BrowserAPIImpl.java is pure in-memory query logic with no persisted or indexed state at risk. |
…#37063) Red phase for the frontend half. No behaviour is implemented: every new production file is a stub that throws, so the specs compile and fail on what they assert rather than on a missing symbol. The stubs are temporary by agreement and T091 checks that none survive the feature. Types first, because the server half is specified but not built. The wire contract is typed once in dotcms-models so a divergence when the real endpoint lands is a compile error at a handful of sites rather than runtime breakage spread through the UI, and one fixtures module gives every spec the same source of truth for the shapes. The one that will be got wrong if it is not written down: the queue's "active" listing returns every non-terminal run, failed and abandoned included. Marking folders from it unfiltered reports a delete that already failed as still running, and the folder stays marked until the framework moves it on. That is why isJobInProgress exists rather than a not-finished check, and why the fixtures carry a failed and an abandoned run. Also adds the frontend contract and data model, including an open gap raised with the backend half: C-004 requires the four submission refusals to be distinguishable but an empty selection and an over-maximum selection are both 400 with nothing to tell them apart. The tests assume an error code in the body; if the server picks differently only the service and its spec change. Recorded at #37063 (comment) 16 tests failing, 1 vacuously passing until the action exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…7063) Green phase for the first story. An author selects folders, confirms once, and the submission is accepted while they keep working. Delete joins the quick-action registry as the first folders-ONLY action, and that distinction earned a new flag. `supportsFolders` widens an action's scope to the whole selection because it acts on contentlets and folders alike; Delete acts on folders and nothing else, so scoping it that way left it rendering at count 0 over a selection of files — a disabled Delete row that applies to nothing, which is noise and mildly alarming noise. `foldersOnly` scopes it to the folders in the selection, so a selection holding none drops the action entirely, matching how the contentlet-only actions already drop from a folder-only selection. Two identities, deliberately kept apart: the server works in site-qualified paths, the listing marks by row key, and the run carries BOTH a folder's inode and its identifier because the search service only backfills one of them when the API returned none. The confirmation says the folders and everything inside them go, permanently. It makes no claim about workflow: "no workflow action runs on the contents" is false where a content type declares an action for the destroy system action, and the issue text proposed saying exactly that. The run's total is replaced with the count the SERVER accepted once the handle comes back, so the first screen and the last agree even when a duplicate or a nested path is dropped. Two test corrections, both cases of the spec being wrong rather than the code: the back button is `action-preview-back`, and the message-service mock echoes its key and drops its arguments, so the folder count is asserted through the service rather than through rendered text. 249 test files green across portlets-content-drive, content-drive-ui, ui and data-access. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…too (#37063) US2. The grid already marked its rows — that shipped with #37166 — and this is the other half. Without it the grid's marking is a door left open: an author can still walk into, expand, or drop content onto a folder that is being destroyed, and what they put there dies with it. Three separate refusals, not one. Selecting a marked node navigates nowhere, expanding it lists nothing, and dropping on it is declined before the node becomes an active drop target — content dropped there would be created and then destroyed with the folder. The tree reads the SAME store signal the grid marks rows from rather than deriving its own. Two sources would drift, and the drift reads as a folder inert in one surface and usable in the other, which is worse than marking neither: it teaches the author that the marking cannot be trusted. `inFlightKeys` is optional and empty by default, because the AssetPicker renders this same tree and has no such concept. It carries both a folder's inode and its identifier for the reason the grid does — the search service only backfills one of them when the API returned none. Marking stops at the folders submitted and their own nodes. It does not walk descendants: a prefix test on every node on every render, for folders that are about to disappear, buys nothing. Both surfaces now report busy to assistive technology. Reduced opacity and disabled pointer events reach one sense only, and for an operation that permanently destroys things the consequence of missing the signal is acting on a folder about to cease to exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eryone (#37063) US3, and the correctness core of the feature. Until now the marking lived only in the page that started the delete: reload, and a folder being destroyed looked perfectly ordinary again — openable, uploadable, a valid drop target. A measured delete of a 20,000-file folder took about eight minutes, which is long enough that an author will reload, open a second tab, or hand the screen to a colleague inside that window. Three mechanisms, each doing one job, and none of them polling. The set is ESTABLISHED once on load from the queue's in-flight listing; KEPT CURRENT from the per-folder announcements the server makes as folders enter and leave a delete; and RE-ESTABLISHED on the next load rather than inherited — a run whose process dies never announces that it ended, so announcements alone would mark a folder indefinitely with nothing to correct it. THE ONE THAT WILL BE GOT WRONG IF IT IS NOT WRITTEN DOWN: the endpoint is called "active" but answers with every run in a NON-TERMINAL state, failed and abandoned ones included. Read unfiltered, it reports a delete that already failed as still running, and the folder stays marked until the framework moves the run on. The symptom reads in QA as "sometimes folders stay marked forever", which looks like a client defect and is not one. Hence isJobInProgress, and hence a fixture that deliberately carries a failed run and an abandoned one. Kept out of withActionExecution deliberately. That feature owns runs this client fired — started here, ended here. This one holds runs other authors started, which this client can never end. Two lifecycles behind one name would make busyRows mean two things. They merge at a single computed instead, so the listing and the sidebar tree cannot disagree. Paths are the key throughout, because that is the identity every server message uses, and they resolve to row keys at the edge — against the rows currently shown, so the work is bounded by the page rather than by how much is being deleted across the instance. One spec fix while writing the tests: a single mocked socket stream made a start announcement reach the finish handler too, so the feature added a folder and immediately dropped it — a green test proving the opposite of what it read as. 250 test files green across the four touched projects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vived (#37063) US4, and the last P1. A run that ends now tells the author the server's counts and names every folder that did not go, with a reason they can act on. Reason -> copy mirrors failure-reasons.ts, which does the same job for upload, and for the same stated reason: a reason with no copy is a hole the author sees. A Record rather than a switch, so adding a reason to the contract fails the build here until someone writes its sentence. Written once because folder copy (#37062) and move (#37165) read the same outcome shape and the same vocabulary. SKIPPED needed two sentences, not one. A folder an ancestor already removed and a folder the run never reached are both "not attempted" for entirely different reasons, and only the second is a problem. Telling an author their folder was skipped, when what happened is that deleting its parent took it, reads as a fault where there was none. The server's per-folder message is never rendered anywhere. It is diagnostic, written for a log, and a test asserts a real stack trace does not reach the DOM. Reporting reuses the store's existing outcome queue and the shell's existing rendering rather than adding a second path: reportFolderDeleteCompleted mirrors reportUploadCompleted down to the hasOwnProperty guard and the refusal to invent counts for a run that recorded no outcome. Substituting zeros would claim a clean run over nothing; substituting the number submitted would claim every folder was deleted. One new field on the result, outcomeKind, picks the describer. The two reason sets overlap only on PERMISSION_DENIED and UNCLASSIFIED, so guessing from the values would resolve a delete's IN_USE through upload's mapping and land on the unclassified fallback — a reason that HAS copy, rendered as though it had none. Delete's lines come back as one group rather than upload's warn/error split: upload splits because a refused filename and a missing permission are different kinds of news, while every delete line is already "this folder survived, here is why". Severity follows whether anything actually failed, so a run whose only shortfall is skipped folders does not arrive in red. 251 test files green across the four touched projects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the outcome (#37063) US5, US6 and US7, plus polish. The feature is complete against its spec on the client side; what remains needs a server to exist. US5 needed no code. Both its tests passed as written, because #37166 had already moved run ownership from the dialog into the store — so a delete surviving its dialog closing is a property the portlet already had rather than one this feature adds. Recorded as a verification rather than dressed up as work. US6: the sidebar tree now reloads when a delete settles, and only then. The listing and the tree load separately, so refreshing one is not refreshing the other, and a tree still offering a folder the listing has dropped is how an author navigates into nothing. An upload changes contents rather than hierarchy, so reloading the tree for one is a request that can only return the same tree. US7 needed a new concept, and the reason is worth stating. Delete is withheld only when the author may delete NONE of the selection; one they may only partly delete is still submitted whole, with the refusals reported per folder. Those two rules cannot both live in `eligibleWhen`, which narrows *which items* an action fires on — using it here would silently shrink a destructive action the author explicitly selected, which is the failure FR-004a exists to prevent. So availability became its own whole-selection predicate, `availableWhen`, and the count stays what will actually be submitted. The gate itself is EDIT + EDIT_PERMISSIONS: what the shipped context-menu delete checks, and what FolderAPIImpl.delete enforces. Absent permissions read as permitted rather than refused — undefined means the search did not ask, which is not the same as an empty array meaning the author holds none. The end-to-end test is authored and skipped, covering the two things a unit test cannot reach: a run surviving a reload, and both surfaces settling together. The suite lints skipped tests as errors, rightly, so the exemption is explicit at the line with its reason and its removal task rather than the rule being weakened. One assertion was reduced rather than contorted. "An upload does not reload the tree" cannot be honestly asserted in the shell spec, because the sidebar renders inside it and owns its own effect that reloads the tree — every form of "was not called" passes or fails on that caller's timing rather than on the behaviour. The comment says so, and says what isolating it would cost. 252 test files green; lint clean; no stub survives the feature. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… pinned them (#37063) The gap this client raised is closed. Submission refusals are told apart by `errorCode` on the REST layer's existing `ErrorEntity` — the type `ValidationException` already builds its body from — rather than by the ad-hoc field the tests had to assume while the question was open. Status alone could never separate the two 400s, and each needs different copy. This is the divergence the typing was for: one method and its spec changed, and nothing else. The refusal kinds the rest of the client switches on were stable throughout, exactly as the contract note promised. The fallback deliberately does NOT guess between the two 400s when a body carries no code. Telling an author they selected nothing when they hit the ceiling sends them looking for the wrong fix; UNCLASSIFIED says less and misleads less. ONE SHORTFALL, recorded rather than worked around. FR-040 asks the client to name the folder an overlapping run collided on, and the body carries no structured field for it — the folder appears inside `message` as server-generated English, which is not localised and so is not rendered. The refusal therefore says something in the selection is already being deleted without saying which. Parsing the sentence was considered and rejected: it breaks the first time the wording changes, and it puts server English on screen by the back door. Closing it needs a structured field, which is now written down in the contract. Also merges the base branch, which was one commit ahead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rjvelazco
force-pushed
the
issue-37063-bulk-folder-delete-frontend-impl
branch
from
September 18, 2026 15:30
5dd7998 to
638c898
Compare
…ds (#37063) CI caught what the unit tests could not: `signalStore` has typed overloads for at most fifteen features, and this feature pushed the Content Drive store to sixteen. The failure mode is worth knowing, because it does not point at the cause. Past the limit the call silently falls off its overload, `store` degrades to `object`, and the compiler reports "Property 'pages' does not exist on type 'object'" on pagination lines nobody touched. Nothing says "too many features"; the one honest signal is a TS2769 on the `signalStore(` call itself, buried in the cascade. Two arguments came back. `allBusyRows` moves into `withFolderDeleteRuns`, which is better placement anyway — it merges that feature's own state with `busyRows`, so the feature owning half the answer now owns all of it and declares the other half as a required input. The two ceiling computeds fold into one block. Both failure describers now take the wide `DotBatchItemResult<string>[]`. Their whole contract is already that an unrecognised reason falls back rather than throwing, so the narrow type bought nothing and cost a cast at the one call site handling a shared outcome — a cast being precisely where a fallback stops being visible to the next reader. WHY THE TESTS MISSED IT: Vitest transpiles through esbuild without typechecking, so a type error cannot fail a unit test here. `nx build` is the gate that catches it, and it is now part of the local loop rather than something the pipeline finds twenty minutes later. Verified locally before pushing: typecheck back to its pre-existing baseline with nothing from this feature, lint clean, `nx build dotcms-ui` green, and 251 test files green across the four touched projects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
) Whitespace only, 27 lines. Folding the two `withComputed` blocks into one left `$canAddChildren` and its JSDoc a level short, which the frontend `format-test` step rejects. Worth recording why this reached CI at all: the local checks in this session ran through `npx prettier`, which does not resolve this workspace's binary — it reported timings and changed nothing. `pnpm exec prettier` is the repo's 3.8.1 and is what `format-test` runs, so it is what the local loop uses from here. Verified with the build's own command: `pnpm exec prettier --check` over all 36 files this PR touches now reports "All matched files use Prettier code style", with tests and `nx build dotcms-ui` still green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ded (#37063) The strict gate checks only the lines a PR writes, and the four spec files this branch touched carried nine violations. None of them changed what the tests assert; all nine were the specs being looser than the code they cover. - the e2e file called `drive.goto()`, which does not exist — the page object's method is `goTo()`. Transpiled fine, would have failed at run time, and the file is skipped until the backend lands so nothing would have caught it. - `dot-folder-bulk-delete.service.spec.ts` typed a request body as `unknown` and then indexed into it. - `withActionExecution.spec.ts` left the submit mock's return type to be inferred from a bare `Subject`. - `with-folder-delete-runs.spec.ts` cast a ten-field-short object literal to `DotContentDriveState`. Replaced with `buildContentDriveState`, a complete builder in `shared/`, sourced from `shared/constants` — `withActionExecution` already records that the hand-rolled fixtures in sibling specs went stale and stayed green, because the runner transpiles without typechecking. The same spec's `signalStore` call also has to supply `busyRows`, which `withFolderDeleteRuns` declares as an input and the real store gets from `withActionExecution`. It is stood up through `signalStoreFeature` with an explicit input rather than an inline `withComputed`: left to positional inference in a chain that long, TypeScript widens the store to `object` and reports it as an unreadable "no overload matches" on `signalStore` itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d seeds (#37063) The strict gate rejected `currentSite: undefined` and `path: DEFAULT_PATH` (which is `undefined`): both fields are declared non-optional on `DotContentDriveInit`. The store's own `initialState` does exactly this to mean "not resolved yet", but those lines are pre-existing and the gate only checks lines a pull request writes — copying them into a new file put the same lie on new lines. Seeded with `SYSTEM_HOST` and an empty path instead. That is also the better default for a feature spec, which tests behaviour after the portlet has settled rather than before it mounts; a spec that wants the pre-mount shape overrides these two and owns the cast. Found by CI, not locally: the file was still untracked when the gate ran, and the harness diffs base...HEAD through git, so it never saw it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Frontend half of #37063 — Red phase
Stacked on
37063-content-drive-bulk-folder-delete-backend, so this PR shows only the client work. It must not merge tomainahead of the server half: Delete would be offered and every submission would fail.Draft on purpose. This is the failing test set and nothing else — no behaviour is implemented yet.
What is here
dotcms-models, typed once so a divergence when the real endpoint lands is a compile error at a handful of sites rather than runtime breakage spread through the UI.specs/37063-bulk-folder-delete-frontend/.Why there are stubs that throw
A spec for a symbol that does not exist is a compile error, which the constitution's Red gate does not accept. So each production file lands first as a stub whose body throws
'… is not implemented yet (T0xx)', letting the specs compile and fail on what they assert. Every failure reads as a behaviour failure, not a module-resolution one.These are temporary by agreement.
T091in the task list checks thatgrep -rn "is not implemented yet" core-web/libsreturns nothing before the feature is done — a stub reachingmainis a feature that throws at runtime, and the tests that would catch it are the ones written against the stub.The one thing worth reading if you read nothing else
The queue's active listing returns every non-terminal run — failed and abandoned ones included, not only working ones (backend C-011). Marking folders from it unfiltered reports a delete that already failed as still running, and the folder stays marked until the framework moves it on.
That is why
isJobInProgressexists rather than a not-finished check, and why the fixtures deliberately carry a failed run and an abandoned one. The symptom — "sometimes folders stay marked forever" — reads in QA as a client defect and is not one.Open question for the backend half
C-004requires the four submission refusals to be distinguishable, but an empty submission and an over-maximum submission are both400with nothing to tell them apart, and they need different messages.contracts/bulk-delete-api.mdon the base branch confirms the statuses without separating those two.The tests assume an error code in the body. If the server picks a different shape, only
dot-folder-bulk-delete.service.tsand its spec change — the refusal kinds the rest of the client switches on are stable.Raised in full at #37063 (comment 5720585127).
Not here, deliberately
busyRowsshipped with Content Drive: bulk file upload (multi-file selection uploads only the first file) #37166 and is reused; only its accessibility half is added later.Testing
Per ADR-0013 the merge queue skips integration, Postman and Karate for a
core-web/**-only change, so the unit layer is most of this PR's safety net rather than a supplement to a broader one.Baseline before these changes: green across
portlets-content-drive,content-drive-ui,ui,data-access. Note thetypechecktarget is already red onmainfordata-accessand the portlet (106 and 8 errors, all indotcms-js/utils) — unchanged by this PR, verified against HEAD.🤖 Generated with Claude Code
This PR fixes: #37063