feat(launch-app): Add auto-install dependencies with full UI feedback #1313
Conversation
When a task is in Human Review, users can now click "Launch App" to start the dev server from the worktree directory. This lets them test changes before merging. Features: - Auto-detects dev command from package.json (dev, start, serve, develop) - Opens a new terminal window with the dev server running - Works on Windows, macOS, and Linux Changes: - Add TASK_WORKTREE_LAUNCH_APP IPC channel - Add handler in worktree-handlers.ts to detect and run dev command - Add worktreeLaunchApp API in preload - Add "Launch App" button in WorkspaceStatus.tsx (prominent, primary style) Partial fix for AndyMik90#1119 Co-Authored-By: Claude Opus 4.5 <[email protected]>
The method was defined in task-api.ts but not in the shared types ElectronAPI interface, causing TypeScript errors. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Added the mock implementation for worktreeLaunchApp to satisfy the ElectronAPI interface requirements in browser mock. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use execSync with 'which' to check if the terminal emulator is installed before attempting to spawn it. This prevents incorrectly reporting success when no terminal is actually launched. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add escapePathForShell() helper function to sanitize file paths before use in shell commands. This prevents command injection attacks where malicious characters in paths could execute arbitrary commands. The function: - Rejects paths containing null bytes or newlines - On Windows: rejects dangerous cmd.exe metacharacters (<>|&^%!`) - On Unix: escapes single quotes using '\'' pattern Applied to TASK_WORKTREE_LAUNCH_APP handler for all platforms: - Windows cmd.exe - macOS AppleScript/Terminal - Linux terminal emulators Addresses Sentry bot CRITICAL severity security finding. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
- Fix TOCTOU race condition in readSettingsFileAsync by reading file directly instead of checking existence first - Improve Linux terminal spawn error handling by listening for async spawn errors and catching immediate failures - Include last error message in "no terminal found" error for debugging Co-Authored-By: Claude Opus 4.5 <[email protected]>
Extract shell escaping to dedicated module for testability. Add 34 tests covering: - Null byte injection prevention - Newline injection prevention - Windows dangerous character rejection - Unix single quote escaping - Command injection attack patterns - Edge cases (empty strings, Unicode, emoji) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
This commit fixes 5 issues identified in PR AndyMik90#1120: 1. HIGH - Fix path escaping for xfce4-terminal/xterm: - xfce4-terminal: use single-quote escaping within the -e argument - xterm: pass arguments separately like gnome-terminal to avoid shell interpolation issues with the '\'' escape pattern 2. MEDIUM - Add error handling for Windows/macOS spawn: - Add proc.once('error') handler and await setImmediate pattern - Return error to caller instead of silently ignoring spawn failures 3. MEDIUM - Handle missing dev script gracefully: - Track whether a valid script was found in package.json - Return descriptive error if no dev/start/serve/develop script exists 4. MEDIUM - Replace hardcoded 'Launch App' with i18n: - Add workspace.launchApp and related keys to taskReview.json - Add French translations in fr/taskReview.json - Update button to use t('taskReview:workspace.launchApp') 5. LOW - Add user feedback via toast notifications: - Import and use useToast hook - Show success toast with command on successful launch - Show error toast with details on failure Co-Authored-By: Claude Opus 4.5 <[email protected]>
…aceStatus - Fix useToast import path in WorkspaceStatus.tsx (use hooks/use-toast) - Fix spawnError type inference in worktree-handlers.ts by using Error | undefined instead of Error | null to help TypeScript understand the callback may have set the value Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add AppleScript-specific escaping for macOS terminal launch (escape backslashes and double quotes for AppleScript string context) - Fix xfce4-terminal escaping by using -x flag instead of -e (avoids double-escaping issues with single quotes in paths) These changes address bot review feedback about path escaping edge cases when launching terminals with paths containing special characters. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use xfce4-terminal's --working-directory option instead of escaping the path in the cd command. This avoids shell escaping issues entirely by passing the path as an option value, similar to how konsole uses --workdir. The xterm fallback continues to use the same escaping approach as gnome-terminal with properly escaped single quotes. Fixes HIGH priority issue from PR AndyMik90#1120 review. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix Linux terminal spawn error handling: Use 'spawn' event to properly verify process started successfully instead of unreliable timeouts - Fix macOS AppleScript double quote escaping: Add new escapePathForAppleScript function to properly escape backslashes and double quotes for AppleScript - Remove redundant dynamic imports: spawn and execSync are already imported at the top of the file - Use platform abstraction helpers: Replace process.platform checks with isWindows() and isMacOS() from the platform module for consistency - Add comprehensive tests for escapePathForAppleScript function Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove unused pytest import in test_dependency_validator.py (github-advanced-security) - Fix TOCTOU race condition in sync readSettingsFile() function (github-advanced-security) Now reads file directly without checking existence first, matching the async version Co-Authored-By: Claude Opus 4.5 <[email protected]>
The Windows `start` command interprets the first quoted argument as a
window title. When the path contained quotes, the cd command was being
treated as the title instead of executed.
Changes:
- Add empty window title ("") as first argument to `start`
- Remove `shell: true` which was mangling nested quotes
Fixes the Launch App button opening wrong directory on Windows.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
# Conflicts: # package-lock.json # tests/test_dependency_validator.py
Level 1-5 implementation for Launch App dependency handling: Backend changes: - Add detectPackageManager() to identify pnpm/yarn/bun/npm from lockfiles - Add TASK_WORKTREE_INSTALL_DEPS IPC handler for installing dependencies - Modify TASK_WORKTREE_LAUNCH_APP to check deps and support auto-install - Return structured response with depsInstalled, packageManager info - Use correct package manager for run commands (pnpm run dev vs npm run dev) Frontend changes: - Add dependency status state tracking in WorkspaceStatus component - Show warning banner when dependencies are missing - Add "Install" button in warning banner for quick installation - Add "Install & Launch" button for one-click install + launch - Show loading state during installation with spinner - Disable Launch App button while installing API changes: - worktreeLaunchApp now accepts optional autoInstall parameter - worktreeLaunchApp returns depsInstalled, packageManager in response - New worktreeInstallDeps API for standalone dependency installation This provides a seamless UX when worktrees are missing node_modules, guiding users to install dependencies instead of showing cryptic errors. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @youngmrz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience when launching applications from a worktree by introducing intelligent dependency management. Previously, users encountered cryptic errors if project dependencies were not installed. The new implementation automatically detects missing dependencies, identifies the correct package manager, and provides clear UI options to install them, including a one-click 'Install & Launch' feature. This streamlines the development workflow and makes the application more robust against common setup issues, while also enhancing security by preventing command injection during shell operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature for automatically handling dependencies and launching applications from worktrees, complete with UI feedback. The implementation is comprehensive, covering backend logic for package manager detection and installation, as well as frontend components for user interaction. The addition of shell-escape utilities with thorough tests is a great step towards securing shell command execution.
My review has identified a few critical issues related to the use of synchronous functions (execSync) for potentially long-running operations like dependency installation. These will block the main process and freeze the application, and should be replaced with asynchronous alternatives. I've also provided suggestions to improve the robustness of process spawning error handling, and to enhance code consistency and maintainability in both the backend handlers and the frontend React component.
Overall, this is a valuable feature, and addressing these points will greatly improve the application's responsiveness and stability.
| execSync(`${packageManager} install`, { | ||
| cwd: worktreePath, | ||
| stdio: 'pipe', | ||
| timeout: 300000, // 5 minute timeout | ||
| encoding: 'utf-8' | ||
| }); |
There was a problem hiding this comment.
Using execSync here is a critical issue as it will block the main Electron process, freezing the entire application's UI for the duration of the command. Dependency installation can take a significant amount of time (as indicated by the 5-minute timeout). You should use an asynchronous alternative like spawn or exec from the child_process module to avoid blocking the event loop. You can wrap it in a Promise to use it with async/await.
| execSync(`${packageManager} install`, { | ||
| cwd: worktreePath, | ||
| stdio: 'pipe', | ||
| timeout: 300000, // 5 minute timeout | ||
| encoding: 'utf-8' | ||
| }); |
There was a problem hiding this comment.
Similar to the TASK_WORKTREE_INSTALL_DEPS handler, using execSync here will block the main process and freeze the UI. This is a critical issue for a potentially long-running operation like installing dependencies. Please use an asynchronous method like spawn or exec to keep the application responsive.
| // Handle spawn errors | ||
| let spawnFailed = false; | ||
| let spawnError: Error | undefined; | ||
| proc.once('error', (err) => { | ||
| spawnFailed = true; | ||
| spawnError = err; | ||
| }); | ||
|
|
||
| // Give a brief moment for synchronous spawn errors to propagate | ||
| await new Promise(resolve => setImmediate(resolve)); | ||
|
|
||
| if (spawnFailed) { | ||
| return { | ||
| success: false, | ||
| error: `Failed to launch terminal: ${spawnError?.message ?? 'Unknown error'}` | ||
| }; | ||
| } |
There was a problem hiding this comment.
The error handling for spawn on Windows is fragile. It uses setImmediate to wait for a potential synchronous error, which is not guaranteed to work and is inconsistent with the more robust Promise-based approach used for macOS and Linux in this same function. To improve reliability and consistency, you should refactor this to use a Promise that resolves or rejects based on the spawn and error events, just like you've done for the other platforms.
// Wait for either 'spawn' event (success) or 'error' event (failure)
const spawnResult = await new Promise<{ success: boolean; error?: Error }>((resolve) => {
proc.once('spawn', () => {
resolve({ success: true });
});
proc.once('error', (err) => {
resolve({ success: false, error: err });
});
});
if (!spawnResult.success) {
return {
success: false,
error: `Failed to launch terminal: ${spawnResult.error?.message ?? 'Unknown error'}`
};
}| // Reject paths with null bytes (always dangerous) | ||
| if (filePath.includes('\0')) { | ||
| return null; | ||
| } | ||
|
|
||
| // Reject paths with newlines (can break command structure) | ||
| if (filePath.includes('\n') || filePath.includes('\r')) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
There's some code duplication here for validating the filePath. The same checks for null bytes and newlines are also present in escapePathForShell (lines 17-25). To improve maintainability and reduce redundancy, consider extracting this validation logic into a private helper function within this module. This function could then be called at the beginning of both escapePathForShell and escapePathForAppleScript.
| if (scripts.dev) { | ||
| devCommand = `${packageManager} run dev`; | ||
| } else if (scripts.start) { | ||
| devCommand = packageManager === 'npm' ? 'npm start' : `${packageManager} run start`; |
There was a problem hiding this comment.
The special handling for npm start is not necessary and can be simplified. All major package managers (npm, yarn, pnpm, bun) support running the start script via [packageManager] start as a shortcut for [packageManager] run start. You can simplify this line to be consistent with the other script commands.
| devCommand = packageManager === 'npm' ? 'npm start' : `${packageManager} run start`; | |
| devCommand = `${packageManager} start`; |
| for (const term of terminals) { | ||
| try { | ||
| // Check if the terminal is installed before spawning | ||
| execSync(`which ${term}`, { stdio: 'ignore' }); |
There was a problem hiding this comment.
While which is usually fast, using execSync in the main process can still introduce small blockages that affect UI responsiveness. It's better to consistently use asynchronous process execution. Consider using the asynchronous exec and wrapping it in a promise, or checking for the executable's existence using an async file system method against directories in the PATH.
| } | ||
| } | ||
|
|
||
| return { success: true, data: { launched: true, command: devCommand } }; |
There was a problem hiding this comment.
For consistency and to prevent potential issues on the frontend, it's a good practice to include the packageManager and depsInstalled status in the success response data, just as it's included in the failure responses. This ensures the frontend always has the most up-to-date information without needing to guess or rely on previous state.
return { success: true, data: { launched: true, command: devCommand, depsInstalled: true, packageManager } };| try { | ||
| const result = await window.electronAPI.worktreeInstallDeps(worktreeStatus.worktreePath); | ||
| if (result.success) { | ||
| setDepsStatus({ missing: false, packageManager: result.data?.packageManager || 'npm', installing: false }); |
There was a problem hiding this comment.
For consistency and robustness, it's better to fall back to the previous package manager from the state (prev.packageManager) instead of hardcoding 'npm'. While the backend is expected to always return packageManager on success here, this change makes the frontend more resilient to potential API changes.
| setDepsStatus({ missing: false, packageManager: result.data?.packageManager || 'npm', installing: false }); | |
| setDepsStatus(prev => ({ missing: false, packageManager: result.data?.packageManager || prev.packageManager, installing: false })); |
| try { | ||
| const result = await window.electronAPI.worktreeLaunchApp(worktreeStatus.worktreePath, autoInstall); | ||
| if (result.success) { | ||
| setDepsStatus({ missing: false, packageManager: result.data?.packageManager || 'npm', installing: false }); |
There was a problem hiding this comment.
When the launch is successful, result.data.packageManager might be undefined because the backend doesn't always return it on success. Falling back to 'npm' could incorrectly reset the detected package manager. It would be safer to fall back to the previous state's package manager to avoid this.
| setDepsStatus({ missing: false, packageManager: result.data?.packageManager || 'npm', installing: false }); | |
| setDepsStatus(prev => ({ missing: false, packageManager: result.data?.packageManager || prev.packageManager, installing: false })); |
- Critical: Replace execSync with async runCommandAsync() to avoid blocking the main process/UI during dependency installation - High: Windows spawn error handling now uses Promise-based pattern instead of unreliable setImmediate timeout - Medium: Simplify start script handling - all package managers support shorthand `start` without `run` - Medium: Use commandExistsAsync() for Linux terminal detection to avoid sync which calls - Medium: Include packageManager and depsInstalled in success response - Medium: Frontend uses prev.packageManager as fallback instead of 'npm' Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Kill existing dev server processes from the same worktree before launching to prevent port conflicts and stale server issues - Only kills processes matching the specific worktree path - other tasks' dev servers are unaffected - Fix Windows command quoting by using `start /d path` instead of embedding `cd /d "path"` in the command string - Improve dependency check to detect broken symlinks and empty node_modules directories Co-Authored-By: Claude Opus 4.5 <[email protected]>
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - Merge conflicts must be resolved before merge.
Blocked: PR has merge conflicts with base branch. Resolve conflicts before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Medium | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Merge Conflicts: PR has conflicts with base branch that must be resolved
Findings Summary
- High: 4 issue(s)
- Medium: 6 issue(s)
Generated by Auto Claude PR Review
Findings (10 selected of 10 total)
🟠 [42c32f65a2e2] [HIGH] IPC handler accepts arbitrary worktreePath without validation
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2958
The TASK_WORKTREE_INSTALL_DEPS handler accepts worktreePath directly from the renderer process and only checks if the path exists. It does not validate that this path belongs to a legitimate project worktree. This allows a compromised renderer to run package manager install commands in any directory, potentially executing malicious postinstall scripts.
Suggested fix:
Validate that worktreePath is a legitimate worktree by deriving it from a taskId (like other handlers do) rather than accepting it as a raw parameter. Use findTaskWorktree() to validate the path belongs to a known project.
🟠 [7a0a62563a0c] [HIGH] Launch app IPC handler accepts unvalidated worktreePath
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:3001
The TASK_WORKTREE_LAUNCH_APP handler accepts worktreePath directly from renderer without validating it belongs to a legitimate project worktree. An attacker could point this to any directory containing a malicious package.json with a crafted dev script, executing arbitrary commands.
Suggested fix:
Accept taskId instead of raw worktreePath, then derive the worktree path internally using findTaskWorktree(). This follows the pattern of other secure handlers in this file.
🟠 [37724db7714c] [HIGH] Arbitrary process killing via pgrep -f on Unix
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2926
killWorktreeProcesses() on Unix uses 'pgrep -f worktreePath' where worktreePath comes from IPC without validation. The -f flag matches against full command line as a substring. An attacker could pass a pattern matching critical system processes or paths like '/home/user/project' would match '/home/user/project-production'.
Suggested fix:
Validate worktreePath against known worktrees before killing. Use more precise matching like checking process working directory instead of command line substring matching.
🟠 [f76d4f83f8a0] [HIGH] PowerShell wildcard injection in process killing
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2900
On Windows, killWorktreeProcesses() uses PowerShell's -like operator with the worktree path. While backslashes are escaped, PowerShell wildcard characters like [, ], ?, * are not escaped. A path containing these could match and kill unintended processes.
Suggested fix:
Escape PowerShell wildcard characters ([, ], ?, *) in the path, or use -eq with exact path matching instead of -like with wildcards.
🟡 [f4bd33c0a4f1] [MEDIUM] Arbitrary threshold for dependency validation causes false negatives
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:3033
The check 'contents.length > 5' to determine if dependencies are installed is arbitrary. A minimal project with exactly 5 dependencies would incorrectly report deps as not installed since 5 > 5 is false.
Suggested fix:
Change to contents.length >= 1 (any content means something was installed) or check for specific required dependencies from package.json, or at minimum use contents.length > 0.
🟡 [f26a73c7da95] [MEDIUM] Process termination delay may be insufficient
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2950
After sending kill signals, there's a 500ms delay. However, kill signals are sent via detached spawn processes which run asynchronously. The 500ms doesn't wait for those commands to complete. On heavily loaded systems, processes may not have terminated, leading to port conflicts.
Suggested fix:
After sending kill signals, poll for process existence until processes are confirmed dead or a longer timeout expires.
🟡 [424a3a9107a3] [MEDIUM] Hardcoded English strings violate i18n guidelines
📁 apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx:127
Multiple toast messages use hardcoded English text instead of i18n translation keys. Found: 'Installing dependencies...', 'Dependencies installed', 'Installation failed', 'Dependencies not installed', 'Install & Launch', 'Installing...'. Per CLAUDE.md, all user-facing text must use i18n translation keys.
Suggested fix:
Add translation keys to taskReview.json: { "dependencies": { "installing": "Installing dependencies...", "installed": "Dependencies installed", etc. } } Then use: t('taskReview:dependencies.installing')
🟡 [98b34b1fa8a2] [MEDIUM] Duplicate validation logic between escape functions
📁 apps/frontend/src/main/ipc-handlers/task/shell-escape.ts:17
escapePathForShell() (lines 17-25) and escapePathForAppleScript() (lines 52-59) contain identical null byte and newline validation logic. Changes to validation rules would need to be made in both places.
Suggested fix:
Extract common validation to a shared helper function: function validatePathSafety(filePath: string): boolean { ... }
🟡 [936cd6740123] [MEDIUM] Utility functions should be extracted to separate module
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2829
worktree-handlers.ts already exceeds 3500+ lines. This PR adds 4 new utility functions inside registerWorktreeHandlers: detectPackageManager, runCommandAsync, commandExistsAsync, killWorktreeProcesses. These are general-purpose utilities that could be reused and tested independently.
Suggested fix:
Extract to separate module apps/frontend/src/main/utils/process-utils.ts. This enables unit testing and improves code organization.
🟡 [294c0b576c5f] [MEDIUM] devCommand not escaped for AppleScript context on macOS
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:3141
The devCommand (e.g., 'npm run dev') is interpolated directly into an AppleScript string without escaping. While devCommand is constructed from package manager names and script names, a malicious package.json with a script name containing AppleScript escape sequences could inject commands.
Suggested fix:
Validate that devCommand only contains safe characters (alphanumeric, space, dash) or escape it for AppleScript context.
This review was generated by Auto Claude.
2 participants
Summary
Complete implementation of dependency handling for the Launch App button (Levels 1-5):
node_modulesexists before launchingAdditional fixes in latest commits:
start /d pathfor reliable directory settingProblem
When clicking "Launch App" on a worktree where dependencies haven't been installed, users get cryptic errors like
'npm-run-all' is not recognizedwith no indication of what to do.Solution
Backend Changes
detectPackageManager()to identify pnpm/yarn/bun/npm from lockfilesTASK_WORKTREE_INSTALL_DEPSIPC handler for installing dependencieskillWorktreeProcesses()to clean up stale dev servers before launchTASK_WORKTREE_LAUNCH_APPto check deps and supportautoInstallparameterdepsInstalled,packageManagerinfopnpm run devvsnpm run dev)Frontend Changes
API Changes
worktreeLaunchAppnow accepts optionalautoInstallparameterworktreeLaunchAppreturnsdepsInstalled,packageManagerin responseworktreeInstallDepsAPI for standalone dependency installationSupersedes
This PR supersedes:
Test plan
Known Limitations (Future Work)
This PR is Node.js-centric. Future improvements could include:
🤖 Generated with Claude Code