Skip to content

fix(markdown-preview): improve rendering and asset handling#1966

Open
bajrangCoder wants to merge 11 commits intoAcode-Foundation:mainfrom
bajrangCoder:fix/markdown-preview-rendering
Open

fix(markdown-preview): improve rendering and asset handling#1966
bajrangCoder wants to merge 11 commits intoAcode-Foundation:mainfrom
bajrangCoder:fix/markdown-preview-rendering

Conversation

@bajrangCoder
Copy link
Member

Fixes: #1355

  • in app markdown viewer
  • footnotes
  • tasklists
  • emojis
  • mermaid
  • anchor links
  • math
  • syntax highlighting
  • assets handling

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 19, 2026

Greptile Summary

This 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 renderer.js that will break all markdown preview functionality before it can be used.

Key issues:

  • P0 — createMarkdownIt() crashes at module load time: The function calls getKatexAndTexmathModule().then(m => m) (which returns a Promise) and immediately tries to destructure { katexModulePromise: katex, mdItTexmathModulePromise: markdownItTexmath } from it. Destructuring a Promise object yields undefined for both keys. md.use(undefined, …) then throws TypeError: undefined is not a function synchronously at module initialization, leaving md = undefined and making every call to renderMarkdown() fail with a null-dereference. The root cause is that createMarkdownIt() is not async and cannot await the dynamic imports.

  • P1 — getKatexAndTexmathModule() returns Promises, not resolved modules: Even if the crash above were fixed with a simple await, the function returns { katexModulePromise: Promise<katex>, mdItTexmathModulePromise: Promise<texmath> } — the values are still unresolved. This is inconsistent with getMermaid() in index.js, which correctly returns the resolved module. Both the function signature and the consumer need to be redesigned to match the mermaid pattern.

  • P1 — Syntax-highlighter HTML output written to innerHTML without sanitization: codeElement.innerHTML = highlighted in enhanceCodeBlocks() sets the raw output of highlightCodeBlock() into the DOM without a DOMPurify pass. Since originalCode comes from textContent (which decodes HTML entities back to raw characters), any highlighter path that doesn't re-escape its output creates a potential XSS vector for maliciously crafted code blocks.

  • P2 — Eager CSS imports conflict with lazy JS loading: katex/dist/katex.min.css and markdown-it-texmath/css/texmath.css are static top-level imports in index.js, so the bundler includes them in the initial chunk on every app start. This negates the bundle-size benefit of lazily importing the KaTeX JS module.

Confidence Score: 1/5

  • Not safe to merge — a synchronous TypeError in renderer.js crashes the markdown preview module on load, making the entire feature non-functional.
  • The P0 crash in createMarkdownIt() means no markdown file can be previewed at all. The feature cannot be tested end-to-end until that bug is fixed. The secondary P1 issues (double-Promise indirection in getKatexAndTexmathModule, unsanitized innerHTML assignment) would need to be resolved as well before this is production-ready.
  • src/pages/markdownPreview/renderer.js requires the most attention — the createMarkdownIt() and getKatexAndTexmathModule() functions need to be redesigned to properly handle async module loading.

Important Files Changed

Filename Overview
src/pages/markdownPreview/renderer.js Contains a critical P0 crash: createMarkdownIt() destructures from an unawaited Promise, producing undefined for both katex and markdownItTexmath, which causes md.use(undefined) to throw a TypeError at module load time — breaking all markdown preview functionality. Also has a design inconsistency in getKatexAndTexmathModule() returning Promises rather than resolved module values.
src/pages/markdownPreview/index.js Well-structured render pipeline with proper version-gating, mermaid lazy-loading with retry-on-failure, DOMPurify sanitization of mermaid SVGs, and mermaid theme-change detection via signature. Minor concern: syntax-highlighter output is set as innerHTML without a DOMPurify pass; eager KaTeX CSS imports conflict with the lazy JS loading strategy.
src/pages/markdownPreview/style.scss Clean stylesheet additions for mermaid, math (eq/eqn/katex-display), and markdown images. No issues found.
src/lib/run.js Minimal change — delegates .md files directly to openMarkdownPreview(). Logic is straightforward and correct.
package.json Adds katex, markdown-it-emoji, markdown-it-texmath, and mermaid as production dependencies. Appropriate additions for the new feature set.

Sequence Diagram

sequenceDiagram
    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
Loading

Last reviewed commit: "Remove unnecessary b..."

@bajrangCoder

This comment was marked as outdated.

@UnschooledGamer
Copy link
Member

@greptile

@UnschooledGamer
Copy link
Member

@bajrangCoder Check 9e6e89d , for handling markdown-it-texmath dep-generated output handling with DOMPurify sanitisations

@UnschooledGamer
Copy link
Member

UnschooledGamer commented Mar 20, 2026

Minor: data-resolved-href is computed twice per link — once in renderer.js during token traversal and again in resolveRenderedImages — producing identical results; the second pass can be removed.

This might be a problem in the future.

@UnschooledGamer
Copy link
Member

@greptile

Comment on lines +138 to +161
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 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();
  // …
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no. I'll fix it tomorrow if this still exists.

Comment on lines +339 to +342

if (highlighted && highlighted !== originalCode) {
codeElement.innerHTML = highlighted;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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. &lt;script&gt;<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:

Suggested change
if (highlighted && highlighted !== originalCode) {
codeElement.innerHTML = highlighted;
}
if (highlighted && highlighted !== originalCode) {
codeElement.innerHTML = DOMPurify.sanitize(highlighted);
}

Comment on lines +16 to +36
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 };
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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().

Comment on lines +1 to +2
import "katex/dist/katex.min.css";
import "markdown-it-texmath/css/texmath.css";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

Improve: Markdown Rendering

2 participants