Repository navigation
Hosted models list - #332
sanskaraggarwal2025 wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 52 minutes and 49 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (13)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 implements a dynamic update system for the hosted models catalog, allowing the application to fetch and sync model metadata from a remote repository. The changes include a new UpdateModelsList RPC, database schema updates for version tracking, and a frontend synchronization interface. The review identifies several critical improvements: implementing database transactions for atomic version updates, avoiding hardcoded URLs, and reusing HTTP clients for better performance. Further feedback suggests addressing potential version sorting bugs, adding timeouts to background contexts, and optimizing string allocations in the DAO layer.
| for _, model := range manifest.Models { | ||
| if strings.TrimSpace(model.ID) == "" { | ||
| slog.Warn("service:UpdateModelsList", "message", "skipping hosted model with empty id") | ||
| continue | ||
| } | ||
|
|
||
| if strings.TrimSpace(model.Name) == "" { | ||
| model.Name = model.ID | ||
| } | ||
|
|
||
| if err := s.dao.UpsertHostedModel(model, version); err != nil { | ||
| slog.Error("service:UpdateModelsList", "message", "failed to upsert hosted model", "error", err, "modelID", model.ID) | ||
| return fmt.Errorf("failed to update models list") | ||
| } | ||
| } |
There was a problem hiding this comment.
The version update logic is flawed. Currently, the version is updated for each model individually within the loop. Since GetModelCatalogVersion (line 1734) retrieves the latest version from any row in the table, a partial failure during this loop will leave the database in an inconsistent state. Specifically, if at least one model is successfully updated, subsequent calls to UpdateModelsList will see the new version and skip the update, leaving the remaining models outdated. The version should be stored in a separate metadata table and updated only after the entire loop completes successfully, or the entire operation should be wrapped in a database transaction.
| SELECT json_schema_version, model_revision_version | ||
| FROM shared_models_metadata | ||
| WHERE COALESCE(model_revision_version, '') <> '' OR COALESCE(json_schema_version, '') <> '' | ||
| ORDER BY model_revision_version DESC, id ASC |
There was a problem hiding this comment.
The lexicographical sorting of model_revision_version is fragile. If the versioning scheme uses a format like 20260522.2 and 20260522.10, the string comparison will incorrectly identify 20260522.2 as the greater (and thus latest) version because '2' > '1'. Consider using a strictly lexicographical format (e.g., zero-padding the minor version) or a numeric comparison if possible.
| func defaultJSONObjectBytes(value []byte) string { | ||
| if strings.TrimSpace(string(value)) == "" { | ||
| return "{}" | ||
| } | ||
| return string(value) | ||
| } |
There was a problem hiding this comment.
The string(value) conversion is performed twice, which is inefficient as it results in redundant allocations. It is better to convert the byte slice once and reuse the resulting string.
| func defaultJSONObjectBytes(value []byte) string { | |
| if strings.TrimSpace(string(value)) == "" { | |
| return "{}" | |
| } | |
| return string(value) | |
| } | |
| func defaultJSONObjectBytes(value []byte) string { | |
| s := string(value) | |
| if strings.TrimSpace(s) == "" { | |
| return "{}" | |
| } | |
| return s | |
| } |
| } | ||
|
|
||
| var row catalogVersionRow | ||
| // <> is not but not equal-to operator in sql |
There was a problem hiding this comment.
Typo in comment: "not but not" should likely be "but not".
| // <> is not but not equal-to operator in sql | |
| // <> is the not equal-to operator in sql |
| hostedModelsCatalogURL = "https://raw.githubusercontent.com/sanskaraggarwal2025/hosted_models/main/models.json" | ||
| hostedModelsVersionURL = "https://raw.githubusercontent.com/sanskaraggarwal2025/hosted_models/main/models_version.json" |
There was a problem hiding this comment.
The URLs for the models catalog and version are hardcoded to a personal GitHub repository. This creates a maintenance risk and a single point of failure. These should be configurable via the application's configuration or environment variables.
| } | ||
| versionReq.Header.Set("Accept", "application/json") | ||
|
|
||
| client := &http.Client{Timeout: hostedModelsHTTPTimeout} |
There was a problem hiding this comment.
Creating a new http.Client on every invocation of UpdateModelsList is inefficient. In Go, http.Client is designed to be reused across multiple requests to benefit from connection pooling and reduced resource overhead. Consider making the client a field in the ChatService struct.
| } | ||
| return nil | ||
|
|
||
| go s.UpdateModelsList(context.Background()) |
There was a problem hiding this comment.
Using context.Background() for a network-dependent background task is risky as it lacks a timeout. If the HTTP request hangs indefinitely, the goroutine will leak. It is recommended to use a context with a reasonable timeout for this background operation.
Closes #323