feat(assets-controller): add transient per-account loading state for user-visible fetches - #10230
Open
Prithpal-Sooriya wants to merge 9 commits into
Open
Prithpal-Sooriya wants to merge 9 commits into
Prithpal-Sooriya wants to merge 9 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.
Closed
5 tasks
Prithpal-Sooriya
left a comment
Contributor
Author
There was a problem hiding this comment.
Look through self-review comments
…, fix queued-switch gap - Replace the trigger union with a simple 'loading' | 'loaded' status, set before acquiring the refresh mutex so queued switches are marked immediately, and settled to 'loaded' when the fetch finishes - Move the lifecycle into the account-switch and unlock handlers; revert getAssets to its original form (no trigger option), keeping the generated action-types file unchanged - Trim comments and shorten the changelog entry
…sh settle Track a per-account token when the loading marker is set; only the invocation that owns the token settles it to 'loaded'. An older refresh finishing while a newer queued refresh owns the marker now leaves it 'loading' instead of clobbering it to 'loaded'.
Per review feedback, mark the assets loading status inside getAssets itself using a trackAssetsLoading method decorator instead of in the startup-refresh and account-group-change handlers, keeping the token ownership logic so an older overlapping fetch cannot clobber a newer fetch's loading marker.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5355bd1. Configure here.
Per review feedback: base the decorator's controller type on the real AssetsController (picking its state), store the loading ownership tokens in the controller's non-persisted state instead of a module WeakMap, and add isolated tests for the decorator. Calls that do not force an update are cache reads and no longer mark the loading status.
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
Medium Risk
Changes
getAssetslifecycle and new UI-facing state around fetch concurrency; behavior is scoped to forced updates but affects account-switch/unlock UX paths.Overview
Adds transient, non-persisted per-account loading signals on
AssetsControllerso the UI can tell when a forced assets fetch is in flight.New state includes
assetsLoadingStatus('loading'|'loaded') and internalassetsLoadingTokensto attribute in-flight work.getAssetsis wrapped with@trackAssetsLoading, which marks requested accounts as loading at the start offorceUpdate: truecalls and settles them to'loaded'in afinallyblock (including on failure). Cache-only calls withoutforceUpdateare not tracked, and overlapping fetches cannot clear a newer invocation’s marker.Loading selectors are exported (
getAccountLoadingStatus,isAccountLoading, group/selected-group helpers). Metadata marksassetsLoadingStatusas UI-visible butpersist: false. Tests cover account switch, unlock, queued switches, concurrent fetches, failures, and persistence exclusion.Also adjusts the temporary ASSETS-3346 heal path to merge healed fields via a spread inside
update.Reviewed by Cursor Bugbot for commit 14b4d9d. Bugbot is set up for automated code reviews on this repo. Configure here.