Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive DAG run outputs collection throughout the system: a new API endpoint retrieves outputs with metadata, steps now support structured output configuration (string or object form), outputs are collected during execution with secret masking and camelCase conversion, persisted to disk, and displayed in a new UI component with filtering and sorting capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Step as Step Execution
participant Agent as Agent Runtime
participant Outputs as Output Collector
participant Masker as Secret Masker
participant Store as File Store
participant API as API Handler
Note over Step,Store: DAG Run Execution Phase
Step->>Agent: Execute step, capture OUTPUT vars
Agent->>Outputs: collectOutputs(plan)
loop For each step in execution plan
Outputs->>Outputs: Parse KEY=VALUE from captured vars
Outputs->>Outputs: Apply OutputOmit filter & custom OutputKey
Outputs->>Outputs: Convert UPPER_CASE to camelCase (default)
Outputs->>Outputs: Accumulate to outputs map (last-wins)
end
Outputs->>Agent: return collected outputs map
Agent->>Agent: buildOutputs(metadata, outputs)
Agent->>Masker: Mask secrets in outputs
Masker->>Store: WriteOutputs(outputs.json)
Store->>Store: Persist DAGRunOutputs with metadata
Note over Step,Store: Execution complete
Note over API,Store: Output Retrieval Phase
API->>Store: ReadOutputs(dag-run-id)
Store->>Store: Parse outputs.json
Store->>API: return DAGRunOutputs
API->>API: Format metadata (status, timestamp, params)
API-->>Client: 200 with DAGRunOutputs payload
sequenceDiagram
participant UI as DAG Status UI
participant Hook as useQuery Hook
participant API as Frontend API
participant Handler as GetDAGRunOutputs
participant Store as File Store
UI->>Hook: Fetch outputs (dagName, dagRunId)
Hook->>API: GET /dag-runs/{name}/{dagRunId}/outputs
API->>Handler: Request GetDAGRunOutputs
Handler->>Store: ReadOutputs()
alt Outputs exist
Store-->>Handler: DAGRunOutputs
Handler->>Handler: Translate StatusLabel → internal Status
Handler->>Handler: Parse CompletedAt (RFC3339 → time.Time)
Handler-->>API: 200 GetDAGRunOutputs200JSONResponse
else No outputs
Store-->>Handler: nil
Handler->>Handler: Initialize empty Outputs map
Handler-->>API: 200 with empty Outputs & metadata
else Error
Handler-->>API: 404 or error response
end
API-->>Hook: Response
Hook->>UI: Render outputs table with filter/sort
UI->>UI: Display metadata header, filtered outputs
UI-->>User: Interactive output view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-12-04T10:34:17.062ZApplied to files:
📚 Learning: 2025-12-04T10:34:17.062ZApplied to files:
⏰ 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)
🔇 Additional comments (21)
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 |
yottahmd
force-pushed
the
1466-collect-outputs
branch
from
97bccd4 to
967b558
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
internal/core/spec/step.go (1)
53-54: Output config parsing and key/omit wiring look correct; consider avoiding triple parsingThe new
outputobject form (name/key/omit) and its propagation intoOutput,OutputKey, andOutputOmitoncore.Stepare consistent with howcollectOutputsconsumes them later. The$-stripping and empty-string handling for both string and object forms are also sensible.The only nit is that
parseOutputConfigis called three times per step (once in each builder) andbuildStepOutputKey/buildStepOutputOmitrely onbuildStepOutputhaving already reported parse errors. If someone ever reordersstepTransformersor reuses these builders independently, parse errors could get silently dropped.If you want to harden this without changing behavior, you could either:
- cache the parsed
outputConfigon the specstepduringparseOutputConfig, or- have
buildStepOutputKey/buildStepOutputOmitsurface their own errors and let higher-level code deduplicate messages if needed.Not urgent, but worth keeping in mind if this area evolves.
Also applies to: 175-176, 599-650, 651-660, 662-671, 673-682
internal/persistence/filedagrun/attempt_test.go (1)
485-590: Outputs read/write tests are thorough; consider adding an old‑format compatibility caseThese tests give solid coverage of the new persistence behavior (happy path, empty/nil, overwrite, non-existent file, corruption, special characters) and use the existing helpers cleanly. This should give good confidence around
outputs.jsonhandling.Given
ReadOutputshas explicit logic to treat “old format (no metadata)” asnil, you might optionally add a small subtest that writes a legacy-style JSON payload withoutmetadata(or withoutdagRunId) and asserts thatReadOutputsreturns(nil, nil). That would lock in the backward‑compat guarantee documented in the implementation.Based on learnings, this nicely exercises failure paths; the extra case would just formalize the compatibility story.
Also applies to: 592-677
internal/runtime/agent/agent.go (1)
555-560: Outputs aggregation/writing pipeline is sound; a couple of minor polish ideasThe overall flow here looks good:
- You aggregate per-step outputs only after the run finishes, respect
OutputOmit, and derive the public key from eitherOutputKeyorScreamingSnakeToCamel(step.Output), which matches the documented behavior.- Non-string
OutputVariablesare safely skipped with a warning, and you warn (rather than fail) when the total payload exceeds ~1MB.- Metadata in
buildOutputs(DAG name, run ID, attempt ID, status, completed timestamp, JSON-serialized params) is complete and matches the new types inexecution/outputs.go.Two small, non-blocking tweaks you might consider:
Use the plan’s finished timestamp for
CompletedAt
Instead oftime.Now(), wiringCompletedAtfrom the same source asfinishedStatus.FinishedAt(e.g., viaa.plan.FinishAt()or by passing the fullDAGRunStatusintobuildOutputs) would keep outputs metadata and status perfectly in sync.Log params JSON marshal failures
InbuildOutputs, ifjson.Marshal(a.dag.Params)fails, you currently just drop params silently. Logging atWarnlevel once would make this diagnosable without changing the API surface.Neither change is required for correctness; the current implementation is safe and matches the API/types contract.
Also applies to: 608-676, 681-706
internal/persistence/filedagrun/attempt.go (1)
38-40: Outputs persistence behavior is correct; optional robustness improvementsThe
OutputsFileplacement alongside the status file and theWriteOutputs/ReadOutputssemantics match how the agent and API consume outputs:
- Skipping writes for
nilor emptyOutputsis consistent with treating “no outputs captured” as a non-existent file.- Pretty-printing JSON and using
0600is reasonable for a human-inspectable artifact.ReadOutputssensibly returns(nil, nil)when the file is missing and also whenmetadata.dagRunIdis empty, which gives you a clean hook for ignoring any legacy or malformed payloads without breaking callers.If you want to tighten this further (not required):
- You could ensure the directory exists in
WriteOutputsviaos.MkdirAll(filepath.Dir(att.file), 0750)to decouple it completely fromOpen, making the method safe even if used in isolation.- For extra safety on partial writes, you could mirror the status file’s
safeRenameflow (write to a temp file and atomically replaceoutputs.json), though for a non-critical auxiliary file that may be overkill.Current behavior is fine as-is.
Also applies to: 531-578
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
api/v2/api.gen.goapi/v2/api.yamlinternal/common/stringutil/stringutil.gointernal/common/stringutil/stringutil_test.gointernal/core/execution/dagrun.gointernal/core/execution/dagrun_test.gointernal/core/execution/outputs.gointernal/core/spec/step.gointernal/core/spec/step_test.gointernal/core/step.gointernal/integration/outputs_collection_test.gointernal/persistence/filedagrun/attempt.gointernal/persistence/filedagrun/attempt_test.gointernal/runtime/agent/agent.gointernal/runtime/agent/dbclient_test.gointernal/service/frontend/api/v2/dagruns.gointernal/service/scheduler/zombie_detector_test.goui/src/api/v2/schema.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/common/stringutil/stringutil_test.gointernal/runtime/agent/dbclient_test.gointernal/persistence/filedagrun/attempt_test.gointernal/core/spec/step_test.gointernal/common/stringutil/stringutil.gointernal/service/frontend/api/v2/dagruns.gointernal/service/scheduler/zombie_detector_test.gointernal/core/execution/outputs.gointernal/core/step.gointernal/core/execution/dagrun_test.gointernal/core/execution/dagrun.gointernal/persistence/filedagrun/attempt.gointernal/core/spec/step.gointernal/integration/outputs_collection_test.gointernal/runtime/agent/agent.goapi/v2/api.gen.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/common/stringutil/stringutil_test.gointernal/runtime/agent/dbclient_test.gointernal/persistence/filedagrun/attempt_test.gointernal/core/spec/step_test.gointernal/service/scheduler/zombie_detector_test.gointernal/core/execution/dagrun_test.gointernal/integration/outputs_collection_test.go
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/api/v2/schema.ts
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/api/v2/schema.ts
🧠 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/common/stringutil/stringutil_test.gointernal/persistence/filedagrun/attempt_test.gointernal/core/spec/step_test.gointernal/integration/outputs_collection_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/common/stringutil/stringutil_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/core/spec/step_test.gointernal/integration/outputs_collection_test.go
🧬 Code graph analysis (12)
internal/common/stringutil/stringutil_test.go (1)
internal/common/stringutil/stringutil.go (1)
ScreamingSnakeToCamel(73-101)
internal/runtime/agent/dbclient_test.go (3)
internal/core/execution/context.go (1)
Context(16-27)api/v2/api.gen.go (1)
Error(545-554)api/v1/api.gen.go (1)
Error(343-352)
internal/persistence/filedagrun/attempt_test.go (2)
internal/persistence/filedagrun/attempt.go (1)
OutputsFile(39-39)internal/core/execution/outputs.go (2)
DAGRunOutputs(4-7)OutputsMetadata(10-17)
internal/service/frontend/api/v2/dagruns.go (4)
api/v2/api.gen.go (9)
GetDAGRunOutputsRequestObject(5914-5918)GetDAGRunOutputsResponseObject(5920-5922)GetDAGRunOutputs404JSONResponse(5933-5933)Error(545-554)DAGRunOutputs(487-493)OutputsMetadata(719-737)GetDAGRunOutputs200JSONResponse(5924-5924)DAGName(414-414)Status(951-951)internal/core/execution/dagrun.go (1)
NewDAGRunRef(174-179)internal/common/logger/context.go (2)
Error(50-52)Errorf(75-77)internal/core/execution/outputs.go (2)
DAGRunOutputs(4-7)OutputsMetadata(10-17)
internal/service/scheduler/zombie_detector_test.go (2)
api/v2/api.gen.go (1)
DAGRunOutputs(487-493)internal/core/execution/outputs.go (1)
DAGRunOutputs(4-7)
internal/core/execution/outputs.go (1)
api/v2/api.gen.go (3)
OutputsMetadata(719-737)DAGName(414-414)Status(951-951)
internal/core/execution/dagrun.go (3)
internal/core/execution/context.go (1)
Context(16-27)api/v2/api.gen.go (1)
DAGRunOutputs(487-493)internal/core/execution/outputs.go (1)
DAGRunOutputs(4-7)
internal/persistence/filedagrun/attempt.go (2)
api/v2/api.gen.go (1)
DAGRunOutputs(487-493)internal/core/execution/outputs.go (1)
DAGRunOutputs(4-7)
internal/core/spec/step.go (1)
internal/core/spec/builder.go (1)
StepBuildContext(25-28)
internal/integration/outputs_collection_test.go (4)
internal/core/execution/runstatus.go (1)
DAGRunStatus(35-58)internal/core/status.go (3)
Succeeded(11-11)NodeSucceeded(57-57)Failed(9-9)internal/core/execution/outputs.go (1)
DAGRunOutputs(4-7)internal/persistence/filedagrun/attempt.go (1)
OutputsFile(39-39)
internal/runtime/agent/agent.go (5)
internal/common/logger/context.go (2)
Error(50-52)Warn(45-47)internal/core/execution/context.go (1)
Context(16-27)internal/runtime/data.go (1)
NodeData(23-26)internal/common/stringutil/stringutil.go (2)
ScreamingSnakeToCamel(73-101)FormatTime(18-24)internal/core/execution/outputs.go (2)
DAGRunOutputs(4-7)OutputsMetadata(10-17)
api/v2/api.gen.go (2)
internal/core/execution/outputs.go (2)
OutputsMetadata(10-17)DAGRunOutputs(4-7)internal/common/config/config.go (1)
RemoteNode(222-231)
⏰ 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 (17)
internal/core/execution/dagrun.go (1)
153-158: LGTM!The interface additions are well-designed with clear documentation. The nil/empty handling semantics for
WriteOutputsand the graceful degradation for legacy v1 formats inReadOutputsare appropriately documented.internal/core/spec/step_test.go (1)
2839-3094: LGTM!The tests are well-structured following table-driven patterns, cover comprehensive edge cases (nil, empty, string forms, object forms with various field combinations, and error scenarios), and properly use
t.Parallel()andstretchr/testify/require. As per coding guidelines, the tests co-locate with the production code and cover failure paths.internal/common/stringutil/stringutil.go (1)
71-101: LGTM!The implementation correctly handles all edge cases including leading/trailing underscores, consecutive underscores, and mixed-case input. The use of
strings.Builderis idiomatic and efficient for string concatenation in Go.internal/common/stringutil/stringutil_test.go (1)
144-169: LGTM!The test follows table-driven patterns, uses
stretchr/testify/requireas per coding guidelines, and provides comprehensive coverage including edge cases for empty strings, underscores at various positions, and mixed-case inputs.internal/core/step.go (1)
59-63: LGTM!The new fields are well-documented and appropriately placed near the related
Outputfield. The JSON tags follow the existing naming conventions in the struct.internal/core/execution/outputs.go (1)
1-17: LGTM!The new types provide a clean data contract for DAG run outputs. Using
stringforCompletedAtandStatusis pragmatic for file-based persistence, allowing flexibility in formatting. The API layer handles the conversion to typed representations (time.Time,StatusLabel) appropriately.internal/service/scheduler/zombie_detector_test.go (1)
529-540: Mock extensions correctly match DAGRunAttempt contractThe added
WriteOutputs/ReadOutputsmethods align with the interface and existing mock patterns, and the nil-check inReadOutputsavoids panics when tests stub a nil value. No changes needed.api/v2/api.yaml (1)
3131-3131: Type alias tightening for IDs/names looks goodSwitching
SubDAGRunDetail.dagRunIdand the variousRunningTaskDAG name/run ID fields over to the sharedDAGRunId/DAGNameschemas keeps the model consistent and enforces the same validation/patterns everywhere. No issues here.Also applies to: 3456-3470
internal/integration/outputs_collection_test.go (3)
18-252: Well-structured table-driven tests with good coverage.The test cases cover essential scenarios including simple outputs, custom keys, omit functionality, multiple steps, duplicate key handling, empty outputs, dollar-prefix variables, and mixed configurations. Good use of parallel execution and the test helper utilities.
255-288: Good failure scenario coverage.This test correctly validates that outputs from successful steps are preserved even when subsequent steps fail, and that outputs from steps that never executed are not collected.
290-327: Good coverage of camelCase conversion edge cases.The table includes useful edge cases like single words, multi-word names, and mixed case inputs.
ui/src/api/v2/schema.ts (3)
679-698: New outputs endpoint and schema types look correct.The
getDAGRunOutputspath definition and theDAGRunOutputs/OutputsMetadataschemas align with the backend types defined ininternal/core/execution/outputs.go. The 404 response for missing outputs is appropriate.Also applies to: 1247-1276
1309-1309: Good type safety improvements.Replacing raw
stringtypes with typed aliases (DAGRunId,DAGName) inSubDAGRunDetailandRunningTaskimproves type safety and consistency with the rest of the schema.Also applies to: 1470-1477
3876-3921: Operation definition is complete and correct.The
getDAGRunOutputsoperation includes proper path parameters, optionalremoteNodequery parameter, and appropriate response types (200, 404, default error).api/v2/api.gen.go (3)
486-493: DAGRunOutputs / OutputsMetadata model definitions look consistent and API-friendlyThe new
DAGRunOutputsandOutputsMetadatastructs are coherent with the runtime concept (metadata + key/value outputs), use existing aliases (DAGName,DAGRunId,StatusLabel), and follow the project's JSON/timestamp conventions used elsewhere in this file. I don't see any structural or tagging issues here.Also applies to: 718-737
867-883: Switch to DAGName/DAGRunId aliases improves semantic clarity without behavior changeUpdating
RunningTaskandSubDAGRunDetailfields to useDAGName/DAGRunId(and pointer variants) keeps the wire format identical while tightening semantics in code. This matches how other types in this file already model DAG identifiers. No issues from a correctness or compatibility standpoint.Also applies to: 1041-1042
9386-9598: Updated embedded swaggerSpec blob – treat as generated artifactThe large base64+gzip
swaggerSpecslice has been updated, presumably from regenerating viaoapi-codegenagainst the modified OpenAPI spec that includes the outputs endpoint and models. Given this is an opaque generated artifact, it shouldn’t be hand-edited; any tweaks should be made inapi.yamland re-generated. No further review needed here.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
internal/common/secrets/resolver_test.go (1)
224-225: Good addition—compile-time interface check.This assertion ensures
mockResolverimplements theResolverinterface at compile time, catching interface mismatches early during development.As per coding guidelines, consider using shared fixtures from
internal/testinstead of maintaining this local mock, though that's a broader refactoring beyond this change.internal/core/spec/step.go (1)
599-644: Consider stricter type validation for object fields.The type assertions for
key(line 629) andomit(line 632) silently ignore values with incorrect types. For example, if a user specifiesomit: "true"instead ofomit: true, orkey: 123instead ofkey: "mykey", these fields are silently ignored and defaults are used.While the lenient approach provides flexibility, it may hide user configuration errors. Consider returning validation errors when these fields are present but have incorrect types.
🔎 Example strict validation
case map[string]any: cfg := &outputConfig{} if name, ok := v["name"].(string); ok { cfg.Name = strings.TrimPrefix(strings.TrimSpace(name), "$") + } else if _, exists := v["name"]; exists { + return nil, fmt.Errorf("output.name must be a string, got %T", v["name"]) } - if key, ok := v["key"].(string); ok { + if key, ok := v["key"].(string); ok && key != "" { cfg.Key = strings.TrimSpace(key) + } else if _, exists := v["key"]; exists { + return nil, fmt.Errorf("output.key must be a string, got %T", v["key"]) } if omit, ok := v["omit"].(bool); ok { cfg.Omit = omit + } else if _, exists := v["omit"]; exists { + return nil, fmt.Errorf("output.omit must be a boolean, got %T", v["omit"]) }ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (1)
104-108: Consider adding error handling for clipboard operations.
navigator.clipboard.writeText()can fail (e.g., if clipboard permissions are denied). Consider wrapping in a try-catch to handle failures gracefully.🔎 Suggested improvement
const handleCopy = async (key: string, value: string) => { - await navigator.clipboard.writeText(value); - setCopiedKey(key); - setTimeout(() => setCopiedKey(null), 2000); + try { + await navigator.clipboard.writeText(value); + setCopiedKey(key); + setTimeout(() => setCopiedKey(null), 2000); + } catch { + // Clipboard write failed - could optionally show an error toast + } };api/v2/api.yaml (1)
3456-3475: RunningTask ID/name fields now correctly share DAGName/DAGRunId typesUsing
DAGRunId/DAGNamefordagRunId,dagName,rootDagRunName/Id, andparentDagRunName/Idimproves type consistency across the API and makes it easier for clients to treat these as proper DAG identifiers instead of arbitrary strings. Optional root/parent links also look appropriately modeled.If you later normalize the related fields in
DAGRunSummaryas well, consider reusing the same$refs there for full symmetry—but that can be deferred.ui/src/api/v2/schema.ts (1)
1247-1276: DAGRunOutputs / OutputsMetadata types align with the specThe
DAGRunOutputsandOutputsMetadatainterfaces accurately mirror the YAML schema (requiredmetadataandoutputs, strongly-typed identifiers, and the outputs map asRecord<string, string>). Any tweak to the descriptive text about “metadata fields may be empty strings” should be done inapi.yamland then re‑generated here.api/v2/api.gen.go (1)
486-493: Clarify metadata “empty strings” wording vs typesThe top‑level comment says metadata fields “may be empty strings”, but
OutputsMetadata.CompletedAtis atime.Time, so the unset case will serialize as a zero timestamp, not an empty string. If you want truly “empty/omitted” semantics when no outputs are present, consider adjusting the OpenAPI description (e.g., making timestamps/fields nullable or pointer types) so the generated docs match behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
api/v2/api.gen.goapi/v2/api.yamlinternal/cmd/dagpicker/dagpicker_test.gointernal/cmd/migrator_test.gointernal/common/backoff/retrypolicy_test.gointernal/common/secrets/resolver_test.gointernal/common/telemetry/collector_test.gointernal/core/spec/step.gointernal/integration/outputs_collection_test.gointernal/runtime/agent/dbclient_test.gointernal/runtime/agent/reporter_test.gointernal/service/coordinator/client_test.gointernal/service/frontend/auth/middleware_test.gointernal/service/scheduler/zombie_detector_test.gointernal/service/worker/poller_test.gointernal/service/worker/worker_test.goschemas/dag.schema.jsonui/src/api/v2/schema.tsui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsxui/src/features/dag-runs/components/dag-run-details/index.tsui/src/features/dag-runs/hooks/useHasOutputs.tsui/src/features/dags/components/dag-details/DAGDetailsContent.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/integration/outputs_collection_test.go
- internal/runtime/agent/dbclient_test.go
🧰 Additional context used
📓 Path-based instructions (5)
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dag-runs/components/dag-run-details/index.tsui/src/features/dag-runs/hooks/useHasOutputs.tsui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsxui/src/features/dags/components/dag-details/DAGDetailsContent.tsxui/src/api/v2/schema.ts
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dag-runs/components/dag-run-details/index.tsui/src/features/dag-runs/hooks/useHasOutputs.tsui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsxui/src/features/dags/components/dag-details/DAGDetailsContent.tsxui/src/api/v2/schema.ts
**/*.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/runtime/agent/reporter_test.gointernal/cmd/migrator_test.gointernal/common/telemetry/collector_test.gointernal/service/scheduler/zombie_detector_test.gointernal/service/frontend/auth/middleware_test.gointernal/service/coordinator/client_test.gointernal/service/worker/poller_test.gointernal/core/spec/step.gointernal/common/secrets/resolver_test.gointernal/service/worker/worker_test.gointernal/common/backoff/retrypolicy_test.goapi/v2/api.gen.gointernal/cmd/dagpicker/dagpicker_test.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/runtime/agent/reporter_test.gointernal/cmd/migrator_test.gointernal/common/telemetry/collector_test.gointernal/service/scheduler/zombie_detector_test.gointernal/service/frontend/auth/middleware_test.gointernal/service/coordinator/client_test.gointernal/service/worker/poller_test.gointernal/common/secrets/resolver_test.gointernal/service/worker/worker_test.gointernal/common/backoff/retrypolicy_test.gointernal/cmd/dagpicker/dagpicker_test.go
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
🧠 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 : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/runtime/agent/reporter_test.gointernal/cmd/migrator_test.gointernal/common/telemetry/collector_test.gointernal/service/frontend/auth/middleware_test.gointernal/service/coordinator/client_test.gointernal/service/worker/poller_test.gointernal/common/secrets/resolver_test.gointernal/service/worker/worker_test.gointernal/common/backoff/retrypolicy_test.gointernal/cmd/dagpicker/dagpicker_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/common/telemetry/collector_test.gointernal/common/backoff/retrypolicy_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 ui/**/*.{ts,tsx} : UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (`JobList.tsx`) and hooks with `use*` (`useJobs.ts`)
Applied to files:
ui/src/features/dags/components/dag-details/DAGDetailsContent.tsx
🧬 Code graph analysis (9)
internal/runtime/agent/reporter_test.go (1)
internal/runtime/agent/reporter.go (1)
Sender(27-29)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (5)
api/v2/api.gen.go (1)
StatusLabel(954-954)ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)ui/src/hooks/api.ts (1)
useQuery(28-28)ui/src/components/ui/input.tsx (1)
Input(21-21)ui/src/components/ui/table.tsx (6)
Table(126-126)TableHeader(132-132)TableRow(133-133)TableHead(131-131)TableBody(127-127)TableCell(129-129)
internal/service/scheduler/zombie_detector_test.go (2)
internal/core/execution/dagrun.go (1)
DAGRunAttempt(131-159)internal/core/execution/outputs.go (1)
DAGRunOutputs(4-7)
internal/service/frontend/auth/middleware_test.go (1)
internal/service/frontend/auth/builtin.go (2)
APIKeyValidator(23-25)TokenValidator(17-19)
internal/service/coordinator/client_test.go (1)
proto/coordinator/v1/coordinator_grpc.pb.go (1)
CoordinatorServiceServer(97-107)
ui/src/features/dags/components/dag-details/DAGDetailsContent.tsx (2)
ui/src/features/dag-runs/hooks/useHasOutputs.ts (1)
useHasOutputs(6-43)api/v2/api.gen.go (1)
DAGRunOutputs(487-493)
internal/core/spec/step.go (1)
internal/core/spec/builder.go (1)
StepBuildContext(25-28)
internal/common/secrets/resolver_test.go (1)
internal/common/secrets/resolver.go (1)
Resolver(13-33)
internal/service/worker/worker_test.go (1)
internal/service/worker/handler.go (1)
TaskHandler(18-20)
⏰ 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). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (44)
internal/service/frontend/auth/middleware_test.go (2)
17-17: LGTM! Excellent addition of compile-time interface check.This compile-time assertion ensures
mockAPIKeyValidatorcorrectly implements theAPIKeyValidatorinterface, catching any interface mismatches early. This is an idiomatic Go pattern that improves type safety.
41-41: LGTM! Compile-time interface check properly added.This compile-time assertion ensures
mockTokenValidatorcorrectly implements theTokenValidatorinterface. Combined with the check at line 17, these assertions provide robust compile-time guarantees for both mock types.internal/service/worker/worker_test.go (1)
515-538: LGTM! Clean and idiomatic mock implementation.The compile-time assertion at line 515 ensures interface conformance, and the
mockHandlerimplementation is well-structured:
- Provides test flexibility via
ExecuteFuncfor custom behavior injection- Properly handles context cancellation in the default execution path
- Follows Go idioms and coding guidelines
The mock is effectively used across multiple test scenarios throughout the file, enabling comprehensive worker behavior validation.
Note: The AI summary mentions the assertion appears "in two places," but only one instance is visible at line 515 in the provided diff.
internal/service/coordinator/client_test.go (1)
348-349: LGTM! Compile-time interface checks improve type safety.These static assertions ensure the mocks conform to their target interfaces at compile time, catching any mismatches early.
Also applies to: 378-379
internal/service/worker/poller_test.go (1)
347-348: LGTM! Compile-time interface check improves type safety.This static assertion ensures
mockCoordinatorCliconforms to thecoordinator.Clientinterface at compile time, catching any mismatches early.internal/common/telemetry/collector_test.go (2)
19-20: LGTM! Compile-time interface conformance check.This adds a compile-time check ensuring
mockDAGStoreproperly implementsexecution.DAGStore. This is an idiomatic Go pattern that prevents runtime interface satisfaction errors.
226-227: LGTM! Compile-time interface conformance check.This adds a compile-time check ensuring
mockServiceRegistryproperly implementsexecution.ServiceRegistry. Consistent with the pattern established throughout this test file.internal/runtime/agent/reporter_test.go (1)
235-236: The compile-time interface check is correct.The
var _ Sender = (*mockSender)(nil)assertion is standard Go idiom and ensuresmockSenderimplementsSender. No duplication was found in the file.internal/cmd/dagpicker/dagpicker_test.go (1)
170-171: Good: Compile-time interface assertion added.The assertion correctly ensures
mockDAGStoreimplementsexecution.DAGStore. However, the test usesassertinstead ofrequirefrom testify. Per the coding guidelines, update the test to usestretchr/testify/requirefor test assertions.Likely an incorrect or invalid review comment.
internal/core/spec/step.go (2)
53-54: LGTM! Output field type broadened appropriately.The type change from
stringtoanyenables both string and object forms while maintaining backward compatibility. The comment accurately documents the new behavior.
645-654: LGTM! Refactored to use centralized parsing.The function correctly delegates parsing to
parseOutputConfigand handles both success and error cases appropriately.internal/cmd/migrator_test.go (1)
20-21: LGTM!Good addition of the compile-time interface satisfaction check. This ensures the mock stays in sync with the
execution.DAGStoreinterface and will cause a compile-time error if the interface changes.internal/common/backoff/retrypolicy_test.go (1)
227-228: LGTM!Compile-time interface assertion ensures
mockRetryPolicycorrectly implementsRetryPolicy. This is consistent with the pattern used across other test files in this PR.schemas/dag.schema.json (1)
652-678: LGTM!Well-designed schema extension that maintains backward compatibility. The
oneOfpattern allows both simple string usage and the new object form withname,key, andomitfields. SettingadditionalProperties: falseensures strict validation of the object form.internal/service/scheduler/zombie_detector_test.go (3)
358-359: LGTM!Compile-time interface assertion for
mockDAGRunStore.
476-477: LGTM!Compile-time interface assertion for
mockDAGRunAttempt.
533-544: LGTM!New mock methods correctly implement the
WriteOutputsandReadOutputsinterface requirements. TheReadOutputsmethod properly handles the nil case to avoid panics when the mock returns nil data.ui/src/features/dag-runs/components/dag-run-details/index.ts (1)
5-5: LGTM!New
DAGRunOutputscomponent export follows the existing pattern and PascalCase naming convention.ui/src/features/dags/components/dag-details/DAGDetailsContent.tsx (3)
140-157: LGTM!Desktop Outputs tab conditionally renders only when
hasOutputsis true. Both modal and non-modal cases are handled correctly with appropriate tab navigation.
235-254: LGTM!Mobile Outputs tab mirrors the desktop implementation with consistent conditional rendering and styling.
324-326: LGTM!Outputs content correctly renders the
DAGRunOutputscomponent when the outputs tab is active.ui/src/features/dag-runs/hooks/useHasOutputs.ts (1)
6-42: LGTM!Well-structured hook with appropriate optimizations:
- Only fetches when the run is in a completed state
- Uses
isPausedto prevent unnecessary API calls- Disables automatic revalidation to reduce network traffic
- Returns
falsesafely when data is unavailable or emptyui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (3)
19-38: LGTM!Comprehensive mapping function covering all
StatusLabelvalues with a sensible default fallback.
71-95: LGTM!Well-structured memoization with proper dependencies. The filter and sort logic is clean and efficient.
129-244: LGTM!Well-implemented UI with:
- Compact styling following coding guidelines (h-8, text-sm)
- Proper dark mode support via Tailwind classes
- Accessible table structure
break-allfor handling long values- Visual feedback for copy operations
api/v2/api.yaml (3)
1805-1835: Outputs endpoint semantics line up with collected outputs behaviorThe
GET /dag-runs/{name}/{dagRunId}/outputscontract looks consistent and clear: it reuses the sharedDAGName/DAGRunIdparams (including the'latest'sentinel), returns200with an emptyoutputsobject when a run has no captured outputs, and reserves404for “DAG-run not found” only. This aligns well with the intended backend behavior and previous review feedback.
3131-3146: SubDAGRunDetail now using DAGRunId ref is a good normalizationSwitching
SubDAGRunDetail.dagRunIdto$ref: "#/components/schemas/DAGRunId"aligns sub-run IDs with the rest of the DAG-run APIs (including the'latest'sentinel and validation pattern) and keeps model shapes consistent.
3171-3171: CommandEntry reference change is a no-op structurallyThe
Step.commands.items$reftoCommandEntryis unchanged in meaning; this looks like formatting/consistency cleanup only, with no impact on generated clients or validation.ui/src/api/v2/schema.ts (4)
679-698: Outputs path typing matches OpenAPI, no manual edits neededThe
/dag-runs/{name}/{dagRunId}/outputsentry inpathscorrectly wiresgettooperations["getDAGRunOutputs"]and disallows other verbs, matching the OpenAPI. Since this file is generated, any future changes should come fromapi/v2/api.yamland regeneration rather than manual edits.
1308-1318: SubDAGRunDetail now reuses DAGRunId typeTyping
SubDAGRunDetail.dagRunIdascomponents["schemas"]["DAGRunId"]keeps sub-run IDs consistent with other DAG-run surfaces and reflects the YAML change correctly.
1469-1477: RunningTask ID/name fields correctly share DAGName/DAGRunId aliases
RunningTasknow usesDAGRunId/DAGNamefordagRunId,dagName,rootDagRunName/Id, andparentDagRunName/Id, which strengthens typing and stays in sync with the OpenAPI. No additional changes needed on the TS side.
3876-3921: getDAGRunOutputs operation is wired correctly for clientsThe
getDAGRunOutputsoperation uses the expectedDAGName/DAGRunIdpath params plus optionalremoteNode, and its responses (200 →DAGRunOutputs, 404/default →Error) match the YAML definition. This should be a straightforward surface for UI hooks/components to consume.api/v2/api.gen.go (12)
718-737: OutputsMetadata shape and types look consistentUsing
DAGName,DAGRunId, andStatusLabelhere aligns this metadata with the rest of the DAG‑run domain model, andParamsbeing optional matches how params are represented elsewhere.
865-884: RunningTask now reuses DAGName/DAGRunId aliases appropriatelySwitching these fields from plain
stringto the existingDAGName/DAGRunIdaliases improves semantic clarity without changing the wire format (they’re type aliases), and matches other structs.
1041-1064: SubDAGRunDetail DagRunId alias keeps API coherentChanging
SubDAGRunDetail.DagRunIdtoDAGRunIdbrings this type in line with other DAG‑run representations while remaining JSON‑compatible.
1324-1329: GetDAGRunOutputsParams matches existing remoteNode patternsThe params struct mirrors other DAG‑run operations that support
remoteNode(e.g.,GetDAGRunDetails,GetDAGRunLog), with consistent form/json tags and optionality.
1937-1939: ServerInterface surface for GetDAGRunOutputs is correctly addedThe new method sits alongside other
/dag-runs/{name}/{dagRunId}operations and uses the sameDAGName,DAGRunId, and params pattern, so implementers get a familiar signature.
2153-2157: Unimplemented.GetDAGRunOutputs stub is consistentReturning
http.StatusNotImplementedhere matches the behavior of the other Unimplemented methods and keeps the generated fallback uniform.
3015-3066: ServerInterfaceWrapper.GetDAGRunOutputs wiring mirrors GetDAGRunLogPath parameter binding, context/auth‑scope setup, optional
remoteNodequery binding, and middleware chaining all follow the same pattern asGetDAGRunLogand similar handlers, so request construction and delegation look correct.
5082-5084: Route registration for outputs endpoint is in the expected place
GET /dag-runs/{name}/{dagRunId}/outputsis grouped with the other DAG‑run subresources under/dag-runs/{name}/{dagRunId}, and uses the standard wrapper handler.
5914-5952: GetDAGRunOutputs request/response types follow the standard visitor patternThe request object bundles
name,dagRunId, and params; the 200 response aliasesDAGRunOutputs, with 404/default wrappers implementingVisitGetDAGRunOutputsResponseexactly like neighboring endpoints.
7631-7633: StrictServerInterface includes GetDAGRunOutputs with consistent signatureThe strict interface method uses the usual
(ctx, request) (response, error)pattern and the same request/response types as the non‑strict surface, so strict handlers integrate cleanly.
8211-8237: strictHandler.GetDAGRunOutputs bridges to StrictServerInterface correctlyRequest construction (
Name,DagRunId,Params), middleware wrapping with the"GetDAGRunOutputs"operation name, and response dispatch viaVisitGetDAGRunOutputsResponseall mirror the other strict handlers, so the runtime glue looks sound.
9386-9599: Updated swaggerSpec blob appears to be a regenerated artifactThis chunk change reflects an updated, compressed OpenAPI spec. As long as this file remains fully generated from the canonical spec via oapi‑codegen (no manual edits), this update is fine; review of behavior should happen at the spec level rather than here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/integration/outputs_collection_test.go (2)
407-424: UsedagRunIDto validate or construct the outputs file path.The
dagRunIDparameter is passed but never used. The function walks the directory to find the firstoutputs.jsonand only checks ifoutputs.Metadata.DAGRunIDis empty. If multiple DAG run attempts exist, this could return unrelated outputs. Either construct the path deterministically usingdagRunID, or validate that the found outputs match the expecteddagRunIDby comparingoutputs.Metadata.DAGRunID == dagRunID. Additionally, silently returningnilon errors (lines 393–400) masks real issues during test debugging—consider logging failures or raising an error.
417-424: Walk error is silently ignored.The return value of
filepath.Walkis discarded. If the walk fails entirely (e.g., directory doesn't exist), the function silently returnsnil, which could mask test setup issues.- _ = filepath.Walk(dagRunDir, func(path string, info os.FileInfo, err error) error { + walkErr := filepath.Walk(dagRunDir, func(path string, info os.FileInfo, err error) error { require.NoError(t, err) if info.Name() == filedagrun.OutputsFile { outputsPath = path return filepath.SkipAll } return nil }) + // Walk error is OK if directory doesn't exist (no outputs case) + if walkErr != nil && !os.IsNotExist(walkErr) { + require.NoError(t, walkErr) + }
🧹 Nitpick comments (8)
ui/src/features/dags/components/visualization/TimelineChart.tsx (3)
229-229: Consider using a stable key for timeline rows.Using
item.nameas the key could cause issues if duplicate step names exist. Consider using the node index or a combination of name and index.- <div - key={item.name} + <div + key={`${item.name}-${idx}`}
246-249: Hardcoded pixel offset may cause layout issues on narrow containers.The
130pxoffset assumes sufficient container width. On narrow screens, this calc could produce negative widths. Consider a more responsive approach or minimum container width constraint.style={{ left: `calc(${leftPercent}% + 130px)`, - width: `calc(${Math.max(widthPercent, 0.5)}% - 130px)`, + width: `max(4px, calc(${Math.max(widthPercent, 0.5)}% - 130px))`, minWidth: '4px',
77-88: Hardcoded colors don't support dark mode.Per coding guidelines, UI components should support both light and dark modes. These hardcoded hex colors won't adapt to theme changes. Consider using CSS variables or Tailwind theme colors that respond to dark mode.
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (3)
53-59: Unused props could be removed or documented.The props
isSubDAGRun,parentName, andparentDagRunIdare defined but immediately prefixed with_indicating they're unused. If these are planned for future use, add a comment; otherwise, consider removing them to keep the interface clean.
113-117: Add error handling for clipboard API.
navigator.clipboard.writeTextcan fail if clipboard permission is denied or unavailable. Consider wrapping in try/catch to provide user feedback on failure.🔎 Suggested fix
const handleCopy = async (key: string, value: string) => { - await navigator.clipboard.writeText(value); - setCopiedKey(key); - setTimeout(() => setCopiedKey(null), 2000); + try { + await navigator.clipboard.writeText(value); + setCopiedKey(key); + setTimeout(() => setCopiedKey(null), 2000); + } catch { + // Clipboard API may fail silently - consider adding toast notification + console.warn('Failed to copy to clipboard'); + } };
235-245: Add explicit button type for accessibility.Add
type="button"to prevent form submission behavior if this component is ever used within a form context. The focus indicator via Tailwind utilities would also improve keyboard navigation.<button + type="button" onClick={() => handleCopy(key, value)} - className="p-1 hover:bg-accent rounded" + className="p-1 hover:bg-accent rounded focus:outline-none focus:ring-2 focus:ring-ring" title="Copy value" >internal/runtime/agent/agent.go (1)
707-713: Consider logging params serialization failures.While silently ignoring the error is acceptable since params are optional, a debug-level log could help troubleshoot issues if params contain unexpected data.
if len(a.dag.Params) > 0 { if data, err := json.Marshal(a.dag.Params); err == nil { paramsJSON = string(data) + } else { + logger.Debug(ctx, "Failed to serialize params for outputs metadata", tag.Error(err)) } }ui/src/features/dags/components/DAGStatus.tsx (1)
48-49: Cookie value may be undefined on first load.
cookie['flowchart']will beundefinedon first visit. TheGraphcomponent should handle this, but consider providing a default value.-const [flowchart, setFlowchart] = useState<FlowchartType>(cookie['flowchart']); +const [flowchart, setFlowchart] = useState<FlowchartType>(cookie['flowchart'] || 'TD');
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/integration/outputs_collection_test.gointernal/runtime/agent/agent.goui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsxui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/dag-details/DAGDetailsContent.tsxui/src/features/dags/components/visualization/TimelineChart.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/features/dags/components/dag-details/DAGDetailsContent.tsx
🧰 Additional context used
📓 Path-based instructions (4)
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsxui/src/features/dags/components/visualization/TimelineChart.tsxui/src/features/dags/components/DAGStatus.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsxui/src/features/dags/components/visualization/TimelineChart.tsxui/src/features/dags/components/DAGStatus.tsx
**/*.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/integration/outputs_collection_test.gointernal/runtime/agent/agent.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/integration/outputs_collection_test.go
🧠 Learnings (4)
📚 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 ui/**/*.{ts,tsx} : UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (`JobList.tsx`) and hooks with `use*` (`useJobs.ts`)
Applied to files:
ui/src/features/dags/components/DAGStatus.tsx
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Applied to files:
ui/src/features/dags/components/DAGStatus.tsx
📚 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/integration/outputs_collection_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/integration/outputs_collection_test.go
🧬 Code graph analysis (4)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (5)
api/v2/api.gen.go (1)
StatusLabel(954-954)ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)ui/src/hooks/api.ts (1)
useQuery(28-28)ui/src/components/ui/input.tsx (1)
Input(21-21)ui/src/components/ui/table.tsx (6)
Table(126-126)TableHeader(132-132)TableRow(133-133)TableHead(131-131)TableBody(127-127)TableCell(129-129)
ui/src/features/dags/components/visualization/TimelineChart.tsx (3)
ui/src/api/v2/schema.ts (1)
components(961-1716)api/v2/api.gen.go (1)
NodeStatus(713-713)ui/src/components/ui/tooltip.tsx (3)
Tooltip(59-59)TooltipTrigger(59-59)TooltipContent(59-59)
ui/src/features/dags/components/DAGStatus.tsx (4)
ui/src/features/dags/components/visualization/index.ts (1)
FlowchartType(10-10)ui/src/features/dags/components/visualization/Graph.tsx (1)
FlowchartType(30-30)ui/src/ui/BorderedBox.tsx (1)
BorderedBox(18-69)ui/src/features/dags/contexts/DAGContext.ts (1)
DAGContext(3-9)
internal/runtime/agent/agent.go (5)
internal/core/execution/context.go (1)
Context(16-27)internal/runtime/context.go (2)
Context(14-14)GetDAGContext(45-47)internal/common/stringutil/stringutil.go (2)
ScreamingSnakeToCamel(73-101)FormatTime(18-24)internal/core/execution/outputs.go (2)
DAGRunOutputs(4-7)OutputsMetadata(10-17)internal/common/masking/masker.go (2)
NewMasker(23-38)SourcedEnvVars(13-15)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (6)
internal/runtime/agent/agent.go (3)
556-561: LGTM - Outputs written before status finalization.Good implementation following the spec to write outputs before finalizing the DAG run status. Error logging without failing the run is appropriate since output collection is non-critical.
689-705: LGTM - Secret masking implementation.Good use of the existing masking infrastructure to prevent sensitive data leakage in outputs. The in-place modification of the outputs map is efficient.
614-618: Verify thatplan.Nodes()returns nodes in execution order.The comment on line 614 states iteration happens "in execution order," but this guarantee depends on the implementation of the
Nodes()method. If it doesn't preserve order, the "last value wins" semantics for duplicate keys could be non-deterministic. Inspect the Plan type and its Nodes() method implementation to confirm the order guarantee.ui/src/features/dags/components/DAGStatus.tsx (2)
275-305: LGTM - Clean tabbed interface implementation.The tab structure follows the compact UI guidelines with appropriate icons and conditional visibility for the Timeline tab. The cursor-pointer and gap styling provide good UX.
400-403: LGTM - Outputs tab integration.Proper integration with the new
DAGRunOutputscomponent, passing the requireddagNameanddagRunIdprops from the parentdagRunobject.internal/integration/outputs_collection_test.go (1)
44-48: Verify test expectation matches implementation.The test expects
outputs["result"]to be"RESULT=42"(full KEY=VALUE string), but the comment suggestsagent.golines 648-649 extract only the value viakv.Value(). If this is correct, the test would fail. Manual verification of theKeyValue.Value()method implementation andcollectOutputs()logic is needed to confirm whether there's an actual discrepancy.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (4)
53-59: Unused props prefixed with underscores.The props
isSubDAGRun,parentName, andparentDagRunIdare declared but unused. If these are planned for future use, consider adding a brief comment explaining the intent. Otherwise, remove them to keep the API surface minimal.
240-240: Usebreak-wordsinstead ofbreak-allfor text wrapping.Per coding guidelines, long text should use
whitespace-normal break-wordsrather thanbreak-all, which can break words mid-character inappropriately.🔎 Proposed fix
- <TableCell className="font-mono text-sm text-muted-foreground break-all"> + <TableCell className="font-mono text-sm text-muted-foreground whitespace-normal break-words">
244-254: Improve keyboard accessibility for copy button.The copy button should have explicit
type="button"and visible focus styles per coding guidelines on keyboard navigation support.🔎 Proposed fix
<button + type="button" onClick={() => handleCopy(key, value)} - className="p-1 hover:bg-accent rounded" + className="p-1 hover:bg-accent rounded focus:outline-none focus:ring-2 focus:ring-ring" title="Copy value" + aria-label={`Copy value for ${key}`} >
203-220: Add ARIA attributes to sortable table headers.The sortable headers should include appropriate ARIA attributes to communicate sort state to assistive technologies.
🔎 Proposed fix
<TableHead className="w-[200px] cursor-pointer select-none" onClick={() => handleSort('name')} + aria-sort={sortConfig.key === 'name' ? (sortConfig.direction === 'asc' ? 'ascending' : 'descending') : 'none'} + role="columnheader" > <div className="flex items-center gap-1"> Key <ArrowUpDown className="h-3 w-3 text-muted-foreground" /> </div> </TableHead> <TableHead className="cursor-pointer select-none" onClick={() => handleSort('value')} + aria-sort={sortConfig.key === 'value' ? (sortConfig.direction === 'asc' ? 'ascending' : 'descending') : 'none'} + role="columnheader" >
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧰 Additional context used
📓 Path-based instructions (2)
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧬 Code graph analysis (1)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (6)
api/v2/api.gen.go (1)
StatusLabel(954-954)ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)ui/src/hooks/api.ts (1)
useQuery(28-28)npm/dagu/lib/platform.js (1)
key(93-93)ui/src/components/ui/input.tsx (1)
Input(21-21)ui/src/components/ui/table.tsx (6)
Table(126-126)TableHeader(132-132)TableRow(133-133)TableHead(131-131)TableBody(127-127)TableCell(129-129)
⏰ 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 (3)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (3)
18-38: LGTM!The mapping function correctly handles all
StatusLabelvalues with a sensible default fallback.
68-77: LGTM!Query configuration is appropriate with proper path parameters and a sensible default for
remoteNode.
150-150: Verifybg-surfaceclass exists in your Tailwind config.The class
bg-surfaceis non-standard Tailwind. If this is a custom utility, ensure it's properly defined and supports dark mode. The coding guidelines prefer explicit class pairs likebg-slate-200 dark:bg-slate-700.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (2)
113-121: Clipboard error handling is properly implemented.The
handleCopyfunction now wraps the clipboard operation in a try-catch block, addressing the previous review feedback. The silent catch is acceptable for clipboard failures since there's no critical user action depending on it.
123-130: Loading state correctly shows stale data while fetching.The condition
isLoading && !dataensures the loading message only appears on initial load, allowing stale data to remain visible during refetches. This follows the coding guidelines.internal/core/spec/step.go (2)
656-676: Error handling now properly propagates parsing errors.Previous review comments flagged that these builders swallowed errors. The current implementation correctly returns errors from
parseOutputConfig, addressing the concern.Note:
parseOutputConfigis called three times per step (once by each builder). If profiling indicates this is expensive, consider caching the parsed result in the build context.
175-176: Transformer ordering dependency is now less fragile.Since all three builders (
buildStepOutput,buildStepOutputKey,buildStepOutputOmit) now properly return errors fromparseOutputConfig, the ordering dependency mentioned in previous reviews is no longer a correctness concern—each builder validates independently.The minor inefficiency of parsing three times could be addressed later if profiling shows it's significant.
internal/service/frontend/api/v2/dagruns.go (1)
358-364: Verify OpenAPI spec alignment for missing outputs behavior.The handler returns HTTP 200 with an empty outputs structure when
outputs.jsondoesn't exist (outputs == nil). A previous review noted that the OpenAPI 404 description says "Outputs file not found (DAG may not have captured any outputs)", which suggests 404 should be returned for missing outputs.Confirm the intended behavior and update either the spec or the handler to be consistent.
🧹 Nitpick comments (4)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (3)
88-92: Add type guard for value in filter operation.The
outputsmap is typed asRecord<string, string>in the schema, but if the API ever returns a non-string value,value.toLowerCase()would throw. Consider adding a defensive check.🔎 Proposed defensive fix
entries = entries.filter( ([key, value]) => key.toLowerCase().includes(lowerFilter) || - value.toLowerCase().includes(lowerFilter) + (typeof value === 'string' && value.toLowerCase().includes(lowerFilter)) );
248-260: Add keyboard accessibility to the copy button.The copy button is missing keyboard accessibility attributes. Consider adding
type="button"and proper focus styling for keyboard navigation support.As per coding guidelines: "Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels".
🔎 Proposed fix
<button + type="button" onClick={() => handleCopy(key, value)} - className="p-1 hover:bg-accent rounded" + className="p-1 hover:bg-accent rounded focus:outline-none focus:ring-2 focus:ring-ring" title="Copy value" + aria-label={`Copy value for ${key}`} >
206-227: Table headers lack keyboard accessibility for sorting.The sortable table headers use
onClickbut aren't keyboard-accessible. Consider using<button>elements or addingtabIndex,role, andonKeyDownhandlers.As per coding guidelines: "Maintain keyboard navigation support in all interactive components".
🔎 Proposed approach
<TableHead - className="w-[200px] cursor-pointer select-none" - onClick={() => handleSort('name')} + className="w-[200px]" > - <div className="flex items-center gap-1"> + <button + type="button" + onClick={() => handleSort('name')} + className="flex items-center gap-1 cursor-pointer select-none hover:text-foreground focus:outline-none focus:ring-2 focus:ring-ring rounded" + > Key <ArrowUpDown className="h-3 w-3 text-muted-foreground" /> - </div> + </button> </TableHead>Apply similar changes to the Value column header.
internal/service/frontend/api/v2/dagruns.go (1)
366-372: Silent parse error for CompletedAt may hide data issues.If
CompletedAtcontains a malformed timestamp, the error is silently ignored andcompletedAtremains zero. Consider logging a warning to aid debugging.🔎 Proposed fix
var completedAt time.Time if outputs.Metadata.CompletedAt != "" { - if t, err := time.Parse(time.RFC3339, outputs.Metadata.CompletedAt); err == nil { + t, parseErr := time.Parse(time.RFC3339, outputs.Metadata.CompletedAt) + if parseErr == nil { completedAt = t + } else { + logger.Warn(ctx, "Failed to parse CompletedAt timestamp", + slog.String("completedAt", outputs.Metadata.CompletedAt), + tag.Error(parseErr), + ) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/core/spec/step.gointernal/service/frontend/api/v2/dagruns.goui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/service/frontend/api/v2/dagruns.gointernal/core/spec/step.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Applied to files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧬 Code graph analysis (2)
internal/service/frontend/api/v2/dagruns.go (4)
api/v2/api.gen.go (10)
GetDAGRunOutputsRequestObject(5914-5918)GetDAGRunOutputsResponseObject(5920-5922)GetDAGRunOutputs404JSONResponse(5933-5933)Error(545-554)DAGRunOutputs(487-493)OutputsMetadata(719-737)GetDAGRunOutputs200JSONResponse(5924-5924)DAGName(414-414)Status(951-951)StatusLabel(954-954)internal/core/execution/dagrun.go (1)
DAGRunAttempt(131-159)internal/core/execution/outputs.go (2)
DAGRunOutputs(4-7)OutputsMetadata(10-17)internal/persistence/legacy/model/status.go (2)
Time(62-64)Status(30-46)
internal/core/spec/step.go (1)
internal/core/spec/builder.go (1)
StepBuildContext(25-28)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (9)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (3)
1-16: LGTM! Clean imports and well-organized dependencies.The component imports are properly organized with React hooks, UI components, and utilities clearly separated.
18-38: Status mapping function is complete and handles all cases.The
statusLabelToStatusfunction covers allStatusLabelenum values with a sensible default fallback.
53-59: Unused props are intentionally prefixed.The underscore-prefixed props (
_isSubDAGRun,_parentName,_parentDagRunId) indicate they're reserved for future use, likely for sub-DAG output navigation.internal/core/spec/step.go (3)
53-54: Output field type change supports both string and object forms.The
Outputfield type change fromstringtoanyenables the new object-based configuration withname,key, andomitfields while maintaining backward compatibility with string values.
599-643: Well-structured parsing for the new output configuration format.The
parseOutputConfigfunction cleanly handles:
nilinput (returnsnil, nil)- String form with optional
$prefix stripping- Object form with validation that
nameis requiredThe empty-string-after-trim check (lines 619-621) is good defensive coding.
645-654: buildStepOutput correctly extracts the output name.The function properly delegates to
parseOutputConfigand returns theNamefield from the parsed config.internal/service/frontend/api/v2/dagruns.go (3)
321-346: Well-structured endpoint with proper DAG-run resolution.The endpoint correctly handles both "latest" and specific
dagRunIdlookups, returning appropriate 404 responses when the DAG-run is not found.
348-356: Error handling for ReadOutputs is appropriate.The error is logged with context (dag name, dagRunId) and properly wrapped before returning. This provides good observability for debugging.
374-385: Response construction is correct and maps all metadata fields.The response properly maps internal types to API types, including the status label conversion.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧰 Additional context used
📓 Path-based instructions (2)
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Applied to files:
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx
🧬 Code graph analysis (1)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (5)
api/v2/api.gen.go (1)
StatusLabel(954-954)ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)ui/src/hooks/api.ts (1)
useQuery(28-28)ui/src/components/ui/input.tsx (1)
Input(21-21)ui/src/components/ui/table.tsx (6)
Table(126-126)TableHeader(132-132)TableRow(133-133)TableHead(131-131)TableBody(127-127)TableCell(129-129)
⏰ 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). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (8)
ui/src/features/dag-runs/components/dag-run-details/DAGRunOutputs.tsx (8)
1-16: LGTM!Imports are well-organized and all dependencies are properly utilized throughout the component.
18-38: LGTM!The status mapping function is well-structured with proper enum handling and a safe default case.
40-58: LGTM!Type definitions are clear. The underscore-prefixed unused props indicate they're intentionally accepted but not yet implemented, which is a valid pattern for interface compliance or future use.
80-106: LGTM!The filtering and sorting logic is well-implemented with proper memoization. The
String()coercion for sorting ensures consistent behavior, and the case-insensitive filtering enhances usability.
108-123: LGTM!Handler functions are well-implemented. The clipboard operation now includes proper error handling as requested in previous reviews, and the sort toggle logic is intuitive.
125-152: LGTM!Conditional rendering properly handles all states. The loading check (
isLoading && !data) correctly avoids blocking content during refreshes, following the coding guidelines and addressing previous feedback.
154-276: Overall structure follows best practices.The component properly implements:
- Compact form elements (h-8 input)
- Keyboard navigation with focus indicators
- Proper ARIA labels for accessibility
- Visual feedback for user actions (copy button)
- Responsive layout
176-176: No action needed. ThecompletedAtfield is a required string property in theOutputsMetadataschema, not optional or nullable. dayjs will receive a valid RFC3339 timestamp string and parse it correctly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/test/helper.go (1)
532-541: Handle errors fromfilepath.Walkexplicitly.The error returned by
filepath.Walkis silently ignored. While this is test code, errors such as permission issues or I/O failures could cause the test to pass incorrectly by returningnilinstead of failing appropriately.🔎 Proposed fix
- var outputsPath string - _ = filepath.Walk(dagRunDir, func(path string, info os.FileInfo, err error) error { + var outputsPath string + walkErr := filepath.Walk(dagRunDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } if info.Name() == filedagrun.OutputsFile { outputsPath = path return filepath.SkipAll } return nil }) + if walkErr != nil && !errors.Is(walkErr, filepath.SkipAll) { + require.NoError(t, walkErr) + }internal/runtime/plan.go (1)
398-401: Consider callingnode.Init()and document ID mutation behavior.Two observations:
Other plan constructors call
node.Init()(lines 53, 81, 111), but this one doesn't. If nodes passed to this function are not pre-initialized, they may have incomplete state.The function directly mutates
node.id = i, which could cause issues if the same node instance is reused across multiple test plans or if nodes already have IDs assigned.Consider these options:
Option 1 (preferred): Call
Init()and document the ID mutation in the function comment:🔎 Proposed fix for Option 1
// NewPlanWithNodes creates a Plan from pre-existing nodes. // This is primarily intended for testing scenarios where nodes need // pre-populated state (e.g., OutputVariables). -// Note: Does not build dependency edges - use only for testing isolated functions. +// Note: Assigns sequential IDs to nodes and initializes them. Does not build +// dependency edges - use only for testing isolated functions. func NewPlanWithNodes(nodes ...*Node) *Plan { p := &Plan{ nodeByID: make(map[int]*Node), nodeByName: make(map[string]*Node), DependencyMap: make(map[int][]int), DependantMap: make(map[int][]int), nodes: make([]*Node, 0, len(nodes)), + startedAt: time.Now(), } for i, node := range nodes { node.id = i + node.Init() p.addNode(node) } return p }Option 2: If nodes are expected to be pre-initialized, verify that in the function and update documentation:
Alternative approach
Add a comment clarifying that nodes should be pre-initialized and IDs will be overwritten:
// NewPlanWithNodes creates a Plan from pre-existing nodes. // This is primarily intended for testing scenarios where nodes need -// pre-populated state (e.g., OutputVariables). +// pre-populated state (e.g., OutputVariables). Nodes should be pre-initialized +// via Init() before being passed to this function. +// WARNING: Node IDs will be overwritten with sequential values. // Note: Does not build dependency edges - use only for testing isolated functions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/runtime/agent/agent_test.gointernal/runtime/plan.gointernal/test/helper.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/runtime/plan.gointernal/runtime/agent/agent_test.gointernal/test/helper.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/runtime/agent/agent_test.go
🧬 Code graph analysis (3)
internal/runtime/plan.go (1)
internal/runtime/node.go (1)
Node(33-43)
internal/runtime/agent/agent_test.go (2)
internal/test/helper.go (3)
Setup(82-226)DAG(427-430)Agent(611-617)internal/runtime/agent/agent.go (1)
Agent(50-129)
internal/test/helper.go (1)
internal/persistence/filedagrun/attempt.go (1)
OutputsFile(39-39)
🪛 GitHub Check: Go Linter
internal/test/helper.go
[failure] 547-547:
G304: Potential file inclusion via variable (gosec)
⏰ 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). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (3)
internal/runtime/agent/agent_test.go (2)
594-617: Good test for secret masking.The test correctly verifies that secrets are masked in outputs by creating a temporary secret file, referencing it in a DAG, and asserting that the output contains a masked placeholder instead of the actual secret value. The test properly uses the
TempFilehelper frominternal/testand follows all Go testing conventions.
498-592: Excellent test coverage for output collection.The table-driven approach comprehensively validates the new output collection feature, including snake_case to camelCase conversion, custom output keys, the omit flag, multiple steps, last-wins behavior, and edge cases. The test structure follows Go best practices with proper use of
t.Parallel()and subtests. Thedag.ReadOutputs(t)helper correctly reads outputs from the persisted outputs.json file, and the underlying persistence layer properly handles writing and reading output collections.internal/test/helper.go (1)
547-548: Static analysis G304 warning is a false positive.The gosec warning about "potential file inclusion via variable" can be safely ignored here. This is a test helper that reads from paths constructed from test configuration and validated constants.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/core/dag_test.go (2)
29-30: Clarify the socket length constraint.The comment states "50 is the maximum length of a unix socket address", but the typical limit for Unix domain socket paths is 108 bytes on Linux (UNIX_PATH_MAX). If 50 is an application-specific constraint rather than a system limit, please clarify this in the comment to avoid confusion.
🔎 Suggested clarification
- // 50 is the maximum length of a unix socket address + // 50 is the application-defined maximum length for socket addresses + // (Unix systems typically support up to 108 bytes) require.LessOrEqual(t, 50, len(dag.SockAddr("")))
308-349: Consider removing redundant struct field tests.These tests (
TestAuthConfigandTestDAGRegistryAuths) only verify basic struct field assignment and map access, which are guaranteed by Go's type system. They don't test any business logic, methods, or edge cases. Consider removing them to reduce maintenance overhead, or if retained, enhance them to test actual AuthConfig-related functionality.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/core/dag_test.gointernal/core/errors_test.gointernal/core/validator_test.gointernal/runtime/plan.go
✅ Files skipped from review due to trivial changes (1)
- internal/runtime/plan.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/core/validator_test.gointernal/core/errors_test.gointernal/core/dag_test.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/core/validator_test.gointernal/core/errors_test.gointernal/core/dag_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/core/errors_test.gointernal/core/dag_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/core/errors_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/core/dag_test.go
🧬 Code graph analysis (3)
internal/core/validator_test.go (1)
internal/core/validator.go (2)
ValidateSteps(41-126)RegisterStepValidator(36-38)
internal/core/errors_test.go (1)
internal/core/spec/errors.go (21)
ErrInvalidSchedule(6-6)ErrScheduleMustBeStringOrArray(7-7)ErrInvalidScheduleType(8-8)ErrInvalidStepData(14-14)ErrStepCommandIsEmpty(31-31)ErrStepCommandMustBeArrayOrString(32-32)ErrInvalidParamValue(30-30)ErrInvalidSignal(21-21)ErrInvalidEnvValue(29-29)ErrExecutorTypeMustBeString(23-23)ErrExecutorConfigValueMustBeMap(24-24)ErrExecutorHasInvalidKey(25-25)ErrExecutorConfigMustBeStringOrMap(26-26)ErrDotEnvMustBeStringOrArray(9-9)ErrPreconditionMustBeArrayOrString(13-13)ErrPreconditionValueMustBeString(10-10)ErrPreconditionHasInvalidKey(12-12)ErrContinueOnOutputMustBeStringOrArray(17-17)ErrContinueOnExitCodeMustBeIntOrArray(16-16)ErrDependsMustBeStringOrArray(22-22)ErrStepsMustBeArrayOrMap(15-15)
internal/core/dag_test.go (1)
internal/core/dag.go (5)
DAG(63-169)InitializeDefaults(378-380)TypeChain(27-27)TypeGraph(25-25)Schedule(463-468)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (12)
internal/core/errors_test.go (1)
1-460: Excellent test coverage following all coding guidelines!This test file demonstrates best practices for Go testing:
- ✅ Table-driven tests where appropriate
- ✅ Proper use of
testify/require(for critical checks) andtestify/assert(for simple assertions)- ✅ Comprehensive coverage of edge cases (empty lists, nil values, multiple errors)
- ✅ Correct use of
t.Parallel()for concurrent test execution- ✅ Tests error wrapping/unwrapping behavior with
errors.Isanderrors.As- ✅ Compile-time and runtime interface compliance verification
- ✅ Verification of all error constants for meaningfulness
The tests cover both success and failure paths thoroughly, including integration with the standard library's error handling mechanisms.
internal/core/dag_test.go (3)
218-306: LGTM! Well-structured JSON and scheduling tests.Good use of shared test fixtures, proper error handling verification, and comprehensive coverage of both success and failure paths.
351-437: LGTM! Excellent table-driven test coverage.Comprehensive test cases covering all combinations of DAG and Step log output modes, including nil handling. Well-structured with clear naming and proper use of
t.Parallel().
439-921: LGTM! Comprehensive test coverage for all DAG methods.Excellent table-driven tests covering:
- Validation logic with multi-level dependencies
- Tag checking with case sensitivity and nil handling
- Parameter parsing with edge cases (multiple equals, no equals, empty values)
- Process group determination
- Filename extraction with various extensions
- String representation verification
- Default initialization with pre-existing values
- Extended scheduling scenarios (empty, single, multiple, nil Parsed)
- Name retrieval logic
All tests follow Go best practices: proper use of
t.Parallel(), testify assertions, clear naming, and comprehensive edge case coverage.internal/core/validator_test.go (8)
13-58: Excellent comprehensive coverage of step ID validation.The table-driven test covers all relevant edge cases including valid formats, invalid starting characters, special characters, unicode, and emoji. The parallel execution and clear test case naming make this very maintainable.
60-120: LGTM: Thorough reserved word detection testing.The test properly validates case-insensitive detection of all reserved words and includes appropriate negative test cases. The parallel execution is correctly applied.
122-445: Outstanding test organization and coverage.This test function exemplifies best practices:
- Clear organization by validation pass (ID validation, conflicts, dependencies, etc.)
- Strategic use of
testExecto avoid side effects from validators registered in other packages- Comprehensive coverage of success and failure paths
- Proper verification of side effects (e.g., dependency resolution at line 291)
The parallel execution and detailed subtest naming make this very maintainable.
447-548: Correct handling of global state in tests.The tests properly avoid parallel execution and use
defer delete()for cleanup. This is the right approach for testing global registry behavior.One minor note: If a test in this function panics before cleanup, or if other test files also register validators with these type names, there could be interference. However, this is an acceptable trade-off for testing global registration behavior, and the current approach follows Go testing conventions.
550-627: LGTM: Comprehensive dependency resolution testing.The tests thoroughly cover ID-to-name resolution, name preservation, mixed references, and edge cases. The verification of mutation side effects (e.g., lines 567-568) ensures the resolution actually modifies the DAG as expected.
629-664: LGTM: Clean boundary testing for step validation.The tests properly validate the 40-character name limit with both the failure case (41 characters) and the success case (exactly 40 characters). Using
errors.Is()for error type checking is the correct idiomatic approach.
666-709: LGTM: Proper testing of validator error wrapping.The tests correctly verify:
- Graceful handling when no validator is registered
- Nil validator handling
- Error wrapping in
ValidationErrorwith the correct field name- Use of
errors.As()anderrors.Is()for wrapped error checking
711-774: Excellent complex scenario coverage.The tests validate important real-world patterns:
- Scalability with a 100-step sequential DAG
- Diamond dependency pattern (common in DAG workflows)
- Multiple independent execution chains
The ASCII art comment for the diamond pattern enhances readability. These tests ensure the validator performs well with realistic complex DAGs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1511 +/- ##
==========================================
+ Coverage 62.17% 62.84% +0.67%
==========================================
Files 209 209
Lines 23190 23343 +153
==========================================
+ Hits 14418 14671 +253
+ Misses 7360 7259 -101
- Partials 1412 1413 +1
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.