Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new "log" executor type: schema entries, runtime registration and implementation, YAML/schema tests, UI utilities and components to display configured log messages, API-driven log fetching/display in status rows, and multiple unit tests covering schema, runtime, and UI paths. ChangesLog Executor Implementation & UI Integration
Sequence DiagramsequenceDiagram
participant User
participant DAGRuntime as DAG Runtime
participant LogExec as Log Executor
participant API as API/Stdout
participant UI
User->>DAGRuntime: Load DAG with log step
DAGRuntime->>DAGRuntime: Validate schema (type="log")
DAGRuntime->>LogExec: Instantiate log executor with config
LogExec->>LogExec: Validate config (message required)
LogExec->>LogExec: Expand variables in message
LogExec->>API: Write message to stdout
UI->>API: Request DAG run & step details
API-->>UI: Return step with executorConfig
UI->>UI: getLogStepMessage / getLogMessageFromConfig
UI->>UI: Render LogStepMessage component
UI->>API: Fetch step stdout (if available)
API-->>UI: Return stdout content
UI->>UI: formatLogStepOutput (strip ANSI, normalize newlines)
UI->>UI: Render formatted output in NodeStatusTableRow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/spec/step_test.go (1)
21-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
"log"executor capabilities not registered inTestMain.Every other builtin type recognized in this package (including
"mail"with empty capabilities) is explicitly registered viacore.RegisterExecutorCapabilitiesinTestMain. The new"log"type is absent.TestLoadYAMLLogStepcurrently passes because the test YAML contains nocommand/script/shell/containerfields, but the omission is inconsistent and will silently affect future tests that probe capability validation for log steps.🛡️ Proposed fix
// mail: no command support core.RegisterExecutorCapabilities("mail", core.ExecutorCapabilities{}) // chat: LLM executor core.RegisterExecutorCapabilities("chat", core.ExecutorCapabilities{LLM: true}) +// log: no command support +core.RegisterExecutorCapabilities("log", core.ExecutorCapabilities{})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/spec/step_test.go` around lines 21 - 62, TestMain is missing registration for the "log" executor; add a call to core.RegisterExecutorCapabilities("log", core.ExecutorCapabilities{}) in TestMain ( alongside the existing registrations like "mail" ) so the "log" type is explicitly recognized by tests and future capability validation (use the same empty capabilities as "mail" unless log should have specific flags).
🧹 Nitpick comments (6)
ui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsx (2)
115-122: 💤 Low value
toHaveClass('w-[320px]')ties the test to a Tailwind implementation detail.If
LogStepMessage's width is ever adjusted, this assertion fails without any semantic regression. The test already validates the element is present and has no SVG icon; the class check adds low informational value.♻️ Consider removing the class assertion
const message = screen.getByLabelText('Log message: Deploying production'); expect(message).toBeInTheDocument(); -expect(message).toHaveClass('w-[320px]'); expect(message.querySelector('svg')).not.toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsx` around lines 115 - 122, Remove the brittle Tailwind-specific assertion in the NodeStatusTableRow test: delete the expect(message).toHaveClass('w-[320px]') line in the test (the variable is message from getByLabelText('Log message: Deploying production')) so the test relies on semantic behavior only (presence, absence of SVG, no interpolated text), not a concrete CSS utility class.
53-65: 💤 Low value
useQuerymock keyed solely oninittruthiness is brittle.The mock distinguishes calls by whether the second argument (
init) is truthy, returning{ content: 'Deploying production\n' }for any call whereinitis provided. IfNodeStatusTableRowadds a seconduseQuerycall that also passes aninitvalue (or the current call loses itsinit), the mock silently changes behavior without a test failure.Consider tying the mock to the actual path argument instead:
♻️ More explicit mock
-mockedUseQuery.mockImplementation((_, init) => ({ - data: init ? { content: 'Deploying production\n' } : undefined, +mockedUseQuery.mockImplementation((path) => ({ + data: String(path).includes('/stdout') ? { content: 'Deploying production\n' } : undefined, isLoading: false, error: undefined, isValidating: false, mutate: vi.fn(), }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsx` around lines 53 - 65, The current mockedUseQuery implementation only branches on init truthiness which is brittle; update the mock (mockedUseQuery.mockImplementation used in the beforeEach for NodeStatusTableRow tests) to switch behavior based on the first argument (the query path) instead of just init, returning the specific { data: { content: 'Deploying production\n' }, isLoading: false, error: undefined, isValidating: false, mutate: vi.fn() } for the exact path NodeStatusTableRow expects and a safe default for other paths so additional useQuery calls won’t accidentally change test behavior.ui/src/lib/executor-utils.ts (1)
76-87: 💤 Low valuePre-compile the ANSI regex once.
new RegExp(ANSI_CODES_REGEX, 'g')is rebuilt on every call toformatLogStepOutput. Since the source is a hardcoded constant, hoisting the compiledRegExpto module scope avoids the per-call recompilation. (Theast-grepReDoS hint is a false positive — the pattern isn't user-controlled.)Proposed fix
-const ANSI_CODES_REGEX = [ +const ANSI_CODES_PATTERN = [ '[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]+)*|[a-zA-Z\\d]+(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?\\u0007)', '(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PR-TZcf-nq-uy=><~]))', ].join('|'); +const ANSI_CODES_REGEX = new RegExp(ANSI_CODES_PATTERN, 'g'); export function formatLogStepOutput(content: string): string { return content - .replace(new RegExp(ANSI_CODES_REGEX, 'g'), '') + .replace(ANSI_CODES_REGEX, '') .replace(/\r\n/g, '\n') .replace(/\r/g, '\n') .replace(/\n+$/, ''); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/lib/executor-utils.ts` around lines 76 - 87, The ANSI regex is being recompiled on every call to formatLogStepOutput; hoist its compiled RegExp to module scope by creating a constant (e.g., ANSI_CODES_RE or ANSI_CODES_REGEX_RE) initialized with new RegExp(ANSI_CODES_REGEX, 'g') and then update formatLogStepOutput to use that precompiled RegExp instead of calling new RegExp(...) each time; keep the existing ANSI_CODES_REGEX string constant unchanged and only replace the RegExp construction site.ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx (1)
199-201: 💤 Low valueDrop unnecessary nullable accesses on the required
dagRunprop.
Props.dagRunis typed as non-optional, sodagRun?.name(line 221) and the|| ''fallback fordagRun.rootDAGRunIdon line 200 are defensive but redundant — and thewhenEnabledgating already ensuresrootDAGRunId/rootDAGRunNameare truthy strings before the sub-DAG query fires. Minor cleanup; aligns with the rest of the file (e.g. line 148 destructuresdagRundirectly).Also applies to: 221-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx` around lines 199 - 201, Remove the redundant nullable checks on the required dagRun prop in NodeStatusTableRow: replace uses like dagRun?.name and dagRun.rootDAGRunId || '' with direct accesses (dagRun.name, dagRun.rootDAGRunId) since Props.dagRun is non-optional and the whenEnabled gating already guarantees rootDAGRunId/rootDAGRunName are truthy before the sub-DAG query runs; update the JSX/logic in NodeStatusTableRow where dagRun is destructured and where rootDAGRunId/rootDAGRunName are referenced to use direct property access without optional chaining or empty-string fallbacks.ui/src/features/dags/components/dag-details/LogStepMessage.tsx (1)
65-65: 💤 Low valueConsider truncating
aria-labelfor long messages.
aria-label={Log message: ${formatSingleLine(message)}}will read out the entire message — including very long or multiline content — to assistive tech users on every focus. Consider using the truncated single-line variant (or a short label) to keep the announcement concise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-details/LogStepMessage.tsx` at line 65, The ARIA label on the LogStepMessage component currently uses the full formatted message (aria-label={`Log message: ${formatSingleLine(message)}`}), which can be excessively long; change it to a concise/truncated variant by computing a shortLabel (e.g., call formatSingleLine(message) then truncate to a safe max length like 100–150 chars and append an ellipsis) and use that in aria-label (`aria-label={`Log message: ${shortLabel}`}`) so assistive tech receives a brief, single-line announcement; implement the truncation where LogStepMessage renders the label (or add a small helper truncate function referenced by LogStepMessage).internal/runtime/builtin/log/log.go (1)
61-70: 💤 Low valueRun ignores
ctxcancellation.For a one-shot
io.WriteStringthis is rarely problematic, but for parity with other executors and to avoid emitting output after a step has been cancelled, consider an earlyctx.Err()check before writing.Optional fix
-func (e *logExecutor) Run(_ context.Context) error { +func (e *logExecutor) Run(ctx context.Context) error { + if err := ctx.Err(); err != nil { + return err + } if _, err := io.WriteString(e.stdout, e.message); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runtime/builtin/log/log.go` around lines 61 - 70, The Run method of logExecutor should respect ctx cancellation: before performing the initial io.WriteString to e.stdout check if ctx.Err() != nil and return that error if set, and likewise check ctx.Err() again before writing the final "\n" so you don't emit output after cancellation; keep existing behavior and error returns from io.WriteString on success paths. Reference the logExecutor.Run method and fields e.stdout and e.message when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmn/schema/dag.schema.json`:
- Around line 1977-1989: The schema allows { "type": "log" } to validate without
the required message because the "if" lacks required:["type"] and the "then"
only defines properties for "with"/"config" but doesn't require them; update the
"if" blocks (the ones surrounding the "type": "log" condition used by step and
executorObject) to include "required": ["type"], and change the corresponding
"then" to enforce that either "with" or deprecated "config" must be present
(e.g., add a oneOf/anyOf with [{"required":["with"]},{"required":["config"]}])
while keeping their $ref to "#/definitions/logExecutorConfig" so the required
message field in logExecutorConfig is enforced; apply the same change to the
other occurrence around lines 5369-5376.
In `@ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx`:
- Around line 187-241: logStepDisplayMessage currently returns 'Loading log
output...' whenever shouldFetchLogStepOutput is true and logOutputContent isn't
a string; change it to fall back to the static logMessage when the corresponding
query has errored. Inspect subDAGLogQuery.error and dagRunLogQuery.error (based
on isSubDAGRun) and, if an error is present, return logMessage instead of the
loading text; otherwise preserve the existing logic that uses
formatLogStepOutput(logOutputContent) for string content and keeps the loading
text while the query is pending.
---
Outside diff comments:
In `@internal/core/spec/step_test.go`:
- Around line 21-62: TestMain is missing registration for the "log" executor;
add a call to core.RegisterExecutorCapabilities("log",
core.ExecutorCapabilities{}) in TestMain ( alongside the existing registrations
like "mail" ) so the "log" type is explicitly recognized by tests and future
capability validation (use the same empty capabilities as "mail" unless log
should have specific flags).
---
Nitpick comments:
In `@internal/runtime/builtin/log/log.go`:
- Around line 61-70: The Run method of logExecutor should respect ctx
cancellation: before performing the initial io.WriteString to e.stdout check if
ctx.Err() != nil and return that error if set, and likewise check ctx.Err()
again before writing the final "\n" so you don't emit output after cancellation;
keep existing behavior and error returns from io.WriteString on success paths.
Reference the logExecutor.Run method and fields e.stdout and e.message when
making the changes.
In
`@ui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsx`:
- Around line 115-122: Remove the brittle Tailwind-specific assertion in the
NodeStatusTableRow test: delete the expect(message).toHaveClass('w-[320px]')
line in the test (the variable is message from getByLabelText('Log message:
Deploying production')) so the test relies on semantic behavior only (presence,
absence of SVG, no interpolated text), not a concrete CSS utility class.
- Around line 53-65: The current mockedUseQuery implementation only branches on
init truthiness which is brittle; update the mock
(mockedUseQuery.mockImplementation used in the beforeEach for NodeStatusTableRow
tests) to switch behavior based on the first argument (the query path) instead
of just init, returning the specific { data: { content: 'Deploying production\n'
}, isLoading: false, error: undefined, isValidating: false, mutate: vi.fn() }
for the exact path NodeStatusTableRow expects and a safe default for other paths
so additional useQuery calls won’t accidentally change test behavior.
In `@ui/src/features/dags/components/dag-details/LogStepMessage.tsx`:
- Line 65: The ARIA label on the LogStepMessage component currently uses the
full formatted message (aria-label={`Log message:
${formatSingleLine(message)}`}), which can be excessively long; change it to a
concise/truncated variant by computing a shortLabel (e.g., call
formatSingleLine(message) then truncate to a safe max length like 100–150 chars
and append an ellipsis) and use that in aria-label (`aria-label={`Log message:
${shortLabel}`}`) so assistive tech receives a brief, single-line announcement;
implement the truncation where LogStepMessage renders the label (or add a small
helper truncate function referenced by LogStepMessage).
In `@ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx`:
- Around line 199-201: Remove the redundant nullable checks on the required
dagRun prop in NodeStatusTableRow: replace uses like dagRun?.name and
dagRun.rootDAGRunId || '' with direct accesses (dagRun.name,
dagRun.rootDAGRunId) since Props.dagRun is non-optional and the whenEnabled
gating already guarantees rootDAGRunId/rootDAGRunName are truthy before the
sub-DAG query runs; update the JSX/logic in NodeStatusTableRow where dagRun is
destructured and where rootDAGRunId/rootDAGRunName are referenced to use direct
property access without optional chaining or empty-string fallbacks.
In `@ui/src/lib/executor-utils.ts`:
- Around line 76-87: The ANSI regex is being recompiled on every call to
formatLogStepOutput; hoist its compiled RegExp to module scope by creating a
constant (e.g., ANSI_CODES_RE or ANSI_CODES_REGEX_RE) initialized with new
RegExp(ANSI_CODES_REGEX, 'g') and then update formatLogStepOutput to use that
precompiled RegExp instead of calling new RegExp(...) each time; keep the
existing ANSI_CODES_REGEX string constant unchanged and only replace the RegExp
construction site.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 423d2c4c-c6a6-4bc3-93cc-b35c9f685793
📒 Files selected for processing (17)
internal/cmn/schema/dag.schema.jsoninternal/core/spec/step_test.gointernal/core/spec/step_types.gointernal/runtime/builtin/builtin.gointernal/runtime/builtin/log/log.gointernal/runtime/builtin/log/log_test.gointernal/runtime/node_internal_test.goui/src/features/dags/components/dag-details/DAGStepTableRow.tsxui/src/features/dags/components/dag-details/LogStepMessage.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-details/SubDAGRunsList.tsxui/src/features/dags/components/dag-details/__tests__/DAGStepTableRow.test.tsxui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsxui/src/features/dags/components/step-details/StepDetails.tsxui/src/features/dags/components/step-details/__tests__/StepDetails.test.tsxui/src/lib/__tests__/executor-utils.test.tsui/src/lib/executor-utils.ts
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
logstep type that writes its resolved message to stdoutlogin the DAG spec/schema/runtimeTesting
go test ./internal/runtime/builtin ./internal/runtime/builtin/log ./internal/core/spec ./internal/runtime ./internal/cmn/schema -count=1pnpm exec vitest run src/lib/__tests__/executor-utils.test.ts src/features/dags/components/dag-details/__tests__/DAGStepTableRow.test.tsx src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsx src/features/dags/components/step-details/__tests__/StepDetails.test.tsxpnpm typecheckpnpm buildgit diff --checkSummary by CodeRabbit
New Features
Tests