[codex] Add opt-in Sentry error monitoring#36
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR implements comprehensive error reporting across the Electron app using Sentry. It adds persistent user-configurable error reporting settings, initializes Sentry on both main and renderer processes with data sanitization, bridges settings through IPC, adds a Privacy settings tab, and includes build configuration, release automation, and telemetry documentation. ChangesError Monitoring Infrastructure and UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
🧹 Nitpick comments (4)
src/windows/SettingsWindow.tsx (1)
23-23: ⚡ Quick winDerive tab validation from
TABSto avoid drift.The valid-tab list is hardcoded inline in both
getInitialTab(Line 23) and theonSwitchTabhandler (Line 52). Adding/renaming a tab now requires editing three places (TABS, both inline checks), which is easy to get wrong.♻️ Proposed helper derived from `TABS`
const TABS: { id: TabId; label: string; icon: typeof KeyRound }[] = [ { id: 'api-keys', label: 'API Keys', icon: KeyRound }, { id: 'shortcuts', label: 'Shortcuts', icon: Keyboard }, { id: 'appearance', label: 'Appearance', icon: Palette }, { id: 'privacy', label: 'Privacy', icon: ShieldCheck }, ] + +function isTabId(value: string | null): value is TabId { + return TABS.some(tab => tab.id === value) +}function getInitialTab(): TabId { const params = new URLSearchParams(window.location.search) const tab = params.get('tab') - if (tab === 'shortcuts' || tab === 'api-keys' || tab === 'appearance' || tab === 'privacy') { + if (isTabId(tab)) { return tab } return 'api-keys' }const unsubscribe = window.electronAPI.settings.onSwitchTab((tab) => { - if (tab === 'shortcuts' || tab === 'api-keys' || tab === 'appearance' || tab === 'privacy') { + if (isTabId(tab)) { setActiveTab(tab) } })Also applies to: 52-52
🤖 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 `@src/windows/SettingsWindow.tsx` at line 23, The tab-validation logic is duplicated and hardcoded; update getInitialTab and the onSwitchTab handler to derive the valid-tabs check from the existing TABS constant instead of the inline string list. Replace the current conditional (tab === 'shortcuts' || ...) with a membership test using TABS (e.g., TABS.includes(tab) or TABSSet.has(tab)) so both getInitialTab and onSwitchTab validate against the single source of truth TABS and cannot drift when tabs are added/renamed.src/components/ErrorBoundary.tsx (1)
17-21: ⚡ Quick winComponent stack is reduced too aggressively for effective debugging.
Line 19 reduces
info.componentStackto just'present'or'missing'to avoid sending potentially sensitive data. However, the component stack shows the React component tree path and is critical for debugging React errors—it's not PII and won't contain user data. Meanwhile, theerrorobject itself (line 18) is sent with its full stack trace, which would contain file paths and code context anyway.Consider sending the actual
componentStackvalue (or at least a sanitized/truncated version) to preserve debugging utility while the main-process sanitization (inelectron/error-monitoring.ts) already handles sensitive data redaction.📊 Proposed fix to preserve component stack
componentDidCatch(error: Error, info: React.ErrorInfo): void { captureRendererError(error, { - componentStack: info.componentStack ? 'present' : 'missing', + hasComponentStack: info.componentStack ? 'true' : 'false', }) + // Let Sentry capture the full error with componentStack for debugging + if (info.componentStack) { + console.error('Component stack:', info.componentStack) + } }Or, to send the component stack as a tag (truncated):
componentDidCatch(error: Error, info: React.ErrorInfo): void { + const context: Record<string, string> = {} + if (info.componentStack) { + // Truncate to first 500 chars to avoid huge payloads + context.componentStack = info.componentStack.slice(0, 500) + } - captureRendererError(error, { - componentStack: info.componentStack ? 'present' : 'missing', - }) + captureRendererError(error, context) }🤖 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 `@src/components/ErrorBoundary.tsx` around lines 17 - 21, The componentDidCatch implementation currently replaces info.componentStack with 'present'/'missing', stripping valuable debugging context; update componentDidCatch (the ErrorBoundary method) to pass the real component stack to captureRendererError instead — either pass info.componentStack directly or a sanitized/truncated substring (e.g., first N chars or lines) before calling captureRendererError so the component tree is preserved for debugging while relying on electron/error-monitoring sanitization for PII; locate captureRendererError and componentDidCatch in ErrorBoundary.tsx to apply this change.electron/error-monitoring.ts (2)
18-40: ⚡ Quick winConsider adding depth limit to recursive sanitization.
The
sanitizeValuefunction recursively processes nested objects without a depth limit. While object breadth is capped at 30 entries (line 32) and arrays at 10 items (line 28), deeply nested structures could still cause stack overflow.🛡️ Proposed fix to add depth guard
-function sanitizeValue(value: unknown, key = ''): unknown { +function sanitizeValue(value: unknown, key = '', depth = 0): unknown { + if (depth > 20) { + return '[max-depth-exceeded]' + } + if (SENSITIVE_KEY_PATTERN.test(key)) { return '[redacted]' } if (typeof value === 'string') { return redactString(value) } if (Array.isArray(value)) { - return value.slice(0, 10).map((item) => sanitizeValue(item)) + return value.slice(0, 10).map((item) => sanitizeValue(item, '', depth + 1)) } if (value && typeof value === 'object') { const entries = Object.entries(value as Record<string, unknown>).slice(0, 30) return Object.fromEntries(entries.map(([entryKey, entryValue]) => [ entryKey, - sanitizeValue(entryValue, entryKey), + sanitizeValue(entryValue, entryKey, depth + 1), ])) } return value }🤖 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 `@electron/error-monitoring.ts` around lines 18 - 40, The sanitizeValue function can recurse indefinitely and needs a depth guard: add an optional depth parameter (e.g., depth = 0) and a MAX_SANITIZE_DEPTH constant, check at the top of sanitizeValue and return a safe placeholder (or the original primitive) when depth >= MAX_SANITIZE_DEPTH to stop recursion; then increment depth when calling sanitizeValue recursively for arrays and object entries (map items and sanitizeValue(entryValue, entryKey, depth + 1)), and ensure SENSITIVE_KEY_PATTERN and redactString usage remain unchanged.
23-25: 💤 Low valueString redaction may be too aggressive for non-sensitive values.
Every string value is passed through
redactString(line 24), which truncates to 500 characters and redacts URLs/long tokens—even for non-sensitive strings like error messages or safe metadata. This could reduce the debugging value of error reports.Consider applying
redactStringonly when the key matches sensitive patterns, or skip truncation for error messages and stack traces that are inherently useful for debugging.🤖 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 `@electron/error-monitoring.ts` around lines 23 - 25, The current branch always passes every string through redactString (the if (typeof value === 'string') return redactString(value) path), which over-redacts; change this to conditionally redact based on the key and context: accept a key parameter (or use the existing key variable) and only call redactString for sensitive keys (match via a sensitivePatterns regex or list like /password|token|secret|apiKey/i), while returning the original string for safe keys such as "message", "error", "stack", or other metadata; for stack/message still optionally sanitize long URLs but avoid truncating the whole string (i.e., use a lighter sanitizer or skip truncation), and update references to redactString usage accordingly so non-sensitive error messages remain intact.
🤖 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.
Nitpick comments:
In `@electron/error-monitoring.ts`:
- Around line 18-40: The sanitizeValue function can recurse indefinitely and
needs a depth guard: add an optional depth parameter (e.g., depth = 0) and a
MAX_SANITIZE_DEPTH constant, check at the top of sanitizeValue and return a safe
placeholder (or the original primitive) when depth >= MAX_SANITIZE_DEPTH to stop
recursion; then increment depth when calling sanitizeValue recursively for
arrays and object entries (map items and sanitizeValue(entryValue, entryKey,
depth + 1)), and ensure SENSITIVE_KEY_PATTERN and redactString usage remain
unchanged.
- Around line 23-25: The current branch always passes every string through
redactString (the if (typeof value === 'string') return redactString(value)
path), which over-redacts; change this to conditionally redact based on the key
and context: accept a key parameter (or use the existing key variable) and only
call redactString for sensitive keys (match via a sensitivePatterns regex or
list like /password|token|secret|apiKey/i), while returning the original string
for safe keys such as "message", "error", "stack", or other metadata; for
stack/message still optionally sanitize long URLs but avoid truncating the whole
string (i.e., use a lighter sanitizer or skip truncation), and update references
to redactString usage accordingly so non-sensitive error messages remain intact.
In `@src/components/ErrorBoundary.tsx`:
- Around line 17-21: The componentDidCatch implementation currently replaces
info.componentStack with 'present'/'missing', stripping valuable debugging
context; update componentDidCatch (the ErrorBoundary method) to pass the real
component stack to captureRendererError instead — either pass
info.componentStack directly or a sanitized/truncated substring (e.g., first N
chars or lines) before calling captureRendererError so the component tree is
preserved for debugging while relying on electron/error-monitoring sanitization
for PII; locate captureRendererError and componentDidCatch in ErrorBoundary.tsx
to apply this change.
In `@src/windows/SettingsWindow.tsx`:
- Line 23: The tab-validation logic is duplicated and hardcoded; update
getInitialTab and the onSwitchTab handler to derive the valid-tabs check from
the existing TABS constant instead of the inline string list. Replace the
current conditional (tab === 'shortcuts' || ...) with a membership test using
TABS (e.g., TABS.includes(tab) or TABSSet.has(tab)) so both getInitialTab and
onSwitchTab validate against the single source of truth TABS and cannot drift
when tabs are added/renamed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc23014d-dfda-4206-b652-1defabbe34d0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
README.mddocs/TELEMETRY.mdelectron/error-monitoring.tselectron/ipc-contract.tselectron/main.tselectron/preload.tselectron/settings-store.tspackage.jsonsrc/components/ErrorBoundary.tsxsrc/components/layout/DeveloperMessageDialog.tsxsrc/components/layout/TopBar.tsxsrc/error-monitoring.tssrc/main.tsxsrc/windows/SettingsWindow.tsxvite.config.js
stepandel
marked this pull request as ready for review
May 31, 2026 04:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
New Features
Documentation
Chores