Revert ForceRelay default to false now that P2P issues are fixed#152
Revert ForceRelay default to false now that P2P issues are fixed#152MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
Conversation
The default was set to true in netbirdio#108 as a workaround for P2P stability issues on Android. Now that the underlying ICE issues are fixed (netbirdio/netbird: guard loop, candidate buffering, anet for Android), P2P connections work reliably and should be enabled by default.
📝 WalkthroughWalkthroughThis pull request updates the netbird submodule reference to a newer commit and changes the default return value of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
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 `@netbird`:
- Line 1: The PR changes the default for keyForceRelayConnection in
Preferences.java from true to false but that file wasn't included for review;
open Preferences.java and verify the change to
getBoolean(keyForceRelayConnection, false) is intentional and consistent with
the rest of the codebase, then update any related logic or documentation that
assumes the old default (search for keyForceRelayConnection, getBoolean, and any
code paths handling forced relay behavior) to align behavior and tests with the
new default, and add or adjust unit/integration tests that assert the previous
default to reflect the new false default.
In `@tool/src/main/java/io/netbird/client/tool/Preferences.java`:
- Line 30: The test
PreferencesInstrumentedTest.shouldReturnTrueWhenConnectionForceRelayedIsNotSet
must be updated to reflect the new default in
Preferences.isConnectionForceRelayed(): change the assertion that currently
calls assertTrue(...) to
Assert.assertFalse(preferences.isConnectionForceRelayed()); so the test verifies
the new default false value returned by
Preferences.getBoolean(keyForceRelayConnection, false).
- Line 30: The change to Preferences.isConnectionForceRelayed() returns false by
default which breaks the test
shouldReturnTrueWhenConnectionForceRelayedIsNotSet; revert the default to true
or update the method to return sharedPref.getBoolean(keyForceRelayConnection,
true) so that isConnectionForceRelayed() yields true when the preference is
unset (refer to the method isConnectionForceRelayed and the key
keyForceRelayConnection).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: adc3d29a-5ebe-4800-bd49-f60d8c1080f6
📒 Files selected for processing (2)
netbirdtool/src/main/java/io/netbird/client/tool/Preferences.java
| @@ -1 +1 @@ | |||
| Subproject commit 7bbe71c3ac4b8d95cb6e5b764dd87d6ba8c9c271 | |||
| Subproject commit d0aa79eebf102ecfcccbbb73fb1bdd8547dbc26e | |||
There was a problem hiding this comment.
Note: Main code change (Preferences.java) not included in review files.
The PR objectives describe a critical code change in tool/src/main/java/io/netbird/client/tool/Preferences.java that changes getBoolean(keyForceRelayConnection, true) to getBoolean(keyForceRelayConnection, false). This file was not included in the files for review. Please ensure this change is also reviewed for correctness and consistency with the submodule update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbird` at line 1, The PR changes the default for keyForceRelayConnection in
Preferences.java from true to false but that file wasn't included for review;
open Preferences.java and verify the change to
getBoolean(keyForceRelayConnection, false) is intentional and consistent with
the rest of the codebase, then update any related logic or documentation that
assumes the old default (search for keyForceRelayConnection, getBoolean, and any
code paths handling forced relay behavior) to align behavior and tests with the
new default, and add or adjust unit/integration tests that assert the previous
default to reflect the new false default.
|
|
||
| public boolean isConnectionForceRelayed() { | ||
| return sharedPref.getBoolean(keyForceRelayConnection, true); | ||
| return sharedPref.getBoolean(keyForceRelayConnection, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the netbird submodule contains the P2P fixes mentioned in the PR
# Check recent commits in the netbird submodule for the referenced fixes
cd netbird 2>/dev/null && git log --oneline --grep="guard loop\|candidate buffering\|anet.*Android" -20 || echo "netbird submodule not found in current directory"
# Search for any migration logic related to this preference
cd ..
rg -n -C5 'keyForceRelayConnection|isConnectionForceRelayed' --type java --type kotlinRepository: netbirdio/android-client
Length of output: 8356
🏁 Script executed:
# Check the entire PreferencesInstrumentedTest file to see all affected tests
cat -n tool/src/androidTest/java/io/netbird/client/tool/PreferencesInstrumentedTest.javaRepository: netbirdio/android-client
Length of output: 3265
Update test to match new default behavior.
The test shouldReturnTrueWhenConnectionForceRelayedIsNotSet (line 48-50 in PreferencesInstrumentedTest.java) will fail because it asserts assertTrue() while the new default is false. Update the assertion to Assert.assertFalse(preferences.isConnectionForceRelayed()); to match the changed default in Preferences.java line 30.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tool/src/main/java/io/netbird/client/tool/Preferences.java` at line 30, The
test
PreferencesInstrumentedTest.shouldReturnTrueWhenConnectionForceRelayedIsNotSet
must be updated to reflect the new default in
Preferences.isConnectionForceRelayed(): change the assertion that currently
calls assertTrue(...) to
Assert.assertFalse(preferences.isConnectionForceRelayed()); so the test verifies
the new default false value returned by
Preferences.getBoolean(keyForceRelayConnection, false).
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests and code that may depend on the default value of isConnectionForceRelayed
# Search for test assertions on isConnectionForceRelayed
rg -n -C3 'isConnectionForceRelayed\(\)' --glob '*Test*.java' --glob '*Test*.kt'
# Search for any other references to verify migration impact
rg -n -C3 'isConnectionForceRelayed' --type javaRepository: netbirdio/android-client
Length of output: 7657
The test shouldReturnTrueWhenConnectionForceRelayedIsNotSet will fail with this change.
The test at tool/src/androidTest/java/io/netbird/client/tool/PreferencesInstrumentedTest.java:49 asserts assertTrue(preferences.isConnectionForceRelayed()), but the new default is false. This will block the build.
Fix
`@Test`
- public void shouldReturnTrueWhenConnectionForceRelayedIsNotSet() {
- Assert.assertTrue(preferences.isConnectionForceRelayed());
+ public void shouldReturnFalseWhenConnectionForceRelayedIsNotSet() {
+ Assert.assertFalse(preferences.isConnectionForceRelayed());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tool/src/main/java/io/netbird/client/tool/Preferences.java` at line 30, The
change to Preferences.isConnectionForceRelayed() returns false by default which
breaks the test shouldReturnTrueWhenConnectionForceRelayedIsNotSet; revert the
default to true or update the method to return
sharedPref.getBoolean(keyForceRelayConnection, true) so that
isConnectionForceRelayed() yields true when the preference is unset (refer to
the method isConnectionForceRelayed and the key keyForceRelayConnection).
1 participant
Summary
The default for
isConnectionForceRelayedwas set totruein #108 as a workaround for P2P stability issues on Android. The underlying ICE issues have since been fixed (netbirdio/netbird: guard loop fix, candidate buffering, anet for Android interface discovery), so P2P connections work reliably and should be enabled by default.This is a one-line change:
getBoolean(keyForceRelayConnection, true)→getBoolean(keyForceRelayConnection, false).Checklist
By submitting this pull request, I confirm that I have read and agree to the terms of the Contributor License Agreement.
Summary by CodeRabbit
Bug Fixes
Chores