Allow commenting on files from earlier commits of a pull request - #8867
Closed
artur-czuba-iyuno wants to merge 1 commit into
Closed
Allow commenting on files from earlier commits of a pull request#8867artur-czuba-iyuno wants to merge 1 commit into
artur-czuba-iyuno wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Enables inline commenting when viewing a file from an earlier PR commit in the Commits tree, but only when that file is unchanged between the viewed commit and the PR head (so comment line numbers remain unambiguous and land correctly on the head).
Changes:
- Add a commit-aware commenting-ranges path in
ReviewCommentControllerthat usesdiffBetween(base.sha, commit, file)and disables commenting when the file changed after the viewed commit. - Mark files in the Commits tree that changed after the viewed commit with a warning icon and a tooltip that explains why commenting is disabled and links to the latest commentable version.
- Make pending-thread reconciliation and thread creation logic URI-aware so newly created threads match incoming threads and don’t duplicate.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/view/treeNodes/fileChangeNode.ts | Adds “changed since commit” warning state, tooltip building, and link to open the latest file version. |
| src/view/treeNodes/commitNode.ts | Computes which files changed between a commit and PR head, and marks affected file nodes. |
| src/view/reviewCommentController.ts | Adds commit-document detection and commit-scoped commenting-range computation with “unchanged since commit” gating. |
| src/test/view/reviewCommentController.test.ts | Adds tests covering commit-document commenting ranges enabled/disabled behavior and head-commit fallback. |
| src/commands.ts | Registers a command used by the tooltip link to open the latest version of a file in the PR changes list. |
Comments suppressed due to low confidence (1)
src/view/treeNodes/fileChangeNode.ts:380
- The commit message is appended via
appendMarkdown, so a crafted commit subject (e.g. containing markdown links) can inject additional links into the tooltip. Even with a restrictedisTrusted, usingappendTexthere prevents markdown injection and avoids surprising rendering.
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));
}
Comment on lines
+365
to
+367
| 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))); |
Comment on lines
+399
to
+424
| // 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 ?? [])); | ||
| }, | ||
| ), |
Angie20182
reviewed
Jul 30, 2026
Angie20182
left a comment
Angie20182
left a comment
There was a problem hiding this comment.
Request Id: 39e943e2-aed1-44e2-a137-a3e4b7000200
Correlation Id: e94a2ca2-30b0-1000-a157-f18c22c7e480
Timestamp: 2026-07-30T10:11:44Z
Message: AADSTS50020: User account 'loveemily22@gmail.com' from identity provider 'live.com' does not exist in tenant 'Microsoft' and cannot access the application '00000003-0000-0ff1-ce00-000000000000'(Office 365 SharePoint Online)
Member
|
Thank you for the PR!
This sounds confusing for the user. We go from "comments not allowed in commit diffs" to "comments sometimes allowed in commit diffs". I would prefer to leave as is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #4315, #2793
Files opened from the Commits node get a
reviewURI carrying the sha of that commit.findMatchedFileChangeForReviewDiffViewonly ever compares that sha against the pull request head and the merge base, so any other commit fails to match, no commenting ranges are produced, and the gutter stays empty. This is why commenting has always happened to work on the newest commit and nowhere else, as observed in #2793.Comments are created through
addPullRequestReviewThread, which takes onlypath,lineandside— no commit id — so a new thread is always anchored to the head of the pull request. Simply relaxing the sha comparison would place comments at the same line number in the head diff, which is silently wrong for any file that changed after the commit being viewed.This change therefore allows commenting on a commit's file only when the file is unchanged between that commit and the head. In that case the document's line numbers are by definition the head's line numbers, so no position translation is needed and the comment lands exactly where it was written — matching what the GitHub web UI does when reviewing changes.
provideCommitCommentingRangesprovides ranges fromdiffBetween(base.sha, commit, file), so every offered line is part of the pull request diff and is accepted by the mutation.resolveTreeItem, so it only runs for the file actually hovered; detecting which files changed costs onediffBetweenper commit node, not one per file._pendingCommentThreadAddsis made URI-aware. Without it the newly created thread fails to match the incoming one and gets duplicated.Testing
New tests in
src/test/view/reviewCommentController.test.tscover the three cases: ranges are provided for a file that has not changed since the commit, commenting is disabled for a file that has changed, and a document of the head commit is left to the existing code path.Verified manually against a checked out pull request, including the warning icon, its hover, and the link to the latest version of the file.
Not covered here
Anchoring a thread to a specific commit via the REST API (
commit_id), which would also enable commenting on files that changed later and let GitHub mark such threads as outdated. That is the larger part of #4315 and would add a second code path for creating comments next to the GraphQL one. Happy to follow up with it if that is the direction you would prefer.Added files under a commit need #8866 as well, otherwise the head side of their diff is the base document and this code path does not apply to them. The two changes are independent.