fix(share): render conversation markdown (#1750)

2026-07-18 16:17:10 +01:00
parent 547f312dbe
commit 5bd0e554bd
10 changed files with 213 additions and 159 deletions
@@ -24,6 +24,7 @@ require (
 	github.com/stretchr/testify v1.11.1
 	github.com/superfly/sprites-go v0.0.0-20260206213632-8176adff485b
 	github.com/tuzig/vt10x v0.0.0-20231206072048-370e51642bf7
+	github.com/yuin/goldmark v1.7.13
 	go.opentelemetry.io/otel v1.43.0
 	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.43.0
 	go.opentelemetry.io/otel/sdk v1.43.0
@@ -357,6 +357,8 @@ github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9de
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
+github.com/yuin/goldmark v1.7.13 h1:GPddIs617DnBLFFVJFgpo1aBfe/4xcvMc3SB5t/D0pA=
+github.com/yuin/goldmark v1.7.13/go.mod h1:ip/1k0VRfGynBgxOz0yCqHrbZXhcjxyuS66Brc7iBKg=
 go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU=
 go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8=
 go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw=
@@ -216,20 +216,42 @@ code, pre { font-family: var(--mono); }
 .text > * { margin: 0; }
 .text > * + * { margin-top: 8px; }
 .text p { line-height: 1.6; }
+.text h1, .text h2, .text h3, .text h4, .text h5, .text h6 {
+  margin: 14px 0 6px;
+  line-height: 1.3;
+}
+.text h1 { font-size: 1.5rem; }
+.text h2 { font-size: 1.3rem; }
+.text h3 { font-size: 1.12rem; }
+.text h4, .text h5, .text h6 { font-size: 1rem; }
+.text ul, .text ol { margin: 8px 0; padding-left: 24px; }
+.text li + li { margin-top: 3px; }
+.text blockquote {
+  margin: 8px 0;
+  padding-left: 12px;
+  border-left: 3px solid var(--accent);
+  color: var(--text-dim);
+}
+.text blockquote > :first-child { margin-top: 0; }
+.text blockquote > :last-child { margin-bottom: 0; }
+.text hr { margin: 16px 0; border: 0; border-top: 1px solid var(--border-strong); }
+.text table { display: block; max-width: 100%; overflow-x: auto; border-collapse: collapse; }
+.text th, .text td { padding: 6px 8px; border: 1px solid var(--border-strong); text-align: left; }
+.text th { background: var(--surface-2); }
 .group-user .text { font-size: 15px; }

-code.inline {
+.text :not(pre) > code {
  background: color-mix(in oklab, var(--foreground) 8%, transparent);
  padding: 1px 5px;
  border-radius: 4px;
  font-size: 0.9em;
 }
-.group-user code.inline {
+.group-user .text :not(pre) > code {
  background: color-mix(in oklab, white 22%, transparent);
  color: var(--primary-foreground);
 }

-pre.code {
+.text pre {
  margin: 4px 0;
  padding: 12px 14px;
  border-radius: var(--radius-lg);
@@ -240,7 +262,7 @@ pre.code {
  line-height: 1.5;
  color: var(--text);
 }
-.group-user pre.code {
+.group-user .text pre {
  background: color-mix(in oklab, black 25%, transparent);
  border-color: color-mix(in oklab, white 14%, transparent);
  color: var(--primary-foreground);
@@ -125,12 +125,38 @@ func TestBuildShareHTML_InlineBackticksAndFencedCode(t *testing.T) {
 		},
 	}
 	doc := BuildShareHTML(snap)
-	assertContains(t, doc, `<code class="inline">t.Cleanup</code>`)
-	assertContains(t, doc, `<pre class="code lang-go"><code>`)
+	assertContains(t, doc, `<code>t.Cleanup</code>`)
+	assertContains(t, doc, `<pre><code class="language-go">`)
 	assertContains(t, doc, "func TestX(t *testing.T)")
 	assertContains(t, doc, "<p>Done.</p>")
 }

+func TestBuildShareHTML_RendersMarkdownWithoutUnsafeHTML(t *testing.T) {
+	t.Parallel()
+	snap := &Snapshot{
+		Task: TaskMeta{Title: "T"},
+		Messages: []Message{
+			{Role: roleAssistant, Blocks: []Block{{
+				Kind: blockKindText,
+				Text: "## Summary\n\n**Pushed** successfully.\n\n- tests passed\n- lint passed\n\n[docs](https://example.com)\n\n![tracker](https://attacker.example/pixel)\n\n[![linked tracker](https://attacker.example/pixel)](https://linked.example/target)\n\n<script>alert('xss')</script>\n\n[bad](javascript:alert('xss'))\n\n[data](data:text/html;base64,PHNjcmlwdD4=)",
+			}}},
+		},
+	}
+
+	doc := BuildShareHTML(snap)
+
+	assertContains(t, doc, "<h2>Summary</h2>")
+	assertContains(t, doc, "<strong>Pushed</strong>")
+	assertContains(t, doc, "<li>tests passed</li>")
+	assertContains(t, doc, `<a href="https://example.com">docs</a>`)
+	if strings.Contains(doc, "<script>") || strings.Contains(doc, `href="javascript:`) || strings.Contains(doc, `href="data:`) {
+		t.Fatalf("unsafe markdown content leaked into rendered HTML:\n%s", doc)
+	}
+	if strings.Contains(doc, "<img") || strings.Contains(doc, "attacker.example") || strings.Contains(doc, "linked.example") {
+		t.Fatalf("external markdown image leaked into rendered HTML:\n%s", doc)
+	}
+}
+
 func TestBuildShareHTML_EmptyMessagesShowsPlaceholder(t *testing.T) {
 	t.Parallel()
 	doc := BuildShareHTML(&Snapshot{Task: TaskMeta{Title: "Empty"}})
@@ -157,24 +183,6 @@ func TestGroupMessages_RunsFusedAcrossEmptyMessages(t *testing.T) {
 	}
 }

-func TestInlineFormat_UnmatchedBacktickIsRendered(t *testing.T) {
-	t.Parallel()
-	got := inlineFormat("an unmatched ` here")
-	if !strings.Contains(got, "`") {
-		t.Fatalf("expected unmatched backtick preserved, got %q", got)
-	}
-}
-
-func TestSanitiseLang_StripsUnsafe(t *testing.T) {
-	t.Parallel()
-	if got := sanitiseLang("GO-1.21"); got != "go-121" {
-		t.Fatalf("got %q", got)
-	}
-	if got := sanitiseLang("<script>"); got != "script" {
-		t.Fatalf("got %q", got)
-	}
-}
-
 func TestRenderedURLForGist(t *testing.T) {
 	t.Parallel()
 	cases := []struct {
@@ -1,148 +1,54 @@
 package share

 import (
-	"fmt"
-	"html"
 	"strings"
+
+	"github.com/yuin/goldmark"
+	"github.com/yuin/goldmark/ast"
+	"github.com/yuin/goldmark/extension"
+	"github.com/yuin/goldmark/parser"
+	"github.com/yuin/goldmark/text"
+	"github.com/yuin/goldmark/util"
 )

-// writeHTMLText renders prose with minimal inline-markdown support: fenced
-// code blocks (```lang … ```) become <pre><code class="lang-X"> blocks,
-// and inline `code` spans become <code> tags. Anything else is escaped and
-// wrapped in <p>. We intentionally stop short of a full markdown parser —
-// the snapshot is plain agent output, not a publishing surface.
+var shareMarkdown = goldmark.New(
+	goldmark.WithExtensions(extension.GFM),
+	goldmark.WithParserOptions(
+		parser.WithASTTransformers(util.Prioritized(removeImagesTransformer{}, 1000)),
+	),
+)
+
+type removeImagesTransformer struct{}
+
+func (removeImagesTransformer) Transform(document *ast.Document, _ text.Reader, _ parser.Context) {
+	removeImages(document)
+}
+
+func removeImages(node ast.Node) {
+	for child := node.FirstChild(); child != nil; {
+		next := child.NextSibling()
+		if child.Kind() == ast.KindImage {
+			node.RemoveChild(node, child)
+		} else {
+			removeImages(child)
+			if child.Kind() == ast.KindLink && child.FirstChild() == nil {
+				node.RemoveChild(node, child)
+			}
+		}
+		child = next
+	}
+}
+
+// writeHTMLText renders conversation text as GitHub-flavoured Markdown.
+// Goldmark's safe default renderer omits embedded HTML and dangerous link
+// destinations, which matters because shared snapshots contain agent and user
+// supplied text.
 func writeHTMLText(b *strings.Builder, text string) {
 	t := strings.TrimSpace(text)
 	if t == "" {
 		return
 	}
 	b.WriteString("<div class=\"text\">\n")
-	for _, segment := range splitFencedCodeBlocks(t) {
-		if segment.fenced {
-			writeFencedCode(b, segment.lang, segment.body)
-			continue
-		}
-		writeProse(b, segment.body)
-	}
+	_ = shareMarkdown.Convert([]byte(t), b)
 	b.WriteString("</div>\n")
 }
-
-// textSegment is either a fenced code block or a run of prose paragraphs.
-type textSegment struct {
-	fenced bool
-	lang   string
-	body   string
-}
-
-// splitFencedCodeBlocks walks `text` line by line and pulls fenced code
-// blocks out as standalone segments so the surrounding prose can render
-// as paragraphs. Recognises only the triple-backtick form; tilde fences
-// and indent-based code blocks aren't worth the complexity here.
-func splitFencedCodeBlocks(text string) []textSegment {
-	var out []textSegment
-	var prose []string
-	var code []string
-	inFence := false
-	lang := ""
-	flushProse := func() {
-		if len(prose) > 0 {
-			out = append(out, textSegment{body: strings.Join(prose, "\n")})
-			prose = nil
-		}
-	}
-	for _, line := range strings.Split(text, "\n") {
-		if strings.HasPrefix(line, "```") {
-			if inFence {
-				out = append(out, textSegment{fenced: true, lang: lang, body: strings.Join(code, "\n")})
-				code = nil
-				lang = ""
-				inFence = false
-				continue
-			}
-			flushProse()
-			lang = strings.TrimSpace(strings.TrimPrefix(line, "```"))
-			inFence = true
-			continue
-		}
-		if inFence {
-			code = append(code, line)
-		} else {
-			prose = append(prose, line)
-		}
-	}
-	if inFence { // unterminated fence — render what we got so we don't drop content
-		out = append(out, textSegment{fenced: true, lang: lang, body: strings.Join(code, "\n")})
-	}
-	flushProse()
-	return out
-}
-
-func writeFencedCode(b *strings.Builder, lang, body string) {
-	body = strings.TrimRight(body, "\n")
-	cls := "code"
-	if lang != "" {
-		cls = "code lang-" + sanitiseLang(lang)
-	}
-	fmt.Fprintf(b, "<pre class=\"%s\"><code>%s</code></pre>\n", cls, html.EscapeString(body))
-}
-
-// sanitiseLang keeps language tags to a safe character set so they can go
-// into a class attribute without escaping. Anything outside [a-z0-9-_] is
-// dropped.
-func sanitiseLang(s string) string {
-	s = strings.ToLower(s)
-	var b strings.Builder
-	for _, r := range s {
-		switch {
-		case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-', r == '_':
-			b.WriteRune(r)
-		}
-	}
-	return b.String()
-}
-
-// writeProse renders a prose run as paragraphs split on blank lines, with
-// inline backtick spans converted to <code>. Soft newlines inside a
-// paragraph become <br>.
-func writeProse(b *strings.Builder, text string) {
-	text = strings.Trim(text, "\n")
-	if text == "" {
-		return
-	}
-	for _, para := range strings.Split(text, "\n\n") {
-		para = strings.TrimSpace(para)
-		if para == "" {
-			continue
-		}
-		fmt.Fprintf(b, "<p>%s</p>\n", inlineFormat(para))
-	}
-}
-
-// inlineFormat handles inline `code` and preserves soft line breaks as <br>.
-// Everything else is HTML-escaped.
-func inlineFormat(s string) string {
-	var b strings.Builder
-	for {
-		i := strings.Index(s, "`")
-		if i < 0 {
-			b.WriteString(htmlEscapeMultiline(s))
-			return b.String()
-		}
-		b.WriteString(htmlEscapeMultiline(s[:i]))
-		rest := s[i+1:]
-		end := strings.Index(rest, "`")
-		if end < 0 {
-			// Unmatched backtick — render the rest as-is so we don't drop chars.
-			b.WriteString(htmlEscapeMultiline(s[i:]))
-			return b.String()
-		}
-		fmt.Fprintf(&b, "<code class=\"inline\">%s</code>", html.EscapeString(rest[:end]))
-		s = rest[end+1:]
-	}
-}
-
-// htmlEscapeMultiline escapes for HTML while preserving soft line breaks as
-// <br> tags so paragraph internals keep their shape.
-func htmlEscapeMultiline(s string) string {
-	return strings.ReplaceAll(html.EscapeString(s), "\n", "<br>")
-}
@@ -53,6 +53,34 @@ describe("ShareDialog", () => {
    expect(screen.getByText(/Redacted: abs-path/)).toBeTruthy();
  });

+  it("renders text blocks as markdown in the preview", async () => {
+    previewShareMock.mockResolvedValueOnce({
+      ...SAMPLE_SNAPSHOT,
+      messages: [
+        {
+          role: "assistant" as const,
+          ts: "now",
+          blocks: [
+            {
+              kind: "text" as const,
+              text: "## Summary\n\n**Pushed** successfully.\n\n- tests passed\n- lint passed\n\n[docs](https://example.com)\n\n![tracker](https://attacker.example/pixel)",
+            },
+          ],
+        },
+      ],
+    });
+
+    render(<ShareDialog open={true} onOpenChange={() => {}} taskId="t-1" sessionId="sess-1" />);
+
+    expect(await screen.findByRole("heading", { level: 2, name: "Summary" })).toBeTruthy();
+    expect(screen.getByText("Pushed").tagName).toBe("STRONG");
+    expect(screen.getByRole("list").children).toHaveLength(2);
+    expect(screen.queryByRole("img", { name: "tracker" })).toBeNull();
+    const docsLink = screen.getByRole("link", { name: "docs" });
+    expect(docsLink.getAttribute("target")).toBe("_blank");
+    expect(docsLink.getAttribute("rel")).toBe("noopener noreferrer");
+  });
+
  it("publishes on click and shows the URL with a copy button", async () => {
    previewShareMock.mockResolvedValueOnce(SAMPLE_SNAPSHOT);
    createShareMock.mockResolvedValueOnce({
@@ -2,6 +2,8 @@

 import { Badge } from "@kandev/ui/badge";
 import { ScrollArea } from "@kandev/ui/scroll-area";
+import ReactMarkdown, { type Components } from "react-markdown";
+import remarkGfm from "remark-gfm";

 import type { SnapshotPreview, SnapshotPreviewMessage } from "@/lib/api/domains/share-api";

@@ -14,6 +16,21 @@ type Props = {
 // the complete snapshot.
 const PREVIEW_HEAD = 8;
 const PREVIEW_TAIL = 8;
+const previewRemarkPlugins = [remarkGfm];
+
+const previewMarkdownComponents: Components = {
+  a: ({ children, href }) => (
+    <a href={href} target="_blank" rel="noopener noreferrer">
+      {children}
+    </a>
+  ),
+  img: () => null,
+  table: ({ children }) => (
+    <div className="overflow-x-auto">
+      <table>{children}</table>
+    </div>
+  ),
+};

 /**
 * Renders the redacted snapshot as a read-only preview. Intentionally not
@@ -98,7 +115,13 @@ function PreviewMessage({ message }: { message: SnapshotPreviewMessage }) {

 function PreviewBlock({ block }: { block: SnapshotPreviewMessage["blocks"][number] }) {
  if (block.kind === "text") {
-    return <p className="whitespace-pre-wrap break-words text-sm">{block.text}</p>;
+    return (
+      <div className="markdown-body min-w-0 text-sm">
+        <ReactMarkdown remarkPlugins={previewRemarkPlugins} components={previewMarkdownComponents}>
+          {block.text ?? ""}
+        </ReactMarkdown>
+      </div>
+    );
  }
  if (block.kind === "tool_call") {
    return (
@@ -0,0 +1,14 @@
+import { test } from "../../fixtures/test-base";
+import { verifyShareMarkdownPreview } from "./share-markdown-flow";
+
+test.describe("Mobile share preview", () => {
+  test("renders conversation markdown without overflowing the dialog", async ({
+    testPage,
+    apiClient,
+    seedData,
+  }) => {
+    test.setTimeout(60_000);
+
+    await verifyShareMarkdownPreview(testPage, apiClient, seedData);
+  });
+});
@@ -0,0 +1,37 @@
+import { expect, type Page } from "@playwright/test";
+import type { SeedData } from "../../fixtures/test-base";
+import type { ApiClient } from "../../helpers/api-client";
+import { SessionPage } from "../../pages/session-page";
+
+export async function verifyShareMarkdownPreview(
+  testPage: Page,
+  apiClient: ApiClient,
+  seedData: SeedData,
+): Promise<void> {
+  const task = await apiClient.createTaskWithAgent(
+    seedData.workspaceId,
+    "Share rendered markdown",
+    seedData.agentProfileId,
+    {
+      description: "/e2e:markdown-table",
+      workflow_id: seedData.workflowId,
+      workflow_step_id: seedData.startStepId,
+      repository_ids: [seedData.repositoryId],
+    },
+  );
+
+  await testPage.goto(`/t/${task.id}`);
+  const session = new SessionPage(testPage);
+  await session.waitForLoad();
+  await session.waitForChatIdle({ timeout: 30_000 });
+
+  await testPage.getByTestId("share-task-button").click();
+  const dialog = testPage.getByRole("dialog", { name: "Share this task" });
+  await expect(dialog.getByRole("heading", { level: 2, name: "Summary" })).toBeVisible();
+  await expect(dialog.locator("table")).toBeVisible();
+
+  expect(await dialog.evaluate((element) => element.scrollWidth <= element.clientWidth + 1)).toBe(
+    true,
+  );
+  await expect(dialog.getByRole("button", { name: "Publish to GitHub Gist" })).toBeVisible();
+}
@@ -0,0 +1,13 @@
+import { test } from "../../fixtures/test-base";
+import { verifyShareMarkdownPreview } from "./share-markdown-flow";
+
+test.describe("Share preview", () => {
+  test("renders conversation markdown without overflowing the dialog", async ({
+    testPage,
+    apiClient,
+    seedData,
+  }) => {
+    test.setTimeout(60_000);
+    await verifyShareMarkdownPreview(testPage, apiClient, seedData);
+  });
+});