feat: add Sentry integration to simulator #676
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdded Sentry for browser-based error monitoring with environment-aware configuration. Created a new sentry.js module that initializes Sentry early in app.js startup. Exposed Array helper class globally and reformatted data structure declarations with minor syntax consistency adjustments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/simulator/src/app.js (1)
1-6: Sentry import and early initialization look good; consider a small guard and config wiringCalling
initSentry()at module load (Lines 1–6) is a solid choice to capture errors as early as possible in the browser. Two follow‑ups worth considering:
- If
src/simulator/src/app.jsis ever imported in a non‑browser context (SSR, unit tests withoutwindow),window.locationinsideinitSentrywill throw. A minimal guard keeps this safe:-// Initialize Sentry first -initSentry(); +// Initialize Sentry first (only in a browser runtime) +if (typeof window !== "undefined") { + initSentry(); +}
- Long‑term, you may want the DSN and environment to come from your build/config system rather than being hard‑coded here, so you don’t need code changes for env switches.
Please double‑check that the versions of
@sentry/browserand@sentry/tracingin yourpackage.jsonmatch the APIs used insrc/simulator/src/sentry.jsand thatapp.jsis not imported from any non‑browser test/SSR entrypoint. If needed, you can verify usage with a repo‑wide search forfrom "./app"andfrom "./sentry".src/simulator/src/sentry.js (1)
1-39: Sentry setup is broadly correct; refine env detection and sampling for robustness and cost controlThe
initSentryimplementation (Lines 1–39) is a solid baseline: imports are correct,BrowserTracingis wired intointegrations, environment is set fromisLocalDev, andbeforeSendcleanly blocks events in local dev while logging them for debugging. A couple of refinements to consider:
Environment detection:
isLocalDevonly checks"localhost"and"127.0.0.1". If you run dev builds on other hosts (e.g.,0.0.0.0, a LAN IP, ordev.circuitverse.org), they’ll be treated as"production"and send events. Consider drivingenvironmentand the “dev vs prod” decision from your build configuration (e.g., a Vite/webpack env flag) and/or widening the hostname check to match your actual dev domains.Trace sampling in production:
tracesSampleRate: 1.0will capture 100% of transactions in production, which can be noisy and expensive at scale. If simulator traffic is non‑trivial, you might want to lower this or make it environment‑dependent (e.g., 1.0 in dev/staging, smaller fraction in production).Config location: The DSN and environment string literals here work, but wiring them through env variables or a centralized config makes it easier to rotate DSNs or add staging environments without code changes.
Please confirm that:
- The deployed environments and dev hosts match the
isLocalDevassumptions, and- The installed versions of
@sentry/browserand@sentry/tracingsupport this initialization shape (especiallyBrowserTracingandtracesSampleRate).If anything differs, adjust the env detection or sampling values accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/app.js(1 hunks)src/simulator/src/sentry.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/app.js (1)
src/simulator/src/sentry.js (1)
initSentry(4-39)
🪛 Biome (2.1.2)
src/simulator/src/app.js
[error] 2-2: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (1)
src/simulator/src/app.js (1)
12-54: Demo simulation JSON reformatting is fineThe
jsdemo simulation structure (Lines 12–54) is a pure data declaration; the reformatting into a more JSON‑like style doesn’t change behavior and remains readable. No issues from a logic or runtime perspective.
| @@ -1,210 +1,58 @@ | |||
| import { setup } from './setup' | |||
| import { setup } from './setup'; | |||
| import Array from './arrayHelpers'; | |||
There was a problem hiding this comment.
Overriding the native Array constructor on window is risky and should be avoided
Importing Array from ./arrayHelpers (Line 2) and then doing window.Array = Array; (Line 57) replaces the built‑in window.Array with your custom implementation. This:
- Conflicts with Biome’s
noShadowRestrictedNameswarning. - Can subtly break any code (including libraries) that relies on native
Arraysemantics,instanceof Array,Array.isArray, or static methods added by the JS engine.
A safer pattern is to avoid shadowing the global and expose the helper under a distinct name:
-import Array from './arrayHelpers';
+import ArrayHelpers from './arrayHelpers';
@@
- // Make array helpers globally available
- window.Array = Array;
+ // Make array helpers globally available without overriding native Array
+ window.ArrayHelpers = ArrayHelpers;If existing simulator code expects a global helper, align on a non‑conflicting name (window.SimulatorArray, window.ArrayHelpers, etc.) rather than reusing Array.
Also applies to: 56-57
🧰 Tools
🪛 Biome (2.1.2)
[error] 2-2: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In src/simulator/src/app.js around lines 2 and 56-57, the file imports a module
as "Array" and assigns it to window.Array which overrides the native global
Array; change the import or the global export so you do NOT shadow the built-in
(for example import as ArrayHelpers or SimulatorArray and assign
window.SimulatorArray = ArrayHelpers), then update any local references that
expect the helper to use the new name (or update code that relied on
window.Array to use window.SimulatorArray/ArrayHelpers) and remove the
assignment to window.Array to avoid breaking native Array semantics.
1 participant
Fixes #
Describe the changes you have made in this PR -
sentry.jsto initialize Sentry and capture runtime errors.Screenshots of the changes (If any) -
N/A
Additional notes
localhost:3000.Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
Release Notes
New Features
Chores