fix(models): make switches reflect final visibility#389
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds atomic model-visibility APIs, GUI visibility reconciliation, combo-provider routing support, coverage for model/provider/combo toggles, and localized dashboard documentation. ChangesModel visibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelsPage
participant VisibilityAPI
participant Config
participant CodexCatalog
ModelsPage->>VisibilityAPI: PUT /api/model-visibility
VisibilityAPI->>Config: Validate and persist visibility state
VisibilityAPI->>CodexCatalog: Refresh catalog cache
VisibilityAPI-->>ModelsPage: Return visibility response
ModelsPage->>VisibilityAPI: GET /api/selected-models
VisibilityAPI-->>ModelsPage: Return selected model map
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
399947f to
e8e5915
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8e5915bd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const providerConfig = hasOwnProvider(config.providers, provider) ? config.providers[provider] : undefined; | ||
| const comboProvider = provider === "combo"; |
There was a problem hiding this comment.
When a valid physical provider named combo exists with no combos configured, routeModelInternal explicitly preserves that provider, but this classification forces every /api/model-visibility request for provider combo down the combo-namespace path below, where targets must exist in config.combos and the request returns 400. Since the Models page now uses this endpoint for row switches and All on/off, users of that supported provider name can no longer change visibility; treat combo as the combo namespace only when combos are configured or no physical provider is being preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in b89c5399. The model-visibility route now shares preservesPhysicalComboProvider with routing, so a configured physical combo provider remains a normal routed provider whenever no virtual combos are configured. The regression covers row on/off and provider All on/off behavior.
| disabled = disabled.filter(stored => !allComboSlugs.has(stored)); | ||
| } else { | ||
| const nativeIds = new Set(targets.filter(target => target.native).map(target => target.id)); | ||
| disabled = disabled.filter(stored => !stored.startsWith(`${provider}/`) && !nativeIds.has(stored)); |
There was a problem hiding this comment.
When a combo is disabled by a public alias that looks like a provider slug, e.g. anthropic/fast, clicking All on for provider anthropic removes that unrelated alias from disabledModels because this prefix filter matches any stored string beginning with anthropic/. Combo aliases are allowed to contain one slash and are persisted in the shared disabled list, so provider all-on can unexpectedly re-expose a disabled combo; exclude known combo public aliases or match only provider-owned model slugs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in b89c5399. Provider All on now preserves every configured combo selector, including canonical IDs and public aliases, while clearing only provider-owned slugs. The regression specifically covers the anthropic/fast alias and verifies it remains disabled until the combo namespace is enabled.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The dashboard truthfulness bug is real and the atomic route is the right shape, but two current cases make the mutation unsafe: a legitimate physical provider named combo is forced into the combo namespace, and provider-level All on can remove a disabled combo alias that merely shares the provider prefix. Please classify physical combo providers consistently with routeModelInternal, exclude known combo aliases from provider cleanup, add regressions for both cases, and re-request review.
![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 `@src/server/management/model-routes.ts`:
- Around line 192-210: The provider-scope enable logic in the native branch of
the model route only removes bare native IDs found in targets, leaving stale IDs
disabled. Update the non-virtual provider branch within the body.enabled
handling to remove all bare native IDs for that provider, while preserving known
combo selectors and prefixed IDs; keep the virtual namespace behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7daec4f3-1470-4c65-b630-4b6f93dd6afd
📒 Files selected for processing (6)
src/combos/index.tssrc/combos/types.tssrc/router.tssrc/server/management/model-routes.tstests/combos.test.tstests/model-visibility-management-api.test.ts
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 `@tests/model-visibility-management-api.test.ts`:
- Around line 96-118: Update the test fixture in “all-on clears stale native ids
while preserving combo selectors” so the unrelated routed model uses a
provider-prefixed identifier instead of “fast-chat”, matching
disabledNativeSlugs classification. Keep the assertion aligned with the
configured preserved identifiers and retain the existing stale-native removal
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 123ab41c-ba44-4209-806b-c9db94d9110c
📒 Files selected for processing (2)
src/server/management/model-routes.tstests/model-visibility-management-api.test.ts
|
@Ingwannu I addressed both requested changes in |
|
Re-reviewed against @Ingwannu's CHANGES_REQUESTED — both concerns are resolved on the current head (
ocx sync relationship (the specific thing I was asked to check): no conflict. #389 does not change the CLI sync path; visibility persists via Two things before merge: (1) this head predates current |
792baab to
1e1fa59
Compare
🔒 Maintainer integration in progress — please hold off on merging@lidge-jun (maintainer) is actively integrating this PR into What is happening This PR is being integrated on the maintainer branch What this means for you
This PR will be closed with a merge receipt (integration commit SHA, verification output, Integration tracker: |
PR #389 (head 1e1fa59, author csa906)를 통합한다. dev의 provider workspace 구조와 충돌하므로 3-way 병합 후 두 conflict를 수동 해소했다. 스위치는 disable 플래그만 읽어서 실제로 Codex에 노출되는 목록과 어긋났다. 이제 provider allowlist가 허용하고 disabled가 아닐 때만 visible로 판정하며, provider 카드 카운트와 workspace rail 카운트가 같은 규칙을 쓴다. 충돌 해소: dev의 controls/Tooltip 구조와 renderGroup/visibleGroups 흐름을 보존하고, PR의 최종 가시성 로직만 renderGroup()으로 이식했다. 구형 page-head와 인라인 groups.map은 폐기했다. 빈 provider에서는 bulk 버튼을 비활성으로 유지한다. rows가 비어 있을 때 every()가 true를 반환해 두 버튼이 동시에 활성화되면 빈 targets PUT이 400으로 실패한다. 테스트 조정과 추가: - 통합 테스트의 shadow Select 조회를 document 기준으로 고쳤다. dev의 Select는 listbox를 document.body로 portal하므로 mount 노드 안에 없다 - poll이 forced refresh보다 늦게 도착하는 stale-generation 순서를 실제로 재현하는 회귀를 추가했다. 기존 테스트는 single-flight만 검증해서 shouldApplyLoadGeneration()이 무력화돼도 통과했다 Co-authored-by: csa906 <csa906@users.noreply.github.com>
✅ Integrated into
|
|
Closing: integrated into |
3 participants
Summary
selectedModelsanddisabledModelsPUT /api/model-visibilitymanagement route so a single enable action can repair both filters without exposing a second selection editorProblem
The Models page currently renders a model as ON whenever it is absent from
disabledModels. The Codex catalog applies an additional per-providerselectedModelsallowlist, so a model can appear ON in the dashboard while still being omitted from/v1/modelsand the Codex app.This is especially visible after a provider discovers new models: an existing non-empty allowlist does not contain the new IDs, but the dashboard still presents them as enabled.
Implementation
Tests
1.3.14bun run typecheckbun run lint:guicd gui && bun run test— 24 passed, 0 failedbun run privacy:scanbun run build:guibun run doctor:gui:if-changed— completed with advisory-only existing recommendationsgit diff --checkThe CI-equivalent Windows root suite (
bun test --isolate tests) was also executed: 4,017 passed, 4 skipped, and 7 failed. The new model-visibility tests passed inside that full run. Every non-zero case was then rerun successfully in isolation: two doctor tests passed after putting Bun onPATH, two unchanged Cursor shell tests passed after adding Git'susr/binas on the hosted runner, and three timing-sensitive tests passed with a 15 s limit after taking 4.9–5.8 s against their default 5 s limit.Scope
Summary by CodeRabbit
GET /api/selected-models,PUT /api/model-visibility).