Skip to content

Commit feb5856

Browse files
committed
fix(fxa-settings): Update hasPassword in localStorage after createPassword and fix loading state flash
After creating a password via createPassword/createPasswordWithJwt, localStorage was not updated with hasPassword: true, causing the app to still show the "set password" flow and resulting in errno 206 on retry. The loading/error state in useAccountData was routed through localStorage where transient fields are stripped, so isLoading was always false. This caused a flash of default values (security features shown as disabled) before the data fetch completed. Switched to local React state for loading/error tracking.
1 parent 13621c4 commit feb5856

5 files changed

Lines changed: 151 additions & 35 deletions

File tree

packages/fxa-settings/src/components/Settings/index.test.tsx

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import AppLocalizationProvider from 'fxa-react/lib/AppLocalizationProvider';
1919
import { Subject } from './mocks';
2020

2121
const mockSessionStatus = jest.fn();
22+
const mockUseAccountData = jest.fn();
2223
const mockAccountState = {
2324
isLoading: false,
2425
error: null,
@@ -61,6 +62,11 @@ jest.mock('../../models/contexts/AccountStateContext', () => ({
6162
useAccountState: jest.fn(),
6263
}));
6364

65+
jest.mock('../../lib/hooks/useAccountData', () => ({
66+
...jest.requireActual('../../lib/hooks/useAccountData'),
67+
useAccountData: (...args: any[]) => mockUseAccountData(...args),
68+
}));
69+
6470
jest.mock('../../lib/totp-utils', () => {
6571
const mockBackupCodes = ['0123456789'];
6672
return {
@@ -115,6 +121,13 @@ describe('Settings App', () => {
115121
jest.spyOn(console, 'error').mockImplementation(() => {});
116122
// Reset account state mock to default values
117123
(useAccountState as jest.Mock).mockReturnValue({ ...mockAccountState, isLoading: false, error: null });
124+
mockUseAccountData.mockReturnValue({
125+
isLoading: false,
126+
error: null,
127+
data: mockAccountState,
128+
refetch: jest.fn(),
129+
refetchField: jest.fn(),
130+
});
118131
mockNavigate.mockReset();
119132
mockSessionStatus.mockResolvedValue({
120133
details: {
@@ -131,9 +144,12 @@ describe('Settings App', () => {
131144
});
132145

133146
it('renders `LoadingSpinner` component when loading initial state is true', () => {
134-
(useAccountState as jest.Mock).mockReturnValueOnce({
135-
...mockAccountState,
147+
mockUseAccountData.mockReturnValue({
136148
isLoading: true,
149+
error: null,
150+
data: mockAccountState,
151+
refetch: jest.fn(),
152+
refetchField: jest.fn(),
137153
});
138154
const { getByLabelText } = renderWithRouter(
139155
<AppContext.Provider value={mockAppContext()}>
@@ -145,11 +161,13 @@ describe('Settings App', () => {
145161
});
146162

147163
it('renders `AppErrorDialog` component when settings query errors', async () => {
148-
(useAccountState as jest.Mock).mockImplementation(() => ({
149-
...mockAccountState,
164+
mockUseAccountData.mockReturnValue({
150165
isLoading: false,
151166
error: new Error('Error'),
152-
}));
167+
data: mockAccountState,
168+
refetch: jest.fn(),
169+
refetchField: jest.fn(),
170+
});
153171
const { getByTestId } = renderWithRouter(
154172
<AppContext.Provider value={mockAppContext()}>
155173
<Subject />
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
4+
5+
import { renderHook, act } from '@testing-library/react';
6+
import { useAccountData } from './useAccountData';
7+
import { sessionToken as getSessionToken } from '../cache';
8+
9+
const mockSetAccountData = jest.fn();
10+
11+
jest.mock('../cache', () => ({ sessionToken: jest.fn() }));
12+
jest.mock('../../models/contexts/AccountStateContext', () => ({
13+
useAccountState: () => ({ setAccountData: mockSetAccountData }),
14+
}));
15+
jest.mock('../config', () => ({
16+
oauth: { clientId: 'test' },
17+
servers: { profile: { url: 'http://localhost' } },
18+
}));
19+
jest.mock('@sentry/browser', () => ({ captureMessage: jest.fn() }));
20+
21+
const authClient = {
22+
account: jest.fn(),
23+
attachedClients: jest.fn(),
24+
createOAuthToken: jest.fn(),
25+
} as any;
26+
27+
describe('useAccountData', () => {
28+
beforeEach(() => jest.clearAllMocks());
29+
30+
it('starts with isLoading=true', () => {
31+
(getSessionToken as jest.Mock).mockReturnValue('tok');
32+
authClient.account.mockReturnValue(new Promise(() => {}));
33+
authClient.attachedClients.mockReturnValue(new Promise(() => {}));
34+
authClient.createOAuthToken.mockReturnValue(new Promise(() => {}));
35+
36+
const { result } = renderHook(() => useAccountData({ authClient }));
37+
expect(result.current.isLoading).toBe(true);
38+
});
39+
40+
it('sets error when no session token', async () => {
41+
(getSessionToken as jest.Mock).mockReturnValue(null);
42+
let result: any;
43+
await act(async () => {
44+
({ result } = renderHook(() => useAccountData({ authClient })));
45+
});
46+
expect(result.current.isLoading).toBe(false);
47+
expect(result.current.error?.message).toBe('No session token available');
48+
});
49+
});

packages/fxa-settings/src/lib/hooks/useAccountData.ts

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
import { useCallback, useEffect, useRef } from 'react';
5+
import { useCallback, useEffect, useRef, useState } from 'react';
66
import AuthClient from 'fxa-auth-client/browser';
77
import { sessionToken as getSessionToken } from '../cache';
88
import {
@@ -185,37 +185,40 @@ export function useAccountData({
185185
authClient,
186186
onError,
187187
}: UseAccountDataOptions): AccountDataResult {
188-
// Ref prevents infinite re-render from unstable onError callback
188+
// Refs prevent infinite re-renders from unstable callback/object references
189189
const onErrorRef = useRef(onError);
190190
onErrorRef.current = onError;
191+
const authClientRef = useRef(authClient);
192+
authClientRef.current = authClient;
191193

192194
const accountState = useAccountState();
193195
const {
194196
setAccountData,
195-
setLoading,
196-
setError,
197-
isLoading,
198-
error,
199197
...stateData
200198
} = accountState;
201199

200+
const [localLoading, setLocalLoading] = useState(true);
201+
const [localError, setLocalError] = useState<Error | null>(null);
202+
202203
const fetchAccountData = useCallback(async () => {
204+
const client = authClientRef.current;
203205
const token = getSessionToken();
204206
if (!token) {
205-
setError(new Error('No session token available'));
207+
setLocalError(new Error('No session token available'));
208+
setLocalLoading(false);
206209
return;
207210
}
208211

209-
setLoading(true);
210-
setError(null);
212+
setLocalLoading(true);
213+
setLocalError(null);
211214

212215
try {
213216
// allSettled (not .all) so a single failure doesn't discard other results
214217
const [accountResult, profileResult, attachedClientsResult] =
215218
await Promise.allSettled([
216-
authClient.account(token),
217-
fetchProfileData(authClient, token),
218-
authClient.attachedClients(token),
219+
client.account(token),
220+
fetchProfileData(client, token),
221+
client.attachedClients(token),
219222
]);
220223

221224
// Check for invalid token errors - user needs to sign in again
@@ -238,28 +241,29 @@ export function useAccountData({
238241
if (displayName !== null) accountData.displayName = displayName;
239242
if (avatar !== null) accountData.avatar = avatar;
240243
} else {
241-
Sentry.captureMessage(`Failed to fetch profile: ${profileResult.reason}`);
244+
Sentry.captureMessage(`Failed to fetch profile: ${profileResult.reason}`);
242245
}
243246

244247
if (attachedClientsResult.status === 'fulfilled') {
245248
accountData.attachedClients = attachedClientsResult.value.map(mapAttachedClient);
246249
} else {
247-
Sentry.captureMessage(`Failed to fetch attached clients: ${attachedClientsResult.reason}`);
250+
Sentry.captureMessage(`Failed to fetch attached clients: ${attachedClientsResult.reason}`);
248251
accountData.attachedClients = [];
249252
}
250253

251254
setAccountData(accountData);
252255
} catch (err) {
253256
const error = err instanceof Error ? err : new Error('Unknown error');
254-
setError(error);
257+
setLocalError(error);
255258
onErrorRef.current?.(error);
256259
} finally {
257-
setLoading(false);
260+
setLocalLoading(false);
258261
}
259-
}, [authClient, setAccountData, setLoading, setError]);
262+
}, [setAccountData]);
260263

261264
const refetchField = useCallback(
262265
async (field: keyof AccountState) => {
266+
const client = authClientRef.current;
263267
const token = getSessionToken();
264268
if (!token) return;
265269

@@ -268,19 +272,19 @@ export function useAccountData({
268272

269273
switch (field) {
270274
case 'attachedClients': {
271-
const clients = await authClient.attachedClients(token);
275+
const clients = await client.attachedClients(token);
272276
fieldData.attachedClients = clients.map(mapAttachedClient);
273277
break;
274278
}
275279
case 'displayName':
276280
case 'avatar': {
277-
const { displayName, avatar } = await fetchProfileData(authClient, token);
281+
const { displayName, avatar } = await fetchProfileData(client, token);
278282
if (displayName !== null) fieldData.displayName = displayName;
279283
if (avatar !== null) fieldData.avatar = avatar;
280284
break;
281285
}
282286
default: {
283-
const account = await authClient.account(token);
287+
const account = await client.account(token);
284288
fieldData = { ...fieldData, ...transformAccountResponse(account) };
285289
break;
286290
}
@@ -291,17 +295,17 @@ export function useAccountData({
291295
console.error(`Failed to refetch ${field}:`, err);
292296
}
293297
},
294-
[authClient, setAccountData]
298+
[setAccountData]
295299
);
296300

297301
useEffect(() => {
298302
fetchAccountData();
299303
}, [fetchAccountData]);
300304

301305
return {
302-
data: { ...stateData, isLoading, error } as AccountState,
303-
isLoading,
304-
error,
306+
data: { ...stateData, isLoading: localLoading, error: localError } as AccountState,
307+
isLoading: localLoading,
308+
error: localError,
305309
refetch: fetchAccountData,
306310
refetchField,
307311
};

packages/fxa-settings/src/models/Account.test.ts

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,23 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
import { getNextAvatar } from './Account';
5+
import { Account, getNextAvatar } from './Account';
6+
import { sessionToken, JwtTokenCache } from '../lib/cache';
7+
import { getFullAccountData, updateExtendedAccountState } from '../lib/account-storage';
68

79
jest.mock('../lib/config', () => ({
8-
servers: {
9-
profile: {
10-
url: 'default-url',
11-
},
12-
},
10+
servers: { profile: { url: 'default-url' } },
11+
oauth: { clientId: 'test' },
12+
}));
13+
jest.mock('../lib/cache', () => ({
14+
sessionToken: jest.fn(),
15+
JwtTokenCache: { hasToken: jest.fn(() => true), getToken: jest.fn() },
16+
isSigningOut: jest.fn(() => false),
17+
}));
18+
jest.mock('../lib/account-storage', () => ({
19+
getFullAccountData: jest.fn(),
20+
updateExtendedAccountState: jest.fn(),
21+
updateBasicAccountData: jest.fn(),
1322
}));
1423

1524
describe('Account', () => {
@@ -54,4 +63,38 @@ describe('Account', () => {
5463
expect(avatar.url).toBe(null);
5564
});
5665
});
66+
67+
describe('createPassword', () => {
68+
let account: Account;
69+
const authClient: any = {
70+
createPassword: jest.fn().mockResolvedValue({ passwordCreated: 123 }),
71+
createPasswordWithJwt: jest.fn().mockResolvedValue({ passwordCreated: 123 }),
72+
};
73+
74+
beforeEach(() => {
75+
jest.clearAllMocks();
76+
(getFullAccountData as jest.Mock).mockReturnValue({
77+
uid: 'abc',
78+
emails: [{ email: '[email protected]', isPrimary: true, verified: true }],
79+
primaryEmail: { email: '[email protected]', isPrimary: true, verified: true },
80+
});
81+
(sessionToken as jest.Mock).mockReturnValue('tok');
82+
(JwtTokenCache.getToken as jest.Mock).mockReturnValue('jwt');
83+
account = new Account(authClient);
84+
});
85+
86+
it('sets hasPassword after createPassword', async () => {
87+
await account.createPassword('pw');
88+
expect(updateExtendedAccountState).toHaveBeenCalledWith(
89+
expect.objectContaining({ hasPassword: true })
90+
);
91+
});
92+
93+
it('sets hasPassword after createPasswordWithJwt', async () => {
94+
await account.createPasswordWithJwt('pw');
95+
expect(updateExtendedAccountState).toHaveBeenCalledWith(
96+
expect.objectContaining({ hasPassword: true })
97+
);
98+
});
99+
});
57100
});

packages/fxa-settings/src/models/Account.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -662,6 +662,7 @@ export class Account implements AccountData {
662662

663663
updateExtendedAccountState({
664664
passwordCreated: passwordCreatedResult.passwordCreated,
665+
hasPassword: true,
665666
});
666667
}
667668

@@ -677,6 +678,7 @@ export class Account implements AccountData {
677678

678679
updateExtendedAccountState({
679680
passwordCreated: passwordCreatedResult.passwordCreated,
681+
hasPassword: true,
680682
});
681683
}
682684

0 commit comments

Comments
 (0)