fix markdown request renderer #14211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eneufeld
force-pushed
the
fix/request-rendering
branch
from
240e143
to
0cb013f
Compare
sdirix
reviewed
Sep 23, 2024
Comment on lines
42
to
56
| // // eslint-disable-next-line no-null/no-null | ||
| // const ref: React.MutableRefObject<HTMLDivElement | null> = useRef(null); | ||
|
|
||
| // useEffect(() => { | ||
| // const host = document.createElement('div'); | ||
| // const html = this.markdownIt.render(response.content); | ||
| // host.innerHTML = DOMPurify.sanitize(html, { | ||
| // ALLOW_UNKNOWN_PROTOCOLS: true // DOMPurify usually strips non http(s) links from hrefs | ||
| // }); | ||
| // while (ref?.current?.firstChild) { | ||
| // ref.current.removeChild(ref.current.firstChild); | ||
| // } | ||
|
|
||
| // ref?.current?.appendChild(host); | ||
| // }, [response.content]); |
Comment on lines
383
to
396
| useEffect(() => { | ||
| const markdownIt = markdownit(); | ||
| const text = node.request.request.displayText ?? node.request.request.text; | ||
| const host = document.createElement('div'); | ||
| const html = markdownIt.render(text); | ||
| host.innerHTML = DOMPurify.sanitize(html, { | ||
| ALLOW_UNKNOWN_PROTOCOLS: true // DOMPurify usually strips non http(s) links from hrefs | ||
| }); | ||
| while (ref?.current?.firstChild) { | ||
| ref.current.removeChild(ref.current.firstChild); | ||
| } | ||
|
|
||
| ref?.current?.appendChild(host); | ||
| }, [node.request]); |
There was a problem hiding this comment.
This is almost the same content as in the response renderer. I would like to suggest a custom hook useMarkDownRendering(markDownText) so we can reuse this logic in both.
The hook could then also be documented on why it exists and why it uses markdownit directly.
| @@ -411,35 +410,35 @@ export class TextChatResponseContentImpl implements TextChatResponseContent { | |||
|
|
|||
| export class MarkdownChatResponseContentImpl implements MarkdownChatResponseContent { | |||
| kind: 'markdownContent' = 'markdownContent'; | |||
| protected _content: MarkdownStringImpl = new MarkdownStringImpl(); | |||
| protected _content: string; | |||
There was a problem hiding this comment.
shouldn't we keep the markdown string content? depending on its attributes we could then hand over different options to markdownit. Thereby we could allow html embeddings if someone wants to have that.
Change the rendering off all markdown renderers to use markdown-it directly. Also no MarkdownString class is used anymore. fixes eclipse-theia#14208
- revert model changes - create custom hook for rendering
eneufeld
force-pushed
the
fix/request-rendering
branch
from
0cb013f
to
0a6f815
Compare
sdirix
reviewed
Sep 26, 2024
packages/ai-chat-ui/src/browser/chat-response-renderer/markdown-part-renderer.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+73
to
+74
| // eslint-disable-next-line no-null/no-null | ||
| const ref = useRef<HTMLDivElement | null>(null); |
There was a problem hiding this comment.
Is there a benefit of using null over undefined?
There was a problem hiding this comment.
the ref paramerter wants null, can't be used with undefined
packages/ai-chat-ui/src/browser/chat-response-renderer/markdown-part-renderer.tsx
Outdated
Show resolved
Hide resolved
| useEffect(() => { | ||
| const markdownIt = markdownit(); | ||
| const host = document.createElement('div'); | ||
| const html = markdownIt.render(markdown); |
There was a problem hiding this comment.
Suggested change
| const html = markdownIt.render(markdown); | |
| // TODO: accept MarkdownString objects and set options accordingly | |
| const html = markdownIt.render(markdown); |
…n-part-renderer.tsx Co-authored-by: Stefan Dirix <[email protected]>
…n-part-renderer.tsx Co-authored-by: Stefan Dirix <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What it does
Currently requests in the ai chat view use a markdown renderer which parses html input and tries to render this. This leads to unexpected behavior when using html in the query.
By deactivating html support there, an alternative is needed as the markdown renderer just renders an empty paragraph if html is now passed to it. So to render the actual text, a span is created and filled with the text content.
fixes #14208
How to test
Use a string like
<img src="../../enter-openai-key.png" alt="Theia IDE Screenshot" style="max-width: 525px">in you query.Follow-ups
Review checklist
Reminder for reviewers