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

Search in Launchpad by PR URL. GitHub (#3543) #3684

Merged
merged 11 commits into from
Nov 6, 2024

Conversation

sergeibbb
Copy link
Member

@sergeibbb sergeibbb commented Oct 18, 2024

Description

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

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number of relevant commits (typically 1 , don't squash my lovely commits, this PR is too big too be squashed).
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@sergeibbb sergeibbb linked an issue Oct 18, 2024 that may be closed by this pull request
@sergeibbb sergeibbb self-assigned this Oct 18, 2024
sergiolms

This comment was marked as resolved.

@sergeibbb sergeibbb force-pushed the feature/3543-extended-pr-search branch 3 times, most recently from d3d4f28 to 4ad520d Compare October 21, 2024 12:23
@sergeibbb

This comment was marked as resolved.

@sergeibbb
Copy link
Member Author

@sergiolms

After opening a URL in the browser, some items disappear but they still count, creating some empty items you cannot interact with:

It looks like existing behavior. Got to an item and put a query that doesn't match any command and these two item appear.

unclickable-items-in-launchpad.mp4

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

I didn't manage to get through all the logic in launchpad.ts yet, so these comments are not yet complete, but cover all the other changes. Mostly just nitpicks and questions at this point - will test more thoroughly on Monday.

src/system/vscode/asyncDebouncer.ts Outdated Show resolved Hide resolved
src/system/function.ts Show resolved Hide resolved
src/plus/launchpad/launchpadProvider.ts Outdated Show resolved Hide resolved
src/plus/launchpad/launchpadProvider.ts Outdated Show resolved Hide resolved
src/plus/integrations/providers/github.ts Outdated Show resolved Hide resolved
src/plus/integrations/providers/github.ts Outdated Show resolved Hide resolved
@sergeibbb sergeibbb force-pushed the feature/3543-extended-pr-search branch 5 times, most recently from 99c86e1 to 3578a54 Compare October 29, 2024 20:14
@sergeibbb sergeibbb force-pushed the feature/3543-extended-pr-search branch from 3578a54 to 10cda1c Compare October 30, 2024 14:21
@axosoft-ramint axosoft-ramint removed their request for review October 30, 2024 20:50
@axosoft-ramint
Copy link
Contributor

Accidentally requested my own review on this PR. I removed it - feel free to re-request my review whenever it's ready to go.

@sergeibbb
Copy link
Member Author

@axosoft-ramint

on the PR items that are search results, we should not have "pin" and "snooze" buttons/options, neither in this stage nor in the next stage after choosing the item (you will need to rebase this on main to see the "pin" and "snooze" options in the next step since that is new).

  1. Do you mean the state of searching? Such as removing "pin" and "snooze" from every item.
  2. Or only items that are "additional" so they don't belong to the current Launchpad set of PRs?

If # 1.

What if I want to pin an item but I don't see it in the launchpad, so I start searching it, then when it becomes clearly visible I click "pin". So, it becomes pinned.

Currently, clicking "pin" or "snooze" will do effectively nothing, as we cannot show categories until the search is empty, nor should those search results persist

Yes. We don't see the effect immediately, but the item has been affected, so in the non-searching state of the Launchpad we'll see the result of our action.

If # 2.
Probably the main problem is that we get "involves:@me" PRs from the GitHub. So, even if I've pinned a PR, so its ID is stored on GK.dev as "pinned", I won't see it in the Launchpad, because it's outside of the set of PRs where I'm involved.

However, it would make sense if we let users pin any PR.

But now I agree, it makes sense to remove these action from "additional" items that have been found.

Thoughts?
cc @eamodio @justinrobots

@axosoft-ramint
Copy link
Contributor

@axosoft-ramint

on the PR items that are search results, we should not have "pin" and "snooze" buttons/options, neither in this stage nor in the next stage after choosing the item (you will need to rebase this on main to see the "pin" and "snooze" options in the next step since that is new).

  1. Do you mean the state of searching? Such as removing "pin" and "snooze" from every item.
  2. Or only items that are "additional" so they don't belong to the current Launchpad set of PRs?

If # 1.

What if I want to pin an item but I don't see it in the launchpad, so I start searching it, then when it becomes clearly visible I click "pin". So, it becomes pinned.

Currently, clicking "pin" or "snooze" will do effectively nothing, as we cannot show categories until the search is empty, nor should those search results persist

Yes. We don't see the effect immediately, but the item has been affected, so in the non-searching state of the Launchpad we'll see the result of our action.

If # 2.
Probably the main problem is that we get "involves:@me" PRs from the GitHub. So, even if I've pinned a PR, so its ID is stored on GK.dev as "pinned", I won't see it in the Launchpad, because it's outside of the set of PRs where I'm involved.

However, it would make sense if we let users pin any PR.

But now I agree, it makes sense to remove these action from "additional" items that have been found.

Thoughts?
cc @eamodio @justinrobots

Yes, I'm referring specifically to the "additional" items found in search from the app, and precisely because of the bug you describe. They should be considered temporary and we should not support pin or snooze on them for now because they won't stick around. If we do want to support that in the future, we can make a ticket for it.

@sergeibbb
Copy link
Member Author

@axosoft-ramint

I suppose, what we do want to fix is to preserve search query on "back" step after selecting an item. Now the query gets lost every time I select an item and then go back.

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

LGTM

@sergeibbb sergeibbb force-pushed the feature/3543-extended-pr-search branch from 88594b0 to 6460b40 Compare November 6, 2024 16:32
@sergeibbb sergeibbb merged commit d2361ca into main Nov 6, 2024
3 checks passed
sergeibbb added a commit that referenced this pull request Nov 6, 2024
sergeibbb added a commit that referenced this pull request Nov 6, 2024
sergeibbb added a commit that referenced this pull request Nov 6, 2024
sergeibbb added a commit that referenced this pull request Nov 6, 2024
sergeibbb added a commit that referenced this pull request Nov 6, 2024
@sergeibbb sergeibbb deleted the feature/3543-extended-pr-search branch November 6, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Launchpad: Search for another PR (GitHub)
3 participants