From 812a83507e01774bac13c0a793df41edd39484e3 Mon Sep 17 00:00:00 2001 From: nrslib <38722970+nrslib@users.noreply.github.com> Date: Wed, 28 Jan 2026 15:20:34 +0900 Subject: [PATCH] =?UTF-8?q?review=E3=82=B3=E3=83=9E=E3=83=B3=E3=83=89?= =?UTF-8?q?=E3=81=AE=E4=B8=8D=E8=A6=81=E3=81=AA=E5=87=A6=E7=90=86=E3=82=92?= =?UTF-8?q?=E5=89=8A=E9=99=A4=E3=80=81=E4=B8=8A=E4=B8=8B=E3=83=90=E3=82=B0?= =?UTF-8?q?=E4=BF=AE=E6=AD=A3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/__tests__/prompt.test.ts | 102 +++++++++++++++++++++++++++++- src/__tests__/reviewTasks.test.ts | 23 +++++++ src/cli.ts | 6 +- src/commands/help.ts | 8 +-- src/commands/reviewTasks.ts | 87 +++++++++++++++++++------ src/prompt/index.ts | 42 +++++++++--- src/utils/index.ts | 1 + src/utils/text.ts | 56 ++++++++++++++++ 8 files changed, 291 insertions(+), 34 deletions(-) create mode 100644 src/utils/text.ts diff --git a/src/__tests__/prompt.test.ts b/src/__tests__/prompt.test.ts index 6095dbe..1fc65cf 100644 --- a/src/__tests__/prompt.test.ts +++ b/src/__tests__/prompt.test.ts @@ -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) diff --git a/src/__tests__/reviewTasks.test.ts b/src/__tests__/reviewTasks.test.ts index 0b42df0..81745c0 100644 --- a/src/__tests__/reviewTasks.test.ts +++ b/src/__tests__/reviewTasks.test.ts @@ -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); + }); +}); diff --git a/src/cli.ts b/src/cli.ts index 958255f..6062d38 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -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); } } diff --git a/src/commands/help.ts b/src/commands/help.ts index e1d20e1..abdf5f2 100644 --- a/src/commands/help.ts +++ b/src/commands/help.ts @@ -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 /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 /run-tasks (/run) Run all pending tasks from .takt/tasks/ + takt /watch Watch for tasks and auto-execute (stays resident) + 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 diff --git a/src/commands/reviewTasks.ts b/src/commands/reviewTasks.ts index e14e2ab..9a80625 100644 --- a/src/commands/reviewTasks.ts +++ b/src/commands/reviewTasks.ts @@ -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 { +): Promise { console.log(); console.log(chalk.bold.cyan(`=== ${item.info.branch} ===`)); console.log(); @@ -50,33 +66,65 @@ async function showDiffAndPromptAction( const action = await selectOption( `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 { 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 { } break; } - case 'skip': - break; } // Refresh worktree list after action diff --git a/src/prompt/index.ts b/src/prompt/index.ts index 14df192..5b487ec 100644 --- a/src/prompt/index.ts +++ b/src/prompt/index.ts @@ -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 { @@ -19,6 +20,7 @@ export interface SelectOptionItem { /** * 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( @@ -26,21 +28,32 @@ export function renderMenu( 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( options: SelectOptionItem[], @@ -153,8 +168,8 @@ function redrawMenu( 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( 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( 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': diff --git a/src/utils/index.ts b/src/utils/index.ts index fc41310..a63b353 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -5,3 +5,4 @@ export * from './ui.js'; export * from './session.js'; export * from './debug.js'; +export * from './text.js'; diff --git a/src/utils/text.ts b/src/utils/text.ts new file mode 100644 index 0000000..e0e1bba --- /dev/null +++ b/src/utils/text.ts @@ -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; +}