[No QA] Clarify why the Workspaces tab resets to WorkspaceInitialPage on narrow layouts - #100321
Conversation
…ialPage Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
The The failure: EvidenceThe broken file is byte-identical on this branch and on So the error exists at When
An earlier commit, This PR's The fix already open — #100314 ( Other failing check: Per Melvin's safety rules I don't push fixes for pre-existing failures originating outside a PR's diff, and duplicating #100314 here would conflict with it. |
|
@MelvinBot please do the following:
|
|
🤖 Web test steps verification results: ✅ pass On a narrow viewport, the bottom tab bar shows on Home/Inbox and the workspace initial page, but is hidden (non-interactive) on workspace sub-pages (Members, More features). After switching to Inbox and tapping Workspaces again, the app correctly lands on the workspace's initial settings page with the tab bar visible, not on the previously visited sub-page. Notes for reviewers
|
|
@ahmedGaber93 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,
One thing to fix before a reviewer picks this up: the |
|
Fixed — Both failures came from one pre-existing breakage on
This PR still changes exactly one file, comment-only:
Why this PR wasn't the causeThe broken file was byte-identical on this branch and on When
Commit On the merge: the local checkout was shallow with no common ancestor available, so I created the merge through GitHub's merges API. It's a true merge commit with both parents ( I skipped browser testing: the only behavioral change here is a code comment, and the fix was a merge, so there's nothing a UI run could verify that CI doesn't. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
ahmedGaber93
left a comment
There was a problem hiding this comment.
LGTM!
More details: #99215 (comment)
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #99215 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
The Every check now passes except |
|
🚧 roryabraham 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/roryabraham in version: 9.4.73-0 🚀
|
|
🤖 No help site changes are required. I did not create a docs PR. Why: This PR adds five lines of code comment to What I checkedThe diff: one file, The behavior being documented is not new. Resetting the workspace split back to WorkspaceInitialPage on narrow layouts already shipped. The comment only records why it works that way, so reviewers stop re-reporting it as a bug (as happened on #99215). There is no behavior change for a help article to describe. The one article that could plausibly be affected is already accurate. Scope note: the underlying behavior — on mobile, tapping Workspaces lands on the workspace list rather than the last sub-page you had open — is a deliberate product decision, not a documented one. If you want that written up for users, that would be a separate content request, not a consequence of this PR. Say the word and I'll draft it. @ahmedGaber93, there is no linked help site PR to review, since no help site change is needed. If you disagree, or if you'd like me to document the mobile Workspaces tab behavior itself as a new help site addition, reply with |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.73-3 🚀
Bundle Size Analysis (Sentry): |




Explanation of Change
Comment-only change. No runtime behavior changes.
navigateToWorkspacesPagedeliberately resets the workspace split back toWorkspaceInitialPageon narrow layouts instead of restoring the last visited workspace sub-page. The existing comment only explained the mechanism ("resetting first prevents any sub-page from flashing"), so the behavior keeps getting reported as a bug — most recently in the linked issue, where a tester expected to land back on the Travel page.This adds the actual reason above the
shouldUseNarrowLayout && !isOnWorkspaceInitialbranch: navigating with the bottom tab bar must always land on a page that still displays that tab bar, and workspace sub-pages hide it — so restoring one would switch the tab and hide the tab bar in a single action.Wording comes from
WojtekBoman's explanation on the issue, who also asked for the snippet to be described more directly.Fixed Issues
$ #99215
PROPOSAL: #99215 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
Melvin ran locally on the branch:
npm run lint-changed— passednpm run spell-changed— passednpm test -- tests/unit/navigateToWorkspacesPageTest.ts tests/unit/hooks/useRestoreWorkspacesTabOnNavigate.test.ts— 18/18 passednpm run typecheck— fails onsrc/hooks/useTransactionInlineEdit.ts(Cannot find name 'originalUseOnyx'). Pre-existing onmainand unrelated to this PR.Verify that no errors appear in the JS console
Offline tests
QA Steps
N/A — comment-only change, no runtime behavior changed. Title includes
[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