Claude finished @Yeraze's task —— View job

PR Review: feat(map): filter out Null Island (0,0) positions (#3763)

Review complete.

• Gather context and understand the request
• Read changed files
• Analyze code quality, bugs, performance, security, test coverage
• Post review feedback

Overall Assessment

This is a clean, well-scoped fix. The implementation is solid and follows the codebase conventions closely. The PR description accurately reflects the work done. A few observations below — most are minor, with one worth discussing.

Code Quality

src/utils/nullIsland.ts — The helper is well-named, well-documented, and handles null/undefined/NaN/Infinity edge cases. Using a radius rather than exact equality is the right call for firmware rounding.

src/components/Dashboard/DashboardMap.tsx (getNodeLatLng) and src/components/MapAnalysis/nodePositionUtil.ts (resolveNodeLatLng) — Both do the same thing: extract lat/lng from either the flat API shape or a nested position object, then reject Null Island. There is minor duplication between the two functions (same lat = node.latitude ?? node.position?.latitude logic). This is acceptable given the slight shape difference ({ lat, lng } vs [number, number]), and the PR description notes the intentional parallel. Not a blocker.

src/server/meshcoreManager.ts (persistContact) — The implementation is correct. The atNullIsland boolean gate before the upsertNode call is clear and avoids null-coalescing confusion.

Potential Issues / Observations

1. Telemetry table not cleaned up by migration 107

The PR description explicitly acknowledges this:

Position trails / coverage heatmap render historical telemetry; since the Meshtastic ingestion gate now also blocks (0,0) from the telemetry table, new history won't contain it either. Migration 107 covers the already-stored node rows.

However, existing telemetry rows with telemetryType = 'latitude' / 'longitude' carrying (0,0) values will still render in position trails and the coverage heatmap for existing deployments until those rows age out or are deleted. If the issue only concerns node markers (as stated in "Scope note"), this is fine. But if a user has been receiving many Null Island positions, trails and heatmaps may still show South Atlantic points for some time. This may be worth a follow-up issue, or at least a note in the scope section.

2. processPositionMessageProtobuf still has the pre-existing if (position.latitudeI && position.longitudeI) guard at line ~6130

This guard was noted in the PR as "only caught exactly 0,0" — but isValidPosition now catches near-zero too. The old guard is still there as the outer condition. Since latitudeI and longitudeI are integers (1e-7 degree scale), a near-zero value like 0.0005° would be 5000 (integer) and is non-zero, so it passes the outer if and then gets caught by isValidPosition. The guard combination is correct. However, it's slightly misleading to have the old comment if (position.latitudeI && position.longitudeI) still as a filter — it reads as "skip if missing" but is really "skip if exactly zero integer" in addition to isValidPosition. Not a bug, just a note.

3. Migration 107 SQLite uses db.exec() (raw SQL)

This is intentional and explicitly marked with the raw-SQL ESLint exception note in CLAUDE.md for migrations, so no issue here. The PG/MySQL variants correctly use parameterized or interpolated-constant queries. The EPS constant is injected via template literal — since it's a compile-time const number (not user input), there's no injection risk.

4. MeshCoreMap — positioned computed only from in-memory contacts

The PR correctly identifies that in-memory contacts bypass the DB store filter, and applies isNullIsland in the useMemo at line ~232-234. This is correct.

Test Coverage

Coverage is thorough:

• nullIsland.test.ts: unit tests covering exact (0,0), near-zero within epsilon, boundary at epsilon, real-world positions (including Greenwich meridian edge case), and null/undefined/NaN/Infinity.
• 107_clear_null_island_positions.test.ts: SQLite migration covering exact/near-zero nulling, legitimate position preservation, manual override preservation, and idempotency.
• meshtasticManager.positionChannel.test.ts: near-(0,0) position dropped without telemetry insertion; legitimate position stores correctly.
• meshcoreManager.contactPersistence.test.ts: (0,0) contact stores null lat/lon.

One gap: there's no test for resolveNodeLatLng or getNodeLatLng directly (they're utility/component-level functions). Given that isNullIsland itself is well-tested and these functions delegate to it, this is acceptable.

Security / Performance

No concerns. The isNullIsland check is O(1) and runs only on path boundaries (ingestion, rendering). No user input is used in the migration SQL template.

Summary

Verdict: LGTM with one point to consider. The primary question is whether existing telemetry rows with (0,0) lat/lon (position trails / heatmap) need cleanup in this PR or as a follow-up. If this is intentionally scoped to node markers only (as stated), the PR is ready to merge.

Thanks for the review. Triage of the points raised:

Point 1 (telemetry / trails + heatmap) — addressed in ce40063. This was the substantive one. Rather than only filtering at render, I handled both sides:

• analysis.getPositions() now skips Null Island pivots. That's the single render-side chokepoint — /coverage-grid reuses getPositions, so it covers position trails, the high-zoom heatmap, and the server-computed low-zoom heatmap cells (which a frontend-only filter can't reach).
• Migration 107 now also deletes the paired (0,0) latitude/longitude rows from telemetry, keyed on (nodeNum, timestamp) so a row is removed only when both axes of the same fix are near zero. Dialect-specific: SQLite/PG use a row-value IN subquery; MySQL uses a multi-table self-join delete to sidestep error 1093.

New tests cover paired deletion, unpaired-latitude preservation, the Greenwich edge case, idempotency, and the getPositions filter.

Points 2, 3, 4 — acknowledged, no change needed, matching your own assessment: the latitudeI && longitudeI guard + isValidPosition combination is correct (the old comment is slightly imprecise but not wrong); migration raw SQL is the sanctioned src/server/migrations/** exception and EPS is a compile-time const (no injection); and the MeshCoreMap in-memory filter is intentional.

Full Vitest suite green (7601 passed). Re-monitoring CI.

Claude finished @Yeraze's task —— View job

PR Review: feat(map): filter out Null Island (0,0) positions (#3763)

Review complete.

• Gather context and understand the request
• Read changed files
• Analyze code quality, bugs, performance, security, test coverage
• Post review feedback

Overall Assessment

This is a well-executed, defense-in-depth fix. The second commit (ce40063) meaningfully addressed the only substantive gap from the first review. All three surface areas are now handled — node markers, position history/trails, and the coverage heatmap — with both a runtime filter and a one-shot data migration. The implementation is clean, idiomatic, and the test coverage is thorough.

Updated Changes (ce40063)

src/db/repositories/analysis.ts — getPositions() pivot filter (analysis.ts:388)

The isNullIsland check is correctly placed inside the pivot loop, after lat and lon are matched but before the row is pushed. This is the right location because:

• It receives the actual floating-point values (not the integer latitudeI/longitudeI), matching isNullIsland's contract.
• It covers getCoverageGrid for free since that method pages through getPositions.
• The server-computed low-zoom heatmap (/coverage-grid) reuses this same path, so a frontend-only filter would have missed it.

One subtle behaviour to be aware of: the over-fetch strategy (pageSize + 1 rows per stream) combined with the post-pivot Null Island skip means a page heavily populated with (0,0) fixes could yield fewer than pageSize pivots even when hasMore was expected to be true. The loop breaks as soon as pivots.length > pageSize, so genuinely near-zero databases would cause under-filled pages. In practice, migration 107 removes those rows, and the ingestion gate prevents new ones, so the edge case won't affect real deployments. Worth noting in case the pagination behavior is ever revisited.

src/server/migrations/107_clear_null_island_positions.ts — telemetry cleanup

The paired-deletion approach is correct for all three dialects:

• SQLite/PostgreSQL: Row-value (nodeNum, timestamp) IN (subquery) correctly ensures only rows where both lat and lon of the same fix are near-zero are deleted. The outer WHERE ABS(value) < EPS is a useful early filter before the join.
• MySQL: Multi-table DELETE lat, lon FROM ... JOIN ... is the right workaround for MySQL error 1093. The join condition and predicate are equivalent to the SQLite/PG path. ✓

One minor observation on the SQLite variant: the outer WHERE telemetryType IN ('latitude','longitude') AND ABS(value) < EPS is evaluated before the subquery, which means any longitude row whose value is outside EPS will not be targeted by the outer DELETE even if its paired latitude is near-zero (the subquery finds paired near-zero lat+lon, and the outer WHERE restricts which rows actually get deleted). For a (0.0004, -35.0) pair: latitude is near-zero but longitude is not, so the outer WHERE ABS(value) < EPS filter on the longitude row would exclude it — but the subquery wouldn't have matched that pair either (longitude ABS(-35.0) < EPS is false in the subquery). The semantics are consistent; this is purely an observation, not a bug.

src/db/repositories/analysis.test.ts — new Null Island test (analysis.test.ts:76–93)

Good: uses 0.0004 and -0.0002 (within epsilon), confirms the fix survives and the Null Island fix is absent from results. The test is straightforward and doesn't over-specify internal pagination behaviour.

Prior Points — Status

Remaining Minor Observations (no action needed)

getNodeLatLng (DashboardMap) vs resolveNodeLatLng (nodePositionUtil) — Minor duplication of the node.latitude ?? node.position?.latitude extraction pattern noted in the first review is still present and remains acceptable given the different return shapes ({lat,lng} vs [number,number]).

isValidPosition redundancy (meshtasticManager.ts:6130) — The outer if (position.latitudeI && position.longitudeI) guard and the inner isValidPosition check are both still present. The outer guard catches the integer 0 exactly, while isValidPosition catches near-zero float values after conversion. The combination is correct; the slightly imprecise comment (// Check for valid numbers) is a pre-existing issue and doesn't affect correctness.

Altitude/speed orphans (migration 107) — The migration comment correctly notes that altitude/speed rows of a cleaned fix become harmless orphans (the render layer requires both lat and lon to plot). This is acceptable since the getPositions pivot already skips unpaired lat rows, so orphaned altitude rows are harmlessly ignored.

Security / Performance

No concerns. EPS is a compile-time constant — no injection risk in migration SQL. isNullIsland is O(1). The migration telemetry subquery does a self-join on (nodeNum, timestamp); with an index on those columns this is efficient; without one it's a full-table scan that runs exactly once on upgrade. No security issues.

Test Coverage Summary

Coverage is comprehensive. The one remaining gap (no direct test of getNodeLatLng / resolveNodeLatLng) is acceptable since isNullIsland is independently well-tested and these are thin wrappers.

Verdict: LGTM. The PR is ready to merge. Both the original node-marker fix and the follow-up telemetry/trail cleanup are well-implemented, tested across all three DB backends, and guarded at every relevant chokepoint.

View job

Thanks — noting the one non-blocking observation for the record: the over-fetch (pageSize + 1) combined with the post-pivot Null Island skip can under-fill a page when a window is dense with (0,0) fixes. Leaving pagination as-is deliberately: migration 107 purges the historical rows and the ingestion gate blocks new ones, so a near-zero-dense window can't occur in a migrated deployment — and adding a re-fetch loop to top up pages would add real complexity for a case that can't arise. Flagging it here in case the pagination path is ever revisited independently. The SQLite outer-WHERE semantics and the noted duplication/redundancy are all consistent/intentional as you called out — no changes there.