Conversation
|
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:
WalkthroughSentry was added and initialized across the app: main process (src/main/main.ts) now imports and initializes Sentry and installs global uncaughtException/unhandledRejection handlers; renderer (src/renderer/index.js) registers process/window error and unhandledrejection handlers and initializes caches; src/utils/sentryInit.js was refactored to build a unified Sentry configuration (process_type, environment, beforeSend, version tags) and perform guarded initialization; production webpack configs for main and renderer include Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/main/main.ts`:
- Around line 45-54: The manual global handlers in
process.on("uncaughtException", ...) and process.on("unhandledRejection", ...)
are double-reporting to Sentry because `@sentry/electron/main` already registers
handlers; remove the Sentry.captureException(...) calls from those handlers and
keep only logger.error(...) to preserve console logging (retain the same handler
functions but drop the capture calls). If you intend to suppress Sentry during
shutdown (interacting with the existing before-quit logic), explicitly remove
these listeners (the functions passed to process.on) before shutdown so they
won't run in addition to other shutdown listeners; reference the existing
process.on handlers and the before-quit shutdown registration when implementing
removal.
- Around line 64-65: Remove the spurious Sentry event by deleting the call to
Sentry.captureException(new Error('Test error from main process')); if you want
a non-production check instead, wrap that call with a development-only guard
(e.g., check process.env.NODE_ENV === 'development' or an equivalent isDev flag)
so it never fires in production environments—locate the statement using the
Sentry.captureException invocation and either remove it or replace it with a
guarded/debug-only block.
In `@src/renderer/index.js`:
- Around line 22-25: The error handler registered via
window.addEventListener("error", ...) currently passes event.error directly to
Sentry.captureException even though event.error can be null for cross-origin
script errors; update the handler in src/renderer/index.js (the listener
function) to guard against event.error === null and instead send a meaningful
fallback (e.g., construct an Error including event.message, event.filename, and
event.lineno or call Sentry.captureMessage with that info) so you never pass
null to Sentry.captureException; keep the console.error logging but only call
Sentry.captureException when event.error is non-null and use
Sentry.captureMessage or a constructed Error for the null case.
- Around line 11-30: Remove the manual Node-style
process.on("uncaughtException", ...) and process.on("unhandledRejection", ...)
handlers (renderer is browser context) and strip out the
Sentry.captureException(...) calls from the window.addEventListener("error",
...) and window.addEventListener("unhandledrejection", ...) listeners so you
only log via console.error; keep the console.error lines in the window listeners
(or remove the whole window listeners if you prefer relying entirely on
`@sentry/electron/renderer`'s GlobalHandlers), but do not call
Sentry.captureException from these handlers to avoid duplicate reporting.
In `@src/utils/sentryInit.js`:
- Around line 8-21: The main process currently always enables error tracking
because isErrorTrackingEnabled defaults to true and the preference load is only
attempted when !isMain; update the logic so the main process also respects the
user's sentry preference (or add a clear comment if the behavior is
intentional). Specifically, in src/utils/sentryInit.js adjust the isMain branch
to read the same preference used by the renderer
(userPreferencesManager.getPreferences().sentry.error_tracking_enabled) via a
mechanism accessible from main (e.g., electron-store or a shared config API) and
assign it to isErrorTrackingEnabled, or if you intentionally want main to ignore
preferences, add a comment next to isMain explaining why main must always enable
Sentry; reference symbols: isErrorTrackingEnabled, isMain,
userPreferencesManager (or switch to electron-store/shared config).
🧹 Nitpick comments (4)
src/renderer/index.js (1)
1-3: Import of@sentry/electron/rendereris unused if you remove manualcaptureExceptioncalls.If you adopt the suggestion to let the SDK handle error capturing natively, the
Sentryimport on line 3 becomes unnecessary and can be removed. The side-effect import ofsentryIniton line 2 is sufficient.src/utils/sentryInit.js (3)
41-43: Remove no-opintegrationscallback.This callback receives and returns integrations unchanged — it does nothing. Removing it reduces noise and avoids confusion about whether integrations were intentionally customized.
♻️ Proposed fix
// Sample rate for performance monitoring tracesSampleRate: 0.1, - // Capture unhandled promise rejections - integrations: (integrations) => { - return integrations; - },
44-65: Redundant tag and per-event overhead frombeforeSend.Two concerns:
Redundant tag:
process_type(line 47) duplicates theprocesstag already set ininitialScope.tags(line 30). Pick one name and set it once.
app.getVersion()on every event:require("electron").app.getVersion()is called insidebeforeSend, meaning it runs for every single Sentry event. Compute this once at init time and capture it in closure scope or ininitialScope.tags:♻️ Suggested approach — set version once at init
+ let appVersion; + if (isMain) { + try { + const { app } = require("electron"); + appVersion = app.getVersion(); + } catch (e) { /* not available yet */ } + } + const sentryConfig = { dsn: "...", environment: process.env.NODE_ENV || "production", + release: appVersion, initialScope: { tags: { - process: processType, + process_type: processType, electron_version: process.versions.electron, ... }, }, ... - beforeSend(event, _hint) { - if (event.tags) { - event.tags.process_type = processType; - } else { - event.tags = { process_type: processType }; - } - try { - if (isMain) { - const { app } = require("electron"); - if (app) { - event.tags.app_version = app.getVersion(); - } - } - } catch (e) {} - return event; - }, + beforeSend(event) { + return event; + }, };Using
releasein the Sentry config is the idiomatic way to associate a version with events, and it enables Sentry's release tracking features.
26-26: DSN is hardcoded in source.While DSNs are considered public for client-side SDKs, externalizing it to an environment variable or build-time config makes it easier to rotate or swap between Sentry projects (e.g., staging vs. production) without code changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.erb/configs/webpack.config.renderer.prod.ts:
- Around line 176-184: The Sentry plugin (sentryWebpackPlugin) is always added
to the webpack plugins array which causes builds to fail when
process.env.SENTRY_AUTH_TOKEN is missing; update the config to conditionally
include the plugin only when SENTRY_AUTH_TOKEN exists by creating an
optionalPlugins array (or similar) that pushes/contains sentryWebpackPlugin(...)
only if process.env.SENTRY_AUTH_TOKEN is truthy, then spread ...optionalPlugins
into the plugins array; ensure you still pass the
sourcemaps/filesToDeleteAfterUpload options when the plugin is included.
In `@src/utils/sentryInit.js`:
- Line 25: The Sentry configuration currently sets tracesSampleRate: 0.1 with an
unresolved inline TODO comment — remove the comment and replace the placeholder
with the agreed-upon sampling value (or make it configurable) in the sentry
initialization (the tracesSampleRate property in the Sentry init call); if the
value should come from the web app or environment, hook tracesSampleRate to a
validated config/env var (e.g., process.env.SENTRY_TRACES_SAMPLE_RATE or a
shared config getter) and ensure it is documented or defaulted so no TODO
remains.
🧹 Nitpick comments (4)
.erb/configs/webpack.config.renderer.prod.ts (1)
19-20: Mixingrequire()with ES module imports in a TypeScript file.The rest of the file uses ESM
importstatements. Usingrequire('dotenv').config(...)at the top level works but is inconsistent. Consider usingimportor at minimum adding a brief comment explaining whyrequireis necessary here (e.g., side-effect-only call with config argument).src/utils/sentryInit.js (3)
27-29: No-opintegrationscallback — remove it.This callback returns the default integrations unchanged. It adds no value and can be removed to reduce noise.
30-60:beforeSendperformsrequire()calls on every event — cache the app version instead.
require("electron")andrequire("../../package.json")are called insidebeforeSend, which runs for every Sentry event. While Node cachesrequire()results, the version lookup and try/catch on every event is unnecessary overhead. ResolveappVersiononce at init time and close over it.Additionally, the
processtag set ininitialScope(line 16) and theprocess_typetag added inbeforeSend(line 33) convey the same information with different names — pick one.🛠️ Suggested approach
+// Resolve app version once at module load +let appVersion = null; +if (isMain) { + try { + if (process.env.NODE_ENV === "development") { + appVersion = require("../../package.json").version; + } else { + const { app } = require("electron"); + appVersion = app?.getVersion() ?? null; + } + } catch (_) {} +} + const sentryConfig = { // ... beforeSend(event, _hint) { - if (event.tags) { - event.tags.process_type = processType; - } else { - event.tags = { process_type: processType }; - } - try { - if (isMain) { - // ... version resolution ... - } - } catch (e) {} + event.tags = { + ...event.tags, + process_type: processType, + ...(appVersion && { app_version: appVersion }), + }; return event; }, };
12-12: DSN is hardcoded — consider moving it to an environment variable or config constant.While Sentry DSNs are not secret (they're client-side keys), externalizing it would make it easier to rotate or differ across environments without a code change.
| sentryWebpackPlugin({ | ||
| org: "requestly", | ||
| project: "desktop-app", | ||
| authToken: process.env.SENTRY_AUTH_TOKEN, | ||
|
|
||
| sourcemaps: { | ||
| filesToDeleteAfterUpload: ["**/*.js.map"] | ||
| } | ||
| }), |
There was a problem hiding this comment.
Sentry plugin is unconditionally added — builds will fail without SENTRY_AUTH_TOKEN.
If SENTRY_AUTH_TOKEN is not set (e.g., local production builds, CI without secrets), the Sentry webpack plugin will fail the build when it tries to upload source maps. Consider conditionally adding the plugin only when the token is available.
🛠️ Proposed fix
Replace the static plugin entry with a conditional inclusion. For example, define a helper array before the config:
+const optionalPlugins = [];
+if (process.env.SENTRY_AUTH_TOKEN) {
+ optionalPlugins.push(
+ sentryWebpackPlugin({
+ org: "requestly",
+ project: "desktop-app",
+ authToken: process.env.SENTRY_AUTH_TOKEN,
+ sourcemaps: {
+ filesToDeleteAfterUpload: ["**/*.js.map"],
+ },
+ })
+ );
+}Then spread ...optionalPlugins into the plugins array instead of the inline sentryWebpackPlugin(...) call.
🤖 Prompt for AI Agents
In @.erb/configs/webpack.config.renderer.prod.ts around lines 176 - 184, The
Sentry plugin (sentryWebpackPlugin) is always added to the webpack plugins array
which causes builds to fail when process.env.SENTRY_AUTH_TOKEN is missing;
update the config to conditionally include the plugin only when
SENTRY_AUTH_TOKEN exists by creating an optionalPlugins array (or similar) that
pushes/contains sentryWebpackPlugin(...) only if process.env.SENTRY_AUTH_TOKEN
is truthy, then spread ...optionalPlugins into the plugins array; ensure you
still pass the sourcemaps/filesToDeleteAfterUpload options when the plugin is
included.
There was a problem hiding this comment.
@nsrCodes this will be taken care in github CI
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/sentryInit.js`:
- Line 25: The Sentry configuration currently hardcodes tracesSampleRate: 1.0
which sends 100% of traces; change it to read a configurable environment
variable and apply a safe default and bounds check: read something like
process.env.SENTRY_TRACES_SAMPLE_RATE (parseFloat), default to a lower
production-friendly value (e.g., 0.05), and clamp to a sensible range (0.0–1.0)
before assigning to tracesSampleRate in the Sentry init (the tracesSampleRate
property in the Sentry initialization code in sentryInit.js); ensure non-numeric
or out-of-range values fall back to the default.
🧹 Nitpick comments (3)
src/utils/sentryInit.js (3)
26-27: Session Replay options are only meaningful in the renderer process.
replaysSessionSampleRateandreplaysOnErrorSampleRateapply to Session Replay, which is a browser/renderer-only feature. When this config is used to initialize@sentry/electron/main, these options are silently ignored. Consider gating them to the renderer path to avoid confusion.Proposed fix
tracesSampleRate: process.env.NODE_ENV === "development" ? 1.0 : 0.1, - replaysSessionSampleRate: 0.1, - replaysOnErrorSampleRate: 1.0, + // Session Replay is only supported in the renderer process + ...(isMain + ? {} + : { + replaysSessionSampleRate: 0.1, + replaysOnErrorSampleRate: 1.0, + }),
29-32: No-opintegrationscallback with a misleading comment — remove it.The callback returns integrations unchanged, so it has no effect. The comment "Capture unhandled promise rejections" is misleading since this callback doesn't add any integration. Remove both to reduce noise.
Proposed fix
- // Capture unhandled promise rejections - integrations: (integrations) => { - return integrations; - },
44-57:appVersionmay benull, resulting in anulltag sent to Sentry.When
app.getVersion()isn't available orpackage.jsondoesn't have a version,appVersionstaysnulland is still assigned toevent.tags.app_version. Consider only setting the tag when the value is truthy.Proposed fix
- event.tags.app_version = appVersion; + if (appVersion) { + event.tags.app_version = appVersion; + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/renderer/index.js`:
- Line 40: The file declares logger twice: remove the duplicate import statement
`import logger from "utils/logger";` (the second declaration) so the module uses
the existing `import logger from "../utils/logger"` binding; ensure no other
references rely on the removed path and keep only the original `logger` import
used throughout the file (e.g., calls to logger.debug/info/error remain
unchanged).
🧹 Nitpick comments (2)
src/renderer/index.js (2)
3-3: UnusedSentryimport — remove it.After removing all
Sentry.captureExceptioncalls (per prior review),Sentryis no longer referenced anywhere in this file. The side-effect import on line 2 (import "../utils/sentryInit") already handles initialization.♻️ Proposed fix
// Initialize Sentry for background renderer (must be first) import "../utils/sentryInit"; -import * as Sentry from "@sentry/electron/renderer"; import logger from "../utils/logger";
51-51: Remove stale commented-out import.This commented-out
sentryInitimport is now redundant since the active import lives on line 2.
* chore: add captureException * cleanups
…d field (#287) * fix: make validation more verbose * fix: error logging * improved naming
2 participants
Summary by CodeRabbit
Screen.Recording.2026-02-10.at.2.09.09.PM.mov