-
Notifications
You must be signed in to change notification settings - Fork 101
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
[SENTRY] Context and error description cleanup #361
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
|
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.
Apart from that one comment LGTM 👍
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.
Soft approve
|
useEffect(() => { | ||
if (windowVisible) { | ||
Sentry.setContext('user', { | ||
userAddress: account || 'DISCONNECTED', |
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.
David, pls validate with legal about this
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.
that will take a while
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 see u removed. pls makes sure is ok and re-enable if possible. in the mean time, ill merge in the current state (without)
Part of ongoing Sentry fixes
Adds Sentry "context" updater and removes unnecessary user params from error messages
e.g on sentry:
Sentry user context