Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1222,12 +1222,20 @@ ${contents}
}),
);

context.subscriptions.push(vscode.commands.registerCommand('review.createSuggestionsFromChanges', async (value: ({ resourceStates: { resourceUri }[] }) | ({ resourceUri: vscode.Uri }), ...additionalSelected: ({ resourceUri: vscode.Uri })[]) => {
interface SCMResourceStates {
resourceStates: { resourceUri: vscode.Uri }[];
}
interface SCMResourceUri {
resourceUri: vscode.Uri;
}
context.subscriptions.push(vscode.commands.registerCommand('review.createSuggestionsFromChanges', async (value: SCMResourceStates | SCMResourceUri, ...additionalSelected: SCMResourceUri[]) => {
let resources: vscode.Uri[];
if ('resourceStates' in value) {
resources = value.resourceStates.map(resource => resource.resourceUri);
const asResourceStates = value as Partial<SCMResourceStates>;
if (asResourceStates.resourceStates) {
resources = asResourceStates.resourceStates.map(resource => resource.resourceUri);
} else {
resources = [value.resourceUri];
const asResourceUri = value as SCMResourceUri;
resources = [asResourceUri.resourceUri];
if (additionalSelected) {
resources.push(...additionalSelected.map(resource => resource.resourceUri));
}
Expand Down
30 changes: 19 additions & 11 deletions src/common/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import { Disposable } from './lifecycle';

export const PR_TREE = 'PullRequestTree';

interface Stringish {
toString: () => string;
}

class Log extends Disposable {
private readonly _outputChannel: vscode.LogOutputChannel;
private readonly _activePerfMarkers: Map<string, number> = new Map();
Expand All @@ -28,36 +32,40 @@ class Log extends Disposable {
this._activePerfMarkers.delete(marker);
}

private logString(message: any, component?: string): string {
private logString(message: string | Error | Stringish | Object, component?: string): string {
let logMessage: string;
if (typeof message !== 'string') {
const asString = message as Partial<Stringish>;
if (message instanceof Error) {
message = message.message;
} else if ('toString' in message) {
message = message.toString();
logMessage = message.message;
} else if (asString.toString) {
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

Checking for toString method existence is similar to the 'in' operator usage that this PR aims to eliminate. Consider using a more explicit type checking approach.

Suggested change
} else if (asString.toString) {
} else if (typeof asString.toString === 'function') {

Copilot uses AI. Check for mistakes.
logMessage = asString.toString();
} else {
message = JSON.stringify(message);
logMessage = JSON.stringify(message);
}
} else {
logMessage = message;
}
return component ? `[${component}] ${message}` : message;
return component ? `[${component}] ${logMessage}` : logMessage;
}

public trace(message: any, component: string) {
public trace(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.trace(this.logString(message, component));
}

public debug(message: any, component: string) {
public debug(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.debug(this.logString(message, component));
}

public appendLine(message: any, component: string) {
public appendLine(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.info(this.logString(message, component));
}

public warn(message: any, component?: string) {
public warn(message: string | Error | Stringish | Object, component?: string) {
this._outputChannel.warn(this.logString(message, component));
}

public error(message: any, component: string) {
public error(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.error(this.logString(message, component));
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/github/activityBarViewProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { formatError } from '../common/utils';
import { getNonce, IRequestMessage, WebviewViewBase } from '../common/webview';
import { ReviewManager } from '../view/reviewManager';
import { FolderRepositoryManager } from './folderRepositoryManager';
import { GithubItemStateEnum, IAccount, isTeam, ITeam, PullRequestMergeability, reviewerId, ReviewEvent, ReviewState } from './interface';
import { GithubItemStateEnum, IAccount, isITeam, ITeam, PullRequestMergeability, reviewerId, ReviewEvent, ReviewState } from './interface';
import { PullRequestModel } from './pullRequestModel';
import { getDefaultMergeMethod } from './pullRequestOverview';
import { PullRequestView } from './pullRequestOverviewCommon';
Expand Down Expand Up @@ -165,9 +165,9 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W
}
}

if (targetReviewer && isTeam(targetReviewer.reviewer)) {
if (targetReviewer && isITeam(targetReviewer.reviewer)) {
teamReviewers.push(targetReviewer.reviewer);
} else if (targetReviewer && !isTeam(targetReviewer.reviewer)) {
} else if (targetReviewer && !isITeam(targetReviewer.reviewer)) {
userReviewers.push(targetReviewer.reviewer);
}

Expand Down
4 changes: 2 additions & 2 deletions src/github/createPRViewProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
titleAndBodyFrom,
} from './folderRepositoryManager';
import { GitHubRepository } from './githubRepository';
import { IAccount, ILabel, IMilestone, IProject, isTeam, ITeam, MergeMethod, RepoAccessAndMergeMethods } from './interface';
import { IAccount, ILabel, IMilestone, IProject, isITeam, ITeam, MergeMethod, RepoAccessAndMergeMethods } from './interface';
import { BaseBranchMetadata, PullRequestGitHelper } from './pullRequestGitHelper';
import { PullRequestModel } from './pullRequestModel';
import { getDefaultMergeMethod } from './pullRequestOverview';
Expand Down Expand Up @@ -310,7 +310,7 @@ export abstract class BaseCreatePullRequestViewProvider<T extends BasePullReques
const users: IAccount[] = [];
const teams: ITeam[] = [];
for (const reviewer of reviewers) {
if (isTeam(reviewer)) {
if (isITeam(reviewer)) {
teams.push(reviewer);
} else {
users.push(reviewer);
Expand Down
6 changes: 4 additions & 2 deletions src/github/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,13 @@ export interface Account extends Actor {
}

export function isAccount(x: Actor | Team | undefined | null): x is Account {
return !!x && 'name' in x && 'email' in x;
const asAccount = x as Partial<Account>;
return !!asAccount && !!asAccount?.name && !!asAccount?.email;
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if an object is an Account is incorrect. It should check if the properties exist (are not undefined), not if they are truthy. An account could have name: '' or email: '', which would make this function return false incorrectly.

Suggested change
return !!asAccount && !!asAccount?.name && !!asAccount?.email;
return !!asAccount && asAccount?.name !== undefined && asAccount?.email !== undefined;

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic requires both name and email to be truthy, but according to the Account interface, email is optional (email?: string). This will incorrectly return false for valid Account objects that don't have an email.

Suggested change
return !!asAccount && !!asAccount?.name && !!asAccount?.email;
return !!asAccount && !!asAccount?.name;

Copilot uses AI. Check for mistakes.
}

export function isTeam(x: Actor | Team | undefined | null): x is Team {
return !!x && 'slug' in x;
const asTeam = x as Partial<Team>;
return !!asTeam && !!asTeam?.slug;
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if an object is a Team is incorrect. It should check if the slug property exists (is not undefined), not if it is truthy. A team could have slug: '', which would make this function return false incorrectly.

Suggested change
return !!asTeam && !!asTeam?.slug;
return !!asTeam && asTeam?.slug !== undefined;

Copilot uses AI. Check for mistakes.
}

export interface Team {
Expand Down
14 changes: 8 additions & 6 deletions src/github/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,26 +105,28 @@ export interface MergeQueueEntry {

export function reviewerId(reviewer: ITeam | IAccount): string {
// We can literally get different login values for copilot depending on where it's coming from (already assignee vs suggested assingee)
return isTeam(reviewer) ? reviewer.id : (reviewer.specialDisplayName ?? reviewer.login);
return isITeam(reviewer) ? reviewer.id : (reviewer.specialDisplayName ?? reviewer.login);
}

export function reviewerLabel(reviewer: ITeam | IAccount | IActor | any): string {
return isTeam(reviewer) ? (reviewer.name ?? reviewer.slug ?? reviewer.id) : (reviewer.specialDisplayName ?? reviewer.login);
return isITeam(reviewer) ? (reviewer.name ?? reviewer.slug ?? reviewer.id) : (reviewer.specialDisplayName ?? reviewer.login);
}

export function isTeam(reviewer: ITeam | IAccount | IActor | any): reviewer is ITeam {
return 'org' in reviewer;
export function isITeam(reviewer: ITeam | IAccount | IActor | any): reviewer is ITeam {
const asITeam = reviewer as Partial<ITeam>;
return !!asITeam.org;
}

export interface ISuggestedReviewer extends IAccount {
isAuthor: boolean;
isCommenter: boolean;
}

export function isSuggestedReviewer(
export function isISuggestedReviewer(
reviewer: IAccount | ISuggestedReviewer | ITeam
): reviewer is ISuggestedReviewer {
return 'isAuthor' in reviewer && 'isCommenter' in reviewer;
const asISuggestedReviewer = reviewer as Partial<ISuggestedReviewer>;
return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter;
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if a reviewer is an ISuggestedReviewer is incorrect. It should check if both properties exist (are not undefined), not if they are truthy. A reviewer could have isAuthor: false and isCommenter: false, which would make this function return false incorrectly.

Suggested change
return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter;
return typeof asISuggestedReviewer.isAuthor !== 'undefined' && typeof asISuggestedReviewer.isCommenter !== 'undefined';

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if an object is an ISuggestedReviewer is incorrect. The function should return true when both properties exist (regardless of their boolean values), but the current implementation requires both to be truthy. This could cause false negatives when isAuthor or isCommenter are explicitly false.

Suggested change
return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter;
return 'isAuthor' in asISuggestedReviewer && 'isCommenter' in asISuggestedReviewer;

Copilot uses AI. Check for mistakes.
}

export interface IProject {
Expand Down
5 changes: 1 addition & 4 deletions src/github/pullRequestGitHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,7 @@ export class PullRequestGitHelper {

static async isRemoteCreatedForPullRequest(repository: Repository, remoteName: string) {
try {
Logger.debug(
`Check if remote '${remoteName}' is created for pull request - start`,
PullRequestGitHelper.ID,
);
Logger.debug(`Check if remote '${remoteName}' is created for pull request - start`, PullRequestGitHelper.ID);
const isForPR = await repository.getConfig(`remote.${remoteName}.${PullRequestRemoteMetadataKey}`);
Logger.debug(`Check if remote '${remoteName}' is created for pull request - end`, PullRequestGitHelper.ID);
return isForPR === 'true';
Expand Down
15 changes: 3 additions & 12 deletions src/github/pullRequestModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1108,20 +1108,14 @@ export class PullRequestModel extends IssueModel<PullRequest> implements IPullRe
commit: OctokitCommon.PullsListCommitsResponseData[0],
): Promise<OctokitCommon.ReposGetCommitResponseFiles> {
try {
Logger.debug(
`Fetch file changes of commit ${commit.sha} in PR #${this.number} - enter`,
PullRequestModel.ID,
);
Logger.debug(`Fetch file changes of commit ${commit.sha} in PR #${this.number} - enter`, PullRequestModel.ID,);
const { octokit, remote } = await this.githubRepository.ensure();
const fullCommit = await octokit.call(octokit.api.repos.getCommit, {
owner: remote.owner,
repo: remote.repositoryName,
ref: commit.sha,
});
Logger.debug(
`Fetch file changes of commit ${commit.sha} in PR #${this.number} - done`,
PullRequestModel.ID,
);
Logger.debug(`Fetch file changes of commit ${commit.sha} in PR #${this.number} - done`, PullRequestModel.ID,);

return fullCommit.data.files ?? [];
} catch (e) {
Expand Down Expand Up @@ -1448,10 +1442,7 @@ export class PullRequestModel extends IssueModel<PullRequest> implements IPullRe
this._onDidChangeChangesSinceReview.fire();
}

Logger.debug(
`Fetch file changes and merge base of PR #${this.number} - done, total files ${files.length} `,
PullRequestModel.ID,
);
Logger.debug(`Fetch file changes and merge base of PR #${this.number} - done, total files ${files.length} `, PullRequestModel.ID,);
return files;
}

Expand Down
14 changes: 7 additions & 7 deletions src/github/pullRequestOverview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { FolderRepositoryManager } from './folderRepositoryManager';
import {
GithubItemStateEnum,
IAccount,
isTeam,
isITeam,
ITeam,
MergeMethod,
MergeMethodsAvailability,
Expand Down Expand Up @@ -382,7 +382,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
quickPick.busy = false;
const acceptPromise: Promise<(IAccount | ITeam)[]> = asPromise<void>(quickPick.onDidAccept).then(() => {
const pickedReviewers: (IAccount | ITeam)[] | undefined = quickPick?.selectedItems.filter(item => item.user).map(item => item.user) as (IAccount | ITeam)[];
const botReviewers = this._existingReviewers.filter(reviewer => !isTeam(reviewer.reviewer) && reviewer.reviewer.accountType === 'Bot').map(reviewer => reviewer.reviewer);
const botReviewers = this._existingReviewers.filter(reviewer => !isITeam(reviewer.reviewer) && reviewer.reviewer.accountType === 'Bot').map(reviewer => reviewer.reviewer);
return pickedReviewers.concat(botReviewers);
});
const hidePromise = asPromise<void>(quickPick.onDidHide);
Expand All @@ -394,15 +394,15 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
const newUserReviewers: IAccount[] = [];
const newTeamReviewers: ITeam[] = [];
allReviewers.forEach(reviewer => {
const newReviewers: (IAccount | ITeam)[] = isTeam(reviewer) ? newTeamReviewers : newUserReviewers;
const newReviewers: (IAccount | ITeam)[] = isITeam(reviewer) ? newTeamReviewers : newUserReviewers;
newReviewers.push(reviewer);
});

const removedUserReviewers: IAccount[] = [];
const removedTeamReviewers: ITeam[] = [];
this._existingReviewers.forEach(existing => {
let newReviewers: (IAccount | ITeam)[] = isTeam(existing.reviewer) ? newTeamReviewers : newUserReviewers;
let removedReviewers: (IAccount | ITeam)[] = isTeam(existing.reviewer) ? removedTeamReviewers : removedUserReviewers;
let newReviewers: (IAccount | ITeam)[] = isITeam(existing.reviewer) ? newTeamReviewers : newUserReviewers;
let removedReviewers: (IAccount | ITeam)[] = isITeam(existing.reviewer) ? removedTeamReviewers : removedUserReviewers;
if (!newReviewers.find(newTeamReviewer => newTeamReviewer.id === existing.reviewer.id)) {
removedReviewers.push(existing.reviewer);
}
Expand Down Expand Up @@ -663,9 +663,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
}
}

if (targetReviewer && isTeam(targetReviewer.reviewer)) {
if (targetReviewer && isITeam(targetReviewer.reviewer)) {
teamReviewers.push(targetReviewer.reviewer);
} else if (targetReviewer && !isTeam(targetReviewer.reviewer)) {
} else if (targetReviewer && !isITeam(targetReviewer.reviewer)) {
userReviewers.push(targetReviewer.reviewer);
}

Expand Down
10 changes: 5 additions & 5 deletions src/github/quickPicks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { DataUri } from '../common/uri';
import { formatError } from '../common/utils';
import { FolderRepositoryManager } from './folderRepositoryManager';
import { GitHubRepository, TeamReviewerRefreshKind } from './githubRepository';
import { AccountType, IAccount, ILabel, IMilestone, IProject, isSuggestedReviewer, isTeam, ISuggestedReviewer, ITeam, reviewerId, ReviewState } from './interface';
import { AccountType, IAccount, ILabel, IMilestone, IProject, isISuggestedReviewer, isITeam, ISuggestedReviewer, ITeam, reviewerId, ReviewState } from './interface';
import { IssueModel } from './issueModel';

async function getItems<T extends IAccount | ITeam | ISuggestedReviewer>(context: vscode.ExtensionContext, skipList: Set<string>, users: T[], picked: boolean, tooManyAssignable: boolean = false): Promise<(vscode.QuickPickItem & { user?: T })[]> {
Expand All @@ -32,7 +32,7 @@ async function getItems<T extends IAccount | ITeam | ISuggestedReviewer>(context
const user = filteredUsers[i];

let detail: string | undefined;
if (isSuggestedReviewer(user)) {
if (isISuggestedReviewer(user)) {
detail = user.isAuthor && user.isCommenter
? vscode.l10n.t('Recently edited and reviewed changes to these files')
: user.isAuthor
Expand All @@ -43,7 +43,7 @@ async function getItems<T extends IAccount | ITeam | ISuggestedReviewer>(context
}

alreadyAssignedItems.push({
label: isTeam(user) ? `${user.org}/${user.slug}` : COPILOT_ACCOUNTS[user.login] ? COPILOT_ACCOUNTS[user.login].name : user.login,
label: isITeam(user) ? `${user.org}/${user.slug}` : COPILOT_ACCOUNTS[user.login] ? COPILOT_ACCOUNTS[user.login].name : user.login,
description: user.name,
user,
picked,
Expand Down Expand Up @@ -120,13 +120,13 @@ export async function getAssigneesQuickPickItems(folderRepositoryManager: Folder
}

function userThemeIcon(user: IAccount | ITeam) {
return (isTeam(user) ? new vscode.ThemeIcon('organization') : new vscode.ThemeIcon('account'));
return (isITeam(user) ? new vscode.ThemeIcon('organization') : new vscode.ThemeIcon('account'));
}

async function getReviewersQuickPickItems(folderRepositoryManager: FolderRepositoryManager, remoteName: string, isInOrganization: boolean, author: IAccount, existingReviewers: ReviewState[],
suggestedReviewers: ISuggestedReviewer[] | undefined, refreshKind: TeamReviewerRefreshKind,
): Promise<(vscode.QuickPickItem & { user?: IAccount | ITeam })[]> {
existingReviewers = existingReviewers.filter(reviewer => isTeam(reviewer.reviewer) || (reviewer.reviewer.accountType !== AccountType.Bot));
existingReviewers = existingReviewers.filter(reviewer => isITeam(reviewer.reviewer) || (reviewer.reviewer.accountType !== AccountType.Bot));
if (!suggestedReviewers) {
return [];
}
Expand Down
39 changes: 32 additions & 7 deletions src/github/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,8 @@ export function convertRESTPullRequestToRawPullRequest(
};

// mergeable is not included in the list response, will need to fetch later
if ('mergeable' in pullRequest) {
item.mergeable = pullRequest.mergeable
if ((pullRequest as OctokitCommon.PullsGetResponseData).mergeable !== undefined) {
item.mergeable = (pullRequest as OctokitCommon.PullsGetResponseData).mergeable
? PullRequestMergeability.Mergeable
: PullRequestMergeability.NotMergeable;
}
Expand Down Expand Up @@ -596,14 +596,39 @@ export interface RestAccount {
type: string;
}

export interface GraphQLAccount {
login: string;
url: string;
avatarUrl: string;
email?: string;
id: string;
name?: string;
__typename: string;
}

export function parseAccount(
author: { login: string; url: string; avatarUrl: string; email?: string, id: string, name?: string, __typename: string } | RestAccount | null,
author: GraphQLAccount | RestAccount | null,
githubRepository?: GitHubRepository,
): IAccount {
if (author) {
const avatarUrl = 'avatarUrl' in author ? author.avatarUrl : author.avatar_url;
const id = 'node_id' in author ? author.node_id : author.id;
const url = 'html_url' in author ? author.html_url : author.url;
let avatarUrl: string;
let id: string;
let url: string;
let accountType: string;
if ((author as RestAccount).html_url) {
const asRestAccount = author as RestAccount;
avatarUrl = asRestAccount.avatar_url;
id = asRestAccount.node_id;
url = asRestAccount.html_url;
accountType = asRestAccount.type;
} else {
const asGraphQLAccount = author as GraphQLAccount;
avatarUrl = asGraphQLAccount.avatarUrl;
id = asGraphQLAccount.id;
url = asGraphQLAccount.url;
accountType = asGraphQLAccount.__typename;
Comment on lines 635 to +651
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

Using property existence to determine the type is not reliable and contradicts the PR's goal of removing 'in' operator usage. Consider using a more explicit type discriminator or a proper type guard function.

Copilot uses AI. Check for mistakes.
}

// In some places, Copilot comes in as a user, and in others as a bot
return {
login: author.login,
Expand All @@ -613,7 +638,7 @@ export function parseAccount(
id,
name: author.name ?? COPILOT_ACCOUNTS[author.login]?.name ?? undefined,
specialDisplayName: COPILOT_ACCOUNTS[author.login] ? (author.name ?? COPILOT_ACCOUNTS[author.login].name) : undefined,
accountType: toAccountType('__typename' in author ? author.__typename : author.type),
accountType: toAccountType(accountType),
};
} else {
return {
Expand Down
6 changes: 2 additions & 4 deletions src/issues/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,9 @@ async function getUpstream(repositoriesManager: RepositoriesManager, repository:
function extractContext(context: LinkContext): { fileUri: vscode.Uri | undefined, lineNumber: number | undefined } {
if (context instanceof vscode.Uri) {
return { fileUri: context, lineNumber: undefined };
} else if (context !== undefined && 'lineNumber' in context && 'uri' in context) {
return { fileUri: context.uri, lineNumber: context.lineNumber };
} else {
return { fileUri: undefined, lineNumber: undefined };
}
const asEditorLineNumberContext = context as Partial<EditorLineNumberContext> | undefined;
return { fileUri: asEditorLineNumberContext?.uri, lineNumber: asEditorLineNumberContext?.lineNumber };
}

function getFileAndPosition(context: LinkContext, positionInfo?: NewIssue): { uri: vscode.Uri | undefined, range: vscode.Range | vscode.NotebookRange | undefined } {
Expand Down
Loading