Launchpad: Search for another PR (GitHub)

axosoft-ramint commented 2 months ago

Add ability to search for a PR not in the Launchpad list. UX needs to be talked through.

Normally our algorithm provides the Launchpad with some subset of all PRs. So, the full list of PRs accessible to the user is wider. The current search looks only for PRs selected for the Launchpad and we want user to be able to search among all other available PRs.

The current idea, but needs discussion and may be changed:

User can use the text input in the Launchpad quickpick, either in the main Launchpad list/step or through some other/new step, to search for a PR using its url. For example: https://github.com/gitkraken/vscode-gitlens/pull/3500. Or if that fails, we could just do a PR search with the integration using the user's search query.
If no item in the existing categorized list matches the search, we check other PRs in the repo (or repos) matching the user's text to see if we can find a matching PR, and show a Focus item (default to "other" category) for it.
The user can then open that item and take actions on it like any other Launchpad item.

Notes

Should first discuss design/UX with the team. For example: should there instead be a "search for a different PR" button or action item in the Launchpad list? Would that make it more discoverable?
Thinking of using integration.getPullRequest for the PR search when the user uses PR url, and integration.searchPullRequests for more general search, but the exact function/query may change depending on final design.
Note that if we use integration.getPullRequest or integration.searchPullRequests, they must be implemented for GitLab since Launchpad supports GitLab as well (currently it is only implemented for GitHub). This might be a good opportunity to add this functionality to the shared provider library, but for now we can implement them locally for GitLab as needed.

Specs and delivery

PR1. Search by URL GitHub

If I copy and paste a PR url directly into the input box on the main step, it should search for that PR

✅ filter the current list for the specified PR if it's there
✅ otherwise search for the specified PR by its ID
✅ make sure it's debounced properly
Fix few search bugs:
- ✅ Fix bug of not clearing the found item
- ✅ Fix bug of incorrect local search by PR-id
✅ be friendly to incomplete URLs or URLs with additional parts

PR2. Add full search mode

Add an always visible item such as "Search everywhere". If it's clicked, switch to the mode that searches across all PRs the user has access to using the search terms.

✅ Add full search mode
✅ Bug: Local search cannot find items of collapsed groups.

Developer's testing

Try different queries

A title or a repo name of a PR that exists in the Launchpad
A PR-number or a URL of a PR that exists in the Launchpad
An URL of a PR that does not exists in the Launchpad
Any query.

Supported expressions that get converted to a PR identity are reflected in the unit-test

Change query while searching

In order to debug this part I increase the debouncing timeout and add delays into the debounced procedure.

While searching get back from API search to local filter:
While searching get Back from API search to local search by URI
- in 1-2, check that the current search is cancelled, its result is not applied
While searching change the search query
- the second result is applied
- the first debounced promise is not cancelled, so it enters the finally method at the same time when the second query enters
- inside of the first debounced request we see that the cancellation token require us to cancel, so we don't apply the result

Change query after the search

After an API search get back to the local filter
After an API search get back to the local search by URI
- in 1-2, check that the additional search is removed from the result (e.g. it cannot be found by its title)

sergeibbb commented 1 month ago

@axosoft-ramint

Question. In the old code I see how the quickpick.value affects that groups are being hidden. But I don't see where does the actual filtering happen. Is it provided by VSCode itself?

axosoft-ramint commented 1 month ago

@sergeibbb VSCode indeed internally filters the quickpick items to only those that match your search (though I'm not sure exactly what parts of the quickpick items it uses to match, other than their main label). There is an alwaysShow (I forgot the exact name) property we can toggle on/off for items as a hack to get around this internal filtering, and that's what the old code is using.

sergeibbb commented 1 month ago

Hi @axosoft-ramint

I have 3 questions for you.

The first is a noob one. What does the returning boolean from the event handler means in a QuickPickStep?

onDidChangeValue?(quickpick: QuickPick<DirectiveQuickPickItem | T>): boolean | Promise<boolean>;

https://github.com/gitkraken/vscode-gitlens/compare/main...draft/3543-pr-search#diff-25ee8b0447aeac38780fa0402ebffabcbbbc8c30024e1a61e681ec5d8e6694bcL555-R609

The next question was discussed on the daily meeting today. So, no need to answer. We also discussed the UX flows that should be used in this task. I'll put it in the description soon.

~~Another question. Who does sort?~~ ~~I'm trying to create two sections as on Justin's mockup:~~

~~I'm adding a header in front of the rest of the items:~~

~~But it goes below the all items~~

Third. Do I undestand right that this setting to alwaysShow is because the VS Code hides all items that don't match the filter, and by setting alwaysShow <- true we tell VS Code that it should be visible even when it doesn't match?

~~Specifically, here, where we setup the item that is matches by prNumber:~~ https://github.com/gitkraken/vscode-gitlens/compare/main...draft/3543-pr-search#diff-25ee8b0447aeac38780fa0402ebffabcbbbc8c30024e1a61e681ec5d8e6694bcR597-R598

Yes. I see it.

axosoft-ramint commented 1 month ago

Hi @axosoft-ramint

I have 3 questions for you.

The first is a noob one. What does the returning boolean from the event handler means in a QuickPickStep?
onDidChangeValue?(quickpick: QuickPick<DirectiveQuickPickItem | T>): boolean | Promise<boolean>;

@eamodio Can you shed some light on this one? I'm actually not sure and the API doesn't specify clearly.

@sergeibbb I didn't see any other open questions in the post - let me know if I missed anything.

sergeibbb commented 3 weeks ago

Hi @axosoft-ramint I have a question for you.

The debouncer is designed to be disposable. But do you have an idea where it's better to register it as a disposable object? Now it's managed by LaunchpadItemQuickPickItem that doesn't implement Disposable nor handling disposing in any other way.

Probably if we could detect a moment when the Launchpad panel becomes hidden, that would let us to cancel the requests that are in progress.

axosoft-ramint commented 3 weeks ago

@sergeibbb I would suggest that we clear/cancel/dispose the debouncer whenever the Launchpad itself, rather than an item, changes to a different step, is hidden, or is otherwise disposed.

sergeibbb commented 3 weeks ago

@axosoft-ramint Right. It's in LaunchpadCommand now. I've pasted the wrong name, I'm sorry.

So, every time we see that the LaunchpadCommand changes step or gets closed. It's on every yield step, isn't it?

How do we detect that it's closed?

or is otherwise disposed. LaunchpadCommand seems to be not disposable.

axosoft-ramint commented 3 weeks ago

@sergeibbb Not the command object but the quickpick, which would be a param in step events like onDidChangeValue.

cc @eamodio in case there's an easier way

sergeibbb commented 3 weeks ago

@axosoft-ramint @eamodio

Yeah. I thought that I should control the quickpick. However, I'm not sure how to do it right.

I get the quickpick object each time from arguments. So, I don't even know whether it's the same object or not, therefore it's not clear when I should subscribe on its events such as onDidHide and how to detect that its step changes.

gitkraken / vscode-gitlens