pierrejeambrun commented on code in PR #65687: URL: https://github.com/apache/airflow/pull/65687#discussion_r3148508582
########## airflow-core/src/airflow/ui/src/pages/Dag/DagImportError.tsx: ########## @@ -0,0 +1,75 @@ +/*! + * 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, useDisclosure } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; + +import { useImportErrorServiceGetImportErrors } from "openapi/queries"; +import type { DAGResponse } from "openapi/requests/types.gen"; +import { DagImportErrorsIconBadge } from "src/components/DagImportErrorsIconBadge"; + +import { StaleDagImportErrorModal } from "./StaleDagImportErrorModal"; +import { selectLatestMatchingImportError } from "./selectLatestMatchingImportError"; + +const IMPORT_ERROR_FETCH_LIMIT = 100; + +type Props = { + readonly dag: Pick<DAGResponse, "bundle_name" | "is_stale" | "relative_fileloc">; +}; + +export const DagImportError = ({ dag }: Props) => { + const { t: translate } = useTranslation("dashboard"); + const { onClose, onOpen, open } = useDisclosure(); + const relativeFileloc = dag.relative_fileloc ?? ""; + const shouldFetch = dag.is_stale && relativeFileloc.length > 0; + + const { data } = useImportErrorServiceGetImportErrors( + { + filenamePattern: relativeFileloc, + limit: IMPORT_ERROR_FETCH_LIMIT, + offset: 0, + orderBy: ["-timestamp"], + }, + undefined, + { enabled: shouldFetch }, + ); + + if (!shouldFetch) { + return undefined; + } + + const matched = selectLatestMatchingImportError(data?.import_errors, relativeFileloc, dag.bundle_name); + + if (matched === undefined) { + return undefined; + } Review Comment: This piece isn't working I think. Basically if the `relativeFileLoc` is super common, or on a very big installation, the right error could not be in the first 100 (page) result. We probably need a small tweak / filter addition on the backend part so we don't need to filter a whole page a do a client side filtering. ########## airflow-core/src/airflow/ui/src/pages/Dag/selectLatestMatchingImportError.ts: ########## @@ -0,0 +1,48 @@ +/*! + * 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 { ImportErrorResponse } from "openapi/requests/types.gen"; + +const sameBundle = (left: string | null | undefined, right: string | null | undefined): boolean => + (left ?? undefined) === (right ?? undefined); + +/** + * Pick the most recent import error row for this DAG's bundle + relative file path. + * The list API may return other files when `filename_pattern` is a substring match. + */ +export const selectLatestMatchingImportError = ( + importErrors: Array<ImportErrorResponse> | undefined, + relativeFileloc: string, + bundleName: string | null | undefined, +): ImportErrorResponse | undefined => { + if (importErrors === undefined || importErrors.length === 0) { + return undefined; + } + + const matching = importErrors.filter( + (row) => row.filename === relativeFileloc && sameBundle(row.bundle_name, bundleName), + ); + + if (matching.length === 0) { + return undefined; + } + + return matching.reduce((latest, current) => + Date.parse(current.timestamp) > Date.parse(latest.timestamp) ? current : latest, + ); +}; Review Comment: API already returns the list ordered by `-timestamp` ########## airflow-core/src/airflow/ui/src/components/DagImportErrorsIconBadge.tsx: ########## @@ -0,0 +1,51 @@ +/*! + * 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 } from "@chakra-ui/react"; +import { LuFileWarning } from "react-icons/lu"; + +import { StateBadge } from "src/components/StateBadge"; + +export type DagImportErrorsIconBadgeProps = { + readonly "aria-label"?: string | undefined; + readonly count?: number; + readonly "data-testid"?: string | undefined; + readonly onClick: () => void; + readonly title: string; +}; + +export const DagImportErrorsIconBadge = ({ + "aria-label": ariaLabel, + count, + "data-testid": dataTestId, + onClick, + title, +}: DagImportErrorsIconBadgeProps) => ( + <StateBadge + aria-label={ariaLabel ?? title} + as={Button} + colorPalette="failed" + {...(dataTestId === undefined ? {} : { "data-testid": dataTestId })} Review Comment: I don't think that's necessary, undefined `data-testid` will just be dropped. ########## airflow-core/src/airflow/ui/src/components/DagImportErrorAccordion.tsx: ########## @@ -0,0 +1,87 @@ +/*! + * 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, ClipboardRoot, HStack, Text } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { LuFileWarning } from "react-icons/lu"; +import { PiFilePy } from "react-icons/pi"; + +import type { ImportErrorResponse } from "openapi/requests/types.gen"; +import { StateBadge } from "src/components/StateBadge"; +import Time from "src/components/Time"; +import { Accordion, ClipboardIconButton } from "src/components/ui"; + +type Props = { + readonly defaultValue?: Array<string>; + readonly importErrors: ReadonlyArray<ImportErrorResponse>; + readonly showFileErrorIndicator?: boolean; +}; + +export const DagImportErrorAccordion = ({ + defaultValue, + importErrors, + showFileErrorIndicator = false, Review Comment: ```suggestion ``` Looks like dead code. ########## airflow-core/src/airflow/ui/src/components/DagImportErrorsIconBadge.tsx: ########## @@ -0,0 +1,51 @@ +/*! + * 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 } from "@chakra-ui/react"; +import { LuFileWarning } from "react-icons/lu"; + +import { StateBadge } from "src/components/StateBadge"; + +export type DagImportErrorsIconBadgeProps = { + readonly "aria-label"?: string | undefined; + readonly count?: number; + readonly "data-testid"?: string | undefined; + readonly onClick: () => void; + readonly title: string; +}; + +export const DagImportErrorsIconBadge = ({ + "aria-label": ariaLabel, + count, + "data-testid": dataTestId, + onClick, + title, +}: DagImportErrorsIconBadgeProps) => ( + <StateBadge + aria-label={ariaLabel ?? title} + as={Button} + colorPalette="failed" + {...(dataTestId === undefined ? {} : { "data-testid": dataTestId })} + height={7} + onClick={onClick} + title={title} + > + <LuFileWarning size={8} /> + {count ?? undefined} Review Comment: That's equivalent to this I believe ```suggestion {count} ``` Also when this is '0' will display '0' no sure it's what we want. ########## airflow-core/src/airflow/ui/src/pages/Dag/StaleDagImportErrorModal.tsx: ########## @@ -0,0 +1,53 @@ +/*! + * 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 { Heading, HStack } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { LuFileWarning } from "react-icons/lu"; + +import type { ImportErrorResponse } from "openapi/requests/types.gen"; +import { DagImportErrorAccordion } from "src/components/DagImportErrorAccordion"; +import { Dialog } from "src/components/ui"; + +type Props = { + readonly importError: ImportErrorResponse; + readonly onClose: () => void; + readonly open: boolean; +}; + +export const StaleDagImportErrorModal = ({ importError, onClose, open }: Props) => { + const { t: translate } = useTranslation("dashboard"); + const itemValue = String(importError.import_error_id); + + return ( + <Dialog.Root lazyMount onOpenChange={onClose} open={open} scrollBehavior="inside" size="lg"> + <Dialog.Content backdrop p={4}> + <Dialog.Header> + <HStack fontSize="xl" gap={2}> + <LuFileWarning /> + <Heading>{translate("importErrors.dagImportError", { count: 1 })}</Heading> + </HStack> + </Dialog.Header> + <Dialog.CloseTrigger /> + <Dialog.Body> + <DagImportErrorAccordion defaultValue={[itemValue]} importErrors={[importError]} /> Review Comment: Having the accordion just for 1 item feels heavy. Like open / close it similarly to how we do it when we have several of them no sure of the value when only 1 item is guaranteed here. -- 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]
