Skip to content

fix(providers): allow replacing preferred models#1104

Open
penso wants to merge 1 commit into
mainfrom
troubled-danger
Open

fix(providers): allow replacing preferred models#1104
penso wants to merge 1 commit into
mainfrom
troubled-danger

Conversation

@penso

@penso penso commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Preselect saved provider model preferences when opening the preferred-model dialog.
  • Replace a provider's previous preferred models on save, including clearing all preferences with an empty selection.
  • Add backend and Playwright regression coverage for de-preferring models.

Validation

Completed

  • cargo fmt --all -- --check
  • cargo test -p moltis-provider-setup save_models_
  • npm run build
  • npm run build:all
  • npx tsc --noEmit
  • npx playwright test e2e/specs/providers.spec.js

Remaining

  • Full repository validation not run.

Manual QA

  • Open Settings > Providers.
  • Open Preferred Models for a provider with existing preferred models.
  • Confirm existing preferences are selected, deselect one, save, and confirm only the remaining selected model stays preferred.
  • Deselect all preferred models, save, and confirm no models remain preferred for that provider.

Fixes #1094

@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes preferred-model preferences so that saving a new selection fully replaces the old one — both in persistent key-store and in the in-memory cross-provider priority list — and pre-populates the dialog with the current saved selection.

  • Backend (credentials.rs): save_models_inner now loads the provider's existing model list before writing, then evicts those old entries from the shared priority list before inserting the new selection. Empty saves (deselect-all) are handled correctly because the removal loop still runs, and the insert loop is simply a no-op.
  • Frontend (auth-flow.ts): selectedIds is now seeded by matching the available model list against saved preferences using both a direct ID lookup and a namespace-stripped token comparison, fixing the mismatch between key-store bare names (e.g. gpt-4) and models.list fully-qualified IDs (e.g. openai::gpt-4).
  • Tests: Two backend unit tests cover the replace and clear scenarios; a new Playwright spec validates end-to-end that the dialog preselects the right cards and emits the correct providers.save_models payload after a deselection.

Confidence Score: 5/5

Safe to merge — the replace logic is correct, the failure path (save_config error) leaves the priority list untouched, and both the backend unit tests and the Playwright spec exercise the key scenarios.

The three changed files are tightly scoped: the Rust side correctly evicts old models before inserting new ones, the TypeScript side resolves the bare-name vs namespaced-ID mismatch, and the tests validate both behaviours end-to-end. No edge cases were found that the new code mishandles.

No files require special attention.

Important Files Changed

Filename Overview
crates/provider-setup/src/service/implementation/credentials.rs Adds previous-model eviction from the priority list before inserting the new selection; load/save ordering is correct, error path leaves priority list untouched, and two unit tests verify both replace and clear cases.
crates/web/ui/src/providers/auth-flow.ts Pre-selects saved preferences in the multi-model dialog using a dual-lookup strategy (direct ID match + namespace-stripped token match) to handle the bare-name vs fully-qualified-ID mismatch between key-store and models.list.
crates/web/ui/e2e/specs/providers.spec.js New Playwright test mocks both models.list and providers.available, verifies the dialog pre-selects both saved models, deselects one, and confirms the save RPC carries only the remaining selected model.

Sequence Diagram

sequenceDiagram
    participant UI as auth-flow.ts
    participant WS as WebSocket RPC
    participant SVC as save_models_inner
    participant KS as KeyStore
    participant PL as PriorityList (RwLock)

    UI->>WS: "models.list {}"
    WS-->>UI: "[{id:"openai::gpt-a", ...}, ...]"
    UI->>WS: "providers.available {}"
    WS-->>UI: "[{name:"openai", models:["gpt-a","gpt-b"]}]"
    Note over UI: Build selectedIds by matching model.id vs savedModels using stripModelNamespace fallback
    UI->>UI: User deselects gpt-b
    UI->>WS: "providers.save_models {provider:"openai", models:["openai::gpt-a"]}"
    WS->>SVC: save_models_inner(params)
    SVC->>KS: load_config("openai") → ["gpt-a","gpt-b"]
    SVC->>KS: "save_config("openai", models=["gpt-a"])"
    SVC->>PL: write lock acquired
    loop for each previous model
        PL->>PL: "retain(existing != previous)"
    end
    loop for each new model (rev)
        PL->>PL: retain + insert(0, m)
    end
    SVC-->>UI: "{ok: true}"
Loading

Reviews (1): Last reviewed commit: "fix(providers): allow replacing preferre..." | Re-trigger Greptile

@codspeed-hq

codspeed-hq Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 39 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing troubled-danger (e8a1b5f) with main (a568a3c)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov

codecov Bot commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...er-setup/src/service/implementation/credentials.rs 98.71% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: De-Preferring Models

1 participant