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.
This commit is contained in:
parent
075523dc42
commit
7fbf240c70
4 changed files with 288 additions and 9 deletions
|
|
@ -24,7 +24,12 @@ import { ScopeWarningBanner } from './ScopeWarningBanner';
|
||||||
import { ViewedProgressBar } from './ViewedProgressBar';
|
import { ViewedProgressBar } from './ViewedProgressBar';
|
||||||
|
|
||||||
import type { EditorView } from '@codemirror/view';
|
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';
|
import type { EditorSelectionAction, EditorSelectionInfo } from '@shared/types/editor';
|
||||||
|
|
||||||
interface ChangeReviewDialogProps {
|
interface ChangeReviewDialogProps {
|
||||||
|
|
@ -77,6 +82,7 @@ export const ChangeReviewDialog = ({
|
||||||
applyReview,
|
applyReview,
|
||||||
applySingleFileDecision,
|
applySingleFileDecision,
|
||||||
removeReviewFile,
|
removeReviewFile,
|
||||||
|
addReviewFile,
|
||||||
editedContents,
|
editedContents,
|
||||||
updateEditedContent,
|
updateEditedContent,
|
||||||
discardFileEdits,
|
discardFileEdits,
|
||||||
|
|
@ -116,6 +122,10 @@ export const ChangeReviewDialog = ({
|
||||||
const lastHunkActionAtRef = useRef<Record<string, number>>({});
|
const lastHunkActionAtRef = useRef<Record<string, number>>({});
|
||||||
const hunkDecisionUndoStackRef = useRef<Record<string, number[]>>({});
|
const hunkDecisionUndoStackRef = useRef<Record<string, number[]>>({});
|
||||||
const newFileApplyInFlightRef = useRef(new Set<string>());
|
const newFileApplyInFlightRef = useRef(new Set<string>());
|
||||||
|
const removedNewFileUndoStackRef = useRef<
|
||||||
|
{ file: FileChangeSummary; index: number; restoreContent: string; removedAt: number }[]
|
||||||
|
>([]);
|
||||||
|
const lastNewFileRemoveAtRef = useRef<number>(0);
|
||||||
|
|
||||||
// Proxy ref for useDiffNavigation (points to active file's editor)
|
// Proxy ref for useDiffNavigation (points to active file's editor)
|
||||||
const activeEditorViewRef = useRef<EditorView | null>(null);
|
const activeEditorViewRef = useRef<EditorView | null>(null);
|
||||||
|
|
@ -242,13 +252,32 @@ export const ChangeReviewDialog = ({
|
||||||
}
|
}
|
||||||
|
|
||||||
// Always apply immediately: rejecting a NEW file means deleting it from disk.
|
// Always apply immediately: rejecting a NEW file means deleting it from disk.
|
||||||
const isNew =
|
const file = activeChangeSet?.files.find((f) => f.filePath === filePath);
|
||||||
activeChangeSet?.files.find((f) => f.filePath === filePath)?.isNewFile ?? false;
|
const isNew = file?.isNewFile ?? false;
|
||||||
if (!isNew) return;
|
if (!isNew) return;
|
||||||
|
|
||||||
const result = await applySingleFileDecision(teamName, filePath, taskId, memberName);
|
const result = await applySingleFileDecision(teamName, filePath, taskId, memberName);
|
||||||
const hasErrorForFile = !!result?.errors.some((e) => e.filePath === filePath);
|
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);
|
removeReviewFile(filePath);
|
||||||
}
|
}
|
||||||
} finally {
|
} finally {
|
||||||
|
|
@ -263,6 +292,7 @@ export const ChangeReviewDialog = ({
|
||||||
taskId,
|
taskId,
|
||||||
memberName,
|
memberName,
|
||||||
removeReviewFile,
|
removeReviewFile,
|
||||||
|
fileContents,
|
||||||
]
|
]
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|
@ -575,6 +605,35 @@ export const ChangeReviewDialog = ({
|
||||||
// Prefer bulk undo (Accept All / Reject All) shortly after bulk action,
|
// 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).
|
// even if focus is inside a CM editor (focus often remains there after clicking buttons).
|
||||||
const now = Date.now();
|
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;
|
const bulkRecently = now - lastBulkActionAtRef.current < 10_000;
|
||||||
if (bulkRecently && useStore.getState().reviewUndoStack.length > 0) {
|
if (bulkRecently && useStore.getState().reviewUndoStack.length > 0) {
|
||||||
e.preventDefault();
|
e.preventDefault();
|
||||||
|
|
@ -616,7 +675,16 @@ export const ChangeReviewDialog = ({
|
||||||
};
|
};
|
||||||
document.addEventListener('keydown', handler, true);
|
document.addEventListener('keydown', handler, true);
|
||||||
return () => document.removeEventListener('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)
|
// Cmd+N IPC listener (forwarded from main process)
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
|
|
||||||
|
|
@ -153,10 +153,7 @@ export const ReviewToolbar = ({
|
||||||
Undo
|
Undo
|
||||||
</button>
|
</button>
|
||||||
</TooltipTrigger>
|
</TooltipTrigger>
|
||||||
<TooltipContent side="bottom">
|
<TooltipContent side="bottom">Undo last bulk operation (Ctrl+Z)</TooltipContent>
|
||||||
Undo last bulk operation (
|
|
||||||
{/Mac|iPhone|iPad/.test(navigator.userAgent) ? '⌘Z' : 'Ctrl+Z'})
|
|
||||||
</TooltipContent>
|
|
||||||
</Tooltip>
|
</Tooltip>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -19,6 +19,7 @@ import type {
|
||||||
ApplyReviewRequest,
|
ApplyReviewRequest,
|
||||||
ApplyReviewResult,
|
ApplyReviewResult,
|
||||||
ChangeStats,
|
ChangeStats,
|
||||||
|
FileChangeSummary,
|
||||||
FileChangeWithContent,
|
FileChangeWithContent,
|
||||||
FileReviewDecision,
|
FileReviewDecision,
|
||||||
HunkDecision,
|
HunkDecision,
|
||||||
|
|
@ -125,6 +126,11 @@ export interface ChangeReviewSlice {
|
||||||
) => Promise<ApplyReviewResult | null>;
|
) => Promise<ApplyReviewResult | null>;
|
||||||
/** Remove a file from the current review set (used for rejecting new files) */
|
/** Remove a file from the current review set (used for rejecting new files) */
|
||||||
removeReviewFile: (filePath: string) => void;
|
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;
|
invalidateChangeStats: (teamName: string) => void;
|
||||||
|
|
||||||
// Editable diff actions
|
// Editable diff actions
|
||||||
|
|
@ -841,6 +847,48 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
|
||||||
});
|
});
|
||||||
},
|
},
|
||||||
|
|
||||||
|
addReviewFile: (
|
||||||
|
file: FileChangeSummary,
|
||||||
|
options?: { index?: number; content?: FileChangeWithContent }
|
||||||
|
) => {
|
||||||
|
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 ──
|
// ── Editable diff actions ──
|
||||||
|
|
||||||
updateEditedContent: (filePath: string, content: string) => {
|
updateEditedContent: (filePath: string, content: string) => {
|
||||||
|
|
|
||||||
166
test/main/services/team/TeamProvisioningServicePrompts.test.ts
Normal file
166
test/main/services/team/TeamProvisioningServicePrompts.test.ts
Normal file
|
|
@ -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<typeof import('@main/utils/pathDecoder')>();
|
||||||
|
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<typeof vi.fn>): 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);
|
||||||
|
});
|
||||||
|
});
|
||||||
Loading…
Reference in a new issue