refactor(n2k-discovery): build outbound PGNs with the typed ts-pgns classes

[dirkwa]

The N2K discovery responder assembled its outbound PGNs (ISO address claim, product/configuration information responses) from hand-rolled field objects; this builds them with @canboat/ts-pgns' typed classes instead — the compiler now checks field names and shapes against the canboat schema, and the discovery code reads as what it sends. No behavior change intended; the emitted records are the same.

Tested: typecheck and the existing discovery tests; discovery responses verified from a staging image against live bus queries.

Summary

• Refactors N2K discovery responses to use typed @canboat/ts-pgns classes.
• Replaces hand-built PGN objects for ISO address claims, ISO requests, and device configuration responses.
• Adds compile-time validation for field names and shapes.
• Preserves existing response behavior and serialized output.
• Typechecking, discovery tests, and live-bus staging verification pass.

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cf645fa3-e260-4bb9-9be4-2ccbff3433d2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The discovery interface now uses typed PGN constructors for ISO Requests and NMEA 2000 device configuration commands. Existing validation, scheduling, parameters, destinations, PGNs, and response behavior remain unchanged.

Changes

Typed NMEA 2000 PGN construction

Layer / File(s)	Summary
Typed ISO request entry point src/interfaces/n2k-discovery.ts	The module imports typed PGN constructors. sendISORequest emits a PGN_59904 instance.
Device command construction src/interfaces/n2k-discovery.ts	Device-instance and installation-description updates emit typed PGN 126208 command-group-function instances.
Sensor and switch configuration src/interfaces/n2k-discovery.ts	Battery, DC, temperature, humidity, and switch-bank updates emit typed command or write-fields constructors while retaining existing parameters and destinations.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main refactor: constructing outbound PGNs with typed ts-pgns classes.
Description check	✅ Passed	The description explains the problem, the intended solution, the expected behavior, and the testing performed.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[dirkwa]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🤖 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/interfaces/n2k-discovery.ts`:
- Around line 11-15: Add `@canboat/ts-pgns` as an explicit production dependency
in the root package manifest and regenerate the lockfile so its resolved version
and integrity metadata are recorded. Keep the existing runtime imports in
n2k-discovery.ts unchanged.
- Around line 1283-1297: Update the PGN 126208 construction around
PGN_126208_NmeaCommandGroupFunction to use typed per-operation target-PGN
descriptors or builders, rather than raw numeric parameter indexes and untyped
values. Define named constants for the target PGN, field indexes, and priority,
enforce valid value types for those fields, and derive numberOfParameters from
the descriptor list instead of hard-coding it.

🪄 Autofix

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 Plus

Run ID: 82989e88-9310-4e38-a190-e34f631c0faf

📥 Commits

Reviewing files that changed from the base of the PR and between eb2c3c8 and 21d5dd5.

📒 Files selected for processing (1)

• src/interfaces/n2k-discovery.ts

[dirkwa]

@coderabbitai Thanks — both findings are accurate observations, but I don't think either should be fixed in this PR. Details below.

1. @canboat/ts-pgns not declared in the root manifest — real, but pre-existing and not introduced here.

You're right that the root package.json declares neither @canboat/ts-pgns nor @canboat/canboatjs. Today it resolves purely as a hoisted transitive of packages/streams → @canboat/canboatjs@^3.20.0 → @canboat/ts-pgns@^1.11.9, which is exactly as fragile as you imply — a hoisting change or a canboatjs major that drops the dep would break server startup.

But this is not new in this PR. master already imports the same package at runtime in the same file:

import { getAllPGNs, getEnumerationName, getEnumerationValue } from '@canboat/ts-pgns'

My change adds three more named imports from a module the file already loaded unconditionally. So the risk profile is unchanged, and fixing it means adding a root production dependency plus a lockfile entry — a dependency-policy decision for the maintainers that reaches well past a no-behavior-change refactor, and one that would be better made once for both packages rather than smuggled in here. Happy to open it as a separate PR if the maintainers want it.

2. Typed target-PGN descriptors — correct about the types, but the erasure is upstream and runtime validation already covers it.

Confirmed in the published .d.ts: N2K_Variable = any and N2K_FieldIndex = number, so you're right that an invalid target field still compiles. Two reasons I'm leaving it:

• The any is in ts-pgns, not here. A local descriptor layer would narrow what this file passes but can't make the library reject anything, so it buys type-checking of our own call sites only — at the cost of 13 construction sites across 8 distinct target PGNs (60928, 126998, 127501, 127506, 127508, 130313, plus two dynamic), each with its own field-index semantics. That's a real design, and it belongs upstream in ts-pgns where the per-PGN field metadata already lives, not duplicated in a server interface file.
• The values are already range-validated at every site before construction, each returning HTTP 400 on failure — e.g. deviceInstance must be 0-253, batteryInstance must be 0-252, temperatureSource must be 0-15, switchBankInstance must be 0-252. For a VARIABLE field the valid range is a property of the addressed target field rather than something a generic TypeScript type could express, so runtime validation is the appropriate defence here regardless.

This PR is a like-for-like swap of hand-built object literals for the typed constructors, with the encoded bytes and priorities verified identical. Broadening it into a descriptor framework would undercut that reviewability.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@dirkwa Thank you for the detailed clarification.

I agree that the root dependency issue is pre-existing. This PR does not increase its runtime dependency risk because src/interfaces/n2k-discovery.ts already imported @canboat/ts-pgns.

I also agree that a local descriptor layer is not appropriate for this refactor. The type erasure is upstream, and the existing per-field runtime validation protects the variable values. The typed constructors still improve validation for the outbound PGN shape without changing the encoded output.

I will withdraw both findings. If maintainers decide to address the undeclared root dependency, it should be a separate dependency-policy change.

🐇 ✓

---

✏️ Learnings added

Learnt from: dirkwa
URL: https://github.com/SignalK/signalk-server/pull/2911

Timestamp: 2026-08-06T15:33:06.493Z
Learning: In `src/interfaces/n2k-discovery.ts`, `canboat/ts-pgns` was already imported at runtime before PR `#2911`. The root `package.json` does not directly declare `canboat/ts-pgns` or `canboat/canboatjs`; this is a pre-existing dependency-management concern and is not introduced by adding named imports in the N2K discovery typed-constructor refactor.

---

Learnt from: dirkwa
URL: https://github.com/SignalK/signalk-server/pull/2911

Timestamp: 2026-08-06T15:33:06.493Z
Learning: In `src/interfaces/n2k-discovery.ts`, target PGN variable values have per-field runtime range validation before construction and return HTTP 400 when invalid. `canboat/ts-pgns` declares `N2K_Variable` as `any` and `N2K_FieldIndex` as `number`; do not request a local target-PGN descriptor framework in behavior-preserving refactors because it would duplicate upstream per-PGN field metadata.

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.

_{You are interacting with an AI system.}

[dirkwa]

ready for human review

[dirkwa]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.