feat: Sentry production readiness — source maps, crash reporter, IPC wrapper#1869
feat: Sentry production readiness — source maps, crash reporter, IPC wrapper#1869
Conversation
…pper for Sentry production readiness - Add @sentry/vite-plugin to upload source maps during CI builds so production stack traces are readable in Sentry (maps are stripped from the bundle after upload) - Configure Electron's native crashReporter for catching GPU process crashes, native module segfaults, and V8 snapshot failures via minidump ingestion - Add withSentryIpc() wrapper utility for ipcMain.handle callbacks with automatic error capture and channel tagging - Pass SENTRY_AUTH_TOKEN, SENTRY_ORG, SENTRY_PROJECT secrets to all build steps in release and beta-release workflows - Add unit tests for withSentryIpc Co-Authored-By: Claude Opus 4.6 <[email protected]>
github-actions
bot
added
area/frontend
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's error monitoring capabilities by implementing a comprehensive Sentry integration strategy. It addresses critical gaps in production readiness by ensuring that source maps are available for debugging, native crashes are reported, and inter-process communication errors are automatically captured, thereby improving the reliability and maintainability of the application. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds Sentry credentials to CI workflows, integrates Sentry into the Electron frontend build (sourcemaps and Vite plugin), initializes native crash reporter, and provides a withSentryIpc wrapper plus tests for IPC error capture and tagging. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (invoke)
participant IPC as Wrapped IPC Handler
participant Handler as Original Handler
participant Sentry as Sentry SDK
Renderer->>IPC: invoke(channel, args)
IPC->>Handler: call(event, args)
alt success
Handler-->>IPC: result
IPC-->>Renderer: result
else error
Handler-->>IPC: throws Error
IPC->>Sentry: captureException(Error, {ipc_channel: channel})
Sentry-->>Sentry: record event
IPC-->>Renderer: re-throws Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves Sentry integration by adding source map uploads, native crash reporting, and a convenient IPC wrapper for error handling. The changes are well-structured and the addition of unit tests for the new IPC wrapper is great. I have a few suggestions to improve type safety and remove potentially redundant code.
| // Start Electron's native crash reporter for catching GPU process crashes, | ||
| // native module segfaults (node-pty, etc.), and V8 snapshot failures. | ||
| // Sentry's JS-level handlers can't catch these — they require minidump ingestion. | ||
| if (shouldEnable) { | ||
| try { | ||
| // Extract the public key and project ID from the DSN | ||
| // DSN format: https://<public_key>@<host>/<project_id> | ||
| const dsnUrl = new URL(cachedDsn); | ||
| const publicKey = dsnUrl.username; | ||
| const projectId = dsnUrl.pathname.slice(1); | ||
| const sentryHost = dsnUrl.hostname; | ||
|
|
||
| if (publicKey && projectId) { | ||
| crashReporter.start({ | ||
| submitURL: `https://${sentryHost}/api/${projectId}/minidump/?sentry_key=${publicKey}`, | ||
| uploadToServer: true, | ||
| compress: true, | ||
| }); | ||
| console.log('[Sentry] Native crash reporter started (minidump ingestion)'); | ||
| } | ||
| } catch (error) { | ||
| console.warn('[Sentry] Failed to start native crash reporter:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The @sentry/electron SDK automatically configures and starts Electron's native crashReporter when Sentry.init() is called in the main process. This manual setup of crashReporter.start() is likely redundant and could potentially conflict with the SDK's internal initialization.
According to the Sentry documentation for the Electron SDK, you do not need to call crashReporter.start() yourself. The SDK handles minidump ingestion automatically.
I recommend removing this entire block to rely on the SDK's built-in functionality. If there was a specific reason for this manual setup (e.g., a workaround for an SDK issue), it would be great to add a comment explaining why.
| function createSentryPlugin() { | ||
| if (!hasSentryUploadConfig) { | ||
| return []; | ||
| } | ||
| return [ | ||
| sentryVitePlugin({ | ||
| org: process.env.SENTRY_ORG, | ||
| project: process.env.SENTRY_PROJECT, | ||
| authToken: process.env.SENTRY_AUTH_TOKEN, | ||
| sourcemaps: { | ||
| filesToDeleteAfterUpload: ['**/*.js.map'], | ||
| }, | ||
| telemetry: false, | ||
| }), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
For improved type safety, consider using non-null assertions (!) for the environment variables within sentryVitePlugin. The hasSentryUploadConfig check already ensures these variables are defined, so asserting their existence makes the code more robust and communicates this guarantee to the TypeScript compiler.
| function createSentryPlugin() { | |
| if (!hasSentryUploadConfig) { | |
| return []; | |
| } | |
| return [ | |
| sentryVitePlugin({ | |
| org: process.env.SENTRY_ORG, | |
| project: process.env.SENTRY_PROJECT, | |
| authToken: process.env.SENTRY_AUTH_TOKEN, | |
| sourcemaps: { | |
| filesToDeleteAfterUpload: ['**/*.js.map'], | |
| }, | |
| telemetry: false, | |
| }), | |
| ]; | |
| } | |
| function createSentryPlugin() { | |
| if (!hasSentryUploadConfig) { | |
| return []; | |
| } | |
| return [ | |
| sentryVitePlugin({ | |
| org: process.env.SENTRY_ORG!, | |
| project: process.env.SENTRY_PROJECT!, | |
| authToken: process.env.SENTRY_AUTH_TOKEN!, | |
| sourcemaps: { | |
| filesToDeleteAfterUpload: ['**/*.js.map'], | |
| }, | |
| telemetry: false, | |
| }), | |
| ]; | |
| } |
| } catch (error) { | ||
| safeCaptureException(error as Error, { tags: { ipc_channel: channel } }); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The catch block casts error to Error using as Error. This is not type-safe, as the thrown value could be a string or another object type, which would lead to poor error reporting in Sentry. It's safer to ensure that an Error object is always captured to get a proper stack trace.
} catch (error) {
const exception = error instanceof Error ? error : new Error(String(error));
safeCaptureException(exception, { tags: { ipc_channel: channel } });
throw error;
}![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (publicKey && projectId) { | ||
| crashReporter.start({ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/electron.vite.config.ts (1)
100-108: 🧹 Nitpick | 🔵 TrivialPreload process is excluded from sourcemaps and Sentry plugin.
The
preloadconfig has neithersourcemapenabled norcreateSentryPlugin()added. Errors originating in preload code will produce minified stack traces in Sentry. Preload scripts are typically minimal, but they handle security-sensitive IPC bridging — having readable traces there is valuable.♻️ Add sourcemaps and Sentry plugin to preload
preload: { plugins: [externalizeDepsPlugin()], build: { + sourcemap: 'hidden', rollupOptions: { input: { index: resolve(__dirname, 'src/preload/index.ts') } } - } + }, + plugins: [...createSentryPlugin()], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/electron.vite.config.ts` around lines 100 - 108, Preload build config is missing sourcemaps and Sentry instrumentation; update the preload block (the preload object in electron.vite.config.ts) to enable build.sourcemap: true and add the Sentry Vite plugin (createSentryPlugin()) to the preload.plugins array (alongside externalizeDepsPlugin()) so preload bundles produce readable stack traces in Sentry; ensure you import/createSentryPlugin where the other plugins are configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 78-80: Remove the redundant Sentry environment variables
(SENTRY_AUTH_TOKEN, SENTRY_ORG, SENTRY_PROJECT) from the packaging workflow
steps so they are only present for the Vite build step; specifically edit the
release.yml packaging jobs named "Package macOS (ARM64)", "Package Windows", and
"Package Linux" (and any other package:* steps) to drop those three env entries
under their env: blocks, leaving Sentry vars only where the Vite build (npm run
build) runs and consumes `@sentry/vite-plugin`.
In `@apps/frontend/electron.vite.config.ts`:
- Around line 38-53: createSentryPlugin currently doesn't set a release so the
uploaded sourcemaps use a different release than the runtime SDK (Sentry.init).
Fix by reading the app version at build time and passing it to sentryVitePlugin
as release.name (e.g. in createSentryPlugin read package.json version and set
release: { name: `auto-claude@${version}` }) so the plugin and SDK match;
alternatively, remove the explicit release: ... from Sentry.init so the
plugin-injected release is used at runtime. Reference: createSentryPlugin,
sentryVitePlugin, Sentry.init, app.getVersion.
- Around line 38-53: The createSentryPlugin function currently returns
sentryVitePlugin(...) without an errorHandler, so upload/release failures will
abort the build; modify the sentryVitePlugin call in createSentryPlugin to
include an errorHandler callback that receives the error and uses the existing
logger (or console.warn) to emit a non-fatal warning message (including error
details) instead of rethrowing, ensuring compilation continues when
sourcemap/upload fails; keep all other options (org, project, authToken,
sourcemaps, telemetry) intact.
- Line 90: Replace the Vite build option sourcemap: true with sourcemap:
'hidden' so Sentry-compatible hidden source maps are produced (avoid embedding
//# sourceMappingURL= in output). Update both occurrences of the sourcemap
setting in this file (the renderer and main build configs where sourcemap: true
appears) to use sourcemap: 'hidden' instead of true; this keeps .js.map files
for upload but prevents emitted sourceMappingURL comments after
filesToDeleteAfterUpload removes the maps.
In `@apps/frontend/src/main/__tests__/sentry.test.ts`:
- Around line 1-81: Add tests for initSentryMain to cover the
crashReporter.start code path: import initSentryMain from '../sentry' (or the
exported name used) after setting up mocks, then (1) mock DEFAULT_APP_SETTINGS
to enable sentry and mock readSettingsFile or the DSN provider to return a valid
DSN string, call initSentryMain(), and assert crashReporter.start was called
once with an object whose submitURL matches the DSN-derived URL you expect; (2)
mock sentry disabled (DEFAULT_APP_SETTINGS.sentryEnabled = false) or return no
DSN, call initSentryMain(), and assert crashReporter.start was not called;
ensure you clear mocks between cases and reference initSentryMain and
crashReporter.start in your assertions.
- Around line 47-51: Tests import '../sentry' in beforeEach but rely on
module-level state which can persist between tests; call vi.resetModules()
before the dynamic import in the beforeEach to clear Node's module cache so that
functions/vars like withSentryIpc, initSentryMain, sentryEnabledState and
cachedDsn are reinitialized for each test run.
In `@apps/frontend/src/main/sentry.ts`:
- Around line 147-167: The DSN parsing can produce a broken projectId when the
pathname contains prefixes (e.g., /sentry/12345); update the parsing logic where
cachedDsn is turned into dsnUrl/publicKey/projectId (the block guarded by
shouldEnable that calls crashReporter.start) to derive projectId as the last
non-empty segment of dsnUrl.pathname (e.g., split on '/' and take the final
truthy element) instead of pathname.slice(1); keep using dsnUrl.username for
publicKey, validate both values, and if projectId is missing log/warn and avoid
calling crashReporter.start to prevent constructing an invalid submitURL.
- Around line 242-254: The catch in withSentryIpc assumes the thrown value is an
Error by using "error as Error"; change the handler's catch to accept unknown
and normalize the value for logging by calling safeCaptureException with either
the Error (if error instanceof Error) or a new Error(String(error)) so the
Sentry API gets an Error object, but still rethrow the original thrown value
(throw error) to preserve behavior; update references in withSentryIpc and
ensure safeCaptureException is called with the normalized Error and tags: {
ipc_channel: channel } while leaving handler invocation unchanged.
---
Outside diff comments:
In `@apps/frontend/electron.vite.config.ts`:
- Around line 100-108: Preload build config is missing sourcemaps and Sentry
instrumentation; update the preload block (the preload object in
electron.vite.config.ts) to enable build.sourcemap: true and add the Sentry Vite
plugin (createSentryPlugin()) to the preload.plugins array (alongside
externalizeDepsPlugin()) so preload bundles produce readable stack traces in
Sentry; ensure you import/createSentryPlugin where the other plugins are
configured.
---
Duplicate comments:
In @.github/workflows/beta-release.yml:
- Around line 125-127: Remove the redundant Sentry environment variables
(SENTRY_AUTH_TOKEN, SENTRY_ORG, SENTRY_PROJECT) from the packaging steps that do
not consume them—specifically the macOS Intel/ARM64, macOS ARM64, Windows, and
Linux package steps in the beta-release workflow—since electron-builder does not
use those step-level envs; locate the entries where those three symbols are
listed under the packaging job steps and delete them (the same change should be
applied at the other occurrences noted: the blocks around the duplicate
locations referenced in the comment).
| SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} | ||
| SENTRY_ORG: ${{ secrets.SENTRY_ORG }} | ||
| SENTRY_PROJECT: ${{ secrets.SENTRY_PROJECT }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sentry credentials in the packaging steps are unused and redundant.
SENTRY_AUTH_TOKEN, SENTRY_ORG, and SENTRY_PROJECT are consumed only during the Vite build step (npm run build) by the @sentry/vite-plugin. The subsequent packaging steps (npm run package:*) invoke electron-builder, which does not call Vite and therefore never reads these variables. Including them in the package steps is harmless but adds noise.
♻️ Remove redundant Sentry env vars from package steps
- name: Package macOS (Intel)
run: cd apps/frontend && npm run package:mac -- --x64
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
CSC_LINK: ${{ secrets.MAC_CERTIFICATE }}
CSC_KEY_PASSWORD: ${{ secrets.MAC_CERTIFICATE_PASSWORD }}
SENTRY_DSN: ${{ secrets.SENTRY_DSN }}
SENTRY_TRACES_SAMPLE_RATE: ${{ secrets.SENTRY_TRACES_SAMPLE_RATE }}
SENTRY_PROFILES_SAMPLE_RATE: ${{ secrets.SENTRY_PROFILES_SAMPLE_RATE }}
- SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }}
- SENTRY_ORG: ${{ secrets.SENTRY_ORG }}
- SENTRY_PROJECT: ${{ secrets.SENTRY_PROJECT }}Apply the same removal to the Package macOS (ARM64), Package Windows, and Package Linux steps.
Also applies to: 152-154, 228-230, 406-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 78 - 80, Remove the redundant
Sentry environment variables (SENTRY_AUTH_TOKEN, SENTRY_ORG, SENTRY_PROJECT)
from the packaging workflow steps so they are only present for the Vite build
step; specifically edit the release.yml packaging jobs named "Package macOS
(ARM64)", "Package Windows", and "Package Linux" (and any other package:* steps)
to drop those three env entries under their env: blocks, leaving Sentry vars
only where the Vite build (npm run build) runs and consumes `@sentry/vite-plugin`.
| function createSentryPlugin() { | ||
| if (!hasSentryUploadConfig) { | ||
| return []; | ||
| } | ||
| return [ | ||
| sentryVitePlugin({ | ||
| org: process.env.SENTRY_ORG, | ||
| project: process.env.SENTRY_PROJECT, | ||
| authToken: process.env.SENTRY_AUTH_TOKEN, | ||
| sourcemaps: { | ||
| filesToDeleteAfterUpload: ['**/*.js.map'], | ||
| }, | ||
| telemetry: false, | ||
| }), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Release name mismatch between the Sentry plugin and Sentry.init().
The plugin has no release.name set, so it defaults to automatically detecting a value for your environment, and otherwise uses the git HEAD's commit SHA. Meanwhile, sentry.ts calls Sentry.init({ release: \auto-claude@${app.getVersion()}` })`.
The plugin injects release information into the build for the SDK to pick up when sending events (defaults to true). Because Sentry.init() explicitly overrides that injected release, events will be tagged with [email protected], while source maps are uploaded under the auto-detected git SHA release. With the modern debug-ID approach, source map resolution still works correctly. However, Sentry's release tracking features (deploy tracking, release health, which errors appeared in which version) will show a mismatch.
Consider either:
- Reading the version from
package.jsonat build time and passing it asrelease: { name: \auto-claude@${version}` }insentryVitePlugin`. - Removing the explicit
release:fromSentry.init()and letting the plugin-injected value propagate through.
♻️ Option 1 — align plugin release with SDK release
+import { readFileSync } from 'fs';
+const { version } = JSON.parse(readFileSync(resolve(__dirname, 'package.json'), 'utf8'));
function createSentryPlugin() {
if (!hasSentryUploadConfig) {
return [];
}
return [
sentryVitePlugin({
org: process.env.SENTRY_ORG,
project: process.env.SENTRY_PROJECT,
authToken: process.env.SENTRY_AUTH_TOKEN,
+ release: {
+ name: `auto-claude@${version}`,
+ },
sourcemaps: {
filesToDeleteAfterUpload: ['**/*.js.map'],
},
telemetry: false,
}),
];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/electron.vite.config.ts` around lines 38 - 53,
createSentryPlugin currently doesn't set a release so the uploaded sourcemaps
use a different release than the runtime SDK (Sentry.init). Fix by reading the
app version at build time and passing it to sentryVitePlugin as release.name
(e.g. in createSentryPlugin read package.json version and set release: { name:
`auto-claude@${version}` }) so the plugin and SDK match; alternatively, remove
the explicit release: ... from Sentry.init so the plugin-injected release is
used at runtime. Reference: createSentryPlugin, sentryVitePlugin, Sentry.init,
app.getVersion.
Missing errorHandler — upload failures will abort the CI release build.
By default, when an error occurs during release creation or sourcemaps upload, the plugin will simply throw an error, thereby stopping the bundling process. If an errorHandler callback is provided, compilation will continue, unless an error is thrown in the provided callback.
If SENTRY_AUTH_TOKEN is set (enabling the plugin via hasSentryUploadConfig) but has insufficient permissions, or if Sentry's ingest endpoint is unreachable during CI, every release build will fail. This could block production releases on transient infrastructure issues. Add an errorHandler to demote upload failures to build warnings while still surfacing the problem.
🛡️ Proposed fix — degrade upload failure to warning, not build halt
function createSentryPlugin() {
if (!hasSentryUploadConfig) {
return [];
}
return [
sentryVitePlugin({
org: process.env.SENTRY_ORG,
project: process.env.SENTRY_PROJECT,
authToken: process.env.SENTRY_AUTH_TOKEN,
sourcemaps: {
filesToDeleteAfterUpload: ['**/*.js.map'],
},
telemetry: false,
+ errorHandler: (err) => {
+ console.warn('[Sentry] Source map upload failed:', err.message);
+ },
}),
];
}📝 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.
| function createSentryPlugin() { | |
| if (!hasSentryUploadConfig) { | |
| return []; | |
| } | |
| return [ | |
| sentryVitePlugin({ | |
| org: process.env.SENTRY_ORG, | |
| project: process.env.SENTRY_PROJECT, | |
| authToken: process.env.SENTRY_AUTH_TOKEN, | |
| sourcemaps: { | |
| filesToDeleteAfterUpload: ['**/*.js.map'], | |
| }, | |
| telemetry: false, | |
| }), | |
| ]; | |
| } | |
| function createSentryPlugin() { | |
| if (!hasSentryUploadConfig) { | |
| return []; | |
| } | |
| return [ | |
| sentryVitePlugin({ | |
| org: process.env.SENTRY_ORG, | |
| project: process.env.SENTRY_PROJECT, | |
| authToken: process.env.SENTRY_AUTH_TOKEN, | |
| sourcemaps: { | |
| filesToDeleteAfterUpload: ['**/*.js.map'], | |
| }, | |
| telemetry: false, | |
| errorHandler: (err) => { | |
| console.warn('[Sentry] Source map upload failed:', err.message); | |
| }, | |
| }), | |
| ]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/electron.vite.config.ts` around lines 38 - 53, The
createSentryPlugin function currently returns sentryVitePlugin(...) without an
errorHandler, so upload/release failures will abort the build; modify the
sentryVitePlugin call in createSentryPlugin to include an errorHandler callback
that receives the error and uses the existing logger (or console.warn) to emit a
non-fatal warning message (including error details) instead of rethrowing,
ensuring compilation continues when sourcemap/upload fails; keep all other
options (org, project, authToken, sourcemaps, telemetry) intact.
| ...createSentryPlugin(), | ||
| ], | ||
| build: { | ||
| sourcemap: true, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer sourcemap: 'hidden' over sourcemap: true per Sentry's official guidance.
Sentry's Vite documentation uses sourcemap: "hidden" for source map generation to avoid embedding //# sourceMappingURL= comments in the output bundles. With sourcemap: true, those comments are written into the deployed JS files. After filesToDeleteAfterUpload removes the .js.map files, the embedded URL references point to non-existent paths — harmless in packaged Electron apps but inconsistent with Sentry's recommended setup.
♻️ Switch to hidden sourcemaps in both main and renderer
main: {
// ...
build: {
- sourcemap: true,
+ sourcemap: 'hidden', renderer: {
// ...
build: {
- sourcemap: true,
+ sourcemap: 'hidden',Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/electron.vite.config.ts` at line 90, Replace the Vite build
option sourcemap: true with sourcemap: 'hidden' so Sentry-compatible hidden
source maps are produced (avoid embedding //# sourceMappingURL= in output).
Update both occurrences of the sourcemap setting in this file (the renderer and
main build configs where sourcemap: true appears) to use sourcemap: 'hidden'
instead of true; this keeps .js.map files for upload but prevents emitted
sourceMappingURL comments after filesToDeleteAfterUpload removes the maps.
| /** | ||
| * Tests for Sentry utility functions | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import type { IpcMainInvokeEvent } from 'electron'; | ||
|
|
||
| const mockCaptureException = vi.fn(); | ||
|
|
||
| vi.mock('@sentry/electron/main', () => ({ | ||
| init: vi.fn(), | ||
| addBreadcrumb: vi.fn(), | ||
| captureException: mockCaptureException, | ||
| })); | ||
|
|
||
| vi.mock('electron', () => ({ | ||
| app: { isPackaged: false, getVersion: () => '0.0.0-test' }, | ||
| ipcMain: { on: vi.fn(), handle: vi.fn() }, | ||
| crashReporter: { start: vi.fn() }, | ||
| })); | ||
|
|
||
| vi.mock('../settings-utils', () => ({ | ||
| readSettingsFile: () => ({}), | ||
| })); | ||
|
|
||
| vi.mock('../../shared/constants', () => ({ | ||
| DEFAULT_APP_SETTINGS: { sentryEnabled: true }, | ||
| })); | ||
|
|
||
| vi.mock('../../shared/constants/ipc', () => ({ | ||
| IPC_CHANNELS: { | ||
| SENTRY_STATE_CHANGED: 'sentry-state-changed', | ||
| GET_SENTRY_DSN: 'get-sentry-dsn', | ||
| GET_SENTRY_CONFIG: 'get-sentry-config', | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock('../../shared/utils/sentry-privacy', () => ({ | ||
| processEvent: vi.fn((e) => e), | ||
| PRODUCTION_TRACE_SAMPLE_RATE: 0.1, | ||
| })); | ||
|
|
||
| describe('withSentryIpc', () => { | ||
| let withSentryIpc: typeof import('../sentry').withSentryIpc; | ||
| const fakeEvent = {} as IpcMainInvokeEvent; | ||
|
|
||
| beforeEach(async () => { | ||
| vi.clearAllMocks(); | ||
| const mod = await import('../sentry'); | ||
| withSentryIpc = mod.withSentryIpc; | ||
| }); | ||
|
|
||
| it('passes through successful handler result', async () => { | ||
| const handler = vi.fn().mockResolvedValue('ok'); | ||
| const wrapped = withSentryIpc('test-channel', handler); | ||
| const result = await wrapped(fakeEvent, 'arg1'); | ||
| expect(result).toBe('ok'); | ||
| expect(handler).toHaveBeenCalledWith(fakeEvent, 'arg1'); | ||
| }); | ||
|
|
||
| it('re-throws and captures exception on handler error', async () => { | ||
| const err = new Error('boom'); | ||
| const handler = vi.fn().mockRejectedValue(err); | ||
| const wrapped = withSentryIpc('test-channel', handler); | ||
| await expect(wrapped(fakeEvent)).rejects.toThrow('boom'); | ||
| expect(mockCaptureException).toHaveBeenCalledWith( | ||
| err, | ||
| { tags: { ipc_channel: 'test-channel' } } | ||
| ); | ||
| }); | ||
|
|
||
| it('tags error with the correct ipc_channel', async () => { | ||
| const handler = vi.fn().mockRejectedValue(new Error('fail')); | ||
| const wrapped = withSentryIpc('my-custom-channel', handler); | ||
| await expect(wrapped(fakeEvent)).rejects.toThrow('fail'); | ||
| expect(mockCaptureException).toHaveBeenCalledWith( | ||
| expect.any(Error), | ||
| { tags: { ipc_channel: 'my-custom-channel' } } | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No test coverage for the new crashReporter.start() code path.
The crash reporter initialization added in initSentryMain() (lines 144–167 of sentry.ts) is not exercised by any test. The crashReporter.start mock is registered but never asserted upon. Given this involves DSN URL parsing and a conditional start call, consider adding a test that calls initSentryMain() with a valid DSN and verifies:
crashReporter.startis called with the correctsubmitURLcrashReporter.startis not called when Sentry is disabled or DSN is absent
Would you like me to draft the initSentryMain + crashReporter test cases?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/main/__tests__/sentry.test.ts` around lines 1 - 81, Add
tests for initSentryMain to cover the crashReporter.start code path: import
initSentryMain from '../sentry' (or the exported name used) after setting up
mocks, then (1) mock DEFAULT_APP_SETTINGS to enable sentry and mock
readSettingsFile or the DSN provider to return a valid DSN string, call
initSentryMain(), and assert crashReporter.start was called once with an object
whose submitURL matches the DSN-derived URL you expect; (2) mock sentry disabled
(DEFAULT_APP_SETTINGS.sentryEnabled = false) or return no DSN, call
initSentryMain(), and assert crashReporter.start was not called; ensure you
clear mocks between cases and reference initSentryMain and crashReporter.start
in your assertions.
| beforeEach(async () => { | ||
| vi.clearAllMocks(); | ||
| const mod = await import('../sentry'); | ||
| withSentryIpc = mod.withSentryIpc; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module caching between beforeEach calls may cause cross-test state bleed.
await import('../sentry') on the second and subsequent test runs returns the cached module — vi.clearAllMocks() clears mock call history but does not re-execute module-level code. Module-level state (sentryEnabledState, cachedDsn, etc.) persists across tests. The three existing tests are unaffected because withSentryIpc is a stateless factory, but any future test that exercises initSentryMain() or modifies module state will see stale values from a prior test.
♻️ Use `vi.resetModules()` for a clean slate per test
beforeEach(async () => {
vi.clearAllMocks();
+ vi.resetModules();
const mod = await import('../sentry');
withSentryIpc = mod.withSentryIpc;
});📝 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.
| beforeEach(async () => { | |
| vi.clearAllMocks(); | |
| const mod = await import('../sentry'); | |
| withSentryIpc = mod.withSentryIpc; | |
| }); | |
| beforeEach(async () => { | |
| vi.clearAllMocks(); | |
| vi.resetModules(); | |
| const mod = await import('../sentry'); | |
| withSentryIpc = mod.withSentryIpc; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/main/__tests__/sentry.test.ts` around lines 47 - 51, Tests
import '../sentry' in beforeEach but rely on module-level state which can
persist between tests; call vi.resetModules() before the dynamic import in the
beforeEach to clear Node's module cache so that functions/vars like
withSentryIpc, initSentryMain, sentryEnabledState and cachedDsn are
reinitialized for each test run.
| export function withSentryIpc<T>( | ||
| channel: string, | ||
| handler: (event: IpcMainInvokeEvent, ...args: unknown[]) => Promise<T> | ||
| ): (event: IpcMainInvokeEvent, ...args: unknown[]) => Promise<T> { | ||
| return async (event: IpcMainInvokeEvent, ...args: unknown[]): Promise<T> => { | ||
| try { | ||
| return await handler(event, ...args); | ||
| } catch (error) { | ||
| safeCaptureException(error as Error, { tags: { ipc_channel: channel } }); | ||
| throw error; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
error as Error cast is unsafe for non-Error thrown values.
IPC handlers may throw strings, numbers, or plain objects. The cast error as Error is a compile-time fiction — at runtime, a throw 'my error string' passes a string to safeCaptureException, whose Sentry.captureException call can still handle it, but the signature contract is violated. A minimal fix wraps unknown thrown values:
💡 Safer pattern for unknown thrown type
} catch (error) {
- safeCaptureException(error as Error, { tags: { ipc_channel: channel } });
+ const err = error instanceof Error ? error : new Error(String(error));
+ safeCaptureException(err, { tags: { ipc_channel: channel } });
throw error;
}📝 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.
| export function withSentryIpc<T>( | |
| channel: string, | |
| handler: (event: IpcMainInvokeEvent, ...args: unknown[]) => Promise<T> | |
| ): (event: IpcMainInvokeEvent, ...args: unknown[]) => Promise<T> { | |
| return async (event: IpcMainInvokeEvent, ...args: unknown[]): Promise<T> => { | |
| try { | |
| return await handler(event, ...args); | |
| } catch (error) { | |
| safeCaptureException(error as Error, { tags: { ipc_channel: channel } }); | |
| throw error; | |
| } | |
| }; | |
| } | |
| export function withSentryIpc<T>( | |
| channel: string, | |
| handler: (event: IpcMainInvokeEvent, ...args: unknown[]) => Promise<T> | |
| ): (event: IpcMainInvokeEvent, ...args: unknown[]) => Promise<T> { | |
| return async (event: IpcMainInvokeEvent, ...args: unknown[]): Promise<T> => { | |
| try { | |
| return await handler(event, ...args); | |
| } catch (error) { | |
| const err = error instanceof Error ? error : new Error(String(error)); | |
| safeCaptureException(err, { tags: { ipc_channel: channel } }); | |
| throw error; | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/main/sentry.ts` around lines 242 - 254, The catch in
withSentryIpc assumes the thrown value is an Error by using "error as Error";
change the handler's catch to accept unknown and normalize the value for logging
by calling safeCaptureException with either the Error (if error instanceof
Error) or a new Error(String(error)) so the Sentry API gets an Error object, but
still rethrow the original thrown value (throw error) to preserve behavior;
update references in withSentryIpc and ensure safeCaptureException is called
with the normalized Error and tags: { ipc_channel: channel } while leaving
handler invocation unchanged.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 issue(s) require attention.
1 issue(s) must be addressed (1 required, 0 recommended), 1 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 1 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (2 selected of 2 total)
🟠 [24c19aa75e54] [HIGH] Missing release config in sentryVitePlugin — source maps won't match runtime release
📁 apps/frontend/electron.vite.config.ts:43
The sentryVitePlugin() is configured without a release property. At runtime in sentry.ts:126, the Sentry SDK is initialized with release: 'auto-claude@${app.getVersion()}' (e.g. [email protected]). Without an explicit matching release in the Vite plugin config, the plugin defaults to auto-detecting the release from git (typically a commit SHA or proposed release name). This mismatch means uploaded source maps will be associated with the wrong release in Sentry, and production stack traces will NOT be de-minified — defeating the primary purpose of this feature.
The release property in sentryVitePlugin should be set to { name: 'auto-claude@' + version } where version comes from package.json, matching the runtime release value.
Suggested fix:
Add a `release` property to the sentryVitePlugin config that matches the runtime release string. For example:
```typescript
import pkg from './package.json';
// ...
sentryVitePlugin({
// ... existing config
release: {
name: `auto-claude@${pkg.version}`,
},
})
#### 🔵 [6217bf3b82ea] [LOW] Unsafe `error as Error` cast — non-Error thrown values will lose context
📁 `apps/frontend/src/main/sentry.ts:250`
In `withSentryIpc`, the caught value is cast with `error as Error` before passing to `safeCaptureException`. In JavaScript, `throw` can throw any value (strings, numbers, objects). When a non-Error value is thrown, `error as Error` produces a type lie — the cast value won't have `.message`, `.stack`, etc. The `safeCaptureException` function signature requires `Error`, so `Sentry.captureException()` receives a non-Error object, which may result in Sentry reporting `[object Object]` or the raw primitive with no stack trace.
This is a minor robustness issue since most code throws Error instances, but since this wrapper is designed for gradual adoption across 40+ IPC handlers, it should handle edge cases gracefully.
**Suggested fix:**
Normalize the error before passing to safeCaptureException:
const normalizedError = error instanceof Error ? error : new Error(String(error));
safeCaptureException(normalizedError, { tags: { ipc_channel: channel } });Alternatively, widen safeCaptureException's parameter type to unknown and let Sentry handle it (Sentry.captureException already accepts unknown).
---
*This review was generated by Auto Claude.*
…PC errors Add release.name to sentryVitePlugin config so uploaded source maps match the runtime release tag (auto-claude@<version>), enabling proper stack trace de-minification in production. Also normalize caught values in withSentryIpc to handle non-Error thrown values without losing context. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/main/sentry.ts`:
- Around line 162-165: Replace the console calls inside the app.isPackaged
block: change the informational console.log('[Sentry] Native crash reporter
started (minidump ingestion)') to record a breadcrumb via safeBreadcrumb
(include a short message and relevant metadata), and replace the catch path
console.warn('[Sentry] Failed to start native crash reporter:', error) with
safeCaptureException(error, { level: 'warning', extra: { context: 'native crash
reporter start' } }) so Sentry captures the failure; update any surrounding code
where these messages live to import/ use safeBreadcrumb and safeCaptureException
consistently.
- Around line 147-167: Gate native crashReporter.start() on the actual user
preference by checking sentryEnabledState (the same flag used in beforeSend) in
addition to shouldEnable/hasDsn/app.isPackaged before calling
crashReporter.start (so if the user has opted out at startup we never start
minidump ingestion); replace console.log and console.warn calls around
crashReporter.start() with calls to safeCaptureException() for errors and add a
Sentry breadcrumb for the successful start (use the existing
safeCaptureException and Sentry breadcrumb APIs), and add a short comment near
crashReporter.start() noting Electron has no crashReporter.stop() so once
started it cannot be disabled until next launch.
---
Duplicate comments:
In `@apps/frontend/electron.vite.config.ts`:
- Around line 47-58: The sentryVitePlugin configuration is missing an
errorHandler so upload failures currently abort the CI/release; add an
errorHandler callback on the sentryVitePlugin config (the same object where
org/project/authToken/release/sourcemaps/telemetry are set) that captures plugin
upload errors, logs them via your logger or console with context (e.g., "Sentry
upload failed for release auto-claude@{pkg.version}") and prevents the plugin
from throwing (return/resolve in the handler or mark as handled) so CI doesn't
fail on sourcemap upload issues; reference the sentryVitePlugin configuration
and implement errorHandler there.
- Line 97: The Vite config currently sets sourcemap: true; update the build
config in apps/frontend/electron.vite.config.ts to use sourcemap: 'hidden'
instead of true by editing the exported config object (the property named
sourcemap in the Vite config) so source maps are generated but not exposed;
locate the sourcemap entry in the config and replace its value accordingly.
In `@apps/frontend/src/main/sentry.ts`:
- Line 153: The assignment of projectId via dsnUrl.pathname.slice(1) is brittle
for self-hosted Sentry installations with path prefixes; change the extraction
of projectId to robustly take the last non-empty path segment from
dsnUrl.pathname (e.g., split on '/', filter out empty segments, then pop the
last item) so projectId correctly resolves regardless of path prefixes; update
the code around the projectId variable in main/sentry.ts where
dsnUrl.pathname.slice(1) is used and adjust any related logic/tests that assumed
the old behavior.
| if (shouldEnable) { | ||
| try { | ||
| // Extract the public key and project ID from the DSN | ||
| // DSN format: https://<public_key>@<host>/<project_id> | ||
| const dsnUrl = new URL(cachedDsn); | ||
| const publicKey = dsnUrl.username; | ||
| const projectId = dsnUrl.pathname.slice(1); | ||
| const sentryHost = dsnUrl.hostname; | ||
|
|
||
| if (publicKey && projectId) { | ||
| crashReporter.start({ | ||
| submitURL: `https://${sentryHost}/api/${projectId}/minidump/?sentry_key=${publicKey}`, | ||
| uploadToServer: true, | ||
| compress: true, | ||
| }); | ||
| console.log('[Sentry] Native crash reporter started (minidump ingestion)'); | ||
| } | ||
| } catch (error) { | ||
| console.warn('[Sentry] Failed to start native crash reporter:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Look for startCrashReporter or crashReporter.start in `@sentry/electron` dist
echo "=== Searching for crashReporter references in `@sentry/electron` ==="
fd -t f "\.js$" --search-path node_modules/@sentry/electron --max-results=50 2>/dev/null | head -20
echo "=== Checking for crashReporter.start in `@sentry/electron` ==="
rg "crashReporter\.start\|startCrashReporter" node_modules/@sentry/electron/ --type js -C 3 2>/dev/null || echo "No results found or node_modules not available"
# 2. Find all manual crashReporter.start calls in project source
echo -e "\n=== Finding all crashReporter.start calls in apps/frontend/src ==="
rg -n "crashReporter\.start" apps/frontend/src/ -C 2 2>/dev/null || echo "No results found"
# 3. Check the actual sentry.ts file for context
echo -e "\n=== Checking apps/frontend/src/main/sentry.ts ==="
wc -l apps/frontend/src/main/sentry.tsRepository: AndyMik90/Auto-Claude
Length of output: 776
🏁 Script executed:
#!/bin/bash
# Read the full sentry.ts file to understand context, especially around sentryEnabledState and shouldEnable
echo "=== Reading sentry.ts to verify sentryEnabledState and variable definitions ==="
cat -n apps/frontend/src/main/sentry.tsRepository: AndyMik90/Auto-Claude
Length of output: 12031
Gate crash reporter startup on user opt-out preference and replace console.log with Sentry diagnostics.
Two confirmed issues:
1. User opt-out preference not honoured for native crash reporter.
sentryEnabledState (line 36) is checked in the beforeSend hook to filter JS errors (line 129), but the crashReporter.start() block (line 147) only guards on shouldEnable, which evaluates hasDsn && app.isPackaged. This means if a user disables error reporting in settings, the native crash reporter (minidump ingestion) still starts on the next app launch. Since Electron provides no crashReporter.stop() API, the only opportunity to honour the preference is at startup.
2. Console.log/warn usage violates coding guidelines.
Per guidelines: "Never use console.log for debugging production issues...Reserve console.log for development only." Lines 162 and 165 use console.log and console.warn in production startup code; these should use Sentry's safeCaptureException() and breadcrumbs instead.
🛡️ Proposed fix
- if (shouldEnable) {
+ if (shouldEnable && sentryEnabledState) {
try {
const dsnUrl = new URL(cachedDsn);
const publicKey = dsnUrl.username;
const projectId = dsnUrl.pathname.slice(1);
const sentryHost = dsnUrl.hostname;
if (publicKey && projectId) {
crashReporter.start({
submitURL: `https://${sentryHost}/api/${projectId}/minidump/?sentry_key=${publicKey}`,
uploadToServer: true,
compress: true,
});
}
} catch (error) {
- console.warn('[Sentry] Failed to start native crash reporter:', error);
+ safeCaptureException(error instanceof Error ? error : new Error(String(error)));
}
}Add a comment that the crash reporter cannot be stopped if the user later toggles the setting off — this is a known Electron limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/main/sentry.ts` around lines 147 - 167, Gate native
crashReporter.start() on the actual user preference by checking
sentryEnabledState (the same flag used in beforeSend) in addition to
shouldEnable/hasDsn/app.isPackaged before calling crashReporter.start (so if the
user has opted out at startup we never start minidump ingestion); replace
console.log and console.warn calls around crashReporter.start() with calls to
safeCaptureException() for errors and add a Sentry breadcrumb for the successful
start (use the existing safeCaptureException and Sentry breadcrumb APIs), and
add a short comment near crashReporter.start() noting Electron has no
crashReporter.stop() so once started it cannot be disabled until next launch.
| console.log('[Sentry] Native crash reporter started (minidump ingestion)'); | ||
| } | ||
| } catch (error) { | ||
| console.warn('[Sentry] Failed to start native crash reporter:', error); |
There was a problem hiding this comment.
console.log / console.warn in production code path — violates coding guidelines.
Lines 162 and 165 are new additions inside an app.isPackaged-gated block. The informational log on line 162 should use safeBreadcrumb; the failure path on line 165 should use safeCaptureException (Sentry is already initialized at this point) rather than silently swallowing the error to console.
♻️ Proposed fix
if (publicKey && projectId) {
crashReporter.start({
submitURL: `https://${sentryHost}/api/${projectId}/minidump/?sentry_key=${publicKey}`,
uploadToServer: true,
compress: true,
});
- console.log('[Sentry] Native crash reporter started (minidump ingestion)');
+ safeBreadcrumb({ message: 'Native crash reporter started (minidump ingestion)', level: 'info' });
}
} catch (error) {
- console.warn('[Sentry] Failed to start native crash reporter:', error);
+ safeCaptureException(error instanceof Error ? error : new Error(String(error)));
}Based on learnings: "Never use console.log for debugging production issues in the Electron app. Use Sentry for error tracking and diagnostics in production. Reserve console.log for development only."
📝 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.
| console.log('[Sentry] Native crash reporter started (minidump ingestion)'); | |
| } | |
| } catch (error) { | |
| console.warn('[Sentry] Failed to start native crash reporter:', error); | |
| if (publicKey && projectId) { | |
| crashReporter.start({ | |
| submitURL: `https://${sentryHost}/api/${projectId}/minidump/?sentry_key=${publicKey}`, | |
| uploadToServer: true, | |
| compress: true, | |
| }); | |
| safeBreadcrumb({ message: 'Native crash reporter started (minidump ingestion)', level: 'info' }); | |
| } | |
| } catch (error) { | |
| safeCaptureException(error instanceof Error ? error : new Error(String(error))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/main/sentry.ts` around lines 162 - 165, Replace the console
calls inside the app.isPackaged block: change the informational
console.log('[Sentry] Native crash reporter started (minidump ingestion)') to
record a breadcrumb via safeBreadcrumb (include a short message and relevant
metadata), and replace the catch path console.warn('[Sentry] Failed to start
native crash reporter:', error) with safeCaptureException(error, { level:
'warning', extra: { context: 'native crash reporter start' } }) so Sentry
captures the failure; update any surrounding code where these messages live to
import/ use safeBreadcrumb and safeCaptureException consistently.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 6 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 7 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 5 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- quality: Native crash reporter ignores user opt-out preference — privacy violation
- quality: DSN parsing breaks for self-hosted Sentry with path prefixes
- quality: console.log/console.warn in production code paths violates CLAUDE.md guidelines
- quality: Missing test coverage for non-Error thrown values in withSentryIpc
- quality: [FROM COMMENTS] DSN parsing not robust for self-hosted Sentry (sentry[bot])
Verdict
✅ All 20 CI checks passing. Both previous findings (HIGH: missing release config, LOW: unsafe error cast) have been properly resolved — excellent work.
However, 1 HIGH and 3 MEDIUM severity confirmed-valid findings remain:
-
[HIGH] CMT-003 — Native crash reporter ignores user opt-out preference. The
crashReporter.start()is gated only onshouldEnable(DSN + isPackaged) but does NOT checksentryEnabledState. Users who opted out of Sentry still have native crash minidumps uploaded — a privacy violation. Fix: add&& sentryEnabledStateto the condition at line 147. -
[MEDIUM] NEW-001 — DSN parsing breaks for self-hosted Sentry with path prefixes.
dsnUrl.pathname.slice(1)extractsprefix/123instead of123for DSNs likehttps://key@host/prefix/123. Cross-validated by both sentry[bot] and new-code-reviewer. -
[MEDIUM] NEW-002 — console.log/console.warn used in production code paths, violating CLAUDE.md guidelines. The project has
appLogfrom app-logger.ts which should be used instead. -
[MEDIUM] NEW-004 — Missing test coverage for the non-Error normalization path in
withSentryIpc— the specific feature this commit introduces.
1 LOW severity finding (NEW-003 — DSN exposed to renderer) is informational and does not block merge. 1 finding (NEW-005 — hardcoded HTTPS) was dismissed as false positive.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (7 selected of 7 total)
🟠 [CMT-003] [HIGH] Native crash reporter ignores user opt-out preference — privacy violation
📁 apps/frontend/src/main/sentry.ts:147
The crashReporter.start() at line 157 is gated only on shouldEnable (DSN + isPackaged) but does NOT check sentryEnabledState (the user's Sentry preference from settings). If a user has disabled Sentry (sentryEnabled: false), the JS-level beforeSend hook blocks SDK events, but the native crash reporter still uploads minidumps because it operates at the Chromium level and bypasses the JS SDK entirely. Electron provides no crashReporter.stop() API, so once started it cannot be disabled mid-session.
Suggested fix:
Add sentryEnabledState to the gate condition at line 147: `if (shouldEnable && sentryEnabledState) {`. Since crashReporter cannot be stopped once started, this at minimum respects the initial user preference.
🟡 [NEW-001] [MEDIUM] DSN parsing breaks for self-hosted Sentry with path prefixes
📁 apps/frontend/src/main/sentry.ts:153
The code uses dsnUrl.pathname.slice(1) to extract the project ID from the Sentry DSN. For standard Sentry.io DSNs (https://[email protected]/456), this works. But for self-hosted instances with path prefixes (https://[email protected]/prefix/123), pathname is /prefix/123 and slice(1) yields prefix/123, producing an invalid minidump URL. Cross-validated by sentry[bot] and new-code-reviewer.
Suggested fix:
Extract only the last path segment: `const pathSegments = dsnUrl.pathname.split('/').filter(Boolean); const projectId = pathSegments.pop();`
🟡 [NEW-002] [MEDIUM] console.log/console.warn in production code paths violates CLAUDE.md guidelines
📁 apps/frontend/src/main/sentry.ts:162
sentry.ts has 7 console.log/console.warn calls in code paths that execute in production (packaged app). CLAUDE.md explicitly states: 'No console.log for debugging production issues — console.log output is not visible in bundled/packaged versions.' The project has appLog from app-logger.ts (backed by electron-log) that writes to persistent log files and should be used instead. Most critically, crash reporter failure on line 165 (console.warn) would be invisible in production.
Suggested fix:
Import appLog from './app-logger' and replace console.log with appLog.info and console.warn with appLog.warn throughout sentry.ts.
🟡 [NEW-004] [MEDIUM] Missing test coverage for non-Error thrown values in withSentryIpc
📁 apps/frontend/src/main/__tests__/sentry.test.ts:43
The withSentryIpc function (sentry.ts:250) has explicit normalization for non-Error thrown values: error instanceof Error ? error : new Error(String(error)). This is the specific IPC error normalization feature added in this commit. However, all 3 test cases only throw Error instances (new Error('boom'), new Error('fail')). The non-Error normalization path has zero test coverage.
Suggested fix:
Add a test: `it('normalizes non-Error thrown values', async () => { const handler = vi.fn().mockRejectedValue('string error'); const wrapped = withSentryIpc('test', handler); await expect(wrapped(fakeEvent)).rejects.toBe('string error'); expect(mockCaptureException).toHaveBeenCalledWith(expect.objectContaining({ message: 'string error' }), ...); });`
🔵 [NEW-003] [LOW] Sentry DSN exposed to renderer via IPC (informational)
📁 apps/frontend/src/main/sentry.ts:176
The full Sentry DSN (with public key) is exposed to the renderer process via GET_SENTRY_DSN and GET_SENTRY_CONFIG IPC handlers. This is expected and safe — Sentry DSNs are designed as client-side public keys that only allow sending events, not reading them. The renderer needs the DSN to initialize its own Sentry client. Standard Sentry Electron pattern.
🟡 [CMT-001] [MEDIUM] [FROM COMMENTS] DSN parsing not robust for self-hosted Sentry (sentry[bot])
📁 apps/frontend/src/main/sentry.ts:153
Duplicate of NEW-001. sentry[bot] flagged that pathname.slice(1) for DSN parsing fails when the DSN contains a path prefix, common for self-hosted Sentry instances.
🔵 [CMT-002] [LOW] [FROM COMMENTS] console.log in production violates guidelines (coderabbitai)
📁 apps/frontend/src/main/sentry.ts:162
Duplicate of NEW-002. coderabbitai[bot] flagged console.log/console.warn in the app.isPackaged-gated block. Suggested using safeBreadcrumb for success and safeCaptureException for failures.
This review was generated by Auto Claude.
1 participant
Summary
Closes the 3 high-priority gaps identified in the Sentry production readiness assessment:
@sentry/vite-pluginto the build pipeline. During CI builds, source maps are uploaded to Sentry then stripped from the bundle (filesToDeleteAfterUpload), so production stack traces are readable without shipping maps to userscrashReporterfor minidump ingestion, catching GPU process crashes, native module segfaults (node-pty), and V8 snapshot failures that JS-level error handlers can't detectwithSentryIpc()utility for wrappingipcMain.handlecallbacks with automatic Sentry error capture and channel tagging, enabling gradual adoption across the 40+ IPC handler modulesCI Setup Required
Add these GitHub repository secrets for source map uploads to work:
SENTRY_AUTH_TOKEN— Sentry API auth token withorg:readandproject:releasesscopesSENTRY_ORG— Sentry organization slugSENTRY_PROJECT— Sentry project slugWithout these secrets, the Sentry plugin is safely disabled (no build failures).
Test plan
withSentryIpcunit tests pass (3 tests)process.crash()in dev produces a minidump in Sentry🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests