New Comments API by rebornix · Pull Request #972 · microsoft/vscode-pull-request-github

rebornix · 2019-02-25T01:46:18Z

The refactoring of registering comment threas for Pull Request list view is done but review mode part is still in progress, following code needs to be improved

Register comment threads for pr, review and file scheme files
Update them properly when there are new comments update
Update commenting range when document is dirtly
Dispose them properly

…ntrol

rebornix · 2019-02-25T17:21:48Z

package.json

        {
          "command": "pr.refreshPullRequest",
-          "when": "view == pr && viewItem =~ /pullrequest|description/",
          "group": "pullrequest@2"


It's a hack as context values seem not working when debugging.

…ntrol

RMacfarlane

I'm still doing some more testing so might add another review

RMacfarlane · 2019-03-15T21:13:46Z

src/github/commands.ts

+
+export function getEmptyCommentThreadCommands(thread: vscode.CommentThread, inDraftMode: boolean, handler: CommentHandler, supportGraphQL: boolean): { acceptInputCommand: vscode.Command, additionalCommands: vscode.Command[] } {
+	let commands: vscode.Command[] = [];
+	let acceptInputCommand = {


I think the title for this command should also change based on whether we are in draft mode like getCommentThreadCommands below, either Add Comment or Add Review Comment

RMacfarlane · 2019-03-15T21:22:29Z

src/github/utils.ts

+		[]
+	);
+
+	thread.comments.forEach(comment => {


could move this above and then call createCommentThread with vscodeThread.comments so there's no update event

comment relies on thread as it's one argument for edit and delete command.

Here I put comments in ctor and then update comments again with proper edit/delete commands, the reason is if we create comment thread with empty comments, there are chances that VS Code focuses in the comment editor.

RMacfarlane · 2019-03-15T21:33:31Z

src/view/treeNodes/pullRequestNode.ts


-		if (fileChange instanceof RemoteFileChangeNode) {
-			throw new Error('Comments not supported on remote file changes');
+				thread.comments = [...thread.comments, convertToVSCodeComment(rawComment!, undefined)];


The edit and delete commands are not being added here, we should have another method that will create comments when the thread already exists

RMacfarlane · 2019-03-15T21:38:28Z

src/github/utils.ts

-		canDelete: comment.canDelete,
-		isDraft: !!comment.isDraft,
+		userIconPath: vscode.Uri.parse(comment.user!.avatarUrl),
+		label: !!comment.isDraft ? 'Pending' : undefined,


This seems to be an upstream issue, if I start a review, and then reload vscode, the comments are not initially shown with the 'Pending' label. I see that the label is correctly being set here. If I add another pending comment to the thread, the labels for the thread are rendered

RMacfarlane · 2019-03-15T21:46:18Z

src/view/treeNodes/pullRequestNode.ts

 		await this._prManager.deleteReviewComment(this.pullRequestModel, comment.commentId);
-		const index = fileChange.comments.findIndex(c => c.id.toString() === comment.commentId);
+		const index = thread.comments.findIndex(c => c.commentId === comment.commentId);
 		if (index > -1) {


If the comment is the last in the thread, we should also dispose the thread. Ditto in the reviewDocumentCommentProvider

rebornix · 2019-03-16T20:04:33Z

@RMacfarlane thanks for your thorough review, I feel confident with our product quality as your review is always high quality ❤️

I addressed your comments and fixed a few upstream issues (not in Insiders yet so please wait till Monday). In addition to issues you ran into, I also fixed a few bugs with Review and comments update. Since we didn't merge this pr when I was still in office, I need your help to merge this pr. You may still run into bugs so it would be great if you have a better understanding how comment threads cache is managed in this PR

PR List View
- Raw comments are saved in fileChanges, which is the same as before
- When an editor is being opened, we will check if we already create comment threads for the newly opened editor or not. If not, we create comment threads based on fileChange.comments.
- When an editor is being closed, we will dispose all related comment threads.
- When we refresh the PR, we will compare the cached comment threads for this PR, with the new comments, and update accordingly.
- Note, we only update all comment threads for PR when users attempt to refresh the PR, which is the same behavior as before.
- When we create/update/delete comments, we need to update fileChanges.comments and visible comment threads.
Review
- Raw comments are saved in fileChanges, which is the same as before
- We will create comment threads when we detect the workspace is in review mode.
- When we open diff view from PR List View or Changes in PR, we will create comment threads when the editor is being opened, and dispose the comment threads when the editor is being closed. The logic is the same as what we have in PR List View
- When we create/update/delete comments, we need to update all visible review/pr comment threads and all workspace file comment threads.

The challenge here is we now moved to push mode from previous pull mode. It means we need to manage the state/cache of comment threads. It can be simple if we only need to support file scheme but life is life, we need to support pr, review and file. So if you run into weird bug, look into how we cache comment threads, usually that's where the bug is from.

The code architecture can be improved by moving the visible editors change listener to PRDocumentCommentsProvider instead of registering in PRNode and remove any pr related code from ReviewDocumentCommentsProvider but we don't need to do that now.

RMacfarlane · 2019-03-18T16:35:00Z

Thanks for the clear explanation! I've done some more testing and found some more issues that need to be fixed, seems like a lot of them are upstream.

Without checking out a PR, open a file and leave a comment. Switch to another file and then back, comment is gone
Leave some text in a comment without submitting it, switch to another file and do the same. Switch back, the pending comment text is overwritten
If clicking the gutter icon quickly, mutiple empty comment threads can be created
No way to remove empty comment threads
Toggling reaction on something I haven't reacted to tries to remove the reaction and errors out
Checkout a PR, then return to master. No commenting ranges are shown on any file opened from the tree after that
Comments on 'review' scheme diffs always show on the right side even if they were made on the base
The inline 'open file' action in the changes in PR tree doesn't work

…ng text collisions in vscode core

RMacfarlane

I think it's OK to merge now.

Fixes #972

rebornix added 7 commits February 21, 2019 10:35

123

0e4cd0e

dispose comment thread when refresh

599651e

create new comment thread support

5686573

Merge remote-tracking branch 'origin/master' into rebornix/commentsco…

55134e7

…ntrol

Format

ea7539d

Do not update unless there are new threads.

28cc12a

register comments in review mode.

ec7793c

rebornix changed the title ~~register comments in review mode.~~ WIP: New Comments API Feb 25, 2019

rebornix commented Feb 25, 2019

View reviewed changes

rebornix added 21 commits February 26, 2019 10:25

remove unused file

d6f71f0

Merge remote-tracking branch 'origin/master' into rebornix/commentsco…

969c0e0

…ntrol

Merge remote-tracking branch 'origin/master' into rebornix/commentsco…

16d2192

…ntrol

move edit/delete command into vscodeCommentConvert

b382336

range updates for local comment thread

06ed74b

there should be only one validateState at one time.

dee11c5

Create workspace comment threads once loaded

55c2187

remove console.log

0f0cd39

we don't need to check if document is dirty when first load

6beb0ef

Update comment threads properly

4e37c37

update draft mode

e311a47

commenting range provider

214f56d

dispose pr comment controller properly

03b4320

Draft label

ecffaa6

update label when review finishes.

2c86de2

update proposed typings

db3b402

Separete default accept input command and additional input commands

1fba4d7

remove old api

eb76364

create new comment thread.

7fd04f6

move edit/delete comment logic back to document providers

3cb00ca

Comment handler should have comment controller

499f7fa

rebornix added 3 commits March 15, 2019 12:28

Merge remote-tracking branch 'origin/master' into rebornix/commentsco…

fd88191

…ntrol

minimal code change.

b5dc25b

Remove comment thread cache when checkout

30483a0

RMacfarlane suggested changes Mar 15, 2019

View reviewed changes

rebornix added 6 commits March 15, 2019 15:53

Update comment edit commands

5d5daf2

check canedit candelete

3cbb302

Update comment label only it's in pending state.

9a95bbc

Trigger update when delete or add new comment thread

9332639

Update comments commands after ctor

35b1460

Dispose comment threads when editors are closed. PR View.

88750d6

rebornix added 8 commits March 16, 2019 15:35

remove cached review comments

1d43519

extract update comment thread comments

ac89b8e

extract update comments

c1bb453

updateCommentReactions

c5b5609

rename update comment commands

76c935e

Toggle reactions for review file

3624fba

handler

f03df41

Update review comments when deleted in review mode ;(

9dc791e

RMacfarlane added 6 commits March 18, 2019 10:27

Fix strict null checks and simply getting comment commands

ce46c0c

Toggling reaction should check if user has already reacted

35a07c2

Add command for deleting empty comment threads

6c5c203

Update file change comments when new comment thread is added

6f60495

Make empty comment threads have unique ids so that there are no pendi…

dfe58cb

…ng text collisions in vscode core

Fix inline open file action

ad8836b

RMacfarlane approved these changes Mar 19, 2019

View reviewed changes

RMacfarlane merged commit f6bb4cd into master Mar 19, 2019

RMacfarlane deleted the rebornix/commentscontrol branch March 19, 2019 23:47

alexr00 added a commit that referenced this pull request Mar 31, 2020

Check repo before setting inputbox in scm

c265e0d

Fixes #972

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Comments API#972

New Comments API#972
RMacfarlane merged 81 commits intomasterfrom
rebornix/commentscontrol

rebornix commented Feb 25, 2019 •

edited

Loading

Uh oh!

rebornix Feb 25, 2019

Uh oh!

RMacfarlane left a comment

Uh oh!

RMacfarlane Mar 15, 2019

Uh oh!

RMacfarlane Mar 15, 2019

Uh oh!

rebornix Mar 16, 2019

Uh oh!

rebornix Mar 16, 2019

Uh oh!

RMacfarlane Mar 15, 2019

Uh oh!

RMacfarlane Mar 15, 2019

Uh oh!

RMacfarlane Mar 15, 2019

Uh oh!

rebornix commented Mar 16, 2019

Uh oh!

RMacfarlane commented Mar 18, 2019 •

edited

Loading

Uh oh!

RMacfarlane left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rebornix commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RMacfarlane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebornix commented Mar 16, 2019

Uh oh!

RMacfarlane commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RMacfarlane left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rebornix commented Feb 25, 2019 •

edited

Loading

RMacfarlane commented Mar 18, 2019 •

edited

Loading