feat(l10n): centralize error message localization for USP features

[HankYuLinksys]

Summary

All USP requests (Get/Set/Add/Operate) across feature pages — except firmware_update (deferred) and SSE subscriptions (separate path) — now flow through a unified error handling pipeline.

When an error occurs, all error sources are aggregated and converted to the sealed ServiceError class at the Service layer. These ServiceError objects are then passed to the View layer for display. Most importantly, all ServiceError subtypes now have corresponding localizations (except UnexpectedError, which displays the raw detail string as a final fallback — this may be English firmware technical messages).

Error display paths

Path	Trigger	Storage	Display
Path 1 (fetch)	Initial data load fails	state.error	ServiceErrorView (empty state widget)
Path 2 (save)	Mutation fails	Provider rethrows	ref.listen + localizeServiceError → snackbar

Key changes

• Add ServiceError.code and ServiceError.detail for diagnostic context
• Add TimeoutError subtype for timeout handling
• Remove unused OTP/admin-password ServiceError subtypes (6 types deleted)
• Add localizeServiceError() central mapper with exhaustive switch on sealed class
• Add ServiceErrorView shared widget replacing 18 per-feature _buildError() methods
• Add 12 ARB keys for error messages
• Change all feature state models: String? errorMessage → ServiceError? error
• Update providers to pass through ServiceError objects (not stringify with '$e')
• Update views to use shared localization components

Coverage

Scope	Status
USP feature pages	✅ Covered
firmware_update	⏸️ Deferred (complex flow)
SSE subscription errors	⏸️ Out of scope (separate path)
StorageError / SerialNumberMismatchError	✅ Mapped but won't reach UI (caught at session/auth layer)

Notes

• UnexpectedError.detail is the final fallback — it surfaces raw technical strings when no typed error matches
• Batch failures (UspPartialFailureError / UspCompleteFailureError) show the first entry's concrete error message, not "N items failed"

Test plan

• flutter analyze passes
• ./run_tests.sh passes (2854 tests)
• Manual test: trigger fetch error → see localized message in ServiceErrorView
• Manual test: trigger save error → see localized snackbar message

Part of #919

🤖 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 · 65887b9..a11a11f (full)

Verdict: 💬 Comment — 1 Critical found, not auto-approving. Solid refactor overall; one user-facing regression blocks a clean approve.

	Where	Issue (one-liner)
🔴	auth_provider.dart:103 + usp_auth_coordinator.dart:165	Account-locked message is swallowed → users see a generic error when locked out
⚠️	service_error_localizations.dart:78 + usp_error.dart:231	[both reviewers] Fault-code table duplicated in two places; already drifting (9999)
⚠️	service_error_localizations.dart:46	[both reviewers] Raw firmware/technical strings can leak to the UI via UnexpectedError.detail
⚠️	service_error.dart:1	ServiceError reverse-imports USP types, breaking the "source-agnostic" contract
⚠️	service_error_localizations.dart, service_error_view.dart	Two new logic-bearing files have zero tests
💡	speed_test_view.dart:49,398	Inconsistent adoption — still hardcodes error UI/titles instead of ServiceErrorView
💡	~12 status models	Repeated error + clearError boilerplate (a base mixin could remove it)

Items marked [both reviewers] were independently flagged by two agents → higher confidence.

🔴 Critical — account-locked translation chain severed

The coordinator now throws UnexpectedError(detail: 'Account locked'), and _mapToViewError dropped its old branch:

-    if (error is AdminAccountLockedError) {
-      return UnexpectedError(message: errorAdminAccountLocked, originalError: error);
-    }

So it falls into the generic branch → detail: errorUnexpected. At login_local_view.dart:142, errorCode becomes '_ErrorUnexpected' → errorCodeHelper maps to unknownHandle. The correct handler errorAdminAccountLocked => localLoginTooManyAttemptsTitle (error_code_helper.dart:45) is now unreachable: the coordinator passes the literal 'Account locked' ≠ the constant errorAdminAccountLocked, and _mapToViewError would overwrite it anyway. Net effect: a locked-out user sees "Something went wrong".

Fix: restore the account-locked branch in _mapToViewError, or have the coordinator throw with detail: errorAdminAccountLocked so the existing localLoginTooManyAttemptsTitle handler fires.

⚠️ Warning — details (4 items)

Duplicated fault-code table — _localizeFaultCode (service_error_localizations.dart:78-86) duplicates the switch in _mapProtocolError (usp_error.dart:231-243); fetch path uses one, batch-write the other, kept in sync only by a comment. Already drifted: 9999→errorNetwork exists only in the localizer. Fix: extract one classifyFaultCode(int) -> ErrorKind called by both; localization only maps ErrorKind → string. A contract test asserting the tables agree would lock it down.

Raw strings leak to UI — UnexpectedError(:final detail) => detail ?? l.errorUnexpected (:46). On the fetch path usp_error.dart:244 builds UnexpectedError(detail: e.message) where e.message is raw router/WASM text, so a failed fetch renders "Protocol error … (code: 7099)". Contradicts the file's own comment ("we deliberately do NOT surface the raw firmware errorMessage"). Also detail is now overloaded (technical text / l10n-key / human message). Fix: fall back to l.errorUnexpected; move auth to typed subtypes instead of stuffing identifiers into detail.

Contract layer coupling — service_error.dart:1-2 imports core/usp/models/... for UspErrorDetail, yet claims (L11-13) to be the source-agnostic contract. Fix: define a source-agnostic FailureDetail in core/errors, convert UspErrorDetail into it at the service layer.

Zero tests on new logic — service_error_localizations.dart (88 lines: batch first-pick, fault-code branches, sealed switch, fallback) and service_error_view.dart (error/null render) have no test references (confirmed via grep). This is exactly the logic that should guard the "two tables in sync" invariant. Fix: add type→key mapping tests + _localizeBatch / _localizeFaultCode branch tests.

✅ What looks good

• Typed-contract migration is clean and exhaustive; sealed switches cover all result categories; batch failures preserve UspErrorDetail.
• Three-layer dependency direction holds; no codegen-model leakage into Provider/View; ServiceErrorView uses ui_kit_library.
• Existing tests updated consistently (errorMessage contains → isA<XxxError>(); message: → detail:; mocks throw typed errors).
• Null/empty + async-race paths checked: empty-failures guard, safe fromMap defaults, correct TimeoutException/ServiceError split in diagnostics.

---

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

[AustinChangLinksys]

🤖 Automated Review — Round 2 · a11a11f..c40e92f (incremental)

Verdict: ✅ APPROVE — Core account-locked l10n fix is clean and well-tested; approving with warnings to clarify before merge (VLAN SET no-op risk + pppoeServiceName 9001 re-enable).

	Where	Issue (one-liner)
⚠	lib/page/internet_settings/services/usp_internet_settings_service.dart:163	[both reviewers] Enabling VLAN when no existing instance is silently a no-op — SET-only path dropped the old Add fallback.
⚠	lib/page/internet_settings/services/usp_internet_settings_service.dart:302	[both reviewers] pppoeServiceName SET re-enabled (was disabled for bbfdm fault 9001); no test guards the new SET path and 9001 would surface as "unauthorized".
⚠	lib/page/internet_settings/services/usp_internet_settings_service.dart:21	Class doc still says "PPP/VLAN multi-instance lifecycle (Add/Delete)" — stale after VLAN moved to pure SET.
💡	lib/core/usp/providers/usp_auth_coordinator.dart:51	Account-locked regex `Account.*(locked
💡	lib/providers/auth/auth_provider.dart:332	An l10n-key string (errorAdminAccountLocked) is reused as a cross-layer control signal via ==; a dedicated sealed subtype (like InvalidCredentialsError) is the repo convention.
💡	lib/page/internet_settings/services/usp_internet_settings_service.dart:448	InternetSettingsFetchResult field doc still calls VLAN path "lifecycle management"; same drift as the class doc.

Items marked [both reviewers] were independently flagged by two agents → higher confidence.

⚠ Warning details (open for evidence + fixes)

W1 — VLAN enable becomes a silent no-op when no instance exists usp_internet_settings_service.dart:149-151
The new code calls _saveVlanSettings only when vlanInstancePath != null. The removed _handleVlanLifecycle previously did VlanTermination.add in the "enable + no instance" case. The risk is corroborated by the PR's own tests: test/page/internet_settings/services/usp_internet_settings_service_test.dart:832 (skips VLAN SET when no vlanInstancePath provided) asserts no VLANTermination key is sent even with edited.vlanEnabled=true, and test/page/instant_setup/services/pnp_service_test.dart:715-716 notes codegen skips instances where all fields are zero/false/empty. So a device whose VLANTermination.1 is disabled (VLANID=0) yields vlanInstancePath==null → toggling VLAN on saves nothing and reports no error.
Fix: confirm firmware always pre-provisions the instance; otherwise keep the Add fallback or raise a visible error when vlanInstancePath==null && edited.vlanEnabled.

W2 — pppoeServiceName SET re-enabled without test + lands as wrong localized error usp_internet_settings_service.dart:302-303
The field was deliberately disabled with the comment "bbfdm rejects SET (fault 9001)"; this round uncomments pppoeServiceName: _diff(...). _diff only emits on change, so it fires only when the user edits Service Name — but if firmware still rejects it, 9001 recurs, and per _localizeFaultCode 9001 => errorUnauthorized, the user sees an "unauthorized" message unrelated to the real cause. The diff adds VLAN and account-locked tests but no assertion that a pppoeServiceName change is included in the PPP SET payload.
Fix: add a test covering the new SET path and cite in the PR description the evidence that fault 9001 is resolved firmware-side.

W3 — Stale class doc comment usp_internet_settings_service.dart:21
Class doc still reads "Handles PPP/VLAN multi-instance lifecycle (Add/Delete)…" after the Add/Delete lifecycle (_handleVlanLifecycle, _handleDeleteResult) was removed. Update to reflect SET-based enable/disable on an existing instance (same drift in the InternetSettingsFetchResult field doc at :448).

✅ What looks good

• Account-locked error-code chain is self-consistent and tested. usp_auth_coordinator.dart:165 now throws UnexpectedError(detail: errorAdminAccountLocked) (import added at :100); auth_provider.dart:_mapToViewError passthrough (diff :332-334) preserves it ahead of the generic errorUnexpected overwrite, so the login view's errorCodeHelper resolves the lockout message and no raw string leaks. Covered by usp_auth_coordinator_test.dart and auth_notifier_test.dart.
• VLAN disable via SET (Enable=false) instead of Delete is an intentional design change; _saveVlanSettings only emits changed params via _diff, no spurious SET or race.
• Layering intact. usp_auth_coordinator.dart:100 imports a constants file (not a codegen model); codegen models (VlanTermination, UspResultParser) stay in the Service layer; the View (usp_ipv4_section.dart) touches no codegen.
• UI uses ui_kit_library. Re-enabled AppTextFormField / AppGap.md() come from package:ui_kit_library/ui_kit.dart (import at :10) — no hand-rolled widgets.
• No dead/orphan imports. parseDeleteResult / _handleDeleteResult removed together with their callers; VlanTermination and UspResultParser still used elsewhere.
• Localization is exhaustive — sealed switch is total, empty failures and unknown vendor codes fall back to errorUnexpected without leaking raw errorMessage; edge cases (first-failure selection, empty list, unknown code) are tested.
• Tooling/docs improvements: .claude/skills/review-pr-readiness/SKILL.md adds a mandatory "run affected tests" gate step; .claude/settings.json drops 2>/dev/null so format errors surface.

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