fix(golden): stabilize flaky golden tests (edit-mode jiggle, tab switching, report links)

[PeterJhongLinksys]

Summary

Cherry-picked golden-test fixes onto dev-2.7.0. Stabilizes several flaky/broken
golden tests and improves the verify report.

Changes

1. fix(golden): repair wifi_settings dialog_edit_name interaction finder
   Fixes the dialog_edit_name interaction so it reliably opens the edit dialog.

2. feat(golden): link golden image from passing rows and drop --embed
   Verify report now links each passing row to its golden image and drops the
   --embed mode (images referenced by relative path).

3. fix(golden): stabilize dashboard edit-mode screenshot via reduce-motion
   JiggleShake now honors the platform reduce-motion flag (renders a fixed
   0-degree angle when on), and the golden runner forces disableAnimations for
   every golden. Eliminates the intermittent dashboard-edit_mode diff without
   touching the global diffThreshold — a genuine accessibility improvement too.

4. fix(golden): switch tabs via TabController to fix long-label locale crashes
   Adds a shared switchToTab(tester, index) helper that drives
   TabController.animateTo() instead of tapping the tab's geometric center.
   Long localized labels (e.g. Danish, German) pushed the rightmost tab
   off-screen, causing a RangeError. Applied across port_forwarding, statistics
   and wifi_settings golden tests.

Verification

Regenerated baselines and ran golden verify across en/ja/de × 480/1280 (full
suite): 1041 success / 0 failure.

🤖 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 · 48fb582..214b7e9 (full)

Verdict: ✅ APPROVE — No Critical issues; 3 Warnings noted. Golden infrastructure improvements are well-designed and the core logic is sound.

	Conf.	Where	Issue (one-liner)
⚠️	🟡Med	jiggle_shake.dart:52	[both reviewers] didChangeDependencies fires on any ancestor InheritedWidget change — without a short-circuit guard, _startShaking() accumulates redundant Future.delayed callbacks on every re-trigger
⚠️	🟢High	usp_wifi_settings_view_test.dart:72	find.text('HomeNetwork') couples the test to fixture data — regression from the stable bySemanticsLabel approach
⚠️	🟢High	golden_interactions.dart:36	Settle pump loop (10×50 ms) duplicates golden_runner.dart's existing _settleWithTimeout; strategies can silently diverge on future changes
💡	🟢High	golden_interactions.dart:30	[both reviewers] assert(controller != null, …) is stripped in non-debug modes; replace with explicit StateError for consistent diagnostics
💡	🟡Med	combine_results.dart:163	Regex r'/[^/]+$' won't match a trailing-slash testCaseFilePath → double-slash goldenPath → 404 "View golden" link
💡	🟢High	html_generate_functions.dart	renderImage() failure-image src is not passed through escapeHtml(), inconsistent with the new .view-golden links which are escaped
💡	🟢High	golden_interactions.dart	No re-export from golden_runner.dart; every test file must add a separate import line
💡	⚪Low	usp_port_forwarding_detail_view_test.dart	Some compound interactions retain extra settle pumps after switchToTab() — minor cleanup opportunity

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 · jiggle_shake.dart:52 — redundant Future.delayed accumulation on unrelated dependency changes 🟡Med · [both reviewers]

// jiggle_shake.dart:52-57
@override
void didChangeDependencies() {
  super.didChangeDependencies();
  _reduceMotion = MediaQuery.maybeDisableAnimationsOf(context) ?? false;
  _syncAnimation();   // called on EVERY ancestor InheritedWidget change (Theme, Directionality, Localizations…)
}

didChangeDependencies fires whenever any ancestor InheritedWidget changes, not only MediaQuery. When widget.active == true and _reduceMotion == false, each trigger calls _startShaking():

// jiggle_shake.dart:76-83
void _startShaking() {
  if (_controller.isAnimating) return;  // guards only after repeat() is running
  Future.delayed(Duration(milliseconds: _random.nextInt(50)), () {
    if (mounted && widget.active && !_reduceMotion) {
      _controller.repeat(reverse: true);
    }
  });
}

The isAnimating guard fires before the delay. If the controller hasn't started yet (delay still pending), each unrelated didChangeDependencies call queues another Future.delayed. All delayed callbacks eventually no-op (the first one sets isAnimating = true), so this is safe — but it means the widget reacts to every ancestor dependency change unnecessarily.

Fix (short-circuit on unchanged value):

@override
void didChangeDependencies() {
  super.didChangeDependencies();
  final rm = MediaQuery.maybeDisableAnimationsOf(context) ?? false;
  if (rm == _reduceMotion) return;  // no-op if value unchanged
  _reduceMotion = rm;
  _syncAnimation();
}

---

W-2 · usp_wifi_settings_view_test.dart:72 — fixture-text coupling regression 🟢High · [single]

Before this PR, dialog_edit_name used:

await tester.tap(find.bySemanticsLabel('wifi-name-2.4GHz').first);

The PR comment explains the golden runner doesn't build a semantics tree, so this finder returned nothing. The replacement:

// usp_wifi_settings_view_test.dart:72
await tester.tap(find.text('HomeNetwork').first);

'HomeNetwork' is the literal SSID from the fixture (quickSetupOffState). If the fixture SSID is renamed (e.g., for more realistic test data), the interaction silently finds the wrong element — a compile-safe failure that only surfaces as a wrong golden image. The PR's overall goal is locale-independence; a fixture-data string is no more stable than a semantic key.

Preferred fix: use a widget Key on the SSID card that does not depend on fixture content:

// Option A — key-based (locale + fixture independent)
await tester.tap(find.byKey(const Key('wifi-ssid-card-2.4GHz')).first);
// Option B — type/hierarchy based if keys are unavailable
await tester.tap(find.descendant(
  of: find.byType(WifiBandCard).first,
  matching: find.byType(InkWell)).first);

---

W-3 · golden_interactions.dart:36 — duplicated settle strategy 🟢High · [single]

// golden_interactions.dart:36-39
await tester.pump();
for (int i = 0; i < 10; i++) {
  await tester.pump(const Duration(milliseconds: 50));
}

golden_runner.dart already defines _settleWithTimeout(), which uses pumpAndSettle with a 500 ms timeout and a fallback for infinite-animation exceptions. The two strategies can now drift independently. If the settle parameters are tuned in golden_runner.dart, switchToTab doesn't benefit.

Fix: expose _settleWithTimeout (or an equivalent) as a public top-level function in golden_interactions.dart (or a shared golden_test_utils.dart) and have both call it.

💡 Suggestion Details

S-1 · golden_interactions.dart:30 — assert + ! null-check pattern 🟢High · [both reviewers]

assert(controller != null, 'TabBar has no controller to drive.');
controller!.animateTo(index);

assert is elided in non-debug modes; ! throws Null check operator used on a null value with no context. Test code always runs in debug mode so the risk is minimal, but a StateError with a clear message is more diagnostic in CI logs.
Fix: if (controller == null) throw StateError('switchToTab($index): TabBar has no controller — pass tabBarFinder if multiple TabBars exist.');

---

S-2 · combine_results.dart:163 — trailing-slash path not handled 🟡Med · [single]

final testDir = relativeTestPath.replaceFirst(RegExp(r'/[^/]+$'), '');

If testCaseFilePath ends with /, r'/[^/]+$' doesn't match (the last char is /), so testDir == relativeTestPath and goldenPath gets a //goldens/ segment → "View golden" link 404s silently.
Fix: final cleanPath = relativeTestPath.endsWith('/') ? relativeTestPath.substring(0, relativeTestPath.length - 1) : relativeTestPath; before the replaceFirst.

---

S-3 · html_generate_functions.dart — renderImage() failure-image src unescaped 🟢High · [single]

The new .view-golden link correctly wraps t.goldenPath with escapeHtml(). The existing renderImage() function builds <img src=" + src + " without escaping. Paths containing apostrophes (e.g., user home directories on macOS) break the HTML attribute. Not a security risk (local-only report), but an inconsistency worth aligning.
Fix: wrap src with escapeHtml() inside renderImage().

---

S-4 · golden_interactions.dart — no re-export from golden_runner.dart 🟢High · [single]

Three test files now independently import golden_interactions.dart. A export 'golden_interactions.dart'; line in golden_runner.dart (which every test file already imports) would remove the boilerplate and prevent future helpers from suffering the same fragmentation.

---

S-5 · Compound interactions in usp_port_forwarding_detail_view_test.dart — residual settle loops ⚪Low · [single]

After switchToTab(tester, N) already pumps 550 ms of frames, some compound interactions (dialog_add_port_range, dialog_edit_port_range, etc.) keep their own pump loops. Likely harmless over-pumping; can be cleaned up incrementally.

✅ What looks good

• Correct Flutter lifecycle: Moving MediaQuery.maybeDisableAnimationsOf to didChangeDependencies is the textbook-correct approach — initState cannot safely access InheritedWidgets.
• build() as single source of truth: final turns = (widget.active && !_reduceMotion) ? _animation.value : 0.0; ensures golden snapshots always render at 0° rotation regardless of controller state — clean defensive design.
• _startShaking() isAnimating guard: Prevents double-start on rapid didUpdateWidget / didChangeDependencies calls.
• switchToTab() rationale: Doc comment clearly explains why geometric tap fails with long-label locales and why controller-driven is the right solution — high readability for future maintainers.
• disableAnimations: true via MaterialApp.builder: Injected at infra level so all golden tests inherit it automatically, without each test needing to configure it — correct DRY design.
• _controller.animateTo(0.5) dead code removed: Old _stopShaking() called animateTo(0.5) immediately followed by reset(), making the former a no-op. PR correctly removes this misleading "approximately center" hack.
• --embed removal is complete and consistent: run_golden_verify.sh, combine_results.dart, and html_generate_functions.dart cleaned in sync — verified no stale references remain.
• Null guard on goldenPath fields: All four required fields (testCaseFilePath, tsName, deviceType, locale) checked before path computation.

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