Refactor providers - Topology, Instant Privacy, and Polling

[HankYuLinksys]

User description

• Create PollingService to handle JNAP communication for polling operations
  • Move checkDeviceMode (formerly checkSmartMode) to PollingService
  • Move buildCoreTransactions (formerly _buildCoreTransaction) to PollingService
  • Move executeTransaction logic to PollingService
  • Move parseCacheData logic to PollingService
  • Move updateFernetKeyFromResult (Fernet key update) to PollingService
  • PollingNotifier now delegates to PollingService while retaining state management and Timer control
  • All public APIs (pollingProvider, CoreTransactionData) remain unchanged
  • Add comprehensive unit tests for PollingService (22 tests)
  • Add unit tests for PollingNotifier state management (11 tests)
  • Add PollingTestData builder for test data generation
• Create InstantPrivacyService with 4 methods:
  - fetchMacFilterSettings() - fetches and transforms MAC filter settings
  - saveMacFilterSettings() - transforms and saves settings to JNAP
  - fetchStaBssids() - fetches STA BSSIDs if supported
  - fetchMyMacAddress() - fetches current device MAC address
  • Refactor InstantPrivacyNotifier to delegate to InstantPrivacyService
  • Remove JNAP imports from provider (architecture compliance)
  • Add error handling with JNAPError to ServiceError mapping
  • Add InstantPrivacyTestData builder for test data creation
  • Add MockServiceHelper.isSupportGetSTABSSID() mock method
  • Add 63 unit tests:
    • 22 service tests (JNAP communication, error handling)
    • 5 provider tests (delegation verification)
    • 36 state tests (enum, data class serialization)
• Create InstantPrivacyService with 4 methods:
  - fetchMacFilterSettings() - fetches and transforms MAC filter settings
  - saveMacFilterSettings() - transforms and saves settings to JNAP
  - fetchStaBssids() - fetches STA BSSIDs if supported
  - fetchMyMacAddress() - fetches current device MAC address
  • Refactor InstantPrivacyNotifier to delegate to InstantPrivacyService
  • Remove JNAP imports from provider (architecture compliance)
  • Add error handling with JNAPError to ServiceError mapping
  • Add InstantPrivacyTestData builder for test data creation
  • Add MockServiceHelper.isSupportGetSTABSSID() mock method
  • Add 63 unit tests

---

PR Type

Enhancement, Tests, Documentation

---

Description

• Create PollingService to encapsulate JNAP polling operations with methods for device mode checking, transaction building, execution, cache parsing, and Fernet key updates

• Create InstantPrivacyService with 4 methods for MAC filter settings management, STA BSSID fetching, and device MAC address retrieval with comprehensive error handling

• Create InstantTopologyService with methods for node reboot, factory reset, and LED blink operations including offline node polling with timeout detection

• Refactor PollingNotifier, InstantPrivacyNotifier, and InstantTopologyNotifier to delegate JNAP operations to respective service layers while maintaining public APIs and state management

• Remove JNAP imports from providers to enforce three-layer architecture compliance (provider → service → JNAP)

• Add 80+ comprehensive unit tests covering service operations, error handling, state serialization, and provider delegation

• Add ServiceError subtypes (TopologyTimeoutError, NodeOfflineError, NodeOperationFailedError) for topology-specific error scenarios

• Create test data builders (PollingTestData, InstantPrivacyTestData, InstantTopologyTestData) for improved test readability and maintainability

• Add mock helper methods for service capability detection (isSupportGetSTABSSID(), isSupportSetup())

• Create comprehensive specification documents including feature specs, API contracts, data models, research, plans, and quickstart guides for both InstantPrivacy and InstantTopology services

---

Diagram Walkthrough

flowchart LR
  PN["PollingNotifier<br/>Provider Layer"]
  PS["PollingService<br/>Service Layer"]
  JNAP1["JNAP<br/>Communication"]
  
  IPN["InstantPrivacyNotifier<br/>Provider Layer"]
  IPS["InstantPrivacyService<br/>Service Layer"]
  JNAP2["JNAP<br/>Communication"]
  
  ITN["InstantTopologyNotifier<br/>Provider Layer"]
  ITS["InstantTopologyService<br/>Service Layer"]
  JNAP3["JNAP<br/>Communication"]
  
  PN -- "delegates" --> PS
  PS -- "executes" --> JNAP1
  
  IPN -- "delegates" --> IPS
  IPS -- "executes" --> JNAP2
  
  ITN -- "delegates" --> ITS
  ITS -- "executes" --> JNAP3
  
  SE["ServiceError<br/>+ TopologyTimeoutError<br/>+ NodeOfflineError<br/>+ NodeOperationFailedError"]
  
  ITS -.-> SE

Loading

File Walkthrough

	Relevant files
Tests	11 files instant_privacy_service_test.dart InstantPrivacyService unit tests with error handling          test/page/instant_privacy/services/instant_privacy_service_test.dart Added 22 comprehensive unit tests for InstantPrivacyService covering JNAP communication Tests validate MAC filter settings fetching with mode-specific address splitting (allow/deny) Tests verify settings persistence with node MAC address merging and deduplication Tests confirm error handling with JNAP-to-ServiceError mapping for authorization and validation failures +688/-0  instant_privacy_state_test.dart InstantPrivacy state classes serialization and equality tests test/page/instant_privacy/providers/instant_privacy_state_test.dart Added 36 unit tests for state classes: MacFilterMode, InstantPrivacyStatus, InstantPrivacySettings, and InstantPrivacyState Tests cover enum resolution, mode validation, and copyWith functionality Tests validate serialization/deserialization (toMap/fromMap, toJson/fromJson) round-trips Tests verify equality comparisons and default initialization +431/-0  instant_topology_service_test.dart InstantTopologyService unit tests for node operations        test/page/instant_topology/services/instant_topology_service_test.dart Added 21 unit tests for InstantTopologyService covering reboot, factory reset, and LED blink operations Tests validate single master node vs. multi-node transaction building with reverse ordering Tests verify offline node polling with timeout detection and error state handling Tests confirm error mapping from JNAP failures to topology-specific ServiceError subtypes +447/-0  polling_service_test.dart PollingService unit tests for polling operations                  test/core/jnap/services/polling_service_test.dart Added 22 unit tests for PollingService covering device mode checking and core transaction building Tests validate conditional transaction command inclusion based on device mode and feature support Tests verify cache data parsing with completeness validation and null handling Tests confirm Fernet key update from device info with graceful error handling +440/-0  polling_test_data.dart PollingTestData builder for test data generation                  test/mocks/test_data/polling_test_data.dart Added PollingTestData builder class with factory methods for creating JNAP mock responses Provides sensible defaults for device mode, device info, radio info, network connections, and system stats Includes transaction builders for successful and partial-error responses Centralizes test data generation to improve test readability and maintainability +336/-0  instant_topology_provider_test.dart InstantTopologyNotifier provider delegation tests                test/page/instant_topology/providers/instant_topology_provider_test.dart Added 11 unit tests verifying InstantTopologyNotifier delegation to InstantTopologyService Tests validate method forwarding for reboot, factory reset, and LED blink operations Tests confirm proper error propagation for NodeOperationFailedError, TopologyTimeoutError, and UnauthorizedError Tests verify error details (device ID, operation, timeout duration) are preserved +299/-0  instant_privacy_provider_test.dart InstantPrivacyNotifier provider delegation tests                  test/page/instant_privacy/providers/instant_privacy_provider_test.dart Added 5 unit tests verifying InstantPrivacyNotifier delegation to InstantPrivacyService Tests validate performFetch() with forceRemote and updateStatusOnly parameters Tests confirm performSave() delegates to service with node MAC addresses Tests verify getMyMACAddress() delegation and proper parameter passing +229/-0  polling_provider_test.dart Add unit tests for polling provider and data classes          test/core/jnap/providers/polling_provider_test.dart Created comprehensive unit tests for CoreTransactionData class covering construction, copyWith updates, equality, and props Added tests for PollingNotifier.build() to verify initial state Added tests for PollingNotifier.init() to verify state reset Added tests for PollingNotifier.paused getter and setter Added tests for PollingNotifier.stopPolling() method Mocked PollingService for provider-level testing +214/-0  instant_topology_test_data.dart Create test data builder for InstantTopology service tests test/mocks/test_data/instant_topology_test_data.dart Created InstantTopologyTestData builder class for test data generation Implemented factory methods for reboot/factory reset/LED blink success responses Implemented factory methods for multi-node transaction responses Implemented createGetDevicesResponse() with configurable online/offline device lists Implemented error creation methods for reboot, factory reset, and LED blink failures Added private helper _createDeviceMap() for consistent device structure in test data +179/-0  instant_privacy_test_data.dart Create test data builder for InstantPrivacy service tests test/mocks/test_data/instant_privacy_test_data.dart Created InstantPrivacyTestData builder class for MAC filtering test data Implemented createMacFilterSettingsSuccess() with configurable filter mode and MAC addresses Implemented createStaBssidsSuccess() for STA BSSID response generation Implemented createLocalDeviceSuccess() for local device response generation Implemented createLinksysDevice() factory method for creating test device objects with connections +89/-0    jnap_service_supported_mocks.dart Add mock methods for service capability detection                test/mocks/jnap_service_supported_mocks.dart Added isSupportGetSTABSSID() mock method to MockServiceHelper for testing STA BSSID support detection Added isSupportSetup() mock method to MockServiceHelper for testing setup capability detection +20/-0
Enhancement	7 files polling_provider.dart PollingNotifier refactored to use PollingService                  lib/core/jnap/providers/polling_provider.dart Refactored PollingNotifier to delegate polling operations to new PollingService Removed inline JNAP communication logic (checkSmartMode, _buildCoreTransaction, cache parsing, Fernet key updates) Added _service getter to access PollingService via Riverpod provider Simplified method signatures by removing RouterRepository parameters +27/-128 instant_topology_service.dart InstantTopologyService for node operations                              lib/page/instant_topology/services/instant_topology_service.dart Created new InstantTopologyService with four public methods: rebootNodes(), factoryResetNodes(), startBlinkNodeLED(), stopBlinkNodeLED() Implemented waitForNodesOffline() helper for polling device status with configurable retry and timeout Added comprehensive error mapping from JNAP errors to ServiceError subtypes (NodeOperationFailedError, TopologyTimeoutError) Provided Riverpod provider for dependency injection +256/-0  instant_privacy_provider.dart InstantPrivacyNotifier refactored to use service layer      lib/page/instant_privacy/providers/instant_privacy_provider.dart Refactored InstantPrivacyNotifier to delegate JNAP operations to new InstantPrivacyService Removed inline JNAP imports and communication logic (MAC filter settings, STA BSSIDs, local device fetching) Simplified performFetch() and performSave() by delegating to service methods Removed direct RouterRepository usage in favor of service layer +30/-89  instant_topology_provider.dart Refactor InstantTopology provider to delegate to service layer lib/page/instant_topology/providers/instant_topology_provider.dart Removed JNAP-related imports (better_action, jnap_transaction, base_command, jnap_result, router_repository) Refactored reboot(), startBlinkNodeLED(), stopBlinkNodeLED(), factoryReset() methods to delegate to InstantTopologyService Removed internal _waitForNodesOffline() method (moved to service layer) Updated toggleBlinkNode() to use service delegation while keeping SharedPreferences state management in provider Added comprehensive documentation comments for all public methods +32/-116 polling_service.dart Create PollingService for JNAP polling operations                lib/core/jnap/services/polling_service.dart Created new PollingService class to encapsulate JNAP polling operations Implemented checkDeviceMode() to fetch current device mode Implemented buildCoreTransactions() to construct list of polling commands based on device capabilities Implemented executeTransaction() to execute JNAP transactions with optional cache bypass Implemented parseCacheData() to validate and convert cached data to JNAP result map Implemented updateFernetKeyFromResult() to extract and update Fernet encryption key from device info Added pollingServiceProvider for Riverpod dependency injection +182/-0  instant_privacy_service.dart Create InstantPrivacyService for MAC filtering operations lib/page/instant_privacy/services/instant_privacy_service.dart Created new InstantPrivacyService class for MAC filtering operations Implemented fetchMacFilterSettings() to fetch and transform MAC filter settings with optional status-only mode Implemented saveMacFilterSettings() to transform and save settings to JNAP with node MAC address merging Implemented fetchStaBssids() to fetch STA BSSIDs with graceful fallback for unsupported devices Implemented fetchMyMacAddress() to fetch current device MAC address from device list Added error mapping with _mapJnapError() to convert JNAPError to ServiceError Added instantPrivacyServiceProvider for Riverpod dependency injection +181/-0  service_error.dart Add topology operation error types to ServiceError              lib/core/errors/service_error.dart Added TopologyTimeoutError for timeout scenarios when nodes don't go offline within configured timeout Added NodeOfflineError for operations on offline nodes that cannot be reached Added NodeOperationFailedError for failed node operations (reboot, factory reset, LED blink) with device ID and operation type +51/-0
Documentation	17 files tasks.md Add detailed task breakdown for InstantTopology service extraction specs/001-instant-topology-service/tasks.md Created comprehensive task breakdown for InstantTopology service extraction with 49 tasks Organized tasks into 7 phases: Setup, Foundational, User Stories (3), Provider Tests, and Polish Defined task dependencies and parallel execution opportunities Mapped tasks to user stories and success criteria Included implementation strategy and checkpoint validation points +280/-0  tasks.md Add detailed task breakdown for InstantPrivacy service extraction specs/001-instant-privacy-service/tasks.md Created comprehensive task breakdown for InstantPrivacy service extraction with 43 tasks Organized tasks into 7 phases: Setup, Foundational, User Stories (4), Verification, and Polish Defined task dependencies with clear blocking relationships Mapped tasks to user stories and success criteria Included parallel execution opportunities and implementation strategies +245/-0  analysis.md Add specification analysis report for InstantTopology service specs/001-instant-topology-service/analysis.md Created specification analysis report validating requirements coverage and constitution compliance Verified all 12 functional requirements and 7 success criteria have corresponding tasks Confirmed 100% constitution compliance across Articles I, III, V, VI, VII, IX, XIII Identified 2 non-critical warnings and 3 observations Provided user story traceability matrix and task dependency validation +223/-0  instant_topology_service_contract.md Add API contract for InstantTopology service                          specs/001-instant-topology-service/contracts/instant_topology_service_contract.md Created API contract defining InstantTopologyService class structure and provider definition Documented 5 public methods: rebootNodes(), factoryResetNodes(), startBlinkNodeLED(), stopBlinkNodeLED(), waitForNodesOffline() Specified error types thrown by each method with detailed documentation Provided usage examples in provider layer Documented JNAP actions used and their authentication requirements +272/-0  spec.md Add feature specification for InstantPrivacy service extraction specs/001-instant-privacy-service/spec.md Created feature specification for InstantPrivacy service extraction Defined 4 user stories with acceptance scenarios and independent test criteria Specified 10 functional requirements and 7 success criteria Documented edge cases and assumptions Focused on architecture compliance and three-layer architecture enforcement +126/-0  quickstart.md Add quickstart guide for InstantPrivacy service extraction specs/001-instant-privacy-service/quickstart.md Created step-by-step implementation guide for InstantPrivacy service extraction Provided code examples for service creation, test data builder, and provider refactoring Included verification commands for architecture compliance checks Documented file summary and success criteria verification procedures +232/-0  spec.md Add feature specification for InstantTopology service extraction specs/001-instant-topology-service/spec.md Created feature specification for InstantTopology service extraction Defined 3 user stories (reboot, factory reset, LED blink) with acceptance scenarios Specified 12 functional requirements covering service creation, error handling, and architecture compliance Documented 7 success criteria for measurable outcomes Included edge cases and clarifications from specification sessions +119/-0  research.md Add research document for InstantTopology service extraction specs/001-instant-topology-service/research.md Created research document analyzing existing service patterns and design decisions Documented decision to follow RouterPasswordService pattern as reference implementation Specified 3 new ServiceError subtypes with implementation patterns Analyzed 7 JNAP actions to extract to service layer Resolved technical unknowns regarding polling logic, SharedPreferences handling, and test data patterns +225/-0  requirements.md Add specification quality checklist for InstantPrivacy service specs/001-instant-privacy-service/checklists/requirements.md Created specification quality checklist validating completeness and readiness Verified all mandatory sections present and filled Confirmed no implementation details leak into specification Validated all requirements are testable and success criteria are measurable Confirmed specification ready for planning phase +60/-0    instant_privacy_service_contract.md InstantPrivacyService API contract and JNAP communication specification specs/001-instant-privacy-service/contracts/instant_privacy_service_contract.md Defines complete API contract for InstantPrivacyService with 4 public methods: fetchMacFilterSettings(), saveMacFilterSettings(), fetchStaBssids(), and fetchMyMacAddress() Documents JNAP action mappings, data transformations between JNAP models and UI models, and error handling strategy Provides usage examples showing integration with InstantPrivacyNotifier and dependency injection pattern Specifies test strategy and dependencies including RouterRepository, JNAPAction, and ServiceError +243/-0  quickstart.md InstantTopologyService extraction developer quickstart guide specs/001-instant-topology-service/quickstart.md Provides step-by-step implementation guide for extracting InstantTopologyService from InstantTopologyNotifier Details 4 implementation steps: adding ServiceError types, creating service class, updating provider, and writing tests Includes verification checklist with bash commands to validate architecture compliance References existing patterns and common pitfalls to avoid during implementation +229/-0  data-model.md InstantPrivacyService data model and transformation specifications specs/001-instant-privacy-service/data-model.md Documents entity diagram showing JNAP layer models transforming to application layer UI models Specifies data transformations for fetch (JNAP → UI) and save (UI → JNAP) operations Lists existing models (MacFilterMode, InstantPrivacySettings, InstantPrivacyStatus) that require no changes Defines validation rules and state transition diagram for MAC filter modes +171/-0  data-model.md InstantTopologyService error types and data flow architecture specs/001-instant-topology-service/data-model.md Defines 3 new ServiceError subtypes: TopologyTimeoutError, NodeOfflineError, and NodeOperationFailedError with detailed field descriptions Documents entity relationships showing Provider → Service → Data layer architecture Illustrates data flows for reboot and LED blink operations with sequence diagrams Specifies validation rules for device UUIDs, device IDs, timeouts, and operation names +183/-0  plan.md InstantTopologyService implementation plan and architecture compliance specs/001-instant-topology-service/plan.md Outlines implementation plan for extracting JNAP communication from InstantTopologyNotifier into InstantTopologyService Provides constitution compliance checklist verifying alignment with Articles I, III, V, VI, IX, and XIII Specifies project structure with new service file location and test organization Lists generated artifacts including plan, research, data model, contract, and quickstart documents +118/-0  research.md InstantPrivacyService design decisions and research findings specs/001-instant-privacy-service/research.md Documents 5 key design decisions: service method signatures, error mapping strategy, capability checks, device manager access patterns, and test data builders Provides rationale for each decision with alternatives considered and rejected References RouterPasswordService as implementation pattern to follow Confirms all research items resolved with no blocking unknowns +98/-0    plan.md InstantPrivacyService implementation plan and constitution compliance specs/001-instant-privacy-service/plan.md Defines implementation plan for extracting InstantPrivacyService from InstantPrivacyNotifier to enforce three-layer architecture Includes constitution compliance gate checks for Articles I, III, V, VI, VIII, and XIII Specifies project structure with new service file, test organization, and test data builder locations Confirms no complexity violations and refactoring follows standard patterns +84/-0    requirements.md InstantTopologyService specification quality validation checklist specs/001-instant-topology-service/checklists/requirements.md Validates specification completeness with checklist covering content quality, requirement completeness, and feature readiness Confirms all mandatory sections completed and no clarification markers remain Verifies requirements are testable, unambiguous, and technology-agnostic Notes specification is ready for planning phase and focuses on architectural compliance +38/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive logging exposure Description: Verbose debug logging during waitForNodesOffline includes device.getDeviceLocation() and online status, which may expose sensitive network/topology identifiers (e.g., device names/locations and potentially derived identifiers) into logs if enabled in production builds. instant_topology_service.dart [211-220] Referred Code logger.d('[Reboot/FactoryReset]: Waiting for all nodes offline'); if (result is JNAPSuccess) { final deviceList = List.from(result.output['devices']) .map((e) => LinksysDevice.fromMap(e)) .where((device) => deviceUUIDs.contains(device.deviceID)) .toList(); for (final device in deviceList) { logger.d( '[Reboot/FactoryReset]: Waiting for - isDevice<${device.getDeviceLocation()}> Online - ${device.isOnline()}'); }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🔴	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Misspelled identifier: The test suite references MacFilterMode.reslove, which appears to be a misspelling and undermines self-documenting code and discoverability. Referred Code group('reslove', () { test('resolves "disabled" to MacFilterMode.disabled', () { expect(MacFilterMode.reslove('disabled'), MacFilterMode.disabled); }); test('resolves "allow" to MacFilterMode.allow', () { expect(MacFilterMode.reslove('allow'), MacFilterMode.allow); }); test('resolves "deny" to MacFilterMode.deny', () { expect(MacFilterMode.reslove('deny'), MacFilterMode.deny); }); test('resolves case-insensitively', () { expect(MacFilterMode.reslove('DISABLED'), MacFilterMode.disabled); expect(MacFilterMode.reslove('Allow'), MacFilterMode.allow); expect(MacFilterMode.reslove('DENY'), MacFilterMode.deny); expect(MacFilterMode.reslove('DiSaBlEd'), MacFilterMode.disabled); }); test('returns disabled for unknown values', () { ... (clipped 5 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Unhandled malformed output: waitForNodesOffline assumes result.output['devices'] exists and is iterable, so malformed/partial JNAP outputs can throw runtime exceptions instead of returning a meaningful ServiceError or handling the edge case gracefully. Referred Code Future<void> waitForNodesOffline( List<String> deviceUUIDs, { int maxRetry = 20, int retryDelayMs = 3000, }) async { final waitingStream = _routerRepository.scheduledCommand( action: JNAPAction.getDevices, retryDelayInMilliSec: retryDelayMs, maxRetry: maxRetry, condition: (result) { if (result is JNAPSuccess) { final deviceList = List.from(result.output['devices']) .map((e) => LinksysDevice.fromMap(e)) .where((device) => deviceUUIDs.contains(device.deviceID)) .toList(); return !deviceList.any((device) => device.isOnline()); } return false; }, auth: true, Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Sensitive unstructured logs: Debug logs are unstructured and include device-identifying information (e.g., device.getDeviceLocation() and online status), which can leak sensitive topology details into logs. Referred Code logger.d('[Reboot/FactoryReset]: Waiting for all nodes offline'); if (result is JNAPSuccess) { final deviceList = List.from(result.output['devices']) .map((e) => LinksysDevice.fromMap(e)) .where((device) => deviceUUIDs.contains(device.deviceID)) .toList(); for (final device in deviceList) { logger.d( '[Reboot/FactoryReset]: Waiting for - isDevice<${device.getDeviceLocation()}> Online - ${device.isOnline()}'); } Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit context: Critical actions (reboot/factory reset/LED blink) are executed without an explicit audit trail that records who initiated the action, timestamp, action details, and outcome in a reconstructable manner. Referred Code Future<void> rebootNodes(List<String> deviceUUIDs) async { try { if (deviceUUIDs.isEmpty) { // Master node only await _routerRepository.send( JNAPAction.reboot, fetchRemote: true, cacheLevel: CacheLevel.noCache, auth: true, ); } else { // Child nodes - build transaction final builder = JNAPTransactionBuilder( commands: deviceUUIDs.reversed .map((uuid) => MapEntry(JNAPAction.reboot2, {'deviceUUID': uuid})) .toList(), auth: true, ); await _routerRepository.transaction( builder, fetchRemote: true, ... (clipped 102 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error exposure risk: Topology errors wrap and carry the originalError (JNAPError), which may contain internal details and could be exposed to user-facing surfaces depending on how ServiceError is rendered upstream. Referred Code /// Maps JNAP errors to ServiceError for topology operations. /// /// Uses the centralized mapper from `jnap_error_mapper.dart` for common errors, /// with topology-specific handling for operation failures. ServiceError _mapJnapError( JNAPError error, String deviceId, String operation) { // For common errors, use centralized mapper final commonError = mapJnapErrorToServiceError(error); if (commonError is! UnexpectedError) { return commonError; } // For topology-specific errors return NodeOperationFailedError( deviceId: deviceId, operation: operation, originalError: error, ); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Missing input validation: Public service methods accept external identifiers (e.g., deviceId/deviceUUIDs) without validation/sanitization (empty/invalid format), and it is not verifiable from the diff whether upstream layers enforce these constraints. Referred Code Future<void> rebootNodes(List<String> deviceUUIDs) async { try { if (deviceUUIDs.isEmpty) { // Master node only await _routerRepository.send( JNAPAction.reboot, fetchRemote: true, cacheLevel: CacheLevel.noCache, auth: true, ); } else { // Child nodes - build transaction final builder = JNAPTransactionBuilder( commands: deviceUUIDs.reversed .map((uuid) => MapEntry(JNAPAction.reboot2, {'deviceUUID': uuid})) .toList(), auth: true, ); await _routerRepository.transaction( builder, fetchRemote: true, ... (clipped 102 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle master node status check correctly Update the condition in waitForNodesOffline to correctly check the master node's status when deviceUUIDs is empty, preventing premature termination of the polling logic. lib/page/instant_topology/services/instant_topology_service.dart [187-208] Future<void> waitForNodesOffline( List<String> deviceUUIDs, { int maxRetry = 20, int retryDelayMs = 3000, }) async { final waitingStream = _routerRepository.scheduledCommand( action: JNAPAction.getDevices, retryDelayInMilliSec: retryDelayMs, maxRetry: maxRetry, condition: (result) { if (result is JNAPSuccess) { - final deviceList = List.from(result.output['devices']) + final allDevices = List.from(result.output['devices']) .map((e) => LinksysDevice.fromMap(e)) + .toList(); + + if (deviceUUIDs.isEmpty) { + // Master node check + final master = allDevices.getMaster(); + return master != null && !master.isOnline(); + } + + final deviceList = allDevices .where((device) => deviceUUIDs.contains(device.deviceID)) .toList(); return !deviceList.any((device) => device.isOnline()); } return false; }, auth: true, ); ... [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: This suggestion identifies a critical bug in waitForNodesOffline where an empty deviceUUIDs list causes the polling to terminate prematurely, making the fix for master node reboots/resets ineffective.	High
	Correct typo in method name Correct the typo in the method name from reslove to resolve. lib/page/instant_privacy/services/instant_privacy_service.dart [61] -final mode = MacFilterMode.reslove(settings.macFilterMode); +final mode = MacFilterMode.resolve(settings.macFilterMode); [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a typo reslove that would cause a NoSuchMethodError at runtime, which is a critical bug.	High
	Wait for master node offline after reboot In rebootNodes, add a call to waitForNodesOffline after rebooting the master node to ensure consistent behavior and prevent race conditions. lib/page/instant_topology/services/instant_topology_service.dart [47-76] Future<void> rebootNodes(List<String> deviceUUIDs) async { try { if (deviceUUIDs.isEmpty) { // Master node only await _routerRepository.send( JNAPAction.reboot, fetchRemote: true, cacheLevel: CacheLevel.noCache, auth: true, ); + // Wait for master node to go offline + await waitForNodesOffline([]); } else { // Child nodes - build transaction final builder = JNAPTransactionBuilder( commands: deviceUUIDs.reversed .map((uuid) => MapEntry(JNAPAction.reboot2, {'deviceUUID': uuid})) .toList(), auth: true, ); await _routerRepository.transaction( builder, fetchRemote: true, cacheLevel: CacheLevel.noCache, ); // Wait for nodes to go offline await waitForNodesOffline(deviceUUIDs); } } on JNAPError catch (e) { throw _mapJnapError(e, deviceUUIDs.firstOrNull ?? 'master', 'reboot'); } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a logical inconsistency where the master node reboot does not wait for the node to go offline, unlike the multi-node case, which could lead to race conditions.	Medium
	Wait for master node offline after reset In factoryResetNodes, add a call to waitForNodesOffline after resetting the master node to ensure consistent behavior and prevent race conditions. lib/page/instant_topology/services/instant_topology_service.dart [94-125] Future<void> factoryResetNodes(List<String> deviceUUIDs) async { try { if (deviceUUIDs.isEmpty) { // Master node only await _routerRepository.send( JNAPAction.factoryReset, fetchRemote: true, cacheLevel: CacheLevel.noCache, auth: true, ); + // Wait for master node to go offline + await waitForNodesOffline([]); } else { // Child nodes - build transaction (reverse order for bottom-up reset) final builder = JNAPTransactionBuilder( commands: deviceUUIDs.reversed .map((uuid) => MapEntry(JNAPAction.factoryReset2, {'deviceUUID': uuid})) .toList(), auth: true, ); await _routerRepository.transaction( builder, fetchRemote: true, cacheLevel: CacheLevel.noCache, ); // Wait for nodes to go offline await waitForNodesOffline(deviceUUIDs); } } on JNAPError catch (e) { throw _mapJnapError( e, deviceUUIDs.firstOrNull ?? 'master', 'factoryReset'); } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a logical inconsistency where the master node factory reset does not wait for the node to go offline, unlike the multi-node case, which could lead to race conditions.	Medium
	Add error handling for service calls Add a try-catch block to toggleBlinkNode to handle ServiceError from service calls, preventing unhandled exceptions and ensuring state consistency. lib/page/instant_topology/providers/instant_topology_provider.dart [236-245] Future<void> toggleBlinkNode(String deviceId, [bool stopOnly = false]) async { final service = ref.read(instantTopologyServiceProvider); final prefs = await SharedPreferences.getInstance(); final blinkDevice = prefs.get(pBlinkingNodeId); - if (blinkDevice != null && deviceId != blinkDevice) { - await service.stopBlinkNodeLED(); + + try { + if (blinkDevice != null && deviceId != blinkDevice) { + await service.stopBlinkNodeLED(); + } + await service.startBlinkNodeLED(deviceId); + await prefs.setString(pBlinkingNodeId, deviceId); + } on ServiceError catch (e) { + logger.e('Blink operation failed: $e'); + // Optionally, rethrow or handle the error to notify the UI } - await service.startBlinkNodeLED(deviceId); - await prefs.setString(pBlinkingNodeId, deviceId); } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the refactored toggleBlinkNode method fails to handle ServiceError, which contradicts the new architectural pattern and can lead to unhandled exceptions, as documented in the PR's own contract files.	Medium
	Prevent returning incomplete cache data Add a check in parseCacheData to ensure the number of valid cache entries matches the number of commands, preventing the return of a partial dataset. lib/core/jnap/services/polling_service.dart [137-156] Map<JNAPAction, JNAPResult>? parseCacheData({ required Map<String, dynamic> cache, required List<MapEntry<JNAPAction, Map<String, dynamic>>> commands, }) { - final checkCacheDataList = commands - .where((command) => cache.keys.contains(command.key.actionValue)); + final checkCacheDataList = + commands.where((command) => cache.keys.contains(command.key.actionValue)); // Have not done any polling yet - cache incomplete if (checkCacheDataList.length != commands.length) return null; final cacheDataList = checkCacheDataList .where((command) => cache[command.key.actionValue]['data'] != null) .map((command) => MapEntry( command.key, JNAPSuccess.fromJson(cache[command.key.actionValue]['data']), )) .toList(); + // Ensure all commands had valid data, not just keys. + if (cacheDataList.length != commands.length) return null; + return Map.fromEntries(cacheDataList); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a subtle bug where the function could return incomplete data, potentially causing null exceptions downstream. The fix ensures data integrity.	Medium
	Add missing field to error class Add the deviceIds field to the TopologyTimeoutError class in research.md to align with definitions in other specification documents and improve error details. specs/001-instant-topology-service/research.md [50-54] // New errors following existing pattern final class TopologyTimeoutError extends ServiceError { final Duration timeout; - const TopologyTimeoutError({required this.timeout}); + final List<String> deviceIds; + const TopologyTimeoutError({required this.timeout, required this.deviceIds}); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an inconsistency in the TopologyTimeoutError definition across different specification files, and the proposed fix improves the documentation's quality and potential for debugging.	Low
General	Implement stopOnly flag handling Implement the logic for the stopOnly flag in toggleBlinkNode to stop the blinking LED and clear the corresponding state. lib/page/instant_topology/providers/instant_topology_provider.dart [236-245] Future<void> toggleBlinkNode(String deviceId, [bool stopOnly = false]) async { final service = ref.read(instantTopologyServiceProvider); final prefs = await SharedPreferences.getInstance(); - final blinkDevice = prefs.get(pBlinkingNodeId); + if (stopOnly) { + await service.stopBlinkNodeLED(); + await prefs.remove(pBlinkingNodeId); + return; + } + final blinkDevice = prefs.getString(pBlinkingNodeId); if (blinkDevice != null && deviceId != blinkDevice) { await service.stopBlinkNodeLED(); } await service.startBlinkNodeLED(deviceId); await prefs.setString(pBlinkingNodeId, deviceId); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the stopOnly parameter is ignored and provides a correct implementation to handle it, fixing a feature bug in the new code.	Medium
	Improve error handling and logging Replace the .onError call in fetchMyMacAddress with a try-catch block to explicitly log errors before returning null, improving debuggability. lib/page/instant_privacy/services/instant_privacy_service.dart [161-172] -Future<String?> fetchMyMacAddress(List<LinksysDevice> deviceList) { - return _routerRepository - .send(JNAPAction.getLocalDevice, auth: true, fetchRemote: true) - .then((result) { +Future<String?> fetchMyMacAddress(List<LinksysDevice> deviceList) async { + try { + final result = await _routerRepository.send(JNAPAction.getLocalDevice, + auth: true, fetchRemote: true); final deviceID = result.output['deviceID']; return deviceList .firstWhereOrNull((device) => device.deviceID == deviceID) ?.getMacAddress(); - }).onError((_, __) { + } catch (e) { + logger.d('Failed to fetch local device MAC address: $e'); return null; - }); + } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly advocates for explicit try-catch with logging over a silent .onError, which improves debuggability and aligns better with robust error handling practices.	Low
	Avoid unnecessary provider data access Move the retrieval of deviceList inside the if (settings != null) block to avoid an unnecessary provider read when updateStatusOnly is true. specs/001-instant-privacy-service/quickstart.md [143-162] @override Future<(InstantPrivacySettings?, InstantPrivacyStatus?)> performFetch({ bool forceRemote = false, bool updateStatusOnly = false, }) async { final service = ref.read(instantPrivacyServiceProvider); - final deviceList = ref.read(deviceManagerProvider).deviceList; final (settings, status) = await service.fetchMacFilterSettings( forceRemote: forceRemote, updateStatusOnly: updateStatusOnly, ); if (settings != null) { + final deviceList = ref.read(deviceManagerProvider).deviceList; final myMac = await service.fetchMyMacAddress(deviceList); return (settings.copyWith(myMac: myMac), status); } return (settings, status); } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out a minor performance optimization by avoiding an unnecessary provider read when it's not needed, improving the code's efficiency.	Low
More

[qodo-code-review]

Suggestion: Correct typo in method name

Suggested change

	final mode = MacFilterMode.reslove(settings.macFilterMode);
	final mode = MacFilterMode.resolve(settings.macFilterMode);