-
Notifications
You must be signed in to change notification settings - Fork 369
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
Try to mitigate missing relay list errors #5596
Try to mitigate missing relay list errors #5596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
line 183 at r1 (raw file):
} is SelectLocationUiState.ShowData -> { if (uiState.countries.isEmpty() && uiState.searchTerm.isNotEmpty()) {
I'm a bit confused about this one. Is it necessary? I mean if the countries list is Empty we should show the message no matter what right? Is there a case where the search term is empty and we don't have any countries? If so shouldn't it have a completely separate state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @Pururun)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
line 183 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
I'm a bit confused about this one. Is it necessary? I mean if the countries list is Empty we should show the message no matter what right? Is there a case where the search term is empty and we don't have any countries? If so shouldn't it have a completely separate state?
This happens when the list we got from the daemon is empty. This currently shows a text that there is no search results for an empty term. But will move this check to the view model.
f41a2f2
to
97b40cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 9 files reviewed, 1 unresolved discussion (waiting on @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
line 183 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
This happens when the list we got from the daemon is empty. This currently shows a text that there is no search results for an empty term. But will move this check to the view model.
Done
97b40cd
to
73aaa4f
Compare
3c521d6
to
16fe40f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 5 files at r1, 4 of 7 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)
16fe40f
to
90d3be2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @Rawa from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved
90d3be2
to
7efda2d
Compare
This PR tries to make situations where a user would see an empty relay list rarer by trying to mitigate some potential sources for this error in the app layer.
The two changes are:
SharingStarted.Eagerly
toSharingStarted.WhileSubscribed()
. This, according to my tests, delays the call to fetch the relay list by around 400 ms. This in my opinion could make things less likely to break since the call is made later and thus the call (or the answer) is less likely be lost.SelectLocationScreen
. This would mean that if the user still encounters this error it can be resolved when they opening the actual screen. It still would cause them to see an empty list for a second.This change is