Skip to content

Commit 861f455

Browse files
GabrielDraporclaude
andcommitted
fix(ui): scope a tool step to its own turn, and let the sheet own its bounds
Four review findings on the surfaces this PR adds. **A tool step could open another turn's trace.** Resolving the step's trace by scanning forward for the next assistant bubble runs past the turn boundary when a tool-only run produces none, attaching the step to the *following* turn's trace — a different trace, which does not contain that span. The search now stops at the next user message, and the link is offered only when that turn's trace is `available`, the same gate the turn's own "View trace" link uses. A new test file covers all three cases. **The embedded viewer ignored the sheet.** TraceViewerPanel is a fixed 70%-viewport overlay, so nesting it in the drawer painted over the page rather than filling it; the AI Assistant button was also inert, since the sheet hard-codes `aiPanelOpen: false` while `setAiPanelOpen` still reached the outer panel. An explicit `embedded` prop drops the fixed positioning and hides that control. The non-embedded class string is written out unchanged rather than composed — the user-trace snapshot test compares this markup byte for byte, and caught a reordering that was visually identical. **A tooltip claimed a finding predated tracing.** Absent execution status means different things depending on whether an analysis ran at all; `rca_status` distinguishes them, so a never-analyzed finding now reads "not analyzed" and a failed one says so, instead of both claiming they ran before tracing existed. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 07a7008 commit 861f455

6 files changed

Lines changed: 148 additions & 23 deletions

File tree

frontend/ui/src/features/ai-assistant/components/agent-trace-sheet.test.tsx

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,14 @@ import { useLayout, LayoutContext } from "@/components/layout/app-layout";
88
// AgentTraceSheet's local LayoutContext.Provider actually intercepts that read
99
// rather than asserting on TraceViewerPanel's (unrelated, heavy) internals.
1010
vi.mock("@/features/traces/components/TraceViewerPanel", () => ({
11-
TraceViewerPanel: (props: { source?: string }) => {
11+
TraceViewerPanel: (props: { source?: string; embedded?: boolean }) => {
1212
const layout = useLayout();
1313
layout.registerAiHost();
1414
return (
1515
<div
1616
data-testid="trace-panel"
1717
data-source={props.source}
18+
data-embedded={String(!!props.embedded)}
1819
data-ai-panel-open={String(layout.aiPanelOpen)}
1920
/>
2021
);
@@ -71,3 +72,11 @@ describe("AgentTraceSheet", () => {
7172
expect(screen.queryByTestId("trace-panel")).toBeNull();
7273
});
7374
});
75+
76+
it("renders the viewer in embedded mode so it fills the sheet", () => {
77+
// TraceViewerPanel is otherwise a fixed 70%-viewport overlay, which would
78+
// ignore the drawer's bounds and paint over the page. Embedded also hides the
79+
// AI Assistant control, whose panel lives outside this container.
80+
render(<AgentTraceSheet projectId="p1" traceId={"a".repeat(32)} onClose={() => {}} />);
81+
expect(screen.getByTestId("trace-panel").getAttribute("data-embedded")).toBe("true");
82+
});

frontend/ui/src/features/ai-assistant/components/agent-trace-sheet.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ export function AgentTraceSheet({
5151
projectId={projectId}
5252
traceId={traceId}
5353
source="agent"
54+
embedded
5455
onClose={onClose}
5556
onNavigate={() => {}}
5657
canNavigateUp={false}
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
// @vitest-environment jsdom
2+
import { afterEach, describe, expect, it, vi } from "vitest";
3+
import { cleanup, fireEvent, render, screen } from "@testing-library/react";
4+
5+
vi.mock("react-markdown", () => ({ default: ({ children }: { children: string }) => <>{children}</> }));
6+
vi.mock("remark-gfm", () => ({ default: () => {} }));
7+
8+
import { MessageList } from "./message-list";
9+
import type { AIMessage } from "../types";
10+
11+
afterEach(cleanup);
12+
13+
const step = (id: string): AIMessage =>
14+
({
15+
id,
16+
role: "tool_step",
17+
content: "",
18+
toolStep: { toolCallId: id, toolName: "read", args: {}, spanId: `span-${id}`, isError: false },
19+
}) as unknown as AIMessage;
20+
21+
const user = (id: string): AIMessage => ({ id, role: "user", content: "ask" }) as AIMessage;
22+
23+
const assistant = (id: string, traceId: string, traceStatus = "available"): AIMessage =>
24+
({ id, role: "assistant", content: "answer", traceId, traceStatus }) as unknown as AIMessage;
25+
26+
/** Expand every tool step so its "Open span" control is in the DOM. */
27+
function openSteps() {
28+
// The step header is a button carrying the raw tool name in parentheses.
29+
for (const b of screen.getAllByRole("button")) {
30+
if (b.textContent?.includes("(read)")) fireEvent.click(b);
31+
}
32+
}
33+
34+
describe("MessageList tool-step trace resolution", () => {
35+
it("links a tool step to its own turn's trace", () => {
36+
const onOpenTrace = vi.fn();
37+
render(
38+
<MessageList
39+
messages={[user("u1"), step("t1"), assistant("a1", "trace-1")]}
40+
onOpenTrace={onOpenTrace}
41+
/>,
42+
);
43+
openSteps();
44+
fireEvent.click(screen.getByText("Open span"));
45+
expect(onOpenTrace).toHaveBeenCalledWith("trace-1", "span-t1");
46+
});
47+
48+
it("does not reach past a turn boundary for a trace", () => {
49+
// A tool-only run produces no assistant bubble. Scanning past the next user
50+
// message would attach this step to the following turn's trace — a
51+
// different trace, which does not contain this span.
52+
const onOpenTrace = vi.fn();
53+
render(
54+
<MessageList
55+
messages={[user("u1"), step("t1"), user("u2"), assistant("a2", "trace-2")]}
56+
onOpenTrace={onOpenTrace}
57+
/>,
58+
);
59+
openSteps();
60+
expect(screen.queryByText("Open span")).toBeNull();
61+
});
62+
63+
it("offers no link while the turn's trace is pending or failed", () => {
64+
for (const status of ["pending", "failed", "disabled"]) {
65+
const onOpenTrace = vi.fn();
66+
render(
67+
<MessageList
68+
messages={[user("u1"), step("t1"), assistant("a1", "trace-1", status)]}
69+
onOpenTrace={onOpenTrace}
70+
/>,
71+
);
72+
openSteps();
73+
expect(screen.queryByText("Open span")).toBeNull();
74+
cleanup();
75+
}
76+
});
77+
});

frontend/ui/src/features/ai-assistant/components/message-list.tsx

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -580,12 +580,19 @@ export function MessageList({ messages, sessionStreaming = false, onOpenTrace }:
580580
<div ref={innerRef}>
581581
{messages.map((msg, index) => {
582582
if (msg.role === "tool_step" && msg.toolStep) {
583-
// The step's own turn hasn't produced its trace-carrying assistant
584-
// bubble yet until that bubble appears later in the list — its
585-
// traceId is the one this tool step's span belongs to.
586-
const turnTraceId = messages
587-
.slice(index + 1)
588-
.find((m) => m.role === "assistant")?.traceId;
583+
// A tool step belongs to the turn that produced it, and that turn's
584+
// trace id arrives on the assistant bubble later in the list. The
585+
// search must stop at the next user message: a tool-only run
586+
// produces no assistant bubble, and scanning past the turn boundary
587+
// would attach this step to the *next* turn's trace — a different
588+
// trace that does not contain this span.
589+
const turnEnd = messages.findIndex((m, i) => i > index && m.role === "user");
590+
const turn = messages.slice(index + 1, turnEnd === -1 ? undefined : turnEnd);
591+
const turnAssistant = turn.find((m) => m.role === "assistant");
592+
// Same gate as the turn's own "View trace" link: a pending or failed
593+
// export has no trace to open.
594+
const turnTraceId =
595+
turnAssistant?.traceStatus === "available" ? turnAssistant.traceId : undefined;
589596
const onOpenSpan =
590597
onOpenTrace && turnTraceId
591598
? (spanId: string) => onOpenTrace(turnTraceId, spanId)

frontend/ui/src/features/detectors/components/detector-runs-table.tsx

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -126,11 +126,20 @@ export function DetectorRunsTable({
126126
title={
127127
run.execution_trace_status === "available"
128128
? `${findingId} — open the analysis trace`
129-
: run.execution_trace_status == null
130-
? `${findingId} — no analysis trace (analysis ran before tracing was enabled)`
131-
: run.execution_trace_status === "pending"
132-
? `${findingId} — analysis trace is being recorded`
133-
: `${findingId} — analysis trace unavailable`
129+
: run.execution_trace_status === "pending"
130+
? `${findingId} — analysis trace is being recorded`
131+
: run.execution_trace_status != null
132+
? `${findingId} — analysis trace unavailable`
133+
: // No execution trace status. Absent means different
134+
// things depending on whether an analysis ran at
135+
// all, and saying "before tracing was enabled" for
136+
// a finding that was never analysed is simply
137+
// wrong — rca_status is what distinguishes them.
138+
run.rca_status == null
139+
? `${findingId} — not analyzed`
140+
: run.rca_status === "done"
141+
? `${findingId} — no analysis trace (analysis ran before tracing was enabled)`
142+
: `${findingId} — analysis ${run.rca_status}; no trace`
134143
}
135144
onOpen={
136145
run.execution_trace_status === "available"

frontend/ui/src/features/traces/components/TraceViewerPanel.tsx

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,14 @@ interface TraceViewerPanelProps {
5151
autoOpenRca?: boolean;
5252
/** When true, the panel mounts already expanded to full width (e.g. opened in a new tab). */
5353
initialFullscreen?: boolean;
54+
/**
55+
* Rendered inside another surface (the agent-trace sheet) rather than as the
56+
* page's own overlay. Drops the fixed full-height positioning so the host
57+
* controls the bounds, and hides the AI Assistant control: the assistant it
58+
* would toggle lives outside this container, so the button either does
59+
* nothing or opens a panel the user cannot see from here.
60+
*/
61+
embedded?: boolean;
5462
/**
5563
* Base path the "open in new tab" button targets, so the trace pops out back
5664
* into the page it was opened from. Defaults to the project traces page; the
@@ -167,6 +175,7 @@ export function TraceViewerPanel({
167175
customEndDate,
168176
autoOpenRca,
169177
initialFullscreen,
178+
embedded,
170179
newTabPath,
171180
traceOverride,
172181
hideDetectors,
@@ -378,16 +387,25 @@ export function TraceViewerPanel({
378387
return (
379388
<div
380389
className={cn(
381-
"animate-slide-in-right fixed bottom-0 right-0 z-50 border-l border-border bg-background shadow-xl transition-[width,top] duration-200",
382-
// Fullscreen stays clear of the chrome it would otherwise cover: it
383-
// starts below the top breadcrumb/header bar (h-14) and to the right of
384-
// the left navbar. Width = 100% minus the sidebar's width, which differs
385-
// when the sidebar is collapsed.
386-
isFullscreen
387-
? sidebarCollapsed
388-
? "top-14 w-[calc(100%-3.5rem)]"
389-
: "top-14 w-[calc(100%-12rem)]"
390-
: "top-0 w-[70%]",
390+
// Embedded, the host (a drawer) owns the bounds: filling it is the whole
391+
// job. Keeping the fixed 70%-viewport overlay here would ignore the
392+
// drawer and paint over the page instead of inside it. The non-embedded
393+
// branch is written out in full, in its original order, because the
394+
// user-trace snapshot test compares this markup byte for byte.
395+
embedded
396+
? "flex h-full w-full flex-col border-l border-border bg-background"
397+
: cn(
398+
"animate-slide-in-right fixed bottom-0 right-0 z-50 border-l border-border bg-background shadow-xl transition-[width,top] duration-200",
399+
// Fullscreen stays clear of the chrome it would otherwise cover: it
400+
// starts below the top breadcrumb/header bar (h-14) and to the right of
401+
// the left navbar. Width = 100% minus the sidebar's width, which differs
402+
// when the sidebar is collapsed.
403+
isFullscreen
404+
? sidebarCollapsed
405+
? "top-14 w-[calc(100%-3.5rem)]"
406+
: "top-14 w-[calc(100%-12rem)]"
407+
: "top-0 w-[70%]",
408+
),
391409
)}
392410
>
393411
<div className="flex h-full flex-col bg-background">
@@ -531,7 +549,10 @@ export function TraceViewerPanel({
531549
<div className="w-2" />
532550
{/* AI Assistant sits immediately left of Close, separated by a gap
533551
from the navigation/view controls, so the agent button stays the
534-
rightmost action regardless of the other header controls. */}
552+
rightmost action regardless of the other header controls. Hidden
553+
when embedded — the assistant it toggles is outside this
554+
container. */}
555+
{!embedded && (
535556
<Button
536557
variant="outline"
537558
size="sm"
@@ -548,6 +569,7 @@ export function TraceViewerPanel({
548569
>
549570
<DOMAIN_ICONS.assistant className="h-4 w-4" />
550571
</Button>
572+
)}
551573
<Button variant="ghost" size="sm" onClick={onClose} className="h-7 w-7 p-0">
552574
<X className="h-4 w-4" />
553575
</Button>

0 commit comments

Comments
 (0)