fix(change-detection): resolve false positive trigger block change detection (#3204)

2026-02-12 07:24:55 -05:00 · 2026-02-11 17:24:17 -08:00
parent bef43f3e84
commit d068ed6d65
6 changed files with 535 additions and 5 deletions
--- 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
@@ -57,6 +57,21 @@ export function useChangeDetection({
        }
      }

+      if (block.triggerMode) {
+        const triggerConfigValue = blockSubValues?.triggerConfig
+        if (
+          triggerConfigValue &&
+          typeof triggerConfigValue === 'object' &&
+          !subBlocks.triggerConfig
+        ) {
+          subBlocks.triggerConfig = {
+            id: 'triggerConfig',
+            type: 'short-input',
+            value: triggerConfigValue,
+          }
+        }
+      }
+
      blocksWithSubBlocks[blockId] = {
        ...block,
        subBlocks,
--- a/apps/sim/lib/workflows/comparison/compare.test.ts
+++ b/apps/sim/lib/workflows/comparison/compare.test.ts
@@ -2364,6 +2364,261 @@ describe('hasWorkflowChanged', () => {
    })
  })

+  describe('Trigger Config Normalization (False Positive Prevention)', () => {
+    it.concurrent(
+      'should not detect change when deployed has null fields but current has values from triggerConfig',
+      () => {
+        // Core scenario: deployed state has null individual fields, current state has
+        // values populated from triggerConfig at runtime by populateTriggerFieldsFromConfig
+        const deployedState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+                botToken: { id: 'botToken', type: 'short-input', value: null },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'secret123', botToken: 'token456' },
+                },
+              },
+            }),
+          },
+        })
+
+        const currentState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: 'secret123' },
+                botToken: { id: 'botToken', type: 'short-input', value: 'token456' },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'secret123', botToken: 'token456' },
+                },
+              },
+            }),
+          },
+        })
+
+        expect(hasWorkflowChanged(currentState, deployedState)).toBe(false)
+      }
+    )
+
+    it.concurrent(
+      'should detect change when user edits a trigger field to a different value',
+      () => {
+        const deployedState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'old-secret' },
+                },
+              },
+            }),
+          },
+        })
+
+        const currentState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: 'new-secret' },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'old-secret' },
+                },
+              },
+            }),
+          },
+        })
+
+        expect(hasWorkflowChanged(currentState, deployedState)).toBe(true)
+      }
+    )
+
+    it.concurrent('should not detect change when both sides have no triggerConfig', () => {
+      const deployedState = createWorkflowState({
+        blocks: {
+          block1: createBlock('block1', {
+            type: 'starter',
+            subBlocks: {
+              signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+            },
+          }),
+        },
+      })
+
+      const currentState = createWorkflowState({
+        blocks: {
+          block1: createBlock('block1', {
+            type: 'starter',
+            subBlocks: {
+              signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+            },
+          }),
+        },
+      })
+
+      expect(hasWorkflowChanged(currentState, deployedState)).toBe(false)
+    })
+
+    it.concurrent(
+      'should not detect change when deployed has empty fields and triggerConfig populates them',
+      () => {
+        // Empty string is also treated as "empty" by normalizeTriggerConfigValues
+        const deployedState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: '' },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'secret123' },
+                },
+              },
+            }),
+          },
+        })
+
+        const currentState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: 'secret123' },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'secret123' },
+                },
+              },
+            }),
+          },
+        })
+
+        expect(hasWorkflowChanged(currentState, deployedState)).toBe(false)
+      }
+    )
+
+    it.concurrent('should not detect change when triggerId differs', () => {
+      const deployedState = createWorkflowState({
+        blocks: {
+          block1: createBlock('block1', {
+            type: 'starter',
+            subBlocks: {
+              model: { value: 'gpt-4' },
+              triggerId: { value: null },
+            },
+          }),
+        },
+      })
+
+      const currentState = createWorkflowState({
+        blocks: {
+          block1: createBlock('block1', {
+            type: 'starter',
+            subBlocks: {
+              model: { value: 'gpt-4' },
+              triggerId: { value: 'slack_webhook' },
+            },
+          }),
+        },
+      })
+
+      expect(hasWorkflowChanged(currentState, deployedState)).toBe(false)
+    })
+
+    it.concurrent(
+      'should not detect change for namespaced system subBlock IDs like samplePayload_slack_webhook',
+      () => {
+        const deployedState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                model: { value: 'gpt-4' },
+                samplePayload_slack_webhook: { value: 'old payload' },
+                triggerInstructions_slack_webhook: { value: 'old instructions' },
+              },
+            }),
+          },
+        })
+
+        const currentState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                model: { value: 'gpt-4' },
+                samplePayload_slack_webhook: { value: 'new payload' },
+                triggerInstructions_slack_webhook: { value: 'new instructions' },
+              },
+            }),
+          },
+        })
+
+        expect(hasWorkflowChanged(currentState, deployedState)).toBe(false)
+      }
+    )
+
+    it.concurrent(
+      'should handle mixed scenario: some fields from triggerConfig, some user-edited',
+      () => {
+        const deployedState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+                botToken: { id: 'botToken', type: 'short-input', value: null },
+                includeFiles: { id: 'includeFiles', type: 'switch', value: false },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'secret123', botToken: 'token456' },
+                },
+              },
+            }),
+          },
+        })
+
+        const currentState = createWorkflowState({
+          blocks: {
+            block1: createBlock('block1', {
+              type: 'starter',
+              subBlocks: {
+                signingSecret: { id: 'signingSecret', type: 'short-input', value: 'secret123' },
+                botToken: { id: 'botToken', type: 'short-input', value: 'token456' },
+                includeFiles: { id: 'includeFiles', type: 'switch', value: true },
+                triggerConfig: {
+                  id: 'triggerConfig',
+                  type: 'short-input',
+                  value: { signingSecret: 'secret123', botToken: 'token456' },
+                },
+              },
+            }),
+          },
+        })
+
+        // includeFiles changed from false to true — this IS a real change
+        expect(hasWorkflowChanged(currentState, deployedState)).toBe(true)
+      }
+    )
+  })
+
  describe('Trigger Runtime Metadata (Should Not Trigger Change)', () => {
    it.concurrent('should not detect change when webhookId differs', () => {
      const deployedState = createWorkflowState({
--- a/apps/sim/lib/workflows/comparison/compare.ts
+++ b/apps/sim/lib/workflows/comparison/compare.ts
@@ -9,6 +9,7 @@ import {
  normalizeLoop,
  normalizeParallel,
  normalizeSubBlockValue,
+  normalizeTriggerConfigValues,
  normalizeValue,
  normalizeVariables,
  sanitizeVariable,
@@ -172,14 +173,18 @@ export function generateWorkflowDiffSummary(
      }
    }

+    // Normalize trigger config values for both states before comparison
+    const normalizedCurrentSubs = normalizeTriggerConfigValues(currentSubBlocks)
+    const normalizedPreviousSubs = normalizeTriggerConfigValues(previousSubBlocks)
+
    // Compare subBlocks using shared helper for filtering (single source of truth)
    const allSubBlockIds = filterSubBlockIds([
-      ...new Set([...Object.keys(currentSubBlocks), ...Object.keys(previousSubBlocks)]),
+      ...new Set([...Object.keys(normalizedCurrentSubs), ...Object.keys(normalizedPreviousSubs)]),
    ])

    for (const subId of allSubBlockIds) {
-      const currentSub = currentSubBlocks[subId] as Record<string, unknown> | undefined
-      const previousSub = previousSubBlocks[subId] as Record<string, unknown> | undefined
+      const currentSub = normalizedCurrentSubs[subId] as Record<string, unknown> | undefined
+      const previousSub = normalizedPreviousSubs[subId] as Record<string, unknown> | undefined

      if (!currentSub || !previousSub) {
        changes.push({
--- a/apps/sim/lib/workflows/comparison/normalize.test.ts
+++ b/apps/sim/lib/workflows/comparison/normalize.test.ts
@@ -4,10 +4,12 @@
 import { describe, expect, it } from 'vitest'
 import type { Loop, Parallel } from '@/stores/workflows/workflow/types'
 import {
+  filterSubBlockIds,
  normalizedStringify,
  normalizeEdge,
  normalizeLoop,
  normalizeParallel,
+  normalizeTriggerConfigValues,
  normalizeValue,
  sanitizeInputFormat,
  sanitizeTools,
@@ -584,4 +586,214 @@ describe('Workflow Normalization Utilities', () => {
      expect(result2).toBe(result3)
    })
  })
+
+  describe('filterSubBlockIds', () => {
+    it.concurrent('should exclude exact SYSTEM_SUBBLOCK_IDS', () => {
+      const ids = ['signingSecret', 'samplePayload', 'triggerInstructions', 'botToken']
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual(['botToken', 'signingSecret'])
+    })
+
+    it.concurrent('should exclude namespaced SYSTEM_SUBBLOCK_IDS (prefix matching)', () => {
+      const ids = [
+        'signingSecret',
+        'samplePayload_slack_webhook',
+        'triggerInstructions_slack_webhook',
+        'webhookUrlDisplay_slack_webhook',
+        'botToken',
+      ]
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual(['botToken', 'signingSecret'])
+    })
+
+    it.concurrent('should exclude exact TRIGGER_RUNTIME_SUBBLOCK_IDS', () => {
+      const ids = ['webhookId', 'triggerPath', 'triggerConfig', 'triggerId', 'signingSecret']
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual(['signingSecret'])
+    })
+
+    it.concurrent('should not exclude IDs that merely contain a system ID substring', () => {
+      const ids = ['mySamplePayload', 'notSamplePayload']
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual(['mySamplePayload', 'notSamplePayload'])
+    })
+
+    it.concurrent('should return sorted results', () => {
+      const ids = ['zebra', 'alpha', 'middle']
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual(['alpha', 'middle', 'zebra'])
+    })
+
+    it.concurrent('should handle empty array', () => {
+      expect(filterSubBlockIds([])).toEqual([])
+    })
+
+    it.concurrent('should handle all IDs being excluded', () => {
+      const ids = ['webhookId', 'triggerPath', 'samplePayload', 'triggerConfig']
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual([])
+    })
+
+    it.concurrent('should exclude setupScript and scheduleInfo namespaced variants', () => {
+      const ids = ['setupScript_google_sheets_row', 'scheduleInfo_cron_trigger', 'realField']
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual(['realField'])
+    })
+
+    it.concurrent('should exclude triggerCredentials namespaced variants', () => {
+      const ids = ['triggerCredentials_slack_webhook', 'signingSecret']
+      const result = filterSubBlockIds(ids)
+      expect(result).toEqual(['signingSecret'])
+    })
+  })
+
+  describe('normalizeTriggerConfigValues', () => {
+    it.concurrent('should return subBlocks unchanged when no triggerConfig exists', () => {
+      const subBlocks = {
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: 'secret123' },
+        botToken: { id: 'botToken', type: 'short-input', value: 'token456' },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect(result).toEqual(subBlocks)
+    })
+
+    it.concurrent('should return subBlocks unchanged when triggerConfig value is null', () => {
+      const subBlocks = {
+        triggerConfig: { id: 'triggerConfig', type: 'short-input', value: null },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect(result).toEqual(subBlocks)
+    })
+
+    it.concurrent(
+      'should return subBlocks unchanged when triggerConfig value is not an object',
+      () => {
+        const subBlocks = {
+          triggerConfig: { id: 'triggerConfig', type: 'short-input', value: 'string-value' },
+          signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+        }
+        const result = normalizeTriggerConfigValues(subBlocks)
+        expect(result).toEqual(subBlocks)
+      }
+    )
+
+    it.concurrent('should populate null individual fields from triggerConfig', () => {
+      const subBlocks = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { signingSecret: 'secret123', botToken: 'token456' },
+        },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+        botToken: { id: 'botToken', type: 'short-input', value: null },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect((result.signingSecret as Record<string, unknown>).value).toBe('secret123')
+      expect((result.botToken as Record<string, unknown>).value).toBe('token456')
+    })
+
+    it.concurrent('should populate undefined individual fields from triggerConfig', () => {
+      const subBlocks = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { signingSecret: 'secret123' },
+        },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: undefined },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect((result.signingSecret as Record<string, unknown>).value).toBe('secret123')
+    })
+
+    it.concurrent('should populate empty string individual fields from triggerConfig', () => {
+      const subBlocks = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { signingSecret: 'secret123' },
+        },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: '' },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect((result.signingSecret as Record<string, unknown>).value).toBe('secret123')
+    })
+
+    it.concurrent('should NOT overwrite existing non-empty individual field values', () => {
+      const subBlocks = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { signingSecret: 'old-secret' },
+        },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: 'user-edited-secret' },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect((result.signingSecret as Record<string, unknown>).value).toBe('user-edited-secret')
+    })
+
+    it.concurrent('should skip triggerConfig fields that are null/undefined', () => {
+      const subBlocks = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { signingSecret: null, botToken: undefined },
+        },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+        botToken: { id: 'botToken', type: 'short-input', value: null },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect((result.signingSecret as Record<string, unknown>).value).toBe(null)
+      expect((result.botToken as Record<string, unknown>).value).toBe(null)
+    })
+
+    it.concurrent('should skip fields from triggerConfig that have no matching subBlock', () => {
+      const subBlocks = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { nonExistentField: 'value123' },
+        },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      expect(result.nonExistentField).toBeUndefined()
+      expect((result.signingSecret as Record<string, unknown>).value).toBe(null)
+    })
+
+    it.concurrent('should not mutate the original subBlocks object', () => {
+      const original = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { signingSecret: 'secret123' },
+        },
+        signingSecret: { id: 'signingSecret', type: 'short-input', value: null },
+      }
+      normalizeTriggerConfigValues(original)
+      expect((original.signingSecret as Record<string, unknown>).value).toBe(null)
+    })
+
+    it.concurrent('should preserve other subBlock properties when populating value', () => {
+      const subBlocks = {
+        triggerConfig: {
+          id: 'triggerConfig',
+          type: 'short-input',
+          value: { signingSecret: 'secret123' },
+        },
+        signingSecret: {
+          id: 'signingSecret',
+          type: 'short-input',
+          value: null,
+          placeholder: 'Enter signing secret',
+        },
+      }
+      const result = normalizeTriggerConfigValues(subBlocks)
+      const normalized = result.signingSecret as Record<string, unknown>
+      expect(normalized.value).toBe('secret123')
+      expect(normalized.id).toBe('signingSecret')
+      expect(normalized.type).toBe('short-input')
+      expect(normalized.placeholder).toBe('Enter signing secret')
+    })
+  })
 })
--- a/apps/sim/lib/workflows/comparison/normalize.ts
+++ b/apps/sim/lib/workflows/comparison/normalize.ts
@@ -418,10 +418,48 @@ export function extractBlockFieldsForComparison(block: BlockState): ExtractedBlo
 */
 export function filterSubBlockIds(subBlockIds: string[]): string[] {
  return subBlockIds
-    .filter((id) => !SYSTEM_SUBBLOCK_IDS.includes(id) && !TRIGGER_RUNTIME_SUBBLOCK_IDS.includes(id))
+    .filter((id) => {
+      if (TRIGGER_RUNTIME_SUBBLOCK_IDS.includes(id)) return false
+      if (SYSTEM_SUBBLOCK_IDS.some((sysId) => id === sysId || id.startsWith(`${sysId}_`)))
+        return false
+      return true
+    })
    .sort()
 }

+/**
+ * Normalizes trigger block subBlocks by populating null/empty individual fields
+ * from the triggerConfig aggregate subBlock. This compensates for the runtime
+ * population done by populateTriggerFieldsFromConfig, ensuring consistent
+ * comparison between client state (with populated values) and deployed state
+ * (with null values from DB).
+ */
+export function normalizeTriggerConfigValues(
+  subBlocks: Record<string, unknown>
+): Record<string, unknown> {
+  const triggerConfigSub = subBlocks.triggerConfig as Record<string, unknown> | undefined
+  const triggerConfigValue = triggerConfigSub?.value
+  if (!triggerConfigValue || typeof triggerConfigValue !== 'object') {
+    return subBlocks
+  }
+
+  const result = { ...subBlocks }
+  for (const [fieldId, configValue] of Object.entries(
+    triggerConfigValue as Record<string, unknown>
+  )) {
+    if (configValue === null || configValue === undefined) continue
+    const existingSub = result[fieldId] as Record<string, unknown> | undefined
+    if (
+      existingSub &&
+      (existingSub.value === null || existingSub.value === undefined || existingSub.value === '')
+    ) {
+      result[fieldId] = { ...existingSub, value: configValue }
+    }
+  }
+
+  return result
+}
+
 /**
 * Normalizes a subBlock value with sanitization for specific subBlock types.
 * Sanitizes: tools (removes isExpanded), inputFormat (removes collapsed)
--- a/apps/sim/triggers/constants.ts
+++ b/apps/sim/triggers/constants.ts
@@ -23,7 +23,12 @@ export const SYSTEM_SUBBLOCK_IDS: string[] = [
 * with default values from the trigger definition on load, which aren't present in
 * the deployed state, causing false positive change detection.
 */
-export const TRIGGER_RUNTIME_SUBBLOCK_IDS: string[] = ['webhookId', 'triggerPath', 'triggerConfig']
+export const TRIGGER_RUNTIME_SUBBLOCK_IDS: string[] = [
+  'webhookId',
+  'triggerPath',
+  'triggerConfig',
+  'triggerId',
+]

 /**
 * Maximum number of consecutive failures before a trigger (schedule/webhook) is auto-disabled.