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

Improvements to the PR flow #2293

Closed
7 of 8 tasks
kieferrm opened this issue Nov 16, 2020 · 4 comments
Closed
7 of 8 tasks

Improvements to the PR flow #2293

kieferrm opened this issue Nov 16, 2020 · 4 comments
Assignees
Labels
feature-request Request for new features or functionality

Comments

@kieferrm
Copy link
Member

kieferrm commented Nov 16, 2020

  • Open a codespace on a PR, merge the PR: What do we do with the codespace?
  • Can we improve the PR creation workflow: using a view, auto-creating a draft PR when creating a branch etc.?
  • What do both of the above flows look like in the web and on the desktop when working in a codespace?
  • Review the non-codespace experience in the desktop based on the changes proposed for the above flows

Create a PR

  • Add "Create PR" command to SCM view @RMacfarlane
  • Add terminal link handler for published branches to start create flow @RMacfarlane
  • 🏃 Integrate with GitLens "Create PR"
  • 🏃 Offer to create PR on branch publish from git extension
  • Webview for entering PR details @RMacfarlane
  • Tree for displaying the potential PR diff @RMacfarlane

Opening a PR

  • 🏃 Focus GitHub viewlet if opening a PR in a codespace @RMacfarlane

Finish a PR

  • Rename "Delete Branch" button to "Clean up..." and add option to suspend current codespace if in codespace @RMacfarlane
@RMacfarlane
Copy link
Contributor

Had a great meeting with @eamodio, here are some notes:

Finishing a PR

Thinking about codespaces, when a user finishes reviewing a PR, they may want to reuse the codespace to continue reviewing other PRs, or to delete the codespace.

A codespace is loosely associated with a branch - from the repo page, the codespaces shown are those created for the currently selected branch. From a PR page, codespaces are also filtered to those that are associated with the branch for the PR. However, the codespace's tie to the branch seems to only matter for the initial state it opens in. If you delete the branch, the codespace remains, and if you open the codespace and switch branches, opening the codespace again doesn't cause you to switch back. So the current model leans towards "create a codespace specifically for reviewing this PR" rather than "create a codespace I will use to review all PRs", but both actions still seem useful, especially given how long it can take to initialize a codespace.

Continue reviewing other PRs

To checkout a different pull request from the list, the "focused mode" view could be changed to:

  1. Have an explicit back button which reverts to the list views. This easiest thing to do would be to switch off of the current PR branch and back to the default branch. We could also explore having a way to leave "focused mode" without leaving the PR branch, but we then need a clear gesture to get back into it. There are a couple of feature requests around this so this might be worth exploring later.
  2. Instead of completely removing the list views in "focused mode", having these views collapse at the bottom of the viewlet. This has the advantage of not having to go through an intermediate step to check out something else, and also being able to reference other PRs while looking at the current one. Doing this requires adding VSCode API to manage view state.
  3. Show the "focused mode" views in an entirely different viewlet. This creates a nice separate context for the PR, so it also has the advantages of 2.

For any of these, the experience could be the same in a codespace or on desktop.

One thing to think about in the future is how "focused mode" interacts with multi-root workspaces, how it should display when you have multiple checked out branches in different folders. (Also, one edge case we don't handle is if a branch is associated with multiple PRs.)

Deleting the codespace

After merging a PR, the merge button is replaced with a "Delete Branch" button. Deleting the codespace is very similar to this, it's also cleaning up resources for the PR. So one option for this is to leverage the current delete flow. If we detect that we are in a codespace based on the remote authority, we could add another entry in the quickpick to delete the codespace.

At this point we talked a lot about the relationship between codespaces and branches - does it make sense to delete a branch but not a codespace, or vice versa? If the branch gets deleted but the codespace doesn't, the codespace is sort of orphaned in the GitHub web UI, you would have to go back to the deleted branch to find it. But I might want to keep the codespace around to continue to review or do other work, but clean up the branch immediately. So, forcing you to delete the codespace and branch at the same time might be too restrictive.

Creating a PR

Create PR flow

The series of quick picks we use right now makes it hard to have a holistic view of what you're creating. In the past we explored using a webview to do this, if we use the new "WebviewView"s in the activity bar, we could have one view for inputting the PR details, target branch, title, and description, and a tree view of the changes between the source branch and target branch. Codestream already has an experience like this.

If we go with these new views, we should match whatever approach we take with "focused mode", either entirely replace the lists, collapsing them, or showing the views in their own transient viewlet.

Right now with the quick pick experience, if your local branch doesn't have an upstream, one of the quick picks asks you what name you want to publish the branch with. Instead of trying to capture this rename ability, we can still have the automatic publish using the local branch name.

The create experience should be the same in codespaces or desktop.

Automatic draft PRs

The problem we would have to work around with automatically creating a draft PR when you create a new branch is that there needs to be some diff between the source and target. Assuming this is a commit diff and not a literal diff, a possible hack would be to push an empty commit to the branch. (Further hack would be to then reset HEAD back after creating the PR and delete the commit.) Would be good to check in with GitHub about removing this limitation.

Some other miscellaneous thoughts about improving PR create:

  • Allow choosing the source branch instead of just having it be the current branch. (Or have changing the source dropdown also change the branch you're on if we don't want to change the assumptions about this.)
  • There could be other triggers to starting the flow, such as when a branch is pushed upstream, from the SCM viewlet, or triggered by another extension like GitLens. (In the extension case, it would also be cool to send parameters that could prepopulate the create view)
  • Automate the creation of a branch as well as publishing it upstream - so if I started making some local changes on the default branch and realize I want to create a PR with them, fold the creation of the branch into this as one action.

@RMacfarlane RMacfarlane added this to the November 2020 milestone Nov 18, 2020
@RMacfarlane RMacfarlane added the feature-request Request for new features or functionality label Nov 18, 2020
@RMacfarlane
Copy link
Contributor

Creating a PR

Create is still in the works, here's what the webview looks like currently:
Screen Shot 2020-12-18 at 1 57 31 PM

The "use commit" and "use branch name" buttons need to be styled, clicking these will prefill the title and ask the user if they want to always use that data for the title (i.e. change the pr title setting).

There isn't anything that surfaces the same thing for the description right now, but it will also pick up that setting and prefill it using the template or commit. In the old flow, it was able to gracefully handle a repo having multiple templates, right now I'm just choosing the first one, should think about how to surface this in the UI later.

I also still need to update the form to have inline validation instead of using error notifications.

Other create entry points

Eric and I talked about what other flows a user might want to do on Codespaces to start a PR, one idea was to mimic what we do for "Start working on issue". On an issue page, there could be the option to open a codespace, which would automatically create a feature branch for working on the issue and prepopulate the SCM commit message with the issue number

@RMacfarlane
Copy link
Contributor

Some more good discussion today with Eric!

Entrypoints to create

  • contribute an action to the SCM view to create a PR
  • use GitLens API to register an action runner that gets called when the GitLens "Create PR" action is used
  • expose new Git extension API for branch publish events. This will only capture publishes from within VSCode, not the command line. Detecting that a publish has happened just by looking for changes to git is difficult, since that requires keeping track of all branches, and an upstream ref being added to a branch doesn't necessarily indicate that it was just published. We could potentially also use the current git state change event to track if the current branch has an added upstream, but once again don't know that this was a publish. It might be possible to use git hooks to detect this more accurately

Finishing create

  • Instead of immediately closing out the webview, we could instead just make it show a success message and options to open the PR in VSCode, on GitHub, and to close out of create
  • We could also transition to focused mode

Focused mode

  • Biggest open question - is focused mode truly a "mode", something that takes over the viewlet that you can transition in and out of, or should it be in a separate viewlet?
  • Decoupling review/focused mode and the currently active branch has been a long-standing feature request

Open PR

  • It would be useful to have a way of opening a PR by number of by url. (also potentially a url handler). The question is what should happen in the UI when this is triggered - revealing something in the PR tree would be difficult. Instead we could just open the description page for that PR, since this has a way of immediately checking out the PR.

Finishing a PR

  • On the description page, the "Delete Branch" button should be "Delete Branch..." to make it clear that a quick pick will appear instead of immediate action being taken
  • After merge, we could expose both "Delete Branch..." and also a "Switch Branch" button at the bottom of the description page
  • For codespaces, we need to detect that we are in a codespace, and determine how to run the suspend or delete action. The button can also be grouped with the delete action

@RMacfarlane
Copy link
Contributor

Closing in favor of #2372, which tracks any remaining work and polish

@RMacfarlane RMacfarlane removed this from the February 2021 milestone Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

3 participants