Code discussions #19
Code discussions #19
Conversation
package.json
Outdated
| }, | ||
| "sourcegraph.accessToken": { | ||
| "type": "string", | ||
| "description": "An access token for your account on the Sourcegraph instance configured in `sourcegraph.url`." |
There was a problem hiding this comment.
We should make it clear that this is not required for regular use of the extension.
src/comments.ts
Outdated
| const targetRepo: SourcegraphGQL.IDiscussionThreadTargetRepoInput = { | ||
| repositoryGitCloneURL: remoteUrl, | ||
| path, | ||
| branch, |
There was a problem hiding this comment.
You need to specify a revision here otherwise as soon as someone pushes a new commit to this repository your threads will become detached from the Git history and it will suck for the web UX later on (you won't be able to jump to the exact code that was commented on).
| } | ||
| const input: SourcegraphGQL.IDiscussionThreadCreateInput = { | ||
| title: text, | ||
| contents: text, |
There was a problem hiding this comment.
Either we need to parse the titles out here now (like we do in the webapp), or I need to fix title handling on the backend before we start using this to create discussions on dogfood/prod.
There was a problem hiding this comment.
Yeah this was a todo I forgot about. My preference is for "title" to be optional and for the backend to handle creating the title in that case.
There was a problem hiding this comment.
Yeah I agree, I'll try to make this change tomorrow or over the weekend.
| } | ||
| ${discussionThreadFieldsFragment} | ||
| `, | ||
| { input, relativeRev: branch } |
There was a problem hiding this comment.
relativeRev here should be the exact Git revision the user is viewing, not the branch name. The branch name would return the selection relative to the server's current HEAD revision of the named branch which is not want you want and will make the discussion threads randomly placed in the wrong locations in files if your local Git is behind.
| } | ||
| ${discussionThreadFieldsFragment} | ||
| `, | ||
| { threadID: thread.threadId, contents: text, relativeRev: branch } |
There was a problem hiding this comment.
Same thing here, relativeRev shouldn't be the branch name
| const beforeRange = new vscode.Range(range.start.line - 3, 0, range.start.line - 1, 0) | ||
| const linesBefore = getLines(document, beforeRange) | ||
|
|
||
| const lines = getLines(document, range) |
There was a problem hiding this comment.
I believe the capturing of context lines here is wrong. This is one of the issues I saw in your PR before but I didn't have time to investigate it.
Additionally, before we start using this against dogfood/prod I need to figure out whether or not you should actually be specifying potentially modified context lines here AND specifying an exact Git revision, or if you should be doing something else in that situation. Otherwise, this will leave our data in an inconsistent state and it will be hard to deal with.
There was a problem hiding this comment.
I believe the capturing of context lines here is wrong.
Do you mean the logic is wrong, or do you mean we shouldn't be capturing them at all?
VS Code does have a case that web doesn't have to deal with: unpushed (or even unsaved) modifications. If we only want to support discussing committed/pushed code (which is fine), then there is some work to be done to limit the commenting ranges (as discussed in another comment).
There was a problem hiding this comment.
When I last tested, the logic was wrong in some way (but I forget exactly how. I will check / reconfirm this logic is solid and that we're capturing what I expect here. I'll comment back here once I've done this.
I've been thinking about the issue of unpushed/unsaved modifications, and I see a few options (in no particular order):
- Do not allow commenting on unpushed/unsaved modifications. I assume this would be hard to implement. It would also be unfortunate / not a good long term solution as it would prevent e.g. asking questions about code you are actively developing (something we could immediately support with e.g. email notifications).
- Allow commenting on unpushed/unsaved file modifications, but clearly denote it by e.g. passing two new fields to the backend when we create threads indicating "this is on code that is modified in the editor" and "this is code that is committed to Git, but may not be pushed to the remote". This lets us e.g. warn the user in the web UI. This seems like the most ideal outcome, but I am unsure how hard detecting these two cases would be. For the first flag, we would need to ask VS Code if the file is modified in their editor somehow (even just a single "is modified" boolean would do, we don't need to do individual line ranges). For the second flag, we can detect this on the backend when creating a comment with a Git revision that doesn't exist in the repository to our knowledge yet.
- Don't do anything here yet, and just pass a flag to the backend when creating comments indicating "this comment came from VS Code". This would let us defer the decision until later while not e.g. harming the web experience.
Of the above I think option 2 would be the best, and probably relatively simple to implement.
There was a problem hiding this comment.
Of the above I think option 2 would be the best, and probably relatively simple to implement.
I agree and I wrote similar logic before in the old Sourcegraph Editor comments implementation. I think it reduces to sending the newest commit hash that the selected lines blame to. If this includes uncommitted lines, then the hash is either omitted or uses a sentinel value 00000000 like what git does. If some lines are committed but not pushed, then the web can detect that (e.g. revision won't be found). This gracefully handles comments on commits that aren't yet pushed but will be.
There was a problem hiding this comment.
Using the sentinel value 00000000 SGTM, but we should document this in the GraphQL API since we will only accept one literal string value (e.g. not 000000000000000000000000000000000000000000 or a different number of zeros like Git does). I will document this. Are you aware of any online Git documentation about this value that I can link to? I couldn't find any.
If some lines are committed but not pushed, then the web can detect that (e.g. revision won't be found).
I agree, this is the best approach. It also means zero work here for that case.
TODO:
- @slimsag to update the GraphQL API docs to state the sentinel value
00000000meaning. - @nicksnyder to add the logic in this PR to send
00000000when comment is on uncomitted code, and send correct commit (or00000000"for now" as a hack) when commenting on committed but unpushed code.
To merge, I am fine with something as simple as "If the file is at all modified, we send commit 00000000" but as the web UX improves it would suck if we have lots of discussions created this way, so we should fix this relatively soon / before we start using this a ton.
There was a problem hiding this comment.
Are you aware of any online Git documentation about this value that I can link to?
No, but it is basically the commit hash of only zeros so forty zeros is the "canonical" value (everything else is just an abbreviation like you can do with any commit hash).
Do you prefer this sentinel value over just not sending a value at all (the later might be more intuitive from an API perspective)?
There was a problem hiding this comment.
Jeez, thank you for questioning me on that. This is exactly why I originally made the field nullable, so yeah, we should just send null in that case (we can/should still send the branch name though):
There was a problem hiding this comment.
I came here to ask too if we can just use null. Sentinel values are bad for type safety.
There was a problem hiding this comment.
I left a lot of comments inline, but didn't test this yet.
I am eager to get this merged, too, but I ask that we make sure to address all of my comments here before merging (or get my final approval). I will not block this PR on the GraphQL stuff.
I'll do my best to unblock on the more difficult points that require me to think through how discussions in general handles things, but seriously merging before we address these would be bad (#19 (comment) and #19 (comment))
|
About these two points from your prior PR:
I think refresh logic is not needed. Let's just figure out the current behavior (do I have to close the file and reopen it? close VS Code entirely? etc) and document that somewhere in the README. Not handling local modifications to files is also OK when reading comments, but we will need to figure out what to do about submitting comments on files that are modified and not comitted, this is what my comment #19 (comment) is about. |
| const beforeRange = new vscode.Range(range.start.line - 3, 0, range.start.line - 1, 0) | ||
| const linesBefore = getLines(document, beforeRange) | ||
|
|
||
| const lines = getLines(document, range) |
There was a problem hiding this comment.
I believe the capturing of context lines here is wrong.
Do you mean the logic is wrong, or do you mean we shouldn't be capturing them at all?
VS Code does have a case that web doesn't have to deal with: unpushed (or even unsaved) modifications. If we only want to support discussing committed/pushed code (which is fine), then there is some work to be done to limit the commenting ranges (as discussed in another comment).
| } | ||
| const input: SourcegraphGQL.IDiscussionThreadCreateInput = { | ||
| title: text, | ||
| contents: text, |
There was a problem hiding this comment.
Yeah this was a todo I forgot about. My preference is for "title" to be optional and for the backend to handle creating the title in that case.
|
Any updates on the progress of this feature? |
|
It is likely to factor into the big upcoming code modification feature (https://docs.sourcegraph.com/dev/roadmap#code-modification), but it's all still experimental now. If the code modification stuff describes a problem you have and you want to give input into solution, shoot us an email and we'd love to show you. |
|
Closing this since code discussions were removed from the Sourcegraph backend. |
This is a reboot of #15 with some differences:
I tested by pushing this to a separate extension ID and, it doesn't work (our APIs never get called, unlike in development), but it also doesn't break anything. I asked for guidance on how to get it to work here: microsoft/vscode#53598 (comment)
I think this is mergeable pending any review comments (and I would like to get it merged so when we do figure out what is required to make it work, I can do that quickly).