Replace work email input with button linking to Contact Methods in card feed flows - #95610
Replace work email input with button linking to Contact Methods in card feed flows#95610MelvinBot wants to merge 22 commits into
Conversation
…rd feed flows Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…heck Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Fixed the failing Compare knip issues against main check. Cause: This PR removed the inline email form from both card‑feed Add work email screens, which left Fix: Removed the now-unused The only other failing check is
|
|
✅ The Compare knip issues against main check now passes after my previous fix (removed the unused The only remaining failing check is PR Author Checklist / checklist — that's a human-only checklist, not a code/CI failure. There's nothing for me to push here; it clears once the checklist items in the PR description are completed by the author. |
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Fixed the failing typecheck check. Cause: The PR added the new Fix: Added The knip check from the earlier fix now passes. The only other failing check is
|
|
✅ The knip fix I pushed earlier worked — Compare knip issues against main now passes. The only remaining failing check is All other checks are passing or still running. |
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Fixed the failing ESLint check (3 errors). The only other failing check is What was wrong & fixed:
Details
|
|
✅ The Compare knip issues against main check I fixed earlier is now passing. The only remaining failing check is PR Author Checklist / checklist. This is not a code issue — it fails because the checklist items in the PR body (Tests / QA / self-review boxes) haven't been completed. Those are human-only sections that I intentionally don't fill out on your behalf. To clear it: fill out the No further code fix is needed from me. |
|
@MelvinBot can you merge main? |
…kEmailButton Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
✅ Merged The merge was clean — no conflicts. Merge commit: Note: I wasn't able to verify anything in the browser this run (web environment unavailable), so this only confirms the git merge itself is clean, not runtime behavior. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@gijoe0295 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] |
![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: 36026fba14
ℹ️ 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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Is this a blocker? Otherwise can I review it tomorrow? Thanks |
|
Not quite — two checks are red. Both are fixable and neither is a problem with the actual change in this PR. 1. ESLint check — 4 errors ( They're all in company-card files this PR doesn't intentionally touch:
These Fix: merge the latest 2. Bun tests — flaky timeout Only Also: the QA Steps section is still the Once ESLint is green and Bun passes, it's ready for review. |
| variant="success" | ||
| size={CONST.BUTTON_SIZE.LARGE} | ||
| // After the user adds their work email, the back button should take them to the feed selector to pick a feed again, not back to this now-stale prompt page. | ||
| onPress={() => Navigation.navigate(ROUTES.SETTINGS_CONTACT_METHODS.getRoute(ROUTES.WORKSPACE_COMPANY_CARDS_SELECT_FEED.getRoute(policyID)))} |
There was a problem hiding this comment.
@MelvinBot Contact methods route was refactored to dynamic routes DYNAMIC_ROUTES.CONTACT_METHODS.
Also DynamicContactMethodsPage doesn't respect the backTo param:
Please fix.
There was a problem hiding this comment.
No change needed here: both points landed in later commits on this branch.
- The button now builds the URL with the dynamic route:
WorkspaceCompanyCardAddWorkEmailPage.tsx:56→createDynamicRoute(DYNAMIC_ROUTES.CONTACT_METHODS.path, ROUTES.WORKSPACE_COMPANY_CARDS_SELECT_FEED.getRoute(policyID)), and the same inWorkspaceExpensifyCardAddWorkEmailPage.tsx:57-64. DynamicContactMethodsPageno longer hardcodes its back target — it derives it from the dynamic back path atDynamicContactMethodsPage.tsx:42-48viauseDynamicBackPath, mirroringDynamicContactMethodDetailsPage.
-- AI Maxence
|
Good catches — both are real. Here's the concrete fix for each. 1. Navigate via the dynamic route, not Contact methods is now the dynamic route
( 2. Make
const backTo = useDynamicBackPath(DYNAMIC_ROUTES.CONTACT_METHODS.path);
// …
onBackButtonPress={() => Navigation.goBack(backTo)}
Note: this touches I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author ( |
|
@MelvinBot can you make these changes? #95610 (comment) |
…kEmailButton Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
…ions The branch was behind main, which narrowed getCardFeedWithDomainID's return type so the 'as CompanyCardFeedWithDomainID' casts became redundant. On the CI merge ref these tripped @typescript-eslint/no-unnecessary-type-assertion. Remove the four casts (and the now-unused import in CardInstructionsStep) to match main. Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Pushed a fix for the failing ESLint check. The other failing checks are the checklist bots (ignored) and a flaky Bun test (details below). Root cause (ESLint): This branch had fallen behind
Fix: Removed the four redundant casts to match Other failing checks
|
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
…n' into claude-cardFeedAddWorkEmailButton Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Done — merged 1. Navigate via the dynamic route (
2. Merge / checks: merging Worth a manual check before merge (I couldn't drive the full flow — it needs a public-domain account with a mock feed across two workspaces, and this change touches a shared page):
|
|
✅ All the real checks are green now — nothing left to fix here.
The only remaining red check is Check independent approval, which isn't a code issue — it just needs a human reviewer's approval. |
|
Alright @gijoe0295 ready again |
|
@MelvinBot The changes didn't work. After adding the new contact Screen.Recording.2026-08-26.at.23.29.46.mov |
|
Thanks for the detailed repro Root causeThe bug is in the dynamic-routes machinery, not in the copy/button change. Both card‑feed screens launch Contact Methods with a
Going through the add‑contact‑method flow (which
Recommended fixThe clean fix is to stop mounting the Contact Methods dynamic chain over a foreign workspace RHP base. Options, most contained first:
Notes & confidence
I haven't pushed a change: I can only make code changes when directed by the PR author, an assignee, or an existing co-author. |
|
Hi @madmax330 can you guide Melvin to continue the work here? The fix didn't work. |
|
Yeah I will work with Claude on it locally |
|
No code fix needed — every real check is green on The two checks that were red on earlier runs of this branch are now passing on your commit:
Why nothing was pushedWhen I looked, The only failing job is the Separately: your view run · no recording available |
|
Ok @gijoe0295 it's working now |
|
Nothing to fix — every real check is green on What I watched, and why it took a whileHEAD moved twice while I was waiting, so CI restarted from scratch each time:
On the final HEAD I waited for the full suite rather than reporting early. All of these came back green: ESLint, typecheck, Bun tests, Jest (jobs 1–8), Knip, Storybook, React Compiler Compliance, unused styles, spellcheck, generateTranslations, baseline/branch perf tests, and all four native builds. The one red job is Check independent approval. It gates on a reviewer approving independently of the PR's authors — no branch change affects it. On the actual bug: your view run · no recording available |
|
This concern is not resolved yet #95610 (comment). Navigating back from contact methods page still resolves to Screen.Recording.2026-09-08.at.10.33.32.movI think we might need to add an effect inside Diffdiff --git a/src/pages/workspace/companyCards/WorkspaceCompanyCardAddWorkEmailPage.tsx b/src/pages/workspace/companyCards/WorkspaceCompanyCardAddWorkEmailPage.tsx
index 45cf9e0a273..8c5dead6149 100644
--- a/src/pages/workspace/companyCards/WorkspaceCompanyCardAddWorkEmailPage.tsx
+++ b/src/pages/workspace/companyCards/WorkspaceCompanyCardAddWorkEmailPage.tsx
@@ -5,21 +5,27 @@ import ScreenWrapper from '@components/ScreenWrapper';
import Text from '@components/Text';
import useLocalize from '@hooks/useLocalize';
+import useOnyx from '@hooks/useOnyx';
+import usePrimaryContactMethod from '@hooks/usePrimaryContactMethod';
import useThemeStyles from '@hooks/useThemeStyles';
+import { isEmailPublicDomain } from '@libs/LoginUtils';
import createDynamicRoute from '@libs/Navigation/helpers/dynamicRoutesUtils/createDynamicRoute';
import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types';
+import { expensifyLoginsSelector } from '@libs/UserUtils';
import Navigation from '@navigation/Navigation';
import type {SettingsNavigatorParamList} from '@navigation/types';
import AccessOrNotFoundWrapper from '@pages/workspace/AccessOrNotFoundWrapper';
+import { useFocusEffect } from '@react-navigation/core';
import CONST from '@src/CONST';
+import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES, {DYNAMIC_ROUTES} from '@src/ROUTES';
import type SCREENS from '@src/SCREENS';
-import React from 'react';
+import React, { useCallback } from 'react';
import {View} from 'react-native';
type WorkspaceCompanyCardAddWorkEmailPageProps = PlatformStackScreenProps<SettingsNavigatorParamList, typeof SCREENS.WORKSPACE.COMPANY_CARD_ADD_WORK_EMAIL>;
@@ -28,6 +34,23 @@ function WorkspaceCompanyCardAddWorkEmailPage({route}: WorkspaceCompanyCardAddWo
const {policyID} = route.params;
const {translate} = useLocalize();
const styles = useThemeStyles();
+ const primaryContactMethod = usePrimaryContactMethod();
+ const [loginList] = useOnyx(ONYXKEYS.LOGINS, {selector: expensifyLoginsSelector});
+
+ const isUserFromPublicDomain = isEmailPublicDomain(primaryContactMethod);
+
+ useFocusEffect(useCallback(() => {
+ if (isUserFromPublicDomain) {
+ return;
+ }
+ const primaryLoginKey = primaryContactMethod ? Object.keys(loginList ?? {}).find((login) => login.toLowerCase() === primaryContactMethod.toLowerCase()) : undefined;
+ const isPrimaryContactValidated = primaryLoginKey ? !!loginList?.[primaryLoginKey]?.validatedDate : !primaryContactMethod;
+ if (!isPrimaryContactValidated) {
+ return;
+ }
+
+ Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.WORKSPACE_COMPANY_CARDS_ADD_NEW.path, ROUTES.WORKSPACE_COMPANY_CARDS_SELECT_FEED.getRoute(policyID)), {forceReplace: true});
+ }, [isUserFromPublicDomain, primaryContactMethod, loginList, policyID]));
return (
<AccessOrNotFoundWrapperPOC: Screen.Recording.2026-09-08.at.11.58.20.movWe might still need further polishes. |
| }} | ||
| onClose={() => { | ||
| Navigation.goBack(listPath); | ||
| // Contact Methods is already underneath this screen, so pop it instead of resolving its dynamic route again. |
There was a problem hiding this comment.
| // Contact Methods is already underneath this screen, so pop it instead of resolving its dynamic route again. |
There was a problem hiding this comment.
Fixed in e62cd8f — comment removed.
-- AI Maxence
| const onBackButtonPress = useCallback(() => { | ||
| Navigation.goBack(listPath); | ||
| }, [listPath]); | ||
| // Contact Methods is already underneath this screen, so pop it instead of resolving its dynamic route again. |
There was a problem hiding this comment.
| // Contact Methods is already underneath this screen, so pop it instead of resolving its dynamic route again. |
There was a problem hiding this comment.
Fixed in f1cc739 — comment removed.
-- AI Maxence
| const [isUserValidated] = useOnyx(ONYXKEYS.ACCOUNT, {selector: isUserValidatedSelector}); | ||
| const {isAccountLocked} = useLockedAccountState(); | ||
| const {showLockedAccountModal} = useLockedAccountActions(); | ||
| // Strip the `contact-methods` suffix off the current URL so the back button returns to wherever this list was launched from, not a hardcoded default. |
There was a problem hiding this comment.
| // Strip the `contact-methods` suffix off the current URL so the back button returns to wherever this list was launched from, not a hardcoded default. | |
| // Strip the `contact-methods` suffix off the current URL so the back button returns to wherever this list was launched from |
There was a problem hiding this comment.
Fixed in 7de0a14 — applied your wording.
-- AI Maxence
| // Strip the `contact-methods` suffix off the current URL so the back button returns to wherever this list was launched from, not a hardcoded default. | ||
| const backPath = useDynamicBackPath(DYNAMIC_ROUTES.CONTACT_METHODS.path); | ||
| const repeatedContactMethodsSuffix = findAllMatchingDynamicSuffixes(backPath).find((match) => match.pattern === DYNAMIC_ROUTES.CONTACT_METHODS.path); | ||
| // Returning from a nested contact-method screen can leave a duplicate Contact Methods route in the stack. Remove it so Back targets the real parent instead of this screen. |
There was a problem hiding this comment.
| // Returning from a nested contact-method screen can leave a duplicate Contact Methods route in the stack. Remove it so Back targets the real parent instead of this screen. | |
| // Remove duplicate Contact Methods route when returning from a nested contact-method screen |
There was a problem hiding this comment.
Fixed in ca4e25a — applied your wording.
-- AI Maxence
…alidateCodePage Addresses review comment: #95610 (comment)
Addresses review comment: #95610 (comment)
Addresses review comment: #95610 (comment)
Addresses review comment: #95610 (comment)
|
No code fix to push — nothing is broken in CI. On
Every real check is green, including the ones that were red on earlier runs of this branch: ESLint, Bun tests, typecheck, all 8 What I checked
One thing worth flagging, since it's the actual blocker rather than CI: view run · no recording available |
6 participants
Explanation of Change
The workspace company cards and Expensify Card "import a feed from another workspace" flows both showed an inline email input on the Add work email screen and called
AddWorkEmaildirectly on submit. That command is an account-takeover vector and is now rejected by the backend for validated callers, so the screen silently failed on submit.This PR replaces the inline email input on both card‑feed Add work email screens with an "Add work email" button. The screen keeps the same title and description text, but the button now navigates the user to Settings → Profile → Contact Methods (
SETTINGS_CONTACT_METHODS), where adding a work email goes through the standard magic‑code‑verified flow. ThebackToparam is set to the current route so the user returns to the card‑feed screen afterward.Files changed:
src/pages/workspace/companyCards/WorkspaceCompanyCardAddWorkEmailPage.tsxsrc/pages/workspace/expensifyCard/WorkspaceExpensifyCardAddWorkEmailPage.tsxsrc/languages/en.ts/src/languages/es.ts— addedworkspace.companyCards.addWorkEmailButton("Add work email").The now-unused
AddWorkEmailinline submit/validation logic was removed from these two screens.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/641791
PROPOSAL:
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
Verify that no errors appear in the JS console
Create a new gmail account
Create a control workspace
Add a Mock Bank feed
Create a new workspace
Try to add the previously created Mock Bank feed:
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.