Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Speed up PR list and other improvements #1737
Conversation
Using GraphQL and a virtualizing list view.
Doesn't actually filter yet though.
By adding a default `ListBox` style that respects the VS theme.
But only by commenting out `PullRequestListViewModelTests` for the moment. These need to be re-done.
…udio into refactor/pr-list
|
@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
|
|
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 |
|
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? |
.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. |
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.) |
And add tests for `UserFilterViewModel`.
|
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. |
|
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) |
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.
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.
grokys
Jul 4, 2018
Author
Contributor
Ah yeah, good point. I will make this change.
Ah yeah, good point. I will make this change.
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 ;)
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. |
jcansdale
Jul 4, 2018
Collaborator
"by the" not "the the"?
"by the" not "the the"?
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.
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:
When filters are active and no PRs are found:
The author filter is now searchable:
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