Skip to content

refactor(mqtt-bridge): optional code improvements for device config loading #344

Description

@groupsky

Context

PR #341 implemented device loading from config with production-ready code. These are optional improvements for code quality and future-proofing identified during code review.

Optional Improvements

Low Priority - YAGNI Fix

1. Simplify Redundant Length Check

Current code (cli.ts:102):

if (config.devices && config.devices.length > 0) {
  console.log(chalk.blue(`Loading ${config.devices.length} device(s)...`))
  for (const deviceConfig of config.devices) {
    await bridge.addDevice(deviceConfig)
    console.log(chalk.green(`✓ Loaded device: ${deviceConfig.deviceId}`))
  }
}

Issue: length > 0 check is unnecessary

  • Iterating over an empty array is a no-op (harmless)
  • Adds cognitive overhead with no functional benefit

Suggested simplification:

if (config.devices) {
  if (config.devices.length > 0) {
    console.log(chalk.blue(`Loading ${config.devices.length} device(s)...`))
  }
  for (const deviceConfig of config.devices) {
    await bridge.addDevice(deviceConfig)
    console.log(chalk.green(`✓ Loaded device: ${deviceConfig.deviceId}`))
  }
}

Or even simpler:

if (config.devices?.length) {
  console.log(chalk.blue(`Loading ${config.devices.length} device(s)...`))
  for (const deviceConfig of config.devices) {
    await bridge.addDevice(deviceConfig)
    console.log(chalk.green(`✓ Loaded device: ${deviceConfig.deviceId}`))
  }
}

Impact: Minor code quality improvement, not blocking

Low Priority - DoS Prevention

2. Add Maximum Devices Limit

Current behavior: No limit on devices array size

Potential issue: Very large device arrays could cause:

  • Memory exhaustion
  • Slow startup times
  • Resource exhaustion

Suggested fix (config-validator.ts):

devices: z.array(deviceConfigSchema).max(100, 'Maximum 100 devices allowed').optional(),

Justification:

  • 100 devices is more than reasonable for single bridge instance
  • Prevents accidental misconfiguration (e.g., copy-paste errors)
  • Prevents DoS via config file manipulation

Trade-off: Some users might legitimately need >100 devices. Consider making this configurable or documenting workaround (run multiple bridge instances).

Very Low Priority - Platform-Specific Validation

3. RTU Port Path Validation

Current behavior: Accepts any string for RTU port

  • port: "/dev/ttyUSB0" ✅ Valid
  • port: "COM3" ✅ Valid
  • port: "notaport" ✅ Valid (fails later at runtime)

Potential improvement: Platform-specific validation

// Linux/macOS
.regex(/^\/dev\/tty.*$/, 'RTU port must be /dev/tty* on Unix systems')

// Windows
.regex(/^COM\d+$/, 'RTU port must be COM* on Windows')

Concern: Cross-platform complexity, may break valid use cases

Recommendation: Skip this unless user reports indicate it's needed

Acceptance Criteria

  • YAGNI fix: Simplify redundant length check
  • DoS prevention: Add max devices limit (with documentation)
  • Platform validation: Evaluate but likely skip

Related

Priority

Low - Nice-to-have improvements, not urgent

Notes

These are quality-of-life improvements identified during architectural review. The current implementation is production-ready; these changes are optional enhancements.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestpriority:lowLow priority taskrefactorCode refactoring and improvementsscope:mqtt-bridgeIssues related to the MQTT bridge package

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions