-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
Overhaul search with a Command Palette #7569
Changes from 103 commits
1b37f3e
39589a3
22d83a7
cbc13a7
a240f54
654d71e
065e979
5048640
4e7f0c3
64864ab
a76919d
9b70f56
0fb9f1f
5bb9393
fb54dc0
1e2f45d
19077cd
f850297
31e345f
d7b4b92
2688ce3
b2f9a44
796395d
1d1df8e
bb9faf0
bc7da9e
0511461
081f60b
6f09f80
72252ec
8dc955d
285776f
51d2626
f44be2a
0a409e0
ce0410e
f203189
157858c
ad2e760
df50631
d61a966
1cf43ca
f608c94
480b919
5576ea2
1617295
accd74d
abd3361
ff66478
a52f773
aec5c9f
15e7c9a
c0ae7da
5fa1ed0
e475abe
4235d82
4b37c45
f205df1
3a14aa9
7569e2e
f4ed718
c498d44
b95a175
6f73030
da84e04
63b4fd2
20a4205
be776a2
d232157
7f53092
ce802bc
6092031
8d295bd
bf76b9c
4639387
a51e660
886e25e
c22f697
74d8598
3c35b30
6086a0f
8e85f30
2a96790
57017e6
94f4b7f
ef34c0b
692944f
c87bc9a
fb1641f
27c778c
972df51
0274960
c63da6b
1eee9d2
f8a6f63
337c05c
63869db
b208d80
30f80e0
3f61445
7af9147
a50e353
8344087
cd638da
8814ca7
8bdea5a
5cebb05
1a85512
5d1587e
599b02f
1ebc4f5
5ab218b
c5d77ee
0982df5
1db14a2
ecbbc8a
0ea776e
0833a4a
97363b4
a20e66a
b0dd174
57905c2
cdff812
7855ffc
13d2021
40e1d6b
d446143
ba7400f
f4f567a
89da576
8c6024c
57d5411
1651fae
6d6d395
ba564db
468a35d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,7 +129,7 @@ public void doSuggestOpenSearch(StaplerRequest req, StaplerResponse rsp, @QueryP | |
public void doSuggest(StaplerRequest req, StaplerResponse rsp, @QueryParameter String query) throws IOException, ServletException { | ||
Result r = new Result(); | ||
for (SuggestedItem item : getSuggestions(req, query)) | ||
r.suggestions.add(new Item(item.getPath())); | ||
r.suggestions.add(new Item(item.getPath(), item.getUrl())); | ||
|
||
rsp.serveExposedBean(req, r, Flavor.JSON); | ||
} | ||
|
@@ -207,12 +207,25 @@ public static class Result { | |
|
||
@ExportedBean(defaultVisibility = 999) | ||
public static class Item { | ||
|
||
@Exported | ||
@SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", justification = "read by Stapler") | ||
public String name; | ||
|
||
private final String url; | ||
|
||
public Item(String name) { | ||
this(name, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this result in correct behavior when used? I cannot tell. This also ends up bloating the responses of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing the autocomplete control with the Design Library and it works fine. It does bloat the response (with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Probably subtype / separate types. Unclear why
Works for me. |
||
} | ||
|
||
public Item(String name, String url) { | ||
this.name = name; | ||
this.url = url; | ||
} | ||
|
||
@Exported | ||
public String getUrl() { | ||
return url; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -62,7 +62,7 @@ private void getPath(StringBuilder buf) { | |||||
buf.append(item.getSearchName()); | ||||||
else { | ||||||
parent.getPath(buf); | ||||||
buf.append(' ').append(item.getSearchName()); | ||||||
buf.append(" → ").append(item.getSearchName()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While much better than before (no separator),
Suggested change
for consistency elsewhere in Jenkins (e.g., List Views with "Recurse in subfolders" enabled). Additionally (but easily deferred to the future I think, it was bad before too), it's weird to show raw job names here, instead of display names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Addressed in 7af9147 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unresolving thread for
(It's fine to defer since I think it's not a regression.) |
||||||
} | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
<!-- | ||
The MIT License | ||
|
||
Copyright (c) 2024, Jenkins contributors | ||
|
||
Permission is hereby granted, free of charge, to any person obtaining a copy | ||
of this software and associated documentation files (the "Software"), to deal | ||
in the Software without restriction, including without limitation the rights | ||
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
copies of the Software, and to permit persons to whom the Software is | ||
furnished to do so, subject to the following conditions: | ||
|
||
The above copyright notice and this permission notice shall be included in | ||
all copies or substantial portions of the Software. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
THE SOFTWARE. | ||
--> | ||
|
||
<?jelly escape-by-default='true'?> | ||
<j:jelly xmlns:j="jelly:core" xmlns:s="jelly:stapler" xmlns:l="/lib/layout"> | ||
<s:documentation> | ||
The command palette overlay | ||
</s:documentation> | ||
|
||
<div id="command-palette-i18n" class="i18n" | ||
data-no-results-for="${%No results for}" | ||
data-help="${%Help}" | ||
data-get-help="${%Get help using Jenkins search}" | ||
/> | ||
|
||
<dialog id="command-palette" class="jenkins-command-palette__dialog"> | ||
<div class="jenkins-command-palette__wrapper"> | ||
<div class="jenkins-command-palette"> | ||
<l:search-bar id="command-bar" clazz="jenkins-command-palette__search" hasKeyboardShortcut="false" /> | ||
<div id="search-results-container" class="jenkins-command-palette__results-container"> | ||
<div id="search-results" class="jenkins-command-palette__results" /> | ||
</div> | ||
</div> | ||
</div> | ||
</dialog> | ||
</j:jelly> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,12 @@ | ||
<?jelly escape-by-default='true'?> | ||
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:l="/lib/layout"> | ||
<div class="searchbox hidden-xs"> | ||
<!-- search box --> | ||
<j:set var="searchURL" value="${h.searchURL}"/> | ||
<form action="${searchURL}" method="get" style="position:relative;" class="no-json" name="search" role="search"> | ||
<!-- this div is used to calculate the width of the text box --> | ||
<div id="search-box-sizer"/> | ||
<div id="searchform"> | ||
<input name="q" placeholder="${searchPlaceholder}" id="search-box" class="main-search__input" value="${request.getParameter('q')}" role="searchbox" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gets dropped, but the legacy search pages and their internal links ( |
||
|
||
<span class="main-search__icon-leading"> | ||
<l:icon src="symbol-search"/> | ||
</span> | ||
<a href="${searchHelpUrl}" class="main-search__icon-trailing"> | ||
<l:icon src="symbol-help-circle"/> | ||
</a> | ||
|
||
<div id="search-box-completion" data-search-url="${searchURL}" /> | ||
<st:adjunct includes="jenkins.views.JenkinsHeader.search-box" /> | ||
timja marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</div> | ||
</form> | ||
<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout"> | ||
<div class="page-header__hyperlinks"> | ||
<button id="button-open-command-palette" tooltip="${%Search}" data-keyboard-shortcut="CMD+K" | ||
data-search-url="${rootURL + '/search/suggest'}" | ||
data-search-help-url="${searchHelpUrl}" | ||
> | ||
<l:icon src="symbol-search" /> | ||
</button> | ||
</div> | ||
<script src="${resURL}/jsbundles/keyboard-shortcuts.js" type="text/javascript"/> | ||
</j:jelly> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
import static org.junit.Assert.assertFalse; | ||
import static org.junit.Assert.assertNotNull; | ||
import static org.junit.Assert.assertTrue; | ||
import static org.jvnet.hudson.test.QueryUtils.waitUntilStringIsPresent; | ||
|
||
import hudson.model.FreeStyleProject; | ||
import hudson.model.ListView; | ||
|
@@ -38,7 +39,6 @@ | |
import hudson.security.GlobalMatrixAuthorizationStrategy; | ||
import java.io.IOException; | ||
import java.lang.reflect.Field; | ||
import java.net.HttpURLConnection; | ||
import java.net.URL; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
|
@@ -48,6 +48,8 @@ | |
import net.sf.json.JSONObject; | ||
import net.sf.json.JSONSerializer; | ||
import org.htmlunit.Page; | ||
import org.htmlunit.html.HtmlButton; | ||
import org.htmlunit.html.HtmlInput; | ||
import org.htmlunit.html.HtmlPage; | ||
import org.junit.Rule; | ||
import org.junit.Test; | ||
|
@@ -64,15 +66,22 @@ public class SearchTest { | |
|
||
@Rule public JenkinsRule j = new JenkinsRule(); | ||
|
||
/** | ||
* No exact match should result in a failure status code. | ||
*/ | ||
private void searchWithoutNavigating(HtmlPage page, String query) throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be moved to |
||
HtmlButton button = page.querySelector("#button-open-command-palette"); | ||
button.click(); | ||
|
||
HtmlInput search = page.querySelector("#command-bar"); | ||
search.setValue(query); | ||
page.executeJavaScript("document.querySelector('#command-bar').dispatchEvent(new Event(\"input\"))"); | ||
} | ||
|
||
@Test | ||
public void testFailure() throws Exception { | ||
WebClient wc = j.createWebClient() | ||
.withThrowExceptionOnFailingStatusCode(false); | ||
HtmlPage resultPage = wc.search("no-such-thing"); | ||
assertEquals(HttpURLConnection.HTTP_NOT_FOUND, resultPage.getWebResponse().getStatusCode()); | ||
HtmlPage page = j.createWebClient().goTo(""); | ||
|
||
searchWithoutNavigating(page, "no-such-thing"); | ||
|
||
waitUntilStringIsPresent(page, "No results for no-such-thing"); | ||
} | ||
|
||
/** | ||
|
@@ -81,13 +90,17 @@ public void testFailure() throws Exception { | |
@Issue("JENKINS-3415") | ||
@Test | ||
public void testXSS() throws Exception { | ||
WebClient wc = j.createWebClient() | ||
.withThrowExceptionOnFailingStatusCode(false); | ||
WebClient wc = j.createWebClient(); | ||
wc.setAlertHandler((page, message) -> { | ||
throw new AssertionError(); | ||
}); | ||
HtmlPage resultPage = wc.search("<script>alert('script');</script>"); | ||
assertEquals(HttpURLConnection.HTTP_NOT_FOUND, resultPage.getWebResponse().getStatusCode()); | ||
FreeStyleProject freeStyleProject = j.createFreeStyleProject("Project"); | ||
freeStyleProject.setDisplayName("<script>alert('script');</script>"); | ||
|
||
Page result = wc.search("<script>alert('script');</script>"); | ||
|
||
assertNotNull(result); | ||
assertEquals(j.getInstance().getRootUrl() + freeStyleProject.getUrl(), result.getUrl().toString()); | ||
} | ||
|
||
@Test | ||
|
@@ -127,7 +140,7 @@ public void testSearchByProjectNameInAFolder() throws Exception { | |
MockFolder myMockFolder = j.createFolder("my-folder-1"); | ||
FreeStyleProject myFreeStyleProject = myMockFolder.createProject(FreeStyleProject.class, "my-job-1"); | ||
|
||
Page result = j.createWebClient().goTo(myMockFolder.getUrl() + "search?q=" + myFreeStyleProject.getFullName()); | ||
Page result = j.search(myFreeStyleProject.getName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Search results no longer change depending on what page you're on. |
||
|
||
assertNotNull(result); | ||
j.assertGoodStatus(result); | ||
|
@@ -181,41 +194,6 @@ public void testSearch2ProjectsWithSameDisplayName() throws Exception { | |
assertFalse(contents.contains(otherDisplayName)); | ||
} | ||
|
||
@Test | ||
public void testProjectNamePrecedesDisplayName() throws Exception { | ||
final String project1Name = "foo"; | ||
final String project1DisplayName = "project1DisplayName"; | ||
final String project2Name = "project2Name"; | ||
final String project2DisplayName = project1Name; | ||
final String project3Name = "project3Name"; | ||
final String project3DisplayName = "project3DisplayName"; | ||
|
||
// create 1 freestyle project with the name foo | ||
FreeStyleProject project1 = j.createFreeStyleProject(project1Name); | ||
project1.setDisplayName(project1DisplayName); | ||
|
||
// create another with the display name foo | ||
FreeStyleProject project2 = j.createFreeStyleProject(project2Name); | ||
project2.setDisplayName(project2DisplayName); | ||
|
||
// create a third project and make sure it's not picked up by search | ||
FreeStyleProject project3 = j.createFreeStyleProject(project3Name); | ||
project3.setDisplayName(project3DisplayName); | ||
|
||
// search for foo | ||
Page result = j.search(project1Name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test was reliant on pressing enter and having Jenkins automagically figure out which project you intended - this is no longer possible with the command palette, if there are no results then you'll get a message saying 'No results' instead. |
||
assertNotNull(result); | ||
j.assertGoodStatus(result); | ||
|
||
// make sure we get the project with the name foo | ||
String contents = result.getWebResponse().getContentAsString(); | ||
assertTrue(contents.contains(String.format("<title>%s [Jenkins]</title>", project1DisplayName))); | ||
// make sure projects 2 and 3 were not picked up | ||
assertFalse(contents.contains(project2Name)); | ||
assertFalse(contents.contains(project3Name)); | ||
assertFalse(contents.contains(project3DisplayName)); | ||
} | ||
|
||
@Test | ||
public void testGetSuggestionsHasBothNamesAndDisplayNames() throws Exception { | ||
final String projectName = "project name"; | ||
|
@@ -335,7 +313,7 @@ public void testProjectNameInAFolderDisplayName() throws Exception { | |
|
||
String name = (String) jsonSuggestion.get("name"); | ||
|
||
if (displayName2.equals(name)) { | ||
if ("my-folder-1 → job-2".equals(name)) { | ||
foundDisplayName = true; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/** | ||
* @param {string} searchTerm | ||
*/ | ||
function search(searchTerm) { | ||
const address = document.getElementById("button-open-command-palette").dataset | ||
.searchUrl; | ||
return fetch(`${address}?query=${encodeURIComponent(searchTerm)}`); | ||
} | ||
|
||
export default { search: search }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import { LinkResult } from "./models"; | ||
import Search from "@/api/search"; | ||
import * as Symbols from "./symbols"; | ||
|
||
export const JenkinsSearchSource = { | ||
execute(query) { | ||
const rootUrl = document.head.dataset.rooturl; | ||
|
||
function correctAddress(url) { | ||
if (url.startsWith("/")) { | ||
url = url.substring(1); | ||
} | ||
|
||
return rootUrl + "/" + url; | ||
} | ||
|
||
return Search.search(query).then((rsp) => | ||
rsp.json().then((data) => { | ||
return data["suggestions"].slice().map((e) => | ||
LinkResult({ | ||
icon: Symbols.SEARCH, | ||
label: e.name, | ||
url: correctAddress(e.url), | ||
}), | ||
); | ||
}), | ||
); | ||
}, | ||
}; |
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.
Why?
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.
Found the UX around these sorts of things a little confusing and offering multiple results for the same thing to be odd. Happy to restore it though.
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.
Makes sense to not have multiple items for the same thing (especially if one's just a substring). This removes all of them though?
OTOH There are other interesting actions (or adjacent views) that could make sense to add here, so if there's a larger plan to add more, that would be fine. Or if this just clutters up the search for jobs named "con", that would be pointless.