Skip to content

chore: only fetch NFTs when section is focused and add skeletons - #33645

Merged
juanmigdr merged 12 commits into
mainfrom
chore/only-fetch-nfts-section-when-focused
Jul 29, 2026
Merged

chore: only fetch NFTs when section is focused and add skeletons#33645
juanmigdr merged 12 commits into
mainfrom
chore/only-fetch-nfts-section-when-focused

Conversation

@juanmigdr

@juanmigdr juanmigdr commented Jul 22, 2026

Copy link
Copy Markdown
Member

Description

  • Lazy-load NFT detection

    • detectNfts now fires only when the NFTs section scrolls into view, instead of on every wallet unlock
    • Matches the existing pattern used by other homepage sections (DeFi, TopTraders)
  • Fix: account switch not triggering NFT detection

    • The 5-minute throttle was shared across all accounts — switching accounts silently skipped detection for up to 5 minutes
    • Throttle is now reset on account switch and a viewport re-check is forced so detection fires immediately if the section is already in view
  • Skeleton loading states

    • Homepage NFTs section shows 3 skeletons while detecting for a zero-NFT account, hiding the section only after detection confirms no results
    • NFT full view appends skeleton cells to the grid while loading, filling the current row plus 6 extra slots
  • New reusable NftSkeletonCell component

    • Pulsing placeholder cell using react-native-skeleton-placeholder, shared between the homepage section and the full grid view

Changelog

CHANGELOG entry: only fetch NFTs when section is focused and add skeletons

Related issues

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

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

Screen.Recording.2026-07-22.at.13.33.28.mov

After

Screen.Recording.2026-07-22.at.13.25.06.mov

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

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 when and how NFT detection runs (viewport + throttle) and homepage section visibility/analytics counts; behavior is well-covered by tests but affects assets loading UX on scroll and account switch.

Overview
Moves NFT detection off the homepage root and into NFTsSection, firing detectNfts only when the section is ≥30% visible, with a 5-minute throttle (re-evaluated on visitId) and throttle reset on account switch so a new address can detect immediately.

The homepage always mounts the NFTs section (analytics sectionIndex / totalSectionsLoaded updated accordingly). The section shows three NftSkeletonCell placeholders until detection runs, then unmounts if detection finishes with zero NFTs; the header is non-interactive until NFTs exist.

Adds reusable NftSkeletonCell (react-native-skeleton-placeholder, optional animated={false} for E2E). NftGrid appends skeleton sentinel rows after real items while isNftFetchingProgress is true (finish current 3-column row + six extra slots). Tests cover skeleton counts, viewport gating, throttle, and account-switch behavior.

Reviewed by Cursor Bugbot for commit f5f9d48. Bugbot is set up for automated code reviews on this repo. Configure here.

@juanmigdr
juanmigdr requested a review from a team as a code owner July 22, 2026 11:36
@juanmigdr juanmigdr added the area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. label Jul 22, 2026
@juanmigdr
juanmigdr requested a review from a team as a code owner July 22, 2026 11:36
@metamask-ci

metamask-ci Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Manual testing steps still contain template content (the Gherkin example title or a [...] placeholder). Replace with real steps, or write N/A — <reason>.
  • Pre-merge author checklist has unchecked items (e.g. "I've followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards."). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

Comment thread app/components/Views/Homepage/Sections/NFTs/NFTsSection.tsx Outdated
Comment thread app/components/Views/Homepage/Sections/NFTs/NFTsSection.tsx Outdated
@github-actions github-actions Bot added the risk:medium AI analysis: medium risk label Jul 22, 2026
@juanmigdr
juanmigdr enabled auto-merge July 22, 2026 13:53
Comment thread app/components/Views/Homepage/Sections/NFTs/NFTsSection.tsx
Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	app/components/Views/Homepage/Homepage.test.tsx
#	app/components/Views/Homepage/Homepage.tsx
@MetaMask MetaMask deleted a comment from github-actions Bot Jul 27, 2026

@cursor cursor 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.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 708fab2. Configure here.

Comment thread app/components/Views/Homepage/Sections/NFTs/NFTsSection.tsx
Comment thread app/components/Views/Homepage/Sections/NFTs/NFTsSection.tsx Outdated
@github-actions

Copy link
Copy Markdown
Contributor

⚡ Performance Test Results

ℹ️ Performance test results are currently non-blocking and will not block this PR.

All tests passed · 2 tests · 1 device

📱 Devices tested (1)

Android: Google Pixel 8 Pro (v14.0)

✅ Passed Tests (2)
Test Platform Device Duration Team Recording
Asset View, SRP 1 + SRP 2 + SRP 3 Android Google Pixel 8 Pro (v14.0) 2.87s @assets-dev-team 📹 Watch
Aggregated Balance Loading Time, SRP 1 + SRP 2 + SRP 3 Android Google Pixel 8 Pro (v14.0) 8.21s @assets-dev-team 📹 Watch

Branch: chore/only-fetch-nfts-section-when-focused · Build: Normal · Commit: 87b16e9 · View full run

@github-actions github-actions Bot added risk:high AI analysis: high risk and removed risk:medium AI analysis: medium risk labels Jul 28, 2026
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/components/UI/NftGrid/NftGrid.test.tsx 0/179 0/315 0/320
app/components/Views/Homepage/Homepage.test.tsx 0/179 0/315 0/320

AI-detected flaky patterns

app/components/UI/NftGrid/NftGrid.test.tsx

  • J1 — Missing act() around async state updates (critical)
    • fireEvent.press on a button whose onPress handler is async (or dispatches Redux actions / setState) must be wrapped in act() per J1. This test (and several similar navigation/longPress tests in the file) does not do so. The addition of skeleton sentinels and non-EVM account tests increased the surface for state updates during render/press. Combined with useFakeTimers() this raises risk of intermittent failures. No historical flakiness recorded.
    • Suggested fix in app/components/UI/NftGrid/NftGrid.test.tsx:
      -    const addButton = getByTestId('import-token-button');
      -    fireEvent.press(addButton);
      -    expect(mockNavigate).toHaveBeenCalledWith('AddAsset', {
      -      assetType: 'collectible',
      -    });
      +    const addButton = getByTestId('import-token-button');
      +    await act(async () => {
      +      await fireEvent.press(addButton);
      +    });
      +    expect(mockNavigate).toHaveBeenCalledWith('AddAsset', {
      +      assetType: 'collectible',
      +    });

app/components/Views/Homepage/Homepage.test.tsx

  • J1 — Missing act() on async state updates (critical)
    • This test calls an async refresh function from the ref (which can trigger internal state updates or effects in the Homepage component, similar to the onRefresh example in the skill). The call is not wrapped in act() despite the test being marked async and act being imported (but unused elsewhere). This matches J1 exactly and can cause intermittent 'not wrapped in act()' warnings or test flakiness under CI load. Historical data showed no prior flakiness for this file, so this is a proactive detection. No other J2-J10 patterns matched (e.g. beforeEach uses clearAllMocks, no waitFor/timers/sleep/spyOn/mutable lets without reset, mocks appear complete).
    • Suggested fix in app/components/Views/Homepage/Homepage.test.tsx:
      -    const result = ref.current?.refresh();
      -
      -    await expect(result).resolves.toBeUndefined();
      +    const result = ref.current?.refresh();
      +    await act(() => result);
      +    await expect(result).resolves.toBeUndefined();

This check is informational only and does not block merging.

bergarces
bergarces previously approved these changes Jul 28, 2026
@github-actions github-actions Bot added risk:medium AI analysis: medium risk and removed risk:high AI analysis: high risk labels Jul 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

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

E2E Test Selection:
The changes affect the Homepage/Wallet view's NFTs section in significant ways:

  1. NFTs section is now always rendered (previously conditional on hasNfts). This changes the Homepage layout for all users — the NFTs section now always appears with skeleton cells until detection completes and finds nothing.

  2. Section indices shifted: Since NFTs is now always present, all section indices in the homepage are affected (e.g., NFTs is now always index 4 when all flags are on, instead of being absent). This could affect analytics/tracking but not E2E flows directly.

  3. NFT detection moved to NFTsSection: Detection is now viewport-gated (only fires when section scrolls into view), which is a behavioral change from the previous always-on throttled detection in Homepage.

  4. SmokeWalletPlatform is the most relevant tag because:

    • It tests homepage sections (network filter, trending, etc.)
    • The homepage-sections-network-filter.spec.ts tests homepage section rendering
    • The NFTs section being always present could affect layout/scroll behavior in these tests
    • No dedicated NFT E2E smoke tests exist in the smoke suite

No other tags are directly impacted — the changes are isolated to the NFT grid/section rendering within the homepage, with no impact on confirmations, swaps, accounts, networks, or other flows.

Performance tests are not warranted — the skeleton loading is a UI enhancement that doesn't meaningfully change app performance characteristics. The NFT detection logic change (viewport-gated vs. always-on) could theoretically reduce unnecessary background work, but this is not a performance regression risk.

Performance Test Selection:
The changes are UI-level improvements to NFT skeleton loading and detection gating. No performance-sensitive paths (app launch, login, asset loading, swaps, etc.) are meaningfully impacted. The viewport-gated detection could reduce unnecessary background work but is not a performance regression risk that warrants performance test runs.

View GitHub Actions results

@vinnyhoward vinnyhoward 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.

Nice work

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.96491% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.79%. Comparing base (8206640) to head (f5f9d48).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nents/Views/Homepage/Sections/NFTs/NFTsSection.tsx 82.50% 6 Missing and 1 partial ⚠️
app/components/UI/NftGrid/NftGrid.tsx 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #33645      +/-   ##
==========================================
- Coverage   84.79%   84.79%   -0.01%     
==========================================
  Files        6247     6248       +1     
  Lines      168342   168383      +41     
  Branches    41162    41171       +9     
==========================================
+ Hits       142741   142774      +33     
- Misses      15853    15860       +7     
- Partials     9748     9749       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud

Copy link
Copy Markdown

@juanmigdr
juanmigdr added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit c1faefc Jul 29, 2026
237 of 239 checks passed
@juanmigdr
juanmigdr deleted the chore/only-fetch-nfts-section-when-focused branch July 29, 2026 07:17
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 29, 2026
@metamask-ci metamask-ci Bot added the release-8.6.0 Issue or pull request that will be included in release 8.6.0 label Jul 29, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. release-8.6.0 Issue or pull request that will be included in release 8.6.0 risk:medium AI analysis: medium risk size-L team-assets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants