Multi-profiles#80
Merged
pappz merged 10 commits intonetbirdio:mainfrom Apr 22, 2026
Merged
Conversation
Split ProfilesView into ProfilesListView, AddProfileSheet, ProfileBadge, AddProfileViewModel - Add server URL + setup key fields to Add Profile screen - Store per-profile connection data (ip/fqdn/managementURL) as typed model in ProfileConnectionCache - Show cached connection info immediately on profile switch; empty if no prior data - Fix profile deletion persistence via tombstone in profiles.json - Fix logout to remove both netbird.cfg and state.json (cfg holds auth tokens) - Preserve managementURL in UI after logout via cache fallback - Guard polling from overwriting new profile's data during disconnect/reconnect cycle
…ow current server URL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-profile support (ProfileManager, per-profile paths and cache), iOS profile UI and view models, profile-aware MainViewModel polling and navigation, adapter/extension IPC and config-path handling, replaces SFSafariViewController with ASWebAuthenticationSession, and updates Xcode project file references. Changes
Sequence DiagramssequenceDiagram
actor User
participant AddProfileSheet
participant AddProfileVM
participant ProfileManager
participant ServerVM
participant NetworkExtension
User->>AddProfileSheet: Tap Create (name, serverUrl, setupKey)
AddProfileSheet->>AddProfileVM: create(name, serverUrl, setupKey)
AddProfileVM->>ProfileManager: addProfile(name)
ProfileManager-->>AddProfileVM: configPath (or error)
AddProfileVM->>ServerVM: init(with: configPath)
AddProfileVM->>ServerVM: login(setupKey) / changeManagementServer(url)
ServerVM->>NetworkExtension: send IPC login/change request
NetworkExtension-->>ServerVM: IPC response (success / error)
alt success
ServerVM-->>AddProfileVM: success
AddProfileVM-->>AddProfileSheet: publish isSuccess -> onCreated
else failure
ServerVM-->>AddProfileVM: errors
AddProfileVM->>ProfileManager: removeProfile(name)
AddProfileVM-->>AddProfileSheet: publish errors
end
sequenceDiagram
actor User
participant ProfilesListView
participant ProfileManager
participant MainVM
participant ProfileConnectionCache
participant NetworkExtension
User->>ProfilesListView: Select "Switch to ProfileB"
ProfilesListView->>ProfileManager: switchProfile("ProfileB")
ProfileManager-->>ProfilesListView: confirmation
ProfilesListView->>MainVM: switchConnectionInfo(to: "ProfileB")
MainVM->>ProfileConnectionCache: entry(for: "ProfileB")
ProfileConnectionCache-->>MainVM: fqdn, ip, managementURL
MainVM->>NetworkExtension: apply profile config/state paths & close/reopen connection
NetworkExtension-->>MainVM: extension state events
alt connected after switch
MainVM->>MainVM: clear profileSwitchPending & refresh UI
MainVM-->>ProfilesListView: update activeProfileName
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NetbirdKit/NetworkExtensionAdapter.swift (1)
541-586:⚠️ Potential issue | 🔴 Critical
login(completion:)still leaks the continuation whensendProviderMessagethrows.
performLogin()awaits this callback withwithCheckedContinuation. IfsendProviderMessagethrows, thecatchpath only logs and returns, so the continuation never resumes and the login flow hangs.💡 Suggested fix
} catch { - print("error when performing network extension action") + logger.error("login: Failed to send provider message: \(error.localizedDescription)") + completion(nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/NetworkExtensionAdapter.swift` around lines 541 - 586, The catch block in login(completion:) swallows errors from session.sendProviderMessage and never resumes the awaiting continuation in performLogin(), causing a hang; modify the catch to call the provided completion (e.g., completion(nil) or completion with an error string) and log the error so the continuation always resumes, and also ensure any other early-exit paths (e.g., failure converting messageString to Data) already call completion—update the catch around try session.sendProviderMessage and any thrown paths in login(completion:) to always invoke completion to guarantee performLogin()'s withCheckedContinuation is resumed.
🧹 Nitpick comments (1)
NetbirdNetworkExtension/PacketTunnelProvider.swift (1)
121-133: Consider adding logging for malformedLogin:payloads.When
parts.count != 2(line 125), the code silently proceeds tologin()without logging the malformed message. This could mask IPC format issues or bugs in the sender.🔧 Suggested improvement
case let s where s.hasPrefix("Login:"): // Format: "Login:<configPath>|<statePath>" let payload = String(s.dropFirst("Login:".count)) let parts = payload.components(separatedBy: "|") if parts.count == 2 { let configPath = parts[0] let statePath = parts[1] if configPath != adapter?.initializedConfigPath { AppLogger.shared.log("handleAppMessage: profile change detected, recreating adapter for \(configPath)") adapter = NetBirdAdapter(with: tunnelManager, configPath: configPath, statePath: statePath) } + } else { + AppLogger.shared.log("handleAppMessage: malformed Login payload, expected 2 parts but got \(parts.count)") } login(completionHandler: completionHandler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdNetworkExtension/PacketTunnelProvider.swift` around lines 121 - 133, The Login: payload handler currently ignores malformed payloads when parts.count != 2 and proceeds to call login(completionHandler:), which hides IPC format errors; add a log entry (via AppLogger.shared.log or similar) in the Login case to record the raw payload (s or payload) and indicate a malformed Login message when parts.count != 2, so you can detect and debug sender/format issues before calling login(completionHandler:).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NetBird/Source/App/ViewModels/AddProfileViewModel.swift`:
- Around line 42-43: The code lowercases the entire serverUrl when creating
trimmedUrl and finalUrl, which can alter case-sensitive path/query components;
change the normalization to only lowercase the URL scheme and host using
URLComponents (parse serverUrl into URLComponents, lowercase components.scheme
and components.host, reassemble) and then fall back to
defaultManagementServerUrl if the resulting trimmed/normalized host+scheme are
empty; update references in AddProfileViewModel where trimmedUrl and finalUrl
are computed (serverUrl, trimmedUrl, finalUrl, defaultManagementServerUrl) to
use this URLComponents-based normalization.
In `@NetBird/Source/App/Views/Components/SafariView.swift`:
- Around line 72-78: The presentationAnchor(for session:
ASWebAuthenticationSession) -> ASPresentationAnchor implementation currently
falls back to creating a bare UIWindow() which can cause presentation failures;
change it to first attempt a proper key window or scene window and if none
exists log a warning and in debug build call fatalError (or assert) to surface
the unexpected state, and in release construct a UIWindow with frame =
UIScreen.main.bounds and a minimal rootViewController before returning it so the
ASWebAuthenticationSession has a valid presentation anchor; update any logging
to include context about missing key window.
- Around line 46-48: ASWebAuthenticationSession is being initialized with
callbackURLScheme: "http", which is invalid; change the callback scheme to a
registered custom scheme (e.g., "netbird") or, for iOS 17.4+ localhost/https
redirects, use Callback.https(host:path:) instead. Locate the
ASWebAuthenticationSession creation in SafariView.swift (the session variable
created with parent.url and callbackURLScheme) and replace the hardcoded "http"
with your app's custom URL scheme registered in Info.plist (or construct the
session with Callback.https(host:path:) on supported OS versions), ensuring the
scheme matches the Info.plist entry and handling any availability checks for iOS
17.4+.
- Around line 44-63: The SafariView currently calls parent.didFinish()
unconditionally; change it to surface success vs failure by updating the
didFinish API and the ASWebAuthenticationSession completion handling: modify the
SafariView's parent.didFinish to accept a Bool or Result (e.g.,
didFinish(success: Bool) or didFinish(result: Result<URL, Error>)), then in
SafariView.startSession's ASWebAuthenticationSession completion closure call
parent.didFinish(true) when callbackURL is non-nil and indicates success (or
pass .success(callbackURL)), and call parent.didFinish(false) (or .failure(error
or a cancellation error) ) when error exists or login was cancelled (check for
ASWebAuthenticationSessionError .canceledLogin); only wire iOSConnectionView to
startVPNConnection when didFinish reports success. Ensure to update all call
sites of didFinish accordingly (e.g., SafariView initializer usage in
iOSConnectionView).
In `@NetBird/Source/App/Views/iOS/ProfilesListView.swift`:
- Around line 168-180: The UI is updating the cached connection state before the
filesystem profile switch completes, because switchToProfile calls
viewModel.switchConnectionInfo(to:) (which clears peers and sets
profileSwitchPending) before ProfileManager.shared.switchProfile(...) can throw;
move the UI updates so they only run after a successful switch: call
viewModel.performClose() first, then attempt try
ProfileManager.shared.switchProfile(profile.name), and only on success call
viewModel.switchConnectionInfo(to:), viewModel.reloadConfiguration(), set
viewModel.activeProfileName = ProfileManager.shared.getActiveProfileName(),
update Preferences.saveManagementURL(...) if managementURL exists, and call
loadProfiles(); keep error handling in the catch block unchanged so a failed
switch does not alter the displayed cached connection info.
In `@NetbirdKit/ProfileConnectionCache.swift`:
- Around line 20-56: Add explicit invalidation paths to ProfileConnectionCache:
implement clear(profile:) that removes the profile key from the persisted
dictionary and persists the result, and implement clearConnectionState(for:)
that resets the connection fields (ip and fqdn) for the given profile but
preserves managementURL, then persist; use the existing helpers load() and
persist(...) and mirror how save(...) and saveManagementURL(...) mutate the
stored dictionary. After adding those methods, call clearConnectionState(for:)
from the logout flow (where logoutProfile(_:) is invoked) so connection state is
cleared but managementURL survives, and call clear(profile:) from the profile
removal flow (where ProfileManager.removeProfile(_:) is executed) to delete all
cached data for deleted profiles. Ensure method names match
ProfileConnectionCache.clear(profile:) and
ProfileConnectionCache.clearConnectionState(for:) so callers can locate them.
In `@NetbirdKit/ProfileManager.swift`:
- Around line 285-293: In migrateIfNeeded(), instead of copying
legacyConfig/legacyState into newConfig/newState (which leaves the originals
behind), move the files so the legacy files are removed; replace the
fileManager.copyItem calls for legacyConfig->newConfig and legacyState->newState
with fileManager.moveItem(atPath:toPath:) and log on success. If you want a safe
fallback, perform copy first and only call fileManager.removeItem(atPath:) on
the source after the copy succeeds; reference the migrateIfNeeded() function and
the legacyConfig, legacyState, newConfig, newState symbols when making the
change.
---
Outside diff comments:
In `@NetbirdKit/NetworkExtensionAdapter.swift`:
- Around line 541-586: The catch block in login(completion:) swallows errors
from session.sendProviderMessage and never resumes the awaiting continuation in
performLogin(), causing a hang; modify the catch to call the provided completion
(e.g., completion(nil) or completion with an error string) and log the error so
the continuation always resumes, and also ensure any other early-exit paths
(e.g., failure converting messageString to Data) already call completion—update
the catch around try session.sendProviderMessage and any thrown paths in
login(completion:) to always invoke completion to guarantee performLogin()'s
withCheckedContinuation is resumed.
---
Nitpick comments:
In `@NetbirdNetworkExtension/PacketTunnelProvider.swift`:
- Around line 121-133: The Login: payload handler currently ignores malformed
payloads when parts.count != 2 and proceeds to call login(completionHandler:),
which hides IPC format errors; add a log entry (via AppLogger.shared.log or
similar) in the Login case to record the raw payload (s or payload) and indicate
a malformed Login message when parts.count != 2, so you can detect and debug
sender/format issues before calling login(completionHandler:).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90b224c4-2c5f-4f47-ae2c-934f1890ec82
📒 Files selected for processing (16)
NetBird.xcodeproj/project.pbxprojNetBird/Source/App/ViewModels/AddProfileViewModel.swiftNetBird/Source/App/ViewModels/MainViewModel.swiftNetBird/Source/App/Views/Components/SafariView.swiftNetBird/Source/App/Views/ServerView.swiftNetBird/Source/App/Views/iOS/AddProfileSheet.swiftNetBird/Source/App/Views/iOS/ProfileBadge.swiftNetBird/Source/App/Views/iOS/ProfilesListView.swiftNetBird/Source/App/Views/iOS/iOSConnectionView.swiftNetBird/Source/App/Views/iOS/iOSSettingsView.swiftNetbirdKit/NetworkExtensionAdapter.swiftNetbirdKit/Preferences.swiftNetbirdKit/ProfileConnectionCache.swiftNetbirdKit/ProfileManager.swiftNetbirdNetworkExtension/NetBirdAdapter.swiftNetbirdNetworkExtension/PacketTunnelProvider.swift
There was a problem hiding this comment.
🧹 Nitpick comments (9)
NetbirdKit/ConfigurationProvider.swift (1)
117-127: Early return preserves stale preferences when config is deleted.The guard prevents
preferencesfrom being recreated when the config file is missing, which avoids overwriting custom server URLs. However, this meanspreferencesmay hold stale values from a deleted config until the app restarts.If a profile is logged out and its config deleted, subsequent reads from
configProviderwill return outdated values. Consider whether callers need to be aware of this behavior, or whether thepreferencesobject should be invalidated/reset when the config file is known to be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/ConfigurationProvider.swift` around lines 117 - 127, The reload() early-return keeps self.preferences populated when the config file is deleted, causing stale values; update reload() in ConfigurationProvider so that when Preferences.configFile() is missing or FileManager reports file not found you explicitly invalidate or reset self.preferences (e.g., set to nil or a cleared Preferences instance) instead of returning early; ensure callers of ConfigurationProvider.reload() and consumers of the preferences property can handle the cleared state or adjust Preferences.newPreferences()/Preferences to provide an explicit empty/default instance.NetbirdKit/NetworkExtensionAdapter.swift (2)
96-142:restoreConfigIfMissing()implementation is sound but duplicated.This method correctly restores a minimal config with the nested
ManagementURLobject format expected by the Go SDK. The fallback chain (savedServerURL → ProfileConnectionCache) is appropriate.However, this logic is duplicated in
PacketTunnelProvider.handleAppMessage(). Consider extracting to a shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/NetworkExtensionAdapter.swift` around lines 96 - 142, The restoreConfigIfMissing() logic is duplicated in PacketTunnelProvider.handleAppMessage(); extract the URL-to-minimal-config serialization and file-write steps into a single shared helper (e.g., a new NetbirdKit utility function like ConfigRestorer.writeMinimalConfig(forProfile:) or Preferences.restoreConfigIfMissing(profileName:)) that encapsulates parsing URL, building the Go-style ManagementURL JSON, jsonEscape, and writing to Preferences.configFile(); then replace both restoreConfigIfMissing() and the code in PacketTunnelProvider.handleAppMessage() to call that helper and retain the same logging behavior via the existing logger.
612-625: Consider using a delimiter-safe format for IPC messages.The format
"Login:<configPath>|<statePath>[|<managementURL>]"uses|as a delimiter. While iOS file paths are unlikely to contain this character, using a delimiter-safe format like JSON would be more robust and easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/NetworkExtensionAdapter.swift` around lines 612 - 625, The current IPC payload built in variable messageString uses a pipe-delimited string ("Login:<configPath>|<statePath>[|<managementURL>]") which is fragile; change construction to a delimiter-safe JSON object instead (e.g., {"action":"Login","configPath":..., "statePath":..., "managementURL":...}) and serialize it to a string before sending. Locate the code that builds messageString in NetworkExtensionAdapter (the block using Preferences.configFile(), Preferences.stateFile(), ProfileManager.shared.getActiveProfileName(), and ProfileConnectionCache().managementURL(for:)) and replace the manual concatenation with creation of a Dictionary or Codable struct representing the fields and convert it to JSON via JSONSerialization or JSONEncoder; ensure you only include managementURL when non-empty so receivers that parse JSON still get the same optional semantics.NetbirdKit/ProfileManager.swift (3)
66-100: Retry deletion inlistProfiles()has no error reporting.Lines 72-76 attempt to delete directories for profiles in the
deletedProfilestombstone set, but failures are silently ignored withtry?. While this is intentional retry logic, consider logging deletion failures to aid debugging when profiles unexpectedly reappear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/ProfileManager.swift` around lines 66 - 100, In listProfiles(), the retry deletion loop uses try? so failures are swallowed; change the deletion logic for each name from a silent try? around fileManager.removeItem(atPath:) to a do/catch that catches errors and logs them (use AppLogger.shared.log or the existing logging facility) including the profile name and error; reference the deletedSet, profileDirectory(for:), fileManager.removeItem(atPath:) and ensure the log message gives context so failed deletions are visible during troubleshooting.
43-61: Singleton initialization triggers side effects.The
ProfileManager.sharedsingleton callsensureProfilesDirectory()andmigrateIfNeeded()duringinit(). If the App Group container is unavailable (returnsnil), these methods silently return without logging, potentially leaving the manager in an inconsistent state where later operations fail unexpectedly.Consider adding defensive logging or an early-exit check in
init()ifcontainerURL()returnsnil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/ProfileManager.swift` around lines 43 - 61, The singleton initializer ProfileManager.shared currently runs ensureProfilesDirectory() and migrateIfNeeded() which rely on containerURL() and may silently no-op if the App Group container is nil; update the private init() in ProfileManager to first call containerURL() (or a helper that wraps it) and if it returns nil log an error via the existing logging facility and return early (avoid calling ensureProfilesDirectory() and migrateIfNeeded()), or alternatively throw/mark the manager as unavailable so later methods can surface a clear error; ensure references to containerURL(), ensureProfilesDirectory(), migrateIfNeeded(), and ProfileManager.shared are used so the check stops the silent failure path.
224-251:managementURL(for:)has side effects on read — consider documenting or separating.This getter not only reads the URL but also persists it to both a dedicated file and
ProfileConnectionCache(lines 241-242). While this ensures durability, it may surprise callers expecting a pure read operation. Consider adding a comment or splitting into explicitreadManagementURLandensureManagementURLPersistedmethods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/ProfileManager.swift` around lines 224 - 251, The managementURL(for:) function currently has hidden write side-effects (it calls saveServerURL(_:for:) and ProfileConnectionCache().saveManagementURL(_:for:) when reading), so refactor to make reads pure: remove the persistence calls from managementURL(for:) and either 1) add a new explicit method ensureManagementURLPersisted(_:for:) that performs saveServerURL and ProfileConnectionCache().saveManagementURL, or 2) document in managementURL(for:) that callers must invoke a new persist method; update callers to call ensureManagementURLPersisted(url, for:) after they obtain a URL, and keep savedServerURL(for:) and ProfileConnectionCache().managementURL(for:) as the read fallbacks. Ensure the new method and managementURL(for:) are referenced where appropriate and add a brief comment to managementURL(for:) explaining it no longer mutates state.NetbirdNetworkExtension/PacketTunnelProvider.swift (2)
123-160: Duplicated config restoration logic and JSON escaping.The minimal config restoration logic (lines 136-153) and
jsonEscapefunction (lines 144-147) are duplicated fromNetworkExtensionAdapter.restoreConfigIfMissing(). Consider extracting this into a shared utility to avoid divergence.Additionally, the JSON escaping only handles backslashes and quotes but not other special characters like newlines or control characters. For URLs, this is likely sufficient, but a more robust approach would use
JSONSerializationorJSONEncoder.💡 Consider using JSONSerialization for safer encoding
// Instead of manual string interpolation: let urlDict: [String: Any] = [ "ManagementURL": [ "Scheme": scheme, "Host": goHost, "Path": path ] ] if let data = try? JSONSerialization.data(withJSONObject: urlDict), let minimalConfig = String(data: data, encoding: .utf8) { // write minimalConfig }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdNetworkExtension/PacketTunnelProvider.swift` around lines 123 - 160, Extract the duplicated minimal-config creation and escaping into a shared utility (e.g., NetworkExtensionAdapter.restoreConfigIfMissing() currently contains similar logic) by moving the logic that builds the ManagementURL dictionary into a common helper (name it something like buildMinimalConfigJSON(managementURL: String) or restoreConfigIfMissing(at:configPath:managementURL:)) and call it from PacketTunnelProvider.handleAppMessage and NetworkExtensionAdapter.restoreConfigIfMissing(); replace the manual jsonEscape function with JSONSerialization/JSONEncoder to produce the minimalConfig JSON string (construct a Dictionary ["ManagementURL":["Scheme":scheme,"Host":goHost,"Path":path]] and serialize it safely before writing to configPath).
155-158: Adapter recreation after config restoration may use stale paths.When
configRestoredis true, the adapter is recreated even ifconfigPath == adapter?.initializedConfigPath. This is correct because the config file contents changed. However, the conditionconfigPath != adapter?.initializedConfigPath || configRestoredcould be simplified for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdNetworkExtension/PacketTunnelProvider.swift` around lines 155 - 158, The current condition mixes path mismatch and configRestored and is unclear; change the logic in PacketTunnelProvider so you recreate the adapter when there is no adapter, when configRestored is true, or when configPath differs from adapter?.initializedConfigPath. Replace the existing if (configPath != adapter?.initializedConfigPath || configRestored) check with a clear check such as if adapter == nil || configRestored || configPath != adapter?.initializedConfigPath, and update the log to indicate whether this is a restore-driven recreation (use adapter, initializedConfigPath, configRestored, NetBirdAdapter(with:tunnelManager,configPath:statePath:) to locate where to change).NetBird/Source/App/ViewModels/MainViewModel.swift (1)
453-458: Profile switch detection relies on state transition — verify timing.The guard clears when
previousExtensionState != .connected && currentState == .connected. If the extension reconnects very quickly (within one polling interval) without going through a disconnected state, the guard might not clear properly.This seems unlikely in practice since
performClose()is called before switching, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetBird/Source/App/ViewModels/MainViewModel.swift` around lines 453 - 458, The current guard that clears profileSwitchPending only on a transition from non-connected to connected can miss very fast reconnects; to make detection robust, when initiating a profile switch (e.g., in performClose() or the profile switch entry point) explicitly set previousExtensionState = .disconnected or clear profileSwitchPending there, or alternately detect the switch by comparing the profile identifier rather than relying solely on previousExtensionState/currentState; update the logic in the methods around profileSwitchPending, previousExtensionState and performClose() to ensure the pending flag is cleared deterministically during the switch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@NetBird/Source/App/ViewModels/MainViewModel.swift`:
- Around line 453-458: The current guard that clears profileSwitchPending only
on a transition from non-connected to connected can miss very fast reconnects;
to make detection robust, when initiating a profile switch (e.g., in
performClose() or the profile switch entry point) explicitly set
previousExtensionState = .disconnected or clear profileSwitchPending there, or
alternately detect the switch by comparing the profile identifier rather than
relying solely on previousExtensionState/currentState; update the logic in the
methods around profileSwitchPending, previousExtensionState and performClose()
to ensure the pending flag is cleared deterministically during the switch.
In `@NetbirdKit/ConfigurationProvider.swift`:
- Around line 117-127: The reload() early-return keeps self.preferences
populated when the config file is deleted, causing stale values; update reload()
in ConfigurationProvider so that when Preferences.configFile() is missing or
FileManager reports file not found you explicitly invalidate or reset
self.preferences (e.g., set to nil or a cleared Preferences instance) instead of
returning early; ensure callers of ConfigurationProvider.reload() and consumers
of the preferences property can handle the cleared state or adjust
Preferences.newPreferences()/Preferences to provide an explicit empty/default
instance.
In `@NetbirdKit/NetworkExtensionAdapter.swift`:
- Around line 96-142: The restoreConfigIfMissing() logic is duplicated in
PacketTunnelProvider.handleAppMessage(); extract the URL-to-minimal-config
serialization and file-write steps into a single shared helper (e.g., a new
NetbirdKit utility function like ConfigRestorer.writeMinimalConfig(forProfile:)
or Preferences.restoreConfigIfMissing(profileName:)) that encapsulates parsing
URL, building the Go-style ManagementURL JSON, jsonEscape, and writing to
Preferences.configFile(); then replace both restoreConfigIfMissing() and the
code in PacketTunnelProvider.handleAppMessage() to call that helper and retain
the same logging behavior via the existing logger.
- Around line 612-625: The current IPC payload built in variable messageString
uses a pipe-delimited string
("Login:<configPath>|<statePath>[|<managementURL>]") which is fragile; change
construction to a delimiter-safe JSON object instead (e.g.,
{"action":"Login","configPath":..., "statePath":..., "managementURL":...}) and
serialize it to a string before sending. Locate the code that builds
messageString in NetworkExtensionAdapter (the block using
Preferences.configFile(), Preferences.stateFile(),
ProfileManager.shared.getActiveProfileName(), and
ProfileConnectionCache().managementURL(for:)) and replace the manual
concatenation with creation of a Dictionary or Codable struct representing the
fields and convert it to JSON via JSONSerialization or JSONEncoder; ensure you
only include managementURL when non-empty so receivers that parse JSON still get
the same optional semantics.
In `@NetbirdKit/ProfileManager.swift`:
- Around line 66-100: In listProfiles(), the retry deletion loop uses try? so
failures are swallowed; change the deletion logic for each name from a silent
try? around fileManager.removeItem(atPath:) to a do/catch that catches errors
and logs them (use AppLogger.shared.log or the existing logging facility)
including the profile name and error; reference the deletedSet,
profileDirectory(for:), fileManager.removeItem(atPath:) and ensure the log
message gives context so failed deletions are visible during troubleshooting.
- Around line 43-61: The singleton initializer ProfileManager.shared currently
runs ensureProfilesDirectory() and migrateIfNeeded() which rely on
containerURL() and may silently no-op if the App Group container is nil; update
the private init() in ProfileManager to first call containerURL() (or a helper
that wraps it) and if it returns nil log an error via the existing logging
facility and return early (avoid calling ensureProfilesDirectory() and
migrateIfNeeded()), or alternatively throw/mark the manager as unavailable so
later methods can surface a clear error; ensure references to containerURL(),
ensureProfilesDirectory(), migrateIfNeeded(), and ProfileManager.shared are used
so the check stops the silent failure path.
- Around line 224-251: The managementURL(for:) function currently has hidden
write side-effects (it calls saveServerURL(_:for:) and
ProfileConnectionCache().saveManagementURL(_:for:) when reading), so refactor to
make reads pure: remove the persistence calls from managementURL(for:) and
either 1) add a new explicit method ensureManagementURLPersisted(_:for:) that
performs saveServerURL and ProfileConnectionCache().saveManagementURL, or 2)
document in managementURL(for:) that callers must invoke a new persist method;
update callers to call ensureManagementURLPersisted(url, for:) after they obtain
a URL, and keep savedServerURL(for:) and
ProfileConnectionCache().managementURL(for:) as the read fallbacks. Ensure the
new method and managementURL(for:) are referenced where appropriate and add a
brief comment to managementURL(for:) explaining it no longer mutates state.
In `@NetbirdNetworkExtension/PacketTunnelProvider.swift`:
- Around line 123-160: Extract the duplicated minimal-config creation and
escaping into a shared utility (e.g.,
NetworkExtensionAdapter.restoreConfigIfMissing() currently contains similar
logic) by moving the logic that builds the ManagementURL dictionary into a
common helper (name it something like buildMinimalConfigJSON(managementURL:
String) or restoreConfigIfMissing(at:configPath:managementURL:)) and call it
from PacketTunnelProvider.handleAppMessage and
NetworkExtensionAdapter.restoreConfigIfMissing(); replace the manual jsonEscape
function with JSONSerialization/JSONEncoder to produce the minimalConfig JSON
string (construct a Dictionary
["ManagementURL":["Scheme":scheme,"Host":goHost,"Path":path]] and serialize it
safely before writing to configPath).
- Around line 155-158: The current condition mixes path mismatch and
configRestored and is unclear; change the logic in PacketTunnelProvider so you
recreate the adapter when there is no adapter, when configRestored is true, or
when configPath differs from adapter?.initializedConfigPath. Replace the
existing if (configPath != adapter?.initializedConfigPath || configRestored)
check with a clear check such as if adapter == nil || configRestored ||
configPath != adapter?.initializedConfigPath, and update the log to indicate
whether this is a restore-driven recreation (use adapter, initializedConfigPath,
configRestored, NetBirdAdapter(with:tunnelManager,configPath:statePath:) to
locate where to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 936cc0ed-d4dc-4406-b356-4e752d3af431
📒 Files selected for processing (7)
NetBird/Source/App/ViewModels/MainViewModel.swiftNetBird/Source/App/Views/MainView.swiftNetbirdKit/ConfigurationProvider.swiftNetbirdKit/GlobalConstants.swiftNetbirdKit/NetworkExtensionAdapter.swiftNetbirdKit/ProfileManager.swiftNetbirdNetworkExtension/PacketTunnelProvider.swift
✅ Files skipped from review due to trivial changes (1)
- NetbirdKit/GlobalConstants.swift
- Normalize only scheme and host to lowercase when saving management server URL; previously the full URL was lowercased which could corrupt case-sensitive paths on self-hosted servers - Move switchConnectionInfo(to:) inside the do-block so the connection UI is only updated after a successful profile switch - Add ProfileConnectionCache.remove(for:) and clearConnectionData(for:) to prevent stale ip/fqdn/managementURL from persisting after a profile is deleted or logged out; call them from removeProfile and logoutProfile - Use ASWebAuthenticationSession.Callback.customScheme on iOS 17.4+, falling back to the deprecated callbackURLScheme initializer on older versions; add a comment explaining why "http" is used and that a proper fix requires custom-scheme support in the SDK - Guard presentationAnchor against a missing key window with assertionFailure in debug builds instead of silently returning an empty UIWindow
Closed
There was a problem hiding this comment.
Actionable comments posted: 3
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/ViewModels/MainViewModel.swift (1)
246-257:⚠️ Potential issue | 🟡 MinorPotential stale-state check after
start().
start()awaitsconfigureManager()+loginIfRequired()but doesn't refreshself.extensionState; that happens asynchronously viacheckExtensionState()on the next poll. When this task resumes,self.extensionStatemay still reflect the pre-start value (e.g..connectedfor a quick re-connect or.disconnectedstale), so the recovery branch can either miss a genuine stuck-connecting state or mis-fire.Consider calling
checkExtensionState()(or awaitingloadCurrentConnectionState()) before the condition, or gating on a timeout rather than the possibly-stale published value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetBird/Source/App/ViewModels/MainViewModel.swift` around lines 246 - 257, The post-start stale-state check can read an outdated self.extensionState; after awaiting networkExtensionAdapter.start() in MainViewModel, explicitly refresh the extension state before deciding to reset UI: call await checkExtensionState() or await loadCurrentConnectionState() (or otherwise await a brief timeout-gated state refresh) and then use the refreshed extensionState when deciding to set connectPressed = false and call updateVPNDisplayState(); this ensures the recovery branch uses up-to-date state and avoids mis-firing on quick reconnects.
🧹 Nitpick comments (4)
NetBird/Source/App/Views/iOS/ProfilesListView.swift (2)
88-96: Nit: hardcoded"default"string.
ProfileManagerhas its owndefaultProfileNameconstant but it'sprivate. Consider exposing it (e.g. astatic let defaultProfileName = "default") so views don't have to duplicate the literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetBird/Source/App/Views/iOS/ProfilesListView.swift` around lines 88 - 96, Replace the hardcoded "default" string in ProfilesListView with a single source of truth by making ProfileManager expose its constant: add a public/static constant (e.g. inside ProfileManager declare `static let defaultProfileName = "default"` or change the existing private constant to `public static let defaultProfileName`) and then use `ProfileManager.defaultProfileName` in the swipeActions conditional in ProfilesListView (where profile.name != "default") so the view references the new constant instead of duplicating the literal.
196-207: Consider refreshingactiveProfileNameafter logging out the active profile.When the user logs out the currently-active profile, you call
performClose()but don't touchviewModel.activeProfileNameor cached IP/FQDN. The profile remains marked active (correctly), but any UI derived from the in-memory connection cache for that profile will still show stale IP/FQDN after the state files are gone. CallingviewModel.switchConnectionInfo(to: profile.name)— which clears peers/resets managementStatus and falls back to the cache entry — would make the UI immediately reflect the logged-out state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetBird/Source/App/Views/iOS/ProfilesListView.swift` around lines 196 - 207, When logging out the active profile inside logoutProfile(_ profile: Profile), after calling viewModel.performClose() also call viewModel.switchConnectionInfo(to: profile.name) so the viewModel refreshes activeProfileName and clears/reset cached connection info (peers/managementStatus) so the UI no longer shows stale IP/FQDN for the logged-out profile; ensure this call happens before loadProfiles()/error handling so the UI state immediately reflects the logout.NetbirdKit/NetworkExtensionAdapter.swift (1)
620-633: Optional: consider a more robust payload encoding for the login IPC.
"|"is used as a delimiter for a triple whose last component is a URL. Real-world paths/URLs shouldn't contain|, so this is safe in practice, but JSON (or URL-percent-encoding) would make the contract explicit and future-proof against any path that happens to contain|.Not blocking — just flagging for future hardening.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/NetworkExtensionAdapter.swift` around lines 620 - 633, The IPC login payload uses a pipe-delimited string in messageString which can break if paths/URLs ever contain "|" — change the payload to an explicit encoded format (e.g., JSON) instead: build a dictionary/object with keys like "action":"Login", "configPath": Preferences.configFile(), "statePath": Preferences.stateFile(), "managementURL": ProfileConnectionCache().managementURL(for: ProfileManager.shared.getActiveProfileName()) and serialize it (JSON or percent-encoding) before sending so parsing on the receiver side is robust and unambiguous.NetbirdKit/ProfileManager.swift (1)
226-253: Consider:managementURL(for:)silently writes to disk as a side effect of reading.When the cfg file resolves a URL, this method also writes it to the server-URL sidecar and updates
ProfileConnectionCache. That's useful for cache-warming, but makes a "get" method I/O-heavy and non-idempotent in terms of filesystem writes (called from UI code inProfilesListView.bodytwice per render of the list). If profile lists get large this could become noticeable; a lazier "resolve and persist" helper, with a cheap read-only accessor for list views, would be a cleaner separation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetbirdKit/ProfileManager.swift` around lines 226 - 253, managementURL(for:) currently performs disk writes and cache updates as a side effect while resolving a URL, which makes a read operation I/O-heavy and non-idempotent; refactor by extracting the pure read-only resolution logic into a new method (e.g., resolveManagementURL(for:) or managementURLReadOnly(for:)) that only reads configPath, fileManager.contents, and JSON parsing and returns the resolved URL without calling saveServerURL(_:for:) or ProfileConnectionCache().saveManagementURL(_:for:), and move the persistence logic into a separate method (e.g., persistManagementURL(_:for:)) that callers can invoke when they explicitly want cache-warming; update callers that run during UI rendering (ProfilesListView.body) to use the new read-only accessor and call the persisting helper only from non-UI flows (e.g., login or explicit cache-refresh paths).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NetBird/Source/App/Views/Components/SafariView.swift`:
- Around line 50-64: Add a short comment above the ASWebAuthenticationSession
completion handler explaining why parent.didFinish() must be called
unconditionally (the Go SDK's local HTTP server may consume the
PKCE/http://localhost redirect so the completion can fire with callbackURL ==
nil and error == nil); remove the raw print of callbackURL.absoluteString in the
completion handler (or replace it with redacted logging that only includes
scheme/host/path) to avoid leaking sensitive query parameters; keep
parent.isPresented = false and the unconditional call to parent.didFinish() in
SafariView's completionHandler.
In `@NetbirdKit/NetworkExtensionAdapter.swift`:
- Around line 122-134: The JSON serialization of parsedURL.Host (used to build
minimalConfig) fails for IPv6 because Swift's parsedURL.host returns unbracketed
literals (e.g. "::1") while Go expects bracketed form when a port is present;
update the logic that builds goHost (and factor it into a small helper used by
NetworkExtensionAdapter.swift and PacketTunnelProvider.swift) to wrap IPv6 hosts
in brackets when they contain ":" and are not already bracketed (so produce
"[::1]" or "[::1]:443"), then continue to append the port if present and use
jsonEscape as before when building minimalConfig.
In `@NetbirdKit/ProfileManager.swift`:
- Around line 117-135: The tombstone-clear path currently swallows removal
errors with try? when calling fileManager.removeItem(atPath: dir) and try?
writeMeta(meta), which leads to misleading ProfileError.alreadyExists(sanitized)
later; change those to propagate failures instead: replace the silent try? on
fileManager.removeItem(atPath:) and writeMeta(_:) with throwing calls and map
the caught errors to ProfileError.fileSystemError (or a new descriptive
ProfileError like .cleanupFailed) so callers can distinguish "cannot remove
stale directory" from a true already-exists condition; keep the existing guard
using fileManager.fileExists(atPath: dir) after the cleanup attempt so failures
are surfaced correctly.
---
Outside diff comments:
In `@NetBird/Source/App/ViewModels/MainViewModel.swift`:
- Around line 246-257: The post-start stale-state check can read an outdated
self.extensionState; after awaiting networkExtensionAdapter.start() in
MainViewModel, explicitly refresh the extension state before deciding to reset
UI: call await checkExtensionState() or await loadCurrentConnectionState() (or
otherwise await a brief timeout-gated state refresh) and then use the refreshed
extensionState when deciding to set connectPressed = false and call
updateVPNDisplayState(); this ensures the recovery branch uses up-to-date state
and avoids mis-firing on quick reconnects.
---
Nitpick comments:
In `@NetBird/Source/App/Views/iOS/ProfilesListView.swift`:
- Around line 88-96: Replace the hardcoded "default" string in ProfilesListView
with a single source of truth by making ProfileManager expose its constant: add
a public/static constant (e.g. inside ProfileManager declare `static let
defaultProfileName = "default"` or change the existing private constant to
`public static let defaultProfileName`) and then use
`ProfileManager.defaultProfileName` in the swipeActions conditional in
ProfilesListView (where profile.name != "default") so the view references the
new constant instead of duplicating the literal.
- Around line 196-207: When logging out the active profile inside
logoutProfile(_ profile: Profile), after calling viewModel.performClose() also
call viewModel.switchConnectionInfo(to: profile.name) so the viewModel refreshes
activeProfileName and clears/reset cached connection info
(peers/managementStatus) so the UI no longer shows stale IP/FQDN for the
logged-out profile; ensure this call happens before loadProfiles()/error
handling so the UI state immediately reflects the logout.
In `@NetbirdKit/NetworkExtensionAdapter.swift`:
- Around line 620-633: The IPC login payload uses a pipe-delimited string in
messageString which can break if paths/URLs ever contain "|" — change the
payload to an explicit encoded format (e.g., JSON) instead: build a
dictionary/object with keys like "action":"Login", "configPath":
Preferences.configFile(), "statePath": Preferences.stateFile(), "managementURL":
ProfileConnectionCache().managementURL(for:
ProfileManager.shared.getActiveProfileName()) and serialize it (JSON or
percent-encoding) before sending so parsing on the receiver side is robust and
unambiguous.
In `@NetbirdKit/ProfileManager.swift`:
- Around line 226-253: managementURL(for:) currently performs disk writes and
cache updates as a side effect while resolving a URL, which makes a read
operation I/O-heavy and non-idempotent; refactor by extracting the pure
read-only resolution logic into a new method (e.g., resolveManagementURL(for:)
or managementURLReadOnly(for:)) that only reads configPath,
fileManager.contents, and JSON parsing and returns the resolved URL without
calling saveServerURL(_:for:) or
ProfileConnectionCache().saveManagementURL(_:for:), and move the persistence
logic into a separate method (e.g., persistManagementURL(_:for:)) that callers
can invoke when they explicitly want cache-warming; update callers that run
during UI rendering (ProfilesListView.body) to use the new read-only accessor
and call the persisting helper only from non-UI flows (e.g., login or explicit
cache-refresh paths).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bea347d-19a7-4f07-92d2-9eec76df5eba
📒 Files selected for processing (12)
NetBird.xcodeproj/project.pbxprojNetBird/Source/App/ViewModels/AddProfileViewModel.swiftNetBird/Source/App/ViewModels/MainViewModel.swiftNetBird/Source/App/Views/Components/SafariView.swiftNetBird/Source/App/Views/MainView.swiftNetBird/Source/App/Views/ServerView.swiftNetBird/Source/App/Views/iOS/ProfilesListView.swiftNetBird/Source/App/Views/iOS/iOSConnectionView.swiftNetbirdKit/GlobalConstants.swiftNetbirdKit/NetworkExtensionAdapter.swiftNetbirdKit/ProfileConnectionCache.swiftNetbirdKit/ProfileManager.swift
✅ Files skipped from review due to trivial changes (3)
- NetBird/Source/App/Views/MainView.swift
- NetbirdKit/GlobalConstants.swift
- NetBird.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (4)
- NetBird/Source/App/Views/iOS/iOSConnectionView.swift
- NetBird/Source/App/Views/ServerView.swift
- NetbirdKit/ProfileConnectionCache.swift
- NetBird/Source/App/ViewModels/AddProfileViewModel.swift
pappz
approved these changes
Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit
New Features
Improvements
Bug Fixes