Claude finished @Yeraze's task —— View job

PR Review: fix(telemetry): unify serial ingest onto shared digit-aware normalizer

Tasks:

• Gather context
• Read changed files
• Review code quality, bugs, performance, security
• Review test coverage
• Post final review

Overall Assessment

This is a well-executed, focused refactor. The motivation is clear — protobuf.js's underscore-before-digit quirk silently dropped fields, and the fix eliminates the category of bug rather than patching individual fields. The code is clean, the reduction of 117 lines is meaningful, and the existing test suite protects against regressions.

Code Quality

buildCanonicalMetrics helper (meshtasticManager.ts:1127): Nicely designed. The typeof raw !== 'number' || !Number.isFinite(raw) guard correctly handles protobuf.js's repeated fields (exposed as []), Long/object values, and non-finite numbers in a single check.

Consistency gap — localStats / hostMetrics / trafficManagementStats (meshtasticManager.ts:6514–6560): These three branches were not converted and still use inline hand-maintained lists with the old pattern. They don't currently suffer the underscore-before-digit problem, but they're inconsistent with the four converted branches. This isn't a bug for now, but worth noting for a follow-up — or at minimum, adding the relevant type maps to CANONICAL_TELEMETRY_UNITS so they could be converted later.

Type mismatch — minor: buildCanonicalMetrics returns Array<{ type, value: number, unit: string }>, but saveTelemetryMetrics accepts Array<{ type, value: number | undefined, unit: string }> (meshtasticManager.ts:1083). This is safely widened (the caller's narrower type is a subtype), but the signature asymmetry is worth keeping in mind if saveTelemetryMetrics is ever refactored.

deviceMetrics nodeData update (meshtasticManager.ts:6437–6440): The nodeData.batteryLevel = deviceMetrics.batteryLevel etc. assignments are now redundant with what buildCanonicalMetrics extracts. The explicit copies are still needed for the upsertNode call, so this is intentional, but a comment acknowledging the dual path would help future readers understand why the fields appear "twice."

Potential Issues

Number.isFinite and protobuf.js Long values: The guard typeof raw !== 'number' correctly rejects Long objects. However, protobuf.js may also return 0 as a Long for unset int64/uint64 fields. If any metric group ever contains a Long-typed field (none of the four targeted groups do currently), it would be silently skipped. This is acceptable given the current schema.

Missing validation of metrics parameter shape: buildCanonicalMetrics accepts Record<string, unknown> but the four call sites pass raw decoded protobuf sub-messages (any). The Object.entries iteration is safe, but if a decoded sub-message is null or undefined (e.g., protobuf.js returns a falsy value for an empty message), the caller guard (if (telemetry.deviceMetrics)) is the only protection. This pattern was already present before this PR.

Test Coverage

The new meshtasticManager.telemetryCanonical.test.ts is solid:

• ✅ Environment leaf renames verified (relativeHumidity→humidity, barometricPressure→pressure, voltage→envVoltage, current→envCurrent)
• ✅ Genuine 0 preserved (battery at 0% — the classic false-falsy trap)
• ✅ Repeated/array fields skipped (oneWireTemperature: [])
• ✅ Unknown future fields skipped (someFutureUntrackedField: 999)
• ✅ Power channels through the shared path

Gaps worth considering:

• No test for the underscore-before-digit quirk specifically (the root cause of fix(telemetry): collect air-quality particle counts (protobuf.js snake_case quirk) #3483/Tech debt: unify serial telemetry ingest onto the shared digit-aware normalizer (prevents #3483 recurrence) #3506). A test passing particles_03um (snake_case, as protobuf.js would decode it) and asserting it maps to particles03um and gets stored would directly pin the fix. The existing meshtasticManager.airQualityParticles.test.ts may cover this — worth verifying.
• No test for NaN / Infinity inputs being excluded.
• No test verifying the localChannelUtilization side-effect for the local node's device metrics (pre-existing coverage, but the refactor touches that branch).

Minor Nits

• The debug log at meshtasticManager.ts:6462 still references envMetrics.relativeHumidity by raw protobuf name, which is fine for debugging but slightly inconsistent with the "we iterate decoded fields" framing. No action needed.
• The MQTT_KEY_MIGRATIONS list in telemetryKeys.ts:143 maintains a separate environmentProtobufLeaves array that could drift if new env fields are added to ENVIRONMENT_LEAF_RENAMES but not the migration list. Low risk since Coverage: wire up newer AirQualityMetrics fields (particles_40um and adjacent) #3507 will presumably extend CANONICAL_TELEMETRY_UNITS and this list together, but worth a comment.

Summary

Approve with minor notes. The core change is correct and well-tested. The inconsistency between the four converted metric groups and the three hand-maintained localStats/hostMetrics/trafficManagementStats branches is the main thing to track — could be addressed in a follow-up issue. No blocking concerns.

Claude finished @Yeraze's task —— View job

PR Review: fix(telemetry): unify serial ingest onto shared digit-aware normalizer

Tasks:

• Gather context
• Read changed files
• Review code quality, bugs, performance, security
• Review test coverage
• Post final review

Overall Assessment

This is a well-targeted refactor that eliminates a whole class of silent data-loss bugs. The buildCanonicalMetrics helper is clean and correct. The review feedback from the initial review has been partially addressed — the NaN/Infinity guard test was added, and the dual-path comment explaining the nodeData + buildCanonicalMetrics "duplication" was added. Here's the state of remaining items.

What was addressed since the initial review

NaN/Infinity test added ✅ — meshtasticManager.telemetryCanonical.test.ts:108-121 covers NaN, +Infinity, and -Infinity inputs and verifies they're excluded by the Number.isFinite guard.

Dual-path comment added ✅ — meshtasticManager.ts:6437-6440 now has a clear comment explaining why nodeData.batteryLevel = deviceMetrics.batteryLevel et al. exist alongside buildCanonicalMetrics. Future readers won't be confused by the apparent duplication.

Remaining observations

localStats / hostMetrics / trafficManagementStats still use inline hand-maintained lists (meshtasticManager.ts:6513-6564). These three branches pass value: localStats.foo directly without the Number.isFinite guard that saveTelemetryMetrics provides — wait, saveTelemetryMetrics does filter with !isNaN(Number(metric.value)) at line 1092. However, the inline lists pass undefined for absent fields (e.g. noiseFloor when the firmware is old), which is handled. The inconsistency with the four converted branches is cosmetic for now since none of these fields have underscore-before-digit names. Still worth tracking as a follow-up.

MQTT_KEY_MIGRATIONS drift risk (telemetryKeys.ts:151-159): The environmentProtobufLeaves array is a static list that could drift from ENVIRONMENT_LEAF_RENAMES and ENVIRONMENT_UNITS. For example, if a new leaf is added to ENVIRONMENT_UNITS but forgotten in environmentProtobufLeaves, MQTT migration won't rewrite existing stored rows. This is a pre-existing risk not introduced by this PR, but worth a follow-up issue.

Type signature asymmetry (minor): buildCanonicalMetrics returns Array<{ value: number }> (strictly number), while saveTelemetryMetrics accepts Array<{ value: number | undefined }>. This is a safe subtype relationship and causes no runtime issue, but if saveTelemetryMetrics is ever tightened to require number, the three inline-list callers that pass undefined would break before the buildCanonicalMetrics callers.

snakeToCamel at telemetryKeys.ts:106: The regex /_([a-z0-9])/g correctly handles the digit case (_03um → 03um uppercased, yielding 03um since 0 has no uppercase — which is intentional). Worth noting this works because digit "uppercase" is identity, not a bug.

Test coverage assessment

The two test files together give strong coverage:

One remaining gap: No test covers localChannelUtilization side-effect — i.e., when a deviceMetrics packet arrives from the local node (from === localNodeInfo.nodeNum), this.localChannelUtilization is updated. The refactor did not change that branch's logic, so this is pre-existing coverage debt, not a regression.

Summary

Approve. All blocking concerns from the initial review have been addressed. The remaining items (three unconverted branches, MQTT migration list drift, localChannelUtilization test) are pre-existing or cosmetic and suitable for follow-up issues. The core fix is correct, well-tested, and durable.