Claude finished @Yeraze's task —— View job

PR Review

Tasks:

• Gather context on PR
• Read changed files
• Analyze code and provide review feedback

Summary

This PR correctly identifies and fixes a real bug: lowBatteryNotificationService and inactiveNodeNotificationService only iterated over sourceManagerRegistry, which never contains MeshCore managers. The fix — combining both registries at scan time — is the right approach given the architectural split between the two registries.

The fix is clean, minimal, and well-tested. A few observations below.

Code Quality

Duplicate pattern in two services (lowBatteryNotificationService.ts:112-116, inactiveNodeNotificationService.ts:110-114)

Both services now contain identical 5-line blocks to build the combined manager list:

type MinimalManager = { sourceId: string; sourceType: string };
const managers: MinimalManager[] = [
  ...sourceManagerRegistry.getAllManagers(),
  ...meshcoreManagerRegistry.list().map(m => ({ sourceId: m.sourceId, sourceType: 'meshcore' as const })),
];

This is fine for two callsites, but if a third notification service appears (e.g., a future "node offline" alert), the same block would need to be copied again. Consider extracting a shared helper (e.g., getAllSourceManagers() in a shared utilities module or in meshcoreRegistry.ts itself) to centralise this pattern. Not a blocker for merging — just worth noting.

Type alias is re-declared in both files

type MinimalManager = { sourceId: string; sourceType: string } is declared inside each private method. Moving it to module scope in each file, or into a shared types file, would be cleaner.

Potential Issues

sourceManagerRegistry.getAllManagers() already includes Meshtastic-only sources — duplicate sourceId risk is low but worth confirming

The combined array could theoretically contain the same sourceId twice if a manager somehow appeared in both registries. Based on the current architecture this can't happen (MeshCore managers are only in meshcoreManagerRegistry), but a quick comment or assert would make that invariant explicit.

nodeNum: 0 for MeshCore alerts (lowBatteryNotificationService.ts:264, inactiveNodeNotificationService.ts:261)

MeshCore nodes don't have a Meshtastic-style nodeNum, so 0 is used as a placeholder. This is consistent with the existing MeshCore data model, but 0 is also a technically valid (if unusual) node number. A sentinel like -1 or null would be unambiguous. Again, not a new issue introduced by this PR, but worth documenting.

Test Coverage

Test coverage is good and the structural change is important: MeshCore tests now set up managers via mockMeshcoreList (the production path) rather than mockGetAllManagers (which would never be hit in production). This correctly validates the real code path.

A few minor observations:

• The beforeEach in lowBatteryNotificationService.test.ts:77-78 sets mockGetAllManagers to a single Meshtastic source and mockMeshcoreList to empty ([]). The MeshCore describe block (line 223-228) overrides both for its tests. This is clear and intentional — good.

• inactiveNodeNotificationService.test.ts has a leftover mockGetInactiveMonitoredNodesAsync mock alias (line 29) that duplicates mockGetInactiveMonitoredNodesAsync (re-aliased to nodes.getInactiveMonitoredNodes on line 30-31). The top-level const on line 16 is declared but the binding on line 29 (getInactiveMonitoredNodesAsync) refers to a different path (databaseService.getInactiveMonitoredNodesAsync) which is the old pre-4.x direct-method path. This is likely dead code from a prior test refactor. It doesn't affect test correctness but could be cleaned up.

Security / Permissions

The permission check (databaseService.checkPermissionAsync) is correctly preserved for MeshCore sources — users who don't have nodes:read on a MeshCore source are skipped before any alert query runs. Good.

Overall Assessment

Approve. The root cause is correctly identified, the fix is minimal and targeted, tests cover the production code path, and no regressions are introduced. The suggestions above are non-blocking improvements.

Claude finished @Yeraze's task —— View job

PR Review

Tasks:

• Gather context on PR
• Read changed files
• Analyze code and provide review feedback

Summary

This PR correctly identifies and fixes a real bug: lowBatteryNotificationService and inactiveNodeNotificationService only iterated over sourceManagerRegistry, which never contains MeshCore managers. The fix — combining both registries at scan time — is the right approach given the architectural split between the two registries.

The fix is clean, minimal, and well-tested. A few observations below.

Code Quality

Duplicate pattern in two services (lowBatteryNotificationService.ts:112-116, inactiveNodeNotificationService.ts:110-114)

Both services now contain identical 5-line blocks to build the combined manager list:

type MinimalManager = { sourceId: string; sourceType: string };
const managers: MinimalManager[] = [
  ...sourceManagerRegistry.getAllManagers(),
  ...meshcoreManagerRegistry.list().map(m => ({ sourceId: m.sourceId, sourceType: 'meshcore' as const })),
];

This is fine for two callsites, but if a third notification service appears (e.g., a future "node offline" alert), the same block would need to be copied again. Consider extracting a shared helper (e.g., getAllSourceManagers() in a shared utilities module or in meshcoreRegistry.ts itself) to centralise this pattern. Not a blocker for merging — just worth noting.

Type alias is re-declared in both files

type MinimalManager = { sourceId: string; sourceType: string } is declared inside each private method. Moving it to module scope in each file, or into a shared types file, would be cleaner.

Potential Issues

sourceManagerRegistry.getAllManagers() already includes Meshtastic-only sources — duplicate sourceId risk is low but worth confirming

The combined array could theoretically contain the same sourceId twice if a manager somehow appeared in both registries. Based on the current architecture this can't happen (MeshCore managers are only in meshcoreManagerRegistry), but a quick comment or assert would make that invariant explicit.

nodeNum: 0 for MeshCore alerts (lowBatteryNotificationService.ts:264, inactiveNodeNotificationService.ts:261)

MeshCore nodes don't have a Meshtastic-style nodeNum, so 0 is used as a placeholder. This is consistent with the existing MeshCore data model, but 0 is also a technically valid (if unusual) node number. A sentinel like -1 or null would be unambiguous. Again, not a new issue introduced by this PR, but worth documenting.

Test Coverage

Test coverage is good and the structural change is important: MeshCore tests now set up managers via mockMeshcoreList (the production path) rather than mockGetAllManagers (which would never be hit in production). This correctly validates the real code path.

A few minor observations:

• The beforeEach in lowBatteryNotificationService.test.ts:77-78 sets mockGetAllManagers to a single Meshtastic source and mockMeshcoreList to empty ([]). The MeshCore describe block (line 223-228) overrides both for its tests. This is clear and intentional — good.

• inactiveNodeNotificationService.test.ts has a leftover mock alias issue on line 28-30: getInactiveMonitoredNodesAsync is declared as a const on line 15 and then also bound to databaseService.getInactiveMonitoredNodesAsync on line 28 and to databaseService.nodes.getInactiveMonitoredNodes on line 30. Both the top-level getInactiveMonitoredNodesAsync path (line 28) and the nodes.getInactiveMonitoredNodes path (line 30) point to the same mock function, but line 28 exposes a now-dead legacy path (databaseService.getInactiveMonitoredNodesAsync) that the service no longer calls. It doesn't affect test correctness but could confuse future readers.

Security / Permissions

The permission check (databaseService.checkPermissionAsync) is correctly preserved for MeshCore sources — users who don't have nodes:read on a MeshCore source are skipped before any alert query runs. Good.

Overall Assessment

Approve. The root cause is correctly identified, the fix is minimal and targeted, tests cover the production code path, and no regressions are introduced. The suggestions above are non-blocking improvements.

Claude finished @Yeraze's task —— View job

PR Review

Tasks:

• Gather context on PR
• Read changed files
• Analyze code and provide review feedback

Summary

This PR correctly identifies and fixes a real bug: lowBatteryNotificationService and inactiveNodeNotificationService only iterated over sourceManagerRegistry, which never contains MeshCore managers. The fix — combining both registries when building the manager list — is the right approach given the architectural split.

The fix is clean, minimal, and well-tested. A few observations below.

Code Quality

Duplicate pattern across two services (lowBatteryNotificationService.ts:112-116, inactiveNodeNotificationService.ts:110-114)

Both services now contain identical 5-line blocks:

type MinimalManager = { sourceId: string; sourceType: string };
const managers: MinimalManager[] = [
  ...sourceManagerRegistry.getAllManagers(),
  ...meshcoreManagerRegistry.list().map(m => ({ sourceId: m.sourceId, sourceType: 'meshcore' as const })),
];

This is fine for two callsites, but if a third notification service is added, the pattern needs to be copied again. Consider extracting a shared helper (e.g., getAllSourceManagers() in a shared utilities module or in meshcoreRegistry.ts itself). Not a blocker — just worth tracking.

type MinimalManager is function-scoped in both files

It's declared inside each private method. Moving it to module scope, or into a shared types file, would be cleaner but is not blocking.

Potential Issues

nodeNum: 0 placeholder for MeshCore alerts (lowBatteryNotificationService.ts:264, inactiveNodeNotificationService.ts:260)

MeshCore nodes don't have a Meshtastic-style nodeNum, so 0 is used as a placeholder. 0 is technically a valid (if unusual) node number. A sentinel like -1 or null would be unambiguous. This is pre-existing behaviour, not introduced by this PR, but worth documenting.

Duplicate sourceId risk

The combined array could theoretically contain the same sourceId twice if a manager somehow appeared in both registries. Based on the current architecture this cannot happen (MeshCore managers are exclusively in meshcoreManagerRegistry), but a brief comment noting this invariant would make the intent clear to future maintainers.

Test Coverage

Test coverage is good. Importantly, the MeshCore tests now use mockMeshcoreList (the real production path) rather than mockGetAllManagers (which the service never populates with MeshCore entries). This correctly validates the actual code path.

Minor observation — inactiveNodeNotificationService.test.ts line 15

The variable mockGetInactiveMonitoredNodesAsync (line 15) is declared as a const but then reused as the mock for databaseService.nodes.getInactiveMonitoredNodes (line 29). The variable name carries an Async suffix from the old pre-4.x direct-method path (databaseService.getInactiveMonitoredNodesAsync). The mock still works correctly because it's bound to the right repository path on line 29, but the name could confuse future readers. Low priority, easy cleanup when someone is next in the file.

Security / Permissions

The databaseService.checkPermissionAsync gate is correctly preserved for MeshCore sources — users lacking nodes:read on a MeshCore source are skipped before any alert query runs.

Overall Assessment

Approve. Root cause is correctly identified, fix is minimal and targeted, tests cover the real production code path, and no regressions are introduced. All suggestions above are non-blocking.