Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[$62.50] Workspace -Previous invite message reappears after clearing the field and entering a character #49996

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.42-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch New Expensify app
  2. Go to workspace settings > Members
  3. Click Invite member
  4. Select a member and proceed to invite message page
  5. Clear the invite message field
  6. Enter a character

Expected Result:

The previous invitation message will not appear after clearing the field and entering a character

Actual Result:

The previous invitation message reappears after clearing the field and entering a character

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/647e950d-324b-4bbf-a6a9-9002c9654517

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021842313701781491251
  • Upwork Job ID: 1842313701781491251
  • Last Price Increase: 2024-10-04
Issue OwnerCurrent Issue Owner: @abdulrahuman5196
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @adelekennedy (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.

lanitochka17 commented 2 weeks ago

@adelekennedy FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 2 weeks ago

We think that this bug might be related to #wave-collect - Release 1

nyomanjyotisa commented 2 weeks ago

Proposal

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

Workspace -Previous invite message reappears after clearing the field and entering a character

What is the root cause of that problem?

The root cause of this problem stems from the interaction between the defaultValue and value props. The defaultValue prop is used to set the initial value of the input. While this works for the first load of the component, it introduces an issue when the user clears the input and entering a character on android https://github.com/Expensify/App/blob/2c07b39d1d2e3d451419002072220486c60c2131/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L192-L193

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

We should either remove the defaultValue or value props, and use only one of it

What alternative solutions did you explore? (Optional)

kushu7 commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-10-01 14:39:43 UTC.

Proposal

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

Previous invite message reappears after clearing the field due to re-render

What is the root cause of that problem?

It is happening because we have workspaceInviteMessageDraft in deps array of callback function and this gets update on onChange of input.

https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L63-L75

which is causing this useEffect to run on each render and setting back to default message as when workspaceInviteMessageDraft is empty

https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L94

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

By removing workspaceInviteMessageDraft from here will fix the issue as we want getDefaultMessage on first render only. we can safely remove it.

https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L75

What alternative solutions did you explore? (Optional)

None

huult commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-10-01 14:54:15 UTC.

Proposal

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

Invite message comes back after deleting it

What is the root cause of that problem?

Since we set getDefaultWelcomeNote as a dependency of this hook, the hook is triggered every time the input's onChange event occurs

https://github.com/Expensify/App/blob/12e2808b10b222e96278b8845332e68b968f3b11/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L94

When we remove all the text and input new text, setWelcomeNote will be triggered. https://github.com/Expensify/App/blob/12e2808b10b222e96278b8845332e68b968f3b11/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L93

and workspaceInviteMessageDraft is not saving completely, so it gets the default value, and this issue occurs https://github.com/Expensify/App/blob/12e2808b10b222e96278b8845332e68b968f3b11/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L67

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

We should remove getDefaultWelcomeNote from the dependency array to avoid re-renders when the input changes.

// .src/pages/workspace/WorkspaceInviteMessagePage.tsx#L94
   useEffect(() => {
-        if (isEmptyObject(invitedEmailsToAccountIDsDraft) || !welcomeNote) {
+        if (isEmptyObject(invitedEmailsToAccountIDsDraft)) {
            return;
        }
        setWelcomeNote(getDefaultWelcomeNote());
-    }, [getDefaultWelcomeNote, invitedEmailsToAccountIDsDraft, welcomeNote]);
+    }, [invitedEmailsToAccountIDsDraft]);

and We need to validate this input, if necessary, to avoid empty values ("") (optional)

// .src/pages/workspace/WorkspaceInviteMessagePage.tsx#L119
        if (isEmptyObject(invitedEmailsToAccountIDsDraft)) {
            errorFields.welcomeMessage = translate('workspace.inviteMessage.inviteNoMembersError');
-        }
+        } else if (isEmptyObject(welcomeNote)) {
+            errorFields.welcomeMessage = 'Please enter a welcome note.';
        }
POC https://github.com/user-attachments/assets/0158f257-1d7d-4f42-bea8-426ea50a4399
bernhardoj commented 2 weeks ago

Proposal

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

The default message shows briefly everytime we type in the ws invite message page.

What is the root cause of that problem?

We have this effect which updates the welcome note to the default every time the welcome note is updated. https://github.com/Expensify/App/blob/7f0bdf08094574ee019f6b1b2470f71163d2d102/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L94

This is added in https://github.com/Expensify/App/pull/48660 because we migrated to useOnyx so when the component is mounted for the first time, workspaceInviteMessageDraft from Onyx is initially undefined, so getDefaultWelcomeNote will return the default message instead of the saved user draft, https://github.com/Expensify/App/blob/7f0bdf08094574ee019f6b1b2470f71163d2d102/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L63-L74

and is set as the input default value. https://github.com/Expensify/App/blob/7f0bdf08094574ee019f6b1b2470f71163d2d102/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L192

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

Remove this useEffect https://github.com/Expensify/App/blob/7f0bdf08094574ee019f6b1b2470f71163d2d102/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L94

and instead, to overcome the issue after migrating to useOnyx, we can re-trigger this effect when the workspaceInviteMessageDraft is loaded. https://github.com/Expensify/App/blob/7f0bdf08094574ee019f6b1b2470f71163d2d102/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L77-L87

const [workspaceInviteMessageDraft, workspaceInviteMessageDraftResult] = useOnyx(`${ONYXKEYS.COLLECTION.WORKSPACE_INVITE_MESSAGE_DRAFT}${route.params.policyID.toString()}`);
const isWorkspaceInviteMessageDraftLoading = isLoadingOnyxValue(workspaceInviteMessageDraftResult);

useEffect(() => {
    if (isWorkspaceInviteMessageDraftLoading) {
        return;
    }
    if (!isEmptyObject(invitedEmailsToAccountIDsDraft)) {
        setWelcomeNote(getDefaultWelcomeNote());
        return;
    }
    if (isEmptyObject(policy)) {
        return;
    }
    Navigation.goBack(ROUTES.WORKSPACE_INVITE.getRoute(route.params.policyID), true);
    // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [isWorkspaceInviteMessageDraftLoading]);

We can consider removing the defaultValue too https://github.com/Expensify/App/blob/7f0bdf08094574ee019f6b1b2470f71163d2d102/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L192

we can optionally returning null when isWorkspaceInviteMessageDraftLoading is true

melvin-bot[bot] commented 1 week ago

@adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

adelekennedy commented 1 week ago

I can replicate this but it's so briefI think it's really a polish - updating price

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

@abdulrahuman5196, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

adelekennedy commented 1 week ago

@abdulrahuman5196 a few proposals to review above!

abdulrahuman5196 commented 1 week ago

@bernhardoj 's proposal here https://github.com/Expensify/App/issues/49996#issuecomment-2386342971 looks good and works well. It addresses the root cause and solution where we don't need to update the defaultWelcomeNote whenever there is a welcomeNote change compared to other proposals.

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 1 week ago

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

abdulrahuman5196 commented 1 week ago

@adelekennedy IMO, this could be lesser bug compared to other normal bugs, but reduced price of 4 times than normal bug seems little undervalued comparing to any normal bug.

dangrous commented 6 days ago

As long as this doesn't reintroduce https://github.com/Expensify/App/issues/49899 I think this works for me!

bernhardoj commented 6 days ago

I agree with @abdulrahuman5196. Not that it's unusable, but it's clearly noticeable and annoying. The previous message shows on every character we type.

https://github.com/user-attachments/assets/b309c499-7cb5-47fe-a829-7874cb032cfa

Btw, PR is ready