[HOLD] Search: Use live onyx data if available#78877
[HOLD] Search: Use live onyx data if available#78877s77rt wants to merge 11 commits intoExpensify:mainfrom
Conversation
|
@dukenv0307 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] |
|
@suneox @luacmartins One of you needs to 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] |
|
This is missing QA steps but otherwise ready for review. |
|
Not sure what happened here, but did Melv assign two C+ on purpose? 🤔 |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I could take it as C+ since I was assigned first |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb Safari |
|
@s77rt Can you pls take a look at the failed tests? |
trjExpensify
left a comment
There was a problem hiding this comment.
I don't think there's anything product related here tbh, but it's part of the Todo counters project rollout and I'm excited for it! 👍
luacmartins
left a comment
There was a problem hiding this comment.
Looking good. Let's look into the failing tests.
|
@fabioh8010 could you please review this PR since you have context on the |
|
I am not up to date with the project but could we:
This way we move the |
|
quick question from my side: Since snapshots are search-related, do we need to force every |
|
It's a heavy screen though, so it's still going to be a regression 🤕 |
|
Yeah I know, but even right now we have some additional performance overhead - on opening the app |
|
@blazejkustra @adhorodyski @TMisiukiewicz I'm not sure if we're on the same page here. Are we testing the performance between the custom useOnyx hook and the one from the library? If so, I'm not sure that's the correct comparison. We already use the custom useOnyx wrapper in App and have had it for a while. That was throughly tested by @fabioh8010 back in the day and we saw no performance loss. This PR is only updating it so that the return value merges live Onyx data and snapshot data, preferring live data if available. |
|
or are you referring to the double call to |
|
After reading the comments above again, I think we're talking about my second comment above. In that case, maybe we could try to update the function in this PR to move the computation to the selector if possible and make a single useOnyx call.
Here's the discussion describing the problem solved by the custom wrapper. TLDR subcomponents used in Search can connect directly to Onyx and if they don't use the correct data source, we'd display incorrect data. So to do something like you suggested, we'd need to know the entire subcomponent tree used within Search (and any future components that are added) and use the wrapper only on those components. |
|
@mountiny wrt #78877 (comment), we're updating the BE to return Search data as live data for the To-dos only. This was a long discussion we had, but TLDR is that for other Searches, we'd have to update the snapshot shape to return a list of the transactions/reports that should be shown as search results and that was deemed outside of the scope of the project. For to-dos, we can use selectors to render the correct reports based on their next action, so we don't need to update the snapshot shape. |
|
@TMisiukiewicz can you share the steps you used to run this analysis please? #78877 (comment) |
Ah I see, I thought we are doing it for all search. Once we will do that (inevitably) though, the snapshots can be removed (only pointer snapshots will remain) |
|
Updated second connection to use |
I use heavy account or imported state on iOS or Android and use Performance tab in React Native devtools: For app startup:
For creating an expense:
Then upload the profile to https://www.speedscope.app/, go to Sandwich view, |
|
@TMisiukiewicz @blazejkustra @adhorodyski did you get a chance to retest the performance after the latest changes? #78877 (comment) |
|
I retested the branch on app startup and I'm still seeing the performance degradation of |
|
This still looks like a degradation, and not a small one. My question is can we do something fundamentally better to make this work, without derailing the design doc release? |
|
Using #73977 (comment) as a source of truth for what we're trying to cover, my best bet is relying on Onyx Derived Values instead of altering the useOnyx wrapper in any way. As per its doc comment, this is what they're for and it sounds exactly like our use case here: Here's how one should look like: Should I work on trying to put this together or @s77rt you feel comfortable with trying out this approach? It should not come with a performance penalty, not the size of the above measurements. |
|
So you're proposing to duplicate the merged data in derived values? |
|
It looks like the usecase is exactly what this API was designed for, yes. It auto-updates on changes to any of the keys and allows for a simple useOnyx usage across React components, just to the new, 'virtual' key instead of the raw one of either data set. You can see other usecases for which we've decided to build a derived value, and here I see how the |
|
I'm not sure if that's feasible. We'd end up duplicating almost the entire Onyx DB in derived values, since the components in Search could potentially access any report, policy, transaction, reportAction, reportNameValuePairs, etc keys |
The condition must never change in any react component lifecycle, otherwise this would cause weird bugs
|
We're going with a different approach #79506 |
|
Hi @s77rt, how are you? Could you help me find the .zip file whmcs for rdpcloud? I understand if the assistance involves payment. My email is espaciovirtual.com@gmail.com |
This comment was marked as off-topic.
This comment was marked as off-topic.
Thanks, would you be so kind as to contact me or leave me a contact? @s77rt |



Explanation of Change
useOnyxhook now makes two useOnyx connections, one regular (same as passed param) and another for search results snapshotshouldUseSnapshotis true, then the search snapshot results is merged with the live onyx datamain)Fixed Issues
$ #73977
PROPOSAL:
Tests
shouldUseSnapshot,originalResult,snapshotResultandresultshouldUseSnapshotisfalsethensnapshotResult[0]isfalseshouldUseSnapshotistruethenresultis the combinedoriginalResult/snapshotResultOffline tests
Same as QA Steps
QA Steps
Test 1:
Screen.Recording.2026-01-06.at.9.54.28.PM.mov
Test 2:
Screen.Recording.2026-01-06.at.9.55.04.PM.mov
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari