diff --git a/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx b/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx index 3e8b4e9088..daafc791eb 100644 --- a/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx +++ b/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx @@ -26,6 +26,11 @@ function recordToHeaderRows(rec: Record, genId: () => string): H return entries.map(([name, value]) => ({ id: genId(), name, value })); } +// Bounded re-run budget for the model param-filter save: if the draft changed while the PUT was +// in flight, the save repeats with the newer draft instead of clearing the dirty flag on a +// payload that no longer matches what the user typed (#8910). +const PARAM_SAVE_MAX_ATTEMPTS = 3; + function parseCommaList(text: string): string[] { return text ? text @@ -112,6 +117,7 @@ export default function ModelCompatPopover({ const [blockText, setBlockText] = useState(""); const [allowText, setAllowText] = useState(""); const [paramSaving, setParamSaving] = useState(false); + const [paramSaveFailed, setParamSaveFailed] = useState(false); const [valuePeekRowId, setValuePeekRowId] = useState(null); const [valueFocusRowId, setValueFocusRowId] = useState(null); const ref = useRef(null); @@ -130,10 +136,21 @@ export default function ModelCompatPopover({ // latest typed values instead of the values captured when the handler was created (#8910). const paramDirtyRef = useRef(false); const paramSavingRef = useRef(false); + // Monotonic draft revision — bumped on every edit so an in-flight save can tell whether the + // draft it snapshotted is still the newest one (#8910 lost update). + const paramRevRef = useRef(0); + // Provider/model the currently displayed draft was loaded for, so a reopen of the same target + // does not clobber a draft that is still pending a (previously failed) save. + const paramLoadedKeyRef = useRef(null); const blockTextRef = useRef(""); const allowTextRef = useRef(""); blockTextRef.current = blockText; allowTextRef.current = allowText; + + const markParamDraftDirty = useCallback(() => { + paramDirtyRef.current = true; + paramRevRef.current += 1; + }, []); const mountedRef = useRef(true); useEffect(() => { mountedRef.current = true; @@ -184,6 +201,11 @@ export default function ModelCompatPopover({ // Load model-level block/allow from param-filters API useEffect(() => { if (!open) return; + const draftKey = `${providerId}\u0000${modelId}`; + // A draft that is still dirty for this exact provider/model was never persisted (failed or + // exhausted save). Reloading server state here would silently revert it — the very complaint + // behind #8910 — so keep the draft on screen and let the user retry instead. + if (paramDirtyRef.current && paramLoadedKeyRef.current === draftKey) return; let cancelled = false; (async () => { try { @@ -197,6 +219,8 @@ export default function ModelCompatPopover({ // Only the freshly loaded server state is clean — a failed load must not // discard drafts the user already typed (#8910). paramDirtyRef.current = false; + paramLoadedKeyRef.current = draftKey; + setParamSaveFailed(false); } catch { // Keep whatever the user has in the fields (and its dirty flag) on load failure. } @@ -212,25 +236,40 @@ export default function ModelCompatPopover({ paramSavingRef.current = true; if (mountedRef.current) setParamSaving(true); try { - const res = await fetch(`/api/providers/${providerId}/param-filters`); - if (!res.ok) throw new Error(`param-filters GET failed: ${res.status}`); - const current = await res.json(); - const payload = buildModelParamFilterPayload( - current, - modelId, - blockTextRef.current, - allowTextRef.current - ); - const putRes = await fetch(`/api/providers/${providerId}/param-filters`, { - method: "PUT", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(payload), - }); - if (!putRes.ok) throw new Error(`param-filters PUT failed: ${putRes.status}`); - // Stay dirty unless the write actually succeeded, so a later close can retry. - paramDirtyRef.current = false; + // Re-run while the draft changed under the in-flight write: the payload is snapshotted + // before the PUT resolves, so a keystroke landing in that window would otherwise be + // acknowledged (dirty cleared) but never persisted — the #8910 lost update. + for (let attempt = 0; attempt < PARAM_SAVE_MAX_ATTEMPTS; attempt += 1) { + const rev = paramRevRef.current; + const res = await fetch(`/api/providers/${providerId}/param-filters`); + if (!res.ok) throw new Error(`param-filters GET failed: ${res.status}`); + const current = await res.json(); + const payload = buildModelParamFilterPayload( + current, + modelId, + blockTextRef.current, + allowTextRef.current + ); + const putRes = await fetch(`/api/providers/${providerId}/param-filters`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(payload), + }); + if (!putRes.ok) throw new Error(`param-filters PUT failed: ${putRes.status}`); + // Only the revision that was actually written may clear the dirty flag. + if (paramRevRef.current === rev) { + paramDirtyRef.current = false; + if (mountedRef.current) setParamSaveFailed(false); + return; + } + } + // Budget exhausted (the user is still typing): stay dirty and flag it, so the draft is + // kept on reopen and the next blur/close retries it. + if (mountedRef.current) setParamSaveFailed(true); } catch { - // Save failed — drafts and dirty state are intentionally preserved. + // Save failed — drafts and dirty state are intentionally preserved, and the failure is + // surfaced in the panel instead of being silently swallowed. + if (mountedRef.current) setParamSaveFailed(true); } finally { paramSavingRef.current = false; if (mountedRef.current) setParamSaving(false); @@ -405,7 +444,7 @@ export default function ModelCompatPopover({ onChange={(e) => { setBlockText(e.target.value); blockTextRef.current = e.target.value; - paramDirtyRef.current = true; + markParamDraftDirty(); }} onBlur={() => saveModelParamFilters()} placeholder={t("compatBlockedParamsPlaceholder")} @@ -415,6 +454,15 @@ export default function ModelCompatPopover({

{t("compatBlockedParamsHint") ?? "Blocked params (stripped from requests)"} {paramSaving && ` ● ${t("compatSaving")}`} + {paramSaveFailed && !paramSaving && ( + + ● {t("failed")} + + )}

@@ -424,7 +472,7 @@ export default function ModelCompatPopover({ onChange={(e) => { setAllowText(e.target.value); allowTextRef.current = e.target.value; - paramDirtyRef.current = true; + markParamDraftDirty(); }} onBlur={() => saveModelParamFilters()} placeholder={t("compatAllowedParamsPlaceholder")} diff --git a/src/app/(dashboard)/dashboard/providers/[id]/components/__tests__/modelCompatPopover-param-filter-concurrency.test.tsx b/src/app/(dashboard)/dashboard/providers/[id]/components/__tests__/modelCompatPopover-param-filter-concurrency.test.tsx new file mode 100644 index 0000000000..713c374895 --- /dev/null +++ b/src/app/(dashboard)/dashboard/providers/[id]/components/__tests__/modelCompatPopover-param-filter-concurrency.test.tsx @@ -0,0 +1,208 @@ +// @vitest-environment jsdom +// Regression coverage for the concurrency defects found while fixing #8910: +// 1. an edit landing after the PUT payload snapshot but before the PUT resolves must still +// be persisted (lost update); +// 2. a save that failed must not be silently reverted on reopen, and the failure must be +// visible in the panel. +import React, { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import ModelCompatPopover from "../ModelCompatPopover"; + +vi.mock("next-intl", () => ({ + useTranslations: () => (key: string) => key, +})); + +interface ParamFilterState { + block: string[]; + allow: string[]; + models?: Record; + autoLearn: boolean; +} + +let container: HTMLDivElement; +let root: Root; + +function setInputValue(input: HTMLInputElement, value: string) { + const setter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, "value")?.set; + setter?.call(input, value); + input.dispatchEvent(new Event("input", { bubbles: true })); +} + +async function flushEffects() { + await act(async () => { + for (let i = 0; i < 8; i += 1) await Promise.resolve(); + }); +} + +function blockInput() { + return document.querySelector( + 'input[placeholder="compatBlockedParamsPlaceholder"]' + ) as HTMLInputElement | null; +} + +function renderPopover() { + act(() => { + root.render( + key} + providerId="openai" + modelId="gpt-test" + effectiveModelNormalize={() => false} + effectiveModelPreserveDeveloper={() => true} + getUpstreamHeadersRecord={() => ({})} + onCompatPatch={vi.fn()} + /> + ); + }); +} + +async function openPopover() { + const trigger = container.querySelector("button") as HTMLButtonElement; + await act(async () => trigger.click()); + await flushEffects(); +} + +async function closePopoverByOutsideClick() { + await act(async () => { + document.body.dispatchEvent(new MouseEvent("mousedown", { bubbles: true })); + }); + await flushEffects(); +} + +describe("ModelCompatPopover param-filter save concurrency (#8910)", () => { + beforeEach(() => { + ( + globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT?: boolean } + ).IS_REACT_ACT_ENVIRONMENT = true; + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + document.body.innerHTML = ""; + vi.unstubAllGlobals(); + }); + + it("persists an edit typed after the payload snapshot but before the PUT resolves", async () => { + let serverState: ParamFilterState = { block: [], allow: [], models: {}, autoLearn: false }; + let releasePut: (() => void) | null = null; + let holdPut = false; + + vi.stubGlobal( + "fetch", + vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === "PUT") { + serverState = JSON.parse(String(init.body)) as ParamFilterState; + if (holdPut) { + await new Promise((resolve) => { + releasePut = resolve; + }); + } + return { ok: true, json: async () => ({ success: true }) } as Response; + } + return { ok: true, json: async () => structuredClone(serverState) } as Response; + }) + ); + + renderPopover(); + await openPopover(); + + await act(async () => setInputValue(blockInput()!, "temperature")); + holdPut = true; + // Blur starts the save: GET resolves, payload is snapshotted, PUT is issued and held open. + await act(async () => { + blockInput()!.dispatchEvent(new FocusEvent("focusout", { bubbles: true })); + }); + await flushEffects(); + expect(releasePut).not.toBeNull(); + + // The user keeps typing while that PUT is still in flight, then closes the popover. + await act(async () => setInputValue(blockInput()!, "temperature, seed")); + await closePopoverByOutsideClick(); + + holdPut = false; + await act(async () => { + releasePut?.(); + await Promise.resolve(); + }); + await flushEffects(); + + expect(serverState.models).toEqual({ + "gpt-test": { block: ["temperature", "seed"], allow: [] }, + }); + }); + + it("keeps the draft and surfaces the failure when the save fails, instead of reverting on reopen", async () => { + const serverState: ParamFilterState = { block: [], allow: [], models: {}, autoLearn: false }; + let putAttempts = 0; + + vi.stubGlobal( + "fetch", + vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === "PUT") { + putAttempts += 1; + return { ok: false, status: 500, json: async () => ({}) } as Response; + } + return { ok: true, json: async () => structuredClone(serverState) } as Response; + }) + ); + + renderPopover(); + await openPopover(); + + await act(async () => setInputValue(blockInput()!, "temperature")); + await closePopoverByOutsideClick(); + expect(putAttempts).toBe(1); + + // Reopening must NOT clobber the unsaved draft with server state (#8910 complaint class). + await openPopover(); + expect(blockInput()!.value).toBe("temperature"); + // The failure is visible to the user rather than silently swallowed. + expect(document.querySelector('[role="alert"]')?.textContent).toContain("failed"); + + // ...and the retained draft is actually retryable through the normal close path. + await closePopoverByOutsideClick(); + expect(putAttempts).toBe(2); + }); + + it("clears the failure indicator once a later save succeeds", async () => { + let serverState: ParamFilterState = { block: [], allow: [], models: {}, autoLearn: false }; + let failNextPut = true; + + vi.stubGlobal( + "fetch", + vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === "PUT") { + if (failNextPut) return { ok: false, status: 500, json: async () => ({}) } as Response; + serverState = JSON.parse(String(init.body)) as ParamFilterState; + return { ok: true, json: async () => ({ success: true }) } as Response; + } + return { ok: true, json: async () => structuredClone(serverState) } as Response; + }) + ); + + renderPopover(); + await openPopover(); + await act(async () => setInputValue(blockInput()!, "temperature")); + await act(async () => { + blockInput()!.dispatchEvent(new FocusEvent("focusout", { bubbles: true })); + }); + await flushEffects(); + expect(document.querySelector('[role="alert"]')).not.toBeNull(); + + failNextPut = false; + await act(async () => setInputValue(blockInput()!, "temperature, seed")); + await act(async () => { + blockInput()!.dispatchEvent(new FocusEvent("focusout", { bubbles: true })); + }); + await flushEffects(); + + expect(document.querySelector('[role="alert"]')).toBeNull(); + expect(serverState.models).toEqual({ + "gpt-test": { block: ["temperature", "seed"], allow: [] }, + }); + }); +});