refactor: 按子系统重组桌面运行时并接入应用自更新桥接#64
Merged
zouyonghe merged 14 commits intoAstrBotDevs:mainfrom Mar 6, 2026
Merged
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the desktop runtime codebase to improve maintainability and architectural clarity. By reorganizing modules into logical subsystems and extracting pure decision logic from orchestration, the changes aim to make the codebase easier to understand, navigate, and extend. This refactoring preserves existing behavior and test coverage, ensuring no functional changes from a user's perspective, but provides a more stable foundation for future development. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了两个问题，并留下了一些整体性的反馈：
- 在
app_runtime::run中，嵌套的调用链configure_setup(configure_page_load_events(configure_window_events(configure_plugins(Builder::default()))))比实际需要的要更难阅读和修改；可以考虑构建一个可变的builder，然后依次应用每个configure_*辅助函数，这样可以让后续的修改和 diff 更清晰。 createPrepareResourcesContext辅助函数目前会返回很多字段的原始值和规范化后的值两个版本（例如sourceRepoUrlRaw和sourceRepoUrl、desktopVersionOverrideRaw和desktopVersionOverride）；值得考虑收紧这个 API，让调用方只接触到他们真正需要的规范化值，从而减少暴露面并降低误用原始值的风险。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- The nested `configure_setup(configure_page_load_events(configure_window_events(configure_plugins(Builder::default()))))` call chain in `app_runtime::run` is harder to read and modify than necessary; consider building a mutable `builder` and applying each `configure_*` helper in sequence to make future changes and diffs clearer.
- The `createPrepareResourcesContext` helper currently returns both raw and normalized variants of many fields (e.g., `sourceRepoUrlRaw` vs `sourceRepoUrl`, `desktopVersionOverrideRaw` vs `desktopVersionOverride`); it may be worth tightening this API so that callers only see the normalized values they actually need, reducing the surface area and chances of misusing the raw values.
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/desktop-bridge-checks.mjs" line_range="79-80" />
<code_context>
-
- for (const expectation of DESKTOP_BRIDGE_EXPECTATIONS) {
- const mustPass = shouldEnforceDesktopBridgeExpectation(expectation);
+ for (const expectation of getDesktopBridgeExpectations()) {
+ const mustPass = shouldEnforceDesktopBridgeExpectation(expectation, {
+ isDesktopBridgeExpectationStrict,
+ isTaggedRelease,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 考虑缓存 `getDesktopBridgeExpectations()` 的结果，或者导出一个被冻结的数组，以避免被意外修改。
目前 `getDesktopBridgeExpectations()` 返回的是共享的 `DESKTOP_BRIDGE_EXPECTATIONS` 数组。任意调用方如果修改了该数组或数组元素，都会影响后续的所有检查。为避免这一点，可以返回一个浅拷贝（例如 `return [...DESKTOP_BRIDGE_EXPECTATIONS];`），或者在定义时对数组及其对象进行冻结，这样一旦发生意外修改可以在运行时快速失败。
建议的实现：
```javascript
function getDesktopBridgeExpectations() {
// 返回浅拷贝以避免对共享数组的意外修改
return [...DESKTOP_BRIDGE_EXPECTATIONS];
}
```
如果 `getDesktopBridgeExpectations` 的签名不同，或者是用 `const` 箭头函数定义的，请在 SEARCH/REPLACE 代码块中适配现有声明，只将返回值改为 `return [...DESKTOP_BRIDGE_EXPECTATIONS];`。
如果 `DESKTOP_BRIDGE_EXPECTATIONS` 定义在其他模块中，请确保在该模块中修改 `getDesktopBridgeExpectations` 的定义，而不是在此文件中修改，或者从定义 `DESKTOP_BRIDGE_EXPECTATIONS` 的模块中导出这个辅助函数并在这里导入。
</issue_to_address>
### Comment 2
<location path="src-tauri/src/backend/restart_strategy.rs" line_range="16" />
<code_context>
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) enum RestartExecution {
+ ReturnSuccess,
+ FallBackToManagedRestart { log_message: String },
</code_context>
<issue_to_address>
**issue (complexity):** 建议移除额外的 RestartExecution 枚举和 resolve_restart_execution 函数，而是在调用点直接（或通过一个简单的数据结构）解释 RestartStrategy 和 GracefulRestartOutcome，从而减少概念层面的复杂度。
你可以通过删除 `RestartExecution` 和 `resolve_restart_execution`，并将 `(RestartStrategy, GracefulRestartOutcome)` 直接映射到 `BackendState` 中的具体行为（日志记录 + 返回 / 回退），同时仍然保留可测试的部分，从而简化代码。
现在的做法：
```rust
pub(crate) enum RestartExecution {
ReturnSuccess,
FallBackToManagedRestart { log_message: String },
ReturnError(String),
}
pub(crate) fn resolve_restart_execution(
strategy: RestartStrategy,
outcome: GracefulRestartOutcome,
) -> RestartExecution {
// ...
}
```
可以改为让 `BackendState` 直接解释 `RestartStrategy` 和 `GracefulRestartOutcome`：
```rust
// 在该模块中：只保留数据 + 分类
pub(crate) enum RestartStrategy { /* ... */ }
pub(crate) enum GracefulRestartOutcome { /* ... */ }
pub(crate) fn compute_restart_strategy(/* ... */) -> RestartStrategy { /* ... */ }
pub(crate) fn map_graceful_restart_outcome(
request_accepted: bool,
wait_result: Result<(), String>,
) -> GracefulRestartOutcome {
// ...
}
```
然后在 `BackendState::restart_backend`（或等价位置）中内联当前 `resolve_restart_execution` 中的逻辑，并在有副作用的地方使用：
```rust
let strategy = compute_restart_strategy(is_windows, packaged_mode, has_managed_child);
let outcome = map_graceful_restart_outcome(request_accepted, wait_result);
match (strategy, outcome) {
(RestartStrategy::ManagedSkipGraceful, _) => {
log::info!(
"skip graceful restart for packaged windows managed backend; using managed restart"
);
return self.managed_restart(); // existing fallback path
}
(RestartStrategy::ManagedWithGracefulFallback, GracefulRestartOutcome::Completed) |
(RestartStrategy::UnmanagedWithGracefulProbe, GracefulRestartOutcome::Completed) => {
return Ok(());
}
(RestartStrategy::ManagedWithGracefulFallback, GracefulRestartOutcome::WaitFailed(error)) => {
log::warn!(
"graceful restart did not complete, fallback to managed restart: {error}"
);
return self.managed_restart();
}
(RestartStrategy::ManagedWithGracefulFallback, GracefulRestartOutcome::RequestRejected) => {
log::warn!(
"graceful restart request was rejected, fallback to managed restart"
);
return self.managed_restart();
}
(RestartStrategy::UnmanagedWithGracefulProbe, GracefulRestartOutcome::WaitFailed(error)) => {
log::warn!(
"graceful restart did not complete for unmanaged backend, \
bootstrap managed restart: {error}"
);
return self.bootstrap_managed_restart();
}
(RestartStrategy::UnmanagedWithGracefulProbe, GracefulRestartOutcome::RequestRejected) => {
log::error!(
"graceful restart request was rejected and backend is not desktop-managed."
);
return Err("graceful restart request was rejected and backend is not desktop-managed."
.to_string());
}
}
```
如果你仍然希望保留一个可测试的、无副作用的“决策”单元，可以保留一个返回原始类型而不是第三个枚举的小辅助函数：
```rust
pub(crate) struct RestartDecision {
pub use_managed_restart: bool,
pub error: Option<String>,
pub log_message: Option<String>,
}
pub(crate) fn compute_restart_decision(
strategy: RestartStrategy,
outcome: GracefulRestartOutcome,
) -> RestartDecision {
match (strategy, outcome) {
(RestartStrategy::ManagedSkipGraceful, _) => RestartDecision {
use_managed_restart: true,
error: None,
log_message: Some(
"skip graceful restart for packaged windows managed backend; using managed restart"
.to_string(),
),
},
// ...mirror current branches here...
}
}
```
然后 `BackendState` 只需要解释 `RestartDecision`（简单的布尔值/字符串），而不需要额外的 DSL 式枚举，从而在保留所有行为和测试覆盖率的同时减少概念复杂度。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The nested
configure_setup(configure_page_load_events(configure_window_events(configure_plugins(Builder::default()))))call chain inapp_runtime::runis harder to read and modify than necessary; consider building a mutablebuilderand applying eachconfigure_*helper in sequence to make future changes and diffs clearer. - The
createPrepareResourcesContexthelper currently returns both raw and normalized variants of many fields (e.g.,sourceRepoUrlRawvssourceRepoUrl,desktopVersionOverrideRawvsdesktopVersionOverride); it may be worth tightening this API so that callers only see the normalized values they actually need, reducing the surface area and chances of misusing the raw values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The nested `configure_setup(configure_page_load_events(configure_window_events(configure_plugins(Builder::default()))))` call chain in `app_runtime::run` is harder to read and modify than necessary; consider building a mutable `builder` and applying each `configure_*` helper in sequence to make future changes and diffs clearer.
- The `createPrepareResourcesContext` helper currently returns both raw and normalized variants of many fields (e.g., `sourceRepoUrlRaw` vs `sourceRepoUrl`, `desktopVersionOverrideRaw` vs `desktopVersionOverride`); it may be worth tightening this API so that callers only see the normalized values they actually need, reducing the surface area and chances of misusing the raw values.
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/desktop-bridge-checks.mjs" line_range="79-80" />
<code_context>
-
- for (const expectation of DESKTOP_BRIDGE_EXPECTATIONS) {
- const mustPass = shouldEnforceDesktopBridgeExpectation(expectation);
+ for (const expectation of getDesktopBridgeExpectations()) {
+ const mustPass = shouldEnforceDesktopBridgeExpectation(expectation, {
+ isDesktopBridgeExpectationStrict,
+ isTaggedRelease,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider caching `getDesktopBridgeExpectations()` results or exporting a frozen array to avoid accidental mutation.
Right now `getDesktopBridgeExpectations()` returns the shared `DESKTOP_BRIDGE_EXPECTATIONS` array. Any caller that mutates the array or its elements will affect all future checks. To avoid this, either return a shallow copy (e.g. `return [...DESKTOP_BRIDGE_EXPECTATIONS];`) or freeze the array and its objects at definition time so accidental mutations fail fast at runtime.
Suggested implementation:
```javascript
function getDesktopBridgeExpectations() {
// Return a shallow copy to avoid accidental mutation of the shared array
return [...DESKTOP_BRIDGE_EXPECTATIONS];
}
```
If `getDesktopBridgeExpectations` is defined with a different signature or as a `const` arrow function, adapt the SEARCH/REPLACE block to match the existing declaration and only change the returned value to `return [...DESKTOP_BRIDGE_EXPECTATIONS];`.
If `DESKTOP_BRIDGE_EXPECTATIONS` is defined in another module, ensure you edit the definition of `getDesktopBridgeExpectations` in that module instead of this file, or export this helper from where `DESKTOP_BRIDGE_EXPECTATIONS` is defined and import it here.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/backend/restart_strategy.rs" line_range="16" />
<code_context>
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) enum RestartExecution {
+ ReturnSuccess,
+ FallBackToManagedRestart { log_message: String },
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the extra RestartExecution enum and resolve_restart_execution function and instead interpret RestartStrategy and GracefulRestartOutcome directly (or via a simple data struct) at the call site to reduce conceptual surface area.
You can simplify this by dropping `RestartExecution` and `resolve_restart_execution` and mapping directly from `(RestartStrategy, GracefulRestartOutcome)` to the concrete behavior in `BackendState` (logging + return / fallback), while still keeping the testable pieces.
Instead of:
```rust
pub(crate) enum RestartExecution {
ReturnSuccess,
FallBackToManagedRestart { log_message: String },
ReturnError(String),
}
pub(crate) fn resolve_restart_execution(
strategy: RestartStrategy,
outcome: GracefulRestartOutcome,
) -> RestartExecution {
// ...
}
```
have `BackendState` interpret `RestartStrategy` and `GracefulRestartOutcome` directly:
```rust
// in this module: keep only data + classification
pub(crate) enum RestartStrategy { /* ... */ }
pub(crate) enum GracefulRestartOutcome { /* ... */ }
pub(crate) fn compute_restart_strategy(/* ... */) -> RestartStrategy { /* ... */ }
pub(crate) fn map_graceful_restart_outcome(
request_accepted: bool,
wait_result: Result<(), String>,
) -> GracefulRestartOutcome {
// ...
}
```
Then, in `BackendState::restart_backend` (or equivalent), inline the logic currently in `resolve_restart_execution` and use it where you have side-effects:
```rust
let strategy = compute_restart_strategy(is_windows, packaged_mode, has_managed_child);
let outcome = map_graceful_restart_outcome(request_accepted, wait_result);
match (strategy, outcome) {
(RestartStrategy::ManagedSkipGraceful, _) => {
log::info!(
"skip graceful restart for packaged windows managed backend; using managed restart"
);
return self.managed_restart(); // existing fallback path
}
(RestartStrategy::ManagedWithGracefulFallback, GracefulRestartOutcome::Completed) |
(RestartStrategy::UnmanagedWithGracefulProbe, GracefulRestartOutcome::Completed) => {
return Ok(());
}
(RestartStrategy::ManagedWithGracefulFallback, GracefulRestartOutcome::WaitFailed(error)) => {
log::warn!(
"graceful restart did not complete, fallback to managed restart: {error}"
);
return self.managed_restart();
}
(RestartStrategy::ManagedWithGracefulFallback, GracefulRestartOutcome::RequestRejected) => {
log::warn!(
"graceful restart request was rejected, fallback to managed restart"
);
return self.managed_restart();
}
(RestartStrategy::UnmanagedWithGracefulProbe, GracefulRestartOutcome::WaitFailed(error)) => {
log::warn!(
"graceful restart did not complete for unmanaged backend, \
bootstrap managed restart: {error}"
);
return self.bootstrap_managed_restart();
}
(RestartStrategy::UnmanagedWithGracefulProbe, GracefulRestartOutcome::RequestRejected) => {
log::error!(
"graceful restart request was rejected and backend is not desktop-managed."
);
return Err("graceful restart request was rejected and backend is not desktop-managed."
.to_string());
}
}
```
If you still want a testable “decision” unit without side effects, keep a small helper that returns primitives instead of a third enum:
```rust
pub(crate) struct RestartDecision {
pub use_managed_restart: bool,
pub error: Option<String>,
pub log_message: Option<String>,
}
pub(crate) fn compute_restart_decision(
strategy: RestartStrategy,
outcome: GracefulRestartOutcome,
) -> RestartDecision {
match (strategy, outcome) {
(RestartStrategy::ManagedSkipGraceful, _) => RestartDecision {
use_managed_restart: true,
error: None,
log_message: Some(
"skip graceful restart for packaged windows managed backend; using managed restart"
.to_string(),
),
},
// ...mirror current branches here...
}
}
```
`BackendState` then only interprets `RestartDecision` (simple booleans/strings) instead of an extra DSL enum, reducing conceptual surface area while preserving all behavior and test coverage.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些高层次的反馈：
- 在
scripts/prepare-resources.mjs中，对createPrepareResourcesContext的解构期望得到sourceDirOverrideRaw和desktopVersionOverrideRaw这样的字段，但工厂函数当前并没有返回这些字段；建议要么把这些字段加到返回的 context 中，要么从解构中去掉它们，以避免依赖undefined并保持 context 的接口清晰明确。 - 后端重启流程现在把逻辑拆分在
backend::restart和backend::restart_strategy中；execute_graceful_restart_strategy中的控制路径，加上restart_backend里对ManagedSkipGraceful的特殊处理，有点难以跟踪——可以考虑简化一下，或者添加一个简短的注释，解释 strategy、outcome 和后续重启决策之间预期的约定。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- 在 `scripts/prepare-resources.mjs` 中，对 `createPrepareResourcesContext` 的解构期望得到 `sourceDirOverrideRaw` 和 `desktopVersionOverrideRaw` 这样的字段，但工厂函数当前并没有返回这些字段；建议要么把这些字段加到返回的 context 中，要么从解构中去掉它们，以避免依赖 `undefined` 并保持 context 的接口清晰明确。
- 后端重启流程现在把逻辑拆分在 `backend::restart` 和 `backend::restart_strategy` 中；`execute_graceful_restart_strategy` 中的控制路径，加上 `restart_backend` 里对 `ManagedSkipGraceful` 的特殊处理，有点难以跟踪——可以考虑简化一下，或者添加一个简短的注释，解释 strategy、outcome 和后续重启决策之间预期的约定。
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/context.mjs" line_range="34-39" />
<code_context>
+ trimEnv(env, 'ASTRBOT_DESKTOP_STRICT_BRIDGE_EXPECTATIONS').toLowerCase(),
+ );
+
+ const { repoUrl: sourceRepoUrl, repoRef: sourceRepoRef } = normalizeSourceRepoConfig(
+ sourceRepoUrlInput,
+ sourceRepoRefInput,
+ );
+
+ const {
+ isCommit: isSourceRepoRefCommitSha,
+ isVersionTag: isSourceRepoRefVersionTag,
</code_context>
<issue_to_address>
**issue (bug_risk):** `getSourceRefInfo` 计算得到的源 ref 规范化结果没有被使用，这可能会在 refs/tags 的处理方式上带来细微变化。
在之前的流程中，`getSourceRefInfo` 规范化后的 `ref` 会作为导出的 `sourceRepoRef`。现在你调用 `getSourceRefInfo(sourceRepoRef, sourceRepoRefCommitHint)` 却忽略了它规范化后的 `ref`，因此 `sourceRepoRef` 现在直接来自 `normalizeSourceRepoConfig`。如果调用方依赖调用 `getSourceRefInfo` 之后的值，这可能会改变 tag/commit ref 的行为。为了保持行为一致，建议像之前那样进行规范化并导出该 `ref`：
```js
const { repoUrl: sourceRepoUrl, repoRef: sourceRepoRefInputNormalized } = normalizeSourceRepoConfig(
sourceRepoUrlInput,
sourceRepoRefInput,
);
const {
ref: sourceRepoRef,
isCommit: isSourceRepoRefCommitSha,
isVersionTag: isSourceRepoRefVersionTag,
} = getSourceRefInfo(sourceRepoRefInputNormalized, sourceRepoRefCommitHint);
```
</issue_to_address>
### Comment 2
<location path="scripts/prepare-resources/context.mjs" line_range="46-55" />
<code_context>
+
+ const sourceDir = resolveSourceDir(projectRoot, sourceDirOverride, cwd);
+
+ return {
+ mode,
+ pythonBuildStandaloneRelease,
+ pythonBuildStandaloneVersion,
+ desktopVersionOverride,
+ isDesktopBridgeExpectationStrict,
+ sourceRepoUrl,
+ sourceRepoRef,
+ isSourceRepoRefCommitSha,
+ isSourceRepoRefVersionTag,
+ sourceDir,
+ };
+};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 返回的 context 结构中缺少了一些被调用方解构出来的字段，这既让人困惑，也有引入未来 bug 的风险。
在 `prepare-resources.mjs` 中，你从 context 中解构了 `sourceDirOverrideRaw` 和 `desktopVersionOverrideRaw`，但 `createPrepareResourcesContext` 并没有返回它们，因此它们总是 `undefined`，API 的约定也变得不清楚。
如果调用方确实需要这些字段，要么把它们添加到返回的 context 中，要么如果它们已经废弃，就停止解构它们。让 context 的结构与其消费方保持一致，会避免未来的脆弱行为。
建议实现如下：
```javascript
const sourceRepoRefInput = trimEnv(env, 'ASTRBOT_SOURCE_GIT_REF');
const sourceRepoRefCommitHint = trimEnv(env, 'ASTRBOT_SOURCE_GIT_REF_IS_COMMIT');
const sourceDirOverride = trimEnv(env, 'ASTRBOT_SOURCE_DIR');
const desktopVersionInput = trimEnv(env, 'ASTRBOT_DESKTOP_VERSION');
const pythonBuildStandaloneRelease = trimEnv(env, 'ASTRBOT_PBS_RELEASE', '20260211');
const pythonBuildStandaloneVersion = trimEnv(env, 'ASTRBOT_PBS_VERSION', '3.12.12');
const sourceDirOverrideRaw = sourceDirOverride;
const desktopVersionOverrideRaw = desktopVersionInput;
```
```javascript
const sourceDir = resolveSourceDir(projectRoot, sourceDirOverride, cwd);
return {
mode,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
desktopVersionOverride,
isDesktopBridgeExpectationStrict,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
isSourceRepoRefVersionTag,
sourceDir,
sourceDirOverrideRaw,
desktopVersionOverrideRaw,
};
};
```
如果 `prepare-resources.mjs` 期望对这些“原始”值有略微不同的语义（例如，真正未 trim 的 env，或已解析 vs. 未解析），请相应调整 `sourceDirOverrideRaw` 和 `desktopVersionOverrideRaw` 的赋值（例如直接使用 `env.ASTRBOT_SOURCE_DIR` 而不是 `trimEnv`）。否则，由于 `prepare-resources.mjs` 已经在解构这些字段，不需要对它再做改动。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据反馈改进之后的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
scripts/prepare-resources.mjs, the destructuring ofcreatePrepareResourcesContextexpects fields likesourceDirOverrideRawanddesktopVersionOverrideRawthat the factory does not currently return; consider either adding these to the context or removing them from the destructuring to avoid relying on undefined values and keep the context surface explicit. - The backend restart flow now splits logic between
backend::restartandbackend::restart_strategy; the control path inexecute_graceful_restart_strategyplus the special-case handling ofManagedSkipGracefulinrestart_backendis a bit hard to follow—consider simplifying or adding a brief comment to explain the intended contract between strategy, outcome, and the subsequent relaunch decision.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `scripts/prepare-resources.mjs`, the destructuring of `createPrepareResourcesContext` expects fields like `sourceDirOverrideRaw` and `desktopVersionOverrideRaw` that the factory does not currently return; consider either adding these to the context or removing them from the destructuring to avoid relying on undefined values and keep the context surface explicit.
- The backend restart flow now splits logic between `backend::restart` and `backend::restart_strategy`; the control path in `execute_graceful_restart_strategy` plus the special-case handling of `ManagedSkipGraceful` in `restart_backend` is a bit hard to follow—consider simplifying or adding a brief comment to explain the intended contract between strategy, outcome, and the subsequent relaunch decision.
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/context.mjs" line_range="34-39" />
<code_context>
+ trimEnv(env, 'ASTRBOT_DESKTOP_STRICT_BRIDGE_EXPECTATIONS').toLowerCase(),
+ );
+
+ const { repoUrl: sourceRepoUrl, repoRef: sourceRepoRef } = normalizeSourceRepoConfig(
+ sourceRepoUrlInput,
+ sourceRepoRefInput,
+ );
+
+ const {
+ isCommit: isSourceRepoRefCommitSha,
+ isVersionTag: isSourceRepoRefVersionTag,
</code_context>
<issue_to_address>
**issue (bug_risk):** Source ref normalization from `getSourceRefInfo` is computed but not used, which can subtly change how refs/tags are handled.
In the previous flow, `getSourceRefInfo`’s normalized `ref` became the exported `sourceRepoRef`. Here you call `getSourceRefInfo(sourceRepoRef, sourceRepoRefCommitHint)` but ignore its normalized `ref`, so `sourceRepoRef` now comes directly from `normalizeSourceRepoConfig`. This may change behavior for tag/commit refs if callers rely on the post-`getSourceRefInfo` value. To preserve behavior, consider normalizing as before and exporting that `ref`:
```js
const { repoUrl: sourceRepoUrl, repoRef: sourceRepoRefInputNormalized } = normalizeSourceRepoConfig(
sourceRepoUrlInput,
sourceRepoRefInput,
);
const {
ref: sourceRepoRef,
isCommit: isSourceRepoRefCommitSha,
isVersionTag: isSourceRepoRefVersionTag,
} = getSourceRefInfo(sourceRepoRefInputNormalized, sourceRepoRefCommitHint);
```
</issue_to_address>
### Comment 2
<location path="scripts/prepare-resources/context.mjs" line_range="46-55" />
<code_context>
+
+ const sourceDir = resolveSourceDir(projectRoot, sourceDirOverride, cwd);
+
+ return {
+ mode,
+ pythonBuildStandaloneRelease,
+ pythonBuildStandaloneVersion,
+ desktopVersionOverride,
+ isDesktopBridgeExpectationStrict,
+ sourceRepoUrl,
+ sourceRepoRef,
+ isSourceRepoRefCommitSha,
+ isSourceRepoRefVersionTag,
+ sourceDir,
+ };
+};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Context return shape doesn’t include some fields the caller destructures, which is confusing and risks future bugs.
In `prepare-resources.mjs` you destructure `sourceDirOverrideRaw` and `desktopVersionOverrideRaw` from the context, but `createPrepareResourcesContext` doesn’t return them, so they’re always `undefined` and the API contract is unclear.
Either add these fields to the returned context if callers should use them, or stop destructuring them if they’re obsolete. Keeping the context shape in sync with its consumers will prevent brittle behavior in future changes.
Suggested implementation:
```javascript
const sourceRepoRefInput = trimEnv(env, 'ASTRBOT_SOURCE_GIT_REF');
const sourceRepoRefCommitHint = trimEnv(env, 'ASTRBOT_SOURCE_GIT_REF_IS_COMMIT');
const sourceDirOverride = trimEnv(env, 'ASTRBOT_SOURCE_DIR');
const desktopVersionInput = trimEnv(env, 'ASTRBOT_DESKTOP_VERSION');
const pythonBuildStandaloneRelease = trimEnv(env, 'ASTRBOT_PBS_RELEASE', '20260211');
const pythonBuildStandaloneVersion = trimEnv(env, 'ASTRBOT_PBS_VERSION', '3.12.12');
const sourceDirOverrideRaw = sourceDirOverride;
const desktopVersionOverrideRaw = desktopVersionInput;
```
```javascript
const sourceDir = resolveSourceDir(projectRoot, sourceDirOverride, cwd);
return {
mode,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
desktopVersionOverride,
isDesktopBridgeExpectationStrict,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
isSourceRepoRefVersionTag,
sourceDir,
sourceDirOverrideRaw,
desktopVersionOverrideRaw,
};
};
```
If `prepare-resources.mjs` expects slightly different semantics for the “raw” values (e.g., truly untrimmed env, or parsed vs. unparsed), adjust the assignments of `sourceDirOverrideRaw` and `desktopVersionOverrideRaw` accordingly (for example, use `env.ASTRBOT_SOURCE_DIR` directly instead of `trimEnv`). Otherwise, no changes are needed in `prepare-resources.mjs` since it already destructures these fields.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the desktop runtime codebase by reorganizing it into clearer Rust subsystems and script seams. The refactoring successfully separates concerns, extracts pure decision logic into dedicated modules, and updates documentation to reflect the new architectural shape. This will greatly enhance maintainability and make future feature development less error-prone. The addition of new test files for the extracted logic is also a strong positive, ensuring stability and correctness.
| "useMendCheckNames": true | ||
| }, | ||
| "issueSettings": { | ||
| "minSeverityLevel": "LOW", |
There was a problem hiding this comment.
The minSeverityLevel is currently set to "LOW". While this might be intentional, setting it to a higher level like "MEDIUM" or "HIGH" would ensure that more significant security vulnerabilities are always reported and addressed. Consider if filtering out medium or high severity issues by default aligns with the project's security posture.
"minSeverityLevel": "MEDIUM",
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些整体性的反馈：
- 在
scripts/prepare-resources.mjs中，版本漂移检查里的console.warn代码块被修改过，但现在似乎缺少结尾的);，而且多了一层缩进；请在该处仔细检查语法，以避免运行时解析错误。 - 既然
createPrepareResourcesContext已经集中处理了环境变量 / CLI 的归一化，现在可以考虑在prepare-resources.mjs中从这个上下文中读取desktopVersionInput和sourceDirOverrideInput，而不是重新读取process.env，从而把所有归一化逻辑和日志输入集中到一个地方。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- 在 `scripts/prepare-resources.mjs` 中，版本漂移检查里的 `console.warn` 代码块被修改过，但现在似乎缺少结尾的 `);`，而且多了一层缩进；请在该处仔细检查语法，以避免运行时解析错误。
- 既然 `createPrepareResourcesContext` 已经集中处理了环境变量 / CLI 的归一化，现在可以考虑在 `prepare-resources.mjs` 中从这个上下文中读取 `desktopVersionInput` 和 `sourceDirOverrideInput`，而不是重新读取 `process.env`，从而把所有归一化逻辑和日志输入集中到一个地方。
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources.mjs" line_range="88" />
<code_context>
}
- await runModeTasks(mode, sourceDir);
+ await runModeTasks(mode, {
+ prepareWebui: () =>
+ prepareWebui({
</code_context>
<issue_to_address>
**issue (complexity):** 考虑通过让 `runModeTasks` 消费简单数据而不是回调，并通过共享的 context 对象暴露原始 env 值，从而简化入口函数，让所有配置都从同一个地方流转。
你可以保留新的 `context` / `mode-dispatch` 结构，但通过以下方式来简化入口函数：
1. **避免针对 `runModeTasks` 的基于回调的控制反转（inversion of control）。**
2. **避免在 `createPrepareResourcesContext` 之后再次读取 `process.env`。**
### 1. 让 `runModeTasks` 接收数据而不是回调
与其传递两个 lambda，不如让 `runModeTasks` 自己调用 `prepareWebui` / `prepareBackend`。这样可以在不增加入口处 IoC 复杂度的前提下，将模式分发逻辑集中管理。
**当前：**
```js
await runModeTasks(mode, {
prepareWebui: () =>
prepareWebui({
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
}),
prepareBackend: () =>
prepareBackend({
sourceDir,
projectRoot,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
}),
});
```
**建议：**
```js
await runModeTasks(mode, {
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
```
然后在 `mode-dispatch.mjs` 中：
```js
import { prepareBackend, prepareWebui } from './mode-tasks.mjs';
export async function runModeTasks(
mode,
{
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
},
) {
if (mode === 'version') return;
if (mode === 'webui' || mode === 'all') {
await prepareWebui({
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
}
if (mode === 'backend' || mode === 'all') {
await prepareBackend({
sourceDir,
projectRoot,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
}
if (!['version', 'webui', 'backend', 'all'].includes(mode)) {
throw new Error(`Unsupported mode: ${mode}. Expected version/webui/backend/all.`);
}
}
```
这样既保留了新模块，又让主文件的控制流更加直接、易于理解。
### 2. 从 `createPrepareResourcesContext` 中暴露原始 env 值
当前入口函数会重新读取环境变量：
```js
const desktopVersionInput = process.env.ASTRBOT_DESKTOP_VERSION?.trim() || '';
const sourceDirOverrideInput = process.env.ASTRBOT_SOURCE_DIR?.trim() || '';
```
你可以让 `createPrepareResourcesContext` 直接暴露这些原始值，把所有 env 解析集中在一起，从而减少“这个值是从哪里来的？”这样的疑问。
**在 `context.mjs` 中（示意）：**
```js
export function createPrepareResourcesContext({ argv, env, projectRoot }) {
const desktopVersionInput = env.ASTRBOT_DESKTOP_VERSION?.trim() || '';
const sourceDirOverrideInput = env.ASTRBOT_SOURCE_DIR?.trim() || '';
// ...existing normalization / derivation...
return {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
desktopVersionInput,
sourceDirOverrideInput,
};
}
```
**然后在主文件中：**
```js
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
desktopVersionInput,
sourceDirOverrideInput,
} = context;
if (desktopVersionInput && desktopVersionInput !== desktopVersionOverride) {
console.log(
`[prepare-resources] Normalized ASTRBOT_DESKTOP_VERSION from ${desktopVersionInput} to ${desktopVersionOverride}`,
);
}
ensureSourceRepo({
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideRaw: sourceDirOverrideInput,
});
```
这样一来，入口函数就完全基于 `context` 对象工作，无需额外的 env 查找或隐式全局变量，同时操作顺序也更易于追踪。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据反馈改进之后的代码评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
scripts/prepare-resources.mjs, theconsole.warnblock inside the version drift check was modified but now appears to be missing its closing);and has an extra indentation level; please double-check the syntax there to avoid a runtime parse error. - Now that
createPrepareResourcesContextcentralizes env/CLI normalization, consider sourcingdesktopVersionInputandsourceDirOverrideInputinprepare-resources.mjsfrom that context instead of re-readingprocess.envto keep all normalization and logging inputs in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `scripts/prepare-resources.mjs`, the `console.warn` block inside the version drift check was modified but now appears to be missing its closing `);` and has an extra indentation level; please double-check the syntax there to avoid a runtime parse error.
- Now that `createPrepareResourcesContext` centralizes env/CLI normalization, consider sourcing `desktopVersionInput` and `sourceDirOverrideInput` in `prepare-resources.mjs` from that context instead of re-reading `process.env` to keep all normalization and logging inputs in one place.
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources.mjs" line_range="88" />
<code_context>
}
- await runModeTasks(mode, sourceDir);
+ await runModeTasks(mode, {
+ prepareWebui: () =>
+ prepareWebui({
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the entrypoint by letting `runModeTasks` consume plain data instead of callbacks and by exposing raw env values through the shared context object so all configuration flows from one place.
You can keep the new `context`/`mode-dispatch` structure but simplify the entrypoint by:
1. **Avoiding the callback-based inversion of control for `runModeTasks`.**
2. **Avoiding re-reading `process.env` after `createPrepareResourcesContext`.**
### 1. Make `runModeTasks` take data, not callbacks
Instead of passing two lambdas, let `runModeTasks` own the call to `prepareWebui` / `prepareBackend`. This keeps the mode dispatch centralized without adding IoC complexity at the entrypoint.
**Current:**
```js
await runModeTasks(mode, {
prepareWebui: () =>
prepareWebui({
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
}),
prepareBackend: () =>
prepareBackend({
sourceDir,
projectRoot,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
}),
});
```
**Suggested:**
```js
await runModeTasks(mode, {
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
```
Then in `mode-dispatch.mjs`:
```js
import { prepareBackend, prepareWebui } from './mode-tasks.mjs';
export async function runModeTasks(
mode,
{
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
},
) {
if (mode === 'version') return;
if (mode === 'webui' || mode === 'all') {
await prepareWebui({
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
}
if (mode === 'backend' || mode === 'all') {
await prepareBackend({
sourceDir,
projectRoot,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
}
if (!['version', 'webui', 'backend', 'all'].includes(mode)) {
throw new Error(`Unsupported mode: ${mode}. Expected version/webui/backend/all.`);
}
}
```
This preserves the new module but makes the main file’s control flow more direct and easier to follow.
### 2. Expose raw env values from `createPrepareResourcesContext`
Right now the entrypoint re-reads env vars:
```js
const desktopVersionInput = process.env.ASTRBOT_DESKTOP_VERSION?.trim() || '';
const sourceDirOverrideInput = process.env.ASTRBOT_SOURCE_DIR?.trim() || '';
```
You can let `createPrepareResourcesContext` expose these raw values and keep all env parsing centralized, reducing “where did this come from?” questions.
**In `context.mjs` (sketch):**
```js
export function createPrepareResourcesContext({ argv, env, projectRoot }) {
const desktopVersionInput = env.ASTRBOT_DESKTOP_VERSION?.trim() || '';
const sourceDirOverrideInput = env.ASTRBOT_SOURCE_DIR?.trim() || '';
// ...existing normalization / derivation...
return {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
desktopVersionInput,
sourceDirOverrideInput,
};
}
```
**Then in the main file:**
```js
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
desktopVersionInput,
sourceDirOverrideInput,
} = context;
if (desktopVersionInput && desktopVersionInput !== desktopVersionOverride) {
console.log(
`[prepare-resources] Normalized ASTRBOT_DESKTOP_VERSION from ${desktopVersionInput} to ${desktopVersionOverride}`,
);
}
ensureSourceRepo({
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideRaw: sourceDirOverrideInput,
});
```
That way, the entrypoint works entirely off the `context` object, with no extra env lookups or hidden globals, and the sequence of operations remains easy to trace.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些整体性的反馈：
- 在
scripts/ci/generate-tauri-latest-json.py中，WINDOWS_RE正则从未被使用，而 Windows 架构是通过简单的子串检查推断出来的；建议要么删除未使用的正则，要么切换为真正使用它（类似 macOS 的路径），这样架构检测会更加明确，也不容易因为文件名变化而出问题。 - 新的更新器相关逻辑现在分散在
bridge::commands和bridge::updater_types中；可以考虑通过文档或类型边界让分工更清晰（例如：commands只负责编排流程，而updater_types只包含纯映射结构），从而让未来更新器行为的修改更加局部化。
供 AI Agent 使用的提示
Please address the comments from this code review:
## Overall Comments
- In `scripts/ci/generate-tauri-latest-json.py`, the `WINDOWS_RE` regex is never used and the Windows arch is inferred via a simple substring check; either remove the unused regex or switch to using it (similar to the macOS path) so the arch detection is explicit and less brittle to filename changes.
- The new updater-related logic is now split between `bridge::commands` and `bridge::updater_types`; consider documenting or enforcing a clearer boundary (e.g., `commands` only orchestrates and `updater_types` only contains pure mapping structures) to keep future updater behavior changes localized.
## Individual Comments
### Comment 1
<location path="src-tauri/src/app_runtime.rs" line_range="32-41" />
<code_context>
+fn configure_window_events(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> {
</code_context>
<issue_to_address>
**issue (bug_risk):** The `event` value is moved in the first `match` and then used again later, which will not compile with the current `on_window_event` signature.
In `configure_window_events`, the closure is `|window, event|`, so `event` is a `WindowEvent` by value. `let action = match event { ... }` moves `event`, so the later `if let WindowEvent::CloseRequested { api, .. } = event` is a use-after-move and will not compile.
You can either:
- Match on a reference for the decision, then match by value only once when you need `api`, e.g.:
```rust
builder.on_window_event(|window, event| {
let is_quitting = window.app_handle().state::<BackendState>().is_quitting();
let action = match &event {
WindowEvent::CloseRequested { .. } => { /* ... */ }
WindowEvent::Focused(false) => { /* ... */ }
_ => app_runtime_events::MainWindowAction::None,
};
if let MainWindowAction::PreventCloseAndHide = action {
if let WindowEvent::CloseRequested { api, .. } = event { /* ... */ }
}
})
```
- Or compute any needed flags from `event` into locals before the first match so you only pattern-match on `event` once.
</issue_to_address>
### Comment 2
<location path="scripts/ci/generate-tauri-latest-json.py" line_range="11-12" />
<code_context>
+from pathlib import Path
+
+
+WINDOWS_RE = re.compile(r"(?P<name>.+?)-setup\.exe$")
+MACOS_RE = re.compile(r"(?P<name>.+?)_(?P<version>[^_]+)_macos_(?P<arch>[^.]+)\.zip$")
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The Windows regex is unused and Windows arch detection is based on a loose filename substring match.
`collect_platforms` currently infers Windows artifacts via filename suffix and substring matching:
```python
if sig_name.endswith(".exe.sig"):
exe_name = sig_name[:-4]
arch = "arm64" if "arm64" in exe_name.lower() else "amd64"
platform_key = platform_key_for_windows(arch)
```
while `WINDOWS_RE` is defined but never used. This both leaves a dead constant (linters will complain) and risks mis-detecting the arch if `arm64` appears in the product name or the naming convention changes.
Suggest parsing with the regex instead (similar to macOS), e.g. a pattern that explicitly captures `name` and `arch`, then mapping `arch` via `platform_key_for_windows`. That also lets you remove the unused `WINDOWS_RE`.
Suggested implementation:
```python
WINDOWS_RE = re.compile(r"(?P<name>.+?)-(?P<arch>[^-]+)-setup\.exe$")
```
```python
if sig_name.endswith(".exe.sig"):
exe_name = sig_name[:-4]
match = WINDOWS_RE.match(exe_name)
if not match:
raise ValueError(f"Unexpected Windows artifact name: {exe_name}")
arch = match.group("arch")
platform_key = platform_key_for_windows(arch)
```
</issue_to_address>
### Comment 3
<location path="scripts/prepare-resources.mjs" line_range="20" />
<code_context>
- throw new Error(`Unsupported mode: ${currentMode}. Expected version/webui/backend/all.`);
-};
-
const main = async () => {
- const sourceDir = resolveSourceDir(projectRoot, sourceDirOverrideRaw);
+ const context = createPrepareResourcesContext({
</code_context>
<issue_to_address>
**issue (complexity):** Consider passing the full context object into `runModeTasks` and only destructuring what `main` itself uses to avoid redundant wiring and context reshaping.
You can keep the new modular structure but reduce mental overhead by avoiding the large destructure/re-assemble pattern in `main`. Right now `main` unpacks almost the entire context, then selectively repacks fields for `runModeTasks`, which couples three places to the context shape.
Instead, destructure only what `main` truly needs, and pass the full `context` object to `runModeTasks` (and optionally to other helpers that can accept it). That keeps a single source of truth for the context shape and simplifies `main`:
```js
// prepare-resources.mjs
const main = async () => {
const context = createPrepareResourcesContext({
argv: process.argv,
env: process.env,
projectRoot,
});
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideInput,
desktopVersionInput,
desktopVersionOverride,
} = context;
const needsSourceRepo = mode !== 'version' || !desktopVersionOverride;
await mkdir(path.join(projectRoot, 'resources'), { recursive: true });
if (desktopVersionInput && desktopVersionInput !== desktopVersionOverride) {
console.log(
`[prepare-resources] Normalized ASTRBOT_DESKTOP_VERSION from ${desktopVersionInput} to ${desktopVersionOverride}`,
);
}
if (needsSourceRepo) {
ensureSourceRepo({
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideRaw: sourceDirOverrideInput,
});
} else {
console.log(
'[prepare-resources] Skip source repo sync in version-only mode because ASTRBOT_DESKTOP_VERSION is set.',
);
}
// ... rest of main unchanged ...
await runModeTasks(mode, context);
};
```
Then update `runModeTasks` to accept `context` instead of a manually curated subset:
```js
// prepare-resources/mode-dispatch.mjs
export const runModeTasks = async (mode, context) => {
const {
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
} = context;
// existing mode dispatch logic using these fields
};
```
This keeps the new testable abstractions (`createPrepareResourcesContext`, `runModeTasks`) while:
- Removing duplication of context field lists.
- Making `main` easier to scan by reducing the amount of wiring code.
- Keeping the context shape defined in exactly one place for consumers.
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- In
scripts/ci/generate-tauri-latest-json.py, theWINDOWS_REregex is never used and the Windows arch is inferred via a simple substring check; either remove the unused regex or switch to using it (similar to the macOS path) so the arch detection is explicit and less brittle to filename changes. - The new updater-related logic is now split between
bridge::commandsandbridge::updater_types; consider documenting or enforcing a clearer boundary (e.g.,commandsonly orchestrates andupdater_typesonly contains pure mapping structures) to keep future updater behavior changes localized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `scripts/ci/generate-tauri-latest-json.py`, the `WINDOWS_RE` regex is never used and the Windows arch is inferred via a simple substring check; either remove the unused regex or switch to using it (similar to the macOS path) so the arch detection is explicit and less brittle to filename changes.
- The new updater-related logic is now split between `bridge::commands` and `bridge::updater_types`; consider documenting or enforcing a clearer boundary (e.g., `commands` only orchestrates and `updater_types` only contains pure mapping structures) to keep future updater behavior changes localized.
## Individual Comments
### Comment 1
<location path="src-tauri/src/app_runtime.rs" line_range="32-41" />
<code_context>
+fn configure_window_events(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> {
</code_context>
<issue_to_address>
**issue (bug_risk):** The `event` value is moved in the first `match` and then used again later, which will not compile with the current `on_window_event` signature.
In `configure_window_events`, the closure is `|window, event|`, so `event` is a `WindowEvent` by value. `let action = match event { ... }` moves `event`, so the later `if let WindowEvent::CloseRequested { api, .. } = event` is a use-after-move and will not compile.
You can either:
- Match on a reference for the decision, then match by value only once when you need `api`, e.g.:
```rust
builder.on_window_event(|window, event| {
let is_quitting = window.app_handle().state::<BackendState>().is_quitting();
let action = match &event {
WindowEvent::CloseRequested { .. } => { /* ... */ }
WindowEvent::Focused(false) => { /* ... */ }
_ => app_runtime_events::MainWindowAction::None,
};
if let MainWindowAction::PreventCloseAndHide = action {
if let WindowEvent::CloseRequested { api, .. } = event { /* ... */ }
}
})
```
- Or compute any needed flags from `event` into locals before the first match so you only pattern-match on `event` once.
</issue_to_address>
### Comment 2
<location path="scripts/ci/generate-tauri-latest-json.py" line_range="11-12" />
<code_context>
+from pathlib import Path
+
+
+WINDOWS_RE = re.compile(r"(?P<name>.+?)-setup\.exe$")
+MACOS_RE = re.compile(r"(?P<name>.+?)_(?P<version>[^_]+)_macos_(?P<arch>[^.]+)\.zip$")
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The Windows regex is unused and Windows arch detection is based on a loose filename substring match.
`collect_platforms` currently infers Windows artifacts via filename suffix and substring matching:
```python
if sig_name.endswith(".exe.sig"):
exe_name = sig_name[:-4]
arch = "arm64" if "arm64" in exe_name.lower() else "amd64"
platform_key = platform_key_for_windows(arch)
```
while `WINDOWS_RE` is defined but never used. This both leaves a dead constant (linters will complain) and risks mis-detecting the arch if `arm64` appears in the product name or the naming convention changes.
Suggest parsing with the regex instead (similar to macOS), e.g. a pattern that explicitly captures `name` and `arch`, then mapping `arch` via `platform_key_for_windows`. That also lets you remove the unused `WINDOWS_RE`.
Suggested implementation:
```python
WINDOWS_RE = re.compile(r"(?P<name>.+?)-(?P<arch>[^-]+)-setup\.exe$")
```
```python
if sig_name.endswith(".exe.sig"):
exe_name = sig_name[:-4]
match = WINDOWS_RE.match(exe_name)
if not match:
raise ValueError(f"Unexpected Windows artifact name: {exe_name}")
arch = match.group("arch")
platform_key = platform_key_for_windows(arch)
```
</issue_to_address>
### Comment 3
<location path="scripts/prepare-resources.mjs" line_range="20" />
<code_context>
- throw new Error(`Unsupported mode: ${currentMode}. Expected version/webui/backend/all.`);
-};
-
const main = async () => {
- const sourceDir = resolveSourceDir(projectRoot, sourceDirOverrideRaw);
+ const context = createPrepareResourcesContext({
</code_context>
<issue_to_address>
**issue (complexity):** Consider passing the full context object into `runModeTasks` and only destructuring what `main` itself uses to avoid redundant wiring and context reshaping.
You can keep the new modular structure but reduce mental overhead by avoiding the large destructure/re-assemble pattern in `main`. Right now `main` unpacks almost the entire context, then selectively repacks fields for `runModeTasks`, which couples three places to the context shape.
Instead, destructure only what `main` truly needs, and pass the full `context` object to `runModeTasks` (and optionally to other helpers that can accept it). That keeps a single source of truth for the context shape and simplifies `main`:
```js
// prepare-resources.mjs
const main = async () => {
const context = createPrepareResourcesContext({
argv: process.argv,
env: process.env,
projectRoot,
});
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideInput,
desktopVersionInput,
desktopVersionOverride,
} = context;
const needsSourceRepo = mode !== 'version' || !desktopVersionOverride;
await mkdir(path.join(projectRoot, 'resources'), { recursive: true });
if (desktopVersionInput && desktopVersionInput !== desktopVersionOverride) {
console.log(
`[prepare-resources] Normalized ASTRBOT_DESKTOP_VERSION from ${desktopVersionInput} to ${desktopVersionOverride}`,
);
}
if (needsSourceRepo) {
ensureSourceRepo({
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideRaw: sourceDirOverrideInput,
});
} else {
console.log(
'[prepare-resources] Skip source repo sync in version-only mode because ASTRBOT_DESKTOP_VERSION is set.',
);
}
// ... rest of main unchanged ...
await runModeTasks(mode, context);
};
```
Then update `runModeTasks` to accept `context` instead of a manually curated subset:
```js
// prepare-resources/mode-dispatch.mjs
export const runModeTasks = async (mode, context) => {
const {
sourceDir,
projectRoot,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
} = context;
// existing mode dispatch logic using these fields
};
```
This keeps the new testable abstractions (`createPrepareResourcesContext`, `runModeTasks`) while:
- Removing duplication of context field lists.
- Making `main` easier to scan by reducing the amount of wiring code.
- Keeping the context shape defined in exactly one place for consumers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
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.
zouyonghe
changed the title
zouyonghe
changed the title
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我在这里给出了一些高层次的反馈：
- 后端重启流程目前被拆分在
backend::restart_strategy以及BackendState上的若干编排（orchestration）辅助函数之间；建议考虑将execute_graceful_restart_strategy和launch_backend_after_restart（或者一个轻量级的协调器）移动到 strategy 模块中，这样可以让BackendState更专注于状态本身，也让重启决策流程更易于理解和推理。 generate-tauri-latest-json.py脚本假设了特定的制品（artifact）命名模式，并抛出了相当笼统的ValueError；或许可以为每个正则表达式添加更清晰的错误信息，或者一段简短的映射说明注释，这样当将来制品命名发生变化导致生成器开始出错时，会更容易进行调试。
供 AI 智能体使用的提示（Prompt）
Please address the comments from this code review:
## Overall Comments
- The backend restart flow is now split between `backend::restart_strategy` and several orchestration helpers on `BackendState`; consider moving `execute_graceful_restart_strategy` and `launch_backend_after_restart` (or a thin coordinator) into the strategy module to keep `BackendState` more state-focused and make the restart decision flow easier to reason about.
- The `generate-tauri-latest-json.py` script assumes specific artifact naming patterns and emits fairly generic `ValueError`s; it may be worth adding clearer error messages or a short mapping comment for each regex so future changes to artifact naming are easier to debug when the generator starts failing.帮我变得更有用！请对每条评论点击 👍 或 👎，我会根据这些反馈来改进后续给你的代码审查。
Original comment in English
Hey - I've left some high level feedback:
- The backend restart flow is now split between
backend::restart_strategyand several orchestration helpers onBackendState; consider movingexecute_graceful_restart_strategyandlaunch_backend_after_restart(or a thin coordinator) into the strategy module to keepBackendStatemore state-focused and make the restart decision flow easier to reason about. - The
generate-tauri-latest-json.pyscript assumes specific artifact naming patterns and emits fairly genericValueErrors; it may be worth adding clearer error messages or a short mapping comment for each regex so future changes to artifact naming are easier to debug when the generator starts failing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The backend restart flow is now split between `backend::restart_strategy` and several orchestration helpers on `BackendState`; consider moving `execute_graceful_restart_strategy` and `launch_backend_after_restart` (or a thin coordinator) into the strategy module to keep `BackendState` more state-focused and make the restart decision flow easier to reason about.
- The `generate-tauri-latest-json.py` script assumes specific artifact naming patterns and emits fairly generic `ValueError`s; it may be worth adding clearer error messages or a short mapping comment for each regex so future changes to artifact naming are easier to debug when the generator starts failing.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Mar 6, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些整体层面的反馈：
desktop_updater_supported()的检查目前对 Windows/macOS 是硬编码的，而更新插件始终会被注册；建议集中管理平台/特性开关（例如通过配置或共享的 helper），从而避免受支持平台的逻辑与 Tauri 配置或未来的 Linux 支持产生偏差。scripts/ci/generate-tauri-latest-json.py脚本在遇到任意不匹配 Windows/macOS 正则表达式的构建产物名称时会失败；如果你预期未来会增加更多类型的构建产物，可能需要忽略未知文件（或仅发出警告而不是报错），以避免在新产物添加时导致整个发布任务失败。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `desktop_updater_supported()` check is currently hardcoded to Windows/macOS while the updater plugin is always registered; consider centralizing platform/feature gating (e.g., via config or shared helper) so supported-platform logic cannot drift from Tauri config or future Linux support.
- The `scripts/ci/generate-tauri-latest-json.py` script fails on any artifact name that doesn’t match the Windows/macOS regexes; if you expect additional asset types in the future, you may want to ignore unknown files (or warn instead of error) to avoid breaking the entire release job when new artifacts are added.
## Individual Comments
### Comment 1
<location path="src-tauri/src/backend_http.rs" line_range="242-251" />
<code_context>
+ struct TimeoutReader {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** TimeoutReader 测试 helper 应该遵守传入缓冲区的长度，以避免潜在的 panic。
在 `TimeoutReader::read` 中，`bytes.len()` 被直接用于切片边界，如果 `bytes.len() > buf.len()` 就可能导致 panic。当前的测试场景刚好掩盖了这个问题，但会使该 helper 变得脆弱。请将拷贝长度限制在 `buf.len()` 之内（例如 `let n = bytes.len().min(buf.len()); buf[..n].copy_from_slice(&bytes[..n]);`），以便在不同缓冲区大小下都能安全工作。
</issue_to_address>
### Comment 2
<location path="src-tauri/src/app_runtime.rs" line_range="11-21" />
<code_context>
+fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> {
+ let updater_builder = {
+ let mut builder = tauri_plugin_updater::Builder::new();
+ if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") {
+ let trimmed = pubkey.trim();
+ if !trimmed.is_empty() {
+ builder = builder.pubkey(trimmed);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion:** 静默忽略空/仅空白字符的更新公钥可能会让配置错误难以及时发现。
在 `configure_plugins` 中，当设置了环境变量 `ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY`，但裁剪后为空字符串时，更新器会在没有任何日志输出的情况下保持未配置状态。这会让配置错误变得难以排查（CI/环境看起来正确，但应用在运行时没有签名密钥）。建议在环境变量存在但标准化后为空，或更泛化地在没有应用公钥时记录一条警告，以便更早暴露这些问题。
```suggestion
fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> {
let updater_builder = {
let mut builder = tauri_plugin_updater::Builder::new();
if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") {
let trimmed = pubkey.trim();
if !trimmed.is_empty() {
builder = builder.pubkey(trimmed);
} else {
append_desktop_log(
"warning: ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY is set but empty after trimming; \
updater signing key not configured",
);
}
}
builder
};
```
</issue_to_address>
### Comment 3
<location path="scripts/prepare-resources.mjs" line_range="81" />
<code_context>
}
- await runModeTasks(mode, sourceDir);
+ await runModeTasks(mode, context);
};
</code_context>
<issue_to_address>
**issue (complexity):** 建议简化 `runModeTasks`，让它只接收所需的最小原始参数，而不是整个 context 对象，从而让 mode 分发逻辑更直接、更易读。
你可以保留新的 `createPrepareResourcesContext` helper 以增强可测试性，同时通过简化 `runModeTasks` 的签名和调用方式来减少模式处理中的间接层次。
当前 `prepare-resources.mjs` 同时对 `context` 进行解构，又把完整的 `context` 传给 `runModeTasks(mode, context)`，这让控制流更难追踪，也鼓励 `runModeTasks` 了解 context 中“所有的东西”。
建议把 `runModeTasks` 收紧为一个简单、显式的分发器，基于一些与旧的线性流程相对应的原始参数，同时仍然使用 context helper 来计算这些原始参数。
在 `prepare-resources.mjs` 中：
```js
// after createPrepareResourcesContext(...)
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideInput,
desktopVersionInput,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
} = context;
// ...
await runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
```
然后在 `mode-dispatch.mjs` 中，你可以去掉通用的 `taskRunner` 抽象，让 `runModeTasks` 成为一个非常直接的 switch：
```js
import { prepareWebui, prepareBackend } from './mode-tasks.mjs';
export async function runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
}) {
switch (mode) {
case 'version':
return;
case 'webui':
return prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
case 'backend':
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
case 'all':
await prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
default:
throw new Error(
`Unsupported mode: ${mode}. Expected version/webui/backend/all.`,
);
}
}
```
这样既保留了基于 context 的归一化（及其可测试性），又让 mode 分发逻辑变得扁平化，并移除了额外的 `taskRunner` 抽象，使得只需要快速浏览 `prepare-resources.mjs` 和 `mode-dispatch.mjs` 就能更容易看清每种 mode 实际在做什么。
</issue_to_address>Hi @zouyonghe! 👋
感谢你通过评论 @sourcery-ai review 来体验 Sourcery！🚀
安装 sourcery-ai 机器人 以在每一个 pull request 上获得自动代码审查 ✨
帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进之后的审查。Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The
desktop_updater_supported()check is currently hardcoded to Windows/macOS while the updater plugin is always registered; consider centralizing platform/feature gating (e.g., via config or shared helper) so supported-platform logic cannot drift from Tauri config or future Linux support. - The
scripts/ci/generate-tauri-latest-json.pyscript fails on any artifact name that doesn’t match the Windows/macOS regexes; if you expect additional asset types in the future, you may want to ignore unknown files (or warn instead of error) to avoid breaking the entire release job when new artifacts are added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `desktop_updater_supported()` check is currently hardcoded to Windows/macOS while the updater plugin is always registered; consider centralizing platform/feature gating (e.g., via config or shared helper) so supported-platform logic cannot drift from Tauri config or future Linux support.
- The `scripts/ci/generate-tauri-latest-json.py` script fails on any artifact name that doesn’t match the Windows/macOS regexes; if you expect additional asset types in the future, you may want to ignore unknown files (or warn instead of error) to avoid breaking the entire release job when new artifacts are added.
## Individual Comments
### Comment 1
<location path="src-tauri/src/backend_http.rs" line_range="242-251" />
<code_context>
+ struct TimeoutReader {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** TimeoutReader test helper should respect the provided buffer length to avoid potential panics.
In `TimeoutReader::read`, `bytes.len()` is used directly for the slice bounds, which can panic if `bytes.len() > buf.len()`. This is currently masked by the specific test setup but makes the helper brittle. Please cap the copy by `buf.len()` (e.g., `let n = bytes.len().min(buf.len()); buf[..n].copy_from_slice(&bytes[..n]);`) so it behaves safely with different buffer sizes.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/app_runtime.rs" line_range="11-21" />
<code_context>
+fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> {
+ let updater_builder = {
+ let mut builder = tauri_plugin_updater::Builder::new();
+ if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") {
+ let trimmed = pubkey.trim();
+ if !trimmed.is_empty() {
+ builder = builder.pubkey(trimmed);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion:** Silently ignoring an empty/whitespace-only updater public key may make misconfiguration hard to diagnose.
In `configure_plugins`, when `ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY` is set but trims to an empty string, the updater is left unconfigured with no log output. This makes misconfigurations hard to detect (CI/env look correct, but the app runs without a signing key). Please add a warning when the env var is present but normalizes to empty, or more generally when the pubkey is not applied, so these issues surface earlier.
```suggestion
fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> {
let updater_builder = {
let mut builder = tauri_plugin_updater::Builder::new();
if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") {
let trimmed = pubkey.trim();
if !trimmed.is_empty() {
builder = builder.pubkey(trimmed);
} else {
append_desktop_log(
"warning: ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY is set but empty after trimming; \
updater signing key not configured",
);
}
}
builder
};
```
</issue_to_address>
### Comment 3
<location path="scripts/prepare-resources.mjs" line_range="81" />
<code_context>
}
- await runModeTasks(mode, sourceDir);
+ await runModeTasks(mode, context);
};
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `runModeTasks` to accept only the minimal primitive parameters it needs instead of the full context object to make the mode dispatch logic more direct and readable.
You can keep the new `createPrepareResourcesContext` helper for testability while reducing indirection in the mode handling by simplifying `runModeTasks`’s signature and usage.
Right now `prepare-resources.mjs` both destructures `context` and passes the whole `context` to `runModeTasks(mode, context)`, which makes the control flow harder to trace and encourages `runModeTasks` to know “everything” about the context.
Consider tightening `runModeTasks` to a simple, explicit dispatcher over primitives that mirror the old linear flow, while still using the context helper to compute those primitives.
In `prepare-resources.mjs`:
```js
// after createPrepareResourcesContext(...)
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideInput,
desktopVersionInput,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
} = context;
// ...
await runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
```
Then in `mode-dispatch.mjs` you can drop the generic `taskRunner` abstraction and make `runModeTasks` a straightforward switch:
```js
import { prepareWebui, prepareBackend } from './mode-tasks.mjs';
export async function runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
}) {
switch (mode) {
case 'version':
return;
case 'webui':
return prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
case 'backend':
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
case 'all':
await prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
default:
throw new Error(
`Unsupported mode: ${mode}. Expected version/webui/backend/all.`,
);
}
}
```
This preserves the new context-based normalization (and its testability) but flattens the mode dispatch and removes the extra `taskRunner` abstraction, making it easier to see what each mode actually does from a quick scan of `prepare-resources.mjs` + `mode-dispatch.mjs`.
</issue_to_address>Hi @zouyonghe! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+242
to
+251
| struct TimeoutReader { | ||
| chunks: Vec<Result<&'static [u8], std::io::ErrorKind>>, | ||
| index: usize, | ||
| } | ||
|
|
||
| impl Read for TimeoutAfterPayloadReader { | ||
| fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> { | ||
| let bytes = self.payload.read(buf)?; | ||
| if bytes > 0 { | ||
| return Ok(bytes); | ||
| } | ||
| if self.timed_out { | ||
| return Ok(0); | ||
| impl Read for TimeoutReader { | ||
| fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> { | ||
| if self.index >= self.chunks.len() { | ||
| return Ok(0); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): TimeoutReader 测试 helper 应该遵守传入缓冲区的长度，以避免潜在的 panic。
在 TimeoutReader::read 中，bytes.len() 被直接用于切片边界，如果 bytes.len() > buf.len() 就可能导致 panic。当前的测试场景刚好掩盖了这个问题，但会使该 helper 变得脆弱。请将拷贝长度限制在 buf.len() 之内（例如 let n = bytes.len().min(buf.len()); buf[..n].copy_from_slice(&bytes[..n]);），以便在不同缓冲区大小下都能安全工作。
Original comment in English
suggestion (bug_risk): TimeoutReader test helper should respect the provided buffer length to avoid potential panics.
In TimeoutReader::read, bytes.len() is used directly for the slice bounds, which can panic if bytes.len() > buf.len(). This is currently masked by the specific test setup but makes the helper brittle. Please cap the copy by buf.len() (e.g., let n = bytes.len().min(buf.len()); buf[..n].copy_from_slice(&bytes[..n]);) so it behaves safely with different buffer sizes.
Comment on lines
+11
to
+21
| fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> { | ||
| let updater_builder = { | ||
| let mut builder = tauri_plugin_updater::Builder::new(); | ||
| if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") { | ||
| let trimmed = pubkey.trim(); | ||
| if !trimmed.is_empty() { | ||
| builder = builder.pubkey(trimmed); | ||
| } | ||
| } | ||
| builder | ||
| }; |
There was a problem hiding this comment.
suggestion: 静默忽略空/仅空白字符的更新公钥可能会让配置错误难以及时发现。
在 configure_plugins 中，当设置了环境变量 ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY，但裁剪后为空字符串时，更新器会在没有任何日志输出的情况下保持未配置状态。这会让配置错误变得难以排查（CI/环境看起来正确，但应用在运行时没有签名密钥）。建议在环境变量存在但标准化后为空，或更泛化地在没有应用公钥时记录一条警告，以便更早暴露这些问题。
Suggested change
| fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> { | |
| let updater_builder = { | |
| let mut builder = tauri_plugin_updater::Builder::new(); | |
| if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") { | |
| let trimmed = pubkey.trim(); | |
| if !trimmed.is_empty() { | |
| builder = builder.pubkey(trimmed); | |
| } | |
| } | |
| builder | |
| }; | |
| fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> { | |
| let updater_builder = { | |
| let mut builder = tauri_plugin_updater::Builder::new(); | |
| if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") { | |
| let trimmed = pubkey.trim(); | |
| if !trimmed.is_empty() { | |
| builder = builder.pubkey(trimmed); | |
| } else { | |
| append_desktop_log( | |
| "warning: ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY is set but empty after trimming; \ | |
| updater signing key not configured", | |
| ); | |
| } | |
| } | |
| builder | |
| }; |
Original comment in English
suggestion: Silently ignoring an empty/whitespace-only updater public key may make misconfiguration hard to diagnose.
In configure_plugins, when ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY is set but trims to an empty string, the updater is left unconfigured with no log output. This makes misconfigurations hard to detect (CI/env look correct, but the app runs without a signing key). Please add a warning when the env var is present but normalizes to empty, or more generally when the pubkey is not applied, so these issues surface earlier.
Suggested change
| fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> { | |
| let updater_builder = { | |
| let mut builder = tauri_plugin_updater::Builder::new(); | |
| if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") { | |
| let trimmed = pubkey.trim(); | |
| if !trimmed.is_empty() { | |
| builder = builder.pubkey(trimmed); | |
| } | |
| } | |
| builder | |
| }; | |
| fn configure_plugins(builder: Builder<tauri::Wry>) -> Builder<tauri::Wry> { | |
| let updater_builder = { | |
| let mut builder = tauri_plugin_updater::Builder::new(); | |
| if let Ok(pubkey) = std::env::var("ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY") { | |
| let trimmed = pubkey.trim(); | |
| if !trimmed.is_empty() { | |
| builder = builder.pubkey(trimmed); | |
| } else { | |
| append_desktop_log( | |
| "warning: ASTRBOT_DESKTOP_UPDATER_PUBLIC_KEY is set but empty after trimming; \ | |
| updater signing key not configured", | |
| ); | |
| } | |
| } | |
| builder | |
| }; |
| } | ||
|
|
||
| await runModeTasks(mode, sourceDir); | ||
| await runModeTasks(mode, context); |
There was a problem hiding this comment.
issue (complexity): 建议简化 runModeTasks，让它只接收所需的最小原始参数，而不是整个 context 对象，从而让 mode 分发逻辑更直接、更易读。
你可以保留新的 createPrepareResourcesContext helper 以增强可测试性，同时通过简化 runModeTasks 的签名和调用方式来减少模式处理中的间接层次。
当前 prepare-resources.mjs 同时对 context 进行解构，又把完整的 context 传给 runModeTasks(mode, context)，这让控制流更难追踪，也鼓励 runModeTasks 了解 context 中“所有的东西”。
建议把 runModeTasks 收紧为一个简单、显式的分发器，基于一些与旧的线性流程相对应的原始参数，同时仍然使用 context helper 来计算这些原始参数。
在 prepare-resources.mjs 中：
// after createPrepareResourcesContext(...)
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideInput,
desktopVersionInput,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
} = context;
// ...
await runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});然后在 mode-dispatch.mjs 中，你可以去掉通用的 taskRunner 抽象，让 runModeTasks 成为一个非常直接的 switch：
import { prepareWebui, prepareBackend } from './mode-tasks.mjs';
export async function runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
}) {
switch (mode) {
case 'version':
return;
case 'webui':
return prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
case 'backend':
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
case 'all':
await prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
default:
throw new Error(
`Unsupported mode: ${mode}. Expected version/webui/backend/all.`,
);
}
}这样既保留了基于 context 的归一化（及其可测试性），又让 mode 分发逻辑变得扁平化，并移除了额外的 taskRunner 抽象，使得只需要快速浏览 prepare-resources.mjs 和 mode-dispatch.mjs 就能更容易看清每种 mode 实际在做什么。
Original comment in English
issue (complexity): Consider simplifying runModeTasks to accept only the minimal primitive parameters it needs instead of the full context object to make the mode dispatch logic more direct and readable.
You can keep the new createPrepareResourcesContext helper for testability while reducing indirection in the mode handling by simplifying runModeTasks’s signature and usage.
Right now prepare-resources.mjs both destructures context and passes the whole context to runModeTasks(mode, context), which makes the control flow harder to trace and encourages runModeTasks to know “everything” about the context.
Consider tightening runModeTasks to a simple, explicit dispatcher over primitives that mirror the old linear flow, while still using the context helper to compute those primitives.
In prepare-resources.mjs:
// after createPrepareResourcesContext(...)
const {
mode,
sourceDir,
sourceRepoUrl,
sourceRepoRef,
isSourceRepoRefCommitSha,
sourceDirOverrideInput,
desktopVersionInput,
desktopVersionOverride,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
} = context;
// ...
await runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});Then in mode-dispatch.mjs you can drop the generic taskRunner abstraction and make runModeTasks a straightforward switch:
import { prepareWebui, prepareBackend } from './mode-tasks.mjs';
export async function runModeTasks(mode, {
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
}) {
switch (mode) {
case 'version':
return;
case 'webui':
return prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
case 'backend':
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
case 'all':
await prepareWebui({
projectRoot,
sourceDir,
sourceRepoRef,
isSourceRepoRefVersionTag,
isDesktopBridgeExpectationStrict,
});
return prepareBackend({
projectRoot,
sourceDir,
pythonBuildStandaloneRelease,
pythonBuildStandaloneVersion,
});
default:
throw new Error(
`Unsupported mode: ${mode}. Expected version/webui/backend/all.`,
);
}
}This preserves the new context-based normalization (and its testability) but flattens the mode dispatch and removes the extra taskRunner abstraction, making it easier to see what each mode actually does from a quick scan of prepare-resources.mjs + mode-dispatch.mjs.
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.
概述
这个 PR 将桌面端运行时代码从“顶层平铺模块”重组为按子系统划分的结构，同时补齐桌面应用自更新桥接能力，并保持现有脚本测试与 Rust 测试通过。
从用户视角看，这个改动主要有两部分：
window.astrbotAppUpdater，这个 PR 补上了桌面壳对应的 bridge、updater runtime 接入和 release 元数据生成骨架，解决“按钮存在但能力未实现”的错位问题。问题
1. 桌面运行时代码结构仍然偏平铺
仓库之前已经做过第一轮拆分，大文件问题基本解决，但 Rust runtime 依然主要以顶层模块平铺存在。这样虽然单个文件不算大，但会让以下问题持续存在：
2. WebUI 与桌面壳在“应用自更新”能力上已经错位
上游 WebUI 已经依赖
window.astrbotAppUpdater.checkForAppUpdate()/installAppUpdate()，但桌面壳此前并没有：window.astrbotAppUpdater这意味着当前“检查更新失败”不是偶发问题，而是功能尚未接通导致的必现失败。
根因
这次问题的根因有两个层面：
架构层面
上一轮重构主要解决“单文件过大”，但还没有完全建立“按子系统组织”的稳定架构。因此虽然文件拆开了，但整体导航、边界和后续扩展仍然不够顺手。
功能层面
桌面端的 app updater 入口已经被前端消费，但桌面壳还停留在只暴露
astrbotDesktop这类运行时/后端控制能力的阶段，尚未把自更新链路真正接通。变更内容
一、脚本侧整理
prepare-resources流程做了进一步职责收敛：scripts/prepare-resources/mode-dispatch.mjsscripts/prepare-resources/context.mjsscripts/prepare-resources/desktop-bridge-expectations.mjsrunModeTasks改成直接消费context，降低入口层 wiring 复杂度二、桌面 runtime 结构重组
Rust 侧 runtime 按子系统拆分并落到目录：
src-tauri/src/backend/src-tauri/src/bridge/src-tauri/src/lifecycle/src-tauri/src/tray/src-tauri/src/window/同时保留少量横切模块（如
app_runtime.rs、app_runtime_events.rs）来承载顶层编排与纯决策逻辑。三、重启与运行时 seam 收敛
为降低编排层复杂度，额外抽出了关键纯逻辑 seam：
src-tauri/src/app_runtime_events.rssrc-tauri/src/backend/restart_strategy.rs并根据 review 把 restart 的副作用解释逻辑收回
backend::restart调用点，减少额外 DSL 式抽象。四、桌面自更新能力接入
这个 PR 补上了桌面端 updater 的第一条真实能力链路：
tauri-plugin-updaterwindow.astrbotAppUpdaterbridge::updater_typeslatest.json生成脚本与 signing 相关 env 注入五、文档同步
补充并更新了以下文档：
docs/architecture.mddocs/repository-structure.mddocs/environment-variables.mddocs/plans/2026-03-06-architecture-refactor-design.mddocs/plans/2026-03-06-architecture-refactor.mddocs/plans/2026-03-06-desktop-updater-design.mddocs/plans/2026-03-06-desktop-updater.md对用户的影响
验证
我使用了以下检查来验证改动：
Rust
cargo fmt --manifest-path src-tauri/Cargo.toml --all -- --checkcargo clippy --manifest-path src-tauri/Cargo.toml --all-targets -- -D warningscargo test --manifest-path src-tauri/Cargo.toml --locked脚本
pnpm run test:prepare-resourcesnode --test scripts/prepare-resources/bridge-bootstrap-updater-contract.test.mjs当前相关测试均已通过。
说明
这个 PR 仍然以“架构收口 + updater 主链路接通”为主，不追求在这一轮里把所有发布与安装细节都做成零后续工作量的最终形态。
但相比之前“前端已接入口、桌面壳完全未实现”的状态，这个 PR 已经把结构、bridge、runtime、配置和 release 元数据骨架对齐到同一条线上，后续完善 updater release 资产即可继续向生产可用闭环推进。
Summary by Sourcery
将桌面运行时重构为以子系统为导向的 Rust 与脚本模块，在保持现有行为和测试完整的前提下，接入首条端到端桌面自更新路径。
New Features:
window.astrbotAppUpdater对象供 WebUI 使用。latest.json），使用已配置的更新端点和签名密钥。Enhancements:
backend、bridge、lifecycle、tray和window子系统，并在app_runtime_events.rs中集中处理事件决策逻辑。Build:
CI:
latest.json。Documentation:
Tests:
Original summary in English
Summary by Sourcery
Refactor the desktop runtime into subsystem-oriented Rust and script modules while wiring in a first end‑to‑end desktop self‑update path and keeping existing behavior and tests intact.
New Features:
window.astrbotAppUpdaterobject for WebUI use.latest.json) from the release workflow, using configured updater endpoints and signing keys.Enhancements:
backend,bridge,lifecycle,tray, andwindowsubsystems with centralized event decision logic inapp_runtime_events.rs.Build:
CI:
latest.jsonusing a new helper script.Documentation:
Tests:
Original summary in English
Summary by Sourcery
将桌面运行时重构为以子系统为导向的 Rust 与脚本模块，在保持现有行为和测试完整的前提下，接入首条端到端桌面自更新路径。
New Features:
window.astrbotAppUpdater对象供 WebUI 使用。latest.json），使用已配置的更新端点和签名密钥。Enhancements:
backend、bridge、lifecycle、tray和window子系统，并在app_runtime_events.rs中集中处理事件决策逻辑。Build:
CI:
latest.json。Documentation:
Tests:
Original summary in English
Summary by Sourcery
Refactor the desktop runtime into subsystem-oriented Rust and script modules while wiring in a first end‑to‑end desktop self‑update path and keeping existing behavior and tests intact.
New Features:
window.astrbotAppUpdaterobject for WebUI use.latest.json) from the release workflow, using configured updater endpoints and signing keys.Enhancements:
backend,bridge,lifecycle,tray, andwindowsubsystems with centralized event decision logic inapp_runtime_events.rs.Build:
CI:
latest.jsonusing a new helper script.Documentation:
Tests: