Sentry

[Jordonbc]

No description provided.

[opencode-agent]

Code Review Summary

Verdict: Good to merge with minor improvements

The Sentry integration is well-implemented with proper consent gating. Here are my findings:

---

Critical Issues

1. env!("OPENVCS_VERSION") compile-time panic (Backend/src/monitoring.rs:103)

let release = format!("openvcs-client@{}", env!("OPENVCS_VERSION"));

This panics if OPENVCS_VERSION isn't set at compile time. The build.rs properly exports it, but a local build without the build script running could fail. Consider using option_env!("OPENVCS_VERSION") with a fallback.

---

Potential Bugs

2. Race condition: monitoring init before settings load (Frontend/src/scripts/main.ts:103)

await syncFrontendMonitoring(cfg);

If loadInitialGlobalSettings() fails and returns null, syncFrontendMonitoring(null) is called which correctly does nothing—but if a subsequent call later passes valid settings, the monitoring state may be stale since monitoringEnabled is module-level and only gets set to true once.

3. Silent error swallowing (Frontend/src/scripts/lib/monitoring.ts:167)

await TAURI.invoke('report_frontend_error', { payload }).catch(() => {});

Errors are silently swallowed. Consider logging at debug level to avoid losing error visibility during debugging.

4. Missing re-sync on settings change (Frontend/src/scripts/features/settings.ts:695, 768)

await syncFrontendMonitoring(next);

Settings modal save/reset calls sync correctly. This is good.

---

Code Quality Suggestions

5. Missing doc comment (Backend/src/monitoring.rs:180)
capture_frontend_error lacks a doc comment explaining when frontend errors are relayed vs dropped.

6. Frontend breadcrumb cap boundary test missing (Frontend/src/scripts/lib/monitoring.test.ts)
The test file exists but should cover the MAX_FRONTEND_BREADCRUMBS + 1 boundary case.

7. Inconsistent error level mapping (Backend/src/monitoring.rs:330)
Unknown levels map to Debug which may hide important events. Consider Warning or Info as default.

---

Strengths

• Clean separation between frontend relay and backend Sentry client
• Proper consent gating via crash_reports setting
• Good stacktrace parsing with multiple format support
• Graceful degradation when DSN isn't configured
• Comprehensive frontend tests
• Build-time fallback embedding in build.rs

opencode session  |  github run

[opencode-agent]

Code Review: Sentry Integration PR

Summary

The Sentry integration is well-implemented with proper consent gating. Here's my analysis:

---

Critical Issues

1. env!("OPENVCS_VERSION") compile-time panic (Backend/src/monitoring.rs:102)

let release = format!("openvcs-client@{}", env!("OPENVCS_VERSION"));

This panics at compile time if OPENVCS_VERSION isn't set. While build.rs (lines 397) always exports it, direct cargo build without running the full build pipeline could fail. Use option_env! with a fallback instead.

---

Potential Bugs

2. Race condition: monitoring init before settings load (Frontend/src/scripts/main.ts:103)

await syncFrontendMonitoring(cfg);

If loadInitialGlobalSettings() fails and returns null, monitoring won't initialize. However, this is partially mitigated by settings modal re-syncing on save/reset.

3. Silent error swallowing (Frontend/src/scripts/lib/monitoring.ts:167)

await TAURI.invoke('report_frontend_error', { payload }).catch(() => {});

Errors are silently discarded. Add debug-level logging for visibility during development.

---

Code Quality Issues

4. Missing doc comment (Backend/src/monitoring.rs:179)
capture_frontend_error lacks documentation explaining when frontend errors are relayed vs dropped.

5. Inconsistent error level mapping (Backend/src/monitoring.rs:330)
Unknown levels map to Debug, which may hide important events. Consider Warning or Info.

6. Breadcrumb cap boundary test exists - The previous review was incorrect here. The test at monitoring.test.ts:64-77 covers exactly 45 breadcrumbs (40 + 5 overflow), testing the cap correctly.

---

Strengths

• Clean separation between frontend relay and backend Sentry client
• Proper consent gating via crash_reports setting
• Good stacktrace parsing with multiple format support (@ and () notation)
• Graceful degradation when DSN isn't configured
• Comprehensive frontend tests
• Build-time fallback embedding in build.rs:399-404
• Idempotent sync operations prevent double-listeners

---

Minor Suggestions

1. Add a test for parse_frontend_location edge case with malformed URLs
2. Consider adding debug level breadcrumb support to match Sentry's Debug level
3. Document the DSN configuration in SECURITY.md or a settings-related doc

Verdict: Good to merge with the critical issue addressed.

opencode session  |  github run

[opencode-agent]

Code Review: Sentry Integration PR

Summary

Well-implemented Sentry integration with proper consent gating. Good to merge with the issues below addressed.

---

Critical Issues

1. env!("OPENVCS_VERSION") compile-time panic (Backend/src/monitoring.rs:102)

let release = format!("openvcs-client@{}", env!("OPENVCS_VERSION"));

This panics if OPENVCS_VERSION isn't set at compile time. Unlike build.rs:397 which exports it via rustc-env, direct cargo build without running the full build pipeline will fail. The same pattern exists in utilities.rs:30 and inner.rs:26.

Fix - use option_env!("OPENVCS_VERSION") with a fallback:

let release = format!(
    "openvcs-client@{}",
    option_env!("OPENVCS_VERSION").unwrap_or("unknown")
);

---

Potential Bugs

2. Race condition: monitoring init before settings load (Frontend/src/scripts/main.ts:102-103)

const cfg = await loadInitialGlobalSettings();
await syncFrontendMonitoring(cfg);

If loadInitialGlobalSettings() fails and returns null, monitoring won't initialize. However, the settings modal re-syncs on save (settings.ts:695) and reset (settings.ts:768), which mitigates this. Consider adding a retry or warning in console when settings fail to load.

3. Silent error swallowing (Frontend/src/scripts/lib/monitoring.ts:167)

await TAURI.invoke('report_frontend_error', { payload }).catch(() => {});

Errors are silently discarded with empty catch. Add debug-level logging:

.catch((e) => console.debug('Failed to relay frontend error:', e));

---

Code Quality Issues

4. Missing doc comment (Backend/src/monitoring.rs:179)
capture_frontend_error lacks a doc comment explaining when/why frontend errors are relayed vs dropped.

5. Inconsistent error level mapping (Backend/src/monitoring.rs:329)
Unknown frontend levels map to Debug, which may hide important events in Sentry. Consider Warning or Info as default.

6. Type narrowing in error listener (Frontend/src/scripts/lib/monitoring.ts:114)

line: typeof event.lineno === 'number' ? event.lineno : null,

The ErrorEvent type already has lineno as optional number. Could simplify to event.lineno ?? null.

---

Observations

• The breadcrumb cap test (monitoring.test.ts:64-77) correctly tests boundary with exactly 45 items → 40 retained ✓
• Idempotent sync (monitoring.ts:80-82) prevents duplicate listeners ✓
• Proper consent gating via crash_reports setting in settings modal and settingsGeneral.ts ✓
• Good stacktrace parsing supporting both @ and () notation (monitoring.rs:267-277) ✓

---

Suggestions

1. Add debug level to frontend MonitoringBreadcrumbLevel type to match Sentry's level
2. Document the DSN configuration in SECURITY.md or settings help text
3. Add boundary test for parse_frontend_location with malformed URLs (e.g., "file.js:abc")

---

Verdict: Good to merge with critical issue #1 addressed.

opencode session  |  github run