Skip to content

Conversation

@Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Nov 19, 2025

Description

The underlying sendflow uses selectChainId which should be marked as deprecated.

  • This selected does not return the correct chain ID when you are using "Popular Networks" view as you can have multiple chainIDs selected now!!

This has unearthed a very large scope at this selector underpins many areas in mobile and extension 💀
Ideally all flows and features should move away from this selector and handle multiple selected chainIDs.

Changelog

CHANGELOG entry: fix: correct nft images available during send flow

Related issues

Fixes: #20982

Manual testing steps

  1. Select Popular Networks Tab
  2. Go through NFT send flow - Select NFT from (e.g.) Linea (not ethereum)
  3. Expected: On review page, should see NFT and can select NFT image without crashing app

Screenshots/Recordings

Before

After

https://www.loom.com/share/683675a4c01b47b0a99f6ff5e9dfa30a

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Fetch NFTs by the correct hex chainId via a new selector in the send flow, refine HeroNft placeholder/interaction, and add selector tests with deprecation notes for legacy selectors.

  • State/Selectors:
    • New reducers/collectibles/selectAllCollectiblesByChain: returns collectibles for selected address and given Hex chainId; includes unit tests.
    • Deprecate collectiblesSelector and selectChainId (comments only).
  • Hooks:
    • Update useNft to use selectAllCollectiblesByChain and hexified chainId; derive NFT by tokenId.
  • UI:
    • HeroNft placeholder: remove "Show" text, only show #tokenId when available; block navigation when no NFT.
    • Tests updated and expanded (placeholder interaction, image from collection.imageUrl, assertions).

Written by Cursor Bugbot for commit 0247941. This will update automatically on new commits. Configure here.

@Prithpal-Sooriya Prithpal-Sooriya requested review from a team as code owners November 19, 2025 19:48
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@github-actions
Copy link
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeNetworkAbstractions, SmokeAssets, SmokeConfirmationsRedesigned, SmokeWalletPlatform
  • Risk Level: medium
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

These changes address multi-chain network selection issues and NFT display in confirmations. Here's the breakdown:

Changes Made:

  1. Deprecated selectChainId selector with warning about multi-network selection reliability
  2. Created new chain-specific collectibles selector selectAllCollectiblesByChain
  3. Deprecated old collectiblesSelector that doesn't work with multiple networks
  4. Updated NFT display hook to use new chain-specific selector
  5. Improved hero-nft component safety and null handling in confirmation flows

Risk Assessment - MEDIUM:

  • Network selector deprecation is marked as CRITICAL file but only adds documentation (no logic change)
  • New collectibles selector changes how NFTs are retrieved (chain-specific vs global)
  • Multiple components use the deprecated selectors (30+ usages of selectChainId found)
  • Changes affect confirmation flows where NFTs are displayed
  • The deprecated selectors are still functional, just marked for future replacement

Tag Selection Rationale:

  1. SmokeNetworkAbstractions (HIGH priority) - Critical file changed with network selector deprecation. Multi-chain logic is being updated to handle "popular networks" (multiple selected networks) properly.

  2. SmokeAssets (HIGH priority) - Core changes to collectibles/NFT selectors and display logic. New selector fundamentally changes how NFTs are filtered by chain.

  3. SmokeConfirmationsRedesigned (HIGH priority) - Hero-NFT component in confirmation flows updated with better null handling and uses new selector. Tests show it's part of confirmation UX.

  4. SmokeWalletPlatform (MEDIUM priority) - Network switching and multi-chain state management could be affected by these selector changes since they relate to how the selected chain is determined.

Confidence: 85% - High confidence based on:

  • Clear understanding of changes through code review
  • Deprecations marked explicitly in code comments
  • Changes are focused on multi-chain NFT handling
  • Test coverage shows impact on confirmations and assets
  • Only minor uncertainty about whether network switching edge cases might be affected

View GitHub Actions results

@sonarqubecloud
Copy link

@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Nov 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2025
@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Nov 20, 2025
Merged via the queue into main with commit e5692de Nov 20, 2025
158 of 160 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the fix/20982-update-send-nft-flow-to-use-correct-chainid branch November 20, 2025 19:46
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2025
@metamaskbot metamaskbot added the release-7.61.0 Issue or pull request that will be included in release 7.61.0 label Nov 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-7.61.0 Issue or pull request that will be included in release 7.61.0 size-M team-assets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Error when sending ERC 721 token and clicking on Undefined NFT image - Cannot read property 'backgroundColor' of undefined

5 participants