From 5ded723bdd9ee2884a1af3d516cfc56aaac7a63e Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 13:09:19 -0700 Subject: [PATCH 1/7] fix(deploy): resolve subblock values to their configuration on both sides of change detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Focusing a deployed trigger block flipped the deploy button from Live to Update for a change the user never made, and redeploying could not clear it. Deploy materializes every declared `defaultValue` into `webhook.providerConfig` (`getConfigValue`). Opening a trigger block's panel reads that derived artifact back into live state through a non-persisting `setValue`, so the block gains keys the DB draft — which deploy snapshots — does not have. The comparison then reported a difference for a field nobody set. Because deploy snapshots the draft, the next deployment lacked the keys too: a fixpoint. Measured on production: 89 of 89 recent `generic_webhook` deployments were missing `acceptOtherMethods`, and 1,046 of 2,139 active webhooks carry at least one `providerConfig` key their block has no entry for. This is the tenth instance of one failure mode — 3c29476604, 41b68048a5, 066e18ac28, 4f722c6439, 5ece9f9e7e, ff23546f30, 01577a18b4, 3cc9b1ae56 and 88065088bf are all the same shape: two pipelines spelled one configuration differently, found in production, patched with a per-field exception. The fix resolves each subblock to the configuration it represents, from the CURRENT block definition, applied to both sides: absent, `null` and (where a default is declared) `''` all mean "unset", as does a value equal to that default. Adding a defaulted field to a block definition is therefore a no-op for already-deployed workflows rather than a retroactive diff. Comparison-time only, deliberately. Writing defaults into storage answers the same question but cannot be rolled back, and would destroy the "key absent means this state predates the field" signal the subblock-rename migrations rely on. `value()` is not consulted — those thunks are generators, so resolving one makes a state unequal to itself. Also fixed here, both found while validating the above: - `deployment-status.ts` compared the RAW version jsonb while the client's `/deployed` endpoint compares a materialized one, so the server and the client answered "needs redeploy" differently for the same workflow. Both now go through `materializeDeploymentState`, and `checkNeedsRedeployment` owns both loads so mismatched operands are unrepresentable. - The deploy button never read `isChangeDetectionSettling` — it only reached the tooltip — and change detection returns false while loading, so every page load rendered Live then Update, and every window focus rendered Update, Live, Update. Replaced with an explicit status seeded from the server's `needsRedeployment`, which the component already fetched and discarded. Comparison got faster: 1.282ms -> 1.101ms per diff on a 55-block workflow, because the always-equal `.properties` comparison is gone. Co-Authored-By: Claude Opus 5 (1M context) --- .../panel/components/deploy/deploy.tsx | 58 +++- .../panel/components/deploy/hooks/index.ts | 3 + .../hooks/use-change-detection-canary.ts | 82 +++++ .../deploy/hooks/use-change-detection.ts | 53 +++- .../hooks/use-deploy-button-status.test.ts | 116 +++++++ .../deploy/hooks/use-deploy-button-status.ts | 69 +++++ apps/sim/hooks/queries/deployments.ts | 13 +- .../sim/lib/workflows/canonical/block-spec.ts | 94 ++++++ .../provider-config-round-trip.test.ts | 170 ++++++++++ .../workflows/canonical/reported-bug.test.ts | 116 +++++++ .../lib/workflows/canonical/subblock-value.ts | 113 +++++++ .../lib/workflows/comparison/compare.test.ts | 7 +- apps/sim/lib/workflows/comparison/compare.ts | 292 +++--------------- apps/sim/lib/workflows/comparison/describe.ts | 240 ++++++++++++++ .../comparison/format-description.test.ts | 4 +- apps/sim/lib/workflows/deployment-status.ts | 44 ++- apps/sim/lib/workflows/persistence/utils.ts | 20 +- .../scripts/dump-change-detection-states.ts | 218 +++++++++++++ apps/sim/scripts/replay-change-detection.ts | 181 +++++++++++ apps/sim/stores/AGENTS.md | 14 +- apps/sim/stores/workflows/subblock/store.ts | 27 +- apps/sim/triggers/constants.ts | 12 +- 22 files changed, 1632 insertions(+), 314 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection-canary.ts create mode 100644 apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts create mode 100644 apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts create mode 100644 apps/sim/lib/workflows/canonical/block-spec.ts create mode 100644 apps/sim/lib/workflows/canonical/provider-config-round-trip.test.ts create mode 100644 apps/sim/lib/workflows/canonical/reported-bug.test.ts create mode 100644 apps/sim/lib/workflows/canonical/subblock-value.ts create mode 100644 apps/sim/lib/workflows/comparison/describe.ts create mode 100644 apps/sim/scripts/dump-change-detection-states.ts create mode 100644 apps/sim/scripts/replay-change-detection.ts diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx index fc2ae3461c2..f1cff7294ce 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx @@ -5,7 +5,9 @@ import { Chip, Tooltip, toast } from '@sim/emcn' import { useRegisterGlobalCommands } from '@/app/workspace/[workspaceId]/providers/global-commands-provider' import { DeployModal } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal' import { + resolveDeployButtonStatus, useChangeDetection, + useChangeDetectionCanary, useDeployment, useDeployReadiness, } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks' @@ -40,13 +42,42 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: const deployedState = isDeployedStateEnabled ? (deployedStateData ?? null) : null const deployReadiness = useDeployReadiness(activeWorkflowId) - const { changeDetected, isChangeDetectionSettling } = useChangeDetection({ + /* + * `isLoading` (no snapshot yet), NOT `isFetching`. A background refetch — which + * `refetchOnWindowFocus` fires on every focus — still has the cached snapshot + * to compare against, so treating it as loading blanked the answer and pushed + * an already-correct "Update" back through "Live" and out again. + */ + const { changeDetected, changedFields, isChangeDetectionSettling } = useChangeDetection({ workflowId: activeWorkflowId, deployedState, - isLoadingDeployedState: isLoadingDeployedState || isFetchingDeployedState, + isLoadingDeployedState, }) const isDeploymentSettling = isChangeDetectionSettling || deployReadiness.isSyncing + const serverNeedsRedeployment = isDeployedStateEnabled + ? deploymentInfo?.needsRedeployment + : undefined + + const buttonStatus = resolveDeployButtonStatus({ + workflowId: activeWorkflowId, + isDeployed, + isAwaitingFirstDeployedState: isLoadingDeployedState, + clientChangeDetected: changeDetected, + hasDeployedState: deployedState !== null, + serverNeedsRedeployment, + }) + const changeDetectedForModal = buttonStatus === 'changed' + + useChangeDetectionCanary({ + workflowId: activeWorkflowId, + clientChangeDetected: changeDetected, + clientChangedFields: changedFields, + serverNeedsRedeployment, + isSettling: isDeploymentSettling || deployedState === null, + isSettled: deployReadiness.status === 'ready', + }) + const { isDeploying, handleDeployClick } = useDeployment({ workflowId: activeWorkflowId, isDeployed, @@ -110,23 +141,28 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: if (deployReadiness.isBlocked && !isDeployed) { return deployReadiness.tooltip } - if (changeDetected) { + if (buttonStatus === 'changed') { return 'Update deployment' } - if (isDeployed) { + if (buttonStatus === 'live') { return 'Active deployment' } return 'Deploy workflow' } const getButtonLabel = () => { - if (changeDetected) { - return 'Update' - } - if (isDeployed) { - return 'Live' + switch (buttonStatus) { + case 'changed': + return 'Update' + case 'live': + return 'Live' + /* + * Only reachable before we know the workflow is deployed, so "Deploy" is + * the answer rather than a guess we would have to take back. + */ + default: + return 'Deploy' } - return 'Deploy' } return ( @@ -151,7 +187,7 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: onOpenChange={setIsModalOpen} workflowId={activeWorkflowId} isDeployed={isDeployed} - needsRedeployment={changeDetected} + needsRedeployment={changeDetectedForModal} deployedState={deployedState} isLoadingDeployedState={isLoadingDeployedState || isFetchingDeployedState} deployReadiness={deployReadiness} diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts index 171b8718bbc..951b8fb8d12 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts @@ -1,4 +1,7 @@ export { useChangeDetection } from './use-change-detection' +export { useChangeDetectionCanary } from './use-change-detection-canary' +export type { DeployButtonStatus } from './use-deploy-button-status' +export { resolveDeployButtonStatus } from './use-deploy-button-status' export type { DeployReadiness } from './use-deploy-readiness' export { getDeployReadinessState, useDeployReadiness } from './use-deploy-readiness' export { useDeployment } from './use-deployment' diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection-canary.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection-canary.ts new file mode 100644 index 00000000000..55768c6a9bc --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection-canary.ts @@ -0,0 +1,82 @@ +import { useEffect, useRef } from 'react' +import { createLogger } from '@sim/logger' + +const logger = createLogger('ChangeDetectionCanary') + +interface UseChangeDetectionCanaryProps { + workflowId: string | null + /** The client's in-memory answer, from `useChangeDetection`. */ + clientChangeDetected: boolean + /** The fields the client's answer rests on, for attribution. */ + clientChangedFields: string[] + /** The server's answer, already fetched by `useDeploymentInfo`. */ + serverNeedsRedeployment: boolean | undefined + /** True while either operand is still loading — a disagreement means nothing yet. */ + isSettling: boolean + /** True only when the operation queue is drained and no diff/reconcile is pending. */ + isSettled: boolean +} + +/** + * Reports when the client and the server disagree about whether a workflow needs + * redeploying. + * + * The two answers are computed from the same comparison over operands that are + * supposed to be equivalent: the server diffs the durable draft against the + * active deployment version, and the client diffs its merged in-memory state + * against the same version. Once the operation queue has drained they must + * agree, so a disagreement is a divergence between the client's state and what + * was actually persisted — the signature of every phantom "Update" this codebase + * has shipped. + * + * Costs nothing: `useDeploymentInfo` already fetches the server's answer for the + * `isDeployed` flag, and the client's answer is already computed for the button. + * Discarding both is why ten instances of this bug class were found by users + * rather than by us. + */ +export function useChangeDetectionCanary({ + workflowId, + clientChangeDetected, + clientChangedFields, + serverNeedsRedeployment, + isSettling, + isSettled, +}: UseChangeDetectionCanaryProps): void { + /** Reported once per (workflow, verdict pair) so a steady disagreement logs once. */ + const reportedRef = useRef(null) + + useEffect(() => { + if (!workflowId || isSettling || !isSettled || serverNeedsRedeployment === undefined) { + return + } + + if (serverNeedsRedeployment === clientChangeDetected) { + reportedRef.current = null + return + } + + const signature = `${workflowId}:${serverNeedsRedeployment}:${clientChangeDetected}` + if (reportedRef.current === signature) return + reportedRef.current = signature + + logger.warn('Change detection disagrees with the server', { + workflowId, + serverNeedsRedeployment, + clientChangeDetected, + /* + * Only populated when the CLIENT sees changes. The inverse case — the + * server sees changes the client does not — reports an empty list, and + * that asymmetry is itself the diagnosis: the client's merged state + * matches the deployment while the persisted draft does not. + */ + clientChangedFields, + }) + }, [ + workflowId, + clientChangeDetected, + clientChangedFields, + serverNeedsRedeployment, + isSettling, + isSettled, + ]) +} diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection.ts index a66bc19990e..a78d5eb92e7 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection.ts @@ -1,28 +1,43 @@ import { useMemo } from 'react' import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks' -import { hasWorkflowChanged } from '@/lib/workflows/comparison' +import { generateWorkflowDiffSummary } from '@/lib/workflows/comparison' import { useVariablesStore } from '@/stores/variables/store' import { useSubBlockStore } from '@/stores/workflows/subblock/store' import { useWorkflowStore } from '@/stores/workflows/workflow/store' import type { WorkflowState } from '@/stores/workflows/workflow/types' +/** Stable identity so an unchanged workflow does not hand consumers a fresh array. */ +const EMPTY_FIELDS: string[] = [] + interface UseChangeDetectionProps { workflowId: string | null deployedState: WorkflowState | null isLoadingDeployedState: boolean } +interface UseChangeDetectionResult { + changeDetected: boolean + /** + * The field names behind `changeDetected`, for diagnostics only — never for + * rendering. Free: `hasWorkflowChanged` is `generateWorkflowDiffSummary(…).hasChanges`, + * so the summary is computed either way and throwing it away only hid which + * fields drove a redeploy prompt. + */ + changedFields: string[] + isChangeDetectionSettling: boolean +} + /** * Detects meaningful changes between current workflow state and deployed state. - * Performs comparison entirely on the client using hasWorkflowChanged — no API - * calls needed. The deployed state snapshot is fetched once via React Query and - * refreshed after deploy/undeploy/version-activate mutations. + * Performs comparison entirely on the client using generateWorkflowDiffSummary — + * no API calls needed. The deployed state snapshot is fetched once via React Query + * and refreshed after deploy/undeploy/version-activate mutations. */ export function useChangeDetection({ workflowId, deployedState, isLoadingDeployedState, -}: UseChangeDetectionProps) { +}: UseChangeDetectionProps): UseChangeDetectionResult { const blocks = useWorkflowStore((state) => state.blocks) const edges = useWorkflowStore((state) => state.edges) const loops = useWorkflowStore((state) => state.loops) @@ -65,13 +80,35 @@ export function useChangeDetection({ workflowVariables, ]) - const changeDetected = useMemo(() => { - if (!currentState || !deployedState || isLoadingDeployedState) return false - return hasWorkflowChanged(currentState, deployedState) + const { changeDetected, changedFields } = useMemo(() => { + if (!currentState || !deployedState || isLoadingDeployedState) { + return { changeDetected: false, changedFields: EMPTY_FIELDS } + } + + const summary = generateWorkflowDiffSummary(currentState, deployedState) + if (!summary.hasChanges) { + return { changeDetected: false, changedFields: EMPTY_FIELDS } + } + + const fields = new Set() + for (const block of summary.modifiedBlocks) { + for (const change of block.changes) { + fields.add(`${block.type}.${change.field}`) + } + } + for (const block of summary.addedBlocks) fields.add(`+block:${block.type}`) + for (const block of summary.removedBlocks) fields.add(`-block:${block.type}`) + if (summary.edgeChanges.added > 0 || summary.edgeChanges.removed > 0) fields.add('edges') + if (summary.loopChanges.modified > 0) fields.add('loops') + if (summary.parallelChanges.modified > 0) fields.add('parallels') + if (summary.variableChanges.modified > 0) fields.add('variables') + + return { changeDetected: true, changedFields: [...fields] } }, [currentState, deployedState, isLoadingDeployedState]) return { changeDetected, + changedFields, isChangeDetectionSettling: Boolean(workflowId && isLoadingDeployedState), } } diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts new file mode 100644 index 00000000000..de14f109039 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts @@ -0,0 +1,116 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + type DeployButtonStatus, + resolveDeployButtonStatus, +} from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status' + +type Input = Parameters[0] + +const base: Input = { + workflowId: 'wf-1', + isDeployed: false, + isAwaitingFirstDeployedState: false, + clientChangeDetected: false, + hasDeployedState: false, + serverNeedsRedeployment: undefined, +} + +/** Replays a render sequence and returns the labels actually committed, deduped. */ +function committed(sequence: Array>): DeployButtonStatus[] { + const seen: DeployButtonStatus[] = [] + for (const step of sequence) { + const status = resolveDeployButtonStatus({ ...base, ...step }) + if (seen[seen.length - 1] !== status) seen.push(status) + } + return seen +} + +describe('resolveDeployButtonStatus', () => { + /** + * The regression this exists for. The old label read `changeDetected`, which + * is forced false while the deployed snapshot loads, so a changed workflow + * rendered "Live" on the way to "Update". + */ + it('never passes through live when loading a workflow that has changes', () => { + const statuses = committed([ + // 1. Nothing loaded. + {}, + // 2. deploymentInfo lands — isDeployed and needsRedeployment arrive together. + { isDeployed: true, serverNeedsRedeployment: true, isAwaitingFirstDeployedState: true }, + // 3. The deployed snapshot lands; the client diff agrees. + { + isDeployed: true, + serverNeedsRedeployment: true, + hasDeployedState: true, + clientChangeDetected: true, + }, + ]) + + expect(statuses).toEqual(['undeployed', 'changed']) + expect(statuses).not.toContain('live') + }) + + it('settles straight to live for a deployed workflow with no changes', () => { + const statuses = committed([ + {}, + { isDeployed: true, serverNeedsRedeployment: false, isAwaitingFirstDeployedState: true }, + { isDeployed: true, serverNeedsRedeployment: false, hasDeployedState: true }, + ]) + + expect(statuses).toEqual(['undeployed', 'live']) + expect(statuses).not.toContain('changed') + }) + + /** + * `refetchOnWindowFocus` is on for both queries, so this fires on every focus. + * A refetch keeps the cached snapshot, so the answer must not move. + */ + it('holds its answer across a background refetch', () => { + const settled: Partial = { + isDeployed: true, + serverNeedsRedeployment: true, + hasDeployedState: true, + clientChangeDetected: true, + } + + const statuses = committed([ + settled, + // Refetching: data is still cached, so `isAwaitingFirstDeployedState` stays false. + settled, + settled, + ]) + + expect(statuses).toEqual(['changed']) + }) + + it('prefers the client diff over the server seed once a snapshot exists', () => { + // Unsaved edits: the server still describes the persisted draft. + const status = resolveDeployButtonStatus({ + ...base, + isDeployed: true, + serverNeedsRedeployment: false, + hasDeployedState: true, + clientChangeDetected: true, + }) + + expect(status).toBe('changed') + }) + + it('reports undeployed without a workflow', () => { + expect(resolveDeployButtonStatus({ ...base, workflowId: null })).toBe('undeployed') + }) + + it('falls back to unknown only when deployed with no verdict from either side', () => { + const status = resolveDeployButtonStatus({ + ...base, + isDeployed: true, + isAwaitingFirstDeployedState: true, + serverNeedsRedeployment: undefined, + }) + + expect(status).toBe('unknown') + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts new file mode 100644 index 00000000000..0da2ce2f46f --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts @@ -0,0 +1,69 @@ +/** + * What the deploy button is allowed to say. + * + * `unknown` is a real state, not a placeholder: before the deployed snapshot + * arrives the client cannot tell `live` from `changed`, and picking either one + * is a guess that gets corrected a frame later. Naming it lets the button hold + * still instead. + */ +export type DeployButtonStatus = 'unknown' | 'undeployed' | 'live' | 'changed' + +interface ResolveDeployButtonStatusInput { + workflowId: string | null + isDeployed: boolean + /** True only before the FIRST deployed snapshot lands — never on a refetch. */ + isAwaitingFirstDeployedState: boolean + /** The client's diff, valid only once a deployed snapshot exists. */ + clientChangeDetected: boolean + hasDeployedState: boolean + /** + * The server's verdict on the persisted draft, delivered alongside + * `isDeployed`. Used only to seed the first paint. + */ + serverNeedsRedeployment: boolean | undefined +} + +/** + * Resolves the button's status without ever passing through a wrong answer. + * + * The bug this replaces: the label read `changeDetected`, which is forced to + * `false` while the deployed snapshot loads, so a deployed workflow rendered + * "Live" and then corrected itself to "Update" on every single page load. And + * because a background refetch also counted as loading, focusing the window + * pushed an already-correct "Update" back through "Live" and out again. + * + * Two rules fix it. A refetch is not a load, so a cached deployed snapshot keeps + * answering while a fresh one is in flight. And the first paint is seeded from + * the server's `needsRedeployment` — already fetched for `isDeployed`, and + * authoritative for the persisted draft — so the common case commits to the + * right label immediately instead of guessing "Live". + * + * The seed only ever decides the first paint. Once a snapshot is cached the + * client's diff is synchronous over store state, so it answers on the same + * render and the server's necessarily-staler view stops being consulted — which + * is what keeps unsaved edits from reading as "Live". + * + * `isDeployed` and `needsRedeployment` ride the same response, so a deployed + * workflow always has a seed available. `unknown` is therefore only reachable + * before we know the workflow is deployed at all, where "Deploy" is correct. + */ +export function resolveDeployButtonStatus({ + workflowId, + isDeployed, + isAwaitingFirstDeployedState, + clientChangeDetected, + hasDeployedState, + serverNeedsRedeployment, +}: ResolveDeployButtonStatusInput): DeployButtonStatus { + if (!workflowId || !isDeployed) return 'undeployed' + + if (hasDeployedState && !isAwaitingFirstDeployedState) { + return clientChangeDetected ? 'changed' : 'live' + } + + if (serverNeedsRedeployment !== undefined) { + return serverNeedsRedeployment ? 'changed' : 'live' + } + + return 'unknown' +} diff --git a/apps/sim/hooks/queries/deployments.ts b/apps/sim/hooks/queries/deployments.ts index 886e12983cc..4b9bcb9531d 100644 --- a/apps/sim/hooks/queries/deployments.ts +++ b/apps/sim/hooks/queries/deployments.ts @@ -466,9 +466,16 @@ export function useGenerateVersionDescription() { onStreamChunk, signal, }: GenerateVersionDescriptionVariables): Promise => { - const { generateWorkflowDiffSummary, formatDiffSummaryForDescriptionAsync } = await import( - '@/lib/workflows/comparison/compare' - ) + /* + * Deep paths, not the barrel: `describe` carries the block/selector + * registries and `apps/sim` has no `sideEffects: false`, so a barrel + * import would re-anchor that graph into this route's initial chunk. + */ + const [{ generateWorkflowDiffSummary }, { formatDiffSummaryForDescriptionAsync }] = + await Promise.all([ + import('@/lib/workflows/comparison/compare'), + import('@/lib/workflows/comparison/describe'), + ]) const currentState = await fetchDeploymentVersionState(workflowId, version, signal) diff --git a/apps/sim/lib/workflows/canonical/block-spec.ts b/apps/sim/lib/workflows/canonical/block-spec.ts new file mode 100644 index 00000000000..1f36e9ffa13 --- /dev/null +++ b/apps/sim/lib/workflows/canonical/block-spec.ts @@ -0,0 +1,94 @@ +import type { CanonicalFieldSpec } from '@/lib/workflows/canonical/subblock-value' +import { getBlock } from '@/blocks' +import type { BlockConfig, SubBlockConfig } from '@/blocks/types' +import type { BlockState } from '@/stores/workflows/workflow/types' + +/** The declared shape of one block type, indexed for O(1) lookup per subblock. */ +export interface CanonicalBlockSpec { + fields: ReadonlyMap +} + +interface BlockSpecVariants { + /** Resolved for a block rendering its action fields. */ + action: CanonicalBlockSpec + /** Resolved for a block in trigger mode. */ + trigger: CanonicalBlockSpec +} + +/** + * Keyed on the config's identity rather than its block type, because `getBlock` + * falls back to the custom-block overlay, whose configs are replaced at runtime. + * A type-keyed cache would keep serving a published block's old field defaults + * after an update; keying on identity re-derives when the object is swapped and + * lets the old entry be collected. Built-in configs are module-scope singletons, + * so they resolve to one stable entry for the life of the process. + */ +const variantsByConfig = new WeakMap() + +function isTriggerDeclaration(subBlock: SubBlockConfig): boolean { + return subBlock.mode === 'trigger' || subBlock.mode === 'trigger-advanced' +} + +/** + * Builds one variant's field index. + * + * A subblock id can be declared twice on the same block, once for its action + * form and once for its trigger form — Gmail declares `includeAttachments` as an + * unconditioned action switch and, via the spread of its poller's subblocks, as + * a trigger switch defaulting to `false`. Only one of them governs the value + * being compared, so the declaration matching the block's mode wins and mere + * declaration order does not decide it. Taking the first declaration made the + * trigger's default invisible, and the round-trip property test caught it on + * seven blocks. + */ +function buildVariant(config: BlockConfig, preferTrigger: boolean): CanonicalBlockSpec { + const fields = new Map() + const matchedPreferredMode = new Set() + + for (const subBlock of config.subBlocks ?? []) { + const matches = isTriggerDeclaration(subBlock) === preferTrigger + + if (fields.has(subBlock.id)) { + /* First declaration wins, unless it lost on mode and this one wins on mode. */ + if (!matches || matchedPreferredMode.has(subBlock.id)) continue + } + + if (matches) matchedPreferredMode.add(subBlock.id) + fields.set(subBlock.id, { + type: subBlock.type, + defaultValue: subBlock.defaultValue, + emptyIsValid: subBlock.emptyIsValid, + }) + } + + return { fields } +} + +/** + * Resolves the declared field specs governing a block's stored values. + * + * Returns `undefined` for a type the registry does not know (a deleted custom + * block, a state written by a newer version). Callers must treat that as "no + * declared defaults", which degrades the canonical form to blank-collapsing + * only — never to reporting a change it would otherwise have suppressed. + */ +export function resolveCanonicalBlockSpec(block: BlockState): CanonicalBlockSpec | undefined { + const config = getBlock(block.type) + if (!config) return undefined + + let variants = variantsByConfig.get(config) + if (!variants) { + variants = { + action: buildVariant(config, false), + trigger: buildVariant(config, true), + } + variantsByConfig.set(config, variants) + } + + /* + * `category === 'triggers'` covers pure trigger blocks, whose fields are all + * trigger-mode without the block carrying the flag. + */ + const inTriggerMode = block.triggerMode === true || config.category === 'triggers' + return inTriggerMode ? variants.trigger : variants.action +} diff --git a/apps/sim/lib/workflows/canonical/provider-config-round-trip.test.ts b/apps/sim/lib/workflows/canonical/provider-config-round-trip.test.ts new file mode 100644 index 00000000000..685897d971d --- /dev/null +++ b/apps/sim/lib/workflows/canonical/provider-config-round-trip.test.ts @@ -0,0 +1,170 @@ +/** + * @vitest-environment node + * + * The regression net for the phantom-redeploy bug class. + * + * Deploy materializes every declared `defaultValue` into `webhook.providerConfig` + * (`getConfigValue`), and the editor reads that back into live state when a + * trigger block's panel opens (`populateTriggerFieldsFromConfig`). The stored + * block it writes into does not necessarily have those keys — a workflow saved + * before a field was added has no entry for it at all. + * + * So for every registered trigger, the round trip + * + * stored block -> providerConfig -> read back into stored block + * + * must be invisible to change detection. When it is not, opening a block's panel + * flips the deploy button to "Update" for a change the user never made, and no + * amount of redeploying clears it. That is what shipped in #6893, when two + * defaulted switches were added to the generic webhook trigger. + * + * This asserts the property over the whole registry rather than per field, so + * the NEXT defaulted field is caught here instead of in production. + */ +import { describe, expect, it, vi } from 'vitest' + +vi.unmock('@/blocks/registry') + +import { buildProviderConfig } from '@/lib/webhooks/deploy' +import { generateWorkflowDiffSummary } from '@/lib/workflows/comparison/compare' +import { getAllBlocks } from '@/blocks' +import type { BlockState, WorkflowState } from '@/stores/workflows/workflow/types' +import { getTrigger, isTriggerValid } from '@/triggers' +import { SYSTEM_SUBBLOCK_IDS } from '@/triggers/constants' +import type { TriggerConfig } from '@/triggers/types' + +const BLOCK_ID = 'trigger-block' + +/** + * Every way a trigger can actually be hosted, which is the unit that matters: + * the canonical form resolves declared defaults from the BLOCK config, and a + * dual-mode block (`slack`) hosts triggers whose ids are not block types. + * Enumerating triggers alone would have tested a block type that never exists. + */ +function hostedTriggers(): Array<{ blockType: string; triggerId: string }> { + const pairs: Array<{ blockType: string; triggerId: string }> = [] + + for (const block of getAllBlocks()) { + const ids = new Set(block.triggers?.available ?? []) + if (block.category === 'triggers' && isTriggerValid(block.type)) ids.add(block.type) + + for (const triggerId of ids) { + if (isTriggerValid(triggerId)) pairs.push({ blockType: block.type, triggerId }) + } + } + + return pairs +} + +function configurableSubBlocks(trigger: TriggerConfig) { + return trigger.subBlocks.filter( + (subBlock) => + (subBlock.mode === 'trigger' || subBlock.mode === 'trigger-advanced') && + !SYSTEM_SUBBLOCK_IDS.includes(subBlock.id) + ) +} + +/** + * A block as the database would hold it. `absent` models a workflow saved before + * the field existed; `null` models one saved while it existed but untouched. + * Both are legal spellings of "the user never set this". + */ +function storedBlock( + blockType: string, + trigger: TriggerConfig, + spelling: 'absent' | 'null' +): BlockState { + const subBlocks: Record = { + selectedTriggerId: { id: 'selectedTriggerId', type: 'short-input', value: trigger.id }, + } + + if (spelling === 'null') { + for (const subBlock of configurableSubBlocks(trigger)) { + subBlocks[subBlock.id] = { id: subBlock.id, type: subBlock.type, value: null } + } + } + + return { + id: BLOCK_ID, + type: blockType, + name: trigger.name, + position: { x: 0, y: 0 }, + subBlocks, + outputs: {}, + enabled: true, + horizontalHandles: true, + triggerMode: true, + data: {}, + } as unknown as BlockState +} + +/** + * A pure port of `populateTriggerFieldsFromConfig` composed with the structural + * half of `mergeSubblockStateWithValues`: a written value lands on the block, + * creating the entry when the structure had none (which is only allowed for a + * non-null value). + */ +function readProviderConfigBack( + block: BlockState, + providerConfig: Record, + trigger: TriggerConfig +): BlockState { + const subBlocks: Record = { ...(block.subBlocks ?? {}) } + + for (const subBlock of configurableSubBlocks(trigger)) { + const configValue = providerConfig[subBlock.id] + if (configValue === undefined) continue + + const existing = subBlocks[subBlock.id] as { value?: unknown } | undefined + const current = existing?.value + if (current !== null && current !== undefined && current !== '') continue + if (configValue === null) continue + + subBlocks[subBlock.id] = { + id: subBlock.id, + type: existing ? (existing as { type?: string }).type : 'short-input', + value: configValue, + } + } + + return { ...block, subBlocks } as BlockState +} + +function stateWith(block: BlockState): WorkflowState { + return { + blocks: { [BLOCK_ID]: block }, + edges: [], + loops: {}, + parallels: {}, + variables: {}, + } as unknown as WorkflowState +} + +const pairs = hostedTriggers() + +describe('deploy -> providerConfig -> read-back round trip', () => { + it('covers every hosted trigger', () => { + expect(pairs.length).toBeGreaterThan(50) + }) + + it.each(pairs.map((p) => [`${p.blockType} / ${p.triggerId}`, p] as const))( + '%s is invisible to change detection', + (_label, { blockType, triggerId }) => { + const trigger = getTrigger(triggerId) + + for (const spelling of ['absent', 'null'] as const) { + const stored = storedBlock(blockType, trigger, spelling) + const { providerConfig } = buildProviderConfig(stored, triggerId, trigger) + const afterFocus = readProviderConfigBack(stored, providerConfig, trigger) + + const summary = generateWorkflowDiffSummary(stateWith(afterFocus), stateWith(stored)) + const reported = summary.modifiedBlocks.flatMap((b) => b.changes.map((c) => c.field)) + + expect( + reported, + `${blockType}/${triggerId} (${spelling}) reported a change the user never made` + ).toEqual([]) + } + } + ) +}) diff --git a/apps/sim/lib/workflows/canonical/reported-bug.test.ts b/apps/sim/lib/workflows/canonical/reported-bug.test.ts new file mode 100644 index 00000000000..559a406c091 --- /dev/null +++ b/apps/sim/lib/workflows/canonical/reported-bug.test.ts @@ -0,0 +1,116 @@ +/** + * @vitest-environment node + * + * The originally reported workflow, kept verbatim as a fixture. + * + * `provider-config-round-trip.test.ts` asserts the same property across the whole + * registry and is the stronger guard, but it synthesizes its blocks. This one is + * a real block a real user hit, including the exact spelling mix that produced + * the bug: `verifyTestEvents` stored as `null`, and `acceptOtherMethods` / + * `exposeRequestHeaders` absent entirely because the workflow predates #6893. + */ +import { describe, expect, it, vi } from 'vitest' + +/** + * The canonical form reads declared defaults, so the globally-mocked registry + * (every block reduced to `subBlocks: []`) would make this pass vacuously. + */ +vi.unmock('@/blocks/registry') + +import { generateWorkflowDiffSummary } from '@/lib/workflows/comparison/compare' +import type { WorkflowState } from '@/stores/workflows/workflow/types' + +const deployedWebhookBlock = { + id: 'ddbc6e68-fbfc-5ac0-970e-40fcd7fa6493', + type: 'generic_webhook', + name: 'AskRVTReturn', + position: { x: 150, y: 143.65 }, + subBlocks: { + token: { id: 'token', type: 'short-input', value: null }, + inputFormat: { id: 'inputFormat', type: 'input-format', value: [] }, + requireAuth: { id: 'requireAuth', type: 'switch', value: false }, + responseBody: { id: 'responseBody', type: 'code', value: null }, + responseMode: { id: 'responseMode', type: 'dropdown', value: 'custom' }, + idempotencyField: { id: 'idempotencyField', type: 'short-input', value: null }, + secretHeaderName: { id: 'secretHeaderName', type: 'short-input', value: null }, + verifyTestEvents: { id: 'verifyTestEvents', type: 'switch', value: null }, + responseStatusCode: { id: 'responseStatusCode', type: 'short-input', value: '200' }, + }, + outputs: {}, + enabled: true, + horizontalHandles: true, + height: 48, + advancedMode: false, + errorEnabled: false, + triggerMode: true, + data: {}, + locked: false, +} + +/** + * What focusing the block produces: `useWebhookManagement` reads the deployed + * `webhook.providerConfig` — into which deploy materialized every declared + * default — and writes it back through `mergeSubblockStateWithValues`, which + * creates a structure entry for any non-null value. + */ +const liveWebhookBlockAfterFocus = { + ...deployedWebhookBlock, + subBlocks: { + ...deployedWebhookBlock.subBlocks, + verifyTestEvents: { id: 'verifyTestEvents', type: 'switch', value: false }, + acceptOtherMethods: { id: 'acceptOtherMethods', type: 'short-input', value: false }, + exposeRequestHeaders: { id: 'exposeRequestHeaders', type: 'short-input', value: false }, + }, +} + +function stateWith(block: Record): WorkflowState { + return { + blocks: { [block.id as string]: block }, + edges: [], + loops: {}, + parallels: {}, + variables: {}, + } as unknown as WorkflowState +} + +describe('generic_webhook focus (the reported bug)', () => { + it('does not report a change when deploy-materialized defaults are read back', () => { + const summary = generateWorkflowDiffSummary( + stateWith(liveWebhookBlockAfterFocus), + stateWith(deployedWebhookBlock) + ) + + expect(summary.modifiedBlocks).toEqual([]) + expect(summary.hasChanges).toBe(false) + }) + + it('still reports a change the user actually made', () => { + const edited = { + ...deployedWebhookBlock, + subBlocks: { + ...deployedWebhookBlock.subBlocks, + responseStatusCode: { id: 'responseStatusCode', type: 'short-input', value: '418' }, + }, + } + + const summary = generateWorkflowDiffSummary(stateWith(edited), stateWith(deployedWebhookBlock)) + + expect(summary.hasChanges).toBe(true) + expect(summary.modifiedBlocks[0]?.changes.map((c) => c.field)).toEqual(['responseStatusCode']) + }) + + it('reports a defaulted switch the user deliberately turned ON', () => { + const enabled = { + ...deployedWebhookBlock, + subBlocks: { + ...deployedWebhookBlock.subBlocks, + acceptOtherMethods: { id: 'acceptOtherMethods', type: 'switch', value: true }, + }, + } + + const summary = generateWorkflowDiffSummary(stateWith(enabled), stateWith(deployedWebhookBlock)) + + expect(summary.hasChanges).toBe(true) + expect(summary.modifiedBlocks[0]?.changes.map((c) => c.field)).toEqual(['acceptOtherMethods']) + }) +}) diff --git a/apps/sim/lib/workflows/canonical/subblock-value.ts b/apps/sim/lib/workflows/canonical/subblock-value.ts new file mode 100644 index 00000000000..cb3d6ca76ef --- /dev/null +++ b/apps/sim/lib/workflows/canonical/subblock-value.ts @@ -0,0 +1,113 @@ +import { + normalizedStringify, + sanitizeInputFormat, + sanitizeTableRows, + sanitizeTools, +} from '@/lib/workflows/comparison/normalize' + +/** + * Everything the canonical form needs to know about one declared subblock. + * Derived from the block definition, never from stored state. + */ +export interface CanonicalFieldSpec { + /** The declared subblock type, or the stored one when the field is undeclared. */ + type?: string + /** Absent when the definition declares no default. Never `null` by convention. */ + defaultValue?: unknown + /** The field declares that an explicitly empty value is a real choice. */ + emptyIsValid?: boolean +} + +/** + * Strips the presentation-only parts of a stored value. + * + * Rules are applied by subblock id AND by declared type, deliberately taking the + * union: the id-keyed forms are what the comparison has always used, and the + * type-keyed forms catch the same shape under a different field name. A value + * can only lose presentation detail here, never gain meaning. + */ +export function shapeSubBlockValue( + subBlockId: string, + value: unknown, + subBlockType: string | undefined +): unknown { + let shaped: unknown = value ?? null + + if (Array.isArray(shaped) && (subBlockId === 'tools' || subBlockType === 'tool-input')) { + shaped = sanitizeTools(shaped) + } + if ( + Array.isArray(shaped) && + (subBlockId === 'inputFormat' || + subBlockType === 'input-format' || + subBlockType === 'response-format') + ) { + shaped = sanitizeInputFormat(shaped) + } + if (Array.isArray(shaped) && subBlockType === 'table') { + const rows = sanitizeTableRows(shaped) + shaped = rows.length > 0 ? rows : null + } + + return shaped +} + +/** + * Resolves a stored subblock value to the configuration it actually represents. + * + * Returns `undefined` for "this field carries no configuration" — which is what + * a blank value and a value equal to the field's declared default both mean. + * Collapsing them is the whole point: an unset field has four legal spellings in + * storage (key absent, `null`, `''`, or the declared default, which deploy + * materializes into `webhook.providerConfig`), and different pipelines pick + * different ones. Any comparison that can tell them apart reports a change the + * user did not make — and, because adding a defaulted field to a block + * definition changes the spelling on one side only, does so retroactively for + * every already-deployed workflow. + * + * Resolution is deliberately comparison-time only. Writing defaults into storage + * would answer the same question, but it is a one-way migration whose rollback + * leaves every seeded block permanently divergent, and it would destroy the + * "key absent means this state predates the field" signal the subblock-rename + * migrations depend on. + * + * `defaultValue` is consulted; `value()` deliberately is not. Value thunks are + * generators (`() => generateId()`), so resolving one would produce a fresh + * value per call and make a state unequal to itself. + */ +export function canonicalizeSubBlockValue( + subBlockId: string, + stored: unknown, + spec: CanonicalFieldSpec | undefined +): unknown { + const shaped = shapeSubBlockValue(subBlockId, stored, spec?.type) + + /* + * Absent, `null` and `undefined` are one state: no consumer distinguishes + * them, and the subblock merge contract already collapses `undefined` into + * "no value recorded". + */ + if (shaped === null || shaped === undefined) return undefined + + if (spec?.defaultValue === undefined) return shaped + + const shapedDefault = shapeSubBlockValue(subBlockId, spec.defaultValue, spec.type) + if (normalizedStringify(shaped) === normalizedStringify(shapedDefault)) return undefined + + /* + * `''` collapses only for a field that declares a default, because that is + * exactly where the substitution happens: `getConfigValue` writes the default + * into `webhook.providerConfig` for a blank OR empty-string value, so the two + * are already indistinguishable in every artifact deploy produces. + * + * Where no default is declared, `''` stays a real value — the serializer fills + * a subblock from its `value()` thunk on `params[id] == null`, which `''` does + * not satisfy, so an empty string suppresses the thunk where `null` fires it. + * + * `emptyIsValid` opts a field out: it declares that an explicitly empty value + * is a choice the user made, not the absence of one. + */ + if (shaped === '' && !spec.emptyIsValid) return undefined + + return shaped +} diff --git a/apps/sim/lib/workflows/comparison/compare.test.ts b/apps/sim/lib/workflows/comparison/compare.test.ts index 63811a90dc5..c9f0d815cd5 100644 --- a/apps/sim/lib/workflows/comparison/compare.test.ts +++ b/apps/sim/lib/workflows/comparison/compare.test.ts @@ -7,11 +7,8 @@ import { } from '@sim/testing' import { describe, expect, it } from 'vitest' import type { WorkflowState } from '@/stores/workflows/workflow/types' -import { - formatDiffSummaryForDescription, - generateWorkflowDiffSummary, - hasWorkflowChanged, -} from './compare' +import { generateWorkflowDiffSummary, hasWorkflowChanged } from './compare' +import { formatDiffSummaryForDescription } from './describe' /** * Type helper for converting test workflow state to app workflow state. diff --git a/apps/sim/lib/workflows/comparison/compare.ts b/apps/sim/lib/workflows/comparison/compare.ts index e030e1b13aa..ccc6e62c57d 100644 --- a/apps/sim/lib/workflows/comparison/compare.ts +++ b/apps/sim/lib/workflows/comparison/compare.ts @@ -1,30 +1,26 @@ -import { createLogger } from '@sim/logger' import { blockRetryEquals, collectErrorSourceBlockIds, resolveEffectiveErrorEnabled, } from '@sim/workflow-types/workflow' +import { resolveCanonicalBlockSpec } from '@/lib/workflows/canonical/block-spec' +import { + type CanonicalFieldSpec, + canonicalizeSubBlockValue, +} from '@/lib/workflows/canonical/subblock-value' import type { WorkflowState } from '@/stores/workflows/workflow/types' import { extractBlockFieldsForComparison, - extractSubBlockRest, filterSubBlockIds, normalizedStringify, normalizeEdge, normalizeLoop, normalizeParallel, - normalizeSubBlockValue, normalizeTriggerConfigValues, normalizeValue, normalizeVariables, sanitizeVariable, } from './normalize' -import { formatValueForDisplay, resolveFieldLabel, resolveValueForDisplay } from './resolve-values' - -const MAX_CHANGES_PER_BLOCK = 6 -const MAX_EDGE_DETAILS = 3 - -const logger = createLogger('WorkflowComparison') /** * Compare the current workflow state with the deployed state to detect meaningful changes. @@ -266,43 +262,44 @@ export function generateWorkflowDiffSummary( ...new Set([...Object.keys(normalizedCurrentSubs), ...Object.keys(normalizedPreviousSubs)]), ]) + /* + * Resolved from the CURRENT definition and applied to both sides, so a field + * added to a block definition after a workflow was deployed reads the same + * on the frozen snapshot as on the live draft. + */ + const blockSpec = resolveCanonicalBlockSpec(currentBlock) + for (const subId of allSubBlockIds) { const currentSub = normalizedCurrentSubs[subId] as Record | undefined const previousSub = normalizedPreviousSubs[subId] as Record | undefined - if (!currentSub || !previousSub) { - changes.push({ - field: subId, - oldValue: (previousSub as Record | undefined)?.value ?? null, - newValue: (currentSub as Record | undefined)?.value ?? null, - }) - continue + /* + * A field the definition does not declare still gets blank-collapsed; it + * just has no default to compare against. Falling back to the stored type + * keeps the shape rules working for undeclared and custom-block fields. + */ + const declared = blockSpec?.fields.get(subId) + const spec: CanonicalFieldSpec = { + type: (declared?.type ?? currentSub?.type ?? previousSub?.type) as string | undefined, + defaultValue: declared?.defaultValue, + emptyIsValid: declared?.emptyIsValid, } - const subType = currentSub.type ?? previousSub.type - const currentValue = normalizeSubBlockValue(subId, currentSub.value, subType) - const previousValue = normalizeSubBlockValue(subId, previousSub.value, subType) - - if (typeof currentValue === 'string' && typeof previousValue === 'string') { - if (currentValue !== previousValue) { - changes.push({ field: subId, oldValue: previousSub.value, newValue: currentSub.value }) - } - } else { - const normalizedCurrent = normalizeValue(currentValue) - const normalizedPrevious = normalizeValue(previousValue) - if (normalizedStringify(normalizedCurrent) !== normalizedStringify(normalizedPrevious)) { - changes.push({ field: subId, oldValue: previousSub.value, newValue: currentSub.value }) - } - } - - const currentSubRest = extractSubBlockRest(currentSub) - const previousSubRest = extractSubBlockRest(previousSub) - - if (normalizedStringify(currentSubRest) !== normalizedStringify(previousSubRest)) { + /* + * Absence and blankness are the same answer here, so a key present on one + * side only is not itself a change — it is a change only if the value it + * holds resolves to something. Comparing presence directly is what made + * `acceptOtherMethods: false` on the live side differ from a deployed + * snapshot that predates the field. + */ + const currentValue = canonicalizeSubBlockValue(subId, currentSub?.value, spec) + const previousValue = canonicalizeSubBlockValue(subId, previousSub?.value, spec) + + if (normalizedStringify(currentValue) !== normalizedStringify(previousValue)) { changes.push({ - field: `${subId}.properties`, - oldValue: previousSubRest, - newValue: currentSubRest, + field: subId, + oldValue: previousSub?.value ?? null, + newValue: currentSub?.value ?? null, }) } } @@ -438,220 +435,3 @@ export function generateWorkflowDiffSummary( return result } - -/** - * Convert a WorkflowDiffSummary to a human-readable string for AI description generation - */ -export function formatDiffSummaryForDescription(summary: WorkflowDiffSummary): string { - if (!summary.hasChanges) { - return 'No structural changes detected (configuration may have changed)' - } - - const changes: string[] = [] - - for (const block of summary.addedBlocks) { - const name = block.name || block.type - changes.push(`Added block: ${name} (${block.type})`) - } - - for (const block of summary.removedBlocks) { - const name = block.name || block.type - changes.push(`Removed block: ${name} (${block.type})`) - } - - for (const block of summary.modifiedBlocks) { - const name = block.name || block.type - const meaningfulChanges = block.changes.filter((c) => !c.field.endsWith('.properties')) - for (const change of meaningfulChanges.slice(0, MAX_CHANGES_PER_BLOCK)) { - const fieldLabel = resolveFieldLabel(block.type, change.field) - const oldStr = formatValueForDisplay(change.oldValue) - const newStr = formatValueForDisplay(change.newValue) - changes.push(`Modified ${name}: ${fieldLabel} changed from "${oldStr}" to "${newStr}"`) - } - if (meaningfulChanges.length > MAX_CHANGES_PER_BLOCK) { - changes.push( - ` ...and ${meaningfulChanges.length - MAX_CHANGES_PER_BLOCK} more changes in ${name}` - ) - } - } - - formatEdgeChanges(summary, changes) - formatCountChanges(summary.loopChanges, 'loop', changes) - formatCountChanges(summary.parallelChanges, 'parallel group', changes) - formatVariableChanges(summary, changes) - - return changes.join('\n') -} - -/** - * Converts a WorkflowDiffSummary to a human-readable string with resolved display names. - * Resolves IDs (credentials, channels, workflows, etc.) to human-readable names using - * the selector registry infrastructure. - * - * @param summary - The diff summary to format - * @param currentState - The current workflow state for context extraction - * @param workflowId - The workflow ID for API calls - * @returns A formatted string describing the changes with resolved names - */ -export async function formatDiffSummaryForDescriptionAsync( - summary: WorkflowDiffSummary, - currentState: WorkflowState, - workflowId: string -): Promise { - if (!summary.hasChanges) { - return 'No structural changes detected (configuration may have changed)' - } - - const changes: string[] = [] - - for (const block of summary.addedBlocks) { - const name = block.name || block.type - changes.push(`Added block: ${name} (${block.type})`) - } - - for (const block of summary.removedBlocks) { - const name = block.name || block.type - changes.push(`Removed block: ${name} (${block.type})`) - } - - const modifiedBlockPromises = summary.modifiedBlocks.map(async (block) => { - const name = block.name || block.type - const blockChanges: string[] = [] - const meaningfulChanges = block.changes.filter((c) => !c.field.endsWith('.properties')) - - const changesToProcess = meaningfulChanges.slice(0, MAX_CHANGES_PER_BLOCK) - const resolvedChanges = await Promise.all( - changesToProcess.map(async (change) => { - const context = { - blockType: block.type, - subBlockId: change.field, - workflowId, - currentState, - blockId: block.id, - } - - const [oldResolved, newResolved] = await Promise.all([ - resolveValueForDisplay(change.oldValue, context), - resolveValueForDisplay(change.newValue, context), - ]) - - return { - field: resolveFieldLabel(block.type, change.field), - oldLabel: oldResolved.displayLabel, - newLabel: newResolved.displayLabel, - } - }) - ) - - for (const resolved of resolvedChanges) { - blockChanges.push( - `Modified ${name}: ${resolved.field} changed from "${resolved.oldLabel}" to "${resolved.newLabel}"` - ) - } - - if (meaningfulChanges.length > MAX_CHANGES_PER_BLOCK) { - blockChanges.push( - ` ...and ${meaningfulChanges.length - MAX_CHANGES_PER_BLOCK} more changes in ${name}` - ) - } - - return blockChanges - }) - - const allModifiedBlockChanges = await Promise.all(modifiedBlockPromises) - for (const blockChanges of allModifiedBlockChanges) { - changes.push(...blockChanges) - } - - formatEdgeChanges(summary, changes) - formatCountChanges(summary.loopChanges, 'loop', changes) - formatCountChanges(summary.parallelChanges, 'parallel group', changes) - formatVariableChanges(summary, changes) - - logger.info('Generated async diff description', { - workflowId, - changeCount: changes.length, - modifiedBlocks: summary.modifiedBlocks.length, - }) - - return changes.join('\n') -} - -function formatEdgeDetailList( - edges: Array<{ sourceName: string; targetName: string }>, - total: number, - verb: string, - changes: string[] -): void { - if (edges.length === 0) { - changes.push(`${verb} ${total} connection(s)`) - return - } - for (const edge of edges.slice(0, MAX_EDGE_DETAILS)) { - changes.push(`${verb} connection: ${edge.sourceName} -> ${edge.targetName}`) - } - if (total > MAX_EDGE_DETAILS) { - changes.push(` ...and ${total - MAX_EDGE_DETAILS} more ${verb.toLowerCase()} connection(s)`) - } -} - -function formatEdgeChanges(summary: WorkflowDiffSummary, changes: string[]): void { - if (summary.edgeChanges.added > 0) { - formatEdgeDetailList( - summary.edgeChanges.addedDetails ?? [], - summary.edgeChanges.added, - 'Added', - changes - ) - } - if (summary.edgeChanges.removed > 0) { - formatEdgeDetailList( - summary.edgeChanges.removedDetails ?? [], - summary.edgeChanges.removed, - 'Removed', - changes - ) - } -} - -function formatCountChanges( - counts: { added: number; removed: number; modified: number }, - label: string, - changes: string[] -): void { - if (counts.added > 0) changes.push(`Added ${counts.added} ${label}(s)`) - if (counts.removed > 0) changes.push(`Removed ${counts.removed} ${label}(s)`) - if (counts.modified > 0) changes.push(`Modified ${counts.modified} ${label}(s)`) -} - -function formatVariableChanges(summary: WorkflowDiffSummary, changes: string[]): void { - const categories = [ - { - count: summary.variableChanges.added, - names: summary.variableChanges.addedNames ?? [], - verb: 'added', - }, - { - count: summary.variableChanges.removed, - names: summary.variableChanges.removedNames ?? [], - verb: 'removed', - }, - { - count: summary.variableChanges.modified, - names: summary.variableChanges.modifiedNames ?? [], - verb: 'modified', - }, - ] as const - - const varParts: string[] = [] - for (const { count, names, verb } of categories) { - if (count > 0) { - varParts.push( - names.length > 0 ? `${verb} ${names.map((n) => `"${n}"`).join(', ')}` : `${count} ${verb}` - ) - } - } - if (varParts.length > 0) { - changes.push(`Variables: ${varParts.join(', ')}`) - } -} diff --git a/apps/sim/lib/workflows/comparison/describe.ts b/apps/sim/lib/workflows/comparison/describe.ts new file mode 100644 index 00000000000..4ef398720a0 --- /dev/null +++ b/apps/sim/lib/workflows/comparison/describe.ts @@ -0,0 +1,240 @@ +import { createLogger } from '@sim/logger' +import type { WorkflowDiffSummary } from '@/lib/workflows/comparison/compare' +import { + formatValueForDisplay, + resolveFieldLabel, + resolveValueForDisplay, +} from '@/lib/workflows/comparison/resolve-values' +import type { WorkflowState } from '@/stores/workflows/workflow/types' + +/** + * Renders a diff summary as prose. + * + * Deliberately separate from `compare.ts`: resolving an id to a human-readable + * name reaches the block registry, the selector registry and the network, and + * `apps/sim` does not declare `sideEffects: false`, so a barrel re-export would + * keep that whole graph alive for every consumer of the comparison — including + * two server modules that only ever ask the yes/no question. + */ + +const MAX_CHANGES_PER_BLOCK = 6 +const MAX_EDGE_DETAILS = 3 + +const logger = createLogger('WorkflowDescribe') + +/** + * Convert a WorkflowDiffSummary to a human-readable string for AI description generation + */ +export function formatDiffSummaryForDescription(summary: WorkflowDiffSummary): string { + if (!summary.hasChanges) { + return 'No structural changes detected (configuration may have changed)' + } + + const changes: string[] = [] + + for (const block of summary.addedBlocks) { + const name = block.name || block.type + changes.push(`Added block: ${name} (${block.type})`) + } + + for (const block of summary.removedBlocks) { + const name = block.name || block.type + changes.push(`Removed block: ${name} (${block.type})`) + } + + for (const block of summary.modifiedBlocks) { + const name = block.name || block.type + const meaningfulChanges = block.changes.filter((c) => !c.field.endsWith('.properties')) + for (const change of meaningfulChanges.slice(0, MAX_CHANGES_PER_BLOCK)) { + const fieldLabel = resolveFieldLabel(block.type, change.field) + const oldStr = formatValueForDisplay(change.oldValue) + const newStr = formatValueForDisplay(change.newValue) + changes.push(`Modified ${name}: ${fieldLabel} changed from "${oldStr}" to "${newStr}"`) + } + if (meaningfulChanges.length > MAX_CHANGES_PER_BLOCK) { + changes.push( + ` ...and ${meaningfulChanges.length - MAX_CHANGES_PER_BLOCK} more changes in ${name}` + ) + } + } + + formatEdgeChanges(summary, changes) + formatCountChanges(summary.loopChanges, 'loop', changes) + formatCountChanges(summary.parallelChanges, 'parallel group', changes) + formatVariableChanges(summary, changes) + + return changes.join('\n') +} + +/** + * Converts a WorkflowDiffSummary to a human-readable string with resolved display names. + * Resolves IDs (credentials, channels, workflows, etc.) to human-readable names using + * the selector registry infrastructure. + * + * @param summary - The diff summary to format + * @param currentState - The current workflow state for context extraction + * @param workflowId - The workflow ID for API calls + * @returns A formatted string describing the changes with resolved names + */ +export async function formatDiffSummaryForDescriptionAsync( + summary: WorkflowDiffSummary, + currentState: WorkflowState, + workflowId: string +): Promise { + if (!summary.hasChanges) { + return 'No structural changes detected (configuration may have changed)' + } + + const changes: string[] = [] + + for (const block of summary.addedBlocks) { + const name = block.name || block.type + changes.push(`Added block: ${name} (${block.type})`) + } + + for (const block of summary.removedBlocks) { + const name = block.name || block.type + changes.push(`Removed block: ${name} (${block.type})`) + } + + const modifiedBlockPromises = summary.modifiedBlocks.map(async (block) => { + const name = block.name || block.type + const blockChanges: string[] = [] + const meaningfulChanges = block.changes.filter((c) => !c.field.endsWith('.properties')) + + const changesToProcess = meaningfulChanges.slice(0, MAX_CHANGES_PER_BLOCK) + const resolvedChanges = await Promise.all( + changesToProcess.map(async (change) => { + const context = { + blockType: block.type, + subBlockId: change.field, + workflowId, + currentState, + blockId: block.id, + } + + const [oldResolved, newResolved] = await Promise.all([ + resolveValueForDisplay(change.oldValue, context), + resolveValueForDisplay(change.newValue, context), + ]) + + return { + field: resolveFieldLabel(block.type, change.field), + oldLabel: oldResolved.displayLabel, + newLabel: newResolved.displayLabel, + } + }) + ) + + for (const resolved of resolvedChanges) { + blockChanges.push( + `Modified ${name}: ${resolved.field} changed from "${resolved.oldLabel}" to "${resolved.newLabel}"` + ) + } + + if (meaningfulChanges.length > MAX_CHANGES_PER_BLOCK) { + blockChanges.push( + ` ...and ${meaningfulChanges.length - MAX_CHANGES_PER_BLOCK} more changes in ${name}` + ) + } + + return blockChanges + }) + + const allModifiedBlockChanges = await Promise.all(modifiedBlockPromises) + for (const blockChanges of allModifiedBlockChanges) { + changes.push(...blockChanges) + } + + formatEdgeChanges(summary, changes) + formatCountChanges(summary.loopChanges, 'loop', changes) + formatCountChanges(summary.parallelChanges, 'parallel group', changes) + formatVariableChanges(summary, changes) + + logger.info('Generated async diff description', { + workflowId, + changeCount: changes.length, + modifiedBlocks: summary.modifiedBlocks.length, + }) + + return changes.join('\n') +} + +function formatEdgeDetailList( + edges: Array<{ sourceName: string; targetName: string }>, + total: number, + verb: string, + changes: string[] +): void { + if (edges.length === 0) { + changes.push(`${verb} ${total} connection(s)`) + return + } + for (const edge of edges.slice(0, MAX_EDGE_DETAILS)) { + changes.push(`${verb} connection: ${edge.sourceName} -> ${edge.targetName}`) + } + if (total > MAX_EDGE_DETAILS) { + changes.push(` ...and ${total - MAX_EDGE_DETAILS} more ${verb.toLowerCase()} connection(s)`) + } +} + +function formatEdgeChanges(summary: WorkflowDiffSummary, changes: string[]): void { + if (summary.edgeChanges.added > 0) { + formatEdgeDetailList( + summary.edgeChanges.addedDetails ?? [], + summary.edgeChanges.added, + 'Added', + changes + ) + } + if (summary.edgeChanges.removed > 0) { + formatEdgeDetailList( + summary.edgeChanges.removedDetails ?? [], + summary.edgeChanges.removed, + 'Removed', + changes + ) + } +} + +function formatCountChanges( + counts: { added: number; removed: number; modified: number }, + label: string, + changes: string[] +): void { + if (counts.added > 0) changes.push(`Added ${counts.added} ${label}(s)`) + if (counts.removed > 0) changes.push(`Removed ${counts.removed} ${label}(s)`) + if (counts.modified > 0) changes.push(`Modified ${counts.modified} ${label}(s)`) +} + +function formatVariableChanges(summary: WorkflowDiffSummary, changes: string[]): void { + const categories = [ + { + count: summary.variableChanges.added, + names: summary.variableChanges.addedNames ?? [], + verb: 'added', + }, + { + count: summary.variableChanges.removed, + names: summary.variableChanges.removedNames ?? [], + verb: 'removed', + }, + { + count: summary.variableChanges.modified, + names: summary.variableChanges.modifiedNames ?? [], + verb: 'modified', + }, + ] as const + + const varParts: string[] = [] + for (const { count, names, verb } of categories) { + if (count > 0) { + varParts.push( + names.length > 0 ? `${verb} ${names.map((n) => `"${n}"`).join(', ')}` : `${count} ${verb}` + ) + } + } + if (varParts.length > 0) { + changes.push(`Variables: ${varParts.join(', ')}`) + } +} diff --git a/apps/sim/lib/workflows/comparison/format-description.test.ts b/apps/sim/lib/workflows/comparison/format-description.test.ts index c1051bafa10..037b685406e 100644 --- a/apps/sim/lib/workflows/comparison/format-description.test.ts +++ b/apps/sim/lib/workflows/comparison/format-description.test.ts @@ -49,11 +49,11 @@ vi.mock('@/hooks/selectors/resolution', () => ({ import { WorkflowBuilder } from '@sim/testing' import type { WorkflowDiffSummary } from '@/lib/workflows/comparison/compare' +import { generateWorkflowDiffSummary } from '@/lib/workflows/comparison/compare' import { formatDiffSummaryForDescription, formatDiffSummaryForDescriptionAsync, - generateWorkflowDiffSummary, -} from '@/lib/workflows/comparison/compare' +} from '@/lib/workflows/comparison/describe' import { formatValueForDisplay, resolveFieldLabel } from '@/lib/workflows/comparison/resolve-values' function emptyDiffSummary(overrides: Partial = {}): WorkflowDiffSummary { diff --git a/apps/sim/lib/workflows/deployment-status.ts b/apps/sim/lib/workflows/deployment-status.ts index 3498506a5f0..ae641b17079 100644 --- a/apps/sim/lib/workflows/deployment-status.ts +++ b/apps/sim/lib/workflows/deployment-status.ts @@ -1,24 +1,32 @@ import { db, workflowDeploymentVersion } from '@sim/db' import { and, desc, eq, sql } from 'drizzle-orm' import { hasWorkflowChanged } from '@/lib/workflows/comparison' -import { loadWorkflowDeploymentSnapshot } from '@/lib/workflows/persistence/utils' +import { + loadWorkflowDeploymentSnapshot, + materializeDeploymentState, +} from '@/lib/workflows/persistence/utils' import type { WorkflowState } from '@/stores/workflows/workflow/types' -/** Compares the current durable draft with the active deployment snapshot. */ -export function computeNeedsRedeployment( - currentSnapshot: WorkflowState | null | undefined, - activeState: WorkflowState | null | undefined -): boolean { - if (!activeState || !currentSnapshot) return false - return hasWorkflowChanged(currentSnapshot, activeState) -} - -/** Reads both sides at repeatable-read isolation so the comparison is coherent. */ +/** + * Reports whether the durable draft has diverged from the active deployment. + * + * Owns both loads deliberately. The two operands are only comparable once each + * has been through its own projection: the draft picks up handle canonicalization + * and the block migrations from `loadWorkflowFromNormalizedTables`, and the + * version's frozen jsonb picks up the equivalents — plus the `errorEnabled` + * backfill — from `materializeDeploymentState`. Accepting either operand from a + * caller is what let this surface compare a raw jsonb blob against a normalized + * draft, so that the server and the client answered the same question + * differently for the same workflow. + */ export async function checkNeedsRedeployment(workflowId: string): Promise { return db.transaction(async (tx) => { await tx.execute(sql`SET TRANSACTION ISOLATION LEVEL REPEATABLE READ`) const [active] = await tx - .select({ state: workflowDeploymentVersion.state }) + .select({ + id: workflowDeploymentVersion.id, + state: workflowDeploymentVersion.state, + }) .from(workflowDeploymentVersion) .where( and( @@ -29,7 +37,17 @@ export async function checkNeedsRedeployment(workflowId: string): Promise { const cached = deployedStateCache.get(version.id) if (cached) { @@ -173,7 +184,8 @@ async function materializeDeploymentState( const { blocks: migratedBlocks } = await applyBlockMigrations( state.blocks || {}, - resolvedWorkspaceId + resolvedWorkspaceId, + executor ) /* * Read straight out of the version's jsonb blob, so unlike every path that diff --git a/apps/sim/scripts/dump-change-detection-states.ts b/apps/sim/scripts/dump-change-detection-states.ts new file mode 100644 index 00000000000..8cbde0d74cc --- /dev/null +++ b/apps/sim/scripts/dump-change-detection-states.ts @@ -0,0 +1,218 @@ +/** + * Produces the JSONL the replay harness consumes, using the SAME loaders + * production compares. + * + * This exists because SQL cannot produce the right operands. The draft side is + * assembled by `loadWorkflowFromNormalizedTables`, which applies the block + * migrations, materializes loop/parallel defaults and canonicalizes edge + * handles; the deployed side is a frozen blob that `materializeDeploymentState` + * re-migrates, re-canonicalizes and backfills `errorEnabled` on. Rebuilding + * either of those in SQL would be a second spelling of the loaders — the exact + * mistake the change this validates exists to remove. + * + * Usage (from apps/sim, with DATABASE_URL pointing at a read replica): + * + * bun run scripts/dump-change-detection-states.ts --limit 500 > dump.jsonl + * bun run scripts/dump-change-detection-states.ts --limit 500 --raw > dump.jsonl # keep secrets + * + * Values under credential-shaped keys are replaced with a deterministic hash by + * default, so equality is preserved on both sides while the plaintext is not + * written to disk. Scoped to those keys rather than all strings on purpose: + * hashing a value that happens to equal its declared `defaultValue` would break + * the exact comparison being validated, and credential fields never declare one. + */ + +import { createHash } from 'node:crypto' +import { closeSync, openSync, writeSync } from 'node:fs' +import { db, workflowDeploymentVersion } from '@sim/db' +import { webhook as webhookTable, workflow as workflowTable } from '@sim/db/schema' +import { and, desc, eq, inArray, isNull } from 'drizzle-orm' +import { + loadWorkflowDeploymentSnapshot, + materializeDeploymentState, +} from '@/lib/workflows/persistence/utils' +import { getTrigger, isTriggerValid } from '@/triggers' +import { SYSTEM_SUBBLOCK_IDS } from '@/triggers/constants' +import { resolveBlockTriggerId } from '@/triggers/webhook-url' + +const SECRET_KEY_PATTERN = /(token|secret|password|apikey|api_key|credential)/i + +function hashValue(value: string): string { + return `scrubbed:${createHash('sha256').update(value).digest('hex').slice(0, 16)}` +} + +function scrubBlocks(blocks: Record | undefined): Record { + const out: Record = {} + + for (const [blockId, block] of Object.entries(blocks ?? {})) { + const subBlocks: Record = {} + for (const [subId, subBlock] of Object.entries( + (block?.subBlocks ?? {}) as Record + )) { + const value = subBlock?.value + subBlocks[subId] = + SECRET_KEY_PATTERN.test(subId) && typeof value === 'string' && value.length > 0 + ? { ...subBlock, value: hashValue(value) } + : subBlock + } + out[blockId] = { ...block, subBlocks } + } + + return out +} + +/** + * Reproduces what opening a trigger block's editor panel does to live state: + * `useWebhookManagement` reads the deployed `webhook.providerConfig` — into which + * deploy materialized every declared default — and writes it back over any field + * the block holds blank. + * + * The replay cannot observe this without simulating it. Those writes go through + * a non-persisting `setValue`, so they exist only in the browser's store and are + * absent from every database-sourced snapshot. A dump taken straight from the + * draft therefore shows the workflow as clean no matter how badly the panel + * misreports it. + */ +function simulateFocus( + blocks: Record, + providerConfigByBlockId: Map> +): Record { + const out: Record = {} + + for (const [blockId, block] of Object.entries(blocks ?? {})) { + const providerConfig = providerConfigByBlockId.get(blockId) + const triggerId = providerConfig ? resolveBlockTriggerId(block) : undefined + + if (!providerConfig || !triggerId || !isTriggerValid(triggerId)) { + out[blockId] = block + continue + } + + const subBlocks: Record = { ...(block.subBlocks ?? {}) } + for (const subBlock of getTrigger(triggerId).subBlocks) { + if (subBlock.mode !== 'trigger' && subBlock.mode !== 'trigger-advanced') continue + if (SYSTEM_SUBBLOCK_IDS.includes(subBlock.id)) continue + + const configValue = providerConfig[subBlock.id] + if (configValue === undefined || configValue === null) continue + + const current = subBlocks[subBlock.id]?.value + if (current !== null && current !== undefined && current !== '') continue + + subBlocks[subBlock.id] = { + id: subBlock.id, + type: subBlocks[subBlock.id]?.type ?? 'short-input', + value: configValue, + } + } + + out[blockId] = { ...block, subBlocks } + } + + return out +} + +async function main(): Promise { + const args = process.argv.slice(2) + const limitArg = args.indexOf('--limit') + const limit = limitArg >= 0 ? Number(args[limitArg + 1]) : 200 + const raw = args.includes('--raw') + const focus = args.includes('--simulate-focus') + const webhooksOnly = args.includes('--webhooks-only') + + /* + * Written to a file, not stdout. `loadWorkflowFromNormalizedTables` logs — it + * warns whenever its fire-and-forget migration write-back fails, which it + * always does against a read replica — and those lines land on stdout and + * corrupt the JSONL. + */ + const outArg = args.indexOf('--out') + if (outArg < 0) { + process.stderr.write( + 'usage: --out [--limit N] [--webhooks-only] [--simulate-focus] [--raw]\n' + ) + process.exit(2) + } + const outFd = openSync(args[outArg + 1], 'w') + + const rows = await db + .select({ + workflowId: workflowTable.id, + workspaceId: workflowTable.workspaceId, + versionId: workflowDeploymentVersion.id, + state: workflowDeploymentVersion.state, + }) + .from(workflowDeploymentVersion) + .innerJoin(workflowTable, eq(workflowTable.id, workflowDeploymentVersion.workflowId)) + .where(and(eq(workflowDeploymentVersion.isActive, true), isNull(workflowTable.archivedAt))) + .orderBy(desc(workflowDeploymentVersion.createdAt)) + .limit(limit) + + const webhookRows = rows.length + ? await db + .select({ + workflowId: webhookTable.workflowId, + blockId: webhookTable.blockId, + providerConfig: webhookTable.providerConfig, + }) + .from(webhookTable) + .where( + inArray( + webhookTable.workflowId, + rows.map((r) => r.workflowId) + ) + ) + : [] + + const webhooksByWorkflow = new Map>>() + for (const wh of webhookRows) { + if (!wh.blockId || !wh.providerConfig) continue + const perBlock = webhooksByWorkflow.get(wh.workflowId) ?? new Map() + perBlock.set(wh.blockId, wh.providerConfig as Record) + webhooksByWorkflow.set(wh.workflowId, perBlock) + } + + let emitted = 0 + let failed = 0 + + for (const row of rows) { + try { + const webhooks = webhooksByWorkflow.get(row.workflowId) + if (webhooksOnly && !webhooks) continue + + const current = await loadWorkflowDeploymentSnapshot(row.workflowId) + if (!current) continue + + const deployed = await materializeDeploymentState( + row.workflowId, + { id: row.versionId, state: row.state }, + row.workspaceId ?? undefined + ) + + const currentBlocks = + focus && webhooks ? simulateFocus(current.blocks, webhooks) : current.blocks + + writeSync( + outFd, + `${JSON.stringify({ + workflowId: row.workflowId, + current: raw + ? { ...current, blocks: currentBlocks } + : { ...current, blocks: scrubBlocks(currentBlocks) }, + deployed: raw ? deployed : { ...deployed, blocks: scrubBlocks(deployed.blocks) }, + })}\n` + ) + emitted++ + } catch (error) { + /* Reported, never silently skipped: a workflow that cannot load is a finding. */ + failed++ + process.stderr.write(`skip ${row.workflowId}: ${(error as Error).message}\n`) + } + } + + closeSync(outFd) + process.stderr.write(`emitted ${emitted} workflow(s), ${failed} failed\n`) + process.exit(0) +} + +void main() diff --git a/apps/sim/scripts/replay-change-detection.ts b/apps/sim/scripts/replay-change-detection.ts new file mode 100644 index 00000000000..c908800dac9 --- /dev/null +++ b/apps/sim/scripts/replay-change-detection.ts @@ -0,0 +1,181 @@ +/** + * Replays change detection over real workflow states and reports, per workflow, + * exactly which fields it would prompt a redeploy for. + * + * This is the ship gate for the canonical-form change: the invariant is that the + * new pipeline must never report a field the old one did not. Erase-to-absent + * only ever removes a distinction, so that should hold by construction — but ten + * previous instances of this bug class were also "obviously" fine, so it gets + * asserted against production data rather than reasoned about. + * + * Usage: + * + * # 1. Record the current (new) behavior. + * bun run apps/sim/scripts/replay-change-detection.ts dump.jsonl > after.jsonl + * + * # 2. Record the old behavior from before the change. + * git stash && bun run apps/sim/scripts/replay-change-detection.ts dump.jsonl > before.jsonl && git stash pop + * + * # 3. Compare. Exits non-zero if any workflow gained a reported field. + * bun run apps/sim/scripts/replay-change-detection.ts --compare before.jsonl after.jsonl + * + * The dump is JSONL, one workflow per line: + * + * {"workflowId": "...", "current": , "deployed": } + * + * `current` must come from `loadWorkflowDeploymentSnapshot` and `deployed` from + * `materializeDeploymentState` — the same projections production compares. A + * dump built from raw jsonb would be measuring a comparison nothing performs. + */ + +import { readFileSync } from 'node:fs' +import { generateWorkflowDiffSummary } from '@/lib/workflows/comparison/compare' +import type { WorkflowState } from '@/stores/workflows/workflow/types' + +interface DumpRow { + workflowId: string + current: WorkflowState + deployed: WorkflowState | null +} + +interface ReplayRow { + workflowId: string + hasChanges: boolean + fields: string[] +} + +/** + * Field identity is `blockId.field` rather than just `field`, so a field that + * stops being reported on one block but starts on another cannot cancel out. + */ +function reportedFields(current: WorkflowState, deployed: WorkflowState | null): string[] { + const summary = generateWorkflowDiffSummary(current, deployed) + const fields = new Set() + + for (const block of summary.modifiedBlocks) { + for (const change of block.changes) fields.add(`${block.id}.${change.field}`) + } + for (const block of summary.addedBlocks) fields.add(`+block.${block.id}`) + for (const block of summary.removedBlocks) fields.add(`-block.${block.id}`) + if (summary.edgeChanges.added > 0) fields.add('edges.added') + if (summary.edgeChanges.removed > 0) fields.add('edges.removed') + if (summary.loopChanges.modified > 0) fields.add('loops.modified') + if (summary.parallelChanges.modified > 0) fields.add('parallels.modified') + if (summary.variableChanges.modified > 0) fields.add('variables.modified') + + return [...fields].sort() +} + +function replay(dumpPath: string): void { + const lines = readFileSync(dumpPath, 'utf8').split('\n').filter(Boolean) + + for (const line of lines) { + const row = JSON.parse(line) as DumpRow + let out: ReplayRow + + try { + const fields = reportedFields(row.current, row.deployed) + out = { workflowId: row.workflowId, hasChanges: fields.length > 0, fields } + } catch (error) { + /* + * A workflow that throws is a finding, not a skip: the comparison runs on + * the deploy button's render path, so a throw is a broken panel. + */ + out = { + workflowId: row.workflowId, + hasChanges: false, + fields: [`__error__:${(error as Error).message}`], + } + } + + process.stdout.write(`${JSON.stringify(out)}\n`) + } +} + +function readReplay(path: string): Map { + const rows = new Map() + for (const line of readFileSync(path, 'utf8').split('\n').filter(Boolean)) { + const row = JSON.parse(line) as ReplayRow + rows.set(row.workflowId, row) + } + return rows +} + +function compare(beforePath: string, afterPath: string): number { + const before = readReplay(beforePath) + const after = readReplay(afterPath) + + const gained: Array<{ workflowId: string; fields: string[] }> = [] + const lostByField = new Map() + let flippedToClean = 0 + let flippedToChanged = 0 + + for (const [workflowId, afterRow] of after) { + const beforeRow = before.get(workflowId) + if (!beforeRow) continue + + const beforeFields = new Set(beforeRow.fields) + const afterFields = new Set(afterRow.fields) + + const newlyReported = [...afterFields].filter((f) => !beforeFields.has(f)) + if (newlyReported.length > 0) gained.push({ workflowId, fields: newlyReported }) + + for (const field of beforeFields) { + if (afterFields.has(field)) continue + /* Bucket by field NAME, not by block, so the report is readable. */ + const name = field.slice(field.indexOf('.') + 1) + lostByField.set(name, (lostByField.get(name) ?? 0) + 1) + } + + if (beforeRow.hasChanges && !afterRow.hasChanges) flippedToClean++ + if (!beforeRow.hasChanges && afterRow.hasChanges) flippedToChanged++ + } + + const log = (message: string) => process.stderr.write(`${message}\n`) + + log(`workflows compared: ${after.size}`) + log(`flipped "Update" -> "Live": ${flippedToClean}`) + log(`flipped "Live" -> "Update": ${flippedToChanged}`) + log('') + log('fields that stopped being reported (bucketed, review every bucket):') + for (const [field, count] of [...lostByField].sort((a, b) => b[1] - a[1])) { + log(` ${String(count).padStart(6)} ${field}`) + } + + if (gained.length > 0) { + log('') + log(`BLOCKING: ${gained.length} workflow(s) gained a reported field.`) + for (const entry of gained.slice(0, 20)) { + log(` ${entry.workflowId}: ${entry.fields.join(', ')}`) + } + if (gained.length > 20) log(` ...and ${gained.length - 20} more`) + return 1 + } + + if (flippedToChanged > 0) { + log('') + log(`BLOCKING: ${flippedToChanged} workflow(s) newly report changes.`) + return 1 + } + + log('') + log('OK: no workflow reports a field it did not report before.') + return 0 +} + +const args = process.argv.slice(2) + +if (args[0] === '--compare') { + if (!args[1] || !args[2]) { + process.stderr.write('usage: --compare \n') + process.exit(2) + } + process.exit(compare(args[1], args[2])) +} + +if (!args[0]) { + process.stderr.write('usage: replay-change-detection.ts \n') + process.exit(2) +} + +replay(args[0]) diff --git a/apps/sim/stores/AGENTS.md b/apps/sim/stores/AGENTS.md index 8883b425d8e..f5253330573 100644 --- a/apps/sim/stores/AGENTS.md +++ b/apps/sim/stores/AGENTS.md @@ -25,5 +25,15 @@ this split correct: store write that skips persistence makes the client's merged state diverge from the DB draft, which deploy snapshots — producing phantom "Update" states on the deploy button that clear on refresh. Hydration-derived local-only writes are allowed only - when change detection compensates (see `populateTriggerFieldsFromConfig` + - `normalizeTriggerConfigValues`). + when change detection compensates, and the exemption list in the store's own + docstring (`workflows/subblock/store.ts`) is the record of which ones do and why — + keep the two in step. +- Change detection compensates by resolving each subblock value to the configuration + it represents (`lib/workflows/canonical/`), so a blank value and a value equal to + the field's declared `defaultValue` compare equal. It does NOT compensate for a + local-only write of a value the user could have chosen. If you cannot name the + declared default your write matches, it is not exempt. +- Deploy materializes declared defaults into `webhook.providerConfig`, so anything + reading a derived artifact back into the store is writing values the DB draft does + not have. That circularity is the origin of this whole failure mode; prefer not + reading derived artifacts back at all. diff --git a/apps/sim/stores/workflows/subblock/store.ts b/apps/sim/stores/workflows/subblock/store.ts index c723f274bca..14c609a57fb 100644 --- a/apps/sim/stores/workflows/subblock/store.ts +++ b/apps/sim/stores/workflows/subblock/store.ts @@ -49,11 +49,28 @@ export const EMPTY_BLOCK_SUBBLOCK_VALUES: Record = {} * server — the client's merged state and the DB draft stay equivalent, which * is what keeps deploy-time change detection honest (deploy snapshots the DB * draft). Direct setValue callers do not persist, and each is safe for a - * different reason: remote-broadcast application (already persisted - * server-side), undo/redo (persists via its own queued inverse operations), - * webhook management (writes trigger-runtime ids the comparison excludes), and - * populateTriggerFieldsFromConfig (values derived from the persisted - * triggerConfig aggregate, compensated via normalizeTriggerConfigValues). + * different reason: + * + * - remote-broadcast application — already persisted server-side + * - undo/redo — persists via its own queued inverse operations + * - synthetic tool subblock ids — excluded from both persistence and comparison + * - whole-document replacement — the server's own state, re-seeded + * - webhook management's runtime ids (webhookId/triggerPath/triggerConfig/ + * triggerId) — excluded by filterSubBlockIds + * - populateTriggerFieldsFromConfig — writes deploy-materialized defaults, which + * the canonical form resolves to the same value on both sides + * + * That last entry previously claimed `normalizeTriggerConfigValues` compensated + * for it. It never could: that helper back-fills from a side's OWN persisted + * `triggerConfig` aggregate, nothing has persisted one since the modal-era + * migration, and it only fills keys that already exist — while the divergence it + * was supposed to cancel is a key that does not. Roughly half of all deployed + * webhooks carried a phantom "Update" because of it. The compensation is now + * `canonicalizeSubBlockValue`, which resolves a blank value and a value equal to + * its declared default to the same thing on both sides of the comparison. + * + * Adding a new direct caller means adding a line above. If it writes an id the + * comparison reads, and nothing here explains why that is safe, it is not. */ export const useSubBlockStore = create()( diff --git a/apps/sim/triggers/constants.ts b/apps/sim/triggers/constants.ts index 94f2cf9c382..82277ef90b6 100644 --- a/apps/sim/triggers/constants.ts +++ b/apps/sim/triggers/constants.ts @@ -17,11 +17,13 @@ export const SYSTEM_SUBBLOCK_IDS: string[] = [ * Trigger-related subblock IDs that represent runtime metadata. They should remain * in the workflow state but must not be modified or cleared by diff operations. * - * Note: 'triggerConfig' is included because it's an aggregate of individual trigger - * field subblocks. Those individual fields are compared separately, so comparing - * triggerConfig would be redundant. Additionally, the client populates triggerConfig - * with default values from the trigger definition on load, which aren't present in - * the deployed state, causing false positive change detection. + * Note: 'triggerConfig' is included because it is an aggregate of the individual + * trigger field subblocks, which are compared separately — comparing the + * aggregate too would double-count them. + * + * It is also a write guard: `edit_workflow` rejects writes to these ids, which + * is what stops the copilot resurrecting the modal-era aggregate. Do not remove + * an entry here on the grounds that the comparison no longer needs it. */ export const TRIGGER_RUNTIME_SUBBLOCK_IDS: string[] = [ 'webhookId', From e0d35bd54ce22a556acb7a33ebdf40a577cf764d Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 13:12:33 -0700 Subject: [PATCH 2/7] chore(test): use neutral identifiers in the change-detection fixture The fixture carried the reporting workflow's name and block UUID. This repo is public, so neither belongs in it. The stored subblock spelling is what the test actually pins, and that is unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/canonical/reported-bug.test.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/sim/lib/workflows/canonical/reported-bug.test.ts b/apps/sim/lib/workflows/canonical/reported-bug.test.ts index 559a406c091..8232f244a56 100644 --- a/apps/sim/lib/workflows/canonical/reported-bug.test.ts +++ b/apps/sim/lib/workflows/canonical/reported-bug.test.ts @@ -1,13 +1,16 @@ /** * @vitest-environment node * - * The originally reported workflow, kept verbatim as a fixture. + * The block shape from the originally reported workflow. Identifiers are + * replaced; the stored subblock spelling is reproduced exactly, because that is + * the part that carries the bug. * * `provider-config-round-trip.test.ts` asserts the same property across the whole - * registry and is the stronger guard, but it synthesizes its blocks. This one is - * a real block a real user hit, including the exact spelling mix that produced - * the bug: `verifyTestEvents` stored as `null`, and `acceptOtherMethods` / - * `exposeRequestHeaders` absent entirely because the workflow predates #6893. + * registry and is the stronger guard, but it synthesizes its blocks. This one + * pins the combination a real deployment actually held: `verifyTestEvents` + * stored as `null`, `acceptOtherMethods` / `exposeRequestHeaders` absent + * entirely because the workflow predates #6893, `responseMode` explicitly set + * away from its default, and `inputFormat` as an empty array rather than null. */ import { describe, expect, it, vi } from 'vitest' @@ -21,9 +24,9 @@ import { generateWorkflowDiffSummary } from '@/lib/workflows/comparison/compare' import type { WorkflowState } from '@/stores/workflows/workflow/types' const deployedWebhookBlock = { - id: 'ddbc6e68-fbfc-5ac0-970e-40fcd7fa6493', + id: 'webhook-block', type: 'generic_webhook', - name: 'AskRVTReturn', + name: 'Webhook', position: { x: 150, y: 143.65 }, subBlocks: { token: { id: 'token', type: 'short-input', value: null }, From 3db915237ad4ccf836bb2845753c18e6de1a7ee4 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 13:18:36 -0700 Subject: [PATCH 3/7] fix(scripts): type the replay dump against BlockState instead of any MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dump and focus simulator described blocks and subblocks as `Record`, so a wrong assumption about workflow shape would have compiled — in the one tool whose whole job is to be trusted about workflow shape. Uses `BlockState`/`SubBlockState` throughout; the single remaining cast narrows a jsonb `providerConfig` value to `SubBlockState['value']`. Also corrects the usage docstring, which still described the pre-`--out` stdout form, and records why `DATABASE_URL` must not carry `sslrootcert`: postgres.js forwards unrecognized query params as session parameters, so a libpq-style URL fails every query with 42704. Co-Authored-By: Claude Opus 5 (1M context) --- .../scripts/dump-change-detection-states.ts | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/apps/sim/scripts/dump-change-detection-states.ts b/apps/sim/scripts/dump-change-detection-states.ts index 8cbde0d74cc..d4d4ae57171 100644 --- a/apps/sim/scripts/dump-change-detection-states.ts +++ b/apps/sim/scripts/dump-change-detection-states.ts @@ -10,10 +10,17 @@ * either of those in SQL would be a second spelling of the loaders — the exact * mistake the change this validates exists to remove. * - * Usage (from apps/sim, with DATABASE_URL pointing at a read replica): + * Usage (from apps/sim, with DATABASE_URL pointing at a read replica). Note that + * `DATABASE_URL` must not carry libpq-only SSL params: postgres.js forwards any + * query param it does not recognize as a session parameter, so `sslrootcert` + * fails every query with `42704`. `?sslmode=verify-full` alone keeps full + * verification. * - * bun run scripts/dump-change-detection-states.ts --limit 500 > dump.jsonl - * bun run scripts/dump-change-detection-states.ts --limit 500 --raw > dump.jsonl # keep secrets + * bun run scripts/dump-change-detection-states.ts --out dump.jsonl --limit 500 + * bun run scripts/dump-change-detection-states.ts --out dump.jsonl --webhooks-only \ + * --simulate-focus # reproduce the panel-focus read-back + * bun run scripts/dump-change-detection-states.ts --out dump.jsonl --raw + * # keep credential values verbatim * * Values under credential-shaped keys are replaced with a deterministic hash by * default, so equality is preserved on both sides while the plaintext is not @@ -31,6 +38,7 @@ import { loadWorkflowDeploymentSnapshot, materializeDeploymentState, } from '@/lib/workflows/persistence/utils' +import type { BlockState, SubBlockState } from '@/stores/workflows/workflow/types' import { getTrigger, isTriggerValid } from '@/triggers' import { SYSTEM_SUBBLOCK_IDS } from '@/triggers/constants' import { resolveBlockTriggerId } from '@/triggers/webhook-url' @@ -41,15 +49,13 @@ function hashValue(value: string): string { return `scrubbed:${createHash('sha256').update(value).digest('hex').slice(0, 16)}` } -function scrubBlocks(blocks: Record | undefined): Record { - const out: Record = {} +function scrubBlocks(blocks: Record): Record { + const out: Record = {} - for (const [blockId, block] of Object.entries(blocks ?? {})) { - const subBlocks: Record = {} - for (const [subId, subBlock] of Object.entries( - (block?.subBlocks ?? {}) as Record - )) { - const value = subBlock?.value + for (const [blockId, block] of Object.entries(blocks)) { + const subBlocks: Record = {} + for (const [subId, subBlock] of Object.entries(block.subBlocks ?? {})) { + const value = subBlock.value subBlocks[subId] = SECRET_KEY_PATTERN.test(subId) && typeof value === 'string' && value.length > 0 ? { ...subBlock, value: hashValue(value) } @@ -74,12 +80,12 @@ function scrubBlocks(blocks: Record | undefined): Record, + blocks: Record, providerConfigByBlockId: Map> -): Record { - const out: Record = {} +): Record { + const out: Record = {} - for (const [blockId, block] of Object.entries(blocks ?? {})) { + for (const [blockId, block] of Object.entries(blocks)) { const providerConfig = providerConfigByBlockId.get(blockId) const triggerId = providerConfig ? resolveBlockTriggerId(block) : undefined @@ -88,7 +94,7 @@ function simulateFocus( continue } - const subBlocks: Record = { ...(block.subBlocks ?? {}) } + const subBlocks: Record = { ...(block.subBlocks ?? {}) } for (const subBlock of getTrigger(triggerId).subBlocks) { if (subBlock.mode !== 'trigger' && subBlock.mode !== 'trigger-advanced') continue if (SYSTEM_SUBBLOCK_IDS.includes(subBlock.id)) continue @@ -102,7 +108,7 @@ function simulateFocus( subBlocks[subBlock.id] = { id: subBlock.id, type: subBlocks[subBlock.id]?.type ?? 'short-input', - value: configValue, + value: configValue as SubBlockState['value'], } } From d54b8d327e7bb2a3dca10d0c1af43d4762df8d0b Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 13:36:27 -0700 Subject: [PATCH 4/7] fix(deploy): derive every deploy surface from one deployment verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The modal could render "Deploy your workflow to see a preview" directly above a version row reading `v1 (live)`, because the General tab inferred "not deployed" from the ABSENCE OF A SNAPSHOT. A missing snapshot is not evidence of anything — usually it just has not arrived — and treating it as evidence let the modal contradict itself. Underneath that, nothing re-fetched the snapshot once it came back empty. `refetchDeploymentBoundary` fires while the query is still disabled (it is gated on `isDeployed`, which is not true yet), and a disabled query cannot be refetched, so a null cached during the activation window survived the whole stale period. `useDeployedWorkflowState` now retries while it holds no snapshot and stops the instant one arrives — the query is only enabled once the workflow IS deployed, so a null there is a contradiction to resolve, not an answer. The deeper problem was that the chip, the modal footer and the preview each derived their own verdict from a different mix of raw flags. That is the same failure this branch fixes one layer down — several derivations of one fact, drifting — so it gets the same treatment. `useDeploymentViewState` derives once; the chip, the modal and the General tab consume it and are given no raw material to re-derive from. `DeployModal` now takes one `deployment` prop in place of four booleans it used to recombine. Also brings the chip in line with the busy-label pattern every sibling control on this surface already follows (`{isUndeploying ? 'Undeploying...' : 'Undeploy'}` in the modal footer): it now reads "Deploying..." while the deploy is in flight, where before it announced nothing and merely went disabled. Scoped to the deploy action, which the mutation bounds. The readiness states stay in the tooltip on purpose — `saving` fires on every settled keystroke, so putting it on the chip would reintroduce the label churn the state machine exists to remove. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/general/general.tsx | 11 +- .../components/deploy-modal/deploy-modal.tsx | 24 ++-- .../panel/components/deploy/deploy.tsx | 81 +++++-------- .../panel/components/deploy/hooks/index.ts | 2 + .../deploy/hooks/use-deployment-view-state.ts | 107 ++++++++++++++++++ apps/sim/hooks/queries/deployments.ts | 15 +++ 6 files changed, 177 insertions(+), 63 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/general/general.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/general/general.tsx index 3238ad37b8e..cb70b4e7950 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/general/general.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/general/general.tsx @@ -33,6 +33,8 @@ interface GeneralDeployProps { workflowId: string | null deployedState?: WorkflowState | null isLoadingDeployedState: boolean + /** A snapshot is expected but has not arrived — render loading, not "undeployed". */ + isAwaitingSnapshot: boolean versions: WorkflowDeploymentVersionResponse[] versionsLoading: boolean isPromotingVersion: boolean @@ -51,6 +53,7 @@ export function GeneralDeploy({ workflowId, deployedState, isLoadingDeployedState, + isAwaitingSnapshot, versions, versionsLoading, isPromotingVersion, @@ -169,7 +172,13 @@ export function GeneralDeploy({ const showToggle = selectedVersion !== null && deployedState const hasDeployedData = deployedState && Object.keys(deployedState.blocks || {}).length > 0 - const showLoadingSkeleton = isLoadingDeployedState && !hasDeployedData + /* + * `isAwaitingSnapshot` counts as loading. A missing snapshot is not evidence + * that the workflow is undeployed — it is usually the snapshot not having + * arrived yet — and treating it as evidence is what rendered "Deploy your + * workflow to see a preview" directly above a row reading `v1 (live)`. + */ + const showLoadingSkeleton = (isLoadingDeployedState || isAwaitingSnapshot) && !hasDeployedData if (showLoadingSkeleton) { return ( diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal.tsx index 45c67249614..0b38610706e 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal.tsx @@ -35,6 +35,7 @@ import { tryAcquireDeployAction, } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/deploy-action-lock' import type { DeployReadiness } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-readiness' +import type { DeploymentViewState } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state' import { runPreDeployChecks } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-predeploy-checks' import { normalizeName, startsWithUuid } from '@/executor/constants' import { useApiKeys } from '@/hooks/queries/api-keys' @@ -66,12 +67,10 @@ interface DeployModalProps { open: boolean onOpenChange: (open: boolean) => void workflowId: string | null - isDeployed: boolean - needsRedeployment: boolean + /** The one derived deployment verdict, shared with the deploy chip. */ + deployment: DeploymentViewState deployedState?: WorkflowState | null - isLoadingDeployedState: boolean deployReadiness: DeployReadiness - isDeploymentSettling: boolean } interface WorkflowDeploymentInfoUI { @@ -96,13 +95,19 @@ export function DeployModal({ open, onOpenChange, workflowId, - isDeployed: isDeployedProp, - needsRedeployment, - deployedState, - isLoadingDeployedState, + deployment, deployReadiness, - isDeploymentSettling, }: DeployModalProps) { + const { + status: deploymentStatus, + isDeployed: isDeployedProp, + deployedState, + isAwaitingSnapshot, + isSettling: isDeploymentSettling, + } = deployment + const needsRedeployment = deploymentStatus === 'changed' + /* A snapshot that is expected but absent reads as loading everywhere. */ + const isLoadingDeployedState = isAwaitingSnapshot const queryClient = useQueryClient() const params = useParams() const workspaceId = params?.workspaceId as string @@ -561,6 +566,7 @@ export function DeployModal({ workflowId={workflowId} deployedState={deployedState} isLoadingDeployedState={isLoadingDeployedState} + isAwaitingSnapshot={isAwaitingSnapshot} versions={versions} versionsLoading={versionsLoading} isPromotingVersion={isActivatingVersion || activateVersionMutation.isPending} diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx index f1cff7294ce..d303016cac4 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx @@ -5,14 +5,11 @@ import { Chip, Tooltip, toast } from '@sim/emcn' import { useRegisterGlobalCommands } from '@/app/workspace/[workspaceId]/providers/global-commands-provider' import { DeployModal } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal' import { - resolveDeployButtonStatus, - useChangeDetection, - useChangeDetectionCanary, useDeployment, + useDeploymentViewState, useDeployReadiness, } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks' import { useCurrentWorkflow } from '@/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-current-workflow' -import { useDeployedWorkflowState, useDeploymentInfo } from '@/hooks/queries/deployments' import type { WorkspaceUserPermissions } from '@/hooks/use-user-permissions' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' @@ -28,55 +25,21 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: const isRegistryLoading = hydrationPhase === 'idle' || hydrationPhase === 'state-loading' const { hasBlocks } = useCurrentWorkflow() - const { data: deploymentInfo } = useDeploymentInfo(activeWorkflowId, { - enabled: !isRegistryLoading, - }) - const isDeployed = deploymentInfo?.isDeployed ?? false - - const isDeployedStateEnabled = Boolean(activeWorkflowId) && isDeployed && !isRegistryLoading - const { - data: deployedStateData, - isLoading: isLoadingDeployedState, - isFetching: isFetchingDeployedState, - } = useDeployedWorkflowState(activeWorkflowId, { enabled: isDeployedStateEnabled }) - const deployedState = isDeployedStateEnabled ? (deployedStateData ?? null) : null const deployReadiness = useDeployReadiness(activeWorkflowId) /* - * `isLoading` (no snapshot yet), NOT `isFetching`. A background refetch — which - * `refetchOnWindowFocus` fires on every focus — still has the cached snapshot - * to compare against, so treating it as loading blanked the answer and pushed - * an already-correct "Update" back through "Live" and out again. + * One derivation for the chip, the modal preview and the modal footer. They + * previously each read their own mix of raw flags, which is how the preview + * could say "Deploy your workflow to see a preview" while the version list + * beneath it said `v1 (live)`. */ - const { changeDetected, changedFields, isChangeDetectionSettling } = useChangeDetection({ + const deployment = useDeploymentViewState({ workflowId: activeWorkflowId, - deployedState, - isLoadingDeployedState, - }) - const isDeploymentSettling = isChangeDetectionSettling || deployReadiness.isSyncing - - const serverNeedsRedeployment = isDeployedStateEnabled - ? deploymentInfo?.needsRedeployment - : undefined - - const buttonStatus = resolveDeployButtonStatus({ - workflowId: activeWorkflowId, - isDeployed, - isAwaitingFirstDeployedState: isLoadingDeployedState, - clientChangeDetected: changeDetected, - hasDeployedState: deployedState !== null, - serverNeedsRedeployment, - }) - const changeDetectedForModal = buttonStatus === 'changed' - - useChangeDetectionCanary({ - workflowId: activeWorkflowId, - clientChangeDetected: changeDetected, - clientChangedFields: changedFields, - serverNeedsRedeployment, - isSettling: isDeploymentSettling || deployedState === null, - isSettled: deployReadiness.status === 'ready', + enabled: !isRegistryLoading, + deployReadiness, }) + const { status: buttonStatus, isDeployed, deployedState } = deployment + const isDeploymentSettling = deployment.isSettling const { isDeploying, handleDeployClick } = useDeployment({ workflowId: activeWorkflowId, @@ -135,7 +98,7 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: if (isDeploying) { return 'Deploying...' } - if (isChangeDetectionSettling) { + if (isDeploymentSettling) { return 'Syncing deployment state...' } if (deployReadiness.isBlocked && !isDeployed) { @@ -151,6 +114,22 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: } const getButtonLabel = () => { + /* + * The label carries the busy state, matching every sibling control on this + * surface (`{isUndeploying ? 'Undeploying...' : 'Undeploy'}` in the modal + * footer) and the vocabulary `deployReadiness` already speaks. This chip was + * the one button that announced nothing and merely went disabled. + * + * Scoped to the deploy action, which is bounded by the mutation. The + * readiness states are deliberately NOT surfaced here: `saving` fires on + * every settled keystroke, so rendering it would reintroduce exactly the + * label churn this state machine exists to remove. Those stay in the + * tooltip, where they explain why the button is disabled. + */ + if (isDeploying) { + return 'Deploying...' + } + switch (buttonStatus) { case 'changed': return 'Update' @@ -186,12 +165,8 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: open={isModalOpen} onOpenChange={setIsModalOpen} workflowId={activeWorkflowId} - isDeployed={isDeployed} - needsRedeployment={changeDetectedForModal} - deployedState={deployedState} - isLoadingDeployedState={isLoadingDeployedState || isFetchingDeployedState} + deployment={deployment} deployReadiness={deployReadiness} - isDeploymentSettling={isDeploymentSettling} /> ) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts index 951b8fb8d12..ca9d607a984 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/index.ts @@ -5,3 +5,5 @@ export { resolveDeployButtonStatus } from './use-deploy-button-status' export type { DeployReadiness } from './use-deploy-readiness' export { getDeployReadinessState, useDeployReadiness } from './use-deploy-readiness' export { useDeployment } from './use-deployment' +export type { DeploymentViewState } from './use-deployment-view-state' +export { useDeploymentViewState } from './use-deployment-view-state' diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts new file mode 100644 index 00000000000..9b5e6e0ef30 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts @@ -0,0 +1,107 @@ +import { useChangeDetection } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection' +import { useChangeDetectionCanary } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection-canary' +import { + type DeployButtonStatus, + resolveDeployButtonStatus, +} from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status' +import type { DeployReadiness } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-readiness' +import { useDeployedWorkflowState, useDeploymentInfo } from '@/hooks/queries/deployments' +import type { WorkflowState } from '@/stores/workflows/workflow/types' + +export interface DeploymentViewState { + /** The single verdict every deploy surface renders from. */ + status: DeployButtonStatus + isDeployed: boolean + /** The active deployment's snapshot, or null while it is not in hand. */ + deployedState: WorkflowState | null + /** + * A snapshot is expected but has not arrived. Distinct from "there is no + * snapshot": the difference is what separates a skeleton from telling the user + * their workflow is not deployed. + */ + isAwaitingSnapshot: boolean + isSettling: boolean + changeDetected: boolean + changedFields: string[] +} + +interface UseDeploymentViewStateProps { + workflowId: string | null + enabled: boolean + deployReadiness: DeployReadiness +} + +/** + * Owns every derived answer the deploy surface renders — the chip's label, the + * modal's preview, the modal's footer — so they cannot disagree. + * + * They used to. The chip resolved a status; the modal read raw `isDeployed` and + * `needsRedeployment`; the General tab decided "not deployed" from the *absence + * of a snapshot*. That last one is the defect that produced "Deploy your + * workflow to see a preview" sitting directly above a row reading `v1 (live)`: + * a missing snapshot is not evidence of anything, and rendering it as one made + * the modal contradict itself. + * + * Which is the same failure this PR fixes one layer down — several derivations + * of one fact, drifting — so it gets the same treatment: derive once, pass it + * down, and give the surfaces no raw material to re-derive from. + */ +export function useDeploymentViewState({ + workflowId, + enabled, + deployReadiness, +}: UseDeploymentViewStateProps): DeploymentViewState { + const { data: deploymentInfo } = useDeploymentInfo(workflowId, { enabled }) + const isDeployed = deploymentInfo?.isDeployed ?? false + + const snapshotEnabled = Boolean(workflowId) && isDeployed && enabled + const { data: deployedStateData, isLoading: isLoadingDeployedState } = useDeployedWorkflowState( + workflowId, + { enabled: snapshotEnabled } + ) + const deployedState = snapshotEnabled ? (deployedStateData ?? null) : null + + /* + * `isLoading` (no snapshot yet), NOT `isFetching`. A background refetch — which + * `refetchOnWindowFocus` fires on every focus — still has the cached snapshot + * to compare against, so treating it as loading blanked the answer and pushed + * an already-correct "Update" back through "Live" and out again. + */ + const { changeDetected, changedFields, isChangeDetectionSettling } = useChangeDetection({ + workflowId, + deployedState, + isLoadingDeployedState, + }) + + const serverNeedsRedeployment = snapshotEnabled ? deploymentInfo?.needsRedeployment : undefined + + const status = resolveDeployButtonStatus({ + workflowId, + isDeployed, + isAwaitingFirstDeployedState: isLoadingDeployedState, + clientChangeDetected: changeDetected, + hasDeployedState: deployedState !== null, + serverNeedsRedeployment, + }) + + const isSettling = isChangeDetectionSettling || deployReadiness.isSyncing + + useChangeDetectionCanary({ + workflowId, + clientChangeDetected: changeDetected, + clientChangedFields: changedFields, + serverNeedsRedeployment, + isSettling: isSettling || deployedState === null, + isSettled: deployReadiness.status === 'ready', + }) + + return { + status, + isDeployed, + deployedState, + isAwaitingSnapshot: snapshotEnabled && deployedState === null, + isSettling, + changeDetected, + changedFields, + } +} diff --git a/apps/sim/hooks/queries/deployments.ts b/apps/sim/hooks/queries/deployments.ts index 4b9bcb9531d..17fd2e77a6a 100644 --- a/apps/sim/hooks/queries/deployments.ts +++ b/apps/sim/hooks/queries/deployments.ts @@ -34,6 +34,8 @@ export type { ChatDetail, DeploymentVersionsResponse } export const DEPLOYMENT_INFO_STALE_TIME = 30 * 1000 export const DEPLOYMENT_STATUS_REFETCH_INTERVAL = 5 * 1000 export const DEPLOYED_WORKFLOW_STATE_STALE_TIME = 30 * 1000 +/** Retry cadence while the deployed snapshot is expected but not yet readable. */ +export const DEPLOYED_STATE_RECOVERY_INTERVAL = 3 * 1000 export const DEPLOYMENT_VERSIONS_STALE_TIME = 30 * 1000 export const CHAT_DEPLOYMENT_STATUS_STALE_TIME = 30 * 1000 export const CHAT_DETAIL_STALE_TIME = 30 * 1000 @@ -158,6 +160,19 @@ export function useDeployedWorkflowState( queryFn: ({ signal }) => fetchDeployedWorkflowState(workflowId!, signal), enabled: Boolean(workflowId) && (options?.enabled ?? true), staleTime: DEPLOYED_WORKFLOW_STATE_STALE_TIME, + /* + * Callers enable this only once deployment info reports the workflow as + * deployed, so a null snapshot is a contradiction rather than an answer: + * the version exists but is not readable yet. + * + * Nothing re-invalidates this key when activation cuts over — + * `refetchDeploymentBoundary` fires while the query is still disabled, and a + * disabled query cannot be refetched — so the null was cached for the whole + * stale window. That is what put an empty preview next to a live version + * row. Retrying resolves the contradiction and stops the instant it does. + */ + refetchInterval: (query) => + query.state.data == null ? DEPLOYED_STATE_RECOVERY_INTERVAL : false, }) } From f3c669abc00c53763310d2cc1182c2a58a6684bb Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 13:47:31 -0700 Subject: [PATCH 5/7] fix(deploy): stop the redeploy check checking out a second connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `checkNeedsRedeployment` opens a REPEATABLE READ transaction and then called `materializeDeploymentState` without a workspaceId, which resolves one through `getActiveWorkflowContext` — on the global pool. A transaction holding one connection while awaiting a second checkout starves the pool under any concurrency, and this endpoint is polled and refetches on window focus. The nested read then failed and surfaced as a 500 on `/api/workflows/[id]/deploy`, with the failing statement being the authz context lookup rather than anything the caller wrote. Introduced by the operand fix earlier on this branch. `materializeDeploymentState` now REQUIRES a workspaceId, so it cannot check out a connection at all and is safe inside any transaction by construction; the two non-transactional entry points resolve theirs through a named helper that says so. The type change surfaced both remaining callers rather than leaving the hazard to be avoided by convention. The UI half: an absent answer was rendered as a positive one, three times over. `isDeployed` is `deploymentInfo?.isDeployed ?? false`, so a pending OR FAILED request was indistinguishable from a genuinely undeployed workflow — the modal told the user to deploy a workflow whose version list showed v4 live. The status now reports `unknown` until deployment info actually answers, the chip is disabled there (a click is interpreted against that same flag: deployed opens the modal, undeployed deploys), and the General tab renders a skeleton rather than claiming undeployed whenever the live workflow cannot yet be shown. No retry or fallback: the earlier draft of this commit polled the snapshot query while it held no data, which papered over the pool starvation instead of fixing it. That is removed. Co-Authored-By: Claude Opus 5 (1M context) --- .../panel/components/deploy/deploy.tsx | 7 +++ .../hooks/use-deploy-button-status.test.ts | 60 +++++++++++++++++-- .../deploy/hooks/use-deploy-button-status.ts | 22 ++++++- .../deploy/hooks/use-deployment-view-state.ts | 10 +++- apps/sim/hooks/queries/deployments.ts | 15 ----- apps/sim/lib/workflows/deployment-status.ts | 22 ++++++- apps/sim/lib/workflows/persistence/utils.ts | 48 ++++++++++----- .../scripts/dump-change-detection-states.ts | 3 +- 8 files changed, 148 insertions(+), 39 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx index d303016cac4..866d244ef0d 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx @@ -54,6 +54,13 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: isDeploying || !canDeploy || isEmpty || + /* + * A click is interpreted against `isDeployed`: deployed opens the modal, + * undeployed deploys. While that is unknown the click has no defined + * meaning, and guessing "undeployed" would turn a failed info read into an + * unintended new version. + */ + buttonStatus === 'unknown' || (!isDeployed && deployReadiness.isBlocked && !deployReadiness.isSyncing) const onDeployClick = async () => { diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts index de14f109039..dec3d3ff753 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts @@ -11,6 +11,7 @@ type Input = Parameters[0] const base: Input = { workflowId: 'wf-1', + isDeploymentInfoResolved: false, isDeployed: false, isAwaitingFirstDeployedState: false, clientChangeDetected: false, @@ -39,9 +40,15 @@ describe('resolveDeployButtonStatus', () => { // 1. Nothing loaded. {}, // 2. deploymentInfo lands — isDeployed and needsRedeployment arrive together. - { isDeployed: true, serverNeedsRedeployment: true, isAwaitingFirstDeployedState: true }, + { + isDeploymentInfoResolved: true, + isDeployed: true, + serverNeedsRedeployment: true, + isAwaitingFirstDeployedState: true, + }, // 3. The deployed snapshot lands; the client diff agrees. { + isDeploymentInfoResolved: true, isDeployed: true, serverNeedsRedeployment: true, hasDeployedState: true, @@ -49,18 +56,28 @@ describe('resolveDeployButtonStatus', () => { }, ]) - expect(statuses).toEqual(['undeployed', 'changed']) + expect(statuses).toEqual(['unknown', 'changed']) expect(statuses).not.toContain('live') }) it('settles straight to live for a deployed workflow with no changes', () => { const statuses = committed([ {}, - { isDeployed: true, serverNeedsRedeployment: false, isAwaitingFirstDeployedState: true }, - { isDeployed: true, serverNeedsRedeployment: false, hasDeployedState: true }, + { + isDeploymentInfoResolved: true, + isDeployed: true, + serverNeedsRedeployment: false, + isAwaitingFirstDeployedState: true, + }, + { + isDeploymentInfoResolved: true, + isDeployed: true, + serverNeedsRedeployment: false, + hasDeployedState: true, + }, ]) - expect(statuses).toEqual(['undeployed', 'live']) + expect(statuses).toEqual(['unknown', 'live']) expect(statuses).not.toContain('changed') }) @@ -70,6 +87,7 @@ describe('resolveDeployButtonStatus', () => { */ it('holds its answer across a background refetch', () => { const settled: Partial = { + isDeploymentInfoResolved: true, isDeployed: true, serverNeedsRedeployment: true, hasDeployedState: true, @@ -90,6 +108,7 @@ describe('resolveDeployButtonStatus', () => { // Unsaved edits: the server still describes the persisted draft. const status = resolveDeployButtonStatus({ ...base, + isDeploymentInfoResolved: true, isDeployed: true, serverNeedsRedeployment: false, hasDeployedState: true, @@ -103,9 +122,40 @@ describe('resolveDeployButtonStatus', () => { expect(resolveDeployButtonStatus({ ...base, workflowId: null })).toBe('undeployed') }) + /** + * `GET /api/workflows/[id]/deploy` returning 500 made `isDeployed` default to + * false, which rendered a live workflow as "Deploy" beside a version list + * showing v4 live — an absence of information presented as a fact. + * + * The click is also interpreted against the same flag (deployed opens the + * modal, undeployed deploys), so guessing here decides an action, not just a + * label. Both reasons say the same thing: do not answer until asked. + */ + it('does not claim undeployed when deployment info has not answered', () => { + const status = resolveDeployButtonStatus({ + ...base, + isDeploymentInfoResolved: false, + isDeployed: false, + }) + + expect(status).toBe('unknown') + expect(status).not.toBe('undeployed') + }) + + it('reports undeployed only once info has actually said so', () => { + const status = resolveDeployButtonStatus({ + ...base, + isDeploymentInfoResolved: true, + isDeployed: false, + }) + + expect(status).toBe('undeployed') + }) + it('falls back to unknown only when deployed with no verdict from either side', () => { const status = resolveDeployButtonStatus({ ...base, + isDeploymentInfoResolved: true, isDeployed: true, isAwaitingFirstDeployedState: true, serverNeedsRedeployment: undefined, diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts index 0da2ce2f46f..b58f6bdc828 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts @@ -10,6 +10,12 @@ export type DeployButtonStatus = 'unknown' | 'undeployed' | 'live' | 'changed' interface ResolveDeployButtonStatusInput { workflowId: string | null + /** + * Whether deployment info has actually answered. `isDeployed` defaults to + * `false` while the request is pending OR failed, so without this the two are + * indistinguishable and a 500 reads as "not deployed". + */ + isDeploymentInfoResolved: boolean isDeployed: boolean /** True only before the FIRST deployed snapshot lands — never on a refetch. */ isAwaitingFirstDeployedState: boolean @@ -49,13 +55,27 @@ interface ResolveDeployButtonStatusInput { */ export function resolveDeployButtonStatus({ workflowId, + isDeploymentInfoResolved, isDeployed, isAwaitingFirstDeployedState, clientChangeDetected, hasDeployedState, serverNeedsRedeployment, }: ResolveDeployButtonStatusInput): DeployButtonStatus { - if (!workflowId || !isDeployed) return 'undeployed' + if (!workflowId) return 'undeployed' + + /* + * Not knowing is its own answer. `isDeployed` is `deploymentInfo?.isDeployed + * ?? false`, so a pending or FAILED info request is indistinguishable from a + * genuinely undeployed workflow — and a transient 500 on that endpoint + * rendered a live workflow as "Deploy", next to a version list showing v4 + * live. Worse, the chip acts on that: with `isDeployed` false a click runs a + * fresh deploy instead of opening the modal, so a failed read could be + * converted into an unintended new version. + */ + if (!isDeploymentInfoResolved) return 'unknown' + + if (!isDeployed) return 'undeployed' if (hasDeployedState && !isAwaitingFirstDeployedState) { return clientChangeDetected ? 'changed' : 'live' diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts index 9b5e6e0ef30..dfe78cba9b0 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts @@ -52,6 +52,8 @@ export function useDeploymentViewState({ deployReadiness, }: UseDeploymentViewStateProps): DeploymentViewState { const { data: deploymentInfo } = useDeploymentInfo(workflowId, { enabled }) + /* Undefined covers both "still loading" and "the request failed". */ + const isDeploymentInfoResolved = deploymentInfo !== undefined const isDeployed = deploymentInfo?.isDeployed ?? false const snapshotEnabled = Boolean(workflowId) && isDeployed && enabled @@ -77,6 +79,7 @@ export function useDeploymentViewState({ const status = resolveDeployButtonStatus({ workflowId, + isDeploymentInfoResolved, isDeployed, isAwaitingFirstDeployedState: isLoadingDeployedState, clientChangeDetected: changeDetected, @@ -99,7 +102,12 @@ export function useDeploymentViewState({ status, isDeployed, deployedState, - isAwaitingSnapshot: snapshotEnabled && deployedState === null, + /* + * "We cannot show you the live workflow yet" covers both a snapshot in + * flight and not knowing whether one exists. Neither is evidence the + * workflow is undeployed, so neither may render as that claim. + */ + isAwaitingSnapshot: status === 'unknown' || (snapshotEnabled && deployedState === null), isSettling, changeDetected, changedFields, diff --git a/apps/sim/hooks/queries/deployments.ts b/apps/sim/hooks/queries/deployments.ts index 17fd2e77a6a..4b9bcb9531d 100644 --- a/apps/sim/hooks/queries/deployments.ts +++ b/apps/sim/hooks/queries/deployments.ts @@ -34,8 +34,6 @@ export type { ChatDetail, DeploymentVersionsResponse } export const DEPLOYMENT_INFO_STALE_TIME = 30 * 1000 export const DEPLOYMENT_STATUS_REFETCH_INTERVAL = 5 * 1000 export const DEPLOYED_WORKFLOW_STATE_STALE_TIME = 30 * 1000 -/** Retry cadence while the deployed snapshot is expected but not yet readable. */ -export const DEPLOYED_STATE_RECOVERY_INTERVAL = 3 * 1000 export const DEPLOYMENT_VERSIONS_STALE_TIME = 30 * 1000 export const CHAT_DEPLOYMENT_STATUS_STALE_TIME = 30 * 1000 export const CHAT_DETAIL_STALE_TIME = 30 * 1000 @@ -160,19 +158,6 @@ export function useDeployedWorkflowState( queryFn: ({ signal }) => fetchDeployedWorkflowState(workflowId!, signal), enabled: Boolean(workflowId) && (options?.enabled ?? true), staleTime: DEPLOYED_WORKFLOW_STATE_STALE_TIME, - /* - * Callers enable this only once deployment info reports the workflow as - * deployed, so a null snapshot is a contradiction rather than an answer: - * the version exists but is not readable yet. - * - * Nothing re-invalidates this key when activation cuts over — - * `refetchDeploymentBoundary` fires while the query is still disabled, and a - * disabled query cannot be refetched — so the null was cached for the whole - * stale window. That is what put an empty preview next to a live version - * row. Retrying resolves the contradiction and stops the instant it does. - */ - refetchInterval: (query) => - query.state.data == null ? DEPLOYED_STATE_RECOVERY_INTERVAL : false, }) } diff --git a/apps/sim/lib/workflows/deployment-status.ts b/apps/sim/lib/workflows/deployment-status.ts index ae641b17079..06ba36f6d7e 100644 --- a/apps/sim/lib/workflows/deployment-status.ts +++ b/apps/sim/lib/workflows/deployment-status.ts @@ -1,4 +1,5 @@ import { db, workflowDeploymentVersion } from '@sim/db' +import { workflow as workflowTable } from '@sim/db/schema' import { and, desc, eq, sql } from 'drizzle-orm' import { hasWorkflowChanged } from '@/lib/workflows/comparison' import { @@ -22,12 +23,23 @@ import type { WorkflowState } from '@/stores/workflows/workflow/types' export async function checkNeedsRedeployment(workflowId: string): Promise { return db.transaction(async (tx) => { await tx.execute(sql`SET TRANSACTION ISOLATION LEVEL REPEATABLE READ`) + /* + * `workspaceId` is selected here, in this transaction, rather than left for + * `materializeDeploymentState` to look up. It resolves an absent one through + * `getActiveWorkflowContext`, which runs on the global pool — a second + * connection checkout while this transaction already holds one. Under any + * concurrency (this endpoint is polled, and refetches on window focus) that + * starves the pool and fails the nested read, surfacing as a 500 on + * `/api/workflows/[id]/deploy`. A transaction must not await a checkout. + */ const [active] = await tx .select({ id: workflowDeploymentVersion.id, state: workflowDeploymentVersion.state, + workspaceId: workflowTable.workspaceId, }) .from(workflowDeploymentVersion) + .innerJoin(workflowTable, eq(workflowTable.id, workflowDeploymentVersion.workflowId)) .where( and( eq(workflowDeploymentVersion.workflowId, workflowId), @@ -37,7 +49,8 @@ export async function checkNeedsRedeployment(workflowId: string): Promise { + if (provided) return provided + const workflowContext = await getActiveWorkflowContext(workflowId) + if (!workflowContext?.workspaceId) { + throw new Error(`Workflow ${workflowId} has no workspace`) + } + return workflowContext.workspaceId +} + +interface DeploymentStateRow { id: string state: unknown } @@ -160,10 +173,18 @@ export interface DeploymentStateRow { * the handle canonicalization and the `errorEnabled` backfill below, the two * surfaces answered the same question differently for the same workflow. */ +/** + * `workspaceId` is required rather than resolved here on purpose. Resolving it + * means `getActiveWorkflowContext`, which runs on the global pool — and this + * function is called from inside a REPEATABLE READ transaction by + * `checkNeedsRedeployment`, where a second connection checkout while holding one + * starves the pool under concurrency and fails the nested read. Taking it as an + * argument makes that impossible instead of merely avoided. + */ export async function materializeDeploymentState( workflowId: string, version: DeploymentStateRow, - providedWorkspaceId?: string, + workspaceId: string, executor?: DbOrTx ): Promise { const cached = deployedStateCache.get(version.id) @@ -172,19 +193,10 @@ export async function materializeDeploymentState( } const state = version.state as WorkflowState & { variables?: Record } - let resolvedWorkspaceId = providedWorkspaceId - if (!resolvedWorkspaceId) { - const workflowContext = await getActiveWorkflowContext(workflowId) - resolvedWorkspaceId = workflowContext?.workspaceId - } - - if (!resolvedWorkspaceId) { - throw new Error(`Workflow ${workflowId} has no workspace`) - } const { blocks: migratedBlocks } = await applyBlockMigrations( state.blocks || {}, - resolvedWorkspaceId, + workspaceId, executor ) /* @@ -255,7 +267,11 @@ export async function loadDeployedWorkflowState( throw new NoActiveDeploymentError(workflowId) } - return materializeDeploymentState(workflowId, active, providedWorkspaceId) + return materializeDeploymentState( + workflowId, + active, + await resolveWorkspaceId(workflowId, providedWorkspaceId) + ) } catch (error) { logger.error(`Error loading deployed workflow state ${workflowId}:`, error) throw error @@ -288,7 +304,11 @@ export async function loadWorkflowDeploymentVersionState( throw new Error(`Deployment ${deploymentVersionId} was not found for workflow ${workflowId}`) } - return materializeDeploymentState(workflowId, version, providedWorkspaceId) + return materializeDeploymentState( + workflowId, + version, + await resolveWorkspaceId(workflowId, providedWorkspaceId) + ) } interface MigrationContext { diff --git a/apps/sim/scripts/dump-change-detection-states.ts b/apps/sim/scripts/dump-change-detection-states.ts index d4d4ae57171..8233632f6d7 100644 --- a/apps/sim/scripts/dump-change-detection-states.ts +++ b/apps/sim/scripts/dump-change-detection-states.ts @@ -186,13 +186,14 @@ async function main(): Promise { const webhooks = webhooksByWorkflow.get(row.workflowId) if (webhooksOnly && !webhooks) continue + if (!row.workspaceId) continue const current = await loadWorkflowDeploymentSnapshot(row.workflowId) if (!current) continue const deployed = await materializeDeploymentState( row.workflowId, { id: row.versionId, state: row.state }, - row.workspaceId ?? undefined + row.workspaceId ) const currentBlocks = From 9a199c3aa12ca9a1994f1c1937bf5610894d53f5 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 13:52:12 -0700 Subject: [PATCH 6/7] docs(deploy): name the tripwire as the mechanism, not pool starvation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit attributed the 500 to pool starvation under concurrency. That was wrong. `packages/db/tx-tripwire.ts` marks the async context for the duration of a transaction callback and reports any query issued on the global pool inside it — throwing outside production, warning in production. So the failure was deterministic in dev, not load-dependent, which is why it reported against the authz lookup rather than anything the caller wrote. Saturation deadlock is what the tripwire exists to PREVENT, not what happened. Recording the real mechanism, since the wrong one would send the next reader looking for a concurrency bug. Also drops a comment claiming a transaction connection "cannot serve concurrent statements". `loadWorkflowDeploymentSnapshot` issues two tx-handle reads under `Promise.all` and has always worked, so the claim is false; the sequential reads stay because they are clearer, not because concurrency is unsafe. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/lib/workflows/deployment-status.ts | 19 +++++++++---------- apps/sim/lib/workflows/persistence/utils.ts | 10 +++++----- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/apps/sim/lib/workflows/deployment-status.ts b/apps/sim/lib/workflows/deployment-status.ts index 06ba36f6d7e..5931625a7d1 100644 --- a/apps/sim/lib/workflows/deployment-status.ts +++ b/apps/sim/lib/workflows/deployment-status.ts @@ -25,12 +25,15 @@ export async function checkNeedsRedeployment(workflowId: string): Promise Date: Sat, 22 Aug 2026 13:57:13 -0700 Subject: [PATCH 7/7] fix(deploy): keep the workspace-id resolver module-private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A mechanical edit stranded an `export` onto `resolveWorkspaceId` and stripped it from `DeploymentStateRow`. It type-checked because the call sites pass the row structurally, so nothing caught it. The export is the part that matters: this helper queries the global pool, so calling it inside a transaction callback is exactly the nested checkout the tripwire throws on — and exporting it invited a caller to do that from somewhere already holding a connection. That widened the surface the previous commit narrowed by making `materializeDeploymentState` require a `workspaceId`. `DeploymentStateRow` stays unexported: nothing imports it, and the call sites satisfy it structurally. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/lib/workflows/persistence/utils.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/apps/sim/lib/workflows/persistence/utils.ts b/apps/sim/lib/workflows/persistence/utils.ts index 1816571b271..4aa02891dff 100644 --- a/apps/sim/lib/workflows/persistence/utils.ts +++ b/apps/sim/lib/workflows/persistence/utils.ts @@ -145,9 +145,12 @@ export function invalidateDeployedStateCache(deploymentVersionId?: string): void deployedStateCache.clear() } -export /** - * Only for entry points that are NOT inside a transaction — it checks out a - * connection of its own. +/** + * Deliberately module-private: it queries the global pool, so calling it inside + * a transaction callback is the nested checkout `packages/db/tx-tripwire.ts` + * throws on. Keeping it unexported is what stops a future caller reaching for it + * from somewhere that already holds a connection — the same reasoning that made + * `materializeDeploymentState` take a `workspaceId` instead of resolving one. */ async function resolveWorkspaceId(workflowId: string, provided?: string): Promise { if (provided) return provided