Conversation
✨ Files requiring CODEOWNER review ✨👨🔧 @MetaMask/perps (13 files, +964 -402)
🧪 @MetaMask/qa (1 files, +34 -28)
|
Builds ready [9cf1eb8]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [6c75538]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…taMask/metamask-extension into feat/tat-2582-merge-perps-home-to-tab
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| const estimatedFees = useMemo( | ||
| () => closeSize * currentPrice * PERPS_MARKET_ORDER_FEE_RATE, | ||
| [closeSize, currentPrice], | ||
| ); |
There was a problem hiding this comment.
Close position modal missing getPerpsController in useCallback deps
Low Severity
The estimatedFees useMemo depends on closeSize and currentPrice but the dependency array omits PERPS_MARKET_ORDER_FEE_RATE. While this is a constant and won't change at runtime, more importantly the handleClose callback's dependency array (lines 134–141) is missing getPerpsController — though since it's a module-level import this is safe. However, the callback is also missing getPerpsStreamManager if it were used, and doesn't guard against unmount during the async operation (no cleanup/cancelled flag pattern).
Triggered by project rule: Front-End Performance Rules: Hooks & Effects
|
|
||
| const result = await controller.closePosition(params); | ||
| if (!result.success) { | ||
| throw new Error(result.error || 'Failed to close position'); |
| onClose(); | ||
| } catch (err) { | ||
| setError( | ||
| err instanceof Error ? err.message : 'An unknown error occurred', |
| <Modal | ||
| isOpen={isOpen} | ||
| onClose={onClose} | ||
| data-testid="perps-close-position-modal" |
There was a problem hiding this comment.
do we cnetralize test-ids in extension?
|
|
||
| // Track created price line objects for cleanup | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const activePriceLinesRef = useRef<any[]>([]); |
There was a problem hiding this comment.
any chance we can avoid the any?
| * Used to estimate fees in close/reverse position flows. | ||
| * TODO: Replace with dynamic fee from the API when available. | ||
| */ | ||
| export const PERPS_MARKET_ORDER_FEE_RATE = 0.0001; |
There was a problem hiding this comment.
this might be a blocker...
There was a problem hiding this comment.
I think this comes directly from the controller right? So we should be able to add this in the integration branch (this is still mocekd UI)
abretonc7s
left a comment
There was a problem hiding this comment.
approving but with a few comments we will need to address
Builds ready [0461897]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
geositta
left a comment
There was a problem hiding this comment.
I want to call out two functional regressions that I think we should fix if not here then in separate tasks:
I believe this is a functional mismatch, but I may be missing product intent. "Add exposure" is currently described as increasing position size, yet it routes to mode=modify, and submit in that mode only calls updatePositionTPSL (no placeOrder). If the intent is truly TP/SL only here, we should rename/recopy the action so it does not promise a size increase. Otherwise, I would ask we route Add Exposure to an order-placing flow that calls placeOrder in the current direction.
The second is in ui/hooks/perps/usePerpsDeposit.ts: findNetworkClientIdByChainId is async, but the current code passes it into addTransaction without awaiting it. That means networkClientId is a promise instead of a string at runtime, which can fail before confirmation opens. We should await the network client lookup before building transaction options.
geositta
left a comment
There was a problem hiding this comment.
This refactor is a textbook composition win! Nice work on the child components with their own UI logic.
Submitting this second review with some code quality improvements.
| const [marginModalMode, setMarginModalMode] = useState< | ||
| 'add' | 'remove' | null | ||
| >(null); | ||
| const [isReverseModalOpen, setIsReverseModalOpen] = useState(false); |
There was a problem hiding this comment.
These can never be open simultaneously, so a single discriminated union is safer and simpler:
type ModalState =
| { type: 'closed' }
| { type: 'modify' }
| { type: 'margin'; mode: 'add' | 'remove' }
| { type: 'reverse' }
| { type: 'tpsl' }
| { type: 'close' };
const [activeModal, setActiveModal] = useState<ModalState>({ type:
'closed' });
This eliminates impossible states (two modals open at once) and reduces 12 state variables to 1.
| : p, | ||
| ); | ||
| streamManager.positions.pushData(optimisticallyUpdatedPositions); | ||
| setTimeout(async () => { |
There was a problem hiding this comment.
Medium term can we make this delayed refetch cancellable and context safe? If account/context changes during this 2.5s window, it can push stale positions into the shared stream manager.
The practical fix is to hold the timeout id in a ref, clear it on unmount/close, and guard before push by checking we are still on the same selected address/current stream context.
| const handleTpPercentInputChange = useCallback( | ||
| (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const { value } = event.target; | ||
| if (value === '' || /^-?\d*(?:\.\d*)?$/u.test(value)) { |
There was a problem hiding this comment.
/^-?\d*(?:.\d*)?$/u // percent inputs
/^[\d,](?:.\d)?$/u // price inputs
Extract to a const:
const NUMERIC_INPUT_PATTERN = /^-?\d*(?:.\d*)?$/u;
const PRICE_INPUT_PATTERN = /^[\d,](?:.\d)?$/u;
|
|
||
| let leverageValue = 1; | ||
| if (typeof position.leverage === 'object' && position.leverage !== null) { | ||
| leverageValue = (position.leverage as { value?: number }).value ?? 1; |
There was a problem hiding this comment.
This as cast defeats TypeScript's narrowing. The typeof check above is a runtime guard but the as cast tells TS to stop checking. For code quality:
Fix the Position type's leverage field to be a proper discriminated union: number | { value: number }.
Then use a type guard or just 'value' in position.leverage for narrowing, no cast needed.
| async waitForBalanceSection(timeout?: number): Promise<void> { | ||
| await this.driver.waitForSelector( | ||
| '[data-testid="perps-balance-actions"], [data-testid="perps-balance-actions-empty"]', | ||
| '[data-testid="perps-balance-dropdown"]', |
There was a problem hiding this comment.
ℹ️ not blocking, as this is a pre-existing issue, but selectors should be moved on the top section
( cc @racitores --> ticket here and original review here )
|
ccharly
left a comment
There was a problem hiding this comment.
LGTM for accounts
ui/components/multichain/account-overview/account-overview-tabs.tsx
Description
Eliminates the standalone Perps Home page and consolidates all its content into the Perps tab on the main account overview. Users no longer navigate to a separate /perps/home route — everything lives in one place.
Also adds modals for managing positions/closing a position, as well as other design adjustments from figma.
Changelog
CHANGELOG entry: Merge Perps Home UI into Perps Tab
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Screen.Recording.2026-03-04.at.9.16.09.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Moderate UI refactor that changes primary navigation and state wiring for Perps, plus new close/reverse position flows that call the Perps controller and could impact trading actions if miswired.
Overview
Consolidates the former standalone Perps Home experience into the wallet’s Perps tab (
PerpsTabView), replacing the old control bar with a newPerpsBalanceDropdown, restructuring positions/orders rendering, and adding new sections like Explore markets, Watchlist (stubbed), Support & Learn, and always-on Recent Activity + tutorial modal.Adds new position-management modals (
ClosePositionModal,ReversePositionModal) and refactors margin editing into reusable modal content (EditMarginModalContent) with a dedicatedEditMarginModal; introduces a fixed market-order fee constant for fee estimates.Updates order entry UI to include an inline Add funds affordance, hardens TP/SL input parsing, and fixes a candlestick chart crosshair update loop. E2E navigation and selectors are updated for the tab-based flow (tests currently skipped), i18n strings are refreshed (new Perps labels in
en/en_GB, old explore/order/positions keys removed across locales), and Jest baselines are adjusted for new component tests.Written by Cursor Bugbot for commit 0461897. This will update automatically on new commits. Configure here.