fix(office): stop unstable use(params) promise from hiding office tree (#1664)
This commit is contained in:
@@ -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", () => {
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
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(
|
||||
<Wrapper>
|
||||
<ChatMessage comment={userMessage({ content: "@hello" })} label="Message" className="" />
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
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(
|
||||
<Wrapper>
|
||||
<ChatMessage comment={userMessage({ content: "@hollow" })} label="Message" className="" />
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
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");
|
||||
|
||||
@@ -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 (
|
||||
<span
|
||||
<PromptMentionChip
|
||||
key={`${keyPrefix}-prompt-${index}`}
|
||||
data-testid="custom-prompt-mention"
|
||||
data-prompt-name={segment.name}
|
||||
title={`Custom prompt: ${segment.name}`}
|
||||
className="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"
|
||||
>
|
||||
{segment.value}
|
||||
</span>
|
||||
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 (
|
||||
<span
|
||||
data-testid="custom-prompt-mention"
|
||||
data-prompt-name={name}
|
||||
title={`Custom prompt: ${name}`}
|
||||
className={PROMPT_MENTION_CHIP_CLASS}
|
||||
>
|
||||
{value}
|
||||
</span>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<HoverCard openDelay={300} closeDelay={0}>
|
||||
<HoverCardTrigger asChild>
|
||||
<span
|
||||
data-testid="custom-prompt-mention"
|
||||
data-prompt-name={name}
|
||||
tabIndex={0}
|
||||
className={cn(PROMPT_MENTION_CHIP_CLASS, "cursor-default")}
|
||||
>
|
||||
{value}
|
||||
</span>
|
||||
</HoverCardTrigger>
|
||||
<HoverCardContent side="top" align="start" className="w-80 max-h-80 overflow-y-auto">
|
||||
<PromptPreview content={content} />
|
||||
</HoverCardContent>
|
||||
</HoverCard>
|
||||
);
|
||||
}
|
||||
|
||||
function parseUserMessageMetadata(comment: Message) {
|
||||
const metadata = comment.metadata as UserMessageMetadata | undefined;
|
||||
const imageAttachments = (metadata?.attachments || []).filter((att) => att.type === "image");
|
||||
|
||||
@@ -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<string> {
|
||||
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<SessionPage> {
|
||||
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,
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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 });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 `<Suspense>` 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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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
|
||||
// `<Suspense>` 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<string, Promise<{ id: string }>>();
|
||||
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 <ProjectDetailPage params={Promise.resolve({ id: projectId })} />;
|
||||
return <ProjectDetailPage params={idParamsPromise(projectId)} />;
|
||||
}
|
||||
|
||||
const routineId = matchSingle(pathname, /^\/office\/routines\/([^/]+)$/);
|
||||
@@ -148,7 +180,7 @@ function renderOfficeRoute(pathname: string) {
|
||||
|
||||
const taskId = matchSingle(pathname, /^\/office\/tasks\/([^/]+)$/);
|
||||
if (taskId) {
|
||||
return <IssueDetailPage params={Promise.resolve({ id: taskId })} />;
|
||||
return <IssueDetailPage params={idParamsPromise(taskId)} />;
|
||||
}
|
||||
|
||||
return OFFICE_ROUTES[pathname]?.() ?? <OfficeRouteFallback pathname={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 (
|
||||
<AgentDetailLayout params={params}>
|
||||
|
||||
Reference in New Issue
Block a user