From ad97d21c2798df3394fec1b57eac11f3b388b593 Mon Sep 17 00:00:00 2001 From: waleed Date: Wed, 24 Jun 2026 08:46:03 -0700 Subject: [PATCH] refactor(stores): model execution and workflow-diff state as status enums --- apps/sim/hooks/use-undo-redo.ts | 21 +- apps/sim/stores/execution/store.test.ts | 192 +++++++++++++++-- apps/sim/stores/execution/store.ts | 31 ++- apps/sim/stores/execution/types.ts | 77 ++++++- apps/sim/stores/workflow-diff/store.test.ts | 228 ++++++++++++++++++++ apps/sim/stores/workflow-diff/store.ts | 37 ++-- apps/sim/stores/workflow-diff/types.ts | 50 +++++ apps/sim/vitest.setup.ts | 3 + 8 files changed, 569 insertions(+), 70 deletions(-) create mode 100644 apps/sim/stores/workflow-diff/store.test.ts diff --git a/apps/sim/hooks/use-undo-redo.ts b/apps/sim/hooks/use-undo-redo.ts index 025c087de0e..22dfdaa6e0e 100644 --- a/apps/sim/hooks/use-undo-redo.ts +++ b/apps/sim/hooks/use-undo-redo.ts @@ -41,6 +41,7 @@ import { type UpdateParentOperation, useUndoRedoStore, } from '@/stores/undo-redo' +import { deriveDiffFlags } from '@/stores/workflow-diff/types' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' import { useSubBlockStore } from '@/stores/workflows/subblock/store' import { useWorkflowStore } from '@/stores/workflows/workflow/store' @@ -1234,9 +1235,7 @@ export function useUndoRedo() { // Restore diff state with baseline (local UI only) diffStore._batchedStateUpdate({ - hasActiveDiff: true, - isShowingDiff: true, - isDiffReady: true, + ...deriveDiffFlags('showing'), baselineWorkflow: originalBaseline || null, baselineWorkflowId: activeWorkflowId, diffAnalysis: diffAnalysis, @@ -1285,9 +1284,7 @@ export function useUndoRedo() { // Restore diff state with baseline (local UI only) const diffStore = useWorkflowDiffStore.getState() diffStore._batchedStateUpdate({ - hasActiveDiff: true, - isShowingDiff: true, - isDiffReady: true, + ...deriveDiffFlags('showing'), baselineWorkflow: baselineSnapshot || null, baselineWorkflowId: activeWorkflowId, diffAnalysis: diffAnalysis, @@ -1805,9 +1802,7 @@ export function useUndoRedo() { // Restore diff state with original baseline (local UI only) diffStore._batchedStateUpdate({ - hasActiveDiff: true, - isShowingDiff: true, - isDiffReady: true, + ...deriveDiffFlags('showing'), baselineWorkflow: baselineSnapshot, baselineWorkflowId: activeWorkflowId, diffAnalysis: diffAnalysis, @@ -1834,9 +1829,7 @@ export function useUndoRedo() { // Clear diff state FIRST to prevent flash of colors (local UI only) // Use setState directly to ensure synchronous clearing useWorkflowDiffStore.setState({ - hasActiveDiff: false, - isShowingDiff: false, - isDiffReady: false, + ...deriveDiffFlags('none'), baselineWorkflow: null, baselineWorkflowId: null, diffAnalysis: null, @@ -1886,9 +1879,7 @@ export function useUndoRedo() { // Clear diff state FIRST to prevent flash of colors (local UI only) // Use setState directly to ensure synchronous clearing useWorkflowDiffStore.setState({ - hasActiveDiff: false, - isShowingDiff: false, - isDiffReady: false, + ...deriveDiffFlags('none'), baselineWorkflow: null, baselineWorkflowId: null, diffAnalysis: null, diff --git a/apps/sim/stores/execution/store.test.ts b/apps/sim/stores/execution/store.test.ts index 6888894b80b..c0b11fc87ce 100644 --- a/apps/sim/stores/execution/store.test.ts +++ b/apps/sim/stores/execution/store.test.ts @@ -1,4 +1,6 @@ /** + * @vitest-environment node + * * Tests for the per-workflow execution store. * * These tests cover: @@ -7,12 +9,19 @@ * - Execution lifecycle (start/stop clears run path) * - Block and edge run status tracking * - Active block management - * - Debug state management + * - The {@link ExecutionStatus} enum and its derived `isExecuting` / + * `isDebugging` booleans (exhaustive status → flag mapping + transitions) * - Execution snapshot management * - Store reset * - Immutability guarantees * * @remarks + * The store under test transitively imports the workflow registry store, + * which drags in the block registry and emcn icon CSS. To keep this a true + * unit test that loads under the node environment, the registry store is + * mocked to a minimal stub (the store actions never touch it — only the + * convenience hooks do, which are not exercised here). + * * Most tests use `it.concurrent` with unique workflow IDs per test. * Because the store isolates state by workflow ID, concurrent tests * do not interfere with each other. The `reset` and `immutability` @@ -21,17 +30,30 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +vi.mock('@/stores/workflows/registry/store', () => ({ + useWorkflowRegistry: Object.assign( + vi.fn(() => null), + { getState: vi.fn(() => ({ activeWorkflowId: null })) } + ), +})) + vi.unmock('@/stores/execution/store') vi.unmock('@/stores/execution/types') import { useExecutionStore } from '@/stores/execution/store' -import { defaultWorkflowExecutionState, initialState } from '@/stores/execution/types' +import { + defaultWorkflowExecutionState, + deriveExecutionFlags, + type ExecutionStatus, + initialState, +} from '@/stores/execution/types' describe('useExecutionStore', () => { describe('getWorkflowExecution', () => { it.concurrent('should return default state for an unknown workflow', () => { const state = useExecutionStore.getState().getWorkflowExecution('wf-get-default') + expect(state.status).toBe('idle') expect(state.isExecuting).toBe(false) expect(state.isDebugging).toBe(false) expect(state.activeBlockIds.size).toBe(0) @@ -63,22 +85,35 @@ describe('useExecutionStore', () => { }) }) + describe('deriveExecutionFlags', () => { + it.concurrent('maps every status to the documented legacy booleans', () => { + const cases: Array<[ExecutionStatus, boolean, boolean]> = [ + ['idle', false, false], + ['running', true, false], + ['debugging', true, true], + ] + for (const [status, isExecuting, isDebugging] of cases) { + expect(deriveExecutionFlags(status)).toEqual({ isExecuting, isDebugging }) + } + }) + }) + describe('setIsExecuting', () => { - it.concurrent('should set isExecuting to true', () => { + it.concurrent('should set isExecuting to true (status running)', () => { useExecutionStore.getState().setIsExecuting('wf-exec-true', true) - expect(useExecutionStore.getState().getWorkflowExecution('wf-exec-true').isExecuting).toBe( - true - ) + const state = useExecutionStore.getState().getWorkflowExecution('wf-exec-true') + expect(state.isExecuting).toBe(true) + expect(state.status).toBe('running') }) - it.concurrent('should set isExecuting to false', () => { + it.concurrent('should set isExecuting to false (status idle)', () => { useExecutionStore.getState().setIsExecuting('wf-exec-false', true) useExecutionStore.getState().setIsExecuting('wf-exec-false', false) - expect(useExecutionStore.getState().getWorkflowExecution('wf-exec-false').isExecuting).toBe( - false - ) + const state = useExecutionStore.getState().getWorkflowExecution('wf-exec-false') + expect(state.isExecuting).toBe(false) + expect(state.status).toBe('idle') }) it.concurrent('should clear lastRunPath and lastRunEdges when starting execution', () => { @@ -107,6 +142,131 @@ describe('useExecutionStore', () => { expect(state.isExecuting).toBe(false) expect(state.lastRunPath.get('block-1')).toBe('success') }) + + it.concurrent('starting a debug run then setIsExecuting(true) clears the run path', () => { + const wf = 'wf-exec-debug-start-clears' + useExecutionStore.getState().setIsExecuting(wf, true) + useExecutionStore.getState().setIsDebugging(wf, true) + useExecutionStore.getState().setBlockRunStatus(wf, 'block-1', 'success') + + useExecutionStore.getState().setIsExecuting(wf, true) + + const state = useExecutionStore.getState().getWorkflowExecution(wf) + expect(state.status).toBe('debugging') + expect(state.isExecuting).toBe(true) + expect(state.isDebugging).toBe(true) + expect(state.lastRunPath.size).toBe(0) + expect(state.lastRunEdges.size).toBe(0) + }) + }) + + describe('setIsDebugging', () => { + it.concurrent('should toggle debug mode', () => { + const wf = 'wf-debug-toggle' + useExecutionStore.getState().setIsDebugging(wf, true) + + expect(useExecutionStore.getState().getWorkflowExecution(wf).isDebugging).toBe(true) + expect(useExecutionStore.getState().getWorkflowExecution(wf).isExecuting).toBe(true) + expect(useExecutionStore.getState().getWorkflowExecution(wf).status).toBe('debugging') + + useExecutionStore.getState().setIsDebugging(wf, false) + expect(useExecutionStore.getState().getWorkflowExecution(wf).isDebugging).toBe(false) + expect(useExecutionStore.getState().getWorkflowExecution(wf).isExecuting).toBe(true) + expect(useExecutionStore.getState().getWorkflowExecution(wf).status).toBe('running') + }) + + it.concurrent('setIsDebugging(false) while idle is a no-op (stays idle)', () => { + const wf = 'wf-debug-false-idle' + useExecutionStore.getState().setIsDebugging(wf, false) + expect(useExecutionStore.getState().getWorkflowExecution(wf).status).toBe('idle') + expect(useExecutionStore.getState().getWorkflowExecution(wf).isExecuting).toBe(false) + }) + + it.concurrent('setIsDebugging(false) while running keeps running', () => { + const wf = 'wf-debug-false-running' + useExecutionStore.getState().setIsExecuting(wf, true) + useExecutionStore.getState().setIsDebugging(wf, false) + expect(useExecutionStore.getState().getWorkflowExecution(wf).status).toBe('running') + expect(useExecutionStore.getState().getWorkflowExecution(wf).isExecuting).toBe(true) + }) + + it.concurrent('does not clear the run path when entering debug mode', () => { + const wf = 'wf-debug-keeps-path' + useExecutionStore.getState().setBlockRunStatus(wf, 'block-1', 'success') + useExecutionStore.getState().setIsDebugging(wf, true) + expect(useExecutionStore.getState().getWorkflowExecution(wf).lastRunPath.get('block-1')).toBe( + 'success' + ) + }) + }) + + describe('status enum', () => { + it.concurrent('idle derives both flags false', () => { + const wf = 'wf-status-idle' + const state = useExecutionStore.getState().getWorkflowExecution(wf) + expect(state.status).toBe('idle') + expect(state.isExecuting).toBe(false) + expect(state.isDebugging).toBe(false) + }) + + it.concurrent('running derives isExecuting only', () => { + const wf = 'wf-status-running' + useExecutionStore.getState().setStatus(wf, 'running') + const state = useExecutionStore.getState().getWorkflowExecution(wf) + expect(state.status).toBe('running') + expect(state.isExecuting).toBe(true) + expect(state.isDebugging).toBe(false) + }) + + it.concurrent('debugging derives both flags true', () => { + const wf = 'wf-status-debugging' + useExecutionStore.getState().setStatus(wf, 'debugging') + const state = useExecutionStore.getState().getWorkflowExecution(wf) + expect(state.status).toBe('debugging') + expect(state.isExecuting).toBe(true) + expect(state.isDebugging).toBe(true) + }) + + it.concurrent('setStatus preserves the run path unless clearRunPath is passed', () => { + const wf = 'wf-status-path-rules' + useExecutionStore.getState().setStatus(wf, 'debugging') + useExecutionStore.getState().setBlockRunStatus(wf, 'block-1', 'success') + expect(useExecutionStore.getState().getWorkflowExecution(wf).lastRunPath.size).toBe(1) + + useExecutionStore.getState().setStatus(wf, 'running') + expect(useExecutionStore.getState().getWorkflowExecution(wf).lastRunPath.size).toBe(1) + + useExecutionStore.getState().setStatus(wf, 'running', { clearRunPath: true }) + expect(useExecutionStore.getState().getWorkflowExecution(wf).lastRunPath.size).toBe(0) + }) + + it.concurrent('the derived booleans always agree with the stored status', () => { + const wf = 'wf-status-no-drift' + for (const status of ['idle', 'running', 'debugging', 'idle'] as const) { + useExecutionStore.getState().setStatus(wf, status) + const state = useExecutionStore.getState().getWorkflowExecution(wf) + expect({ isExecuting: state.isExecuting, isDebugging: state.isDebugging }).toEqual( + deriveExecutionFlags(status) + ) + } + }) + + it.concurrent('setIsExecuting(true) preserves an active debug session', () => { + const wf = 'wf-status-debug-preserve' + useExecutionStore.getState().setStatus(wf, 'debugging') + useExecutionStore.getState().setIsExecuting(wf, true) + expect(useExecutionStore.getState().getWorkflowExecution(wf).status).toBe('debugging') + }) + + it.concurrent('setIsExecuting(false) returns to idle from any mode', () => { + const wf = 'wf-status-stop' + useExecutionStore.getState().setStatus(wf, 'debugging') + useExecutionStore.getState().setIsExecuting(wf, false) + const state = useExecutionStore.getState().getWorkflowExecution(wf) + expect(state.status).toBe('idle') + expect(state.isExecuting).toBe(false) + expect(state.isDebugging).toBe(false) + }) }) describe('setActiveBlocks', () => { @@ -151,18 +311,6 @@ describe('useExecutionStore', () => { }) }) - describe('setIsDebugging', () => { - it.concurrent('should toggle debug mode', () => { - const wf = 'wf-debug-toggle' - useExecutionStore.getState().setIsDebugging(wf, true) - - expect(useExecutionStore.getState().getWorkflowExecution(wf).isDebugging).toBe(true) - - useExecutionStore.getState().setIsDebugging(wf, false) - expect(useExecutionStore.getState().getWorkflowExecution(wf).isDebugging).toBe(false) - }) - }) - describe('setExecutor', () => { it.concurrent('should store and clear executor', () => { const wf = 'wf-executor' diff --git a/apps/sim/stores/execution/store.ts b/apps/sim/stores/execution/store.ts index 8d9e2827f34..5efa313f021 100644 --- a/apps/sim/stores/execution/store.ts +++ b/apps/sim/stores/execution/store.ts @@ -3,9 +3,11 @@ import { useWorkflowRegistry } from '@/stores/workflows/registry/store' import { type BlockRunStatus, defaultWorkflowExecutionState, + deriveExecutionFlags, type EdgeRunStatus, type ExecutionActions, type ExecutionState, + type ExecutionStatus, initialState, type WorkflowExecutionState, } from './types' @@ -78,9 +80,12 @@ export const useExecutionStore = create()((se }) }, - setIsExecuting: (workflowId, isExecuting) => { - const patch: Partial = { isExecuting } - if (isExecuting) { + setStatus: (workflowId, status, options) => { + const patch: Partial = { + status, + ...deriveExecutionFlags(status), + } + if (options?.clearRunPath) { patch.lastRunPath = new Map() patch.lastRunEdges = new Map() } @@ -89,10 +94,24 @@ export const useExecutionStore = create()((se }) }, + setIsExecuting: (workflowId, isExecuting) => { + const current = getOrCreate(get().workflowExecutions, workflowId) + const nextStatus: ExecutionStatus = isExecuting + ? current.status === 'debugging' + ? 'debugging' + : 'running' + : 'idle' + get().setStatus(workflowId, nextStatus, { clearRunPath: isExecuting }) + }, + setIsDebugging: (workflowId, isDebugging) => { - set({ - workflowExecutions: updatedMap(get().workflowExecutions, workflowId, { isDebugging }), - }) + const current = getOrCreate(get().workflowExecutions, workflowId) + const nextStatus: ExecutionStatus = isDebugging + ? 'debugging' + : current.status === 'debugging' + ? 'running' + : current.status + get().setStatus(workflowId, nextStatus) }, setExecutor: (workflowId, executor) => { diff --git a/apps/sim/stores/execution/types.ts b/apps/sim/stores/execution/types.ts index b36ea43a190..bb48c9d45a2 100644 --- a/apps/sim/stores/execution/types.ts +++ b/apps/sim/stores/execution/types.ts @@ -12,6 +12,22 @@ export type BlockRunStatus = 'success' | 'error' */ export type EdgeRunStatus = 'success' | 'error' +/** + * The mutually-exclusive execution mode of a single workflow. + * + * @remarks + * This is the single source of truth for whether a workflow is running. + * The legacy `isExecuting` / `isDebugging` booleans are derived from it + * via {@link deriveExecutionFlags} so illegal combinations — such as + * "debugging while not executing" — are unrepresentable. + * + * - `idle` — not running. + * - `running` — executing normally (derives `isExecuting`). + * - `debugging` — executing in step-by-step debug mode (derives both + * `isExecuting` and `isDebugging`). + */ +export type ExecutionStatus = 'idle' | 'running' | 'debugging' + /** * Execution state scoped to a single workflow. * @@ -19,9 +35,11 @@ export type EdgeRunStatus = 'success' | 'error' * do not interfere with one another. */ export interface WorkflowExecutionState { - /** Whether this workflow is currently executing */ + /** Mutually-exclusive execution mode; the source of truth for run state */ + status: ExecutionStatus + /** Derived from {@link status}: whether this workflow is currently executing */ isExecuting: boolean - /** Whether this workflow is in step-by-step debug mode */ + /** Derived from {@link status}: whether this workflow is in step-by-step debug mode */ isDebugging: boolean /** Block IDs that are currently running (pulsing in the UI) */ activeBlockIds: Set @@ -39,6 +57,24 @@ export interface WorkflowExecutionState { currentExecutionId: string | null } +/** + * Computes the legacy `isExecuting` / `isDebugging` booleans from a status. + * + * @remarks + * Keeping the derived booleans on the stored state object lets existing + * consumers keep reading `state.isExecuting` / `state.isDebugging` + * unchanged while {@link ExecutionStatus} remains the single source of truth. + */ +export function deriveExecutionFlags(status: ExecutionStatus): { + isExecuting: boolean + isDebugging: boolean +} { + return { + isExecuting: status !== 'idle', + isDebugging: status === 'debugging', + } +} + /** * Default values for a workflow that has never been executed. * @@ -48,8 +84,8 @@ export interface WorkflowExecutionState { * re-renders in Zustand selectors that use `Object.is` equality. */ export const defaultWorkflowExecutionState: WorkflowExecutionState = { - isExecuting: false, - isDebugging: false, + status: 'idle', + ...deriveExecutionFlags('idle'), activeBlockIds: new Set(), pendingBlocks: [], executor: null, @@ -83,9 +119,38 @@ export interface ExecutionActions { getWorkflowExecution: (workflowId: string) => WorkflowExecutionState /** Replaces the set of currently-executing block IDs for a workflow */ setActiveBlocks: (workflowId: string, blockIds: Set) => void - /** Marks a workflow as executing or idle. Starting clears the run path */ + /** + * Sets the {@link ExecutionStatus} for a workflow. + * + * @remarks + * Pass `{ clearRunPath: true }` to also reset `lastRunPath` / `lastRunEdges`. + * Run-path clearing is opt-in: it is owned by + * {@link ExecutionActions.setIsExecuting} (which clears on start), matching + * the legacy behavior where only starting execution wiped the run history. + */ + setStatus: ( + workflowId: string, + status: ExecutionStatus, + options?: { clearRunPath?: boolean } + ) => void + /** + * Marks a workflow as executing or idle. Starting (`true`) clears the run path. + * + * @remarks + * Translates to {@link ExecutionActions.setStatus}: `true` preserves an + * active debug session (`debugging`) and otherwise enters `running`, and + * always clears the run path; `false` returns to `idle` and preserves it. + */ setIsExecuting: (workflowId: string, isExecuting: boolean) => void - /** Toggles debug mode for a workflow */ + /** + * Toggles step-by-step debug mode for a workflow. + * + * @remarks + * Translates to {@link ExecutionActions.setStatus}: `true` enters + * `debugging` (which implies executing); `false` returns to `running` only + * when currently `debugging`, otherwise the status is preserved (e.g. calling + * it while `idle` is a no-op). + */ setIsDebugging: (workflowId: string, isDebugging: boolean) => void /** Sets the list of blocks pending execution during debug stepping */ setPendingBlocks: (workflowId: string, blockIds: string[]) => void diff --git a/apps/sim/stores/workflow-diff/store.test.ts b/apps/sim/stores/workflow-diff/store.test.ts new file mode 100644 index 00000000000..c9bca038048 --- /dev/null +++ b/apps/sim/stores/workflow-diff/store.test.ts @@ -0,0 +1,228 @@ +/** + * @vitest-environment node + * + * Tests for the workflow-diff store's status modeling. + * + * Focus: the {@link WorkflowDiffStatus} enum is the single source of truth and + * the legacy `hasActiveDiff` / `isShowingDiff` / `isDiffReady` booleans are + * derived from it, so contradictory combinations are unrepresentable. We assert + * the exhaustive status → boolean mapping and the status transitions driven by + * the tractable actions (`toggleDiffView`, `clearDiff`, `_batchedStateUpdate`). + * + * @remarks + * The store transitively imports the diff engine, serializer, socket + * operations, and the workflow/registry stores, all of which drag in the block + * registry and emcn icon CSS. Every such dependency is mocked so the suite + * loads under the node environment and exercises only the store + its types. + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { applyWorkflowStateToStores } = vi.hoisted(() => ({ + applyWorkflowStateToStores: vi.fn(), +})) + +vi.mock('@sim/logger', () => ({ + createLogger: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }), +})) + +vi.mock('@/lib/workflows/diff', () => ({ + WorkflowDiffEngine: class { + clearDiff = vi.fn() + createDiffFromWorkflowState = vi.fn() + }, + stripWorkflowDiffMarkers: vi.fn((s) => s), +})) + +vi.mock('@/lib/workflows/operations/socket-operations', () => ({ + enqueueReplaceWorkflowState: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock('@/lib/workflows/sanitization/validation', () => ({ + validateWorkflowState: vi.fn(() => ({ valid: true, errors: [], sanitizedState: null })), +})) + +vi.mock('@/serializer', () => ({ + Serializer: class { + serializeWorkflow = vi.fn() + deserializeWorkflow = vi.fn() + }, +})) + +vi.mock('@/stores/workflows/registry/store', () => ({ + useWorkflowRegistry: { getState: vi.fn(() => ({ activeWorkflowId: null })) }, +})) + +vi.mock('@/stores/workflows/utils', () => ({ + mergeSubblockState: vi.fn((blocks) => blocks), +})) + +vi.mock('@/stores/workflows/workflow/store', () => ({ + useWorkflowStore: { + getState: vi.fn(() => ({ + getWorkflowState: vi.fn(() => ({ blocks: {}, edges: [], loops: {}, parallels: {} })), + blocks: {}, + lastSaved: 0, + })), + setState: vi.fn(), + }, +})) + +vi.mock('@/stores/workflow-diff/utils', () => ({ + applyWorkflowStateToStores, + captureBaselineSnapshot: vi.fn(), + cloneWorkflowState: vi.fn((s) => s), + createBatchedUpdater: + (set: (u: Record) => void) => (updates: Record) => + set(updates), + getLatestUserMessageId: vi.fn().mockResolvedValue(null), + persistWorkflowStateToServer: vi.fn().mockResolvedValue(true), + WORKFLOW_DIFF_SETTLED_EVENT: 'workflow-diff-settled', +})) + +import { RESET_DIFF_STATE, useWorkflowDiffStore } from '@/stores/workflow-diff/store' +import { + deriveDiffFlags, + type WorkflowDiffState, + type WorkflowDiffStatus, +} from '@/stores/workflow-diff/types' + +function seedStatus(status: WorkflowDiffStatus): void { + useWorkflowDiffStore.setState(deriveDiffFlags(status)) +} + +describe('useWorkflowDiffStore status modeling', () => { + beforeEach(() => { + vi.clearAllMocks() + useWorkflowDiffStore.setState({ + ...RESET_DIFF_STATE, + pendingExternalUpdates: {}, + remoteUpdateVersions: {}, + reconcilingWorkflows: {}, + reconciliationErrors: {}, + } as Partial) + }) + + describe('deriveDiffFlags', () => { + it('maps every status to the documented legacy booleans', () => { + expect(deriveDiffFlags('none')).toEqual({ + status: 'none', + hasActiveDiff: false, + isShowingDiff: false, + isDiffReady: false, + }) + expect(deriveDiffFlags('staged')).toEqual({ + status: 'staged', + hasActiveDiff: true, + isShowingDiff: false, + isDiffReady: true, + }) + expect(deriveDiffFlags('showing')).toEqual({ + status: 'showing', + hasActiveDiff: true, + isShowingDiff: true, + isDiffReady: true, + }) + }) + + it('keeps hasActiveDiff and isDiffReady in lockstep (legacy invariant)', () => { + for (const status of ['none', 'staged', 'showing'] as const) { + const flags = deriveDiffFlags(status) + expect(flags.hasActiveDiff).toBe(flags.isDiffReady) + } + }) + }) + + describe('initial / reset state', () => { + it('starts in the none-derived state', () => { + const state = useWorkflowDiffStore.getState() + expect(state.status).toBe('none') + expect(state.hasActiveDiff).toBe(false) + expect(state.isShowingDiff).toBe(false) + expect(state.isDiffReady).toBe(false) + }) + + it('RESET_DIFF_STATE carries the none-derived flags and clears diff payload', () => { + expect(RESET_DIFF_STATE.status).toBe('none') + expect(RESET_DIFF_STATE.hasActiveDiff).toBe(false) + expect(RESET_DIFF_STATE.isShowingDiff).toBe(false) + expect(RESET_DIFF_STATE.isDiffReady).toBe(false) + expect(RESET_DIFF_STATE.baselineWorkflow).toBeNull() + expect(RESET_DIFF_STATE.diffAnalysis).toBeNull() + }) + }) + + describe('toggleDiffView', () => { + it('is a guarded no-op when there is no active diff', () => { + seedStatus('none') + useWorkflowDiffStore.getState().toggleDiffView() + expect(useWorkflowDiffStore.getState().status).toBe('none') + }) + + it('toggles showing → staged (hides the proposed changes)', () => { + seedStatus('showing') + useWorkflowDiffStore.getState().toggleDiffView() + + const state = useWorkflowDiffStore.getState() + expect(state.status).toBe('staged') + expect(state.hasActiveDiff).toBe(true) + expect(state.isDiffReady).toBe(true) + expect(state.isShowingDiff).toBe(false) + }) + + it('toggles staged → showing (reveals the proposed changes)', () => { + seedStatus('staged') + useWorkflowDiffStore.getState().toggleDiffView() + + const state = useWorkflowDiffStore.getState() + expect(state.status).toBe('showing') + expect(state.isShowingDiff).toBe(true) + }) + }) + + describe('clearDiff', () => { + it('returns the store to the none status', () => { + seedStatus('showing') + useWorkflowDiffStore.getState().clearDiff({ restoreBaseline: false }) + + const state = useWorkflowDiffStore.getState() + expect(state.status).toBe('none') + expect(state.hasActiveDiff).toBe(false) + expect(state.isShowingDiff).toBe(false) + expect(state.isDiffReady).toBe(false) + }) + }) + + describe('_batchedStateUpdate (undo/redo writer)', () => { + it('restores the showing status via deriveDiffFlags', () => { + seedStatus('none') + useWorkflowDiffStore.getState()._batchedStateUpdate({ + ...deriveDiffFlags('showing'), + baselineWorkflow: null, + baselineWorkflowId: 'wf-1', + }) + + const state = useWorkflowDiffStore.getState() + expect(state.status).toBe('showing') + expect(state.hasActiveDiff).toBe(true) + expect(state.isShowingDiff).toBe(true) + expect(state.isDiffReady).toBe(true) + }) + + it('the derived booleans always agree with the stored status', () => { + for (const status of ['none', 'staged', 'showing', 'none'] as const) { + seedStatus(status) + const state = useWorkflowDiffStore.getState() + expect({ + hasActiveDiff: state.hasActiveDiff, + isShowingDiff: state.isShowingDiff, + isDiffReady: state.isDiffReady, + }).toEqual({ + hasActiveDiff: deriveDiffFlags(status).hasActiveDiff, + isShowingDiff: deriveDiffFlags(status).isShowingDiff, + isDiffReady: deriveDiffFlags(status).isDiffReady, + }) + } + }) + }) +}) diff --git a/apps/sim/stores/workflow-diff/store.ts b/apps/sim/stores/workflow-diff/store.ts index 38d08ad3b97..ec46c2192a3 100644 --- a/apps/sim/stores/workflow-diff/store.ts +++ b/apps/sim/stores/workflow-diff/store.ts @@ -8,7 +8,7 @@ import { Serializer } from '@/serializer' import { useWorkflowRegistry } from '../workflows/registry/store' import { mergeSubblockState } from '../workflows/utils' import { useWorkflowStore } from '../workflows/workflow/store' -import type { WorkflowDiffActions, WorkflowDiffState } from './types' +import { deriveDiffFlags, type WorkflowDiffActions, type WorkflowDiffState } from './types' import { applyWorkflowStateToStores, captureBaselineSnapshot, @@ -21,17 +21,20 @@ import { const logger = createLogger('WorkflowDiffStore') const diffEngine = new WorkflowDiffEngine() -const RESET_DIFF_STATE = { - hasActiveDiff: false, - isShowingDiff: false, - isDiffReady: false, + +/** + * Canonical state patch that clears the diff overlay back to `none`: the + * none-derived flags plus a wipe of all diff payload fields. + */ +export const RESET_DIFF_STATE = { + ...deriveDiffFlags('none'), baselineWorkflow: null, baselineWorkflowId: null, diffAnalysis: null, diffMetadata: null, diffError: null, _triggerMessageId: null, -} +} as const /** * Detects when a diff contains no meaningful changes. @@ -70,9 +73,7 @@ export const useWorkflowDiffStore = create { - const { hasActiveDiff, isDiffReady, isShowingDiff } = get() - if (!hasActiveDiff) { - logger.warn('Cannot toggle diff view without active diff') - return - } - if (!isDiffReady) { - logger.warn('Cannot toggle diff view before diff is ready') + const { status } = get() + if (status === 'none') { + logger.warn('Cannot toggle diff view without an active, ready diff') return } - batchedUpdate({ isShowingDiff: !isShowingDiff }) + batchedUpdate(deriveDiffFlags(status === 'showing' ? 'staged' : 'showing')) }, acceptChanges: async (options) => { diff --git a/apps/sim/stores/workflow-diff/types.ts b/apps/sim/stores/workflow-diff/types.ts index 6c3ea3990a1..d4e05e68760 100644 --- a/apps/sim/stores/workflow-diff/types.ts +++ b/apps/sim/stores/workflow-diff/types.ts @@ -1,9 +1,31 @@ import type { DiffAnalysis, WorkflowDiff } from '@/lib/workflows/diff' import type { WorkflowState } from '../workflows/workflow/types' +/** + * The lifecycle stage of the workflow diff overlay. + * + * @remarks + * This is the single source of truth for the diff overlay. The legacy + * `hasActiveDiff` / `isShowingDiff` / `isDiffReady` booleans are derived from + * it via {@link deriveDiffFlags}, which makes contradictory combinations — + * such as "showing a diff that has no active diff" — unrepresentable. + * + * - `none` — no diff staged; the canvas shows the live workflow. + * - `staged` — a diff is staged and ready, but the canvas is showing the + * baseline (proposed changes hidden). + * - `showing` — a diff is staged and ready, and the canvas is showing the + * proposed changes with diff markers. + */ +export type WorkflowDiffStatus = 'none' | 'staged' | 'showing' + export interface WorkflowDiffState { + /** Lifecycle stage of the diff overlay; the source of truth for diff flags */ + status: WorkflowDiffStatus + /** Derived from {@link status}: a diff is staged (`staged` or `showing`) */ hasActiveDiff: boolean + /** Derived from {@link status}: the canvas is rendering the proposed changes */ isShowingDiff: boolean + /** Derived from {@link status}: a staged diff is ready to view/toggle */ isDiffReady: boolean baselineWorkflow: WorkflowState | null baselineWorkflowId: string | null @@ -48,3 +70,31 @@ export interface WorkflowDiffActions { setWorkflowReconciliationError: (workflowId: string, error: string | null) => void _batchedStateUpdate: (updates: Partial) => void } + +/** + * The {@link WorkflowDiffStatus} fields shared by `status` and its derived + * booleans. Spread this into a state patch so the source of truth and the + * legacy flags never drift apart. + */ +export type DiffStatusFlags = Pick< + WorkflowDiffState, + 'status' | 'hasActiveDiff' | 'isShowingDiff' | 'isDiffReady' +> + +/** + * Computes the legacy `hasActiveDiff` / `isShowingDiff` / `isDiffReady` + * booleans (plus the `status` itself) from a {@link WorkflowDiffStatus}. + * + * @remarks + * Keeping the derived booleans on the stored state lets existing consumers + * keep reading `state.hasActiveDiff` etc. unchanged while + * {@link WorkflowDiffStatus} remains the single source of truth. + */ +export function deriveDiffFlags(status: WorkflowDiffStatus): DiffStatusFlags { + return { + status, + hasActiveDiff: status !== 'none', + isShowingDiff: status === 'showing', + isDiffReady: status !== 'none', + } +} diff --git a/apps/sim/vitest.setup.ts b/apps/sim/vitest.setup.ts index 92e945dcca0..b9a9ae7d14a 100644 --- a/apps/sim/vitest.setup.ts +++ b/apps/sim/vitest.setup.ts @@ -41,6 +41,7 @@ vi.mock('@/stores/execution/store', () => ({ useExecutionStore: { getState: vi.fn().mockReturnValue({ getWorkflowExecution: vi.fn().mockReturnValue({ + status: 'idle', isExecuting: false, isDebugging: false, activeBlockIds: new Set(), @@ -50,6 +51,7 @@ vi.mock('@/stores/execution/store', () => ({ lastRunPath: new Map(), lastRunEdges: new Map(), }), + setStatus: vi.fn(), setIsExecuting: vi.fn(), setIsDebugging: vi.fn(), setPendingBlocks: vi.fn(), @@ -61,6 +63,7 @@ vi.mock('@/stores/execution/store', () => ({ }), }, useCurrentWorkflowExecution: vi.fn().mockReturnValue({ + status: 'idle', isExecuting: false, isDebugging: false, activeBlockIds: new Set(),