Claude finished @Yeraze's task —— View job

PR Review: Phase 1 — Extract Server Routes

Tasks:

• Gather context
• Read new route modules
• Read test files
• Read modified server.ts
• Analyze and provide review

Overall this is a clean, well-executed refactor. The extraction is behaviour-preserving, auth middleware is correctly applied, and the new test files follow the project's established patterns. A few issues worth addressing before merge:

Bug: Duplicate GET / in healthRoutes.ts

src/server/routes/healthRoutes.ts:16-34 — two handlers are registered for the same GET / path. In Express, the second handler will never be reached; the first one always responds. If these were two different handlers in the original server.ts, the original may have had distinct paths or used different middleware chains. Verify what the original two /health handlers looked like:

// Both registered to "GET /" — second is dead code
router.get('/', (_req, res) => { ... });          // line 16
router.get('/', optionalAuth(), (_req, res) => { ... }); // line 27 — unreachable

Action required: Either delete the second handler if it's truly redundant, or fix the path (e.g., /info or /env). This is a behaviour change introduced by this PR if the original server.ts mounted these on different sub-paths.

Bug: Fire-and-forget auditLogAsync calls in purgeRoutes.ts

src/server/routes/purgeRoutes.ts:19,44,69,89 — databaseService.auditLogAsync(...) is called without await. If the audit write fails, the error is silently swallowed. This pattern appears to be carried over from the original server.ts, but it's worth flagging. At minimum, errors should be caught:

// Current (unhandled rejection risk):
databaseService.auditLogAsync(req.user!.id, ...);

// Better:
databaseService.auditLogAsync(req.user!.id, ...).catch((e) =>
  logger.error('Audit log failed:', e)
);

If this was intentionally fire-and-forget in the original (audit failures shouldn't block the response), a comment explaining the intent would help.

Inconsistency: router.use(requireAdmin()) vs per-route middleware

cleanupRoutes.ts:8 and purgeRoutes.ts:9 use router.use(requireAdmin()) to guard all routes on the router, which is correct and consistent. maintenanceRoutes.ts applies requirePermission(...) per-handler instead. Both patterns work, but mixing them makes it easy to miss adding auth when a new route is added. Consider either:

• Using router.use(requirePermission('configuration', 'read')) before the read-only routes and a separate check for write, or
• Adding a comment explaining why per-handler middleware is used here (different read vs. write permissions).

Test coverage gaps

healthRoutes.test.ts — only tests the first GET / handler. The second handler (with optionalAuth() returning nodeEnv) has zero test coverage. Given the duplicate-route bug above, a test would also reveal the dead code.

cleanupRoutes.test.ts — missing 403 tests for /cleanup/nodes and /cleanup/channels (only /cleanup/messages has a 403 test). For consistency with the other test files, those two should be covered.

purgeRoutes.test.ts — no test for the sourceId-scoped paths (e.g., purging nodes for a specific source). The handler branches on purgeNodesSourceId to build the response message and passes it to resolveSourceManager, so a test with a sourceId body would improve confidence.

Minor: parseInt without radix on user input

cleanupRoutes.ts:12 and cleanupRoutes.ts:24:

const days = parseInt(req.body.days) || 30;

parseInt without a radix is technically safe for decimal strings, but ESLint's radix rule flags it. Prefer parseInt(req.body.days, 10). Additionally, parseInt on undefined returns NaN, and NaN || 30 correctly falls back — but the intent is clearer with an explicit guard or Number():

const days = Number(req.body.days) || 30;

Minor: Unused isAdmin parameter in createApp test helper

cleanupRoutes.test.ts:34 and themeRoutes.test.ts:46 — createApp accepts an isAdmin parameter but never uses it. The admin check is driven by the mocked getUserPermissionSetAsync return value, not this parameter. Remove the dead parameter to avoid confusion.

Positive highlights

• Auth guards are correctly placed and tested (401/403 coverage).
• themeRoutes.ts has the most thorough validation logic and the best test coverage of the five modules.
• The server.ts wiring is minimal and clean — imports + apiRouter.use(...) only.
• Test structure (mock patterns, createApp factory, beforeEach resets) is consistent with the rest of the codebase.