fix: address remaining bot-review findings on PR #1852

Working through the outstanding non-blocker findings across all five
automated review passes on the PR:

- MAJOR: GetChannelMessages' rows.Scan() error was silently discarded
  — a schema mismatch would produce zero-valued messages instead of a
  visible error. Now returns the error, matching sibling query loops.
- Extracted the hashRegions name-normalization rule (trim, "#"-prefix,
  dedupe) shared between cmd/ingestor's loadRegionKeys and cmd/server's
  region-utilization diff into a new internal/regions package, so the
  two can no longer drift apart on the rule independently.
- Batched GetRepeaterNamesByKeys' SQL IN (...) clause in chunks of 500
  — an unbounded clause risks SQLITE_MAX_VARIABLE_NUMBER on very large
  deployments' byPathHop candidate sets.
- handleScopeStats' remaining silently-swallowed err==nil branches
  (GetMatchedRegionNames, GetNodesByDefaultScope,
  GetChannelMessageScopeStats, GetChannelScopeAdoption) now log a WARN
  breadcrumb on failure instead of failing invisibly.
- Scope Adoption table was rendering 3 of the 4 ChannelScopeAdoption
  fields (Unscoped omitted) — the visible numbers didn't reconcile to
  the message total without doing the subtraction by hand. Added the
  column.
- Removed a duplicate `vertical-align: middle` declaration on
  .badge-transport (dead, not a behavior change).
- Deleted ChannelMessageResp — flagged for interface{} vs *string/*int
  typing, but turned out to be completely unused dead code; removing
  it resolves the finding more directly than retyping something
  nothing constructs.

Left two NIT/MINOR items as-is with reasoning:
- renderRegionNodeGroups' inline styles match the same pattern used in
  15+ other places in analytics.js — "fixing" only this one function
  would make it less consistent with the file, not more.
- TransmissionResp's interface{} fields (a different struct than the
  one just removed) are an established, actively-used pattern for
  nullable SQL-scanned values across that whole response type;
  retyping it is a much larger, unrelated refactor.

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

cmd/ingestor/go.mod

@@ -51,3 +51,7 @@ replace github.com/meshcore-analyzer/prunequeue => ../../internal/prunequeue
 require github.com/meshcore-analyzer/mbcapqueue v0.0.0
 
 replace github.com/meshcore-analyzer/mbcapqueue => ../../internal/mbcapqueue
+
+require github.com/meshcore-analyzer/regions v0.0.0
+
+replace github.com/meshcore-analyzer/regions => ../../internal/regions

cmd/ingestor/main.go

@@ -23,6 +23,7 @@ import (
 	"time"
 
 	mqtt "github.com/eclipse/paho.mqtt.golang"
+	"github.com/meshcore-analyzer/regions"
 )
 
 func main() {
@@ -1438,14 +1439,11 @@ func loadChannelKeys(cfg *Config, configPath string) map[string]string {
 func loadRegionKeys(cfg *Config) map[string][]byte {
 	keys := make(map[string][]byte)
 	for _, raw := range cfg.HashRegions {
-		name := strings.TrimSpace(raw)
+		name, ok := regions.Normalize(raw)
-		if name == "" {
+		if !ok {
 			log.Printf("[regions] skipping empty hashRegions entry")
 			continue
 		}
-		if !strings.HasPrefix(name, "#") {
-			name = "#" + name
-		}
 		if _, exists := keys[name]; exists {
 			log.Printf("[regions] duplicate region %q ignored", name)
 			continue

cmd/server/db.go

@@ -1912,7 +1912,9 @@ func (db *DB) GetChannelMessages(channelHash string, limit, offset int, region .
 		if db.hasScopeName {
 			scanArgs = append(scanArgs, &scopeName)
 		}
-		rows.Scan(scanArgs...)
+		if err := rows.Scan(scanArgs...); err != nil {
+			return nil, 0, err
+		}
 		if !dj.Valid {
 			continue
 		}
@@ -2163,35 +2165,48 @@ func (db *DB) GetNodeLocationsByKeys(keys []string) map[string]map[string]interf
 // query doubles as the "is this actually a distinct node" existence check.
 // A matched pubkey with an empty/unset name falls back to itself so a
 // real repeater is never silently dropped just because it has no name yet.
+//
+// Queried in chunks of repeaterNamesByKeysBatchSize — SQLite's default
+// SQLITE_MAX_VARIABLE_NUMBER is 999 on older builds, and a large mesh's
+// byPathHop candidate set can exceed that in one IN (...) clause.
+const repeaterNamesByKeysBatchSize = 500
+
 func (db *DB) GetRepeaterNamesByKeys(keys []string) map[string]string {
 	result := make(map[string]string)
 	if len(keys) == 0 {
 		return result
 	}
-	placeholders := make([]string, len(keys))
+	for start := 0; start < len(keys); start += repeaterNamesByKeysBatchSize {
-	args := make([]interface{}, len(keys))
+		end := start + repeaterNamesByKeysBatchSize
-	for i, k := range keys {
+		if end > len(keys) {
-		placeholders[i] = "?"
+			end = len(keys)
-		args[i] = strings.ToLower(k)
+		}
-	}
+		chunk := keys[start:end]
-	query := "SELECT public_key, name FROM nodes WHERE role IN ('repeater','room') AND public_key IN (" + strings.Join(placeholders, ",") + ")"
+		placeholders := make([]string, len(chunk))
-	rows, err := db.conn.Query(query, args...)
+		args := make([]interface{}, len(chunk))
-	if err != nil {
+		for i, k := range chunk {
-		return result
+			placeholders[i] = "?"
-	}
+			args[i] = strings.ToLower(k)
-	defer rows.Close()
+		}
-	for rows.Next() {
+		query := "SELECT public_key, name FROM nodes WHERE role IN ('repeater','room') AND public_key IN (" + strings.Join(placeholders, ",") + ")"
-		var pk string
+		rows, err := db.conn.Query(query, args...)
-		var name sql.NullString
+		if err != nil {
-		if rows.Scan(&pk, &name) != nil {
 			continue
 		}
-		pk = strings.ToLower(pk)
+		for rows.Next() {
-		if name.Valid && name.String != "" {
+			var pk string
-			result[pk] = name.String
+			var name sql.NullString
-		} else {
+			if rows.Scan(&pk, &name) != nil {
-			result[pk] = pk
+				continue
+			}
+			pk = strings.ToLower(pk)
+			if name.Valid && name.String != "" {
+				result[pk] = name.String
+			} else {
+				result[pk] = pk
+			}
 		}
+		rows.Close()
 	}
 	return result
 }

cmd/server/go.mod

@@ -56,3 +56,7 @@ require (
 )
 
 replace github.com/meshcore-analyzer/mbcapqueue => ../../internal/mbcapqueue
+
+require github.com/meshcore-analyzer/regions v0.0.0
+
+replace github.com/meshcore-analyzer/regions => ../../internal/regions

cmd/server/repeater_names_by_keys_batch_test.go

@@ -0,0 +1,72 @@
+package main
+
+import (
+	"fmt"
+	"testing"
+)
+
+// TestGetRepeaterNamesByKeys_Basic covers the role filter and the
+// "unnamed repeater falls back to its own key" behavior.
+func TestGetRepeaterNamesByKeys_Basic(t *testing.T) {
+	db := setupTestDB(t)
+	insertTestNode(t, db, "repeaterkey1", "Repeater One", "repeater")
+	insertTestNode(t, db, "roomkey1", "", "room") // unnamed — should fall back to its own key
+	insertTestNode(t, db, "clientkey1", "Some Client", "client")
+
+	result := db.GetRepeaterNamesByKeys([]string{"repeaterkey1", "roomkey1", "clientkey1", "nonexistent"})
+
+	if got := result["repeaterkey1"]; got != "Repeater One" {
+		t.Errorf("repeaterkey1 = %q, want %q", got, "Repeater One")
+	}
+	if got := result["roomkey1"]; got != "roomkey1" {
+		t.Errorf("unnamed room should fall back to its own key, got %q", got)
+	}
+	if _, ok := result["clientkey1"]; ok {
+		t.Error("client role should be excluded — only repeater/room are relays")
+	}
+	if _, ok := result["nonexistent"]; ok {
+		t.Error("a key with no matching node row should not appear in the result")
+	}
+}
+
+// TestGetRepeaterNamesByKeys_BatchesAcrossChunkBoundary is a regression test
+// for the SQL IN (...) clause batching (bot review on PR #1852: an
+// unbounded IN clause risks hitting SQLite's SQLITE_MAX_VARIABLE_NUMBER on
+// large deployments). Inserts more repeaters than
+// repeaterNamesByKeysBatchSize and asserts every single one still resolves
+// — proving the chunking loop doesn't drop or duplicate results at the
+// batch boundary.
+func TestGetRepeaterNamesByKeys_BatchesAcrossChunkBoundary(t *testing.T) {
+	db := setupTestDB(t)
+	n := repeaterNamesByKeysBatchSize + 50 // spans two chunks
+	keys := make([]string, 0, n)
+	for i := 0; i < n; i++ {
+		key := fmt.Sprintf("repkey%04d", i)
+		insertTestNode(t, db, key, fmt.Sprintf("Repeater %d", i), "repeater")
+		keys = append(keys, key)
+	}
+
+	result := db.GetRepeaterNamesByKeys(keys)
+
+	if len(result) != n {
+		t.Fatalf("resolved %d of %d repeaters across the chunk boundary, want all %d", len(result), n, n)
+	}
+	for i, key := range keys {
+		want := fmt.Sprintf("Repeater %d", i)
+		if got := result[key]; got != want {
+			t.Errorf("key %s = %q, want %q", key, got, want)
+		}
+	}
+}
+
+// insertTestNode inserts a minimal nodes row for GetRepeaterNamesByKeys tests.
+func insertTestNode(t *testing.T, db *DB, pubkey, name, role string) {
+	t.Helper()
+	_, err := db.conn.Exec(
+		`INSERT INTO nodes (public_key, name, role) VALUES (?, ?, ?)`,
+		pubkey, name, role,
+	)
+	if err != nil {
+		t.Fatalf("insert test node %s: %v", pubkey, err)
+	}
+}

cmd/server/routes.go

@@ -20,6 +20,7 @@ import (
 	"github.com/gorilla/mux"
 	"github.com/meshcore-analyzer/packetpath"
 	"github.com/meshcore-analyzer/prunequeue"
+	regionutil "github.com/meshcore-analyzer/regions"
 )
 
 // memBreakdownNote is the static accounting caveat attached to the opt-in
@@ -3453,29 +3454,6 @@ func (s *Server) handleDroppedPackets(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, results)
 }
 
-// normalizeRegionNames mirrors cmd/ingestor's loadRegionKeys name handling
-// (trim, ensure leading "#", dedupe) but only needs the names — the server
-// never derives HMAC keys, it just diffs configured names against observed
-// scope_name values for region-utilization analytics.
-func normalizeRegionNames(raw []string) []string {
-	seen := make(map[string]bool, len(raw))
-	out := make([]string, 0, len(raw))
-	for _, r := range raw {
-		name := strings.TrimSpace(r)
-		if name == "" {
-			continue
-		}
-		if !strings.HasPrefix(name, "#") {
-			name = "#" + name
-		}
-		if !seen[name] {
-			seen[name] = true
-			out = append(out, name)
-		}
-	}
-	return out
-}
-
 func (s *Server) handleScopeStats(w http.ResponseWriter, r *http.Request) {
 	const scopeStatsTTL = 30 * time.Second
 
@@ -3505,7 +3483,7 @@ func (s *Server) handleScopeStats(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if s.cfg != nil && len(s.cfg.HashRegions) > 0 {
-		configured := normalizeRegionNames(s.cfg.HashRegions)
+		configured := regionutil.NormalizeNames(s.cfg.HashRegions)
 		resp.ConfiguredRegions = len(configured)
 		if matched, err := s.db.GetMatchedRegionNames(); err == nil {
 			unused := make([]string, 0, len(configured))
@@ -3516,6 +3494,8 @@ func (s *Server) handleScopeStats(w http.ResponseWriter, r *http.Request) {
 			}
 			sort.Strings(unused)
 			resp.UnusedRegions = unused
+		} else {
+			log.Printf("WARN GetMatchedRegionNames: %v", err)
 		}
 	}
 
@@ -3600,22 +3580,30 @@ func (s *Server) handleScopeStats(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	if byScope, err := s.db.GetNodesByDefaultScope(); err == nil && len(byScope) > 0 {
+	if byScope, err := s.db.GetNodesByDefaultScope(); err == nil {
-		originating := make([]ScopeRegionRepeaters, 0, len(byScope))
+		if len(byScope) > 0 {
-		for region, refs := range byScope {
+			originating := make([]ScopeRegionRepeaters, 0, len(byScope))
-			sort.Slice(refs, func(i, j int) bool { return refs[i].Name < refs[j].Name })
+			for region, refs := range byScope {
-			originating = append(originating, ScopeRegionRepeaters{Region: region, Count: len(refs), Repeaters: refs})
+				sort.Slice(refs, func(i, j int) bool { return refs[i].Name < refs[j].Name })
+				originating = append(originating, ScopeRegionRepeaters{Region: region, Count: len(refs), Repeaters: refs})
+			}
+			sort.Slice(originating, func(i, j int) bool { return originating[i].Count > originating[j].Count })
+			resp.OriginatingNodesByRegion = originating
 		}
-		sort.Slice(originating, func(i, j int) bool { return originating[i].Count > originating[j].Count })
+	} else {
-		resp.OriginatingNodesByRegion = originating
+		log.Printf("WARN GetNodesByDefaultScope: %v", err)
 	}
 
 	if chanStats, err := s.db.GetChannelMessageScopeStats(window); err == nil {
 		resp.ChannelMessages = chanStats
+	} else {
+		log.Printf("WARN GetChannelMessageScopeStats: %v", err)
 	}
 
 	if adoption, err := s.db.GetChannelScopeAdoption(window); err == nil {
 		resp.ChannelScopeAdoption = adoption
+	} else {
+		log.Printf("WARN GetChannelScopeAdoption: %v", err)
 	}
 
 	s.scopeStatsMu.Lock()

cmd/server/types.go

@@ -979,21 +979,6 @@ type ChannelListResponse struct {
 	Channels []map[string]interface{} `json:"channels"`
 }
 
-type ChannelMessageResp struct {
-	Sender          string      `json:"sender"`
-	Text            string      `json:"text"`
-	Timestamp       string      `json:"timestamp"`
-	SenderTimestamp interface{} `json:"sender_timestamp"`
-	PacketID        int64       `json:"packetId"`
-	PacketHash      string      `json:"packetHash"`
-	Repeats         int         `json:"repeats"`
-	Observers       []string    `json:"observers"`
-	Hops            int         `json:"hops"`
-	SNR             interface{} `json:"snr"`
-	Scope           interface{} `json:"scope"`
-	RouteType       interface{} `json:"routeType"`
-}
-
 type ChannelMessagesResponse struct {
 	Messages []map[string]interface{} `json:"messages"`
 	Total    int                      `json:"total"`

internal/regions/go.mod

@@ -0,0 +1,3 @@
+module github.com/meshcore-analyzer/regions
+
+go 1.22

internal/regions/regions.go

@@ -0,0 +1,39 @@
+// Package regions holds the region-name normalization rules shared between
+// cmd/ingestor (which derives HMAC keys from hashRegions) and cmd/server
+// (which only needs the names to diff configured regions against observed
+// scope_name values for region-utilization analytics). Keeping this in one
+// place avoids the two normalizing hashRegions independently and drifting
+// apart on the trim/prefix/dedupe rules.
+package regions
+
+import "strings"
+
+// Normalize applies the hashRegions name convention to a single raw config
+// entry: trim whitespace, ensure a leading "#", and reject blank entries.
+// Returns ok=false for an entry that normalizes to nothing.
+func Normalize(raw string) (name string, ok bool) {
+	name = strings.TrimSpace(raw)
+	if name == "" {
+		return "", false
+	}
+	if !strings.HasPrefix(name, "#") {
+		name = "#" + name
+	}
+	return name, true
+}
+
+// NormalizeNames normalizes and deduplicates a raw hashRegions list,
+// preserving first-seen order.
+func NormalizeNames(raw []string) []string {
+	seen := make(map[string]bool, len(raw))
+	out := make([]string, 0, len(raw))
+	for _, r := range raw {
+		name, ok := Normalize(r)
+		if !ok || seen[name] {
+			continue
+		}
+		seen[name] = true
+		out = append(out, name)
+	}
+	return out
+}

internal/regions/regions_test.go

@@ -0,0 +1,50 @@
+package regions
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestNormalize(t *testing.T) {
+	cases := []struct {
+		raw     string
+		wantOk  bool
+		wantVal string
+	}{
+		{"dk", true, "#dk"},
+		{"#dk", true, "#dk"},
+		{"  dk-oj  ", true, "#dk-oj"},
+		{"  #dk-oj  ", true, "#dk-oj"},
+		{"", false, ""},
+		{"   ", false, ""},
+	}
+	for _, c := range cases {
+		got, ok := Normalize(c.raw)
+		if ok != c.wantOk || got != c.wantVal {
+			t.Errorf("Normalize(%q) = (%q, %v), want (%q, %v)", c.raw, got, ok, c.wantVal, c.wantOk)
+		}
+	}
+}
+
+func TestNormalizeNames(t *testing.T) {
+	got := NormalizeNames([]string{"dk", "#dk", " dk-oj ", "", "  ", "dk-oj"})
+	want := []string{"#dk", "#dk-oj"}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("NormalizeNames = %v, want %v", got, want)
+	}
+}
+
+func TestNormalizeNamesPreservesFirstSeenOrder(t *testing.T) {
+	got := NormalizeNames([]string{"zeta", "alpha", "#zeta"})
+	want := []string{"#zeta", "#alpha"}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("NormalizeNames = %v, want %v (order should follow first appearance, not be sorted)", got, want)
+	}
+}
+
+func TestNormalizeNamesEmptyInput(t *testing.T) {
+	got := NormalizeNames(nil)
+	if len(got) != 0 {
+		t.Errorf("NormalizeNames(nil) = %v, want empty", got)
+	}
+}

public/analytics.js

@@ -4602,11 +4602,12 @@ function destroy() { _stopRolesRefresh(); _stopScopesRefresh(); _analyticsData =
               '<td><code>' + esc(ca.channel) + '</code></td>' +
               '<td>' + ca.totalMessages.toLocaleString() + '</td>' +
               '<td>' + ca.scoped.toLocaleString() + ' (' + pct(ca.scoped, caOverall) + ')</td>' +
+              '<td>' + ca.unscoped.toLocaleString() + '</td>' +
               '<td>' + ca.unknownScope.toLocaleString() + '</td>' +
               '</tr>';
           }).join('');
           adoptBody = '<table class="data-table analytics-table">' +
-            '<thead><tr><th>Channel</th><th>Messages</th><th>Scoped</th><th>Unknown</th></tr></thead>' +
+            '<thead><tr><th>Channel</th><th>Messages</th><th>Scoped</th><th>Unscoped</th><th>Unknown</th></tr></thead>' +
             '<tbody>' + adoptRows + '</tbody>' +
             '</table>';
         } else {

public/style.css

@@ -1428,7 +1428,7 @@ body.scroll-locked { overflow: hidden; }
      ellipsis instead of blowing out the Type column; full name is
      always in the title attribute. */
   max-width: 90px; overflow: hidden; text-overflow: ellipsis;
-  white-space: nowrap; vertical-align: middle;
+  white-space: nowrap;
 }
 /* Transport-scoped but the region couldn't be resolved (no configured
    region matched, or an HMAC collision made the match ambiguous) —