refactor(auto-parent): Extract AutoParentFirstLoginService from Notifier

[HankYuLinksys]

Summary

• Create AutoParentFirstLoginService with JNAP communication logic:
  • setUserAcknowledgedAutoConfiguration(): await JNAP response (bug fix)
  • setFirmwareUpdatePolicy(): await SET operation (bug fix)
  • checkInternetConnection(): retry logic with ExponentialBackoffRetryStrategy
• Refactor AutoParentFirstLoginNotifier to delegate to Service
• Remove JNAP imports from Provider layer (architecture compliance)
• Add comprehensive test coverage:
  • Service tests: 9 tests, 96.8% coverage
  • Provider tests: 8 tests, 100% coverage
  • State tests: 3 tests, 100% coverage
• Add test data builder: AutoParentFirstLoginTestData
• Add spec documentation and service contract

Architecture Compliance

Follows constitution.md:

• Article V: Three-layer architecture (Presentation → Provider/Service → Data)
• Article VI: Services handle JNAP communication, return simple results
• Article XIII: JNAPError → ServiceError conversion via mapJnapErrorToServiceError()

Bug Fixes

Original code had missing await on JNAP operations:

• setUserAcknowledgedAutoConfiguration() - now properly awaits response
• setFirmwareUpdatePolicy() SET operation - now properly awaits response

Test Plan

• All new tests pass (flutter test)
• Coverage requirements met (Service ≥90%, Provider ≥85%)
• flutter analyze passes

Dependencies

This PR depends on #564 (001-add-nodes-service)

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive data in logs Description: Possible sensitive information exposure via verbose logging of onboarding device identifiers (e.g., MAC list and device.getDeviceLocation()), which could leak private network/device metadata into application logs if logs are collected or accessible in production. add_nodes_service.dart [164-205] Referred Code logger.d( '[AddNodesService]: [pollForNodesOnline] Start by MACs: $onboardedMACList'); return _routerRepository .scheduledCommand( action: JNAPAction.getDevices, firstDelayInMilliSec: refreshing ? 1000 : 20000, retryDelayInMilliSec: refreshing ? 3000 : 20000, // Basic 3 minutes, add 2 minutes for each one more node maxRetry: refreshing ? 5 : 9 + onboardedMACList.length * 6, auth: true, condition: (result) { if (result is JNAPSuccess) { final deviceList = List.from(result.output['devices']) .map((e) => LinksysDevice.fromMap(e)) .where((device) => device.isAuthority == false) .toList(); // Check all MAC addresses can be found in the device list bool allFound = onboardedMACList.every((mac) => deviceList.any( (device) => ... (clipped 21 lines)
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: Passed 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: Timestamp parse edge: The new timestamp parsing uses a 12-hour format pattern ("yyyy-MM-ddThh:mm:ssZ") which can mis-parse/return null for ISO timestamps and cause incorrect new-node detection behavior. Referred Code final now = DateTime.now(); final dateFormat = DateFormat("yyyy-MM-ddThh:mm:ssZ"); bool anyOnboarded = false; return _routerRepository .scheduledCommand( action: JNAPAction.getBackhaulInfo, auth: true, firstDelayInMilliSec: 1 * 1000, retryDelayInMilliSec: 10 * 1000, maxRetry: refreshing ? 6 : 60, condition: (result) { if (result is! JNAPSuccess) { return false; } // Keep polling until timeout - no early termination return false; }, onCompleted: (_) { logger.i('[AddWiredNodesService]: poll backhaul info is completed'); }, ... (clipped 96 lines) 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 data in logs: The new/retained debug logs print device payloads (toJson()), MAC lists, and potentially identifiers which can expose sensitive device/network data in application logs. Referred Code '[AddNodes]: Number of onboarded MAC addresses = ${onboardedMACList.length}'); List<LinksysDevice> addedDevices = []; List<LinksysDevice> childNodes = []; List<LinksysDevice> childNodesWithBackhaul = []; state = state.copyWith(isLoading: true, loadingMessage: 'onboarding'); if (onboardingProceed && anyOnboarded) { await for (final result in _service.pollForNodesOnline(onboardedMACList)) { childNodes = result.where((element) => element.nodeType != null).toList(); addedDevices = result .where( (element) => element.nodeType == 'Slave' && (element.knownInterfaces?.any((knownInterface) => onboardedMACList .contains(knownInterface.macAddress)) ?? false), ) .toList(); logger.d( ... (clipped 3 lines) 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: The new service layer performs critical router configuration/actions but the added logs do not include audit-trail essentials (user identity, consistent timestamp, action outcome) needed to reconstruct events. Referred Code Future<void> setAutoOnboardingSettings() async { try { await _routerRepository.send( JNAPAction.setBluetoothAutoOnboardingSettings, data: {'isAutoOnboardingEnabled': true}, auth: true, ); } on JNAPError catch (e) { throw mapJnapErrorToServiceError(e); } } /// Fetches current Bluetooth auto-onboarding setting /// /// Returns: /// - `true` if auto-onboarding is enabled /// - `false` if disabled or not configured /// /// Throws: /// - [NetworkError] if router communication fails /// - [UnauthorizedError] if authentication expired ... (clipped 83 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 surfaced unclear: The new services convert and throw ServiceErrors but the diff does not show whether these errors are presented to users in a generic manner versus leaking internal details. Referred Code Future<void> setAutoOnboardingEnabled(bool enabled) async { try { await _routerRepository.send( JNAPAction.setWiredAutoOnboardingSettings, data: {'isAutoOnboardingEnabled': enabled}, auth: true, ); } on JNAPError catch (e) { throw mapJnapErrorToServiceError(e); } } /// Fetches current wired auto-onboarding setting /// /// Returns: /// - `true` if auto-onboarding is enabled /// - `false` if disabled or not configured /// /// Throws: /// - [NetworkError] if router communication fails /// - [UnauthorizedError] if authentication expired ... (clipped 12 lines) 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: External data assumptions: The new service code assumes JNAP response shapes (e.g., result.output['devices']) without explicit validation/guards in the diff, and compliance depends on upstream guarantees not visible here. Referred Code condition: (result) { if (result is JNAPSuccess) { final deviceList = List.from(result.output['devices']) .map((e) => LinksysDevice.fromMap(e)) .where((device) => device.isAuthority == false) .toList(); // Check all MAC addresses can be found in the device list bool allFound = onboardedMACList.every((mac) => deviceList.any( (device) => device.nodeType == 'Slave' && (device.knownInterfaces?.any((knownInterface) => knownInterface.macAddress == mac) ?? false))); logger.d( '[AddNodesService]: [pollForNodesOnline] are All MACs in device list? $allFound'); // Check if nodes in the mac list all have connections bool ret = deviceList .where((device) => ... (clipped 34 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	Fix bug in new node detection Correct the logic in _isExistingWiredNode to accurately detect new or updated wired nodes by checking if the new timestamp is greater than the snapshot's timestamp. lib/page/nodes/services/add_wired_nodes_service.dart [187-224] bool _isExistingWiredNode( BackHaulInfoData infoData, List<BackhaulInfoUIModel> snapshot, DateTime pollStartTime, DateFormat dateFormat, ) { - // Check if this device was in the snapshot + // Find the corresponding entry in the original snapshot final snapshotEntry = snapshot.firstWhere( (e) => e.deviceUUID == infoData.deviceUUID, orElse: () => const BackhaulInfoUIModel( deviceUUID: '', connectionType: '', timestamp: '', ), ); - // If not found in snapshot, it's new + // If not found in snapshot, it cannot be an existing node. if (snapshotEntry.deviceUUID.isEmpty) { return false; } - // Must be wired connection + // A node is considered existing if its connection type is Wired and its + // timestamp has not been updated since the snapshot was taken. if (infoData.connectionType != 'Wired') { return false; } final infoTimestamp = dateFormat.tryParse(infoData.timestamp)?.millisecondsSinceEpoch ?? 0; final snapshotTimestamp = dateFormat.tryParse(snapshotEntry.timestamp)?.millisecondsSinceEpoch ?? 0; - final nowTimestamp = pollStartTime.millisecondsSinceEpoch; - // Check timestamps: - // - Info timestamp must be before poll start (now) - // - Info timestamp must be same or earlier than snapshot timestamp - return nowTimestamp > infoTimestamp && infoTimestamp <= snapshotTimestamp; + // If the timestamp is the same or older, it's an existing node. + return infoTimestamp <= snapshotTimestamp; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a logic bug in _isExistingWiredNode that deviates from the intended behavior of detecting newly connected wired nodes, which is a critical part of the feature.	Medium
	Enforce required fields during model deserialization Modify the BackhaulInfoUIModel.fromMap factory to throw an ArgumentError if required fields (deviceUUID, connectionType, timestamp) are missing, instead of defaulting to empty strings. This ensures data integrity and fail-fast behavior. lib/page/nodes/models/backhaul_info_ui_model.dart [29-34] -factory BackhaulInfoUIModel.fromMap(Map<String, dynamic> map) => - BackhaulInfoUIModel( - deviceUUID: map['deviceUUID'] ?? '', - connectionType: map['connectionType'] ?? '', - timestamp: map['timestamp'] ?? '', - ); +factory BackhaulInfoUIModel.fromMap(Map<String, dynamic> map) { + if (map['deviceUUID'] == null || + map['connectionType'] == null || + map['timestamp'] == null) { + throw ArgumentError( + 'BackhaulInfoUIModel.fromMap: required fields are missing in the provided map.'); + } + return BackhaulInfoUIModel( + deviceUUID: map['deviceUUID'], + connectionType: map['connectionType'], + timestamp: map['timestamp'], + ); +} Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that defaulting to empty strings can hide data integrity issues. Enforcing required fields by throwing an error improves the model's robustness and prevents the propagation of invalid data, which is a significant improvement.	Medium
	Validate required fields in fromMap In the BackhaulInfoUIModel.fromMap factory, throw an ArgumentError if the deviceUUID field is null instead of defaulting it to an empty string to ensure data integrity. specs/001-add-nodes-service/data-model.md [186-190] -factory BackhaulInfoUIModel.fromMap(Map<String, dynamic> map) => BackhaulInfoUIModel( - deviceUUID: map['deviceUUID'] ?? '', - connectionType: map['connectionType'] ?? '', - timestamp: map['timestamp'] ?? '', -); +factory BackhaulInfoUIModel.fromMap(Map<String, dynamic> map) { + if (map['deviceUUID'] == null) { + throw ArgumentError.notNull('deviceUUID'); + } + return BackhaulInfoUIModel( + deviceUUID: map['deviceUUID'], + connectionType: map['connectionType'] ?? '', + timestamp: map['timestamp'] ?? '', + ); +} Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that defaulting a required field like deviceUUID to an empty string can hide data issues and proposes a more robust validation by throwing an error, which is a good design practice.	Medium
	Guard internet connection result In finishFirstTimeLogin, add a check on the isConnected result. If the connection check returns false, throw an exception to prevent proceeding with user acknowledgment and to surface the error instead of silently failing. lib/page/login/auto_parent/providers/auto_parent_first_login_provider.dart [39-51] Future<void> finishFirstTimeLogin([bool failCheck = false]) async { final service = ref.read(autoParentFirstLoginServiceProvider); - // Keep userAcknowledgedAutoConfiguration to false if check firmware failed if (!failCheck) { - // wait for internet connection final isConnected = await service.checkInternetConnection(); logger.i('[FirstTime]: Internet connection status: $isConnected'); + if (!isConnected) { + throw Exception('Cannot finish login: no internet connection'); + } await service.setUserAcknowledgedAutoConfiguration(); } - // Set firmware update policy await service.setFirmwareUpdatePolicy(); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that the logic should not proceed if the internet connection check fails. This improves the robustness of the login flow by preventing subsequent operations from running in a disconnected state.	Low
General	Avoid overwriting data with null values In collectChildNodeData, avoid overwriting wirelessConnectionInfo with null by conditionally calling copyWith to preserve existing device data. lib/page/nodes/services/add_nodes_service.dart [295-320] +List<LinksysDevice> collectChildNodeData( + List<LinksysDevice> childNodes, + List<BackHaulInfoData> backhaulInfoList, +) { + childNodes.sort((a, b) => a.isAuthority ? -1 : 1); + final newChildNodes = childNodes.map((e) { + final target = + backhaulInfoList.firstWhereOrNull((d) => d.deviceUUID == e.deviceID); + if (target != null) { + return e.copyWith( + wirelessConnectionInfo: target.wirelessConnectionInfo, + connectionType: target.connectionType, + ); + } else { + return e; + } + }).toList(); + return newChildNodes; +} - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that copyWith might overwrite existing data with null, which is a valid concern for data integrity, although the existing_code and improved_code are identical.	Medium
	Use DateTime.parse for timestamps Replace DateFormat.tryParse with the more reliable DateTime.parse for handling ISO-8601 timestamp strings to avoid potential parsing errors. lib/page/nodes/services/add_wired_nodes_service.dart [213-217] -final infoTimestamp = - dateFormat.tryParse(infoData.timestamp)?.millisecondsSinceEpoch ?? 0; -final snapshotTimestamp = - dateFormat.tryParse(snapshotEntry.timestamp)?.millisecondsSinceEpoch ?? - 0; +final infoTimestamp = DateTime.parse(infoData.timestamp).millisecondsSinceEpoch; +final snapshotTimestamp = DateTime.parse(snapshotEntry.timestamp).millisecondsSinceEpoch; Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion improves the robustness of timestamp parsing by using the standard DateTime.parse for ISO-8601 strings, preventing potential format-related bugs.	Medium
	Improve error handling for connection checks Refactor checkInternetConnection to use a try-catch block for more granular error handling. Differentiate between retryable JNAPErrors and other exceptions, re-throwing non-retryable errors as ServiceError instead of silently returning false. lib/page/login/auto_parent/services/auto_parent_first_login_service.dart [130-143] return retryStrategy.execute<bool>(() async { - final result = await _routerRepository.send( - JNAPAction.getInternetConnectionStatus, - fetchRemote: true, - auth: true, - ); - logger.i('[FirstTime]: Internet connection status: ${result.output}'); - final connectionStatus = result.output['connectionStatus']; - return connectionStatus == 'InternetConnected'; -}, shouldRetry: (result) => !result).onError((error, stackTrace) { + try { + final result = await _routerRepository.send( + JNAPAction.getInternetConnectionStatus, + fetchRemote: true, + auth: true, + ); + logger.i('[FirstTime]: Internet connection status: ${result.output}'); + final connectionStatus = result.output['connectionStatus']; + return connectionStatus == 'InternetConnected'; + } on JNAPError catch (e) { + // For expected transient errors (e.g., timeouts), we can return false to allow retries. + // For other errors, we should propagate them. + if (e.isTimeout) { + logger.w('[FirstTime]: Timeout checking internet connection, will retry.'); + return false; + } + // Re-throw other JNAP errors to be caught outside the retry strategy. + rethrow; + } +}, shouldRetry: (result) => !result).catchError((error, stackTrace) { + // Catch errors that were re-thrown from within the retry block. logger.e('[FirstTime]: Error checking internet connection: $error'); + if (error is JNAPError) { + throw mapJnapErrorToServiceError(error); + } + // For other unexpected errors, return false to maintain original behavior, + // but ideally these should also be propagated. return false; }); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that swallowing all errors is poor practice and proposes a more robust error handling strategy that distinguishes between retryable and fatal errors, which aligns with the PR's goal of creating a reliable service layer.	Medium
	Refactor error fallback for clarity In setFirmwareUpdatePolicy, replace the .onError fluent chain with a try-catch block. This ensures that only JNAPError is caught to trigger the fallback to default settings, while other unexpected errors are allowed to propagate. lib/page/login/auto_parent/services/auto_parent_first_login_service.dart [71-92] -// Get current firmware update settings -final firmwareUpdateSettings = await _routerRepository - .send( - JNAPAction.getFirmwareUpdateSettings, - fetchRemote: true, - auth: true, - ) - .then((value) => value.output) - .then( - (output) => FirmwareUpdateSettings.fromMap(output).copyWith( - updatePolicy: FirmwareUpdateSettings.firmwareUpdatePolicyAuto), - ) - .onError((error, stackTrace) { - // Use default settings on fetch failure - return FirmwareUpdateSettings( +FirmwareUpdateSettings firmwareUpdateSettings; +try { + final result = await _routerRepository.send( + JNAPAction.getFirmwareUpdateSettings, + fetchRemote: true, + auth: true, + ); + firmwareUpdateSettings = FirmwareUpdateSettings + .fromMap(result.output) + .copyWith( + updatePolicy: + FirmwareUpdateSettings.firmwareUpdatePolicyAuto, + ); +} on JNAPError catch (_) { + firmwareUpdateSettings = FirmwareUpdateSettings( updatePolicy: FirmwareUpdateSettings.firmwareUpdatePolicyAuto, autoUpdateWindow: FirmwareAutoUpdateWindow( startMinute: 0, durationMinutes: 240, ), ); -}); +} Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the .onError chain swallows all errors. Refactoring to a try/catch block for JNAPError specifically makes the error handling more precise and robust, preventing unexpected errors from being silently ignored.	Medium
	Use explicit type casting In the fromMap factory of AddWiredNodesState, add explicit casts for map['backhaulSnapshot'] to List and for each element to Map<String, dynamic> to ensure type safety during deserialization. lib/page/nodes/providers/add_wired_nodes_state.dart [69-72] backhaulSnapshot: map['backhaulSnapshot'] != null - ? List<BackhaulInfoUIModel>.from(map['backhaulSnapshot'] - ?.map((x) => BackhaulInfoUIModel.fromMap(x))) + ? (map['backhaulSnapshot'] as List<dynamic>) + .map((e) => BackhaulInfoUIModel.fromMap( + e as Map<String, dynamic>)) + .toList() : null, Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion improves type safety during deserialization by adding explicit casts. This is a good practice that makes the code more robust against potential runtime type errors, especially in Dart's strong mode.	Low
	Specify correct async return type Change the return type of getAutoOnboardingStatus from FutureOr to the more specific Future<Map<String, dynamic>> for better type safety and clarity. lib/page/nodes/providers/add_nodes_provider.dart [29-31] -FutureOr getAutoOnboardingStatus() { +Future<Map<String, dynamic>> getAutoOnboardingStatus() { return _service.pollAutoOnboardingStatus(oneTake: true).first; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 4 __ Why: The suggestion correctly improves type safety and code clarity by specifying a more precise return type, which is good practice but has a minor impact.	Low
More