pierrejeambrun commented on code in PR #60718:
URL: https://github.com/apache/airflow/pull/60718#discussion_r3051608113
##########
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx:
##########
Review Comment:
nit: Rename file + component to `DeleteRunsButton` to be consistant with
other `DeleteVariablesButton` and `
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -58,7 +67,54 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string):
Array<ColumnDef<DAGRunResponse>> => [
+const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`;
+const parseRunKey = (key: string): SelectedRun => {
+ const [dagId = "", ...rest] = key.split("__");
+
+ return { dagId, dagRunId: rest.join("__") };
+};
+
+type SelectionConfig = {
+ allRowsSelected: boolean;
+ onRowSelect: (key: string, selected: boolean) => void;
+ onSelectAll: (selected: boolean) => void;
+ selectedRows: Map<string, boolean>;
+};
Review Comment:
This is alreaady defined in `useRowSelection.ts` we should reuse it.
##########
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx:
##########
@@ -0,0 +1,101 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Button, Flex, Heading, Text, useDisclosure, VStack } from
"@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiTrash2 } from "react-icons/fi";
+
+import { ErrorAlert } from "src/components/ErrorAlert";
+import { Dialog } from "src/components/ui";
+import { useBulkDeleteDagRuns, type SelectedRun } from
"src/queries/useBulkDeleteDagRuns";
+
+type BulkDeleteRunsButtonProps = {
+ readonly clearSelections: VoidFunction;
+ readonly selectedRuns: Array<SelectedRun>;
+};
+
+const BulkDeleteRunsButton = ({ clearSelections, selectedRuns }:
BulkDeleteRunsButtonProps) => {
+ const { onClose, onOpen, open } = useDisclosure();
+ const { t: translate } = useTranslation();
+ const count = selectedRuns.length;
+
+ const { error, isPending, mutate } = useBulkDeleteDagRuns({
+ clearSelections,
+ onSuccessConfirm: onClose,
+ });
+
+ return (
+ <>
+ <Button
+ aria-label={translate("dags:runAndTaskActions.delete.button", { type:
translate("dagRun_other") })}
+ colorPalette="danger"
+ disabled={count === 0}
+ onClick={onOpen}
+ size="sm"
+ variant="outline"
+ >
+ <FiTrash2 />
+ {translate("dags:runAndTaskActions.delete.button", { type:
translate("dagRun_other") })}
+ </Button>
Review Comment:
Not consistent. Here it's just delete:
<img width="444" height="196" alt="Image"
src="https://github.com/user-attachments/assets/1a4bc19c-70ab-4b91-b167-75fb14bd16c8";
/>
<img width="589" height="181" alt="Image"
src="https://github.com/user-attachments/assets/6d36b14c-e798-4353-9b23-0560a8123a3f";
/>
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -236,12 +292,33 @@ export const DagRuns = () => {
query.state.data?.dag_runs.some((run) => isStatePending(run.state)) ?
refetchInterval : false,
},
);
+ const rows = data?.dag_runs ?? [];
- const columns = runColumns(translate, dagId);
+ const { allRowsSelected, clearSelections, handleRowSelect, handleSelectAll,
selectedRows } =
+ useRowSelection({
+ data: rows,
+ getKey: (run) => runKey({ dagId: run.dag_id, dagRunId: run.dag_run_id }),
+ });
+
+ const selectedRuns = useMemo<Array<SelectedRun>>(
Review Comment:
React compiler is taking care of `useMemo` I belive. This isn't needed
anymore.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -58,7 +67,54 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string):
Array<ColumnDef<DAGRunResponse>> => [
+const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`;
+const parseRunKey = (key: string): SelectedRun => {
+ const [dagId = "", ...rest] = key.split("__");
+
+ return { dagId, dagRunId: rest.join("__") };
+};
Review Comment:
What if the dag_id / dag_run_id holds a `__` ?
##########
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx:
##########
@@ -0,0 +1,101 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Button, Flex, Heading, Text, useDisclosure, VStack } from
"@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiTrash2 } from "react-icons/fi";
+
+import { ErrorAlert } from "src/components/ErrorAlert";
+import { Dialog } from "src/components/ui";
+import { useBulkDeleteDagRuns, type SelectedRun } from
"src/queries/useBulkDeleteDagRuns";
+
+type BulkDeleteRunsButtonProps = {
+ readonly clearSelections: VoidFunction;
+ readonly selectedRuns: Array<SelectedRun>;
+};
+
+const BulkDeleteRunsButton = ({ clearSelections, selectedRuns }:
BulkDeleteRunsButtonProps) => {
+ const { onClose, onOpen, open } = useDisclosure();
+ const { t: translate } = useTranslation();
+ const count = selectedRuns.length;
+
+ const { error, isPending, mutate } = useBulkDeleteDagRuns({
+ clearSelections,
+ onSuccessConfirm: onClose,
+ });
+
+ return (
+ <>
+ <Button
+ aria-label={translate("dags:runAndTaskActions.delete.button", { type:
translate("dagRun_other") })}
+ colorPalette="danger"
+ disabled={count === 0}
+ onClick={onOpen}
+ size="sm"
+ variant="outline"
+ >
+ <FiTrash2 />
+ {translate("dags:runAndTaskActions.delete.button", { type:
translate("dagRun_other") })}
+ </Button>
+
+ <Dialog.Root onOpenChange={onClose} open={open} size="xl">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <VStack align="start" gap={4}>
+ <Heading size="xl">
+ {translate("dags:runAndTaskActions.delete.dialog.title", {
+ type: translate("dagRun_other"),
+ })}
+ </Heading>
+ </VStack>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body width="full">
+ <Text color="fg" fontSize="md" fontWeight="semibold" mb={4}>
+ {translate("dags:runAndTaskActions.delete.dialog.warning", {
+ type: translate("dagRun_other"),
+ })}{" "}
+ ({count} {translate("dagRun", { count })})
+ </Text>
+
+ <ErrorAlert error={error} />
+
+ <Flex justifyContent="end" mt={3}>
+ <Button
+ colorPalette="danger"
+ loading={isPending}
+ onClick={() => {
+ void mutate(selectedRuns);
+ }}
+ >
+ <FiTrash2 />{" "}
+ <Text as="span" fontWeight="bold">
+ {translate("common:delete")}
Review Comment:
The confirmation modal isn't consistent with other confirmation modal:
<img width="1282" height="611" alt="Image"
src="https://github.com/user-attachments/assets/8bb89e67-0a3c-4df2-b8fe-0af0ec8e9e6e";
/>
<img width="1099" height="505" alt="Image"
src="https://github.com/user-attachments/assets/d51f5e62-7116-426b-a291-4db378056c4f";
/>
<img width="1328" height="638" alt="Image"
src="https://github.com/user-attachments/assets/332537c4-4ef8-4dce-9330-bfb0762f9481";
/>
##########
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts:
##########
@@ -0,0 +1,97 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueryClient } from "@tanstack/react-query";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+
+import {
+ useDagRunServiceDeleteDagRun,
+ useDagRunServiceGetDagRunsKey,
+ UseDagRunServiceGetDagRunKeyFn,
+ useTaskInstanceServiceGetTaskInstancesKey,
+ useTaskInstanceServiceGetHitlDetailsKey,
+} from "openapi/queries";
+import { toaster } from "src/components/ui";
+
+export type SelectedRun = { dagId: string; dagRunId: string };
+
+type Props = {
+ readonly clearSelections?: VoidFunction;
+ readonly onSuccessConfirm?: VoidFunction;
+};
+
+export const useBulkDeleteDagRuns = ({ clearSelections, onSuccessConfirm }:
Props = {}) => {
+ const { t: translate } = useTranslation();
+ const queryClient = useQueryClient();
+ const deleteMutation = useDagRunServiceDeleteDagRun();
+ const [error, setError] = useState<unknown>(undefined);
+
+ const mutate = async (runs: Array<SelectedRun>): Promise<void> => {
+ if (runs.length === 0) {
+ return;
+ }
+ setError(undefined);
+
+ const results = await Promise.allSettled(
+ runs.map(({ dagId, dagRunId }) => deleteMutation.mutateAsync({ dagId,
dagRunId })),
+ );
+
+ const failed = results.filter((result): result is PromiseRejectedResult =>
result.status === "rejected");
+ const queryKeys = [
+ [useDagRunServiceGetDagRunsKey],
+ [useTaskInstanceServiceGetTaskInstancesKey],
+ [useTaskInstanceServiceGetHitlDetailsKey],
+ ...runs.map(({ dagId, dagRunId }) => UseDagRunServiceGetDagRunKeyFn({
dagId, dagRunId })),
+ ];
+
+ await Promise.all(queryKeys.map((key) => queryClient.invalidateQueries({
queryKey: key })));
+
+ if (failed.length > 0) {
+ const [first] = failed;
+
+ if (first) {
+ setError(first.reason);
+ }
+ toaster.create({
+ description: `${failed.length}/${runs.length} failed`,
+ title: translate("dags:runAndTaskActions.delete.error", { type:
translate("dagRun_one") }),
+ type: "error",
+ });
Review Comment:
When multiple deletions fail, only failed[0].reason is stored in error
state. The toast says ${failed.length}/${runs.length} failed but doesn't say
which ones or why. Consider showing all failed run IDs in the error alert.
--
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]
