diff --git a/ui/src/components/environment-variables-editor/EnvironmentVariablesEditor.test.tsx b/ui/src/components/environment-variables-editor/EnvironmentVariablesEditor.test.tsx index 9baa43f34f1..5f367ddf5c8 100644 --- a/ui/src/components/environment-variables-editor/EnvironmentVariablesEditor.test.tsx +++ b/ui/src/components/environment-variables-editor/EnvironmentVariablesEditor.test.tsx @@ -330,6 +330,95 @@ describe("EnvironmentVariablesEditor", () => { expect(revert.className).toContain("h-9"); }); + it("lists which variables changed in the unsaved-changes banner", async () => { + render( + {}} + onCreateSecret={async () => secrets[0]} + />, + ); + const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, "value")!.set!; + + // Edit FOO's value. + const valueInput = container.querySelector('input[aria-label="Variable value"]')!; + setter.call(valueInput, "new"); + valueInput.dispatchEvent(new Event("input", { bubbles: true })); + await flush(); + expect(container.textContent).toContain("Edited: FOO"); + + // Remove BAR. + const removeButtons = [...container.querySelectorAll('button[aria-label^="Remove"]')]; + removeButtons.at(-1)!.click(); + await flush(); + expect(container.textContent).toContain("Removed: BAR"); + + // Add a brand-new variable. + const addButton = [...container.querySelectorAll("button")].find((b) => b.textContent?.includes("Add variable"))!; + addButton.click(); + await flush(); + const newNameInput = nameInputs().at(-1)!; + setter.call(newNameInput, "API_TOKEN"); + newNameInput.dispatchEvent(new Event("input", { bubbles: true })); + await flush(); + expect(container.textContent).toContain("New: API_TOKEN"); + expect(container.textContent).toContain("Edited: FOO"); + expect(container.textContent).toContain("Removed: BAR"); + }); + + it("does not show a phantom unsaved-changes banner for a saved value that round-trips lossily", () => { + // Incomplete refs and unpadded names are dropped by the editor's emit + // semantics — the committed baseline must drop them too, or the banner + // shows the moment the form opens with no user edits. + render( + {}} + onCreateSecret={async () => secrets[0]} + />, + ); + expect(container.textContent).not.toContain("Unsaved changes"); + }); + + it("warns via beforeunload only while the draft is dirty", async () => { + render( + {}} + onCreateSecret={async () => secrets[0]} + />, + ); + + function dispatchBeforeUnload(): boolean { + const event = new Event("beforeunload", { cancelable: true }); + window.dispatchEvent(event); + return event.defaultPrevented; + } + + expect(dispatchBeforeUnload()).toBe(false); + + const valueInput = container.querySelector('input[aria-label="Variable value"]')!; + const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, "value")!.set!; + setter.call(valueInput, "bar"); + valueInput.dispatchEvent(new Event("input", { bubbles: true })); + await flush(); + expect(dispatchBeforeUnload()).toBe(true); + + const revert = [...container.querySelectorAll("button")].find( + (button) => button.textContent?.trim() === "Revert", + )!; + revert.click(); + await flush(); + expect(dispatchBeforeUnload()).toBe(false); + }); + it("marks a newly typed variable name as unsaved before saving", async () => { render( {}} onCreateSecret={async () => secrets[0]} />); const addButton = [...container.querySelectorAll("button")].find((b) => b.textContent?.includes("Add variable"))!; diff --git a/ui/src/components/environment-variables-editor/index.tsx b/ui/src/components/environment-variables-editor/index.tsx index 539f4ba46a0..5bed51534ec 100644 --- a/ui/src/components/environment-variables-editor/index.tsx +++ b/ui/src/components/environment-variables-editor/index.tsx @@ -34,44 +34,57 @@ const DEFAULT_RESERVED_PREFIXES = ["PAPERCLIP_"]; const DEFAULT_HINT = "Set the KEY to the env var name the process expects, for example GH_TOKEN. Choose a secret to resolve a stored value at run start. PAPERCLIP_* variables are injected automatically."; +// Canonical entries for dirty comparison. Must mirror the emit semantics of +// valueFromRows (trimmed names, incomplete refs dropped, last-writer-wins on +// trimmed duplicates) — otherwise a saved value that round-trips lossily shows +// a phantom "Unsaved changes" banner the moment the form opens. +function normalizedEnvEntries( + value: Record | null | undefined, +): Array<[string, Record]> { + if (!value || typeof value !== "object") return []; + const byName = new Map>(); + for (const [rawName, binding] of Object.entries(value)) { + const name = rawName.trim(); + if (!name) continue; + if (typeof binding === "string") { + byName.set(name, { type: "plain", value: binding }); + } else if (binding?.type === "secret_ref") { + const secretId = typeof binding.secretId === "string" ? binding.secretId : ""; + if (!secretId) continue; // incomplete ref — never emitted by the editor + byName.set(name, { + type: "secret_ref", + secretId, + version: typeof binding.version === "number" ? binding.version : "latest", + }); + } else if (binding?.type === "user_secret_ref") { + const key = typeof binding.key === "string" ? binding.key.trim() : ""; + if (!key) continue; // incomplete ref — never emitted by the editor + byName.set(name, { + type: "user_secret_ref", + key, + version: typeof binding.version === "number" ? binding.version : "latest", + required: binding.required !== false, + }); + } else if (binding?.type === "plain") { + byName.set(name, { type: "plain", value: typeof binding.value === "string" ? binding.value : "" }); + } else { + byName.set(name, { type: "plain", value: "" }); + } + } + return [...byName.entries()].sort(([left], [right]) => left.localeCompare(right)); +} + function normalizedEnvKey(value: Record | null | undefined): string { - if (!value || typeof value !== "object") return ""; - const entries = Object.entries(value) - .map(([name, binding]) => { - if (typeof binding === "string") { - return [name, { type: "plain", value: binding }] as const; - } - if (binding?.type === "secret_ref") { - return [ - name, - { - type: "secret_ref", - secretId: typeof binding.secretId === "string" ? binding.secretId : "", - version: typeof binding.version === "number" ? binding.version : "latest", - }, - ] as const; - } - if (binding?.type === "user_secret_ref") { - return [ - name, - { - type: "user_secret_ref", - key: typeof binding.key === "string" ? binding.key : "", - version: typeof binding.version === "number" ? binding.version : "latest", - required: binding.required !== false, - }, - ] as const; - } - if (binding?.type === "plain") { - return [ - name, - { type: "plain", value: typeof binding.value === "string" ? binding.value : "" }, - ] as const; - } - return [name, { type: "plain", value: "" }] as const; - }) - .sort(([left], [right]) => left.localeCompare(right)); - return JSON.stringify(entries); + return JSON.stringify(normalizedEnvEntries(value)); +} + +const CHANGE_SUMMARY_MAX_NAMES = 3; + +function formatChangedNames(names: readonly string[]): string { + const shown = names.slice(0, CHANGE_SUMMARY_MAX_NAMES).join(", "); + return names.length > CHANGE_SUMMARY_MAX_NAMES + ? `${shown} +${names.length - CHANGE_SUMMARY_MAX_NAMES} more` + : shown; } function cloneRows(rows: readonly EnvRow[]): EnvRow[] { @@ -120,6 +133,8 @@ export interface EnvironmentVariablesEditorProps { reservedPrefixes?: readonly string[]; /** Context-specific hint line. `null` hides the default copy; omit for default. */ footerHint?: ReactNode | null; + /** Reports editor-local draft changes that are not yet promoted to the parent value. */ + onDirtyChange?: (dirty: boolean) => void; } export interface EnvironmentVariablesEditorHandle { @@ -140,6 +155,7 @@ export const EnvironmentVariablesEditor = forwardRef(null); @@ -219,6 +235,50 @@ export const EnvironmentVariablesEditor = forwardRef normalizedEnvKey(draftValue), [draftValue]); const hasUnsavedChanges = draftValueKey !== committedValueKey; + useEffect(() => { + onDirtyChange?.(!disabled && hasUnsavedChanges); + }, [disabled, hasUnsavedChanges, onDirtyChange]); + + // Which variables differ from the committed baseline, so the unsaved-changes + // banner can say *what* is unsaved instead of a bare label. A rename shows + // as one addition plus one removal. + const changeSummary = useMemo(() => { + const committed = new Map( + normalizedEnvEntries(valueFromRows(committedRows)).map(([name, binding]) => [name, JSON.stringify(binding)]), + ); + const draft = new Map( + normalizedEnvEntries(draftValue).map(([name, binding]) => [name, JSON.stringify(binding)]), + ); + const added: string[] = []; + const changed: string[] = []; + for (const [name, bindingKey] of draft) { + if (!committed.has(name)) added.push(name); + else if (committed.get(name) !== bindingKey) changed.push(name); + } + const removed = [...committed.keys()].filter((name) => !draft.has(name)); + return { added, changed, removed }; + }, [committedRows, draftValue]); + + const changeSummaryText = useMemo(() => { + const parts: string[] = []; + if (changeSummary.added.length > 0) parts.push(`New: ${formatChangedNames(changeSummary.added)}`); + if (changeSummary.changed.length > 0) parts.push(`Edited: ${formatChangedNames(changeSummary.changed)}`); + if (changeSummary.removed.length > 0) parts.push(`Removed: ${formatChangedNames(changeSummary.removed)}`); + return parts.join(" · "); + }, [changeSummary]); + + // Warn before the tab unloads with a dirty draft (same guard as the skill + // file editor). In-app navigation is not intercepted here. + useEffect(() => { + if (disabled || !hasUnsavedChanges) return; + function handleBeforeUnload(event: BeforeUnloadEvent) { + event.preventDefault(); + event.returnValue = ""; + } + window.addEventListener("beforeunload", handleBeforeUnload); + return () => window.removeEventListener("beforeunload", handleBeforeUnload); + }, [disabled, hasUnsavedChanges]); + const flushPendingDraft = useCallback(() => { if (disabled || !hasUnsavedChanges) return null; pendingSaveValueKeyRef.current = draftValueKey; @@ -463,9 +523,16 @@ export const EnvironmentVariablesEditor = forwardRef -
- - Unsaved changes +
+
+ + Unsaved changes +
+ {changeSummaryText ? ( +

+ {changeSummaryText} +

+ ) : null}