feat(export): added the ability to export workflow #2777
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile OverviewGreptile SummaryThis PR adds workflow export functionality and refactors all hooks to use stable references instead of getter functions. While most of the refactoring is sound, there is a critical bug in Key ChangesNew Functionality:
Hook Refactoring:
Critical Issues Found1. BROKEN: Workflow duplicate/export in workflow-item.tsx 🔴Location: Lines 88-97 in The refactoring introduced a critical bug. The hooks are initialized with: workflowIds: capturedSelectionRef.current?.workflowIds || []At render time, Impact: Users cannot duplicate or export workflows from the context menu. Operations will silently fail or do nothing. Previous implementation (correct): Used getter functions that were called at execution time, not render time. 2. Missing dependency in useExportFolder 🟡Location: Line 230 in The Positive Aspects
RecommendationDo not merge until the critical bug in Confidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant WorkflowItem
participant ContextMenu
participant useDuplicateWorkflow
participant useExportWorkflow
participant API
Note over WorkflowItem: Component renders with<br/>capturedSelectionRef.current = null
WorkflowItem->>useDuplicateWorkflow: Initialize hook with<br/>workflowIds = [] (empty!)
WorkflowItem->>useExportWorkflow: Initialize hook with<br/>workflowIds = [] (empty!)
Note over useDuplicateWorkflow,useExportWorkflow: Hooks capture empty array<br/>in their closures
User->>WorkflowItem: Right-click on workflow
WorkflowItem->>WorkflowItem: captureSelectionState()
WorkflowItem->>WorkflowItem: Updates capturedSelectionRef.current<br/>with selected workflow IDs
WorkflowItem->>ContextMenu: Show context menu
User->>ContextMenu: Click "Duplicate"
ContextMenu->>useDuplicateWorkflow: handleDuplicateWorkflow()
Note over useDuplicateWorkflow: BUG: Hook callback still has<br/>empty array [] from initialization!
useDuplicateWorkflow->>useDuplicateWorkflow: Check if workflowIds exists
useDuplicateWorkflow-->>ContextMenu: Early return (no workflows to duplicate)
Note over User: Operation fails silently.<br/>Nothing happens!
rect rgb(255, 240, 240)
Note over WorkflowItem,API: SAME BUG AFFECTS EXPORT
User->>ContextMenu: Click "Export"
ContextMenu->>useExportWorkflow: handleExportWorkflow()
useExportWorkflow->>useExportWorkflow: Check if workflowIds exists
useExportWorkflow-->>ContextMenu: Early return (no workflows to export)
end
Note over User,API: FIX NEEDED: Use getter functions<br/>or pass IDs to handler at call time
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/sim/app/workspace/[workspaceId]/w/hooks/use-export-folder.ts
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/hooks/use-export-folder.ts
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/hooks/use-export-folder.ts
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/hooks/use-export-folder.ts
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/hooks/use-import-workflow.ts
Outdated
Show resolved
Hide resolved
Additional Comments (1)
Looking at Consider whether duplicate should really be disabled for empty folders, or if this is conflating two different concerns. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/folder-item/folder-item.tsx
Line: 383:383
Comment:
The duplicate functionality is disabled when a folder has no workflows (`!hasWorkflows`), but the export functionality also has this same condition (`!hasWorkflows`). This means:
1. Both operations require workflows to exist (correct for export)
2. However, disabling duplicate for empty folders may not be intended behavior
Looking at `useDuplicateFolder.ts`, the mutation will still work for empty folders (it duplicates the folder structure). The `hasWorkflows` check is coming from `useExportFolder` which makes sense for export, but applying it to duplicate seems incorrect.
Consider whether duplicate should really be disabled for empty folders, or if this is conflating two different concerns.
How can I resolve this? If you propose a fix, please make it concise. |
| const { handleDuplicateWorkflow } = useDuplicateWorkflow({ | ||
| workspaceId, | ||
| getWorkflowIds: () => { | ||
| // Use the selection captured at right-click time | ||
| return capturedSelectionRef.current?.workflowIds || [] | ||
| }, | ||
| workflowIds: capturedSelectionRef.current?.workflowIds || [], | ||
| }) | ||
|
|
||
| // Export workflow hook | ||
| // Export workflow hook (uses captured selection from right-click) | ||
| const { handleExportWorkflow } = useExportWorkflow({ | ||
| workspaceId, | ||
| getWorkflowIds: () => { | ||
| // Use the selection captured at right-click time | ||
| return capturedSelectionRef.current?.workflowIds || [] | ||
| }, | ||
| workflowIds: capturedSelectionRef.current?.workflowIds || [], | ||
| }) |
There was a problem hiding this comment.
Critical Bug: Hooks initialized with stale empty array
The useDuplicateWorkflow and useExportWorkflow hooks are being called with capturedSelectionRef.current?.workflowIds || [] at component render time. When the component first renders, capturedSelectionRef.current is null (initialized on line 65), so both hooks are initialized with an empty array [].
React hooks capture their prop values at initialization. Even though capturedSelectionRef.current is later updated in captureSelectionState() (line 174), the hooks don't see this update because they were already initialized with the empty array.
Result: When users try to duplicate or export workflows, the operations will fail silently or do nothing because the hooks have an empty workflowIds array.
The old implementation was correct - it used getter functions getWorkflowIds: () => capturedSelectionRef.current?.workflowIds || [] which were called at execution time, not render time.
Fix: Either:
- Revert to using getter functions for these two hooks, OR
- Move these hook calls inside the handler functions (which would require refactoring the hooks to not be hooks), OR
- Pass the workflowIds directly to the handler functions instead of to the hooks
| const { handleDuplicateWorkflow } = useDuplicateWorkflow({ | |
| workspaceId, | |
| getWorkflowIds: () => { | |
| // Use the selection captured at right-click time | |
| return capturedSelectionRef.current?.workflowIds || [] | |
| }, | |
| workflowIds: capturedSelectionRef.current?.workflowIds || [], | |
| }) | |
| // Export workflow hook | |
| // Export workflow hook (uses captured selection from right-click) | |
| const { handleExportWorkflow } = useExportWorkflow({ | |
| workspaceId, | |
| getWorkflowIds: () => { | |
| // Use the selection captured at right-click time | |
| return capturedSelectionRef.current?.workflowIds || [] | |
| }, | |
| workflowIds: capturedSelectionRef.current?.workflowIds || [], | |
| }) | |
| // Duplicate workflow hook (uses captured selection from right-click) | |
| const { handleDuplicateWorkflow: duplicateWorkflow } = useDuplicateWorkflow({ | |
| workspaceId, | |
| workflowIds: [], // Pass empty array initially, will be overridden in handler | |
| }) | |
| // Wrapper that passes the actual workflowIds at execution time | |
| const handleDuplicateWorkflow = useCallback(() => { | |
| const workflowIds = capturedSelectionRef.current?.workflowIds || [] | |
| if (workflowIds.length === 0) return | |
| // Call the underlying duplicate function with fresh workflowIds | |
| duplicateWorkflow(workflowIds) | |
| }, [duplicateWorkflow]) | |
| // Export workflow hook (uses captured selection from right-click) | |
| const { handleExportWorkflow: exportWorkflow } = useExportWorkflow({ | |
| workspaceId, | |
| workflowIds: [], // Pass empty array initially, will be overridden in handler | |
| }) | |
| // Wrapper that passes the actual workflowIds at execution time | |
| const handleExportWorkflow = useCallback(() => { | |
| const workflowIds = capturedSelectionRef.current?.workflowIds || [] | |
| if (workflowIds.length === 0) return | |
| // Call the underlying export function with fresh workflowIds | |
| exportWorkflow(workflowIds) | |
| }, [exportWorkflow]) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/workflow-item/workflow-item.tsx
Line: 88:97
Comment:
**Critical Bug: Hooks initialized with stale empty array**
The `useDuplicateWorkflow` and `useExportWorkflow` hooks are being called with `capturedSelectionRef.current?.workflowIds || []` at component render time. When the component first renders, `capturedSelectionRef.current` is `null` (initialized on line 65), so both hooks are initialized with an empty array `[]`.
React hooks capture their prop values at initialization. Even though `capturedSelectionRef.current` is later updated in `captureSelectionState()` (line 174), the hooks don't see this update because they were already initialized with the empty array.
**Result**: When users try to duplicate or export workflows, the operations will fail silently or do nothing because the hooks have an empty workflowIds array.
**The old implementation was correct** - it used getter functions `getWorkflowIds: () => capturedSelectionRef.current?.workflowIds || []` which were called at execution time, not render time.
**Fix**: Either:
1. Revert to using getter functions for these two hooks, OR
2. Move these hook calls inside the handler functions (which would require refactoring the hooks to not be hooks), OR
3. Pass the workflowIds directly to the handler functions instead of to the hooks
```suggestion
// Duplicate workflow hook (uses captured selection from right-click)
const { handleDuplicateWorkflow: duplicateWorkflow } = useDuplicateWorkflow({
workspaceId,
workflowIds: [], // Pass empty array initially, will be overridden in handler
})
// Wrapper that passes the actual workflowIds at execution time
const handleDuplicateWorkflow = useCallback(() => {
const workflowIds = capturedSelectionRef.current?.workflowIds || []
if (workflowIds.length === 0) return
// Call the underlying duplicate function with fresh workflowIds
duplicateWorkflow(workflowIds)
}, [duplicateWorkflow])
// Export workflow hook (uses captured selection from right-click)
const { handleExportWorkflow: exportWorkflow } = useExportWorkflow({
workspaceId,
workflowIds: [], // Pass empty array initially, will be overridden in handler
})
// Wrapper that passes the actual workflowIds at execution time
const handleExportWorkflow = useCallback(() => {
const workflowIds = capturedSelectionRef.current?.workflowIds || []
if (workflowIds.length === 0) return
// Call the underlying export function with fresh workflowIds
exportWorkflow(workflowIds)
}, [exportWorkflow])
```
How can I resolve this? If you propose a fix, please make it concise.| } finally { | ||
| setIsExporting(false) | ||
| } | ||
| }, [folderId, isExporting, workflows, onSuccess]) |
There was a problem hiding this comment.
Missing folders dependency in useCallback
The handleExportFolder callback uses folderStore.folders on line 120 (collectWorkflowsInFolder(folderId, workflows, folderStore.folders)), but folders is not included in the dependency array.
The folders state comes from useFolderStore() on line 67 and should be included in the dependencies to ensure the callback is recreated when folders change.
| }, [folderId, isExporting, workflows, onSuccess]) | |
| }, [folderId, isExporting, workflows, folders, onSuccess]) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/hooks/use-export-folder.ts
Line: 230:230
Comment:
**Missing `folders` dependency in useCallback**
The `handleExportFolder` callback uses `folderStore.folders` on line 120 (`collectWorkflowsInFolder(folderId, workflows, folderStore.folders)`), but `folders` is not included in the dependency array.
The `folders` state comes from `useFolderStore()` on line 67 and should be included in the dependencies to ensure the callback is recreated when folders change.
```suggestion
}, [folderId, isExporting, workflows, folders, onSuccess])
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
Type of Change
Testing
Tested manually
Checklist