Align the Android client with the iOS and desktop clients - #179
Open
pappz wants to merge 146 commits into
Open
Conversation
Replace the side drawer + collapsible bottom-sheet pattern with a bottom navigation bar (4 tabs: Home, Peers, Resources, Settings) to mirror the iOS client's TabView. Sub-screens (Advanced, Profiles, Change Server, Troubleshoot, About) are reached from the Settings tab and use an iOS-style sectioned list layout. - New SettingsFragment (sectioned list mirroring iOSSettingsView) - Promote PeersFragment / NetworksFragment to top-level destinations; drop the modal BottomDialogFragment + PagerAdapter - Profile chip on Home opens a ProfilePickerSheet (one-tap switch + Manage profiles link), echoing the iOS ProfileBadge - Restyle AdvancedFragment + TroubleshootFragment as sectioned lists; Theme Mode now opens a bottom-sheet picker - Refresh menu icons to thinner outlined Material Symbols - NavigationRailView via layout-w960dp for large screens / TV - Toolbar hidden on top-level destinations; visible on sub-screens - Profile cards adopt PR #137 dark-mode contrast fix - Treat the empty-profile-state JSON read as a normal first-launch case in ProfileManagerWrapper instead of logging at error level
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:
📝 WalkthroughWalkthroughThe app switches from drawer navigation to bottom navigation, adds profile and theme bottom sheets, rebuilds home and settings screens around row-based layouts, and removes legacy dialog and Lottie-based UI pieces. ChangesNavigation, profile, and settings shell
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 7
🧹 Nitpick comments (4)
app/src/main/res/layout/list_item_profile_picker.xml (1)
21-29: ⚡ Quick winConstrain profile name to a single line for stable row layout.
At Line 21-29, long names can wrap and push row height unexpectedly. Add
maxLines="1"andellipsize="end"to keep picker rows consistent.🤖 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/res/layout/list_item_profile_picker.xml` around lines 21 - 29, The TextView with id profile_picker_name can wrap long names and change row height; update the element (profile_picker_name) to constrain it to a single line by adding maxLines="1" and ellipsize="end" so overflowing text is truncated with an ellipsis and picker rows remain a stable height.app/src/main/res/drawable/ic_nav_settings.xml (1)
7-8: ⚡ Quick winAvoid hardcoded white fill for navigation icons.
Line 7 uses
#FFFFFFFF, which makes this asset less theme-adaptive. Prefer a theme color (or rely on menu/icon tint) so the icon works across light/dark and future palette changes.🤖 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/res/drawable/ic_nav_settings.xml` around lines 7 - 8, The vector drawable currently hardcodes android:fillColor="#FFFFFFFF" which prevents theming; update the path element in ic_nav_settings.xml to use a theme attribute instead (e.g. replace android:fillColor="#FFFFFFFF" with android:fillColor="?attr/colorControlNormal" or another appropriate theme attr like ?attr/colorOnSurface), or remove the fillColor so the menu/icon tint can apply; ensure the path element with android:pathData remains unchanged.tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java (1)
56-64: ⚡ Quick winConsider improving error detection robustness for first-launch profile state handling.
The code currently detects empty profile state files by matching the error message
"unexpected end of JSON input". While this message originates from Go's standardencoding/jsonpackage (making it inherently stable), relying on message text remains fragile as a detection pattern. For improved maintainability, consider checking for the underlying exception type or implementing a more explicit state-check approach (e.g., attempting to detect empty/missing state files before callinggetActiveProfile(), or requesting gomobile expose a dedicated error code or exception type for this scenario).🤖 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 56 - 64, The code in ProfileManagerWrapper is brittle because it detects the first-launch empty profile by matching the error message text from getActiveProfile; instead, detect the empty/missing state more robustly before parsing (e.g., check the profile state file exists and its length/content is empty) or catch a specific exception type if gomobile exposes one; update getActiveProfile call-site to first inspect the profile state file (or wrap the parsing call and inspect the underlying cause) and only treat the empty-file case as a benign fallback (log via TAG) while letting other exceptions be logged as errors.app/src/main/res/layout/list_item_setting_section.xml (1)
4-12: ⚡ Quick winUse the shared
SettingsSectionHeaderstyle here to avoid style drift.This layout duplicates the same header attributes now defined in
@style/SettingsSectionHeader. Reusing the style keeps section headers consistent across screens.Suggested diff
<TextView xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools" - android:layout_width="match_parent" - android:layout_height="wrap_content" - android:paddingStart="20dp" - android:paddingEnd="20dp" - android:paddingTop="24dp" - android:paddingBottom="8dp" - android:textAllCaps="true" - android:textColor="@color/nb_txt_light" - android:textSize="13sp" + style="@style/SettingsSectionHeader" tools:text="Connection" />🤖 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/res/layout/list_item_setting_section.xml` around lines 4 - 12, The TextView in this layout duplicates header attributes that are already captured by the shared style; replace the inline attributes by applying style="@style/SettingsSectionHeader" on the TextView (remove duplicated android:textAllCaps, android:textColor, android:textSize and any padding/text attributes that the style covers) so the view uses the single source of truth (SettingsSectionHeader) and avoid style drift; ensure any attributes not in the style that are specific to this layout remain, and verify the TextView ID/text content is unchanged.
🤖 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 `@app/src/main/java/io/netbird/client/MainActivity.java`:
- Around line 129-142: The first-install onboarding only hides bottomNav but
still falls through to compute isTopLevel and shows the toolbar; update the
navController.addOnDestinationChangedListener handler so that when
destination.getId() == R.id.firstInstallFragment you both set
bottomNav.setVisibility(View.GONE) and call setToolbarVisible(false) (and then
skip the top-level logic, e.g., return or an else) so the onboarding screen
bypasses both bottom navigation and the toolbar; reference the listener,
firstInstallFragment, bottomNav, setToolbarVisible, and topLevelDestinations
when making the change.
In `@app/src/main/java/io/netbird/client/ui/home/ProfilePickerSheet.java`:
- Around line 133-143: The validation uses sanitizeProfileName(profileName) but
the code still calls profileManager.addProfile(profileName), so invalid
characters get written; change the write to use the sanitized value instead. In
ProfilePickerSheet.replace the call to profileManager.addProfile(profileName)
should be profileManager.addProfile(sanitized) (and likewise use sanitized in
the success Toast/getString call) so the persisted profile and user feedback
match the validated name.
In `@app/src/main/java/io/netbird/client/ui/settings/SettingsFragment.java`:
- Around line 56-58: The click handler for binding.rowDocumentation creates an
ACTION_VIEW Intent with DOCS_URL and directly calls startActivity, which can
crash if no browser-capable handler exists; update the lambda that handles
binding.rowDocumentation.setOnClickListener to first query the PackageManager
(e.g., via requireContext().getPackageManager()) and use
intent.resolveActivity(packageManager) or packageManager.queryIntentActivities
to ensure there's at least one handler before calling startActivity, and if none
is found fail gracefully (e.g., show a toast or disable the action).
In `@app/src/main/res/drawable/ic_add.xml`:
- Line 7: The vector path in ic_add.xml hardcodes android:fillColor="#FFFFFFFF";
remove that literal and make the icon theme-tintable by replacing the hardcoded
value with a neutral theme attribute (e.g. ?attr/colorControlNormal or
?attr/colorOnBackground) or a named color resource, so the host view/menu can
apply tinting; update the android:fillColor attribute accordingly (and ensure
ImageView/MenuItem usage applies tint if needed).
In `@app/src/main/res/layout/activity_main.xml`:
- Around line 27-45: The root ConstraintLayout in fragment_home.xml is missing
the bottom navigation inset padding; open fragment_home.xml, locate the root
ConstraintLayout element and add the attribute
android:paddingBottom="@dimen/bottom_nav_inset" so it matches other top-level
fragments (e.g., fragment_peers, fragment_networks) and prevents content from
being hidden behind the BottomNavigationView; ensure you reference the existing
dimen resource bottom_nav_inset (no other changes needed).
In `@app/src/main/res/layout/sheet_theme_picker.xml`:
- Around line 21-116: The theme rows (theme_row_system, theme_row_light,
theme_row_dark) don't expose selection state to TalkBack; update each row to set
an accessible role and dynamic state by adding a contentDescription or
stateDescription that reflects whether its associated check ImageView
(theme_check_system, theme_check_light, theme_check_dark) is visible/selected,
and update that description whenever selection changes; additionally, after
changing selection call announceForAccessibility with a short message (e.g.
"Light theme selected") or attach a custom AccessibilityDelegate on the row
views to send TYPE_VIEW_SELECTED/STATE_CHANGED events so screen readers announce
the new selection.
In `@app/src/main/res/values/dimens.xml`:
- Around line 11-12: The bottom_nav_inset resource is set universally to 80dp
but large-screen rail layouts shouldn't have that bottom inset; add a
configuration-specific override by creating a w960dp-qualified values dimen
resource (e.g., values with qualifier w960dp) that defines bottom_nav_inset as
0dp (or another rail-appropriate value) while keeping the existing default 80dp
in the base dimens.xml so large screens won't get unnecessary bottom padding.
---
Nitpick comments:
In `@app/src/main/res/drawable/ic_nav_settings.xml`:
- Around line 7-8: The vector drawable currently hardcodes
android:fillColor="#FFFFFFFF" which prevents theming; update the path element in
ic_nav_settings.xml to use a theme attribute instead (e.g. replace
android:fillColor="#FFFFFFFF" with android:fillColor="?attr/colorControlNormal"
or another appropriate theme attr like ?attr/colorOnSurface), or remove the
fillColor so the menu/icon tint can apply; ensure the path element with
android:pathData remains unchanged.
In `@app/src/main/res/layout/list_item_profile_picker.xml`:
- Around line 21-29: The TextView with id profile_picker_name can wrap long
names and change row height; update the element (profile_picker_name) to
constrain it to a single line by adding maxLines="1" and ellipsize="end" so
overflowing text is truncated with an ellipsis and picker rows remain a stable
height.
In `@app/src/main/res/layout/list_item_setting_section.xml`:
- Around line 4-12: The TextView in this layout duplicates header attributes
that are already captured by the shared style; replace the inline attributes by
applying style="@style/SettingsSectionHeader" on the TextView (remove duplicated
android:textAllCaps, android:textColor, android:textSize and any padding/text
attributes that the style covers) so the view uses the single source of truth
(SettingsSectionHeader) and avoid style drift; ensure any attributes not in the
style that are specific to this layout remain, and verify the TextView ID/text
content is unchanged.
In `@tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java`:
- Around line 56-64: The code in ProfileManagerWrapper is brittle because it
detects the first-launch empty profile by matching the error message text from
getActiveProfile; instead, detect the empty/missing state more robustly before
parsing (e.g., check the profile state file exists and its length/content is
empty) or catch a specific exception type if gomobile exposes one; update
getActiveProfile call-site to first inspect the profile state file (or wrap the
parsing call and inspect the underlying cause) and only treat the empty-file
case as a benign fallback (log via TAG) while letting other exceptions be logged
as errors.
🪄 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: 411e484a-8e49-4f4f-b6b3-3b442e3b8742
📒 Files selected for processing (62)
app/src/main/java/io/netbird/client/MainActivity.javaapp/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.javaapp/src/main/java/io/netbird/client/ui/advanced/ThemePickerSheet.javaapp/src/main/java/io/netbird/client/ui/home/BottomDialogFragment.javaapp/src/main/java/io/netbird/client/ui/home/HomeFragment.javaapp/src/main/java/io/netbird/client/ui/home/PagerAdapter.javaapp/src/main/java/io/netbird/client/ui/home/ProfilePickerAdapter.javaapp/src/main/java/io/netbird/client/ui/home/ProfilePickerSheet.javaapp/src/main/java/io/netbird/client/ui/settings/SettingsFragment.javaapp/src/main/res/drawable/ic_add.xmlapp/src/main/res/drawable/ic_arrow_drop_down.xmlapp/src/main/res/drawable/ic_check.xmlapp/src/main/res/drawable/ic_chevron_right.xmlapp/src/main/res/drawable/ic_menu_about.xmlapp/src/main/res/drawable/ic_menu_advanced.xmlapp/src/main/res/drawable/ic_menu_change_server.xmlapp/src/main/res/drawable/ic_menu_docs.xmlapp/src/main/res/drawable/ic_menu_profile.xmlapp/src/main/res/drawable/ic_menu_troubleshoot.xmlapp/src/main/res/drawable/ic_nav_home.xmlapp/src/main/res/drawable/ic_nav_networks.xmlapp/src/main/res/drawable/ic_nav_peers.xmlapp/src/main/res/drawable/ic_nav_settings.xmlapp/src/main/res/drawable/ic_open_in_new.xmlapp/src/main/res/drawable/ic_profile_avatar.xmlapp/src/main/res/drawable/profile_chip_bg.xmlapp/src/main/res/drawable/settings_row_bg.xmlapp/src/main/res/drawable/sheet_row_bg.xmlapp/src/main/res/layout-w960dp/activity_main.xmlapp/src/main/res/layout/activity_main.xmlapp/src/main/res/layout/app_bar_main.xmlapp/src/main/res/layout/content_main.xmlapp/src/main/res/layout/fragment_about.xmlapp/src/main/res/layout/fragment_advanced.xmlapp/src/main/res/layout/fragment_bottom_dialog.xmlapp/src/main/res/layout/fragment_home.xmlapp/src/main/res/layout/fragment_networks.xmlapp/src/main/res/layout/fragment_peers.xmlapp/src/main/res/layout/fragment_profiles.xmlapp/src/main/res/layout/fragment_server.xmlapp/src/main/res/layout/fragment_settings.xmlapp/src/main/res/layout/fragment_troubleshoot.xmlapp/src/main/res/layout/list_item_profile.xmlapp/src/main/res/layout/list_item_profile_picker.xmlapp/src/main/res/layout/list_item_setting.xmlapp/src/main/res/layout/list_item_setting_divider.xmlapp/src/main/res/layout/list_item_setting_section.xmlapp/src/main/res/layout/list_item_setting_toggle.xmlapp/src/main/res/layout/nav_custom_bottom_item.xmlapp/src/main/res/layout/nav_header_main.xmlapp/src/main/res/layout/sheet_profile_picker.xmlapp/src/main/res/layout/sheet_theme_picker.xmlapp/src/main/res/menu/activity_main_drawer.xmlapp/src/main/res/menu/bottom_nav.xmlapp/src/main/res/navigation/mobile_navigation.xmlapp/src/main/res/values-night/colors.xmlapp/src/main/res/values-night/themes.xmlapp/src/main/res/values/colors.xmlapp/src/main/res/values/dimens.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/values/themes.xmltool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java
💤 Files with no reviewable changes (8)
- app/src/main/res/layout/nav_custom_bottom_item.xml
- app/src/main/res/layout/fragment_bottom_dialog.xml
- app/src/main/res/layout/nav_header_main.xml
- app/src/main/java/io/netbird/client/ui/home/PagerAdapter.java
- app/src/main/res/layout/app_bar_main.xml
- app/src/main/res/menu/activity_main_drawer.xml
- app/src/main/res/layout/content_main.xml
- app/src/main/java/io/netbird/client/ui/home/BottomDialogFragment.java
The destination listener was hiding the bottom nav on the first-launch fragment but still falling through to the top-level toolbar logic, so the toolbar stayed visible. Treat firstInstallFragment as a full-screen take-over and bypass the rest of the listener.
ProfilePickerSheet validated the input via sanitizeProfileName() but still wrote the raw user string, so disallowed characters (anything outside [A-Za-z0-9_-]) made it into the engine. Pass the sanitized value to addProfile() and the success/duplicate toasts so what the user sees matches what was stored.
On a TV or stripped-down build there may not be an Activity that handles ACTION_VIEW for an https URL. Catch ActivityNotFoundException and surface a toast instead of crashing the app.
Hardcoded #FFFFFFFF prevents these icons from adapting to theme overlays (e.g. dark mode, focus inversions on TV). Switch to ?attr/colorControlNormal so each icon picks up the correct tint from its host theme. Affects ic_nav_home, ic_nav_peers, ic_nav_networks, ic_nav_settings, ic_add, ic_check, ic_chevron_right, ic_arrow_drop_down, ic_open_in_new — all custom icons introduced in this branch.
Theme picker rows now expose: - contentDescription with the theme label - stateDescription "Selected" on the active row (Android R+) - isSelected=true on the active row for accessibility services After picking, announce "<theme> theme selected" on the sheet root so TalkBack confirms the change before the sheet dismisses.
On large screens the bottom navigation moves to a side rail (layout-w960dp/activity_main.xml). The 80dp bottom padding fragments add to clear the bottom bar is therefore wasted vertical space — set the dimen to 0dp at the same qualifier so layouts pick the right value automatically without per-layout overrides.
A long profile name was wrapping and pushing the picker row taller than the others, breaking the row rhythm. maxLines=1 + ellipsize=end truncates the overflow with "…" so picker rows stay uniform.
list_item_setting_section.xml duplicated the same padding / text size / caps attributes already defined in @style/SettingsSectionHeader, which the section headers inside fragment_settings.xml etc. already apply via the style. Replace the inline attributes with a single style= reference so the section template is the same source of truth.
CodeRabbit suggested catching a typed exception or pre-checking the state file length. The gomobile binding flattens the Go error chain into go.Universe$proxyerror without surfacing the underlying type, and the state-file path is a Go-side constant not exposed to Java, so neither approach is available without a gomobile API change. Expand the comment to make that trade-off explicit so the next reader doesn't try the same refactor.
Squash-merge the four commits from PR #189 (profile-id branch) onto the redesign branch, adapting the profile-id migration to the new bottom-nav UI instead of the old drawer/NavigationView layout: - getActiveProfile() now returns a Profile (with ID) instead of a String; update SettingsFragment and HomeFragment callers to use getName(). - Drop the PR's drawer-specific MainActivity changes (updateProfileMenuItem, drawer onKeyDown) — the redesign replaced the drawer with bottom nav. - Graft the new disable-IPv6 switch listener into AdvancedFragment and add the IPv6 settings row to fragment_advanced.xml in the redesign row style. - Bump netbird submodule to 62afff6 (adds Profile.ID to the gomobile binding).
Replace the Lottie connect button and background mask with a custom pill-shaped SwitchMaterial toggle (white thumb, orange track when connected), and restructure the home layout to match the iOS client: centered profile chip, NetBird logo, status text, hostname with a tappable IP/IPv6 detail section. - Remove Lottie dependency, ButtonAnimation and unused JSON assets - Add Inter and JetBrains Mono fonts; logo from the iOS client - Hostname shown emphasized, IPv4 in the muted summary (with chevron) - Info rows (IPv4 + IPv6) expand on tapping the summary; IPv6 row only shown when an IPv6 address is available; copy-to-clipboard buttons - Append ellipsis to Connecting/Disconnecting status - Make bottom nav unselected items white in night mode - Drop the 'profile created'/'switched to profile' success toasts
* Add profile id migration * Check if ID is set on Profile * Bump netbird * Update profile-id-name branch * Fix active profile errors, bump netbird * Bump netbird to v0.74.0
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 (2)
app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java (1)
121-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject names that sanitize to empty
ServiceManager.AddProfile()already strips invalid characters, so this flow still needs the post-sanitize empty-name check fromProfilePickerSheet(or the backend should enforce it). Inputs like!!!become""and get written as.json, creating a blank profile entry.🤖 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 121 - 135, The add-profile flow in showAddDialog currently only rejects null or raw empty input, but names like !!! can sanitize to an empty string and still be passed to addProfile, creating a blank .json entry. Update the validation in the showAddDialog callback, and mirror the ProfilePickerSheet post-sanitize check, so the sanitized profile name is verified to be non-empty before calling addProfile (or enforce the same rule in ServiceManager.AddProfile).app/src/main/java/io/netbird/client/ui/home/HomeFragment.java (1)
73-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConnect click path doesn't disable the toggle, unlike disconnect.
The disconnect branch disables
buttonConnectimmediately before callingswitchConnection(false), but the connect branch does not disable it beforeswitchConnection(true). A rapid double-tap on "connect" can triggerswitchConnection(true)twice beforeonConnecting()arrives and disables the button.🐛 Proposed fix
} else { // We're currently disconnected, so connect + buttonConnect.setEnabled(false); setStatusText(R.string.main_status_connecting); serviceAccessor.switchConnection(true); }🤖 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/home/HomeFragment.java` around lines 73 - 88, The connect click path in HomeFragment’s buttonConnect listener is missing the immediate disable that the disconnect path already uses, which allows repeated taps to call serviceAccessor.switchConnection(true) multiple times before onConnecting() disables the control. Update the else branch in the buttonConnect onClickListener to disable buttonConnect before setting the connecting status and invoking switchConnection(true), matching the disconnect flow and preventing double-triggered connection attempts.
🧹 Nitpick comments (1)
tool/src/main/java/io/netbird/client/tool/EngineRunner.java (1)
86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm
ProfileoverridestoString()for this log line.
activeProfileis now aProfileobject (previously aString), and it's logged directly via"Initializing engine with profile: " + activeProfile. IfProfiledoesn't overridetoString(), this will log an unhelpfulProfile@hashcodeinstead of the profile name/id, degrading this debug log's usefulness.♻️ Proposed fix
- Profile activeProfile = profileManager.getActiveProfile(); - Log.d(LOGTAG, "Initializing engine with profile: " + activeProfile); + Profile activeProfile = profileManager.getActiveProfile(); + Log.d(LOGTAG, "Initializing engine with profile: " + activeProfile.getName() + " (" + activeProfile.getId() + ")");🤖 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/EngineRunner.java` around lines 86 - 89, The EngineRunner log line now concatenates the activeProfile Profile object directly, so verify Profile overrides toString() to return a meaningful name or id. If it does not, update Profile’s toString() or change the log in EngineRunner to explicitly log the desired profile field via getActiveProfile() so the "Initializing engine with profile" message stays readable and useful.
🤖 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 `@app/src/main/java/io/netbird/client/ui/home/HomeFragment.java`:
- Around line 159-176: The status/button update paths in HomeFragment are
re-reading fragment view fields across the post() thread hop, which can become
null after the initial check. In setStatusText(), setToggle(), and the
onAddressChanged handling, capture textConnStatus, buttonConnect, and binding
into local variables first, null-check those locals, and use only the locals
inside the posted Runnable so onDestroyView() cannot null them between checks
and use.
In `@tool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.java`:
- Around line 77-86: Profile switching is still using the profile name in the
caller while ProfileManagerWrapper.switchProfile now expects an ID. Update the
Home profile chip flow that invokes switchProfile so it passes profile.getID()
instead of the display name, matching the wrapper’s ID-based routing and keeping
the behavior consistent with ProfilePickerSheet.
---
Outside diff comments:
In `@app/src/main/java/io/netbird/client/ui/home/HomeFragment.java`:
- Around line 73-88: The connect click path in HomeFragment’s buttonConnect
listener is missing the immediate disable that the disconnect path already uses,
which allows repeated taps to call serviceAccessor.switchConnection(true)
multiple times before onConnecting() disables the control. Update the else
branch in the buttonConnect onClickListener to disable buttonConnect before
setting the connecting status and invoking switchConnection(true), matching the
disconnect flow and preventing double-triggered connection attempts.
In `@app/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.java`:
- Around line 121-135: The add-profile flow in showAddDialog currently only
rejects null or raw empty input, but names like !!! can sanitize to an empty
string and still be passed to addProfile, creating a blank .json entry. Update
the validation in the showAddDialog callback, and mirror the ProfilePickerSheet
post-sanitize check, so the sanitized profile name is verified to be non-empty
before calling addProfile (or enforce the same rule in
ServiceManager.AddProfile).
---
Nitpick comments:
In `@tool/src/main/java/io/netbird/client/tool/EngineRunner.java`:
- Around line 86-89: The EngineRunner log line now concatenates the
activeProfile Profile object directly, so verify Profile overrides toString() to
return a meaningful name or id. If it does not, update Profile’s toString() or
change the log in EngineRunner to explicitly log the desired profile field via
getActiveProfile() so the "Initializing engine with profile" message stays
readable and useful.
🪄 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: 3acf034a-011b-4f62-a07e-db1353f256a2
⛔ Files ignored due to path filters (6)
app/src/main/res/drawable-night-xxhdpi/bg_bottom.pngis excluded by!**/*.pngapp/src/main/res/drawable-night/nb_logo_full.pngis excluded by!**/*.pngapp/src/main/res/drawable-xxhdpi/bg_bottom.pngis excluded by!**/*.pngapp/src/main/res/drawable/nb_logo_full.pngis excluded by!**/*.pngapp/src/main/res/font/inter_variable.ttfis excluded by!**/*.ttfapp/src/main/res/font/jetbrains_mono_variable.ttfis excluded by!**/*.ttf
📒 Files selected for processing (52)
Readme.mdapp/build.gradle.ktsapp/src/main/assets/button_full.jsonapp/src/main/assets/button_full_dark.jsonapp/src/main/assets/loading.jsonapp/src/main/java/io/netbird/client/MainActivity.javaapp/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.javaapp/src/main/java/io/netbird/client/ui/advanced/ThemePickerSheet.javaapp/src/main/java/io/netbird/client/ui/home/ButtonAnimation.javaapp/src/main/java/io/netbird/client/ui/home/HomeFragment.javaapp/src/main/java/io/netbird/client/ui/home/Peer.javaapp/src/main/java/io/netbird/client/ui/home/PeersAdapter.javaapp/src/main/java/io/netbird/client/ui/home/PeersFragmentViewModel.javaapp/src/main/java/io/netbird/client/ui/home/ProfilePickerSheet.javaapp/src/main/java/io/netbird/client/ui/home/Resource.javaapp/src/main/java/io/netbird/client/ui/profile/ProfilesFragment.javaapp/src/main/java/io/netbird/client/ui/settings/SettingsFragment.javaapp/src/main/res/color/connect_thumb_tint.xmlapp/src/main/res/color/connect_track_tint.xmlapp/src/main/res/color/tab_icon_color.xmlapp/src/main/res/color/tab_text_color.xmlapp/src/main/res/drawable/connect_switch_thumb.xmlapp/src/main/res/drawable/connect_switch_track.xmlapp/src/main/res/drawable/ic_add.xmlapp/src/main/res/drawable/ic_arrow_drop_down.xmlapp/src/main/res/drawable/ic_check.xmlapp/src/main/res/drawable/ic_chevron_right.xmlapp/src/main/res/drawable/ic_content_copy.xmlapp/src/main/res/drawable/ic_nav_home.xmlapp/src/main/res/drawable/ic_nav_networks.xmlapp/src/main/res/drawable/ic_nav_peers.xmlapp/src/main/res/drawable/ic_nav_settings.xmlapp/src/main/res/drawable/ic_open_in_new.xmlapp/src/main/res/drawable/info_row_bg.xmlapp/src/main/res/layout/fragment_advanced.xmlapp/src/main/res/layout/fragment_home.xmlapp/src/main/res/layout/list_item_profile_picker.xmlapp/src/main/res/layout/list_item_setting_section.xmlapp/src/main/res/menu/peer_clipboard_menu.xmlapp/src/main/res/values-night/colors.xmlapp/src/main/res/values-w960dp/dimens.xmlapp/src/main/res/values/colors.xmlapp/src/main/res/values/strings.xmlgradle/libs.versions.tomlnetbirdtool/src/main/java/io/netbird/client/tool/EngineRunner.javatool/src/main/java/io/netbird/client/tool/IFace.javatool/src/main/java/io/netbird/client/tool/NetworkChangeNotifier.javatool/src/main/java/io/netbird/client/tool/Profile.javatool/src/main/java/io/netbird/client/tool/ProfileManagerWrapper.javatool/src/main/java/io/netbird/client/tool/TUNParameters.javatool/src/main/java/io/netbird/client/tool/VPNService.java
💤 Files with no reviewable changes (6)
- app/src/main/assets/loading.json
- app/src/main/java/io/netbird/client/ui/home/ButtonAnimation.java
- app/src/main/assets/button_full.json
- app/src/main/assets/button_full_dark.json
- app/build.gradle.kts
- gradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (14)
- app/src/main/res/color/connect_thumb_tint.xml
- app/src/main/res/drawable/ic_chevron_right.xml
- app/src/main/res/values-w960dp/dimens.xml
- app/src/main/res/drawable/ic_check.xml
- app/src/main/res/drawable/ic_nav_settings.xml
- app/src/main/res/color/connect_track_tint.xml
- app/src/main/res/drawable/ic_nav_home.xml
- app/src/main/res/layout/list_item_setting_section.xml
- app/src/main/res/color/tab_text_color.xml
- app/src/main/res/drawable/ic_add.xml
- app/src/main/res/layout/list_item_profile_picker.xml
- Readme.md
- app/src/main/res/drawable/ic_nav_peers.xml
- app/src/main/res/values-night/colors.xml
🚧 Files skipped from review as they are similar to previous changes (9)
- app/src/main/res/drawable/ic_nav_networks.xml
- app/src/main/res/drawable/ic_arrow_drop_down.xml
- app/src/main/java/io/netbird/client/ui/home/ProfilePickerSheet.java
- app/src/main/java/io/netbird/client/ui/settings/SettingsFragment.java
- app/src/main/res/layout/fragment_advanced.xml
- app/src/main/java/io/netbird/client/ui/advanced/ThemePickerSheet.java
- app/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.java
- app/src/main/res/values/strings.xml
- app/src/main/java/io/netbird/client/MainActivity.java
Resolve conflicts in favor of the iOS-style bottom-nav redesign: - MainActivity: keep bottom-nav AppBarConfiguration; drop drawer setup, updateProfileMenuItem and drawer-based onKeyDown (drawer no longer exists) - AdvancedFragment: keep compact row-click listeners; drop redundant main block and a duplicate layoutDisableIpv6 listener - fragment_advanced.xml: keep settings_row_bg IPv6/firewall rows; drop main's ConstraintLayout variant that duplicated @+id/layout_force_relay_connection - ProfilesFragment: keep no success toast on profile switch - ProfileManagerWrapper: take main's getActiveProfile (getIsActive + throw); fresh-install empty state is now handled inside the engine - netbird submodule: advance to current main pointer (3aa6c02) Note: gomobile netbird.aar must be regenerated from the new submodule so the Profile.getIsActive() binding is available.
- Rework the connect toggle thumb as a centred layer so the white circle size is independent of the track height - Give the disconnected track a light grey fill with a thin darker border and disable Material auto-tint so the custom per-state colours render - Make the bottom nav background white and drive item icon/text colour off state_checked; use opaque icon fills so itemIconTint applies at full strength - Lighten the home background
- Give the connect toggle thumb a soft drop shadow (baked into the drawable; drop thumbTint so the shadow is not tinted white) - Merge the two info rows (IP / secondary value) into a single bordered box with a middle divider; showDividers hides the separator when the secondary row is gone
Revert to a plain white circle for the connect toggle thumb.
The recent white-surface tweaks hardcoded @color/white and #4A4A4A, which have no night variant, breaking dark mode (white bottom nav, white profile chip, invisible unselected icons). Introduce theme-aware semantic colours instead: - nb_bottom_nav_bg / nb_chip_bg: white in light, dark surface in night - tab_icon_unselected: #4A4A4A in light, white in night
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/res/drawable/connect_switch_track.xml (1)
10-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting repeated dimensions.
88dp/44dp/100dpare duplicated across both selector items. Minor, purely stylistic.Also applies to: 19-20
🤖 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/res/drawable/connect_switch_track.xml` around lines 10 - 11, The switch track drawable repeats the same size and corner radius values in both selector items; extract the shared 88dp, 44dp, and 100dp measurements into reusable dimension resources and reference them from the shape definitions in the connect_switch_track drawable so both items stay consistent and easier to maintain.
🤖 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.
Nitpick comments:
In `@app/src/main/res/drawable/connect_switch_track.xml`:
- Around line 10-11: The switch track drawable repeats the same size and corner
radius values in both selector items; extract the shared 88dp, 44dp, and 100dp
measurements into reusable dimension resources and reference them from the shape
definitions in the connect_switch_track drawable so both items stay consistent
and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09e5f192-5e9b-4894-aa5d-ddc9fa50b5ac
📒 Files selected for processing (15)
app/src/main/res/color/connect_track_tint.xmlapp/src/main/res/color/tab_icon_color.xmlapp/src/main/res/color/tab_text_color.xmlapp/src/main/res/drawable/connect_switch_thumb.xmlapp/src/main/res/drawable/connect_switch_track.xmlapp/src/main/res/drawable/ic_nav_home.xmlapp/src/main/res/drawable/ic_nav_networks.xmlapp/src/main/res/drawable/ic_nav_peers.xmlapp/src/main/res/drawable/ic_nav_settings.xmlapp/src/main/res/drawable/info_row_divider.xmlapp/src/main/res/drawable/profile_chip_bg.xmlapp/src/main/res/layout/activity_main.xmlapp/src/main/res/layout/fragment_home.xmlapp/src/main/res/values-night/colors.xmlapp/src/main/res/values/colors.xml
✅ Files skipped from review due to trivial changes (7)
- app/src/main/res/drawable/connect_switch_thumb.xml
- app/src/main/res/color/tab_text_color.xml
- app/src/main/res/drawable/ic_nav_networks.xml
- app/src/main/res/drawable/profile_chip_bg.xml
- app/src/main/res/drawable/info_row_divider.xml
- app/src/main/res/color/connect_track_tint.xml
- app/src/main/res/values-night/colors.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/res/color/tab_icon_color.xml
- app/src/main/res/drawable/ic_nav_settings.xml
- app/src/main/res/layout/activity_main.xml
- app/src/main/res/layout/fragment_home.xml
- app/src/main/res/values/colors.xml
- Reduce row height (52->40dp), text (15->12sp) and copy buttons (40->34dp) - Middle-ellipsize the IPv6 row so a long address is truncated in the centre while the copy button still copies the full value
Use opaque fillColor so app:tint renders the profile picker + and check icons at full nb_orange intensity, matching the manage icon.
Replace the default circular ripple mask on bottom nav / navigation rail items with a rounded-rect (8dp) mask.
Navigating back to home re-inflates the fragment, so the hardcoded android:text default painted "Disconnected" before the real state arrived — and the state update was deferred by two nested post() calls even though registration replays it on the main thread. Move the layout default to tools:text, and apply view updates inline when already on the main thread (engine callbacks, which arrive off the main thread, still post). Snap the toggle thumb after setChecked so it doesn't animate into place on a fresh view.
The sheet listed every profile in a wrap_content RecyclerView, so past a handful of profiles the list grew past the screen and pushed the "Add profile" and "Manage profiles" rows out of reach. Cap the list height, show only the five most recently used profiles, and surface a "Show all profiles (N)" row into the manage screen when there are more. Expand the sheet on open so a long list no longer parks at peek height. Recency is tracked in SharedPreferences since the gomobile Profile has no last-used field. The store prunes entries for profiles that no longer exist on every read, so deleting a profile needs no bookkeeping from the caller and the store cannot outgrow the profile count. Also fix the sheet passing a profile name where switchProfile expects an ID, which made switching from the sheet fail whenever the two differed.
The portrait lock added in 4c45fab applied to every non-TV device. On a landscape-only display such as a car head unit the system letterboxes a portrait-locked activity into a narrow strip in the middle of the screen and sizes the keyboard to that strip, which made the app unusable there. Gate the lock on a lock_portrait bool that is true by default and false from sw600dp up, so phones keep the portrait-only UX while tablets, head units and TV rotate freely.
The client refuses management-requested remote jobs unless RemoteJobsAllowed is enabled, and the Android app had no way to turn it on. Add an "Allow remote debug bundles" switch to the Troubleshoot screen below the anonymize switch, wired to the new Preferences accessors, and bump the netbird submodule to the commit that exposes them.
The es, fr, it, ja and ru locales were left out when the toggle was added.
MainActivity.onStart also runs when the system rebuilds the activity while the process is still in a cached background state, most visibly after a long idle. Android 12+ rejects startService from there with BackgroundServiceStartNotAllowedException, which was the top crash on v0.5.0 (Crashlytics afc5032635d8574f7a70e7ab7b519acf). The startService call only existed to refresh the foreground notification with the engine's current state on every return to the foreground. Binding with BIND_AUTO_CREATE is allowed from the background and already creates the service, so the refresh now goes over the binder once onServiceConnected runs. It re-posts the notification only while the service is already in the foreground: with the engine stopped the service is not foreground, and promoting a bind-only, background-started service via startForeground is rejected the same way. The INTENT_ACTION_START branch in onStartCommand stays for the quick settings tile, which starts the service with startForegroundService and therefore needs the prompt startForeground call.
Crashlytics issue 5edaa820: tapping the zero-peer screen's learn-why button on a device without a browser threw ActivityNotFoundException and killed the app. Catch it the same way the settings documentation row already does and show the existing no-browser toast. The about screen's terms and privacy links used the same unguarded pattern, so they get the same handling.
Crashlytics issue 8b135d6f: when the Go profile manager could not resolve the active profile paths (a rename of active_profile.json failed with permission denied on a custom-ROM device), runClient rethrew the error as a RuntimeException on its worker thread, which killed the whole app. Route the failure through notifyError and the stopped callback like a failed engine run, reset the running flag, and end the thread cleanly so the UI returns to idle and the user can retry.
Crashlytics issue ae2e7a40: on a device shipping without the system vpndialogs package, launching the intent returned by VpnService.prepare threw ActivityNotFoundException from switchConnection and killed the app. Catch it, log it, and tell the user the device cannot show the VPN permission dialog instead of crashing. The message is added to every locale.
* Let the user choose which apps the tunnel carries Adds a split tunnelling screen with three modes: off, exclude the picked apps, or include only them. The two selections are stored apart, since Android's builder takes an allow list or a deny list but never both. A change forces a TUN rebuild, so it applies without reconnecting. * Keep the split tunnelling selection with the profile Moves the mode and both app selections out of SharedPreferences and into the profile's preferences on the Go side, so they follow the active profile instead of being shared by every profile on the device. SplitTunnelConfig is unchanged and still owns the VpnService rules: allow list against deny list, the historic exclusions, and the fallback when an include selection is empty. The new SplitTunnelStore only translates between it and the Go settings, and resolves the active profile. Needs the matching submodule change. * Take the split tunnelling mode as a number across the binding The submodule bump also carries the split tunnel store the previous commit needed.
The row icon and the search field glyph were left orange while every other settings row and search field paints its icon in the muted text colour. The row also reused the advanced settings tune icon, so the two entries were indistinguishable in the list. Give the row a call-split glyph of its own, tint both it and the search drawable like their siblings, and move the empty-include warning below the search field so the warning sits next to the list it describes instead of pushing the search box down.
The split tunnelling screen shipped with English-only strings, and the SSH menu entry, the peer SSH action and the profile actions label had been left untranslated earlier, so all nine locales were fourteen keys short of the base file. Fill them in and keep the feature name itself in English: it is an established term of art, so the menu label is marked untranslatable and dropped from the locale files rather than rendered nine different ways. Only the explanatory sentences around it are translated. Paint the empty-include warning in the muted text colour too, matching the hint above it now that the row no longer stands out in orange.
In off mode the search field stayed on screen, merely disabled, and it sat above the hint explaining why there is nothing to search. That put a dead input between the mode row and the sentence describing the state. Move the field above the hint in the layout and drop it entirely while the mode is off, so the hint follows the mode row directly. Clear the query as the field goes away: a hidden EditText keeps its text, which would leave the list filtered by an invisible query after switching back.
The four packages the tunnel has always kept out were only applied in SplitTunnelConfig.resolve, so the list showed them with an unchecked switch in Exclude mode while the tunnel dropped them, and let Include mode pull them into the tunnel against the reason they were excluded in the first place. Their rows are now locked: on in Exclude, off in Include, never toggleable, with a short note saying why. Include mode strips them from the allow list, and an include selection made only of them counts as inactive. Stale picks of these packages from older builds are pruned with the uninstalled ones.
Pruning rewrites the stored selection but went through persist(), which only touches the store. The running tunnel kept the pruned packages in its rules until something else happened to save, so the stored and the live state disagreed. Go through save() like every other writer, so the code that changes the stored selection is also the code that reconciles the tunnel with it.
queueTUNRenewal did a check-then-create on a plain field and then read it twice. That was reachable from the route change callback alone until the split tunnelling binder method started calling it from the main thread, which also nulls the field in onDestroy. Two callers racing on the create could each start a looper thread, orphaning one with a live Looper, and a callback arriving during teardown could dereference the field after onDestroy cleared it, aborting the process from a gomobile callback. Build the thread with the service and start it in onCreate, leaving queueTUNRenewal to only post, with one getHandler() call per renewal. The field is final now, so teardown just quits the looper: a late message is refused by sendMessage, which the existing log already reports.
On Pixel devices com.google.android.apps.scone probes the network through the VPN, misjudges the Wi-Fi as broken and drops it. The package has no launcher entry, so the user could not exclude it from the app list either.
An Include pick that was uninstalled is skipped by the builder, so a stored selection made only of such picks raised a tunnel that allowed nothing but this app until the list screen was next opened. The resolution now takes a predicate for what is installed and falls back to carrying everything when no pick survives it.
The list only holds packages with an enabled launcher entry, so a disabled app or one whose launcher activity went away in an update was taken for uninstalled, dropped from the selection and the tunnel rebuilt without it. Stale picks are harmless, the tunnel already skips them, and keeping them lets the setting survive a reinstall. Only the always-excluded packages are still cleaned out of the stored selections.
onNetworkChanged walks the list on the Go callback thread while the main thread adds and removes listeners on every bind and unbind, so a route update that landed during a rebind could throw ConcurrentModificationException inside the gomobile callback and take the service process down.
theodorsm
previously approved these changes
Sep 7, 2026
The Gradle daemon JVM is declared in gradle/gradle-daemon-jvm.properties (toolchainVersion=17, generated by updateDaemonJvm with the foojay resolver so the wrapper downloads it where missing). The NDK is a constant in build-android-lib.sh, which now sets ANDROID_NDK_HOME itself and stops with the install command when that version is absent. gomobile bind gets -androidapi 26 to match minSdk; without it the API 16 default only works with NDK r23 and older. CI reads both versions from those two files instead of carrying its own copies, drops the gomobile install step the script already performs, and switches the test jobs from the defunct "adopt" distribution to temurin.
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworks the Android UI, and brings over features the desktop client already had.
Navigation
Home
New screens and features
SSO session expiry
SSH client
Split tunneling
Fixes