Skip to content

Split useExpenseSubmission hook - #101746

Merged
mountiny merged 22 commits into
Expensify:mainfrom
callstack-internal:VickyStash/refactor/99449-split-useExpenseSubmission
Oct 6, 2026
Merged

mountiny merged 22 commits into
Expensify:mainfrom
callstack-internal:VickyStash/refactor/99449-split-useExpenseSubmission

Conversation

@VickyStash

@VickyStash VickyStash commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

useExpenseSubmission was a 1369-line hook that resolved which API command a confirmation submits through and implemented all seven of them. This splits it into one hook per submission path plus a pure resolver, so the page's submission layer can later fork per path along with the list and footer forks already landed in #99578 , #100041 and #101198.

No behavior change.

New submission hooks:

  • useDistanceSubmission.ts
  • useSplitSubmission.ts
  • useInvoiceSubmission.ts
  • useTrackExpenseSubmission.ts
  • usePerDiemSubmission.ts
  • useRequestMoneySubmission.ts
  • useSendMoneySubmission.ts

The composer still mounts all seven hooks, so no subscription is saved yet. When IOURequestStepConfirmation is split, each page variant will mount only its own submission hook (a distance variant gets useDistanceSubmission, an invoice variant useInvoiceSubmission, etc) instead of all seven. That is when the deduplicated reads actually shrink: useExpenseSubmission.ts is deleted, every TEMP: param disappears, and each hook goes back to reading the Onyx keys it needs.

Fixed Issues

$ #99449
PROPOSAL: N/A

Tests

  • Verify that no errors appear in the JS console

This is a refactor with no intended behavior change, so testing is regression-focused: every submission path must work the same way as before.

Check next submission flows:

  1. Invoice
    • with an existing invoice room
    • without one
  2. Pay
  3. Per diem
    • submit to a workspace
    • track to self-DM
  4. Distance
  5. Split
  6. Track
    • submit a tracked expense to a workspace
    • "submit to my employer" with no existing workspace
  7. Request money
    • manual
    • scan
    • multi-scan

Offline tests

Same, as in Tests section

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

  • Verify that no errors appear in the JS console

Same, as in Tests section

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Invoice
invoice-w-room.mp4
Pay
pay.mp4
Per diem
perdiem-wp.mp4
perdiem-selfdm.mp4
Distance
distance.mp4
Split
split.mp4
Track
track.mp4
Request money
scan-multi-scan.mp4
request-manual.mp4

@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

@VickyStash

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Reviewed the split of useExpenseSubmission against its pre-split version (base 54b6817). The decomposition is faithful almost everywhere — I traced all six paths param-by-param and the argument objects handed to requestMoney, trackExpense, splitBill/splitBillAndOpenReport/startSplitBill, createDistanceRequest, submitPerDiemExpense and sendInvoice are identical, and markSubmitExpenseEnd() fires on every branch and early bail exactly where it used to.

One behavior change slipped in, plus two things worth deciding on before this leaves WIP.


🔴 1. The isPolicyExpenseChat guard on useBlockDistanceRequest was dropped

Before (useExpenseSubmission.ts:360-366):

// Only a workspace destination can enforce a workspace's distance rules.
const blockDistanceRequestIfNeeded = useBlockDistanceRequest({
    policyID: isPolicyExpenseChat ? policy?.id : undefined,

After (useExpenseSubmission.ts:181-186) — the ternary and the comment explaining it are both gone:

const blockDistanceRequestIfNeeded = useBlockDistanceRequest({
    policyID: policy?.id,

policy is resolved even when the destination is not a policy expense chat: usePolicyForTransaction.ts:58-62 falls back to policyForMovingExpenses for track-creates and unreported expenses. So with a workspace that has requireMapOrGPS on, a manual-distance or odometer expense headed for the self-DM now hits getBlockReason, pops the "map or GPS required" modal and returns before setIsConfirmed(true) — the submit is silently refused where it previously went through.

The page's other useBlockDistanceRequest instance at IOURequestStepConfirmation.tsx:248 doesn't cover this — both its call sites pass an explicit policy ID rather than relying on the default. Restoring isPolicyExpenseChat ? policy?.id : undefined (and the comment) is the fix.

🟡 2. The "hoist so they open once" comment doesn't hold for the composite hooks

The TEMP block hoists the plain useOnyx keys, but each of the seven submission hooks still calls useDelegateAccountID, and that is two subscriptions each — including a PERSONAL_DETAILS_LIST read with an inline selector that linearly scans every personal detail (useDelegateAccountID.ts:10-29). That's 14 subscriptions and 7 full scans per render where the old file had 2 and 1. useParticipantsPolicyTags is likewise called twice now (split + distance), each with a freshly-allocated selector over the whole POLICY_TAGS collection, and useLocalize / useCurrencyListActions / usePermissions are duplicated 5–7×.

Since all six hooks mount together until the page forks, these belong in the same hoisted block as the rest.

🟡 3. Log prefixes changed

[useExpenseSubmission] became [useGpsCapture], [useSplitSubmission] and [usePerDiemSubmission]. Nothing in the repo greps for the old strings, but usePerDiemSubmission.ts:165 is a Log.alert, so worth confirming no saved log search or alert rule keys off the old prefix.


What I verified as equivalent
  • Dispatch order. resolveSubmissionPath reproduces the original if-chain exactly: distance → split → invoice → track → per diem → requestMoney, with the same guards (!isTrackExpense && !isSelfDMDestination && isDistanceRequest && !isMovingTransactionFromTrackExpense && !isUnreported, and per diem falling through on action === SUBMIT). Every input is render-time state and createTransaction closes over the same render's values the old one did, so no new staleness. The new resolveSubmissionPathTest.ts pins all of it.
  • GPS branch. submitWithGpsPoint collapses the duplicated receipt/zero-amount/locationPermissionGranted nest correctly, including !isSubmittingExpenseToDraftWorkspace being present on the track path only, and the deliberate absence of markSubmitExpenseEnd() on the getCurrentPosition branch.
  • Violations. Split/distance/requestMoney read transactionViolationsRef.current; per diem reads the render-time value via hasViolations. Matches the original split between the two.
  • Participant filtering. getSelectedParticipantsForSubmission reproduces splitParticipants / selectedParticipantsForRequest, including the isPolicyExpenseChat ? ownerAccountID : accountID key.
  • Cleanup. performPostBatchCleanup preserves the allTransactionsCreated bail, the preResolvedChatTarget values per path (isExpenseReport ? report : undefined etc.) and the self-DM override on track.
  • Local jest/typecheck couldn't run in this environment, but CI is green on typecheck, ESLint check, React Compiler Compliance and all 8 test shards — only checklist and Check independent approval fail, which is expected for a WIP.
Nits
  • requestType is still in UseExpenseSubmissionParams and now has no reader at all — good moment to drop it.
  • getSelectedParticipantsForSubmission is computed independently in both useSplitSubmission and useDistanceSubmission from identical inputs; it could sit next to selectedParticipants in the composer.
  • submitByPath is rebuilt every render. Harmless, but a module-level SUBMISSION_PATH → key map plus one lookup would avoid allocating six closures per render.
  • The extracted pure helpers (getTransactionTaxValues, getCurrentReceiptState, performPostBatchCleanup, getSelectedParticipantsForSubmission) are cheap to unit-test directly and would lock in the behavior this refactor is trying to preserve — right now only resolveSubmissionPath has its own test.

Next Steps: Reply with @MelvinBot <your feedback> to dig into any of these.


view run

@VickyStash

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Re-reviewed at f0c3ab1c. All three findings from my last pass are fixed correctly, and I traced the delta (f184813 → f0c3ab1c) in full — it's purely param plumbing plus the guard restore, no new behavior drift. One leftover worth a line of code, and one correction to my earlier advice.

Finding Status
isPolicyExpenseChat guard dropped on useBlockDistanceRequest ✅ restored at useExpenseSubmission.ts:184-189
useDelegateAccountID / useParticipantsPolicyTags duplicated per hook ✅ hoisted to useExpenseSubmission.ts:181-182 and passed down; participants dropped from the split and distance param types since nothing else used it
Unused requestType ✅ gone from the param type, the call site and the test builder — the remaining hits in confirmation/ are other components that genuinely use it

Both hoists are value-identical to the per-hook calls they replace (useDelegateAccountID takes no arguments, and useParticipantsPolicyTags gets the same participants ?? []), so this is a pure subscription-count win. useDelegateAccountID alone drops from 14 subscriptions and 7 full PERSONAL_DETAILS_LIST scans per render back to the base's 2 and 1.

🟡 Bring back the comment above the restored guard

The ternary is back but its explanation isn't:

const blockDistanceRequestIfNeeded = useBlockDistanceRequest({
    policyID: isPolicyExpenseChat ? policy?.id : undefined,

The original line above it read // Only a workspace destination can enforce a workspace's distance rules. A bare isPolicyExpenseChat ? policy?.id : undefined reads like a redundant null-guard, which is exactly why it got dropped during the extraction in the first place — and it'll read the same way to whoever splits this file again when the page forks into per-path variants. Worth the one line.

↩️ Correction to my earlier note — don't hoist the rest

I listed useLocalize, useCurrencyListActions and usePermissions alongside the two that mattered. Having checked them, they're all pure context reads — useLocalize.ts:6-8 and useCurrencyList.ts:10-12 are one-line useContext calls, and usePermissions.ts:16-19 is three useContext reads behind a useMemo. No Onyx subscriptions, so duplicating them across the hooks costs nothing. Leave them where they are — keeping them local is what lets each hook stand alone once the page forks.

Remaining nit and CI
  • useRequestMoneySubmission.ts:135 still opens its own ACCOUNT read for delegateEmail using the same delegateEmailSelector that the now-hoisted useDelegateAccountID uses. Not a regression — the pre-split file had both — and Onyx dedupes identical key subscriptions, so this is cosmetic only. Passing delegateEmail down alongside delegateAccountID would put all the delegate reads in one place.
  • CI is green on this head: typecheck, ESLint check, React Compiler Compliance, spellcheck, knip, Bun tests and all 8 test shards pass. Only checklist and Check independent approval fail, as expected while this is WIP.

Next Steps: Reply with @MelvinBot <your feedback> if you want me to look at anything else before this comes out of WIP.


view run

@VickyStash

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@VickyStash

Copy link
Copy Markdown
Contributor Author

@codex review

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Re-reviewed at 3966d08. The only change since my last pass is the one line I asked for, and with it every finding from all three rounds is now closed.

f0c3ab1 → 3966d08:  1 file changed, 1 insertion(+)

That insertion is the restored // Only a workspace destination can enforce a workspace's distance rules. above the guard at useExpenseSubmission.ts:184-190. Nothing else moved.

Finding Status
isPolicyExpenseChat guard dropped on useBlockDistanceRequest ✅ restored, and now explained again
useDelegateAccountID / useParticipantsPolicyTags duplicated per hook ✅ hoisted
Unused requestType ✅ removed
Missing comment above the restored guard ✅ this commit

Two closing checks I hadn't run before

The hoist is complete. Grepping the whole submission/ tree, no submission hook calls useDelegateAccountID or useParticipantsPolicyTags any more — all seven take delegateAccountID as a plain param, and split/distance take participantsPolicyTags. No stragglers left behind.

Onyx surface is byte-identical to the pre-split file. The set of ONYXKEYS.* referenced across the new composer plus the entire submission/ tree is exactly the 21 keys the old 1,204-line useExpenseSubmission.ts referenced — nothing added, nothing dropped:

ACCOUNT · BETAS · CONCIERGE_REPORT_ID · GPS_DRAFT_DETAILS · NVP_INTRO_SELECTED
NVP_ONBOARDING · NVP_QUICK_ACTION_GLOBAL_CREATE · NVP_RECENT_WAYPOINTS · ODOMETER_DRAFT
RECENTLY_USED_CURRENCIES · USER_LOCATION · COLLECTION.{POLICY_RECENTLY_USED_CATEGORIES,
POLICY_RECENTLY_USED_DESTINATIONS, POLICY_RECENTLY_USED_TAGS, POLICY_TAGS, REPORT,
REPORT_ACTIONS, REPORT_DRAFT, REPORT_NAME_VALUE_PAIRS, RULE, TRANSACTION_VIOLATIONS}

For a refactor whose main risk is a subscription silently appearing or vanishing, that's the check I'd want green before this leaves WIP — and it is.

One cosmetic leftover, and CI
  • useRequestMoneySubmission.ts:135 still opens its own ACCOUNT read for delegateEmail (needed by completeTestDriveTask), using the same delegateEmailSelector the hoisted useDelegateAccountID uses. Not a regression — the pre-split file had both reads too, and Onyx dedupes identical key subscriptions — so this is purely about keeping the delegate reads in one place. Fine to leave.
  • CI on 3966d08 is green: typecheck, ESLint check, React Compiler Compliance, Oxfmt, spellcheck, knip, Bun tests, Storybook tests, branch-perf-tests and all 8 test shards pass. Only checklist and Check independent approval fail, which is expected while the title carries [WIP].
  • Still worth doing before merge (not blocking): the PR body's Tests / QA Steps sections are unfilled, and the six paths this splits apart (requestMoney, track, split, distance, per diem, invoice) each want one manual pass, since the safety net here is the existing useExpenseSubmission suite rather than tests written against the new hooks.

From my side this reads as behavior-preserving against base 54b6817 and ready to come out of WIP.


Next Steps: Reply with @MelvinBot <your feedback> if you want another look after the test steps go in.


view run

@VickyStash VickyStash changed the title [WIP] Split useExpenseSubmission Split useExpenseSubmission Sep 22, 2026
Comment thread src/pages/iou/request/step/confirmation/submission/useGpsCapture.ts
@VickyStash

Copy link
Copy Markdown
Contributor Author

@situchan kind bump here ^^

@VickyStash

Copy link
Copy Markdown
Contributor Author

@situchan any ETA on review?

@situchan

situchan commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

started reviewing

@situchan

situchan commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
test1.mp4
test2.mov

@situchan

situchan commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Regression test completed. No blockers found

@melvin-bot
melvin-bot Bot requested a review from mountiny October 6, 2026 09:44
@MelvinBot

MelvinBot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Web test steps verification results: ⚠️ partial

I found no regressions. On web, manual request, track, track → submit, distance, pay and invoice all created the expense on the right report with no failed submit calls. Scan and per diem need a file upload, which the test harness can't do, and the split run likely didn't exercise the split path. A code check of the 2026-09-29 main merge (1f730810) and fa1972a7 found every main change ported into the new hooks: [Performance] Move Search and submit-expense onto write-readiness barriers and delete deferredLayoutWrite, Remove dead betas plumbing, Remove getPersonalDetailByEmail from getMemberAccountIDsForWorkspace #3 and refactor reportDrafts explicitly in the expense confirmation flow.
Invoice room

Step Status
Request money (manual)Submitted $25.00 "Melvin Coffee Co" to the workspace. The report shows Submitted with the correct amount and merchant.
Manual expense submitted
✅
Request money (scan)Not run. Uploading a receipt image isn't supported by the web test harness.
⚠️
Track (self-DM)Tracked $12.50 "Melvin Taxi Ride" in the self-DM. The submit / categorize options appeared.
Tracked expense in self-DM
✅
Track → submit to workspace"Submit to my employer" moved the $12.50 expense into the workspace expense report, marked submitted.
Tracked expense submitted to workspace
✅
SplitA $40.00 expense with melvinsplitpartner@example.com shows "owes $40.00". That looks like a normal request, not a split (an even split would show $20.00), so the split path was probably not exercised. Please recheck split manually.
Split attempt chat
⚠️
Distance (manual)Submitted 15 miles ($11.40 at the default rate) to the workspace. The report shows the distance and amount, marked Submitted.
Distance expense submitted
✅
Pay (elsewhere)Paid $20.00. The DM shows "paid $20.00 elsewhere".
Paid elsewhere
✅
Per diemNot run. Per diem was enabled, but rates can only be added by importing a spreadsheet, and the web test harness can't upload files.
⚠️
Invoice (new recipient)The first try returned the backend error "Company website is invalid", and the app rolled back correctly. The retry with a valid website created the invoice room with "owes $75.00".
Invoice sent
Invoice room
✅
No JS console errorsThe console couldn't be read directly. Network traffic was checked instead: RequestMoney, CreateDistanceRequest, SendInvoice and SubmitReport all returned 200. The only non-200 response was one unrelated AuthenticatePusher 403.
⚠️

view run · view recording

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Code-quality review at fa1972a7: the split is clean and nothing blocks merging. The new hooks use no useMemo/useCallback, casts, ! or any, and knip and React Compiler pass. Most of what's left is stale or dropped comments, plus two small duplications the split created.

Should fix

  1. Why-comments were lost in the extraction. The old hook explained each of these. The new code doesn't:
  2. The split left some comments stale:
    • useDistanceSubmission.ts:138 says "Same reasoning as above", but that reasoning is now in another file. Restate it.
    • useExpenseSubmission.ts:168 says "six" hooks. Seven mount.
    • useExpenseSubmission.ts:393 says "once this file is split up". It is split now, so point it at the coming page split.
    • cleanupAfterSkipConfirmSubmit.ts:8 and cleanupAndNavigateAfterExpenseCreate.ts:39 in src/libs/Navigation/helpers/ still say this logic lives in useExpenseSubmission.
  3. The chat-report lookup is now copied in four places. The same getReportOrDraftReport(id, undefined, undefined, reportDrafts?.[REPORT_DRAFT + id] ?? {}) call appears in useTrackExpenseSubmission.ts:138-141, useDistanceSubmission.ts:113-116, usePerDiemSubmission.ts:179-181 and performPostBatchCleanup.ts:75. The old hook computed it once. Move it into one submission/utils helper.
  4. useGpsCapture has no test. It is the one piece the PR restructured: it merges two GPS branches into one. Its true return value drives pendingWrite.settleAfterSubmit. Add a hook test for three cases: a cached location, a live lookup that returns true, and no location permission.
Nice to have
  • Header comments (CONSISTENCY-13): 8c0e8e61 added JSDoc, but it sits below a type or const in most files, so the bot will likely flag them again if it re-runs. These files have no comment at all: useDistanceDraftData.ts, useSubmissionViolations.ts, getTransactionTaxValues.ts, getSelectedParticipantsForSubmission.ts, getCurrentReceiptState.ts, logSubmittedReceiptMilestone.ts.
  • Pass params straight through: useExpenseSubmission.ts:416 applies defaults, rebuilds the same object at line 432, and then every hook applies the same defaults again. Pass params through unchanged so the defaults live in one place.
  • Dead params: on the request-money path, isCategorizingTrackExpense and isSharingTrackExpense are always false, because resolveSubmissionPath.ts:41-45 sends those cases to track or per diem first. You can drop them from useRequestMoneySubmission.ts:71-72 and the check at line 358. This check also existed before the split.
  • Pure-helper tests: getTransactionTaxValues and the zero-amount splitShares filter in getSelectedParticipantsForSubmission have no tests. A table test like resolveSubmissionPathTest would be cheap.
  • Duplicated distance logic (existed before the split): the per-item distance math at useTrackExpenseSubmission.ts:192-198 repeats useDistanceDraftData.ts:22-29.
  • Comment placement: in useSendMoneySubmission.ts:28-33, onExpenseWriteWillStart sits under the "TEMP: hoisted Onyx keys" doc block. Move it above, as the other hooks do.

view run

@mountiny mountiny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I feel like we should create unit test files for the helper utils?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add unit test for this specifically?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, shoule we make submission utils test file?

@mountiny
mountiny merged commit af3c97e into Expensify:main Oct 6, 2026
36 of 38 checks passed
@mountiny

mountiny commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

We will add the unit tests in follow up

@OSBotify

OSBotify commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify OSBotify mentioned this pull request Oct 6, 2026
100 tasks done
@OSBotify

OSBotify commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.4-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site update needed. This PR only splits the internal useExpenseSubmission hook into one hook per submission path, and it doesn't change any feature, label, or workflow that users see.


view run

@OSBotify

OSBotify commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.4-3 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants