Remove uses of property existence with `in` #6932

alexr00 · 2025-05-08T14:59:16Z

No description provided.

Copilot

Pull Request Overview

This PR refactors the codebase to remove uses of property existence checking with the in operator and replaces them with type-safe alternatives using explicit type casting and nullish coalescing operators. The changes improve type safety by avoiding runtime property existence checks that can be unreliable.

Key changes include:

Replacing in operator checks with explicit type casting and property access using nullish coalescing
Renaming type guard functions to be more explicit (e.g., isTeam → isITeam, isSuggestedReviewer → isISuggestedReviewer)
Adding proper TypeScript interfaces for better type safety in command parameters and logger methods

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
webviews/createPullRequestViewNew/app.tsx	Updates import and usage of renamed `isITeam` function
webviews/components/reviewer.tsx	Updates import and usage of renamed `isITeam` function
webviews/components/comment.tsx	Replaces `in` operator checks with type casting and nullish coalescing
src/notifications/notificationsFeatureRegistar.ts	Adds proper typing and replaces `in` operator with type casting
src/notifications/notificationItem.ts	Adds new `NotificationID` interface
src/issues/util.ts	Replaces `in` operator checks with type casting approach
src/github/utils.ts	Replaces `in` operator with explicit type checking and adds interface
src/github/quickPicks.ts	Updates imports and usage of renamed type guard functions
src/github/pullRequestOverview.ts	Updates import and usage of renamed `isITeam` function
src/github/pullRequestModel.ts	Reformats logger calls (cosmetic changes)
src/github/pullRequestGitHelper.ts	Reformats logger calls (cosmetic changes)
src/github/interface.ts	Renames and refactors type guard functions with proper type casting
src/github/graphql.ts	Updates type guard functions to use explicit type casting
src/github/createPRViewProvider.ts	Updates import and usage of renamed `isITeam` function
src/github/activityBarViewProvider.ts	Updates import and usage of renamed `isITeam` function
src/common/logger.ts	Adds proper typing and replaces `in` operator with type casting
src/commands.ts	Adds interfaces and replaces `in` operator with type casting

Copilot · 2025-07-17T13:46:52Z

src/github/interface.ts

 	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;


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 · 2025-07-17T13:46:52Z

src/github/graphql.ts

@@ -113,11 +113,13 @@ export interface Account extends Actor {
 }

 export function isAccount(x: Actor | Team | Node | 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;


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 · 2025-07-17T13:46:53Z

webviews/components/comment.tsx

-	const author = ('user' in comment) ? comment.user! : (comment as PullRequest).author!;
-	const createdAt = ('createdAt' in comment) ? comment.createdAt : (comment as ReviewEvent).submittedAt;
+	const author = asNotPullRequest.user ?? (comment as PullRequest).author;
+	const createdAt = (comment as IComment | CommentEvent | PullRequest).createdAt ?? (comment as ReviewEvent).submittedAt;


The type casting is inconsistent and potentially unsafe. The first cast includes PullRequest but the fallback assumes ReviewEvent. Consider using a more systematic approach to handle the different comment types.

Suggested change

const createdAt = (comment as IComment | CommentEvent | PullRequest).createdAt ?? (comment as ReviewEvent).submittedAt;

let createdAt: string | undefined;

if ('createdAt' in comment) {

createdAt = comment.createdAt;

} else if (isReviewEvent(comment)) {

createdAt = comment.submittedAt;

}

Copilot · 2025-07-17T13:46:53Z

src/github/utils.ts

 	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;


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 · 2025-07-17T13:46:53Z

src/common/logger.ts

-			} else if ('toString' in message) {
-				message = message.toString();
+				logMessage = message.message;
+			} else if (asString.toString) {


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') {

Remove uses of property existence with in

28cf23a

alexr00 self-assigned this May 8, 2025

vs-code-engineering bot added this to the May 2025 milestone May 8, 2025

karthiknadig previously approved these changes May 8, 2025

View reviewed changes

alexr00 enabled auto-merge (squash) May 8, 2025 16:40

Merge branch 'main' into alexr00/particular-bug

7eb1133

alexr00 dismissed karthiknadig’s stale review via 7eb1133 May 9, 2025 08:41

alexr00 force-pushed the alexr00/particular-bug branch from 33c41cb to 7eb1133 Compare May 9, 2025 09:08

alexr00 assigned alexr00 and unassigned alexr00 Jun 19, 2025

alexr00 assigned Copilot and unassigned Copilot Jul 17, 2025

alexr00 requested review from karthiknadig and Copilot and removed request for Copilot July 17, 2025 13:30

This comment was marked as outdated.

Sign in to view

alexr00 requested review from Copilot and removed request for Copilot July 17, 2025 13:45

Copilot AI reviewed Jul 17, 2025

View reviewed changes

Merge branch 'main' into alexr00/particular-bug

4f91735

aeschli approved these changes Aug 11, 2025

View reviewed changes

alexr00 merged commit 46bbef3 into main Aug 11, 2025
4 checks passed

alexr00 deleted the alexr00/particular-bug branch August 11, 2025 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove uses of property existence with `in` #6932

Remove uses of property existence with `in` #6932

Uh oh!

alexr00 commented May 8, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 17, 2025

Uh oh!

Copilot AI Jul 17, 2025

Uh oh!

Copilot AI Jul 17, 2025

Uh oh!

Copilot AI Jul 17, 2025

Uh oh!

Copilot AI Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

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

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

-	const createdAt = (comment as IComment | CommentEvent | PullRequest).createdAt ?? (comment as ReviewEvent).submittedAt;
+	let createdAt: string | undefined;
+	if ('createdAt' in comment) {
+		createdAt = comment.createdAt;
+	} else if (isReviewEvent(comment)) {
+		createdAt = comment.submittedAt;
+	}

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

Remove uses of property existence with in #6932

Remove uses of property existence with in #6932

Uh oh!

Conversation

alexr00 commented May 8, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Remove uses of property existence with `in` #6932

Remove uses of property existence with `in` #6932