feat(pnp): add "Log into router" bypass on no-internet page (#1104)

[HankYuLinksys]

Summary

• Adds a "Log into router" button to the PnP no-internet page, giving users an exit when the router is online but has no WAN (a node paired behind an upstream router, or a factory-default box with no cable). Previously they were trapped with only Restart modem / Enter ISP / Try again. Addresses [2.5.0] No option to bypass the No Internet detection page (Router w/o WAN or Router setup using Pair button) #1104 (Problem A).
• Ports the dev-1.3.0 escape hatch, with two deliberate differences from 1.3.0.

Decision 1 — Bypass acknowledges PnP completion

Tapping "Log into router" calls pnpStatusService.acknowledge() (TR-181 SetUserAcknowledgedAutoConfig) before navigating.

Why: the USP router redirect only re-checks needsPnp when it passes through / or /localLoginPassword (router_provider._prepare). Without acknowledging, a full-page reload back to / would bounce the user right back into PnP. Acknowledging mirrors 1.3.0, where a configured router's save already sends SetUserAcknowledgedAutoConfig.

Trade-off: the user bypasses without configuring anything, yet we mark the router "PnP done" — slightly different from 1.3.0 (which acknowledges only after a WiFi step's save). Accepted because the USP flow has no unconfigured/configured split (checkFactoryDefault always returns false), and keeping the user stably in the dashboard matters more.

Decision 2 — Bypass goes straight to the dashboard (no mini-wizard)

Unlike 1.3.0 (which routed into a trimmed-down forceLogin wizard), the USP bypass navigates directly to the dashboard.

Why: the USP wizard is phase-driven and has no forceLogin branch; grafting one on would fight the architecture and carry a large surface (new wizard path, save, post-WiFi-change reconnect). The core complaint in #1104 is "router is online but I can't reach the UI" — going straight to the dashboard addresses that directly.

Trade-off: a first-time-setup user with no WAN lands on the dashboard without being guided to set WiFi (stays at factory defaults; changeable later in settings). A future "show the wizard only for a true factory default" needs checkFactoryDefault fixed first (tied to #1104 Problem B/C).

Known limitation

acknowledge is fire-and-forget (best-effort): a silent TR-181 write failure would leave the router un-acknowledged, so a later reload could re-offer PnP once. This is benign, needs no WAN (normally succeeds while offline), and is the same acknowledge the normal wizard path uses. bypassToDashboard() deliberately swallows any error and still navigates — an escape hatch must never trap the user.

Testing

• Manual: on a real device with the WAN cable unplugged, "Log into router" successfully enters the dashboard.
• Unit tests: acknowledge + saveSelectedNetwork on SN-present; skip on SN-missing; no-throw on acknowledge failure (16 tests pass).
• Adds logIntoRouter l10n key (26 locales, values from dev-1.3.0).

[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..a4a5e32 (full)

Verdict: ✅ APPROVE — 0 Critical; 5 Warnings flagged (4 worth addressing before merge). Logic is sound, auth guard is correct, notifier-layer test coverage is solid.

	Conf.	Where	Issue (one-liner)
⚠️	High	pnp_no_internet_view.dart:102	[both reviewers] Cross-button guard missing — _retrying and _bypassing are independent; both buttons can fire concurrently causing a double-navigation race
⚠️	High	pnp_notifier.dart:425-441	Duplicate acknowledge+save sequence from saveChanges() (line 266-274) with inconsistent error-handling semantics — extract to _acknowledgeAndSaveSession()
⚠️	Med	pnp_notifier.dart:433	acknowledge() is a USP Operate mutation but is not wrapped in uspMutationLockProvider.withLock() — violates Provider mutation rule
⚠️	Med	pnp_notifier.dart:427-430	SN-null early-return is silent to caller — navigation still proceeds without persisting pCurrentSN or acknowledging; PnP re-offered on next / reload (by design per doc, but no view-layer test covers this path)
⚠️	Med	pnp_notifier.dart:425-441	No terminal phase transition after bypass — state stays NoInternet; _onLogIntoRouter calls context.go() directly, bypassing the view's ref.listen routing pattern
💡	High	pnp_no_internet_view.dart:103	Trailing ) indented 2 extra spaces vs. sibling AppButton.text block — likely triggers Dart formatter warning
💡	High	test/page/instant_setup/	No View-layer widget tests for _bypassing loading state, button disable (onTap: null), or navigation to uspDashboard
💡	Low	pnp_notifier_test.dart:27	SpySessionNotifier diamond-inherits Notifier<SessionState> + SessionNotifier — safe if always used via overrideWith(() => SpySessionNotifier()), risky if directly instantiated (please double-check)
💡	Low	pnp_notifier.dart:425	Success path in bypassToDashboard() has no info-level log; other notifier methods log on success for diagnostics

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] — Cross-button mutual exclusion missing
pnp_no_internet_view.dart:102 (new onTap) and existing onTap: _retrying ? null : _onRetry

// Log into router button (new, ~line 102):
onTap: _bypassing ? null : _onLogIntoRouter,

// Try again button (existing):
onTap: _retrying ? null : _onRetry,

Each button only guards itself. When _retrying = true, the "Log into router" button is still tappable and vice versa. Scenario: user taps "Try again" -> retryInternetCheck() starts -> user taps "Log into router" -> bypassToDashboard() starts concurrently -> context.go(uspDashboard) fires -> retryInternetCheck() completes, internet may recover -> ref.listen triggers context.go(pnp) -> two context.go calls race, final destination is undefined.

Fix: Use a combined _busy = _retrying || _bypassing guard on both onTap fields, or add cross-checks in each handler (if (_retrying) return; / if (_bypassing) return;).

---

W-2 [single] — SN-null early-return: navigation proceeds unguarded
pnp_notifier.dart:427-430

if (sn == null || sn.isEmpty) {
  logger.w('[PnP] bypassToDashboard: no serial number, skipping acknowledge');
  return;  // method completes normally — no throw
}

Caller _onLogIntoRouter cannot distinguish this case from a successful bypass and proceeds to context.go(RoutePath.uspDashboard). The user enters the dashboard without pCurrentSN persisted and without UserAcknowledgedAutoConfig set. On a subsequent full-page reload routed through /, router_provider._prepare re-runs the PnP check and bounces the user back. The doc comment acknowledges the acknowledge-fail case but not the SN-null navigation case.

Fix (low-friction): Add a widget test that exercises the SN-null path from the view layer and asserts that context.go is still called (confirming the design is intentional). Medium friction: consider returning a bool/enum from bypassToDashboard() so the view can decide whether to navigate.

---

W-3 [single] — Duplicate acknowledge+save logic with divergent error semantics
pnp_notifier.dart:425-441 (new) vs. pnp_notifier.dart:266-274 (existing saveChanges)

// saveChanges() — no local catch, errors propagate upward:
await ref.read(pnpStatusServiceProvider).acknowledge(state.serialNumber!);
await ref.read(sessionProvider.notifier).saveSelectedNetwork(state.serialNumber!, '');

// bypassToDashboard() — swallows all errors silently:
try {
  await ref.read(pnpStatusServiceProvider).acknowledge(sn);
  await ref.read(sessionProvider.notifier).saveSelectedNetwork(sn, '');
} catch (e) { logger.w(...); }

The two copies have different error-propagation semantics; future changes to one will likely miss the other.

Fix: Extract _acknowledgeAndSaveSession(String sn, {bool swallowErrors = false}) and have both callers use it. Explicitly document the fire-and-forget vs. propagating distinction.

---

W-4 [single] — withLock() missing on USP Operate
pnp_notifier.dart:433

await ref.read(pnpStatusServiceProvider).acknowledge(sn);
// Tr181PnpStatusService.acknowledge -> SetupOperations.setUserAcknowledgedAutoConfig(_usp)
// This is a USP Operate (write) — architecture rule: all Provider mutations must
// wrap uspMutationLockProvider.withLock()

If another Provider is mid-mutation when bypass is triggered, concurrent USP writes could occur. Note: saveChanges() also places acknowledge() outside its withLock block (line 267) — this may be an accepted existing pattern. Please confirm whether Tr181 Operate calls are considered lock-exempt.

---

W-5 [single] — No terminal phase transition after bypass
pnp_notifier.dart:425-441; pnp_no_internet_view.dart:36-37

// bypassToDashboard() does not emit any new phase — state.phase remains NoInternet.
// _onLogIntoRouter navigates directly (imperative), not via ref.listen:
await ref.read(pnpProvider.notifier).bypassToDashboard();
if (mounted) context.go(RoutePath.uspDashboard);

All other navigation from this view flows through ref.listen(pnpProvider, ...). The bypass is the sole exception. If pnpProvider is read elsewhere after bypass, it still reports NoInternet phase, which could confuse downstream logic. Emitting a PnpBypassed (or similar) phase would make the state machine self-documenting and consistent.

✅ What looks good

• Auth guard intact: RoutePath.uspDashboard starts with /usp; router_provider.dart checks isLoggedIn before allowing any /usp* route. The bypass cannot be used by an unauthenticated user.
• No hardcoded secrets in any changed file.
• Escape-hatch error contract is correct: double try/catch (inner in acknowledge() service + outer in bypassToDashboard()) ensures navigation is never blocked by a USP write failure. Design intent is clearly documented.
• Notifier-layer test coverage solid: three bypassToDashboard test cases (happy path, no-SN, acknowledge-throws) with SpySessionNotifier correctly mounted via overrideWith — this is the right pattern.
• mounted guards present in all async widget callbacks — no setState-after-dispose risk in the happy path.
• L10n complete: all 26 locale files updated with the logIntoRouter key.

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

[HankYuLinksys]

Thanks for the review. Point-by-point response, using your W-/S- labels (pushed in d86f3b5):

W1 — Cross-button guard missing (pnp_no_internet_view.dart:102) — ✅ Fixed. Added a combined _busy = _retrying || _bypassing; both buttons' onTap guard on it and both handlers early-return if (_busy) return;. A retry and a bypass can no longer run concurrently.

W2 — SN-null early-return is silent; no test covers navigation on that path — ✅ Test added. The SN-null branch is by design (navigation should proceed regardless). The notifier-level "skips acknowledge when serial number is missing" test already covers the no-throw behavior; I added a view-level test asserting the view still navigates to the dashboard when the notifier does nothing on bypass.

W3 — Duplicate acknowledge+save with inconsistent error semantics (pnp_notifier.dart) — ✅ Documented, not extracted. The divergence is intentional: saveChanges() lets errors propagate (a failed WiFi save should keep the user on the form to retry), while bypassToDashboard() swallows them (an escape hatch must never block navigation). The duplicated part is only two await lines; extracting a helper with a swallowErrors flag would obscure that deliberate difference rather than clarify it. I added cross-referencing comments at both sites so future edits keep them in sync.

W4 — acknowledge() not wrapped in withLock() (pnp_notifier.dart:433) — ⏭️ Confirmed lock-exempt, no change. As you note, saveChanges() also calls acknowledge() outside its withLock block — this is the existing pattern. acknowledge() is a USP Operate (SetUserAcknowledgedAutoConfig), not a Set/GET on the working-copy tree that the mutation lock serializes, so it's treated as lock-exempt. Keeping consistent with saveChanges().

W5 — No terminal phase transition after bypass — ⏭️ No change. _onLogIntoRouter navigates to /uspDashboard, which leaves the PnP route subtree; pnpProvider is autoDispose and gets torn down, so there's no lingering NoInternet state for downstream logic to misread. Adding a PnpBypassed phase would only be cosmetic state-machine tidiness at the cost of a new sealed-class variant + switch handling, so I've skipped it.

S1 — Trailing ) indentation / formatter warning (pnp_no_internet_view.dart:103) — ⏭️ Not reproducible. dart format --set-exit-if-changed reports 0 changes on all touched files; the two buttons are structurally symmetric. No change needed.

S2 — No View-layer widget tests — ✅ Added. New pnp_no_internet_view_test.dart (tagged ui) covers: both buttons render, tapping "Log into router" calls bypass and navigates to /uspDashboard, and the bypass still navigates when acknowledge is skipped.

S3 — SpySessionNotifier diamond inheritance — ⏭️ No change. It is only ever used via sessionProvider.overrideWith(() => SpySessionNotifier()), never instantiated directly, so the noted risk doesn't apply.

S4 — No success-level log in bypassToDashboard() — ✅ Added. logger.i('[PnP] bypassToDashboard: acknowledged, entering dashboard') on the success path.