fix(markdown-preview): improve rendering and asset handling#1966
fix(markdown-preview): improve rendering and asset handling#1966bajrangCoder wants to merge 11 commits intoAcode-Foundation:mainfrom
Conversation
Greptile SummaryThis PR significantly upgrades the in-app markdown preview by adding footnotes, task lists, emoji, mermaid diagrams, math (KaTeX), anchor links, syntax highlighting with copy buttons, and proper asset/image handling. The architecture is generally sound — version-gating prevents stale renders, DOMPurify is applied to mermaid SVG output, mermaid lazy-loading includes retry-on-failure logic, and mermaid re-initialization is guarded behind a theme-signature check. However, there is a critical crash in Key issues:
Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant run.js
participant index.js
participant renderer.js
participant DOMPurify
participant Mermaid
participant KaTeX
User->>run.js: Open .md file
run.js->>index.js: openMarkdownPreview(file)
index.js->>index.js: createMarkdownPreview() / bind()
index.js->>index.js: render()
index.js->>renderer.js: renderMarkdown(text, file)
Note over renderer.js: md.parse() + md.renderer.render()<br/>fence rule: mermaid → div.mermaid<br/>image rule → placeholder src + data-markdown-local-src
renderer.js-->>index.js: { html }
index.js->>DOMPurify: sanitize(html, { FORBID_TAGS:["style"], ADD_TAGS:["eq","eqn"] })
DOMPurify-->>index.js: clean HTML
index.js->>index.js: content.innerHTML = clean HTML
index.js->>index.js: resolveRenderedImages()<br/>fileToObjectUrl() → blob: URLs
index.js->>index.js: enhanceCodeBlocks(version)
Note over index.js: highlightCodeBlock() → codeElement.innerHTML<br/>Add copy buttons
index.js->>Mermaid: getMermaid() [lazy import, retry-on-fail]
Mermaid-->>index.js: mermaid module
index.js->>index.js: initializeMermaid() [guarded by theme signature]
index.js->>Mermaid: mermaid.render(id, source)
Mermaid-->>index.js: { svg, bindFunctions }
index.js->>DOMPurify: sanitize(svg, { USE_PROFILES: svg+svgFilters })
DOMPurify-->>index.js: clean SVG
index.js->>index.js: block.innerHTML = clean SVG
index.js->>Mermaid: bindFunctions?.(block)
Note over renderer.js,KaTeX: KaTeX lazy import (broken — see P0 issue)<br/>getKatexAndTexmathModule() Promise not awaited<br/>→ md.use(undefined) throws TypeError at module init
Last reviewed commit: "Remove unnecessary b..." |
This comment was marked as outdated.
This comment was marked as outdated.
|
@greptile |
|
@bajrangCoder Check 9e6e89d , for handling |
This might be a problem in the future. |
Added lazy loading for KaTeX and markdown-it-texmath modules.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
@greptile |
| function createMarkdownIt() { | ||
| const { | ||
| katexModulePromise: katex, | ||
| mdItTexmathModulePromise: markdownItTexmath, | ||
| } = getKatexAndTexmathModule().then((m) => m); | ||
|
|
||
| const md = markdownIt({ | ||
| html: true, | ||
| linkify: true, | ||
| }); | ||
|
|
||
| md.use(MarkdownItGitHubAlerts) | ||
| .use(anchor, { slugify }) | ||
| .use(markdownItTaskLists) | ||
| .use(markdownItFootnote) | ||
| .use(markdownItTexmath, { | ||
| engine: katex, | ||
| delimiters: ["dollars", "beg_end"], | ||
| katexOptions: { | ||
| throwOnError: false, | ||
| strict: "ignore", | ||
| }, | ||
| }) | ||
| .use(markdownItEmoji); |
There was a problem hiding this comment.
Math plugin initialization crashes at module load time
getKatexAndTexmathModule() is an async function, so calling it returns a Promise. Calling .then((m) => m) on that Promise returns yet another Promise. Destructuring { katexModulePromise: katex, mdItTexmathModulePromise: markdownItTexmath } from a Promise object yields undefined for both properties, because Promise instances don't have those keys.
As a result, markdownItTexmath is undefined when md.use(markdownItTexmath, …) is called. markdown-it's .use() will immediately try to invoke the plugin as a function (plugin.apply(plugin, args)), which throws a TypeError: undefined is not a function.
Since createMarkdownIt() is invoked synchronously at module scope (const md = createMarkdownIt() — line 201), this exception propagates at module initialization time, leaving md as undefined and breaking every markdown render call regardless of whether math is present in the document.
There is also a secondary design issue: even if getKatexAndTexmathModule() were properly awaited, it still returns { katexModulePromise, mdItTexmathModulePromise } where both values are Promise<Module>, not the actual resolved modules.
The correct approach is to make createMarkdownIt async, await the module loading, and use the resolved values:
async function getKatexAndTexmathModules() {
const [katex, markdownItTexmath] = await Promise.all([
import("katex").then(({ default: m }) => m),
import("markdown-it-texmath").then(({ default: m }) => m),
]);
return { katex, markdownItTexmath };
}
async function createMarkdownIt() {
const { katex, markdownItTexmath } = await getKatexAndTexmathModules();
const md = markdownIt({ html: true, linkify: true });
md.use(MarkdownItGitHubAlerts)
.use(anchor, { slugify })
.use(markdownItTaskLists)
.use(markdownItFootnote)
.use(markdownItTexmath, {
engine: katex,
delimiters: ["dollars", "beg_end"],
katexOptions: { throwOnError: false, strict: "ignore" },
})
.use(markdownItEmoji);
// … add renderer overrides …
return md;
}
let mdPromise = null;
async function getMd() {
if (!mdPromise) mdPromise = createMarkdownIt().catch((e) => { mdPromise = null; throw e; });
return mdPromise;
}
export async function renderMarkdown(text, file) {
const md = await getMd();
// …
}There was a problem hiding this comment.
Oh no. I'll fix it tomorrow if this still exists.
|
|
||
| if (highlighted && highlighted !== originalCode) { | ||
| codeElement.innerHTML = highlighted; | ||
| } |
There was a problem hiding this comment.
Unsanitized syntax-highlighter output written to innerHTML
highlightCodeBlock(originalCode, language) returns an HTML string that is injected directly via codeElement.innerHTML with no DOMPurify pass. originalCode is obtained through codeElement.textContent, which decodes HTML entities back to raw characters (e.g. <script> → <script>). If highlightCodeBlock's output is ever not fully escaped — for example, because the underlying highlighter passes through tag-like content as-is for an unknown language — this creates a stored XSS path for a maliciously crafted code block.
Consider sanitizing the output before assignment:
| if (highlighted && highlighted !== originalCode) { | |
| codeElement.innerHTML = highlighted; | |
| } | |
| if (highlighted && highlighted !== originalCode) { | |
| codeElement.innerHTML = DOMPurify.sanitize(highlighted); | |
| } |
| async function getKatexAndTexmathModule() { | ||
| if (!katexModulePromise) { | ||
| katexModulePromise = import("katex") | ||
| .then(({ default: katex }) => katex) | ||
| .catch((error) => { | ||
| katexModulePromise = null; | ||
| throw error; | ||
| }); | ||
| } | ||
|
|
||
| if (!mdItTexmathModulePromise) { | ||
| mdItTexmathModulePromise = import("markdown-it-texmath") | ||
| .then(({ default: markdownItTexmath }) => markdownItTexmath) | ||
| .catch((error) => { | ||
| mdItTexmathModulePromise = null; | ||
| throw error; | ||
| }); | ||
| } | ||
|
|
||
| return { katexModulePromise, mdItTexmathModulePromise }; | ||
| } |
There was a problem hiding this comment.
Function returns Promises inside the resolved object, not the actual modules
getKatexAndTexmathModule() resolves to { katexModulePromise, mdItTexmathModulePromise } where both properties are themselves Promise<Module> objects — they are the pending import chains, not the loaded modules. Any consumer that properly awaits this function will still receive unresolved promises, requiring a second layer of awaiting.
Compare this to getMermaid() in index.js, which correctly resolves to the mermaid object itself.
The function should either return the already-awaited values directly, or — as is already the pattern used for mermaid — keep two separate lazy-getters that each resolve to their module. This inconsistency is the root cause of the crash described in createMarkdownIt().
| import "katex/dist/katex.min.css"; | ||
| import "markdown-it-texmath/css/texmath.css"; |
There was a problem hiding this comment.
Eager CSS imports undermine lazy JS loading
katex/dist/katex.min.css and markdown-it-texmath/css/texmath.css are static imports resolved at bundle time, so the bundler (rspack/webpack) will include these stylesheets in the initial chunk regardless of whether the user ever opens a document containing math. The entire point of the lazy import("katex") / import("markdown-it-texmath") in renderer.js is to defer the cost until it is needed.
Given that KaTeX's CSS alone is roughly 30–40 KB minified, consider loading these stylesheets dynamically alongside the JS modules, or accepting the eager CSS cost and documenting that the JS is deferred only for parse time, not download time.
Fixes: #1355