mirror of
https://github.com/simstudioai/sim.git
synced 2026-08-31 01:11:53 +08:00
fix(workflows): exclude block locked from diff detection (#4631)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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', () => {
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -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> = {}): 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<string, unknown>).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<string, unknown>
|
||||
expect(rest.name).toBe('A')
|
||||
expect(rest.enabled).toBe(false)
|
||||
expect(rest.locked).toBeUndefined()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string, unknown>): Record<s
|
||||
|
||||
/**
|
||||
* Normalizes a workflow state for comparison or hashing.
|
||||
* Excludes non-functional fields (position, layout, height, outputs, diff markers)
|
||||
* Excludes non-functional fields (position, layout, height, outputs, diff markers, locked)
|
||||
* and system/trigger runtime subBlocks.
|
||||
*
|
||||
* @param state - The workflow state to normalize
|
||||
|
||||
@@ -136,7 +136,7 @@ describe('WorkflowDiffEngine', () => {
|
||||
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', () => {
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user