Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[8.x] [Unified Data Table] Prevent `undefined` errors when …
…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]>
- Loading branch information