Fix - Company cards verification always forwards to Add New, skipping feed and upgrade branches - #99184
Conversation
…eed and upgrade branches
|
@codex review |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
|
@aimane-chnaif 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: 14c0ecfc23
ℹ️ 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".
| } | ||
| if (!isUserValidated) { | ||
| Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_VERIFY_ACCOUNT.getRoute(policyID, selectedFeed)); | ||
| verifyAccountAndResume(undefined); |
There was a problem hiding this comment.
When an unvalidated user starts a card-feed refresh and then reloads the web page or restores the app after process termination, the resume callback is lost because useVerifyAccountAndResume stores it only in component state. The removed verification route encoded feed in the URL and supplied a deterministic forward destination, but the restored generic verify page now merely returns to settings, so this callback never runs and the refresh flow silently stops. Persist or encode the pending refresh intent so it can survive remounting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
While true, I think we're accepting this on all resume after verification flows
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@aimane-chnaif kind bump |
|
@aimane-chnaif bumpity bump |
|
reviewing today |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb1.movweb2.movweb3.mov |
|
@aimane-chnaif bampity bumpity bump |
|
I was testing this.
How were you able to add one card feed without validating account? |
You add it in workspace from validated account and add new account to said workspace as admin, then assign a card to it |
| } | ||
| if (!isUserValidated) { | ||
| Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_VERIFY_ACCOUNT.getRoute(policyID, selectedFeed)); | ||
| verifyAccountAndResume(undefined); |
There was a problem hiding this comment.
The test case for this change is missing.
After validation, Assign new cards modal briefly shows and closes. Sometimes infinite loading shows. <-- bug?
With already validated, infinite loading shows. <-- this also happens on production.
If this is not caused by this PR, no need to block on this.
ios.mov
web3.mov
|
Please add Scenario 3 for #99184 (comment) and perform test |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
Review — LGTM ✅The refactor is correct and matches the fix described. Unvalidated users now run the same branching logic after validation that validated users get up front (upgrade gate / feed selector / add-new), instead of always being forwarded to add-new. Nice cleanup deleting the bespoke verify-account route/screen/page in favor of the shared What I checked:
Minor nits (non-blocking)
What I could not verify in this run
|
Regression scan — no regressions found ✅I traced every behavior this refactor touches (static analysis) and exercised the flow live on web. Nothing broke for existing/validated users, and the risky part of the refactor (the dynamic Runtime result: partial — the CI test account was actually unvalidated, which is the exact precondition this PR fixes, but that means the flow hit the real magic-code wall, which the automated tester can't clear (no mailbox). So I confirmed the entry into verification but not the resume after validation. Positive signal: pressing Add cards opened a correctly-rendered "Validate your account" RHP — not a Not Found / blank / crash — and backing out returned cleanly to the company-cards page. That's the main thing the entry-screen registration had to get right. What I checked for regressionsCode-level regression vectors (all clear)
Behavior changes worth being aware of (not bugs)
Not verified at runtime (OTP-gated)Because the tester couldn't clear the emailed code, these still want a manual check with an unvalidated account (the PR's own test precondition) to fully close the loop:
Live test — Company cards (web), unvalidated accountCompany cards empty state loaded cleanly
Add cards opened Validate your account RHP (dynamic verify-account route resolved, no Not Found)
Back navigation from verify screen returns cleanly to Company cards empty state
Home shows account is genuinely unvalidated
|
Updated steps & fixed spinner bug - will add vids soom, run out of verification codes for now :( |
|
@aimane-chnaif vids for scenario 3 Screen.Recording.2026-08-26.at.16.16.40.movNative Screen.Recording.2026-08-26.at.16.18.47.mov |
|
@MelvinBot review last commit |
| // When refreshing the feed, a healthy connection must not short-circuit into the assignee step. | ||
| // RefreshCardFeedConnectionPage can't render it and the modal would spin forever. | ||
| if (!assignCard?.isRefreshing) { |
There was a problem hiding this comment.
ok, so this fixes the separate bug happening on production
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5539b0eac6
ℹ️ 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".
| return; | ||
| // When refreshing the feed, a healthy connection must not short-circuit into the assignee step. | ||
| // RefreshCardFeedConnectionPage can't render it and the modal would spin forever. | ||
| if (!assignCard?.isRefreshing) { |
There was a problem hiding this comment.
When an expired OAuth feed is successfully refreshed, isFeedExpired changes from true to false while assignCard.isRefreshing is still true. This effect therefore closes the completed popup, falls through this new condition, and calls openBankConnection(url) again; the parent RefreshCardFeedConnectionPage then closes the RHP after observing the expiration change, leaving a newly opened authentication popup behind in environments that permit it. Distinguish the initial healthy-feed launch from the expired-to-healthy completion so the latter returns without reopening OAuth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I didn't notice this effect when testing, did you @aimane-chnaif ?
Review of last commit —
|
3 participants
Explanation of Change
useVerifyAccountAndResume.entryScreenson theverify-accountdynamic route and deleted the now-redundant staticWORKSPACE_COMPANY_CARDS_VERIFY_ACCOUNTroute, screen, and page.Fixed Issues
$ #98066
PROPOSAL:
Tests
Precondition: account that is unvalidated .
(can be faked with
await Onyx.merge('account', {validated: false});in browser console)Scenario 1
Scenario 2
Collectworkspace with at least one existing company card feed.Scenario 3
Offline 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-08-21.at.16.20.34.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-08-21.at.16.12.01.mov
iOS: mWeb Safari
Screen.Recording.2026-08-21.at.16.15.31.mov
MacOS: Chrome / Safari
Screen.Recording.2026-08-21.at.15.52.47.mov