Repository navigation
fix(providers): save the dashboard provider editor atomically #3296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e36d683
01257e7
e7cceee
35fa983
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,33 @@ import { useCallback, useEffect, useRef, useState } from "react"; | |
| export interface Config { | ||
| port: number; | ||
| defaultProvider: string; | ||
| providers: Record<string, { adapter: string; baseUrl: string; hasApiKey?: boolean; hasHeaders?: boolean; defaultModel?: string; models?: string[]; liveModels?: boolean; upstreamHttpVersion?: "auto" | "http1.1" | "h1" | "http2" | "h2"; reasoningWireFormat?: "gateway-object"; authMode?: string; keyOptional?: boolean; disabled?: boolean; note?: string; codexAccountMode?: "direct" | "pool" }>; | ||
| providers: Record<string, Record<string, unknown> & { adapter: string; baseUrl: string; hasApiKey?: boolean; hasHeaders?: boolean; xaiResponsesOptInState?: boolean | "mixed" }>; | ||
| } | ||
|
|
||
| const PROVIDER_EDITOR_DERIVED_FIELDS = [ | ||
| "hasApiKey", | ||
| "hasHeaders", | ||
| "xaiResponsesOptInState", | ||
| ] as const; | ||
|
|
||
| type ProviderEditorConfig = { | ||
| defaultProvider: string; | ||
| providers: Record<string, Record<string, unknown>>; | ||
| }; | ||
|
|
||
| const PROVIDER_EDITOR_DERIVED_FIELD_SET = new Set<string>(PROVIDER_EDITOR_DERIVED_FIELDS); | ||
|
|
||
| function projectProviderEditorConfig(config: Config): ProviderEditorConfig { | ||
| return { | ||
| defaultProvider: config.defaultProvider, | ||
| providers: Object.fromEntries(Object.entries(config.providers).map(([name, provider]) => { | ||
| const projected: Record<string, unknown> = {}; | ||
| for (const [field, value] of Object.entries(provider)) { | ||
| if (!PROVIDER_EDITOR_DERIVED_FIELD_SET.has(field)) projected[field] = structuredClone(value); | ||
| } | ||
| return [name, projected]; | ||
| })), | ||
| }; | ||
| } | ||
|
|
||
| export function useJsonConfigEditor(deps: { | ||
|
|
@@ -25,17 +51,25 @@ export function useJsonConfigEditor(deps: { | |
| const jsonEditorOpenRef = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| if (config && !jsonEditorOpenRef.current) setDraft(JSON.stringify(config, null, 2)); | ||
| if (config && !jsonEditorOpenRef.current) setDraft(JSON.stringify(projectProviderEditorConfig(config), null, 2)); | ||
| }, [config]); | ||
|
|
||
| const saveConfig = useCallback(async (): Promise<boolean> => { | ||
| setJsonSaving(true); | ||
| let parsed: unknown; | ||
| try { | ||
| parsed = JSON.parse(draft); | ||
| } catch { | ||
| notify(t("prov.invalidJson"), false); | ||
| setJsonSaving(false); | ||
| return false; | ||
| } | ||
| try { | ||
| const parsed = JSON.parse(draft); | ||
| const res = await fetch(`${apiBase}/api/config`, { | ||
| const baseline = JSON.parse(jsonBaseline) as unknown; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win A malformed
If This matters more because of what the new tests claim. Move the baseline parse next to the draft parse, so both local parse failures are classified as local, and only transport failures reach the outer catch. 🐛 Proposed fix for the failure classification setJsonSaving(true);
let parsed: unknown;
+ let baseline: unknown;
try {
parsed = JSON.parse(draft);
} catch {
notify(t("prov.invalidJson"), false);
setJsonSaving(false);
return false;
}
+ try {
+ baseline = JSON.parse(jsonBaseline);
+ } catch {
+ // An unset or corrupt baseline is a local state fault, not a transport fault.
+ // Reopening the editor reseeds it from the projected config.
+ notify(t("prov.saveFailed"), false);
+ setJsonSaving(false);
+ return false;
+ }
try {
- const baseline = JSON.parse(jsonBaseline) as unknown;
const res = await fetch(`${apiBase}/api/providers`, {🤖 Prompt for AI Agents |
||
| const res = await fetch(`${apiBase}/api/providers`, { | ||
| method: "PUT", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(parsed), | ||
| body: JSON.stringify({ baseline, next: parsed }), | ||
| }); | ||
| if (!res.ok) { | ||
| const data = await res.json().catch(() => ({})) as { error?: string }; | ||
|
Comment on lines
74
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When another dashboard or CLI changes provider configuration while this editor is open, the PUT returns AGENTS.md reference: gui/AGENTS.md:L7-L10 Useful? React with 👍 / 👎. |
||
|
|
@@ -53,15 +87,15 @@ export function useJsonConfigEditor(deps: { | |
| onSaved(); | ||
| return true; | ||
| } catch { | ||
| notify(t("prov.invalidJson"), false); | ||
| notify(t("prov.saveFailed"), false); | ||
| return false; | ||
| } finally { | ||
| setJsonSaving(false); | ||
| } | ||
| }, [apiBase, draft, fetchConfig, fetchProviderQuotas, notify, onSaved, t]); | ||
| }, [apiBase, draft, fetchConfig, fetchProviderQuotas, jsonBaseline, notify, onSaved, t]); | ||
|
|
||
| const openJsonEditor = useCallback(() => { | ||
| const baseline = config ? JSON.stringify(config, null, 2) : draft; | ||
| const baseline = config ? JSON.stringify(projectProviderEditorConfig(config), null, 2) : draft; | ||
| setJsonBaseline(baseline); | ||
| setDraft(baseline); | ||
| setJsonLeaveOpen(false); | ||
|
|
@@ -73,7 +107,7 @@ export function useJsonConfigEditor(deps: { | |
| setJsonLeaveOpen(false); | ||
| setJsonEditorOpen(false); | ||
| jsonEditorOpenRef.current = false; | ||
| const baseline = config ? JSON.stringify(config, null, 2) : jsonBaseline; | ||
| const baseline = config ? JSON.stringify(projectProviderEditorConfig(config), null, 2) : jsonBaseline; | ||
| setJsonBaseline(baseline); | ||
| setDraft(baseline); | ||
| }, [config, jsonBaseline]); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,176 @@ | ||||||||||||||||||
| import { afterEach, beforeEach, expect, test } from "bun:test"; | ||||||||||||||||||
| import { Window } from "happy-dom"; | ||||||||||||||||||
| import { act } from "react"; | ||||||||||||||||||
| import type { Root } from "react-dom/client"; | ||||||||||||||||||
| import { useJsonConfigEditor, type Config } from "../src/hooks/useJsonConfigEditor"; | ||||||||||||||||||
|
|
||||||||||||||||||
| const originalFetch = globalThis.fetch; | ||||||||||||||||||
| const originalDocument = globalThis.document; | ||||||||||||||||||
| const originalWindow = globalThis.window; | ||||||||||||||||||
| const originalNavigator = globalThis.navigator; | ||||||||||||||||||
|
|
||||||||||||||||||
| const config: Config = { | ||||||||||||||||||
| port: 10100, | ||||||||||||||||||
| defaultProvider: "alpha", | ||||||||||||||||||
| providers: { | ||||||||||||||||||
| alpha: { | ||||||||||||||||||
| adapter: "openai-chat", | ||||||||||||||||||
| baseUrl: "https://alpha.example.test/v1", | ||||||||||||||||||
| defaultModel: "alpha-old", | ||||||||||||||||||
| modelContextWindows: { "alpha-old": 131_072 }, | ||||||||||||||||||
| modelReasoningEfforts: { "alpha-old": ["low", "high"] }, | ||||||||||||||||||
| noVisionModels: ["alpha-old"], | ||||||||||||||||||
| allowPrivateNetwork: true, | ||||||||||||||||||
| hasApiKey: true, | ||||||||||||||||||
| hasHeaders: true, | ||||||||||||||||||
| note: "derived registry note", | ||||||||||||||||||
|
Comment on lines
+24
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win The fixture exercises only two of the three declared derived fields, so it cannot detect a drifted policy list.
The fixture also omits The Extend the fixture to carry every field the derived policy names, including As per path instructions, this suite must "check that GUI state changes stay consistent with the management API responses." 🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||||
| }, | ||||||||||||||||||
| beta: { | ||||||||||||||||||
| adapter: "anthropic", | ||||||||||||||||||
| baseUrl: "https://beta.example.test/v1", | ||||||||||||||||||
| hasApiKey: false, | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| } as Config; | ||||||||||||||||||
|
|
||||||||||||||||||
| type Editor = ReturnType<typeof useJsonConfigEditor>; | ||||||||||||||||||
| type RequestRecord = { url: string; method: string; body: unknown }; | ||||||||||||||||||
|
|
||||||||||||||||||
| let testWindow: Window; | ||||||||||||||||||
| let host: HTMLElement; | ||||||||||||||||||
| let root: Root | null; | ||||||||||||||||||
| let editor: Editor | null; | ||||||||||||||||||
| let requests: RequestRecord[]; | ||||||||||||||||||
| let responseFactory: () => Promise<Response>; | ||||||||||||||||||
| let configRefreshes: number; | ||||||||||||||||||
| let quotaRefreshes: number; | ||||||||||||||||||
| let savedCallbacks: number; | ||||||||||||||||||
| let notifications: Array<{ message: string; ok?: boolean }>; | ||||||||||||||||||
|
|
||||||||||||||||||
| function Harness() { | ||||||||||||||||||
| editor = useJsonConfigEditor({ | ||||||||||||||||||
| apiBase: "/editor", | ||||||||||||||||||
| config, | ||||||||||||||||||
| notify: (message, ok) => { notifications.push({ message, ok }); }, | ||||||||||||||||||
| fetchConfig: async () => { configRefreshes += 1; }, | ||||||||||||||||||
| fetchProviderQuotas: async () => { quotaRefreshes += 1; }, | ||||||||||||||||||
| onSaved: () => { savedCallbacks += 1; }, | ||||||||||||||||||
| t: key => key, | ||||||||||||||||||
| }); | ||||||||||||||||||
| return null; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| async function mountHook(): Promise<void> { | ||||||||||||||||||
| const { createRoot } = await import("react-dom/client"); | ||||||||||||||||||
| await act(async () => { | ||||||||||||||||||
| root = createRoot(host); | ||||||||||||||||||
| root.render(<Harness />); | ||||||||||||||||||
| }); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| beforeEach(() => { | ||||||||||||||||||
| testWindow = new Window({ url: "http://localhost" }); | ||||||||||||||||||
| Object.defineProperties(globalThis, { | ||||||||||||||||||
| document: { configurable: true, value: testWindow.document }, | ||||||||||||||||||
| window: { configurable: true, value: testWindow }, | ||||||||||||||||||
| navigator: { configurable: true, value: testWindow.navigator }, | ||||||||||||||||||
| IS_REACT_ACT_ENVIRONMENT: { configurable: true, value: true }, | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Restore
Bun executes test files in one process by default, so the flag survives into unrelated suites. React reads that flag to decide how to treat updates outside Note that the file already gets the pattern right for 🧪 Proposed fix for the global leak+const originalActEnvironment = (globalThis as Record<string, unknown>).IS_REACT_ACT_ENVIRONMENT;
+
beforeEach(() => { Object.defineProperties(globalThis, {
document: { configurable: true, value: originalDocument },
window: { configurable: true, value: originalWindow },
navigator: { configurable: true, value: originalNavigator },
+ IS_REACT_ACT_ENVIRONMENT: { configurable: true, value: originalActEnvironment },
});
globalThis.fetch = originalFetch;
});📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| }); | ||||||||||||||||||
| host = testWindow.document.createElement("div") as unknown as HTMLElement; | ||||||||||||||||||
| testWindow.document.body.appendChild(host as never); | ||||||||||||||||||
| root = null; | ||||||||||||||||||
| editor = null; | ||||||||||||||||||
| requests = []; | ||||||||||||||||||
| configRefreshes = 0; | ||||||||||||||||||
| quotaRefreshes = 0; | ||||||||||||||||||
| savedCallbacks = 0; | ||||||||||||||||||
| notifications = []; | ||||||||||||||||||
| responseFactory = async () => Response.json({ success: true }); | ||||||||||||||||||
| globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { | ||||||||||||||||||
| requests.push({ | ||||||||||||||||||
| url: String(input), | ||||||||||||||||||
| method: init?.method ?? "GET", | ||||||||||||||||||
| body: typeof init?.body === "string" ? JSON.parse(init.body) : init?.body, | ||||||||||||||||||
| }); | ||||||||||||||||||
| return responseFactory(); | ||||||||||||||||||
| }) as typeof fetch; | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| afterEach(async () => { | ||||||||||||||||||
| if (root) await act(async () => { root?.unmount(); }); | ||||||||||||||||||
| await testWindow.happyDOM?.close?.(); | ||||||||||||||||||
| Object.defineProperties(globalThis, { | ||||||||||||||||||
| document: { configurable: true, value: originalDocument }, | ||||||||||||||||||
| window: { configurable: true, value: originalWindow }, | ||||||||||||||||||
| navigator: { configurable: true, value: originalNavigator }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| globalThis.fetch = originalFetch; | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| test("Save sends one atomic provider PUT with baseline and next, then refreshes", async () => { | ||||||||||||||||||
| await mountHook(); | ||||||||||||||||||
| await act(async () => { editor!.openJsonEditor(); }); | ||||||||||||||||||
|
|
||||||||||||||||||
| const baseline = { | ||||||||||||||||||
| defaultProvider: "alpha", | ||||||||||||||||||
| providers: { | ||||||||||||||||||
| alpha: { | ||||||||||||||||||
| adapter: "openai-chat", | ||||||||||||||||||
| baseUrl: "https://alpha.example.test/v1", | ||||||||||||||||||
| defaultModel: "alpha-old", | ||||||||||||||||||
| modelContextWindows: { "alpha-old": 131_072 }, | ||||||||||||||||||
| modelReasoningEfforts: { "alpha-old": ["low", "high"] }, | ||||||||||||||||||
| noVisionModels: ["alpha-old"], | ||||||||||||||||||
| allowPrivateNetwork: true, | ||||||||||||||||||
| note: "derived registry note", | ||||||||||||||||||
| }, | ||||||||||||||||||
| beta: { | ||||||||||||||||||
| adapter: "anthropic", | ||||||||||||||||||
| baseUrl: "https://beta.example.test/v1", | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| }; | ||||||||||||||||||
| expect(JSON.parse(editor!.draft)).toEqual(baseline); | ||||||||||||||||||
|
|
||||||||||||||||||
| const next = structuredClone(baseline); | ||||||||||||||||||
| next.defaultProvider = "beta"; | ||||||||||||||||||
| next.providers.alpha.defaultModel = "alpha-new"; | ||||||||||||||||||
| await act(async () => { editor!.setDraft(JSON.stringify(next, null, 2)); }); | ||||||||||||||||||
|
|
||||||||||||||||||
| let saved = false; | ||||||||||||||||||
| await act(async () => { saved = await editor!.saveConfig(); }); | ||||||||||||||||||
|
|
||||||||||||||||||
| expect(saved).toBe(true); | ||||||||||||||||||
| expect(requests).toEqual([{ | ||||||||||||||||||
| url: "/editor/api/providers", | ||||||||||||||||||
| method: "PUT", | ||||||||||||||||||
| body: { baseline, next }, | ||||||||||||||||||
| }]); | ||||||||||||||||||
| expect(requests.some(request => request.url.endsWith("/api/config") && request.method === "PUT")).toBe(false); | ||||||||||||||||||
| expect(requests.some(request => ["POST", "PATCH", "DELETE"].includes(request.method))).toBe(false); | ||||||||||||||||||
| expect(configRefreshes).toBe(1); | ||||||||||||||||||
| expect(quotaRefreshes).toBe(1); | ||||||||||||||||||
| expect(savedCallbacks).toBe(1); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| test("parse failures stay distinct from server failures and failed saves do not refresh", async () => { | ||||||||||||||||||
| await mountHook(); | ||||||||||||||||||
| await act(async () => { editor!.openJsonEditor(); }); | ||||||||||||||||||
| await act(async () => { editor!.setDraft("{bad json"); }); | ||||||||||||||||||
|
|
||||||||||||||||||
| await act(async () => { expect(await editor!.saveConfig()).toBe(false); }); | ||||||||||||||||||
| expect(requests).toHaveLength(0); | ||||||||||||||||||
| expect(notifications.at(-1)).toEqual({ message: "prov.invalidJson", ok: false }); | ||||||||||||||||||
|
Comment on lines
+161
to
+163
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Assert that This block proves three things about a bad draft: That matters because the invalid-JSON path is new control flow with its own manual reset. If a later refactor moves or drops that single line, the Save control stays in its pending state permanently and the operator cannot retry without reloading the dashboard. No assertion in this file would fail. One line closes the gap. 🧪 Proposed test addition await act(async () => { expect(await editor!.saveConfig()).toBe(false); });
expect(requests).toHaveLength(0);
expect(notifications.at(-1)).toEqual({ message: "prov.invalidJson", ok: false });
+ // The early return bypasses the try/finally, so the reset is manual and needs its own guard.
+ expect(editor!.jsonSaving).toBe(false);Consider the same assertion after the 409 case at Line 167 and the network case at Line 171, so all three exits are pinned. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| await act(async () => { editor!.restoreJsonEditor(); }); | ||||||||||||||||||
| responseFactory = async () => Response.json({ error: "stale baseline" }, { status: 409 }); | ||||||||||||||||||
| await act(async () => { expect(await editor!.saveConfig()).toBe(false); }); | ||||||||||||||||||
|
|
||||||||||||||||||
| expect(notifications.at(-1)).toEqual({ message: "stale baseline", ok: false }); | ||||||||||||||||||
| responseFactory = async () => { throw new Error("network down"); }; | ||||||||||||||||||
| await act(async () => { expect(await editor!.saveConfig()).toBe(false); }); | ||||||||||||||||||
| expect(notifications.at(-1)).toEqual({ message: "prov.saveFailed", ok: false }); | ||||||||||||||||||
| expect(configRefreshes).toBe(0); | ||||||||||||||||||
| expect(quotaRefreshes).toBe(0); | ||||||||||||||||||
| expect(savedCallbacks).toBe(0); | ||||||||||||||||||
| }); | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The provider field policy is restated at three sites with no shared source of truth. This PR introduces one classification of provider fields into editable, derived, and redacted. That classification is then written out independently in the GUI hook, in the GUI test fixture, and in the server DTO tests. Nothing proves the three agree, so a field added to one classification and missed in another produces a 400 on save rather than a failing test. The fix at each site is to bind it to the shared policy instead of restating it.
gui/src/hooks/useJsonConfigEditor.ts#L9-L13: confirm whether the DTO projectsvirtualModels; if it does, add it here, and export the derived-field names from one shared module that both the server policy and this list consume.gui/tests/use-json-config-editor.test.tsx#L24-L26: extend the provider fixture to carry every field the derived policy names, includingxaiResponsesOptInStateandvirtualModels, and keep them out of the expected baseline at Lines 114-132.tests/server-auth.test.ts#L821-L821: pair the newmodelMaxInputTokensexposure assertion with a write-acceptance assertion, by adding the field to the unchanged-round-trip fixture intests/provider-config-batch-management.test.ts.📍 Affects 3 files
gui/src/hooks/useJsonConfigEditor.ts#L9-L13(this comment)gui/tests/use-json-config-editor.test.tsx#L24-L26tests/server-auth.test.ts#L821-L821🤖 Prompt for AI Agents