Repository navigation
Conversation
OlimpiaZurek
changed the title
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.
|
![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: 1198d5642c
ℹ️ 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".
| iouReportID={getIOUReportIDFromReportActionPreview(action)} | ||
| policyID={policyID} | ||
| chatReportID={reportID} | ||
| chatReport={report} |
There was a problem hiding this comment.
When the preview is rendered from the report list, report here is the reportStable projection from ReportActionsList, and that selector deliberately omits lastMessageText, lastMessageHtml, lastReadTime, etc. Passing that projection down into the pay button means payMoneyRequest/payInvoice now build optimistic and failure Onyx updates from a stripped report; if the payment API fails, the failure MERGE with this stripped chatReport cannot restore the previous last-message fields after the optimistic “paid” message was written, leaving the chat preview showing a failed payment as the latest message. Please keep a full report subscription for mutation/rollback paths, or include the omitted fields before calling the pay actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed
|
@mananjadhav 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] |
|
Tagged as NoQA but does it need a review? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03bab5b113
ℹ️ 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".
| iouReportID={getIOUReportIDFromReportActionPreview(action)} | ||
| policyID={policyID} | ||
| chatReportID={reportID} | ||
| chatReport={report} |
There was a problem hiding this comment.
When this preview is rendered from ReportActionsList, report is the reportStable projection from getStableReportSelector, which omits the last-message fields. The fresh evidence is the held-expense path: this same prop reaches ProcessMoneyReportHoldMenu, and useHoldMenuSubmit still calls payMoneyRequest with it, while the full-report fallback was only added in PayActionButton. If the user pays a report with held expenses and the API fails, PayMoneyRequest restores value: chatReport, so the MERGE cannot bring back the previous lastMessageText/lastMessageHtml, leaving the chat preview stuck on the optimistic paid message.
Useful? React with 👍 / 👎.
|
No product review needed |
|
All bot comments have been addressed. Ready for review. |
| const cashOrCard = isTransactionMadeWithCard ? TestHelper.translateLocal('iou.card') : TestHelper.translateLocal('iou.cash'); | ||
| const transactionHeaderText = `${date} ${CONST.DOT_SEPARATOR} ${cashOrCard}`; | ||
| const transactionDisplayAmount = convertToDisplayString(transaction.amount, transaction.currency); | ||
| const transactionDisplayAmount = convertToDisplayString(-transaction.amount, transaction.currency); |
There was a problem hiding this comment.
question: Any reason we're making the amount negative?
There was a problem hiding this comment.
The test showed positive before only because it passed the wrong chatReportID, so the component got an undefined chatReport. Now it gets the real one, so the displayed amount is correctly negative.
| const {transactionDisplayAmount, transactionHeaderText} = getTransactionDisplayAmountAndHeaderText(transaction); | ||
|
|
||
| expect(screen.getByText(transactionDisplayAmount)).toBeOnTheScreen(); | ||
| expect(screen.getAllByText(transactionDisplayAmount).length).toBeGreaterThan(0); |
There was a problem hiding this comment.
question: Is this needed?
There was a problem hiding this comment.
Yes, with the correct chatReport the amount renders in more than one place
| <ActionContentRouter | ||
| action={action} | ||
| report={report} | ||
| chatReport={chatReport} |
There was a problem hiding this comment.
question: Should we update the ReportActionItemTest? It still passes chatReport={undefined}
There was a problem hiding this comment.
chatReport is only read when rendering a REPORT_PREVIEW action, and these tests don't render that action, so it's never used here. undefined is valid and a real report wouldn't change anything
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chromemweb-chrome-perf-1.moviOS: HybridAppios-perf-1.moviOS: mWeb Safarimweb-safari-perf-1.movMacOS: Chrome / Safariweb-perf-4.movweb-perf-3.movweb-perf-2.movweb-perf-1.mov |
Reviewer Checklist
|
| @@ -129,6 +129,7 @@ function MoneyRequestReportPreview({ | |||
| const renderItem: ListRenderItem<Transaction> = ({item}) => ( | |||
| <TransactionPreview | |||
| chatReportID={chatReportID} | |||
There was a problem hiding this comment.
Can we get rid of this param now since its part of the chatReport?
|
|
||
| const animationCallback = () => onConfirm?.(full); | ||
|
|
||
| // `moneyRequestReport`/`chatReport` may be heartbeat-stripped projections (e.g. from the report list). |
There was a problem hiding this comment.
I am not sure what this means heartbeat-stripped, even though I guess. Could you try to rewrite this in a bit more plain english human language please?
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run 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. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.19-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR and no updates to the help site files under This is a pure internal performance optimization (
There are no user-facing changes — no new or renamed features, tabs, settings labels, buttons, or behavior. The help site documents product behavior, which is unchanged here, so there's nothing to update. No draft PR was created since no documentation changes are warranted. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.19-5 🚀
|
Explanation of Change
This PR reduces unnecessary re-renders in chats with several expense report previews. Each preview was listening to the raw chat report directly, so it re-rendered every time a message was sent — even though nothing it displays changed. Now the previews reuse the already-stabilized report the list prepares (via
getStableReportSelector) by receiving it as a prop, and the four redundantuseOnyx(REPORT)subscriptions are removed.Results from profling:
MoneyRequestReportPreviewTransactionPreviewFixed Issues
$ #93771
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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