Repository navigation
Conversation
…ntrols - Add models.dev sync engine (src/lib/modelsDevSync.ts) for pricing, capabilities, and model specs - Fetch from https://models.dev/api.json (109 providers, 4,146+ models, MIT licensed) - 4-layer pricing resolution: User Override > models.dev > LiteLLM > Hardcoded Default - New model_capabilities DB table for synced capability data - UI toggle in Settings > AI tab: enable/disable sync, configure interval (1h-7d), manual sync trigger - Live stats dashboard showing provider/model/capability counts - New API route /api/settings/models-dev for sync status and manual triggers - Fix 39 missing i18n keys across all 30 languages (Memory & Skills tab fully translated) - 25 unit + integration tests, 1,439 existing tests pass, lint clean, typecheck clean Closes cheaper-inference#979
![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 introduces a new models.dev synchronization feature, allowing the application to automatically sync AI model pricing, capabilities, and specifications from an external open-source database. The changes include a new settings tab, API endpoints for managing the sync, and integration with the existing pricing resolution logic. My review identified critical issues in the settings update logic where the sync process is not correctly managed due to incorrect state comparison, as well as UX improvements needed for error handling in the settings component.
| if ("modelsDevSyncEnabled" in body) { | ||
| const { stopPeriodicSync, startPeriodicSync } = await import("@/lib/modelsDevSync"); | ||
| const settings = await getSettings(); | ||
| const wasEnabled = (settings as Record<string, unknown>).modelsDevSyncEnabled === true; | ||
| const isEnabled = body.modelsDevSyncEnabled === true; | ||
| if (wasEnabled && !isEnabled) { | ||
| stopPeriodicSync(); | ||
| } else if (!wasEnabled && isEnabled) { | ||
| startPeriodicSync(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for starting and stopping the periodic models.dev sync is non-functional because it determines the wasEnabled state from settings that have already been updated. This results in comparing the new state to itself, so the conditions for starting or stopping the sync are never met. Furthermore, changes to modelsDevSyncInterval are not handled, meaning the sync interval is never updated for a running process.
To fix this, the PATCH handler needs to be refactored to fetch the settings before they are updated, and then compare the old and new states to correctly manage the sync process. Here is an example of how the PATCH function could be structured:
export async function PATCH(request) {
// ...
try {
const body = await request.json();
// Get old settings for comparison *before* updating
const { stopPeriodicSync, startPeriodicSync } = await import("@/lib/modelsDevSync");
const oldSettings = await getSettings();
const wasEnabled = oldSettings.modelsDevSyncEnabled === true;
// ... validation ...
const settings = await updateSettings(body);
// ... other logic ...
// Compare old and new settings to manage the sync process
if ("modelsDevSyncEnabled" in body || "modelsDevSyncInterval" in body) {
const isEnabled = settings.modelsDevSyncEnabled === true;
const newInterval = settings.modelsDevSyncInterval;
if (wasEnabled && !isEnabled) {
stopPeriodicSync();
} else if (!wasEnabled && isEnabled) {
startPeriodicSync(newInterval);
} else if (isEnabled && oldSettings.modelsDevSyncInterval !== newInterval) {
stopPeriodicSync();
startPeriodicSync(newInterval);
}
}
// ... return response
} catch (error) {
// ... error handling
}
}| setIntervalHours(Math.round(intervalMs / 3600000)); | ||
| } | ||
| }) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
The catch block in the useEffect hook is empty, causing silent failures if initial settings cannot be fetched. This can lead to a confusing user experience where the component might display default or stale data. It's better to handle this error by logging it and providing feedback to the user.
.catch((err) => {
console.error("Failed to fetch settings:", err);
setFeedback({ type: "error", message: "Failed to load settings" });
});
| const updateInterval = async (hours: number) => { | ||
| setIntervalHours(hours); | ||
| try { | ||
| await fetch("/api/settings", { | ||
| method: "PATCH", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ modelsDevSyncInterval: hours * 3600000 }), | ||
| }); | ||
| } catch { | ||
| // Revert on failure | ||
| } | ||
| }; |
There was a problem hiding this comment.
The updateInterval function optimistically updates the UI but doesn't revert the change if the API call fails. The catch block is empty, and there's no check on the response status. This can lead to a state where the UI shows a different interval value than what's saved on the backend.
const updateInterval = async (hours: number) => {
const oldInterval = intervalHours;
setIntervalHours(hours);
try {
const res = await fetch("/api/settings", {
method: "PATCH",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ modelsDevSyncInterval: hours * 3600000 }),
});
if (!res.ok) {
setIntervalHours(oldInterval);
}
} catch {
// Revert on failure
setIntervalHours(oldInterval);
}
};
Fixes check:route-validation:t06 CI failure. The POST handler now uses validateBody with modelsDevActionSchema instead of raw request.json().catch().
…tion - CRITICAL: Fetch old settings BEFORE update in PATCH handler to correctly compare wasEnabled vs isEnabled for sync lifecycle management - CRITICAL: Handle modelsDevSyncInterval changes (restart periodic sync with new interval when it changes) - MEDIUM: Add error logging and user feedback to useEffect catch - MEDIUM: Add revert logic to updateInterval on API failure Fixes all 3 review comments from Gemini Code Assist on PR cheaper-inference#983
|
Integrated locally after fixing missing sync state checks and UI feedback. Merged into release/v3.5.1. Thanks @oyi77 for the great contribution! 🎉 |
…tion - CRITICAL: Fetch old settings BEFORE update in PATCH handler to correctly compare wasEnabled vs isEnabled for sync lifecycle management - CRITICAL: Handle modelsDevSyncInterval changes (restart periodic sync with new interval when it changes) - MEDIUM: Add error logging and user feedback to useEffect catch - MEDIUM: Add revert logic to updateInterval on API failure Fixes all 3 review comments from Gemini Code Assist on PR cheaper-inference#983
…tion - CRITICAL: Fetch old settings BEFORE update in PATCH handler to correctly compare wasEnabled vs isEnabled for sync lifecycle management - CRITICAL: Handle modelsDevSyncInterval changes (restart periodic sync with new interval when it changes) - MEDIUM: Add error logging and user feedback to useEffect catch - MEDIUM: Add revert logic to updateInterval on API failure Fixes all 3 review comments from Gemini Code Assist on PR cheaper-inference#983
Summary
What Changed
New Files
src/lib/modelsDevSync.tssrc/app/api/settings/models-dev/route.tssrc/app/(dashboard)/dashboard/settings/components/ModelsDevSyncTab.tsxtests/unit/modelsDevSync.test.mjstests/integration/modelsDevSync.test.mjsModified Files
src/lib/db/settings.tsmodels_dev_pricingnamespace as Layer 3 in pricing resolutionsrc/app/api/settings/route.tsmodelsDevSyncEnabledtoggle → start/stop periodic syncsrc/shared/validation/settingsSchemas.tsmodelsDevSyncEnabled,modelsDevSyncIntervalschema fieldssrc/app/(dashboard)/dashboard/settings/page.tsxsrc/i18n/messages/en.jsonsrc/i18n/messages/*.json(29 files)Verification
npm run test:unitnpm run lintnpm run typecheck:coreHow to Test Locally
npm run devCloses #979