Repository navigation
fix(provider-models): delete an individual synced model, persistently (#3199) - #3211
diegosouzapw wants to merge 1 commit into
Conversation
…eep it deleted (#3199) Synced (fetched) models were authoritative with no per-model delete, so individual llama-cpp models couldn't be removed (or re-appeared on re-fetch). Add removeSyncedAvailableModel + mark the id hidden so re-import skips it; the DELETE route now removes from both custom and synced sets. Co-authored-by: tjengbudi <tjengbudi@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![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: aaa0d87abe
ℹ️ 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 normalizedModels = normalizeSyncedAvailableModels(models).filter( | ||
| (m) => !getModelIsHidden(providerId, m.id) | ||
| ); |
There was a problem hiding this comment.
When a refresh/auto-fetch runs after a synced model was deleted, this filter prevents the hidden ID from being written back to SQLite, but persistDiscoveredModels() still returns its pre-filtered normalized input and src/app/api/providers/[id]/models/route.ts treats any non-empty discoveredModels as a successful API result and returns the raw upstream models. In that scenario the just-deleted model still appears in the /api/providers/[id]/models?refresh=true response whenever the upstream advertises it, and if all discovered IDs are hidden the route also skips the intended local fallback; return/use the filtered list from replaceSyncedAvailableModelsForConnection() or filter the API response before deciding it is non-empty.
Useful? React with 👍 / 👎.
|
Superseded — #3204 already landed the core synced-model delete on release/v3.8.11. Re-doing as a minimal follow-up (mark deleted synced model hidden + skip it on re-import) so the deletion persists across auto-fetch. |
Code Review SummaryStatus: Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (4 files)
Reviewed by laguna-m.1-20260312:free · 2,261,871 tokens |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const targetId = toNonEmptyString(modelId); | ||
| if (!targetId) return false; | ||
|
|
||
| const rows: Array<{ key: string; value: string }> = []; |
There was a problem hiding this comment.
WARNING: Type annotation mismatch — rows is typed as Array<{ key: string; value: string }>, but getKeyValue returns { key: string | null; value: string | null }. The null checks on lines 796-797 correctly handle this, but the type annotation is misleading and could cause confusion for future maintainers. Consider using Array<{ key: string; value: string } | { key: null; value: null }> or a more precise type.
Closes #3199
Synced (fetched) models were authoritative with no per-model delete, so individual llama-cpp models couldn't be removed (or re-appeared on re-fetch). Adds
removeSyncedAvailableModel+ marks the id hidden so re-import skips it; DELETE now removes from both custom and synced sets. No migration (reuses the existing hidden-model marker).Test:
tests/unit/synced-model-delete-3199.test.ts. Thanks @tjengbudi.