Skip to content

refactor(confirmations): Migrate ButtonLink to MMDS - #34370

Open
brianacnguyen wants to merge 14 commits into
mainfrom
refactor/confirmations-button-link
Open

refactor(confirmations): Migrate ButtonLink to MMDS#34370
brianacnguyen wants to merge 14 commits into
mainfrom
refactor/confirmations-button-link

Conversation

@brianacnguyen

@brianacnguyen brianacnguyen commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Confirmations still used deprecated ButtonLink and ad-hoc pressable Text for link-style actions. Those patterns are being replaced by MMDS: TextButton for inline links in copy, and Button with ButtonVariant.Tertiary for standalone background-less CTAs.

What changed:

  • Replaced ButtonLink / pressable-text links with TextButton in pending-transaction and account-upgrade alerts, staking footer legal links, percentage-row terms link, and Blockaid “Report an issue”
  • Replaced standalone link CTAs with Tertiary Button in confirm-alert-modal (“Review all alerts”) and advanced-details nonce control
  • Removed unused BlockaidBannerLink wrapper
  • Added testIDs for the new MMDS controls and updated unit tests to query by testID instead of copy

Breaking change: none for external consumers. Confirmations-internal only; BlockaidBannerLink deleted (only used by Blockaid alert content).

Changelog

CHANGELOG entry: null

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/DSYS-994

Manual testing steps

Feature: Confirmations ButtonLink migration to MMDS

  Scenario: user opens staking footer legal links
    Given a staking deposit confirmation is open
    When the user taps "Terms of Use"
    Then the Terms of Use URL opens
    When the user taps "Risk disclosure"
    Then the staking risk disclosure URL opens

  Scenario: user reviews alerts from the confirm alert modal
    Given a confirmation with field danger alerts is open
    When the confirm alert modal is shown
    And the user taps "Review all alerts"
    Then the alert details modal opens

  Scenario: user edits nonce from advanced details
    Given a transaction confirmation with smart transactions disabled
    When the user expands Advanced details
    And taps the nonce value
    Then the custom nonce modal opens

  Scenario: user opens Blockaid report link
    Given a confirmation flagged by Blockaid
    When the user expands "See details" in the Blockaid alert
    And taps "Report an issue"
    Then the report URL opens

  Scenario: unit coverage for migrated controls
    Given the branch is checked out
    When running:
      yarn jest app/components/Views/confirmations/hooks/alerts/usePendingTransactionAlert.test.tsx app/components/Views/confirmations/hooks/alerts/useAccountTypeUpgrade.test.tsx app/components/Views/confirmations/components/rows/percentage-row/percentage-row.test.tsx app/components/Views/confirmations/components/footer/footer.test.tsx app/components/Views/confirmations/components/blockaid-alert-content/blockaid-alert-content.test.tsx app/components/Views/confirmations/components/modals/confirm-alert-modal/confirm-alert-modal.test.tsx app/components/Views/confirmations/components/rows/transactions/advanced-details-row/advanced-details-row.test.tsx --no-coverage
    Then all suites pass

Screenshots/Recordings

N/A — design-system component swap for the same link/CTA affordances; no intended product flow change. Optional visual QA screenshots of staking footer links, confirm-alert “Review all alerts”, and advanced-details nonce can be added if reviewers want before/after.

Before

After

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

Low Risk
Mostly design-system swaps and test harness updates in confirmations UI; the bridge hook change only affects loading-state display of trending tokens, not transaction or auth flows.

Overview
Replaces deprecated ButtonLink and underlined pressable Text link patterns across confirmations with MetaMask Design System controls: TextButton for inline legal/learn-more/report links, and a Tertiary Button for the confirm-alert Review all alerts CTA. BlockaidBannerLink is removed in favor of inlined TextButton + Linking.openURL.

Adds centralized testID constants (footer staking links, confirm-alert modal actions, Blockaid report, percentage-row terms, pending-tx and account-upgrade alerts) and updates unit tests to assert via testID and i18n strings where appropriate.

Separately, usePostTradeTrendingTokens now returns an empty token list while Ethereum fallback backfill is loading (when destination results need topping up), so the post-trade trending list does not flash a partial set before the merged list appears.

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

…Tertiary Button

Migrate deprecated ButtonLink and text-as-link patterns to TextButton for
inline links and Button Tertiary for standalone CTAs, with testIDs for stable queries.

Co-authored-by: Cursor <cursoragent@cursor.com>
@brianacnguyen brianacnguyen self-assigned this Aug 5, 2026
@brianacnguyen
brianacnguyen requested a review from a team as a code owner August 5, 2026 19:52
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
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.

@metamask-ci metamask-ci Bot added the team-design-system All issues relating to design system in Mobile label Aug 5, 2026
@github-actions github-actions Bot added the size-M label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 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/Views/confirmations/components/footer/footer.test.tsx 0/84 0/241 0/361
app/components/Views/confirmations/components/rows/percentage-row/percentage-row.test.tsx 0/84 0/241 0/361
app/components/Views/confirmations/hooks/alerts/useAccountTypeUpgrade.test.tsx 0/84 0/241 0/361
app/components/Views/confirmations/hooks/alerts/usePendingTransactionAlert.test.tsx 0/84 0/241 0/361

AI-detected flaky patterns

app/components/Views/confirmations/components/footer/footer.test.tsx

  • J10 — jest.spyOn without jest.restoreAllMocks() in afterEach (medium)
    • This file contains multiple jest.spyOn(QRHardwareHook, 'useQRHardwareContext') calls (e.g. in the tests for QR signing and camera permission at approx lines 140 and 150) that override mockReturnValue. The beforeEach only does jest.clearAllMocks() which does not restore original implementations. No afterEach with jest.restoreAllMocks() exists anywhere in the file (or nested describes). This exactly matches J10 from the skill (spy leaks across tests, order-dependent behavior). History JSON shows 0 failures so historicalHintUsed=false; finding is based solely on code pattern. Adding restoreAllMocks prevents leakage without affecting the per-test mock setups in beforeEach.
    • Suggested fix in app/components/Views/confirmations/components/footer/footer.test.tsx:
      -  beforeEach(() => {
      -    jest.clearAllMocks();
      -
      -    mockUseConfirmationContext.mockReturnValue({
      -      mmPayRequestInProgressNavHandler: { current: false },
      -      headlessBuyError: undefined,
      -      isFooterVisible: true,
      -      isConfirmationSubmitting: false,
      -      isConfirmationSubmittingRef: { current: false },
      -      isHeadlessBuyInProgress: false,
      -      isTransactionDataUpdating: false,
      -      isTransactionValueUpdating: false,
      -      setHeadlessBuyError: jest.fn(),
      -      setIsFooterVisible: jest.fn(),
      -      setIsConfirmationSubmitting: jest.fn(),
      -      setIsHeadlessBuyInProgress: jest.fn(),
      -      setIsTransactionDataUpdating: jest.fn(),
      -      setIsTransactionValueUpdating: jest.fn(),
      -    });
      -
      -    (useAlerts as jest.Mock).mockReturnValue({
      -      fieldAlerts: [],
      -      hasDangerAlerts: false,
      -      setAlertConfirmed: jest.fn(),
      -    });
      -
      -    (useAlertsConfirmed as jest.Mock).mockReturnValue({
      -      hasUnconfirmedDangerAlerts: false,
      -    });
      -
      -    useIsTransactionPayLoadingMock.mockReturnValue(false);
      -    useIsTransactionPayAmountStaleMock.mockReturnValue(false);
      -    useIsGaslessLoadingMock.mockReturnValue({ isGaslessLoading: false });
      -  });
      +  beforeEach(() => {
      +    jest.clearAllMocks();
      +
      +    mockUseConfirmationContext.mockReturnValue({
      +      mmPayRequestInProgressNavHandler: { current: false },
      +      headlessBuyError: undefined,
      +      isFooterVisible: true,
      +      isConfirmationSubmitting: false,
      +      isConfirmationSubmittingRef: { current: false },
      +      isHeadlessBuyInProgress: false,
      +      isTransactionDataUpdating: false,
      +      isTransactionValueUpdating: false,
      +      setHeadlessBuyError: jest.fn(),
      +      setIsFooterVisible: jest.fn(),
      +      setIsConfirmationSubmitting: jest.fn(),
      +      setIsHeadlessBuyInProgress: jest.fn(),
      +      setIsTransactionDataUpdating: jest.fn(),
      +      setIsTransactionValueUpdating: jest.fn(),
      +    });
      +
      +    (useAlerts as jest.Mock).mockReturnValue({
      +      fieldAlerts: [],
      +      hasDangerAlerts: false,
      +      setAlertConfirmed: jest.fn(),
      +    });
      +
      +    (useAlertsConfirmed as jest.Mock).mockReturnValue({
      +      hasUnconfirmedDangerAlerts: false,
      +    });
      +
      +    useIsTransactionPayLoadingMock.mockReturnValue(false);
      +    useIsTransactionPayAmountStaleMock.mockReturnValue(false);
      +    useIsGaslessLoadingMock.mockReturnValue({ isGaslessLoading: false });
      +  });
      +
      +  afterEach(() => {
      +    jest.restoreAllMocks();
      +  });

app/components/Views/confirmations/components/rows/percentage-row/percentage-row.test.tsx

  • J10 — jest.spyOn() without restoreAllMocks()/mockRestore() afterward (medium)
    • The test 'tracks event and opens URL when terms apply tooltip link is pressed' (around line 50) uses jest.spyOn(Linking, 'openURL') without restoring via afterEach. Even with resetAllMocks in beforeEach, explicit restoreAllMocks prevents spy leakage to other tests. Matches J10 pattern directly from code. No historical flakiness.
    • Suggested fix in app/components/Views/confirmations/components/rows/percentage-row/percentage-row.test.tsx:
      -  beforeEach(() => {
      -    jest.resetAllMocks();
      -
      -    mockBuild.mockReturnValue({ name: 'mock-built-event' });
      -    mockAddProperties.mockImplementation(() => ({ build: mockBuild }));
      -    mockCreateEventBuilder.mockImplementation(() => ({
      -      addProperties: mockAddProperties,
      -    }));
      -
      -    useIsTransactionPayLoadingMock.mockReturnValue(false);
      -    useTransactionMetadataRequestMock.mockReturnValue({
      -      type: TransactionType.musdConversion,
      -    } as ReturnType<typeof useTransactionMetadataRequest>);
      -  });
      -
      +  beforeEach(() => {
      +    jest.resetAllMocks();
      +
      +    mockBuild.mockReturnValue({ name: 'mock-built-event' });
      +    mockAddProperties.mockImplementation(() => ({ build: mockBuild }));
      +    mockCreateEventBuilder.mockImplementation(() => ({
      +      addProperties: mockAddProperties,
      +    }));
      +
      +    useIsTransactionPayLoadingMock.mockReturnValue(false);
      +    useTransactionMetadataRequestMock.mockReturnValue({
      +      type: TransactionType.musdConversion,
      +    } as ReturnType<typeof useTransactionMetadataRequest>);
      +  });
      +
      +  afterEach(() => {
      +    jest.restoreAllMocks();
      +  });
      +

app/components/Views/confirmations/hooks/alerts/useAccountTypeUpgrade.test.tsx

  • J3 — Missing jest.clearAllMocks()/resetAllMocks() between tests (high)
    • No beforeEach or afterEach in the describe block, so no jest.clearAllMocks() or resetAllMocks() between the two tests. The second test creates a spyOn that can bleed. This matches J3 exactly and can lead to order-dependent test results. Historical signal is false but pattern is present. Suggested fix also addresses the spy for completeness.
    • Suggested fix in app/components/Views/confirmations/hooks/alerts/useAccountTypeUpgrade.test.tsx:
      -describe('useAccountTypeUpgrade', () => {
      -  it('returns alert for upgrade+batched account request', () => {
      -    const { result } = renderHookWithProvider(() => useAccountTypeUpgrade(), {
      -      state: getAppStateForConfirmation(upgradeAccountConfirmation),
      -    });
      -    const currentAlert = result.current[0];
      -    delete currentAlert.content;
      -
      -    expect(currentAlert).toEqual({
      -      field: 'accountTypeUpgrade',
      -      key: 'accountTypeUpgrade',
      -      severity: 'info',
      -      title: 'Updating your account',
      -    });
      -  });
      -
      -  it('opens smart accounts URL when learn more button is pressed', () => {
      -    const openUrlSpy = jest
      -      .spyOn(Linking, 'openURL')
      -      .mockResolvedValueOnce(undefined);
      -
      -    const { result } = renderHookWithProvider(() => useAccountTypeUpgrade(), {
      -      state: getAppStateForConfirmation(upgradeAccountConfirmation),
      -    });
      -
      -    const { getByTestId } = renderWithProvider(
      -      <>{result.current[0].content}</>,
      -    );
      -
      -    fireEvent.press(
      -      getByTestId(AccountTypeUpgradeAlertTestIds.LEARN_MORE_BUTTON),
      -    );
      -
      -    expect(openUrlSpy).toHaveBeenCalledWith(AppConstants.URLS.SMART_ACCOUNTS);
      -  });
      -});
      -
      +describe('useAccountTypeUpgrade', () => {
      +  beforeEach(() => {
      +    jest.clearAllMocks();
      +  });
      +
      +  afterEach(() => {
      +    jest.restoreAllMocks();
      +  });
      +
      +  it('returns alert for upgrade+batched account request', () => {
      +    const { result } = renderHookWithProvider(() => useAccountTypeUpgrade(), {
      +      state: getAppStateForConfirmation(upgradeAccountConfirmation),
      +    });
      +    const currentAlert = result.current[0];
      +    delete currentAlert.content;
      +
      +    expect(currentAlert).toEqual({
      +      field: 'accountTypeUpgrade',
      +      key: 'accountTypeUpgrade',
      +      severity: 'info',
      +      title: 'Updating your account',
      +    });
      +  });
      +
      +  it('opens smart accounts URL when learn more button is pressed', () => {
      +    const openUrlSpy = jest
      +      .spyOn(Linking, 'openURL')
      +      .mockResolvedValueOnce(undefined);
      +
      +    const { result } = renderHookWithProvider(() => useAccountTypeUpgrade(), {
      +      state: getAppStateForConfirmation(upgradeAccountConfirmation),
      +    });
      +
      +    const { getByTestId } = renderWithProvider(
      +      <>{result.current[0].content}</>,
      +    );
      +
      +    fireEvent.press(
      +      getByTestId(AccountTypeUpgradeAlertTestIds.LEARN_MORE_BUTTON),
      +    );
      +
      +    expect(openUrlSpy).toHaveBeenCalledWith(AppConstants.URLS.SMART_ACCOUNTS);
      +  });
      +});
      +
  • J10 — jest.spyOn() without restoreAllMocks()/mockRestore() afterward (medium)
    • The second test directly uses jest.spyOn without any restoreAllMocks in an afterEach (and the file lacks afterEach entirely). This can cause the mocked Linking.openURL to persist beyond the test, leading to intermittent failures in CI when tests run in varying orders. Matches J10 pattern with concrete line. Combined with the J3 finding for the same file.
    • Suggested fix in app/components/Views/confirmations/hooks/alerts/useAccountTypeUpgrade.test.tsx:
      -    const openUrlSpy = jest
      -      .spyOn(Linking, 'openURL')
      -      .mockResolvedValueOnce(undefined);
      -
      +    const openUrlSpy = jest
      +      .spyOn(Linking, 'openURL')
      +      .mockResolvedValueOnce(undefined);
      +    // Note: restore happens in afterEach added in the J3 fix for this file
      +

app/components/Views/confirmations/hooks/alerts/usePendingTransactionAlert.test.tsx

  • J10 — jest.spyOn() without restoreAllMocks()/mockRestore() afterward (medium)
    • This file uses jest.spyOn(Linking, 'openURL') in the 'opens speed up URL when learn more button is pressed' test (around line 37) but has no afterEach to call jest.restoreAllMocks(). Spies can leak across tests, causing non-deterministic behavior in CI depending on execution order. The beforeEach only clears but does not restore originals. No historical flakiness signal but matches J10 exactly.
    • Suggested fix in app/components/Views/confirmations/hooks/alerts/usePendingTransactionAlert.test.tsx:
      -  beforeEach(() => {
      -    jest.clearAllMocks();
      -    mockUseTransactionMetadataRequest.mockReturnValue(
      -      MOCK_SUBMITTED_TRANSACTION_META,
      -    );
      -  });
      -
      +  beforeEach(() => {
      +    jest.clearAllMocks();
      +    mockUseTransactionMetadataRequest.mockReturnValue(
      +      MOCK_SUBMITTED_TRANSACTION_META,
      +    );
      +  });
      +
      +  afterEach(() => {
      +    jest.restoreAllMocks();
      +  });
      +

This check is informational only and does not block merging.

@github-actions github-actions Bot added the risk:medium AI analysis: medium risk label Aug 5, 2026
Comment thread app/components/Views/confirmations/components/footer/footer.tsx
…ce assertions

Co-authored-by: Cursor <cursoragent@cursor.com>
@matthewwalsh0
matthewwalsh0 requested a review from jpuri August 6, 2026 05:57
jpuri
jpuri previously approved these changes Aug 6, 2026
@codecov-commenter

codecov-commenter commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.08%. Comparing base (3a56af7) to head (e998a8e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #34370      +/-   ##
==========================================
- Coverage   85.08%   85.08%   -0.01%     
==========================================
  Files        6332     6335       +3     
  Lines      172506   172500       -6     
  Branches    42652    42650       -2     
==========================================
- Hits       146784   146775       -9     
+ Misses      15691    15688       -3     
- Partials    10031    10037       +6     

☔ 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.

brianacnguyen and others added 4 commits August 10, 2026 07:42
Tertiary Button defaults to medium height and oversized the compact InfoRow nonce value.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot removed the size-L label Aug 10, 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 1 potential issue.

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 76090b8. Configure here.

@brianacnguyen
brianacnguyen requested a review from a team as a code owner August 10, 2026 23:52
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

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

E2E Test Selection:
The PR makes targeted UI component refactoring changes across the confirmations system and bridge post-trade flow:

  1. SmokeConfirmations (primary): Multiple confirmation UI components were modified:

    • blockaid-alert-content.tsx: Replaced BlockaidBannerLink (now deleted) with TextButton for the "report an issue" link in Blockaid security alerts. This directly affects the Blockaid security alert integration tested in SmokeConfirmations.
    • confirm-alert-modal.tsx: Replaced ButtonLink with Button (tertiary variant) for the "review alerts" button. testIDs were updated but string values remain the same, so existing E2E selectors should still work. However, the component change from ButtonLink to Button could affect rendering/behavior.
    • useAccountTypeUpgrade.tsx: Replaced Text with TextButton for EIP-7702 account upgrade alert "learn more" link — affects batch transaction/smart account upgrade flows.
    • usePendingTransactionAlert.tsx: Replaced ButtonLink with TextButton for pending transaction alert — affects transaction confirmation alerts.
    • ConfirmationView.testIds.ts: Added new testIDs for staking footer buttons and confirm alert modal buttons.
    • footer.tsx: Replaced Text with TextButton for staking terms/risk disclosure links.
    • percentage-row.tsx: Replaced Text with TextButton for terms link in earn/staking rows.
  2. SmokeStake (secondary): The footer.tsx and percentage-row.tsx changes directly affect the staking confirmation footer (terms of use, risk disclosure links) and the percentage row with bonus terms. These are staking-specific UI elements. Per tag description, SmokeStake flows hit confirmations, so SmokeConfirmations is already selected.

  3. SmokeSwap (secondary): The usePostTradeTrendingTokens.ts change in the Bridge PostTradeBottomSheet fixes a logic issue where the token list would flash sparse results before the Ethereum backfill completes. This affects the post-swap/bridge experience. Per tag description, SmokeSwap also requires SmokeConfirmations (already selected).

Risk factors:

  • The BlockaidBannerLink component was deleted entirely — any remaining references would cause build failures, but the diff shows it was only used in blockaid-alert-content.tsx which was updated.
  • testID string values in confirm-alert-modal.tsx remain the same (e.g., 'confirm-alert-checkbox', 'confirm-alert-confirm-button'), so existing E2E selectors won't break.
  • The component type changes (Text→TextButton, ButtonLink→Button) could affect styling/behavior in edge cases.
  • No changes to core Engine, controllers, navigation, or shared infrastructure — impact is contained to confirmations and bridge post-trade.

Performance Test Selection:
No performance-sensitive code paths were changed. The modifications are UI component refactoring (Text→TextButton, ButtonLink→Button/TextButton) within confirmation screens and a minor logic fix in the bridge post-trade token list. These changes do not affect app launch, login, onboarding, asset loading, swap execution timing, or any other performance-measured flows.

View GitHub Actions results

@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 AI analysis: medium risk size-M team-design-system All issues relating to design system in Mobile

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants