diff --git a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/UsageView.tsx b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/UsageView.tsx index 36590fc9655..7e782da39c3 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/UsageView.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/UsageView.tsx @@ -6,7 +6,6 @@ import { Button } from "@superset/ui/button"; import { DropdownMenu, DropdownMenuContent, - DropdownMenuItem, DropdownMenuTrigger, } from "@superset/ui/dropdown-menu"; import { toast } from "@superset/ui/sonner"; @@ -43,6 +42,7 @@ import { useRestartAgentSessions } from "../../hooks/useRestartAgentSessions"; import { useSetDefaultUsageAccount } from "../../hooks/useSetDefaultUsageAccount"; import { LeaderboardCard } from "../LeaderboardCard"; import { UsageHistorySection } from "../UsageHistorySection"; +import { AccountMenuItems } from "./components/AccountMenuItems"; import type { SwitchSignInTarget } from "./components/AddAccountDialog"; import { AddAccountDialog } from "./components/AddAccountDialog"; import { RemoveAccountDialog } from "./components/RemoveAccountDialog"; @@ -243,16 +243,11 @@ function AccountCard({ - {onSwitchSignIn && ( - - Switch sign-in… - - )} - {onRemove && ( - - Remove… - - )} + )} diff --git a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/AccountMenuItems.test.tsx b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/AccountMenuItems.test.tsx new file mode 100644 index 00000000000..ee4b438fdf5 --- /dev/null +++ b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/AccountMenuItems.test.tsx @@ -0,0 +1,78 @@ +import { afterEach, describe, expect, mock, test } from "bun:test"; + +( + globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } +).IS_REACT_ACT_ENVIRONMENT = true; + +const { act, cleanup, fireEvent, render, within } = await import( + "@testing-library/react" +); +const { DropdownMenu, DropdownMenuContent } = await import( + "@superset/ui/dropdown-menu" +); +const { AccountMenuItems } = await import("./AccountMenuItems"); + +afterEach(cleanup); + +async function menu({ + createdByParadev, + removable = true, +}: { + createdByParadev: boolean; + removable?: boolean; +}) { + const onRemove = mock(() => {}); + let view!: ReturnType; + await act(async () => { + view = render( + + + {}} + onRemove={removable ? onRemove : null} + createdByParadev={createdByParadev} + /> + + , + ); + }); + const ui = within(view.baseElement as HTMLElement); + const removeItem = () => + ui.queryByText("Remove…")?.closest("[role=menuitem]") ?? null; + return { ui, onRemove, removeItem }; +} + +describe("a profile Paradev created", () => { + test("can be removed", async () => { + const { onRemove, removeItem } = await menu({ createdByParadev: true }); + const item = removeItem(); + expect(item?.getAttribute("data-disabled")).toBeNull(); + await act(async () => { + fireEvent.click(item as Element); + }); + expect(onRemove).toHaveBeenCalledTimes(1); + }); +}); + +// A profile from an earlier build, a dir the user made, or a host too old +// to say: Remove would delete nothing, so the menu says why up front. +describe("a profile Paradev did not create", () => { + test("lists Remove as unavailable, with the reason", async () => { + const { ui, removeItem } = await menu({ createdByParadev: false }); + expect(removeItem()?.getAttribute("data-disabled")).not.toBeNull(); + expect( + ui.getByText("Paradev only removes profiles it created."), + ).toBeDefined(); + }); +}); + +describe("the system-default login", () => { + test("has no Remove", async () => { + const { ui, removeItem } = await menu({ + createdByParadev: false, + removable: false, + }); + expect(removeItem()).toBeNull(); + expect(ui.getByText("Switch sign-in…")).toBeDefined(); + }); +}); diff --git a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/AccountMenuItems.tsx b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/AccountMenuItems.tsx new file mode 100644 index 00000000000..c72bf8b77a0 --- /dev/null +++ b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/AccountMenuItems.tsx @@ -0,0 +1,43 @@ +import { Trans } from "@lingui/react/macro"; +import { DropdownMenuItem } from "@superset/ui/dropdown-menu"; + +interface AccountMenuItemsProps { + onSwitchSignIn: (() => void) | null; + /** Null for the system-default login, which is never removable. */ + onRemove: (() => void) | null; + /** Whether the host reports that Paradev created the profile dir. Remove + * deletes no other dir. */ + createdByParadev: boolean; +} + +/** The items of an account card's "…" menu. */ +export function AccountMenuItems({ + onSwitchSignIn, + onRemove, + createdByParadev, +}: AccountMenuItemsProps) { + return ( + <> + {onSwitchSignIn && ( + + Switch sign-in… + + )} + {onRemove && + (createdByParadev ? ( + + Remove… + + ) : ( + + + Remove… + + + Paradev only removes profiles it created. + + + ))} + + ); +} diff --git a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/index.ts b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/index.ts new file mode 100644 index 00000000000..2b67cc98624 --- /dev/null +++ b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AccountMenuItems/index.ts @@ -0,0 +1 @@ +export { AccountMenuItems } from "./AccountMenuItems"; diff --git a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AddAccountDialog/AddAccountDialog.test.tsx b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AddAccountDialog/AddAccountDialog.test.tsx new file mode 100644 index 00000000000..3110d0ab1c7 --- /dev/null +++ b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AddAccountDialog/AddAccountDialog.test.tsx @@ -0,0 +1,337 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { + act, + cleanup, + fireEvent, + render, + waitFor, +} from "@testing-library/react"; +import { initTRPC } from "@trpc/server"; +import { fetchRequestHandler } from "@trpc/server/adapters/fetch"; +import type { ContextType } from "react"; +import { LocalHostServiceContext } from "renderer/routes/_authenticated/providers/LocalHostServiceProvider"; +import superjson from "superjson"; +import { AddAccountDialog } from "./AddAccountDialog"; + +( + globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } +).IS_REACT_ACT_ENVIRONMENT = true; + +const HOME = "/home/test"; +const RUN_HINT = "Run in a terminal on this host:"; +const SIGNED_IN = "is signed in."; + +interface CodexLogin { + home: string; + fingerprint: string | null; + credentialKind: "subscription"; + /** Left out to stand for a host older than the field. */ + createdByParadev?: boolean; +} + +const AMBIENT: CodexLogin = { + home: `${HOME}/.codex`, + fingerprint: null, + credentialKind: "subscription", + createdByParadev: false, +}; + +function codexLogin( + name: string, + fingerprint: string, + createdByParadev?: boolean, +): CodexLogin { + return { + home: `${HOME}/.codex-${name}`, + fingerprint, + credentialKind: "subscription", + ...(createdByParadev === undefined ? {} : { createdByParadev }), + }; +} + +function logins(...codex: CodexLogin[]) { + return { + homeDir: HOME, + claudeDefaultFingerprint: null, + claudeDefaultEmail: null, + claude: [], + codex: [AMBIENT, ...codex], + }; +} + +let stopHost: (() => void) | undefined; + +afterEach(() => { + cleanup(); + stopHost?.(); + stopHost = undefined; +}); + +/** A host whose usage.logins answer the test sets, and can hold back. */ +function startHost() { + let release = () => {}; + const host = { + logins: logins(), + held: null as Promise | null, + prepared: [] as unknown[], + url: "", + hold() { + host.held = new Promise((resolve) => { + release = resolve; + }); + }, + release() { + host.held = null; + release(); + }, + }; + const t = initTRPC.create({ transformer: superjson }); + const router = t.router({ + usage: t.router({ + logins: t.procedure.query(async () => { + await host.held; + return host.logins; + }), + prepareAccount: t.procedure + .input((input) => input) + .mutation(({ input }) => { + host.prepared.push(input); + return { success: true as const }; + }), + }), + }); + const server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch: (request) => + fetchRequestHandler({ + endpoint: "/trpc", + req: request, + router, + allowMethodOverride: true, + }), + }); + stopHost = () => void server.stop(true); + host.url = `http://127.0.0.1:${server.port}`; + return host; +} +type Host = ReturnType; + +/** + * Opens the Codex add dialog. A remote host shows the command to copy; the + * local host runs it behind Sign in. + */ +function mount(host: Host, { local = false } = {}) { + const queryClient = new QueryClient(); + let added = 0; + const ui = (open: boolean) => ( + + > + } + > + {}} + hostUrl={host.url} + agent="codex" + onAccountAdded={() => { + added++; + }} + onDefaultSwitched={() => {}} + /> + + + ); + const view = render(ui(true)); + return { + view, + added: () => added, + setOpen: (open: boolean) => act(() => view.rerender(ui(open))), + typeName: (name: string) => + act(() => + fireEvent.change(view.getByLabelText("Profile name"), { + target: { value: name }, + }), + ), + /** One logins poll, answered and rendered. */ + poll: () => + act(async () => { + await queryClient.refetchQueries({ + queryKey: ["host-usage-logins"], + type: "active", + }); + await new Promise((resolve) => setTimeout(resolve, 0)); + }), + }; +} + +describe("adding a profile on a remote host", () => { + test("shows the command once logins have loaded, for a usable name whose home is free", async () => { + const host = startHost(); + host.logins = logins(codexLogin("personal", "a")); + host.hold(); + const dialog = mount(host); + + await dialog.typeName("work"); + expect(dialog.view.queryByText(RUN_HINT)).toBeNull(); + + host.release(); + await dialog.poll(); + expect(await dialog.view.findByText(RUN_HINT)).toBeDefined(); + + await dialog.typeName("personal"); + expect( + dialog.view.getByText(/already exists\. Choose another name\./), + ).toBeDefined(); + expect(dialog.view.queryByText(RUN_HINT)).toBeNull(); + + await dialog.typeName("仕事"); + expect( + dialog.view.getByText("Use letters A–Z, digits, or hyphens."), + ).toBeDefined(); + expect(dialog.view.queryByText(RUN_HINT)).toBeNull(); + + await dialog.typeName(""); + expect(dialog.view.queryByText(RUN_HINT)).toBeNull(); + }); + + test("finds and prepares the new profile's first sign-in", async () => { + const host = startHost(); + const dialog = mount(host); + await dialog.poll(); + await dialog.typeName("work"); + await dialog.poll(); + + host.logins = logins(codexLogin("work", "a", true)); + await dialog.poll(); + + expect(await dialog.view.findByText(SIGNED_IN)).toBeDefined(); + expect(dialog.added()).toBe(1); + await waitFor(() => + expect(host.prepared).toEqual([ + { agent: "codex", selection: `${HOME}/.codex-work` }, + ]), + ); + }); + + test("after Remove, adds the same name again in one visit", async () => { + const host = startHost(); + const dialog = mount(host); + await dialog.poll(); + await dialog.typeName("work"); + await dialog.poll(); + host.logins = logins(codexLogin("work", "a", true)); + await dialog.poll(); + expect(await dialog.view.findByText(SIGNED_IN)).toBeDefined(); + + await dialog.setOpen(false); + host.logins = logins(); + await dialog.setOpen(true); + await dialog.poll(); + await dialog.typeName("work"); + await dialog.poll(); + expect(await dialog.view.findByText(RUN_HINT)).toBeDefined(); + host.logins = logins(codexLogin("work", "b", true)); + await dialog.poll(); + + expect(await dialog.view.findByText(SIGNED_IN)).toBeDefined(); + expect(dialog.added()).toBe(2); + await waitFor(() => expect(host.prepared).toHaveLength(2)); + }); + + // An older host does not say who created a dir, so only a baseline from + // this opening keeps the dialog from taking that login as the new one. + test("does not take a login made while the dialog was closed as the new profile", async () => { + const host = startHost(); + const dialog = mount(host); + await dialog.poll(); + await dialog.setOpen(false); + host.logins = logins(codexLogin("other", "x")); + + await dialog.setOpen(true); + await dialog.typeName("other"); + await dialog.poll(); + await dialog.poll(); + + expect( + dialog.view.getByText(/already exists\. Choose another name\./), + ).toBeDefined(); + expect(dialog.view.queryByText(SIGNED_IN)).toBeNull(); + expect(dialog.added()).toBe(0); + expect(host.prepared).toEqual([]); + }); + + test("does not take a login in a dir its command did not create as the new profile", async () => { + const host = startHost(); + const dialog = mount(host); + await dialog.poll(); + await dialog.typeName("team"); + await dialog.poll(); + + host.logins = logins(codexLogin("team", "x", false)); + await dialog.poll(); + + expect(dialog.view.queryByText(SIGNED_IN)).toBeNull(); + expect(dialog.added()).toBe(0); + expect(host.prepared).toEqual([]); + }); + + test("does not take a token refresh in a home that was already signed in as the new profile", async () => { + const host = startHost(); + host.logins = logins(codexLogin("work", "a", true)); + const dialog = mount(host); + await dialog.poll(); + await dialog.typeName("work"); + await dialog.poll(); + + host.logins = logins(codexLogin("work", "b", true)); + await dialog.poll(); + + expect(dialog.view.queryByText(SIGNED_IN)).toBeNull(); + expect(dialog.added()).toBe(0); + expect(host.prepared).toEqual([]); + }); + + test("starts each opening with an empty name", async () => { + const host = startHost(); + const dialog = mount(host); + await dialog.poll(); + await dialog.typeName("work"); + + await dialog.setOpen(false); + await dialog.setOpen(true); + + expect( + (dialog.view.getByLabelText("Profile name") as HTMLInputElement).value, + ).toBe(""); + }); +}); + +describe("adding a profile on this machine", () => { + test("keeps Sign in off until logins have loaded and the name's home is free", async () => { + const host = startHost(); + host.logins = logins(codexLogin("personal", "a")); + host.hold(); + const dialog = mount(host, { local: true }); + const signIn = () => + dialog.view.getByRole("button", { name: "Sign in" }) as HTMLButtonElement; + + await dialog.typeName("work"); + expect(signIn().disabled).toBe(true); + + host.release(); + await dialog.poll(); + await waitFor(() => expect(signIn().disabled).toBe(false)); + + await dialog.typeName("personal"); + expect(signIn().disabled).toBe(true); + + await dialog.typeName(""); + expect(signIn().disabled).toBe(true); + }); +}); diff --git a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AddAccountDialog/AddAccountDialog.tsx b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AddAccountDialog/AddAccountDialog.tsx index 76a9dfe0c5a..49ba555f16b 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AddAccountDialog/AddAccountDialog.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/settings/usage/components/UsageView/components/AddAccountDialog/AddAccountDialog.tsx @@ -34,6 +34,11 @@ import { } from "../../utils/findCompletedLogin"; import { switchSignInCommand } from "../../utils/switchSignInCommand"; import type { ManagedAgent } from "../../utils/visibleQuotaAgents"; +import { + isProfileHomeTaken, + profileHome, + slugifyProfileName, +} from "./utils/profileName"; type Agent = ManagedAgent; @@ -52,15 +57,6 @@ export interface SwitchSignInTarget { label: string; } -function slugify(name: string): string { - return ( - name - .toLowerCase() - .replace(/[^a-z0-9-]+/g, "-") - .replace(/^-+|-+$/g, "") || "work" - ); -} - interface AddAccountDialogProps { open: boolean; onOpenChange: (open: boolean) => void; @@ -104,22 +100,42 @@ export function AddAccountDialog({ const agent = switchTarget?.agent ?? addAgent; const billingId = useId(); const nameId = useId(); - const [name, setName] = useState("work"); + const nameErrorId = useId(); + const [name, setName] = useState(""); const [credentialKind, setCredentialKind] = useState("subscription"); const [copied, setCopied] = useState(false); const [found, setFound] = useState(null); const baselineRef = useRef(null); - const slug = slugify(name); + const slug = slugifyProfileName(name); + const profileDir = `~/.${agent}-${slug}`; - const loginsQuery = useHostUsageLogins(hostUrl, open && !found); + // Each opening polls its own logins query, so a result cached by an + // earlier opening never becomes this one's baseline. + const [opening, setOpening] = useState(0); + const [wasOpen, setWasOpen] = useState(open); + if (open !== wasOpen) { + setWasOpen(open); + if (open) setOpening((count) => count + 1); + } + const loginsQuery = useHostUsageLogins(hostUrl, open && !found, opening); const setDefault = useSetDefaultUsageAccount(hostUrl); + const homeTaken = + !switchTarget && + !!loginsQuery.data && + isProfileHomeTaken(loginsQuery.data, agent, slug); + const nameUnusable = !switchTarget && name.trim() !== "" && slug === ""; + // A new profile's command waits for this opening's logins, so it is never + // offered for a home that is already signed in. + const canSignIn = + !!switchTarget || (!!loginsQuery.data && slug !== "" && !homeTaken); // The first poll result after opening is the baseline; anything beyond it // is the sign-in we are waiting for. useEffect(() => { if (!open) { baselineRef.current = null; + setName(""); setFound(null); setCopied(false); setTerminalAttempt(0); @@ -135,6 +151,12 @@ export function AddAccountDialog({ return; } if (found || (terminalAttempt > 0 && !commandSucceeded)) return; + if (!switchTarget && !isProfileHomeTaken(logins, agent, slug)) { + // Nothing is signed in at the new profile's dir yet, so a login + // that lands there from now on is the one being added. + baselineRef.current = logins; + return; + } const provisionProfile = (selection: string) => { // Shares the default account's skills, plugins, MCP servers, and @@ -149,7 +171,7 @@ export function AddAccountDialog({ const selection = switchTarget ? switchTarget.selection - : `${logins.homeDir}/.${agent}-${slug}`; + : profileHome(logins.homeDir, agent, slug); const fresh = findCompletedLogin({ agent, credentialKind: switchTarget?.credentialKind ?? credentialKind, @@ -157,6 +179,7 @@ export function AddAccountDialog({ baseline: baselineRef.current, current: logins, commandSucceeded, + newProfile: !switchTarget, }); if (fresh) { setFound(fresh); @@ -179,7 +202,9 @@ export function AddAccountDialog({ const command = switchTarget ? switchSignInCommand(switchTarget) - : addAccountCommand(agent, slug, credentialKind); + : slug + ? addAccountCommand(agent, slug, credentialKind) + : ""; // Codex's API login takes the key at a terminal prompt; every other flow // finishes in the browser. const promptsForKey = @@ -357,8 +382,26 @@ export function AddAccountDialog({ onChange={(event) => setName(event.target.value)} className="h-9 min-w-0 flex-1 text-sm" placeholder="work" + aria-invalid={homeTaken || nameUnusable || undefined} + aria-describedby={ + homeTaken || nameUnusable ? nameErrorId : undefined + } /> + {(homeTaken || nameUnusable) && ( +

+ {homeTaken ? ( + + {profileDir} already exists. Choose another name. + + ) : ( + Use letters A–Z, digits, or hyphens. + )} +

+ )}
Billing @@ -397,7 +440,7 @@ export function AddAccountDialog({
)} - {!canRunLocally && commandDetails} + {!canRunLocally && canSignIn && commandDetails} {canRunLocally && open && terminalAttempt > 0 && (
@@ -432,7 +475,7 @@ export function AddAccountDialog({
)} {terminalAttempt === 0 && - (!canRunLocally || loginsQuery.isError) && + ((!canRunLocally && canSignIn) || loginsQuery.isError) && loginStatus}
@@ -471,7 +514,7 @@ export function AddAccountDialog({ {canRunLocally && !terminalRunning && (