From 9abc9a0487d9ac98047f1fd9a2bbf831c5350d51 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Fri, 1 May 2026 17:19:14 -0700 Subject: [PATCH] feat(canvas): always prompt provider+model on template deploy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the picker modal opened only when preflight failed OR the template offered ≥2 provider options. Single-provider templates with saved keys (claude-code, langgraph) deployed silently using the template's compiled-in default model — denying the user a final chance to override before an EC2 boots and burns billing on the wrong tier. The picker UI already supports the "all-keys-saved single-provider" case as a confirm-only prompt (provider radio is hidden, model input is pre-filled with template.model), so flipping shouldShowPicker to unconditional is a one-line change with the picker UX absorbing it. Test plan - Existing "single-provider skips picker when preflight.ok" regression guard inverted to assert picker always opens. - Three happy-path tests refactored to drive through the picker via a new deployThroughPicker helper instead of expecting an immediate POST. - POST-failure tests likewise refactored — the failure now surfaces through the picker click-through path, not the direct deploy() call. - 15/15 tests pass; deploy-preflight.test.ts unchanged + 20/20. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../__tests__/useTemplateDeploy.test.tsx | 91 +++++++++++++------ canvas/src/hooks/useTemplateDeploy.tsx | 38 ++++---- 2 files changed, 81 insertions(+), 48 deletions(-) diff --git a/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx b/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx index fb081ccf..4e96830e 100644 --- a/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx +++ b/canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx @@ -143,14 +143,38 @@ afterEach(() => { // ── Tests ──────────────────────────────────────────────────────────────────── -describe("useTemplateDeploy — happy path", () => { - it("preflight ok → POST /workspaces → onDeployed fires with new id", async () => { - const onDeployed = vi.fn(); - const { result } = renderHook(() => useTemplateDeploy({ onDeployed })); +/** + * Drive the always-show-picker flow to completion: deploy() opens the + * modal, then we click "keys added" to fire the actual POST. Centralised + * here because as of the always-prompt change, every happy-path test + * must click through the modal before asserting on POST. + */ +async function deployThroughPicker( + result: { current: ReturnType }, + rerender: () => void, + template: Template, +): Promise { + await act(async () => { + await result.current.deploy(template); + }); + rerender(); + render(<>{result.current.modal}); + await act(async () => { + fireEvent.click(screen.getByTestId("modal-keys-added")); + // Let the fire-and-forget executeDeploy resolve. + await Promise.resolve(); + await Promise.resolve(); + }); +} - await act(async () => { - await result.current.deploy(makeTemplate()); - }); +describe("useTemplateDeploy — happy path", () => { + it("preflight ok → modal opens → keys-added → POST /workspaces → onDeployed fires", async () => { + const onDeployed = vi.fn(); + const { result, rerender } = renderHook(() => + useTemplateDeploy({ onDeployed }), + ); + + await deployThroughPicker(result, rerender, makeTemplate()); expect(mockCheckDeploySecrets).toHaveBeenCalledTimes(1); expect(mockApiPost).toHaveBeenCalledWith( @@ -168,11 +192,11 @@ describe("useTemplateDeploy — happy path", () => { it("uses caller-supplied canvasCoords when provided", async () => { const canvasCoords = vi.fn(() => ({ x: 42, y: 99 })); - const { result } = renderHook(() => useTemplateDeploy({ canvasCoords })); + const { result, rerender } = renderHook(() => + useTemplateDeploy({ canvasCoords }), + ); - await act(async () => { - await result.current.deploy(makeTemplate()); - }); + await deployThroughPicker(result, rerender, makeTemplate()); expect(canvasCoords).toHaveBeenCalledTimes(1); expect(mockApiPost).toHaveBeenCalledWith( @@ -182,11 +206,9 @@ describe("useTemplateDeploy — happy path", () => { }); it("falls back to random coords inside [100,500] × [100,400] when canvasCoords omitted", async () => { - const { result } = renderHook(() => useTemplateDeploy()); + const { result, rerender } = renderHook(() => useTemplateDeploy()); - await act(async () => { - await result.current.deploy(makeTemplate()); - }); + await deployThroughPicker(result, rerender, makeTemplate()); const body = (mockApiPost as Mock).mock.calls[0]?.[1] as { canvas: { x: number; y: number }; @@ -436,20 +458,31 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => { ); }); - it("single-provider template still skips picker when preflight.ok", async () => { + it("single-provider template ALSO opens picker when preflight.ok (always-prompt rule)", async () => { // Default preflight mock: ok=true, providers=[]. claude-code is - // single-provider — there's nothing to choose, so the picker - // SHOULD remain hidden. Regression guard: don't accidentally - // make every deploy require a click-through. + // single-provider, but the always-prompt rule means the user must + // still click through the picker to confirm provider+model — even + // when keys are saved and the runtime has only one provider option. + // Reason: the user needs an explicit chance to override the + // template's default model (e.g. opus vs sonnet vs haiku) before + // an EC2 boots and burns billing on the wrong tier. const onDeployed = vi.fn(); - const { result } = renderHook(() => useTemplateDeploy({ onDeployed })); + const { result, rerender } = renderHook(() => + useTemplateDeploy({ onDeployed }), + ); await act(async () => { await result.current.deploy(makeTemplate()); }); - expect(mockApiPost).toHaveBeenCalledTimes(1); - expect(onDeployed).toHaveBeenCalledWith("ws-new"); + rerender(); + render(<>{result.current.modal}); + + expect(screen.getByTestId("missing-keys-modal")).toBeTruthy(); + // POST does NOT fire until the user confirms in the picker. + expect(mockApiPost).not.toHaveBeenCalled(); + expect(onDeployed).not.toHaveBeenCalled(); + expect(result.current.deploying).toBeNull(); }); it("empty configuredKeys (preflight defensive fallback) still opens picker", async () => { @@ -486,11 +519,11 @@ describe("useTemplateDeploy — POST failure", () => { it("POST rejection sets error and clears deploying", async () => { mockApiPost.mockRejectedValueOnce(new Error("server 500")); const onDeployed = vi.fn(); - const { result } = renderHook(() => useTemplateDeploy({ onDeployed })); + const { result, rerender } = renderHook(() => + useTemplateDeploy({ onDeployed }), + ); - await act(async () => { - await result.current.deploy(makeTemplate()); - }); + await deployThroughPicker(result, rerender, makeTemplate()); expect(result.current.error).toBe("server 500"); expect(result.current.deploying).toBeNull(); @@ -499,11 +532,9 @@ describe("useTemplateDeploy — POST failure", () => { it("non-Error rejection still surfaces a message (defensive)", async () => { mockApiPost.mockRejectedValueOnce("plain string"); - const { result } = renderHook(() => useTemplateDeploy()); + const { result, rerender } = renderHook(() => useTemplateDeploy()); - await act(async () => { - await result.current.deploy(makeTemplate()); - }); + await deployThroughPicker(result, rerender, makeTemplate()); expect(result.current.error).toBe("Deploy failed"); expect(result.current.deploying).toBeNull(); diff --git a/canvas/src/hooks/useTemplateDeploy.tsx b/canvas/src/hooks/useTemplateDeploy.tsx index 5c46c740..4f746c98 100644 --- a/canvas/src/hooks/useTemplateDeploy.tsx +++ b/canvas/src/hooks/useTemplateDeploy.tsx @@ -143,25 +143,27 @@ export function useTemplateDeploy( setDeploying(null); return; } - // Open the picker modal whenever preflight failed OR the - // template offers ≥2 provider options. Multi-provider - // templates (e.g. hermes) need an explicit per-workspace - // pick — silently inheriting whichever global key matches - // produces the "No LLM provider configured" 500 because the - // adapter falls back to its compiled-in default model when - // the user hasn't asserted a slug. The "always ask" rule - // skips claude-code / langgraph-style single-provider - // templates where there's nothing to choose between. - const shouldShowPicker = - !preflight.ok || preflight.providers.length >= 2; - if (shouldShowPicker) { - setMissingKeysInfo({ template, preflight }); - setDeploying(null); - return; - } - await executeDeploy(template); + // Always open the picker — every deploy goes through an + // explicit confirm-provider/model step. Reasons: + // 1. Multi-provider templates (e.g. hermes) need a per- + // workspace pick or the adapter falls back to its + // compiled-in default and 500s with "No LLM provider + // configured". + // 2. Single-provider templates (claude-code, langgraph) + // still need the model field — the template's default + // may be wrong for the user's billing tier or a model + // they explicitly want (sonnet vs opus vs haiku). + // 3. Even when keys + model are pre-filled, surfacing the + // modal one-click-away is the cheapest UX for catching + // a misconfigured org BEFORE provisioning an EC2 that + // will then sit in degraded. + // The picker handles the "all-keys-saved single-provider" + // case as a confirm-only prompt (provider radio is hidden, + // model input is pre-filled with template.model). + setMissingKeysInfo({ template, preflight }); + setDeploying(null); }, - [executeDeploy], + [], ); // No useCallback here — consumers call this on every render anyway