feat: resolve Search loading gates from snapshot terminal state - #96388
Conversation
…erminal-state # Conflicts: # src/libs/actions/Search.ts
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@DylanDylann 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Product review isn't required.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b877acb279
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@DylanDylann Are you able to complete review today? |
| // loading/empty states internally via shouldShowLoadingState. | ||
| // When errors are present, let Search mount so it can render FullPageErrorView. | ||
| return (hasNoData && !hasErrors) || isCardFeedsLoading; | ||
| return (hasNoData && !hasErrors && !isLoaded) || isCardFeedsLoading; |
There was a problem hiding this comment.
NAB: This looks redundant, since hasNoData already covers it: resolved → data = {} → hasNoData = false → skeleton off. isLoaded isn't needed
| } | ||
| const shouldSkipWaitForWrites = hasDeferredWrite(CONST.DEFERRED_LAYOUT_WRITE_KEYS.SEARCH); | ||
| search({queryJSON, searchKey: currentSearchKey, offset: 0, shouldCalculateTotals, isLoading: false, skipWaitForWrites: shouldSkipWaitForWrites}); | ||
| }, [hash, isOffline, shouldUseLiveData, queryJSON, isSnapshotDataLoaded, isSnapshotSearchLoading, currentSearchKey, shouldCalculateTotals]); |
There was a problem hiding this comment.
Please remove isSnapshotSearchLoading from dependency
There was a problem hiding this comment.
This one is load-bearing as a dependency. It is only a retry trigger, not a gate. handlePreventSearchAPI flips search.isLoading when the temporary prevention during handleDeleteTransactionsWithNavigation ends, and that transition is what re-runs the effect so the deferred search fires. If I drop it from the deps, the stuck skeleton case comes back and useSearchPageSetup.test.ts fails on exactly that sequence. A stranded true cannot block the initial attempt because the effect body does not gate on it. I can rename it or add a clearer comment if it reads like a stored flag dependency.
| let searchResults: SearchResults | undefined; | ||
| if (currentSearchResults?.data != null || currentSearchResults?.errors) { | ||
| if (isCurrentSearchResolved && currentSearchResults?.search && currentSearchResults.data === undefined) { | ||
| searchResults = {...currentSearchResults, search: currentSearchResults.search, data: {}}; |
There was a problem hiding this comment.
| searchResults = {...currentSearchResults, search: currentSearchResults.search, data: {}}; | |
| searchResults = {...currentSearchResults, data: {}}; |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-23.at.17.24.49.movAndroid: mWeb ChromeScreen.Recording.2026-07-23.at.17.20.54.moviOS: HybridAppiOS: mWeb SafariScreen.Recording.2026-07-23.at.17.21.20.movMacOS: Chrome / SafariScreen.Recording.2026-07-23.at.15.33.26.mov |
|
Discovered a new bug on main: the loading bar in the left sidebar displays infinitely when reloading the page while the search page is loading. @BartekObudzinski It would be worthwhile if you could take a look. On the other hand, I'm fine with leaving it out Screen.Recording.2026-07-23.at.15.37.24.mov |
|
@DylanDylann Confirmed and fixed. The loading lifecycle now lives in shared state keyed by reportID, so a consumer that mounts after OpenReport leaves the queue still reads as pending until the terminal loading update flushes. Added a regression test for that exact sequence: |
|
@BartekObudzinski Wrong place 😄 |
|
Damn right 😂 I've lost in my prs |
|
@BartekObudzinski We got conflicts |
|
🚧 mountiny 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/mountiny in version: 9.4.45-0 🚀
|
No help site changes requiredI reviewed the changes in this PR and confirmed that no updates are needed to Expensify's help site files under Why: This is an internal loading-state bug fix for the Search page. It reworks how the loading skeleton is gated (moving from There is no new feature, UI element, tab, setting, or button — and no change to any documented, user-facing behavior:
Because no help site changes are required, I did not create a draft docs PR — so there is nothing to review or mark @BartekObudzinski, if you believe a help site update is warranted here, let me know what behavior should be documented and I'll draft it. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.45-14 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
The Search page decides whether to show its loading skeleton from the shape of the snapshot data (
data === undefined) and a storedsearch.isLoadingflag. Both can lie: a successful response that writes no snapshot data leavesdataundefined forever, and a reload or dropped response can strandisLoading, so the skeleton hangs indefinitely with no request running.This change moves the Search loading gates onto the snapshot's explicit terminal lifecycle state (
loading | loaded | error), which the search action always resolves on success, failure, or a network-level rejection:isSearchDataLoadedtreats a terminalloaded/errorstate as resolved, so a response that carried no data hands off to Search's own empty view instead of pinning the skeleton. The existing type/hash match still rejects a stale snapshot that Onyx can transiently return for the previous query.useSearchLoadingStatestops the page skeleton once the request resolves.isLoadingflag. It relies on resolved snapshot data plus the search action's existing in-flight de-duplication, which resets on reload, so a stranded loading state re-fires and self-heals instead of blocking forever.hash(alongsidetype) so a terminal state satisfies the anti-stale match even on a response that carried none of its own metadata.jsonCode200 with no snapshot data) now logs a diagnostic so this previously-invisible case stays queryable.Adds a helper
isSearchPending(searchResults)and unit tests for the terminal-state read side. Healthy paths are unchanged; the only behavior difference is that a resolved-but-dataless response now shows the empty view instead of an endless skeleton.Fixed Issues
$ #96488
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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