diff --git a/packages/ui/src/features/task-detail/components/TaskLogsPanel.tsx b/packages/ui/src/features/task-detail/components/TaskLogsPanel.tsx index 1cc7283356..e39f7fd28f 100644 --- a/packages/ui/src/features/task-detail/components/TaskLogsPanel.tsx +++ b/packages/ui/src/features/task-detail/components/TaskLogsPanel.tsx @@ -3,7 +3,7 @@ import type { Task } from "@posthog/shared/domain-types"; import { Box, Flex } from "@radix-ui/themes"; import { useCallback, useEffect } from "react"; import { BackgroundWrapper } from "../../../primitives/BackgroundWrapper"; -import { ErrorBoundary } from "../../../primitives/ErrorBoundary"; +import { ErrorBoundary } from "../../../shell/ErrorBoundary"; import { useHostCapabilities } from "../../../shell/useHostCapabilities"; import { useFolders } from "../../folders/useFolders"; import { useDraftStore } from "../../message-editor/draftStore"; @@ -162,7 +162,11 @@ export function TaskLogsPanel({ taskId, task, hideInput }: TaskLogsPanelProps) { - + {/* The chat subtree has historically tripped a transient React #185 + render loop (see issue #2165) that clears on remount — reported via + telemetry (shell wrapper) and auto-recovered so a re-viewed task + heals itself instead of stranding the user on the error fallback. */} + recovered; +} + +function boundary(props: Partial): ReactNode { + return ( + + + + + + ); +} + +beforeEach(() => { + control.shouldThrow = true; + // React logs caught errors to console.error; silence the expected noise. + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("ErrorBoundary autoRecover", () => { + it("replicates the defect: without autoRecover the fallback stays even after the transient condition clears (no resetKey change, no manual retry)", () => { + const { rerender } = render(boundary({ resetKey: "task-a" })); + expect(screen.getByText("Something went wrong")).toBeInTheDocument(); + + // The transient loop condition clears, but navigating back to the SAME + // task doesn't change resetKey — the boundary is stuck on the error. + control.shouldThrow = false; + rerender(boundary({ resetKey: "task-a" })); + + expect(screen.getByText("Something went wrong")).toBeInTheDocument(); + expect(screen.queryByText("recovered")).not.toBeInTheDocument(); + }); + + it("auto-recovers a transient error without a manual retry, and reports it first", async () => { + const onError = vi.fn(); + render(boundary({ autoRecover: true, onError })); + + // While the bounded recovery is pending it renders nothing — no flash of + // the scary error UI for a one-frame transient — but it HAS reported it. + expect(screen.queryByText("Something went wrong")).not.toBeInTheDocument(); + expect(screen.queryByText("recovered")).not.toBeInTheDocument(); + expect(onError).toHaveBeenCalledTimes(1); + + // The transient condition clears; the scheduled reset then re-renders the + // healthy child on its own. + control.shouldThrow = false; + expect(await screen.findByText("recovered")).toBeInTheDocument(); + expect(screen.queryByText("Something went wrong")).not.toBeInTheDocument(); + }); + + it("is bounded: a persistent loop lands on the manual fallback instead of retrying forever", async () => { + const onError = vi.fn(); + render(boundary({ autoRecover: true, onError })); + + // The child keeps throwing; after the bounded retries are spent the manual + // fallback appears rather than an unbounded catch/retry storm. + expect(await screen.findByText("Something went wrong")).toBeInTheDocument(); + expect(screen.queryByText("recovered")).not.toBeInTheDocument(); + // 1 initial catch + at most MAX_AUTO_RECOVERIES (2) re-throws. + expect(onError.mock.calls.length).toBeGreaterThanOrEqual(1); + expect(onError.mock.calls.length).toBeLessThanOrEqual(3); + + // And it stays settled on the fallback — no further retries fire. + const callsAfterSettle = onError.mock.calls.length; + await new Promise((resolve) => setTimeout(resolve, 500)); + expect(onError.mock.calls.length).toBe(callsAfterSettle); + expect(screen.getByText("Something went wrong")).toBeInTheDocument(); + }); + + it("manual retry still recovers after auto-recovery is exhausted", async () => { + const user = userEvent.setup(); + render(boundary({ autoRecover: true })); + expect(await screen.findByText("Something went wrong")).toBeInTheDocument(); + + control.shouldThrow = false; + await user.click(screen.getByRole("button", { name: /try again/i })); + + await waitFor(() => + expect(screen.getByText("recovered")).toBeInTheDocument(), + ); + }); +}); diff --git a/packages/ui/src/primitives/ErrorBoundary.tsx b/packages/ui/src/primitives/ErrorBoundary.tsx index 27e0772679..4ce873107e 100644 --- a/packages/ui/src/primitives/ErrorBoundary.tsx +++ b/packages/ui/src/primitives/ErrorBoundary.tsx @@ -2,6 +2,20 @@ import { Warning } from "@phosphor-icons/react"; import { Box, Button, Callout, Flex, Text } from "@radix-ui/themes"; import { Component, type ErrorInfo, type ReactNode } from "react"; +// A transient render-loop error (React #185 "Maximum update depth exceeded" from +// a layout-timing setState cycle) trips once, then clears on the next mount — +// which is why clicking "Try again" has always recovered it. `autoRecover` +// replays that click for the user: after catching, it schedules a bounded number +// of resets so a transient error heals itself, while a persistent one still +// lands on the manual fallback instead of thrashing. The error is always +// reported via `onError` first, so root-cause visibility is unaffected. +const MAX_AUTO_RECOVERIES = 2; +const AUTO_RECOVER_BASE_DELAY_MS = 150; +// Once the subtree has rendered cleanly for this long, treat it as healthy and +// refill the recovery budget so a later, unrelated transient error gets its own +// retries rather than being starved by an earlier one. +const RECOVERY_STABLE_RESET_MS = 4000; + export interface ErrorBoundaryProps { children: ReactNode; fallback?: ReactNode; @@ -9,6 +23,16 @@ export interface ErrorBoundaryProps { name?: string; /** When this value changes, the boundary clears its error state. */ resetKey?: unknown; + /** + * Auto-clear a caught error a bounded number of times before showing the + * manual retry fallback. Use for subtrees prone to transient render-loop + * errors that clear on remount (e.g. the chat SessionView, whose #185 loop + * has always been recoverable via "Try again"). While an auto-recovery is + * pending the boundary renders nothing rather than flashing the error UI. The + * error is still reported via `onError` on every catch, so observability is + * unaffected. Off by default. + */ + autoRecover?: boolean; /** * If returns true for a caught error, the boundary renders nothing, * skips the fallback UI, and waits for `resetKey` to change before @@ -30,10 +54,21 @@ export interface ErrorBoundaryProps { interface State { error: Error | null; lastResetKey: unknown; + /** How many times autoRecover has cleared an error since the last healthy + * stretch / resetKey change. Caps auto-recovery so a persistent loop can't + * thrash the boundary. */ + recoveryAttempts: number; } export class ErrorBoundary extends Component { - state: State = { error: null, lastResetKey: this.props.resetKey }; + state: State = { + error: null, + lastResetKey: this.props.resetKey, + recoveryAttempts: 0, + }; + + private recoverTimer: ReturnType | null = null; + private stableTimer: ReturnType | null = null; static getDerivedStateFromError(error: Error): Partial { return { error }; @@ -44,7 +79,9 @@ export class ErrorBoundary extends Component { state: State, ): Partial | null { if (props.resetKey === state.lastResetKey) return null; - return { error: null, lastResetKey: props.resetKey }; + // A genuine new context (e.g. a different task): clear the error and refill + // the auto-recovery budget. + return { error: null, lastResetKey: props.resetKey, recoveryAttempts: 0 }; } componentDidCatch(error: Error, errorInfo: ErrorInfo) { @@ -53,16 +90,79 @@ export class ErrorBoundary extends Component { componentStack: errorInfo.componentStack, suppressed, }); + if (suppressed) return; + // A fresh error means the subtree isn't healthy yet — cancel any pending + // budget refill and, if enabled, schedule the next bounded auto-recovery. + this.clearStableTimer(); + if (this.props.autoRecover) this.scheduleAutoRecover(); + } + + componentDidUpdate(_prevProps: ErrorBoundaryProps, prevState: State) { + // The error just cleared (auto-recovery or manual retry). If it doesn't + // recur within the stability window, refill the recovery budget so a later + // unrelated transient error isn't starved of retries. + if (this.props.autoRecover && prevState.error && !this.state.error) { + this.clearStableTimer(); + this.stableTimer = setTimeout(() => { + this.stableTimer = null; + if (this.state.recoveryAttempts !== 0) { + this.setState({ recoveryAttempts: 0 }); + } + }, RECOVERY_STABLE_RESET_MS); + } + } + + componentWillUnmount() { + this.clearRecoverTimer(); + this.clearStableTimer(); + } + + private scheduleAutoRecover(): void { + if (this.recoverTimer !== null) return; + if (this.state.recoveryAttempts >= MAX_AUTO_RECOVERIES) return; + // Back off a little per attempt so a loop that needs a beat to settle gets + // progressively more time before we give up to the manual fallback. + const delay = + AUTO_RECOVER_BASE_DELAY_MS * (this.state.recoveryAttempts + 1); + this.recoverTimer = setTimeout(() => { + this.recoverTimer = null; + this.setState((s) => + s.error + ? { error: null, recoveryAttempts: s.recoveryAttempts + 1 } + : null, + ); + }, delay); + } + + private clearRecoverTimer(): void { + if (this.recoverTimer !== null) { + clearTimeout(this.recoverTimer); + this.recoverTimer = null; + } + } + + private clearStableTimer(): void { + if (this.stableTimer !== null) { + clearTimeout(this.stableTimer); + this.stableTimer = null; + } } handleRetry = () => { - this.setState({ error: null }); + this.clearRecoverTimer(); + this.setState({ error: null, recoveryAttempts: 0 }); }; render() { - const { error } = this.state; + const { error, recoveryAttempts } = this.state; if (!error) return this.props.children; if (this.props.shouldSuppress?.(error)) return null; + // While a bounded auto-recovery is still pending, render nothing rather than + // flashing the error UI for what is (usually) a one-frame transient loop. + // componentDidCatch has scheduled the reset that will re-render the children. + if (this.props.autoRecover && recoveryAttempts < MAX_AUTO_RECOVERIES) { + return null; + } if (this.props.fallback) return this.props.fallback; return (