Fix cross-Domain Search Router navigation - #100150
carlosmiceli merged 12 commits into
Conversation
|
@marufsharifi 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71bf540fb7
ℹ️ 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".
|
@marufsharifi thanks for the review. I’ve addressed the valid comments and verified the cross-Domain navigation and Back behavior. Could you please review again? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppN/A Android: mWeb ChromeN/A iOS: HybridAppN/A iOS: mWeb SafariN/A MacOS: Chrome / Safariissue.fix.demo.mov |
| if (!navigationRef.isReady()) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
!navigationRef.isReady() early return
Every test stubs isReady.mockReturnValue(true), so the not-ready guard is never taken.
Add a test "navigates directly when navigationRef is not ready": isReady.mockReturnValue(false), call with shouldUseNarrowLayout = false. Assert Navigation.setParams is not called, Navigation.navigate is called with the target route, and getRootState is never read.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@nabi-ebrahimi conflicts now 🙇 |
@carlosmiceli, Resolved, thanks |
|
@trjExpensify all yours 🫡 |
|
@carlosmiceli, I think this is ready to be merged. Thanks! |
|
🚧 carlosmiceli 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/carlosmiceli in version: 9.4.73-0 🚀
|
|
🤖 No help site changes required — I did not create a docs PR. Why: This PR is an internal navigation-state fix. It adds It restores the behavior the help site already documents. Nothing in What I checked
Unrelated pre-existing issue I noticed (not caused by this PR, no PR opened for it): @nabi-ebrahimi, there is no linked help site PR because no help site updates are required for this change. Please confirm you agree with that call — if you think an article does need updating, reply with which one and I'll open the draft PR. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.73-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
When navigating from a page in Domain A to a Search Router destination in Domain B, React Navigation reused the existing Domain split navigator. The destination page changed, but the persistent Domain sidebar retained Domain A's account ID.
This PR updates the persistent Domain sidebar with the destination Domain ID before navigating. The behavior is scoped to cross-Domain navigation on wide layouts. Same-Domain and narrow-layout navigation continue using the existing flow.
Fixed Issues
$ #100112
PROPOSAL: N/A - follow-up for #98753
Tests
Navigate between two Domains
Precondition: The account is an administrator of at least two Domains.
SAML.Expected: The selected page, sidebar, and title all belong to the destination Domain.
Offline tests
Same as the Tests.
QA Steps
Same as the Tests.
// 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
fix.mov