Resolve TODOs

vbabich · vbabich · commit c66c33c72b6c · 2025-03-26T03:27:12.000-06:00
diff --git a/packages/dashboard-core-plugins/src/panels/IrisGridPanel.tsx b/packages/dashboard-core-plugins/src/panels/IrisGridPanel.tsx
@@ -325,7 +325,6 @@ export class IrisGridPanel extends PureComponent<
     const { model } = this.state;
     const { makeModel } = this.props;
     if (model !== prevState.model) {
-      log.debug('[0] componentDidUpdate model changed');
       if (prevState.model != null) {
         this.stopModelListening(prevState.model);
         prevState.model.close();
diff --git a/packages/iris-grid/src/IrisGridSimplePivotModel.ts b/packages/iris-grid/src/IrisGridSimplePivotModel.ts
@@ -1,4 +1,5 @@
 /* eslint class-methods-use-this: "off" */
+/* eslint no-underscore-dangle: "off" */
 import memoize from 'memoize-one';
 import type { dh as DhType } from '@deephaven/jsapi-types';
 import Log from '@deephaven/log';
@@ -10,7 +11,7 @@ import {
   type CancelablePromise,
 } from '@deephaven/utils';
 import { GridRange, type ModelIndex } from '@deephaven/grid';
-import { type ColumnName, type UITotalsTableConfig } from './CommonTypes';
+import { type ColumnName } from './CommonTypes';
 import type { DisplayColumn } from './IrisGridModel';
 import ColumnHeaderGroup from './ColumnHeaderGroup';
 import IrisGridModel from './IrisGridModel';
@@ -20,8 +21,8 @@ import IrisGridUtils from './IrisGridUtils';
 import type { IrisGridThemeType } from './IrisGridTheme';
 import {
   getSimplePivotColumnMap,
+  isColumnMapComplete,
   KEY_TABLE_PIVOT_COLUMN,
-  PIVOT_COLUMN_PREFIX,
   TOTALS_COLUMN,
   type KeyColumnArray,
   type KeyTableSubscriptionData,
@@ -74,6 +75,8 @@ class IrisGridSimplePivotModel extends IrisGridModel {
 
   private totalsRowMap: Map<string, unknown>;
 
+  private _layoutHints: DhType.LayoutHints | null | undefined;
+
   constructor(
     dh: typeof DhType,
     table: DhType.Table,
@@ -114,6 +117,16 @@ class IrisGridSimplePivotModel extends IrisGridModel {
     this.pivotWidget = pivotWidget;
     this.schema = schema;
 
+    this._layoutHints = {
+      backColumns: [TOTALS_COLUMN],
+      hiddenColumns: [],
+      frozenColumns: [],
+      columnGroups: [],
+      areSavedLayoutsAllowed: false,
+      frontColumns: [],
+      searchDisplayMode: this.dh.SearchDisplayMode.SEARCH_DISPLAY_HIDE,
+    };
+
     this.startListeningToKeyTable();
 
     this.startListeningToSchema();
@@ -165,7 +178,7 @@ class IrisGridSimplePivotModel extends IrisGridModel {
   }
 
   /**
-   * Add displayName property from the column map to the given column
+   * Add displayName property to the given column
    * @param column Column to add displayName to
    * @param columnMap Column name map
    * @returns Column with the displayName
@@ -177,12 +190,7 @@ class IrisGridSimplePivotModel extends IrisGridModel {
     return new Proxy(column, {
       get: (target, prop) => {
         if (prop === 'displayName') {
-          if (!columnMap.has(column.name)) {
-            return column.name.startsWith(PIVOT_COLUMN_PREFIX)
-              ? ''
-              : column.name;
-          }
-          return columnMap.get(column.name);
+          return columnMap.get(column.name) ?? column.name;
         }
         return Reflect.get(target, prop);
       },
@@ -193,27 +201,20 @@ class IrisGridSimplePivotModel extends IrisGridModel {
     (
       columnMap: SimplePivotColumnMap,
       schema: SimplePivotSchema
-    ): readonly ColumnHeaderGroup[] => {
-      log.debug('getPivotColumnHeaderGroups', schema.pivotDescription, {
-        schema,
-        columnMap: JSON.stringify([...columnMap]),
-        modelColumns: JSON.stringify(this.model.columns.map(c => c.name)),
-      });
-      return [
-        new ColumnHeaderGroup({
-          name: schema.pivotDescription,
-          children: schema.rowColNames,
-          depth: 1,
-          childIndexes: schema.rowColNames.map((_, index) => index),
-        }),
-        new ColumnHeaderGroup({
-          name: schema.columnColNames.join(', '),
-          children: [...columnMap.keys()],
-          depth: 1,
-          childIndexes: [...columnMap.keys()].map((_, index) => index),
-        }),
-      ];
-    }
+    ): readonly ColumnHeaderGroup[] => [
+      new ColumnHeaderGroup({
+        name: schema.pivotDescription,
+        children: schema.rowColNames,
+        depth: 1,
+        childIndexes: schema.rowColNames.map((_, index) => index),
+      }),
+      new ColumnHeaderGroup({
+        name: schema.columnColNames.join(', '),
+        children: [...columnMap.keys()],
+        depth: 1,
+        childIndexes: [...columnMap.keys()].map((_, index) => index),
+      }),
+    ]
   );
 
   get initialColumnHeaderGroups(): readonly ColumnHeaderGroup[] {
@@ -275,24 +276,6 @@ class IrisGridSimplePivotModel extends IrisGridModel {
     return false;
   }
 
-  set totalsConfig(_: UITotalsTableConfig | null) {
-    // no-op
-  }
-
-  get columnHeaderGroupMap(): ReadonlyMap<string, ColumnHeaderGroup> {
-    log.debug('get columnHeaderGroupMap');
-    return this.model.columnHeaderGroupMap;
-  }
-
-  get columnHeaderGroups(): readonly ColumnHeaderGroup[] {
-    log.debug('get columnHeaderGroups');
-    return this.model.columnHeaderGroups;
-  }
-
-  set columnHeaderGroups(columnHeaderGroups: readonly ColumnHeaderGroup[]) {
-    this.model.columnHeaderGroups = columnHeaderGroups;
-  }
-
   get rowCount(): number {
     return this.model.rowCount + (this.schema.hasTotals ? 1 : 0);
   }
@@ -379,6 +362,7 @@ class IrisGridSimplePivotModel extends IrisGridModel {
   }
 
   handleKeyTableUpdate(e: { detail: KeyTableSubscriptionData }): void {
+    log.debug('Key table updated');
     const pivotIdColumn = this.keyTable.findColumn(KEY_TABLE_PIVOT_COLUMN);
     const columns = this.keyTable.columns.filter(
       c => c.name !== KEY_TABLE_PIVOT_COLUMN
@@ -392,43 +376,46 @@ class IrisGridSimplePivotModel extends IrisGridModel {
       keyColumns.push([TOTALS_COLUMN, 'Totals']);
     }
     const columnMap = new Map(keyColumns);
-    log.debug(
-      'Key table updated',
-      this.model.columns.map(c => c.name),
-      JSON.stringify([...columnMap]),
-      Boolean(this.nextModel),
-      Boolean(this.nextColumnMap)
-    );
-    if (this.nextModel != null) {
-      if (
-        this.nextModel.columns.some(
-          c => c.name.startsWith(PIVOT_COLUMN_PREFIX) && !columnMap.has(c.name)
-        )
-      ) {
-        log.debug('next model not null, but columns do not match');
-        this.nextColumnMap = columnMap;
+
+    if (this.nextModel == null) {
+      if (isColumnMapComplete(columnMap, this.model.columns)) {
+        log.debug2(
+          'Key table update matches the existing model, update columns'
+        );
+        this.columnMap = columnMap;
+        this.columnHeaderGroups = this.getCachedColumnHeaderGroups(
+          this.columnMap,
+          this.schema
+        );
+        this.dispatchEvent(
+          new EventShimCustomEvent(IrisGridModel.EVENT.COLUMNS_CHANGED, {
+            detail: this.columns,
+          })
+        );
       } else {
-        log.debug('next model not null, all columns match');
-        assertNotNull(this.nextTotalsTable);
-        this.setModel(this.nextModel, columnMap, this.nextTotalsTable);
-        this.nextModel = null;
+        log.debug2(
+          'Key table update does not match the existing model, save column map for the next schema update'
+        );
+        this.nextColumnMap = columnMap;
       }
-    } else if (
-      this.model.columns.some(
-        c => c.name.startsWith(PIVOT_COLUMN_PREFIX) && !columnMap.has(c.name)
-      )
-    ) {
-      log.debug(
-        'next model is null, columns do not match - save nextColumnMap'
+      return;
+    }
+    if (isColumnMapComplete(columnMap, this.nextModel.columns)) {
+      log.debug2('Key table update matches the saved model, update the model');
+      assertNotNull(this.nextTotalsTable);
+      this.setModel(this.nextModel, columnMap, this.nextTotalsTable);
+      this.nextModel = null;
+      this.nextTotalsTable = null;
+    } else {
+      log.debug2(
+        'Key table update does not match the saved model, save column map for the next schema update'
       );
-      // TODO: check if current model columns match?
       this.nextColumnMap = columnMap;
     }
   }
 
   async handleSchemaUpdate(e: DhType.Event<DhType.Widget>): Promise<void> {
     log.debug('Schema updated');
-    // Get the object, and make sure the keytable is fetched and usable
     const tables = e.detail.exportedObjects;
     const tablePromise = tables[0].fetch();
     const totalsTablePromise = tables.length === 2 ? tables[1].fetch() : null;
@@ -456,17 +443,7 @@ class IrisGridSimplePivotModel extends IrisGridModel {
   );
 
   get layoutHints(): DhType.LayoutHints | null | undefined {
-    // log.debug('get layoutHints');
-    // TODO: memoize
-    return {
-      backColumns: [TOTALS_COLUMN],
-      hiddenColumns: [],
-      frozenColumns: [],
-      columnGroups: [],
-      areSavedLayoutsAllowed: false,
-      frontColumns: [],
-      searchDisplayMode: this.dh.SearchDisplayMode.SEARCH_DISPLAY_HIDE,
-    };
+    return this._layoutHints;
   }
 
   /**
@@ -544,16 +521,14 @@ class IrisGridSimplePivotModel extends IrisGridModel {
     }
     this.dispatchEvent(
       new EventShimCustomEvent(IrisGridModel.EVENT.COLUMNS_CHANGED, {
-        detail: model.columns,
+        detail: this.columns,
       })
     );
   }
 
   setNextSchema(
     pivotTablesPromise: Promise<[DhType.Table, DhType.Table]>
   ): void {
-    log.debug2('setNextSchema');
-
     if (this.schemaPromise) {
       this.schemaPromise.cancel();
     }
@@ -567,17 +542,31 @@ class IrisGridSimplePivotModel extends IrisGridModel {
     );
     this.schemaPromise
       .then(([table, totalsTable]) => {
+        log.debug('Schema updated');
         this.schemaPromise = null;
         const model = makeModel(this.dh, table, this.formatter);
         if (this.nextColumnMap != null) {
-          log.debug(
-            'Schema updated, nextColumnMap is not null, setting new model'
-          );
-          this.setModel(model, this.nextColumnMap, totalsTable);
-          this.nextColumnMap = null;
+          if (isColumnMapComplete(this.nextColumnMap, model.columns)) {
+            log.debug2(
+              'Schema updated, set new model with the saved column map'
+            );
+            this.setModel(model, this.nextColumnMap, totalsTable);
+            this.nextColumnMap = null;
+          } else {
+            log.debug2(
+              'Saved column map does not match the new model, save the model for the next key table update'
+            );
+            this.nextModel = model;
+            this.nextTotalsTable = totalsTable;
+          }
+          return;
+        }
+        if (isColumnMapComplete(this.columnMap, model.columns)) {
+          log.debug2('Schema updated, set new model with existing column map');
+          this.setModel(model, this.columnMap, totalsTable);
         } else {
-          log.debug(
-            'Schema updated, nextColumnMap is null, save new model as nextModel'
+          log.debug2(
+            'Existing column map does not match the new model, save the model for the next key table update'
           );
           this.nextModel = model;
           this.nextTotalsTable = totalsTable;
diff --git a/packages/iris-grid/src/SimplePivotUtils.test.ts b/packages/iris-grid/src/SimplePivotUtils.test.ts
@@ -0,0 +1,48 @@
+import type { dh as DhType } from '@deephaven/jsapi-types';
+import {
+  PIVOT_COLUMN_PREFIX,
+  isColumnMapComplete,
+  type SimplePivotColumnMap,
+} from './SimplePivotUtils';
+
+describe('isColumnMapComplete', () => {
+  it('returns true if all PIVOT_C_ columns exist in the map', () => {
+    const mockColumns = [
+      { name: `${PIVOT_COLUMN_PREFIX}A` } as DhType.Column,
+      { name: `${PIVOT_COLUMN_PREFIX}B` } as DhType.Column,
+    ];
+    const mockColumnMap: SimplePivotColumnMap = new Map([
+      [`${PIVOT_COLUMN_PREFIX}A`, 'ValA'],
+      [`${PIVOT_COLUMN_PREFIX}B`, 'ValB'],
+    ]);
+
+    const result = isColumnMapComplete(mockColumnMap, mockColumns);
+    expect(result).toBe(true);
+  });
+
+  it('returns false if a pivot column is missing in the map', () => {
+    const mockColumns = [
+      { name: `${PIVOT_COLUMN_PREFIX}A` } as DhType.Column,
+      { name: `${PIVOT_COLUMN_PREFIX}B` } as DhType.Column,
+    ];
+    const mockColumnMap: SimplePivotColumnMap = new Map([
+      [`${PIVOT_COLUMN_PREFIX}A`, 'ValA'],
+    ]);
+
+    const result = isColumnMapComplete(mockColumnMap, mockColumns);
+    expect(result).toBe(false);
+  });
+
+  it('ignores non-pivot columns when checking completeness', () => {
+    const mockColumns = [
+      { name: 'RandomColumn' } as DhType.Column,
+      { name: `${PIVOT_COLUMN_PREFIX}A` } as DhType.Column,
+    ];
+    const mockColumnMap: SimplePivotColumnMap = new Map([
+      [`${PIVOT_COLUMN_PREFIX}A`, 'ValA'],
+    ]);
+
+    const result = isColumnMapComplete(mockColumnMap, mockColumns);
+    expect(result).toBe(true);
+  });
+});
diff --git a/packages/iris-grid/src/SimplePivotUtils.ts b/packages/iris-grid/src/SimplePivotUtils.ts
@@ -22,6 +22,13 @@ export interface KeyTableSubscriptionData {
   getData: (rowKey: DhType.Row, column: DhType.Column) => string;
 }
 
+/**
+ * Get a column map for a simple pivot table based on the key table data
+ * @param data Data from the key table
+ * @param columns Columns to include in the column map
+ * @param pivotIdColumn Key table column containing display names for the pivot columns
+ * @returns Column map for the simple pivot table
+ */
 export function getSimplePivotColumnMap(
   data: KeyTableSubscriptionData,
   columns: DhType.Column[],
@@ -42,3 +49,18 @@ export function getSimplePivotColumnMap(
   }
   return columnMap;
 }
+
+/**
+ * Check if the column map has entries for all pivot columns
+ * @param columnMap Column map to check
+ * @param columns Columns to check against
+ * @returns True if the column map has entries for all pivot columns
+ */
+export function isColumnMapComplete(
+  columnMap: SimplePivotColumnMap,
+  columns: readonly DhType.Column[]
+): boolean {
+  return !columns.some(
+    c => c.name.startsWith(PIVOT_COLUMN_PREFIX) && !columnMap.has(c.name)
+  );
+}
