[Unified Data Table] Prevent undefined errors when row accessed via rows[rowIndex]
#193791
Conversation
This file contains 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
…can result in undefined reference errors that can't be caught by TS
davismcphee
added
Feature:Discover
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
To update your PR or re-run it, just comment with: cc @davismcphee |
davismcphee
changed the title
undefined errors when row accessed view rows[rowIndex]undefined errors when row accessed via rows[rowIndex]
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
jughosta
approved these changes
Sep 24, 2024
| const { getCountOfFilteredSelectedDocs, deselectSomeDocs, selectMoreDocs } = selectedDocsState; | ||
|
|
||
| const docIdsFromCurrentPage = useMemo(() => { | ||
| return getDocIdsForCurrentPage(rows, pageIndex, pageSize); | ||
| }, [rows, pageIndex, pageSize]); | ||
| }, [pageIndex, pageSize]); |
There was a problem hiding this comment.
Was this change intentional?
There was a problem hiding this comment.
Yes because getSelectAllButton is now a function that accepts rows as a param and returns a component, so rows in no longer a part of the React lifecycle and will only change whenever getSelectAllButton is called.
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 24, 2024
… `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)
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Sep 24, 2024
…row accessed via `rows[rowIndex]` (#193791) (#193908) # Backport This will backport the following commits from `main` to `8.x`: - [[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)](#193791) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis McPhee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-24T18:36:11Z","message":"[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)\n\n## Summary\r\n\r\nThis PR fixes an issue present in 8.15, but which no longer exists after\r\nlater refactoring, where saved search panels (possibly only ES|QL\r\npanels? It was hard to nail down since involved a race condition) could\r\nfail in dashboards when adding Unified Search filters that reduce the\r\nnumber of results in the grid.\r\n\r\nI'm still not 100% sure what the source of the issue was since it\r\ninvolved a race condition (didn't fail consistently for me locally) and\r\ninternal EUI data grid code, but I have a hunch. The `undefined` errors\r\noccurred when trying to access a row by index from `DataTableContext` in\r\ncertain cell renderers during the first render after search results had\r\nchanged. I believe what was happening was the change to\r\n`DataTableContext` triggered a re-render of the cells before EUI data\r\ngrid internally updated its state, resulting in attempts to access rows\r\nfrom within the cell renderers that no longer existed in the updated\r\n`DataTableContext`, and causing `undefined` reference errors.\r\n\r\nI'm making these changes in `main` instead of `8.15` directly because\r\nthe updated approach is generally a safer way to retrieve rows and\r\nprevent similar issues in the future. Previously we added `rows` to\r\n`DataTableContext` and retrieved a single row by index using bracket\r\nnotation, which can potentially return `undefined`, but TypeScript does\r\nnot protect against it for us. Instead I've updated `DataTableContext`\r\nwith a `getRowByIndex` method that correctly returns `DataTableRecord |\r\nundefined` and forces consumers to explicitly handle the `undefined`\r\nscenario.\r\n\r\nThe PR will require some manual backporting to 8.15 since some things\r\nhave changed since then, but it shouldn't be difficult.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6ff07918b0490fa7d66202376073a337da55c73e","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:fix","v9.0.0","Team:DataDiscovery","backport:prev-minor","backport:prev-major","Feature:UnifiedDataTable"],"title":"[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]`","number":193791,"url":"https://github.com/elastic/kibana/pull/193791","mergeCommit":{"message":"[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)\n\n## Summary\r\n\r\nThis PR fixes an issue present in 8.15, but which no longer exists after\r\nlater refactoring, where saved search panels (possibly only ES|QL\r\npanels? It was hard to nail down since involved a race condition) could\r\nfail in dashboards when adding Unified Search filters that reduce the\r\nnumber of results in the grid.\r\n\r\nI'm still not 100% sure what the source of the issue was since it\r\ninvolved a race condition (didn't fail consistently for me locally) and\r\ninternal EUI data grid code, but I have a hunch. The `undefined` errors\r\noccurred when trying to access a row by index from `DataTableContext` in\r\ncertain cell renderers during the first render after search results had\r\nchanged. I believe what was happening was the change to\r\n`DataTableContext` triggered a re-render of the cells before EUI data\r\ngrid internally updated its state, resulting in attempts to access rows\r\nfrom within the cell renderers that no longer existed in the updated\r\n`DataTableContext`, and causing `undefined` reference errors.\r\n\r\nI'm making these changes in `main` instead of `8.15` directly because\r\nthe updated approach is generally a safer way to retrieve rows and\r\nprevent similar issues in the future. Previously we added `rows` to\r\n`DataTableContext` and retrieved a single row by index using bracket\r\nnotation, which can potentially return `undefined`, but TypeScript does\r\nnot protect against it for us. Instead I've updated `DataTableContext`\r\nwith a `getRowByIndex` method that correctly returns `DataTableRecord |\r\nundefined` and forces consumers to explicitly handle the `undefined`\r\nscenario.\r\n\r\nThe PR will require some manual backporting to 8.15 since some things\r\nhave changed since then, but it shouldn't be difficult.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6ff07918b0490fa7d66202376073a337da55c73e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193791","number":193791,"mergeCommit":{"message":"[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)\n\n## Summary\r\n\r\nThis PR fixes an issue present in 8.15, but which no longer exists after\r\nlater refactoring, where saved search panels (possibly only ES|QL\r\npanels? It was hard to nail down since involved a race condition) could\r\nfail in dashboards when adding Unified Search filters that reduce the\r\nnumber of results in the grid.\r\n\r\nI'm still not 100% sure what the source of the issue was since it\r\ninvolved a race condition (didn't fail consistently for me locally) and\r\ninternal EUI data grid code, but I have a hunch. The `undefined` errors\r\noccurred when trying to access a row by index from `DataTableContext` in\r\ncertain cell renderers during the first render after search results had\r\nchanged. I believe what was happening was the change to\r\n`DataTableContext` triggered a re-render of the cells before EUI data\r\ngrid internally updated its state, resulting in attempts to access rows\r\nfrom within the cell renderers that no longer existed in the updated\r\n`DataTableContext`, and causing `undefined` reference errors.\r\n\r\nI'm making these changes in `main` instead of `8.15` directly because\r\nthe updated approach is generally a safer way to retrieve rows and\r\nprevent similar issues in the future. Previously we added `rows` to\r\n`DataTableContext` and retrieved a single row by index using bracket\r\nnotation, which can potentially return `undefined`, but TypeScript does\r\nnot protect against it for us. Instead I've updated `DataTableContext`\r\nwith a `getRowByIndex` method that correctly returns `DataTableRecord |\r\nundefined` and forces consumers to explicitly handle the `undefined`\r\nscenario.\r\n\r\nThe PR will require some manual backporting to 8.15 since some things\r\nhave changed since then, but it shouldn't be difficult.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6ff07918b0490fa7d66202376073a337da55c73e"}}]}] BACKPORT--> Co-authored-by: Davis McPhee <[email protected]>
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
davismcphee
added a commit
to davismcphee/kibana
that referenced
this pull request
Sep 24, 2024
… `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
davismcphee
added a commit
that referenced
this pull request
Sep 25, 2024
…sed via `rows[rowIndex]` (#193791) (#193921) # Backport This will backport the following commits from `main` to `8.15`: - [[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)](#193791) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis McPhee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-24T18:36:11Z","message":"[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)\n\n## Summary\r\n\r\nThis PR fixes an issue present in 8.15, but which no longer exists after\r\nlater refactoring, where saved search panels (possibly only ES|QL\r\npanels? It was hard to nail down since involved a race condition) could\r\nfail in dashboards when adding Unified Search filters that reduce the\r\nnumber of results in the grid.\r\n\r\nI'm still not 100% sure what the source of the issue was since it\r\ninvolved a race condition (didn't fail consistently for me locally) and\r\ninternal EUI data grid code, but I have a hunch. The `undefined` errors\r\noccurred when trying to access a row by index from `DataTableContext` in\r\ncertain cell renderers during the first render after search results had\r\nchanged. I believe what was happening was the change to\r\n`DataTableContext` triggered a re-render of the cells before EUI data\r\ngrid internally updated its state, resulting in attempts to access rows\r\nfrom within the cell renderers that no longer existed in the updated\r\n`DataTableContext`, and causing `undefined` reference errors.\r\n\r\nI'm making these changes in `main` instead of `8.15` directly because\r\nthe updated approach is generally a safer way to retrieve rows and\r\nprevent similar issues in the future. Previously we added `rows` to\r\n`DataTableContext` and retrieved a single row by index using bracket\r\nnotation, which can potentially return `undefined`, but TypeScript does\r\nnot protect against it for us. Instead I've updated `DataTableContext`\r\nwith a `getRowByIndex` method that correctly returns `DataTableRecord |\r\nundefined` and forces consumers to explicitly handle the `undefined`\r\nscenario.\r\n\r\nThe PR will require some manual backporting to 8.15 since some things\r\nhave changed since then, but it shouldn't be difficult.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6ff07918b0490fa7d66202376073a337da55c73e","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:fix","v9.0.0","Team:DataDiscovery","backport:prev-minor","backport:prev-major","Feature:UnifiedDataTable"],"number":193791,"url":"https://github.com/elastic/kibana/pull/193791","mergeCommit":{"message":"[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)\n\n## Summary\r\n\r\nThis PR fixes an issue present in 8.15, but which no longer exists after\r\nlater refactoring, where saved search panels (possibly only ES|QL\r\npanels? It was hard to nail down since involved a race condition) could\r\nfail in dashboards when adding Unified Search filters that reduce the\r\nnumber of results in the grid.\r\n\r\nI'm still not 100% sure what the source of the issue was since it\r\ninvolved a race condition (didn't fail consistently for me locally) and\r\ninternal EUI data grid code, but I have a hunch. The `undefined` errors\r\noccurred when trying to access a row by index from `DataTableContext` in\r\ncertain cell renderers during the first render after search results had\r\nchanged. I believe what was happening was the change to\r\n`DataTableContext` triggered a re-render of the cells before EUI data\r\ngrid internally updated its state, resulting in attempts to access rows\r\nfrom within the cell renderers that no longer existed in the updated\r\n`DataTableContext`, and causing `undefined` reference errors.\r\n\r\nI'm making these changes in `main` instead of `8.15` directly because\r\nthe updated approach is generally a safer way to retrieve rows and\r\nprevent similar issues in the future. Previously we added `rows` to\r\n`DataTableContext` and retrieved a single row by index using bracket\r\nnotation, which can potentially return `undefined`, but TypeScript does\r\nnot protect against it for us. Instead I've updated `DataTableContext`\r\nwith a `getRowByIndex` method that correctly returns `DataTableRecord |\r\nundefined` and forces consumers to explicitly handle the `undefined`\r\nscenario.\r\n\r\nThe PR will require some manual backporting to 8.15 since some things\r\nhave changed since then, but it shouldn't be difficult.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6ff07918b0490fa7d66202376073a337da55c73e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193791","number":193791,"mergeCommit":{"message":"[Unified Data Table] Prevent `undefined` errors when row accessed via `rows[rowIndex]` (#193791)\n\n## Summary\r\n\r\nThis PR fixes an issue present in 8.15, but which no longer exists after\r\nlater refactoring, where saved search panels (possibly only ES|QL\r\npanels? It was hard to nail down since involved a race condition) could\r\nfail in dashboards when adding Unified Search filters that reduce the\r\nnumber of results in the grid.\r\n\r\nI'm still not 100% sure what the source of the issue was since it\r\ninvolved a race condition (didn't fail consistently for me locally) and\r\ninternal EUI data grid code, but I have a hunch. The `undefined` errors\r\noccurred when trying to access a row by index from `DataTableContext` in\r\ncertain cell renderers during the first render after search results had\r\nchanged. I believe what was happening was the change to\r\n`DataTableContext` triggered a re-render of the cells before EUI data\r\ngrid internally updated its state, resulting in attempts to access rows\r\nfrom within the cell renderers that no longer existed in the updated\r\n`DataTableContext`, and causing `undefined` reference errors.\r\n\r\nI'm making these changes in `main` instead of `8.15` directly because\r\nthe updated approach is generally a safer way to retrieve rows and\r\nprevent similar issues in the future. Previously we added `rows` to\r\n`DataTableContext` and retrieved a single row by index using bracket\r\nnotation, which can potentially return `undefined`, but TypeScript does\r\nnot protect against it for us. Instead I've updated `DataTableContext`\r\nwith a `getRowByIndex` method that correctly returns `DataTableRecord |\r\nundefined` and forces consumers to explicitly handle the `undefined`\r\nscenario.\r\n\r\nThe PR will require some manual backporting to 8.15 since some things\r\nhave changed since then, but it shouldn't be difficult.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6ff07918b0490fa7d66202376073a337da55c73e"}},{"url":"https://github.com/elastic/kibana/pull/193908","number":193908,"branch":"8.x","state":"OPEN"}]}] BACKPORT-->
delanni
added a commit
that referenced
this pull request
Sep 25, 2024
delanni
added a commit
that referenced
this pull request
Sep 25, 2024
## Summary As the title says. Two competing changes made it in to main (#193415 & #193791), causing a type issue: https://buildkite.com/elastic/kibana-on-merge/builds/50786#019228ae-4487-45c6-867a-ba0590b1266d cc: @davismcphee @stratoula
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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
undefinederrors occurred when trying to access a row by index fromDataTableContextin certain cell renderers during the first render after search results had changed. I believe what was happening was the change toDataTableContexttriggered 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 updatedDataTableContext, and causingundefinedreference errors.I'm making these changes in
maininstead of8.15directly because the updated approach is generally a safer way to retrieve rows and prevent similar issues in the future. Previously we addedrowstoDataTableContextand retrieved a single row by index using bracket notation, which can potentially returnundefined, but TypeScript does not protect against it for us. Instead I've updatedDataTableContextwith agetRowByIndexmethod that correctly returnsDataTableRecord | undefinedand forces consumers to explicitly handle theundefinedscenario.The PR will require some manual backporting to 8.15 since some things have changed since then, but it shouldn't be difficult.
Checklist
For maintainers