kaxil commented on code in PR #75418: URL: https://github.com/apache/airflow/pull/75418#discussion_r4353426337
########## airflow-core/src/airflow/ui/src/components/StatusMessage.tsx: ########## @@ -0,0 +1,50 @@ +/*! + * 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 { HStack, Text, VStack } from "@chakra-ui/react"; +import { FiAlertCircle, FiAlertTriangle } from "react-icons/fi"; + +import type { StatusChipSeverity } from "src/components/StatusChip"; + +type Props = { + /** The detail under the heading. Omit for a status that says all it has to say in its heading. */ + readonly body?: ReactNode; + readonly heading: string; + readonly severity: Exclude<StatusChipSeverity, "info">; +}; + +/** + * One problem stated in a list of them: its severity in the icon and the colour of its text, its + * short form as the heading, the long form underneath. Coloured text rather than a filled block, + * so a list of these stays readable and a long traceback does not become a wall of colour. + */ +export const StatusMessage = ({ body, heading, severity }: Props) => ( + <VStack align="start" gap={1} w="100%"> + <HStack color={`fg.${severity}`} gap={2}> + {severity === "error" ? <FiAlertCircle /> : <FiAlertTriangle />} + <Text fontWeight="bold">{heading}</Text> + </HStack> + {body === undefined ? undefined : ( + <Text color={`fg.${severity}`} fontSize="sm" whiteSpace="pre-wrap" wordBreak="break-word"> Review Comment: `body` is typed `ReactNode` but always lands inside a `Text`, which renders a `<p>`. `DAGWarningsModal` passes `DagImportErrorDetails` here (an `HStack`, two buttons and more `Text`), so opening the modal for a Dag with an import error logs "In HTML, <div> cannot be a descendant of <p>" and "<p> cannot be a descendant of <p>". You can see it by running `DagStatusChip.test.tsx` with `--silent=false`. It also makes the bundle name and filename row red and small. Using a `Box` (or `Text as="div"`) as the wrapper would fix both. ########## airflow-core/src/airflow/ui/src/components/DagStatusChip.test.tsx: ########## @@ -0,0 +1,161 @@ +/*! + * 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 "@testing-library/jest-dom/vitest"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import i18n from "src/i18n/config"; +import { Wrapper } from "src/utils/Wrapper"; + +import componentsLocale from "../../public/i18n/locales/en/components.json"; +import dagLocale from "../../public/i18n/locales/en/dag.json"; +import dashboardLocale from "../../public/i18n/locales/en/dashboard.json"; +import { DagStatusChip } from "./DagStatusChip"; + +const { + mockUseDagServiceGetDag, + mockUseDagWarningServiceListDagWarnings, + mockUseImportErrorServiceGetImportErrors, +} = vi.hoisted(() => ({ + mockUseDagServiceGetDag: vi.fn(), + mockUseDagWarningServiceListDagWarnings: vi.fn(), + mockUseImportErrorServiceGetImportErrors: vi.fn(), +})); + +vi.mock("openapi/queries", async (importOriginal) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- `import()` type is the standard pattern for typing `importOriginal` in Vitest mocks. + const actual = await importOriginal<typeof import("openapi/queries")>(); + + return { + ...actual, + useDagServiceGetDag: mockUseDagServiceGetDag, + useDagWarningServiceListDagWarnings: mockUseDagWarningServiceListDagWarnings, + useImportErrorServiceGetImportErrors: mockUseImportErrorServiceGetImportErrors, + }; +}); + +const staleDagQuery = { data: { is_stale: true, relative_fileloc: "stale_dag.py" }, isLoading: false }; +const liveDagQuery = { data: { is_stale: false, relative_fileloc: "stale_dag.py" }, isLoading: false }; +const noImportErrors = { data: { import_errors: [], total_entries: 0 }, isLoading: false }; +const noWarnings = { data: { dag_warnings: [], total_entries: 0 }, isLoading: false }; + +const importErrorQuery = { + data: { + import_errors: [ + { + bundle_name: "dags-folder", + filename: "stale_dag.py", + import_error_id: 42, + stack_trace: "Traceback (most recent call last):\nSyntaxError: invalid syntax", + timestamp: "2025-02-01T12:00:00Z", + }, + ], + total_entries: 1, + }, + isLoading: false, +}; + +const warningsQuery = { + data: { + dag_warnings: [{ message: "duplicate task id", warning_type: "non-existent pool" }], + total_entries: 1, + }, + isLoading: false, +}; + +const deactivated = () => i18n.t("header.status.deactivated", { ns: "dag" }); +const importErrorName = () => i18n.t("importErrors.dagImportError", { count: 1, ns: "dashboard" }); +const warningName = (count: number) => i18n.t("dagWarnings.warning", { count, ns: "components" }); + +const renderChip = () => + render( + <Wrapper> + <DagStatusChip dagId="stale_dag" /> + </Wrapper>, + ); + +describe("DagStatusChip", () => { + beforeEach(() => { + i18n.addResourceBundle("en", "components", componentsLocale, true, true); + i18n.addResourceBundle("en", "dag", dagLocale, true, true); + i18n.addResourceBundle("en", "dashboard", dashboardLocale, true, true); + mockUseDagServiceGetDag.mockReturnValue(staleDagQuery); + mockUseImportErrorServiceGetImportErrors.mockReturnValue(noImportErrors); + mockUseDagWarningServiceListDagWarnings.mockReturnValue(noWarnings); + }); + + it("renders nothing when the Dag is live and has no warnings", () => { + mockUseDagServiceGetDag.mockReturnValue(liveDagQuery); + + renderChip(); + + expect(screen.queryByRole("button")).not.toBeInTheDocument(); + }); + + it("names the deactivation when that is the only status", () => { + renderChip(); + + expect(screen.getByRole("button", { name: deactivated() })).toBeInTheDocument(); + }); + + it("names the warnings when the Dag is live", () => { + mockUseDagServiceGetDag.mockReturnValue(liveDagQuery); + mockUseDagWarningServiceListDagWarnings.mockReturnValue(warningsQuery); + + renderChip(); + + expect(screen.getByRole("button", { name: warningName(1) })).toBeInTheDocument(); + }); + + it("lets the parse error outrank the warnings for name and severity", async () => { + mockUseImportErrorServiceGetImportErrors.mockReturnValue(importErrorQuery); + mockUseDagWarningServiceListDagWarnings.mockReturnValue(warningsQuery); + + renderChip(); + + // The worst problem names the chip; the deactivation is explained by the parse error, not + // stated twice. + const chip = screen.getByRole("button", { name: importErrorName() }); + + expect(screen.queryByRole("button", { name: deactivated() })).not.toBeInTheDocument(); + + fireEvent.click(chip); + + // Both land in the one modal, the parse error ahead of the warning. + await waitFor(() => expect(screen.getByText(/SyntaxError: invalid syntax/u)).toBeInTheDocument()); + expect(screen.getByText("duplicate task id")).toBeInTheDocument(); + }); + + it("reports a page load failure as an error beside the warnings", async () => { + mockUseDagServiceGetDag.mockReturnValue(liveDagQuery); + mockUseDagWarningServiceListDagWarnings.mockReturnValue(warningsQuery); + + render( + <Wrapper> + <DagStatusChip dagId="stale_dag" error={new Error("request failed")} /> + </Wrapper>, + ); + + fireEvent.click( + screen.getByRole("button", { name: i18n.t("dagWarnings.error_one", { ns: "components" }) }), + ); + + await waitFor(() => expect(screen.getByText("duplicate task id")).toBeInTheDocument()); Review Comment: This only checks that the warning shows up, so it still passes if the modal stops rendering the page error at all. Could we also assert that `request failed` is on screen? ########## airflow-core/src/airflow/ui/src/components/FilterBar/FilterBar.test.tsx: ########## @@ -90,6 +90,20 @@ describe("FilterBar boolean filters", () => { expect(screen.getByTestId("needs_review-pill")).not.toHaveTextContent(":"); }); + it("keeps the remove control out of the pill button", () => { + render( + <FilterBar + configs={[booleanConfig]} + initialValues={{ needs_review: "true" }} + onFiltersChange={vi.fn()} + />, + { wrapper }, + ); + + // A button nested inside another button is invalid HTML and React warns about it at runtime. + expect(within(screen.getByTestId("needs_review-pill")).queryAllByRole("button")).toHaveLength(0); Review Comment: This only looks inside the inner `-pill` element. If someone puts `as="button"` back on the outer `Box`, the remove button is nested again and this still passes. `expect(container.querySelector("button button")).toBeNull()` would check the thing we actually care about. ########## airflow-core/src/airflow/ui/src/components/DagStatusChip.test.tsx: ########## @@ -0,0 +1,161 @@ +/*! + * 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 "@testing-library/jest-dom/vitest"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import i18n from "src/i18n/config"; +import { Wrapper } from "src/utils/Wrapper"; + +import componentsLocale from "../../public/i18n/locales/en/components.json"; +import dagLocale from "../../public/i18n/locales/en/dag.json"; +import dashboardLocale from "../../public/i18n/locales/en/dashboard.json"; +import { DagStatusChip } from "./DagStatusChip"; + +const { + mockUseDagServiceGetDag, + mockUseDagWarningServiceListDagWarnings, + mockUseImportErrorServiceGetImportErrors, +} = vi.hoisted(() => ({ + mockUseDagServiceGetDag: vi.fn(), + mockUseDagWarningServiceListDagWarnings: vi.fn(), + mockUseImportErrorServiceGetImportErrors: vi.fn(), +})); + +vi.mock("openapi/queries", async (importOriginal) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- `import()` type is the standard pattern for typing `importOriginal` in Vitest mocks. + const actual = await importOriginal<typeof import("openapi/queries")>(); + + return { + ...actual, + useDagServiceGetDag: mockUseDagServiceGetDag, + useDagWarningServiceListDagWarnings: mockUseDagWarningServiceListDagWarnings, + useImportErrorServiceGetImportErrors: mockUseImportErrorServiceGetImportErrors, + }; +}); + +const staleDagQuery = { data: { is_stale: true, relative_fileloc: "stale_dag.py" }, isLoading: false }; +const liveDagQuery = { data: { is_stale: false, relative_fileloc: "stale_dag.py" }, isLoading: false }; +const noImportErrors = { data: { import_errors: [], total_entries: 0 }, isLoading: false }; +const noWarnings = { data: { dag_warnings: [], total_entries: 0 }, isLoading: false }; + +const importErrorQuery = { + data: { + import_errors: [ + { + bundle_name: "dags-folder", + filename: "stale_dag.py", + import_error_id: 42, + stack_trace: "Traceback (most recent call last):\nSyntaxError: invalid syntax", + timestamp: "2025-02-01T12:00:00Z", + }, + ], + total_entries: 1, + }, + isLoading: false, +}; + +const warningsQuery = { + data: { + dag_warnings: [{ message: "duplicate task id", warning_type: "non-existent pool" }], + total_entries: 1, + }, + isLoading: false, +}; + +const deactivated = () => i18n.t("header.status.deactivated", { ns: "dag" }); +const importErrorName = () => i18n.t("importErrors.dagImportError", { count: 1, ns: "dashboard" }); +const warningName = (count: number) => i18n.t("dagWarnings.warning", { count, ns: "components" }); + +const renderChip = () => + render( + <Wrapper> + <DagStatusChip dagId="stale_dag" /> + </Wrapper>, + ); + +describe("DagStatusChip", () => { + beforeEach(() => { + i18n.addResourceBundle("en", "components", componentsLocale, true, true); + i18n.addResourceBundle("en", "dag", dagLocale, true, true); + i18n.addResourceBundle("en", "dashboard", dashboardLocale, true, true); + mockUseDagServiceGetDag.mockReturnValue(staleDagQuery); + mockUseImportErrorServiceGetImportErrors.mockReturnValue(noImportErrors); + mockUseDagWarningServiceListDagWarnings.mockReturnValue(noWarnings); + }); + + it("renders nothing when the Dag is live and has no warnings", () => { + mockUseDagServiceGetDag.mockReturnValue(liveDagQuery); + + renderChip(); + + expect(screen.queryByRole("button")).not.toBeInTheDocument(); + }); + + it("names the deactivation when that is the only status", () => { + renderChip(); + + expect(screen.getByRole("button", { name: deactivated() })).toBeInTheDocument(); + }); + + it("names the warnings when the Dag is live", () => { + mockUseDagServiceGetDag.mockReturnValue(liveDagQuery); + mockUseDagWarningServiceListDagWarnings.mockReturnValue(warningsQuery); + + renderChip(); + + expect(screen.getByRole("button", { name: warningName(1) })).toBeInTheDocument(); + }); + + it("lets the parse error outrank the warnings for name and severity", async () => { + mockUseImportErrorServiceGetImportErrors.mockReturnValue(importErrorQuery); + mockUseDagWarningServiceListDagWarnings.mockReturnValue(warningsQuery); + + renderChip(); + + // The worst problem names the chip; the deactivation is explained by the parse error, not + // stated twice. + const chip = screen.getByRole("button", { name: importErrorName() }); + + expect(screen.queryByRole("button", { name: deactivated() })).not.toBeInTheDocument(); Review Comment: This assertion can't fail. With an import error present, the chip label always comes from the import-error branch, so there is never a button named after the deactivation. The dedup the comment describes happens in the modal, so checking `queryByText(deactivated())` after the click would cover it. ########## airflow-core/src/airflow/ui/src/components/DAGWarningsModal.tsx: ########## @@ -20,33 +20,65 @@ import { Heading, HStack, Spacer, VStack } from "@chakra-ui/react"; import { useTranslation } from "react-i18next"; import { LuFileWarning } from "react-icons/lu"; -import type { DAGWarningResponse } from "openapi/requests/types.gen"; +import type { DAGWarningResponse, ImportErrorResponse } from "openapi/requests/types.gen"; import { Modal } from "src/system-components"; -import { ErrorAlert } from "./ErrorAlert"; +import { getApiErrorDetail } from "src/utils"; + +import { DagImportErrorDetails } from "./DagImportErrorDetails"; +import type { ExpandedApiError } from "./ErrorAlert"; +import { StatusMessage } from "./StatusMessage"; import { WarningAlert } from "./WarningAlert"; /** The modal lists the page error as one more item next to the warnings, so the button count does too. */ Review Comment: Now that the DetailsLayout button this comment refers to is gone, `countDagWarnings` has no production caller (only `DAGWarningsModal.test.tsx` uses it). `common:dagWarnings` in `en/common.json` was only used by that button too. Should we drop both, along with the test? ########## airflow-core/src/airflow/ui/src/components/WarningAlert.tsx: ########## @@ -16,26 +16,30 @@ * specific language governing permissions and limitations * under the License. */ -import { Flex } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; -import { Alert } from "src/system-components"; +import { StatusMessage } from "src/components/StatusMessage"; type Props = { readonly warning?: { message: string; + warning_type?: string; }; }; export const WarningAlert = ({ warning }: Props) => { - if (!Boolean(warning)) { + const { t: translate } = useTranslation("components"); + + if (warning === undefined) { return undefined; } + // The type is the warning's own short form; only a synthesised warning arrives without one. return ( - <Alert data-testid="warning-alert" status="warning"> - <Flex align="center" whiteSpace="preserve" wordBreak="break-all"> - {warning?.message} - </Flex> - </Alert> + <StatusMessage + body={warning.message} + heading={warning.warning_type ?? translate("dagWarnings.warning", { count: 1 })} Review Comment: `warning_type` is the raw backend value ("non-existent pool", "runtime varying value", ...), so this heading shows untranslated English in every locale. Before this change only `message` was shown. Could we map the known types to i18n keys (with `defaultValue` for unknown ones), or keep the translated heading? The `??` fallback is also unreachable: the only caller passes a `DAGWarningResponse`, where `warning_type` is required. -- 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]
