Conversation
- Refactor `CommentControllerManager` to update comment threads incrementally instead of recreating them, reducing UI flickering. - Optimize GitHub polling in `main.ts` to use `setInterval` with proper cancellation and configuration listeners, replacing recursive `setTimeout`. - Harden `GitHubRepository` against malformed JSON responses from the `gh` CLI. - Fix test setup in `runTest.ts` to use robust `jj new 'root()'` syntax to avoid ambiguous revision errors.
|
I'm currently testing this, so I will put my experiences here (and later provide fixes):
|
sbarfurth
left a comment
There was a problem hiding this comment.
Not a super detailed review since it's a lot of code. thanks for this, it seems really useful!
| "ukemi.githubToken": { | ||
| "type": "string", | ||
| "default": "", | ||
| "description": "GitHub Personal Access Token (PAT) for fetching PR comments. If not set, the gh CLI's default authentication will be used.", | ||
| "scope": "resource" | ||
| }, | ||
| "ukemi.githubTokens": { | ||
| "type": "object", | ||
| "default": {}, | ||
| "description": "A map of GitHub organization names to Personal Access Tokens (PATs). Useful for working across multiple organizations with different auth requirements. PAT tokens need read/write access to Pull requests and Contents.", | ||
| "scope": "resource" | ||
| }, |
There was a problem hiding this comment.
Remove both of these. Storing tokens in VSCode settings is not a good idea. None of the built-in GitHub extensions have this kind of storage. Either we rely entirely on gh CLI or we build a mechanism where the extension has its own OAuth flow.
| }, | ||
| "ukemi.githubPRSearchLimit": { | ||
| "type": "number", | ||
| "default": 10, |
There was a problem hiding this comment.
I think the default should be 1. I'd expect people to have their working copy right on top of the latest branch state. How was 10 chosen?
| }, | ||
| "ukemi.githubPRPollInterval": { | ||
| "type": "number", | ||
| "default": 5, |
There was a problem hiding this comment.
Does that seem like a long time? I always felt the regular GitHub extension was pretty snappy, so this seems like it would be pretty annoying. Just wondering how you came up with the number.
|
|
||
| export class CommentControllerManager { | ||
| private commentController: vscode.CommentController; | ||
| private prCache = new Map<string, GHPR | null>(); // rev -> PR |
There was a problem hiding this comment.
What does it mean for the cache to point to null? Seems like that should just be ejected.
| import { getLogger } from "./logger"; | ||
| import { ChildProcess } from "child_process"; | ||
|
|
||
| export interface GHComment { |
There was a problem hiding this comment.
No need to prefix everything with GH. This is in github.ts, so it's implicitly namespaced.
| } | ||
| } | ||
|
|
||
| private mapToVSCodeComment(ghComment: GHComment): vscode.Comment { |
There was a problem hiding this comment.
Feels like this should be a function in the github.ts file next to the Comment interface
|
|
||
| private displayComments(uri: vscode.Uri, fileComments: GHComment[], threads: GHThreadInfo[], prNumber: number, repoSlug: string, _clearExisting = true) { | ||
| // We group comments by line number because the GitHub REST API returns a flat list of comments. | ||
| // While GraphQL provides thread threads, we currently rely on the REST data for comment content. |
| this.githubRepository.invalidateAuthCache(); | ||
| } | ||
|
|
||
| private displayComments(uri: vscode.Uri, fileComments: GHComment[], threads: GHThreadInfo[], prNumber: number, repoSlug: string, _clearExisting = true) { |
There was a problem hiding this comment.
Make this more usable at the callsite by providing args as an object
| this.githubRepository.invalidateAuthCache(); | ||
| } | ||
|
|
||
| private displayComments(uri: vscode.Uri, fileComments: GHComment[], threads: GHThreadInfo[], prNumber: number, repoSlug: string, _clearExisting = true) { |
There was a problem hiding this comment.
Why the underscore on clear existing?
| // Create or update threads | ||
| for (const [line, lineComments] of threadsByLine) { | ||
| // Sort comments by creation date | ||
| lineComments.sort((a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime()); |
There was a problem hiding this comment.
This sorts in place inside the threadsByLine. That seems confusing to me. Can you sort a copy of the array instead?
GitHub Pull Request Comment Integration
This PR introduces full integration with GitHub Pull Request comments directly within VS Code, allowing users to view, reply to, and resolve comment threads without leaving the editor.
Features
Configuration & Authentication
The extension relies on the
ghCLI for communication with GitHub. You can configure authentication in two ways:1. Default
ghAuthIf you already use
ghon the command line, simply run:gh auth loginThe extension will automatically use your existing
ghcredentials if the environment variable is correctly injected into vscode (or vscode-server) or access is configured through gh's config files.2. Personal Access Token (PAT)
If you prefer to use a specific token or need to support multiple organizations with different credentials, you can configure tokens in your VS Code settings:
ukemi.githubToken: A default PAT for all repositories.ukemi.githubTokens: A map of organization names to PATs (e.g.,{"my-org": "ghp_..."}).Required Access Rights
If using a Personal Access Token (PAT), ensuring the correct scopes is critical for full functionality:
Classic PAT
repo(Full control of private repositories) - Required for all operations.Fine-grained PAT
Pull requests(Read and Write) - Required to create and read comments.Contents(Read and Write) - Required to resolve threads (via GraphQLresolveReviewThreadmutation) and to read file content for diff context.Settings
ukemi.githubPRPollInterval: Polling interval in minutes (default: 5). Set to 0 to disable background polling.ukemi.githubPRSearchLimit: Number of parent commits to search when trying to associate the working copy with a PR (default: 10).