Refactor own profile management in the Go core#147
Open
pappz wants to merge 9 commits into
Open
Conversation
Replace the parallel Swift profile manager with a thin wrapper over the core's ID-based profilemanager.ServiceManager (via the new NetBirdSDK iOS binding), eliminating the duplicate implementation. Profiles are now ID-keyed (on-disk filename = id, display name lives in the config); the default profile stays "default" at the container root and never becomes a hex id, matching the Go/desktop semantics. - ProfileManager: thin wrapper over NetBirdSDKProfileManager on iOS; tvOS keeps its single-default, container-root behavior (no Go profile manager) - ProfileLayoutMigration: one-time, idempotent, file-coordinated migration from the legacy profiles/<name>/ directory layout to the Go layout; preserves auth tokens (copies unparseable configs verbatim), keeps the active selection and logged-out profiles, and only marks itself done on full success so a failure retries on the next launch - ProfileConnectionCache and all callers re-keyed by profile id - Settings/server changes already target the active profile through the profile manager (Preferences.configFile -> activeConfigPath) - Tests: NetBirdTests/ProfileMigrationTests covers the migration, reads it back through the Go lib, and round-trips a setting via NetBirdSDKPreferences (the test target now links NetBirdSDK) - Bump netbird-core submodule to include the iOS profile manager binding
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR replaces the native Swift name-based multi-profile filesystem implementation with a Go SDK ( ChangesProfile ID Migration to Go SDK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Run the iOS test suite on PRs again to check whether the previously reported test crash still reproduces.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NetBird.xcodeproj/project.pbxproj`:
- Line 1262: The FRAMEWORK_SEARCH_PATHS setting is configured with an overly
permissive pattern "$(PROJECT_DIR)/**" in two locations (near lines 1262 and
1796), which allows frameworks to be resolved from any subdirectory in the
project and can cause unexpected framework resolution. Replace both instances of
FRAMEWORK_SEARCH_PATHS = ("$(inherited)", "$(PROJECT_DIR)/**") with either a
specific explicit directory path that is actually needed, or remove the overly
broad path entirely and rely only on inherited settings and direct file
references to ensure only expected frameworks are resolved.
🪄 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: fede5819-c5c6-42d7-b1c3-3fdbc2098642
📒 Files selected for processing (11)
NetBird.xcodeproj/project.pbxprojNetBird/Source/App/ViewModels/AddProfileViewModel.swiftNetBird/Source/App/ViewModels/MainViewModel.swiftNetBird/Source/App/Views/ServerView.swiftNetBird/Source/App/Views/iOS/ProfilesListView.swiftNetBirdTests/ProfileMigrationTests.swiftNetbirdKit/NetworkExtensionAdapter.swiftNetbirdKit/ProfileConnectionCache.swiftNetbirdKit/ProfileLayoutMigration.swiftNetbirdKit/ProfileManager.swiftnetbird-core
The non-null String path getters (getActiveConfigPath/getActiveStateFilePath/getConfigPath/getStateFilePath) are generated by gomobile as value-returning calls with an explicit NSErrorPointer parameter, not throwing methods. Wrap them via a goPath helper in ProfileManager (and matching helpers in the tests), fixing the iOS app build failure (missing argument for parameter).
The NetBirdTests target was iOS 14.0 while the NetBird app module requires iOS 15.0, so `@testable import NetBird` failed to build ("Compiling for iOS 14.0, but module 'NetBird' has a minimum deployment target of iOS 15.0"). Match the test target to the app so the test bundle compiles and the suite can run.
The dummy GoogleService-Info.plist used a GOOGLE_APP_ID of "dummy",
which fails Firebase's app-ID format validation. At launch the host app
calls FirebaseApp.configure(options:), and +[FIRApp addAppToAppDictionary:]
raised an uncaught NSException, aborting the host before the test runner
could establish a connection ("Test crashed with signal abrt before
establishing connection"). This blocked all hosted tests, not just the
new profile-migration ones.
Give the dummy app ID a well-formed value so configure() succeeds.
The host app calls FirebaseApp.configure() at launch. When launched purely to host unit tests, this aborted the process before the test runner could connect because FirebaseApp.configure() raises an uncaught ObjC exception on an invalid app ID (the CI dummy plist). Gate both the iOS and tvOS configure paths on the XCTestConfigurationFilePath environment variable so Firebase is never initialized in the test host, and fold the duplicated configure logic into a single helper.
The legacy netbird.cfg is written by the Go SDK, which serializes
Config.ManagementURL (a url.URL) as a nested {Scheme,Host,Path} object,
not a plain string. The settings round-trip test drives NetBirdSDKPreferences,
which fully unmarshals Config and rejected the string fixture with
"cannot unmarshal string into Go struct field Config.ManagementURL".
Normalize the fixture to the object form the SDK actually produces.
The test target used a recursive "$(PROJECT_DIR)/**" framework search path, scanning every subdirectory (DerivedData, netbird-core, package checkouts) and risking unexpected framework resolution. NetBirdSDK.xcframework lives at the project root, so point the search path at "$(PROJECT_DIR)" directly.
|
/testflight |
|
TestFlight builds uploaded |
theodorsm
reviewed
Jun 23, 2026
There was a problem hiding this comment.
Running a build with testflght I found two problems:
- Profile names are still restrictive (no special chars etc). We need to update
isNameValidinAddProfileSheet, preferably using a binding forsanitizeDisplayNameif possible. - The profile listing/select/active are showing IDs as the profile names after adding a new profile with this PR:
I have tried to triage the reason for 2. but I can't seem to understand why it happens, the code looks fine to me. It is worth nothing that the name field in the json config is empty after creating a new profile.
|
Another thing we have to keep in mind with the go bindings vs. native swift implementation is that it will be harder to use native libraries such as the keychain (we don't use it now anyways) after this refactor. The profiles contains the wg and ssh private keys that are sensitive, we should consider extra protection beyond the regular app sandbox (keychain or at least exclude from backup). |
Contributor
Author
Good point! I have to consider this. |
Contributor
Author
Please check this change: netbirdio/netbird@39d189b |
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.
Replace the parallel Swift profile manager with a thin wrapper over the core's ID-based profilemanager.ServiceManager (via the new NetBirdSDK iOS binding), eliminating the duplicate implementation. Profiles are now ID-keyed (on-disk filename = id, display name lives in the config); the default profile stays "default" at the container root and never becomes a hex id, matching the Go/desktop semantics.
Description
Summary by CodeRabbit
Refactor
Tests
Chores