refactor trackExpense#79944
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@DylanDylann 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] |
|
@MarioExpensify 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.
Product review not required.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52418f1d78
ℹ️ 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".
src/libs/actions/IOU/index.ts
Outdated
| introSelected: OnyxEntry<OnyxTypes.IntroSelected>; | ||
| activePolicyID: string | undefined; | ||
| quickAction: OnyxEntry<OnyxTypes.QuickAction>; | ||
| allTransactionDrafts: OnyxCollection<OnyxTypes.Transaction>; |
There was a problem hiding this comment.
Avoid serializing full draft collection into retry params
Adding allTransactionDrafts to CreateTrackExpenseParams means trackExpense’s retryParams (built via ...params) now carries the entire draft collection, and getReceiptError JSON‑stringifies it. When a receipt upload fails and the user retries, those drafts already contain errors.retryParams, so each subsequent failure nests the previous payload and can balloon quickly. That can bloat Onyx error data and noticeably slow retries; consider omitting allTransactionDrafts when constructing retryParams (or pass it separately to removeDraftTransactions).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I already omitted allTransactionDrafts from retryParams
|
@dukenv0307 Please merge main again |
|
@dukenv0307 Could you fix the failed eslint? |
|
@DylanDylann Done |
|
Looks like we have many conflicts. |
|
@dukenv0307 The change looks good to me. Lots of conflict on my side that prevents me from testing after merging the latest main. Could you please fix the conflict? |
|
Bump @dukenv0307 for checking the conflicts |
|
@DylanDylann @MarioExpensify I think we should wait for this PR to be merged first |
|
@MarioExpensify I just merged main to resolve the conflicts. We're good to merge now |
|
@dukenv0307 Failed Eslint |
|
@dukenv0307 please check the failing ESLint and feel free to ping me when we're ready to merge |
|
@MarioExpensify I fixed the lint issue |
|
Sorry guys, another conflict has shown up 🤦 @dukenv0307 please fix the conflict. |
|
@dukenv0307 Failed Eslint |
d02ea1a to
ed9e3d7
Compare
|
@DylanDylann I fixed |
|
cc @MarioExpensify For final review |
|
@dukenv0307 another round of conflicts.... this is being hard. Please ping me in Slack if you fix this today so I can merge it as soon as it is ready so we don't have any more conflicts. CC: @DylanDylann |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @MarioExpensify 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MarioExpensify in version: 9.3.36-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.36-10 🚀
|
Explanation of Change
Fixed Issues
$ #67778
PROPOSAL:
Tests
Offline tests
QA Steps
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari