Claude finished @Yeraze's task —— View job

PR Review: feat(notifications): surface device ClientNotifications + firmware 2.8 favorite/ignore cap

Tasks:

• Gather context and understand the PR
• Read changed files
• Analyze code quality, bugs, performance, security, and test coverage
• Post review feedback

Overall this is a well-architected, well-tested feature. The separation of concerns between the pure policy module, the manager handler, and the frontend toaster is clean. Below are findings by category.

Code Quality & Best Practices

1. Duplicated toastTypeForLevel function

toastTypeForLevel is defined twice with different magic numbers:

• src/server/services/clientNotificationPolicy.ts:70 — exported, uses NOTIFICATION_LEVEL constants
• src/components/DeviceNotificationToaster.tsx:24 — private, uses raw magic numbers (40, 30)

The frontend can't import from the backend, so some duplication is expected, but the frontend version uses 40 (ERROR) and 30 (WARNING) as bare literals rather than constants. Consider co-locating the level constants in a shared location (e.g., src/shared/notificationLevels.ts) or documenting the magic numbers more explicitly in the frontend copy to keep them in sync if firmware ever changes the proto enum.

2. Guard refusal && this.sourceId is redundant

In meshtasticManager.ts:5570, this.sourceId is typed as string (non-optional, line 445) and always initialized in the constructor (line 943-944). The && this.sourceId guard will never be falsy. The check is harmless but misleading — it implies sourceId could be null/undefined when it can't. Either drop the guard or add a comment explaining why it's there (e.g., "defensive, satisfies TS nullable guard").

3. Debug log in hot path

meshtasticProtobufService.ts:725-730 logs Object.keys(fromRadio) and a full JSON.stringify(fromRadio, ...) at debug level for every unrecognized FromRadio variant. This is in the hot packet-processing path. While debug level is off by default, JSON.stringify on a protobuf object is not cheap. The clientNotification branch was added at line 794 but comes after this debug block at 726 — so any clientNotification message will NOT trigger the debug dump (it has packet, myInfo, etc. all absent but hits the new branch before the final else). This seems correct but worth a second look: the condition at line 726 (!fromRadio.packet && !fromRadio.myInfo && ...) doesn't exclude clientNotification, so if the protobuf object happened to have none of the earlier "known" keys set, it would dump — but since clientNotification is caught before the final else, it would never reach the dump. The ordering is correct but fragile; adding !((fromRadio as any).clientNotification) to the dump guard would make intent explicit.

Potential Bugs

4. parseProtectedCapRefusal regex only matches exactly 8 hex digits

const PROTECTED_CAP_RE = /can't (favorite|ignore) 0x([0-9a-f]{8}): protected-node limit/i;

The firmware format string is "Can't %s 0x%08x: ..." (zero-padded to 8 digits), so this is correct for well-formed firmware output. However if firmware ever emits a shorter node ID (e.g., 0x1 for a very low node number) the regex won't match and MeshMonitor won't revert the flag. This is a minor edge-case concern given the firmware format string, but {1,8} would be more defensive.

5. Toast emitted even on cap-refusal revert failure

In meshtasticManager.ts:5563-5598, when parseProtectedCapRefusal returns a match, the code:

1. Tries to revert the DB flag (lines 5572-5577)
2. Catches any error (5584-5586)
3. Then falls through to toast suppression check (5591)

If the DB revert fails, the toast is still emitted — the user sees "can't favorite…" but MeshMonitor's state may be inconsistent. This is arguably the right behavior (surface the issue), but worth a comment since the current code silently diverges from the optimistic state on DB error.

6. Missing test: DB revert failure path

The test suite (meshtasticManager.clientNotification.test.ts) covers happy paths for revert and surfacing normal warnings, but doesn't cover the case where setNodeFavorite rejects — ensuring the error is logged and the toast still fires.

Performance

7. ToastThrottle cleanup threshold is coarse

The map cleanup at clientNotificationPolicy.ts:114 only runs when this.last.size > 200. For a long-lived server with many distinct notification messages, the map could hold up to 399 entries before cleanup. Given typical firmware notification patterns this is acceptable, but the threshold and rationale could be documented.

8. ToastThrottle uses string concatenation keys

The throttle key is ${this.sourceId ?? '__default__'}::${message} (meshtasticManager.ts:5592). For clientNotification messages with variable text (e.g., "You can send again in 3 mins" where the timer varies), two messages that differ only in seconds would each get their own throttle slot. This is likely the desired behavior (don't suppress genuinely new information), but the duty-cycle warning example in the PR description ("You can send again in 3 mins") has a fixed string from the firmware, so it works fine.

Security

9. Log injection via device-controlled message field

meshtasticManager.ts:5565 logs:

logger.info(`🔔 [${this.sourceId}] Device notification (level ${data.level}): ${message}`);

And dataEventEmitter.ts:218:

logger.info(`[DataEventEmitter] Client notification (level ${data.level}): ${data.message}`);

The message field comes directly from the connected Meshtastic device (attacker-controlled if the device is compromised or a MitM replaces bytes). Depending on the logger implementation, a crafted message with newlines or ANSI escape codes could pollute log output. This is a low-severity concern since log injection is rarely exploitable, but sanitizing or truncating device-originated strings before logging is a best practice.

10. Toast message length is not bounded

DeviceNotificationToaster.tsx:39 checks data.message.length === 0 but not an upper bound. A device sending a very long notification string would render an oversized toast. Consider truncating at a reasonable limit (e.g., 500 chars) with an ellipsis before passing to showToast.

Test Coverage

The test coverage is solid overall:

• clientNotificationPolicy.test.ts — comprehensive unit tests for suppression, parsing, throttle, and level mapping ✅
• meshtasticManager.clientNotification.test.ts — covers all key behaviors (revert favorite, revert ignore, surface warning, suppress, dedupe) ✅
• meshtasticProtobufService.test.ts — adds dispatch test + snr_q4 regression guard ✅

Missing test cases (minor):

• DB revert failure path (noted in feat: telemetry parsing, direct messages, danger zone, and UX improvements #6 above)
• shouldSuppressToast with message being undefined (currently guarded by ?? '' but not tested directly)
• DeviceNotificationToaster has no unit test — component renders null so there's nothing to snapshot, but the socket listener registration/cleanup and level mapping logic could use a lightweight React Testing Library test

Summary

This is a clean, well-designed feature with good test coverage and clear documentation of the design constraints. The main actionable items are:

• Bug (minor): Consider widening the hex digit match in PROTECTED_CAP_RE to {1,8} for robustness
• Security (low): Sanitize or truncate device-controlled message text before logging and before passing to showToast
• Maintainability: Eliminate the duplicated toastTypeForLevel logic or add constants to keep the frontend copy in sync

Thanks for the thorough review — triaged all findings. Addressed in e1c9894:

Fixed

• feat: Add GitHub Container Registry publishing and pre-built images #9 / fix: Change Docker publishing to trigger only on GitHub Releases #10 (log injection + unbounded toast): Added sanitizeNotificationMessage() applied once in handleClientNotification before logging and forwarding — strips control chars (newlines/ANSI), collapses whitespace, and truncates to 500 chars. The emitted toast and both log sites now use the sanitized value.
• feat: traceroute request tracking and auth documentation #4 (regex 8 vs 1–8 hex): Widened PROTECTED_CAP_RE to {1,8} so a short/non-zero-padded id still reconciles. Added a 0x1 test.
• "Update Claude PR Assistant workflow" #3 (debug dump): Added !clientNotification to the unknown-FromRadio dump guard so it never hits the debug JSON.stringify — you were right that, since the dump runs before the dispatch chain, clientNotification messages were reaching it at debug level.
• feat: traceroute highlighting and UI improvements #2 (redundant guard): Dropped && this.sourceId (it's public sourceId: string, always set) and left a one-line comment.
• feat: add telemetry graphs, node sorting, and fix duplication #5 (toast on revert failure): Kept the behavior (surface the refusal even if the DB revert throws) and documented it in the catch block.
• "Claude PR Assistant workflow" #1 (duplicated toastTypeForLevel): Frontend can't import server code, so the copy stays, but the magic numbers are now named constants with a "mirrors backend NOTIFICATION_LEVEL" note.
• feat: telemetry parsing, direct messages, danger zone, and UX improvements #6 (missing test): Added the DB-revert-failure path (revert rejects → no throw, toast still fires). Also added sanitizer tests.

Considered, leaving as-is

• feat: Messages UI redesign, telemetry fixes, and emoji support #7 (throttle cleanup threshold): Bounded at ~200 in-window keys with expired-entry purge; added a comment on the rationale. For this notification volume it's not worth more machinery.
• feat: enhance device info display and Messages UI improvements #8 (variable-text throttle keys): Intentional — the duty-cycle countdown ("…again in 3 mins") carries genuinely new information each minute, so keying on full text (toasting the 3/2/1-min steps) is preferable to over-suppressing. The truly recurring noise (power-save "sleeping for N interval") is dropped outright by shouldSuppressToast, not relied on the throttle.

Full Vitest suite green (6973 passed) after the changes.

Claude finished @Yeraze's task —— View job

PR Review: feat(notifications): surface device ClientNotifications + firmware 2.8 favorite/ignore cap

Tasks:

• Gather context and understand the PR
• Read changed files
• Analyze code quality, bugs, performance, security, and test coverage
• Post review feedback

This is a well-structured follow-up review on commit e1c9894d which addressed all 10 findings from the prior review. The code is in good shape. Below are findings on the current state.

Code Quality & Best Practices

1. dataEventEmitter.emitClientNotification logs the unsanitized data.message

dataEventEmitter.ts:218 logs:

logger.info(`[DataEventEmitter] Client notification (level ${data.level}): ${data.message}`);

sanitizeNotificationMessage() is applied in meshtasticManager.ts:5567 before calling emitClientNotification, so the data.message passed in is already sanitized. However the log in dataEventEmitter could mislead a future developer into thinking the value is raw. A brief inline comment (// message is pre-sanitized by the caller) or just relying on the module-level comment would help. Low severity, but worth noting.

2. shouldSuppressToast re-trims after sanitization

In handleClientNotification, sanitizeNotificationMessage is applied first (which trims), then shouldSuppressToast is called with { ...data, message }. Inside shouldSuppressToast (line 64), the message is trimmed again:

const msg = (n.message ?? '').trim();

This double-trim is harmless but slightly redundant. Since sanitizeNotificationMessage always returns a trimmed string, the internal .trim() in shouldSuppressToast is defensive but unnecessary after the pre-sanitization contract. Not actionable — the defensive internal trim is good practice in a pure helper.

3. DeviceNotificationToaster listens on all sources

DeviceNotificationToaster.tsx subscribes to client-notification events on the global socket without filtering by sourceId. The server-side WS forwarding scopes events to per-source rooms (as mentioned in the PR description), but the frontend component doesn't assert the sourceId either. If a multi-source user has multiple source tabs open, each tab's component would receive notifications from all sources depending on the room subscription. This is likely handled correctly server-side but worth verifying: does the WS room scoping ensure each client only receives events for its joined source, or does client-notification broadcast globally?

Potential Bugs

4. parseProtectedCapRefusal is called on the sanitized message

sanitizeNotificationMessage converts control chars to spaces and collapses whitespace. The firmware cap-refusal string "Can't favorite 0xdeadbeef: protected-node limit (118) reached" has no control chars or multi-whitespace, so PROTECTED_CAP_RE will match correctly. However, if firmware ever includes unusual characters before the apostrophe (e.g., a Unicode quote '), the regex won't match. The current behavior is correct for known firmware output — this is a minor robustness note, not a bug.

5. Missing logRecord and queueStatus in the unknown-FromRadio debug guard

The debug dump guard at meshtasticProtobufService.ts:727-729 excludes clientNotification but does NOT exclude logRecord, queueStatus, or fileInfo — all three are in the hasLogRecord/hasQueueStatus/hasFileInfo diagnostics just above. This means a logRecord, queueStatus, or fileInfo FromRadio message will hit the JSON.stringify debug dump before falling through to the final else branch. This is a pre-existing issue (not introduced by this PR), but the PR touched this area — a small improvement opportunity if any of those variants are frequent.

Performance

6. ToastThrottle instance is per-manager, not shared

MeshtasticManager constructs a new ToastThrottle() as a private field. This is correct — throttles are per-source. No issue here.

7. Toast duration hardcoded at 8000ms

DeviceNotificationToaster.tsx:45 passes 8000 as the toast duration. This is fine for a first pass, but error-level notifications (cap refusals, duplicate-key warnings) might warrant a longer display time since they're actionable. Non-blocking observation.

Security

8. Sanitized message forwarded correctly

The sanitized message value flows through: handleClientNotification → dataEventEmitter.emitClientNotification → WS event → DeviceNotificationToaster. The sanitization chain is correct. The DeviceNotificationToaster handler at line 44 also guards data.message.length === 0 before calling showToast.

One minor gap: DeviceNotificationToaster:44 checks data.message.length === 0 but the server-side suppression already drops empty messages. The frontend guard is defensive and correct, but the length cap at 500 chars on the server means the frontend will never see a message longer than ~500 characters (plus the … char). ✅

Test Coverage

Well covered overall:

• clientNotificationPolicy.test.ts — suppression patterns, sanitizer (with control chars, ANSI, overlong, empty/undefined), throttle behavior, level mapping, regex parsing (including short hex id 0x1, case-insensitive, verify exclusion) ✅
• meshtasticManager.clientNotification.test.ts — revert favorite, revert ignore, DB revert failure path (new in e1c9894), suppress recurring, dedupe ✅
• meshtasticProtobufService.test.ts — dispatch path and snr_q4 regression guard ✅

Minor gaps (non-blocking):

• DeviceNotificationToaster has no test. The socket listener registration and cleanup path (useEffect teardown) is the main testable behavior. A lightweight RTL test with a mock socket would add confidence, but the component is simple enough that visual inspection suffices.
• No test for the case where data.message is absent or not a string on the frontend handler (guarded by typeof data.message !== 'string'). Works correctly — just not explicitly exercised.

Summary

The previous 10 findings were all addressed correctly in e1c9894d. The sanitization, regex widening, debug guard fix, and DB-revert test are all solid. The remaining observations are minor:

• Cosmetic: dataEventEmitter log comment to clarify message is pre-sanitized
• Investigation: Verify multi-source WS room scoping for client-notification (finding "Update Claude PR Assistant workflow" #3) — this may already be correctly handled server-side
• Pre-existing: logRecord/queueStatus/fileInfo not excluded from the debug dump guard (finding feat: add telemetry graphs, node sorting, and fix duplication #5)

The feature is well-implemented, well-tested, and ready to merge pending the multi-source room scoping check.

Thanks for the follow-up. The three remaining items are non-blocking; closing the loop:

• "Update Claude PR Assistant workflow" #3 (verify multi-source WS scoping): Confirmed correctly scoped server-side. handleClientNotification calls emitClientNotification(..., this.sourceId), and webSocketService skips any event whose sourceId isn't one of the socket's joined source: rooms (webSocketService.ts ~180–185). A per-source tab joins exactly one room and only receives that source's notifications; the legacy single-source view has just one source anyway. No global broadcast. Since the payload itself is only routed (not filtered) by sourceId, I left the frontend handler as-is rather than add redundant client-side filtering.
• "Claude PR Assistant workflow" #1 (dataEventEmitter logs data.message): By design — the value is already sanitized by the caller before emitClientNotification is reached, so the log/forward are both safe. Cosmetic; not worth a separate CI cycle on its own.
• feat: add telemetry graphs, node sorting, and fix duplication #5 (logRecord/queueStatus/fileInfo not excluded from the debug dump): Pre-existing and debug-level only, as you noted — leaving it out of this PR to keep the diff focused on the feature. Good candidate for a small standalone cleanup.

CI is green on the merged head (867182a8) and mergeState is CLEAN, so this should be ready to merge.