feat(workflow-controls): added action bar for picker/hand/undo/redo/zoom workflow controls, added general setting to disabl #2767
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…oom workflow controls, added general setting to disable
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Greptile OverviewGreptile SummaryThis PR introduces a new floating action bar for workflow canvas controls, replacing the previous panel-embedded workflow controls. The action bar provides quick access to hand/cursor mode switching, undo/redo, and zoom controls. Key Architectural Changes:
Implementation Quality:
Issues Found:
The changes are well-structured and follow the codebase's architectural patterns. The main concern is cleanup of the old Shift-based selection logic. Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant ActionBar
participant CanvasModeStore
participant ReactFlow
participant SettingsAPI
participant Database
User->>ActionBar: Click Hand/Cursor button
ActionBar->>CanvasModeStore: setMode('hand' | 'cursor')
CanvasModeStore->>CanvasModeStore: Persist to localStorage
CanvasModeStore->>ReactFlow: Update selectionOnDrag/panOnDrag
ReactFlow->>User: Canvas behavior changes
User->>ActionBar: Right-click context menu
ActionBar->>User: Show "Hide canvas controls"
User->>ActionBar: Click hide
ActionBar->>SettingsAPI: PATCH showActionBar: false
SettingsAPI->>Database: UPDATE settings
Database->>SettingsAPI: Success
SettingsAPI->>ActionBar: Response
ActionBar->>ActionBar: Unmount (showActionBar = false)
User->>ActionBar: Click Zoom to Fit
ActionBar->>ReactFlow: Call useCanvasViewport.fitViewToBounds()
ReactFlow->>ReactFlow: Calculate visible bounds (minus sidebar/panel/terminal)
ReactFlow->>ReactFlow: Center content in visible area
ReactFlow->>User: Animated zoom/pan
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/action-bar/action-bar.tsx
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/action-bar/action-bar.tsx
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx
Outdated
Show resolved
Hide resolved
Collaborator
Author
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/action-bar/action-bar.tsx
Outdated
Show resolved
Hide resolved
| onSelectionStart={onSelectionStart} | ||
| onSelectionEnd={onSelectionEnd} | ||
| panOnDrag={isHandMode ? [0, 1] : false} | ||
| multiSelectionKeyCode={['Meta', 'Control', 'Shift']} |
Contributor
There was a problem hiding this comment.
With the new hand/cursor mode system, having Shift in multiSelectionKeyCode may cause confusion. Previously, Shift was used to toggle selection mode temporarily, but now it's just one of the multi-selection modifiers.
Since the new system uses a persistent mode toggle (hand vs cursor), consider whether Shift should still be in this array. If users press Shift in hand mode, it might create unexpected behavior where they can multi-select even though they're in hand mode.
Consider removing Shift from this array or documenting the intended behavior when Shift is pressed in hand mode.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx
Line: 3326:3326
Comment:
With the new hand/cursor mode system, having Shift in multiSelectionKeyCode may cause confusion. Previously, Shift was used to toggle selection mode temporarily, but now it's just one of the multi-selection modifiers.
Since the new system uses a persistent mode toggle (hand vs cursor), consider whether Shift should still be in this array. If users press Shift in hand mode, it might create unexpected behavior where they can multi-select even though they're in hand mode.
Consider removing Shift from this array or documenting the intended behavior when Shift is pressed in hand mode.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
55
to
61
| const handleHide = async () => { | ||
| try { | ||
| await updateSetting.mutateAsync({ key: 'showActionBar', value: false }) | ||
| } finally { | ||
| setContextMenu(null) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Error handling is incomplete - if updateSetting.mutateAsync fails, the error is silently swallowed. The context menu closes regardless of success or failure, leaving the user unaware that the setting wasn't actually updated.
Consider logging the error or showing a notification to inform the user when the operation fails.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/action-bar/action-bar.tsx
Line: 55:61
Comment:
Error handling is incomplete - if updateSetting.mutateAsync fails, the error is silently swallowed. The context menu closes regardless of success or failure, leaving the user unaware that the setting wasn't actually updated.
Consider logging the error or showing a notification to inform the user when the operation fails.
How can I resolve this? If you propose a fix, please make it concise.
waleedlatif1
commented
Jan 11, 2026
Collaborator
Author
waleedlatif1
left a comment
waleedlatif1
left a comment
There was a problem hiding this comment.
🤖 AI Code Review
Pull Request Review: Workflow Controls Action Bar
🔍 Overview
This PR adds an action bar for workflow controls (picker/hand/undo/redo/zoom) with a general setting to disable it, plus viewport fitting utilities and icon updates.
⚠️ Critical Issues
1. Incomplete PR Title and Description
- The title and description are truncated ("...to disabl")
- Action Required: Complete the description to clearly state what can be disabled and why
2. Missing Test Coverage
- The checklist shows "Tests added/updated and passing" is unchecked
- For a feature touching 30 files, automated tests are essential
- Recommendation: Add unit tests for the new utilities and integration tests for the action bar
📋 General Concerns
Code Quality
Positive:
- Feature appears well-scoped (workflow controls consolidation)
- Includes settings for user customization
Concerns:
- 30 files changed is substantial for a single PR - consider if this could be split into smaller, reviewable chunks:
- Utility functions (fitViewToBounds)
- Icon updates
- Action bar implementation
- Settings integration
Documentation
- No mention of documentation updates
- Questions to address:
- Are there user-facing docs that need updating?
- Is the new setting documented?
- Are the new utilities documented with JSDoc/TSDoc?
🐛 Potential Issues
Without seeing the actual code, here are common pitfalls to check:
1. Viewport/Bounds Calculations
// Ensure fitViewToBounds handles edge cases:
// - Empty/null bounds
// - Negative dimensions
// - Panel/sidebar in collapsed state
// - Multiple monitors with different DPIs
// - Zoom limits (min/max)2. Settings Persistence
- Verify the disable setting persists across sessions
- Check default value is sensible (likely enabled)
- Ensure setting changes apply immediately without requiring reload
3. Undo/Redo State Management
- Confirm undo/redo doesn't conflict with existing keyboard shortcuts
- Verify state consistency when action bar is disabled
- Check for memory leaks in undo history
4. Icon Updates
- Ensure SVG icons are optimized
- Verify accessibility (proper ARIA labels)
- Check icon sizing is consistent across different zoom levels
🔒 Security Considerations
- If settings are stored, ensure they're validated on load
- Verify no XSS vulnerabilities if icons are dynamically loaded
- Check that zoom controls have reasonable min/max limits to prevent DoS
⚡ Performance Implications
Potential Concerns:
- fitViewToBounds calculations - ensure these are debounced/throttled if called frequently
- Panel/sidebar awareness - verify this doesn't cause layout thrashing
- Icon rendering - confirm SVGs are cached appropriately
- Undo/redo history - ensure there's a reasonable limit to prevent memory issues
Recommendations:
// Example: Debounce expensive calculations
const debouncedFitView = debounce(fitViewToBounds, 150);
// Example: Limit undo history
const MAX_UNDO_HISTORY = 50;🎯 Specific Recommendations
1. Code Organization
- Extract the fitViewToBounds utility into a separate, well-tested module
- Consider using a factory pattern for the action bar to improve testability
2. Accessibility
- Ensure all controls have keyboard shortcuts
- Add proper ARIA labels and roles
- Test with screen readers
- Verify focus management
3. User Experience
- Consider adding tooltips to action bar buttons
- Provide visual feedback for disabled state
- Ensure the action bar doesn't obstruct important content
- Consider making the action bar position customizable
4. Testing Strategy
// Suggested test coverage:
// - Unit tests for fitViewToBounds with various panel states
// - Integration tests for action bar interactions
// - E2E tests for undo/redo functionality
// - Visual regression tests for icon updates
// - Accessibility tests (axe-core)This review was automatically generated by an AI assistant.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Type of Change
Testing
Tested manually
Checklist