Repository navigation
Refactor: remove deprecatedReportsTransactions in hasNonReimbursableTransactions(Part 1) - #94806
Conversation
…ransactions part 1
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dupe expense-breakdown check, gate reportsTransactions grouping MoneyRequestReportPreviewProvider was computing hasNonReimbursableTransactions from the pending-delete-filtered transactions list instead of allReportTransactions, which could flip the preview wording during an optimistic delete. Also merges shouldShowExpenseBreakdown into hasNonReimbursableTransactions (duplicate logic), gates the reportAttributes reportsTransactions regroup behind hasKeyTriggeredCompute(TRANSACTION) so it isn't rebuilt on unrelated recomputes, and dedupes the groupTransactionsByReportID test helper into ReportTestUtils.
…ction-grouping helpers, fix per-row Search transaction subscription reportAttributes.ts's previousReportsTransactions cache didn't reset on logout/full recompute like its sibling caches, letting stale cross-session transaction data leak into report names; it now also reuses the existing buildTransactionsByReportID helper instead of a hand-rolled reduce. SearchActionHeader subscribed to the entire TRANSACTION collection per row; switched to useReportTransactionsCollection for an O(1) lookup against the already-grouped derived value. Also dedupes the test-only groupTransactionsByReportID helper, simplifies a couple of redundant checks, and removes a duplicate hasNonReimbursableTransactions call per render.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
…sactions-in-hasNonReimbursableTransactions-p1
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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 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] |
…actions-in-hasNonReimbursableTransactions-p1
a6f7454 to
98178b7
Compare
| @@ -62,7 +59,9 @@ type MoneyRequestReportPreviewProviderProps = ChildrenProps & { | |||
| iouReport: OnyxEntry<Report>; | |||
| chatReport: OnyxEntry<Report>; | |||
| transactions: Transaction[]; | |||
There was a problem hiding this comment.
Could we remove the transaction parameters too? It looks like transactions and allReportTransactions are identical. I think we need to evaluate again whether the current approach is feasible. I'd suggested moving some params to the component level to utilize the selector and limit re-renders, but I noticed you didn't use a selector. Is that because transaction is used in more places?
There was a problem hiding this comment.
That is difference and transactions is also used directly in index.tsx (sizing, payer/receiver check, violations, highlighting), not just inside Provider. So moving it to a selector in component level would mean two components subscribing to the same data instead of one, without actually saving any re-renders
There was a problem hiding this comment.
@linhvovan29546 My initial thought when suggesting we remove reportsTransaction (and transactions too) was to use a selector to optimize performance. But if we still transactions in other places too and we can't remove it entirely, then we should compute the related data in the child component to keep the props clean.
In this case, if we still need to pass transactions, it would be better to pass only allReportTransactions and then compute the related data inside the child component. We might even be able to remove transactions altogether, since it looks almost duplicated with allReportTransactions or if the difference is needed, we can compute it from allReportTransactions in the child component.
This update looks a bit out of scope for the original issue, but I think it'll make things cleaner, so I hope it doesn't bother you
There was a problem hiding this comment.
we can compute it from allReportTransactions in the child component.
If we compute transactions from allReportTransactions, we'll end up computing it twice: once in the parent (index) and once in the child. We still need transactions in the index file, so this would duplicate the computation. Are we still okay with this approach?
There was a problem hiding this comment.
@linhvovan29546 Thanks a lot for the active discussion. Let's leave it as is for now, so we don't expand the scope too much
…actions-in-hasNonReimbursableTransactions-p1
computeReportNameOriginal requires reportTransactions since the linkedTransactions refactor; these two direct calls were missed, failing CI typecheck.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-08-07.at.17.25.40.mov |
|
@codex review |
![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: ceca3ba930
ℹ️ 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".
…actions-in-hasNonReimbursableTransactions-p1
…in Search
For a chat thread whose parent is an invoice report, getChatListItemReportName
swaps to the parent invoice for naming via getReportForHeader, but
SearchActionHeaderContent was still gating its transaction subscription on
isInvoiceReport(report) - the thread itself, which is never an invoice type.
This left linkedTransactions empty, so non-reimbursable invoices always fell
back to reimbursable ("paid") wording in thread headers on the Search page.
Resolve the parent report reactively via useOnyx instead of reusing
getReportForHeader's deprecated global report cache, which isn't guaranteed
to be populated or to trigger a re-render when the parent report loads.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@linhvovan29546 failed eslint |
ESLint's no-nested-ternary flagged the two-level ternary for reportForHeaderReportID; replace with an if/else chain, same logic.
|
Done! |
Co-authored-by: DylanDylann <141406735+DylanDylann@users.noreply.github.com>
|
@linhvovan29546 Thank you. It's a great job |
…actions-in-hasNonReimbursableTransactions-p1 # Conflicts: # src/libs/ReportNameUtils.ts # src/libs/ReportUtils.ts # src/pages/inbox/report/SearchActionHeader.tsx # tests/unit/ReportNameUtilsTest.ts # tests/unit/ReportUtilsTest.ts
|
🚧 Valforte 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/Valforte in version: 9.4.52-0 🚀
|
|
🤖 No help site changes required. I reviewed this PR against the help site articles in Why no docs change is needed
No draft PR was created since no changes are required. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.52-11 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
Continues the
Onyx.connect()deprecation effort (parent issue) by removing the implicitdeprecatedReportsTransactionsglobal lookup thathasNonReimbursableTransactionsused internally.getMoneyRequestReportName,getInvoiceReportName, andcomputeReportNamenow take an explicitlinkedTransactions/reportTransactionsargument instead of relying on theReportUtils.ts-level Onyx-connected cache.Threaded the new required argument through every call site:
src/libs/actions/OnyxDerived/configs/reportAttributes.ts— the Onyx derived value that computes report names for the whole appsrc/libs/ReportUtils.ts'sgetChatListItemReportName— now takeslinkedTransactionsas a parameter, sourced via theuseReportTransactionshook in its only caller,SearchActionHeader.tsxsrc/components/.../MoneyRequestReportPreviewContent.tsxAlso fixed an unrelated broken import (
./MoneyRequestReportUtils→@libs/MoneyRequestReportUtils) inMoneyRequestReportPreviewContent.tsxthat was breaking typecheck, Storybook, and knip CI checks.This is a pure refactor — no behavior change is intended. Report names should render identically before and after.
Fixed Issues
$ #66418
PROPOSAL: N/A
Tests
Test 1
Test 2
Test 3 (Reimbursable column / expense breakdown)
Test 4 (Reimbursable column updates live when transactions change)
Open the report from Test 3 (with a non-reimbursable expense).
Add a new expense, then delete an existing one.
Verify the "Reimbursable" column / expense breakdown section updates immediately to reflect the current set of transactions, without needing to reload or re-navigate.
Verify that no errors appear in the JS console
Offline tests
Same as Tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
telegram-cloud-document-5-6230959843742589544.mp4
Android: mWeb Chrome
telegram-cloud-document-5-6230959843742589545.mp4
iOS: Native
Screen.Recording.2026-07-09.at.17.13.10.mov
iOS: mWeb Safari
Screen.Recording.2026-07-09.at.16.51.52.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-09.at.16.41.46.mov