From 1409450402b071a69e57dee418eed928df0677a9 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Thu, 16 Jul 2026 22:33:20 -0600 Subject: [PATCH 1/3] Light up review agent rows and add a tip once their review is ready When a review agent's review is submitted (the same condition that makes the row clickable), the sidebar row now gets a primary-tinted border and background, a filled Review badge, and full opacity even when stopped. A one-time contextual tip using the built-in TipSpot system anchors to the row, explaining that clicking it opens the review. TipSpot gains an optional triggerClassName so it can wrap block-level anchors like the row. Co-Authored-By: Claude Fable 5 --- .../components/app/child-agent-row.test.tsx | 56 ++++++++++++++----- .../src/components/app/child-agent-row.tsx | 31 ++++++++-- apps/web/src/components/tips/tip-spot.tsx | 6 +- apps/web/src/lib/tips/tips.ts | 8 +++ e2e/review-agent-ui.spec.ts | 7 +++ 5 files changed, 90 insertions(+), 18 deletions(-) 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..f48bc042 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,33 @@ 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("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..ef702738 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,20 @@ export function ChildAgentRow({ ? latestEventColor(agent.latestEvent.type) : "text-muted-foreground"; - return ( + const row = (
@@ -109,7 +113,14 @@ export function ChildAgentRow({ {displayName} {isReviewAgent ? ( - + Review ) : null} @@ -186,4 +197,16 @@ export function ChildAgentRow({ )}
); + + if (!canOpenSubmittedReview) return row; + return ( + + {row} + + ); } diff --git a/apps/web/src/components/tips/tip-spot.tsx b/apps/web/src/components/tips/tip-spot.tsx index 41b69603..d198f2c8 100644 --- a/apps/web/src/components/tips/tip-spot.tsx +++ b/apps/web/src/components/tips/tip-spot.tsx @@ -13,6 +13,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 +74,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,6 +84,7 @@ export function TipSpot({ side = "right", align = "center", sideOffset = 8, + triggerClassName, children, }: TipSpotProps) { const navigate = useNavigate(); @@ -181,7 +185,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(); From b58e5bce68e2ea02e792a9116ab02e48ea6fa7ae Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Thu, 16 Jul 2026 22:56:17 -0600 Subject: [PATCH 2/3] Address review: instance-scoped tip queue, ready-state precedence over connected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Persona review agt_315e90987a6a items #1156/#1157: the tip queue now tracks which TipSpot instance owns the active grant (via useId token), so multiple rows sharing one tipId show a single popover — this also covers the latent duplicate for split-tabs in center-pane-tab-bar. The review-ready row classes now come after isConnected so the lit treatment wins both resting and hover styles when a terminal is attached. Co-Authored-By: Claude Fable 5 --- .../components/app/child-agent-row.test.tsx | 14 +++ .../src/components/app/child-agent-row.tsx | 3 +- .../src/components/tips/tip-queue-context.ts | 4 +- .../tips/tip-queue-provider.test.tsx | 103 ++++++++++++++++++ .../components/tips/tip-queue-provider.tsx | 48 +++++--- apps/web/src/components/tips/tip-spot.tsx | 14 ++- 6 files changed, 161 insertions(+), 25 deletions(-) create mode 100644 apps/web/src/components/tips/tip-queue-provider.test.tsx 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 f48bc042..2e0c835d 100644 --- a/apps/web/src/components/app/child-agent-row.test.tsx +++ b/apps/web/src/components/app/child-agent-row.test.tsx @@ -130,6 +130,20 @@ describe("ChildAgentRow", () => { 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 ef702738..a25a89b8 100644 --- a/apps/web/src/components/app/child-agent-row.tsx +++ b/apps/web/src/components/app/child-agent-row.tsx @@ -72,9 +72,10 @@ export function ChildAgentRow({ className={cn( "group relative flex min-h-11 w-full min-w-0 items-center gap-2 rounded-lg border border-border/60 bg-background/30 px-2 py-1 sm:py-1.5", "transition-colors hover:bg-muted/35", + isConnected && "border-primary/35 bg-muted/40", + // Ready-to-open wins over connected styling: it is the actionable state. canOpenSubmittedReview && "cursor-pointer border-primary/45 bg-primary/[0.06] hover:bg-primary/10", - isConnected && "border-primary/35 bg-muted/40", isStopped && "opacity-65", canOpenSubmittedReview && "opacity-100", showReviewActivity && "child-agent-review-active-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..734dcee3 --- /dev/null +++ b/apps/web/src/components/tips/tip-queue-provider.test.tsx @@ -0,0 +1,103 @@ +// @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("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..3a71e0d8 100644 --- a/apps/web/src/components/tips/tip-queue-provider.tsx +++ b/apps/web/src/components/tips/tip-queue-provider.tsx @@ -2,36 +2,52 @@ 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); + const queueRef = useRef([]); const requestOpen = useCallback( - (tipId: string): boolean => { - if (activeTipId === null) { - setActiveTipId(tipId); + (tipId: string, instanceId: string): boolean => { + if (active === null) { + setActive({ tipId, instanceId }); return true; } - if (activeTipId === tipId) return true; - if (!queueRef.current.includes(tipId)) { - queueRef.current.push(tipId); + if (active.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 active.instanceId === instanceId; + } + if ( + !queueRef.current.some( + (claim) => claim.tipId === tipId && claim.instanceId === instanceId + ) + ) { + queueRef.current.push({ tipId, instanceId }); } return false; }, - [activeTipId] + [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) => { + setActive((current) => { + if ( + current === null || + current.tipId !== tipId || + current.instanceId !== instanceId + ) { + return current; + } + return queueRef.current.shift() ?? null; }); }, []); 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 d198f2c8..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, @@ -90,6 +91,7 @@ export function TipSpot({ 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); @@ -104,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 @@ -143,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) => { From c6138009347799ce1e8f29eb7ae5a01be8618f4e Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Thu, 16 Jul 2026 23:06:18 -0600 Subject: [PATCH 3/3] Harden tip queue against same-tick double grant Reviewer-suggested hardening on feedback #1156: requestOpen previously read the active claim from a stale closure, so two TipSpot timers firing in the same tick could both see null and both win. The claim now lives in a ref checked synchronously; state remains only to re-trigger waiting TipSpot effects after a release. Co-Authored-By: Claude Fable 5 --- .../tips/tip-queue-provider.test.tsx | 15 +++++++ .../components/tips/tip-queue-provider.tsx | 39 ++++++++++++------- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/apps/web/src/components/tips/tip-queue-provider.test.tsx b/apps/web/src/components/tips/tip-queue-provider.test.tsx index 734dcee3..41c4f0a6 100644 --- a/apps/web/src/components/tips/tip-queue-provider.test.tsx +++ b/apps/web/src/components/tips/tip-queue-provider.test.tsx @@ -88,6 +88,21 @@ describe("TipQueueProvider", () => { 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(); diff --git a/apps/web/src/components/tips/tip-queue-provider.tsx b/apps/web/src/components/tips/tip-queue-provider.tsx index 3a71e0d8..2f57550f 100644 --- a/apps/web/src/components/tips/tip-queue-provider.tsx +++ b/apps/web/src/components/tips/tip-queue-provider.tsx @@ -6,19 +6,28 @@ type TipClaim = { tipId: string; instanceId: string }; export function TipQueueProvider({ children }: { children: React.ReactNode }) { 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, instanceId: string): boolean => { - if (active === null) { - setActive({ tipId, instanceId }); + const current = activeRef.current; + if (current === null) { + const claim = { tipId, instanceId }; + activeRef.current = claim; + setActive(claim); return true; } - if (active.tipId === 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 active.instanceId === instanceId; + return current.instanceId === instanceId; } if ( !queueRef.current.some( @@ -29,20 +38,22 @@ export function TipQueueProvider({ children }: { children: React.ReactNode }) { } return false; }, + // eslint-disable-next-line react-hooks/exhaustive-deps -- see comment above [active] ); const release = useCallback((tipId: string, instanceId: string) => { - setActive((current) => { - if ( - current === null || - current.tipId !== tipId || - current.instanceId !== instanceId - ) { - return current; - } - return queueRef.current.shift() ?? null; - }); + 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(