Fix invoice Pay from Search transaction rows (resolve chatReport in PayActionCell)#95873
Fix invoice Pay from Search transaction rows (resolve chatReport in PayActionCell)#95873MelvinBot wants to merge 8 commits into
Conversation
…transaction rows Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
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: Pujan Shah <Pujan92@users.noreply.github.com>
|
Fixed the failing Spell check — pushed Cause: cspell flagged the word The other failing check, |
|
Update: after my push ( The only remaining failing check is |
|
✅ The Spell check is now green after my earlier fix ( The only remaining red check is |
|
Two of the three failing checks are handled; the third is unrelated to this PR. ✅ Spell check — fixed in
This PR only changes I did not touch those files or the shared seatbelt baseline, since that's outside this PR's scope and would absorb an unrelated change's violations. This should be resolved on checklist (PR Author Checklist) — human-only checkboxes; those need to be filled out by the co-author, not me. |
|
@MelvinBot reasoning why you haven't applied your suggested approach? |
|
Good question, Reasoning for the change:
On the proposal's secondary suggestion (key the invoice check on Happy to switch to the row-level approach if you feel the fix belongs there — let me know. |
|
@MelvinBot I think better to pass it with the prop as that is what follows for group item. We can think of refactoring by taking out chatReport prop entirely and use directly in PayActionCell in any of the future PR. cc: @luacmartins as it is added recenetly in #93308 |
…l as a prop Mirror the grouped-report path: resolve the invoice/chat room in TransactionListItemWide and thread it through TransactionItemRow to the Pay action, instead of falling back inside PayActionCell. Revert the PayActionCell self-resolution accordingly and repoint the regression test to the prop contract. Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
Done — switched to the prop approach in What changed:
Agreed on taking out the Checks run locally: oxfmt, |
|
✅ The Spell check failure is fixed and now passing (pushed The only remaining red check is |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot you forgot to add the prop in TransactionItemRow component |
|
@MelvinBot mark this ready for review and complete the author checklist by ticking all checkboxes. I will take care of recordings and other stuff in my reviewer checklist |
TransactionListItemWide already resolves the invoice/chat room and passes it as chatReport to TransactionItemRow, and TransactionItemRowWide already forwards it to DeferredActionCell/PayActionCell. But TransactionItemRow dropped the prop (missing from both the destructure and wideForwardedProps), so PayActionCell received chatReport=undefined on the flat type:invoice row and its confirmPayment guard bailed out. Forwarding the prop completes the chain so payInvoice fires. Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
You're right — good catch, and it was a real bug, not just a cosmetic omission. Fixed in 241f325. After the switch to the prop-threading approach, the chain was wired everywhere except the middle layer:
Net effect: Verification
One coverage caveat: |
|
Status update on the checks:
Nothing further for me to fix here — the code-level failure is resolved. Once the PR Author Checklist is completed the remaining red check will clear. |
|
Done — marked this ready for review. ✅ On the author checklist: I ticked the boxes I can genuinely stand behind from the diff itself, and left the rest for you, since several are personal attestations of work I didn't actually perform. Ticked:
I intentionally left these unchecked for you to confirm, because they attest to manual/runtime work I can't truthfully self-certify:
Also heads-up: the Tests and QA Steps sections still have the |
|
The only actionable failure — Spell check — was already fixed in The one check still red is No other checks are failing ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 241f32587e
ℹ️ 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".
|
|
||
| // Resolve the transaction's chat report (e.g. the invoice room) and pass it to the Pay action, mirroring the grouped | ||
| // report rows in ReportListItemHeader. Without it, Pay from a flat transaction row (e.g. `type:invoice columns:...`) is a no-op. | ||
| const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(transactionItem.report?.chatReportID ?? transactionItem.report?.parentReportID)}`); |
There was a problem hiding this comment.
Read the chat report from live Onyx
In Search rows, @hooks/useOnyx is snapshot-aware, so this subscription reads from the current search snapshot instead of the live report collection. For flat type:invoice columns:... results where the snapshot contains the invoice/IOU report but not the parent invoice room, chatReport stays undefined and PayActionCell still returns at its !chatReport guard. This component's parent already uses originalUseOnyx for live parent report data; the pay path needs the same live fallback here.
Useful? React with 👍 / 👎.
|
No code fix needed this round. The only remaining failing checks are:
Both are human-only checkbox gates — they pass once the author and reviewer tick every box in the checklist. I can't complete those on anyone's behalf. The previously-failing Spell check was already fixed in |
|
@MelvinBot chat report already derived in |
…f a new Onyx connection TransactionListItemWide added its own useOnyx to resolve the transaction's chat report, but TransactionListItem already derives it as parentChatReport. Pass that one down through sharedProps and drop the redundant subscription in the row. Extend parentChatReport with the parentReportID fallback so it matches the row's prior resolution and the grouped-report path in ReportListItemHeader. Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
Done — pushed in b731116.
One small thing I folded in: Verified locally: lint, |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
| ]); | ||
| const currentUserDetails = useCurrentUserPersonalDetails(); | ||
| const [parentChatReport] = originalUseOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(snapshotReport?.chatReportID)}`); | ||
| const [parentChatReport] = originalUseOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(snapshotReport?.chatReportID ?? snapshotReport?.parentReportID)}`); |
There was a problem hiding this comment.
Not strictly for the invoice bug — it's a parity/robustness change. Invoice reports carry a chatReportID (the invoice room), so for the flow this PR fixes the ?? parentReportID branch effectively never fires. I added it so the flat-row resolution matches the two places that already resolve the chat report this way:
- the grouped-report path in
ReportListItemHeader.tsx:276—snapshotReport?.chatReportID ?? reportItem.parentReportID - the adjacent
chatReportActionsline right below it in this same file, which already keys onchatReportID ?? parentReportID
So it keeps parentChatReport consistent with its neighbor and the grouped path, and covers report types where chatReportID is empty but parentReportID is set. If you'd rather keep the diff minimal, I'm happy to revert line 141 to key on chatReportID only — the invoice fix works either way. Let me know which you prefer.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-13.at.15.54.57.mov |
| type TransactionItemRowProps = { | ||
| transactionItem: TransactionWithOptionalSearchFields; | ||
| report?: Report; | ||
| /** Chat report the transaction's report belongs to. Passed to the Pay action so flat transaction rows resolve the invoice room the same way grouped report rows do. */ |
There was a problem hiding this comment.
@MelvinBot I don't think we need comment here or here too
There was a problem hiding this comment.
Agreed — the sibling props (report, policy, etc.) don't carry doc comments, so chatReport shouldn't be the odd one out. Removed both:
src/components/TransactionItemRow/types.tssrc/components/Search/SearchList/ListItem/TransactionListItem/types.ts
Pushed in 6b9d7d5.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
Pujan92
left a comment
There was a problem hiding this comment.
LGTM!
Screen.Recording.2026-07-13.at.15.54.57.mov
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product.
We moved this up the tree because of performance reasons. I think we should continue to pass it down as a prop. |
|
|
|
@MelvinBot merge main |
…/App into claude-fixInvoicePayFromSearchTransactionRow Co-authored-by: Pujan Shah <Pujan92@users.noreply.github.com>
|
🤖 Merged latest
|
| policyTagLists, | ||
| nonPersonalAndWorkspaceCards, | ||
| isAttendeesEnabledForMovingPolicy, | ||
| chatReport: parentChatReport, |
There was a problem hiding this comment.
Can we preserve the Search-snapshot fallback here?
Like we do already for Search:
There was a problem hiding this comment.
Yes, @MelvinBot apply the above suggestion. I think also need a fallback here too.
There was a problem hiding this comment.
Done — pushed in 77080c7.
I resolved the chat report to a single chatReport that prefers live Onyx and falls back to the Search snapshot, mirroring the grouped-report path in ReportListItemHeader.tsx:277-278:
TransactionListItem/index.tsx:142-149—snapshotChatReportreads from the search snapshot andconst chatReport = parentChatReport ?? snapshotChatReport;.- The snapshot fallback now flows through all consumers, including the second spot you flagged —
chatReportPolicyID, thehandleActionButtonPressPay path, and thechatReportshared prop.
chatReportActions now keys on chatReport?.reportID ?? chatReportID too, matching the header path.
Ran fmt, lint-changed, typecheck-tsgo, and the React Compiler compliance check — all pass.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
Explanation of Change
When searching
type:invoicewith a customcolumns:...list, invoices render as flat transaction rows (TransactionListItem→TransactionItemRowWide), not grouped report rows. That path mounts the Action column'sDeferredActionCell→PayActionCellwithout achatReportprop (the grouped-report path viaReportListItemHeaderdoes pass it).As a result, when the user picks Pay → Pay as an individual → Mark as paid,
PayActionCell.confirmPaymenthits its guardif (!type || !reportID || !hash || !amount || !chatReport)and returns immediately becausechatReportisundefined.payInvoice()is never called — the reported "nothing happens" symptom. (payInvoice/payMoneyRequestalso need the invoice room forinvoiceReceiver/policy context, so the missing chat report matters beyond just the guard.)This change resolves
chatReportinsidePayActionCellitself — the single convergence point every per-row Pay flows through. When the prop is absent it falls back to the report referenced byiouReport.chatReportID, reusing theallReportsOnyx collection the component already subscribes to (no new Onyx connection). ThechatReportprop still takes precedence, so grouped report rows are byte-identical, and every downstream consumer (invoiceReceiverPolicyID,canIOUBePaid,useReportPaymentContext, the guard, and both pay calls) automatically gets a real chat report. This fixes flat-row Pay for both invoices and expense reports without threading a new prop through the sharedTransactionItemRow, which is consumed by many non-Search surfaces.Added
tests/ui/components/PayActionCellTest.tsxcovering the regression: an invoice row with nochatReportprop resolves it from Onyx and callspayInvoice, and still bails out when no chat report can be resolved from either source.Fixed Issues
$ #95422
PROPOSAL: #95422 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
Suggested manual test:
type:invoice columns:receipt,date,status,merchant,from,category,tag,amount,action.Offline tests
Same as tests.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssection/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))src/libs/SearchParser/searchParser.js,src/libs/Pusher/pusher.js, etc.), I verified that the changes are made to the source file and the generated file is regenerateden.ts,es.ts), I verified that any changes made to translations are also reflected in the other translation filesTextInputfor text inputs,AmountFormfor amount inputs, etc./** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videosundefined