Claude finished @Yeraze's task —— View job

Claude's Review

• Gather context
• Read changed file
• Analyze fix
• Post review feedback

Summary

The fix is correct and mirrors the existing roomFullKey resolution pattern cleanly. One issue found: the existing test asserts the old (broken) behavior and will fail.

Bug: Test not updated to match fix

src/server/meshcoreManager.rooms.test.ts:92

// This still asserts the PRE-FIX (broken) behavior:
expect(msg.fromPublicKey).toBe(authorPubkey.substring(0, 8));

After this fix, when the author contact is found via resolveContactByPrefix(), msg.fromPublicKey will be authorFullKey (the full 64-char hex key), not the 8-char prefix. This assertion will fail. It should be:

expect(msg.fromPublicKey).toBe(authorPubkey);

A second test case covering the unknown author fallback path (contact not in map → falls back to prefix) would also be valuable to guard against regressions.

Minor Observations

contact_message handler (line 980) is inconsistent

The contact_message handler stores data.pubkey_prefix directly as fromPublicKey without resolving it to the full key:

fromPublicKey: data.pubkey_prefix,   // line 980 — not resolved

The same nameForKey() comment in MeshCoreMessageStream.tsx (lines 80-82) says it was written to handle prefix-based lookups precisely because of this pattern. The room message fix makes the two handlers inconsistent. This isn't a blocker — nameForKey() does prefix-startsWith matching so it works either way — but it's worth noting as a future cleanup.

resolveContactByPrefix called with a full key works correctly (line 3260 does an exact-match first), so no edge case there.

Verdict

The logic change is correct and safe. Please fix the stale test assertion on line 92 before merging.

CI was red: meshcoreManager.rooms.test.ts:92 still asserted the old behavior (fromPublicKey == 8-char wire prefix), which this PR intentionally changes to the full key. Updated that assertion to expect the full author key (symmetric with the toPublicKey assertion right above it, which already expects the full room key) in 077552d. The unknown-contact fallback test is unaffected. The lint annotations in the run are pre-existing on main (non-blocking).

Claude finished @Yeraze's task —— View job

Claude's Review

• Gather context
• Read changed files
• Analyze fix and test updates
• Post review feedback

Summary

The fix is clean, correct, and well-tested. The stale assertion from the previous review has been addressed. Here's a full pass:

meshcoreManager.ts — Fix (lines ~3270–3275)

const authorContact = this.resolveContactByPrefix(authorPrefixHex);
const authorFullKey = authorContact?.publicKey ?? authorPrefixHex;

This mirrors the existing roomFullKey pattern exactly and is idiomatic. The fallback to authorPrefixHex preserves prior behavior for unknown contacts. No issues.

meshcoreManager.rooms.test.ts — Test update (line 95)

// Before (broken assertion):
expect(msg.fromPublicKey).toBe(authorPubkey.substring(0, 8));

// After (correct):
expect(msg.fromPublicKey).toBe(authorPubkey);

Correctly updated. The comment added on lines 92–94 explains the intent clearly and references the issue number — good documentation.

Coverage check

The test suite covers:

• Known author → full key stored (fromPublicKey === authorPubkey) ✅
• Unknown room, unknown author → both fall back to prefix (toPublicKey === 'aabbccddee00') ✅ (line 135)
• Unknown author's fromPublicKey → implicitly covered by the fallback test at line 130 (author_pubkey_prefix: '11223344', no contact in map → fromPublicKey stays as the prefix). ✅

The unknown-author fallback path the previous review asked for as a "second test case" is already covered by the existing 'falls back to prefix when room contact not found' test at line 122.

Minor observation (non-blocking, pre-existing)

contact_message handler still stores data.pubkey_prefix directly as fromPublicKey (~line 980), while the room_message handler now stores the full key. This is an inconsistency but nameForKey() on the frontend handles both via prefix-startsWith matching, so it works correctly either way. No action needed here.

Verdict

The logic change is correct, the test assertion is fixed, and coverage is adequate. Ready to merge.