Claude finished @Yeraze's task —— View job

PR Review

Summary

This PR fixes a legitimate bug where "Newly Found Node" notifications never fired for directly-connected (serial/TCP) Meshtastic nodes. The root cause diagnosis is accurate and the fix is well-structured. Here's a detailed breakdown:

Code Quality & Design

maybeNotifyNewNode() extraction (database.ts:2354)
The shared helper is well-designed. The structural typing approach for nodeData (accepting a minimal shape rather than requiring a full DbNode) is a good call given the two different DbNode definitions in scope. The dedup-before-persist ordering (newNodeNotifiedSet.add(nodeNum) at line 2387 before the import().then()) is correct — it prevents a second caller racing in before the async notification resolves.

upsertNodeAsync() short-circuit (database.ts:2416–2432)
The needsCheck optimization is solid — once a node has been seen, subsequent upserts skip the pre-read entirely. The comment explaining why this matters is clear.

this.sources.getSource() inside maybeNotifyNewNode() (database.ts:2392)
Minor concern: this.sources getter throws 'Database not initialized' if sourcesRepo is null. Inside the dynamic import().then() callback (already fire-and-forget), this would be caught by the .catch() at line 2404 and silently fall back — so it's safe but could produce a confusing error log early in startup. Not a bug, but worth noting.

Dynamic import() inside maybeNotifyNewNode() (database.ts:2389)
This avoids a circular import issue (database.ts → notificationService.js → potentially database.ts), which is the right solution. The pattern is already established for the MQTT path.

Potential Issues

newNodeNotifiedSet is keyed by nodeNum only, not (nodeNum, sourceId) (database.ts:2372, 2387)
If the same physical node appears on two different sources (e.g., node !aabbccdd heard on both a direct serial connection and MQTT), the dedup set will suppress the notification for the second source. Given the intent is "notify once per node per process lifetime" this is likely intentional behavior, but it's worth calling out explicitly: a node discovered on source A will never trigger a notification when later discovered on source B in the same process run. The PR description doesn't mention this cross-source behavior.

Race condition between getNode() and nodes.upsertNode() (database.ts:2425–2427)
For PostgreSQL/MySQL, getNode() reads from the in-memory cache (nodesCache), while nodes.upsertNode() is an async DB write. If a concurrent upsert for the same node completes between the getNode cache read and the upsertNode call, existingNode would be stale. This is a pre-existing pattern in the codebase and the dedup set prevents double-notification, so this is acceptable but worth documenting.

Ghost suppression bypass in upsertNodeAsync() (database.ts:2416)
The sync upsertNode() path has ghost suppression logic (isNodeSuppressed()) at line 2205, but upsertNodeAsync() delegates directly to this.nodes.upsertNode() (the repository layer), which doesn't have that guard. Nodes suppressed via the ghost detection mechanism can bypass suppression when written through upsertNodeAsync(). This is a pre-existing gap — the repository layer never had ghost suppression — but the PR widens exposure to it by routing 31 call sites through this path.

Test Coverage

The new test suite in database.service.test.ts:697–768 is comprehensive and covers:

• ✅ Incomplete→complete transition fires notification
• ✅ Incomplete node (no NodeInfo yet) does NOT fire
• ✅ Dedup: second upsert of same node does not re-notify
• ✅ Repository persistence still occurs

Gap: no test for the cross-source dedup behavior. A test asserting that node !aabbccdd on src2 does NOT re-notify when already notified from src1 would document (and lock in) that intentional cross-source suppression.

Gap: no test for the wasComplete guard. A node that was already complete before the upsert (reconnect scenario) should not re-notify. The dedup set covers this in practice (since a complete node would have been notified once), but an explicit test for a pre-existing complete node would add regression value.

The 4 updated test mocks (ignoreReapply, node-identity-guards, traceroute-hops, virtualNode) correctly switch from nodes.upsertNode to upsertNodeAsync — these are necessary and correct.

Performance

The needsCheck optimization (database.ts:2423–2424) is good: a Set.has() is O(1) and avoids a potentially expensive getNode() call for every subsequent packet from already-seen nodes. For a system with hundreds of known nodes, this is meaningful.

Security

No security concerns. This is internal notification plumbing with no new API surface, no user-controlled inputs flowing into the notification path (node data comes from trusted Meshtastic hardware), and no new permissions or auth changes.

Minor Nits

• (nodeData as any).sourceId cast appears in 3 places in maybeNotifyNewNode() and upsertNodeAsync(). A typed helper or narrowed interface would eliminate these casts, but it's not worth a separate refactor.
• The settle() helper in tests (two awaits to drain microtasks) is a bit fragile — vi.waitFor() is used elsewhere in the same test suite and would be more robust for the "does NOT fire" assertions too.

Overall Assessment

Approve with optional follow-ups. The bug fix is correct, the dedup/gate logic is sound, the test coverage is good, and the PR description accurately describes the change. The ghost suppression bypass is a pre-existing gap that this PR didn't introduce but slightly expands; it can be addressed in a separate issue if needed.

Claude finished @Yeraze's task —— View job

PR Review

Tasks

• Gather context and understand the PR
• Review database.ts changes
• Review meshtasticManager.ts changes
• Review test files
• Post final review

Summary

This PR correctly diagnoses and fixes a real bug: notifyNewNode was only reachable through the sync upsertNode() wrapper, which the MQTT path uses, while meshtasticManager bypassed it by calling databaseService.nodes.upsertNode() directly. The solution — extracting maybeNotifyNewNode() and adding upsertNodeAsync() — is clean and the implementation is sound.

Code Quality & Design

maybeNotifyNewNode() extraction (database.ts:2354)
The shared helper is well-designed. Using a structural type for nodeData rather than requiring a full DbNode is the right call given the two different DbNode shapes in scope. The dedup-before-persist ordering (line 2387, newNodeNotifiedSet.add(nodeNum) before the async import resolves) correctly prevents a race where a second caller fires before the first notification completes.

upsertNodeAsync() short-circuit (database.ts:2423–2424)
The needsCheck optimization is solid — a Set.has() O(1) check eliminates the getNode() pre-read for all already-notified nodes. Good for the hot path in high-traffic deployments.

Dynamic import() for notificationService (database.ts:2389)
Avoids a circular import, and the pattern is already established in the MQTT path. The .catch() at line 2404 ensures notification failures never propagate to callers.

Potential Issues

Ghost suppression bypass (database.ts:2204–2215 vs 2416)
The sync upsertNode() path checks isNodeSuppressed() before writing. upsertNodeAsync() delegates directly to this.nodes.upsertNode() (the repository), which has no ghost suppression guard. The 31 rerouted meshtasticManager call sites now bypass ghost suppression for new node creation. This was a pre-existing gap in the repository layer, but this PR widens its exposure. Worth a follow-up issue — it won't cause data corruption but ghost nodes could slip through.

newNodeNotifiedSet keyed by nodeNum only, not (nodeNum, sourceId) (database.ts:2372, 2387)
Cross-source dedup is intentional (and now documented by the test at line 791), but it means: if node !aabbccdd is discovered on source A, then the device moves to source B, no notification fires for source B. For most deployments this is correct ("notify once per node per process run"), but if someone has intentionally different source-scoped notification configs (e.g., one source notifies to Discord, another to Slack) they'd miss the second. Since the PR description acknowledges this behavior and a test now locks it in, this is acceptable as-is.

this.sources.getSource() inside the fire-and-forget (database.ts:2392)
this.sources throws 'Database not initialized' if sourcesRepo is null. This would be swallowed by the .catch() at 2404 and fall back silently, but could produce confusing Failed to send new node notification log entries during early startup. Low risk, but a this.sources != null guard before the try block would be cleaner.

Test Coverage

The new suite at database.service.test.ts:697–804 is comprehensive:

• ✅ Incomplete→complete fires notification
• ✅ Incomplete node (no NodeInfo) does NOT fire
• ✅ Dedup: second upsert does not re-notify
• ✅ Repository persistence still occurs
• ✅ wasComplete guard: pre-existing complete node (reconnect scenario) does NOT re-notify
• ✅ Cross-source dedup documented and tested

The cross-source dedup test (line 791) and the wasComplete reconnect test (line 769) directly address the two gaps called out in the prior review. Good coverage.

The 4 updated mock files (ignoreReapply, node-identity-guards, traceroute-hops, virtualNode) correctly switch from nodes.upsertNode to upsertNodeAsync — necessary and correct given the rerouting.

Minor nit: The settle() helper (two awaits to drain microtasks) is slightly fragile. The "does NOT fire" assertions use it, and vi.waitFor() is used elsewhere in the suite for positive assertions. For the negative assertions, settle() is fine as long as the dynamic import mock resolves synchronously, which it does in the test setup.

Performance

No regressions. The needsCheck short-circuit ensures that for any node seen more than once (the overwhelming majority of upserts in a running system), upsertNodeAsync() adds exactly one Set.has() call over the raw repository path — negligible overhead.

Security

No concerns. Node data originates from trusted Meshtastic hardware over the existing transport layer. No new API surface, no user-controlled inputs, no new auth changes.

Overall Assessment

Approve. The bug fix is correct, the abstraction is clean, test coverage is thorough (including the reconnect guard and cross-source dedup cases that were flagged in the previous review), and the PR description accurately reflects the implementation. The ghost suppression gap is pre-existing and worth a follow-up issue but is not a blocker.