diff --git a/apps/web/src/components/app/child-agent-row.test.tsx b/apps/web/src/components/app/child-agent-row.test.tsx index 4d8d80ba..2e0c835d 100644 --- a/apps/web/src/components/app/child-agent-row.test.tsx +++ b/apps/web/src/components/app/child-agent-row.test.tsx @@ -1,6 +1,7 @@ // @vitest-environment jsdom import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import type { ComponentProps } from "react"; +import { MemoryRouter } from "react-router-dom"; import { afterEach, describe, expect, it, vi } from "vitest"; import type { Agent } from "@/components/app/types"; @@ -42,19 +43,21 @@ function renderRow( const startAgent = vi.fn().mockResolvedValue(undefined); const openSubmittedReview = vi.fn(); render( - - - + + + + + ); return { attachToAgent, @@ -100,6 +103,47 @@ describe("ChildAgentRow", () => { expect(row.className).not.toContain("child-agent-review-active-row"); }); + it("keeps the row muted until a review has been submitted", () => { + renderRow(baseAgent); + + const row = screen.getByTestId("child-agent-row-agt_child"); + expect(row.dataset.reviewReady).toBe("false"); + expect(row.className).not.toContain("cursor-pointer"); + const badge = screen.getByText("Review"); + expect(badge.className).toContain("bg-background"); + }); + + it("lights up the row once the review can be opened", () => { + renderRow( + { ...baseAgent, status: "stopped", submittedReviewId: 42 }, + { state: "stopped", isInitialReviewActive: false } + ); + + const row = screen.getByTestId("child-agent-row-agt_child"); + expect(row.dataset.reviewReady).toBe("true"); + expect(row.className).toContain("cursor-pointer"); + expect(row.className).toContain("border-primary/45"); + expect(row.className).toContain("opacity-100"); + expect(row.className).not.toContain("opacity-65"); + const badge = screen.getByText("Review"); + expect(badge.className).toContain("bg-primary"); + expect(badge.className).toContain("text-primary-foreground"); + }); + + it("keeps the ready treatment when the row is terminal-connected", () => { + renderRow( + { ...baseAgent, submittedReviewId: 42 }, + { isConnected: true, isInitialReviewActive: false } + ); + + const row = screen.getByTestId("child-agent-row-agt_child"); + expect(row.className).toContain("border-primary/45"); + expect(row.className).toContain("bg-primary/[0.06]"); + expect(row.className).toContain("hover:bg-primary/10"); + expect(row.className).toContain("cursor-pointer"); + expect(row.className).not.toContain("border-primary/35"); + }); + it("opens a submitted review from the row without attaching its terminal", () => { const submittedAgent = { ...baseAgent, diff --git a/apps/web/src/components/app/child-agent-row.tsx b/apps/web/src/components/app/child-agent-row.tsx index a2bdf3d8..a25a89b8 100644 --- a/apps/web/src/components/app/child-agent-row.tsx +++ b/apps/web/src/components/app/child-agent-row.tsx @@ -7,6 +7,7 @@ import { } from "@/components/app/agent-event-utils"; import { AgentTypeIcon } from "@/components/app/agent-type-icon"; import { type Agent, type AgentVisualState } from "@/components/app/types"; +import { TipSpot } from "@/components/tips/tip-spot"; import { Badge } from "@/components/ui/badge"; import { Button } from "@/components/ui/button"; import { @@ -62,17 +63,21 @@ export function ChildAgentRow({ ? latestEventColor(agent.latestEvent.type) : "text-muted-foreground"; - return ( + const row = (
@@ -109,7 +114,14 @@ export function ChildAgentRow({ {displayName} {isReviewAgent ? ( - + Review ) : null} @@ -186,4 +198,16 @@ export function ChildAgentRow({ )}
); + + if (!canOpenSubmittedReview) return row; + return ( + + {row} + + ); } diff --git a/apps/web/src/components/tips/tip-queue-context.ts b/apps/web/src/components/tips/tip-queue-context.ts index 131f48c3..a0b3b6f1 100644 --- a/apps/web/src/components/tips/tip-queue-context.ts +++ b/apps/web/src/components/tips/tip-queue-context.ts @@ -2,8 +2,8 @@ import { createContext, useContext } from "react"; export type TipQueueContextValue = { activeTipId: string | null; - requestOpen: (tipId: string) => boolean; - release: (tipId: string) => void; + requestOpen: (tipId: string, instanceId: string) => boolean; + release: (tipId: string, instanceId: string) => void; }; export const TipQueueContext = createContext({ diff --git a/apps/web/src/components/tips/tip-queue-provider.test.tsx b/apps/web/src/components/tips/tip-queue-provider.test.tsx new file mode 100644 index 00000000..41c4f0a6 --- /dev/null +++ b/apps/web/src/components/tips/tip-queue-provider.test.tsx @@ -0,0 +1,118 @@ +// @vitest-environment jsdom +import { act, cleanup, render } from "@testing-library/react"; +import { afterEach, describe, expect, it } from "vitest"; + +import { useTipQueue } from "./tip-queue-context"; +import { TipQueueProvider } from "./tip-queue-provider"; + +type QueueHandle = ReturnType; + +function Capture({ handle }: { handle: { current: QueueHandle | null } }) { + handle.current = useTipQueue(); + return null; +} + +function renderQueue() { + const handle: { current: QueueHandle | null } = { current: null }; + render( + + + + ); + return handle as { current: QueueHandle }; +} + +afterEach(cleanup); + +describe("TipQueueProvider", () => { + it("grants the first instance and denies a second instance of the same tip", () => { + const queue = renderQueue(); + + let first = false; + act(() => { + first = queue.current.requestOpen("review-row-open", "instance-a"); + }); + expect(first).toBe(true); + + let second = true; + act(() => { + second = queue.current.requestOpen("review-row-open", "instance-b"); + }); + expect(second).toBe(false); + + // The owner keeps its grant on re-request (effect re-runs). + let ownerAgain = false; + act(() => { + ownerAgain = queue.current.requestOpen("review-row-open", "instance-a"); + }); + expect(ownerAgain).toBe(true); + }); + + it("does not hand the same tip to another instance after the owner releases", () => { + const queue = renderQueue(); + + act(() => { + queue.current.requestOpen("review-row-open", "instance-a"); + }); + act(() => { + queue.current.requestOpen("review-row-open", "instance-b"); + }); + act(() => { + queue.current.release("review-row-open", "instance-a"); + }); + + expect(queue.current.activeTipId).toBeNull(); + }); + + it("queues a different tip and grants it after release", () => { + const queue = renderQueue(); + + act(() => { + queue.current.requestOpen("review-row-open", "instance-a"); + }); + let other = true; + act(() => { + other = queue.current.requestOpen("split-tabs", "instance-c"); + }); + expect(other).toBe(false); + + act(() => { + queue.current.release("review-row-open", "instance-a"); + }); + expect(queue.current.activeTipId).toBe("split-tabs"); + + let granted = false; + act(() => { + granted = queue.current.requestOpen("split-tabs", "instance-c"); + }); + expect(granted).toBe(true); + }); + + it("denies a second same-tick grant before state commits", () => { + const queue = renderQueue(); + + let first = false; + let second = true; + act(() => { + // Both calls happen in one tick, before the first grant's state commit. + first = queue.current.requestOpen("review-row-open", "instance-a"); + second = queue.current.requestOpen("review-row-open", "instance-b"); + }); + + expect(first).toBe(true); + expect(second).toBe(false); + }); + + it("ignores a release from a non-owning instance", () => { + const queue = renderQueue(); + + act(() => { + queue.current.requestOpen("review-row-open", "instance-a"); + }); + act(() => { + queue.current.release("review-row-open", "instance-b"); + }); + + expect(queue.current.activeTipId).toBe("review-row-open"); + }); +}); diff --git a/apps/web/src/components/tips/tip-queue-provider.tsx b/apps/web/src/components/tips/tip-queue-provider.tsx index b896059f..2f57550f 100644 --- a/apps/web/src/components/tips/tip-queue-provider.tsx +++ b/apps/web/src/components/tips/tip-queue-provider.tsx @@ -2,36 +2,63 @@ import { useCallback, useMemo, useRef, useState } from "react"; import { TipQueueContext } from "@/components/tips/tip-queue-context"; +type TipClaim = { tipId: string; instanceId: string }; + export function TipQueueProvider({ children }: { children: React.ReactNode }) { - const [activeTipId, setActiveTipId] = useState(null); - const queueRef = useRef([]); + const [active, setActive] = useState(null); + // Synchronous source of truth: two open timers can fire in the same tick, + // before the `active` state from the first grant has committed. + const activeRef = useRef(null); + const queueRef = useRef([]); + // `active` is an intentional dep even though reads go through activeRef: + // its identity change re-runs waiting TipSpot effects so a queued tip + // re-requests (and wins) after the previous owner releases. const requestOpen = useCallback( - (tipId: string): boolean => { - if (activeTipId === null) { - setActiveTipId(tipId); + (tipId: string, instanceId: string): boolean => { + const current = activeRef.current; + if (current === null) { + const claim = { tipId, instanceId }; + activeRef.current = claim; + setActive(claim); return true; } - if (activeTipId === tipId) return true; - if (!queueRef.current.includes(tipId)) { - queueRef.current.push(tipId); + if (current.tipId === tipId) { + // Only the owning instance holds the grant. Another instance of the + // same tip is denied without queueing — after the owner dismisses, + // the tip lands in dismissedTips and no other instance should open. + return current.instanceId === instanceId; + } + if ( + !queueRef.current.some( + (claim) => claim.tipId === tipId && claim.instanceId === instanceId + ) + ) { + queueRef.current.push({ tipId, instanceId }); } return false; }, - [activeTipId] + // eslint-disable-next-line react-hooks/exhaustive-deps -- see comment above + [active] ); - const release = useCallback((tipId: string) => { - setActiveTipId((current) => { - if (current !== tipId) return current; - const next = queueRef.current.shift() ?? null; - return next; - }); + const release = useCallback((tipId: string, instanceId: string) => { + const current = activeRef.current; + if ( + current === null || + current.tipId !== tipId || + current.instanceId !== instanceId + ) { + return; + } + const next = queueRef.current.shift() ?? null; + activeRef.current = next; + setActive(next); }, []); const value = useMemo( - () => ({ activeTipId, requestOpen, release }), - [activeTipId, requestOpen, release] + () => ({ activeTipId: active?.tipId ?? null, requestOpen, release }), + [active, requestOpen, release] ); return ( diff --git a/apps/web/src/components/tips/tip-spot.tsx b/apps/web/src/components/tips/tip-spot.tsx index 41b69603..e2d8e697 100644 --- a/apps/web/src/components/tips/tip-spot.tsx +++ b/apps/web/src/components/tips/tip-spot.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, + useId, useLayoutEffect, useRef, useState, @@ -13,6 +14,7 @@ import { PopoverTrigger, } from "@/components/ui/popover"; import { useTip } from "@/lib/tips/use-tip"; +import { cn } from "@/lib/utils"; import { TipPopoverContent } from "./tip-popover-content"; import { useTipQueue } from "./tip-queue-context"; @@ -73,6 +75,8 @@ type TipSpotProps = { side?: "top" | "bottom" | "left" | "right"; align?: "start" | "center" | "end"; sideOffset?: number; + /** Extra classes for the trigger wrapper, e.g. "flex w-full" for block-level anchors. */ + triggerClassName?: string; children: React.ReactNode; }; @@ -81,11 +85,13 @@ export function TipSpot({ side = "right", align = "center", sideOffset = 8, + triggerClassName, children, }: TipSpotProps) { const navigate = useNavigate(); const { tip, shouldShowInline, dismiss, disableAll } = useTip(tipId); const { requestOpen, release } = useTipQueue(); + const instanceId = useId(); const [open, setOpen] = useState(false); const eligibleRef = useRef(false); const triggerRef = useRef(null); @@ -100,10 +106,10 @@ export function TipSpot({ useEffect(() => { if (!shouldShowInline || open) return; const timer = setTimeout(() => { - if (requestOpen(tipId)) setOpen(true); + if (requestOpen(tipId, instanceId)) setOpen(true); }, 500); return () => clearTimeout(timer); - }, [shouldShowInline, open, tipId, requestOpen]); + }, [shouldShowInline, open, tipId, instanceId, requestOpen]); // Post-commit reachability: checked in useLayoutEffect so we read // the actual committed DOM (not the stale previous-commit snapshot @@ -139,14 +145,14 @@ export function TipSpot({ const handleDismiss = useCallback(() => { setOpen(false); dismiss(); - release(tipId); - }, [dismiss, release, tipId]); + release(tipId, instanceId); + }, [dismiss, release, tipId, instanceId]); const handleDisableAll = useCallback(() => { setOpen(false); disableAll(); - release(tipId); - }, [disableAll, release, tipId]); + release(tipId, instanceId); + }, [disableAll, release, tipId, instanceId]); const handleOpenChange = useCallback( (nextOpen: boolean) => { @@ -181,7 +187,7 @@ export function TipSpot({ return ( - + {children} diff --git a/apps/web/src/lib/tips/tips.ts b/apps/web/src/lib/tips/tips.ts index b4b31244..4a654551 100644 --- a/apps/web/src/lib/tips/tips.ts +++ b/apps/web/src/lib/tips/tips.ts @@ -196,6 +196,14 @@ export const tips: Tip[] = [ since: "0.27.0", surfaces: ["ambient"], }, + { + id: "review-row-open", + title: "Open Submitted Reviews", + body: "A review agent's row lights up once its review is submitted. Click the row to open the review and work through its feedback.", + docsSection: "personas", + since: "0.29.1", + surfaces: ["inline"], + }, { id: "job-webhook-trigger", title: "Job Webhook Triggers", diff --git a/e2e/review-agent-ui.spec.ts b/e2e/review-agent-ui.spec.ts index 7691ad2d..ccf4a453 100644 --- a/e2e/review-agent-ui.spec.ts +++ b/e2e/review-agent-ui.spec.ts @@ -58,6 +58,13 @@ test.describe("Review agent UI", () => { page.getByTestId(`child-agent-row-${fixture.openReviewAgentId}`) ).toHaveAttribute("data-review-active", "false"); + await expect( + page.getByTestId(`child-agent-row-${fixture.openReviewAgentId}`) + ).toHaveAttribute("data-review-ready", "true"); + await expect( + page.getByTestId(`child-agent-row-${fixture.activeAgentId}`) + ).toHaveAttribute("data-review-ready", "false"); + await page .getByTestId(`child-agent-open-review-${fixture.openReviewAgentId}`) .click();