Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore): manual telemetry poll buttons

Tasks

• Gather context and understand the request
• Read changed files
• Review backend scheduler refactor
• Review backend route implementation
• Review frontend component changes
• Review test coverage
• Post final review

Summary

This is a well-designed, well-tested feature. The architecture is solid: shared logic is correctly extracted into requestTelemetryForNode, throttling is correctly re-used, and both the route and UI are properly gated. A few things worth noting:

Code Quality & Best Practices

src/server/routes/meshcoreRoutes.ts (lines 2395–2396)

The route uses Date.now() directly for the throttle check, while the scheduler uses an injectable nowFn (for testability). This is a small inconsistency but doesn't cause a real problem in practice.

const sinceLastTx = Date.now() - lastTx;

Minor: the scheduler's tick() passes its own now to requestTelemetryForNode, while the route stamps now before calling the same function — both are correct, but the two timestamps could theoretically differ by a few ms if the request is slow to check the DB. This is harmless in practice.

src/server/routes/meshcoreRoutes.ts (line 2417)

markTelemetryRequested is stamped before requestTelemetryForNode is called — good for fair-rotation — but the comment says "Stamp before issuing so the gate applies regardless of result". Worth noting: if requestTelemetryForNode throws an unhandled error from the DB batch insert, the telemetry request was already stamped, so the 60s gate blocks a retry. This is the same behavior as the scheduler and is intentional, but it's subtle.

Potential Bugs / Issues

src/components/MeshCore/MeshCoreNodeTelemetryConfig.tsx (lines 148–178)

The poll() function doesn't clear pollMsg on a second invocation if the previous message is still showing. The setPollMsg(null) at line 151 handles this correctly — this is fine.

src/components/MeshCore/MeshCoreNodeTelemetryConfig.tsx (lines 278–298)

Both buttons are disabled when either is polling (polling !== null). This is correct: the 60s gate means triggering the second button while the first is in flight would immediately get a 429 anyway. The UX is sound.

src/server/routes/meshcoreRoutes.ts (lines 2382–2385)

The route correctly checks !manager || !manager.isConnected() and returns 409. However, the router-level guard at line 57 already guarantees the manager exists. The !manager check here is defensive but redundant — not a bug.

src/server/routes/meshcoreRoutes.ts (line 2388–2390)

If getMeshCoreRemoteTelemetryScheduler() returns null, the route returns 503. But this path stamps recordMeshTx and markTelemetryRequested AFTER the scheduler check, so a missing scheduler does not incorrectly consume the gate. ✓

Performance

No concerns. The route is gated by meshcoreDeviceLimiter at the HTTP layer and by the 60s mesh-TX gate at the business-logic layer. The DB lookup (getNodeByPublicKeyAndSource) is a single keyed read and is fast.

Security

• requireAuth() + requirePermission('nodes', 'read', { sourceIdFrom: 'params.id' }) is correct and consistent with the codebase convention.
• publicKey is validated against isValidPublicKey (64-char hex) before use. ✓
• type is validated as a strict union 'status' | 'lpp' before use. ✓
• The 60s gate prevents burst RF transmission. ✓

One minor concern: the route gates on nodes:read but it causes an RF transmission. The PR description acknowledges this design choice ("a user-initiated read that transmits"), and the permission name matches the backend. This is a judgment call — configuration:write might be more appropriate since the action causes a real-world side effect (air time consumption). Not a blocker but worth a deliberate decision.

Test Coverage

Backend tests (meshcoreRoutes.test.ts):

• ✓ 401 without auth
• ✓ 400 for malformed key
• ✓ 400 for invalid type
• ✓ status dispatch + arg assertions
• ✓ lpp dispatch + arg assertions
• ✓ null advType fallback (unknown node → companion)
• ✓ 409 when not connected
• ✓ 429 + Retry-After header when within 60s gate

One gap: no test for the 503 case (scheduler is null / unavailable). This is a minor edge case but the branch is there.

Frontend tests (MeshCoreNodeTelemetryConfig.test.tsx):

• ✓ Both buttons rendered
• ✓ { type: "status" } POST
• ✓ { type: "lpp" } POST
• ✓ 429 error surfaced
• ✓ Disabled without nodes:read

One gap: no test for the "Request sent — no telemetry returned" path (when written === 0). This is a distinct UI state (empty result vs. error) worth covering.

Scheduler refactor: PR states 32 existing scheduler tests pass unchanged — good.

Minor Nits

• meshcoreRoutes.ts:2396: const sinceLastTx = Date.now() - lastTx; is computed before the lastTx > 0 guard, which means sinceLastTx could be Infinity when lastTx === 0. The guard short-circuits before using the value, so no bug, but the order feels slightly backward. Swapping the guard order (if (lastTx > 0 && sinceLastTx < MIN_INTERVAL_BETWEEN_REQUESTS_MS)) already handles this correctly — this is fine as-is.
• MeshCoreNodeTelemetryConfig.tsx:165: The i18n key meshcore.telemetry_config.poll_wrote uses a template literal in the fallback string (Wrote ${written} telemetry row(s).). This works because the fallback is evaluated at call-site with written in scope, but a future i18n extraction tool might not handle this correctly. Consider using the count interpolation pattern for full i18n correctness.

Verdict

Approve with minor suggestions. The implementation is correct, the throttling strategy is sound, the extracted shared method is clean, and test coverage is strong. The two test gaps (503 case and written === 0 UI path) and the nodes:read vs configuration:write permission decision are the only items worth a follow-up discussion. No blockers.