diff --git a/.changeset/flat-jobs-wink.md b/.changeset/flat-jobs-wink.md new file mode 100644 index 00000000000..803161cd514 --- /dev/null +++ b/.changeset/flat-jobs-wink.md @@ -0,0 +1,6 @@ +--- +"kilo-code": major +"@kilocode/cli": patch +--- + +Add Local review mode diff --git a/cli/src/state/atoms/__tests__/keyboard.test.ts b/cli/src/state/atoms/__tests__/keyboard.test.ts index 056a27555e9..473e090421a 100644 --- a/cli/src/state/atoms/__tests__/keyboard.test.ts +++ b/cli/src/state/atoms/__tests__/keyboard.test.ts @@ -1182,8 +1182,8 @@ describe("keypress atoms", () => { it("should wrap around to first mode when at the last mode", async () => { // Set initial mode to the last default mode - // DEFAULT_MODES order: architect, code, ask, debug, orchestrator - store.set(extensionModeAtom, "orchestrator") + // DEFAULT_MODES order: architect, code, ask, debug, orchestrator, review + store.set(extensionModeAtom, "review") store.set(customModesAtom, []) // Press Shift+Tab @@ -1206,8 +1206,8 @@ describe("keypress atoms", () => { }) it("should include custom modes in the cycle", async () => { - // Set initial mode to "orchestrator" (last default mode) - store.set(extensionModeAtom, "orchestrator") + // Set initial mode to "review" (last default mode) + store.set(extensionModeAtom, "review") // Add a custom mode store.set(customModesAtom, [ { @@ -1233,7 +1233,7 @@ describe("keypress atoms", () => { // Wait for async operations to complete await new Promise((resolve) => setTimeout(resolve, 10)) - // Should have cycled to the custom mode (after orchestrator) + // Should have cycled to the custom mode (after review) const newMode = store.get(extensionModeAtom) expect(newMode).toBe("custom-mode") }) diff --git a/packages/types/src/mode.ts b/packages/types/src/mode.ts index 572d586cdff..b74cfaddf9f 100644 --- a/packages/types/src/mode.ts +++ b/packages/types/src/mode.ts @@ -213,4 +213,66 @@ export const DEFAULT_MODES: readonly ModeConfig[] = [ customInstructions: "Your role is to coordinate complex workflows by delegating tasks to specialized modes. As an orchestrator, you should:\n\n1. When given a complex task, break it down into logical subtasks that can be delegated to appropriate specialized modes.\n\n2. For each subtask, use the `new_task` tool to delegate. Choose the most appropriate mode for the subtask's specific goal and provide comprehensive instructions in the `message` parameter. These instructions must include:\n * All necessary context from the parent task or previous subtasks required to complete the work.\n * A clearly defined scope, specifying exactly what the subtask should accomplish.\n * An explicit statement that the subtask should *only* perform the work outlined in these instructions and not deviate.\n * An instruction for the subtask to signal completion by using the `attempt_completion` tool, providing a concise yet thorough summary of the outcome in the `result` parameter, keeping in mind that this summary will be the source of truth used to keep track of what was completed on this project.\n * A statement that these specific instructions supersede any conflicting general instructions the subtask's mode might have.\n\n3. Track and manage the progress of all subtasks. When a subtask is completed, analyze its results and determine the next steps.\n\n4. Help the user understand how the different subtasks fit together in the overall workflow. Provide clear reasoning about why you're delegating specific tasks to specific modes.\n\n5. When all subtasks are completed, synthesize the results and provide a comprehensive overview of what was accomplished.\n\n6. Ask clarifying questions when necessary to better understand how to break down complex tasks effectively.\n\n7. Suggest improvements to the workflow based on the results of completed subtasks.\n\nUse subtasks to maintain clarity. If a request significantly shifts focus or requires a different expertise (mode), consider creating a subtask rather than overloading the current one.", }, + // kilocode_change start - Review mode for local code reviews + { + slug: "review", + name: "Review", + iconName: "codicon-git-compare", + roleDefinition: + "You are Kilo Code, an expert code reviewer with deep expertise in software engineering best practices, security vulnerabilities, performance optimization, and code quality. Your role is advisory - provide clear, actionable feedback on code quality and potential issues.", + whenToUse: + "Use this mode when you need to review code changes. Ideal for reviewing uncommitted work before committing, comparing your branch against main/develop, or analyzing changes before merging.", + description: "Review code changes locally", + groups: ["read", "browser", "mcp", "command"], + customInstructions: `When you enter Review mode, you will receive a list of changed files. Use tools to explore the changes dynamically. + +## How to Review + +1. **Start with git diff**: Use \`execute_command\` to run \`git diff\` (for uncommitted) or \`git diff ..HEAD\` (for branch) to see the actual changes. + +2. **Examine specific files**: For complex changes, use \`read_file\` to see the full file context, not just the diff. + +3. **Gather history context**: Use \`git log\`, \`git blame\`, or \`git show\` when you need to understand why code was written a certain way. + +4. **Be confident**: Only flag issues where you have high confidence. Use these thresholds: + - **CRITICAL (95%+)**: Security vulnerabilities, data loss risks, crashes, authentication bypasses + - **WARNING (85%+)**: Bugs, logic errors, performance issues, unhandled errors + - **SUGGESTION (75%+)**: Code quality improvements, best practices, maintainability + - **Below 75%**: Don't comment - gather more context first + +5. **Focus on what matters**: + - Security: Injection, auth issues, data exposure + - Bugs: Logic errors, null handling, race conditions + - Performance: Inefficient algorithms, memory leaks + - Error handling: Missing try-catch, unhandled promises + +6. **Don't flag**: + - Style preferences that don't affect functionality + - Minor naming suggestions + - Patterns that match existing codebase conventions + +## Output Format + +### Summary +2-3 sentences describing what this change does and your overall assessment. + +### Issues Found +| Severity | File:Line | Issue | +|----------|-----------|-------| +| CRITICAL | path/file.ts:42 | Brief description | +| WARNING | path/file.ts:78 | Brief description | + +If no issues: "No issues found." + +### Detailed Findings +For each issue: +- **File:** \`path/to/file.ts:line\` +- **Confidence:** X% +- **Problem:** What's wrong and why it matters +- **Suggestion:** Recommended fix with code snippet + +### Recommendation +One of: **APPROVE** | **APPROVE WITH SUGGESTIONS** | **NEEDS CHANGES** | **NEEDS DISCUSSION**`, + }, + // kilocode_change end ] as const diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 6a126407230..136434f98ad 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -256,7 +256,8 @@ export interface ExtensionMessage { | "customToolsResult" | "modes" | "taskWithAggregatedCosts" - | "skillsData" // kilocode_change: Skills data response + | "skillsData" + | "askReviewScope" // kilocode_change: Review mode scope selection text?: string // kilocode_change start completionRequestId?: string // Correlation ID from request @@ -442,6 +443,23 @@ export interface ExtensionMessage { childrenCost: number } historyItem?: HistoryItem + // kilocode_change start: Review mode + reviewScopeInfo?: { + uncommitted: { + available: boolean + fileCount: number + filePreview?: string[] + } + branch: { + available: boolean + currentBranch: string + baseBranch: string + fileCount: number + filePreview?: string[] + } + error?: string + } + // kilocode_change end: Review mode } export type ExtensionState = Pick< @@ -929,7 +947,8 @@ export interface WebviewMessage { | "requestModes" | "switchMode" | "debugSetting" - | "refreshSkills" // kilocode_change: Request skills data refresh + | "refreshSkills" + | "reviewScopeSelected" // kilocode_change: Review mode scope selection text?: string suggestionLength?: number // kilocode_change: Length of accepted suggestion for telemetry completionRequestId?: string // kilocode_change @@ -1049,6 +1068,9 @@ export interface WebviewMessage { codebaseIndexOpenRouterApiKey?: string } updatedSettings?: RooCodeSettings + // kilocode_change start: Review mode + reviewScope?: "uncommitted" | "branch" + // kilocode_change end: Review mode } // kilocode_change: Create discriminated union for type-safe messages diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 394790e8bfc..108c8985795 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1493,6 +1493,12 @@ export class ClineProvider } await this.postStateToWebview() + + // kilocode_change start: Review mode scope selection + if (newMode === "review") { + await this.triggerReviewScopeSelection() + } + // kilocode_change end } // Provider Profile Management @@ -3379,6 +3385,64 @@ export class ClineProvider await this.setValues({ mode }) } + // kilocode_change start: Review mode + /** + * Triggers the review scope selection UI + * Called when user enters review mode + */ + public async triggerReviewScopeSelection(): Promise { + try { + const cwd = getWorkspacePath() + if (!cwd) { + this.log("Cannot start review: no workspace folder open") + return + } + + const { ReviewService } = await import("../../services/review") + const reviewService = new ReviewService({ cwd }) + const scopeInfo = await reviewService.getScopeInfo() + + await this.postMessageToWebview({ + type: "askReviewScope", + reviewScopeInfo: scopeInfo, + }) + } catch (error) { + this.log( + `Error triggering review scope selection: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } + + /** + * Handles the user's review scope selection + * Gets lightweight summary and starts the review task + * The agent will dynamically explore changes using tools + */ + public async handleReviewScopeSelected(scope: "uncommitted" | "branch"): Promise { + try { + const cwd = getWorkspacePath() + if (!cwd) { + this.log("Cannot start review: no workspace folder open") + vscode.window.showErrorMessage("Cannot start review: no workspace folder open") + return + } + + const { ReviewService, buildReviewPrompt } = await import("../../services/review") + const reviewService = new ReviewService({ cwd }) + + // Get lightweight summary - agent will explore details with tools + const summary = await reviewService.getReviewSummary(scope) + + // Build the review prompt and start the task + // Let the agent handle cases with no changes - it can explain and offer alternatives + const reviewPrompt = buildReviewPrompt(summary) + await this.createTask(reviewPrompt) + } catch (error) { + this.log(`Error handling review scope selection: ${error instanceof Error ? error.message : String(error)}`) + } + } + // kilocode_change end: Review mode + // Provider Profiles public async getProviderProfiles(): Promise<{ name: string; provider?: string }[]> { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 536fc668bf6..db9d8247a28 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -4594,6 +4594,16 @@ export const webviewMessageHandler = async ( break } + // kilocode_change start: Review mode scope selection + case "reviewScopeSelected": { + const scope = message.reviewScope + if (scope === "uncommitted" || scope === "branch") { + await provider.handleReviewScopeSelected(scope) + } + break + } + // kilocode_change end + default: { // console.log(`Unhandled message type: ${message.type}`) // diff --git a/src/services/review/ReviewService.ts b/src/services/review/ReviewService.ts new file mode 100644 index 00000000000..f342359b445 --- /dev/null +++ b/src/services/review/ReviewService.ts @@ -0,0 +1,168 @@ +// kilocode_change - new file + +/** + * Review Service + * + * Lightweight service for gathering review scope information. + * The actual review is done dynamically by the agent using tools. + */ + +import type { ReviewSummary, ReviewScopeInfo, ReviewServiceOptions, FileSummary, FileStatus } from "./types" +import { + getCurrentBranch, + detectBaseBranch, + hasUncommittedChanges, + getUncommittedFiles, + getBranchFilesChanged, + isOnBaseBranch, + type GitFileChange, +} from "../../utils/git" + +/** + * Converts git status code to FileStatus + */ +function gitStatusToFileStatus(status: string): FileStatus { + const upper = status.toUpperCase() + if (upper === "A" || upper === "?") return "A" + if (upper === "M") return "M" + if (upper === "D") return "D" + if (upper === "R") return "R" + if (upper === "C") return "C" + if (upper === "U") return "U" + return "M" // Default to modified +} + +/** + * Converts GitFileChange array to FileSummary array + */ +function toFileSummaries(files: GitFileChange[]): FileSummary[] { + return files.map((file) => ({ + path: file.path, + status: gitStatusToFileStatus(file.status), + oldPath: file.oldPath, + })) +} + +/** + * ReviewService - Provides review scope information for the UI + * and lightweight summaries for the agent to start reviews + */ +export class ReviewService { + private cwd: string + + constructor(options: ReviewServiceOptions) { + this.cwd = options.cwd + } + + /** + * Gets information about available review scopes + * Used by UI to show preview before user selects + */ + async getScopeInfo(): Promise { + try { + // Get current branch + const currentBranch = (await getCurrentBranch(this.cwd)) || "HEAD" + + // Check uncommitted changes + const hasUncommitted = await hasUncommittedChanges(this.cwd) + const uncommittedFiles = hasUncommitted ? await getUncommittedFiles(this.cwd) : [] + + // Check branch diff - available as long as not on base branch + const onBaseBranch = await isOnBaseBranch(this.cwd) + const baseBranch = await detectBaseBranch(this.cwd) + const branchFiles = !onBaseBranch ? await getBranchFilesChanged(this.cwd, baseBranch) : [] + + const result = { + uncommitted: { + available: hasUncommitted, + fileCount: uncommittedFiles.length, + filePreview: uncommittedFiles.slice(0, 5).map((f) => f.path), + }, + branch: { + available: !onBaseBranch, + currentBranch, + baseBranch, + fileCount: branchFiles.length, + filePreview: branchFiles.slice(0, 5).map((f) => f.path), + }, + } + + return result + } catch (error) { + console.error("Error getting scope info:", error) + return { + uncommitted: { + available: false, + fileCount: 0, + }, + branch: { + available: false, + currentBranch: "unknown", + baseBranch: "main", + fileCount: 0, + }, + error: error instanceof Error ? error.message : String(error), + } + } + } + + /** + * Gets a lightweight review summary for the selected scope + * This provides minimal context - the agent will explore details with tools + */ + async getReviewSummary(scope: "uncommitted" | "branch"): Promise { + try { + const currentBranch = (await getCurrentBranch(this.cwd)) || "HEAD" + + if (scope === "uncommitted") { + return this.getUncommittedSummary(currentBranch) + } else { + return this.getBranchSummary(currentBranch) + } + } catch (error) { + console.error(`Error getting review summary for ${scope}:`, error) + return { + scope, + currentBranch: "unknown", + files: [], + totalFiles: 0, + hasChanges: false, + error: error instanceof Error ? error.message : String(error), + } + } + } + + /** + * Gets summary for uncommitted changes + */ + private async getUncommittedSummary(currentBranch: string): Promise { + const gitFiles = await getUncommittedFiles(this.cwd) + const files = toFileSummaries(gitFiles) + + return { + scope: "uncommitted", + currentBranch, + files, + totalFiles: files.length, + hasChanges: files.length > 0, + } + } + + /** + * Gets summary for branch comparison + */ + private async getBranchSummary(currentBranch: string): Promise { + const baseBranch = await detectBaseBranch(this.cwd) + const gitFiles = await getBranchFilesChanged(this.cwd, baseBranch) + const files = toFileSummaries(gitFiles) + + return { + scope: "branch", + currentBranch, + baseBranch, + files, + totalFiles: files.length, + hasChanges: files.length > 0, + } + } +} diff --git a/src/services/review/__tests__/ReviewService.spec.ts b/src/services/review/__tests__/ReviewService.spec.ts new file mode 100644 index 00000000000..83f0260598f --- /dev/null +++ b/src/services/review/__tests__/ReviewService.spec.ts @@ -0,0 +1,241 @@ +// npx vitest run services/review/__tests__/ReviewService.spec.ts + +// Use vi.hoisted to ensure mocks are available during hoisting +const { + mockGetCurrentBranch, + mockDetectBaseBranch, + mockHasUncommittedChanges, + mockGetUncommittedFiles, + mockGetBranchFilesChanged, + mockIsOnBaseBranch, +} = vi.hoisted(() => ({ + mockGetCurrentBranch: vi.fn(), + mockDetectBaseBranch: vi.fn(), + mockHasUncommittedChanges: vi.fn(), + mockGetUncommittedFiles: vi.fn(), + mockGetBranchFilesChanged: vi.fn(), + mockIsOnBaseBranch: vi.fn(), +})) + +vi.mock("../../../utils/git", () => ({ + getCurrentBranch: mockGetCurrentBranch, + detectBaseBranch: mockDetectBaseBranch, + hasUncommittedChanges: mockHasUncommittedChanges, + getUncommittedFiles: mockGetUncommittedFiles, + getBranchFilesChanged: mockGetBranchFilesChanged, + isOnBaseBranch: mockIsOnBaseBranch, +})) + +import { ReviewService } from "../ReviewService" + +describe("ReviewService", () => { + const cwd = "/test/project" + let service: ReviewService + + beforeEach(() => { + vi.clearAllMocks() + service = new ReviewService({ cwd }) + }) + + describe("getScopeInfo", () => { + it("returns scope info when both uncommitted and branch changes are available", async () => { + mockGetCurrentBranch.mockResolvedValue("feature/test") + mockHasUncommittedChanges.mockResolvedValue(true) + mockGetUncommittedFiles.mockResolvedValue([ + { path: "src/file1.ts", status: "M" }, + { path: "src/file2.ts", status: "A" }, + ]) + mockIsOnBaseBranch.mockResolvedValue(false) + mockDetectBaseBranch.mockResolvedValue("main") + mockGetBranchFilesChanged.mockResolvedValue([ + { path: "src/file1.ts", status: "M" }, + { path: "src/file3.ts", status: "A" }, + { path: "src/file4.ts", status: "D" }, + ]) + + const result = await service.getScopeInfo() + + expect(result.uncommitted.available).toBe(true) + expect(result.uncommitted.fileCount).toBe(2) + expect(result.uncommitted.filePreview).toEqual(["src/file1.ts", "src/file2.ts"]) + + expect(result.branch.available).toBe(true) + expect(result.branch.currentBranch).toBe("feature/test") + expect(result.branch.baseBranch).toBe("main") + expect(result.branch.fileCount).toBe(3) + expect(result.branch.filePreview).toEqual(["src/file1.ts", "src/file3.ts", "src/file4.ts"]) + }) + + it("returns unavailable uncommitted when no uncommitted changes", async () => { + mockGetCurrentBranch.mockResolvedValue("feature/test") + mockHasUncommittedChanges.mockResolvedValue(false) + mockIsOnBaseBranch.mockResolvedValue(false) + mockDetectBaseBranch.mockResolvedValue("main") + mockGetBranchFilesChanged.mockResolvedValue([{ path: "src/file.ts", status: "M" }]) + + const result = await service.getScopeInfo() + + expect(result.uncommitted.available).toBe(false) + expect(result.uncommitted.fileCount).toBe(0) + expect(result.branch.available).toBe(true) + }) + + it("returns unavailable branch when on base branch", async () => { + mockGetCurrentBranch.mockResolvedValue("main") + mockHasUncommittedChanges.mockResolvedValue(true) + mockGetUncommittedFiles.mockResolvedValue([{ path: "src/file.ts", status: "M" }]) + mockIsOnBaseBranch.mockResolvedValue(true) + mockDetectBaseBranch.mockResolvedValue("main") + + const result = await service.getScopeInfo() + + expect(result.uncommitted.available).toBe(true) + expect(result.branch.available).toBe(false) + }) + + it("returns branch available even with zero file changes", async () => { + mockGetCurrentBranch.mockResolvedValue("feature/test") + mockHasUncommittedChanges.mockResolvedValue(false) + mockIsOnBaseBranch.mockResolvedValue(false) + mockDetectBaseBranch.mockResolvedValue("main") + mockGetBranchFilesChanged.mockResolvedValue([]) + + const result = await service.getScopeInfo() + + // Branch is available as long as not on base branch - agent will handle "no changes" + expect(result.branch.available).toBe(true) + expect(result.branch.fileCount).toBe(0) + }) + + it("handles errors gracefully", async () => { + mockGetCurrentBranch.mockRejectedValue(new Error("Git not found")) + + const result = await service.getScopeInfo() + + expect(result.uncommitted.available).toBe(false) + expect(result.branch.available).toBe(false) + expect(result.error).toBe("Git not found") + }) + + it("limits file preview to 5 files", async () => { + mockGetCurrentBranch.mockResolvedValue("feature/test") + mockHasUncommittedChanges.mockResolvedValue(true) + mockGetUncommittedFiles.mockResolvedValue([ + { path: "src/file1.ts", status: "M" }, + { path: "src/file2.ts", status: "M" }, + { path: "src/file3.ts", status: "M" }, + { path: "src/file4.ts", status: "M" }, + { path: "src/file5.ts", status: "M" }, + { path: "src/file6.ts", status: "M" }, + { path: "src/file7.ts", status: "M" }, + ]) + mockIsOnBaseBranch.mockResolvedValue(true) + mockDetectBaseBranch.mockResolvedValue("main") + + const result = await service.getScopeInfo() + + expect(result.uncommitted.fileCount).toBe(7) + expect(result.uncommitted.filePreview).toHaveLength(5) + }) + }) + + describe("getReviewSummary", () => { + describe("uncommitted scope", () => { + it("returns summary with changed files", async () => { + mockGetCurrentBranch.mockResolvedValue("feature/test") + mockGetUncommittedFiles.mockResolvedValue([ + { path: "src/file1.ts", status: "M" }, + { path: "src/file2.ts", status: "A" }, + { path: "src/file3.ts", status: "D" }, + ]) + + const result = await service.getReviewSummary("uncommitted") + + expect(result.scope).toBe("uncommitted") + expect(result.currentBranch).toBe("feature/test") + expect(result.baseBranch).toBeUndefined() + expect(result.totalFiles).toBe(3) + expect(result.hasChanges).toBe(true) + expect(result.files).toHaveLength(3) + expect(result.files[0]).toEqual({ path: "src/file1.ts", status: "M", oldPath: undefined }) + expect(result.files[1]).toEqual({ path: "src/file2.ts", status: "A", oldPath: undefined }) + expect(result.files[2]).toEqual({ path: "src/file3.ts", status: "D", oldPath: undefined }) + }) + + it("returns hasChanges=false when no uncommitted files", async () => { + mockGetCurrentBranch.mockResolvedValue("main") + mockGetUncommittedFiles.mockResolvedValue([]) + + const result = await service.getReviewSummary("uncommitted") + + expect(result.hasChanges).toBe(false) + expect(result.totalFiles).toBe(0) + expect(result.files).toHaveLength(0) + }) + + it("converts untracked (?) to added (A) status", async () => { + mockGetCurrentBranch.mockResolvedValue("main") + mockGetUncommittedFiles.mockResolvedValue([{ path: "new-file.ts", status: "?" }]) + + const result = await service.getReviewSummary("uncommitted") + + expect(result.files[0].status).toBe("A") + }) + }) + + describe("branch scope", () => { + it("returns summary with branch comparison", async () => { + mockGetCurrentBranch.mockResolvedValue("feature/test") + mockDetectBaseBranch.mockResolvedValue("main") + mockGetBranchFilesChanged.mockResolvedValue([ + { path: "src/a.ts", status: "M" }, + { path: "src/b.ts", status: "A" }, + ]) + + const result = await service.getReviewSummary("branch") + + expect(result.scope).toBe("branch") + expect(result.currentBranch).toBe("feature/test") + expect(result.baseBranch).toBe("main") + expect(result.totalFiles).toBe(2) + expect(result.hasChanges).toBe(true) + }) + + it("returns hasChanges=false when no branch diff", async () => { + mockGetCurrentBranch.mockResolvedValue("main") + mockDetectBaseBranch.mockResolvedValue("main") + mockGetBranchFilesChanged.mockResolvedValue([]) + + const result = await service.getReviewSummary("branch") + + expect(result.hasChanges).toBe(false) + expect(result.totalFiles).toBe(0) + }) + + it("handles renamed files with oldPath", async () => { + mockGetCurrentBranch.mockResolvedValue("feature/test") + mockDetectBaseBranch.mockResolvedValue("main") + mockGetBranchFilesChanged.mockResolvedValue([ + { path: "src/new-name.ts", status: "R", oldPath: "src/old-name.ts" }, + ]) + + const result = await service.getReviewSummary("branch") + + expect(result.files[0]).toEqual({ + path: "src/new-name.ts", + status: "R", + oldPath: "src/old-name.ts", + }) + }) + }) + + it("handles errors gracefully", async () => { + mockGetCurrentBranch.mockRejectedValue(new Error("Git failed")) + + const result = await service.getReviewSummary("uncommitted") + + expect(result.hasChanges).toBe(false) + expect(result.error).toBe("Git failed") + }) + }) +}) diff --git a/src/services/review/__tests__/prompts.spec.ts b/src/services/review/__tests__/prompts.spec.ts new file mode 100644 index 00000000000..75e37d82d88 --- /dev/null +++ b/src/services/review/__tests__/prompts.spec.ts @@ -0,0 +1,200 @@ +// npx vitest run services/review/__tests__/prompts.spec.ts + +import { buildReviewPrompt } from "../prompts" +import type { ReviewSummary } from "../types" + +describe("buildReviewPrompt", () => { + describe("uncommitted scope", () => { + it("builds prompt for uncommitted changes", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "feature/test", + files: [ + { path: "src/file1.ts", status: "M" }, + { path: "src/file2.ts", status: "A" }, + { path: "src/file3.ts", status: "D" }, + ], + totalFiles: 3, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary) + + expect(prompt).toContain("**uncommitted changes**") + expect(prompt).toContain("`feature/test`") + expect(prompt).toContain("Changed Files (3)") + expect(prompt).toContain("[modified] src/file1.ts") + expect(prompt).toContain("[added] src/file2.ts") + expect(prompt).toContain("[deleted] src/file3.ts") + expect(prompt).toContain("git diff") + expect(prompt).not.toContain("baseBranch") + }) + + it("includes git diff --cached suggestion", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "main", + files: [{ path: "file.ts", status: "M" }], + totalFiles: 1, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary) + + expect(prompt).toContain("git diff --cached") + }) + }) + + describe("branch scope", () => { + it("builds prompt for branch diff", () => { + const summary: ReviewSummary = { + scope: "branch", + currentBranch: "feature/new-feature", + baseBranch: "main", + files: [ + { path: "src/new.ts", status: "A" }, + { path: "src/modified.ts", status: "M" }, + ], + totalFiles: 2, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary) + + expect(prompt).toContain("**branch diff**") + expect(prompt).toContain("`feature/new-feature`") + expect(prompt).toContain("`main`") + expect(prompt).toContain("Changed Files (2)") + expect(prompt).toContain("[added] src/new.ts") + expect(prompt).toContain("[modified] src/modified.ts") + expect(prompt).toContain("git diff main") + }) + + it("handles renamed files with arrow notation", () => { + const summary: ReviewSummary = { + scope: "branch", + currentBranch: "feature/rename", + baseBranch: "main", + files: [{ path: "src/new-name.ts", status: "R", oldPath: "src/old-name.ts" }], + totalFiles: 1, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary) + + expect(prompt).toContain("[renamed] src/old-name.ts → src/new-name.ts") + }) + }) + + describe("empty changes", () => { + it("shows no files message when empty", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "main", + files: [], + totalFiles: 0, + hasChanges: false, + } + + const prompt = buildReviewPrompt(summary) + + expect(prompt).toContain("Changed Files (0)") + expect(prompt).toContain("No files changed.") + }) + }) + + describe("user input", () => { + it("includes user instructions when provided", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "main", + files: [{ path: "file.ts", status: "M" }], + totalFiles: 1, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary, "Focus on security issues only") + + expect(prompt).toContain("## Additional Instructions") + expect(prompt).toContain("Focus on security issues only") + }) + + it("does not include additional instructions section when empty", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "main", + files: [{ path: "file.ts", status: "M" }], + totalFiles: 1, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary, "") + + expect(prompt).not.toContain("## Additional Instructions") + }) + + it("trims whitespace from user input", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "main", + files: [{ path: "file.ts", status: "M" }], + totalFiles: 1, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary, " Check for bugs ") + + expect(prompt).toContain("Check for bugs") + expect(prompt).not.toContain(" Check for bugs ") + }) + }) + + describe("file status labels", () => { + it("maps all status codes to labels", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "main", + files: [ + { path: "added.ts", status: "A" }, + { path: "modified.ts", status: "M" }, + { path: "deleted.ts", status: "D" }, + { path: "renamed.ts", status: "R" }, + { path: "copied.ts", status: "C" }, + { path: "unmerged.ts", status: "U" }, + { path: "untracked.ts", status: "?" }, + ], + totalFiles: 7, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary) + + expect(prompt).toContain("[added] added.ts") + expect(prompt).toContain("[modified] modified.ts") + expect(prompt).toContain("[deleted] deleted.ts") + expect(prompt).toContain("[renamed] renamed.ts") + expect(prompt).toContain("[copied] copied.ts") + expect(prompt).toContain("[unmerged] unmerged.ts") + expect(prompt).toContain("[untracked] untracked.ts") + }) + }) + + describe("tool instructions", () => { + it("includes execute_command instructions", () => { + const summary: ReviewSummary = { + scope: "uncommitted", + currentBranch: "main", + files: [{ path: "file.ts", status: "M" }], + totalFiles: 1, + hasChanges: true, + } + + const prompt = buildReviewPrompt(summary) + + expect(prompt).toContain("`execute_command`") + expect(prompt).toContain("`read_file`") + expect(prompt).toContain("`git log`") + expect(prompt).toContain("`git blame`") + }) + }) +}) diff --git a/src/services/review/index.ts b/src/services/review/index.ts new file mode 100644 index 00000000000..d173525bff6 --- /dev/null +++ b/src/services/review/index.ts @@ -0,0 +1,25 @@ +// kilocode_change - new file + +/** + * Review Service Module + * + * Provides lightweight local code review capabilities using git. + * The agent dynamically explores changes using tools rather than + * receiving pre-generated diff content. + */ + +// Types +export type { + ReviewScope, + FileStatus, + FileSummary, + ReviewSummary, + ReviewServiceOptions, + ReviewScopeInfo, +} from "./types" + +// Service +export { ReviewService } from "./ReviewService" + +// Prompts +export { buildReviewPrompt } from "./prompts" diff --git a/src/services/review/prompts.ts b/src/services/review/prompts.ts new file mode 100644 index 00000000000..aa0320db1a1 --- /dev/null +++ b/src/services/review/prompts.ts @@ -0,0 +1,72 @@ +// kilocode_change - new file + +/** + * Review Prompt Builder + * + * Builds lightweight review prompts that provide minimal context. + * The agent will dynamically explore changes using tools. + */ + +import type { ReviewSummary, FileStatus } from "./types" + +/** + * Status code descriptions for display + */ +const STATUS_LABELS: Record = { + A: "added", + M: "modified", + D: "deleted", + R: "renamed", + C: "copied", + U: "unmerged", + "?": "untracked", +} + +/** + * Builds the review prompt with minimal context + * The agent will use tools to explore the actual changes + * + * @param summary The lightweight review summary + * @param userInput Optional additional instructions from the user + * @returns The review prompt to start the task + */ +export function buildReviewPrompt(summary: ReviewSummary, userInput?: string): string { + const scopeDescription = + summary.scope === "uncommitted" + ? "**uncommitted changes** (staged and unstaged)" + : `**branch diff**: \`${summary.currentBranch}\` → \`${summary.baseBranch}\`` + + // Format file list + const fileList = summary.files + .map((f) => { + const status = STATUS_LABELS[f.status] || f.status + const path = f.oldPath ? `${f.oldPath} → ${f.path}` : f.path + return ` [${status}] ${path}` + }) + .join("\n") + + const userInstructions = userInput?.trim() ? `\n## Additional Instructions\n${userInput.trim()}\n` : "" + + return `I need you to review ${scopeDescription}. + +**Branch:** \`${summary.currentBranch}\`${summary.baseBranch ? `\n**Base:** \`${summary.baseBranch}\`` : ""} + +## Changed Files (${summary.totalFiles}) +${fileList || "No files changed."} +${userInstructions} +## Instructions + +Use the following tools to explore the changes: +${ + summary.scope === "uncommitted" + ? `- \`execute_command\` with \`git diff HEAD\` to see all uncommitted changes +- \`execute_command\` with \`git diff --cached\` for staged changes only +- \`execute_command\` with \`git diff\` for unstaged changes only` + : `- \`execute_command\` with \`git diff ${summary.baseBranch}\` to see all changes vs base branch +- \`execute_command\` with \`git diff ${summary.baseBranch} -- \` for specific file changes` +} +- \`read_file\` to examine full file context when needed +- \`execute_command\` with \`git log\` or \`git blame\` for history context + +Start by examining the diff, then provide your review following the standard format.` +} diff --git a/src/services/review/types.ts b/src/services/review/types.ts new file mode 100644 index 00000000000..253868f7297 --- /dev/null +++ b/src/services/review/types.ts @@ -0,0 +1,90 @@ +// kilocode_change - new file + +/** + * Review Mode Types + * + * Type definitions for the local code review feature. + * Simplified for dynamic tool-based review approach. + */ + +/** + * Review scope - what the user wants to review + */ +export type ReviewScope = "uncommitted" | "branch" + +/** + * File status from git + */ +export type FileStatus = "A" | "M" | "D" | "R" | "C" | "U" | "?" + +/** + * Summary of a single changed file + */ +export interface FileSummary { + /** File path relative to repository root */ + path: string + /** Change status: A=added, M=modified, D=deleted, R=renamed, C=copied, U=unmerged, ?=untracked */ + status: FileStatus + /** Original path for renamed/copied files */ + oldPath?: string +} + +/** + * Lightweight review summary - minimal context for dynamic review + * The agent will use tools to explore details as needed + */ +export interface ReviewSummary { + /** The review scope selected by user */ + scope: ReviewScope + /** Current branch name */ + currentBranch: string + /** Base branch for comparison (only for branch scope) */ + baseBranch?: string + /** List of changed files with their status */ + files: FileSummary[] + /** Total number of files changed */ + totalFiles: number + /** Whether there are any changes to review */ + hasChanges: boolean + /** Error message if summary gathering failed */ + error?: string +} + +/** + * Options for the ReviewService + */ +export interface ReviewServiceOptions { + /** Working directory (repository root) */ + cwd: string +} + +/** + * Information about available review scopes + * Used by UI to show preview before user selects + */ +export interface ReviewScopeInfo { + /** Info about uncommitted changes scope */ + uncommitted: { + /** Whether this scope is available (has changes) */ + available: boolean + /** Number of files with uncommitted changes */ + fileCount: number + /** Preview of file names (first few) */ + filePreview?: string[] + } + /** Info about branch comparison scope */ + branch: { + /** Whether this scope is available (not on base branch, has commits) */ + available: boolean + /** Current branch name */ + currentBranch: string + /** Detected base branch for comparison */ + baseBranch: string + /** Number of files changed between branches */ + fileCount: number + /** Preview of file names (first few) */ + filePreview?: string[] + } + /** Error message if unable to get scope info */ + error?: string +} diff --git a/src/utils/git.ts b/src/utils/git.ts index e64bde65198..529546e079b 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -422,3 +422,363 @@ export async function getCurrentBranch(cwd: string): Promise return undefined } } + +// kilocode_change start - Review mode git utilities + +/** + * File change info from git status or diff + */ +export interface GitFileChange { + /** File path relative to repository root */ + path: string + /** Git status code (M, A, D, R, C, U, ?) */ + status: string + /** Original path for renamed files */ + oldPath?: string +} + +/** + * Detects the base branch (main/master/develop) from remote or local branches + * @param cwd The working directory + * @returns The detected base branch name (defaults to "main" if detection fails) + */ +export async function detectBaseBranch(cwd: string): Promise { + try { + const isInstalled = await checkGitInstalled() + if (!isInstalled) { + return "main" + } + + const isRepo = await checkGitRepo(cwd) + if (!isRepo) { + return "main" + } + + // Try to get the default branch from remote + try { + const { stdout } = await execAsync("git symbolic-ref refs/remotes/origin/HEAD", { cwd }) + const remoteBranch = stdout.trim().replace(/^refs\/remotes\/origin\//, "") + if (remoteBranch) { + return remoteBranch + } + } catch { + // Remote HEAD not set, continue with fallback + } + + // Fallback: check which common base branches exist locally + const baseBranchCandidates = ["main", "master", "develop", "development"] + + for (const candidate of baseBranchCandidates) { + try { + await execAsync(`git rev-parse --verify ${candidate}`, { cwd }) + return candidate + } catch { + // Branch doesn't exist, try next + } + } + + // Last resort: return "main" + return "main" + } catch (error) { + console.error("Error detecting base branch:", error) + return "main" + } +} + +/** + * Checks if there are uncommitted changes (staged or unstaged) + * @param cwd The working directory + * @returns True if there are uncommitted changes + */ +export async function hasUncommittedChanges(cwd: string): Promise { + try { + const isInstalled = await checkGitInstalled() + if (!isInstalled) { + return false + } + + const isRepo = await checkGitRepo(cwd) + if (!isRepo) { + return false + } + + // Check for any changes (staged, unstaged, or untracked) + const { stdout } = await execAsync("git status --porcelain", { cwd }) + return stdout.trim().length > 0 + } catch (error) { + console.error("Error checking uncommitted changes:", error) + return false + } +} + +/** + * Gets the uncommitted diff (staged + unstaged changes) + * @param cwd The working directory + * @returns The diff output or empty string if no changes + */ +export async function getUncommittedDiff(cwd: string): Promise { + try { + const isInstalled = await checkGitInstalled() + if (!isInstalled) { + return "" + } + + const isRepo = await checkGitRepo(cwd) + if (!isRepo) { + return "" + } + + // Get diff of all changes compared to HEAD (includes staged and unstaged) + const { stdout } = await execAsync("git diff HEAD", { cwd, maxBuffer: 10 * 1024 * 1024 }) + return stdout + } catch (error) { + console.error("Error getting uncommitted diff:", error) + return "" + } +} + +/** + * Gets list of uncommitted files with their status + * @param cwd The working directory + * @returns Array of file changes with status + */ +export async function getUncommittedFiles(cwd: string): Promise { + try { + const isInstalled = await checkGitInstalled() + if (!isInstalled) { + return [] + } + + const isRepo = await checkGitRepo(cwd) + if (!isRepo) { + return [] + } + + // Get porcelain status for parsing + const { stdout } = await execAsync("git status --porcelain", { cwd }) + + if (!stdout.trim()) { + return [] + } + + const files: GitFileChange[] = [] + const lines = stdout.trim().split("\n") + + for (const line of lines) { + if (!line || line.length < 3) { + continue + } + + // Porcelain format: XY filename + // X = index status, Y = worktree status + const statusCode = line.substring(0, 2).trim() + let filePath = line.substring(3) + + // Handle renamed files (R oldpath -> newpath) + let oldPath: string | undefined + if (statusCode.startsWith("R") && filePath.includes(" -> ")) { + const parts = filePath.split(" -> ") + oldPath = parts[0] + filePath = parts[1] + } + + // Determine the primary status + let status = statusCode[0] !== " " && statusCode[0] !== "?" ? statusCode[0] : statusCode[1] + if (statusCode === "??") { + status = "?" + } + + files.push({ + path: filePath, + status, + ...(oldPath && { oldPath }), + }) + } + + return files + } catch (error) { + console.error("Error getting uncommitted files:", error) + return [] + } +} + +/** + * Resolves a branch name to an existing ref (local or remote) + * @param cwd The working directory + * @param branchName The branch name to resolve + * @returns The resolved ref or null if not found + */ +async function resolveBranchRef(cwd: string, branchName: string): Promise { + const refsToTry = [branchName, `origin/${branchName}`] + + for (const ref of refsToTry) { + try { + await execAsync(`git rev-parse --verify ${ref}`, { cwd }) + return ref + } catch { + // Ref not found, try next + } + } + + return null +} + +/** + * Gets the diff between current branch and base branch + * @param cwd The working directory + * @param baseBranch The base branch to compare against + * @returns The diff output or empty string if no changes + */ +export async function getBranchDiff(cwd: string, baseBranch: string): Promise { + try { + const isInstalled = await checkGitInstalled() + if (!isInstalled) { + return "" + } + + const isRepo = await checkGitRepo(cwd) + if (!isRepo) { + return "" + } + + // Resolve branch to local or remote ref + const resolvedRef = await resolveBranchRef(cwd, baseBranch) + if (!resolvedRef) { + console.error(`Could not resolve branch ref: ${baseBranch}`) + return "" + } + + // Get diff between base branch and current working directory + // This includes both committed and uncommitted changes + const { stdout } = await execAsync(`git diff ${resolvedRef}`, { cwd, maxBuffer: 10 * 1024 * 1024 }) + return stdout + } catch (error) { + console.error("Error getting branch diff:", error) + return "" + } +} + +/** + * Gets list of files changed between current branch and base branch + * @param cwd The working directory + * @param baseBranch The base branch to compare against + * @returns Array of file changes with status + */ +export async function getBranchFilesChanged(cwd: string, baseBranch: string): Promise { + try { + const isInstalled = await checkGitInstalled() + if (!isInstalled) { + return [] + } + + const isRepo = await checkGitRepo(cwd) + if (!isRepo) { + return [] + } + + // Resolve branch to local or remote ref + const resolvedRef = await resolveBranchRef(cwd, baseBranch) + if (!resolvedRef) { + console.error(`Could not resolve branch ref: ${baseBranch}`) + return [] + } + + // Get file list with status using name-status format + // This includes both committed and uncommitted changes vs base branch + const { stdout } = await execAsync(`git diff --name-status ${resolvedRef}`, { cwd }) + + if (!stdout.trim()) { + return [] + } + + const files: GitFileChange[] = [] + const lines = stdout.trim().split("\n") + + for (const line of lines) { + if (!line) { + continue + } + + // Format: STATUS\tfilename (or STATUS\toldname\tnewname for renames) + const parts = line.split("\t") + if (parts.length < 2) { + continue + } + + const status = parts[0][0] // First character is the status + let filePath = parts[1] + let oldPath: string | undefined + + // Handle renames (R100\toldpath\tnewpath) + if (status === "R" && parts.length >= 3) { + oldPath = parts[1] + filePath = parts[2] + } + + files.push({ + path: filePath, + status, + ...(oldPath && { oldPath }), + }) + } + + return files + } catch (error) { + console.error("Error getting branch files changed:", error) + return [] + } +} + +/** + * Checks if the current branch is a base branch (main/master/develop) + * @param cwd The working directory + * @returns True if current branch is a base branch + */ +export async function isOnBaseBranch(cwd: string): Promise { + try { + const currentBranch = await getCurrentBranch(cwd) + if (!currentBranch) { + return false + } + + const baseBranches = ["main", "master", "develop", "development"] + return baseBranches.includes(currentBranch) + } catch (error) { + console.error("Error checking if on base branch:", error) + return false + } +} + +/** + * Gets the number of commits between current branch and base branch + * @param cwd The working directory + * @param baseBranch The base branch to compare against + * @returns Number of commits ahead of base branch + */ +export async function getCommitCountFromBase(cwd: string, baseBranch: string): Promise { + try { + const isInstalled = await checkGitInstalled() + if (!isInstalled) { + return 0 + } + + const isRepo = await checkGitRepo(cwd) + if (!isRepo) { + return 0 + } + + // Resolve branch to local or remote ref + const resolvedRef = await resolveBranchRef(cwd, baseBranch) + if (!resolvedRef) { + return 0 + } + + const { stdout } = await execAsync(`git rev-list --count ${resolvedRef}..HEAD`, { cwd }) + return parseInt(stdout.trim(), 10) || 0 + } catch (error) { + console.error("Error getting commit count from base:", error) + return 0 + } +} + +// kilocode_change end diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index 839825e1b6c..1c24f6e7ed6 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -52,6 +52,7 @@ import { CheckpointWarning } from "./CheckpointWarning" import { IdeaSuggestionsBox } from "../kilocode/chat/IdeaSuggestionsBox" // kilocode_change import { KilocodeNotifications } from "../kilocode/KilocodeNotifications" // kilocode_change import { QueuedMessages } from "./QueuedMessages" +import { ReviewScopeSelector, type ReviewScopeInfo } from "./ReviewScopeSelector" // kilocode_change: Review mode import { buildDocLink } from "@/utils/docLinks" // import DismissibleUpsell from "../common/DismissibleUpsell" // kilocode_change: unused // import { useCloudUpsell } from "@src/hooks/useCloudUpsell" // kilocode_change: unused @@ -207,6 +208,11 @@ const ChatViewComponent: React.ForwardRefRenderFunction >(new Map()) + // kilocode_change start: Review mode state + const [showReviewScopeSelector, setShowReviewScopeSelector] = useState(false) + const [reviewScopeInfo, setReviewScopeInfo] = useState(null) + // kilocode_change end: Review mode state + const clineAskRef = useRef(clineAsk) useEffect(() => { clineAskRef.current = clineAsk @@ -963,6 +969,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction {/* kilocode_change: disable */} {/* */} + + {/* kilocode_change: Review mode scope selector */} + ) } diff --git a/webview-ui/src/components/chat/ReviewScopeSelector.tsx b/webview-ui/src/components/chat/ReviewScopeSelector.tsx new file mode 100644 index 00000000000..343edcd5906 --- /dev/null +++ b/webview-ui/src/components/chat/ReviewScopeSelector.tsx @@ -0,0 +1,220 @@ +// kilocode_change - new file +import { useState, useMemo } from "react" +import { useTranslation } from "react-i18next" +import { GitBranch, FileText, AlertCircle } from "lucide-react" +import { + Dialog, + DialogContent, + DialogHeader, + DialogTitle, + DialogDescription, + DialogFooter, + Button, +} from "@/components/ui" +import { vscode } from "@src/utils/vscode" + +export type ReviewScope = "uncommitted" | "branch" + +export interface ReviewScopeInfo { + uncommitted: { + available: boolean + fileCount: number + filePreview?: string[] + } + branch: { + available: boolean + currentBranch: string + baseBranch: string + fileCount: number + filePreview?: string[] + } + error?: string +} + +interface ReviewScopeSelectorProps { + open: boolean + onOpenChange: (open: boolean) => void + scopeInfo: ReviewScopeInfo | null +} + +export function ReviewScopeSelector({ open, onOpenChange, scopeInfo }: ReviewScopeSelectorProps) { + const { t } = useTranslation() + const [selectedScope, setSelectedScope] = useState("uncommitted") + + const handleStartReview = () => { + vscode.postMessage({ + type: "reviewScopeSelected", + reviewScope: selectedScope, + }) + onOpenChange(false) + } + + const handleCancel = () => { + onOpenChange(false) + } + + // Determine if each option is available + const uncommittedAvailable = scopeInfo?.uncommitted.available ?? false + const branchAvailable = scopeInfo?.branch.available ?? false + + // Auto-select the first available option if current selection is unavailable + const effectiveScope = useMemo((): ReviewScope => { + if (selectedScope === "uncommitted" && !uncommittedAvailable && branchAvailable) { + return "branch" + } + if (selectedScope === "branch" && !branchAvailable && uncommittedAvailable) { + return "uncommitted" + } + return selectedScope + }, [selectedScope, uncommittedAvailable, branchAvailable]) + + // Check if there's anything to review + const hasAnythingToReview = uncommittedAvailable || branchAvailable + + return ( + + + + {t("review:scopeSelector.title", "Select Review Scope")} + + {t("review:scopeSelector.description", "Choose what you want to review")} + + + + {scopeInfo?.error ? ( +
+ + {scopeInfo.error} +
+ ) : !hasAnythingToReview ? ( +
+ + + {t("review:scopeSelector.nothingToReview", "No changes found to review")} + +
+ ) : ( +
+ {/* Uncommitted Changes Option */} + + + {/* Branch Diff Option */} + +
+ )} + + + + + + + + ) +} diff --git a/webview-ui/src/components/chat/__tests__/ReviewScopeSelector.spec.tsx b/webview-ui/src/components/chat/__tests__/ReviewScopeSelector.spec.tsx new file mode 100644 index 00000000000..8a5f6e3169e --- /dev/null +++ b/webview-ui/src/components/chat/__tests__/ReviewScopeSelector.spec.tsx @@ -0,0 +1,260 @@ +// npx vitest run src/components/chat/__tests__/ReviewScopeSelector.spec.tsx + +import { render, screen, fireEvent } from "@/utils/test-utils" +import { vi } from "vitest" +import { ReviewScopeSelector, type ReviewScopeInfo } from "../ReviewScopeSelector" + +// Mock vscode postMessage +const mockPostMessage = vi.fn() +vi.mock("@src/utils/vscode", () => ({ + vscode: { + postMessage: (message: unknown) => mockPostMessage(message), + }, +})) + +// Mock react-i18next +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (key: string, defaultValue?: string | Record, options?: Record) => { + // Handle i18next interpolation + if (typeof defaultValue === "object" && defaultValue !== null && !("count" in defaultValue)) { + options = defaultValue as Record + defaultValue = key + } + let result = defaultValue || key + if (typeof result === "string" && options) { + Object.entries(options).forEach(([k, v]) => { + result = (result as string).replace(`{{${k}}}`, String(v)) + }) + } + return result + }, + }), +})) + +describe("ReviewScopeSelector", () => { + const baseScopeInfo: ReviewScopeInfo = { + uncommitted: { + available: true, + fileCount: 3, + filePreview: ["src/file1.ts", "src/file2.ts", "src/file3.ts"], + }, + branch: { + available: true, + currentBranch: "feature/test", + baseBranch: "main", + fileCount: 5, + filePreview: ["src/a.ts", "src/b.ts", "src/c.ts", "src/d.ts", "src/e.ts"], + }, + } + + const defaultProps = { + open: true, + onOpenChange: vi.fn(), + scopeInfo: baseScopeInfo, + } + + beforeEach(() => { + vi.clearAllMocks() + }) + + describe("Basic Rendering", () => { + it("renders dialog when open", () => { + render() + + expect(screen.getByText("Select Review Scope")).toBeInTheDocument() + expect(screen.getByText("Uncommitted changes")).toBeInTheDocument() + expect(screen.getByText("Branch diff")).toBeInTheDocument() + }) + + it("does not render when open is false", () => { + render() + + expect(screen.queryByText("Select Review Scope")).not.toBeInTheDocument() + }) + + it("shows file counts for both options", () => { + render() + + expect(screen.getByText("3 file(s) changed")).toBeInTheDocument() + expect(screen.getByText("feature/test vs main (5 file(s))")).toBeInTheDocument() + }) + + it("shows file preview for uncommitted changes", () => { + render() + + expect(screen.getByText("src/file1.ts")).toBeInTheDocument() + expect(screen.getByText("src/file2.ts")).toBeInTheDocument() + expect(screen.getByText("src/file3.ts")).toBeInTheDocument() + }) + + it("shows file preview for branch diff with more indicator", () => { + render() + + expect(screen.getByText("src/a.ts")).toBeInTheDocument() + expect(screen.getByText("src/b.ts")).toBeInTheDocument() + expect(screen.getByText("src/c.ts")).toBeInTheDocument() + expect(screen.getByText("+2 more")).toBeInTheDocument() + }) + }) + + describe("Option Availability", () => { + it("disables uncommitted option when not available", () => { + const scopeInfo: ReviewScopeInfo = { + ...baseScopeInfo, + uncommitted: { available: false, fileCount: 0 }, + } + render() + + const uncommittedRadio = screen.getByRole("radio", { name: /uncommitted/i }) + expect(uncommittedRadio).toBeDisabled() + expect(screen.getByText("No uncommitted changes")).toBeInTheDocument() + }) + + it("disables branch option when not available", () => { + const scopeInfo: ReviewScopeInfo = { + ...baseScopeInfo, + branch: { + available: false, + currentBranch: "main", + baseBranch: "main", + fileCount: 0, + }, + } + render() + + const branchRadio = screen.getByRole("radio", { name: /branch/i }) + expect(branchRadio).toBeDisabled() + expect(screen.getByText("Already on base branch or no changes")).toBeInTheDocument() + }) + + it("shows error when provided", () => { + const scopeInfo: ReviewScopeInfo = { + ...baseScopeInfo, + error: "Failed to detect git repository", + } + render() + + expect(screen.getByText("Failed to detect git repository")).toBeInTheDocument() + }) + + it("shows nothing to review message when both options unavailable", () => { + const scopeInfo: ReviewScopeInfo = { + uncommitted: { available: false, fileCount: 0 }, + branch: { + available: false, + currentBranch: "main", + baseBranch: "main", + fileCount: 0, + }, + } + render() + + expect(screen.getByText("No changes found to review")).toBeInTheDocument() + }) + }) + + describe("User Interactions", () => { + it("selects uncommitted by default", () => { + render() + + const uncommittedRadio = screen.getByRole("radio", { name: /uncommitted/i }) + expect(uncommittedRadio).toBeChecked() + }) + + it("allows selecting branch diff", () => { + render() + + const branchRadio = screen.getByRole("radio", { name: /branch/i }) + fireEvent.click(branchRadio) + + expect(branchRadio).toBeChecked() + }) + + it("sends correct message when starting uncommitted review", () => { + render() + + fireEvent.click(screen.getByText("Start Review")) + + expect(mockPostMessage).toHaveBeenCalledWith({ + type: "reviewScopeSelected", + reviewScope: "uncommitted", + }) + }) + + it("sends correct message when starting branch review", () => { + render() + + const branchRadio = screen.getByRole("radio", { name: /branch/i }) + fireEvent.click(branchRadio) + fireEvent.click(screen.getByText("Start Review")) + + expect(mockPostMessage).toHaveBeenCalledWith({ + type: "reviewScopeSelected", + reviewScope: "branch", + }) + }) + + it("closes dialog when cancel is clicked", () => { + const onOpenChange = vi.fn() + render() + + fireEvent.click(screen.getByText("Cancel")) + + expect(onOpenChange).toHaveBeenCalledWith(false) + }) + + it("closes dialog after starting review", () => { + const onOpenChange = vi.fn() + render() + + fireEvent.click(screen.getByText("Start Review")) + + expect(onOpenChange).toHaveBeenCalledWith(false) + }) + + it("disables start review button when nothing to review", () => { + const scopeInfo: ReviewScopeInfo = { + uncommitted: { available: false, fileCount: 0 }, + branch: { + available: false, + currentBranch: "main", + baseBranch: "main", + fileCount: 0, + }, + } + render() + + const startButton = screen.getByText("Start Review") + expect(startButton).toBeDisabled() + }) + }) + + describe("Auto-selection", () => { + it("auto-selects branch when uncommitted is not available", () => { + const scopeInfo: ReviewScopeInfo = { + uncommitted: { available: false, fileCount: 0 }, + branch: { + available: true, + currentBranch: "feature/test", + baseBranch: "main", + fileCount: 5, + }, + } + render() + + const branchRadio = screen.getByRole("radio", { name: /branch/i }) + expect(branchRadio).toBeChecked() + }) + }) + + describe("Null scopeInfo handling", () => { + it("handles null scopeInfo gracefully", () => { + render() + + expect(screen.getByText("Select Review Scope")).toBeInTheDocument() + // Should show nothing to review since both are unavailable + expect(screen.getByText("No changes found to review")).toBeInTheDocument() + }) + }) +})