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 fc2ae3461c2..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 @@ -5,12 +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 { - useChangeDetection, 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' @@ -26,26 +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) - const { changeDetected, isChangeDetectionSettling } = useChangeDetection({ + /* + * 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 deployment = useDeploymentViewState({ workflowId: activeWorkflowId, - deployedState, - isLoadingDeployedState: isLoadingDeployedState || isFetchingDeployedState, + enabled: !isRegistryLoading, + deployReadiness, }) - const isDeploymentSettling = isChangeDetectionSettling || deployReadiness.isSyncing + const { status: buttonStatus, isDeployed, deployedState } = deployment + const isDeploymentSettling = deployment.isSettling const { isDeploying, handleDeployClick } = useDeployment({ workflowId: activeWorkflowId, @@ -60,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 () => { @@ -104,29 +105,50 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: if (isDeploying) { return 'Deploying...' } - if (isChangeDetectionSettling) { + if (isDeploymentSettling) { return 'Syncing deployment state...' } 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' + /* + * 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...' } - 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 ( @@ -150,12 +172,8 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }: open={isModalOpen} onOpenChange={setIsModalOpen} workflowId={activeWorkflowId} - isDeployed={isDeployed} - needsRedeployment={changeDetected} - 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 171b8718bbc..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 @@ -1,4 +1,9 @@ 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' +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-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..dec3d3ff753 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts @@ -0,0 +1,166 @@ +/** + * @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', + isDeploymentInfoResolved: false, + 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. + { + 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, + clientChangeDetected: true, + }, + ]) + + 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([ + {}, + { + isDeploymentInfoResolved: true, + isDeployed: true, + serverNeedsRedeployment: false, + isAwaitingFirstDeployedState: true, + }, + { + isDeploymentInfoResolved: true, + isDeployed: true, + serverNeedsRedeployment: false, + hasDeployedState: true, + }, + ]) + + expect(statuses).toEqual(['unknown', '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 = { + isDeploymentInfoResolved: true, + 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, + isDeploymentInfoResolved: true, + 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') + }) + + /** + * `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, + }) + + 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..b58f6bdc828 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts @@ -0,0 +1,89 @@ +/** + * 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 + /** + * 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 + /** 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, + isDeploymentInfoResolved, + isDeployed, + isAwaitingFirstDeployedState, + clientChangeDetected, + hasDeployedState, + serverNeedsRedeployment, +}: ResolveDeployButtonStatusInput): DeployButtonStatus { + 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' + } + + if (serverNeedsRedeployment !== undefined) { + return serverNeedsRedeployment ? 'changed' : 'live' + } + + return 'unknown' +} 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..dfe78cba9b0 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts @@ -0,0 +1,115 @@ +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 }) + /* Undefined covers both "still loading" and "the request failed". */ + const isDeploymentInfoResolved = deploymentInfo !== undefined + 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, + isDeploymentInfoResolved, + 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, + /* + * "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 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..8232f244a56 --- /dev/null +++ b/apps/sim/lib/workflows/canonical/reported-bug.test.ts @@ -0,0 +1,119 @@ +/** + * @vitest-environment node + * + * 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 + * 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' + +/** + * 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: 'webhook-block', + type: 'generic_webhook', + name: 'Webhook', + 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..5931625a7d1 100644 --- a/apps/sim/lib/workflows/deployment-status.ts +++ b/apps/sim/lib/workflows/deployment-status.ts @@ -1,25 +1,48 @@ 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 { 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`) + /* + * `workspaceId` is selected here, in this transaction, rather than left for + * `materializeDeploymentState` to look up: resolving an absent one goes + * through `getActiveWorkflowContext`, which queries the global pool, and + * this callback already holds a pooled connection. + * + * `packages/db/tx-tripwire.ts` detects exactly that and throws outside + * production, so it did not degrade quietly — it 500'd every + * `/api/workflows/[id]/deploy` in dev, reported against the authz lookup + * rather than anything this function wrote. Hoisting the read into the + * transaction is the tripwire's own first remedy. + */ const [active] = await tx - .select({ state: workflowDeploymentVersion.state }) + .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), @@ -29,7 +52,19 @@ 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 } -async function materializeDeploymentState( +/** + * Projects a deployment version's frozen jsonb into the shape change detection + * compares against. + * + * Exported because both sides of "needs redeploy" must be materialized the same + * way. The client asks through `/api/workflows/[id]/deployed`; the server asks + * through `checkNeedsRedeployment`. When only one of them ran the migrations, + * 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 queries the global pool, and + * `checkNeedsRedeployment` calls this from inside a REPEATABLE READ transaction + * that already holds a pooled connection — the nested checkout + * `packages/db/tx-tripwire.ts` exists to catch. Taking the id as an argument + * makes the violation unrepresentable rather than merely avoided. + */ +export async function materializeDeploymentState( workflowId: string, version: DeploymentStateRow, - providedWorkspaceId?: string + workspaceId: string, + executor?: DbOrTx ): Promise { const cached = deployedStateCache.get(version.id) if (cached) { @@ -161,19 +196,11 @@ 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 ) /* * Read straight out of the version's jsonb blob, so unlike every path that @@ -243,7 +270,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 @@ -276,7 +307,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 new file mode 100644 index 00000000000..8233632f6d7 --- /dev/null +++ b/apps/sim/scripts/dump-change-detection-states.ts @@ -0,0 +1,225 @@ +/** + * 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). 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 --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 + * 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 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' + +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): Record { + const out: Record = {} + + 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) } + : 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 as SubBlockState['value'], + } + } + + 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 + + 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 + ) + + 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',