Repository navigation
Pr 3211 - #3239
Pr 3211#3239diegosouzapw wants to merge 2 commits 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>
# Conflicts: # src/app/api/provider-models/route.ts # src/lib/db/models.ts
![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 addresses issue #3199 by ensuring that deleted provider models are marked as hidden and excluded from synced lists, preventing them from reappearing during subsequent auto-fetches. It updates the DELETE and GET API routes, refactors removeSyncedAvailableModel in the database layer to support connection-scoped deletions, and adds comprehensive unit tests. The review feedback highlights two performance and atomicity concerns: an N+1 query issue in the GET route due to calling getModelIsHidden inside a filter loop, and the lack of an explicit transaction when executing multiple database write operations in removeSyncedAvailableModel. Batch-fetching hidden IDs and wrapping database updates in a transaction are recommended to resolve these issues.
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.
| const allSynced = await getSyncedAvailableModels(provider); | ||
| if (Array.isArray(allSynced) && allSynced.length > 0) { | ||
| providerSyncedModels = allSynced.map((m) => ({ | ||
| // #3199 — a model the operator deleted is flagged hidden; drop it from the | ||
| // authoritative synced list so the deletion sticks across re-fetch even when | ||
| // excludeHidden is not requested by the caller. | ||
| const visibleSynced = allSynced.filter((m) => !getModelIsHidden(provider, m.id)); |
There was a problem hiding this comment.
Calling getModelIsHidden inside a filter loop triggers two database queries per model (one for custom models and one for compat overrides). For providers with many models, this results in an N+1 query problem that severely degrades performance. Fetching the hidden model IDs in batch once per request avoids this overhead.
const allSynced = await getSyncedAvailableModels(provider);
// #3199 — a model the operator deleted is flagged hidden; drop it from the
// authoritative synced list so the deletion sticks across re-fetch even when
// excludeHidden is not requested by the caller.
const { getCustomModels, getModelCompatOverrides } = await import("@/lib/localDb");
const [customModels, compatOverrides] = await Promise.all([
getCustomModels(provider),
getModelCompatOverrides(provider),
]);
const hiddenIds = new Set<string>();
for (const m of customModels || []) {
if (m?.isHidden) hiddenIds.add(m.id);
}
for (const co of compatOverrides || []) {
if (co?.isHidden) hiddenIds.add(co.id);
}
const visibleSynced = allSynced.filter((m) => !hiddenIds.has(m.id));| freshSynced = (await getSyncedAvailableModels(provider)).filter( | ||
| (m) => !getModelIsHidden(provider, m.id) | ||
| ); |
There was a problem hiding this comment.
Calling getModelIsHidden inside a filter loop triggers two database queries per model. For providers with many models, this results in an N+1 query problem that severely degrades performance. Fetching the hidden model IDs in batch once per request avoids this overhead.
const allSynced = await getSyncedAvailableModels(provider);
const { getCustomModels, getModelCompatOverrides } = await import("@/lib/localDb");
const [customModels, compatOverrides] = await Promise.all([
getCustomModels(provider),
getModelCompatOverrides(provider),
]);
const hiddenIds = new Set<string>();
for (const m of customModels || []) {
if (m?.isHidden) hiddenIds.add(m.id);
}
for (const co of compatOverrides || []) {
if (co?.isHidden) hiddenIds.add(co.id);
}
freshSynced = allSynced.filter((m) => !hiddenIds.has(m.id));| let changed = false; | ||
| for (const { key, value } of rows) { | ||
| let models: SyncedAvailableModel[]; | ||
| try { | ||
| models = normalizeSyncedAvailableModels(JSON.parse(value)); | ||
| } catch { | ||
| continue; | ||
| } | ||
| const filtered = models.filter((m) => m.id !== targetId); | ||
| if (filtered.length === models.length) continue; | ||
| changed = true; | ||
| if (filtered.length === 0) { | ||
| db.prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?").run( | ||
| key | ||
| ); | ||
| } else { | ||
| db.prepare( | ||
| "UPDATE key_value SET value = ? WHERE namespace = 'syncedAvailableModels' AND key = ?" | ||
| ).run(JSON.stringify(filtered), key); | ||
| } | ||
| } | ||
|
|
||
| if (changed) backupDbFile("pre-write"); | ||
| return changed; |
There was a problem hiding this comment.
Executing multiple database write operations inside a loop without an explicit transaction can lead to performance degradation (due to multiple disk syncs) and a lack of atomicity. Wrapping the updates in a transaction ensures they are executed efficiently and atomically.
let changed = false;
const runUpdates = db.transaction(() => {
for (const { key, value } of rows) {
let models: SyncedAvailableModel[];
try {
models = normalizeSyncedAvailableModels(JSON.parse(value));
} catch {
continue;
}
const filtered = models.filter((m) => m.id !== targetId);
if (filtered.length === models.length) continue;
changed = true;
if (filtered.length === 0) {
db.prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?").run(
key
);
} else {
db.prepare(
"UPDATE key_value SET value = ? WHERE namespace = 'syncedAvailableModels' AND key = ?"
).run(JSON.stringify(filtered), key);
}
}
if (changed) backupDbFile("pre-write");
});
runUpdates();
return changed;
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (4 files)
Note: The existing inline comments for lines 814, 942, and 831 in the PR were noted but not duplicated. Fix Link: https://app.kilo.ai/cloud-agent-fork/review/ed01ad7e-d6c3-4132-a909-a11dd12f5200 Reviewed by laguna-m.1-20260312:free · 1,042,027 tokens |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| let changed = false; |
There was a problem hiding this comment.
CRITICAL: Missing transaction wrapper — the old implementation wrapped all DB operations in db.transaction() for atomicity, but this implementation performs multiple UPDATE/DELETE operations without a transaction. This could leave the database in an inconsistent state if an error occurs mid-operation.
Summary
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Reviewer Notes