Conversation
Align cached search results and callback memoization to satisfy react hooks lint rules. Made-with: Cursor
melvin-bot
bot
requested review from
eVoloshchak and
trjExpensify
and removed request for
a team
|
@eVoloshchak 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] |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![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: bfddc5b092
ℹ️ 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".
| ...(oldIOUAction && !oldIOUAction.childReportID | ||
| ? { | ||
| transactionThreadReportID, | ||
| transactionThreadCreatedReportActionID, |
There was a problem hiding this comment.
The payload for ChangeTransactionsReport now includes transactionThreadReportID/transactionThreadCreatedReportActionID only when oldIOUAction exists, but a new thread is also created when oldIOUAction is missing (e.g., legacy/stale transactions with no loaded IOU action). In that path, we still create optimistic thread/report-action objects but omit their IDs from transactionIDToReportActionAndThreadData, so the backend cannot reconcile those optimistic entities correctly and the move can leave broken/duplicate pending thread state.
Useful? React with 👍 / 👎.
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@luacmartins Looks like there are changes in useSearchBulkActions that weren't in the original PR. I have left these as suggestions for your review. Otherwise, the revert LGTM.
|
@rojiphil there were conflicts in |
|
@rojiphil can you please review again? |
|
Sure... Looking into the changes. Will update soon. |
src/hooks/useSearchBulkActions.ts
Outdated
| firstTransactionPolicy, | ||
| handleDeleteSelectedTransactions, | ||
| undeleteTransactions, | ||
| currentUserPersonalDetails?.email, |
There was a problem hiding this comment.
@luacmartins currentUserPersonalDetails?.email was added by the original PR. Once this is removed, the revert LGTM
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@luacmartins LGTM. Thanks.
|
TS check is unrelated. We'll CP this so I'd rather not fix it here |
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
Revert 85818 (cherry picked from commit 4fe0948) (cherry-picked to staging by luacmartins)
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Cherry-picked to staging by https://github.com/luacmartins in version: 9.3.51-5 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help Site Review: No changes required I reviewed PR #86804 (revert of #85818) against all help site articles under What was reverted: The "undelete transactions" feature — including the Help site impact: None. The New Expensify help site articles never documented the undelete/restore feature:
The only references to restoring deleted expenses exist in Expensify Classic articles ( Conclusion: Since the undelete feature was never documented in the help site before being reverted, no documentation changes are needed. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.3.51-10 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/luacmartins in version: 9.3.52-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This PR reverts #85818, which added an "undelete" bulk action for transactions in the search UI. I searched Since the reverted feature was never documented in the help site, there's nothing to update or remove. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.52-9 🚀
|
6 participants
Explanation of Change
cc @jnowakow @rojiphil
Reverts #85818 to fix blockers. This is not a straight revert. The following conflicts were fixed:
src/components/Search/SearchList/ListItem/TransactionListItem.tsxConflict was around passing onUndelete into handleActionButtonPressUtil(...) (undelete-related param added by the PR).
src/hooks/useSearchBulkActions.tsConflict was around undelete wiring, including a useUndeleteTransactions() usage and differing names for the billing-grace-period Onyx value.
src/libs/actions/Search.tsConflict was around the HandleActionButtonPressParams shape and function args — it had undelete additions like onUndelete alongside the newer billing restriction argument (ownerBillingGracePeriodEnd).
Fixed Issues
$ #86780
$ #86782
$ #86783
$ #86788
$ #86790
$ #86791
$ #86793
$ #86796
$ #86799
$ #86850
$ #86835
Tests
Same as fixed issues
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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