Fix - Company cards - Broken card feed connection RHP spins forever with no error when the feed has a feed-level error - #100687
Conversation
…ith no error when the feed has a feed-level error
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3775879c1b
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d472a96239
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e2e062d37
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d79654d12
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33fc2462d2
ℹ️ 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".
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: ONYXKEYS.ASSIGN_CARD, | ||
| value: {isRefreshing: null}, | ||
| value: {isRefreshing: null, ...(isRepairingFeed ? {errors: getMicroSecondOnyxErrorWithTranslationKey('common.genericErrorMessage')} : {})}, |
There was a problem hiding this comment.
Scope refresh completion to repair imports
The new isRepairingFeed flag scopes only the error, while every failed import still clears the shared ASSIGN_CARD.isRefreshing flag. For example, useAssignCard starts a non-repair import with the default false; if that request is still pending when the user starts the banner refresh, its failure produces a true → null transition with no error, and RefreshCardFeedConnectionPage closes the RHP before the new reauthentication completes. Only repair imports should update refresh state, ideally tied to the active attempt; the unchanged success path has the same overlapping-request problem.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Race predates the PR: importPlaidAccounts has cleared isRefreshing unconditionally on both success and failure before. This PR only scopes the new errors write.
|
@MelvinBot review |
|
🤖 Code review The core diagnosis looks right: the broken/refresh connection pages only render Five things worth a look before merge. 1. Removing the
Suggestion: instead of dropping the guard, gate on "the feed entry has been observed at least once" — e.g. keep a 2. The expired-feed assign flow now silently discards the in-progress assignment Both platforms replaced the step write with a plain close (web 3.
4. Native: the
5. Card-level broken connections still never reach the bank login — intentional? For a direct feed where Nits
I reviewed this statically and did not run the test steps in the app — they need a workspace with an existing direct Plaid/OAuth feed plus the console-injected Onyx state from the PR description, neither of which the automated test account can reach. Reply with |
|
@MelvinBot please check fix on regression findings 1, 2 and 4. |
Fix check — findings 1, 2, 4Verified against
1. Non-direct feed error copy — fixed
Two things worth a moment before you merge:
2. Plaid import error leaking into the assign form — half fixed
What's still open: Reaching it is narrow — 4. Refresh page closing without a reconnect — fixedTracking the feed object instead of the scalar ( Why it holds for both the late-load and uncached-OAuth cases
One narrow residual, strictly better than before: if a reconnect completes before the collection first loads (hard reload mid-reconnect with The new test at 3 — agreedLeaving it matches Not verified: I couldn't run |
|
@aimane-chnaif addressed all above |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
|
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🤖 Regression review of this diff (static analysis — I read the changed files and everything that feeds them; I was not able to execute the Jest suites in this environment, so nothing below is backed by a test run). Four things look like real regressions or gaps. The first two are the ones I'd fix before merge. 1. The OAuth auto-close guard got weaker and can close the panel mid-login
The new test only covers 2.
|
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.82-0 🚀
|
|
🤖 Yes — help site changes are required. Draft PR: Docs updates for E/A#100687 Broken company card feed reconnect flow One article covers this flow — 1. The reconnect link now only exists on direct feeds. 2. There is a new error screen. 3. The panel's open/close behavior changed. The reconnect stays open through the bank login and closes itself once the reconnect completes, and an assign-card flow that detoured into the reconnect closes rather than resuming — an admin has to assign the card again. What the docs PR changes
No new article — all of it belongs to the existing broken-connection workflow, and splitting it would break the one-workflow-per-article rule in Label verification. Workspaces and Company cards were confirmed against the live UI on web. Strings that need a broken feed to reach were taken verbatim from @jmusial, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Asked @joekaufmanexpensify if he can help with testing this feature, though he cannot exactly qa the flow mentioned in the test steps |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.86-0 🚀
|
|
Yes — help site changes are required. Draft PR: Docs updates for E/A#100687 broken card feed reconnect behavior Three things this PR changed are visible to Workspace Admins and were not documented:
What the docs PR changes
One copy correction worth flagging: the PR description calls the error page button "Confirm", but I verified the UI labels against the live web build rather than the source strings — Workspaces tab, Company cards row, and the More features > Spend > Company cards toggle all match. Written against @jmusial, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.89-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.89-0 🚀
Bundle Size Analysis (Sentry): |
I was only able to test the normal broken feed error on staging. Fixing the error did work. Though after fixing it, we didn't seem to automatically scrape the card, so I had to manually scrape it to remove the feed-level error. Which is not normally expected. Unsure if specifically related to this PR, though. |
Explanation of Change
Fixed Issues
$ #100465
PROPOSAL:
Tests
For dev tests - how to get local feed into desired state
Scenario 1 - Feed lvl error
Scenario 2 - Expired feed
Scenario 3 - Failed import
Offline tests
N/A
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
MacOS: Chrome / Safari
Screen.Recording.2026-09-09.at.11.10.30.mov
Screen.Recording.2026-09-09.at.12.03.37.mov