pierrejeambrun commented on code in PR #74516: URL: https://github.com/apache/airflow/pull/74516#discussion_r4260835991
########## airflow-core/src/airflow/ui/src/components/BackfillInProgressBadge.tsx: ########## @@ -0,0 +1,58 @@ +/*! + * 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 } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { RiArrowGoBackFill } from "react-icons/ri"; + +import { useBackfillServiceListBackfillsUi } from "openapi/queries"; + +import { RouterLink } from "src/system-components"; + +import { useAutoRefresh } from "src/utils"; + +type Props = { + readonly dagId: string; +}; + +/** Reports a backfill that has not finished yet, linking to the Dag's Backfills tab. */ +export const BackfillInProgressBadge = ({ dagId }: Props) => { + const { t: translate } = useTranslation("components"); + // Not keyed to the Dag, whose paused state stops auto-refresh: a backfill keeps running + // on a paused Dag, so the badge has to keep up with it either way. + const refetchInterval = useAutoRefresh({}); + + const { data } = useBackfillServiceListBackfillsUi({ active: true, dagId }, undefined, { + enabled: dagId !== "", + refetchInterval, + }); Review Comment: With no `dagId`, I think `useAutoRefresh` always returns the interval, so every Dag page polls `/ui/backfills` on each tick even when nothing is backfilling. The old banner only polled while a backfill was running. Could we bring that back, maybe something like this? ```suggestion const refetchInterval = useAutoRefresh({}); const { data } = useBackfillServiceListBackfillsUi({ active: true, dagId }, undefined, { enabled: dagId !== "", refetchInterval: (query) => query.state.data?.backfills.some((backfill) => backfill.completed_at === null) ? refetchInterval : false, }); ``` ########## airflow-core/src/airflow/ui/src/components/DrainingBadge.tsx: ########## @@ -17,24 +17,72 @@ * under the License. */ import type { BadgeProps } from "@chakra-ui/react"; +import { Button, HStack, Stack, Text } from "@chakra-ui/react"; import { useTranslation } from "react-i18next"; import { MdHourglassTop } from "react-icons/md"; -import { Tooltip } from "src/system-components"; +import { Popover } from "src/system-components"; + +import { useTogglePause } from "src/queries/useTogglePause"; import { StateBadge } from "./StateBadge"; -export const DrainingBadge = (props: BadgeProps) => { +type Props = { + /** Enables the drain actions. Omit in list contexts that only report the state. */ + readonly dagId?: string; Review Comment: All three callers pass `dagId`, including the Dags list card and table, so the list views do get the drain actions. Is that intended? If so, maybe make `dagId` required and drop the no-`dagId` branch and its test. If not, I think the list callers should stop passing it. ########## airflow-core/src/airflow/ui/src/components/StatusChip.tsx: ########## @@ -0,0 +1,85 @@ +/*! + * 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 type { ReactNode } from "react"; + +import { Badge, HStack } from "@chakra-ui/react"; + +import { Popover, RouterLink } from "src/system-components"; + +/** Highest severity first; callers render chips in this order. */ +export type StatusChipSeverity = "error" | "info" | "warning"; + +type Props = { + /** Detail and actions shown on click. Omit for a chip that only states status. */ + readonly children?: ReactNode; + readonly icon?: ReactNode; + readonly label: string; + /** Click handler for a chip whose detail lives in a modal rather than a popover. */ + readonly onClick?: () => void; + readonly severity: StatusChipSeverity; + /** Destination for a chip whose detail lives on another page. */ + readonly to?: string; +}; + +export const StatusChip = ({ children, icon, label, onClick, severity, to }: Props) => { + const content = ( + <HStack gap={1}> + {icon} + {label} + </HStack> + ); + // Only an error should compete with the object's own state badge for attention. + const variant = severity === "error" ? "solid" : "subtle"; + + if (to !== undefined) { Review Comment: It looks like only `DagDeactivatedChip` uses this, via `onClick`. Could we drop the `to` and popover branches until something needs them? ########## airflow-core/src/airflow/ui/src/components/DagDeactivatedChip.tsx: ########## @@ -47,25 +48,21 @@ export const DagDeactivatedBanner = () => { const importError = data?.import_errors[0]; return ( - <HStack bg="bg.warning" color="fg.warning" justifyContent="space-between" px={3} py={1}> - <Text>{translate("header.status.deactivated")}</Text> - {importError ? ( - <> - <Button - borderColor="fg.warning" - colorPalette="warning" - onClick={onOpen} - size="xs" - variant="outline" - > - <HStack gap={1}> - <LuFileWarning size={14} /> - {translate("dashboard:importErrors.dagImportError", { count: 1 })} - </HStack> - </Button> - <DagImportErrorModal importError={importError} onClose={onClose} open={open} /> - </> - ) : undefined} - </HStack> + <> + <StatusChip + icon={<LuFileWarning size={14} />} + label={ + importError === undefined + ? translate("dag:header.status.deactivated") + : translate("dashboard:importErrors.dagImportError_one") + } + // Inert without a parse error, since there is nothing to open. + onClick={importError === undefined ? undefined : onOpen} + severity="error" Review Comment: In my local test a plain deactivated Dag gets the same solid red chip as a parse error. Would `warning` fit better when there's no import error, like the old banner? Something like `severity={importError === undefined ? "warning" : "error"}`. -- 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]
