Skip to content

Optimistically delete card feed from WS when deleted #68860

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.

Sign up for GitHub

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

Merged
amyevans merged 9 commits into from
Sep 15, 2025
Merged

Optimistically delete card feed from WS when deleted #68860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
26dfd26
optimistically delete card feed from WS when deleted
bernhardoj Aug 20, 2025
ea39a5a
lint
bernhardoj Aug 20, 2025
227ae7a
Merge branch 'main' into fix/68137-card-feed-filter-still-shown-after…
bernhardoj Aug 26, 2025
8b9815c
Merge branch 'main' into fix/68137-card-feed-filter-still-shown-after…
bernhardoj Sep 10, 2025
acf3e5b
Merge branch 'main' into fix/68137-card-feed-filter-still-shown-after…
bernhardoj Sep 12, 2025
e6dc0b8
get the card feeds with Onyx.connectWithView
bernhardoj Sep 12, 2025
4930837
lint
bernhardoj Sep 12, 2025
bb3526c
Merge branch 'main' into fix/68137-card-feed-filter-still-shown-after…
bernhardoj Sep 14, 2025
896200f
Revert "get the card feeds with Onyx.connectWithView"
bernhardoj Sep 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/hooks/useCardFeeds.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,20 @@
* @param policyID - The workspace policyID to filter and construct card feeds for.
* @returns -
* A tuple containing:
* 1. Card feeds specific to the given policyID (or `undefined` if unavailable).
* 1. Combined workspace and domain card feeds specific to the given policyID (or `undefined` if unavailable).
* 2. The result metadata from the Onyx collection fetch.
* 3. Card feeds specific to the given policyID (or `undefined` if unavailable).
*/
const useCardFeeds = (policyID: string | undefined): [CardFeeds | undefined, ResultMetadata<OnyxCollection<CardFeeds>>] => {
const useCardFeeds = (policyID: string | undefined): [CardFeeds | undefined, ResultMetadata<OnyxCollection<CardFeeds>>, CardFeeds | undefined] => {
const workspaceAccountID = useWorkspaceAccountID(policyID);
const [allFeeds, allFeedsResult] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER, {canBeMissing: true});
const defaultFeed = allFeeds?.[`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`];

const workspaceFeeds = useMemo(() => {
if (!policyID || !allFeeds) {
return undefined;
}

const defaultFeed = allFeeds?.[`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`];
const {companyCards = {}, companyCardNicknames = {}, oAuthAccountDetails = {}} = defaultFeed?.settings ?? {};

const result: CardFeeds & {settings: Required<CardFeeds['settings']>} = {
Expand Down Expand Up @@ -68,9 +69,9 @@

return acc;
}, result);
}, [allFeeds, policyID, workspaceAccountID]);

Check warning on line 72 in src/hooks/useCardFeeds.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

React Hook useMemo has missing dependencies: 'defaultFeed?.isLoading' and 'defaultFeed?.settings'. Either include them or remove the dependency array

Check warning on line 72 in src/hooks/useCardFeeds.tsx

View workflow job for this annotation

GitHub Actions / ESLint check 

React Hook useMemo has missing dependencies: 'defaultFeed?.isLoading' and 'defaultFeed?.settings'. Either include them or remove the dependency array

return [workspaceFeeds, allFeedsResult];
return [workspaceFeeds, allFeedsResult, defaultFeed];
};

export default useCardFeeds;
28 changes: 23 additions & 5 deletions src/libs/actions/Policy/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
import type {OnboardingAccounting} from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {
CardFeeds,
DuplicateWorkspace,
IntroSelected,
InvitedEmailsToAccountIDs,
Expand Down Expand Up @@ -183,7 +184,7 @@
};

const allPolicies: OnyxCollection<Policy> = {};
Onyx.connect({

Check warning on line 187 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.POLICY,
callback: (val, key) => {
if (!key) {
Expand Down Expand Up @@ -216,7 +217,7 @@
});

let allReports: OnyxCollection<Report>;
Onyx.connect({

Check warning on line 220 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -225,7 +226,7 @@
});

let allReportActions: OnyxCollection<ReportActions>;
Onyx.connect({

Check warning on line 229 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
waitForCollectionCallback: true,
callback: (actions) => {
Expand All @@ -235,7 +236,7 @@

let sessionEmail = '';
let sessionAccountID = 0;
Onyx.connect({

Check warning on line 239 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.SESSION,
callback: (val) => {
sessionEmail = val?.email ?? '';
Expand All @@ -244,31 +245,31 @@
});

let allPersonalDetails: OnyxEntry<PersonalDetailsList>;
Onyx.connect({

Check warning on line 248 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (val) => (allPersonalDetails = val),
});

let reimbursementAccount: OnyxEntry<ReimbursementAccount>;
Onyx.connect({

Check warning on line 254 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
callback: (val) => (reimbursementAccount = val),
});

let allRecentlyUsedCurrencies: string[];
Onyx.connect({

Check warning on line 260 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.RECENTLY_USED_CURRENCIES,
callback: (val) => (allRecentlyUsedCurrencies = val ?? []),
});

let activePolicyID: OnyxEntry<string>;
Onyx.connect({

Check warning on line 266 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
callback: (value) => (activePolicyID = value),
});

let allTransactionViolations: OnyxCollection<TransactionViolations> = {};
Onyx.connect({

Check warning on line 272 in src/libs/actions/Policy/Policy.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS,
waitForCollectionCallback: true,
callback: (value) => (allTransactionViolations = value),
Expand Down Expand Up @@ -364,12 +365,22 @@
/**
* Delete the workspace
*/
function deleteWorkspace(policyID: string, policyName: string, lastAccessedWorkspacePolicyID: string | undefined, lastUsedPaymentMethods?: LastPaymentMethod) {
function deleteWorkspace(
policyID: string,
policyName: string,
lastAccessedWorkspacePolicyID: string | undefined,
policyCardFeeds: CardFeeds | undefined,

@bernhardoj bernhardoj Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fixed the conflict. @ishpaul777 do you think we should just get the card feeds from Onyx.connectWithoutView since the data doesn't affect rendering?

@ishpaul777 ishpaul777 Sep 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Onyx.connectWithoutView oh i am hearing about it for first time, is its same as Onyx.connect? or do we any documentation around it? like when to use and when not to, if its not a restricted thing i think it makes sense to me, as it will help reduce diffs

@bernhardoj bernhardoj Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a guideline yet for that, but Onyx.connectWithoutView is a replacement for Onyx.connect. It does the same thing, just a different name to make it clear that it should only be used for a non-UI thing.

https://github.com/Expensify/react-native-onyx/blob/6978c02088147db2dc13be9700291c128aded406/API.md?plain=1#L11-L17

For example,

App/src/libs/HttpUtils.ts

Lines 25 to 35 in 2120b7d

// We have used `connectWithoutView` here because HttpUtils is not connected to any UI component
Onyx.connectWithoutView({
key: ONYXKEYS.NETWORK,
callback: (network) => {
if (!network) {
return;
}
shouldFailAllRequests = !!network.shouldFailAllRequests;
shouldForceOffline = !!network.shouldForceOffline;
},
});

@ishpaul777 ishpaul777 Sep 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, in that case i think the usage here makes perfect sense to me, Thanks for explaination!

@bernhardoj bernhardoj Sep 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

lastUsedPaymentMethods?: LastPaymentMethod,
) {
if (!allPolicies) {
return;
}

const filteredPolicies = Object.values(allPolicies).filter((policy): policy is Policy => policy?.id !== policyID);
// This will be fixed as part of https://github.com/Expensify/Expensify/issues/507850
// eslint-disable-next-line deprecation/deprecation
const policy = getPolicy(policyID);
const filteredPolicies = Object.values(allPolicies).filter((p): p is Policy => p?.id !== policyID);
const workspaceAccountID = policy?.workspaceAccountID;
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
Expand All @@ -380,6 +391,11 @@
errors: null,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`,
value: null,
},
...(!hasActiveChatEnabledPolicies(filteredPolicies, true)
? [
{
Expand All @@ -393,9 +409,6 @@
: []),
];

// This will be fixed as part of https://github.com/Expensify/Expensify/issues/507850
// eslint-disable-next-line deprecation/deprecation
const policy = getPolicy(policyID);
// Restore the old report stateNum and statusNum
const failureData: OnyxUpdate[] = [
{
Expand All @@ -413,6 +426,11 @@
pendingAction: null,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`,
value: policyCardFeeds,
},
];

if (policyID === activePolicyID) {
Expand Down
6 changes: 3 additions & 3 deletions src/pages/workspace/WorkspaceOverviewPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function WorkspaceOverviewPage({policyDraft, policy: policyProp, route}: Workspa

// We need this to update translation for deleting a workspace when it has third party card feeds or expensify card assigned.
const workspaceAccountID = policy?.workspaceAccountID ?? CONST.DEFAULT_NUMBER_ID;
const [cardFeeds] = useCardFeeds(policy?.id);
const [cardFeeds, , defaultCardFeeds] = useCardFeeds(policy?.id);
const [cardsList] = useOnyx(`${ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST}${workspaceAccountID}_${CONST.EXPENSIFY_CARD.BANK}`, {
selector: filterInactiveCards,
canBeMissing: true,
Expand Down Expand Up @@ -197,10 +197,10 @@ function WorkspaceOverviewPage({policyDraft, policy: policyProp, route}: Workspa
return;
}

deleteWorkspace(policy.id, policyName, lastAccessedWorkspacePolicyID, lastPaymentMethod);
deleteWorkspace(policy.id, policyName, lastAccessedWorkspacePolicyID, defaultCardFeeds, lastPaymentMethod);
setIsDeleteModalOpen(false);
goBackFromInvalidPolicy();
}, [policy?.id, policyName, lastAccessedWorkspacePolicyID, lastPaymentMethod]);
}, [policy?.id, policyName, lastAccessedWorkspacePolicyID, defaultCardFeeds, lastPaymentMethod]);

useEffect(() => {
if (isLoadingBill) {
Expand Down
4 changes: 2 additions & 2 deletions src/pages/workspace/WorkspacesListPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ function WorkspacesListPage() {

// We need this to update translation for deleting a workspace when it has third party card feeds or expensify card assigned.
const workspaceAccountID = policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyIDToDelete}`]?.workspaceAccountID ?? CONST.DEFAULT_NUMBER_ID;
const [cardFeeds] = useCardFeeds(policyIDToDelete);
const [cardFeeds, , defaultCardFeeds] = useCardFeeds(policyIDToDelete);
const [cardsList] = useOnyx(`${ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST}${workspaceAccountID}_${CONST.EXPENSIFY_CARD.BANK}`, {
selector: filterInactiveCards,
canBeMissing: true,
Expand Down Expand Up @@ -162,7 +162,7 @@ function WorkspacesListPage() {
return;
}

deleteWorkspace(policyIDToDelete, policyNameToDelete, lastAccessedWorkspacePolicyID, lastPaymentMethod);
deleteWorkspace(policyIDToDelete, policyNameToDelete, lastAccessedWorkspacePolicyID, defaultCardFeeds, lastPaymentMethod);
setIsDeleteModalOpen(false);
};

Expand Down
2 changes: 1 addition & 1 deletion tests/actions/IOUTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4414,7 +4414,7 @@ describe('actions/IOU', () => {
)
.then(() => {
if (policy) {
deleteWorkspace(policy.id, policy.name, undefined);
deleteWorkspace(policy.id, policy.name, undefined, undefined);
}
return waitForBatchedUpdates();
})
Expand Down
10 changes: 5 additions & 5 deletions tests/actions/PolicyTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ describe('actions/Policy', () => {

// When deleting a workspace fails
mockFetch?.fail?.();
Policy.deleteWorkspace(fakePolicy.id, fakePolicy.name, undefined);
Policy.deleteWorkspace(fakePolicy.id, fakePolicy.name, undefined, undefined);

await waitForBatchedUpdates();

Expand Down Expand Up @@ -897,7 +897,7 @@ describe('actions/Policy', () => {
{name: 'hold', type: CONST.VIOLATION_TYPES.WARNING},
]);

Policy.deleteWorkspace(policyID, 'test', undefined);
Policy.deleteWorkspace(policyID, 'test', undefined, undefined);

await waitForBatchedUpdates();

Expand All @@ -922,7 +922,7 @@ describe('actions/Policy', () => {

jest.spyOn(PolicyUtils, 'getPersonalPolicy').mockReturnValue(personalPolicy);

Policy.deleteWorkspace(teamPolicy.id, teamPolicy.name, undefined);
Policy.deleteWorkspace(teamPolicy.id, teamPolicy.name, undefined, undefined);
await waitForBatchedUpdates();

const activePolicyID: OnyxEntry<string> = await new Promise((resolve) => {
Expand All @@ -946,7 +946,7 @@ describe('actions/Policy', () => {
await Onyx.merge(ONYXKEYS.LAST_ACCESSED_WORKSPACE_POLICY_ID, lastAccessedWorkspacePolicyID);
await waitForBatchedUpdates();

Policy.deleteWorkspace(policyToDelete.id, policyToDelete.name, lastAccessedWorkspacePolicyID);
Policy.deleteWorkspace(policyToDelete.id, policyToDelete.name, lastAccessedWorkspacePolicyID, undefined);
await waitForBatchedUpdates();

const lastAccessedWorkspacePolicyIDAfterDelete: OnyxEntry<string> = await new Promise((resolve) => {
Expand All @@ -972,7 +972,7 @@ describe('actions/Policy', () => {
await Onyx.merge(ONYXKEYS.LAST_ACCESSED_WORKSPACE_POLICY_ID, lastAccessedWorkspacePolicyID);
await waitForBatchedUpdates();

Policy.deleteWorkspace(policyToDelete.id, policyToDelete.name, lastAccessedWorkspacePolicyID);
Policy.deleteWorkspace(policyToDelete.id, policyToDelete.name, lastAccessedWorkspacePolicyID, undefined);
await waitForBatchedUpdates();

const lastAccessedWorkspacePolicyIDAfterDelete: OnyxEntry<string> = await new Promise((resolve) => {
Expand Down
Loading