Skip to content

android: defer vpn permission until activity is resumed #647

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

Merged
merged 1 commit into from
May 12, 2025
Merged

Conversation

kari-ts
Copy link
Collaborator

@kari-ts kari-ts commented May 9, 2025

Right now, we register the launcher in MainActivity.onCreate(), inject this into the ViewModel, then show the launcher in MainView. There is no guarantee that the activity is in RESUMED when the Composable runs, showing the launcher. This can lead to a silent RESULT_CANCELED on some OEMs. The fix is to add a lifecycle-aware wrapper that defers the launch.

Updates tailscale/tailscale#15419

@kari-ts kari-ts requested a review from barnstar May 9, 2025 19:04
Copy link

review-ai-agent bot commented May 9, 2025

Pull Request Revisions

RevisionDescription
r2
VPN permission request deferred to resumeAdded lifecycle-aware deferral of VPN permission launcher to prevent potential silent cancellation on Android
r1
VPN permission request flow refactoredIntroduced new maybeRequestVpnPermission() method and LaunchVpnPermissionIfNeeded composable to improve VPN permission request lifecycle management

✅ AI review completed for r2
Help React with emojis to give feedback on AI-generated reviews:
  • 👍 means the feedback was helpful and actionable
  • 👎 means the feedback was incorrect or unhelpful
💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at [email protected].

Comment on lines +63 to +64
private val _requestVpnPermission = MutableStateFlow(false)
val requestVpnPermission: StateFlow<Boolean> = _requestVpnPermission

Choose a reason for hiding this comment

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

When initializing the _requestVpnPermission MutableStateFlow, consider explicitly setting the initial value to false for clarity rather than relying on the default value. This would make the initial state more obvious to future readers.

Right now, we register the launcher in MainActivity.onCreate(), inject this into the ViewModel, then show the launcher in MainView.
There is no guarantee that the activity is in RESUMED when the Composable runs, showing the launcher. This can lead to a silent RESULT_CANCELED on some OEMs.
The fix is to add a lifecycle-aware wrapper that defers the launch.

Updates tailscale/tailscale#15419

Signed-off-by: kari-ts <[email protected]>
@kari-ts kari-ts merged commit d3f34c5 into main May 12, 2025
5 checks passed
@kari-ts kari-ts deleted the kari/vpnperm branch May 12, 2025 16:22
kari-ts added a commit that referenced this pull request May 12, 2025
Right now, we register the launcher in MainActivity.onCreate(), inject this into the ViewModel, then show the launcher in MainView.
There is no guarantee that the activity is in RESUMED when the Composable runs, showing the launcher. This can lead to a silent RESULT_CANCELED on some OEMs.
The fix is to add a lifecycle-aware wrapper that defers the launch.

Updates tailscale/tailscale#15419

Signed-off-by: kari-ts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants