Skip to content

fix: restore X button during save/reject prompts (#6865) #6866

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+34 −13
Closed

fix: restore X button during save/reject prompts (#6865) #6866

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 34 additions & 13 deletions webview-ui/src/components/chat/ChatView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1942,25 +1942,46 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
</VSCodeButton>
</StandardTooltip>
)}
{(secondaryButtonText || isStreaming) && (
{(secondaryButtonText || isStreaming) && !isStreaming && (
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

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

Is this condition intentional? The logic checks for (secondaryButtonText || isStreaming) but then immediately requires !isStreaming, which effectively simplifies to just secondaryButtonText && !isStreaming. Could we simplify this for clarity?

<StandardTooltip
content={
isStreaming
? t("chat:cancel.tooltip")
: secondaryButtonText === t("chat:startNewTask.title")
? t("chat:startNewTask.tooltip")
: secondaryButtonText === t("chat:reject.title")
? t("chat:reject.tooltip")
: secondaryButtonText === t("chat:terminate.title")
? t("chat:terminate.tooltip")
: undefined
secondaryButtonText === t("chat:startNewTask.title")
? t("chat:startNewTask.tooltip")
: secondaryButtonText === t("chat:reject.title")
? t("chat:reject.tooltip")
: secondaryButtonText === t("chat:terminate.title")
? t("chat:terminate.tooltip")
: undefined
}>
<VSCodeButton
appearance="secondary"
disabled={!enableButtons && !(isStreaming && !didClickCancel)}
className={isStreaming ? "flex-[2] ml-0" : "flex-1 ml-[6px]"}
disabled={!enableButtons}
className="flex-1 ml-[6px]"
onClick={() => handleSecondaryButtonClick(inputValue, selectedImages)}>
{isStreaming ? t("chat:cancel.title") : secondaryButtonText}
{secondaryButtonText}
</VSCodeButton>
</StandardTooltip>
)}
{isStreaming && (
<StandardTooltip content={t("chat:cancel.tooltip")}>
<VSCodeButton
appearance="secondary"
disabled={!(isStreaming && !didClickCancel)}
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

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

The outer condition already ensures isStreaming is true, so this inner check isStreaming && !didClickCancel could be simplified to just !didClickCancel. Is there a reason for the redundant check?

className="flex-[2] ml-0"
onClick={() => handleSecondaryButtonClick(inputValue, selectedImages)}>
{t("chat:cancel.title")}
</VSCodeButton>
</StandardTooltip>
)}
{/* Add stop/close button when not streaming but showing approval buttons */}
{!isStreaming && enableButtons && primaryButtonText && (
<StandardTooltip content={t("chat:terminate.tooltip")}>
<VSCodeButton
appearance="icon"
disabled={false}
className="ml-2"
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

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

Minor styling inconsistency: This button uses className="ml-2" while other buttons in this section use ml-[6px]. Consider using consistent spacing for visual harmony.

onClick={() => startNewTask()}>
<span className="codicon codicon-x"></span>
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

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

Consider adding an aria-label attribute for better accessibility. Screen readers would benefit from a descriptive label like aria-label="Close task" since the button only contains an icon.

</VSCodeButton>
</StandardTooltip>
)}
Expand Down
Loading