feat: remote assistance for local login users

[PeterJhongLinksys]

Summary

• Enable support agent icon in TopBar for local login users
• Detect active remote session via polling and auto-show alert dialog in passive mode
• Improve remote checkDeviceInfo timeout (3s→10s) and retry (0→1) for remote mode
• Fix countdown timer using .abs() instead of * -1

Test plan

• Verify support agent icon appears for local login users
• Verify remote session detection triggers alert dialog automatically
• Verify dialog does not appear for remote login users
• Verify countdown timer displays correctly
• Verify checkDeviceInfo works reliably in remote mode

🤖 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 →

[HankYuLinksys]

Reviewed the Remote Assistance changes on this branch. The functional scope is small and focused: enabling the support-agent icon in the TopBar for local-login users, detecting an active remote session via the existing polling loop and auto-showing the dialog in passive mode, and a few countdown/timeout fixes. (Most of the 233-file diff is dart format churn, which is expected — no concern there.) The core logic reads well; I traced the passive-dialog flow and the _isRemoteSessionDialogShown flag does reset correctly via .then on close, so no issue there. A couple of points below, plus one bit of positive feedback:

🟠 No tests for the new RA detection logic
This PR adds checkActiveSession, startSessionInfoStream, the passive-dialog flow, and the ref.listen status-watching in DashboardShell, but none of it is covered by tests. Worth at least covering checkActiveSession (empty sessions / active session found / throws) and the countdown's .abs() behavior, so the auto-popup path doesn't silently regress.

🟢 Magic number 2700 in _buildPendingWidget
final initialSeconds = (2700 + (state.sessionInfo?.expiredIn ?? 0)).abs(); — the 2700 (presumably 45 minutes, the total PENDING window?) has no named constant or comment, so it's opaque to anyone reading this later. Suggest extracting it to a named const with a short note on what it represents.

🟢 * -1 → .abs() is a good fix
Switching the countdown from expiredIn * -1 to .abs() is the right call: since expiredIn is negative while time remains, * -1 would produce a negative display once the session crosses into expired territory, whereas .abs() stays positive in both cases. One thing still worth tightening up eventually — both forms rely on the sign convention of expiredIn and don't explicitly model the "already expired" state — but that's a low-priority follow-up, not a blocker.

[AustinChangLinksys]

🤖 Automated Review — Round 1

This PR's diff (3,517 lines across 233 files) exceeds the automated review limits (max 4,000 lines / max 60 files). Auto AI review is skipped for this round.

Recommendation: Please request a human code review.

First 15 changed files (click to expand)

1. integration_test/actions/add_nodes_actions.dart
2. integration_test/actions/dashboard_home_actions.dart
3. integration_test/actions/external_speed_test_actions.dart
4. integration_test/actions/incredible_wifi_actions.dart
5. integration_test/actions/instant_admin_actions.dart
6. integration_test/actions/instant_devices_actions.dart
7. integration_test/actions/instant_privacy_actions.dart
8. integration_test/actions/instant_topology_actions.dart
9. integration_test/actions/instant_verify_actions.dart
10. integration_test/actions/local_login_actions.dart
11. integration_test/actions/pnp_setup_actions.dart
12. integration_test/actions/prepair_pnp_setup_actions.dart
13. integration_test/actions/speed_test_actions.dart
14. integration_test/apps_and_gaming_test.dart
15. integration_test/config/integration_test_config.dart

Automated — this comment was posted by the PrivacyGUI PR review bot.

[PeterJhongLinksys]

Thanks for the review @HankYuLinksys! Addressed both actionable points in c5ab78a:

🟠 Tests for RA detection logic
Added test/core/cloud/providers/remote_assistance/remote_client_provider_test.dart covering:

• checkActiveSession: empty sessions → null, active session found → ACTIVE status + state updated, exception thrown → null (error swallowed)
• Countdown .abs() behavior: negative/positive expiredIn both produce a positive value, plus the pending-window calculation

🟢 Magic number 2700
Extracted to a named constant kPendingSessionDurationSec with a comment noting it's the 45-minute PENDING session window.

On the last point — agreed the sign-convention reliance on expiredIn and the lack of an explicit "already expired" model is worth tightening up; I'll leave that as a follow-up rather than expanding this PR's scope.

[AustinChangLinksys]

🤖 Automated Review — Round 2 · 9925300..c5ab78a (incremental)

Verdict: 💬 Comment — Contains Critical findings; final decision deferred to Austin.

	Where	Issue (one-liner)
🔴	remote_client_provider_test.dart (countdown tests)	[both reviewers] Tests hardcode 2700 instead of referencing kPendingSessionDurationSec — defeats the entire purpose of the constant extraction
🔴	remote_client_provider_test.dart (checkActiveSession tests)	[both reviewers] Tests reference checkActiveSession method which does not exist in production provider; tests will not compile/pass against current codebase
⚠️	remote_assistance_dialog.dart:94	.abs() vs * -1 formula change: numerically equivalent for expected inputs but diverges if expiredIn > 0 (server clock skew / sign-convention bug); semantics undocumented
⚠️	remote_client_provider_test.dart (countdown test)	Test only exercises expiredIn.abs() in isolation — never validates the full composed formula (kPendingSessionDurationSec + expiredIn).abs()
⚠️	remote_client_provider.dart:43	initiateRemoteAssistance() has no try/catch; test covers exception→null on checkActiveSession but production path on network error leaves dialog in permanent loading state
⚠️	remote_assistance_dialog.dart:11	kPendingSessionDurationSec is a domain/business-rule constant declared in the View layer — belongs in Service/core layer per 3-layer arch rule
⚠️	remote_client_provider_test.dart	Bespoke _FakeDeviceManagerNotifier duplicates existing MockDeviceManagerNotifier in test/mocks — will silently diverge as DeviceManagerNotifier evolves
⚠️	remote_assistance_dialog.dart:94,121	_buildPendingWidget now uses .abs(); sibling _buildCountingWidget still uses * -1 — inconsistent idiom for identical operation
💡	remote_assistance_dialog.dart:11	Doc comment on constant omits sign-convention context for expiredIn arithmetic (why the sum then .abs() pattern is needed)

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

🔴 Critical — Details

C-1 · Test hardcodes 2700 instead of kPendingSessionDurationSec [both reviewers]
File: test/core/cloud/providers/remote_assistance/remote_client_provider_test.dart (countdown timer test, ~line 141)

The new test file re-introduces the same magic number 2700 that this PR was specifically changing to a named constant. If kPendingSessionDurationSec changes (e.g., business rule changes to 3600 s), the constant is updated, the production code is correct, but the test still asserts against 2700 and passes — giving false confidence that the value is tested.

Fix: Import kPendingSessionDurationSec from the dialog file (and from the Service layer once C-1 of architecture review is addressed) and use it in the test assertion.

---

C-2 · checkActiveSession method does not exist in production [both reviewers]
File: test/core/cloud/providers/remote_assistance/remote_client_provider_test.dart

Three tests call notifier.checkActiveSession() but the production RemoteClientNotifier has no such method. The closest methods are fetchSessions() (line 35) and initiateRemoteAssistance() (line 43). These tests will produce a compile error. Either the method was planned but not added, or the test is testing a future interface.

Fix: Either add checkActiveSession() to the provider with appropriate implementation, or rewrite the tests to cover the actual existing methods.

⚠️ Warning — Details

W-1 · .abs() vs * -1 formula semantics
File: lib/page/components/styled/remote_assistance/remote_assistance_dialog.dart:94

Original: (2700 + (expiredIn ?? 0)) * -1
New: (kPendingSessionDurationSec + (expiredIn ?? 0)).abs()

For the expected domain (expiredIn ≤ 0 for pending sessions), both yield the same positive result. But .abs() and * -1 diverge when expiredIn > 0: * -1 yields a negative value (widget shows expired/zero); .abs() yields a large positive value (widget counts from a huge number). Since TimerCountdownWidget uses .take(initialSeconds + 1), a large spurious positive would cause a very long countdown. No code comment explains why .abs() is the correct choice here.

Fix: Add a comment: // expiredIn is ≤ 0 for pending sessions; adding to window then abs() yields remaining seconds. Consider max(0, kPendingSessionDurationSec + (expiredIn ?? 0)) as a safer guard.

---

W-2 · Test covers only sub-expression, not full formula
File: test/core/cloud/providers/remote_assistance/remote_client_provider_test.dart (~line 113)

The test at lines 113–123 exercises sessionInfo.expiredIn.abs() in isolation, not (kPendingSessionDurationSec + sessionInfo.expiredIn).abs(). With testSessionInfo.expiredIn = -748, the expected composed result is (2700 + (-748)).abs() = 1952. No test asserts this.

Fix: Add a test case: expect((kPendingSessionDurationSec + (-748)).abs(), 1952) or exercise _buildPendingWidget end-to-end.

---

W-3 · initiateRemoteAssistance has no error handling
File: lib/core/cloud/providers/remote_assistance/remote_client_provider.dart:43

The production initiateRemoteAssistance() method is a bare await chain with no try/catch. The test correctly verifies that exceptions return null from checkActiveSession, but if initiateRemoteAssistance() throws (network error, 401), the exception bubbles to the dialog's .then() handler at remote_assistance_dialog.dart:28, which has no .catchError(). The dialog will remain in a loading state indefinitely.

Fix: Wrap initiateRemoteAssistance() body in try/catch and emit an error state (or at minimum verify the dialog's error handling covers this path).

---

W-4 · Domain constant in View layer
File: lib/page/components/styled/remote_assistance/remote_assistance_dialog.dart:11

kPendingSessionDurationSec represents a cloud/business rule (45-minute PENDING window). By PrivacyGUI's 3-layer rule, View-layer files must not define domain constants that other layers need. If a Provider or Service class ever needs this timeout value, it cannot import from the View layer. Fix: Move to lib/core/cloud/constants.dart or alongside GRASessionInfo in guardians_remote_assistance.dart.

---

W-5 · _FakeDeviceManagerNotifier duplicates existing mock
File: test/core/cloud/providers/remote_assistance/remote_client_provider_test.dart (bottom)

The project has MockDeviceManagerNotifier in test/mocks/device_manager_notifier_mocks.dart. Using a bespoke _FakeDeviceManagerNotifier creates a second implementation that will diverge silently when DeviceManagerNotifier adds new abstract methods. Fix: Use overrideWith(() => MockDeviceManagerNotifier()) consistent with existing test patterns.

---

W-6 · Inconsistent sign-flip idiom in sibling functions
File: lib/page/components/styled/remote_assistance/remote_assistance_dialog.dart:94,121

_buildPendingWidget now uses .abs() while adjacent _buildCountingWidget still uses * -1 for the same sign-flip operation. This creates a fragile inconsistency for anyone reasoning about the sign contract of expiredIn. Fix: Standardise on .abs() by updating _buildCountingWidget in the same PR.

✅ What looks good

• Extracting 2700 into kPendingSessionDurationSec is the right intention — magic number was a real readability issue. The constant name is clear and includes the unit.
• The doc comment /// Total duration (in seconds) of the PENDING session window (45 minutes). is useful.
• New test file has good structural coverage intent: empty sessions list, active session found, and exception path — exactly the three meaningful checkActiveSession branches.
• Test uses @GenerateNiceMocks + Mockito consistently with the rest of the test suite.
• setUp/tearDown with ProviderContainer.dispose() is properly lifecycle-managed.

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

[AustinChangLinksys]

🤖 Automated Review — Round 3 · c5ab78a..7b8229d (incremental)
Verdict: 💬 Comment — Critical architecture violations and async correctness issues require resolution before merge.

	Where	Issue (one-liner)
🔴	remote_client_state.dart	[both reviewers] isDialogShown is View-layer UI state stored in service-layer state object, pollutes serialization
🔴	remote_client_provider.dart	Active poll timer (_activePollTimer) never cancelled in dispose() — resource leak / runaway API traffic
⚠️	remote_client_provider.dart · pollSessionOnce	[both reviewers] fetchSessionInfo returning null silently preserves stale sessionInfo/pin in state
⚠️	remote_assistance_dialog.dart	[both reviewers] setDialogShown(false) stuck if showDialog throws — cleanup only in .then(), no .catchError
⚠️	remote_client_provider.dart · _scheduleNextPoll	In-flight pollSessionOnce (incl. createPin) continues executing after endRemoteAssistance stops polling
⚠️	remote_client_provider.dart	Two parallel session-tracking mechanisms (timer vs. stream) with divergent null-handling semantics
⚠️	remote_client_provider.dart · initiateRemoteAssistance	Polling starts unconditionally even when initial poll fails — no back-off, no retry limit
⚠️	dashboard_shell.dart	Dual-boolean guard (_isRemoteSessionDialogShown + isDialogAlreadyShown) is redundant and can diverge
💡	remote_client_provider.dart · pollSessionOnce	sessions.first without sort/filter guarantee — may latch onto stale session
💡	remote_client_state.dart	isDialogShown (ephemeral UI flag) included in toMap/fromJson — can restore as true after hot-restart

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

🔴 Critical Details

remote_client_state.dart — isDialogShown is View-layer state in service-layer object

RemoteClientState is a service-layer object tracking remote-session lifecycle data. Whether a dialog is currently visible on screen is a View-layer ephemeral UI concern. Storing it in RemoteClientState crosses the three-layer boundary upward, violates single-responsibility, and creates a serialization artifact: isDialogShown is added to fromMap/toMap/props, so if the app is force-quit while a dialog is open, it will deserialize as true and permanently block re-display.

Fix: Move isDialogShown to a local bool field in _DashboardShellState, or use a minimal StateProvider<bool> in the View layer.

---

remote_client_provider.dart — Active poll timer not cancelled in dispose()

RemoteClientNotifier extends Notifier with no dispose() override anywhere in the file. The _activePollTimer self-reschedules indefinitely via _scheduleNextPoll(). If the container is torn down (hot-restart, router replacement, test teardown) before endRemoteAssistance() is called, the timer orphans and keeps firing cloud API calls every 5 seconds.

Fix:

@override
void dispose() {
  _stopActivePolling();
  _expiredCountdownTimer?.cancel();
  super.dispose();
}

⚠️ Warning Details

Stale sessionInfo when fetchSessionInfo returns null [remote_client_provider.dart · pollSessionOnce]

final sessionInfo = await fetchSessionInfo(sessions.first.id);
if (sessionInfo == null) {
  return null;   // ← state.sessionInfo NOT cleared
}

The previous initiateRemoteAssistance did state = RemoteClientState() on null return — a full reset. Now state is left stale, and _scheduleNextPoll() uses the old state.sessionInfo?.status for its interval, potentially keeping 60-second cadence after the session is gone.

Fix: Add state = state.copyWith(sessionInfo: () => null, pin: () => null); before the early return.

---

setDialogShown(false) can get permanently stuck [remote_assistance_dialog.dart]

setDialogShown(true) is called before showDialog; the reset is only in .then(). If showDialog throws synchronously (stale context, disposed provider), .then() never fires and isDialogShown stays true permanently, blocking all future dialog presentations.

Fix: Chain .catchError((_) => notifier.setDialogShown(false)) alongside .then().

---

In-flight pollSessionOnce after endRemoteAssistance [remote_client_provider.dart · _scheduleNextPoll]

pollSessionOnce performs 3 sequential awaits. During that window, endRemoteAssistance can fire. _stopActivePolling() sets _activePolling = false, but the in-flight coroutine resumes and state mutations (including createPin) can still fire for a session being cleaned up.

---

Two parallel session-tracking mechanisms [remote_client_provider.dart]

Post-merge the notifier has two entirely separate session-tracking mechanisms: initiateRemoteAssistance (timer-based, local-login) and initiateRemoteAssistanceCA (stream-based, cloud-auth), with different null-handling and idempotency contracts. Consider extracting a unified _RemoteSessionPoller collaborator.

---

No back-off when initial poll fails [remote_client_provider.dart · initiateRemoteAssistance]

_startActivePolling() is called unconditionally even when initial pollSessionOnce throws. If the device is offline, the loop hammers the cloud API every 5 seconds indefinitely with no back-off.

---

Dual-boolean guard in dashboard_shell.dart

Two independent booleans (_isRemoteSessionDialogShown + isDialogAlreadyShown) guard the same condition. If they diverge (stuck-flag scenario from above), the composite guard either allows a duplicate dialog or permanently prevents it. Consolidate to one authoritative source.

✅ What looks good

• 200+ lines of well-structured unit tests covering pollSessionOnce, nextPollInterval, polling lifecycle, and dialog flag — excellent regression coverage.
• The hasInitialized guard in showRemoteAssistanceDialog correctly prevents re-running init on every Consumer rebuild.
• setDialogShown(true) called before showDialog (not inside the builder) avoids a window where the dialog is open but the flag is unset.
• _stopActivePolling() called first in _startActivePolling() is a clean idempotent restart pattern.
• The 5s/60s adaptive interval is a sensible performance/responsiveness tradeoff.
• Formatting-only changes in pnp_*.dart files are appropriate housekeeping.

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

[PeterJhongLinksys]

Thanks for the automated review. Note this round was based on 9925300..c5ab78a, which predates some follow-up work already pushed to this branch — a few findings were resolved in the meantime. Here's the status of each item:

Already resolved on the branch (predating this review's base):

• C-2 (checkActiveSession doesn't exist): it does exist in the provider and is covered by tests.
• W-3 (initiateRemoteAssistance has no error handling): now wrapped in try/catch; the poll loop also survives transient errors.
• W-6 (.abs() vs * -1 inconsistency): both _buildPendingWidget and _buildCountingWidget now use .abs().

Addressed in this round:

• C-1 / W-2: countdown tests now reference kPendingSessionDurationSec instead of hardcoding 2700, and assert the full pending-window formula (including the testSessionInfo case).
• W-4: moved kPendingSessionDurationSec out of the view layer into the core model (guardians_remote_assistance.dart) per the 3-layer rule.
• W-5: replaced the bespoke _FakeDeviceManagerNotifier with the existing generated MockDeviceManagerNotifier.

Deferred (out of this PR's scope):

• W-1: I looked into adding a max(0, ...) guard, but real session logs show expiredIn is actually a positive "seconds remaining" value across all states (PENDING ≈ 3567), which means the existing 2700 + expiredIn formula in _buildPendingWidget is itself questionable. Rather than patch a formula whose semantics need clarifying, I'm leaving the pending-window calculation as-is and will follow up separately once the intended expiredIn contract is confirmed.

[HankYuLinksys]

Thanks for the thorough breakdown @PeterJhongLinksys — the item-by-item status is clear, and I agree with deferring W-1.

One thing worth flagging from that last point: if expiredIn is actually a positive "seconds remaining" value in practice, then the model doc in guardians_remote_assistance.dart (which states negative = time remaining, example -748) contradicts the real API, and it affects #955 too (my earlier review there assumed the negative convention). Might be worth opening a small issue to track clarifying the expiredIn contract + fixing the model comment, so both lines converge on the same assumption rather than relying on .abs() to paper over it. Not blocking this PR.

[HankYuLinksys]

Approving — review feedback addressed and verified on the branch. The deferred expiredIn contract question is fine to follow up separately. Thanks for the turnaround.

[AustinChangLinksys]

🤖 Automated Review — Round 4 · 7b8229d..7345544 (incremental)
Verdict: 💬 Comment — Critical: constant moved but main usage site still hardcodes 2700

	Where	Issue (one-liner)
🔴	lib/page/components/styled/remote_assistance/remote_assistance_dialog.dart:94	_buildPendingWidget still uses magic 2700 — constant move is incomplete
⚠️	test/core/cloud/providers/remote_assistance/remote_client_provider_test.dart	New test uses active-session fixture for pending-formula — test intent is unclear
⚠️	lib/page/components/styled/remote_assistance/remote_assistance_dialog.dart:94	* -1 (production) vs .abs() (tests) semantic divergence not fixed in this PR
⚠️	lib/core/cloud/model/guardians_remote_assistance.dart:8	kPendingSessionDurationSec only consumed by UI layer — mild SRP concern for model file
💡	lib/core/cloud/model/guardians_remote_assistance.dart:44	Docstring example contains real-looking serial number — recommend placeholder
💡	Test suite	Other per-file private fake classes remain; consider migrating to shared mocks

No items were flagged by both reviewers simultaneously.

🔴 Critical Details

remote_assistance_dialog.dart:94 still hardcodes 2700

This PR moves kPendingSessionDurationSec = 2700 from remote_assistance_dialog.dart to guardians_remote_assistance.dart to create a single source of truth. However, the actual usage site in _buildPendingWidget was not updated:

// lib/page/components/styled/remote_assistance/remote_assistance_dialog.dart:94 (current)
final initialSeconds = (2700 + (state.sessionInfo?.expiredIn ?? 0)) * -1;
//                       ^^^^ should be kPendingSessionDurationSec

The import guardians_remote_assistance.dart (line 10) is already present, so the constant is accessible. This means the refactoring is only half-complete — if kPendingSessionDurationSec is ever changed, line 94 silently diverges.

Fix: Replace 2700 with kPendingSessionDurationSec at line 94. One-character change.

⚠️ Warning Details

W-1: New test uses active-session fixture for pending-formula — remote_client_provider_test.dart (new test)

testSessionInfo has status: GRASessionStatus.active and expiredIn: -748. The pending countdown formula (kPendingSessionDurationSec + expiredIn).abs() is semantically for PENDING sessions. Using an active-status fixture makes the test intent ambiguous for future maintainers.

Assertion: expect(initialSeconds, kPendingSessionDurationSec - 748) = 1952 ✓ (math is correct; testSessionInfo.expiredIn = -748 matches the model docstring example).

Fix: Add a comment: // Tests arithmetic correctness of the abs() formula; session status is irrelevant here — or use a pending-specific fixture.

W-2: * -1 vs .abs() semantic mismatch

Production code (remote_assistance_dialog.dart:94): (2700 + expiredIn) * -1 → for expiredIn = -748, result is -1952.
All tests: (kPendingSessionDurationSec + expiredIn).abs() → result is 1952.

Same magnitude, opposite signs. TimerCountdownWidget presumably expects a positive count-down value, which the * -1 produces when expiredIn is negative. The tests use .abs() which is algebraically equivalent for negative expiredIn, but the semantic intent differs. This pre-existing mismatch is not fixed in this PR. Recommend a follow-up issue.

W-3: kPendingSessionDurationSec SRP concern — guardians_remote_assistance.dart:8

This constant represents a UI countdown window, not a domain data attribute. It is consumed only by remote_assistance_dialog.dart (UI layer). All 8 other importers of guardians_remote_assistance.dart gain implicit access to a UI timing constant. Not a blocker, but consider a remote_assistance_config.dart for future constants.

✅ What looks good

• Constant extraction from dialog to model layer is directionally correct — tests now correctly reference kPendingSessionDurationSec (7 test references updated)
• _FakeDeviceManagerNotifier → MockDeviceManagerNotifier + Mockito when().thenReturn() is the right shared-mock pattern
• New test case for full formula with testSessionInfo increases coverage of the pending-calculation logic
• Test fixture data (AA:BB:CC:DD:EE:FF, serial TEST123) are clearly placeholder values — no security concern
• mockDeviceManagerNotifier.build() + ProviderContainer.overrideWith() correctly initializes Riverpod state for tests

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