Skip to content

refactor(perps): migrate Close All Positions sheet to MMDS#33365

Open
brianacnguyen wants to merge 5 commits into
mainfrom
refactor/perps-home-closeallpositions
Open

refactor(perps): migrate Close All Positions sheet to MMDS#33365
brianacnguyen wants to merge 5 commits into
mainfrom
refactor/perps-home-closeallpositions

Conversation

@brianacnguyen

@brianacnguyen brianacnguyen commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Description

Aligns the Perps Close All Positions bottom sheet with the MetaMask Design System (MMDS) so it matches other redesigned Perps surfaces, instead of the older component-library sheet/footer/text primitives.

What changed:

  • Migrated PerpsCloseAllPositionsView to MMDS BottomSheet, BottomSheetHeader, BottomSheetFooter, Box, and Text (including goBack / onClose wiring and primary/secondary footer button props with isDanger on Close all).
  • Updated PerpsCloseSummary fees, you-receive, and estimated-points rows to MMDS KeyValueRow + SectionDivider (margin/P&L row kept custom for nested includes P&L).
  • Pointed PerpsHomeView’s close-all sheet ref at MMDS BottomSheetRef while keeping cancel-all on the component-library sheet ref.
  • Updated unit tests/mocks for the MMDS bottom sheet APIs and removed unused stylesheet rules.

Changelog

CHANGELOG entry: Updated the Perps Close All Positions sheet to use the MetaMask Design System

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/TAT-3566

Manual testing steps

Feature: Perps Close All Positions sheet (MMDS)

  Scenario: user opens Close All Positions from Perps Home
    Given the user has an open Perps account with one or more open positions
    And the user is on Perps Home

    When the user opens the Close All Positions sheet
    Then the sheet title, description, margin/fees/you-receive summary, and Keep positions / Close all footer render with MMDS styling
    And the Close all button is shown as a danger primary action

  Scenario: user keeps positions
    Given the Close All Positions sheet is open with positions listed

    When the user taps Keep positions
    Then the sheet closes and positions remain open

  Scenario: user closes all positions
    Given the Close All Positions sheet is open with positions listed

    When the user taps Close all
    Then the closing/loading state is shown
    And positions are closed and the sheet dismisses (or the expected success toast/feedback appears)

  Scenario: fee and receive tooltips (if shown on this entry path)
    Given the Close All Positions summary shows fee / you-receive rows

    When tooltips are enabled for that summary surface
    Then info icons open the expected closing fees and you-receive tooltips

Unit tests:

yarn jest app/components/UI/Perps/Views/PerpsCloseAllPositionsView/PerpsCloseAllPositionsView.test.tsx
yarn jest app/components/UI/Perps/components/PerpsCloseSummary/PerpsCloseSummary.test.tsx

Screenshots/Recordings

Before

Simulator Screenshot - iPhone 15 Pro Max - 2026-07-14 at 11 50 51 Simulator Screenshot - iPhone 15 Pro Max - 2026-07-14 at 11 52 34

After

Simulator Screenshot - iPhone 15 Pro Max - 2026-07-15 at 00 03 15 Simulator Screenshot - iPhone 15 Pro Max - 2026-07-15 at 00 03 24

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
Touches the close-all trading confirmation UI and sheet dismiss/ref wiring on Perps Home, but close-all business logic stays in existing hooks; risk is mainly visual/regression in a high-impact user action.

Overview
Migrates the Perps Close All Positions flow from component-library bottom sheet primitives to MetaMask Design System (MMDS) so it matches other redesigned Perps surfaces.

PerpsCloseAllPositionsView now uses MMDS BottomSheet, BottomSheetHeader, BottomSheetFooter, Box, and Text, with goBack / onClose instead of shouldNavigateBack, and footer actions wired via primaryButtonProps / secondaryButtonProps (including isDanger on Close all). Adds PerpsCloseAllPositionsViewSelectorsIDs testIDs across sheet states and buttons; trims layout styles that MMDS/Box replace.

PerpsCloseSummary refactors fees, you-receive, and estimated-points rows to MMDS KeyValueRow and SectionDivider (margin + nested P&L row stays custom). Tooltip info icons use keyEndButtonIconProps when enabled.

PerpsHomeView types the close-all overlay ref with MMDS BottomSheetRef while cancel-all still uses the component-library ref.

Unit tests drop manual bottom-sheet mocks, assert via testIDs, use a fixed position timestamp, and PerpsCloseSummary tests switch to toBeOnTheScreen().

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

Co-authored-by: Cursor <cursoragent@cursor.com>
@brianacnguyen brianacnguyen self-assigned this Jul 15, 2026
@brianacnguyen brianacnguyen requested a review from a team as a code owner July 15, 2026 16:26
@brianacnguyen brianacnguyen added the team-design-system All issues relating to design system in Mobile label Jul 15, 2026
@github-actions

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.

@github-actions github-actions Bot added pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. size-L risk:low AI analysis: low risk labels Jul 15, 2026
@github-actions

github-actions Bot commented Jul 15, 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/Perps/Views/PerpsCloseAllPositionsView/PerpsCloseAllPositionsView.test.tsx 0/122 0/167 0/374

AI-detected flaky patterns

app/components/UI/Perps/Views/PerpsCloseAllPositionsView/PerpsCloseAllPositionsView.test.tsx

  • J9 — Module-level mutable let bindings not reset in beforeEach (high)
    • mockCloseAllHook is a const object defined at describe-scope whose jest.fn() values (handleCloseAll, handleKeepPositions) are created exactly once and shared across every test. Although jest.clearAllMocks() in beforeEach resets call counts, the object reference itself is never re-created. More critically, tests that call mockUsePerpsCloseAllPositions.mockReturnValue({ ...mockCloseAllHook, isClosing: true }) spread the stale jest.fn() references from the shared object. If any test mutates a property on mockCloseAllHook directly (e.g. mockCloseAllHook.error = 'some error'), that mutation persists into subsequent tests because the object is not re-initialized in beforeEach. The pattern is one step away from a classic J9 bleed: the shared mutable object is not reset, only the mock return value is re-set. The safe fix is to move the object construction into beforeEach (or a factory function called in beforeEach) so each test gets a fresh object with fresh jest.fn() instances.
    • Suggested fix in app/components/UI/Perps/Views/PerpsCloseAllPositionsView/PerpsCloseAllPositionsView.test.tsx:196:
      -  const mockCloseAllHook = {
      -    isClosing: false,
      -    positionCount: 1,
      -    handleCloseAll: jest.fn(),
      -    handleKeepPositions: jest.fn(),
      -    error: null,
      -  };
      +  let mockCloseAllHook: {
      +    isClosing: boolean;
      +    positionCount: number;
      +    handleCloseAll: jest.Mock;
      +    handleKeepPositions: jest.Mock;
      +    error: null | string;
      +  };
      +
      +  beforeEach(() => {
      +    jest.clearAllMocks();
      +    mockCloseAllHook = {
      +      isClosing: false,
      +      positionCount: 1,
      +      handleCloseAll: jest.fn(),
      +      handleKeepPositions: jest.fn(),
      +      error: null,
      +    };
      +    mockUsePerpsLivePositions.mockReturnValue({
      +      positions: mockPositions,
      +      isInitialLoading: false,
      +    });
      +    mockUsePerpsCloseAllCalculations.mockReturnValue(mockCalculations);
      +    mockUsePerpsCloseAllPositions.mockReturnValue(mockCloseAllHook);
      +    mockUsePerpsRewardAccountOptedIn.mockReturnValue({
      +      accountOptedIn: true,
      +      account: mockRewardAccountOptedIn as unknown as InternalAccount,
      +    });
      +  });
  • J3 — Missing jest.clearAllMocks() / jest.resetAllMocks() (high)
    • The four typed mock references are created using the as jest.MockedFunction<> cast pattern instead of jest.mocked(). Per the MetaMask unit-testing guidelines, jest.mocked(fn) must be used — never fn as jest.Mock or fn as jest.MockedFunction<>. While this is primarily a coding-guideline violation, it also carries a subtle flakiness risk: jest.mocked() is type-safe and always returns the live mock reference from the module registry, whereas a cast alias can silently diverge if the module mock is replaced between tests (e.g. via jest.isolateModules or a per-test jest.mock override). Replacing the casts with jest.mocked() eliminates this risk and aligns with project standards.
    • Suggested fix in app/components/UI/Perps/Views/PerpsCloseAllPositionsView/PerpsCloseAllPositionsView.test.tsx:130:
      -const mockUsePerpsLivePositions = usePerpsLivePositions as jest.MockedFunction<
      -  typeof usePerpsLivePositions
      ->;
      -const mockUsePerpsCloseAllCalculations =
      -  usePerpsCloseAllCalculations as jest.MockedFunction<
      -    typeof usePerpsCloseAllCalculations
      -  >;
      -const mockUsePerpsCloseAllPositions =
      -  usePerpsCloseAllPositions as jest.MockedFunction<
      -    typeof usePerpsCloseAllPositions
      -  >;
      -const mockUsePerpsRewardAccountOptedIn =
      -  usePerpsRewardAccountOptedIn as jest.MockedFunction<
      -    typeof usePerpsRewardAccountOptedIn
      -  >;
      +const mockUsePerpsLivePositions = jest.mocked(usePerpsLivePositions);
      +const mockUsePerpsCloseAllCalculations = jest.mocked(usePerpsCloseAllCalculations);
      +const mockUsePerpsCloseAllPositions = jest.mocked(usePerpsCloseAllPositions);
      +const mockUsePerpsRewardAccountOptedIn = jest.mocked(usePerpsRewardAccountOptedIn);

This check is informational only and does not block merging.

@brianacnguyen brianacnguyen removed the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label Jul 15, 2026
brianacnguyen and others added 3 commits July 15, 2026 10:59
Pin mock timestamps and replace weak/vacuous matchers with toBeOnTheScreen so presence checks stay deterministic and meaningful.

Co-authored-by: Cursor <cursoragent@cursor.com>
@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
Perps add funds Android Google Pixel 8 Pro (v14.0) 10.06s @mm-perps-engineering-team 📹 Watch
Perps open position and close it Android Google Pixel 8 Pro (v14.0) 23.04s @mm-perps-engineering-team 📹 Watch

Branch: refactor/perps-home-closeallpositions · Build: Normal · Commit: 87a5058 · View full run

Remove the MMDS BottomSheet mock and assert via shared selectors instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

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

E2E Test Selection:
All 8 changed files are within the Perps UI feature area (app/components/UI/Perps/). The changes involve:

  1. PerpsCloseAllPositionsView - Migrated from component-library to @metamask/design-system-react-native for BottomSheet, BottomSheetFooter, BottomSheetHeader, Box, and Text components. The button props API changed from buttonPropsArray to secondaryButtonProps/primaryButtonProps, and shouldNavigateBack was replaced with goBack. New testIDs were added throughout. This is a functional UI change that could affect the Close All Positions flow.

  2. PerpsHomeView - Minor import alias fix: BottomSheetRef now imported from design-system-react-native for closeAllSheetRef, while cancelAllSheetRef keeps the component-library version. This ensures the correct ref type is used for the migrated PerpsCloseAllPositionsView.

  3. PerpsCloseSummary - Migrated from custom TouchableOpacity rows to KeyValueRow/SectionDivider from design-system-react-native. Tooltip handling changed from TouchableOpacity wrappers to keyEndButtonIconProps. FontWeight styling added.

  4. Style files - Removed styles now handled by design system components.

  5. Test files - Unit tests for the above components.

Tag selection rationale:

  • SmokePerps: Primary tag — changes directly affect Perps UI components (Close All Positions flow, Close Summary display in position closing flows)
  • SmokeWalletPlatform: Required per SmokePerps description — Perps is a section inside the Trending tab, and changes to Perps views affect Trending
  • SmokeConfirmations: Required per SmokePerps description — Add Funds deposits are on-chain transactions that go through confirmations

No other feature areas are affected. The changes are UI-only migrations with no controller, navigation, or shared infrastructure changes.

Performance Test Selection:
The changes are purely UI component migrations (component-library → design-system-react-native) within the Perps feature. No data fetching, controller logic, rendering loops, or performance-sensitive code paths were modified. The PerpsPreps performance tag covers perps market loading and position management, but these changes only affect the visual presentation of the Close All Positions modal and Close Summary component — not the data loading or rendering performance of the main perps views. No performance tests are warranted.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant