Load models in the UI without navigating the page #173
Conversation
WalkthroughA new asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModelsPage
participant APIContext
participant Backend
User->>ModelsPage: Clicks "Load" button
ModelsPage->>APIContext: Calls loadModel(modelId)
APIContext->>Backend: GET /upstream/{modelId}/
Backend-->>APIContext: Response (success or error)
APIContext-->>ModelsPage: Resolves or throws error
ModelsPage-->>User: (Optional) Updates UI or shows error
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ui/src/contexts/APIProvider.tsx (1)
143-155: Consider using POST method for model loading operation.The
loadModelfunction uses a GET request, which is typically for retrieving data rather than triggering actions. Loading a model is a state-changing operation that would conventionally use POST or PUT method.Consider changing to POST method if the API supports it:
- const response = await fetch(`/upstream/${model}/`, { - method: "GET", + const response = await fetch(`/upstream/${model}/load`, { + method: "POST", });ui/src/pages/Models.tsx (1)
6-6: Consider adding loading state for individual models.Similar to how
isUnloadingstate is managed for "Unload All Models", consider adding loading state for individual model operations to provide better user feedback.You could track loading state per model:
+ const [loadingModels, setLoadingModels] = useState<Set<string>>(new Set());Then update the button to show loading state and disable during loading.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/src/contexts/APIProvider.tsx(3 hunks)ui/src/index.css(1 hunks)ui/src/pages/Models.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (3)
ui/src/contexts/APIProvider.tsx (1)
15-15: LGTM! Proper context integration.The
loadModelfunction is correctly added to the interface, context value, and dependency array. The implementation follows established patterns in the codebase.Also applies to: 162-162, 173-173
ui/src/index.css (1)
146-148: Good UX enhancement.Adding pointer cursor on button hover provides clear visual feedback for interactive elements, enhancing the user experience with the new Load button functionality.
ui/src/pages/Models.tsx (1)
46-47: Well-structured table updates.The new table columns are logically organized:
- "Upstream" column provides direct access to model endpoints
- "Actions" column consolidates model operations
- Load button is properly disabled for non-stopped models
- Model name simplification improves clarity
Also applies to: 55-64
| </a> | ||
| </td> | ||
| <td className="p-2"> | ||
| <button className="btn btn--sm" disabled={model.state !== "stopped"} onClick={() => loadModel(model.id)}>Load</button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for model loading.
The Load button calls loadModel without error handling, which could result in unhandled promise rejections if the API call fails. Consider adding try-catch with user feedback.
- <button className="btn btn--sm" disabled={model.state !== "stopped"} onClick={() => loadModel(model.id)}>Load</button>
+ <button className="btn btn--sm" disabled={model.state !== "stopped"} onClick={async () => {
+ try {
+ await loadModel(model.id);
+ // Optional: Add success feedback
+ } catch (error) {
+ console.error('Failed to load model:', error);
+ // Optional: Add user-visible error feedback
+ }
+ }}>Load</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button className="btn btn--sm" disabled={model.state !== "stopped"} onClick={() => loadModel(model.id)}>Load</button> | |
| <button | |
| className="btn btn--sm" | |
| disabled={model.state !== "stopped"} | |
| onClick={async () => { | |
| try { | |
| await loadModel(model.id); | |
| // Optional: Add success feedback | |
| } catch (error) { | |
| console.error('Failed to load model:', error); | |
| // Optional: Add user-visible error feedback | |
| } | |
| }} | |
| > | |
| Load | |
| </button> |
🤖 Prompt for AI Agents
In ui/src/pages/Models.tsx at line 63, the Load button's onClick handler calls
loadModel without error handling, risking unhandled promise rejections. Wrap the
loadModel call in an async function with a try-catch block, and in the catch
block provide user feedback such as an alert or notification indicating the
loading failure.
|
Got a screenshot? The model table looks OK on mobile and I wonder if this makes it too wide. |
|
Tweaked the table for mobile. |
|
looks great! |
2 participants
This tweaks the models page in the UI to allow you to load a model without navigating away from the current page.
Summary by CodeRabbit