-
Notifications
You must be signed in to change notification settings - Fork 6
Feat: add sentry to web-app #1472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
For privacy concerns I'd aim just to log browser and time of action, no other user data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds Sentry error tracking integration to the web-app for production monitoring. It configures Sentry to capture unhandled errors with session replay, user actions, store state, and basic telemetry.
Key Changes:
- Integrated Sentry SDK with React Router v6 instrumentation and session replay capabilities
- Added Redux state sanitization to prevent sensitive data leakage in error reports
- Created a custom error fallback UI component for better user experience during crashes
- Configured GitHub Actions workflows to inject Sentry credentials only for production deployments
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| web-app/src/sentry.ts | New Sentry initialization module with environment-based configuration and React Router integration |
| web-app/src/index.tsx | Wraps app in SentryErrorBoundary with custom fallback component |
| web-app/src/app/store/store.ts | Adds Redux enhancer for Sentry with state sanitization to limit captured data |
| web-app/src/app/components/SentryErrorFallback.tsx | New error fallback UI component with collapsible error details |
| web-app/package.json | Adds @sentry/react dependency (v10.26.0) |
| web-app/yarn.lock | Lock file updates for Sentry packages and transitive dependencies |
| .github/workflows/web-prod.yml | Adds Sentry DSN secret reference for production deployment |
| .github/workflows/web-app-deployer.yml | Configures Sentry environment variables and sample rates in deployment pipeline |
| /* eslint-disable */ | ||
| const makeStore = () => | ||
| configureStore({ | ||
| reducer: persistedReducer, | ||
| devTools: process.env.NODE_ENV !== 'production', | ||
| middleware: (getDefaultMiddleware) => [ | ||
| ...getDefaultMiddleware({ | ||
| serializableCheck: { | ||
| ignoredActions: [FLUSH, REHYDRATE, PAUSE, PERSIST, PURGE, REGISTER], | ||
| }, | ||
| }), | ||
| sagaMiddleware, | ||
| ], | ||
| enhancers: (existing) => | ||
| sentryReduxEnhancer ? [...existing, sentryReduxEnhancer] : existing, | ||
| }); | ||
| /* eslint-enable */ |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment disables eslint for the entire makeStore function, but it's unclear what rule is being violated. Consider using a more specific eslint disable comment (e.g., eslint-disable-next-line rule-name) or fixing the underlying issue instead of disabling linting for the entire block.
web-app/src/sentry.ts
Outdated
| const routerTracingIntegration = | ||
| (Sentry as any).reactRouterV6BrowserTracingIntegration?.({ | ||
| useEffect: React.useEffect, | ||
| reactRouterV6: { createRoutesFromChildren, matchRoutes }, | ||
| }) || | ||
| (Sentry as any).browserTracingIntegration?.({ | ||
| routingInstrumentation: (Sentry as any).reactRouterV6Instrumentation?.( | ||
| React.useEffect, | ||
| true, | ||
| createRoutesFromChildren, | ||
| matchRoutes, | ||
| ), | ||
| }); | ||
|
|
||
| Sentry.init({ | ||
| dsn, | ||
| environment, | ||
| release, | ||
| integrations: [ | ||
| routerTracingIntegration, | ||
| (Sentry as any).replayIntegration?.(), | ||
| ].filter(Boolean), |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using type assertions with (Sentry as any) throughout this file bypasses TypeScript's type checking. This is risky because:
- It assumes these methods exist at runtime without compile-time verification
- If the Sentry API changes, TypeScript won't catch the breaking changes
- It makes the code harder to maintain
Consider importing the specific integration functions directly from @sentry/react or using proper type guards to check for method existence before calling them.
| if (dsn) { | ||
| // Prefer dedicated react-router v6 integration if available, else fall back to generic browser tracing with manual routing instrumentation. | ||
| const routerTracingIntegration = | ||
| (Sentry as any).reactRouterV6BrowserTracingIntegration?.({ | ||
| useEffect: React.useEffect, | ||
| reactRouterV6: { createRoutesFromChildren, matchRoutes }, | ||
| }) || | ||
| (Sentry as any).browserTracingIntegration?.({ | ||
| routingInstrumentation: (Sentry as any).reactRouterV6Instrumentation?.( | ||
| React.useEffect, | ||
| true, | ||
| createRoutesFromChildren, | ||
| matchRoutes, | ||
| ), | ||
| }); | ||
|
|
||
| Sentry.init({ | ||
| dsn, | ||
| environment, | ||
| release, | ||
| integrations: [ | ||
| routerTracingIntegration, | ||
| (Sentry as any).replayIntegration?.(), | ||
| ].filter(Boolean), | ||
| tracesSampleRate, | ||
| replaysSessionSampleRate, | ||
| replaysOnErrorSampleRate, | ||
| ignoreErrors: [/ResizeObserver loop limit exceeded/i], | ||
| }); | ||
| } |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the PR description, Sentry is "only setup for PROD", but the workflow changes show that Sentry configuration (DSN and sample rates) is being set for the production deployment. However, the sentry.ts file will attempt to initialize Sentry whenever a DSN is present, regardless of environment. If the DSN is accidentally set in development or staging environments, Sentry will be active there too. Consider adding an explicit environment check to ensure Sentry only runs in production:
const isProduction = process.env.REACT_APP_FIREBASE_PROJECT_ID === 'mobility-feeds-prod';
if (dsn && isProduction) {
// Initialize Sentry
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ok, opens the door if we want to have a sentry QA project
| OP_SENTRY_DSN: | ||
| description: 1Password Sentry DSN secret reference | ||
| type: string | ||
| required: false |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The OP_SENTRY_DSN input parameter is marked as required: false, but in production it's expected to be provided. Consider making this required or adding validation in the workflow to ensure it's provided for production deployments. Without the DSN, Sentry won't function, which could lead to silent failures in error tracking.
| required: false | |
| required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine due to the qa and dev environment not requiring it
web-app/src/app/store/store.ts
Outdated
| sagaMiddleware.run(rootSaga); | ||
|
|
||
| export type RootState = ReturnType<typeof store.getState>; | ||
| export type RootState = ReturnType<typeof persistedReducer>; |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing RootState from ReturnType<typeof store.getState> to ReturnType<typeof persistedReducer> is a breaking change that could affect type inference. The former provides the actual runtime state type from the configured store (which includes middleware effects), while the latter only provides the reducer's return type. This might cause type mismatches in selectors or components that rely on the full store state type.
| export type RootState = ReturnType<typeof persistedReducer>; | |
| export type RootState = ReturnType<typeof store.getState>; |
web-app/src/sentry.ts
Outdated
|
|
||
| const dsn = process.env.REACT_APP_SENTRY_DSN || ''; | ||
| const environment = | ||
| process.env.REACT_APP_FIREBASE_PROJECT_ID || process.env.NODE_ENV || "mobility-feeds-dev"; |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The fallback environment value "mobility-feeds-dev" is a hardcoded string that doesn't match the expected environment naming. If both REACT_APP_FIREBASE_PROJECT_ID and NODE_ENV are undefined (which could happen in certain build scenarios), this arbitrary default will be used. Consider using a more meaningful default like "development" or "unknown" to better align with standard environment naming conventions.
| process.env.REACT_APP_FIREBASE_PROJECT_ID || process.env.NODE_ENV || "mobility-feeds-dev"; | |
| process.env.REACT_APP_FIREBASE_PROJECT_ID || process.env.NODE_ENV || "development"; |
web-app/src/sentry.ts
Outdated
| integrations: [ | ||
| routerTracingIntegration, | ||
| (Sentry as any).replayIntegration?.(), | ||
| ].filter(Boolean), |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The filter(Boolean) call on line 43 is used to remove falsy values from the integrations array. However, since both routerTracingIntegration and replayIntegration are already conditionally assigned (with fallback to undefined for routerTracingIntegration and potentially undefined for replayIntegration if it doesn't exist), this pattern works. However, it would be clearer to build the array conditionally:
const integrations = [];
if (routerTracingIntegration) {
integrations.push(routerTracingIntegration);
}
const replayIntegration = (Sentry as any).replayIntegration?.();
if (replayIntegration) {
integrations.push(replayIntegration);
}This makes the intent more explicit and doesn't rely on filter(Boolean) to clean up the array.
| integrations: [ | |
| routerTracingIntegration, | |
| (Sentry as any).replayIntegration?.(), | |
| ].filter(Boolean), | |
| // Build integrations array explicitly for clarity | |
| integrations: (() => { | |
| const integrations = []; | |
| if (routerTracingIntegration) { | |
| integrations.push(routerTracingIntegration); | |
| } | |
| const replayIntegration = (Sentry as any).replayIntegration?.(); | |
| if (replayIntegration) { | |
| integrations.push(replayIntegration); | |
| } | |
| return integrations; | |
| })(), |
| echo "REACT_APP_SENTRY_DSN=${{ env.SENTRY_DSN }}" >> $GITHUB_ENV | ||
| echo "REACT_APP_SENTRY_REPLAY_SESSION_SAMPLE_RATE=0.1" >> $GITHUB_ENV | ||
| echo "REACT_APP_SENTRY_REPLAY_ERROR_SAMPLE_RATE=0.1" >> $GITHUB_ENV | ||
| echo "REACT_APP_SENTRY_TRACES_SAMPLE_RATE=0.05" >> $GITHUB_ENV |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The sample rates for Sentry are only configured in the production environment (within the if block), but they're not set for the dev environment (in the else block). If Sentry DSN is somehow provided in development, it will use the default values from sentry.ts. For consistency and to avoid confusion, consider explicitly setting these values in both environments or documenting why they're production-only.
| const formatError = (error: unknown): string => { | ||
| if (error instanceof Error) { | ||
| return `${error.message}\n${error.stack}`; | ||
| } | ||
| try { | ||
| return typeof error === 'string' ? error : JSON.stringify(error, null, 2); | ||
| } catch { | ||
| return String(error); | ||
| } | ||
| }; |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error formatting logic in formatError doesn't handle circular references in objects. If the error object contains circular references, JSON.stringify(error, null, 2) will throw a TypeError. Consider using a safe stringify utility or adding a replacer function to handle circular references:
const formatError = (error: unknown): string => {
if (error instanceof Error) {
return `${error.message}\n${error.stack}`;
}
try {
const seen = new WeakSet();
return typeof error === 'string' ? error : JSON.stringify(error, (key, value) => {
if (typeof value === 'object' && value !== null) {
if (seen.has(value)) {
return '[Circular]';
}
seen.add(value);
}
return value;
}, 2);
} catch {
return String(error);
}
};2e4513d to
f7e02d1
Compare
|
@emmambd it does a great job at keeping privacy, this is what the dashboard looks like. Sentry logs as anonymous user and even during the replay, all the text is modified to prevent user identification. I can look into removing geography
|
| beforeSend(event) { | ||
| // remove user IP and geo context | ||
| if (event.user) { | ||
| delete event.user.ip_address; | ||
| } | ||
| if (event.contexts && event.contexts.geo) { | ||
| delete event.contexts.geo; | ||
| } | ||
| return event; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emmambd removed the collection of user IP and geography
|
*Lighthouse ran on https://mobility-feeds-dev--pr-1472-ol9k7s34.web.app/ * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1472-ol9k7s34.web.app/feeds * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1472-ol9k7s34.web.app/feeds/gtfs/mdb-2126 * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1472-ol9k7s34.web.app/feeds/gtfs_rt/mdb-2585 * (Desktop)
*Lighthouse ran on https://mobility-feeds-dev--pr-1472-ol9k7s34.web.app/gbfs/gbfs-flamingo_porirua * (Desktop)
|
|
Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-1472-ol9k7s34.web.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this script was modified to accept optional_variables as an optional parameter. This script is used in a few place, these modifications shouldn't affect anything. Optional parameters are used to set env variables for env specific (ex: Sentry just for PROD)
| type: string | ||
| required: false | ||
| OP_SENTRY_DSN: | ||
| description: 1Password Sentry DSN secret reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really really nitpicky:
| description: 1Password Sentry DSN secret reference | |
| description: reference to the 1Password Sentry DSN secret |
My first reaction when I saw this was: But that reference is not a secret! Then I realized that that the string "1Password Sentry DSN secret reference" could be interpreted 2 ways: "A reference to a secret" or that the reference itself is a secret. Anyway, like I wrote really nitpicky.
scripts/replace-variables.sh
Outdated
| fi | ||
| done | ||
|
|
||
| # Substitute optional variables only when they have a non-empty value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it makes a difference in the end, but this loop does not do exactly what is described in the comment.
It unconditionally replaces all optional variables that are unset or empty by an empty string in the file.
I think that's what you want, since leaving something like "REACT_APP_SENTRY_DSN={{REACT_APP_SENTRY_DSN}}" (if "REACT_APP_SENTRY_DSN" is unset for example) in the env file does not make sense. So the comment should be corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, that was the initial functionality, but it changed
| type: string | ||
| required: false | ||
| OP_SENTRY_DSN: | ||
| description: Reference to the 1Password Sentry DSN secret reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one "I-cant-believe-it-hes-so-annoying" nitpick:
| description: Reference to the 1Password Sentry DSN secret reference | |
| description: Reference to the 1Password Sentry DSN secret |
|
I approve but only for the non-UI stuff (I haven't look at the UI changes) |
Co-authored-by: Copilot <[email protected]>
b046e5a to
cd32c40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-UI stuff looks good.

Summary:
closes #1091
Adds support for Sentry in the web-app. Also configures the deploy variables for Sentry to work.
The project is setup on the Sentry website
Is only setup for PROD
Expected behavior:
In production, if an unhandled error occurs, Sentry will catch it and log it. It will store information such as
To note: When doing a release we should make a habit to bump the package.json version as Sentry logs it and would be good to know what version the error is occurring on
Testing tips:
Quite difficult to test, must run locally. Go to
sentry.tsand replace the dsn value with one found in 1Password. From there (in localhost) cause an unhandled error and it should be logged to the sentry dashboardI tested it locally and saw that it correctly logged the error and sent a notifying email
Please make sure these boxes are checked before submitting your pull request - thanks!
./scripts/api-tests.shto make sure you didn't break anything