[Discover] Disable refresh interval for data views without time fields and rollups #137134
Conversation
davismcphee
added
Feature:Discover
| @@ -293,6 +312,8 @@ export function getState({ | |||
| stateStorage | |||
| ); | |||
|
|
|||
| const { start, stop } = syncAppState(); | |||
There was a problem hiding this comment.
Calling syncState from within initializeAndSync fixes the back button state issue. This is because syncState takes a snapshot of the initial state to compare against before syncing state updates: https://github.com/elastic/kibana/blob/main/src/plugins/kibana_utils/public/state_sync/state_sync.ts#L151. When syncState is called from outside of initializeAndSync, the snapshot doesn't get reset when the data view is changed. Then when the user presses the back button, the new state appears to be the same as the initial state, so syncState ignores the update. Moving the call to syncState inside of initializeAndSync means the snapshot gets updated each time the data view changes and no state changes are ignored.
There was a problem hiding this comment.
Great findings Davis, makes sense. Could you inline a comment how it when syncAppState is executed? best where the function is defined. I think it is helpful for future work, wasn't aware of the fact
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
| return { | ||
| kbnUrlStateStorage: stateStorage, | ||
| appStateContainer: appStateContainerModified, | ||
| startSync: start, | ||
| stopSync: stop, | ||
| startSync: () => { |
There was a problem hiding this comment.
I just thought, it might be worth to mention here that we just use this function for testing
… where going back and forward through history was not updating data view
davismcphee
force-pushed
the
fix-discover-refresh-interval
branch
from
bf85afc to
6495774
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @davismcphee |
…s and rollups (elastic#137134) * [Discover] Fix refresh interval running for non time series data * [Discover] Clean up disable auto refresh interval code, and fix issue where going back and forward through history was not updating data view * [Discover] Clean up discover_state tests * [Discover] Add tests for disable auto refresh * [Discover] Add comments for state syncing code (cherry picked from commit 998b11a)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added
the
backport missing
|
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
…s and rollups (#137134) (#137316) * [Discover] Fix refresh interval running for non time series data * [Discover] Clean up disable auto refresh interval code, and fix issue where going back and forward through history was not updating data view * [Discover] Clean up discover_state tests * [Discover] Add tests for disable auto refresh * [Discover] Add comments for state syncing code (cherry picked from commit 998b11a) Co-authored-by: Davis McPhee <[email protected]>
kibanamachine
removed
the
backport missing
Summary
This PR fixes an issue where the auto refresh interval continues to run after switching to a data view without time fields even though the date picker is hidden. This means there is no way to disable the refresh interval except by switching back to a data view with a time field. Now the refresh interval will automatically be paused when the date picker is hidden.
There is also a fix for an issue that was affecting the interval refresh where pressing the back button would update the URL and global state, but the app state and the data view wouldn't update:
Fixes #102132.
Checklist
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsAny UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listThis renders correctly on smaller devices using a responsive layout. (You can test this in your browser)For maintainers