diff --git a/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx b/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx index a293b24f06..4454f0c9e3 100644 --- a/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx +++ b/src/app/(dashboard)/dashboard/providers/[id]/components/ModelCompatPopover.tsx @@ -58,6 +58,10 @@ interface ParamFilterDraft { allow: string; } +function paramTargetKeyOf(providerId: string, modelId: string): string { + return `${providerId}\u0000${modelId}`; +} + // Builds the PUT body for the model-level block/allow save. Extracted so the // caller's async handler stays simple — this is pure payload-shaping logic. function buildModelParamFilterPayload( @@ -146,13 +150,17 @@ export default function ModelCompatPopover({ // Param-filter drafts are mirrored into a ref so the close/unmount save path reads the // latest typed values instead of the values captured when the handler was created (#8910). const paramSavingRef = useRef(false); - // The single unsaved draft, together with the provider/model it was typed for. Non-null means - // "dirty". Recorded when the draft is edited — never derived from a completed load — so an - // unsaved draft is protected even when no load ever succeeded for that target, and every write - // lands on the draft's own target rather than whatever the popover currently points at (#8910). - const paramDraftRef = useRef(null); + // Every unsaved draft, keyed by the provider/model it was typed for. A per-target map (rather + // than a single slot) is required because a live popover can be re-pointed at another target + // while a draft is still unsaved: with one slot the next keystroke on the new target destroyed + // the previous target's unsaved work, and the new target's successful save then cleared the + // failure indicator — a green UI over data that was never written, i.e. exactly the silent + // data loss reported in #8910. Drafts are recorded when edited — never derived from a completed + // load — so an unsaved draft survives even when no load ever succeeded for that target, and + // every write lands on the draft's own target rather than whatever the popover now points at. + const paramDraftsRef = useRef>(new Map()); - const paramTargetKey = `${providerId}\u0000${modelId}`; + const paramTargetKey = paramTargetKeyOf(providerId, modelId); const paramTargetRef = useRef<{ key: string; providerId: string; modelId: string }>({ key: paramTargetKey, providerId, @@ -165,19 +173,44 @@ export default function ModelCompatPopover({ const allowTextRef = useRef(""); blockTextRef.current = blockText; allowTextRef.current = allowText; + // Which target the values currently in the fields belong to. Guards the invariant that + // blockTextRef/allowTextRef never hold content belonging to a target other than the one being + // displayed — the desync that let one model's server values be saved under another (#8910). + const fieldsTargetKeyRef = useRef(null); - // Every edit replaces the draft with a fresh object bound to the CURRENT target. Object - // identity doubles as the draft revision an in-flight save compares against (#8910). - const markParamDraftDirty = useCallback(() => { - const target = paramTargetRef.current; - paramDraftRef.current = { - key: target.key, - providerId: target.providerId, - modelId: target.modelId, - block: blockTextRef.current, - allow: allowTextRef.current, - }; + const applyParamFields = useCallback((targetKey: string, block: string, allow: string) => { + fieldsTargetKeyRef.current = targetKey; + blockTextRef.current = block; + allowTextRef.current = allow; + setBlockText(block); + setAllowText(allow); }, []); + + // Every edit rewrites the draft for the CURRENT target. The counterpart field is only trusted + // when the values on screen belong to this target; otherwise it is taken from this target's own + // pending draft (or empty), so another target's value can never be captured into this draft and + // then persisted here (#8910). Object identity doubles as the draft revision an in-flight save + // compares against. + const editParamDraft = useCallback( + (field: "block" | "allow", value: string) => { + const target = paramTargetRef.current; + const fieldsOwned = fieldsTargetKeyRef.current === target.key; + const pending = paramDraftsRef.current.get(target.key); + const block = + field === "block" ? value : fieldsOwned ? blockTextRef.current : (pending?.block ?? ""); + const allow = + field === "allow" ? value : fieldsOwned ? allowTextRef.current : (pending?.allow ?? ""); + applyParamFields(target.key, block, allow); + paramDraftsRef.current.set(target.key, { + key: target.key, + providerId: target.providerId, + modelId: target.modelId, + block, + allow, + }); + }, + [applyParamFields] + ); const mountedRef = useRef(true); useEffect(() => { mountedRef.current = true; @@ -228,12 +261,20 @@ 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. - const draftIsDirtyForThisTarget = () => paramDraftRef.current?.key === draftKey; - if (draftIsDirtyForThisTarget()) return; + const draftKey = paramTargetKeyOf(providerId, modelId); + const draftForThisTarget = () => paramDraftsRef.current.get(draftKey); + // The fields must always show THIS target's content, and nothing else may ever be read back + // out of them. A draft that is still pending for this exact provider/model was never persisted + // (failed or exhausted save): restore it into the inputs instead of loading server state over + // it, because reloading here would silently revert it — the very complaint behind #8910. + const pending = draftForThisTarget(); + if (pending) { + applyParamFields(draftKey, pending.block, pending.allow); + return; + } + // No draft for this target: drop whatever the previously displayed target left on screen so + // the fields can never present (or contribute) another target's values. + if (fieldsTargetKeyRef.current !== draftKey) applyParamFields(draftKey, "", ""); let cancelled = false; (async () => { try { @@ -243,14 +284,17 @@ export default function ModelCompatPopover({ if (cancelled || !mountedRef.current) return; // Re-check after the await: the user may have typed while the GET was in flight, and a // load result must never overwrite (or acknowledge) a draft that is not on the server. - if (draftIsDirtyForThisTarget()) return; + if (draftForThisTarget()) return; const modelCfg = data?.models?.[modelId]; - setBlockText(modelCfg ? (modelCfg.block ?? []).join(", ") : ""); - setAllowText(modelCfg ? (modelCfg.allow ?? []).join(", ") : ""); - // A load never clears the draft: any draft still pending here belongs to a DIFFERENT + applyParamFields( + draftKey, + modelCfg ? (modelCfg.block ?? []).join(", ") : "", + modelCfg ? (modelCfg.allow ?? []).join(", ") : "" + ); + // A load never clears a draft: any draft still pending here belongs to a DIFFERENT // target and is still owed a write to that target (#8910). The failure indicator is // only cleared once nothing is left unsaved anywhere. - if (!paramDraftRef.current) setParamSaveFailed(false); + if (paramDraftsRef.current.size === 0) setParamSaveFailed(false); } catch { // Keep whatever the user has in the fields (and its dirty flag) on load failure. } @@ -259,57 +303,68 @@ export default function ModelCompatPopover({ cancelled = true; }; // Reload only when opening or when the popover targets a different provider/model. - }, [open, providerId, modelId]); + }, [open, providerId, modelId, applyParamFields]); - // The save always writes through the draft's OWN provider/model — never the props this - // callback happens to be bound to — so a draft typed for target A can never be persisted under - // a target B the user never edited (#8910). The draft is re-read after every await for the same - // reason the load effect re-checks its guard: the target can change while the save is in flight. + // Drains EVERY pending draft, each written through its OWN provider/model — never the props this + // callback happens to be bound to — so a draft typed for target A can never be persisted under a + // target B the user never edited, and re-pointing the popover cannot destroy target A's unsaved + // work (#8910). Each draft is re-read after every await for the same reason the load effect + // re-checks its guard: the user can keep typing while a write is in flight. const saveModelParamFilters = useCallback(async () => { - if (!paramDraftRef.current || paramSavingRef.current) return; + if (paramDraftsRef.current.size === 0 || paramSavingRef.current) return; paramSavingRef.current = true; if (mountedRef.current) setParamSaving(true); - try { + + // Returns true once nothing is owed for this target any more. + const saveDraftForTarget = async (key: string): Promise => { // 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. + // acknowledged (draft dropped) but never persisted — the #8910 lost update. for (let attempt = 0; attempt < PARAM_SAVE_MAX_ATTEMPTS; attempt += 1) { - const draft = paramDraftRef.current; - if (!draft) return; - const res = await fetch(`/api/providers/${draft.providerId}/param-filters`); - if (!res.ok) throw new Error(`param-filters GET failed: ${res.status}`); - const current = await res.json(); - // The fetched config belongs to draft.providerId; if the draft was replaced by one for - // another provider/model while the GET was in flight, restart with a matching GET. - if (paramDraftRef.current?.key !== draft.key) continue; - const payload = buildModelParamFilterPayload( - current, - draft.modelId, - draft.block, - draft.allow - ); - const putRes = await fetch(`/api/providers/${draft.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 exact draft that was written may clear the dirty flag. - if (paramDraftRef.current === draft) { - paramDraftRef.current = null; - if (mountedRef.current) setParamSaveFailed(false); - return; + const draft = paramDraftsRef.current.get(key); + if (!draft) return true; + try { + const res = await fetch(`/api/providers/${draft.providerId}/param-filters`); + if (!res.ok) throw new Error(`param-filters GET failed: ${res.status}`); + const current = await res.json(); + // The fetched config belongs to draft.providerId; if the draft was replaced by a newer + // one for the same target while the GET was in flight, restart with a fresh read. + if (paramDraftsRef.current.get(key) !== draft) continue; + const payload = buildModelParamFilterPayload( + current, + draft.modelId, + draft.block, + draft.allow + ); + const putRes = await fetch(`/api/providers/${draft.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 exact draft that was written may be discarded. + if (paramDraftsRef.current.get(key) === draft) { + paramDraftsRef.current.delete(key); + return true; + } + } catch { + // Save failed — the draft (and the target it belongs to) is intentionally preserved so + // the next blur/close/unmount save retries it against its own provider/model. + return false; } } - // 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 — the draft (and the target it belongs to) is intentionally preserved, and - // the failure is surfaced in the panel instead of being silently swallowed. An orphaned - // draft whose target is no longer displayed is neither dropped nor redirected: it keeps its - // own provider/model and is retried by the next blur/close/unmount save. - if (mountedRef.current) setParamSaveFailed(true); + // Budget exhausted (the user is still typing): stay dirty for this target. + return false; + }; + + try { + const failures: string[] = []; + for (const key of Array.from(paramDraftsRef.current.keys())) { + if (!(await saveDraftForTarget(key))) failures.push(key); + } + // The indicator tracks unsaved work across ALL targets: a successful write for the target + // now on screen must not signal "saved" while another target's draft is still owed a write. + if (mountedRef.current) setParamSaveFailed(failures.length > 0); } finally { paramSavingRef.current = false; if (mountedRef.current) setParamSaving(false); @@ -482,11 +537,7 @@ export default function ModelCompatPopover({ { - setBlockText(e.target.value); - blockTextRef.current = e.target.value; - markParamDraftDirty(); - }} + onChange={(e) => editParamDraft("block", e.target.value)} onBlur={() => saveModelParamFilters()} placeholder={t("compatBlockedParamsPlaceholder")} disabled={disabled} @@ -510,11 +561,7 @@ export default function ModelCompatPopover({ { - setAllowText(e.target.value); - allowTextRef.current = e.target.value; - markParamDraftDirty(); - }} + onChange={(e) => editParamDraft("allow", e.target.value)} onBlur={() => saveModelParamFilters()} placeholder={t("compatAllowedParamsPlaceholder")} disabled={disabled} diff --git a/src/app/(dashboard)/dashboard/providers/[id]/components/__tests__/modelCompatPopover-param-filter-target-repoint.test.tsx b/src/app/(dashboard)/dashboard/providers/[id]/components/__tests__/modelCompatPopover-param-filter-target-repoint.test.tsx new file mode 100644 index 0000000000..e6db451f3d --- /dev/null +++ b/src/app/(dashboard)/dashboard/providers/[id]/components/__tests__/modelCompatPopover-param-filter-target-repoint.test.tsx @@ -0,0 +1,261 @@ +// @vitest-environment jsdom +// Regression coverage for the two target-re-point defects found while fixing #8910. +// +// A ModelCompatPopover instance is not always keyed by a stable identity, so a re-render can +// re-point a LIVE, still-open popover at a different provider/model while a draft for the previous +// target is unsaved. Two failures followed from that: +// +// 1. (C10) The inputs are driven by blockText/allowText, which used to be written only by the +// load effect — and that effect early-returned whenever a draft was dirty. Re-pointing +// A -> B -> A therefore left B's server values on screen under A, and the next keystroke +// snapshotted them into A's draft, persisting B's content into A's entry. +// 2. (C11) The pending draft lived in a single slot that every edit overwrote, so typing into +// the new target destroyed the previous target's unsaved work, and the new target's +// successful save cleared the failure indicator — a green UI over data never written. +// +// Contract asserted here: the fields always show the displayed target's own content (its pending +// draft when it has one), never another target's; and every target's unsaved draft survives until +// it is actually persisted to its own provider/model. +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(rounds = 40) { + await act(async () => { + for (let i = 0; i < rounds; i += 1) await Promise.resolve(); + }); +} + +function blockInput() { + return document.querySelector( + 'input[placeholder="compatBlockedParamsPlaceholder"]' + ) as HTMLInputElement | null; +} + +function renderPopover(modelId: string) { + act(() => { + root.render( + key} + providerId="openai" + modelId={modelId} + effectiveModelNormalize={() => false} + effectiveModelPreserveDeveloper={() => true} + getUpstreamHeadersRecord={() => ({})} + onCompatPatch={vi.fn()} + /> + ); + }); +} + +// React 19 delegates onBlur through focusout — a bare "blur" event does not reach the handler. +async function blurBlockInput() { + await act(async () => { + blockInput()!.dispatchEvent(new FocusEvent("focusout", { bubbles: true })); + }); + await flushEffects(); +} + +describe("ModelCompatPopover param-filter target re-point (#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(() => { + try { + act(() => root.unmount()); + } catch { + // already unmounted by the test + } + document.body.innerHTML = ""; + vi.unstubAllGlobals(); + }); + + it("restores the dirty draft into the fields on return, instead of showing the other model's values", async () => { + const server: ParamFilterState = { + block: [], + allow: [], + models: { "model-b": { block: ["secret-b"], allow: [] } }, + autoLearn: false, + }; + let putFails = true; + + vi.stubGlobal( + "fetch", + vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === "PUT") { + if (putFails) return { ok: false, status: 500, json: async () => ({}) } as Response; + const body = JSON.parse(String(init.body)); + server.models = body.models ?? {}; + return { ok: true, json: async () => ({ success: true }) } as Response; + } + return { ok: true, json: async () => structuredClone(server) } as Response; + }) + ); + + renderPopover("model-a"); + const trigger = container.querySelector("button") as HTMLButtonElement; + await act(async () => trigger.click()); + await flushEffects(); + + // model-a has no server entry, so the field loads empty; the user types a draft whose save fails. + await act(async () => setInputValue(blockInput()!, "aaa")); + await blurBlockInput(); + expect(blockInput()!.value).toBe("aaa"); + + // The live popover is re-pointed at model-b, which does have a server value... + renderPopover("model-b"); + await flushEffects(); + expect(blockInput()!.value).toBe("secret-b"); + + // ...and back to model-a, whose draft is still unsaved: the field must show model-a's draft. + renderPopover("model-a"); + await flushEffects(); + expect(blockInput()!.value).toBe("aaa"); + + // The user appends to what they can see and closes; the write must stay inside model-a. + putFails = false; + await act(async () => setInputValue(blockInput()!, `${blockInput()!.value}, extra`)); + await act(async () => { + document.body.dispatchEvent(new MouseEvent("mousedown", { bubbles: true })); + }); + await flushEffects(); + + expect(server.models["model-a"]).toEqual({ block: ["aaa", "extra"], allow: [] }); + expect(JSON.stringify(server.models["model-a"])).not.toContain("secret-b"); + expect(server.models["model-b"]).toEqual({ block: ["secret-b"], allow: [] }); + }); + + it("keeps one model's unsaved draft alive while the user edits another model", async () => { + const server: ParamFilterState = { block: [], allow: [], models: {}, autoLearn: false }; + let putFails = true; + + vi.stubGlobal( + "fetch", + vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === "PUT") { + if (putFails) return { ok: false, status: 500, json: async () => ({}) } as Response; + const body = JSON.parse(String(init.body)); + server.models = body.models ?? {}; + return { ok: true, json: async () => ({ success: true }) } as Response; + } + return { ok: true, json: async () => structuredClone(server) } as Response; + }) + ); + + renderPopover("model-a"); + const trigger = container.querySelector("button") as HTMLButtonElement; + await act(async () => trigger.click()); + await flushEffects(); + + await act(async () => setInputValue(blockInput()!, "aaa")); + await blurBlockInput(); + expect(document.querySelector('[role="alert"]')?.textContent).toContain("failed"); + + // Re-pointed at model-b; the network recovers and the user edits model-b. + renderPopover("model-b"); + await flushEffects(); + putFails = false; + await act(async () => setInputValue(blockInput()!, "bbb")); + await blurBlockInput(); + + // model-a's draft must not have been destroyed by the model-b edit: both are persisted... + expect(server.models["model-a"]).toEqual({ block: ["aaa"], allow: [] }); + expect(server.models["model-b"]).toEqual({ block: ["bbb"], allow: [] }); + // ...and with nothing left unsaved anywhere the failure indicator is finally cleared. + expect(document.querySelector('[role="alert"]')).toBeNull(); + }); + + it("does not report success while another target's draft is still unsaved", async () => { + const byProvider: Record = { + alpha: { block: [], allow: [], models: {}, autoLearn: false }, + beta: { block: [], allow: [], models: {}, autoLearn: false }, + }; + let failing = new Set(["alpha"]); + + vi.stubGlobal( + "fetch", + vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const providerId = url.match(/providers\/([^/]+)\//)![1]; + if (init?.method === "PUT") { + if (failing.has(providerId)) { + return { ok: false, status: 503, json: async () => ({}) } as Response; + } + const body = JSON.parse(String(init.body)); + byProvider[providerId].models = body.models ?? {}; + return { ok: true, json: async () => ({ success: true }) } as Response; + } + return { ok: true, json: async () => structuredClone(byProvider[providerId]) } as Response; + }) + ); + + const renderFor = (providerId: string) => { + act(() => { + root.render( + key} + providerId={providerId} + modelId="gpt-test" + effectiveModelNormalize={() => false} + effectiveModelPreserveDeveloper={() => true} + getUpstreamHeadersRecord={() => ({})} + onCompatPatch={vi.fn()} + /> + ); + }); + }; + + renderFor("alpha"); + const trigger = container.querySelector("button") as HTMLButtonElement; + await act(async () => trigger.click()); + await flushEffects(); + await act(async () => setInputValue(blockInput()!, "alpha-draft")); + await blurBlockInput(); + expect(document.querySelector('[role="alert"]')?.textContent).toContain("failed"); + + // beta saves fine, but alpha is still broken: the indicator must stay up. + renderFor("beta"); + await flushEffects(); + await act(async () => setInputValue(blockInput()!, "beta-draft")); + await blurBlockInput(); + + expect(byProvider.beta.models).toEqual({ "gpt-test": { block: ["beta-draft"], allow: [] } }); + expect(byProvider.alpha.models).toEqual({}); + expect(document.querySelector('[role="alert"]')?.textContent).toContain("failed"); + + // Once alpha recovers, the preserved draft is written to alpha and the indicator clears. + failing = new Set(); + await act(async () => { + document.body.dispatchEvent(new MouseEvent("mousedown", { bubbles: true })); + }); + await flushEffects(); + expect(byProvider.alpha.models).toEqual({ "gpt-test": { block: ["alpha-draft"], allow: [] } }); + }); +});