Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ServiceTargetOptions to reverse-proxy interfaces and extends ReverseProxyTarget. Introduces a tabbed ReverseProxyTarget modal with an Options tab, a useReverseProxyTargetOptions hook, custom header management, timeout and header validation, and HelpTooltip API enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Modal as ReverseProxyTargetModal
participant Hook as useReverseProxyTargetOptions
participant Headers as ReverseProxyTargetCustomHeaders
participant API as Save Handler
User->>Modal: Open "Add/Edit Target"
Modal->>Hook: initialize (initialOptions?)
Modal->>Headers: render headerEntries UI
User->>Headers: add/update/remove header
Headers-->>Hook: update headerEntries
User->>Modal: edit Details (host/port) or switch to Options
User->>Hook: updateOption (timeout, path_rewrite, skip_tls_verify)
Hook-->>Modal: provide errors / getTargetOptions
User->>Modal: Click Save
Modal->>Hook: getTargetOptions()
Hook-->>Modal: combined options or undefined
Modal->>API: submit target payload (includes options if present)
API-->>Modal: persist / respond
Modal-->>User: close modal / show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
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 |
lixmal
changed the title
![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
🤖 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/targets/ReverseProxyTargetModal.tsx`:
- Around line 108-113: The validateTimeout function in ReverseProxyTargetModal
currently only checks format (DURATION_RE) but must also enforce the
UI-documented 5m cap; update validateTimeout to parse the duration string
(handle suffixes s, m) into seconds or milliseconds, compare against a 5-minute
limit (300s/300000ms), and return a validation error like "Duration must be <=
5m" when exceeded; apply the same cap enforcement to the other timeout validator
used elsewhere in this module (the same validation logic referenced in the
review) so client-side validation prevents values >5m before downstream
rejection.
- Around line 57-60: The BLOCKED_HEADERS set in ReverseProxyTargetModal.tsx
lists the hop-by-hop header as "trailers" (plural) which is incorrect; change
the entry in the BLOCKED_HEADERS Set from "trailers" to "trailer" (singular) so
the reserved Trailer header is correctly blocked by the validation logic that
uses BLOCKED_HEADERS.
- Around line 795-801: The remove-header button in ReverseProxyTargetModal is
icon-only (Button containing MinusCircleIcon) and lacks an accessible name;
update the Button (the element that calls removeHeader) to include an accessible
label such as aria-label="Remove header" or aria-labelledby (or add
visually-hidden text) so screen readers can identify it, ensuring the label
clearly describes the action (e.g., "Remove header") and keeping the onClick
handler removeHeader(entry.id) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac42def0-8d74-4c60-98cc-f7b8d1c7d804
📒 Files selected for processing (2)
src/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx
lixmal
force-pushed
the
feature/per-target-options
branch
from
d76fcf2 to
7b61201
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx (1)
108-113:⚠️ Potential issue | 🟡 MinorEnforce the documented 5m timeout cap in validator.
Line 108-113 validates syntax but not the max duration promised in Line 726-727, so oversized values still pass client-side.
Proposed fix
const DURATION_RE = /^(\d+(\.\d+)?(ns|us|µs|ms|s|m|h))+$/; +const DURATION_PART_RE = /(\d+(\.\d+)?)(ns|us|µs|ms|s|m|h)/g; +const MAX_TIMEOUT_MS = 5 * 60 * 1000; + +function durationToMs(input: string): number | null { + let total = 0; + let consumed = 0; + for (const match of input.matchAll(DURATION_PART_RE)) { + const value = Number(match[1]); + const unit = match[3]; + consumed += match[0].length; + const factor = + unit === "h" + ? 3_600_000 + : unit === "m" + ? 60_000 + : unit === "s" + ? 1_000 + : unit === "ms" + ? 1 + : unit === "us" || unit === "µs" + ? 0.001 + : 0.000001; + total += value * factor; + } + return consumed === input.length ? total : null; +} function validateTimeout(timeout: string): string | undefined { if (!timeout) return undefined; if (!DURATION_RE.test(timeout)) return 'Invalid duration, use e.g. "10s", "30s", "1m"'; + const timeoutMs = durationToMs(timeout); + if (timeoutMs === null) + return 'Invalid duration, use e.g. "10s", "30s", "1m"'; + if (timeoutMs > MAX_TIMEOUT_MS) return "Timeout must be 5m or less"; return undefined; }🤖 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 108 - 113, The validateTimeout function currently only checks syntax via DURATION_RE but must also enforce the documented 5m cap; update validateTimeout to, after DURATION_RE.test(timeout) succeeds, parse the duration string (supporting s, m units as used by DURATION_RE), convert to seconds or milliseconds, and return an error if it exceeds 5 minutes (e.g., "Timeout must be <= 5m"); keep the existing error for invalid format and return undefined only when format is valid and value <= 5 minutes.
🧹 Nitpick comments (1)
src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx (1)
160-161: Reset tab state when options are not selectable.Consider forcing
tab="details"when the modal opens or whenhasTargetbecomes false, to avoid landing on a disabled Options context.Proposed refactor
-import React, { useCallback, useMemo, useRef, useState } from "react"; +import React, { useCallback, useEffect, useMemo, useRef, useState } from "react"; const [tab, setTab] = useState("details"); + useEffect(() => { + if (!open || !hasTarget) setTab("details"); + }, [open, hasTarget]);Also applies to: 404-417
🤖 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 160 - 161, The tab state (const [tab, setTab] = useState("details") in ReverseProxyTargetModal) can leave the UI on a disabled "Options" tab when a target is not selectable; add an effect that resets the tab to "details" whenever the modal opens or whenever hasTarget becomes false—e.g. in a useEffect that watches the modal open flag (isOpen/open) and hasTarget and calls setTab("details") when the modal is opened or hasTarget is falsy so the user never lands on a disabled Options context.
🤖 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/targets/ReverseProxyTargetModal.tsx`:
- Around line 108-113: The validateTimeout function currently only checks syntax
via DURATION_RE but must also enforce the documented 5m cap; update
validateTimeout to, after DURATION_RE.test(timeout) succeeds, parse the duration
string (supporting s, m units as used by DURATION_RE), convert to seconds or
milliseconds, and return an error if it exceeds 5 minutes (e.g., "Timeout must
be <= 5m"); keep the existing error for invalid format and return undefined only
when format is valid and value <= 5 minutes.
---
Nitpick comments:
In `@src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx`:
- Around line 160-161: The tab state (const [tab, setTab] = useState("details")
in ReverseProxyTargetModal) can leave the UI on a disabled "Options" tab when a
target is not selectable; add an effect that resets the tab to "details"
whenever the modal opens or whenever hasTarget becomes false—e.g. in a useEffect
that watches the modal open flag (isOpen/open) and hasTarget and calls
setTab("details") when the modal is opened or hasTarget is falsy so the user
never lands on a disabled Options context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae4e5f07-798a-47b5-b1a6-895861c2f6eb
📒 Files selected for processing (2)
src/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx
…ader into separate component
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/HelpTooltip.tsx (1)
35-67: Minor: Remove unnecessary fragment wrapper.The empty fragment (
<>...</>) wrapping the singleFullTooltipchild is redundant and can be removed for cleaner JSX.♻️ Suggested cleanup
return ( - <> - <FullTooltip - interactive={interactive} - ... - </FullTooltip> - </> + <FullTooltip + interactive={interactive} + ... + </FullTooltip> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpTooltip.tsx` around lines 35 - 67, The top-level empty fragment wrapping the FullTooltip is redundant; remove the surrounding <>...</> so the component returns FullTooltip directly. Edit the JSX where FullTooltip (and its children: the children conditional and the HelpCircle trigger) is the sole root element and delete the fragment tokens to clean up the markup while keeping props and className/children logic intact.src/modules/reverse-proxy/targets/useReverseProxyTargetOptions.ts (1)
59-80: NarrowupdateOptionto the fields this hook actually owns.
updateOptioncurrently accepts everykeyof ServiceTargetOptions, butgetTargetOptions()always rebuildscustom_headersfromheaderEntries. A caller can setcustom_headerssuccessfully and still lose that value on save.Suggested fix
+type ManagedTargetOptionKey = Exclude< + keyof ServiceTargetOptions, + "custom_headers" +>; + const updateOption = useCallback( - <K extends keyof ServiceTargetOptions>( + <K extends ManagedTargetOptionKey>( key: K, value: ServiceTargetOptions[K], ) => { setTargetOptions((prev) => ({ ...prev, [key]: value })); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/targets/useReverseProxyTargetOptions.ts` around lines 59 - 80, updateOption is typed to allow any keyof ServiceTargetOptions which lets callers set custom_headers even though getTargetOptions always rebuilds custom_headers from headerEntries; restrict updateOption so it cannot accept 'custom_headers' by narrowing its generic to exclude that key (e.g. define an OwnedKeys type = Exclude<keyof ServiceTargetOptions, 'custom_headers'> and change the updateOption signature to use K extends OwnedKeys) and keep using setTargetOptions as-is; this prevents callers from writing custom_headers that will later be overwritten by getTargetOptions which reads headerEntries.
🤖 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/targets/ReverseProxyTargetCustomHeaders.tsx`:
- Around line 145-164: The header name and value Input components are currently
unlabeled for assistive tech; add programmatic accessible names to each Input
(the ones rendering entry.name and entry.value) by providing either an explicit
aria-label (e.g., "Header name for entry {entry.id}" and "Header value for entry
{entry.id}") or by associating a visible <label> with a unique id for those
Inputs (use entry.id to generate the id), ensuring the inputs remain tied to
updateHeaderEntry(entry.id, ...) and error display via headerErrors[index]; do
not change other behavior.
- Around line 98-104: Validation currently allows entries with an empty name and
a non-empty value (which headerEntriesToRecord later drops); update the
validation logic in the header validation block (the headerEntries map that
builds headerErrors using validateHeaderName and validateHeaderValue) to treat
any entry where name is blank but value is non-blank as a name error.
Concretely, augment the name validation for each entry (in the same place
validateHeaderName(entry.name, allHeaderNames) is called) to also return an
error when entry.name is empty/whitespace while entry.value is non-empty, so
those value-only rows surface a user-facing name error instead of being silently
discarded by headerEntriesToRecord.
In `@src/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsx`:
- Around line 635-639: The help text under the Request Timeout field in
ReverseProxyTargetModal (Label "Request Timeout" and the HelpText element)
incorrectly says "Leave this field empty for no timeout" but the backend treats
empty as "use the default"; update the copy to match backend behavior (e.g.,
"Leave this field empty to use the default timeout") while preserving the "Max
time... (max 5m)" note so the UI message aligns with the actual fallback.
---
Nitpick comments:
In `@src/components/HelpTooltip.tsx`:
- Around line 35-67: The top-level empty fragment wrapping the FullTooltip is
redundant; remove the surrounding <>...</> so the component returns FullTooltip
directly. Edit the JSX where FullTooltip (and its children: the children
conditional and the HelpCircle trigger) is the sole root element and delete the
fragment tokens to clean up the markup while keeping props and
className/children logic intact.
In `@src/modules/reverse-proxy/targets/useReverseProxyTargetOptions.ts`:
- Around line 59-80: updateOption is typed to allow any keyof
ServiceTargetOptions which lets callers set custom_headers even though
getTargetOptions always rebuilds custom_headers from headerEntries; restrict
updateOption so it cannot accept 'custom_headers' by narrowing its generic to
exclude that key (e.g. define an OwnedKeys type = Exclude<keyof
ServiceTargetOptions, 'custom_headers'> and change the updateOption signature to
use K extends OwnedKeys) and keep using setTargetOptions as-is; this prevents
callers from writing custom_headers that will later be overwritten by
getTargetOptions which reads headerEntries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 487f3cfd-cdf8-4a8d-b842-af28b3796713
📒 Files selected for processing (5)
src/components/HelpTooltip.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetCustomHeaders.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsxsrc/modules/reverse-proxy/targets/useReverseProxyTargetOptions.ts
heisbrot
approved these changes
Mar 6, 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.
Adds an Options tab to the reverse proxy target modal with skip TLS verification, request timeout, path rewrite, and custom headers settings per target.
See netbirdio/netbird#5501
Summary by CodeRabbit