From 3ba924d17495359f65ea854c3fdf0c52e25bfbba Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Fri, 1 May 2026 13:40:58 -0700 Subject: [PATCH] review: drop destructive Override + single-fetch configuredKeys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of #2460 found two issues: 1. Critical: Override button in ProviderPickerModal called /settings/secrets when no workspaceId, overwriting the GLOBAL secret used by every workspace. The only consumers of this modal today (TemplatePalette, EmptyState via useTemplateDeploy) never pass workspaceId, so Override was always destructive. Removed entirely — the picker still solves the user-reported bug (always-ask + reuse saved keys); per-workspace key override can be a separate PR that plumbs secrets through POST /workspaces. 2. Optional: /settings/secrets was being fetched twice — once inside checkDeploySecrets (silently) and again in the hook to populate configuredKeys. Surfaced configuredKeys on PreflightResult so the hook re-uses the existing fetch. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/MissingKeysModal.tsx | 28 ++++------------- .../__tests__/useTemplateDeploy.test.tsx | 19 ++++++++---- canvas/src/hooks/useTemplateDeploy.tsx | 26 ++++------------ .../lib/__tests__/deploy-preflight.test.ts | 21 +++++++++++++ canvas/src/lib/deploy-preflight.ts | 30 +++++++++++++++++-- 5 files changed, 72 insertions(+), 52 deletions(-) diff --git a/canvas/src/components/MissingKeysModal.tsx b/canvas/src/components/MissingKeysModal.tsx index c4b795e3..1c3ef3cf 100644 --- a/canvas/src/components/MissingKeysModal.tsx +++ b/canvas/src/components/MissingKeysModal.tsx @@ -400,28 +400,12 @@ function ProviderPickerModal({
{entry.key}
{entry.saved && ( -
- - - Saved - - {/* Allow override when the saved state came from a - pre-configured global secret — the user may want - to use a different key for this workspace. */} - {configuredKeys?.has(entry.key) && ( - - )} -
+ + + Saved + )} diff --git a/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx b/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx index 43cba046..fb081ccf 100644 --- a/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx +++ b/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx @@ -129,6 +129,7 @@ beforeEach(() => { missingKeys: [], providers: [], runtime: "claude-code", + configuredKeys: new Set(), }); mockApiPost.mockResolvedValue({ id: "ws-new" }); // Default: secrets endpoint returns nothing so the picker @@ -232,6 +233,7 @@ describe("useTemplateDeploy — preflight failure modes", () => { missingKeys: ["ANTHROPIC_API_KEY"], providers: [], runtime: "claude-code", + configuredKeys: new Set(), }); const onDeployed = vi.fn(); @@ -259,6 +261,7 @@ describe("useTemplateDeploy — modal lifecycle", () => { missingKeys: ["ANTHROPIC_API_KEY"], providers: [], runtime: "claude-code", + configuredKeys: new Set(), }); const onDeployed = vi.fn(); const { result, rerender } = renderHook(() => @@ -293,6 +296,7 @@ describe("useTemplateDeploy — modal lifecycle", () => { missingKeys: ["ANTHROPIC_API_KEY"], providers: [], runtime: "claude-code", + configuredKeys: new Set(), }); const { result, rerender } = renderHook(() => useTemplateDeploy()); @@ -345,11 +349,8 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => { { id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] }, ], runtime: "hermes", + configuredKeys: new Set(["MINIMAX_API_KEY", "ANTHROPIC_API_KEY"]), }); - mockApiGet.mockResolvedValueOnce([ - { key: "MINIMAX_API_KEY", has_value: true, created_at: "", updated_at: "" }, - { key: "ANTHROPIC_API_KEY", has_value: true, created_at: "", updated_at: "" }, - ]); const { result, rerender } = renderHook(() => useTemplateDeploy()); await act(async () => { @@ -381,6 +382,7 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => { { id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] }, ], runtime: "hermes", + configuredKeys: new Set(), }); const { result, rerender } = renderHook(() => useTemplateDeploy()); @@ -408,6 +410,7 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => { { id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] }, ], runtime: "hermes", + configuredKeys: new Set(), }); const { result, rerender } = renderHook(() => useTemplateDeploy()); @@ -449,7 +452,11 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => { expect(onDeployed).toHaveBeenCalledWith("ws-new"); }); - it("secrets fetch failure still opens picker (empty configuredKeys)", async () => { + it("empty configuredKeys (preflight defensive fallback) still opens picker", async () => { + // checkDeploySecrets falls back to an empty Set when the + // /settings/secrets endpoint errors — the modal must still + // open so the user isn't blocked, just with every entry + // rendered as input rather than Saved. mockCheckDeploySecrets.mockResolvedValueOnce({ ok: true, missingKeys: [], @@ -458,8 +465,8 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => { { id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] }, ], runtime: "hermes", + configuredKeys: new Set(), }); - mockApiGet.mockRejectedValueOnce(new Error("secrets fetch down")); const { result, rerender } = renderHook(() => useTemplateDeploy()); await act(async () => { diff --git a/canvas/src/hooks/useTemplateDeploy.tsx b/canvas/src/hooks/useTemplateDeploy.tsx index eb749043..5c46c740 100644 --- a/canvas/src/hooks/useTemplateDeploy.tsx +++ b/canvas/src/hooks/useTemplateDeploy.tsx @@ -6,7 +6,6 @@ import { checkDeploySecrets, resolveRuntime, type PreflightResult, - type SecretEntry, type Template, } from "@/lib/deploy-preflight"; import { MissingKeysModal } from "@/components/MissingKeysModal"; @@ -45,15 +44,14 @@ export interface UseTemplateDeployOptions { /** Paired template + preflight result carried through the "user * clicked deploy → modal opens → keys saved → retry" loop. Named * so the `useState` generic and any future signature change have - * a single place to track. `configuredKeys` lets the modal mark - * pre-saved entries (global secrets) without re-prompting — the + * a single place to track. `preflight.configuredKeys` lets the + * modal mark pre-saved entries without re-prompting — the * template-deploy "always ask" flow surfaces the picker even when * preflight.ok is true so the user can pick a different provider * per workspace. */ interface MissingKeysInfo { template: Template; preflight: PreflightResult; - configuredKeys: Set; } export interface UseTemplateDeployResult { @@ -157,21 +155,7 @@ export function useTemplateDeploy( const shouldShowPicker = !preflight.ok || preflight.providers.length >= 2; if (shouldShowPicker) { - // Read the secret set the modal needs to mark pre-saved - // entries. We did this inside checkDeploySecrets too but - // didn't surface it; pull it again so a slow secrets - // endpoint failing here doesn't block the modal — empty - // set just means everything renders as input. - let configuredKeys = new Set(); - try { - const secrets = await api.get("/settings/secrets"); - configuredKeys = new Set( - secrets.filter((s) => s.has_value).map((s) => s.key), - ); - } catch { - // Empty set — modal will render every entry as input. - } - setMissingKeysInfo({ template, preflight, configuredKeys }); + setMissingKeysInfo({ template, preflight }); setDeploying(null); return; } @@ -201,7 +185,7 @@ export function useTemplateDeploy( ? "Configure Workspace" : undefined; const modalDescription = allConfigured - ? "Pick the provider and model for this workspace. Saved API keys are reused — click Override to use a different one." + ? "Pick the provider and model for this workspace. Saved API keys are reused automatically." : undefined; const modal: ReactNode = ( { const result = await checkDeploySecrets(LANGGRAPH); expect(result.ok).toBe(false); expect(result.missingKeys).toEqual(["OPENAI_API_KEY"]); + // Empty Set on fetch failure — useTemplateDeploy relies on this + // so the picker still opens with every entry rendered as input. + expect(result.configuredKeys).toEqual(new Set()); + }); + + it("surfaces configuredKeys (has_value=true entries only) so callers skip a second fetch", async () => { + (global.fetch as ReturnType).mockResolvedValueOnce({ + ok: true, + json: () => + Promise.resolve([ + { key: "ANTHROPIC_API_KEY", has_value: true, created_at: "", updated_at: "" }, + { key: "OPENROUTER_API_KEY", has_value: false, created_at: "", updated_at: "" }, + { key: "RANDOM_OTHER_KEY", has_value: true, created_at: "", updated_at: "" }, + ]), + } as Response); + + const result = await checkDeploySecrets(HERMES); + // Only has_value=true entries belong in the set. + expect(result.configuredKeys).toEqual( + new Set(["ANTHROPIC_API_KEY", "RANDOM_OTHER_KEY"]), + ); }); }); diff --git a/canvas/src/lib/deploy-preflight.ts b/canvas/src/lib/deploy-preflight.ts index a1f1d7a6..f2821d35 100644 --- a/canvas/src/lib/deploy-preflight.ts +++ b/canvas/src/lib/deploy-preflight.ts @@ -91,6 +91,12 @@ export interface PreflightResult { * required (AllKeysModal renders the N envVars inline). */ providers: ProviderChoice[]; runtime: string; + /** Set of env var names already configured (i.e. `has_value: true`) at + * the relevant scope (workspace if `workspaceId` was passed, otherwise + * global). Surfaced so callers can mark pre-saved entries in the + * picker without making a second `/settings/secrets` round trip. + * Empty Set on secrets-endpoint failure (treated as "nothing set"). */ + configuredKeys: Set; } /* ---------- Provider options ---------- */ @@ -235,7 +241,13 @@ export async function checkDeploySecrets( if (providers.length === 0) { // Template declares no env requirements — nothing to preflight. - return { ok: true, missingKeys: [], providers: [], runtime }; + return { + ok: true, + missingKeys: [], + providers: [], + runtime, + configuredKeys: new Set(), + }; } let configured: Set; @@ -254,7 +266,13 @@ export async function checkDeploySecrets( } if (findSatisfiedProvider(providers, configured)) { - return { ok: true, missingKeys: [], providers, runtime }; + return { + ok: true, + missingKeys: [], + providers, + runtime, + configuredKeys: configured, + }; } // Nothing configured — surface every candidate env var so the modal @@ -262,5 +280,11 @@ export async function checkDeploySecrets( const missingKeys = Array.from( new Set(providers.flatMap((p) => p.envVars)), ); - return { ok: false, missingKeys, providers, runtime }; + return { + ok: false, + missingKeys, + providers, + runtime, + configuredKeys: configured, + }; }