Fix opening an expense report from Reports page shows the same report when open Inbox#67437
Conversation
|
@ishpaul777 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] |
src/pages/home/ReportScreen.tsx
Outdated
| if (!isReportInRHP) { | ||
| const {moneyRequestReportActionID, transactionID, iouReportID} = route.params; | ||
|
|
||
| // When we get here with a moneyRequestReportActionID and a transactionID from the route it means we don't have the transaction thread created yet | ||
| // so we have to call OpenReport in a way that the transaction thread will be created and attached to the parentReportAction | ||
| if (transactionID && currentUserEmail && !report) { | ||
| const iouReport = getReportOrDraftReport(iouReportID); | ||
| const iouAction = getReportAction(iouReportID, moneyRequestReportActionID); | ||
| const optimisticTransactionThread = buildTransactionThread(iouAction, iouReport, undefined, reportIDFromRoute); | ||
| openReport(reportIDFromRoute, undefined, [currentUserEmail], optimisticTransactionThread, moneyRequestReportActionID, false, [], undefined, transactionID); | ||
| // When we get here with a moneyRequestReportActionID and a transactionID from the route it means we don't have the transaction thread created yet | ||
| // so we have to call OpenReport in a way that the transaction thread will be created and attached to the parentReportAction | ||
| if (transactionID && currentUserEmail && !report) { | ||
| const iouReport = getReportOrDraftReport(iouReportID); | ||
| const iouAction = getReportAction(iouReportID, moneyRequestReportActionID); | ||
| const optimisticTransactionThread = buildTransactionThread(iouAction, iouReport, undefined, reportIDFromRoute); | ||
| openReport(reportIDFromRoute, undefined, [currentUserEmail], optimisticTransactionThread, moneyRequestReportActionID, false, [], undefined, transactionID); | ||
| } |
There was a problem hiding this comment.
looks like this cause a regression
- Create a expense, dont open it.
- go to reports page and open the expense
- notice it shows not found page
Screen.Recording.2025-08-03.at.4.06.22.AM.mov
There was a problem hiding this comment.
Yeah, I experience this and am really confused with it because sometimes I also experience it on main. Also, the updated code above will only run if it is not in RHP. In main, the code also only runs if it's not in RHP because in RHP, there is no moneyRequestReportActionID, transactionID, iouReportID params.
Can you recheck on main?
There was a problem hiding this comment.
I just merged with main and can't repro it anymore.
There was a problem hiding this comment.
not reproducible anymore! Thank you!
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-08-04.at.12.40.59.AM.movAndroid: mWeb ChromeScreen.Recording.2025-08-04.at.12.42.57.AM.moviOS: HybridAppScreen.Recording.2025-08-04.at.12.19.15.AM.moviOS: mWeb SafariScreen.Recording.2025-08-03.at.4.04.05.AM.movMacOS: Chrome / SafariScreen.Recording.2025-08-03.at.3.25.27.AM.movMacOS: DesktopScreen.Recording.2025-08-04.at.5.23.19.PM.mov |
|
How is this looking? |
| const rootState = navigationRef.getRootState() as State<RootNavigatorParamList>; | ||
| const lastReportNavigator = rootState.routes.findLast((route) => route.name === NAVIGATORS.REPORTS_SPLIT_NAVIGATOR); | ||
| const lastReportNavigatorState = lastReportNavigator && lastReportNavigator.key ? getPreservedNavigatorState(lastReportNavigator?.key) : undefined; | ||
| const lastReportRoute = lastReportNavigatorState?.routes.findLast((route) => route.name === SCREENS.REPORT); | ||
|
|
||
| if (lastReportRoute) { | ||
| const {reportID} = lastReportRoute.params as ReportsSplitNavigatorParamList[typeof SCREENS.REPORT]; | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportID)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
@adamgrzybowski @WojtekBoman is this the cleanest way to write this logic? @bernhardoj Can you please add plain English comments here too? We should explain what this is doing and why. Thank you
There was a problem hiding this comment.
Added comment. FYI, I copied this logic from the search tab
App/src/components/Navigation/NavigationTabBar/index.tsx
Lines 122 to 145 in 5eca224
There was a problem hiding this comment.
As @bernhardoj wrote, it's handled the same way for the Search page. However, this solution will soon need to be adjusted once the PR introducing route preloading is merged. @mountiny I'm considering what we should merge first? Should I adapt preloading to this solution or should this solution be adapted to preloading?
There was a problem hiding this comment.
Seems like preloading is quite close so I will hold this PR
|
on HOLD for #66890 |
|
i see #66890 got merged, @bernhardoj can we resume work here? |
|
@ishpaul777 sorry, I missed this. I just retested it and can't reproduce it because OpenReport doesn't update the report's |
|
^ Still same |
|
on my radar will check this soon! |
|
@bernhardoj i can still reproduce Screen.Recording.2025-09-09.at.1.13.33.AM.mov |
|
Hmm, I can't reproduce it because web.mp4 |
|
tried few times on different accounts i can still reproduce the orignal issue : /, cant reproduce what you are seeing @bernhardoj Screen.Recording.2025-09-12.at.1.48.57.AM.mov |
|
@aimane-chnaif can you please review? |
| const rootState = navigationRef.getRootState() as State<RootNavigatorParamList>; | ||
| const lastReportNavigator = rootState.routes.findLast((route) => route.name === NAVIGATORS.REPORTS_SPLIT_NAVIGATOR); | ||
| const lastReportNavigatorState = lastReportNavigator && lastReportNavigator.key ? getPreservedNavigatorState(lastReportNavigator?.key) : undefined; | ||
| const lastReportRoute = lastReportNavigatorState?.routes.findLast((route) => route.name === SCREENS.REPORT); |
There was a problem hiding this comment.
As code is duplicated, can we define function on top of this file and use in both places?
|
|
||
| const lastReportRoute = getLastRoute(NAVIGATORS.REPORTS_SPLIT_NAVIGATOR, SCREENS.REPORT); | ||
| if (lastReportRoute) { | ||
| const {reportID} = lastReportRoute.params as ReportsSplitNavigatorParamList[typeof SCREENS.REPORT]; |
There was a problem hiding this comment.
Is it possible that reportID might not exist or 0 here?
Just asking to avoid any unexpected regressions of being redirected to weird route, which leads to not found page or infinite loading.
There was a problem hiding this comment.
might not exist
Hmm, I'm not sure, but it shouldn't be a problem because we will always use the last accessed report when the reportID is missing. Report route also accepts undefined reportID.
App/src/pages/home/ReportScreen.tsx
Lines 212 to 222 in e219999
or 0 here?
This one is possible if the user opens a report with ID 0, but this isn't a problem.
web.mp4
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb Chromemchrome.moviOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecb143dbe1
ℹ️ 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".
| shouldShowFloatingCameraButton?: boolean; | ||
| }; | ||
|
|
||
| function getLastRoute(navigator: string, screen: string) { |
There was a problem hiding this comment.
Could you update the types in some future PR @bernhardoj please
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.97-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.99-0 🚀
|
|
@bernhardoj please consider #79249, #79315 in v2 |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.2.99-8 🚀
|
Explanation of Change
Fixed Issues
$ #67122
PROPOSAL: #67122 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: have an expense
Web:
Android/iOS/mWeb:
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))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.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
Uploading ios mweb.mp4…
MacOS: Chrome / Safari
web.mp4