Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v1.0, chore, ui] close processes when window closes #584

Closed
wants to merge 5 commits into from
+14 −12
Closed

[v1.0, chore, ui] close processes when window closes #584

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a48c66b
close processes when window closes
michaelneale Jan 14, 2025
0689f5b
moving process around
michaelneale Jan 14, 2025
5f96afe
updating
michaelneale Jan 14, 2025
b73ab5b
adding back in check
michaelneale Jan 14, 2025
6c906a4
add window back in
michaelneale Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions ui/desktop/src/goosed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { createServer } from 'net';
import os from 'node:os';
import { getBinaryPath } from './utils/binaryPath';
import log from './utils/logger';
import { ChildProcessByStdio } from 'node:child_process';
import { Readable } from 'node:stream';

// Find an available port to start goosed on
export const findAvailablePort = (): Promise<number> => {
Expand Down Expand Up @@ -58,7 +60,7 @@ const checkServerStatus = async (port: number, maxAttempts: number = 60, interva
return false;
};

export const startGoosed = async (app, dir=null, env={}): Promise<[number, string, string]> => {
export const startGoosed = async (app, dir=null, env={}): Promise<[number, string, ChildProcessByStdio<null, Readable, Readable>]> => {
// In will use this later to determine if we should start process
const isDev = process.env.NODE_ENV === 'development';

Expand All @@ -71,7 +73,7 @@ export const startGoosed = async (app, dir=null, env={}): Promise<[number, strin
// Skip starting goosed if configured in dev mode
if (isDev && !app.isPackaged && process.env.VITE_START_EMBEDDED_SERVER === 'no') {
log.info('Skipping starting goosed in development mode');
return [3000, dir, 'dev'];
return [3000, dir, null];
}

// Get the goosed binary path using the shared utility
Expand Down Expand Up @@ -137,10 +139,6 @@ export const startGoosed = async (app, dir=null, env={}): Promise<[number, strin
goosedProcess.kill();
});

// Wait for the server to start and fetch the agent version
await new Promise(resolve => setTimeout(resolve, 1000)); // Give the server time to start
const agentVersion = await fetchAgentVersion(port);

log.info(`Goosed server successfully started on port ${port}`);
return [port, dir, agentVersion];
return [port, dir, goosedProcess];
};
46 changes: 9 additions & 37 deletions ui/desktop/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,11 @@ const createChat = async (app, query?: string, dir?: string, version?: string) =
if (checkApiCredentials()) {
return startGoosed(app, dir);
} else {
return [0, '', ''];
return [0, '', null];
}
}

const [port, working_dir, agentVersion] = await maybeStartGoosed();
const [port, working_dir, goosedProcess] = await maybeStartGoosed();

const mainWindow = new BrowserWindow({
titleBarStyle: 'hidden',
Expand All @@ -152,7 +152,6 @@ const createChat = async (app, query?: string, dir?: string, version?: string) =
...appConfig,
GOOSE_SERVER__PORT: port,
GOOSE_WORKING_DIR: working_dir,
GOOSE_AGENT_VERSION: agentVersion,
REQUEST_DIR: dir
})],
},
Expand Down Expand Up @@ -192,6 +191,13 @@ const createChat = async (app, query?: string, dir?: string, version?: string) =
mainWindow.on('closed', () => {
windowMap.delete(windowId);
});

mainWindow.on('closed', () => {
if (goosedProcess) {
goosedProcess?.kill();
}
});

};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't this be t same block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably as I was tired abnd missed it!


const createTray = () => {
Expand All @@ -207,7 +213,6 @@ const createTray = () => {
const tray = new Tray(iconPath);

const contextMenu = Menu.buildFromTemplate([
{ label: 'Show Window', click: showWindow },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we dropping this option. Wouldn't the remaining context menu just have a separator and then the Quit item?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - but I don't think it did anything? (it was a relic from wing to wing I think?) - but could check that out again

{ type: 'separator' },
{ label: 'Quit', click: () => app.quit() }
]);
Expand All @@ -216,39 +221,6 @@ const createTray = () => {
tray.setContextMenu(contextMenu);
};

const showWindow = () => {
const windows = BrowserWindow.getAllWindows();

if (windows.length === 0) {
log.info("No windows are currently open.");
return;
}

// Define the initial offset values
const initialOffsetX = 30;
const initialOffsetY = 30;

// Iterate over all windows
windows.forEach((win, index) => {
const currentBounds = win.getBounds();
const newX = currentBounds.x + initialOffsetX * index;
const newY = currentBounds.y + initialOffsetY * index;

win.setBounds({
x: newX,
y: newY,
width: currentBounds.width,
height: currentBounds.height,
});

if (!win.isVisible()) {
win.show();
}

win.focus();
});
};

const buildRecentFilesMenu = () => {
const recentDirs = loadRecentDirs();
return recentDirs.map(dir => ({
Expand Down
Loading