failing test for selection after rows update (#596)

This is a failing test for an issue we ran into around selecting items and then the table row content changing.

For our specific case, changing the sort triggers a network request, which replaces the rows in the table. If a user had selected some items and then changed the sort, they could no longer select more items.

However, the sort changing does not appear to be core to the issue. Just changing the rows is enough to reveal the bug.

I plan to dig in and see if I can find a fix, but any guidance would be appreciated 😄.

Author: kturney

addon/-private/collapse-tree.js

@@ -16,8 +16,16 @@ export const SELECT_MODE = {
   MULTIPLE: 'multiple',
 };
 
-class TableRowMeta extends EmberObject {
+export class TableRowMeta extends EmberObject {
   _rowValue = null;
+
+  /**
+    The map that contains cell meta information for this row. Is meant to be
+    unique to this row, which is why it is created here. In order to prevent
+    memory leaks, we need to be able to clean the cache manually when the row
+    is destroyed or updated, which is why we use a Map instead of WeakMap
+  */
+  _cellMetaCache = new Map();
   _isCollapsed = false;
 
   @computed('_rowValue.isCollapsed')
@@ -222,6 +230,12 @@ class TableRowMeta extends EmberObject {
 
     tree._lastSelectedIndex = rowIndex;
   }
+
+  destroy() {
+    super.destroy();
+
+    this._cellMetaCache.clear();
+  }
 }
 
 function reduceSelectedRows(selection, groupingCounts, rowMetaCache) {

addon/components/-private/row-wrapper.js

@@ -12,26 +12,20 @@ import { computed } from '@ember-decorators/object';
 import { objectAt } from '../../-private/utils/array';
 import { dynamicAlias } from '../../-private/utils/computed';
 
-import { guidFor } from '@ember/object/internals';
-
 class CellWrapper extends EmberObject {
   @dynamicAlias('rowValue', 'columnValue.valuePath')
   cellValue;
 
-  @computed('rowValue', 'columnValue')
+  @computed('rowMeta', 'columnValue')
   get cellMeta() {
-    let row = get(this, 'rowValue');
-    let rowId = guidFor(row);
-    let columnId = guidFor(get(this, 'columnValue'));
-    let cellMetaCache = get(this, 'cellMetaCache');
-
-    let cellId = `${rowId}:${columnId}`;
+    let rowMeta = get(this, 'rowMeta');
+    let columnValue = get(this, 'columnValue');
 
-    if (!cellMetaCache.has(cellId)) {
-      cellMetaCache.set(cellId, EmberObject.create());
+    if (!rowMeta._cellMetaCache.has(columnValue)) {
+      rowMeta._cellMetaCache.set(columnValue, EmberObject.create());
     }
 
-    return cellMetaCache.get(cellId);
+    return rowMeta._cellMetaCache.get(columnValue);
   }
 }
 
@@ -44,7 +38,6 @@ export default class RowWrapper extends Component {
   @argument rowValue;
   @argument columns;
 
-  @argument cellMetaCache;
   @argument columnMetaCache;
   @argument rowMetaCache;
 
@@ -88,8 +81,6 @@ export default class RowWrapper extends Component {
     'rowSelectionMode'
   )
   get cells() {
-    let cellMetaCache = this.get('cellMetaCache');
-
     let columns = this.get('columns');
     let numColumns = get(columns, 'length');
 
@@ -103,16 +94,11 @@ export default class RowWrapper extends Component {
 
     if (numColumns !== _cells.length) {
       while (_cells.length < numColumns) {
-        let cell = CellWrapper.create({
-          cellMetaCache,
-        });
-
-        _cells.pushObject(cell);
+        _cells.pushObject(CellWrapper.create());
       }
 
       while (_cells.length > numColumns) {
-        let cell = _cells.popObject();
-        cell.destroy();
+        _cells.popObject().destroy();
       }
     }
 

addon/components/ember-tbody/component.js

@@ -1,4 +1,5 @@
 import Component from '@ember/component';
+import { isArray } from '@ember/array';
 
 import { tagName } from '@ember-decorators/component';
 import { computed } from '@ember-decorators/object';
@@ -192,14 +193,6 @@ export default class EmberTBody extends Component {
   @type('string')
   key = '@identity';
 
-  /**
-    The map that contains cell meta information for this table. Is meant to be
-    unique to this table, which is why it is created here. In order to prevent
-    memory leaks, we need to be able to clean the cache manually when the table
-    is destroyed or updated, which is why we use a Map instead of WeakMap
-  */
-  cellMetaCache = new Map();
-
   /**
     The map that contains row meta information for this table.
   */
@@ -212,7 +205,6 @@ export default class EmberTBody extends Component {
   */
   collapseTree = CollapseTree.create({
     sendAction: this.sendAction.bind(this),
-    rowMetaCache: this.rowMetaCache,
   });
 
   /**
@@ -269,10 +261,11 @@ export default class EmberTBody extends Component {
   */
   @computed('rows')
   get wrappedRows() {
-    this._cleanCaches();
-
     let rows = this.get('rows');
 
+    this._cleanCaches(rows);
+
+    this.collapseTree.set('rowMetaCache', this.rowMetaCache);
     this.collapseTree.set('rows', rows);
 
     return this.collapseTree;
@@ -282,12 +275,24 @@ export default class EmberTBody extends Component {
     Helper method that is used to clear the row and cell caches whenever the
     underlying data has changed.
   */
-  _cleanCaches() {
-    this.cellMetaCache.clear();
+  _cleanCaches(newRows) {
+    let newRowMetaCache = new Map();
+    let meta;
+
+    if (isArray(newRows)) {
+      newRows.forEach(row => {
+        if ((meta = this.rowMetaCache.get(row))) {
+          newRowMetaCache.set(row, meta);
+          this.rowMetaCache.delete(row);
+        }
+      });
+    }
 
     for (let [row, meta] of this.rowMetaCache.entries()) {
       meta.destroy();
       this.rowMetaCache.delete(row);
     }
+
+    this.set('rowMetaCache', newRowMetaCache);
   }
 }

addon/components/ember-tbody/template.hbs

@@ -18,7 +18,6 @@
     rowValue=rowValue
     columns=columns
 
-    cellMetaCache=cellMetaCache
     columnMetaCache=columnMetaCache
     rowMetaCache=rowMetaCache
 

addon/components/ember-tfoot/template.hbs

@@ -3,7 +3,6 @@
     rowValue=rowValue
     columns=columns
 
-    cellMetaCache=cellMetaCache
     columnMetaCache=columnMetaCache
     rowMetaCache=rowMetaCache
 

tests/integration/components/selection-test.js

@@ -169,6 +169,36 @@ module('Integration | selection', () => {
 
         assert.ok(table.validateSelected(0, 1, 2, 3), 'row and its children are selected');
       });
+
+      test('selection continues to work after rows are updated', async function(assert) {
+        let columns = [
+          {
+            name: 'Name',
+            valuePath: 'name',
+          },
+          {
+            name: 'Age',
+            valuePath: 'age',
+          },
+        ];
+
+        let rows = [{ name: 'Zoe', age: 34 }, { name: 'Alex', age: 43 }, { name: 'Liz', age: 25 }];
+
+        await generateTable(this, { columns, rows });
+
+        assert.ok(table.validateSelected(), 'the row is not marked as selected on initialization');
+
+        await table.selectRow(0);
+
+        assert.ok(table.validateSelected(0), 'Zoe is selected after being clicked');
+
+        this.set('rows', rows.slice());
+
+        assert.ok(table.validateSelected(0), 'Zoe is still selected rows update');
+
+        await table.selectRow(1);
+        assert.ok(table.validateSelected(1), 'Liz is selected after being clicked');
+      });
     });
 
     componentModule('single', function() {