Skip to content

Commit 51d4906

Browse files
7418claude
andcommitted
fix: address Codex review — Node auto-install, first-launch prompt, IPC consistency
P1: When Node.js is missing, auto-install via brew (macOS) or winget (Windows) instead of stopping and asking user to install manually. P1: Auto-open install wizard on first disconnect detection in Electron, with localStorage flag to avoid re-prompting after dismissal. P2: IPC install:start now throws on concurrent calls instead of returning silent { error } object. Removed unused return values from handlers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent e4b9171 commit 51d4906

File tree

4 files changed

+139
-58
lines changed

4 files changed

+139
-58
lines changed

electron/main.ts

Lines changed: 93 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -418,20 +418,28 @@ app.whenReady().then(async () => {
418418
return { hasNode, nodeVersion, hasClaude, claudeVersion };
419419
});
420420

421-
ipcMain.handle('install:start', () => {
421+
ipcMain.handle('install:start', (_event: Electron.IpcMainInvokeEvent, options?: { includeNode?: boolean }) => {
422422
if (installState.status === 'running') {
423-
return { error: 'Installation is already running' };
423+
throw new Error('Installation is already running');
424424
}
425425

426+
const needsNode = options?.includeNode === true;
427+
426428
// Reset state
429+
const steps: InstallStep[] = [];
430+
if (needsNode) {
431+
steps.push({ id: 'install-node', label: 'Installing Node.js', status: 'pending' });
432+
}
433+
steps.push(
434+
{ id: 'check-node', label: 'Checking Node.js', status: 'pending' },
435+
{ id: 'install-claude', label: 'Installing Claude Code', status: 'pending' },
436+
{ id: 'verify', label: 'Verifying installation', status: 'pending' },
437+
);
438+
427439
installState = {
428440
status: 'running',
429441
currentStep: null,
430-
steps: [
431-
{ id: 'check-node', label: 'Checking Node.js', status: 'pending' },
432-
{ id: 'install-claude', label: 'Installing Claude Code', status: 'pending' },
433-
{ id: 'verify', label: 'Verifying installation', status: 'pending' },
434-
],
442+
steps,
435443
logs: [],
436444
};
437445

@@ -465,6 +473,83 @@ app.whenReady().then(async () => {
465473
// Run the installation sequence asynchronously
466474
(async () => {
467475
try {
476+
// Step 0 (optional): Install Node.js via package manager
477+
if (needsNode) {
478+
setStep('install-node', 'running');
479+
480+
const nodeInstalled = await new Promise<boolean>((resolve) => {
481+
const isWin = process.platform === 'win32';
482+
const isMac = process.platform === 'darwin';
483+
let cmd: string;
484+
let args: string[];
485+
486+
if (isMac) {
487+
// Try Homebrew
488+
const brewPaths = ['/opt/homebrew/bin/brew', '/usr/local/bin/brew'];
489+
const brewPath = brewPaths.find(p => fs.existsSync(p));
490+
if (!brewPath) {
491+
addLog('Homebrew not found. Cannot auto-install Node.js on macOS without Homebrew.');
492+
resolve(false);
493+
return;
494+
}
495+
cmd = brewPath;
496+
args = ['install', 'node'];
497+
addLog(`Running: ${brewPath} install node`);
498+
} else if (isWin) {
499+
cmd = 'winget';
500+
args = ['install', '-e', '--id', 'OpenJS.NodeJS.LTS', '--accept-source-agreements', '--accept-package-agreements'];
501+
addLog('Running: winget install -e --id OpenJS.NodeJS.LTS');
502+
} else {
503+
// Linux — no universal package manager
504+
addLog('Auto-install of Node.js is not supported on this platform.');
505+
resolve(false);
506+
return;
507+
}
508+
509+
const child = spawn(cmd, args, {
510+
env: execEnv,
511+
shell: isWin,
512+
stdio: ['ignore', 'pipe', 'pipe'],
513+
});
514+
515+
installProcess = child;
516+
517+
child.stdout?.on('data', (data: Buffer) => {
518+
for (const line of data.toString().split('\n').filter(Boolean)) {
519+
addLog(line);
520+
}
521+
});
522+
child.stderr?.on('data', (data: Buffer) => {
523+
for (const line of data.toString().split('\n').filter(Boolean)) {
524+
addLog(line);
525+
}
526+
});
527+
child.on('error', (err) => {
528+
addLog(`Error: ${err.message}`);
529+
resolve(false);
530+
});
531+
child.on('close', (code) => {
532+
installProcess = null;
533+
resolve(code === 0);
534+
});
535+
});
536+
537+
if (installState.status === 'cancelled') {
538+
setStep('install-node', 'failed', 'Cancelled');
539+
return;
540+
}
541+
542+
if (!nodeInstalled) {
543+
setStep('install-node', 'failed', 'Could not auto-install Node.js.');
544+
installState.status = 'failed';
545+
sendProgress();
546+
return;
547+
}
548+
549+
setStep('install-node', 'success');
550+
addLog('Node.js installation completed.');
551+
}
552+
468553
// Step 1: Check node
469554
setStep('check-node', 'running');
470555
try {
@@ -576,13 +661,11 @@ app.whenReady().then(async () => {
576661
sendProgress();
577662
}
578663
})();
579-
580-
return { ok: true };
581664
});
582665

583666
ipcMain.handle('install:cancel', () => {
584667
if (installState.status !== 'running') {
585-
return { error: 'No installation is running' };
668+
return;
586669
}
587670

588671
installState.status = 'cancelled';
@@ -597,7 +680,6 @@ app.whenReady().then(async () => {
597680
}
598681

599682
mainWindow?.webContents.send('install:progress', installState);
600-
return { ok: true };
601683
});
602684

603685
ipcMain.handle('install:get-logs', () => {

electron/preload.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ contextBridge.exposeInMainWorld('electronAPI', {
99
},
1010
install: {
1111
checkPrerequisites: () => ipcRenderer.invoke('install:check-prerequisites'),
12-
start: () => ipcRenderer.invoke('install:start'),
12+
start: (options?: { includeNode?: boolean }) => ipcRenderer.invoke('install:start', options),
1313
cancel: () => ipcRenderer.invoke('install:cancel'),
1414
getLogs: () => ipcRenderer.invoke('install:get-logs'),
1515
onProgress: (callback: (data: unknown) => void) => {

src/components/layout/ConnectionStatus.tsx

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ export function ConnectionStatus() {
3434
const stableCountRef = useRef(0);
3535
const lastConnectedRef = useRef<boolean | null>(null);
3636
const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
37+
const autoPromptedRef = useRef(false);
3738

3839
// Use a ref-based approach to avoid circular deps between check and schedule
3940
const checkRef = useRef<() => void>(() => {});
@@ -87,6 +88,32 @@ export function ConnectionStatus() {
8788
checkStatus();
8889
}, [checkStatus]);
8990

91+
// Auto-prompt install wizard on first disconnect detection (Electron only)
92+
useEffect(() => {
93+
if (
94+
status !== null &&
95+
!status.connected &&
96+
isElectron &&
97+
!autoPromptedRef.current &&
98+
!dialogOpen &&
99+
!wizardOpen
100+
) {
101+
const dismissed = localStorage.getItem("codepilot:install-wizard-dismissed");
102+
if (!dismissed) {
103+
autoPromptedRef.current = true;
104+
setWizardOpen(true); // eslint-disable-line react-hooks/set-state-in-effect -- intentional: auto-prompt on first disconnect
105+
}
106+
}
107+
}, [status, isElectron, dialogOpen, wizardOpen]);
108+
109+
const handleWizardOpenChange = useCallback((open: boolean) => {
110+
setWizardOpen(open);
111+
if (!open) {
112+
// Remember that user dismissed the wizard so we don't auto-prompt again
113+
localStorage.setItem("codepilot:install-wizard-dismissed", "1");
114+
}
115+
}, []);
116+
90117
const connected = status?.connected ?? false;
91118

92119
return (
@@ -199,7 +226,7 @@ export function ConnectionStatus() {
199226

200227
<InstallWizard
201228
open={wizardOpen}
202-
onOpenChange={setWizardOpen}
229+
onOpenChange={handleWizardOpenChange}
203230
onInstallComplete={handleManualRefresh}
204231
/>
205232
</>

src/components/layout/InstallWizard.tsx

Lines changed: 17 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ interface InstallWizardProps {
4040

4141
type WizardPhase =
4242
| "checking"
43-
| "node-missing"
4443
| "already-installed"
4544
| "installing"
4645
| "success"
@@ -57,7 +56,7 @@ function getInstallAPI() {
5756
hasClaude: boolean;
5857
claudeVersion?: string;
5958
}>;
60-
start: () => Promise<void>;
59+
start: (options?: { includeNode?: boolean }) => Promise<void>;
6160
cancel: () => Promise<void>;
6261
getLogs: () => Promise<string[]>;
6362
onProgress: (
@@ -104,7 +103,7 @@ export function InstallWizard({
104103
scrollToBottom();
105104
}, [logs, scrollToBottom]);
106105

107-
const startInstall = useCallback(async () => {
106+
const startInstall = useCallback(async (options?: { includeNode?: boolean }) => {
108107
const api = getInstallAPI();
109108
if (!api) return;
110109

@@ -124,7 +123,7 @@ export function InstallWizard({
124123
});
125124

126125
try {
127-
await api.start();
126+
await api.start(options);
128127
} catch (err: unknown) {
129128
setPhase("failed");
130129
const msg = err instanceof Error ? err.message : String(err);
@@ -143,31 +142,30 @@ export function InstallWizard({
143142
try {
144143
const result = await api.checkPrerequisites();
145144

146-
if (!result.hasNode) {
147-
setPhase("node-missing");
145+
if (result.hasClaude) {
148146
setLogs((prev) => [
149147
...prev,
150-
"Node.js not found.",
148+
`Node.js ${result.nodeVersion} found.`,
149+
`Claude Code ${result.claudeVersion} already installed.`,
151150
]);
151+
setPhase("already-installed");
152152
return;
153153
}
154154

155-
setLogs((prev) => [
156-
...prev,
157-
`Node.js ${result.nodeVersion} found.`,
158-
]);
159-
160-
if (result.hasClaude) {
161-
setPhase("already-installed");
155+
if (!result.hasNode) {
162156
setLogs((prev) => [
163157
...prev,
164-
`Claude Code ${result.claudeVersion} already installed.`,
158+
"Node.js not found. Will attempt to install Node.js and Claude Code...",
165159
]);
166-
return;
160+
startInstall({ includeNode: true });
161+
} else {
162+
setLogs((prev) => [
163+
...prev,
164+
`Node.js ${result.nodeVersion} found.`,
165+
"Claude Code not found. Starting installation...",
166+
]);
167+
startInstall();
167168
}
168-
169-
setLogs((prev) => [...prev, "Claude Code not found. Starting installation..."]);
170-
startInstall();
171169
} catch (err: unknown) {
172170
setPhase("failed");
173171
const msg = err instanceof Error ? err.message : String(err);
@@ -268,26 +266,6 @@ export function InstallWizard({
268266
</div>
269267
)}
270268

271-
{phase === "node-missing" && (
272-
<div className="rounded-lg bg-red-500/10 px-4 py-3 text-sm space-y-2">
273-
<p className="font-medium text-red-700 dark:text-red-400">
274-
Node.js 18+ is required
275-
</p>
276-
<p className="text-muted-foreground">
277-
Please install it from{" "}
278-
<a
279-
href="https://nodejs.org"
280-
target="_blank"
281-
rel="noopener noreferrer"
282-
className="underline text-blue-600 dark:text-blue-400"
283-
>
284-
https://nodejs.org
285-
</a>{" "}
286-
and try again.
287-
</p>
288-
</div>
289-
)}
290-
291269
{phase === "already-installed" && (
292270
<div className="flex items-center gap-3 rounded-lg bg-emerald-500/10 px-4 py-3">
293271
<CheckIcon className="size-5 text-emerald-500 shrink-0" />
@@ -342,12 +320,6 @@ export function InstallWizard({
342320
{copied ? "Copied" : "Copy Logs"}
343321
</Button>
344322

345-
{phase === "node-missing" && (
346-
<Button size="sm" onClick={checkPrereqs}>
347-
Retry
348-
</Button>
349-
)}
350-
351323
{phase === "installing" && (
352324
<Button variant="destructive" size="sm" onClick={handleCancel}>
353325
Cancel

0 commit comments

Comments
 (0)