Fix review button disable logic with special handling for approve button (#7507)

* Initial plan

* Fix review button disable logic - prevent empty review submission

Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>

* Use hasReviewDraft in PR overview

* Apply same disable logic to AddComment components in comment.tsx

Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>

* Allow approve button even with empty content and no pending review

Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>

* Handle disablement in dropdown button

* nit

* Fix test

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>

Author: Copilot

package.json

@@ -1343,12 +1343,14 @@
 			{
 				"command": "review.comment",
 				"title": "%command.review.comment.title%",
-				"category": "%command.pull.request.category%"
+				"category": "%command.pull.request.category%",
+				"enablement": "github:reviewCommentCommentEnabled"
 			},
 			{
 				"command": "review.requestChanges",
 				"title": "%command.review.requestChanges.title%",
-				"category": "%command.pull.request.category%"
+				"category": "%command.pull.request.category%",
+				"enablement": "github:reviewRequestChangesEnabled"
 			},
 			{
 				"command": "review.approveOnDotCom",
@@ -1368,12 +1370,14 @@
 			{
 				"command": "review.commentDescription",
 				"title": "%command.review.comment.title%",
-				"category": "%command.pull.request.category%"
+				"category": "%command.pull.request.category%",
+				"enablement": "github:reviewCommentCommentEnabled"
 			},
 			{
 				"command": "review.requestChangesDescription",
 				"title": "%command.review.requestChanges.title%",
-				"category": "%command.pull.request.category%"
+				"category": "%command.pull.request.category%",
+				"enablement": "github:reviewRequestChangesEnabled"
 			},
 			{
 				"command": "review.approveOnDotComDescription",

src/github/pullRequestOverview.ts

@@ -236,6 +236,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
 				mergeability,
 				emailForCommit,
 				coAuthors,
+				hasReviewDraft
 			] = await Promise.all([
 				this._folderRepositoryManager.resolvePullRequest(
 					pullRequestModel.remote.owner,
@@ -255,7 +256,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
 				this._folderRepositoryManager.isHeadUpToDateWithBase(pullRequestModel),
 				pullRequestModel.getMergeability(),
 				this._folderRepositoryManager.getPreferredEmail(pullRequestModel),
-				pullRequestModel.getCoAuthors()
+				pullRequestModel.getCoAuthors(),
+				pullRequestModel.validateDraftMode(),
 			]);
 			if (!pullRequest) {
 				throw new Error(
@@ -300,6 +302,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
 				isDraft: pullRequest.isDraft,
 				mergeMethodsAvailability,
 				defaultMergeMethod,
+				hasReviewDraft,
 				autoMerge: pullRequest.autoMerge,
 				allowAutoMerge: pullRequest.allowAutoMerge,
 				autoMergeMethod: pullRequest.autoMergeMethod,

src/test/github/pullRequestOverview.test.ts

@@ -119,6 +119,7 @@ describe('PullRequestOverview', function () {
 			const resolveStub = sinon.stub(pullRequestManager, 'resolvePullRequest').resolves(prModel0);
 			sinon.stub(prModel0, 'getReviewRequests').resolves([]);
 			sinon.stub(prModel0, 'getTimelineEvents').resolves([]);
+			sinon.stub(prModel0, 'validateDraftMode').resolves(true);
 			sinon.stub(prModel0, 'getStatusChecks').resolves([{ state: CheckState.Success, statuses: [] }, null]);
 			await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, prModel0);
 
@@ -132,6 +133,7 @@ describe('PullRequestOverview', function () {
 			resolveStub.resolves(prModel1);
 			sinon.stub(prModel1, 'getReviewRequests').resolves([]);
 			sinon.stub(prModel1, 'getTimelineEvents').resolves([]);
+			sinon.stub(prModel1, 'validateDraftMode').resolves(true);
 			sinon.stub(prModel1, 'getStatusChecks').resolves([{ state: CheckState.Success, statuses: [] }, null]);
 			await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, prModel1);
 

webviews/components/comment.tsx

@@ -44,7 +44,7 @@ export function CommentView(commentProps: Props) {
 	const [bodyMd, setBodyMd] = useStateProp(body);
 	const [bodyHTMLState, setBodyHtml] = useStateProp(bodyHTML);
 	const { deleteComment, editComment, setDescription, pr } = useContext(PullRequestContext);
-	const currentDraft = pr.pendingCommentDrafts && pr.pendingCommentDrafts[id];
+	const currentDraft = pr?.pendingCommentDrafts && pr.pendingCommentDrafts[id];
 	const [inEditMode, setEditMode] = useState(!!currentDraft);
 	const [showActionBar, setShowActionBar] = useState(false);
 
@@ -55,7 +55,7 @@ export function CommentView(commentProps: Props) {
 				key={`editComment${id}`}
 				body={currentDraft || bodyMd}
 				onCancel={() => {
-					if (pr.pendingCommentDrafts) {
+					if (pr?.pendingCommentDrafts) {
 						delete pr.pendingCommentDrafts[id];
 					}
 					setEditMode(false);
@@ -114,7 +114,7 @@ export function CommentView(commentProps: Props) {
 				comment={comment as IComment}
 				bodyHTML={bodyHTMLState}
 				body={bodyMd}
-				canApplyPatch={pr.isCurrentlyCheckedOut}
+				canApplyPatch={!!pr?.isCurrentlyCheckedOut}
 				allowEmpty={!!commentProps.allowEmpty}
 				specialDisplayBodyPostfix={(comment as IComment).specialDisplayBodyPostfix}
 			/>
@@ -349,6 +349,7 @@ export function AddComment({
 	currentUserReviewState,
 	lastReviewType,
 	busy,
+	hasReviewDraft,
 }: PullRequest) {
 	const { updatePR, requestChanges, approve, close, openOnGitHub, submit } = useContext(PullRequestContext);
 	const [isBusy, setBusy] = useState(false);
@@ -413,8 +414,13 @@ export function AddComment({
 			}
 			: commentMethods(isIssue);
 
+	// Disable buttons when summary comment is empty AND there are no review comments
+	// Note: Approve button is allowed even with empty content and no pending review
+	const shouldDisableNonApproveButtons = !pendingCommentText?.trim() && !hasReviewDraft;
+	const shouldDisableApproveButton = false; // Approve is always allowed (when not busy)
+
 	return (
-		<form id="comment-form" ref={form as React.MutableRefObject<HTMLFormElement>} className="comment-form main-comment-form" onSubmit={() => submit(textareaRef.current?.value ?? '')}>
+		<form id="comment-form" ref={form as React.MutableRefObject<HTMLFormElement>} className="comment-form main-comment-form" >
 			<textarea
 				id="comment-textarea"
 				name="body"
@@ -445,20 +451,20 @@ export function AddComment({
 
 
 				<ContextDropdown
-					optionsContext={() => makeCommentMenuContext(availableActions, pendingCommentText)}
+					optionsContext={() => makeCommentMenuContext(availableActions, pendingCommentText, shouldDisableNonApproveButtons)}
 					defaultAction={defaultSubmitAction}
 					defaultOptionLabel={() => availableActions[currentSelection]!}
 					defaultOptionValue={() => currentSelection}
 					allOptions={() => {
-						const actions: { label: string; value: string; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void }[] = [];
+						const actions: { label: string; value: string; optionDisabled: boolean; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void }[] = [];
 						if (availableActions.approve) {
-							actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve) });
+							actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton });
 						}
 						if (availableActions.comment) {
-							actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment) });
+							actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment), optionDisabled: shouldDisableNonApproveButtons });
 						}
 						if (availableActions.requestChanges) {
-							actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges) });
+							actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges), optionDisabled: shouldDisableNonApproveButtons });
 						}
 						return actions;
 					}}
@@ -486,7 +492,7 @@ const COMMENT_METHODS = {
 	requestChanges: 'Request Changes',
 };
 
-const makeCommentMenuContext = (availableActions: { comment?: string, approve?: string, requestChanges?: string }, pendingCommentText: string | undefined) => {
+const makeCommentMenuContext = (availableActions: { comment?: string, approve?: string, requestChanges?: string }, pendingCommentText: string | undefined, shouldDisableNonApproveButtons: boolean) => {
 	const createMenuContexts = {
 		'preventDefaultContextMenuItems': true,
 		'github:reviewCommentMenu': true,
@@ -500,10 +506,16 @@ const makeCommentMenuContext = (availableActions: { comment?: string, approve?:
 	}
 	if (availableActions.comment) {
 		createMenuContexts['github:reviewCommentComment'] = true;
+		if (!shouldDisableNonApproveButtons) {
+			createMenuContexts['github:reviewCommentCommentEnabled'] = true;
+		}
 	}
 	if (availableActions.requestChanges) {
 		if (availableActions.requestChanges === COMMENT_METHODS.requestChanges) {
 			createMenuContexts['github:reviewCommentRequestChanges'] = true;
+			if (!shouldDisableNonApproveButtons) {
+				createMenuContexts['github:reviewRequestChangesEnabled'] = true;
+			}
 		} else {
 			createMenuContexts['github:reviewCommentRequestChangesOnDotCom'] = true;
 		}
@@ -568,6 +580,11 @@ export const AddCommentSimple = (pr: PullRequest) => {
 			}
 			: commentMethods(pr.isIssue);
 
+	// Disable buttons when summary comment is empty AND there are no review comments
+	// Note: Approve button is allowed even with empty content and no pending review
+	const shouldDisableNonApproveButtons = !pr.pendingCommentText?.trim() && !pr.hasReviewDraft;
+	const shouldDisableApproveButton = false; // Approve is always allowed (when not busy)
+
 	return (
 		<span className="comment-form">
 			<textarea
@@ -582,20 +599,20 @@ export const AddCommentSimple = (pr: PullRequest) => {
 			/>
 			<div className='comment-button'>
 				<ContextDropdown
-					optionsContext={() => makeCommentMenuContext(availableActions, pr.pendingCommentText)}
+					optionsContext={() => makeCommentMenuContext(availableActions, pr.pendingCommentText, shouldDisableNonApproveButtons)}
 					defaultAction={defaultSubmitAction}
 					defaultOptionLabel={() => availableActions[currentSelection]!}
 					defaultOptionValue={() => currentSelection}
 					allOptions={() => {
-						const actions: { label: string; value: string; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void }[] = [];
+						const actions: { label: string; value: string; optionDisabled: boolean; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void }[] = [];
 						if (availableActions.approve) {
-							actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve) });
+							actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton });
 						}
 						if (availableActions.comment) {
-							actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment) });
+							actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment), optionDisabled: shouldDisableNonApproveButtons });
 						}
 						if (availableActions.requestChanges) {
-							actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges) });
+							actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges), optionDisabled: shouldDisableNonApproveButtons });
 						}
 						return actions;
 					}}

webviews/components/contextDropdown.tsx

@@ -6,17 +6,17 @@
 import React, { useEffect, useLayoutEffect, useRef, useState } from 'react';
 import { chevronDownIcon } from './icon';
 
-interface ContextDropdownProps {
-	optionsContext: () => string;
-	defaultOptionLabel: () => string | React.ReactNode;
-	defaultOptionValue: () => string;
-	defaultAction: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void;
-	allOptions?: () => { label: string; value: string; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void }[];
-	optionsTitle: string;
-	disabled?: boolean;
-	hasSingleAction?: boolean;
-	spreadable: boolean;
-	isSecondary?: boolean;
+interface ContextDropdownProps {
+	optionsContext: () => string;
+	defaultOptionLabel: () => string | React.ReactNode;
+	defaultOptionValue: () => string;
+	defaultAction: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void;
+	allOptions?: () => { label: string; value: string; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void; optionDisabled?: boolean }[];
+	optionsTitle: string;
+	disabled?: boolean;
+	hasSingleAction?: boolean;
+	spreadable: boolean;
+	isSecondary?: boolean;
 }
 
 function useWindowSize() {
@@ -55,14 +55,14 @@ export const ContextDropdown = ({ optionsContext, defaultOptionLabel, defaultOpt
 	useWindowSize();
 
 	return <div className={`dropdown-container${spreadable ? ' spreadable' : ''}`} ref={divRef}>
-		{divRef.current && spreadable && (divRef.current.clientWidth > 375) && options && !hasSingleAction ? options().map(({ label, value, action }) => {
-			return <button className='inlined-dropdown' key={value} title={label} disabled={disabled} onClick={action} value={value}>{label}</button>;
+		{divRef.current && spreadable && (divRef.current.clientWidth > 375) && options && !hasSingleAction ? options().map(({ label, value, action, optionDisabled }) => {
+			return <button className='inlined-dropdown' key={value} title={label} disabled={optionDisabled || disabled} onClick={action} value={value}>{label}</button>;
 		})
 			:
-			<div className='primary-split-button'>
-				<button className={`split-left${isSecondary ? ' secondary' : ''}`} disabled={disabled} onClick={defaultAction} value={defaultOptionValue()}
-					title={typeof defaultOptionLabel() === 'string' ? defaultOptionLabel() as string : optionsTitle}>
-					{defaultOptionLabel()}
+			<div className='primary-split-button'>
+				<button className={`split-left${isSecondary ? ' secondary' : ''}`} disabled={disabled} onClick={defaultAction} value={defaultOptionValue()}
+					title={typeof defaultOptionLabel() === 'string' ? defaultOptionLabel() as string : optionsTitle}>
+					{defaultOptionLabel()}
 				</button>
 				{hasSingleAction ? null :
 					<div className={`split${isSecondary ? ' secondary' : ''}${disabled ? ' disabled' : ''}`}><div className={`separator${disabled ? ' disabled' : ''}`}></div></div>

webviews/components/timeline.tsx

@@ -270,13 +270,14 @@ function CommentThread({ thread, event }: { thread: IComment[]; event: ReviewEve
 
 function AddReviewSummaryComment() {
 	const { requestChanges, approve, submit, pr } = useContext(PullRequestContext);
-	const { isAuthor } = pr;
+	const isAuthor = pr?.isAuthor;
 	const comment = useRef<HTMLTextAreaElement>();
 	const [isBusy, setBusy] = useState(false);
+	const [commentText, setCommentText] = useState('');
 
 	async function submitAction(event: React.MouseEvent | React.KeyboardEvent, action: ReviewType): Promise<void> {
 		event.preventDefault();
-		const { value } = comment.current!;
+		const value = commentText;
 		setBusy(true);
 		switch (action) {
 			case ReviewType.RequestChanges:
@@ -297,20 +298,30 @@ function AddReviewSummaryComment() {
 		}
 	};
 
+	const onTextareaChange = (event: React.ChangeEvent<HTMLTextAreaElement>) => {
+		setCommentText(event.target.value);
+	};
+
+	// Disable buttons when summary comment is empty AND there are no review comments
+	// Note: Approve button is allowed even with empty content and no pending review
+	const shouldDisableButtons = !commentText.trim() && !pr.hasReviewDraft;
+
 	return (
 		<form>
 			<textarea
 				id='pending-review'
 				ref={comment}
 				placeholder="Leave a review summary comment"
 				onKeyDown={onKeyDown}
+				onChange={onTextareaChange}
+				value={commentText}
 			></textarea>
 			<div className="form-actions">
 				{isAuthor ? null : (
 					<button
 						id="request-changes"
 						className='secondary'
-						disabled={isBusy || pr.busy}
+						disabled={isBusy || pr.busy || shouldDisableButtons}
 						onClick={(event) => submitAction(event, ReviewType.RequestChanges)}
 					>
 						Request Changes
@@ -326,7 +337,7 @@ function AddReviewSummaryComment() {
 					</button>
 				)}
 				<button
-					disabled={isBusy || pr.busy}
+					disabled={isBusy || pr.busy || shouldDisableButtons}
 					onClick={(event) => submitAction(event, ReviewType.Comment)}
 				>Submit Review</button>
 			</div>