fix(reverse-proxy): allow multi-label subdomains in service input - #728
fix(reverse-proxy): allow multi-label subdomains in service input#728khannoussi-malek wants to merge 2 commits into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReverse-proxy subdomain handling now uses shared sanitization and validation helpers across initialization, input errors, and modal progression. A standalone test script covers normalization and dot-separated subdomain validity cases. ChangesReverse-proxy subdomain validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/modules/reverse-proxy/domain/subdomain.test.tsOops! Something went wrong! :( ESLint: 9.39.3 TypeError: Converting circular structure to JSON src/modules/reverse-proxy/domain/subdomain.tsOops! Something went wrong! :( ESLint: 9.39.3 TypeError: Converting circular structure to JSON 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.
Pull request overview
This PR fixes the Reverse Proxy “Add Service” modal so operators can enter multi-label subdomains (e.g. dev.app) without dots being stripped, aligning the UI with what the management API already supports.
Changes:
- Introduces shared subdomain helpers (
sanitizeSubdomain,isValidSubdomain) to separate live input filtering from submit/flow validation. - Updates the domain input to preserve dots during typing and show an inline validation error for malformed subdomains.
- Tightens modal progression gating to require a syntactically valid subdomain (when present/required) before continuing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/reverse-proxy/ReverseProxyModal.tsx | Adds explicit subdomain validity gating for modal navigation/Continue behavior. |
| src/modules/reverse-proxy/domain/useReverseProxyDomain.ts | Seeds initial subdomain using the shared sanitizer to avoid dot-stripping from resource names. |
| src/modules/reverse-proxy/domain/subdomain.ts | Adds centralized sanitize/validate helpers for subdomain handling (including multi-label). |
| src/modules/reverse-proxy/domain/subdomain.test.ts | Adds a standalone tsx-runnable test script covering sanitize/validate cases. |
| src/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsx | Uses shared sanitizer on change and shows inline error on invalid subdomain syntax. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [subdomain, setSubdomain] = useState(() => { | ||
| return ( | ||
| parsed?.subdomain || | ||
| initialSubdomain | ||
| ?.toLowerCase() | ||
| .replace(/\s+/g, "-") | ||
| .replace(/[^a-z0-9-]/g, "") || | ||
| "" | ||
| sanitizeSubdomain(initialSubdomain?.replace(/\s+/g, "-") ?? "") | ||
| ); |
There was a problem hiding this comment.
Traced this and the bug is real, but I would rather not fix it in this PR.
Confirmed: domains comes from useFetchApi in ReverseProxiesProvider and is passed straight into the modal, which mounts on modalOpen with no loading guard. On a cold SWR cache, parseDomain runs with domains === undefined, falls through to the first-dot split, and dev.app.example.com initializes as subdomain dev + baseDomain app.example.com. The useState initializer never re-runs when domains arrive, so it stays wrong.
Two reasons to keep it separate:
- It is pre-existing on
main. Both the first-dot fallback and the once-only initializer predate this PR, and nothing here changes that path — this PR only touches the sanitize/validate step. You are right that multi-label domains make it easier to hit, but that is a change in the data, not in the code path. - It is the edit flow, not the input-stripping bug in Reverse Proxy service modal strips dots from the subdomain input, blocking nested subdomains the API accepts #667, and the fix needs a dirty-ref plus a sync effect — new state machinery that would not be covered by this PR title or its tests.
Happy to open a follow-up issue with the above repro, or send a separate PR if a maintainer would rather have it bundled.
| // Live input filter. Dots are kept so multi-label subdomains ("dev.app") can | ||
| // be typed at all — dot placement is only checked on submit, because an | ||
| // anchored check would reject valid in-progress input like "dev." mid-typing. |
There was a problem hiding this comment.
Fixed in ead8c19 — the comment was stale against my own final code. It now says the sanitizer deliberately skips dot-placement checks (so dev. survives mid-typing), and isValidSubdomain carries a note that it drives both the inline error and the modal gate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/reverse-proxy/domain/subdomain.ts`:
- Line 10: Update SUBDOMAIN_PATTERN so every dot-separated DNS label begins and
ends with an alphanumeric character while retaining lowercase letters, digits,
and internal hyphens. Add regression cases covering leading-hyphen,
trailing-hyphen, and single-hyphen labels, and ensure the existing input
validation and modal gating use the corrected pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3518a10e-3fe2-4b3f-8b81-71ce0279bde7
📒 Files selected for processing (5)
src/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/domain/ReverseProxyDomainInput.tsxsrc/modules/reverse-proxy/domain/subdomain.test.tssrc/modules/reverse-proxy/domain/subdomain.tssrc/modules/reverse-proxy/domain/useReverseProxyDomain.ts
Issue ticket number and link
Fixes #667 — #667
The subdomain input in the Reverse Proxy "Add Service" modal stripped dots on
every keystroke, making it impossible to enter multi-label subdomains (e.g.
dev.app.example.com) even though the management API already accepts andprovisions them correctly.
Changes
src/modules/reverse-proxy/domain/subdomain.tsholding the two concernsas separate pure functions, so the live filter and the submit-time check
can't drift apart:
sanitizeSubdomain— the live per-keystroke filter, now[^a-z0-9.-].Deliberately not anchored: users type incrementally, and an anchored
pattern would reject valid in-progress input like
dev.on the way todev.app.isValidSubdomain— anchored^[a-z0-9-]+(\.[a-z0-9-]+)*$, so emptylabels (leading/trailing dot,
dev..app) still can't reach the API.ReverseProxyDomainInput.tsxusessanitizeSubdomaininonChangeandsurfaces an inline error for malformed input, matching how the existing
domainAlreadyExistserror already behaves in this field.ReverseProxyModal.tsxgatescanContinueToSettingsonisValidSubdomain.Previously the only subdomain check was
length > 0, which was sufficientonly because the input made malformed values unreachable — allowing dots
removes that guarantee, so the check now has to be explicit.
useReverseProxyDomain.tsuses the same helper when seeding the field froma resource name (
initialSubdomain). This was the same dot-stripping bug ona sibling path: a resource named
dev.appused to seeddevapp.subdomain.test.tscovers both functions, following the existing standalonescript pattern in
src/utils/ip.test.ts(run withnpx tsx src/modules/reverse-proxy/domain/subdomain.test.ts) — the repo hasno unit-test runner wired up, so I matched what's already there rather than
introducing one.
Testing
npx tsx src/modules/reverse-proxy/domain/subdomain.test.ts— 19 cases pass,covering
dev.app/a.b.c.dpreserved, and.app,dev.,dev..app,..rejected.npm run build— passes.running locally), so the modal flow itself is unverified by me. The changed
logic is pure and covered by the test above;
fullDomaincomposition inuseReverseProxyDomainis untouched, sodev.app+example.comstillyields
dev.app.example.com.Note:
npm run lintis currently broken onmain(next lintwas removed inNext 16), so I verified formatting with the repo's pinned Prettier and
typechecking via
next buildinstead.Documentation
Select exactly one:
This is a bug fix that brings the UI in line with what the management API
already accepts. No documented behavior changes — multi-label subdomains were
already supported server-side, they just couldn't be typed.
Docs PR URL (required if "docs added" is checked)
n/a
E2E tests
management-cloud-tag: main
reverse-proxy-tag: main
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Tests