fix(pnp): distinguish router read failure from no-internet (#1098)

[HankYuLinksys]

Fixes #1098

Background / Root cause

During PnP first-time setup, the app runs an internet check after login. In the reported session the device returned an empty {} payload for every USP GET (396/396 empty), so the WAN read failed. The user saw a brief spinner, landed on the no-internet page, and "Try again" could never recover — even after the WAN cable was reconnected and the internet was genuinely back.

The internet check conflated two distinct outcomes into a single NoInternet state. They are already separated by control flow — the code just discarded that distinction:

• Path A — genuine no-internet. WanStatus.fetch succeeds, WAN Status != 'Up' → checkInternetConnected() returns false (no throw).
• Path B — read failure. The USP GET returns empty / missing fields → WanStatus.fetch throws (codegen fault 9998) → mapped to a ServiceError and rethrown.

PnpNotifier._checkInternet() had the else branch (path A) and the catch block (path B) both produce NoInternet. So a read failure — where we genuinely cannot tell the WAN state — was reported as the router confirming there is no internet, and "Try again" just looped.

Note: the fix keys off the control-flow boundary (fetch threw = read failure), not the 9998 code alone — the code is carried only as diagnostic detail. The reported session is unambiguous (396/396 GETs empty, incl. DeviceInfo): the whole data plane was unreadable.

Affected paths

_checkInternet() is not only triggered by the entry view — it has multiple callers, each in a different view context. It also does not rethrow: it writes the resulting phase itself and returns.

#	Trigger	View	Gap before this PR
1	startPostLoginFlow()	entry view	none — switch(phase) renders it
2	retryInternetCheck()	no-internet view	ref.listen only; read failure → spinner ends, stuck
3a/3b	saveIspWithProgress()	PPPoE / Static IP	listener misses AdminReadFailure → stuck on saving / blank
3c	saveIspWithProgress() (DHCP)	ISP settings	_onDhcpTap misses it → silently does nothing
4	saveIspSettingsAndCheck()	—	dead code, zero callers

The save-vs-read distinction inside saveIspWithProgress

Two different failure points must not share a fate:

1. saveIspSettings() write fails (bad value, PPP instance fail) → not a read failure. Stay on the ISP form + errorMessage snackbar so the user can fix it.
2. Save succeeds, trailing _checkInternet() read-fails → real read failure → AdminReadFailure.

Because _checkInternet() doesn't rethrow, case 2 is handled inside it and never reaches the saveIspWithProgress catch. Only case 1 reaches that catch — so it stays NoInternet + snackbar, unchanged.

Solution

Introduce a dedicated AdminReadFailure phase (replacing the former AdminError, whose only job was an error card + retry — a strict superset, so no orphan is left), carrying the ServiceError code/detail for diagnostics.

Provider

• startPostLoginFlow() + _checkInternet() catch: on ServiceError → AdminReadFailure (covers a read failure at either the SystemInfo or WanStatus step).
• saveIspWithProgress() catch: unchanged (NoInternet + errorMessage — write-failure path).
• Deleted dead code saveIspSettingsAndCheck().

Views — every read-failure path converges on the entry view (RoutePath.pnp): no-internet / PPPoE / Static IP listeners + DHCP _onDhcpTap each route to it on AdminReadFailure.

Why "navigate to entry" = implicit retry (important — non-obvious)

/pnp (PnpEntryView) and /pnpNoInternetConnection (+ its isp/pppoe/static-ip children) are two independent top-level route trees — the no-internet subtree is not nested under /pnp. The entry view reaches the no-internet subtree via context.go(...), which replaces the stack, so PnpEntryView is already unmounted by the time the user is there.

Therefore go(RoutePath.pnp) from paths #2/#3 is a cross-tree switch that mounts a fresh PnpEntryView, whose initState re-runs startPostLoginFlow(). Since pnpProvider is not autoDispose, the phase persists; startPostLoginFlow overwrites the lingering AdminReadFailure with AdminCheckingInternet and re-checks.

Net effect: back to entry → re-run the whole flow → only if it still fails does it settle on AdminReadFailure and render the error card (no infinite loop). If the transient condition cleared (e.g. FW warm-up finished), it proceeds straight into the wizard. Cost: one redundant check (a few seconds), which is the intended retry.

Two recovery behaviors coexist: the entry view renders the read-failure card in place (manual retry via the button); the other views navigate to entry (implicit retry).

Expected behavior after the fix

Scenario	Before	After
WAN read succeeds, Status != 'Up'	NoInternet	NoInternet (unchanged)
WAN read fails at entry	NoInternet, retry loops forever	AdminReadFailure → entry error card + retry
Read fails at SystemInfo step (reported session)	AdminError	AdminReadFailure (consistent)
"Try again" on no-internet page hits a read failure	stuck	navigate to entry → implicit retry → card if still failing
ISP save → trailing check read-fails	stuck / silent	navigate to entry → implicit retry
ISP save write fails	NoInternet + snackbar	NoInternet + snackbar (unchanged — stay on form)

Tests

• Read failure → AdminReadFailure, not NoInternet (regression for this bug).
• Read at SystemInfo fails → AdminReadFailure.
• saveIspWithProgress where save succeeds but trailing check read-fails → AdminReadFailure.
• Existing: save write failure stays NoInternet + errorMessage; genuine no-internet stays NoInternet.

No widget tests: the added navigation is a thin if (phase is AdminReadFailure) go(pnp) branch, and this feature's test suite is provider/service/model only. Core logic is covered by the provider tests above.

Scope note

The firmware-side trigger (why every USP GET returns empty after a successful login) still needs FW confirmation (need-fw-confirm). This PR is the UI-resilience fix, which also hardens against any future transient read failure.

[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 · b9c33cd..78b5ece (full)

Verdict: ✅ APPROVE — Core fix is correct and well-tested; four minor suggestions only.

	Conf.	Where	Issue (one-liner)
💡	🟡Med	pnp_notifier.dart:47-57 + :79-90	[both reviewers] Duplicated on ServiceError/catch pattern in two methods — minor DRY gap
💡	🟡Med	pnp_notifier.dart:471-477	saveIspWithProgress outer catch maps save-write failure to NoInternet — intentional but undocumented
💡	🟡Med	pnp_state.dart:AdminReadFailure	code/detail both nullable with no assert; a fully-null instance is undiagnosable
💡	⚪Low	pnp_notifier.dart (4 AdminReadFailure(detail:'$e') sites)	detail stores raw exception string in state; future UI misuse could surface internal error text

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.

Notes on false-positive findings from sub-agents (evidence trail)

Two items initially flagged as Critical by Reviewer B were confirmed as false positives upon reading HEAD source:

• pnp_entry_view.dart:63 — switch arm already reads AdminReadFailure() (not AdminError), compiles correctly.
• pnp_notifier_test.dart:130+ — all three new tests are complete with proper setup; old AdminError test was correctly updated to AdminReadFailure.

One item initially flagged as Warning by Reviewer A was confirmed as false positive:

• _checkInternet() on ServiceError scope — fetchCurrentSsid() at pnp_service.dart:547 has an internal catch (_) { return null; } that swallows all exceptions; ServiceError cannot propagate to the outer block, so the on ServiceError handler only covers checkInternetConnected() as intended.

💡 Suggestion Details

S1 — Duplicated catch pattern [both reviewers] (🟡Med)
pnp_notifier.dart:47-57 (startPostLoginFlow) and :79-90 (_checkInternet) each contain nearly identical on ServiceError + catch blocks differing only in the log tag:

} on ServiceError catch (e) {
  logger.e('[PnP] ... read failure: $e (code=${e.code})');
  state = state.copyWith(phase: AdminReadFailure(code: e.code, detail: '$e'));
} catch (e) {
  logger.e('[PnP] ... unexpected error: $e');
  state = state.copyWith(phase: AdminReadFailure(detail: '$e'));
}

Consider extracting a _setReadFailure(String tag, Object e) helper to avoid future divergence.

S2 — saveIspWithProgress outer catch semantics (🟡Med)
pnp_notifier.dart:471-477: the outer catch fires only for saveIspSettings() write failures (because _checkInternet() handles its own errors internally and never re-throws). Mapping a save failure back to NoInternet + errorMessage is intentional — user sees the ISP form again to retry — but this intent is undocumented. A brief comment like // Write failure: stay on NoInternet so user can retry the save would prevent future maintainers from inadvertently "fixing" this into AdminReadFailure.

S3 — AdminReadFailure nullable fields, no assert (🟡Med)
pnp_state.dart:

class AdminReadFailure extends PnpPhase {
  final int? code;
  final String? detail;
  const AdminReadFailure({this.code, this.detail});

Both fields are nullable. A const AdminReadFailure() with no context is undiagnosable in logs. Consider:

const AdminReadFailure({this.code, this.detail})
  : assert(code != null || detail != null,
      'AdminReadFailure: at least one of code/detail must be set for diagnostics');

S4 — detail stores raw exception string in state (⚪Low)
pnp_notifier.dart (four AdminReadFailure(detail: '$e') sites): ServiceError.toString() can include TR-181 paths or firmware error strings. The doc comment already says "for logging only" and pnp_entry_view.dart correctly uses loc(context).unableToGatherDeviceInfo (not detail). No current UI leakage. Suggest adding an explicit "MUST NOT surface in UI" note to the class-level doc or field doc.

✅ What looks good

• Core semantic distinction is correct: renaming AdminError -> AdminReadFailure and adding dedicated on ServiceError intercepts cleanly separates "router confirmed no internet" (returns false, no throw) from "read itself failed" (throws). Directly resolves #1098.
• saveIspSettingsAndCheck removal is safe: confirmed no external callers in the codebase; dead code cleanly deleted.
• All 5 ISP/troubleshooter views updated: pnp_isp_settings_view, pnp_no_internet_view, pnp_pppoe_view, pnp_static_ip_view, pnp_entry_view all handle AdminReadFailure -> context.go(RoutePath.pnp) — implicit retry is consistent.
• Test coverage is solid: three new regression tests covering device-info read failure, WAN read failure (not collapsed into NoInternet), and ISP-save-success-but-check-read-failure. All properly structured and mocked.
• Architecture compliance: ServiceError import from core/errors/ follows three-layer dependency rule. uspMutationLockProvider.withLock() correctly wraps the mutation in saveIspWithProgress. No autoDispose violations detected.
• UI uses l10n fixed string: loc(context).unableToGatherDeviceInfo instead of surfacing raw error details.

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