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
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThe PR refactors the app's navigation architecture from a drawer-based model to bottom navigation, introduces profile management and theme selection via bottom sheets, and adds a dedicated settings screen. Navigation wiring, fragment implementations, and layouts are comprehensively updated to support the new structure. ChangesNavigation Architecture & Profile Management Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Summary by CodeRabbit
Release Notes
New Features
UI Improvements