Claude finished @Yeraze's task —— View job

PR Review: refactor(server): extract /channels route group

Tasks:

• Gather context and understand PR scope
• Review channelRoutes.ts
• Review channelRoutes.test.ts
• Review server.ts changes
• Compile and post final review

Summary

This is a well-executed verbatim extraction. Handler bodies are unchanged, paths are correctly made relative to the /channels prefix, the mount point in server.ts is clean (apiRouter.use('/channels', channelRoutes)), and the orphaned imports (migrateAutomationChannels, modemPresetChannelName) were correctly removed from server.ts. The three channel-move helpers moved cleanly with their consumers. Overall this is a solid, low-risk refactor.

A few items worth noting:

Issues Found

1. Stale import in channelRoutes.ts (minor bug / dead import)

src/server/routes/channelRoutes.ts:31-32 still imports migrateAutomationChannels and modemPresetChannelName, both of which ARE used — so this is not a bug. However, worth double-checking since the PR description says these imports were "removed" from server.ts as orphans. They are correctly re-imported in channelRoutes.ts and used at lines 276 and 94, so this is fine.

2. transformChannel still imported but unused in server.ts (potential dead import)

src/server/server.ts:25 still imports transformChannel from ./utils/channelView.js. Based on the search, it is used in the file at lines 3026/3034 (in the remaining inline /config or /nodes handlers), so this is not a dead import — but worth confirming if the remaining usages are still correct after the extraction.

3. Content-Disposition header with unsanitized channel name (security concern)

src/server/routes/channelRoutes.ts:225-226:

const filename = `meshmonitor-channel-${channel.name.replace(/[^a-z0-9]/gi, '_').toLowerCase()}-${Date.now()}.json`;
res.setHeader('Content-Disposition', `attachment; filename="${filename}"`);

The regex [^a-z0-9] is applied and special chars replaced with _, so the value going into the header is safe. However, the filename parameter uses the legacy unquoted/quoted form — per RFC 6266, the safer modern form is filename*=UTF-8''<encoded>. This is a pre-existing pattern across the codebase (see deviceRoutes.ts, backupRoutes.ts, securityRoutes.ts) and not introduced here, so not a blocker, but a note for future hardening.

4. normalizeBoolean is duplicated within the same file (code quality)

src/server/routes/channelRoutes.ts:203-208 and src/server/routes/channelRoutes.ts:595-613 define two separate normalizeBoolean functions — one inside the /export handler closure and one inside the /:slotId/import handler closure. They have slightly different signatures (the import version has a defaultValue parameter). These could be module-level helpers to avoid duplication. This is pre-existing (carried verbatim from server.ts), but now that they live in the same module it's more visible.

5. Long setTimeout delays in route handlers make tests slow (test quality)

src/server/routes/channelRoutes.ts:727, 773, 777, 945, 981, 1010 — there are multiple setTimeout(resolve, 2000), setTimeout(resolve, 1000), and setTimeout(resolve, 1500) calls inline in the /reorder and /import-config route handlers. These are carried verbatim from server.ts and are empirically necessary for device pacing.

However, the tests in channelRoutes.test.ts do NOT use vi.useFakeTimers(). The test for /import-config at line 341 exercises the full path including beginEditSettings, channel import with a 1000ms sleep, LoRa config with a 1500ms sleep, and commitEditSettings — this means that particular test runs for ~6.5 seconds of real wall-clock time. The three /reorder tests avoid the actual reorder path (they test validation + identity), so they don't hit the sleep, but any integration test that exercises a real reorder would accumulate ~8.5 seconds per call.

Recommendation: Add vi.useFakeTimers() / vi.useRealTimers() around the happy-path import-config and reorder tests, or extract the sleep delays into a configurable injectable so tests can pass 0 delays. This is a pre-existing issue but it's now testable in isolation here.

6. /reorder happy-path not tested (test coverage gap)

The /reorder describe block (lines 262-280) only tests:

• rejection of wrong-length newOrder
• rejection of non-permutation newOrder
• identity order short-circuit (no device call)

There is no test for an actual reorder that changes slots (e.g. [1, 0, 2, 3, 4, 5, 6, 7]), meaning beginEditSettings, setChannelConfig, commitEditSettings, and the message migration path are untested. This is the most complex handler in the file. Related to point 5 (timers make it hard to test), but worth calling out explicitly.

7. MeshCore deleteChannel happy-path not tested

The DELETE describe block verifies Meshtastic paths (sourceId required, primary channel blocked, messages purged). The MeshCore branch (sourceType === 'meshcore', lines 505-526) is never exercised in the test suite, in contrast to PUT which has MeshCore-specific tests (16-byte secret validation, unregistered manager 503). Adding a MeshCore DELETE happy-path test would bring it to parity.

Minor / Nits

• Line 461 (PUT /:id): After the Meshtastic write path, getChannelById(channelId) is called without sourceId, while the MeshCore path at line 426 correctly passes chanSourceId. This looks like a pre-existing inconsistency carried verbatim.

• Line 631 (/:slotId/import): resolveSourceManager(importSourceId) is called without null-checking importSourceId — if the body omits sourceId, importSourceId is undefined and resolveSourceManager falls back to the singleton. This is intentional (same behavior as server.ts), but a comment clarifying the fallback intent would help future readers.

• Dynamic imports for channelUrlService (lines 822, 846, 926): These are await import(...) inside request handlers. This is fine for lazy-loading, but since the module is imported three times, the first call will incur the module-load cost. Node.js caches the module after first import, so subsequent calls are O(1). No action needed, just noting the pattern.

Verdict

Approve with minor comments. The extraction is clean and behavior-preserving. The main actionable items are the missing happy-path test for /reorder and vi.useFakeTimers() to prevent the import-config test from being a wall-clock slow test. The rest are pre-existing patterns carried verbatim from server.ts or cosmetic.