fix: default new task executor to worktree (#1321)

This commit is contained in:
Carlos Florêncio
2026-06-09 16:40:27 +01:00
committed by GitHub
parent 63434f29cf
commit 71bf90d9e0
3 changed files with 392 additions and 173 deletions
@@ -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<DefaultSelFake> = {}): 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> = {}): 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();
});
});
@@ -181,6 +181,8 @@ type DefaultSelFake = Pick<
| "setExecutorProfileId"
| "noRepository"
| "repositories"
| "remoteRepos"
| "useRemote"
>;
function makeDefaultSelFs(overrides: Partial<DefaultSelFake> = {}): DialogFormState {
return {
@@ -193,6 +195,8 @@ function makeDefaultSelFs(overrides: Partial<DefaultSelFake> = {}): 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<MultiRepoGuardFake> = {}): 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();
});
});
+169 -64
View File
@@ -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<string | null>(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<string | null>(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