feat(workflow): resolve canvas re-rendering and logs perf issues#3844
feat(workflow): resolve canvas re-rendering and logs perf issues#3844adithyaakrishna wants to merge 6 commits intosimstudioai:stagingfrom
Conversation
PR SummaryMedium Risk Overview Terminal/output changes: Workflow canvas changes: introduces new hooks ( Written by Cursor Bugbot for commit 0ec73b0. This will update automatically on new commits. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-derivation.ts
Outdated
Show resolved
Hide resolved
...app/workspace/[workspaceId]/w/[workflowId]/components/terminal/hooks/use-terminal-filters.ts
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR fixes severe UI lag during parallel/loop workflow execution by decoupling execution state from canvas node derivation and virtualizing the terminal log list. The approach is architecturally sound and the core optimizations are well-targeted. Key changes:
Issues found:
Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/best-practice issues with no runtime impact. The core optimization (decoupling execution state from canvas node derivation, virtualizing terminal logs) is correct and addresses a real performance problem. No P0/P1 bugs were found. The previous review concern about
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before["Before PR (cascade re-render)"]
ES1[ExecutionStore setActiveBlocks] --> ND1[derivedNodes recomputes ALL nodes]
ND1 --> DN1[displayNodes updates]
DN1 --> RF1[ReactFlow re-renders all WorkflowBlocks]
end
subgraph After["After PR (isolated re-render)"]
ES2[ExecutionStore setActiveBlocks] --> UIBA[useIsBlockActive per-block selector]
UIBA --> BV[useBlockVisual only affected block]
BV --> WB[Only that WorkflowBlock re-renders]
WS[WorkflowStore block structure change] --> ND2[derivedNodes recomputes]
ND2 --> DN2[displayNodes updates all nodes]
end
subgraph Terminal["Terminal Virtualization"]
CE[ConsoleEntries] --> EG[groupEntriesByExecution]
EG --> FVR[flattenVisibleExecutionRows]
FVR --> VL[react-window List virtualized rows]
RSS[RowSignalStore useSyncExternalStore] --> VER[VirtualEntryNodeRow re-renders on change]
VL --> VER
end
Reviews (3): Last reviewed commit: "chore: fix review changes" | Re-trigger Greptile |
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-derivation.ts
Outdated
Show resolved
Hide resolved
|
@adithyaakrishna is attempting to deploy a commit to the Sim Team on Vercel. A member of the Team first needs to authorize it. |
.../sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/terminal/terminal.tsx
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/terminal/terminal.tsx
Show resolved
Hide resolved
|
@greptile review |
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/workflow-canvas-helpers.ts
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/terminal/terminal.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if (prevDataJsonRef.current !== newJson) { | ||
| prevDataJsonRef.current = newJson | ||
| setExpandedPaths(computeInitialPaths(data, isError)) | ||
| } |
There was a problem hiding this comment.
Unguarded JSON.stringify can throw on non-serializable data
Medium Severity
The new deep-equality check calls JSON.stringify(data) without a try-catch. If data contains circular references, BigInt values, or other non-JSON-serializable structures, this will throw an unhandled error and crash the structured output component. The previous code only compared by reference (prevDataRef.current !== data), which can never throw.
Additional Locations (1)
| dataRef.current = { rows } | ||
|
|
||
| const signalStoreRef = useRef(createRowSignalStore()) | ||
| signalStoreRef.current.update(selectedEntryId, expandedNodes, onSelectEntry, onToggleNode) |
There was a problem hiding this comment.
Signal store mutated during React render phase
Low Severity
signalStoreRef.current.update() is called during the render of TerminalLogsPane, not inside a useEffect. When the update detects changes, it synchronously fires all useSyncExternalStore listeners. This is a side effect during render, violating React's requirement that rendering be pure. It can cause issues in React Strict Mode (double invocation) and is fragile in concurrent rendering scenarios.


Summary
Fixes severe UI lag and re-rendering issues on the workflow page during parallel loop execution. The canvas, output panel, input tab, and terminal logs all suffered from cascading re-renders when many iterations ran simultaneously.
Type of Change
Testing
Checklist