[Payment due @linhvovan29546] Drive card feed selectors off linkedPolicyIDs, remove preferredPolicy decisioning#93928
Conversation
… decisioning Fix duplicate domain feeds and missing domain feeds in the company-card and Expensify-card feed selectors by enumerating each feed once and partitioning available vs From other workspaces purely by linkedPolicyIDs membership. Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Fixed the two failing checks (commit c2cea9e):
The other failing check (
|
…kedPolicyIDs visibility model Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Pushed a second fix (commit 0d55bde) for the 1. Typecheck (2 errors)
2. Jest — Root cause: those tests set up admin policies but never satisfied the feed‑visibility gate. Per this PR's own design, Fixes:
|
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.
|
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Fixed the failing The This was the only error tsc reported. The remaining failing
|
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Fixed the failing ESLint check (commit 3e1c66e). My earlier prettier/spellcheck fixes now pass; the remaining All 13 ESLint errors were
I updated the seatbelt baseline to the exact current counts. I chose the baseline route rather than refactoring because I can't run prettier/eslint/tsc in this environment, so any code edit (typed
|
|
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. |
|
Where are we at here? |
|
I need to finish testing this |
…tion to 2 Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the company-card and Expensify-card feed selector logic to enumerate feeds once per source feed and partition them based solely on linkedPolicyIDs membership (removing preferredPolicy decisioning), addressing duplicated and missing domain feed rows in the selector UI.
Changes:
- Refactors Expensify Card selector visibility/partitioning to remove
preferredPolicyand issued-card gating, and uselinkedPolicyIDsonly for “primary vs other”. - Introduces
getVisibleCompanyCardFeedsForSelector()to enumerate visible company card feeds once (domain/workspace admin–gated) for the selector path. - Updates
useOtherFeedsForFeedSelectorand related unit tests to consume the new selector-specific enumeration and use a stable per-feed list key.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/hooks/useOtherFeedsForFeedSelector.test.tsx | Updates hook tests to mock the new selector feed enumeration and validate dedupe behavior/key stability. |
| tests/unit/hooks/useExpensifyCardFeedsForFeedSelector.test.ts | Adjusts partitioning expectations to reflect removal of preferredPolicy primary selection. |
| tests/unit/ExpensifyCardFeedSelectorUtilsTest.ts | Updates utils tests to validate new admin-gated visibility rules without issued-card gating. |
| tests/unit/CardFeedUtilsTest.ts | Adds unit tests for getVisibleCompanyCardFeedsForSelector() visibility and deduping. |
| src/libs/ExpensifyCardFeedSelectorUtils.ts | Removes preferredPolicy and issued-card visibility gating; makes “primary” depend only on linkedPolicyIDs. |
| src/libs/CardFeedUtils.ts | Adds getVisibleCompanyCardFeedsForSelector() to enumerate company feeds once with admin gating for selector usage. |
| src/hooks/useOtherFeedsForFeedSelector.tsx | Switches “From other workspaces” list to the new visible-feeds enumerator and stable per-feed keyForList. |
| src/hooks/useExpensifyCardFeedsForFeedSelector.ts | Updates call signature after removing cardList dependency from Expensify selector utils. |
| config/eslint/eslint.seatbelt.tsv | Updates seatbelt counts for newly introduced lint baselines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@MelvinBot can you address the PR Review comments? |
… assertion - Partition feeds case-insensitively against linkedPolicyIDs (matches Expensify-card path) - Uppercase the linked policy ID when indexing the POLICY collection so alternateText resolves - Use a membership check on the domain-ID-suffixed companyCardFeeds key instead of an unsafe CompanyCardFeedWithNumber assertion (seatbelt count 4 -> 2) Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
…okup Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
@MelvinBot can you address the latest PR Review comments? |
… selector Make mockVisibleFeeds a void helper to avoid an unsafe-return, and precompute the set of admin-backed policy fundIDs once so per-domain visibility is a constant-time lookup instead of scanning all policies. Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
Co-authored-by: Maxence Coulibaly <madmax330@users.noreply.github.com>
|
Fixed the failing Oxfmt check and pushed to the branch. Cause: the recent Fix: removed the trailing whitespace. I ran (The earlier spellcheck failure is already resolved and now passing.) |
|
@MelvinBot any idea how this could happen? #93928 (comment) |
|
The "3 rows = one per linked policy" pattern is exactly the pre-fix duplication signature. Since the selector code is identical on every platform (no 1. Android isn't running this PR's JS (most likely). Web was built off the branch (shows the fixed count of 1). The Android HybridApp build may be on an older/staging bundle that still uses the old per- 2. A real gap in the new dedup (if Android is on the branch). The dedup key is How to tell which one it is: on the Android account, dump the
Evidence
|
|
@linhvovan29546 is android running these changes? It doesn't make sense otherwise since the code is the same for every platform 🤔 |
|
Ah, I think my Android app was using an old build, since I just tested again and didn't see Metro running on the device |
|
🎯 @linhvovan29546, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
| // `policyID` is stable for this hook invocation, so uppercase it once instead of per feed evaluation. | ||
| const upperPolicyID = policyID.toUpperCase(); |
There was a problem hiding this comment.
Are there some cases when we know the policyID here are not uppercase?
There was a problem hiding this comment.
Tbh, not 100% sure it's the AI reviewer that suggested it. But I think it makes sense since there's for sure some places in the App where it's not uppercase, so we could inadvertently pass that in if it doesn't already happen.
| /** | ||
| * Resolves the domain backing a fund (card account). Domains are normally keyed by their account ID, | ||
| * but as a fallback we scan for a domain whose `accountID` matches the fund. | ||
| */ | ||
| function getDomainByFundID(domains: OnyxCollection<Domain> | undefined, fundID: number): OnyxEntry<Domain> { | ||
| return domains?.[`${ONYXKEYS.COLLECTION.DOMAIN}${fundID}`] ?? Object.values(domains ?? {}).find((entry) => entry?.accountID === fundID); | ||
| } |
There was a problem hiding this comment.
Can you add unit tests for this method in a follow up please?
There was a problem hiding this comment.
@MelvinBot can you make a new Expensify/App PR that addresses this comment please?
There was a problem hiding this comment.
Done — opened a follow-up PR that adds unit tests for getDomainByFundID: #95507
It adds a describe('getDomainByFundID', ...) block in tests/unit/CardUtilsTest.ts covering both resolution paths (keyed by domain_<fundID> and the accountID fallback), their precedence, and the undefined/no-match/undefined-entry cases. Test-only, no production code changed. typecheck-tsgo passes; CI runs the rest.
The PR is a draft assigned to madmax330 — please mark it "Ready for review" when it looks good.
|
🚧 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.28-0 🚀
|
Help site review — no changes required 📚I reviewed this PR against the help site articles under Why: This PR is an internal bug fix to the card-feed selector logic — it enumerates each feed once off What I checked
The PR brings the app's actual behavior in line with what the docs already describe, rather than changing documented behavior — so there is nothing to update. @madmax330 — since I'm reporting no docs update is needed, there's no linked help site PR to review. If you believe a help article should be updated (e.g. to add a note about feeds appearing once per domain), let me know and I'll draft one. |
|
Could you please provide more detailed QA steps?
Or is this something that should be verified internally instead? |
|
There are no duplicate feeds showing |
|
@madmax330 are we good to check it off? |
|
Yes |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.28-2 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #95521 |



Explanation of Change
Both the company-card and Expensify-card feed selectors previously decided which feeds to show — and where to show them ("available" vs From other workspaces) — using a fallback chain of
linkedPolicyIDs→preferredPolicy→policyAccountIDmatching. That produced two reported bugs when a domain feed had nolinkedPolicyIDsand nopreferredPolicy:policyAccountIDmatched the fund, so the "From other workspaces" list rendered one row per such workspace (count grew each time the feed was shared to another workspace).This PR rewrites both selectors to the model requested in the linked proposal: enumerate each feed exactly once from its source NVP, and partition into available vs "From other workspaces" purely by whether the active policy is in that feed's
linkedPolicyIDs. AllpreferredPolicydecisioning is removed.src/libs/ExpensifyCardFeedSelectorUtils.ts):isExpensifyCardFeedVisibleToAdminnow gathers a feed only when the user is an admin of the feed's domain (domain accountID === fundID) or of a policy whosepolicyAccountID=== fundID. ThepreferredPolicybranch and thehasIssuedExpensifyCardgate (the cause of the missing Expensify-card feeds) are removed, andisFeedPrimaryForPolicyis reduced to alinkedPolicyIDsmembership check.getVisibleCompanyCardFeedsForSelectorenumerates eachsettings.companyCardsfeed once, gated by the same domain/workspace-admin rule, anduseOtherFeedsForFeedSelectorconsumes it — filtering out feeds linked to the active policy (and any already in the active policy's available list) and using a stable per-feedkeyForListso the same feed can no longer render twice.Notes for the reviewer (
madmax330) — please confirm:getCardFeedsForDisplayPerPolicyanduseCardFeedsForActivePolicies, but those are also consumed by the search path (useCardFeedsForDisplay→getDefaultCardFeed,SearchUIUtils, YourSpend) and the wallet path. Admin-gating / removing the eligible-policy filter there would drop search feeds for non-admin employees and change wallet logic. So I left those shared functions untouched for search/wallet and added a dedicatedgetVisibleCompanyCardFeedsForSelectorfor the selector instead. If you'd also like the search/display path migrated offpreferredPolicy, that should be a separate, explicitly-scoped change.useCompanyCards(policyID)pipeline (already correct for "feeds on the active policy"); its subtitle still usesgetLinkedPolicyName(... preferredPolicy ...)for display only. Let me know if you want that cosmeticpreferredPolicyusage removed too.🤖 Generated with MelvinBot
Fixed Issues
$ #93925
PROPOSAL: https://github.com/Expensify/Expensify/issues/649586#issuecomment-4740868912
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
linkedPolicyIDs/preferredPolicy), open a workspace's company-card feed selector.linkedPolicyIDsincludes the active policy appears as an available feed, not under "From other workspaces".PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop