[LLM] When deleting a token from the list of hidden tokens, it should be shown in the application #1617
Conversation
π¦ Changeset detectedLatest commit: bd67a06 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
common
Screenshots: β
It seems this PR contains screenshots that are different from the base branch.
linux
|
|
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
ghost
self-requested a review
lvndry
changed the title
|
/generate-screenshots |
apps/ledger-live-mobile/src/screens/Settings/Accounts/index.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-mobile/src/screens/Portfolio/GraphCardContainer.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportBase: 47.77% // Head: 44.13% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1617 +/- ##
===========================================
- Coverage 47.77% 44.13% -3.65%
===========================================
Files 689 627 -62
Lines 30325 26682 -3643
Branches 7949 7209 -740
===========================================
- Hits 14489 11775 -2714
+ Misses 14622 13769 -853
+ Partials 1214 1138 -76
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. β View full report at Codecov. |
lvndry
force-pushed
the
feat/live-3300-hiden-token-redisplay
branch
from
b757865 to
bd67a06
Compare
|
Ready to move to QA ! π |
| @@ -230,6 +233,7 @@ export type ChildAccountRaw = { | |||
| id: string; | |||
| name: string; | |||
| starred?: boolean; | |||
| hidden?: boolean; | |||
There was a problem hiding this comment.
are we sure we want to go that way? isn't it duplicating the current approach of "hidding" token of the system called "blacklist token ids"? We already have a way to hide token and it's via this env and via retriggering a synchronisation: that way, we were just REMOVING the accounts out of tokenAccounts instead of having a boolean to check everywhere (which is costly, it can be annoying to always have to filter these out, all the code that do subAccounts.length would also be badly impacted (eg to condition presence of sub accounts))
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice tips @gre I wasn't aware of the blacklistedTokenIds yet.
The issue I noticed here is that there are 2 reducers triggered for 1 redux action (settings and account).
The action in the settings reducer is correct but not in the account reducer (there are no SHOW_TOKEN action): https://github.com/LedgerHQ/ledger-live/blob/develop/apps/ledger-live-mobile/src/reducers/accounts.ts#L111.
An other issue is that when the withoutToken function is called we remove the token from from the list of subAccounts so how do we show the subAccount again to the user ?
If it's possible I could try to refactor the withoutToken and enableToken functions to actually add/remove the tokens from this list and then rebuild the subAccount only from the id.
However I don't think the current solution is that costing if we use the correct selectors on the redux side (something like enabledCurrencies or enableAccounts) as redux would cache the value
| @@ -230,6 +233,7 @@ export type ChildAccountRaw = { | |||
| id: string; | |||
| name: string; | |||
| starred?: boolean; | |||
| hidden?: boolean; | |||
There was a problem hiding this comment.
|
Creating an other PR using |
π Description
β Context
LLMLLCβ Checklist
π Expectations to reach
Please make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.