Wire Settings Models to live model health#380
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughДобавлена вкладка «Models» в ChangesВкладка Models в SettingsModal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
This pull request transitions the 'Models' tab in the settings modal from a mockup placeholder to a live, read-only view displaying model health and registry information. It integrates the /api/model-health and /api/models endpoints, introduces state management composables, adds comprehensive localization, and updates tests to verify the new functionality. The review feedback suggests enhancing the user experience by automatically refreshing the model data when the tab is opened to prevent stale views, and adding defensive checks to prevent potential runtime errors if the API returns malformed non-array data.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async function refreshModelSurfaces() { | ||
| await Promise.all([ | ||
| modelHealthState.refresh(), | ||
| modelRegistryState.refresh(), | ||
| ]) | ||
| } |
There was a problem hiding this comment.
The model health and registry states are currently only loaded once when the application starts (via startOnce). When an operator opens the settings modal or switches to the 'Models' tab, they might be presented with stale data if the status has changed in the background. It is highly recommended to automatically refresh these surfaces when the modal is opened and the 'models' tab is active.
async function refreshModelSurfaces() {
await Promise.all([
modelHealthState.refresh(),
modelRegistryState.refresh(),
])
}
watch([open, activeTab], ([isOpen, tab]) => {
if (isOpen && tab === 'models') {
refreshModelSurfaces()
}
}, { immediate: true })
| function mapModelRegistry(payload: ApiModelsResponse | undefined): ModelRegistrySnapshot { | ||
| return { | ||
| models: [...new Set((payload?.models || []).map(modelRegistryName).filter((value): value is string => Boolean(value)))], | ||
| defaultModel: modelRegistryName(payload?.default) || '—', | ||
| currentModel: modelRegistryName(payload?.current) || '—', | ||
| } | ||
| } |
There was a problem hiding this comment.
If the API returns a malformed response where models is not an array (e.g., null or an object), (payload?.models || []) will evaluate to that non-array value, causing .map() to throw a TypeError and crash the state refresh. It is safer to explicitly check if payload?.models is an array using Array.isArray.
| function mapModelRegistry(payload: ApiModelsResponse | undefined): ModelRegistrySnapshot { | |
| return { | |
| models: [...new Set((payload?.models || []).map(modelRegistryName).filter((value): value is string => Boolean(value)))], | |
| defaultModel: modelRegistryName(payload?.default) || '—', | |
| currentModel: modelRegistryName(payload?.current) || '—', | |
| } | |
| } | |
| function mapModelRegistry(payload: ApiModelsResponse | undefined): ModelRegistrySnapshot { | |
| const rawModels = payload && Array.isArray(payload.models) ? payload.models : [] | |
| return { | |
| models: [...new Set(rawModels.map(modelRegistryName).filter((value): value is string => Boolean(value)))], | |
| defaultModel: modelRegistryName(payload?.default) || '—', | |
| currentModel: modelRegistryName(payload?.current) || '—', | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/operator-console/composables/useMockData.ts`:
- Around line 456-459: The mapModelRegistry function assumes that
payload?.models is either falsy or an array, but if the server returns a truthy
non-array value, calling .map() will throw a TypeError. Normalize
payload?.models by adding an Array.isArray() check to ensure it's always treated
as an array before calling .map(modelRegistryName). Replace the current fallback
logic (payload?.models || []) with a proper type guard that returns an empty
array if payload?.models exists but is not an array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58cf6e16-5057-4a39-9a5d-8cb97f20bbfb
📒 Files selected for processing (9)
apps/operator-console/PARITY.jsonapps/operator-console/components/SettingsModal.vueapps/operator-console/composables/useMockData.tsapps/operator-console/i18n/locales/en.jsonapps/operator-console/i18n/locales/ru.jsonapps/operator-console/i18n/locales/zh.jsonapps/operator-console/scripts/mock-operator-api.mjsapps/operator-console/scripts/seam-contract.test.mjsapps/operator-console/tests/browser/settings-modal.spec.ts
| function mapModelRegistry(payload: ApiModelsResponse | undefined): ModelRegistrySnapshot { | ||
| return { | ||
| models: [...new Set((payload?.models || []).map(modelRegistryName).filter((value): value is string => Boolean(value)))], | ||
| defaultModel: modelRegistryName(payload?.default) || '—', |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Нормализуйте payload.models перед вызовом .map()
На Line 458 используется (payload?.models || []).map(...). Если сервер вернёт truthy-значение, которое не является массивом, здесь будет TypeError, и вкладка реестра уйдёт в ошибку из-за формы payload, а не из-за бизнес-ошибки.
💡 Предлагаемое исправление
function mapModelRegistry(payload: ApiModelsResponse | undefined): ModelRegistrySnapshot {
+ const models = Array.isArray(payload?.models) ? payload.models : []
return {
- models: [...new Set((payload?.models || []).map(modelRegistryName).filter((value): value is string => Boolean(value)))],
+ models: [...new Set(models.map(modelRegistryName).filter((value): value is string => Boolean(value)))],
defaultModel: modelRegistryName(payload?.default) || '—',
currentModel: modelRegistryName(payload?.current) || '—',
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/operator-console/composables/useMockData.ts` around lines 456 - 459, The
mapModelRegistry function assumes that payload?.models is either falsy or an
array, but if the server returns a truthy non-array value, calling .map() will
throw a TypeError. Normalize payload?.models by adding an Array.isArray() check
to ensure it's always treated as an array before calling
.map(modelRegistryName). Replace the current fallback logic (payload?.models ||
[]) with a proper type guard that returns an empty array if payload?.models
exists but is not an array.
1 participant
Summary
/api/model-health/api/modelsregistry seam and render its v5-empty state honestlyVerification
npm ci --prefer-offlinenpm run test:seamnpm run paritynpm run buildnpx playwright test tests/browser/settings-modal.spec.tsnpm run test:browsergit diff --checkSummary by CodeRabbit
New Features
Documentation