Claude finished @Yeraze's task —— View job

PR Review: refactor(server): extract backup + notification route groups

Tasks:

• Gather context and understand the request
• Review backupRoutes.ts
• Review notificationRoutes.ts
• Review backupRoutes.test.ts
• Review notificationRoutes.test.ts
• Review server.ts changes
• Compile and post final review

Summary

This is a clean, well-structured refactor that correctly extracts 24 route handlers out of server.ts into two dedicated modules. Behavior is preserved, the module boundaries are sensible, and the new test files are thorough. A few issues are worth raising below.

Findings

notificationRoutes.ts — Deprecated singleton import

Lines 7, 121, 420 — Both the push /test and apprise /test handlers use the deprecated meshtasticManager default-export singleton to get localNodeInfo:

import meshtasticManager from '../meshtasticManager.js';
// ...
const localNodeInfo = meshtasticManager.getLocalNodeInfo();

The PR notes this is "exactly as before" (carried over verbatim), but since these handlers are already being source-scoped for Apprise (they accept a sourceId), this is a good opportunity to look up the local node info via sourceManagerRegistry.getManager(sourceId) instead of the deprecated singleton — especially for the Apprise test handler at line 420, where sourceId is always present (it's required). The push test handler at line 121 doesn't have a scoped sourceId, so it's harder to fix there, but the Apprise one could be straightforward.

This is a low-urgency finding since it's preexisting behavior, but it's worth flagging given CLAUDE.md's explicit rule: "No global meshtasticManager singleton. Look up per-source instances via sourceManagerRegistry.getManager(sourceId)".

notificationRoutes.ts — VAPID subject format not validated

Lines 38–52 — The /push/vapid-subject PUT handler validates that subject is a non-empty string, but the Web Push spec requires the VAPID subject to be either a mailto: URI or an https: URL:

if (!subject || typeof subject !== 'string') {
  return res.status(400).json({ error: 'Subject is required and must be a string' });
}

An arbitrary string like "hello" will be accepted and passed to the notification service. This matches the pre-refactor behavior but is worth noting as a latent bug / missing validation.

notificationRoutes.ts — Inconsistent error response shape in /apprise/test

Lines 393–408 — The apprise /test handler returns { success: false, message: ... } for 401/400 errors, while all other error handlers in this file return { error: ... }. This inconsistency exists in the original code (carried over verbatim), but it's now more visible:

// apprise/test uses:
return res.status(401).json({ success: false, message: 'Not authenticated' });

// All other handlers use:
return res.status(400).json({ error: 'Some error' });

The test at line 233 (notificationRoutes.test.ts) validates against res.body.message, which will continue to work — but callers consuming these endpoints need to be aware of the inconsistency.

backupRoutes.ts — fs.existsSync inside async handler

Line 204 — The system backup download handler uses fs.existsSync (synchronous) inside an async route handler to check whether a backup directory exists:

const fs = await import('fs');
if (!fs.existsSync(backupPath)) {

This blocks the event loop. The fs/promises module (also dynamically imported on line 459 in notificationRoutes.ts) is already available. Using fs.promises.access or fs.promises.stat in an await call would be preferable. This is preexisting behavior but straightforward to fix in the route module.

backupRoutes.ts — Response after archive error may fail

Lines 217–220 — If the archiver fires an error event after headers have already been sent (e.g., after archive.pipe(res) begins streaming), calling res.status(500).json(...) will throw "Cannot set headers after they are sent":

archive.on('error', err => {
  logger.error('❌ Error creating archive:', err);
  res.status(500).json({ error: 'Failed to create archive' });
});

A guard like if (!res.headersSent) would prevent the crash. Again, this is carried over verbatim from the original code, but it's now isolated to a module where it's easy to fix.

Test coverage — Missing GET /system/backup/download happy path

backupRoutes.test.ts covers the invalid-dirname 400 case (line 169–172) and both delete happy/sad paths, but there is no test for the GET /system/backup/download/:dirname success path (the tar.gz streaming path). The archiver interaction is complex (mocking archiver's TarArchive export is nontrivial), so it's understandable that this was omitted — but it's a gap worth noting for completeness.

notificationRoutes.test.ts — Apprise URLs endpoint not tested

GET /apprise/urls and the ENOENT fallback path for that handler are not covered by the test file. These would be straightforward to add.

Minor / Non-blocking

• notificationRoutes.ts lines 148–197 and 200–361: the handler body indentation is one level deeper than the pushRouter.get/post(...) call — this is a pre-existing style inherited from the original code and doesn't affect behavior.
• The dynamic await import('archiver') and await import('fs') on every download request (lines 198–201) creates new module resolution overhead per request. Since both are unconditional for the download path, top-level static imports would be more idiomatic; this is a micro-optimization.

Verdict

The refactor is solid and safe to merge. The issues above are all inherited from the original server.ts code — nothing was introduced by this PR — and the new test files catch the most important validation paths. The deprecated singleton usage in the notification test handlers and the sync existsSync are the items most worth addressing, either in a follow-up or as part of the source-scoping work still to come.