fix(icons): keep the asset icon that actually loads, not the first one listed

[piyalbasu]

TL;DR

Asset icons come from sources we don't control — several curated token lists, plus the issuer's SEP-1 TOML — and any of them can carry a URL that no longer serves an image. Resolution used to take whichever URL it read last and hand it straight to the <img>, so a dead URL meant no icon. Worse, when the image then failed, the retry wrote a "no icon" marker into the cache that persists to disk, and the lookup reads that marker as "we already tried, never look again" — so one transient failure removed the asset's icon permanently.

Balance icons are now resolved by loading every URL the lists offer and keeping the first that actually renders, stopping there. Only a URL we've watched render reaches the cache, so a later cache hit is trusted without re-checking.

The whole search shares a single 1.2s budget, split across the candidates still to try — so one hanging host can neither blow the budget nor starve a healthy candidate behind it.

Scope: this is a hotfix for the account view, where the bug was reported. Asset search, sign-transaction rows and history rows still render the first listed URL unchecked; covering them needs a different shape and is called out as a follow-up below.

Reported against USDT0:GATISXX6BZ6NC7IKQBY37CJD4SOZL3CYZJWXEDG6JVIY4WBS6KXJHN6Q, which sits on both the Soroswap list (a direct HTTPS icon) and the LOBSTR list (an ipfs.io gateway icon). LOBSTR won on ordering alone, and this issuer publishes no home domain — its master key weight is 0, so it never can — leaving the TOML fallback permanently unreachable and "no icon" as the final answer.

Steps to reproduce

1. Hold USDT0:GATISXX6BZ6NC7IKQBY37CJD4SOZL3CYZJWXEDG6JVIY4WBS6KXJHN6Q on Mainnet with the default asset lists enabled.
2. Open the account view. The icon resolves to the ipfs.io gateway URL from the LOBSTR list rather than the direct HTTPS URL from the Soroswap list.
3. Let that image fail once (the gateway throttles browser traffic).
4. The icon is now gone, and stays gone across popup reloads and extension restarts.

Upgrade note

Wallets that already hit this have a persisted "no icon" marker on disk, and could not recover on their own — with no icon there is no <img>, so nothing fires the error handler that would trigger a retry. A one-time migration clears those on upgrade, so the fix reaches the wallets that actually have the bug.

---

Implementation details (for agents)

Root cause chain

1. getIconFromTokenLists iterated the lists in order but its break only exited the inner record loop, so the outer loop ran to completion and the last matching list overwrote every earlier match. Nothing ever checked whether the chosen URL loads.
2. AccountAssets handles the <img> onError by calling retryAssetIcon, which sent iconUrl: null to CACHE_ASSET_ICON and then consulted only getIconUrlFromIssuer (home domain → SEP-1 TOML). The asset lists were never re-checked.
3. cacheAssetIcon persisted that null verbatim into CACHED_ASSET_ICONS_ID in the background's localStore, and getCachedAssetIconList hands the raw map back including nulls.
4. getAssetIcons short-circuits on a cached null with a comment claiming the null lives only in Redux and will be retried on the next app load. That comment was wrong — the null was on disk, so the lookup was never attempted again.

For USDT0 the TOML branch can never recover: Horizon reports home_domain: null for the issuer with thresholds {0,0,0} and a single signer at weight 0. A valid stellar.toml with the correct image does exist at usdt0.to, but the wallet has no way to discover that domain.

Key code

Candidate selection and the shared budget, split so no single host can starve the rest:

freighter/@shared/api/helpers/iconProbe.ts

Lines 70 to 98 in 39ed705

	export const firstLoadableIconUrl = async (
	urls: string[],
	{
	budgetMs = ICON_LOAD_BUDGET_MS,
	probe = canLoadIcon,
	}: { budgetMs?: number; probe?: IconProbe } = {},
	): Promise<string | undefined> => {
	const deadline = Date.now() + budgetMs;
	for (let index = 0; index < urls.length; index += 1) {
	const remaining = deadline - Date.now();
	if (remaining <= 0) {
	return undefined;
	}
	// Split what's left evenly across the candidates still to try, rather than
	// offering the whole remainder to the next one. A throttled host that never
	// answers would otherwise spend the entire budget and starve a healthy
	// candidate behind it — the very failure this is here to prevent. A
	// candidate that answers quickly leaves its unused share to the rest.
	const share = Math.ceil(remaining / (urls.length - index));
	if (await probe(urls[index], share)) {
	return urls[index];
	}
	}
	return undefined;
	};

Why the budget is 1.2s:

freighter/@shared/api/helpers/iconProbe.ts

Lines 13 to 28 in 39ed705

	/**
	* Total time allowed to find a working icon for one asset, shared across all of
	* its candidates rather than granted per candidate — so the worst case stays
	* flat instead of scaling with how many lists happen to carry the asset.
	*
	* Measured against the real candidates for
	* USDT0:GATISXX6BZ6NC7IKQBY37CJD4SOZL3CYZJWXEDG6JVIY4WBS6KXJHN6Q, both ~8KB
	* pngs: 130-300ms each including cold DNS + TLS. 1.2s leaves several times that
	* headroom for a slow connection while staying well short of a visible stall.
	*
	* Running out of budget is cheap: the asset falls back to its generic icon for
	* this load only. Nothing negative is persisted (see cacheAssetIcon), and the
	* browser generally finishes the abandoned request into its own HTTP cache, so
	* the next attempt resolves immediately.
	*/
	export const ICON_LOAD_BUDGET_MS = 1200;

The serialized cache write:

freighter/extension/src/background/messageListener/handlers/cacheAssetIcon.ts

Lines 21 to 51 in 39ed705

	export const cacheAssetIcon = async ({
	request,
	localStore,
	}: {
	request: CacheAssetIconMessage;
	localStore: DataStorageAccess;
	}) => {
	const { assetCanonical, iconUrl } = request;
	const applyUpdate = async () => {
	const assetIconCache =
	(await localStore.getItem(CACHED_ASSET_ICONS_ID)) || {};
	if (iconUrl) {
	assetIconCache[assetCanonical] = iconUrl;
	} else {
	// A falsy iconUrl means "forget what we cached" (retryAssetIcon sends
	// null to drop a url that failed to load). Delete the entry rather than
	// storing the null: getAssetIcons treats a cached null as "already
	// tried, never look again", and this cache is persisted to disk — so a
	// single transient image failure would blacklist the asset's icon for
	// good.
	delete assetIconCache[assetCanonical];
	}
	await localStore.setItem(CACHED_ASSET_ICONS_ID, assetIconCache);
	};
	pendingCacheWrite = pendingCacheWrite.then(applyUpdate, applyUpdate);
	return pendingCacheWrite;
	};

The migration that reaches already-broken wallets:

freighter/extension/src/background/helpers/dataStorage.ts

Lines 394 to 409 in 39ed705

	export const dropNullAssetIconCacheEntries = async () => {
	const localStore = dataStorageAccess(browserLocalStorage);
	const storageVersion = (await localStore.getItem(STORAGE_VERSION)) as string;
	if (shouldRunMigration({ storageVersion, migrationVersion: "5.46.0" })) {
	const assetIconCache =
	(await localStore.getItem(CACHED_ASSET_ICONS_ID)) || {};
	const cleaned = Object.fromEntries(
	Object.entries(assetIconCache).filter(([, iconUrl]) => iconUrl),
	);
	await localStore.setItem(CACHED_ASSET_ICONS_ID, cleaned);
	await migrateDataStorageVersion("5.46.0");
	}
	};

What changed

• New @shared/api/helpers/iconProbe.ts — canLoadIcon is an <img> load rather than a fetch: the extension declares no host_permissions, so a cross-origin fetch to an arbitrary icon host is CORS-gated while an image load is not. That confines this to a DOM context, which is fine — icon resolution only runs in the popup, never in the service worker. The probe is injectable, so tests drive selection logic without a network.
  • ICON_LOAD_BUDGET_MS = 1200, spent as one deadline across all candidates, so the worst case doesn't scale with candidate count. Measured against the two real USDT0 candidates (~8KB PNGs): 130-300ms each including cold DNS + TLS.
  • The remaining budget is split across the candidates still to try, so a host that hangs can't spend it all and leave a working candidate untried — the very failure this change exists to prevent. A candidate that answers quickly leaves its unused share to the rest.
  • On timeout the <img> src is deliberately not cleared, so the browser finishes the abandoned request into its own HTTP cache and a merely-slow candidate resolves instantly next time.
  • mapWithConcurrency resolves several assets at once, since getAssetIcons is awaited before the balances render.
• getIconCandidatesFromTokenLists (new) — pure collector returning every matching icon URL, deduped, in list order. Order decides probe order, not priority.
• getIconFromTokenLists — unchanged in behavior for its existing callers: it still returns the first candidate without loading it. Those three surfaces are out of scope for this hotfix (see follow-ups).
• getAssetIcons — split into a synchronous cache pass and a concurrent lookup pass. A cache hit is not re-probed: it was proven loadable when written, and re-probing every load would defeat the cache; a URL that rots later is caught by the onError -> retryAssetIcon path. The TOML fallback gets the same treatment, and a TOML URL that doesn't load is cleared rather than left for the next load to trust.
• cacheAssetIcon — a falsy iconUrl now deletes the entry instead of storing null, and updates are serialized: the cache is one storage key, concurrent resolution means overlapping read-modify-writes, and storage hands each handler its own copy, so unchained writes lose icons. CacheAssetIconMessage.iconUrl is now string | null so the delete command is part of the contract rather than something callers cast past.
• dropNullAssetIconCacheEntries — one-time migration (storage v5.46.0) sweeping the nulls earlier versions persisted, wired into versionedMigration. Without it the fix never reaches the wallets that have the bug.
• retryAssetIcon — re-resolves through the same chain with the URL that just failed removed from the running, so it can never hand back the same broken src; clears a rejected TOML URL; and matches contract assets by contract id (Soroban balances arrive with a contract id as their key, and the lists key those by contract).

Verification

• 40 new tests, each watched failing first. Highlights: the budget is shared rather than per-candidate (three candidates, one 20ms budget → exactly one load attempted); getAssetIcons picks the loadable candidate over the first-listed one, caches what it settled on rather than what it rejected, and doesn't re-probe a cache hit; the concurrency is pinned by measuring peak in-flight loads through the real getAssetIcons, so it can't silently regress to serial; retryAssetIcon never re-offers the failed URL even when that URL would load; the migration keeps real icons while dropping nulls.
• Regressions are checked by diffing failing test names against the base, not counts — a suite-level or count-level comparison hid a real break during this work (a history test failing at exactly the 1.2s budget). The final diff is empty in both directions.
• tsc --noEmit: 243 errors on master, 243 here — no new ones. Those are pre-existing stellar-sdk v17 drift (xdr.encodeBytes, fromXdr/toXdr casing) from the v17 back-port.
• ESLint clean on all changed source files; Prettier applied.

Note for reviewers: the pre-commit hook (.husky/addTranslations.sh → yarn build:extension:translations) fails on a clean master checkout for that same pre-existing v17 drift, so these commits were made with --no-verify. No CI job runs that build, so it doesn't affect this PR — but it does mean nobody can commit to this repo without the flag. Worth a separate fix.

Follow-ups / out of scope

• Asset search, sign-transaction rows and history rows still render the first listed URL unchecked. Probing them synchronously needs a session-wide memo and a warm-up pass to stay affordable, and that machinery caused its own problems when tried here (it was reverted in this PR — see the revert commit). The better shape is to hand the candidate list to the image component and let it fall through on error: no probing ahead of render, no shared memo, no added latency. Worth its own change.
• The persisted null was also the only cross-session negative cache. An asset with genuinely no discoverable icon now re-runs the chain on every cold popup open. That wants a TTL or an attempt count rather than a permanent verdict, and is deliberately left out of this PR.
• Contract matching still uses new RegExp(contractId, "i") where a case-insensitive string comparison is what's meant, which also makes it a substring match. Contract IDs are StrKey so there's no injection surface, but it's needlessly fragile.
• getIconCandidatesFromTokenLists reports one canonicalAsset, taken from the first match. A caller passing a contractId and an issuerId that disagree would get a canonical keyed on the contract. Nothing does today — neither backend attaches a contractId to classic balances — but it's a trap for a future caller.
• Sign-transaction resolves the issuer TOML before the token lists, the opposite order from every other surface. Not changed here, but it means that path pays a Horizon round trip for assets the lists could have answered instantly.

[github-actions]

PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-9a89b8e0ed33a997b435
Backend: sandbox (piyalbasu). SDF collaborators only — install instructions in the release description.

[Copilot]

Pull request overview

Improves balance icon resolution by probing candidate URLs and avoiding persistent negative cache entries.

Changes:

• Collects and probes token-list/TOML icon candidates.
• Revises cache clearing and retry behavior.
• Adds regression and probe tests.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
@shared/api/helpers/iconProbe.ts	Adds bounded image probing.
@shared/api/helpers/getIconFromTokenList.ts	Collects ordered, deduplicated candidates.
@shared/api/internal.ts	Integrates probing and retry logic.
extension/src/background/messageListener/handlers/cacheAssetIcon.ts	Deletes cleared cache entries.
extension/src/popup/components/account/AccountAssets/index.tsx	Supplies token lists during retries.
@shared/api/helpers/__tests__/iconProbe.test.ts	Tests probing behavior.
extension/src/popup/helpers/__tests__/getIconFromTokenLists.test.js	Tests candidate collection.
@shared/api/__tests__/internal.test.ts	Tests resolution and retries.
extension/src/background/messageListener/handlers/__tests__/cacheAssetIcon.test.ts	Tests cache deletion.
extension/src/popup/views/__tests__/Account.test.tsx	Updates account icon mocks.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Suppressed comments (2)

@shared/api/helpers/iconProbe.ts:84

• A candidate that never emits load or error receives the entire remaining budget, so after it times out the deadline has expired and every later URL is skipped. Thus a hanging first URL still prevents a valid second URL from being selected. Probe candidates concurrently under the shared deadline, or reserve budget for later candidates.

    const remaining = deadline - Date.now();
    if (remaining <= 0) {
      return undefined;
    }
    if (await probe(url, remaining)) {

@shared/api/internal.ts:1569

• Returning a replacement map does not display the replacement in the current popup. AssetIcon sets its local hasError to true on the original failure, and line 109 then keeps imgSrc pinned to ImageMissingIcon even after canonicalAsset changes. Reset the error/loading state when the resolved URL changes (and add a retry rendering test).

  if (tokenListIcon) {
    await cacheAssetIconUrl(canonical, tokenListIcon);
    newAssetIcons[canonical] = tokenListIcon;
    return newAssetIcons;

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Suppressed comments (1)

@shared/api/internal.ts:1431

• These concurrent workers each call cacheAssetIconUrl, while the background handler updates the cache with a read-modify-write of the entire CACHED_ASSET_ICONS_ID map. Two messages can read the same snapshot and then overwrite each other, so only the last resolved icon remains persisted. Keep image probes concurrent, but serialize cache writes or send one batched background update.

    const resolvedIcons = await mapWithConcurrency(
      needsLookup,
      ICON_LOOKUP_CONCURRENCY,
      ({ key, code, contractId }) =>

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

extension/src/popup/components/account/AccountAssets/index.tsx:272

• Passing alternate candidates into the retry does not make the replacement visible. AssetIcon sets its local hasError to true on the failing image, and thereafter computes imgSrc as ImageMissingIcon regardless of the updated assetIcons; this update cannot display the URL returned by retryAssetIcon until a remount. Reset the child’s error/loading state when its canonical URL changes, or await a successful retry and clear the error state.

@shared/api/helpers/getIconFromTokenList.ts:98

• getIconFromTokenLists is still used by asset search, account history, and sign-transaction, but returning candidates[0] never checks whether it loads. A dead first URL therefore remains broken on three of the four surfaces, contrary to the PR’s all-surface resolution behavior. Route these callers through the load probe and cache only the successful result; history should batch/warm its candidates to avoid serial row latency.

    icon: candidates[0],

@shared/api/helpers/iconProbe.ts:75

• The default path calls canLoadIcon directly, and this module has no per-URL verdict cache. Repeated resolutions therefore reload the same URL—including failures—and can consume the full budget again, so the session memoization promised in the PR is absent. Memoize both in-flight and settled results by URL and use that wrapper as the default probe.

    budgetMs = ICON_LOAD_BUDGET_MS,
    probe = canLoadIcon,
  }: { budgetMs?: number; probe?: IconProbe } = {},

@shared/api/internal.ts:1431

• These workers also persist successful icons concurrently, but cacheAssetIcon performs a non-atomic read/modify/write of the entire cache object. Two workers can read the same snapshot and the last setItem then drops the other worker’s entry; the former serial loop avoided this race. Separate probing from persistence and serialize/batch cache updates, or make the background update path atomic.

    const resolvedIcons = await mapWithConcurrency(
      needsLookup,
      ICON_LOOKUP_CONCURRENCY,
      ({ key, code, contractId }) =>

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

@shared/api/internal.ts:1586

• A successful retry cannot display this replacement in the current popup. AssetIcon sets its local hasError to true when the original URL fails, and while that remains true it always selects ImageMissingIcon; updating the parent map does not reset that state. Reset the row's error/loading state when its canonical icon URL changes, or clear it after the retry succeeds.
  extension/src/popup/components/account/AccountAssets/index.tsx:272
• The retry result is applied only to this component's local state; the Redux icon cache still contains the failed truthy URL. useGetBalances prefers a non-empty Redux cache over background storage, and getAssetIcons trusts that value, so navigating away and back in the same popup serves the broken URL again. Dispatch saveIconsForBalances with the replacement (or cleared entry) when the retry finishes.

This issue also appears on line 269 of the same file.

@shared/api/helpers/getIconFromTokenList.ts:98

• getIconFromTokenLists still returns the first listed URL without probing it. Its callers in asset search, sign-transaction, and history render this value directly, so a dead first URL still reproduces the bug on three of the four surfaces that the PR says are covered. Route this helper through firstLoadableIconUrl and cache only the successful URL, or migrate all three callers to the shared probing resolver.

    icon: candidates[0],

@shared/api/helpers/iconProbe.ts:75

• The default probe is canLoadIcon itself, so every call creates a new Image; there is no session-level URL verdict or shared in-flight promise. Repeated rows can therefore retry the same dead URL and repeatedly consume the timeout, contrary to the PR's memoization guarantee. Add a URL-keyed promise cache and use its wrapper as the default probe.

    budgetMs = ICON_LOAD_BUDGET_MS,
    probe = canLoadIcon,
  }: { budgetMs?: number; probe?: IconProbe } = {},

extension/src/popup/components/account/AccountAssets/index.tsx:272

• This new token-list retry is still guarded by the component-wide hasIconFetchRetried flag. Once one row completes a retry, every other asset row's onError returns before using these lists, so a wallet with multiple stale cached icons can recover only the first one. Track retries per canonical asset (or per AssetIcon) instead.

        // Session cache of the user's asset lists, populated by the balances
        // fetch. Empty on a cold popup, in which case the retry degrades to
        // the issuer-TOML lookup it used to do exclusively.
        assetsListsData: cachedTokenLists,

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

@shared/api/internal.ts:1596

• The replacement URL returned here is not displayed by the account row. AssetIcon sets its local hasError to true when the original image fails (AccountAssets/index.tsx:165-170), and while that flag remains true it forces imgSrc to ImageMissingIcon instead of the updated canonical URL (:108-110). Updating the parent map does not remount the row because its key remains the same, so this retry can successfully resolve and cache an alternate icon while the UI continues showing the missing-image state. Reset the row's error/loading state when its canonical URL changes (and add a rendered retry regression test).

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

@shared/api/internal.ts:1441

• lookupDeadline only prevents a new worker item from starting. An item begun just before the deadline still gets the full 1.2-second budget inside resolveIconFromTokenLists, so the documented 4-second ceiling (and smaller injected budgets) can be exceeded by up to another per-asset budget. Pass the remaining account budget into firstLoadableIconUrl, or otherwise bound in-flight work by this deadline.

        Date.now() >= lookupDeadline

@shared/api/helpers/getIconFromTokenList.ts:98

• This is not behavior-preserving for the out-of-scope callers as the PR description claims. The old outer loop overwrote verifiedToken for every matching list (its break only exited the record loop), so it returned the last match; candidates[0] now makes asset search, signing, and history use the first match. Either preserve the old ordering for those callers or update the scope/impact description to acknowledge the behavior change.

    icon: candidates[0],

[piyalbasu]

Closing in favour of #2994, which fixes the reported USDT0 bug in four lines of production code.

This PR grew well past what the bug needed. The reported failure has two causes — icon lookup keeping the last matching list instead of the first, and a "no icon" verdict being persisted to disk — and #2994 addresses exactly those. Everything else here was built around loading each candidate URL to check it renders before using it, and that machinery is what three rounds of review kept finding problems in: budget starvation, premature rejection on slow connections, a session memo that recorded timeouts as permanent failures, cache writes racing under concurrency.

That idea is still worth having. Ordering only fixes the known case; a list shipping a URL browsers reject is caught only by actually loading it. But it needs a per-URL verdict cache and concurrency control to stay off the balances render path, and it deserves its own change rather than riding along with a hotfix.

Two things here are worth salvaging separately:

• The AssetIcon error latch. setHasError(true) is never cleared when a new URL arrives, so once an icon fails the row keeps the broken-image glyph for the life of the component — a successful retry is invisible until the popup is reopened. Small, self-contained, and independent of any probing.
• The probe itself, if we want the stronger guarantee, along with the three surfaces that still render an unchecked URL (asset search, sign-transaction rows, history rows).

Worth noting for whoever picks this up: LOBSTR's list ships an ipfs.io URL for USDT0 that returns 200 to curl and 403 to a browser User-Agent. It is the only such entry across the three default lists today, and it is worth reporting upstream — #2994 works because Soroswap happens to list USDT0 first, not because the bad entry went away.