Repository navigation
[client] Migrate profile identity from display name to ID and allow renaming of profiles - #6367
Merged
Merged
Conversation
Contributor
Author
|
This is the based on #6326, but a new PR due to rebase diff issues with main. |
Contributor
|
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:
📝 WalkthroughWalkthroughProfile identity is migrated from human-readable names to stable, filename-safe IDs across profilemanager internals, daemon protobufs/server handlers, CLI, Android, and UI clients; Add/Remove/Resolve, persisted active state, path resolution, and tests are updated to use IDs while preserving legacy compatibility. ChangesProfile Identity Migration to ID-Based System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@client/android/profile_manager.go`:
- Around line 205-230: getProfileConfigPath and GetStateFilePath currently
concatenate the provided id directly into filepath.Join which allows absolute
paths or path traversal (../) to escape profilesDir; validate and sanitize
non-default ids before joining (e.g., reject empty/DefaultProfileName as already
handled, then ensure id is a safe filename/stem with no path separators or
absolute path prefix and does not contain ".."), and reuse a single validated
helper (or add a validateProfileID(id string) used by getProfileConfigPath and
GetStateFilePath) so both GetConfigPath (via getProfileConfigPath) and
GetStateFilePath produce only paths inside pm.configDir/profilesSubdir for
non-default profiles.
In `@client/cmd/profile.go`:
- Around line 147-152: The code reads resp.Id before checking err after calling
daemonClient.AddProfile, which can cause a nil dereference; update the
AddProfile handling (the call to daemonClient.AddProfile in profile.go) to check
if err != nil immediately and return or handle the error before accessing resp
or converting resp.Id to profilemanager.ID, ensuring any logic that uses id
(profilemanager.ID(resp.Id)) only runs after err == nil.
In `@client/internal/profilemanager/config.go`:
- Around line 106-108: The Config.Name invariant (sanitized via
sanitizeDisplayName) must be enforced when loading/updating configs, so modify
Config.apply() to validate and normalize Name (call sanitizeDisplayName on
c.Name and either truncate/replace or return an error if invalid) before any
code reuses it; this ensures configs coming from ConfigFromJSON, ReadConfig, or
UpdateOrCreateConfig are normalized even if hand-edited, and keeps AddProfile's
existing sanitization as a creation-time guarantee.
In `@client/server/server.go`:
- Around line 1032-1041: The logoutFromProfile function calls
profilemanager.GetConfig(profile.Path) without applying the same default-path
normalization used by SetConfig/GetConfig, causing logout to fail for the
default profile; update logoutFromProfile (the function named logoutFromProfile)
to normalize profile.Path to profilemanager.DefaultConfigPath when profile.Path
== "" (or otherwise mirror the resolved.Path fallback behavior used by
GetConfig/SetConfig) before calling profilemanager.GetConfig so the
default-profile path lookup uses the same fallback.
In `@client/ui/profile.go`:
- Around line 468-470: The menu refresh comparison in updateMenu() currently
sorts only by Name which allows equal-name profiles to reorder and trigger
unnecessary rebuilds; modify the sort of both profiles and state inside
updateMenu() to use a stable total order by comparing Name first and, if equal,
comparing ID (e.g., if profiles[i].Name != profiles[j].Name { ... } else {
compare profiles[i].ID and profiles[j].ID }), so slices.Equal sees a
deterministic ordering and avoids spurious menu refreshes.
- Around line 302-305: Profiles can share the same Profile.Name so update the UI
to disambiguate entries by appending a short form of Profile.ID when duplicates
exist: add a helper like formatProfileLabel(profile, allProfiles) that checks
the slice of profiles for duplicate Names and returns profile.Name when unique
or `${profile.Name} (${shortID})` where shortID is the first ~8 chars of
Profile.ID when not unique; use this helper in both the profiles window renderer
and the tray/menu label builder so all places that currently render profile.Name
instead use formatProfileLabel(Profile, profiles).
🪄 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: 42277719-9455-4983-a599-b9fa741978c3
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/profile_manager.goclient/cmd/login.goclient/cmd/login_test.goclient/cmd/profile.goclient/cmd/up.goclient/cmd/up_daemon_test.goclient/internal/debug/debug_test.goclient/internal/profilemanager/config.goclient/internal/profilemanager/id.goclient/internal/profilemanager/profilemanager.goclient/internal/profilemanager/profilemanager_test.goclient/internal/profilemanager/service.goclient/internal/profilemanager/service_test.goclient/internal/profilemanager/state.goclient/proto/daemon.protoclient/server/login_overrides_test.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/client_ui.goclient/ui/profile.go
theodorsm
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/cmd/profile.go (1)
314-327:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
renamein the ambiguity hint, or make the hint command-agnostic.
wrapAmbiguityErroris now shared byrenameProfileFunc, but the message still saysselect|remove <id-prefix>. Ambiguous rename failures will point users at the wrong command.💡 Suggested fix
- return errors.New(msg + "\nRun `netbird profile list --show-id` to see IDs, then select by ID prefix:\n netbird profile select|remove <id-prefix>") + return errors.New(msg + "\nRun `netbird profile list --show-id` to see IDs, then retry with a longer ID prefix.")🤖 Prompt for 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. In `@client/cmd/profile.go` around lines 314 - 327, wrapAmbiguityError currently suggests only "select|remove" when an ambiguous ID is detected, which is misleading for callers like renameProfileFunc; update the ambiguity hint in wrapAmbiguityError to include "rename" (e.g., "... Run `netbird profile list --show-id` to see IDs, then select by ID prefix:\n netbird profile select|remove|rename <id-prefix>") or replace the command list with a command-agnostic instruction (e.g., "use the appropriate profile command such as select/remove/rename with an <id-prefix>"); modify the message in function wrapAmbiguityError so all consumers (including renameProfileFunc) get correct guidance.client/server/server.go (2)
771-774:⚠️ Potential issue | 🟡 MinorReturn
codes.InvalidArgumentfor missing username when a non-default profile handle is provided
switchProfileIfNeededis used byLogin,Up, andSwitchProfile, but this branch returnsfmt.Errorf(...). In gRPC-Go, non-statuserrors are surfaced to clients ascodes.Unknown(andstatus.FromError(err)hasok == false), so callers can’t reliably distinguish this as a validation error.💡 Suggested fix
if handle != profilemanager.DefaultProfileName && (userName == nil || *userName == "") { - log.Errorf("profile name is set to %s, but username is not provided", handle) - return nil, fmt.Errorf("profile name is set to %s, but username is not provided", handle) + return nil, gstatus.Errorf(codes.InvalidArgument, "username must be provided when profile handle %q is specified", handle) }🤖 Prompt for 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. In `@client/server/server.go` around lines 771 - 774, The branch in switchProfileIfNeeded that checks "if handle != profilemanager.DefaultProfileName && (userName == nil || *userName == "")" currently returns a plain fmt.Errorf, causing gRPC to surface it as codes.Unknown; change this to return a gRPC status error using status.Errorf(codes.InvalidArgument, ...) (or status.Error(codes.InvalidArgument, ...)) with the same descriptive message so callers receive a proper InvalidArgument code for missing username when a non-default profile handle is provided.
311-319:⚠️ Potential issue | 🟠 MajorSetConfig/GetConfig: empty
ProfileNameno longer falls back to active profile
profilemanager.ResolveProfile(handle, username)returns an error whenhandle == ""(“profile handle is empty”), andServer.SetConfig/Server.GetConfigalways callresolveProfileHandle(msg.ProfileName, msg.Username)without a fallback. This will break backward compatibility for any clients that previously sent an emptyProfileNameto mean “current/active profile.”🤖 Prompt for 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. In `@client/server/server.go` around lines 311 - 319, When msg.ProfileName is empty we must preserve the old behavior of using the active/current profile instead of calling profilemanager.ResolveProfile with an empty handle; update Server.SetConfig and Server.GetConfig to check if msg.ProfileName == "" and, in that case, obtain the active profile handle (via the server's profile manager or an existing helper that returns the active profile handle) and pass that into resolveProfileHandle (or call profilemanager.ResolveProfile with that active handle) so resolveProfileHandle is only invoked with a non-empty handle; retain the existing profPath defaulting to profilemanager.DefaultConfigPath if resolution yields an empty Path.
🤖 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.
Outside diff comments:
In `@client/cmd/profile.go`:
- Around line 314-327: wrapAmbiguityError currently suggests only
"select|remove" when an ambiguous ID is detected, which is misleading for
callers like renameProfileFunc; update the ambiguity hint in wrapAmbiguityError
to include "rename" (e.g., "... Run `netbird profile list --show-id` to see IDs,
then select by ID prefix:\n netbird profile select|remove|rename <id-prefix>")
or replace the command list with a command-agnostic instruction (e.g., "use the
appropriate profile command such as select/remove/rename with an <id-prefix>");
modify the message in function wrapAmbiguityError so all consumers (including
renameProfileFunc) get correct guidance.
In `@client/server/server.go`:
- Around line 771-774: The branch in switchProfileIfNeeded that checks "if
handle != profilemanager.DefaultProfileName && (userName == nil || *userName ==
"")" currently returns a plain fmt.Errorf, causing gRPC to surface it as
codes.Unknown; change this to return a gRPC status error using
status.Errorf(codes.InvalidArgument, ...) (or
status.Error(codes.InvalidArgument, ...)) with the same descriptive message so
callers receive a proper InvalidArgument code for missing username when a
non-default profile handle is provided.
- Around line 311-319: When msg.ProfileName is empty we must preserve the old
behavior of using the active/current profile instead of calling
profilemanager.ResolveProfile with an empty handle; update Server.SetConfig and
Server.GetConfig to check if msg.ProfileName == "" and, in that case, obtain the
active profile handle (via the server's profile manager or an existing helper
that returns the active profile handle) and pass that into resolveProfileHandle
(or call profilemanager.ResolveProfile with that active handle) so
resolveProfileHandle is only invoked with a non-empty handle; retain the
existing profPath defaulting to profilemanager.DefaultConfigPath if resolution
yields an empty Path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a847fa11-3779-461e-9588-0fe841ef8cff
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
client/cmd/profile.goclient/cmd/root.goclient/internal/profilemanager/service.goclient/proto/daemon.protoclient/server/server.go
✅ Files skipped from review due to trivial changes (1)
- client/cmd/root.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/profilemanager/service.go
riccardomanfrin
left a comment
riccardomanfrin
left a comment
Contributor
There was a problem hiding this comment.
I made some comments around and there is the ui binary to remove
* Remove ui binary * Fix getProfileConfigPath not validating id
|
riccardomanfrin
approved these changes
Jun 17, 2026
riccardomanfrin
left a comment
riccardomanfrin
left a comment
Contributor
There was a problem hiding this comment.
Did few tests also removing default.json and renaming it ant it works fine
This was referenced Jun 19, 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
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.
Describe your changes
This PR decouples a profile's identity from its display name. Previously the profile name was used for its on-disk config filename, forcing unique, character-restricted names. Now each profile has a stable ID (the filename) while the name is free-form and stored inside the profile JSON. New rpc and cli cmd are added for renaming of profiles.
Changes
Profile,GetActiveProfileResponse, and theAdd/Switch/Removeresponses gain an additive id field (name retained for older clients). NewRenameProfilerpc.profileNamesin profile requests acts as handles instead of only profile names (backward compat).profile list --show-id adds an ID column: select/remove accept name, ID, or unique prefix, with disambiguation help on ambiguity.profile rename` allows users to rename profiles, even the default one.Compatibility: fully backward compatible with legacy profiles, existing state files, and older clients all keep working.
Tests: new service_test.go plus updates across profilemanager/server/cmd tests.
OS test: have done manual testing on linux, macos, windows and android as they all use the go implementation of profiles (ios uses a native swift implementation).
Related PR for android-client: netbirdio/android-client#189
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#781
Summary by CodeRabbit
Summary
New Features
profile list --show-id.netbird profile renamecommand.Bug Fixes
Tests