From 49d1abc458671519bf3683ce6fe9ebd00e0d5b48 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 10 Jul 2026 17:14:28 -0700 Subject: [PATCH] fix(ui): detail the env unsaved-changes banner and guard against draft loss (#9391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Instance settings include an Environments section where operators configure execution environments, each with an environment-variables editor for run-time bindings > - The editor showed a bare "Unsaved changes" banner that never said which variables changed, sometimes appeared the moment a saved config was opened (a lossy round-trip through the editor's emit rules made clean values look dirty), and the environment form let you navigate away without any confirmation, silently dropping the draft > - Operators could not tell what was unsaved, distrusted the phantom banner, and lost half-finished environment edits to a stray click — the agent configuration page already confirms before discarding, so environments behaved inconsistently > - This pull request lists the new/edited/removed variable names under the banner, normalizes both sides of the dirty comparison so saved values no longer look dirty on open, and confirms before cancel, in-app navigation, or tab unload while the form has unsaved changes > - The benefit is that the banner is trustworthy and specific, and unsaved environment edits can no longer be lost without an explicit confirmation ## Linked Issues or Issue Description Related (not fixed by this PR): #8930 introduced the current environment-variables editor and its unsaved-changes banner; #9386 moved environment create/edit from a modal to routed pages, which this PR's navigation guard builds on. No existing public issue for the defects themselves; described per the bug report template: **What happened?** The environment-variables editor in Environments settings showed a bare "Unsaved changes" banner with no indication of which variables changed. For some saved configurations (names with surrounding whitespace, incomplete secret references, duplicate names differing only by whitespace) the banner appeared immediately on opening the edit form, before any user input. Navigating away from the environment form — cancel, an in-app link, or closing the tab — silently discarded the draft with no confirmation. **Expected behavior** The banner should say which variables are new, edited, or removed; a freshly opened saved configuration should show no banner; and leaving the form with unsaved changes should require an explicit confirmation, consistent with the agent configuration page. **Steps to reproduce** 1. Open Settings → Instance settings → Environments and edit an environment whose saved config round-trips lossily (e.g. an env var name stored with trailing whitespace) — the "Unsaved changes" banner appears with no user edits. 2. Add or edit a variable — the banner gives no hint of what is unsaved. 3. With a dirty draft, click any in-app link or Cancel — the draft is dropped with no confirmation. **Deployment mode** Self-hosted (local development instance), reproducible on `master`. ## What Changed - The unsaved-changes banner in `EnvironmentVariablesEditor` now renders a change summary line — `New: … · Edited: … · Removed: …` — showing up to three names per group with a `+N more` overflow and the full list in a `title` tooltip. A rename shows as one addition plus one removal. - Dirty detection normalizes both the committed value and the draft through the same rules the editor uses when emitting values (trimmed names, incomplete secret refs dropped, last-writer-wins on trimmed duplicates), so a saved config that round-trips lossily no longer shows a phantom banner on first open. - The editor exposes an `onDirtyChange` callback and warns via `beforeunload` while its local draft is dirty. - The environment create/edit page (`CompanyEnvironments`) tracks a payload-level baseline fingerprint of the form as initialized and treats the page as having unsaved changes when the current form differs from it or the editor draft is dirty. While dirty it confirms ("Discard unsaved environment changes?") on Cancel, intercepts same-origin in-app link clicks, and warns on tab unload. ## Verification - `node_modules/.bin/vitest run ui/src/pages/CompanyEnvironments.test.tsx ui/src/components/environment-variables-editor/EnvironmentVariablesEditor.test.tsx` — 48 tests pass, including new coverage for: the change-summary banner text, no phantom banner for lossy round-trip values, beforeunload only while dirty, cancel confirmation on the edit page, and unload/link-click warnings after edits are staged into the form. - `tsc -b` in `ui/` passes. - Manual: edit an environment, add/edit/remove variables, observe the summary line; click Cancel or an in-app link and observe the confirmation; save and observe navigation proceeds without prompting. ## Risks - Low risk, UI-only. The click interceptor is scoped to same-origin anchor navigation while the environment form page has unsaved changes and is removed on cleanup; modified-key/middle-button clicks and external links are left alone. - The dirty-normalization intentionally ignores differences the editor could never persist (incomplete secret refs, untrimmed duplicate names); those were previously reported as unsaved changes that could not be saved away. ## Model Used - Claude Fable 5 (`claude-fable-5`, Anthropic) via Claude Code, with extended thinking and tool use (code editing, test execution). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- .../EnvironmentVariablesEditor.test.tsx | 89 +++++++++++ .../environment-variables-editor/index.tsx | 147 +++++++++++++----- ui/src/pages/CompanyEnvironments.test.tsx | 90 +++++++++++ ui/src/pages/CompanyEnvironments.tsx | 108 ++++++++++++- 4 files changed, 392 insertions(+), 42 deletions(-) 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}