Detect duplicate Plaid feeds to prevent infinite spinner in BankConnection#88520
Conversation
…nite spinner When adding a duplicate Plaid direct feed, the BankConnection component shows an infinite spinner because checkIfNewFeedConnected cannot detect the existing feed - it gets filtered out by getCombinedCardFeedsFromAllFeeds. Pass raw (unfiltered) feed keys from settings.companyCards to checkIfNewFeedConnected so it can detect the pre-existing Plaid feed in the backend data. Also add missing return in native BankConnection after isNewFeedConnected block to prevent unnecessary importPlaidAccounts call. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
… effect - CardUtils.ts: Replace `||` with `??` for nullish coalescing compliance - BankConnection/index.tsx: Convert shouldBlockWindowOpen from useState to useRef since it's only read inside the effect, avoiding the set-state-in-effect lint warning and unnecessary re-renders Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Fixed the failing ESLint check:
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot Please update the PR according to your latest proposal here. The modal text should be: Card feed already connected |
…navigating When a user tries to add a Plaid card feed that already exists on the workspace, show a modal explaining the feed is already connected instead of silently closing the RHP. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Updated the PR per the proposal. When a duplicate Plaid card feed is detected, a
Changes:
|
This comment has been minimized.
This comment has been minimized.
|
The failing Analysis: The failure is in Evidence: This is a known flaky perf test. Re-running the check should resolve it. The only other failing check is |
|
@MelvinBot Please apply the diff from here. |
Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Applied the Polyglot Parrot translations for |
| isVisible={isDuplicateFeed && !isDuplicateFeedDismissed} | ||
| title={translate('workspace.companyCards.addNewCard.duplicateFeedModal.title')} | ||
| prompt={translate('workspace.companyCards.addNewCard.duplicateFeedModal.prompt')} | ||
| confirmText={translate('workspace.companyCards.addNewCard.duplicateFeedModal.confirmText')} |
There was a problem hiding this comment.
@MelvinBot Let's use common.buttonConfirm instead and remove the confirmText key from the translations.
There was a problem hiding this comment.
Done — switched to common.buttonConfirm and removed the confirmText key from duplicateFeedModal in all language files.
There was a problem hiding this comment.
Switched to common.buttonConfirm and removed the confirmText key from duplicateFeedModal in all language files.
Remove custom confirmText translation key from duplicateFeedModal and use the existing common.buttonConfirm translation instead. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
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. |
|
@MelvinBot BUG1: the modal briefly shows even when adding a feed for the first time. |
|
@MelvinBot BUG2: The RHP is still open in the background with a spinner whilst the modal shows and only closes once the modal is dismissed. I think it would be better if the RHP is dismissed first, if that's possible. 
|
Track whether onImportPlaidAccounts was called via isConfirmedNewFeed state. For genuinely new feeds, the import is called first (setting the flag), so the subsequent rawFeedKeys match is recognized as a new feed in transition — not a duplicate. For true duplicates, the plaid feed already exists in rawFeedKeys before any import is called, so isConfirmedNewFeed stays false and the modal correctly shows. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Fixed BUG1. The issue was that for genuinely new Plaid feeds, the raw feed key appears in Fix: Added
|
The ConfirmModal was previously rendered inside BankConnection, causing the spinner to show behind the modal while the RHP remained open. Now BankConnection sets an Onyx flag and navigates away immediately, and the WorkspaceCompanyCardsPage reads the flag to show the modal after the RHP has closed. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Fixed. The When a duplicate feed is detected, BankConnection now:
WorkspaceCompanyCardsPage reads the flag and shows the modal after the RHP has fully closed. Dismissing the modal clears the flag. |
|
@MelvinBot Now the modal only appears briefly after adding a duplicate feed, i.e. immediately self-closes. Please also merge main. |
…page The modal was self-closing because AddNewCardPage's cleanup effect calls clearAddNewCardFlow() on unmount, which clears the isDuplicateFeed Onyx data before the Company Cards page can show the modal. Fix: Show the ConfirmModal directly in BankConnection using a latched state variable (set during render, never reset by derived value changes) so the modal stays visible until the user taps "Got it". Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
…aidFeedInfiniteSpinner Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
The violation is now suppressed via eslint-disable-next-line comment, so the seatbelt entry is no longer needed. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Fixed the self-closing modal and merged main. Root cause: Fix: Moved the Also removed the now-unused |
This comment has been minimized.
This comment has been minimized.
|
@MelvinBot Looks like that latest change causes lint and compiler compliance errors. Can you check how this is handled elsewhere in the codebase? |
Wraps showConfirmModalRef and translateRef updates in useEffect hooks instead of updating during render, which violates the react-hooks/refs ESLint rule and React Compiler compliance. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Fixed the lint and compiler errors by moving the ref updates ( |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ 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". |
| const {updateBrokenConnection, isFeedConnectionBroken} = useUpdateFeedBrokenConnection({policyID, feed}); | ||
| const isNewFeedHasError = !!(newFeed && cardFeeds?.[newFeed]?.errors); | ||
| const {isBlockedToAddNewFeeds, isAllFeedsResultLoading} = useIsBlockedToAddFeed(policyID); | ||
| const hasEverDetectedNewFeed = useRef(false); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The duplicate feed detection logic added here (refs hasEverDetectedNewFeed, hasShownDuplicateModal, showConfirmModalRef, translateRef, the two useEffect hooks to sync refs, and the duplicate detection block inside the main effect) is copied identically into both index.tsx and index.native.tsx. This violates the DRY principle -- if the detection logic needs to change, both files must be updated in lockstep.
Extract the shared logic into a custom hook, for example:
// useDuplicateFeedDetection.ts
function useDuplicateFeedDetection({
isNewFeedConnected,
newFeed,
isPlaid,
policyID,
}: { ... }) {
const hasEverDetectedNewFeed = useRef(false);
const hasShownDuplicateModal = useRef(false);
const {showConfirmModal} = useConfirmModal();
const {translate} = useLocalize();
const checkForDuplicate = useCallback(() => {
if (!isNewFeedConnected) return false;
if (newFeed) {
hasEverDetectedNewFeed.current = true;
return false;
}
const isDuplicate = !newFeed && isPlaid && !hasEverDetectedNewFeed.current;
if (isDuplicate && !hasShownDuplicateModal.current) {
hasShownDuplicateModal.current = true;
Navigation.closeRHPFlow();
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS.getRoute(policyID), {forceReplace: true});
showConfirmModal({ ... });
return true;
}
return false;
}, [isNewFeedConnected, newFeed, isPlaid, policyID, showConfirmModal, translate]);
return {checkForDuplicate};
}Then use it in both platform files.
Reviewed at: 0cda279 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Done — extracted the shared duplicate-feed detection logic into a useDuplicateFeedDetection hook. Both platform files now call checkForDuplicateFeed(newFeed) from the hook instead of duplicating the refs, sync effects, and modal handling inline.
| const isNewFeedHasError = !!(newFeed && cardFeeds?.[newFeed]?.errors); | ||
| const onImportPlaidAccounts = useImportPlaidAccounts(policyID); | ||
| const {isBlockedToAddNewFeeds, isAllFeedsResultLoading} = useIsBlockedToAddFeed(policyID); | ||
| const hasEverDetectedNewFeed = useRef(false); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The duplicate feed detection logic added here (refs hasEverDetectedNewFeed, hasShownDuplicateModal, showConfirmModalRef, translateRef, the two useEffect hooks to sync refs, and the duplicate detection block inside the main effect) is copied identically from index.native.tsx. This violates the DRY principle.
Extract the shared duplicate feed detection logic into a reusable custom hook (e.g., useDuplicateFeedDetection) that both platform files can import. See the comment on index.native.tsx for a suggested implementation.
Reviewed at: 0cda279 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const showConfirmModalRef = useRef(showConfirmModal); | ||
| const translateRef = useRef(translate); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-4 (docs)
Two useEffect hooks are added solely to keep showConfirmModalRef and translateRef in sync with their respective values. These ref-syncing effects, combined with the duplicate feed detection logic embedded in the larger main effect, represent multiple unrelated concerns mixed into this component. The duplicate feed detection (refs, sync effects, and modal logic) is a self-contained concern that should be extracted into a focused custom hook like useDuplicateFeedDetection.
This extraction would:
- Remove the need for the ref-syncing
useEffectpattern entirely (the hook can includeshowConfirmModalandtranslatedirectly in its own effect dependency array) - Give the logic a descriptive name
- Make it independently testable
- Reduce the complexity of the already-large main
useEffect
Reviewed at: 0cda279 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const showConfirmModalRef = useRef(showConfirmModal); | ||
| const translateRef = useRef(translate); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-4 (docs)
Two useEffect hooks are added solely to keep showConfirmModalRef and translateRef in sync with their respective values. These ref-syncing effects, combined with the duplicate feed detection logic embedded in the larger main effect, represent multiple unrelated concerns mixed into this component. The duplicate feed detection (refs, sync effects, and modal logic) is a self-contained concern that should be extracted into a focused custom hook like useDuplicateFeedDetection.
This extraction would:
- Remove the need for the ref-syncing
useEffectpattern entirely (the hook can includeshowConfirmModalandtranslatedirectly in its own effect dependency array) - Give the logic a descriptive name
- Make it independently testable
- Reduce the complexity of the already-large main
useEffect
Reviewed at: 0cda279 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Addressed — the ref-syncing effects and duplicate detection logic are now encapsulated in the useDuplicateFeedDetection hook, removing these concerns from both platform files.
|
Looks like the Codex review came back clean — no major issues found. Nothing further for me to action here. 👍 |
|
@MelvinBot No it didn't 😄 Please check the comments here and here. |
Moves the shared duplicate-feed detection logic (refs, ref-syncing effects, and modal handling) from both index.tsx and index.native.tsx into a reusable custom hook, eliminating code duplication. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
@MelvinBot Shouldn't the hook be in the |
Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Moved |
|
@MelvinBot Please fix lint errors. |
Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
Fixed the ESLint errors: changed |
|
Nice, over to you @inimaga! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @inimaga 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.3.81-0 🚀
|
|
I reviewed the changes in this PR and confirmed that a help site update is needed. What changed: This PR adds a "Card feed already connected" modal when a user tries to add a duplicate Plaid card feed to the same workspace. The existing Set up a Direct Company Card Feed Connection article has an FAQ about connecting across workspaces, but doesn't address attempting to add the same feed to the same workspace. Draft PR created: #91594
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.81-2 🚀
|
Explanation of Change
When adding a duplicate Plaid direct feed (e.g., Wells Fargo), the RHP auto-closes silently because
checkIfNewFeedConnecteddetects the existing feed. The user gets no feedback that the feed is already connected.This PR detects the duplicate case (
isNewFeedConnected && !newFeed && isPlaid) and shows aConfirmModaldirectly insideBankConnectioninforming the user that the card feed is already connected. On dismiss, the user is navigated back to the company cards page.Fixed Issues
$ #81573
PROPOSAL: #81573 (comment)
Tests
Offline tests
N/A
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari