Skip to content

[codex] Address Sentry filter review follow-up#436

Merged
charlesrhoward merged 1 commit into
mainfrom
codex/pr435-sentry-review-followup
May 12, 2026
Merged

[codex] Address Sentry filter review follow-up#436
charlesrhoward merged 1 commit into
mainfrom
codex/pr435-sentry-review-followup

Conversation

@charlesrhoward
Copy link
Copy Markdown
Contributor

@charlesrhoward charlesrhoward commented May 12, 2026

Summary

  • follow-up to [codex] Filter injected Sentry host errors #435 for the Mogplex review comments that landed after the original PR was merged
  • uses Sentry callback types for beforeSend and beforeBreadcrumb filters
  • adds GETJSURL coverage for frame, hint, and breadcrumb signal paths
  • documents the message gate vs injected-host evidence split and removes the trailing-space construction

Validation

  • pnpm --filter @memories.sh/web test -- src/lib/sentry/client-filters.test.ts
  • pnpm --filter @memories.sh/web typecheck
  • pnpm --filter @memories.sh/web lint
  • pnpm --filter @memories.sh/web build

View in Codesmith
Need help on this PR? Tag @codesmith with what you need.

  • Let Codesmith autofix CI failures and bot reviews

@vercel
Copy link
Copy Markdown

vercel Bot commented May 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
memories Ignored Ignored Open in v0 May 12, 2026 3:17pm

Request Review

@charlesrhoward charlesrhoward marked this pull request as ready for review May 12, 2026 15:17
@charlesrhoward charlesrhoward enabled auto-merge (squash) May 12, 2026 15:17
@mogplex
Copy link
Copy Markdown

mogplex Bot commented May 12, 2026

Mogplex PR Review

Status: No material issues found

Summary

Clean, purposeful follow-up. No material issues found.

The three substantive changes are all improvements:

  1. SDK types over hand-rolled structs — replacing SentryLike* with Breadcrumb, ErrorEvent, EventHint, StackFrame from @sentry/nextjs is strictly better. It removes an ongoing maintenance burden and lets the compiler catch API drift automatically.

  2. exceptionText join fix — the old ${eventText} ${valueToSearchText(...)} always emitted a trailing space when originalException resolved to "". The new [eventText, ...].filter(Boolean).join(" ") is correct and consistent with the rest of the file.

  3. hint now required — changing hint?: unknown to hint: EventHint is a real API tightening. It's safe: {} satisfies EventHint, all existing call sites have been updated in the test file, and typecheck in the PR validation gate would surface any missed call sites.

New GETJSURL test coverage is well-targeted: one test exercises the frame path, one exercises the hint path, both are isolated and deterministic. The errorEvent() helper is a nice DX touch that satisfies the type: undefined shape requirement without polluting every test fixture.

Verdict

APPROVE — The PR does exactly what it describes, the logic is correct, and the new tests provide meaningful signal coverage.

Affected files:

  • packages/web/src/lib/sentry/client-filters.ts
  • packages/web/src/lib/sentry/client-filters.test.ts

View check run

@charlesrhoward charlesrhoward merged commit 1a8ce19 into main May 12, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant