fix(dashboard): avoid lost update and surface failed param-filter saves (#8910)

The close-time save could clear the dirty flag for a payload snapshotted before the PUT resolved, silently discarding any keystroke that landed in that window. Track a monotonic draft revision and only acknowledge the revision that was actually written, re-running the save (bounded) otherwise. A failed save previously stayed dirty to 'retry on a later close', but reopening the popover reloaded server state and silently reverted the draft. Keep a dirty draft for the same provider/model on reopen and show a failure marker next to the saving indicator instead.
2026-08-12 02:02:13 +03:00 · 2026-07-30 03:43:52 +08:00
parent 152a3e13f7
commit 59d5f43d7b
2 changed files with 276 additions and 20 deletions
--- 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<string, string>, 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<string | null>(null);
  const [valueFocusRowId, setValueFocusRowId] = useState<string | null>(null);
  const ref = useRef<HTMLDivElement>(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<string | null>(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({
                  <p className="text-[10px] text-text-muted">
                    {t("compatBlockedParamsHint") ?? "Blocked params (stripped from requests)"}
                    {paramSaving && ` ● ${t("compatSaving")}`}
+                    {paramSaveFailed && !paramSaving && (
+                      <span
+                        role="alert"
+                        className="ml-1 font-medium text-red-600 dark:text-red-400"
+                        title={t("failedSaveConnectionRetry")}
+                      >
+                        ● {t("failed")}
+                      </span>
+                    )}
                  </p>
                </div>
                <div>
@@ -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")}
--- 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
@@ -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<string, { block: string[]; allow: string[] }>;
+  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(
+      <ModelCompatPopover
+        t={(key) => 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<void>((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: [] },
+    });
+  });
+});