Refactor mergeDuplicates to pass currentUserData#88292
Refactor mergeDuplicates to pass currentUserData#88292tgolen merged 5 commits intoExpensify:mainfrom
Conversation
|
@ikevin127 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] |
|
@danieldoglas 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e6027143d
ℹ️ 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".
| transactionsMergeParams.transactionThreadReportID = transactionThreadReportID; | ||
| } | ||
| mergeDuplicates(transactionsMergeParams); | ||
| mergeDuplicates({...transactionsMergeParams, currentUserAccountID: currentUserPersonalDetails.accountID, currentUserLogin: currentUserPersonalDetails?.login ?? ''}); |
There was a problem hiding this comment.
Keep a non-empty login when creating optimistic thread action
currentUserPersonalDetails.login is optional, and CurrentUserPersonalDetailsProvider only guarantees accountID (and backfills email, not login) before personal details fully load. Passing currentUserPersonalDetails?.login ?? '' can therefore send an empty string into mergeDuplicates, which then builds the optimistic CREATED action with blank actor text (" created this report") when a new thread is created. This regresses the optimistic UI in startup/offline timing windows; use a fallback that includes the guaranteed email/session value.
Useful? React with 👍 / 👎.
| currentUserLogin: RORY_EMAIL, | ||
| currentUserAccountID: RORY_ACCOUNT_ID, |
There was a problem hiding this comment.
Avoid asserting transient user fields in API param matcher
These new properties are now part of mergeParams, but mergeDuplicates explicitly strips currentUserLogin and currentUserAccountID before calling API.write (allParams is built from ...params after destructuring). As a result, existing assertions like expect.objectContaining(mergeParams) now require keys that are intentionally absent from the API payload, causing the updated unit tests to fail even when behavior is correct.
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-04-20.at.13.56.08.mov |
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM
The PR refactors the mergeDuplicates function to receive currentUserLogin and currentUserAccountID as parameters instead of reading them internally from Onyx via getCurrentUserEmail() and getUserAccountID(). This follows the Onyx best practice pattern of parameterizing state access for better testability ✅
|
@ikevin127, can you please tag @tgolen after your review on these PRs going forward? |
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.
|
|
@tgolen Ready. |
|
@tgolen Ready for merge. |
|
🚧 @tgolen 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/tgolen in version: 9.3.62-5 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. It's a purely internal code refactoring that changes the There are no user-facing changes — no new features, UI changes, or modified terminology. No help site documentation changes are required. |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.3.64-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Fixed Issues
$ #66519
PROPOSAL:
Tests
keep oneof the expenses.Offline tests
QA Steps
same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
19.04.2026_21.50.22_REC.mp4