[codex] Filter injected Sentry host errors#435
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
This is a well-scoped, well-tested Sentry noise-filtering PR. The logic is sound and deliberately narrow — it only suppresses events it can positively identify as coming from the injected host hook, which is the right approach. There are two actionable issues: a type mismatch with Sentry's actual beforeSend / beforeBreadcrumb callback signatures (which will produce TypeScript errors or require @ts-ignore workarounds), and one untested filter signal (GETJSURL). Neither is a blocker, but the type issue should be resolved before merge.
3 findings were added inline.
| ) | ||
| } | ||
|
|
||
| export function filterClientSentryBreadcrumb<T extends SentryLikeBreadcrumb>( |
There was a problem hiding this comment.
Warning: filterClientSentryEvent hint type is unknown, mismatching Sentry's beforeSend signature
Sentry's beforeSend option expects a callback typed as (event: Event, hint: EventHint) => PromiseLike<Event | null> | Event | null. The exported filterClientSentryEvent accepts hint?: unknown, which will cause a TypeScript assignability error when passed directly to beforeSend: filterClientSentryEvent in instrumentation-client.ts (unless the project's tsconfig is lenient enough to coerce it).
Similarly, filterClientSentryBreadcrumb doesn't accept a second hint argument at all, whereas beforeBreadcrumb is typed as (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | null. In practice this is safe because the hint is ignored, but it may require a cast at the call site.
Suggestion: Import and use Sentry's own types:
import type { Breadcrumb, BreadcrumbHint, ErrorEvent, EventHint } from "@sentry/nextjs"
export function filterClientSentryEvent(event: ErrorEvent, hint: EventHint): ErrorEvent | null { ... }
export function filterClientSentryBreadcrumb(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): Breadcrumb | null { ... }This eliminates the local SentryLike* type definitions entirely (or you can keep them as internal implementation details) and gives you compile-time assurance the callbacks are correctly shaped.
|
|
||
| expect(filterClientSentryBreadcrumb(breadcrumb)).toBe(breadcrumb) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Warning: GETJSURL filter signal is untested
GETJSURL is checked as an injected-host signal in three places (hasInjectedHostFrame, hasInjectedHostHint, and isInjectedHostBreadcrumb), but no test exercises this path. If the string match or placement is wrong it will silently fail to filter those events — or, if the string is too broad, it could over-filter.
Suggestion: Add at least one test case for each of the three GETJSURL paths (frame, hint, breadcrumb) so that the signal's behaviour is locked in alongside the [jshost] / addEL_hook tests.
| } | ||
|
|
||
| function exceptionText(event: SentryLikeErrorEvent, hint?: unknown): string { | ||
| const exceptionValues = event.exception?.values ?? [] |
There was a problem hiding this comment.
Suggestion: exceptionText unconditionally appends hint text, producing a trailing space when hint is absent
exceptionText always returns "${eventText} ${hintText}". When hint is undefined, valueToSearchText returns "" and the result is "...eventText " (trailing space). This is harmless for .includes() checks, but it also means NULL_TAG_NAME_MESSAGE in the hint's originalException is already covered by exceptionText — making the early-exit check at L97 (if (!text.includes(NULL_TAG_NAME_MESSAGE)) return false) also guard against hint-only cases.
As a consequence, hasInjectedHostHint(hint) at L102 re-parses hint a second time for the addEL_hook / [jshost] / GETJSURL signals that exceptionText does not check. This is correct but the dual-pass on hint content is non-obvious. A brief comment explaining that exceptionText covers the message match while hasInjectedHostHint covers the stack/name signals would help future readers.
Summary
[jshost]/addEL_hooknulltagNamecrash from client Sentry eventsRoot Cause
The Sentry event came from a foreign inline browser/session host hook wrapping
top.addEventListener, not from app code. It crashed while Sentry/Next/rrweb registered navigation and visibility listeners, then our client config captured the injected failure and its base64[jshost]breadcrumbs.Validation
pnpm --filter @memories.sh/web test -- src/lib/sentry/client-filters.test.tspnpm --filter @memories.sh/web typecheckpnpm --filter @memories.sh/web lintpnpm --filter @memories.sh/web buildNeed help on this PR? Tag
@codesmithwith what you need.