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

Handle Device revoked on connect screen #5604

Merged

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Dec 19, 2023

This PR handles the event of a device being revoked on the Connect screen, a regression from migrating to compose navigation. It also makes sure that we properly logout the user when "Go to login" is pressed in DeviceRevoked screen.


This change is Reviewable

Copy link

linear bot commented Dec 19, 2023

@Rawa Rawa force-pushed the device-revoked-is-not-handled-on-connectscreen-droid-600 branch from dfd73cf to 1dd86c8 Compare December 19, 2023 15:09
@Rawa Rawa force-pushed the device-revoked-is-not-handled-on-connectscreen-droid-600 branch from 1dd86c8 to b18b337 Compare December 19, 2023 15:13
@Rawa Rawa self-assigned this Dec 20, 2023
@Rawa Rawa added the Android Issues related to Android label Dec 20, 2023
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModel.kt line 50 at r2 (raw file):

            )

    private val _uiSideEffect = Channel<DeviceRevokedSideEffect>(1, BufferOverflow.DROP_OLDEST)

Any specific reason to use a Channel instead of a shared flow?

Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModel.kt line 50 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Any specific reason to use a Channel instead of a shared flow?

Yes! We've migrated almost all SharedFlows over after discovering a navigational issue where the stream would miss messages. Basically a SharedFlow wont buffer if there is no subscribers whilst a channel may. You can read a bit more about it here:
Kotlin/kotlinx.coroutines#3002

Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModel.kt line 50 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Yes! We've migrated almost all SharedFlows over after discovering a navigational issue where the stream would miss messages. Basically a SharedFlow wont buffer if there is no subscribers whilst a channel may. You can read a bit more about it here:
Kotlin/kotlinx.coroutines#3002

Clarifiycation: All uiSideEffect sharedflows has been migrated over.

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeviceRevokedViewModel.kt line 50 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Clarifiycation: All uiSideEffect sharedflows has been migrated over.

Ah right I had missed that we had migrated over those.

@Pururun Pururun force-pushed the device-revoked-is-not-handled-on-connectscreen-droid-600 branch from b18b337 to a1e4de9 Compare January 3, 2024 15:34
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun force-pushed the device-revoked-is-not-handled-on-connectscreen-droid-600 branch 2 times, most recently from 4dbb833 to 3406902 Compare January 4, 2024 11:21
@Pururun Pururun force-pushed the device-revoked-is-not-handled-on-connectscreen-droid-600 branch from 3406902 to 301d8a7 Compare January 4, 2024 13:59
@Pururun Pururun merged commit aa98204 into main Jan 4, 2024
@Pururun Pururun deleted the device-revoked-is-not-handled-on-connectscreen-droid-600 branch January 4, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants