feat(dashboard): show spinner on card toggle during mutation (#1055) by HankYuLinksys · Pull Request #1126 · linksys/PrivacyGUI

HankYuLinksys · 2026-07-14T01:03:33Z

Summary

Add loading spinner to dashboard card toggles during mutation, replacing the previous "disabled with no feedback" behavior
Fix Professional preset missing the WiFi Networks card

Changes

Toggle Spinner

Added isLoading parameter to ToggleRow and NetworkRow components. When true, displays a CircularProgressIndicator in place of the switch, providing clear visual feedback that an operation is in progress.

Professional Preset Fix

Added missing wifi_networks card to the Professional preset. The preset description says "all cards enabled" but was missing this card (17 → 18 cards).

Affected Toggles

Dashboard Card	Toggle Location
WiFi Networks	Each SSID network
DHCP Reservations	Each reservation entry
Port Forwarding	Each forwarding rule
Port Triggering	Each triggering rule

Test Plan

Toggle WiFi network on/off → spinner appears during mutation
Toggle DHCP reservation on/off → spinner appears during mutation
Toggle port forwarding rule on/off → spinner appears during mutation
Toggle port triggering rule on/off → spinner appears during mutation
Switch to Professional preset → WiFi Networks card is now visible

Notes

Existing loadable switch components in this project
- AppSwitchTriggerTile at lib/components/composed/app_switch_trigger_tile.dart — self-manages _isLoading, replaces AppSwitch with CircularProgressIndicator when loading.
  Currently unused.
- AppLoadableWidget.appSwitch at lib/components/composed/app_loadable_widget.dart — same as above but more generic, includes controller.
  Currently unused.
UI Kit status
- UI Kit (ui_kit_library) only provides the atomic AppSwitch component
- AppSwitch has no built-in loading parameter (pure StatelessWidget)
- UI Kit has no equivalent loadable-switch composite component

qodo-code-review · 2026-07-14T01:03:37Z

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 →

- Add isLoading param to ToggleRow and NetworkRow components - Display CircularProgressIndicator in place of AppSwitch when loading - Pass isLoading to WiFi Networks, DHCP Reservations, Port Forwarding cards - Add missing wifi_networks card to Professional preset (17→18 cards) - Update test expectation for Professional preset card count

AustinChangLinksys

🤖 Automated Review — Round 1 · 69eb8f1..9874e7c (full)

Verdict: 💬 COMMENT — Critical finding blocks auto-approve; manual review required.

	Conf.	Where	Issue (one-liner)
🔴	🟢High	`row_blocks.dart:173,293`	[both reviewers] `CircularProgressIndicator` used directly instead of `AppLoader` from `ui_kit_library` — violates UI architecture rule
⚠️	🟢High	`row_blocks.dart:175,295`	[both reviewers] `semanticsLabel: 'Loading'` hardcoded English string — bypasses l10n, breaks accessibility on non-English locales
⚠️	🟢High	`usp_port_forwarding_card.dart:86,106`	[single, Reviewer A] Missing `instancePath == null` guard before bang-unwrap — crashes if instancePath is null (DHCP card already guards correctly)
⚠️	🟢High	`usp_wifi_networks_card.dart:97`	[both reviewers] Single `'wifi_network'` loading key causes ALL SSID rows to spin simultaneously; `ref.watch` called N times inside loop
⚠️	🟢High	`row_blocks.dart:169–301`	[single, Reviewer B] Duplicate spinner construction across `ToggleRow` + `NetworkRow` — same pattern, only dimension constants differ
⚠️	🟡Med	`usp_dashboard_preset_test.dart:61`	[single, Reviewer B] Stale test description says "17 cards" after count updated to 18
⚠️	🟡Med	golden test files	[single, Reviewer B] No golden state for `isLoading=true` visual path — spinner rendering has zero visual regression coverage
💡	🟡Med	`usp_dashboard_preset.dart` professional layout	[single, Reviewer B] Implicit y=27 gap in left column (wifi_status h=4 ends row 26; wifi_networks starts row 28)
💡	⚪Low	`usp_wifi_networks_card.dart:97`	[single, Reviewer B] `ref.watch` inside per-row helper creates N subscriptions — hoist to `build()` per DHCP/PortForwarding pattern
💡	🟢High	`usp_wifi_networks_card.dart:141`	[single, Reviewer A] Loading key `'wifi_network'` (snake_case) inconsistent with peers `'portForwarding'` (camelCase) — document or align

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.

🔴 Critical Details

C-1 — `CircularProgressIndicator` instead of `AppLoader` from `ui_kit_library` — violates UI architecture rule

Location: lib/page/_shared/components/layout_blocks/row_blocks.dart:173 (ToggleRow) and :293 (NetworkRow)

Code (head version — ToggleRow):

// row_blocks.dart:169–178
child: isLoading
    ? SizedBox(
        width: 26,
        height: 26,
        child: CircularProgressIndicator(   // bare Flutter widget, NOT AppLoader
          strokeWidth: 2,
          semanticsLabel: 'Loading',
        ),
      )
    : AppSwitch(...)

Code (head version — NetworkRow):

// row_blocks.dart:289–299
child: SizedBox(
  width: 24,
  height: 24,
  child: CircularProgressIndicator(   // bare Flutter widget, NOT AppLoader
    strokeWidth: 2,
    semanticsLabel: 'Loading',
  ),
),

Why it's a bug: The project's architecture rule states UI MUST use ui_kit_library, not raw Flutter widgets. Codebase-wide grep confirms every other loading indicator uses AppLoader (verified: pnp_entry_view.dart:74, usp_topology_view.dart:48, firmware_update_card.dart:79, usp_local_network_view.dart:132, etc.). AppLoader applies theme-driven color tokens, WCAG 2.2.2 reduce-motion compliance, and visual language consistency across Glass/NB/Flat themes. Raw CircularProgressIndicator ignores all theme tokens.

Trigger condition: Any dashboard card toggle mutation in any non-default design theme.

Fix:

SizedBox.square(dimension: 24, child: AppLoader(strokeWidth: 2))

Replace both CircularProgressIndicator instances with AppLoader.

⚠️ Warning Details

W-1 — Hardcoded semanticsLabel: 'Loading' bypasses l10n — 🟢High [both reviewers]

lib/page/_shared/components/layout_blocks/row_blocks.dart:175, 295

semanticsLabel is read aloud by TalkBack/VoiceOver. Every other user-visible string uses loc(context).<key>. Hardcoded English breaks accessibility on non-English devices.

Fix: Use loc(context).loading (or add key), or add String? loadingSemanticLabel constructor parameter.

W-2 — Missing instancePath == null guard before bang-unwrap in port forwarding card — 🟢High [single, Reviewer A]

lib/page/port_forwarding/cards/usp_port_forwarding_card.dart:86, 106

// Port Forwarding row (line 86):
.immediateToggleForwarding(rule.instancePath!, value)  // bang, no null guard

// Port Triggering row (line 106):
.immediateToggleTriggering(trigger.instancePath!, value)  // bang, no null guard

PortForwardingRuleUIModel.instancePath is declared final String? instancePath (null for locally-created unsaved rules). The onChanged guard only checks isLoading, not instancePath == null. The DHCP card (the pattern this PR extends) already does the right thing:

// usp_dhcp_reservations_card.dart:76-77 (correct):
onChanged: isLoading || reservation.instancePath == null
    ? null
    : (value) => ...immediateToggle(reservation.instancePath!, value),

A null instancePath during a state race or failed add will crash with Null check operator used on a null value.

Fix: Add || rule.instancePath == null to each guard condition (mirroring DHCP pattern).

W-3 — Single loading key spins all SSID rows simultaneously — 🟢High [both reviewers]

lib/page/wifi_settings/cards/usp_wifi_networks_card.dart:97

// Called per-SSID in a loop:
final isLoading = ref.watch(uspMutationLoadingProvider) == 'wifi_network';
return NetworkRow(...isLoading: isLoading...);

When any one SSID is being mutated, all SSID rows show a spinner and block their toggles. A user with 4 SSIDs sees all 4 frozen when only 1 is mutating. Incorrect UI feedback.

Fix: Use per-SSID keys and hoist the ref.watch:

// In build():
final mutatingKey = ref.watch(uspMutationLoadingProvider);
// In _buildNetworkRow:
final isLoading = mutatingKey == 'wifi_network_${network.ssidName}';
// In performUspMutation:
loadingKey: 'wifi_network_${network.ssidName}',

W-4 — Duplicate spinner construction across ToggleRow + NetworkRow — 🟢High [single, Reviewer B]

lib/page/_shared/components/layout_blocks/row_blocks.dart:169–178, 285–301

Identical SizedBox > CircularProgressIndicator construction duplicated with only size constants varying. Next change to spinner appearance requires finding and updating two places.

Fix: Extract a private _SwitchSpinner({required double size}) widget, or replace both with AppLoader.

W-5 — Stale test description after count update — 🟡Med [single, Reviewer B]

test/page/dashboard/models/usp_dashboard_preset_test.dart:61

test('professional has 17 cards (all)', () {  // description not updated
  expect(UspDashboardPreset.professional.cardIds.length, 18);
});

Fix: Update string to 'professional has 18 cards (all)'.

W-6 — No golden test state for isLoading=true visual path — 🟡Med [single, Reviewer B]

Affected golden tests for DHCP reservations, port forwarding, and WiFi networks cards only cover 'with_data' and 'empty' states. The spinner-replacing-switch is the primary new visual change — zero visual regression coverage.

Fix: Add a 'loading' golden state to each affected card test using uspMutationLoadingProvider override.

✅ What Looks Good

Double-tap prevention correct: onChanged: null and isLoading: true driven by the same boolean — no window where spinner shows but toggle still fires.
UspMutationLock serializes at backend: Even if UI guard bypassed, USP mutations cannot race (30s timeout + force-release).
context.mounted checked after async gap: _confirmToggleNetwork correctly guards before performUspMutation.
performUspMutation resets loading key in finally: Spinner guaranteed to clear on exception.
Professional layout coordinates verified: No grid coordinate overlaps. wifi_networks correctly added to both cardIds and _professionalLayout().
Test count updates consistent: Both test files updated 17 → 18.

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

AustinChangLinksys

🤖 Automated Review — Round 2 · 69eb8f1..9874e7c (full)

Verdict: 💬 COMMENT — Critical architecture violation (C-1: raw CircularProgressIndicator instead of AppLoader) blocks auto-approve; manual review required.

Re-review of the same diff. C-1 confirmed still present; W-5 resolved ✅; W-5(new)/W-6(new) are additions this round.

	Conf.	Where	Issue (one-liner)
🔴	🟢High	`row_blocks.dart:173,293`	[both reviewers] `CircularProgressIndicator` used directly instead of `AppLoader` from `ui_kit_library` — violates UI architecture rule
⚠️	🟢High	`row_blocks.dart:175,295`	[both reviewers] `semanticsLabel: 'Loading'` hardcoded English string — bypasses l10n, breaks accessibility on non-English locales
⚠️	🟢High	`usp_port_forwarding_card.dart:87,108`	[single, Reviewer A] Missing `instancePath == null` guard before bang-unwrap — crashes if instancePath is null (DHCP card already guards correctly)
⚠️	🟢High	`usp_wifi_networks_card.dart:97`	[both reviewers] Single `'wifi_network'` loading key causes ALL SSID rows to spin simultaneously
⚠️	🟢High	`row_blocks.dart:169–303`	[both reviewers] Duplicate spinner construction across `ToggleRow` + `NetworkRow` — same pattern, only dimension constants differ
⚠️	🟡Med	`row_blocks.dart:278–283`	[single, Reviewer B] NEW: Share button remains tappable during mutation — half-frozen affordance inconsistency
⚠️	🟡Med	golden test files	[single, Reviewer B] No golden/widget test for `isLoading=true` visual path — zero visual regression coverage
💡	🟢High	`usp_wifi_networks_card.dart:97`	[single, Reviewer B] NEW: `ref.watch` called per-row inside loop — hoist to `build()` per port-forwarding card pattern
✅	—	`usp_dashboard_preset_test.dart:60`	Test description `'17 cards'` → `'18 cards (all)'` — FIXED ✓

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.

🔴 Critical Details

C-1 — `CircularProgressIndicator` instead of `AppLoader` from `ui_kit_library` — violates UI architecture rule

Location: lib/page/_shared/components/layout_blocks/row_blocks.dart:173 (ToggleRow) and :293 (NetworkRow)

Code (head version — ToggleRow, lines 169–177):

child: isLoading
    ? SizedBox(
        width: 26,
        height: 26,
        child: CircularProgressIndicator(   // bare Flutter widget, NOT AppLoader
          strokeWidth: 2,
          semanticsLabel: 'Loading',
        ),
      )
    : AppSwitch(...)

Code (head version — NetworkRow, lines 285–299):

isLoading
    ? SizedBox(
        width: 52,
        height: 32,
        child: Center(
          child: SizedBox(
            width: 24,
            height: 24,
            child: CircularProgressIndicator(   // bare Flutter widget, NOT AppLoader
              strokeWidth: 2,
              semanticsLabel: 'Loading',
            ),
          ),
        ),
      )
    : AppSwitch(...)

Why it's a bug: Architecture rule — UI MUST use ui_kit_library, not raw Flutter widgets. Codebase-wide: every other loading indicator uses AppLoader (verified: pnp_entry_view.dart, usp_topology_view.dart, firmware_update_card.dart, usp_local_network_view.dart, etc.). AppLoader applies theme-driven color tokens, WCAG 2.2.2 reduce-motion compliance, and visual language consistency across Glass/NB/Flat themes. Raw CircularProgressIndicator ignores all theme tokens.

Trigger condition: Any dashboard card toggle mutation when a non-default design theme is active (Glass, NB, Flat variants).

Fix:

SizedBox.square(dimension: 24, child: AppLoader(strokeWidth: 2))

Replace both CircularProgressIndicator instances with AppLoader. This also addresses W-1 (if AppLoader handles semantics internally) and W-4 (deduplication with a shared helper).

⚠️ Warning Details

W-1 — Hardcoded semanticsLabel: 'Loading' bypasses l10n — 🟢High [both reviewers]

lib/page/_shared/components/layout_blocks/row_blocks.dart:175, 295

semanticsLabel is read aloud by TalkBack/VoiceOver. Every other user-visible string uses loc(context).<key>. Hardcoded English breaks accessibility on non-English devices.

Fix: Use loc(context).loading (or add key); switching to AppLoader (C-1 fix) may handle semantics internally.

W-2 — Missing instancePath == null guard before bang-unwrap in port forwarding card — 🟢High [single, Reviewer A]

lib/page/port_forwarding/cards/usp_port_forwarding_card.dart:87, 108

// Port Forwarding row (line 87):
.immediateToggleForwarding(rule.instancePath!, value)  // bang, no null guard

// Port Triggering row (line 108):
.immediateToggleTriggering(trigger.instancePath!, value)  // bang, no null guard

PortForwardingRuleUIModel.instancePath is final String? instancePath (null for locally-created unsaved rules). onChanged guard only checks isLoading, not instancePath == null. The DHCP card (the pattern this PR extends) correctly guards:

// usp_dhcp_reservations_card.dart:76-77 (correct):
onChanged: isLoading || reservation.instancePath == null ? null : ...

A null instancePath during a state race or failed add will crash with Null check operator used on a null value.

Fix: Add || rule.instancePath == null to each guard condition (mirroring DHCP pattern).

W-3 — Single loading key spins all SSID rows simultaneously — 🟢High [both reviewers]

lib/page/wifi_settings/cards/usp_wifi_networks_card.dart:97

// Called per-SSID in a loop:
final isLoading = ref.watch(uspMutationLoadingProvider) == 'wifi_network';
return NetworkRow(...isLoading: isLoading...);

When any one SSID is being mutated, all SSID rows show a spinner and block their toggles. A user with 4 SSIDs sees all 4 frozen when only 1 is mutating.

Fix: Use per-SSID keys and hoist ref.watch to build():

// In build():
final mutatingKey = ref.watch(uspMutationLoadingProvider);
// In _buildNetworkRow:
final isLoading = mutatingKey == 'wifi_network_${network.ssidName}';
// In performUspMutation:
loadingKey: 'wifi_network_${network.ssidName}',

W-4 — Duplicate spinner construction across ToggleRow + NetworkRow — 🟢High [both reviewers]

lib/page/_shared/components/layout_blocks/row_blocks.dart:169–177, 285–301

Identical SizedBox > CircularProgressIndicator construction duplicated with only size constants varying. NetworkRow adds unnecessary extra nesting (SizedBox(52×32) > Center > SizedBox(24×24)).

Fix: Extract _SwitchSpinner({required double size}), or replace both with AppLoader (addresses C-1 simultaneously).

W-5 (new) — Share button remains tappable during mutation — 🟡Med [single, Reviewer B]

lib/page/_shared/components/layout_blocks/row_blocks.dart:278–283

if (isEnabled && onShareTap != null) ...[
    _ShareButton(onTap: onShareTap!),   // still tappable when isLoading=true
    AppGap.sm(),
],

When isLoading=true, only the toggle is replaced by a spinner; the QR share button remains active — a half-frozen affordance that is inconsistent with the mutation state.

Fix: Guard the share button on !isLoading:

if (!isLoading && isEnabled && onShareTap != null) ...[
    _ShareButton(onTap: onShareTap!),
    AppGap.sm(),
],

W-6 — No golden/widget test for isLoading=true visual path — 🟡Med [single, Reviewer B]

No widget-level golden test covers ToggleRow(isLoading: true) or NetworkRow(isLoading: true). The spinner-replacing-switch is the primary new visual change — zero visual regression coverage.

Fix: Add a 'loading' golden state to each affected card test using uspMutationLoadingProvider override, or widget tests asserting find.byType(AppLoader) when isLoading=true.

✅ What Looks Good

Prior W-5 RESOLVED ✓: Test description '17 cards' → '18 cards (all)' correctly updated in usp_dashboard_preset_test.dart:60; layout controller test similarly updated. Well done.
Double-tap prevention correct: onChanged: null and isLoading: true driven by the same boolean — no window where spinner shows but toggle still fires.
UspMutationLock serializes at backend: Even if UI guard bypassed, USP mutations cannot race (30s timeout + force-release).
context.mounted checked after async gap: _confirmToggleNetwork correctly guards before performUspMutation.
performUspMutation resets loading key in finally: Spinner guaranteed to clear on exception.
DHCP card pattern correctly extended: usp_dhcp_reservations_card.dart correctly adds the null guard and isLoading parameter.
Professional layout coordinates verified: No grid coordinate overlaps. wifi_networks correctly added to both cardIds and _professionalLayout().

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

PeterJhongLinksys

Automated PR Review

Verdict: 🔴 Comment — unresolved critical issue(s); not approving

Findings

🔴 Critical — none new. The one unresolved critical (raw CircularProgressIndicator instead of AppLoader, row_blocks.dart:173,293) was already raised by the automated review and is still open — see "Already raised" below; it is what blocks approval.
🟡 Warning — none new (all warning-level concerns were already raised).
🟢 Nit — NetworkRow wraps its whole trailing area in Opacity(disabledAlpha) when isEnabled == false; while toggling a currently-disabled SSID on, isEnabled is still false during the mutation, so the spinner that replaces the switch renders dimmed. lib/page/_shared/components/layout_blocks/row_blocks.dart:222,285.

Already raised (skipped to avoid duplication)

🔴 Raw CircularProgressIndicator instead of AppLoader (row_blocks.dart:173,293) — AustinChangLinksys automated review (R1+R2), still open (head unchanged; violates constitution Art. XIV "UI Kit First").
⚠️ Hardcoded semanticsLabel: 'Loading' bypasses l10n (row_blocks.dart:175,295) — automated review, still open (note: a loading key already exists in every ARB locale, so the fix is trivial).
⚠️ Missing instancePath == null guard before bang-unwrap in port forwarding / triggering (usp_port_forwarding_card.dart:86,106) — automated review, still open; confirmed a real Null check operator crash path (model declares instancePath nullable; DHCP card guards it, this card does not).
⚠️ Single 'wifi_network' loading key makes ALL SSID rows spin/freeze simultaneously; ref.watch also called per-row inside the loop (usp_wifi_networks_card.dart:97) — automated review, still open.
⚠️ Duplicate spinner construction across ToggleRow + NetworkRow (row_blocks.dart:169-303) — automated review, still open.
⚠️ Share button remains tappable during mutation (row_blocks.dart:264-267) — automated review R2, still open.
⚠️ No golden/widget coverage for the isLoading = true visual path — automated review, still open.
💡 Implicit y=27 gap in Professional left column, and 'wifi_network' (snake_case) vs 'portForwarding' (camelCase) loading-key naming inconsistency — automated review, still open (cosmetic).
⚠️ Stale '17 cards' test description — automated review R1, FIXED (head now reads 'professional has 18 cards (all)' with expect(..., 18)).

Verified

Head commit 9874e7c is unchanged since both automated review rounds, so every open item above remains open against the current diff.
performUspMutation resets uspMutationLoadingProvider to null in its finally block (usp_mutation_helper.dart:36-37) — no stuck spinner on success or failure; the toggle returns to its real (unchanged) state on error.
Double-submit is guarded: onChanged is forced null whenever isLoading is true in all four cards (DHCP/PF/PT/WiFi), and the spinner + disabled state are driven by the same provider value, not a local bool.
No generated/ imports were introduced in any touched view/provider/model file.
DHCP card correctly guards instancePath == null (usp_dhcp_reservations_card.dart:76) — the pattern the PR should mirror in the port-forwarding card.
Professional preset is internally consistent: cardIds (18) and _professionalLayout() (18 items) match, both count tests updated to 18, and the grid coordinates have no overlaps (wifi_networks correctly registered via the standard-preset spec).

AustinChangLinksys

🤖 Automated Review — Round 1 · 69eb8f1..9874e7c (full)

Verdict: 💬 COMMENT — Two Critical issues block auto-approve: C-1 (raw CircularProgressIndicator instead of AppLoader from ui_kit_library — violates UI arch rule); C-2 (NEW: unguarded instancePath! bang in DHCP delete path). All prior Round 2 findings remain open; author has not responded.

	Conf.	Where	Issue (one-liner)
🔴	🟢High	`row_blocks.dart:173,293`	[both reviewers] `CircularProgressIndicator` used directly instead of `AppLoader` — violates UI architecture rule
🔴	🟢High	`usp_dhcp_reservations_card.dart:202`	[single, Reviewer A] `_confirmDeleteDhcp` calls `reservation.instancePath!` with no null guard on delete button — crash if instancePath is null
⚠️	🟢High	`usp_port_forwarding_card.dart:87,108`	[both reviewers; severity contested: A=Critical, B=Warning] Missing `instancePath==null` guard before `!` on port forwarding toggle
⚠️	🟢High	`row_blocks.dart:175,295`	[both reviewers] `semanticsLabel:'Loading'` hardcoded English — bypasses l10n, breaks accessibility
⚠️	🟢High	`usp_wifi_networks_card.dart:97`	[both reviewers] Single `'wifi_network'` key causes ALL SSID rows to spin simultaneously on any toggle
⚠️	🟢High	`row_blocks.dart:170-177,286-299`	[both reviewers] Duplicate spinner construction across `ToggleRow` + `NetworkRow`
⚠️	🟢High	`usp_port_forwarding_card.dart:78`	[single, Reviewer A] Spinner shown on `instancePath==null` rows — misleading loading UX
⚠️	🟡Med	`row_blocks.dart:281-284`	[single, Reviewer B] Share button remains tappable during mutation
⚠️	🟢High	test/	[both reviewers] No golden/widget test for `isLoading=true` visual path — zero visual regression coverage
💡	🟡Med	`usp_wifi_networks_card.dart:97`	[single, Reviewer B] `ref.watch` called N times in build loop — hoist to `build()`
💡	🟢High	`usp_dashboard_preset.dart:162`	[single, Reviewer B] Stale doc comment: `WiFiNetworks (6×6)` but code has `h: 4`

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.

🔴 Critical Details

C-1 — `CircularProgressIndicator` instead of `AppLoader` (UI architecture violation)

Location: lib/page/_shared/components/layout_blocks/row_blocks.dart:173 (ToggleRow) and :293 (NetworkRow)

Code (ToggleRow, lines 170-177):

child: isLoading
    ? SizedBox(
        width: 26,
        height: 26,
        child: CircularProgressIndicator(   // bare Flutter widget, NOT AppLoader
          strokeWidth: 2,
          semanticsLabel: 'Loading',
        ),
      )
    : AppSwitch(...)

Code (NetworkRow, lines 286-298):

isLoading
    ? SizedBox(
        width: 52,
        height: 32,
        child: Center(
          child: SizedBox(
            width: 24,
            height: 24,
            child: CircularProgressIndicator(   // bare Flutter widget, NOT AppLoader
              strokeWidth: 2,
              semanticsLabel: 'Loading',
            ),
          ),
        ),
      )
    : AppSwitch(...)

Why it's a bug: Architecture rule — UI MUST use ui_kit_library, not raw Flutter widgets. Every other loading indicator uses AppLoader (verified: pnp_entry_view.dart, usp_topology_view.dart, firmware_update_card.dart, usp_network_health_card.dart, usp_system_status_card.dart, usp_traffic_analysis_card.dart). AppLoader applies theme-driven color tokens, WCAG 2.2.2 reduce-motion compliance, and visual-language consistency across Glass/NB/Flat themes.

Trigger condition: Any dashboard card toggle mutation when a non-default design theme is active.

Fix:

SizedBox.square(dimension: 24, child: AppLoader(strokeWidth: 2))

Replace both CircularProgressIndicator instances with AppLoader. Also resolves W-2 (semantics) and W-4 (duplication).

C-2 — `_confirmDeleteDhcp` calls `instancePath!` with no null guard on delete button (NEW)

Location: lib/page/local_network/cards/usp_dhcp_reservations_card.dart:202

Evidence chain (A to C):

A — DhcpReservationUIModel.instancePath is nullable (dhcp_reservation_ui_model.dart:9):

// "[instancePath] is null for newly created (local-only) reservations that have not yet been saved."
final String? instancePath;

B — Delete button guards only isLoading, NOT instancePath == null (usp_dhcp_reservations_card.dart:88-92):

AppIconButton(
  icon: AppIcon.font(Icons.delete_outline, size: 18),
  onTap: isLoading            // only isLoading checked, no instancePath guard
      ? null
      : () => _confirmDeleteDhcp(context, ref, reservation),
),

C — _confirmDeleteDhcp bang-unwraps unconditionally (usp_dhcp_reservations_card.dart:196-203):

await performUspMutation(
  context, ref,
  loadingKey: 'dhcp',
  mutation: () => ref
      .read(uspDhcpReservationsProvider.notifier)
      .immediateDelete(reservation.instancePath!),  // NULL DEREFERENCE if null
  successMessage: loc(context).reservationDeleted,
);

Contrast: Toggle onChanged at line 77 correctly guards isLoading || reservation.instancePath == null. The delete button does not.

Trigger condition: A freshly added reservation (instancePath == null, not yet persisted) + user taps delete while isLoading == false → Null check operator used on a null value crash.

Fix:

onTap: isLoading || reservation.instancePath == null
    ? null
    : () => _confirmDeleteDhcp(context, ref, reservation),

⚠️ Warning Details

W-1 — Missing instancePath==null guard in port forwarding card — 🟢High [both reviewers; severity contested]

lib/page/port_forwarding/cards/usp_port_forwarding_card.dart:87, 108

// Forwarding row:
onChanged: isLoading   // no instancePath check
    ? null
    : (value) => performUspMutation(... immediateToggleForwarding(rule.instancePath!, value)),
// Triggering row:
onChanged: isLoading
    ? null
    : (value) => performUspMutation(... immediateToggleTriggering(trigger.instancePath!, value)),

PortForwardingRuleUIModel.instancePath and PortTriggeringRuleUIModel.instancePath are both String?. DHCP card correctly guards isLoading || reservation.instancePath == null — port forwarding must do the same.

Fix: Add || rule.instancePath == null and || trigger.instancePath == null to respective guards.

W-2 — Hardcoded semanticsLabel: 'Loading' bypasses l10n — 🟢High [both reviewers]

row_blocks.dart:175, 295 — Use loc(context).loading or equivalent. (C-1 fix via AppLoader likely handles this internally.)

W-3 — Single 'wifi_network' key spins all SSID rows simultaneously — 🟢High [both reviewers]

usp_wifi_networks_card.dart:97

final isLoading = ref.watch(uspMutationLoadingProvider) == 'wifi_network';

Called per-SSID in the build loop. When one SSID toggle fires, every row enters isLoading=true.

Fix: Use per-SSID keys ('wifi_network_${network.ssidName}') and hoist ref.watch to build().

W-4 — Duplicate spinner construction — 🟢High [both reviewers]

row_blocks.dart:170-177, 286-299 — Identical construction duplicated with only dimension constants differing.

Fix: Extract _SwitchSpinner({required double size}), or resolve via C-1 fix.

W-5 — Spinner shown on instancePath==null rows — 🟢High [single, Reviewer A]

usp_port_forwarding_card.dart:78 — Global loading key shows spinner on all rows including ones that cannot be mutated.

Fix: isLoading: isLoading && rule.instancePath != null

W-6 — Share button tappable during mutation — 🟡Med [single, Reviewer B]

row_blocks.dart:281-284 — _ShareButton still rendered and tappable when isLoading==true.

Fix: if (!isLoading && isEnabled && onShareTap != null)

W-7 — No golden/widget test for isLoading=true visual path — 🟢High [both reviewers]

Golden test suites cover with_data/empty states only — zero coverage for the primary new visual feature (spinner replacing switch).

Fix: Add isLoading golden state overriding uspMutationLoadingProvider for each affected card.

✅ What Looks Good

Double-tap prevention correct: onChanged: null + isLoading: true driven by same boolean — no race window.
context.mounted checked after async gap: _confirmToggleNetwork correctly guards.
performUspMutation resets loading key in finally: Spinner always clears on exception.
DHCP toggle correctly extended: usp_dhcp_reservations_card.dart toggle onChanged guards instancePath == null — correct pattern.
Professional layout coordinates correct: No grid overlaps; wifi_networks added to both cardIds and _professionalLayout().
Test count updates correct: Descriptions updated 17→18 in both test files.

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

- Replace CircularProgressIndicator with AppLoader from ui_kit_library - Add instancePath null guard to DHCP delete button - Add instancePath null guard to port forwarding/triggering toggles - Disable share button during loading state - Hoist ref.watch to build() in WiFi networks card

HankYuLinksys · 2026-07-14T07:04:33Z

Thanks for the thorough reviews!

Addressed in this update

C-1: Replaced CircularProgressIndicator with AppLoader from ui_kit_library
C-2: Added instancePath == null guard to DHCP delete button
W-2: Added instancePath == null guard to port forwarding/triggering toggles
W-5: Share button now disabled during loading state
W-7: Hoisted ref.watch to build() in WiFi networks card

Not addressing in this PR

W-3 (single loading key spins all rows): This is existing behavior across all dashboard cards (DHCP, Port Forwarding, WiFi Networks). Changing to per-item keys would require broader architectural changes — will track separately if needed.
W-6 (no golden test for isLoading=true): Can be added as a follow-up.
Nits (loading key naming inconsistency, doc comment typo, dimmed spinner on disabled SSID): Cosmetic, can address later.

AustinChangLinksys

🤖 Automated Review — Round 1 · 69eb8f1..6bd5d7c (full)

Verdict: ✅ APPROVE — All 4 prior Criticals resolved; 2 Warnings and 2 Suggestions remain open.

	Conf.	Where	Issue (one-liner)
⚠️	🟢High	`row_blocks.dart:~171,~284`	[both reviewers] `AppLoader` called without `semanticLabel` — spinner invisible to screen readers (regression vs. prior form)
⚠️	🟢High	`usp_wifi_networks_card.dart:57,~142`	[both reviewers] Single `'wifi_network'` loading key makes all SSID rows spin simultaneously on any one toggle
⚠️	🟡Med	`usp_dhcp_reservations_card.dart:76` / `usp_port_forwarding_card.dart:78,99`	[single, Reviewer A; contested by Reviewer B] `isLoading` passed unconditionally — `instancePath==null` rows show spinner during unrelated mutations
💡	🟢High	`row_blocks.dart:~171,~284`	[both reviewers] Two `AppLoader` constructions with divergent sizes (26 vs 24 px) — extract shared helper for visual consistency
💡	🟢High	`test/`	[both reviewers] No widget/golden test for `isLoading=true` path — zero regression coverage for primary new visual feature

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.

✅ Resolved from prior round

C-1 ✅ — CircularProgressIndicator replaced with AppLoader(strokeWidth: 2) in both ToggleRow and NetworkRow. Architecture rule satisfied.
C-2 ✅ — DHCP delete button now guards reservation.instancePath == null: onTap: isLoading || reservation.instancePath == null ? null : _confirmDeleteDhcp(...) (usp_dhcp_reservations_card.dart:91-93). The bang at line 203 is unreachable when instancePath == null.
W-1 ✅ — Port forwarding toggles guard instancePath == null before ! dereference: onChanged: isLoading || rule.instancePath == null ? null : ... (and same for triggering row) at usp_port_forwarding_card.dart:79,100.
W-6 ✅ — Share button now hidden during mutation: if (!isLoading && isEnabled && onShareTap != null) in NetworkRow.

⚠️ Warning Details

W-1 — AppLoader called without semanticLabel — 🟢High [both reviewers]

lib/page/_shared/components/layout_blocks/row_blocks.dart:~171 (ToggleRow) and :~284 (NetworkRow)

// ToggleRow
child: AppLoader(strokeWidth: 2),   // no semanticLabel

// NetworkRow
child: AppLoader(strokeWidth: 2),   // no semanticLabel

AppLoader.semanticLabel defaults to null; when null, no Semantics wrapper is applied and the spinner is entirely invisible to VoiceOver / TalkBack. The prior form had semanticsLabel: 'Loading' (hardcoded English, but present); the new form has no announcement at all — a regression for assistive-technology users.

Fix: AppLoader(strokeWidth: 2, semanticLabel: loc(context).loading) at both locations.

W-2 — Single 'wifi_network' loading key spins all SSID rows simultaneously — 🟢High [both reviewers]

lib/page/wifi_settings/cards/usp_wifi_networks_card.dart:57

// build() — one boolean shared by all rows:
final isLoading = ref.watch(uspMutationLoadingProvider) == 'wifi_network';

// _confirmToggleNetwork (~line 142):
loadingKey: 'wifi_network',  // same key regardless of which SSID was toggled

When any SSID toggle fires, every row receives isLoading=true. On a router with 4 SSIDs, toggling one causes all 4 rows to show spinners. The refactor correctly hoisted ref.watch to build() (performance improvement), but the root cause — the shared key — was not addressed.

Fix: Use a per-SSID key: loadingKey: 'wifi_network_${network.ssidName}' and compute isLoading per-row, or document that card-level mutual exclusion is intentional.

W-3 (contested) — isLoading passed unconditionally to rows with instancePath==null — 🟡Med [single, Reviewer A; Reviewer B disagrees]

usp_dhcp_reservations_card.dart:76 / usp_port_forwarding_card.dart:78,99

isLoading: isLoading is passed unconditionally. Rows with instancePath==null (locally created, unsaved items) have onChanged: null (cannot be mutated), but still display a spinner during any mutation on another row. Reviewer B considers this acceptable (global loading state intentionally locks all rows); Reviewer A flags it as misleading UX. Please confirm intent.

Fix if desired: isLoading: isLoading && reservation.instancePath != null

💡 Suggestion Details

S-1 — Two AppLoader constructions with divergent sizes — 🟢High [both reviewers]

row_blocks.dart:~171 uses SizedBox.square(dimension: 26, child: AppLoader(strokeWidth: 2)) while :~284 uses SizedBox(w:52, h:32, child: Center(child: SizedBox.square(dimension: 24, child: AppLoader(strokeWidth: 2)))). Sizes differ (26 vs 24 px) with no documented rationale. Any future style change requires editing two places.

Fix: Extract Widget _buildRowSpinner({double size = 24}) => SizedBox.square(dimension: size, child: AppLoader(strokeWidth: 2));

S-2 — No widget/golden test for isLoading=true path — 🟢High [both reviewers]

The primary visual change (spinner replacing switch) has zero regression coverage. The diff only updates card-count assertions (17→18). A future AppLoader sizing change or ToggleRow.isLoading branching refactor could silently break the loading UI.

Fix: Add widget tests for ToggleRow(isLoading: true) and NetworkRow(isLoading: true) verifying AppLoader is in the tree and AppSwitch is absent; plus card-level tests verifying uspMutationLoadingProvider drives isLoading.

✅ What Looks Good

AppLoader (ui_kit_library) correctly used in both ToggleRow and NetworkRow — architecture rule satisfied.
instancePath==null guards added to DHCP delete button and port forwarding toggles — correct pattern, matches existing DHCP toggle guard.
Share button correctly hidden during loading in NetworkRow.
ref.watch(uspMutationLoadingProvider) hoisted to build() in UspWifiNetworksCard — correct refactor, no per-row subscription overhead.
wifi_networks added to both professional.cardIds and _professionalLayout() — consistent; grid coordinates non-overlapping.
Test count assertions updated 17→18 in both test files.
context.mounted checked after async gap in _confirmToggleNetwork.
performUspMutation resets loading key in finally — spinner always clears on exception.

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

AustinChangLinksys · 2026-07-14T07:49:41Z

+                  dimension: 26,
+                  child: AppLoader(strokeWidth: 2),
+                )
+              : AppSwitch(


Warning W-1: AppLoader called without semanticLabel -- spinner is invisible to VoiceOver/TalkBack. Pass semanticLabel: loc(context).loading here and at the equivalent NetworkRow site.

AustinChangLinksys · 2026-07-14T07:49:41Z

    final data = wifiData ?? ref.watch(wifiDataProvider).valueOrNull;
    if (data == null) return const CardSkeleton.list(rows: 3);

+    final isLoading = ref.watch(uspMutationLoadingProvider) == 'wifi_network';


Warning W-2: Single 'wifi_network' loading key means all SSID rows spin simultaneously on any one toggle. Consider per-SSID keys or document that card-level locking is intentional.

HankYuLinksys force-pushed the fix/1055-dashboard-toggle-spinner branch from ca45a7c to 9874e7c Compare July 14, 2026 01:11

AustinChangLinksys linked an issue Jul 14, 2026 that may be closed by this pull request

[Suggestion] Dashboard: WFi Status/WiFi Network: It is recommended to show a spinner when toggling on/off the WiFi band/WiFi Network #1055

Open

HankYuLinksys requested review from AustinChangLinksys and PeterJhongLinksys July 14, 2026 02:53

AustinChangLinksys reviewed Jul 14, 2026

View reviewed changes

AustinChangLinksys added the need-decide Automated review left findings needing Austin to decide (fix/defer/dismiss). label Jul 14, 2026

AustinChangLinksys reviewed Jul 14, 2026

View reviewed changes

PeterJhongLinksys reviewed Jul 14, 2026

View reviewed changes

AustinChangLinksys reviewed Jul 14, 2026

View reviewed changes

AustinChangLinksys approved these changes Jul 14, 2026

View reviewed changes

AustinChangLinksys merged commit b9c33cd into dev-2.6.0 Jul 15, 2026
2 checks passed

AustinChangLinksys deleted the fix/1055-dashboard-toggle-spinner branch July 15, 2026 02:07

HankYuLinksys mentioned this pull request Jul 15, 2026

[Suggestion] Dashboard: WFi Status/WiFi Network: It is recommended to show a spinner when toggling on/off the WiFi band/WiFi Network #1055

Open

Navigation Menu

Uh oh!

feat(dashboard): show spinner on card toggle during mutation (#1055) - #1126

feat(dashboard): show spinner on card toggle during mutation (#1055)#1126
AustinChangLinksys merged 2 commits into
dev-2.6.0from
fix/1055-dashboard-toggle-spinner

HankYuLinksys commented Jul 14, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jul 14, 2026

Uh oh!

AustinChangLinksys left a comment

Uh oh!

AustinChangLinksys left a comment

Uh oh!

PeterJhongLinksys left a comment

Uh oh!

AustinChangLinksys left a comment

Uh oh!

HankYuLinksys commented Jul 14, 2026

Uh oh!

AustinChangLinksys left a comment

Uh oh!

AustinChangLinksys Jul 14, 2026

Uh oh!

AustinChangLinksys Jul 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

HankYuLinksys commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Toggle Spinner

Professional Preset Fix

Affected Toggles

Test Plan

Notes

Uh oh!

qodo-code-review Bot commented Jul 14, 2026

Qodo reviews are paused for this user.

Uh oh!

AustinChangLinksys left a comment

Choose a reason for hiding this comment

C-1 — CircularProgressIndicator instead of AppLoader from ui_kit_library — violates UI architecture rule

Uh oh!

AustinChangLinksys left a comment

Choose a reason for hiding this comment

C-1 — CircularProgressIndicator instead of AppLoader from ui_kit_library — violates UI architecture rule

Uh oh!

PeterJhongLinksys left a comment

Choose a reason for hiding this comment

Automated PR Review

Findings

Already raised (skipped to avoid duplication)

Verified

Uh oh!

AustinChangLinksys left a comment

Choose a reason for hiding this comment

C-1 — CircularProgressIndicator instead of AppLoader (UI architecture violation)

C-2 — _confirmDeleteDhcp calls instancePath! with no null guard on delete button (NEW)

Uh oh!

HankYuLinksys commented Jul 14, 2026

Addressed in this update

Not addressing in this PR

Uh oh!

AustinChangLinksys left a comment

Choose a reason for hiding this comment

Uh oh!

AustinChangLinksys Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

AustinChangLinksys Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HankYuLinksys commented Jul 14, 2026 •

edited

Loading

C-1 — `CircularProgressIndicator` instead of `AppLoader` from `ui_kit_library` — violates UI architecture rule

C-1 — `CircularProgressIndicator` instead of `AppLoader` from `ui_kit_library` — violates UI architecture rule

C-1 — `CircularProgressIndicator` instead of `AppLoader` (UI architecture violation)

C-2 — `_confirmDeleteDhcp` calls `instancePath!` with no null guard on delete button (NEW)