Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes two issues in the FXA settings application: a flash of default state before account loads, and the hasPassword field not being updated in local cache after setting a password.
Changes:
- Modified
useAccountDatahook to use local React state for loading/error instead of persisting to global AccountState context, preventing the flash of default state on page reload - Updated
createPasswordandcreatePasswordWithJwtmethods to sethasPassword: truein the account state after creating a password - Refactored
useAccountDatato use refs forauthClientto prevent unnecessary re-renders and dependency issues
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/fxa-settings/src/models/Account.ts | Added hasPassword: true to state updates in createPassword and createPasswordWithJwt methods |
| packages/fxa-settings/src/lib/hooks/useAccountData.ts | Refactored to use local state for loading/error, added authClient ref, updated dependency arrays, and removed trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const accountState = useAccountState(); | ||
| const { | ||
| setAccountData, |
There was a problem hiding this comment.
The destructuring here only removes setAccountData from accountState, but accountState also contains other action functions like updateField, setLoading, setFieldLoading, setError, and clearAccount from the AccountStateActions interface. These action functions will be included in stateData and then spread into the returned data object (line 306), even though the return type AccountState doesn't include them.
Consider destructuring all action functions to keep only the state data, similar to how the old code explicitly removed setLoading and setError. For example:
const {
setAccountData,
updateField,
setLoading,
setFieldLoading,
setError,
clearAccount,
...stateData
} = accountState;This would improve type safety and make the code more explicit about what's being included in the returned data.
| setAccountData, | |
| setAccountData, | |
| updateField, | |
| setLoading, | |
| setFieldLoading, | |
| setError, | |
| clearAccount, |
0cf4e6c to
0d5bcaa
Compare
…sword 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.
nshirley
left a comment
There was a problem hiding this comment.
I was able to test locally and it appears to be working - no delay to show the password field when adding one after unlinking a 3rd party auth account
Because
This pull request
Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13107
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13109
Checklist
Other information (Optional)
Its kinda hard to reproduce settings refresh issue locally (I wasn't actually able too), but this seems to be reasonable to me what might be going on.