Claude finished @Yeraze's task —— View job

PR Review: fix(position): ground_speed is km/h, not m/s

• Read changed files
• Analyze code quality and logic
• Check test coverage
• Post review feedback

This is a clean, well-researched fix. The root cause analysis (tracing to GPS.cpp line 1824 and reader.speed.kmph()) is compelling, and the PR correctly identifies that the previous ×3.6 multiplier was inverting the wrong assumption. Below is my review.

Core fix — src/utils/speedConversion.ts

Excellent. The new implementation is minimal and correct:

export function convertSpeed(kilometersPerHour: number, distanceUnit: string): { speed: number; unit: string } {
  const speed = distanceUnit === 'mi' ? kilometersPerHour * 0.621371 : kilometersPerHour;
  const unit = distanceUnit === 'mi' ? 'mph' : 'km/h';
  return { speed: parseFloat(speed.toFixed(1)), unit };
}

Minor observation: The parameter is now named kilometersPerHour, which is semantically accurate and self-documenting — no issues here. The 0.621371 constant is the standard km/h→mph conversion factor.

Potential edge case (low priority): convertSpeed doesn't guard against negative values. ground_speed should always be ≥ 0, but if a corrupted packet delivers a negative value, the function will return a negative speed. The existing groundSpeed > 0 guards in meshtasticManager.ts (lines 6245, 8242) already filter out zeroes before storage, so this isn't a bug in the current flow — just something to be aware of if convertSpeed is ever called from another path.

src/server/meshtasticManager.ts (lines 6243, 8245)

Both telemetry storage sites correctly update the unit label to 'km/h'. The comment at line 6242–6243 is clear and accurate. No issues.

src/utils/mapHelpers.tsx (line 447–448) and src/contexts/MapContext.tsx (line 11)

Comments and interface JSDoc are updated to reflect the correct unit. The PositionHistoryItem.groundSpeed field is now documented as // km/h (firmware emits ground_speed via TinyGPS++ .kmph(); see #3797). Good.

One minor note: generateHeadingAwarePath at line 327 still has @param speed Ground speed in m/s (affects control point distance) in its JSDoc, which wasn't changed in this PR. That parameter comment is now misleading since groundSpeed passed in is in km/h. Worth a follow-up (not a blocker).

src/pages/UnifiedTelemetryPage.tsx (line 207)

TYPE_UNITS['ground_speed'] changed from 'm/s' to 'km/h'. Correct — this is the display-only unit label on the telemetry card, and the stored value (the raw wire km/h) now has an accurate label. No issues.

src/utils/mapHelpers.test.tsx — test coverage

The convertSpeed suite is well-written:

• Pass-through for metric ✓
• km/h→mph conversion with tolerance ✓
• Zero speed ✓
• Regression test explicitly naming #3797 and asserting 90 → 90 ✓
• High speed (highway) ✓
• Walking speed ✓
• Metric/imperial consistency ✓

One suggestion: Consider adding a test for convertSpeed with an unrecognized distanceUnit value (e.g., 'ft' or an empty string). Currently it falls through to the kilometersPerHour pass-through (since the only branch is === 'mi'), which is fine, but an explicit test would document that contract.

No migration needed — correct

Existing ground_speed rows already store the raw wire value (km/h). Since the buggy ×3.6 was applied at display time (in convertSpeed), the stored values are correct and no backfill is required. The PR's reasoning here is sound.

Summary

Recommend merging. The JSDoc @param speed in generateHeadingAwarePath is a nit worth fixing in a follow-up; it doesn't affect behavior.

Claude finished @Yeraze's task —— View job

PR Review: fix(position): ground_speed is km/h, not m/s

• Read changed files
• Analyze code quality and logic
• Check test coverage
• Post review feedback

This is a well-researched, clean fix. The root cause analysis (tracing to GPS.cpp line 1824 and reader.speed.kmph()) is compelling and the previous ×3.6 multiplier was inverting the wrong assumption. The JSDoc nit from the previous review has been resolved — generateHeadingAwarePath at line 327 now correctly says "Ground speed in km/h (affects control point distance; see #3797)".

Core fix — src/utils/speedConversion.ts

Correct and minimal. The implementation is self-documenting and the parameter rename to kilometersPerHour makes the contract explicit:

export function convertSpeed(kilometersPerHour: number, distanceUnit: string): { speed: number; unit: string } {
  const speed = distanceUnit === 'mi' ? kilometersPerHour * 0.621371 : kilometersPerHour;
  const unit = distanceUnit === 'mi' ? 'mph' : 'km/h';
  return { speed: parseFloat(speed.toFixed(1)), unit };
}

Minor observation (non-blocking): convertSpeed has no guard against negative values. ground_speed should always be ≥ 0 from the firmware, and the existing groundSpeed > 0 checks in meshtasticManager.ts filter out zeroes at storage time. No action needed in this PR — just noting it for future callers.

src/server/meshtasticManager.ts

Both telemetry storage sites correctly update the unit label to 'km/h'. No issues.

src/contexts/MapContext.tsx (line 11) and src/utils/mapHelpers.tsx (line 327)

• PositionHistoryItem.groundSpeed JSDoc updated: // km/h (firmware emits ground_speed via TinyGPS++ .kmph(); see #3797) ✅
• generateHeadingAwarePath @param speed updated: Ground speed in km/h (affects control point distance; see #3797) ✅

The JSDoc nit flagged in the previous review is now resolved. No issues.

src/pages/UnifiedTelemetryPage.tsx

TYPE_UNITS['ground_speed'] changed from 'm/s' to 'km/h'. Correct — the stored value was always km/h; this just fixes the display label. No issues.

Test coverage — src/utils/mapHelpers.test.tsx

The convertSpeed suite (lines 317–366) is thorough:

One optional addition (non-blocking): An explicit test for an unrecognized distanceUnit (e.g. '' or 'ft') would document that it falls through to the metric pass-through. The behavior is correct today since the only branch is === 'mi', but a test would pin the contract.

No migration needed — correct

The bug was entirely at display time: convertSpeed applied ×3.6 at render. Stored values are the raw wire km/h values and are correct. No backfill required.

Summary

Recommend merging. All previous review nits have been addressed.