From b75a3048388b2c9090a5f9500acbc58fa90f01eb Mon Sep 17 00:00:00 2001 From: Kiran Gadhave Date: Tue, 15 Sep 2026 20:23:50 -0700 Subject: [PATCH 1/5] feat: tolerate a missing request client in the issue-report modal FeedbackModal now reads a nullable request client so a crash-time report still produces partial environment details. Source collection and environment retry stay disabled until a client exists. --- .../__tests__/feedback-button.test.tsx | 90 +++++++++++++++++++ .../chrome/components/feedback-button.tsx | 51 ++++++----- 2 files changed, 119 insertions(+), 22 deletions(-) diff --git a/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx b/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx index 2a0ce95474d..31297ae62e1 100644 --- a/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx +++ b/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx @@ -208,4 +208,94 @@ describe("FeedbackModal issue reporting", () => { expect(href).toContain(encodeURIComponent('"marimo": "1.2.3"')); }); }); + + it("shows partial environment details when the request client is missing", async () => { + store.set(requestClientAtom, null); + render(, { wrapper }); + + await screen.findByText("Environment details"); + expect( + screen.getByText("Server environment information unavailable"), + ).toBeInTheDocument(); + expect( + screen.queryByText("Loading environment details…"), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Retry" }), + ).not.toBeInTheDocument(); + + const link = screen.getByRole("link", { name: "Open GitHub issue" }); + const href = link.getAttribute("href") ?? ""; + expect(href).toContain("&env="); + expect(href).toContain(encodeURIComponent("Environment Collection Error")); + + fireEvent.click( + screen.getByRole("button", { name: "Copy environment JSON" }), + ); + await waitFor(() => + expect(copyModule.copyToClipboard).toHaveBeenCalledWith( + expect.stringContaining("Environment Collection Error"), + ), + ); + }); + + it("disables source collection when the request client is missing", async () => { + localStorage.setItem( + "marimo:issue-report:include-code", + JSON.stringify(true), + ); + store.set(requestClientAtom, null); + render(, { wrapper }); + + await screen.findByText("Environment details"); + expect( + screen.getByRole("checkbox", { name: "Include notebook code" }), + ).toBeDisabled(); + expect( + screen.getByText("Notebook source is unavailable."), + ).toBeInTheDocument(); + const link = screen.getByRole("link", { name: "Open GitHub issue" }); + expect(link.getAttribute("href") ?? "").not.toContain("reproduction-code="); + }); + + it("preserves partial diagnostics when the environment request throws", async () => { + store.set( + requestClientAtom, + MockRequestClient.create({ + getEnvironmentInfo: vi.fn().mockImplementation(() => { + throw new Error("offline"); + }), + }), + ); + render(, { wrapper }); + + await screen.findByText("Server environment information unavailable"); + expect(screen.getByRole("button", { name: "Retry" })).toBeInTheDocument(); + expect( + screen.getByText(/Environment Collection Error/), + ).toBeInTheDocument(); + }); + + it("replaces partial diagnostics after a successful environment retry", async () => { + const getEnvironmentInfo = vi + .fn() + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValueOnce(environment); + store.set( + requestClientAtom, + MockRequestClient.create({ getEnvironmentInfo }), + ); + render(, { wrapper }); + + await screen.findByText("Server environment information unavailable"); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + + await screen.findByText(/"marimo": "1.2.3"/); + expect( + screen.queryByText("Server environment information unavailable"), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Retry" }), + ).not.toBeInTheDocument(); + }); }); diff --git a/frontend/src/components/editor/chrome/components/feedback-button.tsx b/frontend/src/components/editor/chrome/components/feedback-button.tsx index 2818f2497ff..127fa4ca60d 100644 --- a/frontend/src/components/editor/chrome/components/feedback-button.tsx +++ b/frontend/src/components/editor/chrome/components/feedback-button.tsx @@ -36,7 +36,7 @@ import { import { useNotebookCodeAvailable } from "@/core/meta/code-visibility"; import { getMarimoVersion } from "@/core/meta/globals"; import { connectionAtom } from "@/core/network/connection"; -import { useRequestClient } from "@/core/network/requests"; +import { requestClientAtom } from "@/core/network/requests"; import { filenameAtom } from "@/core/saving/file-state"; import { store } from "@/core/state/jotai"; import { WebSocketState } from "@/core/websocket/types"; @@ -87,11 +87,13 @@ export const FeedbackButton: React.FC = ({ children }) => { export const FeedbackModal: React.FC<{ onClose: () => void; }> = () => { - const { getEnvironmentInfo, readCode } = useRequestClient(); - const environmentRequest = useAsyncData( - async () => getEnvironmentInfo(), - [getEnvironmentInfo], - ); + const requestClient = useAtomValue(requestClientAtom); + const environmentRequest = useAsyncData(async () => { + if (requestClient == null) { + return undefined; + } + return requestClient.getEnvironmentInfo(); + }, [requestClient]); const notebook = useAtomValue(notebookAtom); const errors = getCellErrorEntries(store); @@ -105,7 +107,8 @@ export const FeedbackModal: React.FC<{ const notebookSourceAvailable = filename !== null && codeAvailable && - connection.state === WebSocketState.OPEN; + connection.state === WebSocketState.OPEN && + requestClient != null; const notebookSourceReason = notebookSourceAvailable ? undefined @@ -113,7 +116,9 @@ export const FeedbackModal: React.FC<{ ? "Save the notebook first." : !codeAvailable ? "Notebook source is hidden in this view." - : "Connect the notebook to include its source."; + : connection.state !== WebSocketState.OPEN + ? "Connect the notebook to include its source." + : "Notebook source is unavailable."; const [includeErrors, setIncludeErrors] = useLocalStorage( "marimo:issue-report:include-errors", @@ -127,7 +132,7 @@ export const FeedbackModal: React.FC<{ const environment: EnvironmentDiagnostics | undefined = environmentRequest.data ? enrichEnvironment(environmentRequest.data, navigator.userAgent) - : environmentRequest.status === "error" + : environmentRequest.status === "error" || requestClient == null ? createPartialEnvironment( getMarimoVersion(), navigator.userAgent, @@ -137,12 +142,12 @@ export const FeedbackModal: React.FC<{ : undefined; const codeRequest = useAsyncData(async () => { - if (!includeCode || !notebookSourceAvailable) { + if (!includeCode || !notebookSourceAvailable || requestClient == null) { return undefined; } - const { contents } = await readCode(); + const { contents } = await requestClient.readCode(); return contents; - }, [includeCode, notebookSourceAvailable, readCode]); + }, [includeCode, notebookSourceAvailable, requestClient]); let githubIssueUrl = Constants.bugReportUrl; let omitted: string[] = []; @@ -255,7 +260,7 @@ export const FeedbackModal: React.FC<{ )} - {environmentRequest.status === "pending" && ( + {environmentRequest.status === "pending" && requestClient != null && (
Loading environment details… @@ -266,18 +271,20 @@ export const FeedbackModal: React.FC<{
)} - {environmentRequest.status === "error" && ( + {(environmentRequest.status === "error" || requestClient == null) && (
Server environment information unavailable - + {requestClient != null && ( + + )}
)} From 19dbd2738660d5bd51982290695607f7582cf37e Mon Sep 17 00:00:00 2001 From: Kiran Gadhave Date: Tue, 15 Sep 2026 20:45:49 -0700 Subject: [PATCH 2/5] feat: open the issue-report modal from the error fallback The shared error screen now owns a local dialog and mounts FeedbackModal with the application store, because root crashes remove ancestor modal and tooltip providers. --- .../editor/boundary/ErrorBoundary.tsx | 45 +++++++---- .../boundary/__tests__/ErrorBoundary.test.tsx | 78 +++++++++++++++++++ 2 files changed, 107 insertions(+), 16 deletions(-) create mode 100644 frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx diff --git a/frontend/src/components/editor/boundary/ErrorBoundary.tsx b/frontend/src/components/editor/boundary/ErrorBoundary.tsx index 96d6c4a0742..d999cbc307d 100644 --- a/frontend/src/components/editor/boundary/ErrorBoundary.tsx +++ b/frontend/src/components/editor/boundary/ErrorBoundary.tsx @@ -1,11 +1,15 @@ /* Copyright 2026 Marimo. All rights reserved. */ -import type { PropsWithChildren } from "react"; +import { Provider } from "jotai"; +import { type PropsWithChildren, useState } from "react"; import { type FallbackProps, ErrorBoundary as ReactErrorBoundary, } from "react-error-boundary"; -import { Constants } from "@/core/constants"; +import { store } from "@/core/state/jotai"; import { Button } from "../../ui/button"; +import { Dialog, DialogTrigger } from "../../ui/dialog"; +import { TooltipProvider } from "../../ui/tooltip"; +import { FeedbackModal } from "../chrome/components/feedback-button"; export const ErrorBoundary: React.FC = (props) => { return ( @@ -16,26 +20,35 @@ export const ErrorBoundary: React.FC = (props) => { }; const FallbackComponent: React.FC = (props) => { + const [open, setOpen] = useState(false); + return ( -
+

Something went wrong

         {props.error?.message}
-
- If this is an issue with marimo, please report it on{" "} - - GitHub - - . +
+ + + + + {open && ( + + + setOpen(false)} /> + + + )} + +
-
); }; diff --git a/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx b/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx new file mode 100644 index 00000000000..f1e1de390b7 --- /dev/null +++ b/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx @@ -0,0 +1,78 @@ +/* Copyright 2026 Marimo. All rights reserved. */ + +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { MockRequestClient } from "@/__mocks__/requests"; +import { viewStateAtom } from "@/core/mode"; +import { connectionAtom } from "@/core/network/connection"; +import { requestClientAtom } from "@/core/network/requests"; +import type { EnvironmentInfo } from "@/core/network/types"; +import { filenameAtom } from "@/core/saving/file-state"; +import { store } from "@/core/state/jotai"; +import { WebSocketState } from "@/core/websocket/types"; +import { ErrorBoundary } from "../ErrorBoundary"; + +const environment: EnvironmentInfo = { + marimo: "1.2.3", + editable: false, + location: "~/.venv/site-packages/marimo", + OS: "Darwin", + "OS Version": "25.0", + Processor: "arm", + "Python Version": "3.12.9", + Locale: "en_US", + Binaries: { Browser: "chrome 140", Node: "v22", uv: "0.11" }, + Dependencies: { click: "8.4.2" }, + "Optional Dependencies": { pandas: "3.0.0" }, + "Experimental Flags": {}, +}; + +function CrashingChild(): never { + throw new Error("cell output crashed"); +} + +describe("ErrorBoundary report dialog", () => { + beforeEach(() => { + vi.clearAllMocks(); + localStorage.clear(); + store.set(viewStateAtom, { mode: "edit", cellAnchor: null }); + store.set(connectionAtom, { state: WebSocketState.OPEN }); + store.set(filenameAtom, "/project/example.py"); + }); + + afterEach(() => { + store.set(requestClientAtom, null); + }); + + it("opens the report modal after a child render exception", async () => { + const getEnvironmentInfo = vi.fn().mockResolvedValue(environment); + store.set( + requestClientAtom, + MockRequestClient.create({ getEnvironmentInfo }), + ); + + const consoleError = vi + .spyOn(console, "error") + .mockImplementation(() => {}); + render( + + + , + ); + + expect(screen.getByText("Something went wrong")).toBeVisible(); + expect(screen.getByText("cell output crashed")).toBeVisible(); + expect(screen.getByTestId("reset-error-boundary-button")).toBeVisible(); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(getEnvironmentInfo).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByRole("button", { name: "Report an issue" })); + + const dialog = await screen.findByRole("dialog"); + await screen.findByText("Environment details"); + expect(dialog).toHaveTextContent("Report an issue"); + await waitFor(() => expect(getEnvironmentInfo).toHaveBeenCalledOnce()); + + consoleError.mockRestore(); + }); +}); From 1b3af762fb8c66135880238864795f2b0f61c689 Mon Sep 17 00:00:00 2001 From: Kiran Gadhave Date: Wed, 16 Sep 2026 08:48:08 -0700 Subject: [PATCH 3/5] test: cover error fallback report dialog recovery Boundary tests exercise provider loss, a missing client, closed-dialog request skipping, focus restore, retry, and source opt-in. --- .../boundary/__tests__/ErrorBoundary.test.tsx | 267 ++++++++++++++++-- 1 file changed, 242 insertions(+), 25 deletions(-) diff --git a/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx b/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx index f1e1de390b7..b68cf67b129 100644 --- a/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx +++ b/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx @@ -1,8 +1,12 @@ /* Copyright 2026 Marimo. All rights reserved. */ import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import type { PropsWithChildren, ReactNode } from "react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { MockRequestClient } from "@/__mocks__/requests"; +import { ModalProvider } from "@/components/modal/ImperativeModal"; +import { TooltipProvider } from "@/components/ui/tooltip"; +import { initialNotebookState, notebookAtom } from "@/core/cells/cells"; import { viewStateAtom } from "@/core/mode"; import { connectionAtom } from "@/core/network/connection"; import { requestClientAtom } from "@/core/network/requests"; @@ -12,6 +16,10 @@ import { store } from "@/core/state/jotai"; import { WebSocketState } from "@/core/websocket/types"; import { ErrorBoundary } from "../ErrorBoundary"; +vi.mock("@/utils/copy", () => ({ + copyToClipboard: vi.fn().mockResolvedValue(undefined), +})); + const environment: EnvironmentInfo = { marimo: "1.2.3", editable: false, @@ -27,38 +35,134 @@ const environment: EnvironmentInfo = { "Experimental Flags": {}, }; +const CRASH_MESSAGE = "cell output crashed"; + function CrashingChild(): never { - throw new Error("cell output crashed"); + throw new Error(CRASH_MESSAGE); +} + +function RecoverableChild({ crash }: { crash: boolean }) { + if (crash) { + throw new Error(CRASH_MESSAGE); + } + return

recovered child

; +} + +function DescendantProviders({ children }: PropsWithChildren) { + return ( + + {children} + + ); +} + +function isExpectedReactExceptionOutput(args: unknown[]): boolean { + return args.some((arg) => { + if (arg instanceof Error) { + return arg.message.includes(CRASH_MESSAGE); + } + return ( + typeof arg === "string" && + (arg.includes(CRASH_MESSAGE) || + arg.includes("The above error occurred") || + arg.includes("React will try to recreate this component tree")) + ); + }); +} + +function suppressExpectedReactException() { + return vi.spyOn(console, "error").mockImplementation((...args) => { + if (isExpectedReactExceptionOutput(args)) { + return; + } + throw new Error(`Unexpected console.error: ${args.map(String).join(" ")}`); + }); +} + +let consoleError: ReturnType | undefined; + +function renderCrashedBoundary(child: ReactNode = ) { + consoleError?.mockRestore(); + consoleError = suppressExpectedReactException(); + return render({child}); +} + +function setRequestClient( + overrides?: Parameters[0], +) { + const client = MockRequestClient.create(overrides); + store.set(requestClientAtom, client); + return client; +} + +async function openReportDialog() { + const trigger = screen.getByRole("button", { name: "Report an issue" }); + trigger.focus(); + fireEvent.click(trigger); + return screen.findByRole("dialog"); +} + +async function expectReportButtonFocused() { + await waitFor(() => { + expect( + screen.getByRole("button", { name: "Report an issue" }), + ).toHaveFocus(); + }); +} + +async function closeReportDialog(method: "escape" | "close") { + const dialog = await screen.findByRole("dialog"); + if (method === "escape") { + fireEvent.keyDown(dialog, { key: "Escape" }); + } else { + fireEvent.click(screen.getByRole("button", { name: "Close" })); + } + await waitFor(() => { + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + await expectReportButtonFocused(); +} + +async function expectRealReportModal() { + const dialog = await openReportDialog(); + expect(dialog).toHaveTextContent("Report an issue"); + expect(screen.getByRole("link", { name: "Open GitHub issue" })).toBeVisible(); + expect( + screen.getByRole("checkbox", { name: "Include errors" }), + ).toBeVisible(); + expect( + screen.getByRole("checkbox", { name: "Include notebook code" }), + ).toBeVisible(); + await screen.findByText("Environment details"); + return dialog; +} + +function resetSharedState() { + localStorage.clear(); + store.set(requestClientAtom, null); + store.set(notebookAtom, initialNotebookState()); + store.set(viewStateAtom, { mode: "edit", cellAnchor: null }); + store.set(connectionAtom, { state: WebSocketState.OPEN }); + store.set(filenameAtom, "/project/example.py"); } describe("ErrorBoundary report dialog", () => { beforeEach(() => { - vi.clearAllMocks(); - localStorage.clear(); - store.set(viewStateAtom, { mode: "edit", cellAnchor: null }); - store.set(connectionAtom, { state: WebSocketState.OPEN }); - store.set(filenameAtom, "/project/example.py"); + resetSharedState(); }); afterEach(() => { - store.set(requestClientAtom, null); + consoleError?.mockRestore(); + consoleError = undefined; + resetSharedState(); }); it("opens the report modal after a child render exception", async () => { - const getEnvironmentInfo = vi.fn().mockResolvedValue(environment); - store.set( - requestClientAtom, - MockRequestClient.create({ getEnvironmentInfo }), - ); + const { getEnvironmentInfo } = setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + }); - const consoleError = vi - .spyOn(console, "error") - .mockImplementation(() => {}); - render( - - - , - ); + renderCrashedBoundary(); expect(screen.getByText("Something went wrong")).toBeVisible(); expect(screen.getByText("cell output crashed")).toBeVisible(); @@ -66,13 +170,126 @@ describe("ErrorBoundary report dialog", () => { expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); expect(getEnvironmentInfo).not.toHaveBeenCalled(); - fireEvent.click(screen.getByRole("button", { name: "Report an issue" })); + await expectRealReportModal(); + await waitFor(() => expect(getEnvironmentInfo).toHaveBeenCalledOnce()); + }); - const dialog = await screen.findByRole("dialog"); - await screen.findByText("Environment details"); - expect(dialog).toHaveTextContent("Report an issue"); + it("still opens one report dialog after descendant providers disappear", async () => { + const { getEnvironmentInfo } = setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + }); + + renderCrashedBoundary( + + + , + ); + + await expectRealReportModal(); + expect(screen.getAllByRole("dialog")).toHaveLength(1); await waitFor(() => expect(getEnvironmentInfo).toHaveBeenCalledOnce()); + }); + + it("shows partial diagnostics when the request client is missing", async () => { + renderCrashedBoundary(); + + await openReportDialog(); + await screen.findByText("Environment details"); + + expect( + screen.getByText("Server environment information unavailable"), + ).toBeVisible(); + expect( + screen.queryByText("Loading environment details…"), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Retry" }), + ).not.toBeInTheDocument(); + expect( + screen.getByRole("checkbox", { name: "Include notebook code" }), + ).toBeDisabled(); + expect(screen.getByText("Notebook source is unavailable.")).toBeVisible(); + const link = screen.getByRole("link", { name: "Open GitHub issue" }); + expect(link.getAttribute("href") ?? "").toContain("&env="); + }); + + it("does not request environment or source while the report dialog is closed", async () => { + localStorage.setItem( + "marimo:issue-report:include-code", + JSON.stringify(true), + ); + const { getEnvironmentInfo, readCode } = setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + readCode: vi.fn().mockResolvedValue({ contents: "import marimo" }), + }); + + renderCrashedBoundary(); + + expect(screen.getByText("Something went wrong")).toBeVisible(); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(getEnvironmentInfo).not.toHaveBeenCalled(); + expect(readCode).not.toHaveBeenCalled(); + }); + + it("restores focus to the report button after closing with Escape", async () => { + setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + }); + renderCrashedBoundary(); + await openReportDialog(); + await closeReportDialog("escape"); + expect(screen.getByText(CRASH_MESSAGE)).toBeVisible(); + }); + + it("restores focus to the report button after closing with the close control", async () => { + setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + }); + renderCrashedBoundary(); + await openReportDialog(); + await closeReportDialog("close"); + expect(screen.getByText(CRASH_MESSAGE)).toBeVisible(); + }); + + it("reopens the report dialog while the original error remains", async () => { + setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + }); + renderCrashedBoundary(); + await openReportDialog(); + await closeReportDialog("escape"); + expect(screen.getByText(CRASH_MESSAGE)).toBeVisible(); + await expectRealReportModal(); + expect(screen.getByText(CRASH_MESSAGE)).toBeVisible(); + }); + + it("restores the child after try again once the failure is cleared", () => { + setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + }); + const view = renderCrashedBoundary(); + expect(screen.getByText("Something went wrong")).toBeVisible(); + + view.rerender( + + + , + ); + fireEvent.click(screen.getByTestId("reset-error-boundary-button")); + + expect(screen.getByText("recovered child")).toBeVisible(); + expect(screen.queryByText("Something went wrong")).not.toBeInTheDocument(); + }); + + it("does not read notebook source when opened with fresh inclusion preferences", async () => { + const { readCode } = setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + readCode: vi.fn().mockResolvedValue({ contents: "import marimo" }), + }); + + renderCrashedBoundary(); + await expectRealReportModal(); - consoleError.mockRestore(); + expect(readCode).not.toHaveBeenCalled(); }); }); From 86ede9e8b239a4903f6f4f4e89844bcae95e9b37 Mon Sep 17 00:00:00 2001 From: Kiran Gadhave Date: Wed, 16 Sep 2026 15:09:02 -0700 Subject: [PATCH 4/5] fix: restore copy toasts on the error fallback Mount a local Toaster after a root crash, lazy-load FeedbackModal so table tests do not hit a static-state mock TDZ, and explain a missing request client before connection state. --- .../components/editor/boundary/ErrorBoundary.tsx | 15 ++++++++++++--- .../boundary/__tests__/ErrorBoundary.test.tsx | 16 ++++++++++++++++ .../__tests__/feedback-button.test.tsx | 4 ++++ .../editor/chrome/components/feedback-button.tsx | 16 ++++++++-------- frontend/src/components/ui/use-toast.ts | 8 ++++++++ 5 files changed, 48 insertions(+), 11 deletions(-) diff --git a/frontend/src/components/editor/boundary/ErrorBoundary.tsx b/frontend/src/components/editor/boundary/ErrorBoundary.tsx index d999cbc307d..ca094a6f106 100644 --- a/frontend/src/components/editor/boundary/ErrorBoundary.tsx +++ b/frontend/src/components/editor/boundary/ErrorBoundary.tsx @@ -1,6 +1,6 @@ /* Copyright 2026 Marimo. All rights reserved. */ import { Provider } from "jotai"; -import { type PropsWithChildren, useState } from "react"; +import { type PropsWithChildren, lazy, Suspense, useState } from "react"; import { type FallbackProps, ErrorBoundary as ReactErrorBoundary, @@ -8,8 +8,14 @@ import { import { store } from "@/core/state/jotai"; import { Button } from "../../ui/button"; import { Dialog, DialogTrigger } from "../../ui/dialog"; +import { Toaster } from "../../ui/toaster"; import { TooltipProvider } from "../../ui/tooltip"; -import { FeedbackModal } from "../chrome/components/feedback-button"; + +const FeedbackModal = lazy(() => + import("../chrome/components/feedback-button").then((mod) => ({ + default: mod.FeedbackModal, + })), +); export const ErrorBoundary: React.FC = (props) => { return ( @@ -36,7 +42,10 @@ const FallbackComponent: React.FC = (props) => { {open && ( - setOpen(false)} /> + + + + )} diff --git a/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx b/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx index b68cf67b129..f98025b9d57 100644 --- a/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx +++ b/frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { MockRequestClient } from "@/__mocks__/requests"; import { ModalProvider } from "@/components/modal/ImperativeModal"; import { TooltipProvider } from "@/components/ui/tooltip"; +import { clearToasts } from "@/components/ui/use-toast"; import { initialNotebookState, notebookAtom } from "@/core/cells/cells"; import { viewStateAtom } from "@/core/mode"; import { connectionAtom } from "@/core/network/connection"; @@ -139,6 +140,7 @@ async function expectRealReportModal() { function resetSharedState() { localStorage.clear(); + clearToasts(); store.set(requestClientAtom, null); store.set(notebookAtom, initialNotebookState()); store.set(viewStateAtom, { mode: "edit", cellAnchor: null }); @@ -292,4 +294,18 @@ describe("ErrorBoundary report dialog", () => { expect(readCode).not.toHaveBeenCalled(); }); + + it("renders the copy toast after the ancestor toaster is gone", async () => { + setRequestClient({ + getEnvironmentInfo: vi.fn().mockResolvedValue(environment), + }); + renderCrashedBoundary(); + await expectRealReportModal(); + + fireEvent.click( + screen.getByRole("button", { name: "Copy environment JSON" }), + ); + + expect(await screen.findByText("Environment details copied")).toBeVisible(); + }); }); diff --git a/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx b/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx index 31297ae62e1..b5307c2f996 100644 --- a/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx +++ b/frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx @@ -244,6 +244,7 @@ describe("FeedbackModal issue reporting", () => { "marimo:issue-report:include-code", JSON.stringify(true), ); + store.set(connectionAtom, { state: WebSocketState.CONNECTING }); store.set(requestClientAtom, null); render(, { wrapper }); @@ -254,6 +255,9 @@ describe("FeedbackModal issue reporting", () => { expect( screen.getByText("Notebook source is unavailable."), ).toBeInTheDocument(); + expect( + screen.queryByText("Connect the notebook to include its source."), + ).not.toBeInTheDocument(); const link = screen.getByRole("link", { name: "Open GitHub issue" }); expect(link.getAttribute("href") ?? "").not.toContain("reproduction-code="); }); diff --git a/frontend/src/components/editor/chrome/components/feedback-button.tsx b/frontend/src/components/editor/chrome/components/feedback-button.tsx index 127fa4ca60d..0fa63d1ff59 100644 --- a/frontend/src/components/editor/chrome/components/feedback-button.tsx +++ b/frontend/src/components/editor/chrome/components/feedback-button.tsx @@ -85,7 +85,7 @@ export const FeedbackButton: React.FC = ({ children }) => { }; export const FeedbackModal: React.FC<{ - onClose: () => void; + onClose?: () => void; }> = () => { const requestClient = useAtomValue(requestClientAtom); const environmentRequest = useAsyncData(async () => { @@ -112,13 +112,13 @@ export const FeedbackModal: React.FC<{ const notebookSourceReason = notebookSourceAvailable ? undefined - : filename === null - ? "Save the notebook first." - : !codeAvailable - ? "Notebook source is hidden in this view." - : connection.state !== WebSocketState.OPEN - ? "Connect the notebook to include its source." - : "Notebook source is unavailable."; + : requestClient == null + ? "Notebook source is unavailable." + : filename === null + ? "Save the notebook first." + : !codeAvailable + ? "Notebook source is hidden in this view." + : "Connect the notebook to include its source."; const [includeErrors, setIncludeErrors] = useLocalStorage( "marimo:issue-report:include-errors", diff --git a/frontend/src/components/ui/use-toast.ts b/frontend/src/components/ui/use-toast.ts index 1b33c62da99..2f09e86a10c 100644 --- a/frontend/src/components/ui/use-toast.ts +++ b/frontend/src/components/ui/use-toast.ts @@ -161,6 +161,14 @@ function dispatch(action: Action) { }); } +export function clearToasts() { + for (const timeout of toastTimeouts.values()) { + clearTimeout(timeout); + } + toastTimeouts.clear(); + dispatch({ type: "REMOVE_TOAST" }); +} + type Toast = Omit; function toast({ From e15ed78b0c8b66655af40ca3a8b59609bcc5e772 Mon Sep 17 00:00:00 2001 From: Kiran Gadhave Date: Thu, 17 Sep 2026 10:20:16 -0700 Subject: [PATCH 5/5] refactor: flatten notebook source unavailable reasons Replace the nested ternary with an early-return helper so the missing-client explanation stays readable. --- .../chrome/components/feedback-button.tsx | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/frontend/src/components/editor/chrome/components/feedback-button.tsx b/frontend/src/components/editor/chrome/components/feedback-button.tsx index 0fa63d1ff59..ba1737a77e3 100644 --- a/frontend/src/components/editor/chrome/components/feedback-button.tsx +++ b/frontend/src/components/editor/chrome/components/feedback-button.tsx @@ -74,6 +74,27 @@ const CollapsiblePreview: React.FC<{ content: string }> = ({ content }) => { ); }; +function getNotebookSourceUnavailableReason(args: { + hasRequestClient: boolean; + filename: string | null; + codeAvailable: boolean; + connectionState: WebSocketState; +}): string | undefined { + if (!args.hasRequestClient) { + return "Notebook source is unavailable."; + } + if (args.filename === null) { + return "Save the notebook first."; + } + if (!args.codeAvailable) { + return "Notebook source is hidden in this view."; + } + if (args.connectionState !== WebSocketState.OPEN) { + return "Connect the notebook to include its source."; + } + return undefined; +} + export const FeedbackButton: React.FC = ({ children }) => { const { openModal, closeModal } = useImperativeModal(); @@ -104,21 +125,13 @@ export const FeedbackModal: React.FC<{ const codeAvailable = useNotebookCodeAvailable(cells); const filename = useAtomValue(filenameAtom); const connection = useAtomValue(connectionAtom); - const notebookSourceAvailable = - filename !== null && - codeAvailable && - connection.state === WebSocketState.OPEN && - requestClient != null; - - const notebookSourceReason = notebookSourceAvailable - ? undefined - : requestClient == null - ? "Notebook source is unavailable." - : filename === null - ? "Save the notebook first." - : !codeAvailable - ? "Notebook source is hidden in this view." - : "Connect the notebook to include its source."; + const notebookSourceReason = getNotebookSourceUnavailableReason({ + hasRequestClient: requestClient != null, + filename, + codeAvailable, + connectionState: connection.state, + }); + const notebookSourceAvailable = notebookSourceReason === undefined; const [includeErrors, setIncludeErrors] = useLocalStorage( "marimo:issue-report:include-errors",