refactor: extract clampSetting + evictOldestFifo helpers (deduplicates 15 sites)

Simplify-pass over the 1.9.1 perf batch flagged two repeated patterns:

1. clampSetting(value, floor, default) wraps Math.max(floor, Number(value) || default).
   Used 11 times across eventPublisher, deviceStateManager, cgateWebBridge,
   throttledQueue, staleDeviceDetector, and haBridgeDiagnostics for reading
   numeric settings with a hard floor and a fallback when the value is
   missing/NaN/zero.

2. evictOldestFifo(map) encapsulates `const k = map.keys().next().value;
   map.delete(k); return k;` - the unintuitive FIFO-from-Map idiom. Used 4
   times across mqttManager (_pendingPublishQueue), deviceStateManager
   (_deviceLevels with sibling _lastSeen evicted in lockstep), and
   eventPublisher (_topicCache + _recentPublishes).

Both helpers live in a new src/utils.js with their own unit tests (10 cases
covering defaults, floors, FIFO order, edge cases).

Two smaller follow-ups also applied:
- The 4-line comment about the deferred web server start in cgateWebBridge.js
  trimmed to one line (only the WHY survives - WHAT was just narration).
- The readiness-before-post-connect-work test in
  bridgeInitializationService.test.js switched from a brittle
  `eventOrder[0] === 'readiness'` check to indexOf comparisons that verify
  the actual ordering constraint (readiness before queue-add AND
  discovery-trigger).

No behaviour change anywhere; 1254/1254 tests pass.

Author: dougrathbone

src/cgateWebBridge.js

@@ -16,6 +16,7 @@ const StaleDeviceDetector = require('./staleDeviceDetector');
 const { createLogger } = require('./logger');
 const { LineProcessor } = require('./lineProcessor');
 const { MQTT_RETAINED_STATE_OPTIONS } = require('./constants');
+const { clampSetting } = require('./utils');
 
 /**
  * Main bridge class that connects C-Gate (Clipsal C-Bus automation system) to MQTT.
@@ -294,10 +295,7 @@ class CgateWebBridge {
         this.staleDeviceDetector.start();
         this._updateBridgeReadiness('startup-complete');
 
-        // The C-Bus Labels web UI is never exercised during the boot window,
-        // so start it in the background after readiness has fired. Keeping it
-        // off the await chain shaves it off the critical startup path; failure
-        // logs without blocking the bridge.
+        // Off the await chain so it never gates the critical startup path.
         this.webServer.start().catch((err) => {
             this.logger.warn(`Web server failed to start: ${err.message}`);
         });
@@ -431,8 +429,8 @@ class CgateWebBridge {
     }
 
     _getAdaptiveQueueIntervalMs() {
-        const baseInterval = Math.max(10, Number(this.settings.messageinterval) || 200);
-        const minInterval = Math.max(5, Number(this.settings.commandMinIntervalMs) || 10);
+        const baseInterval = clampSetting(this.settings.messageinterval, 10, 200);
+        const minInterval = clampSetting(this.settings.commandMinIntervalMs, 5, 10);
         const stats = this.commandConnectionPool?.getStats?.();
         if (!stats || stats.healthyConnections <= 0) {
             return baseInterval;

src/deviceStateManager.js

@@ -1,5 +1,6 @@
 const { EventEmitter } = require('events');
 const { createLogger } = require('./logger');
+const { clampSetting, evictOldestFifo } = require('./utils');
 const {
     MQTT_TOPIC_SUFFIX_LEVEL,
     CGATE_CMD_ON,
@@ -59,9 +60,8 @@ class DeviceStateManager {
         // Track last-seen timestamp per device address (network/app/group → ms since epoch)
         this._lastSeen = new Map();
 
-        // Bound on _deviceLevels / _lastSeen growth. Matches the floor + default
-        // pattern used by eventPublisher's topicCacheMaxEntries / dedup cache.
-        this._maxEntries = Math.max(100, Number(this.settings.deviceStateMaxEntries) || 5000);
+        // Bound on _deviceLevels / _lastSeen growth.
+        this._maxEntries = clampSetting(this.settings.deviceStateMaxEntries, 100, 5000);
     }
 
     /**
@@ -117,10 +117,10 @@ class DeviceStateManager {
         if (levelValue !== null) {
             this.logger.debug(`Level update: ${simpleAddr} = ${levelValue}`);
             // FIFO-evict the oldest entry before inserting to keep both maps
-            // bounded. Matches eventPublisher's topic-cache pattern.
+            // bounded; _lastSeen tracks the same key space so it must evict in
+            // lockstep.
             if (!this._deviceLevels.has(simpleAddr) && this._deviceLevels.size >= this._maxEntries) {
-                const oldestKey = this._deviceLevels.keys().next().value;
-                this._deviceLevels.delete(oldestKey);
+                const oldestKey = evictOldestFifo(this._deviceLevels);
                 this._lastSeen.delete(oldestKey);
             }
             // Store latest known level so callers can retrieve it synchronously

src/eventPublisher.js

@@ -1,4 +1,5 @@
 const { createLogger } = require('./logger');
+const { clampSetting, evictOldestFifo } = require('./utils');
 const {
     MQTT_TOPIC_PREFIX_READ,
     MQTT_TOPIC_SUFFIX_STATE,
@@ -34,9 +35,9 @@ class EventPublisher {
         this.labelLoader = options.labelLoader || null;
         this.coverRampTracker = options.coverRampTracker || null;
         this.onEventLog = options.onEventLog || null;
-        this.eventPublishDedupWindowMs = Math.max(0, Number(this.settings.eventPublishDedupWindowMs) || 0);
-        this.eventPublishDedupMaxEntries = Math.max(100, Number(this.settings.eventPublishDedupMaxEntries) || 5000);
-        this.topicCacheMaxEntries = Math.max(100, Number(this.settings.topicCacheMaxEntries) || 5000);
+        this.eventPublishDedupWindowMs = clampSetting(this.settings.eventPublishDedupWindowMs, 0, 0);
+        this.eventPublishDedupMaxEntries = clampSetting(this.settings.eventPublishDedupMaxEntries, 100, 5000);
+        this.topicCacheMaxEntries = clampSetting(this.settings.topicCacheMaxEntries, 100, 5000);
         this.eventPublishCoalesce = this.settings.eventPublishCoalesce === true;
         this._recentPublishes = new Map();
         this._topicCache = new Map();
@@ -388,7 +389,7 @@ class EventPublisher {
         };
 
         if (this._topicCache.size >= this.topicCacheMaxEntries) {
-            this._topicCache.delete(this._topicCache.keys().next().value);
+            evictOldestFifo(this._topicCache);
         }
         this._topicCache.set(key, topics);
         this._publishStats.topicCacheMiss += 1;
@@ -411,9 +412,8 @@ class EventPublisher {
 
         // Second pass: enforce max size by oldest insertion order.
         while (this._recentPublishes.size > this.eventPublishDedupMaxEntries) {
-            const oldestKey = this._recentPublishes.keys().next().value;
+            const oldestKey = evictOldestFifo(this._recentPublishes);
             if (oldestKey === undefined) break;
-            this._recentPublishes.delete(oldestKey);
             this._publishStats.dedupEvicted += 1;
         }
     }

src/haBridgeDiagnostics.js

@@ -1,6 +1,7 @@
 const fs = require('fs');
 const { createLogger } = require('./logger');
 const { MQTT_TOPIC_STATUS, MQTT_RETAINED_STATE_OPTIONS, entityIdFields, HA_COMPONENT_SENSOR, HA_COMPONENT_BINARY_SENSOR, HA_DEVICE_VIA } = require('./constants');
+const { clampSetting } = require('./utils');
 
 const CGATE_VERSION_FILE = '/data/cgate/.version';
 
@@ -19,7 +20,7 @@ class HaBridgeDiagnostics {
             return;
         }
 
-        const intervalSeconds = Math.max(10, Number(this.settings.ha_bridge_diagnostics_interval_sec) || 60);
+        const intervalSeconds = clampSetting(this.settings.ha_bridge_diagnostics_interval_sec, 10, 60);
         if (this._intervalId) {
             clearInterval(this._intervalId);
         }

src/mqttManager.js

@@ -2,6 +2,7 @@ const mqtt = require('mqtt');
 const { EventEmitter } = require('events');
 const { createLogger } = require('./logger');
 const { createErrorHandler } = require('./errorHandler');
+const { evictOldestFifo } = require('./utils');
 const { 
     MQTT_TOPIC_PREFIX_WRITE,
     MQTT_TOPIC_STATUS,
@@ -161,8 +162,7 @@ class MqttManager extends EventEmitter {
         // Bounded queue: evict the oldest entry when over the cap. Map
         // iteration is insertion order so the first key is the oldest.
         if (this._pendingPublishQueue.size > this._pendingPublishMaxEntries) {
-            const oldest = this._pendingPublishQueue.keys().next().value;
-            this._pendingPublishQueue.delete(oldest);
+            evictOldestFifo(this._pendingPublishQueue);
             this._pendingPublishEvicted += 1;
         }
     }

src/staleDeviceDetector.js

@@ -1,5 +1,6 @@
 const { createLogger } = require('./logger');
 const { MQTT_TOPIC_STATUS, MQTT_RETAINED_STATE_OPTIONS, entityIdFields, HA_COMPONENT_SENSOR, HA_DEVICE_VIA } = require('./constants');
+const { clampSetting } = require('./utils');
 
 /**
  * Periodically checks for C-Bus devices that have not reported a state change
@@ -44,7 +45,7 @@ class StaleDeviceDetector {
             return;
         }
 
-        const intervalSec = Math.max(60, Number(this.settings.stale_device_check_interval_sec) || 3600);
+        const intervalSec = clampSetting(this.settings.stale_device_check_interval_sec, 60, 3600);
 
         this._publishDiscovery();
         this._check();
@@ -73,7 +74,7 @@ class StaleDeviceDetector {
      */
     _check() {
         try {
-            const thresholdMs = Math.max(1, Number(this.settings.stale_device_threshold_hours) || 24) * 60 * 60 * 1000;
+            const thresholdMs = clampSetting(this.settings.stale_device_threshold_hours, 1, 24) * 60 * 60 * 1000;
             const staleDevices = this._getStaleDevices(thresholdMs);
             this._publishStaleCount(staleDevices.length, staleDevices);
         } catch (error) {
@@ -120,7 +121,7 @@ class StaleDeviceDetector {
      * @private
      */
     _publishStaleCount(count, staleDevices) {
-        const thresholdHours = Math.max(1, Number(this.settings.stale_device_threshold_hours) || 24);
+        const thresholdHours = clampSetting(this.settings.stale_device_threshold_hours, 1, 24);
 
         this.mqttClient.publish('cbus/bridge/stale_devices', String(count), MQTT_RETAINED_STATE_OPTIONS);
 

src/throttledQueue.js

@@ -1,4 +1,5 @@
 const { createLogger } = require('./logger');
+const { clampSetting } = require('./utils');
 
 class ThrottledQueue {
     /**
@@ -123,7 +124,7 @@ class ThrottledQueue {
         if (this._timer !== null) {
             return;
         }
-        const delay = Math.max(0, Number(delayMs) || 0);
+        const delay = clampSetting(delayMs, 0, 0);
         this._timer = setTimeout(() => {
             this._timer = null;
             this._process();

src/utils.js

@@ -0,0 +1,30 @@
+/**
+ * Clamp a settings value to a minimum, falling back to a default when the
+ * configured value is missing, NaN, or zero. Encodes the canonical pattern
+ * for reading numeric settings (Math.max(floor, Number(value) || default))
+ * used throughout the bridge.
+ *
+ * @param {*} value         The raw configured value (any type; coerced via Number).
+ * @param {number} floor    Hard lower bound enforced regardless of value or default.
+ * @param {number} defaultValue Value used when Number(value) is falsy (0, NaN, undefined, etc).
+ * @returns {number}
+ */
+function clampSetting(value, floor, defaultValue) {
+    return Math.max(floor, Number(value) || defaultValue);
+}
+
+/**
+ * Remove the oldest key from a Map (FIFO order matches insertion order in JS)
+ * and return the evicted key. Used by the bounded caches across the bridge to
+ * keep size at or below configured limits.
+ *
+ * @param {Map} map
+ * @returns {*} the evicted key (undefined if map was empty)
+ */
+function evictOldestFifo(map) {
+    const oldestKey = map.keys().next().value;
+    map.delete(oldestKey);
+    return oldestKey;
+}
+
+module.exports = { clampSetting, evictOldestFifo };

tests/bridgeInitializationService.test.js

@@ -450,10 +450,14 @@ describe('BridgeInitializationService', () => {
             const svc = new BridgeInitializationService(bridge);
             await svc.handleAllConnected();
 
-            // readiness fires first - then the rest of the post-connect work.
-            expect(eventOrder[0]).toBe('readiness');
-            expect(eventOrder).toContain('queue-add');
-            expect(eventOrder).toContain('discovery-trigger');
+            // Verify the actual ordering constraint (readiness before each
+            // post-connect work item) rather than the brittle "readiness is
+            // first" check - keeps the test robust if a future change adds
+            // bookkeeping before readiness.
+            const readinessIdx = eventOrder.indexOf('readiness');
+            expect(readinessIdx).toBeGreaterThanOrEqual(0);
+            expect(readinessIdx).toBeLessThan(eventOrder.indexOf('queue-add'));
+            expect(readinessIdx).toBeLessThan(eventOrder.indexOf('discovery-trigger'));
         });
 
         it('passes working publish callback to HaDiscovery', async () => {

tests/utils.test.js

@@ -0,0 +1,53 @@
+const { clampSetting, evictOldestFifo } = require('../src/utils');
+
+describe('clampSetting', () => {
+    it('uses default when value is undefined', () => {
+        expect(clampSetting(undefined, 100, 5000)).toBe(5000);
+    });
+
+    it('uses default when value is 0 (treated as "not configured")', () => {
+        expect(clampSetting(0, 100, 5000)).toBe(5000);
+    });
+
+    it('uses default when value is NaN', () => {
+        expect(clampSetting(NaN, 100, 5000)).toBe(5000);
+    });
+
+    it('returns configured value when above floor', () => {
+        expect(clampSetting(2000, 100, 5000)).toBe(2000);
+    });
+
+    it('clamps to floor when configured value is below it', () => {
+        expect(clampSetting(50, 100, 5000)).toBe(100);
+    });
+
+    it('coerces string values via Number()', () => {
+        expect(clampSetting('3000', 100, 5000)).toBe(3000);
+    });
+
+    it('clamps to floor when default is below floor', () => {
+        expect(clampSetting(undefined, 100, 0)).toBe(100);
+    });
+});
+
+describe('evictOldestFifo', () => {
+    it('removes and returns the oldest inserted key', () => {
+        const m = new Map([['a', 1], ['b', 2], ['c', 3]]);
+        expect(evictOldestFifo(m)).toBe('a');
+        expect([...m.keys()]).toEqual(['b', 'c']);
+    });
+
+    it('returns undefined for an empty map (no-op)', () => {
+        const m = new Map();
+        expect(evictOldestFifo(m)).toBeUndefined();
+        expect(m.size).toBe(0);
+    });
+
+    it('FIFO order matches insertion order even after updates', () => {
+        const m = new Map();
+        m.set('a', 1);
+        m.set('b', 2);
+        m.set('a', 99); // update in place; does NOT move to end
+        expect(evictOldestFifo(m)).toBe('a');
+    });
+});