Skip to content

Add pre tag support to paste HTML subset tool#56

Closed
simonw wants to merge 2 commits intomainfrom
codex/add-pre-support-to-paste-html-tool
Closed

Add pre tag support to paste HTML subset tool#56
simonw wants to merge 2 commits intomainfrom
codex/add-pre-support-to-paste-html-tool

Conversation

@simonw
Copy link
Owner

@simonw simonw commented Oct 3, 2025

Summary

  • allow the sanitizer to retain <pre> tags and surface them in the supported elements list
  • update the formatter to preserve whitespace inside preformatted blocks
  • document that preformatted blocks are now supported by the tool

Testing

  • no automated tests were run (not available)

https://chatgpt.com/codex/tasks/task_e_68e0182480d883268e0ecc306f4bf163

@claude
Copy link
Contributor

claude bot commented Oct 3, 2025

Claude finished @simonw's task —— View job


PR Review: Add pre tag support to paste HTML subset tool

Review Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Review modified files and understand changes
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage and documentation
  • Provide comprehensive feedback

Summary

This PR successfully adds <pre> tag support to the HTML sanitizer tool with proper whitespace preservation. The implementation is solid and addresses the stated goals well.

✅ Strengths

Code Quality:

  • Clean, readable implementation that follows existing patterns
  • Good separation of concerns with the preStack mechanism
  • Proper whitespace preservation using rawPart vs part in formatHtml() at line 280
  • Consistent code style with the rest of the file

Implementation Details:

  • Correctly adds 'pre' to allowedTags array at line 160
  • Stack-based tracking of <pre> elements in formatHtml() prevents nested indentation issues
  • Uses regex /^<pre\b/i and /^<\/pre\b/i for robust tag detection (lines 269, 256)
  • Documentation updated to mention "preformatted blocks"

🔍 Minor Issues & Suggestions

Performance Considerations:

  • The formatHtml() function processes HTML character-by-character which could be slow for very large content, but this is acceptable for the intended use case of pasting clipboard content

Code Robustness:

  1. Edge case handling: The preStack implementation looks solid, but consider what happens with malformed HTML like <pre><pre></pre> - the current implementation handles this gracefully by popping only one item from the stack

  2. Regex specificity: The regex patterns are good - using \b word boundaries prevents false matches on tags like <prepare>

Documentation:

  • The change from "Code snippets" to "Inline code snippets" for <code> is a nice clarification that distinguishes it from <pre> blocks

🔒 Security Assessment

No security concerns identified:

  • The <pre> tag is safe as it only affects text formatting
  • No new attributes are being preserved (following the existing pattern of only allowing href for links)
  • The sanitization logic remains robust

🧪 Test Coverage

Current state: As noted in the PR description, no automated tests were run (not available).

Recommendation: While not blocking for this PR, consider adding basic test cases for:

  • Whitespace preservation inside <pre> blocks
  • Nested elements handling within <pre>
  • Malformed <pre> tags

✅ Overall Assessment

This is a well-implemented feature that cleanly adds <pre> tag support while maintaining code quality and security. The whitespace preservation logic is particularly well done. The changes are minimal, focused, and align perfectly with the stated goals.

Recommendation: Approve - This PR is ready to merge.


@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 3, 2025

Deploying tools with  Cloudflare Pages  Cloudflare Pages

Latest commit: c33a5cf
Status: ✅  Deploy successful!
Preview URL: https://374edba8.tools-b1q.pages.dev
Branch Preview URL: https://codex-add-pre-support-to-pas.tools-b1q.pages.dev

View logs

@claude
Copy link
Contributor

claude bot commented Oct 3, 2025

Claude finished @simonw's task —— View job


PR Review: Add pre tag support to paste HTML subset tool

Review Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md file found
  • Review modified files and understand changes
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage and documentation
  • Provide comprehensive feedback

Summary

This PR successfully adds <pre> tag support to the HTML sanitization tool with proper whitespace preservation during formatting. The implementation is solid and follows existing patterns in the codebase.

✅ Positive Aspects

Clean Implementation

  • <pre> tag correctly added to allowedTags array (paste-html-subset.html:160)
  • Updated documentation in supported elements list (paste-html-subset.html:119)
  • Consistent with existing code patterns and style

Smart Whitespace Handling

  • Proper preformatted block detection using preStack array (paste-html-subset.html:243)
  • Preserves original content in <pre> blocks: const content = preStack.length > 0 ? rawPart : part; (paste-html-subset.html:280)
  • Prevents trimming of whitespace inside preformatted content

Robust Formatting Logic

  • Stack-based tracking handles nested elements correctly
  • Regex patterns properly match opening/closing pre tags (paste-html-subset.html:256,269)
  • Maintains indentation integrity outside pre blocks

🔍 Code Quality Assessment

Security: ✅ Excellent

  • No XSS vulnerabilities introduced
  • <pre> tag sanitization follows same safe patterns as other elements
  • No attribute copying for <pre> tags (only href for links is preserved)

Performance: ✅ Good

  • Minimal overhead from pre-tag handling
  • Stack operations are O(1)
  • No significant performance impact on existing functionality

Maintainability: ✅ Very Good

  • Code is clear and well-structured
  • Consistent naming conventions (preStack, descriptive regex patterns)
  • Changes are minimal and focused

🧪 Testing Considerations

Missing Test Coverage

  • No automated tests exist for this functionality (as noted in PR description)
  • Consider adding tests for:
    • Basic <pre> tag preservation
    • Whitespace preservation inside pre blocks
    • Nested pre handling edge cases
    • Mixed content with pre and other elements

Manual Testing Suggestions

<pre>function example() {
    console.log("whitespace preserved");
}</pre>

<pre>
Multi-line
  with indentation
    preserved
</pre>

🐛 Potential Issues

Minor Edge Case (paste-html-subset.html:250)

if (!part && preStack.length === 0) continue;

Empty parts are skipped outside pre blocks but processed inside them. This is correct behavior but could benefit from a code comment explaining the logic.

Documentation Removal
The deletion of paste-html-subset.docs.md removes tool documentation. Consider:

  • Was this intentional cleanup?
  • Is documentation maintained elsewhere?
  • Should usage examples be preserved?

📝 Recommendations

  1. Add inline comment explaining preStack whitespace preservation logic
  2. Consider test coverage for future maintainability
  3. Verify documentation strategy - ensure tool description exists elsewhere if needed

🎯 Overall Assessment

This is a high-quality implementation that successfully adds pre-tag support while maintaining code safety and existing functionality. The changes are minimal, focused, and follow good practices.

Approval Recommendation: ✅ Approve - Ready to merge pending documentation verification.


@simonw
Copy link
Owner Author

simonw commented Oct 3, 2025

I tested this on https://374edba8.tools-b1q.pages.dev/paste-html-subset and it didn't work correctly, so I am abandoning it.

@simonw simonw closed this Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant