diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 9d37df0996..284ae56abf 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -4726,7 +4726,7 @@ const ( // Subagent summaries reuse the final report instead of generating // text, so their work timeout only covers two database round trips. subagentReportSummaryTimeout = 15 * time.Second - // Bound the extracted report snippet near the 1-3 sentence + // Bound the extracted report snippet near the headline of the // generated summaries that root chats get, so subagent and parent // summary panels read the same. subagentReportSummaryMaxRunes = 300 @@ -4948,7 +4948,10 @@ func (p *Server) storeSubagentReportSummary( slog.F("chat_id", chat.ID), slog.Error(err)) return } - summary := subagentReportSummarySnippet(report) + // Subagent summaries are extracted from an existing report rather than + // generated, so they have no bullets; the serializer keeps the headline + // as-is and only adds structure when bullets exist. + summary := formatChatSummaryMarkdown(subagentReportSummarySnippet(report), nil) if summary == "" { return } diff --git a/coderd/x/chatd/quickgen.go b/coderd/x/chatd/quickgen.go index ccc73b3ed3..debd2f5f91 100644 --- a/coderd/x/chatd/quickgen.go +++ b/coderd/x/chatd/quickgen.go @@ -969,11 +969,13 @@ func generateManualTitle( } const chatSummaryGenerationPrompt = "You summarize an AI coding chat for a quick-reference popover. " + - "Populate the summary field with 1 to 3 plain sentences describing what the conversation is about and what was accomplished or attempted. " + + "Populate the headline field with one sentence naming what the conversation is about and its outcome. " + + "Populate the bullets field with 2 to 4 short bullets covering what was done or attempted, each a single line. " + "Write about the conversation in the third person. " + - "Preserve specific identifiers such as PR numbers, repo names, file paths, function names, and error messages. " + + "Preserve specific identifiers such as PR numbers, repo names, file paths, function names, and error messages, " + + "wrapping them in backticks. " + "Do not address the user, give instructions, or continue the task. " + - "No markdown, lists, headings, code fences, or surrounding quotes." + "No headings, code fences, tables, or nested lists." const ( // Bound the transcript so the summary call stays cheap and within context; @@ -982,14 +984,20 @@ const ( // Cap a single turn so one long message cannot dominate the budget. summaryTranscriptPerMessageMaxRunes = 4000 summaryMaxOutputTokens = 512 - // Reject pathologically long or verbose summaries, with slack over the - // 1-3 sentence target. - summaryMaxRunes = 1000 - summaryMaxSentences = 6 + // Reject pathologically long or verbose summaries. The caps apply to the + // structured fields before serialization, plus a ceiling on the rendered + // markdown so the panel stays scannable. + summaryMaxRunes = 600 + summaryHeadlineMaxRunes = 200 + summaryHeadlineMaxSentences = 2 + summaryBulletMaxRunes = 160 + summaryMinBullets = 2 + summaryMaxBullets = 4 ) type generatedChatSummary struct { - Summary string `json:"summary" description:"1-3 sentence summary of the whole chat"` + Headline string `json:"headline" description:"One sentence naming what the chat is about and its outcome"` + Bullets []string `json:"bullets" description:"2-4 short bullets, each one line, covering what was done or attempted"` } // renderChatSummaryTranscript renders chat history as plain text for summary @@ -1088,9 +1096,10 @@ func boundTranscriptHeadTail(lines []string, maxRunes int) string { return out.String() } -// generateChatSummary generates a 1-3 sentence whole-chat summary from a -// transcript. A blank or invalid result returns an error so callers preserve -// any existing summary rather than clearing it. +// generateChatSummary generates a whole-chat summary from a transcript as a +// one-sentence headline plus 2-4 bullets, serialized to markdown by +// formatChatSummaryMarkdown. A blank or invalid result returns an error so +// callers preserve any existing summary rather than clearing it. func generateChatSummary( ctx context.Context, model fantasy.LanguageModel, @@ -1123,7 +1132,7 @@ func generateChatSummary( result, genErr = object.Generate[generatedChatSummary](retryCtx, model, fantasy.ObjectCall{ Prompt: prompt, SchemaName: "chat_summary", - SchemaDescription: "Summarize the whole chat in 1-3 sentences.", + SchemaDescription: "Summarize the whole chat as a one-sentence headline plus 2-4 short bullets.", MaxOutputTokens: &maxOutputTokens, }) return genErr @@ -1136,22 +1145,94 @@ func generateChatSummary( return "", usage, xerrors.Errorf("generate chat summary: %w", err) } - summary := normalizeShortTextOutput(result.Object.Summary) + summary := generatedChatSummary{ + Headline: normalizeSummaryField(result.Object.Headline), + Bullets: normalizeSummaryBullets(result.Object.Bullets), + } if err := validateGeneratedChatSummary(summary); err != nil { return "", result.Usage, err } - return summary, result.Usage, nil + return formatChatSummaryMarkdown(summary.Headline, summary.Bullets), result.Usage, nil } -func validateGeneratedChatSummary(summary string) error { - if summary == "" { - return xerrors.New("generated chat summary was empty") +// normalizeSummaryField collapses internal whitespace so a field stays on one +// line, and strips surrounding quotes. Unlike normalizeShortTextOutput it +// preserves backticks, so a field ending in an inline code span keeps a +// balanced pair. +func normalizeSummaryField(text string) string { + text = strings.TrimSpace(text) + if text == "" { + return "" } - if len([]rune(summary)) > summaryMaxRunes { - return xerrors.Errorf("generated chat summary exceeded %d runes", summaryMaxRunes) + + text = strings.Trim(text, "\"'") + return strings.Join(strings.Fields(text), " ") +} + +// normalizeSummaryBullets normalizes each bullet and drops the ones that +// normalize to nothing, so blank model output does not render as an empty +// list item. +func normalizeSummaryBullets(bullets []string) []string { + normalized := make([]string, 0, len(bullets)) + for _, bullet := range bullets { + if bullet = normalizeSummaryField(bullet); bullet != "" { + normalized = append(normalized, bullet) + } } - if countSentenceTerminators(summary) > summaryMaxSentences { - return xerrors.Errorf("generated chat summary exceeded %d sentences", summaryMaxSentences) + return normalized +} + +// formatChatSummaryMarkdown renders the stored summary as a headline paragraph +// followed by an optional bullet list, separated by a blank line. Both summary +// producers go through here so the stored format stays consistent. +func formatChatSummaryMarkdown(headline string, bullets []string) string { + headline = strings.TrimSpace(headline) + if len(bullets) == 0 { + return headline + } + + var out strings.Builder + _, _ = out.WriteString(headline) + _, _ = out.WriteString("\n") + for _, bullet := range bullets { + if bullet = strings.TrimSpace(bullet); bullet != "" { + _, _ = out.WriteString("\n- ") + _, _ = out.WriteString(bullet) + } + } + return strings.TrimSpace(out.String()) +} + +// validateGeneratedChatSummary checks the structured fields before they are +// serialized. Validating here rather than over the rendered markdown keeps the +// sentence cap meaningful: bullets routinely omit trailing punctuation, so a +// sentence count over the serialized string would pass almost anything. +func validateGeneratedChatSummary(summary generatedChatSummary) error { + if summary.Headline == "" { + return xerrors.New("generated chat summary headline was empty") + } + if len([]rune(summary.Headline)) > summaryHeadlineMaxRunes { + return xerrors.Errorf("generated chat summary headline exceeded %d runes", summaryHeadlineMaxRunes) + } + if countSentenceTerminators(summary.Headline) > summaryHeadlineMaxSentences { + return xerrors.Errorf("generated chat summary headline exceeded %d sentences", summaryHeadlineMaxSentences) + } + if len(summary.Bullets) < summaryMinBullets || len(summary.Bullets) > summaryMaxBullets { + return xerrors.Errorf( + "generated chat summary had %d bullets, want %d to %d", + len(summary.Bullets), summaryMinBullets, summaryMaxBullets, + ) + } + for _, bullet := range summary.Bullets { + if len([]rune(bullet)) > summaryBulletMaxRunes { + return xerrors.Errorf("generated chat summary bullet exceeded %d runes", summaryBulletMaxRunes) + } + if strings.ContainsAny(bullet, "\n\r") { + return xerrors.New("generated chat summary bullet contained a newline") + } + } + if rendered := formatChatSummaryMarkdown(summary.Headline, summary.Bullets); len([]rune(rendered)) > summaryMaxRunes { + return xerrors.Errorf("generated chat summary exceeded %d runes", summaryMaxRunes) } return nil } diff --git a/coderd/x/chatd/summarygen_internal_test.go b/coderd/x/chatd/summarygen_internal_test.go index 824dbb3c6d..0a1908228b 100644 --- a/coderd/x/chatd/summarygen_internal_test.go +++ b/coderd/x/chatd/summarygen_internal_test.go @@ -225,10 +225,98 @@ func TestShouldGenerateChatSummary(t *testing.T) { func TestValidateGeneratedChatSummary(t *testing.T) { t.Parallel() - require.Error(t, validateGeneratedChatSummary("")) - require.Error(t, validateGeneratedChatSummary(strings.Repeat("a", summaryMaxRunes+1))) - require.Error(t, validateGeneratedChatSummary("One. Two. Three. Four. Five. Six. Seven.")) - require.NoError(t, validateGeneratedChatSummary("Implemented the summary feature. Added tests.")) + validBullets := []string{"Traced the race in `cache.go`", "Added a regression test"} + + tests := []struct { + name string + summary generatedChatSummary + wantErr bool + }{ + { + name: "Valid", + summary: generatedChatSummary{Headline: "Fixed the flaky CI job.", Bullets: validBullets}, + }, + { + name: "EmptyHeadline", + summary: generatedChatSummary{Bullets: validBullets}, + wantErr: true, + }, + { + name: "HeadlineTooLong", + summary: generatedChatSummary{ + Headline: strings.Repeat("a", summaryHeadlineMaxRunes+1), + Bullets: validBullets, + }, + wantErr: true, + }, + { + name: "HeadlineTooManySentences", + summary: generatedChatSummary{ + Headline: "One. Two. Three.", + Bullets: validBullets, + }, + wantErr: true, + }, + { + name: "TooFewBullets", + summary: generatedChatSummary{Headline: "Fixed it.", Bullets: []string{"Only one"}}, + wantErr: true, + }, + { + name: "NoBullets", + summary: generatedChatSummary{Headline: "Fixed it."}, + wantErr: true, + }, + { + name: "TooManyBullets", + summary: generatedChatSummary{ + Headline: "Fixed it.", + Bullets: []string{"One", "Two", "Three", "Four", "Five"}, + }, + wantErr: true, + }, + { + name: "BulletTooLong", + summary: generatedChatSummary{ + Headline: "Fixed it.", + Bullets: []string{"Fine", strings.Repeat("b", summaryBulletMaxRunes+1)}, + }, + wantErr: true, + }, + { + name: "BulletWithNewline", + summary: generatedChatSummary{ + Headline: "Fixed it.", + Bullets: []string{"Fine", "Broken\nacross lines"}, + }, + wantErr: true, + }, + { + name: "SerializedTooLong", + summary: generatedChatSummary{ + Headline: strings.Repeat("a", summaryHeadlineMaxRunes), + Bullets: []string{ + strings.Repeat("b", summaryBulletMaxRunes), + strings.Repeat("c", summaryBulletMaxRunes), + strings.Repeat("d", summaryBulletMaxRunes), + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := validateGeneratedChatSummary(tt.summary) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } } func TestCountSentenceTerminators(t *testing.T) { @@ -239,10 +327,96 @@ func TestCountSentenceTerminators(t *testing.T) { require.Equal(t, 3, countSentenceTerminators("One. Two! Three?")) require.Equal(t, 0, countSentenceTerminators("auth.rbac.Policy")) - // Dotted identifiers must not push a valid summary over the sentence cap. - require.NoError(t, validateGeneratedChatSummary( - "Refactored pkg.cmd.server and auth.rbac.Policy in main.go and util.go. Added coverage in foo_test.go.", - )) + // Dotted identifiers must not push a valid headline over the sentence cap. + require.NoError(t, validateGeneratedChatSummary(generatedChatSummary{ + Headline: "Refactored pkg.cmd.server and auth.rbac.Policy in main.go and util.go.", + Bullets: []string{"Updated call sites", "Added coverage in foo_test.go"}, + })) +} + +func TestNormalizeSummaryField(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + text string + want string + }{ + {name: "Empty", text: " ", want: ""}, + {name: "CollapsesNewlines", text: "Fixed the race\nin cache.go", want: "Fixed the race in cache.go"}, + {name: "CollapsesRuns", text: "Fixed the\t\trace", want: "Fixed the race"}, + {name: "StripsSurroundingQuotes", text: `"Fixed the race"`, want: "Fixed the race"}, + { + // normalizeShortTextOutput would strip this trailing backtick and + // leave an unbalanced inline code span. + name: "PreservesTrailingBacktick", + text: "Fixed `cache.go`", + want: "Fixed `cache.go`", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + require.Equal(t, tt.want, normalizeSummaryField(tt.text)) + }) + } +} + +func TestNormalizeSummaryBullets(t *testing.T) { + t.Parallel() + + require.Equal(t, + []string{"First bullet", "Second bullet"}, + normalizeSummaryBullets([]string{" First\nbullet ", " ", "Second bullet", ""}), + ) + require.Empty(t, normalizeSummaryBullets(nil)) +} + +func TestFormatChatSummaryMarkdown(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + headline string + bullets []string + want string + }{ + { + name: "HeadlineOnly", + headline: "Fixed the flaky CI job.", + want: "Fixed the flaky CI job.", + }, + { + // A blank line must separate the paragraph from the list, or + // CommonMark folds the first bullet into the headline paragraph. + name: "HeadlineAndBullets", + headline: "Fixed the flaky CI job.", + bullets: []string{"Traced the race", "Added a test"}, + want: "Fixed the flaky CI job.\n\n- Traced the race\n- Added a test", + }, + { + name: "DropsEmptyBullets", + headline: "Fixed it.", + bullets: []string{"Kept", " ", "Also kept"}, + want: "Fixed it.\n\n- Kept\n- Also kept", + }, + { + name: "AllBulletsEmptyKeepsHeadline", + headline: "Fixed it.", + bullets: []string{" ", ""}, + want: "Fixed it.", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + require.Equal(t, tt.want, formatChatSummaryMarkdown(tt.headline, tt.bullets)) + }) + } } func TestSubagentReportSummarySnippet(t *testing.T) { diff --git a/site/src/pages/AgentsPage/components/ChatSummary.stories.tsx b/site/src/pages/AgentsPage/components/ChatSummary.stories.tsx index d680df898e..c31ebd1982 100644 --- a/site/src/pages/AgentsPage/components/ChatSummary.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatSummary.stories.tsx @@ -1,13 +1,31 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; -import { expect, within } from "storybook/test"; +import { useState } from "react"; +import { expect, userEvent, waitFor, within } from "storybook/test"; import { ChatSummary } from "./ChatSummary"; +const MARKDOWN_SUMMARY = [ + "Investigated the flaky CI job in `coderd/x/chatd` and landed a fix.", + "", + "- Traced the failure to a cache-layer race in `chatd.go`", + "- Added a regression test covering the race", + "- Opened PR #26649", +].join("\n"); + +const LONG_SUMMARY = [ + "Audited the whole chat pipeline and shipped a batch of fixes.", + "", + ...Array.from( + { length: 12 }, + (_, i) => + `- Reviewed subsystem number ${i + 1} and applied the corresponding fix so the behaviour matches the specification`, + ), +].join("\n"); + const meta: Meta = { title: "pages/AgentsPage/ChatSummary", component: ChatSummary, args: { - summary: - "Investigated the flaky CI job, traced it to a race in the cache layer, and added a regression test.", + summary: MARKDOWN_SUMMARY, createdAt: "2024-05-01T12:00:00Z", updatedAt: "2024-05-02T15:30:00Z", costMicros: 1_250_000, @@ -39,6 +57,149 @@ export const WithSummary: Story = { }, }; +export const HeadlineAndBullets: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect( + canvas.getByText(/Investigated the flaky CI job/), + ).toBeInTheDocument(); + + const list = canvas.getByRole("list"); + await expect(within(list).getAllByRole("listitem")).toHaveLength(3); + + // Identifiers wrapped in backticks render as inline code, not literal + // backticks. + await expect(canvas.getByText("chatd.go")).toBeInTheDocument(); + await expect(canvas.queryByText(/`/)).not.toBeInTheDocument(); + + // Short content fits the bound, so no toggle is offered. + await expect( + canvas.queryByRole("button", { name: "Show more" }), + ).not.toBeInTheDocument(); + }, +}; + +// Summaries generated before the structured format are plain prose. They must +// still render, since there is no backfill. +export const LegacyProseSummary: Story = { + args: { + summary: + "Investigated the flaky CI job, traced it to a race in the cache layer, and added a regression test.", + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect( + canvas.getByText(/traced it to a race in the cache layer/), + ).toBeInTheDocument(); + await expect(canvas.queryByRole("list")).not.toBeInTheDocument(); + }, +}; + +// A legacy prose summary that happens to start with "1. " parses as an ordered +// list. `ol` is allowlisted so the items keep a list parent instead of +// rendering as orphan `li` elements. +export const LegacyOrderedList: Story = { + args: { summary: "1. Fixed the race\n2. Added a test" }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const list = canvas.getByRole("list"); + await expect(list.tagName).toBe("OL"); + await expect(within(list).getAllByRole("listitem")).toHaveLength(2); + }, +}; + +// Links are rendered as plain text. `overflow-hidden` clips the collapsed +// content visually only, so a mounted anchor below the bound would still be +// reachable by keyboard and screen readers while invisible. +export const LinksRenderAsPlainText: Story = { + args: { + summary: + "Investigated the failure in [PR #26649](https://example.com/pr) and fixed it.", + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect(canvas.getByText(/PR #26649/)).toBeInTheDocument(); + await expect(canvas.queryByRole("link")).not.toBeInTheDocument(); + }, +}; + +// The generation prompt preserves identifiers and wraps them in backticks, so +// a single token can be wider than the panel. It has to wrap, because the +// collapsed bound only reveals its toggle for vertical overflow. +export const LongIdentifierWraps: Story = { + args: { + summary: + "Fixed `TestValidateGeneratedChatSummaryHeadlineExceedsBothTheRuneAndSentenceCaps` in `coderd/x/chatd/summarygen_internal_test.go`.", + }, + // Pinned narrower than the panel's 360px minimum so the identifier cannot + // fit on one line. The wrapped layout itself is covered by visual + // regression snapshots; per FE10 the assertion here stays semantic rather + // than measuring geometry. + decorators: [ + (Story) => ( +
+ +
+ ), + ], + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect( + canvas.getByText( + "TestValidateGeneratedChatSummaryHeadlineExceedsBothTheRuneAndSentenceCaps", + ), + ).toBeVisible(); + }, +}; + +// A cache update can replace the summary in place, without remounting the +// panel, so the overflow toggle has to re-evaluate on new content. +export const SummaryReplacedInPlace: Story = { + render: (args) => { + const [summary, setSummary] = useState(MARKDOWN_SUMMARY); + return ( +
+ + +
+ ); + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect( + canvas.queryByRole("button", { name: "Show more" }), + ).not.toBeInTheDocument(); + + await userEvent.click( + canvas.getByRole("button", { name: "Simulate update" }), + ); + + await waitFor(async () => { + await expect( + canvas.getByRole("button", { name: "Show more" }), + ).toBeInTheDocument(); + }); + }, +}; + +export const LongSummaryExpands: Story = { + args: { summary: LONG_SUMMARY }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + + const showMore = await canvas.findByRole("button", { name: "Show more" }); + await userEvent.click(showMore); + + await waitFor(async () => { + await expect( + canvas.getByRole("button", { name: "Show less" }), + ).toBeInTheDocument(); + }); + }, +}; + export const NoSummary: Story = { args: { summary: null }, play: async ({ canvasElement }) => { diff --git a/site/src/pages/AgentsPage/components/ChatSummary.tsx b/site/src/pages/AgentsPage/components/ChatSummary.tsx index 41ca7328bc..121620853b 100644 --- a/site/src/pages/AgentsPage/components/ChatSummary.tsx +++ b/site/src/pages/AgentsPage/components/ChatSummary.tsx @@ -1,10 +1,27 @@ -import type { FC, ReactNode } from "react"; +import { + type FC, + type ReactNode, + useLayoutEffect, + useRef, + useState, +} from "react"; +import { InlineMarkdown } from "#/components/Markdown/InlineMarkdown"; import { Skeleton } from "#/components/Skeleton/Skeleton"; import { formatCostMicros } from "#/utils/currency"; import { DATE_FORMAT, formatDateTime } from "#/utils/time"; const EMPTY_VALUE = "-"; +/** Compact list spacing that keeps markers inside the narrow summary column. */ +const LIST_CLASSES = "my-2 flex flex-col gap-1 pl-5"; + +/** + * How long resizing must settle before overflow is remeasured. Dragging the + * right panel resizes the summary every frame, and the overflow verdict is a + * boolean that flips at most a couple of times per drag. + */ +const RESIZE_SETTLE_MS = 100; + interface ChatSummaryProps { summary: string | null; createdAt: string; @@ -40,9 +57,7 @@ export const ChatSummary: FC = ({ return (
{trimmedSummary ? ( -

- {trimmedSummary} -

+ ) : (

{isSubagent ? "Summary pending agent completion." : "No summary yet."} @@ -88,6 +103,126 @@ export const ChatSummary: FC = ({ ); }; +interface ChatSummaryBodyProps { + summary: string; +} + +/** + * Renders the stored summary markdown (a headline paragraph plus an optional + * bullet list) inside a bounded box, revealing a toggle only when the content + * actually overflows. `max-height` is used instead of `line-clamp` because + * `line-clamp` relies on `display: -webkit-box`, which clamps unreliably once + * the content contains nested block children such as `

  • `. + * + * Overflow is measured on the clamped box but observed on an inner unclamped + * element, so both panel resizes and in-place summary updates re-evaluate the + * toggle. + */ +const ChatSummaryBody: FC = ({ summary }) => { + const clampRef = useRef(null); + const contentRef = useRef(null); + const [isExpanded, setIsExpanded] = useState(false); + const [isOverflowing, setIsOverflowing] = useState(false); + + useLayoutEffect(() => { + // Overflow only needs measuring while collapsed. Skipping the expanded + // state preserves the verdict computed while collapsed, so the toggle + // stays visible; collapsing reruns this effect and remeasures. + if (isExpanded) { + return; + } + const clamp = clampRef.current; + const content = contentRef.current; + if (!clamp || !content) { + return; + } + const measure = () => + setIsOverflowing(clamp.scrollHeight > clamp.clientHeight); + measure(); + + if (typeof ResizeObserver === "undefined") { + return; + } + // Observe the inner element rather than the clamped one. The clamped box + // stops growing at its max height, so a summary that arrives via a cache + // update while the box is already pinned there would resize nothing and + // stay clipped with no toggle. The inner element is unclamped, so its + // height tracks the content and its width tracks the resizable panel. + // + // Debounce rather than schedule on an animation frame: observer + // callbacks are already delivered at most once per frame, so a frame + // callback would defer the same work instead of doing less of it. + // Settling skips the layout reads entirely while a drag is in flight. + let settleTimeout: ReturnType | undefined; + const observer = new ResizeObserver(() => { + clearTimeout(settleTimeout); + settleTimeout = setTimeout(measure, RESIZE_SETTLE_MS); + }); + observer.observe(content); + return () => { + clearTimeout(settleTimeout); + observer.disconnect(); + }; + }, [isExpanded]); + + return ( +
    +
    +
    + ( +

    {children}

    + ), + ul: ({ children }) => ( +
      {children}
    + ), + ol: ({ children }) => ( +
      {children}
    + ), + li: ({ children }) => ( +
  • {children}
    • + ), + // Render link text without an anchor. Clipping below the + // collapsed bound is visual only, so a mounted anchor would + // stay reachable by keyboard and screen readers while + // invisible. Summaries are generated text, not navigation. + a: ({ children }) => <>{children}, + }} + > + {summary} + +
+ + + {(isOverflowing || isExpanded) && ( + + )} + + ); +}; + interface ChatSummaryRowProps { label: string; children: ReactNode; diff --git a/site/src/pages/AgentsPage/components/ChatSummaryPanel.stories.tsx b/site/src/pages/AgentsPage/components/ChatSummaryPanel.stories.tsx index 896ee03d98..0cf15c6143 100644 --- a/site/src/pages/AgentsPage/components/ChatSummaryPanel.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatSummaryPanel.stories.tsx @@ -1,6 +1,6 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; -import type { FC } from "react"; -import { expect, spyOn, waitFor, within } from "storybook/test"; +import { type FC, useState } from "react"; +import { expect, spyOn, userEvent, waitFor, within } from "storybook/test"; import { API } from "#/api/api"; import type * as TypesGen from "#/api/typesGenerated"; import { MockChat } from "#/testHelpers/chatEntities"; @@ -8,6 +8,17 @@ import { withDashboardProvider } from "#/testHelpers/storybook"; import { ChatSummaryPanel } from "./ChatSummaryPanel"; const ROOT_CHAT_ID = "root-chat-id"; +const OTHER_CHAT_ID = "other-chat-id"; + +const LONG_SUMMARY = [ + "Audited the whole chat pipeline and shipped a batch of fixes.", + "", + ...Array.from( + { length: 12 }, + (_, i) => + `- Reviewed subsystem number ${i + 1} and applied the corresponding fix so the behaviour matches the specification`, + ), +].join("\n"); const mockCost: TypesGen.ChatCost = { chat_id: MockChat.id, @@ -71,17 +82,24 @@ type Story = StoryObj; export const WithSummary: Story = { beforeEach: () => mockRequests({ - summary: - "Investigated the flaky CI job, traced it to a cache-layer race, and added a regression test.", + summary: [ + "Investigated the flaky CI job and landed a fix.", + "", + "- Traced it to a cache-layer race in `chatd.go`", + "- Added a regression test covering the race", + ].join("\n"), }), play: async ({ canvasElement }) => { const canvas = within(canvasElement); await waitFor(() => { expect( - canvas.getByText(/traced it to a cache-layer race/), + canvas.getByText(/Traced it to a cache-layer race/), ).toBeInTheDocument(); expect(canvas.getByText("$1.25")).toBeInTheDocument(); }); + expect( + within(canvas.getByRole("list")).getAllByRole("listitem"), + ).toHaveLength(2); }, }; @@ -156,3 +174,72 @@ export const GatewayUnavailable: Story = { expect(API.experimental.getChatCost).not.toHaveBeenCalled(); }, }; + +// Navigating between chats swaps `chatId` on the existing panel instead of +// remounting it, so the disclosure state must not carry over. Without a reset +// the next chat renders fully expanded behind a "Show less" button, even when +// its own summary fits. +export const ExpansionResetsBetweenChats: Story = { + beforeEach: () => { + spyOn(API.experimental, "getChat").mockImplementation(async (chatId) => ({ + ...MockChat, + id: chatId, + summary: chatId === MockChat.id ? LONG_SUMMARY : "A summary that fits.", + })); + spyOn(API.experimental, "getChatCost").mockResolvedValue(mockCost); + }, + render: (args) => { + const [chatId, setChatId] = useState(MockChat.id); + return ( +
+ + +
+ ); + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const switchChat = canvas.getByRole("button", { name: "Switch chat" }); + const expand = async () => { + const showMore = await canvas.findByRole("button", { + name: "Show more", + }); + await userEvent.click(showMore); + await expect( + canvas.getByRole("button", { name: "Show less" }), + ).toBeInTheDocument(); + }; + + // Warm both chats in the query cache first. While a chat is still + // uncached the panel renders nothing, which unmounts the summary and + // resets the disclosure state as a side effect. Once cached, the data is + // returned synchronously and the panel keeps the same summary instance + // across the switch, which is where the state can leak. + await expand(); + await userEvent.click(switchChat); + await waitFor(async () => { + await expect(canvas.getByText("A summary that fits.")).toBeVisible(); + }); + await userEvent.click(switchChat); + + await expand(); + await userEvent.click(switchChat); + await waitFor(async () => { + await expect(canvas.getByText("A summary that fits.")).toBeVisible(); + }); + + // The switched-to summary fits, so it must not offer to collapse. + await expect( + canvas.queryByRole("button", { name: "Show less" }), + ).not.toBeInTheDocument(); + }, +}; diff --git a/site/src/pages/AgentsPage/components/ChatSummaryPanel.tsx b/site/src/pages/AgentsPage/components/ChatSummaryPanel.tsx index 7f4e0b0bed..8bc9c5a21a 100644 --- a/site/src/pages/AgentsPage/components/ChatSummaryPanel.tsx +++ b/site/src/pages/AgentsPage/components/ChatSummaryPanel.tsx @@ -32,6 +32,11 @@ export const ChatSummaryPanel: FC = ({ } else if (chatData) { content = (