Skip to content
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

[Watcher] Retain query in watcher list #163297

Merged
merged 6 commits into from
Aug 8, 2023

Conversation

sabarasaba
Copy link
Member

Summary

When working on #162687 we changed the table search to be controlled by the component it self. But as a side effect, the auto-refresh from the table clears up the query after x amount of time. We didnt caught this back then because the test that checks that was disabled and we only figured out after we tried to re-enable them with #162592.

This PR's patches it up so that we retain the query only when we dont have any errors.

@sabarasaba sabarasaba added Feature:Watcher Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.10.0 labels Aug 7, 2023
@sabarasaba sabarasaba self-assigned this Aug 7, 2023
@sabarasaba
Copy link
Member Author

@elasticmachine merge upstream

@sabarasaba sabarasaba marked this pull request as ready for review August 8, 2023 12:25
@sabarasaba sabarasaba requested a review from a team as a code owner August 8, 2023 12:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for patching this up @sabarasaba! Left one comment about surfacing the error message.

@@ -446,7 +449,15 @@ export const WatchListPage = () => {
: '',
};

const handleOnChange = ({ queryText, error: queryError }: EuiSearchBarOnChangeArgs) => {
if (!queryError) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we surface an error message if there is an error? Otherwise, it might not be clear to the user why a particular search returned no results.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! I've updated the changes and added a test too:

Screenshot 2023-08-08 at 16 36 03

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest LGTM. Thanks @sabarasaba!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
watcher 163.4KB 163.9KB +497.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @sabarasaba

@sabarasaba sabarasaba merged commit 49eadc5 into elastic:main Aug 8, 2023
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Watcher release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants