feat: implement battery optimizations with adaptive polling #38
Conversation
- Add adaptive polling intervals (10s/20s/60s based on state) - Implement hash-based status change detection - Reduce extension state checks to 30s interval - Add background mode optimization - Move polling operations to background queue - Fix threading issues with main thread dispatch Reduces battery consumption by ~30-50% through ~90% reduction in polling requests.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughConsolidates lifecycle handling to a single ScenePhase watcher; adds UI immediate/short polling on connect and throttled extension-state polling in the view model; implements adaptive, hash-driven polling with dynamic intervals and background/inactive modes in the network extension adapter; makes two GlobalConstants public. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as NetBirdApp
participant VM as MainViewModel
participant Adapter as NetworkExtensionAdapter
participant Provider as NE Provider / IPC
Note over App,VM: ScenePhase change (active / inactive / background)
App->>VM: onChange(scenePhase)
VM->>VM: throttle check / immediate "Connecting" when initiating
alt entering foreground (.active)
VM->>Adapter: setBackgroundMode(false)
VM->>Adapter: setInactiveMode(false)
VM->>Adapter: checkExtensionState() / startPollingDetails()
else entering inactive (.inactive)
VM->>Adapter: setInactiveMode(true)
VM->>Adapter: startPollingDetails()
else entering background (.background)
VM->>Adapter: setBackgroundMode(true)
VM->>Adapter: stopTimer()
end
Adapter->>Adapter: fetchData on pollingQueue -> decode StatusDetails
Adapter->>Adapter: calculateStatusHash(status)
alt status changed
Adapter->>Adapter: switch to fast interval, reset stability counter
Adapter->>VM: update status details (ip/fqdn/peers)
VM->>VM: `@MainActor` UI updates (Connected/Connecting/Disconnected)
else status stable
Adapter->>Adapter: increment consecutiveStablePolls -> increase interval
end
Adapter->>Provider: request status (NE IPC)
Provider-->>Adapter: StatusDetails
Adapter->>Adapter: restartTimerIfNeeded (schedule next fetch)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NetBird/Source/App/NetBirdApp.swift (1)
33-66: Remove duplicated lifecycle hooks to avoid double polling starts/stops.You now have two
.onReceive(...didBecomeActive...)blocks and two.onReceive(...willResignActive...)blocks (new at Line 33-43, existing at Line 58-66). This can triggerstartPollingDetails()/stopPollingDetails()twice per transition and undermine the battery gains.Suggested consolidation (keep just
scenePhase, drop both NotificationCenter observers):MainView() .environmentObject(viewModel) - .onReceive(NotificationCenter.default.publisher(for: UIApplication.didBecomeActiveNotification)) {_ in - print("App is active!") - viewModel.networkExtensionAdapter.setBackgroundMode(false) - viewModel.checkExtensionState() - viewModel.startPollingDetails() - } - .onReceive(NotificationCenter.default.publisher(for: UIApplication.willResignActiveNotification)) {_ in - print("App is inactive!") - viewModel.networkExtensionAdapter.setBackgroundMode(true) - viewModel.stopPollingDetails() - } .onChange(of: scenePhase) { newPhase in switch newPhase { case .background: print("App moved to background") viewModel.networkExtensionAdapter.setBackgroundMode(true) + viewModel.stopPollingDetails() case .active: print("App became active") viewModel.networkExtensionAdapter.setBackgroundMode(false) + viewModel.checkExtensionState() + viewModel.startPollingDetails() case .inactive: - break + viewModel.stopPollingDetails() @unknown default: break } } - .onReceive(NotificationCenter.default.publisher(for: UIApplication.didBecomeActiveNotification)) {_ in - print("App is active!") - viewModel.checkExtensionState() - viewModel.startPollingDetails() - } - .onReceive(NotificationCenter.default.publisher(for: UIApplication.willResignActiveNotification)) {_ in - print("App is inactive!") - viewModel.stopPollingDetails() - }
🧹 Nitpick comments (4)
NetBird/Source/App/NetBirdApp.swift (1)
33-56: PreferLogger(and gate noisy logs) for lifecycle/polling traces.
print(...)in lifecycle transitions will spam device logs in normal usage; consideros.Loggerand/or compiling out in release.NetBird/Source/App/ViewModels/MainViewModel.swift (1)
147-156: Avoid callingisLoginRequired()twice per update tick.This is likely non-trivial work (client init); compute once and reuse:
- print("Status: \(details.managementStatus) - Extension: \(self.extensionState) - LoginRequired: \(self.networkExtensionAdapter.isLoginRequired())") + let loginRequired = self.networkExtensionAdapter.isLoginRequired() + print("Status: \(details.managementStatus) - Extension: \(self.extensionState) - LoginRequired: \(loginRequired)") - if details.managementStatus == .disconnected && self.extensionState == .connected && self.networkExtensionAdapter.isLoginRequired() { + if details.managementStatus == .disconnected && self.extensionState == .connected && loginRequired { self.networkExtensionAdapter.stop() self.showAuthenticationRequired = true }NetbirdKit/NetworkExtensionAdapter.swift (2)
307-318: Status hash likely misses “meaningful changes” and is order-sensitive.
calculateStatusHashignores fields you later compare/display (e.g.,relayed,direct,connStatusUpdate, routes), and will also treat reorderings ofpeerInfoas changes. Consider hashing a stable, sorted projection of peers and include the fields that should speed polling up.
320-364: Timer restart path currently causes an extra immediate fetch; consider restarting without re-fetch.
restartTimerIfNeeded → startTimertriggersstartTimer’s “Initial fetch” (Line 340-342) even though you already have freshdecodedStatus. This adds avoidable requests on each interval shift.A small refactor: split “schedule timer” from “initial fetch”, and have restarts only reschedule.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
NetBird/Source/App/NetBirdApp.swift(1 hunks)NetBird/Source/App/ViewModels/MainViewModel.swift(1 hunks)NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/NetBirdApp.swift (2)
NetbirdKit/NetworkExtensionAdapter.swift (1)
setBackgroundMode(372-382)NetBird/Source/App/ViewModels/MainViewModel.swift (3)
checkExtensionState(178-188)startPollingDetails(119-314)stopPollingDetails(174-176)
🔇 Additional comments (2)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
132-135: Verify authentication prompt condition after throttling extension-state checks.
showAuthenticationRequired = trueis triggered whenextensionState == .disconnected && extensionStateText == "Connected"; with extension-state checks now throttled, this may be hit due to temporarily staleextensionStateTextrather than a real auth-required condition.NetbirdKit/NetworkExtensionAdapter.swift (1)
366-381: Reset more adaptive-polling state on stop/background transitions (verify desired behavior).
stopTimer()resetsconsecutiveStablePollsandcurrentPollingInterval, but notlastStatusHash/lastTimerInterval. If the app stops and later restarts, the first poll may be treated as “stable” and not ramp to fast polling when you’d want it most.
- Replace DispatchQueue.main.async with Task { @mainactor in } in MainViewModel
- Serialize polling state mutations through pollingQueue to prevent race conditions
- Maintains 100% of battery optimization benefits while improving code quality
- Use Task { @mainactor in } instead of DispatchQueue.main.async in startPollingDetails
- Improves concurrency safety and works better with strict concurrency checks
- Maintains 100% of battery optimization benefits
- Remove duplicate didBecomeActiveNotification handler (was registered twice) - Remove duplicate willResignActiveNotification handler (was registered twice) - Keep onChange(scenePhase) for background-specific state handling - Prevents redundant callbacks and potential state inconsistencies
- Access all state variables (currentPollingInterval, consecutiveStablePolls, lastStatusHash, isInBackground, lastTimerInterval) only from pollingQueue - Use pollingQueue.sync in startTimer to safely read state - Use pollingQueue.async in stopTimer and setBackgroundMode to safely modify state - Prevents data races and unpredictable polling behavior - Maintains 100% of battery optimization benefits
- Add checkExtensionState() and startPollingDetails() to .active case - Add stopPollingDetails() to .background case - Ensures polling management works even if notifications fail to fire - Provides redundant mechanism for app lifecycle management - Maintains 100% of battery optimization benefits
- Remove onReceive(didBecomeActiveNotification) - redundant with scenePhase .active - Remove onReceive(willResignActiveNotification) - redundant with scenePhase .background - Keep only onChange(scenePhase) which covers all app lifecycle events - Prevents duplicate calls to startPollingDetails(), stopPollingDetails(), and setBackgroundMode() - Eliminates redundant network requests and potential race conditions - Uses modern SwiftUI-native approach - Maintains 100% of battery optimization benefits
…ace condition - Read isInBackground via pollingQueue.sync before DispatchQueue.main.async - Store value in local variable backgroundState to avoid reading from main thread - Prevents race condition where isInBackground could change while being read - Maintains thread-safety guarantee that all state access happens on pollingQueue - Maintains 100% of battery optimization benefits
…ters - Add private startTimer variant that accepts interval and backgroundState as parameters - restartTimerIfNeeded now captures state values on pollingQueue and passes them directly - Avoids pollingQueue.sync call from main thread when restartTimerIfNeeded triggers startTimer - Prevents potential deadlock: pollingQueue → main.async → pollingQueue.sync - Maintains thread-safety and 100% of battery optimization benefits
- Change lastTimerInterval update from async to sync in startTimer - Prevents race condition where restartTimerIfNeeded could read stale value - Safe because values are already captured on pollingQueue before sync call - Maintains thread-safety and 100% of battery optimization benefits
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
132-135: Clarify the authentication prompt condition.The condition checks if
extensionState == .disconnectedwhileextensionStateText == "Connected". This seems to detect a stale UI state, but the logic is subtle and could benefit from a comment explaining when this scenario occurs (e.g., "Extension disconnected unexpectedly while UI showed connected state").Apply this diff to add clarifying documentation:
+// Detect when extension disconnected unexpectedly while UI was showing connected state if self.extensionState == .disconnected && self.extensionStateText == "Connected" { self.showAuthenticationRequired = true self.extensionStateText = "Disconnected" }NetbirdKit/NetworkExtensionAdapter.swift (1)
371-373: Consider consistent weak self usage in pollingQueue.sync blocks.Line 371 uses
[weak self]inpollingQueue.sync, while line 379 does not. Whilesyncblocks have deterministic lifetime (they complete before the closure returns), usingweak selfconsistently would be safer and more maintainable.Apply this diff for consistency:
} else { // Called directly, must read from pollingQueue (but this is safe as we're not in a deadlock situation) var intervalValue: TimeInterval = minPollingInterval var backgroundValue: Bool = false - pollingQueue.sync { + pollingQueue.sync { [weak self] in + guard let self = self else { return } backgroundValue = isInBackground intervalValue = backgroundValue ? backgroundPollingInterval : currentPollingInterval lastTimerInterval = intervalValue } intervalToUse = intervalValue backgroundStateToUse = backgroundValue }Also applies to: 379-383
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
NetBird/Source/App/NetBirdApp.swift(1 hunks)NetBird/Source/App/ViewModels/MainViewModel.swift(1 hunks)NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
NetbirdKit/NetworkExtensionAdapter.swift (4)
startTimer(349-351)startTimer(353-405)isLoginRequired(109-115)stop(148-150)
🔇 Additional comments (11)
NetBird/Source/App/NetBirdApp.swift (1)
33-48: LGTM! Clean lifecycle consolidation.The centralized
scenePhasehandling properly coordinates background mode, extension state checks, and polling lifecycle. This is a cleaner pattern than the previous notification-based approach.NetBird/Source/App/ViewModels/MainViewModel.swift (4)
115-117: LGTM! Throttling implementation is sound.The 30-second throttling interval for extension state checks aligns with the battery optimization goals. Using
Date.distantPastensures the first check executes immediately.
124-130: Previous review comment addressed.The use of
Task { @MainActor in ... }instead ofDispatchQueue.main.asyncproperly ensures main-actor execution and satisfies Swift concurrency requirements. The throttling logic correctly limits extension state checks to the 30-second interval.
137-152: LGTM! Status update logic is correct.The change detection and persistence logic properly avoids unnecessary UserDefaults writes by checking for actual changes before updating. The debug logging provides useful diagnostics.
159-169: LGTM! Efficient peer info update logic.The sorted comparison with detailed field checking prevents unnecessary UI updates, reducing battery consumption from redundant view refreshes.
NetbirdKit/NetworkExtensionAdapter.swift (6)
20-34: LGTM! Well-structured polling state.The polling state variables are properly documented with clear access requirements, and the interval values (10s, 20s, 60s) align with the PR objectives. Using a dedicated
pollingQueuewith.utilityQoS is appropriate for background work.
261-305: Previous review comment addressed.The response handling is now properly serialized through
pollingQueue.asyncat line 265, preventing race conditions on polling state mutations. The adaptive interval logic correctly resets to fast polling on changes and gradually transitions to stable polling after 3 consecutive stable polls.
327-347: LGTM! Deadlock-safe timer restart logic.The approach of capturing state values on
pollingQueue(lines 336-337) before dispatching to the main thread prevents potential deadlocks from callingpollingQueue.syncon the main thread. The 2-second threshold avoids excessive timer churn.
353-405: LGTM! Well-documented deadlock avoidance.The conditional parameter handling (lines 365-386) effectively prevents deadlocks by avoiding
pollingQueue.synccalls from the main thread when called viarestartTimerIfNeeded. The comments clearly explain the rationale. Timer creation on the main thread (line 389) is correct for RunLoop integration, and scheduling fetches onpollingQueue(line 395) maintains proper serialization.
407-415: LGTM! Proper state cleanup.The state reset on
pollingQueueensures thread-safe cleanup and prepares for the next polling session with initial values.
417-431: LGTM! Lazy background mode transition.The background mode change is properly serialized through
pollingQueue, and the lazy restart approach (waiting for the nextfetchDatacall) avoids immediate timer manipulation complexity. The logging provides useful diagnostics.
…iming - Add hasStoppedForLoginFailure flag to prevent multiple stop() calls - Flag is set when stop() is called for login failure condition - Flag is reset when extensionState changes to .disconnected - Prevents repeated stop() calls when condition remains true between polling cycles - Fixes timing mismatch between managementStatus and extensionState updates
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
NetBird/Source/App/ViewModels/MainViewModel.swift (2)
142-167: Avoid callingisLoginRequired()on the main actor (and avoid double-calling it).
Line 152 and Line 160-163 callself.networkExtensionAdapter.isLoginRequired()on the main actor (and effectively twice in the “changed details” path). GivenisLoginRequired()constructs a client (seeNetbirdKit/NetworkExtensionAdapter.swift), this is a good candidate to (a) compute once, and (b) compute off-main, then use the boolean for UI decisions.At minimum, compute once per update block:
- print("Status: \(details.managementStatus) - Extension: \(self.extensionState) - LoginRequired: \(self.networkExtensionAdapter.isLoginRequired())") + let loginRequired = self.networkExtensionAdapter.isLoginRequired() + print("Status: \(details.managementStatus) - Extension: \(self.extensionState) - LoginRequired: \(loginRequired)") ... - if details.managementStatus == .disconnected && - self.extensionState == .connected && - self.networkExtensionAdapter.isLoginRequired() && + if details.managementStatus == .disconnected && + self.extensionState == .connected && + loginRequired && !self.hasStoppedForLoginFailure {Longer-term, I’d move that computation into the adapter’s background/polling queue (or cache it with a TTL) and only publish the boolean to the view-model.
170-180: Peer sorting/diffing on the main actor may become a UI hot spot.
details.peerInfo.sorted(...)andelementsEqual(...)run inside the@MainActortask. IfpeerInfocan be large, consider doing sort + comparison off-main, then only assigningpeerViewModel.peerInfoon the main actor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
NetBird/Source/App/ViewModels/MainViewModel.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
NetbirdKit/NetworkExtensionAdapter.swift (4)
startTimer(349-351)startTimer(353-405)isLoginRequired(109-115)stop(148-150)
🔇 Additional comments (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
122-134: Main-actor hop in polling callback looks correct.
UsingTask { @MainActor in ... }here is the right shape for an adapter callback that may arrive off-main, and the 30s throttle forcheckExtensionState()is straightforward.
…onale - Document that hash includes only core connectivity fields (ip, fqdn, managementStatus, peer.ip, peer.connStatus, peer count) - Explain deliberate omission of peer.relayed, peer.direct, peer.connStatusUpdate, and peer.routes - Clarify hash is used for battery optimization: major connectivity changes trigger fast (10s) polling, while secondary/visual-only updates use slower intervals - Reference that MainViewModel performs more detailed comparisons for UI updates
…upling - Reset hasStoppedForLoginFailure when extensionState == .disconnected, independent of extensionStateText - Prevents flag from remaining stuck if extensionStateText doesn't match (string drift / future UI changes) - Decouples flag reset logic from UI text state - Keeps existing UX logic for extensionStateText update separate - Ensures future stops are not suppressed due to flag being stuck
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
NetbirdKit/NetworkExtensionAdapter.swift (2)
20-34: Consider avoiding a sharedPropertyListDecoderacross concurrent decodes.
decoderis shared, whilegetRoutes(...)andfetchData(...)may decode concurrently; Foundation decoders aren’t clearly documented as thread-safe. Prefer a local decoder per decode, or serialize all decoding on a single queue.- let decoder = PropertyListDecoder() + private let decoder = PropertyListDecoder()(If you want to be safest, instantiate
PropertyListDecoder()inside each decode block instead of sharing one.)
358-410: Timer thread-affinity: ensure invalidate/create always happens on main (or switch toDispatchSourceTimer).
You already create/schedule theTimeron main; keep all timer invalidation on main as well, and considerDispatchSourceTimeronpollingQueueto eliminate main-runloop coupling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🔇 Additional comments (3)
NetbirdKit/NetworkExtensionAdapter.swift (3)
314-330: Hash scoping comment is clear and matches the intent.
This nicely documents the “battery vs UI fidelity” tradeoff and should prevent future regressions.
422-436: Background transition defers interval change until “next fetch”; verify UX on returning to foreground.
If the timer is at 60s and you come back to foreground, make sure something (e.g.,startTimer) triggers an immediate refresh rather than waiting up to 60s.
261-305: The code is already safe as designed.fetchData()intentionally completes onpollingQueue(background thread), but its only caller—MainViewModel.startPollingDetails()—properly marshals all results and state mutations to the main thread usingTask { @MainActor in ... }. The completion callback is not directly invoked for UI updates; instead, it passes data to the ViewModel, which handles the main-thread requirement.Adding
DispatchQueue.main.asyncinsidefetchData()would create redundant marshaling and unnecessarily couple the method to always require main-thread completion semantics, preventing flexible use by different callers. The current design—where background work completes on a background queue and the caller chooses threading context—is the correct pattern.Likely an incorrect or invalid review comment.
…imer() - Add isPollingActive Bool field, only mutated and read on pollingQueue - Set isPollingActive = true when starting timer (both code paths) - Set isPollingActive = false in stopTimer() - Bail early in restartTimerIfNeeded if isPollingActive is false (checked on pollingQueue) - Prevents in-flight fetchResponse from recreating timer after stopTimer() was called - Ensures all accesses to isPollingActive occur on pollingQueue to avoid races
- Set isPollingActive = true when starting timer in both code paths (provided values and direct call) - Ensures flag is set synchronously on pollingQueue to prevent race conditions - Completes the isPollingActive flag implementation
…Timer - Change setBackgroundMode to use pollingQueue.sync instead of async - Ensures isInBackground is updated before startTimer reads it - Fixes timing issue where startPollingDetails immediately calls startTimer after setBackgroundMode - Prevents wrong polling interval calculation due to stale isInBackground value - All state mutations still happen on pollingQueue, maintaining thread-safety
…ondition - Change stopTimer to use pollingQueue.sync instead of async for isPollingActive - Ensures flag is set immediately before in-flight fetchData callbacks can check it - Prevents restartTimerIfNeeded from checking isPollingActive while it's still true - Prevents timer from being recreated after stopTimer() was called - Fixes race condition where async flag update could be delayed
|
Thanks for you contribution @uid0c0der and welcome to github. Can you please confirm which version of NetBird's Go code this was built against? Were you able to confirm the build and if it was working against a valid NetBird deployment? |
Thank you for the welcome. NetBird Go version: Build confirmation: Testing results: Edit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
NetbirdKit/NetworkExtensionAdapter.swift (2)
364-418: Consider ensuringtimer.invalidate()is called on main thread.Line 365 calls
timer.invalidate()outside theDispatchQueue.main.asyncblock. While current call sites (from@MainActorViewModel orrestartTimerIfNeeded's main dispatch) are safe, the function itself doesn't enforce this precondition.For defensiveness, consider moving the invalidation inside the main dispatch or adding a precondition:
private func startTimer(interval: TimeInterval?, backgroundState: Bool?, completion: @escaping (StatusDetails) -> Void) { - self.timer.invalidate() - - // Initial fetch - self.fetchData(completion: completion) + // Invalidate on main thread where timer was scheduled + DispatchQueue.main.async { [weak self] in + self?.timer.invalidate() + } + // Initial fetch + self.fetchData(completion: completion)Alternatively, document that this function must be called from the main thread.
420-430: Same thread safety concern fortimer.invalidate()instopTimer().Line 421 invalidates the timer without explicitly ensuring execution on the main thread. Current callers (from
MainViewModel.close()) dispatch to main, but the function could be more defensive:func stopTimer() { - self.timer.invalidate() + DispatchQueue.main.async { [weak self] in + self?.timer.invalidate() + } // Reset state variables and set isPollingActive to false synchronouslyOr combine with the sync block if you want stop to be synchronous for the caller.
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
177-188: Inconsistent indentation within the Task block.Lines 180-188 have reduced indentation compared to the surrounding code within the
Task { @MainActor in }block. Consider aligning for consistency:self.statusDetailsValid = true - let sortedPeerInfo = details.peerInfo.sorted(by: { a, b in - a.ip < b.ip - }) - if sortedPeerInfo.count != self.peerViewModel.peerInfo.count || !sortedPeerInfo.elementsEqual(self.peerViewModel.peerInfo, by: { a, b in - a.ip == b.ip && a.connStatus == b.connStatus && a.relayed == b.relayed && a.direct == b.direct && a.connStatusUpdate == b.connStatusUpdate && a.routes.count == b.routes.count - }) { - print("Setting new peer info: \(sortedPeerInfo.count) Peers") - self.peerViewModel.peerInfo = sortedPeerInfo - } + let sortedPeerInfo = details.peerInfo.sorted(by: { a, b in + a.ip < b.ip + }) + if sortedPeerInfo.count != self.peerViewModel.peerInfo.count || !sortedPeerInfo.elementsEqual(self.peerViewModel.peerInfo, by: { a, b in + a.ip == b.ip && a.connStatus == b.connStatus && a.relayed == b.relayed && a.direct == b.direct && a.connStatusUpdate == b.connStatusUpdate && a.routes.count == b.routes.count + }) { + print("Setting new peer info: \(sortedPeerInfo.count) Peers") + self.peerViewModel.peerInfo = sortedPeerInfo + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
NetBird/Source/App/ViewModels/MainViewModel.swift(1 hunks)NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (2)
NetbirdKit/NetworkExtensionAdapter.swift (4)
startTimer(360-362)startTimer(364-418)isLoginRequired(110-116)stop(149-151)NetbirdNetworkExtension/NetBirdAdapter.swift (1)
stop(126-128)
🔇 Additional comments (8)
NetbirdKit/NetworkExtensionAdapter.swift (5)
20-36: Well-structured battery optimization state management.The polling state variables are properly documented with thread-safety comments, and the dedicated
pollingQueueprovides the necessary serialization. TheisPollingActiveflag addresses the previous review concern about in-flight responses recreating timers after stop.
262-313: Response handling correctly serialized through pollingQueue.The refactored
fetchDataproperly routes all state mutations throughpollingQueue.async, addressing the previous threading concern. The adaptive polling logic (reset to 10s on changes, increase to 20s after 3 stable polls) is well-implemented.
315-331: Hash scope documentation addresses previous review.The added comments clearly explain the intentional field exclusions for battery optimization, noting that MainViewModel handles detailed UI comparisons separately.
333-358: Timer restart logic correctly implements theisPollingActivegate.The early bail on
!isPollingActiveprevents in-flight responses from recreating the timer afterstopTimer(). State values are captured onpollingQueuebefore dispatching to main thread, avoiding potential deadlocks.
432-447: Background mode handling is thread-safe and well-documented.State mutation is synchronized through
pollingQueue.sync, and the comment correctly notes that timer restart is deferred to the nextfetchDatacall viarestartTimerIfNeeded.NetBird/Source/App/ViewModels/MainViewModel.swift (3)
119-125: Battery optimization state variables are well-designed.Initializing
lastExtensionStateChecktoDate.distantPastensures the first check happens immediately. The 30-second throttle interval andhasStoppedForLoginFailureguard address the previous review concerns about repeated stop() calls.
126-148: Throttled polling and stop guard correctly implemented.The
Task { @MainActor in }pattern properly addresses the previous review about avoidingDispatchQueue.main.asyncinside a@MainActorclass. The unconditional reset ofhasStoppedForLoginFailurewhenextensionState == .disconnected(lines 140-141) decouples it fromextensionStateTextas suggested.
166-176: Stop guard prevents repeated calls during async state transitions.The multi-condition check with
hasStoppedForLoginFailureensuresstop()is called only once per login failure state, addressing the previous review concern about timing mismatches between polling and extension state updates.
- Wrap timer.invalidate() in DispatchQueue.main.async in startTimer() - Wrap timer.invalidate() in DispatchQueue.main.async in stopTimer() - Ensures timer operations happen on main thread where timer was scheduled - Makes code more defensive, independent of caller thread - Addresses thread-safety concerns from code review
- Fix inconsistent indentation in Task { @mainactor in } block
- Align sortedPeerInfo and if statement with surrounding code
- Improve code readability and consistency
…tion - Change timer.invalidate() from async to sync in startTimer() - Use Thread.isMainThread check to avoid deadlock if already on main thread - Ensures old timer is invalidated before fetchData() is called - Prevents old and new timers from running simultaneously - Prevents concurrent fetchData() calls and race conditions in state updates
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
NetbirdKit/NetworkExtensionAdapter.swift (1)
364-373: AvoidDispatchQueue.main.synchere (potential deadlock); enforce call-site preconditions.
IfstartTimer(...)is ever invoked frompollingQueuewhile main is blocked in apollingQueue.sync(e.g.,stopTimer()called on main), themain.syncat Line 370 can deadlock. Prefer a precondition or force this API onto the main actor/thread.private func startTimer(interval: TimeInterval?, backgroundState: Bool?, completion: @escaping (StatusDetails) -> Void) { + dispatchPrecondition(condition: .notOnQueue(pollingQueue)) // Invalidate timer synchronously on main thread to prevent old timer from running concurrently // This is safe because startTimer is either called from main thread or via restartTimerIfNeeded's main.async if Thread.isMainThread { self.timer.invalidate() } else { - DispatchQueue.main.sync { [weak self] in - self?.timer.invalidate() - } + DispatchQueue.main.async { [weak self] in + self?.timer.invalidate() + } }If you truly need “invalidate before continuing”, consider making this whole method
@MainActorand moving the non-UI work topollingQueue.
🧹 Nitpick comments (3)
NetbirdKit/NetworkExtensionAdapter.swift (2)
20-35: Makedecoderprivate and consider narrowing mutable surface (session,vpnManager).
decoder(Line 20) and the adapter’s core state are implementation details; reducing visibility helps prevent accidental cross-thread access.- let decoder = PropertyListDecoder() + private let decoder = PropertyListDecoder()
252-306:fetchDatacompletion runs onpollingQueue—document or normalize callback queue.
Right nowcompletion(...)is invoked frompollingQueue(Line 266+). That’s fine if all callers hop to@MainActor(asMainViewModeldoes), but it’s an easy footgun for other call sites.- completion(decodedStatus) + DispatchQueue.main.async { + completion(decodedStatus) + }(Or add a short doc comment on
fetchData/startTimerstating which queuecompletionis invoked on.)NetBird/Source/App/ViewModels/MainViewModel.swift (1)
150-175: ComputeisLoginRequired()once (and ideally off-main) to avoid UI hitching.
Inside the@MainActorblock you callnetworkExtensionAdapter.isLoginRequired()in the log (Line 160) and again in the stop condition (Line 170). If this touches disk / initializes SDK objects, it can stutter the UI.- print("Status: \(details.managementStatus) - Extension: \(self.extensionState) - LoginRequired: \(self.networkExtensionAdapter.isLoginRequired())") + let loginRequired = self.networkExtensionAdapter.isLoginRequired() + print("Status: \(details.managementStatus) - Extension: \(self.extensionState) - LoginRequired: \(loginRequired)") ... - self.networkExtensionAdapter.isLoginRequired() && + loginRequired &&Optionally: only evaluate
loginRequiredwhendetails.managementStatus == .disconnected && self.extensionState == .connected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
NetBird/Source/App/ViewModels/MainViewModel.swift(1 hunks)NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (2)
NetbirdKit/NetworkExtensionAdapter.swift (4)
startTimer(360-362)startTimer(364-426)isLoginRequired(110-116)stop(149-151)NetbirdNetworkExtension/NetBirdAdapter.swift (1)
stop(126-128)
🔇 Additional comments (4)
NetbirdKit/NetworkExtensionAdapter.swift (2)
333-358: Timer restart logic looks solid and correctly gated byisPollingActive.
CapturingintervalToUse/backgroundStateToUseonpollingQueuebefore hopping to main avoids the sync deadlock that the earlier approach risked.
315-331: Hash scope documentation + background-mode mutation discipline are clear.
The comment abovecalculateStatusHashand the “all state onpollingQueue” approach insetBackgroundModemake the tradeoffs explicit and keep mutation ordering predictable.Also applies to: 444-458
NetBird/Source/App/ViewModels/MainViewModel.swift (2)
119-125: Throttling extension-state checks to 30s is a good battery tradeoff.
ThelastExtensionStateCheck/extensionStateCheckIntervalmechanism is straightforward and keeps status polling from cascading into frequent NE preference loads.
126-148: Main-actor hop + stop-guard reset logic looks correct.
Task { @MainActor in ... }ensures UI safety, and the unconditional reset on.disconnectedavoids coupling toextensionStateText.
| self.timer = Timer(timeInterval: intervalToUse, repeats: true) { [weak self] _ in | ||
| guard let self = self else { return } | ||
| // Use background queue for actual network work | ||
| self.pollingQueue.async { | ||
| self.fetchData(completion: completion) | ||
| } | ||
| } |
There was a problem hiding this comment.
stopTimer() is async-invalidating, but the timer tick doesn’t check isPollingActive → extra queued polls after stop.
Even after isPollingActive = false, the existing timer can still fire until the main-queue invalidation runs (Line 430+), and the tick always enqueues fetchData (Line 416-418). Add a cheap gate to avoid work-after-stop.
self.timer = Timer(timeInterval: intervalToUse, repeats: true) { [weak self] _ in
guard let self = self else { return }
// Use background queue for actual network work
self.pollingQueue.async {
+ guard self.isPollingActive else { return }
self.fetchData(completion: completion)
}
}And consider invalidating immediately when already on main:
func stopTimer() {
- DispatchQueue.main.async { [weak self] in
- self?.timer.invalidate()
- }
+ if Thread.isMainThread {
+ self.timer.invalidate()
+ } else {
+ DispatchQueue.main.async { [weak self] in self?.timer.invalidate() }
+ }
pollingQueue.sync { ... isPollingActive = false ... }
}Also applies to: 428-442
🤖 Prompt for AI Agents
In NetbirdKit/NetworkExtensionAdapter.swift around lines 413–419 (and similarly
428–442), the timer closure always enqueues fetchData even if stopTimer() has
set isPollingActive = false but the timer hasn’t been invalidated on the main
queue yet; add a cheap gate at the top of the timer block to return early if
self.isPollingActive is false before dispatching to pollingQueue, preserving the
[weak self] capture and guard; also update stopTimer() to immediately invalidate
the timer if already on the main queue (or dispatch synchronously to main to
invalidate) to avoid extra firings, and keep thread-safe access to
isPollingActive when read/modified.
- Call checkExtensionState() immediately after networkExtensionAdapter.start() - Ensures polling starts right away if extension is already connected - Works together with previous fix to eliminate need for multiple connect attempts - Provides immediate feedback and status updates after connect()
- Set extensionStateText to 'Connecting' immediately when connect() is called - Add pollExtensionStateUntilConnected() to check extension state every 1 second - Ensures UI updates immediately when extension becomes connected - Fixes issue where UI showed 'Disconnected' for ~20 seconds after connect - Stops polling automatically once extension is connected - Max 15 attempts (15 seconds) to prevent infinite polling
- Update extensionStateText immediately when extensionState changes - Ensures UI shows correct status without waiting for CustomLottieView - Provides immediate feedback when extension becomes connected - Works together with pollExtensionStateUntilConnected() for instant UI updates
- Change GlobalConstants struct to public - Make all static properties public - Fixes 'cannot find GlobalConstants in scope' error in MainViewModel - Allows NetBird target to access NetbirdKit constants
- Change pollingQueue QoS from .utility to .userInitiated - Prevents priority inversion when main thread (user-interactive) waits on pollingQueue - Main thread uses semaphore.wait() in startTimer, stopTimer, setBackgroundMode, setInactiveMode - .userInitiated is appropriate for user-initiated background work that main thread depends on - Reduces risk of main thread blocking on lower-priority queue operations - Improves responsiveness and prevents potential UI freezes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
212-220: Peer route changes may not trigger UI update if content changes but count stays the same.A previous review flagged that comparing
a.routes.count == b.routes.count(line 216) won't detect in-place route changes. IfRouteconforms toEquatable, consider:-a.ip == b.ip && a.connStatus == b.connStatus && a.relayed == b.relayed && a.direct == b.direct && a.connStatusUpdate == b.connStatusUpdate && a.routes.count == b.routes.count +a.ip == b.ip && a.connStatus == b.connStatus && a.relayed == b.relayed && a.direct == b.direct && a.connStatusUpdate == b.connStatusUpdate && a.routes == b.routesNetbirdKit/NetworkExtensionAdapter.swift (1)
453-459: Timer tick can still fire afterstopTimer()setsisPollingActive = false.The past review flagged that after
stopTimer()setsisPollingActive = false, the existing timer can still fire (since invalidation is async on main queue), and the tick unconditionally enqueuesfetchDatawithout checkingisPollingActive. This remains unaddressed.Add a guard inside the timer closure to avoid work after stop:
self.timer = Timer(timeInterval: intervalToUse, repeats: true) { [weak self] _ in guard let self = self else { return } // Use background queue for actual network work self.pollingQueue.async { + guard self.isPollingActive else { return } self.fetchData(completion: completion) } }
🧹 Nitpick comments (3)
NetBird/Source/App/NetBirdApp.swift (1)
33-53: Scene phase handling looks correct for battery optimization.The consolidated
scenePhaseapproach is cleaner than notification-based handling. The priority of background > inactive > foreground for polling intervals is properly maintained.One minor consideration:
startPollingDetails()is called unconditionally when becoming.active, which triggers a new timer even if polling is already active. TheNetworkExtensionAdapter.startTimer()handles this by invalidating the existing timer first, but this still causes an unnecessaryfetchDatacall on each.activetransition. If app-switcher dismissals are frequent, consider gating on polling state:case .active: print("App became active") viewModel.networkExtensionAdapter.setBackgroundMode(false) viewModel.networkExtensionAdapter.setInactiveMode(false) viewModel.checkExtensionState() - viewModel.startPollingDetails() + if viewModel.extensionState == .connected { + viewModel.startPollingDetails() + }NetbirdKit/NetworkExtensionAdapter.swift (1)
468-490: Semaphore pattern is fragile ifstopTimer()is ever called from an async context.The
semaphore.wait()call blocks the calling thread. The comment claims this is safe becausestopTimeris called from main thread, but there's no enforcement (unlikestartTimerwhich hasdispatchPrecondition). IfstopTimer()is ever invoked from a Swift Concurrency context (e.g., inside aTask), it will block the cooperative thread pool and potentially deadlock.Consider adding the same precondition for consistency, or document the contract more prominently:
func stopTimer() { + // IMPORTANT: Must not be called from Swift Concurrency context (Task, async function) + // as semaphore.wait() would block the cooperative thread pool + dispatchPrecondition(condition: .notOnQueue(pollingQueue)) + // Invalidate timer on main thread where it was scheduled DispatchQueue.main.async { [weak self] inNetBird/Source/App/ViewModels/MainViewModel.swift (1)
114-132: Consider canceling existing fast-poll sequence ifconnect()is called again.If the user rapidly taps connect multiple times, multiple
pollExtensionStateUntilConnectedchains could run concurrently, each checking and potentially callingcheckExtensionState()simultaneously.Consider tracking the polling task and canceling it on new connect attempts:
+private var connectionPollingTask: Task<Void, Never>? + private func pollExtensionStateUntilConnected(attempt: Int, maxAttempts: Int) { + connectionPollingTask?.cancel() + connectionPollingTask = Task { @MainActor in guard attempt < maxAttempts else { print("Max attempts reached for extension state polling") return } checkExtensionState() - DispatchQueue.main.asyncAfter(deadline: .now() + 1.0) { [weak self] in - guard let self = self else { return } + try? await Task.sleep(nanoseconds: 1_000_000_000) + guard !Task.isCancelled else { return } + if self.extensionState != .connected { self.pollExtensionStateUntilConnected(attempt: attempt + 1, maxAttempts: maxAttempts) } - } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
NetBird/Source/App/NetBirdApp.swift(1 hunks)NetBird/Source/App/ViewModels/MainViewModel.swift(3 hunks)NetbirdKit/GlobalConstants.swift(1 hunks)NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/NetBirdApp.swift (2)
NetbirdKit/NetworkExtensionAdapter.swift (2)
setBackgroundMode(492-516)setInactiveMode(518-550)NetBird/Source/App/ViewModels/MainViewModel.swift (3)
stopPollingDetails(225-227)checkExtensionState(229-257)startPollingDetails(153-223)
🔇 Additional comments (9)
NetbirdKit/GlobalConstants.swift (1)
8-10: LGTM!Making
GlobalConstantsand its memberspublicenables proper cross-module access for shared configuration keys. The change is appropriate for constants used across the app and extension modules.NetbirdKit/NetworkExtensionAdapter.swift (4)
22-38: Well-structured state management for adaptive polling.The polling state variables are properly isolated to
pollingQueueaccess, and the interval hierarchy (background > inactive > foreground) is clearly defined. TheisPollingActiveflag addresses the previously identified race condition concern.
264-316: Response handling properly serialized throughpollingQueue.This addresses the previously identified race condition where state mutations (
currentPollingInterval,consecutiveStablePolls,lastStatusHash) could occur on arbitrary completion handler threads. All mutations now happen onpollingQueue, and completions are dispatched to main queue for UI safety.
326-342: Hash calculation documentation is comprehensive.The comment clearly explains which fields are included/excluded and the rationale (battery optimization vs. UI updates). This addresses the previous review feedback about documenting the hash scope.
344-377:restartTimerIfNeededcorrectly guards against stopped polling state.The
isPollingActivecheck at line 347 prevents in-flight responses from recreating the timer afterstopTimer()was called. State is captured before themain.asyncdispatch to avoid deadlock withpollingQueue.sync.NetBird/Source/App/ViewModels/MainViewModel.swift (4)
100-112: Good UX improvement with immediate "Connecting" feedback.Setting
extensionStateTextto "Connecting" immediately and using fast polling provides better user feedback during connection establishment.
146-151: Battery optimization state tracking is well-designed.The 30-second throttle for extension state checks (
extensionStateCheckInterval) combined withhasStoppedForLoginFailureguard prevents redundant operations and addresses the previously identified timing issues.
153-175: Polling callback correctly usesTask { @MainActor }pattern.This addresses the previous review feedback about using structured concurrency instead of
DispatchQueue.main.asyncinside a@MainActorViewModel. The unconditional reset ofhasStoppedForLoginFailureon.disconnectedstate also addresses the prior coupling concern.
229-257: Extension state handling provides good immediate feedback.The immediate
extensionStateTextupdates and conditional polling start on connection ensure responsive UI. ThewasConnectedcheck prevents redundant polling restarts.
- Change from a.routes.count == b.routes.count to Set(a.routes) == Set(b.routes) - Consistent with PeerInfo: Equatable which uses Set(lhs.routes) == Set(rhs.routes) - Properly detects route changes regardless of order - Count-only comparison could miss changes (e.g., ['A','B'] vs ['C','D'] both count=2) - Set comparison ignores order and checks actual content, matching Equatable behavior
- Add isPollingActive guard inside timer closure before fetchData() - Prevents timer tick from executing after stopTimer() sets isPollingActive = false - Timer invalidation is async on main queue, so timer can still fire before invalidation completes - Guard ensures no unnecessary work is done after polling is stopped - Fixes race condition where timer could fire between stopTimer() and actual invalidation
- Gate startPollingDetails() on extensionState == .connected - Prevents unnecessary fetchData() calls when app becomes active but extension is not connected - startTimer() invalidates existing timer and immediately calls fetchData(), which is wasteful if not connected - App-switcher dismissals are frequent, so this optimization reduces battery usage - checkExtensionState() is still called to update extension state - Polling will start automatically when extension becomes connected (via checkExtensionState callback)
- Add dispatchPrecondition(condition: .notOnQueue(pollingQueue)) to stopTimer() - Add prominent comment documenting that stopTimer() must not be called from Swift Concurrency context - semaphore.wait() blocks the calling thread, which would block cooperative thread pool if called from Task/async - Ensures consistency with startTimer() which already has the same precondition - Prevents potential deadlocks when stopTimer() is called from async contexts
- Add connectionPollingTask property to track polling task - Cancel existing task before starting new one to prevent concurrent polling chains - Replace DispatchQueue.main.asyncAfter with Task.sleep for modern async/await pattern - Add Task.isCancelled check to handle cancellation gracefully - Prevents multiple pollExtensionStateUntilConnected chains running simultaneously - Reduces redundant checkExtensionState() calls when user rapidly taps connect - Improves resource usage and prevents race conditions
- Replace DispatchQueue.main.asyncAfter with Task.sleep for modern async/await pattern - Add Task.isCancelled check to handle cancellation gracefully - Enables proper cancellation of polling chain when connect() is called again - Prevents multiple concurrent polling chains from running simultaneously - Improves resource usage and prevents race conditions
- Replace recursive calls with while loop to prevent resource leaks - Recursive calls were overwriting connectionPollingTask, leaving earlier Task instances running untracked - Now a single Task runs from start to finish, properly tracked and cancellable - Add multiple Task.isCancelled checks for proper cancellation handling - Early exit when extension becomes connected - Prevents multiple concurrent polling tasks and resource leaks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
NetbirdKit/NetworkExtensionAdapter.swift (3)
385-471: Consider refactoring to avoid blocking the main thread with semaphores.Lines 427-449 use
semaphore.wait()on the main thread to read state frompollingQueue. While the comments indicate this is safe (not called from Swift Concurrency contexts) and the blocking time should be minimal, this can still cause brief UI hitching ifpollingQueueis busy.Consider refactoring to use an async pattern or pre-captured state to eliminate main-thread blocking. For example, you could store the computed interval in an atomic property or use
async/awaitwith@MainActorannotations.That said, given the current architecture constraints and the minimal blocking time, this is a pragmatic solution that works correctly.
473-500: Consider async alternative to semaphore blocking in stopTimer().Similar to
startTimer,stopTimerblocks the main thread withsemaphore.wait()(Line 499). While the precondition enforcement prevents deadlocks and the blocking time is minimal, an async/await refactor would eliminate the main-thread blocking entirely.For example:
func stopTimer() async { await MainActor.run { self.timer.invalidate() } await pollingQueue.asyncTask { self.consecutiveStablePolls = 0 self.currentPollingInterval = self.minPollingInterval self.isPollingActive = false } }However, this would require callers to be in async contexts, which might be a larger refactor.
502-560: Semaphore blocking in state setters could be replaced with async patterns.Both
setBackgroundModeandsetInactiveModeblock the main thread with semaphores (Lines 525, 559). While this ensures state is synchronized before returning, it introduces brief blocking.Since these are called from SwiftUI lifecycle observers (scene phase changes), consider whether the immediate return value is necessary, or if an async update pattern would work equally well without blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
NetBird/Source/App/NetBirdApp.swift(1 hunks)NetBird/Source/App/ViewModels/MainViewModel.swift(3 hunks)NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- NetBird/Source/App/NetBirdApp.swift
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
NetbirdKit/NetworkExtensionAdapter.swift (5)
start(62-70)startTimer(381-383)startTimer(385-471)isLoginRequired(114-120)stop(153-155)
🔇 Additional comments (8)
NetBird/Source/App/ViewModels/MainViewModel.swift (4)
100-101: LGTM: Immediate UI feedback improves UX.Setting the UI state to "Connecting" immediately provides better user feedback while the extension starts.
163-171: LGTM: Well-structured battery optimization state.The battery optimization variables are clearly named and documented. The 30-second throttling interval, stop-guard flag, and task lifecycle management provide a solid foundation for the optimizations.
174-242: LGTM: Comprehensive polling refactor with proper concurrency handling.The refactored polling logic properly:
- Uses
[weak self]andTask { @MainActor in ... }for safe async updates- Throttles extension state checks to every 30 seconds
- Guards against repeated
stop()calls withhasStoppedForLoginFailure- Unconditionally resets the stop guard on disconnect (addressing past review feedback)
- Performs detailed peerInfo comparison including route Set equality
253-273: LGTM: Immediate state transitions improve responsiveness.The immediate UI text updates and automatic polling start on connection provide better user experience without waiting for periodic checks.
NetbirdKit/NetworkExtensionAdapter.swift (4)
20-40: LGTM: Well-organized adaptive polling infrastructure.The battery optimization state variables are clearly documented, and the use of
userInitiatedQoS forpollingQueueproperly avoids priority inversion when the main thread synchronizes with this queue.
266-319: LGTM: Response handling properly serialized through pollingQueue.The refactored
fetchDatacorrectly:
- Serializes all state mutations through
pollingQueue(addressing past concurrency concerns)- Handles nil responses safely
- Calculates status hash to detect changes
- Adjusts polling intervals dynamically
- Dispatches completions to the main queue for thread safety
328-344: LGTM: Hash calculation scope is now well-documented.The documentation clearly explains why certain fields (relayed, direct, connStatusUpdate, routes) are intentionally omitted from the hash calculation for battery optimization, while MainViewModel performs more detailed comparisons for UI updates.
346-379: LGTM: Timer restart logic prevents races and avoids deadlocks.The function properly:
- Guards with
isPollingActiveto prevent in-flight responses from recreating the timer after stop- Captures state on
pollingQueuebefore dispatching to main thread (avoiding deadlock)- Only restarts when the interval changes significantly
| private func pollExtensionStateUntilConnected(attempt: Int, maxAttempts: Int) { | ||
| // Cancel existing polling task if connect() was called again | ||
| connectionPollingTask?.cancel() | ||
|
|
||
| connectionPollingTask = Task { @MainActor in | ||
| var currentAttempt = attempt | ||
| while currentAttempt < maxAttempts { | ||
| guard !Task.isCancelled else { | ||
| print("Connection polling task was cancelled") | ||
| return | ||
| } | ||
|
|
||
| checkExtensionState() | ||
|
|
||
| // If connected, stop polling (checkExtensionState will start polling if needed) | ||
| if self.extensionState == .connected { | ||
| print("Extension connected, stopping state polling") | ||
| return | ||
| } | ||
|
|
||
| // Wait 1 second before next check | ||
| try? await Task.sleep(nanoseconds: 1_000_000_000) | ||
|
|
||
| guard !Task.isCancelled else { | ||
| print("Connection polling task was cancelled during sleep") | ||
| return | ||
| } | ||
|
|
||
| currentAttempt += 1 | ||
| } | ||
|
|
||
| print("Max attempts reached for extension state polling") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check getExtensionStatus implementation for concurrency handling
rg -n "func getExtensionStatus" -A 15 | head -50Repository: netbirdio/ios-client
Length of output: 1149
🏁 Script executed:
#!/bin/bash
# Find the NetworkExtensionAdapter file
fd "NetworkExtensionAdapter" --type fRepository: netbirdio/ios-client
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Search for any concurrency-related patterns in NetworkExtensionAdapter
rg -n "DispatchQueue|NSLock|os_unfair_lock|nonisolated|Sendable" --type swift | head -30Repository: netbirdio/ios-client
Length of output: 3335
🏁 Script executed:
#!/bin/bash
# Find checkExtensionState implementation
rg -n "func checkExtensionState" -A 20 --type swiftRepository: netbirdio/ios-client
Length of output: 2475
🏁 Script executed:
#!/bin/bash
# Check if there's any serialization or locking around loadAllFromPreferences calls
rg -n "loadAllFromPreferences" --type swift -B 3 -A 3Repository: netbirdio/ios-client
Length of output: 1365
🏁 Script executed:
#!/bin/bash
# Search for any comments about thread safety or synchronization around loadAllFromPreferences
rg -n "thread|concurrent|serial|sync|lock" NetbirdKit/NetworkExtensionAdapter.swift --type swiftRepository: netbirdio/ios-client
Length of output: 4734
🏁 Script executed:
#!/bin/bash
# Check the full context around getExtensionStatus to see if there's any protection
sed -n '550,580p' NetbirdKit/NetworkExtensionAdapter.swiftRepository: netbirdio/ios-client
Length of output: 1156
Add synchronization to prevent concurrent loadAllFromPreferences() calls in getExtensionStatus().
The polling loop calls checkExtensionState() every second for up to 15 seconds, potentially spawning 15 concurrent Task instances that each call getExtensionStatus(). Each invocation triggers NETunnelProviderManager.loadAllFromPreferences() without synchronization, allowing overlapping concurrent calls. While Apple's NetworkExtension framework may handle concurrent preference reads, the pattern is inconsistent with the rest of the codebase, which uses the existing pollingQueue to serialize similar operations. Consider serializing these calls using the existing pollingQueue or another appropriate synchronization mechanism.
…llFromPreferences() - Serialize getExtensionStatus() calls through pollingQueue to prevent concurrent loadAllFromPreferences() invocations - pollExtensionStateUntilConnected() calls checkExtensionState() every second for up to 15 seconds, potentially spawning 15 concurrent Task instances - Each Task calls getExtensionStatus() which triggers loadAllFromPreferences() without synchronization - Now all getExtensionStatus() calls are serialized through pollingQueue, ensuring only one loadAllFromPreferences() call at a time - Dispatch completion to main thread for thread safety - Consistent with rest of codebase which uses pollingQueue for serialization
- Set extensionStateText to 'Disconnecting' immediately when disconnect is pressed - Call checkExtensionState() immediately after stop() to update UI without waiting for 30s periodic check - Provides immediate UI feedback similar to connect() behavior - Improves user experience by showing disconnect state immediately instead of delayed update
- Add fallback to .disconnected status when no manager is found - Add error handling to return .disconnected on loadAllFromPreferences() failure - Prevents UI hang when getExtensionStatus() is called during app initialization - Ensures completion handler is always called, even on error - Improves app startup reliability
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
NetbirdKit/NetworkExtensionAdapter.swift (1)
473-500: Same semaphore risk applies tostopTimer().The semaphore pattern here has the same fragility as in
startTimer. The comment at lines 474-476 is helpful, but adispatchPrecondition(condition: .onQueue(.main))would provide stronger enforcement that this is called from the main thread specifically (not just "not pollingQueue").func stopTimer() { // IMPORTANT: Must not be called from Swift Concurrency context (Task, async function) // as semaphore.wait() would block the cooperative thread pool and potentially deadlock. // This function must be called from main thread or synchronous context only. dispatchPrecondition(condition: .notOnQueue(pollingQueue)) + // Prefer calling from main thread to ensure semaphore.wait() doesn't block cooperative threads + assert(Thread.isMainThread, "stopTimer() should be called from main thread")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
NetBird/Source/App/ViewModels/MainViewModel.swift(3 hunks)NetbirdKit/NetworkExtensionAdapter.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
NetbirdKit/NetworkExtensionAdapter.swift (5)
start(62-70)stop(153-155)startTimer(381-383)startTimer(385-471)isLoginRequired(114-120)
🔇 Additional comments (11)
NetbirdKit/NetworkExtensionAdapter.swift (6)
20-39: Well-structured state management for adaptive polling.The polling state variables are properly documented and centralized. The use of
.userInitiatedQoS forpollingQueueis appropriate given the comment about avoiding priority inversion when main thread waits on this queue via semaphores.
266-318: Good serialization of response handling through pollingQueue.All state mutations (
consecutiveStablePolls,currentPollingInterval,lastStatusHash) now occur onpollingQueue, addressing the previous race condition concern. Completions are correctly dispatched to main queue for thread safety.
328-344: Hash documentation clearly explains the design rationale.The inline comment effectively communicates why certain peer fields are excluded from the hash and how this relates to battery optimization vs. UI update responsibilities.
346-379: Proper guard against timer recreation after stop.The
isPollingActiveguard at line 349 correctly prevents in-flight responses from recreating the timer afterstopTimer()has been called. State capture before dispatching to main thread avoids potential deadlocks.
455-464: Timer callback correctly guards against work-after-stop.The
guard self.isPollingActive else { return }check at line 461 prevents unnecessaryfetchDatacalls afterstopTimer()sets the flag tofalse, even if timer invalidation is still pending.
502-560: Background/inactive mode setters follow consistent patterns.Both methods correctly use
pollingQueuefor state mutations and defer timer restart to the nextfetchDatacall. The priority logic (background > inactive > foreground) is correctly implemented in both.NetBird/Source/App/ViewModels/MainViewModel.swift (5)
100-112: Good immediate UI feedback on connect.Setting
extensionStateText = "Connecting"before starting the async work provides immediate visual feedback. The polling initiation withpollExtensionStateUntilConnectedensures the UI updates promptly when connected.
159-167: Immediate UI feedback on disconnect is appropriate.Setting
extensionStateText = "Disconnecting"beforestop()provides good UX. ThecheckExtensionState()call afterward will eventually update the state, though it may take a moment for the VPN to actually disconnect.
170-179: State tracking variables properly address previous concerns.
hasStoppedForLoginFailureprevents repeatedstop()calls (addressing a past review comment), andconnectionPollingTaskenables proper cancellation of polling whenconnect()is called again.
180-250: Polling details refactored with proper threading and state guards.Key improvements:
Task { @MainActor in ... }for UI updates (addresses past review)- Unconditional
hasStoppedForLoginFailurereset on.disconnected(addresses past review)Set(a.routes) == Set(b.routes)for proper route comparison (addresses past review)isLoginRequired()computed once per iteration to avoid UI hitching
256-284: Consider handling.disconnectingstate in UI text update.The
extensionStateTextupdate handles.connected,.connecting, and.disconnected, but not.disconnecting. When the user taps disconnect,close()sets "Disconnecting" immediately, but ifcheckExtensionStateis called while status is.disconnecting, it won't update the text (which may already be correct fromclose()). This is likely fine, but worth noting for completeness.
| // Poll extension state repeatedly until connected or max attempts reached | ||
| // This provides immediate UI feedback after connect() instead of waiting for periodic check | ||
| private func pollExtensionStateUntilConnected(attempt: Int, maxAttempts: Int) { | ||
| // Cancel existing polling task if connect() was called again | ||
| connectionPollingTask?.cancel() | ||
|
|
||
| connectionPollingTask = Task { @MainActor in | ||
| var currentAttempt = attempt | ||
| while currentAttempt < maxAttempts { | ||
| guard !Task.isCancelled else { | ||
| print("Connection polling task was cancelled") | ||
| return | ||
| } | ||
|
|
||
| checkExtensionState() | ||
|
|
||
| // If connected, stop polling (checkExtensionState will start polling if needed) | ||
| if self.extensionState == .connected { | ||
| print("Extension connected, stopping state polling") | ||
| return | ||
| } | ||
|
|
||
| // Wait 1 second before next check | ||
| try? await Task.sleep(nanoseconds: 1_000_000_000) | ||
|
|
||
| guard !Task.isCancelled else { | ||
| print("Connection polling task was cancelled during sleep") | ||
| return | ||
| } | ||
|
|
||
| currentAttempt += 1 | ||
| } | ||
|
|
||
| print("Max attempts reached for extension state polling") | ||
| } | ||
| } |
There was a problem hiding this comment.
Polling loop may check stale state due to async checkExtensionState.
checkExtensionState() at line 128 is fire-and-forget—it calls getExtensionStatus with a completion handler that updates extensionState asynchronously. The subsequent check at line 131 (if self.extensionState == .connected) may evaluate before the completion handler has run, potentially causing extra polling iterations.
Consider awaiting the state update or using a completion-based check:
- checkExtensionState()
-
- // If connected, stop polling (checkExtensionState will start polling if needed)
- if self.extensionState == .connected {
+ await withCheckedContinuation { continuation in
+ networkExtensionAdapter.getExtensionStatus { [weak self] status in
+ DispatchQueue.main.async {
+ if let self = self, status == .connected && self.extensionState != .connected {
+ self.extensionState = status
+ self.extensionStateText = "Connected"
+ self.startPollingDetails()
+ }
+ continuation.resume()
+ }
+ }
+ }
+
+ if self.extensionState == .connected {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In NetBird/Source/App/ViewModels/MainViewModel.swift around lines 114–149, the
polling loop calls checkExtensionState() which updates extensionState
asynchronously via a completion handler, so the immediate if self.extensionState
== .connected check can read a stale value; change checkExtensionState to
provide an async result (or add a completion closure) and await that result
inside the Task before evaluating extensionState, e.g., convert
getExtensionStatus call path to an async function or use withCheckedContinuation
to bridge the callback, then use the returned state to decide whether to stop
polling; keep existing cancellation checks and sleep logic.
| let semaphore = DispatchSemaphore(value: 0) | ||
| var intervalValue: TimeInterval = minPollingInterval | ||
| var backgroundValue: Bool = false | ||
| pollingQueue.async { | ||
| backgroundValue = isInBackground | ||
| let inactiveValue = isInactive | ||
| // Priority: background > inactive > current (foreground) | ||
| if backgroundValue { | ||
| intervalValue = backgroundPollingInterval | ||
| } else if inactiveValue { | ||
| intervalValue = inactivePollingInterval | ||
| } else { | ||
| intervalValue = currentPollingInterval | ||
| } | ||
| lastTimerInterval = intervalValue | ||
| isPollingActive = true | ||
| semaphore.signal() | ||
| } | ||
| // Wait for async operation to complete (safe here as we're not in Swift Concurrency context) | ||
| semaphore.wait() | ||
| intervalToUse = intervalValue | ||
| backgroundStateToUse = backgroundValue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "startTimer\(" --type swift -B 5 -A 2Repository: netbirdio/ios-client
Length of output: 5942
🏁 Script executed:
# Check for Task, async/await, or Swift Concurrency patterns in the adapter file
rg -n "(Task|async let|await|actor|nonisolated)" NetbirdKit/NetworkExtensionAdapter.swift | head -50Repository: netbirdio/ios-client
Length of output: 1092
🏁 Script executed:
# Get lines around 474 and 388 to see the full precondition and comments
sed -n '385,395p' NetbirdKit/NetworkExtensionAdapter.swift
echo "---"
sed -n '470,480p' NetbirdKit/NetworkExtensionAdapter.swiftRepository: netbirdio/ios-client
Length of output: 1382
🏁 Script executed:
# Show the exact snippet from lines 427-449 with context
sed -n '420,450p' NetbirdKit/NetworkExtensionAdapter.swiftRepository: netbirdio/ios-client
Length of output: 1510
Improve comment clarity for semaphore usage to match the explicit Swift Concurrency constraint documented elsewhere.
The comment at line 446 stating "safe here as we're not in Swift Concurrency context" is vague. Compare this to stopTimer()'s explicit warning (line 474): "Must not be called from Swift Concurrency context (Task, async function) as semaphore.wait() would block the cooperative thread pool and potentially deadlock."
Update the comment for the semaphore.wait() at line 446 to match this clarity:
- Explicitly state the constraint: "Must not be called from Swift Concurrency context"
- Explain the risk: "semaphore.wait() blocks the cooperative thread pool"
- Note the safety mechanism: "dispatchPrecondition enforces this constraint"
The dispatchPrecondition(condition: .notOnQueue(pollingQueue)) at line 388 already prevents unsafe calls, so no code changes are needed—only clarify the documentation to match the safety comments in setBackgroundMode() and setInactiveMode().
🤖 Prompt for AI Agents
In NetbirdKit/NetworkExtensionAdapter.swift around lines 427 to 449, the inline
comment "safe here as we're not in Swift Concurrency context" is vague; update
it to explicitly state "Must not be called from Swift Concurrency context (Task,
async function) as semaphore.wait() blocks the cooperative thread pool and can
deadlock", mention that this risk is why dispatchPrecondition(condition:
.notOnQueue(pollingQueue)) enforces the constraint, and mirror the clearer
wording used in stopTimer() so the semaphore.wait() usage and its safety
mechanism are clearly documented.
| func getExtensionStatus(completion: @escaping (NEVPNStatus) -> Void) { | ||
| Task { | ||
| do { | ||
| let managers = try await NETunnelProviderManager.loadAllFromPreferences() | ||
| if let manager = managers.first(where: { $0.localizedDescription == self.extensionName }) { | ||
| completion(manager.connection.status) | ||
| // Serialize loadAllFromPreferences() calls to prevent concurrent access | ||
| // This is especially important when pollExtensionStateUntilConnected() calls | ||
| // checkExtensionState() multiple times per second, which would otherwise | ||
| // spawn multiple concurrent Task instances calling loadAllFromPreferences() | ||
| // Note: Task is created outside pollingQueue to avoid blocking the queue with async work | ||
| pollingQueue.async { [weak self] in | ||
| guard let self = self else { return } | ||
|
|
||
| // Create Task outside pollingQueue to avoid blocking the serial queue | ||
| // The async work (loadAllFromPreferences) will run concurrently, but | ||
| // we ensure only one call to getExtensionStatus is processed at a time | ||
| Task { | ||
| do { | ||
| let managers = try await NETunnelProviderManager.loadAllFromPreferences() | ||
| if let manager = managers.first(where: { $0.localizedDescription == self.extensionName }) { | ||
| DispatchQueue.main.async { | ||
| completion(manager.connection.status) | ||
| } | ||
| } else { | ||
| // No manager found, return disconnected status | ||
| DispatchQueue.main.async { | ||
| completion(.disconnected) | ||
| } | ||
| } | ||
| } catch { | ||
| print("Error loading from preferences: \(error)") | ||
| // Return disconnected status on error to prevent UI hang | ||
| DispatchQueue.main.async { | ||
| completion(.disconnected) | ||
| } | ||
| } | ||
| } catch { | ||
| print("Error loading from preferences: \(error)") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Task creation inside pollingQueue doesn't serialize async operations.
The comment at lines 571-573 acknowledges the issue: "The async work (loadAllFromPreferences) will run concurrently." Creating a Task inside pollingQueue.async immediately returns control to the queue, allowing subsequent getExtensionStatus calls to spawn additional concurrent Task instances.
For true serialization of loadAllFromPreferences() calls during aggressive polling (15 calls/second in pollExtensionStateUntilConnected), consider using an AsyncStream or actor-based approach. However, given that Apple's NetworkExtension framework likely handles concurrent reads safely, this may be acceptable for now.
- Delay setBackgroundMode, setInactiveMode, and checkExtensionState by 0.1s in .active case - These operations use semaphores that could block app launch if pollingQueue is busy - Prevents app from hanging on launch screen after 'App became active' log - App can fully load before state updates are performed - Improves app startup reliability
- Separate checkExtensionState() call with longer delay (0.5s) from state updates (0.1s) - Prevents app from hanging if extension is not configured or not running - App can now start even if VPN is not manually started - setBackgroundMode/setInactiveMode still execute early (0.1s) for proper state - checkExtensionState() executes later (0.5s) to avoid blocking app launch - Improves app startup reliability when extension is not available
…blocking - Only call checkExtensionState() when extensionState is .disconnected (first launch scenario) - On first launch, extensionState defaults to .disconnected, so check is delayed (0.5s) - When extension state is already known (not first launch), check immediately - Prevents app from blocking on first launch when extension doesn't exist yet - Improves app startup reliability for new installations
- Remove conditional logic that was always true (extensionState defaults to .disconnected) - Always delay checkExtensionState() by 0.5s in .active case - getExtensionStatus() already has fallback to .disconnected if extension doesn't exist - This ensures app can start on first launch when extension doesn't exist yet - Simpler, clearer logic that works for all scenarios
- Remove artificial delays (0.1s, 0.5s) that were workarounds for blocking operations - Remove checkExtensionState() call from .active case - it will be called automatically when needed: * When user taps connect (pollExtensionStateUntilConnected) * When extension becomes connected (checkExtensionState in startPollingDetails) * Periodically during polling (every 30s) - Call setBackgroundMode/setInactiveMode asynchronously on main queue (non-blocking) - App can now start immediately without any blocking operations - Prevents blocking on first launch when extension doesn't exist yet - Cleaner code without workarounds
…launch - Remove setBackgroundMode(false) and setInactiveMode(false) calls in .active case - Initial state is already false (foreground, active), so these calls are redundant - These functions use semaphore.wait() which could block even when called asynchronously - State will be properly updated when app actually transitions from background/inactive - Prevents any potential blocking during app launch - Cleaner code without unnecessary state updates
|
Thanks for clarifying @uid0c0der. I had issues compiling it with some missing closing curly braces, and after fixing (probably breaking) it, it wasn't stable. In any case, yesterday we started improving the reconnecting logic too, here #39. We like your take, especially on memory and status. We are still validating it too. |
@mlsmaycon my test was without #39, i can confirm that this has some impact on this PR. Tried to fix this yesterday but did not work very well. My successfull test was without #39 and Battery consumption went down from 23% on my iphone to 8% (~24hours) |
2 participants
Reduces battery consumption by ~30-50% through ~90% reduction in polling requests.
Summary by CodeRabbit
Performance & Optimization
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.