diff --git a/.qwen/e2e-tests/electron-desktop/inline-command-approval-cards.md b/.qwen/e2e-tests/electron-desktop/inline-command-approval-cards.md new file mode 100644 index 000000000..c20ca71ce --- /dev/null +++ b/.qwen/e2e-tests/electron-desktop/inline-command-approval-cards.md @@ -0,0 +1,51 @@ +# Inline Command Approval Cards + +- Slice date: 2026-04-26 +- Executable harness: `packages/desktop/scripts/e2e-cdp-smoke.mjs` +- Command: + `cd packages/desktop && npm run e2e:cdp` +- Result: pass +- Artifact directory: + `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-25T17-47-26-492Z/` + +## Scenario + +1. Launch the real Electron app with isolated HOME, runtime, user-data, and a + fake dirty Git workspace. +2. Open the fake project through the desktop directory picker path. +3. Send the first composer prompt without manually creating a thread. +4. Wait for the fake ACP command permission request. +5. Assert the request renders as an inline conversation card with the command + title, command preview, pending status, and approval/deny actions. +6. Assert the old detached permission strip is absent and the body does not + show a generic `Permission requested` event. +7. Approve once, assert the pending card resolves, then continue the existing + changed-files, review, settings, terminal, and final layout smoke path. + +## Assertions + +- The inline approval card is inside the chat timeline and stays above the + composer without overlap. +- The card exposes `Approve Once`, `Approve for Thread`, and `Deny` actions. +- The card includes `Run desktop E2E command`, `printf desktop-e2e`, and a + pending status. +- `.permission-strip` is absent. +- The conversation body does not contain `Permission requested`. +- The changed-files summary appears after approval and no approval card remains. +- Console errors: 0. +- Failed local network requests: 0. + +## Artifacts + +- `inline-command-approval.json` +- `inline-command-approval.png` +- `conversation-changes-summary.json` +- `completed-workspace.png` +- `electron.log` +- `summary.json` + +## Known Uncovered Risk + +The harness covers deterministic fake ACP command approval with a string command +input. It does not yet validate live ACP approvals with structured tool input, +ask-user free-form answer capture, or long command wrapping at compact widths. diff --git a/design/qwen-code-electron-desktop-implementation-plan.md b/design/qwen-code-electron-desktop-implementation-plan.md index 7b423fb2b..ec013200f 100644 --- a/design/qwen-code-electron-desktop-implementation-plan.md +++ b/design/qwen-code-electron-desktop-implementation-plan.md @@ -22,6 +22,103 @@ execution order, verification, decisions, and remaining work. ## Codex Alignment Progress +### Completed Slice: Inline Command Approval Cards + +Status: completed in iteration 9. + +Goal: make command approvals and ask-user prompts part of the conversation +timeline instead of a detached permission strip or protocol-like event row. + +User-visible value: users see what command/action needs attention in the same +reading flow as the agent plan, tool activity, and changed-files summary. The +main conversation can answer "what needs me now?" without exposing ACP request +plumbing. + +Expected files: + +- `packages/desktop/src/renderer/components/layout/ChatThread.tsx` +- `packages/desktop/src/renderer/components/layout/WorkspacePage.test.tsx` +- `packages/desktop/src/renderer/stores/chatStore.ts` +- `packages/desktop/src/renderer/stores/chatStore.test.ts` +- `packages/desktop/src/renderer/styles.css` +- `packages/desktop/scripts/e2e-cdp-smoke.mjs` +- `.qwen/e2e-tests/electron-desktop/inline-command-approval-cards.md` +- `design/qwen-code-electron-desktop-implementation-plan.md` + +Acceptance criteria: + +- Pending command permissions render as compact inline conversation cards with + command/tool title, optional command input, status, and approval/deny actions. +- Pending ask-user questions render inline with question text, options, and + Cancel/Submit actions. +- The old permission strip is no longer rendered as a separate surface between + the timeline and composer. +- Permission and ask-user server messages no longer append generic + `Permission requested` or `Question requested` event rows to the timeline. +- Approval controls keep stable accessible labels and continue to send the + same permission response. +- The composer remains docked and usable while a pending approval card is + visible; changed-files summary still appears after the request resolves. + +Verification: + +- Unit/component test command: + `cd packages/desktop && SHELL=/bin/bash npx vitest run src/renderer/stores/chatStore.test.ts src/renderer/components/layout/WorkspacePage.test.tsx` +- Build/typecheck/lint commands: + `cd packages/desktop && npm run typecheck && npm run lint && npm run build` +- Real Electron harness: + `cd packages/desktop && npm run e2e:cdp` +- Harness path: `packages/desktop/scripts/e2e-cdp-smoke.mjs` +- E2E scenario steps: launch real Electron with isolated HOME/runtime/user-data + and fake ACP, open the fake Git project, send from the composer, assert the + pending command approval appears as an inline conversation card with the fake + command title/input and no separate permission strip, approve it, assert the + card resolves away and the changed-files summary appears, then continue the + existing review, settings, and terminal smoke path. +- E2E assertions: inline approval card is present before approval, has compact + geometry within the chat timeline, exposes approval/deny actions, does not + render protocol request events, and console errors/failed local requests are + absent. +- Diagnostic artifacts: CDP screenshots, inline approval JSON, conversation + summary JSON, Electron log, summary JSON under + `.qwen/e2e-tests/electron-desktop/artifacts/`. +- Required skills applied: `frontend-design` for prototype-constrained inline + card density, action hierarchy, and conversation-first placement; + `electron-desktop-dev` for renderer changes and real Electron CDP + verification. + +Notes and decisions: + +- The prototype keeps approvals and task state in the reading flow, so this + slice removes the separate permission strip instead of duplicating the same + action in two places. +- The backing permission response contract remains unchanged; only renderer + placement and noise filtering change. +- The inline card intentionally shows only the tool title/kind/status and a + string or `command` preview from tool input; request IDs and session IDs stay + out of the main conversation. + +Verification results: + +- `cd packages/desktop && SHELL=/bin/bash npx vitest run src/renderer/stores/chatStore.test.ts src/renderer/components/layout/WorkspacePage.test.tsx` + passed with 11 tests. +- `cd packages/desktop && npm run typecheck` passed. +- `cd packages/desktop && npm run lint` passed. +- `cd packages/desktop && npm run build` passed. +- `cd packages/desktop && npm run e2e:cdp` passed after launch through real + Electron over CDP. +- Passing artifacts: + `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-25T17-47-26-492Z/`. + +Next work: + +- Continue rich conversation primitives by improving tool-call cards with + file-reference chips, copy/retry/open actions, and clearer completed/failed + command output summaries. +- Run another prototype fidelity pass on message density and assistant action + rows now that approvals, changed files, terminal, review, and settings have + all moved into supporting surfaces. + ### Completed Slice: Settings Information Architecture Status: completed in iteration 8. diff --git a/packages/desktop/scripts/e2e-cdp-smoke.mjs b/packages/desktop/scripts/e2e-cdp-smoke.mjs index c8aa92082..002e1995e 100644 --- a/packages/desktop/scripts/e2e-cdp-smoke.mjs +++ b/packages/desktop/scripts/e2e-cdp-smoke.mjs @@ -73,6 +73,8 @@ async function main() { await setFieldByAriaLabel('Message', 'Please exercise command approval.'); await clickButton('Send'); await waitForText('Approve Once'); + await assertInlineCommandApproval('inline-command-approval.json'); + await saveScreenshot('inline-command-approval.png'); await clickButton('Approve Once'); await waitForText('E2E fake ACP response received'); await assertConversationChangesSummary('conversation-changes-summary.json'); @@ -603,6 +605,8 @@ async function assertConversationChangesSummary(fileName) { return label === 'Review Changes'; }) ), + hasPendingApprovalCard: + document.querySelector('[data-testid="conversation-approval-card"]') !== null, reviewOpen: Boolean(document.querySelector('[data-testid="review-panel"]')) }; })()`); @@ -645,6 +649,10 @@ async function assertConversationChangesSummary(fileName) { throw new Error('Changed-files summary is missing its review action.'); } + if (snapshot.hasPendingApprovalCard) { + throw new Error('Approval card should resolve after approval.'); + } + if (snapshot.reviewOpen) { throw new Error('Changed-files summary should not open review by default.'); } @@ -662,6 +670,111 @@ async function assertConversationChangesSummary(fileName) { } } +async function assertInlineCommandApproval(fileName) { + await waitForSelector('[data-testid="conversation-approval-card"]'); + const snapshot = await evaluate(`(() => { + const card = document.querySelector( + '[data-testid="conversation-approval-card"]' + ); + const timeline = document.querySelector('.chat-timeline'); + const composer = document.querySelector('[data-testid="message-composer"]'); + const rectFor = (element) => { + if (!element) { + return null; + } + const rect = element.getBoundingClientRect(); + return { + top: rect.top, + right: rect.right, + bottom: rect.bottom, + left: rect.left, + width: rect.width, + height: rect.height + }; + }; + const buttons = [...(card?.querySelectorAll('button') ?? [])].map( + (button) => + button.getAttribute('aria-label') || + button.getAttribute('title') || + button.textContent.trim() + ); + return { + bodyText: document.body.innerText, + cardText: card?.innerText ?? '', + buttons, + cardRect: rectFor(card), + timelineRect: rectFor(timeline), + composerRect: rectFor(composer), + hasPermissionStrip: document.querySelector('.permission-strip') !== null, + hasRequestEvent: document.body.innerText.includes('Permission requested') + }; + })()`); + + await writeFile( + join(artifactDir, fileName), + `${JSON.stringify(snapshot, null, 2)}\n`, + 'utf8', + ); + + const cardText = snapshot.cardText.toLowerCase(); + for (const expectedText of [ + 'run desktop e2e command', + 'printf desktop-e2e', + 'pending', + ]) { + if (!cardText.includes(expectedText)) { + throw new Error( + `Inline approval card is missing ${expectedText}: ${snapshot.cardText}`, + ); + } + } + + for (const expectedAction of ['Approve Once', 'Approve for Thread', 'Deny']) { + if (!snapshot.buttons.includes(expectedAction)) { + throw new Error( + `Inline approval card missing action ${expectedAction}; buttons=${snapshot.buttons.join( + ', ', + )}`, + ); + } + } + + if (snapshot.hasPermissionStrip) { + throw new Error( + 'Permission approval should render inline, not in a strip.', + ); + } + + if (snapshot.hasRequestEvent) { + throw new Error('Permission request protocol event leaked into the body.'); + } + + if (!snapshot.cardRect || !snapshot.timelineRect || !snapshot.composerRect) { + throw new Error( + `Inline approval geometry is missing: ${JSON.stringify(snapshot)}`, + ); + } + + if (snapshot.cardRect.width < 360 || snapshot.cardRect.height > 180) { + throw new Error( + `Inline approval card geometry is unexpected: ${JSON.stringify( + snapshot.cardRect, + )}`, + ); + } + + if ( + snapshot.cardRect.left < snapshot.timelineRect.left || + snapshot.cardRect.right > snapshot.timelineRect.right + 1 + ) { + throw new Error('Inline approval card should stay inside the timeline.'); + } + + if (snapshot.cardRect.bottom > snapshot.composerRect.top) { + throw new Error('Inline approval card overlaps the composer.'); + } +} + async function assertReviewDrawerLayout(fileName) { const metrics = await evaluate(`(() => { const rectFor = (selector) => { diff --git a/packages/desktop/src/renderer/components/layout/ChatThread.tsx b/packages/desktop/src/renderer/components/layout/ChatThread.tsx index edd2defde..dab040b85 100644 --- a/packages/desktop/src/renderer/components/layout/ChatThread.tsx +++ b/packages/desktop/src/renderer/components/layout/ChatThread.tsx @@ -12,7 +12,11 @@ import type { } from '../../api/client.js'; import type { ChatState, ChatTimelineItem } from '../../stores/chatStore.js'; import type { ModelState } from '../../stores/modelStore.js'; -import type { DesktopApprovalMode } from '../../../shared/desktopProtocol.js'; +import type { + DesktopApprovalMode, + DesktopAskUserQuestionRequest, + DesktopPermissionRequest, +} from '../../../shared/desktopProtocol.js'; export function ChatThread({ activeProject, @@ -76,11 +80,8 @@ export function ChatThread({ activeSessionId={activeSessionId} gitDiff={gitDiff} isDraftSession={isDraftSession} - onOpenReview={onOpenReview} - /> -
void; onOpenReview: () => void; + onPermissionResponse: (requestId: string, optionId: string) => void; state: ChatState; }) { const timelineRef = useRef(null); const pendingPermissionId = state.pendingPermission?.requestId ?? ''; const pendingQuestionId = state.pendingAskUserQuestion?.requestId ?? ''; + const hasPendingPrompt = Boolean( + state.pendingPermission || state.pendingAskUserQuestion, + ); useEffect(() => { const timeline = timelineRef.current; @@ -216,7 +224,12 @@ function ChatTimeline({ return
Open a project to start
; } - if (!activeSessionId && !isDraftSession && state.items.length === 0) { + if ( + !activeSessionId && + !isDraftSession && + state.items.length === 0 && + !hasPendingPrompt + ) { return ( ( ))} + @@ -303,6 +322,166 @@ function TimelineItem({ item }: { item: ChatTimelineItem }) { return
{item.label}
; } +function InlinePendingPrompts({ + onAskUserQuestionResponse, + onPermissionResponse, + pendingAskUserQuestion, + pendingPermission, +}: { + onAskUserQuestionResponse: (requestId: string, optionId: string) => void; + onPermissionResponse: (requestId: string, optionId: string) => void; + pendingAskUserQuestion: ChatState['pendingAskUserQuestion']; + pendingPermission: ChatState['pendingPermission']; +}) { + if (!pendingPermission && !pendingAskUserQuestion) { + return null; + } + + return ( + <> + {pendingPermission ? ( + + ) : null} + {pendingAskUserQuestion ? ( + + ) : null} + + ); +} + +function CommandApprovalCard({ + onPermissionResponse, + permission, + requestId, +}: { + onPermissionResponse: (requestId: string, optionId: string) => void; + permission: DesktopPermissionRequest; + requestId: string; +}) { + const toolCall = permission.toolCall; + const title = toolCall.title || toolCall.kind || 'Command approval'; + const inputPreview = formatToolInput(toolCall.rawInput); + const status = toolCall.status || 'waiting for approval'; + + return ( +
+
+
+ {toolCall.kind || 'approval'} + {title} +
+ {status} +
+ {inputPreview ? ( + 
{inputPreview}
+ ) : null} +
+ {permission.options.map((option) => ( + + ))} +
+
+ ); +} + +function AskUserQuestionCard({ + onAskUserQuestionResponse, + questionRequest, + requestId, +}: { + onAskUserQuestionResponse: (requestId: string, optionId: string) => void; + questionRequest: DesktopAskUserQuestionRequest; + requestId: string; +}) { + return ( +
+
+
+ question + Input needed +
+ waiting +
+
+ {questionRequest.questions.map((item) => ( +
+ {item.header} + {item.question} + {item.options.length > 0 ? ( +
    + {item.options.map((option) => ( +
  • + {option.label} +
    • + ))} +
+ ) : null} +
+ ))} +
+
+ + +
+
+ ); +} + +function formatToolInput(input: unknown): string | null { + if (typeof input === 'string') { + return input.length > 0 ? input : null; + } + + if (input && typeof input === 'object' && 'command' in input) { + const command = (input as { command?: unknown }).command; + return typeof command === 'string' && command.length > 0 ? command : null; + } + + return null; +} + function ChangedFilesSummaryCard({ gitDiff, onOpenReview, @@ -436,94 +615,3 @@ const fallbackModeOptions = [ description: 'Ask before running commands.', }, ]; - -function PermissionPrompts({ - onAskUserQuestionResponse, - onPermissionResponse, - state, -}: { - onAskUserQuestionResponse: (requestId: string, optionId: string) => void; - onPermissionResponse: (requestId: string, optionId: string) => void; - state: ChatState; -}) { - const permission = state.pendingPermission; - const question = state.pendingAskUserQuestion; - if (!permission && !question) { - return null; - } - - return ( -
- {permission ? ( -
-
- - {permission.request.toolCall.kind || 'permission'} - - - {permission.request.toolCall.title || - permission.request.toolCall.toolCallId} - -
-
- {permission.request.options.map((option) => ( - - ))} -
-
- ) : null} - {question ? ( -
- {question.request.questions.map((item) => ( -
- {item.header} - {item.question} - {item.options.length > 0 ? ( -
    - {item.options.map((option) => ( -
  • - {option.label} -
    • - ))} -
- ) : null} -
- ))} -
- - -
-
- ) : null} -
- ); -} diff --git a/packages/desktop/src/renderer/components/layout/WorkspacePage.test.tsx b/packages/desktop/src/renderer/components/layout/WorkspacePage.test.tsx index 5a819d35d..8d292cb01 100644 --- a/packages/desktop/src/renderer/components/layout/WorkspacePage.test.tsx +++ b/packages/desktop/src/renderer/components/layout/WorkspacePage.test.tsx @@ -16,7 +16,7 @@ import type { DesktopSessionSummary, DesktopTerminal, } from '../../api/client.js'; -import { createInitialChatState } from '../../stores/chatStore.js'; +import { chatReducer, createInitialChatState } from '../../stores/chatStore.js'; import { createInitialModelState } from '../../stores/modelStore.js'; import { createInitialSettingsState } from '../../stores/settingsStore.js'; import { WorkspacePage } from './WorkspacePage.js'; @@ -300,6 +300,62 @@ describe('WorkspacePage', () => { } }); + it('renders command approvals inline in the conversation timeline', () => { + const onPermissionResponse = vi.fn(); + const chatState = chatReducer(createInitialChatState(), { + type: 'server_message', + message: { + type: 'permission_request', + requestId: 'permission-1', + request: { + sessionId: session.sessionId, + toolCall: { + toolCallId: 'tool-1', + kind: 'execute', + title: 'Run tests', + status: 'pending', + rawInput: 'npm test', + }, + options: [ + { + optionId: 'approve_once', + name: 'Approve Once', + kind: 'allow_once', + }, + { + optionId: 'deny', + name: 'Deny', + kind: 'reject_once', + }, + ], + }, + }, + }); + + const renderedContainer = renderWorkspace({ + chatState, + onPermissionResponse, + }); + const approvalCard = renderedContainer.querySelector( + '[data-testid="conversation-approval-card"]', + ); + + expect(approvalCard).toBeTruthy(); + expect(approvalCard?.textContent).toContain('Run tests'); + expect(approvalCard?.textContent).toContain('npm test'); + expect(renderedContainer.querySelector('.permission-strip')).toBeNull(); + expect(renderedContainer.textContent).not.toContain('Permission requested'); + + act(() => { + clickButton(renderedContainer, 'Approve Once'); + }); + + expect(onPermissionResponse).toHaveBeenCalledWith( + 'permission-1', + 'approve_once', + ); + }); + it('routes terminal output through an attach action', () => { const onAttachTerminalOutput = vi.fn(); const renderedContainer = renderWorkspace({ diff --git a/packages/desktop/src/renderer/stores/chatStore.test.ts b/packages/desktop/src/renderer/stores/chatStore.test.ts index 30fbb3049..7c650e934 100644 --- a/packages/desktop/src/renderer/stores/chatStore.test.ts +++ b/packages/desktop/src/renderer/stores/chatStore.test.ts @@ -85,4 +85,34 @@ describe('chatStore', () => { expect(JSON.stringify(complete.items)).not.toContain('session-e2e-1'); expect(JSON.stringify(complete.items)).not.toContain('end_turn'); }); + + it('tracks pending approvals without adding protocol request events', () => { + const state = chatReducer(createInitialChatState(), { + type: 'server_message', + message: { + type: 'permission_request', + requestId: 'permission-1', + request: { + sessionId: 'session-1', + toolCall: { + toolCallId: 'tool-1', + kind: 'execute', + title: 'Run tests', + rawInput: 'npm test', + }, + options: [ + { + optionId: 'approve_once', + name: 'Approve Once', + kind: 'allow_once', + }, + ], + }, + }, + }); + + expect(state.pendingPermission?.requestId).toBe('permission-1'); + expect(state.items).toHaveLength(0); + expect(JSON.stringify(state.items)).not.toContain('Permission requested'); + }); }); diff --git a/packages/desktop/src/renderer/stores/chatStore.ts b/packages/desktop/src/renderer/stores/chatStore.ts index 73e1e103a..ae6e3ce20 100644 --- a/packages/desktop/src/renderer/stores/chatStore.ts +++ b/packages/desktop/src/renderer/stores/chatStore.ts @@ -211,15 +211,6 @@ function applyServerMessage( requestId: message.requestId, request: message.request, }, - items: [ - ...state.items, - createEventItem( - `Permission requested: ${ - message.request.toolCall.title || - message.request.toolCall.toolCallId - }`, - ), - ], }; case 'ask_user_question': @@ -229,7 +220,6 @@ function applyServerMessage( requestId: message.requestId, request: message.request, }, - items: [...state.items, createEventItem('Question requested')], }; case 'message_complete': diff --git a/packages/desktop/src/renderer/styles.css b/packages/desktop/src/renderer/styles.css index 8781eb69a..9150226db 100644 --- a/packages/desktop/src/renderer/styles.css +++ b/packages/desktop/src/renderer/styles.css @@ -885,37 +885,77 @@ summary:focus-visible { flex: 0 0 auto; } -.permission-strip { - display: grid; - width: min(900px, calc(100% - 36px)); - justify-self: center; - gap: 10px; - padding: 0 0 12px; -} - -.permission-panel { +.conversation-approval-card { display: grid; + width: min(860px, 100%); + align-self: center; gap: 12px; - padding: 12px; + padding: 12px 14px; border: 1px solid rgba(240, 182, 110, 0.48); border-radius: var(--radius); - background: var(--warning-soft); + background: + linear-gradient(180deg, rgba(240, 182, 110, 0.1), transparent), + var(--warning-soft); + color: var(--text-soft); } -.permission-panel > div { +.conversation-approval-heading, +.conversation-approval-actions { + display: flex; + align-items: center; + gap: 10px; +} + +.conversation-approval-heading { + justify-content: space-between; +} + +.conversation-approval-heading > div, +.conversation-question-list, +.conversation-question-list > div { display: grid; + min-width: 0; gap: 6px; } -.permission-panel strong { +.conversation-approval-heading strong, +.conversation-question-list strong { + min-width: 0; + overflow: hidden; color: var(--text); font-size: 14px; + text-overflow: ellipsis; + white-space: nowrap; } -.permission-actions { - display: flex; +.conversation-approval-status { + flex: 0 0 auto; + color: var(--warning); + font-size: 11px; + font-weight: 820; + text-transform: uppercase; +} + +.conversation-approval-command { + max-height: 82px; + min-width: 0; + margin: 0; + overflow: auto; + padding: 8px 10px; + border-radius: 6px; + background: rgba(8, 10, 11, 0.36); + color: var(--text); + font-family: + ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, 'Liberation Mono', + monospace; + font-size: 12px; + line-height: 1.45; + white-space: pre-wrap; + word-break: break-word; +} + +.conversation-approval-actions { flex-wrap: wrap; - gap: 8px; justify-content: flex-end; }