Skip to content

docs(desktop): correct Bug F diagnosis — renderer SDK was not broken#645

Merged
jeevanpillay merged 2 commits intomainfrom
docs/sentry-bridge-correction
May 6, 2026
Merged

docs(desktop): correct Bug F diagnosis — renderer SDK was not broken#645
jeevanpillay merged 2 commits intomainfrom
docs/sentry-bridge-correction

Conversation

@jeevanpillay
Copy link
Copy Markdown
Member

@jeevanpillay jeevanpillay commented May 6, 2026

Summary

Follow-up experiment on `@sentry/electron@7.13.0` (latest) with the renderer SDK restored shows `Sentry.getClient()` returns a fully constructed client and the v10 carrier exposes it at `SENTRY[ver].defaultCurrentScope._client` (not the v8/v9 `.client` / `.defaultClient` paths I was inspecting during the dry-run). The "Sentry.init silently fails to register a client" diagnosis from PR #643 / the dry-run report was wrong.

What actually caused "zero events ingested" on rc.1/rc.2/rc.3 was Sentry org quota exhaustion (flagged by you near the end of the dry-run) — not a broken SDK.

PR #643's main-side bridge still stands as the chosen architecture (smaller bundle, single SDK init, insulated from carrier shape changes) — but as an architectural choice, not as an SDK workaround. No upstream issue filed.

What this PR changes

  • `thoughts/shared/2026-05-06-desktop-rc1-ad-hoc-dry-run-report.md` — TL;DR Correction callout + new §Correction section near the end with experiment evidence, why the misdiagnosis happened, why the bridge still stands.
  • `thoughts/shared/research/2026-04-23-codex-vs-lightfast-desktop-production-gap.md` — G-12 row amended with strikethrough on the wrong claim and pointer to the report's §Correction.
  • `apps/desktop/src/renderer/src/main.ts` — replaces the "renderer SDK was broken" comment with an honest architectural rationale and notes the trade-offs (no automatic breadcrumbs / replay until renderer SDK is restored).

Test plan

  • Typecheck passes (`pnpm --filter @lightfast/desktop typecheck`)
  • Report renders cleanly in GitHub markdown
  • G-12 strikethrough renders correctly
  • Code reviewer confirms the renderer comment makes the trade-off clear

Follow-ups (separate PRs, not blocking)

  • Bump `@sentry/electron` from 7.11.0 → 7.13.0 (two minor versions behind; small win, no behavior change for our usage)
  • Revisit the bridge architecture if/when we want session replay or rich renderer breadcrumbs in v0.2.0+ — recipe documented in the report's §Correction

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Updated internal comments and documentation around error-reporting and how renderer errors are forwarded to the main process.
    • Adjusted internal error-routing notes to reflect a single main-process reporting path and an optional renderer opt-in.

Note: No user-facing functionality or public APIs were changed.

Follow-up experiment on @sentry/electron 7.13.0 with the renderer SDK
restored shows Sentry.getClient() returns a fully constructed client
and the v10 carrier exposes it at __SENTRY__[ver].defaultCurrentScope._client
(not the v8/v9 .client / .defaultClient paths I was inspecting during
the dry-run). The "Sentry.init silently fails" diagnosis was wrong;
the "zero events" outcome on rc.1/rc.2/rc.3 was Sentry org quota
exhaustion, not a broken SDK.

PR #643's main-side bridge still stands as the chosen architecture —
smaller bundle, single SDK init, insulated from carrier shape changes —
but as an architectural choice rather than an SDK workaround.

Updates the dry-run report with a §Correction and a TL;DR callout,
amends the codex-gap doc's G-12 row, and rewrites the misleading
comment in apps/desktop/src/renderer/src/main.ts.

No upstream issue filed — the SDK works.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 6, 2026

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

3 Skipped Deployments
Project Deployment Actions Updated (UTC)
lightfast-app Ignored Ignored Preview May 6, 2026 8:55am
lightfast-platform Ignored Ignored Preview May 6, 2026 8:55am
lightfast-www Ignored Ignored Preview May 6, 2026 8:55am

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Replaces direct Sentry main SDK usage with a dedicated captureException import in the main process and updates renderer comment guidance to document a single main-process Sentry init with optional renderer opt‑in. No public APIs or exported signatures changed.

Changes

Sentry renderer→main routing

Layer / File(s) Summary
Imports / API surface
apps/desktop/src/main/index.ts
Replaces import * as Sentry from "@sentry/electron/main" with import { captureException } from "@vendor/observability/sentry-electron-main".
Error forwarding core
apps/desktop/src/main/index.ts
forwardRendererErrorToSentry now calls captureException(error, { tags, extra }) (previously Sentry.captureException), preserving Error construction, stack, tags, and extra payload.
Docs / Comments
apps/desktop/src/renderer/src/main.ts
Replaces renderer-centric Sentry notes with main-process-centric guidance: single SDK init via @vendor/observability/sentry-electron-main and optional renderer opt‑in via @vendor/observability/sentry-electron-renderer.

Sequence Diagram

sequenceDiagram
    participant Renderer as Renderer (web)
    participant IPC as IPC
    participant Main as Main process
    participant Obs as Observability (`sentry-electron-main`)

    Renderer->>IPC: send error payload (message, stack, metadata)
    IPC->>Main: forward renderer error
    Main->>Obs: captureException(Error, { tags, extra })
    Obs->>Obs: enrich & send to Sentry backend
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Review notes (concise, actionable)

  • [nit] apps/desktop/src/main/index.ts: confirm the main-process SDK is initialized once (single source) so captureException has configured transport/context. Explain WHY: otherwise captured events may lack DSN/config.
  • [nit] apps/desktop/src/renderer/src/main.ts: keep comment synced with onboarding docs or link to a canonical README to avoid divergence. Explain WHY: comments are the primary guidance for opt‑in renderer usage.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title follows conventional commits format (docs:) and is under 70 characters (68 chars). Content accurately reflects the PR's main objective: correcting the misdiagnosis about renderer SDK status.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/sentry-bridge-correction
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch docs/sentry-bridge-correction

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@apps/desktop/src/renderer/src/main.ts`:
- Around line 21-25: Update the inline guidance in main.ts to avoid recommending
direct third-party imports: replace the reference to `@sentry/electron/renderer`
and `Sentry.init` with the repo's vendor abstraction (e.g., `@vendor/sentry` or
`@vendor/*` re-exports). Keep the same behavioral note about renderer-side
breadcrumbs/replay but instruct developers to import the renderer SDK via the
vendor abstraction (e.g., "add `@vendor/sentry` + `Sentry.init`") rather than
naming `@sentry/electron/renderer` directly so the comment aligns with the
vendor-abstraction policy.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 127df7fa-fb94-475d-a0f6-4524a2847032

📥 Commits

Reviewing files that changed from the base of the PR and between d4f0cb4 and 381bec3.

⛔ Files ignored due to path filters (2)
  • thoughts/shared/2026-05-06-desktop-rc1-ad-hoc-dry-run-report.md is excluded by !thoughts/**
  • thoughts/shared/research/2026-04-23-codex-vs-lightfast-desktop-production-gap.md is excluded by !thoughts/**
📒 Files selected for processing (1)
  • apps/desktop/src/renderer/src/main.ts

Comment thread apps/desktop/src/renderer/src/main.ts Outdated
CodeRabbit flagged the renderer comment recommending `@sentry/electron/renderer`
as a direct third-party SDK reference, which violates repo policy. Same policy
violation in apps/desktop/src/main/index.ts where PR #643 added a direct
`import * as Sentry from "@sentry/electron/main"` instead of routing through
the existing `@vendor/observability/sentry-electron-main` re-export
(which already exposes captureException).

- Switch main/index.ts to import { captureException } from the vendor wrap.
- Update the renderer comment to point at a hypothetical
  `@vendor/observability/sentry-electron-renderer` re-export rather than
  the underlying `@sentry/electron/renderer` SDK.
- Mirror the same guidance in the dry-run report's §Correction follow-ups.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jeevanpillay jeevanpillay merged commit fb43582 into main May 6, 2026
14 of 15 checks passed
@jeevanpillay jeevanpillay deleted the docs/sentry-bridge-correction branch May 6, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant