hypertrace-ui icon indicating copy to clipboard operation
hypertrace-ui copied to clipboard

fix: row expansion when a row is already selected

Open Christian862 opened this issue 3 years ago • 2 comments

Description

Bug: Unable to expand table row when a row has already been selected

  1. In table component, toggleRowExpanded() , we do a mutation on the toggled row: row.$$state.expanded = !row.$$state.expanded;
  2. When we do this before a table row selection has occurred, row in toggledRowExpanded() does not share the same memory address as any of the this.dataSource.cachedData.rows (in the dataSource, cdk-table-data-source)
  3. However when we do this after a selection has occurred, the nth row does share the same memory address as this.dataSource.cachedData.rows[n] which means when the mutation in point 1. happens, we are modifying the cached row.
  4. The reason that point 3. matters, is because toggleRowExpanded() triggers buildDataObservable() -> fetchAndAppendNewChildren() to run, which is responsible for getting all the rows + new child rows we want expanded. However it fails to append new children because isNewlyExpandedParentRow fails the check, this check fails because this.cachedData.rows[n].expanded has been modified, because of point 1/3.
  5. The reason row selection causes the table component stateful table rows to reference the same objects as this.cachedData.rows:
  • row selection triggers this.selectionsChange in toggleRowSelected() which eventually ends up calling toggleRowSelections() (from CD)
  • which in turn calls the data source methods unselectAllRows() and selectAllRows() <- these methods are the culprit for everything. They read this.cachedData.rows and without creating new objects, they modify, and emit these rows to rowsChange$
  • this triggers the whole this.changeSubscription cycle to emit in connect() and the rows(same ones referencing the cache) are passed on to be used in the component

Changes in this PR:

Removes the row mutation made in toggleRowExpanded

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

  • [ ] My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

Christian862 avatar Nov 25 '22 16:11 Christian862

Codecov Report

Merging #1930 (c2d03bd) into main (b0ab5a7) will not change coverage. The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #1930   +/-   ##
=======================================
  Coverage   83.22%   83.22%           
=======================================
  Files         890      890           
  Lines       19555    19555           
  Branches     2772     2772           
=======================================
  Hits        16275    16275           
  Misses       3131     3131           
  Partials      149      149           
Impacted Files Coverage Δ
projects/components/src/table/table.component.ts 84.64% <0.00%> (ø)
...ts/assets-library/src/icons/icon-library.module.ts 100.00% <0.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Nov 25 '22 16:11 codecov[bot]

Unit Test Results

       4 files  ±0     300 suites  ±0   24m 5s :stopwatch: +21s 1 073 tests ±0  1 073 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  1 081 runs  ±0  1 081 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit c2d03bd4. ± Comparison against base commit 48db3a1f.

github-actions[bot] avatar Nov 25 '22 16:11 github-actions[bot]