Copilot commented on code in PR #60718: URL: https://github.com/apache/airflow/pull/60718#discussion_r3066502704
########## 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 })}) Review Comment: The translation call `translate("dagRun", { count })` is likely incorrect: in `public/i18n/locales/*/common.json`, `dagRun` is a nested object (e.g. for column labels), not a pluralizable string key. This will render as the key name (or `[object Object]`) instead of “Dag Run(s)”. Use the existing plural keys like `dagRun_one` / `dagRun_other` (or an existing pluralized resourceName pattern) for the count label. ```suggestion ({count} {count === 1 ? translate("dagRun_one") : translate("dagRun_other")}) ``` ########## 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>; +}; + +const runColumns = ( + translate: TFunction, + dagId?: string, + selection?: SelectionConfig, +): Array<ColumnDef<DAGRunResponse>> => [ + ...(selection + ? [ + { + cell: ({ row: { original } }: DagRunRow) => { + const key = runKey({ dagId: original.dag_id, dagRunId: original.dag_run_id }); + + return ( + <Checkbox + borderWidth={1} + checked={selection.selectedRows.get(key)} + colorPalette="brand" + onCheckedChange={(event) => selection.onRowSelect(key, Boolean(event.checked))} + /> + ); + }, + enableSorting: false, + header: () => ( + <Checkbox + borderWidth={1} + checked={selection.allRowsSelected} + colorPalette="brand" + onCheckedChange={(event) => selection.onSelectAll(Boolean(event.checked))} + /> + ), + id: "select", + meta: { skeletonWidth: 2 }, + } satisfies ColumnDef<DAGRunResponse>, + ] Review Comment: The selection checkbox column is missing `enableHiding: false` (and uses `id` instead of the established `accessorKey: "select"` pattern). Since `DataTable` enables column hiding globally, users can hide this column and lose the ability to change selections. Consider matching the existing selection-column pattern used in e.g. `pages/Connections/Connections.tsx` and `pages/Variables/Variables.tsx` by setting `accessorKey: "select"`, `enableHiding: false`, and `enableSorting: false`. ########## 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", + }); + + return; + } + + toaster.create({ + description: translate("dags:runAndTaskActions.delete.success.description", { + type: translate("dagRun_one"), + }), + title: translate("dags:runAndTaskActions.delete.success.title", { type: translate("dagRun_one") }), + type: "success", Review Comment: The bulk-failure toast text is not localized (`"${failed.length}/${runs.length} failed"`) and the toast titles/descriptions use `dagRun_one` even though multiple runs may be deleted. Please switch to existing i18n keys (e.g. `common:toaster.bulkDelete.*` or a new `dags:*` bulk-delete key) and use the plural resource name (`dagRun_other`) and/or include the counts so the message is accurate. ########## 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: `runKey`/`parseRunKey` rely on a `"__"` delimiter to round-trip `{ dagId, dagRunId }`. This breaks if `dagId` itself contains `"__"` (DAG IDs are user-defined strings), leading to deletes being issued against the wrong DAG. Prefer an encoding that can’t collide (e.g. `JSON.stringify`/`JSON.parse`, or base64/URL-encoding of each part with an unambiguous separator). ```suggestion const runKey = (run: SelectedRun) => JSON.stringify({ dagId: run.dagId, dagRunId: run.dagRunId }); const parseRunKey = (key: string): SelectedRun => { try { const parsed = JSON.parse(key) as Partial<SelectedRun>; return { dagId: typeof parsed.dagId === "string" ? parsed.dagId : "", dagRunId: typeof parsed.dagRunId === "string" ? parsed.dagRunId : "", }; } catch { return { dagId: "", dagRunId: "" }; } ``` ########## 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", + }); + + return; + } + + toaster.create({ + description: translate("dags:runAndTaskActions.delete.success.description", { + type: translate("dagRun_one"), + }), + title: translate("dags:runAndTaskActions.delete.success.title", { type: translate("dagRun_one") }), + type: "success", + }); + + clearSelections?.(); + onSuccessConfirm?.(); + }; + + return { + error, + isPending: deleteMutation.isPending, Review Comment: `useDagRunServiceDeleteDagRun()` returns a single React Query mutation instance, but `mutateAsync` is invoked concurrently for every selected run. Mutation state (notably `isPending`) only tracks the most recently started mutation, so the UI loading state can flip to false while earlier deletions are still in flight. Consider implementing this as one mutation that accepts the array of runs (and internally runs deletes with a concurrency limit or sequentially), and manage a dedicated `isPending` state for the bulk operation. ```suggestion const [isPending, setIsPending] = useState(false); const mutate = async (runs: Array<SelectedRun>): Promise<void> => { if (runs.length === 0) { return; } setError(undefined); setIsPending(true); try { const failed: Array<PromiseRejectedResult> = []; for (const { dagId, dagRunId } of runs) { try { await deleteMutation.mutateAsync({ dagId, dagRunId }); } catch (reason) { failed.push({ reason, 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", }); return; } toaster.create({ description: translate("dags:runAndTaskActions.delete.success.description", { type: translate("dagRun_one"), }), title: translate("dags:runAndTaskActions.delete.success.title", { type: translate("dagRun_one") }), type: "success", }); clearSelections?.(); onSuccessConfirm?.(); } finally { setIsPending(false); } }; return { error, isPending, ``` -- 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]
