diff --git a/apps/web/components/task-create-dialog-effects-executor.test.ts b/apps/web/components/task-create-dialog-effects-executor.test.ts new file mode 100644 index 0000000000..980d0597bd --- /dev/null +++ b/apps/web/components/task-create-dialog-effects-executor.test.ts @@ -0,0 +1,219 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { renderHook, waitFor } from "@testing-library/react"; +import { useDefaultSelectionsEffect } from "./task-create-dialog-effects"; +import type { DialogFormState, StoreSelections } from "@/components/task-create-dialog-types"; + +beforeEach(() => { + localStorage.clear(); +}); + +const PROFILE_DOCKER = "profile-docker"; +const PROFILE_LOCAL = "profile-local"; +const PROFILE_WORKTREE = "profile-worktree"; + +type DefaultSelFake = Pick< + DialogFormState, + | "agentProfileId" + | "workflowAgentProfileId" + | "selectedWorkflowId" + | "executorId" + | "executorProfileId" + | "setAgentProfileId" + | "setExecutorId" + | "setExecutorProfileId" + | "noRepository" + | "repositories" + | "remoteRepos" + | "useRemote" +>; + +function makeDefaultSelFs(overrides: Partial = {}): DialogFormState { + return { + agentProfileId: "", + workflowAgentProfileId: "", + selectedWorkflowId: null, + executorId: "exec-1", + executorProfileId: "profile-1", + setAgentProfileId: vi.fn(), + setExecutorId: vi.fn(), + setExecutorProfileId: vi.fn(), + noRepository: false, + repositories: [], + remoteRepos: [], + useRemote: false, + ...overrides, + } as unknown as DialogFormState; +} + +function makeSel(overrides: Partial = {}): StoreSelections { + return { + agentProfiles: [], + compatibleAgentProfiles: [], + authLoaded: true, + executors: [], + workspaceDefaults: null, + ...overrides, + }; +} + +function localExecutor(): StoreSelections["executors"][number] { + return { + id: "exec-local", + type: "local", + profiles: [{ id: PROFILE_LOCAL, executor_type: "local" }], + } as unknown as StoreSelections["executors"][number]; +} + +function dockerExecutor(): StoreSelections["executors"][number] { + return { + id: "exec-docker", + type: "local_docker", + profiles: [{ id: PROFILE_DOCKER, executor_type: "local_docker" }], + } as unknown as StoreSelections["executors"][number]; +} + +function worktreeExecutor(): StoreSelections["executors"][number] { + return { + id: "exec-worktree", + type: "worktree", + profiles: [{ id: PROFILE_WORKTREE, executor_type: "worktree" }], + } as unknown as StoreSelections["executors"][number]; +} + +describe("useDefaultSelectionsEffect - executor profile defaults", () => { + it("defaults repo-backed tasks to the worktree profile when no profile was saved", async () => { + const fs = makeDefaultSelFs({ executorId: "", executorProfileId: "" }); + const local = localExecutor(); + const worktree = worktreeExecutor(); + const sel = makeSel({ executors: [local, worktree] }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await waitFor(() => expect(fs.setExecutorId).toHaveBeenCalledWith(worktree.id)); + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_WORKTREE)); + }); + + it("defaults repo-less tasks to a local profile because worktree needs a repo", async () => { + const fs = makeDefaultSelFs({ executorId: "", executorProfileId: "", noRepository: true }); + const worktree = worktreeExecutor(); + const local = localExecutor(); + const sel = makeSel({ executors: [worktree, local] }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await waitFor(() => expect(fs.setExecutorId).toHaveBeenCalledWith(local.id)); + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_LOCAL)); + }); + + it("defaults explicit local-path tasks to a local profile when no profile was saved", async () => { + const fs = makeDefaultSelFs({ + executorId: "", + executorProfileId: "", + repositories: [{ key: "row-0", localPath: "/workspace/custom", branch: "" }], + }); + const worktree = worktreeExecutor(); + const local = localExecutor(); + const sel = makeSel({ executors: [worktree, local] }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await waitFor(() => expect(fs.setExecutorId).toHaveBeenCalledWith(local.id)); + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_LOCAL)); + }); + + it("ignores a workspace-default worktree executor for explicit local-path tasks", async () => { + const fs = makeDefaultSelFs({ + executorId: "", + executorProfileId: "", + repositories: [{ key: "row-0", localPath: "/workspace/custom", branch: "" }], + }); + const worktree = worktreeExecutor(); + const local = localExecutor(); + const sel = makeSel({ + executors: [worktree, local], + workspaceDefaults: { + default_executor_id: worktree.id, + } as StoreSelections["workspaceDefaults"], + }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await waitFor(() => expect(fs.setExecutorId).toHaveBeenCalledWith(local.id)); + expect(fs.setExecutorId).not.toHaveBeenCalledWith(worktree.id); + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_LOCAL)); + }); + + it("does not fall back to a worktree profile for explicit local-path tasks", async () => { + const fs = makeDefaultSelFs({ + executorId: "", + executorProfileId: "", + repositories: [{ key: "row-0", localPath: "/workspace/custom", branch: "" }], + }); + const localWithoutProfiles = { ...localExecutor(), profiles: [] }; + const worktree = worktreeExecutor(); + const docker = dockerExecutor(); + const sel = makeSel({ executors: [worktree, localWithoutProfiles, docker] }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_DOCKER)); + expect(fs.setExecutorProfileId).not.toHaveBeenCalledWith(PROFILE_WORKTREE); + }); +}); + +describe("useDefaultSelectionsEffect - multi-repo guard counts Remote rows", () => { + it("swaps a non-worktree profile to worktree when 2+ Remote URL rows are set", async () => { + // Regression: the guard used to count only workspace/local rows, so 2 + // Remote rows slipped past it with an already-selected non-worktree profile. + const fs = makeDefaultSelFs({ + executorId: "exec-docker", + executorProfileId: PROFILE_DOCKER, + useRemote: true, + remoteRepos: [ + { key: "remote-0", url: "github.com/acme/a", branch: "", source: "paste" }, + { key: "remote-1", url: "github.com/acme/b", branch: "", source: "paste" }, + ] as DialogFormState["remoteRepos"], + }); + const sel = makeSel({ executors: [dockerExecutor(), worktreeExecutor()] }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_WORKTREE)); + }); + + it("leaves a worktree profile alone when 2+ Remote rows are set", async () => { + const fs = makeDefaultSelFs({ + executorId: "exec-worktree", + executorProfileId: PROFILE_WORKTREE, + useRemote: true, + remoteRepos: [ + { key: "remote-0", url: "github.com/acme/a", branch: "", source: "paste" }, + { key: "remote-1", url: "github.com/acme/b", branch: "", source: "paste" }, + ] as DialogFormState["remoteRepos"], + }); + const sel = makeSel({ executors: [worktreeExecutor()] }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); + }); + + it("does not swap when only a single Remote row is filled", async () => { + const fs = makeDefaultSelFs({ + executorId: "exec-docker", + executorProfileId: PROFILE_DOCKER, + useRemote: true, + remoteRepos: [ + { key: "remote-0", url: "github.com/acme/a", branch: "", source: "paste" }, + { key: "remote-1", url: "", branch: "", source: "paste" }, + ] as DialogFormState["remoteRepos"], + }); + const sel = makeSel({ executors: [dockerExecutor(), worktreeExecutor()] }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/components/task-create-dialog-effects.test.ts b/apps/web/components/task-create-dialog-effects.test.ts index 562168a130..432cd68d8b 100644 --- a/apps/web/components/task-create-dialog-effects.test.ts +++ b/apps/web/components/task-create-dialog-effects.test.ts @@ -181,6 +181,8 @@ type DefaultSelFake = Pick< | "setExecutorProfileId" | "noRepository" | "repositories" + | "remoteRepos" + | "useRemote" >; function makeDefaultSelFs(overrides: Partial = {}): DialogFormState { return { @@ -193,6 +195,8 @@ function makeDefaultSelFs(overrides: Partial = {}): DialogFormSt setExecutorId: vi.fn(), setExecutorProfileId: vi.fn(), noRepository: false, + remoteRepos: [], + useRemote: false, // Multi-repo guard effect inside useDefaultSelectionsEffect reads // fs.repositories when executorProfileId is set. Empty list keeps that // branch a no-op for these autopick-focused tests. @@ -476,112 +480,3 @@ describe("useGitHubUrlErrorEffect", () => { expect(setGitHubUrlError).toHaveBeenCalledWith(null); }); }); - -const PROFILE_DOCKER = "profile-docker"; -const PROFILE_WORKTREE = "profile-worktree"; - -function dockerExecutor(): StoreSelections["executors"][number] { - return { - id: "exec-docker", - type: "local_docker", - profiles: [{ id: PROFILE_DOCKER, executor_type: "local_docker" }], - } as unknown as StoreSelections["executors"][number]; -} -function worktreeExecutor(): StoreSelections["executors"][number] { - return { - id: "exec-worktree", - type: "worktree", - profiles: [{ id: PROFILE_WORKTREE, executor_type: "worktree" }], - } as unknown as StoreSelections["executors"][number]; -} - -type MultiRepoGuardFake = Pick< - DialogFormState, - | "agentProfileId" - | "workflowAgentProfileId" - | "selectedWorkflowId" - | "executorId" - | "executorProfileId" - | "setAgentProfileId" - | "setExecutorId" - | "setExecutorProfileId" - | "noRepository" - | "repositories" - | "remoteRepos" - | "useRemote" ->; -function makeMultiRepoFs(overrides: Partial = {}): DialogFormState { - return { - agentProfileId: "", - workflowAgentProfileId: "", - selectedWorkflowId: null, - executorId: "exec-docker", - executorProfileId: PROFILE_DOCKER, - setAgentProfileId: vi.fn(), - setExecutorId: vi.fn(), - setExecutorProfileId: vi.fn(), - noRepository: false, - repositories: [], - remoteRepos: [], - useRemote: false, - ...overrides, - } as unknown as DialogFormState; -} - -describe("useDefaultSelectionsEffect — multi-repo guard counts Remote rows", () => { - it("swaps a non-worktree profile to worktree when 2+ Remote URL rows are set", async () => { - // Regression: the multi-repo guard used to count only fs.repositories - // (workspace/local rows), so 2 Remote rows + 0 workspace rows slipped - // past the guard. The submit gate disabled incompatible profiles at the - // option level, but a profile already pre-selected stayed active and the - // backend rejected submit. - const fs = makeMultiRepoFs({ - executorProfileId: PROFILE_DOCKER, - useRemote: true, - remoteRepos: [ - { key: "remote-0", url: "github.com/acme/a", branch: "", source: "paste" }, - { key: "remote-1", url: "github.com/acme/b", branch: "", source: "paste" }, - ] as DialogFormState["remoteRepos"], - }); - const sel = makeSel({ executors: [dockerExecutor(), worktreeExecutor()] }); - - renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); - - await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_WORKTREE)); - }); - - it("leaves a worktree profile alone when 2+ Remote rows are set (already compatible)", async () => { - const fs = makeMultiRepoFs({ - executorProfileId: PROFILE_WORKTREE, - useRemote: true, - remoteRepos: [ - { key: "remote-0", url: "github.com/acme/a", branch: "", source: "paste" }, - { key: "remote-1", url: "github.com/acme/b", branch: "", source: "paste" }, - ] as DialogFormState["remoteRepos"], - }); - const sel = makeSel({ executors: [worktreeExecutor()] }); - - renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); - - await new Promise((resolve) => setTimeout(resolve, 10)); - // No swap needed — guard sees a worktree profile already. - expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); - }); - - it("does not swap when only a single Remote row is filled (single-repo Remote tasks keep all executors)", async () => { - const fs = makeMultiRepoFs({ - executorProfileId: PROFILE_DOCKER, - useRemote: true, - remoteRepos: [ - { key: "remote-0", url: "github.com/acme/a", branch: "", source: "paste" }, - { key: "remote-1", url: "", branch: "", source: "paste" }, - ] as DialogFormState["remoteRepos"], - }); - const sel = makeSel({ executors: [dockerExecutor(), worktreeExecutor()] }); - - renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); - - await new Promise((resolve) => setTimeout(resolve, 10)); - expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); - }); -}); diff --git a/apps/web/components/task-create-dialog-effects.ts b/apps/web/components/task-create-dialog-effects.ts index e37f429589..1a26e69be2 100644 --- a/apps/web/components/task-create-dialog-effects.ts +++ b/apps/web/components/task-create-dialog-effects.ts @@ -1,7 +1,7 @@ "use client"; import { useEffect, useMemo, useRef } from "react"; -import type { Repository, Executor } from "@/lib/types/http"; +import type { Repository, Executor, ExecutorProfile } from "@/lib/types/http"; import { DEFAULT_LOCAL_EXECUTOR_TYPE } from "@/lib/utils"; import { useToast } from "@/components/toast-provider"; import { @@ -199,89 +199,94 @@ export function useCurrentLocalBranchEffect( /** * Picks the default executor ID to auto-fill on dialog open. Repo-less tasks - * skip the worktree executor (it needs a repo); other modes use the workspace - * default → DEFAULT_LOCAL_EXECUTOR_TYPE → first available, in priority order. + * skip the worktree executor (it needs a repo). Explicit local paths prefer + * the local executor because the user chose an on-machine working tree. + * Otherwise repo-backed tasks use the workspace default → + * DEFAULT_LOCAL_EXECUTOR_TYPE → first available, in priority order. */ function pickDefaultExecutorId( executors: Executor[], workspaceDefaults: { default_executor_id?: string | null } | null | undefined, noRepository: boolean, + preferLocalExecutor: boolean, ): string | null { - const eligible = noRepository - ? executors.filter((e: Executor) => e.type !== "worktree") - : executors; + const eligible = + noRepository || preferLocalExecutor + ? executors.filter((e: Executor) => e.type !== "worktree") + : executors; if (eligible.length === 0) return null; const defId = workspaceDefaults?.default_executor_id ?? null; if (defId && eligible.some((e: Executor) => e.id === defId)) return defId; + if (noRepository || preferLocalExecutor) { + const directLocal = eligible.find((e: Executor) => isDirectLocalExecutorType(e.type)); + if (directLocal) return directLocal.id; + } const local = eligible.find((e: Executor) => e.type === DEFAULT_LOCAL_EXECUTOR_TYPE); return local?.id ?? eligible[0].id; } -export function useDefaultSelectionsEffect( - fs: DialogFormState, +type ExecutorProfileCandidate = ExecutorProfile & { + _executorId: string; + _executorType: string; +}; + +function isDirectLocalExecutorType(executorType: string | undefined): boolean { + return executorType === "local" || executorType === "local_pc"; +} + +function isWorktreeExecutorType(executorType: string | undefined): boolean { + return executorType === "worktree"; +} + +function flattenExecutorProfiles(executors: Executor[]): ExecutorProfileCandidate[] { + return executors.flatMap((e) => + (e.profiles ?? []).map((p) => ({ + ...p, + _executorId: e.id, + _executorType: p.executor_type ?? e.type, + })), + ); +} + +function pickDefaultExecutorProfileId( + executors: Executor[], + workspaceDefaults: { default_executor_id?: string | null } | null | undefined, + noRepository: boolean, + preferLocalExecutor: boolean, +): string | null { + const allProfiles = flattenExecutorProfiles(executors); + if (allProfiles.length === 0) return null; + const eligibleProfiles = + noRepository || preferLocalExecutor + ? allProfiles.filter((p) => !isWorktreeExecutorType(p._executorType)) + : allProfiles; + if (eligibleProfiles.length === 0) return null; + + const lastId = getLocalStorage(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, null); + if (lastId && eligibleProfiles.some((p) => p.id === lastId)) return lastId; + + const executorId = pickDefaultExecutorId( + executors, + workspaceDefaults, + noRepository, + preferLocalExecutor, + ); + const executorProfile = eligibleProfiles.find((p) => p._executorId === executorId); + return executorProfile?.id ?? eligibleProfiles[0].id; +} + +function useMultiRepoGuardEffect( open: boolean, - sel: StoreSelections, - workflows: Array<{ id: string; agent_profile_id?: string }>, + executorProfileId: string, + setExecutorProfileId: (id: string) => void, + executors: Executor[], + selectedRepoCount: number, ) { - const { executors, workspaceDefaults } = sel; - const { executorId, executorProfileId, setExecutorId, setExecutorProfileId, noRepository } = fs; - useAgentProfileAutopickEffect(fs, open, sel, workflows); - - useEffect(() => { - if (!open || executorId || executors.length === 0) return; - const pick = pickDefaultExecutorId(executors, workspaceDefaults, noRepository); - if (pick) void Promise.resolve().then(() => setExecutorId(pick)); - }, [open, executorId, executors, workspaceDefaults, setExecutorId, noRepository]); - - useEffect(() => { - // Auto-select executor profile: last used (localStorage) → first available - if (!open || executorProfileId || executors.length === 0) return; - const allProfiles = executors.flatMap((e) => - (e.profiles ?? []).map((p) => ({ ...p, _executorId: e.id })), - ); - if (allProfiles.length === 0) return; - const lastId = getLocalStorage(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, null); - const pick = lastId && allProfiles.some((p) => p.id === lastId) ? lastId : allProfiles[0].id; - void Promise.resolve().then(() => setExecutorProfileId(pick)); - }, [open, executorProfileId, executors, setExecutorProfileId]); - - // Derive executorId from the selected executor profile - useEffect(() => { - if (!executorProfileId) return; - for (const executor of executors) { - const match = (executor.profiles ?? []).find((p) => p.id === executorProfileId); - if (match) { - void Promise.resolve().then(() => setExecutorId(executor.id)); - return; - } - } - }, [executorProfileId, executors, setExecutorId]); - // Multi-repo guard: when 2+ repos are selected, only worktree profiles can // run the task (Docker / Sprites / standalone don't yet provision sibling // repos under one task root). If the current profile is non-worktree, swap // to a worktree profile — preferring the last-used worktree, otherwise the // first one available. Single-repo selections leave the profile alone. - // - // Count is mode-aware: Remote mode counts non-empty URL rows, workspace - // mode counts rows with a repo/path. Without this, 2 Remote rows + 0 - // workspace rows would slip past the guard because the legacy check only - // inspected `fs.repositories` — `computeSelectedRepoCount` handles both. - // Depend on the count primitive, not the whole `fs` object. `fs` is a fresh - // literal every render, so listing it in the dep array would re-run this - // effect on every render. computeSelectedRepoCount only reads noRepository / - // useRemote / remoteRepos / repositories, so memoize over exactly those. - const { noRepository: fsNoRepository, useRemote, remoteRepos, repositories } = fs; - const selectedRepoCount = useMemo( - () => - computeSelectedRepoCount({ - noRepository: fsNoRepository, - useRemote, - remoteRepos, - repositories, - } as DialogFormState), - [fsNoRepository, useRemote, remoteRepos, repositories], - ); useEffect(() => { if (!open || !executorProfileId || executors.length === 0) return; if (selectedRepoCount <= 1) return; @@ -302,6 +307,106 @@ export function useDefaultSelectionsEffect( }, [open, executorProfileId, executors, selectedRepoCount, setExecutorProfileId]); } +export function useDefaultSelectionsEffect( + fs: DialogFormState, + open: boolean, + sel: StoreSelections, + workflows: Array<{ id: string; agent_profile_id?: string }>, +) { + const { executors, workspaceDefaults } = sel; + const { + executorId, + executorProfileId, + setExecutorId, + setExecutorProfileId, + noRepository, + useRemote, + repositories, + remoteRepos, + } = fs; + const preferLocalExecutor = + !noRepository && !useRemote && repositories.some((row) => Boolean(row.localPath)); + useAgentProfileAutopickEffect(fs, open, sel, workflows); + + useEffect(() => { + if (!open || executorId || executors.length === 0) return; + const pick = pickDefaultExecutorId( + executors, + workspaceDefaults, + noRepository, + preferLocalExecutor, + ); + if (pick) void Promise.resolve().then(() => setExecutorId(pick)); + }, [ + open, + executorId, + executors, + workspaceDefaults, + setExecutorId, + noRepository, + preferLocalExecutor, + ]); + + useEffect(() => { + // Auto-select executor profile: last used (localStorage) → source-aware + // executor default → first eligible profile. + if (!open || executorProfileId || executors.length === 0) return; + const pick = pickDefaultExecutorProfileId( + executors, + workspaceDefaults, + noRepository, + preferLocalExecutor, + ); + if (pick) void Promise.resolve().then(() => setExecutorProfileId(pick)); + }, [ + open, + executorProfileId, + executors, + workspaceDefaults, + setExecutorProfileId, + noRepository, + preferLocalExecutor, + ]); + + // Derive executorId from the selected executor profile + useEffect(() => { + if (!executorProfileId) return; + for (const executor of executors) { + const match = (executor.profiles ?? []).find((p) => p.id === executorProfileId); + if (match) { + void Promise.resolve().then(() => setExecutorId(executor.id)); + return; + } + } + }, [executorProfileId, executors, setExecutorId]); + + // Count is mode-aware: Remote mode counts non-empty URL rows, workspace + // mode counts rows with a repo/path. Without this, 2 Remote rows + 0 + // workspace rows would slip past the guard because the legacy check only + // inspected `fs.repositories` — `computeSelectedRepoCount` handles both. + // Depend on the count primitive, not the whole `fs` object. `fs` is a fresh + // literal every render, so listing it in the dep array would re-run this + // effect on every render. computeSelectedRepoCount only reads noRepository / + // useRemote / remoteRepos / repositories, so memoize over exactly those. + const selectedRepoCount = useMemo( + () => + computeSelectedRepoCount({ + noRepository, + useRemote, + remoteRepos, + repositories, + } as DialogFormState), + [noRepository, useRemote, remoteRepos, repositories], + ); + useMultiRepoGuardEffect( + open, + executorProfileId, + setExecutorProfileId, + executors, + selectedRepoCount, + ); +} + /** * Surfaces a "Invalid GitHub URL" error for the first remote row when its URL * doesn't parse as a repo or a PR URL. Per-row PR-info fetching + branch