Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a notion of pending / draft state to code review comments #171166

Open
hermannloose opened this issue Jan 12, 2023 · 9 comments
Open

Add a notion of pending / draft state to code review comments #171166

hermannloose opened this issue Jan 12, 2023 · 9 comments
Assignees
Labels
api api-proposal comments Comments Provider/Widget/Panel issues feature-request Request for new features or functionality
Milestone

Comments

@hermannloose
Copy link
Contributor

hermannloose commented Jan 12, 2023

#127473 introduced CommentThreadState with Unresolved and Resolved values to indicate the actionable vs non-actionable state of a particular conversation in code review.

In addition to that, GitHub pull requests and other code review systems frequently treat new comment threads or specific replies added by a user as pending / draft, being visible only to the user until they publish all pending comments at the same time.

We propose adding a new field to Comment objects to model this state.

For type, we think a boolean might be enough, but similar to #127473 we might want to use an enum instead for flexibility.

We don't have a preference for a particular name, draft, pending, or published (with inverse semantics) would all work well.

Areas in VS Code that would benefit from having a draft state:

  • the line gutter and the comments panel could render different icons for threads with drafts, to better indicate to users which conversations they have already replied to, for overview
  • there could be built-in commands to expand or collapse threads with drafts in the editor, so that users can hide threads that they have already replied to
  • the filtering functionality in the comments panel could have an option to "Show Drafts" so that users can hide threads they have already replied to

cc @laurentlb @JonasMa

@alexr00 alexr00 added feature-request Request for new features or functionality comments Comments Provider/Widget/Panel issues labels Jan 12, 2023
@alexr00 alexr00 added this to the Backlog milestone Jan 12, 2023
@alexr00
Copy link
Member

alexr00 commented Jan 12, 2023

The GitHub Pull Requests extension uses the Comment label for this.

@hermannloose
Copy link
Contributor Author

@alexr00 This is also our current approach, but it's text-only and does not enable the other use cases.

I also realized I didn't make it very explicit above that the three listed use cases all work on the level of threads, not individual comments / replies. They rely on a notion of a CommentThread being in a state of "having drafts" (orthogonal to whether the thread is unresolved or resolved) whenever one of its Comment children is a draft. The exact mechanics of this would still need to be formalized.

hermannloose added a commit to hermannloose/vscode that referenced this issue Feb 3, 2023
@hermannloose
Copy link
Contributor Author

@alexr00 I opened a PR so we have something more concrete to discuss. As mentioned, we are very flexible regarding the naming, this is just a starting point.

hermannloose added a commit to hermannloose/vscode that referenced this issue Feb 3, 2023
hermannloose added a commit to hermannloose/vscode that referenced this issue Feb 13, 2023
hermannloose added a commit to hermannloose/vscode that referenced this issue Mar 6, 2023
@alexr00
Copy link
Member

alexr00 commented Mar 7, 2023

Proposal:

export enum CommentVisibility {
Published = 0,
Draft = 1
}
export interface Comment {
visibility?: CommentVisibility;
}
export interface CommentThread {
readonly hasDraftComments: boolean;
}

@alexr00
Copy link
Member

alexr00 commented Mar 7, 2023

Adding to the March milestone for discussion.

@alexr00
Copy link
Member

alexr00 commented Mar 9, 2023

Notes from early API discussion:

  • We should not have hasDraftComments in CommentThread as this is sugar, which we try to avoid in API.
  • Comment visibility can be state to better align with CommentThread state. CommentVisibility would then likely follow and be CommentState.
  • We don't have a good suggestion yet for what to name the values of CommentState/CommentVisibility. There isn't a good precedent in the API. Since one of the consumers of this API would be GitHub Pull Requests and Issues, we could match GitHub and say Pending, as this would also be reflected in the UI and would be consistent with GitHub. However, "pending" actually doesn't describe well that the comment is not yet available to other users.

@hermannloose
Copy link
Contributor Author

I updated #173305 to remove hasDraftComments. Is there a rough timeline for the other decisions?

@alexr00
Copy link
Member

alexr00 commented Mar 13, 2023

@hermannloose this isn't planned work, so I don't have time allocated for it at the moment. I intend to take another look at this next week and I'll try to put it on the plan for April if I don't get to it next week.

@alexr00 alexr00 modified the milestones: March 2023, April 2023 Mar 22, 2023
alexr00 added a commit that referenced this issue Mar 28, 2023
* Allow individual comments to be marked as draft

This is a proposal for #171166.

* Remove `hasDraftComments` from `CommentThread`

* Rename `CommentVisibility` to `CommentState`

* Rename `CommentVisibility` to `CommentState`

* Add api proposal check

---------

Co-authored-by: Alex Ross <[email protected]>
@alexr00 alexr00 modified the milestones: April 2023, On Deck Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api api-proposal comments Comments Provider/Widget/Panel issues feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

2 participants