Skip to content
This repository was archived by the owner on Mar 10, 2022. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"type": "extensionHost",
"request": "launch",
"runtimeExecutable": "${execPath}",
"args": ["--extensionDevelopmentPath=${workspaceRoot}"],
"args": ["--extensionDevelopmentPath=${workspaceRoot}", "--enable-proposed-api=sourcegraph.sourcegraph"],
"stopOnEntry": false,
"sourceMaps": true,
"outFiles": ["${workspaceRoot}/out/**/*.js"],
Expand Down
26 changes: 19 additions & 7 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@
"type": "git",
"url": "https://github.com/sourcegraph/sourcegraph-vscode.git"
},
"enableProposedApi": true,
Comment thread
emidoots marked this conversation as resolved.
"engines": {
"vscode": "^1.11.0"
"vscode": "^1.27.1"
},
"categories": [
"Other"
],
"activationEvents": [
"onCommand:extension.open",
"onCommand:extension.search"
"*"
],
"main": "./out/extension",
"contributes": {
Expand Down Expand Up @@ -51,11 +51,18 @@
"title": "Sourcegraph extension configuration",
"properties": {
"sourcegraph.url": {
"type": [
"string"
],
"type": "string",
"default": "https://sourcegraph.com",
"description": "The base URL of the Sourcegraph instance to use."
},
"sourcegraph.accessToken": {
"type": "string",
"description": "An access token for your account on the Sourcegraph instance configured in `sourcegraph.url`. You do not need to provide an access token unless you have `\"sourcegraph.discussionsPreviewEnabled\": true` in your settings."
},
"sourcegraph.discussionsPreviewEnabled": {
"type": "boolean",
"default": false,
"description": "Enables Sourcegraph discussions (preview)"
}
}
}
Expand Down Expand Up @@ -90,7 +97,8 @@
"build": "tsc -p .",
"watch": "tsc -w -p .",
"postinstall": "node ./node_modules/vscode/bin/install",
"test": "node ./node_modules/vscode/bin/test"
"test": "node ./node_modules/vscode/bin/test",
"graphql": "get-graphql-schema https://sourcegraph.com/.api/graphql --json | gql2ts -n SourcegraphGQL -o src/typings/SourcegraphGQL.d.ts && prettier src/typings/SourcegraphGQL.d.ts --write"
},
"devDependencies": {
"@commitlint/cli": "^7.0.0",
Expand All @@ -101,7 +109,10 @@
"@types/execa": "^0.9.0",
"@types/mocha": "^2.2.32",
"@types/node": "^10.3.3",
"@types/node-fetch": "^2.1.2",
"@types/opn": "^5.1.0",
"get-graphql-schema": "^2.1.1",
"gql2ts": "^1.8.2",
"husky": "^0.14.3",
"mocha": "^5.2.0",
"prettier": "1.13.7",
Expand All @@ -114,6 +125,7 @@
},
"dependencies": {
"execa": "^0.6.3",
"node-fetch": "^2.2.0",
"opn": "^5.0.0"
}
}
299 changes: 299 additions & 0 deletions src/comments.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,299 @@
import vscode from 'vscode'
import { repoInfo } from './git'
import { gql, mutateGraphQL, queryGraphQL } from './graphql'
import { log } from './log'

export function activateComments(context: vscode.ExtensionContext): void {
if (!vscode.workspace.getConfiguration('sourcegraph').get<string>('discussionsPreviewEnabled')) {
return
}
log.appendLine('Discussions preview is enabled')
const commentProvider = new CommentProvider()
context.subscriptions.push(vscode.workspace.registerDocumentCommentProvider(commentProvider))
}

class CommentProvider implements vscode.DocumentCommentProvider {
public async provideDocumentComments(
document: vscode.TextDocument,
token: vscode.CancellationToken
): Promise<vscode.CommentInfo> {
let threads: vscode.CommentThread[] = []
try {
threads = await provideDocumentComments(document)
} catch (e) {
log.appendLine(`provideDocumentComments ${e}`)
}

// commentingRanges are the ranges where the user can create a new comment.
// For now, this is the entire document. In the future we may wish to limit this
// in certain ways (e.g. not comment on lines which don't blame to a git revision that has been pushed to a remote).
const lastLine = document.lineCount - 1
const commentingRanges = [new vscode.Range(0, 0, lastLine, document.lineAt(lastLine).range.end.character)]
Comment thread
emidoots marked this conversation as resolved.

return { threads, commentingRanges }
}

public async createNewCommentThread(
document: vscode.TextDocument,
range: vscode.Range,
text: string,
token: vscode.CancellationToken
): Promise<vscode.CommentThread> {
return await createNewCommentThread(document, range, text)
}

public async replyToCommentThread(
document: vscode.TextDocument,
range: vscode.Range,
commentThread: vscode.CommentThread,
text: string,
token: vscode.CancellationToken
): Promise<vscode.CommentThread> {
return await replyToCommentThread(document, range, commentThread, text)
}

private didChangeCommentThreads = new vscode.EventEmitter<vscode.CommentThreadChangedEvent>()

public onDidChangeCommentThreads = this.didChangeCommentThreads.event
}

async function provideDocumentComments(document: vscode.TextDocument): Promise<vscode.CommentThread[]> {
log.appendLine(`provideDocumentComments ${document.uri}`)
const [remoteUrl, branch, path] = await repoInfo(document.fileName)
Comment thread
emidoots marked this conversation as resolved.
if (!remoteUrl) {
throw new Error('Git repository has no remote url configured')
}
const data = await queryGraphQL(
gql`
query DiscussionThreads(
$targetRepositoryGitCloneURL: String!
$targetRepositoryPath: String!
$relativeRev: String!
) {
discussionThreads(
first: 10000
targetRepositoryGitCloneURL: $targetRepositoryGitCloneURL
targetRepositoryPath: $targetRepositoryPath
) {
totalCount
pageInfo {
hasNextPage
}
nodes {
...DiscussionThreadFields
}
}
}
${discussionThreadFieldsFragment}
`,
{
targetRepositoryGitCloneURL: remoteUrl,
targetRepositoryPath: path,
relativeRev: branch,
}
)

if (!data.discussionThreads || !data.discussionThreads.nodes) {
throw new Error(`Invalid GraphQL response for DiscussionThreads`)
}

const threads: vscode.CommentThread[] = []
for (const thread of data.discussionThreads.nodes) {
Comment thread
nicksnyder marked this conversation as resolved.
if (thread.target.__typename !== 'DiscussionThreadTargetRepo') {
continue
}
// TODO: this assumes there is no diff between the document state and the revision
const sel = thread.target.relativeSelection
if (sel) {
const range = new vscode.Range(sel.startLine, sel.startCharacter, sel.endLine, sel.endCharacter)
threads.push(discussionToCommentThread(document, range, thread))
}
}

return threads
}

async function createNewCommentThread(
document: vscode.TextDocument,
range: vscode.Range,
text: string
): Promise<vscode.CommentThread> {
log.appendLine(`createNewCommentThread ${document.uri} ${range}`)
const [remoteUrl, branch, path] = await repoInfo(document.fileName)
Comment thread
emidoots marked this conversation as resolved.
if (!remoteUrl) {
throw new Error('Git repository has no remote url configured')
}

const selection = getSelection(document, range)
const input: SourcegraphGQL.IDiscussionThreadCreateInput = {
title: text,
contents: text,

@emidoots emidoots Oct 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, I'll try to make this change tomorrow or over the weekend.

targetRepo: {
repositoryGitCloneURL: remoteUrl,
path,
branch,
selection,
},
}

const data = await mutateGraphQL(
gql`
mutation CreateThread($input: DiscussionThreadCreateInput!, $relativeRev: String!) {
discussions {
createThread(input: $input) {
...DiscussionThreadFields
}
}
}
${discussionThreadFieldsFragment}
`,
{ input, relativeRev: branch }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

if (!data.discussions || !data.discussions.createThread) {
throw new Error(`Invalid GraphQL response for CreateThread`)
}

return discussionToCommentThread(document, range, data.discussions.createThread)
}

async function replyToCommentThread(
document: vscode.TextDocument,
range: vscode.Range,
thread: vscode.CommentThread,
text: string
): Promise<vscode.CommentThread> {
const [, branch] = await repoInfo(document.fileName)
Comment thread
emidoots marked this conversation as resolved.
const data = await mutateGraphQL(
gql`
mutation AddCommentToThread($threadID: ID!, $contents: String!, $relativeRev: String!) {
discussions {
addCommentToThread(threadID: $threadID, contents: $contents) {
...DiscussionThreadFields
}
}
}
${discussionThreadFieldsFragment}
`,
{ threadID: thread.threadId, contents: text, relativeRev: branch }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, relativeRev shouldn't be the branch name

)

if (!data.discussions || !data.discussions.addCommentToThread) {
throw new Error(`Invalid GraphQL response for AddCommentToThread`)
}

return discussionToCommentThread(document, range, data.discussions.addCommentToThread)
}
function discussionToCommentThread(
document: vscode.TextDocument,
range: vscode.Range,
thread: SourcegraphGQL.IDiscussionThread
): vscode.CommentThread {
const comments = thread.comments.nodes.map(comment => ({
commentId: comment.id,
body: new vscode.MarkdownString(comment.contents),
userName: comment.author.username,
gravatar: comment.author.avatarURL || '',
}))

return {
threadId: thread.id,
resource: document.uri,
range,
comments,
}
}

function getSelection(
document: vscode.TextDocument,
range: vscode.Range
): SourcegraphGQL.IDiscussionThreadTargetRepoSelectionInput {
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. 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).
  2. 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.
  3. 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 00000000 meaning.
  • @nicksnyder to add the logic in this PR to send 00000000 when comment is on uncomitted code, and send correct commit (or 00000000 "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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

@emidoots emidoots Oct 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

https://github.com/sourcegraph/sourcegraph/blob/1d7722a5295141024f3d2b0e32be790a04827112/cmd/frontend/graphqlbackend/schema.graphql#L387-L389

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came here to ask too if we can just use null. Sentinel values are bad for type safety.


const afterRange = new vscode.Range(range.end.line + 1, 0, range.end.line + 3, 0)
const linesAfter = getLines(document, afterRange)

return {
startLine: range.start.line,
startCharacter: range.start.character,
endLine: range.end.line,
endCharacter: range.end.character,
linesBefore,
lines,
linesAfter,
}
}

function getLines(document: vscode.TextDocument, range: vscode.Range): string[] {
const lines: string[] = []
for (let i = range.start.line; i <= range.end.line; i++) {
if (i >= 0 && i < document.lineCount) {
lines.push(document.lineAt(i).text)
}
}
return lines
}

const discussionThreadFieldsFragment = gql`
fragment DiscussionThreadFields on DiscussionThread {
id
author {
...UserFields
}
comments {
totalCount
nodes {
id
author {
...UserFields
}
contents
}
}
title
target {
__typename
... on DiscussionThreadTargetRepo {
repository {
name
}
path
relativePath(rev: $relativeRev)
branch {
displayName
}
revision {
displayName
}
selection {
startLine
startCharacter
endLine
endCharacter
linesBefore
lines
linesAfter
}
relativeSelection(rev: $relativeRev) {
startLine
startCharacter
endLine
endCharacter
}
}
}
inlineURL
createdAt
updatedAt
archivedAt
}

fragment UserFields on User {
displayName
username
avatarURL
}
`
4 changes: 4 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ export function getSourcegraphUrl(): string {
}
return url
}

export function getAccessToken(): string | undefined {
return vscode.workspace.getConfiguration('sourcegraph').get<string>('accessToken')
}
Loading