Skip to content

Commit d6a95cf

Browse files
committed
fix(vars): add socket persistence when variable names are changed, update variable name normalization to match block name normalization, added space constraint on envvar names
1 parent 942da88 commit d6a95cf

File tree

7 files changed

+456
-46
lines changed

7 files changed

+456
-46
lines changed

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tag-dropdown/tag-dropdown.tsx

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import { useVariablesStore } from '@/stores/panel/variables/store'
3434
import type { Variable } from '@/stores/panel/variables/types'
3535
import { useWorkflowRegistry } from '@/stores/workflows/registry/store'
3636
import { useSubBlockStore } from '@/stores/workflows/subblock/store'
37+
import { normalizeBlockName, normalizeVariableName } from '@/stores/workflows/utils'
3738
import { useWorkflowStore } from '@/stores/workflows/workflow/store'
3839
import type { BlockState } from '@/stores/workflows/workflow/types'
3940
import { getTool } from '@/tools/utils'
@@ -117,20 +118,6 @@ const TAG_PREFIXES = {
117118
VARIABLE: 'variable.',
118119
} as const
119120

120-
/**
121-
* Normalizes a block name by removing spaces and converting to lowercase
122-
*/
123-
const normalizeBlockName = (blockName: string): string => {
124-
return blockName.replace(/\s+/g, '').toLowerCase()
125-
}
126-
127-
/**
128-
* Normalizes a variable name by removing spaces
129-
*/
130-
const normalizeVariableName = (variableName: string): string => {
131-
return variableName.replace(/\s+/g, '')
132-
}
133-
134121
/**
135122
* Ensures the root tag is present in the tags array
136123
*/

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/environment/environment.tsx

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,17 @@ interface UIEnvironmentVariable {
5252
id?: number
5353
}
5454

55+
/**
56+
* Validates an environment variable key.
57+
* Returns an error message if invalid, undefined if valid.
58+
*/
59+
function validateEnvVarKey(key: string): string | undefined {
60+
if (!key) return undefined
61+
if (key.includes(' ')) return 'Spaces are not allowed'
62+
if (!ENV_VAR_PATTERN.test(key)) return 'Only letters, numbers, and underscores allowed'
63+
return undefined
64+
}
65+
5566
interface EnvironmentVariablesProps {
5667
registerBeforeLeaveHandler?: (handler: (onProceed: () => void) => void) => void
5768
}
@@ -222,6 +233,10 @@ export function EnvironmentVariables({ registerBeforeLeaveHandler }: Environment
222233
return envVars.some((envVar) => !!envVar.key && Object.hasOwn(workspaceVars, envVar.key))
223234
}, [envVars, workspaceVars])
224235

236+
const hasInvalidKeys = useMemo(() => {
237+
return envVars.some((envVar) => !!envVar.key && validateEnvVarKey(envVar.key))
238+
}, [envVars])
239+
225240
useEffect(() => {
226241
hasChangesRef.current = hasChanges
227242
}, [hasChanges])
@@ -551,6 +566,7 @@ export function EnvironmentVariables({ registerBeforeLeaveHandler }: Environment
551566
const renderEnvVarRow = useCallback(
552567
(envVar: UIEnvironmentVariable, originalIndex: number) => {
553568
const isConflict = !!envVar.key && Object.hasOwn(workspaceVars, envVar.key)
569+
const keyError = validateEnvVarKey(envVar.key)
554570
const maskedValueStyle =
555571
focusedValueIndex !== originalIndex && !isConflict
556572
? ({ WebkitTextSecurity: 'disc' } as React.CSSProperties)
@@ -571,7 +587,7 @@ export function EnvironmentVariables({ registerBeforeLeaveHandler }: Environment
571587
spellCheck='false'
572588
readOnly
573589
onFocus={(e) => e.target.removeAttribute('readOnly')}
574-
className={`h-9 ${isConflict ? conflictClassName : ''}`}
590+
className={`h-9 ${isConflict ? conflictClassName : ''} ${keyError ? 'border-[var(--text-error)]' : ''}`}
575591
/>
576592
<div />
577593
<EmcnInput
@@ -627,7 +643,12 @@ export function EnvironmentVariables({ registerBeforeLeaveHandler }: Environment
627643
</Tooltip.Root>
628644
</div>
629645
</div>
630-
{isConflict && (
646+
{keyError && (
647+
<div className='col-span-3 mt-[4px] text-[12px] text-[var(--text-error)] leading-tight'>
648+
{keyError}
649+
</div>
650+
)}
651+
{isConflict && !keyError && (
631652
<div className='col-span-3 mt-[4px] text-[12px] text-[var(--text-error)] leading-tight'>
632653
Workspace variable with the same name overrides this. Rename your personal key to use
633654
it.
@@ -707,14 +728,17 @@ export function EnvironmentVariables({ registerBeforeLeaveHandler }: Environment
707728
<Tooltip.Trigger asChild>
708729
<Button
709730
onClick={handleSave}
710-
disabled={isLoading || !hasChanges || hasConflicts}
731+
disabled={isLoading || !hasChanges || hasConflicts || hasInvalidKeys}
711732
variant='primary'
712-
className={`${PRIMARY_BUTTON_STYLES} ${hasConflicts ? 'cursor-not-allowed opacity-50' : ''}`}
733+
className={`${PRIMARY_BUTTON_STYLES} ${hasConflicts || hasInvalidKeys ? 'cursor-not-allowed opacity-50' : ''}`}
713734
>
714735
Save
715736
</Button>
716737
</Tooltip.Trigger>
717738
{hasConflicts && <Tooltip.Content>Resolve all conflicts before saving</Tooltip.Content>}
739+
{hasInvalidKeys && !hasConflicts && (
740+
<Tooltip.Content>Fix invalid variable names before saving</Tooltip.Content>
741+
)}
718742
</Tooltip.Root>
719743
</div>
720744

@@ -808,27 +832,31 @@ export function EnvironmentVariables({ registerBeforeLeaveHandler }: Environment
808832
<ModalHeader>Unsaved Changes</ModalHeader>
809833
<ModalBody>
810834
<p className='text-[12px] text-[var(--text-tertiary)]'>
811-
{hasConflicts
812-
? 'You have unsaved changes, but conflicts must be resolved before saving. You can discard your changes to close the modal.'
835+
{hasConflicts || hasInvalidKeys
836+
? `You have unsaved changes, but ${hasConflicts ? 'conflicts must be resolved' : 'invalid variable names must be fixed'} before saving. You can discard your changes to close the modal.`
813837
: 'You have unsaved changes. Do you want to save them before closing?'}
814838
</p>
815839
</ModalBody>
816840
<ModalFooter>
817841
<Button variant='default' onClick={handleCancel}>
818842
Discard Changes
819843
</Button>
820-
{hasConflicts ? (
844+
{hasConflicts || hasInvalidKeys ? (
821845
<Tooltip.Root>
822846
<Tooltip.Trigger asChild>
823847
<Button
824848
disabled={true}
825849
variant='primary'
826-
className='cursor-not-allowed opacity-50'
850+
className={`${PRIMARY_BUTTON_STYLES} cursor-not-allowed opacity-50`}
827851
>
828852
Save Changes
829853
</Button>
830854
</Tooltip.Trigger>
831-
<Tooltip.Content>Resolve all conflicts before saving</Tooltip.Content>
855+
<Tooltip.Content>
856+
{hasConflicts
857+
? 'Resolve all conflicts before saving'
858+
: 'Fix invalid variable names before saving'}
859+
</Tooltip.Content>
832860
</Tooltip.Root>
833861
) : (
834862
<Button onClick={handleSave} variant='primary' className={PRIMARY_BUTTON_STYLES}>
Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,219 @@
1+
import { describe, expect, it, vi } from 'vitest'
2+
import type { ResolutionContext } from './reference'
3+
import { WorkflowResolver } from './workflow'
4+
5+
vi.mock('@/lib/logs/console/logger', () => ({
6+
createLogger: vi.fn().mockReturnValue({
7+
debug: vi.fn(),
8+
info: vi.fn(),
9+
warn: vi.fn(),
10+
error: vi.fn(),
11+
}),
12+
}))
13+
14+
vi.mock('@/lib/workflows/variables/variable-manager', () => ({
15+
VariableManager: {
16+
resolveForExecution: vi.fn((value) => value),
17+
},
18+
}))
19+
20+
/**
21+
* Creates a minimal ResolutionContext for testing.
22+
* The WorkflowResolver only uses context.executionContext.workflowVariables,
23+
* so we only need to provide that field.
24+
*/
25+
function createTestContext(workflowVariables: Record<string, any>): ResolutionContext {
26+
return {
27+
executionContext: { workflowVariables },
28+
executionState: {},
29+
currentNodeId: 'test-node',
30+
} as ResolutionContext
31+
}
32+
33+
describe('WorkflowResolver', () => {
34+
describe('canResolve', () => {
35+
it.concurrent('should return true for variable references', () => {
36+
const resolver = new WorkflowResolver({})
37+
expect(resolver.canResolve('<variable.myvar>')).toBe(true)
38+
expect(resolver.canResolve('<variable.test>')).toBe(true)
39+
})
40+
41+
it.concurrent('should return false for non-variable references', () => {
42+
const resolver = new WorkflowResolver({})
43+
expect(resolver.canResolve('<block.output>')).toBe(false)
44+
expect(resolver.canResolve('<loop.index>')).toBe(false)
45+
expect(resolver.canResolve('plain text')).toBe(false)
46+
})
47+
})
48+
49+
describe('resolve with normalized matching', () => {
50+
it.concurrent('should resolve variable with exact name match', () => {
51+
const variables = {
52+
'var-1': { id: 'var-1', name: 'myvar', type: 'plain', value: 'test-value' },
53+
}
54+
const resolver = new WorkflowResolver(variables)
55+
56+
const result = resolver.resolve('<variable.myvar>', createTestContext(variables))
57+
expect(result).toBe('test-value')
58+
})
59+
60+
it.concurrent('should resolve variable with normalized name (lowercase)', () => {
61+
const variables = {
62+
'var-1': { id: 'var-1', name: 'MyVar', type: 'plain', value: 'test-value' },
63+
}
64+
const resolver = new WorkflowResolver(variables)
65+
66+
const result = resolver.resolve('<variable.myvar>', createTestContext(variables))
67+
expect(result).toBe('test-value')
68+
})
69+
70+
it.concurrent('should resolve variable with normalized name (spaces removed)', () => {
71+
const variables = {
72+
'var-1': { id: 'var-1', name: 'My Variable', type: 'plain', value: 'test-value' },
73+
}
74+
const resolver = new WorkflowResolver(variables)
75+
76+
const result = resolver.resolve('<variable.myvariable>', createTestContext(variables))
77+
expect(result).toBe('test-value')
78+
})
79+
80+
it.concurrent(
81+
'should resolve variable with fully normalized name (JIRA TEAM UUID case)',
82+
() => {
83+
const variables = {
84+
'var-1': { id: 'var-1', name: 'JIRA TEAM UUID', type: 'plain', value: 'uuid-123' },
85+
}
86+
const resolver = new WorkflowResolver(variables)
87+
88+
const result = resolver.resolve('<variable.jirateamuuid>', createTestContext(variables))
89+
expect(result).toBe('uuid-123')
90+
}
91+
)
92+
93+
it.concurrent('should resolve variable regardless of reference case', () => {
94+
const variables = {
95+
'var-1': { id: 'var-1', name: 'jirateamuuid', type: 'plain', value: 'uuid-123' },
96+
}
97+
const resolver = new WorkflowResolver(variables)
98+
99+
const result = resolver.resolve('<variable.JIRATEAMUUID>', createTestContext(variables))
100+
expect(result).toBe('uuid-123')
101+
})
102+
103+
it.concurrent('should resolve by variable ID (exact match)', () => {
104+
const variables = {
105+
'my-uuid-id': { id: 'my-uuid-id', name: 'Some Name', type: 'plain', value: 'id-value' },
106+
}
107+
const resolver = new WorkflowResolver(variables)
108+
109+
const result = resolver.resolve('<variable.my-uuid-id>', createTestContext(variables))
110+
expect(result).toBe('id-value')
111+
})
112+
113+
it.concurrent('should return undefined for non-existent variable', () => {
114+
const variables = {
115+
'var-1': { id: 'var-1', name: 'existing', type: 'plain', value: 'test' },
116+
}
117+
const resolver = new WorkflowResolver(variables)
118+
119+
const result = resolver.resolve('<variable.nonexistent>', createTestContext(variables))
120+
expect(result).toBeUndefined()
121+
})
122+
123+
it.concurrent('should handle nested path access', () => {
124+
const variables = {
125+
'var-1': {
126+
id: 'var-1',
127+
name: 'config',
128+
type: 'object',
129+
value: { nested: { value: 'deep' } },
130+
},
131+
}
132+
const resolver = new WorkflowResolver(variables)
133+
134+
const result = resolver.resolve(
135+
'<variable.config.nested.value>',
136+
createTestContext(variables)
137+
)
138+
expect(result).toBe('deep')
139+
})
140+
141+
it.concurrent('should resolve with mixed case and spaces in reference', () => {
142+
const variables = {
143+
'var-1': { id: 'var-1', name: 'api key', type: 'plain', value: 'secret-key' },
144+
}
145+
const resolver = new WorkflowResolver(variables)
146+
147+
const result = resolver.resolve('<variable.APIKEY>', createTestContext(variables))
148+
expect(result).toBe('secret-key')
149+
})
150+
151+
it.concurrent('should handle real-world variable naming patterns', () => {
152+
const testCases = [
153+
{ varName: 'User ID', refName: 'userid', value: 'user-123' },
154+
{ varName: 'API Key', refName: 'apikey', value: 'key-456' },
155+
{ varName: 'STRIPE SECRET KEY', refName: 'stripesecretkey', value: 'sk_test' },
156+
{ varName: 'Database URL', refName: 'databaseurl', value: 'postgres://...' },
157+
]
158+
159+
for (const { varName, refName, value } of testCases) {
160+
const variables = {
161+
'var-1': { id: 'var-1', name: varName, type: 'plain', value },
162+
}
163+
const resolver = new WorkflowResolver(variables)
164+
165+
const result = resolver.resolve(`<variable.${refName}>`, createTestContext(variables))
166+
expect(result).toBe(value)
167+
}
168+
})
169+
})
170+
171+
describe('edge cases', () => {
172+
it.concurrent('should handle empty workflow variables', () => {
173+
const resolver = new WorkflowResolver({})
174+
175+
const result = resolver.resolve('<variable.anyvar>', createTestContext({}))
176+
expect(result).toBeUndefined()
177+
})
178+
179+
it.concurrent('should handle invalid reference format', () => {
180+
const resolver = new WorkflowResolver({})
181+
182+
const result = resolver.resolve('<variable>', createTestContext({}))
183+
expect(result).toBeUndefined()
184+
})
185+
186+
it.concurrent('should handle null variable values in the map', () => {
187+
const variables: Record<string, any> = {
188+
'var-1': null,
189+
'var-2': { id: 'var-2', name: 'valid', type: 'plain', value: 'exists' },
190+
}
191+
const resolver = new WorkflowResolver(variables)
192+
193+
const result = resolver.resolve('<variable.valid>', createTestContext(variables))
194+
expect(result).toBe('exists')
195+
})
196+
197+
it.concurrent('should handle variable with empty name', () => {
198+
const variables = {
199+
'var-1': { id: 'var-1', name: '', type: 'plain', value: 'empty-name' },
200+
}
201+
const resolver = new WorkflowResolver(variables)
202+
203+
// Empty name normalizes to empty string, which matches "<variable.>" reference
204+
const result = resolver.resolve('<variable.>', createTestContext(variables))
205+
expect(result).toBe('empty-name')
206+
})
207+
208+
it.concurrent('should prefer name match over ID match when both could apply', () => {
209+
const variables = {
210+
apikey: { id: 'apikey', name: 'different', type: 'plain', value: 'by-id' },
211+
'var-2': { id: 'var-2', name: 'apikey', type: 'plain', value: 'by-name' },
212+
}
213+
const resolver = new WorkflowResolver(variables)
214+
215+
const result = resolver.resolve('<variable.apikey>', createTestContext(variables))
216+
expect(['by-id', 'by-name']).toContain(result)
217+
})
218+
})
219+
})

apps/sim/executor/variables/resolvers/workflow.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
type ResolutionContext,
77
type Resolver,
88
} from '@/executor/variables/resolvers/reference'
9+
import { normalizeVariableName } from '@/stores/workflows/utils'
910

1011
const logger = createLogger('WorkflowResolver')
1112

@@ -32,12 +33,17 @@ export class WorkflowResolver implements Resolver {
3233
}
3334

3435
const [_, variableName, ...pathParts] = parts
36+
const normalizedRefName = normalizeVariableName(variableName)
3537

3638
const workflowVars = context.executionContext.workflowVariables || this.workflowVariables
3739

3840
for (const varObj of Object.values(workflowVars)) {
3941
const v = varObj as any
40-
if (v && (v.name === variableName || v.id === variableName)) {
42+
if (!v) continue
43+
44+
// Match by normalized name or exact ID
45+
const normalizedVarName = v.name ? normalizeVariableName(v.name) : ''
46+
if (normalizedVarName === normalizedRefName || v.id === variableName) {
4147
const normalizedType = (v.type === 'string' ? 'plain' : v.type) || 'plain'
4248
let value: any
4349
try {

0 commit comments

Comments
 (0)