Claude finished @Yeraze's task —— View job

PR Review: fix(map): render every position fix + paginate full history

• Gather context and read changed files
• Review backend changes (telemetry repo, server routes)
• Review frontend changes (App.tsx pagination, mapHelpers.tsx)
• Review test coverage
• Post final review

Good fix for a real user-facing bug. The logic is correct and the test coverage is solid. A few things worth examining before merge:

Issues / Bugs

1. getPositionTelemetryByNodeAsync returns a non-Promise from a sync path on SQLite (src/services/database.ts:3919)

// @deprecated facade
async getPositionTelemetryByNodeAsync(...): Promise<DbTelemetry[]> {
  return this.telemetry.getPositionTelemetryByNode(...)  // ← returns Promise<DbTelemetry[]> (async repo method)
}

The underlying TelemetryRepository.getPositionTelemetryByNode is async and returns Promise<DbTelemetry[]>, so wrapping it in as unknown as Promise<DbTelemetry[]> is hiding a real double-awaiting issue. Fortunately it works in practice because await Promise<Promise<T>> auto-flattens, but the as unknown as cast is a code smell. This pre-existed the PR, but the PR now wires a new beforeTimestamp arg through this path — worth noting.

2. parseInt truncates float cursor values (src/server/server.ts:1340)

The before query param is parsed with parseInt, so a client sending before=1719412345678.5 will silently truncate to the integer. This is consistent with how hours is handled and probably fine in practice (timestamps are integers), but using Number(req.query.before) would be more semantically correct since parseInt stops at the first non-digit and can silently accept values like "123abc".

3. No-progress guard has an off-by-one edge case (src/App.tsx:1568)

if (beforeCursor !== undefined && oldest >= beforeCursor) break; // no-progress guard

The cursor for the next page is set to oldest (the first item in pageItems, which is the oldest fix in that page). If two consecutive pages both return a page where the oldest timestamp equals beforeCursor, this correctly breaks. However: the cursor is pageItems[0].timestamp (line 1567), and the server returns fixes oldest-first (pivotPositionHistory sorts ascending). If the oldest fix on two consecutive pages happens to share a timestamp (e.g., two simultaneous GPS fixes), the guard fires immediately and stops paginating early. This is a corner case, but worth noting.

Performance Considerations

4. unshift(...pageItems) on a growing array (src/App.tsx:1561)

accumulated.unshift(...pageItems);

For large histories (50 pages × 300 fixes = 15,000 items) unshift on an array is O(n) per call because it shifts every existing element right. After 50 pages this becomes O(n²) in total. Consider collecting pages in a separate pages array and merging at the end: [...collected.flat(), ...latestPage]. For typical deployments with a few hundred fixes this won't matter, but GPS trackers with years of history could hit this.

5. 150 ms inter-page delay × up to 50 pages = up to 7.5 seconds (src/App.tsx:1572)

The gentle pacing is a good idea to avoid hammering the server, but 7.5 seconds of background loading may be noticeable. This is a product decision rather than a bug — just flagging it. The state updates on each page mitigate the perceived latency.

Code Quality

6. sourceId delegation is missing in getPositionTelemetryByNodeAsync (src/services/database.ts:3917-3919)

async getPositionTelemetryByNodeAsync(nodeId: string, limit: number = 1500, sinceTimestamp?: number, beforeTimestamp?: number) {
  return this.telemetry.getPositionTelemetryByNode(nodeId, limit, sinceTimestamp, undefined, beforeTimestamp);

The fourth positional argument to TelemetryRepository.getPositionTelemetryByNode is sourceId (per the repo signature at telemetry.ts:272), and it's hardcoded to undefined here. This means sourceId filtering isn't applied in the paginated path when called from the route handler. The route handler does extract posHistSourceId (server.ts:1348-1350) but passes it as req.query.sourceId only to the privacy check — it does not pass it to getPositionTelemetryByNodeAsync. This is a pre-existing gap in the facade (the old single-page call also omitted it), so it's out of scope for this PR, but the new paginated path inherits the same gap.

7. MAX_PAGES comment may be slightly optimistic (src/App.tsx:1541)

const MAX_PAGES = 50; // safety bound (~15k fixes)

The comment says ~15k fixes but 50 pages × 1500 rows × ~1/5 ratio (5 telemetry types per fix) = 50 × 300 = 15,000 fixes. The math checks out — just be aware that 1500 telemetry rows is the server cap, not 1500 fixes, since each fix produces 5 rows (lat/lon/alt/speed/track).

Test Coverage

The new tests are well-written and exercise the right things:

• ✅ getPositionTelemetryByNode beforeTimestamp cursor test in telemetry.extra.test.ts:337-346
• ✅ generatePositionHistoryArrows dot-per-fix tests in mapHelpers.test.tsx:396-413
• ✅ Subsampling of heading triangles is verified independently

Missing test:

• The client-side pagination loop in fetchAllPositionHistory is not unit-tested. A test mocking authFetch to return two pages and asserting the merged result and cursor progression would be valuable for a future follow-up.

Summary

The core approach is sound — cursor-based backward pagination with a safety bound, and rendering a CircleMarker at every fix instead of subsampling dots. The most actionable items before merge:

• Issue 3 (no-progress guard) — low risk but worth a comment acknowledging same-timestamp behavior
• Issue 6 (sourceId not forwarded through async facade) — pre-existing but this PR adds a second call site that inherits the gap; consider noting in a follow-up issue

Everything else is minor style / future optimization.