Skip to content

fix: SeText heading markdown rendering differently from web version#9907

Open
divyanshu-patil wants to merge 1 commit into
mattermost:mainfrom
divyanshu-patil:fix/markdown
Open

fix: SeText heading markdown rendering differently from web version#9907
divyanshu-patil wants to merge 1 commit into
mattermost:mainfrom
divyanshu-patil:fix/markdown

Conversation

@divyanshu-patil

Copy link
Copy Markdown
Contributor

Summary

This PR fixes a Markdown rendering inconsistency where setext headings (headings created by underlining a line with --- or ===) were being rendered differently in the mobile app compared to the web app.

Issue

Given input like:

text 1
text 2
text 3
---
text 4

The mobile parser (based on commonmark) follows the strict CommonMark spec, which states that a setext underline converts the entire preceding paragraph into a heading — not just the line directly above the underline. Since text 1, text 2, and text 3 have no blank lines between them, the parser treats them as a single paragraph node, and that whole block becomes the heading. text 4 is left as a separate paragraph.

This does not match the web app's behavior, where only text 3 (the line immediately preceding the ---) becomes the heading, and text 1 / text 2 remain a normal paragraph.

Root cause

In blocks.js, the setext heading block-start handler assigns the entire accumulated paragraph string (container._string_content, which includes all prior lines) directly to the new heading node:

heading._string_content = container._string_content;

Because container._string_content contains every line collected in the paragraph so far ("text 1\ntext 2\ntext 3\n"), all of it gets wrapped in the heading, rather than isolating just the final line.

Fix

  • Split container._string_content into individual lines and drop the trailing empty entry produced by the final line break.
  • Pop off only the last line and use it as the heading's content, so the heading now reflects only the line directly above the setext underline.
  • If any lines remain before the last one, rebuild them into a new paragraph node (with recalculated sourcepos) and insert it into the tree immediately before the heading, preserving them as normal paragraph text instead of discarding them.
  • If the original paragraph only had one line, behavior is unchanged (heading is created directly from it, no extra paragraph node needed).

Follow-up fix during testing

An initial version of the patch attempted to reassign the heading's sourcepos directly:

heading.sourcepos = [...]; // TypeError: Cannot assign to property 'sourcepos' which has only a getter

This threw a runtime error because sourcepos on Node is a getter-only property (backed internally by _sourcepos). The fix was updated to assign to the internal field instead:

heading._sourcepos = [...];

This resolves the crash while keeping the source position metadata accurate for the newly split heading and paragraph nodes.

Ticket Link

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E/Run (or E2E/Run-iOS / E2E/Run-Android for platform-specific runs).

Device Information

This PR was tested on:

Screenshots

Web

image

Before

image

After

image

Release Note

Fixed setext-style Markdown headings (underlined with --- or ===) incorrectly converting all preceding paragraph lines into the heading instead of only the line directly above the underline, to match web app behavior.

@mm-cloud-bot mm-cloud-bot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 2, 2026
@mattermost-build

Copy link
Copy Markdown
Contributor

Hello @divyanshu-patil,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 725f1a65-3c04-41f5-baab-1157c8f4cb7e

📥 Commits

Reviewing files that changed from the base of the PR and between cefb70c and 7a6c710.

📒 Files selected for processing (1)
  • patches/commonmark+0.31.2-0.patch

📝 Walkthrough

Walkthrough

This PR updates the commonmark patch file (patches/commonmark+0.31.2-0.patch) to fix a comment typo and change setext heading resolution logic so that any preceding lines in an accumulated paragraph are preserved as a separate paragraph node before the heading, rather than being discarded.

Changes

Commonmark Patch Update

Layer / File(s) Summary
Comment fix and formatting
patches/commonmark+0.31.2-0.patch
Corrects a comment typo ("definitiosn" → "definitions") and reformats a refmap trimming call to use slice(pos).
Setext heading paragraph preservation
patches/commonmark+0.31.2-0.patch
Splits accumulated paragraph text by line so the last line becomes the setext heading's content, while preceding lines are inserted as a separate paragraph node with adjusted source positions before the heading.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: fixing setext heading rendering to match the web version.
Description check ✅ Passed The description accurately explains the setext heading bug, the fix, and the source position adjustment.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@divyanshu-patil

Copy link
Copy Markdown
Contributor Author

hello @enahum can you take a look at this PR, it overrides the default behaviour of the CommonMark but brings UI consistency across web and app version

Thank you for you time !

@enahum

enahum commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@divyanshu-patil Thanks for the PR, but I think this fits better in https://github.com/mattermost/commonmark.js instead of a patch. @hmhealey thoughts?

@divyanshu-patil

Copy link
Copy Markdown
Contributor Author

@enahum Fair point, but the CommonMark setext spec actually says:

A setext heading consists of one or more lines of text, not interrupted by a blank line

So this behavior is spec-compliant, raising it in commonmark.js won't change anything.

The real issue is that web doesn't match this behavior. Web uses marked (also CommonMark-compliant, just faster), but it handles this differently. E.g.:

hello
hello
---

renders as:

hello
hello

Even GitHub follows the spec-correct behavior here, so web looks like the odd one out.

I think the fix is on the web side, probably something overriding marked's default behavior. Happy to dig in and fix it if that works for you!

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

Labels

Contributor kind/bug Categorizes issue or PR as related to a bug. release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants