jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

Overhaul search with a Command Palette

Open janfaracik opened this issue 2 years ago • 43 comments

image image

This PR introduces the Command Palette concept to Jenkins, first discussed back in the day in the November 2021 UX SIG meeting. You've probably used something similar if you've used VS Code, Jenkins.io, GitHub, Spotify etc. They're handy overlays that allow you to quickly use your keyboard to navigate, be that searching for different pages or finding functions in menus.

This is an early implementation and as such doesn't have all the features I want it to have, such as grouped results, icons and richer details for results, all aimed at making the search experience in Jenkins easier to use. I'd also like for plugin developers to eventually be able to integrate with the Command Palette so they can provide their own rich results/actions, for example being able to change theme from the palette.

Testing done

  • Clicking the search button shows the Command Palette
  • CMD + K shows the Command Palette
  • Searching for terms brings up identical results to the previous search bar

Proposed changelog entries

  • Add Command Palette as replacement for search bar

Proposed upgrade guidelines

N/A

Submitter checklist

  • [ ] The Jira issue, if it exists, is well-described.
  • [ ] The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • [ ] There is automated testing or an explanation as to why this change has no tests.
  • [ ] New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • [ ] New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • [ ] New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • [ ] For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • [ ] For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • [ ] There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • [ ] Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • [ ] Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • [ ] Proper changelog labels are set so that the changelog can be generated automatically.
  • [ ] If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

janfaracik avatar Jan 06 '23 00:01 janfaracik

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Apr 03 '23 12:04 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Apr 21 '23 07:04 github-actions[bot]

one test failing

timja avatar May 29 '23 14:05 timja

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Jun 05 '23 15:06 github-actions[bot]

There's been several tests commented out/removed and this is due to the JS behind the Command Palette not being supported by HTMLUnit.

Is that still the case with now HTMLUnit 3?

NotMyFault avatar Jun 05 '23 21:06 NotMyFault

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Aug 15 '23 14:08 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Aug 18 '23 14:08 github-actions[bot]

is this ready for re-review by the security team?

timja avatar Aug 19 '23 16:08 timja

is this ready for re-review by the security team?

Yep :)

janfaracik avatar Aug 19 '23 18:08 janfaracik

I've tested the PR locally, and the XSS is no longer present. Everything else looks fine from a security perspective

Kevin-CB avatar Aug 22 '23 08:08 Kevin-CB

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Aug 23 '23 06:08 github-actions[bot]

With the current very narrow fix, the same problem occurs when searching for "Built-In Node", "manage", or "admin" (a user name), probably everything that's not a job.

How are you running the PR? I can't seem to reproduce this.

Should this be a draft? It doesn't seem ready yet.

👍

janfaracik avatar Aug 26 '23 12:08 janfaracik

How are you running the PR? I can't seem to reproduce this.

docker run --rm -ti -p 8080:8080 -e ID=7569 jenkins/core-pr-tester

Steps to reproduce:

  1. Create a job "fs"
  2. Navigate to /job/fs/
  3. Cmd-K
    1. "Built-In Node" -> /job/fs/computer/(built-in)/ (404)
    2. "manage" -> /job/fs/manage/ (404)
    3. "admin" -> /job/fs/user/admin/ (404)
    4. "All" -> /job/fs (instead of the root URL, for the default view, a secondary view is /job/fs/view/whatever/, 404)

daniel-beck avatar Aug 26 '23 13:08 daniel-beck

How are you running the PR? I can't seem to reproduce this.

docker run --rm -ti -p 8080:8080 -e ID=7569 jenkins/core-pr-tester

Steps to reproduce:

  1. Create a job "fs"

  2. Navigate to /job/fs/

  3. Cmd-K

    1. "Built-In Node" -> /job/fs/computer/(built-in)/ (404)
    2. "manage" -> /job/fs/manage/ (404)
    3. "admin" -> /job/fs/user/admin/ (404)
    4. "All" -> /job/fs (instead of the root URL, for the default view, a secondary view is /job/fs/view/whatever/, 404)

Thanks! I can't run Docker on my machine at the moment due to some issues however I've tried my best to reproduce it by altering the Jenkins URL and updating any relative URLs.

They should all be sorted now.

janfaracik avatar Aug 26 '23 15:08 janfaracik

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Jan 03 '24 09:01 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar May 18 '24 15:05 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar May 23 '24 15:05 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar May 24 '24 21:05 github-actions[bot]

Tests passing and ATH passing - https://github.com/jenkinsci/acceptance-test-harness/pull/1103

janfaracik avatar May 26 '24 17:05 janfaracik

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar May 27 '24 21:05 github-actions[bot]

To help this MR forward I've removed the keyboard shortcut tooltip - will restore it in a future MR.

janfaracik avatar May 29 '24 18:05 janfaracik

Might be color blindness, but the highlight of the selected row is too subtle. It's basically not present at all. In particular with a single result, it's difficult to tell it is selected.

Agreed, increased the contrast there - let me know if that's better.

The /search URL remains functional, is this intentional?

I haven't made any changes to it - would be happy to remove it in a follow up if its no longer used/convert the page to use the command palette for backwards compatibility.

Typing multiple characters quickly results in the same request being sent multiple times, indicating a problem in JS.

Screenshot 2024-06-06 at 23 21 15

Great spot - I had messed up debounce in the same way I had in the build history MR 🤦‍♂️ Fixed it now

It would be nice if, when closing and reopening the popup, the previous input were selected, for easy deletion (easily deferred).

Done

janfaracik avatar Jun 07 '24 08:06 janfaracik

Thanks for addressing some of my feedback, I'll try to take a look later.

Note that most of my line comments remain unaddressed (not even rejected), in case that is just an oversight.

daniel-beck avatar Jun 10 '24 13:06 daniel-beck

Thanks for addressing some of my feedback, I'll try to take a look later.

Note that most of my line comments remain unaddressed (not even rejected), in case that is just an oversight.

Thanks! From what I can see I've responded to your line comments unless I'm missing some

janfaracik avatar Jun 10 '24 14:06 janfaracik

There's a search bug that becomes slightly worse with this change. I think it's non-blocking, but noting in case you think different.

Create folders Plugins, Plugins/tinfoil-scan, and freestyle job Plugins/tinfoil-scan/PR-22 (I found this on ci.j.io trying to compare before/after). Search for plugin fo. The first hit will be the non-existent job Plugins » PR-22 and selecting it will get a 404 Rage Jenkins. (The existing UI kept you in an infinite loop of search result pages linking to themselves, never jumping to a (non-existing) exact match.)

daniel-beck avatar Jun 10 '24 20:06 daniel-beck

Is it deliberate that there is currently no indicator for the Cmd-K shortcut? Is that something an updated #8435 would add on top?

daniel-beck avatar Jun 10 '24 20:06 daniel-beck

https://plugins.jenkins.io/lucene-search/ kinda breaks with this PR as the (now augmented) legacy result pages are no longer used. https://github.com/jenkinsci/lucene-search-plugin/blob/lucene-search/src/main/java/org/jenkinsci/plugins/lucene/search/FreeTextSearchFactory.java#L20 is how the plugin currently provides custom search result pages -- is there something we can tell the maintainer to adapt to this change? (This is the only issue that for me is still (semi-)blocking; if we can do something for this plugin without an extensive rewrite of this PR or that plugin, we should.)

Eventually I'd like the command palette and its results to be extensible by developers (and maybe that's something that plugin could implement in the future).

Could a workaround for the timebeing for the plugin to add an additional control to the header/somewhere to access that search page?

janfaracik avatar Jun 10 '24 21:06 janfaracik

Could a workaround for the timebeing for the plugin to add an additional control to the header/somewhere to access that search page?

I think something like that would be reasonable, if it can be implemented in a way that doesn't break regularly whenever the header is changed. Even better, ability to remove the new search, and restore the old text field via Jelly somehow?

Alternatively, an extension point that overrides where you go for a search result from the popup? By default, directly there, and the plugin could override it with its fancy results page?

daniel-beck avatar Jun 11 '24 07:06 daniel-beck

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Jun 14 '24 13:06 github-actions[bot]

Hey.

image

Let me know if this is the right approach.. I've added support for plugins to add their own search buttons to the header, e.g. in this example Lucene has added a 'Lucene' button to access their UI by implementing search.jelly. It's fairly minimal to implement and shouldn't be too much work for the plugin owners to shift the search input to their page.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
    <a href="...">
        Lucene
    </a>
</j:jelly>

janfaracik avatar Jun 14 '24 16:06 janfaracik