Claude finished @Yeraze's task —— View job

PR Review: fix(channels): stop proto3 boolean elision re-enabling uplink/downlink

• Read changed files
• Review code quality and correctness
• Review test coverage
• Post final feedback

Summary

This is a well-scoped, correct fix for a genuine proto3 wire-format bug. The root cause analysis in the PR description is accurate. Here's the detailed review:

src/server/meshtasticManager.ts — Core Fix

Correctness: ✅

The change at lines 5091–5092 from ?? true → ?? false is exactly right. Meshtastic's ChannelSettings proto defines both uplink_enabled and downlink_enabled as plain bool with an implicit proto3 default of false. When a user disables one of these fields, proto3 omits it on the wire, protobuf.js decodes that absence as undefined, and the old ?? true fallback silently re-enabled it. Changing to ?? false faithfully reconstructs the proto3 default.

The comment block (lines 5084–5090) is thorough and directly references the issue number — good for future maintainers.

One minor observation: the debug log on line 5095 still reads channel.settings.uplinkEnabled and channel.settings.downlinkEnabled directly (pre-coalesced values), so it will log undefined instead of false when the fields are elided. This is cosmetically confusing in logs but has no functional impact. Consider capturing the coalesced values into locals if log readability matters:

const uplinkEnabled = channel.settings.uplinkEnabled ?? false;
const downlinkEnabled = channel.settings.downlinkEnabled ?? false;

await databaseService.channels.upsertChannel({
  ...
  uplinkEnabled,
  downlinkEnabled,
  ...
}, this.sourceId);
logger.debug(`... uplink: ${uplinkEnabled}, downlink: ${downlinkEnabled} ...`);

src/db/repositories/channels.ts — INSERT Branch Fix

Correctness: ✅

The INSERT branch change at lines 179–180 (?? false instead of ?? true) is correct and consistent with the fix above. The explanatory comment (lines 175–178) clearly documents the #3594 rationale.

UPDATE branch intentionally untouched: ✅

Lines 140–141 correctly use ?? existingChannel.uplinkEnabled / ?? existingChannel.downlinkEnabled for updates, preserving the stored value when the field is absent. This is the right behavior for reconnect-path ingest on a channel the DB already knows about — the PR description correctly explains why this path was left alone.

src/server/meshtasticManager.channelProto3Elision.test.ts — Test File

Coverage: ✅ Comprehensive

The 5 test cases cover the important branches:

Mock setup: ✅

The mock infrastructure is thorough. All heavy dependencies (TcpTransport, protobufLoader, cronScheduler, etc.) are cleanly mocked. The test correctly uses vi.clearAllMocks() in beforeEach and vi.restoreAllMocks() in afterEach.

Minor nit — redundant assertion in test case 1:

// line 143-145
expect(stored.downlinkEnabled).toBe(false);
// Bug would have stored true via `?? true`.
expect(stored.downlinkEnabled).not.toBe(true);

expect(x).toBe(false) already implies x !== true. The .not.toBe(true) assertion is redundant. Same pattern appears in test case 2 (lines 164–165). Not a blocking issue, just noise.

Minor nit — manager typed as any:

// line 111
let manager: any;

If MeshtasticManager is exported, a typed reference would give better IDE support and surface regressions at compile time. But given the complexity of mocking this class, any is pragmatic and acceptable for a focused regression test.

Overall Assessment

Approve. The fix is minimal, correct, well-documented, and backed by a regression test that was verified to fail against the pre-fix code. No security concerns. No performance concerns. The behavioral change is safe: proto3 false (the zero default) is the only value that was being incorrectly inflated to true, and the fix precisely targets that path without touching the user-save route or the UPDATE branch.