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

PM message keeps refreshing infinitely #3869

Open
Maskedman99 opened this issue Feb 5, 2020 · 14 comments
Open

PM message keeps refreshing infinitely #3869

Maskedman99 opened this issue Feb 5, 2020 · 14 comments

Comments

@Maskedman99
Copy link
Contributor

test
Steps to reproduce:

  • Open PM and scroll down to bottom
  • Click on the + icon in compose box
  • Close the expanded options
  • swipe down on the chat screen

I am using Android, and the beta version of the app.

@jaskeerat789
Copy link

@chrisbobbe i would like to work on it!

@chrisbobbe
Copy link
Contributor

OK, thanks!

@shreyaspapi
Copy link

Hey @chrisbobbe and @jaskeerat789 ! if the problem is not solved yet I can help.

@gnprice
Copy link
Member

gnprice commented Feb 29, 2020

Interesting -- thanks @Maskedman99 for the report!

To make sure I understand it correctly: after you swipe down in that last step, you don't touch the screen again? But it shows the spinner, then the spinner disappears, then it comes back again, then it disappears again, etc., etc., in a loop that seems to just keep going?

I don't seem to reproduce this myself, either on a Pixel 4 running Android 10 and the current release, or an emulated Pixel 3 running Android 10 and an app version from master.

So I think the most valuable next step, for anyone who wants to work on this, would be to try to pin down what conditions it happens in and how to reproduce it.

@sumukhah
Copy link
Contributor

This issue is not only limited to the PM message list.
stream message list also behaves in the same way

ezgif-2-6034804061b2

@sumukhah
Copy link
Contributor

sumukhah commented Mar 16, 2020

@chrisbobbe, @gnprice, @jaskeerat789
I would like to work on the issue if @jaskeerat789 is not working on the issue

@rk-for-zulip
Copy link
Contributor

Given the lack of response from @jaskeerat789, I've unassigned them. @sumukhah, are you still interested?

@sumukhah
Copy link
Contributor

sumukhah commented Mar 20, 2020

To reproduce,

  1. Create a stream (or any empty chat list)
  2. type 51 messages.
  3. refresh the app.
  4. go to the stream/chat screen.

5.scroll up, scroll down as shown in the gif

ezgif-2-6034804061b2

a simple way to observe this is, replace

if (scrollY < config.messageListThreshold) {
dispatch(fetchOlder(narrow));
}

with

dispatch(fetchOlder(narrow));

i.e remove if condition at webViewEventHandler.js

@sumukhah
Copy link
Contributor

sumukhah commented Mar 20, 2020

chat screens having less number of messages (scrollY<4000),
fetchOlder, fetchNewer actions dispatched repeatedly..
but chat screens having less number of messages(<50) will not fall into this category, bcz the chat screen renders for the first time, it will set "caughtUp.newer", "caughtUp.older" to true.
due to this, conditional rendering fails at fetchActions.js and it wont call fetchMessage repeatedly.

@gnprice
Copy link
Member

gnprice commented Mar 24, 2020

Thanks @sumukhah for the investigation!

To reproduce,

  1. Create a stream (or any empty chat list)
  2. type 51 messages.
  3. refresh the app.
  4. go to the stream/chat screen.

5.scroll up, scroll down as shown in the gif

Hmm, I still don't reproduce. But there's enough uncertainty in these steps that I'm probably doing something different from what you are. Can you say more specifically exactly what you do to reproduce?

  • It's more important to have one clear repro recipe that works than to have a range of options with "or".
  • If you can find a recipe that doesn't involve typing out 51 messages, that will help a lot too 😉 What about navigating to a topic with a large number of unreads?
  • What does "type" mean -- do you send them from the app, from web, from the same user, from a different one?
  • What does "refresh the app" mean -- do you quit it and relaunch it?

Also, can you answer the questions I asked the original reporter above?

To make sure I understand it correctly: after you swipe down in that last step, you don't touch the screen again? But it shows the spinner, then the spinner disappears, then it comes back again, then it disappears again, etc., etc., in a loop that seems to just keep going?

@Maskedman99
Copy link
Contributor Author

Maskedman99 commented Mar 24, 2020

@gnprice I am not able to reproduce this issue in beta or the debug build, ( maybe because I have entered new messages and the condition @sumukhah mentioned has started failing ). My understanding of what exactly caused this issue before was wrong, and doesn't have anything to do with '+' icon in compose box.
Following @sumukhah suggestion removing that if condition in the webViewEventHandlers.js, the issue starts happening again,
Try creating a condition where if (scrollY < config.messageListThreshold) results in TRUE inside fetchMore() and you can reproduce the error.

Here's what I remember

  • Sorry, I didn't mean swipe down I meant scroll down, scroll down like you're trying to get to the latest ( most recent ) message.
  • Touch the screen till the first spinner comes out, then after a while the first spinner will disappear, and then after a brief pause with or without user interaction, the spinner jumps out again and does this repeatedly.
  • You have to make the first spinner come out, then touching or not touching the screen doesn't affect the result

@sumukhah
Copy link
Contributor

sumukhah commented Mar 24, 2020

@Maskedman99 @gnprice
I think having a visual example will clarify,
I have created a video,
where i created a stream "Testing 12345", sent(composed,t̶y̶p̶e̶) 50 messages (from 0:5 sec to 2:20 sec, you can skip)in the stream.

https://drive.google.com/file/d/11K-zxb9Z3ymm3ogXrnJfAKNuw5PFdwPC/view?usp=sharing

What about navigating to a topic with a large number of unreads?

optionally you can navigate to a stream/PM which has

  1. at least 50 messages (read or unread no difference)
  2. scrollY less than 4000 ( try printing scrollY of fetchMore function at webViewEventHandler.js)

@sumukhah
Copy link
Contributor

sumukhah commented Mar 24, 2020

If you can find a recipe that doesn't involve typing out 51 messages, that will help a lot too 😉

sure there is a way, set messagesPerRequest: 10, in Config.js.
navigate to a stream having less number of messages(less than 100, greater than 10).

@chrisbobbe
Copy link
Contributor

We've had another report of this, at #4033 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants