From dc6fcd1ff1eaa52df7685c8d12257b650dbf611c Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 14 Aug 2026 08:03:20 -0400 Subject: [PATCH] fix(ui): move agent secret access to searchable secrets tab (#11283) 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. > - The agent configuration UI controls each agent and its allowed secrets. > - The environment variable editor already has a secret selector with search and folder navigation. > - The secret access editor used a basic list and made large secret stores hard to use. > - The secret access controls also occupied the main Configuration tab. > - This pull request reuses the rich selector and moves secret access to a dedicated Secrets tab. > - The benefit is one consistent secret selection workflow with clearer agent configuration navigation. ## Linked Issues or Issue Description **What existing behavior does this improve?** The agent detail configuration view and its secret access editor. **Subsystem affected** `ui/` — React and Vite board UI. **Current behavior** The secret access editor uses a basic select control. It does not provide the search and folder navigation available in the environment variable editor. The editor also appears inside the Configuration tab. **Proposed behavior** The secret access editor uses the shared secret picker. Users can search secrets and browse slash-delimited folders. Agent details provide a dedicated Secrets tab for this editor. **Reason and benefit** Large secret stores are slow to scan in a flat list. Reusing one selector reduces UI differences and makes scoped secret access easier to manage. **Breaking changes** None. The API and saved secret access data do not change. ## What Changed - Reused the environment variable secret picker in the agent secret access editor. - Preserved secret version selection and the create-secret action, including nested-popover focus handling. - Added a route-backed Secrets tab to agent details and removed secret access controls from Configuration. - Guarded unsaved configuration across tab, link, browser-history, and action-triggered navigation. - Rechecked dirty state when navigation-producing agent actions finish, covering edits made while a request is pending. - Added component, page, and Storybook coverage for the workflow. ## Verification - `pnpm --filter @paperclipai/ui exec vitest run src/components/AgentActionButtons.test.tsx src/components/AgentConfigForm.render.test.tsx src/components/AgentSecretAccessEditor.test.tsx src/components/environment-variables-editor/EnvironmentVariablesEditor.test.tsx src/pages/AgentDetail.progress.test.ts` — 82 tests passed. - `pnpm --filter @paperclipai/ui typecheck` - `pnpm check:token-gates` - All GitHub PR checks passed on `5209c5b787`, including build, canary, general and serialized tests, and all three e2e shards. - Greptile completed at 5/5 with zero unresolved review threads. ## Risks - Low risk. The API and persisted binding format are unchanged; this changes agent configuration navigation and secret selection UI. - Dirty-state guards now cover direct navigation, Back/Forward history, and navigation-producing agent actions, including pending-request races. - Tests cover tab separation, secret access updates, search, folder navigation, focus restoration, and navigation rejection. - No documentation change is required because commands, contracts, and setup steps do not change. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex with GPT-5. This runtime did not expose a more specific model ID or context window. The model used agentic reasoning, repository tools, and code 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 - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- ui/src/components/AgentActionButtons.test.tsx | 62 ++++- ui/src/components/AgentActionButtons.tsx | 40 +++- .../AgentConfigForm.render.test.tsx | 28 ++- ui/src/components/AgentConfigForm.tsx | 53 +++-- .../AgentSecretAccessEditor.test.tsx | 66 +++++- ui/src/components/AgentSecretAccessEditor.tsx | 130 +++++++++-- ui/src/components/PageTabBar.tsx | 2 +- .../SecretPicker.tsx | 32 +-- ui/src/pages/AgentDetail.progress.test.ts | 52 +++++ ui/src/pages/AgentDetail.tsx | 216 +++++++++++++++--- .../stories/agent-management.stories.tsx | 56 ++++- 11 files changed, 629 insertions(+), 108 deletions(-) diff --git a/ui/src/components/AgentActionButtons.test.tsx b/ui/src/components/AgentActionButtons.test.tsx index 507530bdbd..901717f9b7 100644 --- a/ui/src/components/AgentActionButtons.test.tsx +++ b/ui/src/components/AgentActionButtons.test.tsx @@ -126,7 +126,7 @@ describe("AgentActionButtons", () => { }); function render(agent: Agent, props: Partial> = {}) { - root = createRoot(container); + root ??= createRoot(container); root.render( @@ -205,4 +205,64 @@ describe("AgentActionButtons", () => { expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: ["agents", "detail", "alpha"] }); expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: ["agents", "company-1"] }); }); + + it("does not terminate when navigation away from a dirty detail page is rejected", async () => { + const onBeforeNavigate = vi.fn().mockReturnValue(false); + render(makeAgent(), { onBeforeNavigate, onTerminateSuccess: vi.fn() }); + await flushReact(); + + await act(async () => { + container.querySelector('[aria-label="Open actions for Alpha Agent"]')?.click(); + }); + await flushReact(); + + const terminateButton = Array.from(document.body.querySelectorAll("button")) + .find((button) => button.textContent?.includes("Terminate")); + await act(async () => { + terminateButton?.click(); + }); + await flushReact(); + + expect(onBeforeNavigate).toHaveBeenCalledOnce(); + expect(mockAgentsApi.terminate).not.toHaveBeenCalled(); + }); + + it("rechecks navigation when the form becomes dirty while termination is pending", async () => { + let resolveTermination!: (agent: Agent) => void; + mockAgentsApi.terminate.mockReturnValue(new Promise((resolve) => { + resolveTermination = resolve; + })); + const onBeforeNavigate = vi.fn().mockReturnValueOnce(true).mockReturnValue(false); + const onTerminateSuccess = vi.fn(); + const agent = makeAgent(); + render(agent, { + hasPendingNavigationChanges: false, + onBeforeNavigate, + onTerminateSuccess, + }); + await flushReact(); + + await act(async () => { + container.querySelector('[aria-label="Open actions for Alpha Agent"]')?.click(); + }); + await flushReact(); + const terminateButton = Array.from(document.body.querySelectorAll("button")) + .find((button) => button.textContent?.includes("Terminate")); + await act(async () => { + terminateButton?.click(); + }); + await flushReact(); + + render(agent, { + hasPendingNavigationChanges: true, + onBeforeNavigate, + onTerminateSuccess, + }); + await flushReact(); + resolveTermination(makeAgent({ status: "terminated" })); + await flushReact(); + + expect(onBeforeNavigate).toHaveBeenCalledTimes(2); + expect(onTerminateSuccess).not.toHaveBeenCalled(); + }); }); diff --git a/ui/src/components/AgentActionButtons.tsx b/ui/src/components/AgentActionButtons.tsx index 2460e4e8eb..a062a71af2 100644 --- a/ui/src/components/AgentActionButtons.tsx +++ b/ui/src/components/AgentActionButtons.tsx @@ -1,4 +1,4 @@ -import { useCallback, useState, type ReactNode } from "react"; +import { useCallback, useRef, useState, type ReactNode } from "react"; import { useNavigate } from "@/lib/router"; import { useMutation, useQueryClient } from "@tanstack/react-query"; import { @@ -165,6 +165,8 @@ export function AgentActionButtons({ workActionsDisabled = false, workActionsDisabledReason, navigateToRunOnInvoke = true, + hasPendingNavigationChanges = false, + onBeforeNavigate, onActionError, onTerminateSuccess, pauseConfirm, @@ -182,6 +184,10 @@ export function AgentActionButtons({ workActionsDisabled?: boolean; workActionsDisabledReason?: string; navigateToRunOnInvoke?: boolean; + /** Whether the caller currently has an unsaved draft that navigation would discard. */ + hasPendingNavigationChanges?: boolean; + /** Return false to stop an action whose success would navigate away. */ + onBeforeNavigate?: () => boolean; /** * When set, pausing prompts a confirmation dialog first (e.g. for built-in * agents that power a feature). Omit for the immediate-pause default. @@ -207,6 +213,25 @@ export function AgentActionButtons({ const { pushToast } = useToastActions(); const [moreOpen, setMoreOpen] = useState(false); const [pauseConfirmOpen, setPauseConfirmOpen] = useState(false); + const pendingNavigationChangesRef = useRef(hasPendingNavigationChanges); + const beforeNavigateRef = useRef(onBeforeNavigate); + const agentActionStartedDirtyRef = useRef(false); + const duplicateStartedDirtyRef = useRef(false); + pendingNavigationChangesRef.current = hasPendingNavigationChanges; + beforeNavigateRef.current = onBeforeNavigate; + + function confirmNavigationStart(startedDirtyRef: React.MutableRefObject) { + startedDirtyRef.current = pendingNavigationChangesRef.current; + return beforeNavigateRef.current?.() !== false; + } + + function confirmLateNavigationChanges(startedDirtyRef: React.MutableRefObject) { + return ( + !pendingNavigationChangesRef.current || + startedDirtyRef.current || + beforeNavigateRef.current?.() !== false + ); + } const resolvedCompanyId = companyId ?? agent.companyId; const canonicalAgentRef = agentRouteRef(agent); @@ -251,9 +276,11 @@ export function AgentActionButtons({ onActionError?.(null); invalidateAgent(); if (action === "terminate") { + if (!confirmLateNavigationChanges(agentActionStartedDirtyRef)) return; onTerminateSuccess?.(data as Agent); } if (action === "invoke" && navigateToRunOnInvoke && data && typeof data === "object" && "id" in data) { + if (!confirmLateNavigationChanges(agentActionStartedDirtyRef)) return; navigate(`/agents/${canonicalAgentRef}/runs/${(data as HeartbeatRun).id}`); } }, @@ -285,6 +312,7 @@ export function AgentActionButtons({ await queryClient.invalidateQueries({ queryKey: queryKeys.agents.list(resolvedCompanyId) }); } pushToast({ title: "Agent duplicated", body: createdAgent.name, tone: "success" }); + if (!confirmLateNavigationChanges(duplicateStartedDirtyRef)) return; navigate(`/agents/${agentRouteRef(createdAgent)}/dashboard`); }, onError: (err) => { @@ -299,7 +327,7 @@ export function AgentActionButtons({ const nextName = duplicateAgentName(agent.name); const confirmed = window.confirm(`Duplicate ${agent.name} as ${nextName}?`); setMoreOpen(false); - if (!confirmed) return; + if (!confirmed || !confirmNavigationStart(duplicateStartedDirtyRef)) return; duplicateAgent.mutate(); }, [agent.name, duplicateAgent]); @@ -334,7 +362,10 @@ export function AgentActionButtons({ {assignLabel} agentAction.mutate("invoke")} + onClick={() => { + if (navigateToRunOnInvoke && !confirmNavigationStart(agentActionStartedDirtyRef)) return; + agentAction.mutate("invoke"); + }} disabled={assignAndRunDisabled} label={runLabel} size={size} @@ -423,8 +454,9 @@ export function AgentActionButtons({ + + + )} + +
+ {cards + ?

Secret access

+ :
Secret access
+ } +
+

{help.secretAccess}

+ createSecret.mutateAsync({ name, value })} + proposals={agentBindingProposals} + onApproveProposal={proposalReview.requestApprove} + onRejectProposal={proposalReview.requestReject} + /> + {proposalReview.dialogs} +
+
+ + ); + } + return (
{/* ---- Floating Save button (edit mode, when dirty) ---- */} @@ -1538,20 +1577,6 @@ export function AgentConfigForm(props: AgentConfigFormProps) { /> - {!isCreate && ( - - - {proposalReview.dialogs} - - )} - {/* Edit-only: timeout + grace period */} {!isCreate && ( <> diff --git a/ui/src/components/AgentSecretAccessEditor.test.tsx b/ui/src/components/AgentSecretAccessEditor.test.tsx index bc1df77a46..9078713752 100644 --- a/ui/src/components/AgentSecretAccessEditor.test.tsx +++ b/ui/src/components/AgentSecretAccessEditor.test.tsx @@ -5,18 +5,29 @@ import { createRoot, type Root } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { CompanySecret, EnvSecretRefBinding } from "@paperclipai/shared"; -// Stub SecretBindingPicker so the editor renders without CompanyContext / -// react-query. The stub exposes a button that binds a fixed secret. -vi.mock("./SecretBindingPicker", () => ({ - SecretBindingPicker: ({ - onChange, - }: { - onChange: (next: { secretId: string; version?: number | "latest" } | null) => void; - }) => ( - - ), +const mockSecretPickerRender = vi.hoisted(() => vi.fn()); + +// Keep this component test focused on access-row behavior while asserting that +// the editor routes selection through the shared, folder-aware env picker. +vi.mock("./environment-variables-editor/SecretPicker", () => ({ + SecretPicker: (props: { + secretId: string; + secrets: readonly CompanySecret[]; + onSelect: (secretId: string) => void; + onCreateNew?: (query: string) => void; + }) => { + mockSecretPickerRender(props); + return <> + + {props.onCreateNew ? ( + + ) : null} + ; + }, })); import { @@ -155,12 +166,43 @@ describe("AgentSecretAccessEditor component", () => { // Bind a secret via the stubbed picker. const pick = container.querySelector('[data-testid="pick-secret"]')!; + expect(mockSecretPickerRender).toHaveBeenLastCalledWith( + expect.objectContaining({ secretId: "", secrets }), + ); flushSync(() => pick.click()); const last = emitted.at(-1)!; expect(last).toEqual({ STRIPE: { type: "secret_ref", secretId: "s1", version: "latest" } }); }); + it("keeps the create form open when focus returns to the shared picker anchor", async () => { + vi.useFakeTimers(); + try { + render( + {}} + onCreateSecret={async () => secrets[0]!} + />, + ); + + const createButton = container.querySelector('[data-testid="create-secret"]')!; + flushSync(() => createButton.click()); + flushSync(() => vi.runAllTimers()); + expect(document.body.textContent).toContain("Create secret"); + + const pickerButton = container.querySelector('[data-testid="pick-secret"]')!; + pickerButton.focus(); + flushSync(() => {}); + + expect(document.body.textContent).toContain("Create secret"); + expect(document.querySelector('input[aria-label="Secret name"]')).toBeTruthy(); + } finally { + vi.useRealTimers(); + } + }); + it("renders pending binding proposals as Proposed rows with approve/reject", () => { const approved: string[] = []; const rejected: string[] = []; diff --git a/ui/src/components/AgentSecretAccessEditor.tsx b/ui/src/components/AgentSecretAccessEditor.tsx index 1152964836..272e02568f 100644 --- a/ui/src/components/AgentSecretAccessEditor.tsx +++ b/ui/src/components/AgentSecretAccessEditor.tsx @@ -9,14 +9,16 @@ import type { import { cn } from "../lib/utils"; import { Input } from "@/components/ui/input"; import { Badge } from "@/components/ui/badge"; -import { SecretBindingPicker, type SecretBindingValue } from "./SecretBindingPicker"; +import { SecretPicker } from "./environment-variables-editor/SecretPicker"; +import { CreateSecretPopover } from "./environment-variables-editor/CreateSecretPopover"; +import { Popover, PopoverAnchor, PopoverContent } from "@/components/ui/popover"; import { AGENT_ACCESS_CONFIG_PATH_PREFIX, ENV_CONFIG_PATH_PREFIX, SECRET_ALIAS_RE, deliveryModeDescription, } from "../lib/secret-delivery"; -import { envKeyFromSecretName } from "./environment-variables-editor/model"; +import { envKeyFromSecretName, secretNameFromKey } from "./environment-variables-editor/model"; import { DeliveryBadge as ProposalDeliveryBadge, ProposalActions, @@ -157,6 +159,8 @@ export interface AgentSecretAccessEditorProps { * parent diffs this against the current `access.*` keys to add/remove them. */ onChange: (next: Record) => void; + /** Create a company secret from the shared picker's pinned create action. */ + onCreateSecret?: (name: string, value: string) => Promise; disabled?: boolean; /** Pending binding proposals targeting this agent (PAP-14731). */ proposals?: readonly SecretProposalView[]; @@ -191,6 +195,7 @@ export function AgentSecretAccessEditor({ config, secrets, onChange, + onCreateSecret, disabled, proposals, onApproveProposal, @@ -208,6 +213,8 @@ export function AgentSecretAccessEditor({ const incomingKey = useMemo(() => normalizeAccessMapKey(incomingMap), [incomingMap]); const [rows, setRows] = useState(() => entriesToRows(apiBindings)); + const [createRequest, setCreateRequest] = useState<{ rowId: string; name: string } | null>(null); + const pickerAnchorRefs = useRef(new Map()); const lastEmittedKeyRef = useRef(incomingKey); const lastIncomingKeyRef = useRef(incomingKey); @@ -334,9 +341,7 @@ export function AgentSecretAccessEditor({ const trimmedAlias = row.alias.trim(); const aliasInvalid = Boolean(trimmedAlias) && !SECRET_ALIAS_RE.test(trimmedAlias); const aliasDuplicate = Boolean(trimmedAlias) && (aliasCounts.get(trimmedAlias) ?? 0) > 1; - const bindingValue: SecretBindingValue | null = row.secretId - ? { secretId: row.secretId, version: row.version } - : null; + const selectedSecret = secrets.find((secret) => secret.id === row.secretId) ?? null; return (
@@ -360,23 +365,106 @@ export function AgentSecretAccessEditor({ )} />
-
- +
+ { + if (!open && createRequest?.rowId === row.id) setCreateRequest(null); + }} + > + +
{ + if (node) pickerAnchorRefs.current.set(row.id, node); + else pickerAnchorRefs.current.delete(row.id); + }} + className="min-w-0 flex-1" + > + + patchRow(row.id, { + secretId, + version: "latest", + alias: + !row.alias.trim() && secretId + ? envKeyFromSecretName(secretName(secretId)) + : row.alias, + }) + } + onCreateNew={onCreateSecret + ? (query) => { + window.setTimeout(() => { + setCreateRequest({ + rowId: row.id, + name: secretNameFromKey(query) || query.trim(), + }); + }, 0); + } + : undefined} + disabled={disabled} + triggerClassName="h-9 min-h-9" + /> +
+ + { + // Closing the picker returns focus to its trigger inside + // this popover's anchor. Keep that focus restoration from + // dismissing the create form that just opened. + const target = event.detail.originalEvent.target as Node | null; + if (target && pickerAnchorRefs.current.get(row.id)?.contains(target)) { + event.preventDefault(); + } + }} + > + {createRequest?.rowId === row.id && onCreateSecret ? ( + secret.name)} + onCancel={() => setCreateRequest(null)} + onSubmit={async (name, value) => { + const created = await onCreateSecret(name, value); + patchRow(row.id, { + secretId: created.id, + version: "latest", + alias: row.alias.trim() || envKeyFromSecretName(created.name), + }); + setCreateRequest(null); + }} + /> + ) : null} + + +
+
+
+ +
+
+