Add profile switch #115
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.
WalkthroughAdds profile management (Profile, ProfileManagerWrapper, UI, resources), replaces static Preferences path usage with ProfileManagerWrapper, removes VPNServiceRepository in favor of ServiceAccessor, updates EngineRunner and VPNService to use ProfileManagerWrapper, and exposes route APIs through MainActivity. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ProfilesFragment / MainActivity
participant PM as ProfileManagerWrapper
participant App as VPNService
participant Engine as EngineRunner
UI->>PM: switchProfile(name)
PM->>App: broadcast ACTION_STOP_ENGINE
App->>App: receive ACTION_STOP_ENGINE → stop running engine
App-->>PM: (implicit) engine stopped
PM->>PM: change active profile (gomobile ProfileManager)
PM-->>UI: return success
UI->>Engine: start/run (on next engine start)
Engine->>PM: getActiveConfigPath() / getActiveStateFilePath()
Engine->>Engine: create AndroidPlatformFiles with paths and start client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 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.
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java
Outdated
Show resolved
Hide resolved
| 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)
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.