BLE Provider API with ESP32 gateway support and admin UI#2588
Open
dirkwa wants to merge 4 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Signal K v2 BLE API contracts, TypeBox schemas, local and remote providers, GATT management, REST/WebSocket exposure, admin interfaces, plugin wiring, configuration support, and documentation. ChangesBLE feature
Estimated code review effort: 5 (Critical) | ~90+ minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/index.ts (1)
110-121:⚠️ Potential issue | 🟠 MajorMake
startApisasync and await API startup promises.The
Promise.all([...])at lines 110–120 fires withoutawait, allowing API startup failures to become unhandled rejections instead of failing startup explicitly. While a global unhandled rejection handler exists, startup errors should be caught and propagated here.Make the function
async,awaitthe Promise.all, and let errors surface naturally:🔧 Proposed fix
-export const startApis = ( +export const startApis = async ( app: SignalKMessageHub & WithSecurityStrategy & IRouter & WithConfig & WithFeatures -) => { +) => { const apiList: Array<SignalKApiId> = [] // ... API initialization ... - Promise.all([ + await Promise.all([ resourcesApi.start(), courseApi.start(), weatherApi.start(), featuresApi.start(), autopilotApi.start(), radarApi.start(), historyApiHttpRegistry.start(), notificationApi.start(), bleApi.start() ]) return apiList }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/index.ts` around lines 110 - 121, The startApis function currently calls Promise.all([...]) without awaiting it, causing startup failures to become unhandled rejections; change the startApis function signature to be async and await the Promise.all call (which includes resourcesApi.start(), courseApi.start(), weatherApi.start(), featuresApi.start(), autopilotApi.start(), radarApi.start(), historyApiHttpRegistry.start(), notificationApi.start(), bleApi.start()) so that errors propagate out of startApis instead of being swallowed, and then return apiList after the await so startup failures surface to the caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/develop/plugins/ble_provider_plugins.md`:
- Around line 167-185: The sample implementation of subscribeGATT returns an
object missing the required read(charUuid) method required by the
GATTSubscriptionHandle interface; update the object returned by subscribeGATT to
include an async read: (charUuid) => { /* read from characteristic and return
Buffer or appropriate type */ }, and ensure its signature and return type match
the GATTSubscriptionHandle definition (as used in
packages/server-api/src/bleapi.ts); keep the existing write, close, connected,
onDisconnect, and onConnect members intact and implement/read-call behavior
consistent with subscribeGATT.
In `@docs/develop/rest-api/ble_api.md`:
- Around line 64-70: The documented endpoints use Express-style route
parameters; update the paths in ble_api.md to client-facing syntax by replacing
`GET /devices/:mac` and `GET /devices/:mac/gatt` with either parameter braces
like `GET /devices/{mac}` or a concrete example such as `GET
/devices/AA:BB:CC:DD:EE:FF` (ensure both occurrences in the file, including the
headings and any descriptive text for the GATT status endpoint, are updated to
the chosen format).
- Around line 34-35: Update the description that currently states gateways
"remain listed even when offline" to accurately reflect the offline TTL: change
the wording around the `ble:gateway:<hostname>` reference so it clearly states
that offline gateway snapshots are retained for 60 seconds after disconnect
(instead of indefinitely), and mirror this correction in the later paragraph
that repeats the claim (the other occurrence describing gateway persistence).
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 376-385: In BLEManager.tsx, the table row that maps
Object.keys(adapterErrors) currently only shows a "Not available" Badge and a
"-" instead of the actual error text; update the mapping (the JSX that iterates
over adapterName) to render the adapter-specific message from
bleSettings.adapterErrors[adapterName] (safely handling null/undefined) in the
third cell, and keep or augment the Badge for status so admins see both the "Not
available" indicator and the detailed error message for each adapter.
- Around line 165-187: The WebSocket opened in the useEffect never retries,
causing a permanent disconnect after transient failures; update the useEffect
that creates the WebSocket (references: useEffect, wsRef, setWsConnected,
advCountRef, BLE_API) to implement automatic reconnects with exponential
backoff: encapsulate socket creation in a function (e.g., createWs) that assigns
wsRef.current, wires onopen/onmessage/onclose/onerror handlers, and on
onclose/onerror schedule a reconnect via setTimeout with increasing delay
(capped max) stored in a reconnectTimerRef so it can be cleared; ensure you
reset backoff on successful open, update setWsConnected appropriately, and in
the cleanup return clear the reconnect timer and close the current ws to avoid
leaks.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 116-133: The number input for localMaxGATTSlots allows values
outside the intended 1–10 range because min/max attributes don’t block manual
typing; update the onChange handler in BLESettings.tsx (the Form.Control using
id/name localMaxGATTSlots and setSettings) to clamp the parsed integer into the
allowed range (e.g., let v = parseInt(e.target.value); if NaN use default 3;
then v = Math.max(1, Math.min(10, v))) before calling setSettings so only 1–10
is persisted; ensure you still handle the prev null case as currently written.
In `@src/api/ble/index.ts`:
- Around line 488-493: The docblock describing "Release all GATT claims..." is
attached to _buildSettingsResponse() but belongs to
releaseGATTClaimsForProvider(); move that comment block so it sits immediately
above the releaseGATTClaimsForProvider() method, remove the misplaced block from
above _buildSettingsResponse(), and verify the doc text still accurately
references the providerId parameter and gateway WS disconnect behavior in
releaseGATTClaimsForProvider().
- Around line 508-519: The releaseGATTClaimsForProvider path deletes claims
without running the subscribeGATT cleanup that clears the BLEApi keepAliveTimer,
leaking intervals; modify releaseGATTClaimsForProvider so for each matching
claim it triggers the same cleanup as subscribeGATT (i.e., invoke the wrapped
close/cleanup path on claim.handle rather than just deleting the map entry) or
explicitly clear the claim.keepAliveTimer before deleting the claim, and then
fire the disconnect callback; reference functions/fields:
releaseGATTClaimsForProvider, subscribeGATT, claim.handle.close (the wrapped
close), and claim.keepAliveTimer to locate where to apply the fix.
- Around line 445-463: The synthetic GATT claim created in connectGATT leaves
this.gattClaims populated when conn disconnects; update the syntheticHandle to
(1) derive connected dynamically from conn.connected (not a one-time snapshot)
and (2) attach a conn.onDisconnect handler that removes the claim
(this.gattClaims.delete(mac)) and invokes any syntheticHandle.onDisconnect
callback; also ensure syntheticHandle.close removes that disconnect listener and
calls conn.disconnect(); modify the syntheticHandle construction in connectGATT
(and the stored claim object with pluginId/providerId) to register and
unregister this cleanup handler so claims are released automatically on
underlying disconnects.
In `@src/api/ble/localProvider.ts`:
- Around line 379-384: If executeGATTLifecycle(session) throws after the device
is connected, only deleting this.sessions (this.sessions.delete(mac)) leaves the
BlueZ connection open; update the catch to first attempt to cleanly tear down
the partially-open GATT connection (e.g., await a disconnect/close method on the
session object or call the agent's existing GATT teardown helper) and
swallow/handle any errors from that teardown, then remove the session from
this.sessions and rethrow the original error so the adapter slot is freed;
reference executeGATTLifecycle and this.sessions.delete(mac) when locating where
to add the teardown.
- Around line 281-338: The partial-attributes path currently forces rssi to -127
when RSSI isn't present, causing bad rankings; change the PropertiesChanged
handling so rssi is only updated when changedProps.RSSI exists (i.e., check for
changedProps.RSSI !== undefined) and otherwise preserve the previous rssi value
(or trigger a GetAll read if you want a fresh value) instead of resetting to
-127; keep the existing full-read behavior that uses device._propsProxy.GetAll
and unwrapVariant, and ensure the emitted BLEAdvertisement.rssi contains the
preserved or freshly-read value.
- Around line 214-216: availableGATTSlots() currently only subtracts
this.sessions.size and ignores raw GATT connections opened by connectGATT(),
allowing oversubscription; update availableGATTSlots() to also subtract a
tracked rawGATTConnections count (or equivalent) and ensure connectGATT()
increments/decrements that counter on open/close so slot accounting includes
both session-based and raw connections; also update the raw write()
implementation to respect the withResponse flag (call the BLE method for
write-with-response vs write-without-response or emulate waiting for a response
where supported) instead of always calling writeValue(), and adjust error
handling accordingly so characteristics that only support write-without-response
or write-with-response behave correctly.
- Around line 531-571: The disconnect handler is being added on every reconnect
and connectCallbacks get invoked twice; fix by ensuring the 'disconnect'
listener on session.device is attached only once (use session.device.once or
track a flag like session.disconnectListenerAttached) instead of re-registering
on each lifecycle run, and remove the duplicate firing of connect callbacks
after the setTimeout reconnect branch (since executeGATTLifecycle already
invokes connect callbacks). Update the code around
session.device.on('disconnect'), the reconnectTimer callback, and the
post-lifecycle loop that calls for (const cb of session.connectCallbacks) to
prevent double notifications.
In `@src/api/ble/openApi.json`:
- Around line 17-20: The OpenAPI spec's servers entry is pointing to the wrong
base path (currently "/signalk/v2/api/vessels/self/ble"), causing the gateway
endpoints (the paths defined around the gateway/gateways operations) to resolve
incorrectly; update the "servers" -> "url" value in src/api/ble/openApi.json to
the correct base path "/signalk/v2/api/ble" so the documented gateway endpoints
(the operations around the gateway/... and gateways paths referenced in the
diff) match the actual implementation and generated clients will call the
correct URLs.
- Around line 258-457: The OpenAPI spec is missing path entries for the
implemented endpoints documented in src/api/ble/index.ts: add definitions for
GET /consumers and GET /settings and PUT /settings to src/api/ble/openApi.json
under "paths"; for /consumers create a GET response returning application/json
with the correct schema (e.g., $ref to an existing or new BLEConsumers schema),
for /settings add GET and PUT entries where GET returns the settings schema and
PUT accepts a requestBody with the settings schema and appropriate responses
(200/400); ensure the path objects use the same tags/summaries/descriptions
style as other BLE paths and reference the exact handler semantics in index.ts
(consumers and settings handlers) so the OpenAPI contract matches the
implemented handlers.
In `@src/api/ble/openApi.ts`:
- Around line 4-7: The OpenAPI record bleApiRecord currently registers the wrong
base path; update the path property on bleApiRecord to use the BLE base path
'/signalk/v2/api/vessels/self/ble' (replace the existing
'/signalk/v2/api/vessels/self') so the BLE endpoints are correctly placed in
generated docs and routing metadata.
In `@src/api/ble/remoteProvider.ts`:
- Around line 546-548: The code currently only stops returning a gateway after
TTL but leaves its state in memory; update the cleanup path (the logic that
hides/expires gateways — triggered around getGatewayInfo and where
_registerGatewayProvider is called) to fully remove all related state for a
gatewayId: delete entries from sessions, seenMacs, advCallbacks,
httpGatewayMeta, lastPostTime, and unregister/remove the provider entry created
by _registerGatewayProvider; apply the same full-removal logic to the other
expiration site referenced (around lines 909-932) so stale/untrusted gateway IDs
are purged instead of merely hidden.
- Around line 823-826: The code calls session.handleDisconnect() after
releaseGATTClaims(`ble:gateway:${gatewayId}`), but releaseGATTClaims already
triggers each handle's _fireDisconnect() and thus invokes disconnectCallbacks;
remove the redundant session.handleDisconnect() call (or guard it behind a check
that ensures no claims were released) so disconnectCallbacks are only fired
once; update references around releaseGATTClaims and session.handleDisconnect to
ensure no other code paths rely on the extra call.
- Around line 522-526: The handler currently casts req.body to
BLEGatewayAdvertisementBatch and only checks gateway_id and devices array;
instead, validate the entire POST body (the parsed body variable) before
ingesting by asserting gateway_id is a non-empty string and devices is an array
whose elements each include a valid device_id, numeric rssi (finite number),
valid timestamp/age (or parseable), and metadata fields of expected types (or
strip unexpected keys). Update the validation logic in remoteProvider.ts around
the body/ BLEGatewayAdvertisementBatch handling to iterate devices and reject
the request with res.status(400).json({ error: ... }) if any device record fails
(include a concise error message), or normalize/coerce safe values before
continuing. Ensure downstream code that uses body.devices relies only on the
validated/normalized structure.
In `@src/dummysecurity.ts`:
- Around line 137-143: The dummy implementation of requestAccess currently
returns a Promise but never calls the callback that the WebSocket flow expects;
modify requestAccess so it still returns the Promise-resolved response object
but also invokes the provided updateCb with that response (i.e., construct the
response object, call updateCb(response) and then resolve/return it). Ensure the
function signature and behavior match callers that pass updateCb (see
requestAccess and updateCb usage in src/interfaces/ws.ts) so WS requests get a
callback-driven response.
---
Outside diff comments:
In `@src/api/index.ts`:
- Around line 110-121: The startApis function currently calls Promise.all([...])
without awaiting it, causing startup failures to become unhandled rejections;
change the startApis function signature to be async and await the Promise.all
call (which includes resourcesApi.start(), courseApi.start(),
weatherApi.start(), featuresApi.start(), autopilotApi.start(), radarApi.start(),
historyApiHttpRegistry.start(), notificationApi.start(), bleApi.start()) so that
errors propagate out of startApis instead of being swallowed, and then return
apiList after the await so startup failures surface to the caller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c0cc299-78e9-47aa-a4bb-6efd1d81a1ce
📒 Files selected for processing (28)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.jsonsrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
dirkwa
force-pushed
the
ble-provider-api
branch
from
12eb884 to
dbc4b8d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
src/api/ble/openApi.json (1)
17-20:⚠️ Potential issue | 🟡 MinorGateway endpoints still resolve to wrong URLs.
The server URL at line 19 is
/signalk/v2/api/vessels/self/ble, but gateway endpoints (/gateway/advertisements,/gateway/ws,/gateways) are actually mounted at/signalk/v2/api/ble/...per the implementation. While the descriptions mention the correct full paths, generated OpenAPI clients will construct incorrect URLs.Consider either:
- Moving gateway paths to a separate OpenAPI spec with the correct base URL, or
- Using absolute paths in the paths section for gateway endpoints
Also applies to: 402-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/openApi.json` around lines 17 - 20, The servers.url currently set to "/signalk/v2/api/vessels/self/ble" causes gateway endpoints to resolve incorrectly; update the OpenAPI so gateway paths (/gateway/advertisements, /gateway/ws, /gateways and others in lines ~402-449) resolve under "/signalk/v2/api/ble" by either changing the top-level "servers"[].url to "/signalk/v2/api/ble" or making each gateway path entry use absolute paths (prefixing them with "/signalk/v2/api/ble") or extracting the gateway paths into a separate spec with the correct base URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/develop/plugins/ble_provider_plugins.md`:
- Around line 13-18: Add a language identifier to the fenced ASCII diagram code
block under "Subscribing to Advertisements" so linting/rendering is satisfied;
edit the triple-backtick fence that currently precedes the diagram (near the
module.exports / plugin snippet) to use ```text (or ```plaintext) instead of
plain ```, leaving the diagram content unchanged.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 101-112: fetchDevices currently logs fetch failures but
fetchConsumers, fetchGateways, and fetchBleSettings silently swallow errors;
update those functions (fetchConsumers, fetchGateways, fetchBleSettings) to
handle errors consistently by adding error logging (e.g., console.error or
processLogger) in their catch blocks and/or rethrowing or returning a failure
signal so callers can react; ensure each fetch function includes a descriptive
message with the thrown error to preserve diagnostic context.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 37-57: The handleSave function currently sends a PUT to save BLE
settings but doesn't refresh local state afterwards; update handleSave to call
and await fetchSettings() when the response is ok (after the PUT completes
successfully) so the UI picks up any server-normalized values, and ensure
setSaving(false) still runs in the finally block; reference the handleSave
callback and fetchSettings helper when making this change.
In `@src/api/ble/index.ts`:
- Around line 275-300: Prune stale entries from device.seenBy in this update
block before recomputing device.rssi, device.lastSeen and before
selectGATTProvider() is relied on: remove any seenBy entries whose lastSeen is
older than a configurable threshold (e.g., now or adv.timestamp minus STALE_MS)
so old providers that stopped reporting don’t influence providerEntry selection;
then proceed to update or push the current providerEntry (adv/providerId), and
recompute device.rssi = Math.max(...) and device.lastSeen = Math.max(...) from
the pruned device.seenBy.
- Around line 202-220: The register(pluginId: string, provider: BLEProvider)
function currently leaves the old provider in this.bleProviders while
subscribing to the new provider and storing its unsubscriber in
this.providerUnsubscribers, causing a mismatch; either reject duplicate pluginId
or fully replace: check this.bleProviders.has(pluginId) and if duplicates are
not allowed, throw an error; if replacement is allowed, first retrieve and call
the existing unsubscriber from this.providerUnsubscribers to tear down the old
provider, then replace the entry in this.bleProviders with the new provider and
set the new unsubscriber returned by provider.methods.onAdvertisement(…) into
this.providerUnsubscribers (ensuring the unsub variable corresponds to the
stored provider), keeping registry state and teardown aligned.
In `@src/api/ble/localProvider.ts`:
- Around line 558-573: The reconnect logic in the session.reconnectTimer
currently attempts a single reconnect and then stops on failure, leaving the
session and BLE claim stuck; update the logic in the reconnect handler
(referencing session.reconnectTimer and this.executeGATTLifecycle) to retry
until success or a configurable maxAttempts/timeout: implement a retry/backoff
loop that reschedules session.reconnectTimer on failure (or increments an
attempt counter on the session), keep invoking session.connectCallbacks on
successful reconnect, and if max attempts or overall timeout is exceeded, ensure
you clean up by releasing the BLE API claim, remove the session from
this.sessions, clear timers, and mark session.closed so resources aren’t held
permanently.
- Around line 620-635: Reserve a raw GATT slot before starting the async connect
sequence: increment this.rawConnections (and ensure you can roll back on
failure) prior to calling connectQueue.enqueue/await this.adapter.waitDevice so
the slot is taken while the connect is in-flight; if connectQueue.enqueue,
device.gatt(), or the subsequent steps throw, decrement this.rawConnections and
avoid leaving a leaked reservation; also ensure the usual disconnect cleanup
(the disconnect callbacks and any decrement currently done after successful
connect) still runs when the connection succeeds. Use the existing symbols
connectQueue.enqueue, this.adapter.waitDevice, device.gatt(), rawConnections,
and the disconnect callbacks to locate where to add the reservation and rollback
logic.
- Around line 3-5: Update the header comment in localProvider.ts to reflect the
current behavior: state that the Local BLE Provider wraps BlueZ via
`@naugehyde/node-ble` and can be registered by BLEApi when the
localBluetoothManaged setting is enabled, and change the stated default from
`true` to `false` to match DEFAULT_BLE_SETTINGS.localBluetoothManaged; remove
any wording that implies it is automatically registered by default so the doc
describes current state rather than historical behavior.
- Line 1: Remove the file-wide "eslint-disable
`@typescript-eslint/no-explicit-any`" and instead add targeted
eslint-disable-next-line comments only where untyped D-Bus objects require any
(for example on the declarations/uses of adapter, bluetooth, device,
gattServer), change catch blocks from catch (e: any) to catch (e: unknown) and
narrow error handling, and update the ConnectionQueue type usages by adding
proper generics or typed callback signatures so its callbacks are strongly
typed; review other locals in this file and replace remaining broad any types
with precise interfaces or union types where possible, leaving inline eslint
disables only on the specific lines that truly need any.
In `@src/api/ble/remoteProvider.ts`:
- Around line 494-509: The new attach method exposes routes under GW_PATH
without the BLE API authorization; modify attach to apply the same authorization
middleware used by the BLE API before registering these endpoints (e.g., call
router.use(GW_PATH, <bleAuthMiddleware>) or add the middleware as the first
argument to router.get/router.post), so that GET `${GW_PATH}/gateways` and POST
`${GW_PATH}/gateway/advertisements` are protected, and ensure _attachWebSocket
also enforces the same auth for WebSocket connections.
In `@src/config/config.ts`:
- Around line 92-96: The BLE config interface currently declares
bleApi.localAdapter?: string but the runtime BLE initialization/selection logic
expects bleApi.localAdapters as an array; update the config shape by replacing
localAdapter?: string with localAdapters?: string[] in the bleApi interface
(symbol: bleApi, property: localAdapters) and then fix any code that referenced
localAdapter to use localAdapters (or adapt consumers to accept the array),
ensuring the type change compiles across the BLE initialization/selection logic
that consumes localAdapters.
In `@src/serverroutes.ts`:
- Around line 977-983: The handler updates
app.config.settings.bleApi.localBluetoothManaged from
settings.options.useBLEManager but never copies that change into
updatedSettings, so the persisted write (where app.config.settings =
updatedSettings occurs later) loses the change; modify the code handling
settings.options.useBLEManager to also set updatedSettings.bleApi (create
updatedSettings.bleApi if missing) and assign
updatedSettings.bleApi.localBluetoothManaged = settings.options.useBLEManager
(in addition to app.config.settings) so the change is included when
updatedSettings is written to disk.
---
Duplicate comments:
In `@src/api/ble/openApi.json`:
- Around line 17-20: The servers.url currently set to
"/signalk/v2/api/vessels/self/ble" causes gateway endpoints to resolve
incorrectly; update the OpenAPI so gateway paths (/gateway/advertisements,
/gateway/ws, /gateways and others in lines ~402-449) resolve under
"/signalk/v2/api/ble" by either changing the top-level "servers"[].url to
"/signalk/v2/api/ble" or making each gateway path entry use absolute paths
(prefixing them with "/signalk/v2/api/ble") or extracting the gateway paths into
a separate spec with the correct base URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf2f88f5-e2d5-49ec-9068-0edc1fe5e474
📒 Files selected for processing (28)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.jsonsrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
dirkwa
force-pushed
the
ble-provider-api
branch
from
dbc4b8d to
5b4866c
Compare
Contributor
Author
|
@coderabbitai All findings from both review rounds have been addressed and squashed into the 7 logical commits. Round 1 (all 20 fixed): Round 2 (8 fixed, 3 intentionally skipped):
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The rationale for the three intentionally skipped findings is sound:
The BLE Provider API is a substantial addition — covering core API, local BlueZ, remote ESP32 gateway, Admin UI, OpenAPI spec, and documentation in a cohesive set of commits. Everything looks good to proceed. 🐇✨
🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (7)
src/config/config.ts (1)
92-96:⚠️ Potential issue | 🟠 MajorAlign
bleApiconfig typing with runtimelocalAdaptersshape.At Line 94, the config type declares
localAdapter?: string, but BLE runtime settings and update handlers operate onlocalAdapters: string[]. This mismatch weakens type safety and can mask incorrect settings payloads.Suggested fix
bleApi?: { localBluetoothManaged?: boolean - localAdapter?: string + localAdapters?: string[] localMaxGATTSlots?: number }As per coding guidelines: "When converting JavaScript to TypeScript, use pre-existing types when possible instead of creating new local types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/config.ts` around lines 92 - 96, Update the bleApi config type so it matches the runtime shape: replace the property localAdapter?: string with localAdapters?: string[] (i.e., an array of adapter IDs) and, if there's an existing type representing adapter lists in the codebase, reuse that type instead of declaring a new one; keep localBluetoothManaged and localMaxGATTSlots as-is to preserve existing settings.src/api/ble/remoteProvider.ts (3)
494-509:⚠️ Potential issue | 🟠 MajorProtect the gateway routes with the same auth gate as the rest of the BLE API.
These handlers are mounted outside the main BLE router auth path, so
GET /signalk/v2/api/ble/gatewaysis currently public and the POST route only does a partial token check. That bypasses the security contract the rest of this API uses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/remoteProvider.ts` around lines 494 - 509, The gateway routes registered in attach (GW_PATH) are mounted outside the BLE API auth gate, making GET /signalk/v2/api/ble/gateways public and the POST using only a partial check; wrap or mount these endpoints with the same authentication middleware used by the main BLE router so both router.get(`${GW_PATH}/gateways`, ...) and router.post(`${GW_PATH}/gateway/advertisements`, ...) call the full auth/token verification before invoking getGatewayInfo or _handleAdvertisementPost, and ensure _attachWebSocket is also only enabled after successful auth setup.
525-598:⚠️ Potential issue | 🟠 MajorValidate the full gateway POST body before ingesting it.
This still trusts
req.body as BLEGatewayAdvertisementBatchafter only checkinggateway_idandArray.isArray(body.devices). Invalid nested fields can still flow intoparseAdvData,Object.entries(...), ormanufacturerData[parseInt(id)], which means bad payloads can poison gateway/device state instead of being rejected cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/remoteProvider.ts` around lines 525 - 598, The handler currently only checks gateway_id and that devices is an array but still trusts req.body as BLEGatewayAdvertisementBatch; add full validation early (before any state mutation or calling parseAdvData) to reject malformed nested fields—verify each device in body.devices is an object with a string mac, numeric rssi, and optional fields of the expected types (adv_data string, manufacturer_data object with numeric-string keys and hex strings, service_data object of strings, service_uuids array of strings, connectable boolean, tx_power number) and validate top-level optional fields (mac, hostname, firmware, uptime, free_heap) types; if validation fails return 400 and do not touch this.httpGatewayMeta, this.sessions, this.seenMacs, or call parseAdvData/_registerGatewayProvider. Use the existing symbols (BLEGatewayAdvertisementBatch, parseAdvData, _registerGatewayProvider, this.httpGatewayMeta, this.sessions, this.seenMacs, this.advCallbacks) to locate and guard the logic.
916-924:⚠️ Potential issue | 🟠 MajorFully purge expired HTTP-only gateways, not just their visible metadata.
When the POST TTL expires, this deletes the per-gateway device/meta maps but leaves the registered
ble:gateway:${gatewayId}provider and itsadvCallbacksentry behind. The gateway vanishes fromgetGatewayInfo(), but stale provider IDs and callback state still accumulate indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/remoteProvider.ts` around lines 916 - 924, The purge branch that handles expired POST TTL currently removes seenMacs, lastPostTime and httpGatewayMeta but leaves the registered provider and callback state behind; inside the Date.now() - lastPost > OFFLINE_SNAPSHOT_MS branch (the loop iterating over this.seenMacs and checking this.sessions/this.snapshots), also remove the provider entry for the gateway and its advertising callbacks by deleting the registered provider id (e.g. this.providers.delete(`ble:gateway:${gatewayId}`)) and the adv callback state (e.g. this.advCallbacks.delete(gatewayId)) so stale provider IDs and advCallbacks are fully purged along with httpGatewayMeta, lastPostTime, and seenMacs.src/api/ble/openApi.json (1)
17-20:⚠️ Potential issue | 🟠 MajorThe gateway operations are still documented under the wrong base path.
With
servers[0].urlset to/signalk/v2/api/vessels/self/ble, these gateway paths resolve to non-existent URLs. The implementation mounts them under/signalk/v2/api/ble, so generated clients will call the wrong endpoints.#!/bin/bash python - <<'PY' import json with open('src/api/ble/openApi.json', 'r', encoding='utf-8') as f: doc = json.load(f) base = doc["servers"][0]["url"].rstrip("/") for path in ["/gateway/advertisements", "/gateway/ws", "/gateways"]: print(f"{path} -> {base}{path}") PYExpected result: the printed URLs include
/signalk/v2/api/vessels/self/ble/..., which shows why these operations do not match the implemented/signalk/v2/api/ble/...routes.Also applies to: 402-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/openApi.json` around lines 17 - 20, The OpenAPI document's servers[0].url is incorrect: change the URL value currently set to "/signalk/v2/api/vessels/self/ble" to the actual mount base "/signalk/v2/api/ble" so gateway operations resolve to the implemented endpoints; update any other server/path entries in the same file (the block mentioned around the other occurrences) that reference the old "/signalk/v2/api/vessels/self/ble" to the new "/signalk/v2/api/ble" so paths like "/gateway/advertisements", "/gateway/ws" and "/gateways" generate correct client URLs.src/serverroutes.ts (1)
977-983:⚠️ Potential issue | 🟠 MajorPersist
useBLEManagerintoupdatedSettings, notapp.config.settings.This branch still writes the BLE toggle to
app.config.settings.bleApi, but the handler persistsupdatedSettingsand then replacesapp.config.settingswith it at Line 1021. The new value is therefore dropped both on disk and in memory after a successful save.Suggested fix
if (!isUndefined(settings.options.useBLEManager)) { - if (!app.config.settings.bleApi) { - app.config.settings.bleApi = {} + if (!updatedSettings.bleApi) { + updatedSettings.bleApi = {} } - app.config.settings.bleApi.localBluetoothManaged = + updatedSettings.bleApi.localBluetoothManaged = settings.options.useBLEManager }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serverroutes.ts` around lines 977 - 983, The BLE toggle is being written to app.config.settings.bleApi but the handler persists updatedSettings and then replaces app.config.settings, so the change is lost; instead assign the value from settings.options.useBLEManager onto updatedSettings (e.g. updatedSettings.settings.bleApi.localBluetoothManaged or updatedSettings.options/bleApi path matching your settings shape) rather than app.config.settings.bleApi so the persisted object contains the new localBluetoothManaged flag before the save that replaces app.config.settings.src/api/ble/localProvider.ts (1)
558-571:⚠️ Potential issue | 🟠 MajorDon't fire
onConnecttwice after a successful reconnect.
executeGATTLifecycle()already invokes the registered connect callbacks at Lines 588-594. The extra loop here sends a secondonConnectevent after every successful reconnect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/localProvider.ts` around lines 558 - 571, The reconnect logic in attemptReconnect is calling session.connectCallbacks twice because executeGATTLifecycle(session) already invokes the connect callbacks; remove the redundant for-loop that iterates over session.connectCallbacks (or add a guard so callbacks are only invoked in one place) inside attemptReconnect to prevent duplicate onConnect events—look for the attemptReconnect function and the executeGATTLifecycle(session) call and ensure only executeGATTLifecycle or the explicit loop (not both) triggers session.connectCallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/develop/rest-api/ble_api.md`:
- Around line 146-147: Update the documentation bullet that currently reads
"**Status row**: gateway count, consumer count, device count, live advertisement
rate." to accurately reflect shipped behavior by replacing "live advertisement
rate" with "cumulative advertisement count since page load" (or similar
phrasing) so the docs describe current UI; ensure the related "BLE Gateways"
line remains unchanged unless it also implies a live rate.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 496-499: The render is mutating state by calling devices.sort(...)
directly; clone the devices array before sorting to avoid in-place mutation
during render (e.g., use a shallow copy of devices and sort that) and then map
over the cloned/sorted array in the JSX where devices.sort(...) and <tr
key={device.mac}> are used so the original state remains immutable.
- Around line 172-181: The WebSocket handlers in connect() close over the
mutable ws variable, causing old socket events to affect new sockets; fix by
creating a local const socket = new WebSocket(wsUrl), assign ws = socket and
wsRef.current = socket, and attach onopen/onclose/onerror directly to that
socket (and where you need to compare the active socket, check socket ===
wsRef.current) so old socket callbacks cannot close or toggle state on a new
connection; also replace ws?.close() in onerror with socket.close() (or
conditional on socket === wsRef.current before closing) and ensure
reconnectTimer is set only when the closed socket is still the current one.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 152-157: The "Restart Required" Badge in the BLESettings component
is stale and should be removed: locate the JSX in BLESettings where the Badge
with text "Restart Required" is rendered (inside the supported conditional) and
delete that Badge element and its surrounding unnecessary whitespace/fragment so
the UI no longer shows the warning; also remove any now-unused Badge import if
it becomes unused.
In `@packages/server-api/src/bleapi.ts`:
- Around line 89-101: The typed contract GATTSubscriptionHandle must expose
provider-offline signaling instead of relying on an any cast; add a public
method (e.g. fireDisconnect(): void or signalProviderOffline(): void) to the
GATTSubscriptionHandle interface and implementers, and update callers (remove
casts to (claim.handle as any)._fireDisconnect) to call that method; keep
close() semantics unchanged (do not fold this into close()) so
onDisconnect/onConnect callbacks can be invoked by the provider via the new
public method.
In `@src/api/ble/index.ts`:
- Around line 358-360: getDevice currently returns the cached BLEDeviceInfo from
deviceTable without updating its gattClaimedBy, causing stale claim state;
modify getDevice(mac: string) to uppercase the mac, fetch the device from
this.deviceTable, and if found overwrite or set device.gattClaimedBy from
this.gattClaims.get(macUpper) ?? null before returning (or return a shallow copy
with the updated gattClaimedBy) so the /devices/:mac endpoint stays in sync with
current claim state; reference functions/fields: getDevice, getDevices,
deviceTable, gattClaims, BLEDeviceInfo.
In `@src/api/ble/localProvider.ts`:
- Around line 186-195: stopDiscovery() only calls BlueZ StopDiscovery but leaves
internal timers/listeners running; update stopDiscovery to also stop and clear
the watcher interval created by startDeviceWatcher (clearInterval/clearTimeout
on the watcher handle), remove all per-device PropertiesChanged listeners added
in attachDeviceListener (iterate the device listener map and call the
corresponding remove/off method or detach helper), cancel any advertisement
emission timers started by emitDeviceAdvertisement, and clear any cached
device/advertisement state so no further advertisements are emitted after
scanning is false; ensure you reference and clear the same fields used elsewhere
(e.g., watcher interval variable from startDeviceWatcher, the deviceListeners
map from attachDeviceListener, and the advertisement timer/cache used by
emitDeviceAdvertisement) before returning.
- Around line 166-183: In startDiscovery (localProvider.startDiscovery) don’t
swallow StartDiscovery errors: either remove the try/catch or re-throw the
caught error after logging so callers (e.g., code in src/api/ble/index.ts that
assumes discovery succeeded) can detect failure; also ensure this.scanning
remains false on error (clear or never set it) and keep the debug log before
re-throwing to preserve context.
- Around line 637-655: The code increments raw connection capacity when
reserving a slot but only calls releaseSlot() if connectQueue.enqueue throws;
ensure releaseSlot() is also called if subsequent setup fails (e.g., after await
device.gatt() or while installing the disconnect listener). Wrap the
post-connect setup (the await device.gatt() call and the disconnect-listener
installation logic) in a try/catch/finally or try/catch that calls releaseSlot()
on any failure, and also cleanly disconnect or close the device if needed;
update functions referenced (connectQueue.enqueue, adapter.waitDevice,
device.gatt, the disconnect-listener setup) so that releaseSlot() runs on all
error paths to avoid leaking rawConnections/GATT capacity.
In `@src/api/ble/openApi.json`:
- Around line 423-429: The OpenAPI operations for get on paths "/gateway/ws" and
"/advertisements" are missing the required responses object; add a responses
object to each get operation (e.g., include at least a 101 "Switching Protocols"
response with a short description and an optional default/200 fallback) so the
Operation Objects are valid—update the get block for "/gateway/ws" and the get
block for "/advertisements" to include these responses entries.
In `@src/api/ble/openApi.ts`:
- Around line 4-8: Import and register the exported bleApiRecord in the OpenAPI
registration so the BLE API appears in the UI: add an import for bleApiRecord
(symbol name bleApiRecord) into the module that defines apiDocs (where other
records like courseApiRecord, resourcesApiRecord, weatherApiRecord,
radarApiRecord are imported) and include bleApiRecord in the apiDocs array
alongside the other API records; ensure the import uses the same exported symbol
from src/api/ble/openApi.ts and that apiDocs is updated to include it.
In `@src/api/ble/remoteProvider.ts`:
- Around line 327-340: The code building remote command (variable cmd) drops or
mis-translates non-default GATTSubscriptionDescriptor options; preserve and
forward poll.writeBeforeRead and periodicWrite[].withResponse and translate
camelCase fields in descriptor.init to the wire snake_case format (e.g.,
intervalMs -> interval_ms, writeBeforeRead -> write_before_read, withResponse ->
with_response) so the gateway receives the full descriptor; update the mapping
in the block that constructs cmd.poll, cmd.periodic_write and cmd.init
(referencing descriptor, cmd, poll.map, periodicWrite.map, and init processing)
to copy these fields through with the proper snake_case keys and only omit
undefined values.
---
Duplicate comments:
In `@src/api/ble/localProvider.ts`:
- Around line 558-571: The reconnect logic in attemptReconnect is calling
session.connectCallbacks twice because executeGATTLifecycle(session) already
invokes the connect callbacks; remove the redundant for-loop that iterates over
session.connectCallbacks (or add a guard so callbacks are only invoked in one
place) inside attemptReconnect to prevent duplicate onConnect events—look for
the attemptReconnect function and the executeGATTLifecycle(session) call and
ensure only executeGATTLifecycle or the explicit loop (not both) triggers
session.connectCallbacks.
In `@src/api/ble/openApi.json`:
- Around line 17-20: The OpenAPI document's servers[0].url is incorrect: change
the URL value currently set to "/signalk/v2/api/vessels/self/ble" to the actual
mount base "/signalk/v2/api/ble" so gateway operations resolve to the
implemented endpoints; update any other server/path entries in the same file
(the block mentioned around the other occurrences) that reference the old
"/signalk/v2/api/vessels/self/ble" to the new "/signalk/v2/api/ble" so paths
like "/gateway/advertisements", "/gateway/ws" and "/gateways" generate correct
client URLs.
In `@src/api/ble/remoteProvider.ts`:
- Around line 494-509: The gateway routes registered in attach (GW_PATH) are
mounted outside the BLE API auth gate, making GET /signalk/v2/api/ble/gateways
public and the POST using only a partial check; wrap or mount these endpoints
with the same authentication middleware used by the main BLE router so both
router.get(`${GW_PATH}/gateways`, ...) and
router.post(`${GW_PATH}/gateway/advertisements`, ...) call the full auth/token
verification before invoking getGatewayInfo or _handleAdvertisementPost, and
ensure _attachWebSocket is also only enabled after successful auth setup.
- Around line 525-598: The handler currently only checks gateway_id and that
devices is an array but still trusts req.body as BLEGatewayAdvertisementBatch;
add full validation early (before any state mutation or calling parseAdvData) to
reject malformed nested fields—verify each device in body.devices is an object
with a string mac, numeric rssi, and optional fields of the expected types
(adv_data string, manufacturer_data object with numeric-string keys and hex
strings, service_data object of strings, service_uuids array of strings,
connectable boolean, tx_power number) and validate top-level optional fields
(mac, hostname, firmware, uptime, free_heap) types; if validation fails return
400 and do not touch this.httpGatewayMeta, this.sessions, this.seenMacs, or call
parseAdvData/_registerGatewayProvider. Use the existing symbols
(BLEGatewayAdvertisementBatch, parseAdvData, _registerGatewayProvider,
this.httpGatewayMeta, this.sessions, this.seenMacs, this.advCallbacks) to locate
and guard the logic.
- Around line 916-924: The purge branch that handles expired POST TTL currently
removes seenMacs, lastPostTime and httpGatewayMeta but leaves the registered
provider and callback state behind; inside the Date.now() - lastPost >
OFFLINE_SNAPSHOT_MS branch (the loop iterating over this.seenMacs and checking
this.sessions/this.snapshots), also remove the provider entry for the gateway
and its advertising callbacks by deleting the registered provider id (e.g.
this.providers.delete(`ble:gateway:${gatewayId}`)) and the adv callback state
(e.g. this.advCallbacks.delete(gatewayId)) so stale provider IDs and
advCallbacks are fully purged along with httpGatewayMeta, lastPostTime, and
seenMacs.
In `@src/config/config.ts`:
- Around line 92-96: Update the bleApi config type so it matches the runtime
shape: replace the property localAdapter?: string with localAdapters?: string[]
(i.e., an array of adapter IDs) and, if there's an existing type representing
adapter lists in the codebase, reuse that type instead of declaring a new one;
keep localBluetoothManaged and localMaxGATTSlots as-is to preserve existing
settings.
In `@src/serverroutes.ts`:
- Around line 977-983: The BLE toggle is being written to
app.config.settings.bleApi but the handler persists updatedSettings and then
replaces app.config.settings, so the change is lost; instead assign the value
from settings.options.useBLEManager onto updatedSettings (e.g.
updatedSettings.settings.bleApi.localBluetoothManaged or
updatedSettings.options/bleApi path matching your settings shape) rather than
app.config.settings.bleApi so the persisted object contains the new
localBluetoothManaged flag before the save that replaces app.config.settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 896c566a-5eb4-4a5f-abda-7804ad4fec62
📒 Files selected for processing (28)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.jsonsrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (17)
src/config/config.ts (1)
92-96:⚠️ Potential issue | 🟠 MajorAlign BLE adapter config key naming before merge.
At Line 94,
localAdapter?: stringmay not match the adapter-management shape used elsewhere (localAdaptersarray). If both shapes exist without explicit mapping, type checks won’t protect persisted settings compatibility.Proposed type shape
bleApi?: { localBluetoothManaged?: boolean - localAdapter?: string + localAdapters?: string[] + localAdapter?: string // optional legacy compatibility key localMaxGATTSlots?: number }#!/bin/bash # Verify adapter key usage consistency across the repo rg -nP --type=ts -C2 '\blocalAdapters?\b'Expected result: a single canonical key, or explicit legacy mapping at boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/config.ts` around lines 92 - 96, The bleApi config currently defines localAdapter?: string which conflicts with the rest of the codebase expecting localAdapters (an array) — update the config shape to the canonical form (e.g., change bleApi.localAdapter to localAdapters?: string[]), and update any loader/serializer logic to accept the legacy single-string key by mapping it into the new array (handle both in the code paths that read/write the config). Make sure to update references to bleApi.localAdapter and any consumers to use bleApi.localAdapters (or add a conversion helper) so type checks and persisted settings remain compatible.src/serverroutes.ts (1)
977-983:⚠️ Potential issue | 🔴 Critical
useBLEManageris written to the wrong object and gets lost on save.At Line 977–983 you mutate
app.config.settings.bleApi, but Line 1017 persistsupdatedSettings, and Line 1021 replaces runtime settings withupdatedSettings. Result: the BLE manager toggle is not persisted.🔧 Proposed fix
if (!isUndefined(settings.options.useBLEManager)) { - if (!app.config.settings.bleApi) { - app.config.settings.bleApi = {} + if (!updatedSettings.bleApi) { + updatedSettings.bleApi = {} } - app.config.settings.bleApi.localBluetoothManaged = + updatedSettings.bleApi.localBluetoothManaged = settings.options.useBLEManager }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serverroutes.ts` around lines 977 - 983, The code writes settings.options.useBLEManager into app.config.settings.bleApi (app.config.settings.bleApi.localBluetoothManaged) which never gets persisted because later the code persists updatedSettings and replaces runtime settings with updatedSettings; move the assignment so the BLE toggle is saved to the persisted settings object instead: ensure updatedSettings.options exists and set updatedSettings.options.useBLEManager = settings.options.useBLEManager (or copy into updatedSettings.settings/appropriate persisted subtree) before the persistence step that uses updatedSettings, leaving app.config only for runtime copies like localBluetoothManaged if still needed.src/api/ble/openApi.ts (1)
4-8:⚠️ Potential issue | 🟠 MajorRegister
bleApiRecordin Swagger aggregation or BLE docs stay hidden.
bleApiRecordis defined at Line 4, but it is still missing fromsrc/api/swagger.ts(apiDocs), so/doc/openapiwill not expose BLE endpoints.🔧 Proposed fix (in
src/api/swagger.ts)import { historyApiRecord } from './history/openApi' import { radarApiRecord } from './radar/openApi' +import { bleApiRecord } from './ble/openApi' const apiDocs = [ discoveryApiRecord, appsApiRecord, autopilotApiRecord, courseApiRecord, notificationsApiRecord, resourcesApiRecord, weatherApiRecord, securityApiRecord, historyApiRecord, - radarApiRecord + radarApiRecord, + bleApiRecord ].reduce<ApiRecords>((acc, apiRecord: OpenApiRecord) => { acc[apiRecord.name] = apiRecord return acc }, {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/openApi.ts` around lines 4 - 8, The BLE OpenAPI record bleApiRecord is defined but not included in the Swagger aggregation, so the BLE endpoints won't appear; to fix, import bleApiRecord where the API docs are aggregated (the apiDocs array) and add bleApiRecord to that apiDocs array (and export if needed) so the OpenAPI aggregator includes the BLE spec.docs/develop/rest-api/ble_api.md (1)
146-147:⚠️ Potential issue | 🟡 MinorThe documentation describes "live advertisement rate" but the UI shows a cumulative count.
Line 146 states the status row displays a "live advertisement rate," but
BLEManager.tsxincrementsadvCountRefon each WebSocket message and displays the total count since page load, not a rate. Update the documentation to match the actual behavior.🔧 Suggested fix
-- **Status row**: gateway count, consumer count, device count, live advertisement rate. +- **Status row**: gateway count, consumer count, device count, advertisement count since page load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/develop/rest-api/ble_api.md` around lines 146 - 147, The docs incorrectly call the UI metric a "live advertisement rate"; update the wording in ble_api.md so it describes the actual behavior: the status row shows a cumulative advertisement count (total ads since page load), matching the implementation in BLEManager.tsx where advCountRef is incremented on each WebSocket message and rendered as a running total rather than a rate. Mention advCountRef and BLEManager.tsx in the docs change so readers can cross-reference the component behavior.src/api/ble/openApi.json (2)
423-429:⚠️ Potential issue | 🟡 MinorWebSocket operations still missing required
responsesfield.OpenAPI 3.0 Operation Objects require a
responsesfield. Both WebSocket upgrade endpoints (GET /gateway/wsat lines 423-429 andGET /advertisementsat lines 544-549) lack this required field, making the spec invalid.🔧 Proposed fix
Add minimal responses to both WebSocket operations:
"/gateway/ws": { "get": { "tags": ["Gateway"], "summary": "WebSocket GATT command channel for ESP32 gateways", - "description": "Upgrade to WebSocket for GATT subscribe/data/disconnect messaging. Full path: /signalk/v2/api/ble/gateway/ws" + "description": "Upgrade to WebSocket for GATT subscribe/data/disconnect messaging. Full path: /signalk/v2/api/ble/gateway/ws", + "responses": { + "101": { "description": "Switching Protocols" } + } } },"/advertisements": { "get": { "tags": ["BLE"], "summary": "WebSocket stream of BLE advertisements", - "description": "Upgrade to WebSocket to receive a real-time stream of BLEAdvertisement JSON objects from all registered providers." + "description": "Upgrade to WebSocket to receive a real-time stream of BLEAdvertisement JSON objects from all registered providers.", + "responses": { + "101": { "description": "Switching Protocols" } + } } }Also applies to: 544-549
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/openApi.json` around lines 423 - 429, The OpenAPI Operation objects for the WebSocket endpoints (GET on path "/gateway/ws" and GET on path "/advertisements") are missing the required responses field; add a minimal responses object to each operation (e.g., include a 101 Switching Protocols response for successful WebSocket upgrades and at least a default or 400/500 error response) so the spec is valid, updating the operations under the "/gateway/ws" and "/advertisements" paths accordingly.
17-20:⚠️ Potential issue | 🟠 MajorGateway endpoints documented under the wrong server base path.
The
servers.urlis set to/signalk/v2/api/vessels/self/ble(line 19), but gateway endpoints like/gateway/advertisements,/gateway/ws, and/gatewaysare actually mounted at/signalk/v2/api/ble/...according to the descriptions. Generated API clients will call non-existent URLs because OpenAPI resolves paths relative to the server URL.Consider either:
- Moving gateway paths to a separate OpenAPI document with the correct base URL, or
- Documenting the gateway endpoints with their full absolute paths and adding a note about the separate mount point
Also applies to: 402-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/openApi.json` around lines 17 - 20, The OpenAPI servers.url is set to "/signalk/v2/api/vessels/self/ble" but gateway routes (e.g., "/gateway/advertisements", "/gateway/ws", "/gateways") are actually mounted under "/signalk/v2/api/ble", so generated clients will build wrong base URLs; fix by either (A) moving all gateway path entries into a separate OpenAPI document whose servers.url is "/signalk/v2/api/ble" (keeping vessel BLE routes in the current document), or (B) keep a single document but change the gateway path entries to use absolute paths (full "/signalk/v2/api/ble/gateway/..." values) or change the servers.url to a common base like "/signalk/v2/api" and adjust paths accordingly; update references to servers.url and gateway path definitions (look for "servers": [...], and path keys "/gateway/advertisements", "/gateway/ws", "/gateways") to implement one of these fixes.packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx (2)
496-499:⚠️ Potential issue | 🟡 MinorAvoid mutating React state during render.
devices.sort(...)mutates the state array in place. Clone the array before sorting to keep render side-effect free.🔧 Suggested fix
- {devices - .sort((a, b) => (b.rssi ?? -999) - (a.rssi ?? -999)) + {[...devices] + .sort((a, b) => (b.rssi ?? -999) - (a.rssi ?? -999)) .map((device) => (As per coding guidelines, "Prefer immutable data structures where practical".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` around lines 496 - 499, The render is mutating React state by calling devices.sort(...) directly; make a non-mutating copy before sorting (e.g., use [...devices] or devices.slice()) and then sort that copy prior to mapping so you don't modify the original devices array in the BLEManager render pipeline; update the expression around devices.sort(...) used before .map(...) to operate on the copied array to preserve immutability.
172-185:⚠️ Potential issue | 🟡 MinorWebSocket handlers close over mutable
wsvariable.The handlers assigned to
ws(lines 176-184) close over the mutablewsvariable. Ifconnect()is called again before old socket events fire,ws.onerror = () => ws?.close()will close the new socket instead of the old one.🔧 Suggested fix
const connect = () => { if (disposed) return - ws = new WebSocket(wsUrl) - wsRef.current = ws - ws.onopen = () => setWsConnected(true) - ws.onclose = () => { + const socket = new WebSocket(wsUrl) + ws = socket + wsRef.current = socket + socket.onopen = () => setWsConnected(true) + socket.onclose = () => { setWsConnected(false) if (!disposed) reconnectTimer = setTimeout(connect, 5000) } - ws.onerror = () => ws?.close() - ws.onmessage = () => { + socket.onerror = () => socket.close() + socket.onmessage = () => { advCountRef.current += 1 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` around lines 172 - 185, The WebSocket handlers in connect() close over the mutable ws variable causing a new socket to be closed by old handlers; change connect() to capture the newly created socket in a local constant (e.g., const socket = new WebSocket(wsUrl)) and bind handlers to that local variable (socket.onopen, socket.onclose, socket.onerror, socket.onmessage), update wsRef.current = socket, and when scheduling reconnects or closing ensure you reference socket (or check wsRef.current === socket) so old handlers cannot affect a newer socket; also clear or null out reconnectTimer and remove handlers on dispose to avoid stale closures (references: connect, ws, wsRef, ws.onopen, ws.onclose, ws.onerror, ws.onmessage, reconnectTimer, disposed, advCountRef).packages/server-api/src/bleapi.ts (1)
89-102: 🧹 Nitpick | 🔵 TrivialConsider exposing provider-offline signaling in the typed handle contract.
The
GATTSubscriptionHandleinterface definesonDisconnect(callback)for consumers to register handlers, but providers have no public method to signal forced disconnects (e.g., when a provider goes offline). Based on context snippet 2 fromsrc/api/ble/index.ts, the implementation falls back to(claim.handle as any)._fireDisconnect.Consider adding a method like
signalDisconnect(): voidto the interface, or documenting thatclose()will invoke registeredonDisconnectcallbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-api/src/bleapi.ts` around lines 89 - 102, The interface GATTSubscriptionHandle currently exposes onDisconnect/onConnect but has no typed way for providers to signal a forced disconnect (implementations currently call (claim.handle as any)._fireDisconnect); add a public method to the interface (e.g., signalDisconnect(): void or notifyProviderOffline(): void) so providers can invoke it instead of using a cast to any, and update implementations that currently call _fireDisconnect to implement this new method; alternatively, if you want to keep a single semantic entrypoint, document that close() will synchronously invoke registered onDisconnect callbacks and ensure implementations call those callbacks from close() rather than relying on an untyped private method.packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (1)
152-157:⚠️ Potential issue | 🟡 MinorRemove the stale "Restart Required" badge.
The PUT
/settingshandler insrc/api/ble/index.ts(lines 766-772) applies provider changes immediately by callingshutdownLocalProviders()andinitLocalProviders(). The "Restart Required" badge misleads admins into thinking they need to restart the server.🔧 Suggested fix
<Card.Footer> <Button size="sm" variant="primary" onClick={handleSave} disabled={saving || !supported} > <FontAwesomeIcon icon={faFloppyDisk} /> Save </Button> - {supported && ( - <> - {' '} - <Badge bg="danger" className="float-end"> - Restart Required - </Badge> - </> - )} </Card.Footer>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 152 - 157, Remove the stale "Restart Required" Badge from the BLESettings UI: locate the JSX in BLESettings.tsx that renders the conditional block using the supported variable (the fragment that contains <Badge bg="danger" className="float-end">Restart Required</Badge>) and delete that Badge (and surrounding stray whitespace/fragment if now unnecessary) so the UI no longer displays a restart warning when provider changes are applied immediately by shutdownLocalProviders() and initLocalProviders().src/api/ble/localProvider.ts (4)
166-184:⚠️ Potential issue | 🟠 MajorDiscovery startup failures are still swallowed.
When
StartDiscoveryfails, the method logs and returns without re-throwing. Callers (likesrc/api/ble/index.tsline 169) won't know discovery failed, leaving the adapter in a half-initialized state.As per coding guidelines "Handle errors explicitly at system boundaries".
🔧 Suggested fix
} catch (e: any) { debug(`Error starting discovery: ${e.message}`) + throw e } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/localProvider.ts` around lines 166 - 184, In startDiscovery(), do not swallow errors: when adapter.helper.callMethod('StartDiscovery') (and related calls) throws, log the error using debug but then re-throw or propagate it so callers (e.g., the code that invokes startDiscovery in src/api/ble/index.ts) can handle the failure; update the catch block in async startDiscovery() to either throw e after debug(`Error starting discovery: ${e.message}`) or return a rejected Promise, ensuring this.adapter.scanning is not left true and startDeviceWatcher() is only called on success.
655-671:⚠️ Potential issue | 🟠 MajorRaw slot leaked if
device.gatt()or disconnect listener setup fails.The slot is reserved at line 632, but if
device.gatt()(line 655) or the subsequent setup throws,releaseSlot()isn't called. The slot remains reserved and the connection may stay open.🔧 Suggested fix
- const gattServer = await device.gatt() - let connected = true - const disconnectCallbacks: Array<() => void> = [] - - device.on('disconnect', () => { + let connected = true + const disconnectCallbacks: Array<() => void> = [] + let gattServer: any + try { + gattServer = await device.gatt() + device.on('disconnect', () => { + if (connected) { + connected = false + releaseSlot() + } + for (const cb of disconnectCallbacks) { + try { + cb() + } catch (_e) { + // ignore + } + } + }) + } catch (e) { + try { + await device.disconnect() + } catch (_e) { + // ignore + } + releaseSlot() + throw e + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/localProvider.ts` around lines 655 - 671, The slot reserved earlier can leak if device.gatt() or the listener registration throws: ensure releaseSlot() is always called on error by wrapping the await device.gatt() and the device.on('disconnect', ...) setup in a try/catch/finally (or try { ... } catch (err) { releaseSlot(); throw err } ) so releaseSlot() runs when gatt() or listener setup fails; also ensure any partially-registered state (e.g., connected flag or added listener in device.on) is cleaned up in the same error path so disconnectCallbacks and connected are consistent after failure (references: device.gatt(), device.on('disconnect', ...), releaseSlot(), connected, disconnectCallbacks).
558-584:⚠️ Potential issue | 🟠 MajorConnect callbacks fire twice on reconnect.
After a successful reconnect,
executeGATTLifecycle()firesconnectCallbacksat lines 588-594, and then the reconnect handler fires them again at lines 565-571. This causes duplicateonConnectnotifications.🔧 Suggested fix — remove duplicate callback invocation in reconnect handler
try { await this.executeGATTLifecycle(session) - for (const cb of session.connectCallbacks) { - try { - cb() - } catch (_e) { - // ignore - } - } } catch (e: any) {Also applies to: 588-594
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/localProvider.ts` around lines 558 - 584, The reconnect flow is invoking session.connectCallbacks twice: executeGATTLifecycle(session) already triggers the connect callbacks, and attemptReconnect's success block also iterates session.connectCallbacks; remove the duplicate invocation in attemptReconnect (or guard it with a check like session.connected) so callbacks are triggered only once by executeGATTLifecycle. Update the attemptReconnect function (the success branch after await this.executeGATTLifecycle(session)) to not call session.connectCallbacks, leaving callback invocation solely to executeGATTLifecycle or implement a single explicit place that manages connect callbacks to avoid duplication.
186-195:⚠️ Potential issue | 🟠 Major
stopDiscovery()leaves internal listeners active.This stops BlueZ discovery but doesn't clear the watcher interval or per-device
PropertiesChangedlisteners. Cached devices can still produce advertisements after discovery is supposed to be stopped. The cleanup only happens inshutdown().🔧 Suggested fix
async stopDiscovery(): Promise<void> { if (!this.scanning) return try { await this.adapter.helper.callMethod('StopDiscovery') this.scanning = false + // Clean up device watcher + const watcherCleanup = this.deviceListeners.get('__watcher__') + if (watcherCleanup) { + watcherCleanup() + this.deviceListeners.delete('__watcher__') + } debug('Discovery stopped') } catch (e: any) { debug(`Error stopping discovery: ${e.message}`) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/localProvider.ts` around lines 186 - 195, stopDiscovery currently stops BlueZ discovery but leaves internal watcher interval and per-device PropertiesChanged handlers active; update stopDiscovery to clear the watcher interval (e.g., clearInterval on whatever field stores the interval such as this.watcherInterval or this.watcher), iterate the tracked devices (e.g., this.devices or this.deviceListeners map) and remove/unsubscribe each stored PropertiesChanged listener (call off/removeListener with the stored callback references), clear the per-device listener map and any cached device entries, then set this.scanning = false and emit/log "Discovery stopped"; mirror the same cleanup logic used in shutdown() so no device advertisements remain after stopDiscovery returns.src/api/ble/index.ts (1)
358-360:⚠️ Potential issue | 🟡 Minor
getDevice()may return stalegattClaimedBystate.Unlike
getDevices(), this method doesn't refreshgattClaimedByfrom current claims. The claim state updates on advertisements, but a brief window of staleness exists.🔧 Suggested fix
async getDevice(mac: string): Promise<BLEDeviceInfo | null> { - return this.deviceTable.get(mac.toUpperCase()) ?? null + const key = mac.toUpperCase() + const device = this.deviceTable.get(key) + if (!device) return null + device.gattClaimedBy = this.gattClaims.get(key)?.pluginId + return device }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/index.ts` around lines 358 - 360, getDevice currently returns the device entry directly from deviceTable and can return stale gattClaimedBy; update getDevice to refresh the claim state the same way getDevices does: retrieve the stored device from deviceTable (mac.toUpperCase()), then rehydrate/overwrite its gattClaimedBy from the current claim source used by getDevices (or call the same helper used there) before returning so the returned BLEDeviceInfo reflects up-to-date claim state.src/api/ble/remoteProvider.ts (2)
327-341:⚠️ Potential issue | 🟠 MajorNon-default descriptor options are still dropped when forwarding to the gateway.
The remote command construction omits
poll.writeBeforeRead,periodicWrite[].withResponse, and doesn't convertinit[]fields to snake_case. ESP32 gateways won't receive these options thatGATTSubscriptionDescriptorsupports.🔧 Suggested fix
if (descriptor.poll) { cmd.poll = descriptor.poll.map((p) => ({ uuid: p.uuid, - interval_ms: p.intervalMs + interval_ms: p.intervalMs, + ...(p.writeBeforeRead !== undefined && { + write_before_read: p.writeBeforeRead + }) })) } - if (descriptor.init) cmd.init = descriptor.init + if (descriptor.init) { + cmd.init = descriptor.init.map((item) => ({ + uuid: item.uuid, + data: item.data, + ...(item.withResponse !== undefined && { + with_response: item.withResponse + }) + })) + } if (descriptor.periodicWrite) { cmd.periodic_write = descriptor.periodicWrite.map((pw) => ({ uuid: pw.uuid, data: pw.data, - interval_ms: pw.intervalMs + interval_ms: pw.intervalMs, + ...(pw.withResponse !== undefined && { + with_response: pw.withResponse + }) })) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/remoteProvider.ts` around lines 327 - 341, The remote command builder (in the block that maps descriptor → cmd) is dropping non-default options: ensure you copy poll.writeBeforeRead into cmd.poll (map p.writeBeforeRead to interval_ms/uuid plus write_before_read), include periodicWrite[].withResponse when mapping descriptor.periodicWrite to cmd.periodic_write (map pw.withResponse → with_response), and convert all init[] descriptor fields to snake_case when assigning cmd.init (e.g., map init.name → name, init.value → value, etc., matching the gateway's expected snake_case keys); update the mappings around the descriptor, cmd, poll, periodicWrite, and init handling so the ESP32 gateway receives poll.writeBeforeRead, periodicWrite.withResponse, and snake_cased init fields.
525-529: 🧹 Nitpick | 🔵 TrivialValidation skips invalid device records but doesn't reject malformed batches.
The code validates individual device records (lines 557-559) by skipping those without valid
macorrssi. This is reasonable for batch ingestion resilience. However, completely malformed payloads (e.g., non-object devices array elements) could still cause issues.♻️ Optional: Add type guard for device records
for (const dev of body.devices) { + if (typeof dev !== 'object' || dev === null) continue if (!dev.mac || typeof dev.mac !== 'string') continue if (typeof dev.rssi !== 'number') continueAlso applies to: 557-559
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ble/remoteProvider.ts` around lines 525 - 529, The batch handler reads req.body as BLEGatewayAdvertisementBatch and currently only checks gateway_id and that devices is an array, then later skips invalid device entries by testing device.mac and device.rssi; add a stricter validation/type-guard for each device element before processing so non-object or malformed entries are rejected or filtered out early: implement a helper (e.g., isValidBLEDevice(device): boolean) and use it to filter devices (or return 400 if all entries are invalid) before downstream processing in the handler that reads body.devices; ensure references to BLEGatewayAdvertisementBatch, body.devices, and the existing mac/rssi checks are updated to use this guard to avoid runtime errors on non-object elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/ble/remoteProvider.ts`:
- Around line 916-924: When cleaning up timed-out HTTP-only gateways in the loop
that inspects this.seenMacs (the block checking Date.now() - lastPost >
OFFLINE_SNAPSHOT_MS), also remove any remaining advertising callbacks and the
registered provider for that gateway to avoid leaking state: delete the entry in
this.advCallbacks for gatewayId and unregister/clear the provider that was
created by _registerGatewayProvider for that same gatewayId (the same place you
track providers), in addition to the existing deletions of this.seenMacs,
this.lastPostTime, and this.httpGatewayMeta.
---
Duplicate comments:
In `@docs/develop/rest-api/ble_api.md`:
- Around line 146-147: The docs incorrectly call the UI metric a "live
advertisement rate"; update the wording in ble_api.md so it describes the actual
behavior: the status row shows a cumulative advertisement count (total ads since
page load), matching the implementation in BLEManager.tsx where advCountRef is
incremented on each WebSocket message and rendered as a running total rather
than a rate. Mention advCountRef and BLEManager.tsx in the docs change so
readers can cross-reference the component behavior.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 496-499: The render is mutating React state by calling
devices.sort(...) directly; make a non-mutating copy before sorting (e.g., use
[...devices] or devices.slice()) and then sort that copy prior to mapping so you
don't modify the original devices array in the BLEManager render pipeline;
update the expression around devices.sort(...) used before .map(...) to operate
on the copied array to preserve immutability.
- Around line 172-185: The WebSocket handlers in connect() close over the
mutable ws variable causing a new socket to be closed by old handlers; change
connect() to capture the newly created socket in a local constant (e.g., const
socket = new WebSocket(wsUrl)) and bind handlers to that local variable
(socket.onopen, socket.onclose, socket.onerror, socket.onmessage), update
wsRef.current = socket, and when scheduling reconnects or closing ensure you
reference socket (or check wsRef.current === socket) so old handlers cannot
affect a newer socket; also clear or null out reconnectTimer and remove handlers
on dispose to avoid stale closures (references: connect, ws, wsRef, ws.onopen,
ws.onclose, ws.onerror, ws.onmessage, reconnectTimer, disposed, advCountRef).
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 152-157: Remove the stale "Restart Required" Badge from the
BLESettings UI: locate the JSX in BLESettings.tsx that renders the conditional
block using the supported variable (the fragment that contains <Badge
bg="danger" className="float-end">Restart Required</Badge>) and delete that
Badge (and surrounding stray whitespace/fragment if now unnecessary) so the UI
no longer displays a restart warning when provider changes are applied
immediately by shutdownLocalProviders() and initLocalProviders().
In `@packages/server-api/src/bleapi.ts`:
- Around line 89-102: The interface GATTSubscriptionHandle currently exposes
onDisconnect/onConnect but has no typed way for providers to signal a forced
disconnect (implementations currently call (claim.handle as
any)._fireDisconnect); add a public method to the interface (e.g.,
signalDisconnect(): void or notifyProviderOffline(): void) so providers can
invoke it instead of using a cast to any, and update implementations that
currently call _fireDisconnect to implement this new method; alternatively, if
you want to keep a single semantic entrypoint, document that close() will
synchronously invoke registered onDisconnect callbacks and ensure
implementations call those callbacks from close() rather than relying on an
untyped private method.
In `@src/api/ble/index.ts`:
- Around line 358-360: getDevice currently returns the device entry directly
from deviceTable and can return stale gattClaimedBy; update getDevice to refresh
the claim state the same way getDevices does: retrieve the stored device from
deviceTable (mac.toUpperCase()), then rehydrate/overwrite its gattClaimedBy from
the current claim source used by getDevices (or call the same helper used there)
before returning so the returned BLEDeviceInfo reflects up-to-date claim state.
In `@src/api/ble/localProvider.ts`:
- Around line 166-184: In startDiscovery(), do not swallow errors: when
adapter.helper.callMethod('StartDiscovery') (and related calls) throws, log the
error using debug but then re-throw or propagate it so callers (e.g., the code
that invokes startDiscovery in src/api/ble/index.ts) can handle the failure;
update the catch block in async startDiscovery() to either throw e after
debug(`Error starting discovery: ${e.message}`) or return a rejected Promise,
ensuring this.adapter.scanning is not left true and startDeviceWatcher() is only
called on success.
- Around line 655-671: The slot reserved earlier can leak if device.gatt() or
the listener registration throws: ensure releaseSlot() is always called on error
by wrapping the await device.gatt() and the device.on('disconnect', ...) setup
in a try/catch/finally (or try { ... } catch (err) { releaseSlot(); throw err }
) so releaseSlot() runs when gatt() or listener setup fails; also ensure any
partially-registered state (e.g., connected flag or added listener in device.on)
is cleaned up in the same error path so disconnectCallbacks and connected are
consistent after failure (references: device.gatt(), device.on('disconnect',
...), releaseSlot(), connected, disconnectCallbacks).
- Around line 558-584: The reconnect flow is invoking session.connectCallbacks
twice: executeGATTLifecycle(session) already triggers the connect callbacks, and
attemptReconnect's success block also iterates session.connectCallbacks; remove
the duplicate invocation in attemptReconnect (or guard it with a check like
session.connected) so callbacks are triggered only once by executeGATTLifecycle.
Update the attemptReconnect function (the success branch after await
this.executeGATTLifecycle(session)) to not call session.connectCallbacks,
leaving callback invocation solely to executeGATTLifecycle or implement a single
explicit place that manages connect callbacks to avoid duplication.
- Around line 186-195: stopDiscovery currently stops BlueZ discovery but leaves
internal watcher interval and per-device PropertiesChanged handlers active;
update stopDiscovery to clear the watcher interval (e.g., clearInterval on
whatever field stores the interval such as this.watcherInterval or
this.watcher), iterate the tracked devices (e.g., this.devices or
this.deviceListeners map) and remove/unsubscribe each stored PropertiesChanged
listener (call off/removeListener with the stored callback references), clear
the per-device listener map and any cached device entries, then set
this.scanning = false and emit/log "Discovery stopped"; mirror the same cleanup
logic used in shutdown() so no device advertisements remain after stopDiscovery
returns.
In `@src/api/ble/openApi.json`:
- Around line 423-429: The OpenAPI Operation objects for the WebSocket endpoints
(GET on path "/gateway/ws" and GET on path "/advertisements") are missing the
required responses field; add a minimal responses object to each operation
(e.g., include a 101 Switching Protocols response for successful WebSocket
upgrades and at least a default or 400/500 error response) so the spec is valid,
updating the operations under the "/gateway/ws" and "/advertisements" paths
accordingly.
- Around line 17-20: The OpenAPI servers.url is set to
"/signalk/v2/api/vessels/self/ble" but gateway routes (e.g.,
"/gateway/advertisements", "/gateway/ws", "/gateways") are actually mounted
under "/signalk/v2/api/ble", so generated clients will build wrong base URLs;
fix by either (A) moving all gateway path entries into a separate OpenAPI
document whose servers.url is "/signalk/v2/api/ble" (keeping vessel BLE routes
in the current document), or (B) keep a single document but change the gateway
path entries to use absolute paths (full "/signalk/v2/api/ble/gateway/..."
values) or change the servers.url to a common base like "/signalk/v2/api" and
adjust paths accordingly; update references to servers.url and gateway path
definitions (look for "servers": [...], and path keys "/gateway/advertisements",
"/gateway/ws", "/gateways") to implement one of these fixes.
In `@src/api/ble/openApi.ts`:
- Around line 4-8: The BLE OpenAPI record bleApiRecord is defined but not
included in the Swagger aggregation, so the BLE endpoints won't appear; to fix,
import bleApiRecord where the API docs are aggregated (the apiDocs array) and
add bleApiRecord to that apiDocs array (and export if needed) so the OpenAPI
aggregator includes the BLE spec.
In `@src/api/ble/remoteProvider.ts`:
- Around line 327-341: The remote command builder (in the block that maps
descriptor → cmd) is dropping non-default options: ensure you copy
poll.writeBeforeRead into cmd.poll (map p.writeBeforeRead to interval_ms/uuid
plus write_before_read), include periodicWrite[].withResponse when mapping
descriptor.periodicWrite to cmd.periodic_write (map pw.withResponse →
with_response), and convert all init[] descriptor fields to snake_case when
assigning cmd.init (e.g., map init.name → name, init.value → value, etc.,
matching the gateway's expected snake_case keys); update the mappings around the
descriptor, cmd, poll, periodicWrite, and init handling so the ESP32 gateway
receives poll.writeBeforeRead, periodicWrite.withResponse, and snake_cased init
fields.
- Around line 525-529: The batch handler reads req.body as
BLEGatewayAdvertisementBatch and currently only checks gateway_id and that
devices is an array, then later skips invalid device entries by testing
device.mac and device.rssi; add a stricter validation/type-guard for each device
element before processing so non-object or malformed entries are rejected or
filtered out early: implement a helper (e.g., isValidBLEDevice(device): boolean)
and use it to filter devices (or return 400 if all entries are invalid) before
downstream processing in the handler that reads body.devices; ensure references
to BLEGatewayAdvertisementBatch, body.devices, and the existing mac/rssi checks
are updated to use this guard to avoid runtime errors on non-object elements.
In `@src/config/config.ts`:
- Around line 92-96: The bleApi config currently defines localAdapter?: string
which conflicts with the rest of the codebase expecting localAdapters (an array)
— update the config shape to the canonical form (e.g., change
bleApi.localAdapter to localAdapters?: string[]), and update any
loader/serializer logic to accept the legacy single-string key by mapping it
into the new array (handle both in the code paths that read/write the config).
Make sure to update references to bleApi.localAdapter and any consumers to use
bleApi.localAdapters (or add a conversion helper) so type checks and persisted
settings remain compatible.
In `@src/serverroutes.ts`:
- Around line 977-983: The code writes settings.options.useBLEManager into
app.config.settings.bleApi (app.config.settings.bleApi.localBluetoothManaged)
which never gets persisted because later the code persists updatedSettings and
replaces runtime settings with updatedSettings; move the assignment so the BLE
toggle is saved to the persisted settings object instead: ensure
updatedSettings.options exists and set updatedSettings.options.useBLEManager =
settings.options.useBLEManager (or copy into
updatedSettings.settings/appropriate persisted subtree) before the persistence
step that uses updatedSettings, leaving app.config only for runtime copies like
localBluetoothManaged if still needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0d76984-3f52-4f53-b682-db6124346e90
📒 Files selected for processing (28)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.jsonsrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
Contributor
Author
|
ready for human review |
Contributor
Author
|
Worth reading, has screenshots too: |
dirkwa
force-pushed
the
ble-provider-api
branch
2 times, most recently
from
5ca1de8 to
f1d1025
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/serverroutes.ts (1)
1132-1138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist
useBLEManageronupdatedSettings, not live config.Line 1133 mutates
app.config.settings, but Line 1172 persistsupdatedSettingsand Line 1176 replaces live settings with it, so this BLE change is dropped.Suggested fix
if (!isUndefined(settings.options.useBLEManager)) { - if (!app.config.settings.bleApi) { - app.config.settings.bleApi = {} + if (!updatedSettings.bleApi) { + updatedSettings.bleApi = {} } - app.config.settings.bleApi.localBluetoothManaged = + updatedSettings.bleApi.localBluetoothManaged = settings.options.useBLEManager }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serverroutes.ts` around lines 1132 - 1138, The code currently mutates app.config.settings.bleApi.localBluetoothManaged using settings.options.useBLEManager which is overwritten later when updatedSettings is persisted and applied; instead set the BLE flag on updatedSettings (create updatedSettings.settings.bleApi if needed) using settings.options.useBLEManager so the change is saved when updatedSettings is persisted and then replaces the live config; reference the symbols settings.options.useBLEManager, app.config.settings.bleApi.localBluetoothManaged, and updatedSettings to locate where to move the assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 121-123: Extract the magic numbers for GATT slot bounds and
default into named constants (e.g., MIN_GATT_SLOTS, MAX_GATT_SLOTS,
DEFAULT_GATT_SLOTS) at module scope in BLESettings.tsx and replace the inline
literals used in the JSX props and any related logic (references to
settings.localMaxGATTSlots and the other occurrences around lines 127-131) with
those constants so the UI contract is centralized and cannot drift.
- Around line 37-57: The handleSave useCallback lacks a catch for network-level
failures from fetch, so add a catch block in the async flow around the fetch
call (inside handleSave) to handle thrown errors, show a user-facing failure
message (e.g., alert with error.message or a generic "Network error while saving
BLE settings"), and ensure setSaving(false) still runs (keep the existing
finally). Update the handleSave function so that any thrown exception from fetch
is caught and handled (referencing handleSave and the existing fetch to
`${BLE_API}/settings`) rather than allowing the promise to reject unhandled.
In `@packages/server-api/src/typebox/ble-schemas.ts`:
- Around line 13-14: The mac fields currently allow any string; update the
Type.String schema for both the device "mac" and the gateway "mac" to include a
regex pattern that enforces MAC format (e.g. colon-separated hex pairs). Replace
Type.String({ description: '...' }) with Type.String({ description: '...',
pattern: '^([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}$' }) for the top-level mac and the
gateway.mac schema entries so malformed MACs are rejected by the API boundary.
In `@src/api/ble/index.ts`:
- Around line 301-306: After pruning stale providers (using DEVICE_STALE_MS)
guard against an empty device.seenBy before calling Math.max: check if
device.seenBy.length === 0 and in that case set device.rssi and device.lastSeen
to null (or another sentinel you prefer) instead of calling Math.max(...[]) ;
otherwise compute device.rssi = Math.max(...device.seenBy.map(s => s.rssi)) and
device.lastSeen = Math.max(...device.seenBy.map(s => s.lastSeen)). Ensure you
update the code paths that consume device.rssi/device.lastSeen to handle the
null sentinel if needed.
In `@src/api/ble/localProvider.ts`:
- Around line 291-296: The code treats changedProps.RSSI with a truthy check so
a valid RSSI of 0 is considered missing; change the condition in the RSSI
handling to an explicit undefined check (e.g., check changedProps.RSSI !==
undefined or Object.prototype.hasOwnProperty.call(changedProps, 'RSSI')) so
unwrapVariant(changedProps.RSSI) and this.lastRssi.set(mac, rssi) run for 0
values, otherwise fall back to this.lastRssi.get(mac) ?? -127.
In `@src/api/ble/remoteProvider.ts`:
- Around line 269-299: In the 'gatt_disconnected' handler, you currently mark
session.connected = false and call disconnectCallbacks but you do not release
the session slot; update the case 'gatt_disconnected' handling to mirror
'gatt_error' by deleting the session from this.sessions and decrementing
this.activeSlots (e.g., this.activeSlots = Math.max(0, this.activeSlots - 1))
after invoking disconnectCallbacks so slots are correctly freed for future
subscriptions.
---
Duplicate comments:
In `@src/serverroutes.ts`:
- Around line 1132-1138: The code currently mutates
app.config.settings.bleApi.localBluetoothManaged using
settings.options.useBLEManager which is overwritten later when updatedSettings
is persisted and applied; instead set the BLE flag on updatedSettings (create
updatedSettings.settings.bleApi if needed) using settings.options.useBLEManager
so the change is saved when updatedSettings is persisted and then replaces the
live config; reference the symbols settings.options.useBLEManager,
app.config.settings.bleApi.localBluetoothManaged, and updatedSettings to locate
where to move the assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f976485-b0ca-4c09-bf2e-effa94217e07
📒 Files selected for processing (28)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.jsonsrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
dirkwa
force-pushed
the
ble-provider-api
branch
3 times, most recently
from
927bb4b to
efc385c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (15)
packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (2)
153-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale restart warning.
This badge tells admins to restart even though BLE provider changes are applied immediately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 153 - 159, In BLESettings.tsx remove the stale "Restart Required" Badge that is shown when the supported flag is true; locate the JSX block that renders Badge (the fragment containing Badge with bg="danger" and className="float-end") and delete that Badge (and surrounding empty fragment/spaces) so BLE provider changes no longer show a restart warning in the BLESettings component.
37-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle thrown fetch failures in save flow.
fetch()network failures currently bypass user feedback. Add acatchto avoid silent/unhandled save failures.Proposed fix
const handleSave = useCallback(async () => { if (!settings) return setSaving(true) try { const res = await fetch(`${BLE_API}/settings`, { @@ if (!res.ok) { const err = await res.json().catch(() => ({ message: res.statusText })) alert(err.message || 'Failed to save BLE settings') } + } catch (e) { + const message = + e instanceof Error ? e.message : 'Failed to save BLE settings' + alert(message) } finally { setSaving(false) } }, [settings])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 37 - 56, The save flow in handleSave currently doesn't catch network-level fetch failures so users get no feedback; wrap the await fetch call in a try/catch (or add a .catch) to capture thrown errors, call alert with a helpful message (e.g., err.message || 'Failed to save BLE settings'), and ensure setSaving(false) still runs (the finally block can remain); locate and update the handleSave function that references settings, setSaving, and BLE_API to implement this error handling.packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx (2)
172-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBind handlers to the socket instance, not the mutable
wsbinding.Old socket callbacks can act on a newer socket because handlers capture
wsby reference.Proposed fix
const connect = () => { if (disposed) return - ws = new WebSocket(wsUrl) - wsRef.current = ws - ws.onopen = () => setWsConnected(true) - ws.onclose = () => { + const socket = new WebSocket(wsUrl) + ws = socket + wsRef.current = socket + socket.onopen = () => setWsConnected(true) + socket.onclose = () => { + if (socket !== wsRef.current) return setWsConnected(false) if (!disposed) reconnectTimer = setTimeout(connect, 5000) } - ws.onerror = () => ws?.close() - ws.onmessage = () => { + socket.onerror = () => { + if (socket === wsRef.current) socket.close() + } + socket.onmessage = () => { advCountRef.current += 1 } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` around lines 172 - 182, The WebSocket handlers are being attached to the mutable outer binding `ws`, which lets stale handlers act on a newer socket; in the `connect` function create a local socket variable (e.g., const socket = new WebSocket(wsUrl)), assign that instance to `ws` and `wsRef.current`, and attach handlers to `socket.onopen`, `socket.onclose`, `socket.onerror`, and `socket.onmessage` (use `socket.close()` inside error handler) so each set of callbacks is bound to the specific WebSocket instance rather than the changing `ws` variable.
496-498:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid mutating
devicesstate during render.
devices.sort(...)mutates state in place. Sort a copy instead.Proposed fix
- {devices + {[...devices] .sort((a, b) => (b.rssi ?? -999) - (a.rssi ?? -999)) .map((device) => (As per coding guidelines, "Prefer immutable data structures where practical".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` around lines 496 - 498, The render is mutating state by calling devices.sort(...) inside BLEManager rendering; instead create a non-mutating copy (e.g. [...devices] or devices.slice()) and call .sort(...) on that copy before mapping. Update the expression around devices.sort(...) to sort the copied array (the code that currently does {devices.sort((a, b) => (b.rssi ?? -999) - (a.rssi ?? -999)).map(device => (...))}) so the original devices state is not modified during render.src/api/ble/openApi.json (1)
423-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd required
responsesto WebSocket GET operations.Both operation objects are missing required
responses, which makes the OpenAPI document invalid for many validators/toolchains.#!/bin/bash python - <<'PY' import json doc=json.load(open('src/api/ble/openApi.json')) for p in ['/gateway/ws','/advertisements']: op=doc['paths'][p]['get'] print(p, 'responses' in op) PYProposed fix
"/gateway/ws": { "servers": [{ "url": "/signalk/v2/api/ble" }], "get": { "tags": ["Gateway"], "summary": "WebSocket GATT command channel for ESP32 gateways", - "description": "Upgrade to WebSocket for GATT subscribe/data/disconnect messaging." + "description": "Upgrade to WebSocket for GATT subscribe/data/disconnect messaging.", + "responses": { + "101": { "description": "Switching Protocols" } + } } }, @@ "/advertisements": { "get": { "tags": ["BLE"], "summary": "WebSocket stream of BLE advertisements", - "description": "Upgrade to WebSocket to receive a real-time stream of BLEAdvertisement JSON objects from all registered providers." + "description": "Upgrade to WebSocket to receive a real-time stream of BLEAdvertisement JSON objects from all registered providers.", + "responses": { + "101": { "description": "Switching Protocols" } + } } }Also applies to: 546-551
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/openApi.json` around lines 423 - 429, The GET operations for the WebSocket endpoints (paths "/gateway/ws" and "/advertisements") are missing the required "responses" object which makes the OpenAPI invalid; add a "responses" property to each GET operation (e.g., in the operation objects under paths["/gateway/ws"].get and paths["/advertisements"].get) containing at least a default or 101/200 response description (and content if appropriate) so validators accept the document; ensure the responses are descriptive (e.g., "101 Switching Protocols" for WebSocket upgrade or a default error response) and follow the same structure as other operations in the spec.packages/server-api/src/typebox/ble-schemas.ts (1)
13-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce MAC format in schema, not only descriptions.
Both MAC fields still accept arbitrary strings. Add a MAC regex
patternto validate inputs at the API boundary.Proposed fix
import { Type, type Static } from '`@sinclair/typebox`' +const MAC_PATTERN = '^([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}$' @@ - mac: Type.String({ description: 'Device MAC address (AA:BB:CC:DD:EE:FF)' }), + mac: Type.String({ + pattern: MAC_PATTERN, + description: 'Device MAC address (AA:BB:CC:DD:EE:FF)' + }), @@ mac: Type.Optional( Type.String({ + pattern: MAC_PATTERN, description: "Gateway's own MAC address (AA:BB:CC:DD:EE:FF)" }) ),Also applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-api/src/typebox/ble-schemas.ts` around lines 13 - 14, The mac fields currently use Type.String with only a description; add a strict MAC address regex via the Type.String option "pattern" to validate format at the API boundary (update both occurrences of the mac property in this file where Type.String is used, e.g., the mac field in the BLE device schema and the other mac field around lines 67–70). Use a regex that matches six hex byte pairs separated by colons (case-insensitive) and include a concise description indicating the enforced format; leave other properties unchanged.src/api/ble/localProvider.ts (5)
166-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDiscovery startup errors are swallowed.
When
StartDiscoveryfails, the error is logged but not re-thrown. Callers (e.g.,src/api/ble/index.tsline 178) treat the provider as successfully started. This can leave the adapter in a half-initialized state where it's registered but not actually scanning.🐛 Proposed fix
} catch (e: any) { debug(`Error starting discovery: ${e.message}`) + throw e } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 166 - 184, In startDiscovery (async startDiscovery) you currently catch errors from the call to StartDiscovery and only debug-log them, which swallows failures and leaves the provider registered but not scanning; change the handler to not hide the failure: remove the silent swallow and rethrow the caught error (or return a rejected Promise) after logging, and ensure that side effects (setting this.scanning = true and calling this.startDeviceWatcher()) only happen after StartDiscovery succeeds; reference the StartDiscovery call, the this.scanning assignment, and startDeviceWatcher to locate where to move the state changes and where to rethrow the error.
291-296:⚠️ Potential issue | 🟡 Minor | 💤 Low valueUse an explicit undefined check for
RSSI.The truthy check
if (changedProps.RSSI)treats an RSSI value of0as absent, falling back to cached or sentinel value. While RSSI of 0 dBm is rare, it's technically valid.Proposed fix
- if (changedProps.RSSI) { + if (changedProps.RSSI !== undefined) { rssi = this.unwrapVariant(changedProps.RSSI) this.lastRssi.set(mac, rssi) } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 291 - 296, The RSSI presence check uses a truthy test which treats 0 as missing; update the condition in the block that reads changedProps.RSSI (in the method using unwrapVariant and this.lastRssi) to explicitly check for undefined (e.g., changedProps.RSSI !== undefined) so a legitimate 0 value is accepted, then call rssi = this.unwrapVariant(changedProps.RSSI) and this.lastRssi.set(mac, rssi) as before; keep the else branch that falls back to this.lastRssi.get(mac) ?? -127 unchanged.
563-571:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
connectCallbacksare fired twice after reconnect.After a successful reconnect,
attemptReconnectfiresconnectCallbacksat lines 565-571. However,executeGATTLifecycle()(called at line 564) already fires them at lines 585-591. This results in duplicateonConnectnotifications to consumer plugins.🐛 Proposed fix — remove duplicate firing in attemptReconnect
try { await this.executeGATTLifecycle(session) - for (const cb of session.connectCallbacks) { - try { - cb() - } catch (_e) { - // ignore - } - } } catch (e: any) { debug(`GATT reconnect failed for ${mac}: ${e.message}`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 563 - 571, Duplicate firing occurs because attemptReconnect calls executeGATTLifecycle(session) which itself invokes session.connectCallbacks, and attemptReconnect then re-iterates session.connectCallbacks; remove the redundant invocation in attemptReconnect by deleting the post-executeGATTLifecycle loop (the for (const cb of session.connectCallbacks) { try { cb() } catch {} }) or replace it with a guard that only calls callbacks if executeGATTLifecycle did not already signal success (e.g., check a session.connected flag set by executeGATTLifecycle) so connectCallbacks are invoked exactly once.
186-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
stopDiscovery()does not stop the device watcher.This stops BlueZ discovery but leaves the watcher interval from
startDeviceWatcher()running. Cached devices can still produce advertisements viaPropertiesChangedlisteners after discovery is "stopped". The watcher and device listeners are only cleaned up inshutdown().🛠️ Proposed fix
async stopDiscovery(): Promise<void> { if (!this.scanning) return try { + // Stop the device watcher + const watcherCleanup = this.deviceListeners.get('__watcher__') + if (watcherCleanup) { + watcherCleanup() + this.deviceListeners.delete('__watcher__') + } await this.adapter.helper.callMethod('StopDiscovery') this.scanning = false debug('Discovery stopped') } catch (e: any) { debug(`Error stopping discovery: ${e.message}`) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 186 - 195, stopDiscovery() currently only stops BlueZ discovery but leaves the device watcher and PropertiesChanged listeners running; update stopDiscovery() to also stop the watcher and remove device listeners the same way shutdown() does: clear the interval/timeout created by startDeviceWatcher() (e.g. clearInterval(this.deviceWatcher) and set this.deviceWatcher = undefined), remove/unsubscribe any PropertiesChanged listeners or signal handlers attached to devices, and ensure this.scanning is set to false; reference startDeviceWatcher(), shutdown(), and this.deviceWatcher/PropertiesChanged listeners to locate where to perform the cleanup.
652-668:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw GATT slot not released if setup fails after connect.
If
device.gatt()at line 652 or the disconnect listener setup throws,releaseSlot()is never called. The slot stays reserved and the device connection may remain open, permanently reducing available GATT capacity.🐛 Proposed fix
- const gattServer = await device.gatt() - let connected = true - const disconnectCallbacks: Array<() => void> = [] - - device.on('disconnect', () => { + let connected = true + const disconnectCallbacks: Array<() => void> = [] + let gattServer: any + try { + gattServer = await device.gatt() + device.on('disconnect', () => { + if (connected) { + connected = false + releaseSlot() + } + for (const cb of disconnectCallbacks) { + try { + cb() + } catch (_e) { + // ignore + } + } + }) + } catch (e) { + try { + await device.disconnect() + } catch (_e) { + // ignore + } + releaseSlot() + throw e + } - if (connected) { - connected = false - releaseSlot() - } - for (const cb of disconnectCallbacks) { - try { - cb() - } catch (_e) { - // ignore - } - } - })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 652 - 668, The code can throw during device.gatt() or while attaching the 'disconnect' listener which prevents releaseSlot() from running; wrap the gatt setup and listener registration in a try/catch/finally (or ensure cleanup on any thrown error) so that releaseSlot() is always called if the connection setup fails, and also ensure the partially-initialized connection is closed (e.g., call device.disconnect() or close gattServer if created) when an error occurs; update the block around device.gatt(), the connected flag logic, and the listener registration to perform these cleanup steps and avoid leaking reserved GATT slots.src/api/ble/remoteProvider.ts (3)
269-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
gatt_disconnectedhandler does not release the session slot.Unlike the
gatt_errorhandler (lines 285-299) which deletes the session and decrementsactiveSlots,gatt_disconnectedonly marksconnected = falseand fires callbacks. The session remains in the map and the slot stays consumed, which can exhaust GATT capacity until a later status update or gateway reconnect.🐛 Proposed fix
case 'gatt_disconnected': { const session = this.sessions.get(sessionId) if (!session) return session.connected = false for (const cb of session.disconnectCallbacks) { try { cb() } catch { /* ignore */ } } + this.sessions.delete(sessionId) + this.activeSlots = Math.max(0, this.activeSlots - 1) debug( `[${this.gatewayId}] session ${sessionId} disconnected: ${msg.reason}` ) break }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/remoteProvider.ts` around lines 269 - 283, The gatt_disconnected handler currently only sets session.connected = false and invokes session.disconnectCallbacks, leaving the session in this.sessions and keeping the slot counted; mirror the logic from the gatt_error handler by removing the session from this.sessions and decrementing this.activeSlots when handling 'gatt_disconnected' (use the same sessionId/session retrieval pattern), ensuring you still call disconnectCallbacks and log the reason, and wrap callback invocation in the same try/catch used now so no behavior changes aside from releasing the slot.
923-932:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHTTP-only gateway cleanup is incomplete.
When an HTTP-only gateway times out (lines 927-931), only
seenMacs,lastPostTime, andhttpGatewayMetaare deleted. TheadvCallbacksentry and the registered provider created by_registerGatewayProvider()remain, leaking state for abandoned gateways.🛠️ Proposed fix
if (Date.now() - lastPost > OFFLINE_SNAPSHOT_MS) { this.seenMacs.delete(gatewayId) this.lastPostTime.delete(gatewayId) this.httpGatewayMeta.delete(gatewayId) + this.advCallbacks.delete(gatewayId) + this.unregisterProvider(`ble:gateway:${gatewayId}`) continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/remoteProvider.ts` around lines 923 - 932, The cleanup loop for timed-out HTTP-only gateways currently deletes seenMacs, lastPostTime, and httpGatewayMeta but misses removing advCallbacks entries and the provider registered via _registerGatewayProvider(), causing memory/state leaks; update the block (the same place that checks this.sessions and this.snapshots and compares OFFLINE_SNAPSHOT_MS) to also delete this.advCallbacks.delete(gatewayId) and call the corresponding unregister/remove for the provider created by _registerGatewayProvider() (or remove it from whatever provider registry/map you store it in) so the gateway's callbacks and provider are fully cleaned up.
327-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDescriptor options are dropped when forwarding to the gateway.
The remote command construction omits
poll[].writeBeforeRead,periodicWrite[].withResponse, and passesinit[]through without translatingwithResponseto the snake_case wire format. ESP32 gateways receiving this command cannot execute theseGATTSubscriptionDescriptoroptions.🐛 Proposed fix
if (descriptor.poll) { cmd.poll = descriptor.poll.map((p) => ({ uuid: p.uuid, - interval_ms: p.intervalMs + interval_ms: p.intervalMs, + ...(p.writeBeforeRead !== undefined && { + write_before_read: p.writeBeforeRead + }) })) } - if (descriptor.init) cmd.init = descriptor.init + if (descriptor.init) { + cmd.init = descriptor.init.map((item) => ({ + uuid: item.uuid, + data: item.data, + ...(item.withResponse !== undefined && { + with_response: item.withResponse + }) + })) + } if (descriptor.periodicWrite) { cmd.periodic_write = descriptor.periodicWrite.map((pw) => ({ uuid: pw.uuid, data: pw.data, - interval_ms: pw.intervalMs + interval_ms: pw.intervalMs, + ...(pw.withResponse !== undefined && { + with_response: pw.withResponse + }) })) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/remoteProvider.ts` around lines 327 - 341, The code building the remote `cmd` drops/fromats descriptor options needed by the gateway; update the mappings in the `if (descriptor.poll)`, `if (descriptor.periodicWrite)`, and `if (descriptor.init)` branches so that poll[].writeBeforeRead is forwarded as poll[].write_before_read, periodicWrite[].withResponse as periodic_write[].with_response, and any init[] items with withResponse are translated to with_response (keep existing translations like intervalMs -> interval_ms and uuid/data fields); specifically modify the `descriptor.poll.map`, `descriptor.periodicWrite.map`, and the assignment for `descriptor.init` to include these snake_case fields (e.g., add write_before_read and with_response) so ESP32 gateways receive the GATTSubscriptionDescriptor options.src/api/ble/index.ts (1)
367-369:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
getDevice()does not refreshgattClaimedBystate.Unlike
getDevices()which updatesgattClaimedByfromthis.gattClaims, this method returns the cached device object as-is. After a claim is acquired or released,/devices/:maccan report stale claim status until another advertisement arrives or/devicesis called.🛠️ Proposed fix
async getDevice(mac: string): Promise<BLEDeviceInfo | null> { - return this.deviceTable.get(mac.toUpperCase()) ?? null + const key = mac.toUpperCase() + const device = this.deviceTable.get(key) + if (!device) return null + device.gattClaimedBy = this.gattClaims.get(key)?.pluginId + return device }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/index.ts` around lines 367 - 369, getDevice currently returns the cached BLEDeviceInfo from deviceTable without updating its gattClaimedBy, causing stale claim state; modify getDevice(mac: string) to look up the device via this.deviceTable.get(mac.toUpperCase()), then if found override/update its gattClaimedBy from this.gattClaims.get(mac.toUpperCase()) (or set to undefined/null if no claim) before returning, matching the logic used in getDevices so /devices/:mac always reflects current claim state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/plugins/ble_provider_plugins.md`:
- Around line 177-194: The BLE advertisement example currently embeds a full
BLEAdvertisement object (fields like mac, rssi, manufacturerData, serviceData,
serviceUuids, providerId, timestamp, connectable, txPower) which duplicates the
canonical API contract and risks doc drift; replace the inline object with a
concise conceptual example (e.g., "BLEAdvertisement: { mac, rssi, providerId,
optional name/manufacturerData/serviceData }") and add a direct link to the
authoritative TypeDoc/OpenAPI entry for BLEAdvertisement/providerId so readers
can find the complete, versioned schema, ensuring the doc only highlights
important semantics (mac format uppercase colon-separated, providerId must match
registered plugin ID) rather than repeating the full type definition.
In `@packages/server-api/src/bleapi.ts`:
- Around line 157-159: The public TypeScript type for the property gattClaimedBy
must include null to match the OpenAPI spec; update the declaration of
gattClaimedBy in the exported interface/type (the property currently declared as
gattClaimedBy?: string) to allow null (e.g., gattClaimedBy?: string | null) so
consumers see the nullable contract; ensure any related uses or type aliases
that reference gattClaimedBy are updated similarly.
In `@src/api/index.ts`:
- Around line 94-95: Replace the per-assignment "as any" casts by defining a
single ApiApplication interface that extends the existing per-API interfaces
(e.g., BLEApplication, CourseApplication, NotificationApplication, etc.), then
assert the express app to ApiApplication once and attach properties like bleApi
via (app as ApiApplication).bleApi = bleApi; alternatively, if you prefer the
dynamic pattern, add a single file-level eslint exception comment and keep using
the existing per-API interfaces for clarity; locate symbols bleApi,
BLEApplication, CourseApplication, NotificationApplication and update the
top-level app typing accordingly.
---
Duplicate comments:
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 172-182: The WebSocket handlers are being attached to the mutable
outer binding `ws`, which lets stale handlers act on a newer socket; in the
`connect` function create a local socket variable (e.g., const socket = new
WebSocket(wsUrl)), assign that instance to `ws` and `wsRef.current`, and attach
handlers to `socket.onopen`, `socket.onclose`, `socket.onerror`, and
`socket.onmessage` (use `socket.close()` inside error handler) so each set of
callbacks is bound to the specific WebSocket instance rather than the changing
`ws` variable.
- Around line 496-498: The render is mutating state by calling devices.sort(...)
inside BLEManager rendering; instead create a non-mutating copy (e.g.
[...devices] or devices.slice()) and call .sort(...) on that copy before
mapping. Update the expression around devices.sort(...) to sort the copied array
(the code that currently does {devices.sort((a, b) => (b.rssi ?? -999) - (a.rssi
?? -999)).map(device => (...))}) so the original devices state is not modified
during render.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 153-159: In BLESettings.tsx remove the stale "Restart Required"
Badge that is shown when the supported flag is true; locate the JSX block that
renders Badge (the fragment containing Badge with bg="danger" and
className="float-end") and delete that Badge (and surrounding empty
fragment/spaces) so BLE provider changes no longer show a restart warning in the
BLESettings component.
- Around line 37-56: The save flow in handleSave currently doesn't catch
network-level fetch failures so users get no feedback; wrap the await fetch call
in a try/catch (or add a .catch) to capture thrown errors, call alert with a
helpful message (e.g., err.message || 'Failed to save BLE settings'), and ensure
setSaving(false) still runs (the finally block can remain); locate and update
the handleSave function that references settings, setSaving, and BLE_API to
implement this error handling.
In `@packages/server-api/src/typebox/ble-schemas.ts`:
- Around line 13-14: The mac fields currently use Type.String with only a
description; add a strict MAC address regex via the Type.String option "pattern"
to validate format at the API boundary (update both occurrences of the mac
property in this file where Type.String is used, e.g., the mac field in the BLE
device schema and the other mac field around lines 67–70). Use a regex that
matches six hex byte pairs separated by colons (case-insensitive) and include a
concise description indicating the enforced format; leave other properties
unchanged.
In `@src/api/ble/index.ts`:
- Around line 367-369: getDevice currently returns the cached BLEDeviceInfo from
deviceTable without updating its gattClaimedBy, causing stale claim state;
modify getDevice(mac: string) to look up the device via
this.deviceTable.get(mac.toUpperCase()), then if found override/update its
gattClaimedBy from this.gattClaims.get(mac.toUpperCase()) (or set to
undefined/null if no claim) before returning, matching the logic used in
getDevices so /devices/:mac always reflects current claim state.
In `@src/api/ble/localProvider.ts`:
- Around line 166-184: In startDiscovery (async startDiscovery) you currently
catch errors from the call to StartDiscovery and only debug-log them, which
swallows failures and leaves the provider registered but not scanning; change
the handler to not hide the failure: remove the silent swallow and rethrow the
caught error (or return a rejected Promise) after logging, and ensure that side
effects (setting this.scanning = true and calling this.startDeviceWatcher())
only happen after StartDiscovery succeeds; reference the StartDiscovery call,
the this.scanning assignment, and startDeviceWatcher to locate where to move the
state changes and where to rethrow the error.
- Around line 291-296: The RSSI presence check uses a truthy test which treats 0
as missing; update the condition in the block that reads changedProps.RSSI (in
the method using unwrapVariant and this.lastRssi) to explicitly check for
undefined (e.g., changedProps.RSSI !== undefined) so a legitimate 0 value is
accepted, then call rssi = this.unwrapVariant(changedProps.RSSI) and
this.lastRssi.set(mac, rssi) as before; keep the else branch that falls back to
this.lastRssi.get(mac) ?? -127 unchanged.
- Around line 563-571: Duplicate firing occurs because attemptReconnect calls
executeGATTLifecycle(session) which itself invokes session.connectCallbacks, and
attemptReconnect then re-iterates session.connectCallbacks; remove the redundant
invocation in attemptReconnect by deleting the post-executeGATTLifecycle loop
(the for (const cb of session.connectCallbacks) { try { cb() } catch {} }) or
replace it with a guard that only calls callbacks if executeGATTLifecycle did
not already signal success (e.g., check a session.connected flag set by
executeGATTLifecycle) so connectCallbacks are invoked exactly once.
- Around line 186-195: stopDiscovery() currently only stops BlueZ discovery but
leaves the device watcher and PropertiesChanged listeners running; update
stopDiscovery() to also stop the watcher and remove device listeners the same
way shutdown() does: clear the interval/timeout created by startDeviceWatcher()
(e.g. clearInterval(this.deviceWatcher) and set this.deviceWatcher = undefined),
remove/unsubscribe any PropertiesChanged listeners or signal handlers attached
to devices, and ensure this.scanning is set to false; reference
startDeviceWatcher(), shutdown(), and this.deviceWatcher/PropertiesChanged
listeners to locate where to perform the cleanup.
- Around line 652-668: The code can throw during device.gatt() or while
attaching the 'disconnect' listener which prevents releaseSlot() from running;
wrap the gatt setup and listener registration in a try/catch/finally (or ensure
cleanup on any thrown error) so that releaseSlot() is always called if the
connection setup fails, and also ensure the partially-initialized connection is
closed (e.g., call device.disconnect() or close gattServer if created) when an
error occurs; update the block around device.gatt(), the connected flag logic,
and the listener registration to perform these cleanup steps and avoid leaking
reserved GATT slots.
In `@src/api/ble/openApi.json`:
- Around line 423-429: The GET operations for the WebSocket endpoints (paths
"/gateway/ws" and "/advertisements") are missing the required "responses" object
which makes the OpenAPI invalid; add a "responses" property to each GET
operation (e.g., in the operation objects under paths["/gateway/ws"].get and
paths["/advertisements"].get) containing at least a default or 101/200 response
description (and content if appropriate) so validators accept the document;
ensure the responses are descriptive (e.g., "101 Switching Protocols" for
WebSocket upgrade or a default error response) and follow the same structure as
other operations in the spec.
In `@src/api/ble/remoteProvider.ts`:
- Around line 269-283: The gatt_disconnected handler currently only sets
session.connected = false and invokes session.disconnectCallbacks, leaving the
session in this.sessions and keeping the slot counted; mirror the logic from the
gatt_error handler by removing the session from this.sessions and decrementing
this.activeSlots when handling 'gatt_disconnected' (use the same
sessionId/session retrieval pattern), ensuring you still call
disconnectCallbacks and log the reason, and wrap callback invocation in the same
try/catch used now so no behavior changes aside from releasing the slot.
- Around line 923-932: The cleanup loop for timed-out HTTP-only gateways
currently deletes seenMacs, lastPostTime, and httpGatewayMeta but misses
removing advCallbacks entries and the provider registered via
_registerGatewayProvider(), causing memory/state leaks; update the block (the
same place that checks this.sessions and this.snapshots and compares
OFFLINE_SNAPSHOT_MS) to also delete this.advCallbacks.delete(gatewayId) and call
the corresponding unregister/remove for the provider created by
_registerGatewayProvider() (or remove it from whatever provider registry/map you
store it in) so the gateway's callbacks and provider are fully cleaned up.
- Around line 327-341: The code building the remote `cmd` drops/fromats
descriptor options needed by the gateway; update the mappings in the `if
(descriptor.poll)`, `if (descriptor.periodicWrite)`, and `if (descriptor.init)`
branches so that poll[].writeBeforeRead is forwarded as
poll[].write_before_read, periodicWrite[].withResponse as
periodic_write[].with_response, and any init[] items with withResponse are
translated to with_response (keep existing translations like intervalMs ->
interval_ms and uuid/data fields); specifically modify the
`descriptor.poll.map`, `descriptor.periodicWrite.map`, and the assignment for
`descriptor.init` to include these snake_case fields (e.g., add
write_before_read and with_response) so ESP32 gateways receive the
GATTSubscriptionDescriptor options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2904632-d935-4ade-bd7e-d990fcf4e03e
📒 Files selected for processing (28)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.jsonsrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
tkurki
reviewed
May 18, 2026
tkurki
left a comment
tkurki
left a comment
Member
There was a problem hiding this comment.
Some initial comments inline.
Additionally: I thought the idea with TypeBox is that we use it as the source of truth for OpenApi data structures and API validation, but none of that is happening here?
And what aboutAsyncApi?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
packages/server-api/src/bleapi.ts (1)
89-102:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExpose provider-offline disconnect signaling on
GATTSubscriptionHandle.
src/api/ble/index.tsstill needs an internal_fireDisconnectpath because this interface cannot represent forced provider-offline disconnect notification. Please add a public method onGATTSubscriptionHandlefor this event and remove internal escape-hatch dependence.#!/bin/bash # Verify remaining untyped/off-contract disconnect hooks rg -n "_fireDisconnect|as any" packages/server-api/src src/api/bleAs per coding guidelines "Use strict type checking; avoid
anyor equivalent escape hatches".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-api/src/bleapi.ts` around lines 89 - 102, The GATTSubscriptionHandle interface lacks a public method to signal provider-offline disconnects, causing code to rely on the internal _fireDisconnect escape hatch; add a clearly named public method (e.g., notifyProviderOffline or signalProviderOffline) to GATTSubscriptionHandle, update all concrete implementations of the interface in src/api/ble (where _fireDisconnect currently exists) to implement this method, and replace internal calls that cast to any or call _fireDisconnect with calls to the new public method so the provider-offline disconnect is exposed via the typed API rather than an internal escape hatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/plugins/ble_provider_plugins.md`:
- Around line 195-200: The section currently hardcodes wire-level frame
sequencing (the HTTP POST endpoint `/signalk/v2/api/ble/gateway/advertisements`,
the WebSocket `/signalk/v2/api/ble/gateway/ws` and listed frames like `hello`,
`gatt_subscribe`, `gatt_write`, `gatt_data`, `gatt_connected`,
`gatt_disconnected`, `status`); remove the detailed frame flow and replace it
with a concise, conceptual description that the gateway supports advertisement
delivery (via HTTP POST) and bidirectional GATT control (via WebSocket), and add
a clear pointer that exact message/frame contracts are defined in the
OpenAPI/AsyncAPI specs (link to ../rest-api/ble_api.md) so implementation
details are not duplicated in this doc.
In `@docs/develop/rest-api/ble_api.md`:
- Line 55: The listed remote-gateway endpoints currently repeat the `/ble`
segment and should be written relative to the declared base path
`/signalk/v2/api/ble`; update the entries that show `GET /ble/gateways`, `POST
/ble/gateway/advertisements`, and `WS /ble/gateway/ws` to either full absolute
paths or base-relative paths consistently — e.g. change them to `GET /gateways`,
`POST /gateway/advertisements`, and `WS /gateway/ws` (or to
`/signalk/v2/api/ble/gateways`, `/signalk/v2/api/ble/gateway/advertisements`,
`/signalk/v2/api/ble/gateway/ws` if you prefer absolute form).
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 310-311: The JSX currently uses truthy checks so numeric zero
values for gw.uptime and gw.freeHeap render as '-' instead of showing "0";
update the checks to nullish checks (e.g. use gw.uptime != null ?
formatDuration(gw.uptime) : '-' and gw.freeHeap != null ?
formatBytes(gw.freeHeap) : '-') so zero is treated as a valid value while still
hiding null/undefined. Ensure you change the expressions around formatDuration
and formatBytes in BLEManager.tsx where gw.uptime and gw.freeHeap are rendered.
In `@src/api/ble/asyncApi.ts`:
- Around line 29-31: Update the AsyncAPI description string that currently reads
"For the REST API documentation, see OpenAPI at `/admin/openapi/`" to point to
the correct docs route `/doc/openapi`; locate the string literal in the asyncApi
description (the template/text block in asyncApi.ts) and replace
`/admin/openapi/` with `/doc/openapi`, preserving the surrounding text and any
`.trim()` or template formatting.
In `@src/api/ble/index.ts`:
- Line 324: The debug message in the advertisement handling hot path eagerly
constructs the string (debug(`Advertisement callback error: ${e.message}`)) even
when debug logging is disabled; update the call to short-circuit using the
debug.enabled guard (i.e., wrap the debug invocation with debug.enabled && ...)
in the advertisement callback loop where the debug variable is used so the
string interpolation is skipped when logging is off.
- Line 1: Remove the file-wide "/* eslint-disable
`@typescript-eslint/no-explicit-any` */", then replace explicit any usages: type
the module's server field as http.Server | undefined (symbol: server), update
the HTTP upgrade event handler parameters to IncomingMessage, Socket, and Buffer
(symbols: the upgrade handler function / upgrade event listener), change error
handler parameter types to Error (symbols: any functions named handleError or
error callbacks), and replace any config access typed as any with the existing
ConfigApp type (symbol: ConfigApp / config). Import IncomingMessage, Socket,
Buffer and http.Server from Node.js and the ConfigApp type from the app config
module, then fix remaining explicit any occurrences to these concrete types.
In `@src/api/ble/localProvider.ts`:
- Around line 94-100: The constructor's default providerId ignores the
adapterName and can collide; change the constructor to accept providerId
optionally (remove fixed default '_localBLE:hci0') and set this.providerId =
providerId ?? `_localBLE:${adapterName}` after assigning adapterName, so that
when callers pass only adapterName (e.g., 'hci1') the providerId is computed
from adapterName; update references to the constructor parameters (constructor,
adapterName, providerId, this.providerId) accordingly to ensure providerId is
derived from adapterName when not explicitly provided.
- Around line 544-578: The disconnect listener is bound to the original
session.device and becomes orphaned when executeGATTLifecycle replaces
session.device (via adapter.waitDevice()); remove or change the one-time guard
and ensure the handler is attached to the current device instance each time
session.device is set — e.g., add/ call a small helper like
attachDisconnectListener(session) from executeGATTLifecycle after assigning
session.device (or re-attach when device changes) so the disconnect handler,
clearSessionTimers, reconnect logic (attemptReconnect) and disconnectCallbacks
always run against the active device instance instead of an old object.
- Around line 600-609: The closeSession method calls session.device.disconnect()
without handling a possible Promise rejection; update closeSession (or the
LocalGATTSession teardown) to handle async disconnects by either making
closeSession async and awaiting session.device.disconnect() inside a try/catch
or by appending a .catch(...) to session.device.disconnect() to swallow/log
errors; ensure you reference the same LocalGATTSession instance and that any
logging uses the existing logger so disconnect failures are not left as
unhandled promise rejections.
In `@src/api/ble/openApi.ts`:
- Line 72: The global OpenAPI security setting in openApi.ts is applying
authentication to all operations including the public GET /gateways path; update
the GET /gateways operation (the paths["/gateways"].get entry) to override and
mark it unauthenticated by adding an operation-level "security": [] entry so it
does not inherit the global security requirement.
---
Duplicate comments:
In `@packages/server-api/src/bleapi.ts`:
- Around line 89-102: The GATTSubscriptionHandle interface lacks a public method
to signal provider-offline disconnects, causing code to rely on the internal
_fireDisconnect escape hatch; add a clearly named public method (e.g.,
notifyProviderOffline or signalProviderOffline) to GATTSubscriptionHandle,
update all concrete implementations of the interface in src/api/ble (where
_fireDisconnect currently exists) to implement this method, and replace internal
calls that cast to any or call _fireDisconnect with calls to the new public
method so the provider-offline disconnect is exposed via the typed API rather
than an internal escape hatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffddf07b-97ce-43fb-9124-57f1eb57113d
📒 Files selected for processing (29)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/asyncApi.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/api/ble/localProvider.ts (1)
3-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the header comment to the current default behavior.
This comment states
localBluetoothManageddefaults to true on Linux, which is stale and misleading for readers of this file.As per coding guidelines "Documentation describes current state, not development history - avoid changelog-style language".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 3 - 5, Update the header comment for the Local BLE Provider to remove the stale "default: true on Linux" claim and instead state the current behavior: indicate that registration is controlled by the localBluetoothManaged setting (refer to BLEApi configuration) rather than asserting a platform-specific default; update the comment text in src/api/ble/localProvider.ts where "Local BLE Provider" and "localBluetoothManaged" are mentioned to reflect this change.packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (1)
123-124: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract GATT slot bounds/default into named constants.
1and10are repeated in props/clamping, which makes the validation contract easy to drift. Please centralize these as module-level constants (and reuse in both JSX props and clamp logic).As per coding guidelines "No magic numbers; use named constants".
Also applies to: 133-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 123 - 124, Extract the repeated magic numbers for GATT slot bounds into module-level named constants (e.g., GATT_SLOT_MIN = 1, GATT_SLOT_MAX = 10 and a GATT_SLOT_DEFAULT if applicable) and replace all literal 1/10 occurrences in the BLESettings component with those constants; update both the JSX props (min={...} max={...}) and any clamp/validation logic (the code around the clamp usage near the min/max props) to reference the new constants so the validation contract is centralized and cannot drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server-api/src/typebox/ble-schemas.ts`:
- Around line 298-299: The BLESettingsRequest schema allows fractional slot
counts because localMaxGATTSlots uses Type.Optional(Type.Number({ minimum: 1,
maximum: 10 })); change that to Type.Optional(Type.Integer({ minimum: 1,
maximum: 10 })) so the request boundary only accepts integers. Locate the
BLESettingsRequest schema and update the localMaxGATTSlots type accordingly,
keeping the optional wrapper and same min/max constraints.
In `@src/api/ble/localProvider.ts`:
- Around line 567-573: The retry path after executeGATTLifecycle() failures may
schedule reconnects without cleaning up partial connections, causing stale
state; before scheduling attemptReconnect() in the catch block (where debug and
nextDelay are computed) ensure you run the session/device cleanup routine (e.g.,
call the existing disconnect/cleanup function or
session.disconnect()/cleanupSession() equivalent) and await or best-effort catch
its errors, mark session as closed if appropriate, then proceed to schedule
attemptReconnect(attempt + 1) only after that cleanup attempt; reference
executeGATTLifecycle, attemptReconnect, and session.closed to locate where to
insert this cleanup logic.
In `@src/api/ble/remoteProvider.ts`:
- Around line 650-656: The debug call inside the advertisement callback loop
(the try/catch in the for (const cb of callbacks) loop where cb(adv) is invoked)
should be guarded to avoid string interpolation when logging is disabled: check
debug.enabled before calling debug(...) (e.g., wrap the debug invocation in an
if (debug.enabled) block) so the message `(e as Error).message` is only
evaluated when logging is active; update the catch block around the adv callback
to perform this guard.
---
Duplicate comments:
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 123-124: Extract the repeated magic numbers for GATT slot bounds
into module-level named constants (e.g., GATT_SLOT_MIN = 1, GATT_SLOT_MAX = 10
and a GATT_SLOT_DEFAULT if applicable) and replace all literal 1/10 occurrences
in the BLESettings component with those constants; update both the JSX props
(min={...} max={...}) and any clamp/validation logic (the code around the clamp
usage near the min/max props) to reference the new constants so the validation
contract is centralized and cannot drift.
In `@src/api/ble/localProvider.ts`:
- Around line 3-5: Update the header comment for the Local BLE Provider to
remove the stale "default: true on Linux" claim and instead state the current
behavior: indicate that registration is controlled by the localBluetoothManaged
setting (refer to BLEApi configuration) rather than asserting a
platform-specific default; update the comment text in
src/api/ble/localProvider.ts where "Local BLE Provider" and
"localBluetoothManaged" are mentioned to reflect this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eef3116d-7dc1-44ed-aafe-6e0dd621ed21
📒 Files selected for processing (29)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/asyncApi.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
Comment on lines
+298
to
+299
| localMaxGATTSlots: Type.Optional(Type.Number({ minimum: 1, maximum: 10 })) | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where localMaxGATTSlots is defined and consumed as a count-like value.
rg -n -C2 '\blocalMaxGATTSlots\b'Repository: SignalK/signalk-server
Length of output: 6831
Make localMaxGATTSlots an integer in BLESettingsRequest schema
packages/server-api/src/typebox/ble-schemas.ts currently uses Type.Optional(Type.Number({ minimum: 1, maximum: 10 })), which allows fractional slot counts (e.g. 2.5). Since this is a slot count, switch to Type.Integer({ minimum: 1, maximum: 10 }) at the request boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server-api/src/typebox/ble-schemas.ts` around lines 298 - 299, The
BLESettingsRequest schema allows fractional slot counts because
localMaxGATTSlots uses Type.Optional(Type.Number({ minimum: 1, maximum: 10 }));
change that to Type.Optional(Type.Integer({ minimum: 1, maximum: 10 })) so the
request boundary only accepts integers. Locate the BLESettingsRequest schema and
update the localMaxGATTSlots type accordingly, keeping the optional wrapper and
same min/max constraints.
Comment on lines
+567
to
+573
| } catch (e: any) { | ||
| debug(`GATT reconnect failed for ${mac}: ${e.message}`) | ||
| if (!session.closed) { | ||
| const nextDelay = Math.min(5000 * Math.pow(2, attempt), 60000) | ||
| debug(`GATT retry ${mac} in ${nextDelay / 1000}s`) | ||
| attemptReconnect(attempt + 1) | ||
| } |
Contributor
There was a problem hiding this comment.
Add cleanup before scheduling reconnect retries after lifecycle failure.
If executeGATTLifecycle() fails after a partial connect, the retry path immediately reschedules without a best-effort disconnect/cleanup, which can accumulate stale connection state across attempts.
Possible minimal hardening
} catch (e: any) {
debug(`GATT reconnect failed for ${mac}: ${e.message}`)
+ session.connected = false
+ try {
+ await session.device?.disconnect?.()
+ } catch (_e) {
+ // ignore cleanup errors
+ }
if (!session.closed) {
const nextDelay = Math.min(5000 * Math.pow(2, attempt), 60000)
debug(`GATT retry ${mac} in ${nextDelay / 1000}s`)
attemptReconnect(attempt + 1)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/ble/localProvider.ts` around lines 567 - 573, The retry path after
executeGATTLifecycle() failures may schedule reconnects without cleaning up
partial connections, causing stale state; before scheduling attemptReconnect()
in the catch block (where debug and nextDelay are computed) ensure you run the
session/device cleanup routine (e.g., call the existing disconnect/cleanup
function or session.disconnect()/cleanupSession() equivalent) and await or
best-effort catch its errors, mark session as closed if appropriate, then
proceed to schedule attemptReconnect(attempt + 1) only after that cleanup
attempt; reference executeGATTLifecycle, attemptReconnect, and session.closed to
locate where to insert this cleanup logic.
4a87d14 to
add50ed
Compare
Contributor
Author
|
@tkurki Good catch — reworked so TypeBox really is the single source of truth:
AsyncAPI: yes — both WebSocket channels (the advertisement stream and the gateway GATT command channel) are described in an AsyncAPI document ( |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
Contributor
Author
|
Seems I broke the rabbit. What do you prefer? |
dirkwa
force-pushed
the
ble-provider-api
branch
2 times, most recently
from
a27180f to
c3898a4
Compare
Contributor
Author
|
rebase |
tkurki
reviewed
May 31, 2026
tkurki
left a comment
tkurki
left a comment
Member
There was a problem hiding this comment.
I have such shallow BT knowledge that I feel a bit ill at ease to comment on the actual subject matter here.
The server has an idea of interfaces, in server settings and (in limited format) in code.
Would it make sense to make this an interface? Or is there another way to turn it off, if the user does not to provide remote access to their BT?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server-admin-ui/src/views/ServerConfig/Settings.tsx (1)
162-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore failure handling in
handleSaveSettings.This now handles only the success path. A 4xx/5xx response looks like a no-op, and a thrown
fetchrejection becomes unhandled. Keep the non-blocking error feedback, but move it to component state and an inlineAlert/toast instead ofalert(). Based on learnings, this UI prefers persistent, non-blocking feedback over popups.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/Settings.tsx` around lines 162 - 175, handleSaveSettings currently only handles the success path, so 4xx/5xx responses and fetch rejections are silently ignored. Update the Settings component’s handleSaveSettings flow to detect non-ok responses and catch thrown fetch errors, then store the failure message in component state. Surface that state through the existing UI with an inline Alert or toast instead of alert(), while keeping the current non-blocking feedback pattern and preserving the success path that calls useStore.getState().setRestartRequired(true).Source: Learnings
♻️ Duplicate comments (4)
packages/server-api/src/bleapi.ts (1)
74-87: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftExpose provider-forced disconnects in the public handle.
GATTSubscriptionHandlestill gives the BLE core no typed way to tell consumers that a claim was torn down because the provider disappeared. That keeps the provider-offline path coupled to provider-specific internals again, which is the same contract gap raised earlier.#!/bin/bash set -euo pipefail ast-grep outline packages/server-api/src/bleapi.ts src/api/ble/index.ts src/api/ble/localProvider.ts src/api/ble/remoteProvider.ts --view expanded echo echo "Searching for provider-offline disconnect hooks / any-casts..." rg -n -C3 "_fireDisconnect|fireDisconnect|signalDisconnect|provider.*offline|handle as any" \ packages/server-api/src/bleapi.ts \ src/api/ble/index.ts \ src/api/ble/localProvider.ts \ src/api/ble/remoteProvider.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-api/src/bleapi.ts` around lines 74 - 87, Expose a typed provider-forced disconnect path on GATTSubscriptionHandle so the BLE core can signal when a claim is torn down because the provider disappeared. Update the GATTSubscriptionHandle contract in bleapi.ts with a dedicated disconnect notification or reason API, then wire the provider-offline flow through the handle implementation in localProvider/remoteProvider and the BLE entrypoint so consumers no longer need any-casts or provider-specific internals to observe this state.src/api/ble/localProvider.ts (3)
564-573: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the partially reopened session before scheduling another retry.
If
executeGATTLifecycle()fails afterConnect, this catch just queues the next attempt. The old BlueZ connection/state is left behind, so repeated retries can accumulate stale sessions and permanently eat GATT capacity. This was flagged earlier and still appears unresolved.Suggested hardening
try { // executeGATTLifecycle fires connectCallbacks itself on success await this.executeGATTLifecycle(session) } catch (e: any) { debug(`GATT reconnect failed for ${mac}: ${e.message}`) + session.connected = false + try { + await session.device?.disconnect?.() + } catch (_e) { + // ignore cleanup errors + } if (!session.closed) { const nextDelay = Math.min(5000 * Math.pow(2, attempt), 60000) debug(`GATT retry ${mac} in ${nextDelay / 1000}s`) attemptReconnect(attempt + 1) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 564 - 573, The reconnect catch in localProvider.ts leaves a failed GATT session hanging after executeGATTLifecycle(session) throws, so clean up the partially reopened session before scheduling attemptReconnect again. Update the reconnect path in the GATT retry logic to explicitly close/disconnect or otherwise dispose the current session state when connect succeeds but lifecycle fails, then only queue the next attempt after cleanup. Use the existing symbols executeGATTLifecycle, attemptReconnect, session, and the GATT reconnect failure catch block to locate and harden this path.
579-585: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCapture the bound device in
removeDisconnectListener().This cleanup closes over
session.device, but that field has already been reassigned before the old listener is removed on reconnect. IfwaitDevice()returns a new object, the old listener stays attached and can trigger duplicate reconnects/callbacks later. This is still the same disconnect-listener bug path raised earlier.Suggested fix
- session.device.on('disconnect', onDisconnect) + const boundDevice = session.device + boundDevice.on('disconnect', onDisconnect) session.removeDisconnectListener = () => { try { - session.device?.removeListener?.('disconnect', onDisconnect) + boundDevice?.removeListener?.('disconnect', onDisconnect) } catch (_e) { // ignore } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 579 - 585, The disconnect-listener cleanup in localProvider’s session setup still closes over session.device, so reconnects can leave the old listener attached. Update the logic around session.removeDisconnectListener and the session.device.on('disconnect', onDisconnect) registration to capture the current device in a local variable and remove the listener from that bound device instead of reading session.device later. Keep the cleanup tied to the same device instance that was registered, so waitDevice() reconnects do not leave stale disconnect handlers behind.
254-270: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort listener setup if discovery was stopped mid-attach.
stopDiscovery()clears the current listener map, but anattachDeviceListener()already past Line 257 will still emit an advertisement and install a freshPropertiesChangedlistener after scanning has been turned off. That reopens the post-stop leak on a simple stop/attach race.Suggested fix
private async attachDeviceListener(mac: string) { // Reserve the slot before the await chain so concurrent watchDevices() // iterations don't both kick off an attach for the same MAC. this.deviceListeners.set(mac, () => {}) try { const device = await this.adapter.waitDevice(mac, 1) + if (!this.scanning || !this.deviceListeners.has(mac)) return await device.helper._prepare() + if (!this.scanning || !this.deviceListeners.has(mac)) return this.emitDeviceAdvertisement(device, mac)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 254 - 270, Abort the attach flow in attachDeviceListener if discovery has been stopped after the initial reservation, since stopDiscovery() can clear deviceListeners while the await chain is in flight. After waitDevice() and after _prepare(), check whether the MAC is still present in deviceListeners (or another equivalent active-discovery guard) before calling emitDeviceAdvertisement or registering the PropertiesChanged handler on device.helper. If the listener was cleared, exit without emitting or re-adding the cleanup callback so stop/attach races cannot reopen the leak.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/rest-api/ble_api.md`:
- Around line 63-69: Replace the implementation-detail-heavy prose in this BLE
API docs section with a stable, conceptual summary of the public contract:
explain that the TypeBox schemas in `@signalk/server-api/typebox` are the source
of truth, that they generate the OpenAPI schema and consumer types, and that
method-bearing interfaces are documented separately in TypeDoc. Remove direct
references to internal file names, validation calls, and generated-code
mechanics; keep only the high-level architecture and the pointers to the
generated documentation entry points.
In `@docs/develop/rest-api/README.md`:
- Line 35: The BLE API summary in the README should stay gateway-agnostic and
describe the feature as a generic remote-provider capability. Update the table
entry for BLE so it no longer mentions ESP32 gateways, and instead uses wording
that matches the broader contract implemented by the BLE API and provider
plugins. Use the BLE API label and its surrounding top-level summary table as
the location cue.
In `@package.json`:
- Line 3: Revert the top-level version change in package.json so the root
package version stays untouched by contributors. Update the package.json version
field back to its previous value and leave any version management to maintainers
at release time; only dependency ranges should remain modified here.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Line 217: Remove the section-marker comments in BLEManager that only restate
the nearby card titles, since they add no value and violate the “explain why,
not what” guidance. Delete the standalone marker comments near the relevant JSX
blocks in the BLEManager component, keeping the actual card titles and
functional code intact; only retain comments that provide non-obvious reasoning
or context.
- Around line 140-150: The fetchBleSettings flow in BLEManager currently leaves
bleSettings as null on load failure, which makes the derived
localBLESupported/localManaged defaults render a false “Local Bluetooth is
disabled” state. Add an explicit load/error state alongside bleSettings, set it
in fetchBleSettings and any related settings-loading paths, and update the
render logic so the empty-state message only appears after settings have
successfully loaded. Apply the same distinction in the other affected BLEManager
rendering spots tied to the same state.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 49-55: The BLE settings save flow in BLESettings should not use
blocking alert() calls for failures. Update the save handler around the res.ok
check and catch block to set a local error state instead, and render that state
with a Bootstrap Alert or toast near the form. Keep the existing save logic in
the same function, but replace the alert-based error reporting with the
component’s local UI feedback path.
In `@src/api/ble/asyncApi.ts`:
- Around line 18-20: Update the advertisement stream documentation in
asyncApi.ts so it no longer claims the WebSocket channel is deduplicated
server-side; the stream emits every raw BLEAdvertisement frame from registered
providers, while only /devices is deduplicated. Adjust the description near the
Advertisement Stream section and the related schema/docs around the referenced
BLEAdvertisement channel to clearly state raw frame delivery and provider-level
visibility.
In `@src/api/ble/index.ts`:
- Line 82: The BLE API is introducing a default-provider concept that conflicts
with the approved multi-provider/no-default scope. Remove the persisted
default-provider state and the public get/set default-provider endpoints from
the BLE API, and update `selectGATTProvider()` so it no longer prefers a stored
default provider when choosing a provider. Keep the provider selection and
failover logic aligned with the existing multi-provider model in `BLEApi` and
related default-provider methods.
- Around line 788-823: The BLE settings update flow in the handler that mutates
`this.settings` and calls `writeSettingsFile()` is applying changes in memory
before persistence succeeds, which can leave `/settings` and provider state out
of sync on failure. Update the settings write path so the incoming
`body.localBluetoothManaged`, `body.localAdapters`, and `body.localMaxGATTSlots`
values are staged separately, persist first, and only assign them to
`this.settings` and `this.app.config.settings.bleApi` after
`writeSettingsFile()` succeeds.
- Around line 406-464: subscribeGATT() only releases the claim in the wrapped
handle.close(), so an unexpected provider disconnect leaves the MAC stuck in
gattClaims and the keepAliveTimer running. Update the subscribeGATT flow to also
hook handle.onDisconnect() (or the provider disconnect path) in the same place
where handle.close is wrapped, and make it share the same cleanup logic that
clears the interval, removes the gattClaims entry, and logs the release for the
mac/pluginId pair.
- Around line 356-389: `BLEService.getDevices()` already prunes stale entries,
but `getDevice()` and `selectGATTProvider()` can still read or act on expired
state. Add a `pruneStaleDevices()` call at the start of `getDevice()` and inside
`selectGATTProvider()` before checking `deviceTable` or choosing a provider, so
direct lookups and GATT claim routing only use devices seen within
`DEVICE_STALE_MS`.
In `@src/api/ble/remoteProvider.ts`:
- Around line 659-662: The HTTP-only gateway cleanup is only triggered from
_handleAdvertisementPost(), so stale providers can remain registered if no later
POST arrives. Update RemoteProvider to also prune expired HTTP-only gateways
from the provider registry in the same lifecycle used by
_pruneStaleHttpGateways(), and ensure the registration removal logic stays
consistent with the gateway aging behavior in /gateways and /_providers. Use the
existing symbols _pruneStaleHttpGateways, _handleAdvertisementPost, and the
provider registration path in RemoteProvider to wire this cleanup into a place
that runs even without new POSTs.
---
Outside diff comments:
In `@packages/server-admin-ui/src/views/ServerConfig/Settings.tsx`:
- Around line 162-175: handleSaveSettings currently only handles the success
path, so 4xx/5xx responses and fetch rejections are silently ignored. Update the
Settings component’s handleSaveSettings flow to detect non-ok responses and
catch thrown fetch errors, then store the failure message in component state.
Surface that state through the existing UI with an inline Alert or toast instead
of alert(), while keeping the current non-blocking feedback pattern and
preserving the success path that calls
useStore.getState().setRestartRequired(true).
---
Duplicate comments:
In `@packages/server-api/src/bleapi.ts`:
- Around line 74-87: Expose a typed provider-forced disconnect path on
GATTSubscriptionHandle so the BLE core can signal when a claim is torn down
because the provider disappeared. Update the GATTSubscriptionHandle contract in
bleapi.ts with a dedicated disconnect notification or reason API, then wire the
provider-offline flow through the handle implementation in
localProvider/remoteProvider and the BLE entrypoint so consumers no longer need
any-casts or provider-specific internals to observe this state.
In `@src/api/ble/localProvider.ts`:
- Around line 564-573: The reconnect catch in localProvider.ts leaves a failed
GATT session hanging after executeGATTLifecycle(session) throws, so clean up the
partially reopened session before scheduling attemptReconnect again. Update the
reconnect path in the GATT retry logic to explicitly close/disconnect or
otherwise dispose the current session state when connect succeeds but lifecycle
fails, then only queue the next attempt after cleanup. Use the existing symbols
executeGATTLifecycle, attemptReconnect, session, and the GATT reconnect failure
catch block to locate and harden this path.
- Around line 579-585: The disconnect-listener cleanup in localProvider’s
session setup still closes over session.device, so reconnects can leave the old
listener attached. Update the logic around session.removeDisconnectListener and
the session.device.on('disconnect', onDisconnect) registration to capture the
current device in a local variable and remove the listener from that bound
device instead of reading session.device later. Keep the cleanup tied to the
same device instance that was registered, so waitDevice() reconnects do not
leave stale disconnect handlers behind.
- Around line 254-270: Abort the attach flow in attachDeviceListener if
discovery has been stopped after the initial reservation, since stopDiscovery()
can clear deviceListeners while the await chain is in flight. After waitDevice()
and after _prepare(), check whether the MAC is still present in deviceListeners
(or another equivalent active-discovery guard) before calling
emitDeviceAdvertisement or registering the PropertiesChanged handler on
device.helper. If the listener was cleared, exit without emitting or re-adding
the cleanup callback so stop/attach races cannot reopen the leak.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d30c7ac-caba-4aed-9711-bec80f679a51
📒 Files selected for processing (29)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/asyncApi.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
💤 Files with no reviewable changes (4)
- src/serverroutes.ts
- src/dummysecurity.ts
- src/config/config.ts
- src/interfaces/plugins.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server-admin-ui/src/views/ServerConfig/Settings.tsx (1)
162-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore failure handling in
handleSaveSettings.This now handles only the success path. A 4xx/5xx response looks like a no-op, and a thrown
fetchrejection becomes unhandled. Keep the non-blocking error feedback, but move it to component state and an inlineAlert/toast instead ofalert(). Based on learnings, this UI prefers persistent, non-blocking feedback over popups.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/Settings.tsx` around lines 162 - 175, handleSaveSettings currently only handles the success path, so 4xx/5xx responses and fetch rejections are silently ignored. Update the Settings component’s handleSaveSettings flow to detect non-ok responses and catch thrown fetch errors, then store the failure message in component state. Surface that state through the existing UI with an inline Alert or toast instead of alert(), while keeping the current non-blocking feedback pattern and preserving the success path that calls useStore.getState().setRestartRequired(true).Source: Learnings
♻️ Duplicate comments (4)
packages/server-api/src/bleapi.ts (1)
74-87: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftExpose provider-forced disconnects in the public handle.
GATTSubscriptionHandlestill gives the BLE core no typed way to tell consumers that a claim was torn down because the provider disappeared. That keeps the provider-offline path coupled to provider-specific internals again, which is the same contract gap raised earlier.#!/bin/bash set -euo pipefail ast-grep outline packages/server-api/src/bleapi.ts src/api/ble/index.ts src/api/ble/localProvider.ts src/api/ble/remoteProvider.ts --view expanded echo echo "Searching for provider-offline disconnect hooks / any-casts..." rg -n -C3 "_fireDisconnect|fireDisconnect|signalDisconnect|provider.*offline|handle as any" \ packages/server-api/src/bleapi.ts \ src/api/ble/index.ts \ src/api/ble/localProvider.ts \ src/api/ble/remoteProvider.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-api/src/bleapi.ts` around lines 74 - 87, Expose a typed provider-forced disconnect path on GATTSubscriptionHandle so the BLE core can signal when a claim is torn down because the provider disappeared. Update the GATTSubscriptionHandle contract in bleapi.ts with a dedicated disconnect notification or reason API, then wire the provider-offline flow through the handle implementation in localProvider/remoteProvider and the BLE entrypoint so consumers no longer need any-casts or provider-specific internals to observe this state.src/api/ble/localProvider.ts (3)
564-573: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the partially reopened session before scheduling another retry.
If
executeGATTLifecycle()fails afterConnect, this catch just queues the next attempt. The old BlueZ connection/state is left behind, so repeated retries can accumulate stale sessions and permanently eat GATT capacity. This was flagged earlier and still appears unresolved.Suggested hardening
try { // executeGATTLifecycle fires connectCallbacks itself on success await this.executeGATTLifecycle(session) } catch (e: any) { debug(`GATT reconnect failed for ${mac}: ${e.message}`) + session.connected = false + try { + await session.device?.disconnect?.() + } catch (_e) { + // ignore cleanup errors + } if (!session.closed) { const nextDelay = Math.min(5000 * Math.pow(2, attempt), 60000) debug(`GATT retry ${mac} in ${nextDelay / 1000}s`) attemptReconnect(attempt + 1) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 564 - 573, The reconnect catch in localProvider.ts leaves a failed GATT session hanging after executeGATTLifecycle(session) throws, so clean up the partially reopened session before scheduling attemptReconnect again. Update the reconnect path in the GATT retry logic to explicitly close/disconnect or otherwise dispose the current session state when connect succeeds but lifecycle fails, then only queue the next attempt after cleanup. Use the existing symbols executeGATTLifecycle, attemptReconnect, session, and the GATT reconnect failure catch block to locate and harden this path.
579-585: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCapture the bound device in
removeDisconnectListener().This cleanup closes over
session.device, but that field has already been reassigned before the old listener is removed on reconnect. IfwaitDevice()returns a new object, the old listener stays attached and can trigger duplicate reconnects/callbacks later. This is still the same disconnect-listener bug path raised earlier.Suggested fix
- session.device.on('disconnect', onDisconnect) + const boundDevice = session.device + boundDevice.on('disconnect', onDisconnect) session.removeDisconnectListener = () => { try { - session.device?.removeListener?.('disconnect', onDisconnect) + boundDevice?.removeListener?.('disconnect', onDisconnect) } catch (_e) { // ignore } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 579 - 585, The disconnect-listener cleanup in localProvider’s session setup still closes over session.device, so reconnects can leave the old listener attached. Update the logic around session.removeDisconnectListener and the session.device.on('disconnect', onDisconnect) registration to capture the current device in a local variable and remove the listener from that bound device instead of reading session.device later. Keep the cleanup tied to the same device instance that was registered, so waitDevice() reconnects do not leave stale disconnect handlers behind.
254-270: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort listener setup if discovery was stopped mid-attach.
stopDiscovery()clears the current listener map, but anattachDeviceListener()already past Line 257 will still emit an advertisement and install a freshPropertiesChangedlistener after scanning has been turned off. That reopens the post-stop leak on a simple stop/attach race.Suggested fix
private async attachDeviceListener(mac: string) { // Reserve the slot before the await chain so concurrent watchDevices() // iterations don't both kick off an attach for the same MAC. this.deviceListeners.set(mac, () => {}) try { const device = await this.adapter.waitDevice(mac, 1) + if (!this.scanning || !this.deviceListeners.has(mac)) return await device.helper._prepare() + if (!this.scanning || !this.deviceListeners.has(mac)) return this.emitDeviceAdvertisement(device, mac)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 254 - 270, Abort the attach flow in attachDeviceListener if discovery has been stopped after the initial reservation, since stopDiscovery() can clear deviceListeners while the await chain is in flight. After waitDevice() and after _prepare(), check whether the MAC is still present in deviceListeners (or another equivalent active-discovery guard) before calling emitDeviceAdvertisement or registering the PropertiesChanged handler on device.helper. If the listener was cleared, exit without emitting or re-adding the cleanup callback so stop/attach races cannot reopen the leak.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/rest-api/ble_api.md`:
- Around line 63-69: Replace the implementation-detail-heavy prose in this BLE
API docs section with a stable, conceptual summary of the public contract:
explain that the TypeBox schemas in `@signalk/server-api/typebox` are the source
of truth, that they generate the OpenAPI schema and consumer types, and that
method-bearing interfaces are documented separately in TypeDoc. Remove direct
references to internal file names, validation calls, and generated-code
mechanics; keep only the high-level architecture and the pointers to the
generated documentation entry points.
In `@docs/develop/rest-api/README.md`:
- Line 35: The BLE API summary in the README should stay gateway-agnostic and
describe the feature as a generic remote-provider capability. Update the table
entry for BLE so it no longer mentions ESP32 gateways, and instead uses wording
that matches the broader contract implemented by the BLE API and provider
plugins. Use the BLE API label and its surrounding top-level summary table as
the location cue.
In `@package.json`:
- Line 3: Revert the top-level version change in package.json so the root
package version stays untouched by contributors. Update the package.json version
field back to its previous value and leave any version management to maintainers
at release time; only dependency ranges should remain modified here.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Line 217: Remove the section-marker comments in BLEManager that only restate
the nearby card titles, since they add no value and violate the “explain why,
not what” guidance. Delete the standalone marker comments near the relevant JSX
blocks in the BLEManager component, keeping the actual card titles and
functional code intact; only retain comments that provide non-obvious reasoning
or context.
- Around line 140-150: The fetchBleSettings flow in BLEManager currently leaves
bleSettings as null on load failure, which makes the derived
localBLESupported/localManaged defaults render a false “Local Bluetooth is
disabled” state. Add an explicit load/error state alongside bleSettings, set it
in fetchBleSettings and any related settings-loading paths, and update the
render logic so the empty-state message only appears after settings have
successfully loaded. Apply the same distinction in the other affected BLEManager
rendering spots tied to the same state.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 49-55: The BLE settings save flow in BLESettings should not use
blocking alert() calls for failures. Update the save handler around the res.ok
check and catch block to set a local error state instead, and render that state
with a Bootstrap Alert or toast near the form. Keep the existing save logic in
the same function, but replace the alert-based error reporting with the
component’s local UI feedback path.
In `@src/api/ble/asyncApi.ts`:
- Around line 18-20: Update the advertisement stream documentation in
asyncApi.ts so it no longer claims the WebSocket channel is deduplicated
server-side; the stream emits every raw BLEAdvertisement frame from registered
providers, while only /devices is deduplicated. Adjust the description near the
Advertisement Stream section and the related schema/docs around the referenced
BLEAdvertisement channel to clearly state raw frame delivery and provider-level
visibility.
In `@src/api/ble/index.ts`:
- Line 82: The BLE API is introducing a default-provider concept that conflicts
with the approved multi-provider/no-default scope. Remove the persisted
default-provider state and the public get/set default-provider endpoints from
the BLE API, and update `selectGATTProvider()` so it no longer prefers a stored
default provider when choosing a provider. Keep the provider selection and
failover logic aligned with the existing multi-provider model in `BLEApi` and
related default-provider methods.
- Around line 788-823: The BLE settings update flow in the handler that mutates
`this.settings` and calls `writeSettingsFile()` is applying changes in memory
before persistence succeeds, which can leave `/settings` and provider state out
of sync on failure. Update the settings write path so the incoming
`body.localBluetoothManaged`, `body.localAdapters`, and `body.localMaxGATTSlots`
values are staged separately, persist first, and only assign them to
`this.settings` and `this.app.config.settings.bleApi` after
`writeSettingsFile()` succeeds.
- Around line 406-464: subscribeGATT() only releases the claim in the wrapped
handle.close(), so an unexpected provider disconnect leaves the MAC stuck in
gattClaims and the keepAliveTimer running. Update the subscribeGATT flow to also
hook handle.onDisconnect() (or the provider disconnect path) in the same place
where handle.close is wrapped, and make it share the same cleanup logic that
clears the interval, removes the gattClaims entry, and logs the release for the
mac/pluginId pair.
- Around line 356-389: `BLEService.getDevices()` already prunes stale entries,
but `getDevice()` and `selectGATTProvider()` can still read or act on expired
state. Add a `pruneStaleDevices()` call at the start of `getDevice()` and inside
`selectGATTProvider()` before checking `deviceTable` or choosing a provider, so
direct lookups and GATT claim routing only use devices seen within
`DEVICE_STALE_MS`.
In `@src/api/ble/remoteProvider.ts`:
- Around line 659-662: The HTTP-only gateway cleanup is only triggered from
_handleAdvertisementPost(), so stale providers can remain registered if no later
POST arrives. Update RemoteProvider to also prune expired HTTP-only gateways
from the provider registry in the same lifecycle used by
_pruneStaleHttpGateways(), and ensure the registration removal logic stays
consistent with the gateway aging behavior in /gateways and /_providers. Use the
existing symbols _pruneStaleHttpGateways, _handleAdvertisementPost, and the
provider registration path in RemoteProvider to wire this cleanup into a place
that runs even without new POSTs.
---
Outside diff comments:
In `@packages/server-admin-ui/src/views/ServerConfig/Settings.tsx`:
- Around line 162-175: handleSaveSettings currently only handles the success
path, so 4xx/5xx responses and fetch rejections are silently ignored. Update the
Settings component’s handleSaveSettings flow to detect non-ok responses and
catch thrown fetch errors, then store the failure message in component state.
Surface that state through the existing UI with an inline Alert or toast instead
of alert(), while keeping the current non-blocking feedback pattern and
preserving the success path that calls
useStore.getState().setRestartRequired(true).
---
Duplicate comments:
In `@packages/server-api/src/bleapi.ts`:
- Around line 74-87: Expose a typed provider-forced disconnect path on
GATTSubscriptionHandle so the BLE core can signal when a claim is torn down
because the provider disappeared. Update the GATTSubscriptionHandle contract in
bleapi.ts with a dedicated disconnect notification or reason API, then wire the
provider-offline flow through the handle implementation in
localProvider/remoteProvider and the BLE entrypoint so consumers no longer need
any-casts or provider-specific internals to observe this state.
In `@src/api/ble/localProvider.ts`:
- Around line 564-573: The reconnect catch in localProvider.ts leaves a failed
GATT session hanging after executeGATTLifecycle(session) throws, so clean up the
partially reopened session before scheduling attemptReconnect again. Update the
reconnect path in the GATT retry logic to explicitly close/disconnect or
otherwise dispose the current session state when connect succeeds but lifecycle
fails, then only queue the next attempt after cleanup. Use the existing symbols
executeGATTLifecycle, attemptReconnect, session, and the GATT reconnect failure
catch block to locate and harden this path.
- Around line 579-585: The disconnect-listener cleanup in localProvider’s
session setup still closes over session.device, so reconnects can leave the old
listener attached. Update the logic around session.removeDisconnectListener and
the session.device.on('disconnect', onDisconnect) registration to capture the
current device in a local variable and remove the listener from that bound
device instead of reading session.device later. Keep the cleanup tied to the
same device instance that was registered, so waitDevice() reconnects do not
leave stale disconnect handlers behind.
- Around line 254-270: Abort the attach flow in attachDeviceListener if
discovery has been stopped after the initial reservation, since stopDiscovery()
can clear deviceListeners while the await chain is in flight. After waitDevice()
and after _prepare(), check whether the MAC is still present in deviceListeners
(or another equivalent active-discovery guard) before calling
emitDeviceAdvertisement or registering the PropertiesChanged handler on
device.helper. If the listener was cleared, exit without emitting or re-adding
the cleanup callback so stop/attach races cannot reopen the leak.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d30c7ac-caba-4aed-9711-bec80f679a51
📒 Files selected for processing (29)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/asyncApi.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
💤 Files with no reviewable changes (4)
- src/serverroutes.ts
- src/dummysecurity.ts
- src/config/config.ts
- src/interfaces/plugins.ts
🛑 Comments failed to post (12)
docs/develop/rest-api/ble_api.md (1)
63-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Trim the implementation-detail dump from this section.
This reads like internal maintenance notes, not stable API docs. Hard-coding
src/api/ble/openApi.ts,Value.Check,Static<>, andbleapi.tswill drift as soon as the internals move, while readers only need the conceptual contract and the generated docs entry points. As per path instructions, "Documentation should describe architecture and how things work conceptually — not step-by-step instructions that duplicate or shadow the code itself."Suggested simplification
-The TypeBox schemas under `@signalk/server-api/typebox` (`ble-schemas.ts`) are the single source of truth. They drive three things: - -- the OpenAPI `components.schemas` (generated from the schemas in `src/api/ble/openApi.ts`, referenced from each path via `$ref`), -- runtime validation of request input (`Value.Check` on the `PUT /settings` body, the gateway advertisement-batch POST body, and the `{mac}` path parameter), and -- the data-shape TypeScript types of the consumer API (`BLEAdvertisement`, `BLEDeviceInfo`, `BLEConsumerInfo`, `BLEProviders`, …), which are derived from the schemas via `Static<>`. - -The method-bearing interfaces that TypeBox cannot express (`BLEApi`, `BLEProvider`, `GATTSubscriptionHandle`, `BLEGattConnection`, …) are hand-written in `bleapi.ts`. All consumer-API types are documented in the TypeDoc output for `@signalk/server-api`. +The BLE TypeBox schemas are the source of truth for the REST/WS contracts and +their corresponding TypeScript data shapes. See the generated OpenAPI, +AsyncAPI, and TypeDoc output for the current concrete schema details.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.The BLE TypeBox schemas are the source of truth for the REST/WS contracts and their corresponding TypeScript data shapes. See the generated OpenAPI, AsyncAPI, and TypeDoc output for the current concrete schema details.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/develop/rest-api/ble_api.md` around lines 63 - 69, Replace the implementation-detail-heavy prose in this BLE API docs section with a stable, conceptual summary of the public contract: explain that the TypeBox schemas in `@signalk/server-api/typebox` are the source of truth, that they generate the OpenAPI schema and consumer types, and that method-bearing interfaces are documented separately in TypeDoc. Remove direct references to internal file names, validation calls, and generated-code mechanics; keep only the high-level architecture and the pointers to the generated documentation entry points.Source: Path instructions
docs/develop/rest-api/README.md (1)
35-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the API summary gateway-agnostic.
The BLE API being added here is a generic remote-provider capability. Calling out "ESP32 gateways" in the top-level table makes the summary narrower than the actual contract and will age badly once other gateway implementations exist. As per path instructions, "Documentation should describe architecture and how things work conceptually — not step-by-step instructions that duplicate or shadow the code itself."
Suggested wording
-| [BLE](./ble_api.md) | Unified BLE device discovery and GATT connection management via provider plugins and ESP32 gateways. | `vessels/self/ble` | +| [BLE](./ble_api.md) | Unified BLE device discovery and GATT connection management via provider plugins and remote BLE gateways. | `vessels/self/ble` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.| [BLE](./ble_api.md) | Unified BLE device discovery and GATT connection management via provider plugins and remote BLE gateways. | `vessels/self/ble` |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/develop/rest-api/README.md` at line 35, The BLE API summary in the README should stay gateway-agnostic and describe the feature as a generic remote-provider capability. Update the table entry for BLE so it no longer mentions ESP32 gateways, and instead uses wording that matches the broader contract implemented by the BLE API and provider plugins. Use the BLE API label and its surrounding top-level summary table as the location cue.Source: Path instructions
package.json (1)
3-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Revert the root
versionbump.Contributors should not change the package’s top-level version; release maintainers handle that at publish time. As per coding guidelines,
**/{package.json,version.ts,VERSION}: NEVER change version numbers - maintainers will update versions when publishing releases.Based on learnings, only dependency ranges should be touched here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 3, Revert the top-level version change in package.json so the root package version stays untouched by contributors. Update the package.json version field back to its previous value and leave any version management to maintainers at release time; only dependency ranges should remain modified here.Sources: Coding guidelines, Learnings
packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx (2)
140-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep “settings unavailable” distinct from “Bluetooth disabled”.
If
/settingsnever resolves,bleSettingsstaysnull, but these defaults turn that intolocalBLESupported = trueandlocalManaged = false, so the page shows a false “Local Bluetooth is disabled” state. Track a load/error state and only render that empty-state message after settings have actually loaded.Also applies to: 210-213, 359-369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` around lines 140 - 150, The fetchBleSettings flow in BLEManager currently leaves bleSettings as null on load failure, which makes the derived localBLESupported/localManaged defaults render a false “Local Bluetooth is disabled” state. Add an explicit load/error state alongside bleSettings, set it in fetchBleSettings and any related settings-loading paths, and update the render logic so the empty-state message only appears after settings have successfully loaded. Apply the same distinction in the other affected BLEManager rendering spots tied to the same state.
217-217: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop these section-marker comments.
They just repeat the following card titles and don't explain anything new. As per path instructions, comments should explain why, not what.
Also applies to: 270-270, 348-348, 417-417, 477-477
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` at line 217, Remove the section-marker comments in BLEManager that only restate the nearby card titles, since they add no value and violate the “explain why, not what” guidance. Delete the standalone marker comments near the relevant JSX blocks in the BLEManager component, keeping the actual card titles and functional code intact; only retain comments that provide non-obvious reasoning or context.Source: Path instructions
packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (1)
49-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid
alert()for save failures here.Use local error state and render a Bootstrap
Alertor toast near the form instead of blocking popups. Based on learnings, this UI avoidsalert()on both success and error paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 49 - 55, The BLE settings save flow in BLESettings should not use blocking alert() calls for failures. Update the save handler around the res.ok check and catch block to set a local error state instead, and render that state with a Bootstrap Alert or toast near the form. Keep the existing save logic in the same function, but replace the alert-based error reporting with the component’s local UI feedback path.Source: Learnings
src/api/ble/asyncApi.ts (1)
18-20: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix the advertisement stream contract.
The runtime pushes every raw
BLEAdvertisementframe to WebSocket clients; only/devicesis deduplicated. Describing this channel as "deduplicated server-side" will mislead consumers about frame cardinality and provider-level visibility.Also applies to: 46-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/asyncApi.ts` around lines 18 - 20, Update the advertisement stream documentation in asyncApi.ts so it no longer claims the WebSocket channel is deduplicated server-side; the stream emits every raw BLEAdvertisement frame from registered providers, while only /devices is deduplicated. Adjust the description near the Advertisement Stream section and the related schema/docs around the referenced BLEAdvertisement channel to clearly state raw frame delivery and provider-level visibility.src/api/ble/index.ts (4)
82-82: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Remove the default-provider feature or update the approved scope.
Issue
#2411says this API should support multiple providers without a default provider, but this adds persisted default-provider state, public get/set endpoints, and selection bias inselectGATTProvider(). That changes the failover model the PR was supposed to implement.Also applies to: 571-596, 662-687
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/index.ts` at line 82, The BLE API is introducing a default-provider concept that conflicts with the approved multi-provider/no-default scope. Remove the persisted default-provider state and the public get/set default-provider endpoints from the BLE API, and update `selectGATTProvider()` so it no longer prefers a stored default provider when choosing a provider. Keep the provider selection and failover logic aligned with the existing multi-provider model in `BLEApi` and related default-provider methods.
356-389: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prune stale device state before direct lookups and GATT selection.
pruneStaleDevices()only runs ingetDevices()./devices/:macandselectGATTProvider()can therefore operate on entries older thanDEVICE_STALE_MS, which exposes expired devices and can route a new claim through a provider that has not seen the device recently.Also applies to: 571-599
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/index.ts` around lines 356 - 389, `BLEService.getDevices()` already prunes stale entries, but `getDevice()` and `selectGATTProvider()` can still read or act on expired state. Add a `pruneStaleDevices()` call at the start of `getDevice()` and inside `selectGATTProvider()` before checking `deviceTable` or choosing a provider, so direct lookups and GATT claim routing only use devices seen within `DEVICE_STALE_MS`.
406-464: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Auto-release subscription claims on unexpected disconnect.
subscribeGATT()only clearsgattClaimsandkeepAliveTimerinside the wrappedclose(). If the provider drops the subscription and fireshandle.onDisconnect(), the MAC stays claimed and the keepalive timer keeps the device artificially fresh until the consumer closes it manually.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/index.ts` around lines 406 - 464, subscribeGATT() only releases the claim in the wrapped handle.close(), so an unexpected provider disconnect leaves the MAC stuck in gattClaims and the keepAliveTimer running. Update the subscribeGATT flow to also hook handle.onDisconnect() (or the provider disconnect path) in the same place where handle.close is wrapped, and make it share the same cleanup logic that clears the interval, removes the gattClaims entry, and logs the release for the mac/pluginId pair.
788-823: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mutate
this.settingsbefore persistence succeeds.A failed
writeSettingsFile()returns 500 after the in-memory settings have already changed. That leaves/settings, provider behavior, and the saved config out of sync until another write or restart fixes it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/index.ts` around lines 788 - 823, The BLE settings update flow in the handler that mutates `this.settings` and calls `writeSettingsFile()` is applying changes in memory before persistence succeeds, which can leave `/settings` and provider state out of sync on failure. Update the settings write path so the incoming `body.localBluetoothManaged`, `body.localAdapters`, and `body.localMaxGATTSlots` values are staged separately, persist first, and only assign them to `this.settings` and `this.app.config.settings.bleApi` after `writeSettingsFile()` succeeds.src/api/ble/remoteProvider.ts (1)
659-662: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
HTTP-only gateway expiry still depends on a later POST.
_pruneStaleHttpGateways()only runs from_handleAdvertisementPost(). If the last HTTP-only gateway disappears and no more POSTs arrive, its provider stays registered indefinitely, so/_providerscan keep exposing a gateway that/gatewaysalready ages out.Also applies to: 904-923, 967-990
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/remoteProvider.ts` around lines 659 - 662, The HTTP-only gateway cleanup is only triggered from _handleAdvertisementPost(), so stale providers can remain registered if no later POST arrives. Update RemoteProvider to also prune expired HTTP-only gateways from the provider registry in the same lifecycle used by _pruneStaleHttpGateways(), and ensure the registration removal logic stays consistent with the gateway aging behavior in /gateways and /_providers. Use the existing symbols _pruneStaleHttpGateways, _handleAdvertisementPost, and the provider registration path in RemoteProvider to wire this cleanup into a place that runs even without new POSTs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (1)
123-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill using magic numbers for GATT slot bounds.
1/10remain inline in both the input'smin/maxand the clamp logic. As per coding guidelines, "No magic numbers; use named constants" — extractMIN_GATT_SLOTS/MAX_GATT_SLOTSmodule-level constants and reuse them in both places.Also applies to: 133-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 123 - 124, The GATT slot bounds are still hardcoded as magic numbers in the BLE settings view. In BLESettings.tsx, extract the inline `1` and `10` values into module-level constants such as `MIN_GATT_SLOTS` and `MAX_GATT_SLOTS`, then reuse those constants both on the input `min`/`max` props and in the clamp logic inside the relevant handler in the same component.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/plugins/ble_provider_plugins.md`:
- Around line 44-78: The BLE provider plugin example uses gattHandle in
plugin.start and plugin.stop without declaring it first, which will fail in
strict/module usage. Add an explicit gattHandle declaration in the sample before
it is assigned or checked, and keep the subscribeGATT, onDisconnect, and close
flow unchanged so the snippet is copy-paste safe.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 40-47: The local BLE settings interface in BLEManager.tsx is
duplicating the `/settings` payload shape instead of using the shared API
schemas. Update the BLESettings type to be derived from the exported
BLESettingsResponseSchema and BLESettingsRequestSchema in `@signalk/server-api`,
and apply the same shared-type approach anywhere BLESettings.tsx is hand-rolling
the payload so the two views stay aligned with the source schema.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 36-59: `handleSave` in `BLESettings` still uses blocking `alert()`
calls for both failed responses and caught errors. Replace those notifications
with persistent non-blocking feedback by adding state for a save error/message
and rendering it as an inline `Alert` or banner in the `BLESettings` card, while
keeping the success/error handling inside `handleSave` and removing all
`alert()` usage.
In `@src/api/ble/index.ts`:
- Line 611: The mutating-method check in the BLE request handler is recreating
an array and using Array.includes on every call. Update the BLE API module to
define a module-level Set for the methods used by the request guard, and have
the request path check membership against that Set in the handler near the
method filter logic. This should be located around the req.method check in the
BLE API entrypoint.
In `@src/api/ble/localProvider.ts`:
- Around line 352-358: Guard the interpolated debug logging in
emitDeviceAdvertisement so the message is only built when debugging is enabled;
this hot path currently formats `Advertisement callback error: ${e.message}` for
every advertisement even when `debug` is off. Update the callback error handling
inside `localProvider.ts` to use the same `debug.enabled && debug(...)` pattern
already used in `index.ts`, referencing `emitDeviceAdvertisement` and the
`advCallbacks` loop so the string interpolation is skipped unless debug logging
is active.
In `@src/api/ble/remoteProvider.ts`:
- Around line 408-416: The close() handler is decrementing activeSlots even when
the session was already removed by gatt_disconnected or gatt_error, causing a
double-release. Update the close method in remoteProvider.ts to only decrement
activeSlots when this.sessions.delete(sessionId) actually removes an existing
session, and keep the WebSocket gatt_close send behavior unchanged. Use the
close callback and the this.sessions / this.activeSlots state to gate the
cleanup.
In `@src/interfaces/plugins.ts`:
- Around line 704-706: The `debug()` call in the `setDefaultMetadata` flow
eagerly builds the interpolated message even when debugging is off. Guard the
logging in `src/interfaces/plugins.ts` by checking `debug.enabled` before
constructing the template literal, so `skPath` and `validationError` are only
evaluated when the logger will emit. Use the existing `debug` symbol in the
surrounding `setDefaultMetadata` logic to keep the change localized.
- Around line 668-729: Validate skPath before it reaches the metadata writes in
setDefaultMetadata: this plugin-facing entrypoint currently forwards skPath
directly into baseDeltaEditor.getMeta/setMeta and getMetadata, so add a guard at
the start of the function to reject empty or malformed paths before any
computeDefaultFields, writeBaseDeltasFile, or handleMessage work runs. Keep the
check close to setDefaultMetadata in src/interfaces/plugins.ts and return false
(with a debug/error log if appropriate) when skPath is invalid, so plugins
cannot write under unintended keys or corrupt baseDeltas.json.
- Around line 828-833: The shared BLEApi exposure in the plugin proxy is too
broad because it lets plugins access provider registration and raw GATT
operations across IDs. Update the proxy setup around appCopy.registerBLEProvider
and appCopy.bleApi so only registerBLEProvider remains on the proxy for
providers, and replace appCopy.bleApi with a narrower consumer-only BLE
interface that excludes register, unRegister, and raw GATT helpers. Use the
existing BLEApi and BLEProvider symbols to locate the split and ensure plugins
can only use their intended BLE capabilities.
In `@src/serverroutes.ts`:
- Around line 1141-1147: Avoid concurrent writes to the shared bleApi settings
subtree: the generic settings update path in serverroutes.ts and the BLE API
persistence path in src/api/ble/index.ts are both modifying the same data
independently, so overlapping requests can overwrite each other. Refactor the
settings flow so only one code path writes bleApi, or serialize/queue updates
through a single shared writer, and ensure the logic around updatedSettings and
the BLE settings mutation in the BLE route both go through that common updater.
---
Duplicate comments:
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 123-124: The GATT slot bounds are still hardcoded as magic numbers
in the BLE settings view. In BLESettings.tsx, extract the inline `1` and `10`
values into module-level constants such as `MIN_GATT_SLOTS` and
`MAX_GATT_SLOTS`, then reuse those constants both on the input `min`/`max` props
and in the clamp logic inside the relevant handler in the same component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7d79b34-55ce-474b-b3f5-9492a240d9f4
📒 Files selected for processing (29)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/asyncApi.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
♻️ Duplicate comments (1)
packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (1)
123-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill using magic numbers for GATT slot bounds.
1/10remain inline in both the input'smin/maxand the clamp logic. As per coding guidelines, "No magic numbers; use named constants" — extractMIN_GATT_SLOTS/MAX_GATT_SLOTSmodule-level constants and reuse them in both places.Also applies to: 133-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 123 - 124, The GATT slot bounds are still hardcoded as magic numbers in the BLE settings view. In BLESettings.tsx, extract the inline `1` and `10` values into module-level constants such as `MIN_GATT_SLOTS` and `MAX_GATT_SLOTS`, then reuse those constants both on the input `min`/`max` props and in the clamp logic inside the relevant handler in the same component.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/plugins/ble_provider_plugins.md`:
- Around line 44-78: The BLE provider plugin example uses gattHandle in
plugin.start and plugin.stop without declaring it first, which will fail in
strict/module usage. Add an explicit gattHandle declaration in the sample before
it is assigned or checked, and keep the subscribeGATT, onDisconnect, and close
flow unchanged so the snippet is copy-paste safe.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Around line 40-47: The local BLE settings interface in BLEManager.tsx is
duplicating the `/settings` payload shape instead of using the shared API
schemas. Update the BLESettings type to be derived from the exported
BLESettingsResponseSchema and BLESettingsRequestSchema in `@signalk/server-api`,
and apply the same shared-type approach anywhere BLESettings.tsx is hand-rolling
the payload so the two views stay aligned with the source schema.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 36-59: `handleSave` in `BLESettings` still uses blocking `alert()`
calls for both failed responses and caught errors. Replace those notifications
with persistent non-blocking feedback by adding state for a save error/message
and rendering it as an inline `Alert` or banner in the `BLESettings` card, while
keeping the success/error handling inside `handleSave` and removing all
`alert()` usage.
In `@src/api/ble/index.ts`:
- Line 611: The mutating-method check in the BLE request handler is recreating
an array and using Array.includes on every call. Update the BLE API module to
define a module-level Set for the methods used by the request guard, and have
the request path check membership against that Set in the handler near the
method filter logic. This should be located around the req.method check in the
BLE API entrypoint.
In `@src/api/ble/localProvider.ts`:
- Around line 352-358: Guard the interpolated debug logging in
emitDeviceAdvertisement so the message is only built when debugging is enabled;
this hot path currently formats `Advertisement callback error: ${e.message}` for
every advertisement even when `debug` is off. Update the callback error handling
inside `localProvider.ts` to use the same `debug.enabled && debug(...)` pattern
already used in `index.ts`, referencing `emitDeviceAdvertisement` and the
`advCallbacks` loop so the string interpolation is skipped unless debug logging
is active.
In `@src/api/ble/remoteProvider.ts`:
- Around line 408-416: The close() handler is decrementing activeSlots even when
the session was already removed by gatt_disconnected or gatt_error, causing a
double-release. Update the close method in remoteProvider.ts to only decrement
activeSlots when this.sessions.delete(sessionId) actually removes an existing
session, and keep the WebSocket gatt_close send behavior unchanged. Use the
close callback and the this.sessions / this.activeSlots state to gate the
cleanup.
In `@src/interfaces/plugins.ts`:
- Around line 704-706: The `debug()` call in the `setDefaultMetadata` flow
eagerly builds the interpolated message even when debugging is off. Guard the
logging in `src/interfaces/plugins.ts` by checking `debug.enabled` before
constructing the template literal, so `skPath` and `validationError` are only
evaluated when the logger will emit. Use the existing `debug` symbol in the
surrounding `setDefaultMetadata` logic to keep the change localized.
- Around line 668-729: Validate skPath before it reaches the metadata writes in
setDefaultMetadata: this plugin-facing entrypoint currently forwards skPath
directly into baseDeltaEditor.getMeta/setMeta and getMetadata, so add a guard at
the start of the function to reject empty or malformed paths before any
computeDefaultFields, writeBaseDeltasFile, or handleMessage work runs. Keep the
check close to setDefaultMetadata in src/interfaces/plugins.ts and return false
(with a debug/error log if appropriate) when skPath is invalid, so plugins
cannot write under unintended keys or corrupt baseDeltas.json.
- Around line 828-833: The shared BLEApi exposure in the plugin proxy is too
broad because it lets plugins access provider registration and raw GATT
operations across IDs. Update the proxy setup around appCopy.registerBLEProvider
and appCopy.bleApi so only registerBLEProvider remains on the proxy for
providers, and replace appCopy.bleApi with a narrower consumer-only BLE
interface that excludes register, unRegister, and raw GATT helpers. Use the
existing BLEApi and BLEProvider symbols to locate the split and ensure plugins
can only use their intended BLE capabilities.
In `@src/serverroutes.ts`:
- Around line 1141-1147: Avoid concurrent writes to the shared bleApi settings
subtree: the generic settings update path in serverroutes.ts and the BLE API
persistence path in src/api/ble/index.ts are both modifying the same data
independently, so overlapping requests can overwrite each other. Refactor the
settings flow so only one code path writes bleApi, or serialize/queue updates
through a single shared writer, and ensure the logic around updatedSettings and
the BLE settings mutation in the BLE route both go through that common updater.
---
Duplicate comments:
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 123-124: The GATT slot bounds are still hardcoded as magic numbers
in the BLE settings view. In BLESettings.tsx, extract the inline `1` and `10`
values into module-level constants such as `MIN_GATT_SLOTS` and
`MAX_GATT_SLOTS`, then reuse those constants both on the input `min`/`max` props
and in the clamp logic inside the relevant handler in the same component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7d79b34-55ce-474b-b3f5-9492a240d9f4
📒 Files selected for processing (29)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/asyncApi.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
🛑 Comments failed to post (10)
docs/develop/plugins/ble_provider_plugins.md (1)
44-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Declare
gattHandlebefore assigning to it.The sample uses
gattHandle = ...andif (gattHandle)without declaring the variable, so the snippet throws in strict/module mode if copied verbatim.🔧 Proposed fix
module.exports = function (app) { const plugin = { id: 'my-ble-plugin', name: 'My BLE Plugin' } let unsubscribe = null + let gattHandle = null plugin.start = async function () {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.module.exports = function (app) { const plugin = { id: 'my-ble-plugin', name: 'My BLE Plugin' } let unsubscribe = null let gattHandle = null plugin.start = async function () {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/develop/plugins/ble_provider_plugins.md` around lines 44 - 78, The BLE provider plugin example uses gattHandle in plugin.start and plugin.stop without declaring it first, which will fail in strict/module usage. Add an explicit gattHandle declaration in the sample before it is assigned or checked, and keep the subscribeGATT, onDisconnect, and close flow unchanged so the snippet is copy-paste safe.packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx (1)
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check whether server-admin-ui already depends on server-api's typebox exports for reuse. rg -n "server-api" packages/server-admin-ui/package.json rg -n "BLESettings|ble-schemas" packages/server-api/src/typebox/index.tsRepository: SignalK/signalk-server
Length of output: 193
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== packages/server-admin-ui/package.json ==\n' cat -n packages/server-admin-ui/package.json printf '\n== packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx ==\n' sed -n '1,140p' packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx | cat -n printf '\n== packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx ==\n' sed -n '1,220p' packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx | cat -n printf '\n== packages/server-api/src/typebox/ble-schemas.ts ==\n' sed -n '1,240p' packages/server-api/src/typebox/ble-schemas.ts | cat -n printf '\n== packages/server-api/src/typebox/index.ts ==\n' sed -n '1,120p' packages/server-api/src/typebox/index.ts | cat -nRepository: SignalK/signalk-server
Length of output: 26072
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== workspace/package dependency hints ==\n' rg -n '"workspaces"|"packages/"|"`@signalk/server-api`"|from '\''`@signalk/server-api`'\''' package.json packages/server-admin-ui/package.json packages/server-api/package.json printf '\n== BLE settings schema/type exports in server-api ==\n' rg -n "BLESettings|settings.*BLE|localBluetoothManaged|localMaxGATTSlots|localBLESupported|adapterErrors|activeAdapters" packages/server-api/src/typebox printf '\n== all server-admin-ui imports from server-api or typebox shared packages ==\n' rg -n "from ['\"][^'\"]*server-api|from ['\"][^'\"]*typebox|from ['\"][^'\"]*ble-schemas" packages/server-admin-ui/src printf '\n== BLESettings.tsx references ==\n' rg -n "BLESettingsData|localBluetoothManaged|localMaxGATTSlots|localBLESupported" packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxRepository: SignalK/signalk-server
Length of output: 1588
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== packages/server-api/src/typebox/ble-schemas.ts (BLE settings block) ==\n' sed -n '290,340p' packages/server-api/src/typebox/ble-schemas.ts | cat -n printf '\n== Static type exports in server-api ble schemas ==\n' rg -n "export type .*Static<typeof .*BLESettings|export type .*Static<typeof .*Schema>|type .*Static<typeof" packages/server-api/src/typebox/ble-schemas.ts printf '\n== UI imports from `@signalk/server-api` anywhere in server-admin-ui ==\n' rg -n "from '`@signalk/server-api`'|from \"`@signalk/server-api`\"" packages/server-admin-ui/src packages/server-admin-uiRepository: SignalK/signalk-server
Length of output: 2656
Share the BLE settings shape
BLEManager.tsxandBLESettings.tsxboth hand-roll the/settingspayload that already exists asBLESettingsResponseSchema/BLESettingsRequestSchemain@signalk/server-api. Derive the local types from the exported TypeBox schemas to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` around lines 40 - 47, The local BLE settings interface in BLEManager.tsx is duplicating the `/settings` payload shape instead of using the shared API schemas. Update the BLESettings type to be derived from the exported BLESettingsResponseSchema and BLESettingsRequestSchema in `@signalk/server-api`, and apply the same shared-type approach anywhere BLESettings.tsx is hand-rolling the payload so the two views stay aligned with the source schema.packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (1)
36-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace
alert()with non-blocking feedback.
handleSavestill usesalert()on both the non-ok response and the catch branch. Based on learnings, this codebase should use persistent, non-blocking feedback (inline banner/toast) instead ofalert()for save/error reporting.💡 Suggested direction
+ const [saveError, setSaveError] = useState<string | null>(null) + const handleSave = useCallback(async () => { if (!settings) return setSaving(true) + setSaveError(null) try { const res = await fetch(`${BLE_API}/settings`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ localBluetoothManaged: settings.localBluetoothManaged, localMaxGATTSlots: settings.localMaxGATTSlots }), credentials: 'include' }) if (!res.ok) { const err = await res.json().catch(() => ({ message: res.statusText })) - alert(err.message || 'Failed to save BLE settings') + setSaveError(err.message || 'Failed to save BLE settings') } } catch (err: unknown) { const message = err instanceof Error ? err.message : String(err) - alert(`Failed to save BLE settings: ${message}`) + setSaveError(`Failed to save BLE settings: ${message}`) } finally { setSaving(false) } }, [settings])Render
saveErroras an inlineAlert/banner in the card body/footer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 36 - 59, `handleSave` in `BLESettings` still uses blocking `alert()` calls for both failed responses and caught errors. Replace those notifications with persistent non-blocking feedback by adding state for a save error/message and rendering it as an inline `Alert` or banner in the `BLESettings` card, while keeping the success/error handling inside `handleSave` and removing all `alert()` usage.Source: Learnings
src/api/ble/index.ts (1)
611-611: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Use a module-level
Setfor the mutating-method check.This membership check runs on every BLE API request and re-allocates the array each call.
♻️ Suggested change
+const MUTATING_METHODS = new Set(['PUT', 'POST', 'DELETE']) + // ... inside middleware: - if (['PUT', 'POST', 'DELETE'].includes(req.method)) { + if (MUTATING_METHODS.has(req.method)) {As per coding guidelines: "Prefer
SetoverArray.includesfor repeated membership checks".📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const MUTATING_METHODS = new Set(['PUT', 'POST', 'DELETE']) if (MUTATING_METHODS.has(req.method)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/index.ts` at line 611, The mutating-method check in the BLE request handler is recreating an array and using Array.includes on every call. Update the BLE API module to define a module-level Set for the methods used by the request guard, and have the request path check membership against that Set in the handler near the method filter logic. This should be located around the req.method check in the BLE API entrypoint.Source: Coding guidelines
src/api/ble/localProvider.ts (1)
352-358: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Guard the interpolated
debug()in this per-advertisement hot path.
emitDeviceAdvertisementruns for every advertisement from every device; the message string is built even when debug is disabled. The equivalent call inindex.tsis already guarded.🔧 Suggested change
} catch (e: any) { - debug(`Advertisement callback error: ${e.message}`) + debug.enabled && debug(`Advertisement callback error: ${e.message}`) }Based on learnings: prefer
debug.enabled && debug(<expression>)when thedebug()argument involves interpolation, so the expression is only evaluated when debugging is enabled.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for (const cb of this.advCallbacks) { try { cb(adv) } catch (e: any) { debug.enabled && debug(`Advertisement callback error: ${e.message}`) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/localProvider.ts` around lines 352 - 358, Guard the interpolated debug logging in emitDeviceAdvertisement so the message is only built when debugging is enabled; this hot path currently formats `Advertisement callback error: ${e.message}` for every advertisement even when `debug` is off. Update the callback error handling inside `localProvider.ts` to use the same `debug.enabled && debug(...)` pattern already used in `index.ts`, referencing `emitDeviceAdvertisement` and the `advCallbacks` loop so the string interpolation is skipped unless debug logging is active.Source: Learnings
src/api/ble/remoteProvider.ts (1)
408-416: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail file="src/api/ble/remoteProvider.ts" # Show the relevant sections around the close handler and the debug call nl -ba "$file" | sed -n '360,470p' printf '\n----\n' nl -ba "$file" | sed -n '630,680p' # Search for all activeSlots mutations and session deletions printf '\n==== activeSlots/session references ====\n' rg -n "activeSlots|sessions\.delete|gatt_disconnected|gatt_error|availableGATTSlots|debug\(" "$file"Repository: SignalK/signalk-server
Length of output: 200
🏁 Script executed:
#!/bin/bash set -euo pipefail file="src/api/ble/remoteProvider.ts" echo '--- close() / session handlers ---' awk 'NR>=360 && NR<=470 { printf "%6d %s\n", NR, $0 }' "$file" echo echo '--- debug section ---' awk 'NR>=630 && NR<=680 { printf "%6d %s\n", NR, $0 }' "$file" echo echo '--- references ---' rg -n "activeSlots|sessions\.delete|gatt_disconnected|gatt_error|availableGATTSlots|debug\(" "$file"Repository: SignalK/signalk-server
Length of output: 7965
Prevent
close()from releasing an already-cleared session twice.gatt_disconnected/gatt_erroralready delete the session and decrementactiveSlots, so a laterhandle.close()drops the count again and overstates free GATT slots. Guard the decrement onthis.sessions.delete(sessionId).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/ble/remoteProvider.ts` around lines 408 - 416, The close() handler is decrementing activeSlots even when the session was already removed by gatt_disconnected or gatt_error, causing a double-release. Update the close method in remoteProvider.ts to only decrement activeSlots when this.sessions.delete(sessionId) actually removes an existing session, and keep the WebSocket gatt_close send behavior unchanged. Use the close callback and the this.sessions / this.activeSlots state to gate the cleanup.src/interfaces/plugins.ts (3)
668-729: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate
skPathat this plugin-facing boundary.
setDefaultMetadataacceptsskPath/valuedirectly from plugin code and passesskPathstraight intobaseDeltaEditor.getMeta/setMetaandgetMetadatawith no check that it's a non-empty, well-formed path. A buggy plugin passing an empty or malformed path could silently corruptbaseDeltas.json(viawriteBaseDeltasFile) or write meta under an unintended key.🛡️ Proposed fix
setDefaultMetadata: async ( skPath: string, value: MetaValue ): Promise<boolean> => { + if (!skPath || typeof skPath !== 'string') { + throw new Error('setDefaultMetadata: skPath must be a non-empty string') + } const context = 'vessels.self'As per coding guidelines,
src/interfaces/**/*.{ts,js}: "Validate external inputs at system boundaries."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.setDefaultMetadata: async ( skPath: string, value: MetaValue ): Promise<boolean> => { if (!skPath || typeof skPath !== 'string') { throw new Error('setDefaultMetadata: skPath must be a non-empty string') } const context = 'vessels.self' const existingMeta = app.config.baseDeltaEditor.getMeta( context, skPath ) as Record<string, unknown> | undefined const { hasNewFields, fieldsToSet, merged } = DeltaEditor.computeDefaultFields( existingMeta, value as unknown as Record<string, unknown> ) if (!hasNewFields) { return false } const displayUnits = fieldsToSet.displayUnits as { category?: string } | undefined if (displayUnits?.category) { const schemaMeta = getMetadata('vessels.self.' + skPath) as Record< string, unknown > | null const pathSiUnit = (fieldsToSet.units as string | undefined) ?? (existingMeta?.units as string | undefined) ?? (schemaMeta?.units as string | undefined) const validationError = validateCategoryAssignment( pathSiUnit, displayUnits.category ) if (validationError) { debug( `setDefaultMetadata: invalid category for ${skPath}: ${validationError}` ) return false } } app.config.baseDeltaEditor.setMeta(context, skPath, merged) await writeBaseDeltasFile(app as unknown as ConfigApp) app.handleMessage(plugin.id, { context: 'vessels.self' as Context, updates: [ { meta: [ { path: skPath as Path, value: merged } ] } ] }) return true },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interfaces/plugins.ts` around lines 668 - 729, Validate skPath before it reaches the metadata writes in setDefaultMetadata: this plugin-facing entrypoint currently forwards skPath directly into baseDeltaEditor.getMeta/setMeta and getMetadata, so add a guard at the start of the function to reject empty or malformed paths before any computeDefaultFields, writeBaseDeltasFile, or handleMessage work runs. Keep the check close to setDefaultMetadata in src/interfaces/plugins.ts and return false (with a debug/error log if appropriate) when skPath is invalid, so plugins cannot write under unintended keys or corrupt baseDeltas.json.Source: Coding guidelines
704-706: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Guard the interpolated
debug()call.
skPath/validationErrorare interpolated into the template literal unconditionally, so the string is built even when debug is disabled.🔧 Proposed fix
- debug( - `setDefaultMetadata: invalid category for ${skPath}: ${validationError}` - ) + debug.enabled && + debug( + `setDefaultMetadata: invalid category for ${skPath}: ${validationError}` + )Based on learnings, "guard against eager evaluation: if the
debug()argument involves interpolation or non-trivial expressions... preferdebug.enabled && debug(<expression>)."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.debug.enabled && debug( `setDefaultMetadata: invalid category for ${skPath}: ${validationError}` )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interfaces/plugins.ts` around lines 704 - 706, The `debug()` call in the `setDefaultMetadata` flow eagerly builds the interpolated message even when debugging is off. Guard the logging in `src/interfaces/plugins.ts` by checking `debug.enabled` before constructing the template literal, so `skPath` and `validationError` are only evaluated when the logger will emit. Use the existing `debug` symbol in the surrounding `setDefaultMetadata` logic to keep the change localized.Source: Learnings
828-833: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect the ServerAPI.bleApi contract to see if a narrower interface is expected rg -n 'bleApi' packages/server-api/src/serverapi.ts rg -n 'IBLEApi|interface BLEApi' packages/server-api/src/bleapi.ts src/api/ble/index.tsRepository: SignalK/signalk-server
Length of output: 334
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- packages/server-api/src/bleapi.ts (around interface) ---' sed -n '180,280p' packages/server-api/src/bleapi.ts | cat -n echo echo '--- src/interfaces/plugins.ts (around BLE lines) ---' sed -n '760,860p' src/interfaces/plugins.ts | cat -n echo echo '--- nearby BLE API implementation exports ---' sed -n '1,140p' src/api/ble/index.ts | cat -nRepository: SignalK/signalk-server
Length of output: 14763
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 3 'interface BLEApi|register\(|unRegister\(|read|scan|connect|discov' packages/server-api/src/bleapi.ts src/api/ble/index.ts src/interfaces/plugins.tsRepository: SignalK/signalk-server
Length of output: 24985
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n 'appCopy\.(weatherApi|autopilotApi|resourcesApi|historyApiHttpRegistry|bleApi|register[A-Z].*Provider|Api = )' src/interfaces/plugins.tsRepository: SignalK/signalk-server
Length of output: 593
Split the BLE consumer API from the provider registry.
src/interfaces/plugins.ts:828-833exposes the sharedBLEApiobject, which includesregister/unRegisterand raw GATT helpers, so any plugin can act on another plugin’s ID. KeepregisterBLEProvideron the proxy and expose a narrower consumer-only BLE interface instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interfaces/plugins.ts` around lines 828 - 833, The shared BLEApi exposure in the plugin proxy is too broad because it lets plugins access provider registration and raw GATT operations across IDs. Update the proxy setup around appCopy.registerBLEProvider and appCopy.bleApi so only registerBLEProvider remains on the proxy for providers, and replace appCopy.bleApi with a narrower consumer-only BLE interface that excludes register, unRegister, and raw GATT helpers. Use the existing BLEApi and BLEProvider symbols to locate the split and ensure plugins can only use their intended BLE capabilities.src/serverroutes.ts (1)
1141-1147: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect the full BLE settings PUT persistence path for interaction with the generic settings writer rg -n -A20 "this.app.put" src/api/ble/index.ts | rg -n -A20 "'/settings'" rg -n "writeSettingsFile" src/api/ble/index.ts src/serverroutes.tsRepository: SignalK/signalk-server
Length of output: 1295
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- src/api/ble/index.ts around PUT /settings ---' sed -n '760,860p' src/api/ble/index.ts echo echo '--- src/serverroutes.ts around generic bleApi update ---' sed -n '1120,1195p' src/serverroutes.tsRepository: SignalK/signalk-server
Length of output: 5941
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path for path, start, end in [ ("src/api/ble/index.ts", 780, 840), ("src/serverroutes.ts", 1130, 1188), ]: print(f"--- {path}:{start}-{end} ---") lines = Path(path).read_text().splitlines() for i in range(start, min(end, len(lines)) + 1): print(f"{i}: {lines[i-1]}") print() PYRepository: SignalK/signalk-server
Length of output: 4693
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -A80 -B20 "function writeSettingsFile|const writeSettingsFile|export .*writeSettingsFile" src/config/config.ts src/config -g '!**/*.map'Repository: SignalK/signalk-server
Length of output: 11603
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline src/config/config.ts --view expandedRepository: SignalK/signalk-server
Length of output: 2297
Avoid concurrent writes to
bleApi. The generic settings flow clonesapp.config.settingsand later replaces the root object, whilesrc/api/ble/index.tsmutates and persists the same subtree independently. Overlapping requests can drop one side’sbleApiupdate; route both through a single writer or serialize settings updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serverroutes.ts` around lines 1141 - 1147, Avoid concurrent writes to the shared bleApi settings subtree: the generic settings update path in serverroutes.ts and the BLE API persistence path in src/api/ble/index.ts are both modifying the same data independently, so overlapping requests can overwrite each other. Refactor the settings flow so only one code path writes bleApi, or serialize/queue updates through a single shared writer, and ensure the logic around updatedSettings and the BLE settings mutation in the BLE route both go through that common updater.
dirkwa
force-pushed
the
ble-provider-api
branch
2 times, most recently
from
c728e02 to
cde7e05
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (1)
packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx (1)
123-133: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract GATT slot bounds into named constants.
Inline
1and10appear in both the JSXmin/maxprops and theMath.max/Math.minclamping logic. As per coding guidelines, "No magic numbers; use named constants." This was previously flagged and remains unaddressed.♻️ Proposed refactor
const BLE_API = '/signalk/v2/api/vessels/self/ble' +const MIN_GATT_SLOTS = 1 +const MAX_GATT_SLOTS = 10<Form.Control style={{ width: 'auto' }} type="number" id="localMaxGATTSlots" name="localMaxGATTSlots" - min={1} - max={10} + min={MIN_GATT_SLOTS} + max={MAX_GATT_SLOTS} value={settings.localMaxGATTSlots} onChange={(e) => setSettings((prev) => { if (!prev) return prev const val = parseInt(e.target.value) if (isNaN(val)) return prev return { ...prev, - localMaxGATTSlots: Math.max(1, Math.min(10, val)) + localMaxGATTSlots: Math.max(MIN_GATT_SLOTS, Math.min(MAX_GATT_SLOTS, val)) } }) } />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx` around lines 123 - 133, Define named constants for the minimum and maximum GATT slot values, then use them in the input’s min/max props and the clamping logic within the settings onChange handler. Update the relevant BLESettings JSX and state-update code to eliminate the duplicated magic numbers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/plugins/ble_provider_plugins.md`:
- Around line 98-106: Remove the branch-narrating comments inside plugin.start,
including the comments before startBleApiMode and startDirectBlueZMode, while
preserving the existing conditional behavior.
- Around line 193-195: Update the “Remote Gateway Protocol” documentation link
to reference the actual AsyncAPI UI/spec rather than the REST API page’s
`#schemas` section, while retaining the OpenAPI reference for HTTP contracts.
In `@docs/develop/rest-api/ble_api.md`:
- Line 28: Update the Bluetooth configuration instructions to reference Server
Settings → Bluetooth instead of Data → BLE Manager, while preserving the
existing localBluetoothManaged and localAdapters details.
- Line 41: Update the BLE API documentation link in the paragraph describing the
protocol so it points directly to the actual AsyncAPI specification rather than
the local `#schemas` section; preserve the existing SensESP reference and
surrounding text.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx`:
- Line 217: Remove the redundant section comments, including the “Status
overview” comment and the corresponding comments at the other referenced
locations in the BLEManager component, since each duplicates the following
card’s visible heading.
- Around line 153-198: Define named duration constants near the component/module
constants for the 5-second device polling and WebSocket reconnect cadence, plus
the 1-second advertisement counter refresh cadence. Replace the corresponding
5000 and 1000 literals in the polling effect, reconnect setTimeout, and
countInterval with those constants.
- Around line 62-88: Extract the inline time cutoffs in formatAge,
formatDuration, and any related display logic, plus the RSSI cutoffs in
rssiColor, into clearly named module-level constants. Update those functions to
reference the constants while preserving the existing thresholds and output
behavior.
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 49-55: Replace both alert() calls in the BLE settings save handler
with the UI’s existing non-blocking feedback mechanism, such as a toast or
inline error banner. Preserve the response error message and caught exception
details, and ensure the feedback state is rendered persistently without blocking
the user.
In `@packages/server-api/src/typebox/ble-schemas.ts`:
- Around line 231-261: Export Static<typeof ...Schema> type aliases for
BLEGatewayInfoSchema, BLESettingsResponseSchema, BLESettingsRequestSchema,
BLEProviderInfoSchema, BLEDefaultProviderSchema, and BLEGattClaimStatusSchema
alongside the existing static exports. In remoteProvider.ts, replace the
duplicated local GatewayInfo interface with the imported BLEGatewayInfo type and
update all references accordingly.
In `@src/api/ble/index.ts`:
- Around line 356-400: deviceTable stale entries are only removed during
getDevices(), allowing unbounded growth when REST polling is inactive. Start a
periodic cleanup interval in start() that invokes pruneStaleDevices() at
DEVICE_STALE_MS, and store or clear the interval during shutdown to avoid leaks;
ensure cleanup remains independent of REST and WebSocket traffic.
In `@src/api/ble/remoteProvider.ts`:
- Around line 230-239: Validate WebSocket control-message fields at runtime
before assigning them in handleHello and the status-message handler. Ensure
max_gatt_connections and active_gatt_connections are finite numbers, and
firmware, mac, and hostname are strings when present; otherwise use the existing
safe defaults or reject/log the malformed message. Remove the unchecked as
number/as string casts and mirror the validation approach used by
_handleAdvertisementPost.
- Around line 481-486: Replace the any-typed securityStrategy property in the
RemoteGatewayApp interface with the shared SecurityStrategy type imported from
src/security.ts, and remove the now-unnecessary eslint-disable comment.
- Around line 904-923: Make stale HTTP-only gateway cleanup run independently of
POST traffic. Add a periodic timer or equivalent scheduled cleanup that invokes
_pruneStaleHttpGateways, initialize it with the provider lifecycle, and clear it
during teardown so _unregisterGatewayProvider and related state cleanup occur
after inactivity without requiring another advertisement POST.
- Around line 408-419: Update the session handle’s close implementation to set
session.connected to false before deleting the session, ensuring connected
reports false after explicit closure. In the initial gatt_subscribe send logic,
guard ws.send with the same WebSocket.OPEN readyState check used by write() to
avoid sending on a closed socket.
In `@src/interfaces/plugins.ts`:
- Around line 882-887: Remove the redundant `appCopy.bleApi = bleApi` assignment
in the plugin app construction within the function containing
`registerBLEProvider`. Before exposing the app copy, delete the inherited raw
`bleApi` property so plugins cannot access unscoped methods such as
`unRegister`; retain only the scoped `registerBLEProvider` wrapper that
registers using `plugin.id`, following the existing `historyApiHttpRegistry` and
`notificationApi` patterns.
---
Duplicate comments:
In `@packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsx`:
- Around line 123-133: Define named constants for the minimum and maximum GATT
slot values, then use them in the input’s min/max props and the clamping logic
within the settings onChange handler. Update the relevant BLESettings JSX and
state-update code to eliminate the duplicated magic numbers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1023287-e36a-4bab-b624-79617df24337
📒 Files selected for processing (29)
docs/develop/plugins/README.mddocs/develop/plugins/ble_provider_plugins.mddocs/develop/rest-api/README.mddocs/develop/rest-api/ble_api.mdpackage.jsonpackages/server-admin-ui/src/components/Sidebar/Sidebar.tsxpackages/server-admin-ui/src/containers/Full/Full.tsxpackages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxpackages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxpackages/server-admin-ui/src/views/ServerConfig/Settings.tsxpackages/server-api/src/bleapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/ble-schemas.tspackages/server-api/src/typebox/index.tssrc/api/ble/asyncApi.tssrc/api/ble/bleCompanyIds.jsonsrc/api/ble/bleCompanyIds.tssrc/api/ble/index.tssrc/api/ble/localProvider.tssrc/api/ble/openApi.tssrc/api/ble/remoteProvider.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tssrc/dummysecurity.tssrc/interfaces/plugins.tssrc/serverroutes.ts
Comment on lines
+98
to
+106
| ```javascript | ||
| plugin.start = async function () { | ||
| if (app.bleApi) { | ||
| // Server manages BLE — use the BLE API | ||
| await startBleApiMode() | ||
| } else { | ||
| // Fall back to direct BlueZ access | ||
| await startDirectBlueZMode() | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove the branch-narrating comments.
These comments restate the if branches rather than explaining a non-obvious reason.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/develop/plugins/ble_provider_plugins.md` around lines 98 - 106, Remove
the branch-narrating comments inside plugin.start, including the comments before
startBleApiMode and startDirectBlueZMode, while preserving the existing
conditional behavior.
Sources: Coding guidelines, Path instructions
Comment on lines
+193
to
+195
| ## Remote Gateway Protocol | ||
|
|
||
| Remote BLE gateways feed advertisements via HTTP POST and (optionally) participate in GATT subscribe/notify/write over a bidirectional WebSocket. The exact request bodies, message frames, and connection flow are defined in the [OpenAPI and AsyncAPI specs](../rest-api/ble_api.md) — see them for the canonical wire contract. The reference firmware is [SensESP](https://github.com/dirkwa/SensESP) for ESP32 boards. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the AsyncAPI reference.
Line 195 links to this page’s #schemas section, not an AsyncAPI specification. Point gateway implementers to the actual AsyncAPI UI/spec instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/develop/plugins/ble_provider_plugins.md` around lines 193 - 195, Update
the “Remote Gateway Protocol” documentation link to reference the actual
AsyncAPI UI/spec rather than the REST API page’s `#schemas` section, while
retaining the OpenAPI reference for HTTP contracts.
|
|
||
| ### Local Bluetooth Adapter | ||
|
|
||
| Enable under **Data → BLE Manager → Bluetooth Settings** (`localBluetoothManaged: true`). The server enumerates all available BlueZ adapters and registers each as a separate provider (`_localBLE:hci0`, `_localBLE:hci1`, …). An explicit adapter list can be configured via `localAdapters`. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the Bluetooth settings location.
Line 28 directs users to Data → BLE Manager, but that page is status-only; BLESettings is rendered under Server Settings. Document Server Settings → Bluetooth instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/develop/rest-api/ble_api.md` at line 28, Update the Bluetooth
configuration instructions to reference Server Settings → Bluetooth instead of
Data → BLE Manager, while preserving the existing localBluetoothManaged and
localAdapters details.
|
|
||
| Each connected gateway is registered as `ble:gateway:<hostname>`. The advertisement-batch path is intended for memory-constrained devices that cannot keep a persistent WebSocket open; richer gateways use the WebSocket for full GATT subscribe/notify/write flows. | ||
|
|
||
| The reference implementation is the [SensESP BLE gateway firmware](https://github.com/dirkwa/SensESP) running on ESP32 boards (ESP32-P4 with bundled Bluedroid, ESP32-C5 stand-alone). The protocol is documented in the [AsyncAPI spec](#schemas) so other gateway implementations are straightforward. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Link to the actual AsyncAPI specification.
#schemas resolves to this page’s TypeBox-schema discussion, not the AsyncAPI protocol definition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/develop/rest-api/ble_api.md` at line 41, Update the BLE API
documentation link in the paragraph describing the protocol so it points
directly to the actual AsyncAPI specification rather than the local `#schemas`
section; preserve the existing SensESP reference and surrounding text.
Comment on lines
+62
to
+88
| function formatAge(lastSeen: number): string { | ||
| const seconds = Math.round((Date.now() - lastSeen) / 1000) | ||
| if (seconds < 5) return 'just now' | ||
| if (seconds < 60) return `${seconds}s ago` | ||
| return `${Math.floor(seconds / 60)}m ago` | ||
| } | ||
|
|
||
| function formatDuration(seconds: number): string { | ||
| if (seconds < 60) return `${seconds}s` | ||
| if (seconds < 3600) return `${Math.floor(seconds / 60)}m` | ||
| const h = Math.floor(seconds / 3600) | ||
| const m = Math.floor((seconds % 3600) / 60) | ||
| if (h < 24) return `${h}h ${m}m` | ||
| const d = Math.floor(h / 24) | ||
| return `${d}d ${h % 24}h` | ||
| } | ||
|
|
||
| function formatBytes(bytes: number): string { | ||
| if (bytes < 1024) return `${bytes} B` | ||
| return `${Math.round(bytes / 1024)} KB` | ||
| } | ||
|
|
||
| function rssiColor(rssi: number): string { | ||
| if (rssi >= -50) return 'success' | ||
| if (rssi >= -70) return 'primary' | ||
| if (rssi >= -85) return 'warning' | ||
| return 'danger' |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Name the display-policy thresholds.
The time and RSSI cutoffs encode UI behavior; use named constants rather than inline values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsx` around lines
62 - 88, Extract the inline time cutoffs in formatAge, formatDuration, and any
related display logic, plus the RSSI cutoffs in rssiColor, into clearly named
module-level constants. Update those functions to reference the constants while
preserving the existing thresholds and output behavior.
Source: Coding guidelines
Comment on lines
+230
to
+239
| handleHello(msg: Record<string, unknown>) { | ||
| this.maxSlots = (msg.max_gatt_connections as number) || 0 | ||
| this.activeSlots = (msg.active_gatt_connections as number) || 0 | ||
| this.firmware = (msg.firmware as string) || null | ||
| this.mac = (msg.mac as string) || null | ||
| this.hostname = (msg.hostname as string) || null | ||
| debug( | ||
| `[${this.gatewayId}] hello: ${this.maxSlots} max slots, fw=${this.firmware}, mac=${this.mac}` | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
WS control-message fields are trusted without runtime type checks, unlike the HTTP POST path.
handleHello (and the status case at lines 309-315) cast raw JSON fields with as number/as string, only guarded by || 0/|| null. Unlike _handleAdvertisementPost, which validates the full body against a TypeBox schema, nothing here stops a buggy/malicious gateway from sending e.g. a non-numeric string for max_gatt_connections, silently corrupting maxSlots/activeSlots accounting (arithmetic on a non-numeric string yields NaN), which can leave that gateway stuck reporting broken GATT slot availability.
Also applies to: 309-315.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/ble/remoteProvider.ts` around lines 230 - 239, Validate WebSocket
control-message fields at runtime before assigning them in handleHello and the
status-message handler. Ensure max_gatt_connections and active_gatt_connections
are finite numbers, and firmware, mac, and hostname are strings when present;
otherwise use the existing safe defaults or reject/log the malformed message.
Remove the unchecked as number/as string casts and mirror the validation
approach used by _handleAdvertisementPost.
Comment on lines
+481
to
+486
| interface RemoteGatewayApp extends IRouter { | ||
| server?: import('http').Server | ||
| config?: { settings?: { security?: unknown } } | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| securityStrategy?: any | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'interface SecurityStrategy|type SecurityStrategy' src/ --type tsRepository: SignalK/signalk-server
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== remoteProvider excerpt ==\n'
sed -n '430,520p' src/api/ble/remoteProvider.ts | cat -n
printf '\n== security.ts SecurityStrategy ==\n'
sed -n '130,210p' src/security.ts | cat -n
printf '\n== ws.ts SecurityStrategy ==\n'
sed -n '170,220p' src/interfaces/ws.ts | cat -n
printf '\n== references to securityStrategy in repo ==\n'
rg -n 'securityStrategy' src/ --type tsRepository: SignalK/signalk-server
Length of output: 19141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== remoteProvider securityStrategy usages ==\n'
sed -n '520,770p' src/api/ble/remoteProvider.ts | cat -n
printf '\n== WithSecurityStrategy type ==\n'
sed -n '1,120p' src/types.ts | cat -n
printf '\n== streams app type context ==\n'
sed -n '1,80p' src/api/streams/index.ts | cat -n
printf '\n== security.ts app type context ==\n'
sed -n '1,80p' src/security.ts | cat -n
printf '\n== other imports of SecurityStrategy in api/ble ==\n'
rg -n '\bSecurityStrategy\b' src/api/ble src/types.ts src/security.ts src/interfaces/ws.tsRepository: SignalK/signalk-server
Length of output: 18139
Replace any with the shared SecurityStrategy type. remoteProvider.ts only uses canAuthorizeWS() and authorizeWS(), both already defined on src/security.ts’s SecurityStrategy, so the local eslint-disable isn’t needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/ble/remoteProvider.ts` around lines 481 - 486, Replace the any-typed
securityStrategy property in the RemoteGatewayApp interface with the shared
SecurityStrategy type imported from src/security.ts, and remove the
now-unnecessary eslint-disable comment.
Source: Coding guidelines
Comment on lines
+904
to
+923
| /** | ||
| * Remove state for HTTP-only gateways that haven't POSTed within the | ||
| * snapshot TTL. Called from the POST handler so cleanup happens on | ||
| * the same path that updates `lastPostTime` — keeping `getGatewayInfo` | ||
| * a pure read. | ||
| */ | ||
| private _pruneStaleHttpGateways() { | ||
| for (const [gatewayId] of this.seenMacs) { | ||
| if (this.sessions.has(gatewayId) || this.snapshots.has(gatewayId)) | ||
| continue | ||
| const lastPost = this.lastPostTime.get(gatewayId) ?? 0 | ||
| if (Date.now() - lastPost > OFFLINE_SNAPSHOT_MS) { | ||
| this.seenMacs.delete(gatewayId) | ||
| this.lastPostTime.delete(gatewayId) | ||
| this.httpGatewayMeta.delete(gatewayId) | ||
| this.advCallbacks.delete(gatewayId) | ||
| this._unregisterGatewayProvider(gatewayId) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Locate relevant symbols and file size first
git ls-files 'src/api/ble/remoteProvider.ts'
wc -l src/api/ble/remoteProvider.ts
rg -n "_pruneStaleHttpGateways|lastPostTime|seenMacs|advCallbacks|httpGatewayMeta|_unregisterGatewayProvider|getGatewayInfo|_handleAdvertisementPost" src/api/ble/remoteProvider.ts
# Read the relevant portions around the symbols
sed -n '820,1025p' src/api/ble/remoteProvider.tsRepository: SignalK/signalk-server
Length of output: 8255
🏁 Script executed:
set -euo pipefail
sed -n '536,720p' src/api/ble/remoteProvider.ts
printf '\n----\n'
sed -n '780,830p' src/api/ble/remoteProvider.tsRepository: SignalK/signalk-server
Length of output: 7963
🏁 Script executed:
set -euo pipefail
rg -n "setInterval|setTimeout|_pruneStaleHttpGateways\(" src/api/ble/remoteProvider.tsRepository: SignalK/signalk-server
Length of output: 427
Prune stale HTTP-only gateways without waiting for another POST
_pruneStaleHttpGateways() only runs at the end of _handleAdvertisementPost(), so the last HTTP-only gateway can keep its provider, callbacks, and metadata around indefinitely once traffic stops. A timer-based prune or similar periodic cleanup path would release that state on time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/ble/remoteProvider.ts` around lines 904 - 923, Make stale HTTP-only
gateway cleanup run independently of POST traffic. Add a periodic timer or
equivalent scheduled cleanup that invokes _pruneStaleHttpGateways, initialize it
with the provider lifecycle, and clear it during teardown so
_unregisterGatewayProvider and related state cleanup occur after inactivity
without requiring another advertisement POST.
Comment on lines
+882
to
+887
| const bleApi: BLEApi = app.bleApi | ||
| appCopy.registerBLEProvider = (provider: BLEProvider) => { | ||
| bleApi.register(plugin.id, provider) | ||
| } | ||
| appCopy.bleApi = bleApi | ||
|
|
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Consider not exposing the raw BLEApi instance to plugins.
appCopy.bleApi = bleApi (line 886) is redundant — appCopy already inherits bleApi via _.assign({}, app, {...}) at line 680, since app.bleApi is set globally in src/api/index.ts. More importantly, this hands every plugin the full BLEApi instance, including unRegister(pluginId), which takes a bare string with no ownership check — any plugin could call app.bleApi.unRegister('other-plugin-id') to tear down another plugin's BLE provider/claims. historyApiHttpRegistry (line 855) and notificationApi (line 918) already establish the safer pattern in this same function: delete the raw instance from appCopy and expose only a scoped proxy (here, registerBLEProvider already does this correctly).
🔒️ Proposed fix to stop exposing the raw BLEApi instance
const bleApi: BLEApi = app.bleApi
appCopy.registerBLEProvider = (provider: BLEProvider) => {
bleApi.register(plugin.id, provider)
}
- appCopy.bleApi = bleApi
+ delete (appCopy as any).bleApi // expose only the plugin-specific proxy📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bleApi: BLEApi = app.bleApi | |
| appCopy.registerBLEProvider = (provider: BLEProvider) => { | |
| bleApi.register(plugin.id, provider) | |
| } | |
| appCopy.bleApi = bleApi | |
| const bleApi: BLEApi = app.bleApi | |
| appCopy.registerBLEProvider = (provider: BLEProvider) => { | |
| bleApi.register(plugin.id, provider) | |
| } | |
| delete (appCopy as any).bleApi // expose only the plugin-specific proxy |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/interfaces/plugins.ts` around lines 882 - 887, Remove the redundant
`appCopy.bleApi = bleApi` assignment in the plugin app construction within the
function containing `registerBLEProvider`. Before exposing the app copy, delete
the inherited raw `bleApi` property so plugins cannot access unscoped methods
such as `unRegister`; retain only the scoped `registerBLEProvider` wrapper that
registers using `plugin.id`, following the existing `historyApiHttpRegistry` and
`notificationApi` patterns.
added 4 commits
July 12, 2026 08:40
Add the BLEProviderRegistry/BLEApi contract and TypeBox schemas to server-api, and wire BLE into the server: config block, plugin registration, API startup, swagger registration, settings round-trip, and the WS-upgrade auth stub.
Implement the BLEApi manager (device dedup, GATT claim management, RSSI-based provider selection, advertisement fanout), the local BlueZ-backed provider and the ESP32 remote gateway provider, plus the company-id lookup and generated OpenAPI/AsyncAPI documents. Local Bluetooth management is enabled by default on Linux.
Add the BLE Manager page (gateway/adapter status, device table, consumer plugins, GATT claims) and the Bluetooth settings panel, wired into the sidebar, routes and server settings view.
Document the BLE REST/WebSocket endpoints and the consumer/provider plugin contracts, and add them to the docs navigation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a BLE Provider API following the v2 provider pattern (Weather, Autopilot, Radar). Providers supply BLE hardware access; consumer plugins subscribe to a merged advertisement stream and request GATT connections through the server.
Closes #2411
Summary
This PR adds a Signal K v2 BLE Provider API that follows the v2 provider pattern. It implements an in-process BLE API manager with device deduplication, server-managed exclusive GATT claim lifecycle, and provider selection, and it exposes REST and WebSocket endpoints for BLE advertisements, devices, consumers, GATT claim status, and settings. It also adds local (BlueZ/Linux) and remote gateway (ESP32-style) providers, TypeBox-driven OpenAPI/AsyncAPI specifications with runtime validation, and an admin UI for BLE monitoring and configuration.
Core BLE API & plugin/provider types
src/api/ble/index.tsadds aBLEApithat:src/api/ble/bleCompanyIds.ts), and continuously updatesgattClaimedByfrom active GATT claims.subscribeGATT()and rawconnectGATT()escape hatches, with claim release, reconnect behavior, and slot accounting./signalk/v2/api/vessels/self/blefor providers, default-provider selection, device discovery/lookup, per-device GATT claim status, consumer listing, settings (GET/PUT), and a REST-backed advertisement upgrade flow.packages/server-api/src/bleapi.tsadds the server-managed BLE plugin API surface:GATTSubscriptionDescriptor,GATTSubscriptionHandle) plus raw GATT (BLEGattConnection).BLEProvider,BLEProviderMethods,BLEProviderRegistry) and anisBLEProvider()runtime type guard.packages/server-api/src/features.tsadds the'ble'feature id, andpackages/server-api/src/serverapi.tsupdatesServerAPIto extendBLEProviderRegistry.packages/server-api/src/index.tsre-exports the new BLE API and schema types.Local BlueZ (Linux) provider
src/api/ble/localProvider.tsaddsLocalBLEProvider(backed by BlueZ via optional deps):BLEAdvertisementframes with MAC normalization, RSSI handling, and conversion/normalization of manufacturer/service data.subscribeGATT()with managed per-MAC session lifecycle: service discovery, notification enablement, optional init writes, optional polling/periodic writes, keep-alive/last-seen refresh, exclusive slot enforcement, reconnect with exponential backoff, and cleanup on shutdown/disconnect.connectGATT()as a raw escape hatch with notification control and disconnect handling.availableGATTSlots()and enforces “one managed subscription per MAC” behavior.Remote gateway provider (ESP32-style)
src/api/ble/remoteProvider.tsaddsRemoteGatewayProvider:POST /signalk/v2/api/ble/gateway/advertisementsto ingest remote advertisements./signalk/v2/api/ble/gateway/ws) that manages GATT control/data via gateway messages (hello/session lifecycle,gatt_subscribe,gatt_write,gatt_data, status/error/disconnect).gatt_datato the correct per-subscription callback, snapshots offline gateway state, and releases related GATT claims on disconnect.GET /signalk/v2/api/ble/gatewaysaggregation of gateway status from live WS sessions plus stored HTTP-only snapshots/TTL.REST/WebSocket API documentation and schema validation
packages/server-api/src/typebox/ble-schemas.tsadds TypeBox schemas as the single source of truth for BLE REST/WebSocket payloads and responses (including MAC validation, device/consumer/provider info, gateway payloads, settings, and GATT claim status).packages/server-api/src/typebox/index.tsre-exports BLE schemas.src/api/ble/openApi.tsadds OpenAPI documentation for the BLE endpoints, including REST paths and the BLE WebSocket advertisement stream, and it registers cookie/bearer security schemes.src/api/ble/asyncApi.tsadds AsyncAPI documentation for BLE advertisement streaming and remote gateway GATT WS bidirectional control/data channels.src/api/swagger.tsregisters both the BLE OpenAPI and BLE AsyncAPI entries into the Swagger/AsyncAPI explorers.Security strategy compatibility
src/dummysecurity.tsupdates the dummy security strategy sorequestAccessaccepts the expected parameters and returns a typed completion response, supporting the BLE WebSocket authorization flow.Server integration and configuration
src/api/index.tsstartsbleApi.start()during server v2 API startup and registers'ble'in the active API list.src/api/ble/openApi.ts+src/api/swagger.tswire BLE into the documented APIs.src/interfaces/plugins.tsexposesapp.bleApito plugins and addsregisterBLEProvider()so plugins can register providers underplugin.id; plugin stop unregisters BLE providers.src/config/config.tsextends server settings with an optionalsettings.bleApiblock (local adapter management flags and max GATT slots).src/serverroutes.tsadds asettings.options.useBLEManagerknob, derived fromapp.config.settings.bleApi?.localBluetoothManaged, and supports updating that option via settings PUT.Admin UI for BLE commissioning/diagnostics
packages/server-admin-ui/src/components/Sidebar/Sidebar.tsxadds “BLE Manager” to the Admin-only Data dropdown.packages/server-admin-ui/src/containers/Full/Full.tsxadds a protected route at/data/blemanagerthat lazy-loads the BLE manager view.packages/server-admin-ui/src/views/ServerConfig/BLEManager.tsxadds a BLE monitoring dashboard:packages/server-admin-ui/src/views/ServerConfig/BLESettings.tsxadds a BLE settings page:packages/server-admin-ui/src/views/ServerConfig/Settings.tsxincludes the BLE settings subcomponent.Packaging and docs
package.jsonadds optional dependencies for local BLE support (@jellybrick/dbus-next,@naugehyde/node-ble).docs/develop/plugins/ble_provider_plugins.md) and BLE REST API docs (docs/develop/rest-api/ble_api.md), and wire them into the existing docs navigation.