refactor(blocks): give the error-output flag a column instead of two homes

`errorEnabled` had no column, so it persisted inside the block's `data` jsonb
and was mirrored onto the block as a field on load. Every writer had to route
its `data` through `withPersistedErrorEnabled` or silently drop the toggle, the
realtime op `jsonb_set`, and change detection saw the same value twice — which
is what made the deploy badge flip between Live and "Update deployment" after
toggling the port.

Its siblings — `enabled`, `horizontal_handles`, `advanced_mode`, `trigger_mode`,
`locked` — are all boolean columns; `data` is for React Flow and subflow state.
The flag belongs with them, so it now has `error_enabled` and one home. The
shuttle helper, its `BlockData` mirror, the store's fallback read, and the
comparison exclusion the duplication forced are all gone.

Backwards compatibility, since released versions draw the error port with no
toggle in front of it: a block already wired to an error edge HAS the output on,
because there was no other way to draw that edge. That rule is now stated in
three places and none may be narrowed to read the flag alone —

- the migration backfills `error_enabled` from the edges, so live rows are true
  before any new code reads them;
- `materializeDeploymentState` derives it for a version's frozen jsonb, which the
  migration cannot reach — otherwise every workflow deployed before the toggle
  would ask to be redeployed once;
- `workflow-block.tsx` keeps it at render time for states that reach the canvas
  through neither (imports, copilot edits), where unmounting the port would make
  React Flow drop the edge leaving it.

The migration also moves any `data.errorEnabled` a developer created on this
branch onto the column and strips the key; both statements match zero rows in
production, where it never shipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vikhyath Mondreti
2026-08-10 13:10:54 -07:00
co-authored by Claude Opus 5
parent 066e18ac28
commit 7934df7f88
14 changed files with 18914 additions and 91 deletions
+5 -12
View File
@@ -34,7 +34,6 @@ import {
isWorkflowBlockProtected, isWorkflowBlockProtected,
normalizeWorkflowEdgeSourceHandle, normalizeWorkflowEdgeSourceHandle,
normalizeWorkflowEdgeTargetHandle, normalizeWorkflowEdgeTargetHandle,
withPersistedErrorEnabled,
} from '@sim/workflow-types/workflow' } from '@sim/workflow-types/workflow'
import { and, eq, inArray, isNull, or, sql } from 'drizzle-orm' import { and, eq, inArray, isNull, or, sql } from 'drizzle-orm'
import { drizzle } from 'drizzle-orm/postgres-js' import { drizzle } from 'drizzle-orm/postgres-js'
@@ -755,12 +754,7 @@ async function handleBlockOperationTx(
const updateResult = await tx const updateResult = await tx
.update(workflowBlocks) .update(workflowBlocks)
.set({ .set({
data: sql`jsonb_set( errorEnabled: payload.errorEnabled,
coalesce(${workflowBlocks.data}, '{}'::jsonb),
'{errorEnabled}',
${JSON.stringify(payload.errorEnabled)}::jsonb,
true
)`,
updatedAt: new Date(), updatedAt: new Date(),
}) })
.where(and(eq(workflowBlocks.id, payload.id), eq(workflowBlocks.workflowId, workflowId))) .where(and(eq(workflowBlocks.id, payload.id), eq(workflowBlocks.workflowId, workflowId)))
@@ -960,16 +954,14 @@ async function handleBlocksOperationTx(
name: block.name as string, name: block.name as string,
positionX: (block.position as { x: number; y: number }).x, positionX: (block.position as { x: number; y: number }).x,
positionY: (block.position as { x: number; y: number }).y, positionY: (block.position as { x: number; y: number }).y,
data: withPersistedErrorEnabled( data: (block.data as Record<string, unknown> | undefined) || {},
block.data as Record<string, unknown> | undefined,
block.errorEnabled as boolean | undefined
),
subBlocks: mergedSubBlocks, subBlocks: mergedSubBlocks,
outputs: (block.outputs as Record<string, unknown>) || {}, outputs: (block.outputs as Record<string, unknown>) || {},
enabled: (block.enabled as boolean) ?? true, enabled: (block.enabled as boolean) ?? true,
horizontalHandles: (block.horizontalHandles as boolean) ?? true, horizontalHandles: (block.horizontalHandles as boolean) ?? true,
advancedMode: (block.advancedMode as boolean) ?? false, advancedMode: (block.advancedMode as boolean) ?? false,
triggerMode: (block.triggerMode as boolean) ?? false, triggerMode: (block.triggerMode as boolean) ?? false,
errorEnabled: (block.errorEnabled as boolean) ?? false,
height: (block.height as number) || 0, height: (block.height as number) || 0,
locked: (block.locked as boolean) ?? false, locked: (block.locked as boolean) ?? false,
} }
@@ -2179,7 +2171,8 @@ async function handleWorkflowOperationTx(
name: block.name, name: block.name,
positionX: block.position.x, positionX: block.position.x,
positionY: block.position.y, positionY: block.position.y,
data: withPersistedErrorEnabled(block.data, block.errorEnabled), errorEnabled: block.errorEnabled ?? false,
data: block.data || {},
subBlocks: block.subBlocks || {}, subBlocks: block.subBlocks || {},
outputs: block.outputs || {}, outputs: block.outputs || {},
enabled: block.enabled ?? true, enabled: block.enabled ?? true,
@@ -741,6 +741,16 @@ export const WorkflowBlock = memo(function WorkflowBlock({
() => new Set(highlightedHandleKey ? highlightedHandleKey.split('|') : []), () => new Set(highlightedHandleKey ? highlightedHandleKey.split('|') : []),
[highlightedHandleKey] [highlightedHandleKey]
) )
/*
* An existing error edge means the output is on, whatever the flag says.
* Every released version drew the error port with no toggle in front of it, so
* a block already wired that way had no other way to make the connection —
* reading the flag alone would unmount a port a live workflow routes failures
* through, and React Flow drops an edge whose handle is not mounted. The
* migration backfills those rows, but this keeps the rule true for any state
* that reaches the canvas without passing through it (an imported workflow, a
* deployment snapshot, a copilot edit).
*/
const errorOutputEnabled = Boolean(currentBlock?.errorEnabled || hasErrorConnection) const errorOutputEnabled = Boolean(currentBlock?.errorEnabled || hasErrorConnection)
const handleToggleErrorOutput = useCallback( const handleToggleErrorOutput = useCallback(
(next: boolean) => { (next: boolean) => {
@@ -331,43 +331,23 @@ describe('hasWorkflowChanged', () => {
}) })
/** /**
* `errorEnabled` persists inside the block's `data` jsonb and is mirrored onto * The flag drives the error port, and the deploy badge is what tells a user the
* the block as a field on load, so the same flag reaches the comparison twice * port they just switched on is not live yet.
* — and only some paths populate the copy. Counted through `data`, a block
* read one way differed from the identical block read another, which flipped
* the deploy badge between Live and "Update deployment" as each query landed.
* The top-level field is the one the comparison trusts.
*/ */
describe('Error Output Flag', () => { describe('Error Output Flag', () => {
/* Spread on after the factory, which returns a fixed block shape. */ const withErrorFlag = (errorEnabled: boolean) =>
const withErrorFlag = (errorEnabled: boolean, data: Record<string, unknown>) =>
createWorkflowState({ createWorkflowState({
blocks: { block1: { ...createBlock('block1', { data }), errorEnabled } }, blocks: { block1: { ...createBlock('block1'), errorEnabled } },
}) })
it.concurrent('ignores a stale data mirror when the flag itself matches', () => { it.concurrent('detects the flag being turned on', () => {
expect( expect(hasWorkflowChanged(withErrorFlag(true), withErrorFlag(false))).toBe(true)
hasWorkflowChanged(
withErrorFlag(true, { errorEnabled: false }),
withErrorFlag(true, { errorEnabled: true })
)
).toBe(false)
}) })
it.concurrent('ignores a data mirror only one side carries', () => { it.concurrent('treats an unset flag as off', () => {
expect(hasWorkflowChanged(withErrorFlag(false, {}), withErrorFlag(false, {}))).toBe(false) const unset = createWorkflowState({ blocks: { block1: createBlock('block1') } })
expect( expect(hasWorkflowChanged(unset, withErrorFlag(false))).toBe(false)
hasWorkflowChanged(withErrorFlag(false, {}), withErrorFlag(false, { errorEnabled: false })) expect(hasWorkflowChanged(unset, withErrorFlag(true))).toBe(true)
).toBe(false)
})
it.concurrent('still detects the flag being turned on', () => {
expect(
hasWorkflowChanged(
withErrorFlag(true, { errorEnabled: true }),
withErrorFlag(false, { errorEnabled: false })
)
).toBe(true)
}) })
}) })
@@ -47,17 +47,6 @@ export const EXCLUDED_BLOCK_DATA_FIELDS: readonly string[] = [
// Parallel fields - duplicated in parallels state and/or subBlocks // Parallel fields - duplicated in parallels state and/or subBlocks
'parallelType', // Duplicated in parallels state 'parallelType', // Duplicated in parallels state
'distribution', // Parallel distribution (derived during execution) 'distribution', // Parallel distribution (derived during execution)
/*
* Duplicated from the block's own `errorEnabled`. `data` is where the flag
* persists (the realtime server `jsonb_set`s it, and load mirrors it back
* onto the block), so the same value reaches this comparison twice — and only
* some paths populate the copy. Counted here, a block read one way differed
* from the identical block read another and the deploy badge flipped between
* Live and "Update deployment" as each query landed. The block field is
* compared on its own, with `!!`, so absent and `false` agree there.
*/
'errorEnabled',
] as const ] as const
/** /**
@@ -120,6 +120,7 @@ function toDbBlock(block: ReturnType<typeof createBlock>, workflowId: string) {
horizontalHandles: block.horizontalHandles, horizontalHandles: block.horizontalHandles,
advancedMode: block.advancedMode ?? false, advancedMode: block.advancedMode ?? false,
triggerMode: block.triggerMode ?? false, triggerMode: block.triggerMode ?? false,
errorEnabled: block.errorEnabled ?? false,
height: block.height ?? 150, height: block.height ?? 150,
subBlocks: block.subBlocks ?? {}, subBlocks: block.subBlocks ?? {},
outputs: block.outputs ?? {}, outputs: block.outputs ?? {},
@@ -1294,6 +1295,31 @@ describe('Database Helpers', () => {
dbHelpers.invalidateDeployedStateCache() dbHelpers.invalidateDeployedStateCache()
}) })
/**
* Every version before the error toggle drew that port unconditionally, so a
* snapshot carrying an error edge was taken from a block that had the output
* on. The migration backfilling the flag only reaches the live tables, never
* a version's frozen jsonb — so without deriving it here the deployed side
* reads `false` against a live `true`, and every workflow deployed before the
* toggle asks to be redeployed once.
*/
it('reads an error edge in an old snapshot as the error output being on', async () => {
const state = buildDeployedState()
state.edges.push({
id: 'edge-err',
source: 'block-1',
target: 'block-2',
sourceHandle: 'error',
targetHandle: 'input',
})
queueActiveVersion('dv-error-edge', state)
const deployed = await dbHelpers.loadDeployedWorkflowState('wf-error-edge', 'workspace-1')
expect(deployed?.blocks['block-1'].errorEnabled).toBe(true)
expect(deployed?.blocks['block-2'].errorEnabled).toBeUndefined()
})
it('serves a cache HIT, skipping migrations on the second call for the same active version', async () => { it('serves a cache HIT, skipping migrations on the second call for the same active version', async () => {
queueActiveVersion('dv-hit', buildDeployedState()) queueActiveVersion('dv-hit', buildDeployedState())
queueActiveVersion('dv-hit', buildDeployedState()) queueActiveVersion('dv-hit', buildDeployedState())
+31 -9
View File
@@ -172,16 +172,38 @@ async function materializeDeploymentState(
state.blocks || {}, state.blocks || {},
resolvedWorkspaceId resolvedWorkspaceId
) )
/*
* Read straight out of the version's jsonb blob, so unlike every path that
* goes through `loadWorkflowFromNormalizedTables` these handles were never
* canonicalized. Change detection diffs this against a normalized live state,
* so a snapshot holding a side-anchored id would report every edge as
* added-and-removed and pin the workflow to "needs redeploy" forever.
*/
const edges = normalizeWorkflowEdgeHandles(state.edges)
/*
* An error edge means the error output is on. Every version before the toggle
* drew that port unconditionally, so a snapshot with such an edge was taken
* from a block that had the output — and the migration backfilling the flag
* only reaches the live tables, never a version's frozen jsonb. Without this
* the deployed side reads `false` against a live `true` and every workflow
* deployed before the toggle asks to be redeployed once. Same rule as
* `workflow-block.tsx` applies at render time; neither may read the flag alone.
*/
const errorSourceBlockIds = new Set(
edges.filter((edge) => edge.sourceHandle === 'error').map((edge) => edge.source)
)
const blocks: DeployedWorkflowData['blocks'] = {}
for (const [blockId, block] of Object.entries(migratedBlocks)) {
blocks[blockId] =
block.errorEnabled || !errorSourceBlockIds.has(blockId)
? block
: { ...block, errorEnabled: true }
}
const deployedState: DeployedWorkflowData = { const deployedState: DeployedWorkflowData = {
blocks: migratedBlocks, blocks,
/* edges,
* Read straight out of the version's jsonb blob, so unlike every path that
* goes through `loadWorkflowFromNormalizedTables` these handles were never
* canonicalized. Change detection diffs this against a normalized live
* state, so a snapshot holding a side-anchored id would report every edge
* as added-and-removed and pin the workflow to "needs redeploy" forever.
*/
edges: normalizeWorkflowEdgeHandles(state.edges),
loops: state.loops || {}, loops: state.loops || {},
parallels: state.parallels || {}, parallels: state.parallels || {},
variables: state.variables || {}, variables: state.variables || {},
+1 -1
View File
@@ -172,7 +172,7 @@ export const useWorkflowStore = create<WorkflowStore>()(
enabled: block.enabled ?? true, enabled: block.enabled ?? true,
horizontalHandles: block.horizontalHandles ?? true, horizontalHandles: block.horizontalHandles ?? true,
advancedMode: block.advancedMode ?? false, advancedMode: block.advancedMode ?? false,
errorEnabled: block.errorEnabled ?? block.data?.errorEnabled === true, errorEnabled: block.errorEnabled ?? false,
triggerMode: block.triggerMode ?? false, triggerMode: block.triggerMode ?? false,
height: block.height ?? 0, height: block.height ?? 0,
data: block.data, data: block.data,
@@ -0,0 +1,22 @@
-- migration-safe: an additive column with a non-null default, so old and new app versions read and write these rows throughout the deploy.
ALTER TABLE "workflow_blocks" ADD COLUMN "error_enabled" boolean DEFAULT false NOT NULL;--> statement-breakpoint
-- Every released version draws the error port with no toggle in front of it, so a
-- block someone already wired an error edge out of HAS the output on — there was
-- no other way to draw that edge. Defaulting those rows to false would hide a port
-- a live workflow routes failures through, so the edges decide the flag for every
-- workflow that predates the toggle. `workflow-block.tsx` keeps the same rule at
-- render time for states that never pass through here (imports, snapshots, copilot
-- edits); do not narrow either one to read the flag alone.
-- migration-safe: idempotent (sets a constant, on a column no released version reads or writes, so there is nothing to race); touches only the blocks that already have an error edge.
UPDATE "workflow_blocks" AS b
SET "error_enabled" = true
FROM "workflow_edges" AS e
WHERE e."source_block_id" = b."id" AND e."source_handle" = 'error';--> statement-breakpoint
-- The flag also briefly persisted inside `data` on this unmerged branch and never
-- reached production. These move any row a developer created onto the column and
-- leave the value one home, so a block saved before the change stops differing
-- from one saved after it. Both match zero rows on a database that never saw the key.
-- migration-safe: idempotent (re-running sets the same value and removes an already-absent key); the key is written by no released version, so no concurrent writer can reintroduce it.
UPDATE "workflow_blocks" SET "error_enabled" = true WHERE "data" ->> 'errorEnabled' = 'true';--> statement-breakpoint
-- migration-safe: idempotent (re-running removes an already-absent key); the key is written by no released version, so no concurrent writer can reintroduce it.
UPDATE "workflow_blocks" SET "data" = "data" - 'errorEnabled' WHERE "data" ->> 'errorEnabled' IS NOT NULL;
File diff suppressed because it is too large Load Diff
@@ -2003,6 +2003,13 @@
"when": 1786226773546, "when": 1786226773546,
"tag": "0286_tiktok_routing_key", "tag": "0286_tiktok_routing_key",
"breakpoints": true "breakpoints": true
},
{
"idx": 287,
"version": "7",
"when": 1786238404458,
"tag": "0287_workflow_blocks_error_enabled",
"breakpoints": true
} }
] ]
} }
+1
View File
@@ -294,6 +294,7 @@ export const workflowBlocks = pgTable(
isWide: boolean('is_wide').notNull().default(false), isWide: boolean('is_wide').notNull().default(false),
advancedMode: boolean('advanced_mode').notNull().default(false), advancedMode: boolean('advanced_mode').notNull().default(false),
triggerMode: boolean('trigger_mode').notNull().default(false), triggerMode: boolean('trigger_mode').notNull().default(false),
errorEnabled: boolean('error_enabled').notNull().default(false),
locked: boolean('locked').notNull().default(false), locked: boolean('locked').notNull().default(false),
height: decimal('height').notNull().default('0'), height: decimal('height').notNull().default('0'),
+2 -3
View File
@@ -5,7 +5,6 @@ import {
normalizeWorkflowEdgeSourceHandle, normalizeWorkflowEdgeSourceHandle,
normalizeWorkflowEdgeTargetHandle, normalizeWorkflowEdgeTargetHandle,
SUBFLOW_TYPES, SUBFLOW_TYPES,
withPersistedErrorEnabled,
} from '@sim/workflow-types/workflow' } from '@sim/workflow-types/workflow'
import { and, eq, getTableColumns, isNull, sql } from 'drizzle-orm' import { and, eq, getTableColumns, isNull, sql } from 'drizzle-orm'
import type { Edge } from 'reactflow' import type { Edge } from 'reactflow'
@@ -87,7 +86,7 @@ export async function loadWorkflowFromNormalizedTablesRaw(
enabled: block.enabled, enabled: block.enabled,
horizontalHandles: block.horizontalHandles, horizontalHandles: block.horizontalHandles,
advancedMode: block.advancedMode, advancedMode: block.advancedMode,
errorEnabled: blockData?.errorEnabled === true, errorEnabled: block.errorEnabled,
triggerMode: block.triggerMode, triggerMode: block.triggerMode,
height: Number(block.height), height: Number(block.height),
subBlocks: (block.subBlocks as BlockState['subBlocks']) || {}, subBlocks: (block.subBlocks as BlockState['subBlocks']) || {},
@@ -244,7 +243,7 @@ export async function persistMigratedBlocks(
.update(workflowBlocks) .update(workflowBlocks)
.set({ .set({
subBlocks: block.subBlocks, subBlocks: block.subBlocks,
data: withPersistedErrorEnabled(block.data, block.errorEnabled), data: block.data,
updatedAt: new Date(), updatedAt: new Date(),
}) })
.where(whereClause) .where(whereClause)
+2 -2
View File
@@ -6,7 +6,6 @@ import {
normalizeWorkflowEdgeSourceHandle, normalizeWorkflowEdgeSourceHandle,
normalizeWorkflowEdgeTargetHandle, normalizeWorkflowEdgeTargetHandle,
SUBFLOW_TYPES, SUBFLOW_TYPES,
withPersistedErrorEnabled,
} from '@sim/workflow-types/workflow' } from '@sim/workflow-types/workflow'
import type { InferInsertModel } from 'drizzle-orm' import type { InferInsertModel } from 'drizzle-orm'
import { eq } from 'drizzle-orm' import { eq } from 'drizzle-orm'
@@ -48,7 +47,8 @@ export async function saveWorkflowToNormalizedTables(
height: String(block.height || 0), height: String(block.height || 0),
subBlocks: block.subBlocks || {}, subBlocks: block.subBlocks || {},
outputs: block.outputs || {}, outputs: block.outputs || {},
data: withPersistedErrorEnabled(block.data, block.errorEnabled), errorEnabled: block.errorEnabled ?? false,
data: block.data || {},
parentId: block.data?.parentId || null, parentId: block.data?.parentId || null,
extent: block.data?.extent || null, extent: block.data?.extent || null,
locked: block.locked ?? false, locked: block.locked ?? false,
-23
View File
@@ -56,8 +56,6 @@ export interface BlockData {
batchSize?: number batchSize?: number
type?: string type?: string
canonicalModes?: Record<string, 'basic' | 'advanced'> canonicalModes?: Record<string, 'basic' | 'advanced'>
/** Persisted mirror of {@link BlockState.errorEnabled}. */
errorEnabled?: boolean
} }
export interface BlockLayoutState { export interface BlockLayoutState {
@@ -65,27 +63,6 @@ export interface BlockLayoutState {
measuredHeight?: number measuredHeight?: number
} }
/**
* Mirrors {@link BlockState.errorEnabled} into the block's `data` blob.
*
* `workflow_blocks` has no `error_enabled` column, so the jsonb `data` column
* is the flag's only persisted home while the canonical in-memory value lives
* at the top level. Every writer that inserts or replaces a block row must
* route its `data` through this, or the toggle is silently dropped on reload.
*
* The fallback reads the mirror already on the CALLER's `data` — it does not
* consult the stored row — so it only covers a payload that carries `data`
* but no top-level flag. A writer that replaces `data` wholesale still needs
* the caller to supply the flag.
*/
export function withPersistedErrorEnabled<T extends object>(
data: T | null | undefined,
errorEnabled: boolean | undefined
): T & { errorEnabled: boolean } {
const base = (data ?? {}) as T & { errorEnabled?: boolean }
return { ...base, errorEnabled: errorEnabled ?? base.errorEnabled === true }
}
export interface BlockState { export interface BlockState {
id: string id: string
type: string type: string