Skip to content

Conversation

@tomquist
Copy link
Owner

@tomquist tomquist commented Nov 15, 2025

When all configured devices had invalid/unknown device types, the system would enter an endless loop because getPollingInterval() attempted to reduce an empty array, resulting in undefined being passed to setInterval().

Changes:

  • Add validation in DeviceManager constructor to fail fast when no valid devices are configured
  • Add safety check in getPollingInterval() to throw clear error
  • Add error handling in setupPeriodicPolling() to gracefully handle the failure case
  • Improve error logging to display actual error messages instead of [object Object]
  • Add tests for invalid device type scenarios

This ensures the application exits immediately with a helpful error message rather than consuming CPU resources in an endless loop.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed issue where loops occur when all configured devices have invalid or unknown device types
    • Enhanced error logging with improved stack trace visibility at debug level
    • Added graceful error handling when polling intervals cannot be retrieved
  • Tests

    • Expanded test coverage for invalid and mixed device type scenarios

When all configured devices had invalid/unknown device types, the
system would enter an endless loop because getPollingInterval()
attempted to reduce an empty array, resulting in undefined being
passed to setInterval().

Changes:
- Add validation in DeviceManager constructor to fail fast when no
  valid devices are configured
- Add safety check in getPollingInterval() to throw clear error
- Add error handling in setupPeriodicPolling() to gracefully handle
  the failure case
- Improve error logging to display actual error messages instead of
  [object Object]
- Add tests for invalid device type scenarios

This ensures the application exits immediately with a helpful error
message rather than consuming CPU resources in an endless loop.
@coderabbitai
Copy link

coderabbitai bot commented Nov 15, 2025

Walkthrough

This PR adds validation to DeviceManager to detect when no configured devices have recognized device types and implements graceful error handling. The application now throws an error during initialization if all devices are invalid, and error logging is improved across multiple files.

Changes

Cohort / File(s) Summary
Device Validation & Error Handling
src/deviceManager.ts
Added device type validation during initialization; increments counter for valid devices and throws error if none exist. Added error handling in getPollingInterval() to throw when no valid polling intervals found.
Device Validation Tests
src/deviceManager.test.ts
Added test cases verifying constructor throws on all-invalid devices, invalid devices yield undefined topics, valid devices yield topics, and getPollingInterval() executes without throwing when a valid device exists.
Error Handling & Logging
src/index.ts
Refactored error logging to log generic initialization failure first, then conditionally log error message and stack trace (in debug mode) or raw error value.
Polling Setup Error Handling
src/mqttClient.ts
Wrapped getPollingInterval() call in try-catch block within setupPeriodicPolling() to gracefully handle retrieval failures and exit early.
Documentation
CHANGELOG.md
Added changelog entry documenting fix for loop when all configured devices have invalid/unknown device types.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant DM as DeviceManager
    participant MC as MqttClient

    rect rgb(240, 248, 255)
    note over App,MC: New Validation Flow
    App->>DM: constructor(devices)
    activate DM
    DM->>DM: Count valid devices
    alt No valid devices
        DM-->>App: throw Error
        App->>App: Catch & log error
        App->>App: Exit with code 1
    else At least one valid device
        DM-->>App: Success
    end
    deactivate DM
    
    App->>MC: setupPeriodicPolling()
    activate MC
    MC->>DM: getPollingInterval()
    activate DM
    alt No valid polling interval
        DM-->>MC: throw Error
        MC->>MC: Catch error & log
        MC-->>App: Early exit
    else Valid interval found
        DM-->>MC: Return interval
        MC->>MC: Configure polling
    end
    deactivate DM
    deactivate MC
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Device validation logic: Verify counter increments correctly and error conditions are properly triggered
  • Error handling consistency: Ensure try-catch blocks in mqttClient.ts align with new throws in deviceManager.ts
  • Test coverage: Confirm all edge cases (all invalid, all valid, mixed) are adequately covered
  • Error message clarity: Review log messages for debugging clarity and user understanding

Poem

🐰 Oh, what care we now take to see,
Which devices are valid, which cannot be!
No loops on naught but phantom types,
The app now fails with proper gripes! 🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main fix: preventing an endless loop caused by invalid device configurations, which aligns with the core problem addressed throughout the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-endless-loop

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9acbd77 and 90794aa.

📒 Files selected for processing (5)
  • CHANGELOG.md (1 hunks)
  • src/deviceManager.test.ts (1 hunks)
  • src/deviceManager.ts (3 hunks)
  • src/index.ts (1 hunks)
  • src/mqttClient.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/deviceManager.test.ts (3)
src/types.ts (1)
  • MqttConfig (220-238)
src/constants.ts (1)
  • DEFAULT_TOPIC_PREFIX (5-5)
src/deviceManager.ts (1)
  • DeviceManager (32-295)
src/deviceManager.ts (1)
src/deviceDefinition.ts (1)
  • getDeviceDefinition (196-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test-addon
  • GitHub Check: build-addon (linux/arm64)
  • GitHub Check: build-addon (linux/arm/v7)
🔇 Additional comments (7)
src/deviceManager.ts (3)

46-54: LGTM! Clean fail-fast validation.

The valid device counter correctly tracks devices with recognized definitions during initialization.


78-82: LGTM! Critical validation prevents the endless loop.

This fail-fast check ensures the application exits immediately with a clear error message when no valid devices are configured, preventing the undefined polling interval issue that caused the endless loop.


271-275: Good defensive validation layer.

This check ensures reduce() on Line 284 doesn't fail with an empty array. While the constructor validation should prevent this in typical scenarios, this provides an additional safety net if a device definition exists but has no messages with polling intervals.

src/index.ts (1)

326-334: Excellent improvement to error logging!

This properly handles error formatting by:

  • Checking the error type before accessing properties
  • Displaying the actual error message instead of "[object Object]"
  • Conditionally showing stack traces only in debug mode
  • Gracefully handling non-Error types
src/deviceManager.test.ts (2)

57-74: LGTM! Good test coverage for the fail-fast validation.

This test correctly verifies that the constructor throws when all configured devices have unrecognized types, preventing the endless loop scenario.


76-105: LGTM! Excellent edge case coverage.

This test verifies the system handles mixed configurations correctly:

  • Invalid devices are skipped (undefined topics)
  • Valid devices work normally
  • getPollingInterval() succeeds when at least one valid device exists

This ensures the application remains functional even when some devices have invalid types.

src/mqttClient.ts (1)

162-169: LGTM! Critical fix prevents the endless loop.

This try-catch block is essential for gracefully handling the case where getPollingInterval() throws due to no valid devices. The early return prevents undefined from being passed to setInterval() on Line 184, which would cause an endless loop and high CPU usage.

The error messages are clear and help with debugging the root cause.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tomquist tomquist merged commit 131cbb4 into develop Nov 15, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants