perf(analysis): bound the hop-counts scan; fix(ui): surface telemetry request failures

[Yeraze]

Summary

The two follow-ups the #4570 and #4568 reviews flagged. Independent changes, one per commit — reviewable separately.

---

1. perf(analysis) — bound the hop-counts scan (27d018a6)

getHopCounts fetched every traceroute row for the requested sources and reduced to one row per node in JS.

Traceroute history is capped per node-pair by TRACEROUTE_HISTORY_LIMIT (default 50, env-tunable higher), so the scan read roughly 50× the rows it needed — a 500-node mesh pulls ~25,000 rows to produce ~500 entries, on every Map Analysis load. Pending rows, generated continuously by auto-traceroute, were fetched only to be discarded.

Now narrowed with route IS NOT NULL on both halves and the newest row per (sourceId, toNodeNum) picked via GROUP BY + INNER JOIN. Plain standard SQL rather than a window function, so there's no dialect branch to get wrong.

The coverage gap this exposed: AnalysisRepository was constructed with 'sqlite' in all 15 existing cases — this query had zero PostgreSQL or MySQL coverage. That's not acceptable for a change whose entire risk is dialect compatibility, so this adds analysis.hopCounts.multiBackend.test.ts: the same seven behaviours on all three backends, following the newsCache.test.ts pattern. 21 assertions, 0 skipped with the containers up — verified all three actually ran rather than skipping vacuously.

Behavior change worth a reviewer's eye: picking the newest answered row now happens in SQL, which can't judge whether route is valid JSON. A corrupt newest row therefore yields grey instead of falling back to an older row. Both writers store JSON.stringify(route), so reaching this needs manual DB editing — and grey is the honest answer for data we can't read. Pinned by a test.

---

2. fix(ui) — surface telemetry request failures (b19d77fc)

handleRequestTelemetry throws on any non-TX-disabled HTTP error, and that throw — plus any genuine network failure — landed in a catch that only called logger.error. Spinner cleared, user told nothing: the identical silent-failure shape #4568 just fixed for neighbor info.

Unlike that endpoint there's no 403/429 needing special explanation — the server answers 400 (validation), 503 (not connected) or 500 — and the thrown Error already carries detail.error when the server sent one. So it surfaces that directly rather than adding a second status-mapping helper.

---

Test plan

• npx tsc --noEmit and npx tsc -p tsconfig.server.json --noEmit — clean
• npm run lint:ci — clean, no baseline growth
• Full suite with PostgreSQL and MySQL containers up: 14,154 passed, 0 failed (109 skipped)
• New analysis.hopCounts.multiBackend.test.ts — 7 behaviours × 3 backends
• Existing SQLite analysis.test.ts suite still green, including every Map Analysis hop shading falls back to 0-hop/local on missing traceroute data #4570 assertion

Not covered

No browser validation. The hop-shading outcome is asserted at the repository layer, and the telemetry toast is not exercised by a rendering test (App.tsx isn't renderable in this suite — the same limitation that let the silent failure survive). The telemetry toast in particular is a one-line behaviour I could not unit-test without extracting it, and unlike #4568's status mapping there was no logic worth extracting.

---

Generated by Claude Code

https://claude.ai/code/session_01EtJnjbUgYwJfNU6XXACbFf

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

Tasks

• Gather context
• Read changed files
• Review analysis.ts SQL changes
• Review App.tsx error handling fix
• Review test coverage
• Review locale changes
• Post final review

---

Summary

This is a clean, well-scoped pair of fixes. Both changes are correct and the reasoning is sound. Notes below are minor.

---

perf(analysis) — getHopCounts SQL rewrite (analysis.ts)

Overall: solid. The GROUP BY + INNER JOIN approach is the right idiom here and avoids window functions that would need dialect branching.

One subtle correctness note on the outer WHERE clause:

analysis.ts:684:

.where(and(inArray(traceroutes.sourceId, args.sourceIds), isNotNull(traceroutes.route)));

The outer query already joins against newest which was built with isNotNull(traceroutes.route), so the isNotNull predicate on the outer scan is redundant. It's harmless — and actually good as a belt-and-braces guard (the code comment at line 697 calls it out explicitly) — but worth knowing it provides defence-in-depth rather than narrowing that the join couldn't already provide.

Tie-breaking behavior (analysis.ts:687-695): the seen Map skips duplicates with a "first wins" approach when two rows share the same max timestamp. This is correct and pinned by a test, but it's worth noting the choice is arbitrary — either row is equally "newest." The comment acknowledges this. No action needed.

isNotNull import (analysis.ts:23): isNotNull is now imported from drizzle-orm — that import was added correctly alongside the use site. No issue.

---

fix(ui) — Telemetry request failures surfaced (App.tsx)

Overall: correct fix, consistent with the #4568 neighbor info pattern.

Observation on error message quality: When the server returns a non-OK response without a parseable JSON body, the catch gets:

throw new Error(detail.error || `Telemetry request failed (${response.status})`);

The fallback detail.error will be undefined when .catch(() => ({})) fires, so it becomes the generic Telemetry request failed (${response.status}). That's fine — it's a clear message. But if the body does parse but lacks detail.error (e.g. { message: "..." }), the fallback also fires and the server's message is silently dropped. This is the same pattern used elsewhere in the codebase, so it's acceptable, but it's worth noting.

Toast error message format (en.json): "Telemetry request failed: {{error}}" — the {{error}} placeholder will be populated by either the server's detail.error string or the network fallback from t('errors.network'). Clean.

---

Test coverage

New file analysis.hopCounts.multiBackend.test.ts: Excellent addition. The seven scenarios cover the key behaviours, and the approach of using describe.skipIf(!postgresAvailable) mirrors the established pattern.

One observation on the INSERT helper (analysis.hopCounts.multiBackend.test.ts:109-113):

const literal = sql.replace(/\$\d+|\?/g, () => {
  const v = row.shift() as string | number | null;
  if (v === null) return 'NULL';
  return typeof v === 'number' ? String(v) : `'${v}'`;
});

The comment notes "Every value here is test-authored, so there is no injection surface" — that's correct for this test context. The row.shift() approach is a bit fragile (relies on positional ordering matching the regex replacement order), but it works correctly here since the regex reliably matches left-to-right and the column list is fixed. Acceptable for test code.

Existing analysis.test.ts coverage (+19 lines): The new assertions on the hop counts behavior in the SQLite suite are clear and complement the multiBackend test well.

---

Minor items

• The PR description's "Behavior change worth a reviewer's eye" section is useful — the grey-instead-of-fallback-to-older-row tradeoff is explicitly called out and pinned by the corrupt-data test. No concern.
• No new ESLint violations introduced (PR claims clean lint:ci).
• TypeScript types remain sound — HopEntry.hops is number, parsed safely via JSON.parse + Array.isArray guard.

---

Verdict

Both changes are correct, well-tested, and follow project conventions. The performance improvement is meaningful for larger meshes. The bug fix matches the established pattern for surfacing failures. No blocking issues.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

Tasks

• Gather context
• Read changed files
• Review analysis.ts SQL changes
• Review App.tsx error handling fix
• Review test coverage
• Review locale changes
• Post final review

---

Summary

This is a clean, well-scoped pair of fixes. Both changes are correct and the reasoning is sound. Notes below are minor.

---

perf(analysis) — getHopCounts SQL rewrite (analysis.ts)

Overall: solid. The GROUP BY + INNER JOIN approach is the right idiom here and avoids window functions that would need dialect branching.

One subtle correctness note on the outer WHERE clause:

analysis.ts:684:

.where(and(inArray(traceroutes.sourceId, args.sourceIds), isNotNull(traceroutes.route)));

The outer query already joins against newest which was built with isNotNull(traceroutes.route), so the isNotNull predicate on the outer scan is redundant. It's harmless — and actually good as a belt-and-braces guard (the code comment at line 697 calls it out explicitly) — but worth knowing it provides defence-in-depth rather than narrowing that the join couldn't already provide.

Tie-breaking behavior (analysis.ts:687-695): the seen Map skips duplicates with a "first wins" approach when two rows share the same max timestamp. This is correct and pinned by a test, but it's worth noting the choice is arbitrary — either row is equally "newest." The comment acknowledges this. No action needed.

isNotNull import (analysis.ts:23): isNotNull is now imported from drizzle-orm — that import was added correctly alongside the use site. No issue.

---

fix(ui) — Telemetry request failures surfaced (App.tsx)

Overall: correct fix, consistent with the #4568 neighbor info pattern.

Observation on error message quality: When the server returns a non-OK response without a parseable JSON body, the catch gets:

throw new Error(detail.error || `Telemetry request failed (${response.status})`);

The fallback detail.error will be undefined when .catch(() => ({})) fires, so it becomes the generic Telemetry request failed (${response.status}). That's fine — it's a clear message. But if the body does parse but lacks detail.error (e.g. { message: "..." }), the fallback also fires and the server's message is silently dropped. This is the same pattern used elsewhere in the codebase, so it's acceptable, but it's worth noting.

Toast error message format (en.json): "Telemetry request failed: {{error}}" — the {{error}} placeholder will be populated by either the server's detail.error string or the network fallback from t('errors.network'). Clean.

---

Test coverage

New file analysis.hopCounts.multiBackend.test.ts: Excellent addition. The seven scenarios cover the key behaviours, and the approach of using describe.skipIf(!postgresAvailable) mirrors the established pattern.

One observation on the INSERT helper (analysis.hopCounts.multiBackend.test.ts:109-113):

const literal = sql.replace(/\$\d+|\?/g, () => {
  const v = row.shift() as string | number | null;
  if (v === null) return 'NULL';
  return typeof v === 'number' ? String(v) : `'${v}'`;
});

The comment notes "Every value here is test-authored, so there is no injection surface" — that's correct for this test context. The row.shift() approach is a bit fragile (relies on positional ordering matching the regex replacement order), but it works correctly here since the regex reliably matches left-to-right and the column list is fixed. Acceptable for test code.

Existing analysis.test.ts coverage (+19 lines): The new assertions on the hop counts behavior in the SQLite suite are clear and complement the multiBackend test well.

---

Minor items

• The PR description's "Behavior change worth a reviewer's eye" section is useful — the grey-instead-of-fallback-to-older-row tradeoff is explicitly called out and pinned by the corrupt-data test. No concern.
• No new ESLint violations introduced (PR claims clean lint:ci).
• TypeScript types remain sound — HopEntry.hops is number, parsed safely via JSON.parse + Array.isArray guard.

---

Verdict

Both changes are correct, well-tested, and follow project conventions. The performance improvement is meaningful for larger meshes. The bug fix matches the established pattern for surfacing failures. No blocking issues.