chore: Replace Settings modals with design-system BottomSheets - #34847
chore: Replace Settings modals with design-system BottomSheets#34847georgewrmarshall wants to merge 7 commits into
Conversation
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
metamask-ci
Bot
added
the
team-design-system
PR template — items to address before "Ready for review"Warnings — informational, address before merging:
See docs/readme/ready-for-review.md for the full Definition of Ready for Review. |
🧪 Flaky unit test detectionRun history flaky detectionHistorical 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:
AI-detected flaky patterns
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: LoaderModal close race condition
- Guarded stale close completions against reopen and reopened the sheet on visibility=true to avoid calling onCancel or unmounting during a programmatic close.
Or push these changes by commenting:
@cursor push c978ef0a47
Preview (c978ef0a47)
diff --git a/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx b/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx
--- a/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx
+++ b/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx
@@ -16,17 +16,30 @@
const [isMounted, setIsMounted] = useState(isVisible);
const sheetRef = useRef<BottomSheetRef>(null);
const closingDueToVisibilityRef = useRef(false);
+ // Track latest visibility to disambiguate stale close callbacks.
+ const isVisibleRef = useRef(isVisible);
+ useEffect(() => {
+ isVisibleRef.current = isVisible;
+ }, [isVisible]);
useEffect(() => {
if (isVisible) {
+ // Reset programmatic-close marker on explicit reopen.
closingDueToVisibilityRef.current = false;
setIsMounted(true);
+ // Ensure the sheet is opened in case a previous close finished.
+ sheetRef.current?.onOpenBottomSheet();
return;
}
if (isMounted) {
closingDueToVisibilityRef.current = true;
sheetRef.current?.onCloseBottomSheet(() => {
+ // If visibility flipped back to true while the close was animating,
+ // ignore this stale completion to avoid cancel/unmount flicker.
+ if (isVisibleRef.current) {
+ return;
+ }
setIsMounted(false);
closingDueToVisibilityRef.current = false;
});
@@ -34,9 +47,16 @@
}, [isMounted, isVisible]);
const handleSheetClosed = useCallback(() => {
+ // If the parent wants it visible again, ignore this stale close event.
+ if (isVisibleRef.current) {
+ return;
+ }
setIsMounted(false);
if (!closingDueToVisibilityRef.current) {
onCancel();
+ } else {
+ // Programmatic close finished as intended; reset flag.
+ closingDueToVisibilityRef.current = false;
}
}, [onCancel]);You can send follow-ups to the cloud agent here.
…ancel and flicker by guarding close callbacks with current visibility and reopening sheet on visibility true
| <Text style={styles.modalTitle} variant={TextVariant.HeadingMD}> | ||
| {strings('app_settings.reset_account_modal_title')} | ||
| </Text> |
There was a problem hiding this comment.
@cursoragent remove the wrapping text BottomSheet header should be standardized font size and is handled by the component
| <Text style={styles.modalTitle} variant={TextVariant.HeadingMD}> | |
| {strings('app_settings.reset_account_modal_title')} | |
| </Text> | |
| {strings('app_settings.reset_account_modal_title')} |
There was a problem hiding this comment.
Updated ResetAccountModal to match the review suggestion by removing the wrapped Text inside BottomSheetHeader and passing just:
| <Text style={styles.modalTitle} variant={TextVariant.HeadingMD}> | |
| {strings('app_settings.reset_account_modal_title')} | |
| </Text> | |
| {strings('app_settings.reset_account_modal_title')} |
Pushed the fix to the existing PR: #34847.
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
…ancel and flicker by guarding close callbacks with current visibility and reopening sheet on visibility true Applied via @cursor push command
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dismiss blocked while visible
- Updated handleSheetClosed to gate the early return behind programmatic-close state so interactive dismiss still triggers onCancel even when isVisible is true.
Or push these changes by commenting:
@cursor push 8928d9ad1e
Preview (8928d9ad1e)
diff --git a/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx b/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx
--- a/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx
+++ b/app/components/UI/Notification/SwitchLoadingModal/LoaderModal.tsx
@@ -47,17 +47,19 @@
}, [isMounted, isVisible]);
const handleSheetClosed = useCallback(() => {
- // If the parent wants it visible again, ignore this stale close event.
- if (isVisibleRef.current) {
+ // If a programmatic close finished but the parent reopened mid-animation,
+ // ignore this stale completion to avoid cancel/unmount flicker.
+ if (closingDueToVisibilityRef.current && isVisibleRef.current) {
return;
}
setIsMounted(false);
- if (!closingDueToVisibilityRef.current) {
- onCancel();
- } else {
- // Programmatic close finished as intended; reset flag.
+ if (closingDueToVisibilityRef.current) {
+ // Programmatic close finished as intended; reset flag and do not cancel.
closingDueToVisibilityRef.current = false;
+ return;
}
+ // Interactive dismiss: notify parent so it can clear visibility.
+ onCancel();
}, [onCancel]);
if (!isMounted) {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 6020eff. Configure here.
| // If the parent wants it visible again, ignore this stale close event. | ||
| if (isVisibleRef.current) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Dismiss blocked while visible
Medium Severity
The new isVisibleRef early return in handleSheetClosed skips onCancel whenever the parent still wants the sheet visible. That blocks intentional swipe/backdrop dismiss on an isInteractable sheet, so close events never sync parent state and the sheet can stay closed while isVisible remains true.
Reviewed by Cursor Bugbot for commit 6020eff. Configure here.
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: Performance Test Selection: |
|
|
| Platform | Device | Reason | Recording |
|---|---|---|---|
| Android | Google Pixel 8 Pro (v14.0) | Quality gates exceeded | 📹 Watch |
🔬 App profiling check · Current run 31894865929 · Baseline (last run on main (scenario also failing)) run 30897750395 @ 67486d2
⚠️ No green baseline onmain— comparing against the latest usable profiling.
Summary:
ℹ️ API calls unavailable:
Network logs API error: Bad Request
Full metric table (+10% variance rules)
Disclaimer — allowed variance: a +10% margin over the baseline is permitted.
- If
Current <= Baseline + 10%, treated as acceptable noise.- If
Current > Baseline + 10%, Current and variance % are highlighted with⚠️ .
| Metric | Baseline | Current | Δ |
|---|---|---|---|
| CPU avg | 5.95% | 7.36% | +1.41 (+23.7%) |
| CPU max | 19.17% | 19.11% | -0.06 (-0.3%) |
| Memory avg | 586.2 MB | 711.22 MB | +125.02 (+21.3%) |
| Memory max | 722.61 MB | 927.89 MB | +205.28 (+28.4%) |
| Slow frames | 2.51% | 9.11% | +6.6 (+263%) |
| Frozen frames | 0% | 0% | 0 (0%) |
| ANRs | 0 | 0 | 0 (0%) |
| Issues | 2 | 2 | 0 (0%) |
| Critical issues | 1 | 1 | 0 (0%) |
| App size | 328.67 MB | 329.53 MB | +0.86 (+0.3%) |
Seedless Onboarding: Apple Login New User
| Platform | Device | Reason | Recording |
|---|---|---|---|
| Android | Google Pixel 8 Pro (v14.0) | Quality gates exceeded | 📹 Watch |
🔬 App profiling check · Current run 31894865929 · Baseline (last run on main (scenario also failing)) run 30897750395 @ 67486d2
⚠️ No green baseline onmain— comparing against the latest usable profiling.
Summary:
ℹ️ API calls unavailable:
Network logs API error: Bad Request
Full metric table (+10% variance rules)
Disclaimer — allowed variance: a +10% margin over the baseline is permitted.
- If
Current <= Baseline + 10%, treated as acceptable noise.- If
Current > Baseline + 10%, Current and variance % are highlighted with⚠️ .
| Metric | Baseline | Current | Δ |
|---|---|---|---|
| CPU avg | 13.93% | 5.04% | -8.89 (-63.8%) |
| CPU max | 18.74% | 26.15% | +7.41 (+39.5%) |
| Memory avg | 540.28 MB | 595.69 MB | +55.41 (+10.3%) |
| Memory max | 604.4 MB | 962.88 MB | +358.48 (+59.3%) |
| Slow frames | 3.05% | 15.69% | +12.64 (+414.4%) |
| Frozen frames | 0% | 0% | 0 (0%) |
| ANRs | 0 | 0 | 0 (0%) |
| Issues | 1 | 2 | +1 (+100%) |
| Critical issues | 1 | 1 | 0 (0%) |
| App size | 328.67 MB | 329.53 MB | +0.86 (+0.3%) |
@mm-perps-engineering-team
Perps open position and close it
| Platform | Device | Reason | Recording |
|---|---|---|---|
| Android | Google Pixel 8 Pro (v14.0) | no_performance_metrics | 📹 Watch |
✅ Passed Tests (19)
| Test | Platform | Device | Duration | Team | Recording |
|---|---|---|---|---|---|
| Aggregated Balance Loading Time, SRP 1 + SRP 2 + SRP 3 | Android | Google Pixel 8 Pro (v14.0) | 8.34s | @assets-dev-team | 📹 Watch |
| Cross-chain swap flow - ETH to SOL - 50+ accounts, SRP 1 + SRP 2 + SRP 3 | Android | Google Pixel 8 Pro (v14.0) | 4.83s | @swap-bridge-dev-team | 📹 Watch |
| Asset View, SRP 1 + SRP 2 + SRP 3 | Android | Google Pixel 8 Pro (v14.0) | 2.19s | @assets-dev-team | 📹 Watch |
| Swap flow - ETH to LINK, SRP 1 + SRP 2 + SRP 3 | Android | Google Pixel 8 Pro (v14.0) | 1.24s | @swap-bridge-dev-team | 📹 Watch |
| Import SRP with +50 accounts, SRP 1, SRP 2, SRP 3 | Android | Google Pixel 8 Pro (v14.0) | 3.80s | @Accounts-team | 📹 Watch |
| Cold Start: Measure ColdStart To Login Screen | Android | Google Pixel 8 Pro (v14.0) | 3.62s | @metamask-mobile-platform | 📹 Watch |
| Measure Warm Start: Login To Wallet Screen | Android | Google Pixel 8 Pro (v14.0) | 1.46s | @metamask-mobile-platform | 📹 Watch |
| Measure Warm Start: Warm Start to Login Screen | Android | Google Pixel 8 Pro (v14.0) | 0.18s | @metamask-mobile-platform | 📹 Watch |
| Perps add funds | Android | Google Pixel 8 Pro (v14.0) | 9.80s | @mm-perps-engineering-team | 📹 Watch |
| Predict Available Balance - Complete Flow Performance | Android | Google Pixel 8 Pro (v14.0) | 0.84s | @team-predict | 📹 Watch |
| Predict Deposit - Complete Flow Performance | Android | Google Pixel 8 Pro (v14.0) | 11.24s | @team-predict | 📹 Watch |
| Predict Market Details - Complete Flow Performance | Android | Google Pixel 8 Pro (v14.0) | 2.79s | @team-predict | 📹 Watch |
| Measure Cold Start To Onboarding Screen | Android | Google Pixel 8 Pro (v14.0) | 2.83s | @metamask-mobile-platform | 📹 Watch |
| Onboarding Import SRP with +50 accounts, SRP 3 | Android | Google Pixel 8 Pro (v14.0) | 4.55s | @metamask-onboarding-team | 📹 Watch |
| Money Home after fresh wallet creation with empty balance | Android | Google Pixel 8 Pro (v14.0) | 2.17s | @mm-earn-team | 📹 Watch |
| Money Home after importing SRP with funded balance | Android | Google Pixel 8 Pro (v14.0) | 3.32s | @mm-earn-team | 📹 Watch |
| Account creation after fresh install | Android | Google Pixel 8 Pro (v14.0) | 1.80s | @metamask-onboarding-team | 📹 Watch |
| Seedless Onboarding: Google Login New User | Android | Google Pixel 8 Pro (v14.0) | 2.08s | @metamask-onboarding-team | 📹 Watch |
| Seedless Onboarding: Telegram Login New User | Android | Google Pixel 8 Pro (v14.0) | 6.19s | @metamask-onboarding-team | 📹 Watch |
Branch: cursor/settings-bottomsheets-e2f5 · Build: E2E · Commit: a1bbfc2 · View full run
georgewrmarshall
changed the title
2 participants
Description
Converted Settings confirmation dialogs that were using deprecated /
react-native-modal-based modals toBottomSheetfrom@metamask/design-system-react-native, mounted at the screen root so they properly overlay content.Follow-up: migrated the new sheets to the most up-to-date MMDS usage patterns (string titles for
BottomSheetHeader,Box/MMDSText, andtwClassNamewhere applicable) and removed leftover custom modal styles.Follow-up: ran
yarn format:checkand applied formatting fixes (no behavior changes).Specifically:
react-native-modalwrapper with a design-systemBottomSheetChangelog
CHANGELOG entry: Updated Settings confirmation dialogs to use BottomSheets instead of deprecated modals.
Related issues
Refs: N/A
Manual testing steps
Screenshots/Recordings
Before
N/A
After
N/A
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist