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

Speed up PR list and other improvements #1737

Merged
merged 41 commits into from Jul 4, 2018

Conversation

Projects
None yet
6 participants
@grokys
Copy link
Contributor

grokys commented Jun 15, 2018

This PR refactors the PR list to:

Use GraphQL for loading

By using GraphQL we can load only the fields we're interested in, which speeds up our queries.

Use data virtualization so that only visible items will be loaded

Previously, when the GitHub pane was opened, all pull requests were loaded. This could take minutes for repositories with a lot of pull requests, and the whole time they were loading VS would be slow.

With this PR, only the first 100 PRs will be loaded when the GitHub pane is shown, which is a single request. When the user scrolls down, more PRs will be loaded on demand.

Display a message when no items found

We currently have two messages:

When no filters are active and no PRs are found:

image

When filters are active and no PRs are found:

image

  • Improve the author filter

The author filter is now searchable:

image

Unfortunately because we're now only loading the first page of results, we don't have a full list of all users that have opened a PR, instead we use the GraphQL "Assignable Users" endpoint to get a list similar to on .com and allow the user to also select logins that they enter that aren't on that list.

Depends on #1712

@grokys grokys added the WIP label Jun 15, 2018

@grokys grokys force-pushed the refactor/pr-list branch from 4ce5a0a to 373226f Jun 17, 2018

grokys added some commits Jun 17, 2018

Fix dark theme.
By adding a default `ListBox` style that respects the VS theme.

@meaghanlewis meaghanlewis added this to the 2.5.4 milestone Jun 18, 2018

grokys and others added some commits Jun 19, 2018

Fix non-compiling tests.
But only by commenting out `PullRequestListViewModelTests` for the moment. These need to be re-done.

@StanleyGoldman StanleyGoldman force-pushed the refactor/pr-list branch from e119d79 to 5b6ac81 Jun 20, 2018

@meaghanlewis

This comment has been minimized.

Copy link
Contributor

meaghanlewis commented Jun 20, 2018

@grokys Dogfooding this and am so impressed with all the improvements to the PR list! 💥

Leaving some feedback and questions. I know this is still WIP so apologies if you're already addressing some of these things.

General feedback (and screenshots 😄 )

  • The new PR list is looking so smooth. It's nice to tell from a glance things like which PR branch I'm on, how many comments a PR has, and the number of open/closed/all PRs.
    new pr list

  • I love the new PR author filter!
    new author filter

  • And the view when there are no PR's is great too.
    no prs

Things to fix

  • No link to open PR in browser
    no link on pr

  • Link to open pulls in browser doesn't do anything
    link to open pulls doesnt work

Questions

  • When the PR list is still loading the count shows 0 for PRs. I think it's fine as is, but wondering if it would be possible to not show this count until it's loaded?
    loading prs

  • When there are no PRs the author filter shows but there is nothing to filter so this option doesn't really do anything. Is it possible to not show this filter or the count of PRs here?
    filter when no prs

cc @donokuda for thoughts on the above questions ^

@donokuda

This comment has been minimized.

Copy link
Member

donokuda commented Jun 28, 2018

FYI: After chatting with @grokys, we're going to move any design discussions to a separate issue so that this pull request can stay focused on refactoring instead of redesigning 😄

@donokuda

This comment has been minimized.

Copy link
Member

donokuda commented Jun 29, 2018

Out of curiosity, what is the timestamp of each PR list item referencing? It looks like it's something other than the time it was opened. Would it make sense to use the "Opened at" date instead?

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jul 2, 2018

Would it make sense to use the "Opened at" date instead?

👍

.com allows to sort by "Least recently updated". It's interesting because it highlights which are the more "active" PRs. There could be "old" PRs that just take a while, but still get updated regularly.

On the other hand, sorted by "Newest" makes the list much more constant/predictable. There is no random re-ordering and new PRs just show up at the top. Maybe good to show that as the default.

@donokuda

This comment has been minimized.

Copy link
Member

donokuda commented Jul 2, 2018

On the other hand, sorted by "Newest" makes the list much more constant/predictable. There is no random re-ordering and new PRs just show up at the top. Maybe good to show that as the default.

Yeah, I think if we're settled on sorting by recently updated, then we should consider reflecting that in the timestamp (like what you mentioned that .com does.)

grokys added some commits Jul 3, 2018

Clear user filter when item selected.
And add tests for `UserFilterViewModel`.

@grokys grokys changed the title WIP: Refactoring PR list Refactoring PR list Jul 4, 2018

@grokys

This comment has been minimized.

Copy link
Contributor Author

grokys commented Jul 4, 2018

This PR is now ready for review! There's still some discussion around #1768 so I've not merged that in yet. If a decision is made before this PR is merged, then we can merge it in here, otherwise it can be made to master.

@jcansdale
Copy link
Collaborator

jcansdale left a comment

This looks and works great!

Just spotted a typo and suggested generalizing a method.

this.graphqlFactory = graphqlFactory;
}

public async Task<string> ReadParentOwnerLogin(HostAddress address, string owner, string name)

This comment has been minimized.

@jcansdale

jcansdale Jul 4, 2018

Collaborator

I wonder if this would be more useful as a general FindParent method that returns a (string owner, string repositoryName)? A parent repository might not have the same name as its child and we may want to support this as some point.

This comment has been minimized.

@grokys

grokys Jul 4, 2018

Author Contributor

Ah yeah, good point. I will make this change.

This comment has been minimized.

@grokys

grokys Jul 4, 2018

Author Contributor

I changed this to return the repository name and owner. We don't actually use the name yet,: UriString doesn't support getting a new URL with a different name as well as owner so want to do that in a separate PR, this is big enough as it is ;)

/// </summary>
/// <typeparam name="T">The item type.</typeparam>
/// <remarks>
/// This interface is used the the <see cref="VirtualizingList{T}"/> class to load pages of data.

This comment has been minimized.

@jcansdale

jcansdale Jul 4, 2018

Collaborator

"by the" not "the the"?

grokys added some commits Jul 4, 2018

ReadParentOwnerLogin -> FindParent.
Make the method to find the parent repository return the repository name and owner. We don't actually use the `name` yet, and assume that forks have the same name as the parent repository. This is usually the case but not always. `UriString` doesn't support getting a new URL with a different name as well as owner so want to do that in a separate PR.

@grokys grokys merged commit 1d801fb into master Jul 4, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@grokys grokys changed the title Refactoring PR list Refactored PR list Jul 10, 2018

@grokys grokys changed the title Refactored PR list Speed up PR list and other improvements Jul 10, 2018

@grokys grokys referenced this pull request Nov 14, 2018

Merged

Remove trackingcollection #2055

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.