Skip to content
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

[$250] Cards - In company cards settings page, on refresh entered details are not shown #56746

Open
2 of 8 tasks
IuliiaHerets opened this issue Feb 12, 2025 · 28 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Feb 12, 2025

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: V9.0.97-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
Issue reported by: Applause Internal Team
Device used: Redmi note 10s android 13
App Component: Workspace Settings

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Go to workspace settings - enable company cards
  3. Tap company cards
  4. Add a American express card selecting American express corporate cards
  5. Tap settings
  6. Change the card feed name and save
  7. Toggle on "allow deleting transactions"
  8. Refresh the page
  9. Now card feed name is empty
  10. Tap on card feed name
  11. Note in preview saved feed name is shown
  12. Note "allow deleting transactions" is toggle off
  13. Navigate back to company cards page
  14. Tap settings
  15. Now note feed name is displayed and "allow deleting transactions" toggle is on.

Expected Result:

In company cards settings page, on refresh entered details are not shown similarly it must not be displayed in preview and while revisiting the page.

Actual Result:

In company cards settings page, on refresh entered details are not shown but displayed in preview and while revisiting the page.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6740346_1739345791710.Screenrecorder-2025-02-12-12-55-05-817.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021890532491432510986
  • Upwork Job ID: 1890532491432510986
  • Last Price Increase: 2025-02-21
Issue OwnerCurrent Issue Owner: @
@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 12, 2025
Copy link

melvin-bot bot commented Feb 12, 2025

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@IuliiaHerets IuliiaHerets changed the title mWeb - Cards - In company cards settings page, on refresh entered details are not shown Cards - In company cards settings page, on refresh entered details are not shown Feb 12, 2025
@twilight2294
Copy link
Contributor

twilight2294 commented Feb 12, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-12 10:12:09 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

In company cards settings page, on refresh entered details are not shown

What is the root cause of that problem?

We do not have cardFeeds as a dependency while extracting selectedFeed, intially selectedFeed is undefined for sometime and it gets the onyx value after sometime, but the useMemo in selectedFeed only runs once:

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

Which causes the selectedFeed to the undefined and we do not get the values

What changes do you think we should make in order to solve the problem?

We need to add cardFeeds as a dependency to the useMemo this will make sure that we get the values for the feed whenever the values for SHARED_NVP_PRIVATE_DOMAIN_MEMBER are fetched.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

This is just a dependency addition and i don't think that we need a test for this, the values are not fetched on the update in onyx, so i think we can skip test for this issue

What alternative solutions did you explore? (Optional)

Copy link
Contributor

⚠️ @twilight2294 Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@bernhardoj
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Company card settings page shows empty name after refresh.

What is the root cause of that problem?

The card data is intentionally memoized once only.

const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

However, after refresh, the onyx data is still loading, so the data is undefined.

What changes do you think we should make in order to solve the problem?

Add loading state to the memo deps.

const [lastSelectedFeed, lastSelectedFeedResult] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
const isLoading = isLoadingOnyxValue(lastSelectedFeedResult);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), [isLoading]);

We can show loading indicator when it's loading if needed.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

@melvin-bot melvin-bot bot added the Overdue label Feb 14, 2025
@mallenexpensify mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Feb 14, 2025
@melvin-bot melvin-bot bot changed the title Cards - In company cards settings page, on refresh entered details are not shown [$250] Cards - In company cards settings page, on refresh entered details are not shown Feb 14, 2025
Copy link

melvin-bot bot commented Feb 14, 2025

Job added to Upwork: https://www.upwork.com/jobs/~021890532491432510986

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 14, 2025
Copy link

melvin-bot bot commented Feb 14, 2025

Triggered auto assignment to Contributor-plus team member for initial proposal review - @brunovjk (External)

@melvin-bot melvin-bot bot removed the Overdue label Feb 14, 2025
@mallenexpensify
Copy link
Contributor

@brunovjk 👀 plz on the proposals above. Thx

@brunovjk
Copy link
Contributor

Sure! Im on it :D

@nkdengineer
Copy link
Contributor

nkdengineer commented Feb 15, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-18 02:31:42 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

In company cards settings page, on refresh entered details are not shown but displayed in preview and while revisiting the page.

What is the root cause of that problem?

After we refresh the page, both cardFeeds and lastSelectedFeed are still undefined while the onboarding data is loading. Then selectedFeed is undefined because this useMemo has no dependency. The reason we add this useMemo is here

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

What changes do you think we should make in order to solve the problem?

Add the loading status of cardFeeds and lastSelectedFeed data as dependency of this useMemo

const [cardFeeds, cardFeedsMetaData] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed, lastSelectedFeedMetaData] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
const isLoadingLastSelectedFeedData = isLoadingOnyxValue(lastSelectedFeedMetaData);
const isLoadingCardFeedsData = isLoadingOnyxValue(cardFeedsMetaData);

// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), [isLoadingLastSelectedFeedData, isLoadingCardFeedsData]);

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

The reason we need to add both of them is when we reset cache or login again, lastSelectedFeed will be undefined and then we need to get the defaultFeed from cardFeeds data. So if we don't add isLoadingCardFeedsData as dependency, in this case, the selectedFeed is also undefined.

const defaultFeed = Object.keys(getCompanyFeeds(cardFeeds, true)).at(0) as CompanyCardFeed | undefined;

OPTIONAL: We can show the loading page while the feed data is still loading

To fix the deeplink case, I think we need to use useEffect here instead of useFocusEffect

useFocusEffect(fetchCompanyCards);

and also add cardFeeds.isLoading as a dependency here

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), [isLoadingLastSelectedFeedData, isLoadingCardFeedsData, cardFeeds?.isLoading]);

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

We can add only the isLoadingLastSelectedFeedData as the dependency but we need to update the last selected feed when we navigate to the company card setting page here.

onPress={() => {
    updateSelectedFeed(selectedFeed, policyID);
    Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS.getRoute(policyID));
}}

onPress={() => Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS.getRoute(policyID))}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

We can add cardFeeds and lastSelectedFeed as dependencies here, which will also fix the deep link case

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

And to fix the minor issue here, we can delay the API call instead of delaying the navigation logic because the problem of the minor issue is lastSelectedFeed is changed while we dismiss this page

const deleteCompanyCardFeed = () => {
        setDeleteCompanyCardConfirmModalVisible(false);
      Navigation.goBack();
      if (selectedFeed) {
          const {cardList, ...cards} = cardsList ?? {};
          const cardIDs = Object.keys(cards);
          const feedToOpen = (Object.keys(companyFeeds) as CompanyCardFeed[])
              .filter((feed) => feed !== selectedFeed && companyFeeds[feed]?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
              .at(0);
          InteractionManager.runAfterInteractions(() => {
              deleteWorkspaceCompanyCardFeed(policyID, workspaceAccountID, selectedFeed, cardIDs, feedToOpen);
          });
      }
  };

const deleteCompanyCardFeed = () => {
if (selectedFeed) {
const {cardList, ...cards} = cardsList ?? {};
const cardIDs = Object.keys(cards);
const feedToOpen = (Object.keys(companyFeeds) as CompanyCardFeed[])
.filter((feed) => feed !== selectedFeed && companyFeeds[feed]?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
.at(0);
deleteWorkspaceCompanyCardFeed(policyID, workspaceAccountID, selectedFeed, cardIDs, feedToOpen);
}
setDeleteCompanyCardConfirmModalVisible(false);
Navigation.setNavigationActionToMicrotaskQueue(Navigation.goBack);

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@huult
Copy link
Contributor

huult commented Feb 15, 2025

Proposal

Please re-state the problem that we are trying to solve in this issue.

In company cards settings page, on refresh entered details are not shown

What is the root cause of that problem?

cardFeeds and lastSelectedFeed are retrieved from useOnyx, but they are initially undefined until the data is fully loaded.
The useMemo hook runs only once due to an empty dependency array ([]), so it does not update when cardFeeds and lastSelectedFeed change.
This causes selectedFeed to remain undefined, leading to unexpected behavior.

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

What changes do you think we should make in order to solve the problem?

To resolve this issue, we just remove useMemo

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

 const selectedFeed = getSelectedFeed(lastSelectedFeed, cardFeeds);

Ran the pipeline to test ESLint and saw that it passed, so we don’t add useMemo here."

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@brunovjk
Copy link
Contributor

Reviewing proposals.

@mallenexpensify mallenexpensify added Internal Requires API changes or must be handled by Expensify staff External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Internal Requires API changes or must be handled by Expensify staff labels Feb 17, 2025
Copy link

melvin-bot bot commented Feb 17, 2025

Current assignee @brunovjk is eligible for the External assigner, not assigning anyone new.

@mallenexpensify
Copy link
Contributor

^ Sorry, was testing

@brunovjk
Copy link
Contributor

Thanks to everyone for the proposals. @twilight2294, @bernhardoj and @nkdengineer If we update the dependencies, the original issue disappears, but when I test it, I notice something else. If we clear the cache and try to access the settings via link, for example https://dev.new.expensify.com:8082/settings/workspaces/18901AD36B681B3D/company-cards/settings, we also not get the data correctly. Do you think this issue is related and we can fix it? Thanks.

Screen.Recording.2025-02-17.at.19.31.12.mov

@brunovjk
Copy link
Contributor

@VickyStash, since you worked on this PR, specifically about this comment, can you tell me what unexpected feed change you were expecting when adding the dependencies? I believe that this may be causing this one. Thanks a lot.

@nkdengineer
Copy link
Contributor

nkdengineer commented Feb 18, 2025

If we clear the cache and try to access the settings via link, for example https://dev.new.expensify.com:8082/settings/workspaces/18901AD36B681B3D/company-cards/settings

@brunovjk This is another case, if we access by deep link after resetting the cache, cardFeeds data doesn't exist. It need to fetch from OpenPolicyCompanyCardsPage API.

@nkdengineer
Copy link
Contributor

I notice something else. If we clear the cache and try to access the settings via link, for example https://dev.new.expensify.com:8082/settings/workspaces/18901AD36B681B3D/company-cards/settings, we also not get the data correctly. Do you think this issue is related and we can fix it?

Updated proposal to fix this case.

@VickyStash
Copy link
Contributor

@VickyStash, since you worked on this #52893, specifically about this #52893 (comment), can you tell me what unexpected feed change you were expecting when adding the dependencies? I believe that this may be causing this one. Thanks a lot.

@brunovjk the fix was made for this minor issue.

@brunovjk
Copy link
Contributor

brunovjk commented Feb 18, 2025

Thank you for the proposal @twilight2294 and @huult, but we should not use cardFeeds as a dependency, or remove the useMemo, the fix was made for this minor issue.

I think we can conclude that the root cause is due to selectedFeed being undefined while Onyx values are still loading. To fix this, we need to track the loading state using isLoadingOnyxValue.

@bernhardoj's proposal and @nkdengineer's proposal correctly address the issue by introducing a loading state dependency:

  • @nkdengineer also pointed out an additional case: when we reset the cache or log in again, lastSelectedFeed is undefined, so we need to ensure selectedFeed gets a valid value from cardFeeds. This requires tracking the loading state of both cardFeeds and lastSelectedFeed.
  • There's a question regarding the "deeplink case" (accessing settings via direct URL after clearing cache). While related, we're unsure if it's within the scope of this issue.

Next Steps:

  • We need internal confirmation that tracking both isLoadingLastSelectedFeedData and isLoadingCardFeedsData is the right approach.
  • Should we also consider the deeplink case in this fix or handle it separately?

Would love input from the internal team before we finalize the implementation. Thanks!

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 18, 2025

Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@danieldoglas
Copy link
Contributor

@brunovjk, thanks for testing and checking the edge case. Let's include the deeplink in this fix as well. Once we have the proposals updated to fix those, I'll increase the issue $ based on how hard the fix is.

@brunovjk
Copy link
Contributor

brunovjk commented Feb 20, 2025

Awesome! Thank you @danieldoglas

Let's include the deeplink in this fix as well. Once we have the proposals updated to fix those, ...

@twilight2294, @bernhardoj, @nkdengineer and @huult

@nkdengineer
Copy link
Contributor

To fix the deeplink case, I think we need to use useEffect here instead of useFocusEffect

@brunovjk I've updated my proposal from here

@brunovjk
Copy link
Contributor

Thanks @nkdengineer, did you test it? Because here, it even solves our edge case, but the App kind of got worse. I'm off the computer now, I'll record a video later of my test.

@nkdengineer
Copy link
Contributor

Thanks @nkdengineer, did you test it?

@brunovjk I tested and it works.

it even solves our edge case, but the App kind of got worse
Updated proposal with a simplier solution

Copy link

melvin-bot bot commented Feb 21, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@brunovjk
Copy link
Contributor

brunovjk commented Feb 21, 2025

Thanks @nkdengineer, I believe we can go with your proposal then, what do you think @danieldoglas? thanks a lot.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 21, 2025

Current assignee @danieldoglas is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Status: LOW
Development

No branches or pull requests

9 participants