Add layer 4 protocol support to reverse proxy#579
Merged
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 service mode (HTTP/TLS/TCP/UDP) support across reverse-proxy: new types and helpers, provider exposure of resources/peers, mode-aware modal/target UI and payloads, L4-specific (TCP/UDP/TLS) flows and validations, table/cell rendering changes, and related UI components/hooks. Changes
Sequence DiagramsequenceDiagram
participant User
participant Provider as ReverseProxiesProvider
participant Modal as ReverseProxyModal
participant TargetModal as ReverseProxyTargetModal
participant Validation as useReverseProxyTargetOptions
participant API as Backend/API
User->>Provider: open reverse-proxy modal (optional initialEndpointMode)
Provider->>Modal: pass resources, peers, initialEndpointMode
Modal->>Modal: set endpointMode (reverseProxy.mode || initialEndpointMode)
alt HTTP/TLS flow
User->>Modal: configure domain/subdomain and HTTP targets
Modal->>TargetModal: open HTTP/S target editor
TargetModal->>Validation: validate headers/timeouts
else L4 (TCP/UDP/TLS) flow
User->>Modal: select L4 mode, configure listen_port and L4 target
Modal->>TargetModal: open L4 target editor (ReverseProxyLayer4Content)
TargetModal->>Validation: validate session_idle_timeout (UDP) and options
end
Validation-->>TargetModal: return errors or ok
alt valid
TargetModal->>API: submit payload (includes mode, listen_port, targets, options)
API-->>Modal: persist response
else invalid
TargetModal-->>User: show validation errors
end
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 unit tests (beta)
📝 Coding Plan
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 (2)
src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx (1)
62-98: Consider lifting peer/resource data to avoid redundant fetches.The
L4TargetDisplaycomponent fetchespeersandresourcesviauseFetchApi. While SWR caches these requests, the parent table likely already has access to this data. Consider passing it as props if performance becomes a concern with many L4 services.The current implementation is functional. This is a minor optimization that could be addressed later if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx` around lines 62 - 98, L4TargetDisplay currently calls useFetchApi for peers and resources inside the component causing redundant fetches; change the component signature to accept optional props (e.g., peers?: Peer[] and resources?: NetworkResource[]) and use those passed-in arrays to resolve peer/resource (falling back to the existing useFetchApi calls only if the props are undefined), update all callers of L4TargetDisplay to pass the parent’s cached peers/resources where available, and keep the existing DeviceCard/address resolution logic unchanged (references: L4TargetDisplay, useFetchApi, DeviceCard).src/contexts/ReverseProxiesProvider.tsx (1)
57-57: Consider usingServiceModeenum for type consistency.The
initialEndpointModeuses string literals ("http" | "tls" | "tcp" | "udp") that mirrorServiceModeenum values. Using the enum directly would ensure type safety and prevent drift if enum values change.♻️ Suggested refactor
+import { ServiceMode } from "@/interfaces/ReverseProxy"; type OpenModalOptions = { proxy?: ReverseProxy; initialTab?: string; - initialEndpointMode?: "http" | "tls" | "tcp" | "udp"; + initialEndpointMode?: ServiceMode; initialPeer?: Peer; ... }; -const [modalInitialEndpointMode, setModalInitialEndpointMode] = useState< - "http" | "tls" | "tcp" | "udp" | undefined ->(); +const [modalInitialEndpointMode, setModalInitialEndpointMode] = useState< + ServiceMode | undefined +>();Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/ReverseProxiesProvider.tsx` at line 57, The prop/type uses a string union for endpoint modes; switch the type of initialEndpointMode to the ServiceMode enum to ensure consistency: update the prop/interface in ReverseProxiesProvider (initialEndpointMode) to use ServiceMode, import ServiceMode where needed, and update any state, default values, and comparisons (the other occurrences around the initial state/usage) to use ServiceMode enum members instead of raw strings so all references (initialEndpointMode, any state setters or comparisons) remain type-safe and consistent.
🤖 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/ReverseProxyModal.tsx`:
- Around line 294-296: The modal currently always reads/writes the timeout into
request_timeout (state variables requestTimeout/setRequestTimeout and
existingL4Target?.options?.request_timeout), which breaks the UDP path that
expects session_idle_timeout; change initialization to read from
existingL4Target?.options?.session_idle_timeout when the selected L4 mode is UDP
and change the serialization logic that builds the target options to write
session_idle_timeout instead of request_timeout for UDP services (leave
request_timeout for other L4 modes). Ensure the timeout collectors (the places
that gather values for all L4 modes) and the save/submit code use the selected
mode to choose which key (request_timeout vs session_idle_timeout) to persist so
values round-trip correctly.
- Around line 214-245: Currently the code only hydrates existingL4Target from
reverseProxy.targets?.[0] and always submits a single-element array [l4Target],
which discards any other L4 targets; change the logic to treat L4 targets as an
array: hydrate state from reverseProxy.targets (preserve the whole array)
instead of only targets[0] (replace existingL4Target references with the targets
array or a selectedIndex), keep per-target fields (tlsTargetType, tlsPeerId,
tlsResourceId, tlsHost, tlsPort, tlsListenPort) tied to the selected target
index or maintain a targets state like l4Targets: ReverseProxyTarget[], update
the correct target in that array when fields change (or replace the target at
index 0 only if the user intends) and, on submit, send reverseProxy.targets (the
full updated array) instead of always submitting [l4Target]; update places
referencing existingL4Target and the submit code that builds l4Target to
read/write from this array (e.g., existingL4Target, tls* state variables, and
the code that constructs l4Target for submission).
- Around line 218-230: The initial state logic for TLS target selection in
ReverseProxyModal sets tlsTargetType to PEER regardless of initialResource,
causing resource-driven create flows to start with no selected target; update
the initializer for tlsTargetType to choose ReverseProxyTargetType.RESOURCE when
initialResource is present (instead of defaulting to PEER), and ensure
tlsResourceId is initialized from initialResource?.id while tlsPeerId is
undefined when initialResource exists; adjust the related setters
(setTlsTargetType, setTlsPeerId, setTlsResourceId) so existingL4Target and
existingL4IsPeer remain honored but initialResource takes precedence when no
existing target is provided.
---
Nitpick comments:
In `@src/contexts/ReverseProxiesProvider.tsx`:
- Line 57: The prop/type uses a string union for endpoint modes; switch the type
of initialEndpointMode to the ServiceMode enum to ensure consistency: update the
prop/interface in ReverseProxiesProvider (initialEndpointMode) to use
ServiceMode, import ServiceMode where needed, and update any state, default
values, and comparisons (the other occurrences around the initial state/usage)
to use ServiceMode enum members instead of raw strings so all references
(initialEndpointMode, any state setters or comparisons) remain type-safe and
consistent.
In `@src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx`:
- Around line 62-98: L4TargetDisplay currently calls useFetchApi for peers and
resources inside the component causing redundant fetches; change the component
signature to accept optional props (e.g., peers?: Peer[] and resources?:
NetworkResource[]) and use those passed-in arrays to resolve peer/resource
(falling back to the existing useFetchApi calls only if the props are
undefined), update all callers of L4TargetDisplay to pass the parent’s cached
peers/resources where available, and keep the existing DeviceCard/address
resolution logic unchanged (references: L4TargetDisplay, useFetchApi,
DeviceCard).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eeb2821f-f31c-4976-8ddc-d61751b6001f
📒 Files selected for processing (11)
src/contexts/ReverseProxiesProvider.tsxsrc/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyNameCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyStatusCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyTable.tsxsrc/modules/reverse-proxy/table/ReverseProxyTargetsCell.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
♻️ Duplicate comments (1)
src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
218-229:⚠️ Potential issue | 🟠 MajorRestore
initialResourcein the L4 initial state.When there is no
existingL4Target,existingL4IsPeerisfalse, so Line 228 still takes theexistingL4Target?.target_idbranch and dropsinitialResource?.id. Line 222 also hard-codes every preset resource toHOST, which loses subnet targets. Resource-driven create flows will still open with no selected target, and subnet resources can be serialized with the wrong type.Proposed fix
const [tlsTargetType, setTlsTargetType] = useState<ReverseProxyTargetType>( existingL4Target ? existingL4Target.target_type : initialResource - ? ReverseProxyTargetType.HOST + ? (initialResource.type as ReverseProxyTargetType) : ReverseProxyTargetType.PEER, ); const [tlsPeerId, setTlsPeerId] = useState<string | undefined>( existingL4IsPeer ? existingL4Target?.target_id : initialPeer?.id, ); const [tlsResourceId, setTlsResourceId] = useState<string | undefined>( - !existingL4IsPeer ? existingL4Target?.target_id : initialResource?.id, + existingL4Target + ? existingL4IsPeer + ? undefined + : existingL4Target.target_id + : initialResource?.id, );🤖 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 218 - 229, The initial L4 state ignores initialResource when no existingL4Target, causing HOST to be hard-coded and dropping subnet/other resource types; update the initializers for tlsTargetType, tlsPeerId and tlsResourceId so they first check existingL4Target (and existingL4IsPeer for target id), and if absent fall back to initialResource (using initialResource?.type for tlsTargetType and initialResource?.id for tlsResourceId) and only then to the default (e.g., ReverseProxyTargetType.PEER) or initialPeer for tlsPeerId; adjust the logic around tlsTargetType, tlsPeerId and tlsResourceId to use existingL4Target ? ... : initialResource ? ... : ... so resource-driven create flows preserve subnet/resource types.
🤖 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/ReverseProxyModal.tsx`:
- Around line 564-577: The port-based branch (isPortBased) removed the UI bound
to subdomain so TCP/UDP services can't be named or renamed; restore a name field
in that branch by rendering an input (matching the non-port branch pattern)
bound to the subdomain state (value={subdomain}, onChange={setSubdomain})—e.g.,
add a Label/HelpText + text input next to CustomDomainSelector inside the
isPortBased block so the code that uses subdomain (service naming logic
referencing subdomain) continues to work for new and existing TCP/UDP services.
- Around line 1008-1018: The Connection Timeout UI block rendered when isL4Mode
is true currently uses TCP/TLS wording; update the Label and HelpText in the JSX
block (the element containing Timer, "Connection Timeout" Label and the
HelpText) to be mode-aware: when the L4 backend protocol is UDP show a label
like "Session Idle Timeout" and help text that explains this saves
session_idle_timeout (idle timeout for UDP sessions), otherwise keep the
existing "Connection Timeout" and text about establishing backend connections
for TCP/TLS; implement the conditional using the existing isL4Mode/protocol
state (or the same prop that distinguishes UDP) inside the same component so
copy is accurate for both UDP and non-UDP L4 modes.
---
Duplicate comments:
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 218-229: The initial L4 state ignores initialResource when no
existingL4Target, causing HOST to be hard-coded and dropping subnet/other
resource types; update the initializers for tlsTargetType, tlsPeerId and
tlsResourceId so they first check existingL4Target (and existingL4IsPeer for
target id), and if absent fall back to initialResource (using
initialResource?.type for tlsTargetType and initialResource?.id for
tlsResourceId) and only then to the default (e.g., ReverseProxyTargetType.PEER)
or initialPeer for tlsPeerId; adjust the logic around tlsTargetType, tlsPeerId
and tlsResourceId to use existingL4Target ? ... : initialResource ? ... : ... so
resource-driven create flows preserve subnet/resource types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dea610bf-ce3f-4175-b169-98521f576aed
📒 Files selected for processing (1)
src/modules/reverse-proxy/ReverseProxyModal.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
218-233:⚠️ Potential issue | 🟠 MajorKeep preselected resource targets typed as the actual resource.
initialResourceis now forced toHOST, and the CIDR lookup only comes from the async/networks/resourcesresult. If this modal is opened from a subnet resource, the payload can still go out as a host target and skip CIDR host validation before that lookup finishes.🛠️ Suggested fix
const [tlsTargetType, setTlsTargetType] = useState<ReverseProxyTargetType>( existingL4Target ? existingL4Target.target_type : initialResource - ? ReverseProxyTargetType.HOST + ? (initialResource.type as ReverseProxyTargetType) : ReverseProxyTargetType.PEER, ); const [tlsPeerId, setTlsPeerId] = useState<string | undefined>( - existingL4IsPeer ? existingL4Target?.target_id : initialPeer?.id, + existingL4IsPeer + ? existingL4Target?.target_id + : initialResource + ? undefined + : initialPeer?.id, ); const [tlsResourceId, setTlsResourceId] = useState<string | undefined>( existingL4Target ? existingL4IsPeer ? undefined : existingL4Target.target_id : initialResource?.id, ); const tlsResourceAddress = useMemo(() => { if (!tlsResourceId) return ""; - const resource = resources?.find((r) => r.id === tlsResourceId); + const resource = + resources?.find((r) => r.id === tlsResourceId) ?? + (initialResource?.id === tlsResourceId ? initialResource : undefined); return resource?.address || ""; - }, [tlsResourceId, resources]); + }, [tlsResourceId, resources, initialResource]);Also applies to: 252-281
🤖 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 218 - 233, The modal currently forces preselected initialResource to ReverseProxyTargetType.HOST which can send a host payload and skip CIDR validation when initialResource is actually a subnet; update the initial state logic used in tlsTargetType (and the similar blocks around tlsPeerId/tlsResourceId between the other mentioned lines) to infer the correct ReverseProxyTargetType from initialResource.type (e.g., NETWORK/CIDR vs HOST) instead of always defaulting to HOST, and ensure the initial tlsResourceId is set accordingly so CIDR validation is applied until the async /networks/resources lookup confirms the resource details.
🤖 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/ReverseProxyModal.tsx`:
- Around line 524-536: The modal title logic uses initialEndpointMode instead of
the live selection, so when creating a new service the header doesn't update; in
ReverseProxyModal.tsx update the title branch that runs when reverseProxy is
falsy to consult endpointMode (the state/prop holding the current selection)
rather than initialEndpointMode so the title shows "Add TLS Passthrough"/"Add
TCP Service"/"Add UDP Service"/"Add HTTP Service" based on the current
endpointMode; keep the existing reverseProxy check and only swap
initialEndpointMode → endpointMode in the ternary chain used to set title.
- Around line 652-671: The button list currently only uses color to indicate
selection; update the component so assistive tech can detect the selected
endpoint by adding appropriate ARIA radio semantics: wrap the buttons' container
(where the map over modes occurs) with role="radiogroup" and for each button
(the element using setEndpointMode, endpointMode and mode) set role="radio" and
aria-checked={endpointMode === mode} (or aria-pressed if you prefer toggle
semantics) and ensure the onClick still calls setEndpointMode(mode); also add
tabIndex={endpointMode === mode ? 0 : -1} so keyboard focus moves to the
selected item and include an accessible label via aria-label or use the existing
label text.
---
Duplicate comments:
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 218-233: The modal currently forces preselected initialResource to
ReverseProxyTargetType.HOST which can send a host payload and skip CIDR
validation when initialResource is actually a subnet; update the initial state
logic used in tlsTargetType (and the similar blocks around
tlsPeerId/tlsResourceId between the other mentioned lines) to infer the correct
ReverseProxyTargetType from initialResource.type (e.g., NETWORK/CIDR vs HOST)
instead of always defaulting to HOST, and ensure the initial tlsResourceId is
set accordingly so CIDR validation is applied until the async
/networks/resources lookup confirms the resource details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66dcd4e7-f590-4479-b247-4a524be842cb
📒 Files selected for processing (1)
src/modules/reverse-proxy/ReverseProxyModal.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsx (1)
206-253: Consider extracting the Add Service dropdown into a shared component.This dropdown implementation is nearly identical to
AddServiceDropdowninReverseProxyTable.tsx(lines 273-322). Extracting it into a shared component would reduce duplication and ensure consistent behavior across tables.♻️ Example extraction
Create a shared component:
// src/modules/reverse-proxy/components/AddServiceDropdown.tsx type AddServiceDropdownProps = { openModal: (options?: { initialEndpointMode?: ServiceMode }) => void; disabled?: boolean; className?: string; }; export function AddServiceDropdown({ openModal, disabled, className }: Readonly<AddServiceDropdownProps>) { return ( <DropdownMenu modal={false}> <DropdownMenuTrigger asChild> <Button variant="primary" disabled={disabled} className={className}> <PlusCircle size={16} /> Add Service <ChevronDown size={14} /> </Button> </DropdownMenuTrigger> <DropdownMenuContent align="end" className="w-auto min-w-[200px]"> {/* ... menu items ... */} </DropdownMenuContent> </DropdownMenu> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsx` around lines 206 - 253, The Add Service dropdown in ReverseProxyFlatTargetsTable is duplicated from the one in ReverseProxyTable.tsx; extract it into a shared component (e.g., AddServiceDropdown) that accepts props { openModal: (opts?: { initialEndpointMode?: ServiceMode }) => void, disabled?: boolean, className?: string }, move the DropdownMenu, DropdownMenuTrigger/Button and DropdownMenuContent/MenuItems into that component, replace the inline JSX in ReverseProxyFlatTargetsTable and ReverseProxyTable to render <AddServiceDropdown openModal={openModal} disabled={!permission?.services?.create} className="ml-auto" />, and ensure the menu item handlers still call openModal({ initialEndpointMode: ServiceMode.HTTP|TLS|TCP|UDP }) so behavior and icons are preserved.src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
777-781: IPv4-only character filter may limit IPv6 support.The regex
.replace(/[^0-9.]/g, "")restricts input to IPv4-compatible characters only. If subnet resources with IPv6 addresses are supported, this filter would need to allow colons and hex characters (e.g.,/[^0-9a-fA-F:.]/g).🤖 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 777 - 781, The onChange handler inside ReverseProxyModal that setsTlsHost currently strips all characters except digits and dots using e.target.value.replace(/[^0-9.]/g,""), which prevents IPv6 addresses; update the filter used in the onChange for setTlsHost (and any related input sanitization for tlsIsHostEditable) to allow IPv6 characters (hex digits, colons, and dots) or replace the ad-hoc regex with a proper validation/normalization function (e.g., use a regex permitting 0-9 a-f A-F : . or a dedicated IPv4/IPv6 parser) so IPv6 host inputs are accepted while still rejecting invalid characters.
🤖 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/ReverseProxyModal.tsx`:
- Line 491: The assignment uses an unsafe double-cast endpointMode as unknown as
ReverseProxyTargetProtocol because ReverseProxyTargetProtocol only allows "http"
| "https" while ServiceMode includes "tcp" | "udp" | "tls"; fix by expanding the
type system instead of casting: update the ReverseProxyTargetProtocol type (or
enum) to include "tcp", "udp", "tls", or introduce a new union (e.g.,
ReverseProxyTargetProtocolL4 or Protocol = ReverseProxyTargetProtocol |
ServiceMode) and update the ReverseProxyTarget.protocol type accordingly, then
remove the cast in ReverseProxyModal.tsx and assign endpointMode directly to
protocol (or narrow/convert types explicitly). Ensure references to
ReverseProxyTarget, ReverseProxyTargetProtocol, and endpointMode are updated
consistently.
---
Nitpick comments:
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 777-781: The onChange handler inside ReverseProxyModal that
setsTlsHost currently strips all characters except digits and dots using
e.target.value.replace(/[^0-9.]/g,""), which prevents IPv6 addresses; update the
filter used in the onChange for setTlsHost (and any related input sanitization
for tlsIsHostEditable) to allow IPv6 characters (hex digits, colons, and dots)
or replace the ad-hoc regex with a proper validation/normalization function
(e.g., use a regex permitting 0-9 a-f A-F : . or a dedicated IPv4/IPv6 parser)
so IPv6 host inputs are accepted while still rejecting invalid characters.
In `@src/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsx`:
- Around line 206-253: The Add Service dropdown in ReverseProxyFlatTargetsTable
is duplicated from the one in ReverseProxyTable.tsx; extract it into a shared
component (e.g., AddServiceDropdown) that accepts props { openModal: (opts?: {
initialEndpointMode?: ServiceMode }) => void, disabled?: boolean, className?:
string }, move the DropdownMenu, DropdownMenuTrigger/Button and
DropdownMenuContent/MenuItems into that component, replace the inline JSX in
ReverseProxyFlatTargetsTable and ReverseProxyTable to render <AddServiceDropdown
openModal={openModal} disabled={!permission?.services?.create}
className="ml-auto" />, and ensure the menu item handlers still call openModal({
initialEndpointMode: ServiceMode.HTTP|TLS|TCP|UDP }) so behavior and icons are
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7882199c-cdd4-4dea-a3d6-38c860d4d419
📒 Files selected for processing (4)
src/contexts/ReverseProxiesProvider.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/table/ReverseProxyTable.tsxsrc/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/contexts/ReverseProxiesProvider.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/reverse-proxy/ReverseProxyModal.tsx (2)
208-212: Defer L4 target data loading until an L4 flow actually needs it.
/peersand/networks/resourcesare fetched even for HTTP-only opens, where this data is never used. Gating or lazy-loading these requests would avoid extra API traffic on the common path.🤖 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 208 - 212, The component ReverseProxyModal is eagerly fetching peers and network resources via useFetchApi("/peers") and useFetchApi("/networks/resources"); change this to lazy/gated loading so those requests only run when an L4 target is required (e.g., when the selected protocol/flow indicates TCP/UDP). Implement this by moving or conditionally calling the fetch logic (or passing an enabled flag) tied to the L4 selection state used by the TLS target selector, so peers and resources are requested only after the modal state indicates an L4 flow; update any handlers that open the TLS/L4 target UI to trigger the fetch and surface loading state accordingly.
487-497: Drop the remaining protocol double-cast.
endpointMode as string as ReverseProxyTargetProtocolstill sidesteps the type checker, soServiceModeandReverseProxyTargetProtocolcan drift apart again without a compile-time failure. Prefer an explicit mapper or a shared union here.♻️ Safer direction
+const toL4Protocol = (mode: ServiceMode): ReverseProxyTargetProtocol => { + switch (mode) { + case ServiceMode.TCP: + return ReverseProxyTargetProtocol.TCP; + case ServiceMode.UDP: + return ReverseProxyTargetProtocol.UDP; + case ServiceMode.TLS: + return ReverseProxyTargetProtocol.TLS; + default: + throw new Error(`Unsupported L4 mode: ${mode}`); + } +}; + const l4Target: ReverseProxyTarget = { target_id: tlsPeerId || tlsResourceId || "", target_type: l4TargetType, port: tlsPort, - protocol: endpointMode as string as ReverseProxyTargetProtocol, + protocol: toL4Protocol(endpointMode), host: tlsIsCidrRange ? tlsHost : undefined,🤖 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 487 - 497, The code constructs l4Target with protocol: endpointMode as string as ReverseProxyTargetProtocol which bypasses type safety; replace that double-cast by mapping endpointMode to a ReverseProxyTargetProtocol explicitly (either via a small mapper function or a switch/lookup) and use the mapper's return type so protocol is strongly typed; update any usages of endpointMode in ReverseProxyModal (the l4Target object) to call the mapper (or use a shared union type) instead of the double-cast to ensure compile-time alignment between ServiceMode/endpointMode and ReverseProxyTargetProtocol.
🤖 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/ReverseProxyModal.tsx`:
- Around line 756-808: Add accessible labels for each of the three L4 inputs
(public port, host/IP, destination port) so screen readers can identify them;
locate the three Input components that use setTlsListenPort, setTlsHost, and
setTlsPort and give each a unique id and either a visible <label htmlFor="...">
or an aria-label/aria-labelledby that describes the control (e.g., "Public
port", "Host or IP", "Destination port"). Ensure the host input's
readOnly/disabled states still have an accessible label and update any
placeholder usage so it doesn't serve as the only accessible name.
---
Nitpick comments:
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 208-212: The component ReverseProxyModal is eagerly fetching peers
and network resources via useFetchApi("/peers") and
useFetchApi("/networks/resources"); change this to lazy/gated loading so those
requests only run when an L4 target is required (e.g., when the selected
protocol/flow indicates TCP/UDP). Implement this by moving or conditionally
calling the fetch logic (or passing an enabled flag) tied to the L4 selection
state used by the TLS target selector, so peers and resources are requested only
after the modal state indicates an L4 flow; update any handlers that open the
TLS/L4 target UI to trigger the fetch and surface loading state accordingly.
- Around line 487-497: The code constructs l4Target with protocol: endpointMode
as string as ReverseProxyTargetProtocol which bypasses type safety; replace that
double-cast by mapping endpointMode to a ReverseProxyTargetProtocol explicitly
(either via a small mapper function or a switch/lookup) and use the mapper's
return type so protocol is strongly typed; update any usages of endpointMode in
ReverseProxyModal (the l4Target object) to call the mapper (or use a shared
union type) instead of the double-cast to ensure compile-time alignment between
ServiceMode/endpointMode and ReverseProxyTargetProtocol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b814aa9-b673-45a5-933d-17ec978fbe72
📒 Files selected for processing (2)
src/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyModal.tsx
# Conflicts: # src/modules/reverse-proxy/table/ReverseProxyStatusCell.tsx
lixmal
force-pushed
the
feature/reverse-proxy
branch
from
d4b33bf to
fbe7ff6
Compare
lixmal
force-pushed
the
feature/reverse-proxy
branch
from
22c9fd7 to
5033cae
Compare
…ure/reverse-proxy
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/DeviceCard.tsx (1)
28-32:⚠️ Potential issue | 🟡 MinorMissing
resourceinuseMemodependency array.The computation references
resource?.addressbutresourceis not listed in the dependency array. Ifresourceupdates whiledescription,address, anddeviceremain unchanged,descriptionTextwill be stale.🐛 Proposed fix
const descriptionText = useMemo(() => { return description !== undefined ? description : address || device?.ip || resource?.address; - }, [description, address, device]); + }, [description, address, device, resource]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DeviceCard.tsx` around lines 28 - 32, The useMemo that computes descriptionText references resource?.address but omits resource from its dependency array; update the dependency list for the useMemo that returns descriptionText to include resource (e.g., [description, address, device, resource]) so descriptionText recalculates when resource changes, ensuring the computed value stays current.
🧹 Nitpick comments (3)
src/utils/helpers.ts (1)
266-275: Consider validating edge cases for negative or NaN inputs.The function handles positive durations well, but edge cases may produce unexpected results:
- Negative values (e.g.,
-500) would return"-500ms"NaNwould return"NaNh"(all comparisons return false)🛡️ Suggested defensive handling
export const formatDuration = (ms: number): string => { try { + if (!Number.isFinite(ms) || ms < 0) return "0ms"; if (ms < 1000) return `${ms}ms`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/helpers.ts` around lines 266 - 275, The formatDuration function should defensively handle negative and NaN inputs: detect if ms is NaN or less than 0 and return "0ms" (or clamp to 0 before formatting) instead of producing "-500ms" or "NaNh"; update the logic in formatDuration to validate the input at the top (using Number.isFinite or Number.isNaN checks and a negative check) and then proceed with the existing branch formatting so all outputs remain sensible for invalid inputs.src/assets/icons/ReverseProxyIcon.tsx (1)
11-16: Minor: Redundantfillattribute on SVG element.The
fill="currentColor"on line 11 is redundant since the<path>element on line 14 explicitly sets its own fill. The path's fill takes precedence, so the SVG-level fill has no effect here.🧹 Suggested cleanup
{...iconProperties(props)} - fill={"currentColor"} > <path fill={"currentColor"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/icons/ReverseProxyIcon.tsx` around lines 11 - 16, Remove the redundant fill attribute from the outer SVG element in ReverseProxyIcon (the SVG tag that currently has fill={"currentColor"}); the inner <path> already sets fill="currentColor" so delete the SVG-level fill to clean up the markup while leaving the path's fill intact.src/modules/reverse-proxy/targets/ReverseProxyTargetsTable.tsx (1)
77-77: Remove unnecessary optional chaining on required properties.The
reverseProxyprop andtargetsproperty are both required by their type definitions (ReverseProxyinterface and component props), so optional chaining (?.) is unnecessary. ChangeinitialPageSize={reverseProxy?.targets?.length}toinitialPageSize={reverseProxy.targets.length}for consistency with howtargetsis accessed directly on line 83 (data={reverseProxy.targets}).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/targets/ReverseProxyTargetsTable.tsx` at line 77, The component uses optional chaining on a required prop: change the prop usage in the ReverseProxyTargetsTable component so initialPageSize reads reverseProxy.targets.length instead of reverseProxy?.targets?.length; update the reference tied to initialPageSize (and ensure consistency with the existing data={reverseProxy.targets} usage) so you remove unnecessary optional chaining on reverseProxy and its targets property.
🤖 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/domain/useReverseProxyDomain.ts`:
- Around line 79-104: The parsed domain is only computed on first render (parsed
from parseDomain(reverseProxy.domain, domains)) so subdomain and baseDomain
never update when domains arrives; fix by recomputing parsed when domains or
reverseProxy.domain change (e.g., move parseDomain into a useMemo or recompute
in a useEffect) and update the state via setSubdomain and setBaseDomain when
parsed changes; reference parseDomain, parsed, reverseProxy.domain, domains,
subdomain/setSubdomain and baseDomain/setBaseDomain to locate where to add the
useMemo/useEffect so the derived values reflect the latest domains instead of
sticking to the initial fallback.
- Around line 115-122: isClusterConnected currently compares
reverseProxy.proxy_cluster to d.domain and can return undefined while domains
load; change the comparison to d.target_cluster (i.e., match
reverseProxy.proxy_cluster against d.target_cluster) and coerce the result to a
real boolean (use Boolean(...) or !!) so isClusterConnected always returns
true/false; update the useMemo that references reverseProxy?.proxy_cluster,
domains, ReverseProxyDomainType.FREE and domains?.some(...) accordingly.
In `@src/modules/reverse-proxy/events/ReverseProxyEventsBytesCell.tsx`:
- Around line 19-27: The two byte rows (the elements rendering
ArrowDownIcon/ArrowUpIcon and formatBytes(event.bytes_download ?? 0) /
formatBytes(event.bytes_upload ?? 0)) lack accessible labels; update the JSX in
ReverseProxyEventsBytesCell to include explicit text like "Download" and
"Upload" or add screen-reader-only spans (e.g., <span
className="sr-only">Download:</span> and <span
className="sr-only">Upload:</span>) next to the icons or values so assistive
tech can distinguish bytes_download and bytes_upload for ArrowDownIcon and
ArrowUpIcon rows while preserving the visual layout.
In `@src/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsx`:
- Around line 33-37: The code is rebuilding L4 endpoints using the current
service.listen_port (listenPort), which causes historical events to show updated
ports; change hostWithPort/fullUrl construction to rely only on event-owned
data: use event.port (if provided) to append `:${event.port}` when isL4,
otherwise fall back to event.host without synthesizing service.listen_port;
remove use of service.listen_port in hostWithPort and fullUrl (also update the
similar logic around the block handling lines 52-57) so past events reflect
their original data.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 352-357: The timeout string stored in timeoutOption is being
written directly into opts[request_timeout|session_idle_timeout] without
validation; reuse the shared duration validator used by ReverseProxyTargetModal
(the validator invoked by useReverseProxyTargetOptions) to validate
timeoutOption before assigning it to opts, surface any validation error via the
same error prop used by errorTooltip (instead of only errorTooltip) so the
submit is blocked when the validator reports an error, and apply the same change
to the other occurrence around lines 564-571 (the second timeout assignment) so
malformed durations are rejected client-side.
In `@src/modules/reverse-proxy/ReverseProxyServiceModeSelector.tsx`:
- Around line 67-74: The isL4Supported check incorrectly treats undefined
differently from false; change its definition to use a strict boolean check
(e.g. const isL4Supported = !!domain?.supports_custom_ports or
domain?.supports_custom_ports === true) so explicit false prevents L4 modes;
update the useEffect that uses isL4Supported (and the other occurrence around
isL4ServiceMode/onChange/ServiceMode.HTTP at lines ~100-104) to rely on this
boolean value so unsupported domains reset the selected mode back to
ServiceMode.HTTP.
In `@src/modules/reverse-proxy/table/ReverseProxyClusterCell.tsx`:
- Around line 57-74: The render currently calls isNetBirdHosted() directly (in
ReverseProxyClusterCell.jsx/tsx), which accesses window and breaks static
builds; guard that runtime-only call by checking typeof window !== "undefined"
before invoking isNetBirdHosted(), e.g., compute a boolean like isHosted only
when window exists (use a useMemo/useState + useEffect or inline typeof guard)
and then use that boolean in the JSX branches instead of calling
isNetBirdHosted() during server-side/static render. Ensure all references in the
JSX (the ternary that reads isNetBirdHosted()) use this safe variable so no
window access happens at build time.
In `@src/modules/reverse-proxy/targets/ReverseProxyAddressInput.tsx`:
- Around line 11-16: The validation allows subnet hosts before the resource's
CIDR is loaded because resourceAddress can be empty; update the logic in
ReverseProxyAddressInput (the useMemo/resolution of resourceAddress and the
functions isCidrRange / isValidCidrHost / cidrInfo) to require a non-empty
resourceAddress (resolved CIDR) before treating a target as a SUBNET host — if
resourceAddress is not yet available and target.type === 'SUBNET' return false
from the membership/validation checks (or alternatively accept an injected
initialResourceAddress parameter into the hook and use that for immediate
membership checks) so you never fall back to cidr.isValidAddress(target.host)
without the correct subnet context.
---
Outside diff comments:
In `@src/components/DeviceCard.tsx`:
- Around line 28-32: The useMemo that computes descriptionText references
resource?.address but omits resource from its dependency array; update the
dependency list for the useMemo that returns descriptionText to include resource
(e.g., [description, address, device, resource]) so descriptionText recalculates
when resource changes, ensuring the computed value stays current.
---
Nitpick comments:
In `@src/assets/icons/ReverseProxyIcon.tsx`:
- Around line 11-16: Remove the redundant fill attribute from the outer SVG
element in ReverseProxyIcon (the SVG tag that currently has
fill={"currentColor"}); the inner <path> already sets fill="currentColor" so
delete the SVG-level fill to clean up the markup while leaving the path's fill
intact.
In `@src/modules/reverse-proxy/targets/ReverseProxyTargetsTable.tsx`:
- Line 77: The component uses optional chaining on a required prop: change the
prop usage in the ReverseProxyTargetsTable component so initialPageSize reads
reverseProxy.targets.length instead of reverseProxy?.targets?.length; update the
reference tied to initialPageSize (and ensure consistency with the existing
data={reverseProxy.targets} usage) so you remove unnecessary optional chaining
on reverseProxy and its targets property.
In `@src/utils/helpers.ts`:
- Around line 266-275: The formatDuration function should defensively handle
negative and NaN inputs: detect if ms is NaN or less than 0 and return "0ms" (or
clamp to 0 before formatting) instead of producing "-500ms" or "NaNh"; update
the logic in formatDuration to validate the input at the top (using
Number.isFinite or Number.isNaN checks and a negative check) and then proceed
with the existing branch formatting so all outputs remain sensible for invalid
inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bca7fd80-4d2d-49e8-9407-7a3abdc8dca1
📒 Files selected for processing (34)
src/assets/icons/ReverseProxyIcon.tsxsrc/components/DeviceCard.tsxsrc/components/RadioCard.tsxsrc/components/Select.tsxsrc/contexts/ReverseProxiesProvider.tsxsrc/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyHTTPTargets.tsxsrc/modules/reverse-proxy/ReverseProxyLayer4Content.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/ReverseProxyServiceModeSelector.tsxsrc/modules/reverse-proxy/domain/CustomDomainModal.tsxsrc/modules/reverse-proxy/domain/CustomDomainVerificationModal.tsxsrc/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsxsrc/modules/reverse-proxy/domain/useReverseProxyDomain.tssrc/modules/reverse-proxy/events/ReverseProxyEventsBytesCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsDurationCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsStatusCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsTable.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyClusterCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyNameCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyStatusCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyTable.tsxsrc/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyTypeCell.tsxsrc/modules/reverse-proxy/targets/ReverseProxyAddressInput.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetDevice.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetSelector.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetsTable.tsxsrc/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetActionCell.tsxsrc/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsxsrc/utils/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx
- src/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsx
- src/modules/reverse-proxy/table/ReverseProxyNameCell.tsx
- src/modules/reverse-proxy/table/ReverseProxyTable.tsx
Comment on lines
+33
to
+37
| const listenPort = service?.listen_port; | ||
|
|
||
| const hostWithPort = | ||
| isL4 && listenPort ? `${event.host}:${listenPort}` : event.host || "-"; | ||
| const fullUrl = isL4 ? hostWithPort : `${event.host}${event.path}`; |
There was a problem hiding this comment.
Don't reconstruct past event ports from the current service config.
For L4 rows, the displayed/copied endpoint is built with service.listen_port, not the event itself. If the service port is edited later, older events will show the new port, which makes the event log inaccurate. Prefer event-owned data only here; if the backend does not send the original port, fall back to event.host instead of synthesizing :${listenPort}.
Also applies to: 52-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsx` around
lines 33 - 37, The code is rebuilding L4 endpoints using the current
service.listen_port (listenPort), which causes historical events to show updated
ports; change hostWithPort/fullUrl construction to rely only on event-owned
data: use event.port (if provided) to append `:${event.port}` when isL4,
otherwise fall back to event.host without synthesizing service.listen_port;
remove use of service.listen_port in hostWithPort and fullUrl (also update the
similar logic around the block handling lines 52-57) so past events reflect
their original data.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/modules/reverse-proxy/ReverseProxyServiceModeSelector.tsx (1)
67-74:⚠️ Potential issue | 🟠 MajorUse the boolean value of
supports_custom_ports.
domain?.supports_custom_ports !== undefinedtreatsfalseas supported. Domains that explicitly disallow custom ports still expose TCP/TLS/UDP and won't get reset back to HTTP.Suggested fix
- const isL4Supported = domain?.supports_custom_ports !== undefined; + const isL4Supported = domain?.supports_custom_ports === true;Also applies to: 100-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/ReverseProxyServiceModeSelector.tsx` around lines 67 - 74, The code treats domain?.supports_custom_ports !== undefined as support which incorrectly treats false as supported; update the isL4Supported check to use the actual boolean (e.g. domain?.supports_custom_ports === true or Boolean(domain?.supports_custom_ports)) so domains that explicitly disallow custom ports are not considered L4-supported; locate the isL4Supported definition in ReverseProxyServiceModeSelector, keep the existing useEffect that calls isL4ServiceMode(selected) and onChange(ServiceMode.HTTP) but only trigger that reset when the corrected isL4Supported becomes false.src/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsx (1)
31-37:⚠️ Potential issue | 🟠 MajorDon't rebuild historical L4 endpoints from the current service port.
Lines 33-37 and 55-57 append
service.listen_port, which is today's config, not the port captured with the event. If the service port changes later, old log rows become wrong. Prefer event-owned data only here; until the backend includes the original port, fall back toevent.host.Suggested fix
-export const ReverseProxyEventsUrlCell = ({ event, service }: Props) => { +export const ReverseProxyEventsUrlCell = ({ event }: Props) => { const isL4 = isL4Event(event); - const listenPort = service?.listen_port; - - const hostWithPort = - isL4 && listenPort ? `${event.host}:${listenPort}` : event.host || "-"; + const hostWithPort = event.host || "-"; const fullUrl = isL4 ? hostWithPort : `${event.host}${event.path}`; @@ <CopyToClipboardText message={"URL has been copied to your clipboard"}> <span className="font-mono text-[0.82rem] whitespace-nowrap"> - <span className="text-nb-gray-200">{event.host}</span> - {isL4 && listenPort && ( - <span className="text-nb-gray-300">:{listenPort}</span> - )} + <span className="text-nb-gray-200">{event.host || "-"}</span> {!isL4 && ( <span className="text-nb-gray-300">{event.path}</span> )} </span> </CopyToClipboardText>Also applies to: 52-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsx` around lines 31 - 37, The code in ReverseProxyEventsUrlCell (functions/variables: isL4Event, hostWithPort, fullUrl) is appending the current service.listen_port to L4 events, which rebuilds historical endpoints from today's config; change the logic to use only event-owned data (e.g., event.port if present) and fall back to event.host when event.port is missing — remove usage of service.listen_port in hostWithPort/fullUrl so past events remain accurate.src/modules/reverse-proxy/events/ReverseProxyEventsBytesCell.tsx (1)
19-27:⚠️ Potential issue | 🟡 MinorLabel the byte rows for screen readers.
These two rows are distinguished only by arrow direction and color. Assistive tech will read two unlabeled byte values, so users can't tell which one is download vs upload.
Suggested fix
<div className={"flex gap-2 items-center whitespace-nowrap"}> <ArrowDownIcon size={15} className={cn("text-sky-400")} /> + <span className="sr-only">Download:</span> {formatBytes(event.bytes_download ?? 0)} </div> <div className={"flex gap-2 items-center whitespace-nowrap"}> <ArrowUpIcon size={15} className={cn("text-netbird")} /> + <span className="sr-only">Upload:</span> {formatBytes(event.bytes_upload ?? 0)} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/events/ReverseProxyEventsBytesCell.tsx` around lines 19 - 27, The two byte rows in ReverseProxyEventsBytesCell.tsx (the ArrowDownIcon/formatBytes(event.bytes_download) and ArrowUpIcon/formatBytes(event.bytes_upload) lines) are not labelled for assistive tech; add accessible labels for each row by including a screen-reader-only text node or aria-label indicating "Download" and "Upload" respectively (e.g., add a visually-hidden <span> with "Download" before the download bytes and "Upload" before the upload bytes, or set aria-label on the row divs) so formatBytes(event.bytes_download) and formatBytes(event.bytes_upload) are announced with their context.src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
352-357:⚠️ Potential issue | 🟠 MajorL4 timeout input lacks client-side validation before submission.
The timeout string is written directly to
request_timeoutorsession_idle_timeoutwithout validation. The Input haserrorTooltip={true}but noerrorprop to display validation errors. Invalid duration formats (not matching Gotime.ParseDuration) will only fail after the API request.Consider reusing the duration validator from
useReverseProxyTargetOptionsto validate and display errors:Suggested approach
// Add validation similar to useReverseProxyTargetOptions const DURATION_RE = /^(\d+(\.\d+)?(ns|us|µs|ms|s|m|h))+$/; const timeoutError = useMemo(() => { if (!timeoutOption) return undefined; if (!DURATION_RE.test(timeoutOption)) return 'Invalid duration, use e.g., "10s", "30s", "1m"'; return undefined; }, [timeoutOption]); // Then in the Input: <Input ... error={timeoutError} errorTooltip={true} /> // And block submit: disabled={!canContinueToSettings || !!timeoutError || ...}Also applies to: 564-571
🤖 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 352 - 357, The timeout value from timeoutOption is written straight to request_timeout/session_idle_timeout without client-side validation; add the same duration validation used in useReverseProxyTargetOptions (e.g., DURATION_RE) as a derived timeoutError (useMemo or similar) that returns an error string when timeoutOption doesn't match the duration regex, pass that error to the Input via its error prop while keeping errorTooltip={true}, and prevent form submission by including !!timeoutError in the disabled condition used for continuing/save; update both places referenced (around timeoutOption handling and the other block at lines 564-571) so the UI shows the error and blocks submit prior to calling the API.src/modules/reverse-proxy/targets/ReverseProxyAddressInput.tsx (1)
45-48:⚠️ Potential issue | 🟠 MajorCIDR membership check falls back to syntax-only validation when resource is loading.
When
target.type === SUBNETbutresourceshasn't loaded yet,resourceAddressis empty, makingcidrInfonull. The expression then falls back tocidr.isValidAddress(target.host), which only checks IP syntax—not subnet membership. This allows any valid IP to pass validation until the resource loads.Consider returning
falseuntilresourceAddressis available for subnet targets:Suggested fix
const isValidCidrHost = !isCidrRange || + (isCidrRange && !resourceAddress) ? false : (!!target?.host && (cidrInfo ? isHostInCidrRange : cidr.isValidAddress(target.host)));Or more clearly:
+ const isValidCidrHost = useMemo(() => { + if (!isCidrRange) return true; + if (!resourceAddress) return false; // Still loading + if (!target?.host) return false; + return cidrInfo ? isHostInCidrRange : false; + }, [isCidrRange, resourceAddress, target?.host, cidrInfo, isHostInCidrRange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/targets/ReverseProxyAddressInput.tsx` around lines 45 - 48, The validation currently lets a syntactically valid IP pass when target.type === SUBNET and cidrInfo is null (resources still loading); update the logic in isValidCidrHost so that if isCidrRange is true and target?.type === SUBNET and cidrInfo (or resourceAddress) is not available, the expression returns false (do not fall back to cidr.isValidAddress); otherwise when cidrInfo exists use isHostInCidrRange, and for non-subnet CIDR ranges keep the existing cidr.isValidAddress behavior.
🧹 Nitpick comments (3)
src/modules/reverse-proxy/table/ReverseProxyTypeCell.tsx (1)
54-67: Consider combining identical TCP/UDP icon cases.Both
ServiceMode.TCPandServiceMode.UDPrender the sameArrowRightFromLineIcon. Combining them improves maintainability.♻️ Combine cases
case ServiceMode.TLS: return <LockKeyhole size={size} className={cn("shrink-0", className)} />; case ServiceMode.TCP: - return ( - <ArrowRightFromLineIcon - size={size} - className={cn("shrink-0", className)} - /> - ); case ServiceMode.UDP: return ( <ArrowRightFromLineIcon🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/table/ReverseProxyTypeCell.tsx` around lines 54 - 67, The TCP and UDP branches in ReverseProxyTypeCell both render the same ArrowRightFromLineIcon; consolidate them by combining ServiceMode.TCP and ServiceMode.UDP into a single case (or use a grouped conditional) that returns the ArrowRightFromLineIcon with the existing props (size and className via cn), removing the duplicated JSX to improve maintainability.src/assets/icons/ReverseProxyIcon.tsx (1)
11-16: Minor redundancy:fillspecified on both SVG and path.The
fill="currentColor"on the<svg>element (line 11) is inherited by child elements, making the duplicate on<path>(line 14) redundant. This works correctly but could be simplified.♻️ Optional cleanup
fill={"currentColor"} > - <path - fill={"currentColor"} - d="M11.4488 2.1499..." + <path + d="M11.4488 2.1499..." />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/icons/ReverseProxyIcon.tsx` around lines 11 - 16, In ReverseProxyIcon, the fill="currentColor" is duplicated on both the <svg> element and the child <path>; remove the redundant fill attribute from the <path> (keep the single fill on the <svg>) so the path inherits currentColor and the JSX is simplified while preserving the same rendering.src/modules/reverse-proxy/ReverseProxyHTTPTargets.tsx (1)
138-147: Redundant className in conditional.The
cn("w-full mt-1", targets?.length > 0 && "mt-1")always appliesmt-1regardless of the condition. If different margins were intended, adjust the logic; otherwise, simplify.♻️ Simplify className
<Button variant="dotted" - className={cn("w-full mt-1", targets?.length > 0 && "mt-1")} + className="w-full mt-1" size="sm"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/ReverseProxyHTTPTargets.tsx` around lines 138 - 147, The className expression on the Button is redundant because cn("w-full mt-1", targets?.length > 0 && "mt-1") always applies "mt-1"; update the Button's className (in ReverseProxyHTTPTargets.tsx) to a single, non-redundant value—e.g., replace the cn(...) call with cn("w-full mt-1") if the margin should always be mt-1, or adjust the conditional branch to return a different class when targets?.length > 0 (for example "mt-2") if a different margin was intended; modify the cn usage and keep onClick={onAddTarget} and disabled as-is.
🤖 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/domain/CustomDomainModal.tsx`:
- Around line 103-131: The component calls isNetBirdHosted() (which reads
window.location.hostname) directly and must be made safe for server
environments; update both usages in CustomDomainModal (and also apply the same
guard to CustomDomainVerificationModal mentioned in the review) to check typeof
window !== "undefined" && isNetBirdHosted() before rendering the host-specific
Callout, or alternatively add a "use client" directive at the top of the
component file if the component is intended to be client-only; ensure the unique
symbol isNetBirdHosted and the component names CustomDomainModal and
CustomDomainVerificationModal are the targets of this change.
In `@src/modules/reverse-proxy/table/ReverseProxyStatusCell.tsx`:
- Around line 35-41: The HTTP rendering branch in ReverseProxyStatusCell.tsx
currently doesn't consider the failure flags so services with errors show as
pending; update the HTTP path to consume the same computed booleans (hasError
and isTunnelNotCreated) and also include a check for
ReverseProxyStatus.CERTIFICATE_FAILED (using meta?.status and
dataRef.current?.meta?.status like the L4 branch does) so the HTTP badges render
the appropriate error/failure (red) and certificate-failed states instead of
falling through to the yellow pending badges; apply the same adjustments to the
other status-rendering code in the file that mirrors lines 61-137.
In `@src/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsx`:
- Around line 20-31: The L4 render path in isL4Mode(reverseProxy.mode) assumes
reverseProxy.targets[0] exists; guard the code that computes const target =
reverseProxy?.targets?.[0] and the subsequent address calculation so it first
checks for a missing target and returns a safe fallback (e.g., a
placeholder/null cell) instead of attempting to access host/port; update the
branch that returns <ReverseProxyTargetDevice ...> to only render that component
when target is defined (otherwise return the fallback) and keep the rest of the
logic (address construction and wrapperClassName) inside that guarded block so
rendering never throws when target is undefined.
In `@src/modules/reverse-proxy/table/ReverseProxyTypeCell.tsx`:
- Around line 15-20: In the useMemo that computes serviceModeLabel (the
serviceModeLabel constant in ReverseProxyTypeCell.tsx), guard the SERVICE_MODES
lookup against invalid reverseProxy.mode values by checking that
SERVICE_MODES?.[reverseProxy.mode] exists before accessing .label; if it doesn't
exist, return a safe fallback (e.g., "HTTP/S" or a default label) so that
SERVICE_MODES?.[reverseProxy.mode].label cannot throw when reverseProxy.mode is
unexpected.
In `@src/utils/helpers.ts`:
- Around line 266-273: The formatDuration function currently formats malformed
numeric inputs into "NaN" or "Infinity" strings; before any computations in
formatDuration, validate the input with Number.isFinite(ms) (and optionally ms
>= 0) and return a safe default like "0ms" for non-finite or negative values so
the subsequent checks (ms < 1000, ms < 60000, etc.) never produce NaN/Infinity
outputs.
---
Duplicate comments:
In `@src/modules/reverse-proxy/events/ReverseProxyEventsBytesCell.tsx`:
- Around line 19-27: The two byte rows in ReverseProxyEventsBytesCell.tsx (the
ArrowDownIcon/formatBytes(event.bytes_download) and
ArrowUpIcon/formatBytes(event.bytes_upload) lines) are not labelled for
assistive tech; add accessible labels for each row by including a
screen-reader-only text node or aria-label indicating "Download" and "Upload"
respectively (e.g., add a visually-hidden <span> with "Download" before the
download bytes and "Upload" before the upload bytes, or set aria-label on the
row divs) so formatBytes(event.bytes_download) and
formatBytes(event.bytes_upload) are announced with their context.
In `@src/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsx`:
- Around line 31-37: The code in ReverseProxyEventsUrlCell (functions/variables:
isL4Event, hostWithPort, fullUrl) is appending the current service.listen_port
to L4 events, which rebuilds historical endpoints from today's config; change
the logic to use only event-owned data (e.g., event.port if present) and fall
back to event.host when event.port is missing — remove usage of
service.listen_port in hostWithPort/fullUrl so past events remain accurate.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 352-357: The timeout value from timeoutOption is written straight
to request_timeout/session_idle_timeout without client-side validation; add the
same duration validation used in useReverseProxyTargetOptions (e.g.,
DURATION_RE) as a derived timeoutError (useMemo or similar) that returns an
error string when timeoutOption doesn't match the duration regex, pass that
error to the Input via its error prop while keeping errorTooltip={true}, and
prevent form submission by including !!timeoutError in the disabled condition
used for continuing/save; update both places referenced (around timeoutOption
handling and the other block at lines 564-571) so the UI shows the error and
blocks submit prior to calling the API.
In `@src/modules/reverse-proxy/ReverseProxyServiceModeSelector.tsx`:
- Around line 67-74: The code treats domain?.supports_custom_ports !== undefined
as support which incorrectly treats false as supported; update the isL4Supported
check to use the actual boolean (e.g. domain?.supports_custom_ports === true or
Boolean(domain?.supports_custom_ports)) so domains that explicitly disallow
custom ports are not considered L4-supported; locate the isL4Supported
definition in ReverseProxyServiceModeSelector, keep the existing useEffect that
calls isL4ServiceMode(selected) and onChange(ServiceMode.HTTP) but only trigger
that reset when the corrected isL4Supported becomes false.
In `@src/modules/reverse-proxy/targets/ReverseProxyAddressInput.tsx`:
- Around line 45-48: The validation currently lets a syntactically valid IP pass
when target.type === SUBNET and cidrInfo is null (resources still loading);
update the logic in isValidCidrHost so that if isCidrRange is true and
target?.type === SUBNET and cidrInfo (or resourceAddress) is not available, the
expression returns false (do not fall back to cidr.isValidAddress); otherwise
when cidrInfo exists use isHostInCidrRange, and for non-subnet CIDR ranges keep
the existing cidr.isValidAddress behavior.
---
Nitpick comments:
In `@src/assets/icons/ReverseProxyIcon.tsx`:
- Around line 11-16: In ReverseProxyIcon, the fill="currentColor" is duplicated
on both the <svg> element and the child <path>; remove the redundant fill
attribute from the <path> (keep the single fill on the <svg>) so the path
inherits currentColor and the JSX is simplified while preserving the same
rendering.
In `@src/modules/reverse-proxy/ReverseProxyHTTPTargets.tsx`:
- Around line 138-147: The className expression on the Button is redundant
because cn("w-full mt-1", targets?.length > 0 && "mt-1") always applies "mt-1";
update the Button's className (in ReverseProxyHTTPTargets.tsx) to a single,
non-redundant value—e.g., replace the cn(...) call with cn("w-full mt-1") if the
margin should always be mt-1, or adjust the conditional branch to return a
different class when targets?.length > 0 (for example "mt-2") if a different
margin was intended; modify the cn usage and keep onClick={onAddTarget} and
disabled as-is.
In `@src/modules/reverse-proxy/table/ReverseProxyTypeCell.tsx`:
- Around line 54-67: The TCP and UDP branches in ReverseProxyTypeCell both
render the same ArrowRightFromLineIcon; consolidate them by combining
ServiceMode.TCP and ServiceMode.UDP into a single case (or use a grouped
conditional) that returns the ArrowRightFromLineIcon with the existing props
(size and className via cn), removing the duplicated JSX to improve
maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c82c1066-bc23-4c6b-93a0-94a242515988
📒 Files selected for processing (34)
src/assets/icons/ReverseProxyIcon.tsxsrc/components/DeviceCard.tsxsrc/components/RadioCard.tsxsrc/components/Select.tsxsrc/contexts/ReverseProxiesProvider.tsxsrc/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyHTTPTargets.tsxsrc/modules/reverse-proxy/ReverseProxyLayer4Content.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/ReverseProxyServiceModeSelector.tsxsrc/modules/reverse-proxy/domain/CustomDomainModal.tsxsrc/modules/reverse-proxy/domain/CustomDomainVerificationModal.tsxsrc/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsxsrc/modules/reverse-proxy/domain/useReverseProxyDomain.tssrc/modules/reverse-proxy/events/ReverseProxyEventsBytesCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsDurationCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsRequestCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsStatusCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsTable.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyClusterCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyNameCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyStatusCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyTable.tsxsrc/modules/reverse-proxy/table/ReverseProxyTargetsCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyTypeCell.tsxsrc/modules/reverse-proxy/targets/ReverseProxyAddressInput.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetDevice.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetSelector.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetsTable.tsxsrc/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetActionCell.tsxsrc/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsxsrc/utils/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx
- src/modules/reverse-proxy/targets/flat/ReverseProxyFlatTargetsTable.tsx
- src/contexts/ReverseProxiesProvider.tsx
heisbrot
approved these changes
Mar 18, 2026
This was referenced Mar 19, 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
Backend PR
netbirdio/netbird#5530
Summary by CodeRabbit
New Features
Bug Fixes / UX