Message does not load if Internet is back. #3281
Comments
|
Thanks for opening an issue for that. |
Fixes zulip#3281 This simple change greatly improves the experience with spotty network connections. I have a small concern about the requests retry not being cancelled I find the compromise to be worth it. I am working on a more generic reqork of `tryUntilScuccessful` which will allow it to be cancelled and will include all of this function's invocations.
gnprice
added
a-message list
a-data-sync
Fixes zulip#3281. This simple change greatly improves the experience with spotty network connections. I have a small concern about the request's retry not being cancelled. I find the compromise to be worth it. I am working on a more generic rework of `tryUntilScuccessful` which will allow it to be cancelled and will include all of this function's invocations.
This reverts #3288, aka commit 7ed7e52 (and so re-opens #3281.) After this change, Jest would hang after all tests had completed. (Not sure how I missed that before merge -- did I neglect to run the tests? >_>) I noticed the problem a few commits later, and it took a bit of debugging to identify this test file, and then this commit, as the culprit. Some background I found helpful in debugging: jestjs/jest#1456 . The root of the issue seems to be that a lot of these tests fire off `fetchMessages` actions, or others that in turn invoke that one -- and then don't wait for them. Compounding that, many of them are doomed to fail at their HTTP requests because their fixture states have no accounts, so there's no realm to base the URLs on. Anyway, we don't knowingly merge changes that break things. Merging this was a mistake; take it back out. We can merge a version of this change once we have one that doesn't cause breakage.
|
The fix in #3288 broke tests 🤕 , so I've just reverted it. I think the actual code change in #3288 is fine -- it's the tests that are at fault. See abc7814 (shown in this thread, just above) for details. So the next step would be to pick up that change that was #3288, and go on to fix the tests so we can merge a fixed version. |
Currently, no attempt is made to retry fetching messages if it fails. Add `tryUntilSuccessful()` to `fetchMessages()` to retry fetching messages. Fixes zulip#3281.
|
On reconsideration, I think an automatic retry would not be a good solution here. A major reason for that is that when a message fetch fails, there may well be something wrong so that retrying would just fail again and again. Instead, when the fetch fails, we should show an error in place of the messages. That's #4186, just filed. With a retry button, you can have it retry if you think the issue may be transient (like because the network wasn't working and now it's back). And then if it keeps failing, you know something is wrong and that there isn't a need to sit there waiting for 30 more seconds to see if it's just slow. Related discussion at #4165 -- some of the circumstances are different but I think the core point is the same. |
|
Closing in favor of #4186 . |
If you open a message during weak internet connection, grey bars are displayed instead of the real meassage.
This state lasts forever even if Internet connection is back or change to a WiFi.
It also happens to me that the message was removed from the new message list after turning back. I wanted to go back and forth to trigger reload. I cannot confirm this now though. It is Zulip 21.2.106 on Android.
The text was updated successfully, but these errors were encountered: