Improve Mopar UX, diagnostics, and timers by frankea · Pull Request #4 · frankea/homebridge-mopar

frankea · 2025-11-21T23:14:20Z

Summary

enforce mandatory PINs with better UX, guard commands, and align docs
refactor lock service/timer handling, contact sensors, rate limiter, and background tasks
modularize MoparAuth with sanitized diagnostics and add tests + README/CHANGELOG updates

All tests:

homebridge-mopar@0.9.14-beta.2 test
jest

- Fixed formatting issues in src/platform.test.js - Fixed formatting issues in src/integration.test.js - Fixed unused variable warning by adding assertion - All linting checks now passing

Fixed Puppeteer evaluate() mocking: - Changed from sequential mockResolvedValueOnce chain to smart mockImplementation - Mock now inspects function content and returns appropriate values - Handles all evaluate() calls in auth.js (email fill, password fill, validation, button clicks, Gigya API, etc.) Fixed error recovery test: - Changed from testing network error retry (not implemented) to empty response retry (actual behavior) - Test now matches actual getVehicles() retry logic Result: All 127 tests passing - ✅ 48 platform tests - ✅ 40 API tests - ✅ 32 auth tests - ✅ 13 integration tests

- Added logFriendlyError() helper method to API class - Enhanced auth.js login error messages for common failure modes - Improved platform.js error handling for init, discovery, and refreshes - Network errors now have specific actionable guidance - HTTP errors categorized with helpful user messages - All errors include debug stack traces when debug mode enabled

- Created ConfigValidator class with email, password, PIN validation - Email: Required, format validation with regex - Password: Required, minimum 8 characters - PIN: Optional, must be exactly 4 digits if provided - Debug mode: Optional, must be boolean if provided - Integrated into platform.js with user-friendly error display - Shows all validation errors at once with numbered list - Added 21 comprehensive tests (all passing) - Updated platform.test.js to match new error messages - Total test count: 148 tests passing - Completed Priority 1 Task #4 from roadmap

- Enhanced getVehicleStatus() with actual API implementation - Supports optional vehicle refresh/wakeup before getting status - Primary data source: Vehicle Health Report (VHR) endpoint - Fallback: Vehicle list data when VHR unavailable - Created parseVHRData() to handle VHR API responses - Created parseDoorStatus() for door status extraction - Created parseBatteryLevel() for battery data parsing - Handles multiple API response formats and field variations - Added 12 comprehensive tests (all passing) - Total test count: 160 tests passing - Features: door status, lock status, battery, engine, odometer, fuel - Graceful degradation when data unavailable - Completed Priority 2 Task #5 from roadmap

- Created RateLimiter class with per-command, per-vehicle limits - Configured sensible limits: 3 starts/hour, 10 locks/5min, 5 horn/5min - Integrated into all command methods in platform.js - User-friendly warnings when limits exceeded with wait times - Shows why limits exist (account protection, battery, neighbors) - Added getUsageStats() for monitoring current usage - Added reset() and resetAll() for admin/testing - Created 20 comprehensive tests (all passing) - Total test count: 180 tests passing - Prevents Mopar account blocks from excessive API use - Protects vehicle battery from too many remote starts - Completed Priority 2 Task #6 from roadmap

- Created Logger class with unified logging interface - Supports error, warn, info, log, debug, and trace levels - Smart detection of Homebridge logger vs simple function - Proper method binding for error/warn when available - Falls back to console when no logger provided - Debug and trace levels only show when debug mode enabled - Backward compatible with existing code - Created 20 comprehensive tests (all passing) - Total test count: 200 tests passing - Completed Priority 2 Task #7 from roadmap

- Created Metrics class for LOCAL debugging only - PRIVACY GUARANTEE: NO external calls, NO tracking, NO data transmission - All metrics stay on user's machine - Homebridge verified compatible - Tracks command success rates, API calls, errors, logins, refreshes - Calculates average durations and success percentages - Plugin uptime tracking with human-readable formatting - logSummary() displays stats in debug mode - Privacy verification test ensures no network calls exist - Created 26 comprehensive tests (all passing) - Total test count: 226 tests passing - Completed Priority 2 Task #8 from roadmap - Meets Homebridge verified plugin requirements

- Length: 8-16 characters (not just min 8) - Must have uppercase letter (A-Z) - Must have lowercase letter (a-z) - Must have number (0-9) - Must have special character from @$!%*?&_- - No character repeated more than twice - No more than two sequential characters (ABC, xyz, 123, etc.) - Added hasSequentialCharacters() helper method - Updated all tests to use Mopar-compliant passwords - All 30 config validator tests passing

- Mopar API often returns 403 on profile endpoint after login - Session needs 3-6 seconds to fully propagate on backend - Added 3 retry attempts with 3-second delays to getProfile() - Automatically retries on 403, succeeds once session is ready - Added 2 tests for retry behavior (6 total profile tests) - Fixes initialization failures that were blocking plugin startup - All 228 tests passing

- Mopar.com password requirements vary and have changed over time - Existing working passwords may not meet current new password requirements - Simplified validation to only check: length >= 8, length <= 20 - Removed strict requirements: uppercase, lowercase, number, special char, no repeats, no sequential - User's working password was being rejected on startup - All 25 config validator tests passing

Copilot

Pull request overview

This PR significantly improves the Homebridge Mopar plugin's user experience, reliability, and maintainability by enforcing mandatory PINs, refactoring the lock service architecture, adding rate limiting, and enhancing error handling throughout the codebase.

Key Changes:

Mandatory PIN enforcement with clear UX feedback when commands are blocked
Consolidated lock service architecture (single service per vehicle instead of separate lock/unlock services)
Rate limiting to prevent API abuse and account blocks
Comprehensive configuration validator with helpful error messages
Enhanced error handling with user-friendly messages across all modules
New logger wrapper, metrics collector, and extensive test coverage (99 new tests)

Reviewed changes

Copilot reviewed 21 out of 23 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
`src/config-validator.js`	New comprehensive configuration validator enforcing required PIN, email format, and password constraints
`src/config-validator.test.js`	Complete test coverage for all validation scenarios including edge cases
`src/rate-limiter.js`	New rate limiting system with per-vehicle, per-command tracking to prevent API abuse
`src/rate-limiter.test.js`	Thorough tests covering rate limits, time windows, and multi-vehicle scenarios
`src/metrics.js`	Local-only metrics collector for debugging with explicit privacy guarantees
`src/metrics.test.js`	Tests validating metrics collection and privacy guarantees
`src/logger.js`	Logging wrapper providing consistent interface across simple functions and Homebridge loggers
`src/logger.test.js`	Tests for various logger configurations and fallback behavior
`src/platform.js`	Major refactor: PIN enforcement in all commands, consolidated lock service, contact sensor fixes, rate limiting integration, improved error handling
`src/platform.test.js`	Updated tests for PIN validation, lock service changes, and command blocking scenarios
`src/api.js`	Added retry logic for 403 errors, implemented real vehicle status parsing, user-friendly error logging
`src/api.test.js`	New tests for retry logic, vehicle status parsing, and error handling
`src/auth.js`	Enhanced error handling with specific messages for network, timeout, and SSL errors
`src/integration.test.js`	Updated mocks to support new auth flow and improved test descriptions
`README.md`	Updated to clarify PIN requirement for all remote commands
`CHANGELOG.md`	Comprehensive release notes covering all changes with user-focused descriptions
`package.json`	Version bump to 0.9.14-beta.2
`package-lock.json`	Lock file updates with dependency changes
`.npmignore`	Added `.cursorrules/` directory to exclusions
`.gitignore`	Added `.cursorrules/` directory
`.github/workflows/publish.yml`	Enhanced to support beta releases with proper tagging
`.cursorrules/changelog-maintenance.md`	New documentation for maintaining changelog in Homebridge-compatible format

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-21T23:19:10Z

+    // Add this request to history
+    recent.push(now);
+    this.requests.set(key, recent);
+
+    return { allowed: true };
+  }
+
+  /**


The rate limiter's canExecute() method modifies state (adds timestamps to the requests array at line 60) even when called to check if execution is allowed. This violates the principle of query methods being side-effect free. If a caller checks canExecute() multiple times before actually executing, it will incorrectly consume rate limit quota. Consider splitting this into two methods: canExecute() for checking only, and recordExecution() for actually recording the execution after it completes.

Suggested change

// Add this request to history

recent.push(now);

this.requests.set(key, recent);

return { allowed: true };

}

/**

// If allowed, return true (do not mutate state)

return { allowed: true };

}

/**

* Record execution of a command (should be called after actual execution)

* @param {string} command - Command type

* @param {string} vin - Vehicle VIN

*/

recordExecution(command, vin = '') {

const now = Date.now();

const limit = this.limits[command];

if (!limit) {

return;

}

const key = `${command}_${vin}`;

const history = this.requests.get(key) || [];

// Remove old requests outside the time window

const recent = history.filter((timestamp) => now - timestamp < limit.window);

recent.push(now);

this.requests.set(key, recent);

}

/**

Copilot · 2025-11-21T23:19:11Z

+      // Only validate minimum length - existing passwords may not meet current Mopar requirements
+      if (password.length < 8) {
+        errors.push('Password must be at least 8 characters');
+      }


[nitpick] The password validation at lines 30-31 checks for a minimum length of 8 characters, but this is enforced even for existing users who may have older passwords that worked before Mopar changed their requirements. The comment at line 29 mentions "existing passwords should work" but the validation still rejects passwords shorter than 8 characters. Consider making this a warning rather than an error for backward compatibility, or allowing users to bypass this check with a configuration flag.

Copilot · 2025-11-21T23:19:11Z

+      // User-friendly error messages
+      if (error.message.includes('net::ERR_NAME_NOT_RESOLVED') || error.code === 'ENOTFOUND') {
+        this.log.error('Cannot reach Mopar.com - Check your internet connection');
+      } else if (error.message.includes('timeout') || error.message.includes('Navigation timeout')) {
+        this.log.error('Login timed out - Mopar.com may be slow or unreachable');
+        this.log.error('Try again in a few minutes or check https://www.mopar.com/en-us/sign-in.html');
+      } else if (error.message.includes('ERR_CERT')) {
+        this.log.error('SSL/Certificate error - Check your system time and date settings');
+      } else if (error.message.includes('Execution context was destroyed')) {
+        this.log.error('Browser session crashed - This is usually temporary, try restarting Homebridge');
+      } else {
+        this.log.error(`Login failed: ${error.message}`);
+        this.log.error('Please verify your Mopar.com credentials are correct');
+      }
+
+      this.debug(`Full error: ${error.stack}`);
+


The error handling improvements at lines 844-860 provide much better user feedback. However, there's inconsistent use of this.log.error() vs this.log(). At lines 846, 848, 851, 853, 855-856, the code calls this.log.error(), but if this is an older codebase where this.log is a simple function (as mentioned in the PR summary about API error handling fixes), these calls to this.log.error() may fail with "not a function" errors. Verify that MoparAuth has been updated to use the new Logger class wrapper that provides the .error() method, or change these to this.log('ERROR: ...') for consistency with the API class fixes mentioned in CHANGELOG.md line 82.

Copilot · 2025-11-21T23:19:11Z

+class RateLimiter {
+  constructor() {
+    // Map of command types to their timestamp history
+    this.requests = new Map(); // command -> timestamps[]


The requests Map at line 9 stores command execution history per vehicle using ${command}_${vin} keys. Over time, as vehicles are added/removed or if VINs change, this map could accumulate stale entries that are never cleaned up. Consider adding a cleanup mechanism to remove entries that haven't been accessed in a configurable time period (e.g., 24 hours), or implement a maximum map size with LRU eviction to prevent unbounded memory growth.

Copilot · 2025-11-21T23:19:11Z

+    const retryDelay = 3000; // 3 seconds between retries
+
+    for (let attempt = 1; attempt <= maxRetries; attempt++) {
+      if (attempt > 1) {
+        this.debug(`Profile retry attempt ${attempt}/${maxRetries} after ${retryDelay}ms delay...`);
+        await new Promise((resolve) => setTimeout(resolve, retryDelay));


[nitpick] The getProfile() method now includes retry logic for 403 errors, which is good. However, the retry delay is hardcoded to 3 seconds at line 148. If the first retry at 3 seconds fails, the second retry also waits another 3 seconds, and the third waits another 3 seconds - totaling up to 9 seconds of blocking delays during initialization. Consider using exponential backoff (e.g., 1s, 2s, 4s) or reducing the initial delay to improve initialization time while still handling transient 403s.

Suggested change

const retryDelay = 3000; // 3 seconds between retries

for (let attempt = 1; attempt <= maxRetries; attempt++) {

if (attempt > 1) {

this.debug(`Profile retry attempt ${attempt}/${maxRetries} after ${retryDelay}ms delay...`);

await new Promise((resolve) => setTimeout(resolve, retryDelay));

const baseDelay = 1000; // 1 second initial delay

for (let attempt = 1; attempt <= maxRetries; attempt++) {

if (attempt > 1) {

const delay = baseDelay * Math.pow(2, attempt - 2); // 1s, 2s, 4s

this.debug(`Profile retry attempt ${attempt}/${maxRetries} after ${delay}ms delay...`);

await new Promise((resolve) => setTimeout(resolve, delay));

Copilot · 2025-11-21T23:19:12Z

+## Unreleased

-## [0.9.10] - 2025-10-20
+## 0.9.14-beta.0 (2025-10-20)


The changelog entry for version 0.9.14-beta.0 at line 9 indicates this is the version being released, but package.json shows the version as 0.9.14-beta.2 (line 4). The changelog should be updated to include entries for beta.1 and beta.2 if they were released, or the version should be consolidated. This inconsistency could confuse users about which version they're using and what changes are included.

Copilot · 2025-11-21T23:19:13Z

-    // Lock service
    const lockDisplayName = `${name} Lock`;
-    const lockService = accessory.addService(Service.LockMechanism, lockDisplayName, vehicle.vin + '-lock');
+    let lockService = accessory.getServiceById(Service.LockMechanism, `${vehicle.vin}-lock`);


The lock service refactor has improved the architecture by consolidating into a single LockMechanism per vehicle. However, the service is only created if it doesn't exist (if (!lockService)), but there's no cleanup of the old dual lock/unlock services that existed before this refactor. In configureAccessory() at lines 395-397, services are removed except AccessoryInformation and LockMechanism, but this happens during restore. For existing installations upgrading to this version, users might have orphaned lock/unlock services. Consider explicitly removing services with subtypes ${vin}-lock and ${vin}-unlock before creating the new consolidated service to ensure clean migration.

Suggested change

let lockService = accessory.getServiceById(Service.LockMechanism, `${vehicle.vin}-lock`);

// Remove any old lock/unlock services with legacy subtypes

const oldLockService = accessory.getServiceById(Service.LockMechanism, `${vehicle.vin}-lock`);

const oldUnlockService = accessory.getServiceById(Service.LockMechanism, `${vehicle.vin}-unlock`);

if (oldUnlockService) {

accessory.removeService(oldUnlockService);

}

let lockService = oldLockService;

Copilot · 2025-11-21T23:19:13Z

+        const lastCall = accessory.context[lastCallKey] || 0;
+        if (now - lastCall < 10000) {
+          this.log(`Ignoring duplicate ${desiredAction.toLowerCase()} command (within 10s)`);
+          throw new this.api.hap.HapStatusError(this.api.hap.HAPStatus.RESOURCE_BUSY);


In the lock service's onSet handler, when a duplicate command is detected within 10 seconds, the code throws this.api.hap.HapStatusError(this.api.hap.HAPStatus.RESOURCE_BUSY) at line 471. However, this will result in HomeKit showing an error to the user. The previous implementation logged the duplicate and returned silently, which provided a better user experience. Consider returning early without throwing an error, or using a different HAP status code that doesn't present as an error in the Home app.

Suggested change

throw new this.api.hap.HapStatusError(this.api.hap.HAPStatus.RESOURCE_BUSY);

return;

Copilot · 2025-11-21T23:19:13Z

+  return { MoparPlatform: registrationCall[2], mockHomebridge };
 })();

+const Characteristic = mockHomebridge.hap.Characteristic;


Unused variable Characteristic.

Suggested change

const Characteristic = mockHomebridge.hap.Characteristic;

Copilot · 2025-11-21T23:19:14Z

+const Service = mockHomebridge.hap.Service;
+


Unused variable Service.

Suggested change

const Service = mockHomebridge.hap.Service;

Copilot

Pull request overview

Copilot reviewed 28 out of 31 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

frankea added 30 commits October 20, 2025 03:00

Fix linting errors in integration tests

ca95e21

- Fixed formatting issues in src/platform.test.js - Fixed formatting issues in src/integration.test.js - Fixed unused variable warning by adding assertion - All linting checks now passing

Mark releases as beta/prerelease

2ec6eec

Fix: Use correct log method in API error handling

9bb450a

Bump to 0.9.11 - fix critical log error bug

b330941

Bump to 0.9.12 - fix critical log error bug

a7f7b15

Fix: Add contents write permission for GitHub releases

1337fff

Bump to 0.9.13 - workflow permissions fix

fbe944c

Update CHANGELOG with v0.9.12 and v0.9.13 details

d70149e

Add changelog maintenance cursor rule

39e3d88

Add .cursorrules/ to .gitignore

890e95c

Fix: Update CHANGELOG format for Homebridge UI compatibility

7f2dd8f

Update CHANGELOG for v0.9.14-beta.0 release

36fae02

Fix metrics test timing issues with real timers

0893f3e

Bump to 0.9.14-beta.0 - beta testing release

c7d5480

Fix linting errors and remove unused variable

0a55701

Bump to 0.9.14-beta.1 - beta testing release

aa3c8e3

Support beta tag publishing in GitHub workflow

a3bf48f

Fix linting errors in config validator

c36d404

Fix linting in profile retry logic

d91f6e1

Bump to 0.9.14-beta.2 - 403 profile retry fix

b150b31

Complete Mopar improvements per plan

b8128d0

Copilot AI review requested due to automatic review settings November 21, 2025 23:14

Copilot started reviewing on behalf of frankea November 21, 2025 23:14 View session

Copilot finished reviewing on behalf of frankea November 21, 2025 23:16

Copilot AI reviewed Nov 21, 2025

View reviewed changes

Test engineering plan implementation

a2cdf25

frankea requested a review from Copilot November 22, 2025 04:06

Copilot started reviewing on behalf of frankea November 22, 2025 04:07 View session

Copilot finished reviewing on behalf of frankea November 22, 2025 04:08

Copilot AI reviewed Nov 22, 2025

View reviewed changes

Adam Franke added 3 commits November 21, 2025 23:57

Fix lint formatting issues in config validator

b089ee1

Run test workflow on all branches

7b9e086

Gate e2e workflow job on secret availability

18ca238

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Mopar UX, diagnostics, and timers#4

Improve Mopar UX, diagnostics, and timers#4
frankea wants to merge 35 commits into
mainfrom
feature/lock-service-refactor

frankea commented Nov 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    // Add this request to history
-    recent.push(now);
-    this.requests.set(key, recent);
-    return { allowed: true };
-  }
-  /**
+    // If allowed, return true (do not mutate state)
+    return { allowed: true };
+  }
+  /**
+   * Record execution of a command (should be called after actual execution)
+   * @param {string} command - Command type
+   * @param {string} vin - Vehicle VIN
+   */
+  recordExecution(command, vin = '') {
+    const now = Date.now();
+    const limit = this.limits[command];
+    if (!limit) {
+      return;
+    }
+    const key = `${command}_${vin}`;
+    const history = this.requests.get(key) || [];
+    // Remove old requests outside the time window
+    const recent = history.filter((timestamp) => now - timestamp < limit.window);
+    recent.push(now);
+    this.requests.set(key, recent);
+  }
+  /**

-    let lockService = accessory.getServiceById(Service.LockMechanism, `${vehicle.vin}-lock`);
+    // Remove any old lock/unlock services with legacy subtypes
+    const oldLockService = accessory.getServiceById(Service.LockMechanism, `${vehicle.vin}-lock`);
+    const oldUnlockService = accessory.getServiceById(Service.LockMechanism, `${vehicle.vin}-unlock`);
+    if (oldUnlockService) {
+      accessory.removeService(oldUnlockService);
+    }
+    let lockService = oldLockService;

	throw new this.api.hap.HapStatusError(this.api.hap.HAPStatus.RESOURCE_BUSY);
	return;

Conversation

frankea commented Nov 21, 2025

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants