Fix app_start trace not firing in SwiftUI apps using @UIApplicationDelegateAdaptor (#15802)#15912
Fix app_start trace not firing in SwiftUI apps using @UIApplicationDelegateAdaptor (#15802)#15912JesusRojass wants to merge 5 commits intofirebase:mainfrom
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where app start traces were not firing correctly in SwiftUI applications that use @UIApplicationDelegateAdaptor. The fix cleverly defers the decision of whether a launch occurred in the background from applicationDidFinishLaunching to appDidBecomeActive, using a time threshold to distinguish between a genuine background launch and the false positive from SwiftUI. The implementation is clean and well-commented. I have one minor suggestion to improve a comment for better clarity.
| // App launched in background so we invalidate the captured app start time | ||
| // to prevent incorrect measurement when user later opens the app |
There was a problem hiding this comment.
This comment is a bit misleading as it appears to be a leftover from the previous implementation. The code no longer invalidates appStartTime. It just skips the app start trace if it's determined to be a genuine background launch. Consider updating the comment to more accurately reflect the current logic.
| // App launched in background so we invalidate the captured app start time | |
| // to prevent incorrect measurement when user later opens the app | |
| // If the time since launch is too long, it's a genuine background launch. |
|
Ready for review @visumickey 😄 💯 |
| // Distinguish genuine background launches by checking how long ago +load captured appStartTime. | ||
| // A real foreground launch reaches didBecomeActive within seconds; a background launch that is | ||
| // later opened by the user will have a much larger gap. | ||
| if (sLaunchedInBackgroundState) { |
There was a problem hiding this comment.
Worried about a scenario where a non-switftUI app starts itself in background, but the app gets launched before the max app start duration value - this code would account for that app start time.
I think the ideal would be differentiate if this is a SwiftUI app and make some exceptions rather than impacting all the scenarios.
There was a problem hiding this comment.
I have added a commit that checks for UIApplicationSceneManifest in the info.plist before deferring to keep the original behavior in the rest of the escenarios
LMK what you think 😄
|
Comment addressed! LMK what you think @visumickey 😄 |
ncooke3
left a comment
ncooke3
left a comment
There was a problem hiding this comment.
Nice! Please also include a changelog entry, under a new # Unreleased header.
|
thanks @ncooke3 |
3 participants
Description
Fixes #15802
SwiftUI apps using
@UIApplicationDelegateAdaptorreportapplicationStateas.backgroundduringdidFinishLaunchingWithOptionseven on normal foreground launches, because scenes haven't connected yet. This caused the_as(app start) trace to be incorrectly skipped.Changes
Defer the background launch decision from
didFinishLaunchingtodidBecomeActive. RecordsLaunchedInBackgroundStateat launch time and use the existing 60-minute threshold (gAppStartMaxValidDuration) to distinguish genuine background launches from SwiftUI's false.backgroundreport.Testing
Verified with a SwiftUI test app using
@UIApplicationDelegateAdaptor:_astrace fires correctly (~580ms)API Changes