fix(android): fix view state lifecycle around tunnel/auth#9621
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
I don't think this works. Have you tested all flows and combinations of connectOnStart?
- Sign in / out
- Disconnect VPN via system and relaunch app
I think I've tried this or a very similar implementation and there is a routing loop somewhere between the fragments.
|
Good catch. It appears that the behavior of the TunnelService changed on disconnect and doesn't shutdown any longer. This is causing the Should have a fix in a bit. |
jamilbk
changed the title
|
Tested:
|
| val token = repo.getTokenSync() | ||
|
|
||
| actionMutableLiveData.postValue( | ||
| if (authFlowLaunched || token != null) { | ||
| ViewAction.NavigateToSignIn | ||
| } else { | ||
| authFlowLaunched = true | ||
| ViewAction.LaunchAuthFlow("${config.authUrl}/${config.accountSlug}?state=$state&nonce=$nonce&as=client") | ||
| }, |
There was a problem hiding this comment.
This appears to be from a roundabout way to start the tunnel if the token was provided. I think it can be safely removed - this logic is handled in the checkTunnelState of the SplashViewModel now.
There was a problem hiding this comment.
Was able to clean up the logic in this file a bit.
|
|
||
| stopNetworkMonitoring() | ||
| stopDisconnectMonitoring() | ||
| stopSelf() |
There was a problem hiding this comment.
This was the cause of one loop.
There was a problem hiding this comment.
We have to tie tunnel state UP / DOWN to the service running because there isn't a good way to get the running service instance to call connect on it otherwise.
So we start/stop the service, which is probably correct since it shows/hides the VPN stick foreground notification.
There was a problem hiding this comment.
Nice catch, I didn't know about this.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the splash flow to prevent duplicate VPN permission prompts, introduces a first-launch flag to control tunnel connection, and ensures the VPN service stops itself when disconnected.
- Call
stopSelf()inTunnelServiceto properly terminate the service - Move tunnel state checks entirely into
onResumewith anisInitialLaunchflag inSplashFragmentand simplifySplashViewModellogic - Simplify
AuthViewModelto always launch the auth flow (removing prior token checks)
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt | Added stopSelf() after stopping monitors |
| kotlin/android/app/src/main/java/dev/firezone/android/features/splash/ui/SplashViewModel.kt | Removed internal launch flag, reordered and clarified state logic |
| kotlin/android/app/src/main/java/dev/firezone/android/features/splash/ui/SplashFragment.kt | Added isInitialLaunch field, moved check into onResume |
| kotlin/android/app/src/main/java/dev/firezone/android/features/auth/ui/AuthViewModel.kt | Always launch auth flow, removed token-based branch |
Comments suppressed due to low confidence (1)
kotlin/android/app/src/main/java/dev/firezone/android/features/auth/ui/AuthViewModel.kt:33
- The previous logic checked for an existing token or prior auth launch before navigating to sign-in, but the new code always starts the auth flow. This could trigger redundant auth attempts even when a valid token exists. Consider restoring the conditional branch to navigate directly to SignIn when
repo.getTokenSync()returns a non-null token or the auth flow has already been launched.
actionMutableLiveData.postValue(
…/splash/ui/SplashViewModel.kt Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Nice work, thank you for handling this.
onViewCreated()is called when the view initializes, and thenonResume()is called right after, in addition to anytime the view is shown again.To prevent showing the VPN permission activity twice, we remove the
checkTunnelState()from onViewCreated, allowing onlyonResume()to call it.A boolean flag is added to track whether this is the "first" launch of the app in order to determine whether to
connectOnStart.Fixes #9584