fix(web): pull request replies can be added to chat

apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx

@@ -27,6 +27,7 @@ import {
   type PullRequestSummary,
   resolveEnvironmentMachineKind,
   type ScopedThreadRef,
+  pullRequestConversationThreads,
 } from "@t3tools/contracts";
 import {
   ArrowDownUpIcon,
@@ -1144,14 +1145,14 @@ export function PullRequestDetailPanel({
 
   /**
    * Brings a remark into the composer beside this panel, with the pull request it belongs to,
-   * and nothing else: what to ask about it is the reader's to write. A remark on a line travels
-   * as its whole conversation, the way fixing one does.
+   * and nothing else: what to ask about it is the reader's to write. A remark in a thread travels
+   * as its whole conversation, so a reply arrives with what it answers.
    */
   const addCommentToChat = (subject: PullRequestChatSubject) => {
     if (!detail || attachTarget === null) return;
     const thread =
       subject.kind === "comment"
-        ? detail.reviewThreads.find((candidate) =>
+        ? pullRequestConversationThreads(detail).find((candidate) =>
             candidate.comments.some((comment) => comment.id === subject.comment.id),
           )
         : undefined;

apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx

@@ -230,15 +230,14 @@ function CommentBody({
 }
 
 /**
- * What Add to chat hands over for a remark: a remark on a line goes as its whole conversation,
- * the way fixing one does. Null when there are no words in it to talk about.
+ * What Add to chat hands over for a remark: a remark in a thread goes as its whole conversation.
+ * Null when there are no words in it to talk about.
  */
 function chatSubject(
   comment: PullRequestComment,
   thread: PullRequestConversationThread | undefined,
 ): PullRequestChatSubject | null {
-  // An overview thread has no line to attach, so it goes as its opening remark, as fixing it does.
-  if (thread !== undefined && thread.path !== null) {
+  if (thread !== undefined) {
     return thread.comments.some((entry) => visibleBody(entry.body) !== null)
       ? { kind: "thread", thread }
       : null;
@@ -732,8 +731,7 @@ export function PullRequestSummaryTab({
             ? null
             : { kind: "comment", comment }
           : { kind: "thread", thread };
-    // A reply goes with its conversation, from the card that heads it.
-    const chat = reply ? null : chatSubject(comment, thread);
+    const chat = chatSubject(comment, thread);
     return (
       <article
         key={`${detail.url}:${comment.id}`}

apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx

@@ -235,16 +235,13 @@ function ConversationCard({
   onOpen,
   reactions,
   showPath = true,
-  reply = false,
 }: {
   event: PullRequestTimelineEvent;
   editable: PullRequestComment | null;
   cwd: string;
   onOpen: (url: string) => void;
   reactions: ReactionSurface;
   showPath?: boolean;
-  /** A reply goes with its conversation, from the card that heads it. */
-  reply?: boolean;
 }) {
   const canReact = reactions.canReact(event);
   const [editing, setEditing] = useState(false);
@@ -303,7 +300,7 @@ function ConversationCard({
           {editable !== null && !editing ? (
             <PullRequestEditButton aria-label="Edit comment" onClick={() => setEditing(true)} />
           ) : null}
-          {event.body && event.comment && !reply ? (
+          {event.body && event.comment ? (
             <AddToChatButton comment={event.comment} reactions={reactions} />
           ) : null}
           {canReact || event.reactions.length > 0 ? (
@@ -551,7 +548,6 @@ function ReviewThreadEvent({
                   onOpen={onOpen}
                   reactions={reactions}
                   showPath={false}
-                  reply
                 />
               ))}
             </div>

apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts

@@ -1,5 +1,6 @@
 import { SourceControlProviderKind } from "@t3tools/contracts";
 import {
+  COMPOSER_CONTEXT_REVIEW_TEXT_MAX_CHARS,
   GitCommandError,
   ProjectId,
   type PullRequestCheck,
@@ -1100,6 +1101,116 @@ describe("adding a remark to the chat", () => {
     expect(context?.text).toBe("octocat: Why not reuse the existing parser?");
   });
 
+  it("brings an overview thread's replies along with its opening remark", () => {
+    const context = buildPullRequestCommentContext(pullRequest, {
+      kind: "thread",
+      thread: {
+        id: "t1",
+        path: null,
+        line: null,
+        side: "right",
+        isResolved: false,
+        isOutdated: false,
+        comments: [
+          remark("c1", "Does this skip the prompt?"),
+          {
+            ...remark("c2", "One concern remains."),
+            author: { login: "coderabbit", name: null, avatarUrl: null },
+          },
+        ],
+      },
+    });
+
+    expect(context && reviewCommentContextLabel(context)).toBe("#42 @octocat thread");
+    expect(context?.text).toBe(
+      "octocat: Does this skip the prompt?\ncoderabbit: One concern remains.",
+    );
+  });
+
+  it("keeps a reply when the remark it answers runs past the length bound", () => {
+    const context = buildPullRequestCommentContext(pullRequest, {
+      kind: "thread",
+      thread: {
+        id: "t1",
+        path: "src/app.ts",
+        line: 3,
+        side: "right",
+        isResolved: false,
+        isOutdated: false,
+        comments: [remark("c1", "x".repeat(5_000)), remark("c2", "One concern remains.")],
+      },
+    });
+
+    expect(context?.text).toMatch(/\.\.\.\noctocat: One concern remains\.$/u);
+  });
+
+  it("puts a remark's visible text ahead of its collapsed details", () => {
+    const context = buildPullRequestCommentContext(pullRequest, {
+      kind: "comment",
+      comment: remark(
+        "c1",
+        `<details title="open logs">\n<summary>Analysis chain</summary>\n\n${"script ".repeat(400)}\n</details>\n\nOne concern remains.`,
+      ),
+    });
+
+    expect(context?.text).toMatch(/^octocat: One concern remains\.\n\n<details title/u);
+  });
+
+  it("closes a details section whose closing tag has text after it", () => {
+    const context = buildPullRequestCommentContext(pullRequest, {
+      kind: "comment",
+      comment: remark("c1", `<details>\n${"script ".repeat(400)}</details> Tail.\nVisible.`),
+    });
+
+    expect(context?.text).toMatch(/^octocat: Tail\.\nVisible\./u);
+  });
+
+  it("gives way to the newest replies when a thread outgrows what the composer sends", () => {
+    const comments = Array.from({ length: 20 }, (_, index) =>
+      remark(`c${index}`, `${index} ${"x".repeat(990)}`),
+    );
+    const context = buildPullRequestCommentContext(pullRequest, {
+      kind: "thread",
+      thread: {
+        id: "t1",
+        path: null,
+        line: null,
+        side: "right",
+        isResolved: false,
+        isOutdated: false,
+        comments,
+        nextCommentsCursor: "more",
+      },
+    });
+
+    expect(context?.text.length).toBeLessThanOrEqual(COMPOSER_CONTEXT_REVIEW_TEXT_MAX_CHARS);
+    expect(context?.text).toMatch(/^octocat: 0 /u);
+    expect(context?.text).toMatch(/\n\(5 earlier replies omitted\.\)\noctocat: 6 /u);
+    expect(context?.text).toMatch(/octocat: 19 x+\.\.\.\n\(More replies are on the host/u);
+  });
+
+  it("keeps a details section that starts open in place", () => {
+    const context = buildPullRequestCommentContext(pullRequest, {
+      kind: "comment",
+      comment: remark(
+        "c1",
+        `<details open>\n<summary>Finding</summary>\n\nRename x.\n</details>\n\n${"Context. ".repeat(200)}`,
+      ),
+    });
+
+    expect(context?.text).toMatch(/^octocat: <details open>/u);
+  });
+
+  it("leaves a details tag shown inside code where it is", () => {
+    const body = `\`\`\`html\n<details>\n<summary>Example</summary>\n</details>\n\`\`\`\n${"Explanation. ".repeat(100)}`;
+    const context = buildPullRequestCommentContext(pullRequest, {
+      kind: "comment",
+      comment: remark("c1", body),
+    });
+
+    expect(context?.text).toMatch(/^octocat: ```html\n<details>/u);
+  });
+
   it("keeps every added remark when a later hand-off lands", () => {
     const added = ["c1", "c2"].flatMap(
       (id) =>

apps/web/src/components/pullRequest/pullRequestDetail.logic.ts

@@ -14,6 +14,7 @@ import {
 } from "@t3tools/client-runtime/source-control-clients";
 import * as Schema from "effect/Schema";
 import {
+  COMPOSER_CONTEXT_REVIEW_TEXT_MAX_CHARS,
   GitCommandError,
   PullRequestDetail,
   PullRequestOperationError,
@@ -702,6 +703,47 @@ function readableBody(raw: string): string | null {
   return readable === "" ? null : readable;
 }
 
+/**
+ * A body too long for the bound, with collapsed `<details>` sections moved after the text a
+ * reader sees so the bound keeps that text. Details shown inside code are source and stay put.
+ */
+function visibleTextFirst(body: string): string {
+  if (body.length <= FINDING_BODY_MAX_LENGTH) return body;
+  const codeLines = new Set<number>();
+  const visit = (node: MdastNodes) => {
+    if (node.type === "code" && node.position !== undefined) {
+      for (let line = node.position.start.line; line <= node.position.end.line; line += 1) {
+        codeLines.add(line);
+      }
+    }
+    if ("children" in node) for (const child of node.children) visit(child);
+  };
+  visit(fromMarkdown(body));
+  const shown: string[] = [];
+  const folded: string[] = [];
+  // Whether each open section starts collapsed; `<details open>` reads as visible text.
+  const sections: boolean[] = [];
+  for (const [index, line] of body.split("\n").entries()) {
+    const inCode = codeLines.has(index + 1);
+    const opener = inCode ? null : /^\s*<details(?=[\s>])([^>]*)/iu.exec(line);
+    // Quoted values are dropped first so `title="open logs"` does not read as the attribute.
+    const attributes = opener?.[1]?.replace(/"[^"]*"|'[^']*'/gu, "") ?? "";
+    if (opener) sections.push(!/(?:^|\s)open(?:[\s=/]|$)/iu.test(attributes));
+    const closer = inCode || sections.length === 0 ? null : /<\/details>/iu.exec(line);
+    if (closer === null) {
+      (sections.includes(true) ? folded : shown).push(line);
+      continue;
+    }
+    // Text after the closing tag is outside the section, so it goes with what follows.
+    const end = closer.index + closer[0].length;
+    (sections.includes(true) ? folded : shown).push(line.slice(0, end));
+    sections.pop();
+    const after = line.slice(end).trim();
+    if (after !== "") (sections.includes(true) ? folded : shown).push(after);
+  }
+  return folded.length === 0 ? body : [...shown, "", ...folded].join("\n").trim();
+}
+
 /**
  * Merged wins over closed: GitHub sets both timestamps on a merge, and reporting "closed" for a
  * merged pull request would misstate what happened.
@@ -1250,8 +1292,34 @@ export function buildPullRequestContext(input: PullRequestContextMetadata): Revi
   return pullRequestContextComment(input, []);
 }
 
-/** A remark the reader can bring into the chat: a conversation on a line, or one comment. */
-export type PullRequestChatSubject = Extract<PullRequestFinding, { kind: "thread" | "comment" }>;
+/**
+ * A conversation's lines within what the composer sends. Earlier replies give way first, since a
+ * reader reaches for the latest one, and the agent is told what it is not seeing.
+ */
+function threadText(lines: ReadonlyArray<string>, moreOnHost: boolean): string {
+  const tail = moreOnHost ? ["(More replies are on the host and not included here.)"] : [];
+  let kept = lines;
+  let omitted = 0;
+  const join = () =>
+    [
+      ...kept.slice(0, 1),
+      ...(omitted > 0
+        ? [`(${omitted} earlier ${omitted === 1 ? "reply" : "replies"} omitted.)`]
+        : []),
+      ...kept.slice(1),
+      ...tail,
+    ].join("\n");
+  while (kept.length > 2 && join().length > COMPOSER_CONTEXT_REVIEW_TEXT_MAX_CHARS) {
+    kept = [kept[0]!, ...kept.slice(2)];
+    omitted += 1;
+  }
+  return join();
+}
+
+/** A remark the reader can bring into the chat: a whole conversation, or one comment. */
+export type PullRequestChatSubject =
+  | { readonly kind: "thread"; readonly thread: PullRequestConversationThread }
+  | { readonly kind: "comment"; readonly comment: PullRequestComment };
 
 /**
  * A remark or conversation brought into the chat to talk about, not to fix. Its id carries the
@@ -1262,30 +1330,45 @@ export function buildPullRequestCommentContext(
   subject: PullRequestChatSubject,
 ): ReviewCommentContext | null {
   const pullRequestNumber = pullRequest.number;
-  if (subject.kind === "thread") {
-    const context = reviewThreadContext(subject.thread, pullRequestNumber);
-    return context.text === ""
-      ? null
-      : { ...context, id: `pr-comment:${pullRequest.url}:thread:${subject.thread.id}` };
+  const comments = subject.kind === "thread" ? subject.thread.comments : [subject.comment];
+  // Bounded per comment so a long bot report cannot crowd the replies out of the thread.
+  const lines = comments.flatMap((comment) => {
+    const body = readableBody(comment.body);
+    return body === null
+      ? []
+      : [bounded(`${comment.author?.login ?? "ghost"}: ${visibleTextFirst(body)}`)];
+  });
+  const first = comments[0];
+  if (lines.length === 0 || first === undefined) return null;
+  const text = threadText(
+    lines,
+    subject.kind === "thread" &&
+      ((subject.thread.commentCount ?? 0) > comments.length ||
+        subject.thread.nextCommentsCursor !== undefined),
+  );
+  if (subject.kind === "thread" && subject.thread.path !== null) {
+    return {
+      ...reviewThreadContext(subject.thread, pullRequestNumber),
+      id: `pr-comment:${pullRequest.url}:thread:${subject.thread.id}`,
+      text,
+    };
   }
-  const comment = subject.comment;
-  const body = readableBody(comment.body);
-  if (body === null) return null;
-  const author = comment.author?.login ?? "ghost";
+  const path = subject.kind === "comment" ? subject.comment.path : null;
   return {
-    id: `pr-comment:${pullRequest.url}:comment:${comment.id}`,
+    id:
+      subject.kind === "thread"
+        ? `pr-comment:${pullRequest.url}:thread:${subject.thread.id}`
+        : `pr-comment:${pullRequest.url}:comment:${subject.comment.id}`,
     sectionId: `pull-request:${pullRequestNumber}`,
     sectionTitle: `PR #${pullRequestNumber} conversation`,
-    // The chip wears `filePath rangeLabel`: the file a remark names, or who wrote one that names none.
-    filePath: comment.path ?? `@${author}`,
+    // The chip wears `filePath rangeLabel`: the file a remark names, or who opened one that names none.
+    filePath: path ?? `@${first.author?.login ?? "ghost"}`,
     startIndex: 0,
     endIndex: 0,
-    rangeLabel: "comment",
-    text: bounded(`${author}: ${body}`),
+    rangeLabel: subject.kind === "thread" ? "thread" : "comment",
+    text,
     diff: "",
-    ...(comment.path === null
-      ? {}
-      : { fenceLanguage: inferReviewCommentFenceLanguage(comment.path) }),
+    ...(path === null ? {} : { fenceLanguage: inferReviewCommentFenceLanguage(path) }),
   };
 }