Claude finished @Yeraze's task —— View job

PR Review: refactor(server): extract device & system route groups

Overall this is a clean, well-structured refactor that continues the decomposition of server.ts in a principled way. The callback-injection pattern for gracefulShutdown is a solid design choice. A few issues worth noting:

Bugs / Correctness

1. meshtasticManager singleton usage in systemRoutes.ts (line 93–94)

// systemRoutes.ts:23
import meshtasticManager from '../meshtasticManager.js';
// ...
const connectionStatus = await meshtasticManager.getConnectionStatus();
const localNode = meshtasticManager.getLocalNodeInfo();

The GET /status handler uses the deprecated singleton meshtasticManager instead of resolving from the source registry. The CLAUDE.md and the PR description both highlight that there is no global singleton — per-source scoping is mandatory. This handler silently leaks data (returns only the default source's connection/node info). Given that the PR description lists this route as moved "verbatim," this appears to be a pre-existing issue carried over, but it's worth flagging.

2. versionCheckCache is module-scoped mutable state (systemRoutes.ts:122)

let versionCheckCache: { data: any; timestamp: number } | null = null;

This is fine for production but the test comment in systemRoutes.test.ts:115–117 reveals a non-obvious ordering dependency: the failure test must run before the success test because the success test populates the module-level cache. Vitest randomizes test order when sequence.shuffle is on. A beforeEach(() => { versionCheckCache = null; }) reset (or exposing a resetVersionCheckCache() for tests) would make the test suite order-independent.

3. compareVersions treats pre-release segments as numeric (systemInfo.ts:30)

const aParts = a.split(/[-.]/).map(p => parseInt(p) || 0);

Splitting on both - and . means 4.11.0-beta.1 becomes [4, 11, 0, 0, 1], and parseInt('beta') returns NaN which is coerced to 0. Practically harmless for GitHub release tags (which are pure semver), but silently drops pre-release identifiers. Not a regression — this is existing logic moved verbatim.

Design / Best Practices

4. checkDockerImageExists fetches a token on every call with no caching (systemInfo.ts:57–80)

Each call to checkDockerImageExists fetches an anonymous GHCR token for every tag variant, even when the upstream is called from the 5-minute cached version-check handler. This is benign today but could be tightened with a short-lived token cache if rate-limit pressure arises. Low priority.

5. systemRoutes.ts uses any in the version-check result type (line 122)

let versionCheckCache: { data: any; timestamp: number } | null = null;

A named interface (e.g. VersionCheckResult) would make this self-documenting. Minor.

6. Default callbacks object is mutated at module load (systemRoutes.ts:40–44)

let callbacks: SystemCallbacks = {
  gracefulShutdown: () => {
    logger.warn('gracefulShutdown called before system callbacks were registered');
  },
};

The pattern is consistent with settingsRoutes.ts and prevents a hard crash if setSystemCallbacks is not called, which is good defensive programming. No issue here.

Security

No new attack surface introduced. Auth middleware (requirePermission, optionalAuth) is preserved identically on all moved handlers. The gracefulShutdown route is gated behind requirePermission('settings', 'write'), which is appropriate.

The GET /status endpoint uses optionalAuth() and intentionally exposes version + connection info — this is unchanged behavior.

Test Coverage

Tests are solid and follow the existing supertest pattern. Two specific points:

7. deviceRoutes.test.ts — missing test for null localNodeInfo on backup (line 77)

The backup handler falls back to '!unknown' when getLocalNodeInfo() returns null. There's no test for the null branch, leaving the fallback path untested.

8. systemRoutes.test.ts — version check cache ordering (already noted in bug #2 above)

The comment at line 115–117 acknowledges the ordering dependency. This is a correctness risk if tests are ever run with shuffle enabled.

Summary

The core refactor is clean and the setSystemCallbacks injection pattern is the right approach. The singleton meshtasticManager usage on the /status endpoint is the most notable issue — it predates this PR but now lives in an extracted module, making it more visible and worth addressing.