Claude finished @Yeraze's task —— View job

PR Review

Tasks

• Read changed files
• Review MeshCoreDirectMessagesView changes
• Review MessagesTab changes
• Review NodesTab changes
• Review test coverage
• Post final review

Summary

This is a solid, well-scoped fix. The overall approach is correct: keep nodes visible in sidebars, replace the messaging UI with a details-only view when the node can't receive DMs. The implementation correctly reuses the existing mqttReadOnly pattern in MessagesTab and adds a clean symmetric gate in MeshCoreDirectMessagesView. Test coverage for the MeshCore path is good. A few notes below.

Code Quality

MeshCoreDirectMessagesView.tsx:252 — isSelectedRepeater derivation

const isSelectedRepeater = selected ? contactsByKey.get(selected)?.advType === 2 : false;

The magic number 2 is used directly. The existing codebase already has advType !== 3 inline on line 186, so this is consistent with the surrounding pattern — but if there's a MESHCORE_ADV_TYPE constants object anywhere it would be worth using. Minor; not a blocker.

MessagesTab.tsx:764 — effectiveReadOnly derivation

const effectiveReadOnly = mqttReadOnly || selectedNode?.isUnmessagable === true;

The === true guard is defensive against undefined coercion — good. One subtlety: selectedNode is derived from nodes.find(n => n.user?.id === selectedDMNode) at line 757. If a node is selected but not yet in the local nodes array (e.g. a fresh selection before the next data poll), selectedNode is undefined, so effectiveReadOnly falls back to mqttReadOnly only — meaning the composer would momentarily show for an unmessagable node if the node list hasn't loaded yet. This is a pre-existing race condition with the old mqttReadOnly path and not introduced by this PR, but worth noting.

NodesTab.tsx:1689 — permission check ordering

{hasPermission('messages', 'read') && !node.isUnmessagable && (

Checking messages: read before isUnmessagable is the right order — cheap permission check first, then the node-level gate. Looks good.

Potential Issues

Unmessagable node still appears in Messages DM sidebar

MessagesTab hides the composer when an unmessagable node is selected, but the node can still appear in the DM sidebar if it has prior message history. The NodesTab 💬 button is now correctly hidden, but a user could still navigate directly to the DM conversation (e.g. from browser history or a bookmarked URL). This is consistent with the stated design ("keep the node selectable in the DM list and still show its per-node telemetry"), but it's worth confirming this is intentional — the read-only mode shows the telemetry, which is correct, but no notice is displayed to explain why the compose box is absent (unlike the MeshCore path which shows the .meshcore-dm-no-messaging notice). A user selecting an unmessagable node in MessagesTab will just see... nothing where the message log usually is, with no explanation.

MeshCoreDirectMessagesView.tsx:386-400 — notice placement

{isSelectedRepeater ? (
  <div className="meshcore-dm-no-messaging" role="note">
    {t('meshcore.repeater_no_messaging', 'Repeaters cannot receive...')}
  </div>
) : (
  <MeshCoreMessageStream ... />
)}
<div className="meshcore-detail-pane">
  <MeshCoreContactDetailPanel ... />
  ...
</div>

The notice renders above the detail pane, which is the right visual order. The role="note" is semantically appropriate. ✓

i18n / Localization

meshcore.repeater_no_messaging key is not in any locale file.

The t(key, fallback) pattern will use the English hardcoded fallback for all users, so it won't break — but for completeness the key should be added to public/locales/en.json (and ideally the other locale files, at least with the English text as a placeholder). This is consistent with the project's i18n approach where keys live in locale files.

None of the other locale files (de.json, es.json, fr.json, etc.) have this key either — only en.json would need it as the canonical source. Other translators can then pick it up.

Test Coverage

MeshCore tests (4 new in MeshCoreDirectMessagesView.test.tsx): All four cover the stated behavior:

• Repeater stays listed ✓
• Composer hidden + notice shown on repeater select ✓
• Composer still shown for companions ✓
• Telemetry still mounts for a selected repeater ✓

The test for composer presence uses queryByPlaceholderText('Type a message…') (line 449) — this relies on the placeholder text being static. If it's ever translated/changed that assertion would break, but it's a reasonable approach given the mock setup.

Meshtastic (MessagesTab/NodesTab) — no new tests. The PR note explains that MessagesTab has no render harness due to its size. The change is a one-liner reuse of an already-tested code path (mqttReadOnly), so the risk is low. Accepted.

Performance

No concerns. isSelectedRepeater is a single map lookup on every render (O(1)), not memoized — that's fine given how cheap it is and how infrequently selection changes.

Security

No concerns. No new API surface, no new data paths. The change is purely UI-gating.

Minor Nits

1. MeshCorePage.css:329 — font-style: italic for the notice is a slightly unconventional choice for a status/notice element (usually a distinct background or border-left is used). Not wrong, but may look understated. Low priority.

2. MessagesTab.tsx:1357 — the messages container uses display: effectiveReadOnly ? 'none' : undefined. Hiding with display: none rather than conditional rendering means the DOM nodes (including refs) are still present — this is intentional and consistent with the original mqttReadOnly behavior. ✓

Verdict

Approve with minor suggestions. The core logic is correct, the approach is clean, and the MeshCore test coverage is solid. The main follow-up worth addressing before or after merge:

1. Add meshcore.repeater_no_messaging to public/locales/en.json.
2. Consider adding a brief notice in MessagesTab for unmessagable nodes (similar to the MeshCore notice) so users understand why the compose area is absent.

Claude finished @Yeraze's task —— View job

PR Review

Tasks

• Read changed files
• Review MeshCoreDirectMessagesView changes
• Review MessagesTab changes
• Review NodesTab changes
• Review test coverage
• Post final review

Summary

This is a well-scoped fix with clean implementation. The approach is correct: keep nodes visible in sidebars, replace the messaging UI with a details-only view when the node can't receive DMs. The core implementation is solid and the MeshCore test coverage is thorough.

Code Quality

MeshCoreDirectMessagesView.tsx:252 — magic number for advType

const isSelectedRepeater = selected ? contactsByKey.get(selected)?.advType === 2 : false;

The value 2 is a bare magic number. The existing code on line 186 already uses c.advType !== 3 inline (for room servers), so this is stylistically consistent — but worth noting that neither has a named constant. If a MESHCORE_ADV_TYPE enum or constants object exists or is planned, this would benefit from it. Not a blocker.

MessagesTab.tsx:764 — effectiveReadOnly derivation

const effectiveReadOnly = mqttReadOnly || selectedNode?.isUnmessagable === true;

Good defensive use of === true to guard against undefined. One edge case: selectedNode is derived from nodes.find(n => n.user?.id === selectedDMNode) at line 757. If a node is selected but not yet present in the local nodes array (e.g., a fresh selection before the next data poll), selectedNode will be undefined and effectiveReadOnly falls back to mqttReadOnly only — meaning the composer could momentarily show for an unmessagable node. This is a pre-existing race shared with the mqttReadOnly path and is not introduced by this PR, just worth noting.

NodesTab.tsx:1689 — permission check ordering

{hasPermission('messages', 'read') && !node.isUnmessagable && (

The permission check precedes the node-level gate, which is correct ordering: cheap first, specific second. ✓

Potential Issues

No notice for unmessagable nodes in MessagesTab

The MeshCore path shows a .meshcore-dm-no-messaging notice explaining why the composer is absent. The Meshtastic path (MessagesTab) shows nothing — when an unmessagable node is selected, the messages container is hidden via display: none (line 1357) and the send area is gated by !effectiveReadOnly, but there is no UI explanation. A user who navigates directly to an unmessagable node's conversation (e.g. via URL/history) will see a mostly blank pane with only telemetry and no indication of why.

This is a consistency gap between the two paths. Consider adding a brief notice similar to the MeshCore one — something parallel to the existing MQTT-bridge notice pattern at line 1340.

Unmessagable nodes still appear in the DM sidebar

Per the PR description this is intentional ("keep the node selectable in the DM list"), but it means a user could reach an unmessagable node conversation without ever touching the Nodes tab 💬 button. The NodesTab correctly hides the 💬 button, but the DM sidebar itself doesn't visually distinguish these nodes from messageable ones. Again, if this is by design, a sidebar indicator (similar to the role icons) would help users understand why selecting one shows a different view.

i18n / Localization

meshcore.repeater_no_messaging key is missing from public/locales/en.json

t('meshcore.repeater_no_messaging', 'Repeaters cannot receive direct messages — showing node details only.')

The t(key, fallback) pattern works fine without the key in the locale file — English users get the fallback. However, it means the key is invisible to translators and won't appear in the i18n pipeline. It should be added to public/locales/en.json as the canonical source so translators can pick it up for other locales.

Test Coverage

MeshCore (4 new tests in MeshCoreDirectMessagesView.test.tsx) — all four cover the stated behavior correctly:

• keeps the repeater listed in the contact sidebar ✓
• hides the message composer and shows a notice when a repeater is selected ✓
• still renders the message composer for a normal (companion) contact ✓
• still renders node details (telemetry) for a selected repeater ✓

Note: the composer presence test (line 466) uses queryByPlaceholderText('Type a message…') which ties the assertion to a static English placeholder string. This is fine given the mocked t() but would break if the placeholder is ever i18n'd. Accepted.

Meshtastic (MessagesTab/NodesTab) — no new tests. The PR note explains the size constraint on MessagesTab. The change is a one-liner reuse of the established mqttReadOnly code path, tsc-verified. The risk is low given the breadth of effectiveReadOnly usage already exercised. Accepted.

Performance

No concerns. isSelectedRepeater is a single O(1) Map lookup on every render. Not memoized, which is correct — the computation is trivial and selection changes are infrequent.

Security

No concerns. No new API surface, no new data flows. Purely UI-gating over pre-existing fields.

Minor Nits

1. MeshCorePage.css:329 — font-style: italic for the notice is a bit understated for a status/notice element; a left border or subtle background would be more conventional. Low priority.

2. MessagesTab.tsx:1357 — hiding with display: none rather than conditional rendering means DOM nodes (including refs) remain mounted. This is intentional and consistent with the pre-existing mqttReadOnly behavior. ✓

Verdict

Approve with two minor follow-ups:

1. Add meshcore.repeater_no_messaging to public/locales/en.json so the key enters the i18n pipeline.
2. Consider a brief notice in MessagesTab for unmessagable nodes (mirroring the MeshCore path) so users understand why the message area is absent when they select such a node.

Neither is blocking — the core logic is correct and the feature is immediately safe to merge.