Copilot commented on code in PR #64010:
URL: https://github.com/apache/airflow/pull/64010#discussion_r3025335698
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -66,7 +71,39 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string):
Array<ColumnDef<DAGRunResponse>> => [
+const runColumns = (
+ translate: TFunction,
+ dagId?: string,
+ rowSelectionParams?: Omit<GetColumnsParams, "multiTeam">,
+): Array<ColumnDef<DAGRunResponse>> => [
+ ...(rowSelectionParams
+ ? [
+ {
+ accessorKey: "select",
+ cell: ({ row }: DagRunRow) => (
+ <Checkbox
+ borderWidth={1}
+
checked={rowSelectionParams.selectedRows.get(row.original.dag_run_id)}
+ colorPalette="brand"
+ onCheckedChange={(event) =>
+ rowSelectionParams.onRowSelect(row.original.dag_run_id,
Boolean(event.checked))
+ }
Review Comment:
Row selection is using `dag_run_id` as the key for `selectedRows.get(...)`
and `onRowSelect(...)`, but `useRowSelection` is configured with a composite
key (`${run.dag_id}:${run.dag_run_id}`). This mismatch will prevent checkboxes
from reflecting selection state and can cause collisions across DAGs. Use the
same composite key here (ideally via a shared `getKey` helper) for both reading
and writing selection state.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -280,6 +328,35 @@ export const DagRuns = () => {
onStateChange={setTableURLState}
total={data?.total_entries}
/>
+ <ActionBar.Root closeOnInteractOutside={false}
open={Boolean(selectedRows.size)}>
+ <ActionBar.Content>
+ <ActionBar.SelectionTrigger>
+ {selectedRows.size} {translate("common:selected")}
+ </ActionBar.SelectionTrigger>
+ <ActionBar.Separator />
+
+ <Button
+ colorPalette="blue"
+ onClick={() => {
+ [...selectedRows.keys()].forEach((key) => {
+ const [selectedDagId, dagRunId] = key.split(":");
+
+ clearDagRun({
+ dagId: selectedDagId,
+ dagRunId,
+ requestBody: { dry_run: false, only_failed: false },
+ });
+ });
+ clearSelections();
+ }}
+ size="sm"
+ variant="outline"
+ >
+ {translate("dags:runAndTaskActions.clear.button", { type:
translate("dagRun_other") })}
+ </Button>
Review Comment:
`Button` is used in the ActionBar but is not imported in this file, which
will fail compilation. Import `Button` from the appropriate UI package (likely
`@chakra-ui/react`, consistent with other usage in this file).
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -280,6 +328,35 @@ export const DagRuns = () => {
onStateChange={setTableURLState}
total={data?.total_entries}
/>
+ <ActionBar.Root closeOnInteractOutside={false}
open={Boolean(selectedRows.size)}>
+ <ActionBar.Content>
+ <ActionBar.SelectionTrigger>
+ {selectedRows.size} {translate("common:selected")}
+ </ActionBar.SelectionTrigger>
+ <ActionBar.Separator />
+
+ <Button
+ colorPalette="blue"
+ onClick={() => {
+ [...selectedRows.keys()].forEach((key) => {
+ const [selectedDagId, dagRunId] = key.split(":");
+
+ clearDagRun({
+ dagId: selectedDagId,
+ dagRunId,
+ requestBody: { dry_run: false, only_failed: false },
+ });
Review Comment:
The bulk action parses selected keys with `key.split(":")`, but `dag_run_id`
commonly contains `:` (e.g. ISO timestamps in scheduled run ids), so this will
corrupt `selectedDagId`/`dagRunId`. Store selected values in a
structured/escaped form (e.g. JSON stringified tuple) or split only on the
first delimiter after encoding components.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -280,6 +328,35 @@ export const DagRuns = () => {
onStateChange={setTableURLState}
total={data?.total_entries}
/>
+ <ActionBar.Root closeOnInteractOutside={false}
open={Boolean(selectedRows.size)}>
+ <ActionBar.Content>
+ <ActionBar.SelectionTrigger>
+ {selectedRows.size} {translate("common:selected")}
+ </ActionBar.SelectionTrigger>
+ <ActionBar.Separator />
+
+ <Button
+ colorPalette="blue"
+ onClick={() => {
+ [...selectedRows.keys()].forEach((key) => {
+ const [selectedDagId, dagRunId] = key.split(":");
+
+ clearDagRun({
+ dagId: selectedDagId,
+ dagRunId,
+ requestBody: { dry_run: false, only_failed: false },
+ });
+ });
+ clearSelections();
+ }}
+ size="sm"
+ variant="outline"
+ >
+ {translate("dags:runAndTaskActions.clear.button", { type:
translate("dagRun_other") })}
+ </Button>
+ <ActionBar.CloseTrigger onClick={clearSelections} />
Review Comment:
This ActionBar currently only exposes a bulk "Clear" action, but the PR
description states bulk actions for Mark Success and Mark Failed as well.
Either add the missing bulk actions or update the PR description/scope to match
what’s implemented here.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -280,6 +328,35 @@ export const DagRuns = () => {
onStateChange={setTableURLState}
total={data?.total_entries}
/>
+ <ActionBar.Root closeOnInteractOutside={false}
open={Boolean(selectedRows.size)}>
+ <ActionBar.Content>
+ <ActionBar.SelectionTrigger>
+ {selectedRows.size} {translate("common:selected")}
+ </ActionBar.SelectionTrigger>
+ <ActionBar.Separator />
+
+ <Button
+ colorPalette="blue"
+ onClick={() => {
+ [...selectedRows.keys()].forEach((key) => {
+ const [selectedDagId, dagRunId] = key.split(":");
+
+ clearDagRun({
+ dagId: selectedDagId,
+ dagRunId,
+ requestBody: { dry_run: false, only_failed: false },
+ });
+ });
+ clearSelections();
Review Comment:
Bulk clear calls the generated `useDagRunServiceClearDagRun` mutation
directly, bypassing the existing `useClearDagRun` wrapper (toasts + query
invalidation) used by the per-row Clear dialog. Without equivalent
success/error handling and invalidation, the UI may not refresh correctly and
users won’t get feedback for partial failures; consider introducing a
bulk-clear wrapper that invalidates relevant queries once and reports errors
before clearing selections.
```suggestion
onClick={async () => {
const dagRunKeys = [...selectedRows.keys()];
const results = await Promise.allSettled(
dagRunKeys.map((key) => {
const [selectedDagId, dagRunId] = key.split(":");
return clearDagRun({
dagId: selectedDagId,
dagRunId,
requestBody: { dry_run: false, only_failed: false },
});
}),
);
const failedCount = results.filter((result) => result.status
=== "rejected").length;
const totalCount = results.length;
if (failedCount > 0) {
// Provide simple feedback for partial/total failures
without relying on additional hooks.
window.alert(
`${failedCount} of ${totalCount} DAG runs failed to clear.
Check logs or individual runs for details.`,
);
}
// Only clear selections if at least one clear succeeded.
if (failedCount < totalCount) {
clearSelections();
}
```
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -280,6 +328,35 @@ export const DagRuns = () => {
onStateChange={setTableURLState}
total={data?.total_entries}
/>
+ <ActionBar.Root closeOnInteractOutside={false}
open={Boolean(selectedRows.size)}>
+ <ActionBar.Content>
+ <ActionBar.SelectionTrigger>
+ {selectedRows.size} {translate("common:selected")}
+ </ActionBar.SelectionTrigger>
+ <ActionBar.Separator />
+
+ <Button
+ colorPalette="blue"
+ onClick={() => {
+ [...selectedRows.keys()].forEach((key) => {
+ const [selectedDagId, dagRunId] = key.split(":");
+
+ clearDagRun({
+ dagId: selectedDagId,
+ dagRunId,
+ requestBody: { dry_run: false, only_failed: false },
+ });
+ });
+ clearSelections();
+ }}
+ size="sm"
+ variant="outline"
+ >
+ {translate("dags:runAndTaskActions.clear.button", { type:
translate("dagRun_other") })}
+ </Button>
+ <ActionBar.CloseTrigger onClick={clearSelections} />
+ </ActionBar.Content>
+ </ActionBar.Root>
Review Comment:
Multi-select and the ActionBar are new user-facing behavior here, but there
are no tests covering selecting rows (including select-all) and triggering a
bulk action. Add UI tests asserting checkbox selection state and that the
bulk-clear mutation is invoked with the expected dagId/dagRunId pairs.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -75,7 +112,7 @@ const runColumns = (translate: TFunction, dagId?: string):
Array<ColumnDef<DAGRu
cell: ({ row: { original } }: DagRunRow) => (
<Link asChild color="fg.info">
<RouterLink to={`/dags/${original.dag_id}`}>
- <TruncatedText text={original.dag_display_name} />
+ <TruncatedText text={original.dag_display_name} />{" "}
Review Comment:
There’s an extra JSX whitespace literal (`{" "}`) after `TruncatedText`
inside the DAG link. This adds a trailing space in the rendered link text and
looks accidental; remove it unless it’s intentionally needed for layout.
```suggestion
<TruncatedText text={original.dag_display_name} />
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
