Fix markdown export output formats#1
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @mikeldev0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the markdown visualizer's export capabilities by ensuring that exported HTML, Word, and PDF documents accurately reflect the rendered markdown, including proper formatting and a complete document structure. It achieves this through strategic refactoring of script loading, introduction of a shared HTML sanitization utility, and leveraging advanced PDF rendering features to provide a more robust and consistent user experience for document exports. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the export functionality for the Markdown Visualizer. It correctly loads external libraries, refactors the sanitization logic for better reuse, and fixes the output formats for HTML, Word, and PDF exports. The change to use jsPDF.html for PDF generation is a great fix that ensures the rendered HTML is exported instead of raw markup. Similarly, wrapping HTML and Word exports in a full HTML document will help preserve formatting. I've found one minor performance issue in the exportToMarkdown function where an expensive function is called unnecessarily, which I've detailed in a comment. Overall, these are excellent improvements to the tool's functionality and security.
| const buildDocumentHtml = () => { | ||
| const markdown = markdownInput.value; | ||
| const sanitizedHtml = sanitizeMarkdownToHtml(markdown); | ||
|
|
||
| return { | ||
| markdown, | ||
| sanitizedHtml, | ||
| fullHtml: `<!DOCTYPE html><html><head><meta charset="UTF-8"><title>Documento Markdown</title></head><body>${sanitizedHtml}</body></html>` | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The buildDocumentHtml function is a good step towards centralizing logic, but it currently calculates all possible outputs (markdown, sanitizedHtml, fullHtml) on every call. This is inefficient for functions that only need a subset of this data, such as exportToMarkdown which only needs the raw markdown, or exportToPDF which doesn't need the fullHtml string.
Consider refactoring this to avoid unnecessary computations. You could split it into smaller, more focused functions. For example:
const getMarkdown = () => markdownInput.value;
const getSanitizedHtml = () => {
const markdown = getMarkdown();
return sanitizeMarkdownToHtml(markdown);
}
const getFullHtml = () => {
const sanitizedHtml = getSanitizedHtml();
return `<!DOCTYPE html><html><head><meta charset="UTF-8"><title>Documento Markdown</title></head><body>${sanitizedHtml}</body></html>`;
}Then, each export function can call only the helpers it needs. For instance, exportToMarkdown would only call getMarkdown().
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| doc.html(tempContainer, { | ||
| callback: () => doc.save('documento.pdf'), | ||
| margin: [20, 20, 20, 20], | ||
| autoPaging: 'text', | ||
| html2canvas: { scale: 0.8 } |
There was a problem hiding this comment.
The new exportToPDF now calls doc.html(..., { html2canvas: { ... } }), which requires window.html2canvas to be loaded, but the page only loads marked, DOMPurify, and jsPDF (lines 743‑745) and never includes the html2canvas script. Clicking “Exportar a PDF” will therefore raise html2canvas is not defined and no PDF is produced, breaking the PDF export feature introduced here. Please import html2canvas from a CDN before invoking doc.html.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes markdown export functionality by improving sanitization, script loading order, and export format handling. The changes ensure exported documents maintain proper formatting and include rendered HTML content instead of raw markup.
Key Changes:
- Moved external library script tags (DOMPurify and jsPDF) before the inline script to ensure dependencies are loaded before use
- Introduced
sanitizeMarkdownToHtml()helper function andbuildDocumentHtml()to centralize HTML generation and wrap exports in full HTML documents - Updated PDF export to use
jsPDF.html()method for rendering sanitized HTML instead of outputting raw markdown text
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Función para exportar a Markdown | ||
| const exportToMarkdown = () => { | ||
| const markdown = markdownInput.value; | ||
| const { markdown } = buildDocumentHtml(); |
There was a problem hiding this comment.
The exportToMarkdown function unnecessarily calls buildDocumentHtml() just to extract the markdown value. This function performs markdown-to-HTML conversion and document building that aren't needed for markdown export. Consider directly accessing markdownInput.value instead to avoid unnecessary computation.
| const { markdown } = buildDocumentHtml(); | |
| const markdown = markdownInput.value; |
| doc.html(tempContainer, { | ||
| callback: () => doc.save('documento.pdf'), | ||
| margin: [20, 20, 20, 20], | ||
| autoPaging: 'text', | ||
| html2canvas: { scale: 0.8 } | ||
| }); |
There was a problem hiding this comment.
The PDF export callback doesn't handle potential errors from the doc.html() operation. If the HTML rendering fails or the PDF generation encounters an issue, the user won't receive any feedback. Consider wrapping the callback in a try-catch block or adding error handling to inform users if the export fails.
| doc.html(tempContainer, { | |
| callback: () => doc.save('documento.pdf'), | |
| margin: [20, 20, 20, 20], | |
| autoPaging: 'text', | |
| html2canvas: { scale: 0.8 } | |
| }); | |
| try { | |
| doc.html(tempContainer, { | |
| callback: () => { | |
| try { | |
| doc.save('documento.pdf'); | |
| } catch (error) { | |
| console.error('Error al guardar el PDF:', error); | |
| alert('Se produjo un error al exportar el PDF. Inténtalo de nuevo.'); | |
| } | |
| }, | |
| margin: [20, 20, 20, 20], | |
| autoPaging: 'text', | |
| html2canvas: { scale: 0.8 } | |
| }); | |
| } catch (error) { | |
| console.error('Error al generar el PDF:', error); | |
| alert('Se produjo un error al generar el PDF. Inténtalo de nuevo.'); | |
| } |
| tempContainer.innerHTML = sanitizedHtml; | ||
|
|
||
| doc.html(tempContainer, { | ||
| callback: () => doc.save('documento.pdf'), |
There was a problem hiding this comment.
The tempContainer element is created but never removed from memory after the PDF export completes. While it's not added to the DOM, it could contribute to memory leaks in applications with frequent exports. Consider explicitly clearing the tempContainer after the PDF save completes in the callback.
| callback: () => doc.save('documento.pdf'), | |
| callback: () => { | |
| doc.save('documento.pdf'); | |
| // Clear temporary container contents to allow garbage collection | |
| tempContainer.innerHTML = ''; | |
| }, |
Summary
Testing
Codex Task