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

Fix global search crash on missing tag #159196

Merged
merged 8 commits into from Jun 9, 2023

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jun 7, 2023

Summary

Close #157682, #159152

Fixes global search component crashing on missing tag. Ideally this should never happen as data is inconsistent, but we also shouldn't just crash the component.

Adds a console warning in case hitting this edge case.

@Dosant Dosant added release_note:fix Feature:Navigational Search Global search bar Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) backport:prev-minor Backport to the previous minor version (i.e. one version back from main) labels Jun 7, 2023
@Dosant Dosant requested a review from tsullivan June 7, 2023 11:51
@Dosant
Copy link
Contributor Author

Dosant commented Jun 7, 2023

@elasticmachine merge upstream

@Dosant Dosant marked this pull request as ready for review June 7, 2023 12:55
@Dosant Dosant requested a review from a team as a code owner June 7, 2023 12:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@Dosant Dosant requested a review from vadimkibana June 7, 2023 12:56
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM. I left a small note about updating the unit test, but I won't block merge because of it.

@Dosant
Copy link
Contributor Author

Dosant commented Jun 8, 2023

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Jun 8, 2023

@tsullivan, I didn't see the note

@Dosant
Copy link
Contributor Author

Dosant commented Jun 8, 2023

@elasticmachine merge upstream

Comment on lines 111 to 126
const option = resultToOption(input, [], getTag);
expect(option.append).toMatchInlineSnapshot(`
<ResultTagList
searchTagIds={Array []}
tags={
Array [
Object {
"color": "#000000",
"description": "Known",
"id": "known",
"name": "Known",
},
]
}
/>
`);
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding a spy on the console to verify the message that is logged:

    const logSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); // added

    const option = resultToOption(input, [], getTag); // current code
    // ... more current code

    expect(logSpy).toBeCalledWith(
      'SearchBar: Tag with id "unknown" not found. Tag "unknown" is referenced by the search result "dashboard:id". Skipping displaying the missing tag.'
    ); // added

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
globalSearchBar 21.1KB 21.3KB +217.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
globalSearchBar 1 2 +1
securitySolution 414 418 +4
total +7

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
globalSearchBar 1 2 +1
securitySolution 498 502 +4
total +7

History

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

@Dosant Dosant merged commit aa4a5e5 into elastic:main Jun 9, 2023
16 checks passed
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 9, 2023
## Summary

Close elastic#157682,
elastic#159152

Fixes global search component crashing on the missing tag. Ideally this
should never happen as data is inconsistent, but we also shouldn't just
crash the component.

Adds a console warning in case hitting this edge case.

(cherry picked from commit aa4a5e5)
kibanamachine added a commit that referenced this pull request Jun 9, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [Fix global search crash on missing tag
(#159196)](#159196)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Anton
Dosov","email":"anton.dosov@elastic.co"},"sourceCommit":{"committedDate":"2023-06-09T10:27:41Z","message":"Fix
global search crash on missing tag (#159196)\n\n## Summary\r\n\r\nClose
#157682
global search component crashing on the missing tag. Ideally
this\r\nshould never happen as data is inconsistent, but we also
shouldn't just\r\ncrash the component.\r\n\r\nAdds a console warning in
case hitting this edge
case.","sha":"aa4a5e5cc4459be3929f02d63ebb679448a531bd","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Feature:Navigational
Search","Team:SharedUX","backport:prev-minor","v8.9.0"],"number":159196,"url":"#159196
global search crash on missing tag (#159196)\n\n## Summary\r\n\r\nClose
#157682
global search component crashing on the missing tag. Ideally
this\r\nshould never happen as data is inconsistent, but we also
shouldn't just\r\ncrash the component.\r\n\r\nAdds a console warning in
case hitting this edge
case.","sha":"aa4a5e5cc4459be3929f02d63ebb679448a531bd"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"#159196
global search crash on missing tag (#159196)\n\n## Summary\r\n\r\nClose
#157682
global search component crashing on the missing tag. Ideally
this\r\nshould never happen as data is inconsistent, but we also
shouldn't just\r\ncrash the component.\r\n\r\nAdds a console warning in
case hitting this edge
case.","sha":"aa4a5e5cc4459be3929f02d63ebb679448a531bd"}}]}] BACKPORT-->

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this pull request Jun 14, 2023
## Summary

Close elastic#157682,
elastic#159152

Fixes global search component crashing on the missing tag. Ideally this
should never happen as data is inconsistent, but we also shouldn't just
crash the component.

Adds a console warning in case hitting this edge case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to the previous minor version (i.e. one version back from main) Feature:Navigational Search Global search bar release_note:fix Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.8.2 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When attempting to search for ‘dashboard’ in Kibana, the search bar disappears.
5 participants