Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReschedule and retry flows were changed to support loading DAG specs from on-disk source files, propagate a new DAG.SourceFile provenance field through task dispatch, and enqueue rescheduled runs via the queue system with a config guard requiring queues enabled. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as API Server
participant Config as Config
participant Store as DagRunStore
participant Loader as Spec Loader
participant Queue as Queue System
participant Executor as Executor
User->>API: POST /api/v1/dag-runs/{name}/{id}/reschedule
API->>Config: Check queues.enabled
alt queues disabled
Config-->>API: false
API-->>User: 400 BadRequest
else queues enabled
API->>Store: FindAttempt / LatestAttempt
Store-->>API: attempt (includes dag.SourceFile)
API->>Loader: if useCurrentDagFile -> load from disk(sourceFile)
alt useCurrentDagFile
Loader-->>API: DAG (SourceFile set)
else use snapshot
Loader-->>API: DAG built from stored YAML
end
API->>Queue: enqueueDAGRun(ctx, trigger=manual, options include SourceFile)
Queue-->>API: queued=true, dagRunId
API-->>User: 200 OK + queued status
Queue->>Executor: deliver queued task (includes SourceFile)
Executor-->>Queue: execute run (uses SourceFile for provenance)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/frontend/api/v1/dagruns.go (1)
1537-1542:⚠️ Potential issue | 🔴 CriticalGuard the
latestpath against a nil status before dereferencing it.
ReadStatus()is already treated as nullable later in this file insiderescheduleDAGRun. If the latest attempt exists before its first status snapshot is written,*statuswill panic this handler.💡 Suggested change
status, err := attempt.ReadStatus(ctx) if err != nil { return api.GetDAGRunDetails200JSONResponse{}, fmt.Errorf("error getting latest status: %w", err) } + if status == nil { + return api.GetDAGRunDetails200JSONResponse{}, fmt.Errorf("latest dag-run status is unavailable for DAG %s", dagName) + } return api.GetDAGRunDetails200JSONResponse{ DagRunDetails: a.toDAGRunDetailsWithSpecSource(ctx, attempt, *status), }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dagruns.go` around lines 1537 - 1542, The handler currently dereferences status returned by attempt.ReadStatus and can panic if that status is nil; add a nil check after calling ReadStatus() (which may return nil) and handle the "latest" path without dereferencing: if status == nil, return api.GetDAGRunDetails200JSONResponse with DagRunDetails produced for the attempt when no status exists (e.g., call or create the same DAG run details path used elsewhere for nil statuses or pass an explicit empty/zero status), otherwise continue to call a.toDAGRunDetailsWithSpecSource(ctx, attempt, *status). Ensure you reference attempt.ReadStatus, a.toDAGRunDetailsWithSpecSource, and the GetDAGRunDetails200JSONResponse return to locate the change.
🧹 Nitpick comments (3)
internal/cmd/helper.go (1)
116-116: Remove redundantfresh.SourceFileassignment.
freshis not returned, anddag.SourceFileis not overwritten later, so this write has no effect.♻️ Proposed cleanup
- fresh.SourceFile = dag.SourceFile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/helper.go` at line 116, Remove the redundant assignment to fresh.SourceFile: delete the statement "fresh.SourceFile = dag.SourceFile" in internal/cmd/helper.go (leave fresh and dag usages unchanged); if the original intent was to propagate SourceFile into an object that will be returned, instead ensure that fresh is returned or that the destination field is used—otherwise simply remove the assignment since it has no effect.ui/src/features/dag-runs/components/common/DAGRunActions.tsx (1)
98-148: Consider logging errors in the catch block for debuggability.The error is silently swallowed, which could make troubleshooting difficult if the DAG run details fetch fails. Consider adding a
console.erroror similar logging.The useEffect correctly handles cancellation to prevent state updates on unmounted components, and includes
remoteNodein the API call as required.♻️ Optional: Add error logging
.catch(() => { if (cancelled) { return; } + console.error('Failed to fetch DAG run details for reschedule source'); setSpecFromFile(false); setUseCurrentDagFile(false); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dag-runs/components/common/DAGRunActions.tsx` around lines 98 - 148, The catch block in the React.useEffect that calls client.GET for '/dag-runs/{name}/{dagRunId}' silently swallows errors; update that catch handler to log the error (e.g., via console.error or processLogger) while preserving the existing cancelled checks and state updates (setSpecFromFile(false), setUseCurrentDagFile(false), setRescheduleSourceLoading(false) in finally); locate the effect by the React.useEffect wrapper, the client.GET call, and the setSpecFromFile/setUseCurrentDagFile state setters to add the logging without changing the cancellation logic.ui/src/features/dags/components/common/DAGActions.tsx (1)
119-150: Prefer the sameasync/awaitflow used elsewhere in this component.This new request path is harder to compare with the
asyncIIFE below because success, failure, and cleanup are split across.then/.catch/.finally. Converting it toasync/await+try/catch/finallywould keep the modal fetch logic consistent.As per coding guidelines
Prefer async/await over .then() for promise handlingandUse error handling with try/catch for async operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dags/components/common/DAGActions.tsx` around lines 119 - 150, Convert the client.GET(...).then/.catch/.finally chain into the same async/await + try/catch/finally pattern used elsewhere: create an async function or IIFE that awaits client.GET('/dag-runs/{name}/{dagRunId}', { params: { path: { name: status.name, dagRunId: retryDagRunId }, query: { remoteNode: appBarContext.selectedRemoteNode || 'local' } } }), then inside try check cancelled before using response.data to call setSpecFromFile and setUseCurrentDagFile; in catch check cancelled before setting both to false; and in finally check cancelled before calling setRescheduleSourceLoading(false). Ensure all references to cancelled, setSpecFromFile, setUseCurrentDagFile, and setRescheduleSourceLoading remain and preserve the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/frontend/api/v1/dagruns_test.go`:
- Around line 1331-1334: The test currently treats a nil SpecFromFile pointer as
equivalent to false; change it to assert the field is present first by requiring
body.DagRunDetails.SpecFromFile is not nil (use require.NotNil or
require.Assert) and then dereference it to compare its boolean value to want
(i.e., require.NotNil(t, body.DagRunDetails.SpecFromFile) followed by
require.Equal(t, want, *body.DagRunDetails.SpecFromFile)), referencing body and
DagRunDetails.SpecFromFile and the existing want/got comparison.
In `@ui/src/features/dags/components/common/DAGActions.tsx`:
- Around line 135-137: The code currently sets both setSpecFromFile and
setUseCurrentDagFile from data?.dagRunDetails?.specFromFile causing file-backed
DAGs to default to using the on-disk file; instead preserve the snapshot-default
behavior by only initializing the specFromFile state from
data?.dagRunDetails?.specFromFile and do NOT change useCurrentDagFile from this
value—remove the setUseCurrentDagFile(available) call (or explicitly
setUseCurrentDagFile(false) if you must initialize) so useCurrentDagFile remains
controlled by the user's checkbox rather than the specFromFile flag; update the
block that references data?.dagRunDetails?.specFromFile, setSpecFromFile, and
setUseCurrentDagFile accordingly.
---
Outside diff comments:
In `@internal/service/frontend/api/v1/dagruns.go`:
- Around line 1537-1542: The handler currently dereferences status returned by
attempt.ReadStatus and can panic if that status is nil; add a nil check after
calling ReadStatus() (which may return nil) and handle the "latest" path without
dereferencing: if status == nil, return api.GetDAGRunDetails200JSONResponse with
DagRunDetails produced for the attempt when no status exists (e.g., call or
create the same DAG run details path used elsewhere for nil statuses or pass an
explicit empty/zero status), otherwise continue to call
a.toDAGRunDetailsWithSpecSource(ctx, attempt, *status). Ensure you reference
attempt.ReadStatus, a.toDAGRunDetailsWithSpecSource, and the
GetDAGRunDetails200JSONResponse return to locate the change.
---
Nitpick comments:
In `@internal/cmd/helper.go`:
- Line 116: Remove the redundant assignment to fresh.SourceFile: delete the
statement "fresh.SourceFile = dag.SourceFile" in internal/cmd/helper.go (leave
fresh and dag usages unchanged); if the original intent was to propagate
SourceFile into an object that will be returned, instead ensure that fresh is
returned or that the destination field is used—otherwise simply remove the
assignment since it has no effect.
In `@ui/src/features/dag-runs/components/common/DAGRunActions.tsx`:
- Around line 98-148: The catch block in the React.useEffect that calls
client.GET for '/dag-runs/{name}/{dagRunId}' silently swallows errors; update
that catch handler to log the error (e.g., via console.error or processLogger)
while preserving the existing cancelled checks and state updates
(setSpecFromFile(false), setUseCurrentDagFile(false),
setRescheduleSourceLoading(false) in finally); locate the effect by the
React.useEffect wrapper, the client.GET call, and the
setSpecFromFile/setUseCurrentDagFile state setters to add the logging without
changing the cancellation logic.
In `@ui/src/features/dags/components/common/DAGActions.tsx`:
- Around line 119-150: Convert the client.GET(...).then/.catch/.finally chain
into the same async/await + try/catch/finally pattern used elsewhere: create an
async function or IIFE that awaits client.GET('/dag-runs/{name}/{dagRunId}', {
params: { path: { name: status.name, dagRunId: retryDagRunId }, query: {
remoteNode: appBarContext.selectedRemoteNode || 'local' } } }), then inside try
check cancelled before using response.data to call setSpecFromFile and
setUseCurrentDagFile; in catch check cancelled before setting both to false; and
in finally check cancelled before calling setRescheduleSourceLoading(false).
Ensure all references to cancelled, setSpecFromFile, setUseCurrentDagFile, and
setRescheduleSourceLoading remain and preserve the same logic.
🪄 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: ab01d66d-0eea-4c87-8fff-e31087af3862
⛔ Files ignored due to path filters (1)
proto/coordinator/v1/coordinator.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
api/v1/api.gen.goapi/v1/api.yamlinternal/cmd/exec_spec.gointernal/cmd/helper.gointernal/cmd/start.gointernal/core/dag.gointernal/core/spec/loader.gointernal/core/spec/loader_test.gointernal/intg/reschedule_inline_api_test.gointernal/runtime/executor/dag_runner.gointernal/runtime/executor/task.gointernal/runtime/executor/task_test.gointernal/runtime/subcmd.gointernal/runtime/subcmd_test.gointernal/service/coordinator/handler.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/dags.gointernal/service/scheduler/dag_executor.gointernal/service/worker/handler.gointernal/service/worker/remote_handler.goproto/coordinator/v1/coordinator.protoui/src/api/v1/schema.tsui/src/features/dag-runs/components/common/DAGRunActions.tsxui/src/features/dags/components/common/DAGActions.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1966 +/- ##
==========================================
- Coverage 68.13% 68.10% -0.03%
==========================================
Files 475 475
Lines 61389 61413 +24
==========================================
- Hits 41829 41827 -2
- Misses 15588 15612 +24
- Partials 3972 3974 +2
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This was referenced Apr 10, 2026
This was referenced Apr 21, 2026
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
Testing
Summary by CodeRabbit
New Features
Bug Fixes