-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip failed balance reload #5550
base: main
Are you sure you want to change the base?
Conversation
|
||
onMount(() => { | ||
loadCkBTCTokens(); | ||
}); | ||
|
||
let loadSnsAccountsBalancesRequested = false; | ||
let loadCkBTCAccountsBalancesRequested = false; | ||
let loadedIcrcAccountsBalancesRequested = new Set<CanisterIdString>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is reset when you navigate to another page, right?
Do we reload the balances every time you navigate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Correct.
- Yes.
return; | ||
} | ||
newIds.forEach((id) => loadedIcrcAccountsBalancesRequested.add(id)); | ||
await loadAccountsBalances(newIds); | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub won't allow me to comment below, but on line 141:
$: (async () => {
if ($authSignedInStore) {
await Promise.allSettled([
loadSnsAccountsBalances($snsProjectsCommittedStore),
loadCkBTCAccountsBalances($ckBTCUniversesStore),
loadIcrcTokenAccounts($icrcCanistersStore),
]);
}
})();
this will execute when any of the stores change.
But we don't want to reload store B because store A changed, right?
Shouldn't this be 3 separate blocks?
it("should not reload balances after an imported token becomes failed", async () => { | ||
const po = await renderPage(); | ||
const rows = await po.getTokensPagePo().getTokensTable().getRows(); | ||
const notFailedTokenCount = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We number what this number is, right? It's always the same.
Can we expect it so it's not accidentally 0 or something?
notFailedTokenCount | ||
); | ||
|
||
failedImportedTokenLedgerIdsStore.add(importedToken2Id.toText()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we check that importedToken2Id
is not already in failedImportedTokenLedgerIdsStore
so we know the test actually tests something?
Motivation
Currently, on the tokens page, there is a flag-based mechanism to prevent reloading of token data (here and here). This is applied to the Sns and ckBTC tokens but not to the ICRC tokens, as the ICRC token list was previously static. Now, when the dapp fails to load a token, the failed token is removed from the ICRC token list, which triggers a reload of all ICRC token data. To prevent this, we apply the same flag mechanism to avoid unnecessary reloading.
Changes
Tests
Todos
Not necessary.