feat: add visualization support with charts and mermaid diagrams#89
feat: add visualization support with charts and mermaid diagrams#89honix wants to merge 2 commits into
Conversation
Add comprehensive visualization capabilities to Sviter: - Install recharts and mermaid dependencies - Create ChartViewer component for CSV data visualization - Supports line, bar, pie, and area charts - Auto-detects chart type from data - Fetches CSV data from wiki assets - Create MermaidDiagram component for diagram rendering - Extend ProseMirror schema with mermaid and chart nodes - Add custom markdown parsing for visualization syntax: - Mermaid: ```mermaid ... ``` code blocks - Charts:  image syntax - Implement React-based NodeViews for rendering in ProseMirror - Integrate NodeViews into both CollaborativeEditor and ProseMirrorEditor Syntax: - Charts:  - Diagrams: ```mermaid ... ```
|
Claude finished @honix's task in 1m 58s —— View job Code Review Complete ✓I've completed a thorough review of the visualization feature PR. This is an excellent addition to Sviter that brings powerful chart and diagram capabilities! However, there are several security issues and bugs that should be addressed before merging. 🔴 Critical Issues (Must Fix)1. Security: Mermaid
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| '#a28dff', | ||
| ]; | ||
|
|
||
| function parseCSV(csv: string): Array<Record<string, any>> { |
There was a problem hiding this comment.
Security Concern: CSV Parsing Vulnerability
The current CSV parser is overly simplistic and vulnerable to CSV injection attacks. It only splits by commas without proper escaping or quoted field handling.
Issues:
- Fields with commas inside quotes aren't handled (e.g.,
"City, State"would be split incorrectly) - No handling of escaped quotes or newlines within fields
- Could lead to data corruption or XSS if CSV data contains malicious content
Recommendation: Use a proper CSV parsing library like papaparse or csv-parse that handles:
- Quoted fields with commas
- Escaped quotes
- Multi-line fields
- Different delimiters
Example fix:
import Papa from 'papaparse';
function parseCSV(csv: string): Array<Record<string, any>> {
const result = Papa.parse(csv, {
header: true,
dynamicTyping: true, // Auto-converts numbers
skipEmptyLines: true,
});
return result.data;
}
| headers.forEach((header, index) => { | ||
| const value = values[index]; | ||
| // Try to parse as number, otherwise keep as string | ||
| row[header] = isNaN(Number(value)) ? value : Number(value); |
There was a problem hiding this comment.
Bug: Incorrect number parsing
Using isNaN(Number(value)) has issues:
Number("")returns0, notNaN- empty strings become 0Number(" ")returns0- whitespace becomes 0Number("123abc")returnsNaNcorrectly
This means empty cells will show as 0 in charts instead of being skipped, which could mislead users.
Better approach:
const trimmed = value.trim();
const num = Number(trimmed);
row[header] = trimmed !== '' && !isNaN(num) ? num : value;
| mermaid.initialize({ | ||
| startOnLoad: false, | ||
| theme: 'default', | ||
| securityLevel: 'loose', |
There was a problem hiding this comment.
Security Risk: securityLevel: 'loose'
Setting securityLevel: 'loose' disables security features and could expose the app to XSS attacks through malicious mermaid diagrams.
From Mermaid docs:
'loose': Allows all HTML in labels'strict': Disallows HTML and JavaScript'antiscript': Sanitizes HTML
Recommendation: Use 'strict' or 'antiscript' instead:
mermaid.initialize({
startOnLoad: false,
theme: 'default',
securityLevel: 'strict', // or 'antiscript'
fontFamily: 'inherit',
});If you need HTML in labels, use 'antiscript' which sanitizes but allows safe HTML.
| export function MermaidDiagram({ chart }: MermaidDiagramProps) { | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [id] = useState(() => `mermaid-${Math.random().toString(36).substr(2, 9)}`); |
There was a problem hiding this comment.
Code Quality: ID generation
Using Math.random().toString(36).substr(2, 9) is deprecated - substr is replaced by substring.
Also, consider using a more reliable ID generator to avoid potential collisions:
const [id] = useState(() => `mermaid-${crypto.randomUUID()}`);Or if you need shorter IDs:
const [id] = useState(() => `mermaid-${Math.random().toString(36).substring(2, 11)}`);
| /** | ||
| * Helper to fetch CSV data from the wiki | ||
| */ | ||
| async function fetchCSVData(src: string, currentPagePath: string | null): Promise<string> { |
There was a problem hiding this comment.
Performance Concern: Missing request cancellation
The fetchCSVData function doesn't support aborting ongoing requests. If a user rapidly navigates between pages or updates chart sources, this could lead to:
- Race conditions (older requests completing after newer ones)
- Memory leaks from orphaned promises
- Unnecessary network traffic
Recommendation: Use AbortController:
async function fetchCSVData(
src: string,
currentPagePath: string | null,
signal?: AbortSignal
): Promise<string> {
try {
const resolvedPath = resolvePath(src, currentPagePath);
const url = `${getApiUrl()}/api/assets/${encodeURIComponent(resolvedPath).replace(/%2F/g, '/')}`;
const response = await fetch(url, { signal });
if (!response.ok) {
throw new Error(`Failed to fetch: ${response.statusText}`);
}
return await response.text();
} catch (error) {
if (error.name === 'AbortError') {
throw new Error('Request cancelled');
}
console.error('Error fetching CSV data:', error);
throw error;
}
}Then in ChartNodeView, track and cancel the request on update/destroy.
| private root: Root; | ||
| private currentPagePath: string | null; | ||
|
|
||
| constructor(node: ProseMirrorNode, _view: EditorView, _getPos: () => number | undefined, currentPagePath: string | null) { |
There was a problem hiding this comment.
Bug: Unused parameters with underscore prefix
Parameters _view and _getPos are prefixed with underscore but the constructor signature doesn't match ProseMirror's NodeView interface correctly. The third parameter should be getPos, not a function that returns number | undefined.
Correct signature:
constructor(
node: ProseMirrorNode,
view: EditorView,
getPos: boolean | (() => number | undefined),
currentPagePath: string | null
) {You're currently not using view or getPos, but if you need them later (e.g., for handling clicks), you should store them without the underscore prefix.
| let processed = markdown; | ||
|
|
||
| // Extract mermaid blocks | ||
| const mermaidRegex = /```mermaid\n([\s\S]*?)```/g; |
There was a problem hiding this comment.
Bug: Regex state persistence issue
Using regex .exec() in a while loop with a global flag can cause issues because the regex maintains state. If the regex is reused elsewhere or the loop breaks unexpectedly, it can lead to incorrect matches.
Better approach: Use matchAll() or reset lastIndex:
// Option 1: Using matchAll (cleaner)
const mermaidRegex = /```mermaid\n([\s\S]*?)```/g;
const matches = Array.from(markdown.matchAll(mermaidRegex));
matches.forEach((match, index) => {
const
special.push({
type: 'mermaid',
data: { content: match[1].trim() },
placeholder,
});
processed = processed.replace(match[0], placeholder);
});
// Option 2: Reset lastIndex after loop
mermaidRegex.lastIndex = 0;Same issue exists for chartRegex on line 246.
| let doc = markdownParser.parse(processed) || schema.node('doc', null, [schema.node('paragraph')]); | ||
|
|
||
| // Replace placeholders with actual nodes | ||
| if (special.length > 0) { |
There was a problem hiding this comment.
Bug: Placeholder replacement logic flaw
The current implementation has issues:
- Partial matches: If placeholder text appears in regular content, it will incorrectly replace it
- Only first match: It breaks after finding one placeholder, but a node could theoretically contain multiple placeholders
- Doesn't preserve other nodes: Nodes without placeholders are added, but what about nodes with placeholders that also have other content?
Recommended approach:
doc.forEach((node) => {
if (node.textContent) {
let foundMatch = false;
for (const item of special) {
// Use exact match, not includes
if (node.textContent.trim() === item.placeholder) {
foundMatch = true;
if (item.type === 'mermaid') {
newContent.push(schema.node('mermaid', item.data));
} else if (item.type === 'chart') {
newContent.push(schema.node('chart', item.data));
}
break;
}
}
if (!foundMatch) {
newContent.push(node);
}
} else {
newContent.push(node);
}
});
|
Claude finished @honix's task in 54s —— View job Review Complete ✅I've reviewed the latest changes to the visualization feature PR. Excellent work addressing all the security concerns! The code quality has significantly improved. ✅ All Critical Issues FixedI'm pleased to report that all critical security issues from my previous review have been addressed: 1. ✅ Mermaid Security Fixed
2. ✅ CSV Parsing Security Fixed
3. ✅ Placeholder Replacement Logic Fixed
4. ✅ Fetch Request Cancellation Added
5. ✅ Number Parsing Improved
6. ✅ Regex State Issues Fixed
7. ✅ Deprecated
|
Security Fixes: - Change Mermaid securityLevel from 'loose' to 'strict' to prevent XSS - Replace naive CSV parser with papaparse library to prevent injection attacks - Fix placeholder replacement to use exact matching instead of includes() - Add AbortController to cancel pending fetch requests and prevent race conditions Additional Improvements: - Fix deprecated substr() call, use substring() instead - Fix regex state issues by using matchAll() instead of exec() in loop - Improve number parsing to handle empty strings correctly - Add proper error handling for aborted requests
honix
force-pushed
the
claude/add-visualizations-yDAks
branch
from
51a6b3c to
e5c8369
Compare
|
Claude finished @honix's task in 38s —— View job PR Status UpdateI see you've triggered me again! Just to confirm: this PR is still approved and ready to merge ✅ My previous review (completed ~2 minutes ago) confirmed that all critical security issues have been addressed in commit Quick Summary of Current StateAll Critical Issues Fixed:
Code Quality: 9/10 If you'd like me to do something specific (re-test something, check a particular aspect, or help with next steps), just let me know what you need! |
|
This is kinda broken in design and implementation. I'll do it again with more control |
2 participants
Summary
This PR adds comprehensive visualization capabilities to Sviter, enabling users to embed interactive charts and diagrams directly in wiki pages.
Features Added
Chart Visualization: Embed charts from CSV data using image syntax
Mermaid Diagrams: Create flowcharts, sequence diagrams, and more
```mermaid ... ```code blocksImplementation Details
rechartsfor charts andmermaidfor diagramsmermaidandchartnode typesChartViewer: Fetches CSV data and renders interactive chartsMermaidDiagram: Renders Mermaid diagrams with error handlingVisualizations-Demo.mdandweekly-stats.chart.csvto wikiTesting
Test the visualizations by viewing
Visualizations-Demo.mdin the wiki.Note: This PR was generated with assistance from Claude Code.
Test plan
Visualizations-Demo.mdpage and verify charts render correctly