[HOLD Web-Expensify#52112] Replace individual private detail edit pages with unified form + magic code#87928
[HOLD Web-Expensify#52112] Replace individual private detail edit pages with unified form + magic code#87928jasperhuangg wants to merge 19 commits intomainfrom
Conversation
Adds the API plumbing for the new unified personal details update: - UPDATE_PRIVATE_PERSONAL_DETAILS write command - UpdatePrivatePersonalDetailsParams type (all personal detail fields + validateCode) - updatePrivatePersonalDetails() action with optimistic/success/failure data that sets isLoading on PRIVATE_PERSONAL_DETAILS Part of Expensify/Expensify#623849. Made-with: Cursor
Registers two new screens in the Settings RHP: - PRIVATE_PERSONAL_DETAILS: unified edit form for all private fields - PRIVATE_PERSONAL_DETAILS_CONFIRM_MAGIC_CODE: magic code verification Adds routes, screen constants, navigation param types, linking config, and Settings-to-RHP relations. Part of Expensify/Expensify#623849. Made-with: Cursor
PrivatePersonalDetailsPage: single form with all private fields (legal name, DOB, phone, address). Pre-populates from PRIVATE_PERSONAL_DETAILS, validates that previously-set fields cannot be emptied, and navigates to magic code confirmation when any field changes. PrivatePersonalDetailsConfirmMagicCodePage: uses ValidateCodeActionContent to collect a magic code, then calls updatePrivatePersonalDetails() to submit all changes in a single validate-code-protected API call. Part of Expensify/Expensify#623849. Made-with: Cursor
All four private detail rows (legal name, DOB, phone, address) now navigate to the unified PrivatePersonalDetailsPage instead of individual edit pages. The individual pages are kept for backward compatibility with deep links. Part of Expensify/Expensify#623849. Made-with: Cursor
melvin-bot
Bot
requested review from
DylanDylann
and removed request for
a team
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
Bot
requested review from
trjExpensify
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a14d480bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const handleSubmitForm = useCallback( | ||
| (validateCode: string) => { | ||
| updatePrivatePersonalDetails(values, validateCode, countryCode); | ||
| }, |
There was a problem hiding this comment.
After handleSubmitForm calls updatePrivatePersonalDetails, this page never observes a successful update to close itself or return to the form/profile. That leaves users on the magic-code screen after a valid code (and offline submits can appear stuck while isLoading remains true), unlike other validate-code flows that navigate away when the action succeeds.
Useful? React with 👍 / 👎.
| // Country: required if previously set | ||
| const countryValue = values[INPUT_IDS.COUNTRY] ?? ''; | ||
| if (country && !countryValue) { | ||
| errors[INPUT_IDS.COUNTRY] = translate('common.error.fieldRequired'); | ||
| } |
There was a problem hiding this comment.
The new validator only enforces required checks for street, city, and country, but not state or zipPostCode. For users who already have a populated address, clearing state or ZIP still passes client validation and gets submitted in the consolidated update call, which can silently erase address data or force a server-side failure after code verification.
Useful? React with 👍 / 👎.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…vatePersonalDetails
Convert SETTINGS_PRIVATE_PERSONAL_DETAILS from a plain string to an object with getRoute(fieldToFocus?) so callers can specify which field to auto-focus when the form opens. Update the linking config and navigation types accordingly. Made-with: Cursor
Each private detail row in ProfilePage now passes its corresponding INPUT_ID so the form auto-focuses the tapped field: legal name, DOB, phone number, or address. Made-with: Cursor
Read the fieldToFocus query param and set autoFocus on the matching InputWrapper. For the DatePicker (DOB), autoFocus triggers the calendar modal to open immediately. Made-with: Cursor
After the private personal details update succeeds (isLoading transitions from true to false with no errors), navigate back to the Profile settings page instead of the intermediate form. Made-with: Cursor
When the user edits fields but navigates away without submitting, the draft values were persisted via shouldSaveDraft. Now we call clearDraftValues on unmount so the form always resets to the current saved values when reopened. Made-with: Cursor
Check both privatePersonalDetails.errors and validateCodeAction.errorFields before navigating back to Profile. Invalid magic code errors are stored on the validateCodeAction key, not privatePersonalDetails. Made-with: Cursor
The server sets validate code errors on PRIVATE_PERSONAL_DETAILS.errorFields.personalDetails via Onyx::handleException, but the component was reading from .errors which is never populated. Use getLatestErrorField to read from the correct path so errors are displayed and the success navigation doesn't fire on failure. Made-with: Cursor
jasperhuangg
changed the title
|
@MelvinBot pull main on this PR |
…ersonalDetails Co-authored-by: Jasper Huang <jasperhuangg@users.noreply.github.com>
|
Merged main into this branch. ✅ |
- Add 'Basic details' and 'Address' section headers with sectionTitle style - Replace Country text input with CountrySelector picker component - Add basicDetails translation key to all language files Made-with: Cursor
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/fr.ts b/src/languages/fr.ts
index f115885e..50bf5895 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -3261,7 +3261,7 @@ ${amount} pour ${merchant} - ${date}`,
enterPhoneNumber: 'Quel est votre numéro de téléphone ?',
personalDetails: 'Informations personnelles',
privateDataMessage: 'Ces informations sont utilisées pour les déplacements et les paiements. Elles ne s’affichent jamais sur votre profil public.',
- basicDetails: 'Informations de base',
+ basicDetails: 'Détails de base',
legalName: 'Nom légal',
legalFirstName: 'Prénom légal',
legalLastName: 'Nom de famille légal',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| [], | ||
| ); | ||
|
|
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled in this codebase and automatically memoizes closures based on their captured variables. The useCallback wrapper around validate is redundant and adds maintenance overhead with the manual dependency array. Similar existing pages in this directory (e.g., PersonalAddressPage.tsx) compile successfully with React Compiler.
Remove useCallback and define validate as a plain function:
const validate = (values: FormOnyxValues<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> => {
// ... validation logic unchanged
};Apply the same change to hasChanges (line 135) and onSubmit (line 172) in this file.
Reviewed at: f486a7b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const legalFirstName = privatePersonalDetails?.legalFirstName ?? ''; | ||
| const legalLastName = privatePersonalDetails?.legalLastName ?? ''; | ||
| const phoneNumber = privatePersonalDetails?.phoneNumber ?? ''; | ||
| const dob = privatePersonalDetails?.dob ?? ''; |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and automatically caches derived values. The useMemo wrapper around the address computation is redundant.
Remove useMemo and compute address directly:
const address = normalizeCountryCode(getCurrentAddress(privatePersonalDetails)) as Address | undefined;Reviewed at: f486a7b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| if (wasLoading.current && !hasErrors) { | ||
| wasLoading.current = false; | ||
| resetValidateActionCodeSent(); | ||
| Navigation.goBack(ROUTES.SETTINGS_PROFILE.route); |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and automatically caches derived values and closures. The useMemo on line 64 and useCallback on line 66 are redundant.
Remove both manual memoization wrappers:
const values = normalizeCountryCode(getFormValues(privatePersonalDetails, draftValues)) as PersonalDetailsForm;
const handleSubmitForm = (validateCode: string) => {
updatePrivatePersonalDetails(values, validateCode, countryCode);
};Reviewed at: f486a7b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| ); | ||
|
|
||
| const reasonAttributes: SkeletonSpanReasonAttributes = {context: 'PrivatePersonalDetailsPage', isLoadingApp}; | ||
|
|
There was a problem hiding this comment.
❌ UI-1 (docs)
FullScreenLoadingIndicator is used as an early return when isLoadingApp is true. In this code path, no navigation component (e.g., HeaderWithBackButton) is rendered, so the user has no way to escape if loading hangs.
Add shouldUseGoBackButton to provide an emergency escape route:
return <FullScreenLoadingIndicator reasonAttributes={reasonAttributes} shouldUseGoBackButton />;Reviewed at: f486a7b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| import type {PersonalDetailsForm} from '@src/types/form'; | ||
| import INPUT_IDS from '@src/types/form/PersonalDetailsForm'; | ||
| import {isEmptyObject} from '@src/types/utils/EmptyObject'; | ||
|
|
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The getFormValues helper (lines 18-30) duplicates address extraction logic that also appears in PrivatePersonalDetailsPage.tsx (lines 42-53). Both call getCurrentAddress(privatePersonalDetails), split street by \n, and extract the same set of fields (legalFirstName, legalLastName, dob, phoneNumber, address fields) with the same fallback patterns.
Extract a shared utility function (e.g., in PersonalDetailsUtils.ts) that converts privatePersonalDetails into a PersonalDetailsForm shape, and use it from both pages. The confirm page can then merge draft overrides on top of the shared result.
Reviewed at: f486a7b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f486a7b649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <InputWrapper | ||
| InputComponent={CountrySelector} | ||
| inputID={INPUT_IDS.COUNTRY} | ||
| value={country} |
There was a problem hiding this comment.
This field is wired as a controlled input with value={country} from the initial Onyx snapshot, but there is no onValueChange updating that source. In FormProvider, any defined value is reapplied to form state on each render, so user selections get overwritten and the country cannot actually be changed from this unified form. Use a form-managed default/uncontrolled value or update a local state value in onValueChange before passing it back.
Useful? React with 👍 / 👎.
| const primaryLogin = usePrimaryContactMethod(); | ||
|
|
||
| const [validateCodeAction] = useOnyx(ONYXKEYS.VALIDATE_ACTION_CODE); | ||
| const personalDetailsError = getLatestErrorField(privatePersonalDetails, 'personalDetails'); |
There was a problem hiding this comment.
errors before treating update as success
This page reads personalDetailsError from errorFields.personalDetails, but this flow’s helpers and related magic-code flow use PRIVATE_PERSONAL_DETAILS.errors for submission failures. If the backend writes a failure into errors, hasErrors remains false and the success effect can navigate back after loading, hiding the failed update from the user. Check errors (or both sources) when computing submit failure state.
Useful? React with 👍 / 👎.
- Bind CountrySelector to local state with onValueChange so picker selections persist across renders (P1) - Validate state and zip as required when previously set (P2) - Add shouldUseGoBackButton to FullScreenLoadingIndicator so users can escape a hung loading state (UI-1) - Remove redundant useCallback/useMemo wrappers now that React Compiler handles memoization (CLEAN-REACT-PATTERNS-0) - Extract shared getPrivatePersonalDetailsFormValues helper into PersonalDetailsUtils (CONSISTENCY-3) Made-with: Cursor
- Check both errors and errorFields sources to avoid navigating away on failure (P1) - Use shared getPrivatePersonalDetailsFormValues helper instead of duplicated local function (CONSISTENCY-3) - Remove redundant useMemo/useCallback wrappers (CLEAN-REACT-PATTERNS-0) Made-with: Cursor
3 participants
Explanation of Change
Replaces the four individual edit pages for private personal details (legal name, DOB, phone, address) with a single unified form that requires magic code verification when any field changes. This prevents session hijackers from silently modifying sensitive personal information.
New pages:
PrivatePersonalDetailsPage- Unified form showing all private detail fields, pre-populated from Onyx. Validates that previously-set fields aren't emptied. On submit, detects changes and navigates to magic code confirmation.PrivatePersonalDetailsConfirmMagicCodePage- UsesValidateCodeActionContentto collect a magic code, then callsupdatePrivatePersonalDetails()to submit all changes in a single API call.API changes:
UPDATE_PRIVATE_PERSONAL_DETAILSwrite command with optimistic/success/failure dataupdatePrivatePersonalDetails()action inPersonalDetails.tsNavigation changes:
Depends on Auth PR and Web-Expensify PR.
Fixed Issues
part of https://github.com/Expensify/Expensify/issues/623849
PROPOSAL:
Tests
Offline tests
QA Steps
Same as Tests section above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssection/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Made with Cursor