Claude finished @Yeraze's task —— View job

PR Review: fix(meshtastic): resolve position-packet channel from decryption context (#3682)

• Gather context and read changed files
• Review code quality and best practices
• Check for potential bugs
• Evaluate test coverage
• Post final review

Summary

This is a well-scoped, correctness fix for a real channel-resolution inconsistency. The root cause analysis in the PR description is accurate and matches the code. Overall the change is solid; a few observations below.

Code Quality

resolveBroadcastChannelIndex (meshtasticManager.ts:6081)

The helper is a clean extraction and mirrors the text-message path faithfully. One observation: the inline logic for the server-decryption path in processTextMessageProtobuf (lines 5781–5810) is now a near-identical copy of resolveBroadcastChannelIndex. The helper was introduced for positions but the text path was left as-is. This is technically fine (both produce the same result), but it leaves two diverging copies that could drift in future. Consider a follow-up to refactor processTextMessageProtobuf to also call resolveBroadcastChannelIndex for the broadcast case — that would make "text and position use the exact same logic" true by construction, not just by comment.

Context spreading (meshtasticManager.ts:5537–5540)

await this.processPositionMessageProtobuf(meshPacket, processedPayload as any, {
  ...context,
  decryptedBy,
  decryptedChannelId: decryptedChannelId ?? undefined,
});

The ...context spread here is slightly surprising — context at this point is a ProcessingContext object built earlier in the dispatch loop. If context already carries decryptedBy/decryptedChannelId fields (from an earlier iteration or outer call), the explicit overrides below the spread will win, so it's safe. But documenting why ...context is spread (rather than just constructing { decryptedBy, decryptedChannelId } directly as the text-message call does on line 5526–5530) would help future readers. Compare with the text path:

// TEXT_MESSAGE_APP (line 5526):
{ decryptedBy, decryptedChannelId: decryptedChannelId ?? undefined }
// POSITION_APP (line 5537):
{ ...context, decryptedBy, decryptedChannelId: decryptedChannelId ?? undefined }

The asymmetry is minor but worth noting.

resolveBroadcastChannelIndex return type

The method returns Promise<number>. The fallback path return meshPacket.channel !== undefined ? meshPacket.channel : 0 returns a number from the raw protobuf field. In protobuf-js, numeric fields on unset messages can be 0 (the protobuf default), so the !== undefined check and the 0 default are consistent — this is fine.

Potential Bugs

No issues found. The logic correctly handles all four cases:

1. Server-decrypted, device channel match → device slot (correct)
2. Server-decrypted, no device match → CHANNEL_DB_OFFSET + dbId (correct, fixes Position packets from secondary channels display as 'channel 39' instead of correct channel #3682)
3. Node-decrypted / no context → meshPacket.channel (preserved existing behavior)
4. meshPacket.channel === undefined → 0 (safe default)

The TELEMETRY_APP, PAXCOUNTER_APP, and NEIGHBORINFO_APP dispatch cases (lines 5549–5558) still do not thread decryptedBy/decryptedChannelId to their handlers. If those handlers also read meshPacket.channel for display, they could have the same latent issue on a secondary channel. This is out of scope for this PR but worth tracking.

Test Coverage

The 7 tests in meshtasticManager.positionChannel.test.ts are well-structured and cover the important cases:

• ✅ Raw-hash is not used when server-decrypted
• ✅ Device-channel preference (PSK + name match)
• ✅ No-context fallback (unencrypted/primary)
• ✅ Node-decrypted fallback
• ✅ meshPacket.channel === undefined default
• ✅ End-to-end processPositionMessageProtobuf for server-decrypted case
• ✅ End-to-end processPositionMessageProtobuf for unencrypted/primary case

One gap: there's no test asserting the positionChannel field on the upsertNode call (line 6286) also receives the resolved index rather than the raw hash. The existing tests check insertTelemetry rows (latitude/longitude) but upsertNode is separately called with positionChannel: channelIndex. Adding a upsertNode assertion (or at least a comment explaining it's covered by the same channelIndex variable) would close that coverage gap.

Performance

resolveBroadcastChannelIndex always awaits two DB calls (getByIdAsync + getAllChannels) when decryptedBy === 'server'. This matches the existing text-message path exactly, so no regression. The lookups are on small tables (channel database entries and device channels) and are already present in the text path, so this is acceptable.

Security

No concerns. The fix is purely a display/storage correctness issue. Channel decryption itself is unchanged; this only affects which slot index is stored after decryption succeeds.

Verdict

Approved with minor suggestions. The fix is correct, well-tested, and doesn't introduce regressions. The two suggestions above (refactor text path to call the shared helper; add a upsertNode assertion) are non-blocking follow-ups.