MetaMask / MetaMask/metamask-mobile
Add co-located test for live transaction selector in TransactionDetailsSheet
- Dominant language
- TypeScript
- Stars
- 3k
- Forks
- 1.7k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 669
Description
> **Performance audit finding** · Severity: **Low** · Effort: Easy · Fix risk: Simple · Test safety net: Uncovered (no co-located TransactionDetailsSheet.test.tsx)
> Owner: `@MetaMask/mobile-core-ux`
> File: `app/components/UI/TransactionElement/TransactionDetailsSheet/TransactionDetailsSheet.tsx:61`
### What is this about?
`TransactionDetailsSheet` subscribes to live transaction state with an inline `useSelector` that calls the parametrized selector `selectTransactionMetadataById(state, tx.id)`, then derives `currentTx` via `useMemo`. The TransactionController updates the metadata for an in-flight transaction frequently (status, gas, confirmations), so this component re-renders on each update. The `useMemo` for `currentTx` clones into a new object every time `liveTransaction` changes, which is correct, but there is no co-located test asserting render behavior, so a regression that broadens the selector (e.g. selecting the whole transactions array) would not be caught.
**Why it matters**
This sheet is shown for pending/recent transactions and reads live-updating Redux state. If a future change replaces the per-id parametrized selector with a coarser selector or drops the `useMemo`, the sheet would re-render on every transaction-controller tick (which fires often during confirmations), with no test to flag the regression.
### Scenario
N/A — see Technical Details.
### Design
N/A — internal performance change; no UI/design impact.
### Technical Details
**Evidence**
`app/components/UI/TransactionElement/TransactionDetailsSheet/TransactionDetailsSheet.tsx:61`
```ts
const liveTransaction = useSelector((state: RootState) =>
selectTransactionMetadataById(state, tx.id),
);
const currentTx = useMemo(
() =>
liveTransaction
? { ...liveTransaction, txParams: { ...liveTransaction.txParams } }
: tx,
[liveTransaction, tx],
);
```
**Fix**
The current implementation is already reasonable (parametrized selector + memoized projection). The actionable item is to add a co-located `TransactionDetailsSheet.test.tsx` that mounts the sheet, dispatches an unrelated transaction-state change, and asserts the component does not re-render / `currentTx` reference is stable for the same `tx.id`. This locks in the per-id subscription so future selector changes cannot silently regress it.
### Threat Modeling Framework
N/A — performance-only change; behavior is preserved, no new data flow / trust boundary / attack surface.
### Acceptance Criteria
- - Add `app/components/UI/TransactionElement/TransactionDetailsSheet/TransactionDetailsSheet.test.tsx` with a render-count assertion across an unrelated TransactionController update.
- Run `yarn jest app/components/UI/TransactionElement/TransactionDetailsSheet`.
### References
- File: `app/components/UI/TransactionElement/TransactionDetailsSheet/TransactionDetailsSheet.tsx:61`
- Source: MetaMask Mobile performance audit — finding `redux-transactiondetailssheet-livetransaction`
- Owner (CODEOWNERS / best-effort): @MetaMask/mobile-core-ux
- Status: **UNVALIDATED**
Contributor guide
Research direction
Start with app/components/UI/TransactionElement/TransactionDetailsSheet/TransactionDetailsSheet.tsx at the live selector, then add the co-located TransactionDetailsSheet.test.tsx. Mount the sheet, dispatch an unrelated TransactionController transaction-state update, and assert the component does not re-render for the same tx.id. Run yarn jest app/components/UI/TransactionElement/TransactionDetailsSheet.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- react-native, typescript
- Domain
- mobile, testing
- Issue type
- Feature
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100