feat(canvas): always prompt provider+model on template deploy
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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang
parent
f80e054a95
commit
9abc9a0487
@ -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<T>(
|
||||
result: { current: ReturnType<typeof useTemplateDeploy> },
|
||||
rerender: () => void,
|
||||
template: Template,
|
||||
): Promise<void> {
|
||||
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();
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
Reference in New Issue
Block a user