fix(dashboard): hide link-local IPv6 in LAN Information widget (#1129)#1138
Conversation
The LAN Information widget surfaced the interface's link-local IPv6 address (fe80::/10, scope link) when the LAN bridge held only a link-local address and no global/ULA prefix. A link-local address is only valid on a single link and is not a meaningful LAN IPv6 address. Filter fe80::/10 addresses at the source in UspLanDataService._fetchLanIpv6Addresses(), so all consumers (dashboard card, PDF report, AI section) render consistently. When IPv6 is enabled but no meaningful address remains, the card now shows '-' (consistent with the DNS field) instead of the link-local address or a bare 'Enabled' label. Refs #1129
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 1 · f2af7bb..f7c1cb2 (full)
Verdict: 💬 COMMENT — GitHub API rejected APPROVE (same author account on agent-team PR). No Critical issues found; APPROVE intent. Please coordinate with PR #1139 on the duplicated IPv6 logic before or shortly after both land.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| 🟢High | lib/page/local_network/services/usp_lan_data_service.dart:79–87 |
[both reviewers] Private _isLinkLocalIpv6() duplicates classifyIpv6Scope() from PR #1139 — two independent fe80::/10 implementations will diverge |
|
| 🟡Med | lib/page/local_network/services/usp_lan_data_service.dart:87 |
_isLinkLocalIpv6 does not strip prefix-length /NN suffix — inconsistent with ipv6_address.dart:95 from PR #1139 |
|
| 💡 | 🟢High | test/page/local_network/providers/lan_data_provider_test.dart |
Missing ULA passthrough test (fd00::1, fc00::1 should survive the filter) |
| 💡 | 🟢High | test/page/local_network/providers/lan_data_provider_test.dart |
Missing edge-case tests: FE80::1 (uppercase), fec0::1 (deprecated site-local, must not be filtered), ::1 (loopback, must not be filtered) |
| 💡 | 🟡Med | lib/page/local_network/cards/usp_lan_info_card.dart:111 |
label: 'IPv6' and value: '-' are hardcoded strings; l10n pattern used elsewhere in same file |
| 💡 | ⚪Low | lib/page/local_network/services/usp_lan_data_service.dart:79–94 |
Blacklist-only filter misses loopback ::1 and multicast ff00::/8; whitelist approach more defensive (low priority) |
Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.
⚠️ Warning Details
W-1 — [both reviewers] Duplicate link-local classification — will diverge with PR #1139 (Confidence: 🟢High)
lib/page/local_network/services/usp_lan_data_service.dart:79–87:
static bool _isLinkLocalIpv6(String ip) {
final addr = ip.split('%').first.trim();
final firstHextet = addr.split(':').first;
if (firstHextet.isEmpty) return false;
final value = int.tryParse(firstHextet, radix: 16);
if (value == null) return false;
return (value & 0xffc0) == 0xfe80;
}PR #1139 (gate/feat-1128) adds lib/core/utils/ipv6_address.dart with:
if (firstByte == 0xFE && (secondByte & 0xC0) == 0x80) return Ipv6Scope.linkLocal;Both algorithms exhaustively verified equivalent across the full fe80::/10 range. However once both PRs land on dev-2.6.0, the codebase will have two independent implementations of RFC 4291 §2.5.6. Any future correction must be applied in two places.
Fix (near-term): Add // TODO(#1129-follow-up): replace with classifyIpv6Scope() after gate/feat-1128 lands to the private method.
Fix (after #1139 lands):
import 'package:privacy_gui/core/utils/ipv6_address.dart';
.where((ip) => ip.isNotEmpty && classifyIpv6Scope(ip) != Ipv6Scope.linkLocal)W-2 — _isLinkLocalIpv6 does not strip prefix-length notation (Confidence: 🟡Med)
lib/page/local_network/services/usp_lan_data_service.dart:87:
final addr = ip.split('%').first.trim(); // strips zone-id only — no /NN strippingFor TR-181 IPAddress fields, CIDR notation is currently not expected. In practice addr.split(':').first still yields "fe80" correctly for "fe80::1/64". But this is inconsistent with PR #1139's ipv6_address.dart:95 which strips both.
Fix:
final addr = ip.split('%').first.split('/').first.trim();✅ What looks good
- RFC 4291 bitmask correctness (High):
(value & 0xffc0) == 0xfe80correctly identifies the fullfe80::/10range. Independently verified exhaustively against PR #1139 — results identical. - Zone-ID stripping (High):
ip.split('%').firstcorrectly handlesfe80::1%eth0. - Architecture layer (High): Filter logic lives in the Service layer, not View. All downstream consumers benefit automatically.
LanDataNotifierprovider rules (High):AsyncNotifierProvider(non-autoDispose);ref.readfor service injection; no state mutations outsidebuild()/mutations. All four provider rules satisfied.- UI consistency (High):
value: '-'for missing IPv6 matches the existing DNS field pattern — consistent empty-state presentation. - Test coverage (High): Three new scenarios (link-local only, zone-index, mixed) directly validate the #1129 regression cases.
- UI Kit compliance (High):
InfoGridItem/InfoGridfrom shared components;AppText,AppGap,AppIcon,MetricTile,AppSpacingfrompackage:ui_kit_library/ui_kit.dart.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 1 · gate/feat-1129 (full, 3 files, 80 lines)
Verdict: 💬 COMMENT — dispatch-chain PR; no Critical issues found, APPROVE intent. Posting as COMMENT because GitHub API blocks same-account self-approve.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| High | lib/page/local_network/services/usp_lan_data_service.dart:88-101 |
[both reviewers] _isLinkLocalIpv6() duplicates classifyIpv6Scope() from sibling PR #1139 — two independent fe80::/10 implementations will silently diverge post-merge |
|
| Med | lib/page/local_network/services/usp_lan_data_service.dart:90 |
_isLinkLocalIpv6 strips zone-index (%eth0) but does not explicitly strip CIDR prefix-length (/64) — accidentally safe by parse order, unlike canonical ipv6_address.dart which guards this explicitly |
|
| High | usp_lan_data_service.dart vs usp_wan_data_service.dart (PR #1139) |
[both reviewers] LAN uses filter (drop link-local), WAN uses reorder (keep all, sort global first) — contradictory strategies for the same root cause (#1128/#1129) | |
| High | test/page/local_network/providers/lan_data_provider_test.dart |
Missing fec0::1 boundary test — the address just above fe80::/10 range must NOT be filtered; if mask constant changes, no test catches regression |
|
| 💡 | High | test/page/local_network/providers/lan_data_provider_test.dart |
Missing ULA passthrough test — fd12:3456::1 and fc00::1 must be kept; confirms mask does not accidentally catch fc00::/7 |
| 💡 | High | lib/page/local_network/services/usp_lan_data_service.dart:88 |
_isLinkLocalIpv6 is private static in service class — cannot be unit-tested directly; all coverage flows through integration-style provider tests |
| 💡 | Med | lib/page/local_network/cards/usp_lan_info_card.dart:106,116 |
Hardcoded label: 'IPv6' string — localization key \"ipv6\" exists in app_en.arb; use loc(context).ipv6 |
| 💡 | Med | lib/page/local_network/services/usp_lan_data_service.dart:90 |
Missing explicit split('/').first to strip CIDR suffix — currently accidental correctness; add defensive guard matching canonical ipv6_address.dart |
Confidence: High = code-verified · Med = located + reasoned, not fully confirmed.
Items marked [both reviewers] were independently flagged by two agents -> higher confidence.
⚠️ Warning Details
W-1 — [both reviewers] _isLinkLocalIpv6() duplicates classifyIpv6Scope() from sibling PR #1139 (Confidence: High)
lib/page/local_network/services/usp_lan_data_service.dart:88-101:
static bool _isLinkLocalIpv6(String ip) {
final addr = ip.split('%').first.trim();
final firstHextet = addr.split(':').first;
if (firstHextet.isEmpty) return false;
final value = int.tryParse(firstHextet, radix: 16);
if (value == null) return false;
return (value & 0xffc0) == 0xfe80;
}PR #1139 (gate/feat-1128, sha bbcd8c0e) introduces lib/core/utils/ipv6_address.dart with classifyIpv6Scope(String address) that performs the same fe80::/10 detection using the canonical two-byte masking approach with explicit zone-index AND prefix-length stripping. Both PRs target dev-2.6.0. When merged, the codebase will have two independent link-local classifiers with subtly different robustness.
Fix: Replace _isLinkLocalIpv6(ip) with:
import 'package:privacy_gui/core/utils/ipv6_address.dart';
// ...
classifyIpv6Scope(ip) == Ipv6Scope.linkLocalCoordinate merge order with PR #1139 (must merge first or simultaneously). This also resolves the testability concern.
W-2 — _isLinkLocalIpv6 does not explicitly strip CIDR prefix-length suffix (Confidence: Med)
lib/page/local_network/services/usp_lan_data_service.dart:90:
final addr = ip.split('%').first.trim(); // "fe80::1/64" -> "fe80::1/64" (NOT stripped)
final firstHextet = addr.split(':').first; // "fe80::1/64".split(':')[0] = "fe80" (accidentally correct)This is safe for standard CIDR notation because /N appears after the first colon. However, ipv6_address.dart explicitly handles this with .split('/').first, making the intent clear. The private copy's behavior is accidental rather than intentional.
Fix: Add .split('/').first:
final addr = ip.split('%').first.split('/').first.trim();W-3 — [both reviewers] Inconsistent strategy: LAN filter vs WAN reorder (Confidence: High)
PR #1139 WAN approach: preferGlobalIpv6First — keeps all addresses, sorts global unicast first. The WAN widget shows .first but all addresses are available.
This PR LAN approach: filter — drops link-local entirely, never returns it.
Both fix the same root cause with contradictory algorithms. A future developer reading both services will see different approaches and will not know which is canonical.
Fix: Decide on one canonical strategy. Recommend reorder (not filter) since it preserves all addresses. Apply preferGlobalIpv6First() from ipv6_address.dart in both services. If intentional (LAN widget genuinely should never show link-local), add an explicit comment in both services.
W-4 — Missing fec0::1 boundary test (Confidence: High)
The test suite covers fe80::1, febf::1 (top of fe80::/10), and 2001:db8::1, but does NOT test fec0::1.
fec0 = 1111 1110 1100 0000
ffc0 = 1111 1111 1100 0000
AND = 1111 1110 1100 0000 = 0xfec0 != 0xfe80 -> NOT link-local
fec0::/10 (former site-local, RFC 3879 deprecated) is now global unicast. Without a test for fec0::1, any future refactor of the mask constant would silently pass.
Fix: Add a test asserting fec0::1 is NOT filtered and appears in ipv6Addresses.
✅ What looks good
- RFC 4291 bitmask correct (High):
(value & 0xffc0) == 0xfe80covers the fullfe80::/10block (fe80throughfebf). Test forfebf::1confirms upper boundary. - Zone-index stripping (High):
ip.split('%').first.trim()handlesfe80::1%eth0. Confirmed in test. - Fix applied at Service layer (High): Filter in
_fetchLanIpv6Addresses()meansLanInfoUIModel.ipv6Addressesis clean before reaching any consumer — correct separation of concerns. - '-' fallback consistent with DNS field (High):
usp_lan_info_card.dart:88uses... ? info.dnsServers : '-'; the PR now makes IPv6 use the same visual convention. - ipv6Enabled guard correct (High): The outer
else if (info.ipv6Enabled)at line 95 ensures the '-' cell renders when IPv6 is enabled but addresses are empty. - Architecture compliance (High): Three-layer dependency respected;
LanDataNotifier.build()callsref.read(uspLanDataServiceProvider)correctly; UI usesui_kit_librarywidgets throughout. - Existing test updated (High): Baseline test correctly updated to expect
['2001:db8::1']instead of pre-PR['fe80::1', '2001:db8::1'], confirming the filter is exercised end-to-end. - No security issues (High): Pure read-only display logic; no user input, no injection surface, no auth path. All string operations safe against pathological inputs.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
…lter (#1129) W-4 review-fix: fec0::1 is the first address just above the fe80::/10 range (link-local spans fe80::-febf::). It is a deprecated site-local address (RFC 3513), not link-local, so the filter must keep it. The suite already covered febf::1 (top of range) but not fec0 (first address outside), leaving the upper boundary of fe80::/10 untested.
🔧 Automated review-fix (L2) — PR #1138The hourly read-only PR-review cron flagged 4 warnings (W-1…W-4). Each was re-read against the actual code. Only W-4 is a true in-scope defect; the other three are comment-only. Disposition per finding below. W-1 (both reviewers, High): Duplicate
|
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 2 · f7c1cb2..5f8bd89 (incremental)
Verdict: 💬 COMMENT — dispatch-chain PR; 0 Critical. Posting as COMMENT because GitHub API blocks same-account self-approve. W-4 boundary test fix verified correct; W-1/W-3 cross-PR tech debt dispositions accepted.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| 💡 | 🟢High | test/page/local_network/providers/lan_data_provider_test.dart |
[single reviewer] ULA passthrough test still missing — fd12:3456::1 and fc00::1 must not be filtered; no test guards against mask-constant regression |
| 💡 | 🟢High | lib/page/local_network/cards/usp_lan_info_card.dart:106,116 |
[both reviewers] Hardcoded label: 'IPv6' — l10n key loc(context).ipv6 exists in app_en.arb; prior-round Suggestion still unresolved |
| 💡 | 🟢High | lib/page/local_network/services/usp_lan_data_service.dart:88–101 |
[single reviewer] ipv6_ranges.dart (extracted in sibling PR #1139 cdbd2e5f) now provides isLinkLocalBytes() — _isLinkLocalIpv6() could be simplified to use SSOT before merge |
Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.
⚠️ Warning Details
(No new Warnings this round — all prior Warnings resolved or accepted as comment-only.)
✅ Prior-round findings status + W-4 fix verification
W-4 — fec0::1 boundary test: ✅ Fixed and verified
New test at test/page/local_network/providers/lan_data_provider_test.dart (commit 5f8bd891):
- Math verified:
0xfec0 & 0xffc0 == 0xfec0 ≠ 0xfe80→_isLinkLocalIpv6('fec0::1')returnsfalse→ address passes filter ✅ - Mock routing verified:
paths.any((p) => p.toString().contains('IPv6Address'))correctly isolates the_fetchLanIpv6Addressescall without interfering withIPv4Addresspath ✅ - Test follows existing patterns (same
createContainer()/mockUsp.get/container.dispose()structure) ✅ - Production code path fully exercised:
lanDataProvider.future→UspLanDataService.fetch()→_fetchLanIpv6Addresses()→_isLinkLocalIpv6()filter →LanInfoUIModel.ipv6Addresses✅ - Architecture: test-only commit, correct placement within
group('LanDataNotifier', ...), guards the upper boundary offe80::/10complementing existingfebf::1coverage ✅
W-1 _isLinkLocalIpv6 vs classifyIpv6Scope duplication: ✅ Accepted — cross-PR tech debt
Note: PR #1139 (cdbd2e5f) has now extracted isLinkLocalBytes() into lib/core/utils/ipv6_ranges.dart. Both implementations verified semantically equivalent (65,536-combination check). The _isLinkLocalIpv6 hextet-mask approach ((value & 0xffc0) == 0xfe80) and ipv6_ranges.dart's isLinkLocalBytes(firstByte, secondByte) byte approach produce identical results. Consolidation before merge is a Suggestion (see table above).
W-2 CIDR strip (accidental correctness): ✅ Accepted — structurally safe
Re-verified: for 'fe80::1/64', split(':').first → 'fe80' (: precedes / in all valid IPv6 addresses). The /64 suffix never reaches int.tryParse. Safe by structure, not by accident.
W-3 LAN filter vs WAN reorder inconsistency: ✅ Accepted — cross-PR tech debt
No new evidence in this diff; prior disposition stands.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
PeterJhongLinksys
left a comment
There was a problem hiding this comment.
Automated PR Review
Verdict: 🟢 Approve — no unresolved critical issues
Findings
- 🔴 Critical — none
- 🟡 Warning —
_fetchLanIpv6Addressesfilters only by address prefix (link-local) and never inspects the TR-181IPv6Addressstatus (IPAddressStatus: Deprecated/Tentative/Invalid), so a non-preferred global/ULA address could still be surfaced asinfo.ipv6Addresses.firstin the card —lib/page/local_network/services/usp_lan_data_service.dart:74-77. Minor / edge case; not required by #1129. - 🟢 Nit — none new
Already raised (skipped to avoid duplication)
- Duplicate
_isLinkLocalIpv6()vs sibling PR #1139classifyIpv6Scope()/isLinkLocalBytes()— automated reviewer (W-1), still open by design (cross-PR tech debt; #1139 util is not present in this tree yet, so it cannot be consolidated within this PR). _isLinkLocalIpv6does not explicitly strip CIDR/NNsuffix — automated reviewer (W-2), accepted as structurally safe.- LAN filter vs WAN reorder strategy inconsistency (#1128/#1129) — automated reviewer (W-3), accepted as cross-PR tech debt.
- Missing
fec0::1boundary test — automated reviewer (W-4), fixed (test added, commit5f8bd891). - Missing ULA passthrough test (
fd00::1/fc00::1) — automated reviewer, still open (suggestion). - Missing uppercase
FE80::1/ loopback::1tests — automated reviewer, still open (suggestion; verified code handles both correctly —int.tryParseis case-insensitive, and::1→ empty first hextet → not filtered). - Hardcoded
label: 'IPv6'vs existingloc(context).ipv6key — automated reviewer, still open (suggestion). - Blacklist-only filter (misses loopback/multicast) — automated reviewer, low priority.
- Private
staticmethod not directly unit-testable — automated reviewer, suggestion.
Verified
- Bitmask
(value & 0xffc0) == 0xfe80correctly matches the fullfe80::/10block (fe80–febf) and leaves global (2001:), ULA (fc00/fd00), and deprecated site-local (fec0) untouched — hand-verified across all boundary cases; no accidental hiding of global/ULA addresses. - Empty-state renders correctly: outer visibility guard (
usp_lan_info_card.dart:95-97) includesinfo.ipv6Enabled, and the innerelse if (info.ipv6Enabled)renders'-'when the filtered list is empty — the reported link-local-only case shows'-'rather than thefe80::address. - Service-layer filtering keeps all consumers consistent — card, PDF report (
usp_pdf_service.dart:287-288), and AI LAN section (lan_section.dart:44-47) all read the already-filteredipv6Addresses; no downstream consumer regresses. ::1loopback / IPv4-mapped::-prefixed addresses produce an empty first hextet →return false→ kept (harmless on a LAN IPv6 interface).- Provider architecture unchanged and compliant: filtering lives in the L1 Service, no codegen model leaks upward, UI uses
ui_kit_library(InfoGrid/InfoGridItem/MetricTile). - Zero prior critical findings were raised by anyone; all earlier findings were Warnings/Suggestions, with W-4 now fixed.
…ge (#1146) * fix(dashboard/detail): unify IPv6 link-local display with a scope badge Across every view that surfaces an IPv6 address (Dashboard Network Status & LAN Information cards, Device Detail, Node Detail), a link-local (fe80::/10) address is now always shown but tagged with an Ipv6ScopeBadge (public_off icon + tooltip) rather than hidden or dropped. Data services keep every address and reorder so a globally routable one is preferred as the representative value; the UI marks link-local instead of filtering it. This supersedes the earlier filter/`-` behavior from #1128 (#1139) and #1129 (#1138): a WAN/LAN with no global/ULA prefix now shows its link-local address with the badge instead of a bare "-". - ipv6_address.dart: add public isLinkLocalIpv6() (shared classifier) - WAN/LAN data services: keep all addresses + preferGlobalIpv6First() - Ipv6ScopeBadge + InfoGridItem.labelTrailing + DetailCopyableTile.leading - Detail views swap the leading icon for the badge; cards tag the label - add ipv6ScopeLinkLocal string across all 26 locales - update WAN/LAN service tests for the keep-and-reorder behavior Refs #1128 #1129 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(golden): add IPv6 link-local/global screenshot states Screenshot-test coverage for the IPv6 scope badge across all four views that surface an IPv6 address. Each view now generates screenshots for both a global (no badge) and a link-local (badge) state, confirming the badge renders in the correct state: - dashboard cards (WAN Network Status, LAN Information): add link_local_ipv6 state (existing online_dhcp/dhcp_enabled already cover global/ULA) - Device Detail: add global_ipv6 + link_local_ipv6 states - Node Detail: add global_ipv6 + link_local_ipv6 states Baselines are regenerated by the screenshot suite (gitignored, not committed). This also refreshes the node_detail slave_with_devices screenshot, whose on-disk baseline predated the backhaul card. Refs #1128 #1129 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes the Dashboard LAN Information widget so it no longer shows the link-local IPv6 address (
fe80::…, scope link) when the LAN interface has no global/ULA IPv6 address.Refs #1129.Root cause
UspLanDataService._fetchLanIpv6Addresses()collected every entry underDevice.IP.Interface.1.IPv6Address.indiscriminately, and the card renderedinfo.ipv6Addresses.first. Whenbr-lanholds only a link-local address (as confirmed byip -6 addrin the report), thatfe80::address became.firstand was displayed. A link-local address is only valid on a single link and is not a meaningful LAN IPv6 address.Fix
fe80::/10link-local addresses in_fetchLanIpv6Addresses(). Detection masks the first hextet with0xffc0and compares to0xfe80(coversfe80–febf), stripping any zone index (%eth0) first. Filtering at the source keeps the dashboard card, PDF report, and AI LAN section consistent.-(consistent with how the DNS field renders when unavailable) instead of a bareEnabledlabel. If a global/ULA address later appears it is displayed normally.Tier isolation preserved: filtering lives in the L1 Service, no codegen model leaks upward, view unchanged in structure.
Acceptance (issue #1129)
-/empty_isLinkLocalIpv6filtersfe80::/10; card shows-Changed files
Tests (real output)
White-box service/provider tests via
flutter test test/page/local_network/providers/lan_data_provider_test.dart:fe80::7612:13ff:fe21:5394) → empty.fe80::1%eth0) +febf::1filtered, global2001:db8:abcd::5kept.build fetches LAN info and IPv6:fe80::1now filtered, only2001:db8::1remains.flutter analyzeon changed files:No issues found!dart format --set-exit-if-changed: clean.