Claude finished @Yeraze's task —— View job

PR Review

• Gather context and understand the PR
• Review migrations (089, 090)
• Review schema changes
• Review repository changes
• Review frontend changes (mapHelpers, MapContext, NodesTab)
• Review API route changes
• Review test coverage
• Post comprehensive feedback

Overall this is a well-constructed PR. The architecture is sound, the migrations are idempotent across all three backends, and the feature is cleanly decomposed. A few things worth discussing before merge:

Potential Issues

1. positionMap grouping key: timestamp collisions

File: src/server/routes/v1/positionHistory.ts:117–160

The route groups telemetry rows by t.timestamp. A node that sends two position fixes within the same second (same millisecond timestamp) will collide into one map entry. Only the first lat/lon values are kept, and the second fix is silently dropped. The PR description doesn't indicate this is new behavior — I'd want to confirm the previous code had the same characteristic and this PR didn't regress it. If the prior code also grouped by timestamp, the existing bug isn't introduced here, but it may be worth a follow-up.

2. heardDirectly logic: hops === 0 is true when SNR is unknown

File: src/utils/mapHelpers.tsx:469–470

const heardDirectly = hops === 0;
const showSnr = heardDirectly && item.snr != null;

hops = Math.max(0, hopStart - hopLimit) can legitimately return 0 if both fields are zero (e.g. a device running old firmware that doesn't populate them), not only when hopStart === hopLimit. In that edge case, heardDirectly is true even though we can't verify the fix was actually heard directly. However, showSnr still requires item.snr != null, so the practical effect is only that the tooltip would say "Heard directly (0 hops)" for a node where both values happened to be default-zero — which is a mild display inaccuracy rather than a security concern.

Consider adding a guard: only compute hops when hopStart > 0 || hopLimit > 0 (or match the existing hopStart > 0 guard already used elsewhere in meshtasticManager.ts:5378).

3. Migration 089 SQLite down silently eats all errors

File: src/server/migrations/089_add_position_snr_hops.ts:40

try { db.exec(`ALTER TABLE ${TABLE} DROP COLUMN ${col.name}`); } catch { /* older SQLite lacks DROP COLUMN */ }

The down implementation swallows any error unconditionally. If there's a real schema issue (wrong table name, etc.) the rollback would silently no-op. In practice down is rarely called on this project, but the comment that masks the error is slightly misleading — "older SQLite" is only one possible error source. Low severity, but consider logging.

4. before filter applied in-memory but not to the DB query

File: src/server/routes/v1/positionHistory.ts:131–132

if (beforeTimestamp !== undefined && t.timestamp >= beforeTimestamp) return;

The before filter is applied after fetching from the database. For large datasets this means the DB may return rows that are immediately discarded. The internalLimit calculation ((offsetNum + maxLimit) * 5) can also over-count if many rows are filtered out by before. This is a pre-existing characteristic (not introduced by this PR), but the new SNR/hop metadata capture makes the query slightly heavier per row, so the inefficiency compounds slightly.

Minor Notes

packetId assignment: src/server/routes/v1/positionHistory.ts:151

pos.packetId = t.packetId ?? undefined;

t.packetId is number | null | undefined. If t.packetId is null, ?? undefined evaluates to undefined, and then the if (t.packetId != null) guard is never entered with a null value anyway — so the ?? undefined on line 151 is redundant but harmless.

SNR tooltip in hover vs popup: The hover <Tooltip> shows {dateStr} {timeStr} on one line (space-separated). On narrow map tiles this might render awkwardly, but this is a style preference rather than a bug.

showPolarGrid not persisted to server: MapContext.tsx:232–235 — setShowPolarGrid calls savePreferenceToServer({ showPolarGrid: value }) but there's no positionHistoryPointsOnly column in the schema for showPolarGrid. This appears to be pre-existing; the new positionHistoryPointsOnly setter at line 211–214 correctly calls savePreferenceToServer. No issue with the new code.

Code Quality

Migrations: Both 089 and 090 are well-structured — idempotent SQLite (try/catch on duplicate column), IF NOT EXISTS for PostgreSQL, information_schema check for MySQL. The eslint-disable-next-line comment in migration 090 for raw DDL is properly annotated.

Schema consistency: The new columns follow existing conventions — rxSnr as REAL/doublePrecision/DOUBLE, integer hop fields using the same int types as channel and precisionBits. The sourceId column ordering (at the end) matches the pattern.

Frontend: The fix in generatePositionHistoryArrows is clean — always emit a CircleMarker dot, then conditionally overlay the non-interactive Marker triangle. The interactive={false} prop on the triangle ensures hover/click hit-testing falls through to the dot.

Test coverage: Good coverage of the new behavior:

• mapHelpers.test.tsx: dot at every fix, triangle only when heading, cap, empty
• misc.mapprefs.test.ts: column mapping for all three backends
• telemetry.extra.test.ts: round-trip SNR/hop capture via getPositionTelemetryByNode
• migrations.test.ts: count and last migration name updated

Summary

The PR is functionally correct and well-tested. The main item worth addressing before merge is item #2 (the heardDirectly edge case with both-zero hop values showing an inaccurate "Heard directly" label). Items #1, #3, and #4 are pre-existing or low-severity and can reasonably be follow-up issues.

Claude finished @Yeraze's task —— View job

PR Review

• Gather context and understand the PR
• Review migrations (089, 090)
• Review schema changes
• Review repository and service changes
• Review frontend changes (mapHelpers, MapContext, NodesTab)
• Review API route changes
• Review test coverage
• Post comprehensive feedback

Overall this is a well-structured PR. The architecture is clean, the migrations are idempotent across all three backends, and the three features (bug fix + points-only mode + hover tooltip) are well-decomposed. Here's the full review:

Potential Issues

1. heardDirectly false positive when both hop fields are default-zero

File: src/utils/mapHelpers.tsx:466–470

if (item.hopStart != null && item.hopLimit != null) {
  hops = Math.max(0, item.hopStart - item.hopLimit);
}
const heardDirectly = hops === 0;
const showSnr = heardDirectly && item.snr != null;

hops === 0 is true both when hopStart === hopLimit (legitimately heard directly) and when both fields are 0 due to old firmware not populating them. The existing message-hops ingestion in meshtasticManager.ts:5378 guards against this with hopStart > 0, but the display path here has no such guard.

In practice, showSnr still requires item.snr != null, which limits the damage to the tooltip saying "Heard directly (0 hops)" for old-firmware nodes that happen to have an SNR stored. A minimal fix consistent with the rest of the codebase:

if (item.hopStart != null && item.hopLimit != null && (item.hopStart > 0 || item.hopLimit > 0)) {
  hops = Math.max(0, item.hopStart - item.hopLimit);
}

2. Redundant ?? undefined on line 151

File: src/server/routes/v1/positionHistory.ts:150–151

if (t.packetId != null && pos.packetId === undefined) {
  pos.packetId = t.packetId ?? undefined;
}

The if guard already ensures t.packetId is non-null before entering the block, so ?? undefined inside is a no-op. Minor but slightly misleading — t.packetId alone suffices here.

3. Migration 089 down swallows all errors unconditionally

File: src/server/migrations/089_add_position_snr_hops.ts:40

try { db.exec(`ALTER TABLE ${TABLE} DROP COLUMN ${col.name}`); } catch { /* older SQLite lacks DROP COLUMN */ }

The catch comment is accurate for the intended case but the empty handler also silences real errors (wrong table name, database locked, etc.). Migration 090's down is better — it explicitly logs and does nothing rather than attempting and silently failing. Low severity since down is rarely invoked, but worth noting.

4. before filter applied in-memory, not pushed to the DB query

File: src/server/routes/v1/positionHistory.ts:131

The before timestamp filter is applied after the DB fetch. The internalLimit calculation ((offsetNum + maxLimit) * 5) may not fetch enough rows if a large fraction falls after beforeTimestamp, causing silent under-counting in total. This is pre-existing behaviour, not introduced here, but the new per-row SNR/hop metadata makes the query slightly heavier, so the trade-off is worth noting for a future follow-up.

Code Quality

Migrations (089, 090): Both are idempotent. Migration 089 follows the try/catch-on-duplicate column pattern for SQLite, IF NOT EXISTS for PostgreSQL, and information_schema check for MySQL. Migration 090 does the same plus correctly logs the skip. The eslint-disable-next-line annotations are properly placed.

Schema: New columns follow existing conventions — rxSnr as REAL/doublePrecision/DOUBLE, integer hop fields matching the channel/precisionBits types, sourceId at the end. All three backend schemas are in sync.

Ingest (meshtasticManager.ts:5971–5988): SNR/hop capture is extracted cleanly before the two insertTelemetry calls and stored on both latitude and longitude rows. Using ?? undefined instead of ?? null is intentional — insertTelemetry skips the column entirely when the value is null | undefined (lines 114–116 of telemetry.ts), avoiding writes on old-firmware packets.

generatePositionHistoryArrows: The fix is clean — always emit a CircleMarker dot, then conditionally overlay a non-interactive Marker triangle. interactive={false} correctly lets hover/click fall through to the dot. The step-based cap logic correctly tracks rendered points (not React elements), so dots + triangles from the same fix don't both count against maxArrows.

positionHistoryPointsOnly toggle: The continue at NodesTab.tsx:550 skips only the Polyline push, not the segmentColors accumulation or the subsequent generatePositionHistoryArrows call, which is exactly correct — dots still need their color array.

MapContext.tsx: setPositionHistoryPointsOnly calls savePreferenceToServer exactly like all other preference setters. The schema column naming (camelCase in Drizzle, snake_case in the SQLite DDL, snake_case in the PG/MySQL migration) is consistent with the existing pattern.

Test Coverage

• mapHelpers.test.tsx: covers dot-at-every-fix, triangle-only-on-heading, maxArrows cap, and empty input — solid.
• misc.mapprefs.test.ts: column mapping for all three backends.
• telemetry.extra.test.ts: round-trip SNR/hop capture via getPositionTelemetryByNode.
• migrations.test.ts: count = 90, last = add_position_points_only_to_map_prefs.

One gap: no test explicitly covers the heardDirectly = hops === 0 branch with SNR present (both in the tooltip display and in the showSnr guard). A test case with hopStart: 2, hopLimit: 2, snr: 5 (direct) vs hopStart: 3, hopLimit: 2, snr: 5 (relayed) would pin the tooltip behaviour and catch the edge case in issue #1 above.

Summary

The PR is functionally correct and well-tested. The only item I'd recommend addressing before merge is issue #1 (the heardDirectly false positive for default-zero hop fields). It's a one-line fix and aligns the display path with the same guard already used in the ingest path. Items #2 and #3 are cosmetic/low-severity; item #4 is pre-existing.