Claude finished @Yeraze's task —— View job

PR Review: fix(map): apply per-node position override on multi-source nodes endpoint

Tasks:

• Gather context and understand the PR
• Review sourceRoutes.ts changes
• Review test file
• Review related files for comparison
• Post final review feedback

Review Summary

This is a clean, well-scoped bug fix. The root cause analysis is accurate and the implementation is sound. Here are my observations:

Correctness ✅

The fix correctly identifies the gap: GET /api/sources/:id/nodes returns raw DB rows that never went through enhanceNodeForClient, so the position override was invisible to the dashboard map. The patch applies getEffectiveDbNodePosition at the right point in the pipeline — after channel filtering/masking (line 1021 in sourceRoutes.ts), so it operates on already-filtered rows.

Privacy Semantics ✅

The private-override handling mirrors enhanceNodeForClient faithfully:

• Private override coordinates are stripped for users without nodes_private:read
• The override is not applied for those users (they continue to see raw GPS)
• positionIsOverride: true is only set when the override is actually applied

One subtle behavioral difference vs. enhanceNodeForClient to note (not necessarily wrong, but worth flagging): in enhanceNodeForClient (nodeEnhancer.ts:93-96), canViewPrivate is checked against nodes_private:read without a sourceId parameter, which means it's a global permission check. The new code in sourceRoutes.ts (line 1018-1020) does the same — no sourceId is passed to hasPermission. This is consistent, so ✅, but it means nodes_private is not source-scoped (which appears to be intentional design).

Minor Observations

Wrapper function getEffectivePosition (line 59):

const getEffectivePosition = (node: any) => getEffectiveDbNodePosition(node);

This one-liner module-level wrapper is a thin indirection with no added value. It's likely a leftover artifact. Consider removing it and calling getEffectiveDbNodePosition(n) directly at the two call sites (lines 1034 and 1203). Not a blocking issue, just clutter.

nodes_private check inside the map loop (lines 1018-1020):

const canViewPrivate = user?.isAdmin
  ? true
  : (user ? await hasPermission(user, 'nodes_private', 'read') : false);

The canViewPrivate check is correctly hoisted outside the .map(), so the hasPermission DB call runs once per request, not once per node. ✅ (The code uses await with .map() via masked.map(node => {...}) on line 1021, but since canViewPrivate is pre-computed above, the closure is synchronous. No issue.)

neighbor-info endpoint already uses getEffectivePosition (lines 1203-1204):
The neighbor-info endpoint already calls getEffectivePosition for its enriched response. This PR doesn't affect that endpoint, but it confirms the pattern is established.

Test Coverage ✅

Five tests covering:

1. Override applied to flat coords ✅
2. No-override left untouched ✅
3. Incomplete override (null lat/lng) ignored ✅
4. Admin sees private override ✅
5. Non-admin gets private override hidden + coords stripped ✅

One gap: There's no test for the MeshCore branch (lines 941-974). MeshCore nodes are returned early before the override logic, so the override is not applied to them — this is probably intentional since MeshCore contacts don't have the same DB schema. A test or comment documenting this intent would be a nice addition, but it's not a regression risk for this PR.

Performance ✅

The canViewPrivate permission check is made once per request (not in the loop). The .map() is synchronous (closure captures the pre-computed boolean). No N+1 issues introduced.

Verdict

Approved with minor suggestions. The fix is correct and well-tested. The only actionable suggestion is removing the redundant getEffectivePosition wrapper. The rest is informational.

Claude finished @Yeraze's task —— View job

PR Review: fix(map): apply per-node position override on multi-source nodes endpoint

Tasks:

• Gather context and understand the PR
• Review sourceRoutes.ts changes
• Review test file
• Review related files for comparison
• Post final review feedback

Review Summary

This is a clean, correct, and well-tested fix. The root cause is accurately identified and the patch is minimal and focused.

Correctness ✅

The fix correctly closes the gap: GET /api/sources/:id/nodes returns raw DB rows that never passed through enhanceNodeForClient, so position overrides were invisible to the dashboard map. The patch applies getEffectiveDbNodePosition after channel filtering/masking (lines 1017–1044 in sourceRoutes.ts), operating on correctly filtered rows.

The previous review flagged a redundant getEffectivePosition wrapper at line 59 — that has been removed in this update. getEffectiveDbNodePosition is now called directly at the two call sites (lines 1033, 1202-1203). ✅

Privacy Semantics ✅

The private-override handling faithfully mirrors enhanceNodeForClient (nodeEnhancer.ts:92-104):

• canViewPrivate is computed once outside the .map() loop — no N+1 DB calls. ✅
• Private override coordinates are stripped (latitudeOverride, longitudeOverride, altitudeOverride) from the response for users without nodes_private:read. ✅
• Override is not applied for those users — raw GPS is preserved. ✅

One small behavioral difference to note (not a bug, but worth being aware of): enhanceNodeForClient (nodeEnhancer.ts:89) checks positionOverrideEnabled === true (strict equality), while the new code delegates to getEffectiveDbNodePosition which uses a truthy check (node.positionOverrideEnabled && ...). This is actually the correct approach for a raw-DB-row endpoint since SQLite stores booleans as 0/1, not true/false. The existing getEffectiveDbNodePosition function was already written with this in mind (its comment at nodeEnhancer.ts:44-46 explicitly notes this). ✅

MeshCore Early Return (Documented) ✅

The MeshCore branch (lines 934-973) returns early before the override logic. This is intentional and is now documented with a comment explaining that MeshCore contacts don't have override columns. The previous review noted this as a gap — it's been addressed. ✅

Test Coverage ✅

Five targeted tests covering all key scenarios:

1. Override replaces flat lat/lng ✅
2. No-override leaves raw GPS untouched ✅
3. Incomplete override (null lat/lng) is ignored ✅
4. Admin sees private override ✅
5. Non-admin gets private override hidden + coords stripped ✅

The test for the MeshCore early-return path was noted as a gap in the prior review — it's still absent, but this is low risk since MeshCore nodes have a fundamentally different data shape (no override columns at all).

Minor Observations

positionOverrideIsPrivate stripping only — no coordinate nulling: When a private override is hidden (lines 1025-1031), only the *Override coordinate columns are stripped. The raw GPS (latitude/longitude) is left as-is, which is correct — that's what a non-privileged user should see. ✅

positionIsOverride is never set to false explicitly: For nodes where the override doesn't apply, positionIsOverride is not added to the response at all (it's undefined, not false). enhanceNodeForClient initializes it to false explicitly. This is a minor inconsistency in the API shape but unlikely to matter since clients will treat undefined the same as false. Low risk.

Verdict

Approved. The fix is correct, well-scoped, and the earlier feedback (redundant wrapper, MeshCore comment) has been addressed. Ready to merge.