Skip to content

Add markdown rendering support for slide text#280

Merged
simonw merged 1 commit into
mainfrom
claude/add-markdown-support-aZZ1R
May 7, 2026
Merged

Add markdown rendering support for slide text#280
simonw merged 1 commit into
mainfrom
claude/add-markdown-support-aZZ1R

Conversation

@simonw
Copy link
Copy Markdown
Owner

@simonw simonw commented May 7, 2026

Summary

This PR adds inline markdown rendering capabilities to the slide text display, allowing users to format text with bold, italic, underline, and code styling.

Key Changes

  • Removed white-space: pre-wrap CSS property from the text styling to allow proper text wrapping with the new markdown rendering
  • Added escapeHtml() function to safely escape HTML special characters in user input before processing
  • Added renderInlineMarkdown() function that processes inline markdown syntax:
    • **text** for bold (<strong>)
    • *text* for italic (<em>)
    • _text_ for underline (<u>)
    • `text` for code (<code>)
    • Supports escaping special characters with backslash (\*, \_, etc.)
  • Added renderMarkdown() function that splits text by newlines and applies inline markdown rendering to each line, joining them with <br> tags
  • Updated applyFromState() to use innerHTML with the new markdown renderer instead of textContent, enabling formatted text display

Implementation Details

The markdown rendering uses a token stashing approach with a Unicode sentinel character (U+E000) to safely handle escaped characters and code blocks without interference from subsequent regex replacements. This ensures that content within code blocks and escaped characters are preserved exactly as intended.

https://claude.ai/code/session_01A6bDUnVTwZZ5D7gZPmb1Xo

Adds *italic*, **bold**, _underline_, and `code` to the rendered text,
with backslash-escapes for literal markers. Input is HTML-escaped before
markdown processing so script/HTML in the ?text= URL parameter cannot
execute.
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @simonw's task —— View job


Code Review Complete ✅

I've thoroughly reviewed this PR that adds inline markdown rendering support to the big-words.html application. Here's my comprehensive feedback:

Todo List:

  • Read and analyze the changed file (big-words.html)
  • Review markdown rendering implementation for bugs and edge cases
  • Evaluate security aspects of HTML handling
  • Check performance implications
  • Assess test coverage needs
  • Provide comprehensive feedback

🟢 Strengths

  1. Excellent security approach - Using escapeHtml() first before markdown processing prevents XSS vulnerabilities
  2. Smart token stashing strategy - The Unicode sentinel approach cleanly handles escaped characters and code blocks
  3. Clean integration - Minimal changes to existing code while adding significant functionality
  4. Performance conscious - Efficient regex-based parsing suitable for typical use cases

🟡 Areas for Improvement

1. Regex Edge Cases & Robustness (renderInlineMarkdown function, lines 328-345)

Potential Issues:

  • Nested bold/italic conflicts: ***bold italic*** will only match the outer bold, leaving orphaned asterisks
  • Cross-tag nesting: **bold _and italic**_ creates malformed HTML <strong>bold <u>and italic</strong>_
  • Empty matches: **** or __ could create empty <strong></strong> tags

Suggested improvements:

// More robust patterns that prevent empty matches
s = s.replace(/\*\*([^*\n]+?)\*\*/g, '<strong>$1</strong>');
s = s.replace(/\*([^*\n]+?)\*/g, '<em>$1</em>');
s = s.replace(/_([^_\n]+?)_/g, '<u>$1</u>');

2. Code Block Security (line 337)

The current code block regex /([^\n]+)/g` allows any content inside backticks. Consider sanitizing code content:

s = s.replace(/`([^`\n]+)`/g, (_, code) => 
    stash('<code>' + escapeHtml(code) + '</code>'));

3. Performance Optimization (line 347-348)

The split('\n').map().join('<br>') pattern processes each line separately. For large texts, consider:

  • Limiting maximum text length
  • Adding early returns for plain text (no markdown syntax detected)

🔵 Minor Suggestions

  1. Function documentation: Add JSDoc comments explaining the token stashing approach
  2. Constant extraction: Move regex patterns to constants for better maintainability
  3. Error handling: Consider graceful degradation if markdown parsing fails

🔍 Security Assessment

Strong security posture:

  • HTML escaping before processing prevents XSS
  • Token stashing protects escaped characters
  • No eval() or dangerous DOM manipulation

🧪 Test Coverage Recommendation

Since this is a standalone HTML file, consider adding:

  • Edge case testing for nested/malformed markdown
  • XSS attempt testing (script tags, event handlers)
  • Performance testing with large text inputs
  • Cross-browser compatibility testing

📊 Performance Analysis

Current approach is well-suited for:

  • Short to medium text lengths (typical slide content)
  • Real-time preview updates
  • Single-pass processing

Potential bottlenecks:

  • Multiple regex passes on the same text
  • String concatenation in large texts

🎯 Overall Assessment

This is a well-implemented feature with strong security practices and clean code organization. The markdown rendering is appropriate for the use case and integrates smoothly with the existing application. The main areas for improvement are edge case handling and robustness, but these don't represent critical issues for typical usage.

Recommendation: ✅ Approve with minor suggestions

The implementation demonstrates good understanding of security principles and provides genuine value to users. The suggested improvements would enhance robustness but aren't blockers for merging.


@simonw
Copy link
Copy Markdown
Owner Author

simonw commented May 7, 2026

Bold doesn't work because slide titles are bold already. I'll tolerate that bug for the moment.

@simonw simonw merged commit 88649b8 into main May 7, 2026
2 of 4 checks passed
@simonw simonw deleted the claude/add-markdown-support-aZZ1R branch May 7, 2026 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants