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

If app left open overnight, messages still display that were sent "Today" #3272

Closed
isagoico opened this issue Jun 1, 2021 · 25 comments · Fixed by #3573
Closed

If app left open overnight, messages still display that were sent "Today" #3272

isagoico opened this issue Jun 1, 2021 · 25 comments · Fixed by #3573
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

isagoico commented Jun 1, 2021

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


Expected Result:

Messages should update to show the correct date without the need of refreshing the page.

Actual Result:

Message that was sent the day before still shows that were sent "Today" instead of "Yesterday".

Action Performed:

  1. From account A send a message to account B
  2. Leave the app open overnight
  3. Check the same message on the day after.

Workaround:

No need. A refresh will update the message and show the correct date.

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.59-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork


From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1622558685411700

When I opened chat this morning it was on a group one with James and it says today at 9pm when the message was sent yesterday. Once I clicked out of the chat then returned it correctly showed as 'yesterday'. I reckon this has to do with updating in the background (possibly similar to issue below)

@MelvinBot
Copy link

Triggered auto assignment to @Jag96 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Jag96 Jag96 added Weekly KSv2 and removed Daily KSv2 labels Jun 1, 2021
@Jag96 Jag96 removed their assignment Jun 1, 2021
@Jag96 Jag96 added the External Added to denote the issue can be worked on by a contributor label Jun 1, 2021
@MelvinBot
Copy link

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 1, 2021
@kadiealexander
Copy link
Contributor

@MelvinBot
Copy link

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

@dklymenk
Copy link
Contributor

dklymenk commented Jun 3, 2021

The issue is caused by the fact that component ReportActionItemDate is not updated at midnight when rendered output should supposedly change.

https://github.com/Expensify/Expensify.cash/blob/6cf79acf86232024968354e166ea53d60eec8fda/src/pages/home/report/ReportActionItemDate.js#L13-L17

The component has one prop that never changes and thus it is never re-rendered.

I believe that chat apps normally avoid this kind of issue by simply not rendering the date on the messages and instead just grouping the messages by date and separating the groups with lines like on the screenshot below (slack). Having less date aware components is a huge help for sure.
DeepinScreenshot_select-area_20210603150319

However, I think I have a few possible solutions:

  1. Similar to what one can find via searching online for similar issues, first option is to add state to that component, name it currentDate or something and check every second or minute if current date has changed and update state accordingly. This will trigger component re-render and the wording will always be up-to-date.

  2. Similar to option 1 above we can instead create a HOC withCurrentDate that would in a similar manner have state and check for date updates regularly, it will then pass that date as a prop to ReportActionItemDate causing it to re-render.

  3. Both options 1 and 2 imply having a setInterval for EACH chat message which is not very good, which is why ideally we should have one setInterval running somewhere globally that would update some onyx record we can subscribe to in ReportActionItemDate via withOnyx HOC. Onyx record changes => components get re-rendered => win. I guess it will still make the app slower, but not to such extend as previous options.

@deetergp
Copy link
Contributor

deetergp commented Jun 3, 2021

This one makes me nervous enough that I want to get a second opinion. I thought the cost may outweigh the benefit here and we may want to rethink wether or not we want relative dates on every post. In any event, the person I want to look at this, @marcaaron, will be back in the office on Monday, so let's sit on this tell then.

@marcaaron
Copy link
Contributor

I am not really sure what level of granularity we are expecting in the relative dates from a product perspective. Seems like a good thing to bring to #expensify-open-source for discussion.

One other solution not mentioned is that we could update the relative times based on user activity instead of some arbitrary time elapsing (e.g. with the setInterval approach) and instead update all times when the app becomes active or a user moves their mouse.

@deetergp
Copy link
Contributor

deetergp commented Jun 7, 2021

Solid suggestion, @marcaaron. I agree that user activity is a better trigger than setInterval.

@dklymenk
Copy link
Contributor

dklymenk commented Jun 8, 2021

Ok, If using user interactions is preferred, I suggest the following:

Track events:

  • mousedown
  • mousemove
  • touchstart
  • keydown
  • scroll

And have a single callback function for all of them that will update the new date key in Onyx. Now we obviously will need to throttle the callback to make sure it doesn't get called let's say more often than 1 time per minute. Then as suggested above, I pass the date as prop to ReportActionItemDate via withOnyx.

This combo should be enough to make it so, if you leave a chat open overnight, get on your PC in the morning and move your mouse or press a key, the dates on all chats will re-render.

@marcaaron
Copy link
Contributor

Now we obviously will need to throttle the callback to make sure it doesn't get called let's say more often than 1 time per minute.

I think a minute is perhaps too little time and it could maybe even be something like 3 hours to start off. Agree that we can rate limit these various events with _.throttle() and just make sure to execute the callback on the leading edge.

Also, not sure if we need mousedown since mousemove most likely would occur first.

@dklymenk
Copy link
Contributor

dklymenk commented Jun 8, 2021

Also, not sure if we need mousedown since mousemove most likely would occur first.

I agree.

I think a minute is perhaps too little time and it could maybe even be something like 3 hours to start off.

Setting throttle to 3h will work for overnight case as described in the original post, but if one opens the app at 11PM, browses it a little and then goes afk for 2 hours, comes back and moves the mouse, the dates won't update because there is still 1 hour on throttle timer. If that's ok, we can go with 3 hour timer, no problem.

@marcaaron
Copy link
Contributor

Based on the issue ("app left overnight") I'd be Ok with assuming that we are mainly concerned with large amounts of time. My concern is that if we are updating very frequently then it will cause performance issues. But we can always tweak things later. @deetergp what do you think about this plan?

@deetergp
Copy link
Contributor

deetergp commented Jun 9, 2021

I think three hours sounds like a reasonable amount of time. I don't think we need to worry about the time rolling over at exactly the stroke of midnight in the user's timezone, and are more concerned about leaving the app open at the end of the day, then hopping in first thing the next morning. I think we have a solution we can move forward on.

@mallenexpensify
Copy link
Contributor

@deetergp should @kadiealexander hire @dklymenk or do we want more specifics on the proposed fix?

@deetergp
Copy link
Contributor

@mallenexpensify I think we have enough to move forward with hiring @dklymenk.

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Jun 11, 2021

Hired @dklymenk in Upwork (@kadiealexander is out of office, leaving it assigned to Kadie as Contributor Manager), please submit a PR for @deetergp to review

@deetergp
Copy link
Contributor

The PR should get randomly assigned to a reviewer, so assigning it to me to review would not be the correct course of action in general. I'm also out of the office all next week, so it would specifically not be helpful to get a speedy review. 😅

@mallenexpensify
Copy link
Contributor

Thanks @deetergp , had wires crossed.

@deetergp
Copy link
Contributor

No worries @mallenexpensify. And never fear, once we get used to this flow, it will change! 😂

dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jun 12, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jun 13, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jun 13, 2021
@dklymenk
Copy link
Contributor

I have submitted the PR #3573 with proposed changes. The original post mentions only Web, but I made an assumption that desktop app should have this fix too. For iOS and Android there is probably little sense sense in such fix, I would expect it's not very common to have this app open overnight on a phone. Please let me know if my assumptions are wrong on there.

@parasharrajat
Copy link
Member

cc @marcaaron #3272 (comment). I believe, it's better to test if the app auto-updates the time-date when it's in the background. It very common for me not to close the apps and put them in the background on mobile.

@dklymenk
Copy link
Contributor

@parasharrajat Good catch. If your phone has enough RAM and you don't switch between too many other applications, the react-native app will be in the memory and switching back to it normally doesn't cause a re-render.

@marcaaron If this would be required to implement, I think we can use utilize this function
https://github.com/Expensify/Expensify.cash/blob/9974e7984dc0845f144dbc919ca4220a6af302dd/src/libs/AppStateMonitor/index.js#L7-L31

@mallenexpensify
Copy link
Contributor

@kadiealexander paid this today. Sorry it took a while @dklymenk (we have a new process now so this shouldn't happen with new issues going forward)

@marcaaron
Copy link
Contributor

@parasharrajat @dklymenk Not really following what's being proposed. Do we need to create a new issue ?

@marcaaron
Copy link
Contributor

Oh just noticed those comments are very old so probably we do not need a new issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants