-
Notifications
You must be signed in to change notification settings - Fork 17
UI reporting wrong connection state when VPN is running #48
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
Conversation
By checking if there's a VPN Manager instance from preferences whose name matches the extension's. If so, it sets the extension's session with its connection, and returns the current VPN status. Returns false, otherwise.
This will be used to both report the current connection state properly and configure the ExtensionAdpater's session before polling. This doesn't start or create a new VPN manager, it only checks for one that is already running in case the VPN is connected by the time the app cold starts.
📝 WalkthroughWalkthroughNetBirdApp now creates and cancels a MainActor-bound activationTask on activation/resign events to asynchronously load the current VPN extension state via NetworkExtensionAdapter.loadCurrentConnectionState(), apply it to the view model, then continue with existing checks and polling. A new async API was added to NetworkExtensionAdapter and project version/configs were bumped. Changes
Sequence Diagram(s)sequenceDiagram
participant App as NetBirdApp (Lifecycle)
participant Task as activationTask
participant Adapter as NetworkExtensionAdapter
participant Manager as NEVPNManager
participant ViewModel as ViewModel
App->>Task: cancel existing (if any)
App->>Task: start MainActor Task
activate Task
Task->>Adapter: loadCurrentConnectionState()
activate Adapter
Adapter->>Manager: loadFromPreferences()
Manager-->>Adapter: vpnManagers
Adapter->>Adapter: select matching extension\nupdate vpnManager & session
Adapter-->>Task: NEVPNStatus? (or nil)
deactivate Adapter
Task->>ViewModel: set extensionState (initialStatus)
Task->>App: checkExtensionState()
Task->>App: checkLoginRequiredFlag()
Task->>App: startPollingDetails()
deactivate Task
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@NetBird/Source/App/NetBirdApp.swift`:
- Around line 63-76: The async loadCurrentConnectionState() call can finish
after the app has become inactive and inadvertently trigger
startPollingDetails(); to fix, create and store the Task when handling
UIApplication.didBecomeActiveNotification (and the tvOS equivalent), keep a
cancellable reference on the view (or viewModel), and cancel that Task when
willResignActiveNotification fires; after awaiting loadCurrentConnectionState(),
check the current active state (or Task.isCancelled) before setting
viewModel.extensionState and before calling viewModel.checkExtensionState(),
viewModel.checkLoginRequiredFlag(), and viewModel.startPollingDetails() so
polling is not restarted while the app is backgrounded.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
When VPN is already running, the app is reporting a wrong UI state.
This happens because polling for connection status is happening before a session is established, and as it is, it is only being established if the user taps the button and goes to a Connected state.
checkExtensionStatewould eventually update extensionStatus to connected, and thus when the user tapped the button from a cold start, it'd attempt to stop a connection without the vpnManager even being set, so it'd do nothing.This PR adds a method to set the current connection state by filtering the list returned from
NETunnelProviderManager.loadAllFromPreferences()by the extension name and return the current VPN status.When app becomes active, this method will be called before polling starts to properly set the session and report the right connection state to UI.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.