Plan selector modal migration#96745
Conversation
|
Linked sub-issue for this migration: #96756 |
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.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
![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: 820ed95261
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| WORKSPACE_CONFIRMATION_PLAN_TYPE: { | ||
| path: 'plan-type', | ||
| entryScreens: [SCREENS.WORKSPACE_CONFIRMATION.DYNAMIC_ROOT], |
There was a problem hiding this comment.
When an approved accountant opens this shared WorkspaceConfirmationForm from the travel workspace confirmation or the IOU upgrade flow, the plan row now calls createDynamicRoute(...plan-type) from SCREENS.TRAVEL.WORKSPACE_CONFIRMATION or SCREENS.MONEY_REQUEST.STEP_UPGRADE; getStateFromPath validates dynamic routes against entryScreens, so limiting this suffix to only SCREENS.WORKSPACE_CONFIRMATION.DYNAMIC_ROOT rejects those URLs and the selector won't open in those existing flows. Include the other screens that render the form, or keep this selector local for non-workspace-confirmation hosts.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const backPath = useDynamicBackPath(DYNAMIC_ROUTES.WORKSPACE_CONFIRMATION_PLAN_TYPE.path); | ||
|
|
||
| const [workspaceConfirmationFormDraft] = useOnyx(ONYXKEYS.FORMS.WORKSPACE_CONFIRMATION_FORM_DRAFT); | ||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
This subscribes to the entire ONYXKEYS.COLLECTION.POLICY collection without a selector, but the only use of policies is to derive a single scalar (getDefaultWorkspacePlanType(policies) returns a PolicyType). Without a selector Onyx will re-render this page whenever any field of any policy changes, even though the page only cares about whether the user belongs to a Control workspace.
Use a selector that reduces the collection to the primitive the page actually needs, so deepEqual runs on a tiny value and unrelated policy updates don't trigger re-renders:
const [defaultPlanType] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {
selector: (policies) =>
Object.values(policies ?? {}).some((policy) => policy?.type === CONST.POLICY.TYPE.CORPORATE)
? CONST.POLICY.TYPE.CORPORATE
: CONST.POLICY.TYPE.TEAM,
});
const planType = workspaceConfirmationFormDraft?.planType ?? defaultPlanType;Reviewed at: 820ed95 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| isSelected: policyType === planType, | ||
| })); | ||
|
|
||
| const handleSelect = (option: PlanTypeItem) => { |
There was a problem hiding this comment.
❌ CONSISTENCY-12 (docs)
The handler handleSelect is named for the event slot it is wired to (onSelectRow) rather than for the action it performs. It saves the chosen plan type as a draft value and navigates back — its name should describe that behavior. The on*/handle* prefix belongs on the JSX prop, not the function definition.
const savePlanType = (option: PlanTypeItem) => {
setDraftValues(ONYXKEYS.FORMS.WORKSPACE_CONFIRMATION_FORM, {[INPUT_IDS.PLAN_TYPE]: option.value});
Navigation.goBack(backPath, {shouldSkipFocusRestore: true});
};
// ...
<SelectionList onSelectRow={savePlanType} ... />Reviewed at: 820ed95 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| // uses to keep the initially selected row in sync with what the form shows. | ||
| const planType = workspaceConfirmationFormDraft?.planType ?? getDefaultWorkspacePlanType(policies); | ||
|
|
||
| const handleBackPress = () => { |
There was a problem hiding this comment.
❌ CONSISTENCY-12 (docs)
The handler handleBackPress is named for the triggering event (back press) rather than the action it performs (navigating back). Name it for the behavior and keep the on* prefix on the JSX prop only.
const goBack = () => {
Navigation.goBack(backPath);
};
// ...
<HeaderWithBackButton title={translate('workspace.common.planType')} onBackButtonPress={goBack} />Reviewed at: 820ed95 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
2 participants
Explanation of Change
Migrates the workspace plan type selector in the workspace confirmation flow from an inline popover component (
PlanTypeSelector) to a dedicated navigation page (DynamicWorkspacePlanTypeSelectionPage) registered as a dynamic route in the RHP.The selection is now persisted via the form draft in Onyx, and the shared default-plan logic was extracted to
getDefaultWorkspacePlanTypeinPolicyUtils.Fixed Issues
$ #96756
PROPOSAL:
Tests
Pre requisite:
User needs to be an approved accountant or to test locally w/o this paste following snippet in console:
This will result in workspace creation failing though.
+ NewPlaninput is visibleOffline tests
Same as tests.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-07-23.at.15.06.41.mov
Android: mWeb Chrome
Screen.Recording.2026-07-23.at.15.07.46.mov
iOS: Native
Screen.Recording.2026-07-23.at.15.01.27.mov
iOS: mWeb Safari
Screen.Recording.2026-07-23.at.15.03.08.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-23.at.14.49.33.mov