feat: received-message log in core (DSC first)#2805
Open
clarkbw wants to merge 1 commit 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 communications message-log contracts and schemas, a DSC adapter, a SQLite-backed store, a communications API with notification mirroring and REST/OpenAPI support, and tests covering contracts, storage, adapter mapping, and API behavior. ChangesCommunications message log
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🤖 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 `@src/api/communications/dscAdapter.ts`:
- Around line 37-43: The payload object in the DSC adapter is currently
unchecked because MessageLogEntryInput.payload is unknown, so use the existing
DscPayload type here to enforce the contract. Update the payload construction in
dscAdapter so the local object is explicitly typed as DscPayload, relying on the
existing DscPayload definition to catch field renames or typos at compile time
without introducing a new local type.
In `@src/api/communications/store.ts`:
- Around line 140-172: The MessageLogStore.query method currently returns
unbounded results when filter.limit is missing or invalid, which can make GET
/messages scan and materialize the entire message_log table. Update query() to
apply a sane default cap and enforce an upper bound using named constants such
as DEFAULT_QUERY_LIMIT and MAX_QUERY_LIMIT near SCHEMA_VERSION, and clamp
filter.limit before appending the LIMIT clause so rowToEntry only processes a
bounded result set.
- Line 1: The import of DatabaseSync from node:sqlite in store.ts requires a
newer Node runtime than the current engines.node floor allows. Update
package.json to raise the minimum supported Node version to one that ships
node:sqlite unflagged (22.13.0+ or 23.4.0+), or otherwise guard the import path
used by the store module so older Node 22.x releases do not load it.
In `@test/communications-api.ts`:
- Line 68: Remove the “Correction B” prefix from the cleanup comment in the test
setup and keep only a neutral rationale. Update the comment near the stale DB
deletion in communications-api test code so it no longer references an
intermediate-commit label, while preserving the intent of the cleanup.
- Line 16: Remove the obsolete “(unused until Task 4)” reference from the
comment in the communications-api test so it only describes the stubs that
remain relevant in the current branch state. Update the inline comment near the
minimal express stubs to eliminate any mention of intermediate-commit task
numbering or build-phase milestones that no longer exist.
- Around line 167-174: The acknowledge/clear tests are asserting on disposition
too early because `mirrorNotificationDelta` updates the store asynchronously and
the REST call may return before `acknowledgedAt` or `clearedAt` is visible.
Update the test flow around the acknowledge/clear checks to wait for the
disposition to appear, either by polling/retrying after the POST before reading
`/messages/${entry.id}`, or by making the delta handler await the `store.update`
path so the write completes before the response is observed.
🪄 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: b0ff5685-ce15-4ad7-ae27-fef43d7c6448
📒 Files selected for processing (17)
.gitignorepackages/server-api/src/communications.test.tspackages/server-api/src/communications.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/communications-schemas.tspackages/server-api/src/typebox/index.tssrc/api/communications/dscAdapter.tssrc/api/communications/index.tssrc/api/communications/openApi.tssrc/api/communications/store.tssrc/api/index.tssrc/api/swagger.tstest/communications-api.tstest/communications-dsc-adapter.tstest/communications-store.ts
clarkbw
force-pushed
the
feat/communications-message-log
branch
from
2dcdb38 to
099553e
Compare
Contributor
Author
|
One callout that's bigger than the rest of the diff: this raises
It's a project-wide minimum change though, so it's your call. If you'd rather not |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/api/communications/index.ts`:
- Line 92: The debug logging in the communications flow is eagerly constructing
strings even when debug is disabled. Update the `debug(...)` calls in
`src/api/communications/index.ts` (including the `logMessage` path and the other
listed debug sites) to be guarded with `debug.enabled &&` so the message
interpolation only happens when debugging is on. Use the existing `debug` symbol
at each call site and keep the same log text, just wrap the invocation to
prevent unnecessary per-request/ingest work.
- Around line 117-148: The concurrent unawaited store updates in
mirrorNotificationDelta can overwrite each other and drop acknowledgedAt when a
value is both acknowledged and cleared. Update the logic in
mirrorNotificationDelta to build one combined patch per update.value and call
this.store.update only once, including both acknowledgedAt and clearedAt when
both conditions apply. Keep the notificationId/entryId flow and the
this.notiToEntry cleanup for the normal state path.
- Line 39: The in-memory notiToEntry map can grow without bound for calls that
are never cleared and it is lost on restart, so ack/clear reconciliation can
drift from stored dispositions. Update the communications flow around
notiToEntry and the clearedAt handling to either persist this mapping or evict
it with a bounded lifecycle/cleanup strategy, and make sure post-restart delta
processing can rebuild or safely recover the mapping before applying
dispositions.
In `@src/api/communications/store.ts`:
- Around line 105-136: The `append` method in `MessageLogStore` currently
persists `entry.receivedAt` as-is, which can leave non-canonical timestamps in
`received_at` and break string-based ordering/filtering in `query()`. Normalize
`receivedAt` to a canonical UTC ISO-8601 string before the INSERT by parsing the
incoming value, converting valid dates to UTC, and falling back to `new
Date().toISOString()` for empty, null, or malformed inputs. Keep the change
localized to `append` so all persisted `message_log` rows use the same canonical
format.
🪄 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: 53365671-4847-420c-b45f-55b6832f03ee
📒 Files selected for processing (18)
.gitignorepackage.jsonpackages/server-api/src/communications.test.tspackages/server-api/src/communications.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/communications-schemas.tspackages/server-api/src/typebox/index.tssrc/api/communications/dscAdapter.tssrc/api/communications/index.tssrc/api/communications/openApi.tssrc/api/communications/store.tssrc/api/index.tssrc/api/swagger.tstest/communications-api.tstest/communications-dsc-adapter.tstest/communications-store.ts
|
|
||
| export class CommunicationsApi { | ||
| private store!: MessageLogStore | ||
| private notiToEntry: Map<string, string> = new Map() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
notiToEntry mapping is in-memory only and unbounded for un-cleared calls.
Entries are removed only on clearedAt; mappings for actionable calls that are never cleared accumulate, and the map is lost on restart so post-restart ack/clear deltas won't mirror onto stored dispositions. Acceptable for this PR, but worth noting as an operational limitation.
Also applies to: 144-144
🤖 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/communications/index.ts` at line 39, The in-memory notiToEntry map
can grow without bound for calls that are never cleared and it is lost on
restart, so ack/clear reconciliation can drift from stored dispositions. Update
the communications flow around notiToEntry and the clearedAt handling to either
persist this mapping or evict it with a bounded lifecycle/cleanup strategy, and
make sure post-restart delta processing can rebuild or safely recover the
mapping before applying dispositions.
Contributor
Author
There was a problem hiding this comment.
Leaving this for the PR — it's a known v1 limitation (the entry itself is always persisted; only the live ack/clear mirror is in-memory). Persisting or rebuilding the map on restart is an architectural change that's out of scope here. Follow-up once the disposition lifecycle gets wired through Kip.
Contributor
There was a problem hiding this comment.
@clarkbw, understood — thanks for the clarification. Treating this as a known v1 limitation rather than something to block here makes sense, especially since the persisted part is the entry itself and only the live ack/clear mirroring is in-memory for now.
If you'd like, I can open a follow-up GitHub issue to track persisting/rebuilding the notification-to-entry linkage once the disposition lifecycle is wired through Kip.
(•ᴗ•)🥕
✏️ Learnings added
Learnt from: clarkbw
Repo: SignalK/signalk-server PR: 2805
File: src/api/communications/index.ts:39-39
Timestamp: 2026-06-29T13:47:09.105Z
Learning: In `src/api/communications/index.ts`, the `CommunicationsApi` in-memory notification-to-entry mapping used for mirroring acknowledgement/clear disposition updates is an intentional v1 limitation of the communications message-log feature. The message log entry itself is persisted, but persisting or rebuilding that live mirror state across restarts is deferred to a follow-up change once the disposition lifecycle is wired through Kip.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
Author
There was a problem hiding this comment.
sounds like a good idea to have a followup issue for this
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="SignalK/signalk-server"
pr_url="https://github.com/SignalK/signalk-server/pull/2805"
comment_url="https://github.com/SignalK/signalk-server/pull/2805#discussion_r3481178757"
assignee="clarkbw"
title="Follow up communications message-log notification linkage across restarts"
body_file="$(mktemp)"
cat > "$body_file" <<'EOF'
## Summary
The communications message log currently persists each message entry, but the live notification-to-entry linkage used to mirror acknowledgement and clear events (`notiToEntry` in `src/api/communications/index.ts`) is maintained only in memory.
This is an accepted v1 limitation for the work in `#2805`, but it should be addressed in a follow-up once the disposition lifecycle is wired through Kip.
## Why this follow-up is needed
Because the linkage is in-memory only:
- actionable messages that are never cleared can leave entries in the map indefinitely
- process restarts lose the linkage entirely
- post-restart acknowledgement/clear deltas cannot be reconciled back into stored message dispositions unless the mapping is rebuilt or otherwise recovered safely
The message log entry itself is already persisted; the gap is specifically in the live mirroring path for disposition updates.
## Affected areas
- `src/api/communications/index.ts`
- communications message disposition lifecycle
- notification acknowledgement/clear mirroring
- any future Kip-driven disposition integration
## Requested changes
Implement a durable or recoverable strategy for the notification-to-entry relationship used by `CommunicationsApi`, for example by:
- persisting the relationship in storage, or
- rebuilding it on startup from durable state, or
- introducing a bounded lifecycle/cleanup strategy that safely handles restart recovery
The solution should preserve the current behavior where entries are always persisted even if notification mirroring is unavailable.
## Acceptance criteria
- acknowledgement and clear updates can be mirrored to the correct stored message entry after a server restart, or there is an explicit bounded recovery strategy that prevents silent drift
- the notification-to-entry lifecycle does not grow without bound for long-lived actionable messages
- the design is aligned with the planned Kip-based disposition lifecycle
- tests cover restart/recovery behavior and lifecycle cleanup expectations
## Backlinks
- PR: https://github.com/SignalK/signalk-server/pull/2805
- Review comment: https://github.com/SignalK/signalk-server/pull/2805#discussion_r3481178757
Requested by `@clarkbw`.
EOF
issue_url="$(gh issue create \
--repo "$repo" \
--title "$title" \
--body-file "$body_file" \
--assignee "$assignee")"
echo "$issue_url"Length of output: 3127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="SignalK/signalk-server"
title="Follow up communications message-log notification linkage across restarts"
body_file="$(mktemp)"
cat > "$body_file" <<'EOF'
## Summary
The communications message log currently persists each message entry, but the live notification-to-entry linkage used to mirror acknowledgement and clear events (`notiToEntry` in `src/api/communications/index.ts`) is maintained only in memory.
This is an accepted v1 limitation for the work in `#2805`, but it should be addressed in a follow-up once the disposition lifecycle is wired through Kip.
## Why this follow-up is needed
Because the linkage is in-memory only:
- actionable messages that are never cleared can leave entries in the map indefinitely
- process restarts lose the linkage entirely
- post-restart acknowledgement/clear deltas cannot be reconciled back into stored message dispositions unless the mapping is rebuilt or otherwise recovered safely
The message log entry itself is already persisted; the gap is specifically in the live mirroring path for disposition updates.
## Affected areas
- `src/api/communications/index.ts`
- communications message disposition lifecycle
- notification acknowledgement/clear mirroring
- any future Kip-driven disposition integration
## Requested changes
Implement a durable or recoverable strategy for the notification-to-entry relationship used by `CommunicationsApi`, for example by:
- persisting the relationship in storage, or
- rebuilding it on startup from durable state, or
- introducing a bounded lifecycle/cleanup strategy that safely handles restart recovery
The solution should preserve the current behavior where entries are always persisted even if notification mirroring is unavailable.
## Acceptance criteria
- acknowledgement and clear updates can be mirrored to the correct stored message entry after a server restart, or there is an explicit bounded recovery strategy that prevents silent drift
- the notification-to-entry lifecycle does not grow without bound for long-lived actionable messages
- the design is aligned with the planned Kip-based disposition lifecycle
- tests cover restart/recovery behavior and lifecycle cleanup expectations
## Backlinks
- PR: https://github.com/SignalK/signalk-server/pull/2805
- Review comment: https://github.com/SignalK/signalk-server/pull/2805#discussion_r3481178757
Requested by `@clarkbw`.
EOF
gh issue create \
--repo "$repo" \
--title "$title" \
--body-file "$body_file"Length of output: 2704
@clarkbw Opened the follow-up issue: #2811
The GitHub integration didn’t have permission to assign it, so it was created unassigned, but the tracking issue is in place with the PR and review comment linked.
Contributor
Author
There was a problem hiding this comment.
The map deliberately shares the notifications' lifetime: notifications are in-memory and don't survive a restart, so post-restart there are no live notifications that could reference a pre-restart entry — an empty map is consistent, and any disposition already persisted stays put. Growth is bounded by actionable calls that are never acked/cleared, which in practice is a handful of two-string entries. Persisting the mapping would add a write path with no observable behavior change.
Contributor
There was a problem hiding this comment.
@clarkbw Thanks for spelling that out — that's a clean invariant: since notifications are themselves in-memory and don't survive a restart, there's no scenario where a stale map entry could reference a pre-restart entry post-restart. The empty-map-on-restart state is trivially consistent with the empty-notifications state, and persisted dispositions are unaffected either way. Bounded growth for un-acked/uncleared actionable calls being just a handful of small entries also make sense — persisting the mapping would indeed just be a redundant write path here. Good to leave as-is.
✏️ Learnings added
Learnt from: clarkbw
Repo: SignalK/signalk-server PR: 2805
File: src/api/communications/index.ts:41-41
Timestamp: 2026-07-10T13:17:31.497Z
Learning: In `src/api/communications/index.ts`, the `CommunicationsApi` in-memory `notiToEntry` map deliberately shares its lifetime with the (also in-memory, non-persisted) notification subsystem: since notifications don't survive a server restart, an empty `notiToEntry` map post-restart is always consistent (no live notification can reference a pre-restart entry), and any disposition already persisted to the message log remains intact. Growth is bounded in practice to a small number of two-string entries for actionable calls that are never acknowledged/cleared. Persisting this mapping was considered and rejected as adding a write path with no observable behavior change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
clarkbw
force-pushed
the
feat/communications-message-log
branch
from
099553e to
cd19a80
Compare
This was referenced Jun 29, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
src/api/communications/index.ts (1)
46-46: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winGuard the remaining
debug()sites.These calls still eagerly build log strings when debugging is off. As per coding guidelines, guard
debug()arguments withdebug.enabled &&to avoid eager evaluation of disabled debug statements.Also applies to: 53-53, 107-107, 141-142, 178-178
🤖 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/communications/index.ts` at line 46, Guard the remaining Communications API debug statements so their arguments are only built when debugging is enabled; update the `debug()` calls in the `communications` module to use `debug.enabled &&` before constructing the log strings. Apply this pattern to the existing debug sites in the startup, request, error, and shutdown paths so symbols like the module-level startup log and the other `debug()` calls no longer eagerly evaluate when debugging is off.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 `@packages/server-api/src/communications.test.ts`:
- Around line 10-48: The communications type test is relying on a casted fixture
and weak assertions instead of validating the real contract. In
communications.test.ts, replace the manual MessageLogEntryInput/DscPayload
object with a fixture that fully satisfies the exported types (including
required DscPayload fields like format), or move this to a compile-time type
assertion test; keep the focus on MessageLogEntryInput, MessageLogEntry, and
MessageLogStore contracts rather than hard-coded runtime literals. Also replace
the trivial equality checks with assertions that exercise actual behavior or
schema compatibility so the test proves the shape is valid without bypassing
type safety.
In `@packages/server-api/src/communications.ts`:
- Around line 32-45: The MessageLogEntryInput payload is still untyped even
though MessageType only allows the DSC case, so update the communications
contract to use a discriminated union or make the dsc variant carry DscPayload
directly. Adjust the MessageLogEntryInput type and any related TypeBox
schema/validators in communications.ts so the payload shape is enforced at the
API boundary and stays in sync with the dsc message type.
- Around line 98-100: The WithMessageLog capability still declares logMessage as
optional, even though app.logMessage(entry) is now wired unconditionally as a
required ServerAPI capability. Update the WithMessageLog type to make logMessage
required, and make sure any related ServerAPI capability definitions or
consumers that reference logMessage are adjusted to reflect the non-optional
contract so missing wiring fails fast.
In `@src/api/communications/dscAdapter.ts`:
- Line 11: Remove the echo comments in dscAdapter.ts, including the top-of-file
DSC category mapping comment and the similar comment near dscToMessageInput(),
since priorityForCategory() and dscToMessageInput() already make the behavior
self-explanatory. Keep the implementation unchanged and delete only comments
that restate what the code already says.
- Around line 58-60: The `debug()` call in `dscAdapter` is not guarded per the
repo’s debug usage rule. Update the `logMessage` missing branch in `dscAdapter`
so the `debug('logMessage not available; dropping DSC call')` invocation is
wrapped with `debug.enabled &&` to prevent eager evaluation when debug logging
is disabled.
In `@src/api/communications/index.ts`:
- Around line 95-113: The notification creation flow in the communications
handler currently raises the alarm before persistence and can leave an active
notification if store.append fails. Update the logic around this block in the
communications index handler so that persistence happens first, or add
compensation to undo the notificationApi.raise result if append throws. Make
sure the notificationId to entry mapping in notiToEntry is only recorded after a
successful append and that any failure path in this sequence is handled
consistently.
In `@src/api/communications/store.ts`:
- Around line 159-165: Normalize the range bounds in `query()` before building
the SQL clauses: `filter.from` and `filter.to` are currently passed through raw,
so convert them to the same canonical UTC form used by `append()` for
`received_at` in `src/api/communications/store.ts`. Add validation in the
`query()` path so malformed timestamps are rejected early instead of being sent
to the `received_at` comparisons, and keep the existing `clauses.push` /
`params.push` flow using the normalized values.
- Line 60: The `debug()` call in the message-log store still eagerly evaluates
its template string during every construction of the store. Update the logging
in `store.ts` around the message-log database open path to guard the call with
`debug.enabled &&` so the message is only built when debugging is enabled. Use
the existing `debug` logger symbol in this store initialization path and keep
the log content unchanged.
In `@test/communications-api.ts`:
- Around line 174-175: Remove the echo comments in the test around the
app.logMessage, app.acknowledgeMessage, and app.clearMessage flows; they only
restate the next statement and should be deleted or rewritten to explain
non-obvious intent. Update the relevant test blocks in
test/communications-api.ts so the remaining comments, if any, provide rationale
rather than narrating what the code does.
- Around line 110-117: The polling in the communications API test dereferences
got!.disposition before the async mirror has guaranteed it exists, so the loop
can throw on the first pre-mirror read. Update the retry condition in the test
around api.getStore().get(entry.id) to first verify got and got.disposition are
set before checking acknowledgedAt and clearedAt, then keep polling until both
timestamps appear. Use the existing test block that asserts acknowledgedAt and
clearedAt to locate the change.
- Around line 199-214: The teardown in the `after` hook should not throw if
`before` failed before `server` was assigned, since `await server.stop()` can
mask the real startup error. Update the cleanup logic around `server.stop()` to
be best-effort by guarding against an uninitialized `server` (or catching and
ignoring stop failures), while keeping the rest of the DB and environment
cleanup intact in `test/communications-api.ts`.
In `@test/communications-dsc-adapter.ts`:
- Around line 19-59: The current tests only cover the pure dscToMessageInput
mapper, so add coverage for the registerDscAdapter boundary behavior as well.
Extend this suite to verify that registerDscAdapter() forwards mapped DSC
messages into app.logMessage() with the expected envelope, and add a separate
case for when logMessage is missing to confirm the message is dropped without
error. Use the existing dscToMessageInput and registerDscAdapter symbols to keep
the tests focused on observable behavior.
---
Duplicate comments:
In `@src/api/communications/index.ts`:
- Line 46: Guard the remaining Communications API debug statements so their
arguments are only built when debugging is enabled; update the `debug()` calls
in the `communications` module to use `debug.enabled &&` before constructing the
log strings. Apply this pattern to the existing debug sites in the startup,
request, error, and shutdown paths so symbols like the module-level startup log
and the other `debug()` calls no longer eagerly evaluate when debugging is off.
🪄 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: a0ef383c-da8c-490a-8977-553f43f75c3e
📒 Files selected for processing (18)
.gitignorepackage.jsonpackages/server-api/src/communications.test.tspackages/server-api/src/communications.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/communications-schemas.tspackages/server-api/src/typebox/index.tssrc/api/communications/dscAdapter.tssrc/api/communications/index.tssrc/api/communications/openApi.tssrc/api/communications/store.tssrc/api/index.tssrc/api/swagger.tstest/communications-api.tstest/communications-dsc-adapter.tstest/communications-store.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/communications.ts`:
- Line 31: Remove the echo doc comments in communications.ts that only restate
the field/type names, including the comment above the shared message fields and
the one at the other flagged location. Either delete them entirely or replace
them with a brief why-focused note in the relevant interface/type definitions
(for example around the common/persisted split) using the existing symbols in
this file.
In `@src/api/communications/index.ts`:
- Around line 115-118: The append error handling in `logMessage()` currently
lets a failing `notificationApi.clear()` replace the original persistence
failure. Update the `catch` block around `append` so `clear(raisedId)` is
treated as best-effort cleanup: preserve the original `err` from `append`, and
if `clear` throws, suppress or log that cleanup error without changing the
thrown failure. Use the `logMessage` and `notificationApi.clear` symbols to
locate the fix.
In `@test/communications-dsc-adapter.ts`:
- Around line 68-70: The stubbed logMessage in the test uses an unnecessary type
escape hatch, so remove the as never cast and return undefined directly. Update
the test fixture around registerDscAdapter/logMessage to keep the
Promise<unknown> contract while preserving strict type checking and avoiding
equivalent suppressions.
🪄 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: 4ce9fde2-b65f-4798-ab65-7322a3dd71f6
📒 Files selected for processing (9)
packages/server-api/src/communications.test.tspackages/server-api/src/communications.tspackages/server-api/src/typebox/communications-schemas.tssrc/api/communications/dscAdapter.tssrc/api/communications/index.tssrc/api/communications/store.tstest/communications-api.tstest/communications-dsc-adapter.tstest/communications-store.ts
💤 Files with no reviewable changes (1)
- src/api/communications/dscAdapter.ts
| clearedAt?: string | ||
| } | ||
|
|
||
| /** Fields every message carries, regardless of `type`. */ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove these echo comments.
These doc blocks just restate the type names. Either drop them or use the space to explain why the common/persisted split matters.
Suggested cleanup
-/** Fields every message carries, regardless of `type`. */
export interface MessageLogEntryCommon {
@@
-/** A persisted entry: the input plus server-assigned fields. */
export type MessageLogEntry = MessageLogEntryInput & {As per coding guidelines, "**/*.{ts,tsx,js}: Write self-documenting code; comments explain 'why', not 'what' - avoid echo comments restating what the code already says". As per path instructions, "## Echo comments Flag any comment that merely restates what the code already says."
Also applies to: 59-59
🤖 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/communications.ts` at line 31, Remove the echo doc
comments in communications.ts that only restate the field/type names, including
the comment above the shared message fields and the one at the other flagged
location. Either delete them entirely or replace them with a brief why-focused
note in the relevant interface/type definitions (for example around the
common/persisted split) using the existing symbols in this file.
Sources: Coding guidelines, Path instructions
clarkbw
force-pushed
the
feat/communications-message-log
branch
from
f7c5d02 to
80f1519
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/api/communications/index.ts`:
- Around line 145-146: Replace the any cast for v.value with a specific
notification-value type containing status?.acknowledged and state: ALARM_STATE,
using the already-imported ALARM_STATE type or enum as appropriate. Remove the
eslint-disable comment and update the cast at the affected notification handling
code.
In `@src/api/communications/openApi.ts`:
- Around line 122-134: Extend the `/messages` endpoint response definitions in
the OpenAPI configuration to document both 400 responses for malformed time
bounds and 500 responses for server errors, alongside the existing 200 response.
Add clear descriptions and appropriate JSON error schemas consistent with the
API’s existing conventions.
- Around line 144-155: Add a documented 500 response to the `/messages/{id}`
OpenAPI definition in `src/api/communications/openApi.ts`, alongside the
existing 200 and 404 responses, using a clear description such as “Internal
server error.”
🪄 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: 7adb860f-77f9-41fb-922a-5797acf04bbd
📒 Files selected for processing (18)
.gitignorepackage.jsonpackages/server-api/src/communications.test.tspackages/server-api/src/communications.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tspackages/server-api/src/typebox/communications-schemas.tspackages/server-api/src/typebox/index.tssrc/api/communications/dscAdapter.tssrc/api/communications/index.tssrc/api/communications/openApi.tssrc/api/communications/store.tssrc/api/index.tssrc/api/swagger.tstest/communications-api.tstest/communications-dsc-adapter.tstest/communications-store.ts
A server-owned log of received digital messages, History-API-shaped, so the SOLAS / 47 CFR §80.409 record for distress/urgency/safety calls is a server property rather than something every plugin reinvents. DSC is the only v1 type; the envelope is generic so NAVTEX and AIS safety/addressed text can be added later without a schema change. - generic envelope in @signalk/server-api, discriminated on `type` so the `dsc` arm carries a typed DscPayload; new types add an arm without touching storage - MessageLogStore seam with a node:sqlite default (WAL, foreign keys, migrations, bounded queries, UTC-canonical time bounds) - a single app.logMessage() ingestion door + a core DSC adapter - GET /signalk/v2/api/communications/messages[/:id], anonymous read under allow_readonly, with OpenAPI + typebox schemas - distress/urgency/safety raise a notification and the entry's disposition mirrors ack/clear via the existing notifications API; routine = log only node:sqlite needs no native build (the win on a Pi) and is unflagged from Node 22.13, so the engines floor moves to >=22.13.0. Proposal: https://gist.github.com/clarkbw/1c925d7ac2469e57cdbd1421b17672c2
clarkbw
force-pushed
the
feat/communications-message-log
branch
from
80f1519 to
e58923f
Compare
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
1 participant
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.
Implements the received-message log we sketched out a while back —
proposal: https://gist.github.com/clarkbw/1c925d7ac2469e57cdbd1421b17672c2
A server-owned log of received digital messages, History-API-shaped, with DSC as
the first (and v1-only) message type. The envelope is generic so NAVTEX and AIS
safety/addressed text can slot in later without a schema change. The point is to
keep the SOLAS / 47 CFR §80.409 "log every distress/urgency/safety call" record
as a server property instead of every plugin reinventing it.
What's here
@signalk/server-api): a generic envelope + type-specificpayload; DSC payload + aDscCallparser contract.MessageLogStoreseam, defaultSqliteMessageLogStore.app.logMessage(entry)door (+ a small core DSC adapterthat maps parser output onto the envelope).
GET /signalk/v2/api/communications/messages(filter byfrom/to/type/priority/sender, order, limit) and
/messages/:id; anonymous readunder
allow_readonly, like resources. OpenAPI + typebox included.dispositionmirrors the existing ack/clear lifecycle; routine = log only.node:sqlite
This is core's first built-in DB. It needs no native build step (the win on a Pi)
and Node ≥ 22 is already the floor. WAL mode, foreign keys, a schema_version
table, single writer. It's still flagged experimental, so it emits an
ExperimentalWarningon startup — I deliberately did not suppress that.Calls I made without your feedback (happy to change any)
app.logMessage+ core adapter (not acommunications.*deltapath), since these are discrete events and one door also covers manual / future
non-NMEA sources.
communications/messagesfilterable bytype.notification + mirrored disposition.
Not in this PR
DscCall(separate repos) — theadapter + contract are here and unit-tested; wiring lands when the parsers
surface it.
Tests: store round-trip/reopen, ingestion, REST filters/404, and the notification
raise + ack/clear mirror.
npm testgreen.This PR adds a server-owned received-message log to core, starting with DSC support. It introduces shared message-log types and TypeBox schemas in the server API, plus a SQLite-backed
SqliteMessageLogStorewith append/get/query/update support, WAL + foreign keys, schema versioning, and persistence across restarts.It adds an ingestion seam
app.logMessage(entry), a DSC adapter that maps parsed DSC calls into message-log entries (type: 'dsc', category-to-priority mapping, and DSC payload shaping), and a communications subsystem wired into core startup. It exposes REST endpoints atGET /signalk/v2/api/communications/messagesandGET /signalk/v2/api/communications/messages/:idwith filtering by from/to/type/priority/sender, ordering and limits, plus 404 handling for unknown ids (supporting anonymous reads whenallow_readonlyis enabled). It also adds Swagger/OpenAPI records and aggregates them into the server’s API docs.For actionable messages (
distress,urgency,safety), the implementation raises notifications and mirrors notification acknowledgement/clear lifecycle into the stored entry viadisposition; routine messages get logged without a notification/disposition. Tests cover store round-trips and reopen, DSC mapping, ingestion + REST filtering/ordering/limits and 404 behavior, and notification/disposition mirroring. The PR also updates.gitignoreto ignore SQLite*.db-shmand*.db-walcompanion files and tightens the Node.js engine requirement to>=22.13.0to supportnode:sqlitereliably.