refactor(SelectModelPopup): use the same base popup for chat and agent pages#13700
Merged
DeJeune merged 8 commits intoCherryHQ:mainfrom Mar 23, 2026
Merged
Conversation
…t pages - `SelectModelPopup` is the base popup now. It accepts filtered models and sorted providers. - `SelectChatModelPopup` is for the chat page. - `SelectAgentModelPopup` is for the agent page.
Collaborator
GeorgeDong32
left a comment
GeorgeDong32
left a comment
There was a problem hiding this comment.
📋 代码审查总结
感谢这个重构 PR！成功消除了约 369 行重复代码，统一了 Chat 和 Agent 页面的模型选择弹窗实现。
✅ 良好实践
- 工厂模式应用得当 -
createModelPopup有效复用了 popup 逻辑 - 组件组合清晰 - 职责分离明确
- 排序逻辑集中化 - 消除了重复实现
- Memoization 使用正确 -
useMemo和useCallback使用恰当
⚠️ 待改进项
| 优先级 | 问题 | 文件 |
|---|---|---|
| 🔴 高 | createModelPopup 中 as any 类型安全问题 |
base-popup.tsx:628 |
| 🔴 高 | Agent popup 中类型转换风险 | agent-model-popup.tsx:80 |
| 🟡 低 | 注释掉的 logger 应清理 | base-popup.tsx:36 |
建议
- 考虑为
createModelPopup添加更严格的泛型约束 - 为类型转换添加运行时检查或类型守卫
- 清理注释代码
整体来说这是一个高质量的重构，建议解决上述类型安全问题后合并。
src/renderer/src/components/Popups/SelectModelPopup/base-popup.tsx
Outdated
Show resolved
Hide resolved
src/renderer/src/components/Popups/SelectModelPopup/base-popup.tsx
Outdated
Show resolved
Hide resolved
src/renderer/src/components/Popups/SelectModelPopup/agent-model-popup.tsx
Outdated
Show resolved
Hide resolved
alephpiece
force-pushed
the
refactor/select-model-popups
branch
from
caa3ec8 to
86de7c2
Compare
EurFelux
reviewed
Mar 22, 2026
src/renderer/src/components/Popups/SelectModelPopup/agent-model-popup.tsx
Show resolved
Hide resolved
src/renderer/src/components/Popups/SelectModelPopup/agent-model-popup.tsx
Outdated
Show resolved
Hide resolved
GeorgeDong32
approved these changes
Mar 22, 2026
Collaborator
GeorgeDong32
left a comment
GeorgeDong32
left a comment
There was a problem hiding this comment.
代码审查总结
✅ 优点
架构改进: 成功消除代码重复，将 ~500 行的 agent-model-popup 精简至 ~100 行，净减少 357 行代码。
设计模式: createModelPopup 工厂函数设计优雅，类型安全，可复用性强。
关注点分离: base-popup、chat-model-popup、agent-model-popup 各司其职，职责清晰。
性能优化: useMemo、useCallback 使用得当，虚拟列表实现高效渲染。
文档质量: fallback provider 的 TODO 注释清晰说明了技术债务和后续改进方向。
类型清理: 正确移除了未使用的 FlatListApiModel 和 FlatListApiItem 类型。
📝 建议性改进
已在内联评论中提出两个建议性改进：
- 添加 API 错误状态处理
- 在非预期情况下添加警告日志
这些都是非阻塞性建议，不影响代码功能正确性。
🎯 结论
批准合并 ✅
这是一个高质量的重构 PR，成功统一了 Chat 和 Agent 页面的模型选择弹窗实现。代码结构清晰，消除了大量重复代码，遵循了 DRY 原则。建议性改进可在后续迭代中考虑。
src/renderer/src/components/Popups/SelectModelPopup/agent-model-popup.tsx
Show resolved
Hide resolved
src/renderer/src/components/Popups/SelectModelPopup/agent-model-popup.tsx
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Uses the same base popup for Chat and Agent.
SelectModelPopupis now the base popup. It accepts filtered models and sorted providers.SelectChatModelPopupis used on the Chat page.SelectAgentModelPopupis used on the Agent page.Maintains consistent sorting logic across Chat and Agent.
Before this PR:
SelectModelPopuphad duplicated implementations.After this PR:
Popup on the Agent page:
Fixes #
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Breaking changes
If this PR introduces breaking changes, please describe the changes and the impact on users.
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note