From 261dda2bc79d019a67863b26926121f65ea3a94e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 15 May 2026 19:12:36 -0700 Subject: [PATCH] fix(workflows): exclude block locked from diff detection Toggling a block's locked state is UI metadata and should not register as a workflow drift/diff. Strip locked from hasBlockChanged, computeFieldDiff, the compare.ts blockFields list, and from extractBlockFieldsForComparison so it's also excluded from the normalized stringify-based block equality check used by drift detection and hashing. Co-Authored-By: Claude Opus 4.7 --- .../lib/workflows/comparison/compare.test.ts | 16 +++- apps/sim/lib/workflows/comparison/compare.ts | 2 +- .../workflows/comparison/normalize.test.ts | 44 ++++++++++- .../sim/lib/workflows/comparison/normalize.ts | 5 +- .../lib/workflows/diff/diff-engine.test.ts | 77 +++++++++++-------- apps/sim/lib/workflows/diff/diff-engine.ts | 16 +--- 6 files changed, 110 insertions(+), 50 deletions(-) diff --git a/apps/sim/lib/workflows/comparison/compare.test.ts b/apps/sim/lib/workflows/comparison/compare.test.ts index f3a139ac915..1921619f795 100644 --- a/apps/sim/lib/workflows/comparison/compare.test.ts +++ b/apps/sim/lib/workflows/comparison/compare.test.ts @@ -297,14 +297,14 @@ describe('hasWorkflowChanged', () => { expect(hasWorkflowChanged(state1, state2)).toBe(true) }) - it.concurrent('should detect locked/unlocked changes', () => { + it.concurrent('should not detect locked/unlocked toggle as a workflow change', () => { const state1 = createWorkflowState({ blocks: { block1: createBlock('block1', { locked: false }) }, }) const state2 = createWorkflowState({ blocks: { block1: createBlock('block1', { locked: true }) }, }) - expect(hasWorkflowChanged(state1, state2)).toBe(true) + expect(hasWorkflowChanged(state1, state2)).toBe(false) }) it.concurrent('should not detect changes when locked state is the same', () => { @@ -316,6 +316,18 @@ describe('hasWorkflowChanged', () => { }) expect(hasWorkflowChanged(state1, state2)).toBe(false) }) + + it.concurrent('should not include locked changes in diff summary', () => { + const state1 = createWorkflowState({ + blocks: { block1: createBlock('block1', { locked: false }) }, + }) + const state2 = createWorkflowState({ + blocks: { block1: createBlock('block1', { locked: true }) }, + }) + const summary = generateWorkflowDiffSummary(state2, state1) + expect(summary.hasChanges).toBe(false) + expect(summary.modifiedBlocks).toEqual([]) + }) }) describe('SubBlock Changes', () => { diff --git a/apps/sim/lib/workflows/comparison/compare.ts b/apps/sim/lib/workflows/comparison/compare.ts index 8f16b5792fd..be5e6ab7820 100644 --- a/apps/sim/lib/workflows/comparison/compare.ts +++ b/apps/sim/lib/workflows/comparison/compare.ts @@ -195,7 +195,7 @@ export function generateWorkflowDiffSummary( newValue: currentBlock.enabled, }) } - const blockFields = ['horizontalHandles', 'advancedMode', 'triggerMode', 'locked'] as const + const blockFields = ['horizontalHandles', 'advancedMode', 'triggerMode'] as const for (const field of blockFields) { if (!!currentBlock[field] !== !!previousBlock[field]) { changes.push({ diff --git a/apps/sim/lib/workflows/comparison/normalize.test.ts b/apps/sim/lib/workflows/comparison/normalize.test.ts index 0cdac720978..f85e0140855 100644 --- a/apps/sim/lib/workflows/comparison/normalize.test.ts +++ b/apps/sim/lib/workflows/comparison/normalize.test.ts @@ -2,8 +2,9 @@ * Tests for workflow normalization utilities */ import { describe, expect, it } from 'vitest' -import type { Loop, Parallel } from '@/stores/workflows/workflow/types' +import type { BlockState, Loop, Parallel } from '@/stores/workflows/workflow/types' import { + extractBlockFieldsForComparison, filterSubBlockIds, normalizedStringify, normalizeEdge, @@ -829,4 +830,45 @@ describe('Workflow Normalization Utilities', () => { expect(normalized.placeholder).toBe('Enter signing secret') }) }) + + describe('extractBlockFieldsForComparison', () => { + function createBlock(overrides: Partial = {}): BlockState { + return { + id: 'block-1', + type: 'agent', + name: 'Test', + enabled: true, + position: { x: 0, y: 0 }, + subBlocks: {}, + outputs: {}, + ...overrides, + } as BlockState + } + + it.concurrent('should strip the locked field from blockRest', () => { + const { blockRest } = extractBlockFieldsForComparison(createBlock({ locked: true })) + expect((blockRest as Record).locked).toBeUndefined() + }) + + it.concurrent( + 'should yield identical blockRest when only locked differs between two blocks', + () => { + const lockedBlock = createBlock({ locked: true }) + const unlockedBlock = createBlock({ locked: false }) + const { blockRest: lockedRest } = extractBlockFieldsForComparison(lockedBlock) + const { blockRest: unlockedRest } = extractBlockFieldsForComparison(unlockedBlock) + expect(normalizedStringify(lockedRest)).toBe(normalizedStringify(unlockedRest)) + } + ) + + it.concurrent('should keep functional fields like name and enabled', () => { + const { blockRest } = extractBlockFieldsForComparison( + createBlock({ name: 'A', enabled: false, locked: true }) + ) + const rest = blockRest as Record + expect(rest.name).toBe('A') + expect(rest.enabled).toBe(false) + expect(rest.locked).toBeUndefined() + }) + }) }) diff --git a/apps/sim/lib/workflows/comparison/normalize.ts b/apps/sim/lib/workflows/comparison/normalize.ts index 556ee4a4fb8..8e4f97af77f 100644 --- a/apps/sim/lib/workflows/comparison/normalize.ts +++ b/apps/sim/lib/workflows/comparison/normalize.ts @@ -386,7 +386,7 @@ export function normalizeBlockData( /** * Extracts block fields for comparison, excluding visual-only and runtime fields. - * Excludes: position, layout, height, outputs, is_diff, field_diffs + * Excludes: position, layout, height, outputs, is_diff, field_diffs, locked * * @param block - The block state * @returns Extracted fields suitable for comparison @@ -401,6 +401,7 @@ export function extractBlockFieldsForComparison(block: BlockState): ExtractedBlo outputs: _outputs, is_diff: _isDiff, field_diffs: _fieldDiffs, + locked: _locked, ...blockRest } = blockWithDiff @@ -503,7 +504,7 @@ export function extractSubBlockRest(subBlock: Record): Record { describe('hasBlockChanged detection', () => { describe('locked state changes', () => { it.concurrent( - 'should detect when block locked state changes from false to true', + 'should NOT detect a diff when only the locked state changes (false -> true)', async () => { const freshEngine = new WorkflowDiffEngine() const baseline = createMockWorkflowState({ @@ -154,7 +154,10 @@ describe('WorkflowDiffEngine', () => { ) expect(result.success).toBe(true) - expect(result.diff?.diffAnalysis?.edited_blocks).toContain('block-1') + expect(result.diff?.diffAnalysis?.edited_blocks ?? []).not.toContain('block-1') + expect( + result.diff?.diffAnalysis?.field_diffs?.['block-1']?.changed_fields ?? [] + ).not.toContain('locked') } ) @@ -171,43 +174,57 @@ describe('WorkflowDiffEngine', () => { const result = await freshEngine.createDiffFromWorkflowState(proposed, undefined, baseline) expect(result.success).toBe(true) - expect(result.diff?.diffAnalysis?.edited_blocks).not.toContain('block-1') + expect(result.diff?.diffAnalysis?.edited_blocks ?? []).not.toContain('block-1') }) - it.concurrent('should detect change when locked goes from undefined to true', async () => { - const freshEngine = new WorkflowDiffEngine() - const baseline = createMockWorkflowState({ - 'block-1': createMockBlock({ id: 'block-1' }), // locked undefined - }) + it.concurrent( + 'should NOT detect a diff when locked goes from undefined to true', + async () => { + const freshEngine = new WorkflowDiffEngine() + const baseline = createMockWorkflowState({ + 'block-1': createMockBlock({ id: 'block-1' }), + }) - const proposed = createMockWorkflowState({ - 'block-1': createMockBlock({ id: 'block-1', locked: true }), - }) + const proposed = createMockWorkflowState({ + 'block-1': createMockBlock({ id: 'block-1', locked: true }), + }) - const result = await freshEngine.createDiffFromWorkflowState(proposed, undefined, baseline) + const result = await freshEngine.createDiffFromWorkflowState( + proposed, + undefined, + baseline + ) - expect(result.success).toBe(true) - // The hasBlockChanged function uses !!locked for comparison - // so undefined -> true should be detected as a change - expect(result.diff?.diffAnalysis?.edited_blocks).toContain('block-1') - }) + expect(result.success).toBe(true) + expect(result.diff?.diffAnalysis?.edited_blocks ?? []).not.toContain('block-1') + } + ) - it.concurrent('should not detect change when both locked states are falsy', async () => { - const freshEngine = new WorkflowDiffEngine() - const baseline = createMockWorkflowState({ - 'block-1': createMockBlock({ id: 'block-1' }), // locked undefined - }) + it.concurrent( + 'should still detect real edits on a block whose locked state also changed', + async () => { + const freshEngine = new WorkflowDiffEngine() + const baseline = createMockWorkflowState({ + 'block-1': createMockBlock({ id: 'block-1', enabled: true, locked: false }), + }) - const proposed = createMockWorkflowState({ - 'block-1': createMockBlock({ id: 'block-1', locked: false }), // locked false - }) + const proposed = createMockWorkflowState({ + 'block-1': createMockBlock({ id: 'block-1', enabled: false, locked: true }), + }) - const result = await freshEngine.createDiffFromWorkflowState(proposed, undefined, baseline) + const result = await freshEngine.createDiffFromWorkflowState( + proposed, + undefined, + baseline + ) - expect(result.success).toBe(true) - // undefined and false should both be falsy, so !! comparison makes them equal - expect(result.diff?.diffAnalysis?.edited_blocks).not.toContain('block-1') - }) + expect(result.success).toBe(true) + expect(result.diff?.diffAnalysis?.edited_blocks).toContain('block-1') + const changed = result.diff?.diffAnalysis?.field_diffs?.['block-1']?.changed_fields ?? [] + expect(changed).toContain('enabled') + expect(changed).not.toContain('locked') + } + ) }) describe('parent scope changes', () => { diff --git a/apps/sim/lib/workflows/diff/diff-engine.ts b/apps/sim/lib/workflows/diff/diff-engine.ts index 670ac63ca78..fa6c0c44b77 100644 --- a/apps/sim/lib/workflows/diff/diff-engine.ts +++ b/apps/sim/lib/workflows/diff/diff-engine.ts @@ -42,7 +42,6 @@ function hasBlockChanged(currentBlock: BlockState, proposedBlock: BlockState): b if (currentBlock.name !== proposedBlock.name) return true if (currentBlock.enabled !== proposedBlock.enabled) return true if (currentBlock.triggerMode !== proposedBlock.triggerMode) return true - if (!!currentBlock.locked !== !!proposedBlock.locked) return true if ((currentBlock.data?.parentId ?? null) !== (proposedBlock.data?.parentId ?? null)) return true // Compare subBlocks @@ -74,22 +73,11 @@ function computeFieldDiff( const unchangedFields: string[] = [] // Check basic fields - const fieldsToCheck = [ - 'type', - 'name', - 'enabled', - 'triggerMode', - 'horizontalHandles', - 'locked', - ] as const + const fieldsToCheck = ['type', 'name', 'enabled', 'triggerMode', 'horizontalHandles'] as const for (const field of fieldsToCheck) { const currentValue = currentBlock[field] const proposedValue = proposedBlock[field] - if ( - field === 'locked' - ? !!currentValue !== !!proposedValue - : JSON.stringify(currentValue) !== JSON.stringify(proposedValue) - ) { + if (JSON.stringify(currentValue) !== JSON.stringify(proposedValue)) { changedFields.push(field) } else if (currentValue !== undefined) { unchangedFields.push(field)