feat(cmd): add tree-style detailed result#1545
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughReplaces table-based DAG status display with a tree renderer, adds log-tail reading utilities, makes agent summary/status terminal-aware with colored output, updates agent progress UI to wait for a keypress before exiting, and adjusts agent manager types. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (status / agent)
participant Term as TerminalDetector
participant Renderer as TreeRenderer
participant FS as LogReader
participant Std as Stdout/Stderr
User->>CLI: request status/summary
CLI->>Term: is terminal? (stdout/stderr)
Term-->>CLI: yes/no
CLI->>Renderer: NewRenderer(DefaultConfig(color based on terminal))
activate Renderer
Renderer->>FS: read tails for stdout/stderr files
FS-->>Renderer: cleaned lines / markers (binary / too-large / truncated)
Renderer->>Renderer: build tree (header, nodes, durations, outputs, errors)
Renderer-->>Std: write formatted output (with color if enabled)
deactivate Renderer
Std-->>User: display tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/output/tree_test.go (1)
352-400: Consider: Unchecked file close errors flagged by linter.Static analysis flags unchecked
Close()andRemove()error returns. While these are acceptable in test code, you can silence the linter with explicit ignores or use a helper:🔎 Optional fix to satisfy linter
- defer os.Remove(stdoutFile.Name()) + defer func() { _ = os.Remove(stdoutFile.Name()) }() _, _ = stdoutFile.WriteString("Hello from stdout\nLine 2\n") - stdoutFile.Close() + _ = stdoutFile.Close()This pattern applies to other similar locations (lines 367, 369, 409, 413, 539, 567, 597, 1007).
internal/output/tree.go (1)
295-313: Consider caching log content to avoid redundant file reads.
hasOutputreads log files to check for content, and thenrenderOutputs/renderOutputread them again. For a CLI tool with bounded file sizes, this is acceptable, but caching the content on first read would improve efficiency.🔎 Potential optimization approach
Consider reading log content once in
renderStepand passing it to child methods, or caching in theRendererstruct during a singleRenderDAGStatuscall. This would avoid reading each log file up to 3 times per node.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/cmd/context.gointernal/cmd/status.gointernal/output/colors.gointernal/output/log_reader.gointernal/output/tree.gointernal/output/tree_test.gointernal/runtime/agent/agent.gointernal/runtime/agent/progress_tea.gointernal/runtime/agent/progress_tea_test.gointernal/runtime/agent/reporter.go
💤 Files with no reviewable changes (1)
- internal/runtime/agent/reporter.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/cmd/context.gointernal/output/colors.gointernal/runtime/agent/progress_tea.gointernal/cmd/status.gointernal/output/log_reader.gointernal/runtime/agent/agent.gointernal/output/tree_test.gointernal/runtime/agent/progress_tea_test.gointernal/output/tree.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/output/tree_test.gointernal/runtime/agent/progress_tea_test.go
🧠 Learnings (5)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/cmd/context.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*.go : Repository linting relies on `golangci-lint`; prefer idiomatic Go patterns, minimal global state, and structured logging helpers in `internal/common`
Applied to files:
internal/cmd/context.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/output/tree_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/output/tree_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/output/tree_test.go
🧬 Code graph analysis (5)
internal/cmd/context.go (2)
internal/common/logger/tag/tag.go (1)
Name(271-273)internal/common/logger/logger.go (1)
WithQuiet(76-80)
internal/cmd/status.go (2)
internal/core/execution/runstatus.go (1)
DAGRunStatus(35-59)internal/output/tree.go (2)
DefaultConfig(50-58)NewRenderer(84-86)
internal/runtime/agent/agent.go (1)
internal/output/tree.go (2)
DefaultConfig(50-58)NewRenderer(84-86)
internal/output/tree_test.go (4)
internal/core/status.go (13)
Succeeded(11-11)NodeSucceeded(57-57)Failed(9-9)NodeFailed(55-55)Running(8-8)Aborted(10-10)NodeAborted(56-56)PartiallySucceeded(13-13)NodePartiallySucceeded(59-59)Queued(12-12)NotStarted(7-7)NodeNotStarted(53-53)NodeSkipped(58-58)internal/output/tree.go (3)
DefaultConfig(50-58)NewRenderer(84-86)DefaultMaxOutputLines(23-23)internal/output/log_reader.go (1)
ReadLogFileTail(28-67)internal/output/colors.go (1)
StatusText(12-17)
internal/runtime/agent/progress_tea_test.go (1)
internal/runtime/agent/progress_tea.go (2)
FinalizeMsg(41-41)ProgressModel(45-74)
🪛 GitHub Check: Go Linter
internal/output/tree_test.go
[failure] 1007-1007:
Error return value of stdoutFile.Close is not checked (errcheck)
[failure] 597-597:
Error return value of tmpfile.Close is not checked (errcheck)
[failure] 567-567:
Error return value of tmpfile.Close is not checked (errcheck)
[failure] 539-539:
Error return value of tmpfile.Close is not checked (errcheck)
[failure] 413-413:
Error return value of stdoutFile.Close is not checked (errcheck)
[failure] 409-409:
Error return value of os.Remove is not checked (errcheck)
[failure] 369-369:
Error return value of stderrFile.Close is not checked (errcheck)
[failure] 367-367:
Error return value of os.Remove is not checked (errcheck)
[failure] 361-361:
Error return value of stdoutFile.Close is not checked (errcheck)
[failure] 359-359:
Error return value of os.Remove is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (31)
internal/runtime/agent/progress_tea_test.go (1)
118-124: LGTM!The test correctly verifies the new
waitingForKeyflow. After receivingFinalizeMsg, the model should setwaitingForKey=trueand return anilcommand (instead oftea.Quit), deferring termination until the user presses a key. This aligns with the implementation changes inprogress_tea.go.internal/cmd/context.go (2)
16-17: LGTM!The
golang.org/x/termimport is correctly added to support terminal detection for the new conditional logging behavior.
216-225: Good helper function with clear intent.The function correctly identifies agent commands that display progress or tree output. Consider adding
"exec"if it also uses similar display features, per the note above.internal/output/colors.go (2)
1-17: LGTM!Clean utility function for human-readable status text. The package documentation and function signature are clear.
19-34: No action needed. ThetoTitleCasefunction correctly handles the lowercase strings provided bystatus.String()(e.g.,"partially_succeeded"becomes"Partially Succeeded"). The theoretical concern about uppercase input is not applicable here sincestatus.String()always returns lowercase.internal/cmd/status.go (2)
78-79: LGTM!Clean transition to the new tree-structured status rendering.
84-92: LGTM!The new
displayTreeStatusfunction correctly:
- Uses
DefaultConfig()as a baseline- Enables color only when stdout is a terminal
- Creates a renderer and outputs the tree
This is consistent with the rendering approach used in
agent.go.internal/runtime/agent/progress_tea.go (5)
62-73: LGTM!The new
waitingForKeyfield properly controls the finalization flow, and the explicit style field types improve readability.
169-173: LGTM!The key handling correctly prioritizes the
waitingForKeystate, allowing any key to exit when waiting for user acknowledgment.
567-581: LGTM!The footer rendering provides clear user feedback when waiting for a key press, with status-appropriate styling for success, failure, and abort states.
796-804: LGTM!The
Stop()method correctly waits for the program to finish via the done channel, then exits the alternate screen buffer and restores the cursor. This ensures the tree summary can be displayed after the progress UI.
157-162: The progress display with key-waiting behavior is properly gated to interactive terminal environments. TheshouldEnableProgress()function explicitly checksisTerminal(os.Stderr)usinggolang.org/x/term.IsTerminal(), which detects whether stderr is connected to a terminal. In non-interactive contexts (CI/CD), this check returns false, so the progress display is never initialized and the key-waiting code never executes. No changes needed—the code already handles non-interactive environments correctly.Likely an incorrect or invalid review comment.
internal/output/tree_test.go (4)
13-49: LGTM!Comprehensive test for basic success rendering. The test verifies key elements including DAG name, step name, status labels, and result line.
715-738: LGTM!Good table-driven test for
StatusTextcovering all status types including the snake_case to Title Case conversion forPartiallySucceeded.
954-977: LGTM!Thorough coverage of
nodeStatusToStatusmapping including the unknown status case. This ensures robustness when encountering unexpected node statuses.
1235-1298: LGTM!Excellent edge case coverage for
cleanLogLinehandling carriage returns, including real-world scenarios like curl progress bars. This ensures clean log output in the tree renderer.internal/runtime/agent/agent.go (3)
19-21: LGTM!Clean import organization. The otel imports support tracing, and the new
outputpackage enables tree-structured rendering.Also applies to: 36-36
75-76: LGTM!Removing the
runtime1alias simplifies the code. The field type is now directlyruntime.Manager.
755-769: Good refactor to tree-based summary rendering.The implementation correctly:
- Fetches current status
- Creates a minimal DAG with just the name (sufficient for the renderer)
- Configures color based on terminal detection
- Renders and prints the tree
Minor note: Using
println(summary)(builtin) vsfmt.Println(summary)is slightly inconsistent with the rest of the codebase, but functionally equivalent here.internal/output/log_reader.go (5)
1-67: Well-structured log reading utility with good safety guards.The entry point
ReadLogFileTailhandles edge cases well: empty paths, non-existent files, and file size limits. The#nosecannotation for G304 is appropriate given the trusted source context.
69-122: Ring buffer implementation is correct and memory-efficient.The logic properly handles:
- Modular indexing for circular buffer behavior
- Correct extraction order when buffer wraps
- Accurate truncation count calculation
One observation:
cleanLogLinecan return multiple lines (when splitting on\r), and each cleaned line is counted individually towardtotalLines. This behavior is intentional and correctly reflects the actual line count shown to users.
160-187: Good handling of carriage returns for progress bar output.The approach of keeping only the final segment after
\ris appropriate for CLI tools likecurlthat use carriage returns to update progress bars in-place.
189-202: Efficient control character cleaning.Pre-allocating with
Grow(len(s))is a good optimization. The logic correctly preserves printable characters and tabs while removing control characters.
204-229: Reasonable binary detection heuristics.The two-pronged approach (null bytes + high non-printable ratio) is effective. Note that this only checks the first line, which is sufficient for early detection without reading the entire file.
internal/output/tree.go (7)
21-58: Well-designed configuration with sensible defaults.The config options provide good flexibility for different output contexts (terminal vs. file, with/without colors).
65-81: Color helper methods are correctly implemented.Proper guards for
ColorEnabledand use of ANSI 256 color codes with reset sequences.
130-142: Header rendering with appropriate fallback.The fallback to
time.Now()for missing start times is reasonable for display purposes.
155-284: Comprehensive step rendering with proper tree structure handling.The logic correctly handles various step states, command formats, and child elements. The field-counting approach for proper tree continuation is well thought out.
367-395: Word wrapping implementation is straightforward and functional.Note that single words exceeding
maxWidthwon't be broken mid-word, which is acceptable behavior for readability.
598-622: Duration calculation handles edge cases well.Properly differentiates between running (uses current time) and completed (uses finish time) statuses, with appropriate fallbacks.
630-646: Status conversion mapping is complete for duration calculation needs.The default case safely handles any unmapped statuses by returning
NotStarted.
| for i, line := range wrapped { | ||
| if i == 0 { | ||
| buf.WriteString(prefix + TreeLastBranch + r.text(line) + "\n") | ||
| } else { | ||
| // 4 spaces to match width of "└─" | ||
| buf.WriteString(prefix + " " + r.text(line) + "\n") | ||
| } | ||
| } | ||
| return buf.String() | ||
| } |
There was a problem hiding this comment.
Potential alignment mismatch in wrapped error lines.
The continuation indent uses 4 spaces (" "), but TreeLastBranch ("└─") has a visual width of 2 characters (though 6 bytes in UTF-8). This may cause wrapped lines to be over-indented compared to the first line.
🔎 Suggested fix
for i, line := range wrapped {
if i == 0 {
buf.WriteString(prefix + TreeLastBranch + r.text(line) + "\n")
} else {
- // 4 spaces to match width of "└─"
- buf.WriteString(prefix + " " + r.text(line) + "\n")
+ // 2 spaces to match visual width of "└─"
+ buf.WriteString(prefix + " " + r.text(line) + "\n")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, line := range wrapped { | |
| if i == 0 { | |
| buf.WriteString(prefix + TreeLastBranch + r.text(line) + "\n") | |
| } else { | |
| // 4 spaces to match width of "└─" | |
| buf.WriteString(prefix + " " + r.text(line) + "\n") | |
| } | |
| } | |
| return buf.String() | |
| } | |
| for i, line := range wrapped { | |
| if i == 0 { | |
| buf.WriteString(prefix + TreeLastBranch + r.text(line) + "\n") | |
| } else { | |
| // 2 spaces to match visual width of "└─" | |
| buf.WriteString(prefix + " " + r.text(line) + "\n") | |
| } | |
| } | |
| return buf.String() |
🤖 Prompt for AI Agents
internal/output/tree.go around lines 564 to 573: the continuation indent is
hard-coded to four spaces which over-indents compared to the visual width of
TreeLastBranch ("└─"); replace the literal " " with a computed repeat of
spaces matching the rune count of TreeLastBranch (e.g. strings.Repeat(" ",
len([]rune(TreeLastBranch)))) so wrapped lines align with the first line, and
add an import for "strings" if not already present.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/output/tree.go (1)
564-573: Alignment mismatch in wrapped error lines (previously flagged).The continuation indent uses 4 spaces (line 569), but
TreeLastBranch("└─") has a visual width of 2 characters. This causes wrapped error lines to be over-indented compared to the first line.This issue was already identified in a previous review.
🔎 Suggested fix
for i, line := range wrapped { if i == 0 { buf.WriteString(prefix + TreeLastBranch + r.text(line) + "\n") } else { - // 4 spaces to match width of "└─" - buf.WriteString(prefix + " " + r.text(line) + "\n") + // 2 spaces to match visual width of "└─" + buf.WriteString(prefix + " " + r.text(line) + "\n") } }
🧹 Nitpick comments (2)
internal/output/tree_test.go (1)
13-400: Consider using testify/require for assertions.The tests currently use
t.Errorwithstrings.Containschecks. Per the coding guidelines, tests should usestretchr/testify/require. This improves test readability and halts execution on first failure, preventing cascading errors.Example refactor for TestRenderDAGStatus_BasicSuccess
+import ( + "github.com/stretchr/testify/require" +) + func TestRenderDAGStatus_BasicSuccess(t *testing.T) { t.Parallel() // ... setup ... - if !strings.Contains(output, "test-dag") { - t.Error("Output should contain DAG name") - } + require.Contains(t, output, "test-dag", "Output should contain DAG name")Apply similar changes to other test functions for consistency with project guidelines.
Based on coding guidelines, learnings.
internal/output/tree.go (1)
295-313: Minor inefficiency: reading log files twice.The
hasOutput()method reads log files to check for content (line 301, 307), and thenrenderOutputs()reads them again. While the impact is minimal sincehasOutput()only reads 1 line, this pattern could be optimized by caching the check result or restructuring the logic.This is acceptable for the current implementation but could be optimized in the future if performance becomes a concern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/output/tree.gointernal/output/tree_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/output/tree_test.gointernal/output/tree.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/output/tree_test.go
🧠 Learnings (3)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/output/tree_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/output/tree_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/output/tree_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (9)
internal/output/tree_test.go (2)
526-713: LGTM! Comprehensive log reading tests.The log tail reading tests provide excellent coverage of edge cases including empty files, nonexistent files, binary content detection, and large file handling. The use of temporary files with proper cleanup is well done.
715-1393: Excellent table-driven tests for helper functions.These tests for utility functions (StatusText, binary detection, line cleaning, error message cleaning) properly use table-driven approaches with comprehensive edge cases. The carriage return handling tests are particularly thorough in testing real-world scenarios like curl progress output.
internal/output/tree.go (7)
1-27: LGTM! Well-defined constants and imports.The package structure, imports, and tree-drawing constants are well-organized. The Unicode box-drawing characters provide clear visual structure, and the default values for output lines and width are reasonable.
28-86: LGTM! Well-structured configuration and renderer.The Config struct provides good control over rendering behavior, and DefaultConfig returns sensible defaults. The color methods properly check ColorEnabled before applying ANSI escape codes, which is important for non-terminal output.
88-153: LGTM! Clean main rendering flow.The main rendering methods follow a logical structure: header → DAG line → tree continuation → steps → final status. The fallback to current time for missing start times is a sensible defensive practice.
397-517: LGTM! Comprehensive output rendering logic.The legacy command handling properly supports both historical formats (CmdWithArgs and Command+Args). The output rendering logic is well-designed with proper handling of truncation, inline vs multi-line display, and wrapping. Skipping empty outputs entirely (lines 447-449) keeps the tree clean.
519-540: LGTM! Clear sub-DAG run rendering.The sub-run rendering properly displays DAGRunID and optional parameters, with appropriate tree branch characters for visual structure.
575-586: LGTM! Proper error message cleaning.The
cleanErrorMessagefunction appropriately removes the "recent stderr (tail):" section since stderr is already displayed separately in the tree structure, avoiding duplication.
588-649: LGTM! Robust duration calculation with proper edge case handling.The duration calculation logic properly handles various edge cases: empty/dash times, invalid formats, and running vs completed states. The fallback to
time.Now()for invalid finish times and running statuses provides sensible behavior. ThenodeStatusToStatusmapping is complete and includes all node status types.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1545 +/- ##
==========================================
+ Coverage 63.22% 63.32% +0.09%
==========================================
Files 212 215 +3
Lines 24167 24410 +243
==========================================
+ Hits 15279 15457 +178
- Misses 7408 7463 +55
- Partials 1480 1490 +10
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Behavior Changes
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.