Conversation
|
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:
📝 WalkthroughWalkthroughAdds a large Reverse Proxy feature (types, provider, pages, tables, modals, events), ServerPagination and useUrlTab hooks, Sonner toast migration, many new UI components/icons, Peer/Network page reorganizations with providers and tabs, assorted component API changes, styling tweaks, and removals/cleanups. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ReverseProxyModal
participant Provider as ReverseProxiesProvider
participant API as NetBird API
participant TargetModal as ReverseProxyTargetModal
participant AuthModal as Auth Modal
User->>UI: open modal / edit proxy
UI->>Provider: request domains/proxies (useReverseProxies)
Provider->>API: GET /reverse-proxies, /domains
API-->>Provider: return data
Provider-->>UI: provide domains/proxies
User->>TargetModal: add/edit target
TargetModal->>API: validate/resolve peers/resources
API-->>TargetModal: validation/result
User->>AuthModal: configure auth
AuthModal-->>UI: auth config
User->>UI: submit proxy
UI->>Provider: handleCreateOrUpdateProxy(payload)
Provider->>API: POST/PUT /reverse-proxies
API-->>Provider: success
Provider->>Provider: mutate() refresh cache
Provider-->>UI: onSuccess -> close modal
UI->>User: show notification (sonner)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/(dashboard)/settings/page.tsx (1)
58-64:⚠️ Potential issue | 🔴 CriticalInconsistent optional chaining — Line 93 will throw if
identity_providersis undefined.Line 59 was updated to use
permission?.identity_providers?.read, but Line 93 still usespermission.identity_providers.readwithout optional chaining. Ifidentity_providersis undefined, the tab trigger is safely hidden but the content render on Line 93 will throw aTypeError.🐛 Proposed fix
{account?.settings?.embedded_idp_enabled && - permission.identity_providers.read && <IdentityProvidersTab />} + permission?.identity_providers?.read && <IdentityProvidersTab />}Also applies to: 92-93
src/modules/networks/resources/NetworkResourceModal.tsx (1)
92-107:⚠️ Potential issue | 🟠 MajorSilent group loss when
saveGroups()fails.If
saveGroups()returns a falsy value due to an error (e.g., network failure while persisting a newly created group), the resource is still created withgroups: undefined, silently discarding the user's group selection. The user sees a success toast but their groups are gone.Consider distinguishing between "user selected no groups" and "saving groups failed" — e.g., return early or show an error when
saveGroups()fails unexpectedly. The same applies toupdateResourceon Line 120.src/components/DeviceCard.tsx (1)
40-41:⚠️ Potential issue | 🔴 CriticalBoth icons render simultaneously in the same 36×36px container when
deviceandresource.typeare both present.
ResourceNodepasses bothresourceanddevice(peer) props toDeviceCard. Since lines 40–41 are independent conditionals and the icon container is onlyh-9 w-9(36×36 pixels), bothPeerOSIconandResourceIconwill render and visually overlap when both props are truthy. If they should be mutually exclusive, refactor to use a conditional ternary. Otherwise, add a comment clarifying that this is intentional and document the expected UI behavior.Proposed fix (if mutually exclusive)
- {device && <PeerOSIcon os={device.os} />} - {resource?.type && <ResourceIcon type={resource.type} />} + {device ? <PeerOSIcon os={device.os} /> : resource?.type ? <ResourceIcon type={resource.type} /> : null}
🤖 Fix all issues with AI agents
In `@src/app/globals.css`:
- Around line 186-189: The --scale formula in the
[data-sonner-toast][data-expanded="false"][data-front="false"] rule is inverted
and produces negative values (causing flipped toasts); update the --scale
calculation used in that selector (the --scale custom property) from
calc(var(--toasts-before) * 0.03 - 1) to calc(1 - var(--toasts-before) * 0.03)
(preserve the !important) so stacked toasts shrink progressively with positive
scale values.
In `@src/modules/networks/routing-peers/NetworkRoutingPeersTabContent.tsx`:
- Around line 29-30: The user lookup incorrectly uses router.peer (a peer ID)
instead of the resolved peer's user ID; update the users search to use the found
peer's user_id (i.e., after const peer = peers?.find(...), change
users?.find((user) => user.id === router.peer) to users?.find((user) => user.id
=== peer?.user_id)), and ensure you handle the case where peer may be undefined
so the user variable becomes undefined when no peer is found.
In `@src/modules/reverse-proxy/events/ReverseProxyEventsLocationIpCell.tsx`:
- Around line 77-83: The CopyToClipboardText component in
ReverseProxyEventsLocationIpCell is sending the literal notification string as
the clipboard payload because its message prop is used as the value written to
clipboard; change the prop so the actual IP is copied by passing event.source_ip
(or the equivalent value shown in the span) as the clipboard payload while still
showing the user-friendly notification text via a separate prop or internal UI;
locate the CopyToClipboardText usage in ReverseProxyEventsLocationIpCell (and
mirror the fix applied in ReverseProxyEventsUrlCell) to ensure the component
writes event.source_ip to the clipboard and retains the notification text for
display.
- Around line 48-54: The tooltip is copying the wrong value for region; update
the CopyToClipboardText usage so it explicitly receives the region string as the
payload to copy instead of reusing another value—e.g. ensure the children remain
{region} and add the explicit copy prop your component expects (for example
textToCopy={region} or value={region}), and guard null/undefined with region ??
""; keep the message as "Region has been copied to your clipboard" on the
CopyToClipboardText component.
In `@src/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsx`:
- Around line 34-39: The CopyToClipboardText component is currently receiving
the human-readable notification string in its message prop, so that exact text
gets written to the clipboard; change the message prop to the actual URL built
from the event data (e.g., construct the URL using event.host and event.path —
e.g., `${event.host}${event.path}` or include protocol like
`https://${event.host}${event.path}` as appropriate) so copyToClipboard(message)
writes the URL, while keeping the visible notification text (like "URL has been
copied to your clipboard") shown to the user via the component's UI/props.
In `@src/modules/reverse-proxy/table/ReverseProxyDestinationCell.tsx`:
- Around line 23-27: The CopyToClipboardText component is missing its required
message prop so clicking copies undefined; update the JSX where
CopyToClipboardText is rendered (the instance wrapping the span in
ReverseProxyDestinationCell) to pass message={target.destination} (or
String(target.destination) to be safe) so the component's
copyToClipboard(message) receives the correct destination string.
🟠 Major comments (21)
src/modules/networks/NetworkProvider.tsx-106-115 (1)
106-115:⚠️ Potential issue | 🟠 MajorGuard on
resourcebeing defined before buildingdestinationResource.
openPolicyModalcan be called without aresourceparameter. Whenresourceisundefined, the code sets{ id: resource?.id, type: resource?.type }which becomes{ id: undefined, type: undefined }. This violates thePolicyRuleResourcecontract whereidis required (string, notstring | undefined), and theascast bypasses TypeScript's type checking. This can cause bugs downstream when the undefinedidis sent to API calls.Proposed fix
const openPolicyModal = (network?: Network, resource?: NetworkResource) => { const hasResourceGroups = (resource?.groups?.length || 0) > 0; setPolicyDefaultSettings({ destinationGroups: hasResourceGroups ? resource?.groups : undefined, - destinationResource: hasResourceGroups + destinationResource: hasResourceGroups ? undefined - : ({ - id: resource?.id, - type: resource?.type, - } as PolicyRuleResource), + : resource + ? ({ + id: resource.id, + type: resource.type, + } as PolicyRuleResource) + : undefined, name:src/components/SettingCard.tsx-90-92 (1)
90-92:⚠️ Potential issue | 🟠 MajorAdd explicit type declaration for
SettingCard.Itemto satisfy strict TypeScript.With
strict: truein tsconfig.json, TypeScript won't automatically infer theItemproperty on theSettingCardfunction. The direct assignment on line 90 works at runtime but violates type safety. Consumers usingSettingCard.Itemin ReverseProxyModal.tsx will see TS errors like "Property 'Item' does not exist on type…".Add an interface extension or type assertion:
interface SettingCard extends React.FC<Readonly<SettingCardProps>> { Item: typeof SettingCardItem; }Then apply it to the function or use a type assertion
as SettingCardafter the assignment.src/components/SidebarItem.tsx-42-45 (1)
42-45:⚠️ Potential issue | 🟠 MajorFix false-positive route matching in collapsible item detection.
The
path.startsWith(href)check inhasActiveChildcauses unintended matches with similarly-named sibling routes. For example, with routes like/networksand/network-routes, navigating to/network-routeswould incorrectly expand the/networkscollapsible item. The component has anexactPathMatchprop (line 20) used elsewhere but not for collapsible items. Update the logic to check path boundaries:const hasActiveChild = useMemo(() => { if (!collapsible || !href) return false; return path === href || path.startsWith(href + '/'); }, [collapsible, href, path]);src/modules/reverse-proxy/auth/AuthPinModal.tsx-25-26 (1)
25-26:⚠️ Potential issue | 🟠 MajorStale state when modal reopens without remounting.
useState(currentPin)anduseState(isEnabled && currentPin === "")only initialize on the first mount. If the parent keepsAuthPinModalmounted and togglesopen, subsequent opens will show the previous session'spinandisMaskedvalues instead of reinitializing from the newcurrentPin/isEnabledprops.🐛 Proposed fix — sync state when modal opens
+import React, { useState, useEffect } from "react"; export default function AuthPinModal({ open, onOpenChange, currentPin, isEnabled, onSave, onRemove, }: Readonly<Props>) { const [pin, setPin] = useState(currentPin); const [isMasked, setIsMasked] = useState(isEnabled && currentPin === ""); - const isEditing = isEnabled; + + useEffect(() => { + if (open) { + setPin(currentPin); + setIsMasked(isEnabled && currentPin === ""); + } + }, [open, currentPin, isEnabled]); + + const isEditing = isEnabled;src/modules/reverse-proxy/auth/AuthSSOModal.tsx-24-25 (1)
24-25:⚠️ Potential issue | 🟠 MajorStale local state when
currentGroupsprop changes.
useState(currentGroups)only uses the prop as the initial value. If the modal is kept mounted andcurrentGroupschanges between opens (e.g., groups are added/removed externally), the localgroupsstate will be stale. Sync it on open:🐛 Proposed fix
+import React, { useEffect, useState } from "react"; -import React, { useState } from "react"; ... const [groups, setGroups] = useState<Group[]>(currentGroups); + + useEffect(() => { + if (open) { + setGroups(currentGroups); + } + }, [open, currentGroups]);Alternatively, ensure the parent remounts this component (via a
keyprop) each time the modal opens.src/contexts/ReverseProxiesProvider.tsx-583-591 (1)
583-591:⚠️ Potential issue | 🟠 Major
sanitizeTargetsremoveshostonly for non-SUBNET targets but doesn't strip frontend-onlydestinationfield.The
destinationfield onReverseProxyTargetis annotated as// Frontendin the interface. Sending it to the API could cause unexpected behavior if the backend doesn't expect it.(See the related comment on
handleToggleabove for the proposed fix.)src/contexts/ReverseProxiesProvider.tsx-303-330 (1)
303-330:⚠️ Potential issue | 🟠 MajorInconsistent target identity matching across operations.
handleSaveTarget(Lines 194-198) matches targets by four fields:path,host,port, andprotocol. However,handleToggleTarget(Lines 308-311) andhandleDeleteTarget(Lines 382-385) match by only three fields:path,host,port— missingprotocol.If two targets share the same path/host/port but differ in protocol (e.g., HTTP vs HTTPS), toggle/delete would affect both.
Proposed fix for handleToggleTarget
const isSameTarget = t.path === target.path && t.host === target.host && - t.port === target.port; + t.port === target.port && + t.protocol === target.protocol;Proposed fix for handleDeleteTarget
const isSameTarget = t.path === target.path && t.host === target.host && - t.port === target.port; + t.port === target.port && + t.protocol === target.protocol;Also applies to: 356-405, 191-200
src/contexts/ReverseProxiesProvider.tsx-277-301 (1)
277-301:⚠️ Potential issue | 🟠 Major
handleTogglesends the entire proxy object in the PUT request.
request.put({ ...proxy, enabled: newEnabled, targets: sanitizeTargets(proxy.targets) })spreads all proxy fields (including frontend-only fields likedestinationon targets) into the API request. ThesanitizeTargetsstripshostfrom non-SUBNET targets, but doesn't strip thedestinationfield which is marked as// Frontendin the interface.Proposed fix in sanitizeTargets
export function sanitizeTargets( targets: ReverseProxyTarget[], ): ReverseProxyTarget[] { return targets.map((t) => { - if (t.target_type === ReversProxyTargetType.SUBNET) return t; - const { host: _, ...rest } = t; + const { destination: _, ...withoutDestination } = t; + if (t.target_type === ReversProxyTargetType.SUBNET) return withoutDestination as ReverseProxyTarget; + const { host: _h, ...rest } = withoutDestination; return rest as ReverseProxyTarget; }); }src/modules/reverse-proxy/ReverseProxyModal.tsx-218-220 (1)
218-220:⚠️ Potential issue | 🟠 Major
linkAuthEnabledhas no UI control in the Authentication tab despite being used in auth logic.
linkAuthEnabledis initialized fromreverseProxy?.auth?.link_auth?.enabled, included in thehasNoAuthcheck (line 279), and sent in the auth payload (lines 311-313). However, the Authentication tab (lines 569-607) only provides UI controls for three auth methods (SSO, Password, PIN Code)—no toggle exists for link auth.This creates an inconsistency:
- Existing proxies with link auth enabled will preserve that state but users cannot disable it.
- New proxies can never enable link auth through the UI.
Since
link_authis defined in theReverseProxyAuthinterface and already tracked in auth counts (seeReverseProxyAuthCell), either add aSettingCard.Itemfor link auth or add a comment explaining why it's intentionally omitted.src/interfaces/ReverseProxy.ts-14-18 (1)
14-18:⚠️ Potential issue | 🟠 Major
created_atandcertificate_issued_attyped asDatebut JSON deserialization yieldsstring.Unless a custom deserializer is in place,
useFetchApi/fetchwill parse these as strings from JSON. Typing them asDatewill mislead consumers into calling.getTime()etc., which will fail at runtime.🐛 Proposed fix
export interface ReverseProxyMeta { - created_at: Date; + created_at: string; status: ReverseProxyStatus; - certificate_issued_at?: Date; + certificate_issued_at?: string; }src/interfaces/ReverseProxy.ts-73-78 (1)
73-78:⚠️ Potential issue | 🟠 MajorFix typo in enum name:
ReversProxyTargetType→ReverseProxyTargetType.Missing the 'e' in "Reverse". This enum is already used across 5 files (30+ references), so correcting it now is critical to avoid greater disruption later. Requires coordinated updates in:
- src/interfaces/ReverseProxy.ts (definition)
- src/contexts/ReverseProxiesProvider.tsx
- src/modules/reverse-proxy/table/ReverseProxyTable.tsx
- src/modules/reverse-proxy/targets/ReverseProxyTargetDevice.tsx
- src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx
🐛 Proposed fix
-export enum ReversProxyTargetType { +export enum ReverseProxyTargetType { PEER = "peer", HOST = "host", DOMAIN = "domain", SUBNET = "subnet", }src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx-44-45 (1)
44-45:⚠️ Potential issue | 🟠 MajorUpdate permission check to use
services.createinstead ofroutes.createfor consistency with target edit/delete operations.The "Add Target" button in ReverseProxyTargetsCell uses
permission?.routes?.create, but editing and deleting targets both usepermission?.services?.updateandpermission?.services?.delete. Additionally, the equivalent functionality in ReverseProxyFlatTargetsTable correctly usespermission?.services?.create. Theservicesmodule is the dedicated permission scope for reverse proxy operations, whileroutesis shared with network routes and exit nodes. Align this button to usepermission?.services?.createto maintain consistent permission checks across all target operations.src/interfaces/ReverseProxy.ts-106-116 (1)
106-116:⚠️ Potential issue | 🟠 MajorFix incorrect documentation URL path and map each constant to distinct subsections.
The URL path
/how-to/reverse-proxyappears to be incorrect—the actual NetBird docs use/selfhosted/reverse-proxy. Additionally, the documentation page has subsections (Quick Setup, Configuration Templates, Troubleshooting, etc.) that these constants should reference distinctly rather than all pointing to the same URL:
REVERSE_PROXY_DOCS_LINK→ General reverse-proxy pageREVERSE_PROXY_CLUSTERS_DOCS_LINK→ Relevant subsection for clustersREVERSE_PROXY_CUSTOM_DOMAINS_DOCS_LINK→ Relevant subsection for custom domainsREVERSE_PROXY_DOMAIN_VERIFICATION_LINK→ Relevant subsection for domain verificationsrc/modules/reverse-proxy/targets/ReverseProxyTargetActiveCell.tsx-11-31 (1)
11-31:⚠️ Potential issue | 🟠 MajorMissing permission gate — inconsistent with
ReverseProxyActiveCell.
ReverseProxyActiveCell(the proxy-level toggle) disables the switch when!permission?.services?.update, but this target-level toggle has no such guard. A user without update permission can still toggle individual targets.Proposed fix
export default function ReverseProxyTargetActiveCell({ target, }: Readonly<Props>) { const reverseProxy = useReverseProxyTarget(); const { handleToggleTarget } = useReverseProxies(); + const { permission } = usePermissions(); return ( <div onClick={(e) => { e.stopPropagation(); }} > <ToggleSwitch + disabled={!permission?.services?.update} checked={target.enabled !== false} size={"small"} onCheckedChange={() => { handleToggleTarget(reverseProxy, target); }} /> </div> ); }You'll also need to add the import:
import { usePermissions } from "@/contexts/PermissionsProvider";src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx-229-248 (1)
229-248:⚠️ Potential issue | 🟠 MajorBug: editing a disabled target silently re-enables it.
enabledis hardcoded totruein the save payload (Line 243). When a user edits a disabled target (e.g., changing its port), saving will flip it back to enabled without any indication.Proposed fix
const targetData: ReverseProxyTarget = { target_type: resolvedType, target_id: resolvedType === ReversProxyTargetType.PEER ? targetPeerId : targetResourceId, protocol: targetProtocol, host: resolvedType === ReversProxyTargetType.SUBNET ? targetHost : undefined, port: targetPort, path: targetPath || undefined, - enabled: true, + enabled: currentTarget?.enabled ?? true, access_local: isResource ? accessLocal : undefined, };src/app/(dashboard)/reverse-proxy/page.tsx-10-12 (1)
10-12:⚠️ Potential issue | 🟠 MajorUse
router.replaceinstead ofrouter.pushto avoid a redirect loop on back navigation.
router.pushadds/reverse-proxyto the browser history stack. When users press the back button, they'll land on/reverse-proxyagain, which immediately redirects forward — creating an effectively broken back button.Proposed fix
useEffect(() => { - router.push("/reverse-proxy/services"); + router.replace("/reverse-proxy/services"); }, [router]);src/modules/networks/resources/ResourceExposeServiceCell.tsx-67-67 (1)
67-67:⚠️ Potential issue | 🟠 MajorWrong permission check — should be
services?.create, notroutes?.create.This button creates a reverse proxy service, but the disabled check uses
permission.routes?.create. Other reverse proxy UI in this PR (e.g.,ReverseProxyFlatTargetsTableline 196) correctly checkspermission?.services?.create. This mismatch could grant or deny access based on the wrong capability.🐛 Proposed fix
- disabled={!permission.routes?.create} + disabled={!permission?.services?.create}src/app/(dashboard)/peer/page.tsx-304-314 (1)
304-314:⚠️ Potential issue | 🟠 MajorSave button checks
peers.readinstead ofpeers.update.Line 310 disables the Save button when
!permission.peers.read, but the intent is to prevent saving when the user lacks update permission. TheupdatePeerfunction (Line 189) already branches onpermission.peers.update, so the button guard should match:🐛 Proposed fix
disabled={ !hasChanges || - !permission.peers.read || + !permission.peers.update || !permission.groups.update }src/modules/reverse-proxy/table/ReverseProxyTable.tsx-214-214 (1)
214-214:⚠️ Potential issue | 🟠 MajorFallback to example data in production is likely unintended.
When
reverseProxiesisundefined(e.g., still loading or no data), the table renders hardcoded example entries instead of showing the empty-stategetStartedCard. ThegetStartedCardwill never display becausedataalways has entries due to the fallback.Consider using
reverseProxiesdirectly and letting the empty state handle the no-data case:🐛 Proposed fix
- const data = reverseProxies ?? exampleReverseProxies; + const data = reverseProxies;If the example data is intended only for development, gate it behind a
process.env.NODE_ENV === 'development'check, or remove it entirely.src/modules/reverse-proxy/table/ReverseProxyTable.tsx-104-192 (1)
104-192:⚠️ Potential issue | 🟠 MajorHardcoded credentials in example data shipped to the client.
This example dataset contains plaintext secrets (
password: "secret",pin: "1234") and will be bundled into the client-side JavaScript. Even as demo data, this sets a poor precedent and could confuse users or leak into production UI.🛡️ Suggested fix: remove sensitive fields from example data
auth: { - password_auth: { enabled: true, password: "secret" }, + password_auth: { enabled: true }, },auth: { - bearer_auth: { enabled: true, distribution_groups: ["developers"] }, - pin_auth: { enabled: true, pin: "1234" }, + bearer_auth: { enabled: true, distribution_groups: ["developers"] }, + pin_auth: { enabled: true }, },src/app/(dashboard)/peer/page.tsx-361-369 (1)
361-369:⚠️ Potential issue | 🟠 MajorUse
permission.services.readinstead ofpermission.routes.readfor the reverse-proxies tab.The reverse-proxies tab is inconsistently gated on
permission.routes.readwhile all other reverse-proxy UIs consistently usepermission.services.*(ReverseProxyTable.tsx line 259, ReverseProxyActionCell.tsx lines 44 & 58, ReverseProxyActiveCell.tsx line 20, etc.). Thepermission.services.readpermission is already defined and used throughout the codebase (Navigation.tsx, reverse-proxy/services/page.tsx, reverse-proxy/custom-domains/page.tsx, events/proxy/page.tsx). Align this tab with the established permission pattern.
🟡 Minor comments (18)
src/components/Input.tsx-130-131 (1)
130-131:⚠️ Potential issue | 🟡 MinorMissing
dark:prefix on readOnly styles — breaks light mode.Every other usage of
nb-gray-*tokens in this file is scoped behind thedark:variant (e.g.,dark:bg-nb-gray-900,dark:border-nb-gray-700). These classes will apply dark-themed colors unconditionally, including in light mode.Proposed fix
- props.readOnly && - "!bg-nb-gray-920 text-nb-gray-400 !border-nb-gray-800", + props.readOnly && + "dark:!bg-nb-gray-920 dark:text-nb-gray-400 dark:!border-nb-gray-800",src/modules/peer/PeerNetworkRoutesSection.tsx-48-51 (1)
48-51:⚠️ Potential issue | 🟡 MinorComplete the
headingTargetprop removal inPeerRoutesTable.The prop was removed from the call site (lines 66-70 in
PeerNetworkRoutesSection.tsx) but not from the component definition.PeerRoutesTablestill defines, destructures, and passesheadingTargettoDataTable. Remove it from the Props type, function signature, and theDataTablecomponent call inPeerRoutesTable.tsxto fully complete the refactoring.src/components/SettingCard.tsx-25-29 (1)
25-29:⚠️ Potential issue | 🟡 MinorClickable
divlacks keyboard accessibility.The container
divacts as a button (hascursor-pointerandonClick) but lacksrole="button",tabIndex={0}, and anonKeyDownhandler for Enter/Space. Screen-reader and keyboard-only users cannot activate this item.Proposed fix
<div onClick={onClick} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + onClick(); + } + }} className={ "flex justify-between gap-10 px-6 border-t border-nb-gray-920 first:border-t-0 py-5 hover:bg-nb-gray-935 cursor-pointer transition-colors" }src/components/Breadcrumbs.tsx-48-54 (1)
48-54:⚠️ Potential issue | 🟡 MinorAccessibility: clickable
<span>lacks keyboard navigation and link semantics.Using a
<span onClick>for navigation is not accessible — it's not focusable, doesn't respond to Enter/Space, and screen readers won't announce it as a link. Consider using Next.js<Link>or at minimum an<a>tag, which also gives users right-click context menus and status bar URL previews.♻️ Suggested fix using Next.js Link
+import Link from "next/link";{href ? ( - <span onClick={() => router.push(href)} data-cy={"breadcrumb-item"}> + <Link href={href} data-cy={"breadcrumb-item"}> {label} - </span> + </Link> ) : ( label )}src/modules/reverse-proxy/table/ReverseProxyNameCell.tsx-60-62 (1)
60-62:⚠️ Potential issue | 🟡 MinorEmpty domain produces an invalid link.
When
displayDomainfalls through to the default""(Line 20), the renderedhrefbecomes"https://"— a broken URL that users can click. Consider either hiding the link when the domain is empty or rendering plain text instead.Proposed fix
- <div className="flex flex-col gap-0 dark:text-neutral-300 text-neutral-500 truncate"> - <ExternalLinkText href={`https://${displayDomain}`}> - <span className="font-medium truncate">{displayDomain}</span> - </ExternalLinkText> - </div> + <div className="flex flex-col gap-0 dark:text-neutral-300 text-neutral-500 truncate"> + {displayDomain ? ( + <ExternalLinkText href={`https://${displayDomain}`}> + <span className="font-medium truncate">{displayDomain}</span> + </ExternalLinkText> + ) : ( + <span className="font-medium truncate text-nb-gray-400">—</span> + )} + </div>src/modules/networks/routing-peers/NetworkRoutingPeersTabContent.tsx-35-39 (1)
35-39:⚠️ Potential issue | 🟡 MinorSearch string includes literal
"undefined"for missing values.When
peer,user, orgroupisundefined, template literal interpolation produces the string"undefined", which becomes searchable text. Use nullish coalescing to substitute empty strings:🐛 Proposed fix
return { ...router, - search: `${peer?.name} ${peer?.ip} ${user?.name} ${user?.id} ${group?.name}`, + search: `${peer?.name ?? ""} ${peer?.ip ?? ""} ${user?.name ?? ""} ${user?.id ?? ""} ${group?.name ?? ""}`, };src/components/Label.tsx-21-27 (1)
21-27:⚠️ Potential issue | 🟡 MinorProps are not spread onto the
divelement.When
as="div", onlyclassNameandchildrenare forwarded. Any additional props (e.g.,id,data-*,aria-*, event handlers) passed to<Label as="div" onClick={...}>are silently dropped, unlike thelabelbranch which spreads...props. If this is intentional (to avoid passing label-specific props to a div), consider at least forwarding HTML-safe props:♻️ Proposed fix
if (as === "div") { return ( - <div ref={ref as React.Ref<HTMLDivElement>} className={classes}> + <div ref={ref as React.Ref<HTMLDivElement>} className={classes} {...props}> {children} </div> ); }If label-specific props (like
htmlFor) shouldn't land on adiv, destructure them out before spreading.src/contexts/ReverseProxiesProvider.tsx-407-424 (1)
407-424:⚠️ Potential issue | 🟡 Minor
createDomainsilently swallows errors.The
catchblock returnsundefinedwithout logging or notifying the user. WhileuseApiCallmay handle error display internally, callers ofcreateDomainthat check forundefinedwon't know the difference between a network error and an intentional absence.src/modules/reverse-proxy/domain/CustomDomainModal.tsx-117-131 (1)
117-131:⚠️ Potential issue | 🟡 MinorEmpty
<HelpText></HelpText>renders an unnecessary DOM element.Line 119 renders an empty
HelpTextcomponent with no content. Either add descriptive text (e.g., "Enter the domain you want to use for your service") or remove it.Proposed fix (option: remove)
<div> <Label>Domain</Label> - <HelpText></HelpText> <Inputsrc/modules/reverse-proxy/auth/AuthPasswordModal.tsx-77-113 (1)
77-113:⚠️ Potential issue | 🟡 MinorMissing bottom padding on modal content.
The container
divat Line 52 hasclassName="px-8"but no bottom padding. The button row at Line 77 usesmt-6but there's nopb-6or similar to add spacing below the buttons before the modal edge.Proposed fix
- <div className="px-8"> + <div className="px-8 pb-6">src/modules/reverse-proxy/domain/CustomDomainVerificationModal.tsx-148-153 (1)
148-153:⚠️ Potential issue | 🟡 Minor"Start Verification" button is enabled even when no proxy cluster is available.
When
cnameTargetis empty (no proxy clusters connected), the modal shows a warning callout but the "Start Verification" button remains enabled. Clicking it would invokeonStartVerification(domain)in a state where DNS verification cannot succeed since there's no CNAME target to point to.Proposed fix
- <Button variant={"primary"} onClick={handleStartVerification}> + <Button variant={"primary"} onClick={handleStartVerification} disabled={!cnameTarget}> Start Verification </Button>src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx-23-35 (1)
23-35:⚠️ Potential issue | 🟡 MinorNo-op
onClickon a visually interactive badge is misleading.The badge has
useHover={true}andcursor-pointer, signaling to users that it's clickable, butonClick={() => void 0}does nothing. Either implement the click action (e.g., open the targets list/modal) or remove the interactive styling.♻️ If the badge shouldn't be clickable yet
<Badge variant={"gray"} - useHover={true} - className={"cursor-pointer"} - onClick={() => void 0} + useHover={false} >src/interfaces/ReverseProxy.ts-20-27 (1)
20-27:⚠️ Potential issue | 🟡 MinorTypo in enum member:
CERTIFICATED_FAILEDshould beCERTIFICATE_FAILED.The string value
"certificate_failed"is correct for the API, but the member nameCERTIFICATED_FAILEDis grammatically wrong and will be confusing in code.🐛 Proposed fix
- CERTIFICATED_FAILED = "certificate_failed", + CERTIFICATE_FAILED = "certificate_failed",src/modules/reverse-proxy/table/ReverseProxyDestinationCell.tsx-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor
destinationis optional — consider guarding againstundefinedin addition to"unknown".
ReverseProxyTarget.destinationis typed asstring | undefined. When it'sundefined(but not"unknown"), the component will render an emptyCopyToClipboardText. Consider treatingundefinedthe same as"unknown".♻️ Suggested guard
- if (target.destination === "unknown") return <EmptyRow />; + if (!target.destination || target.destination === "unknown") return <EmptyRow />;src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx-176-191 (1)
176-191:⚠️ Potential issue | 🟡 MinorPath duplicate detection uses path+host+port identity — fragile when editing.
The duplicate check skips the current target by matching on the combination of
path,host, andport. If the user changes any of these fields during edit, the old target won't be excluded, causing a false-positive duplicate warning. Consider usingtarget_idfor identity when available:Proposed fix
return existingTargets.some((t) => { - // Skip the target being edited - match by path, host, and port - if ( - currentTarget && - normalizePath(t.path) === normalizePath(currentTarget.path) && - t.host === currentTarget.host && - t.port === currentTarget.port - ) { + if ( + currentTarget && + currentTarget.target_id && + t.target_id === currentTarget.target_id + ) { return false; } return normalizePath(t.path) === normalizedCurrentPath; });src/modules/networks/resources/ResourcesTabContent.tsx-28-38 (1)
28-38:⚠️ Potential issue | 🟡 MinorMissing whitespace between "about" and the link text.
JSX collapses the newline between "Learn more about" and
<InlineLink>, resulting in "Learn more aboutResources" being rendered without a space.Proposed fix
<Paragraph> - Learn more about - <InlineLink + Learn more about{" "} + <InlineLink href={"https://docs.netbird.io/how-to/networks#resources"} target={"_blank"} > Resources <ExternalLinkIcon size={12} /> </InlineLink> + {" "} in our documentation. </Paragraph>src/modules/networks/resources/ResourceExposeServiceCell.tsx-44-44 (1)
44-44:⚠️ Potential issue | 🟡 MinorNavigation URL will contain literal
"undefined"ifnetworkis nullish.
network?.idevaluates toundefinedwhennetworkis absent, producing/network?id=undefined&tab=services&target=.... Consider guarding against this or asserting thatnetworkis always available in this context.🛡️ Proposed guard
onClick={(e) => { e.preventDefault(); e.stopPropagation(); + if (!network?.id) return; router.push(`/network?id=${network?.id}&tab=services&target=${resource.id}`); }}Apply the same guard to the
onSuccesscallback at line 63.src/modules/reverse-proxy/events/ReverseProxyEventsTable.tsx-227-262 (1)
227-262:⚠️ Potential issue | 🟡 MinorDisabled controls when events list is empty can trap users.
When all filter buttons (All / Success / Failed) and the refresh button are disabled based on
!events?.length, two problems arise:
- If a user selects "Failed" and no failed events exist,
eventsbecomes[], all buttons including "All" are disabled, and the user can't revert the filter without using the reset-filter button.- If no events exist yet (first use), the refresh button is disabled, so the user can't poll for new events.
Consider keeping at least the "All" button and the refresh button always enabled:
🐛 Proposed fix for refresh button
<DataTableRefreshButton - isDisabled={!events?.length} + isDisabled={false} onClick={() => mutate()} />
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@src/app/`(dashboard)/peer/page.tsx:
- Around line 361-369: The Reverse Proxies tab trigger (TabsTrigger value
"reverse-proxies") is shown when peer?.id && permission.services?.read, but the
corresponding TabsContent is gated by permission.routes.read causing a mismatch;
update the TabsContent (the block rendering content for value "reverse-proxies")
to use the same permission check as the trigger (permission.services?.read) and
include peer?.id if the trigger requires it so both trigger and content are
shown/hidden together (look for TabsTrigger, TabsContent,
permission.services?.read and permission.routes.read and make the condition
consistent).
In `@src/contexts/ReverseProxiesProvider.tsx`:
- Around line 569-576: The current isSameTarget function compares only path,
host, port, and protocol and can misidentify different backends; update
isSameTarget(ReverseProxyTarget) to also compare target_id when present (e.g.,
if both a.target_id and b.target_id are defined, require them to match) while
preserving the existing field comparisons (path, host, port, protocol) so
edits/toggles/deletes match the exact target_id-backed entry.
In `@src/interfaces/ReverseProxy.ts`:
- Around line 106-116: The four exported constants REVERSE_PROXY_DOCS_LINK,
REVERSE_PROXY_CLUSTERS_DOCS_LINK, REVERSE_PROXY_CUSTOM_DOMAINS_DOCS_LINK, and
REVERSE_PROXY_DOMAIN_VERIFICATION_LINK all point to the same generic URL; update
each constant to its specific documentation URL or anchor/path (e.g., add the
correct section anchors like /reverse-proxy#clusters,
/reverse-proxy#custom-domains, /reverse-proxy#domain-verification or the real
distinct doc paths) so each constant references the precise docs section it
represents.
In `@src/modules/reverse-proxy/domain/CustomDomainModal.tsx`:
- Around line 42-56: The modal keeps stale values because domain and
selectedCluster aren't cleared when the modal is reopened; add a React.useEffect
that watches the modal's open prop and resets state by calling setDomain("") and
setSelectedCluster("") (and optionally re-run the existing auto-select logic)
whenever open changes (e.g., when open becomes true or on close) so the form is
cleared; reference the domain, setDomain, selectedCluster, setSelectedCluster
state variables and the availableClusters useMemo when adding this effect.
In `@src/modules/reverse-proxy/table/ReverseProxyTable.tsx`:
- Around line 247-252: The refresh button in DataTableRefreshButton is calling
mutate("/reverse-proxies") but the data source used by ReverseProxiesProvider is
"/reverse-proxies/services", so the SWR cache key is wrong and the refetch won't
run; update the onClick handler in ReverseProxyTable (the mutate call next to
DataTableRefreshButton) to call mutate("/reverse-proxies/services") so it
targets the same key used by ReverseProxiesProvider and triggers a proper
refetch.
In `@src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx`:
- Around line 24-34: The Badge in ReverseProxyTargetsCell is styled as
interactive (useHover, cursor-pointer, onClick={() => void 0}) but the handler
is a no-op; either wire it to the real action or remove the interactive
affordances: if this should open the targets details, replace the no-op with the
real handler (e.g., call the component/row toggle such as props.onToggleTargets
or the row.toggleOpen function) and also add accessible keyboard support/role;
otherwise remove useHover, the cursor-pointer className and the onClick prop so
the Badge is non-interactive.
In `@src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx`:
- Around line 475-480: The onChange handler in ReverseProxyTargetModal currently
strips input when isHostEditable by using a regex that only allows 0-9 and dots,
which prevents entering IPv6 CIDRs; update the input sanitization in that
onChange (where setTargetHost is called) to allow IPv6 characters by permitting
0-9, a-f, A-F and colon (e.g., include a-fA-F: in the allowed character set) or
replace the naive strip-with-regex approach with a validation step that accepts
both IPv4 and IPv6 CIDR syntaxes before calling setTargetHost; ensure you update
the branch that checks isHostEditable so IPv6 hex digits and colons are
preserved.
- Around line 177-185: The duplicate-path check in the useMemo for
isPathDuplicate uses reference equality (t === currentTarget) which is brittle;
change that to compare a stable identifier instead (for example use t.target_id
=== currentTarget.target_id or, if target_id may be missing, compare a combined
key like `${t.target_id||''}:${normalizePath(t.path)}` against the same for
currentTarget). Update the predicate inside existingTargets.some to skip the
target being edited by comparing these stable ids (and guard for undefined
currentTarget or missing target_id) while keeping normalizePath(targetPath) as
the normalizedCurrentPath.
- Around line 93-99: The current useState initializer unsafely casts
initialResource.type to ReverseProxyTargetType; replace that cast with a small
mapping/validation routine (e.g., a function mapNetworkTypeToReverseProxyType)
that accepts NetworkResource.type and returns a valid ReverseProxyTargetType or
a safe default (ReverseProxyTargetType.HOST or PEER) if the value is undefined
or unknown. Locate the initialization around targetType/setTargetType and the
use of initialResource, implement the mapping function to explicitly handle
"domain"|"host"|"subnet"|undefined -> corresponding ReverseProxyTargetType
values, and use that function in place of the (initialResource.type as
ReverseProxyTargetType) cast so runtime mismatches are handled
deterministically.
- Around line 352-358: The resource prop passed into PeerGroupSelector in
ReverseProxyTargetModal currently hardcodes type: "host" for
isResourceTargetType branches; change it to use the actual targetType value so
when targetType is DOMAIN or SUBNET the resource becomes { id: targetResourceId,
type: targetType } instead of type: "host" (keep the existing branch for
ReverseProxyTargetType.PEER that uses { id: targetPeerId, type: "peer" }
unchanged); update the resource construction around
isResourceTargetType(targetType) && targetResourceId to reference targetType for
the type field so PeerGroupSelector receives the correct resource type.
🧹 Nitpick comments (11)
src/modules/posture-checks/usePostureCheck.ts (1)
117-137: Clean refactor — sharing a single promise with bothnotifyand the caller is correct.Two optional nits:
asyncon line 117 is unnecessary since noawaitis used; the function already returns aPromise. Removing it avoids wrapping the returned promise in an extraPromiselayer.- Line 123:
onSuccess?.(check)is slightly more idiomatic thanonSuccess && onSuccess(check).Suggested diff
- const updateOrCreateAndNotify = async ( + const updateOrCreateAndNotify = ( checkToUpdateOrCreate?: PostureCheck, ) => { const call = () => updateOrCreate(checkToUpdateOrCreate || state); const promise = call().then((check) => { mutate("/posture-checks"); - onSuccess && onSuccess(check); + onSuccess?.(check); return check; });src/modules/networks/NetworkProvider.tsx (1)
110-117: Remove unnecessary type assertionas PolicyRuleResource(lines 113–116).
NetworkResource.typeis a subset ofPolicyRuleResource.type("domain" | "host" | "subnet"vs."domain" | "host" | "subnet" | "peer"). The cast is safe but unnecessary; TypeScript allows the subset to be assigned to the superset without an explicit assertion. Removing the cast improves code clarity.Suggested change
destinationResource: hasResourceGroups ? undefined : resource ? ({ id: resource.id, type: resource.type, }) : undefined,src/components/SidebarItem.tsx (1)
50-54: Missingopenin useEffect dependency array will triggerreact-hooks/exhaustive-depslint warning.The omission of
openis clearly intentional (you only want to react to route changes, not re-run when the user manually toggles). Suppress the lint rule with a comment to document the intent:Suggested fix
useEffect(() => { if (hasActiveChild && !open) { setOpen(true); } + // eslint-disable-next-line react-hooks/exhaustive-deps }, [hasActiveChild]);src/modules/reverse-proxy/domain/CustomDomainModal.tsx (1)
52-52: Minor: inconsistent hook import style.
useMemoanduseStateare imported directly, butuseEffectis called asReact.useEffect. Pick one style for consistency.Suggested fix
-import { useMemo, useState } from "react"; +import { useEffect, useMemo, useState } from "react";Then replace
React.useEffectwithuseEffect.src/contexts/ReverseProxiesProvider.tsx (2)
97-101: Redundant data fetching — peers and resources are already loaded byReverseProxiesProviderbutReverseProxyTargetDevicefetches them again independently.The provider fetches
/peersand/networks/resources(Lines 98-101) for destination resolution. However,ReverseProxyTargetDevice.tsx(Lines 27-32) also callsuseFetchApifor the same endpoints plus/networks. Since SWR deduplicates by cache key, this isn't a correctness bug, but it means the provider holds peer/resource data that could be exposed through the context instead of having eachReverseProxyTargetDeviceinstance independently fetch. Consider exposingpeersandresources(and potentiallynetworks) from the context to avoid coupling child components to the same API calls.
214-224: Direct DOM manipulation (querySelector+click()) is fragile.Using
document.querySelectorwithdata-row-idto find and programmatically click accordion rows is brittle — it breaks if the table doesn't render in time (the 200ms timeout is a guess), or if the data attribute naming changes. This is a minor concern since it's a non-critical UX enhancement (auto-expand + scroll after create), but worth noting for future maintainability.Also applies to: 253-262
src/modules/reverse-proxy/table/ReverseProxyTable.tsx (1)
199-245: Use strict equality (===) instead of loose equality (==) for length checks.Multiple comparisons use
== 0(Lines 199, 205, 219, 233, 245). While functionally equivalent here sincelengthis always a number, strict equality is idiomatic TypeScript/JavaScript.src/modules/reverse-proxy/targets/ReverseProxyTargetDevice.tsx (1)
27-47: Entity lookups are not memoized —find()runs on every render.
peer,resource, andnetworkare computed viaArray.find()on every render. Since this component is rendered per-row in a table, consider wrapping these inuseMemoto avoid redundant linear scans when unrelated state changes trigger re-renders.♻️ Suggested improvement
- const peer = isPeer - ? peers?.find((p) => p.id === target.target_id) - : undefined; - const resource = isResource - ? resources?.find((r) => r.id === target.target_id) - : undefined; - - const network = - isResource && resource - ? networks?.find((n) => n.resources?.includes(resource.id)) - : undefined; + const peer = React.useMemo( + () => (isPeer ? peers?.find((p) => p.id === target.target_id) : undefined), + [isPeer, peers, target.target_id], + ); + const resource = React.useMemo( + () => (isResource ? resources?.find((r) => r.id === target.target_id) : undefined), + [isResource, resources, target.target_id], + ); + const network = React.useMemo( + () => + isResource && resource + ? networks?.find((n) => n.resources?.includes(resource.id)) + : undefined, + [isResource, resource, networks], + );src/modules/networks/resources/ResourceExposeServiceCell.tsx (1)
4-7: Consolidate imports from the same module.
useReverseProxies(line 5) andisResourceTargetType(line 7) are both imported from@/contexts/ReverseProxiesProviderin separate statements.Suggested fix
-import { useReverseProxies } from "@/contexts/ReverseProxiesProvider"; -import { NetworkResource } from "@/interfaces/Network"; -import { isResourceTargetType } from "@/contexts/ReverseProxiesProvider"; +import { + useReverseProxies, + isResourceTargetType, +} from "@/contexts/ReverseProxiesProvider"; +import { NetworkResource } from "@/interfaces/Network";src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx (2)
513-517: Port input allows negative values through keyboard entry.
parseInt(e.target.value) || 0will parse negative numbers (e.g.,-5→-5), and whileisValidPortwill correctly reject them for the save button, the input will display the negative value. Consider clamping:Suggested fix
onChange={(e) => - setTargetPort(parseInt(e.target.value) || 0) + setTargetPort(Math.max(0, parseInt(e.target.value) || 0)) }
244-245: Dead code:showAdvancedSettingsis alwaysfalse.The advanced settings section (lines 528–558) is unreachable. If this is intentional scaffolding for a future iteration, a brief comment explaining the plan would help. Otherwise, consider removing the dead code and the accordion section.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/globals.css`:
- Around line 191-196: The stylelint error flags the missing empty line before
the opacity declaration inside the toast CSS rule for the selector
[data-sonner-toast][data-removed='true'][data-front='false'][data-swipe-out='false'][data-expanded='false'];
fix it by inserting a single blank line immediately before the "opacity: 0
!important;" declaration (between the --y translateY line and the opacity line)
so the declaration-empty-line-before rule is satisfied, then run the linter to
confirm the warning is gone.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/contexts/ReverseProxiesProvider.tsx`:
- Around line 393-410: The createDomain function currently swallows all errors
by returning undefined; change the catch to capture the thrown error (e.g.,
catch (error)), log the error (console.error or the app logger) with a clear
message referencing createDomain/domainRequest.post, and then re-throw the error
(or return a structured error object) so callers can distinguish
network/validation/409 conflicts; ensure mutate("/reverse-proxies/domains")
remains only on successful post.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 221-223: The component initializes linkAuthEnabled from
reverseProxy?.auth?.link_auth?.enabled but provides no UI or save handling to
change it; update the component so the modal offers a control (e.g., a
Switch/Checkbox) bound to linkAuthEnabled and call setLinkAuthEnabled on change,
and ensure the form save/submit logic that persists auth settings includes this
value (update the same handler that writes other auth fields) so toggling
actually updates reverseProxy.auth.link_auth.enabled; alternatively, if link
auth is not supported yet, remove linkAuthEnabled from the hasNoAuth calculation
(reference hasNoAuth and linkAuthEnabled) so the "no auth" warning logic is
correct.
- Around line 310-312: The current assignment to bearer_auth.distribution_groups
uses savedGroups.map((g) => g.id as string) which unsafely casts possibly
undefined ids; change this to first filter out groups with missing ids (e.g.,
savedGroups.filter(g => g.id != null)) and then map to their ids so
distribution_groups receives only real strings; update the code referencing
bearer_auth or distribution_groups accordingly to produce a string[] without
using the unsafe "as string" cast.
In `@src/modules/reverse-proxy/table/ReverseProxyTable.tsx`:
- Around line 88-98: The search string builder (id: "searchString" in the
accessorFn) concatenates target ports with .join("") which merges numbers (e.g.,
80 and 443 -> "80443") and can produce false matches; change the join for ports
(and similarly for destination and host if needed) to use a separator such as a
space or comma (e.g., .map(t => t.port).join(" ")) so each field remains
distinct when the overall search string is created from row?.domain,
row?.proxy_cluster, and row?.targets?.map(...).
In `@src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx`:
- Around line 159-161: The isCidrRange logic currently assumes IPv4 by checking
cidrMask < 32; update it to choose the max prefix based on address family (IPv4
=> 32, IPv6 => 128) and compare cidrMask against that max so single-host
prefixes (/32 for IPv4, /128 for IPv6) are handled correctly. Detect the family
using the existing CIDR/address parsing available in this component (e.g., from
the same source that sets cidrMask or via the ip-cidr helper), compute maxMask =
isIPv6 ? 128 : 32, then set isCidrRange = cidrMask !== null && cidrMask <
maxMask and also adjust any default prefix logic (the default /32) to use the
family-aware max when choosing defaults.
🧹 Nitpick comments (12)
src/interfaces/ReverseProxy.ts (1)
42-58: Sensitive credentials stored as plain strings in the auth interface.
passwordandpinare typed as plainstring. While this is a frontend interface and the actual security depends on the API, ensure these values are never logged, persisted to localStorage, or included in error reports. Verify the backend stores them securely (hashed/encrypted).src/contexts/ReverseProxiesProvider.tsx (3)
546-557:target.target_id || ""can silently pass theincludescheck on unexpected data.If
target_idisundefined, this falls back to checkingnetwork.resources?.includes(""), which will never match — so it's safe but misleading. Consider an early return instead for clarity.♻️ Suggested improvement
if (network && !peer) { if (isResourceTargetType(target.target_type)) { + if (!target.target_id) return; const isResourceInNetwork = network.resources?.includes( - target.target_id || "", + target.target_id, ); if (!isResourceInNetwork) return;
213-224: DOM manipulation (querySelector,click,scrollIntoView) for scroll-to-row UX.Direct DOM manipulation with
querySelectorand simulatedclick()is fragile — it depends on specificdata-row-idattributes and accordiondata-accordionattribute values being present. If the table rendering changes, this will silently break. Consider using a React ref-based approach or a callback that theDataTableexposes for programmatic row expansion and scrolling.This pattern repeats in
handleCreateOrUpdateProxy(Lines 253-261) as well.
130-146: Consider consolidating modal state into a reducer or object.Eight separate
useStatecalls manage the modal lifecycle. A singleuseReduceror a state object (e.g.,modalState: { open, proxy, tab, peer, network, resource, onSuccess }) would make open/close transitions atomic and reduce the risk of partial state updates.src/modules/reverse-proxy/table/ReverseProxyTable.tsx (1)
199-246: Loose equality (==) used for length checks.Multiple comparisons use
reverseProxies?.length == 0(e.g., Lines 199, 205, 219, 233, 245, 248). While functionally fine here since the LHS is alwaysnumber | undefined, prefer strict equality (=== 0) for consistency and to avoid any confusion.src/app/(dashboard)/peer/page.tsx (3)
146-226:PeerSettingsProvideris defined inline — consider extracting to its own file.The
PeerSettingsContext,usePeerSettings, andPeerSettingsProviderare defined inline withinpage.tsx(which is already ~850 lines). According to the AI summary, a separatesrc/contexts/PeerSettingsProvider.tsxfile exists but the context is actually defined here. Consider moving it to keep the page file focused on layout/composition.
110-118: Verify thatReverseProxiesProviderwrapping the entire peer page doesn't cause unnecessary API calls.
ReverseProxiesProviderfetches/reverse-proxies/services,/peers, and/networks/resources(three API calls). These fire for every peer page visit regardless of whether the user ever navigates to the "Reverse Proxies" tab. If this tab is infrequently used, consider lazy-loading the provider or deferring the fetch until the tab is active.
329-332:flatTargetsis recomputed on everyreverseProxiesorpeerchange, butpeerobject identity may change on every render.The
useMemodepends on[reverseProxies, peer]. IfusePeer()returns a newpeerobject reference on each render (e.g., from SWR's immutable returns),flatTargetswill be recomputed every time. Consider depending onpeer.idinstead, or ensurepeerhas stable identity.src/modules/reverse-proxy/ReverseProxyModal.tsx (3)
88-120:parseDomainuses hardcoded known domains — fragile if new domains are introduced.The
knownDomainsarray["netbird.cloud", "netbird.io", "netbird.app"]is hardcoded. If the backend introduces new managed domains, this function will incorrectly classify them as custom. Consider deriving the known domains from thedomainsprop (filtered byReverseProxyDomainType.FREE) instead of hardcoding.
782-793: Auth modal callbacks all usesetTimeout(..., 200)— fragile timing dependency.Every auth modal
onSave/onRemovecallback wraps state updates in a 200mssetTimeout, presumably to avoid conflicts with modal close animations. This is a fragile pattern — if animation durations change, or if the component unmounts before the timeout fires, state updates will be lost or trigger React warnings. Consider using the modal'sonOpenChangecallback or anonExitCompleteevent to coordinate timing instead.Also applies to: 802-812, 821-832
246-248: UnnecessaryuseMemoforsubmitDisabled.
submitDisabledis just!canContinueToSettings— wrapping it inuseMemoadds overhead without benefit. A plain derived value suffices.♻️ Suggested simplification
- const submitDisabled = useMemo(() => { - return !canContinueToSettings; - }, [canContinueToSettings]); + const submitDisabled = !canContinueToSettings;src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx (1)
174-177:normalizePathis a plain function defined inside the component — recreated every render.Since it's a pure utility with no dependencies on component state, move it outside the component to avoid recreation on every render and make it reusable.
♻️ Suggested refactor
+// Normalize path for comparison (ensure it starts with / and handle empty as /) +const normalizePath = (path: string | undefined) => { + if (!path || path === "") return "/"; + return path.startsWith("/") ? path : `/${path}`; +}; + export default function ReverseProxyTargetModal({ ... - // Normalize path for comparison (ensure it starts with / and handle empty as /) - const normalizePath = (path: string | undefined) => { - if (!path || path === "") return "/"; - return path.startsWith("/") ? path : `/${path}`; - };
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores