feat(assets-controller): add transient per-account loading state for user-visible fetches - #10229
Closed
Prithpal-Sooriya wants to merge 2 commits into
Closed
Prithpal-Sooriya wants to merge 2 commits into
Prithpal-Sooriya wants to merge 2 commits into
Conversation
…user-visible fetches Add a non-persisted 'assetsLoadingStatus' state map (Record<AccountId, 'accountSwitch' | 'unlock'>) that is populated while getAssets runs for user-visible moments and cleared when the fetch settles. Only the two user-action fetch sites (account group change, unlock/startup refresh) pass the new optional 'trigger' option, so background fetches (polling, price/tx/currency refreshes) never flap the indicator. The field is marked persist: false, so no migration is needed. Also export selectors (getAccountLoadingStatus, isAccountLoading, getAccountsLoadingStatus, isAnyAccountLoading) for the UX layer. getAssets is refactored into a thin lifecycle wrapper around the extracted #getAssetsInternal pipeline (body unchanged), and tempHealAssetsInfoMetadata is made generic over the state slice so its return type stays assignable to the widened controller state.
This was referenced Sep 14, 2026
Contributor
Author
|
Closing in favor of #10230 (same-repo branch, identical commits). The two changelog jobs on this PR ( |
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
Adds a transient, non-persisted per-account loading state to
AssetsController, published only while assets are being fetched for specific user-visible UX moments (account switch, unlock), plus selectors for the UX layer to read it.Problem
The UI has no signal for "an assets fetch is in flight because the user just did something" (switched accounts / unlocked the wallet). Balance state can't be used to infer this (stale balances are indistinguishable from fresh ones), and
getAssetsis called constantly in the background (polling, price/currency/tx refreshes) — so "any fetch in flight" is not the same as "the user is waiting".Approach
assetsLoadingStatus: Record<AccountId, AssetsLoadingTrigger>whereAssetsLoadingTrigger = 'accountSwitch' | 'unlock'. A string-literal union was chosen overRecord<_, boolean>: the key's presence already encodes "loading", the value tells the UI which moment is loading (so it can render the right skeleton/label), and the union is extensible for future triggers without a second field or a breaking change.persist: false(withincludeInStateLogs,includeInDebugSnapshot,usedInUiset). A restarted client always begins with no fetch in flight, so nothing is persisted and no migration is needed.getAssetsaccepts a new optionaltriggeroption. Only the two user-action call sites pass it —#handleAccountGroupChanged('accountSwitch') and#runStartupRefresh('unlock'). Every othergetAssetscaller (polling, price updates, tx confirmations, currency changes, network events) is unchanged and never publishes loading state, so the indicator cannot flap due to background work.getAssets: the public method is now a thin wrapper — set the entries,try { await } finally { clear }— around the extracted, unchanged fetch pipeline (#getAssetsInternal). Entries are cleared conditionally (only if they still hold this fetch's trigger), so an older fetch settling can never clobber a newer fetch's marker for the same account.getAccountLoadingStatus(state, accountId),isAccountLoading(state, accountId),getAccountsLoadingStatus(state),isAnyAccountLoading(state)are exported from the package index for the UX team, mirroring the existingselectors/balance.tspattern.getAssetscomplexity & decorator evaluation (design note)The requested analysis of
getAssetscomplexity and whether a TypeScript decorator would be cleaner:getAssetswas already a long, multi-stage pipeline (force-update fast/slow paths, middleware chain, merge/replace modes). The loading concern is not deeply entangled with that logic — it is a strict request-scoped lifecycle (set → run → clear). Modeling it inside the pipeline body would have raised cyclomatic complexity and added error-handling paths through ~180 lines that today have none.@withLoadingStatus) was evaluated and rejected:withTraceinsrc/utils/trace.ts). Introducing the first decorator raises toolchain risk (the repo's tsconfig does not enableexperimentalDecorators; the TC39 stage-3 decorators proposal has evolving semantics) and review cost.triggerfrom an options bag property) — a decorator would need bespoke argument inspection anyway, buying no reuse for other methods.AssetsController-method-action-types.ts(auto-generated fromAssetsController['getAssets']) naturally picks up.getAssetsand two small private helpers, with zero cyclomatic-complexity growth in the fetch pipeline itself (its body moved verbatim to#getAssetsInternal). This is the smallest diff that satisfies the lifecycle requirement while staying within repo idioms.Related issues
Checklist
packages/assets-controller/CHANGELOG.md).accountSwitch,unlock),stateChangedemission, non-persistence viaderiveStateFromMetadata, and concurrent-fetch marker safety; 12 selector tests.yarn workspace @metamask/assets-controller run test— 1034/1034 passing.yarn build(type check) passing.yarn eslint … --fix --prune-suppressionsandyarn lint:misc:checkclean on touched files;AssetsController-method-action-types.tsregenerated viayarn workspace @metamask/assets-controller run messenger-action-types:generate.Test plan
yarn workspace @metamask/assets-controller run test— all suites pass.assetsLoadingStatusmid-flight (Accounts API calls frozen via an armed gate) and after release — entries present exactly while the fetch is in flight and cleared on settle.deriveStateFromMetadatatest asserts the field is excluded from persisted state.Note
Low Risk
Additive transient UI state with opt-in triggers; asset fetch logic is unchanged aside from a wrapper, and background refreshes still omit loading markers.
Overview
Adds a transient, non-persisted
assetsLoadingStatusmap onAssetsControllerso the UI can show loading during account switch and unlock refreshes without treating background polls as “user is waiting.”getAssetsgains an optionaloptions.trigger('accountSwitch' | 'unlock'). When set, the controller sets per-account entries before the fetch and clears them in afinallyblock (including on failure), only removing markers that still match that trigger so overlapping fetches do not clobber each other. Only startup refresh after unlock and selected account-group change pass a trigger; other callers stay unchanged.The fetch pipeline is extracted to
#getAssetsInternalunchanged; the public method is a thin lifecycle wrapper. State metadata marksassetsLoadingStatusaspersist: false. Newselectors/loadinghelpers (getAccountLoadingStatus,isAccountLoading, etc.) are exported for UX subscriptions. Tests cover in-flight/clear behavior, event wiring,stateChanged, and non-persistence.A small generic typing tweak to
tempHealAssetsInfoMetadatapreserves full controller state types when healing runs.Reviewed by Cursor Bugbot for commit 47b7803. Bugbot is set up for automated code reviews on this repo. Configure here.