Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore): pin favorited nodes to top of DM list

• Read and analyze changed files
• Review code quality and best practices
• Check for potential bugs
• Assess performance considerations
• Review test coverage
• Post final feedback

Summary

This is a clean, well-scoped frontend-only change that addresses a legitimate UX gap. The implementation is consistent with patterns used elsewhere in the codebase. A few observations:

Code Quality & Best Practices

MeshCoreDirectMessagesView.tsx

• The approach of building a favoriteByKey lookup map via useMemo (lines 110–116) is correct and idiomatic — avoids repeated linear scans through nodes during sort.
• The sort logic (lines 194–206) correctly places favorites above non-favorites regardless of sortField/sortDirection, matching the stated requirement.
• The nodes = [] default parameter (line 53) ensures legacy callers that don't pass nodes degrade gracefully without crashing — good defensive coding.

Minor observation: The favoriteByKey map only stores entries where n.isFavorite === true (line 113), so favoriteByKey.get(key) ?? false at lines 197–198 is functionally correct. This is fine but slightly asymmetric — alternatively, storing all keys with their boolean value would make the map more self-documenting, though the current approach is more memory-efficient for sparse favorites.

Potential Bugs

Search + favorites interaction (line 209–217): The filteredPeers memo applies the search filter after sorting. This means favorites are correctly pinned within the filtered result set. ✅

Favorites from nodes not in contacts (line 196–199): If a node is in nodes with isFavorite: true but has no corresponding contact in contacts, it would still appear in dmPeers (added via messages), and favoriteByKey.get(key) would correctly return true. ✅

One edge case to verify: The nodes prop carries MeshCoreNode objects keyed by publicKey. The dmPeers set is populated using canonicalize() which resolves prefix keys to full pubkeys. If a node in nodes has a full 64-hex publicKey but the DM peer was added via a prefix-only inbound message and canonicalize() resolved it, the lookup favoriteByKey.get(a) should still match correctly — this appears safe since canonicalize resolves to the full key from contacts, and both contacts and nodes share the same full pubkey format. ✅

Performance

• Both favoriteByKey and contactsByKey are useMemo-cached and only recompute when their dependencies change — no issues.
• dmPeers already had contacts, messages, etc. in its dependency array; adding favoriteByKey is correct and minimal.
• No concerns for typical MeshCore node counts.

Security

No security concerns — this is purely a read-only UI sort/display change with no API calls, auth changes, or user-controlled data paths added.

Test Coverage

The new test at line 261–290 (pins favorited peers to the top of the DM list regardless of sort order) is well-constructed:

• Uses a favorited node with the oldest lastSeen (1000) to robustly verify it wins over the default lastMessage desc sort.
• Uses span:not(.mc-dm-row-favorite) selector to extract only the name text, correctly ignoring the ★ character.
• Verifies both ordering and the ★ indicator in a single test.

Missing coverage: There's no test asserting that favorites remain pinned when the user switches to name sort (ascending or descending). The sortField = 'name' path in the sort function at line 200–201 is exercised by the existing test at line 230, but not with favorites present. This would be a useful regression guard since the aFav !== bFav guard (line 199) applies before both sort branches. Consider adding a test like:

// favorites stay pinned even when sortField='name'

Also no test for the ★ indicator being absent when nodes prop is omitted — but the graceful-degradation behavior (default nodes = []) is implicitly covered by all the other tests that don't pass nodes.

Overall

Approved with minor suggestions. The implementation is correct, well-tested for the primary case, and consistent with codebase patterns. The only gap is a test for favorites-stay-pinned under name sort, which is low risk given the sort logic structure but would be a solid regression guard.

Addressed the review suggestion in 17e1b63: added a regression test (keeps favorited peers pinned when the user switches to name sort) asserting that a favorited peer stays pinned to the top under name/asc sort, where it would otherwise sort last alphabetically. This guards the aFav !== bFav pin that runs ahead of both sort branches.

Left the asymmetric favoriteByKey map (true-only entries) as-is — as noted, it's functionally correct and more memory-efficient for sparse favorites.

Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore): pin favorited nodes to top of DM list

• Read and analyze changed files
• Review code quality and best practices
• Check for potential bugs
• Assess performance considerations
• Review test coverage
• Post final feedback

Summary

This is a clean, well-scoped follow-up that directly addresses the feedback from the prior review. The implementation remains sound, and the new regression test (keeps favorited peers pinned when the user switches to name sort) fills the only gap called out previously.

New test (17e1b63) — keeps favorited peers pinned when the user switches to name sort

Test design is correct and robust:

• Uses 'Zeta' as the favorited node — alphabetically last, so it would naturally sort to the bottom under name/asc. This makes the test a real regression guard: if the aFav !== bFav early-exit in the sort is accidentally removed or reordered, Zeta would drop to the bottom and the assertion names[0] === 'Zeta' would immediately fail.
• Reuses the span:not(.mc-dm-row-favorite) selector pattern from the earlier favorites test — consistent with the test suite's established convention for extracting name text without the ★ character.
• Explicitly asserts the full post-sort order (['Zeta', 'alpha', 'Charlie']), not just names[0], verifying that non-favorites continue to follow alphabetical order below the pinned favorite.
• UI interaction (fireEvent.change + fireEvent.click) matches the pattern from the existing sorts DM peers by name test.

One minor observation: The sortDirection default is 'desc', and the test clicks getByTitle('Descending') to flip to 'asc'. For name/asc, 'Zeta' would sort last, making this the most adversarial possible case. The test would also pass under name/desc (where 'Zeta' would sort first without pinning), but the current choice of name/asc is strictly stronger — no suggestion to change it.

Implementation (MeshCoreDirectMessagesView.tsx) — unchanged since prior review; still correct

All observations from the prior review still apply: favoriteByKey memo is correct, sort guard is in the right place, nodes = [] default gives graceful degradation. No issues.

CSS (.mc-dm-row-favorite) — appropriate

flex: 0 0 auto + margin-right on a <span> inside a flex row keeps the star from stretching and prevents the adjacent name from shifting. Using var(--ctp-yellow) is consistent with how other "highlight" elements are colored in the MeshCore CSS. No concerns.

Overall

Approved. The prior review's one open suggestion (regression test for name-sort + favorites) has been addressed well. The implementation is complete, the test suite now covers both the default sort and the name sort paths for the favorites pin, and no issues remain.