Migrate to profile ids#189
Open
theodorsm wants to merge 3 commits into
Open
Conversation
Contributor
|
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 (1)
📝 WalkthroughWalkthroughRefactors profile identity from using names to using immutable IDs: Profile gains an id field; ProfileManagerWrapper and UI (ProfilesFragment, MainActivity) now use Profile objects and pass profile IDs to gomobile calls; EngineRunner updated accordingly; netbird submodule pointer bumped. ChangesProfile Identity Refactoring
Sequence DiagramsequenceDiagram
participant UI as ProfilesFragment
participant Wrapper as ProfileManagerWrapper
participant Backend as gomobile ProfileManager
UI->>Wrapper: switchProfile(profile.getID())
Wrapper->>Wrapper: stop VPN engine (if active)
Wrapper->>Backend: switchProfile(id)
Backend-->>Wrapper: success/failure
Wrapper-->>UI: callback/result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (1)
183-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways surface add-profile failures to the user.
When
e.getMessage()is null, this path only logs. Since the dialog callback still returnstrue, the dialog closes with no feedback.Suggested fix
Log.e(TAG, "Failed to add profile", e); String errorMsg = e.getMessage(); - if (errorMsg != null) { - Toast.makeText(requireContext(), - "Failed to add profile: " + e.getMessage(), - Toast.LENGTH_SHORT).show(); - } + Toast.makeText( + requireContext(), + errorMsg == null ? "Failed to add profile" : "Failed to add profile: " + errorMsg, + Toast.LENGTH_SHORT + ).show(); }🤖 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 `@app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java` around lines 183 - 188, In ProfilesFragment (the add-profile error handler where errorMsg = e.getMessage()), always show user feedback even when e.getMessage() is null: replace the conditional that only Toasts when errorMsg != null with logic that computes a safe message (use e.getMessage() if non-null, otherwise e.toString() or a generic "Failed to add profile" string) and call Toast.makeText(requireContext(), safeMessage, Toast.LENGTH_SHORT).show(); keep the rest of the dialog callback behavior unchanged so the dialog still returns true.
🤖 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 `@tool/src/main/java/io/netbird/client/tool/Profile.java`:
- Around line 10-15: Constructor Profile(String id, String name, boolean
isActive) currently allows a null id which breaks equals(), hashCode(), and
callers like ProfilesFragment.removeProfile(); validate the id parameter and
throw an IllegalArgumentException if id is null or empty, similarly to the
existing name check. Update the Profile constructor (Profile(String id, String
name, boolean isActive)) to perform the id null/empty check and document that id
is required so equals() and hashCode() can safely assume a non-null id (this
will prevent issues in ProfilesFragment.removeProfile() and calls like
profile.getID().equals(...)).
In `@tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java`:
- Around line 52-59: Do not return a fabricated Profile when
profileManager.getActiveProfile() fails; in getActiveProfile()
(ProfileManagerWrapper.getActiveProfile) remove the fallback new
Profile("default",...) and either return null or rethrow the exception so
callers can detect a real failure. Update callers such as logoutProfile() to
handle a null/exceptional result (check for null before comparing IDs and ensure
stopEngine() is invoked for the actual active profile) so failures don't
masquerade as a valid "default" profile.
---
Outside diff comments:
In `@app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java`:
- Around line 183-188: In ProfilesFragment (the add-profile error handler where
errorMsg = e.getMessage()), always show user feedback even when e.getMessage()
is null: replace the conditional that only Toasts when errorMsg != null with
logic that computes a safe message (use e.getMessage() if non-null, otherwise
e.toString() or a generic "Failed to add profile" string) and call
Toast.makeText(requireContext(), safeMessage, Toast.LENGTH_SHORT).show(); keep
the rest of the dialog callback behavior unchanged so the dialog still returns
true.
🪄 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: 5e4c18d5-613b-470a-bbf1-e9e6ae944678
📒 Files selected for processing (6)
app/src/main/java/io/netbird/client/MainActivity.javaapp/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.javanetbirdtool/src/main/java/io/netbird/client/tool/EngineRunner.javatool/src/main/java/io/netbird/client/tool/Profile.javatool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java
Comment on lines
+52
to
59
| public Profile getActiveProfile() { | ||
| try { | ||
| return profileManager.getActiveProfile(); | ||
| io.netbird.gomobile.android.Profile p = profileManager.getActiveProfile(); | ||
| return new Profile(p.getID(), p.getName(), true); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Failed to get active profile", e); | ||
| return "default"; | ||
| return new Profile("default", "default", true); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t fabricate "default" when active-profile lookup fails.
This turns a real lookup failure into a false active profile. logoutProfile() then compares against the wrong ID and can skip stopEngine() for the actual active profile.
Suggested fix
public Profile getActiveProfile() {
try {
io.netbird.gomobile.android.Profile p = profileManager.getActiveProfile();
- return new Profile(p.getID(), p.getName(), true);
+ if (p == null) {
+ throw new IllegalStateException("Active profile is unavailable");
+ }
+ return new Profile(p.getID(), p.getName(), p.getIsActive());
} catch (Exception e) {
Log.e(TAG, "Failed to get active profile", e);
- return new Profile("default", "default", true);
+ throw new IllegalStateException("Failed to get active profile", e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public Profile getActiveProfile() { | |
| try { | |
| return profileManager.getActiveProfile(); | |
| io.netbird.gomobile.android.Profile p = profileManager.getActiveProfile(); | |
| return new Profile(p.getID(), p.getName(), true); | |
| } catch (Exception e) { | |
| Log.e(TAG, "Failed to get active profile", e); | |
| return "default"; | |
| return new Profile("default", "default", true); | |
| } | |
| public Profile getActiveProfile() { | |
| try { | |
| io.netbird.gomobile.android.Profile p = profileManager.getActiveProfile(); | |
| if (p == null) { | |
| throw new IllegalStateException("Active profile is unavailable"); | |
| } | |
| return new Profile(p.getID(), p.getName(), p.getIsActive()); | |
| } catch (Exception e) { | |
| Log.e(TAG, "Failed to get active profile", e); | |
| throw new IllegalStateException("Failed to get active profile", e); | |
| } | |
| } |
🤖 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 `@tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java` around
lines 52 - 59, Do not return a fabricated Profile when
profileManager.getActiveProfile() fails; in getActiveProfile()
(ProfileManagerWrapper.getActiveProfile) remove the fallback new
Profile("default",...) and either return null or rethrow the exception so
callers can detect a real failure. Update callers such as logoutProfile() to
handle a null/exceptional result (check for null before comparing IDs and ensure
stopEngine() is invoked for the actual active profile) so failures don't
masquerade as a valid "default" profile.
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
1 participant
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.
This PR uses the new profile ids implemented in netbirdio/netbird#6326
The go bindings are updated to use the new profile with the id field. Sanitation is removed, as this is done by the go profilemanager.
Summary by CodeRabbit
Improvements
Bug Fixes