From 02827208f6e35ff153ef6719cf31f8d1cbf273b0 Mon Sep 17 00:00:00 2001 From: nova28 Date: Tue, 18 Aug 2026 06:57:51 +0000 Subject: [PATCH] fix(web): stop Escape from silently rejecting clarification questions (#2729) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- .../quick-chat/quick-chat-content.tsx | 99 +------ .../quick-chat/quick-chat-modal.test.tsx | 171 ++++++++--- .../quick-chat/quick-chat-modal.tsx | 14 +- .../chat/clarification-input-overlay.test.tsx | 137 +++++++++ .../task/chat/clarification-input-overlay.tsx | 107 ++----- .../chat/clarification-overlay-header.tsx | 119 ++++++++ .../task/chat/clarification-overlay-parts.tsx | 2 +- .../chat/clarification-panel-section.test.tsx | 276 ++++++++++++++++++ .../task/chat/clarification-panel-section.tsx | 160 ++++++++++ apps/web/components/task/task-chat-panel.tsx | 77 +---- apps/web/e2e/pages/session-page.ts | 16 + .../tests/chat/clarification-resize.spec.ts | 72 ++++- apps/web/e2e/tests/chat/clarification.spec.ts | 220 +++++++++++++- apps/web/e2e/tests/chat/quick-chat.spec.ts | 32 ++ .../use-clarification-escape-guard.test.tsx | 35 +++ .../hooks/use-clarification-escape-guard.ts | 25 ++ ...se-resizable-clarification-overlay.test.ts | 26 +- .../use-resizable-clarification-overlay.ts | 22 +- 18 files changed, 1302 insertions(+), 308 deletions(-) create mode 100644 apps/web/components/task/chat/clarification-input-overlay.test.tsx create mode 100644 apps/web/components/task/chat/clarification-overlay-header.tsx create mode 100644 apps/web/components/task/chat/clarification-panel-section.test.tsx create mode 100644 apps/web/components/task/chat/clarification-panel-section.tsx create mode 100644 apps/web/hooks/use-clarification-escape-guard.test.tsx create mode 100644 apps/web/hooks/use-clarification-escape-guard.ts diff --git a/apps/web/components/quick-chat/quick-chat-content.tsx b/apps/web/components/quick-chat/quick-chat-content.tsx index 40f20339be..9088f88957 100644 --- a/apps/web/components/quick-chat/quick-chat-content.tsx +++ b/apps/web/components/quick-chat/quick-chat-content.tsx @@ -1,8 +1,6 @@ "use client"; -import { memo, useCallback, useEffect, useId, useRef, useState, type RefObject } from "react"; -import { IconChevronDown, IconChevronUp, IconMessageQuestion } from "@tabler/icons-react"; -import { Button } from "@kandev/ui/button"; +import { memo, useCallback, useEffect, useRef, useState } from "react"; import { useSettingsData } from "@/hooks/domains/settings/use-settings-data"; import { type ChatInputContainerHandle } from "@/components/task/chat/chat-input-container"; import { MessageList } from "@/components/task/chat/message-list"; @@ -12,13 +10,9 @@ import { useSubmitHandler, useChatPanelHandlers, } from "@/components/task/chat/chat-input-area"; -import { ClarificationInputOverlay } from "@/components/task/chat/clarification-input-overlay"; -import { ResizeHandle } from "@/components/task/chat/resize-handle"; -import { useResizableClarificationOverlay } from "@/hooks/use-resizable-clarification-overlay"; -import type { Message } from "@/lib/types/http"; +import { ClarificationPanelSection } from "@/components/task/chat/clarification-panel-section"; import { getSessionWorkspacePath } from "@/lib/session-workspace-path"; import { routePanelMouseDown } from "@/components/task/chat/route-panel-mouse-down"; -import { useTranslation } from "react-i18next"; type QuickChatContentProps = { sessionId: string; @@ -28,92 +22,6 @@ type QuickChatContentProps = { onInitialPromptSent?: () => void; }; -type QuickChatClarificationSectionProps = { - pending: boolean; - messages: readonly Message[] | null | undefined; - onResolved: () => void; - shortcutScopeRef: RefObject; -}; - -function QuickChatClarificationSection({ - pending, - messages, - onResolved, - shortcutScopeRef, -}: QuickChatClarificationSectionProps) { - const { t } = useTranslation(); - const [collapsed, setCollapsed] = useState(false); - const contentId = useId(); - const { height, containerRef, resetHeight, resizeHandleProps } = - useResizableClarificationOverlay(); - - // A newly opened clarification starts expanded and auto-sized. Collapsing - // an active one leaves its form state and user-selected height intact. - useEffect(() => { - if (!pending) { - setCollapsed(false); - resetHeight(); - } - }, [pending, resetHeight]); - - if (!pending) return null; - - const actionLabel = collapsed ? t("chat:expandClarification") : t("chat:collapseClarification"); - - return ( -
- {!collapsed && } -
-
-
- - {t("chat:clarificationNeeded")} -
- -
-
- -
-
-
- ); -} - function useQuickChatState(sessionId: string) { const chatInputRef = useRef(null); @@ -190,12 +98,13 @@ export const QuickChatContent = memo(function QuickChatContent({ onOpenFile={undefined} /> - ({ useAppStore: ( @@ -27,7 +32,24 @@ vi.mock("@/lib/routing/client-dynamic", () => ({ vi.mock("@kandev/ui/dialog", () => ({ Dialog: ({ open, children }: { open: boolean; children: React.ReactNode }) => open ?
{children}
: null, - DialogContent: ({ children }: { children: React.ReactNode }) =>
{children}
, + DialogContent: ({ + children, + onEscapeKeyDown, + }: { + children: React.ReactNode; + onEscapeKeyDown?: (event: { preventDefault: () => void }) => void; + }) => ( +
+ {children} + {/* Stands in for Radix's DismissableLayer consulting onEscapeKeyDown + before deciding whether to close the dialog on a real Escape press. */} +
+ ), DialogTitle: ({ children }: { children: React.ReactNode }) =>
{children}
, })); @@ -73,8 +95,22 @@ vi.mock("@/components/config-chat/use-config-chat", () => ({ vi.mock("./quick-chat-delete-dialog", () => ({ QuickChatDeleteDialog: () => null, })); + +function EscapeGuardProbe() { + const [guarded, setGuarded] = useState(false); + useClarificationEscapeGuard(guarded); + return ( +
+
+ ); +} vi.mock("./quick-chat-session-view", () => ({ - QuickChatSessionView: () =>
, + QuickChatSessionView: () => , })); vi.mock("./quick-chat-setup", () => ({ QuickChatSetup: () =>
, @@ -82,54 +118,57 @@ vi.mock("./quick-chat-setup", () => ({ vi.mock("@/components/config-chat/config-chat-setup", () => ({ ConfigChatSetup: () =>
, })); +const useQuickChatModalMock = vi.fn(() => defaultQuickChatModalState); vi.mock("./use-quick-chat-modal", () => ({ - useQuickChatModal: () => ({ - isOpen: true, - sessions: [{ sessionId: "chat-1", workspaceId: WORKSPACE_ID, kind: "chat", name: CHAT_NAME }], - terminalTabs: [ - { - tabId: TERMINAL_TAB_ID, - workspaceId: WORKSPACE_ID, - sessionId: "pty-1", - sequence: 1, - status: "running", - }, - ], - activeKind: "terminal", - activeSessionId: "chat-1", - activeTerminalTabId: TERMINAL_TAB_ID, - activeTerminalTab: { + useQuickChatModal: () => useQuickChatModalMock(), +})); + +const defaultQuickChatModalState = { + isOpen: true, + sessions: [{ sessionId: "chat-1", workspaceId: WORKSPACE_ID, kind: "chat", name: CHAT_NAME }], + terminalTabs: [ + { tabId: TERMINAL_TAB_ID, workspaceId: WORKSPACE_ID, sessionId: "pty-1", sequence: 1, status: "running", }, - activeSession: { - sessionId: "chat-1", - workspaceId: WORKSPACE_ID, - kind: "chat", - name: CHAT_NAME, - }, - sessionToClose: null, - setupKey: 0, - activeSessionNeedsAgent: true, - pendingAgentId: null, - setActiveQuickChatSession: vi.fn(), - setSessionToClose: vi.fn(), - handleOpenChange, - handleNewChat, - handleNewTerminal, - handleActivateTerminal, - handleTerminalStateChange: vi.fn(), - handleSetupKindChange: vi.fn(), - handleSelectAgent: vi.fn(), - handleCloseTab: vi.fn(), - handleCloseTerminal, - handleConfirmClose: vi.fn(), - handleRename: vi.fn(), - }), -})); + ], + activeKind: "terminal", + activeSessionId: "chat-1", + activeTerminalTabId: TERMINAL_TAB_ID, + activeTerminalTab: { + tabId: TERMINAL_TAB_ID, + workspaceId: WORKSPACE_ID, + sessionId: "pty-1", + sequence: 1, + status: "running", + }, + activeSession: { + sessionId: "chat-1", + workspaceId: WORKSPACE_ID, + kind: "chat", + name: CHAT_NAME, + }, + sessionToClose: null, + setupKey: 0, + activeSessionNeedsAgent: true, + pendingAgentId: null, + setActiveQuickChatSession: vi.fn(), + setSessionToClose: vi.fn(), + handleOpenChange, + handleNewChat, + handleNewTerminal, + handleActivateTerminal, + handleTerminalStateChange: vi.fn(), + handleSetupKindChange: vi.fn(), + handleSelectAgent: vi.fn(), + handleCloseTab: vi.fn(), + handleCloseTerminal, + handleConfirmClose: vi.fn(), + handleRename: vi.fn(), +}; import { QuickChatModal } from "./quick-chat-modal"; @@ -156,3 +195,47 @@ describe("QuickChatModal mixed tabs", () => { expect(screen.queryByTestId("quick-chat-menu-terminal-terminal-1")).toBeNull(); }); }); + +describe("QuickChatModal — Escape with a pending clarification", () => { + function renderWithConversationTab() { + // mockReturnValue (not -Once): QuickChatModal's own escape-guard state + // lives in this component and re-renders it on every guard toggle, which + // re-invokes useQuickChatModal() more than the one time a *Once value + // would cover. + useQuickChatModalMock.mockReturnValue({ + ...defaultQuickChatModalState, + activeKind: "conversation", + activeSessionNeedsAgent: false, + }); + render(); + } + + it("lets Escape close the modal when no clarification is guarding it", () => { + renderWithConversationTab(); + + fireEvent.click(screen.getByTestId(FIRE_ESCAPE_TESTID)); + + expect(escapePreventDefault).not.toHaveBeenCalled(); + }); + + it("swallows the first Escape while a clarification is pending and expanded, keeping the modal open", () => { + renderWithConversationTab(); + + fireEvent.click(screen.getByTestId(TOGGLE_ESCAPE_GUARD_TESTID)); + fireEvent.click(screen.getByTestId(FIRE_ESCAPE_TESTID)); + + expect(escapePreventDefault).toHaveBeenCalledTimes(1); + expect(screen.getByTestId("quick-chat-dialog")).toBeTruthy(); + }); + + it("lets the second Escape close the modal once the clarification has collapsed", () => { + renderWithConversationTab(); + + fireEvent.click(screen.getByTestId(TOGGLE_ESCAPE_GUARD_TESTID)); // guard on: pending + expanded + fireEvent.click(screen.getByTestId(FIRE_ESCAPE_TESTID)); // 1st Escape: collapses, swallowed + fireEvent.click(screen.getByTestId(TOGGLE_ESCAPE_GUARD_TESTID)); // simulates the resulting collapse + fireEvent.click(screen.getByTestId(FIRE_ESCAPE_TESTID)); // 2nd Escape: unguarded, would close + + expect(escapePreventDefault).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/web/components/quick-chat/quick-chat-modal.tsx b/apps/web/components/quick-chat/quick-chat-modal.tsx index edd9e7897c..7ac66d969c 100644 --- a/apps/web/components/quick-chat/quick-chat-modal.tsx +++ b/apps/web/components/quick-chat/quick-chat-modal.tsx @@ -1,6 +1,6 @@ "use client"; -import { memo, type CSSProperties } from "react"; +import { memo, useState, type CSSProperties } from "react"; import { useTranslation } from "react-i18next"; import { Dialog, DialogContent, DialogTitle } from "@kandev/ui/dialog"; import { Button } from "@kandev/ui/button"; @@ -15,6 +15,7 @@ import { QuickTabAddMenu } from "./quick-tab-add-menu"; import { QuickChatSetup } from "./quick-chat-setup"; import { useQuickChatModal } from "./use-quick-chat-modal"; import { useQuickChatWidth } from "@/hooks/use-quick-chat-width"; +import { ClarificationEscapeGuardProvider } from "@/hooks/use-clarification-escape-guard"; import { ConfigChatSetup } from "@/components/config-chat/config-chat-setup"; import { useConfigChat } from "@/components/config-chat/use-config-chat"; import type { QuickChatSession, QuickTerminalTab } from "@/lib/state/slices/ui/types"; @@ -240,14 +241,21 @@ export const QuickChatModal = memo(function QuickChatModal({ workspaceId }: Quic const quickChat = useQuickChatModal(workspaceId, configChat.reset); const setQuickChatInitialPrompt = useAppStore((state) => state.setQuickChatInitialPrompt); const { width, leftResizeHandleProps, rightResizeHandleProps } = useQuickChatWidth(); + const [clarificationEscapeGuarded, setClarificationEscapeGuarded] = useState(false); return ( - <> + { + // A pending, expanded clarification collapses first; the modal + // stays open. A second Escape (now unguarded) closes it, matching + // the main task chat panel's two-stage Escape after #2729. + if (clarificationEscapeGuarded) event.preventDefault(); + }} > {t("common:commandQuickChat")} @@ -266,6 +274,6 @@ export const QuickChatModal = memo(function QuickChatModal({ workspaceId }: Quic onOpenChange={(open) => !open && quickChat.setSessionToClose(null)} onConfirm={quickChat.handleConfirmClose} /> - + ); }); diff --git a/apps/web/components/task/chat/clarification-input-overlay.test.tsx b/apps/web/components/task/chat/clarification-input-overlay.test.tsx new file mode 100644 index 0000000000..569a44d8fa --- /dev/null +++ b/apps/web/components/task/chat/clarification-input-overlay.test.tsx @@ -0,0 +1,137 @@ +import { createRef } from "react"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { render, cleanup, fireEvent, screen } from "@testing-library/react"; +import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/lib/types/http"; +import { ClarificationInputOverlay } from "./clarification-input-overlay"; + +vi.mock("@/lib/config", () => ({ + getBackendConfig: () => ({ apiBaseUrl: "https://api.test" }), +})); + +const mockUpdateMessage = vi.fn(); +vi.mock("@/components/state-provider", () => ({ + useAppStoreApi: () => ({ + getState: () => ({ updateMessage: mockUpdateMessage }), + }), +})); + +vi.mock("@kandev/ui/tooltip", () => ({ + Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipProvider: ({ children }: { children: React.ReactNode }) => <>{children}, +})); + +const fetchMock = vi.fn(); + +function clarMessage(opts: { + id: string; + questionId: string; + index: number; + total: number; +}): Message { + return { + id: opts.id, + session_id: toSessionId("s1"), + task_id: toTaskId("t1"), + author_type: "agent", + content: "Q", + type: "clarification_request", + created_at: "2026-05-04T00:00:00Z", + metadata: { + pending_id: "p1", + question_id: opts.questionId, + question_index: opts.index, + question_total: opts.total, + status: "pending", + question: { + id: opts.questionId, + title: "", + prompt: `Question ${opts.index + 1}?`, + options: [{ option_id: "o1", label: "Option 1", description: "" }], + }, + }, + }; +} + +function renderOverlay( + messages: Message[], + overrides: Partial<{ onResolved: () => void; onDismiss: () => void }> = {}, +) { + const scopeRef = createRef(); + const onResolved = overrides.onResolved ?? vi.fn(); + const onDismiss = overrides.onDismiss ?? vi.fn(); + const utils = render( +
+ +
, + ); + return { ...utils, scopeRef, onResolved, onDismiss }; +} + +beforeEach(() => { + fetchMock.mockReset(); + mockUpdateMessage.mockReset(); + fetchMock.mockResolvedValue(new Response(null, { status: 200 })); + globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; +}); + +afterEach(() => { + cleanup(); +}); + +describe("ClarificationInputOverlay — Escape key", () => { + it("dismisses locally without posting a rejection, leaving the question pending", () => { + const messages = [clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })]; + const { onDismiss, scopeRef } = renderOverlay(messages); + + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it("does not call skipAll (no store update to rejected) on Escape", () => { + const messages = [clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })]; + const { scopeRef } = renderOverlay(messages); + + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + expect(mockUpdateMessage).not.toHaveBeenCalled(); + }); + + it("still calls onDismiss from any step of a multi-question carousel", () => { + const messages = [ + clarMessage({ id: "m1", questionId: "q1", index: 0, total: 2 }), + clarMessage({ id: "m2", questionId: "q2", index: 1, total: 2 }), + ]; + const { onDismiss, scopeRef } = renderOverlay(messages); + + fireEvent.click(screen.getAllByTestId("clarification-step")[1]); + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(onDismiss).toHaveBeenCalledTimes(1); + }); +}); + +describe("ClarificationInputOverlay — labelled Skip button", () => { + it("still POSTs a rejection when the Skip button is clicked", async () => { + const messages = [clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })]; + renderOverlay(messages); + + fireEvent.click(screen.getByTestId("clarification-skip")); + + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + const [, init] = fetchMock.mock.calls[0]; + expect(JSON.parse(String(init.body))).toEqual({ + rejected: true, + reject_reason: "User skipped", + }); + }); +}); diff --git a/apps/web/components/task/chat/clarification-input-overlay.tsx b/apps/web/components/task/chat/clarification-input-overlay.tsx index 90815a23cf..7b588bd38e 100644 --- a/apps/web/components/task/chat/clarification-input-overlay.tsx +++ b/apps/web/components/task/chat/clarification-input-overlay.tsx @@ -1,8 +1,7 @@ "use client"; import { useCallback, useEffect, useMemo, useRef, useState, type RefObject } from "react"; -import { IconX, IconMessageQuestion, IconInfoCircle, IconCheck } from "@tabler/icons-react"; -import { cn } from "@/lib/utils"; +import { IconMessageQuestion, IconInfoCircle } from "@tabler/icons-react"; import ReactMarkdown from "react-markdown"; import { markdownComponents, remarkPlugins } from "@/components/shared/markdown-components"; import type { @@ -12,14 +11,13 @@ import type { ClarificationQuestion, } from "@/lib/types/http"; import { useClarificationGroup } from "@/hooks/domains/session/use-clarification-group"; -import { KeyboardShortcutTooltip } from "@/components/keyboard-shortcut-tooltip"; -import { SHORTCUTS } from "@/lib/keyboard/constants"; import { ClarificationCarouselNav, ClarificationCustomInput, ClarificationOptions, ClarificationStepper, } from "./clarification-overlay-parts"; +import { ClarificationHeaderActions } from "./clarification-overlay-header"; import { useTranslation } from "react-i18next"; type ClarificationInputOverlayProps = { @@ -27,6 +25,13 @@ type ClarificationInputOverlayProps = { onResolved: () => void; shortcutScopeRef: RefObject; keyboardShortcutsEnabled?: boolean; + // Called when the user presses Escape. Unlike Skip, this must not answer or + // reject the bundle — it only dismisses the UI (e.g. collapses the panel). + // The question stays pending and the agent stays blocked. + onDismiss: () => void; + // Called by the expanded header's collapse control. + onCollapse?: () => void; + collapseContentId?: string; }; type SingleQuestionMeta = { @@ -187,7 +192,7 @@ type CarouselShortcutArgs = { onPick: (index: number) => void; onPrev: () => void; onNext: () => void; - onSkip: () => void; + onDismiss: () => void; onSubmit: () => void; }; @@ -224,7 +229,7 @@ function CarouselKeyboardShortcuts(args: CarouselShortcutArgs) { const { enabled, scopeRef } = args; const optionsCount = args.meta.question.options.length; const isLast = args.activeIndex === args.total - 1; - const { canSubmit, onPick, onPrev, onNext, onSkip, onSubmit } = args; + const { canSubmit, onPick, onPrev, onNext, onDismiss, onSubmit } = args; useEffect(() => { if (!enabled) return; const onKey = (e: KeyboardEvent) => { @@ -233,7 +238,7 @@ function CarouselKeyboardShortcuts(args: CarouselShortcutArgs) { if (shouldIgnoreShortcut(e)) return; if (e.key === "Escape") { e.preventDefault(); - onSkip(); + onDismiss(); return; } if (e.key === "ArrowLeft") { @@ -264,7 +269,7 @@ function CarouselKeyboardShortcuts(args: CarouselShortcutArgs) { onPick, onPrev, onNext, - onSkip, + onDismiss, onSubmit, ]); return null; @@ -282,6 +287,7 @@ type CarouselBodyProps = { shortcutScopeRef: RefObject; keyboardShortcutsEnabled: boolean; onSubmit: () => void; + onDismiss: () => void; }; type QuestionHandlerCtx = { @@ -388,6 +394,7 @@ function ClarificationCarouselBody({ shortcutScopeRef, keyboardShortcutsEnabled, onSubmit, + onDismiss, }: CarouselBodyProps) { const total = sortedMessages.length; const activeMessage = sortedMessages[Math.min(activeIndex, total - 1)] ?? null; @@ -451,85 +458,21 @@ function ClarificationCarouselBody({ onPick={(idx) => onSelectOption(meta.question.options[idx].option_id)} onPrev={() => setActiveIndex(Math.max(0, activeIndex - 1))} onNext={() => setActiveIndex(Math.min(total - 1, activeIndex + 1))} - onSkip={() => void group.skipAll("User skipped")} + onDismiss={onDismiss} onSubmit={onSubmit} /> ); } -function ClarificationHeaderActions({ - total, - allAnswered, - isSubmitting, - onSubmit, - onSkip, -}: { - total: number; - allAnswered: boolean; - isSubmitting: boolean; - onSubmit: () => void; - onSkip: () => void; -}) { - const { t } = useTranslation(); - return ( -
- {total > 1 && ( - - - - - - )} - - - - - -
- ); -} - export function ClarificationInputOverlay({ messages, onResolved, shortcutScopeRef, keyboardShortcutsEnabled = true, + onDismiss, + onCollapse, + collapseContentId, }: ClarificationInputOverlayProps) { const sortedMessages = useMemo( () => sortMessagesByQuestionIndex(resolveQuestionMessages(messages)), @@ -565,8 +508,11 @@ export function ClarificationInputOverlay({ return (
-
-
+
+
{total > 1 && ( 1 && ( {group.answeredCount} of {group.total} answered @@ -592,6 +538,8 @@ export function ClarificationInputOverlay({ isSubmitting={isSubmitting} onSubmit={handleSubmit} onSkip={() => void group.skipAll("User skipped")} + onCollapse={onCollapse} + collapseContentId={collapseContentId} />
{sharedContext && ( @@ -614,6 +562,7 @@ export function ClarificationInputOverlay({ shortcutScopeRef={shortcutScopeRef} keyboardShortcutsEnabled={keyboardShortcutsEnabled} onSubmit={handleSubmit} + onDismiss={onDismiss} />
); diff --git a/apps/web/components/task/chat/clarification-overlay-header.tsx b/apps/web/components/task/chat/clarification-overlay-header.tsx new file mode 100644 index 0000000000..99f4a25a90 --- /dev/null +++ b/apps/web/components/task/chat/clarification-overlay-header.tsx @@ -0,0 +1,119 @@ +"use client"; + +import { IconChevronDown, IconCheck, IconX } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; +import { KeyboardShortcutTooltip } from "@/components/keyboard-shortcut-tooltip"; +import { SHORTCUTS } from "@/lib/keyboard/constants"; +import { cn } from "@/lib/utils"; +import { useTranslation } from "react-i18next"; + +type ClarificationHeaderActionsProps = { + total: number; + allAnswered: boolean; + isSubmitting: boolean; + onSubmit: () => void; + onSkip: () => void; + onCollapse?: () => void; + collapseContentId?: string; +}; + +function ClarificationSkipButton({ + isSubmitting, + onSkip, + label, +}: { + isSubmitting: boolean; + onSkip: () => void; + label: string; +}) { + const button = ( + + + + ); + + if (isSubmitting) return button; + + return ( + + {button} + {label} + + ); +} + +export function ClarificationHeaderActions({ + total, + allAnswered, + isSubmitting, + onSubmit, + onSkip, + onCollapse, + collapseContentId, +}: ClarificationHeaderActionsProps) { + const { t } = useTranslation(); + return ( +
+ {total > 1 && ( + + + + + + )} + + {onCollapse && ( + + )} +
+ ); +} diff --git a/apps/web/components/task/chat/clarification-overlay-parts.tsx b/apps/web/components/task/chat/clarification-overlay-parts.tsx index dea72fb4cb..61d767e481 100644 --- a/apps/web/components/task/chat/clarification-overlay-parts.tsx +++ b/apps/web/components/task/chat/clarification-overlay-parts.tsx @@ -42,7 +42,7 @@ export function ClarificationStepper({ const { t } = useTranslation(); return (
diff --git a/apps/web/components/task/chat/clarification-panel-section.test.tsx b/apps/web/components/task/chat/clarification-panel-section.test.tsx new file mode 100644 index 0000000000..b5ac41c9e5 --- /dev/null +++ b/apps/web/components/task/chat/clarification-panel-section.test.tsx @@ -0,0 +1,276 @@ +import { createRef } from "react"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { render, cleanup, fireEvent, screen } from "@testing-library/react"; +import { i18n } from "@/lib/i18n"; +import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/lib/types/http"; +import { ClarificationPanelSection } from "./clarification-panel-section"; + +vi.mock("@/lib/config", () => ({ + getBackendConfig: () => ({ apiBaseUrl: "https://api.test" }), +})); + +const mockUpdateMessage = vi.fn(); +vi.mock("@/components/state-provider", () => ({ + useAppStoreApi: () => ({ + getState: () => ({ updateMessage: mockUpdateMessage }), + }), +})); + +vi.mock("@kandev/ui/tooltip", () => ({ + Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipProvider: ({ children }: { children: React.ReactNode }) => <>{children}, +})); + +const fetchMock = vi.fn(); + +function clarMessage(opts: { + pendingId: string; + id: string; + questionId: string; + index: number; + total: number; +}): Message { + return { + id: opts.id, + session_id: toSessionId("s1"), + task_id: toTaskId("t1"), + author_type: "agent", + content: "Q", + type: "clarification_request", + created_at: "2026-05-04T00:00:00Z", + metadata: { + pending_id: opts.pendingId, + question_id: opts.questionId, + question_index: opts.index, + question_total: opts.total, + status: "pending", + question: { + id: opts.questionId, + title: "", + prompt: `Question ${opts.index + 1}?`, + options: [{ option_id: "o1", label: "Option 1", description: "" }], + }, + }, + }; +} + +function renderSection(pending: boolean, messages: Message[], maxHeightVh = 50) { + const scopeRef = createRef(); + const onResolved = vi.fn(); + const utils = render( +
+ +
, + ); + return { ...utils, scopeRef, onResolved }; +} + +beforeEach(() => { + fetchMock.mockReset(); + mockUpdateMessage.mockReset(); + fetchMock.mockResolvedValue(new Response(null, { status: 200 })); + globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; +}); + +afterEach(async () => { + cleanup(); + // Restoring the locale is load-bearing: changeLanguage mutates the shared + // instance vitest.setup.ts initializes, so leaving it on pseudo would leak + // into every test that runs after this file. + await i18n.changeLanguage("en"); +}); + +const SCROLL_REGION_TESTID = "clarification-scroll-region"; +const CONTAINER_TESTID = "clarification-overlay-container"; +const QUESTION_COUNT_TESTID = "clarification-question-count"; + +describe("ClarificationPanelSection — collapse affordance", () => { + it("keeps the expanded question controls in one header row", () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 3 }), + clarMessage({ pendingId: "p1", id: "m2", questionId: "q2", index: 1, total: 3 }), + clarMessage({ pendingId: "p1", id: "m3", questionId: "q3", index: 2, total: 3 }), + ]; + renderSection(true, messages); + + const header = screen.getByTestId("clarification-overlay-header"); + expect(header.querySelector('[data-testid="clarification-stepper"]')).not.toBeNull(); + expect(header.querySelector('[data-testid="clarification-submit"]')).not.toBeNull(); + expect(header.querySelector('[data-testid="clarification-skip"]')).not.toBeNull(); + expect(header.querySelector('[data-testid="clarification-collapse-toggle"]')).not.toBeNull(); + expect(screen.queryByText("Clarification needed")).toBeNull(); + }); + + it("collapses on Escape without posting a rejection, and stays reachable to expand again", () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 1 }), + ]; + const { scopeRef } = renderSection(true, messages); + + expect(screen.getByTestId(SCROLL_REGION_TESTID).className).not.toContain("hidden"); + + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(screen.queryByTestId(CONTAINER_TESTID)).not.toBeNull(); + expect(screen.getByTestId(SCROLL_REGION_TESTID).className).toContain("hidden"); + + // Restore: clicking the toggle re-expands the same still-pending bundle. + fireEvent.click(screen.getByRole("button", { name: "Expand clarification" })); + expect(screen.getByTestId(SCROLL_REGION_TESTID).className).not.toContain("hidden"); + }); + + it("the labelled Skip button inside an expanded section still posts a rejection", async () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 1 }), + ]; + renderSection(true, messages); + + fireEvent.click(screen.getByTestId("clarification-skip")); + + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + }); + + it("resets to expanded when a new bundle (different pending_id) arrives while collapsed", () => { + const bundleA = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 1 }), + ]; + const bundleB = [ + clarMessage({ pendingId: "p2", id: "m2", questionId: "q2", index: 0, total: 1 }), + ]; + const { rerender, scopeRef } = renderSection(true, bundleA); + + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + expect(screen.getByTestId(SCROLL_REGION_TESTID).className).toContain("hidden"); + + rerender( +
+ +
, + ); + + expect(screen.getByTestId(SCROLL_REGION_TESTID).className).not.toContain("hidden"); + }); +}); + +describe("ClarificationPanelSection — per-surface max-height cap", () => { + it("renders the CSS max-height from the maxHeightVh prop instead of a hardcoded value", () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 1 }), + ]; + renderSection(true, messages, 50); + + const container = screen.getByTestId(CONTAINER_TESTID); + expect(container.style.maxHeight).toBe("50vh"); + }); + + it("a different surface's maxHeightVh renders a different cap (e.g. Quick Chat's 35vh)", () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 1 }), + ]; + renderSection(true, messages, 35); + + const container = screen.getByTestId(CONTAINER_TESTID); + expect(container.style.maxHeight).toBe("35vh"); + }); +}); + +describe("ClarificationPanelSection — question-count localization", () => { + it("uses the singular form for exactly one question in the bundle", () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 2 }), + ]; + const { scopeRef } = renderSection(true, messages); + + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + expect(screen.getByTestId(QUESTION_COUNT_TESTID).textContent).toBe("1 question"); + }); + + it("uses the plural form for more than one question in the bundle", () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 2 }), + clarMessage({ pendingId: "p1", id: "m2", questionId: "q2", index: 1, total: 2 }), + ]; + const { scopeRef } = renderSection(true, messages); + + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + expect(screen.getByTestId(QUESTION_COUNT_TESTID).textContent).toBe("2 questions"); + }); + + it("keeps the total count wording after a partial answer", () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 2 }), + clarMessage({ pendingId: "p1", id: "m2", questionId: "q2", index: 1, total: 2 }), + ]; + const { scopeRef } = renderSection(true, messages); + + fireEvent.click(screen.getAllByTestId("clarification-option")[0]); + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + expect(screen.getByTestId(QUESTION_COUNT_TESTID).textContent).toBe("2 questions"); + }); + + it("resolves the question-count text through the catalog on a locale switch, not an English fallback", async () => { + const messages = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 1 }), + ]; + const { scopeRef } = renderSection(true, messages); + + fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); + + await i18n.changeLanguage("pseudo"); + + expect(screen.getByTestId(QUESTION_COUNT_TESTID).textContent).toBe("1 qũēśţĩōń"); + }); +}); + +describe("ClarificationPanelSection — dragged height does not survive a bundle handover", () => { + it("resets the user-dragged height when a new bundle (different pending_id) replaces one still pending", () => { + const bundleA = [ + clarMessage({ pendingId: "p1", id: "m1", questionId: "q1", index: 0, total: 1 }), + ]; + const bundleB = [ + clarMessage({ pendingId: "p2", id: "m2", questionId: "q2", index: 0, total: 1 }), + ]; + const { rerender, scopeRef } = renderSection(true, bundleA); + const container = screen.getByTestId(CONTAINER_TESTID); + + const handle = screen.getByRole("button", { name: /resize/i }); + fireEvent.mouseDown(handle, { clientY: 500 }); + fireEvent.mouseMove(document, { clientY: 300 }); // drag up 200px → explicit pixel height + fireEvent.mouseUp(document); + + expect(container.style.height).toMatch(/^\d+(\.\d+)?px$/); + + rerender( +
+ +
, + ); + + // Bundle B must not inherit bundle A's dragged pixel height. + expect(container.style.height).toBe(""); + }); +}); diff --git a/apps/web/components/task/chat/clarification-panel-section.tsx b/apps/web/components/task/chat/clarification-panel-section.tsx new file mode 100644 index 0000000000..ef5be6ef83 --- /dev/null +++ b/apps/web/components/task/chat/clarification-panel-section.tsx @@ -0,0 +1,160 @@ +"use client"; + +import { useEffect, useId, useRef, useState, type RefObject } from "react"; +import { IconChevronDown, IconChevronUp, IconMessageQuestion } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { useTranslation } from "react-i18next"; +import { ClarificationInputOverlay } from "./clarification-input-overlay"; +import { ResizeHandle } from "./resize-handle"; +import { useResizableClarificationOverlay } from "@/hooks/use-resizable-clarification-overlay"; +import { useClarificationEscapeGuard } from "@/hooks/use-clarification-escape-guard"; +import type { ClarificationRequestMetadata, Message } from "@/lib/types/http"; + +type ClarificationPanelSectionProps = { + pending: boolean; + messages: readonly Message[] | null | undefined; + onResolved: () => void; + shortcutScopeRef: RefObject; + /** + * Caps the expanded overlay's height as a percentage of the viewport. + * Drives both the rendered CSS max-height and the resize hook's drag + * clamp — the two MUST agree, or the drag handle silently stops + * responding before its own visible ceiling. + */ + maxHeightVh: number; +}; + +function pendingIdFromMessages(messages: readonly Message[] | null | undefined): string | null { + const first = messages?.[0]; + if (!first) return null; + const metadata = first.metadata as ClarificationRequestMetadata | undefined; + return metadata?.pending_id ?? null; +} + +// Tracks collapse state per bundle: a bundle the user dismissed stays +// collapsed while it's still pending, but a *new* bundle (different +// pending_id) always opens expanded, even if the previous one never resolved. +function useCollapsedForBundle(pendingId: string | null) { + const [collapsed, setCollapsed] = useState(false); + const lastPendingId = useRef(pendingId); + useEffect(() => { + if (pendingId !== lastPendingId.current) { + lastPendingId.current = pendingId; + setCollapsed(false); + } + }, [pendingId]); + return [collapsed, setCollapsed] as const; +} + +/** + * Collapsible clarification section shared by the task chat panel and Quick + * Chat. Escape (or the labelled toggle) collapses this to a persistent header + * bar instead of answering or rejecting the bundle, so a blocked agent stays + * visibly blocked and the question stays reachable until answered or skipped. + */ +export function ClarificationPanelSection({ + pending, + messages, + onResolved, + shortcutScopeRef, + maxHeightVh, +}: ClarificationPanelSectionProps) { + const { t } = useTranslation(); + const pendingId = pendingIdFromMessages(messages); + const [collapsed, setCollapsed] = useCollapsedForBundle(pendingId); + const contentId = useId(); + const { height, containerRef, resetHeight, resizeHandleProps } = useResizableClarificationOverlay( + maxHeightVh / 100, + ); + + // A newly opened clarification starts expanded and auto-sized. Collapsing + // an active one leaves its form state and user-selected height intact. + useEffect(() => { + if (!pending) resetHeight(); + }, [pending, resetHeight]); + + // A new bundle (different pending_id) replacing a still-pending one must + // not inherit the previous bundle's dragged height. + useEffect(() => { + resetHeight(); + }, [pendingId, resetHeight]); + + // While this panel is expanded and pending, tell an ancestor dialog (Quick + // Chat) to swallow Escape instead of closing over it, so the first Escape + // collapses the panel like it does in the non-modal task chat panel. + useClarificationEscapeGuard(pending && !collapsed); + + if (!pending) return null; + + const questionCount = messages?.length ?? 0; + const actionLabel = collapsed ? t("chat:expandClarification") : t("chat:collapseClarification"); + const compact = collapsed || questionCount === 0; + + return ( +
+ {!compact && } +
+ {compact && ( +
+
+ + {t("chat:clarificationNeeded")} + {questionCount > 0 && ( + + {t("task:questionCount", { count: questionCount })} + + )} +
+ +
+ )} +
+ setCollapsed(true)} + onCollapse={() => setCollapsed(true)} + collapseContentId={contentId} + /> +
+
+
+ ); +} diff --git a/apps/web/components/task/task-chat-panel.tsx b/apps/web/components/task/task-chat-panel.tsx index 60e9877808..5df54fa6d8 100644 --- a/apps/web/components/task/task-chat-panel.tsx +++ b/apps/web/components/task/task-chat-panel.tsx @@ -22,10 +22,8 @@ import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; import { useIsTaskArchived } from "./task-archived-context"; import { useChatPanelState } from "./chat/use-chat-panel-state"; import { ChatInputArea, useSubmitHandler, useChatPanelHandlers } from "./chat/chat-input-area"; -import { ClarificationInputOverlay } from "./chat/clarification-input-overlay"; +import { ClarificationPanelSection } from "./chat/clarification-panel-section"; import { useComposerAgentStartHint } from "./chat/use-composer-agent-start-hint"; -import { ResizeHandle } from "./chat/resize-handle"; -import { useResizableClarificationOverlay } from "@/hooks/use-resizable-clarification-overlay"; import { PanelSearchBar } from "@/components/search/panel-search-bar"; import { SessionSearchHits } from "@/components/task/chat/session-search-hits"; import { usePanelSearch } from "@/hooks/use-panel-search"; @@ -35,7 +33,6 @@ import { findUnreadDividerItemId, lastRenderedMessageId } from "@/lib/session-un import { useSessionReadTracking } from "./chat/use-session-read-tracking"; import { useDrainOlderMessages } from "@/components/task/chat/use-drain-older-messages"; import { useAppStore } from "@/components/state-provider"; -import type { Message } from "@/lib/types/http"; import { getSessionWorkspacePath } from "@/lib/session-workspace-path"; import { routePanelMouseDown } from "./chat/route-panel-mouse-down"; import { useTranslation } from "react-i18next"; @@ -250,18 +247,6 @@ export const TaskChatPanel = memo(function TaskChatPanel({ ); const { handleCancelTurn } = useChatPanelHandlers(resolvedSessionId, chatInputRef); const { clarificationKey, handleClarificationResolved } = useClarificationKey(agentMessageCount); - const { - height: clarificationHeight, - containerRef: clarificationContainerRef, - resetHeight: clarificationResetHeight, - resizeHandleProps: clarificationResizeProps, - } = useResizableClarificationOverlay(); - - // Reset the dragged height when the overlay closes so a fresh - // clarification starts auto-sized instead of inheriting a stale value. - useEffect(() => { - if (!pendingClarification) clarificationResetHeight(); - }, [pendingClarification, clarificationResetHeight]); const panelRef = useRef(null); const messageListRef = useRef(null); @@ -402,16 +387,15 @@ export const TaskChatPanel = memo(function TaskChatPanel({ /> - + {!isArchived && ( + + )} ; - resizeProps: { onMouseDown: (e: React.MouseEvent) => void; onDoubleClick: () => void }; - height: number | null; - messages: readonly Message[] | null | undefined; - onResolved: () => void; - shortcutScopeRef: RefObject; -}; - -function ClarificationSection({ - pendingClarification, - isArchived, - containerRef, - resizeProps, - height, - messages, - onResolved, - shortcutScopeRef, -}: ClarificationSectionProps) { - if (!pendingClarification || isArchived) return null; - return ( -
- -
- -
-
- ); -} - type ChatFooterProps = { isArchived: boolean; chatInputRef: RefObject< diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 93bf351811..9b6d04aa86 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -474,6 +474,22 @@ export class SessionPage { return this.activeChat().getByTestId("clarification-overlay"); } + /** + * The persistent bar wrapping the clarification overlay. Stays mounted + * (collapsed to a header row) while the bundle is pending, even after the + * user dismisses it with Escape or the collapse toggle. + */ + clarificationBar(): Locator { + return this.activeChat().getByTestId("clarification-overlay-container"); + } + + /** Expand/collapse toggle in the clarification bar's header row. */ + clarificationCollapseToggle(): Locator { + // The expanded overlay stays mounted but is hidden when the compact bar + // is shown, so scope this locator to the one visible toggle. + return this.activeChat().locator('[data-testid="clarification-collapse-toggle"]:visible'); + } + /** Shared context shown once above the active clarification question. */ clarificationContext(): Locator { return this.clarificationOverlay().getByTestId("clarification-context"); diff --git a/apps/web/e2e/tests/chat/clarification-resize.spec.ts b/apps/web/e2e/tests/chat/clarification-resize.spec.ts index d46e1cc049..e3fb58bf6b 100644 --- a/apps/web/e2e/tests/chat/clarification-resize.spec.ts +++ b/apps/web/e2e/tests/chat/clarification-resize.spec.ts @@ -78,8 +78,8 @@ test.describe("Clarification overlay resizable layout", () => { seedData, }) => { // Tall viewport so the content-sized overlay has plenty of room to grow - // without bumping into the 50vh safety cap on the first drag. - await testPage.setViewportSize({ width: 1280, height: 1000 }); + // without bumping into the 35vh safety cap on the first drag. + await testPage.setViewportSize({ width: 1280, height: 3000 }); const session = await seedTaskAndWaitForIdle( testPage, @@ -94,7 +94,10 @@ test.describe("Clarification overlay resizable layout", () => { const container = testPage.getByTestId("clarification-overlay-container"); await expect(container).toBeVisible(); - const scrollRegion = container; + // The container now also carries the persistent header row (collapse + // toggle + waiting count), so it no longer scrolls itself — the nested + // scroll region below the header does. + const scrollRegion = container.getByTestId("clarification-scroll-region"); const initial = await container.evaluate((el) => { return { height: el.getBoundingClientRect().height, @@ -102,13 +105,13 @@ test.describe("Clarification overlay resizable layout", () => { inlineHeight: (el as HTMLElement).style.height, }; }); - await expect(scrollRegion).toHaveCSS("overflow-y", "scroll"); + await expect(scrollRegion).toHaveCSS("overflow-y", "auto"); // Default state: no inline height → container sizes to its content. expect(initial.inlineHeight).toBe(""); // Sanity check: content-sized overlay is at least tall enough for the - // question card but well under the 50vh cap. + // question card but well under the 35vh safety cap. expect(initial.height).toBeGreaterThan(200); - expect(initial.height).toBeLessThan(1000 * 0.5); + expect(initial.height).toBeLessThan(3000 * 0.35); const handle = container.locator("xpath=..").locator("button[aria-label='Resize']"); await expect(handle).toBeVisible(); @@ -142,4 +145,61 @@ test.describe("Clarification overlay resizable layout", () => { expect(afterReset.inlineHeight).toBe(""); expect(Math.abs(afterReset.height - initial.height)).toBeLessThanOrEqual(2); }); + + // Regression coverage for the task-chat-panel cap: the rendered CSS + // max-height and the resize hook's drag clamp must agree on 50vh here. + // Before the fix, ClarificationPanelSection rendered a flat 35vh cap + // (inherited from Quick Chat) while the hook still clamped drags to 50vh, + // so the handle silently stopped growing the box ~30% short of its own + // documented ceiling with zero feedback. + test("dragging well past the old 35vh mark keeps growing the task panel up to its 50vh cap, with no dead zone", async ({ + testPage, + apiClient, + seedData, + }) => { + // 2000px viewport → 35vh = 700px, 50vh = 1000px. Round numbers make the + // two caps unambiguous to distinguish in assertions. + await testPage.setViewportSize({ width: 1280, height: 2000 }); + + const session = await seedTaskAndWaitForIdle( + testPage, + apiClient, + seedData, + "Clarification Resize Cap", + ); + + await session.sendMessage("/ask-multiple"); + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + const container = session.clarificationBar(); + await expect(container).toBeVisible(); + await expect(container).toHaveCSS("max-height", "1000px"); + + const handle = container.locator("xpath=..").locator("button[aria-label='Resize']"); + await expect(handle).toBeVisible(); + + // Drag far enough (900px) that a working cap must clamp it — well past + // the old, wrong 700px (35vh) ceiling. + const handleBox = await handle.boundingBox(); + expect(handleBox).not.toBeNull(); + const startX = handleBox!.x + handleBox!.width / 2; + const startY = handleBox!.y + handleBox!.height / 2; + await testPage.mouse.move(startX, startY); + await testPage.mouse.down(); + await testPage.mouse.move(startX, startY - 900, { steps: 15 }); + await testPage.mouse.up(); + + const afterDrag = await container.evaluate((el) => ({ + height: el.getBoundingClientRect().height, + inlineHeight: (el as HTMLElement).style.height, + })); + + // The drag target overshoots 50vh, so the hook's own clamp pins the + // inline height at exactly 1000px... + expect(afterDrag.inlineHeight).toBe("1000px"); + // ...and because the CSS cap now agrees, the box actually renders there + // instead of stopping dead at the old 700px ceiling. + expect(afterDrag.height).toBeGreaterThan(700); + expect(afterDrag.height).toBeCloseTo(1000, 0); + }); }); diff --git a/apps/web/e2e/tests/chat/clarification.spec.ts b/apps/web/e2e/tests/chat/clarification.spec.ts index e03cad11ec..e260e13b69 100644 --- a/apps/web/e2e/tests/chat/clarification.spec.ts +++ b/apps/web/e2e/tests/chat/clarification.spec.ts @@ -5,6 +5,7 @@ import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { seedClarificationSession } from "../../helpers/clarification"; import { waitForSessionState } from "../../helpers/session"; +import { dwell } from "../../helpers/causal-waits"; import { SessionPage } from "../../pages/session-page"; import { KanbanPage } from "../../pages/kanban-page"; import { SidebarFilterPopoverPage } from "../../pages/sidebar-filter-popover"; @@ -306,6 +307,84 @@ test.describe("Clarification flow", () => { await expect(session.chat).toContainText("Use the embedded database for this task"); }); + test("Escape after the clarification detaches still doesn't reject, and the question stays answerable", async ({ + testPage, + apiClient, + seedData, + }) => { + const workflow = await apiClient.createWorkflow( + seedData.workspaceId, + "Clarification Timeout Esc Workflow", + ); + const timeoutStep = await apiClient.createWorkflowStep(workflow.id, "Timeout Step", 0); + + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Clarification Timeout Esc", + seedData.agentProfileId, + { + description: "/e2e:clarification-timeout", + workflow_id: workflow.id, + workflow_step_id: timeoutStep.id, + repository_ids: [seedData.repositoryId], + }, + ); + if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); + + await testPage.goto(`/t/${task.id}`); + + const session = new SessionPage(testPage); + await session.waitForLoad(); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + await expect(session.chat).toContainText("Question timed out", { timeout: 30_000 }); + await expect(session.clarificationDeferredNotice()).toBeVisible({ timeout: 10_000 }); + + await waitForSessionState(apiClient, { + taskId: task.id, + sessionId: task.session_id, + expectedState: "WAITING_FOR_INPUT", + message: "timed-out clarification session must remain ready for a deferred answer", + timeout: 30_000, + }); + + let respondCalls = 0; + await testPage.route("**/api/v1/clarification/*/respond", async (route) => { + respondCalls += 1; + await route.continue(); + }); + + // The MCP wait already detached (agent_disconnected cleanup ran), but + // Escape must still be a local-only dismiss — not a rejection — and the + // deferred question must stay answerable after restoring it. + await session.chat.focus(); + await testPage.keyboard.press("Escape"); + await expect(session.clarificationOverlay()).not.toBeVisible(); + await expect(session.clarificationBar()).toBeVisible(); + + await dwell( + testPage, + 300, + "negative-assertion", + "asserts Escape never posts a rejection after detach; there is no event for the request that must not fire", + ); + expect(respondCalls).toBe(0); + + await session.clarificationCollapseToggle().click(); + await expect(session.clarificationOverlay()).toBeVisible(); + + const input = session.clarificationInput(); + const inputRow = session.clarificationCustomInput(); + await expect(input).toBeEnabled(); + await inputRow.click({ position: { x: 4, y: 4 } }); + await expect(input).toBeFocused(); + await input.pressSequentially("Use the embedded database for this task"); + await input.press("Enter"); + + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await expect(session.chat).toContainText("Use the embedded database for this task"); + }); + test("options render label and description on separate rows", async ({ testPage, apiClient, @@ -765,9 +844,12 @@ test.describe("Multi-question clarification carousel", () => { await expect(submitTooltip).toContainText(/Ctrl|⌘/); await expect(submitTooltip).toContainText("Enter"); + // Skip has no keyboard shortcut: Escape only dismisses the bundle locally + // (see the "Escape dismisses" tests below), so its tooltip must not claim "Esc". await testPage.getByTestId("clarification-skip-shortcut").hover(); const skipTooltip = testPage.getByRole("tooltip", { name: /Skip all questions/ }); - await expect(skipTooltip).toContainText("Esc"); + await expect(skipTooltip).toBeVisible(); + await expect(skipTooltip).not.toContainText("Esc"); await session.clarificationOption("PostgreSQL").click(); await session.clarificationPrev().hover(); @@ -820,7 +902,7 @@ test.describe("Multi-question clarification carousel", () => { await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); }); - test("Esc skips the entire bundle from anywhere in the carousel", async ({ + test("Escape dismisses the bundle locally from anywhere in the carousel, without rejecting it", async ({ testPage, apiClient, seedData, @@ -829,17 +911,147 @@ test.describe("Multi-question clarification carousel", () => { testPage, apiClient, seedData, - "Multi-q esc", + "Multi-q esc dismiss", "clarification-multi", ); await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + let respondCalls = 0; + await testPage.route("**/api/v1/clarification/*/respond", async (route) => { + respondCalls += 1; + await route.continue(); + }); + await session.clarificationStep(1).click(); await testPage.keyboard.press("Escape"); + // The overlay collapses locally, but the bar stays put — the bundle is + // still pending and reachable, not answered or rejected. + await expect(session.clarificationOverlay()).not.toBeVisible(); + await expect(session.clarificationBar()).toBeVisible(); + await expect(session.clarificationCollapseToggle()).toBeVisible(); + + await dwell( + testPage, + 300, + "negative-assertion", + "asserts Escape never posts a rejection; there is no event for the request that must not fire", + ); + expect(respondCalls).toBe(0); + await expect(session.chat).not.toContainText("rejected"); + }); + + test("Escape leaves the agent blocked on the pending clarification", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Clarification esc keeps agent blocked", + seedData.agentProfileId, + { + description: "/e2e:clarification-multi", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); + + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + const { sessions: before } = await apiClient.listTaskSessions(task.id); + const stateBeforeEscape = before.find((s) => s.id === task.session_id)?.state; + if (!stateBeforeEscape) throw new Error("expected to find the seeded session's state"); + + await session.chat.focus(); + await testPage.keyboard.press("Escape"); + await expect(session.clarificationOverlay()).not.toBeVisible(); + + await dwell( + testPage, + 500, + "negative-assertion", + "asserts Escape never resumes the blocked turn; there is no event for the state transition that must not happen", + ); + + const { sessions: after } = await apiClient.listTaskSessions(task.id); + expect(after.find((s) => s.id === task.session_id)?.state).toBe(stateBeforeEscape); + }); + + test("dismissing with Escape and restoring lets the user answer normally", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q esc restore answer", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + await session.chat.focus(); + await testPage.keyboard.press("Escape"); + await expect(session.clarificationOverlay()).not.toBeVisible(); + await expect(session.clarificationBar()).toBeVisible(); + + await session.clarificationCollapseToggle().click(); + await expect(session.clarificationOverlay()).toBeVisible(); + + await session.clarificationOption("PostgreSQL").click(); + await session.clarificationOption("Go").click(); + await session.clarificationOption("Docker").click(); + await session.clarificationSubmit().click(); + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); - await expect(session.chat).toContainText("rejected"); + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + }); + + test("dismissing mid-bundle preserves recorded answers through the round trip", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q esc preserves answers", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + await session.clarificationOption("PostgreSQL").click(); + await expect(session.clarificationStep(0)).toHaveAttribute("data-answered", "true"); + await expect(session.clarificationGroupProgress()).toContainText("1 of 3 answered"); + + await session.chat.focus(); + await testPage.keyboard.press("Escape"); + await expect(session.clarificationOverlay()).not.toBeVisible(); + + await session.clarificationCollapseToggle().click(); + await expect(session.clarificationOverlay()).toBeVisible(); + + // Both the stepper's answered mark and the recorded answer itself must + // survive the dismiss/restore round trip. + await expect(session.clarificationStep(0)).toHaveAttribute("data-answered", "true"); + await expect(session.clarificationGroupProgress()).toContainText("1 of 3 answered"); + + await session.clarificationStep(0).click(); + const selectedOption = session + .clarificationQuestionCardById("db") + .locator('[data-testid="clarification-option"][data-selected="true"]'); + await expect(selectedOption).toContainText("PostgreSQL"); }); test("Back button is disabled on the first step", async ({ testPage, apiClient, seedData }) => { diff --git a/apps/web/e2e/tests/chat/quick-chat.spec.ts b/apps/web/e2e/tests/chat/quick-chat.spec.ts index c669d2e779..e8f51b8b1b 100644 --- a/apps/web/e2e/tests/chat/quick-chat.spec.ts +++ b/apps/web/e2e/tests/chat/quick-chat.spec.ts @@ -63,6 +63,38 @@ test.describe("Quick Chat", () => { ).toHaveAttribute("data-active", "true"); }); + test("Escape collapses the clarification panel before closing the modal", async ({ + testPage, + }) => { + const dialog = await openQuickChatWithAgent(testPage); + await sendQuickChatMessage(dialog, testPage, "/e2e:clarification-multi"); + + const clarification = dialog.getByTestId("clarification-overlay"); + await expect(clarification).toBeVisible({ timeout: 30_000 }); + + const bar = dialog.getByTestId("clarification-overlay-container"); + + // The collapse shortcut only fires for keydowns targeting the shortcut + // scope (quick-chat-content), same as the numeric-step shortcut above. + await dialog.getByTestId("quick-chat-messages").click({ position: { x: 8, y: 8 } }); + await expect(dialog.getByTestId("quick-chat-content")).toBeFocused(); + + await testPage.keyboard.press("Escape"); + + // First Escape: a pending, expanded clarification collapses in place and + // the modal stays open, matching the main task chat panel's two-stage + // Escape after #2729 — this is what distinguishes it from Radix's default + // (close-on-Escape) DismissableLayer behavior. + await expect(dialog).toBeVisible(); + await expect(clarification).not.toBeVisible(); + await expect(bar).toBeVisible(); + + await testPage.keyboard.press("Escape"); + + // Second Escape is now unguarded: it closes the whole modal. + await expect(dialog).not.toBeVisible(); + }); + test("offers configuration chat in setup and hides it once one exists", async ({ testPage, apiClient, diff --git a/apps/web/hooks/use-clarification-escape-guard.test.tsx b/apps/web/hooks/use-clarification-escape-guard.test.tsx new file mode 100644 index 0000000000..5959715a22 --- /dev/null +++ b/apps/web/hooks/use-clarification-escape-guard.test.tsx @@ -0,0 +1,35 @@ +import { describe, it, expect, vi } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { + ClarificationEscapeGuardProvider, + useClarificationEscapeGuard, +} from "./use-clarification-escape-guard"; + +describe("useClarificationEscapeGuard", () => { + it("does nothing outside a provider, so non-modal callers can invoke it unconditionally", () => { + expect(() => renderHook(() => useClarificationEscapeGuard(true))).not.toThrow(); + }); + + it("reports guarded state changes to the provider, and clears the guard on unmount", () => { + const setGuarded = vi.fn(); + const { rerender, unmount } = renderHook( + ({ guarded }) => useClarificationEscapeGuard(guarded), + { + initialProps: { guarded: false }, + wrapper: ({ children }) => ( + + {children} + + ), + }, + ); + + expect(setGuarded).toHaveBeenLastCalledWith(false); + + rerender({ guarded: true }); + expect(setGuarded).toHaveBeenLastCalledWith(true); + + unmount(); + expect(setGuarded).toHaveBeenLastCalledWith(false); + }); +}); diff --git a/apps/web/hooks/use-clarification-escape-guard.ts b/apps/web/hooks/use-clarification-escape-guard.ts new file mode 100644 index 0000000000..5c189f7562 --- /dev/null +++ b/apps/web/hooks/use-clarification-escape-guard.ts @@ -0,0 +1,25 @@ +"use client"; + +import { createContext, useContext, useEffect } from "react"; + +type ClarificationEscapeGuardSetter = (guarded: boolean) => void; + +const ClarificationEscapeGuardContext = createContext(null); + +export const ClarificationEscapeGuardProvider = ClarificationEscapeGuardContext.Provider; + +/** + * Tells an ancestor dialog (via ClarificationEscapeGuardProvider) to swallow + * its Escape-closes-dialog default while `guarded` is true, so the first + * Escape collapses this panel instead of dismissing the surrounding modal. + * No-ops outside a provider, so callers like the non-modal task chat panel + * can invoke this unconditionally. + */ +export function useClarificationEscapeGuard(guarded: boolean) { + const setGuarded = useContext(ClarificationEscapeGuardContext); + useEffect(() => { + if (!setGuarded) return; + setGuarded(guarded); + return () => setGuarded(false); + }, [guarded, setGuarded]); +} diff --git a/apps/web/hooks/use-resizable-clarification-overlay.test.ts b/apps/web/hooks/use-resizable-clarification-overlay.test.ts index 84df628b92..6ea02820a6 100644 --- a/apps/web/hooks/use-resizable-clarification-overlay.test.ts +++ b/apps/web/hooks/use-resizable-clarification-overlay.test.ts @@ -81,7 +81,7 @@ describe("useResizableClarificationOverlay", () => { expect(result.current.height).toBe(MIN_HEIGHT); }); - it("clamps above 50% of viewport when dragging up past it", () => { + it("clamps above 50% of viewport when dragging up past it (default ratio)", () => { setInnerHeight(800); // 50% = 400 cap const { result } = renderHook(() => useResizableClarificationOverlay()); attachContainerWithHeight(result.current.containerRef, 300); @@ -170,3 +170,27 @@ describe("useResizableClarificationOverlay", () => { expect(result.current.height).toBeNull(); }); }); + +describe("useResizableClarificationOverlay — maxViewportRatio parameter", () => { + beforeEach(() => { + setInnerHeight(1000); + document.body.style.cursor = ""; + document.body.style.userSelect = ""; + }); + + afterEach(() => { + document.body.style.cursor = ""; + document.body.style.userSelect = ""; + }); + + it("honors a caller-supplied maxViewportRatio instead of the 50% default", () => { + setInnerHeight(800); // 35% = 280 cap, well below the 400 the default would allow + const { result } = renderHook(() => useResizableClarificationOverlay(0.35)); + attachContainerWithHeight(result.current.containerRef, 300); + + act(() => result.current.resizeHandleProps.onMouseDown(fakeReactMouseEvent(500))); + act(() => dispatchMouseMove(0)); // delta=500 → 800 requested, must clamp to 280 (35%), not 400 (50%) + + expect(result.current.height).toBe(280); + }); +}); diff --git a/apps/web/hooks/use-resizable-clarification-overlay.ts b/apps/web/hooks/use-resizable-clarification-overlay.ts index bfb7948ab9..399d70b0a3 100644 --- a/apps/web/hooks/use-resizable-clarification-overlay.ts +++ b/apps/web/hooks/use-resizable-clarification-overlay.ts @@ -3,14 +3,14 @@ import { useCallback, useEffect, useRef, useState } from "react"; const MIN_HEIGHT = 120; -const MAX_VIEWPORT_RATIO = 0.5; +const DEFAULT_MAX_VIEWPORT_RATIO = 0.5; function viewportHeight(): number { return typeof window === "undefined" ? 800 : window.innerHeight; } -function clampHeight(value: number): number { - const max = Math.round(viewportHeight() * MAX_VIEWPORT_RATIO); +function clampHeight(value: number, maxViewportRatio: number): number { + const max = Math.round(viewportHeight() * maxViewportRatio); return Math.min(Math.max(value, MIN_HEIGHT), max); } @@ -20,14 +20,20 @@ function clampHeight(value: number): number { * Default behaviour: the container sizes to its content (`height === null` → * caller omits the inline height style). Once the user drags the resize * handle, the height switches to an explicit pixel value clamped between - * MIN_HEIGHT and 50% of the viewport. Double-clicking the handle returns to - * the auto-sized default. + * MIN_HEIGHT and `maxViewportRatio` (default 50%) of the viewport. + * Double-clicking the handle returns to the auto-sized default. + * + * `maxViewportRatio` MUST match whatever CSS `max-height` the caller renders + * on the same container — a mismatch creates a dead zone where the drag + * keeps computing a larger height than CSS will actually render. * * Callers MUST invoke `resetHeight()` when the overlay closes so a fresh * clarification starts with auto-sized height instead of inheriting a * dragged value. */ -export function useResizableClarificationOverlay() { +export function useResizableClarificationOverlay( + maxViewportRatio: number = DEFAULT_MAX_VIEWPORT_RATIO, +) { // null = auto-size to content; number = user-driven pixel height. const [height, setHeight] = useState(null); const containerRef = useRef(null); @@ -55,7 +61,7 @@ export function useResizableClarificationOverlay() { // Dragging the handle UP grows the overlay (it expands toward the top // of the screen), so a smaller clientY → larger height. const delta = startY.current - e.clientY; - setHeight(clampHeight(startHeight.current + delta)); + setHeight(clampHeight(startHeight.current + delta, maxViewportRatio)); }; const handleMouseUp = () => { if (!isDragging.current) return; @@ -74,7 +80,7 @@ export function useResizableClarificationOverlay() { document.body.style.userSelect = ""; } }; - }, []); + }, [maxViewportRatio]); return { height,