Skip to content

fix: refresh all enabled networks when "All popular networks" is selected#27839

Open
juanmigdr wants to merge 3 commits intomainfrom
fix/nft-refresh-should-trigger-for-all-chains
Open

fix: refresh all enabled networks when "All popular networks" is selected#27839
juanmigdr wants to merge 3 commits intomainfrom
fix/nft-refresh-should-trigger-for-all-chains

Conversation

@juanmigdr
Copy link
Copy Markdown
Member

@juanmigdr juanmigdr commented Mar 24, 2026

Description

WAIT FOR THIS TO BE MERGED

useNftRefresh was building the list of networks to check ownership status from tokenNetworkFilter in PreferencesController. That value is never updated when the user selects "All popular networks" — that code path only updates NetworkEnablementController. So on pull-to-refresh, detectNfts correctly ran against all enabled networks (via chainIdsToDetectNftsFor), but checkAndUpdateAllNftsOwnershipStatus only ran against the last single network the user had selected before switching to "All popular networks".

The fix replaces tokenNetworkFilter with chainIdsToDetectNftsFor sourced from useNftDetection, which reads from NetworkEnablementController — the actual source of truth. As a follow-on, detectNfts and chainIdsToDetectNftsFor are now passed into useNftRefresh from the caller, eliminating the previously duplicate useNftDetection() hook instances in NftGrid and NFTsSection (two independent abort controllers was an additional latent bug).

Changelog

CHANGELOG entry: fixed NFT pull-to-refresh not checking ownership status across all enabled networks when "All popular networks" is selected

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-2976

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

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

Medium Risk
Changes NFT pull-to-refresh to derive enabled networks from useNftDetection and alters useNftRefresh’s API, which could affect refresh behavior and network RPC usage across multiple screens.

Overview
Fixes NFT pull-to-refresh ownership checks to run across all enabled networks by having useNftRefresh take chainIdsToDetectNftsFor (from useNftDetection) instead of relying on the stale PreferencesController token network filter.

Refactors NftGrid and NFTsSection to pass detectNfts/chainIdsToDetectNftsFor into useNftRefresh, avoiding duplicate useNftDetection() instances, and updates useNftRefresh tests to match the new options-based signature.

Also trims delegated contract-read actions in nft-controller-messenger by removing AssetsContractController:getERC721OwnerOf and AssetsContractController:getERC1155BalanceOf.

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

@github-actions github-actions bot added size-M risk-low Low testing needed · Low bug introduction risk labels Mar 24, 2026
@juanmigdr juanmigdr enabled auto-merge March 24, 2026 10:17
bergarces
bergarces previously approved these changes Mar 24, 2026
@juanmigdr juanmigdr added this pull request to the merge queue Mar 24, 2026
@juanmigdr juanmigdr removed this pull request from the merge queue due to a manual request Mar 24, 2026
@juanmigdr juanmigdr requested a review from a team as a code owner March 25, 2026 13:24
@github-actions github-actions bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-low Low testing needed · Low bug introduction risk labels Mar 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeConfirmations, SmokeWalletPlatform
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 72%
click to see 🤖 AI reasoning details

E2E Test Selection:
The PR makes two categories of changes:

  1. NFT Controller Messenger change (CRITICAL file): Removes AssetsContractController:getERC721OwnerOf and AssetsContractController:getERC1155BalanceOf from the NftController's allowed messenger actions. This is a meaningful architectural change - the NftController can no longer call these contract methods via the messenger. The getERC1155BalanceOf is still used in useNfts.ts for send confirmations (via assetsContractController directly), so the confirmation flow for ERC-1155 tokens should still work. However, removing getERC721OwnerOf from the messenger could affect ownership verification flows. SmokeConfirmations covers ERC-721/ERC-1155 contract interactions, token approvals (setApprovalForAll), and contract deployments.

  2. UI Refactoring (NftGrid, NFTsSection, useNftRefresh): Dependency injection pattern change - useNftRefresh now accepts detectNfts and chainIdsToDetectNftsFor as props instead of internally calling useNftDetection. This is a clean refactor with no functional behavior change. The NFT grid and NFTs section in the wallet homepage are affected. SmokeWalletPlatform covers wallet platform features including the NFT display sections.

No NFT-specific smoke tests exist in the available tags. The regression NFT tests (nft-detection-modal.spec.ts, nft-details.spec.ts) are marked as describe.skip, so they won't run. The most relevant coverage comes from SmokeConfirmations (ERC-721/ERC-1155 contract interactions) and SmokeWalletPlatform (wallet homepage NFT section display).

Performance Test Selection:
The changes are a refactoring of dependency injection patterns in NFT UI components and a messenger action removal. There are no performance-sensitive changes such as list rendering optimizations, data loading changes, or app initialization modifications that would warrant performance testing.

View GitHub Actions results

@github-actions
Copy link
Copy Markdown
Contributor

E2E Fixture Validation — Schema is up to date
16 value mismatches detected (expected — fixture represents an existing user).
View details

@sonarqubecloud
Copy link
Copy Markdown

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

Labels

risk-medium Moderate testing recommended · Possible bug introduction risk size-M team-assets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants