mirror of
https://github.com/getpaseo/paseo.git
synced 2026-11-01 06:02:56 +00:00
fix(app): render markdown verbatim instead of substituting characters (#3253)
markdown-it's typographer option rewrites characters after parsing, so a literal (c) came out as ©. Agents label steps and options (a), (b), (c) all the time, and an ordinary list rendering as (a), (b), © stops your eye every time. The same option turns ... into an ellipsis, --- into an em dash, and straight quotes into curly ones. Agent output is text people paste back into a shell or a file, so a curly quote in --name="my repo" looks correct on screen and fails when pasted. The markdown file preview had the same problem, showing characters that are not in the file on disk. Every in-app markdown surface now goes through one factory in utils/markdown-parser.ts, which leaves typographer off: agent chat, plan cards, pull request comment bodies and the file preview. The plan card was rendering through its library's built-in parser because it never passed a markdownit prop, and that default has typographer on. Link behaviour is unchanged. The assistant parser keeps its file:// permission and no other surface gains it, and linkify stays on for chat and the default renderer and off for plan cards, matching what each did before. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
51a435d3cf
commit
3d69b0f458
@@ -0,0 +1,37 @@
|
||||
import { expect, test } from "../support/fixtures";
|
||||
import { waitForPermissionPrompt } from "../support/helpers/permissions";
|
||||
import { openAgentRoute, seedMockAgentWorkspace } from "../support/helpers/mock-agent";
|
||||
|
||||
// The plan card has to pass its own markdown parser. Drop that prop and
|
||||
// react-native-markdown-display silently supplies one with `typographer: true`
|
||||
// (node_modules/react-native-markdown-display/src/index.js:139-141), which
|
||||
// turns (c) into ©, curls straight quotes, and rewrites --- as an em dash.
|
||||
// No unit test can catch that, because the defect lives in the JSX.
|
||||
//
|
||||
// Nothing here is provider-specific: the mock provider drives it, and the
|
||||
// behavior is the same for every agent.
|
||||
test.describe("Plan card markdown", () => {
|
||||
test("renders plan text verbatim", async ({ page }) => {
|
||||
test.setTimeout(180_000);
|
||||
|
||||
const session = await seedMockAgentWorkspace({
|
||||
repoPrefix: "plan-card-markdown-",
|
||||
title: "Plan card markdown e2e",
|
||||
initialPrompt: "Emit synthetic plan approval.",
|
||||
});
|
||||
|
||||
try {
|
||||
await openAgentRoute(page, session);
|
||||
await waitForPermissionPrompt(page, 120_000);
|
||||
|
||||
const planCard = page.getByTestId("permission-plan-card");
|
||||
await expect(planCard).toContainText("(c)");
|
||||
await expect(planCard).toContainText('--name="my repo"');
|
||||
await expect(planCard).toContainText("---buzz");
|
||||
await expect(planCard).not.toContainText("©");
|
||||
await expect(planCard).not.toContainText("—buzz");
|
||||
} finally {
|
||||
await session.cleanup();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -28,6 +28,7 @@ import { MarkdownParagraphView, MarkdownTextSpan } from "@/components/markdown-t
|
||||
import { MarkdownTableCellText } from "@/components/markdown-text-selection";
|
||||
import { getMarkdownListMarker, getMarkdownListSpacing } from "@/utils/markdown-list";
|
||||
import { markdownNodeContainsType } from "@/utils/markdown-ast";
|
||||
import { createMarkdownParser } from "@/utils/markdown-parser";
|
||||
import { createCompactMarkdownStyles, createMarkdownStyles } from "@/styles/markdown-styles";
|
||||
import type { Theme } from "@/styles/theme";
|
||||
import { openExternalUrl } from "@/utils/open-external-url";
|
||||
@@ -65,7 +66,9 @@ function compactMarkdownStyleMapping(theme: Theme): Partial<MarkdownWithStableRe
|
||||
return { style: createCompactMarkdownStyles(theme) };
|
||||
}
|
||||
|
||||
const defaultMarkdownParser = MarkdownIt({ typographer: true, linkify: true });
|
||||
// Serves PR comment bodies and the markdown file preview; agent chat passes its
|
||||
// own parser. The preview has to show the bytes on disk, so no typographer.
|
||||
const defaultMarkdownParser = createMarkdownParser({ linkify: true });
|
||||
const EMPTY_TEXT_STYLE: TextStyle = {};
|
||||
const MARKDOWN_LIST_ITEM_CONTENT_FLEX: ViewStyle = { flex: 1, flexShrink: 1, minWidth: 0 };
|
||||
export interface MarkdownRendererProps {
|
||||
|
||||
@@ -5,6 +5,12 @@ import { StyleSheet, useUnistyles } from "react-native-unistyles";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { createMarkdownStyles } from "@/styles/markdown-styles";
|
||||
import { getMarkdownListMarker } from "@/utils/markdown-list";
|
||||
import { createMarkdownParser } from "@/utils/markdown-parser";
|
||||
|
||||
// Without this prop react-native-markdown-display builds its own parser with
|
||||
// `typographer: true`, which would render a plan's literal `(c)` as ©. Its
|
||||
// default also leaves linkify off, so this one keeps bare URLs as plain text.
|
||||
const planMarkdownParser = createMarkdownParser({ linkify: false });
|
||||
|
||||
type MarkdownRuleStyles = Record<string, TextStyle & ViewStyle & { [key: string]: unknown }>;
|
||||
|
||||
@@ -219,7 +225,7 @@ export function PlanCard({
|
||||
<View testID={testID} style={containerStyle}>
|
||||
<Text style={titleStyle}>{resolvedTitle}</Text>
|
||||
{description ? <Text style={descriptionStyle}>{description}</Text> : null}
|
||||
<Markdown style={markdownStyles} rules={markdownRules}>
|
||||
<Markdown style={markdownStyles} rules={markdownRules} markdownit={planMarkdownParser}>
|
||||
{text}
|
||||
</Markdown>
|
||||
{footer ? <View style={styles.footer}>{footer}</View> : null}
|
||||
|
||||
@@ -16,7 +16,11 @@ function contentHeadingLineHeight(contentSize: number, tier: keyof typeof FONT_S
|
||||
*
|
||||
* Usage:
|
||||
* const markdownStyles = useMemo(() => createMarkdownStyles(theme), [theme]);
|
||||
* <Markdown style={markdownStyles}>{content}</Markdown>
|
||||
* <Markdown style={markdownStyles} markdownit={parser}>{content}</Markdown>
|
||||
*
|
||||
* Always pass `markdownit` from `@/utils/markdown-parser`. Omit it and
|
||||
* react-native-markdown-display builds its own parser with `typographer: true`,
|
||||
* which rewrites a literal `(c)` as ©.
|
||||
*/
|
||||
export function createMarkdownStyles(theme: Theme) {
|
||||
return {
|
||||
|
||||
@@ -0,0 +1,28 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { createAssistantMarkdownParser } from "./assistant-markdown-parser";
|
||||
|
||||
describe("createAssistantMarkdownParser", () => {
|
||||
it("renders agent text verbatim", () => {
|
||||
const parser = createAssistantMarkdownParser();
|
||||
|
||||
// The reported bug, plus the substitutions that share its cause.
|
||||
expect(parser.renderInline("(c) (C) (r) (tm) (p)")).toBe("(c) (C) (r) (tm) (p)");
|
||||
expect(parser.renderInline("wait for it...")).toBe("wait for it...");
|
||||
expect(parser.renderInline("a -- b")).toBe("a -- b");
|
||||
// Smart quotes are off too: a curled quote is not pasteable into a shell.
|
||||
expect(parser.renderInline(`run --name="my repo"`)).toBe("run --name="my repo"");
|
||||
expect(parser.renderInline("it's fine")).toBe("it's fine");
|
||||
});
|
||||
|
||||
it("allows file:// links, unlike every other parser", () => {
|
||||
const parser = createAssistantMarkdownParser();
|
||||
|
||||
expect(parser.render("[open](file:///tmp/a.ts)")).toContain('href="file:///tmp/a.ts"');
|
||||
});
|
||||
|
||||
it("still rejects javascript: links", () => {
|
||||
const parser = createAssistantMarkdownParser();
|
||||
|
||||
expect(parser.render("[x](javascript:alert(1))")).not.toContain("href");
|
||||
});
|
||||
});
|
||||
@@ -1,13 +1,12 @@
|
||||
import MarkdownIt from "markdown-it";
|
||||
import type MarkdownIt from "markdown-it";
|
||||
import { createMarkdownParser } from "@/utils/markdown-parser";
|
||||
|
||||
export function createAssistantMarkdownParser(): MarkdownIt {
|
||||
const parser = new MarkdownIt({
|
||||
html: false,
|
||||
linkify: true,
|
||||
typographer: true,
|
||||
});
|
||||
const parser = createMarkdownParser({ linkify: true });
|
||||
const defaultValidateLink = parser.validateLink.bind(parser);
|
||||
|
||||
// Assistant messages are the only surface allowed to link into the
|
||||
// filesystem. Every other parser keeps markdown-it's stricter default.
|
||||
parser.validateLink = (url: string) =>
|
||||
url.trim().toLowerCase().startsWith("file://") || defaultValidateLink(url);
|
||||
|
||||
|
||||
@@ -0,0 +1,75 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { createMarkdownParser } from "./markdown-parser";
|
||||
|
||||
// Every string markdown-it's typographer would rewrite, with the character it
|
||||
// would rewrite it to. Sourced from markdown-it/lib/rules_core/replacements.js
|
||||
// and smartquotes.js. `--flag` is deliberately absent: the en-dash rules need
|
||||
// whitespace or a word character on both sides, so a CLI flag after a space is
|
||||
// never touched and asserting on it would prove nothing.
|
||||
const REWRITTEN_BY_TYPOGRAPHER = [
|
||||
"(c)",
|
||||
"(C)",
|
||||
"(r)",
|
||||
"(R)",
|
||||
"(tm)",
|
||||
"(TM)",
|
||||
"(p)",
|
||||
"(P)",
|
||||
"+-",
|
||||
"two dots .. here",
|
||||
"wait for it...",
|
||||
"what????",
|
||||
"stop!!!!",
|
||||
"hmm,, ok",
|
||||
"a -- b",
|
||||
"word--word",
|
||||
"a --- b",
|
||||
'run --name="my repo"',
|
||||
"it's fine",
|
||||
];
|
||||
|
||||
describe("createMarkdownParser", () => {
|
||||
it("renders every typographer-rewritten sequence verbatim", () => {
|
||||
const parser = createMarkdownParser({ linkify: true });
|
||||
|
||||
for (const source of REWRITTEN_BY_TYPOGRAPHER) {
|
||||
expect(parser.renderInline(source)).toBe(escapeHtml(source));
|
||||
}
|
||||
});
|
||||
|
||||
it("would fail if typographer were switched back on", () => {
|
||||
// Guards the assertion above: proves the fixture actually exercises the
|
||||
// rules, so a future refactor can't quietly make the test vacuous.
|
||||
const typographer = createMarkdownParser({ linkify: true });
|
||||
typographer.set({ typographer: true });
|
||||
|
||||
for (const source of REWRITTEN_BY_TYPOGRAPHER) {
|
||||
expect(typographer.renderInline(source)).not.toBe(escapeHtml(source));
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects file:// links", () => {
|
||||
const parser = createMarkdownParser({ linkify: true });
|
||||
|
||||
expect(parser.render("[open](file:///tmp/a.ts)")).not.toContain("href");
|
||||
});
|
||||
|
||||
it("rejects javascript: links", () => {
|
||||
const parser = createMarkdownParser({ linkify: true });
|
||||
|
||||
expect(parser.render("[x](javascript:alert(1))")).not.toContain("href");
|
||||
});
|
||||
|
||||
it("linkifies bare URLs only when asked", () => {
|
||||
expect(createMarkdownParser({ linkify: true }).render("see https://paseo.sh now")).toContain(
|
||||
'href="https://paseo.sh"',
|
||||
);
|
||||
expect(
|
||||
createMarkdownParser({ linkify: false }).render("see https://paseo.sh now"),
|
||||
).not.toContain("href");
|
||||
});
|
||||
});
|
||||
|
||||
function escapeHtml(value: string): string {
|
||||
return value.replaceAll("&", "&").replaceAll("<", "<").replaceAll('"', """);
|
||||
}
|
||||
@@ -0,0 +1,19 @@
|
||||
import MarkdownIt from "markdown-it";
|
||||
|
||||
/**
|
||||
* The one place that decides how the app parses markdown.
|
||||
*
|
||||
* `typographer` stays off. It switches on two markdown-it core rules that
|
||||
* rewrite characters the user may copy back into a shell or a file:
|
||||
* `replacements` turns `(c)` into ©, `a -- b` into an en dash and `...` into
|
||||
* an ellipsis, and `smartquotes` curls straight quotes so `--name="x"` stops
|
||||
* being pasteable. Agent output, plans and file previews all have to render
|
||||
* the text they were handed.
|
||||
*
|
||||
* `linkify` is a parameter rather than a shared default because the surfaces
|
||||
* disagree today: chat and the default renderer linkify bare URLs, plan cards
|
||||
* never have. Unifying that is a product decision on its own.
|
||||
*/
|
||||
export function createMarkdownParser({ linkify }: { linkify: boolean }): MarkdownIt {
|
||||
return new MarkdownIt({ html: false, linkify });
|
||||
}
|
||||
@@ -1283,7 +1283,11 @@ export class MockLoadTestAgentSession implements AgentSession {
|
||||
title: "Plan",
|
||||
description: "Review the proposed plan before implementation starts.",
|
||||
input: {
|
||||
plan: "1. Add the README note.\n2. Keep the change scoped.\n3. Verify the diff.",
|
||||
// The (c), the quoted flag and the --- are load-bearing: they trip
|
||||
// three different markdown-it typographer rules, and
|
||||
// plan-card-markdown.spec.ts asserts all three render verbatim. That
|
||||
// is the only guard against the plan card's parser prop going missing.
|
||||
plan: '1. Add the (c) README note.\n2. Run --name="my repo".\n3. Verify ---buzz in the diff.',
|
||||
},
|
||||
actions: [
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user