fix(copilot): edit workflow race condition #1036
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR addresses a race condition in the copilot workflow editing system by implementing a comprehensive architectural refactor. The main changes involve:
Core Race Condition Fix: The primary issue was that multiple components (CopilotStore, client tools, diff store) were simultaneously attempting to update workflow state during edit operations, causing timing conflicts and inconsistent UI behavior.
Architectural Changes:
- New Client Tool Execution Pattern: Refactored
EditWorkflowClientToolandBuildWorkflowClientToolto use a three-phase execution model: (1) execute logic via dedicated server endpoints (/api/copilot/workflows/edit/execute,/api/copilot/workflows/build/execute), (2) wait for any ongoing message operations using polling with timeout, and (3) separately notify completion via/api/copilot/tools/complete - State Management Separation: Modified CopilotStore to skip diff updates for client-handled tools (
edit_workflow,build_workflow) since they now manage their own state transitions, preventing dual ownership conflicts - Unified Workflow State Access: Introduced
buildUserWorkflowJsonhelper function that implements hierarchical fallback (diff store → workflow store → registry) with proper subblock merging, ensuring all tools access consistent workflow state
Server-Side Improvements:
- Parallel Data Prefetching: Updated copilot chat API to prefetch blocks/tools, environment variables, and OAuth credentials in parallel using
Promise.all, reducing sequential API calls and improving performance - Block Sanitization: Added validation to filter out invalid blocks missing required properties (type, name) in diff creation to prevent canvas errors
- Retry Mechanism: Implemented simple retry logic for 5xx HTTP errors in copilot API requests
UI Consistency: Standardized all copilot tool icons from 'skip' to 'circle-slash' for rejected states across 20+ files, providing consistent visual feedback.
The solution transforms a concurrent state management problem into a controlled sequential execution flow while maintaining backward compatibility and improving overall system reliability.
Confidence score: 4/5
- This PR addresses a complex race condition with a well-architected solution that separates concerns and prevents concurrent state updates
- Score reflects the comprehensive nature of changes across multiple critical files and the complexity of the timing coordination logic
- Pay close attention to the new execution flow in
edit-workflow.tsandbuild-workflow.ts, the state management logic incopilot/store.ts, and the workflow state access patterns inworkflow-helpers.ts
26 files reviewed, 8 comments
| try { | ||
| const { | ||
| buildUserWorkflowJson, | ||
| } = require('@/lib/copilot/tools/client-tools/workflow-helpers') | ||
| return { userWorkflow: buildUserWorkflowJson(workflowId) } | ||
| } catch (e) { | ||
| logger.warn('Failed to build userWorkflow for prefetch; continuing without it') | ||
| return {} | ||
| } |
There was a problem hiding this comment.
style: Using require() inside a conditional is risky - if the module fails to load, this could break the entire request. Consider importing at the top level or using dynamic import() with proper error handling.
| gitbranch: GitBranch, // Git branching icon showing workflow paths | ||
| brain: Brain, // Brain icon for reasoning/AI thinking | ||
| listTodo: ListTodo, // List with checkboxes for planning/todos | ||
| 'circle-slash': CircleSlash, |
There was a problem hiding this comment.
style: Redundant entry - 'circle-slash' maps to the same icon as 'skip' on line 56
| // Remove invalid blocks that are missing required properties to avoid canvas warnings | ||
| try { | ||
| const rawBlocks = result.diff.proposedState.blocks || {} | ||
| const sanitizedBlocks: Record<string, any> = {} | ||
| Object.entries(rawBlocks).forEach(([id, block]: [string, any]) => { | ||
| if (block && typeof block === 'object' && block.type && block.name) { | ||
| sanitizedBlocks[id] = block | ||
| } else { | ||
| logger.warn(`[${requestId}] Dropping invalid proposed block`, { id, block }) | ||
| } | ||
| }) | ||
| result.diff.proposedState.blocks = sanitizedBlocks | ||
| } catch (e) { | ||
| logger.warn(`[${requestId}] Failed to sanitize proposed blocks`, e) | ||
| } |
There was a problem hiding this comment.
style: Consider extracting this sanitization logic into a reusable function to avoid code duplication with lines 293-306
| // Remove invalid blocks that are missing required properties to avoid canvas warnings | |
| try { | |
| const rawBlocks = result.diff.proposedState.blocks || {} | |
| const sanitizedBlocks: Record<string, any> = {} | |
| Object.entries(rawBlocks).forEach(([id, block]: [string, any]) => { | |
| if (block && typeof block === 'object' && block.type && block.name) { | |
| sanitizedBlocks[id] = block | |
| } else { | |
| logger.warn(`[${requestId}] Dropping invalid proposed block`, { id, block }) | |
| } | |
| }) | |
| result.diff.proposedState.blocks = sanitizedBlocks | |
| } catch (e) { | |
| logger.warn(`[${requestId}] Failed to sanitize proposed blocks`, e) | |
| } | |
| // Remove invalid blocks that are missing required properties to avoid canvas warnings | |
| const sanitizeBlocks = (blocks: Record<string, any>, context: string): Record<string, any> => { | |
| const sanitized: Record<string, any> = {} | |
| Object.entries(blocks).forEach(([id, block]: [string, any]) => { | |
| if (block && typeof block === 'object' && block.type && block.name) { | |
| sanitized[id] = block | |
| } else { | |
| logger.warn(`[${requestId}] Dropping invalid ${context} block`, { id, block }) | |
| } | |
| }) | |
| return sanitized | |
| } | |
| try { | |
| const rawBlocks = result.diff.proposedState.blocks || {} | |
| result.diff.proposedState.blocks = sanitizeBlocks(rawBlocks, 'proposed') | |
| } catch (e) { | |
| logger.warn(`[${requestId}] Failed to sanitize proposed blocks`, e) | |
| } |
|
|
||
| // Prefer diff/preview store if it has content | ||
| const diffStore = useWorkflowDiffStore.getState() | ||
| let workflowState: any = null |
There was a problem hiding this comment.
style: Using any type loses type safety. Consider using WorkflowState | null or a union type.
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
| if (workflowState) { | ||
| if (!workflowState.loops) workflowState.loops = {} | ||
| if (!workflowState.parallels) workflowState.parallels = {} | ||
| if (!workflowState.edges) workflowState.edges = [] | ||
| if (!workflowState.blocks) workflowState.blocks = {} | ||
| } |
There was a problem hiding this comment.
style: State mutation could cause side effects. Consider creating a new object instead of mutating workflowState.
| toolId: z.string().min(1), | ||
| methodId: z.string().min(1), | ||
| success: z.boolean(), | ||
| data: z.any().optional(), |
There was a problem hiding this comment.
style: Avoid using z.any() for better type safety. Consider defining a proper schema for the expected data structure.
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
| data: execResult.data, | ||
| }), | ||
| }) | ||
| } catch {} |
There was a problem hiding this comment.
style: Empty catch block silently ignores completion notification failures. Consider logging the error for debugging purposes
| } catch {} | |
| } catch (error) { | |
| logger.warn('Failed to notify completion to agent', { | |
| error: error instanceof Error ? error.message : String(error), | |
| }) | |
| } |
| hasDescription: !!description, | ||
| yamlLength: yamlContent.length, | ||
| // 1) Call logic-only execute route to get build result without emitting completion | ||
| const execResp = await fetch('/api/copilot/workflows/build/execute', { |
There was a problem hiding this comment.
logic: This route /api/copilot/workflows/build/execute doesn't exist in the repository. Should this be calling /api/copilot/methods with build_workflow methodId instead?
| data: execResult.data, | ||
| }), | ||
| }) | ||
| } catch {} |
There was a problem hiding this comment.
style: Empty catch block silently ignores completion notification failures. Consider logging the error for debugging.
* Fixes * Updates * Build workflow names * Refactor icons * Lint * Update build tool name
Summary
Fixes edit workflow race condition and some other bugs
Type of Change
Testing
Manual
Checklist