Comfy-Org / Comfy-Org/ComfyUI_frontend

Consolidate workspace-transition sequence into one owned state machine (follow-up to #14290)

Open
#14,300 3 comments 1 reaction 1 assignee Assigned to @dante01yoon View on GitHub
area:auth area:workspace-management refactor
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Context

Follow-up from review on #14290 (comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/14290#pullrequestreview-4813556736). That PR patches a real cross-workspace data leak, but the fix is call-order convention (5 hand-copied statement pairs, verified only by `mock.invocationCallOrder` assertions) rather than a structurally-enforced sequence. This issue proposes the durable shape.

Related: #13970 (harden session-cookie recovery after failed account switch) is the same "deliberately out of scope for the focused isolation fix" pattern on the same PR lineage (#13832 → #14266 → #14290) — both are follow-ups the original account-switch-isolation work deferred. Worth landing together or at least cross-referencing, since both touch the same transition boundary (what happens to in-flight state when the active workspace/account changes).

## Problem

Today, "switch the active workspace" is not one function — it's a sequence re-implemented at 5+ call sites in `teamWorkspaceStore.ts` (`switchWorkspace`, `createWorkspace`, `deleteWorkspace`, `leaveWorkspace`, `forgetRevokedActiveWorkspace`), plus a 6th, inconsistent variant in `initialize()`'s fallback branch that's missing steps the others have (see PR #14290 review, finding C1). The correctness of every one of these call sites rests on:

1. Two statements (`prepareWorkflowWorkspaceTransition()` then `workspaceAuthStore.clearWorkspaceContext()`) staying in the order a human wrote them, in a file (`teamWorkspaceStore.ts`) that doesn't import or reference the module (`storageIO.ts`) whose internal state they're sequencing against.
2. A module-level mutable boolean (`workflowWritesBlocked` in `storageIO.ts`) that is set to `true` at write-time and reset to `false` **nowhere in the codebase** — it's only "safe" because every write site happens to be followed by `window.location.reload()`, except `logout()`, which isn't guaranteed to reload.
3. `storageAvailable` (meant to be a sticky, permanent quota-exceeded flag) and `workflowWritesBlocked` (meant to be transient, per-transition) being ANDed together as two independently-mutable module `let`s with no shared type — so a workspace-switch reload can silently un-degrade a real permanent storage failure.

None of this is enforced by the compiler. It's enforced by test assertions on which mock was called in which order (`teamWorkspaceStore.test.ts`'s `expectCleanupBeforeContextAndReload()`), which is exactly the anti-pattern Christian flagged in the Slack review this rubric is drawn from ("[the coachmark] lifecycle exists implicitly in reactive side effects rather than one explicit command or state machine... state transitions are spread across unrelated callbacks. No single function owns the transition rules.").

## Proposed ideal state

Apply the same principles: **one command owns the entire causal sequence; the sequence's progress is one discriminated-union state, not several independent booleans; effects are reserved for syncing with external systems (storage, network), never for driving the transition itself.**

### 1. Replace the 3 independent module-level booleans with one discriminated union

```ts
// storageIO.ts
type WorkflowStorageState =
| { status: 'available' }
| { status: 'quota-exceeded' } // was storageAvailable = false — sticky, permanent
| { status: 'transitioning' } // was workflowWritesBlocked = true — transient, must resolve

let workflowStorageState: WorkflowStorageState = { status: 'available' }
```

`isStorageAvailable()` derives from one authoritative fact instead of ANDing two flags that were never designed together. A `'transitioning'` state can only be entered and exited by the one command below — not by any of the 6+ call sites independently flipping a boolean.

### 2. Fold the entire transition into one owned async command

```ts
// A new module, e.g. platform/workspace/commands/transitionWorkspace.ts
async function transitionWorkspace(
next: { workspaceId: string } | { fallback: true }
): Promise {
// 1. flush — synchronous, must complete before step 3 removes the key
// it reads (sessionStorage CURRENT_WORKSPACE)
flushPendingWorkflowPersistence()

// 2. block — enter 'transitioning', not a bare boolean flip
setWorkflowStorageState({ status: 'transitioning' })

// 3. clear transient restore pointers (not scoped drafts)
clearWorkflowRestoreState()

// 4. clear auth context (this is what actually removes CURRENT_WORKSPACE)
workspaceAuthStore.clearWorkspaceContext()

// 5. persist new id / resolve target, mint new context if needed
if ('workspaceId' in next) {
setLastWorkspaceId(next.workspaceId)
await workspaceAuthStore.switchWorkspace(next.workspaceId) // if no reload
}

// 6. either reload (tears down the module, resetting state naturally)
// or, for the no-reload fallback path, explicitly resume:
if (shouldReload) {
window.location.reload()
} else {
setWorkflowStorageState({ status: 'available' }) // <- doesn't exist today, anywhere
}
}
```

Every one of the 5+ call sites (plus the `initialize()` fallback, plus the auth-store-internal `clearWorkspaceContext()` callers in `scheduleClearAtExpiry`/`scheduleTokenRefreshRetry`/`handleRecoveryFailure`) calls this one function instead of re-typing the sequence. Ordering becomes a property of one function body, reviewable in one place, instead of a convention repeated 8+ times across two files that don't import each other.

This also closes finding C1 from the #14290 review for free — the `initialize()` fallback becomes just another caller of the same safe command, instead of a hand-rolled variant that's missing steps.

### 3. Guard the flush loop; make failure mode explicit

`flushPendingWorkflowPersistence()` currently runs registered callbacks with no error handling — a non-quota `DOMException` during flush can abort the transition before `'transitioning'` is ever entered, in the 2 call sites (`forgetRevokedActiveWorkspace`, `leaveWorkspace`) that have no surrounding try/catch. Wrap per-callback, log-and-continue, so one bad write can't compromise the isolation guarantee for everyone else's writes.

### 4. Test the real sequence, not mock call order

Add one integration-style test with `storageIO` and `workspaceAuthStore`'s storage internals **unmocked** — real `sessionStorage`/`localStorage`, real `registerWorkflowPersistenceFlush`, asserting actual key contents after a real `transitionWorkspace()` call (mock only the network layer and `window.location.reload`). Today `teamWorkspaceStore.test.ts` fully mocks `storageIO`, so its ordering assertions prove two mocks were called in order — not that the real flush flushes or the real clear clears the right key.

## Acceptance criteria

- [ ] `workflowWritesBlocked`, `storageAvailable`, and the flush-registry no-op-if-empty case are unified under one discriminated union with one owner.
- [ ] All transition call sites (`switchWorkspace`, `createWorkspace`, `deleteWorkspace`, `leaveWorkspace`, `forgetRevokedActiveWorkspace`, `initialize()` fallback, and the `workspaceAuthStore.ts`-internal `clearWorkspaceContext()` callers) go through one exported command.
- [ ] The no-reload fallback path (`initialize()`) has an explicit, reachable "resume writes" transition — not a permanent block with no reset.
- [ ] Flush loop failures can't skip the block-and-clear steps; the two currently-unguarded call sites get try/catch.
- [ ] At least one test exercises the real (unmocked) end-to-end sequence and asserts on actual storage contents, not mock call order.

## References

- PR #14290 review (full findings): https://github.com/Comfy-Org/ComfyUI_frontend/pull/14290#pullrequestreview-4813556736
- Related follow-up in the same lineage: #13970 (harden session-cookie recovery after failed account switch)
- PR family: #13832 → #14266 → #14290

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.