Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore): per-message scope/region override (#3701)

• Read changed files
• Review backend changes (meshcoreManager.ts, meshcoreRoutes.ts)
• Review frontend changes (useMeshCore.ts, MeshCoreChannelsView.tsx, MeshCorePage.css)
• Review test coverage (meshcoreManager.scope.test.ts)
• Post review feedback

Overall this is a clean, well-considered implementation. The design is sound and the key invariant (scope-assert → send serialization) is preserved correctly. A few things worth discussing:

Backend — meshcoreManager.ts

normalizeScopeOverride strips disallowed chars silently (src/server/meshcoreManager.ts:1995)

const normalized = scope.trim().replace(/^#/, '').replace(/[^0-9A-Za-z-]/g, '');

'Bad Scope!' → 'BadScope' rather than a validation error. This is intentional per the test at line 195-200, but it means an operator who typos a scope (e.g. a space between words) silently gets a different scope than intended. Consider at least a server-side warning log so ops can diagnose "why didn't my scope apply?". The route already enforces 63-char max but doesn't reject non-matching chars — the manager silently strips them. This diverges from the config/default-scope endpoint which returns a 400 for the same input. Inconsistency between the two validation paths could surprise callers.

Three-value return (undefined / null / string) is subtle but correct (src/server/meshcoreManager.ts:1992). The distinction between "no override" (undefined) and "explicit unscoped" (null) is load-bearing. The JSDoc is clear, but this is easy to misuse in future callers. Consider a discriminated union type alias to make it harder to pass the wrong value.

Cache invalidation after a scoped override (src/server/meshcoreManager.ts:2080-2084): After an override send with e.g. augsburg, activeFloodScope is now augsburg. The next normal send will resolve muenchen (channel scope) and re-assert because the cache differs — this is correct and verified by the does NOT persist test at line 214. No issue, but worth noting the cache stays valid across override uses.

Backend — meshcoreRoutes.ts

scope: null is silently treated as "no override" (src/server/routes/meshcoreRoutes.ts:1434)

if (scope !== undefined && scope !== null) {

This means a client sending { "scope": null } gets the same behavior as omitting the key entirely — normal channel/default resolution. The PR description says "explicit unscoped" requires passing scope: '' (empty string). This is a reasonable choice, but it differs from how sendMessage on the manager accepts null as explicit-unscoped. The asymmetry could trip up a future API client that passes null expecting unscoped behavior. A comment explaining the deliberate choice would help.

No route-level test for the new scope parameter in meshcoreRoutes.ts. The manager tests (meshcoreManager.scope.test.ts) cover the business logic well, but there's no integration test verifying the route correctly passes scopeOverride through to sendMessage, nor that it rejects scope: 123 (non-string) with a 400. Given the PR description calls out "full Vitest suite passed", this gap is acceptable, but worth a follow-up.

Frontend — MeshCoreChannelsView.tsx

discoverRegions is called on every connect (src/components/MeshCore/MeshCoreChannelsView.tsx:223), which fires the POST .../regions/discover endpoint. This triggers active radio traffic on the mesh. An operator who connects/reconnects frequently (e.g. after network hiccups) could be surprised that simply opening the channels view causes repeated region discovery floods. Consider only running this once at mount rather than on every status?.connected change, or gate it behind user intent.

overrideScope is reset to null on channel switch but the input control stays mounted — the useEffect at line 234-237 handles this correctly. No issue.

resolvedScope logic (src/components/MeshCore/MeshCoreChannelsView.tsx:208):

const resolvedScope = (active.scope && active.scope.trim()) || defaultScope.trim() || '';

defaultScope is fetched asynchronously, so on first render (before the effect fires) it's ''. The placeholder shows "unscoped" until the fetch completes, which could briefly mislead the operator. Minor UX issue only.

datalist id is not unique per channel — id="mc-scope-region-suggestions" is hardcoded. If multiple MeshCoreChannelsView instances were ever rendered on the same page (e.g. two sources in a split view), the <datalist> id would collide and the wrong suggestions could appear. Not a current issue given the single-source-per-page layout, but worth noting.

Frontend — useMeshCore.ts

scope spread into request body (src/components/MeshCore/hooks/useMeshCore.ts:1193):

...(scope !== undefined ? { scope } : {}),

Clean — avoids sending scope: undefined which JSON serializes to omitting the key anyway, but this explicit guard makes intent clear.

sendMessage hook is not useCallback-memoized on scope — the hook signature correctly has scope as a call-time argument (not a memoization dependency), so this is fine. No issue.

Tests — meshcoreManager.scope.test.ts

Test coverage for the override path is solid:

• Override beats channel scope
• Override beats source default scope
• Override normalization (strips #, removes disallowed chars)
• Empty/whitespace = explicit unscoped
• Omitting override = normal resolution
• Override not persisted (channel row untouched, next send re-asserts channel scope)
• set_flood_scope still issued before send_message

Missing test: concurrent override + non-override sends. E.g., one send with scopeOverride='augsburg' and one without, fired concurrently — verify the serializer still atomically pairs each scope with its send. The existing concurrency test (line 165-174) covers two different channel scopes but not override vs. normal.

Summary

The core implementation is correct and the serialization invariant is preserved. The main actionable items are:

1. Inconsistency: normalizeScopeOverride silently strips invalid chars while /config/default-scope returns 400 for them. Consider aligning the two paths or adding a warning log.
2. scope: null vs scope: '': Document (or align) the API semantics for null vs empty string.
3. discoverRegions on every reconnect: Reconsider whether to trigger active radio discovery automatically every time the connection status cycles.

Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore): per-message scope/region override (#3701)

• Read changed files
• Review backend changes (meshcoreManager.ts, meshcoreRoutes.ts)
• Review frontend changes (useMeshCore.ts, MeshCoreChannelsView.tsx, MeshCorePage.css)
• Review test coverage (meshcoreManager.scope.test.ts)
• Post review feedback

This is a well-implemented PR. The previous review feedback (from the initial CI run) has been addressed thoughtfully. Here's my assessment of the current state:

What was addressed from the first review

discoverRegions on every reconnect — Fixed. The regionsDiscoveredRef guard (MeshCoreChannelsView.tsx:116) ensures discovery emits radio traffic at most once per mount, gated on the operator explicitly opening the scope-override control (showScopeOverride). The comment at line 113-116 makes the rationale explicit.

normalizeScopeOverride silently stripping chars — Fixed. A logger.warn is now emitted when chars are stripped (meshcoreManager.ts:2006-2011), making mangling visible in logs without failing the send.

scope: null vs scope: '' ambiguity — Fixed. The route comment at lines 1427-1439 now clearly documents the three-case contract (absent/null → no override, empty → unscoped, string → override), matching the manager's normalizeScopeOverride JSDoc. Both collapse JSON-null to undefined ("no override") intentionally.

Concurrent override + non-override serialization test — Added. The new test at line 224-248 fires an override send and a normal send concurrently with a floodScopeDelayMs to force real interleaving, and asserts the correct scope:augsburg → send → scope:muenchen → send sequence. Solid.

Remaining observations (minor)

datalist id collision (MeshCoreChannelsView.tsx:384)

<datalist id="mc-scope-region-suggestions">

The id is not unique per-source. If two MeshCoreChannelsView instances ever render in the same document (e.g. a future multi-source side-by-side layout), the browser would serve the wrong suggestions to the second input. Low risk for the current single-source-per-page architecture, but worth a ${sourceId}-mc-scope-suggestions pattern proactively.

resolvedScope flash on first render (MeshCoreChannelsView.tsx:213)

const resolvedScope = (active.scope && active.scope.trim()) || defaultScope.trim() || '';

defaultScope is empty string until the async effect at line 218-230 resolves, so the placeholder briefly shows "unscoped" even for a source that has a default scope configured. Only visible for the fraction of a second between mount and the effect settling — low impact, acceptable as-is.

sendMessage route — no test for the scope wire path (meshcoreRoutes.ts:1427-1451)
The manager tests cover scope logic exhaustively, but there's no integration test verifying that POST .../messages/send with { scope: "foo" } actually passes scopeOverride: "foo" through to sendMessage, or that { scope: 123 } is rejected with a 400. The route logic is simple enough that bugs here would be caught quickly, but a targeted route test would provide certainty.

sendMessage passes scope: null to the API (useMeshCore.ts:1193)

...(scope !== undefined ? { scope } : {}),

When the caller passes scope: null (which the hook signature allows), this serializes to { "scope": null } in the request body. The route handler collapses that to undefined (no override), so the behavior is correct, but a caller who passes null hoping for explicit-unscoped behavior would be surprised. The route comment documents this; the hook could add a short comment noting that null here means "omit" not "unscoped" (passing '' achieves unscoped).

No blockers

The core invariant (scope-assert → send serialized per source) is preserved, the override is correctly one-off, the concurrency test is thorough, and the radio-traffic concern from the first review has been cleanly resolved. The items above are all minor polish.

The system test failure (Reverse Proxy Test) appears to be pre-existing infrastructure flakiness unrelated to this PR's changes.

View job