Merged
Conversation
📝 WalkthroughWalkthroughAdds reverse-proxy access control types and UI (editor, table cell, modal integration); threads subdivision_code through region rendering; extends Select/CountrySelector with icon sizing/truncation; refactors reverse-proxy auth badge rendering; and removes numeric upper-bound timeout validation. Changes
Sequence DiagramsequenceDiagram
participant Modal as ReverseProxyModal
participant Rules as ReverseProxyAccessControlRules
participant Validator as ip-cidr Validator
participant CountryCtx as Country Context
Modal->>Rules: provide value (AccessRestrictions) + onChange + onValidationChange
Rules->>Rules: initialize internal rule rows from AccessRestrictions
User->>Rules: add / edit / remove rule
Rules->>Validator: validate IP or CIDR
Validator-->>Rules: validation result
Rules->>CountryCtx: resolve country names for country rules
CountryCtx-->>Rules: country name(s)
Rules->>Modal: call onChange(converted AccessRestrictions)
Rules->>Modal: call onValidationChange(hasErrors)
Modal->>Modal: enable/disable submit and persist access_restrictions on save
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx (1)
488-491:⚠️ Potential issue | 🟡 MinorInconsistent help text: still mentions "(max 10m)" but validation was removed.
The
validateSessionIdleTimeoutfunction no longer enforces the max-duration limit, but the help text here still states "(max 10m)". This creates user confusion—the UI claims a constraint that isn't enforced.Update the help text to use example-based guidance consistent with the Request Timeout field.
✏️ Suggested fix
<HelpText className={"mb-0"}> How long a UDP session stays alive without traffic - (max 10m). <br /> Defaults to 30s when empty. + (e.g. 30s, 2m, 5m). <br /> Defaults to 30s when empty. </HelpText>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx` around lines 488 - 491, The help text for the UDP session idle timeout in ReverseProxyTargetModal.tsx is inconsistent with the updated validateSessionIdleTimeout behavior (which no longer enforces a 10m max); update the HelpText near the session idle timeout input (the same area that references Request Timeout) to remove "(max 10m)" and instead provide example-based guidance consistent with the Request Timeout field (e.g., state the default when empty and show a sample value like "e.g., 30s"); ensure the change references the HelpText element and aligns wording with validateSessionIdleTimeout and the Request Timeout help text.src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
326-337:⚠️ Potential issue | 🟠 MajorKeep the public-exposure warning for unprotected L4 services.
Access control is now the only protection path for TCP/TLS/UDP, but the confirmation dialog still runs only for
!isL4Mode. A new L4 service with no rules now saves without any warning even though it is fully public.
⚠️ Suggested fix- if (!isL4Mode && isUnprotected) { + if (isUnprotected) { const confirmed = await confirm({ title: "No Protection Configured", - description: - "This service has no authentication or access control rules configured. It will be publicly accessible to everyone on the internet. Are you sure you want to continue?", + description: isL4Mode + ? "This service has no access control rules configured. It will be publicly accessible to everyone on the internet. Are you sure you want to continue?" + : "This service has no authentication or access control rules configured. It will be publicly accessible to everyone on the internet. Are you sure you want to continue?", type: "warning",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/ReverseProxyModal.tsx` around lines 326 - 337, The confirmation dialog is only shown when !isL4Mode but should also run for L4 services; update the guard around the confirm(...) call in ReverseProxyModal so it triggers whenever isUnprotected is true (instead of only when !isL4Mode && isUnprotected). Locate the block using the symbols isL4Mode, isUnprotected, confirm and reverseProxy and change the condition to check isUnprotected (or isUnprotected || <any other existing checks you need to keep) so unprotected L4/TCP/TLS/UDP services show the same "No Protection Configured" warning before saving.
🧹 Nitpick comments (1)
src/components/select/SelectDropdown.tsx (1)
226-234: Consider making the denser menu spacing opt-in.These padding changes hit every
SelectDropdown, not just the new access-control editor. Since the shared component already has fixed-width consumers likesrc/modules/reverse-proxy/ReverseProxyAccessControlRules.tsx:1-50andsrc/modules/setup-netbird-modal/WindowsTab.tsx:1-50, I'd rather gate the compact layout behind a prop so unrelated dropdowns do not silently restyle.♻️ Possible direction
interface SelectDropdownProps { value: string; onChange: (value: string) => void; @@ triggerClassName?: string; iconSize?: number; truncate?: boolean; + compact?: boolean; } @@ export function SelectDropdown({ @@ triggerClassName, iconSize = 14, truncate = false, + compact = false, }: Readonly<SelectDropdownProps>) { @@ <ScrollArea className={cn( - "overflow-y-auto flex flex-col gap-1 pl-1 pr-1", - !showSearch && "pt-1", + "overflow-y-auto flex flex-col gap-1", + compact ? "pl-1 pr-1" : "pl-2 pr-3", + !showSearch && (compact ? "pt-1" : "pt-2"), )} @@ <CommandGroup> - <div className={"grid grid-cols-1 gap-1 pb-1 w-full"}> + <div + className={cn( + "grid grid-cols-1 gap-1 w-full", + compact ? "pb-1" : "pb-2", + )} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/SelectDropdown.tsx` around lines 226 - 234, The new denser padding should be opt-in: add a boolean prop like dense (default false) to SelectDropdown and use it to conditionally apply the tighter spacing classes (e.g., replace the hardcoded "gap-1 pb-1" / "!showSearch && 'pt-1'" usage inside the CommandGroup wrapper with dense ? "gap-1 pb-1 pt-1" : the original spacing classes). Update the SelectDropdown props/type signature (e.g., interface SelectDropdownProps) and all callers that need the compact layout (the access-control editor and the WindowsTab consumer) to pass dense={true}; leave other callers unchanged so unrelated dropdowns keep their original spacing.
🤖 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/reverse-proxy/ReverseProxyAccessControlRules.tsx`:
- Around line 126-145: validateRule currently returns an empty string for empty
values (and for country rules), so blank rows bypass validation and are dropped
by rulesToRestrictions (which does "if (!rule.value) continue"). Fix by changing
validateRule(rule) to return a non-empty error string when rule.value is an
empty string (e.g., "Value required") for all rule types so the UI validation
prevents saving blank rows; keep rulesToRestrictions as-is (it can still skip
empty values) but ensure validateRule enforces non-empty inputs before rules are
emitted. Reference validateRule and rulesToRestrictions to locate the changes.
- Around line 107-113: The loops that build rules from
restrictions.allowed_cidrs and restrictions.blocked_cidrs create single-host
"ip" entries by assuming a "/32" mask; update the logic used around nextId() and
rules.push so it detects address family (IPv4 vs IPv6) and uses "/32" for IPv4
and "/128" for IPv6 when normalizing single-host entries, and when the selected
type is "ip" reject/strip inputs that contain a CIDR suffix (i.e., contain "/")
so saved restriction type matches the chosen rule type; apply the same fix to
the other occurrences referenced (the blocks around the other rules at lines
roughly 132-133 and 160-170) so all normalization and validation consistently
handle IPv4/IPv6 and disallow CIDR input for "ip" rules.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 318-323: The reconstructed auth payload and isUnprotected check
are ignoring header_auths, which causes header-auth-only services to lose
protection; when rebuilding the auth object (the code that uses passwordEnabled,
pinEnabled, bearerEnabled, linkAuthEnabled, accessRestrictions) include the
existing header_auths (e.g., existingAuth?.header_auths or the component state
variable header_auths/headerAuths) instead of dropping it, and update the
isUnprotected expression to also require no header_auths (e.g., &&
!(header_auths && header_auths.length > 0)); apply the same change in the other
auth-rebuild locations referenced (around the blocks at lines ~341-357 and
~388-399).
In `@src/modules/reverse-proxy/table/ReverseProxyAccessControlCell.tsx`:
- Around line 137-142: The access-control cell's onClick handler (the div that
calls openModal in ReverseProxyAccessControlCell) is not gated by the same
services.update permission as the Settings button; update the click path to
check the same permission (services.update) before calling openModal so the
badge click is inert for users without update rights, and apply the same change
to the second click handler instance around lines 181-189 to ensure both click
paths respect the permission gate.
In `@src/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx`:
- Around line 185-196: The icon-only Settings Button (component Button with
Settings icon, onClick calling openModal({ proxy: reverseProxy, initialTab:
"auth" }) and disabled={!permission?.services?.update}) has no accessible name;
add an accessible label (e.g., aria-label="Configure authentication" or
aria-labelledby pointing to a visually-hidden label) to the Button so screen
readers announce its purpose while keeping the icon-only UI, and ensure the
label is present even when disabled.
- Around line 15-16: The import and JSX usage use an invalid lucide-react export
`LockOpenIcon`; change the named import to `LockOpen` (replace `LockOpenIcon`
with `LockOpen` in the import list alongside `LockKeyhole`) and update any JSX
references (e.g., `LockOpenIcon` instances around the auth cell rendering) to
use `LockOpen` instead so the component uses the correct lucide-react export in
ReverseProxyAuthCell (also update the occurrence noted near line 181).
---
Outside diff comments:
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 326-337: The confirmation dialog is only shown when !isL4Mode but
should also run for L4 services; update the guard around the confirm(...) call
in ReverseProxyModal so it triggers whenever isUnprotected is true (instead of
only when !isL4Mode && isUnprotected). Locate the block using the symbols
isL4Mode, isUnprotected, confirm and reverseProxy and change the condition to
check isUnprotected (or isUnprotected || <any other existing checks you need to
keep) so unprotected L4/TCP/TLS/UDP services show the same "No Protection
Configured" warning before saving.
In `@src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx`:
- Around line 488-491: The help text for the UDP session idle timeout in
ReverseProxyTargetModal.tsx is inconsistent with the updated
validateSessionIdleTimeout behavior (which no longer enforces a 10m max); update
the HelpText near the session idle timeout input (the same area that references
Request Timeout) to remove "(max 10m)" and instead provide example-based
guidance consistent with the Request Timeout field (e.g., state the default when
empty and show a sample value like "e.g., 30s"); ensure the change references
the HelpText element and aligns wording with validateSessionIdleTimeout and the
Request Timeout help text.
---
Nitpick comments:
In `@src/components/select/SelectDropdown.tsx`:
- Around line 226-234: The new denser padding should be opt-in: add a boolean
prop like dense (default false) to SelectDropdown and use it to conditionally
apply the tighter spacing classes (e.g., replace the hardcoded "gap-1 pb-1" /
"!showSearch && 'pt-1'" usage inside the CommandGroup wrapper with dense ?
"gap-1 pb-1 pt-1" : the original spacing classes). Update the SelectDropdown
props/type signature (e.g., interface SelectDropdownProps) and all callers that
need the compact layout (the access-control editor and the WindowsTab consumer)
to pass dense={true}; leave other callers unchanged so unrelated dropdowns keep
their original spacing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1c2ba09-8207-455b-b551-048ec966d784
📒 Files selected for processing (14)
src/components/Select.tsxsrc/components/select/SelectDropdown.tsxsrc/components/ui/CountrySelector.tsxsrc/contexts/CountryProvider.tsxsrc/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyAccessControlRules.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsLocationIpCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyAccessControlCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyTable.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsxsrc/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsxsrc/modules/reverse-proxy/targets/useReverseProxyTargetOptions.ts
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/reverse-proxy/table/ReverseProxyAuthCell.tsx`:
- Around line 95-117: The badges currently show pointer/hover affordances even
when clicking does nothing because configuration is read-only; introduce a
single boolean (e.g., canConfigure) computed from services.update and reuse it
both for the click handler and for the Badge classes used in authBadge (the
Badge rendering branches with SingleAuthIcon and the authCount branch and the
other affected branches around the second occurrence), so when canConfigure is
false remove the "cursor-pointer" and hover styles and ensure the onClick
handler is conditional (no-op or not attached) based on canConfigure to keep
visuals and behavior consistent.
- Around line 147-150: The label logic currently infers SSO scope from resolved
Group records (ssoGroups) which causes "All Users" to show when groups haven't
loaded; in the ReverseProxyAuthCell component change the conditional that
renders {key === "bearer_auth" && ssoGroups.length === 0 ? "All Users" :
"Enabled"} to instead check the raw distribution_groups array length (e.g.,
distribution_groups?.length === 0) so the label reflects configured scope, and
update the part where you render group entries to handle missing/resolved Group
objects separately (keep ssoGroups for resolved display but treat null/undefined
entries as "Unknown group" or similar) rather than driving the top-level label
from ssoGroups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f96bb4f9-cc05-43a1-b31c-b31c67bc45b8
📒 Files selected for processing (2)
src/modules/reverse-proxy/table/ReverseProxyAccessControlCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/reverse-proxy/table/ReverseProxyAccessControlCell.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/modules/reverse-proxy/ReverseProxyAccessControlRules.tsx (2)
126-127:⚠️ Potential issue | 🟠 MajorBlank rows still bypass validation and disappear on save.
rulesToRestrictions()drops empty rows, butvalidateRule()returns no error for empty values. A user can save with a blank access-control row and silently lose it.Suggested fix
function validateRule(rule: AccessRule): string { - if (rule.type === "country" || !rule.value) return ""; + if (!rule.value) { + return rule.type === "country" + ? "Please select a country" + : rule.type === "ip" + ? "Please enter a valid IP address, e.g., 85.203.15.42" + : "Please enter a valid CIDR block, e.g., 74.125.0.0/16"; + } + if (rule.type === "country") return ""; if (rule.type === "ip") {Also applies to: 160-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/ReverseProxyAccessControlRules.tsx` around lines 126 - 127, The loop that drops empty rows (for (const rule of rules) { if (!rule.value) continue; }) allows blank rows to be removed silently because validateRule() doesn't treat empty rule.value as an error; update validateRule(rule) to return an error when rule.value is empty/only-whitespace (or trim and check length === 0) so the UI flags the row and prevents save, and/or adjust rulesToRestrictions() to not silently drop rows but surface validation failures; reference validateRule and rulesToRestrictions to implement the check and ensure the save flow respects validation errors.
107-113:⚠️ Potential issue | 🔴 CriticalNormalize IP rules by address family and keep CIDR out of the IP path.
This still serializes every host rule as
/32, which corrupts IPv6 hosts and misclassifies IPv6/32networks as single-IP rules. It also accepts CIDR input while the row is inipmode, so the saved restriction can stop matching the selected rule type.Suggested fix
+function toHostCidr(value: string) { + return `${value}/${value.includes(":") ? "128" : "32"}`; +} + function restrictionsToRules( restrictions: AccessRestrictions | undefined, ): AccessRule[] { @@ restrictions.allowed_cidrs?.forEach((v) => { - const isIp = v.endsWith("/32"); - rules.push({ id: nextId(), action: "allow", type: isIp ? "ip" : "cidr", value: isIp ? v.replace(/\/32$/, "") : v }); + const isIp = v.endsWith("/32") || v.endsWith("/128"); + rules.push({ + id: nextId(), + action: "allow", + type: isIp ? "ip" : "cidr", + value: isIp ? v.replace(/\/(32|128)$/, "") : v, + }); }); restrictions.blocked_cidrs?.forEach((v) => { - const isIp = v.endsWith("/32"); - rules.push({ id: nextId(), action: "block", type: isIp ? "ip" : "cidr", value: isIp ? v.replace(/\/32$/, "") : v }); + const isIp = v.endsWith("/32") || v.endsWith("/128"); + rules.push({ + id: nextId(), + action: "block", + type: isIp ? "ip" : "cidr", + value: isIp ? v.replace(/\/(32|128)$/, "") : v, + }); }); @@ - const value = rule.type === "ip" && !rule.value.includes("/") ? `${rule.value}/32` : rule.value; + const value = rule.type === "ip" ? toHostCidr(rule.value) : rule.value; @@ if (rule.type === "ip") { - const val = rule.value.includes("/") ? rule.value : `${rule.value}/32`; + if (rule.value.includes("/")) { + return "Please enter a single IP address, not a CIDR block"; + } + const val = toHostCidr(rule.value); if (!cidr.isValidAddress(val)) { return "Please enter a valid IP address, e.g., 85.203.15.42"; }Also applies to: 132-133, 160-169
src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
318-323:⚠️ Potential issue | 🔴 CriticalPreserve
header_authswhen deciding “unprotected” and rebuildingauth.A header-auth-only service is still treated as unprotected here, and saving it will still submit an
authobject withoutheader_auths, silently stripping that protection.Suggested fix
+ const hasHeaderAuth = + reverseProxy?.auth?.header_auths?.some((config) => config.enabled) ?? false; + const isUnprotected = !passwordEnabled && !pinEnabled && !bearerEnabled && !linkAuthEnabled && + !hasHeaderAuth && !accessRestrictions; @@ const auth: ReverseProxyAuth = { + ...reverseProxy?.auth, password_auth: { enabled: passwordEnabled, password: password, },Also applies to: 341-357, 388-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/ReverseProxyModal.tsx` around lines 318 - 323, The current isUnprotected check and subsequent auth rebuild drop header_auths, so update the logic to treat header-auth-only services as protected and to preserve header_auths when constructing the auth object: include header_auths in the isUnprotected condition (i.e., consider header_auths truthy as a protection) and when you assemble the new auth payload (the code paths that rebuild/set auth) merge or retain existing header_auths into the auth object instead of omitting it; update the references around isUnprotected and the auth construction logic (variables/functions named isUnprotected, passwordEnabled, pinEnabled, bearerEnabled, linkAuthEnabled, accessRestrictions, header_auths, and the auth rebuild/assignment code) so header-based auth is neither treated as unprotected nor silently stripped.
🤖 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/reverse-proxy/ReverseProxyAccessControlRules.tsx`:
- Around line 126-127: The loop that drops empty rows (for (const rule of rules)
{ if (!rule.value) continue; }) allows blank rows to be removed silently because
validateRule() doesn't treat empty rule.value as an error; update
validateRule(rule) to return an error when rule.value is empty/only-whitespace
(or trim and check length === 0) so the UI flags the row and prevents save,
and/or adjust rulesToRestrictions() to not silently drop rows but surface
validation failures; reference validateRule and rulesToRestrictions to implement
the check and ensure the save flow respects validation errors.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 318-323: The current isUnprotected check and subsequent auth
rebuild drop header_auths, so update the logic to treat header-auth-only
services as protected and to preserve header_auths when constructing the auth
object: include header_auths in the isUnprotected condition (i.e., consider
header_auths truthy as a protection) and when you assemble the new auth payload
(the code paths that rebuild/set auth) merge or retain existing header_auths
into the auth object instead of omitting it; update the references around
isUnprotected and the auth construction logic (variables/functions named
isUnprotected, passwordEnabled, pinEnabled, bearerEnabled, linkAuthEnabled,
accessRestrictions, header_auths, and the auth rebuild/assignment code) so
header-based auth is neither treated as unprotected nor silently stripped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ceb54abe-a544-4e19-8b32-9853d83bc33b
📒 Files selected for processing (4)
src/components/select/SelectDropdown.tsxsrc/modules/reverse-proxy/ReverseProxyAccessControlRules.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx
✅ Files skipped from review due to trivial changes (1)
- src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/select/SelectDropdown.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx (3)
15-16:⚠️ Potential issue | 🔴 CriticalUse
LockOpeninstead ofLockOpenIconfor lucide-react import compatibility.At Line 16 and Line 181,
LockOpenIconis used; with lucide-react v3 this is typically exported asLockOpen, which can otherwise fail at build/type-check time.🐛 Proposed fix
import { ArrowRightIcon, Binary, HelpCircle, LockKeyhole, - LockOpenIcon, + LockOpen, LucideIcon, RectangleEllipsis, Settings, Users, } from "lucide-react"; @@ - <LockOpenIcon size={12} className="text-red-500" /> + <LockOpen size={12} className="text-red-500" />#!/bin/bash set -euo pipefail # Verify current usage in this file rg -n '\bLockOpenIcon\b|\bLockOpen\b' src/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx # Verify actual exports from installed lucide-react types (if dependencies are present) if [ -d node_modules/lucide-react ]; then rg -n '\bLockOpenIcon\b|\bLockOpen\b' node_modules/lucide-react else echo "node_modules/lucide-react not present. Run dependency install, then rerun this script." fiExpected result:
LockOpenappears in lucide-react exports;LockOpenIcondoes not.Also applies to: 181-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx` around lines 15 - 16, The file imports and uses LockOpenIcon from lucide-react but lucide-react v3 exports the icon as LockOpen; update the import list to replace LockOpenIcon with LockOpen and update any usage sites (e.g., the JSX reference to LockOpenIcon) to use LockOpen instead so the component (ReverseProxyAuthCell) compiles against the v3 exports.
101-112:⚠️ Potential issue | 🟡 MinorKeep read-only visuals consistent with click behavior.
At Line 124-Line 126, opening is permission-gated, but badges at Line 101-Line 112 and Line 179-Line 180 still always render pointer/hover affordances, making read-only state look clickable.
♻️ Proposed fix
- className={"cursor-pointer !rounded-r-none !border-r-0 !h-[34px] min-w-[100px] !justify-start hover:bg-nb-gray-930 transition-all"} + className={`${canConfigure ? "cursor-pointer hover:bg-nb-gray-930" : "cursor-default"} !rounded-r-none !border-r-0 !h-[34px] min-w-[100px] !justify-start transition-all`} @@ - className={"cursor-pointer !rounded-r-none !border-r-0 !h-[34px] min-w-[100px] !justify-start hover:bg-nb-gray-930 transition-all"} + className={`${canConfigure ? "cursor-pointer hover:bg-nb-gray-930" : "cursor-default"} !rounded-r-none !border-r-0 !h-[34px] min-w-[100px] !justify-start transition-all`}Also applies to: 122-126, 179-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx` around lines 101 - 112, The Badge elements in ReverseProxyAuthCell.tsx (the Badge rendering branches that use authCount/enabled) always include pointer and hover affordances even when the cell is read-only; update the className construction for those Badge instances to conditionally remove "cursor-pointer" and hover-related classes (e.g., "!justify-start hover:bg-nb-gray-930 transition-all" and "!rounded-r-none !border-r-0 !h-[34px] min-w-[100px]") when canConfigure is false so the visual state matches the permission gating; locate the Badge usages tied to authCount, enabled and the canConfigure variable and apply conditional class names or a small helper to toggle the pointer/hover classes accordingly.
147-149:⚠️ Potential issue | 🟡 MinorDerive “All Users” from configured
distribution_groups, not resolved group records.Line 147 currently uses
ssoGroups.length === 0, which can incorrectly show “All Users” when groups haven’t loaded yet butdistribution_groupsis configured.💡 Proposed fix
- {key === "bearer_auth" && ssoGroups.length === 0 + {key === "bearer_auth" && + (auth?.bearer_auth?.distribution_groups?.length ?? 0) === 0 ? "All Users" : "Enabled"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx` around lines 147 - 149, The UI currently decides to render "All Users" using the resolved ssoGroups length (key === "bearer_auth" && ssoGroups.length === 0) which can be wrong while groups are still loading; change the condition to derive "All Users" from the configured distribution_groups instead (e.g., key === "bearer_auth" && (distribution_groups?.length ?? 0) === 0), updating the JSX in ReverseProxyAuthCell.tsx where that expression sits so it checks the distribution_groups config variable (or prop) rather than ssoGroups before rendering "All Users".
🤖 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/reverse-proxy/table/ReverseProxyAuthCell.tsx`:
- Around line 15-16: The file imports and uses LockOpenIcon from lucide-react
but lucide-react v3 exports the icon as LockOpen; update the import list to
replace LockOpenIcon with LockOpen and update any usage sites (e.g., the JSX
reference to LockOpenIcon) to use LockOpen instead so the component
(ReverseProxyAuthCell) compiles against the v3 exports.
- Around line 101-112: The Badge elements in ReverseProxyAuthCell.tsx (the Badge
rendering branches that use authCount/enabled) always include pointer and hover
affordances even when the cell is read-only; update the className construction
for those Badge instances to conditionally remove "cursor-pointer" and
hover-related classes (e.g., "!justify-start hover:bg-nb-gray-930
transition-all" and "!rounded-r-none !border-r-0 !h-[34px] min-w-[100px]") when
canConfigure is false so the visual state matches the permission gating; locate
the Badge usages tied to authCount, enabled and the canConfigure variable and
apply conditional class names or a small helper to toggle the pointer/hover
classes accordingly.
- Around line 147-149: The UI currently decides to render "All Users" using the
resolved ssoGroups length (key === "bearer_auth" && ssoGroups.length === 0)
which can be wrong while groups are still loading; change the condition to
derive "All Users" from the configured distribution_groups instead (e.g., key
=== "bearer_auth" && (distribution_groups?.length ?? 0) === 0), updating the JSX
in ReverseProxyAuthCell.tsx where that expression sits so it checks the
distribution_groups config variable (or prop) rather than ssoGroups before
rendering "All Users".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47bc33f8-6747-41e2-a99f-3528a6c50ed9
📒 Files selected for processing (2)
src/modules/reverse-proxy/table/ReverseProxyAccessControlCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx
lixmal
approved these changes
Mar 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 by CodeRabbit
New Features
Improvements