refactor: Extract shared hooks for report preview action buttons#95941
Conversation
OlimpiaZurek
changed the title
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![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: f33f17151b
ℹ️ 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".
|
@linhvovan29546 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}
…-hooks # Conflicts: # src/components/ReportActionItem/MoneyRequestReportPreview/ApproveActionButton.tsx # src/components/ReportActionItem/MoneyRequestReportPreview/PayActionButton.tsx
|
Reviewing... |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApptelegram-cloud-document-5-6249154579338895976.mp4Android: mWeb Chrometelegram-cloud-document-5-6249154579338895977.mp4iOS: HybridAppScreen.Recording.2026-07-15.at.22.07.57.moviOS: mWeb SafariScreen.Recording.2026-07-15.at.22.00.39.movMacOS: Chrome / SafariScreen.Recording.2026-07-15.at.21.56.32.mov |
| const transactions = Object.values(reportTransactionsCollection ?? {}).filter( | ||
| (t): t is Transaction => !!t && (isOffline || t.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE), | ||
| ); | ||
| const transactions = useReportPreviewFilteredTransactions(iouReportID); |
There was a problem hiding this comment.
Question: does this need its own Onyx subscription, or can it read from context?
Looking at the render tree, iouReportID here always resolves to the same value that MoneyRequestReportPreview/index.tsx:60-65 already used to compute its own transactions:
const reportTransactionsCollection = useReportTransactionsCollection(iouReportID);
const transactions = allReportTransactions.filter((t) => isOffline || t.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE);That value is passed down as a prop through MoneyRequestReportPreviewContent → MoneyRequestReportPreviewProvider, which already puts it into dataValue and exposes it via ReportPreviewDataContext (MoneyRequestReportPreviewContext.ts:37, readable today as useReportPreviewData().transactions).
Since SubmitActionButton/PayActionButton get their iouReportID from that same useReportPreviewData() call, it's guaranteed to be the same report/same filter — so this hook re-subscribes to useReportTransactionsCollection and re-runs the identical filter that's already sitting in context for free.
Could SubmitActionButton/PayActionButton just do:
- const transactions = useReportPreviewFilteredTransactions(iouReportID);
+ const {transactions} = useReportPreviewData();instead, and drop this hook? That would remove 2 redundant TRANSACTION collection subscriptions per report preview, in the same spirit as #96053's transactionViolations context slice — just one level earlier in the chain. Let me know if there's a reason these need to stay independent (e.g. something about isOffline timing) that I'm missing.
There was a problem hiding this comment.
You're right. Submit and Pay already have this exact list in context, so I changed it as suggested and dropped the extra hook
There was a problem hiding this comment.
One last concern that I missed in my first review: I think we should keep this PR open and only merge it after the previous PR has been deployed to production to avoid conflicts.
| import {delegateEmailSelector} from '@selectors/Account'; | ||
|
|
||
| function useReportPreviewActionButtonData(iouReportID: string | undefined) { | ||
| const [iouReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${iouReportID}`); |
There was a problem hiding this comment.
I think this can use const {iouReport, policy} = useReportPreviewData(); instead of useOnyx as well.
There was a problem hiding this comment.
I checked, but these two aren't the same as the context values:
- The context
policyis the chat's policy (report?.policyID), but here we need the previewed report's own policy (iouReport?.policyID) - The context
iouReportis a trimmedgetStableReportSelectorversion, not the full report the approve/pay/submit actions need.
These are just single-record subscriptions (not a whole collection like transactions), so I'd keep them as useOnyx here
PR already merged and deployed to production, so we're clear to move forward here |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 cristipaval 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/cristipaval in version: 9.4.41-0 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site articles under This PR is a pure internal refactor: it de-duplicates the report-preview action buttons ( Since there is:
there is nothing to document or update on the help site, so I did not create a draft docs PR. |
5 participants
Explanation of Change
This PR de-duplicates the money-request report-preview action buttons.
SubmitActionButton,ApproveActionButton, andPayActionButtoneach independently repeated the same Onyx subscription block, transaction derivation, andconfirmApprovalhandler. Those are now consolidated into three shared hooks:useReportPreviewActionButtonData,useReportPreviewFilteredTransactions, anduseConfirmApproveReportAction.Fixed Issues
$ #96002
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari