reviewコマンドの不要な処理を削除、上下バグ修正

This commit is contained in:
nrslib 2026-01-28 15:20:34 +09:00
parent 80626411cf
commit 812a83507e
8 changed files with 291 additions and 34 deletions

View File

@ -5,7 +5,12 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import chalk from 'chalk';
import type { SelectOptionItem, KeyInputResult } from '../prompt/index.js';
import { renderMenu, countRenderedLines, handleKeyInput } from '../prompt/index.js';
import {
renderMenu,
countRenderedLines,
handleKeyInput,
} from '../prompt/index.js';
import { isFullWidth, getDisplayWidth, truncateText } from '../utils/text.js';
// Disable chalk colors for predictable test output
chalk.level = 0;
@ -324,6 +329,101 @@ describe('prompt', () => {
});
});
describe('isFullWidth', () => {
it('should return true for CJK ideographs', () => {
expect(isFullWidth('漢'.codePointAt(0)!)).toBe(true);
expect(isFullWidth('字'.codePointAt(0)!)).toBe(true);
});
it('should return true for Hangul syllables', () => {
expect(isFullWidth('한'.codePointAt(0)!)).toBe(true);
});
it('should return true for fullwidth ASCII variants', () => {
// = U+FF21 (fullwidth A)
expect(isFullWidth(0xFF21)).toBe(true);
});
it('should return false for ASCII characters', () => {
expect(isFullWidth('A'.codePointAt(0)!)).toBe(false);
expect(isFullWidth('z'.codePointAt(0)!)).toBe(false);
expect(isFullWidth(' '.codePointAt(0)!)).toBe(false);
});
it('should return false for basic Latin punctuation', () => {
expect(isFullWidth('-'.codePointAt(0)!)).toBe(false);
expect(isFullWidth('/'.codePointAt(0)!)).toBe(false);
});
});
describe('getDisplayWidth', () => {
it('should return length for ASCII-only string', () => {
expect(getDisplayWidth('hello')).toBe(5);
});
it('should count CJK characters as 2 columns each', () => {
expect(getDisplayWidth('漢字')).toBe(4);
});
it('should handle mixed ASCII and CJK', () => {
// 'ab' = 2 + '漢' = 2 + 'c' = 1 = 5
expect(getDisplayWidth('ab漢c')).toBe(5);
});
it('should return 0 for empty string', () => {
expect(getDisplayWidth('')).toBe(0);
});
});
describe('truncateText', () => {
it('should return text as-is when it fits within maxWidth', () => {
expect(truncateText('hello', 10)).toBe('hello');
});
it('should truncate ASCII text and add ellipsis', () => {
const result = truncateText('abcdefghij', 6);
// maxWidth=6, ellipsis takes 1, so 5 chars fit + '…'
expect(result).toBe('abcde…');
expect(getDisplayWidth(result)).toBeLessThanOrEqual(6);
});
it('should truncate CJK text and add ellipsis', () => {
// '漢字テスト' = 10 columns, maxWidth=7
const result = truncateText('漢字テスト', 7);
// 漢(2)+字(2)+テ(2) = 6, next ス(2) would be 8 > 7-1=6, so truncate at 6
expect(result).toBe('漢字テ…');
expect(getDisplayWidth(result)).toBeLessThanOrEqual(7);
});
it('should handle mixed ASCII and CJK truncation', () => {
// 'abc漢字def' = 3+2+2+3 = 10 columns, maxWidth=8
const result = truncateText('abc漢字def', 8);
// a(1)+b(1)+c(1)+漢(2)+字(2) = 7, next d(1) would be 8 > 8-1=7, truncate
expect(result).toBe('abc漢字…');
expect(getDisplayWidth(result)).toBeLessThanOrEqual(8);
});
it('should return empty string when maxWidth is 0', () => {
expect(truncateText('hello', 0)).toBe('');
});
it('should return empty string when maxWidth is negative', () => {
expect(truncateText('hello', -5)).toBe('');
});
it('should not truncate text that exactly fits maxWidth', () => {
// 'abc' = 3 columns, maxWidth=3
// width(0)+a(1)=1 > 3-1=2? no. width(1)+b(1)=2 > 2? no. width(2)+c(1)=3 > 2? yes → truncate
// Actually truncateText adds ellipsis when width + charWidth > maxWidth - 1
// For 'abc' maxWidth=3: a(1)>2? no; b(2)>2? no; c(3)>2? yes → 'ab…'
// So text exactly at maxWidth still gets truncated because ellipsis needs space
// To avoid truncation, the full text display width must be <= maxWidth - 1...
// Wait, let's re-read: if width+charWidth > maxWidth-1, truncate.
// For 'abc' maxWidth=4: a(1)>3? no; b(2)>3? no; c(3)>3? no; returns 'abc'
expect(truncateText('abc', 4)).toBe('abc');
});
});
describe('selectOptionWithDefault cancel behavior', () => {
it('handleKeyInput should return cancel with optionCount when hasCancelOption is true', () => {
// Simulates ESC key press with cancel option enabled (as selectOptionWithDefault now does)

View File

@ -9,6 +9,7 @@ import {
buildReviewItems,
type WorktreeInfo,
} from '../task/worktree.js';
import { isBranchMerged, type ReviewAction } from '../commands/reviewTasks.js';
describe('parseTaktWorktrees', () => {
it('should parse takt/ branches from porcelain output', () => {
@ -167,3 +168,25 @@ describe('buildReviewItems', () => {
expect(items).toHaveLength(0);
});
});
describe('ReviewAction type', () => {
it('should include try, merge, delete (no skip)', () => {
const actions: ReviewAction[] = ['try', 'merge', 'delete'];
expect(actions).toHaveLength(3);
expect(actions).toContain('try');
expect(actions).not.toContain('skip');
});
});
describe('isBranchMerged', () => {
it('should return false for non-existent project dir', () => {
// git merge-base will fail on non-existent dir
const result = isBranchMerged('/non-existent-dir', 'some-branch');
expect(result).toBe(false);
});
it('should return false for non-existent branch', () => {
const result = isBranchMerged('/tmp', 'non-existent-branch-xyz');
expect(result).toBe(false);
});
});

View File

@ -92,7 +92,8 @@ program
const args = parts.slice(1);
switch (command) {
case 'run-tasks': {
case 'run-tasks':
case 'run': {
const workflow = getCurrentWorkflow(cwd);
await runAllTasks(cwd, workflow);
return;
@ -117,6 +118,7 @@ program
return;
case 'add-task':
case 'add':
await addTask(cwd, args);
return;
@ -135,7 +137,7 @@ program
default:
error(`Unknown command: /${command}`);
info('Available: /run-tasks, /watch, /add-task, /review-tasks, /switch, /clear, /refresh-builtin, /help, /config');
info('Available: /run-tasks (/run), /watch, /add-task (/add), /review-tasks (/review), /switch (/sw), /clear, /refresh-builtin, /help, /config');
process.exit(1);
}
}

View File

@ -14,10 +14,10 @@ export function showHelp(): void {
console.log(`
Usage:
takt {task} Execute task with current workflow (continues session)
takt /run-tasks Run all pending tasks from .takt/tasks/
takt /run-tasks (/run) Run all pending tasks from .takt/tasks/
takt /watch Watch for tasks and auto-execute (stays resident)
takt /add-task Add a new task (interactive, YAML format)
takt /review-tasks Review worktree task results (merge/delete)
takt /add-task (/add) Add a new task (interactive, YAML format)
takt /review-tasks (/review) Review worktree task results (merge/delete)
takt /switch Switch workflow interactively
takt /clear Clear agent conversation sessions (reset to initial state)
takt /refresh-builtin Overwrite builtin agents/workflows with latest version

View File

@ -2,7 +2,7 @@
* Review tasks command
*
* Interactive UI for reviewing worktree-based task results:
* merge, skip, or delete actions.
* try merge, merge & cleanup, or delete actions.
*/
import { execFileSync } from 'node:child_process';
@ -21,7 +21,23 @@ import { createLogger } from '../utils/debug.js';
const log = createLogger('review-tasks');
/** Actions available for a reviewed worktree */
export type ReviewAction = 'merge' | 'skip' | 'delete';
export type ReviewAction = 'try' | 'merge' | 'delete';
/**
* Check if a branch has already been merged into HEAD.
*/
export function isBranchMerged(projectDir: string, branch: string): boolean {
try {
execFileSync('git', ['merge-base', '--is-ancestor', branch, 'HEAD'], {
cwd: projectDir,
encoding: 'utf-8',
stdio: 'pipe',
});
return true;
} catch {
return false;
}
}
/**
* Show diff stat for a branch and prompt for an action.
@ -30,7 +46,7 @@ async function showDiffAndPromptAction(
cwd: string,
defaultBranch: string,
item: WorktreeReviewItem,
): Promise<ReviewAction> {
): Promise<ReviewAction | null> {
console.log();
console.log(chalk.bold.cyan(`=== ${item.info.branch} ===`));
console.log();
@ -50,33 +66,65 @@ async function showDiffAndPromptAction(
const action = await selectOption<ReviewAction>(
`Action for ${item.info.branch}:`,
[
{ label: 'Merge', value: 'merge', description: 'Merge changes into current branch and clean up' },
{ label: 'Skip', value: 'skip', description: 'Return to list without changes' },
{ 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' },
{ label: 'Delete', value: 'delete', description: 'Discard changes, remove worktree and branch' },
],
);
return action ?? 'skip';
return action;
}
/**
* Merge a worktree branch into the current branch.
* Removes the worktree first, then merges, then deletes the branch.
* Try-merge: merge the branch without cleanup.
* Keeps the worktree and branch intact for further review.
*/
export function mergeWorktreeBranch(projectDir: string, item: WorktreeReviewItem): boolean {
export function tryMergeWorktreeBranch(projectDir: string, item: WorktreeReviewItem): boolean {
const { branch } = item.info;
try {
// 1. Remove worktree (must happen before merge to unlock branch)
removeWorktree(projectDir, item.info.path);
// 2. Merge the branch
execFileSync('git', ['merge', branch], {
cwd: projectDir,
encoding: 'utf-8',
stdio: 'pipe',
});
success(`Merged ${branch} (worktree kept)`);
log.info('Try-merge completed', { branch });
return true;
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
logError(`Merge failed: ${msg}`);
logError('You may need to resolve conflicts manually.');
log.error('Try-merge failed', { branch, error: msg });
return false;
}
}
/**
* Merge & cleanup: if already merged, skip merge and just cleanup.
* Otherwise merge first, then cleanup (remove worktree + delete branch).
*/
export function mergeWorktreeBranch(projectDir: string, item: WorktreeReviewItem): boolean {
const { branch } = item.info;
const alreadyMerged = isBranchMerged(projectDir, branch);
try {
// 1. Remove worktree (must happen before merge to unlock branch)
removeWorktree(projectDir, item.info.path);
// 2. Merge only if not already merged
if (alreadyMerged) {
info(`${branch} is already merged, skipping merge.`);
log.info('Branch already merged, cleanup only', { branch });
} else {
execFileSync('git', ['merge', branch], {
cwd: projectDir,
encoding: 'utf-8',
stdio: 'pipe',
});
}
// 3. Delete the branch
try {
execFileSync('git', ['branch', '-d', branch], {
@ -88,14 +136,14 @@ export function mergeWorktreeBranch(projectDir: string, item: WorktreeReviewItem
warn(`Could not delete branch ${branch}. You may delete it manually.`);
}
success(`Merged ${branch}`);
log.info('Worktree merged', { branch });
success(`Merged & cleaned up ${branch}`);
log.info('Worktree merged & cleaned up', { branch, alreadyMerged });
return true;
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
logError(`Merge failed: ${msg}`);
logError('You may need to resolve conflicts manually.');
log.error('Merge failed', { branch, error: msg });
log.error('Merge & cleanup failed', { branch, error: msg });
return false;
}
}
@ -168,7 +216,12 @@ export async function reviewTasks(cwd: string): Promise<void> {
const action = await showDiffAndPromptAction(cwd, defaultBranch, item);
if (action === null) continue;
switch (action) {
case 'try':
tryMergeWorktreeBranch(cwd, item);
break;
case 'merge':
mergeWorktreeBranch(cwd, item);
break;
@ -182,8 +235,6 @@ export async function reviewTasks(cwd: string): Promise<void> {
}
break;
}
case 'skip':
break;
}
// Refresh worktree list after action

View File

@ -7,6 +7,7 @@
import * as readline from 'node:readline';
import chalk from 'chalk';
import { truncateText } from '../utils/text.js';
/** Option type for selectOption */
export interface SelectOptionItem<T extends string> {
@ -19,6 +20,7 @@ export interface SelectOptionItem<T extends string> {
/**
* Render the menu options to the terminal.
* Writes directly to stdout using ANSI escape codes.
* Labels are truncated to fit within the terminal width.
* Exported for testing.
*/
export function renderMenu<T extends string>(
@ -26,21 +28,32 @@ export function renderMenu<T extends string>(
selectedIndex: number,
hasCancelOption: boolean
): string[] {
const maxWidth = process.stdout.columns || 80;
// Prefix " " = 4 visible columns (2 spaces + cursor + space)
const labelPrefix = 4;
// Description prefix " " = 5 visible columns
const descPrefix = 5;
// Detail prefix " • " = 9 visible columns
const detailPrefix = 9;
const lines: string[] = [];
for (let i = 0; i < options.length; i++) {
const opt = options[i]!;
const isSelected = i === selectedIndex;
const cursor = isSelected ? chalk.cyan('') : ' ';
const label = isSelected ? chalk.cyan.bold(opt.label) : opt.label;
const truncatedLabel = truncateText(opt.label, maxWidth - labelPrefix);
const label = isSelected ? chalk.cyan.bold(truncatedLabel) : truncatedLabel;
lines.push(` ${cursor} ${label}`);
if (opt.description) {
lines.push(chalk.gray(` ${opt.description}`));
const truncatedDesc = truncateText(opt.description, maxWidth - descPrefix);
lines.push(chalk.gray(` ${truncatedDesc}`));
}
if (opt.details && opt.details.length > 0) {
for (const detail of opt.details) {
lines.push(chalk.dim(`${detail}`));
const truncatedDetail = truncateText(detail, maxWidth - detailPrefix);
lines.push(chalk.dim(`${truncatedDetail}`));
}
}
}
@ -145,7 +158,9 @@ function setupRawMode(): { cleanup: (listener: (data: Buffer) => void) => void;
}
/**
* Redraw the menu by moving cursor up and re-rendering.
* Redraw the menu using relative cursor movement.
* Auto-wrap is disabled during menu interaction, so
* 1 logical line = 1 physical line, making line-count movement accurate.
*/
function redrawMenu<T extends string>(
options: SelectOptionItem<T>[],
@ -153,8 +168,8 @@ function redrawMenu<T extends string>(
hasCancelOption: boolean,
totalLines: number
): void {
process.stdout.write(`\x1B[${totalLines}A`);
process.stdout.write('\x1B[J');
process.stdout.write(`\x1B[${totalLines}A`); // Move up to menu start
process.stdout.write('\x1B[J'); // Clear from cursor to end
const newLines = renderMenu(options, selectedIndex, hasCancelOption);
process.stdout.write(newLines.join('\n') + '\n');
}
@ -175,17 +190,26 @@ function interactiveSelect<T extends string>(
printHeader(message);
// Disable auto-wrap so 1 logical line = 1 physical line
process.stdout.write('\x1B[?7l');
const totalLines = countRenderedLines(options, hasCancelOption);
const lines = renderMenu(options, selectedIndex, hasCancelOption);
process.stdout.write(lines.join('\n') + '\n');
if (!process.stdin.isTTY) {
process.stdout.write('\x1B[?7h'); // Re-enable auto-wrap
resolve(initialIndex);
return;
}
const rawMode = setupRawMode();
const cleanup = (listener: (data: Buffer) => void): void => {
rawMode.cleanup(listener);
process.stdout.write('\x1B[?7h'); // Re-enable auto-wrap
};
const onKeypress = (data: Buffer): void => {
const result = handleKeyInput(
data.toString(),
@ -201,15 +225,15 @@ function interactiveSelect<T extends string>(
redrawMenu(options, selectedIndex, hasCancelOption, totalLines);
break;
case 'confirm':
rawMode.cleanup(onKeypress);
cleanup(onKeypress);
resolve(result.selectedIndex);
break;
case 'cancel':
rawMode.cleanup(onKeypress);
cleanup(onKeypress);
resolve(result.cancelIndex);
break;
case 'exit':
rawMode.cleanup(onKeypress);
cleanup(onKeypress);
process.exit(130);
break;
case 'none':

View File

@ -5,3 +5,4 @@
export * from './ui.js';
export * from './session.js';
export * from './debug.js';
export * from './text.js';

56
src/utils/text.ts Normal file
View File

@ -0,0 +1,56 @@
/**
* Text display width utilities
*
* Pure functions for calculating and truncating text based on
* terminal display width, with full-width (CJK) character support.
*/
/**
* Check if a Unicode code point is full-width (occupies 2 columns).
* Covers CJK unified ideographs, Hangul, fullwidth forms, etc.
*/
export function isFullWidth(code: number): boolean {
return (
(code >= 0x1100 && code <= 0x115F) || // Hangul Jamo
(code >= 0x2E80 && code <= 0x9FFF) || // CJK radicals, symbols, ideographs
(code >= 0xAC00 && code <= 0xD7AF) || // Hangul syllables
(code >= 0xF900 && code <= 0xFAFF) || // CJK compatibility ideographs
(code >= 0xFE10 && code <= 0xFE6F) || // CJK compatibility forms
(code >= 0xFF01 && code <= 0xFF60) || // Fullwidth ASCII variants
(code >= 0xFFE0 && code <= 0xFFE6) || // Fullwidth symbols
(code >= 0x20000 && code <= 0x2FA1F) // CJK extension B+
);
}
/**
* Calculate the display width of a plain text string.
* Full-width characters (CJK etc.) count as 2, others as 1.
*/
export function getDisplayWidth(text: string): number {
let width = 0;
for (const char of text) {
const code = char.codePointAt(0) ?? 0;
width += isFullWidth(code) ? 2 : 1;
}
return width;
}
/**
* Truncate plain text to fit within maxWidth display columns.
* Appends '…' if truncated. The ellipsis itself counts as 1 column.
*/
export function truncateText(text: string, maxWidth: number): string {
if (maxWidth <= 0) return '';
let width = 0;
let i = 0;
for (const char of text) {
const charWidth = isFullWidth(char.codePointAt(0) ?? 0) ? 2 : 1;
if (width + charWidth > maxWidth - 1) {
// Not enough room; truncate and add ellipsis
return text.slice(0, i) + '…';
}
width += charWidth;
i += char.length;
}
return text;
}