fix(diagnostics): disable Speed Test across app pending FW support (#857)

[AustinChangLinksys]

Summary

Speed Test relies on TR-181 IP.Diagnostics paths that firmware does not support (#857). The dashboard card was already disabled, but three user-facing entry points remained live:

• the Speed Test menu entry,
• the standalone uspSpeedTest route (still reachable by direct URL on web), and
• the speed-test step inside the Network Diagnostics flow.

This PR disables all remaining entry points and removes the speed-test step from the diagnostics flow, bringing the whole app in line with the already-disabled dashboard card.

Changes

• menu (usp_menu_view.dart): comment out the Speed Test menu entry
• route (route_usp_dashboard.dart, router_provider.dart): comment out the uspSpeedTest route + SpeedTestView import
• diagnostics notifier (unified_diagnostics_notifier.dart): remove the speed-test blocks from both the full and internet flows; drop the now write-only connectivityOk tracking; remove the unused _runSharedSpeedTest / _evaluateSpeedTest helpers, the speed-test recommendation case, and speed-test imports
• running view (diagnostic_running_view.dart): remove speed-test checklist entries and adjust the (discarded) step counts (7 to 6, 10 to 9)
• test (unified_diagnostics_notifier_test.dart): update internet-flow result count (7 to 6)

Kept for future restore

The DiagnosticStep.runningSpeedTest enum value, SpeedTestView, speedTestProvider/notifier, and the speed-test models are intentionally left in place (as dead code) so the feature can be restored once FW supports it — no test edits or enum-removal blast radius required.

Test plan

• ./run_tests.sh — 3,327 / 3,327 passed, 0 fail, 0 skip
• flutter analyze on changed files — no new issues (only pre-existing curly_braces info lints)
• dart format — clean (0 changed)
• menu golden localization test — passes

🤖 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 · d796b51..1a7aadd (full)

Verdict: 💬 Self-review (comment only) — Clean feature-disable PR; no Criticals found. Three Warnings flagged (inconsistent dead-code treatment, re-enablement trap, missing test assertion). Verdict forced COMMENT per self-review rule.

Standards

	Conf.	Where	Issue (one-liner)
⚠️	🟢High	diagnostic_running_view.dart:5,74	[Dead Code] Live speedTestProvider import + ref.watch in permanently-unreachable branch; all other touched files got imports commented out
⚠️	🟢High	usp_speed_test_card.dart:27	[Dead Code] detailRoute: RouteNamed.uspSpeedTest references deregistered route; go_router crash if card re-enabled without also re-enabling route
⚠️	🟢High	unified_diagnostics_notifier_test.dart:~753	[Incomplete Coverage] Full-flow test has no results.length == 9 assertion after removing speed-test step (internet flow correctly updated 7→6; full flow not locked)
⚠️	🟡Med	diagnostic_running_view.dart:142	[Dead Code] Third int of _getFlowMeta tuple always discarded (_) at call site; pre-existing, but touched by this PR's step-count edits
💡	🟢High	diagnostic_state.dart:158, notifier:185,254,281	DiagnosticState.speedTest field + clearSpeedTest: true calls now permanently no-op; consider tombstoning with #857 comments like the imports
💡	🟢High	usp_menu_view.dart:117	Pre-existing System Logs commented block has no issue reference, unlike this PR's consistent #857 annotation style

Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
[X] = 1 agent · [XX] = 2 agents flagged independently → higher confidence (Critical only).

Spec

	Conf.	Where	Issue (one-liner)
⚠️	🟢High	diagnostic_running_view.dart:5,74	PR claims to disable all entry points; live speedTestProvider import+watch left active in this file inconsistently
💡	🟢High	unified_diagnostics_notifier_test.dart:106	Dead speedTestProvider.overrideWith() + _MockSpeedTestNotifier in test scaffolding; notifier no longer uses this provider
💡	🟢High	unified_diagnostics_notifier_test.dart:~753	Full-diagnostic test has no results.length assertion to lock the claimed 10→9 step reduction
💡	🟡Med	golden test running_speed_test scenario	Tests now-unreachable state; consider annotating with // intentional dead-state coverage (#857)

Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
[X] = 1 agent · [XX] = 2 agents flagged independently → higher confidence (Critical only).

⚠️ Warning details

1. diagnostic_running_view.dart:5,74-76 — Dead speedTestProvider import + unreachable watch 🟢High
(Flagged by all three reviewers: A+B Standards axis + C Spec axis)

unified_diagnostics_notifier.dart and router_provider.dart both had their speed-test imports commented out in this PR. However diagnostic_running_view.dart still has:

// line 5 — live import, NOT commented out
import 'package:privacy_gui/page/unified_diagnostics/providers/speed_test_notifier.dart';

// lines 74-76 — branch permanently dead (runningSpeedTest removed from all flow lists at :113, :133)
if (isCurrent && step == DiagnosticStep.runningSpeedTest) {
  final speedState = ref.watch(speedTestProvider).valueOrNull;
  progressDetail = speedState?.progressMessage;
}

The if can never fire. The import is still compiled, causing Riverpod to register speedTestProvider on widget load. No immediate crash, but inconsistent with the PR's own treatment of other files. If runningSpeedTest is ever re-added to a flow list without re-enabling the notifier-side logic, this watch silently reads null state with no visible error.

Fix: Comment out lines 5 and 72-77 with // Speed Test disabled: blocked by FW support (#857), consistent with the pattern used in unified_diagnostics_notifier.dart.

---

2. usp_speed_test_card.dart:27 — Broken detailRoute reference 🟢High
(Flagged by Reviewers A+B)

// usp_speed_test_card.dart:27
detailRoute: RouteNamed.uspSpeedTest,  // reference to deregistered route

The uspSpeedTest route was commented out in route_usp_dashboard.dart. The card factory already comments the card out (usp_widget_factory.dart), so no live caller exists now. But if a developer re-enables the factory line without also re-enabling the route, context.pushNamed(RouteNamed.uspSpeedTest) (via DashboardCardTemplate) will throw GoException: There is no route with the name "uspSpeedTest" at runtime.

Fix: Add a guard comment at line 27: // Route must be re-registered before re-enabling this card (#857) — or set detailRoute: null so the card degrades gracefully if accidentally instantiated.

---

3. unified_diagnostics_notifier_test.dart:~753 — Missing full-flow results.length assertion 🟢High
(Flagged by Reviewers B+C)

The internet-flow test was correctly updated:

// line 173 — correctly updated
expect(state.results.length, 6); // was 7; Speed test disabled: #857

The full-diagnostic test only asserts expect(state.results, isNotEmpty) — no exact count. Post-PR the expected count is 9 (was 10). A future regression re-adding speed test to the full flow would not be caught.

Fix: Add expect(state.results.length, 9); after the existing isNotEmpty assertion in the full-flow happy-path test, with a comment enumerating the 9 steps.

---

4. diagnostic_running_view.dart:142 — _getFlowMeta third tuple element always discarded 🟡Med
(pre-existing; exposed by this PR's step-count edits)

// call site
final (flowIcon, flowTitle, _) = _getFlowMeta(flow);  // third element always dropped

The step-count integer in _getFlowMeta is never used. This PR correctly decremented it (7→6, 10→9) but the values have no runtime effect. Future maintainers may spend time keeping them accurate without knowing they do nothing.

Fix (optional): Either use the count (e.g., as a progress-bar denominator) or collapse the tuple to (IconData, String) and delete the dead integer.

✅ What looks good

• All three entry points disabled: menu entry, uspSpeedTest route (prevents direct-URL access on web), and speed-test step in diagnostics — comprehensive sweep matching the PR description.
• Both diagnostic flow methods patched: _runFullDiagnosticFlow and _runInternetDiagnostics both had their speed-test blocks removed; no partial removal.
• connectivityOk cleanly removed: all six tracking assignments correctly removed since the variable's sole purpose was gating the speed test.
• Helper methods fully deleted: _runSharedSpeedTest() and _evaluateSpeedTest() removed from the notifier — no dead helpers remain.
• Test count correctly updated: internet-flow expect(state.results.length, 6) is precise and includes an explanatory comment.
• Consistent #857 annotation style: every disabled block annotated for easy future grep/restore.
• Dead-code preservation strategy sound: keeping DiagnosticStep.runningSpeedTest enum, SpeedTestView, speedTestProvider, and models intact is architecturally justified; avoids re-implementation blast radius.
• No security findings: purely subtractive PR — no new secrets, injection surfaces, or auth bypass introduced.
• Step counts verified: internet=6 and full=9 match the actual step lists post-removal.
• flutter analyze clean: no new issues per PR description.

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

[AustinChangLinksys]

🤖 Automated Review — Round 1 · d796b51..1a7aadd (full)

Verdict: 💬 Self-review (comment only) — Clean feature-disable PR; 0 Criticals. Six Warnings (four carried from prior round, two new), five Suggestions. Verdict forced COMMENT per self-review rule.

Standards

	Conf.	Where	Issue (one-liner)
⚠️	🟢High	diagnostic_running_view.dart:5,74	[Dead Code] Live speedTestProvider import+ref.watch in permanently-unreachable branch; all other touched files had imports commented out
⚠️	🟢High	usp_speed_test_card.dart:27	[Dead Code] detailRoute: RouteNamed.uspSpeedTest references deregistered route; go_router crash if card re-enabled without route
⚠️	🟢High	unified_diagnostics_notifier_test.dart:751	[Incomplete Coverage] Full-flow test asserts isNotEmpty only; results.length == 9 lock missing after speed-test step removal
⚠️	🟡Med	diagnostic_running_view.dart:142	[Dead Code] _getFlowMeta third int always discarded at call site; pre-existing, but touched by this PR's step-count edits
⚠️	🟢High	diagnostic_running_view.dart:206-209	[NEW] [Dead Code] Unreachable _getStepSummary speed-test branch contains unsafe rawData['downloadMbps'] as double cast; silent crash if accidentally triggered
⚠️	🟡Med	unified_diagnostics_notifier.dart:26	[NEW] [Dead Code] Class-level doc "Scenario B" still lists Speed Test; now describes a non-existent flow
💡	🟢High	diagnostic_running_view.dart:183	[Dead Code] Unreachable runningSpeedTest case in _getStepLabel switch
💡	🟢High	unified_diagnostics_notifier.dart:363→377	Step comment numbering jumps 4→6 after Speed Test removal; sub-steps carry stale numbers
💡	🟢High	diagnostic_state.dart:158	DiagnosticState.speedTest field + clearSpeedTest: true calls now permanently no-op; consider tombstone #857 comment
💡	🟡Med	unified_diagnostics_notifier.dart:320	Scope comment stale: "ping and speed test operations" — speed test now removed
💡	🟡Med	usp_menu_view.dart:117	Pre-existing: System Logs commented block has no issue reference (unlike this PR's consistent #857 style)

Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
[X] = 1 agent · [XX] = 2 agents flagged independently → higher confidence (Critical only).

Spec

	Conf.	Where	Issue (one-liner)
⚠️	🟢High	diagnostic_running_view.dart:5,74	PR claims to disable all entry points; live speedTestProvider import+watch left active in this file
⚠️	🟡Med	diagnostic_running_view.dart:206-209	[NEW] Unreachable _getStepSummary speed-test branch is inconsistent with "disable all entry points" claim
⚠️	🟡Med	usp_speed_test_card.dart:27	Defensive gap (not in diff, pre-existing): detailRoute: RouteNamed.uspSpeedTest references removed route; factory blocks it today but latent go_router crash if factory is re-enabled
💡	🟢High	unified_diagnostics_notifier_test.dart:106	Dead speedTestProvider.overrideWith() + _MockSpeedTestNotifier in test scaffolding; notifier no longer uses this provider
💡	🟢High	unified_diagnostics_notifier_test.dart:751	Full-diagnostic test has no results.length assertion to lock the claimed 10→9 step reduction
💡	🟡Med	golden running_speed_test scenario	Tests now-unreachable state; consider annotating with // intentional dead-state coverage (#857)

Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
[X] = 1 agent · [XX] = 2 agents flagged independently → higher confidence (Critical only).

⚠️ Warning details

1. diagnostic_running_view.dart:5, 74-76 — Dead speedTestProvider import + unreachable watch 🟢High
(Flagged by Reviewers A+B, prior round + confirmed this round)

// line 5 — live import, NOT commented out
import 'package:privacy_gui/page/unified_diagnostics/providers/speed_test_notifier.dart';

// lines 74-76 — branch permanently dead (runningSpeedTest removed from all flow lists)
if (isCurrent && step == DiagnosticStep.runningSpeedTest) {
  final speedState = ref.watch(speedTestProvider).valueOrNull;
  progressDetail = speedState?.progressMessage;
}

_getFlowSteps() no longer returns runningSpeedTest for any flow. The if can never fire. However, speedTestProvider is still subscribed via ref.watch on every DiagnosticRunningView rebuild, keeping the provider alive unnecessarily. Inconsistent with how other files handle the same (imports commented out in router_provider.dart, unified_diagnostics_notifier.dart).

Fix: Comment out line 5 and lines 72-77 with // Speed Test disabled: blocked by FW support (#857).

---

2. usp_speed_test_card.dart:27 — Broken detailRoute reference 🟢High
(Flagged by Reviewers A+B, prior round + confirmed this round)

// usp_speed_test_card.dart:27
detailRoute: RouteNamed.uspSpeedTest,  // references deregistered route

The uspSpeedTest route was commented out in route_usp_dashboard.dart:219-224. The card factory already disables the card (usp_widget_factory.dart:49), so no live caller exists today. But if a developer re-enables the factory line without also re-enabling the route, context.pushNamed(RouteNamed.uspSpeedTest) will throw GoException: There is no route with the name "uspSpeedTest" at runtime.

Fix: Add guard comment at line 27 or set detailRoute: null for graceful degradation. Alternatively, annotate the entire card class with a #857 disable comment consistent with the rest of the PR.

---

3. unified_diagnostics_notifier_test.dart:~751 — Missing full-flow results.length assertion 🟢High
(Flagged by Reviewers A+B+C, prior round + confirmed this round)

// line 751-753 — only isNotEmpty, no precise count
final state = container.read(unifiedDiagnosticsProvider);
expect(state.step, DiagnosticStep.showingResults);
expect(state.results, isNotEmpty);  // ← should be results.length == 9

The internet-flow test was correctly updated (expect(state.results.length, 6)). The full-flow test still uses isNotEmpty. Post-PR the expected count is 9 (was 10). A future regression re-adding speed test to the full flow without updating the test would not be caught.

Fix: Add expect(state.results.length, 9); // WAN + DHCP Pool + pingGateway + pingDns + dnsLookup + pingInternet + WiFi + Mesh + Devices after the existing assertion.

---

4. diagnostic_running_view.dart:142 — _getFlowMeta third tuple element always discarded 🟡Med
(pre-existing; touched by this PR's step-count edits)

// call site (line ~19)
final (flowIcon, flowTitle, _) = _getFlowMeta(context, state);  // third element always dropped

The step-count integer in _getFlowMeta (now 6 for internet, 9 for full) is computed but never used at the call site. This PR correctly decremented the values, but they have no runtime effect.

Fix (optional): Either use the count (e.g., as progress-bar denominator) or collapse the tuple to (IconData, String).

---

5. diagnostic_running_view.dart:206-209 — Unreachable _getStepSummary speed-test branch 🟢High (NEW this round)

// line 206-209 — permanently unreachable
_ when result.step == DiagnosticStep.runningSpeedTest => result.rawData
        .containsKey('downloadMbps')
    ? '${(result.rawData['downloadMbps'] as double).toStringAsFixed(1)} Mbps'
    : loc(context).completed,

_getStepSummary is called with items from state.results, populated by the diagnostics flows. runningSpeedTest is removed from all flows → this branch can never be reached. Additionally, result.rawData['downloadMbps'] as double is an unchecked dynamic cast — if somehow triggered (e.g., re-enabling the step without updating this switch), it will throw TypeError at runtime.

Fix: Remove this when branch (can be part of the same cleanup commit as item 1).

---

6. unified_diagnostics_notifier.dart:26 — Stale class-level doc "Scenario B" 🟡Med (NEW this round)

/// - Scenario B: Slow Network (Speed Test → WiFi → Devices → Traceroute)

Speed Test has been removed from the flow. The doc now describes a non-existent scenario and will mislead future maintainers.

Fix:

/// - Scenario B: Slow Network (WiFi signal → DHCP → Mesh → Devices)
///   // Speed Test: disabled pending FW support (#857)

✅ What looks good

• All three user-facing entry points disabled: menu entry, uspSpeedTest route (blocks direct URL on web), and speed-test step in diagnostics — comprehensive sweep matching the PR description.
• Both diagnostic flow methods patched: _runFullDiagnosticFlow and _runInternetDiagnostics both had their speed-test blocks removed; no partial removal.
• connectivityOk cleanly removed: all six tracking assignments correctly removed since the variable's sole purpose was gating the speed test.
• Helper methods fully deleted: _runSharedSpeedTest() and _evaluateSpeedTest() removed from the notifier — no dead helpers remain in the notifier.
• Test count correctly updated: internet-flow expect(state.results.length, 6) is precise and includes an explanatory comment.
• Consistent #857 annotation style: every disabled block annotated for easy future grep/restore.
• Dead-code preservation strategy sound: keeping DiagnosticStep.runningSpeedTest enum, SpeedTestView, speedTestProvider, and models intact is architecturally justified — avoids re-implementation blast radius.
• No security findings: purely subtractive PR — no new secrets, injection surfaces, or auth bypass introduced.
• flutter analyze clean: no new issues per PR description.
• 3,327/3,327 tests passing: full test suite green.

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

[HankYuLinksys]

Code Review

Overall this looks good and correctly scoped. The three remaining Speed Test entry points (menu, uspSpeedTest route, and the speed-test step in the diagnostics flow) are disabled to match the already-disabled dashboard card. I verified the PR-head versions of unified_diagnostics_notifier.dart and diagnostic_running_view.dart compile cleanly with no dangling SpeedTestResult / speedTestProvider / connectivityOk references, and no live caller still points at the dead uspSpeedTest route.

A few notes below — none block merge.

🟢 Low — golden coverage for the diagnostics running view

test/golden_test/.../usp_unified_diagnostics_view_test.dart still has a running_speed_test case whose fixture sets step: DiagnosticStep.runningSpeedTest, flow: DiagnosticFlow.internet. Since this PR removes runningSpeedTest from the internet flow's _getFlowSteps, the current step is no longer in the step list, so that golden would render a state that's no longer reachable (no node is "current"). Also, the test plan only ran the menu golden, but this PR touches diagnostic_running_view.dart, which affects every golden that renders DiagnosticRunningView.

Goldens are gitignored (test/**/goldens/*), so this doesn't block CI compilation — it's a visual-regression coverage gap, not a functional bug. Suggest (as follow-up) dropping the running_speed_test case + runningSpeedTestState fixture and regenerating the unified_diagnostics goldens.

🟢 Informational

• _getFlowMeta count is a discarded value. diagnostic_running_view.dart destructures it as final (flowIcon, flowTitle, _) = ... and nothing reads the third element. The 7 → 6 / 10 → 9 edits are documentation-only with no runtime effect (this was pre-existing dead code, not introduced here).
• Commented-out vs deleted. Disabling via comment + #857 marker (rather than deletion) is a deliberate, issue-tracked choice for future restore — acceptable, and the consistent #857 anchors make the restore easy to grep. Just noting the usual preference for relying on git history over long-lived commented-out code.
• Recommendation switch: removing case DiagnosticStep.runningSpeedTest is safe — it's a statement-switch with default: break, and no result carries that step anymore.
• Test count 7 → 6 is correct. The Full Diagnostic Flow test uses isNotEmpty + verify() rather than an exact count, so the full flow dropping a step (10 → 9) won't fail existing tests — though it also means there's no exact-count regression guard for the full flow (pre-existing).

Nice work keeping the enum value, view, provider, and models in place as dead code with #857 anchors so the feature can be restored once FW supports it.

[HankYuLinksys]

LGTM. Correctly scoped disable of the remaining Speed Test entry points, matching the already-disabled dashboard card. Verified the PR-head sources compile cleanly with no dangling references. The golden coverage note is a follow-up, not a blocker.

[PeterJhongLinksys]

🤖 Automated Review — Round 1 · d796b51..1a7aadd (full)

Verdict: ✅ APPROVE — Functionally safe "disable Speed Test" change; no Critical and no architecture red-line violations, but the disable is done inconsistently (delete + comment-block + orphaned files) leaving several dead-code / stale-test warnings worth cleaning up before or alongside merge.

	Conf.	Where	Issue (one-liner)
⚠️	🟢	test/golden_test/page/unified_diagnostics/fixtures/unified_diagnostics_test_data.dart:127-131	[XX] Golden fixture runningSpeedTestState + running_speed_test variant still render the removed step → golden test likely goes RED / renders stale frame
⚠️	🟢	lib/page/dashboard/mascot/mascot_providers.dart:161 · lib/page/unified_diagnostics/services/diagnostic_report_service.dart:44	[XX] state.speedTest is now never written (always null) → if (speedTest != null) is a dead branch; speed-test section silently disappears from report & mascot (expected, but undocumented)
⚠️	🟢	lib/page/unified_diagnostics/views/widgets/diagnostic_running_view.dart:5,74	[XX] Still imports & ref.watch(speedTestProvider) in a now-unreachable branch → dead code + live coupling to a disabled provider (possible unused_import analyze warning)
⚠️	🟢	lib/page/menu/views/usp_menu_view.dart:131 · lib/route/route_usp_dashboard.dart:219 · notifier	[X] Inconsistent disable method: notifier deletes cleanly (-175 lines) but menu/route/import are large commented-out blocks → recommend a single build_config feature flag instead
⚠️	🟢	lib/page/unified_diagnostics/views/speed_test_view.dart · providers/speed_test_notifier.dart · cards/usp_speed_test_card.dart (whole files)	[X] Orphaned implementation files no longer reached by any live path → unreachable dead code (biggest maintainability gap)
⚠️	🟡	lib/page/unified_diagnostics/views/widgets/diagnostic_running_view.dart:149,170	[X] [Duplicated Code] hardcoded step counts 6/9 duplicate _getFlowSteps().length (this PR had to hand-edit 7→6, 10→9) → derive from list length
⚠️	🟢	lib/page/unified_diagnostics/models/diagnostic_state.dart:78 + switch sites	[X] Contested — Reviewer B: enum value + its switch branches (step_result_tile, diagnostic_result_card, diagnostic_report_service) are now unreachable dead code, clean up. Reviewer A: retention is correct — removing the enum breaks exhaustive switch. Maintainer to decide.
💡	🟢	lib/page/unified_diagnostics/cards/usp_speed_test_card.dart:27	Card still pushNamed(RouteNamed.uspSpeedTest); route now disabled → unreachable today (factory registration commented out) but a re-enable landmine → GoRouter runtime error
💡	🟡	test/page/unified_diagnostics/providers/unified_diagnostics_notifier_test.dart:170	Test only bumps expected count 7→6; add an explicit assertion that results exclude DiagnosticStep.runningSpeedTest to lock the behavior
💡	🟡	scattered #857 comments across 6+ files	[Shotgun Surgery] no single source of truth for the disable; a feature flag would centralize enable/disable

_{Confidence: 🟢High (read head code, evidence attached) · 🟡Med (file:line + reason, not fully proven) · ⚪Low (speculative). [X] = raised by 1 agent · [XX] = both agents.}

⚠️ Warning evidence

W1 — Golden fixture drives the removed step
unified_diagnostics_test_data.dart:127-131:

const runningSpeedTestState = UnifiedDiagnosticsState(
  step: DiagnosticStep.runningSpeedTest,
  flow: DiagnosticFlow.internet,
  progress: 0.45,
);

usp_unified_diagnostics_view_test.dart:39-41 still registers the 'running_speed_test' variant. But diagnostic_running_view.dart:105-113 (internet) & 127-138 (full) removed runningSpeedTest from the step lists, so when this state renders, no step matches isCurrent and the header count is now 6. The golden image no longer matches the old baseline → test fails (or needs --update-goldens). Not updated in this PR.

W2 — state.speedTest read but never written
Writers removed: unified_diagnostics_notifier.dart:375,522 are now // Speed test step disabled comments; only clearSpeedTest: true (185/254/281) remains. Readers still live:

• mascot_providers.dart:161-168: final speedTest = state.speedTest; if (speedTest != null) { message += ...Download... }
• diagnostic_report_service.dart:44-55: final speed = state.speedTest; if (speed != null) { '--- Speed Test ---' ... }

speedTest is now permanently null → both if branches are dead; report/mascot silently drop the speed-test section. No crash, but a silent feature-loss trap with no #857 note at the read sites.

W3 — Dead import + watch of disabled provider
diagnostic_running_view.dart:5 import '.../providers/speed_test_notifier.dart';, and :74-76:

if (isCurrent && step == DiagnosticStep.runningSpeedTest) {
  final speedState = ref.watch(speedTestProvider).valueOrNull;
  progressDetail = speedState?.progressMessage;
}

runningSpeedTest never appears in any flow list now → condition is always false → dead branch, and the import keeps a UI file coupled to a disabled provider.

W4/W5/W6 — inconsistent disable + orphaned files + magic numbers: see table; whole files speed_test_view.dart / speed_test_notifier.dart / usp_speed_test_card.dart + route constants constants.dart:45,206 are now unreachable. Step counts 6/9 at diagnostic_running_view.dart:149,170 duplicate the list lengths.

✅ What looks good

• _runSharedSpeedTest / _evaluateSpeedTest genuinely deleted (verified 0 hits repo-wide), not left as dead methods (-175 lines).
• Step counts are internally consistent: internet list = 6 steps ↔ meta 6; full list = 9 steps ↔ meta 9.
• No reachable dangling navigation to uspSpeedTest — repo-wide search found only the commented menu item and the unreachable card. No live goNamed/pushNamed to the deleted route.
• Architecture red-lines clean: no reverse-tier dependency, no generated/ import leaking into view/provider (Art. V §5.4), USP 4 hard rules intact (Art. IV — speedTestProvider is a legit L2 autoDispose), ServiceError isolation intact (Art. XIII); the removed catch (e) was existing notifier logic, no new on Exception catch misuse.
• Security: pure disable/comment change — no hardcoded secrets/tokens, no injection surface, no unsafe data handling.

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

[PeterJhongLinksys]

🤖 Automated Review — Round 1 · d796b51..1a7aadd (full)

Verdict: ✅ APPROVE — Clean, self-consistent temporary disable of the Speed Test feature pending FW support (#857); no dangling active references, compiles cleanly, test expectation updated to match.

	Conf.	Where	Issue (one-liner)
💡	🟡Med	lib/page/unified_diagnostics/providers/unified_diagnostics_notifier.dart, route_usp_dashboard.dart, router_provider.dart, usp_menu_view.dart	Feature disabled via large commented-out code blocks + commented imports rather than a build_config feature flag; acceptable for a short-lived revert but leaves dead text to rot if #857 slips.
💡	🟢High	lib/page/unified_diagnostics/views/widgets/step_result_tile.dart:37,139,272 · diagnostic_result_card.dart:206,265	Display branches still handle DiagnosticStep.runningSpeedTest, but the notifier no longer emits it → these become dead (but harmless) code paths.
💡	🟢High	lib/page/unified_diagnostics/views/widgets/diagnostic_running_view.dart:150,171	Total-step counts are hardcoded magic numbers (correctly updated to 6/9 this PR), but stay decoupled from _getFlowSteps list length — fragile on the next step change. Consider deriving from .length.

_{Confidence: 🟢High = verified against head code · 🟡Med = file:line + rationale, not fully traced · ⚪Low = speculative. [X]=1 agent / [XX]=2 agents (not shown here — reviewed directly).}

✅ What looks good

• The disable is complete and consistent: menu item, USP dashboard route, router_provider import, and the Step 5 speed-test logic in unified_diagnostics_notifier.dart (169-line block incl. _runSharedSpeedTest/_evaluateSpeedTest) are all removed/commented together — no half-disabled state.
• Verified no active (uncommented) references remain to RouteNamed.uspSpeedTest, SpeedTestView, _runSharedSpeedTest, or _evaluateSpeedTest anywhere in lib/ → no navigation to a non-existent route, no dead method calls, no compile break.
• DiagnosticState.speedTest field and clearSpeedTest are retained; the state model stays intact so a future revert is low-friction.
• Test expectation updated in lockstep: unified_diagnostics_notifier_test.dart now asserts 6 results (was 7) with a clear comment referencing #857.
• Every disabled block carries a uniform // ... blocked by FW support (#857) marker → easy to grep and revert.
• No architecture-rule impact: no generated-model leaks, no USP Provider four-rule changes, no ServiceError-boundary changes, no security-sensitive surface touched.

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