Skip to content

Commit ff7e096

Browse files
authored
fix(sessions): auto-recover + report the chat SessionView render-loop crash
The task Chat panel sometimes shows "Something went wrong / Minified React error #185" (Maximum update depth exceeded) when navigating back to a task, and stays stuck on the fallback. Issue #2165 notes the loop is transient — "Try again recovers it" — so the render loop trips once and clears on the next mount. Two problems on `main`: - The SessionView boundary keys recovery on `resetKey={taskId}`, which doesn't change when you return to the same task, so a transient error strands the user until they manually retry. - That boundary was wired to the bare `ErrorBoundary` primitive (no `onError`), so these crashes no longer report to error tracking — the historical `boundary_name:"SessionView"` events came from older builds using the shell wrapper. Changes: - Add opt-in, bounded `autoRecover` to the `ErrorBoundary` primitive. On catch it reports via `onError` first, then schedules up to 2 self-resets (replaying the manual "Try again"); while pending it renders nothing instead of flashing the error UI. A persistent loop still lands on the manual fallback — no unbounded retry storm — and the budget refills after a healthy stretch. - Point the SessionView boundary at the telemetry-reporting shell `ErrorBoundary` and enable `autoRecover`, restoring observability and auto-healing the transient loop. Does not change the underlying (layout-timing) loop trigger, which needs the real app to reproduce; restored telemetry will confirm whether it still fires. Refs #2165 Generated-By: PostHog Code Task-Id: 24d17b80-ad39-4217-9eb6-dcc5ad043fb8
1 parent 0092213 commit ff7e096

3 files changed

Lines changed: 214 additions & 6 deletions

File tree

packages/ui/src/features/task-detail/components/TaskLogsPanel.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type { Task } from "@posthog/shared/domain-types";
33
import { Box, Flex } from "@radix-ui/themes";
44
import { useCallback, useEffect } from "react";
55
import { BackgroundWrapper } from "../../../primitives/BackgroundWrapper";
6-
import { ErrorBoundary } from "../../../primitives/ErrorBoundary";
6+
import { ErrorBoundary } from "../../../shell/ErrorBoundary";
77
import { useHostCapabilities } from "../../../shell/useHostCapabilities";
88
import { useFolders } from "../../folders/useFolders";
99
import { useDraftStore } from "../../message-editor/draftStore";
@@ -162,7 +162,11 @@ export function TaskLogsPanel({ taskId, task, hideInput }: TaskLogsPanelProps) {
162162
<BackgroundWrapper>
163163
<Flex direction="column" height="100%" width="100%">
164164
<Box className="min-h-0 flex-1">
165-
<ErrorBoundary name="SessionView" resetKey={taskId}>
165+
{/* The chat subtree has historically tripped a transient React #185
166+
render loop (see issue #2165) that clears on remount — reported via
167+
telemetry (shell wrapper) and auto-recovered so a re-viewed task
168+
heals itself instead of stranding the user on the error fallback. */}
169+
<ErrorBoundary name="SessionView" resetKey={taskId} autoRecover>
166170
<SessionView
167171
events={events}
168172
taskId={taskId}
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
import { Theme } from "@radix-ui/themes";
2+
import { render, screen, waitFor } from "@testing-library/react";
3+
import userEvent from "@testing-library/user-event";
4+
import type { ReactNode } from "react";
5+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
6+
import { ErrorBoundary, type ErrorBoundaryProps } from "./ErrorBoundary";
7+
8+
// External control so the child's throwing behaviour can change across the
9+
// boundary's OWN internal re-render — children don't re-render from a parent
10+
// while the fallback is showing, so a plain prop wouldn't take effect. This
11+
// models a transient render loop (React #185) that trips once and then clears
12+
// on the next mount, exactly the shape issue #2165 describes ("Try again
13+
// recovers it"). Uses real timers so the actual setTimeout-driven recovery is
14+
// exercised end to end.
15+
const control = { shouldThrow: true };
16+
17+
function Flaky() {
18+
if (control.shouldThrow) throw new Error("Maximum update depth exceeded");
19+
return <div>recovered</div>;
20+
}
21+
22+
function boundary(props: Partial<ErrorBoundaryProps>): ReactNode {
23+
return (
24+
<Theme>
25+
<ErrorBoundary {...props}>
26+
<Flaky />
27+
</ErrorBoundary>
28+
</Theme>
29+
);
30+
}
31+
32+
beforeEach(() => {
33+
control.shouldThrow = true;
34+
// React logs caught errors to console.error; silence the expected noise.
35+
vi.spyOn(console, "error").mockImplementation(() => {});
36+
});
37+
38+
afterEach(() => {
39+
vi.restoreAllMocks();
40+
});
41+
42+
describe("ErrorBoundary autoRecover", () => {
43+
it("replicates the defect: without autoRecover the fallback stays even after the transient condition clears (no resetKey change, no manual retry)", () => {
44+
const { rerender } = render(boundary({ resetKey: "task-a" }));
45+
expect(screen.getByText("Something went wrong")).toBeInTheDocument();
46+
47+
// The transient loop condition clears, but navigating back to the SAME
48+
// task doesn't change resetKey — the boundary is stuck on the error.
49+
control.shouldThrow = false;
50+
rerender(boundary({ resetKey: "task-a" }));
51+
52+
expect(screen.getByText("Something went wrong")).toBeInTheDocument();
53+
expect(screen.queryByText("recovered")).not.toBeInTheDocument();
54+
});
55+
56+
it("auto-recovers a transient error without a manual retry, and reports it first", async () => {
57+
const onError = vi.fn();
58+
render(boundary({ autoRecover: true, onError }));
59+
60+
// While the bounded recovery is pending it renders nothing — no flash of
61+
// the scary error UI for a one-frame transient — but it HAS reported it.
62+
expect(screen.queryByText("Something went wrong")).not.toBeInTheDocument();
63+
expect(screen.queryByText("recovered")).not.toBeInTheDocument();
64+
expect(onError).toHaveBeenCalledTimes(1);
65+
66+
// The transient condition clears; the scheduled reset then re-renders the
67+
// healthy child on its own.
68+
control.shouldThrow = false;
69+
expect(await screen.findByText("recovered")).toBeInTheDocument();
70+
expect(screen.queryByText("Something went wrong")).not.toBeInTheDocument();
71+
});
72+
73+
it("is bounded: a persistent loop lands on the manual fallback instead of retrying forever", async () => {
74+
const onError = vi.fn();
75+
render(boundary({ autoRecover: true, onError }));
76+
77+
// The child keeps throwing; after the bounded retries are spent the manual
78+
// fallback appears rather than an unbounded catch/retry storm.
79+
expect(await screen.findByText("Something went wrong")).toBeInTheDocument();
80+
expect(screen.queryByText("recovered")).not.toBeInTheDocument();
81+
// 1 initial catch + at most MAX_AUTO_RECOVERIES (2) re-throws.
82+
expect(onError.mock.calls.length).toBeGreaterThanOrEqual(1);
83+
expect(onError.mock.calls.length).toBeLessThanOrEqual(3);
84+
85+
// And it stays settled on the fallback — no further retries fire.
86+
const callsAfterSettle = onError.mock.calls.length;
87+
await new Promise((resolve) => setTimeout(resolve, 500));
88+
expect(onError.mock.calls.length).toBe(callsAfterSettle);
89+
expect(screen.getByText("Something went wrong")).toBeInTheDocument();
90+
});
91+
92+
it("manual retry still recovers after auto-recovery is exhausted", async () => {
93+
const user = userEvent.setup();
94+
render(boundary({ autoRecover: true }));
95+
expect(await screen.findByText("Something went wrong")).toBeInTheDocument();
96+
97+
control.shouldThrow = false;
98+
await user.click(screen.getByRole("button", { name: /try again/i }));
99+
100+
await waitFor(() =>
101+
expect(screen.getByText("recovered")).toBeInTheDocument(),
102+
);
103+
});
104+
});

packages/ui/src/primitives/ErrorBoundary.tsx

Lines changed: 104 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,37 @@ import { Warning } from "@phosphor-icons/react";
22
import { Box, Button, Callout, Flex, Text } from "@radix-ui/themes";
33
import { Component, type ErrorInfo, type ReactNode } from "react";
44

5+
// A transient render-loop error (React #185 "Maximum update depth exceeded" from
6+
// a layout-timing setState cycle) trips once, then clears on the next mount —
7+
// which is why clicking "Try again" has always recovered it. `autoRecover`
8+
// replays that click for the user: after catching, it schedules a bounded number
9+
// of resets so a transient error heals itself, while a persistent one still
10+
// lands on the manual fallback instead of thrashing. The error is always
11+
// reported via `onError` first, so root-cause visibility is unaffected.
12+
const MAX_AUTO_RECOVERIES = 2;
13+
const AUTO_RECOVER_BASE_DELAY_MS = 150;
14+
// Once the subtree has rendered cleanly for this long, treat it as healthy and
15+
// refill the recovery budget so a later, unrelated transient error gets its own
16+
// retries rather than being starved by an earlier one.
17+
const RECOVERY_STABLE_RESET_MS = 4000;
18+
519
export interface ErrorBoundaryProps {
620
children: ReactNode;
721
fallback?: ReactNode;
822
/** Optional name to identify which boundary caught the error */
923
name?: string;
1024
/** When this value changes, the boundary clears its error state. */
1125
resetKey?: unknown;
26+
/**
27+
* Auto-clear a caught error a bounded number of times before showing the
28+
* manual retry fallback. Use for subtrees prone to transient render-loop
29+
* errors that clear on remount (e.g. the chat SessionView, whose #185 loop
30+
* has always been recoverable via "Try again"). While an auto-recovery is
31+
* pending the boundary renders nothing rather than flashing the error UI. The
32+
* error is still reported via `onError` on every catch, so observability is
33+
* unaffected. Off by default.
34+
*/
35+
autoRecover?: boolean;
1236
/**
1337
* If returns true for a caught error, the boundary renders nothing,
1438
* skips the fallback UI, and waits for `resetKey` to change before
@@ -30,10 +54,21 @@ export interface ErrorBoundaryProps {
3054
interface State {
3155
error: Error | null;
3256
lastResetKey: unknown;
57+
/** How many times autoRecover has cleared an error since the last healthy
58+
* stretch / resetKey change. Caps auto-recovery so a persistent loop can't
59+
* thrash the boundary. */
60+
recoveryAttempts: number;
3361
}
3462

3563
export class ErrorBoundary extends Component<ErrorBoundaryProps, State> {
36-
state: State = { error: null, lastResetKey: this.props.resetKey };
64+
state: State = {
65+
error: null,
66+
lastResetKey: this.props.resetKey,
67+
recoveryAttempts: 0,
68+
};
69+
70+
private recoverTimer: ReturnType<typeof setTimeout> | null = null;
71+
private stableTimer: ReturnType<typeof setTimeout> | null = null;
3772

3873
static getDerivedStateFromError(error: Error): Partial<State> {
3974
return { error };
@@ -44,7 +79,9 @@ export class ErrorBoundary extends Component<ErrorBoundaryProps, State> {
4479
state: State,
4580
): Partial<State> | null {
4681
if (props.resetKey === state.lastResetKey) return null;
47-
return { error: null, lastResetKey: props.resetKey };
82+
// A genuine new context (e.g. a different task): clear the error and refill
83+
// the auto-recovery budget.
84+
return { error: null, lastResetKey: props.resetKey, recoveryAttempts: 0 };
4885
}
4986

5087
componentDidCatch(error: Error, errorInfo: ErrorInfo) {
@@ -53,16 +90,79 @@ export class ErrorBoundary extends Component<ErrorBoundaryProps, State> {
5390
componentStack: errorInfo.componentStack,
5491
suppressed,
5592
});
93+
if (suppressed) return;
94+
// A fresh error means the subtree isn't healthy yet — cancel any pending
95+
// budget refill and, if enabled, schedule the next bounded auto-recovery.
96+
this.clearStableTimer();
97+
if (this.props.autoRecover) this.scheduleAutoRecover();
98+
}
99+
100+
componentDidUpdate(_prevProps: ErrorBoundaryProps, prevState: State) {
101+
// The error just cleared (auto-recovery or manual retry). If it doesn't
102+
// recur within the stability window, refill the recovery budget so a later
103+
// unrelated transient error isn't starved of retries.
104+
if (this.props.autoRecover && prevState.error && !this.state.error) {
105+
this.clearStableTimer();
106+
this.stableTimer = setTimeout(() => {
107+
this.stableTimer = null;
108+
if (this.state.recoveryAttempts !== 0) {
109+
this.setState({ recoveryAttempts: 0 });
110+
}
111+
}, RECOVERY_STABLE_RESET_MS);
112+
}
113+
}
114+
115+
componentWillUnmount() {
116+
this.clearRecoverTimer();
117+
this.clearStableTimer();
118+
}
119+
120+
private scheduleAutoRecover(): void {
121+
if (this.recoverTimer !== null) return;
122+
if (this.state.recoveryAttempts >= MAX_AUTO_RECOVERIES) return;
123+
// Back off a little per attempt so a loop that needs a beat to settle gets
124+
// progressively more time before we give up to the manual fallback.
125+
const delay =
126+
AUTO_RECOVER_BASE_DELAY_MS * (this.state.recoveryAttempts + 1);
127+
this.recoverTimer = setTimeout(() => {
128+
this.recoverTimer = null;
129+
this.setState((s) =>
130+
s.error
131+
? { error: null, recoveryAttempts: s.recoveryAttempts + 1 }
132+
: null,
133+
);
134+
}, delay);
135+
}
136+
137+
private clearRecoverTimer(): void {
138+
if (this.recoverTimer !== null) {
139+
clearTimeout(this.recoverTimer);
140+
this.recoverTimer = null;
141+
}
142+
}
143+
144+
private clearStableTimer(): void {
145+
if (this.stableTimer !== null) {
146+
clearTimeout(this.stableTimer);
147+
this.stableTimer = null;
148+
}
56149
}
57150

58151
handleRetry = () => {
59-
this.setState({ error: null });
152+
this.clearRecoverTimer();
153+
this.setState({ error: null, recoveryAttempts: 0 });
60154
};
61155

62156
render() {
63-
const { error } = this.state;
157+
const { error, recoveryAttempts } = this.state;
64158
if (!error) return this.props.children;
65159
if (this.props.shouldSuppress?.(error)) return null;
160+
// While a bounded auto-recovery is still pending, render nothing rather than
161+
// flashing the error UI for what is (usually) a one-frame transient loop.
162+
// componentDidCatch has scheduled the reset that will re-render the children.
163+
if (this.props.autoRecover && recoveryAttempts < MAX_AUTO_RECOVERIES) {
164+
return null;
165+
}
66166
if (this.props.fallback) return this.props.fallback;
67167

68168
return (

0 commit comments

Comments
 (0)