-
-
Notifications
You must be signed in to change notification settings - Fork 376
fix: in_foreground and is_active app context #7188
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
base: main
Are you sure you want to change the base?
Conversation
Fix the mismatch of the app context in foreground between fatal and non-fatal events. Also, fix the breadcrumbs to correctly add in_foreground and in background crumbs, and add new active crumbs for more info for the user. Fixes GH-6178
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Metrics
Other
Bug Fixes 🐛
Build / dependencies / internal 🔧Deps
Other
Other
🤖 This preview updates automatically when you update the PR. |
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Fixes
- in_foreground and is_active app context ([#7188](https://github.com/getsentry/sentry-cocoa/pull/7188))If none of the above apply, you can opt out of this check by adding |
| - (void)trackApplicationNotifications | ||
| { | ||
| #if SENTRY_HAS_UIKIT | ||
| NSNotificationName foregroundNotificationName = UIApplicationDidBecomeActiveNotification; |
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.
Here's a key change. The foreground crumb now gets added when UIApplicationWillEnterForegroundNotification and not when UIApplicationDidBecomeActiveNotification. I think that's more accurate.
| BOOL inForeground = appState == UIApplicationStateActive; | ||
| app[@"in_foreground"] = @(inForeground); |
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.
Here was the mismatch between fatal and non-fatal events. Here we used to set in_foreground to true if the app was active, but the app could also be in the foreground but not active and then this was false before. Now it's aligned with SentryCrash
sentry-cocoa/Sources/SentryCrash/Recording/SentryCrash.m
Lines 521 to 539 in 7e93d85
| - (void)applicationDidBecomeActive | |
| { | |
| sentrycrash_notifyAppActive(true); | |
| } | |
| - (void)applicationWillResignActive | |
| { | |
| sentrycrash_notifyAppActive(false); | |
| } | |
| - (void)applicationDidEnterBackground | |
| { | |
| sentrycrash_notifyAppInForeground(false); | |
| } | |
| - (void)applicationWillEnterForeground | |
| { | |
| sentrycrash_notifyAppInForeground(true); | |
| } |
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| withDataValue:@"foreground"]; | ||
| }]; | ||
|
|
||
| [NSNotificationCenter.defaultCenter addObserverForName:didBecomeActiveNotificationName |
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.
Looks like a swift name
📜 Description
This is still a draft PR, I would love to get some feedback on the chosen approach. I initially wanted to fix only the in_foreground app context, but then I realized there's a benefit in knowing if the app is in the foreground, but also if it's active or not. If we agree that this is the right approach, I'm going to add tests.
Fix the mismatch of the app context in foreground between fatal and non-fatal events. Also, fix the breadcrumbs to correctly add in_foreground and in background crumbs, and add new active crumbs for more info for the user.
💡 Motivation and Context
Fixes GH-6178
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.