-
-
Notifications
You must be signed in to change notification settings - Fork 4k
feat/provide sentry userID and orgID #908
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
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 user context tracking by implementing utility functions to associate user and organization IDs with Sentry error reports. This will help with debugging by providing user context when errors occur.
Key changes:
- Created shared utility functions for setting Sentry user context in both React and NestJS libraries
- Integrated Sentry context setting into authentication middleware and user context components
- Added isolation scopes to prevent context leaking between requests
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/react-shared-libraries/src/sentry/sentry.user.context.ts | New utility function for setting Sentry user context in React apps |
| libraries/nestjs-libraries/src/sentry/sentry.user.context.ts | New utility function for setting Sentry user context in NestJS apps |
| libraries/nestjs-libraries/src/sentry/sentry.background.context.ts | New wrapper function for background jobs with Sentry isolation |
| apps/frontend/src/components/layout/user.context.tsx | Integration of Sentry user context in React user provider |
| apps/backend/src/services/auth/auth.middleware.ts | Integration of Sentry user context in authentication middleware |
| apps/backend/src/app.module.ts | Minor formatting fix (trailing comma) |
Copilot
AI
Jul 30, 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 function continues execution and calls callback() even when userId is missing, but then sets user context tags later. This creates inconsistent behavior. Consider restructuring to set all context at once or return early only when Sentry is not configured.
Copilot
AI
Jul 30, 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.
Multiple early returns with callback() execution creates code duplication. Consider setting both user.id and org.id tags first, then executing callback() once at the end.
Copilot
AI
Jul 30, 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 isolation scope wrapping doesn't properly handle the async middleware pattern. The next() call should be awaited and the function should return the result of withIsolationScope, otherwise the middleware chain may not wait for completion.
@nevo-david Do not merge this please.
There's a different, better, way to do this.
Redoing this
Uh oh!
There was an error while loading. Please reload this page.
What kind of change does this PR introduce?
eg: Bug fix, feature, docs update, ...
Why was this change needed?
Please link to related issues when possible, and explain WHY you changed things, not WHAT you changed.
Other information:
eg: Did you discuss this change with anybody before working on it (not required, but can be a good idea for bigger changes). Any plans for the future, etc?
Checklist:
Put a "X" in the boxes below to indicate you have followed the checklist;