Enable GitHub Flavored Markdown (GFM) rendering for dashboard #807
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables GitHub Flavored Markdown (GFM) rendering for dashboard components by creating a centralized Markdown component with URL sanitization and security features.
- Replaces direct react-markdown usage with a custom Markdown wrapper component
- Adds remark-gfm plugin support for enhanced markdown features like tables and strikethrough
- Implements URL sanitization for links and images to prevent security vulnerabilities
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/components/Markdown.tsx | New centralized Markdown component with GFM support and security features |
| src/components/artifacts/MarkdownVisualization.tsx | Updated to use the new custom Markdown component |
| src/app/overview/pipelines-grid/pipeline-sheet.tsx | Updated to use the new custom Markdown component |
| package.json | Added remark-gfm dependency for GFM rendering support |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdded remark-gfm to dependencies. Introduced a new Markdown component that wraps react-markdown with GFM and URL sanitization for links and images. Updated two consumers to use the new local Markdown component instead of react-markdown. No other logic or public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Caller (PipelineSheet / MarkdownVisualization)
participant MD as Markdown (local wrapper)
participant RM as ReactMarkdown
participant GFM as remark-gfm
participant SAN as URL Sanitizer
UI->>MD: render(children, components?, className?)
Note over MD: Merge default link/image overrides with user components
MD->>RM: <ReactMarkdown remarkPlugins=[GFM] ...>
RM->>GFM: Parse Markdown (GFM extensions)
par Link rendering
RM->>SAN: sanitizeUrl(href)
alt safe or empty
SAN-->>RM: sanitized href
RM-->>MD: <a href target="_blank" rel="noopener noreferrer">
else unsafe
SAN-->>RM: unsafe/null
RM-->>MD: <a> (no href)
end
and Image rendering
RM->>SAN: sanitizeUrl(src)
alt safe
SAN-->>RM: sanitized src
RM-->>MD: <img src ...>
else unsafe
SAN-->>RM: null
RM-->>MD: skip image
end
end
MD-->>UI: Rendered HTML
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
strickvl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Markdown.tsx (2)
17-27: Avoid rendering<a>without href; fallback to non-interactive element.When a URL is unsafe/absent, render a span to prevent confusing keyboard/focus semantics.
- return ( - <a {...props} href={safeHref} target={anchorTarget} rel={anchorRel}> - {children} - </a> - ); + if (!safeHref) { + return <span {...props}>{children}</span>; + } + return ( + <a {...props} href={safeHref} target={anchorTarget} rel={anchorRel}> + {children} + </a> + );
28-32: Add image loading/perf safeguards.Lazy-load images and decode asynchronously; also drop referrer info.
- return <img {...props} src={sanitized} alt={alt ?? ""} />; + return ( + <img + {...props} + src={sanitized} + alt={alt ?? ""} + loading="lazy" + decoding="async" + referrerPolicy="no-referrer" + /> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)src/app/overview/pipelines-grid/pipeline-sheet.tsx(1 hunks)src/components/Markdown.tsx(1 hunks)src/components/artifacts/MarkdownVisualization.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Markdown.tsx (1)
src/lib/url.ts (1)
sanitizeUrl(64-66)
🔇 Additional comments (3)
src/app/overview/pipelines-grid/pipeline-sheet.tsx (1)
23-23: Swapping to local Markdown wrapper is correct.This centralizes GFM + sanitization for the sheet summary. No further changes needed here.
src/components/artifacts/MarkdownVisualization.tsx (1)
2-2: Good switch to the wrapped Markdown.Keeps visualization consistent with GFM and sanitization.
package.json (1)
53-53: Add remark-gfm — lockfile updated; no stray react-markdown imports.pnpm-lock.yaml contains [email protected] and react-markdown is only imported in src/components/Markdown.tsx (the wrapper) which already uses remarkGfm; refresh CI dependency caches.
| } | ||
| }; | ||
|
|
||
| const mergedComponents: Components = { ...defaultComponents, ...components }; |
There was a problem hiding this comment.
Do not allow overriding sanitized a and img components.
Merging order currently lets callers replace the secure defaults, weakening guarantees. Lock these two to preserve sanitization.
Apply:
- const mergedComponents: Components = { ...defaultComponents, ...components };
+ const mergedComponents: Components = {
+ ...components,
+ a: defaultComponents.a,
+ img: defaultComponents.img
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mergedComponents: Components = { ...defaultComponents, ...components }; | |
| const mergedComponents: Components = { | |
| ...defaultComponents, | |
| ...components, | |
| a: defaultComponents.a, | |
| img: defaultComponents.img | |
| }; |
🤖 Prompt for AI Agents
In src/components/Markdown.tsx around line 35, the current merge lets callers
override sanitized anchor and image renderers; ensure the sanitized defaults
cannot be replaced by always enforcing defaultComponents.a and
defaultComponents.img after merging user components. Implement this by merging
user components into defaults as now, then explicitly set mergedComponents.a =
defaultComponents.a and mergedComponents.img = defaultComponents.img (or remove
any incoming a/img keys before merge) so callers cannot override these two keys
while keeping other custom components honored.
There was a problem hiding this comment.
As of right now, I dont see a need for users to pass in their custom components. For now I think its fine to use the predefined set fo components, which means we can adjust the props type, and the merge behavior (as its not required). This should solve this issue.
There was a problem hiding this comment.
@Cahllagerfeld You're absolutely right! Removing the custom components functionality is a much cleaner solution that eliminates the security concern entirely. Here's the updated implementation:
import ReactMarkdown from "react-markdown";
import type { Options } from "react-markdown";
import remarkGfm from "remark-gfm";
import { sanitizeUrl } from "@/lib/url";
type MarkdownProps = Omit<
Options,
"children" | "remarkPlugins" | "rehypePlugins" | "components"
> & {
children?: string | null;
className?: string;
};
export default function Markdown({ children, className, ...rest }: MarkdownProps) {
const components = {
a({ href, node: _node, children, target, rel, ...props }) {
const sanitized = href ? sanitizeUrl(href) : undefined;
const safeHref = sanitized && sanitized.startsWith("unsafe:") ? undefined : sanitized;
const anchorTarget = target ?? "_blank";
const anchorRel = rel ?? "noopener noreferrer";
return (
<a {...props} href={safeHref} target={anchorTarget} rel={anchorRel}>
{children}
</a>
);
},
img({ src, alt, node: _node, ...props }) {
const sanitized = src ? sanitizeUrl(src) : undefined;
if (!sanitized || sanitized.startsWith("unsafe:")) return null;
return <img {...props} src={sanitized} alt={alt ?? ""} />;
}
};
return (
<ReactMarkdown
{...rest}
className={className}
components={components}
remarkPlugins={[remarkGfm]}
>
{children ?? ""}
</ReactMarkdown>
);
}This approach:
- Removes the
componentsprop from the type and function signature - Eliminates the merge logic that created the security vulnerability
- Keeps the sanitized
aandimgcomponents locked in place - Simplifies the overall implementation
Perfect solution that addresses both security and simplicity! 🎯
- Add MarkdownViewToggle component for switching between preview and raw modes - Create VisualizationUIStateContext to manage per-visualization UI preferences - Move useArtifactVisualization hook before early returns to fix React hooks rule - Fix regex escape in password validation schema
| .regex(/[a-z]/, "Password must contain at least one lowercase letter") | ||
| .regex(/[0-9]/, "Password must contain at least one number") | ||
| .regex(/[!@#$%^&*(),.?":{}|<>\/]/, "Password must contain at least one special character"); | ||
| .regex(/[!@#$%^&*(),.?":{}|<>/]/, "Password must contain at least one special character"); |
There was a problem hiding this comment.
This change the linter suggested. Feels like I shouldn't touch it, but idk...
There was a problem hiding this comment.
yep, forward slash doesn't need escaping
- Add .claude and design/ to .gitignore to prevent tracking of these files in version control.
| .regex(/[a-z]/, "Password must contain at least one lowercase letter") | ||
| .regex(/[0-9]/, "Password must contain at least one number") | ||
| .regex(/[!@#$%^&*(),.?":{}|<>\/]/, "Password must contain at least one special character"); | ||
| .regex(/[!@#$%^&*(),.?":{}|<>/]/, "Password must contain at least one special character"); |
There was a problem hiding this comment.
yep, forward slash doesn't need escaping
3 participants
This PR enhances markdown rendering in the ZenML dashboard by adding support for GitHub Flavored Markdown (GFM) features, implementing security improvements, and adding a raw markdown view toggle.
Changes
✨ New Features
GFM Support: Added
remark-gfmplugin to enable GitHub Flavored Markdown features:~~text~~)- [ ]/- [x])Markdown View Toggle: Added ability to switch between preview and raw modes for markdown artifact visualizations:
🔒 Security Improvements
Markdowncomponent (src/components/Markdown.tsx) that:hrefandsrcattributes) to prevent XSS attackstarget="_blank",rel="noopener noreferrer")🔧 Refactoring
react-markdownimports with the new centralized component in:src/components/artifacts/MarkdownVisualization.tsxsrc/app/overview/pipelines-grid/pipeline-sheet.tsxuseArtifactVisualizationhook before early returns to comply with React hooks rules🐛 Bug Fixes
Technical Details
New Components & Context
MarkdownViewToggle(src/components/artifacts/MarkdownViewToggle.tsx):aria-pressedfor screen reader supportVisualizationUIStateContext(src/context/VisualizationUIStateContext.tsx):useCallbackanduseMemoto prevent unnecessary re-rendersSecure Markdown Component (
src/components/Markdown.tsx):react-markdownwith theremark-gfmplugin<a>and<img>tagssanitizeUrlutilityDependencies Added
remark-gfm@^4.0.0- Official plugin for GFM support in react-markdownTesting