Native set load measured #8242
Native set load measured #8242
Conversation
There was a problem hiding this comment.
The changes in this package are because of changes in iOS 18.
There was a problem hiding this comment.
There is a better solution for this.. I'll share when I can.
Btw this is cause Xcode 16 not ios 18
There was a problem hiding this comment.
The better way to fix this issue is by adding this line typealias Expression = SQLite.Expression after the imports in the Storage/Database.swift file, all other changes to the Gekidou library would not be needed.
package.json
Outdated
| @@ -39,7 +39,7 @@ | |||
| "@react-navigation/native": "6.1.18", | |||
| "@react-navigation/stack": "6.4.1", | |||
| "@rneui/base": "4.0.0-rc.8", | |||
| "@sentry/react-native": "5.27.0", | |||
There was a problem hiding this comment.
The package upgrade is because errors, probably related to iOS 18.
There was a problem hiding this comment.
Changing this needs to ipdate the cocoaSDK version on the xcode project as well
There was a problem hiding this comment.
Make sure that version v8.36.0 is used in the workspace Package.resolve of the xcode project
| let idCol = Expression<String>("id") | ||
| let teamIdCol = Expression<String>("team_id") | ||
| let typeCol = Expression<String>("type") | ||
| let idCol = SQLite.Expression<String>("id") |
There was a problem hiding this comment.
Are these changes because of Xcode 16? If they are Since this changes are caused by Xcode 16 unrelated to the problem, would it have been better to handle those in a separate PR beforehand. It would help keep the focus on the main bug fix here.
Updated: since I read previous comment that the problem is due to Xcode 16.
There was a problem hiding this comment.
I admit, when I saw 23 files changed, I made a note to myself that I need to carve a bit more time to run through this PR.
| await TestHelper.tick(); | ||
| expect(mockApiClient.post).toHaveBeenCalledWith(expectedUrl, expectedRequest); | ||
| mockApiClient.post.mockClear(); | ||
| PerformanceMetricsManager.finishLoad('HOME', serverUrl); |
There was a problem hiding this comment.
Q: we expected to not see another call if we only call finishLoad() again, right? Besides that scenario, is there any merit to see if we do another call to performance.mark() and then finishLoad() during the same "session"?
There was a problem hiding this comment.
Not sure what you mean with that. The performance.mark() call we do in the test is to mock what happens in the native side, since unit tests don't deal with native side. Calling again performance.mark(), on top of not giving us extra information, would not reflect what happens on the native side.
Or did I misunderstood your comment?
There was a problem hiding this comment.
Silly me, I misunderstood. performance.mark() just instantiate a new class with value from markName and will not affect what you're testing here which is finishLoad().
There was a problem hiding this comment.
Follow-up suggestion, I think there might be a merit to cover the skipLoadMetric() just to make sure that no API calls are being posted.
|
Since we want to know and confirm the effectiveness of this fix, is there a follow-up ticket or some sort of reminder or a temp chart/grafana measurement that will indicate these changes do solve the problem? Not a requirement, interested in the process. |
|
I created a new PR (#8254) for the XCode16 changes. When that is merged, I will update this one accordingly. |
|
@enahum Friendly reminder on this |
larkox
added
4: Reviews Complete
|
@yasserfaraazkhan Skipping QA review since there are no functional changes, and I already tested what this should fix. |
Summary
The React context may disappear for several reasons while leaving the native context alive. This makes it so the initial mark created by react-native-performance in the native side lives between React context restarts. This may be creating issues about recognizing whether the load metric to have been measured or not, which may lead to wrong measures.
In order to avoid this, we store whether we have measured or not the metric on the native side. This way we make sure this variable stays "alive" until the end of life of the native context.
Ticket Link
Fix https://mattermost.atlassian.net/browse/MM-59857
Release Note