diff --git a/.qwen/e2e-tests/electron-desktop/cdp-renderer-observability.md b/.qwen/e2e-tests/electron-desktop/cdp-renderer-observability.md index f3ece854a..527673d98 100644 --- a/.qwen/e2e-tests/electron-desktop/cdp-renderer-observability.md +++ b/.qwen/e2e-tests/electron-desktop/cdp-renderer-observability.md @@ -109,5 +109,24 @@ under ignored This harness covers renderer/CDP observability and the main P0 workbench paths, but it is a development E2E smoke using fake ACP. Final MVP verification still -needs the remaining review/terminal polish called out in the implementation -plan. +needs the remaining terminal polish and final packaging smoke called out in the +implementation plan. + +## Iteration 10 Review Path Extension + +The CDP harness now also exercises the hunk review surface after opening the +temporary Git workspace: + +- waits for a visible Accept Hunk control in the Review panel; +- clicks Accept Hunk and verifies the hunk state changes to Accepted; +- adds an inline review note for `README.md`; +- continues through session creation, permission approval, settings save, and + project-scoped terminal output. + +Execution result: + +- `npm run e2e:cdp --workspace=packages/desktop` passed. +- Passing run artifacts: + `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-25T03-08-06-087Z/`. +- The passing run reported no renderer console errors or failed network + requests. diff --git a/.qwen/e2e-tests/electron-desktop/diff-review-commit.md b/.qwen/e2e-tests/electron-desktop/diff-review-commit.md index e118d8329..46900a264 100644 --- a/.qwen/e2e-tests/electron-desktop/diff-review-commit.md +++ b/.qwen/e2e-tests/electron-desktop/diff-review-commit.md @@ -39,25 +39,36 @@ Slice 12 basic diff review and commit. ## Automated Coverage Added This Iteration -The full Electron E2E harness is still pending. This iteration added -server-level coverage in `packages/desktop/src/server/index.test.ts`: +Iteration 10 extended the server and Electron E2E coverage: - opens a registered project and reads `/git/diff`; - verifies modified and untracked files are returned with diff text; +- verifies changed files include typed hunk metadata; - stages all changes and verifies status counts; +- stages one hunk from a multi-hunk tracked file and verifies only that hunk is + accepted into the index; +- reverts a remaining unstaged hunk and verifies the file content is restored + while the accepted hunk remains staged; - commits staged changes and verifies a clean status; - reverts all changes and verifies the workspace returns to the initial file content. +- launches Electron through `npm run e2e:cdp --workspace=packages/desktop`, + opens a temporary Git workspace, clicks Accept Hunk, verifies the accepted + state, adds an inline review note, and continues through the existing + permission/settings/terminal smoke. ## Execution Results -- `npm run test --workspace=packages/desktop` passed: 8 files, 50 tests. +- `npm run test --workspace=packages/desktop` passed: 9 files, 54 tests. - `npm run typecheck --workspace=packages/desktop` passed. - `npm run lint --workspace=packages/desktop` passed. - `npm run build --workspace=packages/desktop` passed. +- `npm run e2e:cdp --workspace=packages/desktop` passed with artifacts under + `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-25T03-08-06-087Z/`. ## Remaining Risk Hunk-level accept/revert, inline comments, Open in Editor, and real Electron -renderer assertions are not complete yet. They remain required before the MVP -can be marked done. +renderer assertions now have initial coverage. Remaining review risk is around +complex Git states such as renames, binary files, conflicting stale hunks, and +persisting review comments beyond the local renderer session. diff --git a/design/qwen-code-electron-desktop-implementation-plan.md b/design/qwen-code-electron-desktop-implementation-plan.md index 98d20d44a..76314b99a 100644 --- a/design/qwen-code-electron-desktop-implementation-plan.md +++ b/design/qwen-code-electron-desktop-implementation-plan.md @@ -22,11 +22,12 @@ execution order, verification, decisions, and remaining work. ## Current Status -Slices 1-11 established the desktop package, Electron main/preload/renderer +Slices 1-15 established the desktop package, Electron main/preload/renderer startup, authenticated health/runtime/settings/session APIs, ACP process wrapper, WebSocket chat loop, permission bridge, settings/model/mode controls, packaging configuration, package smoke verification, project/Git status, -renderer asset/CDP startup support, and the componentized workspace shell. +renderer asset/CDP startup support, the componentized workspace shell, the +CDP-driven Electron E2E harness, and hunk-aware diff review controls. Important correction from iteration 7: the previous plan text called the MVP complete after packaging smoke, but the architecture P0 also requires project @@ -313,6 +314,63 @@ scope before a DONE marker can be created. - Recorded in `.qwen/e2e-tests/electron-desktop/cdp-renderer-observability.md`. +### Slice 15: Hunk-Level Diff Review Controls + +- Status: complete in iteration 10 +- Goal: complete the next review-depth increment with hunk metadata, hunk + accept/revert mutations, visible file/hunk review actions, inline review + comments, and Open in Editor wiring. +- Files: + - `packages/desktop/src/server/services/gitReviewService.ts` + - `packages/desktop/src/server/index.ts` + - `packages/desktop/src/server/index.test.ts` + - `packages/desktop/src/renderer/api/client.ts` + - `packages/desktop/src/renderer/App.tsx` + - `packages/desktop/src/renderer/components/layout/WorkspacePage.tsx` + - `packages/desktop/src/renderer/components/layout/ReviewPanel.tsx` + - `packages/desktop/src/renderer/components/layout/WorkspacePage.test.tsx` + - `packages/desktop/src/renderer/styles.css` + - `packages/desktop/scripts/e2e-cdp-smoke.mjs` + - `.qwen/e2e-tests/electron-desktop/diff-review-commit.md` + - `.qwen/e2e-tests/electron-desktop/cdp-renderer-observability.md` +- Acceptance criteria: + - `GET /api/projects/:id/git/diff` includes per-file hunks with stable ids, + source state, range metadata, and visible hunk lines. + - `POST /api/projects/:id/git/stage` accepts `{ scope: 'hunk', filePath, +hunkId }` and stages only that current hunk. + - `POST /api/projects/:id/git/revert` accepts the same hunk target and + reverts only that current hunk when the patch still applies. + - Review panel exposes Accept/Revert All, file-level Accept/Revert, hunk-level + Accept/Revert, Open file, and inline file comments. + - Electron CDP smoke drives the visible hunk accept and comment path without + console errors or failed network requests. +- Completed: + - Added server-side hunk parsing from current Git diffs and recomputed hunk + patches on mutation so the renderer does not submit patch contents. + - Added hunk-target parsing and token-protected hunk stage/revert routes + scoped to registered projects. + - Added renderer API target types and response guards for hunk metadata. + - Reworked the review panel to show file actions, hunk cards, accepted/pending + state, local inline comments, and Open in Editor via the existing preload + `openPath` whitelist. + - Extended the CDP smoke to accept a hunk and add a review comment after + opening the temporary Git workspace. +- Verification: + - `npm run test --workspace=packages/desktop` passed: 9 files, 54 tests. + - `npm run typecheck --workspace=packages/desktop` passed. + - `npm run lint --workspace=packages/desktop` passed. + - `npm run build --workspace=packages/desktop` passed. + - `npm run e2e:cdp --workspace=packages/desktop` passed. Success artifacts + were written under ignored + `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-25T03-08-06-087Z/`. + - `npm run typecheck` passed across workspaces. + - `npm run build` passed across workspaces. Existing VS Code companion lint + warnings were reported by its build script, with no errors. +- E2E coverage: + - Updated + `.qwen/e2e-tests/electron-desktop/diff-review-commit.md` and + `.qwen/e2e-tests/electron-desktop/cdp-renderer-observability.md`. + ## Decision Log - 2026-04-25: Use a main-process hosted `DesktopServer` for MVP, matching the @@ -350,6 +408,10 @@ scope before a DONE marker can be created. `QWEN_DESKTOP_E2E_FAKE_ACP=1` so the Electron E2E harness can cover session, prompt, and permission UI without credentials or network calls. Production startup still creates the real `AcpProcessClient`. +- 2026-04-25: Implement hunk-level review by exposing stable hunk ids and + server-derived patch application, not renderer-submitted patches. This keeps + hunk accept/revert scoped to the current registered project state and avoids + trusting client-provided diff text. ## Verification Log @@ -400,6 +462,11 @@ scope before a DONE marker can be created. - `npm run e2e:cdp --workspace=packages/desktop` passed and reported no renderer console errors or failed network requests. - `npm run typecheck` passed across workspaces. + - `npm run build` passed across workspaces. Existing VS Code companion lint + warnings were reported by its build script, with no errors. + - After pre-commit formatting, focused desktop test/typecheck/lint passed + again on the committed tree. + - `npm run typecheck` passed across workspaces. - `npm run build` passed across workspaces. Existing VS Code companion lint warnings were reported by its build script, with no errors. - Bundle/package smoke passed: @@ -408,6 +475,13 @@ scope before a DONE marker can be created. consistent with prior package runs. - After tightening the E2E fake ACP gate, package dir, package smoke, and packaged launch smoke passed again. +- 2026-04-25 Slice 15 hunk-level diff review: + - `npm run test --workspace=packages/desktop` passed: 9 files, 54 tests. + - `npm run typecheck --workspace=packages/desktop` passed. + - `npm run lint --workspace=packages/desktop` passed. + - `npm run build --workspace=packages/desktop` passed. + - `npm run e2e:cdp --workspace=packages/desktop` passed and reported no + renderer console errors or failed network requests. ## Self Review Notes @@ -445,9 +519,14 @@ scope before a DONE marker can be created. `QWEN_DESKTOP_E2E_USER_DATA_DIR`, and `QWEN_DESKTOP_TEST_SELECT_DIRECTORY`. Normal desktop startup still uses the native directory picker and real ACP process. +- Slice 15 hunk review recomputes hunk patches from server-side Git state for + each mutation. A stale hunk id fails closed with `git_hunk_not_found` or + `git_error` instead of applying renderer-provided patch contents. +- Review comments are currently local renderer notes for the active review + session. Persisting them into ACP/session artifacts or Git commit metadata is + deferred. ## Remaining Work -- Implement hunk-level diff review, terminal PTY/write/send-output-to-AI - refinements, final package smoke, and any remaining MVP polish before - creating the DONE marker. +- Implement terminal PTY/write/send-output-to-AI refinements, run final package + smoke, and complete any remaining MVP polish before creating the DONE marker. diff --git a/packages/desktop/scripts/e2e-cdp-smoke.mjs b/packages/desktop/scripts/e2e-cdp-smoke.mjs index 94045dba4..910ee2376 100644 --- a/packages/desktop/scripts/e2e-cdp-smoke.mjs +++ b/packages/desktop/scripts/e2e-cdp-smoke.mjs @@ -69,6 +69,15 @@ async function main() { await clickButton('Open Project'); await waitForText('desktop-e2e-workspace'); await waitForText('README.md'); + await waitForText('Accept Hunk'); + await clickButton('Accept Hunk'); + await waitForText('Accepted'); + await setFieldByAriaLabel( + 'Review comment for README.md', + 'Review note from E2E', + ); + await clickButton('Add Comment'); + await waitForText('Review note from E2E'); await waitForSelector('[data-testid="project-list"]'); await clickButton('New Thread'); diff --git a/packages/desktop/src/renderer/App.tsx b/packages/desktop/src/renderer/App.tsx index 63c794c13..16ef8b4ff 100644 --- a/packages/desktop/src/renderer/App.tsx +++ b/packages/desktop/src/renderer/App.tsx @@ -36,6 +36,7 @@ import { stageDesktopProjectChanges, updateDesktopUserSettings, type DesktopGitDiff, + type DesktopGitReviewTarget, type DesktopProject, type DesktopSessionSummary, type DesktopTerminal, @@ -393,37 +394,63 @@ export function App() { [activeProject], ); - const stageAllChanges = useCallback(async () => { - if (loadState.state !== 'ready' || !activeProject) { - return; - } + const stageReviewTarget = useCallback( + async (target: DesktopGitReviewTarget) => { + if (loadState.state !== 'ready' || !activeProject) { + return; + } - try { - const result = await stageDesktopProjectChanges( - loadState.status.serverInfo, - activeProject.id, - ); - applyReviewMutation(result.status, result.diff); - } catch (error) { - setReviewError(getErrorMessage(error)); - } - }, [activeProject, applyReviewMutation, loadState]); + try { + const result = await stageDesktopProjectChanges( + loadState.status.serverInfo, + activeProject.id, + target, + ); + applyReviewMutation(result.status, result.diff); + } catch (error) { + setReviewError(getErrorMessage(error)); + } + }, + [activeProject, applyReviewMutation, loadState], + ); - const revertAllChanges = useCallback(async () => { - if (loadState.state !== 'ready' || !activeProject) { - return; - } + const revertReviewTarget = useCallback( + async (target: DesktopGitReviewTarget) => { + if (loadState.state !== 'ready' || !activeProject) { + return; + } - try { - const result = await revertDesktopProjectChanges( - loadState.status.serverInfo, - activeProject.id, - ); - applyReviewMutation(result.status, result.diff); - } catch (error) { - setReviewError(getErrorMessage(error)); - } - }, [activeProject, applyReviewMutation, loadState]); + try { + const result = await revertDesktopProjectChanges( + loadState.status.serverInfo, + activeProject.id, + target, + ); + applyReviewMutation(result.status, result.diff); + } catch (error) { + setReviewError(getErrorMessage(error)); + } + }, + [activeProject, applyReviewMutation, loadState], + ); + + const openReviewFile = useCallback( + async (filePath: string) => { + if (!activeProject) { + return; + } + + try { + await window.qwenDesktop.openPath( + joinProjectFilePath(activeProject.path, filePath), + ); + setReviewError(null); + } catch (error) { + setReviewError(getErrorMessage(error)); + } + }, + [activeProject], + ); const commitChanges = useCallback(async () => { if ( @@ -666,14 +693,15 @@ export function App() { onModelChange={changeModel} onPermissionResponse={respondToPermission} onRefreshProjectGitStatus={refreshProjectGitStatus} - onRevertAllChanges={revertAllChanges} + onOpenReviewFile={openReviewFile} + onRevertReviewTarget={revertReviewTarget} onRunTerminalCommand={runTerminalCommand} onSaveSettings={saveSettings} onSelectProject={selectProject} onSelectSession={setActiveSessionId} onSendMessage={sendMessage} onSettingsDispatch={dispatchSettings} - onStageAllChanges={stageAllChanges} + onStageReviewTarget={stageReviewTarget} onStopGeneration={stopGeneration} onTerminalCommandChange={setTerminalCommand} /> @@ -708,3 +736,12 @@ function isApprovalMode(value: string): value is DesktopApprovalMode { function getErrorMessage(error: unknown): string { return error instanceof Error ? error.message : 'Desktop operation failed.'; } + +function joinProjectFilePath(projectPath: string, filePath: string): string { + const separator = projectPath.includes('\\') ? '\\' : '/'; + const base = + projectPath.endsWith('/') || projectPath.endsWith('\\') + ? projectPath.slice(0, -1) + : projectPath; + return `${base}${separator}${filePath}`; +} diff --git a/packages/desktop/src/renderer/api/client.ts b/packages/desktop/src/renderer/api/client.ts index 09f08e075..55f3455ca 100644 --- a/packages/desktop/src/renderer/api/client.ts +++ b/packages/desktop/src/renderer/api/client.ts @@ -59,6 +59,19 @@ export type DesktopGitChangeStatus = | 'untracked' | 'unknown'; +export type DesktopGitChangeSource = 'staged' | 'unstaged' | 'untracked'; + +export interface DesktopGitDiffHunk { + id: string; + source: DesktopGitChangeSource; + header: string; + oldStart: number; + oldLines: number; + newStart: number; + newLines: number; + lines: string[]; +} + export interface DesktopGitChangedFile { path: string; status: DesktopGitChangeStatus; @@ -66,6 +79,7 @@ export interface DesktopGitChangedFile { unstaged: boolean; untracked: boolean; diff: string; + hunks: DesktopGitDiffHunk[]; } export interface DesktopGitDiff { @@ -88,6 +102,11 @@ export interface DesktopGitCommitMutation extends DesktopGitReviewMutation { }; } +export type DesktopGitReviewTarget = + | { scope: 'all' } + | { scope: 'file'; filePath: string } + | { scope: 'hunk'; filePath: string; hunkId: string }; + export type DesktopTerminalStatus = 'running' | 'exited' | 'failed' | 'killed'; export interface DesktopTerminal { @@ -249,12 +268,13 @@ export async function getDesktopProjectGitDiff( export async function stageDesktopProjectChanges( serverInfo: DesktopServerInfo, projectId: string, + target: DesktopGitReviewTarget = { scope: 'all' }, ): Promise { return writeJson( serverInfo, `/api/projects/${encodeURIComponent(projectId)}/git/stage`, 'POST', - { scope: 'all' }, + target, isGitReviewMutation, ); } @@ -262,12 +282,13 @@ export async function stageDesktopProjectChanges( export async function revertDesktopProjectChanges( serverInfo: DesktopServerInfo, projectId: string, + target: DesktopGitReviewTarget = { scope: 'all' }, ): Promise { return writeJson( serverInfo, `/api/projects/${encodeURIComponent(projectId)}/git/revert`, 'POST', - { scope: 'all' }, + target, isGitReviewMutation, ); } @@ -807,7 +828,28 @@ function isGitChangedFile(value: unknown): value is DesktopGitChangedFile { typeof candidate.staged === 'boolean' && typeof candidate.unstaged === 'boolean' && typeof candidate.untracked === 'boolean' && - typeof candidate.diff === 'string' + typeof candidate.diff === 'string' && + Array.isArray(candidate.hunks) && + candidate.hunks.every(isGitDiffHunk) + ); +} + +function isGitDiffHunk(value: unknown): value is DesktopGitDiffHunk { + if (!value || typeof value !== 'object') { + return false; + } + + const candidate = value as Partial; + return ( + typeof candidate.id === 'string' && + isGitChangeSource(candidate.source) && + typeof candidate.header === 'string' && + typeof candidate.oldStart === 'number' && + typeof candidate.oldLines === 'number' && + typeof candidate.newStart === 'number' && + typeof candidate.newLines === 'number' && + Array.isArray(candidate.lines) && + candidate.lines.every((line) => typeof line === 'string') ); } @@ -823,6 +865,10 @@ function isGitChangeStatus(value: unknown): value is DesktopGitChangeStatus { ); } +function isGitChangeSource(value: unknown): value is DesktopGitChangeSource { + return value === 'staged' || value === 'unstaged' || value === 'untracked'; +} + function isDesktopTerminal(value: unknown): value is DesktopTerminal { if (!value || typeof value !== 'object') { return false; diff --git a/packages/desktop/src/renderer/components/layout/ReviewPanel.tsx b/packages/desktop/src/renderer/components/layout/ReviewPanel.tsx index 293e306b0..a6f66a2a3 100644 --- a/packages/desktop/src/renderer/components/layout/ReviewPanel.tsx +++ b/packages/desktop/src/renderer/components/layout/ReviewPanel.tsx @@ -4,8 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { Dispatch } from 'react'; -import type { DesktopGitDiff, DesktopProject } from '../../api/client.js'; +import { useState, type Dispatch } from 'react'; +import type { + DesktopGitChangedFile, + DesktopGitDiff, + DesktopGitDiffHunk, + DesktopGitReviewTarget, + DesktopProject, +} from '../../api/client.js'; import type { ChatState } from '../../stores/chatStore.js'; import type { ModelState } from '../../stores/modelStore.js'; import type { @@ -31,10 +37,11 @@ export function ReviewPanel({ onCommitMessageChange, onModeChange, onModelChange, - onRevertAll, + onOpenFile, + onRevertTarget, onSaveSettings, onSettingsDispatch, - onStageAll, + onStageTarget, }: { activeProject: DesktopProject | null; activeSessionId: string | null; @@ -51,10 +58,11 @@ export function ReviewPanel({ onCommitMessageChange: (message: string) => void; onModeChange: (mode: DesktopApprovalMode) => void; onModelChange: (modelId: string) => void; - onRevertAll: () => void; + onOpenFile: (filePath: string) => void; + onRevertTarget: (target: DesktopGitReviewTarget) => void; onSaveSettings: () => void; onSettingsDispatch: Dispatch; - onStageAll: () => void; + onStageTarget: (target: DesktopGitReviewTarget) => void; }) { return (
void; onCommitMessageChange: (message: string) => void; - onRevertAll: () => void; - onStageAll: () => void; + onOpenFile: (filePath: string) => void; + onRevertTarget: (target: DesktopGitReviewTarget) => void; + onStageTarget: (target: DesktopGitReviewTarget) => void; project: DesktopProject | null; reviewError: string | null; }) { + const [commentDrafts, setCommentDrafts] = useState>( + {}, + ); + const [reviewComments, setReviewComments] = useState< + Record + >({}); + if (!project) { return (
@@ -164,7 +182,7 @@ function ReviewSummary({ className="secondary-button" disabled={changedFiles.length === 0} type="button" - onClick={onRevertAll} + onClick={() => onRevertTarget({ scope: 'all' })} > Revert All @@ -172,23 +190,46 @@ function ReviewSummary({ className="secondary-button" disabled={changedFiles.length === 0} type="button" - onClick={onStageAll} + onClick={() => onStageTarget({ scope: 'all' })} > - Stage All + Accept All
{changedFiles.length === 0 ? (
No changes
) : ( - changedFiles.map((file) => ( -
- - {file.path} - {file.status} - - 
{file.diff || 'No textual diff available.'}
-
+ changedFiles.map((file, index) => ( + { + const comment = (commentDrafts[file.path] ?? '').trim(); + if (!comment) { + return; + } + setReviewComments((current) => ({ + ...current, + [file.path]: [...(current[file.path] ?? []), comment], + })); + setCommentDrafts((current) => ({ + ...current, + [file.path]: '', + })); + }} + onCommentDraftChange={(comment) => + setCommentDrafts((current) => ({ + ...current, + [file.path]: comment, + })) + } + onOpenFile={onOpenFile} + onRevertTarget={onRevertTarget} + onStageTarget={onStageTarget} + /> )) )}
@@ -213,6 +254,163 @@ function ReviewSummary({ ); } +function ChangedFileReview({ + commentDraft, + comments, + file, + isInitiallyOpen, + onAddComment, + onCommentDraftChange, + onOpenFile, + onRevertTarget, + onStageTarget, +}: { + commentDraft: string; + comments: string[]; + file: DesktopGitChangedFile; + isInitiallyOpen: boolean; + onAddComment: () => void; + onCommentDraftChange: (comment: string) => void; + onOpenFile: (filePath: string) => void; + onRevertTarget: (target: DesktopGitReviewTarget) => void; + onStageTarget: (target: DesktopGitReviewTarget) => void; +}) { + const fileTarget = { scope: 'file' as const, filePath: file.path }; + + return ( +
+ + {file.path} + + {file.status} ยท {file.hunks.length} hunk + {file.hunks.length === 1 ? '' : 's'} + + +
+ + + +
+ {file.hunks.length === 0 ? ( + 
{file.diff || 'No textual diff available.'}
+ ) : ( +
+ {file.hunks.map((hunk) => ( + + ))} +
+ )} +
+