From 205f66202234f1987450c3c6d68ca791a54bfc28 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 18 Feb 2026 22:11:31 -0800 Subject: [PATCH] fix: Improve error handling and validation across multiple services --- apps/server/src/providers/codex-provider.ts | 3 +- apps/server/src/routes/worktree/common.ts | 18 +- .../routes/worktree/routes/check-changes.ts | 6 +- .../routes/worktree/routes/checkout-branch.ts | 10 +- apps/server/src/services/auto-mode/facade.ts | 20 +-- apps/server/src/services/branch-utils.ts | 155 ++++++++++++++++++ .../src/services/checkout-branch-service.ts | 126 +++----------- apps/server/src/services/merge-service.ts | 19 ++- .../src/services/pipeline-orchestrator.ts | 3 +- apps/server/src/services/pull-service.ts | 19 ++- apps/server/src/services/rebase-service.ts | 8 +- .../src/services/worktree-branch-service.ts | 104 +----------- .../services/pipeline-orchestrator.test.ts | 13 +- apps/ui/src/components/ui/git-diff-panel.tsx | 138 +++++++++------- apps/ui/src/components/usage-popover.tsx | 36 ++-- .../dialogs/create-branch-dialog.tsx | 36 ++-- apps/ui/src/types/electron.d.ts | 20 +++ 17 files changed, 395 insertions(+), 339 deletions(-) create mode 100644 apps/server/src/services/branch-utils.ts diff --git a/apps/server/src/providers/codex-provider.ts b/apps/server/src/providers/codex-provider.ts index f23dd6c6..3c8979f6 100644 --- a/apps/server/src/providers/codex-provider.ts +++ b/apps/server/src/providers/codex-provider.ts @@ -204,6 +204,7 @@ function isSdkEligible(options: ExecuteOptions): boolean { function isSdkEligibleWithApiKey(options: ExecuteOptions): boolean { // When using an API key (not CLI OAuth), prefer SDK over CLI to avoid OAuth issues. // SDK mode is used when MCP servers are not configured (MCP requires CLI). + // Tool requests are handled by the SDK, so we allow SDK mode even with tools. return !hasMcpServersConfigured(options); } @@ -922,7 +923,7 @@ export class CodexProvider extends BaseProvider { `Try again, or switch to a different model.`; } else if ( errorLower.includes('command not found') || - (errorLower.includes('not found') && !errorLower.includes('model')) + errorLower.includes('is not recognized as an internal or external command') ) { enhancedError = `${errorText}\n\nTip: Make sure the Codex CLI is installed. Run 'npm install -g @openai/codex-cli' to install.`; } diff --git a/apps/server/src/routes/worktree/common.ts b/apps/server/src/routes/worktree/common.ts index 2164fe61..43c66bce 100644 --- a/apps/server/src/routes/worktree/common.ts +++ b/apps/server/src/routes/worktree/common.ts @@ -62,13 +62,21 @@ export const execEnv = { /** * Validate git remote name to prevent command injection. - * Allowed characters: alphanumerics, hyphen, underscore, dot, and slash. - * Rejects empty strings and names that are too long. + * Matches the strict validation used in add-remote.ts: + * - Rejects empty strings and names that are too long + * - Disallows names that start with '-' or '.' + * - Forbids the substring '..' + * - Rejects '/' characters + * - Rejects NUL bytes + * - Must consist only of alphanumerics, hyphens, underscores, and dots */ export function isValidRemoteName(name: string): boolean { - return ( - name.length > 0 && name.length < MAX_BRANCH_NAME_LENGTH && /^[a-zA-Z0-9._\-/]+$/.test(name) - ); + if (!name || name.length === 0 || name.length >= MAX_BRANCH_NAME_LENGTH) return false; + if (name.startsWith('-') || name.startsWith('.')) return false; + if (name.includes('..')) return false; + if (name.includes('/')) return false; + if (name.includes('\0')) return false; + return /^[a-zA-Z0-9._-]+$/.test(name); } /** diff --git a/apps/server/src/routes/worktree/routes/check-changes.ts b/apps/server/src/routes/worktree/routes/check-changes.ts index 5de1f260..b49ba8ff 100644 --- a/apps/server/src/routes/worktree/routes/check-changes.ts +++ b/apps/server/src/routes/worktree/routes/check-changes.ts @@ -82,6 +82,10 @@ export function createCheckChangesHandler() { const hasChanges = staged.length > 0 || unstaged.length > 0 || untracked.length > 0; + // Deduplicate file paths across staged, unstaged, and untracked arrays + // to avoid double-counting partially staged files + const uniqueFilePaths = new Set([...staged, ...unstaged, ...untracked]); + res.json({ success: true, result: { @@ -89,7 +93,7 @@ export function createCheckChangesHandler() { staged, unstaged, untracked, - totalFiles: staged.length + unstaged.length + untracked.length, + totalFiles: uniqueFilePaths.size, }, }); } catch (error) { diff --git a/apps/server/src/routes/worktree/routes/checkout-branch.ts b/apps/server/src/routes/worktree/routes/checkout-branch.ts index d7830a64..d8a9d828 100644 --- a/apps/server/src/routes/worktree/routes/checkout-branch.ts +++ b/apps/server/src/routes/worktree/routes/checkout-branch.ts @@ -185,13 +185,11 @@ export function createCheckoutBranchHandler(events?: EventEmitter) { } /** - * Determine whether an error message represents a client error (400) + * Determine whether an error message represents a client error (400). + * Stash failures are server-side errors and are intentionally excluded here + * so they are returned as HTTP 500 rather than HTTP 400. */ function isBranchError(error?: string): boolean { if (!error) return false; - return ( - error.includes('already exists') || - error.includes('does not exist') || - error.includes('Failed to stash') - ); + return error.includes('already exists') || error.includes('does not exist'); } diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index abecb46d..5b606e36 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/apps/server/src/services/auto-mode/facade.ts @@ -519,17 +519,15 @@ export class AutoModeServiceFacade { useWorktrees = false, _calledInternally = false ): Promise { - try { - return await this.recoveryService.resumeFeature( - this.projectPath, - featureId, - useWorktrees, - _calledInternally - ); - } catch (error) { - this.handleFacadeError(error, 'resumeFeature', featureId); - throw error; - } + // Do not call handleFacadeError here: ExecutionService.executeFeature already + // classifies and emits auto_mode_error, so calling handleFacadeError would + // produce duplicate error events. Simply let the error propagate to the caller. + return await this.recoveryService.resumeFeature( + this.projectPath, + featureId, + useWorktrees, + _calledInternally + ); } /** diff --git a/apps/server/src/services/branch-utils.ts b/apps/server/src/services/branch-utils.ts new file mode 100644 index 00000000..66510d28 --- /dev/null +++ b/apps/server/src/services/branch-utils.ts @@ -0,0 +1,155 @@ +/** + * branch-utils - Shared git branch helper utilities + * + * Provides common git operations used by both checkout-branch-service and + * worktree-branch-service. Extracted to avoid duplication and ensure + * consistent behaviour across branch-related services. + */ + +import { createLogger, getErrorMessage } from '@automaker/utils'; +import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js'; + +const logger = createLogger('BranchUtils'); + +// ============================================================================ +// Types +// ============================================================================ + +export interface HasAnyChangesOptions { + /** + * When true, lines that refer to worktree-internal paths (containing + * ".worktrees/" or ending with ".worktrees") are excluded from the count. + * Use this in contexts where worktree directory entries should not be + * considered as real working-tree changes (e.g. worktree-branch-service). + */ + excludeWorktreePaths?: boolean; +} + +// ============================================================================ +// Helpers +// ============================================================================ + +/** + * Returns true when a `git status --porcelain` output line refers to a + * worktree-internal path that should be ignored when deciding whether there + * are "real" local changes. + */ +function isExcludedWorktreeLine(line: string): boolean { + return line.includes('.worktrees/') || line.endsWith('.worktrees'); +} + +// ============================================================================ +// Exported Utilities +// ============================================================================ + +/** + * Check if there are any changes (including untracked) that should be stashed. + * + * @param cwd - Working directory of the git repository / worktree + * @param options - Optional flags controlling which lines are counted + * @param options.excludeWorktreePaths - When true, lines matching worktree + * internal paths are excluded so they are not mistaken for real changes + */ +export async function hasAnyChanges(cwd: string, options?: HasAnyChangesOptions): Promise { + try { + const stdout = await execGitCommand(['status', '--porcelain'], cwd); + const lines = stdout + .trim() + .split('\n') + .filter((line) => { + if (!line.trim()) return false; + if (options?.excludeWorktreePaths && isExcludedWorktreeLine(line)) return false; + return true; + }); + return lines.length > 0; + } catch (err) { + logger.error('hasAnyChanges: execGitCommand failed — returning false', { + cwd, + error: getErrorMessage(err), + }); + return false; + } +} + +/** + * Stash all local changes (including untracked files if requested). + * Returns true if a stash was created, false if there was nothing to stash. + * Throws on unexpected errors so callers abort rather than proceeding silently. + * + * @param cwd - Working directory of the git repository / worktree + * @param message - Stash message + * @param includeUntracked - When true, passes `--include-untracked` to git stash + */ +export async function stashChanges( + cwd: string, + message: string, + includeUntracked: boolean = true +): Promise { + try { + const args = ['stash', 'push']; + if (includeUntracked) { + args.push('--include-untracked'); + } + args.push('-m', message); + + await execGitCommandWithLockRetry(args, cwd); + return true; + } catch (error) { + const errorMsg = getErrorMessage(error); + + // "Nothing to stash" is benign – no work was lost, just return false + if ( + errorMsg.toLowerCase().includes('no local changes to save') || + errorMsg.toLowerCase().includes('nothing to stash') + ) { + logger.debug('stashChanges: nothing to stash', { cwd, message, error: errorMsg }); + return false; + } + + // Unexpected error – log full details and re-throw so the caller aborts + // rather than proceeding with an un-stashed working tree + logger.error('stashChanges: unexpected error during stash', { + cwd, + message, + error: errorMsg, + }); + throw new Error(`Failed to stash changes in ${cwd}: ${errorMsg}`); + } +} + +/** + * Pop the most recent stash entry. + * Returns an object indicating success and whether there were conflicts. + * + * @param cwd - Working directory of the git repository / worktree + */ +export async function popStash( + cwd: string +): Promise<{ success: boolean; hasConflicts: boolean; error?: string }> { + try { + await execGitCommand(['stash', 'pop'], cwd); + // If execGitCommand succeeds (zero exit code), there are no conflicts + return { success: true, hasConflicts: false }; + } catch (error) { + const errorMsg = getErrorMessage(error); + if (errorMsg.includes('CONFLICT') || errorMsg.includes('Merge conflict')) { + return { success: false, hasConflicts: true, error: errorMsg }; + } + return { success: false, hasConflicts: false, error: errorMsg }; + } +} + +/** + * Check if a local branch already exists. + * + * @param cwd - Working directory of the git repository / worktree + * @param branchName - The branch name to look up (without refs/heads/ prefix) + */ +export async function localBranchExists(cwd: string, branchName: string): Promise { + try { + await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], cwd); + return true; + } catch { + return false; + } +} diff --git a/apps/server/src/services/checkout-branch-service.ts b/apps/server/src/services/checkout-branch-service.ts index eba5511c..521d375a 100644 --- a/apps/server/src/services/checkout-branch-service.ts +++ b/apps/server/src/services/checkout-branch-service.ts @@ -19,11 +19,10 @@ * 7. Handle error recovery (restore stash if checkout fails) */ -import { createLogger, getErrorMessage } from '@automaker/utils'; -import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js'; +import { getErrorMessage } from '@automaker/utils'; +import { execGitCommand } from '../lib/git.js'; import type { EventEmitter } from '../lib/events.js'; - -const logger = createLogger('CheckoutBranchService'); +import { hasAnyChanges, stashChanges, popStash, localBranchExists } from './branch-utils.js'; // ============================================================================ // Types @@ -52,101 +51,6 @@ export interface CheckoutBranchResult { stashPopConflictMessage?: string; } -// ============================================================================ -// Helper Functions -// ============================================================================ - -/** - * Check if there are any changes (including untracked) that should be stashed - */ -async function hasAnyChanges(cwd: string): Promise { - try { - const stdout = await execGitCommand(['status', '--porcelain'], cwd); - const lines = stdout - .trim() - .split('\n') - .filter((line) => line.trim()); - return lines.length > 0; - } catch (err) { - logger.error('hasAnyChanges: execGitCommand failed — returning false', { - cwd, - error: getErrorMessage(err), - }); - return false; - } -} - -/** - * Stash all local changes (including untracked files if requested) - * Returns true if a stash was created, false if there was nothing to stash. - * Throws on unexpected errors so callers abort rather than proceeding silently. - */ -async function stashChanges( - cwd: string, - message: string, - includeUntracked: boolean -): Promise { - try { - const args = ['stash', 'push']; - if (includeUntracked) { - args.push('--include-untracked'); - } - args.push('-m', message); - - await execGitCommandWithLockRetry(args, cwd); - return true; - } catch (error) { - const errorMsg = getErrorMessage(error); - - // "Nothing to stash" is benign - if ( - errorMsg.toLowerCase().includes('no local changes to save') || - errorMsg.toLowerCase().includes('nothing to stash') - ) { - logger.debug('stashChanges: nothing to stash', { cwd, message, error: errorMsg }); - return false; - } - - logger.error('stashChanges: unexpected error during stash', { - cwd, - message, - error: errorMsg, - }); - throw new Error(`Failed to stash changes in ${cwd}: ${errorMsg}`); - } -} - -/** - * Pop the most recent stash entry - * Returns an object indicating success and whether there were conflicts - */ -async function popStash( - cwd: string -): Promise<{ success: boolean; hasConflicts: boolean; error?: string }> { - try { - await execGitCommand(['stash', 'pop'], cwd); - return { success: true, hasConflicts: false }; - } catch (error) { - const errorMsg = getErrorMessage(error); - if (errorMsg.includes('CONFLICT') || errorMsg.includes('Merge conflict')) { - return { success: false, hasConflicts: true, error: errorMsg }; - } - return { success: false, hasConflicts: false, error: errorMsg }; - } -} - -/** - * Check if a local branch already exists - */ -async function localBranchExists(cwd: string, branchName: string): Promise { - try { - await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], cwd); - return true; - } catch { - return false; - } -} - // ============================================================================ // Main Service Function // ============================================================================ @@ -175,11 +79,25 @@ export async function performCheckoutBranch( events?.emit('switch:start', { worktreePath, branchName, operation: 'checkout' }); // 1. Get current branch - const currentBranchOutput = await execGitCommand( - ['rev-parse', '--abbrev-ref', 'HEAD'], - worktreePath - ); - const previousBranch = currentBranchOutput.trim(); + let previousBranch: string; + try { + const currentBranchOutput = await execGitCommand( + ['rev-parse', '--abbrev-ref', 'HEAD'], + worktreePath + ); + previousBranch = currentBranchOutput.trim(); + } catch (branchError) { + const branchErrorMsg = getErrorMessage(branchError); + events?.emit('switch:error', { + worktreePath, + branchName, + error: branchErrorMsg, + }); + return { + success: false, + error: `Failed to determine current branch: ${branchErrorMsg}`, + }; + } // 2. Check if branch already exists if (await localBranchExists(worktreePath, branchName)) { diff --git a/apps/server/src/services/merge-service.ts b/apps/server/src/services/merge-service.ts index 72089a5e..1a814acc 100644 --- a/apps/server/src/services/merge-service.ts +++ b/apps/server/src/services/merge-service.ts @@ -6,7 +6,7 @@ import { createLogger, isValidBranchName } from '@automaker/utils'; import { type EventEmitter } from '../lib/events.js'; -import { execGitCommand } from '../lib/git.js'; +import { execGitCommand } from '@automaker/git-utils'; const logger = createLogger('MergeService'); export interface MergeOptions { @@ -157,9 +157,22 @@ export async function performMerge( }); // Unmerged status codes occupy the first two characters of each line. // Standard unmerged codes: UU, AA, DD, AU, UA, DU, UD. - hasUnmergedPaths = statusOutput + const unmergedLines = statusOutput .split('\n') - .some((line) => /^(UU|AA|DD|AU|UA|DU|UD)/.test(line)); + .filter((line) => /^(UU|AA|DD|AU|UA|DU|UD)/.test(line)); + hasUnmergedPaths = unmergedLines.length > 0; + + // If Layer 2 did not populate conflictFiles (e.g. diff failed or returned + // nothing) but Layer 3 does detect unmerged paths, parse the status lines + // to extract filenames and assign them to conflictFiles so callers always + // receive an accurate file list when conflicts are present. + if (hasUnmergedPaths && conflictFiles === undefined) { + const parsedFiles = unmergedLines + .map((line) => line.slice(2).trim()) + .filter((f) => f.length > 0); + // Deduplicate (e.g. rename entries can appear twice) + conflictFiles = [...new Set(parsedFiles)]; + } } catch { // git status failing is itself a sign something is wrong; leave // hasUnmergedPaths as false and rely on the other layers. diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index 4308825b..ba30e65f 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -519,7 +519,8 @@ export class PipelineOrchestrator { targetBranch || 'main', { deleteWorktreeAndBranch: false, - } + }, + this.eventBus.getUnderlyingEmitter() ); if (!result.success) { diff --git a/apps/server/src/services/pull-service.ts b/apps/server/src/services/pull-service.ts index b57bf3eb..9222162a 100644 --- a/apps/server/src/services/pull-service.ts +++ b/apps/server/src/services/pull-service.ts @@ -80,7 +80,11 @@ export async function getLocalChanges( .trim() .split('\n') .filter((line) => line.trim().length > 0) - .map((line) => line.substring(3).trim()); + .map((line) => { + const entry = line.substring(3).trim(); + const arrowIndex = entry.indexOf(' -> '); + return arrowIndex !== -1 ? entry.substring(arrowIndex + 4).trim() : entry; + }); } return { hasLocalChanges, localChangedFiles }; @@ -321,7 +325,11 @@ export async function performPull( if (isConflictError(errorOutput)) { pullConflict = true; - pullConflictFiles = await getConflictFiles(worktreePath); + try { + pullConflictFiles = await getConflictFiles(worktreePath); + } catch { + pullConflictFiles = []; + } } else { // Non-conflict pull error let stashRecoveryFailed = false; @@ -407,7 +415,12 @@ async function reapplyStash(worktreePath: string, branchName: string): Promise

{ - // Reject branch names that start with a dash to prevent them from being - // misinterpreted as git options. - if (ontoBranch.startsWith('-')) { + // Reject empty, whitespace-only, or dash-prefixed branch names. + const normalizedOntoBranch = ontoBranch?.trim() ?? ''; + if (normalizedOntoBranch === '' || normalizedOntoBranch.startsWith('-')) { return { success: false, - error: `Invalid branch name: "${ontoBranch}" must not start with a dash.`, + error: `Invalid branch name: "${ontoBranch}" must not be empty or start with a dash.`, }; } diff --git a/apps/server/src/services/worktree-branch-service.ts b/apps/server/src/services/worktree-branch-service.ts index de2514b4..af6bfedc 100644 --- a/apps/server/src/services/worktree-branch-service.ts +++ b/apps/server/src/services/worktree-branch-service.ts @@ -17,8 +17,9 @@ */ import { createLogger, getErrorMessage } from '@automaker/utils'; -import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js'; +import { execGitCommand } from '../lib/git.js'; import type { EventEmitter } from '../lib/events.js'; +import { hasAnyChanges, stashChanges, popStash, localBranchExists } from './branch-utils.js'; const logger = createLogger('WorktreeBranchService'); @@ -43,92 +44,9 @@ export interface SwitchBranchResult { } // ============================================================================ -// Helper Functions +// Local Helpers // ============================================================================ -function isExcludedWorktreeLine(line: string): boolean { - return line.includes('.worktrees/') || line.endsWith('.worktrees'); -} - -/** - * Check if there are any changes at all (including untracked) that should be stashed - */ -async function hasAnyChanges(cwd: string): Promise { - try { - const stdout = await execGitCommand(['status', '--porcelain'], cwd); - const lines = stdout - .trim() - .split('\n') - .filter((line) => { - if (!line.trim()) return false; - if (isExcludedWorktreeLine(line)) return false; - return true; - }); - return lines.length > 0; - } catch (err) { - logger.error('hasAnyChanges: execGitCommand failed — returning false', { - cwd, - error: getErrorMessage(err), - }); - return false; - } -} - -/** - * Stash all local changes (including untracked files) - * Returns true if a stash was created, false if there was nothing to stash. - * Throws on unexpected errors so callers abort rather than proceeding silently. - */ -async function stashChanges(cwd: string, message: string): Promise { - try { - // Stash including untracked files — a successful execGitCommand is proof - // the stash was created. No need for a post-push listing which can throw - // and incorrectly report a failed stash. - await execGitCommandWithLockRetry(['stash', 'push', '--include-untracked', '-m', message], cwd); - return true; - } catch (error) { - const errorMsg = getErrorMessage(error); - - // "Nothing to stash" is benign – no work was lost, just return false - if ( - errorMsg.toLowerCase().includes('no local changes to save') || - errorMsg.toLowerCase().includes('nothing to stash') - ) { - logger.debug('stashChanges: nothing to stash', { cwd, message, error: errorMsg }); - return false; - } - - // Unexpected error – log full details and re-throw so the caller aborts - // rather than proceeding with an un-stashed working tree - logger.error('stashChanges: unexpected error during stash', { - cwd, - message, - error: errorMsg, - }); - throw new Error(`Failed to stash changes in ${cwd}: ${errorMsg}`); - } -} - -/** - * Pop the most recent stash entry - * Returns an object indicating success and whether there were conflicts - */ -async function popStash( - cwd: string -): Promise<{ success: boolean; hasConflicts: boolean; error?: string }> { - try { - await execGitCommand(['stash', 'pop'], cwd); - // If execGitCommand succeeds (zero exit code), there are no conflicts - return { success: true, hasConflicts: false }; - } catch (error) { - const errorMsg = getErrorMessage(error); - if (errorMsg.includes('CONFLICT') || errorMsg.includes('Merge conflict')) { - return { success: false, hasConflicts: true, error: errorMsg }; - } - return { success: false, hasConflicts: false, error: errorMsg }; - } -} - /** Timeout for git fetch operations (30 seconds) */ const FETCH_TIMEOUT_MS = 30_000; @@ -200,18 +118,6 @@ async function isRemoteBranch(cwd: string, branchName: string): Promise } } -/** - * Check if a local branch already exists - */ -async function localBranchExists(cwd: string, branchName: string): Promise { - try { - await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], cwd); - return true; - } catch { - return false; - } -} - // ============================================================================ // Main Service Function // ============================================================================ @@ -308,7 +214,7 @@ export async function performSwitchBranch( } // 5. Stash local changes if any exist - const hadChanges = await hasAnyChanges(worktreePath); + const hadChanges = await hasAnyChanges(worktreePath, { excludeWorktreePaths: true }); let didStash = false; if (hadChanges) { @@ -320,7 +226,7 @@ export async function performSwitchBranch( }); const stashMessage = `automaker-branch-switch: ${previousBranch} → ${targetBranch}`; try { - didStash = await stashChanges(worktreePath, stashMessage); + didStash = await stashChanges(worktreePath, stashMessage, true); } catch (stashError) { const stashErrorMsg = getErrorMessage(stashError); events?.emit('switch:error', { diff --git a/apps/server/tests/unit/services/pipeline-orchestrator.test.ts b/apps/server/tests/unit/services/pipeline-orchestrator.test.ts index aa543afb..ff039d4c 100644 --- a/apps/server/tests/unit/services/pipeline-orchestrator.test.ts +++ b/apps/server/tests/unit/services/pipeline-orchestrator.test.ts @@ -137,6 +137,7 @@ describe('PipelineOrchestrator', () => { mockEventBus = { emitAutoModeEvent: vi.fn(), + getUnderlyingEmitter: vi.fn().mockReturnValue({}), } as unknown as TypedEventBus; mockFeatureStateManager = { @@ -492,7 +493,8 @@ describe('PipelineOrchestrator', () => { 'feature/test-1', '/test/worktree', 'main', - { deleteWorktreeAndBranch: false } + { deleteWorktreeAndBranch: false }, + expect.anything() ); }); @@ -792,7 +794,8 @@ describe('PipelineOrchestrator', () => { 'feature/test-1', '/test/project', // Falls back to projectPath when worktreePath is null 'main', - { deleteWorktreeAndBranch: false } + { deleteWorktreeAndBranch: false }, + expect.anything() ); }); }); @@ -845,7 +848,8 @@ describe('PipelineOrchestrator', () => { 'feature/test-1', '/test/custom-worktree', 'main', - { deleteWorktreeAndBranch: false } + { deleteWorktreeAndBranch: false }, + expect.anything() ); }); @@ -861,7 +865,8 @@ describe('PipelineOrchestrator', () => { 'feature/custom-branch', '/test/worktree', 'main', - { deleteWorktreeAndBranch: false } + { deleteWorktreeAndBranch: false }, + expect.anything() ); }); diff --git a/apps/ui/src/components/ui/git-diff-panel.tsx b/apps/ui/src/components/ui/git-diff-panel.tsx index 8529a5a8..84fe7cac 100644 --- a/apps/ui/src/components/ui/git-diff-panel.tsx +++ b/apps/ui/src/components/ui/git-diff-panel.tsx @@ -641,6 +641,12 @@ export function GitDiffPanel({ const handleStageAll = useCallback(async () => { const allPaths = files.map((f) => f.path); if (allPaths.length === 0) return; + if (enableStaging && useWorktrees && !worktreePath) { + toast.error('Failed to stage all files', { + description: 'worktreePath required when useWorktrees is enabled', + }); + return; + } await executeStagingAction( 'stage', allPaths, @@ -649,11 +655,17 @@ export function GitDiffPanel({ () => setStagingInProgress(new Set(allPaths)), () => setStagingInProgress(new Set()) ); - }, [worktreePath, projectPath, useWorktrees, files, executeStagingAction]); + }, [worktreePath, projectPath, useWorktrees, enableStaging, files, executeStagingAction]); const handleUnstageAll = useCallback(async () => { const allPaths = files.map((f) => f.path); if (allPaths.length === 0) return; + if (enableStaging && useWorktrees && !worktreePath) { + toast.error('Failed to unstage all files', { + description: 'worktreePath required when useWorktrees is enabled', + }); + return; + } await executeStagingAction( 'unstage', allPaths, @@ -662,7 +674,7 @@ export function GitDiffPanel({ () => setStagingInProgress(new Set(allPaths)), () => setStagingInProgress(new Set()) ); - }, [worktreePath, projectPath, useWorktrees, files, executeStagingAction]); + }, [worktreePath, projectPath, useWorktrees, enableStaging, files, executeStagingAction]); // Compute staging summary const stagingSummary = useMemo(() => { @@ -899,68 +911,70 @@ export function GitDiffPanel({ {/* Fallback for files that have no diff content (shouldn't happen after fix, but safety net) */} {files.length > 0 && parsedDiffs.length === 0 && (

- {files.map((file) => ( -
-
- {getFileIcon(file.status)} - - {enableStaging && } - - {getStatusDisplayName(file.status)} - - {enableStaging && ( -
- {stagingInProgress.has(file.path) ? ( - - ) : getStagingState(file) === 'staged' || - getStagingState(file) === 'partial' ? ( - - ) : ( - + {files.map((file) => { + const stagingState = getStagingState(file); + return ( +
+
+ {getFileIcon(file.status)} + + {enableStaging && } + - )} + > + {getStatusDisplayName(file.status)} + + {enableStaging && ( +
+ {stagingInProgress.has(file.path) ? ( + + ) : stagingState === 'staged' || stagingState === 'partial' ? ( + + ) : ( + + )} +
+ )} +
+
+ {file.status === '?' ? ( + New file - content preview not available + ) : file.status === 'D' ? ( + File deleted + ) : ( + Diff content not available + )} +
-
- {file.status === '?' ? ( - New file - content preview not available - ) : file.status === 'D' ? ( - File deleted - ) : ( - Diff content not available - )} -
-
- ))} + ); + })}
)}
diff --git a/apps/ui/src/components/usage-popover.tsx b/apps/ui/src/components/usage-popover.tsx index 014a0f2c..049a142a 100644 --- a/apps/ui/src/components/usage-popover.tsx +++ b/apps/ui/src/components/usage-popover.tsx @@ -93,11 +93,12 @@ export function UsagePopover() { // Track whether the user has manually selected a tab so we don't override their choice const userHasSelected = useRef(false); - // Check authentication status + // Check authentication status — use explicit boolean coercion so hooks never + // receive undefined for their `enabled` parameter during auth-loading const isClaudeAuthenticated = !!claudeAuthStatus?.authenticated; - const isCodexAuthenticated = codexAuthStatus?.authenticated; - const isZaiAuthenticated = zaiAuthStatus?.authenticated; - const isGeminiAuthenticated = geminiAuthStatus?.authenticated; + const isCodexAuthenticated = !!codexAuthStatus?.authenticated; + const isZaiAuthenticated = !!zaiAuthStatus?.authenticated; + const isGeminiAuthenticated = !!geminiAuthStatus?.authenticated; // Use React Query hooks for usage data // Only enable polling when popover is open AND the tab is active @@ -239,12 +240,6 @@ export function UsagePopover() { return !geminiUsageLastUpdated || Date.now() - geminiUsageLastUpdated > 2 * 60 * 1000; }, [geminiUsageLastUpdated]); - // Refetch functions for manual refresh - const fetchClaudeUsage = () => refetchClaude(); - const fetchCodexUsage = () => refetchCodex(); - const fetchZaiUsage = () => refetchZai(); - const fetchGeminiUsage = () => refetchGemini(); - // Derived status color/icon helper const getStatusInfo = (percentage: number) => { if (percentage >= 75) return { color: 'text-red-500', icon: XCircle, bg: 'bg-red-500' }; @@ -368,13 +363,6 @@ export function UsagePopover() { // Calculate max percentage for header button const claudeSessionPercentage = claudeUsage?.sessionPercentage || 0; - const _codexMaxPercentage = codexUsage?.rateLimits - ? Math.max( - codexUsage.rateLimits.primary?.usedPercent || 0, - codexUsage.rateLimits.secondary?.usedPercent || 0 - ) - : 0; - const zaiMaxPercentage = zaiUsage?.quotaLimits ? Math.max( zaiUsage.quotaLimits.tokens?.usedPercent || 0, @@ -383,7 +371,8 @@ export function UsagePopover() { : 0; // Gemini quota from Google Cloud API (if available) - const geminiMaxPercentage = geminiUsage?.usedPercent ?? (geminiUsage?.authenticated ? 0 : 100); + // Default to 0 when usedPercent is not available to avoid a misleading full-red indicator + const geminiMaxPercentage = geminiUsage?.usedPercent ?? 0; const getProgressBarColor = (percentage: number) => { if (percentage >= 80) return 'bg-red-500'; @@ -397,9 +386,6 @@ export function UsagePopover() { codexSecondaryWindowMinutes && codexPrimaryWindowMinutes ? Math.min(codexPrimaryWindowMinutes, codexSecondaryWindowMinutes) : (codexSecondaryWindowMinutes ?? codexPrimaryWindowMinutes); - const _codexWindowLabel = codexWindowMinutes - ? getCodexWindowLabel(codexWindowMinutes).title - : 'Window'; const codexWindowUsage = codexWindowMinutes === codexSecondaryWindowMinutes ? codexUsage?.rateLimits?.secondary?.usedPercent @@ -537,7 +523,7 @@ export function UsagePopover() { variant="ghost" size="icon" className={cn('h-6 w-6', claudeLoading && 'opacity-80')} - onClick={() => !claudeLoading && fetchClaudeUsage()} + onClick={() => !claudeLoading && refetchClaude()} > @@ -646,7 +632,7 @@ export function UsagePopover() { variant="ghost" size="icon" className={cn('h-6 w-6', codexLoading && 'opacity-80')} - onClick={() => !codexLoading && fetchCodexUsage()} + onClick={() => !codexLoading && refetchCodex()} > @@ -783,7 +769,7 @@ export function UsagePopover() { variant="ghost" size="icon" className={cn('h-6 w-6', zaiLoading && 'opacity-80')} - onClick={() => !zaiLoading && fetchZaiUsage()} + onClick={() => !zaiLoading && refetchZai()} > @@ -899,7 +885,7 @@ export function UsagePopover() { variant="ghost" size="icon" className={cn('h-6 w-6', geminiLoading && 'opacity-80')} - onClick={() => !geminiLoading && fetchGeminiUsage()} + onClick={() => !geminiLoading && refetchGemini()} > diff --git a/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx b/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx index dca3cd4f..ce5087c4 100644 --- a/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx +++ b/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx @@ -32,6 +32,7 @@ import { type UncommittedChangesInfo, type StashConfirmAction, } from './stash-confirm-dialog'; +import { type BranchInfo } from '../worktree-panel/types'; interface WorktreeInfo { path: string; @@ -41,12 +42,6 @@ interface WorktreeInfo { changedFilesCount?: number; } -interface BranchInfo { - name: string; - isCurrent: boolean; - isRemote: boolean; -} - const logger = createLogger('CreateBranchDialog'); interface CreateBranchDialogProps { @@ -67,6 +62,7 @@ export function CreateBranchDialog({ const [branches, setBranches] = useState([]); const [isLoadingBranches, setIsLoadingBranches] = useState(false); const [isCreating, setIsCreating] = useState(false); + const [isChecking, setIsChecking] = useState(false); const [error, setError] = useState(null); const [baseBranchPopoverOpen, setBaseBranchPopoverOpen] = useState(false); const baseBranchTriggerRef = useRef(null); @@ -121,6 +117,7 @@ export function CreateBranchDialog({ setBaseBranchPopoverOpen(false); setShowStashConfirm(false); setUncommittedChanges(null); + setIsChecking(false); fetchBranches(); } }, [open, fetchBranches]); @@ -151,6 +148,7 @@ export function CreateBranchDialog({ const api = getElectronAPI(); if (!api?.worktree?.checkoutBranch) { toast.error('Branch API not available'); + setIsCreating(false); return; } @@ -197,6 +195,8 @@ export function CreateBranchDialog({ * Checks for uncommitted changes first and shows confirmation if needed. */ const handleCreate = async () => { + // Guard against concurrent invocations during the async pre-check or creation + if (isCreating || isChecking) return; if (!worktree || !branchName.trim()) return; // Basic validation @@ -207,6 +207,7 @@ export function CreateBranchDialog({ } setError(null); + setIsChecking(true); // Check for uncommitted changes before proceeding try { @@ -221,6 +222,7 @@ export function CreateBranchDialog({ untracked: changesResult.result.untracked, totalFiles: changesResult.result.totalFiles, }); + setIsChecking(false); setShowStashConfirm(true); return; } @@ -229,6 +231,8 @@ export function CreateBranchDialog({ logger.warn('Failed to check for uncommitted changes, proceeding without stash:', err); } + setIsChecking(false); + // No changes detected, proceed directly doCreate(false); }; @@ -289,11 +293,11 @@ export function CreateBranchDialog({ setError(null); }} onKeyDown={(e) => { - if (e.key === 'Enter' && branchName.trim() && !isCreating) { + if (e.key === 'Enter' && branchName.trim() && !isCreating && !isChecking) { handleCreate(); } }} - disabled={isCreating} + disabled={isCreating || isChecking} autoFocus />
@@ -417,15 +421,27 @@ export function CreateBranchDialog({ - -