feat: supports electron app on desktop#4952
Merged
Soulter merged 24 commits intoAstrBotDevs:masterfrom Feb 8, 2026
Merged
Conversation
dosubot
bot
added
the
size:XL
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
dosubot
bot
added
the
area:webui
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 3 个问题
给 AI Agents 的提示
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `desktop/scripts/build-backend.mjs:31-35` </location>
<code_context>
+ path.join(rootDir, 'main.py'),
+];
+
+const result = spawnSync('uv', args, {
+ cwd: rootDir,
+ stdio: 'inherit',
+ shell: process.platform === 'win32',
+});
+
</code_context>
<issue_to_address>
**suggestion:** `uv` 的调用可以更清晰地报告失败原因，并在非 Windows 平台上更好地处理可执行文件缺失的情况。
目前的错误处理只覆盖到进程启动失败。在非 Windows 平台上，如果 `PATH` 中缺少 `uv`，会以通用的 `ENOENT` 错误表现出来，而不会说明是哪个命令失败。可以考虑记录 `result.error?.message`，让错误信息更清晰，同时检查 `result.status !== 0`，这样在 `uv` / PyInstaller 调用失败时（例如缺少二进制、参数不正确），就可以输出更有信息量的提示，而不是仅仅用状态码退出。
```suggestion
const result = spawnSync('uv', args, {
cwd: rootDir,
stdio: 'inherit',
shell: process.platform === 'win32',
});
if (result.error) {
// Covers failures to spawn the process (e.g. missing executable on non-Windows)
console.error(`Failed to run 'uv': ${result.error.message}`);
process.exitCode = typeof result.status === 'number' ? result.status : 1;
} else if (result.status !== 0) {
// Covers non-zero exit statuses from uv/PyInstaller
console.error(
`'uv' exited with status ${result.status} while running PyInstaller. ` +
'Verify that uv and PyInstaller are installed and that the arguments are valid.'
);
process.exitCode = result.status ?? 1;
}
```
</issue_to_address>
### Comment 2
<location> `desktop/README.md:15` </location>
<code_context>
+
+- Packaging is frontend-only.
+- Backend is not bundled in the installer/package.
+- If backend is unavailable on app startup, the app will show a startup failure dialog and exit.
+
+## Prerequisites
</code_context>
<issue_to_address>
**suggestion (typo):** 为了语句更通顺，你可能想在 "If backend is unavailable" 中加上冠词。
建议表述："If the backend is unavailable on app startup, the app will show a startup failure dialog and exit."
```suggestion
- If the backend is unavailable on app startup, the app will show a startup failure dialog and exit.
```
</issue_to_address>
### Comment 3
<location> `desktop/README.md:56` </location>
<code_context>
+## Notes
+
+- `dist:full` currently runs WebUI build preparation + Electron packaging.
+- If you need backend built-in packaging again, you need to restore backend resource build and packaging config.
</code_context>
<issue_to_address>
**suggestion (typo):** "backend built-in packaging" 这个短语有点别扭，建议为了清晰起见进行改写。
例如："If you need the backend packaged again, restore the backend resource build and packaging configuration."
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的 Review。
Original comment in English
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `desktop/scripts/build-backend.mjs:31-35` </location>
<code_context>
+ path.join(rootDir, 'main.py'),
+];
+
+const result = spawnSync('uv', args, {
+ cwd: rootDir,
+ stdio: 'inherit',
+ shell: process.platform === 'win32',
+});
+
</code_context>
<issue_to_address>
**suggestion:** The `uv` invocation could report failures more clearly and handle missing executables on non-Windows platforms.
Currently the error handling only covers spawn failures, and on non‑Windows platforms a missing `uv` in `PATH` will surface as a generic `ENOENT` without indicating which command failed. Consider logging `result.error?.message` to make this clearer, and also checking `result.status !== 0` so you can emit a more informative message when the `uv`/PyInstaller invocation fails (e.g. missing binary, bad flags) instead of just exiting with the status code.
```suggestion
const result = spawnSync('uv', args, {
cwd: rootDir,
stdio: 'inherit',
shell: process.platform === 'win32',
});
if (result.error) {
// Covers failures to spawn the process (e.g. missing executable on non-Windows)
console.error(`Failed to run 'uv': ${result.error.message}`);
process.exitCode = typeof result.status === 'number' ? result.status : 1;
} else if (result.status !== 0) {
// Covers non-zero exit statuses from uv/PyInstaller
console.error(
`'uv' exited with status ${result.status} while running PyInstaller. ` +
'Verify that uv and PyInstaller are installed and that the arguments are valid.'
);
process.exitCode = result.status ?? 1;
}
```
</issue_to_address>
### Comment 2
<location> `desktop/README.md:15` </location>
<code_context>
+
+- Packaging is frontend-only.
+- Backend is not bundled in the installer/package.
+- If backend is unavailable on app startup, the app will show a startup failure dialog and exit.
+
+## Prerequisites
</code_context>
<issue_to_address>
**suggestion (typo):** You may want to add an article in "If backend is unavailable" for smoother grammar.
Suggested wording: "If the backend is unavailable on app startup, the app will show a startup failure dialog and exit."
```suggestion
- If the backend is unavailable on app startup, the app will show a startup failure dialog and exit.
```
</issue_to_address>
### Comment 3
<location> `desktop/README.md:56` </location>
<code_context>
+## Notes
+
+- `dist:full` currently runs WebUI build preparation + Electron packaging.
+- If you need backend built-in packaging again, you need to restore backend resource build and packaging config.
</code_context>
<issue_to_address>
**suggestion (typo):** The phrase "backend built-in packaging" is a bit awkward; consider rephrasing for clarity.
For example: “If you need the backend packaged again, restore the backend resource build and packaging configuration.”
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dosubot
bot
added
size:XXL
Collaborator
Author
构建方式
运行工作流程
修复的问题
|
- Changed the icon from PNG to SVG format for better scalability. - Updated the border color from #d0d0d0 to #eeeeee for a softer appearance. - Adjusted the width of the startup screen from 460px to 360px for improved responsiveness.
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 4 个问题，并给出了一些整体性的反馈：
- 在
PipInstaller._run_pip_in_process中，从logging.root.handlers中移除所有 handler 的做法非常侵入；建议在修改前先记录快照并在结束后恢复原有 handlers（或者只移除 pip 自己添加的 handler），以避免对全局日志造成意外副作用。 - 新的
desktop/main.js文件体积较大，并且在同一个文件里混合了后端生命周期管理、窗口/托盘管理、IPC 绑定以及启动 UI；建议将其拆分为更聚焦的模块（例如后端管理器、UI/窗口管理器、IPC handler），以便后续改动更易理解和维护。 - Electron shell 中托盘菜单标签、对话框信息以及启动失败信息目前是用英文/中文逻辑硬编码的，而不是通过现有的 i18n 系统；将这些文案集中管理或与 dashboard 的本地化系统集成，会提升一致性并便于未来增加其他语言。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PipInstaller._run_pip_in_process`, removing all handlers from `logging.root.handlers` is very invasive; consider snapshotting and restoring the original handlers (or only removing the handler(s) added by pip) to avoid surprising global logging side effects.
- The new `desktop/main.js` is quite large and mixes backend lifecycle, window/tray management, IPC wiring, and startup UI in a single file; consider splitting this into focused modules (e.g. backend manager, UI/window manager, IPC handlers) to make future changes easier to reason about.
- Tray menu labels, dialog messages, and startup failure texts in the Electron shell are hard-coded in English/Chinese logic instead of going through the existing i18n system; centralizing these strings or integrating with the dashboard’s localization would improve consistency and make future translations easier.
## Individual Comments
### Comment 1
<location> `astrbot/core/utils/pip_installer.py:116-77` </location>
<code_context>
+ await process.wait()
+ return process.returncode
+
+ async def _run_pip_in_process(self, args: list[str]) -> int:
+ pip_main = _get_pip_main()
+ result_code, output = await asyncio.to_thread(
+ _run_pip_main_with_output, pip_main, args
+ )
+ for line in output.splitlines():
+ line = line.strip()
+ if line:
+ logger.info(line)
+
+ for handler in logging.root.handlers[:]:
+ logging.root.removeHandler(handler)
+ return result_code
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid clearing all root logging handlers in `_run_pip_in_process`.
This loop removes all handlers from the root logger every time pip runs in-process. Previously this only happened on a rare fallback path; now `_run_pip_in_process` is used broadly (including frozen runtime), so it will wipe the application’s logging configuration. Instead, only remove handlers that were added for this pip invocation (e.g., track them explicitly), or avoid modifying global logging at all and rely on the captured stdout/stderr for pip output.
</issue_to_address>
### Comment 2
<location> `desktop/main.js:23-32` </location>
<code_context>
+const defaultBackendTimeoutMs = app.isPackaged ? 0 : 20000;
</code_context>
<issue_to_address>
**issue (bug_risk):** Packaged builds can wait indefinitely for backend startup when the process is alive but never becomes reachable.
In packaged mode `defaultBackendTimeoutMs` is `0`, so `startBackendAndWait()` passes `maxWaitMs = 0` into `waitForBackend`. Because the loop only stops when `maxWaitMs > 0 && elapsed >= maxWaitMs`, a zero value means no timeout: it exits only if `pingBackend()` eventually succeeds, or if the process exits and `failOnProcessExit` is `true`.
If the backend process starts but never becomes reachable (e.g., port conflict or hung init), the process stays truthy and the loop never terminates in packaged builds, effectively hanging startup.
Recommend enforcing a finite upper bound even when `backendTimeoutMs` is `0` (e.g., substitute a default) or altering the `maxWaitMs <= 0` behavior to still impose a maximum wait.
</issue_to_address>
### Comment 3
<location> `desktop/README.md:23` </location>
<code_context>
+- Node.js available
+- `pnpm` available
+
+Desktop dependency management uses `pnpm` with lockfile:
+
+- `desktop/pnpm-lock.yaml`
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding an article before "lockfile" for more natural grammar.
For example: "Desktop dependency management uses `pnpm` with a lockfile:" or "...uses `pnpm` with a pnpm lockfile:"
```suggestion
Desktop dependency management uses `pnpm` with a lockfile:
```
</issue_to_address>
### Comment 4
<location> `desktop/main.js:43` </location>
<code_context>
+let tray = null;
+let isQuitting = false;
+let quitInProgress = false;
+let backendProcess = null;
+let backendConfig = null;
+let backendLogFd = null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting backend lifecycle, logging, locale handling, and startup HTML into focused modules (e.g., a BackendManager and helpers) so main.js becomes mostly simple wiring instead of a large, stateful god file.
You can reduce the mental load without changing behavior by extracting a few focused modules and encapsulating the shared state instead of globals. Here are concrete, incremental refactors that preserve functionality.
---
### 1. Encapsulate backend lifecycle into a `backendManager`
Right now `backendProcess`, `backendConfig`, `backendLogFd`, `backendLastExitReason`, `backendStartupFailureReason`, `backendSpawning`, `backendRestarting` and several helpers are global and touched from many places (including IPC). You can keep all logic but centralize it into a small module, and expose a clean API the IPC handlers and `whenReady` can call.
**Example module: `backendManager.js`**
```js
// backendManager.js
'use strict';
const fs = require('fs');
const os = require('os');
const path = require('path');
const { spawn, spawnSync } = require('child_process');
const { app } = require('electron');
const { delay, ensureDir, pingBackend, waitForProcessExit, normalizeUrl } = require('./backendUtils');
const { logElectron } = require('./electronLogger');
const backendUrl = normalizeUrl(
process.env.ASTRBOT_BACKEND_URL || 'http://127.0.0.1:6185/',
);
class BackendManager {
constructor({ backendTimeoutMs }) {
this.backendTimeoutMs = backendTimeoutMs;
this.backendProcess = null;
this.backendConfig = null;
this.backendLogFd = null;
this.lastExitReason = null;
this.startupFailureReason = null;
this.spawning = false;
this.restarting = false;
this.autoStart = process.env.ASTRBOT_BACKEND_AUTO_START !== '0';
}
getUrl() {
return backendUrl;
}
canManage() {
if (!this.backendConfig) {
this.backendConfig = resolveBackendConfig(); // existing logic moved here
}
return Boolean(this.backendConfig?.cmd);
}
async getState() {
const running = await pingBackend(backendUrl);
return {
running,
spawning: this.spawning,
restarting: this.restarting,
canManage: this.canManage(),
};
}
async ensureBackend() {
this.startupFailureReason = null;
if (!this.backendConfig) {
this.backendConfig = resolveBackendConfig();
}
const running = await pingBackend(backendUrl);
if (running) return true;
if (!this.autoStart || !this.backendConfig.cmd) {
this.startupFailureReason =
'Backend auto-start is disabled or backend command is not configured.';
return false;
}
const waitResult = await this.startAndWait(this.backendTimeoutMs);
if (!waitResult.ok) {
this.startupFailureReason = waitResult.reason;
return false;
}
return true;
}
async startAndWait(maxWaitMs) {
if (!this.canManage()) {
return { ok: false, reason: 'Backend command is not configured.' };
}
this.spawning = true;
try {
this.start();
return await waitForBackend(backendUrl, maxWaitMs, () => !this.backendProcess, () => this.lastExitReason); // reuse existing logic
} finally {
this.spawning = false;
}
}
start() {
// move existing startBackend logic here, but use this.* instead of globals
}
async stop() {
// move existing stopBackend logic here, but use this.* instead of globals
}
getStartupFailureReason() {
return this.startupFailureReason;
}
getLastExitReason() {
return this.lastExitReason;
}
}
module.exports = { BackendManager, backendUrl };
```
**Usage in `main`**
```js
// main.js
const { BackendManager, backendUrl } = require('./backendManager');
const backendManager = new BackendManager({ backendTimeoutMs });
// IPC handlers become simple:
ipcMain.handle('astrbot-desktop:get-backend-state', async () => {
return backendManager.getState();
});
ipcMain.handle('astrbot-desktop:restart-backend', async () => {
if (!backendManager.canManage()) {
return { ok: false, reason: 'Backend command is not configured.' };
}
if (backendManager.spawning || backendManager.restarting) {
return { ok: false, reason: 'Backend action already in progress.' };
}
backendManager.restarting = true;
try {
await backendManager.stop();
return await backendManager.startAndWait(backendTimeoutMs);
} finally {
backendManager.restarting = false;
}
});
ipcMain.handle('astrbot-desktop:stop-backend', async () => {
if (!backendManager.canManage()) {
return { ok: false, reason: 'Backend command is not configured.' };
}
// reuse same manager methods instead of duplicating checks
// ...
});
```
This removes duplicated state checks (`backendSpawning`/`backendRestarting`, `backendConfig` resolution) and consolidates lifecycle-oriented behavior into one place without altering semantics.
---
### 2. Move logging helpers to a dedicated module
`logElectron`, `getElectronLogPath`, and `closeBackendLogFd` logic is sprinkled around. Even a tiny `electronLogger.js` improves readability and lets backend and window logic focus on their own concerns.
**`electronLogger.js`**
```js
'use strict';
const fs = require('fs');
const path = require('path');
const { app } = require('electron');
function ensureDir(dir) {
if (!dir) return;
if (fs.existsSync(dir)) return;
fs.mkdirSync(dir, { recursive: true });
}
function getElectronLogPath(rootDirOrConfig) {
const rootDir =
process.env.ASTRBOT_ROOT ||
rootDirOrConfig?.rootDir ||
app.getPath('userData');
return path.join(rootDir, 'logs', 'electron.log');
}
function logElectron(message, { backendConfig } = {}) {
const logPath = getElectronLogPath(backendConfig);
ensureDir(path.dirname(logPath));
const line = `[${new Date().toISOString()}] ${message}\n`;
try {
fs.appendFileSync(logPath, line, 'utf8');
} catch {}
}
module.exports = { logElectron, getElectronLogPath, ensureDir };
```
**Usage in `main.js` / `backendManager.js`**
```js
const { logElectron, ensureDir } = require('./electronLogger');
// replace inline logElectron / ensureDir implementations
```
You can keep backend log FD management in `backendManager`, but this module keeps the electron log plumbing out of the main file.
---
### 3. Extract locale persistence into a `localeStore`
`normalizeLocale`, JSON state file reading/writing, and `persistLocaleFromDashboard` are all coherent but unrelated to tray/window/backend. Moving them out shrinks the main file and makes behavior easier to reason about.
**`localeStore.js`**
```js
'use strict';
const fs = require('fs');
const path = require('path');
const os = require('os');
const { app } = require('electron');
const { delay } = require('./backendUtils');
const LOCALE_STORAGE_KEY = 'astrbot-locale';
const SUPPORTED_STARTUP_LOCALES = new Set(['zh-CN', 'en-US']);
function normalizeLocale(value) {
if (!value) return null;
const raw = String(value).trim();
if (!raw) return null;
if (SUPPORTED_STARTUP_LOCALES.has(raw)) return raw;
const lower = raw.toLowerCase();
if (lower.startsWith('zh')) return 'zh-CN';
if (lower.startsWith('en')) return 'en-US';
return null;
}
function getDesktopStatePath({ backendConfig }) {
const rootDir =
process.env.ASTRBOT_ROOT ||
backendConfig?.rootDir ||
path.join(os.homedir(), '.astrbot') ||
app.getPath('userData');
return path.join(rootDir, 'data', 'desktop_state.json');
}
function readCachedLocale(opts) {
const statePath = getDesktopStatePath(opts);
try {
const raw = fs.readFileSync(statePath, 'utf8');
const parsed = JSON.parse(raw);
return normalizeLocale(parsed?.locale);
} catch {
return null;
}
}
function writeCachedLocale(locale, opts) {
const normalized = normalizeLocale(locale);
if (!normalized) return;
const statePath = getDesktopStatePath(opts);
fs.mkdirSync(path.dirname(statePath), { recursive: true });
try {
fs.writeFileSync(
statePath,
`${JSON.stringify({ locale: normalized }, null, 2)}\n`,
'utf8',
);
} catch {}
}
function resolveStartupLocale(opts) {
const cached = readCachedLocale(opts);
if (cached) return cached;
return normalizeLocale(app.getLocale()) || 'zh-CN';
}
async function persistLocaleFromDashboard(mainWindow, backendUrl, opts, timeoutMs = 1200) {
if (!mainWindow || mainWindow.isDestroyed()) return;
const currentUrl = mainWindow.webContents.getURL();
if (!currentUrl || !currentUrl.startsWith(backendUrl)) return;
try {
const localeRaw = await Promise.race([
mainWindow.webContents.executeJavaScript(
`(() => {
try {
return window.localStorage.getItem('${LOCALE_STORAGE_KEY}') || '';
} catch {
return '';
}
})();`,
true,
),
delay(timeoutMs).then(() => null),
]);
const locale = normalizeLocale(localeRaw);
if (locale) {
writeCachedLocale(locale, opts);
}
} catch {}
}
module.exports = {
normalizeLocale,
resolveStartupLocale,
persistLocaleFromDashboard,
};
```
**Usage in `main.js`**
```js
const { resolveStartupLocale, persistLocaleFromDashboard } = require('./localeStore');
const { backendConfig } = backendManager; // or pass backendManager directly
mainWindow.webContents.on('did-finish-load', () => {
const currentUrl = mainWindow.webContents.getURL();
logElectron(`did-finish-load url=${currentUrl}`);
if (currentUrl.startsWith(backendManager.getUrl())) {
void persistLocaleFromDashboard(mainWindow, backendManager.getUrl(), { backendConfig });
}
});
// in loadStartupScreen
const startupLocale = resolveStartupLocale({ backendConfig });
```
This keeps all locale logic self-contained and lets you change localization behavior without touching the main process boilerplate.
---
### 4. Move the inline startup HTML into a separate helper/module
The inline template string inside `loadStartupScreen` is a major contributor to perceived size. Even if you keep the HTML as a string, moving it out will make the entry file easier to scan.
**`startupScreen.js`**
```js
'use strict';
const fs = require('fs');
const path = require('path');
const { getAssetPath } = require('./assets'); // small helper for assets
const { resolveStartupLocale } = require('./localeStore');
function getStartupTexts(locale) {
if (locale === 'zh-CN') {
return {
title: 'AstrBot 正在启动',
message: '界面很快就会加载完成。',
};
}
return {
title: 'AstrBot is starting',
message: 'The dashboard will be ready in a moment.',
};
}
async function loadStartupScreen(mainWindow, opts) {
if (!mainWindow) return false;
const startupLocale = resolveStartupLocale(opts);
const startupTexts = getStartupTexts(startupLocale);
let iconUrl = '';
try {
const iconBuffer = fs.readFileSync(getAssetPath('icon-no-shadow.svg'));
iconUrl = `data:image/svg+xml;base64,${iconBuffer.toString('base64')}`;
} catch {}
const html = buildStartupHtml(startupTexts, iconUrl);
const startupUrl = `data:text/html;charset=utf-8,${encodeURIComponent(html)}`;
await mainWindow.loadURL(startupUrl);
return true;
}
function buildStartupHtml(startupTexts, iconUrl) {
return `<!doctype html>
<html lang="en">
<head>
<!-- existing CSS/HTML exactly as before -->
</head>
<body>
<div class="card">
${
iconUrl
? `<img class="logo" src="${iconUrl}" alt="AstrBot logo" />`
: '<div class="logo" aria-hidden="true"></div>'
}
<div class="spinner" aria-hidden="true"></div>
<h1>${startupTexts.title}</h1>
<p>${startupTexts.message}</p>
</div>
</body>
</html>`;
}
module.exports = { loadStartupScreen };
```
**Usage in `main.js`**
```js
const { loadStartupScreen } = require('./startupScreen');
app.whenReady().then(async () => {
// ...
await loadStartupScreen(mainWindow, { backendConfig });
// ...
});
```
---
These steps let you:
- Keep all features and behavior.
- Remove globals from the main file by encapsulating them in `BackendManager`.
- Shrink `main.js` to mostly “wire-up” logic (Electron lifecycle + IPC + delegations).
- Localize backend lifecycle, logging, startup HTML, and locale handling to cohesive modules, directly addressing the “too many responsibilities in one file” and “global state soup” concerns without a large rewrite.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- In
PipInstaller._run_pip_in_process, removing all handlers fromlogging.root.handlersis very invasive; consider snapshotting and restoring the original handlers (or only removing the handler(s) added by pip) to avoid surprising global logging side effects. - The new
desktop/main.jsis quite large and mixes backend lifecycle, window/tray management, IPC wiring, and startup UI in a single file; consider splitting this into focused modules (e.g. backend manager, UI/window manager, IPC handlers) to make future changes easier to reason about. - Tray menu labels, dialog messages, and startup failure texts in the Electron shell are hard-coded in English/Chinese logic instead of going through the existing i18n system; centralizing these strings or integrating with the dashboard’s localization would improve consistency and make future translations easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PipInstaller._run_pip_in_process`, removing all handlers from `logging.root.handlers` is very invasive; consider snapshotting and restoring the original handlers (or only removing the handler(s) added by pip) to avoid surprising global logging side effects.
- The new `desktop/main.js` is quite large and mixes backend lifecycle, window/tray management, IPC wiring, and startup UI in a single file; consider splitting this into focused modules (e.g. backend manager, UI/window manager, IPC handlers) to make future changes easier to reason about.
- Tray menu labels, dialog messages, and startup failure texts in the Electron shell are hard-coded in English/Chinese logic instead of going through the existing i18n system; centralizing these strings or integrating with the dashboard’s localization would improve consistency and make future translations easier.
## Individual Comments
### Comment 1
<location> `astrbot/core/utils/pip_installer.py:116-77` </location>
<code_context>
+ await process.wait()
+ return process.returncode
+
+ async def _run_pip_in_process(self, args: list[str]) -> int:
+ pip_main = _get_pip_main()
+ result_code, output = await asyncio.to_thread(
+ _run_pip_main_with_output, pip_main, args
+ )
+ for line in output.splitlines():
+ line = line.strip()
+ if line:
+ logger.info(line)
+
+ for handler in logging.root.handlers[:]:
+ logging.root.removeHandler(handler)
+ return result_code
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid clearing all root logging handlers in `_run_pip_in_process`.
This loop removes all handlers from the root logger every time pip runs in-process. Previously this only happened on a rare fallback path; now `_run_pip_in_process` is used broadly (including frozen runtime), so it will wipe the application’s logging configuration. Instead, only remove handlers that were added for this pip invocation (e.g., track them explicitly), or avoid modifying global logging at all and rely on the captured stdout/stderr for pip output.
</issue_to_address>
### Comment 2
<location> `desktop/main.js:23-32` </location>
<code_context>
+const defaultBackendTimeoutMs = app.isPackaged ? 0 : 20000;
</code_context>
<issue_to_address>
**issue (bug_risk):** Packaged builds can wait indefinitely for backend startup when the process is alive but never becomes reachable.
In packaged mode `defaultBackendTimeoutMs` is `0`, so `startBackendAndWait()` passes `maxWaitMs = 0` into `waitForBackend`. Because the loop only stops when `maxWaitMs > 0 && elapsed >= maxWaitMs`, a zero value means no timeout: it exits only if `pingBackend()` eventually succeeds, or if the process exits and `failOnProcessExit` is `true`.
If the backend process starts but never becomes reachable (e.g., port conflict or hung init), the process stays truthy and the loop never terminates in packaged builds, effectively hanging startup.
Recommend enforcing a finite upper bound even when `backendTimeoutMs` is `0` (e.g., substitute a default) or altering the `maxWaitMs <= 0` behavior to still impose a maximum wait.
</issue_to_address>
### Comment 3
<location> `desktop/README.md:23` </location>
<code_context>
+- Node.js available
+- `pnpm` available
+
+Desktop dependency management uses `pnpm` with lockfile:
+
+- `desktop/pnpm-lock.yaml`
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding an article before "lockfile" for more natural grammar.
For example: "Desktop dependency management uses `pnpm` with a lockfile:" or "...uses `pnpm` with a pnpm lockfile:"
```suggestion
Desktop dependency management uses `pnpm` with a lockfile:
```
</issue_to_address>
### Comment 4
<location> `desktop/main.js:43` </location>
<code_context>
+let tray = null;
+let isQuitting = false;
+let quitInProgress = false;
+let backendProcess = null;
+let backendConfig = null;
+let backendLogFd = null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting backend lifecycle, logging, locale handling, and startup HTML into focused modules (e.g., a BackendManager and helpers) so main.js becomes mostly simple wiring instead of a large, stateful god file.
You can reduce the mental load without changing behavior by extracting a few focused modules and encapsulating the shared state instead of globals. Here are concrete, incremental refactors that preserve functionality.
---
### 1. Encapsulate backend lifecycle into a `backendManager`
Right now `backendProcess`, `backendConfig`, `backendLogFd`, `backendLastExitReason`, `backendStartupFailureReason`, `backendSpawning`, `backendRestarting` and several helpers are global and touched from many places (including IPC). You can keep all logic but centralize it into a small module, and expose a clean API the IPC handlers and `whenReady` can call.
**Example module: `backendManager.js`**
```js
// backendManager.js
'use strict';
const fs = require('fs');
const os = require('os');
const path = require('path');
const { spawn, spawnSync } = require('child_process');
const { app } = require('electron');
const { delay, ensureDir, pingBackend, waitForProcessExit, normalizeUrl } = require('./backendUtils');
const { logElectron } = require('./electronLogger');
const backendUrl = normalizeUrl(
process.env.ASTRBOT_BACKEND_URL || 'http://127.0.0.1:6185/',
);
class BackendManager {
constructor({ backendTimeoutMs }) {
this.backendTimeoutMs = backendTimeoutMs;
this.backendProcess = null;
this.backendConfig = null;
this.backendLogFd = null;
this.lastExitReason = null;
this.startupFailureReason = null;
this.spawning = false;
this.restarting = false;
this.autoStart = process.env.ASTRBOT_BACKEND_AUTO_START !== '0';
}
getUrl() {
return backendUrl;
}
canManage() {
if (!this.backendConfig) {
this.backendConfig = resolveBackendConfig(); // existing logic moved here
}
return Boolean(this.backendConfig?.cmd);
}
async getState() {
const running = await pingBackend(backendUrl);
return {
running,
spawning: this.spawning,
restarting: this.restarting,
canManage: this.canManage(),
};
}
async ensureBackend() {
this.startupFailureReason = null;
if (!this.backendConfig) {
this.backendConfig = resolveBackendConfig();
}
const running = await pingBackend(backendUrl);
if (running) return true;
if (!this.autoStart || !this.backendConfig.cmd) {
this.startupFailureReason =
'Backend auto-start is disabled or backend command is not configured.';
return false;
}
const waitResult = await this.startAndWait(this.backendTimeoutMs);
if (!waitResult.ok) {
this.startupFailureReason = waitResult.reason;
return false;
}
return true;
}
async startAndWait(maxWaitMs) {
if (!this.canManage()) {
return { ok: false, reason: 'Backend command is not configured.' };
}
this.spawning = true;
try {
this.start();
return await waitForBackend(backendUrl, maxWaitMs, () => !this.backendProcess, () => this.lastExitReason); // reuse existing logic
} finally {
this.spawning = false;
}
}
start() {
// move existing startBackend logic here, but use this.* instead of globals
}
async stop() {
// move existing stopBackend logic here, but use this.* instead of globals
}
getStartupFailureReason() {
return this.startupFailureReason;
}
getLastExitReason() {
return this.lastExitReason;
}
}
module.exports = { BackendManager, backendUrl };
```
**Usage in `main`**
```js
// main.js
const { BackendManager, backendUrl } = require('./backendManager');
const backendManager = new BackendManager({ backendTimeoutMs });
// IPC handlers become simple:
ipcMain.handle('astrbot-desktop:get-backend-state', async () => {
return backendManager.getState();
});
ipcMain.handle('astrbot-desktop:restart-backend', async () => {
if (!backendManager.canManage()) {
return { ok: false, reason: 'Backend command is not configured.' };
}
if (backendManager.spawning || backendManager.restarting) {
return { ok: false, reason: 'Backend action already in progress.' };
}
backendManager.restarting = true;
try {
await backendManager.stop();
return await backendManager.startAndWait(backendTimeoutMs);
} finally {
backendManager.restarting = false;
}
});
ipcMain.handle('astrbot-desktop:stop-backend', async () => {
if (!backendManager.canManage()) {
return { ok: false, reason: 'Backend command is not configured.' };
}
// reuse same manager methods instead of duplicating checks
// ...
});
```
This removes duplicated state checks (`backendSpawning`/`backendRestarting`, `backendConfig` resolution) and consolidates lifecycle-oriented behavior into one place without altering semantics.
---
### 2. Move logging helpers to a dedicated module
`logElectron`, `getElectronLogPath`, and `closeBackendLogFd` logic is sprinkled around. Even a tiny `electronLogger.js` improves readability and lets backend and window logic focus on their own concerns.
**`electronLogger.js`**
```js
'use strict';
const fs = require('fs');
const path = require('path');
const { app } = require('electron');
function ensureDir(dir) {
if (!dir) return;
if (fs.existsSync(dir)) return;
fs.mkdirSync(dir, { recursive: true });
}
function getElectronLogPath(rootDirOrConfig) {
const rootDir =
process.env.ASTRBOT_ROOT ||
rootDirOrConfig?.rootDir ||
app.getPath('userData');
return path.join(rootDir, 'logs', 'electron.log');
}
function logElectron(message, { backendConfig } = {}) {
const logPath = getElectronLogPath(backendConfig);
ensureDir(path.dirname(logPath));
const line = `[${new Date().toISOString()}] ${message}\n`;
try {
fs.appendFileSync(logPath, line, 'utf8');
} catch {}
}
module.exports = { logElectron, getElectronLogPath, ensureDir };
```
**Usage in `main.js` / `backendManager.js`**
```js
const { logElectron, ensureDir } = require('./electronLogger');
// replace inline logElectron / ensureDir implementations
```
You can keep backend log FD management in `backendManager`, but this module keeps the electron log plumbing out of the main file.
---
### 3. Extract locale persistence into a `localeStore`
`normalizeLocale`, JSON state file reading/writing, and `persistLocaleFromDashboard` are all coherent but unrelated to tray/window/backend. Moving them out shrinks the main file and makes behavior easier to reason about.
**`localeStore.js`**
```js
'use strict';
const fs = require('fs');
const path = require('path');
const os = require('os');
const { app } = require('electron');
const { delay } = require('./backendUtils');
const LOCALE_STORAGE_KEY = 'astrbot-locale';
const SUPPORTED_STARTUP_LOCALES = new Set(['zh-CN', 'en-US']);
function normalizeLocale(value) {
if (!value) return null;
const raw = String(value).trim();
if (!raw) return null;
if (SUPPORTED_STARTUP_LOCALES.has(raw)) return raw;
const lower = raw.toLowerCase();
if (lower.startsWith('zh')) return 'zh-CN';
if (lower.startsWith('en')) return 'en-US';
return null;
}
function getDesktopStatePath({ backendConfig }) {
const rootDir =
process.env.ASTRBOT_ROOT ||
backendConfig?.rootDir ||
path.join(os.homedir(), '.astrbot') ||
app.getPath('userData');
return path.join(rootDir, 'data', 'desktop_state.json');
}
function readCachedLocale(opts) {
const statePath = getDesktopStatePath(opts);
try {
const raw = fs.readFileSync(statePath, 'utf8');
const parsed = JSON.parse(raw);
return normalizeLocale(parsed?.locale);
} catch {
return null;
}
}
function writeCachedLocale(locale, opts) {
const normalized = normalizeLocale(locale);
if (!normalized) return;
const statePath = getDesktopStatePath(opts);
fs.mkdirSync(path.dirname(statePath), { recursive: true });
try {
fs.writeFileSync(
statePath,
`${JSON.stringify({ locale: normalized }, null, 2)}\n`,
'utf8',
);
} catch {}
}
function resolveStartupLocale(opts) {
const cached = readCachedLocale(opts);
if (cached) return cached;
return normalizeLocale(app.getLocale()) || 'zh-CN';
}
async function persistLocaleFromDashboard(mainWindow, backendUrl, opts, timeoutMs = 1200) {
if (!mainWindow || mainWindow.isDestroyed()) return;
const currentUrl = mainWindow.webContents.getURL();
if (!currentUrl || !currentUrl.startsWith(backendUrl)) return;
try {
const localeRaw = await Promise.race([
mainWindow.webContents.executeJavaScript(
`(() => {
try {
return window.localStorage.getItem('${LOCALE_STORAGE_KEY}') || '';
} catch {
return '';
}
})();`,
true,
),
delay(timeoutMs).then(() => null),
]);
const locale = normalizeLocale(localeRaw);
if (locale) {
writeCachedLocale(locale, opts);
}
} catch {}
}
module.exports = {
normalizeLocale,
resolveStartupLocale,
persistLocaleFromDashboard,
};
```
**Usage in `main.js`**
```js
const { resolveStartupLocale, persistLocaleFromDashboard } = require('./localeStore');
const { backendConfig } = backendManager; // or pass backendManager directly
mainWindow.webContents.on('did-finish-load', () => {
const currentUrl = mainWindow.webContents.getURL();
logElectron(`did-finish-load url=${currentUrl}`);
if (currentUrl.startsWith(backendManager.getUrl())) {
void persistLocaleFromDashboard(mainWindow, backendManager.getUrl(), { backendConfig });
}
});
// in loadStartupScreen
const startupLocale = resolveStartupLocale({ backendConfig });
```
This keeps all locale logic self-contained and lets you change localization behavior without touching the main process boilerplate.
---
### 4. Move the inline startup HTML into a separate helper/module
The inline template string inside `loadStartupScreen` is a major contributor to perceived size. Even if you keep the HTML as a string, moving it out will make the entry file easier to scan.
**`startupScreen.js`**
```js
'use strict';
const fs = require('fs');
const path = require('path');
const { getAssetPath } = require('./assets'); // small helper for assets
const { resolveStartupLocale } = require('./localeStore');
function getStartupTexts(locale) {
if (locale === 'zh-CN') {
return {
title: 'AstrBot 正在启动',
message: '界面很快就会加载完成。',
};
}
return {
title: 'AstrBot is starting',
message: 'The dashboard will be ready in a moment.',
};
}
async function loadStartupScreen(mainWindow, opts) {
if (!mainWindow) return false;
const startupLocale = resolveStartupLocale(opts);
const startupTexts = getStartupTexts(startupLocale);
let iconUrl = '';
try {
const iconBuffer = fs.readFileSync(getAssetPath('icon-no-shadow.svg'));
iconUrl = `data:image/svg+xml;base64,${iconBuffer.toString('base64')}`;
} catch {}
const html = buildStartupHtml(startupTexts, iconUrl);
const startupUrl = `data:text/html;charset=utf-8,${encodeURIComponent(html)}`;
await mainWindow.loadURL(startupUrl);
return true;
}
function buildStartupHtml(startupTexts, iconUrl) {
return `<!doctype html>
<html lang="en">
<head>
<!-- existing CSS/HTML exactly as before -->
</head>
<body>
<div class="card">
${
iconUrl
? `<img class="logo" src="${iconUrl}" alt="AstrBot logo" />`
: '<div class="logo" aria-hidden="true"></div>'
}
<div class="spinner" aria-hidden="true"></div>
<h1>${startupTexts.title}</h1>
<p>${startupTexts.message}</p>
</div>
</body>
</html>`;
}
module.exports = { loadStartupScreen };
```
**Usage in `main.js`**
```js
const { loadStartupScreen } = require('./startupScreen');
app.whenReady().then(async () => {
// ...
await loadStartupScreen(mainWindow, { backendConfig });
// ...
});
```
---
These steps let you:
- Keep all features and behavior.
- Remove globals from the main file by encapsulating them in `BackendManager`.
- Shrink `main.js` to mostly “wire-up” logic (Electron lifecycle + IPC + delegations).
- Localize backend lifecycle, logging, startup HTML, and locale handling to cohesive modules, directly addressing the “too many responsibilities in one file” and “global state soup” concerns without a large rewrite.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
嗨——我已经审查了你的改动，一切看起来都很棒！
帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进之后的代码审查。
Original comment in English
Hey - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Soulter
approved these changes
Feb 8, 2026
Soulter
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
为 AstrBot 新增一个基于 Electron 的桌面外壳应用，将 WebUI 和可选的打包后后端一并集成，并整合所需的运行时路径、安装器和工作流，以支持在打包和开发环境下的构建、运行和重启。
新功能：
增强项：
构建：
CI：
部署：
文档：
Original summary in English
Summary by Sourcery
Add an Electron-based desktop shell for AstrBot that bundles the WebUI and an optional packaged backend, and integrate the necessary runtime paths, installers, and workflows to support building, running, and restarting in both packaged and development environments.
New Features:
Enhancements:
Build:
CI:
Deployment:
Documentation:
Original summary in English
Summary by Sourcery
为 AstrBot 新增一个基于 Electron 的桌面外壳应用，将 WebUI 和可选的打包后后端一并集成，并整合所需的运行时路径、安装器和工作流，以支持在打包和开发环境下的构建、运行和重启。
新功能：
增强项：
构建：
CI：
部署：
文档：
Original summary in English
Summary by Sourcery
Add an Electron-based desktop shell for AstrBot that bundles the WebUI and an optional packaged backend, and integrate the necessary runtime paths, installers, and workflows to support building, running, and restarting in both packaged and development environments.
New Features:
Enhancements:
Build:
CI:
Deployment:
Documentation:
新功能：
增强：
构建：
Original summary in English
Summary by Sourcery
为 AstrBot 新增一个基于 Electron 的桌面外壳应用，将 WebUI 和可选的打包后后端一并集成，并整合所需的运行时路径、安装器和工作流，以支持在打包和开发环境下的构建、运行和重启。
新功能：
增强项：
构建：
CI：
部署：
文档：
Original summary in English
Summary by Sourcery
Add an Electron-based desktop shell for AstrBot that bundles the WebUI and an optional packaged backend, and integrate the necessary runtime paths, installers, and workflows to support building, running, and restarting in both packaged and development environments.
New Features:
Enhancements:
Build:
CI:
Deployment:
Documentation:
Original summary in English
Summary by Sourcery
为 AstrBot 新增一个基于 Electron 的桌面外壳应用，将 WebUI 和可选的打包后后端一并集成，并整合所需的运行时路径、安装器和工作流，以支持在打包和开发环境下的构建、运行和重启。
新功能：
增强项：
构建：
CI：
部署：
文档：
Original summary in English
Summary by Sourcery
Add an Electron-based desktop shell for AstrBot that bundles the WebUI and an optional packaged backend, and integrate the necessary runtime paths, installers, and workflows to support building, running, and restarting in both packaged and development environments.
New Features:
Enhancements:
Build:
CI:
Deployment:
Documentation: