Add profile switch - #115
Conversation
VPNServiceRepository created its own service binding which caused conflicts when unbinding - it would disconnect other components relying on the service. Now NetworksFragmentViewModel uses ServiceAccessor from MainActivity, which maintains a single shared service binding across all components.
WalkthroughThis pull request introduces a new profile management feature to the NetBird VPN client. It adds a ProfileManagerWrapper to abstract profile operations, new UI components for listing and managing profiles, and refactors existing code to use dynamic profile-aware configuration paths retrieved at runtime instead of static file paths. Changes
Sequence DiagramsequenceDiagram
participant User
participant ProfilesFragment
participant ProfileManagerWrapper
participant VPNService
participant EngineRunner
User->>ProfilesFragment: Select profile to switch
ProfilesFragment->>ProfileManagerWrapper: switchProfile(profileName)
ProfileManagerWrapper->>VPNService: Broadcast ACTION_STOP_ENGINE
VPNService->>EngineRunner: Stop running engine
EngineRunner-->>VPNService: Engine stopped
ProfileManagerWrapper->>ProfileManagerWrapper: Update active profile
VPNService->>ProfileManagerWrapper: Get active config/state paths
ProfileManagerWrapper-->>VPNService: Return profile paths
VPNService->>EngineRunner: Initialize with new paths
EngineRunner->>EngineRunner: Create AndroidPlatformFiles from paths
EngineRunner->>EngineRunner: Start client with new profile
EngineRunner-->>VPNService: Engine running with new profile
VPNService-->>ProfilesFragment: Profile switched
ProfilesFragment->>User: Display success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This change introduces multiple new classes with moderate complexity (ProfileManagerWrapper with profile state management and engine lifecycle control, ProfilesFragment with dialog workflows and validation), modifies critical public APIs (EngineRunner constructor signature), refactors configuration path handling across several components, and spans diverse file types (Java logic, UI layouts, navigation, strings). While individual changes are coherent, the breadth of modifications across core infrastructure, UI, and resources requires careful validation of integration points, API compatibility, and error handling. Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
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: 5
🧹 Nitpick comments (14)
app/src/main/java/io/netbird/client/ui/server/ChangeServerFragment.java (1)
111-118: Config path via ProfileManagerWrapper is correct; consider failure handlingUsing
ProfileManagerWrapper.getActiveConfigPath()to populateCONFIG_FILE_PATH_KEYkeeps this aligned with the new profile model. The hardRuntimeExceptionon failure makes this fragment non-recoverable if the active profile/config path can’t be resolved; if that’s not expected to be truly fatal, consider logging and showing a user-facing error instead of crashing.Also applies to: 120-125
app/src/main/res/menu/activity_main_drawer.xml (1)
6-11: Drawer item wiring is fine; verify checked-state behavior across groupsThe new
group_profileandnav_profilesitem are correctly defined and reference the new icon/title. Depending on how you hook the drawer to NavController, you may end up with one checked item in each group (profiles + another destination); if that’s not desired, consider having all navigable items in a single checkable group.app/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.java (1)
71-79: Profile-based config path is correct; reassess throwing on failureInitializing
goPreferencesfromProfileManagerWrapper.getActiveConfigPath()correctly ties advanced settings to the active profile. However, converting any failure here into aRuntimeExceptionwill crash the screen; if active profile resolution can fail in real-world cases (e.g., corrupted profile state), you might instead want to show a Toast/dialog and disable advanced options rather than crashing.app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (3)
79-100: Unused dialog view inflation.
dialogViewis inflated on line 80 but never used. The dialog only uses theEditTextcreated on line 81. Either use the inflated layout or remove the unused inflation.private void showAddDialog() { - View dialogView = LayoutInflater.from(requireContext()).inflate(R.layout.dialog_simple_alert_message, null); EditText input = new EditText(requireContext()); input.setHint(R.string.profiles_dialog_add_hint);
164-167:onBackPressed()is deprecated.
Activity.onBackPressed()is deprecated in API 33+. Consider using theOnBackPressedDispatcherorNavController.popBackStack()for navigation.- // Navigate back to home - requireActivity().onBackPressed(); + // Navigate back to home + requireActivity().getOnBackPressedDispatcher().onBackPressed();Or if using Navigation Component:
NavHostFragment.findNavController(this).popBackStack();
146-150: Hardcoded error message strings.Error messages on lines 148, 170, 188, and 217 use hardcoded English text concatenated with the exception message. Consider using string resources with placeholders for consistency and i18n support.
- Toast.makeText(requireContext(), - "Failed to add profile: " + e.getMessage(), - Toast.LENGTH_SHORT).show(); + Toast.makeText(requireContext(), + getString(R.string.profiles_error_add_failed, e.getMessage()), + Toast.LENGTH_SHORT).show();app/src/main/java/io/netbird/client/ui/profile/ProfilesAdapter.java (2)
7-7: Unused import.
ImageViewis imported but not used in this file.-import android.widget.ImageView;
82-89: Hardcoded "default" profile name.The string
"default"is hardcoded here and inProfilesFragment.java(line 195). Consider extracting this to a constant for maintainability.// In a shared location (e.g., Profile.java or a constants class) public static final String DEFAULT_PROFILE_NAME = "default";app/src/main/java/io/netbird/client/MainActivity.java (2)
380-398: Exception declaration mismatch with actual behavior.Both
selectRouteanddeselectRoutedeclarethrows Exceptionbut silently return whenmBinderis null instead of throwing. This inconsistency can mislead callers who expect an exception on failure.Consider either:
- Throwing an exception when
mBinderis null to match the signature- Removing
throws Exceptionif the null case should be silently ignored@Override public void selectRoute(String route) throws Exception { if (mBinder == null) { Log.w(LOGTAG, "VPN binder is null"); - return; + throw new IllegalStateException("VPN service not bound"); } mBinder.selectRoute(route); } @Override public void deselectRoute(String route) throws Exception { if (mBinder == null) { Log.w(LOGTAG, "VPN binder is null"); - return; + throw new IllegalStateException("VPN service not bound"); } mBinder.deselectRoute(route); }
614-628: InstantiatingProfileManagerWrapperon every call is inefficient.
updateProfileMenuItemis called fromonCreate,onResume, and destination changes. Each invocation creates a newProfileManagerWrapperinstance. Consider caching the wrapper as a field.+ private io.netbird.client.tool.ProfileManagerWrapper profileManager; + private void updateProfileMenuItem(NavigationView navigationView) { try { - // Get active profile from ProfileManager instead of reading file - io.netbird.client.tool.ProfileManagerWrapper profileManager = - new io.netbird.client.tool.ProfileManagerWrapper(this); + if (profileManager == null) { + profileManager = new io.netbird.client.tool.ProfileManagerWrapper(this); + } String activeProfile = profileManager.getActiveProfile(); Menu menu = navigationView.getMenu(); MenuItem profileItem = menu.findItem(R.id.nav_profiles); if (profileItem != null && activeProfile != null) { profileItem.setTitle(activeProfile); } } catch (Exception e) { Log.e(LOGTAG, "Failed to update profile menu item", e); } }app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java (4)
28-39: Suppress unchecked cast warning in factory.The unchecked cast
(T)on line 34 will generate a compiler warning. Consider adding@SuppressWarnings("unchecked")to the method.public static ViewModelProvider.Factory getFactory(ServiceAccessor serviceAccessor) { return new ViewModelProvider.Factory() { @NonNull @Override + @SuppressWarnings("unchecked") public <T extends ViewModel> T create(@NonNull Class<T> modelClass) { if (modelClass.isAssignableFrom(NetworksFragmentViewModel.class)) { return (T) new NetworksFragmentViewModel(serviceAccessor); } throw new IllegalArgumentException("Unknown ViewModel class"); } }; }
51-63: Wrapping inRuntimeExceptionloses error context.Re-throwing a generic
RuntimeExceptionobscures the original exception type and provides no additional context. Consider logging or adding a message, or simply declaring the checked exception if the caller can handle it.private List<String> createPeerRoutesList(PeerRoutes peerRoutes) { List<String> routes = new ArrayList<>(); try { for (int i = 0; i < peerRoutes.size(); i++) { routes.add(peerRoutes.get(i)); } } catch (Exception e) { - throw new RuntimeException(e); + throw new RuntimeException("Failed to create peer routes list", e); } return routes; }
86-88: Same issue: add context to RuntimeException.} catch (Exception e) { - throw new RuntimeException(e); + throw new RuntimeException("Failed to create network domains list", e); }
149-189: Consider using a default adapter or base class forStateListener.All these empty implementations add boilerplate. If
StateListeneris under your control, adding default empty implementations to the interface would simplify this ViewModel. Alternatively, create aStateListenerAdapterbase class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/src/main/java/io/netbird/client/CustomTabURLOpener.java(1 hunks)app/src/main/java/io/netbird/client/MainActivity.java(7 hunks)app/src/main/java/io/netbird/client/MyApplication.java(0 hunks)app/src/main/java/io/netbird/client/ServiceAccessor.java(2 hunks)app/src/main/java/io/netbird/client/repository/VPNServiceBindListener.java(0 hunks)app/src/main/java/io/netbird/client/repository/VPNServiceRepository.java(0 hunks)app/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.java(4 hunks)app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java(4 hunks)app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java(2 hunks)app/src/main/java/io/netbird/client/ui/profile/ProfilesAdapter.java(1 hunks)app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java(1 hunks)app/src/main/java/io/netbird/client/ui/server/ChangeServerFragment.java(2 hunks)app/src/main/res/drawable/ic_menu_profile.xml(1 hunks)app/src/main/res/layout/fragment_profiles.xml(1 hunks)app/src/main/res/layout/list_item_profile.xml(1 hunks)app/src/main/res/menu/activity_main_drawer.xml(1 hunks)app/src/main/res/navigation/mobile_navigation.xml(1 hunks)app/src/main/res/values/strings.xml(2 hunks)netbird(1 hunks)tool/src/main/java/io/netbird/client/tool/EngineRunner.java(2 hunks)tool/src/main/java/io/netbird/client/tool/Preferences.java(0 hunks)tool/src/main/java/io/netbird/client/tool/Profile.java(1 hunks)tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java(1 hunks)tool/src/main/java/io/netbird/client/tool/VPNService.java(4 hunks)
💤 Files with no reviewable changes (4)
- app/src/main/java/io/netbird/client/repository/VPNServiceBindListener.java
- app/src/main/java/io/netbird/client/MyApplication.java
- tool/src/main/java/io/netbird/client/tool/Preferences.java
- app/src/main/java/io/netbird/client/repository/VPNServiceRepository.java
🧰 Additional context used
🧬 Code graph analysis (5)
app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java (1)
app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java (1)
NetworksFragmentViewModel(18-190)
app/src/main/java/io/netbird/client/ui/profile/ProfilesAdapter.java (1)
tool/src/main/java/io/netbird/client/tool/Profile.java (1)
Profile(3-37)
tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java (1)
tool/src/main/java/io/netbird/client/tool/VPNService.java (1)
VPNService(25-321)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (2)
tool/src/main/java/io/netbird/client/tool/Profile.java (1)
Profile(3-37)tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java (1)
ProfileManagerWrapper(16-145)
app/src/main/java/io/netbird/client/ui/server/ChangeServerFragment.java (2)
tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java (1)
ProfileManagerWrapper(16-145)app/src/main/java/io/netbird/client/ui/server/ChangeServerFragmentViewModel.java (1)
ChangeServerFragmentViewModel(19-182)
⏰ 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). (2)
- GitHub Check: build-debug
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (22)
netbird (1)
1-1: Ensure the gomobile API dependency is verified through the build process.The submodule pointer update introduces new gomobile APIs used by ProfileManagerWrapper (Android.newProfileManager(), switchProfile(), addProfile(), etc.). Verify that the new commit
49cd86440f222682f21c17029b1e8a5cc3ad6d05provides these ProfileManager APIs by confirming the Android build succeeds without API compatibility errors. The GitHub Actions workflows will catch any missing or incompatible APIs during compilation.app/src/main/java/io/netbird/client/CustomTabURLOpener.java (1)
30-34: Lambda callback refactor is behaviorally identicalThe lambda correctly preserves the previous behavior (resetting
isOpenedand invokingresultCallback.onClosed()), so this is a safe and cleaner refactor.app/src/main/res/navigation/mobile_navigation.xml (1)
27-31: Profiles destination wiring looks consistent
nav_profilescorrectly targetsio.netbird.client.ui.profile.ProfilesFragmentwith the expected label and layout; this should integrate cleanly with the drawer item and profiles UI.app/src/main/res/drawable/ic_menu_profile.xml (1)
1-9: Vector icon definition is soundThe profile vector drawable is well-formed (24dp viewport, single path, solid fill) and appropriate for use as the new drawer/menu icon.
app/src/main/java/io/netbird/client/ui/home/NetworksFragment.java (1)
39-55: ServiceAccessor injection is correct; ensure hosting activity implements itUsing
ServiceAccessorviaonAttachand passing it intoNetworksFragmentViewModel.getFactory(serviceAccessor)is consistent and keeps the ViewModel decoupled from the Fragment. Just make sure every activity that can hostNetworksFragmentnow implementsServiceAccessor, otherwise this will throw at runtime.Also applies to: 69-70
app/src/main/res/layout/fragment_profiles.xml (1)
1-40: Profiles layout is structurally solidRecyclerView and FAB are correctly constrained; padding and a11y (FAB
contentDescription) look good. This should give a clean profiles list screen out of the box.app/src/main/res/layout/list_item_profile.xml (1)
1-116: LGTM!The layout is well-structured using MaterialCardView and ConstraintLayout with proper constraint chains for button distribution. The active badge visibility toggle and button styling are appropriately configured.
app/src/main/java/io/netbird/client/ServiceAccessor.java (1)
3-19: LGTM!The interface extension is clean. Route management methods appropriately declare checked exceptions, and the listener pattern for route changes is consistent with the existing service architecture.
app/src/main/java/io/netbird/client/ui/profile/ProfilesAdapter.java (1)
69-106: Click listeners re-check conditions that are already enforced by button state.The click listeners on lines 92 and 102 re-check
!profile.isActive()and!profile.getName().equals("default")even though the buttons are already disabled. While this is defensive, it's redundant.The defensive checks are acceptable as they provide an extra safety layer if button state is somehow inconsistent.
tool/src/main/java/io/netbird/client/tool/EngineRunner.java (2)
76-96: LGTM - Runtime profile path resolution enables dynamic switching.The refactoring to retrieve config/state paths at runtime from
ProfileManagerWrapperis well-structured and enables profile switching without recreating the entire client. Error handling withRuntimeExceptionis appropriate for this critical path.
87-90: Uncaught RuntimeException on background thread.If
profileManager.getActiveConfigPath()orgetActiveStateFilePath()fails, theRuntimeExceptionis thrown inside aRunnableon a new thread (line 115). This will crash the thread but may not be surfaced to the user or properly logged beyond the thread's default uncaught exception handler.Verify that the thread's uncaught exception is handled appropriately. Consider catching the exception and calling
notifyError()instead:} catch (Exception e) { Log.e(LOGTAG, "Failed to get profile paths from ProfileManager", e); - throw new RuntimeException("Failed to get profile paths: " + e.getMessage(), e); + notifyError(new Exception("Failed to get profile paths: " + e.getMessage(), e)); + return; }tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java (2)
95-100: TOCTOU betweengetActiveProfile()and conditionalstopEngine().There's a time-of-check-to-time-of-use gap: the active profile could change between the
getActiveProfile()call on line 96 and thestopEngine()call on line 99 if another thread/process modifies the profile state.This is likely a minor concern in practice since profile operations are user-initiated and serialized through the UI. Verify this assumption holds.
31-47: LGTM - Robust profile listing with null-safety.The
listProfiles()method properly handles null checks for both the array and individual profile elements, with exception logging and graceful empty list fallback.tool/src/main/java/io/netbird/client/tool/VPNService.java (3)
57-70: LGTM - Clean integration of ProfileManagerWrapper.The
ProfileManagerWrapperis properly instantiated and passed toEngineRunner, enabling dynamic profile-based configuration. The initialization order is logical, with the profile manager created before the engine runner.
81-99: LGTM - Broadcast receiver properly registered.The
stopEngineReceiveris correctly registered withRECEIVER_NOT_EXPORTEDflag, ensuring only internal broadcasts are received. The null check onengineRunnerbefore callingstop()is good defensive practice.
136-143: LGTM - Proper cleanup with exception handling.The receiver unregistration in
onDestroy()properly catchesIllegalArgumentExceptionin case the receiver was never registered or already unregistered.app/src/main/java/io/netbird/client/MainActivity.java (2)
400-418: LGTM!The route change listener management methods correctly delegate to the binder with appropriate null checks and logging.
119-121: Profile menu updates in multiple lifecycle hooks look correct.Updating the profile menu item in
onCreate,onResume, and when navigating back to home ensures the UI stays in sync after profile switches. The null checks onbindingandbinding.navVieware appropriate.Also applies to: 171-174, 263-266
app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java (2)
23-26: LGTM!The constructor properly initializes the service accessor and registers the route change listener, and
onCleared()correctly unregisters to prevent leaks.Also applies to: 41-45
128-134: Correct use ofpostValuefor background thread updates.Using
postValueinstead ofsetValueis correct here since route changes may be triggered from background threads.app/src/main/res/values/strings.xml (2)
117-143: LGTM! Comprehensive and well-structured profile strings.The string resources cover all necessary UI scenarios including dialogs, validation errors, and success messages. Format specifiers (
%s) are used correctly for dynamic profile names, and apostrophes are properly escaped.
14-14: LGTM!Menu string added appropriately alongside other menu items.
| private void setPreSharedKey(String key, Context context) { | ||
| String configFilePath = Preferences.configFile(context); | ||
| ProfileManagerWrapper profileManager = new ProfileManagerWrapper(context); | ||
| String configFilePath; | ||
| try { | ||
| configFilePath = profileManager.getActiveConfigPath(); | ||
| } catch (Exception e) { | ||
| Toast.makeText(context, "Failed to get config path: " + e.getMessage(), Toast.LENGTH_LONG).show(); | ||
| return; | ||
| } | ||
| io.netbird.gomobile.android.Preferences preferences = new io.netbird.gomobile.android.Preferences(configFilePath); |
There was a problem hiding this comment.
Config path reuse is good; fix pre‑shared key error Toast message
Using ProfileManagerWrapper in both setPreSharedKey and hasPreSharedKey keeps these operations aligned with the active profile and adds reasonable error handling (Toast/log + early return), which is good.
There is, however, a bug in the error Toast in setPreSharedKey: it concatenates the string resource ID directly into the message, resulting in something like "2131230890: <msg>" instead of the localized text. Consider this adjustment:
- } catch (Exception e) {
- Log.e(LOGTAG, "Failed to save pre-shared key", e);
- Toast.makeText(context, R.string.advanced_presharedkey_save_error + ": " + e.getMessage(), Toast.LENGTH_LONG).show();
- }
+ } catch (Exception e) {
+ Log.e(LOGTAG, "Failed to save pre-shared key", e);
+ String message = context.getString(R.string.advanced_presharedkey_save_error) + ": " + e.getMessage();
+ Toast.makeText(context, message, Toast.LENGTH_LONG).show();
+ }Also applies to: 348-356
🤖 Prompt for AI Agents
In app/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.java around
lines 328-337 (and likewise apply to 348-356), the Toast constructs concatenate
a string resource ID instead of using the localized string; change the Toast to
call context.getString(...) (preferably a string resource that accepts the
exception/message as an argument) or build the message via
context.getString(R.string.your_string, e.getMessage()) so the localized text is
used, then pass that resulting String into Toast.makeText(...). Ensure both
occurrences are updated and remove direct concatenation of the resource ID.
| public void switchProfile(String profileName) throws Exception { | ||
| if (profileName == null || profileName.trim().isEmpty()) { | ||
| throw new IllegalArgumentException("Profile name cannot be empty"); | ||
| } | ||
|
|
||
| // Stop VPN service before switching profile | ||
| stopEngine(); | ||
|
|
||
| profileManager.switchProfile(profileName); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd tool/src/main/java/io/netbird/client/tool && cat -n ProfileManagerWrapper.java | head -80Repository: netbirdio/android-client
Length of output: 3125
🏁 Script executed:
# Search for stopEngine implementation and related broadcast code
rg -n "stopEngine|ACTION_STOP_ENGINE" --type=java -A 5 -B 2Repository: netbirdio/android-client
Length of output: 17821
🏁 Script executed:
# Search for VPNService to understand broadcast handling
rg -n "class VPNService|sendBroadcast" --type=java -A 3Repository: netbirdio/android-client
Length of output: 2277
🏁 Script executed:
# Check if profileManager.switchProfile accesses any engine state or config
rg -n "switchProfile|profileManager.switchProfile" --type=java -B 3 -A 3Repository: netbirdio/android-client
Length of output: 3502
🏁 Script executed:
# Look for any synchronization mechanisms (locks, CountdownLatch, etc.) around stopEngine
rg -n "synchronized|Lock|CountdownLatch|Semaphore|wait\(\)|notify\(\)" tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.javaRepository: netbirdio/android-client
Length of output: 50
🏁 Script executed:
# Search for EngineRunner class and stop method more thoroughly
fd -n "EngineRunner" --type f --extension javaRepository: netbirdio/android-client
Length of output: 298
🏁 Script executed:
# Check if there's documentation or comments about thread safety
rg -n "EngineRunner|engineRunner\.stop" --type=java -B 2 -A 8 | head -100Repository: netbirdio/android-client
Length of output: 9379
🏁 Script executed:
# Get the full EngineRunner.java file to see stop() implementation
wc -l tool/src/main/java/io/netbird/client/tool/EngineRunner.javaRepository: netbirdio/android-client
Length of output: 131
🏁 Script executed:
# Read EngineRunner to find stop() method
cat tool/src/main/java/io/netbird/client/tool/EngineRunner.java | head -150Repository: netbirdio/android-client
Length of output: 6018
🏁 Script executed:
# Read the rest of EngineRunner to find stop() method
tail -n +150 tool/src/main/java/io/netbird/client/tool/EngineRunner.javaRepository: netbirdio/android-client
Length of output: 2653
Race condition confirmed: profile switch executes before engine actually stops.
stopEngine() sends an asynchronous broadcast (line 138), but switchProfile() is called synchronously on line 73 without waiting for the engine to stop. The BroadcastReceiver.onReceive() handler in VPNService (which calls engineRunner.stop()) executes later on the main thread, potentially after the profile has already been switched. Although EngineRunner.stop() itself is synchronized, this doesn't prevent the race since the broadcast delivery is inherently asynchronous.
To fix this, consider using a direct synchronous call to stop the engine (e.g., via service binding) instead of broadcasting, or add a synchronization mechanism (e.g., CountDownLatch or callback) to confirm engine stop before proceeding with the profile switch.
🤖 Prompt for AI Agents
tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java around
lines 65-74: the current switchProfile calls stopEngine() which only sends an
asynchronous broadcast, causing a race where
profileManager.switchProfile(profileName) executes before the engine actually
stops; change stopEngine to use a synchronous stop path (preferably bind to the
VPN service and call a direct stop method on the service/EngineRunner) or
implement a synchronization handshake (e.g., send the stop intent but block with
a CountDownLatch or wait on a callback that the VPNService/EngineRunner signals
when fully stopped) and include a reasonable timeout and proper exception
handling so switchProfile only calls profileManager.switchProfile(profileName)
after the engine stop confirmation.
| private void stopEngine() { | ||
| try { | ||
| Intent stopIntent = new Intent(VPNService.ACTION_STOP_ENGINE); | ||
| stopIntent.setPackage(context.getPackageName()); | ||
| context.sendBroadcast(stopIntent); | ||
| Log.d(TAG, "Sent stop engine broadcast for profile operation"); | ||
| } catch (Exception e) { | ||
| Log.w(TAG, "Failed to send stop engine broadcast: " + e.getMessage()); | ||
| // Don't throw exception - profile operations should continue even if stop fails | ||
| } | ||
| } |
There was a problem hiding this comment.
We could use a similar approach that was used with EngineRestarter, wrapping EngineRunner to a class to both stop the engine without a broadcast receiver and to track for sure that it was stopped using the ServiceStateListener interface and pass it as a parameter to ProfileManagerWrapper.
If using this approach, the ProfileManagerWrapper instance could be created in VPNService (either when bound or created), and provided to the ProfilesFragment via the ServiceAccessor interface.
There was a problem hiding this comment.
I chose this approach because a synchronous call during profile switching is not expected. It’s fine if it finishes once in the background, but I prefer your solution. If we want to use this method differently, then it would be better to expose it through a Service Accessor.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (1)
79-114: Tighten add‑dialog implementation and align sanitization semantics/i18nThe validation flow is sound (no empty names, no “all invalid characters” names, dialog doesn’t auto‑dismiss), but a few details can be improved:
dialogView(Line 80) is inflated and then never used. Either bindinputfrom that layout or drop the inflation to avoid unnecessary work and confusion.- The error message for invalid characters is hard‑coded (Lines 105‑107). This should be moved to a string resource (you already appear to have
profiles_error_invalid_name) so it’s localizable and consistent with the rest of the UI.sanitizeProfileName()returns the sanitized value, but you only use it to check for emptiness and still pass the originalprofileNameintoaddProfile()(Line 111). That means:
- The success/error toasts use the unsanitized string.
- The stored/listed name will be whatever the Go client sanitizes to, which can differ from what the user just saw in the toast.
Consider either (a) passing
sanitizedNametoaddProfile()and using it in success messages, or (b) renamingsanitizeProfileName()/ updating its Javadoc to make it clear it is used only for validation, not for the actual persisted name, so future maintainers don’t assume the UI and stored names always match.Also applies to: 116-130
🧹 Nitpick comments (2)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (1)
162-182: Harden user‑visible error handling and avoid leaking raw exception messagesAcross
addProfile,switchProfile,logoutProfile, andremoveProfile, failures are shown with toasts like"Failed to X: " + e.getMessage():
e.getMessage()can be null, resulting in slightly odd “...: null” messages.- Raw exception messages can be technical, inconsistent, or expose internal details that aren’t useful to end users.
- These strings are not localized, unlike the success paths and specific “already exists” / cannot‑remove messages.
Consider:
- Using localized, human‑friendly fallback messages for the generic failure cases (e.g.,
R.string.profiles_error_add_generic,R.string.profiles_error_switch_generic, etc.).- Optionally logging the full exception (as you already do with
Log.e) but keeping the toast generic, withoute.getMessage().Functionally things work, but this would tighten UX and i18n and avoid accidentally surfacing internal error details.
Also applies to: 197-201, 216-219, 245-248
tool/src/main/java/io/netbird/client/tool/Profile.java (1)
5-41: Immutable value object with null‑safety looks goodThis class is in good shape:
- Immutable fields with a constructor guard against
nullnames.equals/hashCodeimplemented viaObjectsand consistent (both key offname).toStringcleanly reflects the active state.If you want to align fully with your use of
Objects, you could optionally replace the manual null check in the constructor withObjects.requireNonNull(name, "Profile name cannot be null");, but that’s purely stylistic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java(1 hunks)netbird(1 hunks)tool/src/main/java/io/netbird/client/tool/Profile.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- netbird
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (2)
tool/src/main/java/io/netbird/client/tool/Profile.java (1)
Profile(5-42)tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java (1)
ProfileManagerWrapper(16-145)
⏰ 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: Analyze (java-kotlin)
* Add dialog_simple_edit_text layout for dialogs It has a title, a description for an input field, an input field, a cancel and a OK button * Use dialog_simple_edit_text in Add Profile * Adjust focus highlight corners to 2dp * Add createDialog method to ProfilesFragment Centralizes layout inflation and dialog creation. Receives a title, a message and an optional hint to be displayed in the dialog's EditText view; if the hint is sent as null, the dialog won't display its EditText. DialogCallback interface defines an onConfirm method that receives a nullable input text to be used as the confirmation button's action; its return value indicates whether the dialog should be dismissed or not after its execution. * Use createDialog in show dialog methods showAddDialog, showSwitchDialog, showLogoutDialog and showRemoveDialog were changed to make use of createDialog. * Replace deprecated call to navigate back to Home
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/res/layout/dialog_simple_edit_text.xml (1)
32-50: Improve EditText configuration for autofill and accessibility.Two minor improvements for the EditText:
Line 44: Empty
autofillHints=""may trigger lint warnings. Useandroid:importantForAutofill="no"instead if autofill is not needed, or provide appropriate hints likenamewhen applicable.Accessibility: The EditText lacks proper labeling for screen readers. Consider adding
android:labelForon the label TextView or usingandroid:contentDescription.🔎 Suggested improvements
<EditText android:id="@+id/edit_text_dialog" android:layout_width="0dp" android:layout_height="48dp" android:layout_marginTop="8dp" android:background="@drawable/edit_text_white" android:padding="12dp" android:textColor="@color/nb_txt" android:textColorHint="@color/nb_txt_light" android:focusable="true" android:focusableInTouchMode="true" android:foreground="@drawable/focus_highlight" - android:autofillHints="" + android:importantForAutofill="no" android:inputType="text" app:layout_constraintEnd_toEndOf="parent" app:layout_constraintStart_toStartOf="parent" app:layout_constraintTop_toBottomOf="@id/text_label_dialog" tools:text="input input input input input input input input input input input" />For accessibility, update the label TextView:
<TextView android:id="@+id/text_label_dialog" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_marginTop="16dp" + android:labelFor="@id/edit_text_dialog" app:layout_constraintStart_toStartOf="parent" app:layout_constraintTop_toBottomOf="@id/text_title_dialog" android:text="" tools:text="Label label label label" />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.javaapp/src/main/res/drawable/focus_highlight.xmlapp/src/main/res/layout/dialog_simple_edit_text.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (2)
tool/src/main/java/io/netbird/client/tool/Profile.java (1)
Profile(5-42)tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java (1)
ProfileManagerWrapper(16-145)
⏰ 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: Analyze (java-kotlin)
🔇 Additional comments (8)
app/src/main/res/drawable/focus_highlight.xml (1)
11-11: LGTM! Corner radius update is consistent.The corner radius reduction from 8dp to 2dp is applied consistently to both focused and unfocused states, creating a sharper visual appearance for the focus highlight.
Also applies to: 19-19
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (5)
31-73: LGTM! Fragment initialization is well-structured.The ProfileManagerWrapper is properly initialized, RecyclerView is set up with the adapter and action listeners, and the FAB is correctly wired. The initial profile load ensures the UI is populated on creation.
75-80: LGTM! Profile loading logic is correct.The method properly clears the existing list, loads fresh data from the profile manager, and notifies the adapter to refresh the UI.
82-119: Excellent solution for validation-aware dialog dismissal.The DialogCallback interface with boolean return value elegantly solves the previous review concern about dialogs dismissing before validation completes. Returning
falsekeeps the dialog open for corrections, whiletruedismisses it on success.
146-160: LGTM! Sanitization correctly matches Go client behavior.The character-by-character filtering matches the Go client's sanitization rules (documented in the comment). This pre-validation prevents users from submitting names that would become empty after sanitization.
162-196: LGTM! Confirmation dialog methods follow a consistent pattern.All three dialog methods (
showSwitchDialog,showLogoutDialog,showRemoveDialog) consistently use thecreateDialoghelper and properly delegate to their respective operation methods.app/src/main/res/layout/dialog_simple_edit_text.xml (2)
1-31: LGTM! Root layout and TextViews are properly configured.The ConstraintLayout has appropriate size constraints and padding. Both title and label TextViews are correctly positioned and styled.
52-80: LGTM! Dialog buttons are correctly configured.Both cancel and OK buttons use the appropriate Material TextButton style, are properly constrained, and have correct focus behavior for dialog interactions.
| private void showAddDialog() { | ||
| createDialog( | ||
| getString(R.string.profiles_dialog_add_title), | ||
| getString(R.string.profiles_dialog_add_message), | ||
| getString(R.string.profiles_dialog_add_hint), | ||
| profileName -> { | ||
| if (profileName == null || profileName.isEmpty()) { | ||
| Toast.makeText(requireContext(), R.string.profiles_error_empty_name, Toast.LENGTH_SHORT).show(); | ||
| return false; | ||
| } | ||
|
|
||
| // Validate profile name based on go client sanitization rules | ||
| String sanitizedName = sanitizeProfileName(profileName); | ||
| if (sanitizedName.isEmpty()) { | ||
| Toast.makeText(requireContext(), | ||
| "Profile name must contain at least one letter, digit, underscore or hyphen", | ||
| Toast.LENGTH_LONG).show(); | ||
| return false; | ||
| } | ||
|
|
||
| addProfile(profileName); | ||
| return true; | ||
| }).show(); | ||
| } |
There was a problem hiding this comment.
Use string resource for the profile name validation error.
The validation logic correctly prevents names that would sanitize to empty. However, the error message on lines 136-137 is hardcoded. For consistency with other messages and to support localization, extract this to a string resource.
🔎 Suggested improvement
Add to strings.xml:
<string name="profiles_error_invalid_characters">Profile name must contain at least one letter, digit, underscore or hyphen</string>Then update the code:
if (sanitizedName.isEmpty()) {
Toast.makeText(requireContext(),
- "Profile name must contain at least one letter, digit, underscore or hyphen",
+ R.string.profiles_error_invalid_characters,
Toast.LENGTH_LONG).show();
return false;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java around
lines 121 to 144, the profile validation error message is hardcoded; add a new
string resource (e.g., profiles_error_invalid_characters with value "Profile
name must contain at least one letter, digit, underscore or hyphen") to
res/values/strings.xml and replace the hardcoded Toast text with
getString(R.string.profiles_error_invalid_characters) so the message is
localized and consistent with other strings.
| private void addProfile(String profileName) { | ||
| try { | ||
| profileManager.addProfile(profileName); | ||
| Toast.makeText(requireContext(), | ||
| getString(R.string.profiles_success_added, profileName), | ||
| Toast.LENGTH_SHORT).show(); | ||
| loadProfiles(); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Failed to add profile", e); | ||
| String errorMsg = e.getMessage(); | ||
| if (errorMsg != null && errorMsg.contains("already exists")) { | ||
| Toast.makeText(requireContext(), | ||
| getString(R.string.profiles_error_already_exists, profileName), | ||
| Toast.LENGTH_SHORT).show(); | ||
| } else { | ||
| Toast.makeText(requireContext(), | ||
| "Failed to add profile: " + e.getMessage(), | ||
| Toast.LENGTH_SHORT).show(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void switchProfile(Profile profile) { | ||
| try { | ||
| // Switch profile (VPN service will be stopped automatically in ProfileManagerWrapper) | ||
| profileManager.switchProfile(profile.getName()); | ||
|
|
||
| Toast.makeText(requireContext(), | ||
| getString(R.string.profiles_success_switched, profile.getName()), | ||
| Toast.LENGTH_SHORT).show(); | ||
|
|
||
| loadProfiles(); | ||
|
|
||
| // Navigate back to home | ||
| requireActivity().getOnBackPressedDispatcher().onBackPressed(); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Failed to switch profile", e); | ||
| Toast.makeText(requireContext(), | ||
| "Failed to switch profile: " + e.getMessage(), | ||
| Toast.LENGTH_SHORT).show(); | ||
| } | ||
| } | ||
|
|
||
| private void logoutProfile(Profile profile) { | ||
| try { | ||
| // Logout from profile (VPN service will be stopped automatically if it's the active profile) | ||
| profileManager.logoutProfile(profile.getName()); | ||
|
|
||
| Toast.makeText(requireContext(), | ||
| getString(R.string.profiles_success_logged_out, profile.getName()), | ||
| Toast.LENGTH_SHORT).show(); | ||
|
|
||
| loadProfiles(); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Failed to logout from profile", e); | ||
| Toast.makeText(requireContext(), | ||
| "Failed to logout: " + e.getMessage(), | ||
| Toast.LENGTH_SHORT).show(); | ||
| } | ||
| } | ||
|
|
||
| private void removeProfile(Profile profile) { | ||
| try { | ||
| if (profile.getName().equals("default")) { | ||
| Toast.makeText(requireContext(), | ||
| R.string.profiles_error_cannot_remove_default, | ||
| Toast.LENGTH_SHORT).show(); | ||
| return; | ||
| } | ||
|
|
||
| if (profile.isActive()) { | ||
| Toast.makeText(requireContext(), | ||
| R.string.profiles_error_cannot_remove_active, | ||
| Toast.LENGTH_SHORT).show(); | ||
| return; | ||
| } | ||
|
|
||
| profileManager.removeProfile(profile.getName()); | ||
| Toast.makeText(requireContext(), | ||
| getString(R.string.profiles_success_removed, profile.getName()), | ||
| Toast.LENGTH_SHORT).show(); | ||
| loadProfiles(); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Failed to remove profile", e); | ||
| Toast.makeText(requireContext(), | ||
| "Failed to remove profile: " + e.getMessage(), | ||
| Toast.LENGTH_SHORT).show(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use string resources for generic error messages.
The profile operation methods (addProfile, switchProfile, logoutProfile, removeProfile) handle success cases well with localized strings. However, the generic error messages on lines 214-215, 236-237, 253-254, and 282-283 are hardcoded. For consistency and to support localization, extract these to string resources.
🔎 Suggested improvements
Add to strings.xml:
<string name="profiles_error_add_failed">Failed to add profile: %1$s</string>
<string name="profiles_error_switch_failed">Failed to switch profile: %1$s</string>
<string name="profiles_error_logout_failed">Failed to logout: %1$s</string>
<string name="profiles_error_remove_failed">Failed to remove profile: %1$s</string>Then update each method's catch block:
// In addProfile
- Toast.makeText(requireContext(),
- "Failed to add profile: " + e.getMessage(),
- Toast.LENGTH_SHORT).show();
+ Toast.makeText(requireContext(),
+ getString(R.string.profiles_error_add_failed, e.getMessage()),
+ Toast.LENGTH_SHORT).show();Apply similar changes to switchProfile, logoutProfile, and removeProfile.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java around
lines 198-286, the catch blocks currently use hardcoded English error messages
(lines ~214-215, ~236-237, ~253-254, ~282-283); replace them with localized
string resources and format them with the exception message. Add four entries to
strings.xml: profiles_error_add_failed, profiles_error_switch_failed,
profiles_error_logout_failed, and profiles_error_remove_failed each accepting a
single %1$s parameter, then change each Toast.makeText(...) in the respective
catch blocks to use getString(R.string.<name>, e.getMessage()) instead of the
hardcoded text.
2 participants
Summary by CodeRabbit
New Features
UI Improvements
Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.