Claude finished @Yeraze's task —— View job

PR Review: fix(traceroute): route over the default-keyed channel + channel dropdowns

Tasks:

• Gather context
• Read changed files
• Review code quality & best practices
• Check for bugs/issues
• Review test coverage
• Post final feedback

Summary

This is a well-thought-out correctness fix. The core insight — that what makes a traceroute traversable is the PSK value (default key), not the slot number (0 is just the primary slot) — is sound and the implementation is clean. Overall the code quality is high. A few items worth discussing below.

resolveBroadcastChannel — Logic & Edge Cases

isMeshReadablePsk treats empty string as unencrypted (line 83)

function isMeshReadablePsk(psk: string | null | undefined): boolean {
  return psk == null || psk === '' || psk === DEFAULT_PSK_BASE64;
}

Empty string is treated as "unencrypted/readable by everyone." Is this definitely what the DB stores for an unencrypted channel, vs. a possibly incomplete row? Worth a comment (or a DB schema cross-reference) confirming this assumption, since it determines whether a channel is included in the traversable set.

Double DB call for traceroutes (meshRequestRoutes.ts, lines 30–35)

const channel = await resolveDestinationChannel(
  destinationNum,
  traceManager,
  databaseService,
  req.body.channel ?? (await resolveBroadcastChannel(traceManager, databaseService)),
);

When req.body.channel is undefined, this eagerly calls resolveBroadcastChannel (fetches all channels) AND then passes the result to resolveDestinationChannel which — because isValidChannelIndex(explicitChannel) returns true for 0–7 — short-circuits without doing a node lookup. That's fine functionally, but the code flow is a little non-obvious.

More importantly: if req.body.channel is undefined, resolveBroadcastChannel is called regardless of whether resolveDestinationChannel will need it — JS evaluates ?? lazily but the await inside the expression means it still runs synchronously in order. Actually, re-reading: since req.body.channel ?? (await resolveBroadcastChannel(...)) — if req.body.channel is undefined or null, resolveBroadcastChannel is awaited before resolveDestinationChannel is even called. This is correct (the result is passed as explicitChannel), but there's a subtle issue: resolveDestinationChannel will always receive a valid channel (0–7) when no explicit channel is sent, meaning the isValidChannelIndex early-return path always fires for traceroutes, so resolveDestinationChannel never queries the node's stored channel. This is intentional, but worth making more explicit. A direct call const channel = req.body.channel !== undefined ? ... : await resolveBroadcastChannel(...) would be more readable, skipping resolveDestinationChannel entirely for the traceroute broadcast case.

No input validation on req.body.channel type before passing to backend

In App.tsx line 2817:

body: JSON.stringify({ destination: nodeNum, sourceId, ...(channel !== undefined && { channel }) }),

channel here comes directly from ch.id in the dropdown (line 1908), which is typed as number from the Channel type. That looks fine. But it's worth noting the frontend doesn't do any client-side range check before sending; the server-side isValidChannelIndex is the sole gatekeeper. This is acceptable but means a malformed UI could send any integer.

Frontend — MessagesTab.tsx

Inline style duplication (minor)

The three split-button blocks (Traceroute, Exchange Node Info, Exchange Position) are nearly identical. The structure, styles, and dropdown logic are copy-pasted three times (lines ~1847–2100). The existing pattern appears to be "mirror the Exchange Position split button," so this is consistent with the codebase, but extraction into a reusable SplitButton component would significantly reduce maintenance surface area if a fourth action is ever added.

Click-outside handlers use document.addEventListener (minor)

Three separate useEffect hooks (lines 359–382) each add/remove a click listener on document when their respective dropdown is open. This is the same pattern used for the existing position dropdown, so it's consistent — but it means three separate document listeners can be active simultaneously. A single useClickOutside hook that accepts refs would be cleaner. Not a bug, just a DRY concern.

Dropdown not closed after main button click

When the user clicks the main "Traceroute" button (not the ▾), showTracerouteChannelDropdown is not explicitly set to false. It will be closed by the document click handler eventually, but if the dropdown is already open and the user clicks the main button, there's a brief window where both are active. This is very minor UX but could be tightened.

channels.length > 1 condition is correct — The dropdown arrow only appears when there's more than one channel, which avoids the UI clutter of a single-channel dropdown. Good.

Test Coverage

Strengths:

• resolveBroadcastChannel tests are comprehensive: covers slot-0 default, non-slot-0 default, unencrypted (null/''), lowest-index preference, all-private fallback, empty channels, and out-of-range indices.
• Route-level tests properly verify PSK-based resolution (not hardcoded slot 0), explicit channel override, and fallback-to-0.
• The nodeinfo/request dropdown override test is present.

Gaps:

• No test for the case where req.body.channel is an out-of-range value (e.g., 101) for /traceroute — the server should reject it and fall back to resolveBroadcastChannel. The unit tests for resolveDestinationChannel cover this, but there's no integration-level test at the route for the traceroute path specifically with an out-of-range explicit channel.
• No test for req.body.channel === 0 explicitly for traceroutes — this is a valid channel and should be honored (slot 0 with a private key selected from dropdown). The existing "honors an explicit channel override" test uses channel 5, which doesn't exercise the "0 is falsy" edge case if anyone ever changes the check from ?? to ||.

Minor Code Style Notes

• The extra parentheses on resolveSourceManager calls (e.g., (resolveSourceManager(traceSourceId)) at line 29) are unnecessary — they look like incomplete destructuring or type assertion attempts. They're harmless but slightly odd.
• The warning log in resolveBroadcastChannel could include manager.sourceId and the channel count for easier debugging.

Verdict

The core logic is correct and well-tested. The isMeshReadablePsk empty-string assumption and the slightly convoluted double-call pattern in the traceroute route handler are the two items most worth addressing before merge. The rest are minor style/DRY suggestions.

Claude finished @Yeraze's task —— View job

PR Review: fix(traceroute): route over the default-keyed channel + channel dropdowns

Tasks:

• Gather context
• Read changed files
• Review code quality & best practices
• Check for bugs/issues
• Review test coverage
• Post final feedback

Summary

This PR correctly fixes the core issue: traceroute traversability depends on the PSK value (default key), not the slot number. The implementation is clean and the previous review's substantive concerns were all addressed in 96a6b67. The current state is in good shape. A few remaining observations below.

Backend

resolveBroadcastChannel — Clean

The logic in resolveDestinationChannel.ts:113–132 is correct and well-documented. The isMeshReadablePsk function (line 87) is sound: the defensive '' case is appropriately commented, the AQ== constant is well-named, and the isValidChannelIndex guard properly filters out MQTT-style channel indices (e.g., 101) before they can pollute the result.

Traceroute route handler — Cleaner than before

// meshRequestRoutes.ts:31–33
const channel = isValidChannelIndex(req.body.channel)
  ? req.body.channel
  : await resolveBroadcastChannel(traceManager, databaseService);

This is a clear improvement over the original resolveDestinationChannel indirection. The explicit comment (lines 24–29) explains the reasoning well.

Minor: Extra parentheses (pre-existing, out of scope)

Lines like const traceManager = (resolveSourceManager(traceSourceId)); (line 30) have redundant parens. This is pre-existing style shared across all five routes in this file — acknowledged in the author's response and correctly left alone.

handleTraceroute in App.tsx has no debounce guard

handleExchangeNodeInfo (line 2892) has explicit debounce protection (if (nodeInfoLoading === nodeId) return), but handleTraceroute (line 2799) does not check tracerouteLoading === nodeId before proceeding. A double-click or rapid re-submission could fire duplicate traceroutes. This is a pre-existing gap but worth noting since the two handlers are now more prominently paired in the UI.

Frontend

Dropdown state is not reset when selectedDMNode changes

showTracerouteChannelDropdown and showNodeInfoChannelDropdown are component-level state. If the user opens the Traceroute dropdown for node A, then clicks a different DM thread (node B), the dropdown will still be open when node B's panel renders. The existing Position dropdown has the same behavior — consistent but not ideal.

Actions menu traceroute/nodeinfo don't show channel selection

The compact actions menu (lines 1118–1171) calls handleTraceroute(selectedDMNode) and handleExchangeNodeInfo(selectedDMNode) without channel selection. The split-button dropdown only exists in the expanded panel. This is a reasonable UX decision — the actions menu is a secondary path — but it's worth documenting if anyone asks why the channel selector isn't accessible from both entry points.

Test Coverage

Strongly improved from the previous version:

• resolveBroadcastChannel unit tests cover: slot-0 default, non-slot-0 default, null/empty unencrypted, lowest-index preference, all-private fallback, no channels, out-of-range indices.
• Route tests cover: explicit channel override, explicit 0 override (guards || vs ?? regression), out-of-range explicit channel falls back to PSK resolution, all-private fallback, the "PSK not slot 0" case.
• The channel: 0 test (line 110) is particularly well-designed — it forces the PSK resolver toward a different slot ({ id: 3, psk: 'AQ==' }) to prove the explicit 0 truly wins, not just coincides.

One remaining gap:

The /nodeinfo/request route has a test for explicit channel override (channel 4), but no test for the case where req.body.channel is 0 explicitly — mirroring the traceroute falsy-trap test. Since resolveDestinationChannel has the same isValidChannelIndex guard, this is lower risk than the traceroute case, but the symmetry would be nice.

Verdict

The logic is correct, well-tested, and the core issue is properly solved. The debounce gap in handleTraceroute and the dropdown-open-on-node-change behavior are the only noteworthy items, and both are either pre-existing or stylistic. This is ready to merge.

Thanks for the thorough review — addressed the substantive items in 96a6b670:

Addressed

• Double DB call / convoluted traceroute resolution — rewritten to a direct isValidChannelIndex(req.body.channel) ? req.body.channel : await resolveBroadcastChannel(...). No more eager call routed through resolveDestinationChannel, one fewer DB read, and the node's stored channel is now explicitly never consulted for traceroutes. isValidChannelIndex is exported for this.
• isMeshReadablePsk empty-string assumption — added a comment: the ingest path (meshtasticManager, only base64-encodes when psk.length > 0) stores an unencrypted channel as NULL; '' is handled purely defensively.
• Test gaps — added two route-level traceroute tests: explicit channel 0 is honored (guards against anyone swapping the check to a || falsy trap), and an out-of-range explicit channel (101) falls back to the default-keyed resolution.
• Warning log — now includes the channel count (sourceId was already there).

Declined (with reasoning)

• Extract a shared SplitButton / useClickOutside — valid DRY point, but deliberately out of scope: this PR mirrors the existing Exchange Position pattern, and a real extraction should also fold in that pre-existing button (3 → 1) as its own cleanup so this fix stays minimal.
• Dropdown not closed on main-button click — the same document click handler that the existing Position button relies on closes it in the same tick; behavior is identical to the established control, so I left it consistent rather than special-casing one of three.
• Extra parens (resolveSourceManager(...)) — pre-existing style shared by all five routes in this file, not introduced here; out of scope for a targeted fix.

Full suite green (7463 passed), CI was green on the prior commit; re-running now.