-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix explore tables don't display data when a global filter is applied #130024
Conversation
30e9dd5
to
6d20b3b
Compare
6d20b3b
to
4cf69a0
Compare
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been discussed and we agreed to go ahead with this approach since it is the fastest way to fix the bug.
However, It is not desirable to have these dependencies between the SiemSearchBar
shared component and all the tables that are using it.
It would be great to move those action dispatches out of the SearchBar in the future.
LGTM
useEffect(() => { | ||
if (fromStr != null && toStr != null) { | ||
timefilter.setTime({ from: fromStr, to: toStr }); | ||
} else if (start != null && end != null) { | ||
setTablesActivePageToZero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need setTablesActivePageToZero
in Line 104 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, we should. But I can't find a use case where it is necessary. Do you know a use case where the user can change fromStr
or toStr
beside the search bar? For example, line 106 is called when we update the time filter by interacting with a histogram.
@angorayc Do you think I should add setTablesActivePageToZero
to 104 for consistency? It would also add the side-effect that active pages are cleaned when the user navigates between pages because line 104 is called don when search_bar initializes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I guess I was confused between start and fromStr, end and toStr. If it is only called on navigating between pages, then no need to add it. Thanks Pablo.
💛 Build succeeded, but was flakyMetrics [docs]Async chunks
To update your PR or re-run it, just comment with: cc @machadoum |
…elastic#130024) (cherry picked from commit 3877763)
…rint-media-attempt-2 * 'main' of github.com:elastic/kibana: (75 commits) [Lens] Hide disabled toolbar entries (elastic#129994) Fix explore tables don't display data when a global filter is applied (elastic#130024) [Console] Add option to disable keyboard shortcuts (elastic#128887) [Discover] Update refreshOnClick flaky test (elastic#130001) [Uptime] remove latency limit warnings when using monitor management (elastic#129597) [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992) Paramaterized Discover tests (elastic#129684) [Security Solution][Investigations] - Minor bug fixes (elastic#130054) [DOCS} Adds technical preview to Lens annotations (elastic#130058) [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773) [Security Solutions] Adds API docs for value lists (elastic#129962) [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045) chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051) [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042) Upgrade RxJS to 7 (elastic#129087) [SecuritySolution] Clean up CaseContext (elastic#130036) Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)" Use RuleDataReader to query for threshold signal history (elastic#129763) Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769) Upgrade EUI to v54.0.0 (elastic#129653) ... # Conflicts: # x-pack/plugins/screenshotting/server/formats/pdf/index.ts
…elastic#130024) (cherry picked from commit 3877763)
issue: #129986
Summary
Tables inside security explore had the unexpected behaviour of persisting the selected page when the filter or time range changed. So when users changed the filter or time range and fewer items were returned, the table would still display the previously selected page even though that page had no data.
Impact: Every table inside Security Solutions / Explore
How to reproduce it
host.name
on the table and click onfilter in
Current behaviour
The table shows 0 items
Expected behaviour
The table should reset the page selection when the query, filter, or time range changes.
Before fix
After fix