From 9488bb0c0680f451690e317d943b7de7fce37827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= Date: Wed, 6 May 2026 02:21:32 +0100 Subject: [PATCH] fix(web): de-dup mobile terminal auto-create + add state-label tests + repo-tap toast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - use-mobile-terminals: move the auto-create-first-shell guard from per-instance refs to a module-level Set keyed by environmentId. MobileTerminalPane mounts useMobileTerminals three times for the same session (the pane itself, the picker pill via useActiveTerminalPillLabel, and the picker sheet content via MobileTerminalsList — Vaul keeps it mounted while closed). With per- instance guards, all three effects ran on first commit and each fired createUserShell, racing into up to three shells. The shared Set ensures only the first effect to observe an empty list creates, with retry-on-failure preserved (delete + next render re-checks). - mobile-repos-section: tapping a repo with no sessions now toasts a hint to open the session picker, instead of silently doing nothing. - state-labels: add tests covering known mappings, null/undefined fallbacks, and unknown-state passthrough for both formatters. --- .../task/mobile/mobile-repos-section.tsx | 15 ++++-- .../domains/session/use-mobile-terminals.ts | 36 ++++++++------ apps/web/lib/ui/state-labels.test.ts | 48 +++++++++++++++++++ 3 files changed, 81 insertions(+), 18 deletions(-) create mode 100644 apps/web/lib/ui/state-labels.test.ts diff --git a/apps/web/components/task/mobile/mobile-repos-section.tsx b/apps/web/components/task/mobile/mobile-repos-section.tsx index dbb2c7699f..ac00a5af94 100644 --- a/apps/web/components/task/mobile/mobile-repos-section.tsx +++ b/apps/web/components/task/mobile/mobile-repos-section.tsx @@ -3,6 +3,7 @@ import { memo, useCallback, useMemo } from "react"; import { IconCheck, IconFolder, IconGitBranch } from "@tabler/icons-react"; import { useAppStore } from "@/components/state-provider"; +import { useToast } from "@/components/toast-provider"; import type { Repository, TaskSession } from "@/lib/types/http"; import type { KanbanState } from "@/lib/state/slices"; @@ -141,6 +142,7 @@ export const MobileReposSection = memo(function MobileReposSection({ activeSessionId ? (s.taskSessions.items[activeSessionId]?.repository_id ?? null) : null, ); const rows = useTaskRepoRows(taskId, workspaceId); + const { toast } = useToast(); const handleSelect = useCallback( (row: RepoRow) => { @@ -148,12 +150,17 @@ export const MobileReposSection = memo(function MobileReposSection({ if (row.switchToSessionId) { setActiveSession(taskId, row.switchToSessionId); onClose(); + return; } - // Without an existing session on this repo, the user can switch to the - // Sessions tab and launch one — we don't auto-launch here to avoid - // surprise side effects from a tap that looked like navigation. + // We don't auto-launch from a repo tap — that would create a side effect + // for what looks like navigation. Surface a hint so the tap doesn't + // appear silently broken. + toast({ + title: "No session on this repo yet", + description: "Open the session picker to launch one.", + }); }, - [taskId, setActiveSession, onClose], + [taskId, setActiveSession, onClose, toast], ); if (!taskId) { diff --git a/apps/web/hooks/domains/session/use-mobile-terminals.ts b/apps/web/hooks/domains/session/use-mobile-terminals.ts index 7cb9eb6114..95cec55d85 100644 --- a/apps/web/hooks/domains/session/use-mobile-terminals.ts +++ b/apps/web/hooks/domains/session/use-mobile-terminals.ts @@ -5,6 +5,17 @@ import { useTerminals } from "./use-terminals"; import { useUserShells } from "./use-user-shells"; import { useAppStore } from "@/components/state-provider"; +/** + * Module-level guard for the auto-create-first-shell effect, keyed by + * environmentId. `useMobileTerminals` is called from several places that all + * end up rendering the same terminal pane (the pane itself, the picker pill, + * the picker sheet content) — a per-instance ref guard would let each one + * fire `createUserShell` on first mount, racing into multiple shells. The + * Set is shared across instances so only the first effect to run for a given + * environment triggers creation. + */ +const autoCreatedEnvironments = new Set(); + /** * Mobile wrapper around `useTerminals`. Auto-creates a first shell when the * server-side shell list loads empty so the user always sees one terminal @@ -15,33 +26,30 @@ export function useMobileTerminals(sessionId: string | null) { sessionId ? (s.environmentIdBySessionId[sessionId] ?? null) : null, ); const result = useTerminals({ sessionId, environmentId }); - // Read user-shell loaded flag directly so the auto-create trigger has a - // primitive dependency (depending on the result object would re-run the - // effect every render and continuously cancel the timer). + // Read user-shell loaded flag directly so the auto-create trigger has + // primitive dependencies — depending on `result` would re-run on every + // render and would also race against React's effect ordering. const { isLoaded: shellsLoaded, shells } = useUserShells(environmentId); const addTerminalRef = useRef(result.addTerminal); useEffect(() => { addTerminalRef.current = result.addTerminal; }, [result.addTerminal]); - const autoCreatedRef = useRef(null); useEffect(() => { if (!environmentId || !shellsLoaded) return; - if (autoCreatedRef.current === environmentId) return; + if (autoCreatedEnvironments.has(environmentId)) return; if (shells.length > 0) { - autoCreatedRef.current = environmentId; + autoCreatedEnvironments.add(environmentId); return; } - autoCreatedRef.current = environmentId; + autoCreatedEnvironments.add(environmentId); // Reset the guard if creation fails so the user gets a retry on the next - // render cycle (e.g. after the WS reconnects). addTerminal returns void + // render cycle (e.g. after the WS reconnects). `addTerminal` returns void // but its inner promise can still reject; guard defensively. - const result = addTerminalRef.current() as unknown; - if (result && typeof (result as Promise).catch === "function") { - (result as Promise).catch(() => { - if (autoCreatedRef.current === environmentId) { - autoCreatedRef.current = null; - } + const promise = addTerminalRef.current() as unknown; + if (promise && typeof (promise as Promise).catch === "function") { + (promise as Promise).catch(() => { + autoCreatedEnvironments.delete(environmentId); }); } }, [environmentId, shellsLoaded, shells.length]); diff --git a/apps/web/lib/ui/state-labels.test.ts b/apps/web/lib/ui/state-labels.test.ts new file mode 100644 index 0000000000..503b30824a --- /dev/null +++ b/apps/web/lib/ui/state-labels.test.ts @@ -0,0 +1,48 @@ +import { describe, it, expect } from "vitest"; +import { formatTaskStateLabel, formatTaskSessionStateLabel } from "./state-labels"; +import type { TaskState, TaskSessionState } from "@/lib/types/http"; + +describe("formatTaskStateLabel", () => { + it("maps known task states to human labels", () => { + expect(formatTaskStateLabel("IN_PROGRESS")).toBe("In progress"); + expect(formatTaskStateLabel("WAITING_FOR_INPUT")).toBe("Waiting for input"); + expect(formatTaskStateLabel("TODO")).toBe("To do"); + expect(formatTaskStateLabel("COMPLETED")).toBe("Completed"); + expect(formatTaskStateLabel("FAILED")).toBe("Failed"); + expect(formatTaskStateLabel("CANCELLED")).toBe("Cancelled"); + expect(formatTaskStateLabel("BLOCKED")).toBe("Blocked"); + expect(formatTaskStateLabel("REVIEW")).toBe("Review"); + expect(formatTaskStateLabel("CREATED")).toBe("Created"); + expect(formatTaskStateLabel("SCHEDULING")).toBe("Scheduling"); + }); + + it("returns 'Not started' for null/undefined", () => { + expect(formatTaskStateLabel(null)).toBe("Not started"); + expect(formatTaskStateLabel(undefined)).toBe("Not started"); + }); + + it("falls back to the raw value for unknown states", () => { + expect(formatTaskStateLabel("UNKNOWN_FUTURE" as TaskState)).toBe("UNKNOWN_FUTURE"); + }); +}); + +describe("formatTaskSessionStateLabel", () => { + it("maps known session states", () => { + expect(formatTaskSessionStateLabel("RUNNING")).toBe("Running"); + expect(formatTaskSessionStateLabel("STARTING")).toBe("Starting"); + expect(formatTaskSessionStateLabel("WAITING_FOR_INPUT")).toBe("Waiting for input"); + expect(formatTaskSessionStateLabel("COMPLETED")).toBe("Completed"); + expect(formatTaskSessionStateLabel("FAILED")).toBe("Failed"); + expect(formatTaskSessionStateLabel("CANCELLED")).toBe("Cancelled"); + expect(formatTaskSessionStateLabel("CREATED")).toBe("Created"); + }); + + it("returns empty string for null/undefined", () => { + expect(formatTaskSessionStateLabel(null)).toBe(""); + expect(formatTaskSessionStateLabel(undefined)).toBe(""); + }); + + it("falls back to the raw value for unknown states", () => { + expect(formatTaskSessionStateLabel("UNKNOWN" as TaskSessionState)).toBe("UNKNOWN"); + }); +});