Add force relay connection option #37
Conversation
MainViewModel code is stil WIP
…AdvancedView to a common function
…wModel in Alerts Their parent view (AdvancedView) also accesses it via @EnvironmentObject property
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a user-facing "Force relay connection" toggle persisted in a shared UserDefaults suite, introduces GlobalConstants and EnvVarPackager to expose that setting as SDK environment variables, and passes the packaged env list into the Network Extension's NetBirdSDKClient.run at startup. Changes
Sequence DiagramsequenceDiagram
actor User
participant AppUI as AdvancedView
participant VM as MainViewModel
participant SharedDefaults as UserDefaults (suite)
participant EnvPackager as EnvVarPackager
participant NetExt as NetBirdAdapter
participant SDK as NetBirdSDKClient
User->>AppUI: Toggle "Force relay"
AppUI->>VM: setForcedRelayConnection(isEnabled)
VM->>SharedDefaults: write key (GlobalConstants.keyForceRelayConnection)
VM-->>AppUI: publish state (forceRelayConnection, showForceRelayAlert)
Note over NetExt,SDK: Network Extension startup
NetExt->>SharedDefaults: open suite (GlobalConstants.userPreferencesSuiteName)
NetExt->>EnvPackager: getEnvironmentVariables(defaults)
EnvPackager->>EnvPackager: create/populate NetBirdSDKEnvList (NB_FORCE_RELAY)
EnvPackager-->>NetExt: return envList
NetExt->>SDK: run(tunnelFileDescriptor, interfaceName, envList)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
NetbirdNetworkExtension/NetBirdAdapter.swift (1)
106-109: Consider simplifying the optional handling.The force unwrap on line 107 is safe due to the nil check, but using
flatMaporif-letwould be more idiomatic Swift.- let userDefaults = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) - let envList = userDefaults != nil ? EnvVarPackager.getEnvironmentVariables(defaults: userDefaults!) : nil + let envList = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) + .flatMap { EnvVarPackager.getEnvironmentVariables(defaults: $0) }NetbirdKit/EnvVarPackager.swift (1)
14-20: Consider simplifying withregister(defaults:)or nil-coalescing.The explicit nil check can be replaced with a more idiomatic pattern. Note:
bool(forKey:)returnsfalsewhen the key doesn't exist, which is why you need the explicit nil check for atruedefault.- var forceRelayConnection : Bool - - if defaults.object(forKey: GlobalConstants.keyForceRelayConnection) == nil { - forceRelayConnection = true - } else { - forceRelayConnection = defaults.bool(forKey: GlobalConstants.keyForceRelayConnection) - } + let forceRelayConnection = defaults.object(forKey: GlobalConstants.keyForceRelayConnection) == nil + ? true + : defaults.bool(forKey: GlobalConstants.keyForceRelayConnection)NetBird/Source/App/ViewModels/MainViewModel.swift (1)
285-289: TheforceRelayConnectionproperty is not updated.The method persists the value to
UserDefaultsbut doesn't update the@PublishedpropertyforceRelayConnection. While the binding in the Toggle keeps the UI in sync, the property won't reflect the saved value if accessed elsewhere before the next initialization.This works because SwiftUI's
$viewModel.forceRelayConnectionbinding updates the property directly when the toggle changes. However, explicitly updating it here would make the code more self-documenting.func setForcedRelayConnection(enabled: Bool) { let userDefaults = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) userDefaults?.set(enabled, forKey: GlobalConstants.keyForceRelayConnection) + self.forceRelayConnection = enabled showForceRelayAlert = true }NetBird/Source/App/Views/AdvancedView.swift (1)
316-319: TheisPresentedbinding appears redundant.
LogLevelAlertnow uses@EnvironmentObjectfor theViewModel, but still receivesisPresentedas a@Binding. The confirm button togglesisPresented, but the overlay'sonDismissalready setsviewModel.showLogLevelChangedAlert = false. Consider removing the binding and usingviewModel.showLogLevelChangedAlertdirectly for consistency withForceRelayAlert.struct LogLevelAlert: View { @EnvironmentObject var viewModel: ViewModel - @Binding var isPresented: Bool var body: some View { VStack(spacing: 20) { // ... SolidButton(text: "Confirm") { - isPresented.toggle() + viewModel.showLogLevelChangedAlert = false } // ... } } }And update the call site:
-LogLevelAlert(isPresented: $viewModel.showLogLevelChangedAlert) +LogLevelAlert()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
NetBird.xcodeproj/project.pbxproj(6 hunks)NetBird/Source/App/ViewModels/MainViewModel.swift(3 hunks)NetBird/Source/App/Views/AdvancedView.swift(7 hunks)NetbirdKit/EnvVarPackager.swift(1 hunks)NetbirdKit/GlobalConstants.swift(1 hunks)NetbirdNetworkExtension/NetBirdAdapter.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
NetbirdNetworkExtension/NetBirdAdapter.swift (1)
NetbirdKit/EnvVarPackager.swift (1)
getEnvironmentVariables(9-25)
NetBird/Source/App/Views/AdvancedView.swift (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
setForcedRelayConnection(285-289)
🔇 Additional comments (8)
NetbirdKit/GlobalConstants.swift (1)
8-11: LGTM!The constants are well-organized and appropriately named. The suite name
group.io.netbird.appmatches the app group identifier used elsewhere in the codebase.NetBird.xcodeproj/project.pbxproj (1)
94-97: LGTM!The new source files are correctly added to both the main app and network extension targets, ensuring the shared constants and environment variable packager are available where needed.
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
291-294: LGTM!The getter correctly handles the nil case with a default of
true, consistent withEnvVarPackager.NetBird/Source/App/Views/AdvancedView.swift (3)
112-132: LGTM!The new "Network & Security" section is well-integrated with consistent styling matching other toggle sections. The toggle correctly persists the value via
setForcedRelayConnection.
167-187: Good abstraction for reusable alert overlays.The
alertOverlayhelper consolidates the overlay pattern nicely. ThebuttonLockmechanism prevents accidental rapid interactions after dismissal.
284-314: LGTM!The
ForceRelayAlertcomponent is clean and follows the same pattern asLogLevelAlert. The message clearly informs users about the need to reconnect.NetbirdKit/EnvVarPackager.swift (1)
22-22: No action needed—the SDK expects string boolean values in the format produced byString(forceRelayConnection), which outputs"true"or"false"as confirmed by NetBird community documentation showingNB_FORCE_RELAY=trueusage.NetbirdNetworkExtension/NetBirdAdapter.swift (1)
99-115: Verify SDK'srunmethod signature and nil handling forenvListparameter.The code passes
envList(which can benilwhenUserDefaultsinitialization fails orgetEnvironmentVariablesreturnsnil) directly toself.client.run(). Confirm the SDK's method signature accepts an optionalenvListparameter and handlesnilgracefully, or add an explicit nil check before callingrun.
If the key isn't set, it will use the value by default
…ForcedRelayConnection is called
There was a problem hiding this comment.
Pull request overview
This PR adds a new "Force relay connection" toggle in the Advanced settings to allow users to control whether VPN connections should enforce relay usage. The setting is persisted in shared UserDefaults and passed to the NetBird SDK as an environment variable when establishing connections.
- Added user-facing toggle control for force relay connection in Advanced settings
- Implemented persistent storage using App Group's UserDefaults for cross-process access
- Integrated environment variable packaging to pass the setting to the NetBird client on startup
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| NetbirdKit/GlobalConstants.swift | Defines shared constants for the force relay connection key and App Group suite name |
| NetbirdKit/EnvVarPackager.swift | Packages UserDefaults settings into environment variables for the NetBird SDK |
| NetbirdNetworkExtension/NetBirdAdapter.swift | Retrieves and passes environment variables to the NetBird client during startup |
| NetBird/Source/App/Views/AdvancedView.swift | Adds UI toggle for force relay connection, refactors alert overlay handling, and removes trailing periods from toggle labels |
| NetBird/Source/App/ViewModels/MainViewModel.swift | Adds state management for force relay connection setting and alert display |
| NetBird.xcodeproj/project.pbxproj | Registers new source files in the Xcode project for both main app and network extension targets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let userDefaults = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) | ||
| userDefaults?.set(isEnabled, forKey: GlobalConstants.keyForceRelayConnection) | ||
| self.forceRelayConnection = isEnabled | ||
| self.showForceRelayAlert = true |
There was a problem hiding this comment.
[nitpick] The forceRelayAlert lacks automatic dismissal unlike the showLogLevelChangedAlert which auto-dismisses after 3 seconds (see lines 49-52 in MainViewModel.swift). This creates an inconsistent UX pattern where one alert auto-dismisses while the other requires manual dismissal. Consider adding auto-dismissal to setForcedRelayConnection for consistency:
self.showForceRelayAlert = true
DispatchQueue.main.asyncAfter(deadline: .now() + 3) {
self.showForceRelayAlert = false
}| self.showForceRelayAlert = true | |
| self.showForceRelayAlert = true | |
| DispatchQueue.main.asyncAfter(deadline: .now() + 3) { | |
| self.showForceRelayAlert = false | |
| } |
| // | ||
|
|
||
| struct GlobalConstants { | ||
| static let keyForceRelayConnection = "isConnectionForceRelayed" |
There was a problem hiding this comment.
[nitpick] The naming "isConnectionForceRelayed" (past tense) is inconsistent with the boolean flag semantics. A more conventional name would be "forceRelayConnection" or "isRelayConnectionForced" to indicate the setting/state rather than a past action. This would also better align with the property name forceRelayConnection used in the ViewModel.
| static let keyForceRelayConnection = "isConnectionForceRelayed" | |
| static let keyForceRelayConnection = "forceRelayConnection" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
NetbirdKit/EnvVarPackager.swift (1)
14-16: Confirm that the defaultforce relayvalue should betrueRegistering
GlobalConstants.keyForceRelayConnectionwithtruemeans that, if the user never toggled the setting, the VPN will run with forced relay enabled. Please confirm this matches:
- The intended product default, and
- The initial state of the toggle in the Advanced UI and any other consumers of this key,
to avoid a mismatch where the UI appears “off” while the extension behaves as “on”.
NetBird/Source/App/Views/AdvancedView.swift (3)
34-34: Note: Unexplained UI adjustment.The
height: 48parameter addition is not mentioned in the PR objectives. While this may improve layout consistency, it appears unrelated to the force relay feature.
63-63: Note: Unexplained cosmetic text changes.Trailing periods removed from toggle labels. While this improves text consistency, it's not mentioned in the PR objectives.
Also applies to: 81-81, 97-97
284-314: Consider abstracting common alert structure and aligning text style.
ForceRelayAlertandLogLevelAlertshare a similar structure (image, message, button/backdrop dismiss). The text messaging style also differs slightly:
- ForceRelayAlert: "To apply the setting, you will need to reconnect."
- LogLevelAlert: "...will take effect after next connect."
Consider:
- Creating a reusable alert component with configurable content to reduce duplication
- Aligning the messaging style for consistency (e.g., both use "reconnect" or both use "next connect")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
NetBird/Source/App/ViewModels/MainViewModel.swift(3 hunks)NetBird/Source/App/Views/AdvancedView.swift(8 hunks)NetbirdKit/EnvVarPackager.swift(1 hunks)NetbirdNetworkExtension/NetBirdAdapter.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- NetBird/Source/App/ViewModels/MainViewModel.swift
- NetbirdNetworkExtension/NetBirdAdapter.swift
🧰 Additional context used
🧬 Code graph analysis (1)
NetBird/Source/App/Views/AdvancedView.swift (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
setForcedRelayConnection(285-290)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (5)
NetbirdKit/EnvVarPackager.swift (2)
8-20: Overall implementation ofEnvVarPackagerlooks goodThe flow (create env list, guard on failure, read UserDefaults, populate env, return) is straightforward and matches the PR goal of centralizing env packaging.
17-17: Verify that"true"/"false"are the expected values forNB_FORCE_RELAY
String(forceRelayConnection)will pass"true"or"false"toNetBirdSDKEnvListfor theNB_FORCE_RELAYenv key. Please double‑check that the NetBird Go client / SDK side is expecting these literal strings (and not"1"/"0"or presence/absence semantics), so the flag is honored correctly at runtime.NetBird/Source/App/Views/AdvancedView.swift (3)
112-133: LGTM! Force relay toggle correctly implemented.The new "Network & Security" section and force relay toggle follow the established pattern used by other settings in this view. The toggle correctly:
- Binds to the view model property
- Calls
setForcedRelayConnectionon change, which persists to shared UserDefaults- Triggers an alert to inform users about the reconnection requirement
141-151: LGTM! Alert presentation follows consistent pattern.Both alerts now use the unified
alertOverlaymechanism with appropriateonDismisscallbacks that reset the visibility flags.
316-339: LGTM! LogLevelAlert now consistent with ForceRelayAlert.The change to
@EnvironmentObjectalignsLogLevelAlertwith the pattern used byForceRelayAlertand eliminates the need for local state management.
| @ViewBuilder | ||
| private func alertOverlay<Content: View>( | ||
| isPresented: Bool, | ||
| onDismiss: @escaping () -> Void, | ||
| @ViewBuilder content: () -> Content | ||
| ) -> some View { | ||
| if isPresented { | ||
| Color.black.opacity(0.4) | ||
| .edgesIgnoringSafeArea(.all) | ||
| .onTapGesture { | ||
| viewModel.buttonLock = true | ||
| onDismiss() | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 2) { | ||
| viewModel.buttonLock = false | ||
| } | ||
| } | ||
|
|
||
| content() | ||
| .frame(maxWidth: UIScreen.main.bounds.width * 0.9) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=swift -C3 '\bbuttonLock\b'Repository: netbirdio/ios-client
Length of output: 6544
🏁 Script executed:
rg -nP --type=swift -A15 'struct ForceRelayAlert|struct LogLevelAlert' --max-count=10Repository: netbirdio/ios-client
Length of output: 2841
🏁 Script executed:
rg -nP --type=swift -B5 -A5 'alertOverlay.*showForceRelayAlert|alertOverlay.*showLogLevelChangedAlert'Repository: netbirdio/ios-client
Length of output: 1593
Consolidate alert dismissal to use consistent debounce pattern.
The alertOverlay backdrop tap wraps dismissal with buttonLock (sets true, calls onDismiss(), resets after 2 seconds), but the alert buttons in ForceRelayAlert and LogLevelAlert directly set visibility flags without using this mechanism. This creates two dismissal paths:
- Backdrop tap: Uses
buttonLockdebounce +onDismiss()callback - Alert button tap: Directly sets flags, bypasses debounce
While both paths currently set the same state, this architectural inconsistency is problematic for maintenance. If onDismiss() gains additional side effects later (logging, analytics, cleanup), the button path would bypass them. Additionally, the button path allows rapid re-toggling while the backdrop path prevents it.
Have alert buttons call the onDismiss() closure instead of directly manipulating state, or add buttonLock checks to button actions for consistency with the pattern used in MainView.
🤖 Prompt for AI Agents
In NetBird/Source/App/Views/AdvancedView.swift around lines 167-187, the
alertOverlay backdrop tap uses viewModel.buttonLock + onDismiss() debounce, but
the buttons inside ForceRelayAlert and LogLevelAlert bypass this by directly
toggling visibility flags; update those alert button actions to use the same
dismissal path: have the alerts accept and call the onDismiss() closure for
confirm/cancel actions (or, if easier, check viewModel.buttonLock and then set
buttonLock = true, call onDismiss(), and schedule resetting buttonLock after 2
seconds) so both backdrop and buttons run the same side effects and debounce
logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
285-296: Persistence helpers are correct; consider guarding against missing app-group suiteThe new helpers correctly persist and read the flag from the shared
UserDefaultssuite usingGlobalConstants.userPreferencesSuiteName/keyForceRelayConnection, with a default oftrue. One improvement worth considering:
- If
UserDefaults(suiteName:)ever returnsnil(e.g., misconfigured app-group entitlements), the write currently no-ops but still updates the in-memoryforceRelayConnection, potentially desynchronizing UI vs. what the extension sees. Adding an explicitguard+assertionFailurewould make such configuration issues easier to catch during development.Example refactor:
- func setForcedRelayConnection(isEnabled: Bool) { - let userDefaults = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) - userDefaults?.set(isEnabled, forKey: GlobalConstants.keyForceRelayConnection) - self.forceRelayConnection = isEnabled - self.showForceRelayAlert = true - } + func setForcedRelayConnection(isEnabled: Bool) { + guard let userDefaults = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) else { + assertionFailure("Missing UserDefaults suite: \(GlobalConstants.userPreferencesSuiteName)") + self.forceRelayConnection = isEnabled + self.showForceRelayAlert = true + return + } + userDefaults.set(isEnabled, forKey: GlobalConstants.keyForceRelayConnection) + self.forceRelayConnection = isEnabled + self.showForceRelayAlert = true + } - func getForcedRelayConnectionEnabled() -> Bool { - let userDefaults = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) - userDefaults?.register(defaults: [GlobalConstants.keyForceRelayConnection: true]) - return userDefaults?.bool(forKey: GlobalConstants.keyForceRelayConnection) ?? true - } + func getForcedRelayConnectionEnabled() -> Bool { + guard let userDefaults = UserDefaults(suiteName: GlobalConstants.userPreferencesSuiteName) else { + assertionFailure("Missing UserDefaults suite: \(GlobalConstants.userPreferencesSuiteName)") + return true + } + userDefaults.register(defaults: [GlobalConstants.keyForceRelayConnection: true]) + return userDefaults.bool(forKey: GlobalConstants.keyForceRelayConnection) + }This keeps current behavior (defaulting to
true) but surfaces misconfiguration early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
NetBird/Source/App/ViewModels/MainViewModel.swift(3 hunks)
🔇 Additional comments (2)
NetBird/Source/App/ViewModels/MainViewModel.swift (2)
58-60: New Published properties for force-relay state look sound
forceRelayConnectionandshowForceRelayAlertare wired consistently with the rest of the view model’s UI state, and the defaulttruematches the default used ingetForcedRelayConnectionEnabled(). No functional issues here.
80-80: Initialization correctly hydratesforceRelayConnectionfrom persisted settingsUsing
getForcedRelayConnectionEnabled()ininitensures the toggle reflects the shared app-group value after relaunch or when the NE extension has read the same setting. This aligns the UI with the effective configuration.
…eue.main.async So that the call happens in the main thread to correctly handle UI state update (Connecting... -> Connected)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
NetbirdKit/ConnectionListener.swift (1)
25-31: Verify the necessity and impact of the async main thread dispatch.The completion handler is now invoked asynchronously on the main thread, changing the behavior from synchronous to asynchronous completion. This threading change could introduce timing issues if callers expect synchronous completion, or could cause subtle race conditions.
Please clarify:
- What thread does
onConnected()get called from?- Does the completion handler require main thread execution (e.g., for UI updates)?
- Could this async behavior break any existing code that depends on synchronous completion?
Consider adding a comment explaining why main thread dispatch is necessary, to aid future maintainers in understanding this design decision.
This PR allows users to choose whether to enforce usage of relay when connecting to the VPN via an extra toggle control in the Advanced section.
An alert is shown to warn the user that the configuration change will only take effect after the user reestablishes their connection to the VPN.
Value is saved in the app group's UserDefaults so that it can be read in NetbirdNetworkExtension's context in order to be sent as a list of environment variables to be exported when the go client's Run method is called.
Summary by CodeRabbit
New Features
Refactor
UI
✏️ Tip: You can customize this high-level summary in your review settings.