Open parent report in super wide RHP from Home and fix background on refresh#93614
Open parent report in super wide RHP from Home and fix background on refresh#93614MelvinBot wants to merge 3 commits into
Conversation
…refresh Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
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.
|
|
The failing checks are not related to this PR's changes — no code fix is needed.
This PR only touches
For the record, CI |
|
Re-checked — the state is unchanged and there's still no code fix to push. The only real failure is The remedy is simply to re-run the failed |
|
@MelvinBot |
|
🤖 I checked off the PR Author Checklist items I can verify objectively from the code and PR state:
I intentionally left the following unchecked because they require human verification I can't honestly attest to:
|
|
The failing Root cause: Gradle couldn't download a dependency from jitpack.io:
Why it's not from this PR: This PR only changes two JS/TS files — Recommended action: Re-run the failed The other failing check ( |
…hpFromHome Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # src/libs/Navigation/linkingConfig/RELATIONS/HOME_TO_RHP.ts
|
@MelvinBot |
|
@ZhenjaHorbach 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, 1. Conflicts resolved. The only conflict was in 2. Re-ran AI tests after the merge — all passing:
3. Description completed and PR marked ready for review. I filled in the
|
|
The failing checks are not related to this PR's changes — no code fix is needed from me. This PR only touches two navigation/UI files:
1. The only failing test is:
This test exercises transaction category auto-selection logic in Since the code under test and the test itself match 2. This is the The remaining Recommendation: This PR's own changes are passing CI. The |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a9a5a023
ℹ️ 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".
| lastRoute?.name === SCREENS.RIGHT_MODAL.SEARCH_MONEY_REQUEST_REPORT || | ||
| (lastRoute?.name === SCREENS.RIGHT_MODAL.EXPENSE_REPORT && currentFullScreenRoute?.name === SCREENS.HOME) | ||
| ) { | ||
| Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: parentReportID, reportActionID: parentReportActionID})); |
There was a problem hiding this comment.
Preserve Home as the backTo when opening the parent RHP
When this new Home-tab EXPENSE_REPORT path opens the parent chat as search/view/... without a backTo, a hard refresh or shared URL for that RHP is rebuilt under the Search tab: getMatchingFullScreenRoute checks RHP_TO_SEARCH before RHP_TO_HOME, so the existing Home relation for SEARCH_REPORT is shadowed. This affects the wide Home Review/Submit flow after tapping the subtitle from an expense report; the parent chat initially appears over Home, but refreshing changes the background/close target to Search. Pass a Home/current backTo when navigating here so the background remains Home.
Useful? React with 👍 / 👎.
|
Following up on my previous analysis — I confirmed the New evidence: I checked other recent open PRs (#94071, #94070, #94067) — they all pass I tried to re-run the failed job myself, but my token doesn't have permission ( Next step (for you, The other failure, |
|
🤖
Still on you (I can't do these):
|
|
Re-checked — the only non-checklist failure is still the same Concrete root cause of the flake: The failing test const categoryName = randWord();
categories[categoryName] = { ... };When two of the 3 random words happen to collide, the object key dedupes to 2 categories instead of 3. The test then disables one, leaving a single enabled category — so This logic is in Recommended action: Re-run the failed The |
…hpFromHome Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
@NicolasBonet 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] |
|
🤖 Fixed, Root cause of the What I did:
The push re-triggers CI, so |
|
@JmillsExpensify @mountiny - this on your radar? |
|
Hmmm idk, i feel like this might be complicating things. Should we just navigate the user to that report in Inbox instead? |
|
If anywhere, Spend I think. What was the intention though? |
|
What do you mean by what was the intention? |
|
I think Spend as well |
I mean, how was it supposed to work. If you click the button on Home were you supposed to go straight to Spend? or were we supposed to open the transaction on Home? Then as a follow-on, is it intended to open the report on Home if you click the report in the header, or navigate to Spend? Similarly, if it's a one-expense report and the header is the workspace chat. Open the chat in Home or go to Inbox? |
|
Yeah but idk if opening the super wide workspace chat from narrow RHP on home makes much sense. I think that the narrow RHP is good for a quick look and action from the home page but if they want to go to the parent they can do it on the Spend page |
Explanation of Change
When opening a money request report's parent (the workspace chat) from the report header subtitle link in the Home tab, two problems occurred:
This change updates
ParentNavigationSubtitleand theHOME_TO_RHPlinking relation so the parent report opens correctly from the Home tab:EXPENSE_REPORT_RHP(the super-wide RHP route) instead of the narrowSEARCH_REPORT.isSmallScreenWidth(real device width) is used rather thanshouldUseNarrowLayout, because the latter is alwaystrueinside an RHP and would block the super-wide path.openParentReportInCurrentTabRHP-stacking logic now also handles theEXPENSE_REPORTroute (scoped to the Home tab for the first branch) so the Home "Review"/"Submit" flows mirror the existing Search flow and avoid stacking duplicate RHPs.HOME_TO_RHPdeclares theEXPENSE_REPORTrelation, so a hard refresh on a route with nobackTorebuilds Home as the background instead of falling back to the Inbox default. Call sites that passbackToare unaffected sincebackTotakes precedence.AI tests run by MelvinBot (not a substitute for human testing)
Re-run after merging
main(conflict inHOME_TO_RHP.tsresolved —mainindependently added theEXPENSE_REPORTrelation via the "Recently added" home section, which is a superset of this PR's intent):npx prettier --checkon changed files — all files use Prettier code stylenpm run lint-changed— passednpm run react-compiler-compliance-check check src/components/ParentNavigationSubtitle.tsx— COMPILEDnpm run typecheck-tsgo— passed with no errors (the duplicate-identifier errors insrc/libs/API/types.tsnoted earlier are no longer present on currentmain)npm test tests/ui/ParentNavigationSubtitleTest.tsx— 6/6 passedFixed Issues
$ #93499
PROPOSAL:
Tests
Offline tests
This change only affects client-side navigation (RHP route selection and the background rebuilt on refresh), so behavior is identical offline. Follow the same steps as above while offline.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionmainbranch 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