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..8318ab2c8 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,63 @@ 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); + 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()); + 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 b987d0610..575881e15 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..a357f5679 --- /dev/null +++ b/src/integrations/datadog/reportReactError.test.ts @@ -0,0 +1,143 @@ +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; +let reactRootErrorHandlers: typeof import('./reportReactError').reactRootErrorHandlers; + +beforeEach(async () => { + vi.resetModules(); + ({ reportReactError, reactRootErrorHandlers } = 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('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() { + throw new Error('getter trap'); + }, + }); + reportReactError(hostile, info); + reportReactError(hostile, info); + + expect(addReactError).toHaveBeenCalledTimes(2); + }); + + 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)).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++) { + 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..c8f50d486 --- /dev/null +++ b/src/integrations/datadog/reportReactError.ts @@ -0,0 +1,111 @@ +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; +} + +/** 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. + */ +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), + contentsOf(error), + ]); +} + +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. 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; } + } + 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); } +} + +/** + * 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..22bc29ac7 --- /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 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); + }); + 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