Merged
Conversation
…escriptions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR introduces peer exposure configuration to account settings. It extends the Account interface with peer_expose_enabled and peer_expose_groups properties, implements UI components for managing peer group selections in the ClientSettingsTab with loading states, and adds activity logging for peer expose/unexpose events. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as ClientSettingsTab
participant State as React State
participant API as Account Service
participant Log as ActivityDescription
User->>UI: Toggle peer_expose_enabled
activate UI
UI->>State: Update peer_expose_enabled state
UI->>UI: Derive peer_expose_groups from selected groups
deactivate UI
User->>UI: Click Save
activate UI
UI->>State: Validate: groups selected if enabled
alt Validation passes
UI->>API: Update account.settings with peer_expose config
activate API
API-->>UI: Settings saved
deactivate API
UI->>State: Update reference state
else Validation fails
UI-->>User: Save button disabled
end
deactivate UI
API->>Log: Log service.peer.expose/unexpose activity
activate Log
Log-->>User: Display activity with peer details
deactivate Log
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
🧹 Nitpick comments (3)
src/modules/settings/ClientSettingsTab.tsx (1)
138-166: Peer expose groups are persisted even when peer expose is disabled.When
peerExposeEnabledisfalse, the save still sendspeer_expose_groups: peerExposeGroupIds. This is likely harmless if the backend ignores groups when the feature is disabled, but it could be more intentional to clear or omit them.🤖 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 138 - 166, The saveChanges handler currently always includes peer_expose_groups in the payload even when peerExposeEnabled is false; update the payload in saveRequest.put (inside saveChanges / notify) to conditionally omit or clear groups by setting peer_expose_groups to either peerExposeGroupIds when peerExposeEnabled is true or to an empty array/undefined when false, and make the corresponding update to updateRef to pass the cleared value too so the local state and mutation reflect the disabled state.custom-zones.patch (2)
1190-1199: StalecurrentZone/currentRecordwhen opening modals for new items.
openZoneModalonly setscurrentZonewhen a zone is passed, andopenRecordModalonly setscurrentRecordwhen a record is passed. If a previous edit left these state variables populated and the user opens a "create new" flow before the close handler clears them, the modal could display stale data.This is likely safe in practice because the close handler clears state, but a defensive reset would be more robust:
Suggested fix
const openZoneModal = (zone?: DNSZone, distributionGroups?: Group[]) => { - if (zone) setCurrentZone(zone); - if (distributionGroups) setInitialDistributionGroups(distributionGroups); + setCurrentZone(zone); + setInitialDistributionGroups(distributionGroups); setDnsModal(true); }; const openRecordModal = (zone: DNSZone, record?: DNSRecord) => { setCurrentZone(zone); - if (record) setCurrentRecord(record); + setCurrentRecord(record); setRecordModal(true); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom-zones.patch` around lines 1190 - 1199, openZoneModal and openRecordModal only set currentZone/currentRecord when a value is passed, leaving previous state if called for "create new"; update openZoneModal to always call setCurrentZone(zone ?? undefined) (or explicitly clear when no zone) and always call setInitialDistributionGroups(distributionGroups ?? [] or undefined) before setDnsModal(true), and update openRecordModal to always call setCurrentRecord(record ?? undefined) (clearing stale record) before setRecordModal(true), using the existing setters setCurrentZone, setInitialDistributionGroups, setCurrentRecord, setDnsModal and setRecordModal.
2275-2276: Inconsistent truthy check forzones_countvs other count fields.Other count fields use explicit
> 0comparisons (e.g.,row.routes_count > 0,row.setup_keys_count > 0), butzones_countrelies on implicit truthiness. While functionally equivalent for numeric values, this inconsistency could mask issues ifzones_countis everundefined.Suggested fix for consistency
- row.resources_count > 0 || - row.zones_count + row.resources_count > 0 || + row.zones_count > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom-zones.patch` around lines 2275 - 2276, The conditional mixes explicit numeric checks (e.g., row.resources_count > 0, row.routes_count > 0, row.setup_keys_count > 0) with an implicit truthy check for row.zones_count; change the implicit check to an explicit numeric comparison (row.zones_count > 0) so all count fields use consistent > 0 checks and avoid false positives when zones_count is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom-zones.patch`:
- Around line 304-305: DNS_RECORDS_DOCS_LINK is incorrectly set to the same URL
as DNS_ZONE_DOCS_LINK; update DNS_RECORDS_DOCS_LINK to point to the specific DNS
records documentation (for example replace
"https://docs.netbird.io/manage/dns/zones" with
"https://docs.netbird.io/manage/dns/zones#records" or the correct records page).
Locate the exported constant DNS_RECORDS_DOCS_LINK in the diff and change its
value to the appropriate records-specific URL so the two constants reference
distinct, correct docs pages.
- Around line 2343-2345: zonesCount can be undefined because zonesGroups may be
undefined; change the computation of zonesCount (derived from zonesGroups and
group.id) to always produce a number (e.g., default to 0) before assigning to
zones_count: ensure you call filter only when zonesGroups is defined or use a
safe fallback like (zonesGroups ? zonesGroups.filter(...) : []) and then take
.length so zonesCount is always a number; update the reference in the object
that sets zones_count to use this numeric zonesCount.
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 77-102: The false-positive comes from initializing
peerExposeGroups as [] before initialGroups loads; fix by initializing state
from initialGroups (const [peerExposeGroups, setPeerExposeGroups] =
useState<Group[]>(initialGroups ?? [])) and in the React.useEffect that runs
when initialGroups changes, setPeerExposeGroups(initialGroups) and then call
updateRef() from useHasChanges to reset the baseline so hasChanges doesn't flip
true on load; ensure peerExposeGroupIds useMemo stays derived from
peerExposeGroups.
---
Nitpick comments:
In `@custom-zones.patch`:
- Around line 1190-1199: openZoneModal and openRecordModal only set
currentZone/currentRecord when a value is passed, leaving previous state if
called for "create new"; update openZoneModal to always call setCurrentZone(zone
?? undefined) (or explicitly clear when no zone) and always call
setInitialDistributionGroups(distributionGroups ?? [] or undefined) before
setDnsModal(true), and update openRecordModal to always call
setCurrentRecord(record ?? undefined) (clearing stale record) before
setRecordModal(true), using the existing setters setCurrentZone,
setInitialDistributionGroups, setCurrentRecord, setDnsModal and setRecordModal.
- Around line 2275-2276: The conditional mixes explicit numeric checks (e.g.,
row.resources_count > 0, row.routes_count > 0, row.setup_keys_count > 0) with an
implicit truthy check for row.zones_count; change the implicit check to an
explicit numeric comparison (row.zones_count > 0) so all count fields use
consistent > 0 checks and avoid false positives when zones_count is undefined.
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 138-166: The saveChanges handler currently always includes
peer_expose_groups in the payload even when peerExposeEnabled is false; update
the payload in saveRequest.put (inside saveChanges / notify) to conditionally
omit or clear groups by setting peer_expose_groups to either peerExposeGroupIds
when peerExposeEnabled is true or to an empty array/undefined when false, and
make the corresponding update to updateRef to pass the cleared value too so the
local state and mutation reflect the disabled state.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 100-113: The useEffect that reads initialGroups and calls
setPeerExposeGroups/updateRef references autoUpdateMethod,
autoUpdateCustomVersion, peerExposeEnabled, and updateRef but intentionally
omits them from the dependency array; add an eslint suppression comment to avoid
react-hooks/exhaustive-deps warnings: place a single-line comment like //
eslint-disable-next-line react-hooks/exhaustive-deps immediately above the
React.useEffect(...) (which uses initialGroups, setPeerExposeGroups, and
updateRef) so the linter knows the omission is intentional.
- Around line 338-367: The direct child of AnimatePresence (the outer wrapper
div rendered when peerExposeEnabled is true) must include a unique React key so
Framer Motion can track enter/exit; update the JSX that renders the wrapper div
(the element containing className "overflow-hidden -top-4 relative z-0") to
include a stable key (e.g. key="peer-expose" or derived from peerExposeEnabled)
while leaving the inner motion.div and PeerGroupSelector, setPeerExposeGroups,
and peerExposeGroups logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 364-368: PeerGroupSelector is interactive for users without update
rights; add a disabled guard by passing disabled={!permission.settings.update}
to the PeerGroupSelector and prevent updates when disabled (e.g., make onChange
a no-op or conditionally call setPeerExposeGroups only if
permission.settings.update is true) so the selector is visibly and functionally
read-only for users lacking permission.
---
Duplicate comments:
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 100-113: The effect that runs on initialGroups is calling
updateRef with live state variables (autoUpdateMethod, autoUpdateCustomVersion,
peerExposeEnabled) causing a stale-closure reset; change the effect to read the
stable initial/saved values from the account/settings snapshot (the mount-time
values) instead of those live variables (e.g., capture initialAutoUpdateMethod,
initialAutoUpdateCustomVersion, initialPeerExposeEnabled from the account
snapshot used on mount) and pass those into updateRef along with groupIds, then
keep setPeerExposeGroups(initialGroups) as-is; finally, add the required //
eslint-disable-line react-hooks/exhaustive-deps comment to the useEffect to
silence the lint rule once only the stable snapshot values are used.
- Around line 345-374: AnimatePresence requires its direct children to have
unique key props so exit animations run; add a stable key to the wrapper div
(the direct child of AnimatePresence) that wraps the motion.div (for example use
key="peer-expose" or a value derived from peerExposeEnabled) or alternatively
make the motion.div the direct child and give it a key; update the wrapper/div
or motion.div inside ClientSettingsTab (the block rendering when
peerExposeEnabled is true) so the direct child of AnimatePresence has a unique
key to ensure the exit animation defined on the motion.div runs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/settings/ClientSettingsTab.tsx (1)
182-202:toggleLazyConnectionspreads staleaccount.settings, risking overwrite of concurrently saved fields.
toggleLazyConnectionsends{ ...account.settings, lazy_connection_enabled: toggle }— if theaccountprop hasn't been refreshed by SWR after a prior "Save Changes" (which updatesauto_update_versionandextra), those fields will be overwritten with their previous values. With the newextra.peer_expose_*fields this surface has grown.This is a pre-existing pattern, so no immediate fix is required, but worth noting: a safer approach would be to PATCH only the changed field or read from the SWR cache directly.
🤖 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 182 - 202, toggleLazyConnection currently spreads the potentially stale account.settings into saveRequest.put which can overwrite concurrently-updated fields; change it to only send the minimal patch (e.g., { lazy_connection_enabled: toggle }) or else read the latest account data from the SWR cache before merging so you don't clobber fields like auto_update_version or extra.*; update the call sites in toggleLazyConnection (saveRequest.put) and keep setLazyConnection and mutate("/accounts") behavior the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 100-113: The useEffect that runs on initialGroups intentionally
omits dependencies autoUpdateMethod, autoUpdateCustomVersion, peerExposeEnabled
and updateRef which trips react-hooks/exhaustive-deps; add an
eslint-disable-next-line comment for react-hooks/exhaustive-deps immediately
above this useEffect (the block containing useEffect(() => { if (initialGroups)
{ setPeerExposeGroups(initialGroups); ... updateRef([...]) } },
[initialGroups]);) so the linter knows the omission is intentional while keeping
the current dependency array.
- Around line 348-377: The AnimatePresence immediate child div rendered when
peerExposeEnabled is true needs a stable unique key (e.g., use
key={peerExposeEnabled ? 'peer-expose-panel' : 'peer-expose-panel-hidden'} or a
constant string) so Framer Motion can track enter/exit; also prevent editing by
read-only users by passing a disabled prop to PeerGroupSelector (or
conditionally render a non-interactive view) using permission.settings.update
(e.g., disabled={!permission.settings.update}) so PeerGroupSelector
(values={peerExposeGroups}, onChange={setPeerExposeGroups}) is not interactive
for users without update permission.
---
Nitpick comments:
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 182-202: toggleLazyConnection currently spreads the potentially
stale account.settings into saveRequest.put which can overwrite
concurrently-updated fields; change it to only send the minimal patch (e.g., {
lazy_connection_enabled: toggle }) or else read the latest account data from the
SWR cache before merging so you don't clobber fields like auto_update_version or
extra.*; update the call sites in toggleLazyConnection (saveRequest.put) and
keep setLazyConnection and mutate("/accounts") behavior the same.
… to allow creating new groups, remove .patch
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/settings/ClientSettingsTab.tsx (1)
143-173:⚠️ Potential issue | 🟠 Major
saveGroups()rejection is silently swallowed — move it inside thenotifypromise.
saveGroups()is awaited beforenotify()is called. If it rejects (e.g., a network error or API failure),notify()is never invoked — no loading indicator, no error toast. The user sees nothing. Additionally, any newly created groups are persisted even if the subsequentsaveRequest.put()fails, leaving orphaned groups.🔧 Proposed fix — chain `saveGroups` inside the `notify` promise
const saveChanges = async () => { - const groups = await saveGroups(); - const peerExposeGroupIds = groups - .map((group) => group.id) - .filter(Boolean) as string[]; - notify({ title: "Client Settings", description: `Client settings successfully updated.`, - promise: saveRequest - .put({ - id: account.id, - settings: { - ...account.settings, - auto_update_version: autoUpdateCustomVersion || autoUpdateMethod, - peer_expose_enabled: peerExposeEnabled, - peer_expose_groups: peerExposeGroupIds, - }, - }) - .then(() => { - mutate("/accounts"); - updateRef([ - autoUpdateMethod, - autoUpdateCustomVersion, - peerExposeEnabled, - peerExposeGroupNames, - ]); - }), + promise: saveGroups().then((groups) => { + const peerExposeGroupIds = groups + .map((group) => group.id) + .filter(Boolean) as string[]; + return saveRequest + .put({ + id: account.id, + settings: { + ...account.settings, + auto_update_version: autoUpdateCustomVersion || autoUpdateMethod, + peer_expose_enabled: peerExposeEnabled, + peer_expose_groups: peerExposeGroupIds, + }, + }) + .then(() => { + mutate("/accounts"); + updateRef([ + autoUpdateMethod, + autoUpdateCustomVersion, + peerExposeEnabled, + peerExposeGroupNames, + ]); + }); + }), loadingMessage: "Updating client settings...", }); };🤖 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 143 - 173, In saveChanges, don't await saveGroups() before calling notify; instead move the saveGroups() call into the notify promise chain so failures trigger notify's loading/error toasts and prevent orphaned groups if saveRequest.put fails: inside notify.promise, call saveGroups().then(groups => { const peerExposeGroupIds = groups.map(g=>g.id).filter(Boolean); return saveRequest.put({ id: account.id, settings: { ...account.settings, auto_update_version: autoUpdateCustomVersion || autoUpdateMethod, peer_expose_enabled: peerExposeEnabled, peer_expose_groups: peerExposeGroupIds } }); }).then(() => { mutate("/accounts"); updateRef([autoUpdateMethod, autoUpdateCustomVersion, peerExposeEnabled, peerExposeGroupNames]); }); ensure you return the combined promise from notify.promise so its loading/error states reflect both saveGroups and saveRequest.put and remove the earlier top-level await of saveGroups().
♻️ Duplicate comments (1)
src/modules/settings/ClientSettingsTab.tsx (1)
303-323:⚠️ Potential issue | 🟡 Minor
PeerGroupSelectorlacks adisabledguard for both the permission check and keyboard interaction.Two related issues:
Keyboard accessibility: The outer
divusespointer-events-nonewhen!peerExposeEnabled, which blocks mouse/touch events but does not prevent keyboard users from tabbing into and interacting withPeerGroupSelector. Consider addinginertto the container div (ortabIndex={-1}on focusable descendants) to fully block interaction.Permission guard:
FancyToggleSwitchis correctly gated withdisabled={!permission.settings.update}, butPeerGroupSelectorhas no equivalent guard — users lacking update permission can still interact with the selector (save is blocked, but the interactive UI is misleading).✨ Proposed fix
<PeerGroupSelector values={peerExposeGroups} onChange={setPeerExposeGroups} placeholder="Select peer groups..." + disabled={!peerExposeEnabled || !permission.settings.update} />🤖 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 303 - 323, The PeerGroupSelector can still be focused/used via keyboard and isn't permission-gated; update the container and component props so it is fully inert when interaction should be blocked: when !peerExposeEnabled or !permission.settings.update, pass a disabled prop to PeerGroupSelector (e.g. values={peerExposeGroups} onChange={setPeerExposeGroups} -> disabled={!peerExposeEnabled || !permission.settings.update}) and make the outer div non-focusable for keyboard users by adding inert (or aria-hidden plus forcing tabIndex={-1} on focusable children) so tabbing cannot reach PeerGroupSelector; mirror the existing FancyToggleSwitch permission check to ensure users without update permission cannot interact with the selector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 143-173: In saveChanges, don't await saveGroups() before calling
notify; instead move the saveGroups() call into the notify promise chain so
failures trigger notify's loading/error toasts and prevent orphaned groups if
saveRequest.put fails: inside notify.promise, call saveGroups().then(groups => {
const peerExposeGroupIds = groups.map(g=>g.id).filter(Boolean); return
saveRequest.put({ id: account.id, settings: { ...account.settings,
auto_update_version: autoUpdateCustomVersion || autoUpdateMethod,
peer_expose_enabled: peerExposeEnabled, peer_expose_groups: peerExposeGroupIds }
}); }).then(() => { mutate("/accounts"); updateRef([autoUpdateMethod,
autoUpdateCustomVersion, peerExposeEnabled, peerExposeGroupNames]); }); ensure
you return the combined promise from notify.promise so its loading/error states
reflect both saveGroups and saveRequest.put and remove the earlier top-level
await of saveGroups().
---
Duplicate comments:
In `@src/modules/settings/ClientSettingsTab.tsx`:
- Around line 303-323: The PeerGroupSelector can still be focused/used via
keyboard and isn't permission-gated; update the container and component props so
it is fully inert when interaction should be blocked: when !peerExposeEnabled or
!permission.settings.update, pass a disabled prop to PeerGroupSelector (e.g.
values={peerExposeGroups} onChange={setPeerExposeGroups} ->
disabled={!peerExposeEnabled || !permission.settings.update}) and make the outer
div non-focusable for keyboard users by adding inert (or aria-hidden plus
forcing tabIndex={-1} on focusable children) so tabbing cannot reach
PeerGroupSelector; mirror the existing FancyToggleSwitch permission check to
ensure users without update permission cannot interact with the selector.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/skeletons/SkeletonSettings.tsxsrc/interfaces/Account.tssrc/modules/settings/ClientSettingsTab.tsx
heisbrot
approved these changes
Feb 24, 2026
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
Changes
Settings - Peer Expose in Client Settings Tab
Activity Log
Summary by CodeRabbit
Release Notes
New Features
Improvements