From 9f8341a7098535b58964c7750290462d7bc4b608 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 22:02:32 -0400 Subject: [PATCH 1/4] fix(errors): report each route failure to RUM once, and offer Try Again for transient ones One failed route load reached Datadog RUM as up to 14 identical error-boundary reports (#1695). TanStack Router re-throws a route's stored error whenever its match object changes, and every reload of the route throws a fresh but identical one, so React's root onCaughtError fired once per catch, not once per failure. Driving the real router with a 403 on the org route, the old handler reported 10 times for one failure. The root handlers now go through reportReactError, which skips a failure identical in name, message, stack, component stack and request endpoint to one reported in the last 10 s (at most 100 signatures held, oldest evicted). A value whose fields throw is still reported, just not deduped. The handlers' devtools copy moves from console.error to console.warn: the RUM SDK reports every console.error as an error of its own, so each boundary catch was also landing as a second, console-sourced event. PanelErrorBoundary and MetricErrorBoundary had the same duplicate in componentDidCatch and move to console.warn too. AGENTS.md records the rule next to the existing console.error note. A route error page for a 5xx or a request that got no answer (ERR_NETWORK, ECONNABORTED, ETIMEDOUT) now says so in words and offers Try Again, which reloads the route via router.invalidate() and is disabled while that reload is pending. 4xx, cancellations and code errors get no retry. The generic title becomes "Something Went Wrong" instead of "Component Error". Closes #1695 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 8 ++ src/components/ErrorComponent.test.tsx | 80 +++++++++-- src/components/ErrorComponent.tsx | 128 ++++++++++++----- .../components/ClusterRouteError.test.tsx | 12 +- .../cluster/components/ClusterRouteError.tsx | 4 +- .../analytics/primitives/MetricRenderer.tsx | 3 +- .../tabs/PanelErrorBoundary.test.tsx | 30 ++++ .../analytics/tabs/PanelErrorBoundary.tsx | 3 +- .../components/OrgRouteError.test.tsx | 6 +- .../organization/components/OrgRouteError.tsx | 4 +- .../datadog/reportReactError.test.ts | 114 +++++++++++++++ src/integrations/datadog/reportReactError.ts | 94 +++++++++++++ src/main.tsx | 23 +--- .../__tests__/routeErrorReporting.test.tsx | 130 ++++++++++++++++++ src/testSetup/failOnRenderPhaseUpdate.ts | 5 +- 15 files changed, 566 insertions(+), 78 deletions(-) create mode 100644 src/features/instance/status/analytics/tabs/PanelErrorBoundary.test.tsx create mode 100644 src/integrations/datadog/reportReactError.test.ts create mode 100644 src/integrations/datadog/reportReactError.ts create mode 100644 src/router/__tests__/routeErrorReporting.test.tsx diff --git a/AGENTS.md b/AGENTS.md index f8633afed..b497a24ed 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -830,6 +830,14 @@ The inverse trap is real too — a `.catch()` written to _reduce_ RUM noise must which is what #1658 was. `console.debug` is not collected, and is the channel for a swallowed failure you still want in devtools. +Error boundaries are the same trap from the other side. React's root `onCaughtError` already reports +every error any boundary catches, through +[`reportReactError`](src/integrations/datadog/reportReactError.ts), so a `componentDidCatch` that +`console.error`s files a second, console-sourced copy of each one. Log there with `console.warn` +(not collected either, but visible in devtools by default). That reporter also collapses repeats: a +TanStack route re-throws its stored error on every match update and every reload throws a fresh +identical one, so one failed load used to reach Error Tracking up to 14 times (#1695). + ## A 5xx body never reaches an auth form `describeError` renders whatever the server sent, and before #1676 that reached the sign-in and diff --git a/src/components/ErrorComponent.test.tsx b/src/components/ErrorComponent.test.tsx index 734ec9238..de65c0900 100644 --- a/src/components/ErrorComponent.test.tsx +++ b/src/components/ErrorComponent.test.tsx @@ -2,16 +2,29 @@ * @vitest-environment jsdom */ import { TestProvider } from '@/lib/test/TestProvider'; -import { cleanup, render, screen } from '@testing-library/react'; +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; import { act } from 'react'; -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { ErrorComponent } from './ErrorComponent'; -afterEach(() => cleanup()); +const { invalidate } = vi.hoisted(() => ({ invalidate: vi.fn(() => Promise.resolve()) })); +// Only ErrorComponent's own import is replaced; the provider's router and its links stay real. +vi.mock('@tanstack/react-router', async (importOriginal) => { + const router = { invalidate }; + return { ...await importOriginal(), useRouter: () => router }; +}); + +afterEach(() => { + cleanup(); + invalidate.mockClear(); +}); const FALLBACK = 'An unexpected error occurred.'; -async function renderError(error: unknown, props: { title?: string; description?: string } = {}) { +async function renderError( + error: unknown, + props: { title?: string; description?: string; reset?: () => void } = {}, +) { render( @@ -46,7 +59,7 @@ describe(ErrorComponent, () => { }, }); await renderError(hostile); - expect(screen.getByRole('heading', { name: 'Component Error' })).toBeTruthy(); + expect(screen.getByRole('heading', { name: 'Something Went Wrong' })).toBeTruthy(); expect(screen.getByText(FALLBACK)).toBeTruthy(); }); @@ -62,10 +75,59 @@ describe(ErrorComponent, () => { expect(screen.queryByText(/doesn't have permission/)).toBeNull(); }); - it('should keep the raw message for a status it has no copy for', async () => { - await renderError(httpError(500)); - expect(screen.getByRole('heading', { name: 'Component Error' })).toBeTruthy(); - expect(screen.getByText('Request failed with status code 500')).toBeTruthy(); + it('should keep the raw message under a generic title for a status it has no copy for', async () => { + await renderError(httpError(409)); + expect(screen.getByRole('heading', { name: 'Something Went Wrong' })).toBeTruthy(); + expect(screen.getByText('Request failed with status code 409')).toBeTruthy(); + }); + + const networkError = (code: string) => Object.assign(new Error('Network Error'), { code }); + const reset = () => {}; + + it.each([ + ['a 500', httpError(500), 'Something Went Wrong', /couldn't load this \(error 500\)/], + ['a 503', httpError(503), 'Something Went Wrong', /couldn't load this \(error 503\)/], + ['ERR_NETWORK', networkError('ERR_NETWORK'), "Couldn't Reach the Server", /didn't get an answer/], + ['ECONNABORTED', networkError('ECONNABORTED'), "Couldn't Reach the Server", /didn't get an answer/], + ['ETIMEDOUT', networkError('ETIMEDOUT'), "Couldn't Reach the Server", /didn't get an answer/], + ])('should offer Try Again on a route for %s', async (_label, error, title, description) => { + await renderError(error, { reset }); + expect(screen.getByRole('heading', { name: title })).toBeTruthy(); + expect(screen.getByText(description)).toBeTruthy(); + expect(screen.getByRole('button', { name: /Try Again/ })).toBeTruthy(); + }); + + it.each([ + ['a 400', httpError(400)], + ['a 403', httpError(403)], + ['a 404', httpError(404)], + ['a cancellation', networkError('ERR_CANCELED')], + ['a thrown TypeError', new TypeError('x is undefined')], + ['a thrown string', 'boom'], + ])('should not offer Try Again for %s, which a reload cannot fix', async (_label, error) => { + await renderError(error, { reset }); + expect(screen.queryByRole('button', { name: /Try Again/ })).toBeNull(); + }); + + it('should not offer Try Again outside a route error boundary', async () => { + await renderError(httpError(503)); + expect(screen.getByText(/couldn't load this \(error 503\)/)).toBeTruthy(); + expect(screen.queryByRole('button', { name: /Try Again/ })).toBeNull(); + }); + + it('should reload the route once per Try Again, and not again while that reload is pending', async () => { + let finishReload!: () => void; + invalidate.mockImplementationOnce(() => new Promise((resolve) => (finishReload = resolve))); + await renderError(httpError(503), { reset }); + const button = screen.getByRole('button', { name: /Try Again/ }); + + fireEvent.click(button); + fireEvent.click(button); + expect(invalidate).toHaveBeenCalledTimes(1); + expect(screen.getByRole('button', { name: /Trying Again/ }).disabled).toBe(true); + + await act(async () => finishReload()); + expect(screen.getByRole('button', { name: /Try Again/ }).disabled).toBe(false); }); it('should render the message of a thrown Error', async () => { diff --git a/src/components/ErrorComponent.tsx b/src/components/ErrorComponent.tsx index b987d0610..74f03477f 100644 --- a/src/components/ErrorComponent.tsx +++ b/src/components/ErrorComponent.tsx @@ -5,9 +5,10 @@ import { useOverallAuth } from '@/hooks/useAuth'; import { cn } from '@/lib/cn'; import { errorStatus } from '@/lib/errorStatus'; import { getDefaultSignedInCloudRouteForUser } from '@/lib/urls/getDefaultSignedInCloudRouteForUser'; -import { Link } from '@tanstack/react-router'; -import { ArrowLeft } from 'lucide-react'; -import { isValidElement, ReactNode } from 'react'; +import { Link, useRouter } from '@tanstack/react-router'; +import { AxiosError } from 'axios'; +import { ArrowLeft, RotateCw } from 'lucide-react'; +import { isValidElement, ReactNode, useState } from 'react'; interface ErrorProps { className?: string | undefined; @@ -20,38 +21,87 @@ interface ErrorProps { title?: string; description?: ReactNode; showReturnToHome?: boolean; + // TanStack Router passes this only to a route error component, so its presence is what says + // reloading the route can retry the failure. Retrying calls `router.invalidate()`, not `reset`: + // a reset alone re-renders the match, which re-throws the error it still holds. + reset?: () => void; children?: ReactNode; } const FALLBACK_ERROR_MESSAGE = 'An unexpected error occurred.'; +const GENERIC_TITLE = 'Something Went Wrong'; -interface StatusCopy { +interface FailureCopy { title: string; description: string; + retryable?: boolean; } -const FORBIDDEN_COPY: StatusCopy = { +const FORBIDDEN_COPY: FailureCopy = { title: 'Access Denied', description: "Your account doesn't have permission to view this. If you think it should, ask an administrator for access.", }; -const NOT_FOUND_COPY: StatusCopy = { +const NOT_FOUND_COPY: FailureCopy = { title: 'Not Found', description: "We couldn't find what you were looking for. It may have been deleted, or the link may be wrong.", }; -function describeStatus(error: unknown): StatusCopy | undefined { - switch (errorStatus(error)) { - case 403: - return FORBIDDEN_COPY; - case 404: - return NOT_FOUND_COPY; - default: - return undefined; +const UNREACHABLE_COPY: FailureCopy = { + title: "Couldn't Reach the Server", + description: "The request didn't get an answer. Check your connection, then try again.", + retryable: true, +}; + +const NO_RESPONSE_CODES: ReadonlySet = new Set([ + AxiosError.ERR_NETWORK, + AxiosError.ECONNABORTED, + AxiosError.ETIMEDOUT, +]); + +function errorCode(error: unknown): unknown { + try { + return (error as { code?: unknown } | null | undefined)?.code; + } catch { + return undefined; } } +function describeFailure(error: unknown): FailureCopy | undefined { + const status = errorStatus(error); + if (status === 403) { return FORBIDDEN_COPY; } + if (status === 404) { return NOT_FOUND_COPY; } + if (typeof status === 'number') { + return status >= 500 && status < 600 + ? { + title: GENERIC_TITLE, + description: `The server couldn't load this (error ${status}). Try again in a moment.`, + retryable: true, + } + : undefined; + } + return NO_RESPONSE_CODES.has(errorCode(error)) ? UNREACHABLE_COPY : undefined; +} + +function RetryButton() { + const router = useRouter(); + const [retrying, setRetrying] = useState(false); + return ( + + ); +} + /** * Whether React can render this value as visible text. * @@ -93,10 +143,13 @@ function getErrorMessage(error: unknown): ReactNode { return FALLBACK_ERROR_MESSAGE; } -export function ErrorComponent({ className, error, title, description, showReturnToHome, children }: ErrorProps) { +export function ErrorComponent( + { className, error, title, description, showReturnToHome, reset, children }: ErrorProps, +) { const { user, isLoading: isUserLoading } = useOverallAuth(); const defaultCloudRoute = getDefaultSignedInCloudRouteForUser(user); - const statusCopy = describeStatus(error); + const failureCopy = describeFailure(error); + const canRetry = reset !== undefined && failureCopy?.retryable === true; return ( -

{title ?? statusCopy?.title ?? 'Component Error'}

+

{title ?? failureCopy?.title ?? GENERIC_TITLE}

 - {description ?? statusCopy?.description ?? getErrorMessage(error)} + {description ?? failureCopy?.description ?? getErrorMessage(error)} {children} - {showReturnToHome !== false && (user && !isUserLoading - ? ( - - - - ) - : ( - - - - ))} + {(canRetry || showReturnToHome !== false) && ( +
+ {canRetry && } + {showReturnToHome !== false && (user && !isUserLoading + ? ( + + + + ) + : ( + + + + ))} +
+ )} ); diff --git a/src/features/cluster/components/ClusterRouteError.test.tsx b/src/features/cluster/components/ClusterRouteError.test.tsx index 22328e47b..3fe01bbb2 100644 --- a/src/features/cluster/components/ClusterRouteError.test.tsx +++ b/src/features/cluster/components/ClusterRouteError.test.tsx @@ -8,7 +8,12 @@ vi.mock('@tanstack/react-router', () => ({ Link: ({ children, to }: { children: ReactNode; to: string }) => {children}, })); vi.mock('@/components/ErrorComponent', () => ({ - ErrorComponent: ({ error }: { error: Error }) =>

generic error: {error.message}

, + ErrorComponent: ({ error, reset }: { error: Error; reset?: () => void }) => ( +

+ generic error: {error.message} + {reset && ' (retryable)'} +

+ ), })); import { ClusterRouteError } from './ClusterRouteError'; @@ -33,4 +38,9 @@ describe('ClusterRouteError', () => { expect(screen.getByText('generic error: Request failed with status code 500')).toBeTruthy(); expect(screen.queryByText('Pending Owner Setup')).toBeNull(); }); + + it("hands the router's reset to the generic error page so it can offer a retry", () => { + render( {}} />); + expect(screen.getByText('generic error: Request failed with status code 503 (retryable)')).toBeTruthy(); + }); }); diff --git a/src/features/cluster/components/ClusterRouteError.tsx b/src/features/cluster/components/ClusterRouteError.tsx index d35beaeff..5e9330e1a 100644 --- a/src/features/cluster/components/ClusterRouteError.tsx +++ b/src/features/cluster/components/ClusterRouteError.tsx @@ -9,10 +9,10 @@ import { ArrowLeft } from 'lucide-react'; * The cluster route loads the cluster before any page under it renders. Central manager answers 403 to members who * aren't org admins until a new cluster's setup is finished; its other per-cluster refusals read as 404. */ -export function ClusterRouteError({ error }: { error: unknown }) { +export function ClusterRouteError({ error, reset }: { error: unknown; reset?: () => void }) { const { organizationId }: { organizationId?: string } = useParams({ strict: false }); if (!isForbiddenError(error)) { - return ; + return ; } return (
diff --git a/src/features/instance/status/analytics/primitives/MetricRenderer.tsx b/src/features/instance/status/analytics/primitives/MetricRenderer.tsx index f7176fcd7..d59df90b2 100644 --- a/src/features/instance/status/analytics/primitives/MetricRenderer.tsx +++ b/src/features/instance/status/analytics/primitives/MetricRenderer.tsx @@ -87,7 +87,8 @@ class MetricErrorBoundary extends Component { + cleanup(); + vi.restoreAllMocks(); +}); + +function Broken(): never { + throw new Error('bad records'); +} + +describe('PanelErrorBoundary', () => { + it('logs a panel crash as a warning, leaving the RUM report to the root error handler', () => { + const consoleError = vi.spyOn(console, 'error'); + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + render( + + + , + { onCaughtError: () => {} }, + ); + + expect(screen.getByText('Panel "cpu" is unavailable')).toBeTruthy(); + expect(consoleWarn).toHaveBeenCalledWith('[panel:cpu] render failed', expect.any(Error)); + expect(consoleError.mock.calls.filter(([first]) => String(first).includes('render failed'))).toEqual([]); + }); +}); diff --git a/src/features/instance/status/analytics/tabs/PanelErrorBoundary.tsx b/src/features/instance/status/analytics/tabs/PanelErrorBoundary.tsx index 1020ad7a6..dfa4cf4fe 100644 --- a/src/features/instance/status/analytics/tabs/PanelErrorBoundary.tsx +++ b/src/features/instance/status/analytics/tabs/PanelErrorBoundary.tsx @@ -30,7 +30,8 @@ export class PanelErrorBoundary extends Component { } componentDidCatch(error: Error) { - console.error(`[panel:${this.props.metric}] render failed`, error); + // Not `console.error`: the root `onCaughtError` already reports this to RUM, which would count that as a second error. + console.warn(`[panel:${this.props.metric}] render failed`, error); } render() { diff --git a/src/features/organization/components/OrgRouteError.test.tsx b/src/features/organization/components/OrgRouteError.test.tsx index bb5102c52..88b828717 100644 --- a/src/features/organization/components/OrgRouteError.test.tsx +++ b/src/features/organization/components/OrgRouteError.test.tsx @@ -122,11 +122,11 @@ describe('OrgRouteError on the organization route', () => { expect(screen.queryByText(/isn't a member of it/)).toBeNull(); }); - it('leaves any other failure to the generic error page', async () => { + it('leaves any other failure to the generic error page, with the retry a route error gets', async () => { rejectWith(500); const heading = await openOrganization('org-broken'); - expect(heading.textContent).not.toMatch(/Organization/); - expect(screen.getByText('Request failed with status code 500')).toBeTruthy(); + expect(heading.textContent).toBe('Something Went Wrong'); + expect(screen.getByRole('button', { name: /Try Again/ })).toBeTruthy(); }); }); diff --git a/src/features/organization/components/OrgRouteError.tsx b/src/features/organization/components/OrgRouteError.tsx index 975f98868..a9f1b28d3 100644 --- a/src/features/organization/components/OrgRouteError.tsx +++ b/src/features/organization/components/OrgRouteError.tsx @@ -6,7 +6,7 @@ import { errorStatus } from '@/lib/errorStatus'; * missing organization and someone else's read the same — the copy must not confirm which. Only staff, * who skip that check, can get a 404. */ -export function OrgRouteError({ error }: { error: unknown }) { +export function OrgRouteError({ error, reset }: { error: unknown; reset?: () => void }) { switch (errorStatus(error)) { case 403: return ( @@ -25,6 +25,6 @@ export function OrgRouteError({ error }: { error: unknown }) { /> ); default: - return ; + return ; } } diff --git a/src/integrations/datadog/reportReactError.test.ts b/src/integrations/datadog/reportReactError.test.ts new file mode 100644 index 000000000..0e036a791 --- /dev/null +++ b/src/integrations/datadog/reportReactError.test.ts @@ -0,0 +1,114 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { addReactError } = vi.hoisted(() => ({ addReactError: vi.fn() })); +vi.mock('@datadog/browser-rum-react', () => ({ addReactError })); + +let now = 0; +let reportReactError: typeof import('./reportReactError').reportReactError; + +beforeEach(async () => { + vi.resetModules(); + ({ reportReactError } = await import('./reportReactError')); + now = 1_000; + vi.spyOn(performance, 'now').mockImplementation(() => now); +}); + +afterEach(() => { + vi.restoreAllMocks(); + addReactError.mockReset(); +}); + +const info = { componentStack: '\n at OrgLayout\n at Dashboard' }; + +function axiosError(url: string, message = 'Request failed with status code 403') { + return Object.assign(new Error(message), { + name: 'AxiosError', + stack: `AxiosError: ${message}\n at settle @ https://fabric.harper.fast/assets/index.js:1:1`, + config: { method: 'get', baseURL: 'https://cm.example', url }, + }); +} + +describe('reportReactError', () => { + it('reports one failure once however many times it is re-caught or re-thrown fresh', () => { + const first = axiosError('/Organization/org-x'); + reportReactError(first, info); + reportReactError(first, info); + now += 378; + reportReactError(axiosError('/Organization/org-x'), info); + reportReactError(axiosError('/Organization/org-x'), info); + + expect(addReactError).toHaveBeenCalledTimes(1); + expect(addReactError).toHaveBeenCalledWith(first, info); + }); + + it('reports the same failure again once the window since its last report has passed', () => { + reportReactError(axiosError('/Organization/org-x'), info); + now += 9_999; + reportReactError(axiosError('/Organization/org-x'), info); + now += 1; + reportReactError(axiosError('/Organization/org-x'), info); + + expect(addReactError).toHaveBeenCalledTimes(2); + }); + + it.each([ + ['message', axiosError('/Organization/org-x', 'Request failed with status code 500')], + ['endpoint', axiosError('/Organization/org-y')], + ['name', Object.assign(axiosError('/Organization/org-x'), { name: 'TypeError' })], + ])('reports a failure that differs only in its %s', (_label, other) => { + reportReactError(axiosError('/Organization/org-x'), info); + reportReactError(other, info); + + expect(addReactError).toHaveBeenCalledTimes(2); + }); + + it('reports the same error under a different component stack', () => { + const error = axiosError('/Organization/org-x'); + reportReactError(error, info); + reportReactError(error, { componentStack: '\n at ClusterLayout' }); + + expect(addReactError).toHaveBeenCalledTimes(2); + }); + + it('keeps thrown primitives of different types apart', () => { + reportReactError('1', info); + reportReactError(1, info); + reportReactError(1, info); + reportReactError(null, info); + + expect(addReactError).toHaveBeenCalledTimes(3); + }); + + it('still reports, without deduplicating, a value whose fields throw when read', () => { + const hostile = new Proxy({}, { + get() { + throw new Error('getter trap'); + }, + }); + reportReactError(hostile, info); + reportReactError(hostile, info); + + expect(addReactError).toHaveBeenCalledTimes(2); + }); + + it('does not remember a failure whose report threw', () => { + addReactError.mockImplementationOnce(() => { + throw new Error('sdk failure'); + }); + expect(() => reportReactError(axiosError('/Organization/org-x'), info)).toThrow('sdk failure'); + reportReactError(axiosError('/Organization/org-x'), info); + + expect(addReactError).toHaveBeenCalledTimes(2); + }); + + it('forgets the oldest signature past its cap, so it is reported again', () => { + reportReactError(axiosError('/Organization/org-0'), info); + for (let index = 1; index <= 100; index++) { + reportReactError(axiosError(`/Organization/org-${index}`), info); + } + reportReactError(axiosError('/Organization/org-100'), info); + reportReactError(axiosError('/Organization/org-0'), info); + + expect(addReactError).toHaveBeenCalledTimes(102); + }); +}); diff --git a/src/integrations/datadog/reportReactError.ts b/src/integrations/datadog/reportReactError.ts new file mode 100644 index 000000000..10a040f38 --- /dev/null +++ b/src/integrations/datadog/reportReactError.ts @@ -0,0 +1,94 @@ +import { addReactError } from '@datadog/browser-rum-react'; +import type { ErrorInfo } from 'react'; +import type { RootOptions } from 'react-dom/client'; + +const REPEAT_WINDOW_MS = 10_000; +const MAX_TRACKED_SIGNATURES = 100; + +/** Signature → when it was last reported. Insertion order is report order, so expiry stops at the first live entry. */ +const lastReportedAt = new Map(); + +function text(value: unknown): string | null { + if (typeof value === 'string' || typeof value === 'number') { return String(value); } + return value == null ? null : typeof value; +} + +/** + * The endpoint fields keep two different requests that failed the same way from collapsing into one; + * the signature never leaves memory. + */ +function signatureOf(error: unknown, componentStack: string | null | undefined): string { + if (typeof error !== 'object' || error === null) { + return JSON.stringify([typeof error, String(error), componentStack ?? null]); + } + const { name, message, stack, config } = error as { + name?: unknown; + message?: unknown; + stack?: unknown; + config?: { method?: unknown; baseURL?: unknown; url?: unknown } | null; + }; + return JSON.stringify([ + typeof error, + text(name), + text(message), + text(stack), + componentStack ?? null, + text(config?.method), + text(config?.baseURL), + text(config?.url), + ]); +} + +function remember(signature: string, now: number) { + for (const [key, at] of lastReportedAt) { + if (now - at < REPEAT_WINDOW_MS) { break; } + lastReportedAt.delete(key); + } + lastReportedAt.delete(signature); + lastReportedAt.set(signature, now); + if (lastReportedAt.size > MAX_TRACKED_SIGNATURES) { + lastReportedAt.delete(lastReportedAt.keys().next().value!); + } +} + +/** + * Report a failure React handed the root to RUM once, not once per time it is caught. + * + * TanStack Router re-throws a route's stored error whenever its match object changes, and every reload + * of the route throws a fresh but identical one, so a single failed load reaches `onCaughtError` many + * times (#1695). An identical failure within `REPEAT_WINDOW_MS` of its last report adds nothing. + */ +export function reportReactError(error: unknown, errorInfo: ErrorInfo): void { + let signature: string | undefined; + try { + signature = signatureOf(error, errorInfo.componentStack); + } catch { + signature = undefined; + } + const now = performance.now(); + if (signature !== undefined) { + const last = lastReportedAt.get(signature); + if (last !== undefined && now - last < REPEAT_WINDOW_MS) { return; } + } + addReactError(error as Error, errorInfo); + if (signature !== undefined) { remember(signature, now); } +} + +/** + * The devtools copy goes to `console.warn`: the RUM SDK reports every `console.error` as an error of + * its own (AGENTS.md, "Dropping `errorHandler` from an `onError` also drops the RUM report"). + */ +export const reactRootErrorHandlers = { + onUncaughtError: (error, errorInfo) => { + reportReactError(error, errorInfo); + console.warn('Uncaught error:', error, errorInfo); + }, + onCaughtError: (error, errorInfo) => { + reportReactError(error, errorInfo); + console.warn('Caught error:', error, errorInfo); + }, + onRecoverableError: (error, errorInfo) => { + reportReactError(error, errorInfo); + console.warn('Recoverable error:', error, errorInfo); + }, +} satisfies RootOptions; diff --git a/src/main.tsx b/src/main.tsx index 6d35c236e..f29b2a000 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -1,10 +1,10 @@ import { App } from '@/App'; import { authStore } from '@/features/auth/store/authStore'; import { scrubLegacySettings } from '@/features/instance/apis/explorer/settings'; +import { reactRootErrorHandlers } from '@/integrations/datadog/reportReactError'; import { installApiUnauthorizedRedirect } from '@/lib/installApiUnauthorizedRedirect'; import { installBrowserTranslationDomGuard } from '@/lib/installBrowserTranslationDomGuard'; import { installStaleDeployReload } from '@/lib/installStaleDeployReload'; -import { addReactError } from '@datadog/browser-rum-react'; import { StrictMode } from 'react'; import { createRoot } from 'react-dom/client'; import './index.css'; @@ -25,26 +25,7 @@ scrubLegacySettings(); // explorer is unmounted — so a tab that navigated away can't later restore a revoked token. authStore.installExplorerCrossTabCleanup(); -createRoot( - document.getElementById('root')!, - { - onUncaughtError: (error, errorInfo) => { - // Report uncaught errors to Datadog - addReactError(error as Error, errorInfo); - console.error('Uncaught error:', error, errorInfo); - }, - onCaughtError: (error, errorInfo) => { - // Report caught errors to Datadog - addReactError(error as Error, errorInfo); - console.error('Caught error:', error, errorInfo); - }, - onRecoverableError: (error, errorInfo) => { - // Report recoverable errors to Datadog - addReactError(error as Error, errorInfo); - console.warn('Recoverable error:', error, errorInfo); - }, - }, -).render( +createRoot(document.getElementById('root')!, reactRootErrorHandlers).render( , diff --git a/src/router/__tests__/routeErrorReporting.test.tsx b/src/router/__tests__/routeErrorReporting.test.tsx new file mode 100644 index 000000000..fa5bad530 --- /dev/null +++ b/src/router/__tests__/routeErrorReporting.test.tsx @@ -0,0 +1,130 @@ +/** + * @vitest-environment jsdom + */ +import { OverallAppSignIn } from '@/features/auth/store/authStore'; +import { orgLayoutRoute } from '@/features/organization/routes'; +import { orgsLayoutRoute } from '@/features/organizations/routes'; +import { reactRootErrorHandlers } from '@/integrations/datadog/reportReactError'; +import { queryClient } from '@/react-query/queryClient'; +import { dashboardLayout } from '@/router/dashboardRoute'; +import { rootRoute } from '@/router/rootRoute'; +import { useNewRouter } from '@/router/useNewRouter'; +import { QueryClientProvider } from '@tanstack/react-query'; +import { createRoute, RouterProvider } from '@tanstack/react-router'; +import { act, cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { AxiosError, AxiosHeaders, type InternalAxiosRequestConfig } from 'axios'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const { get, addReactError, signedIn } = vi.hoisted(() => ({ + get: vi.fn(), + addReactError: vi.fn(), + signedIn: { isLoading: false, user: { id: 'usr-1', roles: {} } }, +})); + +vi.mock('@/config/apiClient', () => ({ apiClient: { get } })); +vi.mock('@datadog/browser-rum-react', () => ({ addReactError })); +vi.mock('@/hooks/useAuth', async (importOriginal) => ({ + ...await importOriginal(), + useOverallAuth: () => signedIn, +})); +vi.mock('@/components/StudioCloud', async () => { + const { Outlet } = await import('@tanstack/react-router'); + return { StudioCloud: Outlet }; +}); +vi.mock('@/features/layouts/Dashboard', async () => { + const { Outlet } = await import('@tanstack/react-router'); + return { Dashboard: Outlet }; +}); + +// The production chain down to the org layout, with a light page in place of its real children. +const orgHomeRoute = createRoute({ + getParentRoute: () => orgLayoutRoute, + path: '/', + component: () =>

organization loaded

, +}); +const routeTree = rootRoute.addChildren([ + dashboardLayout.addChildren([orgsLayoutRoute.addChildren([orgLayoutRoute.addChildren([orgHomeRoute])])]), +]); + +function httpError(url: string, status: number) { + const config = { headers: new AxiosHeaders(), url, method: 'get' } as InternalAxiosRequestConfig; + return new AxiosError( + `Request failed with status code ${status}`, + status >= 500 ? AxiosError.ERR_BAD_RESPONSE : AxiosError.ERR_BAD_REQUEST, + config, + {}, + { status, statusText: '', data: {}, headers: {}, config }, + ); +} + +let router: ReturnType; +function App() { + router = useNewRouter({ routeTree: routeTree as never, authentication: { [OverallAppSignIn]: signedIn } as never }); + return ( + + + + ); +} + +function openOrganization(organizationId: string) { + window.location.hash = `#/${organizationId}`; + render(, { onCaughtError: reactRootErrorHandlers.onCaughtError }); +} + +afterEach(() => { + cleanup(); + queryClient.clear(); + get.mockReset(); + addReactError.mockReset(); + vi.restoreAllMocks(); +}); + +describe('route error reporting', () => { + it('reports a failed route load to RUM once, through re-catches and reloads, and never as a console error', async () => { + const consoleError = vi.spyOn(console, 'error'); + get.mockImplementation(async (url: string) => { + throw httpError(url, 403); + }); + openOrganization('org-forbidden'); + await screen.findByRole('heading', { name: 'No Access to This Organization' }, { timeout: 5000 }); + await act(() => router.invalidate()); + await act(() => router.invalidate()); + + expect(get.mock.calls.length).toBeGreaterThan(1); + expect(addReactError).toHaveBeenCalledTimes(1); + const rootHandlerLogs = consoleError.mock.calls.filter(([first]) => + typeof first === 'string' && first.endsWith('error:') + ); + expect(rootHandlerLogs).toEqual([]); + }); + + it('offers Try Again for a server error and shows the page once a reload succeeds', async () => { + let serverUp = false; + get.mockImplementation(async (url: string) => { + if (!serverUp) { throw httpError(url, 503); } + return { data: { id: 'org-flaky', name: 'Flaky', clusters: [] } }; + }); + openOrganization('org-flaky'); + await screen.findByRole('heading', { name: 'Something Went Wrong' }, { timeout: 5000 }); + expect(screen.getByText(/couldn't load this \(error 503\)/)).toBeTruthy(); + const failedReads = get.mock.calls.length; + + serverUp = true; + fireEvent.click(screen.getByRole('button', { name: /Try Again/ })); + + expect(await screen.findByText('organization loaded')).toBeTruthy(); + expect(get.mock.calls.length).toBeGreaterThan(failedReads); + expect(screen.queryByTestId('error-component')).toBeNull(); + }); + + it('offers no Try Again for a 403, which a reload cannot fix', async () => { + get.mockImplementation(async (url: string) => { + throw httpError(url, 403); + }); + openOrganization('org-denied'); + await screen.findByRole('heading', { name: 'No Access to This Organization' }, { timeout: 5000 }); + + expect(screen.queryByRole('button', { name: /Try Again/ })).toBeNull(); + }); +}); diff --git a/src/testSetup/failOnRenderPhaseUpdate.ts b/src/testSetup/failOnRenderPhaseUpdate.ts index 5c4bdd78c..d67184737 100644 --- a/src/testSetup/failOnRenderPhaseUpdate.ts +++ b/src/testSetup/failOnRenderPhaseUpdate.ts @@ -8,9 +8,8 @@ // only caught by manually watching a real browser console. // // The match is deliberately narrow (this one React message), NOT a blanket -// console.error ban: intentional error paths (PanelErrorBoundary's -// `[panel:x] render failed`, the query-cache toast handler) log through -// console.error legitimately. +// console.error ban: intentional error paths (the query-cache toast handler) +// log through console.error legitimately. // // A test that MUST assert this warning fires (e.g. proving a repro against a // known-bad implementation) can capture console.error itself; replacing the From 9581286b708c6f37f20d6aead536d0eebf2a7b18 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 22:42:14 -0400 Subject: [PATCH 2/4] fix(errors): keep the root error handlers from throwing when the RUM SDK does A throw from addReactError escaped the root onCaughtError callback and skipped the devtools line for the failure it was reporting. reportReactError now swallows it and leaves the signature unrecorded so the next catch tries again. The real-router test name now says what it proves: one report from the root handlers. React Query's per-attempt console.error is still a second channel, filed as #1808. Co-Authored-By: Claude Opus 5.5 --- .../datadog/reportReactError.test.ts | 18 +++++++++++++++--- src/integrations/datadog/reportReactError.ts | 10 ++++++++-- .../__tests__/routeErrorReporting.test.tsx | 2 +- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/integrations/datadog/reportReactError.test.ts b/src/integrations/datadog/reportReactError.test.ts index 0e036a791..471f23407 100644 --- a/src/integrations/datadog/reportReactError.test.ts +++ b/src/integrations/datadog/reportReactError.test.ts @@ -5,10 +5,11 @@ vi.mock('@datadog/browser-rum-react', () => ({ addReactError })); let now = 0; let reportReactError: typeof import('./reportReactError').reportReactError; +let reactRootErrorHandlers: typeof import('./reportReactError').reactRootErrorHandlers; beforeEach(async () => { vi.resetModules(); - ({ reportReactError } = await import('./reportReactError')); + ({ reportReactError, reactRootErrorHandlers } = await import('./reportReactError')); now = 1_000; vi.spyOn(performance, 'now').mockImplementation(() => now); }); @@ -91,16 +92,27 @@ describe('reportReactError', () => { expect(addReactError).toHaveBeenCalledTimes(2); }); - it('does not remember a failure whose report threw', () => { + it('swallows a report that threw, and does not remember it', () => { addReactError.mockImplementationOnce(() => { throw new Error('sdk failure'); }); - expect(() => reportReactError(axiosError('/Organization/org-x'), info)).toThrow('sdk failure'); + expect(() => reportReactError(axiosError('/Organization/org-x'), info)).not.toThrow(); reportReactError(axiosError('/Organization/org-x'), info); expect(addReactError).toHaveBeenCalledTimes(2); }); + it('still logs the original failure when the SDK throws while reporting it', () => { + addReactError.mockImplementation(() => { + throw new Error('sdk failure'); + }); + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const error = axiosError('/Organization/org-x'); + + expect(() => reactRootErrorHandlers.onCaughtError(error, info)).not.toThrow(); + expect(consoleWarn).toHaveBeenCalledWith('Caught error:', error, info); + }); + it('forgets the oldest signature past its cap, so it is reported again', () => { reportReactError(axiosError('/Organization/org-0'), info); for (let index = 1; index <= 100; index++) { diff --git a/src/integrations/datadog/reportReactError.ts b/src/integrations/datadog/reportReactError.ts index 10a040f38..bffab8677 100644 --- a/src/integrations/datadog/reportReactError.ts +++ b/src/integrations/datadog/reportReactError.ts @@ -56,7 +56,7 @@ function remember(signature: string, now: number) { * * TanStack Router re-throws a route's stored error whenever its match object changes, and every reload * of the route throws a fresh but identical one, so a single failed load reaches `onCaughtError` many - * times (#1695). An identical failure within `REPEAT_WINDOW_MS` of its last report adds nothing. + * times. An identical failure within `REPEAT_WINDOW_MS` of its last report adds nothing. */ export function reportReactError(error: unknown, errorInfo: ErrorInfo): void { let signature: string | undefined; @@ -70,7 +70,13 @@ export function reportReactError(error: unknown, errorInfo: ErrorInfo): void { const last = lastReportedAt.get(signature); if (last !== undefined && now - last < REPEAT_WINDOW_MS) { return; } } - addReactError(error as Error, errorInfo); + try { + addReactError(error as Error, errorInfo); + } catch { + // The root callback must not throw on top of the failure it was handed; an unrecorded signature + // lets the next catch try again. + return; + } if (signature !== undefined) { remember(signature, now); } } diff --git a/src/router/__tests__/routeErrorReporting.test.tsx b/src/router/__tests__/routeErrorReporting.test.tsx index fa5bad530..22bc29ac7 100644 --- a/src/router/__tests__/routeErrorReporting.test.tsx +++ b/src/router/__tests__/routeErrorReporting.test.tsx @@ -81,7 +81,7 @@ afterEach(() => { }); describe('route error reporting', () => { - it('reports a failed route load to RUM once, through re-catches and reloads, and never as a console error', async () => { + it('reports a failed route load from the root handlers once, through re-catches and reloads, with no console error of their own', async () => { const consoleError = vi.spyOn(console, 'error'); get.mockImplementation(async (url: string) => { throw httpError(url, 403); From dbc2bc20f5988c1d830de46c692da8dde1b74326 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 10:54:34 -0400 Subject: [PATCH 3/4] fix(errors): tell thrown plain objects apart by their contents before deduplicating them Two non-Error values thrown within the repeat window, such as { error: 'Unauthorized' } and { error: 'Forbidden' }, had no name, message, stack or config to differ on, so the second was dropped as a repeat of the first. A non-Error's serialized contents now join its signature, with its keys standing in when it cannot be serialized, so a cyclic object re-thrown by the router is still reported once. Co-Authored-By: Claude Opus 5.5 --- .../datadog/reportReactError.test.ts | 17 +++++++++++++++++ src/integrations/datadog/reportReactError.ts | 11 +++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/integrations/datadog/reportReactError.test.ts b/src/integrations/datadog/reportReactError.test.ts index 471f23407..a357f5679 100644 --- a/src/integrations/datadog/reportReactError.test.ts +++ b/src/integrations/datadog/reportReactError.test.ts @@ -80,6 +80,23 @@ describe('reportReactError', () => { expect(addReactError).toHaveBeenCalledTimes(3); }); + it('keeps thrown plain objects with different contents apart', () => { + reportReactError({ error: 'Unauthorized' }, info); + reportReactError({ error: 'Forbidden' }, info); + reportReactError({ error: 'Forbidden' }, info); + + expect(addReactError).toHaveBeenCalledTimes(2); + }); + + it('still deduplicates a thrown object that cannot be serialized', () => { + const cyclic: Record = { error: 'Unauthorized' }; + cyclic.self = cyclic; + reportReactError(cyclic, info); + reportReactError(cyclic, info); + + expect(addReactError).toHaveBeenCalledTimes(1); + }); + it('still reports, without deduplicating, a value whose fields throw when read', () => { const hostile = new Proxy({}, { get() { diff --git a/src/integrations/datadog/reportReactError.ts b/src/integrations/datadog/reportReactError.ts index bffab8677..c8f50d486 100644 --- a/src/integrations/datadog/reportReactError.ts +++ b/src/integrations/datadog/reportReactError.ts @@ -13,6 +13,16 @@ function text(value: unknown): string | null { return value == null ? null : typeof value; } +/** A thrown non-Error carries none of the fields an Error is told apart by, so its contents stand in. */ +function contentsOf(error: object): string | null { + if (error instanceof Error) { return null; } + try { + return JSON.stringify(error) ?? null; + } catch { + return Object.keys(error).join(','); + } +} + /** * The endpoint fields keep two different requests that failed the same way from collapsing into one; * the signature never leaves memory. @@ -36,6 +46,7 @@ function signatureOf(error: unknown, componentStack: string | null | undefined): text(config?.method), text(config?.baseURL), text(config?.url), + contentsOf(error), ]); } From 051811606626af266294cf2ff1627653ac5935f5 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 10:59:27 -0400 Subject: [PATCH 4/4] fix(errors): spin the Try Again icon while its reload is pending Co-Authored-By: Claude Opus 5.5 --- src/components/ErrorComponent.test.tsx | 8 ++++++-- src/components/ErrorComponent.tsx | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/components/ErrorComponent.test.tsx b/src/components/ErrorComponent.test.tsx index de65c0900..8318ab2c8 100644 --- a/src/components/ErrorComponent.test.tsx +++ b/src/components/ErrorComponent.test.tsx @@ -124,10 +124,14 @@ describe(ErrorComponent, () => { fireEvent.click(button); fireEvent.click(button); expect(invalidate).toHaveBeenCalledTimes(1); - expect(screen.getByRole('button', { name: /Trying Again/ }).disabled).toBe(true); + const pending = screen.getByRole('button', { name: /Trying Again/ }); + expect(pending.disabled).toBe(true); + expect(pending.querySelector('svg')?.classList.contains('animate-spin')).toBe(true); await act(async () => finishReload()); - expect(screen.getByRole('button', { name: /Try Again/ }).disabled).toBe(false); + const settled = screen.getByRole('button', { name: /Try Again/ }); + expect(settled.disabled).toBe(false); + expect(settled.querySelector('svg')?.classList.contains('animate-spin')).toBe(false); }); it('should render the message of a thrown Error', async () => { diff --git a/src/components/ErrorComponent.tsx b/src/components/ErrorComponent.tsx index 74f03477f..575881e15 100644 --- a/src/components/ErrorComponent.tsx +++ b/src/components/ErrorComponent.tsx @@ -97,7 +97,7 @@ function RetryButton() { void router.invalidate().finally(() => setRetrying(false)); }} > - {retrying ? 'Trying Again…' : 'Try Again'} + {retrying ? 'Trying Again…' : 'Try Again'} ); }