fix: keep createWorkspace getting-started task responsive after opening a connection task - #97440
Conversation
|
@mkhutornyi 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] |
617b56e to
16d592e
Compare
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
@yuvrajangadsingh please complete author checklist |
Review: LGTM ✅The fix is correct, minimal, and well-targeted. I verified the root cause and the reasoning holds up. Root cause confirmed.
Import is still needed — Test looks solid. It forces Minor notes (non-blocking)
|
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ 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". |
|
@mkhutornyi done, author checklist is complete. thanks for testing it across platforms. |
|
🚧 arosiclair 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! 🧪🧪
|
|
✋ 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/arosiclair in version: 9.4.49-0 🚀
|
|
🤖 I reviewed the changes in this PR against the help site content under No help site changes are required. This PR is a client-side navigation bug fix ( Because this only fixes an internal routing behavior with no change to any documented feature, workflow, setting, label, or UI copy, none of the existing help articles (e.g. |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.4.49-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On narrow layout the "Create a workspace" task in the Home "Getting started" section could go dead after you opened the "Connect to accounting" task and came back. Tapping it did nothing.
The route for the createWorkspace item is built at render time and baked
Navigation.getActiveRoute()into itsbackTo:Opening the "Connect to QBO" task hits
PolicyAccountingPage, which writespolicy.connections/ connection sync progress / imported categories over the next few seconds. Those writes re-render the Getting started section while aworkspaces/{activePolicyID}route is the active route, sogetActiveRoute()now returnsworkspaces/{activePolicyID}and gets stored as the backTo. The createWorkspace route becomes self-referential:workspaces/{id}?backTo=workspaces/{id}. When it's tapped,arePathAndBackToEqualinlinkTosees the focused path equal to its own backTo and returns early, so no navigation happens. It's narrow-only because the wide branch usesWORKSPACE_OVERVIEWwith no backTo, so there is nothing to self-reference.The Getting started section only ever renders on Home, so the workspace page's back target is always Home. I pass
ROUTES.HOMEas the backTo instead of the live route.ROUTES.HOMEis'home', which is exactly whatgetActiveRoute()already returns when the row is tapped legitimately from Home, so the happy-path route (workspaces/{id}?backTo=home) is unchanged. It just can no longer drift to a self-reference during the connection flow's re-renders.Navigationis still used elsewhere in the file (the fallbackonPress), so the import stays.I also added a regression test that pins the narrow-layout createWorkspace
backToto Home even whengetActiveRoute()has drifted to the workspace page.Fixed Issues
$ #96172
PROPOSAL: #96172 (comment)
Tests
Unit test:
tests/unit/hooks/useGettingStartedItems.test.ts. The added case forcesNavigation.getActiveRoute()to return the driftedworkspaces/{id}route and asserts the narrow createWorkspace route staysworkspaces/{id}?backTo=home. It fails onmain(route comes outworkspaces/{id}?backTo=workspaces%2F1) and passes with this change. Run:TZ=UTC npx jest tests/unit/hooks/useGettingStartedItems.test.tsOffline tests
Same as Tests. The change only affects which
backTovalue is stored on a client-side route, so behavior is identical offline.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