diff --git a/packages/types/src/__tests__/followup.test.ts b/packages/types/src/__tests__/followup.test.ts new file mode 100644 index 0000000000..7e739b8f4e --- /dev/null +++ b/packages/types/src/__tests__/followup.test.ts @@ -0,0 +1,76 @@ +import { + firstUsableSuggestion, + followUpDataSchema, + hasUsableAnswer, + suggestionItemSchema, + type SuggestionItem, +} from "../followup.js" + +describe("hasUsableAnswer", () => { + it("accepts a non-blank string answer", () => { + expect(hasUsableAnswer({ answer: "Yes, proceed" })).toBe(true) + }) + + it("accepts a non-blank answer with surrounding whitespace, including one that also carries a mode", () => { + const withMode: SuggestionItem = { answer: " spaced ", mode: "code" } + expect(hasUsableAnswer(withMode)).toBe(true) + }) + + it("rejects an empty or whitespace-only answer", () => { + expect(hasUsableAnswer({ answer: "" })).toBe(false) + expect(hasUsableAnswer({ answer: " \n\t " })).toBe(false) + }) + + it("rejects a missing answer (issue #1226)", () => { + expect(hasUsableAnswer({})).toBe(false) + expect(hasUsableAnswer({ answer: undefined })).toBe(false) + }) + + it("rejects non-string answers from malformed transport data (issue #1226)", () => { + expect(hasUsableAnswer({ answer: 42 })).toBe(false) + expect(hasUsableAnswer({ answer: { mode_slug: "code" } })).toBe(false) + expect(hasUsableAnswer({ answer: null })).toBe(false) + }) + + it("rejects a null or undefined suggestion item", () => { + expect(hasUsableAnswer(null)).toBe(false) + expect(hasUsableAnswer(undefined)).toBe(false) + }) +}) + +describe("firstUsableSuggestion", () => { + it("returns undefined for missing or empty suggestions", () => { + expect(firstUsableSuggestion(undefined)).toBeUndefined() + expect(firstUsableSuggestion(null)).toBeUndefined() + expect(firstUsableSuggestion([])).toBeUndefined() + }) + + it("skips blank or missing answers and returns the first usable item", () => { + const suggestions: SuggestionItem[] = [{}, { answer: " " }, { answer: " ok " }] + expect(firstUsableSuggestion(suggestions)).toEqual({ answer: " ok " }) + }) +}) + +describe("suggestionItemSchema", () => { + it("accepts a suggestion without an answer (issue #1226)", () => { + expect(suggestionItemSchema.parse({ mode: "code" })).toEqual({ mode: "code" }) + }) + + it("still rejects a non-string answer", () => { + expect(() => suggestionItemSchema.parse({ answer: 42 })).toThrow() + }) +}) + +describe("followUpDataSchema", () => { + it("accepts suggestions with a mix of usable and missing answers (issue #1226)", () => { + const parsed = followUpDataSchema.parse({ + question: "Pick one?", + suggest: [{ answer: "Yes" }, { mode: "code" }, { answer: undefined }], + }) + + expect(parsed).toEqual({ + question: "Pick one?", + suggest: [{ answer: "Yes" }, { mode: "code" }, {}], + }) + }) +}) diff --git a/packages/types/src/followup.ts b/packages/types/src/followup.ts index b3990c2cee..05f8029e9d 100644 --- a/packages/types/src/followup.ts +++ b/packages/types/src/followup.ts @@ -16,8 +16,15 @@ export interface FollowUpData { * Interface for a suggestion item with optional mode switching */ export interface SuggestionItem { - /** The text of the suggestion */ - answer: string + /** + * The text of the suggestion. + * + * Optional because the model can emit malformed follow-up payloads with a + * missing or blank answer (issue #1226) and the extension-host transport + * does not validate `FollowUpData`. Guard with `hasUsableAnswer()` before + * treating the value as usable text. + */ + answer?: string /** Optional mode to switch to when selecting this suggestion */ mode?: string } @@ -35,11 +42,35 @@ export const getSuggestionMode = (mode: unknown): string | undefined => { return undefined } +/** + * Whether a follow-up suggestion carries a usable answer: a non-blank string. + * + * `SuggestionItem.answer` is optional because a malformed model payload may + * omit it (issue #1226), and the unvalidated transport may deliver a + * non-string value at runtime. The extension-host auto-approval + * (`checkAutoApproval`), the `FollowUpSuggest` visible-suggestions filter, and + * `ChatView`'s suggestion click handler all guard through this helper so the + * definition of "usable answer" stays in one place. + */ +export const hasUsableAnswer = ( + suggestion: { answer?: unknown } | null | undefined, +): suggestion is SuggestionItem & { answer: string } => + typeof suggestion?.answer === "string" && suggestion.answer.trim().length > 0 + +/** + * The first suggestion with a usable answer, if any. + * + * Shared by the extension-host auto-approval (`checkAutoApproval`) and the + * webview so the "first usable suggestion" rule stays in one place. + */ +export const firstUsableSuggestion = (suggestions?: SuggestionItem[] | null): SuggestionItem | undefined => + (suggestions ?? []).find((s) => hasUsableAnswer(s)) + /** * Zod schema for SuggestionItem */ export const suggestionItemSchema = z.object({ - answer: z.string(), + answer: z.string().optional(), mode: z.string().optional(), }) diff --git a/src/core/auto-approval/__tests__/followup.spec.ts b/src/core/auto-approval/__tests__/followup.spec.ts new file mode 100644 index 0000000000..39bd26e9ba --- /dev/null +++ b/src/core/auto-approval/__tests__/followup.spec.ts @@ -0,0 +1,112 @@ +import type { ExtensionState } from "@roo-code/types" +import { checkAutoApproval, type AutoApprovalState, type AutoApprovalStateOptions } from ".." + +type AutoApprovalFields = Pick + +describe("Follow-up question auto-approval", () => { + const baseState: AutoApprovalFields = { + autoApprovalEnabled: true, + alwaysAllowFollowupQuestions: true, + followupAutoApproveTimeoutMs: 10_000, + } + + const followupText = (suggest: unknown) => JSON.stringify({ question: "Pick one?", suggest }) as string + + const run = (state: AutoApprovalFields, text: string) => checkAutoApproval({ state, ask: "followup", text }) + + it("schedules a timeout that auto-answers with the first valid suggestion", async () => { + const result = await run(baseState, followupText([{ answer: "Yes, proceed" }])) + + expect(result.decision).toBe("timeout") + if (result.decision === "timeout") { + expect(result.timeout).toBe(10_000) + expect(result.fn()).toEqual({ + askResponse: "messageResponse", + text: "Yes, proceed", + }) + } + }) + + it("falls back to asking when the follow-up has no text payload", async () => { + // Exercises the `text || "{}"` fallback: a follow-up without any payload must + // not schedule an auto-answer timeout. + const result = await checkAutoApproval({ state: baseState, ask: "followup" }) + + expect(result).toEqual({ decision: "ask" }) + }) + + it("skips a blank or missing first answer and uses the next valid suggestion (issue #1226)", async () => { + // Mirrors a malformed model response where JSON round-tripping drops + // `answer: undefined` and the first item is unusable. + const result = await run(baseState, followupText([{}, { answer: " " }, { answer: "Valid answer" }])) + + expect(result.decision).toBe("timeout") + if (result.decision === "timeout") { + expect(result.fn()).toEqual({ + askResponse: "messageResponse", + text: "Valid answer", + }) + } + }) + + it("skips a non-string first answer and uses the next valid suggestion (issue #1226)", async () => { + // A malformed payload may carry a non-string `answer`; the first usable + // suggestion still wins, so the auto-answer must not be dropped. + const result = await run(baseState, followupText([{ answer: 42 }, { answer: "Valid answer" }])) + + expect(result.decision).toBe("timeout") + if (result.decision === "timeout") { + expect(result.fn()).toEqual({ + askResponse: "messageResponse", + text: "Valid answer", + }) + } + }) + + it("falls back to asking when every suggestion answer is blank or missing (issue #1226)", async () => { + // Before the #1226 fix this scheduled a timeout that auto-answered the + // follow-up with `undefined` text, silently accepting an empty answer. + const result = await run(baseState, followupText([{ answer: "" }, { answer: " \n\t " }, {}])) + + expect(result).toEqual({ decision: "ask" }) + }) + + it("falls back to asking when the suggestion answer is not a string", async () => { + const result = await run(baseState, followupText([{ answer: 42 }])) + + expect(result).toEqual({ decision: "ask" }) + }) + + it("falls back to asking when the follow-up has no suggestions", async () => { + const result = await run(baseState, JSON.stringify({ question: "Pick one?" })) + + expect(result).toEqual({ decision: "ask" }) + }) + + it("falls back to asking when the follow-up text is not valid JSON", async () => { + const result = await run(baseState, "not-json") + + expect(result).toEqual({ decision: "ask" }) + }) + + it("falls back to asking when the auto-approve timeout is not positive", async () => { + const result = await run({ ...baseState, followupAutoApproveTimeoutMs: 0 }, followupText([{ answer: "Yes" }])) + + expect(result).toEqual({ decision: "ask" }) + }) + + it("does not auto-approve when follow-up auto-approval is disabled", async () => { + const result = await run( + { ...baseState, alwaysAllowFollowupQuestions: false }, + followupText([{ answer: "Yes" }]), + ) + + expect(result).toEqual({ decision: "ask" }) + }) + + it("does not auto-approve when global auto-approval is disabled", async () => { + const result = await run({ ...baseState, autoApprovalEnabled: false }, followupText([{ answer: "Yes" }])) + + expect(result).toEqual({ decision: "ask" }) + }) +}) diff --git a/src/core/auto-approval/index.ts b/src/core/auto-approval/index.ts index 751b5c0674..2fc4d1d45e 100644 --- a/src/core/auto-approval/index.ts +++ b/src/core/auto-approval/index.ts @@ -1,4 +1,5 @@ import { + firstUsableSuggestion, type ClineAsk, type ClineSayTool, type McpServerUse, @@ -179,7 +180,11 @@ export async function checkAutoApproval({ if (ask === "followup") { if (state.alwaysAllowFollowupQuestions === true) { try { - const suggestion = (JSON.parse(text || "{}") as FollowUpData).suggest?.[0] + // A missing or blank answer would auto-approve the follow-up with no + // content after the timeout (issue #1226), so pick the first suggestion + // with a usable answer. This mirrors the webview's visible-suggestions + // filter in FollowUpSuggest. + const suggestion = firstUsableSuggestion((JSON.parse(text || "{}") as FollowUpData).suggest) if ( suggestion && diff --git a/webview-ui/src/components/chat/ChatTextArea.tsx b/webview-ui/src/components/chat/ChatTextArea.tsx index 3721021637..2c539766fe 100644 --- a/webview-ui/src/components/chat/ChatTextArea.tsx +++ b/webview-ui/src/components/chat/ChatTextArea.tsx @@ -83,6 +83,11 @@ export const ChatTextArea = forwardRef( }, ref, ) => { + // A malformed follow-up answer can push a non-string value into the input state + // (issue #1226). Normalize once so every string operation below (trim, slice, + // indexing, paste, drop, and the textarea value) is safe. + const normalizedInputValue = typeof inputValue === "string" ? inputValue : "" + const { t } = useAppTranslation() const { filePaths, @@ -160,7 +165,7 @@ export const ChatTextArea = forwardRef( if (message.text && textAreaRef.current) { // Insert the command text at the current cursor position const textarea = textAreaRef.current - const currentValue = inputValue + const currentValue = normalizedInputValue const cursorPos = textarea.selectionStart || 0 // Check if we need to add a space before the command @@ -206,7 +211,7 @@ export const ChatTextArea = forwardRef( window.addEventListener("message", messageHandler) return () => window.removeEventListener("message", messageHandler) - }, [setInputValue, searchRequestId, inputValue]) + }, [setInputValue, searchRequestId, normalizedInputValue]) const [isDraggingOver, setIsDraggingOver] = useState(false) const [textAreaBaseHeight, setTextAreaBaseHeight] = useState(undefined) @@ -229,7 +234,7 @@ export const ChatTextArea = forwardRef( clineMessages, taskHistory, cwd, - inputValue, + inputValue: normalizedInputValue, setInputValue, }) @@ -245,7 +250,7 @@ export const ChatTextArea = forwardRef( }, [selectedType, searchQuery]) const handleEnhancePrompt = useCallback(() => { - const trimmedInput = inputValue.trim() + const trimmedInput = normalizedInputValue.trim() if (trimmedInput) { setIsEnhancingPrompt(true) @@ -253,14 +258,14 @@ export const ChatTextArea = forwardRef( } else { setInputValue(t("chat:enhancePromptDescription")) } - }, [inputValue, setInputValue, t]) + }, [normalizedInputValue, setInputValue, t]) const allModes = useMemo(() => getAllModes(customModes), [customModes]) // Memoized check for whether the input has content (text or images) const hasInputContent = useMemo(() => { - return inputValue.trim().length > 0 || selectedImages.length > 0 - }, [inputValue, selectedImages]) + return normalizedInputValue.trim().length > 0 || selectedImages.length > 0 + }, [normalizedInputValue, selectedImages]) // Compute the key combination text for the send button tooltip based on enterBehavior const sendKeyCombination = useMemo(() => { @@ -509,8 +514,8 @@ export const ChatTextArea = forwardRef( } if (event.key === "Backspace" && !isComposing) { - const charBeforeCursor = inputValue[cursorPosition - 1] - const charAfterCursor = inputValue[cursorPosition + 1] + const charBeforeCursor = normalizedInputValue[cursorPosition - 1] + const charAfterCursor = normalizedInputValue[cursorPosition + 1] const charBeforeIsWhitespace = charBeforeCursor === " " || charBeforeCursor === "\n" || charBeforeCursor === "\r\n" @@ -522,7 +527,7 @@ export const ChatTextArea = forwardRef( if ( charBeforeIsWhitespace && // "$" is added to ensure the match occurs at the end of the string. - inputValue.slice(0, cursorPosition - 1).match(new RegExp(mentionRegex.source + "$")) + normalizedInputValue.slice(0, cursorPosition - 1).match(new RegExp(mentionRegex.source + "$")) ) { const newCursorPosition = cursorPosition - 1 // If mention is followed by another word, then instead @@ -537,9 +542,9 @@ export const ChatTextArea = forwardRef( setCursorPosition(newCursorPosition) setJustDeletedSpaceAfterMention(true) } else if (justDeletedSpaceAfterMention) { - const { newText, newPosition } = removeMention(inputValue, cursorPosition) + const { newText, newPosition } = removeMention(normalizedInputValue, cursorPosition) - if (newText !== inputValue) { + if (newText !== normalizedInputValue) { event.preventDefault() setInputValue(newText) setIntendedCursorPosition(newPosition) // Store the new cursor position in state @@ -559,7 +564,7 @@ export const ChatTextArea = forwardRef( selectedMenuIndex, handleMentionSelect, selectedType, - inputValue, + normalizedInputValue, cursorPosition, setInputValue, justDeletedSpaceAfterMention, @@ -578,7 +583,7 @@ export const ChatTextArea = forwardRef( textAreaRef.current.setSelectionRange(intendedCursorPosition, intendedCursorPosition) setIntendedCursorPosition(null) // Reset the state. } - }, [inputValue, intendedCursorPosition]) + }, [normalizedInputValue, intendedCursorPosition]) // Ref to store the search timeout. const searchTimeoutRef = useRef(null) @@ -678,7 +683,10 @@ export const ChatTextArea = forwardRef( e.preventDefault() const trimmedUrl = pastedText.trim() const newValue = - inputValue.slice(0, cursorPosition) + trimmedUrl + " " + inputValue.slice(cursorPosition) + normalizedInputValue.slice(0, cursorPosition) + + trimmedUrl + + " " + + normalizedInputValue.slice(cursorPosition) setInputValue(newValue) const newCursorPosition = cursorPosition + trimmedUrl.length + 1 setCursorPosition(newCursorPosition) @@ -741,7 +749,7 @@ export const ChatTextArea = forwardRef( } } }, - [shouldDisableImages, setSelectedImages, cursorPosition, setInputValue, inputValue, t], + [shouldDisableImages, setSelectedImages, cursorPosition, setInputValue, normalizedInputValue, t], ) const handleMenuMouseDown = useCallback(() => { @@ -791,7 +799,7 @@ export const ChatTextArea = forwardRef( useLayoutEffect(() => { updateHighlights() - }, [inputValue, updateHighlights]) + }, [normalizedInputValue, updateHighlights]) const updateCursorPosition = useCallback(() => { if (textAreaRef.current) { @@ -823,7 +831,7 @@ export const ChatTextArea = forwardRef( if (lines.length > 0) { // Process each line as a separate file path - let newValue = inputValue.slice(0, cursorPosition) + let newValue = normalizedInputValue.slice(0, cursorPosition) let totalLength = 0 // Using a standard for loop instead of forEach for potential performance gains. @@ -842,7 +850,7 @@ export const ChatTextArea = forwardRef( } // Add space after the last mention and append the rest of the input - newValue += " " + inputValue.slice(cursorPosition) + newValue += " " + normalizedInputValue.slice(cursorPosition) totalLength += 1 setInputValue(newValue) @@ -903,7 +911,7 @@ export const ChatTextArea = forwardRef( [ cursorPosition, cwd, - inputValue, + normalizedInputValue, setInputValue, setCursorPosition, setIntendedCursorPosition, @@ -996,7 +1004,7 @@ export const ChatTextArea = forwardRef( ( } textAreaRef.current = el }} - value={inputValue} + value={normalizedInputValue} onChange={(e) => { handleInputChange(e) updateHighlights() @@ -1256,7 +1264,7 @@ export const ChatTextArea = forwardRef( - {!inputValue && ( + {!normalizedInputValue && (
{ + // The model may emit suggestions with missing or blank answers (issue #1226). + // Ignore them instead of pushing an undefined value into the input, which + // would crash the text area (inputValue.trim on undefined). + const answer = hasUsableAnswer(suggestion) ? suggestion.answer.trim() : "" + if (!answer) { + return + } + // Mark that user has responded if this is a manual click (not auto-approval) if (event) { userRespondedRef.current = true @@ -1455,13 +1463,13 @@ const ChatViewComponent: React.ForwardRefRenderFunction { - return currentValue !== "" ? `${currentValue} \n${suggestion.answer}` : suggestion.answer + return currentValue !== "" ? `${currentValue} \n${answer}` : answer }) } else { // Don't clear the input value when sending a follow-up choice // The message should be sent but the text area should preserve what the user typed const preservedInput = inputValueRef.current - handleSendMessage(suggestion.answer, []) + handleSendMessage(answer, []) // Restore the input value after sending setInputValue(preservedInput) } diff --git a/webview-ui/src/components/chat/FollowUpSuggest.tsx b/webview-ui/src/components/chat/FollowUpSuggest.tsx index 42b41bacfa..4fa6f9ab1d 100644 --- a/webview-ui/src/components/chat/FollowUpSuggest.tsx +++ b/webview-ui/src/components/chat/FollowUpSuggest.tsx @@ -1,11 +1,11 @@ -import { useCallback, useEffect, useState } from "react" +import { useCallback, useEffect, useMemo, useState } from "react" import { ClipboardCopy, Timer } from "lucide-react" import { Button, StandardTooltip } from "@/components/ui" import { useAppTranslation } from "@src/i18n/TranslationContext" import { useExtensionState } from "@src/context/ExtensionStateContext" -import { getSuggestionMode, type SuggestionItem } from "@roo-code/types" +import { getSuggestionMode, hasUsableAnswer, type SuggestionItem } from "@roo-code/types" import { cn } from "@/lib/utils" const DEFAULT_FOLLOWUP_TIMEOUT_MS = 60000 @@ -33,6 +33,14 @@ export const FollowUpSuggest = ({ const [suggestionSelected, setSuggestionSelected] = useState(false) const { t } = useAppTranslation() + // The model may emit suggestions with missing or blank answers. Those render + // as empty buttons, and clicking "Copy to input" on one pushes an undefined + // string into the chat input and crashes the text area (issue #1226), so hide them. + const visibleSuggestions = useMemo( + () => suggestions.filter((suggestion) => hasUsableAnswer(suggestion)), + [suggestions], + ) + // Start countdown timer when auto-approval is enabled for follow-up questions useEffect(() => { // Only start countdown if auto-approval is enabled for follow-up questions and no suggestion has been selected @@ -40,7 +48,7 @@ export const FollowUpSuggest = ({ if ( autoApprovalEnabled && alwaysAllowFollowupQuestions && - suggestions.length > 0 && + visibleSuggestions.length > 0 && !suggestionSelected && !isAnswered && !isFollowUpAutoApprovalPaused @@ -78,7 +86,7 @@ export const FollowUpSuggest = ({ }, [ autoApprovalEnabled, alwaysAllowFollowupQuestions, - suggestions, + visibleSuggestions, followupAutoApproveTimeoutMs, suggestionSelected, onCancelAutoApproval, @@ -102,14 +110,14 @@ export const FollowUpSuggest = ({ [onSuggestionClick, onCancelAutoApproval], ) - // Don't render if there are no suggestions or no click handler. - if (!suggestions?.length || !onSuggestionClick) { + // Don't render if there are no visible suggestions or no click handler. + if (visibleSuggestions.length === 0 || !onSuggestionClick) { return null } return (
- {suggestions.map((suggestion, index) => { + {visibleSuggestions.map((suggestion, index) => { const isFirstSuggestion = index === 0 const suggestionMode = getSuggestionMode(suggestion.mode) diff --git a/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx b/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx index a3a5558748..aae9ac3cc6 100644 --- a/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx @@ -1,7 +1,7 @@ import { providerIdentifiers } from "@roo-code/types" import { defaultModeSlug } from "@roo/modes" -import { render, fireEvent, screen } from "@src/utils/test-utils" +import { render, fireEvent, screen, act } from "@src/utils/test-utils" import { useExtensionState } from "@src/context/ExtensionStateContext" import { vscode } from "@src/utils/vscode" import * as pathMentions from "@src/utils/path-mentions" @@ -1206,4 +1206,295 @@ describe("ChatTextArea", () => { expect(sendButton).toHaveClass("pointer-events-auto") }) }) + + describe("blank suggestion copy crash (issue #1226)", () => { + const getSendButton = (container: HTMLElement) => { + const buttons = container.querySelectorAll("button") + return Array.from(buttons).find((button) => button.querySelector(".lucide-send-horizontal") !== null) + } + + it("renders without crashing and treats an undefined inputValue as empty", () => { + // Intentionally pass the malformed value that #1226 produced at runtime: + // clicking "Copy to input" on an empty follow-up suggestion pushed + // `undefined` into the input state. + const undefinedInput = { ...defaultProps, inputValue: undefined } as unknown as typeof defaultProps + const { container } = render() + + // Before the #1226 fix, mounting with an undefined inputValue threw + // "Cannot read properties of undefined (reading 'trim')" in the + // hasInputContent memo. It should render normally instead. + + // The normalized value drives the textarea: a malformed input renders as empty. + const textarea = container.querySelector("textarea") + expect(textarea).toBeInTheDocument() + expect(textarea).toHaveValue("") + + // Clicking "Enhance prompt" with an undefined input must not crash either: + // the undefined input behaves like an empty one, so nothing is sent. + mockPostMessage.mockClear() + fireEvent.click(getEnhancePromptButton()) + expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "enhancePrompt" })) + + // An undefined input behaves like an empty input: no content to send. + const sendButton = getSendButton(container) + expect(sendButton).toBeInTheDocument() + expect(sendButton).toHaveClass("opacity-0") + expect(sendButton).toHaveClass("pointer-events-none") + }) + + it.each([0, false, { answer: "nope" }])("treats a non-string inputValue of %p as empty", (value) => { + // Only nullish values were normalized before the #1226 follow-up fix; + // any other non-string value must be treated as empty, not crash. + const badInput = { ...defaultProps, inputValue: value } as unknown as typeof defaultProps + const { container } = render() + + // The normalized value drives the textarea: a non-string input renders as empty. + const textarea = container.querySelector("textarea") + expect(textarea).toBeInTheDocument() + expect(textarea).toHaveValue("") + + mockPostMessage.mockClear() + fireEvent.click(getEnhancePromptButton()) + expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "enhancePrompt" })) + + const sendButton = getSendButton(container) + expect(sendButton).toBeInTheDocument() + expect(sendButton).toHaveClass("opacity-0") + expect(sendButton).toHaveClass("pointer-events-none") + }) + }) + + describe("string operations on the normalized input (issue #1226)", () => { + const getTextarea = (container: HTMLElement) => { + const textarea = container.querySelector("textarea") + if (!textarea) { + throw new Error("expected the chat textarea to be rendered") + } + return textarea + } + + it("inserts command text at the cursor via the insertTextIntoTextarea message", () => { + render() + + act(() => { + window.dispatchEvent( + new MessageEvent("message", { data: { type: "insertTextIntoTextarea", text: "/command" } }), + ) + }) + + // The cursor starts at 0, so the command is prepended with a trailing space. + expect(defaultProps.setInputValue).toHaveBeenCalledWith("/command hello ") + }) + + it("inspects the characters around the cursor on Backspace without crashing", () => { + const { container } = render() + const textarea = getTextarea(container) + + fireEvent.keyDown(textarea, { key: "Backspace" }) + + // Plain text: no mention manipulation, so the input is untouched. + expect(defaultProps.setInputValue).not.toHaveBeenCalled() + }) + + it("removes a mention on the second Backspace after deleting the space after it", () => { + const { container } = render() + const textarea = getTextarea(container) + + // Position the cursor after the trailing space and tell the component about it. + textarea.setSelectionRange(16, 16) + fireEvent.mouseUp(textarea) + + // First Backspace: drops the space after the mention and arms the pending flag. + fireEvent.keyDown(textarea, { key: "Backspace" }) + + // Second Backspace: removes the mention itself. + fireEvent.keyDown(textarea, { key: "Backspace" }) + + expect(defaultProps.setInputValue).toHaveBeenCalledWith("hello ") + }) + + it("resets the pending mention flag without changing the input when the cursor is not after a mention", () => { + const { container } = render() + const textarea = getTextarea(container) + + // Arm the pending flag with the first Backspace right after the mention. + textarea.setSelectionRange(16, 16) + fireEvent.mouseUp(textarea) + fireEvent.keyDown(textarea, { key: "Backspace" }) + + // Move the cursor away from the mention before the next Backspace. + textarea.setSelectionRange(2, 2) + fireEvent.mouseUp(textarea) + fireEvent.keyDown(textarea, { key: "Backspace" }) + + // removeMention finds no mention at the cursor, so the input is untouched. + expect(defaultProps.setInputValue).not.toHaveBeenCalled() + }) + + it("adds a trailing space after a pasted URL", () => { + const { container } = render() + const textarea = getTextarea(container) + + const pasteEvent = new window.Event("paste", { bubbles: true, cancelable: true }) + Object.defineProperty(pasteEvent, "clipboardData", { + value: { items: [], getData: () => "https://example.com" }, + }) + + act(() => { + textarea.dispatchEvent(pasteEvent) + }) + + // The URL is inserted at cursor position 0, followed by a space. + expect(defaultProps.setInputValue).toHaveBeenCalledWith("https://example.com visit ") + }) + + it("uses the re-rendered input when inserting command text", () => { + const { container, rerender } = render() + rerender() + // Reset the DOM cursor: JSDOM moves the selection to the end of a + // re-rendered value, so anchor the insertion at position 0 explicitly. + getTextarea(container).setSelectionRange(0, 0) + + act(() => { + window.dispatchEvent( + new MessageEvent("message", { data: { type: "insertTextIntoTextarea", text: "/command" } }), + ) + }) + + // A stale effect would still insert into the original "hello " value. + expect(defaultProps.setInputValue).toHaveBeenCalledWith("/command updated ") + }) + + it("posts the trimmed input on enhance and follows a re-rendered value", () => { + const { rerender } = render() + + fireEvent.click(getEnhancePromptButton()) + // Surrounding whitespace must not reach the extension. + expect(mockPostMessage).toHaveBeenCalledWith({ type: "enhancePrompt", text: "Test prompt" }) + + rerender() + fireEvent.click(getEnhancePromptButton()) + // A stale handler would re-post the original "Test prompt" value. + expect(mockPostMessage).toHaveBeenLastCalledWith({ type: "enhancePrompt", text: "updated" }) + }) + + it("keeps whitespace-only input hidden from the send button and placeholder", () => { + const getSendButton = (container: HTMLElement) => + Array.from(container.querySelectorAll("button")).find((b) => b.querySelector(".lucide-send-horizontal")) + const { container, rerender } = render() + + // Whitespace-only input has no sendable content, and the placeholder only + // renders for a truly empty value. + expect(getSendButton(container)).toHaveClass("opacity-0") + expect(container.querySelector(".left-2.z-30")).toBeNull() + + // A stale content memo would keep the send button hidden for real text. + rerender() + expect(getSendButton(container)).toHaveClass("opacity-100") + }) + + it("moves the cursor to the end of the mention on Backspace at the end of the input", () => { + const { container } = render() + const textarea = getTextarea(container) + + textarea.setSelectionRange(16, 16) + fireEvent.mouseUp(textarea) + fireEvent.keyDown(textarea, { key: "Backspace" }) + + // Nothing follows the trailing space, so the space is not deleted; the + // cursor moves to the end of the mention instead. + expect(textarea.selectionStart).toBe(15) + expect(defaultProps.setInputValue).not.toHaveBeenCalled() + }) + + it("does not intercept Backspace when a word follows the mention", () => { + const { container } = render() + const textarea = getTextarea(container) + + textarea.setSelectionRange(15, 15) + fireEvent.mouseUp(textarea) + fireEvent.keyDown(textarea, { key: "Backspace" }) + + // The mention is not at the end of the inspected prefix, so the + // selection is untouched. + expect(textarea.selectionStart).toBe(15) + expect(defaultProps.setInputValue).not.toHaveBeenCalled() + }) + + it("applies the pending cursor once the pasted value commits", () => { + const { container, rerender } = render() + const textarea = getTextarea(container) + + const pasteEvent = new window.Event("paste", { bubbles: true, cancelable: true }) + Object.defineProperty(pasteEvent, "clipboardData", { + value: { items: [], getData: () => "https://example.com" }, + }) + + act(() => { + textarea.dispatchEvent(pasteEvent) + // Commit the value the paste handler produced so the pending cursor + // (0 + 19 + 1 = 20) is applied against the full 26-char value. + rerender() + }) + + // A stale effect would never re-apply the intended cursor position. + expect(defaultProps.setInputValue).toHaveBeenLastCalledWith("https://example.com visit ") + expect(textarea.selectionStart).toBe(20) + }) + + it("inserts a pasted URL at the cursor and tracks re-rendered input", () => { + const { container, rerender } = render() + const textarea = getTextarea(container) + + textarea.setSelectionRange(6, 6) + fireEvent.mouseUp(textarea) + + const pasteEvent = new window.Event("paste", { bubbles: true, cancelable: true }) + Object.defineProperty(pasteEvent, "clipboardData", { + value: { items: [], getData: () => "https://example.com" }, + }) + + act(() => { + textarea.dispatchEvent(pasteEvent) + }) + + // The URL splits "visit this" at the cursor instead of reusing the whole + // value for the tail. + expect(defaultProps.setInputValue).toHaveBeenLastCalledWith("visit https://example.com this") + + rerender() + textarea.setSelectionRange(0, 0) + fireEvent.mouseUp(textarea) + act(() => { + textarea.dispatchEvent(pasteEvent) + }) + // A stale paste handler would repeat the original "visit this" value. + expect(defaultProps.setInputValue).toHaveBeenLastCalledWith("https://example.com changed ") + }) + + it("re-highlights mentions after the input changes", () => { + const { container, rerender } = render() + rerender() + + // A stale highlight effect would keep the empty highlight from the + // initial render. + expect(container.querySelector('[data-testid="highlight-layer"]')?.innerHTML).toContain(" { + const { container } = render() + const textarea = getTextarea(container) + + textarea.setSelectionRange(3, 3) + fireEvent.mouseUp(textarea) + + fireEvent.drop(container.querySelector(".chat-text-area")!, { + dataTransfer: { getData: () => "/some/path", files: [] }, + preventDefault: vi.fn(), + }) + + // The remainder after the cursor ("def") is preserved. + expect(defaultProps.setInputValue).toHaveBeenCalledWith("abc/some/path def") + }) + }) }) diff --git a/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx b/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx index 6b2fa177c9..8f7de5c459 100644 --- a/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx @@ -54,19 +54,22 @@ vi.mock("../ChatRow", () => ({ default: function MockChatRow({ message, onSuggestionClick, + isFollowUpAnswered, }: { message: ClineMessage onSuggestionClick?: (suggestion: SuggestionItem, event?: React.MouseEvent) => void + isFollowUpAnswered?: boolean }) { if (message.type === "ask" && message.ask === "followup" && message.text) { try { const followUp = JSON.parse(message.text) as { suggest?: SuggestionItem[] } return ( -
+
{followUp.suggest?.map((suggestion) => ( @@ -1460,6 +1463,158 @@ describe("ChatView - Follow-up Suggestions", () => { }) expect(vscode.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "mode" })) }) + + it("ignores a blank or missing suggestion answer instead of crashing (issue #1226)", async () => { + const { getAllByTestId } = renderChatView() + + // JSON.stringify drops `answer: undefined`, mirroring how the extension + // delivers a malformed follow-up suggestion (no `answer` property). + mockPostMessage({ + mode: "ask", + customModes: [], + clineMessages: [ + { + type: "say", + say: "task", + ts: Date.now() - 1000, + text: "Initial task", + }, + { + type: "ask", + ask: "followup", + ts: Date.now(), + text: JSON.stringify({ + question: "Pick one?", + suggest: [{ answer: undefined }, { answer: "Valid answer" }], + }), + partial: false, + }, + ], + }) + + const suggestionButtons = await waitFor(() => { + const buttons = getAllByTestId("followup-suggestion") + if (buttons.length !== 2) { + throw new Error(`expected 2 suggestion buttons, got ${buttons.length}`) + } + return buttons + }) + vscodePostMessageMock.cleanup() + + // Clicking the blank suggestion must be ignored: no response is sent and + // no undefined value is pushed into the input state. + fireEvent.click(suggestionButtons[0]) + + expect(vscode.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "askResponse" })) + expect(vscode.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "mode" })) + // The ignored click must not mark the follow-up as answered either. + expect(suggestionButtons[0].closest('[data-testid="chat-row"]')?.getAttribute("data-answered")).toBe("false") + + // The valid suggestion still sends its answer. + fireEvent.click(suggestionButtons[1]) + + await waitFor(() => { + expect(vscode.postMessage).toHaveBeenCalledWith({ + type: "askResponse", + askResponse: "messageResponse", + text: "Valid answer", + images: [], + }) + }) + }) + + it("appends a valid suggestion to the input on shift-click without sending it (issue #1226)", async () => { + const { getByTestId, getByRole } = renderChatView() + + mockPostMessage({ + mode: "ask", + customModes: [], + clineMessages: [ + { + type: "say", + say: "task", + ts: Date.now() - 1000, + text: "Initial task", + }, + { + type: "ask", + ask: "followup", + ts: Date.now(), + text: JSON.stringify({ + question: "Pick one?", + suggest: [{ answer: undefined }, { answer: "Copy me" }], + }), + partial: false, + }, + ], + }) + + const suggestion = await waitFor(() => getByRole("button", { name: "Copy me" })) + vscodePostMessageMock.cleanup() + + // Pre-fill the draft, then shift-click ("Copy to input") the valid suggestion. + const input = getByTestId("chat-textarea").querySelector("input") + if (!input) { + throw new Error("expected the chat input to be rendered") + } + + fireEvent.change(input, { target: { value: "Draft text" } }) + + fireEvent.click(suggestion, { shiftKey: true }) + + // The answer is appended to the existing draft instead of being sent. + // JSDOM strips line breaks from values (HTML spec "strip newlines"), + // so the appended "\n" is absent from the DOM value. + await waitFor(() => expect(input.value).toBe("Draft text Copy me")) + expect(vscode.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "askResponse" })) + + // With an empty draft the answer is set as-is (no dangling "\n" prefix). + fireEvent.change(input, { target: { value: "" } }) + fireEvent.click(suggestion, { shiftKey: true }) + await waitFor(() => expect(input.value).toBe("Copy me")) + expect(vscode.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "askResponse" })) + }) + + it("trims a padded suggestion answer before appending it on shift-click (issue #1226)", async () => { + const { getByTestId, getByRole } = renderChatView() + + mockPostMessage({ + mode: "ask", + customModes: [], + clineMessages: [ + { + type: "say", + say: "task", + ts: Date.now() - 1000, + text: "Initial task", + }, + { + type: "ask", + ask: "followup", + ts: Date.now(), + text: JSON.stringify({ + question: "Pick one?", + suggest: [{ answer: " Padded " }], + }), + partial: false, + }, + ], + }) + + const suggestion = await waitFor(() => getByRole("button", { name: "Padded" })) + vscodePostMessageMock.cleanup() + + const input = getByTestId("chat-textarea").querySelector("input") + if (!input) { + throw new Error("expected the chat input to be rendered") + } + + // The padded answer reaches the input trimmed (issue #1226). + fireEvent.click(suggestion, { shiftKey: true }) + + await waitFor(() => expect(input.value).toBe("Padded")) + expect(vscode.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "askResponse" })) + }) }) describe("ChatView - Context Condensing Indicator Tests", () => { diff --git a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx index a46df75b80..9f437d685b 100644 --- a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx @@ -1,6 +1,7 @@ import React, { createContext, useContext } from "react" import { render, screen, act } from "@testing-library/react" import { TooltipProvider } from "@radix-ui/react-tooltip" +import type { SuggestionItem } from "@roo-code/types" import { FollowUpSuggest } from "../FollowUpSuggest" @@ -707,4 +708,110 @@ describe("FollowUpSuggest", () => { expect(mockOnCancelAutoApproval).toHaveBeenCalled() }) }) + + describe("suggestions with blank or missing answers (issue #1226)", () => { + /** + * Malformed follow-up payloads as they can arrive from the model: the + * extension-host transport does not validate `FollowUpData`, so a + * suggestion item may be missing `answer` or carry a blank value. + * `SuggestionItem` models a missing or blank `answer` (see + * `hasUsableAnswer`) but not a non-string one, so the fixtures keep + * their malformed shape and cross into the component prop at one + * documented boundary instead of disabling type checks with `any`. + */ + type MalformedSuggestionPayload = { answer?: unknown } + + const renderWithMalformedSuggestions = (payloads: MalformedSuggestionPayload[]) => { + // Documented double assertion at the component boundary: FollowUpSuggest + // must tolerate malformed transport data, which `SuggestionItem` + // cannot fully express (issue #1226). + const suggestions = payloads as unknown as SuggestionItem[] + + return renderWithTestProviders( + , + defaultTestState, + ) + } + + it("should not render anything when all answers are blank, missing, or non-string", () => { + const { container } = renderWithMalformedSuggestions([ + { answer: "" }, + { answer: " \n\t " }, + { answer: undefined }, + { answer: 42 }, + ]) + + // Blank suggestions are filtered out, so no buttons or countdown render. + expect(container.firstChild).toBeNull() + }) + + it("should only render suggestions with non-blank answers", () => { + renderWithMalformedSuggestions([ + { answer: "" }, + { answer: "Valid suggestion" }, + { answer: undefined }, + { answer: " " }, + ]) + + expect(screen.getByText("Valid suggestion")).toBeInTheDocument() + expect(screen.queryAllByRole("button")).toHaveLength(1) + }) + + it("should not start the auto-approve countdown when all answers are blank", () => { + renderWithMalformedSuggestions([{ answer: "" }, { answer: undefined }]) + + vi.advanceTimersByTime(10000) + + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + // Auto-approval must not select a blank suggestion. + expect(mockOnSuggestionClick).not.toHaveBeenCalled() + }) + + it("does not call the cancel callback on unmount when no usable suggestion starts the countdown", () => { + const { unmount } = renderWithMalformedSuggestions([{ answer: "" }]) + + unmount() + + // No countdown started, so unmounting must not cancel a backend timeout. + expect(mockOnCancelAutoApproval).not.toHaveBeenCalled() + }) + }) + + describe("stale visible-suggestions memo (issue #1226)", () => { + it("re-renders the suggestion list when the suggestions prop changes", () => { + const { rerender } = renderWithTestProviders( + , + defaultTestState, + ) + + expect(screen.getByText("First")).toBeInTheDocument() + + // A stale memo would keep rendering the original suggestion. + rerender( + + + + + , + ) + + expect(screen.getByText("Second")).toBeInTheDocument() + expect(screen.queryByText("First")).not.toBeInTheDocument() + }) + }) })