Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts (part 9) - #94628
Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts (part 9)#94628truph01 wants to merge 8 commits into
Conversation
|
@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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| amount: item.amount, | ||
| paymentType: resolvedPaymentType, | ||
| ...(isInvoiceReport(item.reportID) | ||
| ...(isInvoiceReport(allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${item.reportID}`]) |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The inline lookup allReports?.[${ONYXKEYS.COLLECTION.REPORT}${item.reportID}] is duplicated here and again on line 1096 (isItemInvoice). It also re-derives a report that is already resolved into the local iouReport variable a few lines above (const iouReport = getReportFromSearchSnapshot(item.reportID, searchData, allReports)). Worse, this direct allReports access bypasses the snapshot-aware resolution that getReportFromSearchSnapshot performs (it prefers searchData over allReports), so the two code paths can resolve to different reports for the same item.
Reuse the already-resolved report instead of duplicating the lookup:
const iouReport = getReportFromSearchSnapshot(item.reportID, searchData, allReports);
if (!iouReport) {
continue;
}
// ...
...(isInvoiceReport(iouReport)
? getPayMoneyOnSearchInvoiceParams(...)
: {}),
// ...
const isItemInvoice = isInvoiceReport(iouReport);Reviewed at: 8318f93 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8318f93f8d
ℹ️ 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".
| const nextStep = allNextSteps?.[`${ONYXKEYS.COLLECTION.NEXT_STEP}${item.reportID}`]; | ||
| const additionalOnyxData = getSearchPayOnyxData(hash, item.reportID, currentSearchKey); | ||
| const isItemInvoice = isInvoiceReport(item.reportID); | ||
| const isItemInvoice = isInvoiceReport(allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${item.reportID}`]); |
There was a problem hiding this comment.
When bulk-paying an invoice that is present in the current search snapshot but not in allReports, this check returns false even though iouReport was already resolved via getReportFromSearchSnapshot above. That sends the invoice through the normal payMoneyRequest path and also skips the invoice-specific payment params built above, so search bulk pay can fail or pay with the wrong flow for invoice results that only exist in the snapshot. Use the resolved iouReport for invoice detection here and when constructing paymentItem.
Useful? React with 👍 / 👎.
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@truph01 This is a tough refactor since the report collection is big one. Could you consider putting this PR on hold and doing one PR at a time instead? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fa3d2e7b6
ℹ️ 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".
|
|
||
| // Invoice expense is not auto-categorized | ||
| if (isInvoiceReport(transaction.reportID)) { | ||
| if (isInvoiceReport(report)) { |
There was a problem hiding this comment.
When this helper is invoked with only a transaction, the signature still allows it and the existing TransactionUtilsTest covers that case by writing an invoice report to Onyx first; this new check receives undefined, so invoice transactions with a missing category and pending ADD fall through and are treated as “category being analyzed” even though invoice expenses are explicitly excluded. Make the report argument required/update all callers and tests to pass it, or otherwise preserve the report lookup for this case.
Useful? React with 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd14aab1ad
ℹ️ 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".
|
@DylanDylann This PR is ready for review again |
3 participants
Explanation of Change
Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts.
Target call chains:
Fixed Issues
$ #66416
PROPOSAL:
Tests
Analyzing...label in category row"Offline tests
QA Steps
Same as tests
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
MacOS: Chrome / Safari
Screen.Recording.2026-08-06.at.16.27.39.mov