Re-validate report rows against the status filter in Reports search - #98556
Conversation
|
@ChavdaSachin 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-18.at.16.54.31.movAndroid: mWeb ChromeScreen.Recording.2026-08-18.at.16.45.02.moviOS: HybridAppScreen.Recording.2026-08-18.at.16.57.58.moviOS: mWeb SafariScreen.Recording.2026-08-18.at.16.53.15.movMacOS: Chrome / SafariScreen.Recording.2026-08-18.at.16.40.30.mov |
| formatPhoneNumber, | ||
| bankAccountList, | ||
| groupBy, | ||
| queryJSON: lastSearchQuery?.queryJSON, |
There was a problem hiding this comment.
useSearchSections builds its sections from lastSearchQuery's snapshot and takes type from it, but didn't pass that query, so getReportSections falls back to getCurrentSearchQueryJSON().
that was harmless while the type check existed. with it gone, a live route query carrying a status those reports don't match makes isEligibleForStatus false for every row, so the carousel loses prev/next. the two can diverge because saveLastSearchParams is skipped offline.
|
@nkdengineer thanks for reviews. But this is draft yet. will let you know once ready |
c941602 to
59f9966
Compare
getReportSections only ran isEligibleForStatus for type:expense queries, but Spend > Reports issues type:expense-report, so a report kept its place in the list after its status changed. Pass queryJSON explicitly from the two callers that relied on the ambient route query.
# Conflicts: # tests/unit/SignInBackButtonTest.tsx
![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: a267fcf673
ℹ️ 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".
|
|
||
| const isActionLoading = !!isActionLoadingSet?.has(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportItem.reportID}`); | ||
| const shouldShow = !isActionLoading && currentQueryJSON?.type === CONST.SEARCH.DATA_TYPES.EXPENSE ? isEligibleForStatus(currentQueryJSON, reportItem) : true; | ||
| const shouldShow = isActionLoading || isEligibleForStatus(currentQueryJSON, reportItem); |
There was a problem hiding this comment.
When a status-filtered Reports search has exactly one loaded page and hasMoreResults is still true, this new local status filter can drop the updated report before the next page is fetched, so getReportSections() returns the post-filter length as allDataLength. Search/index.tsx then blocks pagination with offset > allDataLength - CONST.SEARCH.RESULTS_PAGE_SIZE (for example, 0 > 49 - 50), so users with more than 50 matching reports who submit/approve/pay one before scrolling can no longer load the remaining matches until they rerun the search. Please keep the pagination count based on the unfiltered snapshot length, or otherwise avoid feeding this post-filter length into the fetch-more guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
correct mechanism, allDataLength is the post filter length and it does gate fetchMoreResults.
two things though. in the scenario described the offset is 0, and the refresh in useSearchHighlightAndScroll re-runs the search at the current offset, so an offset 0 fetch replaces page one and the count comes back. the case that can stick is after scrolling, when the refetch merges at a deeper offset.
and it isn't new here, getTransactionsSections returns its post filter length the same way, so every type:expense status search already behaves like this. fixing it properly means changing that value in both builders, which I'd rather not fold into a one line bug fix. noted it in the PR description.
or @trjExpensify @nkdengineer do you think we should fix this here as well?!
There was a problem hiding this comment.
@nkdengineer sure, I did! with more than 100 matching reports, after acting on a row at offset 50+, allDataLength drops to 99 so 50 > 99 - 50 blocks fetchMoreResults, and the refresh re-fetches at the current offset and merges, so the hidden row isn't replaced. the list then stops loading further pages and looks like it ends at 99 rows until the query is re-run.
There was a problem hiding this comment.
Agree we don't need to care about this here.
|
@nkdengineer I think we are good to move now |
|
@mukhrr Let's merge main. |
|
@nkdengineer done |
|
🚧 luacmartins 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.4.56-0 🚀
|
|
🤖 Help site review: no documentation changes required. I reviewed the changes in this PR against the help articles under This is an internal bug fix, not a user-facing behavior or feature change:
The help site already documents Reports and status filtering as working this way (e.g. @mukhrr — if you believe a specific article should be updated to clarify this behavior, let me know which one and I'll draft the change. |
7 participants
Explanation of Change
Spend > Reports issues a
type:expense-reportquery, butgetReportSections()only re-validated rows against the status filter when the query type wasexpense. That check can never be true there, so a report kept its place in the list after its status changed.The check is dead code since Reports moved off
group-by:report, asgetReportSections()is only reachable from theexpense-reportbranch, so I removed it instead of extending it. The similar check ingetTransactionsSections()stays, it is load bearing for invoice and trip queries.useSearchSectionsalso now passesqueryJSON. It builds sections from that query's snapshot but never forwarded the query, sogetReportSections()fell back to the live route query. The removed type check used to make that mismatch inert; without it a route query carrying a different status drops every report and the report carousel loses its prev/next targets.Known, not introduced here:
getReportSections()returns its post-filter length asallDataLength, which gatesfetchMoreResults. A locally dropped row can therefore stall pagination at a page boundary.getTransactionsSections()already behaves this way fortype:expense, so I left it alone rather than change shared counting behaviour in a bug fix.Fixed Issues
$ #98332
PROPOSAL: #98332 (comment)
Tests
Precondition: an account with more than 50 expense reports sharing the same status (
Draftfor example).DraftSubmiton the last report in the tableOutstandingand no longer matches theDraftfilterOutstandingand verify the same report is listed thereOutstandingfilter and theApproveaction, and with theApprovedfilter and thePayaction, and verify the row leaves the list each timeOffline tests
DraftDraftfilterQA 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_app.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
IOS-app.mp4
iOS: mWeb Safari
IOS-web.mp4
MacOS: Chrome / Safari
web.mp4