UI: Fix XCom list crash when a value request fails (#74131)

* UI: Fix XCom list crash when a value request fails

A failed XCom value request takes down the entire list, hiding entries
whose values are still available. Those entries should remain readable
and copyable when an individual value cannot be fetched.

* UI: Show XCom errors in a shared modal

Keep only the status code and alert-circle icon in each failed XCom
cell. Use a shared ErrorModal for full XCom and Dag import errors.

Reset the details state when an error clears so a later failed refetch
does not reopen the modal automatically. Cover modal interactions,
structured errors, network failures, and recovery in the UI tests.
This commit is contained in:
Jonghyun An authored and GitHub committed 2026-10-06 17:21:08 -04:00
1 parent 9cc55f2ad3
commit b1e4ad0321
6 files changed
+432 -19

No files matched your search

@@ -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 "@testing-library/jest-dom/vitest";
import { render, screen, within } from "@testing-library/react";
import { describe, expect, it, vi } from "vitest";
import type { ImportErrorResponse } from "openapi/requests/types.gen";
import { Wrapper } from "src/utils/Wrapper";
import { DagImportErrorModal } from "./DagImportErrorModal";
const importError: ImportErrorResponse = {
bundle_name: "dags-folder",
file_token: "token",
filename: "dags/broken_dag.py",
import_error_id: 1,
source_reference: null,
stack_trace: 'Traceback (most recent call last):\n File "dags/broken_dag.py", line 1',
timestamp: "2025-01-01T00:00:00Z",
};
describe("DagImportErrorModal", () => {
it("shows the import error metadata and stack trace in a titled dialog", () => {
render(<DagImportErrorModal importError={importError} onClose={vi.fn()} open />, { wrapper: Wrapper });
// i18n resources are not loaded in unit tests, so the title renders as its key.
const dialog = screen.getByRole("dialog", { name: "importErrors.dagImportError" });
expect(dialog).toHaveTextContent("dags-folder");
expect(dialog).toHaveTextContent("dags/broken_dag.py");
expect(within(dialog).getByText(/Traceback \(most recent call last\)/u)).toBeInTheDocument();
});
});
@@ -16,15 +16,16 @@
* specific language governing permissions and limitations
* under the License.
*/
import { ClipboardRoot, Heading, HStack, Text } from "@chakra-ui/react";
import { 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 { ClipboardIconButton, Modal } from "src/system-components";
import { ClipboardIconButton } from "src/system-components";
import { ErrorModal } from "src/components/ErrorModal";
import Time from "src/components/Time";
type Props = {
@@ -37,21 +38,11 @@ export const DagImportErrorModal = ({ importError, onClose, open }: Props) => {
const { t: translate } = useTranslation(["dashboard", "components"]);
return (
<Modal
contentProps={{ padding: 4 }}
headerProps={{
children: (
<HStack gap={2}>
<LuFileWarning />
<Heading fontSize="lg">{translate("importErrors.dagImportError", { count: 1 })}</Heading>
</HStack>
),
}}
lazyMount
onOpenChange={onClose}
<ErrorModal
icon={<LuFileWarning />}
onClose={onClose}
open={open}
scrollBehavior="inside"
size="lg"
title={translate("importErrors.dagImportError", { count: 1 })}
>
<HStack alignItems="center" flexWrap="wrap" gap={2} mb={2}>
<Text fontWeight="bold">
@@ -73,6 +64,6 @@ export const DagImportErrorModal = ({ importError, onClose, open }: Props) => {
<Text color="fg.error" fontSize="sm" whiteSpace="pre-wrap">
<code>{importError.stack_trace}</code>
</Text>
</Modal>
</ErrorModal>
);
};
@@ -0,0 +1,55 @@
/*!
* 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, within } from "@testing-library/react";
import { describe, expect, it, vi } from "vitest";
import { BaseWrapper } from "src/utils/Wrapper";
import { ErrorModal } from "./ErrorModal";
describe("ErrorModal", () => {
it("names the dialog after its title and renders the error details", () => {
render(
<ErrorModal onClose={vi.fn()} open title="Something failed">
Full error details
</ErrorModal>,
{ wrapper: BaseWrapper },
);
const dialog = screen.getByRole("dialog", { name: "Something failed" });
expect(within(dialog).getByText("Full error details")).toBeInTheDocument();
});
it("calls onClose when the close button is clicked", async () => {
const onClose = vi.fn();
render(
<ErrorModal onClose={onClose} open title="Something failed">
Full error details
</ErrorModal>,
{ wrapper: BaseWrapper },
);
fireEvent.click(screen.getByRole("button", { name: "Close" }));
await waitFor(() => expect(onClose).toHaveBeenCalled());
});
});
@@ -0,0 +1,55 @@
/*!
* 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 { Heading, HStack } from "@chakra-ui/react";
import { Dialog, Modal } from "src/system-components";
type Props = {
readonly children: ReactNode;
readonly icon?: ReactNode;
readonly onClose: () => void;
readonly open: boolean;
readonly title: ReactNode;
};
export const ErrorModal = ({ children, icon, onClose, open, title }: Props) => (
<Modal
bodyProps={{ overflowWrap: "anywhere" }}
contentProps={{ padding: 4 }}
headerProps={{
children: (
<HStack gap={2} minW={0}>
{icon}
<Dialog.Title asChild>
<Heading fontSize="lg">{title}</Heading>
</Dialog.Title>
</HStack>
),
}}
lazyMount
onOpenChange={onClose}
open={open}
scrollBehavior="inside"
size="lg"
>
{children}
</Modal>
);
@@ -0,0 +1,216 @@
/*!
* 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 { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import "@testing-library/jest-dom/vitest";
import { act, fireEvent, render, screen, waitFor, within } from "@testing-library/react";
import { http, HttpResponse } from "msw";
import { setupServer } from "msw/node";
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
import { Wrapper } from "src/utils/Wrapper";
import { XComEntry } from "./XComEntry";
const server = setupServer();
const entryUrl = "/api/v2/dags/test_dag/dagRuns/test_run/taskInstances/test_task/xcomEntries/:xcomKey";
const entryProps = { dagId: "test_dag", mapIndex: -1, runId: "test_run", taskId: "test_task" };
// i18n resources are not loaded in unit tests, so translated text renders as its key.
const errorTitle = "error.title";
beforeAll(() => server.listen({ onUnhandledRequest: "bypass" }));
afterEach(() => server.resetHandlers());
afterAll(() => server.close());
describe("XComEntry", () => {
it.each([
{ detail: "XCom entry was deleted", status: 404 },
{ detail: "Access to this XCom entry is denied", status: 403 },
{ detail: "Unable to deserialize the XCom value", status: 500 },
])("shows a $status error without removing another entry", async ({ detail, status }) => {
server.use(
http.get(entryUrl, ({ params }) =>
params.xcomKey === "failed_entry"
? HttpResponse.json({ detail }, { status })
: HttpResponse.json({ key: "healthy_entry", value: "Another XCom value" }),
),
);
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
render(
<QueryClientProvider client={queryClient}>
<XComEntry {...entryProps} xcomKey="failed_entry" />
<XComEntry {...entryProps} xcomKey="healthy_entry" />
</QueryClientProvider>,
{ wrapper: Wrapper },
);
const trigger = await screen.findByRole("button", { name: `${errorTitle} ${String(status)}` });
expect(trigger).toHaveTextContent(String(status));
expect(trigger).not.toHaveTextContent(detail);
expect(await screen.findByText("Another XCom value")).toBeInTheDocument();
expect(screen.getAllByRole("button", { name: /copy/iu })).toHaveLength(1);
expect(screen.queryByTestId("error-alert")).not.toBeInTheDocument();
fireEvent.click(trigger);
const dialog = await screen.findByRole("dialog", { name: errorTitle });
const alert = within(dialog).getByTestId("error-alert");
expect(alert).toHaveTextContent(String(status));
expect(alert).toHaveTextContent(detail);
});
it("closes the full error details from the modal close button", async () => {
server.use(
http.get(entryUrl, () => HttpResponse.json({ detail: "XCom entry was deleted" }, { status: 404 })),
);
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
render(
<QueryClientProvider client={queryClient}>
<XComEntry {...entryProps} xcomKey="failed_entry" />
</QueryClientProvider>,
{ wrapper: Wrapper },
);
fireEvent.click(await screen.findByRole("button", { name: `${errorTitle} 404` }));
const dialog = await screen.findByRole("dialog", { name: errorTitle });
fireEvent.click(within(dialog).getByRole("button", { name: "Close" }));
await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument());
expect(screen.getByRole("button", { name: `${errorTitle} 404` })).toBeInTheDocument();
});
it("shows a refetch error instead of allowing a stale value to be copied", async () => {
server.use(http.get(entryUrl, () => HttpResponse.json({ key: "return_value", value: "Stale value" })));
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
render(
<QueryClientProvider client={queryClient}>
<XComEntry {...entryProps} xcomKey="return_value" />
</QueryClientProvider>,
{ wrapper: Wrapper },
);
await screen.findByText("Stale value");
server.use(
http.get(entryUrl, () => HttpResponse.json({ detail: "XCom entry was deleted" }, { status: 404 })),
);
await act(() => queryClient.invalidateQueries());
expect(await screen.findByTestId("xcom-entry-error")).toHaveTextContent("404");
expect(screen.queryByText("Stale value")).not.toBeInTheDocument();
expect(screen.queryByRole("button", { name: /copy/iu })).not.toBeInTheDocument();
});
it("keeps new error details closed after a successful refetch", async () => {
server.use(
http.get(entryUrl, () => HttpResponse.json({ detail: "XCom entry was deleted" }, { status: 404 })),
);
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
render(
<QueryClientProvider client={queryClient}>
<XComEntry {...entryProps} xcomKey="return_value" />
</QueryClientProvider>,
{ wrapper: Wrapper },
);
fireEvent.click(await screen.findByRole("button", { name: `${errorTitle} 404` }));
await screen.findByRole("dialog", { name: errorTitle });
server.use(
http.get(entryUrl, () => HttpResponse.json({ key: "return_value", value: "Recovered value" })),
);
await act(() => queryClient.invalidateQueries());
await screen.findByText("Recovered value");
expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
server.use(
http.get(entryUrl, () =>
HttpResponse.json({ detail: "Unable to deserialize the XCom value" }, { status: 500 }),
),
);
await act(() => queryClient.invalidateQueries());
expect(await screen.findByRole("button", { name: `${errorTitle} 500` })).toBeInTheDocument();
expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
});
it("exposes the full validation error for structured details", async () => {
server.use(
http.get(entryUrl, () =>
HttpResponse.json(
{ detail: [{ loc: ["query", "map_index"], msg: "Invalid map index", type: "value_error" }] },
{ status: 422 },
),
),
);
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
render(
<QueryClientProvider client={queryClient}>
<XComEntry {...entryProps} xcomKey="failed_entry" />
</QueryClientProvider>,
{ wrapper: Wrapper },
);
fireEvent.click(await screen.findByRole("button", { name: `${errorTitle} 422` }));
const dialog = await screen.findByRole("dialog", { name: errorTitle });
expect(within(dialog).getByTestId("error-alert")).toHaveTextContent("query.map_index Invalid map index");
});
it("shows a network error without an HTTP status or copy button", async () => {
server.use(http.get(entryUrl, () => HttpResponse.error()));
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
render(
<QueryClientProvider client={queryClient}>
<XComEntry {...entryProps} xcomKey="failed_entry" />
</QueryClientProvider>,
{ wrapper: Wrapper },
);
const trigger = await screen.findByRole("button", { name: errorTitle });
expect(trigger).toHaveTextContent(errorTitle);
expect(trigger).not.toHaveTextContent("undefined");
expect(screen.queryByRole("button", { name: /copy/iu })).not.toBeInTheDocument();
fireEvent.click(trigger);
const dialog = await screen.findByRole("dialog", { name: errorTitle });
expect(within(dialog).getByTestId("error-alert")).toHaveTextContent("Network Error");
});
});
@@ -16,13 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
import { Skeleton, HStack, Text, Link } from "@chakra-ui/react";
import { useState } from "react";
import { Skeleton, Button, HStack, Text, Link } from "@chakra-ui/react";
import { useTranslation } from "react-i18next";
import { FiAlertCircle } from "react-icons/fi";
import { useXcomServiceGetXcomEntry } from "openapi/queries";
import type { XComResponseNative } from "openapi/requests/types.gen";
import { ClipboardIconButton, ClipboardRoot } from "src/system-components";
import { ErrorAlert, type ExpandedApiError } from "src/components/ErrorAlert";
import { ErrorModal } from "src/components/ErrorModal";
import RenderedJsonField from "src/components/RenderedJsonField";
import { urlRegex } from "src/constants/urlRegex";
@@ -66,8 +72,43 @@ const renderTextWithLinks = (text: string) => {
);
};
const XComError = ({ error }: { readonly error: unknown }) => {
const { t: translate } = useTranslation("common");
const [errorOpen, setErrorOpen] = useState(false);
const { status } = error as Partial<ExpandedApiError>;
const errorTitle = translate("error.title");
return (
<>
<Button
aria-haspopup="dialog"
aria-label={status === undefined ? errorTitle : `${errorTitle} ${String(status)}`}
color="fg.error"
data-testid="xcom-entry-error"
fontSize="sm"
gap={1}
onClick={() => setErrorOpen(true)}
px={0}
size="xs"
variant="plain"
>
<FiAlertCircle aria-hidden="true" />
{status ?? errorTitle}
</Button>
<ErrorModal
icon={<FiAlertCircle />}
onClose={() => setErrorOpen(false)}
open={errorOpen}
title={errorTitle}
>
<ErrorAlert error={error} />
</ErrorModal>
</>
);
};
export const XComEntry = ({ dagId, mapIndex, open = false, runId, taskId, xcomKey }: XComEntryProps) => {
const { data, isLoading } = useXcomServiceGetXcomEntry<XComResponseNative>({
const { data, error, isLoading } = useXcomServiceGetXcomEntry<XComResponseNative>({
dagId,
dagRunId: runId,
deserialize: true,
@@ -76,6 +117,11 @@ export const XComEntry = ({ dagId, mapIndex, open = false, runId, taskId, xcomKe
taskId,
xcomKey,
});
if (Boolean(error)) {
return <XComError error={error} />;
}
// When deserialize=true, the API returns a stringified representation
// so we don't need to JSON.stringify it again
const xcomValue = data?.value;