fix: prevent "Something went wrong" screen flash on failed spend query#93752
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@QichenZhu 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 574f2f24b6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (validGroupBy || (shouldCalculateTotals && searchResults?.search?.count === undefined)) { | ||
| shouldRetrySearchWithTotalsOrGroupedRef.current = true; | ||
| } | ||
| if (offset === 0 || offset === searchResults?.search?.offset || !isFocused || isOffline || searchResults?.search?.isLoading) { |
There was a problem hiding this comment.
Re-run empty searches after reconnecting
When a user has a cached empty search open while offline and then reconnects, this new guard always returns for offset === 0, so the removed comingBackOnlineWithNoResults path no longer refreshes the snapshot. useSearchPageSetup() does not compensate because isSearchDataLoaded() treats existing empty data as loaded, so users can remain stuck on a stale empty state until they change the query or navigate away.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@nabi-ebrahimi thanks for the prompt update. However, I doubt the above comment is valid. Could you clarify:
- What was the bug before your recent changes?
- How do I verify it's working now?
Asking because I didn't notice any behaviour difference before/after, so I'm not sure whether I was testing it wrong or the bug didn't exist at all.
There was a problem hiding this comment.
@QichenZhu thanks for checking.
That change is only to keep the old reconnect behavior for empty results.
Before, if a search was already loaded but empty, then the app went offline and came back online, Search still refreshed once via comingBackOnlineWithNoResults.
After removing the broad Search effect, that refresh was lost because useSearchPageSetup() sees empty data as already loaded and skips the request.
I tested it with two sessions:
- Session A has an empty expense search open.
- Session A goes offline.
- Session B creates an expense matching that search.
- Session A goes back online.
With the current change, Session A sends one Search request on reconnect and the new expense appears. I also guarded it with hasErrors, so it does not retry the failed-query error case.
Video:
required-search-req.mov
There was a problem hiding this comment.
Thanks for answering the second question.
- How do I verify it's working now?
How about the first one?
- What was the bug before your recent changes?
I tried it without the last 2 commits and got the same expected result.
So I don't think the bot comment is valid, and I don't see what benefit the recent changes provide. Am I missing something?
There was a problem hiding this comment.
@QichenZhu thanks for pointing this out, that's why we have C+s here.
Addressed this.
| const shouldRefreshBehindRHP = isChat || hasManualHighlightTransactionIDs; | ||
| const isSearchStillActive = isFocused || (isSearchTopmostFullScreenRoute() && shouldRefreshBehindRHP); | ||
| if (!isSearchStillActive || isOffline) { | ||
| hasPendingSearchRef.current = true; | ||
| hasPendingSearchRef.current = isOffline || shouldRefreshBehindRHP; |
There was a problem hiding this comment.
Keep pending refreshes for expense changes behind RHP
When a non-chat Search page is covered by an RHP and a matching transaction is added or removed while the RHP is open, isFocused is false and shouldRefreshBehindRHP is also false unless a manual highlight was registered, so this drops the pending refresh instead of preserving it. After the RHP closes, previousTransactions has already advanced and the hook no longer sees an ID change, leaving the expense search snapshot stale until another search is triggered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This comment is genuine. Let's not touch the highlight logic then, since it doesn't contribute to the original issue.
Before
before.mov
After
after.mov
There was a problem hiding this comment.
Addressed, thanks.
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
@QichenZhu thanks for the videos. I agree the The PR now only keeps the issue-scoped |
|
@QichenZhu kindly bump, thanks |
|
@nabi-ebrahimi, it may have escaped your notice, but the question above is still outstanding. |
@QichenZhu sorry, I missed that one. I replied here: #93752 (comment) |
|
@nabi-ebrahimi I replied. Could you take another look? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen_recording_20260625_143920.webmAndroid: mWeb ChromeScreen_recording_20260625_144041.webmiOS: HybridAppSimulator.Screen.Recording.-.iPhone.17.-.2026-06-25.at.21.39.35.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.17.-.2026-06-25.at.14.40.30.movMacOS: Chrome / SafariScreen.Recording.2026-06-25.at.2.06.19.PM.mov |
There was a problem hiding this comment.
@nabi-ebrahimi thanks! One last thing (hopefully) is to fix the lint error.
Done, thanks! for the review. |
|
🚧 blimpich has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.21-2 🚀
|
|
Deploy Blocker #94742 was identified to be related to this PR. |
Explanation of Change
This PR prevents Search from re-running an already resolved initial search request.
useSearchPageSetup()already owns the initialoffset: 0request for the current search query. TheSearchcomponent was also issuing a mount/focus search for the same query. When that query failed, the duplicate request optimistically clearederrors, causing the error page to unmount and the loading skeleton to appear again.This change removes that redundant initial fetch path from
Search, so failed search snapshots remain stable and theSomething went wrongpage stays visible.Fixed Issues
$ #92462
PROPOSAL:#92462(comment)
Tests
Spend>Expenses.type:chat category:abcd.Offline tests
N/A. This issue is about duplicate Search requests after a failed online search response.
QA Steps
Same as tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Screen.Recording.2026-06-17.at.2.11.29.AM.mov
Android: mWeb Chrome
Screen.Recording.2026-06-17.at.2.17.33.AM.mov
iOS: Native
Screen.Recording.2026-06-17.at.1.44.17.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-06-17.at.1.52.47.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-06-17.at.2.24.39.AM.mov