Redirect the root path to Home instead of the Inbox - #97446
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fa4d7b7e4
ℹ️ 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".
3f6b2d9 to
27f30c5
Compare
|
@carlosmiceli 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] |
|
Tracking issue for this change: #97467 |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
There was a problem hiding this comment.
Pull request overview
Updates the React Navigation linking configuration so the root URL (/) resolves to Home (instead of Inbox) by claiming the root pattern via a parse-only alias, while keeping /home as the canonical serialized path. The PR also updates and adds navigation tests to lock in the new URL parsing/serialization behavior and adjusts UI tests that must still boot under Inbox.
Changes:
- Make
SCREENS.HOMEclaim/(and legacy/signin,/Home) via a linking config alias; stop routing the root pattern to the Reports/Inbox navigator. - Remove
/signinand/Homerewriting fromgetAdaptedStateFromPathand rely on linking config parsing instead. - Update/add tests to verify root/legacy paths resolve to Home, canonicalize to
/home, preserve query params, and keep Inbox/report deep links unchanged.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/libs/Navigation/linkingConfig/config.ts |
Moves ownership of the root path to Home via linking aliases and removes root ownership from the Inbox navigator. |
src/libs/Navigation/helpers/getAdaptedStateFromPath.ts |
Removes legacy path rewriting now handled by linking config aliases. |
tests/navigation/rootPathLandsOnHomeTests.ts |
Adds coverage for /, /home, /Home, /signin, query params, and Inbox/report deep links. |
tests/navigation/PublicSignInPageRootUrlTest.tsx |
Updates expected canonicalization for legacy /Home. |
tests/unit/DeeplinkRouteNormalizationTest.ts |
Updates expectations to assert path pass-through instead of rewriting. |
tests/utils/TestHelper.ts |
Allows tests to boot the app at a specific initial URL. |
tests/ui/UnreadIndicatorsTest.tsx |
Boots under Inbox so LHN-dependent assertions remain valid. |
tests/ui/PaginationTest.tsx |
Boots under Inbox so sidebar interactions remain valid. |
tests/ui/GroupChatNameTests.tsx |
Boots under Inbox so LHN-dependent assertions remain valid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| alias: [ | ||
| {path: ROUTES.ROOT, exact: true}, | ||
| {path: CONST.SIGNIN_ROUTE, exact: true}, | ||
| {path: `/${SCREENS.HOME}`, exact: true}, | ||
| ], |
There was a problem hiding this comment.
Agree
It makes sense
| /** | ||
| * Pass `initialUrl` when the test needs the app to boot on a specific screen. The root URL resolves to Home, | ||
| * so tests relying on the reports sidebar have to boot on the Inbox instead. | ||
| */ | ||
| function setupApp(initialUrl = 'https://new.expensify.com/') { | ||
| beforeAll(() => { | ||
| Linking.setInitialURL('https://new.expensify.com/'); | ||
| Linking.setInitialURL(initialUrl); | ||
| appSetup(); |
There was a problem hiding this comment.
Actually, should we set https://new.expensify.com/${ROUTES.INBOX} as the default?
Since we use it almost everywhere
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Explanation of Change
The root URL
/now lands on Home instead of the Inbox. Home owns the empty pattern through a react-navigationaliasin the linking config, which also claims/signinand the legacy/Home. Aliases are parse-only, so/homestays the canonical URL and query strings on root URLs keep working without any path-rewriting helper.Fixed Issues
$ #97467
PROPOSAL:
Tests
1. The root URL lands on Home
https://dev.new.expensify.com:8082//home, and that reloading on/homereopens Home.2. Legacy root patterns still resolve to Home
/signin./Home(capital H)./?delegatorEmail=<any-email>.3. Inbox and report deep links are unaffected
/inboxand verify that the Inbox opens and the URL stays/inbox./r/<reportID>, then reload the page./r/<reportID>).https://dev.new.expensify.com:8082/while logged out.Offline tests
N/A - the change is limited to URL-to-navigation-state parsing and does not touch API calls or Onyx data.
QA Steps
Same as tests
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