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:
📝 WalkthroughWalkthroughThe PR extends the DAG run API to include artifact availability tracking. The backend now sets an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/service/frontend/api/v1/transformer_test.go (1)
23-34: Consider adding a negative-case assertion forArtifactsAvailable.The current test only validates the
truebranch (non-emptyArchiveDir). A symmetric assertion for a run with emptyArchiveDir(e.g., inTestToDAGRunSummaryOmitsAutoRetryLimitWhenUnconfiguredor a new dedicated test) would lock in thefalsebranch and guard against regressions in the mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/transformer_test.go` around lines 23 - 34, Add a negative-case assertion that verifies ArtifactsAvailable is false when ArchiveDir is empty by calling toDAGRunSummary with a status that has ArchiveDir set to "" (e.g., in TestToDAGRunSummaryOmitsAutoRetryLimitWhenUnconfigured or a new test in transformer_test.go), assert that summary.ArtifactsAvailable is false, and ensure you still validate other relevant fields (ScheduleTime, AutoRetryCount/Limit) so the test covers both true and false branches of toDAGRunSummary's artifact availability mapping.
🤖 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/transformer.go`:
- Around line 194-212: The ArtifactsAvailable boolean is currently set from
s.ArchiveDir != "" (a configuration flag set at enqueue time) which
misrepresents actual artifact presence; change the mapping in the DAG run
transformers to reflect configuration semantics instead of availability: replace
ArtifactsAvailable with a clearly named field (e.g. ArtifactsEnabled or
ArchiveDirConfigured) in the api.DAGRunSummary and in ToDAGRunDetails, and set
it to (s.ArchiveDir != ""); update the API type references and any callers/UI
usage to check the new name (or, if you prefer the current name, instead compute
it strictly by checking terminal status via s.Status.IsFinished() and doing a
cheap os.Stat/non-empty-dir check before setting ArtifactsAvailable in
toDAGRunsPageResponse and ToDAGRunDetails—choose one approach and apply it
consistently).
In `@ui/src/features/cockpit/components/ArtifactListModal.tsx`:
- Around line 246-294: handleDownload has multiple robustness issues: ensure the
download request is cancellable and guarded, check for empty responses, and use
a DOM-appended anchor with deferred revoke. Specifically, create an
AbortController (reusing the component's cancel pattern or new controller) and
pass its signal to client.GET so the request can be aborted; guard
setDownloadingPath/setDownloadError calls with a mounted/cancelled flag before
updating state; after verifying request.error is falsy, assert request.data
(blob) exists and throw a clear Error('Empty response') if not; append the
created <a> to document.body before link.click() and call
URL.revokeObjectURL(objectUrl) inside a setTimeout(..., 0) (or after a short
delay) to avoid revoking while the download starts; optionally improve
Content-Disposition parsing in handleDownload to handle unquoted filename and
RFC5987 filename* forms before falling back to node.name.
- Around line 180-244: The effect currently sets setIsLoading(true) on every
refresh, which hides stale artifacts; change it to only show the full-page
loading state on the initial load by computing const isInitialLoad =
(tree.length === 0) at the top of the effect and call
setIsLoading(isInitialLoad) instead of always true inside fetchTree, and in the
finally block only call setIsLoading(false) when !cancelled && isInitialLoad;
also add tree to the effect dependency array so the effect sees the current tree
length. Update references in fetchTree/finally accordingly (functions:
fetchTree, setIsLoading, setTree).
In `@ui/src/features/cockpit/components/KanbanCard.tsx`:
- Around line 86-115: The outer interactive wrapper (the <div role="button">
handled by onClick and handleKeyDown in KanbanCard) contains a nested <button>
(the artifacts button rendered when showArtifacts and invoking
onArtifactsClick), which violates ARIA; move the artifacts control out of the
role="button" wrapper so it is not an interactive descendant—either render the
artifacts button as a positioned sibling (e.g., absolutely positioned inside the
card container but outside the clickable div) or change the outer wrapper to a
native <button> and redesign layout so the artifacts control is not nested;
update the layout CSS and keep onClick/handleKeyDown behavior on the card
wrapper while removing the need for event.stopPropagation on the artifacts
button and ensure keyboard focus order and aria-label (aria-label={`View
artifacts for ${run.name}`}) remain correct.
---
Nitpick comments:
In `@internal/service/frontend/api/v1/transformer_test.go`:
- Around line 23-34: Add a negative-case assertion that verifies
ArtifactsAvailable is false when ArchiveDir is empty by calling toDAGRunSummary
with a status that has ArchiveDir set to "" (e.g., in
TestToDAGRunSummaryOmitsAutoRetryLimitWhenUnconfigured or a new test in
transformer_test.go), assert that summary.ArtifactsAvailable is false, and
ensure you still validate other relevant fields (ScheduleTime,
AutoRetryCount/Limit) so the test covers both true and false branches of
toDAGRunSummary's artifact availability mapping.
🪄 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: cc47adc8-6110-42be-bb12-b74da46806b8
📒 Files selected for processing (22)
api/v1/api.gen.goapi/v1/api.yamlinternal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/transformer_test.goui/src/api/v1/schema.tsui/src/features/cockpit/components/ArtifactListModal.tsxui/src/features/cockpit/components/DateKanbanList.tsxui/src/features/cockpit/components/DateKanbanSection.tsxui/src/features/cockpit/components/KanbanBoard.tsxui/src/features/cockpit/components/KanbanCard.tsxui/src/features/cockpit/components/KanbanColumn.tsxui/src/features/cockpit/components/MobileKanbanBoard.tsxui/src/features/cockpit/components/__tests__/ArtifactListModal.test.tsxui/src/features/cockpit/components/__tests__/DateKanbanList.test.tsxui/src/features/cockpit/components/__tests__/DateKanbanSection.test.tsxui/src/features/cockpit/components/__tests__/KanbanCard.test.tsxui/src/features/cockpit/components/__tests__/KanbanCounts.test.tsxui/src/features/dag-runs/components/common/__tests__/DAGRunActions.test.tsxui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunGroupedView.test.tsxui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunTable.test.tsxui/src/features/dag-runs/hooks/__tests__/useBulkDAGRunSelection.test.tsxui/src/features/dags/components/common/__tests__/DAGActions.test.tsx
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
Tests