From 7fbf240c7074204640ec443a20db17a54ba73ac3 Mon Sep 17 00:00:00 2001 From: iliya Date: Wed, 4 Mar 2026 01:16:27 +0200 Subject: [PATCH] feat: enhance ChangeReviewDialog and changeReviewSlice for improved file handling - Added functionality to re-add rejected new files to the review list, allowing for undo operations. - Introduced a new `addReviewFile` method in changeReviewSlice to facilitate restoring files with their content. - Updated ChangeReviewDialog to manage undo actions for recently rejected files, improving user experience during file reviews. - Refactored file handling logic to ensure accurate state management and enhance the overall review process. --- .../team/review/ChangeReviewDialog.tsx | 78 +++++++- .../components/team/review/ReviewToolbar.tsx | 5 +- .../store/slices/changeReviewSlice.ts | 48 +++++ .../TeamProvisioningServicePrompts.test.ts | 166 ++++++++++++++++++ 4 files changed, 288 insertions(+), 9 deletions(-) create mode 100644 test/main/services/team/TeamProvisioningServicePrompts.test.ts diff --git a/src/renderer/components/team/review/ChangeReviewDialog.tsx b/src/renderer/components/team/review/ChangeReviewDialog.tsx index 6786d5b4..59a7d2c0 100644 --- a/src/renderer/components/team/review/ChangeReviewDialog.tsx +++ b/src/renderer/components/team/review/ChangeReviewDialog.tsx @@ -24,7 +24,12 @@ import { ScopeWarningBanner } from './ScopeWarningBanner'; import { ViewedProgressBar } from './ViewedProgressBar'; import type { EditorView } from '@codemirror/view'; -import type { HunkDecision, TaskChangeSetV2 } from '@shared/types'; +import type { + FileChangeSummary, + FileChangeWithContent, + HunkDecision, + TaskChangeSetV2, +} from '@shared/types'; import type { EditorSelectionAction, EditorSelectionInfo } from '@shared/types/editor'; interface ChangeReviewDialogProps { @@ -77,6 +82,7 @@ export const ChangeReviewDialog = ({ applyReview, applySingleFileDecision, removeReviewFile, + addReviewFile, editedContents, updateEditedContent, discardFileEdits, @@ -116,6 +122,10 @@ export const ChangeReviewDialog = ({ const lastHunkActionAtRef = useRef>({}); const hunkDecisionUndoStackRef = useRef>({}); const newFileApplyInFlightRef = useRef(new Set()); + const removedNewFileUndoStackRef = useRef< + { file: FileChangeSummary; index: number; restoreContent: string; removedAt: number }[] + >([]); + const lastNewFileRemoveAtRef = useRef(0); // Proxy ref for useDiffNavigation (points to active file's editor) const activeEditorViewRef = useRef(null); @@ -242,13 +252,32 @@ export const ChangeReviewDialog = ({ } // Always apply immediately: rejecting a NEW file means deleting it from disk. - const isNew = - activeChangeSet?.files.find((f) => f.filePath === filePath)?.isNewFile ?? false; + const file = activeChangeSet?.files.find((f) => f.filePath === filePath); + const isNew = file?.isNewFile ?? false; if (!isNew) return; const result = await applySingleFileDecision(teamName, filePath, taskId, memberName); const hasErrorForFile = !!result?.errors.some((e) => e.filePath === filePath); - if (result && !hasErrorForFile) { + if (result && !hasErrorForFile && file) { + // Keep undo payload so Ctrl/Cmd+Z can restore the file (and re-add it to the review list). + const cachedModified = fileContents[filePath]?.modifiedFullContent; + const restoreContent = + cachedModified ?? + (() => { + const writeSnippets = file.snippets.filter( + (s) => !s.isError && (s.type === 'write-new' || s.type === 'write-update') + ); + if (writeSnippets.length === 0) return ''; + return writeSnippets[writeSnippets.length - 1].newString; + })(); + const index = activeChangeSet?.files.findIndex((f) => f.filePath === filePath) ?? 0; + removedNewFileUndoStackRef.current.push({ + file, + index: Math.max(0, index), + restoreContent, + removedAt: Date.now(), + }); + lastNewFileRemoveAtRef.current = Date.now(); removeReviewFile(filePath); } } finally { @@ -263,6 +292,7 @@ export const ChangeReviewDialog = ({ taskId, memberName, removeReviewFile, + fileContents, ] ); @@ -575,6 +605,35 @@ export const ChangeReviewDialog = ({ // Prefer bulk undo (Accept All / Reject All) shortly after bulk action, // even if focus is inside a CM editor (focus often remains there after clicking buttons). const now = Date.now(); + + // Undo: recently rejected NEW file (deleted from disk + removed from review list) + const removedRecently = now - lastNewFileRemoveAtRef.current < 30_000; + const removedStack = removedNewFileUndoStackRef.current; + if ( + removedRecently && + removedStack.length > 0 && + !document.activeElement?.closest('.cm-editor') + ) { + e.preventDefault(); + e.stopPropagation(); + const snap = removedStack.pop()!; + const restoredContent: FileChangeWithContent = { + ...snap.file, + originalFullContent: '', + modifiedFullContent: snap.restoreContent, + contentSource: 'disk-current', + }; + addReviewFile(snap.file, { index: snap.index, content: restoredContent }); + setActiveFilePath(snap.file.filePath); + requestAnimationFrame(() => { + requestAnimationFrame(() => scrollToFile(snap.file.filePath)); + }); + updateEditedContent(snap.file.filePath, snap.restoreContent); + // Ensure editedContents is set before saveEditedFile reads it. + void Promise.resolve().then(() => saveEditedFile(snap.file.filePath, projectPath)); + return; + } + const bulkRecently = now - lastBulkActionAtRef.current < 10_000; if (bulkRecently && useStore.getState().reviewUndoStack.length > 0) { e.preventDefault(); @@ -616,7 +675,16 @@ export const ChangeReviewDialog = ({ }; document.addEventListener('keydown', handler, true); return () => document.removeEventListener('keydown', handler, true); - }, [open, handleUndoBulk, clearHunkDecisionByOriginalIndex]); + }, [ + open, + handleUndoBulk, + clearHunkDecisionByOriginalIndex, + addReviewFile, + updateEditedContent, + saveEditedFile, + projectPath, + scrollToFile, + ]); // Cmd+N IPC listener (forwarded from main process) useEffect(() => { diff --git a/src/renderer/components/team/review/ReviewToolbar.tsx b/src/renderer/components/team/review/ReviewToolbar.tsx index 0196074f..1b93dae2 100644 --- a/src/renderer/components/team/review/ReviewToolbar.tsx +++ b/src/renderer/components/team/review/ReviewToolbar.tsx @@ -153,10 +153,7 @@ export const ReviewToolbar = ({ Undo - - Undo last bulk operation ( - {/Mac|iPhone|iPad/.test(navigator.userAgent) ? '⌘Z' : 'Ctrl+Z'}) - + Undo last bulk operation (Ctrl+Z) )} diff --git a/src/renderer/store/slices/changeReviewSlice.ts b/src/renderer/store/slices/changeReviewSlice.ts index 45191888..2c7fbbf3 100644 --- a/src/renderer/store/slices/changeReviewSlice.ts +++ b/src/renderer/store/slices/changeReviewSlice.ts @@ -19,6 +19,7 @@ import type { ApplyReviewRequest, ApplyReviewResult, ChangeStats, + FileChangeSummary, FileChangeWithContent, FileReviewDecision, HunkDecision, @@ -125,6 +126,11 @@ export interface ChangeReviewSlice { ) => Promise; /** Remove a file from the current review set (used for rejecting new files) */ removeReviewFile: (filePath: string) => void; + /** Re-add a file to the current review set (used for undoing new-file reject) */ + addReviewFile: ( + file: FileChangeSummary, + options?: { index?: number; content?: FileChangeWithContent } + ) => void; invalidateChangeStats: (teamName: string) => void; // Editable diff actions @@ -841,6 +847,48 @@ export const createChangeReviewSlice: StateCreator { + set((s) => { + if (!s.activeChangeSet) return s; + if (s.activeChangeSet.files.some((f) => f.filePath === file.filePath)) return s; + + const idxRaw = options?.index; + const idx = + typeof idxRaw === 'number' && Number.isFinite(idxRaw) + ? Math.max(0, Math.min(idxRaw, s.activeChangeSet.files.length)) + : s.activeChangeSet.files.length; + + const nextFiles = [...s.activeChangeSet.files]; + nextFiles.splice(idx, 0, file); + const totalLinesAdded = nextFiles.reduce((sum, f) => sum + f.linesAdded, 0); + const totalLinesRemoved = nextFiles.reduce((sum, f) => sum + f.linesRemoved, 0); + + const nextFileContents = options?.content + ? { ...s.fileContents, [file.filePath]: options.content } + : s.fileContents; + + const nextFileContentsLoading = options?.content + ? { ...s.fileContentsLoading, [file.filePath]: false } + : s.fileContentsLoading; + + return { + activeChangeSet: { + ...s.activeChangeSet, + files: nextFiles, + totalFiles: nextFiles.length, + totalLinesAdded, + totalLinesRemoved, + }, + selectedReviewFilePath: s.selectedReviewFilePath ?? file.filePath, + fileContents: nextFileContents, + fileContentsLoading: nextFileContentsLoading, + }; + }); + }, + // ── Editable diff actions ── updateEditedContent: (filePath: string, content: string) => { diff --git a/test/main/services/team/TeamProvisioningServicePrompts.test.ts b/test/main/services/team/TeamProvisioningServicePrompts.test.ts new file mode 100644 index 00000000..2f9830fe --- /dev/null +++ b/test/main/services/team/TeamProvisioningServicePrompts.test.ts @@ -0,0 +1,166 @@ +import { EventEmitter } from 'events'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +let tempClaudeRoot = ''; +let tempTeamsBase = ''; +let tempTasksBase = ''; + +vi.mock('@main/services/team/ClaudeBinaryResolver', () => ({ + ClaudeBinaryResolver: { resolve: vi.fn() }, +})); + +vi.mock('@main/utils/childProcess', () => ({ + spawnCli: vi.fn(), + killProcessTree: vi.fn(), +})); + +vi.mock('@main/utils/pathDecoder', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getAutoDetectedClaudeBasePath: () => tempClaudeRoot, + getClaudeBasePath: () => tempClaudeRoot, + getTeamsBasePath: () => tempTeamsBase, + getTasksBasePath: () => tempTasksBase, + }; +}); + +import { TeamProvisioningService } from '@main/services/team/TeamProvisioningService'; +import { ClaudeBinaryResolver } from '@main/services/team/ClaudeBinaryResolver'; +import { spawnCli } from '@main/utils/childProcess'; + +function createFakeChild() { + const writeSpy = vi.fn(); + const endSpy = vi.fn(); + const child = Object.assign(new EventEmitter(), { + pid: 12345, + stdin: { writable: true, write: writeSpy, end: endSpy }, + stdout: new EventEmitter(), + stderr: new EventEmitter(), + kill: vi.fn(), + }); + return { child, writeSpy }; +} + +function extractPromptFromWrite(writeSpy: ReturnType): string { + const payload = String(writeSpy.mock.calls[0]?.[0] ?? ''); + const parsed = JSON.parse(payload) as { + type: string; + message?: { role: string; content: { type: string; text?: string }[] }; + }; + const text = parsed.message?.content?.[0]?.text; + if (typeof text !== 'string') { + throw new Error('Failed to extract prompt text from stdin write payload'); + } + return text; +} + +describe('TeamProvisioningService prompt content (solo mode discipline)', () => { + beforeEach(() => { + vi.clearAllMocks(); + tempClaudeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'claude-team-prompts-')); + tempTeamsBase = path.join(tempClaudeRoot, 'teams'); + tempTasksBase = path.join(tempClaudeRoot, 'tasks'); + fs.mkdirSync(tempTeamsBase, { recursive: true }); + fs.mkdirSync(tempTasksBase, { recursive: true }); + }); + + afterEach(() => { + // Best-effort cleanup of temp dir (per-test) + try { + fs.rmSync(tempClaudeRoot, { recursive: true, force: true }); + } catch { + // ignore + } + }); + + it('createTeam prompt (solo) mandates sequential status + frequent user updates', async () => { + vi.mocked(ClaudeBinaryResolver.resolve).mockResolvedValue('/fake/claude'); + const { child, writeSpy } = createFakeChild(); + vi.mocked(spawnCli).mockReturnValue(child as any); + + const svc = new TeamProvisioningService(); + (svc as any).buildProvisioningEnv = vi.fn(async () => ({ + env: { ANTHROPIC_API_KEY: 'test' }, + authSource: 'anthropic_api_key', + })); + (svc as any).startFilesystemMonitor = vi.fn(); + (svc as any).pathExists = vi.fn(async () => false); + + const { runId } = await svc.createTeam( + { + teamName: 'solo-team', + cwd: process.cwd(), + members: [], + description: 'Solo team for prompt test', + }, + () => {} + ); + + expect(writeSpy).toHaveBeenCalledTimes(1); + const prompt = extractPromptFromWrite(writeSpy); + expect(prompt).toContain('SOLO MODE: This team CURRENTLY has ZERO teammates.'); + expect(prompt).toContain('PROGRESS REPORTING (MANDATORY)'); + expect(prompt).toContain('Never bulk-move many tasks at the end'); + expect(prompt).toContain('Default to working ONE task at a time'); + + await svc.cancelProvisioning(runId); + }); + + it('launchTeam prompt (solo) requires sequential execution and incremental updates', async () => { + // Seed config.json so launchTeam can validate team existence. + const teamName = 'solo-team-launch'; + const teamDir = path.join(tempTeamsBase, teamName); + fs.mkdirSync(teamDir, { recursive: true }); + fs.writeFileSync( + path.join(teamDir, 'config.json'), + JSON.stringify({ + name: teamName, + description: 'Solo team for prompt test', + members: [{ name: 'team-lead', agentType: 'team-lead' }], + }), + 'utf8' + ); + + vi.mocked(ClaudeBinaryResolver.resolve).mockResolvedValue('/fake/claude'); + const { child, writeSpy } = createFakeChild(); + vi.mocked(spawnCli).mockReturnValue(child as any); + + const svc = new TeamProvisioningService(); + (svc as any).buildProvisioningEnv = vi.fn(async () => ({ + env: { ANTHROPIC_API_KEY: 'test' }, + authSource: 'anthropic_api_key', + })); + (svc as any).normalizeTeamConfigForLaunch = vi.fn(async () => {}); + (svc as any).updateConfigProjectPath = vi.fn(async () => {}); + (svc as any).restorePrelaunchConfig = vi.fn(async () => {}); + (svc as any).resolveLaunchExpectedMembers = vi.fn(async () => ({ + members: [], + source: 'config-fallback', + warning: undefined, + })); + (svc as any).pathExists = vi.fn(async () => false); + (svc as any).startFilesystemMonitor = vi.fn(); + + const { runId } = await svc.launchTeam( + { + teamName, + cwd: process.cwd(), + clearContext: true, + } as any, + () => {} + ); + + expect(writeSpy).toHaveBeenCalledTimes(1); + const prompt = extractPromptFromWrite(writeSpy); + expect(prompt).toContain('SOLO MODE: This team CURRENTLY has ZERO teammates.'); + expect(prompt).toContain('Execute tasks sequentially and keep the board + user updated'); + expect(prompt).toContain('Do NOT start the next task until the current task is completed'); + + await svc.cancelProvisioning(runId); + }); +});