Conversation
…escriptions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds DNS Zones & Records management (types, provider, CRUD, tables, modals, icon), refactors DNS module paths, integrates zones into navigation and group details, and introduces Peer Expose settings and activity-log rendering for peer expose events. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as "DNS Zones UI"
participant Provider as "DNSZonesProvider"
participant API as "Backend API"
participant Modal as "DNSZone/DNSRecord Modal"
User->>UI: open Zones page
UI->>Provider: useDNSZones()/init
Provider->>API: GET /zones
API-->>Provider: zones list
Provider-->>UI: supply zones data
UI->>User: render zones table
User->>UI: click Create/Edit
UI->>Modal: open modal
User->>Modal: submit form
Modal->>Provider: create/update zone or record
Provider->>API: POST/PUT /zones or /records
API-->>Provider: created/updated entity
Provider-->>UI: refresh list
UI->>User: show updated data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
| +export const DNS_ZONE_DOCS_LINK = "https://docs.netbird.io/manage/dns/zones"; | ||
| +export const DNS_RECORDS_DOCS_LINK = "https://docs.netbird.io/manage/dns/zones"; |
There was a problem hiding this comment.
DNS_RECORDS_DOCS_LINK has the same URL as DNS_ZONE_DOCS_LINK.
Both constants point to "https://docs.netbird.io/manage/dns/zones". DNS_RECORDS_DOCS_LINK should likely point to a more specific URL for DNS records documentation (e.g., https://docs.netbird.io/manage/dns/zones#records or a separate page).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@custom-zones.patch` around lines 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.
| + const zonesCount = zonesGroups?.filter((zone) => { | ||
| + return zone.includes(group.id as string); | ||
| + }).length; |
There was a problem hiding this comment.
zonesCount can be undefined, but zones_count is typed as number.
When zonesGroups is undefined (during loading), zonesGroups?.filter(...) returns undefined, and .length is never called. This means zonesCount is undefined, which gets assigned to zones_count: number — a type mismatch.
Suggested fix
const zonesCount = zonesGroups?.filter((zone) => {
return zone.includes(group.id as string);
- }).length;
+ }).length ?? 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.
| + const zonesCount = zonesGroups?.filter((zone) => { | |
| + return zone.includes(group.id as string); | |
| + }).length; | |
| const zonesCount = zonesGroups?.filter((zone) => { | |
| return zone.includes(group.id as string); | |
| }).length ?? 0; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@custom-zones.patch` around lines 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.
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.
| React.useEffect(() => { | ||
| if (initialGroups) { | ||
| setPeerExposeGroups(initialGroups); | ||
| const groupIds = initialGroups | ||
| .map((g) => g.id) | ||
| .filter(Boolean) as string[]; | ||
| updateRef([ | ||
| autoUpdateMethod, | ||
| autoUpdateCustomVersion, | ||
| peerExposeEnabled, | ||
| groupIds, | ||
| ]); | ||
| } | ||
| }, [initialGroups]); |
There was a problem hiding this comment.
Missing eslint-disable comment may cause lint CI failure.
autoUpdateMethod, autoUpdateCustomVersion, peerExposeEnabled, and updateRef are used inside the effect but intentionally omitted from the dependency array (adding them would reset the baseline on every user interaction). Without a suppression comment, react-hooks/exhaustive-deps will warn or error depending on the project's ESLint config.
🔧 Proposed fix
- }, [initialGroups]);
+ }, [initialGroups]); // eslint-disable-line react-hooks/exhaustive-deps📝 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.
| React.useEffect(() => { | |
| if (initialGroups) { | |
| setPeerExposeGroups(initialGroups); | |
| const groupIds = initialGroups | |
| .map((g) => g.id) | |
| .filter(Boolean) as string[]; | |
| updateRef([ | |
| autoUpdateMethod, | |
| autoUpdateCustomVersion, | |
| peerExposeEnabled, | |
| groupIds, | |
| ]); | |
| } | |
| }, [initialGroups]); | |
| React.useEffect(() => { | |
| if (initialGroups) { | |
| setPeerExposeGroups(initialGroups); | |
| const groupIds = initialGroups | |
| .map((g) => g.id) | |
| .filter(Boolean) as string[]; | |
| updateRef([ | |
| autoUpdateMethod, | |
| autoUpdateCustomVersion, | |
| peerExposeEnabled, | |
| groupIds, | |
| ]); | |
| } | |
| }, [initialGroups]); // eslint-disable-line react-hooks/exhaustive-deps |
🤖 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 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.
| <AnimatePresence> | ||
| {peerExposeEnabled && ( | ||
| <div className={"overflow-hidden -top-4 relative z-0"}> | ||
| <motion.div | ||
| initial={{ opacity: 0, height: 0, scale: 0.98 }} | ||
| animate={{ opacity: 1, height: "auto", scale: 1 }} | ||
| exit={{ opacity: 0, height: 0, scale: 0.98 }} | ||
| > | ||
| <div | ||
| className={ | ||
| "flex flex-col gap-6 bg-nb-gray-940 px-6 pt-5 pb-6 border border-nb-gray-930 rounded-b-md relative mx-3" | ||
| } | ||
| > | ||
| <div> | ||
| <Label>Allowed peer groups</Label> | ||
| <HelpText> | ||
| Restrict which peer groups are allowed to expose | ||
| services. Leave empty to allow all peers. | ||
| </HelpText> | ||
| <PeerGroupSelector | ||
| values={peerExposeGroups} | ||
| onChange={setPeerExposeGroups} | ||
| placeholder="Select peer groups..." | ||
| /> | ||
| </div> | ||
| </div> | ||
| </motion.div> | ||
| </div> | ||
| )} | ||
| </AnimatePresence> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Framer Motion AnimatePresence key prop required direct children documentation
💡 Result:
Framer Motion’s AnimatePresence docs explicitly state that its immediate (direct) children must each have a unique React key prop so AnimatePresence can track whether each child is entering/exiting the React tree. (motion.dev)
The same page reiterates this in Troubleshooting → “Exit animations aren’t working”: ensure all immediate children have a unique, stable key (and avoid using array indexes as keys if items can reorder). (motion.dev)
Citations:
- 1: https://motion.dev/motion/animate-presence/?utm_source=openai
- 2: https://motion.dev/motion/animate-presence/?utm_source=openai
🏁 Script executed:
# Find and examine the ClientSettingsTab.tsx file
find . -name "ClientSettingsTab.tsx" -type fRepository: netbirdio/dashboard
Length of output: 107
🏁 Script executed:
# Read the ClientSettingsTab.tsx file around lines 338-367
sed -n '335,370p' ./src/modules/settings/ClientSettingsTab.tsxRepository: netbirdio/dashboard
Length of output: 1543
🏁 Script executed:
# Verify the broader usage context - check if there are similar AnimatePresence patterns
# to understand if this is a pattern issue across the file
rg "AnimatePresence" ./src/modules/settings/ClientSettingsTab.tsx -A 5Repository: netbirdio/dashboard
Length of output: 588
Add key prop to the direct child of AnimatePresence.
Framer Motion's official documentation explicitly requires that all immediate (direct) children of AnimatePresence must have a unique React key prop so AnimatePresence can track whether each child is entering or exiting the React tree. The wrapper <div> at line 340 is missing this required key. While the exit animation on the nested motion.div may work in practice due to context propagation, omitting the key violates the documented API contract and is not guaranteed to work reliably across different scenarios.
✨ Proposed fix
- <div className={"overflow-hidden -top-4 relative z-0"}>
+ <div key="peer-expose-groups" className={"overflow-hidden -top-4 relative z-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.
| <AnimatePresence> | |
| {peerExposeEnabled && ( | |
| <div className={"overflow-hidden -top-4 relative z-0"}> | |
| <motion.div | |
| initial={{ opacity: 0, height: 0, scale: 0.98 }} | |
| animate={{ opacity: 1, height: "auto", scale: 1 }} | |
| exit={{ opacity: 0, height: 0, scale: 0.98 }} | |
| > | |
| <div | |
| className={ | |
| "flex flex-col gap-6 bg-nb-gray-940 px-6 pt-5 pb-6 border border-nb-gray-930 rounded-b-md relative mx-3" | |
| } | |
| > | |
| <div> | |
| <Label>Allowed peer groups</Label> | |
| <HelpText> | |
| Restrict which peer groups are allowed to expose | |
| services. Leave empty to allow all peers. | |
| </HelpText> | |
| <PeerGroupSelector | |
| values={peerExposeGroups} | |
| onChange={setPeerExposeGroups} | |
| placeholder="Select peer groups..." | |
| /> | |
| </div> | |
| </div> | |
| </motion.div> | |
| </div> | |
| )} | |
| </AnimatePresence> | |
| <AnimatePresence> | |
| {peerExposeEnabled && ( | |
| <div key="peer-expose-groups" className={"overflow-hidden -top-4 relative z-0"}> | |
| <motion.div | |
| initial={{ opacity: 0, height: 0, scale: 0.98 }} | |
| animate={{ opacity: 1, height: "auto", scale: 1 }} | |
| exit={{ opacity: 0, height: 0, scale: 0.98 }} | |
| > | |
| <div | |
| className={ | |
| "flex flex-col gap-6 bg-nb-gray-940 px-6 pt-5 pb-6 border border-nb-gray-930 rounded-b-md relative mx-3" | |
| } | |
| > | |
| <div> | |
| <Label>Allowed peer groups</Label> | |
| <HelpText> | |
| Restrict which peer groups are allowed to expose | |
| services. Leave empty to allow all peers. | |
| </HelpText> | |
| <PeerGroupSelector | |
| values={peerExposeGroups} | |
| onChange={setPeerExposeGroups} | |
| placeholder="Select peer groups..." | |
| /> | |
| </div> | |
| </div> | |
| </motion.div> | |
| </div> | |
| )} | |
| </AnimatePresence> |
🤖 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 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.
| <PeerGroupSelector | ||
| values={peerExposeGroups} | ||
| onChange={setPeerExposeGroups} | ||
| placeholder="Select peer groups..." | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the PeerGroupSelector component definition
fd "PeerGroupSelector" --type ts --type tsxRepository: netbirdio/dashboard
Length of output: 232
🏁 Script executed:
# Search for PeerGroupSelector in the codebase
rg "PeerGroupSelector" --type ts --type tsx -lRepository: netbirdio/dashboard
Length of output: 90
🏁 Script executed:
# Examine the ClientSettingsTab.tsx file around lines 364-368
rg -n "PeerGroupSelector" src/modules/settings/ClientSettingsTab.tsx -A 5 -B 5Repository: netbirdio/dashboard
Length of output: 1096
🏁 Script executed:
# Find the PeerGroupSelector component file
find . -name "*PeerGroupSelector*" -type fRepository: netbirdio/dashboard
Length of output: 101
🏁 Script executed:
# Search for PeerGroupSelector component definition
rg "export.*PeerGroupSelector\|function PeerGroupSelector\|const PeerGroupSelector" -A 20Repository: netbirdio/dashboard
Length of output: 45
🏁 Script executed:
# Check FancyToggleSwitch at line 342 in ClientSettingsTab.tsx
sed -n '340,345p' src/modules/settings/ClientSettingsTab.tsxRepository: netbirdio/dashboard
Length of output: 265
🏁 Script executed:
# Read the PeerGroupSelector component file
cat -n src/components/PeerGroupSelector.tsxRepository: netbirdio/dashboard
Length of output: 39132
PeerGroupSelector is interactive for read-only users — missing disabled guard.
FancyToggleSwitch correctly uses disabled={!permission.settings.update} (line 342), but PeerGroupSelector (lines 364–368) has no equivalent guard. Users lacking settings.update permission can still interact with the selector; the Save button prevents persistence, but the interactive UI is misleading.
Proposed fix
<PeerGroupSelector
values={peerExposeGroups}
onChange={setPeerExposeGroups}
placeholder="Select peer groups..."
+ disabled={!permission.settings.update}
/>📝 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.
| <PeerGroupSelector | |
| values={peerExposeGroups} | |
| onChange={setPeerExposeGroups} | |
| placeholder="Select peer groups..." | |
| /> | |
| <PeerGroupSelector | |
| values={peerExposeGroups} | |
| onChange={setPeerExposeGroups} | |
| placeholder="Select peer groups..." | |
| disabled={!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 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.
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.
1 participant
Summary
Changes
Settings - Peer Expose in Client Settings Tab
Activity Log
Summary by CodeRabbit
New Features
Activity
Navigation
Refactor
Style