fix: restore the Home tab the user actually left (WALLET-1360) - #1433
Open
ost-ptk wants to merge 7 commits into
Open
Conversation
Adds four failing Playwright scenarios for WALLET-1360: the Home tab is rebuilt from whatever activeTabId an earlier navigation stamped onto the history entry, so back from token details lands on NFTs or Activity. Also covers the navigation menu discarding location.state.
Introduces a popup-session-scoped owner for the Home tab, mounted inside HashRouter above the navigation-menu branch so it survives every unmount of the Home page. Not consumed yet.
Tabs is now controlled-optional and addresses tabs by name instead of by position in the children array. Home reads and writes the active tab through HomeTabProvider, so back navigation restores the tab the user actually left rather than the one an earlier navigation intended. Also replaces the hardcoded 33% tab width with flex: 1, which removed the component's undocumented three-tab-only constraint.
The four sites that mean 'send the user to Activity' now say so directly via setActiveHomeTab; the two that meant 'go back' are plain navigate(-1), because the session owner already knows which tab to return to. Removes LocationState.activeTabId, LocationState.navigateHome, the navigateHome prop chain through DeployPlate, and HomePageTabsId, whose numeric indices were never tied to the order the tabs render in. Deleting activeTabId from LocationState also required removing the Home page's scroll effect that was keyed on it. That effect either duplicated TokensList, which already scrolls to top on mount, or competed with the position restoration in NftList and DeploysList; each list owns its own scroll behaviour now.
The three helpers replaced the whole state object with { showNavigationMenu },
discarding everything the current route depends on. Deploy details reads
location.state.deploy and its route carries no parameter, so opening and
closing the menu left the page blank.
Deriving the default tab from children[0] threw on an empty array, where the previous index-based implementation rendered nothing; the type does not rule an empty array out. The internal tab state was also written on every click even when the parent supplies activeTabName and owns the value, letting the shadow state drift from the controlled one.
The success copy told the user to check the 'Transactions' or 'Deploys' tab depending on the network, but neither exists — the Home tabs are Tokens, NFTs and Activity, and have been for some time. The sentence was a template literal inside <Trans>, so i18next-parser never extracted it and no catalog carried a translation. Making it a static string fixes that too. Dropping the network branch leaves selectIsCasper2Network unused in this component.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Going back from a token, an NFT or a deploy could land the user on the wrong Home tab — the ticket reports Activity, but NFTs happens too, depending only on what the user did earlier.
Root cause
The active Home tab lived in the
Tabscomponent's localuseState, initialised once fromlocation.state.activeTabId. React Router bindsstateto a history entry, and nothing wrote the user's tab click back into it. So:activeTabId.useState; the history entry kept the old value.navigate(-1)) returned to that same Home entry, andTabsre-initialised from the staleactiveTabId.The underlying design problem is that one field served two different jobs: restore the tab the user was on, and override it to Activity after a submission. Because both wrote
location.state.activeTabId, an override stuck to the history entry permanently and later impersonated a restore.Fix
HomeTabProvider/useHomeTab(src/apps/popup/hooks/use-home-tab.tsx) owns the active tab for the popup session. It is mounted inside<HashRouter>above<AppRoutes>, which matters:AppRoutesbranches onshowNavigationMenuand renders a separate<Routes>tree that unmounts Home.Tabsis now controlled-optional and addresses tabs by name instead of position in the children array.HomePageTabsId(numeric indices that were never tied to the render order) is gone;HomePageTabNameis the single source of truth.RecipientTabskeeps working uncontrolled and is unchanged.setActiveHomeTab. The two that meant "go back" are plainnavigate(-1), since the session owner already knows the tab — which made thenavigateHomeprop chain throughDeployPlatedead, so it is deleted along withLocationState.activeTabId.storage.local. Closing the popup (or auto-lock) resets it to Tokens, as before.Rider:
useNavigationMenudiscarded the rest oflocation.stateSeparate pre-existing bug, fixed here because it lives in the same mechanism. The three helpers replaced the whole state object with
{ showNavigationMenu }. Consequences today, independent of the tab bug:location.state.deployand its route/deploys-detailscarries no parameter, so there is no fallback anduseFetchSingleDeploynever fires.transfer-nftreadslocation.state.nftData.:tokenNameparam.The fix spreads the existing state. Purely additive.
Second rider: the transfer success screen named a tab that does not exist
The success copy read "You can check its status in the Transactions / Deploys tab on your Wallet home page", branching on
isCasper2Network. Neither tab exists — the Home tabs are Tokens, NFTs and Activity, and have been for some time.It was also a template literal inside
<Trans>, soi18next-parsernever extracted it and no catalog carries a translation for it. Making the sentence static fixes the extraction too, and dropping the network branch leavesselectIsCasper2Networkunused in the component.Notes for reviewers and QA
navigate(Home), every "go Home" path (header Close/Cancel/Done, contacts, account settings, rename/remove account, RateApp completion, account and network switches) now returns to the last-used tab instead of always Tokens. That is what the design asks for, but it is the thing most likely to surprise manual testing.askForReviewAfterisnull, so Done always routes to the RateApp screen. In production any returning user who has already seen the rate prompt takes the Home branch. Repro: dismiss the rate prompt once, then send CSPR → Done should land on Activity.ActiveTabContainer/TabContainernow useflex: 1instead ofwidth: calc(33% - 8px). These already inheritedflex-grow: 1fromTabsContainer'sflexGrow={1}, so with three tabs the old rule resolved to a base of0.33·w − 8plus an equal share of the0.01·w + 24free space — exactlyw/3, which is whatflex: 1gives directly. The change also removes the component's undocumented three-tabs-only constraint. Note this affects the recipient tab strip (Recent / My accounts / Contacts) in the transfer flow as well as Home, so it is worth a quick look at both.TransferSuccessScreenlives insrc/libs/ui/components/and is re-exported from the barrel all five apps import, but it now requires aHomeTabProviderancestor. All current call sites are popup pages, so it is safe today; moving it undersrc/apps/popup/pages/would make the coupling honest and drop it from the other four bundles.Testing
npm run ci-check— passes (405 jest tests, 0 lint errors, tsc clean, coverage gates met).e2e-tests/popup/common/home-tabs.spec.ts— four scenarios, all of which failed before the fix and pass after it: the NFTs variant, the Activity variant, the tab surviving a menu toggle, and deploy details surviving a menu toggle.e2e-tests/popupsuite — no regressions. One unrelated flake (signature-request-scenarios › should cancel the signing process) passed on retry.Linked tickets
WALLET-1360
Checklist
Make sure this PR title follows semantic release conventions: https://semantic-release.gitbook.io/semantic-release/#commit-message-format
If the PR adds any new text to the UI, make sure they are localized — see the note below
On localization: this PR deliberately does not regenerate
lang/. Runningnpm run locale:extract_potproduces 410 new msgids, of which only one is from this PR — the other 409 are accumulated drift from strings added to the codebase since the last extraction, which is untracked in every one of the 10 locales. Committing that here would bury the actual change. The one new string renders in English (i18next falls back to the key), exactly as the sentence it replaces did — it was never extractable either. The catalog drift deserves its own ticket.Include a screenshot or recording if implementing significant UI or user flow change
When this PR affects architecture changes wait for review from Dmytro before merging