Claude finished @Yeraze's task —— View job

PR Review

• Gather context
• Read changed files
• Analyze fix logic
• Provide review feedback

Summary

This is a clean, minimal, and well-targeted fix. The root cause is clear: teardownTransportOnly() mirrored the native backend teardown from disconnect() but omitted the stopVirtualNodeServer() call, leaving the virtual node server accepting mobile app connections while isConnected() was false. The fix correctly mirrors the disconnect() pattern.

Code Quality

src/server/meshcoreManager.ts:4064-4087 — The fix is correct and idiomatic:

• Placement at the top of teardownTransportOnly() mirrors disconnect() (line 841) exactly.
• The inline comment (lines 4065–4068) clearly explains the "why" — the BadState loop — which will help future readers.
• stopVirtualNodeServer() is already null-guarded (if (!this.virtualNodeServer) return; at line 796), so calling it during a reconnect where no virtual node is configured is harmless.

Potential Issues / Edge Cases

1. Reconnect gap vs. virtual node restart

When beginReconnect() fires (line 4057), the sequence is:

1. teardownTransportOnly() → stops virtual node server
2. scheduleNextReconnect() → waits delay ms (exponential backoff, up to 60 s by default)
3. attemptReconnect() → calls connect() → calls startVirtualNodeServer()

During the backoff window the virtual node server is down, so any mobile app that tries to connect will get a TCP connection refused rather than a BadState loop. This is the correct UX — it's actually an improvement over the pre-fix behavior (infinite BadState loop). Worth noting in release notes, but not a blocker.

2. void this.teardownTransportOnly().then(...) (line 4057)

The void here suppresses the promise. If stopVirtualNodeServer() throws an unhandled error, it will be a silent failure that won't prevent scheduleNextReconnect() from being called (since the .then() is attached). The stopVirtualNodeServer() implementation (lines 795–802) has a try/catch that swallows errors, so this is already handled — no issue.

3. disconnect() redundancy

disconnect() (line 841) also calls stopVirtualNodeServer(). Since teardownTransportOnly() now stops it first, calling disconnect() on a node already in the reconnecting state would call stopVirtualNodeServer() twice. The null-guard at line 796 (if (!this.virtualNodeServer) return;) makes this idempotent — no issue.

Performance / Security

No performance or security concerns. This is a state-machine correctness fix with no new I/O paths or privilege changes.

Test Coverage

The PR description lists manual test steps but no automated test changes. The existing meshcoreVirtualNodeServer.test.ts suite validates the server's own start/stop behavior. A unit test for teardownTransportOnly() verifying that stopVirtualNodeServer() is called (e.g. via a spy) would strengthen the regression coverage, but given the simplicity and risk profile of this change, absence of a new test is not a blocker.

Verdict

✅ Looks good to merge. The fix is correct, minimal, well-commented, and consistent with the existing disconnect() pattern. No blocking issues found.

Claude finished @Yeraze's task —— View job

PR Review

• Gather context
• Read changed files
• Analyze fix logic and tests
• Provide review feedback

Summary

This PR adds 6 lines to fix the root cause (stopping the VN server in teardownTransportOnly()) and then backs it with a dedicated test file covering all meaningful paths. Both changes are clean and correct.

Fix (src/server/meshcoreManager.ts)

The one-liner fix at meshcoreManager.ts:4069 is exactly right:

await this.stopVirtualNodeServer();

• Mirrors the disconnect() code path identically.
• stopVirtualNodeServer() (lines 795–803) is null-guarded and swallows stop errors, so calling it when no VN server is configured is a no-op.
• The inline comment (lines 4065–4068) clearly explains the BadState loop — good for future readers.
• No performance or security concerns.

Tests (src/server/meshcoreManager.virtualNode.test.ts)

The new test file adds three focused regression tests. All three are well-structured:

Test 1 — "stops the Virtual Node server when tearing down transport" ✅
Verifies the primary regression: stopVirtualNodeServer() is called exactly once. Clean and minimal.

Test 2 — "tears down native backend after stopping VN server" ✅
This is the strongest test — it verifies ordering using invocationCallOrder, ensuring stopVirtualNodeServer runs before nativeBackend.disconnect. It also validates post-teardown state (connected === false, nativeBackend === null). Good depth.

Test 3 — "delegates to VN server stop() through stopVirtualNodeServer()" ✅
Reaches through to the actual stop() call on the underlying server object and verifies virtualNodeServer is nulled out afterward. This tests the concrete teardown contract, not just the spy boundary.

One minor observation on test 3: The test sets isRunning: () => true on the mock but stopVirtualNodeServer() doesn't actually check isRunning() before calling stop() (it only checks if (!this.virtualNodeServer)). This is harmless — the property is ignored — but it could slightly mislead future readers. Not a blocker.

Verdict

✅ Looks good to merge. The fix is correct, minimal, and well-documented. The new test file addresses the gap noted in the prior review and validates both the call order and the post-teardown state. No blocking issues.