fix(local-network): cross-service SET, post-save IP redirect, pool prefix sync (#1039)

[PeterJhongLinksys]

What this branch does

Fixes local network settings save (issue #1039) across three problems found while investigating the firmware error 7005:

1. Cross-service SET (firmware error 7005)

LAN settings span multiple USP Services — Device.IP, Device.DHCPv4, Device.DeviceInfo — which the firmware cannot apply in one atomic SET (allow_partial=false across more than one Service → 7005). The save now passes allowPartial: true so each Service applies independently.

2. Post-save redirect on IP change

Changing the router IP makes the old address unreachable and the SET response never returns. The IP-changing SET is now treated as terminal: a timeout / transport error is treated as success, while a real firmware fault still surfaces. After a successful IP change the app redirects the browser to https://<hostName>.local and intentionally drops SSE to suppress the recovery dialog. This is triggered only on an IP address change — a mask-only or DHCP-only change is awaited normally with a success message.

3. Address pool prefix sync

The pool prefix only re-synced when the IP changed, not the subnet mask. Tightening the mask (e.g. /16 → /24) widened the locked prefix without updating the pool, leaving pool octets both out-of-subnet (validation error) and locked read-only — a dead end. Sync now runs on either change.

l10n

Adds lanIpRedirect* strings across all 26 locales.

Tests

• Service: terminal-SET classification (timeout/transport = success, real fault rethrows), plus the existing cross-service save cases.
• Notifier: redirect + SSE-disconnect branch vs. normal re-fetch branch; pool prefix auto-sync (real service).
• Dialog: shows the .local URL, button navigates, no dismiss action.

Known limitation (follow-up: #1116)

Changing the LAN IP via USP does not trigger client re-DHCP (no link down/up / port bounce) the way bridge mode does, so a client stays on the old subnet until its DHCP lease expires — the redirect only works once the client picks up a new lease. Tracked as a firmware-side fix in #1116.

🤖 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 · c4b6adb..746df56 (full)

Verdict: ✅ APPROVE — Logic is sound; timeout-as-success is correctly guarded for the IP-change case in the catch block, but the on TimeoutException clause itself lacks the guard. Three warnings worth addressing before ship.

---

	Conf.	Where	Issue (one-liner)
⚠️	🟢 High	usp_local_network_service.dart:85	[both reviewers] on TimeoutException not guarded by ipAddressChanged — throttler timeout on non-IP-change saves silently treated as success
⚠️	🟡 Med	usp_local_network_view.dart:439	IP-change with empty hostName falls through to success snackbar, no redirect shown
⚠️	🟡 Med	lan_ip_redirect_dialog.dart:1-40	Near-verbatim duplicate of bridge_redirect_dialog.dart (DRY violation)
⚠️	🟡 Med	usp_local_network_service.dart:33	_ipChangeSetTimeout = Duration(seconds: 4) duplicated verbatim in usp_internet_settings_service.dart:43
⚠️	🟡 Med	notifier_test.dart:307-370	Pool-prefix regression test instantiates real UspLocalNetworkService inside a Provider-layer test (crosses layer boundary)
💡	🟢 High	usp_local_network_service.dart:101	Transport-error guard uses is-type enumeration; future ServiceError subtypes won't auto-include
💡	🟢 High	assign_ip.dart:19	dart.library.html conditional export; package:web uses dart.library.js_interop — should migrate
💡	🟢 High	lib/l10n/	✅ All 26 ARB locale files consistently include the 3 new keys
💡	🟡 Med	usp_local_network_view.dart:406-449	_onSave redirect-vs-snackbar branch logic has no Widget test coverage
💡	⚪ Low	usp_local_network_notifier.dart:107-138	save() override doesn't call super.save() — intentional but undocumented deviation from mixin template-method contract

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

1. 🟢 [both reviewers] on TimeoutException not guarded by ipAddressChanged — usp_local_network_service.dart:85

// usp_local_network_service.dart:80-91
final result = ipAddressChanged
    ? await updateFuture.timeout(_ipChangeSetTimeout)
    : await updateFuture;   // ← if throttler times out here, TimeoutException propagates

_handleSetResult(result);
} on TimeoutException {
  // Comment says "only reachable on IP change" — incorrect
  logger.i('[USP][Network][LAN]: IP-change SET timed out after '
      '${_ipChangeSetTimeout.inSeconds}s — treating as success ...');

LanNetworkInfo.update() calls _resolveInstance(client) which calls client.get() — and get() routes through BridgeRequestThrottler. If the throttler's 15-second timeout fires during _resolveInstance on a non-IP-change save (e.g. DHCP pool or hostname only), a TimeoutException propagates out of updateFuture and is silently swallowed as "IP-change success", while the settings were never applied. The UI shows success; the router ignores the change.

Trigger: Any non-IP-change save when the router is busy and the bridge throttler times out (15s).

Fix:

} on TimeoutException {
  if (!ipAddressChanged) rethrow; // throttler timeout on non-IP-change = genuine failure
  logger.i('[USP][Network][LAN]: IP-change SET timed out — treating as success');
}

---

2. 🟡 IP-change + empty hostName falls to success snackbar — usp_local_network_view.dart:439

if (ipChanged && hostName.isNotEmpty) {
  await showLanIpRedirectDialog(context, hostName: hostName);
} else {
  showSuccessSnackBar(context, loc(context).localNetworkSettingsSaved);
  // ↑ this branch fires when ipChanged==true && hostName=="" 
}

If the user changes the IP while the hostName field is empty (corner-case: validation should reject, but save() doesn't enforce validation), the IP is applied, the old address becomes unreachable, but the user sees a success snackbar instead of a redirect dialog.

Fix: Show redirect dialog whenever ipChanged, falling back to new IP address string if hostName is empty; or assert-guard that save() cannot be called with validation errors.

---

3. 🟡 lan_ip_redirect_dialog.dart is near-verbatim copy of bridge_redirect_dialog.dart

Diff between the two files shows 19/40 lines differ, and all differences are identifier strings (function name + 4 l10n keys). The entire structure, dismissible: false, AppButton.primary, assignWebLocation injection, URL construction — is identical. Future changes (e.g. adding a countdown timer, tweaking dismissible logic) must be applied twice.

Fix: Extract a shared showPostSaveRedirectDialog(context, {title, message, buttonLabel, hostName, navigate}) helper in lib/components/shortcuts/dialogs.dart or a new shared helper file.

---

4. 🟡 _ipChangeSetTimeout magic constant duplicated — usp_local_network_service.dart:33

// usp_local_network_service.dart:33
static const _ipChangeSetTimeout = Duration(seconds: 4);

// usp_internet_settings_service.dart:43
static const _bridgeSetTimeout = Duration(seconds: 4);

Both represent the same physical concept: "maximum wait before firmware drops the connection after applying a new IP". Two independent static const values — if firmware behavior changes, both must be updated.

Fix: Extract const kTerminalSetTimeout = Duration(seconds: 4) to lib/core/usp/usp_constants.dart and import in both services.

---

5. 🟡 Provider-layer test instantiates real UspLocalNetworkService — notifier_test.dart:307-370

uspLocalNetworkServiceProvider
    .overrideWithValue(UspLocalNetworkService(MockUspClient())), // real Service in Provider test

lockedOctetCount and syncPrefix are currently pure functions (don't call _usp), so MockUspClient() is never invoked. But placing a real Service in a Provider-layer test crosses the architectural boundary: if those methods ever start using _usp, this test begins making unintentional network calls against an unstubbed mock.

Fix: Move this regression test to usp_local_network_service_test.dart where it tests service logic directly; or add a comment marking it as a deliberate cross-layer integration test.

✅ What looks good

• Terminal-SET pattern is correct: The catch (e) block properly guards transport errors with ipAddressChanged && (mapped is NetworkError || mapped is ConnectivityError) — this correctly distinguishes real firmware faults (ServiceError → rethrow) from expected disconnect-on-IP-change (return).
• _handleSetResult correctly handles UspPartialSuccess: UspPartialSuccess throws UspPartialFailureError (a ServiceError) which is re-thrown by the catch block — no silent partial failures.
• allowPartial: true is safe here: The _handleSetResult switch throws on UspPartialSuccess, so partial failures surface as errors.
• SSE disconnect uses intentional: true: sseManagerProvider.disconnect(intentional: true) prevents auto-reconnect during IP-change redirect window.
• Pool prefix sync triggers on both IP and mask change: The ipChanged || maskChanged guard correctly captures the broader case (PR's stated intent).
• Pre-save snapshot of ipChanged/hostName/networkChanged: Captured before save() correctly, so markAsSaved() collapsing the diff doesn't affect redirect logic.
• L10n is complete: All 26 locale ARB files contain lanIpRedirectTitle, lanIpRedirectMessage, lanIpRedirectButton; metadata only in app_en.arb (correct per ARB spec).
• Test coverage: 319 lines of new tests covering timeout-as-success, transport-error-as-success, pool-prefix sync regression, SSE disconnect, and dialog widget rendering.
• Architecture hard rules all pass: L1 not autoDispose ✅, L2 uses ref.read not ref.watch ✅, mutation wrapped in uspMutationLockProvider.withLock() ✅, codegen only imported by Service layer ✅, View uses ui_kit_library components ✅.

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