Skip to content

Commit 1cca93f

Browse files
icecrasher321claude
andcommitted
fix(deploy): resolve subblock values to their configuration on both sides of change detection
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 — 3c29476, 41b6804, 066e18a, 4f722c6, 5ece9f9, ff23546, 01577a1, 3cc9b1a and 8806508 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) <noreply@anthropic.com>
1 parent a721a76 commit 1cca93f

22 files changed

Lines changed: 1632 additions & 314 deletions

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ import { Chip, Tooltip, toast } from '@sim/emcn'
55
import { useRegisterGlobalCommands } from '@/app/workspace/[workspaceId]/providers/global-commands-provider'
66
import { DeployModal } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal'
77
import {
8+
resolveDeployButtonStatus,
89
useChangeDetection,
10+
useChangeDetectionCanary,
911
useDeployment,
1012
useDeployReadiness,
1113
} from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks'
@@ -40,13 +42,42 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }:
4042
const deployedState = isDeployedStateEnabled ? (deployedStateData ?? null) : null
4143
const deployReadiness = useDeployReadiness(activeWorkflowId)
4244

43-
const { changeDetected, isChangeDetectionSettling } = useChangeDetection({
45+
/*
46+
* `isLoading` (no snapshot yet), NOT `isFetching`. A background refetch — which
47+
* `refetchOnWindowFocus` fires on every focus — still has the cached snapshot
48+
* to compare against, so treating it as loading blanked the answer and pushed
49+
* an already-correct "Update" back through "Live" and out again.
50+
*/
51+
const { changeDetected, changedFields, isChangeDetectionSettling } = useChangeDetection({
4452
workflowId: activeWorkflowId,
4553
deployedState,
46-
isLoadingDeployedState: isLoadingDeployedState || isFetchingDeployedState,
54+
isLoadingDeployedState,
4755
})
4856
const isDeploymentSettling = isChangeDetectionSettling || deployReadiness.isSyncing
4957

58+
const serverNeedsRedeployment = isDeployedStateEnabled
59+
? deploymentInfo?.needsRedeployment
60+
: undefined
61+
62+
const buttonStatus = resolveDeployButtonStatus({
63+
workflowId: activeWorkflowId,
64+
isDeployed,
65+
isAwaitingFirstDeployedState: isLoadingDeployedState,
66+
clientChangeDetected: changeDetected,
67+
hasDeployedState: deployedState !== null,
68+
serverNeedsRedeployment,
69+
})
70+
const changeDetectedForModal = buttonStatus === 'changed'
71+
72+
useChangeDetectionCanary({
73+
workflowId: activeWorkflowId,
74+
clientChangeDetected: changeDetected,
75+
clientChangedFields: changedFields,
76+
serverNeedsRedeployment,
77+
isSettling: isDeploymentSettling || deployedState === null,
78+
isSettled: deployReadiness.status === 'ready',
79+
})
80+
5081
const { isDeploying, handleDeployClick } = useDeployment({
5182
workflowId: activeWorkflowId,
5283
isDeployed,
@@ -110,23 +141,28 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }:
110141
if (deployReadiness.isBlocked && !isDeployed) {
111142
return deployReadiness.tooltip
112143
}
113-
if (changeDetected) {
144+
if (buttonStatus === 'changed') {
114145
return 'Update deployment'
115146
}
116-
if (isDeployed) {
147+
if (buttonStatus === 'live') {
117148
return 'Active deployment'
118149
}
119150
return 'Deploy workflow'
120151
}
121152

122153
const getButtonLabel = () => {
123-
if (changeDetected) {
124-
return 'Update'
125-
}
126-
if (isDeployed) {
127-
return 'Live'
154+
switch (buttonStatus) {
155+
case 'changed':
156+
return 'Update'
157+
case 'live':
158+
return 'Live'
159+
/*
160+
* Only reachable before we know the workflow is deployed, so "Deploy" is
161+
* the answer rather than a guess we would have to take back.
162+
*/
163+
default:
164+
return 'Deploy'
128165
}
129-
return 'Deploy'
130166
}
131167

132168
return (
@@ -151,7 +187,7 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }:
151187
onOpenChange={setIsModalOpen}
152188
workflowId={activeWorkflowId}
153189
isDeployed={isDeployed}
154-
needsRedeployment={changeDetected}
190+
needsRedeployment={changeDetectedForModal}
155191
deployedState={deployedState}
156192
isLoadingDeployedState={isLoadingDeployedState || isFetchingDeployedState}
157193
deployReadiness={deployReadiness}
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
export { useChangeDetection } from './use-change-detection'
2+
export { useChangeDetectionCanary } from './use-change-detection-canary'
3+
export type { DeployButtonStatus } from './use-deploy-button-status'
4+
export { resolveDeployButtonStatus } from './use-deploy-button-status'
25
export type { DeployReadiness } from './use-deploy-readiness'
36
export { getDeployReadinessState, useDeployReadiness } from './use-deploy-readiness'
47
export { useDeployment } from './use-deployment'
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import { useEffect, useRef } from 'react'
2+
import { createLogger } from '@sim/logger'
3+
4+
const logger = createLogger('ChangeDetectionCanary')
5+
6+
interface UseChangeDetectionCanaryProps {
7+
workflowId: string | null
8+
/** The client's in-memory answer, from `useChangeDetection`. */
9+
clientChangeDetected: boolean
10+
/** The fields the client's answer rests on, for attribution. */
11+
clientChangedFields: string[]
12+
/** The server's answer, already fetched by `useDeploymentInfo`. */
13+
serverNeedsRedeployment: boolean | undefined
14+
/** True while either operand is still loading — a disagreement means nothing yet. */
15+
isSettling: boolean
16+
/** True only when the operation queue is drained and no diff/reconcile is pending. */
17+
isSettled: boolean
18+
}
19+
20+
/**
21+
* Reports when the client and the server disagree about whether a workflow needs
22+
* redeploying.
23+
*
24+
* The two answers are computed from the same comparison over operands that are
25+
* supposed to be equivalent: the server diffs the durable draft against the
26+
* active deployment version, and the client diffs its merged in-memory state
27+
* against the same version. Once the operation queue has drained they must
28+
* agree, so a disagreement is a divergence between the client's state and what
29+
* was actually persisted — the signature of every phantom "Update" this codebase
30+
* has shipped.
31+
*
32+
* Costs nothing: `useDeploymentInfo` already fetches the server's answer for the
33+
* `isDeployed` flag, and the client's answer is already computed for the button.
34+
* Discarding both is why ten instances of this bug class were found by users
35+
* rather than by us.
36+
*/
37+
export function useChangeDetectionCanary({
38+
workflowId,
39+
clientChangeDetected,
40+
clientChangedFields,
41+
serverNeedsRedeployment,
42+
isSettling,
43+
isSettled,
44+
}: UseChangeDetectionCanaryProps): void {
45+
/** Reported once per (workflow, verdict pair) so a steady disagreement logs once. */
46+
const reportedRef = useRef<string | null>(null)
47+
48+
useEffect(() => {
49+
if (!workflowId || isSettling || !isSettled || serverNeedsRedeployment === undefined) {
50+
return
51+
}
52+
53+
if (serverNeedsRedeployment === clientChangeDetected) {
54+
reportedRef.current = null
55+
return
56+
}
57+
58+
const signature = `${workflowId}:${serverNeedsRedeployment}:${clientChangeDetected}`
59+
if (reportedRef.current === signature) return
60+
reportedRef.current = signature
61+
62+
logger.warn('Change detection disagrees with the server', {
63+
workflowId,
64+
serverNeedsRedeployment,
65+
clientChangeDetected,
66+
/*
67+
* Only populated when the CLIENT sees changes. The inverse case — the
68+
* server sees changes the client does not — reports an empty list, and
69+
* that asymmetry is itself the diagnosis: the client's merged state
70+
* matches the deployment while the persisted draft does not.
71+
*/
72+
clientChangedFields,
73+
})
74+
}, [
75+
workflowId,
76+
clientChangeDetected,
77+
clientChangedFields,
78+
serverNeedsRedeployment,
79+
isSettling,
80+
isSettled,
81+
])
82+
}

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-change-detection.ts

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,43 @@
11
import { useMemo } from 'react'
22
import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks'
3-
import { hasWorkflowChanged } from '@/lib/workflows/comparison'
3+
import { generateWorkflowDiffSummary } from '@/lib/workflows/comparison'
44
import { useVariablesStore } from '@/stores/variables/store'
55
import { useSubBlockStore } from '@/stores/workflows/subblock/store'
66
import { useWorkflowStore } from '@/stores/workflows/workflow/store'
77
import type { WorkflowState } from '@/stores/workflows/workflow/types'
88

9+
/** Stable identity so an unchanged workflow does not hand consumers a fresh array. */
10+
const EMPTY_FIELDS: string[] = []
11+
912
interface UseChangeDetectionProps {
1013
workflowId: string | null
1114
deployedState: WorkflowState | null
1215
isLoadingDeployedState: boolean
1316
}
1417

18+
interface UseChangeDetectionResult {
19+
changeDetected: boolean
20+
/**
21+
* The field names behind `changeDetected`, for diagnostics only — never for
22+
* rendering. Free: `hasWorkflowChanged` is `generateWorkflowDiffSummary(…).hasChanges`,
23+
* so the summary is computed either way and throwing it away only hid which
24+
* fields drove a redeploy prompt.
25+
*/
26+
changedFields: string[]
27+
isChangeDetectionSettling: boolean
28+
}
29+
1530
/**
1631
* Detects meaningful changes between current workflow state and deployed state.
17-
* Performs comparison entirely on the client using hasWorkflowChanged — no API
18-
* calls needed. The deployed state snapshot is fetched once via React Query and
19-
* refreshed after deploy/undeploy/version-activate mutations.
32+
* Performs comparison entirely on the client using generateWorkflowDiffSummary —
33+
* no API calls needed. The deployed state snapshot is fetched once via React Query
34+
* and refreshed after deploy/undeploy/version-activate mutations.
2035
*/
2136
export function useChangeDetection({
2237
workflowId,
2338
deployedState,
2439
isLoadingDeployedState,
25-
}: UseChangeDetectionProps) {
40+
}: UseChangeDetectionProps): UseChangeDetectionResult {
2641
const blocks = useWorkflowStore((state) => state.blocks)
2742
const edges = useWorkflowStore((state) => state.edges)
2843
const loops = useWorkflowStore((state) => state.loops)
@@ -65,13 +80,35 @@ export function useChangeDetection({
6580
workflowVariables,
6681
])
6782

68-
const changeDetected = useMemo(() => {
69-
if (!currentState || !deployedState || isLoadingDeployedState) return false
70-
return hasWorkflowChanged(currentState, deployedState)
83+
const { changeDetected, changedFields } = useMemo(() => {
84+
if (!currentState || !deployedState || isLoadingDeployedState) {
85+
return { changeDetected: false, changedFields: EMPTY_FIELDS }
86+
}
87+
88+
const summary = generateWorkflowDiffSummary(currentState, deployedState)
89+
if (!summary.hasChanges) {
90+
return { changeDetected: false, changedFields: EMPTY_FIELDS }
91+
}
92+
93+
const fields = new Set<string>()
94+
for (const block of summary.modifiedBlocks) {
95+
for (const change of block.changes) {
96+
fields.add(`${block.type}.${change.field}`)
97+
}
98+
}
99+
for (const block of summary.addedBlocks) fields.add(`+block:${block.type}`)
100+
for (const block of summary.removedBlocks) fields.add(`-block:${block.type}`)
101+
if (summary.edgeChanges.added > 0 || summary.edgeChanges.removed > 0) fields.add('edges')
102+
if (summary.loopChanges.modified > 0) fields.add('loops')
103+
if (summary.parallelChanges.modified > 0) fields.add('parallels')
104+
if (summary.variableChanges.modified > 0) fields.add('variables')
105+
106+
return { changeDetected: true, changedFields: [...fields] }
71107
}, [currentState, deployedState, isLoadingDeployedState])
72108

73109
return {
74110
changeDetected,
111+
changedFields,
75112
isChangeDetectionSettling: Boolean(workflowId && isLoadingDeployedState),
76113
}
77114
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import {
6+
type DeployButtonStatus,
7+
resolveDeployButtonStatus,
8+
} from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status'
9+
10+
type Input = Parameters<typeof resolveDeployButtonStatus>[0]
11+
12+
const base: Input = {
13+
workflowId: 'wf-1',
14+
isDeployed: false,
15+
isAwaitingFirstDeployedState: false,
16+
clientChangeDetected: false,
17+
hasDeployedState: false,
18+
serverNeedsRedeployment: undefined,
19+
}
20+
21+
/** Replays a render sequence and returns the labels actually committed, deduped. */
22+
function committed(sequence: Array<Partial<Input>>): DeployButtonStatus[] {
23+
const seen: DeployButtonStatus[] = []
24+
for (const step of sequence) {
25+
const status = resolveDeployButtonStatus({ ...base, ...step })
26+
if (seen[seen.length - 1] !== status) seen.push(status)
27+
}
28+
return seen
29+
}
30+
31+
describe('resolveDeployButtonStatus', () => {
32+
/**
33+
* The regression this exists for. The old label read `changeDetected`, which
34+
* is forced false while the deployed snapshot loads, so a changed workflow
35+
* rendered "Live" on the way to "Update".
36+
*/
37+
it('never passes through live when loading a workflow that has changes', () => {
38+
const statuses = committed([
39+
// 1. Nothing loaded.
40+
{},
41+
// 2. deploymentInfo lands — isDeployed and needsRedeployment arrive together.
42+
{ isDeployed: true, serverNeedsRedeployment: true, isAwaitingFirstDeployedState: true },
43+
// 3. The deployed snapshot lands; the client diff agrees.
44+
{
45+
isDeployed: true,
46+
serverNeedsRedeployment: true,
47+
hasDeployedState: true,
48+
clientChangeDetected: true,
49+
},
50+
])
51+
52+
expect(statuses).toEqual(['undeployed', 'changed'])
53+
expect(statuses).not.toContain('live')
54+
})
55+
56+
it('settles straight to live for a deployed workflow with no changes', () => {
57+
const statuses = committed([
58+
{},
59+
{ isDeployed: true, serverNeedsRedeployment: false, isAwaitingFirstDeployedState: true },
60+
{ isDeployed: true, serverNeedsRedeployment: false, hasDeployedState: true },
61+
])
62+
63+
expect(statuses).toEqual(['undeployed', 'live'])
64+
expect(statuses).not.toContain('changed')
65+
})
66+
67+
/**
68+
* `refetchOnWindowFocus` is on for both queries, so this fires on every focus.
69+
* A refetch keeps the cached snapshot, so the answer must not move.
70+
*/
71+
it('holds its answer across a background refetch', () => {
72+
const settled: Partial<Input> = {
73+
isDeployed: true,
74+
serverNeedsRedeployment: true,
75+
hasDeployedState: true,
76+
clientChangeDetected: true,
77+
}
78+
79+
const statuses = committed([
80+
settled,
81+
// Refetching: data is still cached, so `isAwaitingFirstDeployedState` stays false.
82+
settled,
83+
settled,
84+
])
85+
86+
expect(statuses).toEqual(['changed'])
87+
})
88+
89+
it('prefers the client diff over the server seed once a snapshot exists', () => {
90+
// Unsaved edits: the server still describes the persisted draft.
91+
const status = resolveDeployButtonStatus({
92+
...base,
93+
isDeployed: true,
94+
serverNeedsRedeployment: false,
95+
hasDeployedState: true,
96+
clientChangeDetected: true,
97+
})
98+
99+
expect(status).toBe('changed')
100+
})
101+
102+
it('reports undeployed without a workflow', () => {
103+
expect(resolveDeployButtonStatus({ ...base, workflowId: null })).toBe('undeployed')
104+
})
105+
106+
it('falls back to unknown only when deployed with no verdict from either side', () => {
107+
const status = resolveDeployButtonStatus({
108+
...base,
109+
isDeployed: true,
110+
isAwaitingFirstDeployedState: true,
111+
serverNeedsRedeployment: undefined,
112+
})
113+
114+
expect(status).toBe('unknown')
115+
})
116+
})

0 commit comments

Comments
 (0)