diff --git a/src/__tests__/reviewTasks.test.ts b/src/__tests__/reviewTasks.test.ts index 573a315..51d80e9 100644 --- a/src/__tests__/reviewTasks.test.ts +++ b/src/__tests__/reviewTasks.test.ts @@ -2,14 +2,14 @@ * Tests for review-tasks command */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { parseTaktWorktrees, extractTaskSlug, buildReviewItems, type WorktreeInfo, } from '../task/worktree.js'; -import { isBranchMerged, type ReviewAction } from '../commands/reviewTasks.js'; +import { isBranchMerged, showFullDiff, type ReviewAction } from '../commands/reviewTasks.js'; describe('parseTaktWorktrees', () => { it('should parse takt/ branches from porcelain output', () => { @@ -170,15 +170,36 @@ describe('buildReviewItems', () => { }); describe('ReviewAction type', () => { - it('should include instruct, try, merge, delete (no skip)', () => { - const actions: ReviewAction[] = ['instruct', 'try', 'merge', 'delete']; - expect(actions).toHaveLength(4); + it('should include diff, instruct, try, merge, delete (no skip)', () => { + const actions: ReviewAction[] = ['diff', 'instruct', 'try', 'merge', 'delete']; + expect(actions).toHaveLength(5); + expect(actions).toContain('diff'); expect(actions).toContain('instruct'); expect(actions).toContain('try'); + expect(actions).toContain('merge'); + expect(actions).toContain('delete'); expect(actions).not.toContain('skip'); }); }); +describe('showFullDiff', () => { + it('should not throw for non-existent project dir', () => { + // spawnSync will fail gracefully; showFullDiff catches errors + expect(() => showFullDiff('/non-existent-dir', 'main', 'some-branch')).not.toThrow(); + }); + + it('should not throw for non-existent branch', () => { + expect(() => showFullDiff('/tmp', 'main', 'non-existent-branch-xyz')).not.toThrow(); + }); + + it('should warn when diff fails', () => { + const warnSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + showFullDiff('/non-existent-dir', 'main', 'some-branch'); + warnSpy.mockRestore(); + // No assertion needed — the test verifies it doesn't throw + }); +}); + describe('isBranchMerged', () => { it('should return false for non-existent project dir', () => { // git merge-base will fail on non-existent dir diff --git a/src/commands/reviewTasks.ts b/src/commands/reviewTasks.ts index c3c37b9..f6cc9d0 100644 --- a/src/commands/reviewTasks.ts +++ b/src/commands/reviewTasks.ts @@ -5,7 +5,7 @@ * try merge, merge & cleanup, or delete actions. */ -import { execFileSync } from 'node:child_process'; +import { execFileSync, spawnSync } from 'node:child_process'; import chalk from 'chalk'; import { removeWorktree, @@ -26,7 +26,7 @@ import { DEFAULT_WORKFLOW_NAME } from '../constants.js'; const log = createLogger('review-tasks'); /** Actions available for a reviewed worktree */ -export type ReviewAction = 'try' | 'merge' | 'delete' | 'instruct'; +export type ReviewAction = 'diff' | 'instruct' | 'try' | 'merge' | 'delete'; /** * Check if a branch has already been merged into HEAD. @@ -44,6 +44,28 @@ export function isBranchMerged(projectDir: string, branch: string): boolean { } } +/** + * Show full diff in an interactive pager (less). + * Falls back to direct output if pager is unavailable. + */ +export function showFullDiff( + cwd: string, + defaultBranch: string, + branch: string, +): void { + try { + const result = spawnSync( + 'git', ['diff', '--color=always', `${defaultBranch}...${branch}`], + { cwd, stdio: ['inherit', 'inherit', 'inherit'], env: { ...process.env, GIT_PAGER: 'less -R' } }, + ); + if (result.status !== 0) { + warn('Could not display diff'); + } + } catch { + warn('Could not display diff'); + } +} + /** * Show diff stat for a branch and prompt for an action. */ @@ -71,6 +93,7 @@ async function showDiffAndPromptAction( const action = await selectOption( `Action for ${item.info.branch}:`, [ + { label: 'View diff', value: 'diff', description: 'Show full diff in pager' }, { label: 'Instruct', value: 'instruct', description: 'Give additional instructions to modify this worktree' }, { label: 'Try merge', value: 'try', description: 'Merge without cleanup (keep worktree & branch)' }, { label: 'Merge & cleanup', value: 'merge', description: 'Merge (if needed) and remove worktree & branch' }, @@ -294,7 +317,15 @@ export async function reviewTasks(cwd: string): Promise { const item = items[selectedIdx]; if (!item) continue; - const action = await showDiffAndPromptAction(cwd, defaultBranch, item); + // Action loop: re-show menu after viewing diff + let action: ReviewAction | null; + do { + action = await showDiffAndPromptAction(cwd, defaultBranch, item); + + if (action === 'diff') { + showFullDiff(cwd, defaultBranch, item.info.branch); + } + } while (action === 'diff'); if (action === null) continue;