Skip to content

fix(dot-wizard): isolate output stream so cancelled wizards don't fire workflow actions#35423

Merged
oidacra merged 2 commits intomainfrom
issue-35422-dot-wizard-cancel-fires-workflow
Apr 22, 2026
Merged

fix(dot-wizard): isolate output stream so cancelled wizards don't fire workflow actions#35423
oidacra merged 2 commits intomainfrom
issue-35422-dot-wizard-cancel-fires-workflow

Conversation

@oidacra
Copy link
Copy Markdown
Member

@oidacra oidacra commented Apr 22, 2026

Parent issue

Closes #35422

Summary

  • DotWizardService used a single shared Subject across every open() invocation. When the user dismissed the wizard without submitting, the consumer's subscription stayed alive and fired on the next submission — causing workflow actions (e.g. Send for Review, Push Publish, comment & assign) to be applied to unrelated content items the user had never touched.
  • Each open() now creates a fresh Subject and returns an isolated stream. A new cancel() method completes the current stream without emitting; output$() emits and completes. The wizard component calls cancel() on close so take(1) consumers unsubscribe cleanly.
  • No consumer of DotWizardService required changes — DotWorkflowEventHandlerService, DotEmaWorkflowActionsService, and any other caller continue to work as before.

Files changed

  • core-web/libs/data-access/src/lib/dot-wizard/dot-wizard.service.ts — per-invocation Subject, new cancel(), complete-on-emit semantics
  • core-web/libs/data-access/src/lib/dot-wizard/dot-wizard.service.spec.ts — new specs for cancel, cross-invocation isolation, and complete-after-output
  • core-web/apps/dotcms-ui/src/app/view/components/_common/dot-wizard/dot-wizard.component.tsclose() notifies the service via cancel()

Test plan

  • yarn nx test data-access --testPathPattern=dot-wizard — passes
  • yarn nx test dotcms-ui --testPathPattern=dot-wizard.component — passes
  • yarn nx lint data-access + yarn nx lint dotcms-ui — no new errors
  • yarn nx format:write applied to changed files
  • Manual reproduction on /dotAdmin/#/c/c_Blog-Entries:
    • Right-click Row A → Send for ReviewX to cancel
    • Right-click Row B → Send for Review → fill form → Send
    • Before: both rows move to QA · After: only Row B moves to QA ✅

Links

This PR fixes: #35422

…d subscriptions from firing workflow actions (#35422)

DotWizardService shared a single output Subject across every open() call.
When the user dismissed the wizard without submitting, the consumer's
subscription stayed alive, and the next submission fired every leftover
subscription in addition to the current one — causing workflow actions
to be applied to unrelated content items the user never touched.

Create a fresh Subject per open() call and expose a cancel() method that
completes it without emitting. The wizard component calls cancel() on
close so pending subscriptions unsubscribe cleanly. No consumer needs to
change.
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

Claude finished @oidacra's task in 1m 24s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze and post review

The fix is correct and the test coverage is thorough. A few things worth a second look:

1. Double-cancel on the submit path — comment claim untested at component level

sendValue() calls output$() (which nullifies currentOutput) then immediately calls close() (which calls cancel()). The cancel() is a safe no-op because currentOutput is already null, and the comment in close() explains this. However, there's no component-level test that verifies cancel is called zero additional times (i.e., not double-counted) when a wizard is submitted. The existing test at line 305 of dot-wizard.component.spec.ts only exercises the cancel-on-dismiss path. If the idempotency claim in the comment is load-bearing, a matching spec would lock it in.

Fix this →

2. wasOpen guard uses $data() as proxy for "stream is live"

In close(), wasOpen is derived from !!this.$data(). This means if open() on the service is called but the component's showDialog$ subscription hasn't run yet (race in theory, not in practice given synchronous subscription in the constructor), wasOpen would be false and cancel() would be skipped, leaking the stream. In the current synchronous setup this is harmless, but the guard is slightly fragile — keying off service.currentOutput !== null would be more direct (though that field is private).

3. as unknown as Observable<T> cast in open()

// dot-wizard.service.ts:49
const output$ = this.currentOutput.asObservable() as unknown as Observable<T>;

WizardOutput is { [key: string]: string | string[] } but the default for T is { [key: string]: string } — the string[] possibility is silently dropped. This is pre-existing, not introduced here, but now it's in a more visible spot. Worth a follow-up to either widen T's default or align WizardOutput with actual usage.

4. input Subject is never completed

The input: Subject<DotWizardInput> lives for the lifetime of the providedIn: 'root' service and is never completed. Fine for a singleton, but if the component ever gets destroyed and recreated, the showDialog$ subscription in the constructor uses takeUntilDestroyed, so there's no leak there. No action needed, just confirming it's intentional.


The core bug fix — per-invocation Subject, cancel(), complete-after-emit — is the right design. No blocking issues.

…, open()->open() spec, idempotency comment

- Component spec: assert close() invokes DotWizardService.cancel() — locks in the visibleChange -> close -> cancel wiring that fixes the reported bug at the integration layer.
- Service spec: cover open() called twice without cancel in between — exercises the defensive currentOutput?.complete() inside open().
- Document in close() that cancel() is a safe no-op on the submit path (sendValue() nullifies currentOutput first), so future changes to output$ ordering don't silently enable a double-emit.
@oidacra oidacra added this pull request to the merge queue Apr 22, 2026
Merged via the queue into main with commit 5d1b9ed Apr 22, 2026
29 checks passed
@oidacra oidacra deleted the issue-35422-dot-wizard-cancel-fires-workflow branch April 22, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

dot-wizard fires workflow action from a previously cancelled wizard

2 participants