Update MarkdownBlock.tsx #4841
Update MarkdownBlock.tsx #4841
Conversation
dosubot
bot
added
size:XS
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
dosubot
bot
added
size:M
daniel-lxs
left a comment
daniel-lxs
left a comment
There was a problem hiding this comment.
Thank you @xyOz-dev!
I made a small change so that the new regex only changes the actual link and not the text shown, it should have the same effect as your fix but now the punctuation is preserved on the actual text.
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
| await (async () => { | ||
| const linkElement = screen.getByRole("link") | ||
| expect(linkElement).toHaveAttribute("href", "https://example.com") | ||
| expect(linkElement.textContent).toBe("https://example.com.") |
There was a problem hiding this comment.
Is it expected that the trailing period is part of the link text?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm, looks like the period is still visually inside of the link even though the href is correct.
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/.
There was a problem hiding this comment.
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:
Keeping the punctuation without the link highlight.
There was a problem hiding this comment.
Should this test be failing now if it's fixed to behave like the above? Or am I confused?
daniel-lxs
moved this from PR [Needs Review]
to PR [Changes Requested]
in Roo Code Roadmap
hannesrudolph
moved this from PR [Changes Requested]
to PR [Needs Prelim Review]
in Roo Code Roadmap
|
@daniel-lxs apparently its good now! |
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
|
This is how it looks now @mrubens Now it behaves like github does.
|
|
I'm still confused by https://github.com/RooCodeInc/Roo-Code/pull/4841/files#r2157619976 - what am I missing? |
dosubot
bot
added
size:L
|
@mrubens |
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
* Update MarkdownBlock.tsx * fix: correct URL handling in MarkdownBlock and add tests for trailing punctuation * Fix URL punctuation rendering * fix: improve URL handling in MarkdownBlock and update tests for trailing punctuation --------- Co-authored-by: Daniel Riccio <[email protected]>
4 participants
Related GitHub Issue
Closes #4705
Description
Fixed RegEx to filter common link appendages
Test Procedure
See closed issue.
Type of Change
Important
Fixes URL parsing in
MarkdownBlock.tsxby removing trailing punctuation from links.MarkdownBlock.tsx, updated regex inremarkUrlToLinkto remove trailing punctuation from URLs, ensuring correct link formation.This description was created by
for 6798836. You can customize this summary. It will automatically update as commits are pushed.