feat: implement web config editor#486
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
WalkthroughThis pull request adds a web-based YAML configuration editor to the llama-swap application. It introduces backend API endpoints for retrieving and updating configuration files, a frontend configuration page with Monaco editor and CLI reference panel, and documentation recommending the --watch-config flag for web UI edits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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 |
![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
🤖 Fix all issues with AI agents
In `@proxy/proxymanager_api.go`:
- Around line 285-333: The apiUpdateConfig handler releases pm.Lock() before
validating and writing the config, which can let concurrent calls interleave and
corrupt or lose writes; fix by holding the mutex (pm.Lock()/pm.Unlock()) for the
whole validation-and-write sequence (or introduce a dedicated configMutex used
around the YAML validation, os.WriteFile call, and the subsequent event.Emit of
ConfigFileChangedEvent) so these operations are atomic with respect to other
updates, and change the file write permission from 0644 to 0600 when calling
os.WriteFile to restrict group/world read access for sensitive config content.
🧹 Nitpick comments (4)
ui/package.json (1)
14-14: Move@types/js-yamlto devDependencies.Type definitions are only needed at compile time, not runtime. This package should be in
devDependenciesrather thandependencies.📦 Suggested change
"dependencies": { "@monaco-editor/react": "^4.7.0", - "@types/js-yaml": "^4.0.9", "js-yaml": "^4.1.1", ... }, "devDependencies": { + "@types/js-yaml": "^4.0.9", "@eslint/js": "^9.25.0", ... }ui/src/pages/Config.tsx (3)
25-28: Consider addingloadConfigto the dependency array or suppressing the lint warning.The
useEffectwith an empty dependency array will only run on mount, which is the intended behavior. However, sinceloadConfigis defined withuseCallback, ESLint'sreact-hooks/exhaustive-depsrule may flag this. You can either add a lint suppression comment or includeloadConfigin the deps (it's stable due touseCallback).Option 1: Add dependency
// Load config on mount useEffect(() => { loadConfig(); - }, []); + }, [loadConfig]);Option 2: Suppress lint warning
// Load config on mount + // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => { loadConfig(); }, []);
60-86: Consider replacingalert()andconfirm()with custom modal components for better UX.Using native browser dialogs works but blocks the main thread and doesn't match the application's styling. This is a minor UX polish item that could be addressed in a follow-up.
130-153: Context line inclusion may produce duplicates in edge cases.The
filteredLines.includes()check prevents adding the same line object twice, but if identical text appears on different lines (e.g., blank lines or repeated comments), you may get visual duplicates. This is a minor cosmetic issue.Alternative: track indices instead of content
const filteredCliArgs = useMemo(() => { if (!cliArgsSearch.trim()) return CLI_ARGS_REFERENCE; const searchLower = cliArgsSearch.toLowerCase(); const lines = CLI_ARGS_REFERENCE.split('\n'); - const filteredLines: string[] = []; + const includedIndices = new Set<number>(); for (let i = 0; i < lines.length; i++) { const line = lines[i]; if (line.toLowerCase().includes(searchLower)) { // Include some context: previous line and next line - if (i > 0 && !filteredLines.includes(lines[i - 1])) { - filteredLines.push(lines[i - 1]); - } - filteredLines.push(line); - if (i < lines.length - 1 && !filteredLines.includes(lines[i + 1])) { - filteredLines.push(lines[i + 1]); - } + if (i > 0) includedIndices.add(i - 1); + includedIndices.add(i); + if (i < lines.length - 1) includedIndices.add(i + 1); } } + const filteredLines = [...includedIndices].sort((a, b) => a - b).map(i => lines[i]); return filteredLines.length > 0 ? filteredLines.join('\n') : 'No matches found.'; }, [cliArgsSearch]);
proxy/proxymanager_api.go
Outdated
| func (pm *ProxyManager) apiUpdateConfig(c *gin.Context) { | ||
| pm.Lock() | ||
| configPath := pm.configPath | ||
| pm.Unlock() | ||
|
|
||
| if configPath == "" { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "config path not set"}) | ||
| return | ||
| } | ||
|
|
||
| var req struct { | ||
| Content string `json:"content" binding:"required"` | ||
| } | ||
|
|
||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | ||
| return | ||
| } | ||
|
|
||
| // Validate YAML syntax by attempting to load it | ||
| _, err := config.LoadConfigFromReader(strings.NewReader(req.Content)) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("invalid YAML configuration: %v", err)}) | ||
| return | ||
| } | ||
|
|
||
| // Get the absolute path of the original config to prevent path traversal | ||
| absConfigPath, err := filepath.Abs(configPath) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to resolve config path"}) | ||
| return | ||
| } | ||
|
|
||
| // Write to the config file | ||
| err = os.WriteFile(absConfigPath, []byte(req.Content), 0644) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to write config file: %v", err)}) | ||
| return | ||
| } | ||
|
|
||
| // Emit config file changed event to trigger reload | ||
| event.Emit(ConfigFileChangedEvent{ | ||
| ReloadingState: ReloadingStateStart, | ||
| }) | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{ | ||
| "message": "config updated successfully, reloading...", | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential race condition between lock release and file write.
The lock is released at line 288 before the file validation and write operations. If concurrent requests call apiUpdateConfig, they could interleave, potentially causing one write to be lost or partial writes.
Consider holding the lock through the entire operation, or using a separate mutex for config file operations.
🔧 Suggested fix
func (pm *ProxyManager) apiUpdateConfig(c *gin.Context) {
- pm.Lock()
- configPath := pm.configPath
- pm.Unlock()
+ pm.Lock()
+ defer pm.Unlock()
+
+ configPath := pm.configPath
if configPath == "" {
c.JSON(http.StatusBadRequest, gin.H{"error": "config path not set"})
return
}
+
+ // ... rest of the function with lock heldAdditionally, the file permission 0644 allows group and world read access. For config files that may contain secrets, consider using 0600 for stricter permissions.
📝 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.
| func (pm *ProxyManager) apiUpdateConfig(c *gin.Context) { | |
| pm.Lock() | |
| configPath := pm.configPath | |
| pm.Unlock() | |
| if configPath == "" { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "config path not set"}) | |
| return | |
| } | |
| var req struct { | |
| Content string `json:"content" binding:"required"` | |
| } | |
| if err := c.ShouldBindJSON(&req); err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | |
| return | |
| } | |
| // Validate YAML syntax by attempting to load it | |
| _, err := config.LoadConfigFromReader(strings.NewReader(req.Content)) | |
| if err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("invalid YAML configuration: %v", err)}) | |
| return | |
| } | |
| // Get the absolute path of the original config to prevent path traversal | |
| absConfigPath, err := filepath.Abs(configPath) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to resolve config path"}) | |
| return | |
| } | |
| // Write to the config file | |
| err = os.WriteFile(absConfigPath, []byte(req.Content), 0644) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to write config file: %v", err)}) | |
| return | |
| } | |
| // Emit config file changed event to trigger reload | |
| event.Emit(ConfigFileChangedEvent{ | |
| ReloadingState: ReloadingStateStart, | |
| }) | |
| c.JSON(http.StatusOK, gin.H{ | |
| "message": "config updated successfully, reloading...", | |
| }) | |
| } | |
| func (pm *ProxyManager) apiUpdateConfig(c *gin.Context) { | |
| pm.Lock() | |
| defer pm.Unlock() | |
| configPath := pm.configPath | |
| if configPath == "" { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "config path not set"}) | |
| return | |
| } | |
| var req struct { | |
| Content string `json:"content" binding:"required"` | |
| } | |
| if err := c.ShouldBindJSON(&req); err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request body"}) | |
| return | |
| } | |
| // Validate YAML syntax by attempting to load it | |
| _, err := config.LoadConfigFromReader(strings.NewReader(req.Content)) | |
| if err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("invalid YAML configuration: %v", err)}) | |
| return | |
| } | |
| // Get the absolute path of the original config to prevent path traversal | |
| absConfigPath, err := filepath.Abs(configPath) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to resolve config path"}) | |
| return | |
| } | |
| // Write to the config file | |
| err = os.WriteFile(absConfigPath, []byte(req.Content), 0644) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to write config file: %v", err)}) | |
| return | |
| } | |
| // Emit config file changed event to trigger reload | |
| event.Emit(ConfigFileChangedEvent{ | |
| ReloadingState: ReloadingStateStart, | |
| }) | |
| c.JSON(http.StatusOK, gin.H{ | |
| "message": "config updated successfully, reloading...", | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@proxy/proxymanager_api.go` around lines 285 - 333, The apiUpdateConfig
handler releases pm.Lock() before validating and writing the config, which can
let concurrent calls interleave and corrupt or lose writes; fix by holding the
mutex (pm.Lock()/pm.Unlock()) for the whole validation-and-write sequence (or
introduce a dedicated configMutex used around the YAML validation, os.WriteFile
call, and the subsequent event.Emit of ConfigFileChangedEvent) so these
operations are atomic with respect to other updates, and change the file write
permission from 0644 to 0600 when calling os.WriteFile to restrict group/world
read access for sensitive config content.
1 participant
Implemented a tab in the web ui which allows direct editing, import/export of the config.yaml. Some references and the config.yaml.example are provided at the side panel.
I am aware of the potential issues of command injection by pasting arbitrary file content. This is tackled by yaml validation and the recommendation this should not be run publicly anyways.
Love this project! Will replace my ollama instance which improved to be somewhat restrictive and dont utilize many features of llama.cpp.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.