Conversation
📝 WalkthroughWalkthroughThe runtime address model is refactored from flat IPv4/IPv6 fields into a protocol-aware structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
src/presentation/runners/common/runtime_address_info_test.go (1)
81-122: Add a WSS/UNKNOWN fallback case to this test coverage.
protocolOrFallback(...)is now what preservesWSSettings.Protocol = settings.WSSand theUNKNOWN -> WSfallback, but these assertions only cover matching TCP/UDP/WS inputs. A small extra case here would lock down the remaining protocol-tagging edge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/runners/common/runtime_address_info_test.go` around lines 81 - 122, Extend TestRuntimeAddressInfoFromServerConfiguration_CollectsAllEnabledTunnelAddresses to also assert handling of WSS and UNKNOWN fallbacks: add a serverConfiguration.Configuration variant where WSSettings.Protocol is set to settings.WSS and another where a non-WS protocol triggers the UNKNOWN->WS fallback via protocolOrFallback, then call RuntimeAddressInfoFromServerConfiguration and assert that the resulting TunnelAddresses entry for the WS slot preserves settings.WSS for the first case and maps UNKNOWN to settings.WS for the fallback case; reference WSSettings.Protocol, protocolOrFallback, and RuntimeAddressInfoFromServerConfiguration when locating where to add these assertions.
🤖 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/presentation/runners/common/runtime_address_info.go`:
- Around line 35-36: The code flattens per-protocol server hosts into a single
ServerAddress (RuntimeAddressPair) via MergeMissing, which can hide differences
across protocols; update the code that constructs ServerAddress (and similarly
TunnelAddresses handling) to either (A) validate that all enabled protocols in
serverConfiguration.Configuration have identical Server hosts before merging and
abort/log an error if they differ, or (B) change the model to keep server
addresses per protocol (e.g., map protocol->RuntimeAddressPair) instead of a
single ServerAddress and update consumers accordingly; identify usages of
ServerAddress, RuntimeAddressPair, MergeMissing, and
serverConfiguration.Configuration to implement the chosen approach and ensure
TunnelAddresses (lines 57-59) follow the same rule.
---
Nitpick comments:
In `@src/presentation/runners/common/runtime_address_info_test.go`:
- Around line 81-122: Extend
TestRuntimeAddressInfoFromServerConfiguration_CollectsAllEnabledTunnelAddresses
to also assert handling of WSS and UNKNOWN fallbacks: add a
serverConfiguration.Configuration variant where WSSettings.Protocol is set to
settings.WSS and another where a non-WS protocol triggers the UNKNOWN->WS
fallback via protocolOrFallback, then call
RuntimeAddressInfoFromServerConfiguration and assert that the resulting
TunnelAddresses entry for the WS slot preserves settings.WSS for the first case
and maps UNKNOWN to settings.WS for the fallback case; reference
WSSettings.Protocol, protocolOrFallback, and
RuntimeAddressInfoFromServerConfiguration when locating where to add these
assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c9cb4f4-8457-48e6-afaa-448e2139d584
📒 Files selected for processing (7)
src/presentation/runners/common/runtime_address_info.gosrc/presentation/runners/common/runtime_address_info_test.gosrc/presentation/ui/tui/internal/bubble_tea/runtime_dashboard.gosrc/presentation/ui/tui/internal/bubble_tea/runtime_dashboard_test.gosrc/presentation/ui/tui/runtime_backend_bubbletea.gosrc/presentation/ui/tui/runtime_backend_bubbletea_test.gosrc/presentation/ui/tui/runtime_ui_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/presentation/ui/tui/internal/bubble_tea/runtime_dashboard_test.go (1)
690-704: Add an explicit guard against legacy single-line tunnel output.The test checks for the new grouped section, but it should also assert that legacy
Tunnel IP:is absent in multi-protocol server mode.Suggested assertion
if !strings.Contains(view, "WS: IPv4 10.0.2.1 | IPv6 fd00::3") { t.Fatalf("expected WS tunnel line in server main view, got %q", view) } + if strings.Contains(view, "Tunnel IP: ") { + t.Fatalf("unexpected legacy single tunnel IP line in multi-protocol server view, got %q", view) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/ui/tui/internal/bubble_tea/runtime_dashboard_test.go` around lines 690 - 704, The test currently asserts presence of the new grouped tunnel section by checking substrings in the view variable but does not guard against the old single-line legacy output; update the test in runtime_dashboard_test.go (the assertions that check view for "Tunnel IPs:", "Server IP: IPv4 198.51.100.10", "TCP: IPv4 10.0.0.1 | IPv6 fd00::1", "UDP: IPv4 10.0.1.1 | IPv6 fd00::2", "WS: IPv4 10.0.2.1 | IPv6 fd00::3") to also assert that the legacy string "Tunnel IP:" is not present (e.g., add a t.Fatalf when strings.Contains(view, "Tunnel IP:") is true) so the test fails if the old single-line output appears in multi-protocol server mode.src/presentation/ui/tui/runtime_backend_bubbletea_test.go (1)
75-83: Consider adding a multi-protocol forwarding assertion.This fixture only exercises a single entry, so a future regression that drops additional protocols could still pass.
Suggested test hardening
- if len(options.ProtocolAddresses) != 1 { - t.Fatalf("expected one protocol address entry forwarded, got %d", len(options.ProtocolAddresses)) + if len(options.ProtocolAddresses) != 2 { + t.Fatalf("expected two protocol address entries forwarded, got %d", len(options.ProtocolAddresses)) } + if options.ProtocolAddresses[1].Protocol != settings.UDP { + t.Fatalf("expected second protocol address protocol UDP, got %v", options.ProtocolAddresses[1].Protocol) + } ... - Address: runnerCommon.RuntimeAddressInfo{ - ProtocolAddresses: []runnerCommon.RuntimeProtocolAddress{{ + Address: runnerCommon.RuntimeAddressInfo{ + ProtocolAddresses: []runnerCommon.RuntimeProtocolAddress{{ Protocol: settings.TCP, ServerAddress: runnerCommon.RuntimeAddressPair{ IPv4: netip.MustParseAddr("198.51.100.10"), }, TunnelAddress: runnerCommon.RuntimeAddressPair{ IPv4: netip.MustParseAddr("10.0.0.2"), }, - }}, + }, { + Protocol: settings.UDP, + ServerAddress: runnerCommon.RuntimeAddressPair{ + IPv4: netip.MustParseAddr("198.51.100.11"), + }, + TunnelAddress: runnerCommon.RuntimeAddressPair{ + IPv4: netip.MustParseAddr("10.0.1.2"), + }, + }}, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/ui/tui/runtime_backend_bubbletea_test.go` around lines 75 - 83, The test fixture only includes a single ProtocolAddresses entry (runnerCommon.RuntimeProtocolAddress with Protocol settings.TCP), which won't catch regressions that drop support for other protocols; update the test in runtime_backend_bubbletea_test.go to include at least one additional ProtocolAddresses entry (e.g., a second runnerCommon.RuntimeProtocolAddress with Protocol settings.UDP and appropriate ServerAddress/TunnelAddress pairs) and add assertions that the component under test handles both entries (check that forwarding/translation logic for ProtocolAddresses and RuntimeProtocolAddress processes both TCP and UDP entries).
🤖 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/presentation/ui/tui/internal/bubble_tea/runtime_dashboard_test.go`:
- Around line 690-704: The test currently asserts presence of the new grouped
tunnel section by checking substrings in the view variable but does not guard
against the old single-line legacy output; update the test in
runtime_dashboard_test.go (the assertions that check view for "Tunnel IPs:",
"Server IP: IPv4 198.51.100.10", "TCP: IPv4 10.0.0.1 | IPv6 fd00::1", "UDP: IPv4
10.0.1.1 | IPv6 fd00::2", "WS: IPv4 10.0.2.1 | IPv6 fd00::3") to also assert
that the legacy string "Tunnel IP:" is not present (e.g., add a t.Fatalf when
strings.Contains(view, "Tunnel IP:") is true) so the test fails if the old
single-line output appears in multi-protocol server mode.
In `@src/presentation/ui/tui/runtime_backend_bubbletea_test.go`:
- Around line 75-83: The test fixture only includes a single ProtocolAddresses
entry (runnerCommon.RuntimeProtocolAddress with Protocol settings.TCP), which
won't catch regressions that drop support for other protocols; update the test
in runtime_backend_bubbletea_test.go to include at least one additional
ProtocolAddresses entry (e.g., a second runnerCommon.RuntimeProtocolAddress with
Protocol settings.UDP and appropriate ServerAddress/TunnelAddress pairs) and add
assertions that the component under test handles both entries (check that
forwarding/translation logic for ProtocolAddresses and RuntimeProtocolAddress
processes both TCP and UDP entries).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e293aec-bc37-4934-9356-3cda1e06fdfd
📒 Files selected for processing (7)
src/presentation/runners/common/runtime_address_info.gosrc/presentation/runners/common/runtime_address_info_test.gosrc/presentation/ui/tui/internal/bubble_tea/runtime_dashboard.gosrc/presentation/ui/tui/internal/bubble_tea/runtime_dashboard_test.gosrc/presentation/ui/tui/runtime_backend_bubbletea.gosrc/presentation/ui/tui/runtime_backend_bubbletea_test.gosrc/presentation/ui/tui/runtime_ui_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- src/presentation/ui/tui/runtime_ui_test.go
- src/presentation/runners/common/runtime_address_info_test.go
- src/presentation/runners/common/runtime_address_info.go
- src/presentation/ui/tui/internal/bubble_tea/runtime_dashboard.go
- src/presentation/ui/tui/runtime_backend_bubbletea.go
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
1 participant
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
Why
The server runtime dashboard previously collapsed multiple enabled protocol interfaces into a single
Tunnel IPpair, which made the dataplane view incomplete whenTCP,UDP, andWSwere enabled together.Impact
Server-mode TUI now shows the active tunnel addresses per protocol instead of implying that one address pair represents the full dataplane.
Root cause
RuntimeAddressInfoFromServerConfiguration(...)only kept the first valid tunnel IPv4/IPv6 pair, and the dashboard options/model only carried one tunnel pair through to the UI.Validation
GOCACHE=/tmp/tungo-go-build-cache go test ./...Closes #280
Summary by CodeRabbit