Skip to content

Commit

Permalink
Fix after imported remove (#5597)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
mstrasinskis authored Oct 10, 2024
1 parent 3c14cdb commit 14e9c14
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 15 deletions.
37 changes: 23 additions & 14 deletions frontend/src/lib/routes/Wallet.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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();
}
</script>

Expand Down
34 changes: 33 additions & 1 deletion frontend/src/tests/lib/routes/Wallet.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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);
});
});
});
4 changes: 4 additions & 0 deletions frontend/src/tests/routes/app/tokens/page.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,9 @@ describe("Tokens route", () => {
indexCanisterId: undefined,
},
]);
expect(get(failedImportedTokenLedgerIdsStore)).toEqual([
failedImportedTokenId.toText(),
]);

await removeConfirmationPo.clickYes();
await removeConfirmationPo.waitForClosed();
Expand All @@ -887,6 +890,7 @@ describe("Tokens route", () => {
importedToken1Data,
importedToken2Data,
]);
expect(get(failedImportedTokenLedgerIdsStore)).toEqual([]);
expect(await po.getTokensPagePo().getTokenNames()).toEqual([
"Internet Computer",
"ckBTC",
Expand Down

0 comments on commit 14e9c14

Please sign in to comment.