refactor: unify TopBar + DiagnosticLoggable state logging + trace level logs

[AustinChangLinksys]

Summary

• DiagnosticLoggable mixin: Structured JSON state logging for diagnostic reports — UIModels define their own namedProps, captured by StateLogObserver into _stateLogCache (not main log stream)
• TopBar unification: Delete legacy top_bar.dart, consolidate to UspTopBar with DebugObserver support
• Trace level logging: Move dev-only logs ([App]: build, Throttler dispatch, WiFi/Topology internals) from logger.d() to logger.t() — filtered out in production builds

Test plan

• All 3013 functional tests pass (./run_tests.sh)
• 29 new tests for DiagnosticLoggable + StateLogObserver
• Static analysis clean (flutter analyze)

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[AustinChangLinksys]

🤖 Automated Review — Round 1 · 93e76ca..222d9ce (incremental)

Verdict: 💬 Self-review (comment only) — Own PR; 0 Critical, 5 Warnings noted for author review. Review provided for reference/blind-spot catching; verdict permanently COMMENT on own PRs.

	Conf.	Where	Issue (one-liner)
⚠️	🟢High	state_log_observer.dart:42 + logger.dart:285-286	MAC/SSID written to state log cache via updateStateLog() without MaskingUtils — exposed unmasked in diagnostic download
⚠️	🟢High	sse_event_router.dart:102-103	JsonEncoder.withIndent + replaceAll runs on every SSE notification at logger.d level — not filtered in release
⚠️	🟢High [both reviewers]	wifi_data_provider.dart:44-49	codegenContext silently dropped from WifiData equality; stale codegen context may not propagate on re-fetch
⚠️	🟢High	usp_top_bar.dart:78	Raw IconButton used instead of AppIconButton from ui_kit_library (violates design-system rule)
⚠️	🟢High	diagnostic_loggable.dart:49 + wifi_data_provider.dart:45	namedProps.values.toList() for Equatable props — Dart Map == is reference equality, so two WifiData instances with identical map contents are never equal
💡	🟢High [both reviewers]	usp_wifi_data_service.dart:252-256	catch(_) swallows all exception types silently with no log; hides coding errors in fallback
💡	🟢High	logger.dart:211-214	tag == 'State' branch is now dead code — StateLogObserver bypasses log pipeline entirely
💡	🟢High	diagnostic_loggable_test.dart / state_log_observer_test.dart	No equality tests for Map-typed namedProps; Map equality regression (W-4) has zero test coverage
💡	🟡Med	state_log_observer.dart:38	runtimeType.toString() as cache key causes collision when two providers hold same state type
💡	🟡Med	usp_top_bar.dart:98-110	Missing .select() on demoThemeConfigProvider / themeConfigProvider — over-rebuilds on every watch
💡	🟢High	framework/diagnostic_loggable.dart (new)	lib/framework/ layer has no architecture spec; core importing framework direction is undefined

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 — MAC/SSID unmasked in diagnostic state cache 🟢High [single]
state_log_observer.dart:42, logger.dart:285-286

Evidence chain:

// state_log_observer.dart:38-42
final typeName = value.runtimeType.toString();
final jsonState = value.toString();  // full DiagnosticLoggable JSON — includes MAC addresses
updateStateLog(typeName, jsonState);

// logger.dart:285-286
void updateStateLog(String providerName, String state) {
  _stateLogCache[providerName] = state;  // NO MaskingUtils applied
}

WifiClientUIModel.namedProps includes 'macAddress': macAddress (raw MAC string). WifiData.namedProps embeds wifiClientMap (Map keyed by MAC → WifiClientUIModel) and connectionDetailMap (includes SSID names). Every wifiDataProvider state update triggers StateLogObserver → updateStateLog with unmasked MACs and SSID names stored in _stateLogCache. outputFullWebLog (logger.dart:319-321) renders this verbatim in the diagnostic report download, while the normal logger.d path applies MaskingUtils.maskMacAddress + maskSensitiveJsonValues + encryptJNAPAuth before storage.

Fix: Apply masking in updateStateLog:

void updateStateLog(String providerName, String state) {
  final masked = MaskingUtils.maskSensitiveJsonValues(
      MaskingUtils.maskMacAddress(state));
  _stateLogCache[providerName] = masked;
}

---

W-2 — JsonEncoder.withIndent in SSE hot path at release-visible log level 🟢High [single]
sse_event_router.dart:102-103

logger.d('[SSE]: $subscriptionId ($type)\n'
    '  ${const JsonEncoder.withIndent('  ').convert(json).replaceAll('\n', '\n  ')}');

logger.d maps to Level.debug which passes the release filter (kReleaseMode min level = Level.debug, logger.dart:37-40). This re-serializes the already-parsed Map<String, dynamic> and does a replaceAll string pass on every SSE notification event in production. Prior code logged only a compact subscriptionId (type) line.

Fix: Move the pretty-print to logger.t (trace level, filtered in release):

logger.d('[SSE]: $subscriptionId ($type)');
logger.t('[SSE]: $subscriptionId ($type)\n  ${const JsonEncoder.withIndent('  ').convert(json).replaceAll('\n', '\n  ')}');

---

W-3 — codegenContext dropped from WifiData equality 🟢High [both reviewers]
wifi_data_provider.dart:44-49

Old props:

List<Object?> get props => [codegenContext, wifiClientMap.length, connectionDetailMap.length, radioModels.length];

New namedProps (head):

Map<String, Object?> get namedProps => {
  'wifiClientMap': wifiClientMap,
  'connectionDetailMap': connectionDetailMap,
  'radioModels': radioModels,
  // codegenContext is ABSENT
};

UspWifiSettingsProvider extracts wifiData.codegenContext.raw for settings mutations. If codegen data changes while UI maps remain structurally identical, stale codegen context may not propagate.

Fix: Restore 'codegenContext': codegenContext in namedProps.

---

W-4 — Map reference equality breaks WifiData Equatable contract 🟢High [single]
diagnostic_loggable.dart:49, wifi_data_provider.dart:45

// diagnostic_loggable.dart:49
@override
List<Object?> get props => namedProps.values.toList();

wifiClientMap (Map<String, WifiClientUIModel>) and connectionDetailMap appear in namedProps. Dart Map == is reference equality. Two WifiData instances built from a re-fetch with identical content will have wifiClientMap != wifiClientMap (different instances) — so WifiData != WifiData — Riverpod treats every re-fetch as a state change — all .watch(wifiDataProvider) listeners rebuild on every fetch, even if data is unchanged. Performance regression for all WiFi page providers.

Fix: (a) Use MapEquality from package:collection for Map-typed props, or (b) revert WifiData to explicit props override, or (c) document all consumers must use .select().

---

W-5 — Raw IconButton in UspTopBar violates ui_kit_library rule 🟢High [single]
usp_top_bar.dart:78

IconButton(
  icon: AppIcon.font(Icons.apps, color: colorScheme.onSurface),
  tooltip: loc(context).apps,
  onPressed: () => context.goNamed(RouteNamed.uspApps),
),

AGENTS.md mandates UI must use ui_kit_library components. AppIconButton is the correct replacement.

Fix: AppIconButton.icon(icon: AppIcon.font(Icons.apps, color: colorScheme.onSurface), onTap: () => context.goNamed(RouteNamed.uspApps))

✅ What looks good

• diagnostic_loggable.dart _toJsonSafe — data models are DAG-structured, no circular reference paths
• state_log_observer.dart AsyncError/AsyncLoading guard — if (!newValue.hasValue) return correctly skips both states
• Deletion of components/styled/top_bar.dart — no remaining references to old TopBar class found
• UspTopBar instantiation in ui_kit_page_view.dart:386,484 — null-safe type checks correct
• _debug event early-return in sse_connection_manager.dart — correct removal of synthetic heartbeat event logging
• StateLogObserver registration in main.dart — correct placement in ProviderScope.observers
• New test files state_log_observer_test.dart (283 lines) and diagnostic_loggable_test.dart (349 lines) — good primitives coverage
• Log prefix unification [USP][SSE][X] → [SSE] — cosmetic only, no correctness impact

Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.

[AustinChangLinksys]

🤖 Automated Review — Round 2 · 222d9ce..e58302 (incremental)

Verdict: 💬 Self-review (comment only) — Own PR; review provided for reference/blind-spot catching. Verdict permanently COMMENT on own PRs.

	Conf.	Where	Issue (one-liner)
⚠️	🟢High [both reviewers]	usp_top_bar.dart:101	demoThemeConfigProvider.select((c) => c) is an identity selector — no-op; rebuilds on every state change
⚠️	🟡Med	usp_client.dart:101-108	_genReqId() uses 12-bit entropy + second-precision timestamp; ≥91 concurrent requests/sec triggers >50% collision probability
⚠️	⚪Low	state_log_observer.dart:43	Masking chain omits maskUsernamePasswordBodyValue; inconsistent with main log path
⚠️	🟡Med	diagnostic_loggable.dart:47	diagnosticName default is still runtimeType.toString(); no abstract contract or lint to enforce overrides — future implementors silently re-introduce cache-key collision
💡	🟢High	logger.dart:196	Doc comment still says "If the tag is 'State'…" after the branch was removed — stale documentation

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.

✅ Round 1 warnings — all resolved in this commit

• W-1 (MAC/SSID unmasked in state cache): ✅ state_log_observer.dart now applies maskMacAddress + maskSerialNumber + maskSensitiveJsonValues before updateStateLog().
• W-2 (SSE hot-path JsonEncoder.withIndent at logger.d): ✅ Moved to logger.t — filtered in production.
• W-3 (codegenContext dropped from WifiData equality): ✅ Explicit props override in wifi_data_provider.dart restores codegenContext.
• W-4 (Raw IconButton in UspTopBar): ✅ Replaced with Tooltip(child: AppIconButton(…)).
• W-5 (namedProps.values.toList() Map reference equality): ✅ WifiData explicit props override + new Map equality tests in diagnostic_loggable_test.dart.
• S-1 (catch(_) swallows exceptions): ✅ catch(e) + logger.w('[WiFi] Fallback fetch failed: $e').
• S-2 (dead tag == 'State' branch in logger.dart): ✅ Removed; updateStateLog() is now the sole write path to _stateLogCache.
• S-3 (no Map equality tests): ✅ Four new test cases in diagnostic_loggable_test.dart.
• S-4 (runtimeType.toString() cache-key collision): ✅ diagnosticName getter added to DiagnosticLoggable; 30+ models override with stable string literals.
• S-5 (missing .select() on themeConfigProvider): ✅ themeConfigProvider.select((v) => v.valueOrNull) applied. (Partial — see W-1 below.)

⚠️ Warning Details

W-1 — Identity selector on demoThemeConfigProvider 🟢High [both reviewers]
lib/page/shell/usp_top_bar.dart:101

// head version
final demoConfig = ref.watch(demoThemeConfigProvider.select((c) => c));  // identity fn
final themeConfig = ref.watch(themeConfigProvider.select((v) => v.valueOrNull));  // correct

demoThemeConfigProvider is StateNotifierProvider<DemoThemeConfigNotifier, DemoThemeConfig>. DemoThemeConfig does not override ==; Riverpod compares with Object == (reference equality). .select((c) => c) returns the full object — Riverpod evaluates prevConfig == newConfig with reference equality, which is always false for new state emissions. Result: _buildCurrentDarkTheme() is called on every demoThemeConfigProvider notification, identical to having no .select() at all. Only themeConfigProvider side was correctly fixed.

Fix: Either remove the no-op .select() (matching the intent of "watch whole object") or select only the fields actually used in buildDemoThemeData:

final demoConfig = ref.watch(demoThemeConfigProvider);  // clear — whole object needed
// or:
final demoStyle = ref.watch(demoThemeConfigProvider.select((c) => c.style));

---

W-2 — _genReqId() 12-bit entropy + second-precision timestamp 🟡Med
lib/core/usp/services/usp_client.dart:101-108

static String _genReqId() {
  final ts = (DateTime.now().millisecondsSinceEpoch ~/ 1000)  // second granularity
      .toRadixString(16).toUpperCase();
  final rand =
      _random.nextInt(0xFFF).toRadixString(16).padLeft(3, '0').toUpperCase();
  return 'LNU$ts$rand';
}

0xFFF = 4095 values. Within the same second, birthday paradox gives >50% collision probability at 91+ concurrent requests. USP topology init / bulk-set operations commonly issue 10–20+ parallel requests. The old ++_reqId counter was strictly unique within a session; this ID is not. Correctness impact is limited to log correlation — but collisions make debugging significantly harder.

Fix (option A): Use milliseconds instead of seconds + larger random range:

final ts = DateTime.now().millisecondsSinceEpoch.toRadixString(16).toUpperCase();
final rand = _random.nextInt(0xFFFF).toRadixString(16).padLeft(4, '0').toUpperCase();

Fix (option B): Return to monotonic counter: static int _reqId = 0; return 'LNU${(++_reqId).toRadixString(16).padLeft(6,'0').toUpperCase()}'; — guaranteed unique per session.

---

W-3 — Masking chain inconsistency: maskUsernamePasswordBodyValue absent ⚪Low
lib/core/utils/state_log_observer.dart:43

State cache masking chain (head):

final maskedState = MaskingUtils.maskSensitiveJsonValues(
    MaskingUtils.maskSerialNumber(MaskingUtils.maskMacAddress(jsonState)));

Main log stream path in logger.dart:73-79 includes MaskingUtils.maskUsernamePasswordBodyValue which is absent from the state cache path. If any future DiagnosticLoggable model exposes a password/username field in namedProps, it will appear unmasked in the diagnostic report. Low confidence — current models don't expose raw credentials; this is a defensive gap.

Fix: Add maskUsernamePasswordBodyValue to the state cache masking chain for consistency with the log output path.

---

W-4 — diagnosticName not enforced: default remains runtimeType.toString() 🟡Med
lib/framework/diagnostic_loggable.dart:47

String get diagnosticName => runtimeType.toString();  // default — not abstract

This PR correctly adds overrides to 30+ models. However, any future DiagnosticLoggable implementor that forgets to override diagnosticName silently falls back to runtimeType.toString(), re-introducing the S-4 collision risk with no compile-time warning.

Fix (preferred): Make diagnosticName abstract to force implementors to provide a stable value. If too disruptive, add a debug-mode assertion in StateLogObserver:

assert(!_seenNames.contains(typeName), 'diagnosticName collision: $typeName');

✅ What looks good (this round)

• state_log_observer.dart masking chain: maskMacAddress + maskSerialNumber + maskSensitiveJsonValues — correct ordering, covers primary PII fields.
• diagnosticName getter + docs in DiagnosticLoggable — clear override guidance with example.
• Bulk diagnosticName overrides on 30+ models — stable string literals, no minification risk.
• WifiData dual-track: namedProps stays lean for diagnostic JSON; explicit props restores codegenContext for Riverpod equality — clean separation.
• updateStateLog() + @visibleForTesting accessors properly placed in logger.dart.
• logger.dart dead tag == 'State' branch fully removed — clean.
• New masking tests in state_log_observer_test.dart — good regression coverage.
• New Map equality tests in diagnostic_loggable_test.dart (4 cases) — directly covers the W-5 regression scenario.
• usp_wifi_data_service.dart fallback: catch(e) + logger.w — visible in production diagnostic reports.
• sse_event_router.dart SSE pretty-print: logger.t — correctly filtered in release.

Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.

[AustinChangLinksys]

🤖 Automated Review — Round 3 · e583024..47bf49d (incremental)

Verdict: 💬 Self-review (comment only) — Own PR; review provided for reference/blind-spot catching. Verdict permanently COMMENT on own PRs.

	Conf.	Where	Issue (one-liner)
✅	—	Prior W-1	demoThemeConfigProvider identity selector removed — resolved
✅	—	Prior W-2	_genReqId() upgraded to ms-precision + 16-bit random — resolved
✅	—	Prior S-1	Stale doc comment removed — resolved
⚠️	🟡Med	state_log_observer.dart:43	[both reviewers] Masking chain 3/5 layers vs. main log path — missing maskUsernamePasswordBodyValue + encryptJNAPAuth
⚠️	🟡Med	diagnostic_loggable.dart:47	[both reviewers] diagnosticName default runtimeType.toString() not abstract — future implementors silently re-introduce cache-key collision
⚠️	🟢High	usp_client.dart:99-100	[both reviewers] Doc comment example LNU18F3A2B4C5D6E7F8 doesn't match actual ms-epoch hex format (~11 chars); misleading
💡	🟢High	usp_top_bar.dart:101	Trailing whitespace on line (cosmetic)
💡	🟡Med	usp_client.dart	No unit test for updated _genReqId() entropy

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.

✅ Round 2 warnings resolved in this commit

• W-1 (identity selector): ref.watch(demoThemeConfigProvider.select((c) => c)) → ref.watch(demoThemeConfigProvider). No-op selector removed; DemoThemeConfig does not override == so .select((c) => c) was reference-equal on every emission anyway. ✅
• W-2 (_genReqId entropy): Upgraded to DateTime.now().millisecondsSinceEpoch (ms precision, ~11 hex chars) + _random.nextInt(0xFFFF) (16-bit, 65536 values). Birthday-paradox 50% collision threshold now at ~286 concurrent same-millisecond requests — well above realistic burst. ✅
• S-1 (stale doc comment): _addLogWithTag() doc no longer references the removed 'State' branch. ✅

⚠️ Warning Details

W-3 — state_log_observer.dart masking chain: 3/5 layers · 🟡Med [both reviewers] (carry-over from Round 2)

• Location: lib/core/utils/state_log_observer.dart:43
• Current code (verified at head sha):

  final maskedState = MaskingUtils.maskSensitiveJsonValues(
      MaskingUtils.maskSerialNumber(MaskingUtils.maskMacAddress(jsonState)));
  updateStateLog(typeName, maskedState);

• Main log path (logger.dart:73-83) uses 5 layers:

  MaskingUtils.encryptJNAPAuth(
    MaskingUtils.maskUsernamePasswordBodyValue(
      MaskingUtils.maskSensitiveJsonValues(
        MaskingUtils.maskSerialNumber(
          MaskingUtils.maskMacAddress(stripped)))))

• Gap: maskUsernamePasswordBodyValue and encryptJNAPAuth absent from state-cache path. If any future DiagnosticLoggable model exposes username/password/JNAP-auth fields in namedProps, diagnostic reports would contain less-masked data than the main log stream.
• Current exposure: Low — no known model exposes raw credentials in namedProps. This is a defensive gap, not an active leak.
• Fix: Add the two missing layers for consistency with main log path.

---

W-4 — diagnosticName not abstract/enforced · 🟡Med [both reviewers] (carry-over from Round 2)

• Location: lib/framework/diagnostic_loggable.dart:47
• Code (verified at head sha):

  String get diagnosticName => runtimeType.toString();

• This PR correctly adds overrides to 30+ models. However, any future DiagnosticLoggable implementor that omits diagnosticName silently falls back to runtimeType.toString(), re-introducing S-4 collision/minification risk with no compile-time warning.
• Fix (preferred): Make diagnosticName abstract. If too disruptive, add a debug-mode assertion in StateLogObserver to catch collision at development time.

---

W-doc — _genReqId() doc comment example incorrect · 🟢High [both reviewers]

• Location: lib/core/usp/services/usp_client.dart:99-100
• Code (verified at head sha):

  /// Generates a unique request ID: LNU{HEX-MS-TIMESTAMP}{4-CHAR-RANDOM}
  /// e.g., LNU18F3A2B4C5D6E7F8

• Problem: 18F3A2B4C as a ms-epoch hex equals ~6.7 seconds from Unix epoch (1970) — plainly not a realistic example. Real 2025 ms timestamps are ~13 decimal digits → 11 hex chars (e.g. 19126BE2710). The example doesn't break anything but misleads maintainers about ID anatomy.
• Fix:

  /// e.g., LNU19126BE27101A2F
  ///          ^^^^^^^^^^^  ^^^^
  ///          ~11-char ms  4-char rand

✅ What looks good (this round)

• W-1 fix is clean and correct: Removing no-op .select((c) => c) is right; DemoThemeConfig changes are infrequent enough that whole-object watch is fine.
• W-2 fix solid: 16-bit random + ms-precision timestamp; birthday threshold at 286 concurrent same-ms requests — well above burst scenarios.
• themeConfigProvider.select() preserved: ref.watch(themeConfigProvider.select((v) => v.valueOrNull)) still in place at usp_top_bar.dart:102 — only the demo config watch changed.
• updateStateLog() confirmed defined: Verified at logger.dart:277 in head commit — no compile-time gap.
• state_log_observer.dart does mask: 3-layer masking (MAC, serial, sensitive JSON) confirmed at head. W-3 is a hardening recommendation, not a present leak.
• Doc comment in logger.dart is accurate: Correctly reflects that _addLogWithTag no longer handles 'State' specially.

Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.

[AustinChangLinksys]

🤖 Automated Review — Round 2 · 222d9ce..47bf49d (incremental)

Verdict: 💬 Self-review (comment only) — Round 2 addresses several prior warnings. Three carry-over Warnings remain (masking chain, diagnosticName enforcement, doc comment); four new items found.

	Conf.	Where	Issue (one-liner)
⚠️	High	state_log_observer.dart:41	[both reviewers, CARRY-OVER W-3] Masking chain 3/5 layers — missing maskUsernamePasswordBodyValue + encryptJNAPAuth vs main log path
⚠️	Med	diagnostic_loggable.dart:46	[both reviewers, CARRY-OVER W-4] diagnosticName not abstract/enforced; future implementors silently fall back to mangled runtimeType in dart2js
⚠️	High	logger.dart:196	[both reviewers, NEW] Stale _addLogWithTag doc comment still references the deleted tag == 'State' branch
⚠️	Med	wifi_data_provider.dart:52	[single, NEW] WifiData.props includes Map/List objects compared by identity, not value equality; same-content different-instance maps will be equal
💡	High	usp_client.dart:104	[NEW] nextInt(0xFFFF) off-by-one — range [0, 0xFFFE], value 0xFFFF (65535) unreachable
💡	High	usp_client.dart:99	[CARRY-OVER W-doc] Doc comment example LNU18F3A2B4C5D6E7F8 does not accurately reflect actual format
💡	High	state_log_observer_test.dart	[NEW] No test verifying masking is applied before writing to state cache
💡	High	diagnostic_loggable_test.dart	[NEW] No test for diagnosticName default/override correctness
✅	High	state_log_observer.dart	[RESOLVED] tag == 'State' special-casing removed from _addLogWithTag — masking path is cleaner
✅	High	wifi_data_provider.dart:57	[RESOLVED] codegenContext now included in props for equality; raw codegen excluded from namedProps (good security posture)

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-3: Masking chain 3/5 layers in state_log_observer.dart [both reviewers, CARRY-OVER, High]

lib/core/utils/state_log_observer.dart:41-46:

final maskedState = MaskingUtils.maskSensitiveJsonValues(
    MaskingUtils.maskSerialNumber(MaskingUtils.maskMacAddress(jsonState)));
updateStateLog(typeName, maskedState);

The main web log path in logger.dart applies 5 layers:

MaskingUtils.encryptJNAPAuth(
  MaskingUtils.maskUsernamePasswordBodyValue(
    MaskingUtils.maskSensitiveJsonValues(
      MaskingUtils.maskSerialNumber(
        MaskingUtils.maskMacAddress(stripped)))));

The diagnostic report (outputFullWebLog) outputs both log sections to the same downloadable file. The state cache subsection has weaker PII protection than the main log subsection. While maskUsernamePasswordBodyValue and encryptJNAPAuth are unlikely to match typical JSON state, future DiagnosticLoggable implementors could inadvertently include authentication strings.

Fix: Apply the full 5-layer chain in state_log_observer.dart to match logger.dart.

---

W-4: diagnosticName not abstract/enforced [both reviewers, CARRY-OVER, Med]

lib/framework/diagnostic_loggable.dart:46:

String get diagnosticName => runtimeType.toString();  // unsafe default on web prod

All 30+ models touched in this PR add the override correctly. But new classes that use with DiagnosticLoggable and forget the override will silently produce mangled cache keys in dart2js production builds (minified names like "a" or "b$"), potentially colliding with other types.

Fix (Option A): Make abstract (requires all implementors to add override).
Fix (Option B, preferred): Add @mustBeOverridden annotation. At minimum add a warning comment: // WARNING: runtimeType is mangled in dart2js production. Always override.

---

W-5: Stale _addLogWithTag doc comment [both reviewers, NEW, High]

lib/core/utils/logger.dart:196-202:

/// If the `tag` is 'State', the message is parsed to update the [stateLogCache].
/// Otherwise, the message is added to the corresponding list in [_webLogCache],

The tag == 'State' branch was deleted in this very commit. The doc comment is now factually incorrect and misleading to future maintainers.

Fix: Replace the stale sentence with: /// Adds the message to the corresponding list in [_webLogCache], removing the oldest entry if the list exceeds its maximum size.

---

W-6: WifiData.props Map/List identity equality [single, NEW, Med]

lib/page/wifi_settings/providers/wifi_data_provider.dart:52-65:

@override
List<Object?> get props => [
  codegenContext,
  wifiClientMap,       // Map — Dart == is identity for Map, not value equality
  connectionDetailMap,
  radioModels,         // List — same issue
];

Equatable uses == on these map/list instances. Two WifiData objects with identical contents but different instances will not be equal, potentially causing spurious Riverpod rebuilds on every SSE re-fetch that creates fresh collections.

Fix (Option A): Keep the length-based comparison (the pre-existing pattern in the codebase), but add codegenContext: 'codegenContext': codegenContext, 'wifiClientMap_len': wifiClientMap.length, ... in namedProps and remove the explicit props override.
Fix (Option B): Add package:collection deep equality and document the intent explicitly.

What looks good

• State log masking introduced — even at 3/5 layers, this is a meaningful improvement over the previous state where the cache path had no masking at all.
• codegenContext in equality (RESOLVED) — correctly fixes a pre-existing gap where WifiData equality silently ignored the codegen context. Raw codegen excluded from namedProps is also a good security practice.
• tag == 'State' branch removal (RESOLVED) — the old path parsed state logs via regex through the main log stream, which was fragile and used partial masking. Direct write path in StateLogObserver is cleaner and more reliable.
• 30+ diagnosticName overrides — all touched models consistently add the string-literal override. The approach works for all existing classes; only new ones are at risk.
• usp_wifi_data_service.dart fallback exception logging — changing catch(_) to catch(e) { logger.w(...) } fixes a silent failure that was previously invisible to diagnostics.
• No PrivacyGUI architecture layer violations in this diff.

Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.

[PeterJhongLinksys]

Verified the correctness-sensitive parts of this refactor:

• Equatable (v2.0.8) deep-compares Map/Iterable props via objectsEquals, so the props => namedProps.values.toList() derivation preserves value equality — the Map/List reference-equality concern from the automated self-reviews does not apply to Equatable-backed classes.
• Riverpod setState stores the new value before the updateShouldNotify check, so equality-set changes affect only rebuild notifications, never cause stale ref.read() reads (mutations that consume codegenContext remain correct).
• Trace-level demotions + _AppLogFilter correctly gate dev-only logs out of release; SSE pretty-print moved to logger.t.
• Old TopBar deletion has its only reference migrated to UspTopBar; no dangling refs.

Non-blocking (not previously raised): DevicesData now derives equality from namedProps and drops codegenContext from its equality set — the same pattern you explicitly restored for WifiData. It's benign here (consumers read codegenContext via ref.read, and Riverpod retains the latest state), but adding an explicit props override for consistency with WifiData would avoid future confusion.

The masking-chain gap, diagnosticName enforcement, and doc-comment items already captured in the self-review threads remain reasonable hardening follow-ups. No blocking issues found.

[AustinChangLinksys]

🤖 Automated Review — Oversize PR

This round's changes exceed the automated-review limit (8115 lines / 125 files,
limit 6000 lines / 100 files); AI review was not run. Manual review recommended.

First 15 changed files (for a quick scan):

.claude/settings.json
doc/usp/vendored-artifacts.md
lib/ai/providers/usp_command_provider.dart
lib/generated/gre_tunnel.g.dart
lib/generated/index.dart
lib/generated/l2tp_tunnel.g.dart
lib/generated/ppp_interface.g.dart
lib/generated/wi_fi_access_points.g.dart
lib/l10n/app_ar.arb
lib/l10n/app_da.arb
lib/l10n/app_de.arb
lib/l10n/app_el.arb
lib/l10n/app_en.arb
lib/l10n/app_es.arb
lib/l10n/app_es_ar.arb

[PeterJhongLinksys]

Verified the TopBar unification updates all consumers (ui_kit_page_view.dart is UspTopBar check + import, top_bar.dart deleted with no dangling refs) and that state logging is masked and kept out of the main log stream, with trace logs filtered in release. No blocking issues.

Non-critical (optional follow-up): FirewallData.namedProps narrows equality to ruleCount: ruleSummaries.length and drops ruleContext/dmzSummaries. This is the same equality-narrowing class already addressed for WifiData; consider an explicit props override mirroring that fix for consistency. Non-blocking.

[HankYuLinksys]

Code review — verified against fix/unify-top-bar @ 3c45b28

Solid direction and great test coverage on DiagnosticLoggable / StateLogObserver. Three points worth addressing before merge (one is a real correctness regression), followed by what stands out as well done.

🔴 1. FirewallData equality is silently narrowed → dashboard card can stop rebuilding

Because the mixin derives props from namedProps.values (diagnostic_loggable.dart), and FirewallData.namedProps is:

namedProps => {
  'firewallModel': firewallModel,
  'ruleCount': ruleSummaries.length, // was: full ruleSummaries list
  'dmzModel': dmzModel,              // dmzSummaries dropped from equality entirely
};

FirewallData is an L1 Data provider; SSE invalidation rebuilds it and Riverpod uses == to decide whether to notify listeners. usp_firewall_overview_card.dart aggregates each rule's enabled (activeCount, the pie chart) and dmzSummaries. If a firewall rule's enabled toggles without changing the rule count (and it isn't one of the named toggles tracked by firewallModel), the new and old FirewallData compare equal → listeners are not notified → the card keeps showing stale data. dmzSummaries changes are likewise invisible to equality now.

Notably WifiData already anticipated exactly this and kept a separate props override ("namedProps is kept lean for diagnostic JSON output") — FirewallData should do the same: keep a full props (including ruleSummaries, dmzSummaries, ruleContext) and let namedProps stay lean. This also aligns with Article XI (Equatable is for state comparison; equality shouldn't be sacrificed for log output).

(For the record, the other models moved the opposite direction — PortForwardingData/PortTriggeringData/DevicesData went from .length to full lists, i.e. safer/more correct. FirewallData is the one that regressed.)

🟡 2. StateLogObserver runs unconditionally in production, masking on every state update

main.dart registers StateLogObserver() in ProviderScope.observers with no build-mode gate. On every DiagnosticLoggable state update it runs toString() (full jsonEncode) plus three full-string regex passes (maskMacAddress → maskSerialNumber → maskSensitiveJsonValues) — in release builds too. For large states like DevicesData (full device list) this fires on every mesh/SSE refresh. The previous mechanism only wrote the cache when someone actively logged with the State tag, so this is a meaningful cost increase. Consider gating to debug/profile, deferring masking until the user actually downloads the diagnostic report, or capping state size — or add a perf note if measurements show it's negligible.

🟡 3. _genReqId can collide (log-only, minor)

usp_client.dart replaced the monotonic ++_reqId with LNU{hex-ms}{4-hex-random} via _random.nextInt(0xFFFF). Two requests in the same millisecond have a ~1/65535 chance of colliding, which would make request/response log correlation misleading. The old counter guaranteed uniqueness. Impact is limited to log tracing, so acceptable — just flagging the trade-off.

✅ What's done well

• DiagnosticLoggable._toJsonSafe handles null/bool/num/String/Enum/DateTime/Duration/Map/Iterable/nested-Loggable with a try/catch fallback, and has thorough tests (nested, lists, maps, non-string keys, unicode, escaping).
• StateLogObserver correctly skips AsyncLoading/AsyncError/null and masks MAC/serial/password before caching — the diagnostic report is user-downloadable, so masking first is the right call, and it's covered by a dedicated test.
• diagnosticName override avoids dart2js minification mangling runtimeType — good catch.
• Trace-level migration and the [SSE] tag consolidation match the [USP]/[SSE] grep-prefix convention.
• The DHCP dialog test now drops focus to trigger on-blur _validate() — a genuine test-correctness fix.

[AustinChangLinksys]

🤖 Automated Review — Oversize PR

This round's changes exceed the automated-review limit (8123 lines / 125 files, limit 6000 lines / 100 files); AI review was not run. Manual review recommended.

First 15 changed files (for a quick scan):

.claude/settings.json
doc/usp/vendored-artifacts.md
lib/ai/providers/usp_command_provider.dart
lib/generated/gre_tunnel.g.dart
lib/generated/index.dart
lib/generated/l2tp_tunnel.g.dart
lib/generated/ppp_interface.g.dart
lib/generated/wi_fi_access_points.g.dart
lib/l10n/app_ar.arb
lib/l10n/app_da.arb
lib/l10n/app_de.arb
lib/l10n/app_el.arb
lib/l10n/app_en.arb
lib/l10n/app_es.arb
lib/l10n/app_es_ar.arb

[AustinChangLinksys]

Thanks for the review. Addressed the non-critical follow-up on FirewallData in b233a31.

Mirrored the WifiData fix: namedProps stays lean for diagnostic JSON, and props is now an explicit override covering the full field list — [firewallModel, ruleContext, ruleSummaries, dmzModel, dmzSummaries] — so equality no longer narrows to ruleSummaries.length or drops ruleContext/dmzSummaries. Added regression tests asserting that a content-only rule change and a DMZ-only change both break equality (they fail without the override), plus one confirming namedProps stays lean.

[HankYuLinksys]

Verified the FirewallData fix in b233a31 — issue #1 is resolved.

props is now an explicit override covering the full field list [firewallModel, ruleContext, ruleSummaries, dmzModel, dmzSummaries], mirroring WifiData, while namedProps stays lean for diagnostic JSON. Equality no longer narrows to ruleSummaries.length or drops ruleContext/dmzSummaries.

Confirmed the 3 new regression tests pass on the branch, and verified they're effective — removing the props override makes the content-only and DMZ-only cases fail as expected.

Approving. The two minor follow-ups (StateLogObserver running unconditionally in release + masking on every update; _genReqId collision) are non-blocking and can be deferred.