[CP Staging] fix: refresh Search after an expense moves report or a report changes workspace - #98752
Conversation
98723.mov |
98723.mov |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@QichenZhu 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] |
|
@codex review |
mountiny
changed the title
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
![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: bc7b5d00eb
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-17.at.8.49.14.PM.movScreen.Recording.2026-08-17.at.8.50.13.PM.movAndroid: mWeb ChromeScreen.Recording.2026-08-17.at.8.51.34.PM.movScreen.Recording.2026-08-17.at.8.52.20.PM.moviOS: HybridAppScreen.Recording.2026-08-17.at.8.41.21.PM.movScreen.Recording.2026-08-17.at.8.45.26.PM.moviOS: mWeb SafariScreen.Recording.2026-08-17.at.8.54.21.PM.movScreen.Recording.2026-08-17.at.8.55.12.PM.movMacOS: Chrome / SafariScreen.Recording.2026-08-17.at.8.58.14.PM.movScreen.Recording.2026-08-17.at.8.58.47.PM.mov |
|
Offline mode doesn't work for either issue, which is expected. Screen.Recording.2026-08-17.at.8.59.45.PM.movScreen.Recording.2026-08-17.at.9.00.08.PM.mov |
| const {currentSearchQueryJSON, currentSearchKey} = useSearchQueryContext(); | ||
| const {currentSearchResults} = useSearchResultsContext(); | ||
| const shouldCalculateTotals = useSearchShouldCalculateTotals(currentSearchKey, currentSearchQueryJSON?.hash, true); | ||
|
|
||
| // The snapshot keeps the report row after a workspace change, and only the server can tell whether it still matches the query. | ||
| const refreshSearch = () => { | ||
| refreshSearchAfterReportAction({ | ||
| currentSearchQueryJSON, | ||
| currentSearchKey, | ||
| shouldCalculateTotals, | ||
| isOffline, | ||
| isLoading: !!currentSearchResults?.search?.isLoading, | ||
| }); |
There was a problem hiding this comment.
Is this not going to lead to many extra Search calls?
There was a problem hiding this comment.
nope, it's one call per workspace pick. It only runs when you tap a row in that list, and if a search for the same hash and offset is already in flight, search method throws it away anyway
…regressions [CP Staging] fix: refresh Search after an expense moves report or a report changes workspace (cherry picked from commit f1ba262) (cherry-picked to staging by mountiny)
|
🚧 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! 🧪🧪
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.54-2 🚀
|
Help site review: no changes requiredI reviewed the changes in this PR and no updates to the help site ( Why: This PR is an internal bug fix to Search's refresh logic. It restores already-expected behavior rather than adding or changing any user-facing feature, workflow, screen, or label:
The user-facing flows involved (moving an expense between reports, changing a report's workspace, and filtering Search) are already documented and behave exactly as the existing articles describe — e.g. Managing Expenses in a Report and Use Search Operators to Filter and Analyze. This fix only makes the UI correctly reflect that documented behavior, so none of those articles need editing. @BartekObudzinski, please confirm this assessment. Since no help site changes are required, there is no linked help site PR to mark |
5 participants
Explanation of Change
Both blockers came from one removal in PR 98305, which is currently on main through the reland PR 98558. Before 98305 the trigger in
useSearchHighlightAndScrollwas(!isChat && hasTransactionsIDsChange) || hasReportActionsIDsChange, sohasReportActionsIDsChangeapplied to every search type. Any report action anywhere in the account refetched Search, which is the duplicate traffic reported in issue 98048, and it is also what made both of these flows work by accident. Restoring that condition is not an option, so each flow gets its own correct signal.98698, expenses count does not update after adding an expense to a report. Moving an expense replaces the transaction object because
reportIDchanges, buthasChangedTransactionInSearchResultsonly asked whether that transaction's own ID is in the results. In the repro the expense sits in a self DM that the Reports view does not show, so the answer was no and nothing refetched, while the report row still owes its expense count and total to the server computed snapshot. The check now also counts a transaction whosereportIDmoved into or out of a report the results display, which is a single lookup of thereport_<id>key already present in the snapshot. This lives in the hook rather than the call site becausechangeTransactionsReportis called from many places across the app, and all of them were equally stale.98723, report stays in the old workspace filter. Here there is no transaction side signal at all.
buildOptimisticChangePolicyDatasetsreport.policyIDoptimistically, writes only the policy object into the snapshot and never the report row, and touches transactions only when the destination currency differs. A filtered snapshot therefore keeps a report that no longer belongs to it, and no local patch can decide whether the row still matches the query, only the server can. SoselectPolicynow calls the existingrefreshSearchAfterReportActionfrom@libs/SearchRefreshUtilson all three exit paths (IOU, invite submitter, plain change). Other call sites in the codebase already follow that pattern, includinguseLifecycleActionsanduseSelectionModePayment. It is not done in the hook because the signal isreport.policyIDandSearch/index.tsxdoes not subscribe to the REPORT collection. Adding a whole collection REPORT subscription to that component would be a performance regression in a hot path.This also unblocks the open revert PR 98728, which would otherwise revert the reland again.
Fixed Issues
$ #98698
$ #98723
PROPOSAL:
Tests
test steps described in linked tickets
Offline tests
N/A
QA Steps
98698:
1.Action Performed:
2.Go to staging.new.expensify.com
3.Create an empty report in workspace chat.
4.Create an expense in self DM.
5.Go to Spend > Reports.
6.Select the report via checkbox.
7.Open the report.
8.Click Add expense > Add existing expense.
9.Select self DM expense > Add to report.
10.Close the report.
98723:
Preconditions: User has an unvalidated account, has multiple workspaces, and has multiple reports in Workspace 1
1.Open the NewDot app
2.Navigate to the Spend - Report - Filter - Workspace 1
3.Select any report - More - Change Workspace - Select another Workspace (like Workspace 4)
4.Report move to the new Workspace and is no longer displayed in the filter anymore, like Prod
// TODO: These must be filled out, or the issue title must include "[No QA]."
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