Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore): 2/3-byte path hash support

Tasks:

• Gather context and understand the PR
• Review src/utils/meshcorePath.ts and tests
• Review src/server/meshcoreNativeBackend.ts and tests
• Review src/server/meshcoreManager.ts
• Review src/server/routes/meshcoreRoutes.ts and tests
• Review src/components/MeshCore/MeshCoreContactDetailPanel.tsx and tests
• Review src/components/MeshCore/hooks/useMeshCore.ts
• Compile and post final review

Code Review

This is a well-implemented feature with solid protocol verification, good test coverage, and clean layering. Here are my observations:

Strengths

• Protocol correctness: The packed path_len format (top 2 bits = hash_size−1, bottom 6 bits = hop count) is correctly implemented everywhere. The PR description's firmware trace is accurate and matches the code.
• Backward compatibility: 1-byte paths continue to use setContactPath() (the proven library path), and multi-byte paths correctly bypass it. Callers that omit hashBytes still get 1-byte behavior.
• No DB migration needed: Clever use of token-length encoding in the stored hex string — "a3,7f" vs "a3f2,7f01" encodes the width without a schema change.
• Test coverage: The formatOutPath suite covers edge cases (padding, interior zeros, partial trailing hops). The set_out_path backend tests cover 1-byte/2-byte dispatch, oversize paths, and missing-contact errors.

Issues Found

1. parseHexPathChain redundant hex-range check (src/server/routes/meshcoreRoutes.ts:190)

const n = parseInt(tok.slice(j * 2, j * 2 + 2), 16);
if (!Number.isFinite(n) || n < 0 || n > 0xff) return null;

The tokenRe regex (^[0-9a-fA-F]{N}$) already guarantees tok is valid hex, so parseInt will always yield a finite integer in [0, 255]. The extra check is harmless but dead code.

2. Double getContacts() call in set_out_path (src/server/meshcoreNativeBackend.ts:1003)

// In resolvePublicKey():
const contacts: any[] = await this.connection.getContacts();

// Then immediately after:
const contacts: any[] = await c.getContacts();  // Line 1003

resolvePublicKey already calls getContacts() internally to resolve a prefix to a full key. Then set_out_path calls it again to find the full contact object. This means two serial round-trips to the device for every set_out_path call. The cost is acceptable for a low-frequency write operation, but it's worth noting. A future refactor could have resolvePublicKey return the full contact object.

3. traceContactPath passes raw 1-byte hashes to firmware for multi-byte paths (src/server/meshcoreManager.ts:2328-2336)

const pathBytes = Uint8Array.from(
  contact.outPath.split(',').flatMap((h) => {
    const tok = h.trim();
    const bytes: number[] = [];
    for (let i = 0; i + 2 <= tok.length; i += 2) {
      bytes.push(parseInt(tok.slice(i, i + 2), 16));
    }
    return bytes;
  }),
);

This correctly expands "a3f2" → [0xa3, 0xf2]. However, the trace_path command then calls c.tracePath(path, ...) — and tracePath() in meshcore.js presumably expects a packed path_len byte (same wire format as out_path_len). The PR description says "this PR only fixes the byte extraction so it no longer truncates" and defers full multi-byte trace support. The comment in the PR body makes this explicit. Just confirming the known limitation: tracing a 2/3-byte path will send the raw expanded bytes but without the correct packed out_path_len — behavior may be undefined at the firmware level until the follow-up. A code comment in traceContactPath calling this out would help future contributors.

4. hopOptions recalculates on every render (src/components/MeshCore/MeshCoreContactDetailPanel.tsx:229-232)

const hopOptions = React.useMemo(
  () => repeaterHopOptions(repeaters ?? [], editorHashBytes),
  [repeaters, editorHashBytes],
);

This is correctly memoized. ✅

5. parsePathHops silently drops invalid tokens (src/utils/meshcorePath.ts:31)

.filter((s) => /^[0-9a-f]+$/.test(s) && s.length % 2 === 0 && s.length >= 2 && s.length <= 6);

This filters out bad tokens rather than returning an error. This is fine for display/parsing of stored paths (where the caller just wants the valid hops), but it means a malformed stored path is displayed silently truncated. The behavior is consistent with existing code, just worth being aware of.

6. UI width selector clears hops even on same-width selection (src/components/MeshCore/MeshCoreContactDetailPanel.tsx:316-322)

const changeHashBytes = (next: 1 | 2 | 3) => {
  if (next === editorHashBytes) return;   // ← early exit on same value ✅
  setEditorHashBytes(next);
  setEditorHops([]);
  ...
};

Actually handles this correctly — the early return prevents clearing when the user clicks the already-selected option. ✅

7. Missing test: set_out_path with hash_bytes=3 (src/server/meshcoreNativeBackend.test.ts)

Tests cover 1-byte (setContactPath) and 2-byte (addOrUpdateContact with 0x42), but there's no test for 3-byte paths (packed ((3-1)<<6)|hop_count = 0x80|count). Given 3 is a valid user-facing option, a test covering the 3-byte packed length would close this gap.

8. Route error message uses hardcoded example (src/server/routes/meshcoreRoutes.ts:733)

error: `Invalid outPath — expected a comma-separated hex chain of ${hashBytes}-byte hops (${hashBytes * 2} hex chars each), e.g. "${'a3f27f01'.slice(0, hashBytes * 2)}"`,

'a3f27f01'.slice(0, hashBytes * 2) yields "a3" (1-byte), "a3f2" (2-byte), "a3f27f" (3-byte but only one token is shown, no comma). For 3 bytes the example would be more readable as "a3f27f,0102ab" to show the comma-separated format. Minor UX nit.

Summary

The core logic is correct and safe. The protocol-level packed byte math is right, backward compatibility is preserved, and the test suite covers the critical dispatch paths. The issues above are minor — a missing 3-byte test case is the most meaningful gap. The deferred trace_path limitation is clearly acknowledged.

No blocking issues. The feature is ready to merge pending any nits the team wants to address.