feat: enhance the built-in Markdown preview - #2
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe extension now provides Markdown-It composition, responsive preview enhancement, Mermaid rendering, native Markdown preview integration, separate preview bundles, expanded tests, and documentation for the new rendering and runtime behavior. ChangesMarkdown Preview
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant VSCodePreview
participant PreviewRuntime
participant MermaidRuntime
participant PreviewDOM
VSCodePreview->>PreviewRuntime: load preview script
PreviewRuntime->>PreviewDOM: build layout, TOC, and code-line presentation
PreviewRuntime->>MermaidRuntime: load Mermaid when diagrams exist
MermaidRuntime->>PreviewDOM: insert themed SVG diagrams
VSCodePreview->>PreviewRuntime: notify content or theme changes
PreviewRuntime->>PreviewDOM: re-enhance or rerender affected content
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/markdown/compose.ts (2)
266-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
columnOpentoken variable to avoid shadowing the module regex.Line 13 declares a module-level
columnOpenregex. Line 266 declares a localcolumnOpentoken with the same name. The shadowing is legal, but a later edit inside this block can reference the wrong binding. Rename the local variable.♻️ Proposed rename
- const columnOpen = state.push( + const columnOpenToken = state.push( 'better_markdown_preview_column_open', 'div', 1, ); - columnOpen.block = true; - columnOpen.map = [column.openLine, column.openLine + 1]; - columnOpen.meta = { width: column.width }; + columnOpenToken.block = true; + columnOpenToken.map = [column.openLine, column.openLine + 1]; + columnOpenToken.meta = { width: column.width };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/markdown/compose.ts` around lines 266 - 273, Rename the local token variable `columnOpen` in the column composition block to a distinct name, and update its subsequent `block`, `map`, and `meta` assignments accordingly; leave the module-level `columnOpen` regex unchanged.
503-522: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse markdown-it's
escapeHtmlinstead of the local copy.Lines 516-522 duplicate
md.utils.escapeHtml, which this file already uses at lines 233, 462, 486, and 497. Two escaping implementations can drift. The local copy exists only becauserenderPrevioushas nomdreference. Pass the escaper in, or import it from markdown-it.Note on the static analysis hints for this range: replacing this with DOMPurify or sanitize-html would be wrong. This escapes code text for display; it does not sanitize HTML.
♻️ Proposed refactor
function renderPrevious( previous: FenceRenderRule | undefined, tokens: Token[], index: number, options: Parameters<FenceRenderRule>[2], env: unknown, renderer: Parameters<FenceRenderRule>[4], + escape: (value: string) => string, ): string { return previous ? previous(tokens, index, options, env, renderer) - : `<pre><code>${escapeHtml(tokens[index].content)}</code></pre>\n`; + : `<pre><code>${escape(tokens[index].content)}</code></pre>\n`; } - -function escapeHtml(value: string): string { - return value - .replaceAll('&', '&') - .replaceAll('<', '<') - .replaceAll('>', '>') - .replaceAll('"', '"'); -}Update both call sites at lines 473 and 499 to pass
md.utils.escapeHtml.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/markdown/compose.ts` around lines 503 - 522, Remove the local escapeHtml helper and update renderPrevious to accept an escaping function parameter. At both renderPrevious call sites, pass md.utils.escapeHtml, and use that parameter for fallback code-content escaping so markdown-it remains the single escaping implementation.Source: Linters/SAST tools
src/preview/mermaid-runtime.ts (1)
11-25: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
mermaid.initializeruns once per diagram, not once per theme.
renderis called for each Mermaid block in a pass.src/preview/runtime.tsreads the theme once per pass at Line 99 and then loops the blocks. Thereforemermaid.initializere-applies identical global configuration for every block in the document.Cache the applied theme and call
mermaid.initializeonly when the theme changes.♻️ Proposed fix to initialize only on theme change
let diagramId = 0; +let appliedTheme: string | undefined; export async function render( element: HTMLElement, source: string, theme: MermaidTheme, ): Promise<void> { - mermaid.initialize({ - startOnLoad: false, - securityLevel: 'strict', - theme: 'base', - themeVariables: { - background: theme.background, - primaryColor: theme.background, - primaryTextColor: theme.foreground, - primaryBorderColor: theme.border, - lineColor: theme.foreground, - secondaryColor: theme.background, - tertiaryColor: theme.background, - fontFamily: 'var(--vscode-font-family)', - }, - }); + const signature = JSON.stringify(theme); + if (signature !== appliedTheme) { + appliedTheme = signature; + mermaid.initialize({ + startOnLoad: false, + securityLevel: 'strict', + theme: 'base', + themeVariables: { + background: theme.background, + primaryColor: theme.background, + primaryTextColor: theme.foreground, + primaryBorderColor: theme.border, + lineColor: theme.foreground, + secondaryColor: theme.background, + tertiaryColor: theme.background, + fontFamily: 'var(--vscode-font-family)', + }, + }); + } const id = `better-markdown-preview-mermaid-${diagramId++}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preview/mermaid-runtime.ts` around lines 11 - 25, Cache the theme used by the Mermaid runtime around the mermaid.initialize call, and only reinitialize when the current theme differs from the cached theme. Preserve the existing initialization options and update the cache after applying a changed theme so repeated render calls for blocks in the same pass do not reapply identical global configuration.src/preview/runtime.test.ts (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest isolation depends on every test reaching its own
disposecall.
src/preview/runtime.tsstores controllers in a module-levelWeakMapkeyed byDocument. happy-dom reuses onedocumentfor the whole file. If any test fails or throws before itsdisposecall, the shared controller survives into the next test.enhancePreviewthen returns the stale controller with an already-resolvedready, and the following tests fail for an unrelated reason.Also,
vi.restoreAllMocks()runs inbeforeEach, so the spies of the last test are never restored. The explicitgeometry.mockRestore()at Line 219 works around that gap.Move teardown into
afterEachand track the active controllers.♻️ Proposed fix for deterministic teardown
-import { beforeEach, describe, expect, test, vi } from 'vitest'; -import { enhancePreview, parseLineSet, type MermaidAdapter } from './runtime'; +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; +import { + enhancePreview as createPreview, + parseLineSet, + type MermaidAdapter, + type PreviewController, + type PreviewOptions, +} from './runtime'; + +const active = new Set<PreviewController>(); + +function enhancePreview( + target: Document, + options?: PreviewOptions, +): PreviewController { + const controller = createPreview(target, options); + active.add(controller); + return controller; +} function setDocument(html: string): void { document.body.innerHTML = `<div class="markdown-body">${html}</div>`; } describe('preview runtime', () => { beforeEach(() => { setDocument(''); - vi.restoreAllMocks(); }); + + afterEach(() => { + for (const controller of active) { + controller.dispose(); + } + active.clear(); + vi.restoreAllMocks(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preview/runtime.test.ts` around lines 11 - 14, Move test cleanup from beforeEach to afterEach in the runtime tests, and track controllers created by each test so teardown disposes every active controller even when a test fails before its own dispose call. Restore mocks in afterEach as well, including geometry-related spies, while keeping document reset/setup in beforeEach and ensuring the tracked controllers are cleared after cleanup.test/presentation.test.mjs (1)
14-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the alert assertion so it proves the fallback order inside one declaration.
Line 15 only proves that the escaped form exists somewhere in the file. Line 16-21 only proves that the hyphen form appears somewhere before an
--vscode-editor-foregroundtoken, because[\s\S]*?crosses rule boundaries. The stylesheet could split the escaped form and the fallback chain into unrelated rules and still pass.The guideline requires the escaped dot form first and the normalized hyphen form as the fallback. Assert both inside a single
color:declaration.As per coding guidelines: "For VS Code 1.125 Markdown alert custom properties, escape the dot in CSS and retain the normalized hyphen form as a compatibility fallback."
♻️ Proposed fix to bind the assertion to one declaration
for (const alert of ['note', 'tip', 'important', 'warning', 'caution']) { - assert.ok(css.includes(`--vscode-markdownAlert-${alert}\\.foreground`)); - assert.match( - css, - new RegExp( - `--vscode-markdownAlert-${alert}-foreground[\\s\\S]*?--vscode-editor-foreground`, - ), - ); + const declaration = new RegExp( + `\\.better-markdown-preview-alert-${alert}\\s*\\{[^}]*?` + + `--vscode-markdownAlert-${alert}\\\\\\.foreground[^}]*?` + + `--vscode-markdownAlert-${alert}-foreground[^}]*?` + + `--vscode-editor-foreground[^}]*?\\}`, + ); + assert.match(css, declaration, `${alert} alert fallback chain is out of order`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/presentation.test.mjs` around lines 14 - 22, Update the alert assertions in the test loop to match one complete color declaration containing the escaped-dot custom property first, followed by the normalized hyphen fallback and --vscode-editor-foreground. Replace the separate broad css.includes and cross-rule RegExp checks with a declaration-scoped assertion that verifies this exact fallback order for every alert.Source: Coding guidelines
src/preview/runtime.ts (2)
180-222: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe animation frame handle is cleared by direct calls, so
disposecannot cancel a pending frame.
updateActiveHeadingsetsscrollFrame = 0at Line 181.enhancecallsupdateActiveHeadingdirectly at Line 138. If a scroll frame is already pending at that moment, the handle is discarded while the callback is still queued.disposeat Line 232 then findsscrollFrame === 0and cannot cancel it. The queued callback runs after disposal and mutates the TOC links.The effect is small because the callback is idempotent. Clear the handle only in the frame callback.
♻️ Proposed fix to keep the frame handle authoritative
const updateActiveHeading = (): void => { - scrollFrame = 0; const trackedLinks = Array.from( @@ const onScroll = (): void => { if (!scrollFrame) { - scrollFrame = requestAnimationFrame(updateActiveHeading); + scrollFrame = requestAnimationFrame(() => { + scrollFrame = 0; + updateActiveHeading(); + }); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preview/runtime.ts` around lines 180 - 222, Keep scrollFrame authoritative until the scheduled animation-frame callback executes: remove its reset from updateActiveHeading and clear it inside the requestAnimationFrame callback before invoking updateActiveHeading. Ensure direct calls to updateActiveHeading do not discard a pending frame, so dispose can still cancel it.
9-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfigure TypeScript-aware unused-variable rules.
The
**/*.tsblock keeps the baseno-unused-varsrule enabled and does not enable@typescript-eslint/no-unused-vars. Becausepnpm run lint:codeuses--max-warnings=0, this signature can block lint. Disable the base rule and enable the TypeScript rule, or setargs: 'none'for declarations. Keep the descriptive parameter names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preview/runtime.ts` around lines 9 - 15, Update the TypeScript ESLint configuration for the **/*.ts block to disable the base no-unused-vars rule and enable `@typescript-eslint/no-unused-vars`, or configure the TypeScript rule with args: 'none'. Preserve the descriptive parameter names in MermaidAdapter.render and ensure pnpm run lint:code reports no warnings.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@media/preview.css`:
- Line 162: Update the border declaration near the currentColor value to use the
lowercase currentcolor keyword, resolving the Stylelint value-keyword-case
violation without changing the border styling.
- Around line 78-83: Update the TOC link rule covering
.better-markdown-preview-toc a:hover, .better-markdown-preview-toc-dialog
a:hover, and .better-markdown-preview-toc-active to remove both !important
declarations and preserve the intended styling through higher selector
specificity instead, allowing user markdown.styles overrides to retain
precedence.
---
Nitpick comments:
In `@src/markdown/compose.ts`:
- Around line 266-273: Rename the local token variable `columnOpen` in the
column composition block to a distinct name, and update its subsequent `block`,
`map`, and `meta` assignments accordingly; leave the module-level `columnOpen`
regex unchanged.
- Around line 503-522: Remove the local escapeHtml helper and update
renderPrevious to accept an escaping function parameter. At both renderPrevious
call sites, pass md.utils.escapeHtml, and use that parameter for fallback
code-content escaping so markdown-it remains the single escaping implementation.
In `@src/preview/mermaid-runtime.ts`:
- Around line 11-25: Cache the theme used by the Mermaid runtime around the
mermaid.initialize call, and only reinitialize when the current theme differs
from the cached theme. Preserve the existing initialization options and update
the cache after applying a changed theme so repeated render calls for blocks in
the same pass do not reapply identical global configuration.
In `@src/preview/runtime.test.ts`:
- Around line 11-14: Move test cleanup from beforeEach to afterEach in the
runtime tests, and track controllers created by each test so teardown disposes
every active controller even when a test fails before its own dispose call.
Restore mocks in afterEach as well, including geometry-related spies, while
keeping document reset/setup in beforeEach and ensuring the tracked controllers
are cleared after cleanup.
In `@src/preview/runtime.ts`:
- Around line 180-222: Keep scrollFrame authoritative until the scheduled
animation-frame callback executes: remove its reset from updateActiveHeading and
clear it inside the requestAnimationFrame callback before invoking
updateActiveHeading. Ensure direct calls to updateActiveHeading do not discard a
pending frame, so dispose can still cancel it.
- Around line 9-15: Update the TypeScript ESLint configuration for the **/*.ts
block to disable the base no-unused-vars rule and enable
`@typescript-eslint/no-unused-vars`, or configure the TypeScript rule with args:
'none'. Preserve the descriptive parameter names in MermaidAdapter.render and
ensure pnpm run lint:code reports no warnings.
In `@test/presentation.test.mjs`:
- Around line 14-22: Update the alert assertions in the test loop to match one
complete color declaration containing the escaped-dot custom property first,
followed by the normalized hyphen fallback and --vscode-editor-foreground.
Replace the separate broad css.includes and cross-rule RegExp checks with a
declaration-scoped assertion that verifies this exact fallback order for every
alert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22896aea-c9db-4d56-a843-1bd76a0eca54
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.markdownlint-cli2.jsonc.vscode/tasks.json.vscodeignoreAGENTS.mdCHANGELOG.mdREADME.mddocs/architecture.mddocs/plans/002-renderer-implementation.mddocs/testing.mdesbuild.jsmedia/preview.cssmise.tomlpackage.jsonsrc/extension.tssrc/markdown/compose.test.tssrc/markdown/compose.tssrc/preview/index.tssrc/preview/mermaid-runtime.tssrc/preview/runtime.test.tssrc/preview/runtime.tssrc/test/extension.test.tssrc/types/vendor.d.tstest/fixtures/kitchen-sink.mdtest/harness.test.mjstest/manifest.test.mjstest/package-content.test.mjstest/presentation.test.mjstsconfig.extension-tests.jsontsconfig.jsonvitest.config.mjs
|
@coderabbitai review |
✅ Action performedReview finished.
|
Visual evidenceCaptured from VS Code 1.125.0 with the extension running from PR head Dark Modern — rich fences, native highlighting, Mermaid, and wide contentLight Modern — alerts, definitions, footnotes, responsive columns, and code presentationDark High Contrast — narrow responsive layout and floating TOC trigger |
1 participant
Why
VS Code’s native Markdown preview already provides exact editor-theme syntax highlighting, source maps, and scroll synchronization, but its Markdown and presentation feature set is deliberately narrow. Better Markdown Preview should retain those native behaviors while adding the richer authoring and reading experience used by Airplan and jimeh.me.
What changed
mermaidfences from a local, conditionally loaded bundle under the native preview CSPVerification
mise run verifyScope note
The browser extension bundle is Node-free and package-validated, but this repository does not yet have a stable automated vscode.dev Extension Host harness; web-host support is therefore structurally verified rather than claimed as an executed browser-host test. VS Code 1.125 also collapses some backslash-escaped punctuation before contributed core rules run, making escaped and authored autolink text indistinguishable at that hook boundary.