Fixes unnecessary autocompletes on HTTP methods#163233
Merged
sakurai-youhei merged 29 commits intoelastic:mainfrom Aug 23, 2023
Merged
Fixes unnecessary autocompletes on HTTP methods#163233sakurai-youhei merged 29 commits intoelastic:mainfrom
sakurai-youhei merged 29 commits intoelastic:mainfrom
Conversation
fixes elastic#156254 refixes elastic#120606 - should stay closed unfixes elastic#19961 - should be reopened
Member
Author
|
@elasticmachine merge upstream |
Member
Author
|
@elasticmachine merge upstream |
yuliacech
suggested changes
Aug 8, 2023
Contributor
yuliacech
left a comment
There was a problem hiding this comment.
Thank you for working on this issue, @sakurai-youhei!
Here are my thoughts on the changes:
- Good to have this fixed, it's indeed annoying that the methods autocomplete is constantly displayed on arrow navigation
- I'm not sure we need to fix this: the request works when the method uses mixed lower/upper case letters like
Get /_searchso that is not needed in my opinion - That is regression that I have to block the PR on. I think it's important to have urls displayed for example after an index name and an underscore:
GET /.kibana/_. The tradeoff for me is not worth to fix 1. but introducing this regression
Member
Author
|
@yuliacech thank you for your comments.
|
Member
Author
|
@elasticmachine merge upstream |
Member
Author
|
@yuliacech How do you find this approach and outcome? Thanks. |
Contributor
|
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
yuliacech
reviewed
Aug 14, 2023
Contributor
yuliacech
left a comment
There was a problem hiding this comment.
I was testing locally and noticed that when I type just 1 letter, there is no autocomplete for methods, for example I would expect to see POST, PUT when typing P. This currently works on main. Is this intended?
yuliacech
reviewed
Aug 14, 2023
| // Prompt autocomplete window and provide a initial letter of properties to narrow down the results. E.g. 'b' = 'bool' | ||
| public async promptAutocomplete(letter = 'b') { | ||
| // give browser enough moments to complete events from past UI operations | ||
| await this.common.sleep(200); |
Contributor
There was a problem hiding this comment.
I'd rather avoid using sleep if at all possible. There are other ways to wait for UI loading to compelte, for example waitFor.
Member
Author
There was a problem hiding this comment.
After reading the codes further, I found evaluateCurrentTokenAfterAChange function is protected by debounce with 100ms delay HERE, which is the reason why autocomplete does not appear without a short interval between operations.
So, I'm afraid there's no realistic (<-added) way to avoid using sleep. In addition to that, because I need to depend on sleep more to write tests, I split it out to iconic sleepforDebouncePeriod(duration: number = 100) function HERE.
Member
Author
There was a problem hiding this comment.
Furthermore, I increased the default sleep period to 200ms because lodash's debounce uses inaccurate setTimeout.
https://github.com/lodash/lodash/blob/4.17.21/lodash.js#L10488
Member
Author
There was a problem hiding this comment.
Tests for auto-complete have no longer been flaky since this build. 100ms delay after sync exec JS code seems to work well with lodash's 100ms debounce.
💔 Build #151657 failed 5ec4ced
Contributor
|
Maybe we should add more tests for those regressions that keep popping up |
Member
Author
|
Have added functional tests, which also cover the cases for autocomplete on typing the first character like left=PR / right=8.9.1 Please give me your feedback, @yuliacech . Thank you. |
Contributor
|
For me locally, the autocomplete still doesn't work when only 1 letter is typed in. It does work when it's not the first request though. Very weird behaviour, here is the screen recording Screen.Recording.2023-08-22.at.14.44.31.mov |
Member
Author
|
@yuliacech I agree it's wired. But the behavior is not a regression. I find the same observation even on 8.9.1. Shall we open a new issue for the behavior? I'll probably address it apart from this PR. |
Contributor
|
You're right @sakurai-youhei , that is a pre-existing behaviour. I'll open an issue for that. |
yuliacech
approved these changes
Aug 23, 2023
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Aug 23, 2023
* main: (150 commits) Fixes unnecessary autocompletes on HTTP methods (elastic#163233) [Defend Workflows] Convert filterQuery to kql (elastic#161806) [Fleet] copy `inactivity_timeout` when duplicating agent policy (elastic#164544) Fix 7.17 forward compatibility with 8.2+ (elastic#164274) [ML] Fixes dark mode in flyouts and modals (elastic#164399) [Defend Workflows]Changes to policy settings are not persistent until a refresh (elastic#164403) [Security Solution][Endpoint] Fixes kibana crash when going back to policy details page (elastic#164329) Prepare the Security domain HTTP APIs for Serverless (elastic#162087) skip failing test suite (elastic#160986) [Security Solution] Fix flaky Event Filters test (elastic#164473) [EDR workflows] Osquery serverless tests (elastic#163795) [Fleet] Only show agent dashboard links if there is more than one non-server agent and if the dashboards exist (elastic#164469) [Chrome UI] Fix background color in serverless (elastic#164419) [DOCS] Saved objects - resolve import errors API (elastic#162825) Remove 'Create Rule' button from Rule Group page (elastic#164167) [Security Solution] expandable flyout - fix infinite loop in correlations (elastic#163450) [Remote Clusters] Update copy about port help text (elastic#164442) [api-docs] 2023-08-23 Daily api_docs build (elastic#164524) [data views] Disable scripted fields in serverless environment (elastic#163228) [Reporting] Fix - show diagnostic only when image reporting is enabled (elastic#164336) ...
yuliacech
added a commit
to yuliacech/kibana
that referenced
this pull request
Oct 12, 2023
…)" This reverts commit fade57d
9 tasks
2 tasks
sakurai-youhei
added a commit
that referenced
this pull request
Oct 23, 2023
## Summary This PR fixes autocomplete triggering on URL tokens ~~for url parameters to work with a single character~~. Fixes #168017 (which is a regression introduced by #163233)  ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Notes - No functional tests are added because they would also go flaky. - ~~No unit tests are added because of the lack of existing unit tests.~~ - ~~The change is kept minimal by accepting the growing if-else block.~~ ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 23, 2023
## Summary This PR fixes autocomplete triggering on URL tokens ~~for url parameters to work with a single character~~. Fixes elastic#168017 (which is a regression introduced by elastic#163233)  ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Notes - No functional tests are added because they would also go flaky. - ~~No unit tests are added because of the lack of existing unit tests.~~ - ~~The change is kept minimal by accepting the growing if-else block.~~ ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 8fe2e1a)
kibanamachine
added a commit
that referenced
this pull request
Oct 24, 2023
# Backport This will backport the following commits from `main` to `8.11`: - [Fix autocomplete triggering on URL tokens (#168956)](#168956) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Youhei Sakurai","email":"youhei.sakurai@elastic.co"},"sourceCommit":{"committedDate":"2023-10-23T22:42:30Z","message":"Fix autocomplete triggering on URL tokens (#168956)\n\n## Summary\r\n\r\nThis PR fixes autocomplete triggering on URL tokens ~~for url parameters\r\nto work with a single character~~.\r\n\r\nFixes #168017 (which is a regression introduced by #163233)\r\n\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### Notes\r\n\r\n- No functional tests are added because they would also go flaky.\r\n- ~~No unit tests are added because of the lack of existing unit\r\ntests.~~\r\n- ~~The change is kept minimal by accepting the growing if-else block.~~\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"8fe2e1ac687678e0fe5558ca61853614b287adc0","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Console","Team:Deployment Management","release_note:skip","v8.11.0","v8.12.0"],"number":168956,"url":"https://github.com/elastic/kibana/pull/168956","mergeCommit":{"message":"Fix autocomplete triggering on URL tokens (#168956)\n\n## Summary\r\n\r\nThis PR fixes autocomplete triggering on URL tokens ~~for url parameters\r\nto work with a single character~~.\r\n\r\nFixes #168017 (which is a regression introduced by #163233)\r\n\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### Notes\r\n\r\n- No functional tests are added because they would also go flaky.\r\n- ~~No unit tests are added because of the lack of existing unit\r\ntests.~~\r\n- ~~The change is kept minimal by accepting the growing if-else block.~~\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"8fe2e1ac687678e0fe5558ca61853614b287adc0"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/168956","number":168956,"mergeCommit":{"message":"Fix autocomplete triggering on URL tokens (#168956)\n\n## Summary\r\n\r\nThis PR fixes autocomplete triggering on URL tokens ~~for url parameters\r\nto work with a single character~~.\r\n\r\nFixes #168017 (which is a regression introduced by #163233)\r\n\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### Notes\r\n\r\n- No functional tests are added because they would also go flaky.\r\n- ~~No unit tests are added because of the lack of existing unit\r\ntests.~~\r\n- ~~The change is kept minimal by accepting the growing if-else block.~~\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"8fe2e1ac687678e0fe5558ca61853614b287adc0"}}]}] BACKPORT--> Co-authored-by: Youhei Sakurai <youhei.sakurai@elastic.co>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR,,,
_#19961 differently from Fix autocomplete suggestions for lowercase methods and other related bug #121033[1] left=PR / right=8.9.0
[2] left=PR / right=8.9.0
[3] left=PR / right=8.9.0
Original description
_#19961 - which must be reopened or duplicated after merging this PR[1] left=PR / right=8.9.0
[2] left=PR / right=8.9.0
Autocomplete starts if the method is all uppercase or all lowercase; it doesn't with mixed cases such as
Get,gET, etc. anymore.[3] left=PR / right=8.9.0
Autocomplete no longer starts on first typing
_afterurl.slash. No simple solution makes me leave this issue unfixed.Checklist
For maintainers
Other notes
backport:skipbecause [Console] Unnecessary autocomplete suggestions for HTTP methods #156254 is not applicable to 7.x.The release note doesn't need to mention the unfix of Console doesn't display suggestions when you first type_#19961 because Fix autocomplete suggestions for lowercase methods and other related bug #121033 mentioned it asother related bugonly.Release note
Fixes unnecessary autocompletes on HTTP methods