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

" user is typing ... " doesn't appear after opening and closing the RHP #25486

Closed
1 of 6 tasks
kavimuru opened this issue Aug 18, 2023 · 8 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@kavimuru
Copy link

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


Action Performed:

  1. Login with 2 users ( User A and User B ) in front of you at the same time.
  2. Start a chat between the two users.
  3. As User A click on settings to Open the RHP and then close it.
  4. Now go to User B and start typing.

Expected Result:

" user is typing... " message should appear for User A while User B is typing.

Actual Result:

After opening and closing the RHP, the " user is typing ... " message doesn't appear for User A while User B is typing.
Note: Refreshing the page or leaving and returning to the chat resolves the issue.
Notes:

  1. It doesn't matter which mode you switch to, whether from Most Recent to Focus or vice versa. The issue occurs after changing modes.
  2. Refreshing the page or leaving and returning to the chat resolves the issue

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.55-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Recording.5911.mp4
Screencast.from.10-08-23.03_22_46.1.webm

Expensify/Expensify Issue URL:
Issue reported by: @Ahmed-Abdella
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691544088203549

View all open jobs on GitHub

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 18, 2023
@eh2077
Copy link
Contributor

eh2077 commented Aug 18, 2023

Proposal

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

" user is typing ... " doesn't appear after changing the priority mode.

What is the root cause of that problem?

The root cause of this issue is that we unsubscribe the typing status event in the cleanup function of this useEffect here

Report.unsubscribeFromReportChannel(reportID);

And we don't re-subscribe the typing status event in this useEffect and in the other useEffect

useEffect(() => {
// Ensures subscription event succeeds when the report/workspace room is created optimistically.
// Check if the optimistic `OpenReport` or `AddWorkspaceRoom` has succeeded by confirming
// any `pendingFields.createChat` or `pendingFields.addWorkspaceRoom` fields are set to null.
// Existing reports created will have empty fields for `pendingFields`.
const didCreateReportSuccessfully = !props.report.pendingFields || (!props.report.pendingFields.addWorkspaceRoom && !props.report.pendingFields.createChat);
if (!didSubscribeToReportTypingEvents.current && didCreateReportSuccessfully) {
Report.subscribeToReportTypingEvents(reportID);
didSubscribeToReportTypingEvents.current = true;
}
}, [props.report, didSubscribeToReportTypingEvents, reportID]);

also doesn't re-subscribe the event because the dependency array

}, [props.report, didSubscribeToReportTypingEvents, reportID]);

doesn't trigger the effect to re-run.

So, the typing status event is unsubscribed and won't get re-subscribed as long as this useEffect is triggered to re-run.

Note we explicitly call the cleanup function here apart from being calling every re-render as the official doc mentions.

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

To fix this issue, we should re-subscribe the typing status event if it's unsubscribed by the cleanup function.

To achieve it,

  1. We can mutate
didSubscribeToReportTypingEvents.current = false;

after the line to unsubscribe the event.

  1. Add isReportFullyVisible to the dependency array of the useEffect to subscribe the typing status event. We can change this line

    }, [props.report, didSubscribeToReportTypingEvents, reportID]);

    to

    }, [props.report, didSubscribeToReportTypingEvents, reportID, isReportFullyVisible]);

    We can also remove didSubscribeToReportTypingEvents from the deps array as changing value of useRef doesn't trigger a re-render.

What alternative solutions did you explore? (Optional)

We can also merge the two useEffects, useEffect and useEffect, into one useEffect.

@dukenv0307
Copy link
Contributor

Proposal

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

"User is typing ... " doesn't appear after changing the priority mode.

What is the root cause of that problem?

  1. We have the logic to unsubscribe from this report's channel when opening any RHP

Report.unsubscribeFromReportChannel(reportID);

  1. Therefore, all reportUserIsTyping_ in Onyx is reset to an empty object
  2. As a result,in ReportTypingIndicator component usersTyping return from this line of code is an empty array

const usersTyping = useMemo(() => _.filter(_.keys(props.userTypingStatuses), (loginOrAccountID) => props.userTypingStatuses[loginOrAccountID]), [props.userTypingStatuses]);

  1. The numUsersTyping will be 0 and ReportTypingIndicator return null, which cause "user is typing ..." not showing

const numUsersTyping = _.size(usersTyping);

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

  1. In this cleanup function, we should set didSubscribeToReportTypingEvents.current to false

const cleanup = () => {
if (unsubscribe) {
unsubscribe();
}
Report.unsubscribeFromReportChannel(reportID);
};

  1. We also should add isReportFullyVisible in this condition since we only want to subcribe once the report is fully visible

if (!didSubscribeToReportTypingEvents.current && didCreateReportSuccessfully) {

What alternative solutions did you explore? (Optional)

@eh2077

This comment was marked as outdated.

@melvin-bot
Copy link

melvin-bot bot commented Aug 18, 2023

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 18, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@kavimuru
Copy link
Author

kavimuru commented Aug 18, 2023

@Pujan92 will this be fixed when this deployed to staging?
#18637

@Pujan92
Copy link
Contributor

Pujan92 commented Aug 18, 2023

Yes @kavimuru , some follow up change we can do which I have mentioned here.
cc: @gedu

not reproducible in dev env.

Cause for this issue which won't be reproducible now

Proposal

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

'User is typing' isn't shown once we lose focus from the report(not specifically required to change priority mode)

What is the root cause of that problem?

Currently, we are unsubscribing the Typing Pusher event at the wrong place. Cleanup function will be called whenever the dep gets changed but Typing event needs to be unsubscribed when the component unmounts. So the current issue is whenever the report focus gets lost it will trigger this cleanup and unsubscribe the typing event. Seems it persists after this PR
cc @hannojg

const cleanup = () => {
if (unsubscribe) {
unsubscribe();
}
Report.unsubscribeFromReportChannel(reportID);
};
newActionUnsubscribeMap[reportID] = cleanup;
return () => {
cleanup();
};
}, [isReportFullyVisible, reportID, scrollToBottom, setNewMarkerReportActionID]);

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

We need to move Report.unsubscribeFromReportChannel(reportID); here by creating the cleanup function which will unsubscribe typing event on unmount.

useEffect(() => {
openReportIfNecessary();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

@adelekennedy
Copy link

It seems like this was fixed by #18637 - let's close this for now

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
Projects
None yet
Development

No branches or pull requests

5 participants