Allow comments to be made in files under the commits tree

[simonmichael]

Issue Type: Bug

Enter Review Mode for a pull request. Expand the Commits tree in the sidebar, open a commit, open one of its files, scroll to a diff hunk. Mouse over the + in the right pane's gutter, or the - in the left pane's gutter. When you do this in a file under the Files tree, you can click to create an inline comment. Under the Commits tree, it is not clickable.

This makes it difficult to review by commit.

Extension version: 0.27.1 VS Code version: Code 1.57.1 (Universal) (507ce72a4466fbb27b715c3722558bb15afa9f48, 2021-06-17T13:28:32.912Z) OS version: Darwin arm64 20.3.0 Restricted Mode: No

System Info

Item	Value
CPUs	Apple M1 (8 x 24)
GPU Status	2d_canvas: enabled gpu_compositing: enabled metal: disabled_off multiple_raster_threads: enabled_on oop_rasterization: enabled opengl: enabled_on rasterization: enabled skia_renderer: disabled_off_ok video_decode: enabled webgl: enabled webgl2: enabled
Load (avg)	2, 3, 3
Memory (System)	16.00GB (0.09GB free)
Process Argv	--crash-reporter-id 02db353d-4821-41f7-a6df-92ed86181e77
Screen Reader	no
VM	0%

[ELLIOTTCABLE]

This one is super-important to my workflow. For large, complex PRs, viewing changes "intermingled" is a nightmare — as long as your collaborators are disciplined, it's much easier to view the changes chronologically, which allows you to follow the development of the changes and stay in sync with the author's intentions along the way.

[tizzyapunkt]

Right now, it's at least possible to add comments while viewing the latest commit and to the files this commit touched. Might be coincidence though.

[PeterBenc]

This one together with #3301 is crucial for my workflow, I am kind of sad to see in on deck or in backlog. Not sure how others review PRs but if you can't review by commits they might as well be squashed, not sure why the Commits view exists at all. I am literally waiting for this one feature to migrate from the web UI. :disappointed:

Edit again: I opened the issue for this in February :( :( I came here hoping it got fixed a long time ago.

[ldelossa]

Yeah. This is basically what I'm waiting for to actually use this as a code review tool.

It's pretty good so far, but not being able to review pr's per-commit and comment on them is a major deal breaker for day to day usage.

[PeterBenc]

@alexr00 sorry for the ping, but I suppose not much else I can do at this point, please consider implementing this as I mentioned, "if can't review by commits they might as well be squashed". The notion of commits, is literally one of the core concepts of git/github so honestly I am not sure what has bigger priority :thinking:

[PeterBenc]

@alexr00 any comments at least? :) do you see this as low prio or should I keep hoping ?

[ldelossa]

@PeterBenc, maybe its time to fork and implement? I'd take it on with you if you have the time/resources.

[ldelossa]

@PeterBenc, maybe its time to fork and implement? I'd take it on with you if you have the time/resources.

[PeterBenc]

@ldelossa Yeah, I was already thinking about it but I was hoping to get at least some kind of response :( I am super busy at least for another month, realistically I ll be able to start looking into it in June. Thank you very much for the offer, :pray: once I start working on it, I ll definitely ping you, or in case you start first, I ll be happy to review or help anyhow.

[alexr00]

@PeterBenc thank you for your patience, I was out of office last week.

I'm sad to say that this issue just hasn't been high enough priority. I've wanted to add it for a while (hence the "On Deck" milestone, which I use for things I want to work on but aren't high enough priority to make it onto an iteration plan), but there's always been something higher priority.

If you're thinking of forking and making the change yourself then please note that I do accept PRs, and if you're interested in contributing then I'm happy to take the time to provide a few code pointers on where to get started with this feature! This change might be a big one though.

[ldelossa]

@alexr00 id be interested in doing a PR to include this, any pointers or guidance on how to accomplish it would be helpful!

[alexr00]

@ldelossa some pointers. I recommending reading to the end before diving in as there's some potentially bad news at the bottom.

This is where we create comments (see the calls to createReviewThread and createCommentReply in particular):

https://github.com/microsoft/vscode-pull-request-github/blob/927e7f155ddd70ced3f15a99e0ef91ea4050aa4f/src/view/reviewCommentController.ts#L676-L712

This is where we actually call the GitHub API to create the review thread:

https://github.com/microsoft/vscode-pull-request-github/blob/927e7f155ddd70ced3f15a99e0ef91ea4050aa4f/src/github/pullRequestModel.ts#L567-L580

And here's the GraphQL mutation that we use:

https://github.com/microsoft/vscode-pull-request-github/blob/927e7f155ddd70ced3f15a99e0ef91ea4050aa4f/src/github/queries.gql#L680-L686

You can see if the GraphQL API has what we need to make comments on a commit over at https://docs.github.com/en/graphql/overview/public-schema. I took a quick look, and I'm not optimitic that's going to be possible with the current GitHub API :(. Here's the input to the API, copied for convenience. Unless the path can somehow take a commit ID we might be stuck.

input AddPullRequestReviewThreadInput {
  """
  Body of the thread's first comment.
  """
  body: String!

  """
  A unique identifier for the client performing the mutation.
  """
  clientMutationId: String

  """
  The line of the blob to which the thread refers, required for line-level
  threads. The end of the line range for multi-line comments.
  """
  line: Int

  """
  Path to the file being commented on.
  """
  path: String!

  """
  The node ID of the pull request reviewing
  """
  pullRequestId: ID @possibleTypes(concreteTypes: ["PullRequest"])

  """
  The Node ID of the review to modify.
  """
  pullRequestReviewId: ID @possibleTypes(concreteTypes: ["PullRequestReview"])

  """
  The side of the diff on which the line resides. For multi-line comments, this is the side for the end of the line range.
  """
  side: DiffSide = RIGHT

  """
  The first line of the range to which the comment refers.
  """
  startLine: Int

  """
  The side of the diff on which the start line resides.
  """
  startSide: DiffSide = RIGHT

  """
  The level at which the comments in the corresponding thread are targeted, can be a diff line or a file
  """
  subjectType: PullRequestReviewThreadSubjectType = LINE
}

[ldelossa]

From my previous work with GH API, a "commit comment" is done with the REST API, not so much graph ql.

    local cmd = string.format([[gh api --method POST -H "Accept: application/vnd.github.v3+json" /repos/{owner}/{repo}/pulls/%d/comments -f commit_id=%s -f path=%s -f side=%s -F position=%d -F line=%d -f body=%s]],
        pull_number,
        commit_sha,
        path,
        side,
        position,
        line,
        body
    )

Also, replies outside of "reviews" are done with REST API:

    local cmd = string.format([[gh api --method POST -H "Accept: application/vnd.github.v3+json" /repos/{owner}/{repo}/pulls/%d/comments/%s/replies -f body=%s]],
        pull_number,
        comment_rest_id,
        body
    )

And replies inside a "review" are done with graphql

M.reply_comment_review = [[
mutation ($pull: ID!, $review: ID!, $commit: GitObjectID!, $body: String!, $reply: ID!) {
  addPullRequestReviewComment(
    input: {pullRequestId: $pull, pullRequestReviewId: $review, commitOID: $commit, body: $body, inReplyTo: $reply}
  ) {
    clientMutationId
  }
}
]]

Unfortunately, in my experience working with GH, you cannot just use graphql or just REST API, at this point, and must jump between the two quite a bit to accomplish feature parity with the web ui.

[alexr00]

We do use both already in this extension. The GraphQL API is just newer and sometimes faster, so we always try to use that. If there's a way to add comments on a commit with the REST API, that's fine, we can use REST for it.

One last code pointer for where we set up allowed commenting ranges: https://github.com/microsoft/vscode-pull-request-github/blob/927e7f155ddd70ced3f15a99e0ef91ea4050aa4f/src/view/reviewCommentController.ts#L407-L410

[ldelossa]

Hey @alexr00 I finally found some time to look at this a bit deeper.

So I do see that when a PR is being initialized all the comments are grabbed and each comment has their commit ID.

I think as a first step, I'd like to get the "per-commit" comments displayed when opening a "per-commit" changed file.

For example when opening this this file in a diff view:

I'd like to display any comments which match the commit id and the file being diffed. Any advice here? I don't need step by step advice, but more, where is the data I need kept that I can do this mapping:

[ clicked file ] --> [ lookup parent until I get to commit id ] --> [ find comments that match this commit-id+file_uri ] --> [paint diff window]

[alexr00]

Comments appear in a file based on whether the resource (uri) of the file matches that of the comment.

Comment threads for commits should already be created here:

https://github.com/microsoft/vscode-pull-request-github/blob/90708caa29d35590cf779ceed2363707cfc43913/src/view/reviewCommentController.ts#L86-L107

The file nodes for a commit are created here:

https://github.com/microsoft/vscode-pull-request-github/blob/90708caa29d35590cf779ceed2363707cfc43913/src/view/treeNodes/commitNode.ts#L61-L97

[ldelossa]

Thanks @alexr00 :)

And just to confirm, does this plugin ever attempt to get "per-commit" comments? Basically like this: https://docs.github.com/en/rest/commits/comments?apiVersion=2022-11-28

If not, are they obtained another way, for each commit in the PR?

[alexr00]

@ldelossa no, we never try to do that. We don't try to get comments per commit, rather we get all the comments associated with the PR, some of which may be out of date (associated with a specific commit).

[ldelossa]

Thanks! I think commit comments are a "nice to have" but thats a problem for later, and is only relevant to show comments on the commit itself (not the diff). Just wanted to see if they were grabbed at all.

[ldelossa]

@alexr00 i noticed this ticket has been moving from milestone to milestone but is now "on deck"

i dont currently have the resources to work on this. Are there still plans to address this in the next milestone?

[alexr00]

No plans to address this right now. I use "on deck" to mean "if I have time after my planned work then this is what I will work on".