diff --git a/src/lib/network-error.ts b/src/lib/network-error.ts index 986de9e3bd..5a6f866484 100644 --- a/src/lib/network-error.ts +++ b/src/lib/network-error.ts @@ -1,10 +1,8 @@ const NETWORK_ERROR_PATTERNS = [ /* - * Case-sensitive on purpose. `AbortError` (web `DOMException`) and the - * `Aborted` message thrown by `#/lib/async/cancelable` are transport - * cancellations, but the lowercase word shows up in unrelated failures - a - * multipart upload the service marked `aborted`, or a `TypeError` naming - * `abortController.abort` - which must still be reported. + * Case-sensitive: `AbortError` and `Aborted` are cancellations, but the + * lowercase word appears in unrelated failures ("Multipart upload aborted") + * that must still be reported. */ /\bAbort(?:ed|Error)?\b/, /network request failed/i, @@ -24,12 +22,9 @@ const NETWORK_ERROR_PATTERNS = [ ] /** - * `String(value)` runs code we do not control - a custom `toString`, a - * `Symbol.toPrimitive`, a `Proxy` trap - and throws outright for a - * null-prototype object, so stringifying an arbitrary thrown value can itself - * throw. Returning `undefined` lets callers treat an unreadable value as "not a - * network error" rather than letting the throw escape `logger.error()` or - * Sentry's `beforeSend`, where it would lose the original report. + * `String()` can throw - null-prototype objects, custom `toString`, proxies - + * so an unreadable value counts as "not a network error" rather than crashing + * the error path that asked. */ export function safeStringify(value: unknown): string | undefined { try { @@ -45,9 +40,7 @@ export function safeStringify(value: unknown): string | undefined { * fetch, and the XRPC clients. Error causes are checked because the XRPC * clients wrap the platform-specific fetch error. * - * Never throws: reading `cause` off a hostile value can throw just like - * stringifying it can, and a throw here would escape whatever error path is - * asking the question. + * Never throws; see {@link safeStringify}. */ export function isNetworkError(value: unknown): boolean { try { @@ -57,10 +50,6 @@ export function isNetworkError(value: unknown): boolean { } } -/** - * `seen` stays undefined until the first recursion into an object cause, so the - * common case (a string or an error with no cause) allocates nothing. - */ function isNetworkErrorInner( value: unknown, seen: Set | undefined, diff --git a/src/lib/strings/errors.ts b/src/lib/strings/errors.ts index 0d345b433e..1408709bb0 100644 --- a/src/lib/strings/errors.ts +++ b/src/lib/strings/errors.ts @@ -39,11 +39,7 @@ export function cleanError(e: unknown): string { */ // oxlint-disable-next-line typescript/no-base-to-string const str = typeof e === 'string' ? e : e.toString() - /* - * Passed the original value, not `str`, so the wrapped-cause walk is - * reachable: the XRPC clients report the platform fetch failure as the - * `cause` of a generic message. - */ + // the original value, not `str`, so wrapped causes are checked if (isNetworkError(e)) { return t`Unable to connect. Please check your internet connection and try again.` } diff --git a/src/logger/sentry/network-errors.test.ts b/src/logger/sentry/network-errors.test.ts index 0ed754910d..9873783e3f 100644 --- a/src/logger/sentry/network-errors.test.ts +++ b/src/logger/sentry/network-errors.test.ts @@ -15,10 +15,7 @@ function statusOnlyError(status: number) { return new XrpcResponseError(method, new Response(null, {status}), undefined) } -/** - * An `XrpcResponseError` as the PDS pipethrough produces it: the status is - * rewritten while the upstream lexicon code is forwarded verbatim. - */ +/** A pipethrough `XrpcResponseError`: status rewritten, code forwarded. */ function payloadError(status: number, error: string, message: string) { const body = {error, message} return new XrpcResponseError( @@ -92,10 +89,7 @@ describe('isExpectedSentryNetworkError', () => { describe('dropExpectedNetworkErrors', () => { test('drops automatic captures using the original exception', () => { - /* - * The serialized values do not carry the cause chain, so the hint is the - * only place the transport failure is visible. - */ + // the serialized values carry no cause chain, only the hint has the failure const event = exceptionEvent( 'XrpcInternalError', 'Unable to fulfill XRPC request', diff --git a/src/logger/sentry/network-errors.ts b/src/logger/sentry/network-errors.ts index aefc1ae07f..ae83e7df3a 100644 --- a/src/logger/sentry/network-errors.ts +++ b/src/logger/sentry/network-errors.ts @@ -4,29 +4,19 @@ import {type ErrorEvent} from '@sentry/react-native' import {isNetworkError, safeStringify} from '#/lib/network-error' /** - * HTTP statuses that mean an upstream service was unavailable. - * - * Matched on the status rather than the lexicon code because the code is not - * reliable here: the PDS pipethrough rewrites the status to 502 while - * forwarding the upstream error code verbatim, so an upstream 500 arrives as a - * 502 still named `InternalServerError`. The status is authoritative. - * - * `#/lib/xrpc-error` is deliberately not used for the `instanceof` check - it - * imports `#/lexicons`, which would pull the whole lexicon graph into the - * module evaluated before `Sentry.init()`. + * Upstream-unavailable statuses. Matched on the status, not the lexicon + * code: the PDS pipethrough rewrites the status to 502 but forwards the + * upstream code verbatim, so a 502 can arrive named `InternalServerError`. + * `#/lib/xrpc-error` is deliberately not imported - it pulls `#/lexicons` + * into the module graph evaluated before `Sentry.init()`. */ const TRANSIENT_UPSTREAM_STATUSES = new Set([502, 503, 504]) /** - * Sentry serializes an exception as `{type: error.name, value: error.message}`, - * so these match the message text rather than the `toString()`. Lex builds that - * message from the server's error payload ("Upstream Timeout") or, when the - * response carries no JSON payload, from the status alone ("Upstream server - * responded with a 504 error"). - * - * The optional space matches the spaced prose and the space-free lexicon code - * ("UpstreamFailure") alike, and the word boundaries keep camelCase - * identifiers such as `upstreamTimeoutV2` from matching. + * Sentry serializes `error.message`, not `toString()`, so these match the + * message text: the payload message ("Upstream Timeout"), the lexicon code + * ("UpstreamFailure"), and lex's payload-less "Upstream server responded with + * a 504 error". Word boundaries keep camelCase identifiers from matching. */ const TRANSIENT_UPSTREAM_MESSAGES = [ /\bupstream\s?failure\b/i, @@ -64,11 +54,9 @@ export function isExpectedSentryNetworkError(value: unknown): boolean { /** * Global backstop for automatic captures that bypass the logger transport. * - * Only exception events are inspected. Sentry sets `hint.originalException` to - * the message *string* for a `captureMessage`, and message events reach Sentry - * from exactly two places: the logger transport, which has already run this - * check before capturing, and user bug reports, which must never be dropped - * for happening to contain a word like "aborted" in the report slug. + * Message events are skipped: Sentry sets `hint.originalException` to the + * message string for `captureMessage`, and the only message producers are the + * logger transport (already filtered) and user bug reports (never droppable). */ export function dropExpectedNetworkErrors( event: ErrorEvent, diff --git a/src/logger/transports/sentry.ts b/src/logger/transports/sentry.ts index 026de76a40..bc802b34d6 100644 --- a/src/logger/transports/sentry.ts +++ b/src/logger/transports/sentry.ts @@ -3,20 +3,14 @@ import {isExpectedSentryNetworkError} from '#/logger/sentry/network-errors' import {LogLevel, type Metadata, type Transport} from '#/logger/types' import {prepareMetadata} from '#/logger/util' -/** - * Metadata keys that have historically carried the underlying failure as a - * plain string. - */ +/** Keys that have historically carried the underlying failure as a plain string. */ const CHECKED_METADATA_KEYS = ['safeMessage', 'message', 'error'] /** * Whether this log records a network failure Sentry should not be told about. - * - * The failure is often passed in metadata rather than as the message, and call - * sites do not consistently use the same key, so any `Error` value counts no - * matter which key holds it. Values of other types only count under the keys - * above: metadata is arbitrary, and scanning all of it lets an unrelated string - * - a stream URL containing "abort", say - suppress a real error. + * Call sites do not agree on a metadata key, so any `Error` value counts under + * any key; other types only count under `CHECKED_METADATA_KEYS` - scanning all + * metadata lets an unrelated string (a URL with "abort") suppress a real error. */ function isExpectedNetworkFailure( message: string | Error, @@ -45,11 +39,7 @@ export const sentryTransport: Transport = ( // Skip debug messages entirely for now - esb if (level === LogLevel.Debug) return - /* - * `__metadata__` is ambient context rather than something the call site - * passed, so it is destructured out of the scanned metadata and folded back - * in here to keep the attached data unchanged. - */ + // ambient __metadata__ is kept out of the scan but stays in the attached data const meta = { __context__: context, ...prepareMetadata(__metadata__ ? {__metadata__, ...metadata} : metadata),