diff --git a/package.json b/package.json index a0d91c8278..18d5a2f810 100644 --- a/package.json +++ b/package.json @@ -138,7 +138,7 @@ { "id": "github-pull-requests", "title": "GitHub Pull Requests", - "icon": "resources/icons/github.svg" + "icon": "resources/icons/light/github.svg" } ] }, diff --git a/resources/icons/dark/github.svg b/resources/icons/dark/github.svg new file mode 100644 index 0000000000..393d1c1e12 --- /dev/null +++ b/resources/icons/dark/github.svg @@ -0,0 +1,6 @@ + + + + + + diff --git a/resources/icons/github.svg b/resources/icons/light/github.svg similarity index 100% rename from resources/icons/github.svg rename to resources/icons/light/github.svg diff --git a/src/common/resources.ts b/src/common/resources.ts index e6f6ed445b..3ffe73662b 100644 --- a/src/common/resources.ts +++ b/src/common/resources.ts @@ -23,6 +23,7 @@ export class Resource { Ignored: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'status-ignored.svg')), Conflict: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'status-conflict.svg')), Comment: context.asAbsolutePath(path.join('resources', 'icons', 'comment.svg')), + Avatar: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'github.svg')), Fold: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'fold.svg')), Description: context.asAbsolutePath(path.join('resources', 'icons', 'light', 'git-pull-request.svg')) }, @@ -36,6 +37,7 @@ export class Resource { Ignored: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'status-ignored.svg')), Conflict: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'status-conflict.svg')), Comment: context.asAbsolutePath(path.join('resources', 'icons', 'comment.svg')), + Avatar: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'github.svg')), Fold: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'fold.svg')), Description: context.asAbsolutePath(path.join('resources', 'icons', 'dark', 'git-pull-request.svg')) }, diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index fda70a3ff2..4471dbdf3b 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -16,6 +16,7 @@ import { PRDocumentCommentProvider } from '../view/prDocumentCommentProvider'; import { convertRESTPullRequestToRawPullRequest, parseGraphQLPullRequest } from './utils'; import { PullRequestResponse, MentionableUsersResponse } from './graphql'; const queries = require('./queries.gql'); +import axois, { AxiosResponse } from 'axios'; export const PULL_REQUEST_PAGE_SIZE = 20; @@ -30,6 +31,7 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { static ID = 'GitHubRepository'; private _hub: GitHub | undefined; private _initialized: boolean; + private _repositoryReturnsAvatar: boolean | null; private _metadata: any; private _toDispose: vscode.Disposable[] = []; public commentsController?: vscode.CommentController; @@ -72,6 +74,7 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { } constructor(public remote: Remote, private readonly _credentialStore: CredentialStore) { + this._repositoryReturnsAvatar = remote.host.toLowerCase() === 'github.com' ? true : null; } get supportsGraphQl(): boolean { @@ -206,7 +209,29 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { } async getPullRequests(prType: PRType, page?: number): Promise { - return prType === PRType.All ? this.getAllPullRequests(page) : this.getPullRequestsForCategory(prType, page); + return await (prType === PRType.All ? this.getAllPullRequests(page) : this.getPullRequestsForCategory(prType, page)); + } + + public async ensureRepositoryReturnsAvatar(testAvatarUrl: string): Promise { + if (this._repositoryReturnsAvatar === null) { + let response: AxiosResponse | null = null; + + try { + response = await axois({method: 'get', url: testAvatarUrl, maxRedirects: 0}); + } catch (err) { + if(err && err instanceof Error) { + response = ( err).response as AxiosResponse; + } + } + + if (response && response.status === 200) { + this._repositoryReturnsAvatar = true; + } + + this._repositoryReturnsAvatar = false; + } + + return this._repositoryReturnsAvatar; } private async getAllPullRequests(page?: number): Promise { @@ -221,6 +246,12 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { }); const hasMorePages = !!result.headers.link && result.headers.link.indexOf('rel="next"') > -1; + + let repoReturnsAvatar: boolean = true; + if (result && result.data.length > 0) { + repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(result.data[0].user.avatar_url); + } + if (!result.data) { // We really don't expect this to happen, but it seems to (see #574). // Log a warning and return an empty set. @@ -242,7 +273,8 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { } const item = convertRESTPullRequestToRawPullRequest(pullRequest); - return new PullRequestModel(this, this.remote, item); + + return new PullRequestModel(this, this.remote, item, repoReturnsAvatar); } ) .filter(item => item !== null) as PullRequestModel[]; @@ -288,15 +320,23 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { }); const hasMorePages = !!headers.link && headers.link.indexOf('rel="next"') > -1; - const pullRequests = await Promise.all(promises).then(values => { - return values.map(item => { - if (!item.data.head.repo) { - Logger.appendLine('GitHubRepository> The remote branch for this PR was already deleted.'); - return null; - } - return new PullRequestModel(this, this.remote, convertRESTPullRequestToRawPullRequest(item.data)); - }).filter(item => item !== null) as PullRequestModel[]; - }); + const pullRequestResponses = await Promise.all(promises); + + let repoReturnsAvatar = true; + if (pullRequestResponses && pullRequestResponses.length > 0) { + repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(pullRequestResponses[0].data.user.avatar_url); + } + + const pullRequests = pullRequestResponses.map(response => { + if (!response.data.head.repo) { + Logger.appendLine('GitHubRepository> The remote branch for this PR was already deleted.'); + return null; + } + + const item = convertRESTPullRequestToRawPullRequest(response.data,); + return new PullRequestModel(this, this.remote, item, repoReturnsAvatar); + }).filter(item => item !== null) as PullRequestModel[]; + Logger.debug(`Fetch pull request catogory ${PRType[prType]} - done`, GitHubRepository.ID); return { @@ -328,9 +368,11 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { number: id } }); - Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID); - return new PullRequestModel(this, remote, parseGraphQLPullRequest(data)); + + const repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(data.repository.pullRequest.author.avatarUrl); + + return new PullRequestModel(this, remote, parseGraphQLPullRequest(data), repoReturnsAvatar); } else { let { data } = await octokit.pullRequests.get({ owner: remote.owner, @@ -339,13 +381,15 @@ export class GitHubRepository implements IGitHubRepository, vscode.Disposable { }); Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID); + const repoReturnsAvatar = await this.ensureRepositoryReturnsAvatar(data.user.avatar_url); + if (!data.head.repo) { Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID); return; } let item = convertRESTPullRequestToRawPullRequest(data); - return new PullRequestModel(this, remote, item); + return new PullRequestModel(this, remote, item, repoReturnsAvatar); } } catch (e) { Logger.appendLine(`GithubRepository> Unable to fetch PR: ${e}`); diff --git a/src/github/pullRequestManager.ts b/src/github/pullRequestManager.ts index ae48a8e393..d231eaf425 100644 --- a/src/github/pullRequestManager.ts +++ b/src/github/pullRequestManager.ts @@ -1050,7 +1050,8 @@ export class PullRequestManager { // Create PR let { data } = await repo.octokit.pullRequests.create(params); const item = convertRESTPullRequestToRawPullRequest(data); - const pullRequestModel = new PullRequestModel(repo, repo.remote, item); + const repoReturnsAvatar = await repo.ensureRepositoryReturnsAvatar(item.user.avatarUrl); + const pullRequestModel = new PullRequestModel(repo, repo.remote, item, repoReturnsAvatar); const branchNameSeparatorIndex = params.head.indexOf(':'); const branchName = params.head.slice(branchNameSeparatorIndex + 1); diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 12336856fa..18f52e337d 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -28,25 +28,27 @@ export class PullRequestModel { return this.state === PullRequestStateEnum.Merged; } - public get userAvatar(): string { - if (this.prItem) { + public get userAvatar(): string | undefined { + if (this.prItem && this._repositoryReturnsAvatar) { return this.prItem.user.avatarUrl; } - return ''; + return undefined; } public get userAvatarUri(): vscode.Uri | undefined { if (this.prItem) { let key = this.userAvatar; - let gravatar = vscode.Uri.parse(`${key}&s=${64}`); + if (key) { + let uri = vscode.Uri.parse(`${key}&s=${64}`); - // hack, to ensure queries are not wrongly encoded. - const originalToStringFn = gravatar.toString; - gravatar.toString = function (skipEncoding?: boolean | undefined) { - return originalToStringFn.call(gravatar, true); - }; + // hack, to ensure queries are not wrongly encoded. + const originalToStringFn = uri.toString; + uri.toString = function (skipEncoding?: boolean | undefined) { + return originalToStringFn.call(uri, true); + }; - return gravatar; + return uri; + } } return undefined; @@ -80,7 +82,7 @@ export class PullRequestModel { public head: GitHubRef; public base: GitHubRef; - constructor(public readonly githubRepository: GitHubRepository, public readonly remote: Remote, public prItem: PullRequest) { + constructor(public readonly githubRepository: GitHubRepository, public readonly remote: Remote, public prItem: PullRequest, private _repositoryReturnsAvatar: boolean) { this.update(prItem); } diff --git a/src/github/utils.ts b/src/github/utils.ts index a4b61492f9..17b8d8e731 100644 --- a/src/github/utils.ts +++ b/src/github/utils.ts @@ -35,7 +35,7 @@ export function convertToVSCodeComment(comment: Comment, command: vscode.Command body: new vscode.MarkdownString(comment.body), selectCommand: command, userName: comment.user!.login, - userIconPath: vscode.Uri.parse(comment.user!.avatarUrl), + userIconPath: comment.user && comment.user.avatarUrl ? vscode.Uri.parse(comment.user.avatarUrl) : undefined, label: !!comment.isDraft ? 'Pending' : undefined, commentReactions: comment.reactions ? comment.reactions.map(reaction => { return { label: reaction.label, hasReacted: reaction.viewerHasReacted, count: reaction.count, iconPath: reaction.icon }; diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 15546b3f51..09c1188911 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -171,19 +171,19 @@ const pr: Octokit.PullRequestsGetResponse | Octokit.PullRequestsGetAllResponseIt describe('PullRequestModel', () => { it('should return `state` properly as `open`', () => { - const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest(pr)); + const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest(pr), true); assert.equal(open.state, PullRequestStateEnum.Open); }); it('should return `state` properly as `closed`', () => { - const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, state: 'closed' })); + const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, state: 'closed' }), true); assert.equal(open.state, PullRequestStateEnum.Closed); }); it('should return `state` properly as `merged`', () => { - const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, merged: true, state: 'closed' })); + const open = new PullRequestModel(repo, remote, convertRESTPullRequestToRawPullRequest({ ...pr, merged: true, state: 'closed' }), true); assert.equal(open.state, PullRequestStateEnum.Merged); }); diff --git a/src/view/treeNodes/pullRequestNode.ts b/src/view/treeNodes/pullRequestNode.ts index b887edd0b5..591647411e 100644 --- a/src/view/treeNodes/pullRequestNode.ts +++ b/src/view/treeNodes/pullRequestNode.ts @@ -407,6 +407,8 @@ export class PRNode extends TreeNode implements CommentHandler, vscode.Commentin collapsibleState: 1, contextValue: 'pullrequest' + (this._isLocal ? ':local' : '') + (currentBranchIsForThisPR ? ':active' : ':nonactive'), iconPath: this.pullRequestModel.userAvatarUri + ? this.pullRequestModel.userAvatarUri + : { light: Resource.icons.light.Avatar, dark: Resource.icons.dark.Avatar } }; }