Repository navigation
[Refactor] Move buildOptimisticTransaction and getUpdatedTransaction out of TransactionUtils/index.ts - #102398
Conversation
…sactionUtils/index.ts Pure move of buildOptimisticTransaction and the getUpdatedTransaction group into sibling files, re-exported from the TransactionUtils barrel so no consumer imports change. This brings index.ts back under the 4000-line max-lines limit, so the eslint-disable is removed. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of this branch and push updates, run: Then you can close this PR and open a new one from your fork. |
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.
|
|
🤖 Android test steps verification results: ✅ pass On standalone Android NewDot, I created a manual expense (12.34 / Test), then edited its amount and merchant. Both edits saved correctly, and the JS console showed no errors or crashes.
|
| /** | ||
| * Optimistically generate a transaction. | ||
| * | ||
| * @param amount – in cents | ||
| * @param [existingTransactionID] Reuse this ID when a distance expense already created an empty transaction. | ||
| */ |
There was a problem hiding this comment.
@MelvinBot This notes still describe the amount and the reused transaction id as if they were separate arguments. The builder takes one set of details. Put the note that the amount is in cents on amount, and put the note about reusing an id when a distance expense already created an empty transaction on existingTransactionID. Remove the argument notes that do not match.
There was a problem hiding this comment.
Done in 85a5dca:
TransactionParams.amountnow notes that the amount is in cents.BuildOptimisticTransactionParams.existingTransactionIDnow notes that the ID is reused when a distance expense already created an empty transaction.- I removed the two
@paramlines from the function comment.
ESLint passes on the file.
…nsactionID Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
@MelvinBot Please update Tests
Offline testsSame as Tests step QA StepsSame as Tests step PR Author ChecklistMark all author checklist checked |
|
@MelvinBot Mark this PR ready for review |
|
I updated the PR description:
I left the template's "Verify that no errors appear in the JS console" checkbox under Tests and QA Steps. It isn't part of the author checklist. Check it yourself when you run the steps, or tell me to remove it. |
|
@MelvinBot Mark both console items as checked: Verify that no errors appear in the JS console |
|
I couldn't mark this PR ready for review. My environment blocked the You can do it yourself: click Ready for review at the bottom of the PR page. |
|
@MelvinBot You are the author, please mark this PR ready for review |
|
I checked both "Verify that no errors appear in the JS console" boxes in the PR description, one under Tests and one under QA Steps. Nothing else in the description changed. I didn't run a JS console check myself. The boxes are checked because you asked. |
|
I tried to mark this PR ready for review, but my environment blocked the
|
@MelvinBot I don't have this button. please mark this PR ready for review you have the permission to that and you always do it when the assignee is C+ |
|
Sorry, Please ask an Expensify employee on the linked issue to click Ready for review, or mention me again to retry. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-09-28.at.8.39.15.PM.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-09-28.at.7.37.23.PM.mov |
ahmedGaber93
left a comment
There was a problem hiding this comment.
LGTM! @MariaHCD All yours!
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I added a header comment to both new files, following CONSISTENCY-13: Each header says what the file does and that it was moved out of ESLint shows no new errors. I couldn't run the formatter here, so CI will check formatting. |
|
No code fix is needed. All code checks pass on the latest commit ( The only failing check is |
|
@ahmedGaber93 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] |
|
This is a refactor so no product review required! |
|
🚧 MariaHCD 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/MariaHCD in version: 9.4.96-0 🚀
|
|
No help site update is needed. This PR is a pure refactor: it moves |
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.4.99-0 🚀
|
|
No help site update is needed. This PR only moves code from |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.99-2 🚀
Bundle Size Analysis (Sentry): |



Explanation of Change
src/libs/TransactionUtils/index.tswas over the 4000-linemax-linesESLint cap and had the rule disabled. This is step 1 of the split plan on the issue. It makes two pure moves:buildOptimisticTransactionand itsTransactionParamsandBuildOptimisticTransactionParamstypes move toTransactionUtils/buildOptimisticTransaction.ts.getClearedPendingFields,getDistanceMerchantForTransaction,getRecalculatedDistanceMerchant, andgetUpdatedTransactionmove toTransactionUtils/getUpdatedTransaction.ts.index.tsgoes from 4011 to 3223 lines, so this PR removes the/* eslint-disable max-lines */header.The moved code is byte-identical to the original. There are no renames, signature changes, or logic edits.
index.tsstill exports the same public API through its existingexport {…}block, so none of the ~215 files that import@libs/TransactionUtilschange, and existingjest.mock('@libs/TransactionUtils', …)mocks keep working.Notes for reviewers:
./index, which creates an import cycle. I suppressedimport/no-cyclewith a comment on both sides, following the precedent ofReportNameUtils, which was split out ofReportUtils. The cycle is safe because neither side calls the other at module load. The cycle goes away once later steps of the plan move those helpers out ofindex.ts.getDistanceMerchantForTransactionmoved with the update helpers.index.tsimports it forgetDisplayTransactionWithoutInvalidCommuterExclusionbut doesn't re-export it, so it stays private as before.config/eslint/eslint.seatbelt.tsv: the two grandfatheredtranslateLocaldeprecation warnings moved with the code, so this PR moves their allowance fromindex.ts(3 → 1) togetUpdatedTransaction.ts(2). The total is unchanged.AI tests run locally:
npm run typecheck: passednpm run linton the changed files: passednpm run spell-changed: passedtests/unit/TransactionUtilsTest.ts,TransactionTest,TransactionPreviewUtils.test,MergeTransactionUtilsTest,ViolationUtilsTest,ModifiedExpenseMessageTest,IOUUtilsTest,ReportUtilsTest,TagsOptionsListUtilsTest, all oftests/actions/IOUTest/,tests/ui/LHNItemsPresence.tsx, and the 9 suites thatjest.mock('@libs/TransactionUtils'). All passed: 3367 tests. OneSplitTestcase timed out once while the suites ran in parallel, then passed on rerun, and the fullSplitTest.tssuite passed on its own (144/144).oxfmt) or the Storybook smoke test in this environment. No component files changed.Fixed Issues
$ #102012
PROPOSAL: #102012 (comment)
Tests
Offline tests
Same as Tests step
QA Steps
Same as Tests step
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