Adopt new commenting api

[RMacfarlane]

History

A longer explanation is here: microsoft/vscode#68020 (comment)

The first iteration of the comments API was provider-based, which is a common pattern used by many VS Code APIs. In a provider based model, the extension is responsible for providing the data, and the rendering of the data is controlled by VS Code. The comments provider was essentially a class the extension implemented that had methods for returning comments for a specific document, adding, editing, and deleting comments, an event to fire for updates to comments, and methods for handling reviews.

The pros of this API were that it created consistent commenting experiences across extensions since all of the rendering was done by VS Code, and it was very explicit about how it should be used. The drawback to this API was that it was difficult to extend in a generic way. When we added the ability to create an entire review instead of leaving individual comments, we had to add explicit startDraft, deleteDraft, and finishDraft methods to the comment provider. The major driver for changing the comment API was to allow extensions more control over what actions a comment provider could take.

The new API

In full, the new API looks like:

//#region Comments

	/**
	 * Collapsible state of a [comment thread](#CommentThread)
	 */
	export enum CommentThreadCollapsibleState {
		/**
		 * Determines an item is collapsed
		 */
		Collapsed = 0,

		/**
		 * Determines an item is expanded
		 */
		Expanded = 1
	}

	/**
	 * Comment mode of a [comment](#Comment)
	 */
	export enum CommentMode {
		/**
		 * Displays the comment editor
		 */
		Editing = 0,

		/**
		 * Displays the preview of the comment
		 */
		Preview = 1
	}

	/**
	 * A collection of [comments](#Comment) representing a conversation at a particular range in a document.
	 */
	export interface CommentThread {
		/**
		 * The uri of the document the thread has been created on.
		 */
		readonly uri: Uri;

		/**
		 * The range the comment thread is located within the document. The thread icon will be shown
		 * at the first line of the range.
		 */
		range: Range;

		/**
		 * The ordered comments of the thread.
		 */
		comments: ReadonlyArray<Comment>;

		/**
		 * Whether the thread should be collapsed or expanded when opening the document.
		 * Defaults to Collapsed.
		 */
		collapsibleState: CommentThreadCollapsibleState;

		/**
		 * Context value of the comment thread. This can be used to contribute thread specific actions.
		 * For example, a comment thread is given a context value as `editable`. When contributing actions to `comments/commentThread/title`
		 * using `menus` extension point, you can specify context value for key `commentThread` in `when` expression like `commentThread == editable`.
		 * ```
		 *	"contributes": {
		 *		"menus": {
		 *			"comments/commentThread/title": [
		 *				{
		 *					"command": "extension.deleteCommentThread",
		 *					"when": "commentThread == editable"
		 *				}
		 *			]
		 *		}
		 *	}
		 * ```
		 * This will show action `extension.deleteCommentThread` only for comment threads with `contextValue` is `editable`.
		 */
		contextValue?: string;

		/**
		 * The optional human-readable label describing the [Comment Thread](#CommentThread)
		 */
		label?: string;

		/**
		 * Dispose this comment thread.
		 *
		 * Once disposed, this comment thread will be removed from visible editors and Comment Panel when approriate.
		 */
		dispose(): void;
	}

	/**
	 * Author information of a [comment](#Comment)
	 */
	export interface CommentAuthorInformation {
		/**
		 * The display name of the author of the comment
		 */
		name: string;

		/**
		 * The optional icon path for the author
		 */
		iconPath?: Uri;
	}

	/**
	 * Reactions of a [comment](#Comment)
	 */
	export interface CommentReaction {
		/**
		 * The human-readable label for the reaction
		 */
		readonly label: string;

		/**
		 * Icon for the reaction shown in UI.
		 */
		readonly iconPath: string | Uri;

		/**
		 * The number of users who have reacted to this reaction
		 */
		readonly count: number;

		/**
		 * Whether the [author](CommentAuthorInformation) of the comment has reacted to this reaction
		 */
		readonly authorHasReacted: boolean;
	}

	/**
	 * A comment is displayed within the editor or the Comments Panel, depending on how it is provided.
	 */
	export interface Comment {
		/**
		 * The human-readable comment body
		 */
		body: string | MarkdownString;

		/**
		 * [Comment mode](#CommentMode) of the comment
		 */
		mode: CommentMode;

		/**
		 * The [author information](#CommentAuthorInformation) of the comment
		 */
		author: CommentAuthorInformation;

		/**
		 * Context value of the comment. This can be used to contribute comment specific actions.
		 * For example, a comment is given a context value as `editable`. When contributing actions to `comments/comment/title`
		 * using `menus` extension point, you can specify context value for key `comment` in `when` expression like `comment == editable`.
		 * ```
		 *	"contributes": {
		 *		"menus": {
		 *			"comments/comment/title": [
		 *				{
		 *					"command": "extension.deleteComment",
		 *					"when": "comment == editable"
		 *				}
		 *			]
		 *		}
		 *	}
		 * ```
		 * This will show action `extension.deleteComment` only for comments with `contextValue` is `editable`.
		 */
		contextValue?: string;

		/**
		 * Optional reactions of the [comment](#Comment)
		 */
		reactions?: CommentReaction[];

		/**
		 * Optional label describing the [Comment](#Comment)
		 * Label will be rendered next to authorName if exists.
		 */
		label?: string;
	}

	/**
	 * Command argument for actions registered in `comments/commentThread/context`.
	 */
	export interface CommentReply {
		/**
		 * The active [comment thread](#CommentThread)
		 */
		thread: CommentThread;

		/**
		 * The value in the comment editor
		 */
		text: string;
	}

	/**
	 * Commenting range provider for a [comment controller](#CommentController).
	 */
	export interface CommentingRangeProvider {
		/**
		 * Provide a list of ranges which allow new comment threads creation or null for a given document
		 */
		provideCommentingRanges(document: TextDocument, token: CancellationToken): ProviderResult<Range[]>;
	}

	/**
	 * A comment controller is able to provide [comments](#CommentThread) support to the editor and
	 * provide users various ways to interact with comments.
	 */
	export interface CommentController {
		/**
		 * The id of this comment controller.
		 */
		readonly id: string;

		/**
		 * The human-readable label of this comment controller.
		 */
		readonly label: string;

		/**
		 * Optional commenting range provider. Provide a list [ranges](#Range) which support commenting to any given resource uri.
		 *
		 * If not provided, users can leave comments in any document opened in the editor.
		 */
		commentingRangeProvider?: CommentingRangeProvider;

		/**
		 * Create a [comment thread](#CommentThread). The comment thread will be displayed in visible text editors (if the resource matches)
		 * and Comments Panel once created.
		 *
		 * @param uri The uri of the document the thread has been created on.
		 * @param range The range the comment thread is located within the document.
		 * @param comments The ordered comments of the thread.
		 */
		createCommentThread(uri: Uri, range: Range, comments: Comment[]): CommentThread;

		/**
		 * Optional reaction handler for creating and deleting reactions on a [comment](#Comment).
		 */
		reactionHandler?: (comment: Comment, reaction: CommentReaction) => Promise<void>;

		/**
		 * Dispose this comment controller.
		 *
		 * Once disposed, all [comment threads](#CommentThread) created by this comment controller will also be removed from the editor
		 * and Comments Panel.
		 */
		dispose(): void;
	}

	namespace comments {
		/**
		 * Creates a new [comment controller](#CommentController) instance.
		 *
		 * @param id An `id` for the comment controller.
		 * @param label A human-readable string for the comment controller.
		 * @return An instance of [comment controller](#CommentController).
		 */
		export function createCommentController(id: string, label: string): CommentController;
	}

Now, instead of a provider, the extension creates an instance of a comment controller that can be used to display comments on a document. The comment controller does not have any other methods, instead, all other comment reactions are now specified through contribution points in the package.json. (https://code.visualstudio.com/api/references/contribution-points#contributes.menus) For a particular place in the UI, the extension registers a command it would like to be displayed with some icon or title.

For comments, commands can be added to the comment thread titlebar (top circle), comment titlebar (middle circle), the comment thread context (bottom circle), or comment context (buttons for a particular comment's text area, for example the command for finishing an edit). Since these are all commands, the extension can control their visibility with "when" clauses and their enablement with "enablement" properties. When a command is triggered by the user, it gets called with an argument containing the current thread and any text in the comment editor.

What's going on in this PR

The bulk of the changes are in the pullRequestNode.ts and reviewDocumentCommentProvider.ts, now reviewCommentController.ts files. We have previously already moved away from the provider to the controller, since then the major change was to have commands contributed as part of the package.json instead of on the controller.

[smashwilson]

I've looked over the code and didn't find anything showstopping - a few places that could be tidied up but nothing urgent. I'll take it for a spin locally tomorrow and leave a 👍 if it works properly 😄

[smashwilson]

👍 Probably a good idea.

[smashwilson]

Should we fail more noisily when the comment handler isn't found; emitting a logger method with the thread info, say, rather than silently doing nothing?

I get the sense that this is one of those "this shouldn't happen" guards. I always prefer to know when those assumptions fail or change, though - it would give us a little more to go on than a user reporting "this command does nothing".

[RMacfarlane]

Yeah, adding a log is a good idea. Added it to the resolveCommentHandler method itself

[smashwilson]

instanceof checks are a design smell to me. Usually they hint to me that what you really want to do is add common behavior to the types you're checking against. As a first pass I'd do something like this:

class GHPRComment {
   editGHPRComment(id: string) {
     if (this.commentId === id) {
       this.mode = vscode.CommentMode.Editing;
     }
   }

  editTemporaryComment() {}

  setParentEditMode() {
    for (const parentComment of this.parent.comments) {
       parentComment.editGHPRComment(this.commentId);
    }
  }
}

class TemporaryComment {
  editGHPRComment() {}

  editTemporaryComment(id: string) {
    if (this.id === id) {
      this.mode = vscode.CommentMode.Editing;
    }
  }

  setParentEditMode() {
    for (const parentComment of this.parent.comments) {
       parentComment.editTemporaryComment(this.id);
    }
  }
}

Then these if statements can be replaced with:

context.subscriptions.push(vscode.commands.registerCommand('pr.editComment', async (comment: GHPRComment | TemporaryComment) => {
  comment.setParentEditMode();
}

[RMacfarlane]

IMO having an editTemporaryComment method on a different class that does nothing is a bit harder to glance at and understand than the instanceof check, so I didn't adopt this whole proposal. But I did add startEdit and cancelEdit methods to both comment types in the spirit of having common behavior in class methods

[smashwilson]

Is there a reason this reassignment back to comment.parent.comments is done? It looks like the map function body is doing all of the actual work.

[RMacfarlane]

yeah, mutating the comments property on the thread causes the UI to update, only updated the property on the comment does not. Should have called that out in the description, we didn't want to create VS Code listeners for every single property change

[smashwilson]

A similar pattern could be used here.

[smashwilson]

This will also be useful for marking comments as "first-time contributor", "owner", and so on (author associations).

[smashwilson]

Is this hardcoded body value meant to be here?

[RMacfarlane]

good catch! nope :)

[smashwilson]

This could be another method defined on GHPRComment and TemporaryComment ☝️

[smashwilson]

Should createOrReplyComment() be awaited here?

[RMacfarlane]

yeah, you're right

[smashwilson]

This method is async - should this be awaited too? It might not matter in this case I suppose.

[smashwilson]

A few glitches that I've noticed taking this for a spin this morning:

1. Adding a reaction emoji to a pending comment duplicates it:

The duplicated comment persisted until I checked out another PR and came back.

2. Finishing a review doesn't refresh the "pending" badge on existing review comments.

3. I saw this adding a reaction emoji to a different comment:

(I couldn't find a stack trace.)

4. The "edit" button only works for pending review comments. It does nothing on submitted ones.

5. The "delete" button only works on the root comment of a comment thread. It does nothing on replies (I think).

[RMacfarlane]

@smashwilson Thanks for testing! I'm having trouble reproducing these. Do you remember if you saw these when the PR was checked out? And what type of file was open? (i.e. was the file opened from the "GitHub Pull Requests" tree, the "Changes in Pull Request" tree, or from the file explorer/file search)

[smashwilson]

Hmmm... I wonder if these are race conditions, then. It's possible that they were pre-existing problems as well - I've definitely seen the "duplicated comment" case crop up elsewhere 🤔

Do you remember if you saw these when the PR was checked out?

Yes, the PR was checked out.

And what type of file was open?

It was opened from the "Changes in Pull Request" tree.