Skip to content

fix(super-editor): use fontFamily key for markdown code block monospace font#3820

Closed
smxmdrissel wants to merge 1 commit into
superdoc-dev:mainfrom
smxmdrissel:fix/markdown-code-block-monospace-font
Closed

fix(super-editor): use fontFamily key for markdown code block monospace font#3820
smxmdrissel wants to merge 1 commit into
superdoc-dev:mainfrom
smxmdrissel:fix/markdown-code-block-monospace-font

Conversation

@smxmdrissel

Copy link
Copy Markdown

What changed

Fixes a key mismatch that silently dropped the monospace font override on fenced markdown code blocks.

convertCodeBlock() in the markdown → ProseMirror converter set the monospace run-property override under the key rFonts. Every downstream consumer — the w:rFonts OOXML translator (sdNodeOrKeyName: 'fontFamily') and the style-engine's cascade resolution — reads run font overrides under the key fontFamily. Because the keys never matched, the Courier New override was dropped before reaching the style-engine/DomPainter, so fenced markdown code blocks rendered in the default body font instead of monospace.

This also fixes the same key mismatch in the reverse direction: proseMirrorToMdast.ts's isMonospaceRun() checked runProps.rFonts instead of runProps.fontFamily, which would have prevented a docx-imported monospace run from round-tripping back to markdown inline-code spans on export.

Additionally, each code-block run now gets a textStyle mark ({ fontFamily: 'Courier New' }) alongside the direct runProperties.fontFamily attrs. The direct attrs remain as a fallback for callers that inspect the converter's raw JSON output without dispatching through a live Editor; once dispatched, calculateInlineRunPropertiesPlugin's mark-based recalculation recomputes runProperties.fontFamily from the mark anyway (mirroring how inlineCode already works).

A shared MARKDOWN_MONOSPACE_FONT constant now backs both conversion directions so they can't drift apart again.

Known limitation (not addressed here): proseMirrorToMdast.ts has no path that emits an mdast fenced code block today, so a multi-line monospace run round-trips to a paragraph of inlineCode spans joined by line breaks, not to a fenced code block. Genuine fenced-block round-tripping is a separate, larger change.

Test plan

  • Added mdastToProseMirror.test.ts covering fenced code block conversion, asserting both the textStyle mark and the direct runProperties.fontFamily attrs are set (and that the incorrect rFonts key is never used).
  • Updated proseMirrorToMdast.test.ts to exercise the corrected fontFamily key in isMonospaceRun().
  • Updated calculateInlineRunPropertiesPlugin.test.js to cover mark-based recalculation of code-block runs.
  • Verified end-to-end via the CLI binary and the superdoc-docx demo service's /convert endpoint.
  • pnpm test (scoped to the touched test files) — 76/76 passing.

Closes #3819

…ce font

The markdown-to-ProseMirror converter's convertCodeBlock() set the
monospace run-property override under the key `rFonts`, but every
downstream consumer (the w:rFonts OOXML translator, which declares
sdNodeOrKeyName: 'fontFamily', and the style-engine's cascade
resolution) reads run font overrides under the key `fontFamily`.
Because the keys never matched, the Courier New override was silently
dropped before reaching the style-engine/DomPainter, so fenced
markdown code blocks rendered in the default body font instead of
monospace.

Also fixes the same key mismatch in the reverse direction:
proseMirrorToMdast.ts's isMonospaceRun() checked runProps.rFonts
instead of runProps.fontFamily, which would have prevented a
docx-imported monospace run from round-tripping back to markdown
inline-code spans on export. Note: proseMirrorToMdast.ts has no path
that emits an mdast fenced `code` block today, so even with this fix
a multi-line monospace run round-trips to a paragraph of inlineCode
spans joined by line breaks, not to a fenced code block. Genuine
fenced-block round-tripping is a separate, larger change and is not
addressed here.

Updated the corresponding unit test to exercise the corrected key.

Additionally, attach a `textStyle` mark ({ fontFamily: 'Courier New' })
to each code-block run alongside the direct runProperties.fontFamily
attrs. This is NOT a full mirror of the existing inlineCode conversion,
which sets only the mark. code-block JSON can be produced and consumed
without ever going through an editor.dispatch() (e.g. inspecting the
converter's raw JSON output directly), so the direct attrs are kept as
a fallback for that path; inlineCode runs are always dispatched through
a live Editor, so a mark alone suffices there. Once a code-block run is
dispatched, calculateInlineRunPropertiesPlugin's mark-based run-property
recalculation (which treats fontFamily as a mark-derived key) recomputes
runProperties.fontFamily from the mark via decodeRPrFromMarks anyway, so
the pre-dispatch direct attrs set here are provisional, not final --
without the mark, the directly-set fontFamily is simply dropped on the
very first editor.dispatch(tr) (e.g. the one triggered by the CLI's
'open --content-override' flow), since the plugin recomputes inline run
properties from marks and only preserves non-mark-derived existing keys.
Verified end-to-end via the CLI binary and the superdoc-docx demo
service's /convert endpoint.
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

Grey Divider


Remediation recommended

1. Misses w:ascii monospace 🐞 Bug ≡ Correctness
Description
isMonospaceRun() only checks runProperties.fontFamily.ascii/hAnsi, so runs whose fontFamily
object uses OOXML-prefixed keys (e.g. w:ascii, w:hAnsi) won’t be recognized as monospace and
won’t export to inlineCode. This undermines the intended “docx-imported monospace run” round-trip
behavior when the prefixed key shape is present.
Code

packages/super-editor/src/editors/v1/core/helpers/markdown/proseMirrorToMdast.ts[R375-380]

function isMonospaceRun(runProps: Record<string, unknown> | undefined): boolean {
  if (!runProps) return false;
-  const rFonts = runProps.rFonts as Record<string, string> | undefined;
-  if (!rFonts) return false;
-  return rFonts.ascii === 'Courier New' || rFonts.hAnsi === 'Courier New';
+  const fontFamily = runProps.fontFamily as Record<string, string> | undefined;
+  if (!fontFamily) return false;
+  return fontFamily.ascii === MARKDOWN_MONOSPACE_FONT || fontFamily.hAnsi === MARKDOWN_MONOSPACE_FONT;
+}
Relevance

⭐⭐ Medium

Team often accepts fontFamily/rFonts fidelity fixes (PRs #2768,#3225,#2433), but no evidence on
w:ascii-prefixed keys.

PR-#2768
PR-#3225
PR-#2433

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new implementation only checks unprefixed keys, while other core code explicitly supports
prefixed OOXML keys for fontFamily, indicating both shapes can occur and the current check can miss
monospace runs.

packages/super-editor/src/editors/v1/core/helpers/markdown/proseMirrorToMdast.ts[371-380]
packages/super-editor/src/editors/v1/core/super-converter/styles.js[671-700]
packages/super-editor/src/editors/v1/tests/toolbar/updateToolbarState.test.js[535-548]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`isMonospaceRun()` only detects monospace when `runProps.fontFamily` contains unprefixed keys (`ascii`/`hAnsi`). Other parts of the codebase accept OOXML-prefixed keys (`w:ascii`, `w:hAnsi`, etc.), so monospace runs using that representation will not be converted to mdast `inlineCode` during markdown export.

## Issue Context
`getFontFamilyValue()` already normalizes between prefixed and unprefixed keys, and other code paths/tests demonstrate `runProperties.fontFamily` can carry `w:ascii`.

## Fix Focus Areas
- packages/super-editor/src/editors/v1/core/helpers/markdown/proseMirrorToMdast.ts[371-401]
- packages/super-editor/src/editors/v1/core/helpers/markdown/proseMirrorToMdast.test.ts[287-305]

## Implementation notes
- Update `isMonospaceRun()` to accept both key shapes, e.g.:
 - `const ascii = ff.ascii ?? ff['w:ascii']`
 - `const hAnsi = ff.hAnsi ?? ff['w:hAnsi']`
 - (optionally also check `eastAsia`/`cs` equivalents)
- Consider also handling the case where `runProps.fontFamily` is a string (defensive).
- Add/extend a unit test where `runProperties.fontFamily` uses `{ 'w:ascii': 'Courier New', 'w:hAnsi': 'Courier New' }` and assert it exports to `inlineCode`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines 375 to +380
function isMonospaceRun(runProps: Record<string, unknown> | undefined): boolean {
if (!runProps) return false;
const rFonts = runProps.rFonts as Record<string, string> | undefined;
if (!rFonts) return false;
return rFonts.ascii === 'Courier New' || rFonts.hAnsi === 'Courier New';
const fontFamily = runProps.fontFamily as Record<string, string> | undefined;
if (!fontFamily) return false;
return fontFamily.ascii === MARKDOWN_MONOSPACE_FONT || fontFamily.hAnsi === MARKDOWN_MONOSPACE_FONT;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Misses w:ascii monospace 🐞 Bug ≡ Correctness

isMonospaceRun() only checks runProperties.fontFamily.ascii/hAnsi, so runs whose fontFamily
object uses OOXML-prefixed keys (e.g. w:ascii, w:hAnsi) won’t be recognized as monospace and
won’t export to inlineCode. This undermines the intended “docx-imported monospace run” round-trip
behavior when the prefixed key shape is present.
Agent Prompt
## Issue description
`isMonospaceRun()` only detects monospace when `runProps.fontFamily` contains unprefixed keys (`ascii`/`hAnsi`). Other parts of the codebase accept OOXML-prefixed keys (`w:ascii`, `w:hAnsi`, etc.), so monospace runs using that representation will not be converted to mdast `inlineCode` during markdown export.

## Issue Context
`getFontFamilyValue()` already normalizes between prefixed and unprefixed keys, and other code paths/tests demonstrate `runProperties.fontFamily` can carry `w:ascii`.

## Fix Focus Areas
- packages/super-editor/src/editors/v1/core/helpers/markdown/proseMirrorToMdast.ts[371-401]
- packages/super-editor/src/editors/v1/core/helpers/markdown/proseMirrorToMdast.test.ts[287-305]

## Implementation notes
- Update `isMonospaceRun()` to accept both key shapes, e.g.:
  - `const ascii = ff.ascii ?? ff['w:ascii']`
  - `const hAnsi = ff.hAnsi ?? ff['w:hAnsi']`
  - (optionally also check `eastAsia`/`cs` equivalents)
- Consider also handling the case where `runProps.fontFamily` is a string (defensive).
- Add/extend a unit test where `runProperties.fontFamily` uses `{ 'w:ascii': 'Courier New', 'w:hAnsi': 'Courier New' }` and assert it exports to `inlineCode`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@caio-pizzol

Copy link
Copy Markdown
Contributor

Thanks again for digging into this and putting together such a thoughtful PR. Your diagnosis of the rFonts and fontFamily mismatch is correct.

We have decided not to expand our Markdown support right now, so we will not be merging this. We shared more context in the related issue.

This is a product-scope decision, not an issue with your work. We really appreciate the time and care you put into the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fenced markdown code blocks render in the body font instead of monospace

2 participants