Conversation
WalkthroughAdds admin wiring and a reload callback to ProxyManager, exposes GET/PUT /api/config to view and atomically update YAML config, introduces atomic file-write utility, and a UI ConfigEditor page with route; server startup now attaches admin controls to new ProxyManager instances. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CE as UI: ConfigEditor
participant S as Server: ProxyManager
participant FS as File System
participant EB as Event Bus
participant W as fsnotify Watcher
rect rgb(243,248,255)
note right of CE: Load config
U->>CE: Open /config
CE->>S: GET /api/config
S->>FS: Read pm.configPath
FS-->>S: YAML bytes or error
S-->>CE: 200 text/plain (YAML) or 500
end
rect rgb(241,252,244)
note right of CE: Save edited config
U->>CE: Click Save
CE->>S: PUT /api/config (text/plain body)
S->>S: Validate YAML (LoadConfigFromReader)
alt valid
S->>FS: WriteFileAtomic(pm.configPath, data, mode)
FS-->>S: ok
alt watchConfigEnabled == true
note over S,W: Rely on watcher to detect change
S-->>CE: 200 {"msg":"ok"}
FS-->>W: file change event
W->>S: change detected
S->>EB: ConfigFileChangedEvent (start)
else watchConfigEnabled == false
S->>EB: ConfigFileChangedEvent (start)
S->>S: reloadCallback() (if non-nil)
S-->>CE: 200 {"msg":"ok"}
end
else invalid or write error
S-->>CE: 400/500 with error
end
end
sequenceDiagram
autonumber
participant RS as Reloader (llama-swap.go)
participant PM as ProxyManager (newPM)
participant SRV as HTTP Server
note over RS: Reload or initial start
RS->>PM: proxy.New(config)
RS->>PM: SetAdminControls(configPath, watchConfig, reloadProxyManager)
alt replacing existing handler
RS->>SRV: srv.Handler = PM
else initial start
RS->>SRV: srv.Handler = PM
end
RS->>RS: Emit ConfigFileChangedEvent after delay (unchanged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
llama-swap.go (2)
68-82: Avoid data race: replacing srv.Handler at runtime is unsafe; use an atomic handler switcher.http.Server reads Handler concurrently on each request. Writing srv.Handler from reloadProxyManager causes a data race and may serve a partially torn handler under load. Replace the direct assignment with an always-present delegating handler that atomically swaps the inner handler.
Apply this diff within the changed block to stop assigning to srv.Handler directly:
- srv.Handler = newPM + // swap through atomic switcher instead of mutating http.Server + handlerSwitcher.Store(newPM)And similarly below:
- srv.Handler = newPM + handlerSwitcher.Store(newPM)Outside this hunk (supporting code), introduce an atomic delegating handler and initialize http.Server once with it:
// supporting code (outside selected lines) type atomicHandler struct{ v atomic.Value } // stores http.Handler func (h *atomicHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if hh, ok := h.v.Load().(http.Handler); ok && hh != nil { hh.ServeHTTP(w, r) return } http.NotFound(w, r) } func (h *atomicHandler) Store(next http.Handler) { h.v.Store(next) } // after creating srv := &http.Server{ Addr: *listenStr }, set: var handlerSwitcher atomicHandler srv.Handler = &handlerSwitcher // on first load (else branch) also call: // handlerSwitcher.Store(newPM)This removes the race while allowing hot-swaps without restarting the server. If you prefer, sync/atomic.Value can be replaced with an RWMutex-guarded pointer. Also consider postponing Shutdown of the old manager until the switch is complete (see next comment).
68-82: Serialize concurrent reloads to prevent overlapping Shutdown/New cycles.reloadProxyManager can be called from fsnotify (debounced) and from the config PUT callback. Without a guard, two reloads can interleave, racing on internal ProxyManager state and the handler swap.
Add a reload mutex:
@@ - var reloadProxyManager func() + var reloadProxyManager func() + var reloadMu sync.Mutex reloadProxyManager = func() { + reloadMu.Lock() + defer reloadMu.Unlock()Supporting change: add
syncto imports.
🧹 Nitpick comments (15)
proxy/proxymanager.go (1)
631-637: Make SetAdminControls thread-safe (future-proof), and document call timing.Even if today this is only called on a fresh
ProxyManagerbefore serving, protecting against future races is cheap. Wrap the assignments with the manager’s mutex.func (pm *ProxyManager) SetAdminControls(configPath string, watchEnabled bool, reload func()) { - pm.configPath = configPath - pm.watchConfigEnabled = watchEnabled - pm.reloadCallback = reload + pm.Lock() + defer pm.Unlock() + pm.configPath = configPath + pm.watchConfigEnabled = watchEnabled + pm.reloadCallback = reload }Also add a short doc comment noting it should be called before starting the HTTP server (or that it’s safe to call at runtime).
proxy/fileutil.go (1)
42-47: Redundant cleanup paths; simplify to one well-scoped defer.You both define
cleanup()and alsodefera second best‑effortos.Remove(tmpName). It’s harmless but noisy. Consider a single deferred cleanup that closes/removes the temp file only if rename hasn’t succeeded.- tmpName := tmpFile.Name() - cleanup := func() { - _ = tmpFile.Close() - _ = os.Remove(tmpName) - } - - // Ensure cleanup on any failure path - defer func() { - // If tmpFile still exists (e.g., rename failed), best-effort remove it - _ = os.Remove(tmpName) - }() + tmpName := tmpFile.Name() + renamed := false + defer func() { + // Best-effort close; ignore error if already closed + _ = tmpFile.Close() + if !renamed { + _ = os.Remove(tmpName) + } + }()And then after a successful
os.Rename, setrenamed = true.- if err = os.Rename(tmpName, path); err != nil { + if err = os.Rename(tmpName, path); err != nil { // best-effort cleanup - _ = os.Remove(tmpName) return err } +renamed = trueui/src/App.tsx (2)
38-46: Drop the unnecessary cast; HTMLHeadingElement already has blur().
e.currentTargetisHTMLHeadingElement, which extendsHTMLElementand exposesblur(); the cast is redundant.- (e.currentTarget as HTMLElement).blur(); + e.currentTarget.blur();
35-41: Minor a11y polish for editable title.For a contentEditable heading, add an accessible label to clarify it’s editable.
- <h1 + <h1 contentEditable suppressContentEditableWarning className="flex items-center p-0 outline-none hover:bg-gray-100 dark:hover:bg-gray-700 rounded px-1" + aria-label="Application title (editable)" + title="Click to edit the window title"proxy/proxymanager_config_api.go (5)
15-21: Handle disabled/unknown config path and return YAML content type.When
pm.configPathis empty or the file is missing, returning 404 (or a clear 503) avoids generic 500s. Also set a YAML content type.func (pm *ProxyManager) apiGetConfig(c *gin.Context) { - data, err := os.ReadFile(pm.configPath) + if pm.configPath == "" { + c.JSON(http.StatusNotFound, gin.H{"error": "config API disabled"}) + return + } + data, err := os.ReadFile(pm.configPath) if err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + if os.IsNotExist(err) { + c.JSON(http.StatusNotFound, gin.H{"error": "config file not found"}) + } else { + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + } return } - c.Data(http.StatusOK, "text/plain; charset=utf-8", data) + c.Data(http.StatusOK, "application/x-yaml; charset=utf-8", data) }
26-32: Bound request body size to avoid unbounded allocs.Wrap the body with
http.MaxBytesReader(e.g., 1 MiB) beforeio.ReadAll.func (pm *ProxyManager) apiPutConfig(c *gin.Context) { - // Read entire body as text (accept text/plain or application/x-yaml, but don't hard fail on content-type) - body, err := io.ReadAll(c.Request.Body) + // Limit body to 1 MiB to prevent abuse. + c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, 1<<20) + // Read entire body as text (accept text/plain or application/x-yaml, but don't hard fail on content-type) + body, err := io.ReadAll(c.Request.Body) if err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "failed to read request body"}) return }
46-49: Return write error details for easier troubleshooting.Including
err.Error()helps operators diagnose permission/path issues.- if err := WriteFileAtomic(pm.configPath, body, mode); err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to write config file"}) + if err := WriteFileAtomic(pm.configPath, body, mode); err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to write config file", "details": err.Error()}) return }
51-62: Don’t block the HTTP response while reloading; fire reload asynchronously.Reloads can be slow. Emit the event and return quickly; let SSE convey progress.
} else { // Emit start event then call reload callback event.Emit(ConfigFileChangedEvent{ ReloadingState: ReloadingStateStart, }) if pm.reloadCallback != nil { - pm.reloadCallback() + go pm.reloadCallback() } }
23-65: Security: Pair route-level same-origin guard with handler checks (defense-in-depth).If you don’t adopt the middleware approach, minimally verify same-origin here to prevent CSRF against the PUT endpoint. I can fold an inline check into both handlers.
Would you like me to push an alternative patch that performs an Origin/Host check directly inside these handlers instead of route middleware?
llama-swap.go (1)
79-81: Graceful swap order and in-flight requests.You Shutdown the current manager before constructing and swapping to the new one. If Shutdown cancels request contexts, in-flight requests may fail during a reload. Prefer: build newPM, publish it via handlerSwitcher.Store(newPM), then gracefully drain and Shutdown the old manager.
Example within this block (assuming the atomic switcher from the earlier comment):
- currentPM.Shutdown() - newPM := proxy.New(config) - newPM.SetAdminControls(*configPath, *watchConfig, reloadProxyManager) - handlerSwitcher.Store(newPM) + newPM := proxy.New(config) + newPM.SetAdminControls(*configPath, *watchConfig, reloadProxyManager) + // publish new handler first + handlerSwitcher.Store(newPM) + // now drain old PM + currentPM.Shutdown()This minimizes disruption during reloads.
ui/src/pages/ConfigEditor.tsx (5)
119-126: Avoid re-fetch during an active save.The reconnect-triggered fetch can race with a PUT, overwriting the editor with server content mid-save. Gate on saving as well.
- if (prev !== "connected" && connectionStatus === "connected" && !isDirty) { + if (prev !== "connected" && connectionStatus === "connected" && !isDirty && !saving) {Also add
savingto the dependency list of this effect.
167-180: Improve icon button accessibility with aria-labels.Icon-only buttons need accessible names. Titles aren’t a reliable substitute.
- <button className="btn" onClick={toggleFontSize} title="Font size"> + <button className="btn" onClick={toggleFontSize} title="Font size" aria-label="Toggle font size"> <RiFontSize /> </button> - <button className="btn" onClick={toggleWrap} title={wrapText ? "Disable wrap" : "Enable wrap"}> + <button + className="btn" + onClick={toggleWrap} + title={wrapText ? "Disable wrap" : "Enable wrap"} + aria-label={wrapText ? "Disable wrap" : "Enable wrap"} + > {wrapText ? <RiTextWrap /> : <RiAlignJustify />} </button> <button className="btn" onClick={handleSave} disabled={!isDirty || saving || loading} - title="Save configuration" + title="Save configuration" + aria-label="Save configuration" > {saving ? "Saving..." : "Save"} </button>
195-203: Add keyboard shortcut (Ctrl/Cmd+S) to save.Small UX win for an editor.
<textarea className={`w-full h-full font-mono ${fontSizeClass} bg-background text-foreground border border-border rounded p-3 outline-none focus:border-primary focus:ring-0 resize-none`} value={content} onChange={(e) => setContent(e.target.value)} spellCheck={false} wrap={wrapText ? "soft" : "off"} disabled={loading} aria-label="YAML configuration editor" + onKeyDown={(e) => { + if ((e.ctrlKey || e.metaKey) && e.key.toLowerCase() === "s") { + e.preventDefault(); + handleSave(); + } + }} />
58-75: Broaden JSON error parsing for better messages.Some APIs return {message: "..."} instead of {error: "..."}. Fall back to message if present.
if (ct.includes("application/json")) { const data = await res.json(); if (data && typeof data.error === "string") { return data.error as string; } - return JSON.stringify(data); + if (data && typeof data.message === "string") { + return data.message as string; + } + return JSON.stringify(data); }
135-141: Content-Type Header SemanticsI’ve confirmed that the
apiPutConfighandler inproxy/proxymanager_config_api.goreads the raw request body and does not enforce theContent-Typeheader (it accepts either text/plain or YAML without hard-failing) and then validates the payload as YAML. Sending the config usingContent-Type: "text/plain"will work correctly.• File: ui/src/pages/ConfigEditor.tsx
- Current fetch call (lines 135–141) uses
"Content-Type": "text/plain"for the PUT to/api/config.- The backend ignores this header and parses the body as YAML regardless.
Optional improvement: if you’d like the header to more accurately reflect the payload, consider changing it to one of the standard YAML media types, e.g.,
- headers: { "Content-Type": "text/plain" }, + headers: { "Content-Type": "application/x-yaml" }, // or "text/yaml"This won’t affect functionality but provides clearer intent for proxies, middleware, and API consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
llama-swap.go(3 hunks)proxy/fileutil.go(1 hunks)proxy/proxymanager.go(2 hunks)proxy/proxymanager_api.go(1 hunks)proxy/proxymanager_config_api.go(1 hunks)ui/src/App.tsx(5 hunks)ui/src/pages/ConfigEditor.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
proxy/proxymanager.go (1)
proxy/proxymanager_test.go (1)
TestProxyManager_StartupHooks(837-894)
ui/src/pages/ConfigEditor.tsx (2)
ui/src/contexts/APIProvider.tsx (2)
useAPI(221-227)APIProvider(53-219)ui/src/hooks/usePersistentState.ts (1)
usePersistentState(3-39)
proxy/proxymanager_config_api.go (5)
proxy/proxymanager.go (1)
ProxyManager(27-50)proxy/config.go (1)
LoadConfigFromReader(198-361)proxy/fileutil.go (1)
WriteFileAtomic(17-92)event/default.go (1)
Emit(28-30)proxy/events.go (3)
ConfigFileChangedEvent(37-39)ReloadingState(30-30)ReloadingStateStart(33-33)
ui/src/App.tsx (1)
ui/src/contexts/ThemeProvider.tsx (2)
connectionIcon(31-34)ThemeProvider(24-89)
llama-swap.go (1)
proxy/proxymanager.go (1)
New(52-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (5)
proxy/proxymanager.go (1)
45-50: Admin wiring fields look good; consider explicit "disabled" behavior when unset.Storing
configPath,reloadCallback, andwatchConfigEnabledon the manager cleanly plumbs what the config API needs. Please ensure that whenconfigPath == ""you treat the admin/config API as disabled (registration and handlers), to avoid 500s and to make intent explicit.Would you like me to wire conditional route registration so GET/PUT /api/config are only mounted when
configPathis set?proxy/fileutil.go (1)
78-83: Windows replace semantics: confirm behavior if destination exists.
os.Renamereplaces the destination atomically on Unix, but on Windows it fails if the destination exists. If Windows support matters, you may need a platform conditional toRemovefirst or use a safer replace pattern.Please confirm target OS support. If Windows is in scope, I can propose a small
GOOS == "windows"branch usingos.Removeof the destination before rename (with appropriate failure handling).ui/src/App.tsx (1)
53-74: Routing/nav wiring for Config page looks solid.
The new nav entry and route mount the editor cleanly under/configand respect the existing/uibasename.Also applies to: 89-89
llama-swap.go (2)
96-99: Initial wiring looks correct.Creating a fresh ProxyManager and wiring SetAdminControls on first load is consistent with the reload path. No issues here.
68-82: Reload semantics and raw-body handling verified, no changes needed
- In
proxy/proxymanager_config_api.go(lines 55–61), whenwatchConfigEnabledis false the code emits aConfigFileChangedEventand then callspm.reloadCallback()as expected.- The request body is read unconditionally via
io.ReadAll(c.Request.Body)(lines 26–28), accepting any content-type without hard failure, matching the intended “raw body” behavior.SetAdminControlscorrectly assignspm.watchConfigEnabledandpm.reloadCallback(inproxy/proxymanager.go, lines 633–637), ensuring the injected callback is used byapiPutConfig.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/src/pages/ConfigEditor.tsx (1)
134-151: Resolved: lost-change bug on save.Capturing payload and setting originalContent to that exact payload fixes the in-flight edit desync flagged earlier.
🧹 Nitpick comments (6)
ui/src/pages/ConfigEditor.tsx (6)
185-192: A11y: announce status updates to screen readers.Mark the status area as a live region so “Loading…/Saved/Error/Unsaved changes” are announced.
- <div className="mt-2 min-h-[24px]"> + <div className="mt-2 min-h-[24px]" role="status" aria-live="polite" aria-atomic="true">
86-90: Avoid cached GETs; be explicit about Accept.Browsers/proxies can cache text; be explicit and disable caching.
- const res = await fetch("/api/config", { + const res = await fetch("/api/config", { method: "GET", signal: controller.signal, + headers: { Accept: "text/plain" }, + cache: "no-store", });
149-154: Clean up the “Saved” timeout to avoid setState-after-unmount.Store/clear the timer; also clear any prior timer before setting a new one.
+ const successTimerRef = useRef<number | null>(null); @@ - setSuccessMsg("Saved"); - // transient success - setTimeout(() => setSuccessMsg(null), 1500); + setSuccessMsg("Saved"); + // transient success + if (successTimerRef.current) clearTimeout(successTimerRef.current); + successTimerRef.current = window.setTimeout(() => setSuccessMsg(null), 1500);Add a tiny cleanup effect:
+ useEffect(() => { + return () => { + if (successTimerRef.current) clearTimeout(successTimerRef.current); + }; + }, []);
128-159: Keyboard shortcut: Cmd/Ctrl+S to save.Common affordance for editors.
+ useEffect(() => { + const onKeyDown = (e: KeyboardEvent) => { + if ((e.metaKey || e.ctrlKey) && e.key.toLowerCase() === "s") { + e.preventDefault(); + if (isDirty && !saving && !loading) { + void handleSave(); + } + } + }; + window.addEventListener("keydown", onKeyDown); + return () => window.removeEventListener("keydown", onKeyDown); + }, [isDirty, saving, loading, handleSave]);
161-183: Guard against accidental navigation with unsaved changes.Warn on tab/window close when dirty.
+ useEffect(() => { + const beforeUnload = (e: BeforeUnloadEvent) => { + if (isDirty) { + e.preventDefault(); + e.returnValue = ""; + } + }; + window.addEventListener("beforeunload", beforeUnload); + return () => window.removeEventListener("beforeunload", beforeUnload); + }, [isDirty]);
167-183: Add “Revert” to discard local edits.Handy for quickly restoring the last loaded/saved version.
<div className="flex items-center gap-2"> <button className="btn" onClick={toggleFontSize} title="Font size"> <RiFontSize /> </button> <button className="btn" onClick={toggleWrap} title={wrapText ? "Disable wrap" : "Enable wrap"}> {wrapText ? <RiTextWrap /> : <RiAlignJustify />} </button> + <button + className="btn" + onClick={() => setContent(originalContent)} + disabled={!isDirty || saving || loading} + title="Revert to last saved" + > + Revert + </button> <button className="btn" onClick={handleSave} disabled={!isDirty || saving || loading} title="Save configuration" > {saving ? "Saving..." : "Save"} </button> </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/pages/ConfigEditor.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/src/pages/ConfigEditor.tsx (2)
ui/src/contexts/APIProvider.tsx (2)
useAPI(221-227)APIProvider(53-219)ui/src/hooks/usePersistentState.ts (1)
usePersistentState(3-39)
🔇 Additional comments (1)
ui/src/pages/ConfigEditor.tsx (1)
77-110: Well-contained fetch lifecycle (abort + reconnect handling).Abort-before-new-fetch, unmount cleanup, and reconnect-triggered reloads look solid. Loading disables editing, preventing read/write races during GET. Nicely done.
Also applies to: 111-118, 119-127
|
@mostlygeek - interested in merging this? I've been runing for a few weeks and its very handy. |
|
Hi, my apologies for not getting to this sooner. This looks like a cool feature and I’m not prepared to maintain this in the main repo at this time. You can feel free to leave this PR open. |
|
Understood. I'll keep my own fork. |
3 participants
Adds a page to live edit the config.yaml :).
Summary by CodeRabbit
New Features
Reliability