Render markdown in chat embed #9
Merged
Conversation
This file contains hidden or 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
Greptile OverviewGreptile SummaryThis PR adds markdown rendering support to assistant messages using Key Implementation DetailsComponent Changes:
Styling:
Critical Issues Found1. Bundle Configuration Problem:
2. Missing Security Configurations:
3. Dependency Management:
Recommendations
The markdown rendering functionality itself works correctly, but the implementation has configuration and security gaps that should be addressed before merging. Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant API as Chat API
participant Hook as useChat Hook
participant MessageList
participant Message as Message Component
participant ReactMarkdown
User->>API: Send message via sendMessage()
API-->>Hook: SSE stream with assistant response
Hook->>Hook: Process SSE events (message type)
Hook->>Hook: Update message content incrementally
Hook->>MessageList: Pass messages array
MessageList->>Message: Render each message
alt User Message
Message->>Message: Render as plain text with white-space: pre-wrap
else Assistant Message
Message->>ReactMarkdown: Pass message.content
ReactMarkdown->>ReactMarkdown: Parse markdown syntax
ReactMarkdown->>ReactMarkdown: Apply custom link component (target="_blank")
ReactMarkdown-->>Message: Return formatted React elements
Message->>Message: Apply .content.markdown CSS styles
end
Message-->>MessageList: Rendered message
MessageList-->>User: Display formatted message
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/components/Message/Message.tsx
Outdated
| {isUser ? ( | ||
| message.content | ||
| ) : ( | ||
| <ReactMarkdown>{message.content}</ReactMarkdown> |
There was a problem hiding this comment.
Links rendered in markdown should include security attributes to prevent security vulnerabilities. When rendering external links, it's a best practice to add target="_blank" and rel="noopener noreferrer" to prevent the new page from accessing the window.opener object.
Consider configuring ReactMarkdown with a custom link component:
Suggested change
| <ReactMarkdown>{message.content}</ReactMarkdown> | |
| <ReactMarkdown | |
| components={{ | |
| a: ({ node, ...props }) => ( | |
| <a {...props} target="_blank" rel="noopener noreferrer" /> | |
| ), | |
| }} | |
| > | |
| {message.content} | |
| </ReactMarkdown> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Message/Message.tsx
Line: 50:50
Comment:
Links rendered in markdown should include security attributes to prevent security vulnerabilities. When rendering external links, it's a best practice to add `target="_blank"` and `rel="noopener noreferrer"` to prevent the new page from accessing the `window.opener` object.
Consider configuring ReactMarkdown with a custom link component:
```suggestion
<ReactMarkdown
components={{
a: ({ node, ...props }) => (
<a {...props} target="_blank" rel="noopener noreferrer" />
),
}}
>
{message.content}
</ReactMarkdown>
```
How can I resolve this? If you propose a fix, please make it concise.
src/components/Message/Message.tsx
Outdated
| {isUser ? ( | ||
| message.content | ||
| ) : ( | ||
| <ReactMarkdown>{message.content}</ReactMarkdown> |
There was a problem hiding this comment.
Consider adding URL protocol restrictions to prevent javascript: and other dangerous protocols in links and images. While react-markdown v10 disallows raw HTML by default, it's still best practice to explicitly sanitize URLs.
You can use the urlTransform prop to validate and sanitize URLs:
Suggested change
| <ReactMarkdown>{message.content}</ReactMarkdown> | |
| <ReactMarkdown | |
| urlTransform={(url) => { | |
| try { | |
| const parsed = new URL(url, window.location.href); | |
| // Only allow http, https, and mailto protocols | |
| if (['http:', 'https:', 'mailto:'].includes(parsed.protocol)) { | |
| return url; | |
| } | |
| return ''; | |
| } catch { | |
| return ''; | |
| } | |
| }} | |
| > | |
| {message.content} | |
| </ReactMarkdown> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Message/Message.tsx
Line: 50:50
Comment:
Consider adding URL protocol restrictions to prevent `javascript:` and other dangerous protocols in links and images. While react-markdown v10 disallows raw HTML by default, it's still best practice to explicitly sanitize URLs.
You can use the `urlTransform` prop to validate and sanitize URLs:
```suggestion
<ReactMarkdown
urlTransform={(url) => {
try {
const parsed = new URL(url, window.location.href);
// Only allow http, https, and mailto protocols
if (['http:', 'https:', 'mailto:'].includes(parsed.protocol)) {
return url;
}
return '';
} catch {
return '';
}
}}
>
{message.content}
</ReactMarkdown>
```
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
|
@greptileeai |
Comment on lines
+94
to
96
| "dependencies": { | ||
| "react-markdown": "^10.1.0" | ||
| } |
There was a problem hiding this comment.
react-markdown should be moved from dependencies to peerDependencies to follow best practices for React library development. This ensures:
- The consuming application controls the exact version installed
- Avoids duplicate React context issues
- Reduces bundle size by not bundling the dependency
- Maintains compatibility with different versions
Suggested change
| "dependencies": { | |
| "react-markdown": "^10.1.0" | |
| } | |
| "peerDependencies": { | |
| "react": "^18.0.0 || ^19.0.0", | |
| "react-dom": "^18.0.0 || ^19.0.0", | |
| "react-markdown": "^10.0.0" | |
| }, |
And add to devDependencies:
Suggested change
| "dependencies": { | |
| "react-markdown": "^10.1.0" | |
| } | |
| "react-markdown": "^10.1.0", |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: package.json
Line: 94:96
Comment:
`react-markdown` should be moved from `dependencies` to `peerDependencies` to follow best practices for React library development. This ensures:
1. The consuming application controls the exact version installed
2. Avoids duplicate React context issues
3. Reduces bundle size by not bundling the dependency
4. Maintains compatibility with different versions
```suggestion
"peerDependencies": {
"react": "^18.0.0 || ^19.0.0",
"react-dom": "^18.0.0 || ^19.0.0",
"react-markdown": "^10.0.0"
},
```
And add to `devDependencies`:
```suggestion
"react-markdown": "^10.1.0",
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
50
to
61
| <ReactMarkdown | ||
| components={{ | ||
| a: ({ href, children }) => ( | ||
| <a href={href} target="_blank" rel="noopener noreferrer"> | ||
| {children} | ||
| </a> | ||
| ), | ||
| }} | ||
| > | ||
| {message.content} | ||
| </ReactMarkdown> | ||
| )} |
There was a problem hiding this comment.
Missing security configuration for react-markdown. By default, react-markdown v10 does NOT sanitize HTML and allows potentially dangerous content. While react-markdown escapes HTML by default in recent versions, explicit security measures should be configured to protect against:
- Dangerous URLs (javascript:, data:, vbscript:)
- Malicious link targets
- Future security considerations
Add security props to ReactMarkdown:
Suggested change
| <ReactMarkdown | |
| components={{ | |
| a: ({ href, children }) => ( | |
| <a href={href} target="_blank" rel="noopener noreferrer"> | |
| {children} | |
| </a> | |
| ), | |
| }} | |
| > | |
| {message.content} | |
| </ReactMarkdown> | |
| )} | |
| <ReactMarkdown | |
| components={{ | |
| a: ({ href, children }) => ( | |
| <a href={href} target="_blank" rel="noopener noreferrer"> | |
| {children} | |
| </a> | |
| ), | |
| }} | |
| urlTransform={(url) => { | |
| // Sanitize URLs to prevent javascript:, data:, vbscript: protocols | |
| if (url.startsWith('javascript:') || url.startsWith('data:') || url.startsWith('vbscript:')) { | |
| return ''; | |
| } | |
| return url; | |
| }} | |
| disallowedElements={['script', 'iframe', 'object', 'embed']} | |
| > |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Message/Message.tsx
Line: 50:61
Comment:
Missing security configuration for `react-markdown`. By default, react-markdown v10 does NOT sanitize HTML and allows potentially dangerous content. While react-markdown escapes HTML by default in recent versions, explicit security measures should be configured to protect against:
1. Dangerous URLs (javascript:, data:, vbscript:)
2. Malicious link targets
3. Future security considerations
Add security props to ReactMarkdown:
```suggestion
<ReactMarkdown
components={{
a: ({ href, children }) => (
<a href={href} target="_blank" rel="noopener noreferrer">
{children}
</a>
),
}}
urlTransform={(url) => {
// Sanitize URLs to prevent javascript:, data:, vbscript: protocols
if (url.startsWith('javascript:') || url.startsWith('data:') || url.startsWith('vbscript:')) {
return '';
}
return url;
}}
disallowedElements={['script', 'iframe', 'object', 'embed']}
>
```
How can I resolve this? If you propose a fix, please make it concise.
src/components/Message/Message.tsx
Outdated
Comment on lines
52
to
56
| a: ({ href, children }) => ( | ||
| <a href={href} target="_blank" rel="noopener noreferrer"> | ||
| {children} | ||
| </a> | ||
| ), |
There was a problem hiding this comment.
The custom a component doesn't validate if href exists before rendering, which could lead to invalid HTML. TypeScript types from react-markdown indicate href can be undefined.
Suggested change
| a: ({ href, children }) => ( | |
| <a href={href} target="_blank" rel="noopener noreferrer"> | |
| {children} | |
| </a> | |
| ), | |
| a: ({ href, children }) => { | |
| if (!href) return <>{children}</>; | |
| return ( | |
| <a href={href} target="_blank" rel="noopener noreferrer"> | |
| {children} | |
| </a> | |
| ); | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Message/Message.tsx
Line: 52:56
Comment:
The custom `a` component doesn't validate if `href` exists before rendering, which could lead to invalid HTML. TypeScript types from react-markdown indicate `href` can be `undefined`.
```suggestion
a: ({ href, children }) => {
if (!href) return <>{children}</>;
return (
<a href={href} target="_blank" rel="noopener noreferrer">
{children}
</a>
);
},
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Since Prompt To Fix With AIThis is a comment left during a code review.
Path: vite.config.ts
Line: 22:22
Comment:
`react-markdown` needs to be added to the external dependencies array to prevent it from being bundled with the library. Currently, `react-markdown` and all its transitive dependencies (mdast-util-*, micromark-*, hast-util-*, unist-util-*, etc.) are being bundled into the library output, which significantly increases the bundle size.
Since `react-markdown` is listed as a production dependency in `package.json`, consuming applications will install it themselves. The library should treat it as a peer dependency or external to avoid bundling it twice.
```suggestion
external: ['react', 'react-dom', 'react/jsx-runtime', 'react-markdown'],
```
How can I resolve this? If you propose a fix, please make it concise. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.