-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Filter out stale direct feeds missing oAuthAccountDetails #82056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@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] |
There was a problem hiding this comment.
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: fac9390e52
ℹ️ 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".
Direct feeds (OAuth/Plaid) require oAuthAccountDetails to be functional. Due to race conditions in the backend's non-atomic domain_member NVP updates, companyCards entries can become orphaned when oAuthAccountDetails is removed but the companyCards key remains. This adds an isDirectFeed helper (matching the backend's isOAuthBank/isPlaidBank prefix checks) and filters out stale direct feeds in getOriginalCompanyFeeds and getCombinedCardFeedsFromAllFeeds. Co-authored-by: Cursor <cursoragent@cursor.com>
fac9390 to
e2bf740
Compare
|
@codex review |
|
I will need to test with one of the accounts that reported this issue |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment has been minimized.
This comment has been minimized.
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ 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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb Safari |
|
@aldo-expensify 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] |
|
🎯 @ShridharGoel, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #82081. |
Replace passing full OnyxCollection<WorkspaceCardsList> through the feed utility chain with a lightweight FeedKeysWithAssignedCards map (Record<string, true>) for O(1) lookup. This avoids unnecessary re-renders when individual card details change and reduces memory overhead at each call site. - Add feedKeysWithAssignedCardsSelector in selectors/Card.ts - Refactor directFeedHasCards to use O(1) key lookup - Thread feedKeysWithCards through CardFeedUtils, SearchQueryUtils, SearchUIUtils and all consuming components/hooks - Add comprehensive tests for the new selector Co-authored-by: Cursor <cursoragent@cursor.com>
|
🚧 @mountiny 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! 🧪🧪
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
🚧 @mountiny 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! 🧪🧪
|
|
the builds are not clearing cache so its hard to test |
|
🚧 @mountiny 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! 🧪🧪
|
|
Ok first I had to fix the adhoc builds https://github.com/Expensify/App/pull/82240/changes to run the test, for this user that is showing correctly now
|
|
@codex review |
There was a problem hiding this comment.
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: 15fa8f2f3e
ℹ️ 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".
|
🚧 @mountiny 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! 🧪🧪
|
Resolve import conflict in SearchUIUtils.ts (keep FeedKeysWithAssignedCards type import and expanded date-fns imports from main) and accept main's Mobile-Expensify submodule ref. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@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". |
Resolve Mobile-Expensify submodule conflict (accept main's ref). Co-authored-by: Cursor <cursoragent@cursor.com>
|
🚧 @mountiny 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! 🧪🧪
|
The submodule pointer was inadvertently changed by a prior commit on this branch. Reset it to match main's current ref so the PR has no submodule diff. Co-authored-by: Cursor <cursoragent@cursor.com>
SummaryThis PR by @mountiny introduces a three-tier feed visibility system to filter out stale OAuth/Plaid feeds and orphaned legacy feeds from the company cards UI. The issue arises from backend race conditions in non-atomic 📋 Changes Overview
✅ Strengths
🔍 Concerns / Suggestions1. Type signature change in
|
Add typeof check to ensure cards value is an object before destructuring, guarding against unexpected non-object primitives in Onyx collection data. Co-authored-by: Cursor <cursoragent@cursor.com>
| "Bushwick", | ||
| "BYOC", | ||
| "capitalone", | ||
| "capitalonecards", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB 🤔


Explanation of Change
Stale direct feeds (OAuth/Plaid) and orphaned legacy feeds are showing in the App when
companyCardsentries exist but the feeds are no longer valid. This happens due to race conditions in the backend's non-atomicdomain_memberNVP updates (_setNamedin PHP does a read-modify-write that can orphancompanyCardsentries).This PR introduces a three-tier feed visibility system that classifies feeds and filters them accordingly:
isDirectFeed()helper — aligned with the backend'sCard::isOAuthBank/Card::isPlaidBank: feeds starting withoauthorplaid.isCustomFeed()helper — identifies commercial feeds: VCF (vcf), CDF (cdf), GL1025 (gl1025), GL1205 (gl1205), Amex Direct (amex).Three-tier visibility rules applied in both
getOriginalCompanyFeeds()andgetCombinedCardFeedsFromAllFeeds():isCustomFeed): always shown — these are file-based and don't depend on OAuth state.isDirectFeed): shown if they haveoAuthAccountDetailsOR assigned cards inWORKSPACE_CARDS_LIST.capitalonecards,americanexpressfd.us,ccupload*,stripe): shown only if they have assigned cards.Performance-optimized card lookup — instead of passing the entire
WORKSPACE_CARDS_LISTcollection through utility functions, afeedKeysWithAssignedCardsSelectorinselectors/Card.tsprecomputes a lightweightRecord<string, true>map of"${domainID}_${feedName}"keys for O(1) lookup. This selector is used withuseOnyxacross all hooks/components that display feeds.feedHasCards()helper — performs O(1) check against the precomputed map to determine if a feed has any assigned cards.feedKeysWithCardsthreaded through all display and search hooks/components:useCardFeeds,useCardFeedsForDisplay,SearchUIUtils,SearchQueryUtils,SearchAutocompleteList,SearchFiltersBar,SearchPageHeaderInput,SearchTypeMenu,useSearchTypeMenu, andcardFeedErrorsderived config.40+ unit tests covering all feed type classifications, the stale feed filtering behavior, gray-zone feed filtering, and the
feedKeysWithAssignedCardsSelector.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/596739
Tests
oAuthAccountDetailsstill appear in the company cards pageoAuthAccountDetailsor assigned cardsoAuthAccountDetailsor assigned cardsoAuthAccountDetailsAND without assigned cards are hiddencapitalonecards,americanexpressfd.us,stripe,ccupload*) are hidden when they have no assigned cardsnpm test -- --testPathPattern="tests/unit/CardUtilsTest.ts"and verify all tests passOffline tests
oAuthAccountDetailsand no assigned cards) are still filtered out even when offlineQA Steps
oAuthAccountDetailsand no assigned cards should be hidden)PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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 with no UI modifications
Android: mWeb Chrome
N/A - Logic-only change with no UI modifications
iOS: Native
N/A - Logic-only change with no UI modifications
iOS: mWeb Safari
N/A - Logic-only change with no UI modifications
MacOS: Chrome / Safari
N/A - Logic-only change with no UI modifications