[hold] Add Trip row in MoneyRequestView linking to the trip room#92029
[hold] Add Trip row in MoneyRequestView linking to the trip room#92029ishpaul777 wants to merge 11 commits into
Conversation
|
@ChavdaSachin 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] |
![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: 7ca73ea213
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| style={[styles.moneyRequestMenuItem]} | ||
| titleStyle={[styles.flex1, styles.textBlue]} | ||
| onPress={() => { | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(tripRoomReportID)); |
There was a problem hiding this comment.
When this row is tapped from an expense opened in Search (this component already supports that path via currentSearchResults), navigating directly to r/<tripReportID> drops the search/view/... route and backTo state, so closing/back from the trip room no longer returns the user to their filtered search/expense detail. Other report links from search build ROUTES.SEARCH_REPORT with the active route; use the search-aware report route helper or include the current route here.
Useful? React with 👍 / 👎.
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Let me know when there's visuals here.
|
@trjExpensify please check this Screen.Recording.2026-05-29.at.8.53.09.PM.mov |
ishpaul777
changed the title
|
@ishpaul777, am I supposed to review this? |
|
@ChavdaSachin please hold on reviewing i might have few changed to do, i'll ping you when ready |
|
Alright, but looking at the test steps, the script you linked in the first step ain't accessible to me. |
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.
|
…to DebugUtils validators
|
@ChavdaSachin I dont this it will be easy for you to test since this invovle spotnana booking so removed request, sorry for the noise 🙇 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (grandparent?.tripData?.tripID === transactionTripID) { | ||
| return grandparent; | ||
| } | ||
| return Object.values(reports).find((candidateReport) => candidateReport?.tripData?.tripID === transactionTripID); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
The tripRoomReportSelector subscribes to ONYXKEYS.COLLECTION.REPORT and returns a full Report object. This forces Onyx to run deepEqual on the entire Report object (which has many fields) on every report collection change. Additionally, the Object.values(reports).find(...) fallback performs an O(n) scan over all reports on every collection update.
Since the component only uses reportID and the report name from the result, narrow the selector output to return only the needed primitives. One approach is to compute the display name inside the selector and return a small object:
const tripRoomReportSelector = (reports: OnyxCollection<OnyxTypes.Report>) => {
if (!transactionTripID || !reports) {
return undefined;
}
const grandparent = grandparentReportID ? reports[`${ONYXKEYS.COLLECTION.REPORT}${grandparentReportID}`] : undefined;
const match = grandparent?.tripData?.tripID === transactionTripID
? grandparent
: Object.values(reports).find((r) => r?.tripData?.tripID === transactionTripID);
if (!match) {
return undefined;
}
return {
reportID: match.reportID,
reportName: match.reportName,
};
};This makes the deepEqual comparison trivial (two string fields) instead of comparing the entire Report object on every update.
Reviewed at: da554d2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good call - applied. Selector now returns just {reportID, name} so Onyx is comparing two strings per report mutation instead of the full Report object. getReportName computation moved inside the selector with reportAttributes added to the deps.
…epEqual stays cheap
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da554d256b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5 participants
Explanation of Change
Adds a "Trip" row to
MoneyRequestView(the expense detail view in NewDot). The row appears for any Spotnana travel expense - charge or refund - and tapping it navigates to the corresponding trip room.The trip room is resolved at render time by scanning the loaded
report_collection for a report whosetripData.tripIDmatches the transaction'scomment.tripID. No backend changes are needed for NewDot - the trip room is already part of the Onyx data the client has loaded. The match runs through a memoizeduseOnyxselector so it's only re-evaluated when reports actually change.The companion changes:
GetTripRoomInfocommand (used by OldDot, not by this PR)Adds
tripIDto theCommenttype inTransaction.tsso TypeScript reflects what the backend writes (the field already exists at runtime via Spotnana booking).Fixed Issues
$ https://github.com/Expensify/Expensify/issues/638596
Tests
../script/clitools.sh expensifytravel:trip)Offline tests
The Trip row uses already-loaded reports data; nothing in the rendering path triggers a network call. Tapping the link uses standard in-app navigation.
QA Steps
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
Screen.Recording.2026-05-30.at.2.37.36.AM.mov