From 49e2e2e27f2181a37ab33001b200b4bb0262fe75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 10 Jul 2026 17:52:09 +0100 Subject: [PATCH] fix(office): stop unstable use(params) promise from hiding office tree (#1664) --- .../task/chat/messages/chat-message.test.tsx | 40 +++++++++- .../task/chat/messages/chat-message.tsx | 62 +++++++++++++-- .../chat/chat-prompt-mention-hover.spec.ts | 79 +++++++++++++++++++ .../e2e/tests/office/agent-dashboard.spec.ts | 16 ++++ apps/web/src/office-routes.test.ts | 48 ++++++++++- apps/web/src/office-routes.tsx | 38 ++++++++- 6 files changed, 268 insertions(+), 15 deletions(-) create mode 100644 apps/web/e2e/tests/chat/chat-prompt-mention-hover.spec.ts diff --git a/apps/web/components/task/chat/messages/chat-message.test.tsx b/apps/web/components/task/chat/messages/chat-message.test.tsx index d65bd1d6f1..b106198e7a 100644 --- a/apps/web/components/task/chat/messages/chat-message.test.tsx +++ b/apps/web/components/task/chat/messages/chat-message.test.tsx @@ -19,6 +19,7 @@ const PNG_BASE64 = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII="; const OPEN_ATTACHMENT_1_LABEL = "Open Attachment 1"; const FULL_SIZE_ATTACHMENT_1_ALT = "Full size Attachment 1"; +const PROMPT_MENTION_TESTID = "custom-prompt-mention"; afterEach(() => { cleanup(); @@ -90,13 +91,48 @@ describe("ChatMessage prompt mentions", () => { , ); - const chips = screen.getAllByTestId("custom-prompt-mention"); + const chips = screen.getAllByTestId(PROMPT_MENTION_TESTID); expect(chips).toHaveLength(1); const [chip] = chips; expect(chip.textContent).toBe("@hello"); expect(screen.getByText(/and @missing/)).not.toBeNull(); }); + it("exposes the prompt contents on hover when the prompt is loaded", () => { + const Wrapper = wrapper([], [customPrompt("hello")]); + + render( + + + , + ); + + const [chip] = screen.getAllByTestId(PROMPT_MENTION_TESTID); + // The chip becomes a hover-card trigger so its contents surface on hover, + // rather than relying on the plain browser title tooltip. `data-slot` is a + // shadcn/Radix implementation detail (slot-based CSS targeting), used here + // as a jsdom proxy for "chip is wired as a HoverCard trigger" since we + // can't reliably fire hover in jsdom. + expect(chip.getAttribute("data-slot")).toBe("hover-card-trigger"); + expect(chip.getAttribute("title")).toBeNull(); + }); + + it("falls back to a plain chip with a title when the prompt has no contents", () => { + // A prompt with empty content has nothing to reveal on hover, so keep the + // lightweight title tooltip instead of a hover card. + const Wrapper = wrapper([], [{ ...customPrompt("hollow"), content: "" }]); + + render( + + + , + ); + + const [chip] = screen.getAllByTestId(PROMPT_MENTION_TESTID); + expect(chip.getAttribute("data-slot")).not.toBe("hover-card-trigger"); + expect(chip.getAttribute("title")).toBe("Custom prompt: hollow"); + }); + it("preserves GFM attributes while highlighting prompt mentions", () => { const Wrapper = wrapper([], [customPrompt("hello")]); @@ -113,7 +149,7 @@ describe("ChatMessage prompt mentions", () => { ); const checkbox = screen.getByRole("checkbox") as HTMLInputElement; - expect(screen.getAllByTestId("custom-prompt-mention")).toHaveLength(2); + expect(screen.getAllByTestId(PROMPT_MENTION_TESTID)).toHaveLength(2); expect(checkbox.checked).toBe(true); expect(checkbox.closest("li")?.className).toContain("task-list-item"); expect(screen.getByRole("cell").getAttribute("style")).toContain("text-align: center"); diff --git a/apps/web/components/task/chat/messages/chat-message.tsx b/apps/web/components/task/chat/messages/chat-message.tsx index 334321cf13..057a0a6d24 100644 --- a/apps/web/components/task/chat/messages/chat-message.tsx +++ b/apps/web/components/task/chat/messages/chat-message.tsx @@ -21,6 +21,8 @@ import { MemoizedMarkdown } from "@/components/shared/memoized-markdown"; import { markdownComponents } from "@/components/shared/markdown-components"; import { ImagePreviewDialog } from "@/components/task/chat/image-preview-dialog"; import { useAppStore } from "@/components/state-provider"; +import { HoverCard, HoverCardTrigger, HoverCardContent } from "@kandev/ui/hover-card"; +import { PromptPreview } from "@/components/task/chat/context-items/prompt-preview"; import { buildPromptMentionNames, splitPreparedPromptMentionSegments, @@ -246,19 +248,63 @@ function renderTextWithPromptMentions(text: string, promptNames: string[], keyPr return splitPreparedPromptMentionSegments(text, promptNames).map((segment, index) => { if (segment.kind === "text") return segment.value; return ( - - {segment.value} - + name={segment.name} + value={segment.value} + /> ); }); } +const PROMPT_MENTION_CHIP_CLASS = + "inline rounded-md border border-emerald-300/35 bg-emerald-400/20 px-1.5 py-0.5 font-mono text-[0.88em] font-semibold text-emerald-950 box-decoration-clone break-all dark:text-emerald-100"; + +/** + * Renders a saved-prompt @mention as a chip. When the referenced prompt is + * loaded in the store, hovering the chip reveals its contents so the reader + * doesn't need to switch to the raw message view. + */ +function PromptMentionChip({ name, value }: { name: string; value: string }) { + const content = useAppStore( + useCallback( + (state) => state.prompts.items.find((prompt) => prompt.name === name)?.content ?? null, + [name], + ), + ); + + if (!content) { + return ( + + {value} + + ); + } + + return ( + + + + {value} + + + + + + + ); +} + function parseUserMessageMetadata(comment: Message) { const metadata = comment.metadata as UserMessageMetadata | undefined; const imageAttachments = (metadata?.attachments || []).filter((att) => att.type === "image"); diff --git a/apps/web/e2e/tests/chat/chat-prompt-mention-hover.spec.ts b/apps/web/e2e/tests/chat/chat-prompt-mention-hover.spec.ts new file mode 100644 index 0000000000..cb4a0c1a28 --- /dev/null +++ b/apps/web/e2e/tests/chat/chat-prompt-mention-hover.spec.ts @@ -0,0 +1,79 @@ +import { type Page } from "@playwright/test"; +import { test, expect } from "../../fixtures/test-base"; +import { SessionPage } from "../../pages/session-page"; +import type { ApiClient } from "../../helpers/api-client"; +import type { SeedData } from "../../fixtures/test-base"; + +const PROMPT_NAME = "e2e-hover-template"; +const PROMPT_CONTENT = "Reproduce the bug, isolate the cause, fix with a regression test."; +const CHIP = "custom-prompt-mention"; + +/** + * Create a task whose initial user message is a bare @mention of the seeded + * prompt. The description is persisted verbatim as the first user message, so + * the chat renders the prompt-mention chip once the prompts slice loads. + */ +async function createTaskMentioningPrompt( + apiClient: ApiClient, + seedData: SeedData, +): Promise { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Prompt mention hover", + seedData.agentProfileId, + { + description: `@${PROMPT_NAME}`, + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + return task.id; +} + +async function openTaskChat(page: Page, taskId: string): Promise { + await page.goto(`/t/${taskId}`); + const session = new SessionPage(page); + await session.waitForLoad(); + return session; +} + +test.describe("Chat prompt-mention hover", () => { + test.afterEach(async ({ apiClient }) => { + const { prompts } = await apiClient.listPrompts(); + for (const p of prompts) { + if (!p.builtin && p.name === PROMPT_NAME) { + await apiClient.deletePrompt(p.id).catch(() => undefined); + } + } + }); + + test("hovering a prompt chip reveals the prompt contents", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(90_000); + + await apiClient.createPrompt(PROMPT_NAME, PROMPT_CONTENT); + const taskId = await createTaskMentioningPrompt(apiClient, seedData); + const session = await openTaskChat(testPage, taskId); + + // The mention chip renders once the prompts slice has loaded on the + // session page (via useCustomPrompts) and matched the persisted @mention. + const chip = session.activeChat().getByTestId(CHIP).first(); + await expect(chip).toBeVisible({ timeout: 30_000 }); + await expect(chip).toHaveAttribute("data-prompt-name", PROMPT_NAME); + + // With content loaded, the chip is wired as a Radix hover-card trigger. + await expect(chip).toHaveAttribute("data-slot", "hover-card-trigger", { + timeout: 15_000, + }); + + // Hover reveals the PromptPreview popover with the prompt body. + await chip.hover(); + await expect(testPage.getByText(PROMPT_CONTENT, { exact: false })).toBeVisible({ + timeout: 10_000, + }); + }); +}); diff --git a/apps/web/e2e/tests/office/agent-dashboard.spec.ts b/apps/web/e2e/tests/office/agent-dashboard.spec.ts index 56dd782c83..15c25b2a45 100644 --- a/apps/web/e2e/tests/office/agent-dashboard.spec.ts +++ b/apps/web/e2e/tests/office/agent-dashboard.spec.ts @@ -177,4 +177,20 @@ test.describe("Agent dashboard", () => { await expect(testPage.getByTestId("stacked-bar").first()).toBeVisible({ timeout: 15_000 }); await expect(testPage.locator('[data-segment-key="succeeded"]').first()).toBeAttached(); }); + + test("dashboard content is not hidden by a stuck Suspense boundary", async ({ + testPage, + officeSeed, + }) => { + // Regression guard for a React 19 Suspense loop caused by feeding + // `use(params)` a fresh `Promise.resolve({ id })` on every render of + // `OfficeRoutes`. Symptom: the dashboard tree lived in the DOM but the + // office wrapper was stuck with `display: none !important` from + // React's `hideInstance` because the enclosing Suspense kept re-entering + // fallback. `toBeVisible()` traverses ancestors, so it catches the case + // where a parent hides the subtree even though the target is attached. + await testPage.goto(`/office/agents/${officeSeed.agentId}/dashboard`); + await expect(testPage.getByTestId("agent-detail-section")).toBeVisible({ timeout: 15_000 }); + await expect(testPage.getByTestId("agent-dashboard-view")).toBeVisible({ timeout: 15_000 }); + }); }); diff --git a/apps/web/src/office-routes.test.ts b/apps/web/src/office-routes.test.ts index ef5cadcab0..ad4e027b3c 100644 --- a/apps/web/src/office-routes.test.ts +++ b/apps/web/src/office-routes.test.ts @@ -1,6 +1,10 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it } from "vitest"; -import { resolveActiveOfficeWorkspaceId } from "./office-routes"; +import { + __resetIdParamsPromiseCacheForTests, + idParamsPromise, + resolveActiveOfficeWorkspaceId, +} from "./office-routes"; describe("resolveActiveOfficeWorkspaceId", () => { const wsOffice1 = "ws-office-1"; @@ -68,3 +72,43 @@ describe("resolveActiveOfficeWorkspaceId", () => { expect(activeId).toBe(wsOffice1); }); }); + +describe("idParamsPromise", () => { + // The helper backs Next-style `params: Promise<{ id }>` props consumed via + // `use(params)`. Every call site inside `renderOfficeRoute` runs on each + // render of `OfficeRoutes`, so identity must be stable across calls or the + // enclosing `` re-suspends forever and hides the office tree. + beforeEach(() => { + __resetIdParamsPromiseCacheForTests(); + }); + + it("returns the same promise instance for the same id", () => { + const a = idParamsPromise("agent-123"); + const b = idParamsPromise("agent-123"); + expect(a).toBe(b); + }); + + it("returns distinct promises for different ids", () => { + const a = idParamsPromise("agent-123"); + const b = idParamsPromise("agent-456"); + expect(a).not.toBe(b); + }); + + it("resolves to an object with the requested id", async () => { + await expect(idParamsPromise("agent-789")).resolves.toEqual({ id: "agent-789" }); + }); + + it("re-inserting a cached id after eviction returns a fresh, still-stable promise", () => { + // FIFO eviction runs at MAX_ID_PARAMS_PROMISE_CACHE = 500 entries. Fill + // past that with unique ids, then re-request one of the earliest ids; + // it should have been evicted (new identity) but the *new* identity must + // still be stable on subsequent calls. + const first = idParamsPromise("evict-target"); + for (let i = 0; i < 600; i++) { + idParamsPromise(`fill-${i}`); + } + const refetched = idParamsPromise("evict-target"); + expect(refetched).not.toBe(first); + expect(idParamsPromise("evict-target")).toBe(refetched); + }); +}); diff --git a/apps/web/src/office-routes.tsx b/apps/web/src/office-routes.tsx index f4689ae7e2..9b0e22032e 100644 --- a/apps/web/src/office-routes.tsx +++ b/apps/web/src/office-routes.tsx @@ -130,6 +130,38 @@ export function resolveOfficeHomeSetupRedirect( return hasOfficeWorkspace(workspaceItems) ? null : "/office/setup?mode=new"; } +// The Next.js App Router page/layout convention passes route params as a +// Promise so async server components can `await` them. In this SPA we render +// those same components with `Promise.resolve({ id })`, but every render of +// `OfficeRoutes` would otherwise mint a fresh Promise identity. That makes +// `use(params)` in the consumer re-suspend forever, keeping the enclosing +// `` in fallback and hiding the office tree (`display: none`). +// Caching the resolved promise per id keeps identity stable across renders. +// Bound the cache so long-lived sessions in large workspaces (thousands of +// tasks/agents/projects) don't retain entries for every id ever visited. A +// simple FIFO eviction is enough — identity only needs to be stable across the +// renders that happen while a given route is mounted, and the current route's +// id is always re-inserted on render (which no-ops if it's already present). +const MAX_ID_PARAMS_PROMISE_CACHE = 500; +const idParamsPromiseCache = new Map>(); +export function idParamsPromise(id: string): Promise<{ id: string }> { + let promise = idParamsPromiseCache.get(id); + if (!promise) { + promise = Promise.resolve({ id }); + if (idParamsPromiseCache.size >= MAX_ID_PARAMS_PROMISE_CACHE) { + const oldestKey = idParamsPromiseCache.keys().next().value; + if (oldestKey !== undefined) idParamsPromiseCache.delete(oldestKey); + } + idParamsPromiseCache.set(id, promise); + } + return promise; +} + +/** Test-only: reset the module-level cache so each test starts clean. */ +export function __resetIdParamsPromiseCacheForTests(): void { + idParamsPromiseCache.clear(); +} + function renderOfficeRoute(pathname: string) { const agentRoute = matchAgentRoute(pathname); if (agentRoute) { @@ -138,7 +170,7 @@ function renderOfficeRoute(pathname: string) { const projectId = matchSingle(pathname, /^\/office\/projects\/([^/]+)$/); if (projectId) { - return ; + return ; } const routineId = matchSingle(pathname, /^\/office\/routines\/([^/]+)$/); @@ -148,7 +180,7 @@ function renderOfficeRoute(pathname: string) { const taskId = matchSingle(pathname, /^\/office\/tasks\/([^/]+)$/); if (taskId) { - return ; + return ; } return OFFICE_ROUTES[pathname]?.() ?? ; @@ -276,7 +308,7 @@ type AgentRouteMatch = { }; function renderAgentRoute(route: AgentRouteMatch) { - const params = Promise.resolve({ id: route.id }); + const params = idParamsPromise(route.id); if (route.bare) { return (