From 5519f4b23b361faa1fb8556f8bcb7c06e56e2516 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 12:42:22 +0300 Subject: [PATCH] test(ai-chat): cover role-pick autoStart logic + the rolePickedNoSend reset (#149 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #156 (Request changes) flagged the new CLIENT logic as untested. Extract the decision logic from chat-thread.tsx into pure, unit-testable helpers and cover both branches the reviewer called out: - `roleLaunchMessage(role, default)` — the three-way handleRolePick behavior: autoStart=false -> null (send nothing); autoStart=true + custom -> trimmed message; autoStart=true + empty/null/whitespace -> default fallback. - `shouldResetRolePicked(chatId, roleId, flag)` — the #149 render-phase reset; the regression test asserts the stuck-flag case (New chat after an autoStart=false pick -> cards return) that the pre-fix code never handled, and that a still-bound role keeps the cards hidden. chat-thread.tsx now calls these helpers (behavior unchanged). 9 new pure tests. Also folded the review's cosmetic suggestion: `x ? x : null` -> `x || null` in ai-agent-roles.repo.ts (identical for string|null|undefined). Client tsc clean; role-launch + role-cards green; repo spec green. Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/components/chat-thread.tsx | 19 +++-- .../ai-chat/utils/role-launch.test.ts | 72 +++++++++++++++++++ .../src/features/ai-chat/utils/role-launch.ts | 34 +++++++++ .../ai-agent-roles/ai-agent-roles.repo.ts | 4 +- 4 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 apps/client/src/features/ai-chat/utils/role-launch.test.ts create mode 100644 apps/client/src/features/ai-chat/utils/role-launch.ts diff --git a/apps/client/src/features/ai-chat/components/chat-thread.tsx b/apps/client/src/features/ai-chat/components/chat-thread.tsx index 3d84cf3f..8cd69484 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.tsx @@ -21,6 +21,10 @@ import { IAiChatMessageRow, IAiRole, } from "@/features/ai-chat/types/ai-chat.types.ts"; +import { + roleLaunchMessage, + shouldResetRolePicked, +} from "@/features/ai-chat/utils/role-launch.ts"; import { describeChatError } from "@/features/ai-chat/utils/error-message.ts"; import { extractServerChatId } from "@/features/ai-chat/utils/adopt-chat-id.ts"; import { @@ -330,13 +334,14 @@ export default function ChatThread({ const handleRolePick = (role: IAiRole): void => { roleIdRef.current = role.id; onRolePicked?.(role); - if (role.autoStart) { - // Custom launch message when set; otherwise the built-in default text. - sendMessage({ - text: role.launchMessage?.trim() || t("Take a look at the current document"), - }); + const launch = roleLaunchMessage( + role, + t("Take a look at the current document"), + ); + if (launch !== null) { + sendMessage({ text: launch }); } else { - // Bind only: hide the cards and show the composer, send nothing. + // autoStart=false -> bind only: hide the cards, show the composer. setRolePickedNoSend(true); } }; @@ -348,7 +353,7 @@ export default function ChatThread({ // correctly stay hidden then. Render-phase reset (React "adjust state on prop // change"): one-shot — it re-renders with the flag false and the guard no longer // matches, so it cannot loop. (Review of #149.) - if (chatId === null && roleId == null && rolePickedNoSend) { + if (shouldResetRolePicked(chatId, roleId, rolePickedNoSend)) { setRolePickedNoSend(false); } const showRoleCards = diff --git a/apps/client/src/features/ai-chat/utils/role-launch.test.ts b/apps/client/src/features/ai-chat/utils/role-launch.test.ts new file mode 100644 index 00000000..22d36906 --- /dev/null +++ b/apps/client/src/features/ai-chat/utils/role-launch.test.ts @@ -0,0 +1,72 @@ +import { describe, it, expect } from "vitest"; +import { roleLaunchMessage, shouldResetRolePicked } from "./role-launch.ts"; + +const DEFAULT = "Take a look at the current document"; + +// Covers the three-way handleRolePick behavior (issue #149) without mounting the +// chat-thread component — the logic lives in these pure helpers. +describe("roleLaunchMessage", () => { + it("autoStart=true + custom launchMessage -> the trimmed custom text", () => { + expect( + roleLaunchMessage( + { autoStart: true, launchMessage: " Draft a plan " }, + DEFAULT, + ), + ).toBe("Draft a plan"); + }); + + it("autoStart=true + empty launchMessage -> the default fallback", () => { + expect( + roleLaunchMessage({ autoStart: true, launchMessage: "" }, DEFAULT), + ).toBe(DEFAULT); + }); + + it("autoStart=true + whitespace-only launchMessage -> the default fallback", () => { + expect( + roleLaunchMessage({ autoStart: true, launchMessage: " " }, DEFAULT), + ).toBe(DEFAULT); + }); + + it("autoStart=true + null launchMessage -> the default fallback", () => { + expect( + roleLaunchMessage({ autoStart: true, launchMessage: null }, DEFAULT), + ).toBe(DEFAULT); + }); + + it("autoStart=false -> null (bind only, send nothing) regardless of message", () => { + expect( + roleLaunchMessage( + { autoStart: false, launchMessage: "ignored" }, + DEFAULT, + ), + ).toBeNull(); + expect( + roleLaunchMessage({ autoStart: false, launchMessage: null }, DEFAULT), + ).toBeNull(); + }); +}); + +// Regression guard for #149: the "picked, not sent" flag must reset when the +// user starts a fresh chat after an autoStart=false pick. On pre-fix code there +// was no reset, so the flag stayed stuck and the role cards never returned — +// this is exactly the `true` case below (which the old code never acted on). +describe("shouldResetRolePicked", () => { + it("resets when the thread is empty and the bound role was cleared (New chat)", () => { + // chatId still null, roleId cleared by the parent, flag stuck -> reset. + expect(shouldResetRolePicked(null, null, true)).toBe(true); + expect(shouldResetRolePicked(null, undefined, true)).toBe(true); + }); + + it("does NOT reset while a role is still bound (cards stay hidden, composer shown)", () => { + // Right after the autoStart=false pick, roleId is the picked role -> keep hidden. + expect(shouldResetRolePicked(null, "role-1", true)).toBe(false); + }); + + it("does NOT reset once the chat exists (a message was sent / chat created)", () => { + expect(shouldResetRolePicked("chat-1", null, true)).toBe(false); + }); + + it("is a no-op when the flag is already false", () => { + expect(shouldResetRolePicked(null, null, false)).toBe(false); + }); +}); diff --git a/apps/client/src/features/ai-chat/utils/role-launch.ts b/apps/client/src/features/ai-chat/utils/role-launch.ts new file mode 100644 index 00000000..48ffdfa3 --- /dev/null +++ b/apps/client/src/features/ai-chat/utils/role-launch.ts @@ -0,0 +1,34 @@ +import type { IAiRole } from "@/features/ai-chat/types/ai-chat.types.ts"; + +/** + * Decide what (if anything) to auto-send when an agent role card is picked + * (issue #149). Extracted as a pure function so the three-way behavior is + * unit-testable without mounting the chat-thread component: + * - autoStart=false -> null (bind the role only, send nothing) + * - autoStart=true + message -> the trimmed custom launchMessage + * - autoStart=true + empty/null -> the default fallback text + */ +export function roleLaunchMessage( + role: Pick, + defaultText: string, +): string | null { + if (!role.autoStart) return null; + return role.launchMessage?.trim() || defaultText; +} + +/** + * Whether the "role picked but nothing sent yet" flag (`rolePickedNoSend`) + * should reset to false. After an autoStart=false pick the thread shows the + * composer with chatId still null; when the user then starts a fresh chat the + * parent clears the bound role (roleId -> null) but chatId stays null, so the + * thread never remounts and the flag would otherwise stay set — hiding the role + * cards forever. Reset exactly in that state; a still-bound role (roleId set) + * keeps the cards hidden. (Regression guard for #149.) + */ +export function shouldResetRolePicked( + chatId: string | null, + roleId: string | null | undefined, + rolePickedNoSend: boolean, +): boolean { + return chatId === null && roleId == null && rolePickedNoSend; +} diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts index b2fdd5a6..fb950585 100644 --- a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts @@ -96,7 +96,7 @@ export class AiAgentRoleRepo { enabled: values.enabled ?? true, autoStart: values.autoStart ?? true, // Empty string is treated as "no custom text" => null. - launchMessage: values.launchMessage ? values.launchMessage : null, + launchMessage: values.launchMessage || null, }) .returningAll() .executeTakeFirst(); @@ -133,7 +133,7 @@ export class AiAgentRoleRepo { if (patch.autoStart !== undefined) set.autoStart = patch.autoStart; if (patch.launchMessage !== undefined) { // Empty string clears to null (client default launch message). - set.launchMessage = patch.launchMessage ? patch.launchMessage : null; + set.launchMessage = patch.launchMessage || null; } await db .updateTable('aiAgentRoles')