Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore): server-side node favoriting with pin-to-top (#3588)

Tasks:

• Gather context and understand the PR
• Review schema and migration changes
• Review repository and backend changes
• Review route and API changes
• Review frontend changes
• Review test coverage
• Post final review feedback

Overall this is a well-implemented feature that correctly follows the codebase's patterns. The architecture decision to store the flag server-side only (with clear documentation of the reason) is correct and the implementation is consistent throughout the stack. Here are my findings:

Strengths

• Correct multi-source scoping: The composite PK (sourceId, publicKey) is respected at every layer — schema, repository, route, and tests. Per-source isolation is verified by meshcoreFavorite.perSource.test.ts.
• Stub-row insert pattern: Mirrors setNodeTelemetryConfig exactly — a node seen only in-memory can still be favorited. The contact-backfill in the route handler is a nice touch that seeds a name/type alongside the flag.
• Optimistic update in the frontend: setNodes(prev => prev.map(...)) in useMeshCore.ts:980 gives instant UI feedback, and recomputeNodes at line 342 carries the flag forward so a contact push can't transiently un-pin a favorite.
• Idempotent migrations: SQLite uses try/catch on duplicate column, PostgreSQL uses ADD COLUMN IF NOT EXISTS, MySQL uses information_schema — all three patterns are consistent with other migrations.

Issues Found

Minor Bug: isFavorite not preserved when in-memory contact overlays a DB node in getAllNodes

File: src/server/meshcoreManager.ts around line 3627–3639

for (const contact of this.contacts.values()) {
  const base = byKey.get(contact.publicKey);
  byKey.set(contact.publicKey, {
    ...base,           // isFavorite IS spread from base here
    publicKey: contact.publicKey,
    name: ...,
    advType: ...,
    lastHeard: ...,
    rssi: ...,
    snr: ...,
    latitude: ...,
    longitude: ...,
    // isFavorite not explicitly listed — relies on ...base spread
  });
}

This actually works correctly because ...base includes isFavorite and no later key overwrites it. However, when base is undefined (a contact that hasn't been persisted yet), the spread is a no-op and isFavorite will be undefined on the returned node. The downstream consumer in MeshCoreNodesView.tsx:66 uses n.isFavorite ?? false, so it degrades gracefully to false, but technically a live-only contact that just got favorited (triggering a stub-insert) could briefly show as un-favorited until the next getAllNodes snapshot.

This is a very edge case and the existing optimistic update in the hook mitigates it at the UI level, but it's worth being aware of.

Potential race: contact backfill in route handler can silently no-op

File: src/server/routes/meshcoreRoutes.ts:2432–2436

} catch (err) {
  logger.warn(
    `[API] favorite: contact backfill for ${publicKey.substring(0, 16)}… failed: ${(err as Error).message}`,
  );
}

The upsertNode failure is caught and swallowed (with a warn log). This is intentional — the favorite flag itself is still set even if the backfill fails. This is the right call, but the comment at line 2414 says "Backfill identity from the in-memory contact before seeding the stub row" — the upsertNode and the subsequent setNodeFavorite (which also inserts a stub) can both insert rows independently. If upsertNode fails, setNodeFavorite still inserts a stub without name/type, which is exactly what the PR intends. The logic is correct but slightly surprising to read.

isFavorite not forwarded in the in-memory overlay path for recomputeNodes

File: src/components/MeshCore/hooks/useMeshCore.ts:342–349

const favByKey = new Map(prev.map(n => [n.publicKey, n.isFavorite]));
...
const fav = favByKey.get(c.publicKey);
if (fav !== undefined) node.isFavorite = fav;

This only preserves isFavorite for contacts already present in the previous nodes state. A brand-new contact that arrives via a push event (and therefore has no entry in favByKey) will always get isFavorite: undefined, which renders as the empty star. This is the expected behavior since a new node can't yet be a favorite. This is fine; just noting the intentional asymmetry.

Observations & Suggestions

1. Missing isFavorite in MeshCoreActions type (useMeshCore.ts:99–166): The setNodeFavorite method is correctly added to the returned actions object at line 1397, but it isn't listed in the MeshCoreActions interface. The TypeScript type should be updated to include it, otherwise callers relying on the interface type would not see it as part of the contract.

2. favoriteBusy allows only one in-flight request globally (MeshCoreNodesView.tsx:130): if (!onToggleFavorite || favoriteBusy) return; — clicking any star while one request is in flight silently no-ops for all rows, not just the one being toggled. This is a reasonable UX choice for simplicity (avoids conflicting optimistic updates), but a per-key busy state would be more precise. Given this is a low-frequency action, the current approach is acceptable.

3. Search doesn't filter favorited pinned nodes separately (MeshCoreNodesView.tsx:221–226): sortRows (which includes the favorites-first sort) is applied before the search filter. This means favorites show first within search results too, which is the right behavior. ✓

4. No WebSocket push on favorite change: The API response at line 2441–2444 returns { success: true, data: { publicKey, sourceId, isFavorite } } but doesn't emit a socket event. This is fine for single-user scenarios (the optimistic update in the hook handles it), but in a multi-browser-session scenario, another tab won't see the change until the next snapshot poll. This limitation is consistent with how other local-only mutations work in the codebase.

5. Test: no assertion that upsertNode is NOT called when getContact returns undefined in meshcoreRoutes.test.ts:1248–1258. The test at line 1249 explicitly sets getContact to return undefined and the test passes — but it only asserts that setNodeFavorite was called and the response was 200. An explicit expect(upsertNode).not.toHaveBeenCalled() would make the test's intent more explicit.

Verdict

Approve with minor suggestions. The core feature is correct, well-tested, and properly integrated across all three DB backends. The TypeScript interface gap (point 1 above) is the only thing I'd call a real issue worth addressing before merge; the rest are observations.