Notify user when VPN disconnects due to re-authentication#106
Notify user when VPN disconnects due to re-authentication#106evgeniyChepelev wants to merge 6 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements iOS local notifications for login-required scenarios. It adds notification center delegate support, permission requests, VPN status observation, and notification scheduling from the main app when a session expires. Extension login-required signals trigger notifications that, when tapped, prompt re-authentication. Changes
Sequence DiagramsequenceDiagram
participant App as NetBirdApp
participant NotifCenter as NotificationCenter
participant MainVM as MainViewModel
participant VPN as VPN Adapter
participant Ext as Extension/<br/>PacketTunnelProvider
participant SysNotif as System<br/>Notifications
Ext->>Ext: Token expires<br/>needsLogin() = true
Ext->>Ext: onLoginRequired() callback
Ext->>Ext: signalLoginRequired()<br/>sets shared flag
VPN->>VPN: VPN status changes
VPN->>MainVM: .NEVPNStatusDidChange
MainVM->>MainVM: Check keyLoginRequired flag
MainVM->>MainVM: Verify app backgrounded
alt Permission Authorized
MainVM->>SysNotif: Schedule local notification<br/>(3-sec timer)
SysNotif->>App: Deliver notification
App->>App: userNotificationCenter(<br/>didReceive response)
App->>NotifCenter: Post netbirdLoginNotificationTapped
NotifCenter->>MainVM: Observe event
MainVM->>MainVM: Set showAuthenticationRequired = true
MainVM->>App: Navigate to Auth Flow
else Permission Denied
MainVM->>MainVM: Log, skip scheduling
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
NetbirdNetworkExtension/PacketTunnelProvider.swift (1)
286-313: Optional: extract the 3-second fallback delay into a named constant.The 3s delay is the cornerstone of the dual-path delivery contract (main app must outrun this timer). Hard-coding it here makes the coupling implicit; if it is ever tweaked, it must be remembered as a global timing assumption for the entire flow. Consider lifting it to
GlobalConstants(e.g.,notificationFallbackDelay) so the contract is documented in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdNetworkExtension/PacketTunnelProvider.swift` around lines 286 - 313, Extract the hard-coded 3s timeout in sendLoginNotificationBestEffort into a named constant on GlobalConstants (e.g., notificationFallbackDelay) and use that constant when constructing the UNTimeIntervalNotificationTrigger; keep GlobalConstants.notificationLoginRequired as-is for the identifier so the dual-path contract is documented centrally and the timing can be updated in one place without hunting for magic numbers.NetBird/Source/App/ViewModels/MainViewModel.swift (1)
135-179: Add adeinitthat removes theNEVPNStatusDidChangeobserver.
NotificationCenter.default.addObserver(forName:object:queue:using:)returns a token whose closure is retained byNotificationCenteruntil the token is explicitly removed. Although[weak self]prevents a strong reference toViewModel, the observer registration itself remains alive for the process lifetime, and if the view model is ever recreated (e.g. future refactor ofViewModelLoaderor for tests/SwiftUI previews) you'd silently accumulate stale observers all firing on a deadself.♻️ Proposed cleanup
+ `#if` os(iOS) + deinit { + if let observer = vpnStatusObserver { + NotificationCenter.default.removeObserver(observer) + } + } + `#endif` + init() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetBird/Source/App/ViewModels/MainViewModel.swift` around lines 135 - 179, The NEVPNStatusDidChange observer registered via NotificationCenter.default.addObserver in init stores a token in vpnStatusObserver but is never removed; add a deinit that checks vpnStatusObserver and calls NotificationCenter.default.removeObserver(_:) to remove that token (or call NotificationCenter.default.removeObserver(self) if you prefer) to prevent accumulating stale observers; reference vpnStatusObserver, init, deinit, NotificationCenter.default.addObserver(forName:object:queue:using:) and handleVPNStatusChangeForNotification() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@NetBird/Source/App/ViewModels/MainViewModel.swift`:
- Around line 135-179: The NEVPNStatusDidChange observer registered via
NotificationCenter.default.addObserver in init stores a token in
vpnStatusObserver but is never removed; add a deinit that checks
vpnStatusObserver and calls NotificationCenter.default.removeObserver(_:) to
remove that token (or call NotificationCenter.default.removeObserver(self) if
you prefer) to prevent accumulating stale observers; reference
vpnStatusObserver, init, deinit,
NotificationCenter.default.addObserver(forName:object:queue:using:) and
handleVPNStatusChangeForNotification() when making the change.
In `@NetbirdNetworkExtension/PacketTunnelProvider.swift`:
- Around line 286-313: Extract the hard-coded 3s timeout in
sendLoginNotificationBestEffort into a named constant on GlobalConstants (e.g.,
notificationFallbackDelay) and use that constant when constructing the
UNTimeIntervalNotificationTrigger; keep
GlobalConstants.notificationLoginRequired as-is for the identifier so the
dual-path contract is documented centrally and the timing can be updated in one
place without hunting for magic numbers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2403e005-65c0-47a7-b9ad-cd78d96aa20f
📒 Files selected for processing (7)
NetBird.xcodeproj/project.pbxprojNetBird/Source/App/NetBirdApp.swiftNetBird/Source/App/ViewModels/MainViewModel.swiftNetbirdKit/ConnectionListener.swiftNetbirdKit/GlobalConstants.swiftNetbirdNetworkExtension/NetBirdAdapter.swiftNetbirdNetworkExtension/PacketTunnelProvider.swift
|
/testflight |
|
TestFlight builds uploaded |
|
/testflight |
3 participants
Summary
UNUserNotificationCenterinsideNEPacketTunnelProvideralways reports.notDetermined(checks the extension bundle, not the app) — notificationsscheduled from the extension were silently dropped.
Dual-path delivery:
NEVPNStatusDidChangeobserver) cancels the extension's pendingrequest and delivers its own immediately when the app is backgrounded
Also fixed:
notification — previously only the startup check was handled
How to test
→ "VPN Disconnected" banner appears, no duplicates
Summary by CodeRabbit
New Features
Bug Fixes