Skip to content

feat(assets-controller): add transient per-account loading state for user-visible fetches - #10229

Closed
Prithpal-Sooriya wants to merge 2 commits into
MetaMask:mainfrom
Prithpal-Sooriya:feat/assets-transient-loading-state
Closed

Prithpal-Sooriya wants to merge 2 commits into
MetaMask:mainfrom
Prithpal-Sooriya:feat/assets-transient-loading-state

Conversation

@Prithpal-Sooriya

@Prithpal-Sooriya Prithpal-Sooriya commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

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 getAssets is 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

  • New state field: assetsLoadingStatus: Record<AccountId, AssetsLoadingTrigger> where AssetsLoadingTrigger = 'accountSwitch' | 'unlock'. A string-literal union was chosen over Record<_, 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.
  • Transient by construction: the field's state metadata is persist: false (with includeInStateLogs, includeInDebugSnapshot, usedInUi set). A restarted client always begins with no fetch in flight, so nothing is persisted and no migration is needed.
  • Scoped opt-in: getAssets accepts a new optional trigger option. Only the two user-action call sites pass it — #handleAccountGroupChanged ('accountSwitch') and #runStartupRefresh ('unlock'). Every other getAssets caller (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.
  • Lifecycle in 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.
  • Selectors: getAccountLoadingStatus(state, accountId), isAccountLoading(state, accountId), getAccountsLoadingStatus(state), isAnyAccountLoading(state) are exported from the package index for the UX team, mirroring the existing selectors/balance.ts pattern.

getAssets complexity & decorator evaluation (design note)

The requested analysis of getAssets complexity and whether a TypeScript decorator would be cleaner:

  • getAssets was 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.
  • A decorator (@withLoadingStatus) was evaluated and rejected:
    • No precedent: there are zero decorators in the entire monorepo; the codebase's established idiom for wrapping controller methods is functional composition (e.g. withTrace in src/utils/trace.ts). Introducing the first decorator raises toolchain risk (the repo's tsconfig does not enable experimentalDecorators; the TC39 stage-3 decorators proposal has evolving semantics) and review cost.
    • Not a generic concern: the wrapper is parameter-aware (per-account keys from the first argument, trigger from an options bag property) — a decorator would need bespoke argument inspection anyway, buying no reuse for other methods.
    • Discoverability: decorators hide control flow from readers and from the generated action types; an explicit wrapper keeps the lifecycle visible at the method signature, which is what AssetsController-method-action-types.ts (auto-generated from AssetsController['getAssets']) naturally picks up.
  • The chosen standard implementation adds ~10 lines to getAssets and 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

  • No GitHub issue exists for this feature yet — happy to link one if maintainers want to track it separately.
  • No Jira ticket was provided for this change; if one is required for merge, please advise the ticket ID and I'll attach it to the title.

Checklist

  • I've updated the changelog (Unreleased → Added, packages/assets-controller/CHANGELOG.md).
  • Tests added/updated: 7 new controller tests covering set/clear per trigger, no-trigger fetches, event wiring (accountSwitch, unlock), stateChanged emission, non-persistence via deriveStateFromMetadata, and concurrent-fetch marker safety; 12 selector tests.
  • yarn workspace @metamask/assets-controller run test — 1034/1034 passing.
  • Monorepo yarn build (type check) passing.
  • yarn eslint … --fix --prune-suppressions and yarn lint:misc:check clean on touched files; AssetsController-method-action-types.ts regenerated via yarn workspace @metamask/assets-controller run messenger-action-types:generate.

Test plan

  1. yarn workspace @metamask/assets-controller run test — all suites pass.
  2. Gated-client unit tests observe assetsLoadingStatus mid-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.
  3. deriveStateFromMetadata test 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 assetsLoadingStatus map on AssetsController so the UI can show loading during account switch and unlock refreshes without treating background polls as “user is waiting.”

getAssets gains an optional options.trigger ('accountSwitch' | 'unlock'). When set, the controller sets per-account entries before the fetch and clears them in a finally block (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 #getAssetsInternal unchanged; the public method is a thin lifecycle wrapper. State metadata marks assetsLoadingStatus as persist: false. New selectors/loading helpers (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 tempHealAssetsInfoMetadata preserves 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.

…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.
@Prithpal-Sooriya
Prithpal-Sooriya requested review from a team as code owners September 14, 2026 16:14
@Prithpal-Sooriya

Copy link
Copy Markdown
Contributor Author

Closing in favor of #10230 (same-repo branch, identical commits). The two changelog jobs on this PR (Check changelog and Validate changelog diffs) resolve origin/<head-branch> inside MetaMask/core, which cannot exist for a fork-sourced PR — they failed with fatal: Not a valid object name origin/feat/assets-transient-loading-state regardless of content (the changelog-diff check passes locally on these exact commits: node .github/actions/check-merge-queue-changelogs/check-changelog-diff.cjs → exit 0). All content-level checks were green here; #10230 re-runs the full suite on the same changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant