Claude finished @Yeraze's task —— View job

PR Review: feat(nodes): per-node "Hide from Map" toggle (#3549)

Status: Review complete ✅

• Read and analyze changed files
• Review migration and database layer
• Review backend routes and security
• Review frontend components
• Review test coverage
• Post final review

Overall this is a well-structured, carefully implemented feature. The implementation closely mirrors the existing ignore-flag and position-override patterns, which keeps it consistent and reviewable. A few items worth noting:

Migration & Schema

✅ Solid across all three backends. The migration is idempotent (duplicate-column catch for SQLite, IF NOT EXISTS for PG, information_schema check for MySQL). The new column is correctly defined in all three Drizzle schema variants (nodes.ts:75, nodes.ts:155, nodes.ts:234).

Minor: MySQL column sizing — hideFromMap BOOLEAN in MySQL maps to TINYINT(1), which is correct. One cosmetic inconsistency: the Drizzle MySQL schema uses myBoolean while the raw DDL in nodes.test.ts:149 uses bare BOOLEAN. Both work, but they'd ideally be consistent. Not a functional issue.

Backend Route (server.ts:2051–2108)

✅ Input validation is complete — boolean type check, sourceId presence, and nodeId hex format are all validated before the DB call.

Minor: sourceIdFrom: 'body' vs 'query' discrepancy — The requirePermission('nodes', 'write', { sourceIdFrom: 'body' }) middleware reads the sourceId from the request body, which is correct since the route sends it in the JSON body. However, the pattern in server.ts for the analogous /position-override route also sends sourceId in the body but uses { sourceIdFrom: 'query' } instead (line ~1872). One of these is inconsistent with the middleware's expectation. Worth auditing which is authoritative — if the middleware is scoping permissions by req.query.sourceId but the hide-from-map route correctly uses req.body.sourceId, there may be a permission scope bypass risk on the older route rather than this one.

Minor: No WebSocket broadcast after toggle — When toggleIgnored fires, does the server push a node-updated event to connected clients? Looking at the pattern, the setNodeHideFromMap method only updates the DB and cache — it doesn't emit to dataEventEmitter. That means other browser tabs/sessions won't see the change reflected until the next poll cycle. The same is true of the ignore flag, so this is consistent behavior, but worth noting as a known limitation.

database.ts — setNodeHideFromMap (database.ts:7472)

⚠️ PG/MySQL: fire-and-forget upsert — On the PG/MySQL path, the repo upsert is kicked off with .catch(err => ...) (line 7488) and the function returns synchronously after mutating the in-memory cache. This matches the pattern used by setNodePositionOverride and setFavorite, so it's consistent. The risk: if the DB write fails, the cache shows the new value but the DB retains the old one — a restart would revert the toggle silently. This is an existing design debt, not new to this PR.

Minor: Raw SQL in SQLite path — Lines 7498–7505 use this.db.prepare(...) with an eslint-disable comment. This is correctly annotated and consistent with the existing pattern, but it's worth noting for the eventual Drizzle migration batch.

Frontend Optimistic Update (App.tsx:4374–4433)

✅ Pattern is correct. The flushSync optimistic update + per-source pending key + rollback-on-error mirrors toggleIgnored exactly. The favoritePendingKey(sourceId, nodeNum) key correctly scopes to (sourceId, nodeNum) pairs to prevent cross-source bleed.

⚠️ Module-level mutable maps — pendingHideFromMapRequests (line 104), pendingFavoriteRequests, and pendingIgnoredRequests are all module-level Map singletons. They survive React HMR and aren't reset on unmount. A stale pending entry (e.g., from a failed request where pendingHideFromMapRequests.delete is skipped) would permanently block future toggles for that node until page refresh. The success path relies on the poll loop to clear the entry, but if the server crashes mid-request or the response is lost after flushSync, delete in the catch block at line 4428 should handle it. Worth a closer look to confirm all code paths call delete — on 403 it's deleted (line 4416), on other errors it's deleted in catch (line 4428), on success the poll removes it. Looks covered.

Minor: No await in setNodeHideFromMapAsync — database.ts:9177 declares the wrapper as async but just calls the sync method without await. This is consistent with other Async wrappers in this file (clearNodePositionOverrideAsync, etc.) and the sync method is intentionally synchronous for the SQLite path. Not a bug, but the naming implies an actually async implementation.

Frontend Filtering

✅ All four map surfaces correctly filter. NodesTab.tsx:1344, DashboardMap.tsx:239, NodeMarkersLayer.tsx:100, and embedPublicRoutes.ts:88 all gate on hideFromMap.

Minor: Embed route filters at the data-source level (embedPublicRoutes.ts:88) — this is the right place (drops the node from neighbor/traceroute layers too), but it's worth confirming the embed's neighbor/traceroute data is also separately filtered or simply drops naturally because the node itself is absent. The PR description states this is handled; trust it but worth a manual test.

i18n

✅ All strings are in en.json. Toast strings use the inline fallback pattern (t('key', 'fallback string')) in App.tsx — consistent with the existing convention.

Minor: messages.hide_from_map_title and messages.unhide_from_map_title are defined in en.json (lines 3870, 3872) but the MessagesTab.tsx button doesn't appear to use them as a title attribute. The strings are in the locale file but unused in the UI. Low priority.

Test Coverage

✅ Good coverage: nodes.test.ts adds:

• Round-trip: set hideFromMap: true, upsert a packet update without the field, confirm the value is preserved
• Per-source isolation: same nodeNum in two sources has independent values

Gap: No route-level test — There's no test for POST /api/nodes/:nodeId/hide-from-map that verifies the 400 response for invalid hideFromMap type, missing sourceId, or malformed nodeId. The analogous position-override route does have route tests. Consider adding a hideFromMap.routes.test.ts or similar.

Gap: No optimistic-update reversal test — the frontend rollback on network error isn't covered by Vitest (it would require a React testing approach), but this is consistent with the rest of the codebase.

Summary

The implementation is clean, follows existing patterns, and covers all the critical map surfaces. The main action items are:

1. Investigate the sourceIdFrom: 'body' vs 'query' discrepancy between this route and the position-override route — confirm the middleware correctly reads from the right location for each.
2. Consider adding route-level tests for the new hide-from-map endpoint (invalid inputs, missing fields).
3. The unused i18n title strings are a cosmetic issue; use them on the button title attribute or remove them.

Claude finished @Yeraze's task —— View job

PR Review: feat(nodes): per-node "Hide from Map" toggle (#3549)

Status: Review complete ✅

• Read PR context and previous review
• Analyze migration and database layer
• Analyze backend routes and security
• Analyze frontend components and filtering
• Analyze test coverage
• Post final review

Overall this is a well-structured, well-tested implementation that closely mirrors the existing ignore-flag and position-override patterns. Here's my analysis:

Migration & Schema (092)

✅ All three backends are correctly handled. SQLite uses duplicate column catch, Postgres uses IF NOT EXISTS, MySQL checks information_schema. All idempotent. Column defaults to false/0, preserving existing behavior.

Minor cosmetic inconsistency: The Drizzle SQLite schema (nodes.ts:75) uses integer('hideFromMap', { mode: 'boolean' }) while the test DDL for MySQL (nodes.test.ts:149) uses bare BOOLEAN instead of TINYINT(1). The migration itself correctly uses TINYINT(1) DEFAULT 0 (092_add_hide_from_map_to_nodes.ts:63). Not a functional issue but the test DDL is slightly inconsistent with what the migration applies.

Backend Route (server.ts:2051)

✅ Input validation is thorough — boolean type check, sourceId presence, and 8-hex-char nodeId format are all validated before hitting the DB. This is better than some older routes.

✅ Permission scoping is correct. requirePermission('nodes', 'write', { sourceIdFrom: 'body' }) matches how favorite, favorite-lock, and ignored endpoints work. The position-override DELETE uses { sourceIdFrom: 'query' } (correct for a DELETE without a body), so the inconsistency noted in the prior review is explained by HTTP semantics — not a bug.

Known limitation (consistent with existing code): No WebSocket broadcast after toggle. Other browser sessions won't see the change until the next poll. The ignored and favorite routes have the same limitation — this is pre-existing design debt, not new here.

database.ts:7472 — setNodeHideFromMap

✅ PG/MySQL path correctly updates the cache before firing the async upsert, so sync readers see the new value immediately. The fire-and-forget catch mirrors setFavorite and setNodePositionOverride — a restart would revert the toggle if the DB write failed silently, but this is existing design debt shared across all toggle methods.

✅ SQLite path correctly throws if result.changes === 0 (node not found), and then syncs the in-memory cache. The raw SQL is correctly annotated with // eslint-disable-next-line no-restricted-syntax.

Observation: setNodeHideFromMapAsync (database.ts:9177) is declared async but doesn't await the underlying call — intentional and consistent with clearNodePositionOverrideAsync and others. The Async suffix is a convention marker for the test mock pattern, not an indicator of actual async behavior. Fine.

Frontend — Optimistic Update (App.tsx:4374)

✅ Pattern is correctly implemented. flushSync optimistic update → API call → rollback on 403 or error. All code paths call pendingHideFromMapRequests.delete(hfmKey) on failure (403 at line 4416, other errors in catch at line 4428), preventing stale pending entries from permanently blocking future toggles. Success path relies on poll to clear.

Minor: pendingHideFromMapRequests is a module-level Map singleton (App.tsx:104). It survives HMR but this is consistent with pendingFavoriteRequests and pendingIgnoredRequests on lines 102–103. Not introducing new risk.

Frontend — Map Filtering

✅ All four surfaces correctly filter hideFromMap:

• NodesTab.tsx:1344 — filters before hasValidEffectivePosition
• DashboardMap.tsx:239 — filters in the same pipeline as isIgnored
• NodeMarkersLayer.tsx:100 — filters before source-scope check
• embedPublicRoutes.ts:88 — filters at the data-source level (drops node from neighbor/traceroute layers automatically since they reference the filtered node set)

Minor note on DashboardMap.tsx:239: The filter chain has a subtle ordering difference from NodesTab — it filters isIgnored first, then hideFromMap, then applies the age/favorites cutoff. The order doesn't affect correctness but is worth keeping consistent if this pipeline gets refactored.

Test Coverage

✅ Repository tests are solid:

• Round-trip: hideFromMap: true survives a subsequent packet upsert that doesn't include the field (nodes.test.ts — upsertNode - round-trips and preserves hideFromMap across packet updates)
• Per-source isolation: same nodeNum in source-a and source-b have independent values

✅ Route-level tests exist (server.test.ts) — contrary to the prior review's "gap" note, there are tests for: set/clear the flag, reject non-boolean, reject missing sourceId, reject invalid nodeId format. Good coverage.

Gap (minor): The route test uses a simplified mock app (server.test.ts) that bypasses requirePermission middleware. There's no test verifying that a user without nodes:write gets a 403 from this endpoint. This is consistent with how other route tests are structured in this codebase, so not blocking.

i18n

✅ All strings present in en.json. The hide_from_map_title / unhide_from_map_title keys appear unused as title attributes in the UI — they're referenced as button labels in MessagesTab.tsx via toggleHideFromMap. If they were intended as tooltip title attributes, they're currently unused. Low priority.

Summary

This is a clean, well-scoped feature. It correctly limits the change to display-only behavior, preserves the flag across packet updates, and covers all four map surfaces. The implementation is conservative and consistent with existing patterns.

No blocking issues. Suggestions for follow-up:

1. Add title attribute to the hide/show map buttons in MessagesTab if the i18n *_title strings were intentional.
2. Consider a cross-session WebSocket broadcast for toggle state (future work, affects all toggle endpoints).

Thanks for the thorough review! Addressed in db98f18. Triage of each item:

1. sourceIdFrom: 'body' vs 'query' discrepancy — Verified, no change needed. The hide-from-map POST uses sourceIdFrom: 'body', which is identical to the position-override POST (server.ts:1881) and the ignored POST (server.ts:1701). The only 'query' usage is the position-override DELETE (server.ts:1999), which is correct because a DELETE carries sourceId as a query param, not a body. So the two routes the bot compared are POST-vs-DELETE, not a mismatch. Confirmed working end-to-end against a live source (permission check passes with the body sourceId).

2. Route-level tests — ✅ Added in db98f18. server.test.ts now covers POST /api/nodes/:nodeId/hide-from-map: success (set + clear) and 400s for non-boolean hideFromMap, missing sourceId, and malformed nodeId — mirroring the existing favorite/favorite-lock route test contract.

3. Unused i18n title strings — ✅ Removed messages.hide_from_map_title / messages.unhide_from_map_title in db98f18. The actions are text menu items with no title attribute, so the dead keys are gone.

Noted as accepted-by-design (consistent with existing patterns, not changed):

• No WebSocket broadcast after toggle — mirrors the ignore flag; web clients reconcile on the next poll. The frontend already does an optimistic update + per-source pending reconciliation.
• PG/MySQL fire-and-forget upsert + raw SQLite SQL in setNodeHideFromMap — intentionally identical to setNodePositionOverride/setFavorite; existing design debt, out of scope here.
• MySQL BOOLEAN in test DDL vs myBoolean in schema — cosmetic, both map to TINYINT(1).

Also verified live on the dev container: toggling persists, round-trips through /api/sources/:id/nodes, and the node remains in the node list while its marker is suppressed.