fix: row expansion when a row is already selected
Description
Bug: Unable to expand table row when a row has already been selected
- In table component,
toggleRowExpanded(), we do a mutation on the toggled row:row.$$state.expanded = !row.$$state.expanded; - When we do this before a table row selection has occurred,
rowintoggledRowExpanded()does not share the same memory address as any of thethis.dataSource.cachedData.rows(in the dataSource, cdk-table-data-source) - 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. - The reason that point 3. matters, is because
toggleRowExpanded()triggersbuildDataObservable() -> fetchAndAppendNewChildren()to run, which is responsible for getting all the rows + new child rows we want expanded. However it fails to append new children becauseisNewlyExpandedParentRowfails the check, this check fails becausethis.cachedData.rows[n].expandedhas been modified, because of point 1/3. - 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.
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
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.