Repository navigation
Quality foundations: plug the error/quality loop (PR-1 through PR-5, E1/E2) - #3789
Conversation
Replaces the three hand-rolled config objects in shared/sentry-config.ts with a pure `buildSentryConfig(ctx)` factory plus per-surface builders. No environment reads inside the module — each surface resolves its own environment/release/deployment from the API that is truthful there and passes it in, so the module stays importable from browser, Node, and Electron bundles and is testable without stubbing globals. Fixes two mislabels the old NODE_ENV check caused: - The packaged desktop app never sets NODE_ENV, so every shipped Electron event reported `environment: "dev"`. Now `app.isPackaged`. - The browser bundle has no `process`; now `import.meta.env.PROD`. Adds a `deployment` tag (hosted | self_hosted) to every event, an explicit `release` on each surface, and a browser-noise `ignoreErrors` baseline (ResizeObserver x2, AbortError, the offline-fetch spellings) on the browser surfaces only — those strings would suppress real upstream failures on the server. Two build-side consequences of shipping an explicit release: - client/vite.config.ts pins sentryVitePlugin's release name to the same version the SDK inits with, otherwise the plugin invents a git-derived name and uploaded source maps stop resolving. - server/tsup.config.ts bakes the package version into the bundle; there is no package.json beside dist/server/index.js at runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
…ent fallbacks
Three holes let client crashes disappear:
1. `ErrorBoundary.componentDidCatch` only logged to the console. Every one
of its ~21 mounts swallowed its error — most visibly the panels that
render `fallback={null}`, which showed the user an unexplained gap and
told us nothing. Reporting now happens in the boundary itself, so all
mounts report with zero call-site edits; an optional `name` prop keeps
them distinguishable in Sentry.
2. react-router's data router catches route render errors before they can
reach any React boundary above `<RouterProvider>`, so a crashing route
blanked the app silently. The root route gains an `errorElement`
(`RouteErrorScreen`) that reports once via an effect and offers Reload /
Go home. The new root `<ErrorBoundary name="root">` in main.tsx covers
the other half — crashes in the providers wrapping the router — and sits
OUTSIDE PostHogProvider so a provider-init crash is still caught.
3. The auth-bootstrap catch replaces the entire app with a static error
screen and reported nothing.
New `lib/error-reporting.ts` owns the fan-out to Sentry + PostHog. It never
throws: reporting is diagnostics and must not escalate a path that is
already handling a failure. Boundary-caught errors never reach
window.onerror, so there is no double-count with PostHog's global handler.
Fallback burn-down: the five user-visible `fallback={null}` sites now
render either a retryable ErrorCard (billing panels, integrations cards) or
a muted one-liner (top-up button, pending top-ups).
Catch burn-down wave 1: registry quick-connect (previously cleared its
pending state and left the user with a card that never connected — now
reports and toasts), the wire-mode safety-net reconnect, both App Builder
tool-execution paths, and the chat access-token fetch (warning level: the
request still goes out unauthenticated and usually succeeds).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
Turns on the two things that make PostHog Error Tracking exist (`capture_exceptions`) and make a bug report reproducible (session replay), gated to the surfaces where we have standing to record: hosted and the packaged desktop app. npx/Docker installs run on someone else's machine against their own MCP servers — `isErrorCaptureSurface()` keeps them out. `capture_dead_clicks` is on everywhere: it is rageclick's quieter sibling (a click on something that looks interactive and does nothing) and costs no extra network calls. Masking reuses the repo's existing `data-ph-no-capture` convention as the `maskTextSelector` instead of adding the `data-ph-mask` attribute the plan called for. Three overlapping mechanisms (autocapture opt-out, rrweb `rr-block`, replay text masking) now key off ONE annotation, so a new credential surface is a single attribute rather than a checklist. The OAuth flow diagram's access/refresh token rows are annotated — they were displaying truncated values, and a token prefix is still credential material. Two carve-outs beyond the platform gate: - `/results/<token>` is never recorded. The token in that URL IS the credential; scrubSensitiveUrl keeps it out of event properties but a replay would capture it from the DOM anyway. - The VITE_DISABLE_POSTHOG_LOCAL branch now disables recording and exceptions explicitly. `opt_out_capturing_by_default` suppresses event sending, not recorder loading — dev builds were fetching /relay/static/recorder.js on every page load for events they discarded. Adds docs/session-replay-masking.md as the place to look before rendering a secret. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
`server/sentry.ts` was dead code. It called `Sentry.init` as an import side
effect, and ES imports are hoisted above `server/index.ts`'s
`loadInspectorEnv()` — so it initialized against an empty environment on
every start. The inspector-server Sentry project has been near-empty as a
result. It is now an explicit `initServerSentry()` called right after the
env load.
Ownership is consolidated: the logger owns all server Sentry capture.
`Hono.onError` routes through `appLogger.error` (now with path + method,
without which every unhandled route error groups into one useless blob),
and typed events stay opt-in via `{ sentry: true }`.
`logger.warn` no longer captures. Every warn in the tree used to become a
Sentry event — across self-hosted installs that is the largest quota-spike
vector we have, and a warning is by definition something we chose not to
treat as a failure. Warnings keep going to Axiom.
Guardrails, because this turns on across every OSS install at once:
- `DO_NOT_TRACK` disables it (the same opt-out analytics already honors).
- `SENTRY_ERROR_SAMPLE_RATE` is a quota brake that needs no deploy.
- `tracesSampleRate: 0` — spans would multiply the exposure of a change
whose point is to see errors.
- The default `OnUnhandledRejection` integration is filtered out;
index.ts's own handler deliberately swallows the MCP SDK's routine
"Connection closed" rejections and the integration would capture them all.
`OnUncaughtException` stays for its capture-then-exit behavior, so the new
`uncaughtException` handler logs to Axiom WITHOUT `sentry: true`.
Two bugs the tests caught while writing them:
- Empty-string env vars. Container platforms materialize declared-but-unset
variables as `""`, which `??` does not catch, and `Number("")` is `0` — an
empty `SENTRY_ERROR_SAMPLE_RATE` would have silently dropped 100% of
events.
- The release read has to stay a literal `process.env.X` member expression;
esbuild's `define` is a syntactic substitution and cannot see through
`process.env[name]`. Verified against the built bundle
(`BAKED_VERSION = "2.34.0"`).
Verified: full server suite green (4743), `DO_NOT_TRACK=1 node
dist/server/index.js` logs the disabled init. Rollout risk plan in
docs/server-error-reporting.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
The Continue button on the OAuth debugger is the surface's worst rageclick hotspot (211 rageclicks / 67 people). The cause is plain once you look: `handleAdvance` awaited a network round trip while the button kept its idle label and stayed enabled, so users clicked it again. Its sibling buttons on the same toolbar already had the convention — Connect shows "Connecting...", Refresh shows "Refreshing...". Continue now shows "Continuing...", disables, and sets aria-busy. Three silent failure paths on the same surface: - `handleAdvance` had no catch. Anything escaping the state machine rejected into nothing. - The post-callback auto-advance fired `proceedToNextStep()` on a 500ms timer with no rejection handler — a token exchange that threw left the UI frozen mid-flow with nothing recorded anywhere. - The Electron callback handler's catch was console-only. All three now report and put a message where the user can see it. Step failures are reported by wrapping `updateState` in the adapter. That is the only reliable hook: the SDK state machine catches its own step errors and never rethrows — it writes the message into flow state and returns normally. Reported at `warning` level, because a failing step is frequently the server-under-test misbehaving, which is what a debugger is for; the value is the aggregate trend, not a page. Deduped per distinct message, and re-armed when the error clears so a retry that fails identically counts as a new failure. Flow errors also raise one toast per new error, keyed off the state transition rather than the render — they previously appeared only as inline log text, easy to miss while watching the sequence diagram. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
E1 — crash handlers.
Checked @sentry/electron 5.12's defaults first, as planned. Two findings
changed the shape of this:
- `sentryMinidumpIntegration` (native crash upload) is already default-on.
Nothing to enable; documented and left alone.
- `childProcessIntegration` already registers `render-process-gone` and
`child-process-gone`, but its default `events` list is only
`["abnormal-exit", "launch-failed", "integrity-failure"]`. `crashed`,
`oom`, and `killed` are recorded as BREADCRUMBS only — they surface as
context on some later event and produce no issue of their own. Those three
are precisely what a desktop user experiences as "the app broke".
So rather than registering our own `app.on("render-process-gone")` — which
would double-report every reason the integration already covers — the
integration is reconfigured to capture the crash-shaped reasons as events.
The main-process `uncaughtException` / `unhandledRejection` handlers do NOT
capture either: Sentry's default integrations already do. They exist to
write to electron-log, which is the file a user attaches to a bug report and
the only diagnostic available when the crash happens offline or with
reporting opted out. Both listener bodies are wrapped — an exception thrown
from inside an uncaughtException handler is unrecoverable, and a failing log
transport must not be what takes the app down.
E2 — source maps.
The renderer build had no `build` block at all, so it emitted no source maps
and every desktop issue in Sentry showed minified frames. Both Electron vite
configs now emit them, and the mac/windows release workflows inject debug ids
and upload to inspector-electron (`.vite/build`) and inspector-client
(`.vite/renderer`) under a release name matching what the SDKs init with.
`continue-on-error`, mirroring the SDK precedent in release.yml — a Sentry
hiccup must never fail an otherwise-good signed release. No secrets plumbing
needed; both workflows already get `secrets: inherit`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
GitHub Actions
Inactive
dosubot
Bot
added
size:XL
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds shared Sentry configuration and error-reporting utilities. Client flows report failures, render recovery states, track OAuth continuation, and mask sensitive replay data. Server startup initializes Sentry explicitly and handles process-level errors. Electron adds crash reporting and source maps. Release workflows upload sourcemaps to Sentry. Tests and documentation cover reporting, privacy, recovery, and configuration behavior. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/mac-release.yml:
- Around line 106-126: Update the release steps in
.github/workflows/mac-release.yml (lines 106-126) and
.github/workflows/windows-release.yml (lines 86-107) so Sentry sourcemap
injection and upload run immediately after npm run build and before
electron:make; remove generated .map files before packaging, while preserving
each workflow’s existing release, organization, project, and artifact-directory
settings.
In `@mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx`:
- Around line 277-285: The reactive wire-mode reconnect effect’s catch still
discards failures; update that effect’s catch near the read-back flow to call
reportCaught with source "server_detail_wire_mode_reconnect" and level
"warning", matching the existing timeout fallback catch.
In `@mcpjam-inspector/client/src/components/ui/__tests__/error-boundary.test.tsx`:
- Around line 6-10: Move the reportBoundaryError mock declaration into
vi.hoisted before the vi.mock factory, destructuring the vi.fn() from its
returned object, and remove the current pre-mock declaration so the factory uses
the hoisted mock safely.
In `@mcpjam-inspector/client/src/lib/error-reporting.ts`:
- Around line 50-55: Update reportCaught in error-reporting.ts so PostHog
captureException runs only when isErrorCaptureSurface() &&
!isCredentialBearingPath() permits it, while keeping Sentry reporting
independent. Add or update coverage in
mcpjam-inspector/client/src/lib/__tests__/error-reporting.test.ts (lines 25-39)
to verify the self-hosted case does not call PostHog.
In `@mcpjam-inspector/docs/server-error-reporting.md`:
- Around line 23-36: The documentation’s capture-then-exit claim is missing the
required Sentry configuration. Update the Sentry setup to register
onUncaughtExceptionIntegration with exitEvenIfOtherHandlersAreRegistered enabled
before describing the uncaughtException path as capture-then-exit, while
preserving the existing Axiom handler behavior.
In `@mcpjam-inspector/server/index.ts`:
- Around line 92-102: Keep mcpjam-inspector/server/index.ts lines 92-102 as
logging-only with no direct change. In mcpjam-inspector/server/sentry.ts lines
52-59, replace the filtered default integrations with
Sentry.onUncaughtExceptionIntegration configured with
exitEvenIfOtherHandlersAreRegistered: true, ensuring uncaught exceptions
terminate the server. Update mcpjam-inspector/docs/server-error-reporting.md
lines 23-36 to document this explicit integration option and termination
behavior.
In `@mcpjam-inspector/server/sentry.ts`:
- Around line 32-43: Apply the existing DO_NOT_TRACK-derived enabled gate to
Axiom logger initialization and ingestion, ensuring logger.error, logger.warn,
logger.info, and logger.debug do not call axiom.ingest when tracking is
disabled. Keep the Sentry.init configuration and existing stderr behavior
unchanged, and reuse the enabled value defined alongside Sentry initialization.
- Around line 90-95: Update resolveErrorSampleRate so whitespace-only
SENTRY_ERROR_SAMPLE_RATE values are treated as unset and return the default rate
of 1. Either make the existing blank helper trim whitespace or check raw.trim()
before Number parsing, while preserving validation for numeric values.
In `@mcpjam-inspector/shared/sentry-config.ts`:
- Around line 43-47: Restrict the Convex URL pattern in
TRACE_PROPAGATION_TARGETS to require a host boundary immediately after the
cloud/site suffix, preventing matches such as convex.cloud.evil. In
mcpjam-inspector/shared/__tests__/sentry-config.test.ts lines 57-69, add
coverage asserting suffix-confusion URLs do not match.
- Around line 93-100: Gate both Replay sample rates in buildClientSentryConfig
based on replay eligibility, disabling them for self-hosted web sessions. In
mcpjam-inspector/client/src/lib/sentry.ts lines 13-18, route eligibility through
the existing hosted/Electron boundary used by PostHog/local recording while
keeping self-hosted web replay disabled. Update
mcpjam-inspector/client/src/lib/__tests__/sentry.test.ts lines 56-67 to default
replay sampling to 0 for self-hosted/web, and add coverage in
mcpjam-inspector/shared/__tests__/sentry-config.test.ts lines 88-98 for the
replay-disabled self-hosted configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6462eec4-a975-4ce4-a80a-9e07740b6976
📒 Files selected for processing (47)
.github/workflows/mac-release.yml.github/workflows/windows-release.ymlmcpjam-inspector/client/src/components/OAuthFlowTab.tsxmcpjam-inspector/client/src/components/RouteErrorScreen.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/__tests__/RouteErrorScreen.test.tsxmcpjam-inspector/client/src/components/billing/CreditBalanceCard.tsxmcpjam-inspector/client/src/components/billing/TopupActionButton.tsxmcpjam-inspector/client/src/components/billing/__tests__/CreditBalanceCard.fallback.test.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/oauth/OAuthFlowLogger.tsxmcpjam-inspector/client/src/components/oauth/OAuthFlowProgress.tsxmcpjam-inspector/client/src/components/oauth/__tests__/OAuthFlowLogger.continue-pending.test.tsxmcpjam-inspector/client/src/components/organization/OrganizationBillingSection.tsxmcpjam-inspector/client/src/components/settings/IntegrationsRoute.tsxmcpjam-inspector/client/src/components/ui-playground/hooks/useToolExecution.tsmcpjam-inspector/client/src/components/ui/__tests__/error-boundary.test.tsxmcpjam-inspector/client/src/components/ui/error-boundary.tsxmcpjam-inspector/client/src/hooks/use-chat.tsmcpjam-inspector/client/src/lib/PosthogUtils.tsmcpjam-inspector/client/src/lib/__tests__/error-reporting.test.tsmcpjam-inspector/client/src/lib/__tests__/posthog-utils.test.tsmcpjam-inspector/client/src/lib/__tests__/sentry.test.tsmcpjam-inspector/client/src/lib/error-reporting.tsmcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.tsmcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.tsmcpjam-inspector/client/src/lib/sentry.tsmcpjam-inspector/client/src/main.tsxmcpjam-inspector/client/src/router.tsxmcpjam-inspector/client/vite.config.tsmcpjam-inspector/docs/server-error-reporting.mdmcpjam-inspector/docs/session-replay-masking.mdmcpjam-inspector/server/__tests__/sentry.test.tsmcpjam-inspector/server/index.tsmcpjam-inspector/server/middleware/request-log-context.tsmcpjam-inspector/server/sentry.tsmcpjam-inspector/server/tsup.config.tsmcpjam-inspector/server/utils/__tests__/logger.test.tsmcpjam-inspector/server/utils/log-events.tsmcpjam-inspector/server/utils/logger.tsmcpjam-inspector/shared/__tests__/sentry-config.test.tsmcpjam-inspector/shared/sentry-config.tsmcpjam-inspector/src/crash-reporting.test.tsmcpjam-inspector/src/crash-reporting.tsmcpjam-inspector/src/main.tsmcpjam-inspector/vite.main.config.tsmcpjam-inspector/vite.renderer.config.mts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 47 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/src/main.ts">
<violation number="1" location="mcpjam-inspector/src/main.ts:59">
P2: Main-process uncaught exceptions no longer show Electron's crash dialog: registering this listener after `Sentry.init` makes Sentry's listener-count guard suppress the user-facing fatal-error UI. Register the logging listener before `Sentry.init`, or provide an `onFatalError` callback that explicitly preserves the dialog.</violation>
</file>
<file name="mcpjam-inspector/server/sentry.ts">
<violation number="1" location="mcpjam-inspector/server/sentry.ts:63">
P3: The DO_NOT_TRACK startup notice bypasses the centralized server logging contract and can produce unconditional production stderr output unlike other server diagnostics. Route this through a DNT-safe centralized logging path, or document and implement an explicit local-only logger exception for opt-out notices.</violation>
</file>
<file name="mcpjam-inspector/server/utils/logger.ts">
<violation number="1" location="mcpjam-inspector/server/utils/logger.ts:75">
P3: The new warning behavior contradicts the repository’s contributor contract, so code written according to `AGENTS.md` will no longer receive the promised Sentry capture. Updating `AGENTS.md` and related logging guidance to document Axiom-only warnings and the explicit typed-event opt-in would keep the telemetry contract accurate.</violation>
</file>
<file name="mcpjam-inspector/client/src/components/oauth/OAuthFlowProgress.tsx">
<violation number="1" location="mcpjam-inspector/client/src/components/oauth/OAuthFlowProgress.tsx:847">
P1: The active guided OAuth flow still exposes full access and refresh tokens to PostHog/rrweb because `AuthTab` uses `OAuthFlowProgressSimple`, while these new markers are only in the unused `OAuthFlowProgress` component. Applying the same `data-ph-no-capture`/`ph-no-capture rr-block` annotation to the active token `<pre>` (or switching the active flow to this masked rendering) would close the credential-telemetry gap.</violation>
</file>
<file name="mcpjam-inspector/vite.renderer.config.mts">
<violation number="1" location="mcpjam-inspector/vite.renderer.config.mts:83">
P2: The comment says "the maps are not shipped in the installer," but nothing removes them: the release workflows upload .vite/renderer maps to Sentry without deleting them, and forge.config.ts doesn't exclude *.map, so the source maps are packaged into the asar and shipped in every installer. Delete the .map files after the Sentry upload step (matching the web build's filesToDeleteAfterUpload convention) so the stated intent holds and the installer doesn't include full loose source maps.</violation>
</file>
<file name="mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts">
<violation number="1" location="mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts:235">
P2: The debugger will emit exception telemetry for the non-fatal S256 compatibility warning, inflating the step-failure/error signal even though the OAuth flow continues successfully. Excluding explicitly non-fatal warnings (or representing them separately from `state.error`) would keep the quality metrics meaningful.</violation>
</file>
<file name="mcpjam-inspector/server/__tests__/sentry.test.ts">
<violation number="1" location="mcpjam-inspector/server/__tests__/sentry.test.ts:115">
P3: This 'release baked in' test can't actually validate the build-time define: under vitest the esbuild `define` substitution never occurs, so BAKED_VERSION is read straight from process.env — the same runtime path the 'falls back to npm_package_version' test covers. A regression that breaks the literal `process.env.X` requirement (e.g. a dynamic lookup) would still pass here and ship a server with no release tag in prod. Consider an integration/build-level check that runs the bundled output (tsup) and confirms MCPJAM_INSPECTOR_VERSION is substituted, or at least rename/comment this test to be clear it only guards the env-read fallback, not the bake.</violation>
</file>
<file name="mcpjam-inspector/client/src/components/OAuthFlowTab.tsx">
<violation number="1" location="mcpjam-inspector/client/src/components/OAuthFlowTab.tsx:325">
P3: This new effect routes every flow-state error to `toast.error`, which in this repo renders as a persistent (duration: Infinity) error toast with a copy button. On the OAuth debugger surface, flow-state errors are frequently routine, expected outcomes of testing a misbehaving server (failed metadata fetch, issuer/PKCE mismatch) — exactly the cases a debugger exists to surface. A session that iterates on a broken server will now accumulate persistent error toasts that must each be manually dismissed, where they previously only appeared as inline transcript text. Consider a lower-persistence or less prominent toast for debugger flow errors (e.g. a default-duration or info-level toast), reserving the persistent error toast for genuinely unexpected escapes.</violation>
<violation number="2" location="mcpjam-inspector/client/src/components/OAuthFlowTab.tsx:778">
P2: Switching OAuth targets during an in-flight auto exchange can show the old target's failure on the new target and alter its pending state; guarding the callback with a flow/server generation or aborting and ignoring it during cleanup would keep stale work from mutating the active flow.</violation>
</file>
<file name="mcpjam-inspector/client/src/main.tsx">
<violation number="1" location="mcpjam-inspector/client/src/main.tsx:243">
P3: On the session-token bootstrap failure branch, PostHog is not yet initialized (PostHogProvider only mounts on the success render path), so `reportCaught`'s `posthog.captureException` silently no-ops. That means this most severe failure — the one that replaces the whole app with a static error screen — only reaches Sentry, not PostHog, and it also emits an 'is not initialized' console warning exactly where the user-facing console is already noisy from the underlying error. Consider initializing the PostHog SDK before the bootstrap, or otherwise acknowledging this path is Sentry-only; if PostHog coverage of this specific failure matters, mount the SDK earlier rather than relying on the PostHogProvider-only init.</violation>
</file>
<file name="mcpjam-inspector/server/index.ts">
<violation number="1" location="mcpjam-inspector/server/index.ts:96">
P2: The new `process.on("uncaughtException", ...)` handler silently disables the fail-fast crash the comments and `docs/server-error-reporting.md` say they want.
I verified against the shipped `@sentry/node@8.47.0` source (`build/cjs/integrations/onuncaughtexception.js`): the `OnUncaughtException` integration computes `processWouldExit = userProvidedListenersCount === 0` and only applies fatal handling (which calls `logAndExitProcess`) when `exitEvenIfOtherHandlersAreRegistered || processWouldExit`. Because this handler is registered in `index.ts` before `initServerSentry()`, Sentry sees one user-provided listener, so with the default `exitEvenIfOtherHandlersAreRegistered: false` it will **not** exit the process on an uncaught exception.
Net effect: the comment says the integration "owns ... (and the exit)" and "capture-then-exit behavior is what we want", and the docs' table/ad copy echo that — but after this change an uncaught exception is captured and logged while the server keeps running in a possibly-undefined state, with no supervisor restart. If crash-fast termination is desired (the documented intent), this handler defeats it. Either drop the handler, or set `exitEvenIfOtherHandlersAreRegistered: true` on the integration so the extra listener doesn't suppress the exit; either way the doc/comment need updating to describe the real behavior.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| { | ||
| label: "Access Token", | ||
| value: truncateValue(flowState.oauthTokens.access_token), | ||
| sensitive: true, |
There was a problem hiding this comment.
P1: The active guided OAuth flow still exposes full access and refresh tokens to PostHog/rrweb because AuthTab uses OAuthFlowProgressSimple, while these new markers are only in the unused OAuthFlowProgress component. Applying the same data-ph-no-capture/ph-no-capture rr-block annotation to the active token <pre> (or switching the active flow to this masked rendering) would close the credential-telemetry gap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/components/oauth/OAuthFlowProgress.tsx, line 847:
<comment>The active guided OAuth flow still exposes full access and refresh tokens to PostHog/rrweb because `AuthTab` uses `OAuthFlowProgressSimple`, while these new markers are only in the unused `OAuthFlowProgress` component. Applying the same `data-ph-no-capture`/`ph-no-capture rr-block` annotation to the active token `<pre>` (or switching the active flow to this masked rendering) would close the credential-telemetry gap.</comment>
<file context>
@@ -822,11 +844,13 @@ export const OAuthFlowProgress = ({
{
label: "Access Token",
value: truncateValue(flowState.oauthTokens.access_token),
+ sensitive: true,
},
flowState.oauthTokens.refresh_token
</file context>
| // The baked version is captured at module load (it has to stay a literal | ||
| // `process.env.X` so esbuild's `define` can substitute it), so these two | ||
| // re-import the module rather than stubbing after the fact. | ||
| it("uses the release baked in at build time", async () => { |
There was a problem hiding this comment.
P3: This 'release baked in' test can't actually validate the build-time define: under vitest the esbuild define substitution never occurs, so BAKED_VERSION is read straight from process.env — the same runtime path the 'falls back to npm_package_version' test covers. A regression that breaks the literal process.env.X requirement (e.g. a dynamic lookup) would still pass here and ship a server with no release tag in prod. Consider an integration/build-level check that runs the bundled output (tsup) and confirms MCPJAM_INSPECTOR_VERSION is substituted, or at least rename/comment this test to be clear it only guards the env-read fallback, not the bake.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/__tests__/sentry.test.ts, line 115:
<comment>This 'release baked in' test can't actually validate the build-time define: under vitest the esbuild `define` substitution never occurs, so BAKED_VERSION is read straight from process.env — the same runtime path the 'falls back to npm_package_version' test covers. A regression that breaks the literal `process.env.X` requirement (e.g. a dynamic lookup) would still pass here and ship a server with no release tag in prod. Consider an integration/build-level check that runs the bundled output (tsup) and confirms MCPJAM_INSPECTOR_VERSION is substituted, or at least rename/comment this test to be clear it only guards the env-read fallback, not the bake.</comment>
<file context>
@@ -0,0 +1,133 @@
+ // The baked version is captured at module load (it has to stay a literal
+ // `process.env.X` so esbuild's `define` can substitute it), so these two
+ // re-import the module rather than stubbing after the fact.
+ it("uses the release baked in at build time", async () => {
+ vi.stubEnv("MCPJAM_INSPECTOR_VERSION", "2.34.0");
+ vi.resetModules();
</file context>
| console.error("[Auth] Failed to initialize session token:", error); | ||
| // This branch replaces the whole app with a static screen — without a | ||
| // report the failure is invisible outside the user's own console. | ||
| reportCaught(error, { source: "session_token_bootstrap" }); |
There was a problem hiding this comment.
P3: On the session-token bootstrap failure branch, PostHog is not yet initialized (PostHogProvider only mounts on the success render path), so reportCaught's posthog.captureException silently no-ops. That means this most severe failure — the one that replaces the whole app with a static error screen — only reaches Sentry, not PostHog, and it also emits an 'is not initialized' console warning exactly where the user-facing console is already noisy from the underlying error. Consider initializing the PostHog SDK before the bootstrap, or otherwise acknowledging this path is Sentry-only; if PostHog coverage of this specific failure matters, mount the SDK earlier rather than relying on the PostHogProvider-only init.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/main.tsx, line 243:
<comment>On the session-token bootstrap failure branch, PostHog is not yet initialized (PostHogProvider only mounts on the success render path), so `reportCaught`'s `posthog.captureException` silently no-ops. That means this most severe failure — the one that replaces the whole app with a static error screen — only reaches Sentry, not PostHog, and it also emits an 'is not initialized' console warning exactly where the user-facing console is already noisy from the underlying error. Consider initializing the PostHog SDK before the bootstrap, or otherwise acknowledging this path is Sentry-only; if PostHog coverage of this specific failure matters, mount the SDK earlier rather than relying on the PostHogProvider-only init.</comment>
<file context>
@@ -236,6 +238,9 @@ if (isInIframe) {
console.error("[Auth] Failed to initialize session token:", error);
+ // This branch replaces the whole app with a static screen — without a
+ // report the failure is invisible outside the user's own console.
+ reportCaught(error, { source: "session_token_bootstrap" });
// Show error UI instead of crashing
root.render(
</file context>
| } | ||
| if (lastToastedErrorRef.current === error) return; | ||
| lastToastedErrorRef.current = error; | ||
| toast.error(error); |
There was a problem hiding this comment.
P3: This new effect routes every flow-state error to toast.error, which in this repo renders as a persistent (duration: Infinity) error toast with a copy button. On the OAuth debugger surface, flow-state errors are frequently routine, expected outcomes of testing a misbehaving server (failed metadata fetch, issuer/PKCE mismatch) — exactly the cases a debugger exists to surface. A session that iterates on a broken server will now accumulate persistent error toasts that must each be manually dismissed, where they previously only appeared as inline transcript text. Consider a lower-persistence or less prominent toast for debugger flow errors (e.g. a default-duration or info-level toast), reserving the persistent error toast for genuinely unexpected escapes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/components/OAuthFlowTab.tsx, line 325:
<comment>This new effect routes every flow-state error to `toast.error`, which in this repo renders as a persistent (duration: Infinity) error toast with a copy button. On the OAuth debugger surface, flow-state errors are frequently routine, expected outcomes of testing a misbehaving server (failed metadata fetch, issuer/PKCE mismatch) — exactly the cases a debugger exists to surface. A session that iterates on a broken server will now accumulate persistent error toasts that must each be manually dismissed, where they previously only appeared as inline transcript text. Consider a lower-persistence or less prominent toast for debugger flow errors (e.g. a default-duration or info-level toast), reserving the persistent error toast for genuinely unexpected escapes.</comment>
<file context>
@@ -303,6 +308,23 @@ export const OAuthFlowTab = ({
+ }
+ if (lastToastedErrorRef.current === error) return;
+ lastToastedErrorRef.current = error;
+ toast.error(error);
+ }, [oauthFlowState.error]);
+
</file context>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 37e4bd7. Configure here.
**CI fix.** `PosthogUtils.options` is a module-scope literal, and calling
`isErrorCaptureSurface()` in it made merely IMPORTING the module read
`HOSTED_MODE` from `@/lib/config`. 62 test files partially mock that module,
so `integrations-route.test.tsx` blew up on import. The capture-surface
fields are getters now — evaluated on property access, which is when
posthog-js reads config anyway.
I missed this locally: my full-suite run had 115 suites failing to LOAD
because the SDK wasn't built, which masked it. Re-run after building: 13,162
pass.
**Three findings contradicted the plan's locked decisions and are fixed:**
- `reportCaught` sent to PostHog unconditionally. `capture_exceptions:
false` only disables posthog-js's automatic window.onerror handler — an
explicit `captureException` still sends, so every npx/Docker install was
shipping caught errors to the shared project. Now gated on the same
predicate. The gate evaluation is INSIDE the try: this runs in
`componentDidCatch`, where a throw would escape the boundary that just
caught something. Fails closed.
- **Sentry Replay** was unconditional. I gated PostHog replay and missed
that Sentry Replay records DOM+text the same way, so self-hosted web
sessions were still recorded — the exact boundary PR-4 drew. Now routed
through `isErrorCaptureSurface()`, and the integration doesn't even load
when off (zero rates alone still ship the recorder).
- **The masking went on a dead component.** `OAuthFlowProgress` is imported
by nobody. `OAuthFlowProgressSimple` — what `AuthTab` actually renders —
dumps `JSON.stringify(oauthTokens)` UNTRUNCATED. Annotated.
**Two crash-behavior bugs:**
- @sentry/node 8 computes `processWouldExit = userProvidedListenersCount
=== 0`. Both the server and Electron main register their own
`uncaughtException` listener, so Sentry would capture and then NOT exit —
turning fail-fast into a zombie process, while my own comment claimed
"capture + exit preserved". Both now register
`onUncaughtExceptionIntegration({ exitEvenIfOtherHandlersAreRegistered:
true })` explicitly.
- Sourcemap injection ran AFTER `electron:make`, so the installer shipped
pre-injection bundles and nothing could symbolicate. Moved between build
and package; also uploads `dist/client` (what the packaged app actually
serves via the embedded server) and deletes the maps before packaging.
**Privacy/correctness:**
- `/results/<token>` recording is now enforced at runtime too. The
init-time flag only covers a session that LOADS there; it's an in-app
route, so navigating to it kept an active recorder snapshotting the
address bar.
- `RouteErrorScreen` shipped the raw pathname — including that token.
- Convex trace-propagation regex matched `x.convex.cloud.evil`, which would
attach trace+baggage headers to a suffix-controlled origin.
- OAuth step errors are text from the server UNDER TEST; userinfo in
embedded URLs is stripped and length capped.
- `DO_NOT_TRACK` now gates Axiom too — the startup notice says "error
reporting disabled" and that has to be true of every sink.
- `BROWSER_IGNORE_ERRORS` removed from the Electron MAIN process (Node, not
a browser): "Failed to fetch" there is a real updater/startup failure.
- Whitespace-only `SENTRY_ERROR_SAMPLE_RATE` — `Number(" ")` is 0.
- Second wire-mode reconnect catch (the common path) also reports.
- Step reports attribute to the step the update moves TO.
Also: AGENTS.md updated for the warn-no-longer-captures contract, doc paths
fixed, and two tests strengthened (the fallback test now clicks Retry with
production's `reset` wiring; the env test no longer mirrors the
implementation expression).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0b16c40f-9e4f-414c-850d-821b647db3ba) |
`useSessionRecordingPathGuard` used `useLocation()`, which throws its router invariant when `App` renders outside a router — the legacy hash path and several test harnesses do exactly that. It took down all 62 tests in App.hosted-oauth.test.tsx. The codebase already documents this rule: `AppContent` reads `getRouteFallbackPathname()` and carries the comment "Deliberately NOT `useLocation`: that throws its router invariant on the no-router path". I added a hook to App that broke it. Now subscribes to the module-level router ref (`router-ref.ts`) — the repo's existing escape hatch for non-React and no-router callers. It applies once for the current location (so a hard load onto `/results/` is covered before any navigation) and then on each router state change, and is a no-op when there is no router at all. Verified: App.hosted-oauth.test.tsx 62/62, posthog-utils 21/21, typecheck clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
GitHub Actions
Error
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2a234efb-3284-4509-b3ed-d1b2fb1ad615) |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/src/crash-reporting.ts (1)
25-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a fallback exit path for opt-out or failed Sentry init.
registerMainProcessCrashHandlerssuppresses Node’s defaultuncaughtExceptionexit. The forced Sentry exit requiresSentry.initto succeed andcrashReportingIntegrations()to run normally; disabled or failed Electron Sentry initialization leaves these listeners with no exit behavior. Add anuncaughtExceptionMonitoror explicit fallbackprocess.exit(1)after logging that does not pre-empt Sentry flushing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/src/crash-reporting.ts` around lines 25 - 58, Update registerMainProcessCrashHandlers so uncaughtException still exits with status 1 when Sentry is disabled or initialization fails, while allowing Sentry’s uncaught-exception integration to flush first. Add an uncaughtExceptionMonitor or equivalent fallback exit path after logging, and preserve the existing logger-failure protection and unhandledRejection behavior.
🧹 Nitpick comments (1)
mcpjam-inspector/src/crash-reporting.ts (1)
60-71: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep
killedoutside the captured child-process events.
killedcan describe normal SIGTERM/external termination as well, while Electron only hasapp.relaunch, not a clean restart exit reason. Gate it on an unexpected-exit state; otherwise keep it as a breadcrumb and exclude it fromCAPTURED_EXIT_REASONS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/src/crash-reporting.ts` around lines 60 - 71, Update childProcessIntegrationOptions and CAPTURED_EXIT_REASONS so killed is excluded from the captured child-process event list and remains only as a breadcrumb. If killed must trigger reporting, gate it on an unexpected-exit state rather than treating every killed event as a crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.ts`:
- Around line 21-24: Replace the useEffect guard around
syncSessionRecordingForPath with useLayoutEffect, preserving the posthog and
location.pathname dependencies so recording stops before the results route is
painted. Add a hook-level test covering a route transition to /results/<token>
and asserting the session recording is stopped before the new path is committed.
In `@mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts`:
- Around line 236-239: Expand sanitizeStepError to redact OAuth-sensitive query
parameters, JSON credential fields, and Authorization Bearer or Basic values
before reportCaught constructs the reported Error, while preserving the existing
URL userinfo redaction and length limit. Add coverage in
debug-state-machine-step-reporting.test.ts for query, JSON,
authorization-header, error-handling, and relevant edge cases; update both
affected files as specified.
---
Outside diff comments:
In `@mcpjam-inspector/src/crash-reporting.ts`:
- Around line 25-58: Update registerMainProcessCrashHandlers so
uncaughtException still exits with status 1 when Sentry is disabled or
initialization fails, while allowing Sentry’s uncaught-exception integration to
flush first. Add an uncaughtExceptionMonitor or equivalent fallback exit path
after logging, and preserve the existing logger-failure protection and
unhandledRejection behavior.
---
Nitpick comments:
In `@mcpjam-inspector/src/crash-reporting.ts`:
- Around line 60-71: Update childProcessIntegrationOptions and
CAPTURED_EXIT_REASONS so killed is excluded from the captured child-process
event list and remains only as a breadcrumb. If killed must trigger reporting,
gate it on an unexpected-exit state rather than treating every killed event as a
crash.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 26def29e-aad0-40e7-8767-1701997b8ecc
📒 Files selected for processing (28)
.github/workflows/mac-release.yml.github/workflows/windows-release.ymlmcpjam-inspector/AGENTS.mdmcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/RouteErrorScreen.tsxmcpjam-inspector/client/src/components/billing/__tests__/CreditBalanceCard.fallback.test.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/oauth/OAuthFlowProgressSimple.tsxmcpjam-inspector/client/src/components/ui/__tests__/error-boundary.test.tsxmcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.tsmcpjam-inspector/client/src/lib/PosthogUtils.tsmcpjam-inspector/client/src/lib/__tests__/error-reporting.test.tsmcpjam-inspector/client/src/lib/__tests__/posthog-utils.test.tsmcpjam-inspector/client/src/lib/__tests__/sentry.test.tsmcpjam-inspector/client/src/lib/error-reporting.tsmcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.tsmcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.tsmcpjam-inspector/client/src/lib/sentry.tsmcpjam-inspector/docs/quality-triage.mdmcpjam-inspector/docs/server-error-reporting.mdmcpjam-inspector/docs/session-replay-masking.mdmcpjam-inspector/server/__tests__/sentry.test.tsmcpjam-inspector/server/sentry.tsmcpjam-inspector/server/utils/logger.tsmcpjam-inspector/shared/__tests__/sentry-config.test.tsmcpjam-inspector/shared/sentry-config.tsmcpjam-inspector/src/crash-reporting.test.tsmcpjam-inspector/src/crash-reporting.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- mcpjam-inspector/client/src/components/billing/tests/CreditBalanceCard.fallback.test.tsx
- mcpjam-inspector/src/crash-reporting.test.ts
- mcpjam-inspector/client/src/components/RouteErrorScreen.tsx
- .github/workflows/windows-release.yml
- mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx
- mcpjam-inspector/client/src/lib/tests/error-reporting.test.ts
- .github/workflows/mac-release.yml
- mcpjam-inspector/shared/tests/sentry-config.test.ts
- mcpjam-inspector/client/src/lib/error-reporting.ts
- mcpjam-inspector/client/src/components/ui/tests/error-boundary.test.tsx
- mcpjam-inspector/docs/session-replay-masking.md
| useEffect(() => { | ||
| if (!posthog) return; | ||
| syncSessionRecordingForPath(posthog, location.pathname); | ||
| }, [posthog, location.pathname]); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate target and related files"
fd -a 'useSessionRecordingPathGuard\.ts$|Package\..*json$|package\.json$|posthog|sessionRecording|results' . | sed 's#^\./##' | head -100
echo
echo "Target file"
if [ -f mcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.ts ]; then
cat -n mcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.ts
fi
echo
echo "Relevant package versions"
for f in package.json mcpjam-inspector/client/package.json mcpjam-inspector/client/package-lock.json mcpjam-inspector/client/yarn.lock mcpjam-inspector/client/pnpm-lock.yaml; do
[ -f "$f" ] && echo "--- $f" && (rg -n '"react"\s*:"|posthog-js|rut' "$f" || true) | head -50
done
echo
echo "Search route/result security/session recording usage"
rg -n "results|token|sessionRecording|syncSessionRecordingForPath|startSessionRecording|stopSessionRecording|useSessionRecordingPathGuard|location.pathname|history|navigate" mcpjam-inspector/client/src -S || trueRepository: MCPJam/inspector
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target hook"
cat -n mcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.ts
echo
echo "Focused session recording related files"
fd -a 'SessionRecording|recording|PostHog|posthog|score|results|route|navigation' mcpjam-inspector/client/src | sed 's#^\./##' | head -80
echo
echo "Focused uses of useSessionRecordingPathGuard and syncSessionRecordingForPath"
rg -n "useSessionRecordingPathGuard|syncSessionRecordingForPath|startSessionRecording|stopSessionRecording|sessionRecording" mcpjam-inspector/client/src -S
echo
echo "Focused route/token comments and implementations"
rg -n 'results.*token|token.*results|/results|fetchScoreRun|score/runs|useSessionRecordingPathGuard|useEffect|useLayoutEffect|history\.watch|history\.listen|window\.history|navigateToUrl|navigateApp|createMemoryHistory' mcpjam-inspector/client/src -S --max-count 200
echo
echo "Focused test file for hook, if any"
fd -a '__tests__|test\.ts|test\.tsx|spec\.ts|spec\.tsx' mcpjam-inspector/client/src/hooks | sed 's#^\./##' ; rg -n "useSessionRecordingPathGuard" mcpjam-inspector/client/src/hooks -S || true
echo
echo "Package dependency snippets"
for f in mcpjam-inspector/client/package.json; do
[ -f "$f" ] && {
echo "--- $f"
node - <<'JS'
const fs = require('fs');
const p = JSON.parse(fs.readFileSync(process.argv[1], 'utf8'));
const print = (k) => console.log(`${k}:`, JSON.stringify(p[k] || p.dependencies?.[k] || p.devDependencies?.[k]));
for (const k of ['react','react-dom','posthog-js','`@posthog/react`','rut','`@mcpjam/react-router`','history']) print(k);
JS
} "$f"
doneRepository: MCPJam/inspector
Length of output: 50373
🌐 Web query:
PostHog JS posthog-js session recording route history change stopSessionRecording useEffect layout effect
💡 Result:
To manage PostHog session recording dynamically—such as stopping or starting recording on specific route changes—you can use the posthog.startSessionRecording() and posthog.stopSessionRecording() methods provided by the PostHog JavaScript SDK [1][2]. Manual Control on Route Change If you need to control recording based on routes, you can integrate these methods with your application's router or a navigation observer [1][3]. For React applications, you can use the usePostHog hook to access the PostHog instance within a component that tracks route changes [4][3]. An example implementation using a route listener might look like this: import { useEffect } from 'react'; import { usePostHog } from '@posthog/react'; // Example: Stopping recording on specific "sensitive" routes function useRecordingControl(currentRoute) { const posthog = usePostHog; const ignoredRoutes = ['/payment', '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/settings']; useEffect( => { if (ignoredRoutes.includes(currentRoute)) { posthog.stopSessionRecording; } else { posthog.startSessionRecording; } }, [currentRoute, posthog]); } Configuration Requirements For this manual control to work effectively, you must first disable the default automatic session recording behavior during initialization [1][5]. Set the disable_session_recording option to true in your posthog.init configuration [2][5]: posthog.init('<YOUR_PROJECT_TOKEN>', { api_host: '<YOUR_API_HOST>', disable_session_recording: true, }); Important Considerations 1. Full Snapshots on Navigation: If you are using a Single Page Application (SPA) and notice replay "drift" (where the UI in the recording does not match the actual user experience after a route change), PostHog offers an opt-in configuration: session_recording.full_snapshot_on_navigation: true [6]. This forces a fresh DOM snapshot on each pageview, ensuring the recording remains accurate [6]. 2. Performance vs. Fidelity: The full_snapshot_on_navigation option increases recording data usage because it sends a full DOM snapshot on every route change [6]. It is disabled by default for this reason [6]. 3. Layout Effects: While you mentioned useEffect and layout effect, note that standard useEffect is generally appropriate for triggering these PostHog methods [4][3]. If you experience UI layout "flickering" or glitches (often reported in cross-platform frameworks like React Native rather than web), ensure you are not causing unnecessary re-renders or state updates within the logic that controls the recording state [7]. If you are using a specific framework, always ensure you are using the provider pattern (e.g., PostHogProvider) as recommended in the official documentation to ensure the PostHog instance is available globally [8][9].
Citations:
- 1: https://posthog.com/docs/session-replay/how-to-control-which-sessions-you-record
- 2: https://posthog.com/docs/libraries/js/usage
- 3: https://posthog.com/docs/session-replay/how-to-control-which-sessions-you-record.md
- 4: https://posthog.com/tutorials/limit-session-recordings
- 5: https://docs.autoplay.ai/en/articles/1755301466-how-to-only-record-wanted-sessions
- 6: feat(replay): full snapshot on SPA navigation to prevent replay drift PostHog/posthog-js#3725
- 7: React Native Android layout glitches with session recording PostHog/posthog-js#3140
- 8: https://posthog.com/docs/session-replay/installation/nextjs
- 9: https://posthog.com/docs/session-replay/installation/react
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "React router history/navigation setup"
cat -n mcpjam-inspector/client/src/router.tsx | sed -n '1,220p'
echo
cat -n mcpjam-inspector/client/src/router-ref.ts | sed -n '1,140p'
echo
cat -n mcpjam-inspector/client/src/lib/app-navigation.ts | sed -n '1,220p'
echo
echo "PostHog init and session-recording test"
cat -n mcpjam-inspector/client/src/lib/__tests__/posthog-utils.test.ts | sed -n '140,250p'
echo
rg -n "initialize.*PostHog|posthog\.init|disable_session_recording|session_recording|full_snapshot_on_navigation" mcpjam-inspector/client/src mcpjam-inspector/client -g '!**/*.snap' -S --max-count 120
echo
echo "Package versions"
node - <<'JS'
const fs = require('fs');
const p = JSON.parse(fs.readFileSync('mcpjam-inspector/client/package.json', 'utf8'));
const all = {...(p.dependencies||{}), ...(p.devDependencies||{})};
for (const k of ['react','react-dom','posthog-js','`@posthog/react`','history','`@mcpjam/react-router`','rut']) {
if (all[k]) console.log(`${k}: ${all[k]}`);
}
JSRepository: MCPJam/inspector
Length of output: 30877
🌐 Web query:
React useEffect commit after paint useLayoutEffect before paint after mutation
💡 Result:
In React, the timing of useEffect and useLayoutEffect relative to the browser's paint process is a critical distinction for performance and UI consistency [1][2]. useLayoutEffect: This hook executes synchronously immediately after React performs all DOM mutations, but before the browser paints those changes to the screen [3][1]. Because it blocks the browser from painting, it is primarily intended for operations that require measuring the DOM or performing synchronous mutations to avoid visual flickers [3][1]. useEffect: By default, useEffect is deferred and runs after the browser has painted the updated screen [4][1][5]. This design prevents performance bottlenecks, ensuring that non-critical code does not block the browser's render pipeline [1][5]. Crucial Exceptions: It is a common misconception that useEffect is guaranteed to run after paint [6][7]. While this is the standard behavior, there are specific scenarios where React may run useEffect synchronously before paint [8][9]: 1. User Interactions: If an effect is triggered by a discrete user event (such as a click), or by an update wrapped in flushSync, React may execute the effect before paint to ensure the results are immediately observable by the event system [4][9]. 2. Cascading Updates: If a state update occurs within a useLayoutEffect, React must flush the remaining effects (including passive useEffects) before performing the next render [10][7]. Consequently, these effects will run synchronously before the browser paints [6][7]. Because of these behaviors, developers should not rely on useEffect to consistently fire after paint [7]. If an operation must strictly occur after paint, techniques such as using requestAnimationFrame or setTimeout inside the effect are often used to defer the execution further [4][9].
Citations:
- 1: https://kentcdodds.com/blog/useeffect-vs-uselayouteffect
- 2: https://dev.to/solo474/difference-between-useeffect-and-uselayouteffect-in-react-2aeg
- 3: https://react.dev/reference/react/useLayoutEffect
- 4: https://react.dev/reference/react/useEffect
- 5: https://react.dev/learn/synchronizing-with-effects
- 6: Add a note that useEffect is not guaranteed to run after paint reactjs/react.dev#4107
- 7: https://thoughtspile.github.io/2021/11/15/unintentional-layout-effect/
- 8: https://jser.dev/2023-08-09-effects-run-paint/
- 9: Issue:
useEffectexecuted before browser can paint DOM update react/react#20863 - 10: why setState in useLayoutEffect forces useEffects to run before paint? react/react#17334
🌐 Web query:
posthog-js session recording snapshot pathname history change useEffect useLayoutEffect
💡 Result:
In posthog-js, session recording and history change tracking are primarily handled by the library's internal logic rather than requiring custom implementation via useEffect or useLayoutEffect [1][2]. Key points for your configuration: 1. Automatic History/Pathname Changes: PostHog-js natively handles Single Page Application (SPA) navigation by listening to the browser's history API [1]. Setting capture_pageview: 'history_change' (often the default in newer versions) ensures that PostHog captures pageviews based on path changes [1]. 2. Session Recording Snapshots: By default, session replay tracks DOM changes [3]. If you encounter "replay drift" (where the recorded session state desyncs from the actual DOM) during SPA navigation, you can enable full_snapshot_on_navigation in your posthog.init configuration [4]. When enabled, this forces a fresh full snapshot on each $pageview event, providing a ground-truth state for the replay [4]. 3. Avoid useEffect/useLayoutEffect for Initialization: It is strongly recommended to initialize PostHog outside of React's useEffect lifecycle [5][6]. Initializing inside useEffect can lead to race conditions where methods like get_session_id or tracking hooks return undefined during the initial render [5][6]. Use the recommended provider patterns (such as the PostHogProvider from @posthog/react) to ensure the client is initialized eagerly [7][6]. 4. Manual Triggers: If you need to start session recording based on specific pathnames or user actions, use posthog.startSessionRecording directly [8][9]. Avoid using useEffect for this unless you are specifically guarding against initial page loads or timing-sensitive logic, as the SDK handles URL trigger conditions internally when configured [8][9]. In summary, avoid implementing custom pathname watchers in useEffect for PostHog. Rely on the SDK's built-in history listeners [1], and if you need higher fidelity snapshots on route changes, use the full_snapshot_on_navigation configuration option instead [4].
Citations:
- 1: https://posthog.com/docs/libraries/js/config.md
- 2: https://posthog.com/docs/references/posthog-js
- 3: feat: track recording URL without pageview capture PostHog/posthog-js#1076
- 4: feat(replay): full snapshot on SPA navigation to prevent replay drift PostHog/posthog-js#3725
- 5: Session id and distinc id are undefined PostHog/posthog-js#1925
- 6: feat(next): Add first class support for Next.js PostHog/posthog-js#3122
- 7: https://posthog.com/docs/session-replay/installation/nextjs
- 8: https://posthog.com/tutorials/limit-session-recordings
- 9: https://posthog.com/docs/session-replay/how-to-control-which-sessions-you-record.md
🌐 Web query:
rrweb snapshot current URL pathname while recording
💡 Result:
rrweb does not automatically record the URL pathname as a standalone event within the default event stream [1][2]. To capture the current URL or track navigation changes (pathname updates), the recommended approach is to use the addCustomEvent API [1][3]. To implement this, you can monitor URL changes in your application (for example, by listening to popstate events or hooking into your router) and emit a custom event whenever the path changes [3]: record.addCustomEvent('navigation', { pathname: window.location.pathname, url: window.location.href, timestamp: Date.now }); These custom events are saved alongside your other rrweb events [3]. During replay, you can access these events by attaching a listener to the replayer instance [3]: replayer.on('custom-event', (event) => { if (event.tag === 'navigation') { console.log('Navigated to:', event.payload.pathname); } }); If you are using the rrweb browser-client or specific plugins, some network-level recording plugins may capture request URLs [4], but this does not replace the need for manual custom event tracking for page-level navigation pathnames [3].
Citations:
- 1: https://rrweb.com/product/record-and-replay
- 2: https://github.com/rrweb-io/rrweb/blob/master/docs/recipes/dive-into-event.md
- 3: https://github.com/rrweb-io/rrweb/blob/master/docs/recipes/custom-event.md
- 4: https://rrweb.com/docs/recipes/network
Stop recording before navigation commits the results route.
useEffect runs after the route commit, so an active replay can already capture /results/<token> before syncSessionRecordingForPath(...) stops recording. Use useLayoutEffect or attach this guard directly to the history/route transition before the path changes, and add a hook-level route-transition test.
Proposed minimum change
-import { useEffect } from "react";
+import { useLayoutEffect } from "react";
...
- useEffect(() => {
+ useLayoutEffect(() => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!posthog) return; | |
| syncSessionRecordingForPath(posthog, location.pathname); | |
| }, [posthog, location.pathname]); | |
| useLayoutEffect(() => { | |
| if (!posthog) return; | |
| syncSessionRecordingForPath(posthog, location.pathname); | |
| }, [posthog, location.pathname]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.ts` around
lines 21 - 24, Replace the useEffect guard around syncSessionRecordingForPath
with useLayoutEffect, preserving the posthog and location.pathname dependencies
so recording stops before the results route is painted. Add a hook-level test
covering a route transition to /results/<token> and asserting the session
recording is stopped before the new path is committed.
Source: Learnings
There was a problem hiding this comment.
3 issues found across 28 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/server/utils/logger.ts">
<violation number="1" location="mcpjam-inspector/server/utils/logger.ts:26">
P3: The new `doNotTrack` guard that prevents Axiom initialization is untested in `logger.test.ts`. Add a case asserting `Axiom` is not constructed when `DO_NOT_TRACK` is set (matching the existing missing-token/dataset cases), consistent with how `sentry.test.ts` and `analytics.test.ts` cover the same opt-out and with the repo's all-changes-need-tests rule.</violation>
</file>
<file name="mcpjam-inspector/client/src/components/RouteErrorScreen.tsx">
<violation number="1" location="mcpjam-inspector/client/src/components/RouteErrorScreen.tsx:51">
P1: A crash on `/results/<token>` can still send the bearer token to Sentry: this only redacts the custom `extra.pathname`, while `reportCaught` continues to capture the exception and Sentry records the browser URL separately. The credential-path guard should also suppress this Sentry report or scrub the event's request URL in a global `beforeSend` hook.</violation>
</file>
<file name="mcpjam-inspector/server/sentry.ts">
<violation number="1" location="mcpjam-inspector/server/sentry.ts:72">
P2: Uncaught server crashes can lose the `process.uncaught_exception` Axiom event because the forced Sentry fatal path exits without flushing the asynchronously queued Axiom event. Preserve fail-fast shutdown while flushing `appLogger`/Axiom as part of the fatal-exit path.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Scrubbed: `/results/<token>` is a bearer-credential path, and a crash | ||
| // there would otherwise ship the token straight to Sentry/PostHog — | ||
| // the exact leak the rest of this PR closes elsewhere. | ||
| extra: { pathname: scrubSensitiveUrl(window.location.pathname) }, |
There was a problem hiding this comment.
P1: A crash on /results/<token> can still send the bearer token to Sentry: this only redacts the custom extra.pathname, while reportCaught continues to capture the exception and Sentry records the browser URL separately. The credential-path guard should also suppress this Sentry report or scrub the event's request URL in a global beforeSend hook.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/components/RouteErrorScreen.tsx, line 51:
<comment>A crash on `/results/<token>` can still send the bearer token to Sentry: this only redacts the custom `extra.pathname`, while `reportCaught` continues to capture the exception and Sentry records the browser URL separately. The credential-path guard should also suppress this Sentry report or scrub the event's request URL in a global `beforeSend` hook.</comment>
<file context>
@@ -39,7 +45,10 @@ export function RouteErrorScreen() {
+ // Scrubbed: `/results/<token>` is a bearer-credential path, and a crash
+ // there would otherwise ship the token straight to Sentry/PostHog —
+ // the exact leak the rest of this PR closes elsewhere.
+ extra: { pathname: scrubSensitiveUrl(window.location.pathname) },
});
}, [error]);
</file context>
| // an undefined state — silently turning a fail-fast crash into a zombie | ||
| // server that no supervisor restarts. | ||
| Sentry.onUncaughtExceptionIntegration({ | ||
| exitEvenIfOtherHandlersAreRegistered: true, |
There was a problem hiding this comment.
P2: Uncaught server crashes can lose the process.uncaught_exception Axiom event because the forced Sentry fatal path exits without flushing the asynchronously queued Axiom event. Preserve fail-fast shutdown while flushing appLogger/Axiom as part of the fatal-exit path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/sentry.ts, line 72:
<comment>Uncaught server crashes can lose the `process.uncaught_exception` Axiom event because the forced Sentry fatal path exits without flushing the asynchronously queued Axiom event. Preserve fail-fast shutdown while flushing `appLogger`/Axiom as part of the fatal-exit path.</comment>
<file context>
@@ -49,14 +49,29 @@ export function initServerSentry(): void {
+ // an undefined state — silently turning a fail-fast crash into a zombie
+ // server that no supervisor restarts.
+ Sentry.onUncaughtExceptionIntegration({
+ exitEvenIfOtherHandlersAreRegistered: true,
+ }),
+ ],
</file context>
| * process-lifetime setting, not something that flips at runtime. | ||
| */ | ||
| const doNotTrack = | ||
| process.env.DO_NOT_TRACK === "1" || process.env.DO_NOT_TRACK === "true"; |
There was a problem hiding this comment.
P3: The new doNotTrack guard that prevents Axiom initialization is untested in logger.test.ts. Add a case asserting Axiom is not constructed when DO_NOT_TRACK is set (matching the existing missing-token/dataset cases), consistent with how sentry.test.ts and analytics.test.ts cover the same opt-out and with the repo's all-changes-need-tests rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/utils/logger.ts, line 26:
<comment>The new `doNotTrack` guard that prevents Axiom initialization is untested in `logger.test.ts`. Add a case asserting `Axiom` is not constructed when `DO_NOT_TRACK` is set (matching the existing missing-token/dataset cases), consistent with how `sentry.test.ts` and `analytics.test.ts` cover the same opt-out and with the repo's all-changes-need-tests rule.</comment>
<file context>
@@ -13,8 +13,20 @@ const isVerbose = () => process.env.VERBOSE_LOGS === "true";
+ * process-lifetime setting, not something that flips at runtime.
+ */
+const doNotTrack =
+ process.env.DO_NOT_TRACK === "1" || process.env.DO_NOT_TRACK === "true";
+
const axiom =
</file context>
…ecorders Eight valid findings. Two are bugs I introduced in the previous round. **`DO_NOT_TRACK` was read at module load in logger.ts.** This is the exact bug class I had just fixed for Sentry, reintroduced one file over: `server/index.ts` statically imports the logger, so its module body runs BEFORE `loadInspectorEnv()`. A `DO_NOT_TRACK=1` coming from `.env` would have been invisible — Sentry and PostHog (which read it at call time) would go quiet while Axiom kept shipping logs, which is precisely the inconsistency this PR set out to remove. Now read at ingest time, with the Axiom client constructed lazily for the same reason. **The Electron sourcemaps were never actually injected.** `.vite/build` and `.vite/renderer` are produced by @electron-forge/plugin-vite DURING `electron:make` — they do not exist when `npm run build` finishes, so my workflow step's `[ -d "$dir" ]` guard silently skipped both. PR-E2's whole purpose was quietly not happening. Moved into a `packageAfterCopy` forge hook, the first point where the built bundles exist and are still editable before asar packing. The workflow now handles only `dist/client`, which IS built there and IS what the packaged app serves via the embedded server. Forge config hooks run BEFORE plugin hooks (verified in @electron-forge/core's `runHook`), so `prePackage` would have been the wrong window too. **The replay carve-out only covered one of the two recorders.** My own doc says "if you change the replay boundary, change it in both places or they drift" — and I drifted. Sentry Replay records DOM+text exactly like rrweb, so `/results/<token>` was still landing in Sentry replays. Both now sync on route change. **The runtime guard undid `VITE_DISABLE_POSTHOG_LOCAL`**, calling `startSessionRecording()` unconditionally on the first navigation and turning recording on in a build documented as having it off. Also: the doc still described the runtime guard as "tracked as follow-up" after it shipped; a failed sentry-cli left source maps in the installer (GH Actions runs `bash -e`, so the trailing delete never ran and `continue-on-error` proceeded to packaging — now an EXIT trap); the Convex trace regex's `[^/]*` allowed userinfo smuggling (`https://x.convex.cloud@evil.test/`); and `sanitizeStepError` only caught scheme-prefixed userinfo, missing bare `user:pass@host`, credential query params, `Bearer`/`Basic` values, and JSON credential fields. One pre-existing test asserted Axiom was constructed at import — that assertion encoded the bug, so it now asserts lazy construction. Verified: logger 22 pass, sanitizer 14 pass, shared config 14 pass, typecheck clean, forge.config.ts typechecks. Full suite running. Note: the forge hook only executes during a signed `electron:make` and cannot be exercised here. It is wrapped so a Sentry outage cannot fail a release and skips cleanly without SENTRY_AUTH_TOKEN, but the next desktop release is where it gets proven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_dd73687c-71bc-41b7-900c-f28f8687f12f) |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.ts`:
- Around line 28-38: Update the guard setup around apply and the early PostHog
availability check so syncSentryReplayForPath runs independently even when the
PostHog client is absent, while syncSessionRecordingForPath remains conditional
on PostHog. Add tests covering an absent PostHog client on both the initial
pathname and subsequent router navigation.
In `@mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts`:
- Around line 263-270: Update the credential redaction regex in the
debug-state-machine adapter to consume JSON string escapes, so escaped quotes
and other escaped characters remain within the redacted value. Add a regression
test covering a credential such as client_secret containing an escaped quote and
verify no original value fragment remains in the output.
- Around line 271-272: Update the error-body handling near the chained
sanitization replacements to cap the raw input at 500 characters before any
global replacements, preventing full-body processing. Ensure truncation cannot
expose a partial credential: detect and replace any secret cut by the bound
rather than returning its raw prefix, while preserving the existing sanitization
behavior for retained content.
In `@mcpjam-inspector/forge.config.ts`:
- Around line 202-205: Update the no-token branch in the Forge configuration’s
sourcemap handling so it resolves the configured targets and calls
deleteMapsIn() before returning, preventing loose maps from being packaged when
SENTRY_AUTH_TOKEN is absent. Add a regression test covering this no-token path,
and run the repository Vitest suite with npm run test.
- Around line 234-246: Bound both best-effort Sentry upload paths: in
mcpjam-inspector/forge.config.ts lines 234-246, add finite timeout options to
the Node execFileSync calls for Sentry inject/upload while preserving existing
cleanup behavior; in .github/workflows/windows-release.yml lines 102-107, apply
a timeout to the GitHub bash step running the Sentry upload. Use the existing
release/upload flow and do not alter unrelated steps.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82daa82e-42ae-41b4-a8ba-def77ee62b95
📒 Files selected for processing (13)
.github/workflows/mac-release.yml.github/workflows/windows-release.ymlmcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.tsmcpjam-inspector/client/src/lib/PosthogUtils.tsmcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.tsmcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.tsmcpjam-inspector/client/src/lib/sentry.tsmcpjam-inspector/docs/session-replay-masking.mdmcpjam-inspector/forge.config.tsmcpjam-inspector/server/utils/__tests__/logger.test.tsmcpjam-inspector/server/utils/logger.tsmcpjam-inspector/shared/__tests__/sentry-config.test.tsmcpjam-inspector/shared/sentry-config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- mcpjam-inspector/client/src/lib/PosthogUtils.ts
- .github/workflows/mac-release.yml
- mcpjam-inspector/shared/tests/sentry-config.test.ts
- mcpjam-inspector/docs/session-replay-masking.md
- mcpjam-inspector/shared/sentry-config.ts
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/forge.config.ts">
<violation number="1" location="mcpjam-inspector/forge.config.ts:202">
P1: The new `packageAfterCopy` hook gates on `process.env.SENTRY_AUTH_TOKEN`, but the electron:make steps that trigger it never make that secret available to the hook. In the paired workflows, `SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }}` is only set on the `dist/client` sourcemap step — the "Sign and package application" (mac) and "Make Windows installer" steps set only `NODE_OPTIONS`/Apple signing vars and `WINDOWS_PFX_*`. GitHub Actions secrets are injected only into steps that reference them, so during `electron:make` the envy is empty and the hook always hits the early return (`console.warn("[forge] SENTRY_AUTH_TOKEN unset; skipping sourcemaps")`). Net effect: the `.vite/build` and `.vite/renderer` desktop maps this PR is meant to symbolicate are never injected or uploaded from CI — the desktop crash telemetry/pr-E1-E2 feature silently no-ops on every release.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * Never throws. A Sentry outage must not fail a signed release. | ||
| */ | ||
| packageAfterCopy: async (_forgeConfig, buildPath) => { | ||
| if (!process.env.SENTRY_AUTH_TOKEN) { |
There was a problem hiding this comment.
P1: The new packageAfterCopy hook gates on process.env.SENTRY_AUTH_TOKEN, but the electron:make steps that trigger it never make that secret available to the hook. In the paired workflows, SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} is only set on the dist/client sourcemap step — the "Sign and package application" (mac) and "Make Windows installer" steps set only NODE_OPTIONS/Apple signing vars and WINDOWS_PFX_*. GitHub Actions secrets are injected only into steps that reference them, so during electron:make the envy is empty and the hook always hits the early return (console.warn("[forge] SENTRY_AUTH_TOKEN unset; skipping sourcemaps")). Net effect: the .vite/build and .vite/renderer desktop maps this PR is meant to symbolicate are never injected or uploaded from CI — the desktop crash telemetry/pr-E1-E2 feature silently no-ops on every release.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/forge.config.ts, line 202:
<comment>The new `packageAfterCopy` hook gates on `process.env.SENTRY_AUTH_TOKEN`, but the electron:make steps that trigger it never make that secret available to the hook. In the paired workflows, `SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }}` is only set on the `dist/client` sourcemap step — the "Sign and package application" (mac) and "Make Windows installer" steps set only `NODE_OPTIONS`/Apple signing vars and `WINDOWS_PFX_*`. GitHub Actions secrets are injected only into steps that reference them, so during `electron:make` the envy is empty and the hook always hits the early return (`console.warn("[forge] SENTRY_AUTH_TOKEN unset; skipping sourcemaps")`). Net effect: the `.vite/build` and `.vite/renderer` desktop maps this PR is meant to symbolicate are never injected or uploaded from CI — the desktop crash telemetry/pr-E1-E2 feature silently no-ops on every release.</comment>
<file context>
@@ -179,6 +179,105 @@ const config: ForgeConfig = {
+ * Never throws. A Sentry outage must not fail a signed release.
+ */
+ packageAfterCopy: async (_forgeConfig, buildPath) => {
+ if (!process.env.SENTRY_AUTH_TOKEN) {
+ console.warn("[forge] SENTRY_AUTH_TOKEN unset; skipping sourcemaps");
+ return;
</file context>
…cycle CI: posthog-utils "stops recording when navigating INTO /results/" modelled a build that does not exist under test. `.env.local` ships VITE_DISABLE_POSTHOG_LOCAL=true and vite loads it under vitest, so the round-3 opt-out guard correctly refused to resume — the test needed the stub, not the guard a fix. Desktop sourcemaps were a no-op on every release. Actions injects a secret only into steps that name it, and SENTRY_AUTH_TOKEN was set only on the dist/client step — so during electron:make the forge `packageAfterCopy` hook always hit its early return. Both packaging steps now carry the token. Alongside: - the map cleanup no longer sits behind the token check. A tokenless build still emits maps, and returning early packed them into the asar — the exact leak the hook exists to prevent. - `npx` is `npx.cmd` on Windows and spawning a .cmd with shell:false throws EINVAL since Node's CVE-2024-27980 hardening. The catch swallowed it, so Windows symbolication would have stayed broken even with the token. Now shell-on-Windows with hand-quoted args, plus a 5-minute timeout so a hung sentry-cli cannot stall a signed release. Replay lifecycle, both recorders: - a session that LOADS on /results/<token> now constructs no recorder at all. Stopping post-mount was never enough: `replay.stop()` flushes the buffered segment, which is the token-bearing page itself. - on exit, each recorder resumes only if the guard is what stopped it. `replay.start()` bypasses replaysSessionSampleRate, so resuming unconditionally recorded 100% of sessions that ever touched a results link. - the Sentry half of the guard no longer sits behind a PostHog client. PostHog is routinely ad-blocked; Sentry Replay is gated on the platform. OAuth error sanitizer: - JSON values now consume escapes (`(?:\\.|[^"\\])*`), so an escaped quote no longer ends the match early and leaks the secret's tail. - "Bearer token is expired" keeps its diagnostic word. The bare-scheme redactor only fires on credential-shaped values now; an echoed `Authorization:` header still redacts whatever follows. - the input is bounded before the replace chain rather than after, with a tail guard for the two forms that need a closing delimiter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5396740c-1560-487f-abb6-ffa3ba2d5b94) |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@mcpjam-inspector/client/src/hooks/__tests__/useSessionRecordingPathGuard.test.ts`:
- Around line 61-79: Extend the navigation test around
useSessionRecordingPathGuard to cover an available PostHog client by mocking
usePostHog to return posthogClient, triggering the subscribed router callback
with "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/results/another-token", and asserting syncSessionRecordingForPath
receives that path. Preserve the existing unavailable-client assertions as a
separate case.
In `@mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts`:
- Around line 268-270: Update both redaction patterns in the OAuth
debug-state-machine adapter to consume userinfo through the final authority `@`
before `/`, `?`, or `#`, so embedded `@` characters in passwords are fully
redacted. Add a regression test covering a URL such as
`https://user:secret@part@example.test` and verify no password portion remains
exposed.
- Around line 308-311: Update the truncation guard regex in the
debug-state-machine adapter to use the same escape-aware JSON string matching
strategy as the redactor near line 295, including support for a trailing
backslash at the scan boundary. Add a regression test covering a long redacted
prefix followed by a truncated client_secret containing an escaped quote, and
verify the credential remains redacted within the first 500 reported characters.
In `@mcpjam-inspector/client/src/lib/sentry.ts`:
- Around line 74-79: Update the credential-bearing branch in the pathname guard
so repeated protected-path calls preserve sentryReplayStoppedByGuard after
replay.stop() clears getReplayId(); only overwrite the flag when this invocation
observes an active replay, and retain it until the safe-path transition triggers
replay.start(). Add or update Sentry replay tests covering repeated
/results/<token> synchronization followed by leaving the protected path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ea41549-6c29-476e-abe4-d77db1d8a1ee
📒 Files selected for processing (13)
.github/workflows/mac-release.yml.github/workflows/windows-release.ymlmcpjam-inspector/client/src/hooks/__tests__/useSessionRecordingPathGuard.test.tsmcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.tsmcpjam-inspector/client/src/lib/PosthogUtils.tsmcpjam-inspector/client/src/lib/__tests__/posthog-utils.test.tsmcpjam-inspector/client/src/lib/__tests__/sentry.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.tsmcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.tsmcpjam-inspector/client/src/lib/sentry.tsmcpjam-inspector/docs/session-replay-masking.mdmcpjam-inspector/forge.config.tsmcpjam-inspector/server/utils/__tests__/logger.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- mcpjam-inspector/client/src/lib/tests/posthog-utils.test.ts
- .github/workflows/mac-release.yml
- mcpjam-inspector/docs/session-replay-masking.md
- .github/workflows/windows-release.yml
- mcpjam-inspector/server/utils/tests/logger.test.ts
- mcpjam-inspector/forge.config.ts
- mcpjam-inspector/client/src/lib/PosthogUtils.ts
- mcpjam-inspector/client/src/hooks/useSessionRecordingPathGuard.ts
cubic auto-marked this "addressed in b16e460" because the lines moved, but b16e460 swapped isErrorCaptureSurface() for shouldRecordSession() — a credential-path check, not a packaged one. The finding stands. src/preload.ts:135 exposes `isElectron: true` unconditionally, and the client Sentry config defaults `enabled` to true, so every `electron-forge start` was loading Sentry Replay and PostHog's recorder against the production DSN and project key. The documented boundary is *packaged* desktop, not "anything with a preload attached". `import.meta.env.PROD` is the packaged signal for the renderer — dev is served by the vite dev server, packaged is a `vite build` output — and this file already trusts it for `environment`. HOSTED_MODE needs no equivalent: it comes from VITE_MCPJAM_HOSTED_MODE, which neither .env.local nor .env.production sets, only the deploy config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
…uard The Sentry one is a state bug I introduced in b16e460. `stop()` clears the replay id, so re-reading it on a second credential path (/results/a → /results/b) disarmed the resume and the eventual exit never restarted the recording. The flag is now only ever armed here, never cleared. Sanitizer: - `@` is legal inside a URL password, so `https://user:secret@part@host` has userinfo `user:secret@part`. Both userinfo patterns stopped at the first `@` and reported the rest of the password. They now run to the last `@` of the authority. - the truncation tail guard was not escape-aware, so an unterminated JSON value containing `\"` matched neither redactor. Since a long redactable prefix shrinks, that exposed suffix could land inside the reported 500 characters. Tests: the navigation case now runs with PostHog present and absent, rather than claiming both in its title and asserting only one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
GitHub Actions
Error
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_64046ccd-4fba-4b77-ade2-d3764048fb04) |
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The PostHog half of the credential-path guard armed its resume from `!isPostHogDisabled`. That flag says whether the build permits recording, not whether this session was being recorded — PostHog's own project-side sampling can decline one — so leaving /results/ could start a recording sampling never selected. It now uses `sessionRecordingStarted()`, the same shape as the Sentry half's `getReplayId()`, and carries the same never-disarm-on-the-way-in rule so /results/a to /results/b keeps the resume armed. `stop()` stays unconditional in both halves: it is idempotent, and an SDK build that does not expose the liveness probe must still stop on a credential path. Also: the "client is gone" Sentry test asserted nothing. It ran with HOSTED_MODE off, so the function returned at its first guard and never reached getClient() — the not.toThrow() passed regardless. Split out and moved onto the hosted surface, with cases for a missing client, a missing Replay integration, and a lookup that throws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_473e050c-5f2c-41fe-afa5-0c9a439ae908) |
Four conflicts, all against two PRs that landed on main while this branch was open: #3792 (Electron main Sentry environment) and #3795 (settings routes moved onto SettingsPageShell). shared/sentry-config.ts, src/main.ts, shared/__tests__/sentry-config.test.ts Kept this branch's version. #3792 added `electronMainSentryConfig(isPackaged)` on top of the old `baseSentryConfig` object, which PR-1 replaces wholesale with the `buildSentryConfig(ctx)` factory. The fix itself survives — main.ts already passes `environment: app.isPackaged ? "prod" : "dev"` (plus release and the deployment tag) into `buildElectronSentryConfig`. #3792's unit tests could not be carried over because the function they cover no longer exists; the mapping now lives inline at the call site. client/src/components/settings/IntegrationsRoute.tsx Genuine merge of both sides: main's SettingsPageShell wrapper with this branch's named error boundaries and ErrorCard fallbacks. Auto-merge had emitted GithubChecksCard twice, once per side; dropped the duplicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_65da2a0e-ccaa-4a28-a450-ef1f000f2a1b) |
|
E2E Smoke is red here, and it's failing on
Evidence it predates the merge:
Nothing in this PR touches the sidebar; Everything else on Generated by Claude Code |
Two conflicts, both additive-vs-additive rather than a real disagreement. client/src/lib/sentry.ts This branch appends `syncSentryReplayForPath` at the end of the file and #3791 appends `captureSentryException` at the same spot. Kept both. client/src/main.tsx Import line only. `captureSentryException` alongside this branch's `reportCaught` and `ErrorBoundary`. Note: the merged tree now carries two client error-reporting wrappers. #3791's `captureSentryException` is a Sentry-only passthrough; this branch's `reportCaught` also reports to PostHog, gates on the error-capture surface, normalizes non-Errors, and never throws. They were written in parallel. Left as-is here — collapsing them would change #3791's behavior, which is not a conflict resolution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub Actions
Error
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_26014c62-61b1-404f-a7e9-60029808ac47) |
This branch was successfully deployed
2 participants
Executes the code half of Phase 0 + Phase 1 from the Aug 7 quality audit. The audit's finding was that the error/quality loop is unplugged end to end: server-side Sentry is dead code, PostHog exception capture is off, panels vanish via
fallback={null}, and desktop crashes arrive minified (or not at all).Six commits, each independently green. Land order matches the plan: PR-1 → PR-3 → PR-4 → PR-2 → PR-5, with E1/E2 parallel.
What each commit does
PR-1 — shared Sentry config factory. Replaces three hand-rolled config objects with a pure
buildSentryConfig(ctx)plus per-surface builders. No env reads inside the module; each surface resolves its own environment/release/deployment from the API that is truthful there. Fixes two mislabels: the packaged desktop app never setsNODE_ENV, so every shipped Electron event reportedenvironment: "dev"(nowapp.isPackaged), and the browser bundle has noprocess(nowimport.meta.env.PROD). Adds adeploymenttag (hosted | self_hosted) to every event.PR-3 — boundary reporting, route errorElement, fallback burn-down.
ErrorBoundary.componentDidCatchonly logged to console, so all ~21 mounts swallowed their errors. Reporting moved into the boundary itself — every mount reports with zero call-site edits. react-router's data router catches route errors before any ancestor boundary can see them, so the root route gains anerrorElement; a root<ErrorBoundary>outsidePostHogProvidercovers provider crashes. The five user-visiblefallback={null}sites now render a retryableErrorCardor a muted one-liner. Catch burn-down wave 1 (registry quick-connect, wire-mode reconnect, both App Builder tool paths, chat token fetch).PR-4 — PostHog exception capture, replay gating, masking, dead clicks.
capture_exceptions+ session replay on hosted and packaged desktop only; npx/Docker installs run on someone else's machine./results/<token>is never recorded (the token in that URL is the credential). Masking reuses the repo's existingdata-ph-no-captureconvention as themaskTextSelectorrather than adding thedata-ph-maskattribute the plan called for — three overlapping mechanisms now key off one annotation instead of two.PR-2 — the server actually reports errors now.
server/sentry.tscalledSentry.initas an import side effect, and ES imports hoist aboveserver/index.ts'sloadInspectorEnv()— so it initialized against an empty environment on every start. That is whyinspector-serverhas been near-empty. Now an explicit call after the env load. The logger owns all server capture;logger.warnstops capturing (the largest quota-spike vector across self-hosted installs). Guardrails:DO_NOT_TRACK,SENTRY_ERROR_SAMPLE_RATE,tracesSampleRate: 0, and the defaultOnUnhandledRejectionintegration filtered out so it can't capture the MCP SDK's routine "Connection closed" rejections.PR-5 — OAuth Continue stops looking inert. The worst rageclick hotspot on the surface (211 rageclicks / 67 people):
handleAdvanceawaited a round trip while the button kept its idle label and stayed enabled. Now "Continuing..." + disabled +aria-busy, matching the sibling Connect/Refresh buttons. Three silent failure paths fixed. Step failures report by wrappingupdateState— the only reliable hook, since the SDK machine catches its own errors and never rethrows.PR-E1/E2 — desktop crashes. Checked
@sentry/electron5.12's defaults first, as planned, and two findings changed the shape:sentryMinidumpIntegrationis already default-on (nothing to enable), andchildProcessIntegrationalready registers the process-gone events but its defaulteventslist omitscrashed,oom, andkilled— exactly the reasons a user experiences as "the app broke". Those are breadcrumbs only today. Reconfigured rather than duplicated, so nothing double-reports. The renderer build had nobuildblock at all, so it emitted no source maps; both Electron configs now do, and the mac/windows workflows upload them.Two deliberate deviations from the plan
Both are called out in the relevant commit messages:
data-ph-mask. The repo already hadph-no-capture/rr-block/data-ph-no-captureon its secret surfaces, so a second attribute would mean two things to remember and one of them silently missed.maskTextSelectorpoints at the existing one.Bugs found while writing tests
"", which??does not catch, andNumber("")is0— an emptySENTRY_ERROR_SAMPLE_RATEwould have silently dropped 100% of error events.definecannot see through dynamic lookups. The release read has to stay a literalprocess.env.Xmember expression; esbuild'sdefineis a syntactic substitution. Caught by checking the built bundle, not the test.Verification
npm run typecheck:clientclean;npm run build:serverclean.BAKED_VERSION = "2.34.0").DO_NOT_TRACK=1 node dist/server/index.jslogs the disabled init.Not done here (needs dashboard access or a deploy)
/relay/*is 403'd at the edge, which is why session replay has never worked (the recorder can never load). This is Marcelo's; it gates PR-4's replay actually recording anything, and it gates the PMF survey launch (same path)./mcpauth in-session; the PostHog error-tracking alert needshog_function:write.inspector-server. Rollout risk plan and rollback knobs indocs/server-error-reporting.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01F3WDdBFT5Q41AKUW9JiTxS
Generated by Claude Code
Summary by cubic
Unifies and fixes error reporting across web, server, and desktop; gates PostHog and Sentry Replay to hosted and packaged desktop only (not
electron-forge start), adds a guarded route error UI, symbols desktop crashes, and hardens OAuth scrubbing and replay lifecycle.New Features
buildSentryConfig+ per-surface) with correctenvironment, explicitrelease, and adeploymenttag.initServerSentryafter env load; logger owns Sentry capture; honorsDO_NOT_TRACKandSENTRY_ERROR_SAMPLE_RATE; spans off; fail‑fast viaonUncaughtExceptionIntegration({ exitEvenIfOtherHandlersAreRegistered: true }).errorElement(RouteErrorScreen), named boundaries report viareportCaughtto@sentry/react+posthog-js.capture_exceptions+ session replay only on hosted and packaged desktop; excluded onelectron-forge start;/results/<token>excluded at init and via a runtime path guard; masking viadata-ph-no-capture;capture_dead_clickson everywhere.@sentry/electronchild‑process integration promotescrashed/oom/killed; renderer/main builds emit source maps; forge hook injects debug IDs and uploads; release workflows handle web maps.aria-busy; step failures report via the SDK adapter with credential scrubbing.Bug Fixes
/results/<token>; both recorders stop on entry and resume only if the guard stopped them; PostHog resume usessessionRecordingStarted(); Sentry Replay gated by the same surface check; route guard works outside a router and keepsVITE_DISABLE_POSTHOG_LOCALrespected..vite/buildand.vite/renderer;SENTRY_AUTH_TOKENplumbed to packaging steps; web maps cleaned even without a token; Windows uses shell with quoting and a 5‑minute timeout; failures don’t block a signed release.DO_NOT_TRACKdisables Axiom and is read lazily at ingest; fixed Sentry release naming in theviteplugin; tighter Convex trace‑propagation regex; removed browser‑only ignores from Electron main; warning‑level reporting for non‑fatal chat token fetch; visible boundary fallbacks replacefallback={null}; quick‑connect and reconnect paths report and toast.@; escaped JSON handled; tail guard is escape‑aware; keeps “expired” diagnostics while redacting credentials; stronger redaction of headers, query params, bearer/basic values, and JSON fields.buildSentryConfigfactory andSettingsPageShellintegration; kept bothcaptureSentryExceptionandreportCaughttemporarily to preserve behavior.Written for commit 2837dfc. Summary will update on new commits.
Note
High Risk
First live Sentry reporting from OSS server installs and broad client/desktop telemetry changes affect privacy, quota, and crash behavior; credential-path replay gating and scrubbing reduce but do not eliminate leak risk if misconfigured.
Overview
Plugs the quality loop by making errors and replays actually observable on the surfaces that should report, while keeping self-hosted installs and bearer-credential routes out of session recording.
Shared Sentry moves to pure
buildSentryConfigbuilders with per-surface DSNs,deploymenttags, correct browser/Electronenvironment/release, and optional replay sampling. Server callsinitServerSentry()after env load (replacing hoisted dead init), routesHono.onErrorthroughlogger.erroras the single capture path, stopslogger.warnfrom creating Sentry issues, honorsDO_NOT_TRACK/SENTRY_ERROR_SAMPLE_RATE, and forces fatal exit when otheruncaughtExceptionlisteners exist. Client addsreportCaught/reportBoundaryError, rooterrorElement(RouteErrorScreen), named boundary reporting, and replaces silentfallback={null}billing/integration panels with retryable or muted fallbacks.PostHog + replay enable
capture_exceptionsand session replay only on hosted and packaged desktop (import.meta.env.PRODexcludeselectron-forge start); npx/Docker stays off./results/<token>never starts a recorder at init, anduseSessionRecordingPathGuardstops/resumes both PostHog and Sentry Replay on in-app navigation. Masking reusesdata-ph-no-capturefor text secrets; OAuth token UI getssensitive/ph-no-capturetreatment.OAuth debugger shows “Continuing…” with disabled/
aria-busyduring advances, toasts new flow errors once, reports escaped failures, and wraps the SDKupdateStatewithsanitizeStepErrorbefore telemetry. Desktop releases upload/inject sourcemaps fordist/clientin CI and.vite/*inforge.config.tspackageAfterCopy, always deleting.mapfiles from installers.Docs add
server-error-reporting.md,session-replay-masking.md, andquality-triage.md.Reviewed by Cursor Bugbot for commit 2837dfc. Bugbot is set up for automated code reviews on this repo. Configure here.