feat: improve plugin failure handling and extension list UX#5535
feat: improve plugin failure handling and extension list UX#5535Soulter merged 13 commits intoAstrBotDevs:masterfrom
Conversation
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并留下了一些整体性反馈:
- 隐藏作者列的响应式布局使用了硬编码的
nth-child(4)选择器(.plugin-list-table :deep(th:nth-child(4))),如果表头顺序发生变化,这会非常脆弱;建议改为通过 key/class 而不是位置索引来进行选择。 - 在
uninstallFailedPlugin中,你在 catch 块里调用的是toast(err, "error"),而其他处理函数会用String(err)来包装错误;统一改为使用String(err)可以避免出现[object Object]之类的输出,并保持错误信息展示的一致性。 - 代码在
StarManager.load中仍然构建了fail_rec,但现在忽略它而改用_rebuild_failed_plugin_info;建议删除或重构fail_rec,以避免维护两套并行的失败信息表示。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- 隐藏作者列的响应式布局使用了硬编码的 `nth-child(4)` 选择器(`.plugin-list-table :deep(th:nth-child(4))`),如果表头顺序发生变化,这会非常脆弱;建议改为通过 key/class 而不是位置索引来进行选择。
- 在 `uninstallFailedPlugin` 中,你在 catch 块里调用的是 `toast(err, "error")`,而其他处理函数会用 `String(err)` 来包装错误;统一改为使用 `String(err)` 可以避免出现 `[object Object]` 之类的输出,并保持错误信息展示的一致性。
- 代码在 `StarManager.load` 中仍然构建了 `fail_rec`,但现在忽略它而改用 `_rebuild_failed_plugin_info`;建议删除或重构 `fail_rec`,以避免维护两套并行的失败信息表示。
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="500" />
<code_context>
+ }
+ toast(res.data.message, "success");
+ await getExtensions();
+ } catch (err) {
+ toast(err, "error");
+ }
+ };
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Normalize error-to-string conversion when passing to `toast`.
这里你把原始的错误对象直接传给了 `toast`,而在其他地方(例如 `reloadFailedPlugin`、`reloadPlugin`)你使用的是 `String(err)`。根据 `toast` 的实现方式不同,这里可能会显示为 `[object Object]` 或者失败。请在这里也转换为字符串:`toast(String(err), "error")`。
```suggestion
await getExtensions();
} catch (err) {
toast(String(err), "error");
}
};
```
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈不断改进 Review 质量。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The responsive layout that hides the author column uses hardcoded
nth-child(4)selectors (.plugin-list-table :deep(th:nth-child(4))), which will be fragile if the table headers change order; consider targeting by key/class instead of positional index. - In
uninstallFailedPluginyou calltoast(err, "error")in the catch block, whereas other handlers wrap errors withString(err); normalizing this toString(err)will avoid[object Object]output and keep error messaging consistent. - The code still builds up
fail_recinStarManager.loadbut now ignores it in favor of_rebuild_failed_plugin_info; consider removing or refactoringfail_recto avoid maintaining two parallel representations of the same failure information.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The responsive layout that hides the author column uses hardcoded `nth-child(4)` selectors (`.plugin-list-table :deep(th:nth-child(4))`), which will be fragile if the table headers change order; consider targeting by key/class instead of positional index.
- In `uninstallFailedPlugin` you call `toast(err, "error")` in the catch block, whereas other handlers wrap errors with `String(err)`; normalizing this to `String(err)` will avoid `[object Object]` output and keep error messaging consistent.
- The code still builds up `fail_rec` in `StarManager.load` but now ignores it in favor of `_rebuild_failed_plugin_info`; consider removing or refactoring `fail_rec` to avoid maintaining two parallel representations of the same failure information.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="500" />
<code_context>
+ }
+ toast(res.data.message, "success");
+ await getExtensions();
+ } catch (err) {
+ toast(err, "error");
+ }
+ };
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Normalize error-to-string conversion when passing to `toast`.
Here you're passing the raw error object to `toast`, whereas in other places (e.g. `reloadFailedPlugin`, `reloadPlugin`) you use `String(err)`. Depending on `toast`'s implementation this might display as `[object Object]` or fail. Please convert to a string here as well: `toast(String(err), "error")`.
```suggestion
await getExtensions();
} catch (err) {
toast(String(err), "error");
}
};
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the summary. Please try again in a few hours by commenting |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
给 AI Agent 的提示
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="516-531" />
<code_context>
}
};
+
+ const reloadFailedPlugin = async (dirName) => {
+ if (!dirName) {
+ return;
+ }
+ try {
+ const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
+ if (res.data.status === "error") {
+ toast(res.data.message || "Reload failed", "error");
+ return;
+ }
+ setReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
+ await getExtensions();
+ } catch (err) {
+ toast(String(err), "error");
+ }
+ };
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 仅在成功时才设置重载反馈状态,这可能会在重载失败时留下已过期的成功消息。
`reloadFailedPlugin` 只会在成功时更新 `reloadFeedback`,在其他情况下只显示 toast。如果之前某次重载成功了,那么之后的某次失败仍然会在错误 toast 背后显示旧的成功反馈。为避免陈旧状态,建议在函数一开始就清空 `reloadFeedback`,以及/或者在出错分支中显式设置错误反馈,这样内联状态始终能反映最近一次尝试的结果。
```suggestion
const reloadFailedPlugin = async (dirName) => {
// Clear any existing reload feedback at the start of each attempt
setReloadFeedback("", "success", 0);
if (!dirName) {
return;
}
try {
const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
if (res.data.status === "error") {
const message = res.data.message || tm?.("messages.reloadError") || "Reload failed";
setReloadFeedback(message, "error");
toast(message, "error");
return;
}
setReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
const message = String(err) || tm?.("messages.reloadError") || "Reload failed";
setReloadFeedback(message, "error");
toast(message, "error");
}
};
```
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续的 Review。
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="516-531" />
<code_context>
}
};
+
+ const reloadFailedPlugin = async (dirName) => {
+ if (!dirName) {
+ return;
+ }
+ try {
+ const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
+ if (res.data.status === "error") {
+ toast(res.data.message || "Reload failed", "error");
+ return;
+ }
+ setReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
+ await getExtensions();
+ } catch (err) {
+ toast(String(err), "error");
+ }
+ };
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Reload feedback state is only set on success, which can leave a stale success message visible when reload fails.
`reloadFailedPlugin` only updates `reloadFeedback` on success and otherwise only shows toasts. If a previous reload succeeded, a later failure will still show the old success feedback behind the error toast. To avoid stale state, clear `reloadFeedback` at the start of the function and/or set an explicit error feedback in the error paths so the inline status always reflects the latest attempt.
```suggestion
const reloadFailedPlugin = async (dirName) => {
// Clear any existing reload feedback at the start of each attempt
setReloadFeedback("", "success", 0);
if (!dirName) {
return;
}
try {
const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
if (res.data.status === "error") {
const message = res.data.message || tm?.("messages.reloadError") || "Reload failed";
setReloadFeedback(message, "error");
toast(message, "error");
return;
}
setReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
const message = String(err) || tm?.("messages.reloadError") || "Reload failed";
setReloadFeedback(message, "error");
toast(message, "error");
}
};
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了两个问题,并给出了一些整体性的反馈:
- 在
useExtensionPage.js中,一些新的用户可见文案被直接硬编码为英文(例如reloadFailedPlugin和reloadPlugin里的 "Reload failed"),绕过了现有的 i18n 机制;建议将这些文案移动到扩展的 locale JSON 中,并使用tm(...)来保持一致性。 - 在
InstalledPluginsTab.vue中使用的新的失败插件相关 i18n key(例如failedPlugins.title,failedPlugins.columns.*)需要对照各语言的 locale JSON 文件再检查一遍,确保这些 key 都存在并在不同语言之间对齐,否则新的失败插件区域在运行时可能会直接渲染出原始 key。
给 AI Agent 的提示
请根据这次代码评审中的评论进行修改:
## 整体意见
- 在 `useExtensionPage.js` 中,一些新的用户可见文案被直接硬编码为英文(例如 `reloadFailedPlugin` 和 `reloadPlugin` 里的 "Reload failed"),绕过了现有的 i18n 机制;建议将这些文案移动到扩展的 locale JSON 中,并使用 `tm(...)` 来保持一致性。
- 在 `InstalledPluginsTab.vue` 中使用的新的失败插件相关 i18n key(例如 `failedPlugins.title`, `failedPlugins.columns.*`)需要对照各语言的 locale JSON 文件再检查一遍,确保这些 key 都存在并在不同语言之间对齐,否则新的失败插件区域在运行时可能会直接渲染出原始 key。
## 具体评论
### 评论 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="533-537" />
<code_context>
}
};
+
+ const reloadFailedPlugin = async (dirName) => {
+ // Reset stale inline feedback before each retry attempt
+ setReloadFeedback("", "success", 0);
+
+ if (!dirName) {
+ return;
+ }
+ try {
+ const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
+ if (res.data.status === "error") {
+ const message = res.data.message || "Reload failed";
+ setReloadFeedback(message, "error");
+ toast(message, "error");
+ return;
+ }
+ setReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
+ await getExtensions();
+ } catch (err) {
+ const message = String(err) || "Reload failed";
+ setReloadFeedback(message, "error");
+ toast(message, "error");
</code_context>
<issue_to_address>
**suggestion:** `reloadFailedPlugin` 中对错误的序列化可能会在 Axios 错误的情况下产生类似 `[object Object]` 这种没什么帮助的提示信息。
在 catch 代码块里,`String(err)` 在处理 Axios 错误时通常会被转成 `[object Object]`,从而隐藏掉诸如 `err.response?.data?.message` 这样的有用信息。建议将 `message` 的来源改为:`err.response?.data?.message || err.message || String(err)`,这样 toast 和行内反馈可以呈现更清晰的失败原因。
```suggestion
} catch (err) {
const message =
(err && err.response && err.response.data && err.response.data.message) ||
(err && err.message) ||
String(err) ||
"Reload failed";
setReloadFeedback(message, "error");
toast(message, "error");
}
```
</issue_to_address>
### 评论 2
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="165" />
<code_context>
// 卸载插件确认对话框(列表模式用)
const showUninstallDialog = ref(false);
const pluginToUninstall = ref(null);
+ const failedPluginToUninstall = ref(null);
</code_context>
<issue_to_address>
**issue (complexity):** 建议通过统一卸载状态、让失败卸载 API 只接收一个配置对象,以及把重载反馈的定时逻辑抽到一个小的工具函数里,来简化卸载和反馈处理,从而减少这个 composable 中的分支和协同复杂度。
**1. 简化卸载状态:用单一的 `uninstallTarget` 替代两个 ref**
现在你需要协调三块状态:
```js
const showUninstallDialog = ref(false);
const pluginToUninstall = ref(null);
const failedPluginToUninstall = ref(null);
```
然后在逻辑中分支:
```js
const handleUninstallConfirm = (options) => {
if (failedPluginToUninstall.value) {
uninstallFailedPlugin(failedPluginToUninstall.value, options);
failedPluginToUninstall.value = null;
pluginToUninstall.value = null;
return;
}
if (pluginToUninstall.value) {
uninstallExtension(pluginToUninstall.value, options);
pluginToUninstall.value = null;
}
};
```
你可以通过使用一个单一的目标对象来减少协调成本和潜在的不一致状态:
```js
// 替换:
const pluginToUninstall = ref(null);
const failedPluginToUninstall = ref(null);
// 为:
const uninstallTarget = ref(null);
// 结构:{ type: "normal" | "failed", id: string }
```
更新发起卸载的函数:
```js
const requestUninstallPlugin = (name) => {
if (!name) return;
uninstallTarget.value = { type: "normal", id: name };
showUninstallDialog.value = true;
};
const requestUninstallFailedPlugin = (dirName) => {
if (!dirName) return;
uninstallTarget.value = { type: "failed", id: dirName };
showUninstallDialog.value = true;
};
```
然后简化确认处理函数:
```js
const handleUninstallConfirm = (options) => {
const target = uninstallTarget.value;
if (!target) return;
if (target.type === "failed") {
uninstallFailedPlugin(target.id, options);
} else if (target.type === "normal") {
uninstallExtension(target.id, options);
}
uninstallTarget.value = null;
showUninstallDialog.value = false;
};
```
这样可以把对话框的协调逻辑集中在一个地方,不再需要在多个分支里手动将 `pluginToUninstall` 和 `failedPluginToUninstall` 置空。
---
**2. 让 `uninstallFailedPlugin` 的参数签名更明确,而不是多态**
目前这个“布尔值或配置对象”的参数会让函数更难读,也更容易被误用:
```js
const uninstallFailedPlugin = async (
dirName,
optionsOrSkipConfirm = false,
) => {
let deleteConfig = false;
let deleteData = false;
let skipConfirm = false;
if (typeof optionsOrSkipConfirm === "boolean") {
skipConfirm = optionsOrSkipConfirm;
} else if (typeof optionsOrSkipConfirm === "object" && optionsOrSkipConfirm !== null) {
deleteConfig = optionsOrSkipConfirm.deleteConfig || false;
deleteData = optionsOrSkipConfirm.deleteData || false;
skipConfirm = true;
}
if (!skipConfirm) {
requestUninstallFailedPlugin(dirName);
return;
}
// ...
};
```
你可以在保留所有行为的前提下,让 API 始终接受一个配置对象,从而让意图更清晰:
```js
// 类型:{ deleteConfig?: boolean; deleteData?: boolean; skipConfirm?: boolean }
const uninstallFailedPlugin = async (dirName, options = {}) => {
const {
deleteConfig = false,
deleteData = false,
skipConfirm = false,
} = options;
if (!skipConfirm) {
requestUninstallFailedPlugin(dirName);
return;
}
toast(tm("messages.uninstalling") + " " + dirName, "primary");
try {
const res = await axios.post("/api/plugin/uninstall-failed", {
dir_name: dirName,
delete_config: deleteConfig,
delete_data: deleteData,
});
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(String(err), "error");
}
};
```
已经有确认逻辑的调用点(例如卸载确认对话框)可以这样使用:
```js
handleUninstallConfirm = ({ deleteConfig, deleteData }) => {
const target = uninstallTarget.value;
if (!target) return;
if (target.type === "failed") {
uninstallFailedPlugin(target.id, {
deleteConfig,
deleteData,
skipConfirm: true,
});
}
// ...
};
```
这样可以移除函数内部对参数类型的分支判断,并让调用点的意图更加明确。
---
**3. 把重载反馈的定时逻辑抽取成一个小的 helper,而不是内联管理定时器状态**
当前逻辑把定时器管理直接写在这个 composable 里:
```js
const reloadFeedback = reactive({
message: "",
type: "success",
});
let reloadFeedbackTimer = null;
const setReloadFeedback = (message, type = "success", duration = 4000) => {
reloadFeedback.message = message || "";
reloadFeedback.type = type;
if (reloadFeedbackTimer) {
clearTimeout(reloadFeedbackTimer);
reloadFeedbackTimer = null;
}
if (duration > 0 && reloadFeedback.message) {
reloadFeedbackTimer = setTimeout(() => {
reloadFeedback.message = "";
}, duration);
}
};
onUnmounted(() => {
if (reloadFeedbackTimer) {
clearTimeout(reloadFeedbackTimer);
reloadFeedbackTimer = null;
}
});
```
你可以保留只在重载场景下使用的行内 UI,但把定时器处理封装进一个内部小 helper(或独立的 composable),从而缩小本文件的表面积:
```js
// 简单的内部 helper;之后也可以拆成单独的 composable
const useTimedMessage = (initialType = "success") => {
const state = reactive({
message: "",
type: initialType,
});
let timer = null;
const setMessage = (message, type = initialType, duration = 4000) => {
state.message = message || "";
state.type = type;
if (timer) clearTimeout(timer);
if (duration > 0 && state.message) {
timer = setTimeout(() => {
state.message = "";
}, duration);
}
};
onUnmounted(() => {
if (timer) clearTimeout(timer);
});
return { state, setMessage };
};
// 在当前 composable 中使用:
const { state: reloadFeedback, setMessage: setReloadFeedback } = useTimedMessage();
```
这样 `reloadFailedPlugin`、`reloadPlugin` 仍然可以继续使用 `reloadFeedback` 和 `setReloadFeedback`,但主 composable 不再需要手动管理 `reloadFeedbackTimer`,核心逻辑会更易于阅读和修改。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
useExtensionPage.js, several new user-facing strings are hardcoded in English (e.g. "Reload failed" inreloadFailedPluginandreloadPlugin), which bypasses the existing i18n setup; consider moving these to the extension locale JSON and usingtm(...)for consistency. - The new failed-plugin i18n keys used in
InstalledPluginsTab.vue(e.g.failedPlugins.title,failedPlugins.columns.*) should be double‑checked against the locale JSON files to ensure all keys exist and are aligned across languages, otherwise the new failed-plugin section may render raw keys at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useExtensionPage.js`, several new user-facing strings are hardcoded in English (e.g. "Reload failed" in `reloadFailedPlugin` and `reloadPlugin`), which bypasses the existing i18n setup; consider moving these to the extension locale JSON and using `tm(...)` for consistency.
- The new failed-plugin i18n keys used in `InstalledPluginsTab.vue` (e.g. `failedPlugins.title`, `failedPlugins.columns.*`) should be double‑checked against the locale JSON files to ensure all keys exist and are aligned across languages, otherwise the new failed-plugin section may render raw keys at runtime.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="533-537" />
<code_context>
}
};
+
+ const reloadFailedPlugin = async (dirName) => {
+ // Reset stale inline feedback before each retry attempt
+ setReloadFeedback("", "success", 0);
+
+ if (!dirName) {
+ return;
+ }
+ try {
+ const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
+ if (res.data.status === "error") {
+ const message = res.data.message || "Reload failed";
+ setReloadFeedback(message, "error");
+ toast(message, "error");
+ return;
+ }
+ setReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
+ await getExtensions();
+ } catch (err) {
+ const message = String(err) || "Reload failed";
+ setReloadFeedback(message, "error");
+ toast(message, "error");
</code_context>
<issue_to_address>
**suggestion:** Error serialization in `reloadFailedPlugin` may produce unhelpful messages like `[object Object]` for Axios errors.
In the catch block, `String(err)` will often stringify Axios errors as `[object Object]`, hiding useful details like `err.response?.data?.message`. Consider deriving `message` as `err.response?.data?.message || err.message || String(err)` so the toast and inline feedback surface a clearer failure reason.
```suggestion
} catch (err) {
const message =
(err && err.response && err.response.data && err.response.data.message) ||
(err && err.message) ||
String(err) ||
"Reload failed";
setReloadFeedback(message, "error");
toast(message, "error");
}
```
</issue_to_address>
### Comment 2
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="165" />
<code_context>
// 卸载插件确认对话框(列表模式用)
const showUninstallDialog = ref(false);
const pluginToUninstall = ref(null);
+ const failedPluginToUninstall = ref(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying uninstall and feedback handling by unifying uninstall state, making the failed-uninstall API accept a single options object, and extracting reload feedback timing into a small helper to reduce branching and coordination logic in this composable.
**1. Simplify uninstall state: replace dual refs with a single `uninstallTarget`**
Right now you’re coordinating three pieces of state:
```js
const showUninstallDialog = ref(false);
const pluginToUninstall = ref(null);
const failedPluginToUninstall = ref(null);
```
and then branching:
```js
const handleUninstallConfirm = (options) => {
if (failedPluginToUninstall.value) {
uninstallFailedPlugin(failedPluginToUninstall.value, options);
failedPluginToUninstall.value = null;
pluginToUninstall.value = null;
return;
}
if (pluginToUninstall.value) {
uninstallExtension(pluginToUninstall.value, options);
pluginToUninstall.value = null;
}
};
```
You can reduce coordination and accidental inconsistent state by using a single target object:
```js
// replace:
const pluginToUninstall = ref(null);
const failedPluginToUninstall = ref(null);
// with:
const uninstallTarget = ref(null);
// shape: { type: "normal" | "failed", id: string }
```
Update the request functions:
```js
const requestUninstallPlugin = (name) => {
if (!name) return;
uninstallTarget.value = { type: "normal", id: name };
showUninstallDialog.value = true;
};
const requestUninstallFailedPlugin = (dirName) => {
if (!dirName) return;
uninstallTarget.value = { type: "failed", id: dirName };
showUninstallDialog.value = true;
};
```
Then simplify the confirm handler:
```js
const handleUninstallConfirm = (options) => {
const target = uninstallTarget.value;
if (!target) return;
if (target.type === "failed") {
uninstallFailedPlugin(target.id, options);
} else if (target.type === "normal") {
uninstallExtension(target.id, options);
}
uninstallTarget.value = null;
showUninstallDialog.value = false;
};
```
This keeps the dialog coordination logic in one place and removes the need to manually null both `pluginToUninstall` and `failedPluginToUninstall` in multiple branches.
---
**2. Make `uninstallFailedPlugin` signature explicit instead of polymorphic**
The boolean-or-options parameter makes the function harder to read and easy to misuse:
```js
const uninstallFailedPlugin = async (
dirName,
optionsOrSkipConfirm = false,
) => {
let deleteConfig = false;
let deleteData = false;
let skipConfirm = false;
if (typeof optionsOrSkipConfirm === "boolean") {
skipConfirm = optionsOrSkipConfirm;
} else if (typeof optionsOrSkipConfirm === "object" && optionsOrSkipConfirm !== null) {
deleteConfig = optionsOrSkipConfirm.deleteConfig || false;
deleteData = optionsOrSkipConfirm.deleteData || false;
skipConfirm = true;
}
if (!skipConfirm) {
requestUninstallFailedPlugin(dirName);
return;
}
// ...
};
```
You can keep all behavior and make the API clearer by always accepting an options object:
```js
// type: { deleteConfig?: boolean; deleteData?: boolean; skipConfirm?: boolean }
const uninstallFailedPlugin = async (dirName, options = {}) => {
const {
deleteConfig = false,
deleteData = false,
skipConfirm = false,
} = options;
if (!skipConfirm) {
requestUninstallFailedPlugin(dirName);
return;
}
toast(tm("messages.uninstalling") + " " + dirName, "primary");
try {
const res = await axios.post("/api/plugin/uninstall-failed", {
dir_name: dirName,
delete_config: deleteConfig,
delete_data: deleteData,
});
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(String(err), "error");
}
};
```
Call sites that already have confirmation (like the uninstall dialog) then use:
```js
handleUninstallConfirm = ({ deleteConfig, deleteData }) => {
const target = uninstallTarget.value;
if (!target) return;
if (target.type === "failed") {
uninstallFailedPlugin(target.id, {
deleteConfig,
deleteData,
skipConfirm: true,
});
}
// ...
};
```
This removes the type-branching inside the function and makes intent at call sites explicit.
---
**3. Extract reload feedback timing into a tiny helper instead of inlining timer state**
Current logic mixes timer management into the composable:
```js
const reloadFeedback = reactive({
message: "",
type: "success",
});
let reloadFeedbackTimer = null;
const setReloadFeedback = (message, type = "success", duration = 4000) => {
reloadFeedback.message = message || "";
reloadFeedback.type = type;
if (reloadFeedbackTimer) {
clearTimeout(reloadFeedbackTimer);
reloadFeedbackTimer = null;
}
if (duration > 0 && reloadFeedback.message) {
reloadFeedbackTimer = setTimeout(() => {
reloadFeedback.message = "";
}, duration);
}
};
onUnmounted(() => {
if (reloadFeedbackTimer) {
clearTimeout(reloadFeedbackTimer);
reloadFeedbackTimer = null;
}
});
```
You can keep the inline reload-only UI but encapsulate timer handling in a small internal helper (or separate composable) to shrink this file’s surface area:
```js
// simple internal helper; can be moved to its own composable later
const useTimedMessage = (initialType = "success") => {
const state = reactive({
message: "",
type: initialType,
});
let timer = null;
const setMessage = (message, type = initialType, duration = 4000) => {
state.message = message || "";
state.type = type;
if (timer) clearTimeout(timer);
if (duration > 0 && state.message) {
timer = setTimeout(() => {
state.message = "";
}, duration);
}
};
onUnmounted(() => {
if (timer) clearTimeout(timer);
});
return { state, setMessage };
};
// usage inside this composable:
const { state: reloadFeedback, setMessage: setReloadFeedback } = useTimedMessage();
```
Then the reload flows (`reloadFailedPlugin`, `reloadPlugin`) can keep using `reloadFeedback` and `setReloadFeedback`, but the main composable no longer manually manages `reloadFeedbackTimer`, making the core logic easier to scan and change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="464-469" />
<code_context>
snack_show.value = true;
snack_success.value = success;
};
+ const resolveErrorMessage = (err, fallbackMessage = "") => {
+ return (
+ err?.response?.data?.message ||
+ err?.message ||
+ String(err) ||
+ fallbackMessage
+ );
+ };
</code_context>
<issue_to_address>
**issue (bug_risk):** `fallbackMessage` 参数实际上几乎不会被使用,因为 `String(err)` 始终为 truthy。
在当前实现中,`String(err)` 总会返回一个 truthy 值(例如 `String(undefined)` → `'undefined'`),因此永远不会走到 `fallbackMessage` 这个分支。
如果你希望在没有有意义的错误信息时使用 fallback,可以考虑只在 `err != null` 时调用 `String(err)`,以及在回退之前过滤掉明显无用的值,比如 `'[object Object]'`、`'undefined'` 和 `'null'`:
```js
const resolveErrorMessage = (err, fallbackMessage = "") => {
const fromResponse = err?.response?.data?.message;
const fromError = err?.message;
const raw = err != null ? String(err) : "";
const fromString = ["[object Object]", "undefined", "null"].includes(raw)
? ""
: raw;
return fromResponse || fromError || fromString || fallbackMessage;
};
```
</issue_to_address>
### Comment 2
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="11" />
<code_context>
import { useRoute, useRouter } from "vue-router";
+import { useDisplay } from "vuetify";
+
+const useTimedMessage = (initialType = "success") => {
+ const state = reactive({
+ message: "",
</code_context>
<issue_to_address>
**issue (complexity):** 建议抽取通用的辅助方法,并统一卸载逻辑的处理方式,使 `useExtensionPage` 的职责更聚焦,也更容易理解。
- `useTimedMessage` 和 `resolveErrorMessage` 都是通用逻辑,放在这里会增加该文件的职责。你可以在保持当前行为不变的前提下,将它们抽取出去,从而简化这个文件:
```js
// src/composables/useTimedMessage.js
import { reactive } from "vue";
export const useTimedMessage = (initialType = "success") => {
const state = reactive({ message: "", type: initialType });
let timer = null;
const clearTimer = () => {
if (timer) {
clearTimeout(timer);
timer = null;
}
};
const setMessage = (message, type = initialType, duration = 4000) => {
state.message = message || "";
state.type = type;
clearTimer();
if (duration > 0 && state.message) {
timer = setTimeout(() => {
state.message = "";
}, duration);
}
};
return { state, setMessage, clearTimer };
};
```
```js
// src/utils/errorUtils.js
export const resolveErrorMessage = (err, fallback = "") =>
err?.response?.data?.message ||
err?.message ||
String(err) ||
fallback;
```
```js
// useExtensionPage.js
import { useTimedMessage } from "@/composables/useTimedMessage";
import { resolveErrorMessage } from "@/utils/errorUtils";
```
- `reloadFeedback` 目前直接暴露了组合式函数的内部结构(`reloadFeedback.message = ""`)。如果把 `state` 当作只读,并新增一个清理方法,调用端会更简单,也无需了解内部结构:
```js
// in useTimedMessage.js
const clearMessage = () => {
state.message = "";
clearTimer();
};
return { state, setMessage, clearMessage, clearTimer };
```
```js
// in useExtensionPage
const {
state: reloadFeedback,
setMessage: setReloadFeedback,
clearMessage: clearReloadFeedback,
clearTimer: clearReloadFeedbackTimer,
} = useTimedMessage();
const reloadPlugin = async (plugin_name) => {
try {
clearReloadFeedback();
const res = await axios.post("/api/plugin/reload", { name: plugin_name });
if (res.data.status === "error") {
const msg = res.data.message || tm("messages.reloadFailed");
setReloadFeedback(msg, "error");
toast(msg, "error");
return;
}
setReloadFeedback(tm("messages.reloadSuccess"), "success");
toast(tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
const msg = resolveErrorMessage(err, tm("messages.reloadFailed"));
setReloadFeedback(msg, "error");
toast(msg, "error");
}
};
```
- 卸载流程目前有多个入口(`requestUninstallPlugin`、`requestUninstallFailedPlugin`),并且 `uninstallFailedPlugin` 通过 `skipConfirm` 同时承担了“请求确认”和“执行卸载”两种职责。你可以在保留当前共享弹窗和所有行为的前提下,让流程更具声明性:
```js
// unify target shape
const uninstallTarget = ref(null); // { kind: "normal" | "failed", id }
const requestUninstall = (kind, id) => {
if (!id) return;
uninstallTarget.value = { kind, id };
showUninstallDialog.value = true;
};
const performUninstall = async (target, options) => {
if (target.kind === "failed") {
const { deleteConfig = false, deleteData = false } = options || {};
toast(tm("messages.uninstalling") + " " + target.id, "primary");
const res = await axios.post("/api/plugin/uninstall-failed", {
dir_name: target.id,
delete_config: deleteConfig,
delete_data: deleteData,
});
// handle res (same logic as current uninstallFailedPlugin)
return;
}
return uninstallExtension(target.id, options); // reuse existing logic
};
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
await performUninstall(target, options);
uninstallTarget.value = null;
showUninstallDialog.value = false;
};
```
然后:
```js
// call sites
const requestUninstallPlugin = (name) => requestUninstall("normal", name);
const requestUninstallFailedPlugin = (dirName) => requestUninstall("failed", dirName);
```
这样可以从底层 API 中移除 `skipConfirm` 分支,并将“执行卸载”的逻辑集中在单一位置。
</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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="464-469" />
<code_context>
snack_show.value = true;
snack_success.value = success;
};
+ const resolveErrorMessage = (err, fallbackMessage = "") => {
+ return (
+ err?.response?.data?.message ||
+ err?.message ||
+ String(err) ||
+ fallbackMessage
+ );
+ };
</code_context>
<issue_to_address>
**issue (bug_risk):** The fallbackMessage parameter is effectively never used because `String(err)` is always truthy.
Given this implementation, `String(err)` will always yield a truthy value (e.g. `String(undefined)` → `'undefined'`), so the `fallbackMessage` branch is never reached.
If you want the fallback to be used when there’s no meaningful message, consider only using `String(err)` when `err != null`, and/or filtering out obviously unhelpful values like `'[object Object]'`, `'undefined'`, and `'null'` before falling back:
```js
const resolveErrorMessage = (err, fallbackMessage = "") => {
const fromResponse = err?.response?.data?.message;
const fromError = err?.message;
const raw = err != null ? String(err) : "";
const fromString = ["[object Object]", "undefined", "null"].includes(raw)
? ""
: raw;
return fromResponse || fromError || fromString || fallbackMessage;
};
```
</issue_to_address>
### Comment 2
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="11" />
<code_context>
import { useRoute, useRouter } from "vue-router";
+import { useDisplay } from "vuetify";
+
+const useTimedMessage = (initialType = "success") => {
+ const state = reactive({
+ message: "",
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the generic helpers and unifying uninstall handling to keep useExtensionPage focused and simpler to reason about.
- `useTimedMessage` and `resolveErrorMessage` are both generic and increase the responsibility of this file. You can keep all behavior while simplifying this file by extracting them:
```js
// src/composables/useTimedMessage.js
import { reactive } from "vue";
export const useTimedMessage = (initialType = "success") => {
const state = reactive({ message: "", type: initialType });
let timer = null;
const clearTimer = () => {
if (timer) {
clearTimeout(timer);
timer = null;
}
};
const setMessage = (message, type = initialType, duration = 4000) => {
state.message = message || "";
state.type = type;
clearTimer();
if (duration > 0 && state.message) {
timer = setTimeout(() => {
state.message = "";
}, duration);
}
};
return { state, setMessage, clearTimer };
};
```
```js
// src/utils/errorUtils.js
export const resolveErrorMessage = (err, fallback = "") =>
err?.response?.data?.message ||
err?.message ||
String(err) ||
fallback;
```
```js
// useExtensionPage.js
import { useTimedMessage } from "@/composables/useTimedMessage";
import { resolveErrorMessage } from "@/utils/errorUtils";
```
- `reloadFeedback` currently leaks the composable’s internal structure (`reloadFeedback.message = ""`). If you treat `state` as read-only and add a clear helper, call sites become simpler and you don’t need to know the underlying shape:
```js
// in useTimedMessage.js
const clearMessage = () => {
state.message = "";
clearTimer();
};
return { state, setMessage, clearMessage, clearTimer };
```
```js
// in useExtensionPage
const {
state: reloadFeedback,
setMessage: setReloadFeedback,
clearMessage: clearReloadFeedback,
clearTimer: clearReloadFeedbackTimer,
} = useTimedMessage();
const reloadPlugin = async (plugin_name) => {
try {
clearReloadFeedback();
const res = await axios.post("/api/plugin/reload", { name: plugin_name });
if (res.data.status === "error") {
const msg = res.data.message || tm("messages.reloadFailed");
setReloadFeedback(msg, "error");
toast(msg, "error");
return;
}
setReloadFeedback(tm("messages.reloadSuccess"), "success");
toast(tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
const msg = resolveErrorMessage(err, tm("messages.reloadFailed"));
setReloadFeedback(msg, "error");
toast(msg, "error");
}
};
```
- The uninstall flow now has multiple entry points (`requestUninstallPlugin`, `requestUninstallFailedPlugin`) and `uninstallFailedPlugin` mixes “ask for confirmation” and “perform uninstall” via `skipConfirm`. You can keep the shared dialog and all current behavior while making the flow more declarative:
```js
// unify target shape
const uninstallTarget = ref(null); // { kind: "normal" | "failed", id }
const requestUninstall = (kind, id) => {
if (!id) return;
uninstallTarget.value = { kind, id };
showUninstallDialog.value = true;
};
const performUninstall = async (target, options) => {
if (target.kind === "failed") {
const { deleteConfig = false, deleteData = false } = options || {};
toast(tm("messages.uninstalling") + " " + target.id, "primary");
const res = await axios.post("/api/plugin/uninstall-failed", {
dir_name: target.id,
delete_config: deleteConfig,
delete_data: deleteData,
});
// handle res (same logic as current uninstallFailedPlugin)
return;
}
return uninstallExtension(target.id, options); // reuse existing logic
};
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
await performUninstall(target, options);
uninstallTarget.value = null;
showUninstallDialog.value = false;
};
```
And then:
```js
// call sites
const requestUninstallPlugin = (name) => requestUninstall("normal", name);
const requestUninstallFailedPlugin = (dirName) => requestUninstall("failed", dirName);
```
This removes the `skipConfirm` branch from the lower-level API and centralizes the “perform uninstall” logic at a single point.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 2 个问题
面向 AI 代理的提示
Please address the comments from this code review:
## 单条评论
### 评论 1
<location path="astrbot/core/star/star_manager.py" line_range="460-468" />
<code_context>
+
+ return record
+
+ def _rebuild_failed_plugin_info(self) -> None:
+ if not self.failed_plugin_dict:
+ self.failed_plugin_info = ""
+ return
+
+ lines = []
+ for dir_name, info in self.failed_plugin_dict.items():
+ if isinstance(info, dict):
+ error = info.get("error", "未知错误")
+ else:
+ error = str(info)
+ lines.append(f"加载 {dir_name} 插件时出现问题,原因 {error}。")
+
+ self.failed_plugin_info = "\n".join(lines) + "\n"
+
async def reload_failed_plugin(self, dir_name):
</code_context>
<issue_to_address>
**suggestion:** `failed_plugin_info` 可以暴露更丰富的元数据,以便更好地进行诊断。
由于 `_build_failed_plugin_record` 现在会加入 `display_name`、`name` 以及其他元数据,建议在这里也利用这些信息,而不是只包含 `dir_name` 和 `error`。例如,在有 `display_name` 时优先使用它(并可选地包含版本),这样 `failed_plugin_info` 对用户来说会更容易和控制台里看到的内容对应起来。
```suggestion
lines = []
for dir_name, info in self.failed_plugin_dict.items():
# info 在 _build_failed_plugin_record 中通常是一个包含插件元数据和 error 的字典
if isinstance(info, dict):
error = info.get("error", "未知错误")
# 优先使用 display_name,其次使用 name,最后退回到目录名
display_name = info.get("display_name") or info.get("name") or dir_name
# 版本信息是可选的
version = info.get("version") or info.get("astrbot_version")
if version:
lines.append(
f"加载插件「{display_name}」(目录: {dir_name}, 版本: {version}) 时出现问题,原因:{error}。"
)
else:
lines.append(
f"加载插件「{display_name}」(目录: {dir_name}) 时出现问题,原因:{error}。"
)
else:
# 兼容历史/异常数据结构,仅有错误信息和目录名
error = str(info)
lines.append(f"加载插件目录 {dir_name} 时出现问题,原因:{error}。")
self.failed_plugin_info = "\n".join(lines) + "\n"
```
</issue_to_address>
### 评论 2
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="166" />
<code_context>
// 卸载插件确认对话框(列表模式用)
const showUninstallDialog = ref(false);
- const pluginToUninstall = ref(null);
</code_context>
<issue_to_address>
**issue (complexity):** 建议通过拆分为更清晰、按类型区分的 helper,并集中处理反馈逻辑,而不是使用基于 `kind` 的通用分发器和重载参数,来简化卸载与“重载失败插件”这两条流程。
### 1. 卸载流程:移除 `kind` 间接层 + 重载参数
当前的卸载流程包含了很多分支:
- `requestUninstall*` 只是构造 `{ kind, id }` 的包装
- `uninstallExtension` 让 `optionsOrSkipConfirm` 既可能是布尔值又可能是对象(重载)
- `performUninstall` 再次基于 `target.kind` 分支
在保持现有交互体验(UX)和 API 形式不变的前提下,可以通过以下方式简化:
1. 为不同 kind 提供显式的卸载入口函数。
2. 使用清晰的参数签名:`{ deleteConfig, deleteData }` 加上 `skipConfirm` 标记。
3. 让底层执行 “post + toast + refresh” 的 helper 不需要了解 `kind`。
下面是一个更线性的结构示例,保留了现有的全部能力:
```js
// 1) 状态对 UI 来说仍然是足够通用的
const uninstallTarget = ref(null); // { kind: 'normal' | 'failed', id: string } | null
const showUninstallDialog = ref(false);
const openUninstallDialog = (target) => {
uninstallTarget.value = target;
showUninstallDialog.value = true;
};
// 2) UI 使用这些函数,而不是 requestUninstallXXX
const uninstallNormalPlugin = (name, { skipConfirm = false, ...options } = {}) => {
if (!name) return;
if (!skipConfirm) {
openUninstallDialog({ kind: "normal", id: name });
return;
}
return doUninstallNormal(name, options);
};
const uninstallFailedPlugin = (dirName, { skipConfirm = false, ...options } = {}) => {
if (!dirName) return;
if (!skipConfirm) {
openUninstallDialog({ kind: "failed", id: dirName });
return;
}
return doUninstallFailed(dirName, options);
};
// 3) 针对不同类型的专用 helper,无需通用 target/分支逻辑
const doUninstallNormal = async (name, { deleteConfig = false, deleteData = false } = {}) => {
toast(`${tm("messages.uninstalling")} ${name}`, "primary");
try {
const res = await axios.post("/api/plugin/uninstall", {
name,
delete_config: deleteConfig,
delete_data: deleteData,
});
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
Object.assign(extension_data, res.data);
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
const doUninstallFailed = async (dirName, { deleteConfig = false, deleteData = false } = {}) => {
toast(`${tm("messages.uninstalling")} ${dirName}`, "primary");
try {
const res = await axios.post("/api/plugin/uninstall-failed", {
dir_name: dirName,
delete_config: deleteConfig,
delete_data: deleteData,
});
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
这样,确认处理函数会变得非常直接,不再需要区分布尔值和对象:
```js
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
if (target.kind === "normal") {
await doUninstallNormal(target.id, options);
} else {
await doUninstallFailed(target.id, options);
}
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
这可以移除:
- `performUninstall(target, options)`
- `requestUninstall`, `requestUninstallPlugin`, `requestUninstallFailedPlugin`
- `optionsOrSkipConfirm` 的重载及其类型复杂度
……同时保留当前的所有行为(正常/失败插件、选项、确认对话框)。
### 2. 重载反馈:集中 toast 与行内反馈
目前 `reloadFailedPlugin` 同时管理 `reloadFeedback` 和 toast,并存在重复代码。你可以保留这两种反馈方式,但通过一个小 helper 来减少重复:
```js
const showReloadFeedback = (message, type) => {
clearReloadFeedback();
setReloadFeedback(message, type);
toast(message, type === "success" ? "success" : "error");
};
const reloadFailedPlugin = async (dirName) => {
if (!dirName) return;
clearReloadFeedback();
try {
const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
if (res.data.status === "error") {
showReloadFeedback(res.data.message || tm("messages.reloadFailed"), "error");
return;
}
showReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
showReloadFeedback(resolveErrorMessage(err, tm("messages.reloadFailed")), "error");
}
};
```
这样既保留了行内反馈和 toast,又简化了流程,使意图更清晰。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/star/star_manager.py" line_range="460-468" />
<code_context>
+
+ return record
+
+ def _rebuild_failed_plugin_info(self) -> None:
+ if not self.failed_plugin_dict:
+ self.failed_plugin_info = ""
+ return
+
+ lines = []
+ for dir_name, info in self.failed_plugin_dict.items():
+ if isinstance(info, dict):
+ error = info.get("error", "未知错误")
+ else:
+ error = str(info)
+ lines.append(f"加载 {dir_name} 插件时出现问题,原因 {error}。")
+
+ self.failed_plugin_info = "\n".join(lines) + "\n"
+
async def reload_failed_plugin(self, dir_name):
</code_context>
<issue_to_address>
**suggestion:** `failed_plugin_info` could surface richer metadata for better diagnostics.
Since `_build_failed_plugin_record` now adds `display_name`, `name`, and other metadata, consider using those here instead of only `dir_name` and `error`. For example, prefer `display_name` when present (and optionally include the version) so `failed_plugin_info` is easier for users to match with what they see in the dashboard.
```suggestion
lines = []
for dir_name, info in self.failed_plugin_dict.items():
# info 在 _build_failed_plugin_record 中通常是一个包含插件元数据和 error 的字典
if isinstance(info, dict):
error = info.get("error", "未知错误")
# 优先使用 display_name,其次使用 name,最后退回到目录名
display_name = info.get("display_name") or info.get("name") or dir_name
# 版本信息是可选的
version = info.get("version") or info.get("astrbot_version")
if version:
lines.append(
f"加载插件「{display_name}」(目录: {dir_name}, 版本: {version}) 时出现问题,原因:{error}。"
)
else:
lines.append(
f"加载插件「{display_name}」(目录: {dir_name}) 时出现问题,原因:{error}。"
)
else:
# 兼容历史/异常数据结构,仅有错误信息和目录名
error = str(info)
lines.append(f"加载插件目录 {dir_name} 时出现问题,原因:{error}。")
self.failed_plugin_info = "\n".join(lines) + "\n"
```
</issue_to_address>
### Comment 2
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="166" />
<code_context>
// 卸载插件确认对话框(列表模式用)
const showUninstallDialog = ref(false);
- const pluginToUninstall = ref(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the uninstall and reload-failed flows by splitting them into clearer, type-specific helpers and centralizing feedback handling instead of using a generic `kind`-based dispatcher and overloaded parameters.
### 1. Uninstall flow: remove `kind` indirection + overloaded param
The current uninstall flow adds a lot of branching:
- `requestUninstall*` just build `{ kind, id }` wrappers
- `uninstallExtension` overloads `optionsOrSkipConfirm` (boolean *or* object)
- `performUninstall` re‑branches on `target.kind`
You can keep the same UX and API surface but simplify by:
1. Having explicit per‑kind uninstall entry points.
2. Using a clear signature: `{ deleteConfig, deleteData }` plus `skipConfirm` flag.
3. Making the low‑level “post + toast + refresh” helper *not* know about `kind`.
Example of a more linear structure that keeps all capabilities:
```js
// 1) State stays generic enough for UI
const uninstallTarget = ref(null); // { kind: 'normal' | 'failed', id: string } | null
const showUninstallDialog = ref(false);
const openUninstallDialog = (target) => {
uninstallTarget.value = target;
showUninstallDialog.value = true;
};
// 2) UI calls these instead of requestUninstallXXX
const uninstallNormalPlugin = (name, { skipConfirm = false, ...options } = {}) => {
if (!name) return;
if (!skipConfirm) {
openUninstallDialog({ kind: "normal", id: name });
return;
}
return doUninstallNormal(name, options);
};
const uninstallFailedPlugin = (dirName, { skipConfirm = false, ...options } = {}) => {
if (!dirName) return;
if (!skipConfirm) {
openUninstallDialog({ kind: "failed", id: dirName });
return;
}
return doUninstallFailed(dirName, options);
};
// 3) Type‑specific helpers, no generic target/branching
const doUninstallNormal = async (name, { deleteConfig = false, deleteData = false } = {}) => {
toast(`${tm("messages.uninstalling")} ${name}`, "primary");
try {
const res = await axios.post("/api/plugin/uninstall", {
name,
delete_config: deleteConfig,
delete_data: deleteData,
});
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
Object.assign(extension_data, res.data);
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
const doUninstallFailed = async (dirName, { deleteConfig = false, deleteData = false } = {}) => {
toast(`${tm("messages.uninstalling")} ${dirName}`, "primary");
try {
const res = await axios.post("/api/plugin/uninstall-failed", {
dir_name: dirName,
delete_config: deleteConfig,
delete_data: deleteData,
});
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
Then the confirm handler becomes straightforward and no longer needs to reason about booleans vs objects:
```js
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
if (target.kind === "normal") {
await doUninstallNormal(target.id, options);
} else {
await doUninstallFailed(target.id, options);
}
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
This removes:
- `performUninstall(target, options)`
- `requestUninstall`, `requestUninstallPlugin`, `requestUninstallFailedPlugin`
- The `optionsOrSkipConfirm` overload and its type gymnastics
…while keeping all current behaviors (normal/failed, options, confirmation dialog).
### 2. Reload feedback: centralize toast + inline feedback
`reloadFailedPlugin` currently manages both `reloadFeedback` and toasts in parallel with repeated code. You can keep both channels but reduce duplication with a tiny helper:
```js
const showReloadFeedback = (message, type) => {
clearReloadFeedback();
setReloadFeedback(message, type);
toast(message, type === "success" ? "success" : "error");
};
const reloadFailedPlugin = async (dirName) => {
if (!dirName) return;
clearReloadFeedback();
try {
const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
if (res.data.status === "error") {
showReloadFeedback(res.data.message || tm("messages.reloadFailed"), "error");
return;
}
showReloadFeedback(res.data.message || tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
showReloadFeedback(resolveErrorMessage(err, tm("messages.reloadFailed")), "error");
}
};
```
This keeps both inline + toast feedback but flattens the flow and makes the intent clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="650" />
<code_context>
- getExtensions();
- } catch (err) {
- toast(err, "error");
+ const uninstallExtension = async (extensionName, options = null) => {
+ if (!extensionName) return;
+ if (options && typeof options === "object") {
</code_context>
<issue_to_address>
**issue (complexity):** 请考虑将多个卸载辅助函数折叠为一个基于 target 的流程,通过统一的 request、confirm 和 perform 步骤,让卸载逻辑更易于理解和维护。
卸载流程可以在不丢失任何新功能的前提下简化。
目前你有:
- 底层函数:`doUninstallNormal`、`doUninstallFailed`
- 中层函数:`uninstallNormalPlugin`、`uninstallFailedPlugin`
- 入口函数:`requestUninstallPlugin`、`requestUninstallFailedPlugin`、`uninstallExtension`,以及 `handleUninstallConfirm`(它绕过了 `uninstall*Plugin`,直接调用 `doUninstall*`)
这会产生并行的调用栈和多条略有差异的入口路径,从而让整个流程更难追踪。
你已经引入了 `uninstallTarget`,可以在此基础上把流程收敛为:
- 一个“请求”入口(`requestUninstall(target)`)
- 一个“确认”入口(`confirmUninstall(options)`)
- 一个同时处理 normal/failed 插件的“执行”函数
### 1. 引入单一的 `performUninstall`
你可以用一个函数替换 `doUninstallNormal`/`doUninstallFailed`:
```ts
const performUninstall = async (
target,
{ deleteConfig = false, deleteData = false } = {},
) => {
const { kind, id } = target;
const isNormal = kind === "normal";
toast(`${tm("messages.uninstalling")} ${id}`, "primary");
try {
const payload = isNormal
? { name: id, delete_config: deleteConfig, delete_data: deleteData }
: { dir_name: id, delete_config: deleteConfig, delete_data: deleteData };
const url = isNormal
? "/api/plugin/uninstall"
: "/api/plugin/uninstall-failed";
const res = await axios.post(url, payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
if (isNormal) {
Object.assign(extension_data, res.data);
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
然后你就可以删除 `doUninstallNormal` 和 `doUninstallFailed` 了。
### 2. 统一 “request” 函数
`uninstallNormalPlugin`、`uninstallFailedPlugin`、`requestUninstallPlugin` 和 `requestUninstallFailedPlugin` 可以合并为一个“request uninstall” 辅助函数,再加上两个很薄的调用点:
```ts
const requestUninstall = (target) => {
if (!target?.id) return;
uninstallTarget.value = target;
showUninstallDialog.value = true;
};
const requestUninstallPlugin = (name) => {
requestUninstall({ kind: "normal", id: name });
};
const requestUninstallFailedPlugin = (dirName) => {
requestUninstall({ kind: "failed", id: dirName });
};
```
如果 `uninstallNormalPlugin` 和 `uninstallFailedPlugin` 只用于弹窗路由,那么就可以完全移除它们。
### 3. 简化 `handleUninstallConfirm`
在有了 `performUninstall` 之后,`handleUninstallConfirm` 就不需要再关心 normal 和 failed 的区别:
```ts
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
await performUninstall(target, options);
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
这样确认路径和“跳过确认”的路径就始终经过同一个函数。
### 4. 保留 `uninstallExtension` 作为兼容性封装
你更新后的 `uninstallExtension` 已经是 `uninstallNormalPlugin` 的一个封装。按照上述重构之后,它可以直接委托给 `requestUninstall`/`performUninstall`,同时保持向后兼容:
```ts
const uninstallExtension = async (extensionName, options = null) => {
if (!extensionName) return;
// Called with options => already confirmed
if (options && typeof options === "object") {
await performUninstall(
{ kind: "normal", id: extensionName },
options,
);
return;
}
// Old behavior: just trigger confirmation dialog
requestUninstall({ kind: "normal", id: extensionName });
};
```
这样可以减少需要理解的卸载相关函数数量,同时保留:
- normal 和 failed 两种卸载
- deleteConfig/deleteData 选项
- 确认对话框
- 旧的 `uninstallExtension` 调用方。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会用这些反馈来改进后续的 review。
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="650" />
<code_context>
- getExtensions();
- } catch (err) {
- toast(err, "error");
+ const uninstallExtension = async (extensionName, options = null) => {
+ if (!extensionName) return;
+ if (options && typeof options === "object") {
</code_context>
<issue_to_address>
**issue (complexity):** Consider collapsing the multiple uninstall helper functions into a single, target-based flow with unified request, confirm, and perform steps to make the uninstall logic easier to follow and maintain.
The uninstall flow can be simplified without losing any of the new capabilities.
Right now you have:
- Low‑level: `doUninstallNormal`, `doUninstallFailed`
- Mid‑level: `uninstallNormalPlugin`, `uninstallFailedPlugin`
- Entry points: `requestUninstallPlugin`, `requestUninstallFailedPlugin`, `uninstallExtension`, and `handleUninstallConfirm` (which bypasses `uninstall*Plugin` and calls `doUninstall*` directly)
This creates parallel stacks and multiple entry paths that are slightly different, which makes the flow harder to follow.
You already introduced `uninstallTarget`; you can lean on that and collapse the flow to:
- one “request” entry (`requestUninstall(target)`)
- one “confirm” entry (`confirmUninstall(options)`)
- one “perform” function that handles both normal/failed plugins
### 1. Introduce a single `performUninstall`
You can replace `doUninstallNormal`/`doUninstallFailed` with a single function:
```ts
const performUninstall = async (
target,
{ deleteConfig = false, deleteData = false } = {},
) => {
const { kind, id } = target;
const isNormal = kind === "normal";
toast(`${tm("messages.uninstalling")} ${id}`, "primary");
try {
const payload = isNormal
? { name: id, delete_config: deleteConfig, delete_data: deleteData }
: { dir_name: id, delete_config: deleteConfig, delete_data: deleteData };
const url = isNormal
? "/api/plugin/uninstall"
: "/api/plugin/uninstall-failed";
const res = await axios.post(url, payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
if (isNormal) {
Object.assign(extension_data, res.data);
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
Then you can remove `doUninstallNormal` and `doUninstallFailed`.
### 2. Unify “request” functions
`uninstallNormalPlugin`, `uninstallFailedPlugin`, `requestUninstallPlugin`, and `requestUninstallFailedPlugin` can be collapsed into one “request uninstall” helper plus two thin call‑sites:
```ts
const requestUninstall = (target) => {
if (!target?.id) return;
uninstallTarget.value = target;
showUninstallDialog.value = true;
};
const requestUninstallPlugin = (name) => {
requestUninstall({ kind: "normal", id: name });
};
const requestUninstallFailedPlugin = (dirName) => {
requestUninstall({ kind: "failed", id: dirName });
};
```
You can then drop `uninstallNormalPlugin` and `uninstallFailedPlugin` entirely if they’re only used for dialog routing.
### 3. Simplify `handleUninstallConfirm`
With `performUninstall` in place, `handleUninstallConfirm` no longer needs to know about normal vs failed:
```ts
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
await performUninstall(target, options);
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
This makes the confirm path always go through the same function as the “skip confirm” path.
### 4. Keep `uninstallExtension` as a compatibility wrapper
Your updated `uninstallExtension` is already a wrapper around `uninstallNormalPlugin`. After the above refactor, it can delegate directly to `requestUninstall`/`performUninstall` while staying backward compatible:
```ts
const uninstallExtension = async (extensionName, options = null) => {
if (!extensionName) return;
// Called with options => already confirmed
if (options && typeof options === "object") {
await performUninstall(
{ kind: "normal", id: extensionName },
options,
);
return;
}
// Old behavior: just trigger confirmation dialog
requestUninstall({ kind: "normal", id: extensionName });
};
```
This reduces the number of functions you need to understand for uninstalling, while preserving:
- normal vs failed uninstall
- deleteConfig/deleteData options
- confirmation dialog
- legacy `uninstallExtension` callers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体层面的反馈:
useTimedMessage组合函数在模块作用域中保存了timer,因此多个实例会互相覆盖对方的定时器;建议将定时器句柄存储在每个实例关联的响应式状态或 ref 中,以避免组件之间的相互干扰。uninstall_failed_plugin中删除配置/数据的逻辑在很大程度上重复了uninstall_plugin中对应的路径;可以抽取一个共享的配置/数据清理工具函数来减少重复,并让未来的修改集中在一个地方。
给 AI Agents 的提示
Please address the comments from this code review:
## Overall Comments
- The `useTimedMessage` composable keeps `timer` in module scope, so multiple instances will overwrite each other’s timers; consider storing the timer handle in reactive state or a ref tied to each instance to avoid cross-component interference.
- The logic for deleting config/data in `uninstall_failed_plugin` largely duplicates the corresponding paths in `uninstall_plugin`; extracting a shared helper for config/data cleanup would reduce repetition and keep future changes in one place.
## Individual Comments
### Comment 1
<location path="astrbot/core/star/star_manager.py" line_range="1056-1057" />
<code_context>
return plugin_info
except Exception:
- if cleanup_required and dir_name and plugin_path:
- await self._cleanup_failed_plugin_install(
- dir_name=dir_name,
- plugin_path=plugin_path,
+ if dir_name and plugin_path:
+ logger.warning(
+ f"安装插件 {dir_name} 失败,插件安装目录:{plugin_path}",
</code_context>
<issue_to_address>
**question (bug_risk):** Revisit behavior on install failures to ensure partially installed plugins are handled in a controlled way.
Previously, failures triggered `_cleanup_failed_plugin_install` when `cleanup_required` was set; now failures only log a warning. If an exception occurs after `install()` succeeds but before the plugin is registered/added to `failed_plugin_dict`, we may leave orphaned plugin directories that aren’t active or visible in the failed-plugins UI. If you intend to keep failed installs for inspection, consider either adding these cases to `failed_plugin_dict` with a minimal record or selectively calling `_cleanup_failed_plugin_install` so these directories are handled consistently later.
</issue_to_address>帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The
useTimedMessagecomposable keepstimerin module scope, so multiple instances will overwrite each other’s timers; consider storing the timer handle in reactive state or a ref tied to each instance to avoid cross-component interference. - The logic for deleting config/data in
uninstall_failed_pluginlargely duplicates the corresponding paths inuninstall_plugin; extracting a shared helper for config/data cleanup would reduce repetition and keep future changes in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `useTimedMessage` composable keeps `timer` in module scope, so multiple instances will overwrite each other’s timers; consider storing the timer handle in reactive state or a ref tied to each instance to avoid cross-component interference.
- The logic for deleting config/data in `uninstall_failed_plugin` largely duplicates the corresponding paths in `uninstall_plugin`; extracting a shared helper for config/data cleanup would reduce repetition and keep future changes in one place.
## Individual Comments
### Comment 1
<location path="astrbot/core/star/star_manager.py" line_range="1056-1057" />
<code_context>
return plugin_info
except Exception:
- if cleanup_required and dir_name and plugin_path:
- await self._cleanup_failed_plugin_install(
- dir_name=dir_name,
- plugin_path=plugin_path,
+ if dir_name and plugin_path:
+ logger.warning(
+ f"安装插件 {dir_name} 失败,插件安装目录:{plugin_path}",
</code_context>
<issue_to_address>
**question (bug_risk):** Revisit behavior on install failures to ensure partially installed plugins are handled in a controlled way.
Previously, failures triggered `_cleanup_failed_plugin_install` when `cleanup_required` was set; now failures only log a warning. If an exception occurs after `install()` succeeds but before the plugin is registered/added to `failed_plugin_dict`, we may leave orphaned plugin directories that aren’t active or visible in the failed-plugins UI. If you intend to keep failed installs for inspection, consider either adding these cases to `failed_plugin_dict` with a minimal record or selectively calling `_cleanup_failed_plugin_install` so these directories are handled consistently later.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体性的反馈:
- 新的
uninstallExtension函数签名移除了之前skipConfirm布尔参数的行为,现在假定第二个参数是一个选项对象或 null;建议仔细检查所有调用点(包括本次 diff 之外的),确保没有地方还在传裸的布尔值,从而在无意中改变卸载行为。 useTimedMessage在本次 diff 中被引入,但在任何地方都没有使用;建议要么把它接入新的内联反馈交互,要么直接移除,避免留下未使用的 composable。- 在
uninstall_failed_plugin中,面向用户的错误信息是直接从抛出的Exception消息中透出(例如:“插件目录不存在。”);如果这些信息需要本地化,并与其它 API 响应保持一致,建议通过 i18n/消息层或一个统一的错误格式化器来输出,而不是在后端硬编码字符串。
给 AI Agent 的提示词
请根据这次代码评审中的评论进行修改:
## 总体评论
- 新的 `uninstallExtension` 函数签名移除了之前 `skipConfirm` 布尔参数的行为,现在假定第二个参数是一个选项对象或 null;建议仔细检查所有调用点(包括本次 diff 之外的),确保没有地方还在传裸的布尔值,从而在无意中改变卸载行为。
- `useTimedMessage` 在本次 diff 中被引入,但在任何地方都没有使用;建议要么把它接入新的内联反馈交互,要么直接移除,避免留下未使用的 composable。
- 在 `uninstall_failed_plugin` 中,面向用户的错误信息是直接从抛出的 `Exception` 消息中透出(例如:“插件目录不存在。”);如果这些信息需要本地化,并与其它 API 响应保持一致,建议通过 i18n/消息层或一个统一的错误格式化器来输出,而不是在后端硬编码字符串。
## 逐条评论
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="160" />
<code_context>
// 卸载插件确认对话框(列表模式用)
const showUninstallDialog = ref(false);
- const pluginToUninstall = ref(null);
+ const uninstallTarget = ref(null);
</code_context>
<issue_to_address>
**issue (complexity):** 建议将新的卸载流程、随机插件可见性、失败插件以及安装失败的流程重构为几个聚焦的小型辅助方法,在保持行为不变的前提下,消除临时约定和分散的 watchers。
新逻辑在功能上是可靠的,但有几个地方可以减少编排/分支,而不牺牲现有功能。
### 1. 卸载流程:缩减迷你协议
目前 `uninstallTarget` 和 `performUninstall` 通过 `{ kind, id }` 编码了一个小协议,然后在 `performUninstall` 内部分支。
在保留全部新功能的前提下,可以通过将分支逻辑下沉到调用点,并在 `uninstallTarget` 中保存“实际要执行的动作”来简化。这样既可以让对话框保持通用,又避免 `kind`/`id` 这种协议:
```js
// state
const uninstallTarget = ref(null);
// shape: { label: string, action: (options) => Promise<void> }
const requestUninstallPlugin = (name) => {
if (!name) return;
uninstallTarget.value = {
label: name,
async action(options) {
await performUninstallNormal(name, options);
},
};
showUninstallDialog.value = true;
};
const requestUninstallFailedPlugin = (dirName) => {
if (!dirName) return;
uninstallTarget.value = {
label: dirName,
async action(options) {
await performUninstallFailed(dirName, options);
},
};
showUninstallDialog.value = true;
};
// split the "kind" branches into explicit helpers
const performUninstallNormal = async (name, { deleteConfig = false, deleteData = false } = {}) => {
const payload = { name, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${name}`, "primary");
const res = await axios.post("/api/plugin/uninstall", payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
Object.assign(extension_data, res.data);
toast(res.data.message, "success");
await getExtensions();
};
const performUninstallFailed = async (dirName, { deleteConfig = false, deleteData = false } = {}) => {
const payload = { dir_name: dirName, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${dirName}`, "primary");
const res = await axios.post("/api/plugin/uninstall-failed", payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
toast(res.data.message, "success");
await getExtensions();
};
// dialog confirm becomes very linear
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
await target.action(options);
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
这样仍然只需要一个对话框,也保留了普通卸载/失败卸载的场景,但将 `kind` 的分支从关键执行路径中移除,使整个流程更容易自上而下地阅读。
### 2. 随机插件:将耦合本地化
`showRandomPlugins`、`collapseRandomPlugins` 以及对 `marketSearch`/`currentPage` 的 watchers 目前是分散的。可以在同一个文件中用一个小 helper(不需要新建模块)来集中这些逻辑,让耦合关系变得显式:
```js
const useRandomPluginsDisplay = ({ activeTab, marketSearch, currentPage }) => {
const showRandomPlugins = ref(true);
const toggleRandomPluginsVisibility = () => {
showRandomPlugins.value = !showRandomPlugins.value;
};
const collapseRandomPlugins = () => {
showRandomPlugins.value = false;
};
watch(marketSearch, () => {
if (activeTab.value === "market") {
collapseRandomPlugins();
}
});
watch(currentPage, (newPage, oldPage) => {
if (newPage === oldPage) return;
if (activeTab.value !== "market") return;
collapseRandomPlugins();
});
return {
showRandomPlugins,
toggleRandomPluginsVisibility,
collapseRandomPlugins,
};
};
// usage in composable
const { showRandomPlugins, toggleRandomPluginsVisibility, collapseRandomPlugins } =
useRandomPluginsDisplay({ activeTab, marketSearch, currentPage });
```
行为保持不变,但“随机插件在什么情况下会被折叠/隐藏”的逻辑被放在一个命名清晰的集中区域,从而降低了在主逻辑中的隐式耦合。
### 3. 失败插件:隔离状态和动作
`failedPluginsDict`、`failedPluginItems` 和 `reloadFailedPlugin` 本身已经很内聚。把它们包在一个小 helper 里,可以在不改变行为的情况下,让主 composable 更易阅读:
```js
const useFailedPlugins = (getExtensions, toast, tm) => {
const failedPluginsDict = ref({});
const failedPluginItems = computed(() => {
const raw = failedPluginsDict.value || {};
return Object.entries(raw).map(([dirName, info]) => {
const detail = info && typeof info === "object" ? info : {};
return {
dir_name: dirName,
name: detail.name || dirName,
display_name: detail.display_name || detail.name || dirName,
error: detail.error || "",
traceback: detail.traceback || "",
reserved: !!detail.reserved,
};
});
});
const reloadFailedPlugin = async (dirName) => {
if (!dirName) return;
try {
const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
if (res.data.status === "error") {
toast(res.data.message || tm("messages.reloadFailed"), "error");
return;
}
toast(res.data.message || tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err, tm("messages.reloadFailed")), "error");
}
};
return { failedPluginsDict, failedPluginItems, reloadFailedPlugin };
};
// usage
const { failedPluginsDict, failedPluginItems, reloadFailedPlugin } =
useFailedPlugins(getExtensions, toast, tm);
```
`getExtensions` 仍然可以像现在一样给 `failedPluginsDict.value` 赋值,只是转换逻辑和相关动作不再混在主 composable 里。
### 4. 安装失败刷新:按来源封装流程
与其在各个分支中零散地调用 `refreshExtensionsAfterInstallFailure`,不如继续保留这个 helper,但把它隐藏在两个安装变体的薄封装中:
```js
const installFromFile = async (ignoreVersionCheck) => {
try {
const res = await axios.post("/api/plugin/install", { /* file payload */ });
// handle OK / warning / error exactly as now...
} catch (err) {
await refreshExtensionsAfterInstallFailure();
throw err;
}
};
const installFromUrl = async (ignoreVersionCheck) => {
try {
const res = await axios.post("/api/plugin/install", { /* url payload */ });
// handle OK / warning / error exactly as now...
} catch (err) {
await refreshExtensionsAfterInstallFailure();
throw err;
}
};
const newExtension = async (ignoreVersionCheck = false) => {
if (uploadTab.value === "file") {
await installFromFile(ignoreVersionCheck);
} else {
await installFromUrl(ignoreVersionCheck);
}
};
```
在 `installFromFile` 和 `installFromUrl` 内部,可以把三种情况(warning、显式 error、抛出的 error)都集中处理,这样 `newExtension` 就能非常直接地表达高层流程。
以上改动都会保留现有行为,但压缩分支数量,并将相关逻辑拆分到小而聚焦的单元,有助于解决“多用途 composable 中塞了很多零散小函数”的担忧,同时保留你这次做的功能增强。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new
uninstallExtensionsignature drops the oldskipConfirmboolean behavior and now assumes the second argument is an options object or null; it would be worth double-checking all call sites (including those not in this diff) to ensure none still pass a bare boolean and unintentionally change uninstall behavior. useTimedMessageis introduced but not used anywhere in the provided diff; consider either wiring it into the new inline feedback UX or removing it to avoid leaving unused composables around.- In
uninstall_failed_plugin, user-facing errors are surfaced directly from raisedExceptionmessages (e.g., '插件目录不存在。'); if these are meant to be localized and consistent with other API responses, consider funneling them through the i18n/message layer or a central error formatter instead of hardcoding strings in the backend.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `uninstallExtension` signature drops the old `skipConfirm` boolean behavior and now assumes the second argument is an options object or null; it would be worth double-checking all call sites (including those not in this diff) to ensure none still pass a bare boolean and unintentionally change uninstall behavior.
- `useTimedMessage` is introduced but not used anywhere in the provided diff; consider either wiring it into the new inline feedback UX or removing it to avoid leaving unused composables around.
- In `uninstall_failed_plugin`, user-facing errors are surfaced directly from raised `Exception` messages (e.g., '插件目录不存在。'); if these are meant to be localized and consistent with other API responses, consider funneling them through the i18n/message layer or a central error formatter instead of hardcoding strings in the backend.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="160" />
<code_context>
// 卸载插件确认对话框(列表模式用)
const showUninstallDialog = ref(false);
- const pluginToUninstall = ref(null);
+ const uninstallTarget = ref(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new uninstall, random-plugins visibility, failed-plugins, and install-failure flows into small focused helpers to eliminate ad‑hoc protocols and scattered watchers while keeping behavior the same.
The new logic is functionally solid, but there are a few spots where you can reduce orchestration/branching without losing features.
### 1. Uninstall flow: reduce the mini‑protocol
Right now `uninstallTarget` + `performUninstall` encode a small protocol via `{ kind, id }`, then branch inside `performUninstall`.
You can keep all the new functionality but simplify by pushing the branching to the call sites and storing the actual action in `uninstallTarget`. That keeps the dialog generic and avoids the `kind`/`id` protocol:
```js
// state
const uninstallTarget = ref(null);
// shape: { label: string, action: (options) => Promise<void> }
const requestUninstallPlugin = (name) => {
if (!name) return;
uninstallTarget.value = {
label: name,
async action(options) {
await performUninstallNormal(name, options);
},
};
showUninstallDialog.value = true;
};
const requestUninstallFailedPlugin = (dirName) => {
if (!dirName) return;
uninstallTarget.value = {
label: dirName,
async action(options) {
await performUninstallFailed(dirName, options);
},
};
showUninstallDialog.value = true;
};
// split the "kind" branches into explicit helpers
const performUninstallNormal = async (name, { deleteConfig = false, deleteData = false } = {}) => {
const payload = { name, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${name}`, "primary");
const res = await axios.post("/api/plugin/uninstall", payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
Object.assign(extension_data, res.data);
toast(res.data.message, "success");
await getExtensions();
};
const performUninstallFailed = async (dirName, { deleteConfig = false, deleteData = false } = {}) => {
const payload = { dir_name: dirName, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${dirName}`, "primary");
const res = await axios.post("/api/plugin/uninstall-failed", payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
toast(res.data.message, "success");
await getExtensions();
};
// dialog confirm becomes very linear
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
await target.action(options);
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
This keeps one dialog, preserves normal/failed uninstall, but removes `kind` switches from the hot path and makes the flow more “read the code top‑down”.
### 2. Random plugins: localize the coupling
The `showRandomPlugins` + `collapseRandomPlugins` + watchers on `marketSearch`/`currentPage` are currently scattered. You can centralize this with a tiny helper inside the same file (no new module needed) to make the coupling explicit:
```js
const useRandomPluginsDisplay = ({ activeTab, marketSearch, currentPage }) => {
const showRandomPlugins = ref(true);
const toggleRandomPluginsVisibility = () => {
showRandomPlugins.value = !showRandomPlugins.value;
};
const collapseRandomPlugins = () => {
showRandomPlugins.value = false;
};
watch(marketSearch, () => {
if (activeTab.value === "market") {
collapseRandomPlugins();
}
});
watch(currentPage, (newPage, oldPage) => {
if (newPage === oldPage) return;
if (activeTab.value !== "market") return;
collapseRandomPlugins();
});
return {
showRandomPlugins,
toggleRandomPluginsVisibility,
collapseRandomPlugins,
};
};
// usage in composable
const { showRandomPlugins, toggleRandomPluginsVisibility, collapseRandomPlugins } =
useRandomPluginsDisplay({ activeTab, marketSearch, currentPage });
```
This keeps your behavior but moves the “when do random plugins hide?” logic into one clearly named cluster, reducing implicit coupling in the main body.
### 3. Failed plugins: isolate state + actions
`failedPluginsDict`, `failedPluginItems`, and `reloadFailedPlugin` are already cohesive. Wrapping them into a small helper makes the main composable read cleaner without changing behavior:
```js
const useFailedPlugins = (getExtensions, toast, tm) => {
const failedPluginsDict = ref({});
const failedPluginItems = computed(() => {
const raw = failedPluginsDict.value || {};
return Object.entries(raw).map(([dirName, info]) => {
const detail = info && typeof info === "object" ? info : {};
return {
dir_name: dirName,
name: detail.name || dirName,
display_name: detail.display_name || detail.name || dirName,
error: detail.error || "",
traceback: detail.traceback || "",
reserved: !!detail.reserved,
};
});
});
const reloadFailedPlugin = async (dirName) => {
if (!dirName) return;
try {
const res = await axios.post("/api/plugin/reload-failed", { dir_name: dirName });
if (res.data.status === "error") {
toast(res.data.message || tm("messages.reloadFailed"), "error");
return;
}
toast(res.data.message || tm("messages.reloadSuccess"), "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err, tm("messages.reloadFailed")), "error");
}
};
return { failedPluginsDict, failedPluginItems, reloadFailedPlugin };
};
// usage
const { failedPluginsDict, failedPluginItems, reloadFailedPlugin } =
useFailedPlugins(getExtensions, toast, tm);
```
`getExtensions` can continue to assign to `failedPluginsDict.value` as it does now, but the transformation + action code is no longer mixed into the main composable.
### 4. Install failure refresh: wrap per‑source flows
Instead of sprinkling `refreshExtensionsAfterInstallFailure` at each branch, you can keep the helper but hide it in thin wrappers around the two install variants:
```js
const installFromFile = async (ignoreVersionCheck) => {
try {
const res = await axios.post("/api/plugin/install", { /* file payload */ });
// handle OK / warning / error exactly as now...
} catch (err) {
await refreshExtensionsAfterInstallFailure();
throw err;
}
};
const installFromUrl = async (ignoreVersionCheck) => {
try {
const res = await axios.post("/api/plugin/install", { /* url payload */ });
// handle OK / warning / error exactly as now...
} catch (err) {
await refreshExtensionsAfterInstallFailure();
throw err;
}
};
const newExtension = async (ignoreVersionCheck = false) => {
if (uploadTab.value === "file") {
await installFromFile(ignoreVersionCheck);
} else {
await installFromUrl(ignoreVersionCheck);
}
};
```
Inside `installFromFile` / `installFromUrl`, you can centralize the three cases (warning, explicit error, thrown error) so `newExtension` reads as a straightforward high‑level flow.
All of these changes preserve your current behavior but compress branching and move related logic into small, focused units, which should address the “multi‑purpose composable with many small scattered functions” concern without undoing the feature work.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 3 个问题,并给出了一些整体性的反馈:
- 在
buildFailedPluginItems中,你只从failedPluginsDict里传递了少量字段,这会丢弃现在由_build_failed_plugin_record填充的元数据(如 author、desc、version、repo);建议保留并暴露这些额外字段,以便在需要时 UI 能为失败插件展示更丰富的上下文信息。 - 在
uninstall_failed_plugin中,当插件目录不存在时你会直接报错返回,因此不会尝试执行_cleanup_plugin_optional_artifacts或从failed_plugin_dict中移除条目;你可能希望即使插件目录已经不存在了,也继续清理配置/数据并清除失败记录,从而让用户可以彻底清理掉坏掉的条目。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- In `buildFailedPluginItems` you only pass through a few fields from `failedPluginsDict`, which discards metadata now populated by `_build_failed_plugin_record` (e.g., author, desc, version, repo); consider preserving and exposing these extra fields so the UI can render richer context for failed plugins if needed.
- In `uninstall_failed_plugin` you bail out with an error when the plugin directory does not exist, and therefore never attempt `_cleanup_plugin_optional_artifacts` or removing the entry from `failed_plugin_dict`; you might want to still clean config/data and clear the failed record even when the plugin folder is already gone to allow users to fully clear broken entries.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="549-558" />
<code_context>
+ }
+ toast(res.data.message || tm("messages.reloadSuccess"), "success");
+ await getExtensions();
+ } catch (err) {
+ toast(resolveErrorMessage(err, tm("messages.reloadFailed")), "error");
+ }
</code_context>
<issue_to_address>
**suggestion:** Use `resolveErrorMessage` and align error handling for URL installs with the new helper.
Other call sites like `reloadPlugin` and `reloadFailedPlugin` now use `resolveErrorMessage` so we don’t surface raw error objects (e.g. `[object Object]`). Here we still concatenate `err` directly and pass it into `onLoadingDialogResult`. Please compute a single `const message = resolveErrorMessage(err, tm("messages.installFailed"));` and reuse it for both the toast and the loading dialog to keep error messaging consistent and user-friendly.
</issue_to_address>
### Comment 2
<location path="astrbot/core/star/star_manager.py" line_range="1223-1214" />
<code_context>
- # 删除 data/plugin_data 下的插件持久化数据(单数形式,新版本)
- plugin_data_dir = os.path.join(
- data_base_dir, "plugin_data", root_dir_name
+ plugin_path = os.path.join(self.plugin_store_path, dir_name)
+ if not os.path.exists(plugin_path):
+ raise Exception(
+ self._format_plugin_error("plugin_dir_not_exists"),
)
</code_context>
<issue_to_address>
**suggestion:** Uninstalling a failed plugin errors out if the plugin directory is already gone, leaving stale state.
In `uninstall_failed_plugin`, when the plugin directory is missing, `plugin_dir_not_exists` is raised and the method returns without clearing `failed_plugin_dict` or optional artifacts. This leaves stale failed-plugin entries and makes the operation non-idempotent.
Instead, treat a missing directory as a partially-cleaned state: still run `_cleanup_plugin_optional_artifacts` and remove the entry from `failed_plugin_dict` (and rebuild `failed_plugin_info`). That way, `uninstall_failed_plugin` remains safe to call even if the directory was already removed, and the failed list doesn’t get stuck with an unremovable entry.
Suggested implementation:
```python
plugin_path = os.path.join(self.plugin_store_path, dir_name)
if not os.path.exists(plugin_path):
# 插件目录不存在时,视为已部分卸载的状态,继续后续清理逻辑以保证幂等性
logger.debug(
"插件目录不存在,视为已部分卸载状态,继续清理失败插件记录和可选产物: %s",
plugin_path,
)
```
To fully implement your suggestion, you should also:
1. Ensure that `uninstall_failed_plugin` does **not** `return` immediately after the missing-directory check; it must still:
- Call `_cleanup_plugin_optional_artifacts(root_dir_name or dir_name)` (or the appropriate identifier used in your code).
- Remove the plugin from `self.failed_plugin_dict` (e.g. `self.failed_plugin_dict.pop(root_dir_name, None)`).
- Call `self._rebuild_failed_plugin_info()` after mutating `failed_plugin_dict`.
2. If there is any `try/except` around the directory removal logic that currently swallows exceptions and returns early, adjust it so that a missing directory never prevents the cleanup steps above from running.
3. If `uninstall_failed_plugin` currently raises or returns after catching `plugin_dir_not_exists` elsewhere (e.g. via an error code or re-raising), remove that special handling so that a missing directory is treated as a soft/partial-uninstall state rather than a hard error.
</issue_to_address>
### Comment 3
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="538" />
<code_context>
}
};
+
+ const reloadFailedPlugin = async (dirName) => {
+ if (!dirName) return;
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider unifying the uninstall logic into a single data-driven flow that avoids callback refs and duplicated normal/failed helper functions.
You can keep all new capabilities while simplifying the uninstall flow by removing the callback-in-ref pattern and the duplicated “normal vs failed” helpers.
### 1. Unify uninstall execution logic
`performUninstallNormal` and `performUninstallFailed` are almost identical. You can factor them into a single parametrized helper:
```ts
const performUninstall = async (
target: { kind: "normal" | "failed"; id: string },
{ deleteConfig = false, deleteData = false } = {},
) => {
const isFailed = target.kind === "failed";
const endpoint = isFailed
? "/api/plugin/uninstall-failed"
: "/api/plugin/uninstall";
const payload = isFailed
? { dir_name: target.id, delete_config: deleteConfig, delete_data: deleteData }
: { name: target.id, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${target.id}`, "primary");
try {
const res = await axios.post(endpoint, payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
if (!isFailed) {
Object.assign(extension_data, res.data);
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
Then you can delete `performUninstallNormal` and `performUninstallFailed`.
### 2. Store data, not callbacks, in dialog state
Instead of putting `action` callbacks into `uninstallTarget`, keep a minimal, data-driven state:
```ts
const uninstallTarget = ref<null | { kind: "normal" | "failed"; id: string }>(
null,
);
const requestUninstallPlugin = (name: string) => {
if (!name) return;
uninstallTarget.value = { kind: "normal", id: name };
showUninstallDialog.value = true;
};
const requestUninstallFailedPlugin = (dirName: string) => {
if (!dirName) return;
uninstallTarget.value = { kind: "failed", id: dirName };
showUninstallDialog.value = true;
};
```
`handleUninstallConfirm` becomes a single branch instead of going through a stored callback:
```ts
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
await performUninstall(target, options);
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
### 3. Keep `uninstallExtension` compatibility but simplify routing
You can keep the current API surface (boolean/options) while routing everything through the unified helper and `requestUninstallPlugin`:
```ts
const uninstallExtension = async (
extensionName: string,
options: boolean | { deleteConfig?: boolean; deleteData?: boolean } | null = null,
) => {
if (!extensionName) return;
// Legacy boolean: skip confirm
if (typeof options === "boolean") {
if (options) {
await performUninstall({ kind: "normal", id: extensionName });
} else {
requestUninstallPlugin(extensionName);
}
return;
}
// Explicit options: already confirmed
if (options && typeof options === "object") {
await performUninstall({ kind: "normal", id: extensionName }, options);
return;
}
// No options: show dialog
requestUninstallPlugin(extensionName);
};
```
This keeps all existing behaviour (normal uninstall, failed uninstall, delete-config/data options, and backward compatibility on `uninstallExtension`) while:
- Removing duplicated HTTP logic
- Replacing callback-in-ref with a simple `{ kind, id }` state
- Reducing the number of coordinating functions and indirection layers around uninstall.
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据反馈改进后续的评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- In
buildFailedPluginItemsyou only pass through a few fields fromfailedPluginsDict, which discards metadata now populated by_build_failed_plugin_record(e.g., author, desc, version, repo); consider preserving and exposing these extra fields so the UI can render richer context for failed plugins if needed. - In
uninstall_failed_pluginyou bail out with an error when the plugin directory does not exist, and therefore never attempt_cleanup_plugin_optional_artifactsor removing the entry fromfailed_plugin_dict; you might want to still clean config/data and clear the failed record even when the plugin folder is already gone to allow users to fully clear broken entries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `buildFailedPluginItems` you only pass through a few fields from `failedPluginsDict`, which discards metadata now populated by `_build_failed_plugin_record` (e.g., author, desc, version, repo); consider preserving and exposing these extra fields so the UI can render richer context for failed plugins if needed.
- In `uninstall_failed_plugin` you bail out with an error when the plugin directory does not exist, and therefore never attempt `_cleanup_plugin_optional_artifacts` or removing the entry from `failed_plugin_dict`; you might want to still clean config/data and clear the failed record even when the plugin folder is already gone to allow users to fully clear broken entries.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="549-558" />
<code_context>
+ }
+ toast(res.data.message || tm("messages.reloadSuccess"), "success");
+ await getExtensions();
+ } catch (err) {
+ toast(resolveErrorMessage(err, tm("messages.reloadFailed")), "error");
+ }
</code_context>
<issue_to_address>
**suggestion:** Use `resolveErrorMessage` and align error handling for URL installs with the new helper.
Other call sites like `reloadPlugin` and `reloadFailedPlugin` now use `resolveErrorMessage` so we don’t surface raw error objects (e.g. `[object Object]`). Here we still concatenate `err` directly and pass it into `onLoadingDialogResult`. Please compute a single `const message = resolveErrorMessage(err, tm("messages.installFailed"));` and reuse it for both the toast and the loading dialog to keep error messaging consistent and user-friendly.
</issue_to_address>
### Comment 2
<location path="astrbot/core/star/star_manager.py" line_range="1223-1214" />
<code_context>
- # 删除 data/plugin_data 下的插件持久化数据(单数形式,新版本)
- plugin_data_dir = os.path.join(
- data_base_dir, "plugin_data", root_dir_name
+ plugin_path = os.path.join(self.plugin_store_path, dir_name)
+ if not os.path.exists(plugin_path):
+ raise Exception(
+ self._format_plugin_error("plugin_dir_not_exists"),
)
</code_context>
<issue_to_address>
**suggestion:** Uninstalling a failed plugin errors out if the plugin directory is already gone, leaving stale state.
In `uninstall_failed_plugin`, when the plugin directory is missing, `plugin_dir_not_exists` is raised and the method returns without clearing `failed_plugin_dict` or optional artifacts. This leaves stale failed-plugin entries and makes the operation non-idempotent.
Instead, treat a missing directory as a partially-cleaned state: still run `_cleanup_plugin_optional_artifacts` and remove the entry from `failed_plugin_dict` (and rebuild `failed_plugin_info`). That way, `uninstall_failed_plugin` remains safe to call even if the directory was already removed, and the failed list doesn’t get stuck with an unremovable entry.
Suggested implementation:
```python
plugin_path = os.path.join(self.plugin_store_path, dir_name)
if not os.path.exists(plugin_path):
# 插件目录不存在时,视为已部分卸载的状态,继续后续清理逻辑以保证幂等性
logger.debug(
"插件目录不存在,视为已部分卸载状态,继续清理失败插件记录和可选产物: %s",
plugin_path,
)
```
To fully implement your suggestion, you should also:
1. Ensure that `uninstall_failed_plugin` does **not** `return` immediately after the missing-directory check; it must still:
- Call `_cleanup_plugin_optional_artifacts(root_dir_name or dir_name)` (or the appropriate identifier used in your code).
- Remove the plugin from `self.failed_plugin_dict` (e.g. `self.failed_plugin_dict.pop(root_dir_name, None)`).
- Call `self._rebuild_failed_plugin_info()` after mutating `failed_plugin_dict`.
2. If there is any `try/except` around the directory removal logic that currently swallows exceptions and returns early, adjust it so that a missing directory never prevents the cleanup steps above from running.
3. If `uninstall_failed_plugin` currently raises or returns after catching `plugin_dir_not_exists` elsewhere (e.g. via an error code or re-raising), remove that special handling so that a missing directory is treated as a soft/partial-uninstall state rather than a hard error.
</issue_to_address>
### Comment 3
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="538" />
<code_context>
}
};
+
+ const reloadFailedPlugin = async (dirName) => {
+ if (!dirName) return;
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider unifying the uninstall logic into a single data-driven flow that avoids callback refs and duplicated normal/failed helper functions.
You can keep all new capabilities while simplifying the uninstall flow by removing the callback-in-ref pattern and the duplicated “normal vs failed” helpers.
### 1. Unify uninstall execution logic
`performUninstallNormal` and `performUninstallFailed` are almost identical. You can factor them into a single parametrized helper:
```ts
const performUninstall = async (
target: { kind: "normal" | "failed"; id: string },
{ deleteConfig = false, deleteData = false } = {},
) => {
const isFailed = target.kind === "failed";
const endpoint = isFailed
? "/api/plugin/uninstall-failed"
: "/api/plugin/uninstall";
const payload = isFailed
? { dir_name: target.id, delete_config: deleteConfig, delete_data: deleteData }
: { name: target.id, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${target.id}`, "primary");
try {
const res = await axios.post(endpoint, payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
if (!isFailed) {
Object.assign(extension_data, res.data);
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
Then you can delete `performUninstallNormal` and `performUninstallFailed`.
### 2. Store data, not callbacks, in dialog state
Instead of putting `action` callbacks into `uninstallTarget`, keep a minimal, data-driven state:
```ts
const uninstallTarget = ref<null | { kind: "normal" | "failed"; id: string }>(
null,
);
const requestUninstallPlugin = (name: string) => {
if (!name) return;
uninstallTarget.value = { kind: "normal", id: name };
showUninstallDialog.value = true;
};
const requestUninstallFailedPlugin = (dirName: string) => {
if (!dirName) return;
uninstallTarget.value = { kind: "failed", id: dirName };
showUninstallDialog.value = true;
};
```
`handleUninstallConfirm` becomes a single branch instead of going through a stored callback:
```ts
const handleUninstallConfirm = async (options) => {
const target = uninstallTarget.value;
if (!target) return;
try {
await performUninstall(target, options);
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
### 3. Keep `uninstallExtension` compatibility but simplify routing
You can keep the current API surface (boolean/options) while routing everything through the unified helper and `requestUninstallPlugin`:
```ts
const uninstallExtension = async (
extensionName: string,
options: boolean | { deleteConfig?: boolean; deleteData?: boolean } | null = null,
) => {
if (!extensionName) return;
// Legacy boolean: skip confirm
if (typeof options === "boolean") {
if (options) {
await performUninstall({ kind: "normal", id: extensionName });
} else {
requestUninstallPlugin(extensionName);
}
return;
}
// Explicit options: already confirmed
if (options && typeof options === "object") {
await performUninstall({ kind: "normal", id: extensionName }, options);
return;
}
// No options: show dialog
requestUninstallPlugin(extensionName);
};
```
This keeps all existing behaviour (normal uninstall, failed uninstall, delete-config/data options, and backward compatibility on `uninstallExtension`) while:
- Removing duplicated HTTP logic
- Replacing callback-in-ref with a simple `{ kind, id }` state
- Reducing the number of coordinating functions and indirection layers around uninstall.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我给出了一些总体反馈:
StarManager中新增的_format_plugin_error把面向用户的中文文案直接硬编码在代码里;建议通过现有的 i18n/消息基础设施或共享的错误常量模块来输出这些文案,这样就可以和其他仪表盘文案一起被统一本地化。- 在
errorUtils.resolveErrorMessage中,你对任何传入的err都是直接强制转换为String;如果err是一个很大的对象,或者包含循环引用,这样仍然可能产生不够有用的输出。因此更安全的做法是:显式处理常见的 Axios/HTTP 错误结构,并在无法识别时回退到一个通用的 i18n 文案 key,而不是使用String(err)。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- The new `_format_plugin_error` in `StarManager` hardcodes user-facing Chinese strings; consider routing these through the existing i18n/message infrastructure or a shared error constant module so they can be localized consistently with the rest of the dashboard.
- In `errorUtils.resolveErrorMessage`, you treat any `err` by coercing to `String`; if `err` is a large object or includes circular references this can still produce unhelpful output, so it may be safer to explicitly handle common Axios/HTTP error shapes and fall back to a generic i18n key instead of `String(err)`.帮我变得更有用!请对每条评论点击 👍 或 👎,我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've left some high level feedback:
- The new
_format_plugin_errorinStarManagerhardcodes user-facing Chinese strings; consider routing these through the existing i18n/message infrastructure or a shared error constant module so they can be localized consistently with the rest of the dashboard. - In
errorUtils.resolveErrorMessage, you treat anyerrby coercing toString; iferris a large object or includes circular references this can still produce unhelpful output, so it may be safer to explicitly handle common Axios/HTTP error shapes and fall back to a generic i18n key instead ofString(err).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_format_plugin_error` in `StarManager` hardcodes user-facing Chinese strings; consider routing these through the existing i18n/message infrastructure or a shared error constant module so they can be localized consistently with the rest of the dashboard.
- In `errorUtils.resolveErrorMessage`, you treat any `err` by coercing to `String`; if `err` is a large object or includes circular references this can still produce unhelpful output, so it may be safer to explicitly handle common Axios/HTTP error shapes and fall back to a generic i18n key instead of `String(err)`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="632" />
<code_context>
- if (res.data.status === "error") {
- toast(res.data.message, "error");
- return;
+ const uninstallExtension = async (extensionName, options = null) => {
+ if (!extensionName) return;
+
</code_context>
<issue_to_address>
**issue (complexity):** 建议将卸载和安装流程重构为使用单一规范化入口和集中式编排,这样可以在保持行为不变的前提下,减少辅助函数数量和分支路径的复杂度。
对于卸载和安装流程,你可以保留所有新的行为,通过规范化输入和集中控制流程来缩小编排范围。这样可以在保持兼容性的同时,移除若干零散的小辅助函数。
### 1. 卸载流程 – 规范化目标并集中编排
你其实已经在概念上拥有一个规范化的目标(`{ kind, id }`)。你可以把它作为**唯一的输入类型**,并将 `requestUninstall*` 和 `performUninstall` 收敛到一个更小的接口表面:
```ts
// 1) 单一规范化的卸载入口
type UninstallKind = "normal" | "failed";
interface UninstallTarget {
id: string;
kind: UninstallKind;
}
interface UninstallOptions {
deleteConfig?: boolean;
deleteData?: boolean;
skipConfirm?: boolean;
}
const uninstallTarget = ref<UninstallTarget | null>(null);
const uninstall = async (target: UninstallTarget, options: UninstallOptions = {}) => {
const { deleteConfig = false, deleteData = false, skipConfirm = false } = options;
if (!skipConfirm) {
uninstallTarget.value = target;
showUninstallDialog.value = true;
return;
}
const isFailed = target.kind === "failed";
const endpoint = isFailed ? "/api/plugin/uninstall-failed" : "/api/plugin/uninstall";
const payload = isFailed
? { dir_name: target.id, delete_config: deleteConfig, delete_data: deleteData }
: { name: target.id, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${target.id}`, "primary");
try {
const res = await axios.post(endpoint, payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
if (!isFailed) {
Object.assign(extension_data, res.data);
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
然后:
```ts
// 2) 暴露给模板的 helper 变成简单的适配器
const requestUninstallPlugin = (name: string) =>
name && uninstall({ kind: "normal", id: name }, { skipConfirm: false });
const requestUninstallFailedPlugin = (dirName: string) =>
dirName && uninstall({ kind: "failed", id: dirName }, { skipConfirm: false });
// 3) 弹窗确认只需要使用已存储的 target
const handleUninstallConfirm = async (options: { deleteConfig?: boolean; deleteData?: boolean }) => {
if (!uninstallTarget.value) return;
try {
await uninstall(uninstallTarget.value, { ...options, skipConfirm: true });
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
对 `uninstallExtension` 的向后兼容可以直接通过这个规范化的 API 来实现,而不需要再加一层封装:
```ts
// 保持旧语义,但委托给单一的 `uninstall` API
const uninstallExtension = async (
extensionName: string,
optionsOrSkipConfirm: boolean | { deleteConfig?: boolean; deleteData?: boolean } = false,
) => {
if (!extensionName) return;
if (typeof optionsOrSkipConfirm === "boolean") {
return uninstall(
{ kind: "normal", id: extensionName },
{ skipConfirm: optionsOrSkipConfirm },
);
}
return uninstall(
{ kind: "normal", id: extensionName },
{ ...optionsOrSkipConfirm, skipConfirm: true },
);
};
```
这样你就可以移除 `requestUninstall`、`performUninstall` 这两个独立概念,只保留 `uninstall` + 两个轻量适配器 + 向后兼容封装。
---
### 2. 安装流程 – 使用单一 `performInstall` 并返回显式状态
你已经创建了 `installFromFile`、`installFromUrl` 和 `handleInstallResponse`。可以再进一步,把编排集中到一个 `performInstall` 中,并通过**状态枚举**的返回值让副作用更加清晰:
```ts
type InstallSource = "file" | "url";
type InstallStatus = "ok" | "warning-version" | "error";
interface PerformInstallOptions {
source: InstallSource;
ignoreVersionCheck: boolean;
}
const performInstallRequest = async (
{ source, ignoreVersionCheck }: PerformInstallOptions,
) => {
if (source === "file") {
const formData = new FormData();
formData.append("file", upload_file.value);
formData.append("ignore_version_check", String(ignoreVersionCheck));
return axios.post("/api/plugin/install-upload", formData, {
headers: { "Content-Type": "multipart/form-data" },
});
}
return axios.post("/api/plugin/install", {
url: extension_url.value,
proxy: getSelectedGitHubProxy(),
ignore_version_check: ignoreVersionCheck,
});
};
```
然后让 `handleInstallResponse` 返回一个显式的状态值:
```ts
const handleInstallResponse = async (
resData: any,
{ toastStatus = false } = {},
): Promise<InstallStatus> => {
if (resData.status === "warning" &&
resData.data?.warning_type === "astrbot_version_incompatible") {
onLoadingDialogResult(2, resData.message, -1);
showVersionCompatibilityWarning(resData.message);
await refreshExtensionsAfterInstallFailure();
return "warning-version";
}
if (toastStatus) {
toast(resData.message, resData.status === "ok" ? "success" : "error");
}
if (resData.status === "error") {
onLoadingDialogResult(2, resData.message, -1);
await refreshExtensionsAfterInstallFailure();
return "error";
}
return "ok";
};
```
并把通用的成功收尾逻辑集中到一个 helper 中:
```ts
const finalizeSuccessfulInstall = async (resData: any) => {
onLoadingDialogResult(1, resData.message);
dialog.value = false;
await getExtensions();
viewReadme({
name: resData.data.name,
repo: resData.data.repo || null,
});
await checkAndPromptConflicts();
};
```
现在 `newExtension` 可以委托给一个单一的编排函数,而不是两个高层级的独立流程:
```ts
const newExtension = async (ignoreVersionCheck = false) => {
if (!extension_url.value && !upload_file.value) {
toast(tm("messages.fillUrlOrFile"), "error");
return;
}
if (extension_url.value && upload_file.value) {
toast(tm("messages.dontFillBoth"), "error");
return;
}
loading_.value = true;
loadingDialog.title = tm("status.loading");
loadingDialog.show = true;
const source: InstallSource = upload_file.value ? "file" : "url";
toast(
source === "file"
? tm("messages.installing")
: tm("messages.installingFromUrl") + " " + extension_url.value,
"primary",
);
try {
const res = await performInstallRequest({ source, ignoreVersionCheck });
loading_.value = false;
const status = await handleInstallResponse(res.data, {
toastStatus: source === "url",
});
if (status !== "ok") return;
if (source === "file") {
upload_file.value = null;
} else {
extension_url.value = "";
}
await finalizeSuccessfulInstall(res.data);
} catch (err) {
loading_.value = false;
const message = resolveErrorMessage(err, tm("messages.installFailed"));
if (source === "url") toast(message, "error");
onLoadingDialogResult(2, message, -1);
await refreshExtensionsAfterInstallFailure();
}
};
```
这些调整保留了你当前的所有行为(警告、失败后的刷新、toast、对话框控制等),但将安装流程的接口面压缩为:
- `performInstallRequest`
- `handleInstallResponse`(基于状态)
- `finalizeSuccessfulInstall`
- `newExtension` 作为单一高层级编排器
这样你就可以完全移除 `installFromFile` 和 `installFromUrl`。
---
这些改动在保留所有新能力的前提下(失败插件卸载、更丰富的错误处理、版本警告等),减少了互相依赖的辅助函数数量,使卸载/安装流程更容易理解和维护。
</issue_to_address>Hi @zouyonghe! 👋
感谢你通过评论 @sourcery-ai review 来试用 Sourcery!🚀
安装 sourcery-ai bot,即可在每个 pull request 上自动获得代码审查 ✨
请帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈来改进我的审查质量。Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="dashboard/src/views/extension/useExtensionPage.js" line_range="632" />
<code_context>
- if (res.data.status === "error") {
- toast(res.data.message, "error");
- return;
+ const uninstallExtension = async (extensionName, options = null) => {
+ if (!extensionName) return;
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the uninstall and install flows to use a single normalized entry point and centralized orchestration so behavior stays the same but the number of helpers and branching paths is reduced.
For the uninstall and install flows, you can keep all new behavior but reduce the orchestration surface by normalizing inputs and centralizing the control flow. That lets you remove several tiny helpers while preserving compatibility.
### 1. Uninstall flow – normalize target and centralize orchestration
You already conceptually have a normalized target (`{ kind, id }`). You can make that the **single input type** and fold `requestUninstall*` + `performUninstall` into a smaller surface:
```ts
// 1) Single normalized uninstall entry point
type UninstallKind = "normal" | "failed";
interface UninstallTarget {
id: string;
kind: UninstallKind;
}
interface UninstallOptions {
deleteConfig?: boolean;
deleteData?: boolean;
skipConfirm?: boolean;
}
const uninstallTarget = ref<UninstallTarget | null>(null);
const uninstall = async (target: UninstallTarget, options: UninstallOptions = {}) => {
const { deleteConfig = false, deleteData = false, skipConfirm = false } = options;
if (!skipConfirm) {
uninstallTarget.value = target;
showUninstallDialog.value = true;
return;
}
const isFailed = target.kind === "failed";
const endpoint = isFailed ? "/api/plugin/uninstall-failed" : "/api/plugin/uninstall";
const payload = isFailed
? { dir_name: target.id, delete_config: deleteConfig, delete_data: deleteData }
: { name: target.id, delete_config: deleteConfig, delete_data: deleteData };
toast(`${tm("messages.uninstalling")} ${target.id}`, "primary");
try {
const res = await axios.post(endpoint, payload);
if (res.data.status === "error") {
toast(res.data.message, "error");
return;
}
if (!isFailed) {
Object.assign(extension_data, res.data);
}
toast(res.data.message, "success");
await getExtensions();
} catch (err) {
toast(resolveErrorMessage(err), "error");
}
};
```
Then:
```ts
// 2) Template-facing helpers become simple adapters
const requestUninstallPlugin = (name: string) =>
name && uninstall({ kind: "normal", id: name }, { skipConfirm: false });
const requestUninstallFailedPlugin = (dirName: string) =>
dirName && uninstall({ kind: "failed", id: dirName }, { skipConfirm: false });
// 3) Dialog confirm only needs the stored target
const handleUninstallConfirm = async (options: { deleteConfig?: boolean; deleteData?: boolean }) => {
if (!uninstallTarget.value) return;
try {
await uninstall(uninstallTarget.value, { ...options, skipConfirm: true });
} finally {
uninstallTarget.value = null;
showUninstallDialog.value = false;
}
};
```
Backwards compatibility for `uninstallExtension` can just translate into this normalized API instead of adding an extra layer:
```ts
// Keeps old semantics but delegates to the single `uninstall` API
const uninstallExtension = async (
extensionName: string,
optionsOrSkipConfirm: boolean | { deleteConfig?: boolean; deleteData?: boolean } = false,
) => {
if (!extensionName) return;
if (typeof optionsOrSkipConfirm === "boolean") {
return uninstall(
{ kind: "normal", id: extensionName },
{ skipConfirm: optionsOrSkipConfirm },
);
}
return uninstall(
{ kind: "normal", id: extensionName },
{ ...optionsOrSkipConfirm, skipConfirm: true },
);
};
```
This lets you drop `requestUninstall`, `performUninstall` as distinct concepts and keep just `uninstall` + two thin adapters + BC wrapper.
---
### 2. Install flow – single `performInstall` with explicit status
You already created `installFromFile`, `installFromUrl`, and `handleInstallResponse`. You can push the orchestration one step further into a single `performInstall` with a **status enum** return value to make the side effects clearer:
```ts
type InstallSource = "file" | "url";
type InstallStatus = "ok" | "warning-version" | "error";
interface PerformInstallOptions {
source: InstallSource;
ignoreVersionCheck: boolean;
}
const performInstallRequest = async (
{ source, ignoreVersionCheck }: PerformInstallOptions,
) => {
if (source === "file") {
const formData = new FormData();
formData.append("file", upload_file.value);
formData.append("ignore_version_check", String(ignoreVersionCheck));
return axios.post("/api/plugin/install-upload", formData, {
headers: { "Content-Type": "multipart/form-data" },
});
}
return axios.post("/api/plugin/install", {
url: extension_url.value,
proxy: getSelectedGitHubProxy(),
ignore_version_check: ignoreVersionCheck,
});
};
```
Then make `handleInstallResponse` return an explicit status:
```ts
const handleInstallResponse = async (
resData: any,
{ toastStatus = false } = {},
): Promise<InstallStatus> => {
if (resData.status === "warning" &&
resData.data?.warning_type === "astrbot_version_incompatible") {
onLoadingDialogResult(2, resData.message, -1);
showVersionCompatibilityWarning(resData.message);
await refreshExtensionsAfterInstallFailure();
return "warning-version";
}
if (toastStatus) {
toast(resData.message, resData.status === "ok" ? "success" : "error");
}
if (resData.status === "error") {
onLoadingDialogResult(2, resData.message, -1);
await refreshExtensionsAfterInstallFailure();
return "error";
}
return "ok";
};
```
And centralize the common success tail in one helper:
```ts
const finalizeSuccessfulInstall = async (resData: any) => {
onLoadingDialogResult(1, resData.message);
dialog.value = false;
await getExtensions();
viewReadme({
name: resData.data.name,
repo: resData.data.repo || null,
});
await checkAndPromptConflicts();
};
```
Now `newExtension` can delegate to a single orchestrator instead of two separate high‑level flows:
```ts
const newExtension = async (ignoreVersionCheck = false) => {
if (!extension_url.value && !upload_file.value) {
toast(tm("messages.fillUrlOrFile"), "error");
return;
}
if (extension_url.value && upload_file.value) {
toast(tm("messages.dontFillBoth"), "error");
return;
}
loading_.value = true;
loadingDialog.title = tm("status.loading");
loadingDialog.show = true;
const source: InstallSource = upload_file.value ? "file" : "url";
toast(
source === "file"
? tm("messages.installing")
: tm("messages.installingFromUrl") + " " + extension_url.value,
"primary",
);
try {
const res = await performInstallRequest({ source, ignoreVersionCheck });
loading_.value = false;
const status = await handleInstallResponse(res.data, {
toastStatus: source === "url",
});
if (status !== "ok") return;
if (source === "file") {
upload_file.value = null;
} else {
extension_url.value = "";
}
await finalizeSuccessfulInstall(res.data);
} catch (err) {
loading_.value = false;
const message = resolveErrorMessage(err, tm("messages.installFailed"));
if (source === "url") toast(message, "error");
onLoadingDialogResult(2, message, -1);
await refreshExtensionsAfterInstallFailure();
}
};
```
This preserves all your current behaviors (warnings, refresh on failure, toasts, dialog control) but compresses the install surface area into:
- `performInstallRequest`
- `handleInstallResponse` (status‑based)
- `finalizeSuccessfulInstall`
- `newExtension` as a single high‑level orchestrator
You can then remove `installFromFile` and `installFromUrl` entirely.
---
These adjustments keep all the new capabilities (failed plugin uninstall, richer error handling, version warnings, etc.) while reducing the number of interdependent helpers and making the uninstall/install flows easier to follow.
</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.|
@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.
|
Generated docs update PR (pending manual review): AI change summary:
Experimental bot notice:
|
…evs#5535) * feat: improve plugin failure handling and extension list UX * fix: address plugin review comments * fix: clear stale reload feedback on failed plugin reload * fix: refine extension i18n and uninstall flow * fix: refresh extension list after install failure * feat: add random plugin visibility controls in market * refactor: extract extension helpers and simplify uninstall flow * refactor: improve failed plugin diagnostics and uninstall flow * refactor: streamline extension uninstall flow * fix: harden failed plugin install tracking and cleanup * refactor: simplify extension flows and remove unused timed message * fix: improve failed uninstall idempotency and extension error handling * refactor: unify extension install-uninstall orchestration
变更概览
本 PR 统一了后端与 Dashboard 对失败插件的处理方式,并优化了插件管理页与插件市场的交互体验,重点覆盖“安装失败/加载失败可见性”“失败插件可操作性”“安装与卸载流程收敛”。
后端改动
astrbot/core/star/error_messages.py,统一错误格式化来源。Dashboard 改动
performInstallRequest/handleInstallResponse/finalizeSuccessfulInstall),减少分支与重复逻辑。验证结果
pnpm -C dashboard typecheckuv run ruff format .uv run ruff check .