添加了批量删除页面功能并修复了docker compose不可以找到uv.lock的问题 #143
Conversation
Added batch page deletion and fixed docker compose not finding uv.lock.
Summary of ChangesHello @xyzhy, 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! 此拉取请求引入了页面批量删除功能，允许用户同时选择并删除多个页面，从而显著提高了管理效率。此外，它还解决了 Docker Compose 环境中 Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
本次 PR 添加了批量删除页面的功能，并修复了 Docker 构建中的一个问题。整体实现不错，前端和后端都已涵盖。
我主要有以下几点反馈：
- 后端性能: 在批量删除的控制器中，存在 N+1 查询问题，当删除大量页面时可能会影响性能。我已提供建议通过一次查询来优化。
- 前端状态管理: 在
useProjectStore中，有几处直接修改了状态（Set对象），这在 Zustand 中是反模式，不会触发 UI 更新。我已提供修复建议，确保状态的不可变性。
此外，我还对后端的函数命名提出了一点小建议，以更好地遵循 PEP 8 规范。
请查看具体的评论和代码建议。
| setToSelectList: async (pageId) => { | ||
| const { selectedList } = get() | ||
| selectedList.add(pageId) | ||
| }, | ||
| // 从选择列表中移除页面 | ||
| removeFromSelectList: async (pageId) => { | ||
| const { selectedList } = get() | ||
| selectedList.delete(pageId) | ||
| }, |
There was a problem hiding this comment.
setToSelectList 和 removeFromSelectList 函数直接修改了 selectedList (一个 Set 对象)。在 Zustand 中，直接修改状态对象（特别是像 Set 或 Map 这样的复杂对象）不会被识别为状态变更，因此不会触发依赖此状态的组件重新渲染。这是一个常见的状态管理陷阱。
为了确保状态更新能被正确检测到，你应该始终创建一个新的 Set 实例。此外，这两个函数不需要是 async 的。
| setToSelectList: async (pageId) => { | |
| const { selectedList } = get() | |
| selectedList.add(pageId) | |
| }, | |
| // 从选择列表中移除页面 | |
| removeFromSelectList: async (pageId) => { | |
| const { selectedList } = get() | |
| selectedList.delete(pageId) | |
| }, | |
| setToSelectList: (pageId) => set((state) => ({ | |
| selectedList: new Set(state.selectedList).add(pageId) | |
| })), | |
| // 从选择列表中移除页面 | |
| removeFromSelectList: (pageId) => set((state) => { | |
| const newList = new Set(state.selectedList); | |
| newList.delete(pageId); | |
| return { selectedList: newList }; | |
| }), |
| batchRemovePages: async () => { | ||
| const { selectedList, currentProject } = get() | ||
| if (selectedList.size <= 0 || !currentProject || !currentProject.id) return; | ||
|
|
||
| try { | ||
| await api.deleteBatchPage(currentProject.id, [...selectedList]) | ||
| await get().syncProject(); | ||
| selectedList.clear() | ||
| } catch (error: any) { | ||
| set({ error: error.message || '批量删除页面失败' }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
在 batchRemovePages 函数中，selectedList.clear() 同样直接修改了状态，这不会触发 UI 更新。在批量删除成功后，选择列表应该被重置为一个新的空 Set 以确保状态更新被正确地传播。
另外，为了保持错误处理的一致性，建议在 catch 块中使用 normalizeErrorMessage 函数来格式化错误消息。
| batchRemovePages: async () => { | |
| const { selectedList, currentProject } = get() | |
| if (selectedList.size <= 0 || !currentProject || !currentProject.id) return; | |
| try { | |
| await api.deleteBatchPage(currentProject.id, [...selectedList]) | |
| await get().syncProject(); | |
| selectedList.clear() | |
| } catch (error: any) { | |
| set({ error: error.message || '批量删除页面失败' }); | |
| } | |
| }, | |
| batchRemovePages: async () => { | |
| const { selectedList, currentProject } = get() | |
| if (selectedList.size <= 0 || !currentProject || !currentProject.id) return; | |
| try { | |
| await api.deleteBatchPage(currentProject.id, [...selectedList]) | |
| await get().syncProject(); | |
| set({ selectedList: new Set() }); | |
| } catch (error: any) { | |
| set({ error: normalizeErrorMessage(error.message || '批量删除页面失败') }); | |
| } | |
| }, |
| for page_id in page_ids.split(','): | ||
| page = Page.query.get(page_id) | ||
|
|
||
| if not page or page.project_id != project_id: | ||
| return not_found('Page') | ||
|
|
||
| # Delete page image if exists | ||
| file_service = FileService(current_app.config['UPLOAD_FOLDER']) | ||
| file_service.delete_page_image(project_id, page_id) | ||
|
|
||
| # Delete page | ||
| db.session.delete(page) |
There was a problem hiding this comment.
在循环中为每个 page_id 执行数据库查询（Page.query.get(page_id)）效率低下，会导致 N+1 查询问题。当批量删除大量页面时，这会造成显著的性能瓶颈。
建议在循环外使用 in_ 操作符一次性获取所有页面对象，然后进行处理。这样可以将多次数据库查询合并为一次，大大提高效率。
| for page_id in page_ids.split(','): | |
| page = Page.query.get(page_id) | |
| if not page or page.project_id != project_id: | |
| return not_found('Page') | |
| # Delete page image if exists | |
| file_service = FileService(current_app.config['UPLOAD_FOLDER']) | |
| file_service.delete_page_image(project_id, page_id) | |
| # Delete page | |
| db.session.delete(page) | |
| page_id_list = page_ids.split(',') | |
| # Use a single query to fetch all pages for efficiency. | |
| pages_to_delete = Page.query.filter( | |
| Page.project_id == project_id, | |
| Page.id.in_(page_id_list) | |
| ).all() | |
| # Verify that all requested pages were found. | |
| if len(pages_to_delete) != len(set(page_id_list)): | |
| return not_found('Page') | |
| file_service = FileService(current_app.config['UPLOAD_FOLDER']) | |
| for page in pages_to_delete: | |
| # Delete page image if exists | |
| file_service.delete_page_image(project_id, page.id) | |
| # Delete page | |
| db.session.delete(page) |
| return error_response('SERVER_ERROR', str(e), 500) | ||
|
|
||
| @page_bp.route('/<project_id>/pages/batch/<page_ids>', methods=['DELETE']) | ||
| def delete_pageList(project_id, page_ids): |
There was a problem hiding this comment.
函数名 delete_pageList 不符合 PEP 8 命名规范。Python 函数名应使用蛇形命名法（snake_case）。建议修改为 delete_pages_batch 以提高代码可读性和一致性。
| def delete_pageList(project_id, page_ids): | |
| def delete_pages_batch(project_id, page_ids): |
- 后台批量删除将多次查询改为一次 - 将批量删除方法名从 delete_pageList 变成 delete_pages_batch
|
/gemini review |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
你好，感谢你的贡献！这次的 PR 主要添加了批量删除页面的功能，并修复了 Docker Compose 中 uv.lock 文件未包含的问题。
整体来看，批量删除功能的后端和前端实现都很不错。我发现了一些可以改进的地方：
- 后端批量删除接口在处理空的页面ID列表时存在一个边缘情况，可能会导致意外的 "Not Found" 错误。
- 前端
useProjectStore中意外地移除了一个必要的类型导入，这会引起编译错误。
此外，.dockerignore 的修改是正确的，解决了 uv.lock 的问题。
具体的修改建议请看我的评论。修复这些问题后，代码会更加健壮。做得很好！
| @@ -1,5 +1,5 @@ | |||
| import { create } from 'zustand'; | |||
| import type { Project, Task } from '@/types'; | |||
| import type { Project } from '@/types'; | |||
There was a problem hiding this comment.
你好，这个文件中的 Task 类型从导入中被移除了，但是 useProjectStore state 和 action 中多处都用到了 Task 类型（例如 pollTask 的参数和 getTaskStatus 的返回值）。
这会导致 TypeScript 编译错误。请将 Task 类型重新添加回导入语句中。
| import type { Project } from '@/types'; | |
| import type { Project, Task } from '@/types'; |
| DELETE /api/projects/{project_id}/pages/batch/{page_ids} - Batch delete page | ||
| """ | ||
| try: | ||
| page_id_list = page_ids.split(',') |
There was a problem hiding this comment.
你好，这里对 page_ids 的处理存在一个边缘情况。如果 page_ids 是一个空字符串，page_ids.split(',') 会返回 ['']。这会导致后续的 len(set(page_id_list)) 值为 1，而数据库查询结果为空，从而错误地返回 not_found('Page')。
建议在分割字符串后过滤掉空值，以正确处理空 page_ids 的情况。
| page_id_list = page_ids.split(',') | |
| page_id_list = [pid for pid in page_ids.split(',') if pid] |
- 在后端控制器中添加对空页面ID的过滤逻辑 - 防止因空ID导致的数据库查询错误 fix(frontend): 更新项目状态管理中的类型定义 - 为项目状态管理器添加Task类型导入 - 保持前端类型定义的一致性
2 participants
Added batch page deletion and fixed docker compose not finding uv.lock.