Table: Multi-select bug which removes selected rows when model updates (HDS-4273)

packages/components/src/components/hds/table/index.ts

@@ -105,7 +105,7 @@ export default class HdsTable extends Component<HdsTableSignature> {
   get getSortCriteria(): string | HdsTableSortingFunction<unknown> {
     // get the current column
     const currentColumn = this.args?.columns?.find(
-      (column) => column.key === this.sortBy
+      (column): boolean => column.key === this.sortBy
     );
     if (
       // check if there is a custom sorting function associated with the current `sortBy` column (we assume the column has `isSortable`)
@@ -124,6 +124,9 @@ export default class HdsTable extends Component<HdsTableSignature> {
     if (this.args.identityKey === 'none') {
       return undefined;
     } else {
+      // We return `@identity` as default to cause an update operation instead of replacement when the model changes to avoid unexpected side effects
+      // see: "Specifying Keys" here: https://api.emberjs.com/ember/6.1/classes/Ember.Templates.helpers/methods/each?anchor=each
+      // see also: https://github.com/hashicorp/design-system/pull/1160 + https://github.com/hashicorp/cloud-ui/pull/5178 + https://hashicorp.atlassian.net/browse/HDS-4273
       return this.args.identityKey ?? '@identity';
     }
   }
@@ -289,17 +292,33 @@ export default class HdsTable extends Component<HdsTableSignature> {
     checkbox: HdsFormCheckboxBaseSignature['Element'],
     selectionKey?: string
   ): void {
+    // TEST:
+    console.log(
+      `[${this.args.caption}] Did insert row checkbox`,
+      selectionKey,
+      this._selectableRows.map((row): string => row.selectionKey)
+    );
+
     if (selectionKey) {
+      // Remove any existing item with the same `selectionKey` identifier (this may occur if the model is updated and the rows re-rendered)
+      this._selectableRows = this._selectableRows.filter(
+        (row): boolean => row.selectionKey !== selectionKey
+      );
+      // Add the new item
       this._selectableRows.push({ selectionKey, checkbox });
     }
     this.setSelectAllState();
   }
 
   @action
   willDestroyRowCheckbox(selectionKey?: string): void {
-    this._selectableRows = this._selectableRows.filter(
-      (row) => row.selectionKey !== selectionKey
+    // TEST:
+    console.log(
+      `[${this.args.caption}] Will destroy row checkbox`,
+      selectionKey,
+      this._selectableRows.map((row): string => row.selectionKey)
     );
+
     this.setSelectAllState();
   }
 
@@ -308,7 +327,7 @@ export default class HdsTable extends Component<HdsTableSignature> {
     if (this._selectAllCheckbox) {
       const selectableRowsCount = this._selectableRows.length;
       const selectedRowsCount = this._selectableRows.filter(
-        (row) => row.checkbox.checked
+        (row): boolean => row.checkbox.checked
       ).length;
 
       this._selectAllCheckbox.checked =

showcase/app/controllers/components/table-multi-select-bug.js

@@ -0,0 +1,49 @@
+/**
+ * Copyright (c) HashiCorp, Inc.
+ * SPDX-License-Identifier: MPL-2.0
+ */
+
+import Controller from '@ember/controller';
+import { tracked } from '@glimmer/tracking';
+import { action } from '@ember/object';
+
+// prettier-ignore
+const daysOfChristmas = [
+  { key: 'one', quantity: 1, type: 'bird', line: 'A partridge in a pear tree' },
+  { key: 'two', quantity: 2, type: 'bird', line: 'Two turtle doves' },
+  { key: 'three', quantity: 3, type: 'bird', line: 'Three french hens' },
+  { key: 'four', quantity: 4, type: 'bird', line: 'Four calling birds' },
+  { key: 'five', quantity: 5, type: 'jewelry', line: 'Five gold rings' },
+  { key: 'six', quantity: 6, type: 'more bird', line: 'Six geese a-laying' },
+  { key: 'seven', quantity: 7, type: 'bird again', line: 'Seven swans a-swimming' },
+  { key: 'eight', quantity: 8, type: 'servant', line: 'Eight maids a-milking' },
+  { key: 'nine', quantity: 9, type: 'maybe royalty', line: 'Nine ladies dancing' },
+  { key: 'ten', quantity: 10, type: 'definitely royalty', line: 'Ten lords a-leaping' },
+  { key: 'eleven', quantity: 11, type: 'musician', line: 'Eleven pipers piping' },
+  { key: 'twelve', quantity: 12, type: 'musician, technically', line: 'Twelve drummers drumming' },
+];
+
+export default class extends Controller {
+  @tracked showTable = false;
+  @tracked lyrics = daysOfChristmas.slice();
+
+  @action dirtyModel() {
+    this.lyrics = this.lyrics
+      // Deep copy to destroy references
+      .map((r) => ({ ...r }))
+      // Randomly sort to see what happens in this case
+      .sort(() => Math.random() - 0.5);
+  }
+
+  @action dirtyModelWithLessRows() {
+    this.lyrics = this.lyrics
+      // Deep copy to destroy references
+      .map((r) => ({ ...r }))
+      // Randomly sort to see what happens in this case
+      .sort(() => Math.random() - 0.5)
+      .filter((row) => row.key !== 'five');
+  }
+
+  printSelection = (selection) =>
+    console.log('Printing selection:', selection.selectedRowsKeys);
+}

showcase/app/router.ts

@@ -63,6 +63,7 @@ Router.map(function () {
     this.route('side-nav');
     this.route('stepper');
     this.route('table');
+    this.route('table-multi-select-bug');
     this.route('tag');
     this.route('text');
     this.route('time');

showcase/app/templates/components/table-multi-select-bug.hbs

@@ -0,0 +1,76 @@
+{{!
+  Copyright (c) HashiCorp, Inc.
+  SPDX-License-Identifier: MPL-2.0
+}}
+
+{{page-title "Table multi-select bug"}}
+
+<Shw::Text::H1>Table multi-select bug</Shw::Text::H1>
+
+<section>
+  <p>When a table isn't immediately rendered, a bug occurs where table rows render twice. Normally this is not a big
+    deal, but due to the way that row-level checkboxes are registered, we get this unfortunate sequence of events:</p>
+  <ol>
+    <li><code>didInsertRowCheckbox</code>: Insert selection keys to <code>_selectableRows</code></li>
+    <li><code>didInsertRowCheckbox</code>: Insert selection keys to
+      <code>_selectableRows</code>
+      a second time. This results in an array that's 2x of every selection key.</li>
+    <li><code>willDestroyRowCheckbox</code>: Filters
+      <code>_selectableRows</code>
+      by removed selection key, except this removes the first and second insertions resulting in an empty array.</li>
+  </ol>
+
+  <Shw::Text::H2>This will work until you click the "Force new data" button</Shw::Text::H2>
+
+  <p>
+    <Hds::Button @text="Force new data" {{on "click" this.dirtyModel}} @isInline={{true}} />
+    <Hds::Button @text="Force new data with fewer rows" {{on "click" this.dirtyModelWithLessRows}} @isInline={{true}} />
+  </p>
+
+  <Hds::Table
+    @model={{this.lyrics}}
+    @caption="local property"
+    @columns={{array
+      (hash key="quantity" label="Quantity")
+      (hash key="type" label="Type")
+      (hash key="line" label="Line")
+    }}
+    @isSelectable={{true}}
+    @onSelectionChange={{this.printSelection}}
+  >
+    <:body as |B|>
+      <B.Tr @selectionKey={{B.data.key}} @selectionAriaLabelSuffix=" {{B.data.key}}">
+        <B.Td>{{B.data.quantity}}</B.Td>
+        <B.Td>{{B.data.type}}</B.Td>
+        <B.Td>{{B.data.line}}</B.Td>
+      </B.Tr>
+    </:body>
+  </Hds::Table>
+
+  <Shw::Text::H2>This will always work because
+    <code>@identityKey</code>
+    performs an update not a replace.</Shw::Text::H2>
+
+  <Hds::Button @text="Force new data" {{on "click" this.dirtyModel}} />
+
+  <Hds::Table
+    @model={{this.lyrics}}
+    @caption="local property"
+    @columns={{array
+      (hash key="quantity" label="Quantity")
+      (hash key="type" label="Type")
+      (hash key="line" label="Line")
+    }}
+    @identityKey="key"
+    @isSelectable={{true}}
+    @onSelectionChange={{this.printSelection}}
+  >
+    <:body as |B|>
+      <B.Tr @selectionKey={{B.data.key}} @selectionAriaLabelSuffix=" {{B.data.key}}">
+        <B.Td>{{B.data.quantity}}</B.Td>
+        <B.Td>{{B.data.type}}</B.Td>
+        <B.Td>{{B.data.line}}</B.Td>
+      </B.Tr>
+    </:body>
+  </Hds::Table>
+</section>