Support optional subdomain for reverse proxy domains#589
Conversation
📝 WalkthroughWalkthroughAn optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
251-253: Consider derivingsubdomainRequiredonce to avoid logic drift.The same condition is duplicated in validation and render props. Hoisting it improves maintainability and keeps both paths guaranteed in sync.
🧹 Suggested cleanup
const selectedDomain = useMemo( () => domains?.find( (d) => d.domain === baseDomain || d.target_cluster === baseDomain, ), [domains, baseDomain], ); + const subdomainRequired = selectedDomain?.require_subdomain === true; const canContinueToSettings = useMemo(() => { - const subdomainRequired = - selectedDomain?.require_subdomain === true; const isSubdomainValid = baseDomain.length > 0 && !domainAlreadyExists && (subdomain.length > 0 || !subdomainRequired); @@ }, [ subdomain, baseDomain, domainAlreadyExists, - selectedDomain, + subdomainRequired, serviceMode, @@ <ReverseProxyDomainInput subdomain={subdomain} onSubdomainChange={setSubdomain} baseDomain={baseDomain} onBaseDomainChange={setBaseDomain} domainAlreadyExists={domainAlreadyExists} - subdomainRequired={selectedDomain?.require_subdomain === true} + subdomainRequired={subdomainRequired} clusterOffline={Also applies to: 271-271, 452-452
🤖 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 251 - 253, Derive and reuse a single boolean for the "require subdomain" check instead of repeating the condition: define const subdomainRequired = selectedDomain?.require_subdomain === true (or similar) once near the top of ReverseProxyModal and replace any duplicated checks in the validation logic and in the render/JSX props with that variable (references: subdomainRequired, selectedDomain, validation function, and render props using the same condition) so both validation and rendering always stay in sync.src/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsx (1)
35-37: Expose required state to the input when subdomain is mandatory.The validation logic now distinguishes required vs optional subdomain; reflecting that via input attributes will improve accessibility and browser semantics.
♿ Suggested update
<Input autoFocus value={subdomain} + required={subdomainRequired} + aria-required={subdomainRequired} onChange={(e) => { onSubdomainChange( e.target.value.toLowerCase().replace(/[^a-z0-9-]/g, ""), ); }}Also applies to: 54-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsx` around lines 35 - 37, The subdomain input should reflect the validation state: in ReverseProxyDomainInput, when subdomainRequired is true set the subdomain input element's required attribute and aria-required="true" (and remove/set to false when not required) so browsers and assistive tech know it's mandatory; update both places where the subdomain input is rendered (the block using subdomainRequired around the helper text and the repeated rendering at the second occurrence) to toggle required/aria-required based on the subdomainRequired symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsx`:
- Around line 35-37: The subdomain input should reflect the validation state: in
ReverseProxyDomainInput, when subdomainRequired is true set the subdomain input
element's required attribute and aria-required="true" (and remove/set to false
when not required) so browsers and assistive tech know it's mandatory; update
both places where the subdomain input is rendered (the block using
subdomainRequired around the helper text and the repeated rendering at the
second occurrence) to toggle required/aria-required based on the
subdomainRequired symbol.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 251-253: Derive and reuse a single boolean for the "require
subdomain" check instead of repeating the condition: define const
subdomainRequired = selectedDomain?.require_subdomain === true (or similar) once
near the top of ReverseProxyModal and replace any duplicated checks in the
validation logic and in the render/JSX props with that variable (references:
subdomainRequired, selectedDomain, validation function, and render props using
the same condition) so both validation and rendering always stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0f09192-e4b7-4386-be90-4ed6e9b5615e
📒 Files selected for processing (4)
src/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsxsrc/modules/reverse-proxy/domain/useReverseProxyDomain.ts
Summary
require_subdomainfield toReverseProxyDomaininterfacerequire_subdomainis not explicitlytrue, subdomain input becomes optionalparseDomainto handle bare domain when editing existing servicesfullDomainassembly for empty subdomain caseBackend: netbirdio/netbird#5628