Optimize useSheetValue (#3879)

* Optimize useSheetValue

* Fix lint

* Fix mock bind

* Reduce re-renders

* Update useSheetValue

* Update

* Make QueryState immutable

* Release notes

packages/desktop-client/src/components/spreadsheet/useSheetValue.ts

@@ -1,4 +1,4 @@
-import { useState, useRef, useLayoutEffect } from 'react';
+import { useState, useRef, useLayoutEffect, useMemo } from 'react';
 
 import { type Query } from 'loot-core/shared/query';
 import { useSpreadsheet } from 'loot-core/src/client/SpreadsheetProvider';
@@ -30,15 +30,18 @@ export function useSheetValue<
 ): SheetValueResult<SheetName, FieldName>['value'] {
   const { sheetName, fullSheetName } = useSheetName(binding);
 
-  const bindingObj =
-    typeof binding === 'string'
-      ? { name: binding, value: null, query: undefined }
-      : binding;
+  const bindingObj = useMemo(
+    () =>
+      typeof binding === 'string'
+        ? { name: binding, value: null, query: undefined }
+        : binding,
+    [binding],
+  );
 
   const spreadsheet = useSpreadsheet();
   const [result, setResult] = useState<SheetValueResult<SheetName, FieldName>>({
     name: fullSheetName,
-    value: bindingObj.value === undefined ? null : bindingObj.value,
+    value: bindingObj.value ? bindingObj.value : null,
     query: bindingObj.query,
   });
   const latestOnChange = useRef(onChange);
@@ -48,15 +51,16 @@ export function useSheetValue<
   latestValue.current = result.value;
 
   useLayoutEffect(() => {
-    if (bindingObj.query) {
-      spreadsheet.createQuery(sheetName, bindingObj.name, bindingObj.query);
-    }
+    let isMounted = true;
 
-    return spreadsheet.bind(
+    const unbind = spreadsheet.bind(
       sheetName,
-      binding,
-      null,
+      bindingObj,
       (newResult: SheetValueResult<SheetName, FieldName>) => {
+        if (!isMounted) {
+          return;
+        }
+
         if (latestOnChange.current) {
           latestOnChange.current(newResult);
         }
@@ -66,7 +70,17 @@ export function useSheetValue<
         }
       },
     );
-  }, [sheetName, bindingObj.name, JSON.stringify(bindingObj.query)]);
+
+    return () => {
+      isMounted = false;
+      unbind();
+    };
+  }, [
+    spreadsheet,
+    sheetName,
+    bindingObj.name,
+    bindingObj.query?.serializeAsString(),
+  ]);
 
   return result.value;
 }

packages/loot-core/src/client/SpreadsheetProvider.tsx

@@ -62,22 +62,26 @@ function makeSpreadsheet() {
       });
     }
 
-    bind(sheetName = '__global', binding, fields, cb) {
+    bind(sheetName = '__global', binding, callback) {
       binding =
         typeof binding === 'string' ? { name: binding, value: null } : binding;
 
+      if (binding.query) {
+        this.createQuery(sheetName, binding.name, binding.query);
+      }
+
       const resolvedName = `${sheetName}!${binding.name}`;
-      const cleanup = this.observeCell(resolvedName, cb);
+      const cleanup = this.observeCell(resolvedName, callback);
 
       // Always synchronously call with the existing value if it has one.
       // This is a display optimization to avoid flicker. The LRU cache
       // will keep a number of recent nodes in memory.
       if (LRUValueCache.has(resolvedName)) {
-        cb(LRUValueCache.get(resolvedName));
+        callback(LRUValueCache.get(resolvedName));
       }
 
       if (cellCache[resolvedName] != null) {
-        cellCache[resolvedName].then(cb);
+        cellCache[resolvedName].then(callback);
       } else {
         const req = this.get(sheetName, binding.name);
         cellCache[resolvedName] = req;
@@ -90,7 +94,7 @@ function makeSpreadsheet() {
           // with an old value depending on the order of messages)
           if (cellCache[resolvedName] === req) {
             LRUValueCache.set(resolvedName, result);
-            cb(result);
+            callback(result);
           }
         });
       }

packages/loot-core/src/mocks/spreadsheet.ts

@@ -23,7 +23,7 @@ export function makeSpreadsheet() {
       this._getNode(sheetName, name).value = value;
     },
 
-    bind(sheetName, binding, fields, cb) {
+    bind(sheetName, binding, cb) {
       const { name } = binding;
       const resolvedName = `${sheetName}!${name}`;
       if (!this.observers[resolvedName]) {

packages/loot-core/src/server/main.ts

@@ -516,22 +516,8 @@ handlers['make-filters-from-conditions'] = async function ({ conditions }) {
 };
 
 handlers['getCell'] = async function ({ sheetName, name }) {
-  // Fields is no longer used - hardcode
-  const fields = ['name', 'value'];
   const node = sheet.get()._getNode(resolveName(sheetName, name));
-  if (fields) {
-    const res = {};
-    fields.forEach(field => {
-      if (field === 'run') {
-        res[field] = node._run ? node._run.toString() : null;
-      } else {
-        res[field] = node[field];
-      }
-    });
-    return res;
-  } else {
-    return node;
-  }
+  return { name: node.name, value: node.value };
 };
 
 handlers['getCells'] = async function ({ names }) {
@@ -1111,7 +1097,7 @@ handlers['accounts-bank-sync'] = async function ({ ids = [] }) {
 
   const accounts = await db.runQuery(
     `
-    SELECT a.*, b.bank_id as bankId 
+    SELECT a.*, b.bank_id as bankId
     FROM accounts a
     LEFT JOIN banks b ON a.bank = b.id
     WHERE a.tombstone = 0 AND a.closed = 0

packages/loot-core/src/shared/query.ts

@@ -5,18 +5,18 @@ type ObjectExpression = {
 };
 
 export type QueryState = {
-  table: string;
-  tableOptions: Record<string, unknown>;
-  filterExpressions: Array<ObjectExpression>;
-  selectExpressions: Array<ObjectExpression | string | '*'>;
-  groupExpressions: Array<ObjectExpression | string>;
-  orderExpressions: Array<ObjectExpression | string>;
-  calculation: boolean;
-  rawMode: boolean;
-  withDead: boolean;
-  validateRefs: boolean;
-  limit: number | null;
-  offset: number | null;
+  get table(): string;
+  get tableOptions(): Readonly<Record<string, unknown>>;
+  get filterExpressions(): ReadonlyArray<ObjectExpression>;
+  get selectExpressions(): ReadonlyArray<ObjectExpression | string | '*'>;
+  get groupExpressions(): ReadonlyArray<ObjectExpression | string>;
+  get orderExpressions(): ReadonlyArray<ObjectExpression | string>;
+  get calculation(): boolean;
+  get rawMode(): boolean;
+  get withDead(): boolean;
+  get validateRefs(): boolean;
+  get limit(): number | null;
+  get offset(): number | null;
 };
 
 export class Query {
@@ -76,15 +76,19 @@ export class Query {
       exprs = [exprs];
     }
 
-    const query = new Query({ ...this.state, selectExpressions: exprs });
-    query.state.calculation = false;
-    return query;
+    return new Query({
+      ...this.state,
+      selectExpressions: exprs,
+      calculation: false,
+    });
   }
 
   calculate(expr: ObjectExpression | string) {
-    const query = this.select({ result: expr });
-    query.state.calculation = true;
-    return query;
+    return new Query({
+      ...this.state,
+      selectExpressions: [{ result: expr }],
+      calculation: true,
+    });
   }
 
   groupBy(exprs: ObjectExpression | string | Array<ObjectExpression | string>) {
@@ -140,6 +144,10 @@ export class Query {
   serialize() {
     return this.state;
   }
+
+  serializeAsString() {
+    return JSON.stringify(this.serialize());
+  }
 }
 
 export function getPrimaryOrderBy(

upcoming-release-notes/3879.md

@@ -0,0 +1,6 @@
+---
+category: Maintenance
+authors: [joel-jeremy]
+---
+
+Optimize useSheetValue hook