fix(ui): render forward-referenced DAG dependencies in Graph Viewer#16295
fix(ui): render forward-referenced DAG dependencies in Graph Viewer#16295myzk-a wants to merge 3 commits into
Conversation
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml 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 DAG edge-building logic now resolves dependency task genre from ChangesForward-reference dependency fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/src/shared/components/editors/graph-viewer.tsx (1)
142-146: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: precompute a task-name lookup map to avoid repeated
find.
template.dag.tasks.find(...)runs for every dependency of every task (O(n·m)). Building aMap<string, DAGTask>once per DAG template (e.g., alongsidetemplateMap/templateLeafMapsetup) would avoid the repeated linear scans. Given typical DAG task counts this is low-impact, but worth considering for larger workflows.🤖 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 `@ui/src/shared/components/editors/graph-viewer.tsx` around lines 142 - 146, The dependency lookup in the graph viewer is doing a repeated linear search for every dependency via template.dag.tasks.find, which can become expensive for larger DAGs. Build and reuse a Map<string, DAGTask> for each DAG template during the existing template setup (near templateMap/templateLeafMap initialization), then update the dependency handling in the graph-viewer logic that uses dependencies.forEach to read depTask from the map instead of scanning the array.
🤖 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 `@ui/src/shared/components/editors/graph-viewer.tsx`:
- Around line 142-146: The dependency lookup in the graph viewer is doing a
repeated linear search for every dependency via template.dag.tasks.find, which
can become expensive for larger DAGs. Build and reuse a Map<string, DAGTask> for
each DAG template during the existing template setup (near
templateMap/templateLeafMap initialization), then update the dependency handling
in the graph-viewer logic that uses dependencies.forEach to read depTask from
the map instead of scanning the array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a8097c6-995c-4feb-9463-6dbf9af39c13
📒 Files selected for processing (2)
ui/src/shared/components/editors/graph-viewer.test.tsxui/src/shared/components/editors/graph-viewer.tsx
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Supersedes #16119
Motivation
Opening the Graph tab for a Workflow / WorkflowTemplate / ClusterWorkflowTemplate / CronWorkflow whose DAG contains a forward reference crashes the UI with:
A forward reference is a DAG task that
dependson a sibling task defined later in thetasksarray. Argo's controller does not require tasks to be declared in topological order, so these workflows run perfectly fine — but the Graph tab is unusable for them.populateGraphFromWorkflowbuilds the graph in a single pass: for each task it creates the node and then immediately resolves that task's dependencies. When a task references a dependency that has not been created yet (forward reference, or a name that does not match a sibling task),graph.nodes.get(dependancyName)returnsundefinedand reading.genreon it throws.Reproduction
Save this template and open its Graph tab → the view crashes with
reading 'genre'.Modifications
populateGraphFromWorkflow(ui/src/shared/components/editors/graph-viewer.tsx), resolve the dependency's genre from the template model (template.dag.tasks+getTaskGenre) instead of from the partially-built graph.createNodestores for that task.build → deploy) is created.Why not the guard approach (#16119)?
#16119 guards the
undefinedlookup andreturns early, which stops the crash but drops the forward-reference edge, leaving an orphan node (the dependent task has no incoming edge). Resolving the genre from the model fixes the crash and renders the edge correctly, so it is strictly better for this case.Verification
cd ui && yarn jest graph-viewer→ 25 passed (includes the new forward-reference edge test).cd ui && yarn lint(eslint + tsc) → clean.main(reading 'genre'throw) and on fix(ui): guard missing DAG dependency nodes in GraphViewer #16119's guard approach (missingbuild → deployedge), and green with this change.make start UI=true): applied the reproductionWorkflowTemplateabove, opened its Graph tab. Before the fix the tab crashed withCannot read properties of undefined (reading 'genre'); after the fix the graph renders and thebuild → deployedge is drawn correctly (screenshot below).Graph tab after the fix (the

build → deployedge is rendered):Documentation
Not needed — this is a bug fix for existing behavior with no user-facing API or configuration change.
AI
Generative AI (Claude Code) was used as a pair-programming assistant to investigate the root cause, weigh fix approaches, and draft this PR description and the commit message. The code change and the test were authored and reviewed by me.
Summary by CodeRabbit