Skip to content

[Payment due @situchan] Show OAuth connection errors - #100019

Merged
madmax330 merged 16 commits into
mainfrom
maxence/show-oauth-connection-errors
Oct 1, 2026
Merged

madmax330 merged 16 commits into
mainfrom
maxence/show-oauth-connection-errors

Conversation

@madmax330

@madmax330 madmax330 commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

The OAuth callback backend supplies a provider-safe connection failure message, but the bank-connection screens only used the error bags as booleans and always rendered generic copy. This change displays the backend message for personal and company-card flows on web and native, while retaining the existing recovery guidance. Company-card errors are cleared before acknowledgement or retry so stale failures do not block another connection attempt.

Depends on https://github.com/Expensify/Web-Expensify/pull/55792

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/675115
PROPOSAL:

Tests

  1. Run npm run typecheck.
  2. Run npm run react-compiler-compliance-check check-changed.
  3. Run npm run spell-changed -- <changed files>.
  4. Cancel a supported personal-card OAuth flow and verify the bank rejection message appears with Plaid recovery guidance.
  5. Cancel a supported company-card OAuth flow and verify the bank rejection message appears with card-feed recovery guidance.
  6. Retry via Plaid and verify the prior error no longer blocks the connection flow.
  • Verify that no errors appear in the JS console

Offline tests

  1. Disable the network before opening a bank-connection flow.
  2. Verify the existing offline behavior remains unchanged because this change only renders server-pushed OAuth errors after a callback.

QA Steps

Needs to be ProdQA

  1. Cancel a supported personal-card OAuth flow and verify the bank rejection message appears with Plaid recovery guidance.
  2. Cancel a supported company-card OAuth flow and verify the bank rejection message appears with card-feed recovery guidance.
  3. Retry via Plaid and verify the prior error no longer blocks the connection flow.
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there is a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using that component are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style does not already exist
    • The style cannot be created with an existing StyleUtils function
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App.
  • If the PR modifies a component related to any of the existing Storybook stories, I verified all stories still work.
  • If the PR modifies a page that can be accessed by a direct deeplink, I verified the code functions for logged-in and logged-out accounts.
  • If the PR modifies the UI or form input styles, I verified all inputs inside a form are aligned and added the Design label/tag if needed.
  • I added unit tests for this bug fix.
  • If the main branch was merged into this PR after a review, I tested again and verified the expected outcome.

Screenshots/Videos

Android: Native

Not captured; OAuth bank callbacks require staging credentials.

Android: mWeb Chrome

Not captured; OAuth bank callbacks require staging credentials.

iOS: Native

Not captured; OAuth bank callbacks require staging credentials.

iOS: mWeb Safari

Not captured; OAuth bank callbacks require staging credentials.

MacOS: Chrome / Safari

Not captured; OAuth bank callbacks require staging credentials.

@madmax330
madmax330 requested review from a team as code owners September 1, 2026 11:02
@melvin-bot
melvin-bot Bot requested review from garrettmknight and situchan and removed request for a team September 1, 2026 11:02
@melvin-bot

melvin-bot Bot commented Sep 1, 2026

Copy link
Copy Markdown

@situchan 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
melvin-bot Bot requested review from a team and removed request for a team September 1, 2026 11:02
@codecov

codecov Bot commented Sep 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/hooks/useCompanyCardConnectionError.ts 100.00% <100.00%> (ø)
src/libs/CardUtils.ts 88.57% <100.00%> (+1.60%) ⬆️
...space/companyCards/BankConnection/index.native.tsx 0.00% <0.00%> (ø)
...es/workspace/companyCards/BankConnection/index.tsx 0.00% <0.00%> (ø)
src/libs/actions/CompanyCards.ts 39.60% <0.00%> (+0.16%) ⬆️
...t/PersonalCards/PersonalCardsErrorConfirmation.tsx 0.00% <0.00%> (ø)
...ersonalCards/steps/BankConnection/index.native.tsx 0.00% <0.00%> (ø)
...nyCards/WorkspaceCompanyCardsErrorConfirmation.tsx 0.00% <0.00%> (ø)
...allet/PersonalCards/steps/BankConnection/index.tsx 0.00% <0.00%> (ø)
... and 625 files with indirect coverage changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b22053970

ℹ️ 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".

@garrettmknight
garrettmknight removed their request for review September 2, 2026 10:12
@madmax330

Copy link
Copy Markdown
Contributor Author

@MelvinBot can you fix the Eslint check?

…ESLint

Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor

Fixed and pushed — the ESLint check was failing on src/hooks/useCompanyCardConnectionError.ts, which imported two sibling hooks through the @hooks/ alias:

4:1  error  Unexpected subpath import via alias '@hooks/useCardFeeds'. Use './useCardFeeds' instead  @dword-design/import-alias/prefer-alias
5:1  error  Unexpected subpath import via alias '@hooks/useOnyx'. Use './useOnyx' instead            @dword-design/import-alias/prefer-alias

prefer-alias requires relative paths for files inside the same directory, so I switched both to ./useCardFeeds and ./useOnyx and moved them into the sibling import group to match .oxfmtrc.json sort order (see src/hooks/useDefaultWorkspaceTravelGuard.ts:7-13 for the same pattern).

AI tests run locally
Check Result
npx eslint src/hooks/useCompanyCardConnectionError.ts ✅ pass
npm run typecheck ✅ pass
npm run react-compiler-compliance-check check src/hooks/useCompanyCardConnectionError.ts ✅ pass

Oxfmt formatting was verified against the repo's sortImports groups rather than run directly — the binary was blocked in this environment. CI's Oxfmt check will confirm.

The two other red checks — checklist and Check independent approval — are process gates, not code failures, so I left those alone.


view run

@situchan

situchan commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
ios.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98f5c7d61a

ℹ️ 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".

@MelvinBot

This comment has been minimized.

@chiragsalian

Copy link
Copy Markdown
Contributor

All cool let me know when you've handled the AI review comments and this is clear for my review.

@madmax330

Copy link
Copy Markdown
Contributor Author

Addressing the OAuth error findings in #5631187219 and #5631416532: fixed in 3f65205. Both BankConnection screens now use the hook's hasError predicate for effect and render gating, so an add-flow error cannot navigate away before its confirmation renders. This also removes the duplicate feed subscription and adds hook coverage.

-- AI Maxence

@madmax330

Copy link
Copy Markdown
Contributor Author

@MelvinBot can you review this PR again?

@MelvinBot

This comment has been minimized.

@madmax330

This comment has been minimized.

@MelvinBot

This comment has been minimized.

@madmax330

This comment has been minimized.

@madmax330

Copy link
Copy Markdown
Contributor Author

@MelvinBot can you review again?

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Code review — head 42f59af5

Looks good to merge from my side. Both new commits do what they say, and I found no new regressions. All checks pass except Check independent approval, which is a process gate.

What's fixed

  • Confirm no longer re-runs the connection (🟠 from my last review): onButtonPress no longer clears addNewCompanyCard.errors. So hasError stays true while the panel slides out, and the effect still returns early at BankConnection/index.tsx:122. The popup can't reopen, and Plaid can't re-import. The error is still cleared, because DynamicAddNewCardPage wipes the key when it unmounts. The connect with the Expensify Card link calls onButtonPress, so it gets the fix too. Back and Try again via Plaid keep their clears, which is correct.
  • Hook cleanup: the hook now receives addNewCard and returns only {errorMessage, hasError}. Both callers pass the addNewCard they already subscribe to (index.tsx:78, index.native.tsx:76). The tests still cover all four cases, and they no longer need Onyx.

One optional nit

useCompanyCardConnectionError no longer calls any React hooks. It's now a pure function with a use prefix. You could move it to CardUtils as getCompanyCardConnectionError and test it without renderHook. This isn't blocking.

Still open from earlier reviews (out of scope or pre-existing)
  • Feed-error-only path: if only the new feed has errors, newFeed exists for one render. After that, hasError turns false and the effect can reopen the bank window. This behaves the same on main.
  • Hard reload: a persisted errors value survives a reload, so reopening Add cards lands on the error page. You agreed this is out of scope.
  • Backend contract: the server message is joined to the copy with a bare space, so it needs ending punctuation. It also needs to be a string, not a {translationKey} object. Please confirm both against the companion backend PR.

I reviewed the code only. I did not run the OAuth flows in a browser, because they need real bank credentials on staging.


view run

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Regression hunt — head 42f59af5

No new regressions found. I re-checked the full diff against main, not only the latest commits. Every regression raised earlier in this PR is fixed at this head. What's left depends on when the backend writes the error, and can't be fixed or confirmed from App alone.

Still worth a manual check (backend timing)

  • Late error: if the OAuth callback fails after the user presses Back, picking a bank again opens the error page with the old message. Pressing Back again clears it, so the user can recover.
  • Popup stays open: when the add-flow error shows, the effect returns at BankConnection/index.tsx:122 before it calls customWindow?.close(). If the user retries in that popup and succeeds, the error page stays up. Closing the window on error, the same way the personal-card screen does, would remove this case. This is optional.
  • Personal-card messages: main already showed the error page whenever the backend wrote addNewPersonalCard.errors, including from AddPersonalPlaidCard. Now that backend text also appears word for word (PersonalCardsErrorConfirmation.tsx:50). Please confirm those existing messages are safe to show users and end with punctuation.
Checked and cleared at this head
  • Confirm / Expensify Card link: errors stay set until DynamicAddNewCardPage unmounts, so the effect can't reopen the popup or import through Plaid again during the slide-out.
  • Back and Try again via Plaid: the clear and the step change both merge into the same Onyx key and apply together, so nothing flashes or runs again.
  • Fix-connection and refresh flows: both pass feed, so isAddingNewCard is false and add-flow errors are ignored (index.tsx:78).
  • Existing Plaid feed: newFeed is strictly the new-feed diff again (CardUtils.ts:1254-1269), so Confirm can't delete a feed the user already had.
  • Message rendering: getLatestErrorMessage returns a plain string (ErrorUtils.ts:94-103), and App's own writes to these keys only clear them.
  • Carried over and agreed out of scope: a persisted error survives a hard reload. The feed-error-only page can disappear after one render, which also happens on main.

I reviewed the code only. Earlier web runs on this PR couldn't get past the account-validation code, and the error states need a real bank rejection from the backend.


view run

@madmax330

Copy link
Copy Markdown
Contributor Author

@chiragsalian we're ready now

@chiragsalian

Copy link
Copy Markdown
Contributor
QA Steps

Same as tests.

QA can't do steps 1-3. Did you mean for them to QA 4-6? I'll approve this so its not held on me but please clean up the QA steps and then merge the PR.

@madmax330
madmax330 merged commit 7c5abdb into main Oct 1, 2026
42 of 43 checks passed
@madmax330
madmax330 deleted the maxence/show-oauth-connection-errors branch October 1, 2026 11:24
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

🚧 madmax330 has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

OSBotify commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Oct 1, 2026
73 tasks done
@OSBotify

OSBotify commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/madmax330 in version: 9.5.1-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 cancelled 🔪
🍎 iOS 🍎 cancelled 🔪

@IuliiaHerets

Copy link
Copy Markdown

Hi @madmax330. Just to confirm, does this PR need to be checked on prod or stg?
Also, please share more detailed steps.

cc @chiragsalian @situchan

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site update is needed. This PR only shows the bank's own error message on the existing card connection error screens. In-app error messages explain themselves, and no article sentence becomes incorrect.


view run

@Julesssss

Copy link
Copy Markdown
Contributor

Needs to be ProdQA

We have no productionQA label on App so @madmax330 can you please make sure this is tested on prod once deployed

@OSBotify

OSBotify commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.1-9 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

@melvin-bot

melvin-bot Bot commented Oct 5, 2026

Copy link
Copy Markdown

🤖 Payment issue created: #102950

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants