fix(edit-content): show the workflow comment dialog on first execution (#36883)

[adrianjm-dotCMS]

Spec-Kit PR 2 of 2. Implementation for the spec approved in #37437.

Screen.Recording.2026-09-08.at.12.40.49.PM.mov

What was broken

Firing a workflow action with Allow Comments enabled skipped the comment dialog on its first execution in the new (Angular) Edit Content UI. The contentlet was saved and transitioned, but the author never got to enter the comment — the dialog only appeared on a second execution. Legacy edit mode was correct.

The comment is the audit note recorded in the workflow task history, so this silently degraded the audit trail with no error surfaced.

Root cause

Two REST shapes describe a workflow action, and only one carries actionInputs[]:

Endpoint	Returns	actionInputs[]?
contentlet/{inode}/actions	WorkflowActionView	✅
initialactions/contenttype/{id}	WorkflowDefaultActionView	❌
defaultactions/contenttype/{id}	WorkflowDefaultActionView	❌

The editor gates its input wizard on actionInputs.length, so for content with no inode the array was absent and it took the "no inputs, fire directly" branch. After the first fire the store re-fetches per-inode actions, which do carry the array — hence the dialog on the second click.

Verified against a live instance rather than inferred:

GET /api/v1/workflow/initialactions/contenttype/webPageContent
  Publish → commentable: true,  (no actionInputs key)

GET /api/v1/workflow/contentlet/{inode}/actions?renderMode=EDITING
  Publish → commentable: true,  actionInputs: [{body:{}, id:'commentable'}]

The fix — frontend only, no REST change

Every flag the derivation needs is already serialized on the raw WorkflowAction (none @JsonIgnore), so no backend change is required.

• dot-workflows-actions.utils.ts (new) — deriveActionInputs(), a TypeScript mirror of WorkflowResource#createActionInputViews, plus withDerivedActionInputs() which leaves a server-provided array untouched.
• dot-workflows-actions.service.ts — applied in getDefaultActions() and getWorkFlowActions() only. getByInode() / getByWorkflows() / getBulkActions() are deliberately left alone (commented in place): their endpoints build the array server-side, and deriving over them would hide a future backend regression instead of surfacing it.

libs/edit-content has no production-code change beyond one clarifying comment — all three affected call sites are fixed transitively by the service-boundary fix.

Why not the backend

Emitting WorkflowActionView from those two endpoints is arguably the cleaner contract, but it changes a published REST response on endpoints reachable by customer integrations, and per ADR-0013 it would flip this PR onto the slow CI path — for an identical behavioral outcome. Recorded as accepted tech debt in the plan's Complexity Tracking: the derivation rule now exists in both Java and TypeScript, and each side carries a comment naming the other.

Scope wider than the issue described

The issue describes one reproduction path. There are three, all seeded from the same input-less payload, all closed by this one change:

1. New content, single scheme — initializeNewContent (the reported repro)
2. New content, multi-scheme — setSelectedWorkflow
3. New translation of an untranslated locale — locales.feature

A fourth path claimed by an early draft of the spec — after a Reset action — was disproved during review and removed.

Intentional behavior changes on unsaved content

deriveActionInputs mirrors createActionInputViews in full, so it emits pushPublish too. On content with no inode that routes the action through checkPublishEnvironments() instead of firing directly — so with no publish environment configured, a Push Publish action that previously "saved" now shows the add-an-environment error and does not run.

Deliberate: the previous save only looked like a success. It fired with no whereToSend, and PushPublishActionlet logs "There are no valid environments to sent to" and returns (PushPublishActionlet.java:145-149) — content saved, push publish silently did nothing. Deriving it also keeps new content consistent with saved content, which already gets pushPublish from getByInode.

Narrow reach: needs an admin to put a Push Publish sub-action in the workflow's first step; none of the 7 content types checked on a demo instance have one.

moveable is the second one. A Move action with no preset path also fired directly on unsaved content before; it now folds into a commentAndAssign step and asks the author for a destination. Same reasoning — parity with saved content, which already gets moveable from getByInode — and no environment gate, so it cannot block a save. Worth exercising in QA alongside Push Publish.

assignable is the third. Same mechanism: an action with Assign-to on unsaved content now presents the role picker on the first click instead of firing straight through. Same reasoning again — parity with saved content, which already gets assignable from getByInode — and no gate that can block a save. Already covered by a pre-existing test (dot-edit-content-form.component.spec.ts:691).

Tests

25 tests across the two touched specs plus one new spec file.

deriveActionInputs unit spec (new)	13 — one per rule, combinations, ordering, the move-with-path exclusion, purity, idempotence
Service spec	5 — derivation on both endpoints, [] never undefined, preserve server-provided, and not re-derived on getByInode
Component spec	4 new — dialog gating (AC-002): nothing fires while the dialog is open, fires only on submit carrying the comment, cancel fires nothing, form values survive a cancel
Store spec	1 — actionInputs survives the store's reshape on the reported repro's path

Red confirmed before implementing (Constitution Principle V): Expected: [{"body":{},"id":"commentable"}] / Received: undefined.

A note on what could and could not go Red

Only the utils and service specs can actually fail for this bug. The component's branch logic was never wrong — actionInputs arrives as an argument — and it was already covered. The store specs mock DotWorkflowsActionsService entirely, so they never see the real derivation. Rather than write always-green tests and call them regression guards, the component tests were pointed at the genuinely-uncovered AC-002 gating behavior, and two of the three planned store tests were dropped as redundant copies of the same assertion. Signed off by the developer at the task gate.

Verification

• pnpm nx test data-access — 86 suites, 870 passed
• pnpm nx test edit-content — 116 suites, 2346 passed
• pnpm nx run-many -t lint -p data-access,edit-content — 0 errors (4 warnings, all pre-existing)
• pnpm nx format:check — clean
• npx tsc -p tsconfig.spec.json --noEmit — 0 errors in the touched files (pre-existing baseline unchanged)
• Manual QA in the running app: dialog on first click, gating and cancel behavior, single version and history entry, no-input actions unaffected, and legacy edit mode unchanged (AC-008)
• No file outside core-web/; openapi.yaml untouched

Not run, deliberately: backend integration and Postman. No Java changes, and per ADR-0013 a core-web/**-only PR skips both in the merge queue — confirmed in practice, neither was triggered.

Checklist

• Manual QA, including the legacy-mode check (AC-008)
• E2E — not added. edit-content/workflows/workflow-actions-dialog.spec.ts (the regression spec for [TASK] Show workflow dialogs for comments and user assignment in New Edit Contentlet mode #34347) covers this dialog but always starts from an existing inode, so it never exercises the path this bug lived on. The new-content variant is blocked on fixture behavior: an action created on the workflow's first step does not surface in GET /initialactions/contenttype/{id}. Worth its own investigation.

Additional Info

Resolves #36883. Spec: #37437.

🤖 Generated with Claude Code

This PR fixes: #36883

[claude]

Claude finished @adrianjm-dotCMS's task in 1m 40s —— View job

---

Review

Re-reviewed the current head against origin/main. This is the second review pass — the three Required and four Consider items from nicobytes's review have been worked through in commit b8e0d8c0. I verified each against the code rather than trusting the commit message.

New Issues

No new bugs introduced. The fix is at the right layer (one service boundary), the derivation faithfully mirrors WorkflowResource#createActionInputViews, and the two intentional behavior changes (pushPublish, moveable) are now both tested and documented.

Resolved (prior review findings)

• ✅ dot-workflows-actions.utils.ts:62 — Required gate now uses Array.isArray(action.actionInputs), so a server-sent empty array survives instead of being re-derived. Comment and code agree.
• ✅ dot-edit-content-form.component.spec.ts:176 — Required mock leakage fixed: all four factory-level mocks (DotWizardService.open, containsPushPublish, checkPublishEnvironments, processWorkflowPayload) are now re-established in the root beforeEach, and the one-mock afterEach is gone. Order-independent now.
• ✅ contracts/derive-action-inputs.md:64 — Required sync-obligation claim corrected: it no longer asserts a symmetry that doesn't exist. It honestly states the drift is one-way (TS→Java only) and flags a follow-up, citing ADR-0013 for why no .java comment was added.
• ✅ dot-workflows-actions.mock.ts:421 — Nit/Required createFakeWorkflowAction() added to @dotcms/utils-testing with actionInputs deliberately omitted; both utils.spec.ts and service.spec.ts now use it instead of inline literals.
• ✅ dot-workflows-actions.service.ts:81,122 — Consider three maps collapsed to a single map((res) => deriveInputsOnEach(res?.entity ?? [])).
• ✅ dot-workflows-actions.utils.ts:75 — Consider deriveInputsOnEach moved into the utils file next to withDerivedActionInputs; the long per-endpoint rationale now lives in the contract doc, not inline.
• ✅ dot-edit-content-form.component.spec.ts:820 — Consider moveable second behavior change now has an explicit test ("should open the wizard for a moveable-only action") plus pushPublish-without-environments coverage.
• ✅ dot-workflows-actions.utils.spec.ts:85 — Consider withDerivedActionInputs now has its own direct describe block (preserve vs derive, empty-array preservation, non-mutation, idempotence); the tautological tests are gone.
• ✅ dot-edit-content-form.component.ts:544 — Nit belt-and-braces comment removed; the const { actionInputs = [] } = workflow; default stands on its own.

Existing

• 🟡 Medium: specs/36883-workflow-comments-dialog-new-edit/contracts/derive-action-inputs.md:31 — the header link on line 3 was fixed to ../data-model.md, but this inline reference ([data-model.md](./data-model.md#derivation-rules)) is still relative to contracts/ and resolves to the non-existent contracts/data-model.md. Non-blocking. Fix this →

Nice work — the fix is clean, well-scoped to the frontend, and the test coverage now maps onto behavior that can actually go red. The only thing outstanding is one stale relative doc link, which doesn't block.
· issue-36883-workflow-comments-dialog-new-edit-impl

• Read changed files
• Analyze against prior review findings
• Post review

[nicobytes]

Hand-rolling a DotCMSWorkflowAction here goes against the frontend testing standard: domain fixtures belong in @dotcms/utils-testing as createFake* helpers (same pattern as createFakeContentlet / createFakeSite), not as inline objects in the spec.

There is already mockWorkflowsActions in dot-workflows-actions.mock.ts, but it always includes actionInputs, so it cannot stand in for this suite — these tests need the key omitted, matching WorkflowDefaultActionView. Reusing that constant would hide the bug this describe is covering.

Please add something like createFakeWorkflowAction(overrides?: Partial<DotCMSWorkflowAction>) in @dotcms/utils-testing and use it here (and in dot-workflows-actions.utils.spec.ts, which currently duplicates the same shape as buildRawAction). Keep actionInputs out of the defaults so callers can still represent the raw REST payload this PR is fixing.

[adrianjm-dotCMS]

Done. createFakeWorkflowAction(overrides?) in @dotcms/utils-testing, following the createFakeContentlet pattern, with actionInputs kept out of the defaults as you asked. Used in both specs, so buildRawAction and the duplicated literal are gone. b8e0d8c0f7

[nicobytes]

Review: correctness, quality, simplification, Angular standards

Reviewed locally against 110d64cd46 (branch HEAD matches the PR head), with the Java side verified in the source rather than from the description.

Verdict: approve-with-comments — nothing here blocks. The diagnosis is right, the fix is at the correct layer (one service boundary instead of the three call sites), and the "why not the backend" argument holds up. The three Required items are test hygiene and code-vs-doc consistency, not correctness of the fix.

Verified independently, so treat these as settled

• The Java mirror is faithful. deriveActionInputs matches WorkflowResource#createActionInputViews (WorkflowResource.java:902-928) line for line, including rule order and the moveable-with-path exclusion. The flags it reads are all really serialized: isAssignable() / isCommentable() carry no @JsonIgnore, and the has*Actionlet() getters carry explicit @JsonProperty (Jackson would not auto-detect a hasX() getter otherwise).
• The blast radius is contained. Only three call sites consume getDefaultActions / getWorkFlowActions, all inside libs/edit-content. No other portlet changes behavior. getByWorkflows, left underived, has one consumer — dot-workflows-actions-selector-field.service — which selects actions rather than firing them, so the omission is genuinely inert there.
• No silent no-op path. I checked whether a derived array could make setWizardInput() return null, which would have openWizard call dotWizardService.open(null) and hang with no dialog and no error. It cannot: every id the derivation emits maps to a wizard step. Worth noting the component's openWizard still lacks the if (wizardInput) guard that DotWorkflowEventHandlerService.openWizard has, so this is a near miss rather than a designed-out one.
• Tests pass. pnpm nx test data-access --testPathPatterns=dot-workflows-actions → 2 suites, 25/25 green.

One cross-cutting note

The diff carries roughly 90 lines of comment for 120 lines of code, and three of the findings below (2, 3, 6) are places where a comment and the thing it describes have already diverged before merge. The comments are unusually thoughtful, which is why this is worth saying out loud rather than only case by case: a comment that restates a nearby invariant, or describes code in another file, is a second source of truth that nothing tests. The ones that carry real intent — the Java mirror pointer, the #36883 framing, why getByInode is left alone — are the ones worth keeping.

Findings are labelled Required / Consider / Nit inline; 3 Required, 4 Consider, 6 Nit. I have left the formal approval to you rather than stamping it myself.

---

🤖 Generated with Claude Code

[nicobytes]

Required — this afterEach restores one mock out of four.

The comment diagnoses the problem exactly right: jest.clearAllMocks() (line 190) clears call data but not implementations, and these are factory-level mockProviders shared by the whole file. But the restore only covers DotWizardService.open. Three more overrides introduced in this block are never restored:

• processWorkflowPayload — mockImplementation (line 763)
• containsPushPublish → true (lines 832, 848)
• checkPublishEnvironments → of(false) / of(true) (lines 833, 849)

DotWorkflowEventHandlerService is a factory-level mockProvider too, so from line 832 onward containsPushPublish returns true for the remaining ~1000 lines of this spec. It is latent today only because nothing below describe('Preview Button') fires a workflow action — the next test added there inherits it silently, and will fail for a reason that has nothing to do with its own subject.

Cheaper and order-independent than compensating per-mock: set the default implementations in the file's root beforeEach, or add restoreMocks: true to the Jest config (alternatively jest.restoreAllMocks() in the root afterEach), and drop this afterEach entirely.

[adrianjm-dotCMS]

Confirmed, and worse than it looks: this is the same bug I fixed last round, reintroduced by the new tests.

Went with your first option rather than compensating per mock: the defaults for all four (open, containsPushPublish, checkPublishEnvironments, processWorkflowPayload) are now set in the root beforeEach, and the partial afterEach is gone. Order-independent, and it doesn't depend on the next block remembering.

Ruled out restoreMocks: true — mockProvider creates jest.fn()s, not spies, so restoreAllMocks wouldn't touch their implementations.

[nicobytes]

Required — the gate and its doc comment disagree.

The doc above says "An array the server already sent is left untouched", but action.actionInputs?.length is truthiness on the length: a server-provided empty array is re-derived. The service spec already documents the gap in its own comment ("short-circuits on any NON-EMPTY array"), which is a good sign that the code, not the comment, is the odd one out.

It is harmless today, because both endpoints omit the key entirely. But the stated purpose of this guard is to surface a backend regression rather than mask it, and the one case it fails to guard is exactly the regression shape you would most want to see: an endpoint that starts emitting WorkflowActionView with a legitimately empty actionInputs.

if (Array.isArray(action.actionInputs)) {
    return action;
}

That makes the code match the comment, keeps the "never undefined" guarantee intact, and leaves every current test green — the getByInode assertion still holds, since that fixture omits the key rather than sending [].

[adrianjm-dotCMS]

Right. Array.isArray(action.actionInputs), exactly as proposed. Added an explicit test for the server-sent [] — the one case the guard claimed to cover and didn't. Also updated the guarantees table in contracts/.

[nicobytes]

Required — the sync obligation only exists on one side.

"Each side carries a comment naming the other" is not true as of this PR: there is no Java change in the diff (0 .java files), and WorkflowResource.java contains no reference to dot-workflows-actions.utils.ts or to deriveActionInputs. So a Java dev adding a fifth input type to createActionInputViews has nothing pointing them at the TypeScript mirror — which is the single failure mode this whole section exists to prevent.

Either option is fine:

1. Add the one-line comment above createActionInputViews naming the TS mirror. Note this puts a .java file in the diff — worth confirming against ADR-0013 whether a comment-only Java change still keeps the PR on the fast CI path.
2. Drop the claim from this doc and file a follow-up issue, so the drift risk has a ticket instead of a sentence.

For the record, I verified the mirror itself is faithful line-for-line against WorkflowResource.java:902-928, including rule order and the moveable path exclusion.

[adrianjm-dotCMS]

Confirmed: 0 .java files in the diff, the claim was false.

Took your option 2, for the reason you flagged yourself: a comment-only Java change puts a .java file in the diff and, per ADR-0013, moves the PR off the fast CI path for no behavioral gain. The doc now states the actual situation — the pointer exists only TS→Java — and why it isn't added here. The follow-up issue is still owed; I'll open it if you agree.

[nicobytes]

Consider — moveable is a second behavior change, and it is not documented.

The description calls out pushPublish as the intentional behavior change, but the derivation emits moveable too, and that is a second one. A first-step action with a Move actionlet and no preset path previously fired directly on unsaved content; it now flows through mergeCommentAndAssign into a commentAndAssign step and opens the wizard asking the author for a path.

That is the right outcome — it is parity with saved content, which already gets moveable from getByInode. I also checked the failure mode it could have had: setWizardInput() returning null would have made the action a silent no-op, since openWizard in the component passes the result straight to dotWizardService.open() without the if (wizardInput) guard that DotWorkflowEventHandlerService.openWizard has. It does not happen — isValidActionInput includes MOVEABLE, so a moveable-only action still yields one step — but that is a narrow miss worth knowing about.

Two asks: mention it in the description next to the Push Publish note so QA exercises it, and add a test. The closest existing one asserts the derivation, not that a moveable-only action reaches openWizard.

[adrianjm-dotCMS]

Both asks done: a test that a moveable-only action reaches openWizard, and the PR section went from "Push Publish" to "Intentional behavior changes", with a paragraph on moveable noting it has no environment gate and therefore cannot block a save.

Thanks for the setWizardInput() → null detail — I wasn't aware of that asymmetry between the component's openWizard and the one in DotWorkflowEventHandlerService.

[nicobytes]

Consider — three maps where one says the same thing.

.pipe(map((res) => deriveInputsOnEach(res?.entity ?? [])));

Same behavior, one operator instead of three, two fewer intermediate emissions per call, and ?? [] reads more directly than map((res) => res || []). Applies identically to getWorkFlowActions (line 152).

[adrianjm-dotCMS]

Applied as suggested, both methods.

[nicobytes]

Nit — three lines explaining why an optional chain isn't there. The deriveActionInputs body one line down already makes the requirement obvious, and a comment about absent code is pure carrying cost: it has to be re-read by everyone and re-verified by anyone who touches the guard.

[adrianjm-dotCMS]

Removed.

[nicobytes]

Nit — three lines to say a default is belt-and-braces. Two things make it weaker than it looks: actionInputs is a required (non-optional) field on DotCMSWorkflowAction, so = [] was never the contract to begin with; and "the service now guarantees" reads as stale the day a fourth caller appears that does not go through DotWorkflowsActionsService. One line, or none.

[adrianjm-dotCMS]

Removed entirely rather than shortened. Your point that actionInputs is non-optional settles it: the default was never the contract, so the comment was defending something that didn't exist.

[nicobytes]

Nit — rawCommentableAction is the same 13-field literal as buildRawAction in dot-workflows-actions.utils.spec.ts, in the same folder. Export the builder from one place — or from @dotcms/utils-testing, where the other workflow mocks already live — so the two specs cannot drift apart on what "the raw payload" looks like.

[adrianjm-dotCMS]

Solved by createFakeWorkflowAction in @dotcms/utils-testing. One definition of "the raw payload", shared by both specs.

[nicobytes]

Nit — (action: object) accepts anything and checks no keys. Partial<DotCMSWorkflowAction> types it properly and would catch a typo in a fixture at compile time instead of in a confusing assertion failure.

[adrianjm-dotCMS]

Now Partial<DotCMSWorkflowAction>.

[nicobytes]

Nit — [plan.md](./plan.md) is broken twice over: plan.md is not committed, and from contracts/ this relative path resolves to contracts/plan.md rather than the sibling directory. Same line in data-model.md:3. Either drop the links or commit the plan.

[adrianjm-dotCMS]

Verified broken twice over, as you said: plan.md is gitignored (.gitignore:229) and from contracts/ the path resolved to contracts/plan.md. Replaced with links to spec.md and data-model.md, which are tracked.

[adrianjm-dotCMS]

All 14 applied in b8e0d8c0f7 — 3 Required, 4 Consider, 7 Nits.

I diverged from the suggestion on two, both explained in their threads: Required 3 is resolved by withdrawing the claim rather than adding the Java comment, for the ADR-0013 reason you raised yourself; and Required 1 is resolved in the root beforeEach rather than per-mock, because restoreMocks doesn't apply to mockProvider's jest.fn()s.

Your cross-cutting note landed. Three of your findings were comments that had already drifted before merge, and one of those I had lengthened last round precisely to fix a different inaccuracy. Four comments deleted; the ones left carry real intent — the Java mirror pointer, the #36883 framing, and why getByInode is left alone.

Verification: data-access 873 tests, component spec 98, tsc clean in the touched files (the 7 pre-existing baseline errors unchanged), format clean. Leaving the threads unresolved for you to close.

[oidacra]

DotCMSWorkflowAction.actionInputs is declared non-optional in dot-workflow-action.model.ts:26, so from TypeScript's point of view this Array.isArray check can never be false — yet the function exists precisely because initialactions and defaultactions do omit the key.

Scope: the guard here is added by this PR; the model declaration is pre-existing.

That leaves the model type describing a shape those two endpoints never send, and the mismatch is invisible outside this file: anything writing action.actionInputs.length without a guard compiles clean and throws at runtime on a default-actions payload.

Would it be worth making the type carry that distinction rather than leaving it to this one runtime check? A separate WorkflowDefaultActionView type for the un-derived response would express which endpoints omit the field; narrowing this parameter, or marking the field optional on the model, would each work too — the last one surfaces it to every consumer rather than only here.

Or is the intent that the derivation makes the omission un-observable everywhere downstream, so a non-optional field is the accurate description of what consumers now actually receive?

[adrianjm-dotCMS]

Confirmed — dot-workflow-action.model.ts:26 declares it non-optional, so from the type system's point of view this guard is unreachable, while at runtime it is the line doing the work.

Your last reading is the intent: after the derivation, "always present" describes what consumers actually receive. But it isn't fully true, and the exception is worth naming — getByWorkflows is not derived. Its pipe is still map((x) => x?.entity), and POST /schemes/actions/{systemAction} returns raw List<WorkflowAction> (WorkflowHelper#findActions), so that path hands back actions with no actionInputs while the type says otherwise. Inert today because its only consumer, dot-workflows-actions-selector-field.service, selects actions rather than firing them — but the type does lie there.

Keeping the guard as-is: the type system believing it unreachable is evidence the type is wrong, not that the check is redundant.

On fixing the type itself — I'd rather not do it here. Marking the field optional surfaces it to every consumer, which is the honest option but forces guards across code this PR doesn't touch; a separate WorkflowDefaultActionView type is cleaner but is a model refactor inside a defect fix. Happy to open a follow-up issue with the two options and the getByWorkflows detail, unless you'd rather it land here.