-
Notifications
You must be signed in to change notification settings - Fork 3.2k
v0.3.2: improvement + fix + feat #708
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
waleedlatif1
commented
Jul 16, 2025
- feat(sidebar): sidebar toggle and search (feat(sidebar): sidebar toggle and search #700)
- fix(schedule): fix for custom cron (fix(schedule): fix for custom cron #699)
- fix(sockets): delete block case (fix(sockets): delete block edge case #703)
- fix(invitation): allow admins to remove members from workspace (fix(invitation): allow admins to remove members from workspace #701)
- improvement(voice): interrupt UI + mute mic while agent is talking (improvement(voice): interrupt UI + mute mic while agent is talking #705)
- fix: permissions check for duplicating workflow (fix: permissions check for duplicating workflow #706)
- fix(subflow): fixed subflow execution regardless of path decision (fix(subflow): fixed subflow execution regardless of path decision #707)
* fix: sidebar toggle * feat: search complete
* fix: added cronExpression field and fixed formatting * fix: modified the test.ts file #699 * added additional validation --------- Co-authored-by: Adam Gough <adamgough@Mac.attlocal.net> Co-authored-by: Waleed Latif <walif6@gmail.com>
* fix(invitation): added ability for admins to remove members of their workspace * lint * remove references to workspace_member db table * remove deprecated @next/font * only allow admin to rename workspace * bring workflow name change inline, remove dialog
* fix typo in docs file * fix(subflows): fixed subflows executing irrespective of active path * added routing strategy * reorganized executor * brought folder renaming inline * cleanup
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
✅ No security or compliance issues detected. Reviewed everything up to 92fe353. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. Reply to this PR with |
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 Summary
This PR (v0.3.2) introduces several major improvements and critical fixes across the codebase:
-
Path-Aware Execution System: A significant architectural improvement that fixes subflow execution by ensuring blocks only execute when explicitly selected by router/condition blocks. This includes:
- New PathTracker integration for loop and parallel handlers
- Block categorization into routing, flow control, and regular blocks
- Comprehensive test coverage for nested routing scenarios
-
Code Organization & Type Safety:
- Migration from relative imports to path aliases using '@/executor/*' pattern
- Introduction of BlockType enum replacing string literals for block types
- Improved folder structure with dedicated directories for core components
-
Feature Improvements:
- Sidebar toggle and global search (Cmd+K shortcut)
- Voice interface interruption capability and audio sync
- Custom cron expression validation
- Enhanced workspace member management for admins
-
Permission System Overhaul:
- Consolidated access control using permissions table
- Removed redundant workspaceMember table
- Improved invitation system with better email matching
Confidence score: 4 /5
- This PR is generally safe to merge with careful testing of workflow execution paths
- While the changes are extensive, they are well-tested and improve system reliability. The lower score is due to the significant changes to core execution logic
- Key files needing attention:
- apps/sim/executor/index.ts (core execution logic changes)
- apps/sim/executor/routing/routing.ts (new path tracking system)
- apps/sim/app/api/workspaces/* (permission system changes)
78 files reviewed, 16 comments
Edit PR Review Bot Settings | Greptile
| const mockQuery = { | ||
| from: vi.fn().mockReturnThis(), | ||
| where: vi.fn().mockReturnThis(), | ||
| then: vi.fn((callback: (rows: any[]) => any) => { |
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.
style: consider adding type definition for callback parameter instead of using any[]
| const cronExpression = getSubBlockValue(starterBlock, 'cronExpression') || null | ||
|
|
||
| // Validate cron expression if provided | ||
| if (cronExpression) { | ||
| const validation = validateCronExpression(cronExpression) | ||
| if (!validation.isValid) { | ||
| throw new Error(`Invalid cron expression: ${validation.error}`) | ||
| } | ||
| } |
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.
style: Consider moving validation logic into its own function to keep getScheduleTimeValues more focused on value extraction
| wsInvitation.expiresAt && | ||
| new Date().toISOString() <= wsInvitation.expiresAt.toISOString() | ||
| ) { |
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.
style: same date comparison issue here - use native Date objects instead of string conversion
| wsInvitation.expiresAt && | |
| new Date().toISOString() <= wsInvitation.expiresAt.toISOString() | |
| ) { | |
| wsInvitation.expiresAt && | |
| new Date() <= wsInvitation.expiresAt | |
| ) { |
| stars={template.stars} | ||
| icon={template.icon} | ||
| iconColor={template.color} | ||
| state={template.state as { blocks?: Record<string, { type: string; name?: string }> }} |
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.
style: type assertion to Record<string, { type: string; name?: string }> might be unsafe. Consider defining a proper type
| isOwner: eq(workspace.ownerId, memberId), | ||
| permissionType: permissions.permissionType, | ||
| joinedAt: workspaceMember.joinedAt, | ||
| createdAt: permissions.createdAt, |
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.
style: Consider adding an explicit alias for permissions.createdAt since it's used as both joinedAt and createdAt in the response
| const fetchTemplates = useCallback(async () => { | ||
| setIsTemplatesLoading(true) | ||
| try { | ||
| // Fetch templates from API, ordered by views (most popular first) | ||
| const response = await fetch('/api/templates?limit=8&offset=0') | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch templates: ${response.status}`) | ||
| } | ||
|
|
||
| const apiResponse = await response.json() | ||
|
|
||
| // Map API response to TemplateData format | ||
| const fetchedTemplates: TemplateData[] = | ||
| apiResponse.data?.map((template: any) => ({ | ||
| id: template.id, | ||
| title: template.name, | ||
| description: template.description || '', | ||
| author: template.author, | ||
| usageCount: formatUsageCount(template.views || 0), | ||
| stars: template.stars || 0, | ||
| icon: template.icon || 'FileText', | ||
| iconColor: template.color || '#6B7280', | ||
| state: template.state, | ||
| isStarred: template.isStarred || false, | ||
| })) || [] | ||
|
|
||
| setTemplates(fetchedTemplates) | ||
| logger.info(`Templates loaded successfully: ${fetchedTemplates.length} templates`) | ||
| } catch (error) { | ||
| logger.error('Error fetching templates:', error) | ||
| // Set empty array on error | ||
| setTemplates([]) | ||
| } finally { | ||
| setIsTemplatesLoading(false) | ||
| } | ||
| }, []) |
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.
logic: Consider using a type-safe API response interface instead of 'any' for template mapping
| it('should test the full executor flow and see what happens', async () => { | ||
| // Mock the necessary functions to avoid actual API calls | ||
| const mockInput = {} | ||
|
|
||
| try { | ||
| // Execute the workflow | ||
| const result = await executor.execute('test-workflow-id') | ||
|
|
||
| // Check if it's an ExecutionResult (not StreamingExecution) | ||
| if ('success' in result) { | ||
| // Check if there are any logs that might indicate what happened | ||
| if (result.logs) { | ||
| } | ||
|
|
||
| // The test itself doesn't need to assert anything specific | ||
| // We just want to see what the executor does | ||
| expect(result.success).toBeDefined() | ||
| } else { | ||
| expect(result).toBeDefined() | ||
| } | ||
| } catch (error) { | ||
| console.error('Execution error:', error) | ||
| // Log the error but don't fail the test - we want to see what happens | ||
| } | ||
| }) |
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.
style: First test is incomplete - empty if block and no specific assertions. Either add meaningful assertions or remove test.
| // Create a mock context in the exact state after the condition executes | ||
| const context = (executor as any).createExecutionContext('test-workflow', new Date()) |
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.
style: Using 'as any' to access private method createExecutionContext. Consider making method protected or adding test utilities.
| const flowControlHandles = [ | ||
| 'parallel-start-source', | ||
| 'parallel-end-source', | ||
| 'loop-start-source', | ||
| 'loop-end-source', | ||
| ] |
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.
style: Consider moving flowControlHandles to a constant or configuration object for better maintainability
| } catch (error) { | ||
| logger.warn(`PathTracker check failed for ${blockType} block ${block.id}:`, error) | ||
| // Default to true to maintain existing behavior if PathTracker fails | ||
| isInActivePath = true | ||
| } |
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.
logic: Default to true in error case could mask issues. Consider adding error details to logs