Fix AMEX card feed reconnect by scoping to the specific feed - #85126
Conversation
The useUpdateFeedBrokenConnection hook was using a global aggregate isFeedConnectionBroken flag and picking the first broken card across ALL feeds, which could target the wrong feed entirely. Additionally, only one card was synced even when multiple cards on the same feed had broken connections. This change: - Uses per-feed isFeedConnectionBroken state from cardFeedErrors instead of the global aggregate - Filters broken cards to only those belonging to the target feed - Syncs all broken cards on the target feed, not just the first one Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
The failing check is test (job 8), specifically
This failure is unrelated to this PR. This PR only modifies Recommendation: Re-run the failed test job. If it persists, it should be investigated as a separate flaky test issue. |
|
@MelvinBot This doesn't work until we refresh the page after the connection is done. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ 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". |
|
Thanks for testing, AnalysisHow the flow works
Root Cause: SyncCard response likely reverts the optimistic update
On page refresh, Secondary factor: immediate navigationIn if (isFeedConnectionBroken) {
updateBrokenConnection();
Navigation.closeRHPFlow(); // Navigates away immediately
return;
}The navigation happens synchronously after firing the API call. The derived value Proposed FixCheck function isCardConnectionBroken(card: Card): boolean {
if (card.pendingFields?.lastScrape) {
return false; // Sync in progress — don't mark as broken
}
return !!card.lastScrapeResult && !CONST.COMPANY_CARDS.BROKEN_CONNECTION_IGNORED_STATUSES.includes(card.lastScrapeResult);
}This approach:
Alternatively, if the intent is to guarantee the error clears immediately after triggering the sync, Let me know if you'd like me to implement either of these approaches, or if you have additional context about what you're observing (e.g., does the error briefly disappear then reappear, or does it never clear at all?). |
|
@MelvinBot Ok, implement the updated fix |
Check pendingFields.lastScrape in isCardConnectionBroken — if a sync is pending, treat the card as not broken. This prevents the error banner from reappearing when the server response overwrites the optimistic lastScrapeResult before the sync cycle completes. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Pushed the fix in ea9eb2a. What changed: Added a How it works:
|
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.
|
|
@claude review |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@MelvinBot Mark this as ready for review |
|
Marked this PR as ready for review as requested. |
|
@ShridharGoel 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] |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
I agree that what Melvin describes above sounds like a good change. If you have multiple broken connections and fix one of them, we should still show the error for any other(s). However, this logic is very delicate. I'd love if @carlosmiceli could review to make sure the code melvin is suggesting makes sense. |
|
Let's still wait for @ShridharGoel's review first, though it seems ok to me (not entirely sure about the pending scrape added check). |
|
Does this experience look fine? Screen.Recording.2026-04-03.at.3.39.13.PM.mov |
|
@ShridharGoel Could you be more specific about what you're asking for input on? The duplicate errors thing with a feed level and card level error on Company cards is not expected. We are tackling that in #86589 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-04-03.at.3.39.13.PM.mov |
|
All checks on this PR are currently passing — no failures found. If you're seeing a specific failure, it may have been a transient/flaky issue that resolved on retry. Let me know if you're still seeing problems and I'll take another look. |
|
🚧 @carlosmiceli has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.3.58-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The changes are purely internal bug fixes to the |
|
Hi @MelvinBot. Unfortunately, we are blocked from testing this PR. cc @carlosmiceli @joekaufmanexpensify @fedirjh bandicam.2026-04-10.20-52-40-556.mp4 |
|
I will test in a bit 👍 |
|
@joekaufmanexpensify any update on qa? |
|
I tested. Fixing the broken connection when you have multiple worked fine on my Amex connection. One minor issue:
Neither of these feel like blockers to me, but @carlosmiceli @ShridharGoel could you address in a follow up? Screenshots from test
|
|
Checked off for QA 👍 |
|
Much appreciated! 👏 |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.3.58-9 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.3.58-9 🚀
|



Explanation of Change
The
useUpdateFeedBrokenConnectionhook had two bugs that prevented card feed reconnection from working correctly:Used global aggregate state instead of per-feed state: The hook read
all.isFeedConnectionBrokenwhich is an OR-aggregate across ALL feeds. This meant it could report a broken connection for the wrong feed, or trigger reconnect logic for the wrong reason.Picked the first broken card globally, not from the target feed:
Object.entries(cardsWithBrokenFeedConnection).at(0)grabbed the first broken card across ALL feeds. If the user reconnected AMEX but a different feed's card came first in the dict,SyncCardtargeted the wrong card entirely. Even if the right feed's card was picked, only ONE card was synced — but feeds can have multiple broken cards, and the derivedisFeedConnectionBrokenuses||=aggregation, so leaving any card broken keeps the error banner visible.This PR fixes both issues by:
cardFeedErrors[feed]?.isFeedConnectionBrokenfor per-feed broken connection statecardsWithBrokenFeedConnectionto only cards matching the target feed (viagetCardFeedWithDomainID)Fixed Issues
$ #84081
PROPOSAL: #84081 (comment)
Tests
Offline tests
N/A - The reconnect flow requires an active network connection to authenticate with the bank.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
N/A - Logic-only change, no UI modifications
Android: mWeb Chrome
N/A - Logic-only change, no UI modifications
iOS: Native
N/A - Logic-only change, no UI modifications
iOS: mWeb Safari
N/A - Logic-only change, no UI modifications
MacOS: Chrome / Safari
N/A - Logic-only change, no UI modifications