Repository navigation
Fix the Search footer flickering to a loading skeleton after Select all - #102375
Conversation
…elected offline Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
🤖 Web test steps verification results: ✅ pass On dev NewDot web with 64 unreported Self DM expenses, the Search footer now shows offline after "Select all" (all matching items), and it shows server totals once back online. The PR's Tests section is still a TODO, so these steps come from the linked issue's reproduction steps.
|
…ms are selected offline" This reverts commit e94ac1d.
…ed the footer skeleton after Select all Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed the failing Cause: This PR makes Fix: I added I couldn't run Jest locally in this environment, so CI will confirm the fix. The |
|
@MelvinBot I think it's intentional to call 2 apis (shouldSaveRecentSearch = false / true). |
…re-fire so the footer skeleton doesn't flash Reverts the useSearchPageSetup dedupe from 9f8f032 per review: the page still sends its flagged request. When that request is queued behind a successful request that already returned the same totals, the re-fire no longer sets search.isLoading, so the footer keeps showing the totals instead of flashing its skeleton. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
Done in f8da012. Both Search requests still go out. Only the footer skeleton flicker is fixed.
Testing
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot review |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
I found one likely regression. It needs a narrow timing window, so it's low severity. Load more can now start while the save-only re-fire is still running. Search blocks "load more" only while
Before this PR, Possible fix: On the non-live path, also treat a pending first page as busy, for example What I checked with no regression found
I checked this by reading the code. I didn't test it manually. |
DetailsExplanation of ChangeOnline, picking "Select all" on Search made the footer flicker: correct totals, then the loading skeleton, then correct totals again.
Fix: the API calls are unchanged. When a re-fire only adds the save flag, and the request it waited on succeeded with the same totals setting, the re-fire no longer sets Fixed Issues$ #102339 TestsPrerequisite: an account with 50+ expenses (so "Select all" is offered and there is more than one page).
Offline testsN/A QA StepsSame as Tests
PR Author Checklist
@MelvinBot please set the PR body to the content in the details section above and mark as ready for review |
|
@mkhutornyi 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] |
|
Done. I set the PR body to the content you provided and marked the PR as ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8da012d97
ℹ️ 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".
| shouldSaveRecentSearch: inFlightRequest.pendingShouldSaveRecentSearch, | ||
| // A re-fire that only adds the save flag returns the same results the finished request just showed, | ||
| // so it must not show loading again (the footer would flash its skeleton over the totals). | ||
| shouldShowLoading: !inFlightRequest.didSucceed || inFlightRequest.pendingShouldCalculateTotals !== inFlightRequest.shouldCalculateTotals, |
There was a problem hiding this comment.
Keep the loading state after an empty successful response
When the first request returns HTTP 200 but omits snapshot data—a case the success handler explicitly records as loading_terminal_empty—didSucceed is still set to true, so this queued save-only request suppresses isLoading. Because that response leaves any cached snapshot and its count/total intact, the footer can display stale totals until the re-fire completes rather than indicating that the displayed data was not refreshed. Only suppress the loader after a successful response that actually supplied usable snapshot data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
omits snapshot data - not valid
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari102375.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #102339 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 jasperhuangg 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/jasperhuangg in version: 9.4.95-0 🚀
|
|
No help site changes are required for this PR. This PR fixes a brief loading flicker in the Spend page footer after you choose Select all. It doesn't change any feature, setting, label, button, or workflow. Why no docs update is needed
I didn't create a help site PR because no changes are needed. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.95-4 🚀
|













Explanation of Change
Online, picking "Select all" on Search made the footer flicker: correct totals, then the loading skeleton, then correct totals again.
Searchsends the select-all totals request (shouldSaveRecentSearch: false).shouldSaveRecentSearch: true. The in-flight dedupe insearch()queues it as a re-fire. This second request is intentional and still goes out.isLoading: true, offset: 0while the correctcountwas still there, so the footer showed the skeleton.Fix: the API calls are unchanged. When a re-fire only adds the save flag, and the request it waited on succeeded with the same totals setting, the re-fire no longer sets
search.isLoading. The results on screen are already current, so the footer keeps showing the totals.search.statestill tracks the request as before. A re-fire that adds totals, or that follows a failed request, still shows loading.Fixed Issues
$ #102339
PROPOSAL: #102339 (comment)
Tests
Prerequisite: an account with 50+ expenses (so "Select all" is offered and there is more than one page).
Offline tests
N/A
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.