Claude finished @Yeraze's task —— View job

PR Review — fix(position): surface per-fix SNR/hops in position history (#3590)

• Gather context and understand the request
• Read changed files
• Review code quality and best practices
• Check test coverage
• Post final review

This is a clean, well-scoped bug fix. The root-cause analysis in the PR description is accurate and complete. A few notes:

Strengths

Extraction of pivotPositionHistory() (positionHistoryPivot.ts) is the right call — the grouping logic was previously inline in the route handler and is now testable in isolation. The interface definitions (PivotTelemetryRow, PivotedPosition) are appropriately minimal, and the sentinel constant NO_SNR_SENTINEL = -128 is exported (reusable and testable by name rather than magic number).

The sentinel check fix (rawPosRxSnr != null && rawPosRxSnr !== -128 instead of the old truthiness guard) is correct. The PR description accurately explains why rxSnr && rxSnr !== 0 is wrong for 0 dB.

Test coverage (10 cases in positionHistoryPivot.test.ts) is thorough for the new helper: the 0 dB preservation test directly targets the regression, sentinel stripping, null handling, key-omission, sorting, incomplete-fix filtering, and metadata sourced from either lat or lon row are all covered.

Issues Found

1. Inconsistent fix: snr_local telemetry at line 6418 still uses the old truthiness guard

src/server/meshtasticManager.ts:6418:

// OLD pattern — still present in processMeshPacket for snr_local telemetry
if (meshPacket.rxSnr && meshPacket.rxSnr !== 0) {
  nodeData.snr = meshPacket.rxSnr;
  // ... also inserts snr_local telemetry row

This is the snr_local telemetry path inside processMeshPacket (the handler for all packet types, not just position packets). It will still silently drop a 0 dB SNR when recording the snr_local telemetry series (shown in the SNR telemetry chart). The fix applied to the position handler (processPositionMessageProtobuf) and the node upsert within it is correct, but this parallel path is still broken for the same class of SNR. Whether or not this affects the symptom described in issue #3590, it's a latent inconsistency.

2. Minor: altitude and optional fields are not omitted when absent (minor shape inconsistency)

positionHistoryPivot.ts:101-112: altitude, groundSpeed, groundTrack are spread unconditionally (they will appear as undefined keys in the output object if absent), while snr/hopStart/hopLimit are correctly omitted via ...(pos.x !== undefined && { x: pos.x }). This is cosmetically inconsistent with the SNR/hops handling but unlikely to affect runtime behavior since undefined JSON keys are dropped by JSON.stringify. Not a bug — just noting the inconsistency for future maintainability.

3. sinceTimestamp: 0 is passed for "no cutoff" instead of undefined

src/server/server.ts:1326-1344:

const cutoffTime = hoursParam ? Date.now() - hoursParam * 60 * 60 * 1000 : 0;
// ...
const positionTelemetry = await databaseService.getPositionTelemetryByNodeAsync(nodeId, 1500, cutoffTime);

cutoffTime = 0 is passed when no hours param is given. The repository uses gte(telemetry.timestamp, sinceTimestamp) when defined. For SQLite this is benign (all timestamps are > 0). However, in the repo signature sinceTimestamp?: number, passing 0 is not the same as omitting it — rows at exactly timestamp 0 would be excluded. This is pre-existing behavior and not introduced by this PR, but it's worth noting if the position history route is being touched anyway.

Summary

The core bug fix is correct and the new pivotPositionHistory helper is a solid improvement. The primary concern is the remaining snr_local truthiness guard at line 6418 which will continue to drop 0 dB SNR for the node's SNR telemetry history chart even after this PR lands. Everything else is minor or pre-existing.

Claude finished @Yeraze's task —— View job

PR Review — fix(position): surface per-fix SNR/hops in position history (#3590)

• Gather context and understand the request
• Read changed files
• Review code quality and best practices
• Check test coverage
• Post final review

This is a well-scoped, two-commit fix that correctly addresses both root causes described in the PR. The prior automated review raised several concerns — here is an updated assessment after reading the actual code on the branch.

What was previously flagged vs. what's actually there

Issue 1 (previous review): snr_local truthiness guard at line 6418 — RESOLVED in this PR.

The second commit (0801981) already fixed this. meshtasticManager.ts:6420 now reads:

if (meshPacket.rxSnr != null && meshPacket.rxSnr !== -128) {

with an explanatory comment referencing #3590. The old rxSnr && rxSnr !== 0 guard is gone. Both the snr_local telemetry path and the position handler now use the correct sentinel check.

Issue 2 (previous review): Optional fields not omitted when absent — still present, but truly cosmetic. altitude, groundSpeed, groundTrack spread as undefined in the output object (lines 105–107 of positionHistoryPivot.ts) while snr/hopStart/hopLimit use the conditional spread pattern. Since undefined keys are stripped by JSON.stringify, this has no runtime impact. Noted only for consistency.

Issue 3 (previous review): sinceTimestamp: 0 — pre-existing; not introduced here.

Remaining minor observations

1. {SNR} auto-responder template token at line 11708 still uses a 0-check:

const snrValue = (rxSnr !== undefined && rxSnr !== null && rxSnr !== 0)
  ? rxSnr.toFixed(1)
  : 'N/A';

This is in the auto-responder message templating path, not in the telemetry write path, so it is unrelated to the bug being fixed. However it will display N/A instead of 0.0 for a 0 dB SNR in auto-responder messages. This is not a regression introduced by this PR (it was pre-existing), but it's a third location with the same pattern that was missed.

2. rx_snr / rx_start snake_case fallbacks in processPositionMessageProtobuf:

const rawPosRxSnr = meshPacket.rxSnr ?? (meshPacket as any).rx_snr ?? undefined;
const posHopStart = meshPacket.hopStart ?? (meshPacket as any).hop_start ?? undefined;

These snake_case fallbacks suggest some callers pass un-camelCased protobuf objects. The getPositionTelemetryByNodeAsync return type should already be camelCased from Drizzle, so these are defensive guards against raw protobuf. That's fine, just slightly surprising — worth a brief comment if not already covered.

3. getPositionTelemetryByNodeAsync signature for sinceTimestamp = 0 (pre-existing, not introduced here): If the repository filters with gte(telemetry.timestamp, sinceTimestamp) when sinceTimestamp is defined, passing 0 is subtly different from omitting it (excludes any row at exact epoch 0, which is impossible in practice). No action needed in this PR.

Positive notes

• pivotPositionHistory() extraction is the right architectural call. The pivot logic is now isolated, pure, and independently testable — a significant improvement over inline route handler code.
• Test suite is thorough: the 0 dB regression test directly exercises the exact failure mode. Sentinel stripping, null/undefined handling, key-omission, sort order, incomplete-fix filtering, and metadata from either row are all covered. 10 cases for a 116-line utility is good density.
• NO_SNR_SENTINEL exported from the pivot module rather than scattered as a magic number — the constant is reusable and the test references it by name.
• Both affected paths (position write + snr_local write) are now consistently using != null && !== -128 as intended.

Summary

The core fix is correct and complete. The previous automated review's primary concern (the snr_local guard) was already addressed by the second commit before this review ran. The remaining observations are either cosmetic, pre-existing, or out of scope for this PR. No blocking issues.