Fix proper sentry initialization#10116
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR moves the remote-server daemon through the full app initialization path and adds protocol plumbing for Sentry identity and crash-reporting preference updates.
Security
- The client-side crash-reporting preference stored for future daemon handshakes is only initialized once, so later connects can send a stale opt-in/opt-out state.
Concerns
- Re-enabling daemon crash reporting after it was disabled initializes Sentry without reapplying the client-supplied user identity, despite the new protocol fields.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| fn apply_sentry_user_id(&self, ctx: &mut warpui::AppContext) { | ||
| if !self.auth.sentry_user_id.is_empty() { | ||
| crate::crash_reporting::set_user_id( | ||
| crate::auth::UserUid::new(&self.auth.sentry_user_id), | ||
| if self.auth.sentry_user_email.is_empty() { | ||
| None | ||
| } else { | ||
| Some(self.auth.sentry_user_email.clone()) | ||
| }, | ||
| ctx, | ||
| ); |
There was a problem hiding this comment.
Can we not directly populdate the AuthManager / AuthStateProvider upon receiving the initialize message? I doubt this is the only model that we need for remote code that reads those two models
There was a problem hiding this comment.
This is a follow-up Moira is already working on. I will keep it minimal for now but this can hook into her work
| // privacy settings so that future daemon handshakes send the | ||
| // current value rather than a stale snapshot. | ||
| let privacy_settings = PrivacySettings::handle(ctx); |
There was a problem hiding this comment.
It's not just future daemon handshakes thuogh...we'll need to send an explicit message to the server when the user changes their crash reporting setting right?
There was a problem hiding this comment.
This code path is for initial connection -- the explicit notification on settings change is handled in app/src/remote_server/mod.rs
Although I agree this is unnecessarily complicated here, I cleaned up the code path to compute this on the fly instead
kevinyang372
force-pushed
the
kevin/fix-proper-sentry-initialization
branch
from
65dc295 to
ffd2dad
Compare
2 participants
Description
Tech spec here: https://github.com/warpdotdev/warp/blob/70a0bed2f31a126495f8190e9cb5252cb9fe3ee6/specs/daemon-sentry-initialization/TECH.md
Testing
Tested locally and confirmed sentry event was produced: https://warpdotdev.sentry.io/issues/7459771029/?project=5660375&query=is%3Aunresolved&referrer=issue-stream