Fix: Total spend footer does not always appear when all expenses are selected - #96120
Fix: Total spend footer does not always appear when all expenses are selected#96120lorretheboy wants to merge 15 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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 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] |
|
@lorretheboy Could you close this PR? Or marking it as a draft |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9f2f4cc9e
ℹ️ 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".
|
@lorretheboy Could you explain the RCA again, and why it only happens on accounts that have many expenses? |
|
@DylanDylann On an ad-hoc Expenses search,
Why only big accounts: it's purely a race against the request round-trip. shouldCalculateTotals: true makes the BE aggregate over all matching expenses, and the bigger payload lengthens the Onyx write queue that waitForWrites blocks on. Smallaccount = a few tens of ms, you can't click fast enough (why we couldn't repro). 450+ expenses = seconds, easy to hit. |
|
@lorretheboy In the testing steps, please emphasize that we need to select all right after deselecting in order to test this |
|
@DylanDylann I updated the test steps |
|
@lorretheboy In your fix, you made two changes:
|
|
From my testing, it works well with just the dedupeKey fix. So I'm wondering whether the second change is actually needed here |
|
@lorretheboy Kindly bump |
|
I will recheck today |
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@DylanDylann Please review again. Thanks |
| const hasSearchErrors = Object.keys(currentSearchResults?.errors ?? {}).length > 0; | ||
| // The server count is the only source for how many items "select all" covers, so keep the button loading until it | ||
| // arrives. Offline or on error it never will, so fall back to the count of the items we do have selected. | ||
| const isAllMatchingItemsCountLoading = areAllMatchingItemsSelected && typeof allMatchingItemsCount !== 'number' && !isOffline && !hasSearchErrors; |
There was a problem hiding this comment.
Why do you remove !!currentSearchResults?.search?.isLoading
There was a problem hiding this comment.
One more question, What will display when hasSearchErrors is true?
There was a problem hiding this comment.
They are from 2 different API calls, it belongs to a single request which we should not consider here to avoid another UI glitch @DylanDylann
There was a problem hiding this comment.
@lorretheboy Could you elaborate in more detail? I'm still a bit confused
There was a problem hiding this comment.
So here is the issue @DylanDylann
search.isLoading is set/cleared by any Search API call on that snapshot hash, but search.count only comes back from a call made with shouldCalculateTotals: true. Those are two different requests.
Eg: a refresh/paginated search with shouldCalculateTotals: false nulls count at offset === 0 and flips isLoading back to false, and only after that does the retry effect in Search/index.tsx fire the totals request. So with the old condition there's a window where isLoading === false but count is still missing → the skeleton disappears, the button briefly shows the local selection count, then flips back to loading and finally to the real total.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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 You can do another review now. Thanks |
|
@lorretheboy Bump on this #96120 (comment) |
|
@DylanDylann I explained the situation in more detailed way for you |
|
@lorretheboy Please upload videos for all platforms. It looks like this isn't the first time you missed it, even though you checked this item off in your checklist
|
|
@DylanDylann It was my bad. I will upload them shortly. For this specific PR, it used to be a deploy blocker so I raised a fix PR with only macos chrome recording in order to be a quick fix PR to resolve to DB. However, I will note this for future. Thanks for remind me |
Don't agree. The PR was created weeks ago, and the video is still missing |
Sorry for my wordings that make you misunderstanding. I mean it was my bad, I raised the PR with MacOs chrome recording to fix a DP after 23 mins since assignment to resolve the bug quickly. Then we decide to close this PR for a week then reopen which made me forget to update all other platforms. I noted this case down for future. Thank you for reminding @DylanDylann
|
|
@DylanDylann I uploaded recordings. Thanks |
| let selectionButtonText: string; | ||
| if (areAllMatchingItemsSelected) { | ||
| selectionButtonText = | ||
| typeof allMatchingItemsCount !== 'number' ? translate('search.exportAll.allMatchingItemsSelected') : translate('workspace.common.selected', {count: allMatchingItemsCount}); |
There was a problem hiding this comment.
Why are we getting rid of the allMatchingItemsSelected copy?
There was a problem hiding this comment.
@luacmartins We no longer use this. If users select all expenses, we'll display a loading indicator until the API returns the count
|
@lorretheboy conflicts |
|
@luacmartins Conflicts resolved. Please check. Thanks |

Explanation of Change
Fixed Issues
$ #96116
PROPOSAL:
Tests
Precondition:
Offline tests
QA Steps
Precondition:
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.Screenshots/Videos
Android: Native
Screen.Recording.2026-08-10.at.15.45.50.mov
Android: mWeb Chrome
Screen.Recording.2026-08-10.at.15.48.54.mov
iOS: Native
Screen.Recording.2026-08-10.at.15.41.15.mov
iOS: mWeb Safari
Screen.Recording.2026-08-10.at.15.37.58.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-14.at.14.35.38.mov