diff --git a/app/src/components/flows/ToolActivityChip.test.tsx b/app/src/components/flows/ToolActivityChip.test.tsx deleted file mode 100644 index d77a1be9d..000000000 --- a/app/src/components/flows/ToolActivityChip.test.tsx +++ /dev/null @@ -1,67 +0,0 @@ -import { render, screen } from '@testing-library/react'; -import { describe, expect, it, vi } from 'vitest'; - -import ToolActivityChip from './ToolActivityChip'; - -vi.mock('../../lib/i18n/I18nContext', () => ({ useT: () => ({ t: (key: string) => key }) })); - -describe('ToolActivityChip', () => { - it('renders the proposing label for propose_workflow', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.proposing' - ); - }); - - it('renders the proposing label for revise_workflow too', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.proposing' - ); - }); - - it('renders the dry-running label for dry_run_workflow', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.dryRunning' - ); - }); - - it('renders the saving label for save_workflow', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent('flows.copilot.tool.saving'); - }); - - it('renders a generic "using tools" label for an unrecognized tool name', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.usingTools' - ); - }); - - it('renders nothing for an empty toolNames array', () => { - const { container } = render(); - expect(container).toBeEmptyDOMElement(); - }); - - it('renders the shared label when every tool name maps to the same label', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.proposing' - ); - }); - - it('renders the generic label when tool names map to different labels', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.usingTools' - ); - }); - - it('renders the generic label when one tool is unrecognized, even if another is recognized', () => { - render(); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.usingTools' - ); - }); -}); diff --git a/app/src/components/flows/ToolActivityChip.tsx b/app/src/components/flows/ToolActivityChip.tsx deleted file mode 100644 index 0d39c1ea6..000000000 --- a/app/src/components/flows/ToolActivityChip.tsx +++ /dev/null @@ -1,41 +0,0 @@ -/** - * ToolActivityChip — replaces raw tool-call JSON in the copilot chat with a - * compact, human-readable status pill (B25). Users should know the agent - * used a tool (it explains why the turn took longer), but they should never - * see the raw JSON arguments (e.g. the whole workflow graph payload). - */ -import { useT } from '../../lib/i18n/I18nContext'; - -interface Props { - /** Tool names extracted from the turn's tool-call envelope, in call order. */ - toolNames: string[]; -} - -/** Tools that map to a specific, more informative status label. */ -const KNOWN_TOOL_LABEL_KEYS: Record = { - propose_workflow: 'flows.copilot.tool.proposing', - revise_workflow: 'flows.copilot.tool.proposing', - dry_run_workflow: 'flows.copilot.tool.dryRunning', - save_workflow: 'flows.copilot.tool.saving', -}; - -export default function ToolActivityChip({ toolNames }: Props) { - const { t } = useT(); - if (toolNames.length === 0) return null; - - // Every tool must map to the SAME recognized label before we show a - // specific status (e.g. all of `propose_workflow`/`revise_workflow` map to - // "proposing..."); any unrecognized tool, or a mix of tools with different - // labels, falls back to a generic "Using tools..." pill rather than - // picking one tool's label arbitrarily or dumping tool names verbatim. - const labelKeys = toolNames.map(name => KNOWN_TOOL_LABEL_KEYS[name]); - const labelKey = labelKeys.every(key => key && key === labelKeys[0]) ? labelKeys[0] : undefined; - - return ( - - {t(labelKey ?? 'flows.copilot.tool.usingTools')} - - ); -} diff --git a/app/src/components/flows/WorkflowCopilotPanel.test.tsx b/app/src/components/flows/WorkflowCopilotPanel.test.tsx index f382f1469..1e896b19b 100644 --- a/app/src/components/flows/WorkflowCopilotPanel.test.tsx +++ b/app/src/components/flows/WorkflowCopilotPanel.test.tsx @@ -2,28 +2,30 @@ import { act, fireEvent, render, screen, waitFor } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { WorkflowGraph, WorkflowNode } from '../../lib/flows/types'; -import type { ToolTimelineEntry, WorkflowProposal } from '../../store/chatRuntimeSlice'; +import type { WorkflowProposal } from '../../store/chatRuntimeSlice'; import WorkflowCopilotPanel from './WorkflowCopilotPanel'; vi.mock('../../lib/i18n/I18nContext', () => ({ useT: () => ({ t: (key: string) => key }) })); -interface MockMessage { - id: string; - content: string; - sender: 'user' | 'agent'; - extraMetadata?: { isInterim?: boolean }; -} +// The panel now delegates its entire transcript to the shared `ChatThreadView` +// (message bubbles, tool timeline, sub-agent drawer, streaming previews). That +// component reads the real Redux store; its rendering — including the B25 +// tool-call-envelope unwrap and interim-narration handling — is covered by +// `features/conversations/components/ChatThreadView.test.tsx`. Here we stub it +// so these tests stay focused on the copilot's OWN authoring behavior (the +// `flows_build` send path, seed auto-sends, and the proposal / capped cards) +// without needing a Redux Provider. +vi.mock('../../features/conversations/components/ChatThreadView', () => ({ + ChatThreadView: ({ emptyContent }: { emptyContent?: unknown }) => ( +
{emptyContent as never}
+ ), +})); const hookState = vi.hoisted(() => ({ + threadId: null as string | null, sending: false, proposal: null as WorkflowProposal | null, capped: false, - // Panel renders `displayMessages` (already interim-filtered upstream by - // `useWorkflowBuilderChat`) — kept separate from `messages` in these tests - // so a mismatch between the two proves the panel is reading the right field. - displayMessages: [] as MockMessage[], - toolTimeline: [] as ToolTimelineEntry[], - liveResponse: '', error: null as string | null, send: vi.fn(), clearProposal: vi.fn(), @@ -50,12 +52,10 @@ const baseGraph = graph(['a', 'b']); describe('WorkflowCopilotPanel', () => { beforeEach(() => { + hookState.threadId = null; hookState.sending = false; hookState.proposal = null; hookState.capped = false; - hookState.displayMessages = []; - hookState.toolTimeline = []; - hookState.liveResponse = ''; hookState.error = null; hookState.send = vi.fn().mockResolvedValue({ outcome: 'dispatched', proposed: false }); hookState.clearProposal = vi.fn(); @@ -147,268 +147,13 @@ describe('WorkflowCopilotPanel', () => { expect(thirdArg.request.instruction).toBe('also add a filter step'); }); - it('renders the conversation transcript (user + agent turns)', () => { - hookState.displayMessages = [ - { id: 'm1', content: 'add a Slack step', sender: 'user' }, - { id: 'm2', content: 'Done — proposed a Slack notification.', sender: 'agent' }, - ]; - render( - - ); - expect(screen.getByTestId('workflow-copilot-user')).toHaveTextContent('add a Slack step'); - expect(screen.getByTestId('workflow-copilot-agent')).toHaveTextContent( - 'Done — proposed a Slack notification.' - ); - // With a transcript present, the empty-state hint is gone. - expect(screen.queryByTestId('workflow-copilot-empty')).not.toBeInTheDocument(); - }); - - it('B25: unwraps a raw tool-call envelope message into clean text + a tool activity chip, never raw JSON', () => { - // Repro for B25: a turn that both talks and calls a tool can land in the - // thread transcript as the provider wire-format `{ content, tool_calls }` - // envelope. The panel must render only the human text — never the raw - // JSON — plus a compact status chip for the tool activity. - hookState.displayMessages = [ - { id: 'm1', content: 'build me a Slack digest', sender: 'user' }, - { - id: 'm2', - content: JSON.stringify({ - content: "Here's the workflow I propose.", - tool_calls: [{ id: 'call_1', name: 'propose_workflow', arguments: '{"nodes":[]}' }], - }), - sender: 'agent', - }, - ]; - render( - - ); - const bubble = screen.getByTestId('workflow-copilot-agent'); - expect(bubble).toHaveTextContent("Here's the workflow I propose."); - // The raw envelope must never reach the DOM as text. - expect(bubble).not.toHaveTextContent('tool_calls'); - expect(bubble).not.toHaveTextContent('"nodes":[]'); - expect(screen.getByTestId('tool-activity-chip')).toHaveTextContent( - 'flows.copilot.tool.proposing' - ); - }); - - it('does not render an isInterim agent message as a bubble, only the terminal one', () => { - // The panel only ever renders `displayMessages` — the same filtered set - // `useWorkflowBuilderChat` computes from the raw transcript (isInterim - // agent messages dropped since that narration already streams live via - // the tool timeline / live text). Mirror that filter here so this test - // documents (and would catch a regression in) the composition: an - // isInterim message must never reach the panel as a bubble, while the - // terminal (non-interim) answer still does. - const raw: MockMessage[] = [ - { id: 'm1', content: 'build me a Slack digest', sender: 'user' }, - { - id: 'm2', - content: 'Let me check your calendar first.', - sender: 'agent', - extraMetadata: { isInterim: true }, - }, - { id: 'm3', content: 'Done — proposed a Slack notification.', sender: 'agent' }, - ]; - hookState.displayMessages = raw.filter(m => m.sender === 'user' || !m.extraMetadata?.isInterim); - - render( - - ); - expect(screen.queryByText('Let me check your calendar first.')).not.toBeInTheDocument(); - expect(screen.getByTestId('workflow-copilot-agent')).toHaveTextContent( - 'Done — proposed a Slack notification.' - ); - }); - - it('renders the shared tool timeline + streaming reply during a builder turn', () => { - hookState.sending = true; - hookState.toolTimeline = [ - { id: 'call-1', name: 'propose_workflow', round: 0, status: 'running' } as ToolTimelineEntry, - ]; - hookState.liveResponse = 'Drafting your workflow…'; - render( - - ); - // The shared ToolTimelineBlock renders (not the bespoke transcript), and the - // one-shot "thinking" placeholder is suppressed once activity is streaming. - expect(screen.getByTestId('workflow-copilot-timeline')).toBeInTheDocument(); - expect(screen.queryByTestId('workflow-copilot-thinking')).not.toBeInTheDocument(); - }); - - it('shows the live reply as a bubble before the first tool call streams', () => { - hookState.sending = true; - hookState.toolTimeline = []; - hookState.liveResponse = 'Thinking about your Slack digest…'; - render( - - ); - expect(screen.getByTestId('workflow-copilot-streaming')).toHaveTextContent( - 'Thinking about your Slack digest…' - ); - // No tool timeline yet, and the plain "thinking" line is replaced by the - // streamed text. - expect(screen.queryByTestId('workflow-copilot-timeline')).not.toBeInTheDocument(); - expect(screen.queryByTestId('workflow-copilot-thinking')).not.toBeInTheDocument(); - }); - - // Regression coverage for the "copilot chat gets stuck" bug: the panel used - // to force-scroll to the bottom on every render of a streaming turn (an - // unconditional `scrollTo` effect keyed on messages/tool timeline/live - // text), which fought a user trying to scroll up to read. The panel now - // delegates to the shared `useStickToBottom` hook (same one the main chat - // surfaces use) — these tests exercise the REAL hook (not mocked) wired - // through the actual transcript container. - describe('transcript scroll pinning (#regression: chat gets stuck)', () => { - function scrollContainer() { - return screen.getByTestId('workflow-copilot-transcript'); - } - - // jsdom performs no real layout, so scroll metrics are inert unless - // defined explicitly — mirrors the approach in useStickToBottom.test.ts. - function mockScrollMetrics( - el: HTMLElement, - metrics: { scrollTop: number; scrollHeight: number; clientHeight: number } - ) { - Object.defineProperty(el, 'scrollHeight', { - configurable: true, - value: metrics.scrollHeight, - }); - Object.defineProperty(el, 'clientHeight', { - configurable: true, - value: metrics.clientHeight, - }); - Object.defineProperty(el, 'scrollTop', { - configurable: true, - writable: true, - value: metrics.scrollTop, - }); - } - - function renderPanel() { - return render( - - ); - } - - it('keeps the transcript container freely scrollable (overflow-y-auto)', () => { - renderPanel(); - expect(scrollContainer()).toHaveClass('overflow-y-auto'); - }); - - it('auto-scrolls to the bottom when a new message arrives while the user is pinned to the bottom', () => { - hookState.displayMessages = [{ id: 'm1', content: 'hi', sender: 'user' }]; - const { rerender } = renderPanel(); - const container = scrollContainer(); - - mockScrollMetrics(container, { scrollTop: 50, scrollHeight: 100, clientHeight: 50 }); - rerender( - - ); - - // A new agent turn lands while the user never scrolled away. - hookState.displayMessages = [ - ...hookState.displayMessages, - { id: 'm2', content: 'Done — proposed a Slack notification.', sender: 'agent' }, - ]; - mockScrollMetrics(container, { scrollTop: 50, scrollHeight: 300, clientHeight: 50 }); - rerender( - - ); - - expect(container.scrollTop).toBe(300); - }); - - it('does NOT force-scroll the user back down once they have scrolled up to read history', () => { - hookState.displayMessages = [{ id: 'm1', content: 'hi', sender: 'user' }]; - const { rerender } = renderPanel(); - const container = scrollContainer(); - - mockScrollMetrics(container, { scrollTop: 50, scrollHeight: 100, clientHeight: 50 }); - rerender( - - ); - - // The user scrolls up to read earlier context, well past the stick - // threshold (400 - 0 - 50 = 350px from the bottom). - mockScrollMetrics(container, { scrollTop: 0, scrollHeight: 400, clientHeight: 50 }); - fireEvent.scroll(container); - - // A new agent turn streams in regardless — this is exactly the bug: - // the old unconditional `scrollTo` effect would yank the reader back - // to the bottom here. The container must stay put. - hookState.displayMessages = [ - ...hookState.displayMessages, - { id: 'm2', content: 'Still drafting…', sender: 'agent' }, - ]; - mockScrollMetrics(container, { scrollTop: 0, scrollHeight: 700, clientHeight: 50 }); - rerender( - - ); - - expect(container.scrollTop).toBe(0); - }); - }); + // Transcript rendering (message bubbles, the shared tool timeline + sub-agent + // drawer, streaming previews, the B25 tool-call-envelope unwrap, interim + // narration, and stick-to-bottom scroll pinning) now lives in the shared + // `ChatThreadView` and is covered by + // `features/conversations/components/ChatThreadView.test.tsx`. The panel here + // stubs that component (see the mock above), so these tests assert only the + // copilot's own authoring surface (send path, seeds, proposal / capped cards). it('surfaces a new proposal to the host and shows the added/removed diff', () => { const onProposal = vi.fn(); diff --git a/app/src/components/flows/WorkflowCopilotPanel.tsx b/app/src/components/flows/WorkflowCopilotPanel.tsx index 657e6bbb6..af4a67ea3 100644 --- a/app/src/components/flows/WorkflowCopilotPanel.tsx +++ b/app/src/components/flows/WorkflowCopilotPanel.tsx @@ -8,13 +8,16 @@ * transcript, surfaces each proposal's node-level diff, and hands Accept/Reject * up to the host, which applies it to the local draft overlay. * - * Chat UI parity: the copilot reuses the SHARED chat surface end-to-end — the - * same {@link ChatComposer} the main chat windows use (mic/attachments off - * here), turns render as bubbles via the shared {@link BubbleMarkdown}, and the - * builder turn's live tool activity + streaming reply render through the shared - * {@link ToolTimelineBlock} (fed from the runtime's `toolTimelineByThread` / - * `streamingAssistantByThread`, streamed here by Phase B). So the copilot reads - * like a real chat rather than a one-shot form. + * Chat UI parity: the copilot renders its transcript through the SAME + * {@link ChatThreadView} the home composer chat uses — message bubbles, + * past-turn insights, the shared tool timeline + sub-agent drawer, and the + * streaming / interrupted / parallel previews — driven by this copilot's + * DEDICATED thread. `flows_build` streams the `workflow_builder` turn onto + * that thread via the global `ChatRuntimeProvider` (Phase B), exactly as a + * normal chat turn streams, so the copilot reads like the real chat rather + * than a bespoke transcript. This panel keeps only the authoring concerns: + * the {@link ChatComposer} footer (mic/attachments off), the seed auto-sends, + * and the proposal-preview + capped cards pinned above the composer. * * Invariant: the copilot only PROPOSES — the agent turn itself never * persists. Accept applies the proposal to the local draft AND immediately @@ -27,18 +30,14 @@ import createDebug from 'debug'; import { useCallback, useEffect, useRef, useState } from 'react'; -import { BubbleMarkdown } from '../../features/conversations/components/AgentMessageBubble'; -import { ToolTimelineBlock } from '../../features/conversations/components/ToolTimelineBlock'; -import { useStickToBottom } from '../../hooks/useStickToBottom'; +import { ChatThreadView } from '../../features/conversations/components/ChatThreadView'; import { useWorkflowBuilderChat } from '../../hooks/useWorkflowBuilderChat'; -import { unwrapToolCallEnvelope } from '../../lib/flows/copilotMessageSanitizer'; import { diffGraphs } from '../../lib/flows/graphDiff'; import type { WorkflowGraph } from '../../lib/flows/types'; import { useT } from '../../lib/i18n/I18nContext'; import type { WorkflowProposal } from '../../store/chatRuntimeSlice'; import ChatComposer from '../chat/ChatComposer'; import Button from '../ui/Button'; -import ToolActivityChip from './ToolActivityChip'; const log = createDebug('app:flows:copilot-panel'); @@ -154,19 +153,8 @@ export default function WorkflowCopilotPanel({ fullWidth = false, }: Props) { const { t } = useT(); - const { - threadId, - sending, - turnActive, - proposal, - capped, - displayMessages, - toolTimeline, - liveResponse, - error, - send, - clearProposal, - } = useWorkflowBuilderChat(seedThreadId); + const { threadId, sending, proposal, capped, error, send, clearProposal } = + useWorkflowBuilderChat(seedThreadId); const [text, setText] = useState(''); // Report the (lazily-created) thread id up so the host persists it per flow — @@ -313,36 +301,11 @@ export default function WorkflowCopilotPanel({ onPrefillSeedConsumed?.(); }, [prefillSeed, onPrefillSeedConsumed]); - // Keep the transcript pinned to the newest message / streamed activity — - // but ONLY while the user is already at (or near) the bottom. The previous - // implementation here was an unconditional `scrollTo(bottom)` effect keyed - // on every streaming dependency (messages, tool timeline, live text, …): - // it fired on every streamed token and force-scrolled regardless of where - // the user was reading, which is what made the transcript feel "stuck" — - // any attempt to scroll up got yanked back down by the very next token. - // `useStickToBottom` is the same pinning hook the main chat surfaces use: - // it only auto-scrolls while `stickingRef` is true (user at/near bottom), - // and permanently disengages the moment the user scrolls away, so reading - // history is never fought. `resetKey` is a stable constant here — this - // panel is fully unmounted/remounted on close/reopen (see the seed refs - // above), so there's no in-place "navigation" case to reset for. - const { containerRef: scrollRef } = useStickToBottom( - displayMessages, - threadId, - 'workflow-copilot' - ); - useEffect(() => { - log( - 'scroll: stick-to-bottom deps changed messages=%d thread=%s sending=%s hasProposal=%s timeline=%d liveTextLen=%d', - displayMessages.length, - threadId ?? 'null', - sending, - Boolean(proposal), - toolTimeline.length, - liveResponse.length - ); - }, [displayMessages, threadId, sending, proposal, toolTimeline, liveResponse]); - + // Transcript rendering + scroll pinning (stick-to-bottom) are owned by the + // shared `ChatThreadView` below — the copilot no longer hand-rolls the + // transcript. This component keeps only the authoring concerns: the + // structured `flows_build` send path, the seed auto-sends, and the + // proposal / capped cards surfaced in the footer. const submit = useCallback( async (raw?: string) => { const trimmed = (raw ?? text).trim(); @@ -448,14 +411,6 @@ export default function WorkflowCopilotPanel({ }, [onReject, clearProposal]); const diff = proposal ? diffGraphs(graph, proposal.graph as WorkflowGraph) : null; - const hasTimeline = toolTimeline.length > 0; - // B25: the in-flight streaming text can also carry the raw tool-call - // envelope mid-turn — unwrap once and reuse the clean text everywhere below - // (the pre-tool streaming bubble and the shared `ToolTimelineBlock`). - const liveResponseText = unwrapToolCallEnvelope(liveResponse).text; - const hasLiveText = liveResponseText.trim().length > 0; - const isEmpty = - displayMessages.length === 0 && !proposal && !sending && !error && !hasTimeline && !hasLiveText; return (