feat(models): archived model display and URL encoding fix#77
Conversation
Models that have been deleted but still have historical completion/embedding
records now appear as grayed-out "Archived" entries in the global model
registry. Users can click the history button to trace past requests.
- Backend: `listUniqueSystemNames()` returns `{ active, archived }` using
SQL UNION query on completions/embeddings tables
- Frontend: new `ArchivedModelRow` component with archive icon, opacity,
and strikethrough styling
- i18n: added Archived/ArchivedTooltip keys for en-US and zh-CN
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Eden Treaty does not URL-encode dynamic path segments, causing 404 errors for model names containing slashes (e.g. `Qwen/Qwen3-8B`). Wrap all `by-system-name` path params with `encodeURIComponent()`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthrough后端将 listUniqueSystemNames 的返回值从字符串数组改为包含 Changes
Sequence Diagram(s)sequenceDiagram
participant Route as 路由层 (registry.tsx)
participant Backend as 后端 (listUniqueSystemNames)
participant SettingsPage as 模型设置页面 (ModelsSettingsPage)
participant API as 模型 API (admin.models)
Route->>Backend: 请求模型名称列表
Backend->>Backend: 计算 active 与 archived 集合
Backend-->>Route: 返回 { active: [...], archived: [...] }
Route->>SettingsPage: 传入 activeSystemNames 与 archivedSystemNames
SettingsPage->>SettingsPage: 渲染活跃行与已归档行
SettingsPage->>API: GET /models/by-system-name/{encodeURIComponent(systemName)}
API-->>SettingsPage: 返回模型详情
评估代码审查工作量🎯 3 (中等) | ⏱️ ~35 分钟 诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the model management experience by enhancing data traceability and application robustness. It introduces the capability to visualize models that have been deleted but still possess historical usage data, ensuring that no valuable historical context is lost. Concurrently, it addresses a critical bug by correctly handling URL encoding for model names, which prevents errors and ensures seamless interaction with the API for all model identifiers. 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.
Code Review
This pull request introduces a feature to display archived models and fixes a URL encoding issue. The implementation is mostly solid, but I have a couple of suggestions. In the backend, the query for archived models can be simplified to improve readability and avoid duplicating logic. On the frontend, there's a bug in the new ArchivedModelRow component where the history link is incorrect for embedding models. This will require a backend change to resolve properly. The URL encoding fix looks correct.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/db/index.ts (1)
952-994:⚠️ Potential issue | 🟠 Major
modelType在归档查询中被忽略,归档结果会与请求类型不一致这里
active按modelType过滤,但archived仍固定查询 completions+embeddings,并且排除 active 时也未按model_type对齐。传入chat或embedding时会出现归档集合不准确的问题。🔧 建议修复(按 modelType 对齐 archived 源与排除条件)
export async function listUniqueSystemNames( modelType?: ModelTypeEnumType, ): Promise<{ active: string[]; archived: string[] }> { logger.debug("listUniqueSystemNames", modelType); // Get active model names (non-deleted models with non-deleted providers) const r = await db @@ .orderBy(asc(schema.ModelsTable.systemName)); const active = r.map((x) => x.systemName); + const archivedSourceSql = + modelType === "chat" + ? sql`SELECT DISTINCT model FROM completions WHERE deleted = false` + : modelType === "embedding" + ? sql`SELECT DISTINCT model FROM embeddings WHERE deleted = false` + : sql`SELECT DISTINCT model FROM completions WHERE deleted = false + UNION + SELECT DISTINCT model FROM embeddings WHERE deleted = false`; + // Get archived model names: exist in completions/embeddings but not in active models const archivedResult = await db.execute(sql` - SELECT DISTINCT model AS name FROM ( - SELECT DISTINCT model FROM completions WHERE deleted = false - UNION - SELECT DISTINCT model FROM embeddings WHERE deleted = false - ) AS all_models - WHERE model NOT IN ( + SELECT DISTINCT src.model AS name + FROM (${archivedSourceSql}) AS src + WHERE src.model NOT IN ( SELECT DISTINCT m.system_name FROM models m INNER JOIN providers p ON m.provider_id = p.id - WHERE m.deleted = false AND p.deleted = false + WHERE m.deleted = false + AND p.deleted = false + ${modelType ? sql`AND m.model_type = ${modelType}` : sql``} ) ORDER BY name ASC `);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/db/index.ts` around lines 952 - 994, The archived query in listUniqueSystemNames ignores modelType, causing mismatched archived results; update the archivedResult SQL so when modelType is provided it (1) filters the UNION source to only the relevant table(s) (e.g., only embeddings for embedding type, only completions for chat/type that maps to completions) or includes a WHERE model_type = :modelType in each source, and (2) adds the same model_type filter to the exclusion subquery that selects DISTINCT m.system_name from models (aliases m and p) so the NOT IN comparison aligns with the active query; modify the archivedResult construction (and its parameterization) to pass modelType into the SQL and then map archived from the returned name column as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/pages/settings/models-settings-page.tsx`:
- Around line 173-176: handleHistoryClick currently always navigates to
'/requests' which breaks access for archived models that only have embedding
history; update the click handler (handleHistoryClick) to read a model type
indicator (e.g., modelType on the archived model alongside systemName) and route
conditionally using navigate: if modelType === 'request' go to '/requests', if
modelType === 'embedding' go to the embeddings history route (e.g.,
'/embeddings'), passing model and modelType in search; preserve
e.stopPropagation() and add a safe fallback to '/requests' if modelType is
missing or unknown while ensuring the archived data includes the modelType field
for future use.
---
Outside diff comments:
In `@backend/src/db/index.ts`:
- Around line 952-994: The archived query in listUniqueSystemNames ignores
modelType, causing mismatched archived results; update the archivedResult SQL so
when modelType is provided it (1) filters the UNION source to only the relevant
table(s) (e.g., only embeddings for embedding type, only completions for
chat/type that maps to completions) or includes a WHERE model_type = :modelType
in each source, and (2) adds the same model_type filter to the exclusion
subquery that selects DISTINCT m.system_name from models (aliases m and p) so
the NOT IN comparison aligns with the active query; modify the archivedResult
construction (and its parameterization) to pass modelType into the SQL and then
map archived from the returned name column as before.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/db/index.tsfrontend/src/i18n/locales/en-US.jsonfrontend/src/i18n/locales/zh-CN.jsonfrontend/src/pages/models/models-registry-table.tsxfrontend/src/pages/settings/models-settings-page.tsxfrontend/src/routes/models/registry.tsx
…ting
- Backend: archived query now respects `modelType` param and returns
`{ systemName, modelType }` per archived entry (instead of plain string)
- Backend: simplified SQL by fetching all historical names and filtering
in TypeScript, avoiding duplicated active-model logic in NOT IN subquery
- Frontend: `ArchivedModelRow` routes to `/embeddings` for embedding
models instead of hardcoding `/requests`
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/pages/settings/models-settings-page.tsx (1)
64-66:⚠️ Potential issue | 🟡 MinorReact key 存在潜在冲突风险。
如果后端将来返回相同
systemName但不同modelType的归档模型,当前仅使用item.systemName作为 key 会导致 React 渲染异常。建议使用复合 key 以确保唯一性。🛡️ 建议使用复合 key
- {archivedSystemNames.map((item) => ( - <ArchivedModelRow key={item.systemName} systemName={item.systemName} modelType={item.modelType} /> + {archivedSystemNames.map((item) => ( + <ArchivedModelRow key={`${item.modelType}:${item.systemName}`} systemName={item.systemName} modelType={item.modelType} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/settings/models-settings-page.tsx` around lines 64 - 66, The list rendering uses item.systemName alone as the React key which can collide if two archived entries share systemName but differ in modelType; update the map to use a composite key (e.g., combine item.systemName and item.modelType) when rendering ArchivedModelRow so each element has a unique key (refer to archivedSystemNames, ArchivedModelRow, item.systemName and item.modelType).
🧹 Nitpick comments (1)
frontend/src/pages/settings/models-settings-page.tsx (1)
195-197: 可考虑显示归档模型的类型标签。当前
modelType列显示 "-",但组件已接收modelType属性。显示实际类型(带归档样式)可提升用户体验,便于区分 chat 和 embedding 模型的历史记录。💡 可选:显示带样式的模型类型
<TableCell> - <span className="text-muted-foreground text-sm">-</span> + <Badge variant="outline" className="text-muted-foreground text-xs opacity-70"> + {modelType === 'chat' + ? t('pages.settings.models.columns.Chat') + : t('pages.settings.models.columns.Embedding')} + </Badge> </TableCell>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/settings/models-settings-page.tsx` around lines 195 - 197, Replace the hardcoded "-" in the TableCell with the actual modelType value passed into the component (use the modelType prop or model.modelType used elsewhere in this file) and render it as a styled archived tag (e.g., a small badge/span with classes like "text-sm px-2 rounded bg-muted-foreground/10 text-muted-foreground" or similar) so archived models show "chat" or "embedding" visibly; update the TableCell containing the dash to display this styled model type badge instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/pages/settings/models-settings-page.tsx`:
- Around line 64-66: The list rendering uses item.systemName alone as the React
key which can collide if two archived entries share systemName but differ in
modelType; update the map to use a composite key (e.g., combine item.systemName
and item.modelType) when rendering ArchivedModelRow so each element has a unique
key (refer to archivedSystemNames, ArchivedModelRow, item.systemName and
item.modelType).
---
Nitpick comments:
In `@frontend/src/pages/settings/models-settings-page.tsx`:
- Around line 195-197: Replace the hardcoded "-" in the TableCell with the
actual modelType value passed into the component (use the modelType prop or
model.modelType used elsewhere in this file) and render it as a styled archived
tag (e.g., a small badge/span with classes like "text-sm px-2 rounded
bg-muted-foreground/10 text-muted-foreground" or similar) so archived models
show "chat" or "embedding" visibly; update the TableCell containing the dash to
display this styled model type badge instead.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/db/index.tsfrontend/src/pages/settings/models-settings-page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/db/index.ts
Summary
Qwen/Qwen3-8B). Allby-system-namepath params are now wrapped withencodeURIComponent().Changes
Backend
listUniqueSystemNames()now returns{ active: string[], archived: string[] }instead ofstring[]completions+embeddingstables, excluding active model namesFrontend
ArchivedModelRowcomponent with archive icon, opacity-50, and strikethrough stylingModelsSettingsPageacceptsactiveSystemNamesandarchivedSystemNamespropsencodeURIComponent(systemName)to all Eden Treatyby-system-namecallsTest plan
Qwen/Qwen3-8B) display providers correctlybun run check)🤖 Generated with Claude Code
Summary by CodeRabbit
新功能
改进
本地化