Re-validate report rows against the status filter in Reports search - #98556
Re-validate report rows against the status filter in Reports search#98556mukhrr wants to merge 3 commits into
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: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| formatPhoneNumber, | ||
| bankAccountList, | ||
| groupBy, | ||
| queryJSON: lastSearchQuery?.queryJSON, |
|
@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
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.
Preserve pagination count when locally hiding reports
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 👍 / 👎.
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mp4