[8.15] [Unified Data Table] Prevent undefined errors when row accessed via rows[rowIndex] (#193791)#193921
Merged
davismcphee merged 1 commit intoelastic:8.15from Sep 25, 2024
Conversation
… `rows[rowIndex]` (elastic#193791) ## Summary This PR fixes an issue present in 8.15, but which no longer exists after later refactoring, where saved search panels (possibly only ES|QL panels? It was hard to nail down since involved a race condition) could fail in dashboards when adding Unified Search filters that reduce the number of results in the grid. I'm still not 100% sure what the source of the issue was since it involved a race condition (didn't fail consistently for me locally) and internal EUI data grid code, but I have a hunch. The `undefined` errors occurred when trying to access a row by index from `DataTableContext` in certain cell renderers during the first render after search results had changed. I believe what was happening was the change to `DataTableContext` triggered a re-render of the cells before EUI data grid internally updated its state, resulting in attempts to access rows from within the cell renderers that no longer existed in the updated `DataTableContext`, and causing `undefined` reference errors. I'm making these changes in `main` instead of `8.15` directly because the updated approach is generally a safer way to retrieve rows and prevent similar issues in the future. Previously we added `rows` to `DataTableContext` and retrieved a single row by index using bracket notation, which can potentially return `undefined`, but TypeScript does not protect against it for us. Instead I've updated `DataTableContext` with a `getRowByIndex` method that correctly returns `DataTableRecord | undefined` and forces consumers to explicitly handle the `undefined` scenario. The PR will require some manual backporting to 8.15 since some things have changed since then, but it shouldn't be difficult. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 6ff0791) # Conflicts: # packages/kbn-unified-data-table/__mocks__/table_context.ts # packages/kbn-unified-data-table/src/components/custom_control_columns/additional_row_control/row_control_column.test.tsx # packages/kbn-unified-data-table/src/components/custom_control_columns/additional_row_control/row_control_column.tsx # packages/kbn-unified-data-table/src/components/custom_control_columns/additional_row_control/row_menu_control_column.test.tsx # packages/kbn-unified-data-table/src/components/custom_control_columns/additional_row_control/row_menu_control_column.tsx # packages/kbn-unified-data-table/src/components/custom_control_columns/color_indicator/color_indicator_control_column.test.tsx # packages/kbn-unified-data-table/src/components/data_table.tsx # packages/kbn-unified-data-table/src/components/data_table_columns.tsx # packages/kbn-unified-data-table/src/components/data_table_copy_rows_as_json.tsx # packages/kbn-unified-data-table/src/components/data_table_copy_rows_as_text.tsx # packages/kbn-unified-data-table/src/components/data_table_document_selection.test.tsx # packages/kbn-unified-data-table/src/components/data_table_document_selection.tsx # packages/kbn-unified-data-table/src/components/data_table_expand_button.tsx # packages/kbn-unified-data-table/src/hooks/use_control_column.ts # packages/kbn-unified-data-table/src/utils/copy_value_to_clipboard.test.tsx
10 tasks
davismcphee
added
Feature:Discover
Contributor
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
💚 Build Succeeded
Metrics [docs]Async chunks
To update your PR or re-run it, just comment with: |
kertal
approved these changes
Sep 25, 2024
Member
kertal
left a comment
kertal
left a comment
There was a problem hiding this comment.
LGTM, I've tested this PR on a a-la-carte
- first without this fix (could reproduce the race condition, but not reliable, but this is the nature of race conditions 🏃)
- then I applied this fix, and I could no longer reproduce it, so it works as expected, 🙏 for taking care of this so quickly
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport
This will backport the following commits from
mainto8.15:undefinederrors when row accessed viarows[rowIndex](#193791)Questions ?
Please refer to the Backport tool documentation