From c277822ac1f6ce35343f9b5e39971eed67db7d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 17 Sep 2026 15:24:36 +0100 Subject: [PATCH] fix: unify executor profile editor routes (#3740) (#3762) --- .../[id]/profile/[profileId]/page.tsx | 565 ------------------ .../executors/k8s/[executorId]/page.tsx | 6 +- .../new/[type]/kubernetes-create-page.tsx | 2 +- .../settings/executors/new/[type]/page.tsx | 5 +- .../executors/new/[type]/ssh-create-page.tsx | 3 +- apps/web/app/settings/executors/page.tsx | 3 +- .../settings/settings-menu-branches.test.ts | 12 +- .../settings/settings-menu-branches.ts | 12 +- .../executor-profile-navigation.test.tsx | 114 ++++ .../settings/executor-profiles-card.test.tsx | 6 +- .../settings/executor-profiles-card.tsx | 14 +- .../legacy-executor-settings-route.test.tsx | 151 ++++- .../legacy-executor-settings-route.tsx | 51 +- .../task-create-dialog-form-body.tsx | 3 +- .../task/executor-environment-disclosure.tsx | 5 +- .../settings/executor-profile-routing.spec.ts | 123 ++++ .../mobile-executor-profile-routing.spec.ts | 98 +++ .../lib/settings-discovery/catalog.test.ts | 9 +- apps/web/lib/settings-discovery/resolve.ts | 3 +- .../settings/executor-settings-routes.test.ts | 12 + .../lib/settings/executor-settings-routes.ts | 9 +- .../plan.md | 226 +++++++ .../task-01-unify-profile-editor.md | 187 ++++++ docs/public/executors.md | 2 + .../executors/requirements/profile-editor.md | 47 ++ .../executors/system-design/profile-editor.md | 106 ++++ 26 files changed, 1117 insertions(+), 657 deletions(-) delete mode 100644 apps/web/app/settings/executor/[id]/profile/[profileId]/page.tsx create mode 100644 apps/web/components/settings/executor-profile-navigation.test.tsx create mode 100644 apps/web/e2e/tests/settings/executor-profile-routing.spec.ts create mode 100644 apps/web/e2e/tests/settings/mobile-executor-profile-routing.spec.ts create mode 100644 apps/web/lib/settings/executor-settings-routes.test.ts create mode 100644 docs/plans/executor-profile-editor-unification/plan.md create mode 100644 docs/plans/executor-profile-editor-unification/task-01-unify-profile-editor.md create mode 100644 docs/specs/executors/requirements/profile-editor.md create mode 100644 docs/specs/executors/system-design/profile-editor.md diff --git a/apps/web/app/settings/executor/[id]/profile/[profileId]/page.tsx b/apps/web/app/settings/executor/[id]/profile/[profileId]/page.tsx deleted file mode 100644 index 0b806d74a9..0000000000 --- a/apps/web/app/settings/executor/[id]/profile/[profileId]/page.tsx +++ /dev/null @@ -1,565 +0,0 @@ -"use client"; - -import { useCallback, useEffect, useMemo, useState } from "react"; -import { useRouter } from "@/lib/routing/client-router"; -import { runWithNavigationBlockerBypassed } from "@/lib/routing/navigation-guard"; -import { Button } from "@kandev/ui/button"; -import { Card, CardContent } from "@kandev/ui/card"; -import { Input } from "@kandev/ui/input"; -import { Separator } from "@kandev/ui/separator"; -import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@kandev/ui/select"; -import { - Dialog, - DialogContent, - DialogDescription, - DialogFooter, - DialogHeader, - DialogTitle, -} from "@kandev/ui/dialog"; -import { IconPlus, IconTrash } from "@tabler/icons-react"; -import { useAppStore } from "@/components/state-provider"; -import { useToast } from "@/components/toast-provider"; -import { useSettingsSaveContributor } from "@/components/settings/settings-save-provider"; -import { SettingsCard } from "@/components/settings/settings-card"; -import { SettingsCardHeader } from "@/components/settings/settings-card-header"; -import { SettingsPageHeader, SettingsFieldLabel } from "@/components/settings/settings-typography"; -import { settingsActionClassName } from "@/components/settings/settings-control"; -import { serializeSettingsRevision } from "@/components/settings/settings-save-revision"; -import { useSecrets } from "@/hooks/domains/settings/use-secrets"; -import { - updateExecutorProfile, - deleteExecutorProfile, - listScriptPlaceholders, -} from "@/lib/api/domains/settings-api"; -import type { ScriptPlaceholder } from "@/lib/api/domains/settings-api"; -import { - upsertExecutorProfile, - type SaveStatus, -} from "@/components/settings/profile-edit/profile-edit-page-chrome"; -import { - SpritesConnectionCard, - SpritesInstancesCard, -} from "@/components/settings/sprites-settings"; -import { ScriptCard } from "@/components/settings/profile-edit/script-card"; -import type { Executor, ExecutorProfile, ProfileEnvVar } from "@/lib/types/http"; -import { useTranslation } from "react-i18next"; - -type EnvVarRow = { - key: string; - mode: "value" | "secret"; - value: string; - secretId: string; -}; - -function envVarsToRows(envVars?: ProfileEnvVar[]): EnvVarRow[] { - if (!envVars || envVars.length === 0) return []; - return envVars.map((ev) => ({ - key: ev.key, - mode: ev.secret_id ? "secret" : "value", - value: ev.value ?? "", - secretId: ev.secret_id ?? "", - })); -} - -function rowsToEnvVars(rows: EnvVarRow[]): ProfileEnvVar[] { - return rows - .filter((r) => r.key.trim()) - .map((r) => { - if (r.mode === "secret" && r.secretId) { - return { key: r.key.trim(), secret_id: r.secretId }; - } - return { key: r.key.trim(), value: r.value }; - }); -} - -export default function ProfileDetailPage({ - executorId, - profileId, -}: { - executorId: string; - profileId: string; -}) { - const { t } = useTranslation(); - const router = useRouter(); - const executor = useAppStore( - (state) => state.executors.items.find((e: Executor) => e.id === executorId) ?? null, - ); - const profile = executor?.profiles?.find((p: ExecutorProfile) => p.id === profileId) ?? null; - - if (!executor || !profile) { - return ( - - -

{t("executors:profileNotFound")}

- - - - ); - } - - return ; -} - -function ProfileDetailsCard({ - name, - baselineName, - onNameChange, -}: { - name: string; - baselineName: string; - onNameChange: (v: string) => void; -}) { - const { t } = useTranslation(); - const isDirty = name.trim() !== baselineName.trim(); - return ( - - - -
- {t("executors:name")} - onNameChange(e.target.value)} - data-settings-dirty={isDirty} - /> -
- - - ); -} - -function EnvVarRow({ - row, - index, - secrets, - onUpdate, - onRemove, - baselineRow, -}: { - row: EnvVarRow; - index: number; - secrets: { id: string; name: string }[]; - onUpdate: (index: number, field: keyof EnvVarRow, val: string) => void; - onRemove: (index: number) => void; - baselineRow?: EnvVarRow; -}) { - const { t } = useTranslation(); - const isDirty = !baselineRow || JSON.stringify(row) !== JSON.stringify(baselineRow); - return ( -
- onUpdate(index, "key", e.target.value)} - placeholder="KEY" - className="font-mono text-xs flex-[2]" - data-settings-dirty={!baselineRow || row.key !== baselineRow.key} - /> - - {row.mode === "value" ? ( - onUpdate(index, "value", e.target.value)} - placeholder="value" - className="font-mono text-xs flex-[3]" - data-settings-dirty={!baselineRow || row.value !== baselineRow.value} - /> - ) : ( - - )} - -
- ); -} - -function EnvVarsCard({ - rows, - baselineRows, - secrets, - onAdd, - onUpdate, - onRemove, -}: { - rows: EnvVarRow[]; - baselineRows: EnvVarRow[]; - secrets: { id: string; name: string }[]; - onAdd: () => void; - onUpdate: (index: number, field: keyof EnvVarRow, val: string) => void; - onRemove: (index: number) => void; -}) { - const { t } = useTranslation(); - const isDirty = - JSON.stringify(rowsToEnvVars(rows)) !== JSON.stringify(rowsToEnvVars(baselineRows)); - return ( - - - - {t("executors:add")} - - } - /> - - {rows.length === 0 && ( -

- {t("executors:noEnvironmentVariablesConfigured")} -

- )} - {rows.map((row, idx) => ( - - ))} - - - ); -} - -function ProfileActions({ - executorId, - onRequestDelete, -}: { - executorId: string; - onRequestDelete: () => void; -}) { - const { t } = useTranslation(); - const router = useRouter(); - return ( -
- - -
- ); -} - -function DeleteProfileDialog({ - open, - onOpenChange, - onDelete, - deleting, -}: { - open: boolean; - onOpenChange: (v: boolean) => void; - onDelete: () => void; - deleting: boolean; -}) { - const { t } = useTranslation(); - return ( - - - - {t("executors:deleteProfile")} - {t("executors:areYouSureYouWantTo")} - - - - - - - - ); -} - -function useProfilePersistence(executor: Executor, profile: ExecutorProfile) { - const { t } = useTranslation(); - const router = useRouter(); - const { toast } = useToast(); - const executors = useAppStore((state) => state.executors.items); - const setExecutors = useAppStore((state) => state.setExecutors); - const [saveStatus, setSaveStatus] = useState("idle"); - const [error, setError] = useState(null); - const [deleteDialogOpen, setDeleteDialogOpen] = useState(false); - const [deleting, setDeleting] = useState(false); - - const save = useCallback( - async (data: { - name: string; - prepare_script: string; - cleanup_script: string; - env_vars: ProfileEnvVar[]; - }) => { - setSaveStatus("loading"); - setError(null); - try { - const updated = await updateExecutorProfile(executor.id, profile.id, data); - setSaveStatus("success"); - toast({ title: t("executors:profileSaved"), variant: "success" }); - setExecutors(upsertExecutorProfile(executors, executor, updated)); - window.setTimeout(() => setSaveStatus("idle"), 1500); - } catch (err) { - const message = err instanceof Error ? err.message : t("executors:failedToSaveProfile"); - setError(message); - setSaveStatus("error"); - toast({ - title: t("executors:failedToSaveProfile"), - description: message, - variant: "error", - }); - throw err; - } - }, - [executor, profile.id, executors, setExecutors, toast], - ); - - const remove = useCallback(async () => { - setDeleting(true); - try { - await deleteExecutorProfile(executor.id, profile.id); - setExecutors( - executors.map((e: Executor) => - e.id === executor.id - ? { ...e, profiles: e.profiles?.filter((p) => p.id !== profile.id) } - : e, - ), - ); - runWithNavigationBlockerBypassed(() => router.push(`/settings/executor/${executor.id}`)); - } catch { - setDeleting(false); - setDeleteDialogOpen(false); - } - }, [executor.id, profile.id, executors, setExecutors, router]); - - return { saveStatus, error, deleting, deleteDialogOpen, setDeleteDialogOpen, save, remove }; -} - -function useProfileFormState(executor: Executor, profile: ExecutorProfile) { - const { t } = useTranslation(); - const [name, setName] = useState(profile.name); - const [prepareScript, setPrepareScript] = useState(profile.prepare_script ?? ""); - const [cleanupScript, setCleanupScript] = useState(profile.cleanup_script ?? ""); - const [envVarRows, setEnvVarRows] = useState(() => envVarsToRows(profile.env_vars)); - const [placeholders, setPlaceholders] = useState([]); - - const isRemote = - executor.type === "sprites" || - executor.type === "local_docker" || - executor.type === "remote_docker"; - const isSprites = executor.type === "sprites"; - - const spritesSecretId = useMemo(() => { - const tokenVar = envVarRows.find((r) => r.key === "SPRITES_API_TOKEN" && r.mode === "secret"); - return tokenVar?.secretId; - }, [envVarRows]); - - useEffect(() => { - listScriptPlaceholders() - .then((res) => setPlaceholders(res.placeholders ?? [])) - .catch(() => {}); - }, []); - - const addEnvVar = useCallback(() => { - setEnvVarRows((prev) => [...prev, { key: "", mode: "value", value: "", secretId: "" }]); - }, []); - const removeEnvVar = useCallback((index: number) => { - setEnvVarRows((prev) => prev.filter((_, i) => i !== index)); - }, []); - const updateEnvVar = useCallback((index: number, field: keyof EnvVarRow, val: string) => { - setEnvVarRows((prev) => prev.map((row, i) => (i === index ? { ...row, [field]: val } : row))); - }, []); - - // `{{` is the editor's placeholder-completion trigger, not an interpolation - // token, so it is interpolated as a value rather than written into the - // catalog — i18next would otherwise try to resolve it. - const prepareDesc = isRemote - ? t("executors:prepareScriptDescriptionRemote", { trigger: "{{" }) - : t("executors:prepareScriptDescriptionLocal"); - - return { - name, - setName, - prepareScript, - setPrepareScript, - cleanupScript, - setCleanupScript, - envVarRows, - addEnvVar, - removeEnvVar, - updateEnvVar, - placeholders, - isRemote, - isSprites, - spritesSecretId, - prepareDesc, - }; -} - -function ProfileEditHeader({ executor, profileName }: { executor: Executor; profileName: string }) { - const { t } = useTranslation(); - const router = useRouter(); - return ( - router.push(`/settings/executor/${executor.id}`)} - > - {t("executors:backToExecutor")} - - } - /> - ); -} - -function ProfileEditForm({ executor, profile }: { executor: Executor; profile: ExecutorProfile }) { - const { t } = useTranslation(); - const { items: secrets } = useSecrets(); - const persistence = useProfilePersistence(executor, profile); - const form = useProfileFormState(executor, profile); - - const savePayload = { - name: form.name.trim(), - prepare_script: form.prepareScript, - cleanup_script: form.cleanupScript, - env_vars: rowsToEnvVars(form.envVarRows), - }; - const saveRevision = serializeSettingsRevision(savePayload); - const [savedRevision, setSavedRevision] = useState(saveRevision); - const handleSave = async () => { - await persistence.save(savePayload); - setSavedRevision(saveRevision); - }; - useSettingsSaveContributor({ - id: `legacy-executor-profile:${profile.id}`, - revision: saveRevision, - isDirty: saveRevision !== savedRevision, - canSave: Boolean(form.name.trim()), - invalidReason: form.name.trim() ? undefined : t("executors:profileNameIsRequired"), - save: handleSave, - discard: () => undefined, - }); - - return ( -
- - - - {form.isSprites && form.spritesSecretId && ( - <> - - - - )} - - - {form.isRemote && ( - - )} - {persistence.error &&

{persistence.error}

} - persistence.setDeleteDialogOpen(true)} - /> - -
- ); -} diff --git a/apps/web/app/settings/executors/k8s/[executorId]/page.tsx b/apps/web/app/settings/executors/k8s/[executorId]/page.tsx index ea43b71b7b..b2fdfb9b44 100644 --- a/apps/web/app/settings/executors/k8s/[executorId]/page.tsx +++ b/apps/web/app/settings/executors/k8s/[executorId]/page.tsx @@ -58,11 +58,7 @@ export default function KubernetesExecutorPage({ executorId }: { executorId: str )?.profiles?.[0]?.id ?? null, ); if (profileId) { - return ( - - ); + return ; } return ; } diff --git a/apps/web/app/settings/executors/new/[type]/kubernetes-create-page.tsx b/apps/web/app/settings/executors/new/[type]/kubernetes-create-page.tsx index 5d3364c1e5..5768217d70 100644 --- a/apps/web/app/settings/executors/new/[type]/kubernetes-create-page.tsx +++ b/apps/web/app/settings/executors/new/[type]/kubernetes-create-page.tsx @@ -71,7 +71,7 @@ export function KubernetesCreatePage() { try { const created = await resource.create(payload); runWithNavigationBlockerBypassed(() => - router.push(executorProfileSettingsPath(created.executor, created.profile.id)), + router.push(executorProfileSettingsPath(created.profile.id)), ); } catch (cause) { setError(cause); diff --git a/apps/web/app/settings/executors/new/[type]/page.tsx b/apps/web/app/settings/executors/new/[type]/page.tsx index 40b6560198..739bd8065f 100644 --- a/apps/web/app/settings/executors/new/[type]/page.tsx +++ b/apps/web/app/settings/executors/new/[type]/page.tsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import { useTranslation } from "react-i18next"; +import { executorProfileSettingsPath } from "@/lib/settings/executor-settings-routes"; import { useRouter } from "@/lib/routing/client-router"; import { runWithNavigationBlockerBypassed } from "@/lib/routing/navigation-guard"; import { Badge } from "@kandev/ui/badge"; @@ -407,7 +408,9 @@ function useCreateProfileSave(executorId: string) { e.id === executorId ? { ...e, profiles: [...(e.profiles ?? []), profile] } : e, ), ); - runWithNavigationBlockerBypassed(() => router.push(`/settings/executors/${profile.id}`)); + runWithNavigationBlockerBypassed(() => + router.push(executorProfileSettingsPath(profile.id)), + ); } catch (err) { setError(err instanceof Error ? err.message : t("executors:failedToCreateProfile")); throw err; diff --git a/apps/web/app/settings/executors/new/[type]/ssh-create-page.tsx b/apps/web/app/settings/executors/new/[type]/ssh-create-page.tsx index 007ea87317..35c493e22a 100644 --- a/apps/web/app/settings/executors/new/[type]/ssh-create-page.tsx +++ b/apps/web/app/settings/executors/new/[type]/ssh-create-page.tsx @@ -2,6 +2,7 @@ import { useCallback } from "react"; import { useTranslation } from "react-i18next"; +import { executorProfileSettingsPath } from "@/lib/settings/executor-settings-routes"; import { useRouter } from "@/lib/routing/client-router"; import { Badge } from "@kandev/ui/badge"; import { Button } from "@kandev/ui/button"; @@ -68,7 +69,7 @@ export function SSHCreatePage() { const current = store.getState().executors.items; const merged = current.some((e) => e.id === next.id) ? current : [...current, next]; store.getState().setExecutors(merged); - router.push(`/settings/executors/${profile.id}`); + router.push(executorProfileSettingsPath(profile.id)); }, [router, store], ); diff --git a/apps/web/app/settings/executors/page.tsx b/apps/web/app/settings/executors/page.tsx index be2c84d4b8..594fd35640 100644 --- a/apps/web/app/settings/executors/page.tsx +++ b/apps/web/app/settings/executors/page.tsx @@ -22,6 +22,7 @@ import { EXECUTOR_ICON_MAP, getExecutorLabel } from "@/lib/executor-icons"; import type { Executor, ExecutorProfile } from "@/lib/types/http"; import { KubernetesReadOnlyNotice } from "@/components/settings/kubernetes-read-only-notice"; import { settingsActionClassName } from "@/components/settings/settings-control"; +import { executorProfileSettingsPath } from "@/lib/settings/executor-settings-routes"; type ProfileWithExecutor = ExecutorProfile & { executor_type: string; @@ -114,7 +115,7 @@ function ProfileCard({ return ( router.push(`/settings/executors/${profile.id}`)} + onClick={() => router.push(executorProfileSettingsPath(profile.id))} > diff --git a/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.test.ts b/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.test.ts index 1a6685a54f..4f7861029d 100644 --- a/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.test.ts +++ b/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.test.ts @@ -331,13 +331,11 @@ describe("buildAgentsBranch", () => { }); describe("buildExecutorsBranch", () => { - it("uses the executor-scoped profile route, the one with an executor crumb", () => { + it("uses the canonical profile route", () => { const [executor] = buildExecutorsBranch(EXECUTORS); expect(executor.href).toBe("/settings/executor/exec-1"); - expect(hrefsOf(executor.children ?? [])).toEqual([ - "/settings/executor/exec-1/profile/exec-profile-1", - ]); + expect(hrefsOf(executor.children ?? [])).toEqual(["/settings/executors/exec-profile-1"]); }); it("makes a configured Kubernetes executor disclosure-only and links its profile root", () => { @@ -446,12 +444,12 @@ describe("findActiveNodePath", () => { }); it("stops at the row when the route has no node — the row stays active", () => { - // The install catalogue is not an agent, and the flat profile spelling has - // no executor to nest under. Both are the row's own pages. + // The install catalogue is not an agent, and an unknown profile has no + // executor to nest under. Both are the row's own pages. expect(findActiveNodePath(forest, "/settings/agents/browse")).toEqual([ settingsMenuRowKey("/settings/agents"), ]); - expect(findActiveNodePath(forest, "/settings/executors/exec-profile-1")).toEqual([ + expect(findActiveNodePath(forest, "/settings/executors/unknown-profile")).toEqual([ settingsMenuRowKey(EXECUTORS_HREF), ]); }); diff --git a/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.ts b/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.ts index 2fbbc6abd9..bfc79f80ae 100644 --- a/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.ts +++ b/apps/web/components/app-sidebar/sections/settings/settings-menu-branches.ts @@ -316,11 +316,11 @@ export function buildAgentsBranch( /** * One node per executor, holding its profiles. * - * Most executors use the scoped legacy spellings so their executor breadcrumb - * matches the branch. A configured Kubernetes executor only discloses its - * profile children: connection, diagnostics, sessions and workload settings - * all live on that profile page. Its standalone connection route remains - * reachable only when there is no profile to recover through. + * Profile children always open the canonical profile editor. A configured + * Kubernetes executor only discloses its profile children: connection, + * diagnostics, sessions and workload settings all live on that profile page. + * Its standalone connection route remains reachable only when there is no + * profile to recover through. */ export function buildExecutorsBranch(executors: ReadonlyArray): SettingsMenuNode[] { return executors.map((executor) => { @@ -337,7 +337,7 @@ export function buildExecutorsBranch(executors: ReadonlyArray): // Same split as agents: the executor ships with kandev, the profiles do not. children: profiles.map((profile) => ({ key: `executor:${executor.id}:profile:${profile.id}`, - href: executorProfileSettingsPath(executor, profile.id), + href: executorProfileSettingsPath(profile.id), label: { text: profile.name }, isUserRecord: true, })), diff --git a/apps/web/components/settings/executor-profile-navigation.test.tsx b/apps/web/components/settings/executor-profile-navigation.test.tsx new file mode 100644 index 0000000000..8a6bbc4ad4 --- /dev/null +++ b/apps/web/components/settings/executor-profile-navigation.test.tsx @@ -0,0 +1,114 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import ExecutorsHubPage from "@/app/settings/executors/page"; +import { StateProvider } from "@/components/state-provider"; +import { ExecutorEnvironmentDisclosure } from "@/components/task/executor-environment-disclosure"; +import type { TaskEnvironment } from "@/lib/api/domains/task-environment-api"; +import type { Executor, ExecutorProfile } from "@/lib/types/http"; + +const { push } = vi.hoisted(() => ({ push: vi.fn() })); + +vi.mock("@/lib/routing/client-router", () => ({ + useRouter: () => ({ push }), +})); + +vi.mock("@/components/task/executor-environment-info", () => ({ + EnvironmentInfo: ({ kubernetesActions }: { kubernetesActions?: ReactNode }) => ( + <>{kubernetesActions} + ), +})); + +vi.mock("@kandev/ui/tooltip", () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children}, +})); + +const TIMESTAMP = "2026-08-24T00:00:00Z"; + +afterEach(() => { + cleanup(); + push.mockReset(); +}); + +function profile(executorId: string): ExecutorProfile { + return { + id: "profile/primary", + executor_id: executorId, + executor_type: "local_docker", + name: "Primary profile", + config: {}, + prepare_script: "", + cleanup_script: "", + env_vars: [], + created_at: TIMESTAMP, + updated_at: TIMESTAMP, + }; +} + +function executor(): Executor { + const id = "executor/primary"; + return { + id, + name: "Primary executor", + type: "local_docker", + status: "active", + is_system: false, + config: {}, + profiles: [profile(id)], + created_at: TIMESTAMP, + updated_at: TIMESTAMP, + }; +} + +const kubernetesEnvironment = { + id: "environment-1", + task_id: "task-1", + repository_id: "repository-1", + executor_type: "k8s", + executor_id: "executor/primary", + executor_profile_id: "profile/primary", + agent_execution_id: "execution-1", + control_port: 0, + status: "running", + created_at: TIMESTAMP, +} satisfies TaskEnvironment; + +describe("executor profile navigation", () => { + it("opens the hub profile card in the encoded canonical editor", () => { + const record = executor(); + render( + + + , + ); + + fireEvent.click(screen.getByText("Primary profile")); + + expect(push).toHaveBeenCalledWith("/settings/executors/profile%2Fprimary"); + }); + + it("opens the task disclosure settings action in the encoded canonical editor", () => { + render( + {}} + onReset={vi.fn()} + />, + ); + + expect(screen.getByTestId("executor-settings-link").getAttribute("href")).toBe( + "/settings/executors/profile%2Fprimary", + ); + }); +}); diff --git a/apps/web/components/settings/executor-profiles-card.test.tsx b/apps/web/components/settings/executor-profiles-card.test.tsx index d9408d0aa3..19163c6048 100644 --- a/apps/web/components/settings/executor-profiles-card.test.tsx +++ b/apps/web/components/settings/executor-profiles-card.test.tsx @@ -63,13 +63,11 @@ describe("ExecutorProfilesCard profile navigation", () => { expect(push).toHaveBeenCalledWith("/settings/executors/profile%2Fprimary"); }); - it("preserves the scoped legacy profile route for SSH", () => { + it("opens an SSH profile in the canonical editor", () => { renderCard(executor("ssh")); fireEvent.click(screen.getByText("Primary profile")); - expect(push).toHaveBeenCalledWith( - "/settings/executor/executor%2Fprimary/profile/profile%2Fprimary", - ); + expect(push).toHaveBeenCalledWith("/settings/executors/profile%2Fprimary"); }); }); diff --git a/apps/web/components/settings/executor-profiles-card.tsx b/apps/web/components/settings/executor-profiles-card.tsx index e86d1780e1..72a0f56022 100644 --- a/apps/web/components/settings/executor-profiles-card.tsx +++ b/apps/web/components/settings/executor-profiles-card.tsx @@ -61,7 +61,6 @@ export function ExecutorProfilesCard({ executorId, profiles }: ExecutorProfilesC const [dialogOpen, setDialogOpen] = useState(false); const executors = useAppStore((state) => state.executors.items); const setExecutors = useAppStore((state) => state.setExecutors); - const executorType = executors.find((executor) => executor.id === executorId)?.type; const refreshProfiles = useCallback(async () => { try { @@ -81,9 +80,9 @@ export function ExecutorProfilesCard({ executorId, profiles }: ExecutorProfilesC const handleProfileCreated = useCallback( (profile: ExecutorProfile) => { refreshProfiles(); - router.push(executorProfileSettingsPath({ id: executorId, type: executorType }, profile.id)); + router.push(executorProfileSettingsPath(profile.id)); }, - [executorId, executorType, refreshProfiles, router], + [refreshProfiles, router], ); const handleDelete = useCallback( @@ -117,14 +116,7 @@ export function ExecutorProfilesCard({ executorId, profiles }: ExecutorProfilesC
- router.push( - executorProfileSettingsPath( - { id: executorId, type: executorType }, - profile.id, - ), - ) - } + onClick={() => router.push(executorProfileSettingsPath(profile.id))} >
{profile.name} diff --git a/apps/web/components/settings/legacy-executor-settings-route.test.tsx b/apps/web/components/settings/legacy-executor-settings-route.test.tsx index 61f4e8fd09..e551f64646 100644 --- a/apps/web/components/settings/legacy-executor-settings-route.test.tsx +++ b/apps/web/components/settings/legacy-executor-settings-route.test.tsx @@ -1,10 +1,12 @@ import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import { useEffect } from "react"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { StateProvider } from "@/components/state-provider"; +import { StateProvider, useAppStoreApi } from "@/components/state-provider"; import type { Executor, ExecutorProfile } from "@/lib/types/http"; import { LegacyExecutorSettingsRoute } from "./legacy-executor-settings-route"; +const LEGACY_EXECUTOR_TESTID = "legacy-executor"; const { replace } = vi.hoisted(() => ({ replace: vi.fn() })); vi.mock("@/lib/routing/client-router", () => ({ @@ -13,27 +15,25 @@ vi.mock("@/lib/routing/client-router", () => ({ vi.mock("@/app/settings/executor/[id]/page", () => ({ default: ({ executorId }: { executorId: string }) => ( -
{executorId}
- ), -})); - -vi.mock("@/app/settings/executor/[id]/profile/[profileId]/page", () => ({ - default: ({ profileId }: { profileId: string }) => ( -
{profileId}
+
{executorId}
), })); const TIMESTAMP = "2026-08-24T00:00:00Z"; +const PROFILE_ID = "profile/primary"; +const CANONICAL_PROFILE_PATH = "/settings/executors/profile%2Fprimary"; +const PROFILE_UNAVAILABLE_TESTID = "executor-profile-unavailable"; afterEach(() => { cleanup(); replace.mockReset(); + window.history.replaceState({}, "", "/settings"); }); function executor(type: Executor["type"]): Executor { const id = "executor/primary"; const profile: ExecutorProfile = { - id: "profile/primary", + id: PROFILE_ID, executor_id: id, executor_type: type, name: "Primary profile", @@ -57,11 +57,17 @@ function executor(type: Executor["type"]): Executor { }; } -function renderRoute(record: Executor, profileId?: string) { +function renderRoute( + record: Executor | null, + profileId?: string, + executorId?: string, + options: { executorsLoaded?: boolean } = {}, +) { render( - + , ); } +function HydrateExecutors({ record }: { record: Executor }) { + const store = useAppStoreApi(); + useEffect(() => { + store.getState().setExecutors([record]); + store.getState().setSettingsData({ executorsLoaded: true }); + }, [record, store]); + return null; +} + +it("waits for executor hydration before rendering an unavailable profile state", () => { + renderRoute(null, PROFILE_ID, "executor/missing", { executorsLoaded: false }); + + expect(screen.queryByTestId(PROFILE_UNAVAILABLE_TESTID)).toBeNull(); + expect(replace).not.toHaveBeenCalled(); +}); + describe("LegacyExecutorSettingsRoute", () => { it("redirects a member's bookmarked Kubernetes executor before mounting legacy controls", async () => { renderRoute(executor("k8s")); - await waitFor(() => - expect(replace).toHaveBeenCalledWith("/settings/executors/profile%2Fprimary"), - ); - expect(screen.queryByTestId("legacy-executor")).toBeNull(); + await waitFor(() => expect(replace).toHaveBeenCalledWith(CANONICAL_PROFILE_PATH)); + expect(screen.queryByTestId(LEGACY_EXECUTOR_TESTID)).toBeNull(); }); it("keeps an orphaned Kubernetes executor on the connection recovery page", async () => { @@ -97,32 +120,106 @@ describe("LegacyExecutorSettingsRoute", () => { await waitFor(() => expect(replace).toHaveBeenCalledWith("/settings/executors/k8s/executor%2Fprimary"), ); - expect(screen.queryByTestId("legacy-executor")).toBeNull(); + expect(screen.queryByTestId(LEGACY_EXECUTOR_TESTID)).toBeNull(); }); it("redirects a bookmarked Kubernetes profile to the Kubernetes-aware editor", async () => { - renderRoute(executor("k8s"), "profile/primary"); + renderRoute(executor("k8s"), PROFILE_ID); - await waitFor(() => - expect(replace).toHaveBeenCalledWith("/settings/executors/profile%2Fprimary"), - ); + await waitFor(() => expect(replace).toHaveBeenCalledWith(CANONICAL_PROFILE_PATH)); expect(screen.queryByTestId("legacy-profile")).toBeNull(); }); it("does not redirect a Kubernetes executor to a profile owned by another executor", () => { renderRoute(executor("k8s"), "profile/from-another-executor"); - expect(screen.getByTestId("legacy-profile")).toBeTruthy(); + expect(screen.getByTestId(PROFILE_UNAVAILABLE_TESTID)).toBeTruthy(); expect(replace).not.toHaveBeenCalled(); }); it.each([ - { profileId: undefined, testId: "legacy-executor" }, - { profileId: "profile/primary", testId: "legacy-profile" }, - ])("keeps SSH on its legacy scoped page", ({ profileId, testId }) => { - renderRoute(executor("ssh"), profileId); + "local", + "local_docker", + "ssh", + "sprites", + "local_pc", + "worktree", + "remote_docker", + ] as const)("redirects a %s profile bookmark to the canonical editor", async (type) => { + renderRoute(executor(type), PROFILE_ID); - expect(screen.getByTestId(testId)).toBeTruthy(); + await waitFor(() => expect(replace).toHaveBeenCalledWith(CANONICAL_PROFILE_PATH)); + expect(screen.queryByTestId(LEGACY_EXECUTOR_TESTID)).toBeNull(); + }); + + it("keeps an executor-only SSH route on the connection editor", () => { + renderRoute(executor("ssh")); + + expect(screen.getByTestId(LEGACY_EXECUTOR_TESTID)).toBeTruthy(); expect(replace).not.toHaveBeenCalled(); }); + + it("renders an unavailable state for a missing profile without choosing another profile", () => { + const record = executor("local_docker"); + record.profiles = [ + { + ...record.profiles![0], + id: "profile/other", + }, + ]; + + renderRoute(record, PROFILE_ID); + + expect(screen.getByTestId(PROFILE_UNAVAILABLE_TESTID)).toBeTruthy(); + expect(replace).not.toHaveBeenCalled(); + }); + + it("renders an unavailable state for a missing executor", () => { + renderRoute(null, PROFILE_ID, "executor/missing"); + + expect(screen.getByTestId(PROFILE_UNAVAILABLE_TESTID)).toBeTruthy(); + expect(replace).not.toHaveBeenCalled(); + }); + + it("preserves query parameters and fragments while replacing a valid bookmark", async () => { + window.history.replaceState( + {}, + "", + "/settings/executor/executor%2Fprimary/profile/profile%2Fprimary?tab=advanced#docker", + ); + renderRoute(executor("ssh"), PROFILE_ID); + + await waitFor(() => + expect(replace).toHaveBeenCalledWith( + "/settings/executors/profile%2Fprimary?tab=advanced#docker", + ), + ); + }); + + it("retries a valid bookmark when executor state hydrates after the route mounts", async () => { + const record = executor("ssh"); + render( + + + + , + ); + + await waitFor(() => expect(replace).toHaveBeenCalledWith(CANONICAL_PROFILE_PATH)); + }); }); diff --git a/apps/web/components/settings/legacy-executor-settings-route.tsx b/apps/web/components/settings/legacy-executor-settings-route.tsx index dd60ec490e..751f081163 100644 --- a/apps/web/components/settings/legacy-executor-settings-route.tsx +++ b/apps/web/components/settings/legacy-executor-settings-route.tsx @@ -1,13 +1,36 @@ "use client"; import ExecutorEditPage from "@/app/settings/executor/[id]/page"; -import ProfileDetailPage from "@/app/settings/executor/[id]/profile/[profileId]/page"; +import { Button } from "@kandev/ui/button"; +import { Card, CardContent } from "@kandev/ui/card"; import { useAppStore } from "@/components/state-provider"; +import { useRouter } from "@/lib/routing/client-router"; import { executorConnectionSettingsPath, executorProfileSettingsPath, } from "@/lib/settings/executor-settings-routes"; +import type { Executor } from "@/lib/types/http"; import { SettingsRedirect } from "@/src/settings-route-helpers"; +import { useTranslation } from "react-i18next"; + +const EXECUTORS_ROUTE = "/settings/executors"; + +function ProfileUnavailableState({ executor }: { executor: Executor | null }) { + const { t } = useTranslation(); + const router = useRouter(); + const destination = executor ? executorConnectionSettingsPath(executor) : EXECUTORS_ROUTE; + + return ( + + +

{t("executors:profileNotFound")}

+ + + + ); +} export function LegacyExecutorSettingsRoute({ executorId, @@ -19,23 +42,23 @@ export function LegacyExecutorSettingsRoute({ const executor = useAppStore( (state) => state.executors.items.find((item) => item.id === executorId) ?? null, ); + const executorsLoaded = useAppStore((state) => state.settingsData.executorsLoaded); + + if (profileId !== undefined) { + if (!executorsLoaded) return null; + const profile = executor?.profiles?.find( + (candidate) => candidate.id === profileId && candidate.executor_id === executorId, + ); + if (!executor || !profile) return ; + return ; + } if (executor?.type === "k8s") { - const destinationProfileId = profileId - ? executor.profiles?.find((profile) => profile.id === profileId)?.id - : executor.profiles?.[0]?.id; - if (profileId && !destinationProfileId) { - return ; - } - const destination = destinationProfileId - ? executorProfileSettingsPath(executor, destinationProfileId) + const destination = executor.profiles?.[0]?.id + ? executorProfileSettingsPath(executor.profiles[0].id) : executorConnectionSettingsPath(executor); return ; } - return profileId ? ( - - ) : ( - - ); + return ; } diff --git a/apps/web/components/task-create-dialog-form-body.tsx b/apps/web/components/task-create-dialog-form-body.tsx index cdd6bc2706..1349a7d192 100644 --- a/apps/web/components/task-create-dialog-form-body.tsx +++ b/apps/web/components/task-create-dialog-form-body.tsx @@ -18,6 +18,7 @@ import type { JiraTicket } from "@/lib/types/jira"; import type { LinearIssue } from "@/lib/types/linear"; import type { TaskCreateLaunchPreview } from "@/components/task-create-dialog-launch-preview"; import { RUNNER_INELIGIBLE_REASON_KEYS } from "@/components/task-create-dialog-helpers"; +import { executorProfileSettingsPath } from "@/lib/settings/executor-settings-routes"; import { useTranslation } from "react-i18next"; type SelectorOption = { @@ -89,7 +90,7 @@ type AgentColumnProps = Pick< >; function credentialsHref(executorProfileId: string): string { - return executorProfileId ? `/settings/executors/${executorProfileId}` : "/settings/executors"; + return executorProfileId ? executorProfileSettingsPath(executorProfileId) : "/settings/executors"; } function useExecutorTarget(executorProfileName: string | null): string { diff --git a/apps/web/components/task/executor-environment-disclosure.tsx b/apps/web/components/task/executor-environment-disclosure.tsx index 93a0c4165f..444198bcc0 100644 --- a/apps/web/components/task/executor-environment-disclosure.tsx +++ b/apps/web/components/task/executor-environment-disclosure.tsx @@ -93,10 +93,7 @@ function KubernetesActions({ touch ? "h-11 w-11" : "h-10 w-10", ); const settingsPath = env.executor_profile_id - ? executorProfileSettingsPath( - { id: env.executor_id, type: env.executor_type }, - env.executor_profile_id, - ) + ? executorProfileSettingsPath(env.executor_profile_id) : kubernetesExecutorSettingsPath(env.executor_id); return (
{ + test("converges desktop entry points on the complete Docker profile editor", async ({ + testPage, + apiClient, + }) => { + test.setTimeout(60_000); + const executor = await apiClient.createExecutor("E2E profile routing executor", "local_docker"); + const profile = await apiClient.createExecutorProfile(executor.id, { + name: "E2E profile routing Docker", + config: { dockerfile: DOCKERFILE, image_tag: INITIAL_IMAGE_TAG }, + prepare_script: "", + cleanup_script: "", + env_vars: [], + }); + + await testPage.route("**/api/v1/docker/build", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/x-ndjson", + body: JSON.stringify({ stream: "Successfully built\n" }) + "\n", + }); + }); + + try { + await testPage.goto("/settings/executors"); + await testPage.getByText(profile.name, { exact: true }).click(); + await expectDockerProfileEditor(testPage, profile.id); + + const buildResponse = waitForHttp(testPage, "POST", /^\/api\/v1\/docker\/build$/, { + predicate: (response) => response.ok(), + }); + await testPage.getByRole("button", { name: "Build Image" }).click(); + await buildResponse; + await expect(testPage.getByText("Success", { exact: true })).toBeVisible(); + + await testPage.locator("#image-tag").fill(EDITED_IMAGE_TAG); + const floatingSave = testPage.getByTestId("settings-floating-save"); + const saveResponse = waitForHttp( + testPage, + "PATCH", + /^\/api\/v1\/executors\/[^/]+\/profiles\/[^/]+$/, + { predicate: (response) => response.ok() }, + ); + await floatingSave.getByRole("button", { name: "Save changes" }).click(); + await saveResponse; + await expect(testPage.getByText("Profile saved")).toBeVisible(); + await expect + .poll( + async () => + (await apiClient.getExecutorProfile(executor.id, profile.id)).config?.image_tag, + ) + .toBe(EDITED_IMAGE_TAG); + + await testPage.reload(); + await expect(testPage.locator("#image-tag")).toHaveValue(EDITED_IMAGE_TAG); + + await setSettingsMenuMode(testPage, "accordion"); + const settingsTree = testPage.getByTestId("app-sidebar-settings-mode"); + const expandExecutors = settingsTree.getByRole("button", { name: /Expand Executors/i }); + if (await expandExecutors.count()) await expandExecutors.click(); + const expandExecutor = settingsTree.getByRole("button", { + name: `Expand ${executor.name}`, + }); + if (await expandExecutor.count()) await expandExecutor.click(); + const profileLink = settingsTree.locator(`a[href="${canonicalProfilePath(profile.id)}"]`); + await expect(profileLink).toBeVisible({ timeout: 10_000 }); + await profileLink.click(); + await expectDockerProfileEditor(testPage, profile.id); + + await testPage.goto(`/settings/executor/${encodeURIComponent(executor.id)}`); + await testPage.getByText(profile.name, { exact: true }).click(); + await expectDockerProfileEditor(testPage, profile.id); + + const legacyBookmark = + `/settings/executor/${encodeURIComponent(executor.id)}/profile/${encodeURIComponent(profile.id)}` + + "?tab=advanced#docker"; + await testPage.goto(legacyBookmark); + await expect(testPage).toHaveURL(`${canonicalProfilePath(profile.id)}?tab=advanced#docker`); + await expect(testPage.locator("#image-tag")).toHaveValue(EDITED_IMAGE_TAG); + await expect(testPage.getByRole("button", { name: "Build Image" })).toBeVisible(); + + await testPage.locator("#image-tag").fill(DRAFT_IMAGE_TAG); + const executorsLink = settingsTree.locator('a[href="/settings/executors"]'); + await executorsLink.click(); + const leaveDialog = testPage.getByRole("alertdialog"); + await expect(leaveDialog).toBeVisible(); + await leaveDialog.getByRole("button", { name: "Continue editing" }).click(); + await expect(testPage).toHaveURL(`${canonicalProfilePath(profile.id)}?tab=advanced#docker`); + await expect(testPage.locator("#image-tag")).toHaveValue(DRAFT_IMAGE_TAG); + + await executorsLink.click(); + await expect(leaveDialog).toBeVisible(); + await leaveDialog.getByRole("button", { name: "Discard and leave" }).click(); + await expect(testPage).toHaveURL(/\/settings\/executors$/); + + await testPage.goto(canonicalProfilePath(profile.id)); + await expect(testPage.locator("#image-tag")).toHaveValue(EDITED_IMAGE_TAG); + } finally { + await apiClient.deleteExecutor(executor.id).catch(() => {}); + } + }); +}); diff --git a/apps/web/e2e/tests/settings/mobile-executor-profile-routing.spec.ts b/apps/web/e2e/tests/settings/mobile-executor-profile-routing.spec.ts new file mode 100644 index 0000000000..8b485f7b72 --- /dev/null +++ b/apps/web/e2e/tests/settings/mobile-executor-profile-routing.spec.ts @@ -0,0 +1,98 @@ +import type { Locator } from "@playwright/test"; +import { expect, test } from "../../fixtures/test-base"; +import { waitForHttp } from "../../helpers/causal-waits"; +import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; + +const INITIAL_IMAGE_TAG = "kandev/e2e:mobile-routing-initial"; +const EDITED_IMAGE_TAG = "kandev/e2e:mobile-routing-edited"; +const DOCKERFILE = "FROM busybox\nWORKDIR /workspace\n"; + +function canonicalProfilePath(profileId: string): string { + return `/settings/executors/${encodeURIComponent(profileId)}`; +} + +async function expectTouchTarget(locator: Locator, label: string) { + const box = await locator.boundingBox(); + expect(box, `${label} must have geometry`).not.toBeNull(); + expect(box!.height, `${label} must be at least 44px tall`).toBeGreaterThanOrEqual(44); +} + +test("phone profile navigation reaches the complete Docker editor", async ({ + testPage, + apiClient, +}) => { + test.setTimeout(60_000); + const executor = await apiClient.createExecutor( + "E2E mobile profile routing executor", + "local_docker", + ); + const profile = await apiClient.createExecutorProfile(executor.id, { + name: "E2E mobile profile routing Docker", + config: { dockerfile: DOCKERFILE, image_tag: INITIAL_IMAGE_TAG }, + prepare_script: "", + cleanup_script: "", + env_vars: [], + }); + + await testPage.route("**/api/v1/docker/build", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/x-ndjson", + body: JSON.stringify({ stream: "Successfully built\n" }) + "\n", + }); + }); + + try { + await testPage.goto("/settings/executors"); + const profileCard = testPage.getByText(profile.name, { exact: true }); + await expect(profileCard).toBeVisible(); + await profileCard.tap(); + await expect(testPage).toHaveURL(canonicalProfilePath(profile.id)); + + await expect(testPage.locator("#image-tag")).toBeVisible(); + await expect(testPage.getByText("Dockerfile", { exact: true })).toBeVisible(); + const buildButton = testPage.getByRole("button", { name: "Build Image" }); + await expect(buildButton).toBeVisible(); + await expectTouchTarget(buildButton, "Mobile Docker build button"); + const buildResponse = waitForHttp(testPage, "POST", /^\/api\/v1\/docker\/build$/, { + predicate: (response) => response.ok(), + }); + await buildButton.tap(); + await buildResponse; + await expect(testPage.getByText("Success", { exact: true })).toBeVisible(); + + await testPage.locator("#image-tag").fill(EDITED_IMAGE_TAG); + const floatingSave = testPage.getByTestId("settings-floating-save"); + const saveButton = floatingSave.getByRole("button", { name: "Save changes" }); + await expectTouchTarget(saveButton, "Mobile profile save button"); + const saveResponse = waitForHttp( + testPage, + "PATCH", + /^\/api\/v1\/executors\/[^/]+\/profiles\/[^/]+$/, + { predicate: (response) => response.ok() }, + ); + await saveButton.tap(); + await saveResponse; + await expect(testPage.getByText("Profile saved")).toBeVisible(); + await expect + .poll( + async () => (await apiClient.getExecutorProfile(executor.id, profile.id)).config?.image_tag, + ) + .toBe(EDITED_IMAGE_TAG); + + await testPage.reload(); + await expect(testPage.locator("#image-tag")).toHaveValue(EDITED_IMAGE_TAG); + await assertNoDocumentHorizontalOverflow(testPage, "mobile canonical executor profile editor"); + + const legacyBookmark = + `/settings/executor/${encodeURIComponent(executor.id)}/profile/${encodeURIComponent(profile.id)}` + + "?tab=advanced#docker"; + await testPage.goto(legacyBookmark); + await expect(testPage).toHaveURL(`${canonicalProfilePath(profile.id)}?tab=advanced#docker`); + await expect(testPage.locator("#image-tag")).toHaveValue(EDITED_IMAGE_TAG); + await expect(testPage.getByRole("button", { name: "Build Image" })).toBeVisible(); + await assertNoDocumentHorizontalOverflow(testPage, "mobile legacy executor profile bookmark"); + } finally { + await apiClient.deleteExecutor(executor.id).catch(() => {}); + } +}); diff --git a/apps/web/lib/settings-discovery/catalog.test.ts b/apps/web/lib/settings-discovery/catalog.test.ts index b8f1427a35..e1ec8fcb65 100644 --- a/apps/web/lib/settings-discovery/catalog.test.ts +++ b/apps/web/lib/settings-discovery/catalog.test.ts @@ -197,9 +197,13 @@ describe("resolveSettingsDiscovery dynamic entries", () => { ], executors: [ { - type: "docker", + type: "local_docker", profiles: [{ id: "executor / one", name: "Docker Local" }], }, + { + type: "ssh", + profiles: [{ id: "ssh profile / two", name: "SSH Remote" }], + }, ], }); @@ -212,6 +216,9 @@ describe("resolveSettingsDiscovery dynamic entries", () => { expect(resolved.find((entry) => entry.id === "executor-profile:executor / one")?.href).toBe( "/settings/executors/executor%20%2F%20one", ); + expect(resolved.find((entry) => entry.id === "executor-profile:ssh profile / two")?.href).toBe( + "/settings/executors/ssh%20profile%20%2F%20two", + ); expect(resolved.find((entry) => entry.id === "workspace:workspace / one:name")?.href).toBe( "/settings/workspaces/workspace%20%2F%20one#setting-workspace-workspace%20%2F%20one-name", ); diff --git a/apps/web/lib/settings-discovery/resolve.ts b/apps/web/lib/settings-discovery/resolve.ts index 9c2865437f..308dda0f11 100644 --- a/apps/web/lib/settings-discovery/resolve.ts +++ b/apps/web/lib/settings-discovery/resolve.ts @@ -11,6 +11,7 @@ import { executorProfileDiscoveryTarget, workspaceDiscoveryTarget, } from "./dynamic-targets"; +import { executorProfileSettingsPath } from "../settings/executor-settings-routes"; function workspaceDefinitions( workspaces: SettingsDiscoveryContext["workspaces"], @@ -147,7 +148,7 @@ function executorProfileDefinitions( for (const [executorIndex, executor] of executors.entries()) { for (const [profileIndex, profile] of (executor.profiles ?? []).entries()) { const profileId = `executor-profile:${profile.id}`; - const href = `/settings/executors/${encodeURIComponent(profile.id)}`; + const href = executorProfileSettingsPath(profile.id); const order = 30_000 + executorIndex * 100 + profileIndex * 10; entries.push({ id: profileId, diff --git a/apps/web/lib/settings/executor-settings-routes.test.ts b/apps/web/lib/settings/executor-settings-routes.test.ts new file mode 100644 index 0000000000..0f5414ec57 --- /dev/null +++ b/apps/web/lib/settings/executor-settings-routes.test.ts @@ -0,0 +1,12 @@ +import { describe, expect, it } from "vitest"; + +import { executorProfileSettingsPath } from "./executor-settings-routes"; + +describe("executorProfileSettingsPath", () => { + // @covers AC-EXECUTORS-PROFILE-EDITOR-001.1 + it("routes every profile to the canonical editor", () => { + expect(executorProfileSettingsPath("profile/primary")).toBe( + "/settings/executors/profile%2Fprimary", + ); + }); +}); diff --git a/apps/web/lib/settings/executor-settings-routes.ts b/apps/web/lib/settings/executor-settings-routes.ts index 382f9efe06..f370a71582 100644 --- a/apps/web/lib/settings/executor-settings-routes.ts +++ b/apps/web/lib/settings/executor-settings-routes.ts @@ -13,11 +13,6 @@ export function executorConnectionSettingsPath(executor: ExecutorRouteIdentity): : `/settings/executor/${encodeURIComponent(executor.id)}`; } -export function executorProfileSettingsPath( - executor: ExecutorRouteIdentity, - profileId: string, -): string { - return executor.type === "k8s" - ? `/settings/executors/${encodeURIComponent(profileId)}` - : `${executorConnectionSettingsPath(executor)}/profile/${encodeURIComponent(profileId)}`; +export function executorProfileSettingsPath(profileId: string): string { + return `/settings/executors/${encodeURIComponent(profileId)}`; } diff --git a/docs/plans/executor-profile-editor-unification/plan.md b/docs/plans/executor-profile-editor-unification/plan.md new file mode 100644 index 0000000000..3b5cc98dea --- /dev/null +++ b/docs/plans/executor-profile-editor-unification/plan.md @@ -0,0 +1,226 @@ +--- +created: 2026-09-17 +status: complete +requirements: + - REQ-EXECUTORS-PROFILE-EDITOR-001 +system_design: + - ../../specs/executors/system-design/profile-editor.md +legacy_specs: [] +--- + +# Implementation Plan: Unified executor profile editor + +## Overview + +Repair [issue #3740](https://github.com/kdlbs/kandev/issues/3740) by routing all +profile entry points to the existing complete editor. One sequential work +order covers route generation, bookmark compatibility, and regression tests. + +The [requirement](../../specs/executors/requirements/profile-editor.md) and +[design](../../specs/executors/system-design/profile-editor.md) define the +implementation boundary. Task 01 is complete. + +## Evidence and root cause + +Investigation used commit `f32eb373781bcec12583c37e4a9d52b1ee12072f`, also the +local `origin/main` reference. The issue contained no comments or attachments. + +The hub hardcodes the plural route. `executorProfileSettingsPath` generates an +executor-scoped singular route for every type except `k8s`. +`renderExecutorSettingsRoute` then mounts `LegacyExecutorSettingsRoute`, which +renders a separate reduced form for non-Kubernetes profiles. + +A read-only Node execution of the actual helper produced: + +| Type | Destination for `exec-1` / `profile-1` | +| ---------------------------------------------------------- | --------------------------------------------- | +| local, worktree, local_docker, remote_docker, ssh, sprites | `/settings/executor/exec-1/profile/profile-1` | +| k8s | `/settings/executors/profile-1` | + +A source trace found `DockerSections`, `SSHAgentReadinessCard`, +`SSHTaskDirReclamationCard`, `KubernetesProfileSections`, `SpritesSections`, +and `McpPolicyCard` only in the complete editor. Existing component tests +explicitly expect the reduced SSH route, so passing tests currently preserve +the defect. + +Smallest user reproduction: open a saved Local Docker profile from the hub, +then from the desktop settings tree. Compare the URL and the Dockerfile, +image-tag, and build controls. This investigation used executable route and +source evidence, not a live browser reproduction. + +The issue's remote-Docker connection-card claim is not a requirement to invent +new controls. This repair preserves the complete editor's current capabilities +and existing executor connection pages. + +## Scope + +### In scope + +- Canonical profile URLs from every production entry point. +- Valid legacy bookmark replacement and invalid-pair recovery. +- One editor implementation with existing type-specific controls and save behavior. +- Desktop and phone regression coverage. + +### Out of scope + +- Runtime execution, new controls, connection redesign, backend changes, and migrations. +- Generic settings redesign or unrelated form cleanup. + +## Technical approach + +Change `executorProfileSettingsPath` to accept one profile ID. Migrate all +existing callers and hardcoded production profile destinations. Keep executor +connection helpers unchanged. + +Handle explicit profile IDs first in `LegacyExecutorSettingsRoute`. Validate +executor/profile ownership before redirecting. Preserve executor-only behavior +and the unavailable-profile state. Remove the reduced editor's form logic while +keeping the legacy route adapter for old bookmarks. + +Reuse the complete editor and `SettingsRedirect`. Preserve discovery fragments, +encoded identifiers, shared save coordination, and existing role gates. + +No existing executor requirement explicitly covers entry-point parity. The new +vertical requirement belongs to executors, not the reusable UI system. The +existing card-spacing contract remains a dependency. No new architectural +ownership boundary or persistence model requires an ADR. + +## ASCII UI preview + +### UI-01: Saved Docker profile, desktop + +Entry: hub card, settings tree, executor profile list, task disclosure, or bookmark. + +```text +BEFORE: sidebar AFTER: every entry point ++-----------------------+ +----------+---------------------------+ +| Profile details | | Settings | Profile name | +| Environment variables | | tree | Profile details | +| Prepare / cleanup | | | Dockerfile + image tag | +| No Docker build card | | | [Build Image] | ++-----------------------+ | | Applicable credentials | + | | Environment / scripts | + | | MCP policy | + +----------+---------------------------+ + [Save changes] when dirty +``` + +### UI-02: Saved Docker profile, phone + +Entry: Settings index > Executors > profile card, or a valid bookmark. + +```text ++-----------------------------+ +| Settings > Executors | +| Profile name | +| Profile details | +| Dockerfile + image tag | +| [Build Image] | +| Applicable credentials | +| Environment / scripts | +| MCP policy | +| | +| [Save changes] when dirty | ++-----------------------------+ +``` + +The phone opens a full settings destination without a desktop sidebar. Both +views retain one content scroll region and the shared floating save control. +Cards and labels are illustrative. Existing card order, type applicability, +localized copy, and spacing remain authoritative. Controls stay inside the +viewport, and the save control clears the safe area. + +### UI-03: Invalid legacy bookmark + +```text ++-----------------------------+ +| Profile not found | +| [Back to executor] | ++-----------------------------+ +``` + +Use the existing localized recovery control. Missing executors can recover to +the hub. No editor or save control mounts. Views map to AC-001.1 through +AC-001.6 under `AC-EXECUTORS-PROFILE-EDITOR`. + +## Tests + +The work order lists exact paths and commands. Required regression cases: + +| Criteria | Evidence | +| -------- | ------------------------------------------------------------------------- | +| .1, .7 | Helper unit tests, sidebar/profile-list tests, discovery and route tests | +| .2, .5 | Desktop Docker edit/build/save/reload plus existing executor-specific E2E | +| .3, .4 | Legacy route component tests for valid, missing, and mismatched pairs | +| .5 | Discard, unsaved-navigation guard, and unchanged role gates | +| .6 | Phone navigation, save/reload, and viewport containment | + +First failing regression: `routes every profile to the canonical editor` in +`apps/web/lib/settings/executor-settings-routes.test.ts`. It failed against the +old two-argument helper for non-Kubernetes profiles, then passed after the +helper and all production callers migrated to the canonical route. + +## E2E tests + +Add `settings/executor-profile-routing.spec.ts` for the `chromium` project. +Open one disposable Docker profile through hub, sidebar, profile list, and +legacy bookmark. Assert the same ID and Docker controls. Mock one successful +build, save an image-tag edit, reload, and verify persistence. Exercise discard +and unsaved navigation. Cover task-disclosure navigation with its existing +seed/store fixture pattern. + +Add `settings/mobile-executor-profile-routing.spec.ts` for `mobile-chrome`. +Tap the hub profile card, edit, save, reload, and open the legacy bookmark. +Assert complete controls, safe save reachability, and no horizontal overflow. +Use disposable records and clean them in `finally`. + +Existing Docker persistence, Kubernetes settings, SSH connection-link, and +executor-agent-config suites provide the type-specific and permission checks. +No Docker daemon is required for these settings tests. + +## Work orders + +- [x] [Task 01: Unify profile navigation and bookmark handling](task-01-unify-profile-editor.md) (done) + +## Verification results + +Design validation on 2026-09-17: + +- `python3 scripts/list-docs.py validate`: passed (285 decisions, 988 specifications). +- `python3 scripts/lint-spec-files.test.py`: passed (36 tests). +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check -- docs/specs docs/plans/executor-profile-editor-unification`: passed. +- `git status --short -- docs/plans/executor-profile-editor-unification`: both new plan files accounted for. +- Baseline Vitest run: 43 tests passed across `legacy-executor-settings-route.test.tsx`, + `executor-profiles-card.test.tsx`, and `settings-menu-branches.test.ts`. +- Read-only helper execution and section source trace reproduced the route split. +- Issue assignment: `carlosflorencio`. + +Implementation validation on 2026-09-17: + +- `pnpm exec vitest run lib/settings/executor-settings-routes.test.ts components/settings/legacy-executor-settings-route.test.tsx components/settings/executor-profiles-card.test.tsx components/settings/executor-profile-navigation.test.tsx components/app-sidebar/sections/settings/settings-menu-branches.test.ts src/settings-routes.test.ts src/settings-route-helpers.test.ts lib/settings-discovery/catalog.test.ts components/settings/settings-layout-client.test.tsx`: 9 files and 126 tests passed. +- `pnpm run typecheck`: passed. +- `pnpm run i18n:check`: passed; 8,397 referenced keys and complete required catalogs. +- `pnpm e2e:run --project chromium tests/settings/executor-profile-routing.spec.ts tests/settings/docker-profile-persistence.spec.ts tests/settings/kubernetes-executor.spec.ts tests/settings/ssh-profile-connection-link.spec.ts tests/settings/executor-agent-config.spec.ts`: 12 tests passed, including the production Vite build. +- `pnpm e2e:run --project mobile-chrome tests/settings/mobile-executor-profile-routing.spec.ts tests/settings/mobile-kubernetes-executor.spec.ts tests/settings/mobile-ssh-profile-connection-link.spec.ts tests/settings/mobile-executor-agent-config.spec.ts`: 6 tests passed, including the production Vite build. +- `node --test scripts/validate-public-docs.test.mjs`: 62 tests passed. +- `node scripts/validate-public-docs.mjs`: 46 published pages validated. +- `python3 scripts/list-docs.py validate`: passed for 285 decisions and 988 specifications. +- `python3 scripts/lint-spec-files.py --all`: passed. +- Explicit ESLint checks for every changed TypeScript/TSX file and Prettier checks for changed source/test files and plan Markdown: passed. +- `git diff --check`: passed. + +The reduced profile editor was removed. The legacy route now validates executor +ownership, preserves query/hash suffixes, and redirects valid bookmarks to the +canonical editor. No backend or persistence changes were required. + +## Risks + +- Legacy bookmark redirects validate executor/profile ownership before opening + the canonical editor. +- The canonical editor now owns the profile header, delete-return destination, + type-specific controls, and save coordination. +- The legacy adapter preserves the old route surface while invalid pairs render + localized recovery instead of mounting an editor. +- Role-gated controls, discovery fragments, and mobile safe-area behavior passed + the focused component and browser coverage. diff --git a/docs/plans/executor-profile-editor-unification/task-01-unify-profile-editor.md b/docs/plans/executor-profile-editor-unification/task-01-unify-profile-editor.md new file mode 100644 index 0000000000..dbf435a04e --- /dev/null +++ b/docs/plans/executor-profile-editor-unification/task-01-unify-profile-editor.md @@ -0,0 +1,187 @@ +--- +id: "01-unify-profile-editor" +title: "Unify profile navigation and bookmark handling" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-EXECUTORS-PROFILE-EDITOR-001 +acceptance_criteria: + - AC-EXECUTORS-PROFILE-EDITOR-001.1 + - AC-EXECUTORS-PROFILE-EDITOR-001.2 + - AC-EXECUTORS-PROFILE-EDITOR-001.3 + - AC-EXECUTORS-PROFILE-EDITOR-001.4 + - AC-EXECUTORS-PROFILE-EDITOR-001.5 + - AC-EXECUTORS-PROFILE-EDITOR-001.6 + - AC-EXECUTORS-PROFILE-EDITOR-001.7 +system_design: + - ../../specs/executors/system-design/profile-editor.md +--- + +# Task 01: Unify profile navigation and bookmark handling + +## Summary + +Make every profile entry point open the existing complete editor. Replace valid +legacy bookmarks without losing ownership checks, query parameters, or fragments. + +## In scope + +1. Add the failing canonical-route regression before production changes. +2. Reduce the profile helper to one encoded profile ID and migrate its callers. +3. Replace the reduced form with bookmark compatibility and localized recovery. +4. Add route, navigation, save, and phone regression coverage. +5. Update public executor documentation with one sentence about equivalent entry points. + +## Out of scope + +Runtime changes, data migrations, new profile fields, and connection-page redesign. + +## Acceptance + +- All production profile navigation uses the canonical helper. Exactly one editor owns profile form state and persistence. +- Legacy profile routes validate ownership, preserve suffixes, and replace history. Invalid pairs never mount an editor or mutate data. +- All referenced criteria pass their targeted checks, including desktop and phone save/reload, discard, and existing permissions. + +## Implementation sequence + +Start with the failing helper regression described in the plan. Extend +`LegacyExecutorSettingsRoute` tests with local Docker, SSH, Sprites, local, +worktree, remote Docker, and Kubernetes cases. Cover missing executors, missing +profiles, a foreign profile beside a valid profile, and later store hydration. +The foreign-profile case must not select the executor's first profile. + +Retain existing executor-only Kubernetes recovery and non-Kubernetes connection +behavior. Cover encoded IDs, query/hash preservation, and router replacement. + +Migrate the helper and hardcoded production links. Update obsolete assertions +that expect the singular profile route. Preserve tests for old bookmarked URLs. +Use `rg` to inventory callers and literals after the migration. + +Keep the plural editor as the form owner. Reduce the singular page to a small +adapter or unavailable state. Do not retain its save contributor or duplicate +field components. Do not remove an i18n guard entry to silence path checks. + +Add the two routing E2E files described in the plan. Reuse Docker response mocks +from `docker-profile-persistence.spec.ts` and mobile settings-index navigation. +Use actual clicks or taps for navigation evidence. Add +`components/settings/executor-profile-navigation.test.tsx` for hub and +task-disclosure navigation. Extend `lib/settings-discovery/catalog.test.ts` +for discovery destinations. + +The documentation change belongs in `docs/public/executors.md`, a reference +page. Preserve connection-page guidance. No new public page is required. + +## ASCII UI preview + +Excerpt from [UI-01 through UI-03](plan.md#ascii-ui-preview), covering criteria +.1 through .6: + +```text +UI-01 desktop: Settings tree | Profile > Details > Docker build > Policies +UI-02 phone: Settings > Executors > Profile + Details + Dockerfile + image tag + [Build Image] + Credentials / environment / scripts / MCP + [Save changes] when dirty +UI-03 invalid: Profile not found > recovery link +``` + +The phone uses direct navigation and one content scroll region. The existing +floating save control retains safe-area clearance. No desktop sidebar appears. +Use the full plan preview for structural requirements and type applicability. + +## Verification + +Run from the repository root. Install dependencies once if this is a fresh +worktree. Run desktop and mobile E2E commands sequentially. + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/web && pnpm exec vitest run lib/settings/executor-settings-routes.test.ts components/settings/legacy-executor-settings-route.test.tsx components/settings/executor-profiles-card.test.tsx components/settings/executor-profile-navigation.test.tsx components/app-sidebar/sections/settings/settings-menu-branches.test.ts src/settings-routes.test.ts src/settings-route-helpers.test.ts lib/settings-discovery/catalog.test.ts components/settings/settings-layout-client.test.tsx) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm run i18n:check) +(cd apps/web && pnpm e2e:run --project chromium tests/settings/executor-profile-routing.spec.ts tests/settings/docker-profile-persistence.spec.ts tests/settings/kubernetes-executor.spec.ts tests/settings/ssh-profile-connection-link.spec.ts tests/settings/executor-agent-config.spec.ts) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/settings/mobile-executor-profile-routing.spec.ts tests/settings/mobile-kubernetes-executor.spec.ts tests/settings/mobile-ssh-profile-connection-link.spec.ts tests/settings/mobile-executor-agent-config.spec.ts) +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Run ESLint on every changed TS/TSX file with `pnpm exec eslint --max-warnings 0` +from `apps/web`, using explicit paths from the final diff. Add exact commands +for any further changed test suite to Results before marking this task done. + +## Files likely touched + +All paths below are relative to `apps/web`, except the public document. + +- `lib/settings/executor-settings-routes.ts` and new `.test.ts`. +- `components/settings/legacy-executor-settings-route.tsx` and `.test.tsx`. +- `app/settings/executor/[id]/profile/[profileId]/page.tsx`. +- `app/settings/executors/page.tsx`. +- New `components/settings/executor-profile-navigation.test.tsx`. +- `app/settings/executors/new/[type]/page.tsx`. +- `app/settings/executors/new/[type]/ssh-create-page.tsx`. +- `app/settings/executors/new/[type]/kubernetes-create-page.tsx`. +- `app/settings/executors/k8s/[executorId]/page.tsx`. +- `components/settings/executor-profiles-card.tsx` and `.test.tsx`. +- `components/app-sidebar/sections/settings/settings-menu-branches.ts` and `.test.ts`. +- `components/task/executor-environment-disclosure.tsx`. +- `components/task-create-dialog-form-body.tsx`. +- `lib/settings-discovery/resolve.ts` and `catalog.test.ts`. +- `src/settings-routes.test.ts`, `src/settings-route-helpers.test.ts`. +- `components/settings/settings-layout-client.test.tsx`. +- New `e2e/tests/settings/executor-profile-routing.spec.ts` and `mobile-executor-profile-routing.spec.ts`. +- `docs/public/executors.md` (repository-relative). + +The canonical editor needs no form redesign. Change it only if a small shared +unavailable-state extraction is necessary. + +## Dependencies + +None. + +## Risks + +See the plan's ownership, compatibility, permissions, and test-entry-point risks. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/executors/requirements/profile-editor.md). +- [System design](../../specs/executors/system-design/profile-editor.md). +- Existing legacy route and profile-list component tests. +- Existing Docker persistence and mobile settings-sidebar E2E patterns. +- Repository `/tdd`, `/mobile-parity`, `/e2e`, and `/docs-maintainer` skills. + +## Results + +Implementation complete on 2026-09-17. + +- `executorProfileSettingsPath` now accepts one profile ID and all production + profile links use the encoded canonical `/settings/executors/:profileId` + route. +- The reduced singular profile editor was removed. `LegacyExecutorSettingsRoute` + retains executor-only behavior, validates legacy executor/profile ownership, + preserves query and hash suffixes, and renders localized recovery for invalid + pairs. +- Component coverage includes the RED-to-GREEN helper regression, legacy route + hydration and ownership cases, profile cards, settings tree, discovery, hub, + and task-disclosure navigation. +- Desktop routing coverage exercises hub, settings tree, executor profile list, + legacy bookmarks, Docker build/save/reload, and unsaved-change discard. +- Phone coverage exercises the complete Docker editor, touch-sized controls, + save/reload, legacy bookmarks, and horizontal-overflow protection. +- `pnpm exec vitest run lib/settings/executor-settings-routes.test.ts components/settings/legacy-executor-settings-route.test.tsx components/settings/executor-profiles-card.test.tsx components/settings/executor-profile-navigation.test.tsx components/app-sidebar/sections/settings/settings-menu-branches.test.ts src/settings-routes.test.ts src/settings-route-helpers.test.ts lib/settings-discovery/catalog.test.ts components/settings/settings-layout-client.test.tsx`: 9 files and 126 tests passed. +- `pnpm run typecheck`, `pnpm run i18n:check`, explicit changed-source ESLint + and Prettier checks, and `git diff --check`: passed. +- `pnpm e2e:run --project chromium tests/settings/executor-profile-routing.spec.ts tests/settings/docker-profile-persistence.spec.ts tests/settings/kubernetes-executor.spec.ts tests/settings/ssh-profile-connection-link.spec.ts tests/settings/executor-agent-config.spec.ts`: 12 tests passed. +- `pnpm e2e:run --project mobile-chrome tests/settings/mobile-executor-profile-routing.spec.ts tests/settings/mobile-kubernetes-executor.spec.ts tests/settings/mobile-ssh-profile-connection-link.spec.ts tests/settings/mobile-executor-agent-config.spec.ts`: 6 tests passed. +- `node --test scripts/validate-public-docs.test.mjs`, `node scripts/validate-public-docs.mjs`, `python3 scripts/list-docs.py validate`, and `python3 scripts/lint-spec-files.py --all`: passed. diff --git a/docs/public/executors.md b/docs/public/executors.md index 94ad2d63dc..c4e9476342 100644 --- a/docs/public/executors.md +++ b/docs/public/executors.md @@ -43,6 +43,8 @@ download requirements. Open **Settings > Executors**, then choose **Local**, **Worktree**, **Docker**, **Kubernetes**, **Sprites.dev**, or **SSH** under **Create New Profile**. Local and Worktree profiles already exist in a new database. +Open a saved profile from the Executors hub, the Settings tree, an executor profile list, Settings search, or a task's executor disclosure to use the same complete profile editor. + ![Settings > Executors showing existing Local, Worktree, and Sprites profiles plus Local, Worktree, Docker, Sprites.dev, and SSH profile creation options.](../screenshots/settings-executors.png)