Skip to content

Commit 2af3c67

Browse files
Cal-LCal-L
authored
refactor: Move set existing user from login to vault recovery method (#24252)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This change moves setting existing user from the `Login` component to the `initializeVaultFromBackup` method in `EngineService`. It used to live in the `Login` component since we needed to ensure that existing user was set to true so that users would not re-encounter the vault recovery flow when backgrounding/re-opening post vault recovery. However, it does not need to belong in the component if vault recovery relies on `initializeVaultFromBackup`, which is where we've moved the set existing user call. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: Part of https://consensyssoftware.atlassian.net/browse/MCWP-240 ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> Existing user is set to true on vault recovery https://github.com/user-attachments/assets/a1cb3040-4545-44f3-8472-74b280644e41 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Shifts responsibility for marking returning users to the engine layer during vault recovery. > > - Sets `existingUser` via Redux in `EngineService.initializeVaultFromBackup` after successful re-init; adds test asserting dispatch of `setExistingUser(true)` > - Removes `setExistingUser` usage and `isVaultRecovery` logic from `Login` (no `useDispatch`; route params/type cleaned up) > - Updates `WalletRestored` to navigate to `Routes.ONBOARDING.LOGIN` without `{ isVaultRecovery }`; adjusts related tests > - Cleans up `Login` tests by removing mocks and cases tied to vault recovery flag > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 97b1a1a. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 218f8fd commit 2af3c67

6 files changed

Lines changed: 25 additions & 124 deletions

File tree

‎app/components/Views/Login/index.test.tsx‎

Lines changed: 0 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ import {
3232
TRUE,
3333
} from '../../../constants/storage';
3434
import { useMetrics } from '../../hooks/useMetrics';
35-
import { setExistingUser } from '../../../actions/user';
3635

3736
const mockNavigate = jest.fn();
3837
const mockReplace = jest.fn();
@@ -125,13 +124,6 @@ jest.mock('../../../actions/security', () => ({
125124
},
126125
}));
127126

128-
jest.mock('../../../actions/user', () => ({
129-
setExistingUser: jest.fn((value) => ({
130-
type: 'SET_EXISTING_USER',
131-
existingUser: value,
132-
})),
133-
}));
134-
135127
jest.mock('../../../store/storage-wrapper', () => ({
136128
getItem: jest.fn().mockResolvedValue(null),
137129
setItem: jest.fn(),
@@ -793,102 +785,6 @@ describe('Login', () => {
793785
});
794786
});
795787

796-
describe('Vault Recovery', () => {
797-
beforeEach(() => {
798-
jest.clearAllMocks();
799-
});
800-
801-
it('dispatches setExistingUser after successful login from vault recovery', async () => {
802-
// Arrange
803-
mockRoute.mockReturnValue({
804-
params: {
805-
locked: false,
806-
oauthLoginSuccess: false,
807-
isVaultRecovery: true,
808-
},
809-
});
810-
811-
(StorageWrapper.getItem as jest.Mock).mockImplementation((key) => {
812-
if (key === OPTIN_META_METRICS_UI_SEEN) return Promise.resolve('true');
813-
return Promise.resolve(null);
814-
});
815-
816-
(Authentication.userEntryAuth as jest.Mock).mockResolvedValueOnce(
817-
undefined,
818-
);
819-
(
820-
Authentication.componentAuthenticationType as jest.Mock
821-
).mockResolvedValueOnce({
822-
currentAuthType: 'password',
823-
});
824-
825-
const { getByTestId } = renderWithProvider(<Login />);
826-
const passwordInput = getByTestId(LoginViewSelectors.PASSWORD_INPUT);
827-
const loginButton = getByTestId(LoginViewSelectors.LOGIN_BUTTON_ID);
828-
829-
// Act
830-
await act(async () => {
831-
fireEvent.changeText(passwordInput, 'validPassword123');
832-
});
833-
834-
await act(async () => {
835-
fireEvent.press(loginButton);
836-
});
837-
838-
await act(async () => {
839-
await new Promise((resolve) => setTimeout(resolve, 0));
840-
});
841-
842-
// Assert
843-
expect(setExistingUser).toHaveBeenCalledWith(true);
844-
});
845-
846-
it('does not dispatch setExistingUser when not from vault recovery', async () => {
847-
// Arrange
848-
mockRoute.mockReturnValue({
849-
params: {
850-
locked: false,
851-
oauthLoginSuccess: false,
852-
isVaultRecovery: false,
853-
},
854-
});
855-
856-
(StorageWrapper.getItem as jest.Mock).mockImplementation((key) => {
857-
if (key === OPTIN_META_METRICS_UI_SEEN) return Promise.resolve('true');
858-
return Promise.resolve(null);
859-
});
860-
861-
(Authentication.userEntryAuth as jest.Mock).mockResolvedValueOnce(
862-
undefined,
863-
);
864-
(
865-
Authentication.componentAuthenticationType as jest.Mock
866-
).mockResolvedValueOnce({
867-
currentAuthType: 'password',
868-
});
869-
870-
const { getByTestId } = renderWithProvider(<Login />);
871-
const passwordInput = getByTestId(LoginViewSelectors.PASSWORD_INPUT);
872-
const loginButton = getByTestId(LoginViewSelectors.LOGIN_BUTTON_ID);
873-
874-
// Act
875-
await act(async () => {
876-
fireEvent.changeText(passwordInput, 'validPassword123');
877-
});
878-
879-
await act(async () => {
880-
fireEvent.press(loginButton);
881-
});
882-
883-
await act(async () => {
884-
await new Promise((resolve) => setTimeout(resolve, 0));
885-
});
886-
887-
// Assert
888-
expect(setExistingUser).not.toHaveBeenCalled();
889-
});
890-
});
891-
892788
describe('Remember Me Authentication', () => {
893789
it('set up remember me authentication when auth type is REMEMBER_ME', async () => {
894790
(Authentication.getType as jest.Mock).mockResolvedValueOnce({

‎app/components/Views/Login/index.tsx‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,7 @@ import {
2929
saveOnboardingEvent as saveEvent,
3030
} from '../../../actions/onboarding';
3131
import { setAllowLoginWithRememberMe as setAllowLoginWithRememberMeUtil } from '../../../actions/security';
32-
import { setExistingUser } from '../../../actions/user';
33-
import { connect, useDispatch, useSelector } from 'react-redux';
32+
import { connect, useSelector } from 'react-redux';
3433
import { Dispatch } from 'redux';
3534
import {
3635
passcodeType,
@@ -111,7 +110,6 @@ const EmptyRecordConstant = {};
111110

112111
interface LoginRouteParams {
113112
locked: boolean;
114-
isVaultRecovery?: boolean;
115113
}
116114

117115
interface LoginProps {
@@ -142,15 +140,12 @@ const Login: React.FC<LoginProps> = ({ saveOnboardingEvent }) => {
142140

143141
const navigation = useNavigation<StackNavigationProp<ParamListBase>>();
144142
const route = useRoute<RouteProp<{ params: LoginRouteParams }, 'params'>>();
145-
const dispatch = useDispatch();
146143
const {
147144
styles,
148145
theme: { colors, themeAppearance },
149146
} = useStyles(stylesheet, EmptyRecordConstant);
150147
const setAllowLoginWithRememberMe = (enabled: boolean) =>
151148
setAllowLoginWithRememberMeUtil(enabled);
152-
// coming from vault recovery flow flag
153-
const isComingFromVaultRecovery = route?.params?.isVaultRecovery ?? false;
154149

155150
const isSeedlessPasswordOutdated = useSelector(
156151
selectIsSeedlessPasswordOutdated,
@@ -407,13 +402,6 @@ const Login: React.FC<LoginProps> = ({ saveOnboardingEvent }) => {
407402
},
408403
);
409404

410-
// CRITICAL: Set existingUser = true after successful vault unlock from recovery
411-
// This prevents the vault recovery screen from appearing again on app restart
412-
// Only set after successful unlock to ensure vault is unlocked and credentials are stored
413-
if (isComingFromVaultRecovery) {
414-
dispatch(setExistingUser(true));
415-
}
416-
417405
await checkMetricsUISeen();
418406

419407
setLoading(false);
@@ -428,8 +416,6 @@ const Login: React.FC<LoginProps> = ({ saveOnboardingEvent }) => {
428416
loading,
429417
handleLoginError,
430418
checkMetricsUISeen,
431-
dispatch,
432-
isComingFromVaultRecovery,
433419
]);
434420

435421
const handleLogin = async () => {

‎app/components/Views/RestoreWallet/WalletRestored.test.tsx‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,6 @@ describe('WalletRestored', () => {
122122
await waitFor(() => {
123123
expect(mockNavigation.replace).toHaveBeenCalledWith(
124124
Routes.ONBOARDING.LOGIN,
125-
{ isVaultRecovery: true },
126125
);
127126
});
128127
});
@@ -223,7 +222,6 @@ describe('WalletRestored', () => {
223222
await waitFor(() => {
224223
expect(mockNavigation.replace).toHaveBeenCalledWith(
225224
Routes.ONBOARDING.LOGIN,
226-
{ isVaultRecovery: true },
227225
);
228226
});
229227
});

‎app/components/Views/RestoreWallet/WalletRestored.tsx‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,7 @@ const WalletRestored = () => {
5959
Logger.error(error as Error, 'Failed to clear migration error flag');
6060
}
6161

62-
navigation.replace(Routes.ONBOARDING.LOGIN, {
63-
isVaultRecovery: true,
64-
});
62+
navigation.replace(Routes.ONBOARDING.LOGIN);
6563
}, [navigation]);
6664

6765
const onPressBackupSRP = useCallback(async (): Promise<void> => {

‎app/core/EngineService/EngineService.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
} from '../../store/persistConfig';
1414
import { BACKGROUND_STATE_CHANGE_EVENT_NAMES } from '../Engine/constants';
1515
import { getPersistentState } from '../../store/getPersistentState/getPersistentState';
16+
import { setExistingUser } from '../../actions/user';
1617

1718
// Mock NavigationService
1819
jest.mock('../NavigationService', () => ({
@@ -284,6 +285,23 @@ describe('EngineService', () => {
284285
jest.useFakeTimers();
285286
});
286287

288+
it('sets existingUser flag after successful vault recovery', async () => {
289+
// Arrange
290+
jest.useRealTimers();
291+
292+
// Act
293+
await engineService.start();
294+
await engineService.initializeVaultFromBackup();
295+
296+
// Assert
297+
expect(ReduxService.store.dispatch).toHaveBeenCalledWith(
298+
setExistingUser(true),
299+
);
300+
301+
// Restore fake timers for other tests
302+
jest.useFakeTimers();
303+
});
304+
287305
it('sets up persistence subscriptions after vault recovery', async () => {
288306
// Arrange
289307
jest.useRealTimers();

‎app/core/EngineService/EngineService.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import { trackVaultCorruption } from '../../util/analytics/vaultCorruptionTracki
2929
import { INIT_BG_STATE_KEY, LOG_TAG, UPDATE_BG_STATE_KEY } from './constants';
3030
import { StateConstraint } from '@metamask/base-controller';
3131
import { hasPersistedState } from './utils/persistence-utils';
32+
import { setExistingUser } from '../../actions/user';
3233

3334
export class EngineService {
3435
private engineInitialized = false;
@@ -367,6 +368,10 @@ export class EngineService {
367368
if (instance) {
368369
// Pass state to detect controllers that changed during init
369370
this.initializeControllers(instance, state as Record<string, unknown>);
371+
// CRITICAL: Set existingUser = true after successful vault unlock from recovery
372+
// This prevents the vault recovery screen from appearing again on app restart
373+
// Only set after successful unlock to ensure vault is unlocked and credentials are stored
374+
ReduxService.store.dispatch(setExistingUser(true));
370375
// this is a hack to give the engine time to reinitialize
371376
await new Promise((resolve) => setTimeout(resolve, 2000));
372377
return {

0 commit comments

Comments
 (0)