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>
This commit is contained in:
1 parent
e4b917172c
commit
51d4906c77
4 files changed
+141
-60
No files matched your search
+93
-11
@@ -418,20 +418,28 @@ app.whenReady().then(async () => {
|
||||
return { hasNode, nodeVersion, hasClaude, claudeVersion };
|
||||
});
|
||||
|
||||
ipcMain.handle('install:start', () => {
|
||||
ipcMain.handle('install:start', (_event: Electron.IpcMainInvokeEvent, options?: { includeNode?: boolean }) => {
|
||||
if (installState.status === 'running') {
|
||||
return { error: 'Installation is already running' };
|
||||
throw new Error('Installation is already running');
|
||||
}
|
||||
|
||||
const needsNode = options?.includeNode === true;
|
||||
|
||||
// Reset state
|
||||
const steps: InstallStep[] = [];
|
||||
if (needsNode) {
|
||||
steps.push({ id: 'install-node', label: 'Installing Node.js', status: 'pending' });
|
||||
}
|
||||
steps.push(
|
||||
{ id: 'check-node', label: 'Checking Node.js', status: 'pending' },
|
||||
{ id: 'install-claude', label: 'Installing Claude Code', status: 'pending' },
|
||||
{ id: 'verify', label: 'Verifying installation', status: 'pending' },
|
||||
);
|
||||
|
||||
installState = {
|
||||
status: 'running',
|
||||
currentStep: null,
|
||||
steps: [
|
||||
{ id: 'check-node', label: 'Checking Node.js', status: 'pending' },
|
||||
{ id: 'install-claude', label: 'Installing Claude Code', status: 'pending' },
|
||||
{ id: 'verify', label: 'Verifying installation', status: 'pending' },
|
||||
],
|
||||
steps,
|
||||
logs: [],
|
||||
};
|
||||
|
||||
@@ -465,6 +473,83 @@ app.whenReady().then(async () => {
|
||||
// Run the installation sequence asynchronously
|
||||
(async () => {
|
||||
try {
|
||||
// Step 0 (optional): Install Node.js via package manager
|
||||
if (needsNode) {
|
||||
setStep('install-node', 'running');
|
||||
|
||||
const nodeInstalled = await new Promise<boolean>((resolve) => {
|
||||
const isWin = process.platform === 'win32';
|
||||
const isMac = process.platform === 'darwin';
|
||||
let cmd: string;
|
||||
let args: string[];
|
||||
|
||||
if (isMac) {
|
||||
// Try Homebrew
|
||||
const brewPaths = ['/opt/homebrew/bin/brew', '/usr/local/bin/brew'];
|
||||
const brewPath = brewPaths.find(p => fs.existsSync(p));
|
||||
if (!brewPath) {
|
||||
addLog('Homebrew not found. Cannot auto-install Node.js on macOS without Homebrew.');
|
||||
resolve(false);
|
||||
return;
|
||||
}
|
||||
cmd = brewPath;
|
||||
args = ['install', 'node'];
|
||||
addLog(`Running: ${brewPath} install node`);
|
||||
} else if (isWin) {
|
||||
cmd = 'winget';
|
||||
args = ['install', '-e', '--id', 'OpenJS.NodeJS.LTS', '--accept-source-agreements', '--accept-package-agreements'];
|
||||
addLog('Running: winget install -e --id OpenJS.NodeJS.LTS');
|
||||
} else {
|
||||
// Linux — no universal package manager
|
||||
addLog('Auto-install of Node.js is not supported on this platform.');
|
||||
resolve(false);
|
||||
return;
|
||||
}
|
||||
|
||||
const child = spawn(cmd, args, {
|
||||
env: execEnv,
|
||||
shell: isWin,
|
||||
stdio: ['ignore', 'pipe', 'pipe'],
|
||||
});
|
||||
|
||||
installProcess = child;
|
||||
|
||||
child.stdout?.on('data', (data: Buffer) => {
|
||||
for (const line of data.toString().split('\n').filter(Boolean)) {
|
||||
addLog(line);
|
||||
}
|
||||
});
|
||||
child.stderr?.on('data', (data: Buffer) => {
|
||||
for (const line of data.toString().split('\n').filter(Boolean)) {
|
||||
addLog(line);
|
||||
}
|
||||
});
|
||||
child.on('error', (err) => {
|
||||
addLog(`Error: ${err.message}`);
|
||||
resolve(false);
|
||||
});
|
||||
child.on('close', (code) => {
|
||||
installProcess = null;
|
||||
resolve(code === 0);
|
||||
});
|
||||
});
|
||||
|
||||
if (installState.status === 'cancelled') {
|
||||
setStep('install-node', 'failed', 'Cancelled');
|
||||
return;
|
||||
}
|
||||
|
||||
if (!nodeInstalled) {
|
||||
setStep('install-node', 'failed', 'Could not auto-install Node.js.');
|
||||
installState.status = 'failed';
|
||||
sendProgress();
|
||||
return;
|
||||
}
|
||||
|
||||
setStep('install-node', 'success');
|
||||
addLog('Node.js installation completed.');
|
||||
}
|
||||
|
||||
// Step 1: Check node
|
||||
setStep('check-node', 'running');
|
||||
try {
|
||||
@@ -576,13 +661,11 @@ app.whenReady().then(async () => {
|
||||
sendProgress();
|
||||
}
|
||||
})();
|
||||
|
||||
return { ok: true };
|
||||
});
|
||||
|
||||
ipcMain.handle('install:cancel', () => {
|
||||
if (installState.status !== 'running') {
|
||||
return { error: 'No installation is running' };
|
||||
return;
|
||||
}
|
||||
|
||||
installState.status = 'cancelled';
|
||||
@@ -597,7 +680,6 @@ app.whenReady().then(async () => {
|
||||
}
|
||||
|
||||
mainWindow?.webContents.send('install:progress', installState);
|
||||
return { ok: true };
|
||||
});
|
||||
|
||||
ipcMain.handle('install:get-logs', () => {
|
||||
|
||||
+1
-1
@@ -9,7 +9,7 @@ contextBridge.exposeInMainWorld('electronAPI', {
|
||||
},
|
||||
install: {
|
||||
checkPrerequisites: () => ipcRenderer.invoke('install:check-prerequisites'),
|
||||
start: () => ipcRenderer.invoke('install:start'),
|
||||
start: (options?: { includeNode?: boolean }) => ipcRenderer.invoke('install:start', options),
|
||||
cancel: () => ipcRenderer.invoke('install:cancel'),
|
||||
getLogs: () => ipcRenderer.invoke('install:get-logs'),
|
||||
onProgress: (callback: (data: unknown) => void) => {
|
||||
|
||||
@@ -34,6 +34,7 @@ export function ConnectionStatus() {
|
||||
const stableCountRef = useRef(0);
|
||||
const lastConnectedRef = useRef<boolean | null>(null);
|
||||
const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||
const autoPromptedRef = useRef(false);
|
||||
|
||||
// Use a ref-based approach to avoid circular deps between check and schedule
|
||||
const checkRef = useRef<() => void>(() => {});
|
||||
@@ -87,6 +88,32 @@ export function ConnectionStatus() {
|
||||
checkStatus();
|
||||
}, [checkStatus]);
|
||||
|
||||
// Auto-prompt install wizard on first disconnect detection (Electron only)
|
||||
useEffect(() => {
|
||||
if (
|
||||
status !== null &&
|
||||
!status.connected &&
|
||||
isElectron &&
|
||||
!autoPromptedRef.current &&
|
||||
!dialogOpen &&
|
||||
!wizardOpen
|
||||
) {
|
||||
const dismissed = localStorage.getItem("codepilot:install-wizard-dismissed");
|
||||
if (!dismissed) {
|
||||
autoPromptedRef.current = true;
|
||||
setWizardOpen(true); // eslint-disable-line react-hooks/set-state-in-effect -- intentional: auto-prompt on first disconnect
|
||||
}
|
||||
}
|
||||
}, [status, isElectron, dialogOpen, wizardOpen]);
|
||||
|
||||
const handleWizardOpenChange = useCallback((open: boolean) => {
|
||||
setWizardOpen(open);
|
||||
if (!open) {
|
||||
// Remember that user dismissed the wizard so we don't auto-prompt again
|
||||
localStorage.setItem("codepilot:install-wizard-dismissed", "1");
|
||||
}
|
||||
}, []);
|
||||
|
||||
const connected = status?.connected ?? false;
|
||||
|
||||
return (
|
||||
@@ -199,7 +226,7 @@ export function ConnectionStatus() {
|
||||
|
||||
<InstallWizard
|
||||
open={wizardOpen}
|
||||
onOpenChange={setWizardOpen}
|
||||
onOpenChange={handleWizardOpenChange}
|
||||
onInstallComplete={handleManualRefresh}
|
||||
/>
|
||||
</>
|
||||
|
||||
@@ -40,7 +40,6 @@ interface InstallWizardProps {
|
||||
|
||||
type WizardPhase =
|
||||
| "checking"
|
||||
| "node-missing"
|
||||
| "already-installed"
|
||||
| "installing"
|
||||
| "success"
|
||||
@@ -57,7 +56,7 @@ function getInstallAPI() {
|
||||
hasClaude: boolean;
|
||||
claudeVersion?: string;
|
||||
}>;
|
||||
start: () => Promise<void>;
|
||||
start: (options?: { includeNode?: boolean }) => Promise<void>;
|
||||
cancel: () => Promise<void>;
|
||||
getLogs: () => Promise<string[]>;
|
||||
onProgress: (
|
||||
@@ -104,7 +103,7 @@ export function InstallWizard({
|
||||
scrollToBottom();
|
||||
}, [logs, scrollToBottom]);
|
||||
|
||||
const startInstall = useCallback(async () => {
|
||||
const startInstall = useCallback(async (options?: { includeNode?: boolean }) => {
|
||||
const api = getInstallAPI();
|
||||
if (!api) return;
|
||||
|
||||
@@ -124,7 +123,7 @@ export function InstallWizard({
|
||||
});
|
||||
|
||||
try {
|
||||
await api.start();
|
||||
await api.start(options);
|
||||
} catch (err: unknown) {
|
||||
setPhase("failed");
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
@@ -143,31 +142,30 @@ export function InstallWizard({
|
||||
try {
|
||||
const result = await api.checkPrerequisites();
|
||||
|
||||
if (!result.hasNode) {
|
||||
setPhase("node-missing");
|
||||
setLogs((prev) => [
|
||||
...prev,
|
||||
"Node.js not found.",
|
||||
]);
|
||||
return;
|
||||
}
|
||||
|
||||
setLogs((prev) => [
|
||||
...prev,
|
||||
`Node.js ${result.nodeVersion} found.`,
|
||||
]);
|
||||
|
||||
if (result.hasClaude) {
|
||||
setPhase("already-installed");
|
||||
setLogs((prev) => [
|
||||
...prev,
|
||||
`Node.js ${result.nodeVersion} found.`,
|
||||
`Claude Code ${result.claudeVersion} already installed.`,
|
||||
]);
|
||||
setPhase("already-installed");
|
||||
return;
|
||||
}
|
||||
|
||||
setLogs((prev) => [...prev, "Claude Code not found. Starting installation..."]);
|
||||
startInstall();
|
||||
if (!result.hasNode) {
|
||||
setLogs((prev) => [
|
||||
...prev,
|
||||
"Node.js not found. Will attempt to install Node.js and Claude Code...",
|
||||
]);
|
||||
startInstall({ includeNode: true });
|
||||
} else {
|
||||
setLogs((prev) => [
|
||||
...prev,
|
||||
`Node.js ${result.nodeVersion} found.`,
|
||||
"Claude Code not found. Starting installation...",
|
||||
]);
|
||||
startInstall();
|
||||
}
|
||||
} catch (err: unknown) {
|
||||
setPhase("failed");
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
@@ -268,26 +266,6 @@ export function InstallWizard({
|
||||
</div>
|
||||
)}
|
||||
|
||||
{phase === "node-missing" && (
|
||||
<div className="rounded-lg bg-red-500/10 px-4 py-3 text-sm space-y-2">
|
||||
<p className="font-medium text-red-700 dark:text-red-400">
|
||||
Node.js 18+ is required
|
||||
</p>
|
||||
<p className="text-muted-foreground">
|
||||
Please install it from{" "}
|
||||
<a
|
||||
href="https://nodejs.org"
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="underline text-blue-600 dark:text-blue-400"
|
||||
>
|
||||
https://nodejs.org
|
||||
</a>{" "}
|
||||
and try again.
|
||||
</p>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{phase === "already-installed" && (
|
||||
<div className="flex items-center gap-3 rounded-lg bg-emerald-500/10 px-4 py-3">
|
||||
<CheckIcon className="size-5 text-emerald-500 shrink-0" />
|
||||
@@ -342,12 +320,6 @@ export function InstallWizard({
|
||||
{copied ? "Copied" : "Copy Logs"}
|
||||
</Button>
|
||||
|
||||
{phase === "node-missing" && (
|
||||
<Button size="sm" onClick={checkPrereqs}>
|
||||
Retry
|
||||
</Button>
|
||||
)}
|
||||
|
||||
{phase === "installing" && (
|
||||
<Button variant="destructive" size="sm" onClick={handleCancel}>
|
||||
Cancel
|
||||
|
||||
Reference in new issue
Block a user