-
Notifications
You must be signed in to change notification settings - Fork 3.1k
improvement(variables): add error context for duplicate variable names, only check for collision when focus is lost #1791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…s, only check for collision when focus is lost
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Improved variable name validation by deferring duplicate checks until blur event instead of on every keystroke, preventing auto-generated (n+1) suffixes that were confusing to users.
Major Changes:
- Added local state management (
localNames,nameErrors) invariables.tsxto track temporary edits and validation errors - Implemented
handleVariableNameBlur()to validate duplicate names only when user finishes editing - Added visual error feedback with red border and error message for duplicate names
- Removed automatic
(n+1)suffix generation from store, now throws error instead - Refactored type configuration into
TYPE_CONFIGconstant andVARIABLE_TYPESarray for better maintainability - Simplified duplicate dropdown menu items by using array mapping
Issues Found:
- Empty variable names can be saved to the store (missing validation in
handleVariableNameBlur) - Performance:
useEffectat line 221 iterates over all variables on everyworkflowVariableschange, which could be optimized
Confidence Score: 3/5
- This PR has one critical logic bug that needs to be fixed before merging
- The implementation is well-structured and achieves the stated goal of improving UX by deferring validation. However, there's a critical bug where empty variable names can be saved to the store, which could break workflows. The performance concern with the useEffect is minor but worth addressing.
- Pay close attention to
variables.tsx:110-126- the empty name validation bug must be fixed
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/variables/variables.tsx | 4/5 | Refactored duplicate name validation to use local state with blur-based checking, added error UI feedback. Simplified type configuration using constants. |
| apps/sim/stores/panel/variables/store.ts | 5/5 | Removed auto-incrementing (n+1) suffix logic from store, maintaining only the initial duplicate name handling for new variables. |
Sequence Diagram
sequenceDiagram
participant User
participant Variables Component
participant LocalState
participant CollaborativeWorkflow
participant VariablesStore
User->>Variables Component: Types in variable name
Variables Component->>LocalState: Update localNames[variableId]
Variables Component->>LocalState: Clear nameErrors[variableId]
User->>Variables Component: Blur input field
Variables Component->>Variables Component: handleVariableNameBlur()
Variables Component->>Variables Component: Check if trimmedName is duplicate
alt Duplicate name found
Variables Component->>LocalState: Set nameErrors[variableId]
Variables Component->>User: Show error UI (red border)
else No duplicate
Variables Component->>CollaborativeWorkflow: collaborativeUpdateVariable(id, 'name', trimmedName)
CollaborativeWorkflow->>VariablesStore: updateVariable(id, {name})
VariablesStore->>VariablesStore: Update variable name
VariablesStore->>VariablesStore: Update references in workflow
Variables Component->>LocalState: clearLocalState(variableId)
Variables Component->>User: Name updated successfully
end
Note over Variables Component,LocalState: useEffect syncs localNames<br/>when variables are deleted
2 files reviewed, 2 comments
...p/workspace/[workspaceId]/w/[workflowId]/components/panel/components/variables/variables.tsx
Show resolved
Hide resolved
...p/workspace/[workspaceId]/w/[workflowId]/components/panel/components/variables/variables.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR improves variable name validation by deferring duplicate checking until focus is lost (onBlur), preventing validation on every keystroke. Local state now tracks edited names before they're committed to the store.
Key changes:
- Replaced immediate
collaborativeUpdateVariableon onChange with local state (localNames) that buffers changes - Added
handleVariableNameBlurto validate and save on blur, checking for empty names and duplicates - Removed the automatic "(n)" suffix for duplicate names from store logic
- Added
nameErrorsstate to display validation errors with red styling - Added Enter key handler to trigger blur/validation
Issues identified:
- Critical: Empty variable names can still be saved to the store when a variable already has a non-empty persisted name. The validation in
handleVariableNameBlurreturns early iflocalName === undefined, meaning if user clears the input and never focuses it, the empty string fromcollaborativeAddVariablecall (line 149:name: '') remains in the store - Performance: The
useEffecton lines 229-250 runs on everyworkflowVariableschange and iterates over all variables to sync local state, causing unnecessary re-renders even when nothing changed for most variables
Confidence Score: 2/5
- This PR introduces a critical bug that allows empty variable names to persist in the store
- The validation logic has a flaw where empty names can bypass validation. When
handleAddVariablecreates a variable withname: ''(line 149), if the input never receives focus,localNameremains undefined and the blur handler returns early without setting an error. Additionally, the re-render issue in the sync effect could impact performance in workflows with many variables - Pay close attention to the validation logic in
handleVariableNameBlurand the performance implications of the sync effect on lines 229-250
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/variables/variables.tsx | 2/5 | Added variable name validation on blur with duplicate checking and empty name prevention, but logic allows empty names to be saved despite validation attempt |
Sequence Diagram
sequenceDiagram
participant User
participant Input
participant Handler
participant Validation
participant Store
User->>Input: Types variable name
Input->>Handler: onChange event
Handler->>Validation: validateName(newName)
Validation-->>Handler: validatedName (strips invalid chars)
Handler->>Handler: setLocalNames(validatedName)
Handler->>Handler: clearError(variableId)
User->>Input: Blur or press Enter
Input->>Handler: onBlur event
Handler->>Handler: Check if localName exists
alt localName is undefined
Handler-->>Input: Return early (no changes)
end
Handler->>Handler: trim localName
alt trimmedName is empty
Handler->>Handler: setNameErrors("Variable name cannot be empty")
Handler-->>Input: Return (error set, no store update)
Note over Store: Empty name NOT saved to store
end
Handler->>Validation: isDuplicateName(variableId, trimmedName)
Validation->>Store: Check workflowVariables
alt Duplicate found
Handler->>Handler: setNameErrors("Two variables cannot have the same name")
Handler-->>Input: Return (error set, no store update)
end
Handler->>Store: collaborativeUpdateVariable(variableId, 'name', trimmedName)
Store-->>Store: Update persisted name
Handler->>Handler: clearLocalState(variableId)
Handler-->>Input: Success
1 file reviewed, no comments
…s, only check for collision when focus is lost (#1791) * improvement(variables): add error context for duplicate variable names, only check for collision when focus is lost * disallow empty variable names, performance optimizations * safety guard against empty variables names
Summary
Type of Change
Testing
Tested manually
Checklist