fix(web): surface workflow-def fetch error in execution graph (#1683)#1698
Conversation
WorkflowExecution dropped error/pending state from the workflowDefinition useQuery, so the right-pane graph rendered "Loading graph..." indefinitely when the fetch failed (issue coleam00#1683 example: graph build never finishes, spinner loops forever). Capture error and isPending, mirror the existing run-query message conversion, and split the fallback into three branches: graph ready -> WorkflowDagViewer fetch error -> Failed to load workflow graph + message pending -> spinner neither -> "Workflow graph unavailable for this run." The empty branch covers runs where workflowName resolves but the server has no nodes (older runs, deleted workflow defs).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesDAG Definition Loading Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/components/workflows/WorkflowExecution.tsx (1)
564-578: ⚡ Quick winConsider adding retry capability to the error UI.
The error state is now properly surfaced, but combined with
staleTime: Infinity(line 293), once the query fails it will remain cached indefinitely. Users seeing "Failed to load workflow graph" have no way to retry without refreshing the page.Consider adding a retry button to the error UI:
<div className="flex flex-col items-center justify-center h-full text-text-secondary px-4 text-center"> <p className="text-error mb-1">Failed to load workflow graph</p> <p className="text-xs mb-3">{dagDefinitionErrorMessage}</p> <button onClick={() => queryClient.invalidateQueries({ queryKey: ['workflowDefinition', initialData?.workflowName, codebaseCwd] })} className="text-sm text-primary hover:text-accent-bright" > Retry </button> </div>Alternatively, reduce
staleTimefromInfinityto allow automatic retry on component remount or after a reasonable interval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/components/workflows/WorkflowExecution.tsx` around lines 564 - 578, The error UI currently shows dagDefinitionErrorMessage but offers no retry because the query uses staleTime: Infinity; update the error block (the JSX branch that checks dagDefinitionErrorMessage) to add a Retry button that calls queryClient.invalidateQueries with the same key used to fetch the workflow definition (e.g., ['workflowDefinition', initialData?.workflowName, codebaseCwd]) so users can re-trigger the fetch, or alternatively change the query's staleTime from Infinity to a finite interval to allow automatic refetch on remount; modify the component that defines the query and the JSX branch that renders dagDefinitionErrorMessage to implement one of these fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/web/src/components/workflows/WorkflowExecution.tsx`:
- Around line 564-578: The error UI currently shows dagDefinitionErrorMessage
but offers no retry because the query uses staleTime: Infinity; update the error
block (the JSX branch that checks dagDefinitionErrorMessage) to add a Retry
button that calls queryClient.invalidateQueries with the same key used to fetch
the workflow definition (e.g., ['workflowDefinition', initialData?.workflowName,
codebaseCwd]) so users can re-trigger the fetch, or alternatively change the
query's staleTime from Infinity to a finite interval to allow automatic refetch
on remount; modify the component that defines the query and the JSX branch that
renders dagDefinitionErrorMessage to implement one of these fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6271748-3231-4c6a-b774-c8dedb34a632
📒 Files selected for processing (1)
packages/web/src/components/workflows/WorkflowExecution.tsx
With staleTime: Infinity the cached error stays put until the page reloads. Reuses the existing queryClient and the same queryKey the useQuery declares, so invalidateQueries triggers a fresh fetch.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/components/workflows/WorkflowExecution.tsx (1)
555-580: ⚡ Quick winFallback to runtime DAG data is still bypassed on definition fetch errors.
When
isDagis true due to runtime nodes butdagDefinitionNodesis null, thedagDefinitionErrorMessagebranch blocks graph rendering entirely. This leaves users with an error screen even when run-time DAG data exists. Please either (a) render a degraded graph from runtime nodes in this branch, or (b) remove/adjust the Line 301 fallback claim to match behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/components/workflows/WorkflowExecution.tsx` around lines 555 - 580, The error branch currently shown when dagDefinitionNodes is null prevents rendering a runtime DAG even if isDag is true and workflow.dagNodes contains runtime nodes; update the conditional so that when dagDefinitionNodes is falsy but isDag (or workflow.dagNodes) exists you render a degraded WorkflowDagViewer using workflow.dagNodes (pass liveStatus={workflow.dagNodes}, currentlyExecuting, isRunning, selectedNodeId, onNodeClick) instead of the static error screen, otherwise fall back to showing dagDefinitionErrorMessage; adjust the JSX around dagDefinitionNodes/dagDefinitionErrorMessage to prefer runtime rendering when available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/web/src/components/workflows/WorkflowExecution.tsx`:
- Around line 555-580: The error branch currently shown when dagDefinitionNodes
is null prevents rendering a runtime DAG even if isDag is true and
workflow.dagNodes contains runtime nodes; update the conditional so that when
dagDefinitionNodes is falsy but isDag (or workflow.dagNodes) exists you render a
degraded WorkflowDagViewer using workflow.dagNodes (pass
liveStatus={workflow.dagNodes}, currentlyExecuting, isRunning, selectedNodeId,
onNodeClick) instead of the static error screen, otherwise fall back to showing
dagDefinitionErrorMessage; adjust the JSX around
dagDefinitionNodes/dagDefinitionErrorMessage to prefer runtime rendering when
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e559c02-1748-4961-8996-5571b3be973e
📒 Files selected for processing (1)
packages/web/src/components/workflows/WorkflowExecution.tsx
Review SummaryVerdict: minor-fixes-needed Your PR adds a helpful user-facing improvement — surfacing errors and providing a retry button when the workflow graph can't be loaded. The code is clean and follows React Query conventions. One small error handling gap on the retry button needs a Blocking issues(None — no critical issues) Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage. |
Address review on coleam00#1698: - Retry button now calls queryClient.resetQueries() instead of invalidateQueries(). resetQueries clears the cached error state before refetching, which is the idiomatic React Query pattern for a user-triggered retry. - Wrap the returned Promise in .catch() so a queryClient failure is surfaced via console.error rather than silently swallowed. - Add a brief comment above the final 'unavailable' fallback so future maintainers don't conflate it with the pending branch.
Suggested fixes
Minor / nice-to-have
Verified |
2 participants
Summary
WorkflowExecutioncallsuseQueryfor the workflow definition but only destructuresdata; when the fetch rejects, the right-pane graph stays on "Loading graph..." forever (issue Web UI: workflow execution detail page hangs on 'Loading graph...' with no error state #1683).useQuerynow also surfaceserrorandisPending. The fallback splits into three branches: error message, spinner, empty-state copy.WorkflowDagViewer), the run query, the right-rail metadata, the API shape, the query key, theenabledgate.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
Label Snapshot
risk: lowsize: XSwebweb:workflows-executionChange Metadata
bugwebLinked Issue
Validation Evidence (required)
Component tests are not currently in scope of
packages/web's test runner (bun test src/lib/ && bun test src/stores/ && bun test src/hooks/), so no.test.tsxwas added. Verified through the manual scenarios below.Security Impact (required)
Compatibility / Migration
Human Verification (required)
<WorkflowDagViewer />branch.)getWorkflowcall still shows "Loading graph..."getWorkflowreturned no nodes), the panel reads "Workflow graph unavailable for this run." instead of spinning indefinitely.Errorrejections (e.g. plain strings) are stringified viaString(workflowDefError)to avoid[object Object].enabled: !!initialData?.workflowNamestill gates the query, so no spurious spinner before the run query resolves.fetchJSONinpackages/web/src/lib/api.tsalready produces (it throwsErrorwith the response body, captured by React Query).Side Effects / Blast Radius (required)
Rollback Plan (required)
const { data: workflowDef }destructure plus single-branch fallback returns.workflow.nodeswas unexpectedly nullish.Risks and Mitigations
isPendingbecomes false before data arrives (React Query v5 timing edge).isPendingdocumentation guaranteesisPending === trueuntil eitherdataorerroris populated for an enabled query. The fallback order (nodes → error → pending → empty) means even if the empty branch were hit transiently, it would resolve to nodes on the very next render. No flicker risk in practice.Summary by CodeRabbit