Add Lazy connection toggle to Advanced settings#171
Open
MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
Open
Add Lazy connection toggle to Advanced settings#171MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a "lazy connection" toggle to the Advanced settings screen, enabling deferred per-peer connection setup until first traffic. It updates the layout, adds preference persistence wiring in the fragment, includes TV-remote interaction support, and bumps the netbird submodule. ChangesLazy Connection Advanced Setting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
8 tasks
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.java`:
- Around line 355-383: refreshConnectionModeAdapter contains hardcoded
user-visible strings ("currently", "engine not yet connected") and the
formatHint() text which must be localized; replace these inline literals with
string resources and use getString(...) with placeholders (e.g., a resource like
connection_mode_currently="%1$s (currently: %2$s)" and
connection_mode_engine_not_connected="%1$s (engine not yet connected)") when
building entries, and move the formatHint() literal into its own string resource
and call getString(R.string.<name>, ...) from the same methods
(refreshConnectionModeAdapter and the related block at lines ~428-433) so all UI
copy is loaded from resources and supports localization.
- Around line 437-448: The EditText blur handler in wireTimeoutEditOnBlur
currently clears the visible field but does not clear the persisted override;
when NumberFormatException occurs, call the provided LongConsumer onCommit with
a sentinel to clear the stored override (e.g. onCommit.accept(-1L)) before
returning so the engine stops using the old timeout override; update the catch
block in wireTimeoutEditOnBlur to invoke onCommit.accept(-1L) (or your project's
defined "clear" sentinel) after edit.setText("").
In `@app/src/main/res/layout/fragment_advanced.xml`:
- Around line 319-368: Remove the misleading autofillHints="postalCode" from the
timeout EditText fields (edit_relay_timeout, edit_p2p_timeout,
edit_p2p_retry_max) and disable autofill for these inputs by adding
android:importantForAutofill="no" (or clearing the autofillHints) so system
autofill services stop suggesting postal data for numeric timeout/retry
settings.
🪄 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: a829a070-0cbe-475e-949c-646d9c072e2e
📒 Files selected for processing (12)
app/src/main/java/io/netbird/client/MainActivity.javaapp/src/main/java/io/netbird/client/ServiceAccessor.javaapp/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.javaapp/src/main/res/layout/fragment_advanced.xmlapp/src/main/res/layout/spinner_item_themed.xmlapp/src/main/res/values/connection_mode_array.xmlapp/src/main/res/values/strings.xmlnetbirdtool/src/main/java/io/netbird/client/tool/EngineRunner.javatool/src/main/java/io/netbird/client/tool/EnvVarPackager.javatool/src/main/java/io/netbird/client/tool/Preferences.javatool/src/main/java/io/netbird/client/tool/VPNService.java
💤 Files with no reviewable changes (1)
- tool/src/main/java/io/netbird/client/tool/Preferences.java
✅ Files skipped from review due to trivial changes (2)
- app/src/main/res/layout/spinner_item_themed.xml
- app/src/main/res/values/connection_mode_array.xml
Comment on lines
+355
to
+383
| private void refreshConnectionModeAdapter() { | ||
| if (binding == null) return; | ||
| String[] base = getResources().getStringArray(R.array.connection_mode_entries); | ||
| String[] entries = base.clone(); | ||
| String pushed = ""; | ||
| try { | ||
| if (requireActivity() instanceof io.netbird.client.ServiceAccessor) { | ||
| pushed = ((io.netbird.client.ServiceAccessor) requireActivity()) | ||
| .getServerPushedConnectionMode(); | ||
| } | ||
| } catch (Throwable t) { | ||
| Log.d(LOGTAG, "no server-pushed mode available yet: " + t.getMessage()); | ||
| } | ||
| if (pushed != null && !pushed.isEmpty()) { | ||
| entries[0] = base[0] + " (currently: " + pushed + ")"; | ||
| } else { | ||
| entries[0] = base[0] + " (engine not yet connected)"; | ||
| } | ||
| int currentSelection = binding.spinnerConnectionMode.getSelectedItemPosition(); | ||
| ArrayAdapter<String> adapter = new ArrayAdapter<>( | ||
| requireContext(), R.layout.spinner_item_themed, entries); | ||
| adapter.setDropDownViewResource(R.layout.spinner_item_themed); | ||
| binding.spinnerConnectionMode.setAdapter(adapter); | ||
| if (currentSelection >= 0 && currentSelection < entries.length) { | ||
| binding.spinnerConnectionMode.setSelection(currentSelection); | ||
| } | ||
| // Server-pushed timeout values may have changed too; refresh hints. | ||
| refreshTimeoutHints(); | ||
| } |
Contributor
There was a problem hiding this comment.
Move the new status/hint copy into string resources.
"currently", "engine not yet connected", and the formatHint() text are all hardcoded here, so the new connection-mode UI will not localize with the rest of the screen.
Also applies to: 428-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.java` around
lines 355 - 383, refreshConnectionModeAdapter contains hardcoded user-visible
strings ("currently", "engine not yet connected") and the formatHint() text
which must be localized; replace these inline literals with string resources and
use getString(...) with placeholders (e.g., a resource like
connection_mode_currently="%1$s (currently: %2$s)" and
connection_mode_engine_not_connected="%1$s (engine not yet connected)") when
building entries, and move the formatHint() literal into its own string resource
and call getString(R.string.<name>, ...) from the same methods
(refreshConnectionModeAdapter and the related block at lines ~428-433) so all UI
copy is loaded from resources and supports localization.
Comment on lines
+437
to
+448
| private void wireTimeoutEditOnBlur(EditText edit, String label, LongConsumer onCommit) { | ||
| edit.setOnFocusChangeListener((view, hasFocus) -> { | ||
| if (hasFocus) return; | ||
| String s = edit.getText().toString().trim(); | ||
| long val = 0; | ||
| if (!s.isEmpty()) { | ||
| try { val = Long.parseLong(s); } | ||
| catch (NumberFormatException nfe) { | ||
| Log.w(LOGTAG, "Invalid " + label + " timeout: " + s); | ||
| edit.setText(""); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Clear the stored timeout override on invalid input.
Clearing the EditText and returning here leaves the previous persisted value intact. The field looks empty, but the engine still uses the old override until the user enters another valid value.
Suggested fix
if (!s.isEmpty()) {
try { val = Long.parseLong(s); }
catch (NumberFormatException nfe) {
Log.w(LOGTAG, "Invalid " + label + " timeout: " + s);
edit.setText("");
+ onCommit.accept(0);
return;
}
if (val < 0) val = 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private void wireTimeoutEditOnBlur(EditText edit, String label, LongConsumer onCommit) { | |
| edit.setOnFocusChangeListener((view, hasFocus) -> { | |
| if (hasFocus) return; | |
| String s = edit.getText().toString().trim(); | |
| long val = 0; | |
| if (!s.isEmpty()) { | |
| try { val = Long.parseLong(s); } | |
| catch (NumberFormatException nfe) { | |
| Log.w(LOGTAG, "Invalid " + label + " timeout: " + s); | |
| edit.setText(""); | |
| return; | |
| } | |
| private void wireTimeoutEditOnBlur(EditText edit, String label, LongConsumer onCommit) { | |
| edit.setOnFocusChangeListener((view, hasFocus) -> { | |
| if (hasFocus) return; | |
| String s = edit.getText().toString().trim(); | |
| long val = 0; | |
| if (!s.isEmpty()) { | |
| try { val = Long.parseLong(s); } | |
| catch (NumberFormatException nfe) { | |
| Log.w(LOGTAG, "Invalid " + label + " timeout: " + s); | |
| edit.setText(""); | |
| onCommit.accept(0); | |
| return; | |
| } | |
| if (val < 0) val = 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/io/netbird/client/ui/advanced/AdvancedFragment.java` around
lines 437 - 448, The EditText blur handler in wireTimeoutEditOnBlur currently
clears the visible field but does not clear the persisted override; when
NumberFormatException occurs, call the provided LongConsumer onCommit with a
sentinel to clear the stored override (e.g. onCommit.accept(-1L)) before
returning so the engine stops using the old timeout override; update the catch
block in wireTimeoutEditOnBlur to invoke onCommit.accept(-1L) (or your project's
defined "clear" sentinel) after edit.setText("").
Comment on lines
+319
to
+368
| <EditText | ||
| android:id="@+id/edit_relay_timeout" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:autofillHints="postalCode" | ||
| android:hint="@string/advanced_timeout_hint" | ||
| android:inputType="number" | ||
| android:maxLength="9" | ||
| android:textColor="@color/nb_txt_light" | ||
| android:textColorHint="@color/nb_txt_light" /> | ||
|
|
||
| <TextView | ||
| android:id="@+id/label_p2p_timeout" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginTop="8dp" | ||
| android:text="@string/advanced_p2p_timeout" | ||
| android:textColor="@color/nb_txt_light" | ||
| android:textSize="12sp" /> | ||
|
|
||
| <EditText | ||
| android:id="@+id/edit_p2p_timeout" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:autofillHints="postalCode" | ||
| android:hint="@string/advanced_timeout_hint" | ||
| android:inputType="number" | ||
| android:maxLength="9" | ||
| android:textColor="@color/nb_txt_light" | ||
| android:textColorHint="@color/nb_txt_light" /> | ||
|
|
||
| <TextView | ||
| android:id="@+id/label_p2p_retry_max" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginTop="8dp" | ||
| android:text="@string/advanced_p2p_retry_max" | ||
| android:textColor="@color/nb_txt_light" | ||
| android:textSize="12sp" /> | ||
|
|
||
| <EditText | ||
| android:id="@+id/edit_p2p_retry_max" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:autofillHints="postalCode" | ||
| android:hint="@string/advanced_timeout_hint" | ||
| android:inputType="number" | ||
| android:maxLength="9" | ||
| android:textColor="@color/nb_txt_light" | ||
| android:textColorHint="@color/nb_txt_light" /> |
Contributor
There was a problem hiding this comment.
Disable autofill on these timeout fields instead of labeling them as postal codes.
Using postalCode here will make autofill services suggest ZIP/postal data for numeric engine settings. These inputs should either opt out of autofill or use no hint at all.
Suggested fix
- android:autofillHints="postalCode"
+ android:importantForAutofill="no"
android:hint="@string/advanced_timeout_hint"
android:inputType="number"
android:maxLength="9"
android:textColor="@color/nb_txt_light"
android:textColorHint="@color/nb_txt_light" />
...
- android:autofillHints="postalCode"
+ android:importantForAutofill="no"
android:hint="@string/advanced_timeout_hint"
android:inputType="number"
android:maxLength="9"
android:textColor="@color/nb_txt_light"
android:textColorHint="@color/nb_txt_light" />
...
- android:autofillHints="postalCode"
+ android:importantForAutofill="no"
android:hint="@string/advanced_timeout_hint"
android:inputType="number"
android:maxLength="9"
android:textColor="@color/nb_txt_light"
android:textColorHint="@color/nb_txt_light" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <EditText | |
| android:id="@+id/edit_relay_timeout" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:autofillHints="postalCode" | |
| android:hint="@string/advanced_timeout_hint" | |
| android:inputType="number" | |
| android:maxLength="9" | |
| android:textColor="@color/nb_txt_light" | |
| android:textColorHint="@color/nb_txt_light" /> | |
| <TextView | |
| android:id="@+id/label_p2p_timeout" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:layout_marginTop="8dp" | |
| android:text="@string/advanced_p2p_timeout" | |
| android:textColor="@color/nb_txt_light" | |
| android:textSize="12sp" /> | |
| <EditText | |
| android:id="@+id/edit_p2p_timeout" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:autofillHints="postalCode" | |
| android:hint="@string/advanced_timeout_hint" | |
| android:inputType="number" | |
| android:maxLength="9" | |
| android:textColor="@color/nb_txt_light" | |
| android:textColorHint="@color/nb_txt_light" /> | |
| <TextView | |
| android:id="@+id/label_p2p_retry_max" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:layout_marginTop="8dp" | |
| android:text="@string/advanced_p2p_retry_max" | |
| android:textColor="@color/nb_txt_light" | |
| android:textSize="12sp" /> | |
| <EditText | |
| android:id="@+id/edit_p2p_retry_max" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:autofillHints="postalCode" | |
| android:hint="@string/advanced_timeout_hint" | |
| android:inputType="number" | |
| android:maxLength="9" | |
| android:textColor="@color/nb_txt_light" | |
| android:textColorHint="@color/nb_txt_light" /> | |
| <EditText | |
| android:id="@+id/edit_relay_timeout" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:importantForAutofill="no" | |
| android:hint="@string/advanced_timeout_hint" | |
| android:inputType="number" | |
| android:maxLength="9" | |
| android:textColor="@color/nb_txt_light" | |
| android:textColorHint="@color/nb_txt_light" /> | |
| <TextView | |
| android:id="@+id/label_p2p_timeout" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:layout_marginTop="8dp" | |
| android:text="@string/advanced_p2p_timeout" | |
| android:textColor="@color/nb_txt_light" | |
| android:textSize="12sp" /> | |
| <EditText | |
| android:id="@+id/edit_p2p_timeout" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:importantForAutofill="no" | |
| android:hint="@string/advanced_timeout_hint" | |
| android:inputType="number" | |
| android:maxLength="9" | |
| android:textColor="@color/nb_txt_light" | |
| android:textColorHint="@color/nb_txt_light" /> | |
| <TextView | |
| android:id="@+id/label_p2p_retry_max" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:layout_marginTop="8dp" | |
| android:text="@string/advanced_p2p_retry_max" | |
| android:textColor="@color/nb_txt_light" | |
| android:textSize="12sp" /> | |
| <EditText | |
| android:id="@+id/edit_p2p_retry_max" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:importantForAutofill="no" | |
| android:hint="@string/advanced_timeout_hint" | |
| android:inputType="number" | |
| android:maxLength="9" | |
| android:textColor="@color/nb_txt_light" | |
| android:textColorHint="@color/nb_txt_light" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/layout/fragment_advanced.xml` around lines 319 - 368, Remove
the misleading autofillHints="postalCode" from the timeout EditText fields
(edit_relay_timeout, edit_p2p_timeout, edit_p2p_retry_max) and disable autofill
for these inputs by adding android:importantForAutofill="no" (or clearing the
autofillHints) so system autofill services stop suggesting postal data for
numeric timeout/retry settings.
Adds a switch in the Advanced fragment that controls the LazyConnectionEnabled engine setting. Default is OFF so existing behavior is preserved; users opt in to per-peer connection on first traffic for reduced background load. The toggle wires through the new GetLazyConnectionEnabled / SetLazyConnectionEnabled methods on the gomobile Preferences bridge. Tested on a Galaxy S21 (Android 15): toggling the switch persists LazyConnectionEnabled in the config; on reconnect the engine activates the lazy connection manager (setup lazy connection service, per-peer activity listeners on 127.0.0.1:N, 15-minute inactivity threshold) and peers are wired up on demand instead of eagerly.
MichaelUray
force-pushed
the
feat/lazy-connection-toggle-upstream
branch
from
77bc67f to
113c2b4
Compare
Author
|
Rebased on The connection-mode picker UI + 3 timeouts that I had prototyped on top of this branch are no longer here — they belong to the broader Phase-3.7i Android UI work, which depends on the Go-side bridges that are still in review on the netbirdio/netbird#6081-#6084 stack. I'll open that as a separate android-client PR after the backend stack lands; opening it now would just sit red against current Verified locally on Galaxy S21 (Android 15):
So this should be ready for a maintainer pass once #5994 is in. |
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.
Describe your changes
Adds a "Lazy connection" switch to the Advanced settings fragment (sits between Block inbound connections and Disable client routes). The toggle controls the engine's
LazyConnectionEnabledsetting via the gomobilePreferencesbridge.layout_lazy_connectionblock infragment_advanced.xml; the constraint chain is updated solayout_disable_client_routesis anchored below the new block.advanced_lazy_connection/advanced_lazy_connection_desc.AdvancedFragment.initializeEngineConfigSwitches()reads the current value viagoPreferences.getLazyConnectionEnabled()and writes back viasetLazyConnectionEnabled()+commit()on toggle, with a TV-remote layout click handler wired throughlayout_lazy_connection.toggle().Default is off, so behavior on existing installs does not change until a user enables it explicitly.
Issue ticket number and link
N/A.
Checklist
Dependency
This PR consumes two new bridge methods on the gomobile
Preferencestype:GetLazyConnectionEnabled() (bool, error)SetLazyConnectionEnabled(enabled bool)Those are added in netbirdio/netbird#5994. This PR's submodule pointer is intentionally not bumped — it should land after #5994 merges, at which point a routine submodule update will pick up the new bridge symbols and this PR will compile.
Testing
Verified end-to-end on a Galaxy S21 (SM-G991B, Android 15) with a local build that includes the bridge change:
aapt2 dump resourcesconfirmsid/layout_lazy_connection,id/switch_lazy_connection,string/advanced_lazy_connectionare baked into the APK.dexdumpconfirmsAdvancedFragmentcallsPreferences.{get,set}LazyConnectionEnabled.client/internal/lazyconn/manager/manager.go: setup lazy connection serviceclient/internal/lazyconn/inactivity/manager.go: inactivity threshold configured: 15m0sclient/internal/conn_mgr.go: peer added to lazy conn manager(per peer)client/internal/lazyconn/activity/listener_udp.go: created activity listener: 127.0.0.1:NNNNNremoving activity listener→adding peer to inactivity managerclient/internal/conn_mgr.go: lazy connection manager is disabled.Summary by CodeRabbit