[reverse-proxy] Support multiple port mappings per service - #723
[reverse-proxy] Support multiple port mappings per service#723heywander wants to merge 3 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 Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReverse-proxy L4 services now support multiple TCP, UDP, and TLS port mappings, mode-aware domain conflict checks, separate timeout settings, updated payload handling, and expanded end-to-end coverage. ChangesReverse-proxy L4 services
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant ReverseProxyModal
participant MappingEditor
participant ReverseProxyAPI
Dashboard->>ReverseProxyModal: configure an L4 service
ReverseProxyModal->>MappingEditor: render port mappings
MappingEditor-->>ReverseProxyModal: return validated mappings
ReverseProxyModal->>ReverseProxyAPI: submit service and port mappings
ReverseProxyAPI-->>ReverseProxyModal: return creation result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/reverse-proxy-services-https.spec.ts (1)
1-1: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing
{ force: true }on mapping-protocol combobox clicks (same root cause across two files). These Radix Select triggers are clicked inside the service-creation modal, where every sibling trigger click (service-mode-select-button,service-mode-option-*) uses{ force: true }to avoid the documented Radix outside-click/pointer-events timing issue; these mapping-protocol trigger clicks don't, risking intermittent flakiness.
e2e/tests/reverse-proxy-services-https.spec.ts#L151-155: add{ force: true }to bothsecondMapping.getByRole("combobox").click()calls.e2e/tests/reverse-proxy-services-tcp.spec.ts#L94-94: add{ force: true }toudpMapping.getByRole("combobox").click().🤖 Prompt for 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. In `@e2e/tests/reverse-proxy-services-https.spec.ts` at line 1, Update the mapping-protocol combobox clicks in the HTTPS and TCP reverse-proxy tests to pass { force: true }, including both clicks on secondMapping and the click on udpMapping. Keep the existing selectors and surrounding test flow unchanged.Source: Path instructions
🧹 Nitpick comments (1)
e2e/tests/reverse-proxy-services-https.spec.ts (1)
221-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
.locator("..")parent traversal; prefer a dedicateddata-testidcontainer.
page.getByText("HTTPS Targets").locator("..")climbs to the text node's parent to scope the subsequent table lookup. This is brittle — any markup change (e.g., wrapping the label in an extra<span>) breaks the selector silently. As per path instructions, "Always usedata-testidselectors viapage.getByTestId()for element selection in Playwright tests."🤖 Prompt for 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. In `@e2e/tests/reverse-proxy-services-https.spec.ts` around lines 221 - 222, Replace the parent traversal in the HTTPS Targets assertion with a dedicated data-testid container selected via page.getByTestId(). Scope the existing table row count assertion to that test-id element, and ensure the corresponding container markup exposes the stable test id.Source: Path instructions
🤖 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 `@e2e/tests/reverse-proxy-services-https.spec.ts`:
- Around line 151-155: Update both secondMapping combobox click calls in the
HTTPS reverse-proxy test to pass the force option, matching the existing Radix
trigger interaction pattern; leave the surrounding TLS and TCP option selections
unchanged.
- Around line 163-193: Wrap the viewport-resize loop in a try/finally block and
move the existing page.setViewportSize(originalViewport) call into finally.
Preserve the current assertions and resize behavior while ensuring
dashboardAsOwner’s page viewport is restored even when an assertion or other
operation in the loop fails.
---
Outside diff comments:
In `@e2e/tests/reverse-proxy-services-https.spec.ts`:
- Line 1: Update the mapping-protocol combobox clicks in the HTTPS and TCP
reverse-proxy tests to pass { force: true }, including both clicks on
secondMapping and the click on udpMapping. Keep the existing selectors and
surrounding test flow unchanged.
---
Nitpick comments:
In `@e2e/tests/reverse-proxy-services-https.spec.ts`:
- Around line 221-222: Replace the parent traversal in the HTTPS Targets
assertion with a dedicated data-testid container selected via
page.getByTestId(). Scope the existing table row count assertion to that test-id
element, and ensure the corresponding container markup exposes the stable test
id.
🪄 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
Run ID: 54d4f43e-d4f7-421c-a192-a1d6be46c170
📒 Files selected for processing (8)
e2e/tests/reverse-proxy-services-https.spec.tse2e/tests/reverse-proxy-services-tcp.spec.tse2e/tests/reverse-proxy-services-udp-no-custom-ports.spec.tse2e/tests/reverse-proxy-services-udp.spec.tssrc/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyLayer4Content.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/domain/useReverseProxyDomain.ts
2 participants
Issue ticket number and link
Related to netbirdio/netbird#5821
Backend companion: netbirdio/netbird#6842
This draft adds the dashboard side of the multi-port reverse-proxy feature:
Validation
This is a draft pending maintainer confirmation of the backend model in netbirdio/netbird#6842.
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
netbirdio/docs#867
E2E tests
The new tests require the compatible management and reverse-proxy implementation from netbirdio/netbird#6842. The upstream image tags remain on main until that implementation is available as NetBird-hosted PR images or is merged.
management-cloud-tag: main
reverse-proxy-tag: main
Summary by CodeRabbit