fix: address remaining bot-review findings on the geo_filter/Foreign Traffic work

- MAJOR: config.example.json was missing geoFilterExemptNodeList
  entirely — every other Config field in this repo gets a sibling
  _comment_<field> entry (established, actively-used convention
  throughout the file, ~20 existing examples), documented here to
  match. Also corrected the adjacent geo_filter _comment, which
  predates this PR and claimed it "restricts ingestion" — it doesn't;
  that's foreignAdverts.mode=drop's job, unrelated to this flag.
- MINOR: ?geoFilter=<v> only recognized the literal string "1" to
  enable and treated anything else (including "true") as disabling —
  a real footgun since "true" reads as an obviously valid boolean
  value. Now accepts 1/true and 0/false as explicit overrides; any
  other value (including absent) falls through to the deployment
  default instead of silently flipping it off.
- Tightened the Foreign Traffic tab's stop-hook test: it previously
  only asserted stop() doesn't throw, which a no-op stub would also
  pass. Sandbox's setInterval/clearInterval are now real spies
  tracking live/cleared ids, so the test verifies render() registers
  exactly one interval and stop() actually clears that exact id
  (idempotently on a second call).
- NIT: relays.sort now wraps unscoped_relay_count_24h in Number(...)
  defensively (AGENTS.md § Type Safety — cast at the boundary).
- NIT: foreignNodes.sort now parses last_seen once per node
  (decorate-sort-undecorate) instead of twice per comparator call.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

cmd/server/nodes_geofilter_optin_test.go

@@ -83,6 +83,22 @@ func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) {
 		}
 	})
 
+	t.Run("geoFilter=true/false are accepted synonyms for 1/0", func(t *testing.T) {
+		if !get("&geoFilter=false")["OutsideUntagged"] {
+			t.Error("expected OutsideUntagged to be present with geoFilter=false")
+		}
+		if get("&geoFilter=true")["OutsideUntagged"] {
+			t.Error("expected OutsideUntagged to be excluded with geoFilter=true")
+		}
+	})
+
+	t.Run("an unrecognized geoFilter value falls back to the deployment default instead of silently disabling", func(t *testing.T) {
+		got := get("&geoFilter=yes")
+		if got["OutsideUntagged"] {
+			t.Error("expected geoFilter=yes (not a recognized value) to fall back to the default (filtered), not silently disable filtering")
+		}
+	})
+
 	t.Run("GeoFilterExemptNodeList=true makes the default request return everything, without needing ?geoFilter=0", func(t *testing.T) {
 		srv.cfg.GeoFilterExemptNodeList = true
 		defer func() { srv.cfg.GeoFilterExemptNodeList = false }()

cmd/server/openapi.go

@@ -80,7 +80,7 @@ func routeDescriptions() map[string]routeMeta {
 			QueryParams: []paramMeta{
 				{Name: "role", Description: "Filter by node role", Type: "string"},
 				{Name: "status", Description: "Filter by status (active/stale/offline)", Type: "string"},
-				{Name: "geoFilter", Description: "Overrides the deployment's geo_filter node-list default for this one request: \"1\" excludes nodes outside the configured geo_filter (unless foreign_advert-tagged), \"0\" returns every node regardless. Omitting it uses the deployment default — geo_filter applies to the node list unless config.json sets geoFilterExemptNodeList=true.", Type: "string"},
+				{Name: "geoFilter", Description: "Overrides the deployment's geo_filter node-list default for this one request: \"1\"/\"true\" excludes nodes outside the configured geo_filter (unless foreign_advert-tagged), \"0\"/\"false\" returns every node regardless. Any other value (including omitting it) uses the deployment default — geo_filter applies to the node list unless config.json sets geoFilterExemptNodeList=true.", Type: "string"},
 			}},
 		"GET /api/nodes/search":             {Summary: "Search nodes", Description: "Search nodes by name or public key prefix.", Tag: "nodes", QueryParams: []paramMeta{{Name: "q", Description: "Search query", Type: "string", Required: true}}},
 		"GET /api/nodes/bulk-health":        {Summary: "Bulk node health", Description: "Returns health status for all nodes in one call.", Tag: "nodes"},

cmd/server/routes.go

@@ -1440,8 +1440,12 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) {
 	// tagging (and the explicit prune-geo-filter admin flow) are
 	// unaffected either way — this only gates the passive declutter view.
 	applyGeoFilter := !s.cfg.GeoFilterExemptNodeList
-	if v := q.Get("geoFilter"); v != "" {
-		applyGeoFilter = v == "1"
+	switch q.Get("geoFilter") {
+	case "1", "true":
+		applyGeoFilter = true
+	case "0", "false":
+		applyGeoFilter = false
+		// Any other value (including absent) falls through to the deployment default.
 	}
 	if s.cfg.GeoFilter != nil && applyGeoFilter {
 		filtered := nodes[:0]

config.example.json

@@ -238,12 +238,14 @@
       [37.20, -122.52]
     ],
     "bufferKm": 20,
-    "_comment": "Optional. Restricts ingestion and API responses to nodes within the polygon + bufferKm. Polygon is an array of [lat, lon] pairs (minimum 3). Use the GeoFilter tab in the Customizer (requires apiKey) or the GeoFilter Builder (`/geofilter-builder.html`) to draw a polygon visually and export a config snippet. Remove this section to disable filtering. Nodes with no GPS fix are always allowed through."
+    "_comment": "Optional. By default, also restricts GET /api/nodes (and therefore the live map) to nodes within the polygon + bufferKm — foreign_advert-tagged nodes are always kept regardless (#730, see foreignAdverts below); see geoFilterExemptNodeList to opt out of that. The prune-geo-filter admin flow always uses this polygon regardless of geoFilterExemptNodeList. Polygon is an array of [lat, lon] pairs (minimum 3). Use the GeoFilter tab in the Customizer (requires apiKey) or the GeoFilter Builder (`/geofilter-builder.html`) to draw a polygon visually and export a config snippet. Remove this section to disable filtering entirely. Nodes with no GPS fix are always allowed through."
   },
   "foreignAdverts": {
     "mode": "flag",
     "_comment": "Controls how the ingestor handles ADVERTs whose GPS is OUTSIDE the geo_filter polygon (#730). 'flag' (default): store the advert/node and tag it foreign_advert=1 so operators can see bridged/leaked nodes via the API ('foreign': true on /api/nodes). 'drop': legacy behavior — silently discard the advert (no log, no node row). Only applies when geo_filter is configured; otherwise has no effect."
   },
+  "geoFilterExemptNodeList": false,
+  "_comment_geoFilterExemptNodeList": "Optional, default false. When geo_filter is configured, GET /api/nodes (and therefore the live map) excludes out-of-polygon nodes by default — the long-standing #730 declutter behavior, preserved automatically for every deployment upgrading from before this field existed. Set to true if you want geo_filter purely for foreign_advert classification/analytics (e.g. a 'Foreign Traffic' investigation) WITHOUT also hiding out-of-polygon nodes from the node list/map. Per-request ?geoFilter=0 / ?geoFilter=1 on /api/nodes overrides either default for a single call. Has no effect on ingestion (foreignAdverts.mode) or the prune-geo-filter admin flow, both of which are unaffected by this flag.",
   "areas": {
     "_comment": "Optional. GPS-based display filter. Each entry defines a geographic area by polygon ([lat, lon] pairs) or bounding box (latMin/latMax/lonMin/lonMax). Packets and nodes are attributed to an area based on the transmitting node's own GPS coordinates — not the observer's location. Areas appear as a filter pill bar in the dashboard. Remove this section to disable the area filter UI.",
     "BAY": {

public/analytics.js

@@ -4877,7 +4877,7 @@ function destroy() { _stopRolesRefresh(); _stopScopesRefresh(); _stopForeignTraf
         const relays = allNodes.filter(function(n) {
           return (n.role === 'repeater' || n.role === 'room') && (n.unscoped_relay_count_24h || 0) > 0;
         });
-        relays.sort(function(a, b) { return (b.unscoped_relay_count_24h || 0) - (a.unscoped_relay_count_24h || 0); });
+        relays.sort(function(a, b) { return Number(b.unscoped_relay_count_24h || 0) - Number(a.unscoped_relay_count_24h || 0); });
         const foreignCount = allNodes.filter(function(n) { return n.foreign; }).length;
 
         let body;
@@ -4909,9 +4909,13 @@ function destroy() { _stopRolesRefresh(); _stopScopesRefresh(); _stopForeignTraf
         // first. Only set going forward from when geo_filter was
         // configured — this list grows as nodes send their next ADVERT.
         const foreignNodes = allNodes.filter(function(n) { return n.foreign; });
-        foreignNodes.sort(function(a, b) {
-          return new Date(b.last_seen || 0) - new Date(a.last_seen || 0);
-        });
+        // Parse last_seen once per node (decorate-sort-undecorate) instead
+        // of twice per comparator call — cheap either way at this list
+        // size, but avoids the repeated Date() allocations on principle.
+        foreignNodes
+          .map(function(n) { return { n: n, t: new Date(n.last_seen || 0).getTime() }; })
+          .sort(function(a, b) { return b.t - a.t; })
+          .forEach(function(entry, i) { foreignNodes[i] = entry.n; });
         let foreignNodesBody;
         if (foreignNodes.length > 0) {
           const foreignRows = foreignNodes.map(function(n) {

test-analytics-foreign-traffic-tab.js

@@ -47,7 +47,7 @@ function makeSandbox() {
     console, Date, Infinity, Math, Array, Object, String, Number, JSON, RegExp,
     Error, TypeError, parseInt, parseFloat, isNaN, isFinite,
     encodeURIComponent, decodeURIComponent,
-    setTimeout: () => {}, clearTimeout: () => {}, setInterval: () => {}, clearInterval: () => {},
+    setTimeout: () => {}, clearTimeout: () => {},
     fetch: () => Promise.resolve({ json: () => Promise.resolve({}) }),
     performance: { now: () => Date.now() },
     localStorage: (() => { const s = {}; return { getItem: k => s[k] || null, setItem: (k, v) => { s[k] = String(v); }, removeItem: k => { delete s[k]; } }; })(),
@@ -59,6 +59,26 @@ function makeSandbox() {
     dispatchEvent: () => {},
     requestAnimationFrame: (cb) => setTimeout(cb, 0),
   };
+  // Spies (not just no-ops) so tests can verify a timer that was really
+  // registered gets really cleared, instead of only asserting stop()
+  // doesn't throw — a `function stop(){}` no-op would pass that alone.
+  // Defined as closures (not object methods) so they work under the
+  // sandboxed code's 'use strict', where a bare `setInterval(...)` call
+  // has `this === undefined`.
+  let nextIntervalId = 1;
+  const liveIntervalIds = new Set();
+  const clearedIntervalIds = [];
+  ctx.__liveIntervalIds = liveIntervalIds;
+  ctx.__clearedIntervalIds = clearedIntervalIds;
+  ctx.setInterval = function () {
+    const id = nextIntervalId++;
+    liveIntervalIds.add(id);
+    return id;
+  };
+  ctx.clearInterval = function (id) {
+    liveIntervalIds.delete(id);
+    clearedIntervalIds.push(id);
+  };
   vm.createContext(ctx);
   return ctx;
 }
@@ -191,15 +211,26 @@ function fakeEl() {
     assert.ok(el.innerHTML.includes('None yet'), 'placeholder message should be shown when no node is foreign-flagged');
   });
 
-  await testAsync('a stop-refresh hook is exported and safe to call repeatedly (idempotent)', async () => {
+  await testAsync('rendering registers a real interval, and stop() actually clears it (not a no-op)', async () => {
     const ctx = makeAnalyticsSandbox([]);
     const stop = ctx.window._analyticsStopForeignTrafficRefresh;
     assert.strictEqual(typeof stop, 'function', '_stopForeignTrafficRefresh must be exported for testing/cleanup');
-    // Before any render, and after — must not throw either way.
+
+    // Calling stop() before any render must not throw (no timer registered yet).
     stop();
+    assert.strictEqual(ctx.__clearedIntervalIds.length, 0, 'stop() before any render should not call clearInterval at all');
+
     await ctx.window._analyticsRenderForeignTrafficTab(fakeEl());
+    assert.strictEqual(ctx.__liveIntervalIds.size, 1, 'render should register exactly one live interval');
+    const [registeredId] = ctx.__liveIntervalIds;
+
     stop();
+    assert.strictEqual(ctx.__liveIntervalIds.size, 0, 'stop() should leave zero live intervals');
+    assert.deepStrictEqual(ctx.__clearedIntervalIds, [registeredId], 'stop() should call clearInterval with the exact id that render registered');
+
+    // Idempotent: calling stop() again with nothing live must not clear anything a second time.
     stop();
+    assert.deepStrictEqual(ctx.__clearedIntervalIds, [registeredId], 'a second stop() call must not clear anything again — the timer reference should already be null');
   });
 
   console.log('\n════════════════════════════════════════');