Move invoicing to its own section#82040
Conversation
|
@marcochavezf 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] |
Code Review SummaryI've reviewed the changes in this PR. The change is straightforward and correctly fixes the structural issue where the invoicing section was nested inside the booking section. ✅ What looks good:
📝 Minor observation:While the code works correctly, there is a small style inconsistency with Fragment usage. The codebase generally uses React.Fragment or does not use explicit fragments when returning multiple elements is not necessary at the parent level. However, this is a very minor point and does not affect functionality. The change successfully addresses the merge mistake and restores the intended UI structure. ✅ |
marcochavezf
left a comment
There was a problem hiding this comment.
Code Review: Move invoicing to its own section
Summary
This PR fixes a merge mistake from PR #80142 where WorkspaceTravelInvoicingSection was incorrectly nested inside the "Book or Manage Your Trip" <Section> component, rather than being rendered as a sibling section at the same level.
Analysis
Change scope: 1 file, +3/-1 lines -- very small and focused.
What the change does:
- Wraps the return value of
GetStartedTravelin a React Fragment (<>...</>) - Moves
{isTravelInvoicingEnabled && <WorkspaceTravelInvoicingSection policyID={policyID} />}from inside the<Section>component to after it, making it a sibling
Correctness: The fix is correct. Looking at WorkspaceTravelInvoicingSection, it renders its own <Section> wrapper internally (via renderOptionItem), so it should indeed be a sibling of the booking section, not a child. The parent PolicyTravelPage renders the result of GetStartedTravel inside a <View> container, so returning a Fragment with two children is perfectly fine -- they will both be rendered as children of that <View>.
Edge cases:
- When
isTravelInvoicingEnabledisfalse, only the booking<Section>renders -- same behavior as before, no regression. - The Fragment wrapper is a no-op in terms of the React tree when invoicing is disabled, so there is no unnecessary DOM nesting.
Risk level: LOW -- minimal, well-scoped structural fix that moves one JSX expression from child to sibling position.
Verdict
The change is correct, minimal, and matches the stated intention (confirmed by the screenshots in the PR description). No issues found.
🤖 This comment was generated with the assistance of an AI tool.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @stitesExpensify 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/stitesExpensify in version: 9.3.17-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
Explanation of Change
This fixes a mistake in this PR #80142 (comment). I made a mistake when merging that made invoicing part of the main booking section instead of being on its own
Mistake:

Intention:

Fixed Issues
$ n/a
PROPOSAL: n/a
Tests
Offline tests
QA Steps
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari