Copilot commented on code in PR #62195: URL: https://github.com/apache/airflow/pull/62195#discussion_r3067538195
########## airflow-core/src/airflow/ui/src/pages/Run/DeadlineStatus.tsx: ########## @@ -0,0 +1,192 @@ +/*! + * 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 { Badge, Button, HStack, Text, VStack } from "@chakra-ui/react"; +import dayjs from "dayjs"; +import duration from "dayjs/plugin/duration"; +import relativeTime from "dayjs/plugin/relativeTime"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiAlertTriangle, FiCheck, FiClock } from "react-icons/fi"; + +import { + useDagRunServiceGetDagRun, + useDeadlinesServiceGetDagDeadlineAlerts, + useDeadlinesServiceGetDeadlines, +} from "openapi/queries"; +import type { DeadlineAlertResponse } from "openapi/requests/types.gen"; +import Time from "src/components/Time"; + +import { DeadlineStatusModal } from "./DeadlineStatusModal"; + +dayjs.extend(duration); +dayjs.extend(relativeTime); + +type DeadlineStatusProps = { + readonly dagId: string; + readonly dagRunId: string; +}; + +export const DeadlineStatus = ({ dagId, dagRunId }: DeadlineStatusProps) => { + const { t: translate } = useTranslation("dag"); + const [isModalOpen, setIsModalOpen] = useState(false); + + const { data: deadlineData, isLoading: isLoadingDeadlines } = useDeadlinesServiceGetDeadlines({ + dagId, + dagRunId, + limit: 10, + orderBy: ["deadline_time"], + }); + + // Used to detect whether the DAG has any deadline alerts at all, so we can show "Met" when there are alerts configured but no active deadline instances. + const { data: alertData, isLoading: isLoadingAlerts } = useDeadlinesServiceGetDagDeadlineAlerts({ + dagId, + limit: 100, + }); + + const { data: runData, isLoading: isLoadingRun } = useDagRunServiceGetDagRun({ dagId, dagRunId }); + + const alertMap = new Map<string, DeadlineAlertResponse>(); + + for (const alert of alertData?.deadline_alerts ?? []) { + if (alert.name !== undefined && alert.name !== null && alert.name !== "") { Review Comment: `alertMap` is keyed by `alert.name` and then looked up by `dl.alert_name`. The DB/model does not enforce uniqueness of `DeadlineAlert.name`, so if multiple alerts share a name the map will overwrite entries and deadlines may show the wrong completion rule. A more reliable join key would be the alert UUID; if the deadline instances don’t expose it, consider extending the API to include `deadline_alert_id` (or similar) and key the map by that instead. ```suggestion const alertMap = new Map<string, DeadlineAlertResponse>(); const duplicateAlertNames = new Set<string>(); for (const alert of alertData?.deadline_alerts ?? []) { if (alert.name !== undefined && alert.name !== null && alert.name !== "") { if (duplicateAlertNames.has(alert.name)) { continue; } if (alertMap.has(alert.name)) { alertMap.delete(alert.name); duplicateAlertNames.add(alert.name); continue; } ``` ########## airflow-core/src/airflow/ui/src/pages/Run/DeadlineStatus.tsx: ########## @@ -0,0 +1,192 @@ +/*! + * 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 { Badge, Button, HStack, Text, VStack } from "@chakra-ui/react"; +import dayjs from "dayjs"; +import duration from "dayjs/plugin/duration"; +import relativeTime from "dayjs/plugin/relativeTime"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiAlertTriangle, FiCheck, FiClock } from "react-icons/fi"; + +import { + useDagRunServiceGetDagRun, + useDeadlinesServiceGetDagDeadlineAlerts, + useDeadlinesServiceGetDeadlines, +} from "openapi/queries"; +import type { DeadlineAlertResponse } from "openapi/requests/types.gen"; +import Time from "src/components/Time"; + +import { DeadlineStatusModal } from "./DeadlineStatusModal"; + +dayjs.extend(duration); +dayjs.extend(relativeTime); + +type DeadlineStatusProps = { + readonly dagId: string; + readonly dagRunId: string; +}; + +export const DeadlineStatus = ({ dagId, dagRunId }: DeadlineStatusProps) => { + const { t: translate } = useTranslation("dag"); + const [isModalOpen, setIsModalOpen] = useState(false); + + const { data: deadlineData, isLoading: isLoadingDeadlines } = useDeadlinesServiceGetDeadlines({ + dagId, + dagRunId, + limit: 10, + orderBy: ["deadline_time"], + }); + + // Used to detect whether the DAG has any deadline alerts at all, so we can show "Met" when there are alerts configured but no active deadline instances. + const { data: alertData, isLoading: isLoadingAlerts } = useDeadlinesServiceGetDagDeadlineAlerts({ + dagId, + limit: 100, + }); + + const { data: runData, isLoading: isLoadingRun } = useDagRunServiceGetDagRun({ dagId, dagRunId }); + Review Comment: This component fetches the DAG run again via `useDagRunServiceGetDagRun` solely to read `end_date`, but the parent `Run/Header` already has a full `DAGRunResponse` (including `end_date`). Consider passing `end_date` (or the whole `dagRun`) into `DeadlineStatus` to avoid an extra network request on every run details page load/refresh. ########## airflow-core/src/airflow/ui/src/pages/Run/DeadlineStatusModal.tsx: ########## @@ -0,0 +1,190 @@ +/*! + * 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 { Badge, Heading, HStack, Separator, Skeleton, Text, VStack } from "@chakra-ui/react"; +import dayjs from "dayjs"; +import duration from "dayjs/plugin/duration"; +import relativeTime from "dayjs/plugin/relativeTime"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiAlertTriangle, FiClock } from "react-icons/fi"; + +import { useDeadlinesServiceGetDeadlines } from "openapi/queries"; +import type { DeadlineAlertResponse } from "openapi/requests/types.gen"; +import { ErrorAlert } from "src/components/ErrorAlert"; +import Time from "src/components/Time"; +import { Dialog } from "src/components/ui"; +import { Pagination } from "src/components/ui/Pagination"; + +dayjs.extend(duration); +dayjs.extend(relativeTime); + +const PAGE_LIMIT = 10; + +type DeadlineStatusModalProps = { + readonly alertMap: Map<string, DeadlineAlertResponse>; + readonly dagId: string; + readonly dagRunId: string; + readonly onClose: () => void; + readonly open: boolean; + readonly runEndDate: string | undefined; +}; + +export const DeadlineStatusModal = ({ + alertMap, + dagId, + dagRunId, + onClose, + open, + runEndDate, +}: DeadlineStatusModalProps) => { + const { t: translate } = useTranslation("dag"); + const [page, setPage] = useState(1); + const offset = (page - 1) * PAGE_LIMIT; + + const { data, error, isLoading } = useDeadlinesServiceGetDeadlines( + { + dagId, + dagRunId, + limit: PAGE_LIMIT, + offset, + orderBy: ["deadline_time"], + }, + undefined, + { enabled: open }, + ); + + const deadlines = data?.deadlines ?? []; + const totalEntries = data?.total_entries ?? 0; + + const onOpenChange = () => { + setPage(1); + onClose(); + }; + + return ( + <Dialog.Root onOpenChange={onOpenChange} open={open} scrollBehavior="inside" size="lg"> + <Dialog.Content backdrop p={4}> + <Dialog.Header> + <Heading size="sm">{translate("deadlineStatus.label")}</Heading> + </Dialog.Header> + <Dialog.CloseTrigger /> + <Dialog.Body pb={2}> + <ErrorAlert error={error} /> + {isLoading ? ( + <VStack> + {Array.from({ length: PAGE_LIMIT }).map((_, idx) => ( + // eslint-disable-next-line react/no-array-index-key + <Skeleton height="52px" key={idx} width="100%" /> + ))} + </VStack> + ) : ( + <VStack gap={0} separator={<Separator />}> + {deadlines.map((dl) => { + const alert = + dl.alert_name !== undefined && dl.alert_name !== null && dl.alert_name !== "" + ? alertMap.get(dl.alert_name) + : undefined; + const deadlineTime = dayjs(dl.deadline_time); Review Comment: The modal resolves `dl.alert_name` to a `DeadlineAlertResponse` via `alertMap` keyed on alert name. Since `DeadlineAlert.name` is nullable and not guaranteed unique, this lookup can be incorrect (collisions/overwrites) and show the wrong completion rule. Prefer using a stable identifier (e.g., alert UUID) from the deadline payload to map alerts to deadline instances. ########## airflow-core/src/airflow/ui/src/pages/Dag/Overview/DagDeadlines.tsx: ########## @@ -0,0 +1,247 @@ +/*! + * 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 { Badge, Box, Button, Flex, Heading, HStack, Separator, Skeleton, VStack } from "@chakra-ui/react"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiAlertTriangle, FiClock } from "react-icons/fi"; + +import { + useDagRunServiceGetDagRuns, + useDeadlinesServiceGetDagDeadlineAlerts, + useDeadlinesServiceGetDeadlines, +} from "openapi/queries"; +import type { DAGRunResponse, DagRunState, DeadlineAlertResponse } from "openapi/requests/types.gen"; +import { ErrorAlert } from "src/components/ErrorAlert"; +import { useAutoRefresh } from "src/utils"; + +import { AllDeadlinesModal } from "./AllDeadlinesModal"; +import { DeadlineRow } from "./DeadlineRow"; + +const LIMIT = 5; + +type DagDeadlinesProps = { + readonly dagId: string; + readonly endDate: string; + readonly startDate: string; +}; + +export const DagDeadlines = ({ dagId, endDate, startDate }: DagDeadlinesProps) => { + const { t: translate } = useTranslation("dag"); + const refetchInterval = useAutoRefresh({ dagId }); + const [modalOpen, setModalOpen] = useState<"missed" | "pending" | undefined>(undefined); + + const { + data: pendingData, + error: pendingError, + isLoading: isPendingLoading, + } = useDeadlinesServiceGetDeadlines( + { + dagId, + dagRunId: "~", + deadlineTimeGte: endDate, + limit: LIMIT, + missed: false, + orderBy: ["deadline_time"], + }, + undefined, + { refetchInterval }, + ); + + const { + data: missedData, + error: missedError, + isLoading: isMissedLoading, + } = useDeadlinesServiceGetDeadlines( + { + dagId, + dagRunId: "~", + lastUpdatedAtGte: startDate, + lastUpdatedAtLte: endDate, + limit: LIMIT, + missed: true, + orderBy: ["-last_updated_at"], + }, + undefined, + { refetchInterval }, + ); + + const { data: runsData } = useDagRunServiceGetDagRuns( + { dagId, limit: 100, runAfterGte: startDate, runAfterLte: endDate }, + undefined, + { refetchInterval }, + ); + + const { data: alertData } = useDeadlinesServiceGetDagDeadlineAlerts({ dagId, limit: 100 }, undefined, { + refetchInterval, + }); + + const runStateMap = new Map<string, DagRunState>(); + const runMap = new Map<string, DAGRunResponse>(); + + for (const run of runsData?.dag_runs ?? []) { + runStateMap.set(run.dag_run_id, run.state); + runMap.set(run.dag_run_id, run); + } + + const alertMap = new Map<string, DeadlineAlertResponse>(); + + for (const alert of alertData?.deadline_alerts ?? []) { + if (alert.name !== undefined && alert.name !== null && alert.name !== "") { + alertMap.set(alert.name, alert); + } + } Review Comment: `alertMap` is keyed by `alert.name` and used to resolve deadline instances by `dl.alert_name`. Since `DeadlineAlert.name` is not enforced to be unique, this can mis-associate alerts when duplicate names exist. Prefer keying by a stable identifier (alert UUID) and consider updating the deadline API response to include the alert id so the UI can join reliably. ########## airflow-core/src/airflow/ui/src/pages/Dag/Overview/DagDeadlines.tsx: ########## @@ -0,0 +1,247 @@ +/*! + * 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 { Badge, Box, Button, Flex, Heading, HStack, Separator, Skeleton, VStack } from "@chakra-ui/react"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiAlertTriangle, FiClock } from "react-icons/fi"; + +import { + useDagRunServiceGetDagRuns, + useDeadlinesServiceGetDagDeadlineAlerts, + useDeadlinesServiceGetDeadlines, +} from "openapi/queries"; +import type { DAGRunResponse, DagRunState, DeadlineAlertResponse } from "openapi/requests/types.gen"; +import { ErrorAlert } from "src/components/ErrorAlert"; +import { useAutoRefresh } from "src/utils"; + +import { AllDeadlinesModal } from "./AllDeadlinesModal"; +import { DeadlineRow } from "./DeadlineRow"; + +const LIMIT = 5; + +type DagDeadlinesProps = { + readonly dagId: string; + readonly endDate: string; + readonly startDate: string; +}; + +export const DagDeadlines = ({ dagId, endDate, startDate }: DagDeadlinesProps) => { + const { t: translate } = useTranslation("dag"); + const refetchInterval = useAutoRefresh({ dagId }); + const [modalOpen, setModalOpen] = useState<"missed" | "pending" | undefined>(undefined); + + const { + data: pendingData, + error: pendingError, + isLoading: isPendingLoading, + } = useDeadlinesServiceGetDeadlines( + { + dagId, + dagRunId: "~", + deadlineTimeGte: endDate, + limit: LIMIT, + missed: false, + orderBy: ["deadline_time"], + }, + undefined, + { refetchInterval }, + ); + + const { + data: missedData, + error: missedError, + isLoading: isMissedLoading, + } = useDeadlinesServiceGetDeadlines( + { + dagId, + dagRunId: "~", + lastUpdatedAtGte: startDate, + lastUpdatedAtLte: endDate, + limit: LIMIT, + missed: true, + orderBy: ["-last_updated_at"], + }, + undefined, + { refetchInterval }, + ); + + const { data: runsData } = useDagRunServiceGetDagRuns( + { dagId, limit: 100, runAfterGte: startDate, runAfterLte: endDate }, + undefined, + { refetchInterval }, + ); + + const { data: alertData } = useDeadlinesServiceGetDagDeadlineAlerts({ dagId, limit: 100 }, undefined, { + refetchInterval, + }); + + const runStateMap = new Map<string, DagRunState>(); + const runMap = new Map<string, DAGRunResponse>(); + + for (const run of runsData?.dag_runs ?? []) { + runStateMap.set(run.dag_run_id, run.state); + runMap.set(run.dag_run_id, run); + } + + const alertMap = new Map<string, DeadlineAlertResponse>(); + + for (const alert of alertData?.deadline_alerts ?? []) { + if (alert.name !== undefined && alert.name !== null && alert.name !== "") { + alertMap.set(alert.name, alert); + } + } + + const pendingDeadlines = pendingData?.deadlines ?? []; + const missedDeadlines = missedData?.deadlines ?? []; + + if ( + !isPendingLoading && + !isMissedLoading && + pendingDeadlines.length === 0 && + missedDeadlines.length === 0 + ) { + return undefined; + } Review Comment: The early return hides the section entirely when both deadline queries return no items, even if `pendingError`/`missedError` is set. This means API failures can result in the deadlines widget silently disappearing with no `ErrorAlert` shown. Consider returning early only when there are no results *and* no errors, or render the widget whenever either query errors so the user sees the error state. ########## airflow-core/src/airflow/ui/src/pages/Dag/DeadlineAlertsBadge.tsx: ########## @@ -0,0 +1,90 @@ +/*! + * 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 { Box, Button, Separator, Text, VStack } from "@chakra-ui/react"; +import dayjs from "dayjs"; +import duration from "dayjs/plugin/duration"; +import relativeTime from "dayjs/plugin/relativeTime"; +import { useTranslation } from "react-i18next"; +import { FiClock } from "react-icons/fi"; + +import { useDeadlinesServiceGetDagDeadlineAlerts } from "openapi/queries"; +import type { DeadlineAlertResponse } from "openapi/requests/types.gen"; +import { Popover } from "src/components/ui"; + +dayjs.extend(duration); +dayjs.extend(relativeTime); + +const AlertRow = ({ alert }: { readonly alert: DeadlineAlertResponse }) => { + const { t: translate } = useTranslation("dag"); + const reference = translate(`deadlineAlerts.referenceType.${alert.reference_type}`, { + defaultValue: alert.reference_type, + }); + const interval = dayjs.duration(alert.interval, "seconds").humanize(); + + return ( + <Box py={2} width="100%"> + <Text color="fg.muted" fontSize="xs"> + {translate("deadlineAlerts.completionRule", { interval, reference })} + {Boolean(alert.name) && ( + <Text as="span" color="fg.subtle" fontSize="xs"> + {" "} + ({alert.name}) + </Text> + )} + </Text> + </Box> + ); +}; + +export const DeadlineAlertsBadge = ({ dagId }: { readonly dagId: string }) => { + const { t: translate } = useTranslation("dag"); + + const { data } = useDeadlinesServiceGetDagDeadlineAlerts({ dagId }); + + const alerts = data?.deadline_alerts ?? []; + + if (alerts.length === 0) { + return undefined; + } + + return ( + // eslint-disable-next-line jsx-a11y/no-autofocus + <Popover.Root autoFocus={false} lazyMount unmountOnExit> + <Popover.Trigger asChild> + <Button color="fg.info" size="xs" variant="outline"> + <FiClock /> + {translate("deadlineAlerts.count", { count: alerts.length })} + </Button> Review Comment: The badge count is derived from `alerts.length`, but the API response is paginated and also includes `total_entries`. If the server returns only the first page, the badge can display an incorrect count. Prefer using `data.total_entries` for the count, and consider passing an explicit `limit` if you need to render all alerts in the popover. -- 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]
