Fix route status indicator for dynamic (DNS) routes#117
Merged
Conversation
The status indicator stayed yellow forever for dynamic routes because the previous logic searched for the literal "invalid Prefix" sentinel inside domain strings, which never matched. Match the Android client's logic: a dynamic route is "connected" (green) when any resolved IP for one of its domains appears in a Connected peer's route list. Compare addresses with the CIDR mask stripped, since peer routes carry /32 suffixes while resolved IPs do not. Switch the bridge DTO from a comma-joined ResolvedIPs string to a structured [String] list (mirrors Android's ResolvedIPs collection in client/android/network_domains.go), so consumers no longer depend on the Go-side string formatting. Bumps netbird-core submodule.
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR replaces ChangesResolved IP data-shape, resolution plumbing, and UI status logic
Sequence Diagram(s)sequenceDiagram
participant UI as RouteCard / TVNetworksView
participant App as App Logic
participant NE as PacketTunnelProvider (NetworkExtension)
participant Resolver as Domain Resolver
participant Peers as PeerInfo Store
UI->>App: request route display & status
App->>NE: request selected routes with domain details
NE->>Resolver: domain.getResolvedIPs()
Resolver-->>NE: resolved IP list
NE-->>App: DomainDetails(domain, resolvedIPs)
App->>Peers: query connected peers / addresses
Peers-->>App: connected peer addresses
App->>UI: provide routeDisplayText and statusIndicatorColor (match resolvedIPs vs connected addresses)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
NetbirdKit/RoutesSelectionDetails.swift (1)
60-63: ⚡ Quick winKeep
DomainDetailsdecoding backward-compatible.Lines 60-63 change the IPC payload from a legacy
resolvedips: String?field toresolvedIPs: [String]. That is a breaking wire-format change, so a stale extension process or any older cached plist will fail to decode the whole routes payload. Please add a custominit(from:)that accepts both shapes and defaults missing data to[].Proposed compatibility shim
struct DomainDetails: Codable, Hashable { let domain: String let resolvedIPs: [String] + + enum CodingKeys: String, CodingKey { + case domain + case resolvedIPs + case legacyResolvedIPs = "resolvedips" + } + + init(domain: String, resolvedIPs: [String]) { + self.domain = domain + self.resolvedIPs = resolvedIPs + } + + init(from decoder: Decoder) throws { + let container = try decoder.container(keyedBy: CodingKeys.self) + domain = try container.decode(String.self, forKey: .domain) + + if let resolvedIPs = try container.decodeIfPresent([String].self, forKey: .resolvedIPs) { + self.resolvedIPs = resolvedIPs + } else if let legacyResolvedIPs = try container.decodeIfPresent(String.self, forKey: .legacyResolvedIPs) { + self.resolvedIPs = legacyResolvedIPs + .split(separator: ",") + .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } + .filter { !$0.isEmpty } + } else { + self.resolvedIPs = [] + } + } }🤖 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 `@NetbirdKit/RoutesSelectionDetails.swift` around lines 60 - 63, DomainDetails' Codable decoding must accept both the new resolvedIPs: [String] shape and the legacy resolvedips: String? to remain backward-compatible; implement a custom init(from decoder: Decoder) in the DomainDetails struct that attempts to decode resolvedIPs as [String], falls back to decoding resolvedips as a single optional String (splitting or wrapping it into an array if present), and defaults resolvedIPs to [] when neither field exists or decoding fails, ensuring the decoded properties (domain and resolvedIPs) are populated accordingly.NetBird/Source/App/Views/Components/RouteCard.swift (1)
107-108: ⚡ Quick winStop keying dynamic-route behavior off
"invalid Prefix".Lines 107-108 and Line 180 still couple the UI to a Go-side sentinel string. This PR removed the old string-format dependency around resolved IPs, but this one can still regress the status/detail logic if core changes that placeholder again. Prefer deriving
isDynamicRoutefromdomains?.isEmpty == falseor carrying an explicit flag in the DTO.Possible cleanup
+ private var isDynamicRoute: Bool { + !(route.domains?.isEmpty ?? true) + } + private var statusIndicatorColor: Color { guard route.selected else { return Color.gray.opacity(0.5) } let connectedPeerRoutes = peerViewModel.peerInfo .filter { $0.connStatus == "Connected" } .flatMap { $0.routes } - if let network = route.network, network != "invalid Prefix", + if !isDynamicRoute, let network = route.network, connectedPeerRoutes.contains(network) { return Color.green } @@ private var routeDisplayText: String { - if route.network == "invalid Prefix" { + if isDynamicRoute { if let domains = route.domains, domains.count > 2 { return "\(domains.count) Domains" } return route.domains?.map { $0.domain }.joined(separator: ", ") ?? "" } return route.network ?? "Unknown" } @@ func detailInfo() -> some View { Group { - if route.network == "invalid Prefix" { + if isDynamicRoute { if let domains = route.domains { ForEach(domains, id: \.self) { domain in detailRow(label: domain.domain, value: domain.resolvedIPs.joined(separator: ", ")) } }Also applies to: 180-180
🤖 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 `@NetBird/Source/App/Views/Components/RouteCard.swift` around lines 107 - 108, The view currently treats a route as dynamic by checking route.network != "invalid Prefix", which hard-codes a Go sentinel string; instead change the logic in RouteCard (where route.network and connectedPeerRoutes are used) to derive dynamic routes from route.domains?.isEmpty == false (or an explicit DTO flag if available) and update the same check at the other occurrence (the second use around line 180) to use the new condition; ensure any UI branches that relied on the "invalid Prefix" string now use the domains emptiness check or DTO flag so the UI is no longer coupled to that sentinel value.
🤖 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.
Nitpick comments:
In `@NetBird/Source/App/Views/Components/RouteCard.swift`:
- Around line 107-108: The view currently treats a route as dynamic by checking
route.network != "invalid Prefix", which hard-codes a Go sentinel string;
instead change the logic in RouteCard (where route.network and
connectedPeerRoutes are used) to derive dynamic routes from
route.domains?.isEmpty == false (or an explicit DTO flag if available) and
update the same check at the other occurrence (the second use around line 180)
to use the new condition; ensure any UI branches that relied on the "invalid
Prefix" string now use the domains emptiness check or DTO flag so the UI is no
longer coupled to that sentinel value.
In `@NetbirdKit/RoutesSelectionDetails.swift`:
- Around line 60-63: DomainDetails' Codable decoding must accept both the new
resolvedIPs: [String] shape and the legacy resolvedips: String? to remain
backward-compatible; implement a custom init(from decoder: Decoder) in the
DomainDetails struct that attempts to decode resolvedIPs as [String], falls back
to decoding resolvedips as a single optional String (splitting or wrapping it
into an array if present), and defaults resolvedIPs to [] when neither field
exists or decoding fails, ensuring the decoded properties (domain and
resolvedIPs) are populated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de21a625-540f-464d-a3a2-df53cb34d6a6
📒 Files selected for processing (5)
NetBird/Source/App/Views/Components/RouteCard.swiftNetBirdTVNetworkExtension/PacketTunnelProvider.swiftNetbirdKit/RoutesSelectionDetails.swiftNetbirdNetworkExtension/PacketTunnelProvider.swiftnetbird-core
lixmal
previously approved these changes
May 6, 2026
Contributor
Author
|
/testflight |
|
TestFlight builds uploaded |
Adds verbose NSLog output explaining why each route card resolves to gray, yellow, or green. To be reverted once the dynamic-route status indicator regression is diagnosed.
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 `@NetBird/Source/App/Views/Components/RouteCard.swift`:
- Around line 101-160: The code emits many NSLog calls with sensitive network
metadata (see the tag variable, peerViewModel.peerInfo, route.network,
route.domains, resolvedIPs, connectedPeerRoutes and all NSLog(...) lines);
remove these or wrap all NSLog calls in a compile-time guard (e.g. `#if` DEBUG ...
`#endif`) so they are excluded from release/TestFlight builds, or replace them
with a single non-sensitive debug-only log; ensure you update every NSLog
invocation in this RouteCard computed view to be behind that guard.
🪄 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: 681c6231-1b52-43b1-96a2-ea722649d61a
📒 Files selected for processing (1)
NetBird/Source/App/Views/Components/RouteCard.swift
…cator-dynamic-domains
Contributor
Author
|
/testflight |
|
TestFlight builds uploaded |
Replace NSLog with AppLogger.shared.log so traces land in the shared swift-log.log file. Deduplicate per-route decisions so SwiftUI re-renders don't flood the rotating log.
Contributor
Author
|
/testflight |
|
TestFlight builds uploaded |
The bridge now exposes Domains.SafeString() as the Network value for dynamic (DNS) routes, matching the Android client. That string also appears in peer.routes (it's what dynamic.Route.String() returns), so a single peer.routes.contains(route.network) check works for both static and dynamic routes — no sentinel branching needed. Replace "invalid Prefix" checks with route.domains presence checks in the status indicator, route display text, and tooltip detail view, on both iOS and tvOS. Bumps netbird-core submodule.
Contributor
Author
|
/testflight |
|
TestFlight builds uploaded |
Diagnosis is complete; the verbose AppLogger output and dedup cache in statusIndicatorColor are no longer needed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
NetBird/Source/App/Views/TV/TVNetworksView.swift (1)
179-187: ⚡ Quick win
routeDisplayTextis duplicated withRouteCardwith a silent empty-string fallback.The logic here is identical to
RouteCard.routeDisplayTextexcept the no-domain, no-network fallback is""(renders an invisibleText) rather than"Unknown". Centralising this onRoutesSelectionInfo(or a shared extension) would eliminate the divergence and make a future threshold change a single-site edit.♻️ Suggested extraction onto RoutesSelectionInfo
// In RoutesSelectionInfo (or an extension file): var displayText: String { if let domains = domains, !domains.isEmpty { return domains.count > 2 ? "\(domains.count) Domains" : domains.map { $0.domain }.joined(separator: ", ") } return network ?? "Unknown" }Then both call sites become a single
route.displayText.🤖 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 `@NetBird/Source/App/Views/TV/TVNetworksView.swift` around lines 179 - 187, Duplicate route display logic exists in TVNetworksView.routeDisplayText and RouteCard.routeDisplayText; extract it into a single computed property on RoutesSelectionInfo (or an extension) named something like displayText that implements the domains/count > 2 branch, joins domain names when <= 2, and returns network ?? "Unknown" as the fallback. Replace both TVNetworksView.routeDisplayText and RouteCard.routeDisplayText usages with route.displayText so they share the same implementation and threshold; ensure you remove the silent-empty-string fallback in TVNetworksView and use the centralized property instead.
🤖 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.
Nitpick comments:
In `@NetBird/Source/App/Views/TV/TVNetworksView.swift`:
- Around line 179-187: Duplicate route display logic exists in
TVNetworksView.routeDisplayText and RouteCard.routeDisplayText; extract it into
a single computed property on RoutesSelectionInfo (or an extension) named
something like displayText that implements the domains/count > 2 branch, joins
domain names when <= 2, and returns network ?? "Unknown" as the fallback.
Replace both TVNetworksView.routeDisplayText and RouteCard.routeDisplayText
usages with route.displayText so they share the same implementation and
threshold; ensure you remove the silent-empty-string fallback in TVNetworksView
and use the centralized property instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa237f7d-3a21-4cc3-8580-1cc6df40187f
📒 Files selected for processing (3)
NetBird/Source/App/Views/Components/RouteCard.swiftNetBird/Source/App/Views/TV/TVNetworksView.swiftnetbird-core
The bridge change (structured ResolvedIPs collection + dynamic-route Network exposure) has landed on the netbird-core main branch as f23aaa9ae. Replace the local feature-branch SHA with the merged one.
lixmal
approved these changes
May 7, 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.
Description
The route status indicator stayed yellow forever for dynamic (DNS) routes. The Swift code branched on a
route.network == "invalid Prefix"sentinel and tried to substring-match domains against that sentinel, which never matched.Realign the iOS pipeline with the Android client so a single check works for both static and dynamic routes:
Bridge (
netbird-core)Domains.SafeString()as theNetworkvalue for dynamic routes (matchingclient/android/client.go). The same string is whatdynamic.Route.String()returns and is therefore what ends up inpeer.routes, sopeer.routes.contains(route.network)now matches directly.ParentDomain(mirroringclient/server/network.go). TheresolvedDomainsmap is keyed by the resolved domain, not the configured pattern; the previous lookupresolvedDomains[d]could never hit for wildcard patterns.ResolvedIPs stringto a structuredResolvedIPscollection (Add/Get/Size), mirroringclient/android/network_domains.go. Swift consumers no longer depend on Go-side string formatting.Swift app
RouteCard.statusIndicatorColorbecomes a single, Android-equivalent check:peer.routes.contains(route.network), with a resolved-IP fallback for IP-only static-IP routes."invalid Prefix"sentinel fromRouteCard.swiftandTVNetworksView.swift— branch onroute.domains.isEmptyinstead.RoutesSelectionDetails.DomainDetailscarriesresolvedIPs: [String]instead of an optional CSV string.Bumps
netbird-coresubmodule (corresponding upstream PR contains the bridge changes).Summary by CodeRabbit