Revert "fix: reland Search re-fire fix with the deploy blocker regressions closed" - #98728
Beamanator wants to merge 1 commit into
Conversation
|
@chuckdries 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] |
![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: b21b91f82c
ℹ️ 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".
|
|
||
| // Check if there is a change in the transactions or report actions list | ||
| if ((isChat ? hasReportActionsIDsChange : hasTransactionsIDsChange || hasChangedResultTransaction) || hasPendingSearchRef.current) { | ||
| if ((!isChat && hasTransactionsIDsChange) || hasReportActionsIDsChange || hasPendingSearchRef.current) { |
There was a problem hiding this comment.
When an expense already present in the Search snapshot is edited—for example, its amount, category, or date changes—the transaction key set remains unchanged, so this condition never calls search(). Snapshot-backed rows, filter membership, and server-calculated footer totals therefore continue showing the pre-edit values until another event refreshes the search.
Useful? React with 👍 / 👎.
| ); | ||
| refreshSearchAfterReportAction({ | ||
| currentSearchQueryJSON, | ||
| currentSearchKey, | ||
| shouldCalculateTotals: shouldCalculateTotalsOnRefresh, | ||
| isOffline, | ||
| isLoading: !!currentSearchResults?.search?.isLoading, | ||
| }); | ||
| clearSelectedTransactions(); |
There was a problem hiding this comment.
When Submit is used from an ordinary advanced search rather than the live Submit suggested search, submitMoneyRequestOnSearch() updates loading/report data but does not update the Search snapshot. Clearing the selection immediately without refetching leaves the old report context in the rows, so Submit can remain offered and report-derived values stay stale after a successful submission.
Useful? React with 👍 / 👎.
| const newIDs = isChat ? reportActionsIDs : transactionsIDs; | ||
| let currentSearchResultIDs: string[] = []; | ||
| if (searchResultsData) { | ||
| currentSearchResultIDs = isChat ? extractReportActionIDsFromSearchResults(searchResultsData) : extractTransactionIDsFromSearchResults(searchResultsData); | ||
| } | ||
| const existingSearchResultIDsSet = new Set(currentSearchResultIDs); | ||
| const hasAGenuinelyNewID = (isChat ? reportActionsIDs : addedTransactionIDs).some((id) => !existingSearchResultIDsSet.has(id)); | ||
| const hasAGenuinelyNewID = newIDs.some((id) => !existingSearchResultIDsSet.has(id)); |
There was a problem hiding this comment.
When the transaction collection catches up with an item already present in the Search snapshot, transactionsIDs contains Onyx keys such as transactions_2, while extractTransactionIDsFromSearchResults() returns bare IDs such as 2. Consequently hasAGenuinelyNewID is always true for a nonempty collection, defeating the guard and issuing a redundant Search request even though the added collection entry is already displayed.
Useful? React with 👍 / 👎.
|
|
||
| // Check if there is a change in the transactions or report actions list | ||
| if ((isChat ? hasReportActionsIDsChange : hasTransactionsIDsChange || hasChangedResultTransaction) || hasPendingSearchRef.current) { | ||
| if ((!isChat && hasTransactionsIDsChange) || hasReportActionsIDsChange || hasPendingSearchRef.current) { |
There was a problem hiding this comment.
When any report action is appended while an expense or report search is open, the unconditional hasReportActionsIDsChange branch enters the refresh path even though report actions only drive chat searches. A comment or action written elsewhere can therefore reissue the active non-chat Search query and unnecessarily reload its snapshot.
Useful? React with 👍 / 👎.
|
triggered an adhoc build, let's not merge till that is confirmed fixing the bug(s) |
|
🚧 Beamanator 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! 🧪🧪
|
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
We would really want to deliver this to production as soon as possible, since it should be fixing extra Search calls putting extra load on our backend. approving this revert, but in case we could wait for monday morning to give @BartekObudzinski chance to fix the blockers, that would be nice. But its up to you!
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Working on the deploy blockers right now if the revert is not merged yet @mountiny |
|
Going ahead with the fix pr for now #98752 |
|
Both fixed with the cp so we can close this 🎉 |
|
nice!!! thanks y'all! |
3 participants
Reverts #98558
Can test adhoc build with this revert before merging
Fixes:
$ #98698
$ #98723