Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions webview-ui/src/components/common/MarkdownBlock.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,27 @@ const remarkUrlToLink = () => {

const parts = node.value.split(urlRegex)
const children: any[] = []
const cleanedMatches = matches.map((url: string) => url.replace(/[.,;:!?'"]+$/, ""))

parts.forEach((part: string, i: number) => {
if (part) {
children.push({ type: "text", value: part })
}

if (matches[i]) {
children.push({ type: "link", url: matches[i], children: [{ type: "text", value: matches[i] }] })
if (cleanedMatches[i]) {
const originalUrl = matches[i]
const cleanedUrl = cleanedMatches[i]
const removedPunctuation = originalUrl.substring(cleanedUrl.length)

children.push({
type: "link",
url: cleanedUrl,
children: [{ type: "text", value: cleanedUrl }],
})

if (removedPunctuation) {
children.push({ type: "text", value: removedPunctuation })
}
}
})

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import React from "react"
import { render, screen } from "@testing-library/react"
import MarkdownBlock from "../MarkdownBlock"
import { vi } from "vitest"

vi.mock("@src/utils/vscode", () => ({
vscode: {
postMessage: vi.fn(),
},
}))

vi.mock("@src/context/ExtensionStateContext", () => ({
useExtensionState: () => ({
theme: "dark",
}),
}))

describe("MarkdownBlock", () => {
it("should correctly handle URLs with trailing punctuation", async () => {
const markdown = "Check out this link: https://example.com."
render(<MarkdownBlock markdown={markdown} />)

await (async () => {
const linkElement = screen.getByRole("link")
expect(linkElement).toHaveAttribute("href", "https://example.com")
expect(linkElement.textContent).toBe("https://example.com.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that the trailing period is part of the link text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the expected behaviour is to show the punctuation in the chat block but link to it without a trailing ., So it seems to be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the expected behaviour is to show the punctuation in the chat block but link to it without a trailing ., So it seems to be correct.

I didnt personally write the test, my original code just removed the trailing period, Dan updated it slightly.

Copy link
Collaborator

@mrubens mrubens Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looks like the period is still visually inside of the link even though the href is correct.
Screenshot 2025-06-18 at 3 42 29 PM

IMO we should handle it like Github does 👇 (I don't feel strongly about stripping the https:// though)

Check out this link: https://example.com/.

Copy link
Member

@daniel-lxs daniel-lxs Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I made it so that the actual text keeps the period instead of just removing all punctuation, since it might look weird if the model is doing something like https://example1.com, https://example2.com, etc. and the punctuation is stripped.

However what you are suggesting might be even better:
image

Keeping the punctuation without the link highlight.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this test be failing now if it's fixed to behave like the above? Or am I confused?

})
})
})