Improvements to node filtering and storage (#839)

* Improves node filtering and voltage display Ensures voltage values are always displayed as positive. Enhances node filtering logic to handle unknown or undefined values, preventing nodes from being unexpectedly hidden. Updates UI labels for clarity. Improves Nodes page responsiveness by debouncing node updates and optimizing selector usage, preventing unnecessary re-renders. Adds validation warnings for key conflicts between nodes. * Update packages/web/src/core/stores/nodeDBStore/nodeDBStore.test.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update packages/web/src/core/stores/nodeDBStore/nodeValidation.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Revert copilot suggestion * Review changes --------- Co-authored-by: philon- <philon-@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2025-09-12 17:21:13 +02:00
parent 52fd2f712c
commit 3e2fe721d3
14 changed files with 488 additions and 100 deletions
@@ -2,6 +2,7 @@ import { useDeviceContext } from "@core/hooks/useDeviceContext";
 import { type Device, useDeviceStore } from "@core/stores/deviceStore";
 import { type MessageStore, useMessageStore } from "@core/stores/messageStore";
 import { type NodeDB, useNodeDBStore } from "@core/stores/nodeDBStore";
+import { bindStoreToDevice } from "@core/stores/utils/bindStoreToDevice";

 export {
  CurrentDeviceContext,
@@ -30,13 +31,11 @@ export {
 } from "@core/stores/sidebarStore";

 // Define hooks to access the stores
-export const useNodeDB = (): NodeDB => {
-  const { deviceId } = useDeviceContext();
-  const nodeDB = useNodeDBStore(
-    (s) => s.getNodeDB(deviceId) ?? s.addNodeDB(deviceId),
-  );
-  return nodeDB;
-};
+export const useNodeDB = bindStoreToDevice(
+  useNodeDBStore,
+  (s, deviceId): NodeDB => s.getNodeDB(deviceId) ?? s.addNodeDB(deviceId),
+);
+
 export const useDevice = (): Device => {
  const { deviceId } = useDeviceContext();

@@ -45,6 +44,7 @@ export const useDevice = (): Device => {
  );
  return device;
 };
+
 export const useMessages = (): MessageStore => {
  const { deviceId } = useDeviceContext();

@@ -393,7 +393,7 @@ const persistOptions: PersistOptions<
  PrivateMessageStoreState,
  MessageStorePersisted
 > = {
-  name: "meshtastic-MessageStore-store",
+  name: "meshtastic-message-store",
  storage: createStorage<MessageStorePersisted>(),
  version: CURRENT_STORE_VERSION,
  partialize: (s): MessageStorePersisted => ({
@@ -6,7 +6,11 @@ import { createStorage } from "@core/stores/utils/indexDB.ts";
 import { Protobuf, type Types } from "@meshtastic/core";
 import { produce } from "immer";
 import { create as createStore, type StateCreator } from "zustand";
-import { type PersistOptions, persist } from "zustand/middleware";
+import {
+  type PersistOptions,
+  persist,
+  subscribeWithSelector,
+} from "zustand/middleware";
 import type { NodeError, NodeErrorType, ProcessPacketParams } from "./types.ts";

 const CURRENT_STORE_VERSION = 0;
@@ -102,7 +106,8 @@ function nodeDBFactory(
            // Validation failed and error has been set inside validateIncomingNode
            return;
          }
-          nodeDB.nodeMap.set(node.num, next);
+
+          nodeDB.nodeMap = new Map(nodeDB.nodeMap).set(node.num, next);
        }),
      ),

@@ -113,7 +118,9 @@ function nodeDBFactory(
          if (!nodeDB) {
            throw new Error(`No nodeDB found (id: ${id})`);
          }
-          nodeDB.nodeMap.delete(nodeNum);
+          const updated = new Map(nodeDB.nodeMap);
+          updated.delete(nodeNum);
+          nodeDB.nodeMap = updated;
        }),
      ),

@@ -147,7 +154,10 @@ function nodeDBFactory(
          if (!nodeDB) {
            throw new Error(`No nodeDB found (id: ${id})`);
          }
-          nodeDB.nodeErrors.set(nodeNum, { node: nodeNum, error });
+          nodeDB.nodeErrors = new Map(nodeDB.nodeErrors).set(nodeNum, {
+            node: nodeNum,
+            error,
+          });
        }),
      ),

@@ -158,7 +168,9 @@ function nodeDBFactory(
          if (!nodeDB) {
            throw new Error(`No nodeDB found (id: ${id})`);
          }
-          nodeDB.nodeErrors.delete(nodeNum);
+          const updated = new Map(nodeDB.nodeErrors);
+          updated.delete(nodeNum);
+          nodeDB.nodeErrors = updated;
        }),
      ),

@@ -181,16 +193,21 @@ function nodeDBFactory(
            throw new Error(`No nodeDB found (id: ${id})`);
          }
          const node = nodeDB.nodeMap.get(data.from);
+          const nowSec = Math.floor(Date.now() / 1000); // lastHeard is in seconds(!)
+
          if (node) {
-            node.lastHeard = data.time > 0 ? data.time : Date.now(); // fallback to now if time is 0 or negative
-            node.snr = data.snr;
-            nodeDB.nodeMap.set(data.from, node);
+            const updated = {
+              ...node,
+              lastHeard: data.time > 0 ? data.time : nowSec,
+              snr: data.snr,
+            };
+            nodeDB.nodeMap = new Map(nodeDB.nodeMap).set(data.from, updated);
          } else {
-            nodeDB.nodeMap.set(
+            nodeDB.nodeMap = new Map(nodeDB.nodeMap).set(
              data.from,
              create(Protobuf.Mesh.NodeInfoSchema, {
                num: data.from,
-                lastHeard: data.time > 0 ? data.time : Date.now(), // fallback to now if time is 0 or negative,
+                lastHeard: data.time > 0 ? data.time : nowSec, // fallback to now if time is 0 or negative,
                snr: data.snr,
              }),
            );
@@ -208,9 +225,8 @@ function nodeDBFactory(
          const current =
            nodeDB.nodeMap.get(user.from) ??
            create(Protobuf.Mesh.NodeInfoSchema);
-          current.user = user.data;
-          current.num = user.from;
-          nodeDB.nodeMap.set(user.from, current);
+          const updated = { ...current, user: user.data, num: user.from };
+          nodeDB.nodeMap = new Map(nodeDB.nodeMap).set(user.from, updated);
        }),
      ),

@@ -224,9 +240,12 @@ function nodeDBFactory(
          const current =
            nodeDB.nodeMap.get(position.from) ??
            create(Protobuf.Mesh.NodeInfoSchema);
-          current.position = position.data;
-          current.num = position.from;
-          nodeDB.nodeMap.set(position.from, current);
+          const updated = {
+            ...current,
+            position: position.data,
+            num: position.from,
+          };
+          nodeDB.nodeMap = new Map(nodeDB.nodeMap).set(position.from, updated);
        }),
      ),

@@ -259,22 +278,25 @@ function nodeDBFactory(
              };

              const setErrorProxy = (nodeNum: number, err: NodeErrorType) => {
-                mergedErrors.set(nodeNum, { error: err } as NodeError);
+                mergedErrors.set(nodeNum, {
+                  node: nodeNum,
+                  error: err,
+                });
              };

-              for (const [nodeNum, newNode] of newDB.nodeMap) {
+              for (const [num, newNode] of newDB.nodeMap) {
                const next = validateIncomingNode(
                  newNode,
                  setErrorProxy,
                  getNodesProxy,
                );
                if (next) {
-                  mergedNodes.set(nodeNum, next);
+                  mergedNodes.set(num, next);
                }

-                const err = newDB.getNodeError(nodeNum);
-                if (err && !oldDB.hasNodeError(nodeNum)) {
-                  mergedErrors.set(nodeNum, err);
+                const err = newDB.getNodeError(num);
+                if (err && !oldDB.hasNodeError(num)) {
+                  mergedErrors.set(num, err);
                }
              }

@@ -297,7 +319,10 @@ function nodeDBFactory(

          const node = nodeDB.nodeMap.get(nodeNum);
          if (node) {
-            node.isFavorite = isFavorite;
+            nodeDB.nodeMap = new Map(nodeDB.nodeMap).set(nodeNum, {
+              ...node,
+              isFavorite: isFavorite,
+            });
          }
        }),
      ),
@@ -312,7 +337,10 @@ function nodeDBFactory(

          const node = nodeDB.nodeMap.get(nodeNum);
          if (node) {
-            node.isIgnored = isIgnored;
+            nodeDB.nodeMap = new Map(nodeDB.nodeMap).set(nodeNum, {
+              ...node,
+              isIgnored: isIgnored,
+            });
          }
        }),
      ),
@@ -392,7 +420,7 @@ export const nodeDBInitializer: StateCreator<PrivateNodeDBState> = (
    const nodeDB = nodeDBFactory(id, get, set);
    set(
      produce<PrivateNodeDBState>((draft) => {
-        draft.nodeDBs.set(id, nodeDB);
+        draft.nodeDBs = new Map(draft.nodeDBs).set(id, nodeDB);

        // Enforce retention limit
        evictOldestEntries(draft.nodeDBs, NODEDB_RETENTION_NUM);
@@ -404,7 +432,9 @@ export const nodeDBInitializer: StateCreator<PrivateNodeDBState> = (
  removeNodeDB: (id) => {
    set(
      produce<PrivateNodeDBState>((draft) => {
-        draft.nodeDBs.delete(id);
+        const updated = new Map(draft.nodeDBs);
+        updated.delete(id);
+        draft.nodeDBs = updated;
      }),
    );
  },
@@ -472,7 +502,7 @@ console.debug(
 );

 export const useNodeDBStore = persistNodes
-  ? createStore<PrivateNodeDBState, [["zustand/persist", NodeDBPersisted]]>(
-      persist(nodeDBInitializer, persistOptions),
+  ? createStore(
+      subscribeWithSelector(persist(nodeDBInitializer, persistOptions)),
    )
-  : createStore<PrivateNodeDBState>()(nodeDBInitializer);
+  : createStore(subscribeWithSelector(nodeDBInitializer));
@@ -1,7 +1,6 @@
-/** biome-ignore-all lint/suspicious/noExplicitAny: <tests> */
-/** biome-ignore-all lint/style/noNonNullAssertion: <tests> */
 import { create } from "@bufbuild/protobuf";
 import { Protobuf } from "@meshtastic/core";
+import { act, render, screen } from "@testing-library/react";
 import { toByteArray } from "base64-js";
 import { beforeEach, describe, expect, it, vi } from "vitest";

@@ -18,6 +17,14 @@ vi.mock("idb-keyval", () => ({
  }),
 }));

+let deviceIdForTests = 1;
+vi.mock("@core/hooks/useDeviceContext", () => ({
+  useDeviceContext: () => ({ deviceId: deviceIdForTests }),
+  __setDeviceId: (id: number) => {
+    deviceIdForTests = id;
+  },
+}));
+
 // import a fresh copy of the store module (because the store is created at import time)
 async function freshStore(persist = false) {
  vi.resetModules();
@@ -33,8 +40,9 @@ async function freshStore(persist = false) {
    },
  }));

-  const mod = await import("./index.ts");
-  return mod;
+  const storeMod = await import("./index.ts");
+  const { useNodeDB } = await import("../index.ts");
+  return { ...storeMod, useNodeDB };
 }

 function makeNode(num: number, extras: Record<string, any> = {}) {
@@ -97,7 +105,7 @@ describe("NodeDB store", () => {
    expect(db.getNode(50)?.snr).toBe(9);

    db.processPacket({ from: 50, time: 0, snr: 9 } as any);
-    expect(db.getNode(50)?.lastHeard).toBeCloseTo(Date.now(), -1); // within 10ms
+    expect(db.getNode(50)?.lastHeard).toBeCloseTo(Date.now() / 1000, -1); // within 1s, note lastHeard is in seconds
    expect(db.getNode(50)?.snr).toBe(9);
  });

@@ -336,9 +344,9 @@ describe("NodeDB – merge semantics, PKI checks & extras", () => {
    expect(n5.user?.publicKey).toEqual(keyOld); // keep old PK
    expect(n5.user?.longName).toBe("old-5");

-    // error flagged
+    // error not flagged; dropped silently
    const err = newDB!.getNodeError(5);
-    expect(String(err!.error)).toMatch(/MISMATCH|PK/i);
+    expect(err).toBeUndefined();
  });

  it("old key empty, new key present, store new node", async () => {
@@ -451,3 +459,118 @@ describe("NodeDB – merge semantics, PKI checks & extras", () => {
    expect(newDB.getMyNode().num).toBe(4242);
  });
 });
+
+describe("NodeDB deviceContext & debounce", () => {
+  beforeEach(() => {
+    idbMem.clear();
+    vi.clearAllMocks();
+  });
+
+  it("useNodeDB resolves per-device DB and switches with deviceId", async () => {
+    const { useNodeDBStore, useNodeDB } = await freshStore();
+
+    // device 1
+    deviceIdForTests = 1;
+    const st = useNodeDBStore.getState();
+    const db1 = st.addNodeDB(1);
+    db1.addNode({ num: 10 } as any);
+
+    function Comp() {
+      const len = useNodeDB((db) => db.getNodesLength(), {
+        debounce: 0,
+        equality: (a, b) => a === b,
+      });
+      return <div data-testid="len">{len}</div>;
+    }
+
+    const { rerender } = render(<Comp />);
+    expect(screen.getByTestId("len").textContent).toBe("1");
+
+    // switch to device 2 and add nodes
+    deviceIdForTests = 2;
+    const db2 = st.addNodeDB(2);
+    db2.addNode({ num: 20 } as any);
+    db2.addNode({ num: 21 } as any);
+    db2.addNode({ num: 22 } as any);
+
+    // re-render so the hook re-subscribes with the new deviceId
+    await act(async () => {
+      rerender(<Comp />);
+    });
+
+    expect(screen.getByTestId("len").textContent).toBe("3");
+  });
+
+  it("useNodeDB selector re-renders only when the selected slice changes", async () => {
+    const { useNodeDBStore, useNodeDB } = await freshStore();
+    deviceIdForTests = 1;
+
+    const st = useNodeDBStore.getState();
+    const db = st.addNodeDB(1);
+
+    let renders = 0;
+    function Comp() {
+      const len = useNodeDB((d) => d.getNodesLength(), {
+        debounce: 0,
+        equality: (a, b) => a === b,
+      });
+      renders++;
+      return <div data-testid="len">{len}</div>;
+    }
+
+    render(<Comp />);
+    expect(screen.getByTestId("len").textContent).toBe("0");
+    expect(renders).toBe(1);
+
+    // mutate something unrelated to length
+    db.setNodeError(999, "X" as any);
+    await act(() => Promise.resolve());
+    expect(screen.getByTestId("len").textContent).toBe("0");
+    expect(renders).toBe(1); // no re-render
+
+    // now actually change the slice
+    db.addNode({ num: 1 } as any);
+    await act(() => Promise.resolve());
+    expect(screen.getByTestId("len").textContent).toBe("1");
+    expect(renders).toBe(2);
+  });
+
+  it("useNodeDB debounce coalesces rapid updates", async () => {
+    vi.useFakeTimers();
+    const { useNodeDBStore, useNodeDB } = await freshStore();
+    deviceIdForTests = 1;
+
+    const st = useNodeDBStore.getState();
+    const db = st.addNodeDB(1);
+
+    let renders = 0;
+    function Comp() {
+      const len = useNodeDB((d) => d.getNodesLength(), {
+        debounce: 50,
+        equality: (a, b) => a === b,
+      });
+      renders++;
+      return <div data-testid="len">{len}</div>;
+    }
+
+    render(<Comp />);
+
+    // burst of updates within the debounce window
+    db.addNode({ num: 1 } as any);
+    db.addNode({ num: 2 } as any);
+    db.addNode({ num: 3 } as any);
+
+    await act(() => {
+      vi.advanceTimersByTime(49);
+    });
+    expect(renders).toBe(1); // not yet
+
+    await act(() => {
+      vi.advanceTimersByTime(2);
+    });
+    expect(screen.getByTestId("len").textContent).toBe("3");
+    expect(renders).toBe(2); // single coalesced re-render
+
+    vi.useRealTimers();
+  });
+});
@@ -1,5 +1,28 @@
 import type { NodeErrorType } from "@core/stores";
 import type { Protobuf } from "@meshtastic/core";
+import { fromByteArray } from "base64-js";
+
+export function equalKey(
+  a?: Uint8Array | null,
+  b?: Uint8Array | null,
+): boolean {
+  if (!a || !b) {
+    return false;
+  }
+  if (a === b) {
+    return true;
+  }
+  const len = a.byteLength;
+  if (len !== b.byteLength) {
+    return false;
+  }
+  for (let i = 0; i < len; i++) {
+    if (a[i] !== b[i]) {
+      return false;
+    }
+  }
+  return true;
+}

 // Validates a new incoming node against existing nodes.
 // If valid, returns a node to store, else returns undefined.
@@ -26,6 +49,12 @@ export function validateIncomingNode(
      );
      if (nodesWithSameKey.length > 0) {
        // This is a potential impersonation attempt.
+
+        console.warn(
+          `Node ${num} rejected: Public key already claimed by another node. Key:`,
+          fromByteArray(newNode.user?.publicKey ?? new Uint8Array()),
+        );
+
        setNodeError(num, "DUPLICATE_PKI");
        return undefined; // drop newNode entirely
      }
@@ -41,7 +70,7 @@ export function validateIncomingNode(
    // A public key is considered matching if the incoming key equals
    // the existing key, OR if the existing key is empty.
    const isKeyMatchingOrExistingEmpty =
-      oldNode.user?.publicKey === newNode.user?.publicKey ||
+      equalKey(oldNode.user?.publicKey, newNode.user?.publicKey) ||
      oldNode.user?.publicKey === undefined ||
      oldNode.user?.publicKey.length === 0;

@@ -49,14 +78,34 @@ export function validateIncomingNode(
      // Keys match or existing key was empty: trust the incoming node data completely.
      // This allows for legitimate updates to user info and other fields.
      return newNode;
-    } else {
+    } else if (
+      newNode.user?.publicKey !== undefined &&
+      newNode.user?.publicKey.length > 0
+    ) {
+      console.warn(
+        `Node ${num} rejected: existing key does not match incoming key. Old key:`,
+        fromByteArray(oldNode.user?.publicKey ?? new Uint8Array()),
+        "New key:",
+        fromByteArray(newNode.user?.publicKey ?? new Uint8Array()),
+      );
+
      // Keys do not match and existing key was not empty: potential impersonation attempt.
      setNodeError(num, "MISMATCH_PKI");
      return oldNode; // drop newNode fields and return old
+    } else {
+      // Incoming node has no public key: ignore the new node entirely.
+      console.warn(
+        `Node ${num} rejected: incoming node has no public key, but existing does.`,
+      );
+      return oldNode; // drop newNode fields and return old
    }
  } else {
    // Multiple existing nodes with the same node number
    // This should never happen, but if it does, we drop the new node entirely.
+    console.warn(
+      `Node ${num} rejected: Multiple existing nodes with this node number.`,
+    );
+
    setNodeError(num, "DUPLICATE_PKI");
    return undefined; // drop newNode entirely
  }
@@ -0,0 +1,101 @@
+import { useDeviceContext } from "@core/hooks/useDeviceContext";
+import { useCallback, useMemo, useRef, useSyncExternalStore } from "react";
+import type { StoreApi, UseBoundStore } from "zustand";
+import { shallow } from "zustand/shallow";
+
+type GenericEqualityFn<T> = (a: T, b: T) => boolean;
+
+type DebounceOpts<T> = {
+  debounce?: number; // 0/undefined = no debounce
+  equality?: GenericEqualityFn<T>; // default: shallow
+  fireImmediately?: boolean; // default: true
+};
+
+type StoreWithSelector<S> = UseBoundStore<StoreApi<S>> & {
+  getState(): S;
+  subscribe: <U>(
+    selector: (state: S) => U,
+    listener: (next: U, prev: U) => void,
+    options?: { equalityFn?: GenericEqualityFn<U>; fireImmediately?: boolean },
+  ) => () => void;
+};
+
+export function bindStoreToDevice<S, DB>(
+  store: StoreWithSelector<S>,
+  resolveDB: (state: S, deviceId: number) => DB,
+) {
+  // Overloads:
+  function useBound(): DB;
+  function useBound<T>(selector: (db: DB) => T, opts?: DebounceOpts<T>): T;
+
+  // Implementation:
+  function useBound<T>(
+    selector?: (db: DB) => T,
+    opts?: DebounceOpts<T>,
+  ): DB | T {
+    const { deviceId } = useDeviceContext();
+
+    // Build the store-level selector
+    const storeSelector = useCallback(
+      (state: S) => {
+        const db = resolveDB(state, deviceId);
+        return selector ? selector(db) : db;
+      },
+      [deviceId, resolveDB, selector],
+    );
+
+    type Selected = ReturnType<typeof storeSelector>;
+
+    const wait = opts?.debounce ?? 0;
+    const fireImmediately = opts?.fireImmediately ?? true;
+    const equality: GenericEqualityFn<Selected> =
+      (opts?.equality as GenericEqualityFn<Selected>) ??
+      (shallow as unknown as GenericEqualityFn<Selected>);
+
+    const snapRef = useRef<Selected>(storeSelector(store.getState()));
+    snapRef.current = storeSelector(store.getState()); // this ensures rerenders with a new selector (new deviceId) see the right initial value
+
+    const timerRef = useRef<ReturnType<typeof setTimeout> | undefined>(
+      undefined,
+    );
+
+    const subscribe = useMemo(() => {
+      return (onChange: () => void) => {
+        const unsubscribe = store.subscribe(
+          storeSelector,
+          (next: Selected, prev: Selected) => {
+            const emit = () => {
+              snapRef.current = next;
+              onChange();
+            };
+
+            if (equality(next, prev)) {
+              return;
+            }
+            if (wait > 0) {
+              if (timerRef.current) {
+                clearTimeout(timerRef.current);
+              }
+              timerRef.current = setTimeout(emit, wait);
+            } else {
+              emit();
+            }
+          },
+          { equalityFn: equality, fireImmediately },
+        );
+
+        return () => {
+          if (timerRef.current) {
+            clearTimeout(timerRef.current);
+          }
+          unsubscribe();
+        };
+      };
+    }, [store, storeSelector, equality, wait, fireImmediately]);
+
+    const getSnapshot = () => snapRef.current;
+    return useSyncExternalStore(subscribe, getSnapshot, getSnapshot);
+  }
+
+  return useBound;
+}