From ac87594b5d227e61be7851af62ebae657be7f2b9 Mon Sep 17 00:00:00 2001 From: Stefan de Vogelaere Date: Sun, 11 Jan 2026 16:28:31 +0100 Subject: [PATCH] fix: address code review feedback from PR #423 Addresses feedback from gemini-code-assist and coderabbitai reviewers: ## Duplicate Code (High Priority) - Extract `getEffectiveDefaultEditor` logic into shared `useEffectiveDefaultEditor` hook - Both account-section.tsx and worktree-actions-dropdown.tsx now use the shared hook ## Performance (Medium Priority) - Refactor `detectAllEditors` to use `Promise.all` for parallel editor detection - Replace sequential `await tryAddEditor()` calls with parallel `findEditor()` checks ## Code Quality (Medium Priority) - Remove verbose IIFE pattern for editor icon rendering - Pre-compute icon components before JSX return statement ## Bug Fixes - Use `os.homedir()` instead of `~` fallback which doesn't expand in shell - Normalize Select value to 'auto' when saved editor command not found in editors - Add defensive check for empty editors array in useEffectiveDefaultEditor - Improve mock openInEditor to correctly map all editor commands to display names Co-Authored-By: Claude Opus 4.5 --- .../routes/worktree/routes/open-in-editor.ts | 53 ++++++++++--------- .../components/worktree-actions-dropdown.tsx | 35 ++++-------- .../hooks/use-available-editors.ts | 30 ++++++++++- .../settings-view/account/account-section.tsx | 36 ++++++------- apps/ui/src/lib/electron.ts | 19 ++++++- 5 files changed, 102 insertions(+), 71 deletions(-) diff --git a/apps/server/src/routes/worktree/routes/open-in-editor.ts b/apps/server/src/routes/worktree/routes/open-in-editor.ts index 492ac85a..43053692 100644 --- a/apps/server/src/routes/worktree/routes/open-in-editor.ts +++ b/apps/server/src/routes/worktree/routes/open-in-editor.ts @@ -6,6 +6,7 @@ import type { Request, Response } from 'express'; import { exec } from 'child_process'; import { promisify } from 'util'; +import { homedir } from 'os'; import { getErrorMessage, logError } from '../common.js'; const execAsync = promisify(exec); @@ -48,7 +49,7 @@ async function findMacApp(appName: string): Promise { // Check ~/Applications (used by JetBrains Toolbox and others) try { - const homeDir = process.env.HOME || '~'; + const homeDir = homedir(); await execAsync(`test -d "${homeDir}/Applications/${appName}.app"`); return `${homeDir}/Applications/${appName}.app`; } catch { @@ -57,18 +58,17 @@ async function findMacApp(appName: string): Promise { } /** - * Try to add an editor - checks CLI first, then macOS app bundle + * Try to find an editor - checks CLI first, then macOS app bundle + * Returns EditorInfo if found, null otherwise */ -async function tryAddEditor( - editors: EditorInfo[], +async function findEditor( name: string, cliCommand: string, macAppName: string -): Promise { +): Promise { // Try CLI command first if (await commandExists(cliCommand)) { - editors.push({ name, command: cliCommand }); - return; + return { name, command: cliCommand }; } // Try macOS app bundle (checks /Applications and ~/Applications) @@ -76,9 +76,11 @@ async function tryAddEditor( const appPath = await findMacApp(macAppName); if (appPath) { // Use 'open -a' with full path for apps not in /Applications - editors.push({ name, command: `open -a "${appPath}"` }); + return { name, command: `open -a "${appPath}"` }; } } + + return null; } async function detectAllEditors(): Promise { @@ -87,26 +89,29 @@ async function detectAllEditors(): Promise { return cachedEditors; } - const editors: EditorInfo[] = []; const isMac = process.platform === 'darwin'; - // Try editors (CLI command, then macOS app bundle) - await tryAddEditor(editors, 'Cursor', 'cursor', 'Cursor'); - await tryAddEditor(editors, 'VS Code', 'code', 'Visual Studio Code'); - await tryAddEditor(editors, 'Zed', 'zed', 'Zed'); - await tryAddEditor(editors, 'Sublime Text', 'subl', 'Sublime Text'); - await tryAddEditor(editors, 'Windsurf', 'windsurf', 'Windsurf'); - await tryAddEditor(editors, 'Trae', 'trae', 'Trae'); - await tryAddEditor(editors, 'Rider', 'rider', 'Rider'); - await tryAddEditor(editors, 'WebStorm', 'webstorm', 'WebStorm'); + // Check all editors in parallel for better performance + const editorChecks = [ + findEditor('Cursor', 'cursor', 'Cursor'), + findEditor('VS Code', 'code', 'Visual Studio Code'), + findEditor('Zed', 'zed', 'Zed'), + findEditor('Sublime Text', 'subl', 'Sublime Text'), + findEditor('Windsurf', 'windsurf', 'Windsurf'), + findEditor('Trae', 'trae', 'Trae'), + findEditor('Rider', 'rider', 'Rider'), + findEditor('WebStorm', 'webstorm', 'WebStorm'), + // Xcode (macOS only) - will return null on other platforms + isMac ? findEditor('Xcode', 'xed', 'Xcode') : Promise.resolve(null), + findEditor('Android Studio', 'studio', 'Android Studio'), + findEditor('Antigravity', 'agy', 'Antigravity'), + ]; - // Xcode (macOS only) - if (isMac) { - await tryAddEditor(editors, 'Xcode', 'xed', 'Xcode'); - } + // Wait for all checks to complete in parallel + const results = await Promise.all(editorChecks); - await tryAddEditor(editors, 'Android Studio', 'studio', 'Android Studio'); - await tryAddEditor(editors, 'Antigravity', 'agy', 'Antigravity'); + // Filter out null results (editors not found) + const editors = results.filter((e): e is EditorInfo => e !== null); // Always add file manager as fallback const platform = process.platform; diff --git a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx index af7acd67..8d404341 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx @@ -28,9 +28,8 @@ import { import { cn } from '@/lib/utils'; import type { WorktreeInfo, DevServerInfo, PRInfo, GitRepoStatus } from '../types'; import { TooltipWrapper } from './tooltip-wrapper'; -import { useAvailableEditors } from '../hooks/use-available-editors'; +import { useAvailableEditors, useEffectiveDefaultEditor } from '../hooks/use-available-editors'; import { getEditorIcon } from '@/components/icons/editor-icons'; -import { useAppStore } from '@/store/app-store'; interface WorktreeActionsDropdownProps { worktree: WorktreeInfo; @@ -84,30 +83,19 @@ export function WorktreeActionsDropdown({ onOpenDevServerUrl, }: WorktreeActionsDropdownProps) { // Get available editors for the "Open In" submenu - const { editors, hasMultipleEditors } = useAvailableEditors(); + const { editors } = useAvailableEditors(); - // Get the user's preferred default editor from settings - const defaultEditorCommand = useAppStore((s) => s.defaultEditorCommand); - - // Calculate effective default editor based on user setting or auto-detect (Cursor > VS Code > first) - const getEffectiveDefaultEditor = () => { - if (defaultEditorCommand) { - const found = editors.find((e) => e.command === defaultEditorCommand); - if (found) return found; - } - // Auto-detect: prefer Cursor, then VS Code, then first available - const cursor = editors.find((e) => e.command === 'cursor'); - if (cursor) return cursor; - const vscode = editors.find((e) => e.command === 'code'); - if (vscode) return vscode; - return editors[0]; - }; - - const effectiveDefaultEditor = getEffectiveDefaultEditor(); + // Use shared hook for effective default editor + const effectiveDefaultEditor = useEffectiveDefaultEditor(editors); // Get other editors (excluding the default) for the submenu const otherEditors = editors.filter((e) => e.command !== effectiveDefaultEditor?.command); + // Get icon component for the effective editor (avoid IIFE in JSX) + const DefaultEditorIcon = effectiveDefaultEditor + ? getEditorIcon(effectiveDefaultEditor.command) + : null; + // Check if there's a PR associated with this worktree from stored metadata const hasPR = !!worktree.pr; @@ -240,10 +228,7 @@ export function WorktreeActionsDropdown({ onClick={() => onOpenInEditor(worktree, effectiveDefaultEditor.command)} className="text-xs flex-1 pr-0 rounded-r-none" > - {(() => { - const EditorIcon = getEditorIcon(effectiveDefaultEditor.command); - return ; - })()} + {DefaultEditorIcon && } Open in {effectiveDefaultEditor.name} {/* Chevron trigger for submenu with other editors and Copy Path */} diff --git a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-editors.ts b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-editors.ts index 9cf97576..024a763e 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-editors.ts +++ b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-editors.ts @@ -1,6 +1,7 @@ -import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useMemo } from 'react'; import { createLogger } from '@automaker/utils/logger'; import { getElectronAPI } from '@/lib/electron'; +import { useAppStore } from '@/store/app-store'; const logger = createLogger('AvailableEditors'); @@ -44,3 +45,30 @@ export function useAvailableEditors() { defaultEditor: editors[0] ?? null, }; } + +/** + * Hook to get the effective default editor based on user settings + * Falls back to: Cursor > VS Code > first available editor + */ +export function useEffectiveDefaultEditor(editors: EditorInfo[]): EditorInfo | null { + const defaultEditorCommand = useAppStore((s) => s.defaultEditorCommand); + + return useMemo(() => { + if (editors.length === 0) return null; + + // If user has a saved preference and it exists in available editors, use it + if (defaultEditorCommand) { + const found = editors.find((e) => e.command === defaultEditorCommand); + if (found) return found; + } + + // Auto-detect: prefer Cursor, then VS Code, then first available + const cursor = editors.find((e) => e.command === 'cursor'); + if (cursor) return cursor; + + const vscode = editors.find((e) => e.command === 'code'); + if (vscode) return vscode; + + return editors[0]; + }, [editors, defaultEditorCommand]); +} diff --git a/apps/ui/src/components/views/settings-view/account/account-section.tsx b/apps/ui/src/components/views/settings-view/account/account-section.tsx index ae594166..3d6bfa61 100644 --- a/apps/ui/src/components/views/settings-view/account/account-section.tsx +++ b/apps/ui/src/components/views/settings-view/account/account-section.tsx @@ -13,7 +13,10 @@ import { cn } from '@/lib/utils'; import { logout } from '@/lib/http-api-client'; import { useAuthStore } from '@/store/auth-store'; import { useAppStore } from '@/store/app-store'; -import { useAvailableEditors } from '@/components/views/board-view/worktree-panel/hooks/use-available-editors'; +import { + useAvailableEditors, + useEffectiveDefaultEditor, +} from '@/components/views/board-view/worktree-panel/hooks/use-available-editors'; import { getEditorIcon } from '@/components/icons/editor-icons'; export function AccountSection() { @@ -25,20 +28,16 @@ export function AccountSection() { const defaultEditorCommand = useAppStore((s) => s.defaultEditorCommand); const setDefaultEditorCommand = useAppStore((s) => s.setDefaultEditorCommand); - // Get effective default editor (respecting auto-detect order: Cursor > VS Code > first) - const getEffectiveDefaultEditor = () => { - if (defaultEditorCommand) { - return editors.find((e) => e.command === defaultEditorCommand) ?? editors[0]; - } - // Auto-detect: prefer Cursor, then VS Code, then first available - const cursor = editors.find((e) => e.command === 'cursor'); - if (cursor) return cursor; - const vscode = editors.find((e) => e.command === 'code'); - if (vscode) return vscode; - return editors[0]; - }; + // Use shared hook for effective default editor + const effectiveEditor = useEffectiveDefaultEditor(editors); - const effectiveEditor = getEffectiveDefaultEditor(); + // Normalize Select value: if saved editor isn't found, show 'auto' + const hasSavedEditor = + !!defaultEditorCommand && editors.some((e) => e.command === defaultEditorCommand); + const selectValue = hasSavedEditor ? defaultEditorCommand : 'auto'; + + // Get icon component for the effective editor + const EffectiveEditorIcon = effectiveEditor ? getEditorIcon(effectiveEditor.command) : null; const handleLogout = async () => { setIsLoggingOut(true); @@ -87,7 +86,7 @@ export function AccountSection() {