Repository navigation
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
@jmusial The last bump hit staging, we can proceed with this one now. Could you bump straight to 3.0.138? Your Onyx PR was published in 3.0.133 but all the other releases before and after are just library bumps, we don't need QA for them. See the last Onyx weekly update for reference.
|
|
@MelvinBot review |
|
@bernhardoj Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🤖 Web test steps verification results: Verified basic Inbox and Spend rendering, but Spend shows “No expenses yet,” so the required performance fixture is unavailable. The five-run comparison against main, span timings, reload behavior, and offline behavior remain unverified.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
@jmusial I saw multiple OnyxDerivedCompute_reportAttributes logs. 2 of them were logged before the SearchForTodos request finished. A few were logged after the SearchForTodos request finished. a.mp4 |
I'll clarify manual test steps. |
|
Web this PR: OnyxDerivedCompute_reportAttributes ManualNavigateToInboxTab Web in OnyxDerivedCompute_reportAttributes ManualNavigateToInboxTab @jmusial hmm, looks like this PR is somehow slower |
This is strange, the numbers I got (re-reasured today morning)
Looking @ your numbers: a 60% jump in navigation (565 to 893 ms) seems too big to be caused by an array-equality check (which my change was)... |
|
Hmm, yeah, that's so weird. I tried without switching branches and only changed the Looking at your numbers (especially ManualNavigateToInboxTab), it's really small. I got small numbers too in staging. Do you test it on an ad hoc build? I tested this locally on a dev build. |
|
It's my local dev, but yeah I think we should do an ad hoc test. Edit: @bernhardoj how big is your account ? Edit 2: remeasured with 4x slowdown, 4x, alternating builds (ABBA, 8 reloads each). Numbers are closer to yours, but PR change holds the gain. Looking @ the results the order may matter - second series is much faster for
|
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Ad-hoc: Staging: Retested with ad-hoc and staging still faster 🤔
I have 1152 expenses in total. |
Interesting... I rechecked on Main Applause acc (4,102 reports) and got
I msgd you on slack, could you share you're accounts onyx dump ? I'll try to figure out where the differences come from |
|
@bernhardoj I could not reproduce a slowdown on your account through copilot :( Please take a look @ below and check, maybe we were doing sth different that would cause the discrepancy ?
Metrics
Spans are read from the Method
Steps per run
Accounts
Counts are the locally cached Onyx keys ( 1. Your account, local dev, only Onyx swapped (4x)
The first PR run was the first load after logging in (warm-up). Without it, the PR median for 2. Your account, ad hoc vs staging (4x)
The two builds also differ by 1,290 commits of app code, so this test alone cannot isolate Onyx. Test 1 does. |
|
I finally see it faster in this PR after following the above steps. Maybe rebuild the web after reinstalling onyx did help 🤔
|
I think that rsbuild is caching |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.38-0 🚀
|


Explanation of Change
Bumps
react-native-onyxfrom 3.0.122 to 3.0.138.When
cache.mergegets an array that is deep-equal to the cached one, it now keeps the cached array instead of replacing it. Objects already worked this way (#765), and this extends it to arrays.The rest of the 3.0.122 → 3.0.138 range only changes dev dependencies or removes unused ones.
Fixed Issues
$ #104725
PROPOSAL:
Tests
Use an account with a few hundred expenses that have violations. Compare this PR's E/App build with
main, 5 runs each, using the medians. Spans appear in the JS console as[Sentry][<span name>] Ending span (<N>ms).Web (desktop and mobile browsers)
Ending span.OnyxDerivedCompute_reportAttributeslogged after theSearchForTodosrequest finishes. The earlier ones aren't affected.ManualNavigateToInboxTab.Native (iOS and Android)
Ending span.OnyxDerivedCompute_reportAttributeslogged after theSearchForTodosrequest finishes. The earlier ones aren't affected.ManualNavigateToInboxTab.Offline tests
Same as tests
QA Steps
Ending span.Everything should work the same as prod.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android-native-pr860.mp4
Android: mWeb Chrome
android-chrome-pr860.mp4
iOS: Native
ios-native-pr860.mp4
iOS: mWeb Safari
ios-safari-pr860.mp4
MacOS: Chrome / Safari
web-chrome-pr860.mp4