fix(ui): render board approval payload prose as markdown (#9817)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The approval queue is how agents surface decisions that need a human, so the approval card is often the only thing an operator reads before approving or rejecting > - Agents author those payloads in markdown, because markdown is what they produce everywhere else in the product > - `ApprovalPayload.tsx` renders the four prose fields of a board approval as bare text nodes, while `CommentThread` on the same page renders through `MarkdownBody` — same authors, two different renderers > - So the operator sees literal `##`, `**bold**`, backticks and `[link](url)` in the payload, and correctly formatted text in the comments directly below it > - This pull request routes those four fields through the existing `MarkdownBody` component > - The benefit is that the highest-stakes text in the product becomes readable, with no new dependency and no schema change ## Linked Issues or Issue Description Refs #4911 — prior art, see the note at the bottom of this description. No open issue covers this, so per (B) here is the bug report: **What happened:** On a board approval, the `summary`, `recommendedAction`, `nextActionOnApproval` and `risks` fields display raw markdown source. Headings appear as literal `##` mid-paragraph, inline code keeps its backticks, links show as `[text](url)`, and both levels of a nested bullet list collapse into one run-on paragraph. **What was expected:** The same rendering the comment thread further down the same page already gives, since both are agent-authored markdown. **Steps to reproduce:** Open any `request_board_approval` whose `summary` contains markdown — headings, a nested list, code spans or links. **Where:** `ui/src/components/ApprovalPayload.tsx`, `BoardApprovalPayloadContent`. ## What Changed - `ui/src/components/ApprovalPayload.tsx`: import `MarkdownBody` and render `summary`, `recommendedAction`, `nextActionOnApproval` and each `risks` entry through it instead of `<p>` / `<span>` text nodes. `MarkdownBody` defaults `softBreaks` to `true`, which is the same behaviour `CommentThread` opts into explicitly, so paragraph handling matches the comments. - `stripLeadingListMarker`: risks already render inside a custom bullet row, so an authored leading `-` / `*` / `•` would nest a second bullet inside the first. One leading marker is stripped per entry. - The risk bullet dot gains `shrink-0` so it keeps its shape next to block-level markdown content. - `title` stays plain text — it is a one-liner and markdown there is noise. - `proposedComment` stays a verbatim `<pre>` block — it is draft text intended to be posted elsewhere, so it must not be reinterpreted. - `ui/src/components/ApprovalPayload.test.tsx`: tests for markdown rendering in all four fields, the leading-list-marker strip, and that `title` and `proposedComment` remain verbatim. ## Verification - `npx vitest run ui/src/components/ApprovalPayload.test.tsx` — 5 passed. - `npx vitest run ui/src/components/ApprovalPayload.test.tsx ui/src/components/CommentThread.test.tsx` — 12 passed, confirming the shared `MarkdownBody` path is not disturbed. - Manual, measured rather than eyeballed: I ran a patched build in a throwaway container beside an unpatched one and pointed both at the same real approval payload, then counted nodes in the rendered DOM. | | unpatched | patched | |---|---|---| | `.paperclip-markdown` nodes | 0 | 3 | | raw backticks in visible text | yes | no | | rendered `h2` | 0 | 5 | | rendered `li` | 0 | 16 | ## Risks Low, and confined to the board approval card. - Rendering scope widens from text to markdown on four fields. A payload that contains markdown-significant punctuation but was authored as prose could render differently than before. This is the intended change, and it matches how the same author's text is already rendered in comments on the same page. - `stripLeadingListMarker` removes one leading list marker per risk entry. A risk that genuinely begins with a literal hyphen followed by a space loses that hyphen. Chosen over the alternative of a visible double bullet on the common case. - No schema change, no migration, no new dependency. `MarkdownBody` is already used elsewhere in the same directory. ## Model Used Claude Opus 4.8 (`claude-opus-4-8`), via Claude Code, with extended thinking and tool use (repository search, file editing, local test execution, and headless-browser DOM measurement of the before/after renders). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes — no docs describe this rendering behaviour, so there was nothing to update - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green — pending first CI run on this PR - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending first review - [x] I will address all Greptile and reviewer comments before requesting merge --- ### On the prior PR @alxhrzg opened #4911 for this same bug first, and reached the same conclusion I did: route the four fields through `MarkdownBody`. Credit for spotting it and for the diagnosis goes there. That PR has been conflicting against base and untouched since May. Rather than let the fix sit, this PR reapplies the idea on current `master` and adds what #4911 was missing: test coverage, the nested-bullet fix for `risks`, and the `shrink-0` on the bullet dot. I could not push to #4911 directly as it is on another contributor's fork. @alxhrzg, if you would rather finish #4911 yourself, I am happy to close this and hand over the tests and the two risk-row fixes for you to take across. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Dirk
GitHub
parent
5521d768b2
commit
8b1483e601
|
|
@ -4,6 +4,7 @@ import { act } from "react";
|
|||
import { createRoot } from "react-dom/client";
|
||||
import { afterEach, beforeEach, describe, expect, it } from "vitest";
|
||||
import { ApprovalPayloadRenderer, approvalLabel } from "./ApprovalPayload";
|
||||
import { ThemeProvider } from "../context/ThemeContext";
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true;
|
||||
|
|
@ -35,17 +36,19 @@ describe("ApprovalPayloadRenderer", () => {
|
|||
|
||||
act(() => {
|
||||
root.render(
|
||||
<ApprovalPayloadRenderer
|
||||
type="request_board_approval"
|
||||
payload={{
|
||||
title: "Reply with an ASCII frog",
|
||||
summary: "Board asked for approval before posting the frog.",
|
||||
recommendedAction: "Approve the frog reply.",
|
||||
nextActionOnApproval: "Post the frog comment on the issue.",
|
||||
risks: ["The frog might be too powerful."],
|
||||
proposedComment: "(o)<",
|
||||
}}
|
||||
/>,
|
||||
<ThemeProvider>
|
||||
<ApprovalPayloadRenderer
|
||||
type="request_board_approval"
|
||||
payload={{
|
||||
title: "Reply with an ASCII frog",
|
||||
summary: "Board asked for approval before posting the frog.",
|
||||
recommendedAction: "Approve the frog reply.",
|
||||
nextActionOnApproval: "Post the frog comment on the issue.",
|
||||
risks: ["The frog might be too powerful."],
|
||||
proposedComment: "(o)<",
|
||||
}}
|
||||
/>
|
||||
</ThemeProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
|
|
@ -62,19 +65,140 @@ describe("ApprovalPayloadRenderer", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("renders markdown in board approval prose fields", () => {
|
||||
const root = createRoot(container);
|
||||
|
||||
act(() => {
|
||||
root.render(
|
||||
<ThemeProvider>
|
||||
<ApprovalPayloadRenderer
|
||||
type="request_board_approval"
|
||||
payload={{
|
||||
title: "Reply with an ASCII frog",
|
||||
summary: "**Bold** and `code` and [a link](https://example.com).",
|
||||
recommendedAction: "Approve the **frog** reply.",
|
||||
nextActionOnApproval: "Post the `frog` comment.",
|
||||
risks: ["The **frog** might be too powerful."],
|
||||
}}
|
||||
/>
|
||||
</ThemeProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
const bodies = container.querySelectorAll(".paperclip-markdown");
|
||||
expect(bodies.length).toBe(4);
|
||||
|
||||
const summary = bodies[0];
|
||||
expect(summary.querySelector("strong")?.textContent).toBe("Bold");
|
||||
expect(summary.querySelector("code")?.textContent).toBe("code");
|
||||
const link = summary.querySelector("a");
|
||||
expect(link?.getAttribute("href")).toBe("https://example.com");
|
||||
expect(link?.textContent).toBe("a link");
|
||||
|
||||
// The raw markdown characters must not survive into the rendered text.
|
||||
expect(container.textContent).not.toContain("**Bold**");
|
||||
expect(container.textContent).not.toContain("[a link](https://example.com)");
|
||||
|
||||
expect(bodies[1].querySelector("strong")?.textContent).toBe("frog");
|
||||
expect(bodies[2].querySelector("code")?.textContent).toBe("frog");
|
||||
expect(bodies[3].querySelector("strong")?.textContent).toBe("frog");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("does not nest a second bullet when a risk is authored as a markdown list item", () => {
|
||||
const root = createRoot(container);
|
||||
|
||||
act(() => {
|
||||
root.render(
|
||||
<ThemeProvider>
|
||||
<ApprovalPayloadRenderer
|
||||
type="request_board_approval"
|
||||
payload={{
|
||||
title: "Reply with an ASCII frog",
|
||||
risks: [
|
||||
"- **Leading dash** risk.",
|
||||
"* Leading star risk.",
|
||||
"• Leading dot risk.",
|
||||
"1. Leading number risk.",
|
||||
"2) Leading paren risk.",
|
||||
],
|
||||
}}
|
||||
/>
|
||||
</ThemeProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
const bodies = container.querySelectorAll(".paperclip-markdown");
|
||||
expect(bodies.length).toBe(5);
|
||||
for (const body of bodies) {
|
||||
expect(body.querySelector("ul")).toBeNull();
|
||||
expect(body.querySelector("ol")).toBeNull();
|
||||
expect(body.querySelector("li")).toBeNull();
|
||||
}
|
||||
|
||||
expect(bodies[0].querySelector("strong")?.textContent).toBe("Leading dash");
|
||||
expect(container.textContent).toContain("Leading star risk.");
|
||||
expect(container.textContent).toContain("Leading dot risk.");
|
||||
expect(container.textContent).toContain("Leading number risk.");
|
||||
expect(container.textContent).toContain("Leading paren risk.");
|
||||
expect(container.textContent).not.toContain("- **Leading dash**");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("renders every risk when two entries collapse to the same text after marker stripping", () => {
|
||||
const root = createRoot(container);
|
||||
const errors: unknown[] = [];
|
||||
const originalError = console.error;
|
||||
console.error = (...args: unknown[]) => {
|
||||
errors.push(args);
|
||||
};
|
||||
|
||||
try {
|
||||
act(() => {
|
||||
root.render(
|
||||
<ThemeProvider>
|
||||
<ApprovalPayloadRenderer
|
||||
type="request_board_approval"
|
||||
payload={{
|
||||
title: "Reply with an ASCII frog",
|
||||
risks: ["- Low probability", "* Low probability"],
|
||||
}}
|
||||
/>
|
||||
</ThemeProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
expect(container.querySelectorAll(".paperclip-markdown").length).toBe(2);
|
||||
expect(errors).toEqual([]);
|
||||
} finally {
|
||||
console.error = originalError;
|
||||
act(() => {
|
||||
root.unmount();
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
it("can hide the repeated title when the card header already shows it", () => {
|
||||
const root = createRoot(container);
|
||||
|
||||
act(() => {
|
||||
root.render(
|
||||
<ApprovalPayloadRenderer
|
||||
type="request_board_approval"
|
||||
hidePrimaryTitle
|
||||
payload={{
|
||||
title: "Reply with an ASCII frog",
|
||||
summary: "Board asked for approval before posting the frog.",
|
||||
}}
|
||||
/>,
|
||||
<ThemeProvider>
|
||||
<ApprovalPayloadRenderer
|
||||
type="request_board_approval"
|
||||
hidePrimaryTitle
|
||||
payload={{
|
||||
title: "Reply with an ASCII frog",
|
||||
summary: "Board asked for approval before posting the frog.",
|
||||
}}
|
||||
/>
|
||||
</ThemeProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { UserPlus, Lightbulb, ShieldAlert, ShieldCheck } from "lucide-react";
|
||||
import { MarkdownBody } from "./MarkdownBody";
|
||||
import { formatCents } from "../lib/utils";
|
||||
|
||||
export const typeLabel: Record<string, string> = {
|
||||
|
|
@ -161,11 +162,19 @@ export function BoardApprovalPayload({
|
|||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Risks render inside a custom bullet row, so a leading markdown list marker
|
||||
* would nest a second bullet inside the first. Strip one leading marker.
|
||||
*/
|
||||
function stripLeadingListMarker(value: string): string {
|
||||
return value.replace(/^(?:[-*•]|\d+[.)])\s+/, "");
|
||||
}
|
||||
|
||||
function BoardApprovalPayloadContent({ payload }: { payload: Record<string, unknown> }) {
|
||||
const risks = Array.isArray(payload.risks)
|
||||
? payload.risks
|
||||
.filter((value): value is string => typeof value === "string")
|
||||
.map((value) => value.trim())
|
||||
.map((value) => stripLeadingListMarker(value.trim()))
|
||||
.filter(Boolean)
|
||||
: [];
|
||||
const title = firstNonEmptyString(payload.title);
|
||||
|
|
@ -185,7 +194,7 @@ function BoardApprovalPayloadContent({ payload }: { payload: Record<string, unkn
|
|||
{summary && (
|
||||
<div className="space-y-1">
|
||||
<p className="text-(length:--text-micro) font-medium uppercase tracking-(--tracking-label) text-muted-foreground">Summary</p>
|
||||
<p className="leading-6 text-foreground/90">{summary}</p>
|
||||
<MarkdownBody className="leading-6 text-foreground/90">{summary}</MarkdownBody>
|
||||
</div>
|
||||
)}
|
||||
{recommendedAction && (
|
||||
|
|
@ -193,23 +202,23 @@ function BoardApprovalPayloadContent({ payload }: { payload: Record<string, unkn
|
|||
<p className="text-(length:--text-micro) font-medium uppercase tracking-(--tracking-label) text-amber-700 dark:text-amber-300">
|
||||
Recommended action
|
||||
</p>
|
||||
<p className="mt-1 leading-6 text-foreground">{recommendedAction}</p>
|
||||
<MarkdownBody className="mt-1 leading-6 text-foreground">{recommendedAction}</MarkdownBody>
|
||||
</div>
|
||||
)}
|
||||
{nextActionOnApproval && (
|
||||
<div className="rounded-lg border border-border/60 bg-background/60 px-3.5 py-3">
|
||||
<p className="text-(length:--text-micro) font-medium uppercase tracking-(--tracking-label) text-muted-foreground">On approval</p>
|
||||
<p className="mt-1 leading-6 text-foreground">{nextActionOnApproval}</p>
|
||||
<MarkdownBody className="mt-1 leading-6 text-foreground">{nextActionOnApproval}</MarkdownBody>
|
||||
</div>
|
||||
)}
|
||||
{risks.length > 0 && (
|
||||
<div className="space-y-1.5">
|
||||
<p className="text-(length:--text-micro) font-medium uppercase tracking-(--tracking-label) text-muted-foreground">Risks</p>
|
||||
<ul className="space-y-1 text-sm text-muted-foreground">
|
||||
{risks.map((risk) => (
|
||||
<li key={risk} className="flex items-start gap-2">
|
||||
<span className="mt-2 h-1.5 w-1.5 rounded-full bg-muted-foreground/60" />
|
||||
<span className="leading-6">{risk}</span>
|
||||
{risks.map((risk, index) => (
|
||||
<li key={`${index}-${risk}`} className="flex items-start gap-2">
|
||||
<span className="mt-2 h-1.5 w-1.5 shrink-0 rounded-full bg-muted-foreground/60" />
|
||||
<MarkdownBody className="leading-6">{risk}</MarkdownBody>
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
|
|
|
|||
Loading…
Reference in New Issue