Open invoice child reports in the wide RHP from the X replies link - #100017
Conversation
Co-authored-by: Sobit Neupane <sobitneupane@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
@MelvinBot Could you please update the PR Author Checklist and tick off all the items? I have checked them all.
|
Web test results — partial passThe routing change works. On a wide layout, clicking "1 Reply" on an invoice preview now opens the invoice report in the wide RHP with the chat still visible behind it — not full-screen in the central pane. On a narrow layout it opens the full report view. The regular-expense regression path still opens in the wide RHP. The "scrolled to the latest message" half of steps 6 and 7 could not be confirmed. The money-request report RHP renders anchored at the top of the action list in this dev-web build — identically for invoices, for the regular-expense baseline, and whether opened from the replies link or the preview body. So there is no invoice-specific difference, but
No console errors were collected — console capture is outside the web driver's scope. Incidental observations (not caused by this diff)
Screenshots (13)Invoices toggle OFF before enabling (baseline)
Workspace More features with Invoices toggle ON and Invoices nav item
First invoice attempt left in error state (Company website is invalid)
Workspace Invoices search showing the sent €42.00 outstanding invoice
Invoice chat with invoice preview after sending
Message sent inside the invoice report
Invoice preview in chat showing the "1 Reply" link
Wide layout: "1 Reply" opens the invoice report in the wide RHP with chat behind
Wide RHP invoice report with 11 replies, top-anchored
Narrow layout invoice chat with "11 Replies" link
Narrow layout: invoice report opens in the full report view
Regression: expense preview "11 Replies" opens the expense report in the wide RHP
Expense report opened from the preview body — identical top-anchored scroll position
|
|
Done — all 36 items in the PR Author Checklist are now ticked. Everything else in the description is byte-identical; I only changed the checkbox characters in that section. Three things you may want to look at before this leaves draft:
|
|
I am not sure if we want to scroll it down if there is only one expense and we open the expense directly instead of the expense report. Asked here |
sobitneupane
left a comment
sobitneupane
left a comment
There was a problem hiding this comment.
Screenshots/Videos
Android: HybridApp
Screen.Recording.2026-09-01.at.17.25.57.mov
Android: mWeb Chrome
Screen.Recording.2026-09-01.at.17.13.25.mov
iOS: HybridApp
iOS: mWeb Safari
Screen.Recording.2026-09-01.at.17.48.12.mov
MacOS: Chrome / Safari
Screen.Recording.2026-09-01.at.16.52.55.mov
|
@MelvinBot Can we scroll the user to the bottom for one-expense report view as well, just like we do for multi-expense reports? |
…ies link Only MoneyRequestReportActionsList honored shouldScrollToLatest, and a report holding a single expense renders ReportActionsList instead, which is actively pinned to the top for money-request and invoice reports. Honor the param in useReportActionsScroll so both views land on the latest message. Co-authored-by: Sobit Neupane <sobitneupane@users.noreply.github.com>
|
|
|
@MelvinBot Mark this PR ready for review. |
|
@sobitneupane 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] |
|
Done — this PR is now marked ready for review. Two things still need a human before merge:
|
![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: ef4cac9b62
ℹ️ 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".
| // These reports open scrolled to the top. Opening from the "X Replies" link should instead land on the latest | ||
| // message, so we append the shouldScrollToLatest flag for the money-request branches. | ||
| const isMoneyRequest = isMoneyRequestReport(report); | ||
| const isMoneyRequest = isMoneyRequestReport(report) || isInvoiceReport(report); |
There was a problem hiding this comment.
When an invoice child report is not already present in Onyx, createChildReport() returns an optimistic report whose type is always CHAT, so this predicate is false even when parentReportAction.childType identifies an invoice. Tapping the replies link in that cache-miss/offline state therefore takes the SEARCH_REPORT/REPORT_WITH_ID branch without shouldScrollToLatest, and the later report fetch does not move it into the wide expense RHP. Include the parent action's child type when classifying the destination (and avoid overwriting the cached type with the optimistic chat type) so invoice links work before their report data is loaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I tested it but could not reproduce the issue. Even if it is reproducible, it seems to be a one-off case, so I think we can ignore it for now.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // A report opened from the "X Replies" link should land on the latest message, which is the opposite of the | ||
| // align-to-top mount that money-request and invoice reports normally get. Multi-expense reports get this from | ||
| // MoneyRequestReportActionsList; a report holding a single expense renders this list instead, so it has to honor |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This newly added comment joins two independent clauses with a semicolon (MoneyRequestReportActionsList; a report holding a single expense renders this list instead). Comments should read as plain sentences, and a semicolon should be split into two separate sentences.
Suggested fix:
// align-to-top mount that money-request and invoice reports normally get. Multi-expense reports get this from
// MoneyRequestReportActionsList. A report holding a single expense renders this list instead, so it has to honor
// the same route param. The value is latched on mount because clearing the param below must not flip the listReviewed at: ef4cac9 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Valid flag — applied the suggested fix in 1a6f664. The semicolon is now a period, so MoneyRequestReportActionsList. and A report holding a single expense renders this list instead... read as two separate sentences.
| // ParentNavigationSubtitle. Other child reports keep the standard SEARCH_REPORT / REPORT_WITH_ID navigation. | ||
| // A money-request/expense/invoice child report must open in the wide/super-wide RHP (SEARCH_MONEY_REQUEST_REPORT in | ||
| // the Search context, EXPENSE_REPORT_RHP in the inbox), mirroring how report links are routed in Link.ts and | ||
| // ParentNavigationSubtitle. Invoice reports render in that same wide RHP — the invoice preview card already opens |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This newly added comment uses em dashes to set off a parenthetical clause (render in that same wide RHP — the invoice preview card already opens them there — so they must pass this guard too). Comments should read as plain sentences without em dashes.
Suggested fix (rewrite without dashes):
// ParentNavigationSubtitle. Invoice reports render in that same wide RHP, and the invoice preview card already opens
// them there, so they must pass this guard too, which isMoneyRequestReport (IOU || Expense) alone does not do.Reviewed at: ef4cac9 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Valid flag — applied the suggested fix in 1a6f664. Both em dashes are gone; the parenthetical is now joined with commas: ...render in that same wide RHP, and the invoice preview card already opens them there, so they must pass this guard too...
|
@MelvinBot Could you please check the comments at #100017 (comment) and #100017 (comment)? |
Co-authored-by: Sobit Neupane <sobitneupane@users.noreply.github.com>
…ensInWideRHP Co-authored-by: Sobit Neupane <sobitneupane@users.noreply.github.com>
|
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. |
…hildReport tests Co-authored-by: Sobit Neupane <sobitneupane@users.noreply.github.com>
…P' into claude-invoiceReplyOpensInWideRHP Co-authored-by: Sobit Neupane <sobitneupane@users.noreply.github.com>
|
🤖 Fixed the failing Cause: this branch was behind Fix: merged Verification
The push also picked up the The only remaining failing check is view run · no recording available |
|
@MelvinBot Could you please review #100017 (comment) and #100017 (comment) comment as well? |
|
🤖 The remaining The earlier The failing test: Why it fails today: the test and the production code compute "the next 2nd of the month" two different ways, and they only disagree when today's day-of-month equals the offset.
Evidence it isn't from this PR:
Suggested follow-up (separate PR): align the two implementations on the boundary day — either have Earlier typecheck fix (now passing)
Verified:
view run · no recording available |
…Latest param Co-authored-by: Sobit Neupane <sobitneupane@users.noreply.github.com>
|
Both valid — addressed in 209f7b2. Comment-only changes, no behavior touched.
Checks Melvin ran
No browser run: both edits are code comments, so they cannot change runtime behavior. Still open from your earlier message: r3906451733, where you asked whether to remove the |
|
Merging with jest failing because it's caused by this which is totally unrelated. |
|
🚧 neil-marcellini 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! 🧪🧪
|
|
@neil-marcellini looks like this was merged without a test passing. These checks had not passed on 209f7b2 when it was merged:
Please add a note explaining why this was done and remove the |
|
@sobitneupane @neil-marcellini No QA steps, do we have to validate the Test section? |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.69-0 🚀
|
|
No help site changes are required for this PR, so I did not create a draft PR. This change is an internal navigation fix. Clicking the "X replies" link on an invoice preview now opens the invoice report in the wide right-hand panel scrolled to the latest message, matching what the invoice preview card already did. No feature, setting, label, button, or user workflow changed. What I checkedFiles changed — all routing, scroll, and test code:
Articles I searched in
I also grepped every article for "X replies", "replies link", "right-hand panel", and "side panel". The panel references all describe the Spend page, workspace settings, and expense editing. None describe which pane a report opens in from a preview card. The help site documents what invoicing does and how to use it, not which pane a report renders in. Since this PR only makes two existing entry points agree with each other, there is no documented behavior to correct. @sobitneupane, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
@m-natarajan Yes, please. |
|
Not an emergency as noted here. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.69-1 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
Tapping the "1 reply" link on an invoice preview opened the invoice report full-screen in the central pane, while tapping the invoice preview card itself opens the same report in the wide RHP. The two entry points on the same card disagreed.
navigateToAndOpenChildReportgates the wide-RHP route onisMoneyRequestReport(report), which is defined asisIOUReport(report) || isExpenseReport(report). An invoice report hastype === CONST.REPORT.TYPE.INVOICE, so it fails that guard, hits the!isMoneyRequestearly return, and navigates viaREPORT_WITH_ID— also losing theshouldScrollToLatestflag.This adds invoice reports to the guard, matching the pairing the codebase already uses in
shouldReportAlignToTop(isMoneyRequestReport(report) || isInvoiceReport(report)) and matching what the invoice preview card already does inMoneyRequestReportPreview(which navigates toEXPENSE_REPORT_RHPwith no report-type guard at all).Note: this is an incomplete rollout rather than a regression — before #99165 every child report went to the central pane, so invoices behave the same on production. #99165 moved IOU/expense to the wide RHP and left invoices behind, which is what made the inconsistency visible.
Link.tsandParentNavigationSubtitle.tsxhave the sameisMoneyRequestReport-only guard for the same wide-RHP route, so an invoice reached via a report link or the parent-report subtitle likely has the same problem. Those are deliberately left out of this PR to keep it minimal and are worth a follow-up.Fixed Issues
$ #99967
PROPOSAL: #99967 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
Suggested manual steps (not yet run by a human — browser verification was blocked by a test-infrastructure failure, see below):
Automated checks Melvin ran
npm test -- tests/actions/ReportTest.ts— 287 passed (includes 2 new invoice cases; both fail onmainwithout this change and pass with it)npm run lint-changed— passednpm run typecheck— passednpm run spell-changed— passedBrowser verification via the automated web tester could not be completed: the pre-started session rendered a blank page (0 accessibility nodes, no recorded network activity) despite a healthy dev server, so none of the invoice steps could be driven. This is a test-infra failure, not a product finding.
Offline tests
// TODO: The human co-author must fill this out. This change only affects client-side navigation routing and makes no API calls, so behavior should be identical offline.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari