feat(skills): add batch upload functionality for multiple skill ZIP files#5804
Conversation
…iles - Implemented a new endpoint for batch uploading skills. - Enhanced the SkillsSection component to support multiple file selection and drag-and-drop functionality. - Updated localization files for new upload features and messages. - Added tests to validate batch upload behavior and error handling.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the skill management system by enabling users to upload multiple skill packages concurrently. This feature streamlines the process of adding new skills to the platform, improving efficiency and user experience. The changes span both the backend API for processing batch requests and the frontend interface for an intuitive multi-file upload and feedback mechanism. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并留下了一些总体反馈:
- 在
batch_upload_skills中,建议使用唯一的临时文件名(例如通过uuid4或tempfile)来代替os.path.join(temp_dir, f"batch_{filename}"),以避免当多个上传文件同名时发生冲突,并减少在路径中对用户提供文件名的依赖。 batch_upload_skills中的临时文件清理逻辑既在单个文件循环内部删除文件,又在finally块中通过temp_paths再次清理;可以通过只保留一种清理机制来简化逻辑,避免重复记录和潜在的不一致。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `batch_upload_skills`, consider generating unique temp filenames (e.g., with `uuid4` or `tempfile`) instead of `os.path.join(temp_dir, f"batch_{filename}")` to avoid collisions when multiple uploads share the same filename and to reduce reliance on user-provided names in paths.
- The temp file cleanup logic in `batch_upload_skills` both deletes files inside the per-file loop and again in the `finally` block via `temp_paths`; you can simplify this by choosing a single cleanup mechanism to avoid redundant bookkeeping and potential inconsistencies.
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/routes/skills.py" line_range="192" />
<code_context>
except Exception:
logger.warning(f"Failed to remove temp skill file: {temp_path}")
+ async def batch_upload_skills(self):
+ """批量上传多个 skill ZIP 文件"""
+ if DEMO_MODE:
</code_context>
<issue_to_address>
**issue (complexity):** 考虑通过将单个文件的处理逻辑和响应构建拆分到辅助函数中,并把临时文件清理交由这些辅助函数处理,从而重构 `batch_upload_skills`。
你可以通过抽取每个文件的处理逻辑,并集中构建响应,同时简化临时文件清理逻辑,从而降低 `batch_upload_skills` 中新增的复杂度。
### 1. 抽取单文件处理辅助函数(在两种上传路径中复用)
将单个文件的逻辑(校验、临时文件创建、安装和清理)移动到一个辅助函数中。这可以消除与 `upload_skill` 的重复逻辑,并简化 `batch_upload_skills` 的控制流:
```python
async def _install_skill_zip(
self, upload_file, skill_mgr: SkillManager, temp_dir: str
) -> dict:
filename = os.path.basename(upload_file.filename or "unknown.zip")
if not filename.lower().endswith(".zip"):
return {"filename": filename, "error": "Only .zip files are supported"}
temp_path = os.path.join(temp_dir, f"batch_{filename}")
try:
await upload_file.save(temp_path)
skill_name = skill_mgr.install_skill_from_zip(temp_path, overwrite=True)
return {"filename": filename, "name": skill_name}
except Exception as e:
return {"filename": filename, "error": str(e)}
finally:
if os.path.exists(temp_path):
try:
os.remove(temp_path)
except Exception:
logger.warning(f"Failed to remove temp skill file: {temp_path}")
```
然后 `batch_upload_skills` 就会变成一个更清晰的循环:
```python
async def batch_upload_skills(self):
if DEMO_MODE:
return Response().error("You are not permitted to do this operation in demo mode").__dict__
try:
files = await request.files
file_list = files.getlist("files")
if not file_list:
return Response().error("No files provided").__dict__
succeeded, failed = [], []
skill_mgr = SkillManager()
temp_dir = get_astrbot_temp_path()
os.makedirs(temp_dir, exist_ok=True)
for file in file_list:
result = await self._install_skill_zip(file, skill_mgr, temp_dir)
if "error" in result:
failed.append(result)
else:
succeeded.append(result)
try:
await sync_skills_to_active_sandboxes()
except Exception:
logger.warning("Failed to sync uploaded skills to active sandboxes.")
return self._build_batch_upload_response(file_list, succeeded, failed)
except Exception as e:
logger.error(traceback.format_exc())
return Response().error(str(e)).__dict__
```
你也可以在 `upload_skill` 中调用 `_install_skill_zip`,以避免在那里的临时文件逻辑重复。
### 2. 集中构建响应
与其在三个分支中重复构建相同结构的 `data`,不如把这部分逻辑提取到一个小的辅助函数中:
```python
def _build_batch_upload_response(self, files, succeeded, failed):
total = len(files)
success_count = len(succeeded)
data = {
"total": total,
"succeeded": succeeded,
"failed": failed,
}
if success_count == 0:
msg = f"Upload failed for all {total} file(s)."
resp = Response().error(msg)
resp.data = data
return resp.__dict__
if success_count == total:
msg = f"All {total} skill(s) uploaded successfully."
return Response().ok(data, msg).__dict__
msg = f"Partial success: {success_count}/{total} skill(s) uploaded."
return Response().ok(data, msg).__dict__
```
这样可以去除重复的响应结构构建逻辑,让处理函数更专注于控制流。
### 3. 简化临时文件追踪
当 `_install_skill_zip` 完全负责清理时,你可以移除 `batch_upload_skills` 中的 `temp_paths` 和最外层的 `finally`。全局兜底清理就不再需要了,因为每个文件对应的临时路径都在同一个地方进行管理。
</issue_to_address>帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈不断改进代码审查质量。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
batch_upload_skills, consider generating unique temp filenames (e.g., withuuid4ortempfile) instead ofos.path.join(temp_dir, f"batch_{filename}")to avoid collisions when multiple uploads share the same filename and to reduce reliance on user-provided names in paths. - The temp file cleanup logic in
batch_upload_skillsboth deletes files inside the per-file loop and again in thefinallyblock viatemp_paths; you can simplify this by choosing a single cleanup mechanism to avoid redundant bookkeeping and potential inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `batch_upload_skills`, consider generating unique temp filenames (e.g., with `uuid4` or `tempfile`) instead of `os.path.join(temp_dir, f"batch_{filename}")` to avoid collisions when multiple uploads share the same filename and to reduce reliance on user-provided names in paths.
- The temp file cleanup logic in `batch_upload_skills` both deletes files inside the per-file loop and again in the `finally` block via `temp_paths`; you can simplify this by choosing a single cleanup mechanism to avoid redundant bookkeeping and potential inconsistencies.
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/routes/skills.py" line_range="192" />
<code_context>
except Exception:
logger.warning(f"Failed to remove temp skill file: {temp_path}")
+ async def batch_upload_skills(self):
+ """批量上传多个 skill ZIP 文件"""
+ if DEMO_MODE:
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `batch_upload_skills` by extracting per-file processing and response building into helpers while delegating temp file cleanup to them.
You can reduce the new complexity in `batch_upload_skills` by extracting the per‑file logic and centralizing response construction, while also simplifying temp file cleanup.
### 1. Extract single‑file processing helper (reuse in both upload paths)
Move the per‑file logic (validation, temp file creation, install, cleanup) into a helper. This removes duplication with `upload_skill` and flattens the control flow in `batch_upload_skills`:
```python
async def _install_skill_zip(
self, upload_file, skill_mgr: SkillManager, temp_dir: str
) -> dict:
filename = os.path.basename(upload_file.filename or "unknown.zip")
if not filename.lower().endswith(".zip"):
return {"filename": filename, "error": "Only .zip files are supported"}
temp_path = os.path.join(temp_dir, f"batch_{filename}")
try:
await upload_file.save(temp_path)
skill_name = skill_mgr.install_skill_from_zip(temp_path, overwrite=True)
return {"filename": filename, "name": skill_name}
except Exception as e:
return {"filename": filename, "error": str(e)}
finally:
if os.path.exists(temp_path):
try:
os.remove(temp_path)
except Exception:
logger.warning(f"Failed to remove temp skill file: {temp_path}")
```
Then `batch_upload_skills` becomes a clean loop:
```python
async def batch_upload_skills(self):
if DEMO_MODE:
return Response().error("You are not permitted to do this operation in demo mode").__dict__
try:
files = await request.files
file_list = files.getlist("files")
if not file_list:
return Response().error("No files provided").__dict__
succeeded, failed = [], []
skill_mgr = SkillManager()
temp_dir = get_astrbot_temp_path()
os.makedirs(temp_dir, exist_ok=True)
for file in file_list:
result = await self._install_skill_zip(file, skill_mgr, temp_dir)
if "error" in result:
failed.append(result)
else:
succeeded.append(result)
try:
await sync_skills_to_active_sandboxes()
except Exception:
logger.warning("Failed to sync uploaded skills to active sandboxes.")
return self._build_batch_upload_response(file_list, succeeded, failed)
except Exception as e:
logger.error(traceback.format_exc())
return Response().error(str(e)).__dict__
```
You can also call `_install_skill_zip` from `upload_skill` to avoid duplicating the temp file logic there.
### 2. Centralize response construction
Instead of three branches that all build the same `data` shape, move that logic to a small helper:
```python
def _build_batch_upload_response(self, files, succeeded, failed):
total = len(files)
success_count = len(succeeded)
data = {
"total": total,
"succeeded": succeeded,
"failed": failed,
}
if success_count == 0:
msg = f"Upload failed for all {total} file(s)."
resp = Response().error(msg)
resp.data = data
return resp.__dict__
if success_count == total:
msg = f"All {total} skill(s) uploaded successfully."
return Response().ok(data, msg).__dict__
msg = f"Partial success: {success_count}/{total} skill(s) uploaded."
return Response().ok(data, msg).__dict__
```
This removes the repeated response shape and keeps the handler focused on control flow.
### 3. Simplify temp file tracking
With `_install_skill_zip` owning cleanup, you can drop `temp_paths` and the outer `finally` in `batch_upload_skills`. Global fallback cleanup is no longer needed because each file’s temp path is managed in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds batch upload support for Skill ZIP archives in the AstrBot Dashboard, enabling users to upload multiple skill packages in one action and receive per-file results.
Changes:
- Backend: adds
POST /api/skills/batch-uploadto install multiple ZIPs and return a per-file success/failure summary. - Frontend: enhances the Skills upload dialog to support multi-select, drag-and-drop, queued file list with statuses, and batch upload submission.
- i18n/tests: adds new localized strings and introduces dashboard tests for the new batch upload endpoint.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
astrbot/dashboard/routes/skills.py |
Adds the batch upload API endpoint that saves, installs, and aggregates results for multiple ZIP files. |
dashboard/src/components/extension/SkillsSection.vue |
Updates the upload UI to handle multi-file selection/drag-drop, show queue/status, and call the batch upload API. |
dashboard/src/i18n/locales/zh-CN/features/extension.json |
Adds Chinese strings for the new batch upload UI. |
dashboard/src/i18n/locales/en-US/features/extension.json |
Adds English strings for the new batch upload UI. |
tests/test_dashboard.py |
Adds tests covering error and success flows for the batch upload endpoint. |
There was a problem hiding this comment.
Code Review
这个拉取请求为技能(skills)管理页面引入了批量上传功能,允许用户一次性上传多个 ZIP 文件。后端通过一个新的 API 端点来处理批量上传,前端则更新了 UI,支持多文件选择、拖放以及详细的上传状态反馈。
我的审查主要集中在后端实现上。我发现了一个潜在的错误:如果上传的多个文件同名,临时文件会发生冲突。我已经提出了修复建议以确保文件名的唯一性。此外,我还指出了部分冗余的清理逻辑,简化后可以提高代码的可维护性。前端的改动和新增的测试看起来很不错,不过测试用例可以进一步覆盖文件名冲突的场景。
…ibility and usability
… entries and improving upload dialog styling
…onsistency and usability
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review it |
|
To use Codex here, create a Codex account and connect to github. |
为 AstrBot Dashboard 的 Skills 管理页面添加批量上传功能,支持用户一次选择多个 ZIP 文件上传,并显示每个文件的上传结果。
Modifications / 改动点
后端改动:
astrbot/dashboard/routes/skills.py:新增POST /api/skills/batch-upload端点,循环调用现有install_skill_from_zip()方法,返回每个文件的成功/失败结果前端改动:
dashboard/src/components/extension/SkillsSection.vue:multiple)国际化:
dashboard/src/i18n/locales/zh-CN/features/extension.json:新增批量上传相关中文文案dashboard/src/i18n/locales/en-US/features/extension.json:新增批量上传相关英文文案文件变更统计:
astrbot/dashboard/routes/skills.pydashboard/src/components/extension/SkillsSection.vuedashboard/src/i18n/locales/zh-CN/features/extension.jsondashboard/src/i18n/locales/en-US/features/extension.jsontests/test_dashboard.pyScreenshots or Test Results / 运行截图或测试结果
测试验证:
功能验证:
做了翻译的
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
在仪表板中添加批量技能上传支持,使其能够在一次操作中上传、验证并汇总多个 ZIP 文件的结果。
New Features:
POST /api/skills/batch-upload接口,用于在一次请求中处理多个技能 ZIP 文件,并返回每个文件的处理结果。Enhancements:
Tests:
Original summary in English
Summary by Sourcery
Add batch skill upload support to the dashboard, enabling multiple ZIP files to be uploaded, validated, and reported in a single operation.
New Features:
Enhancements:
Tests: