Skip to content

show only approved asset icons - #416

Merged
bordalix merged 1 commit into
masterfrom
asset-icon
Mar 12, 2026
Merged

bordalix merged 1 commit into
masterfrom
asset-icon

Conversation

@louisinger

@louisinger louisinger commented Mar 12, 2026 •

Copy link
Copy Markdown
Contributor

@bordalix @pietro909 please review

Summary by CodeRabbit

Release Notes

  • Refactor
    • Improved internal asset caching mechanism to streamline the flow between cache retrieval and updates, enhancing code consistency and efficiency across asset management features.

@coderabbitai

coderabbitai Bot commented Mar 12, 2026 •

Copy link
Copy Markdown
Contributor

Walkthrough

The setCacheEntry function in the wallet context provider is updated to return the cached entry (CachedAssetDetails) instead of void. All call sites across multiple asset screens are updated to use the returned value directly instead of performing separate cache lookups.

Changes

Cohort / File(s) Summary
Wallet Provider Core
src/providers/wallet.tsx
Updated setCacheEntry method signature to return CachedAssetDetails instead of void. Implementation now returns the created cache entry. Updated WalletContextProps interface and default context value accordingly.
Asset Detail Screens
src/screens/Apps/Assets/Detail.tsx, src/screens/Apps/Assets/Import.tsx, src/screens/Apps/Assets/Mint.tsx, src/screens/Wallet/Receive/Success.tsx, src/screens/Wallet/Send/Form.tsx
Updated all call sites to use the return value of setCacheEntry() directly for assignment instead of calling the function and then performing a separate cache lookup via assetMetadataCache.get().
Test Mocks
src/test/screens/mocks.ts
Updated mock setCacheEntry in mockWalletContextValue to return ({ cachedAt: 0 }) instead of {}.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • bordalix
🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'show only approved asset icons' does not match the actual changes, which involve refactoring the setCacheEntry API return type and updating multiple call sites across the codebase. Update the PR title to reflect the main technical change, such as 'Make setCacheEntry return the cached entry instead of void' or 'Refactor setCacheEntry to return CachedAssetDetails'.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch asset-icon
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can generate walkthrough in a markdown collapsible section to save space.

Enable the reviews.collapse_walkthrough setting to generate walkthrough in a markdown collapsible section.

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying wallet-bitcoin with  Cloudflare Pages  Cloudflare Pages

Latest commit: 52677ad
Status: ✅  Deploy successful!
Preview URL: https://c0fb359d.wallet-bitcoin.pages.dev
Branch Preview URL: https://asset-icon.wallet-bitcoin.pages.dev

View logs

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying wallet-mutinynet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 52677ad
Status: ✅  Deploy successful!
Preview URL: https://7255cf88.arkade-wallet.pages.dev
Branch Preview URL: https://asset-icon.arkade-wallet.pages.dev

View logs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/test/screens/mocks.ts (1)

146-146: Mock returns incomplete CachedAssetDetails structure.

The mock returns only { cachedAt: 0 }, but callers expect the full CachedAssetDetails which extends AssetDetails (including metadata, supply, etc.). Code accessing meta?.metadata?.name or similar fields will receive undefined from this mock.

Consider returning a more complete mock object to improve test fidelity:

Proposed improvement
-  setCacheEntry: () => ({ cachedAt: 0 }) as any,
+  setCacheEntry: (_assetId: string, details: any) => ({ ...details, cachedAt: Date.now(), hasIcon: false }),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/test/screens/mocks.ts` at line 146, The setCacheEntry mock currently
returns only { cachedAt: 0 } which doesn't satisfy callers expecting a full
CachedAssetDetails (which extends AssetDetails); update the mock in
src/test/screens/mocks.ts (the setCacheEntry function/object) to return a
complete CachedAssetDetails-shaped object including metadata (with
metadata.name, metadata.image, etc.), supply, id and any other required
AssetDetails fields so tests that access meta?.metadata?.name or supply will get
realistic values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/test/screens/mocks.ts`:
- Line 146: The setCacheEntry mock currently returns only { cachedAt: 0 } which
doesn't satisfy callers expecting a full CachedAssetDetails (which extends
AssetDetails); update the mock in src/test/screens/mocks.ts (the setCacheEntry
function/object) to return a complete CachedAssetDetails-shaped object including
metadata (with metadata.name, metadata.image, etc.), supply, id and any other
required AssetDetails fields so tests that access meta?.metadata?.name or supply
will get realistic values.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 67fe7c28-b59c-4854-a920-1daa00d41602

📥 Commits

Reviewing files that changed from the base of the PR and between fda01a1 and 52677ad.

📒 Files selected for processing (7)
  • src/providers/wallet.tsx
  • src/screens/Apps/Assets/Detail.tsx
  • src/screens/Apps/Assets/Import.tsx
  • src/screens/Apps/Assets/Mint.tsx
  • src/screens/Wallet/Receive/Success.tsx
  • src/screens/Wallet/Send/Form.tsx
  • src/test/screens/mocks.ts

@bordalix
bordalix merged commit ce1968a into master Mar 12, 2026
5 checks passed
Kukks added a commit that referenced this pull request Mar 13, 2026
Merge master into wt-navbar-redesign-20260226, incorporating:
- nak version fix (#420)
- password navigation fix (#419)
- chain swaps restore (#415)
- asset avatar UI fixes (#414)
- approved asset icons (#416)
@bordalix
bordalix deleted the asset-icon branch May 18, 2026 16:17
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.

2 participants