[Discover] [Group By] Support restoring UI state when switching tabs#253608
Conversation
There was a problem hiding this comment.
I was planning to add some FTR tests for the Discover changes, but the scroll restoring doesn't seem to work for me, and I noticed an odd regression. I think we're quite close on this, maybe just need to work through the last bit together including scrolling.
828b0d6 to
6a20f72
Compare
davismcphee
left a comment
There was a problem hiding this comment.
Thanks for working on the scroll restoration @eokoneyo, definitely working better now. I added some functional tests to cover our changes and attempted to fix type and lint errors. Not sure if you have any other changes you need to do, but otherwise at this point I think things work well enough for the MVP once it passes CI.
| ); | ||
|
|
||
| virtualizerRef.current = useCascadeVirtualizer<DataTableRecord>({ | ||
| // @ts-expect-error -- it's fine to do this as long as we're not using the sticky group header functionality |
There was a problem hiding this comment.
I'm not a fan of using @ts-expect-error here, but not a blocker for MVP.
There was a problem hiding this comment.
I know... I'll make some changes
There was a problem hiding this comment.
I've rather opted to track this here #255075, when it gets fixed this will error and we'd remove the @ts-expect-error
b532874 to
b55b542
Compare
|
Cloud deployment initiated, see credentials at: https://buildkite.com/elastic/kibana-deploy-cloud-from-pr/builds/774 |
ed79e0f to
5d2743c
Compare
1f25625 to
c827c6e
Compare
| scrollToVirtualizedIndex: ((offset, options = {}) => { | ||
| return virtualizerImpl.options.scrollToFn(offset, options, virtualizerImpl); | ||
| }) satisfies CascadeVirtualizerReturnValue['scrollToVirtualizedIndex'], |
There was a problem hiding this comment.
🟠 High virtualizer/index.tsx:503
scrollToVirtualizedIndex calls virtualizerImpl.options.scrollToFn(offset, ...) which expects a pixel offset, but scrollToLastVirtualizedRow (line 517) passes rows.length - 1 as an index. This causes scrollToLastVirtualizedRow() to scroll to pixel position N-1 instead of the actual offset of the last row. Consider using virtualizerImpl.scrollToIndex which handles the index-to-offset conversion.
- scrollToVirtualizedIndex: ((offset, options = {}) => {
- return virtualizerImpl.options.scrollToFn(offset, options, virtualizerImpl);
- }) satisfies CascadeVirtualizerReturnValue['scrollToVirtualizedIndex'],🤖 Copy this AI Prompt to have your agent fix this:
In file src/platform/packages/shared/shared-ux/document_data_cascade/impl/src/lib/core/virtualizer/index.tsx around lines 503-505:
`scrollToVirtualizedIndex` calls `virtualizerImpl.options.scrollToFn(offset, ...)` which expects a pixel offset, but `scrollToLastVirtualizedRow` (line 517) passes `rows.length - 1` as an index. This causes `scrollToLastVirtualizedRow()` to scroll to pixel position `N-1` instead of the actual offset of the last row. Consider using `virtualizerImpl.scrollToIndex` which handles the index-to-offset conversion.
Evidence trail:
- Kibana code: src/platform/packages/shared/shared-ux/document_data_cascade/impl/src/lib/core/virtualizer/index.tsx lines 454, 503-505, 516-518 at REVIEWED_COMMIT
- TanStack Virtual library source: https://github.com/TanStack/virtual/blob/main/packages/virtual-core/src/index.ts - scrollToFn signature shows first parameter is pixel offset, not index (line ~295 type definition, line ~824 _scrollToOffset implementation)
- TanStack Virtual scrollToIndex method (line ~716) shows proper index-to-offset conversion using getOffsetForIndex()
c827c6e to
ba60223
Compare
ba60223 to
0000ee1
Compare
|
/ci |
1 similar comment
|
/ci |
627a902 to
0e1f7dd
Compare
|
/ci |
0e1f7dd to
8cc9f5a
Compare
8cc9f5a to
a7a82c5
Compare
Appreciate the cleanup... I also made some changes to address valid AI feedback, I'll also reach out to my team to give this a look. |
kowalczyk-krzysztof
left a comment
There was a problem hiding this comment.
Code changes LGTM. I didn't test the functionality, leaving that up to @eokoneyo
- handling for non-viable row marked as expanded in persisted state - get scroll position restoration on switching tabs to work - opt to reset persisted state once the query changes - prevent measurement updates before first paint - improvement to cascade component for restoration phase - scroll position restoration for nested virtualized grid - Minor type cleanup - improve exposed public API for cascade component to include index of topmost visible item - consolidate scroll position restoration into helper hook - leverage scroll restoration util within row grid - consolidate scroll restoration into cascade virtualizer and adopt it for nested virtualizer use case - specify containment options for cascade wrapper, row and cell - defer overscan config till after next paint - Fix types and linting - Fix aggregatedValues badge rendering - Add functional tests - cleanup cascade component - remove reference to removed apis
- structural changes to virtualizer implementation - favor anchor item index over scroll offset for scroll restoration - use cascade virtualizer for nesting in discover - deferred mounting for nested rows - add foundation for controlled child virtualizer - add orchestration for nested virtualized children - adjustments so returning expanded rows do not show loading skeleton - render a loader in place of white screen whilst cascade is bootstrapping
- opt to reset persisted state once the query changes - scroll position restoration for nested virtualized grid - adjustments so scroll restoration matches previous row exactly
akowalska622
left a comment
There was a problem hiding this comment.
Code looks good to me and I tested it locally. I'll leave the final approval to @MiloszRadzynski who volunteered to give it a second look 👀
MiloszRadzynski
left a comment
There was a problem hiding this comment.
LGTM 🚀 Tested I didn't notice any issues! Thank you
|
@elasticmachine merge upstream |
|
/flaky ftrConfig:src/platform/test/functional/apps/discover/cascade_layout/config.ts:25 |
Flaky Test Runner✅ Build triggered - kibana-flaky-test-suite-runner#11358
|
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#11358[❌] src/platform/test/functional/apps/discover/cascade_layout/config.ts: 0/25 tests passed. |
Replace fixed sleep(100) waits with retry-based polling in expectScrollToBeRoughly. The helper already has a 5-second retry window, so the sleep was both wasteful and insufficient. Also measure actual scroll position before tab switch instead of predicting it based on arithmetic, which is unreliable after row expansion.
Wrap isCascadeLayoutRowExpanded and the find+click sequence in toggleCascadeLayoutRow with retry.try() to handle StaleElementReferenceError when the virtualizer re-renders DOM elements between finding and interacting with them.
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#11370[✅] src/platform/test/functional/apps/discover/cascade_layout/config.ts: 25/25 tests passed. |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Async chunks
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
cc @davismcphee |
| }); | ||
|
|
||
| const NO_VALUE_PLACEHOLDER = i18n.translate('discover.dataCascade.row.action.noValue', { | ||
| defaultMessage: '(blank)', |
There was a problem hiding this comment.
(blank) we use for empty strings when formatting field values. Wonder if (null) fit better here.
There was a problem hiding this comment.
Yes I think that makes sense. Opened a followup issue for it here: #261839.
Summary
This PR adds persisted group by UI state in Discover so switching between tabs restores the cascade where the user left it. It preserves the main cascade scroll position, expanded groups, and nested row state for expanded groups, so returning to a tab restores the previous view instead of starting fresh.
Feature flag for testing:
feature_flags.overrides.discover.cascadeLayoutEnabled: trueExample queries for testing:
FROM kibana_sample_data_logs | STATS count = COUNT(*) BY extension.keywordFROM kibana_sample_data_logs | STATS count = COUNT(*) BY ipResolves #249456.
Resolves #246985.
Checklist
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.