From b44b1eca80f71f0fb38a5b3bd1887ff8a0eeadb9 Mon Sep 17 00:00:00 2001 From: Artur Czuba Date: Thu, 30 Jul 2026 12:10:53 +0200 Subject: [PATCH] Allow commenting on files from earlier commits of a pull request Files opened from the Commits node get a `review` uri carrying the sha of that commit, but `findMatchedFileChangeForReviewDiffView` only compares that sha against the pull request head and the merge base. Any other commit fails to match, so no commenting ranges are produced and the gutter stays empty. This is why commenting has always happened to work on the newest commit only. Comments are created through `addPullRequestReviewThread`, which takes a path, a line and a side but no commit id, so a new thread is always anchored to the head of the pull request. Relaxing the sha comparison alone would place comments at the same line number in the head diff, which is silently wrong for a file that changed after the commit being viewed. Commenting on a commit's file is therefore allowed only while the file is unchanged between that commit and the head. The line numbers of the document are then the head's line numbers, so no position translation is needed and the comment lands where it was written. - `provideCommitCommentingRanges` takes the ranges from the base of the pull request against the commit, so every offered line is part of the pull request diff and is accepted by the mutation. - When the file has changed since the commit, commenting is disabled and the file is marked with a warning icon in the Commits tree. The hover explains why, names the most recent commit that touched the file, and links to the version of the file that can be commented on. The git lookup for the hover runs in `resolveTreeItem` so it only happens for the file being hovered, and detecting which files changed costs one `diffBetween` per commit node rather than one per file. - Thread reconciliation in `_pendingCommentThreadAdds` is made uri aware. Without it a thread created this way fails to match and gets duplicated. Co-Authored-By: Claude Opus 5 (1M context) --- src/commands.ts | 28 +++++ src/test/view/reviewCommentController.test.ts | 83 +++++++++++++ src/view/reviewCommentController.ts | 111 ++++++++++++++++-- src/view/treeNodes/commitNode.ts | 33 ++++++ src/view/treeNodes/fileChangeNode.ts | 59 +++++++++- 5 files changed, 306 insertions(+), 8 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 79f4303e59..bc55ed0eea 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -396,6 +396,34 @@ export function registerCommands( ), ); + // Used from the tooltip of a file that has changed since the commit being viewed, to get to the + // version of that file that can actually be commented on. + context.subscriptions.push( + vscode.commands.registerCommand( + 'pr.openCurrentFileChange', + async (args: { rootPath: string; fileName: string } | undefined) => { + if (!args?.fileName) { + return; + } + + const reviewManager = reviewsManager.reviewManagers.find( + manager => manager.repository.rootUri.path === args.rootPath, + ); + const fileChange = reviewManager?.reviewModel.localFileChanges.find( + change => change.fileName === args.fileName, + ); + if (!fileChange) { + return vscode.window.showInformationMessage( + vscode.l10n.t('{0} is not part of the changes of the checked out pull request.', args.fileName), + ); + } + + await fileChange.resolve(); + return vscode.commands.executeCommand(fileChange.command.command, ...(fileChange.command.arguments ?? [])); + }, + ), + ); + context.subscriptions.push( vscode.commands.registerCommand('pr.deleteLocalBranch', async (e: PRNode) => { const folderManager = reposManager.getManagerForIssueModel(e.pullRequestModel); diff --git a/src/test/view/reviewCommentController.test.ts b/src/test/view/reviewCommentController.test.ts index 12e7e87a32..37b07ae8dd 100644 --- a/src/test/view/reviewCommentController.test.ts +++ b/src/test/view/reviewCommentController.test.ts @@ -356,6 +356,89 @@ describe('ReviewCommentController', function () { }); }); + describe('provideCommentingRanges', function () { + const fileName = 'data/products.json'; + const commitSha = '1111111111111111111111111111111111111111'; + const patch = ['@@ -1,3 +1,4 @@', ' line one', ' line two', '+added line', ' line three'].join('\n'); + + function createController(): TestReviewCommentController { + return new TestReviewCommentController( + reviewManager, + manager, + repository, + new ReviewModel(), + gitApiImpl, + telemetry, + ); + } + + /** + * A document as opened from the Commits tree: a `review` uri holding the sha of that commit. + */ + function createCommitDocument(commit: string): vscode.TextDocument { + const uri = toReviewUri( + vscode.Uri.parse(`commit~11111111/${fileName}`), + fileName, + undefined, + commit, + true, + { base: false }, + repository.rootUri, + ); + return { uri, lineCount: 4 } as vscode.TextDocument; + } + + function stubDiffBetween(changedSinceCommit: string) { + const diffBetween = sinon.stub(repository, 'diffBetween'); + // Whether the file changed between the commit being viewed and the head of the PR. + diffBetween.withArgs(commitSha, activePullRequest.head!.sha, fileName).returns(Promise.resolve(changedSinceCommit)); + // The diff the commenting ranges are taken from. + diffBetween.withArgs(activePullRequest.base.sha, commitSha, fileName).returns(Promise.resolve(patch)); + return diffBetween; + } + + it('provides ranges for a file that has not changed since the commit', async function () { + stubDiffBetween(''); + + const result = await createController().provideCommentingRanges( + createCommitDocument(commitSha), + new vscode.CancellationTokenSource().token, + ) as { enableFileComments: boolean; ranges?: vscode.Range[] }; + + assert.ok(result, 'ranges should be provided for a file that can be commented on'); + assert.strictEqual(result.enableFileComments, true); + assert.strictEqual(result.ranges?.length, 1); + assert.strictEqual(result.ranges![0].start.line, 0); + assert.strictEqual(result.ranges![0].end.line, 3); + }); + + it('does not allow commenting on a file that changed after the commit', async function () { + // Comments always go to the head of the pull request, so the line numbers of this + // document can no longer be trusted to point at the same code. + stubDiffBetween(patch); + + const result = await createController().provideCommentingRanges( + createCommitDocument(commitSha), + new vscode.CancellationTokenSource().token, + ) as { enableFileComments: boolean; ranges?: vscode.Range[] }; + + assert.ok(result); + assert.strictEqual(result.enableFileComments, false); + assert.deepStrictEqual(result.ranges, []); + }); + + it('leaves a document of the head commit to the regular code path', async function () { + const diffBetween = stubDiffBetween(''); + + await createController().provideCommentingRanges( + createCommitDocument(activePullRequest.head!.sha), + new vscode.CancellationTokenSource().token, + ); + + assert.ok(diffBetween.notCalled, 'the head commit is handled by matching against the local file changes'); + }); + }); + describe('_findMatchingThread', function () { it('returns the moved thread when an existing workspace thread becomes outdated', function () { const fileName = 'data/products.json'; diff --git a/src/view/reviewCommentController.ts b/src/view/reviewCommentController.ts index 36c0641567..44ff5b540f 100644 --- a/src/view/reviewCommentController.ts +++ b/src/view/reviewCommentController.ts @@ -15,6 +15,7 @@ import { ReviewManager } from './reviewManager'; import { ReviewModel } from './reviewModel'; import { DiffSide, IReviewThread, SubjectType } from '../common/comment'; import { getCommentingRanges } from '../common/commentingRanges'; +import { parsePatch } from '../common/diffHunk'; import { mapNewPositionToOld, mapOldPositionToNew } from '../common/diffPositionMapping'; import { commands, contexts } from '../common/executeCommands'; import { GitChangeType, InMemFileChange } from '../common/file'; @@ -292,13 +293,20 @@ export class ReviewCommentController extends CommentControllerBase implements Co } const index = await arrayFindIndexAsync(this._pendingCommentThreadAdds, async t => { - const fileName = this._folderRepoManager.gitRelativeRootPath(t.uri.path); + const prCommitDocument = this.prCommitDocument(t.uri); + const fileName = prCommitDocument?.fileName ?? this._folderRepoManager.gitRelativeRootPath(t.uri.path); if (fileName !== thread.path) { return false; } - const diff = await this.getContentDiff(t.uri, fileName); - const line = t.range ? mapNewPositionToOld(diff, t.range.end.line) : 0; + let line = 0; + if (t.range) { + if (prCommitDocument) { + line = t.range.end.line; + } else { + line = mapNewPositionToOld(await this.getContentDiff(t.uri, fileName), t.range.end.line); + } + } const sameLine = line + 1 === thread.endLine; return sameLine; }); @@ -513,6 +521,11 @@ export class ReviewCommentController extends CommentControllerBase implements Co Logger.debug('Found matched file for commenting ranges.', ReviewCommentController.ID); return { ranges: getCommentingRanges(await matchedFile.changeModel.diffHunks(), query.base, ReviewCommentController.ID), enableFileComments: true }; } + + const prCommitDocument = this.prCommitDocument(document.uri); + if (prCommitDocument) { + return this.provideCommitCommentingRanges(prCommitDocument.commit, prCommitDocument.fileName); + } } const bestRepoForFile = getRepositoryForFile(this._gitApi, document.uri); @@ -571,6 +584,78 @@ export class ReviewCommentController extends CommentControllerBase implements Co // #endregion + /** + * Commenting ranges for a file shown as of one of the pull request's commits. The ranges are + * taken from the diff of the pull request base against that commit, so the line numbers are the + * ones of the document being shown, and every offered line is part of the pull request's diff. + */ + private async provideCommitCommentingRanges(commit: string, fileName: string): Promise<{ enableFileComments: boolean; ranges?: vscode.Range[] } | undefined> { + const activePullRequest = this._folderRepoManager.activePullRequest; + if (!activePullRequest) { + return; + } + + if (!(await this.isUnchangedSinceCommit(commit, fileName))) { + Logger.debug(`No commenting ranges: ${fileName} has changed since commit ${commit.substring(0, 8)}.`, ReviewCommentController.ID); + return { ranges: [], enableFileComments: false }; + } + + try { + const patch = await this._repository.diffBetween(activePullRequest.base.sha, commit, fileName); + const ranges = getCommentingRanges(parsePatch(patch), false, ReviewCommentController.ID); + Logger.debug(`Providing ${ranges.length} commenting ranges for ${fileName} at commit ${commit.substring(0, 8)}.`, ReviewCommentController.ID); + return { ranges, enableFileComments: true }; + } catch (e) { + Logger.error(`Failed to get commenting ranges for ${fileName} at commit ${commit.substring(0, 8)}: ${formatError(e)}`, ReviewCommentController.ID); + return; + } + } + + /** + * Detects a `review` document that shows a file as of one of the pull request's commits, as + * opened from the Commits node of the tree. Returns `undefined` for every other document, + * including the diff of the head commit, which is already handled by the regular code paths. + */ + private prCommitDocument(uri: vscode.Uri): { commit: string; fileName: string } | undefined { + if (uri.scheme !== Schemes.Review || !uri.query) { + return; + } + + let query: ReviewUriParams; + try { + query = fromReviewUri(uri.query); + } catch (e) { + return; + } + + const headSha = this._folderRepoManager.activePullRequest?.head?.sha; + if (!query.commit || !query.path || query.base || !headSha || query.commit === headSha) { + return; + } + + return { commit: query.commit, fileName: query.path }; + } + + /** + * Comments are always created against the head of the pull request, so a comment made while + * looking at an older commit is only unambiguous when the file hasn't been touched since. When + * that holds, the line numbers of the document match the head and need no translation. + */ + private async isUnchangedSinceCommit(commit: string, fileName: string): Promise { + const headSha = this._folderRepoManager.activePullRequest?.head?.sha; + if (!headSha) { + return false; + } + + try { + const diff = await this._repository.diffBetween(commit, headSha, fileName); + return diff.length === 0; + } catch (e) { + Logger.warn(`Unable to tell whether ${fileName} changed between ${commit} and ${headSha}: ${formatError(e)}`, ReviewCommentController.ID); + return false; + } + } + private async getContentDiff(uri: vscode.Uri, fileName: string, retry: boolean = true): Promise { const matchedEditor = vscode.window.visibleTextEditors.find( editor => editor.document.uri.toString() === uri.toString(), @@ -686,7 +771,8 @@ export class ReviewCommentController extends CommentControllerBase implements Co try { temporaryCommentId = await this.optimisticallyAddComment(thread, input, true); if (!hasExistingComments) { - const fileName = this._folderRepoManager.gitRelativeRootPath(thread.uri.path); + const prCommitDocument = this.prCommitDocument(thread.uri); + const fileName = prCommitDocument?.fileName ?? this._folderRepoManager.gitRelativeRootPath(thread.uri.path); const side = this.getCommentSide(thread); this._pendingCommentThreadAdds.push(thread); @@ -695,7 +781,12 @@ export class ReviewCommentController extends CommentControllerBase implements Co let startLine: number | undefined = undefined; let endLine: number | undefined = undefined; if (thread.range) { - if (side === DiffSide.RIGHT) { + if (prCommitDocument) { + // Commenting is only offered on a commit's document when the file is unchanged + // between that commit and the head, so the lines already match the head diff. + startLine = thread.range.start.line; + endLine = thread.range.end.line; + } else if (side === DiffSide.RIGHT) { const diff = await this.getContentDiff(thread.uri, fileName); startLine = mapNewPositionToOld(diff, thread.range.start.line); endLine = mapNewPositionToOld(diff, thread.range.end.line); @@ -797,7 +888,8 @@ export class ReviewCommentController extends CommentControllerBase implements Co try { if (!hasExistingComments) { - const fileName = this._folderRepoManager.gitRelativeRootPath(thread.uri.path); + const prCommitDocument = this.prCommitDocument(thread.uri); + const fileName = prCommitDocument?.fileName ?? this._folderRepoManager.gitRelativeRootPath(thread.uri.path); this._pendingCommentThreadAdds.push(thread); const side = this.getCommentSide(thread); @@ -806,7 +898,12 @@ export class ReviewCommentController extends CommentControllerBase implements Co let startLine: number | undefined = undefined; let endLine: number | undefined = undefined; if (thread.range) { - if (side === DiffSide.RIGHT) { + if (prCommitDocument) { + // Commenting is only offered on a commit's document when the file is unchanged + // between that commit and the head, so the lines already match the head diff. + startLine = thread.range.start.line; + endLine = thread.range.end.line; + } else if (side === DiffSide.RIGHT) { const diff = await this.getContentDiff(thread.uri, fileName); startLine = mapNewPositionToOld(diff, thread.range.start.line); endLine = mapNewPositionToOld(diff, thread.range.end.line); diff --git a/src/view/treeNodes/commitNode.ts b/src/view/treeNodes/commitNode.ts index ee95b34a12..3c0b208e5c 100644 --- a/src/view/treeNodes/commitNode.ts +++ b/src/view/treeNodes/commitNode.ts @@ -51,6 +51,33 @@ export class CommitNode extends TreeNode implements vscode.TreeItem { return this; } + /** + * Files that have been modified between this commit and the head of the pull request. Comments + * can't be added to those from this view, since comments always go to the head. One git call for + * the whole commit, rather than one per file. + */ + private async getFilesChangedSinceCommit(): Promise> { + const changedFiles = new Set(); + const headSha = this.pullRequest.head?.sha; + if (!headSha || headSha === this.commit.sha) { + return changedFiles; + } + + try { + const changes = await this.pullRequestManager.repository.diffBetween(this.commit.sha, headSha); + for (const change of changes) { + changedFiles.add(this.pullRequestManager.gitRelativeRootPath(change.uri.path)); + if (change.originalUri) { + changedFiles.add(this.pullRequestManager.gitRelativeRootPath(change.originalUri.path)); + } + } + } catch (e) { + // Without this the file simply doesn't get marked, which is the pre-existing behavior. + } + + return changedFiles; + } + override async getChildren(): Promise { super.getChildren(); const fileChanges = (await this.pullRequest.getCommitChangedFiles(this.commit)) ?? []; @@ -59,6 +86,8 @@ export class CommitNode extends TreeNode implements vscode.TreeItem { return [new LabelOnlyNode(this, 'No changed files')]; } + const changedSinceCommit = await this.getFilesChangedSinceCommit(); + const fileChangeNodes = fileChanges.map(change => { const fileName = change.filename!; const uri = reviewPath(fileName, this.commit.sha); @@ -99,6 +128,10 @@ export class CommitNode extends TreeNode implements vscode.TreeItem { fileChangeNode.useViewChangesCommand(); + if (changedSinceCommit.has(fileName)) { + fileChangeNode.markChangedSinceCommit(this.commit.sha); + } + return fileChangeNode; }); diff --git a/src/view/treeNodes/fileChangeNode.ts b/src/view/treeNodes/fileChangeNode.ts index 4e1e8ac0c6..038784ad93 100644 --- a/src/view/treeNodes/fileChangeNode.ts +++ b/src/view/treeNodes/fileChangeNode.ts @@ -93,7 +93,7 @@ export class FileChangeNode extends TreeNode implements vscode.TreeItem { return this.changeModel.sha; } - get tooltip(): string { + get tooltip(): string | vscode.MarkdownString | undefined { return this.resourceUri.fsPath; } @@ -339,6 +339,59 @@ export class GitFileChangeNode extends FileChangeNode implements vscode.TreeItem this._useViewChangesCommand = true; } + private _changedSinceCommit: string | undefined; + private _changedSinceTooltip: vscode.MarkdownString | undefined; + + /** + * Marks this file as one that has been modified between the commit being viewed and the head of + * the pull request. Comments always land on the head, so they can't be placed from this view. + */ + public markChangedSinceCommit(commit: string) { + this._changedSinceCommit = commit; + this.iconPath = new vscode.ThemeIcon('warning', new vscode.ThemeColor('problemsWarningIcon.foreground')); + } + + override get tooltip(): string | vscode.MarkdownString | undefined { + if (!this._changedSinceCommit) { + return super.tooltip; + } + + // Returning undefined until the tooltip has been built makes VS Code call `resolveTreeItem`, + // so the git lookup only happens for the file that is actually hovered. + return this._changedSinceTooltip; + } + + private async buildChangedSinceTooltip(commit: string): Promise { + const tooltip = new vscode.MarkdownString(undefined, true); + tooltip.isTrusted = true; + tooltip.appendMarkdown(vscode.l10n.t('$(warning) `{0}` has changed since commit {1}. Comments are always added to the latest version of a pull request, so they cannot be added from this view.', this.fileName, commit.substring(0, 8))); + + const headSha = this.pullRequest.head?.sha; + if (headSha) { + try { + const log = await this.pullRequestManager.repository.log({ + path: vscode.Uri.joinPath(this.pullRequestManager.repository.rootUri, this.fileName).fsPath, + range: `${commit}..${headSha}`, + maxEntries: 1, + }); + if (log.length) { + const message = log[0].message.split('\n')[0]; + tooltip.appendMarkdown('\n\n' + vscode.l10n.t('Last changed in {0}: {1}', log[0].hash.substring(0, 8), message)); + } + } catch (e) { + // The commit may not be available locally. The explanation above is still useful. + } + } + + const args = encodeURIComponent(JSON.stringify({ + rootPath: this.pullRequestManager.repository.rootUri.path, + fileName: this.fileName, + })); + tooltip.appendMarkdown(`\n\n[${vscode.l10n.t('Open the latest version of this file')}](command:pr.openCurrentFileChange?${args})`); + + return tooltip; + } + private async alternateCommand(): Promise { if (this.status === GitChangeType.DELETE || this.status === GitChangeType.ADD) { // create an empty `review` uri without any path/commit info. @@ -403,6 +456,10 @@ export class GitFileChangeNode extends FileChangeNode implements vscode.TreeItem } async resolve(): Promise { + if (this._changedSinceCommit && !this._changedSinceTooltip) { + this._changedSinceTooltip = await this.buildChangedSinceTooltip(this._changedSinceCommit); + } + if (this._useViewChangesCommand) { this.command = await this.alternateCommand(); } else {