Disable remote assistance for local login#1102
Conversation
Comment out the remote assistance entry points for local login sessions: - Skip the active remote session check during polling in polling_provider, which prevents the passive dialog from being triggered in dashboard shell. - Hide the support agent button that launches the remote assistance dialog from the top bar.
Reformat files with dart format (line wrapping and indentation only, no logic changes) across pnp setup/admin views, add nodes provider and view, wifi name field, top bar, and the related localization tests and pnp notifier mocks.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 1 · 0fc8374..9e383d1 (full)
Verdict: ✅ APPROVE — Remote assistance correctly disabled at both entry points (TopBar button + polling session check); no security or correctness issues found. 5 housekeeping warnings to clean up before re-enabling the feature.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| 🟢High | polling_provider.dart:18 |
[both reviewers] Dead import — remote_client_provider.dart import remains but its only callsite is now commented out |
|
| 🟢High | polling_provider.dart:183 |
[both reviewers] Dead variable loginType — assigned but never consumed; causes unnecessary authProvider read on every polling tick |
|
| 🟢High | top_bar.dart:12 |
[both reviewers] Dead import — remote_assistance_dialog.dart import remains but its only callsite (showRemoteAssistanceDialog) is commented out |
|
| 🟡Med | dashboard_shell.dart:44–72 |
[both reviewers] Passive auto-popup path not annotated — ref.listen on remoteClientProvider.sessionInfo can still trigger showRemoteAssistanceDialog for local login if session state changes; currently unreachable but undocumented landmine |
|
| 🟢High | test/ |
[single — Reviewer B] No regression test asserting Icons.support_agent is absent for LoginType.local after button removal |
|
| 💡 | — | polling_provider.dart:184, top_bar.dart:149 |
TODO comments carry no ticket reference — add // TODO(#TICKET): re-enable when remote assistance is ready |
| 💡 | — | add_nodes_provider.dart:161–177 |
Pre-existing (not introduced by this PR): childNodes.sort(), polling.forcePolling(), and state.copyWith(isLoading: false) run unconditionally outside the if (onboardingProceed && anyOnboarded) guard — worth confirming intentional |
| 💡 | — | add_nodes_provider.dart:107 |
Pre-existing: var deviceOnboardingStatus = [] is untyped (List<dynamic>); opportunity to tighten type in this formatter pass |
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: Dead import in polling_provider.dart 🟢High [both reviewers]
- Location:
lib/core/jnap/providers/polling_provider.dart:18 - Evidence:
import 'package:privacy_gui/core/cloud/providers/remote_assistance/remote_client_provider.dart';— the only callsiteref.read(remoteClientProvider.notifier).checkActiveSession()is commented out at lines 185–188. No other reference toremoteClientProviderin this file. - Impact:
unused_importlint warning; CI configs that treat warnings-as-errors will break the build. - Fix: Remove line 18, or guard with a compile-time feature flag.
W-2: Dead variable loginType in polling_provider.dart 🟢High [both reviewers]
- Location:
lib/core/jnap/providers/polling_provider.dart:183
final loginType = ref.read(authProvider).value?.loginType;
// TODO: Disable remote assistance for now
// logger.i('[Polling]: _additionalPolling loginType: $loginType');
// if (loginType == LoginType.local) { ... }- Impact: Variable is assigned (triggering
ref.read(authProvider)on every polling cycle) but never consumed. Produces anunused_local_variablelint warning. - Fix: Comment out or remove the
loginTypedeclaration alongside the block it guarded.
W-3: Dead import in top_bar.dart 🟢High [both reviewers]
- Location:
lib/page/components/styled/top_bar.dart:12 - Evidence:
import '...remote_assistance_dialog.dart'— the only callsiteshowRemoteAssistanceDialog(context, ref)is commented out at lines 149–159._startRemoteAssistance()(remote-login path) callsshowSimpleAppDialog+initiateRemoteAssistanceCA(), notshowRemoteAssistanceDialog. - Fix: Remove line 12.
W-4: Passive remote-assistance listener not annotated in dashboard_shell.dart 🟡Med [both reviewers]
- Location:
lib/page/dashboard/views/dashboard_shell.dart:44–72 - Evidence:
ref.listen(
remoteClientProvider.select((state) => state.sessionInfo?.status),
(prevStatus, nextStatus) {
final loginType = ref.read(authProvider).value?.loginType;
if (loginType != LoginType.local) return;
if (prevStatus != GRASessionStatus.active &&
nextStatus == GRASessionStatus.active && ...) {
showRemoteAssistanceDialog(context, ref, isPassive: true);
}
},
);- Current state: Currently unreachable —
checkActiveSession()was the only path that could updatesessionInfofor local login, now commented out. Manual button also commented out. - Risk: The TODO in
polling_provider.dart/top_bar.dartsignals a temporary disable. The passive listener carries no matching TODO. Any future commit adding asessionInfoupdate path for local login will silently reactivate the dialog. - Fix: Add
// TODO(#TICKET): re-enable when remote assistance is restoredcomment, or stub the listener body with an explicit guard.
W-5: No regression test for support_agent button removal 🟢High [single — Reviewer B]
- Location:
test/page/components/styled/top_bar_test.dart - Issue: Existing TopBar tests do not assert that
Icons.support_agentis absent whenloginType == LoginType.local. The UI entry-point removal is unverified. - Fix: Add:
expect(find.byIcon(Icons.support_agent), findsNothing)in aLoginType.localscenario test.
✅ What Looks Good
- Both entry points disabled together:
checkActiveSession()in_additionalPolling(polling path) and thesupport_agentbutton in TopBar (manual path) are both commented out in the same PR — no half-open state. - Remote-login path fully preserved:
_startRemoteAssistance(),initiateRemoteAssistanceCA(), the session-expire counter, andendRemoteAssistanceflows are all untouched. checkActiveSession()is safely removable: The method wraps all cloud calls in try/catch; removing it from the polling loop cannot crash or corrupt state.add_nodes_provider.dartdiff is semantically neutral: The indentation fix re-aligns the block to the enclosingtryscope; control flow (childNodes.sort()etc. running unconditionally) is identical before and after.pnp_admin_view.dartandpnp_setup_view.dart: Formatter-only line-wrap changes to.catchErrorchains — no logic altered.- No hardcoded secrets, no injection surface, no JNAP result-code bypass anywhere in the diff.
remoteClientProvidercorrectly non-autoDispose (global session singleton);AddNodesNotifiercorrectlyautoDispose(page-scoped). Layer boundaries respected.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
* chore: bump version to 1.3.0 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat: remote assistance for local login users (#956) * feat: enable remote assistance support agent icon and improve remote checkDeviceInfo timeout - Uncomment and enable support_agent icon in TopBar for local login - Add retry (1) and increased timeout (10s) for checkDeviceInfo in remote mode - Add explicit text color for session expire counter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: format codebase Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: detect active remote session for local login and show alert dialog - Add checkActiveSession in polling for local login users - Add session status listener in DashboardShell with loginType guard - Support passive mode in remote assistance dialog - Fix countdown timer using .abs() instead of * -1 - Prevent dialog from showing in remote login mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add comment explaining async stream state update timing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: format remote assistance code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: extract magic number 2700 to named const and add unit tests for checkActiveSession - Extract 2700 to kPendingSessionDurationSec constant (45-min PENDING window) - Add unit tests for checkActiveSession (empty/active/error cases) - Add unit tests for countdown .abs() behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: continuously poll session info in active remote assistance dialog Keep polling session info while the active-mode remote assistance dialog is open, auto-request a PIN once a session exists, and avoid opening a second dialog when the session becomes active. - Add nextPollInterval helper (5s before active, 60s once active). - Add pollSessionOnce: clears session info and pin when no session exists, requests a PIN when the session is INITIATE or PENDING and no PIN exists. - Rewrite initiateRemoteAssistance to poll once immediately then keep polling via a cancelable timer until the dialog closes; no longer returns early on empty sessions. Guard against re-entry and keep the loop alive across transient poll errors. - Add an isDialogShown flag to RemoteClientState so the dashboard does not auto-open a passive dialog while a dialog is already shown. - Document the hasInitialized guard and the dashboard's local re-entry guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style: apply dart format to pnp files Apply `dart format` formatting to instant setup (PnP) files and the generated pnp mocks. No logic changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: address PR review for remote assistance - Move kPendingSessionDurationSec from the view layer to the core model (guardians_remote_assistance.dart) per the 3-layer architecture rule. - Reference kPendingSessionDurationSec in the countdown tests instead of hardcoding 2700, and assert the full pending-window formula. - Replace the bespoke _FakeDeviceManagerNotifier with the existing generated MockDeviceManagerNotifier. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * feat: add Web Interface Guide link to FAQ page Add a prominent card at the top of the FAQ list linking to the generic Pinnacle router support article (KB #9921). The card includes info and external link icons in primary color. Added translations for all 26 supported languages. Closes #985 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add identifier to AppCard for UI automation and a11y Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: screenshot testing fixes and documentation (#990) * fix: add simpleModeWifi to wifi test data and fix semanticLabel spacing - Add simpleModeWifi field to all wifi list test state constants - Set isSimpleMode to false for advanced mode tests - Fix semanticLabel spacing issue in WifiNameField and WifiPasswordField when semanticLabel is empty - Add IPTV enabled test state and screenshot test for Advanced settings Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: update test data and mocks for screenshot tests - Add macAddress field to all node detail test states - Update MockServiceHelper with new isSupportHealthCheckManager2 method Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * docs: add screenshot testing guide and update mock file - Add English screenshot testing guide covering commands, mock workflow, common errors, and test data locations - Fix minor formatting in jnap_service_supported_mocks.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * feat: add Auto Master status check in PnP flow (#994) * feat: add Auto Master status check in PnP flow to prevent race condition - Add GetAutoMasterStatus JNAP action to detect if Auto Master is running - Implement two defense lines: 1. PnP Admin: check after examineAdminPassword, wait if running 2. PnP Setup: check before save, handle edge case (Idle→Complete) - Add connection error UI with retry when polling fails 3 consecutive times - Add screenshot tests for Auto Master waiting and connection error states Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add auth flag and timeout handling for Auto Master status check - Add auth: true to getAutoMasterStatus API calls in pnp_provider - Add _checkAutoMasterStatus step in admin view login flow - Add timeout fallback logic when Auto Master polling exceeds max retry - Handle router reconnection check after polling timeout Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: handle unauthorized error in Auto Master status check Add ExceptionAutoMasterUnauthorized to handle 401 errors during Auto Master status check, redirecting users to login page when session expires. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address code review issues in Auto Master status check - Add missing ExceptionAutoMasterUnauthorized handler in _retryAutoMasterCheck - Add retry limit to prevent infinite recursion in _saveChanges - Use ValueGetter pattern for autoMasterStatusOnEntry in copyWith - Remove unused ExceptionAutoMasterRunning class Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: throw exception on Auto Master polling failure to stop chain Critical fix: When Auto Master polling fails (consecutive failures or timeout with reconnect failure), the previous `return` statement would let the Promise chain continue, causing uncontrolled navigation to pnpConfig and bypassing the Auto Master protection. Changes: - Add ExceptionAutoMasterPollingFailed exception - Replace `return` with `throw ExceptionAutoMasterPollingFailed()` on failure paths - Add catchError handlers in initState chain, Continue button chain, and retry handler The UI correctly stays on the error view with retry button when polling fails. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * fix: only show DFS confirmation when DFS is being enabled The DFS confirmation dialog was showing whenever any WiFi advanced setting was saved, as long as DFS was already enabled. This was confusing when users only changed IPTV or other settings. Now the dialog only appears when DFS is being turned ON (changing from disabled to enabled), not when it's already enabled. Fixes #1004 Related: linksys/LinksysWRT#399 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: correct PnP saveChanges timing for AutoParent scenario The AutoParent scenario (false, false, true) had incorrect saveChanges logic - WiFi steps used `? null : null` which always returned null, causing saving to happen at YourNetwork instead of before it. Changes: - AutoParent: save before YourNetwork (like Unconfigured) to ensure smart mode = master before adding nodes - PrePaired fallback: restore original design with no saveChanges, letting onLastStep handle saving - Fix Auto Master tests: use immediate Stream.value to avoid pending timer issues, rename unauthorized test to connection error test Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: handle AutoParent flow in _saveChanges and reconnect The initial fix only updated buildSteps but missed the flow control logic that determines what happens after saving completes. Changes: - Update _saveChanges whenComplete to use showYourNetwork instead of isUnconfigured, so AutoParent continues to YourNetwork step - Update reconnect flow to use showYourNetwork for consistent behavior - Update onLastStep to use showYourNetwork to avoid duplicate saving - Improve Auto Master connection error UI with AppCard and maxWidth Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: restore onLastStep for forceLogin + !isPrePaired scenario The previous change to onLastStep condition broke the forceLogin scenario where isUnconfigured=false and isPrePaired=false. In this case: - buildSteps returns only PersonalWiFiStep() without saveChanges - onLastStep was set to null due to !isPrePaired condition - Result: WiFi changes were silently dropped Fix: Exclude forceLogin from the !isPrePaired condition so that forceLogin scenarios still use onLastStep for saving. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: merge duplicate case branches and add canGoNext guard to GuestWiFiStep Address code review feedback: - Merge (false, true, _) and (false, false, true) into (false, _, true) since they had identical bodies - prevents future divergence - Add canGoNext(saveChanges == null) to GuestWiFiStep.onInit to prevent potential double-advance when GuestWiFi is the save-carrying step Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat: add Failed status support for Auto Master - Add 'failed' status to AutoMasterStatus enum for cases where Auto Master exits without configuring the device (e.g., found another Master on network) - When Auto Master status is 'Failed', continue normal PnP flow instead of redirecting to login page, since password is still 'admin' Ref: LinksysWRT#383 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: handle Failed status in pnp_provider and pnp_setup_view - Add failed to pollAutoMasterStatus() condition to stop polling - Add failed status handling in pnp_setup_view's polling loop - Use flag to skip timeout handling when Auto Master failed This fixes the issue where users would be stuck on the waiting screen for ~5 minutes when Auto Master reports Failed status. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add loading indicator and retry for SmartConnect not ready in Add Nodes - Replace AppFilledButton with AppFilledButtonWithLoading for the Next button - Add retry logic (3s interval, 20 retries max = 60s) for ErrorSmartConnectNotReady - Show error snackbar on timeout, allow manual retry - Create AddNodesException sealed class for exception handling - Hide topbar only when entering from PnP flow This fix provides visual feedback while waiting for SmartConnect to be ready. It does not address the root cause (SmartConnect startup time after PnP). Closes #1018 Related: linksys/LinksysWRT#396 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address code review issues for Add Nodes loading retry - Remove unused ExceptionSmartConnectNotReady class - Use try-finally to ensure polling resumes on error/timeout - Add async/await and try-catch to _resultView tryAgain button - Add general catch block to prevent spinner freeze on non-timeout errors - Replace showSimpleSnackBar with showFailedSnackBar for error states Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: reset isLoading in finally block to prevent frozen spinner Ensures isLoading is reset to false even if an exception occurs during the auto-onboarding process, preventing the UI from being stuck on the loading spinner. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: show loading spinner during SmartConnect retry and add mounted check - Move isLoading: true before retry loop so full-screen spinner shows during the entire retry period (~57s), not just after retry succeeds - Update plugins/widgets submodule with mounted check fix - Remove unused import Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Disable remote assistance for local login (#1102) * Disable remote assistance for local login Comment out the remote assistance entry points for local login sessions: - Skip the active remote session check during polling in polling_provider, which prevents the passive dialog from being triggered in dashboard shell. - Hide the support agent button that launches the remote assistance dialog from the top bar. * Apply dart format to touched files Reformat files with dart format (line wrapping and indentation only, no logic changes) across pnp setup/admin views, add nodes provider and view, wifi name field, top bar, and the related localization tests and pnp notifier mocks. * fix: display CPU utilization as integer without decimal places (#1096) Remove unnecessary two decimal places from CPU utilization display. Values now show as "0%" instead of "0.00%" and "18%" instead of "18.00%". Fixes #1095 Related: LinksysWRT#411 Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * fix: Auto Master race condition in ISP Settings flow (#1092) * fix: add Auto Master protection to ISP Settings flow and improve error handling - Add Auto Master status check in pnp_isp_save_settings_view before saving ISP settings (Static IP / PPPoE). Shows waiting UI if Auto Master is running, redirects to PnP entry if completed. Handles polling failures and connection errors gracefully. - Handle _ErrorUnauthorized in pnp_setup_view's _saveChanges catchError. When Auto Master completes during save, the 401 error now redirects to login page instead of showing generic error. - Add WiFi settings verification after reconnect in _showNeedReconnect. Checks if SSID was applied correctly after save timeout, supports split mode. Re-saves once if mismatch detected. Fixes QA test failures: Case 3, 4, 6, 7, 9, 10 from issue #1006. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address code review issues for Auto Master race condition - Change unauthorized error redirect from localLoginPassword to pnp - Reset _waitingForAutoMaster in onRetry to prevent stuck UI state - Fix testConnection to properly await async success callback Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add mounted checks in whenComplete and WiFi verification - Add mounted check at start of whenComplete to prevent setState on disposed widget - Add mounted check before _saveChanges() in WiFi verification re-save path Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Peter Jhong <52424995+PeterJhongLinksys@users.noreply.github.com>
Summary
Disable the remote assistance feature for local login sessions.
Changes
polling_provider), which prevents the passive remote assistance dialog from being triggered in the dashboard shell for local login.dart formatto the touched files (line wrapping and indentation only, no logic changes).Notes
The remote assistance code is commented out (not removed) so it can be easily restored later.