Phase 1: ConnectionModeSelect replaces Lazy toggle (issue netbirdio/netbird#5989)#627
Open
MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
Open
Phase 1: ConnectionModeSelect replaces Lazy toggle (issue netbirdio/netbird#5989)#627MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
Conversation
Phase 1 of issue netbirdio/netbird#5989. The dashboard now exposes the new connection_mode field as a 2-value dropdown (P2P, P2P Lazy) with a conditional relay-timeout input that only appears in P2P-Lazy mode. The two other modes (relay-forced, p2p-dynamic) remain admin-only via the management API in Phase 1; relay-forced because it is a power-user compliance choice that should not be one click away in the UI, and p2p-dynamic because its daemon-side implementation is reserved for Phase 2. Backwards-compat: - The legacy lazy_connection_enabled boolean is written alongside the new mode (true when mode == p2p-lazy, false otherwise) so older daemon versions that only know the boolean keep behaving identically to today. - Existing accounts with no connection_mode in the DB seed the dropdown via the same legacy-bool fallback used on the server side (resolveLegacyLazyBool helper). Mode-change preserves the persisted relay-timeout value (per spec section 5.3): users only lose their entered timeout if they explicitly clear the input, not by switching modes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request replaces the legacy Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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. Review rate limit: 6/8 reviews remaining, refill in 14 minutes and 41 seconds.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: 2
🧹 Nitpick comments (1)
src/modules/settings/ClientSettingsTab.tsx (1)
236-245: 💤 Low valueConsider adding visual feedback for invalid timeout input.
When the user enters an invalid value (non-integer, negative), the handler silently returns without feedback. The user won't know why their input wasn't accepted. Since changes save immediately, consider either:
- Showing an inline error message (similar to
versionErrorhandling)- Using a controlled input that prevents invalid characters
This is minor given the Phase 1 scope and the clear placeholder guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/settings/ClientSettingsTab.tsx` around lines 236 - 245, handleRelayTimeoutChange currently silently ignores invalid timeout input; add visible inline feedback by introducing a local error state (e.g., relayTimeoutError) and update it inside handleRelayTimeoutChange: when trimmed === "" or parsed is valid clear relayTimeoutError and call saveConnectionMode(connectionMode, parsed|null); when parsed is invalid set relayTimeoutError to a short message (e.g., "Timeout must be a non‑negative integer") and do not call saveConnectionMode. Also update the input element to show the error text (similar to versionError handling) and consider adding input-level guards (type="number" and min=0 or inputMode/pattern) to reduce invalid keystrokes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 439-451: The relay timeout Input rendered under
currentMeta.showsRelayTimeout lacks an accessible label; update the Input
component (the instance using value derived from relayTimeoutSeconds,
placeholder DEFAULT_RELAY_TIMEOUT_SECONDS, onChange handleRelayTimeoutChange,
and disabled tied to permission.settings.update) to include an accessible
label—e.g., add an aria-label or aria-labelledby that clearly describes the
field (such as "Relay timeout in seconds") so screen readers can identify the
input while keeping the existing customPrefix icon.
- Around line 200-227: The saveConnectionMode function currently performs an
optimistic update via setConnectionMode and setRelayTimeoutSeconds before
saveRequest.put completes; capture the previous values (e.g., const prevMode =
connectionMode; const prevTimeout = relayTimeoutSeconds) then perform the
optimistic set, call saveRequest.put(...).then(() => mutate("/accounts")), and
in the .catch() revert state with setConnectionMode(prevMode) and
setRelayTimeoutSeconds(prevTimeout) and surface an error notify; ensure the
notify promise uses the saveRequest.put promise (so rollback runs on failure)
and keep references to saveConnectionMode, setConnectionMode,
setRelayTimeoutSeconds, saveRequest.put, and mutate("/accounts") when
implementing the rollback.
---
Nitpick comments:
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 236-245: handleRelayTimeoutChange currently silently ignores
invalid timeout input; add visible inline feedback by introducing a local error
state (e.g., relayTimeoutError) and update it inside handleRelayTimeoutChange:
when trimmed === "" or parsed is valid clear relayTimeoutError and call
saveConnectionMode(connectionMode, parsed|null); when parsed is invalid set
relayTimeoutError to a short message (e.g., "Timeout must be a non‑negative
integer") and do not call saveConnectionMode. Also update the input element to
show the error text (similar to versionError handling) and consider adding
input-level guards (type="number" and min=0 or inputMode/pattern) to reduce
invalid keystrokes.
🪄 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: 4175f2d5-ee22-4e71-8b4b-58e3b961f66c
📒 Files selected for processing (3)
src/interfaces/Account.tssrc/modules/settings/ClientSettingsTab.tsxsrc/modules/settings/connectionmode/modeOptions.ts
Comment on lines
+200
to
227
| // Phase 1 (#5989): persist mode + timeout, AND mirror onto the legacy | ||
| // lazy_connection_enabled boolean so older daemon versions stay in sync. | ||
| const saveConnectionMode = async ( | ||
| nextMode: ConnectionModeValue, | ||
| nextRelayTimeout: number | null, | ||
| ) => { | ||
| setConnectionMode(nextMode); | ||
| setRelayTimeoutSeconds(nextRelayTimeout); | ||
|
|
||
| notify({ | ||
| title: "Lazy Connections", | ||
| description: `Lazy Connections successfully ${ | ||
| toggle ? "enabled" : "disabled" | ||
| }.`, | ||
| title: "Connection Mode", | ||
| description: "Connection mode updated.", | ||
| promise: saveRequest | ||
| .put({ | ||
| id: account.id, | ||
| settings: { | ||
| ...account.settings, | ||
| lazy_connection_enabled: toggle, | ||
| connection_mode: nextMode, | ||
| relay_timeout_seconds: nextRelayTimeout, | ||
| lazy_connection_enabled: modeImpliesLegacyLazy(nextMode), | ||
| }, | ||
| }) | ||
| .then(() => { | ||
| setLazyConnection(toggle); | ||
| mutate("/accounts"); | ||
| }), | ||
| loadingMessage: "Updating Lazy Connections setting...", | ||
| loadingMessage: "Updating connection mode...", | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Optimistic state update without rollback on failure.
Lines 206-207 update local state before the API call completes. If saveRequest.put() fails, the UI will show the new values while the server retains the old ones.
Consider capturing the previous state and reverting on error:
🛡️ Proposed fix to add rollback on failure
const saveConnectionMode = async (
nextMode: ConnectionModeValue,
nextRelayTimeout: number | null,
) => {
+ const prevMode = connectionMode;
+ const prevTimeout = relayTimeoutSeconds;
setConnectionMode(nextMode);
setRelayTimeoutSeconds(nextRelayTimeout);
notify({
title: "Connection Mode",
description: "Connection mode updated.",
promise: saveRequest
.put({
id: account.id,
settings: {
...account.settings,
connection_mode: nextMode,
relay_timeout_seconds: nextRelayTimeout,
lazy_connection_enabled: modeImpliesLegacyLazy(nextMode),
},
})
.then(() => {
mutate("/accounts");
+ })
+ .catch((err) => {
+ setConnectionMode(prevMode);
+ setRelayTimeoutSeconds(prevTimeout);
+ throw err;
}),
loadingMessage: "Updating connection mode...",
});
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/settings/ClientSettingsTab.tsx` around lines 200 - 227, The
saveConnectionMode function currently performs an optimistic update via
setConnectionMode and setRelayTimeoutSeconds before saveRequest.put completes;
capture the previous values (e.g., const prevMode = connectionMode; const
prevTimeout = relayTimeoutSeconds) then perform the optimistic set, call
saveRequest.put(...).then(() => mutate("/accounts")), and in the .catch() revert
state with setConnectionMode(prevMode) and setRelayTimeoutSeconds(prevTimeout)
and surface an error notify; ensure the notify promise uses the saveRequest.put
promise (so rollback runs on failure) and keep references to saveConnectionMode,
setConnectionMode, setRelayTimeoutSeconds, saveRequest.put, and
mutate("/accounts") when implementing the rollback.
Comment on lines
+439
to
+451
| {currentMeta.showsRelayTimeout && ( | ||
| <Input | ||
| value={ | ||
| relayTimeoutSeconds === null | ||
| ? "" | ||
| : String(relayTimeoutSeconds) | ||
| } | ||
| customPrefix={<ClockFadingIcon size={14} />} | ||
| placeholder={String(DEFAULT_RELAY_TIMEOUT_SECONDS)} | ||
| onChange={(e) => handleRelayTimeoutChange(e.target.value)} | ||
| disabled={!permission.settings.update} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Add accessible label for the relay timeout input.
The timeout input uses only an icon as customPrefix with no visible or accessible label. Screen reader users won't understand the field's purpose.
♿ Proposed fix to add aria-label
<Input
value={
relayTimeoutSeconds === null
? ""
: String(relayTimeoutSeconds)
}
customPrefix={<ClockFadingIcon size={14} />}
placeholder={String(DEFAULT_RELAY_TIMEOUT_SECONDS)}
onChange={(e) => handleRelayTimeoutChange(e.target.value)}
disabled={!permission.settings.update}
+ aria-label="Relay timeout in seconds"
/>📝 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
| {currentMeta.showsRelayTimeout && ( | |
| <Input | |
| value={ | |
| relayTimeoutSeconds === null | |
| ? "" | |
| : String(relayTimeoutSeconds) | |
| } | |
| customPrefix={<ClockFadingIcon size={14} />} | |
| placeholder={String(DEFAULT_RELAY_TIMEOUT_SECONDS)} | |
| onChange={(e) => handleRelayTimeoutChange(e.target.value)} | |
| disabled={!permission.settings.update} | |
| /> | |
| )} | |
| {currentMeta.showsRelayTimeout && ( | |
| <Input | |
| value={ | |
| relayTimeoutSeconds === null | |
| ? "" | |
| : String(relayTimeoutSeconds) | |
| } | |
| customPrefix={<ClockFadingIcon size={14} />} | |
| placeholder={String(DEFAULT_RELAY_TIMEOUT_SECONDS)} | |
| onChange={(e) => handleRelayTimeoutChange(e.target.value)} | |
| disabled={!permission.settings.update} | |
| aria-label="Relay timeout in seconds" | |
| /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/settings/ClientSettingsTab.tsx` around lines 439 - 451, The relay
timeout Input rendered under currentMeta.showsRelayTimeout lacks an accessible
label; update the Input component (the instance using value derived from
relayTimeoutSeconds, placeholder DEFAULT_RELAY_TIMEOUT_SECONDS, onChange
handleRelayTimeoutChange, and disabled tied to permission.settings.update) to
include an accessible label—e.g., add an aria-label or aria-labelledby that
clearly describes the field (such as "Relay timeout in seconds") so screen
readers can identify the input while keeping the existing customPrefix icon.
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
2 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.
Summary
Companion to backend PR netbirdio/netbird#6047.
Replaces the binary "Lazy Connection" toggle in the Client Settings tab with a connection-mode dropdown. Phase 1 shows two values (
P2P,P2P Lazy) plus a conditionalrelay-timeoutinput that only appears in P2P-Lazy mode.The other two modes (
relay-forced,p2p-dynamic) remain admin-only via the management API in Phase 1:relay-forcedis a power-user / compliance choice that should not be one click away in the UI.p2p-dynamicis reserved at the proto / DB level but its daemon implementation is Phase 2; surfacing it in the dropdown before the daemon honours it would be misleading.Backwards-compat
The save callback writes both the new fields AND mirrors
p2p-lazyonto the legacylazy_connection_enabledboolean -- so admins running an older backend or older clients continue to see consistent behaviour.The dropdown's initial value comes from
connection_modeif set, falling back to the legacylazy_connection_enabledboolean viaresolveLegacyLazyBool. Mode-change preserves the persisted relay-timeout (per spec section 5.3): users only lose their entered timeout if they explicitly clear the input.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes