Refactor TransactionInlineEdit actions into pure functions (remove connectWithoutView) - #99279
Conversation
Remove the module-level Onyx.connectWithoutView subscriptions from TransactionInlineEdit and pass all Onyx data in from useTransactionInlineEdit via useOnyx, matching the non-inline edit flow. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
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.
|
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Fixed the failing Oxfmt check and pushed to the branch ( Cause: The only other red check was What I ran
|
|
🤖 Ran the testing steps on Android: Native. Since this PR is a behavior-preserving refactor of the transaction inline-edit actions, I verified expense field editing round-trips end to end. All exercised fields pass.
No errors or crashes occurred during any edit, and each change persisted through Onyx/save and rendered on the expense details page.
EvidenceExpense details after merchant edit to The Daily Grind CafeExpense details after description editExpense details after amount edit to EUR 42.50Expense details after date edit to 2026-08-15Category field shows Collect-plan upsell (personal space plan limitation)Final expense details showing all saved field values |
|
🤖 I attempted the testing steps on Web (wide/desktop layout) but could not drive the inline-edit UI end to end with the automated harness. Reporting transparently rather than claiming a pass. What I could verify
What's blocked (and why)
Relevant context
Recommendation: the wide-layout Search-table inline editing needs a hover-capable path to validate (e.g. the App repo's EvidenceSearch/Expenses table in wide layout with editable expense rowsSynthetic click on the Date cell navigates into the report instead of revealing the hover-gated inline-edit pencil |
| function editTransactionMerchantInline(params: TransactionInlineEditParams, newMerchant: string) { | ||
| const transaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`]; | ||
| const iouParams = getIouParamsForTransaction(params); | ||
|
|
||
| if (!isValidMerchant(newMerchant, transaction, params.parentReport)) { | ||
| if (!isValidMerchant(newMerchant, iouParams.transaction, params.parentReport)) { | ||
| return; | ||
| } | ||
|
|
||
| const iouParams = getIouParamsForTransaction(params); | ||
|
|
There was a problem hiding this comment.
editTransactionMerchantInline now builds iouParams before the isValidMerchant early-return, which reorders a side effect ahead of validation. getIouParamsForTransaction can call createTransactionThreadReport(...), and that isn't pure — it fires Onyx.merge(...) + openReport(...) (an API call) when the transaction thread report doesn't exist yet (common in Search). So entering an invalid merchant (e.g. clearing merchant on a reported expense, a PARTIAL/DEFAULT sentinel, or an over-length name) now optimistically creates a thread report and fires openReport before discarding the edit. The old code validated first, so nothing was created. This is a behavior regression on a reachable path.
Suggested fix — validate against the transaction read directly, and only build params after it passes (restores original ordering):
function editTransactionMerchantInline(params: TransactionInlineEditParams, newMerchant: string) {
const transaction = params.transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`];
if (!isValidMerchant(newMerchant, transaction, params.parentReport)) {
return;
}
const iouParams = getIouParamsForTransaction(params);
updateMoneyRequestMerchant({ ...iouParams, ... });
}
There was a problem hiding this comment.
Good catch — confirmed and fixed in daa060f.
Root cause: editTransactionMerchantInline built iouParams before validating, and getIouParamsForTransaction calls createTransactionThreadReport when no thread report exists yet — which is not pure: it fires Onyx.merge(...) (Report/index.ts:2463-2466) and openReport(...) (Report/index.ts:2478, an API call). So an invalid merchant (empty on a reported expense, a PARTIAL/DEFAULT sentinel, or an over-length name) optimistically created a thread report and hit the API before the edit was discarded.
Fix: Read the transaction directly from params.transactions and validate first; only build iouParams after validation passes — restoring the original ordering (TransactionInlineEdit.ts:257-269).
Also fixed the same pattern in editTransactionAmountInline — it likewise built iouParams before the isValidMoneyRequestAmount early-return, so an invalid amount had the identical side effect. It now validates against params directly first (TransactionInlineEdit.ts:298-311). (allowNegative used iouParams.policy, which is just params.policy passed through, so no behavior change there.)
Checks run locally
npm run linton the changed file — cleannpm run spell-changed— 0 issuesnpm test tests/unit/inlineEditing/TransactionInlineEdit.test.ts tests/actions/IOUTest/UpdateMoneyRequestTest.ts— 52/52 passed
| const transaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]; | ||
| const transactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]; | ||
| const isUnreportedExpense = !transaction?.reportID || transaction.reportID === CONST.REPORT.UNREPORTED_REPORT_ID; | ||
|
|
||
| let resolvedParentReport = parentReport; | ||
| if (!resolvedParentReport?.reportID && transaction?.reportID && transaction.reportID !== CONST.REPORT.UNREPORTED_REPORT_ID) { | ||
| resolvedParentReport = allReports[`${ONYXKEYS.COLLECTION.REPORT}${transaction.reportID}`]; | ||
| } | ||
|
|
||
| let resolvedParentReportAction = parentReportAction; | ||
| if (!resolvedParentReportAction && resolvedParentReport?.reportID) { | ||
| const reportActions = allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${resolvedParentReport.reportID}`] ?? {}; | ||
| resolvedParentReportAction = getIOUActionForTransactionID(Object.values(reportActions), transactionID); | ||
| } | ||
|
|
||
| if (isUnreportedExpense) { | ||
| const selfDMReportID = findSelfDMReportID(allReports); | ||
| if (selfDMReportID) { | ||
| resolvedParentReport = allReports[`${ONYXKEYS.COLLECTION.REPORT}${selfDMReportID}`] ?? resolvedParentReport; | ||
|
|
||
| if (!resolvedParentReportAction) { | ||
| const selfDMReportActions = allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${selfDMReportID}`] ?? {}; | ||
| resolvedParentReportAction = getIOUActionForTransactionID(Object.values(selfDMReportActions), transactionID); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@MelvinBot Please explain why you remove this logic?
There was a problem hiding this comment.
That logic wasn't dropped — it moved into the hook (useTransactionInlineEdit) and is now passed in as params, which is the whole point of this refactor (making the action a pure function instead of reading global Onyx collections directly). The action re-deriving parentReport / parentReportAction / the self-DM fallback from allReports / allReportActions / allTransactions was redundant with what the caller already resolves via useOnyx.
Here's the 1:1 mapping of each removed piece to where the caller now does the same thing:
-
transaction/transactionViolationsfrom the global collections → the caller passes scoped collections (the transaction + its duplicates) fromuseDuplicateTransactionsAndViolationsat useTransactionInlineEdit.ts:151-152, and the action reads the row from that param at TransactionInlineEdit.ts:190-191. -
resolvedParentReportfallback (readallReports[transaction.reportID]) → the caller reads it withuseOnyxand folds in the unreported/self-DM case: useTransactionInlineEdit.ts:88-96 (effectiveParentReport = isUnreported ? selfDMReport : parentReport), passed in asparentReportat useTransactionInlineEdit.ts:191. -
resolvedParentReportActionfallback (getIOUActionForTransactionIDoverallReportActions) → same function, now in auseOnyxselector at useTransactionInlineEdit.ts:100-108, passed in asparentReportActionat useTransactionInlineEdit.ts:192. -
isUnreportedExpense+findSelfDMReportID(allReports)self-DM resolution → handled byuseSelfDMReport, which resolves the self-DM report via the sameisSelfDM(report) && !isThread(report)lookup thatfindSelfDMReportIDuses (useSelfDMReport.ts:12-17). For an unreported expense,effectiveParentReportIDbecomes that self-DM report ID, so the sameuseOnyx(REPORT_ACTIONS + effectiveParentReportID)selector recovers the self-DM report action too.
So no resolution behavior was lost — the reads just moved from Onyx.connectWithoutView/global-collection access inside the action to useOnyx in the hook, per the Onyx data-management guidance. The only in-action resolution kept is the createTransactionThreadReport fallback for when a thread report doesn't exist yet.
If you'd prefer I keep any of that resolution inside the action instead, let me know and I'll adjust.
…oid side effects Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
@MelvinBot Fix conflict |
|
I attempted Why: A recently merged PR (#99189,
So the merge isn't a mechanical "take one side": the correct resolution has to fold main's new resolution logic (report/reportAction/self-DM derivation) into the pure-params approach — most likely by confirming the caller ( Conflicting files:
Recommended: resolve locally with |
|
@MelvinBot Please go ahead and resolve it with the pure-params approach (do not re-introduce main's
After resolving, please run |
…EditConnectWithoutView Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com> # Conflicts: # src/hooks/useTransactionInlineEdit.ts # src/libs/actions/TransactionInlineEdit.ts
|
Merged How each item was folded in:
Two deviations worth your eyes (both forced by lint/type gates):
I also updated main's new Checks run locally
|
|
There's no code fix needed here — all real CI checks pass. The only failing check is Check independent approval, which is not a code failure. It fails because this PR was opened by Everything else is green: all 8 So nothing to push — this just needs a human reviewer to approve. |
|
@MelvinBot Please update Tests
Offline testsSame as Tests step QA StepsSame as Tests step PR Author ChecklistMark all author checklist checked |
|
Works as expected. Screen.Recording.2026-08-27.at.10.35.09.PM.movScreen.Recording.2026-08-27.at.10.36.22.PM.mov |
|
Done ✅ — I updated the PR description and verified inline editing on Web. PR body updates
Test run (Web, large screen): ✅ pass — Inline editing of Amount, Date, Merchant, Description, Category, and Tag all worked and persisted, both from the Spend → Expenses (Search) row detail and from the same Workspace expense report opened via Inbox. No failed network calls observed during editing.
Note on methodologyThe compact Search-table row exposes hover-only per-cell pencil icons; the headless browser driver couldn't reliably trigger real ScreenshotsSpend > Expenses Search table with the two created expenses
Amount inline edit screen showing new value before save
Date inline edit calendar picker
Merchant inline edit with updated text
Description inline edit
Search table showing all edited fields persisted
Same workspace expense report opened via Inbox showing edited fields and a live Category edit
|
|
@MelvinBot Mark this PR ready for review |
|
@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] |
| import {useMemo} from 'react'; | ||
| // Use the original useOnyx hook to get the real-time data from Onyx and not from the snapshot | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import {useOnyx as originalUseOnyx} from 'react-native-onyx'; |
There was a problem hiding this comment.
"Original Onyx" doesn't really make sense, how about renaming it to useOnyxWithoutSnapshots?
There was a problem hiding this comment.
how about renaming it to
useOnyxWithoutSnapshots?
@tgolen It looks more meaningful, but it also seems inconsistent with the naming convention in other parts of the app. I’d suggest using originalUseOnyx to keep it consistent with the rest of the app, as we already have 65 usages of it.
Please let me know if you’d like me to rename it here.
There was a problem hiding this comment.
Yes, please let's rename it. Also, I'd pay to have all the other references renamed in another PR too 😁
There was a problem hiding this comment.
@MelvinBot Please rename originalUseOnyx above to useOnyxWithoutSnapshots, only in the useDuplicateTransactionsAndViolations.ts file.
There was a problem hiding this comment.
Renamed in 2dea13e5. All three occurrences in src/hooks/useDuplicateTransactionsAndViolations.ts now use useOnyxWithoutSnapshots — the import alias on L10 and the two call sites in useLiveDuplicateTransactionsAndViolations (L161, L169). No other file was touched, so the 65 existing originalUseOnyx usages elsewhere are unchanged.
One thing to confirm: I left the explanatory comment above the import as-is, since you asked for it word-for-word from TransactionListItem:
// Use the original useOnyx hook to get the real-time data from Onyx and not from the snapshotIt still says "the original useOnyx hook", which is the phrasing tgolen objected to. Say the word if you'd like it reworded to match the new alias.
Checks run locally
eslinton the changed file — passednpm run typecheck— passednpm run react-compiler-compliance-check check— passednpm run spell-changed— 0 issuesnpm test tests/unit/inlineEditing/TransactionInlineEdit.test.ts— 26/26 passed
No UI verification was run: this is an import-alias rename with no behavior change, and the identifier is local to the file.
There was a problem hiding this comment.
@MelvinBot Please open a new PR to rename all other originalUseOnyx references throughout the app to useOnyxWithoutSnapshots.
There was a problem hiding this comment.
Opened #100200 (draft) — renames the remaining originalUseOnyx references app-wide.
Scope: 13 files, 55 occurrences. Every one is the import {useOnyx as originalUseOnyx} alias or one of its call sites, so the diff is a pure rename with no behavior change.
Two spots needed more than a find-and-replace:
src/hooks/useOnyx.ts— the type aliasOriginalUseOnyx(justtypeof originalUseOnyx) becameUseOnyxWithoutSnapshots, so the wrapper's own file doesn't keep the old terminology.tests/ui/CategoryPickerTest.tsx— themockedOriginalUseOnyxvariable and the comment that names the alias in backticks.
Two things need your call:
- The prose comments still say "original". Six files carry
// Use the original useOnyx hook to get the real-time data from Onyx and not from the snapshotabove the import. I left them untouched because you asked for that exact wording word-for-word here. That meanstgolen's objection to the term survives in the comments even though the identifier is gone. Tell me how you'd like them worded and I'll push a follow-up. Fixed Issuesis blank. This rename isn't fixing a tracked issue — it came out of this thread. Add a link or mark it[No QA]before flipping it to ready.
Checks run locally
npm run typecheck— passednpm run lint-changed— passednpm run react-compiler-compliance-check checkon the 12 changed source files — passednpm run spell-changed— 13/13 files, 0 issuesnpm test tests/ui/CategoryPickerTest.tsx— 3/3 passednpm test tests/unit/inlineEditing— 43/43 passed
No UI verification: this is an identifier rename with no runtime change, and typecheck plus the mock-driven CategoryPicker test cover the one file where the alias is wired through a jest mock.
I don't have push access to your fork, so the branch lives on upstream — there are instructions on the PR for moving it to your fork if you'd rather own it there.
|
@MelvinBot Please fix the git conflict |
…sactionsAndViolations Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
…EditConnectWithoutView Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com> # Conflicts: # tests/actions/IOUTest/UpdateMoneyRequestTest.ts
|
Conflict resolved — merged The conflict: one hunk in Resolution: kept both — Verification after the merge
No UI verification was run — the only conflicted file is a unit test, and the resolution changes no product code. |
|
The failed unit test is not related to this PR 
|
ahmedGaber93
left a comment
ahmedGaber93
left a comment
There was a problem hiding this comment.
Updated!
CC @tgolen
|
@MelvinBot Please merge the latest main branch to fix the unit test |
…EditConnectWithoutView Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Merged What was failing: Why merging main fixes it: main already carries a rewrite of that test which pins the clock with The merge was clean — no conflicts. Verified locally after the merge
|
|
All yours @puneetlath @tgolen |
|
🚧 puneetlath 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/puneetlath in version: 9.4.70-0 🚀
|
|
🤖 No help site changes are required for this PR. I did not create a docs PR. Why: this is a pure internal refactor. It moves Onyx reads out of What I checkedFiles changed — all internal source and tests, none under
Existing help site coverage of inline editing — I read the three articles that document it, and each still describes the behavior correctly after this PR: The editable fields the PR's test plan lists (Amount, Date, Merchant, Description, Category, Tag) and the entry points (Spend → Expenses, and expense reports from Inbox) are unchanged, so the articles need no edits. @ahmedGaber93, there is no help site PR to link — this refactor doesn't change anything documented on the help site. If you believe a user-facing behavior did change here, reply with what changed and I'll draft the docs PR. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.72-0 🚀
|
|
No help site changes are required for this PR. I did not create a docs PR. Why: This is a pure refactor with no user-facing change. It removes seven module-level What I checkedAll five changed files are source or test files:
Nothing here changes a feature name, a settings label, a button, a menu path, or a user-visible flow. The PR body states it directly: "Behavior is unchanged — this is a pure refactor." The test plan only asks a reviewer to confirm existing inline editing still works, which means no documented behavior moved. Help site articles under There is no linked help site PR to review. If you think a specific article is now out of date, tell me which one and I'll create the draft PR. @ahmedGaber93, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
5 participants
Explanation of Change
This is follow-up item #7 from the inline-editing follow-up list: refactor
connectWithoutView, as flagged bytgolenin this review comment ("None of theseconnectWithoutView()references should be here. They need to be refactored and removed to be pure functions.").src/libs/actions/TransactionInlineEdit.tsheld seven module-levelOnyx.connectWithoutView()subscriptions (TRANSACTION,TRANSACTION_VIOLATIONS,REPORT,REPORT_ACTIONS,SESSION,BETAS,NVP_INTRO_SELECTED) and read that mutable module state inside its edit actions. That runs against the Onyx data-management guidance ("prefer pure functions taking data as params over direct Onyx reads").What changed:
TransactionInlineEdit.ts— removed all sevenconnectWithoutView()subscriptions (and theimport Onyx). Every value they provided is now a parameter onGetIouParamsInput(transactions,transactionViolations,betas,introSelected,currentUserAccountID,currentUserEmail), so the edit actions are now pure. The report/report-action/thread resolution that the old code re-derived from the global collections was already being resolved by the caller and passed in (parentReport,parentReportAction,transactionThreadReport), so those redundant global-collection fallbacks were dropped; the only remaining resolution is the existingcreateTransactionThreadReportfallback.useTransactionInlineEdit.ts— the caller now reads those values viauseOnyxand passes them in. The scopedtransactions/transactionViolationscollections come fromuseDuplicateTransactionsAndViolations([transactionID])— the exact same pattern the non-inline edit flow already uses inDynamicIOURequestStepDate— so we get the transaction plus its duplicates (needed byremoveTransactionFromDuplicateTransactionViolation) without subscribing every row to the fullTRANSACTION/REPORT/REPORT_ACTIONScollections.UpdateMoneyRequestTest.ts— updated the directeditTransactionMerchantInlineunit test to pass the new params (it previously relied on the module-level subscriptions).Behavior is unchanged — this is a pure refactor.
Automated checks run locally
npm run typecheck-tsgo— passedeslinton the changed files — 0 errors (one pre-existing grandfathered warning, unrelated)npm run react-compiler-compliance-check checkon the changed files — passednpm run spell-changed— passednpm test tests/unit/inlineEditing/TransactionInlineEdit.test.ts— 17/17 passednpm test tests/actions/IOUTest/UpdateMoneyRequestTest.ts— 35/35 passedFixed Issues
$ #82534
Tests
This is a refactor PR, so there are no end-user changes. We only need to confirm that the inline editing feature still works as expected without any regressions.
Test on Web large screen only
Create different types of expenses, for example: workspace expense, IOU, self-DM expense, track expense, split expense, invoice, and per-diem.
Go to Spend → Expenses.
Verify that you can still inline-edit the following fields:
Go to Inbox → Workspace expense reports with transaction lists and repeat step 3.
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