Remove uses of property existence with in - #6932
Conversation
33c41cb to
7eb1133
Compare
There was a problem hiding this comment.
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
inoperator 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 |
| ): reviewer is ISuggestedReviewer { | ||
| return 'isAuthor' in reviewer && 'isCommenter' in reviewer; | ||
| const asISuggestedReviewer = reviewer as Partial<ISuggestedReviewer>; | ||
| return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter; |
There was a problem hiding this comment.
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.
| return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter; | |
| return 'isAuthor' in asISuggestedReviewer && 'isCommenter' in asISuggestedReviewer; |
| 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; |
There was a problem hiding this comment.
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.
| return !!asAccount && !!asAccount?.name && !!asAccount?.email; | |
| return !!asAccount && !!asAccount?.name; |
| 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; |
There was a problem hiding this comment.
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.
| 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; | |
| } |
| 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; |
There was a problem hiding this comment.
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.
| } else if ('toString' in message) { | ||
| message = message.toString(); | ||
| logMessage = message.message; | ||
| } else if (asString.toString) { |
There was a problem hiding this comment.
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.
| } else if (asString.toString) { | |
| } else if (typeof asString.toString === 'function') { |
No description provided.