Add pm2 cli commands #2033
Add pm2 cli commands #2033
Conversation
📊 Performance Test ResultsComparing 8da23f3 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
I collected some thoughts and questions as I went over the PR.
The biggest question, though, is whether we take this pm2 approach or something more homegrown. I have some thoughts there, but I'd like to hear yours, too, after working on this change. Let's discuss this in a huddle later 🙂
There was a problem hiding this comment.
Do you think we should reconsider how CLI dependencies are handled now and move everything that only the CLI depends on to this package.json? Or is pm2 still an edge case?
EDIT: I realize now that Vite copies the entire node_modules folder to the build directory, so we should probably NOT move all CLI dependencies here, since that would make the build bundle unnecessarily large.
| const possiblePaths: string[] = [ | ||
| path.join( __dirname, 'node_modules', 'pm2' ), | ||
| path.join( path.dirname( __dirname ), 'node_modules', 'pm2' ), | ||
| path.join( __dirname, '..', 'node_modules', 'pm2' ), | ||
| path.join( __dirname, '..', '..', 'node_modules', 'pm2' ), | ||
| path.join( __dirname, '..', '..', '..', 'node_modules', 'pm2' ), | ||
| path.resolve( process.cwd(), 'node_modules', 'pm2' ), | ||
| ]; |
There was a problem hiding this comment.
Plenty of variation here 🤔 At first, I thought that it would be convenient if we could continue shipping the CLI as a single JS file to be executed by Electron's bundled node.js runtime, but then I realized that the PHP-WASM binaries will need to be handled similarly anyway, so maybe this is something we can live with after all…
| await ensureDaemonRunning(); | ||
|
|
||
| return new Promise( ( resolve, reject ) => { | ||
| const processConfig: pm2.StartOptions = { |
There was a problem hiding this comment.
| const processConfig: pm2.StartOptions = { | |
| const processConfig: Pm2StartOptions = { |
Doing this and adding an import statement like so at the top seems to work.
import { StartOptions as Pm2StartOptions } from 'pm2';
bcotrim
force-pushed
the
add-pm2-cli-commands
branch
from
e4e05b9 to
6d54776
Compare
bcotrim
force-pushed
the
add-pm2-cli-commands
branch
from
6d54776 to
fa322c3
Compare
|
Claude finished @bcotrim's task —— View job Code Review: Add PM2 CLI CommandsI've completed my review of this PR. This introduces PM2-based process management for CLI sites with a persistent proxy daemon. Here's my comprehensive feedback: SummaryThis PR adds PM2 daemon management to enable CLI-based site management with persistent proxy servers for custom domains. The implementation moves proxy server logic from the main Electron process to a PM2-managed CLI daemon that can run independently. Key Changes:
Architecture Concerns1. Code Duplication Between Electron and CLI Proxy ServersIssue: The proxy server logic is now duplicated:
Key Differences:
Recommendation: This duplication creates maintenance burden. Consider:
Reference: cli/lib/proxy-server.ts:1-397 vs src/lib/proxy-server.ts:1-279 2. PM2 Module Resolution ComplexityIssue: The Problems:
Recommendation:
Reference: cli/lib/pm2-manager.ts:6-35 3. Type Safety IssuesIssue: Using const processConfig: pm2.StartOptions = { // Type used but not importedReviewer fredrikekelund already noted this at cli/lib/pm2-manager.ts:286. Fix: import { StartOptions as Pm2StartOptions } from 'pm2';
const processConfig: Pm2StartOptions = {Reference: cli/lib/pm2-manager.ts:161, 286 4. Security: Running Proxy as RootIssue: The proxy daemon explicitly runs as root (cli/lib/pm2-manager.ts:296): uid: 0, // Run as root to bind to ports 80 and 443Concerns:
Recommendations:
Reference: cli/lib/pm2-manager.ts:296 5. Certificate Manager Path HandlingIssue: The CLI certificate manager falls back to calculating appdata path when Concern: If running as root and the env var is NOT set, the fallback will incorrectly use root's home directory ( Recommendation:
Reference: cli/lib/proxy-server.ts:54-75 Code Quality Issues6. Inconsistent Error HandlingCLI proxy-server.ts:
Recommendation:
Reference: cli/lib/proxy-server.ts:110, 349 7. Cache Invalidation LogicIssue: The cache invalidation is overly aggressive (cli/lib/proxy-server.ts:118-121): function invalidateCache() {
sitesCache = [];
lastLoadTime = 0;
}The file watcher calls
Recommendation:
Reference: cli/lib/proxy-server.ts:38, 118-121, 342-346 8. Missing Error Context in PM2 ManagerIssue: Many PM2 operations fail silently or with generic errors: pm2.start( processConfig, ( error, apps ) => {
disconnect();
if ( error ) {
reject( error ); // No context about what was starting
return;
}Recommendation: Wrap errors with context: if ( error ) {
reject( new Error( `Failed to start PM2 process "${options.name}": ${error.message}` ) );
return;
}Reference: cli/lib/pm2-manager.ts:173-178 9. Health Check Endpoint Returns Internal StateIssue: The health check exposes internal state (cli/lib/proxy-server.ts:142-152): {
status: 'ok',
http: isHttpProxyRunning,
https: isHttpsProxyRunning,
timestamp: Date.now(),
}Concern: This endpoint is accessible to any site since it's checked before host validation. Could leak information. Recommendation:
Reference: cli/lib/proxy-server.ts:142-152, 163-164 Testing & Documentation Concerns10. Missing TestsIssue: No tests added for:
Recommendation: Add at least unit tests for:
11. PR Description is EmptyIssue: The PR description has placeholders but no actual content: ## Related issues
- Fixes #
## Proposed Changes
-
## Testing Instructions
-Recommendation: Fill in:
Performance Concerns12. Site Creation RegressionIssue: Performance tests show site creation time increased by 22.6% (16091ms vs 13121ms). Possible causes:
Recommendation: Investigate why site creation is slower and document findings. Build Configuration13. CLI Node Modules Bundling StrategyIssue: Reviewer fredrikekelund asked about moving CLI dependencies to Current approach:
Concern: This is inconsistent with the rest of the app's bundling. Why treat PM2 specially? Questions to answer:
Reference: vite.cli.config.ts:23-34, cli/package.json:10-13 Recommendations SummaryHigh Priority:
Medium Priority: Low Priority: Architecture Documentation UpdatesShould CLAUDE.md be updated? Yes, but after deciding on the approach. This PR introduces a significant new architectural pattern:
However, fredrikekelund's review comment suggests the team is still deciding between "this pm2 approach or something more homegrown." Recommendation:
Questions for DiscussionBased on reviewer feedback:
|
|
Claude finished @bcotrim's task —— View job Code Review Complete ✅I've performed a comprehensive review of this PR that adds PM2-based CLI commands for managing WordPress sites. Below are my findings organized by priority and category. Tasks:
🚨 Critical Issues1. Windows Compatibility is Broken (cli/lib/pm2-manager.ts:296)uid: 0, // Run as root to bind to ports 80 and 443Problem: The Impact: The entire proxy feature will be non-functional on Windows. Recommendation: Use platform detection: const processConfig: pm2.StartOptions = {
name: PROXY_PROCESS_NAME,
script: scriptPath,
// ... other options
...(process.platform !== 'win32' && { uid: 0 }), // Only on Unix-like systems
};2. Windows Admin Privileges Not Implemented (cli/lib/sudo-exec.ts:18-20)// On Windows, we'd need to check for admin rights
// For now, we'll return false and handle Windows separately
// TODO: Implement Windows admin check
return false;Problem: The proxy start command will never work on Windows because it always returns Impact: Windows users cannot use the proxy feature at all. Recommendation: Implement Windows admin detection using if (process.platform === 'win32') {
try {
execSync('net session', { stdio: 'ignore' });
return true;
} catch {
return false;
}
}3. PM2 Socket Path Assumption (cli/lib/pm2-manager.ts:105-108)const homeDir = os.homedir();
const pm2Dir = path.join(homeDir, '.pm2');
const rpcSocket = path.join(pm2Dir, 'rpc.sock');Problem: On Windows, PM2 uses named pipes, not Unix sockets ( Recommendation: Use PM2's built-in connection check or handle platform differences in socket detection.
|
|
Claude Code is working… I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| min_uptime: '10s', | ||
| restart_delay: 3000, | ||
| kill_timeout: 5000, | ||
| uid: 0, // Run as root to bind to ports 80 and 443 |
There was a problem hiding this comment.
🚨 Critical Windows Compatibility Issue
The uid option doesn't work on Windows and will cause PM2 to fail. Windows doesn't use Unix-style UIDs.
Recommendation:
...(process.platform !== 'win32' && { uid: 0 }),On Windows, you'll need a different approach to run with elevated privileges (the process itself needs to be started as Administrator).
|
|
||
| // On Windows, we'd need to check for admin rights | ||
| // For now, we'll return false and handle Windows separately | ||
| // TODO: Implement Windows admin check |
There was a problem hiding this comment.
🚨 Windows Admin Check Not Implemented
This TODO means the proxy feature will never work on Windows. You can implement Windows admin detection like this:
if (process.platform === 'win32') {
try {
const { execSync } = require('child_process');
execSync('net session', { stdio: 'ignore' });
return true;
} catch {
return false;
}
}Or use a library like is-elevated or check for specific Windows admin tokens.
| export function isDaemonRunning(): boolean { | ||
| const homeDir = os.homedir(); | ||
| const pm2Dir = path.join( homeDir, '.pm2' ); | ||
| const rpcSocket = path.join( pm2Dir, 'rpc.sock' ); |
There was a problem hiding this comment.
🚨 Windows Named Pipes Not Handled
On Windows, PM2 uses named pipes instead of Unix sockets. The .sock file won't exist on Windows, so this check will always fail.
Recommendation:
const rpcSocket = process.platform === 'win32'
? path.join(pm2Dir, 'rpc.sock') // On Windows PM2 uses different mechanism
: path.join(pm2Dir, 'rpc.sock');Or better yet, try connecting to PM2 and catch the error rather than checking for socket files.
| /** | ||
| * Gets the site details for a given domain | ||
| */ | ||
| function getSiteByHost( domain: string ): SiteDetails | null { |
There was a problem hiding this comment.
Domain names from the Host header should be validated before use to prevent potential security issues:
function getSiteByHost( domain: string ): SiteDetails | null {
// Validate domain format
if (!domain || !/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*$/.test(domain)) {
return null;
}
try {
const sites = loadSites();
// ... rest of functionThis prevents malicious domains like ../../etc/passwd.local from being processed.
| try { | ||
| return require( 'pm2' ); | ||
| } catch ( error ) { | ||
| const possiblePaths: string[] = [ |
There was a problem hiding this comment.
🤔 Complex Module Resolution
This function tries 6 different paths to find PM2, which suggests the bundling strategy might need refinement. Consider:
- Ensure PM2 is properly included in the build via
vite.cli.config.ts - Or document why this complex resolution is necessary
- Add logging to help debug when PM2 isn't found
As mentioned in fredrikekelund's comment, moving CLI-specific dependencies to cli/package.json may not be ideal due to bundle size implications.
Related issues
Proposed Changes
Testing Instructions
Pre-merge Checklist