From 14e9c14696b49fddc4c4f478cad1b4872ab8f945 Mon Sep 17 00:00:00 2001 From: Max Strasinsky <98811342+mstrasinskis@users.noreply.github.com> Date: Thu, 10 Oct 2024 18:46:42 +0200 Subject: [PATCH] Fix after imported remove (#5597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Motivation When a user removes a token, the NNS dapp breaks after navigating to the tokens page. Testing indicates that removing a token triggers navigation to the tokens page because the removed token is then considered as "unknown". **From my observations:** 1. The user clicks “remove.” 2. The component waits for the removal service to complete. 3. The service removes the token from the stores. 4. The store update triggers the navigation. 5. The NNS dapp ends up in an unresolvable state where the wallet component cannot clean up properly because it’s still waiting for the removal service to complete, while the redirection has already occurred, causing the token component to render prematurely. Additionally, navigating while a modal or popup is open also results in a broken state. **Note**: I plan to extend the E2E test to re-import the token after removal to cover this issue. ### Update The main goal of having navigation for unknown tokens is to redirect imported tokens to the /tokens page on sign-out. Since the page reloads in that case, we don’t need to listen for changes throughout the entire session. Therefore, the simplest solution to prevent this confusing state in the NNS dapp is to apply the navigation-on-unknown logic only once, so it isn’t triggered when the user removes a token. # Changes - Apply navigation-on-unknown logic only once. # Tests - Added. # Todos - [ ] Add entry to changelog (if necessary). Not necessary. --- frontend/src/lib/routes/Wallet.svelte | 37 ++++++++++++------- frontend/src/tests/lib/routes/Wallet.spec.ts | 34 ++++++++++++++++- .../src/tests/routes/app/tokens/page.spec.ts | 4 ++ 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/frontend/src/lib/routes/Wallet.svelte b/frontend/src/lib/routes/Wallet.svelte index 26d73074733..900942d847a 100644 --- a/frontend/src/lib/routes/Wallet.svelte +++ b/frontend/src/lib/routes/Wallet.svelte @@ -15,7 +15,7 @@ import SnsWallet from "$lib/pages/SnsWallet.svelte"; import { i18n } from "$lib/stores/i18n"; import { layoutTitleStore } from "$lib/stores/layout.store"; - import { nonNullish } from "@dfinity/utils"; + import { nonNullish, isNullish } from "@dfinity/utils"; import { AppPath } from "$lib/constants/routes.constants"; import { goto } from "$app/navigation"; import { snsAggregatorStore } from "$lib/stores/sns-aggregator.store"; @@ -28,22 +28,31 @@ title: $i18n.wallet.title, }); - let isUnknownToken = false; - $: isUnknownToken = - !$isNnsUniverseStore && - !$isCkBTCUniverseStore && - !$isIcrcTokenUniverseStore && + const redirectIfUnknownToken = () => { + if ( + !$isNnsUniverseStore && + !$isCkBTCUniverseStore && + !$isIcrcTokenUniverseStore && + isNullish($snsProjectSelectedStore) + ) { + // When we can't determine the token type, rather than making guesses, + // it’s more reliable to navigate the user to the all tokens page. + // (imported tokens are not available when signed out). + goto(AppPath.Tokens); + } + }; + let tokensReady = false; + $: tokensReady ||= // We can't be sure that the token is unknown // before we have the list of Sns projects. - nonNullish($snsAggregatorStore.data) && // and imported tokens being loaded - (!$authSignedInStore || nonNullish($importedTokensStore.importedTokens)) && - !nonNullish($snsProjectSelectedStore); - $: if (isUnknownToken) { - // This will also cover the case when the user was logged out - // being on the wallet page of an imported token - // (imported tokens are not available when signed out). - goto(AppPath.Tokens); + nonNullish($snsAggregatorStore.data) && + (!$authSignedInStore || nonNullish($importedTokensStore.importedTokens)); + $: if (tokensReady) { + // Check once per page load to handle navigation after signing out from the imported token page. + // Redirecting after token removal may cause a broken state on the tokens page (e.g., blocked scrolling or actions) + // as the Wallet component still waits for the remove completion while the tokens page is already rendered. + redirectIfUnknownToken(); } diff --git a/frontend/src/tests/lib/routes/Wallet.spec.ts b/frontend/src/tests/lib/routes/Wallet.spec.ts index 4a04434f8e0..45335a6b90e 100644 --- a/frontend/src/tests/lib/routes/Wallet.spec.ts +++ b/frontend/src/tests/lib/routes/Wallet.spec.ts @@ -284,7 +284,7 @@ describe("Wallet", () => { expect(await pagePo.getWalletPageHeadingPo().getTitle()).toBe("1.11 ckETH"); }); - describe("unknown token", () => { + describe("unknown token redirection", () => { const unknownUniverseId = "aaaaa-aa"; beforeEach(() => { @@ -408,5 +408,37 @@ describe("Wallet", () => { expect(get(pageStore).path).toEqual(AppPath.Wallet); }); + + it("should not redirect when a token becomes unknown during session", async () => { + page.mock({ + data: { universe: importedTokenId.toText() }, + routeId: AppPath.Wallet, + }); + setSnsProjects([{}]); + importedTokensStore.set({ + importedTokens: [ + { + ledgerCanisterId: importedTokenId, + indexCanisterId: undefined, + }, + ], + certified: true, + }); + + expect(get(pageStore).path).toEqual(AppPath.Wallet); + render(Wallet, { + props: { + accountIdentifier: undefined, + }, + }); + await runResolvedPromises(); + + expect(get(pageStore).path).toEqual(AppPath.Wallet); + + // Remove the imported token + importedTokensStore.remove(importedTokenId); + await runResolvedPromises(); + expect(get(pageStore).path).toEqual(AppPath.Wallet); + }); }); }); diff --git a/frontend/src/tests/routes/app/tokens/page.spec.ts b/frontend/src/tests/routes/app/tokens/page.spec.ts index 36a015fa9bc..d9353013b37 100644 --- a/frontend/src/tests/routes/app/tokens/page.spec.ts +++ b/frontend/src/tests/routes/app/tokens/page.spec.ts @@ -878,6 +878,9 @@ describe("Tokens route", () => { indexCanisterId: undefined, }, ]); + expect(get(failedImportedTokenLedgerIdsStore)).toEqual([ + failedImportedTokenId.toText(), + ]); await removeConfirmationPo.clickYes(); await removeConfirmationPo.waitForClosed(); @@ -887,6 +890,7 @@ describe("Tokens route", () => { importedToken1Data, importedToken2Data, ]); + expect(get(failedImportedTokenLedgerIdsStore)).toEqual([]); expect(await po.getTokensPagePo().getTokenNames()).toEqual([ "Internet Computer", "ckBTC",