Skip to content

fix: keep pipe tables visible in commonmark - #729

Merged
eszlamczyk merged 3 commits into
software-mansion:mainfrom
huytdps13400:fix/commonmark-pipe-tables
Aug 31, 2026
Merged

fix: keep pipe tables visible in commonmark#729
eszlamczyk merged 3 commits into
software-mansion:mainfrom
huytdps13400:fix/commonmark-pipe-tables

Conversation

@huytdps13400

Copy link
Copy Markdown
Contributor

What/Why?

flavor="commonmark" used the single-TextView renderer, but the shared MD4C parser still enabled the GFM tables extension. A table-shaped string therefore became a Table AST node that the CommonMark renderer cannot represent, so the entire message rendered as an empty view.

This change makes table parsing an internal flavor policy:

  • CommonMark disables the MD4C tables extension, preserving pipe-table source as selectable literal text.
  • GitHub flavor keeps table parsing and the segmented table renderer unchanged.
  • The shared native parser exposes the low-level tables flag consistently for Android and iOS consumers.
  • Regression coverage checks JS flavor wiring, the shared parser behavior, and an Android Maestro flow against the exact issue input.

Plain prose and shell pipelines containing a single | already parsed as text and remain unchanged. Spoiler parsing (||text||) also remains enabled and unchanged.

Closes #645.

Testing

  • yarn lint
  • yarn typecheck
  • yarn test --runInBand — 19/19 passed
  • yarn prepare — TypeScript, module build, and native Codegen passed
  • iOS native package tests via xcodebuild test — 274/274 passed
  • SwiftLint 0.65.0 — 0 violations
  • Android native testDebugUnitTest, ktlintCheck, compileDebugAndroidTestKotlin, and assembleRelease — passed
  • React Native Android example/Fabric build — passed
  • React Native iOS example/Fabric build — passed
  • Android Maestro commonmark_pipe_table_test.yaml on API 35 — passed; the preview exposed | Model | Speed | |---|---| | Claude | Fast |
  • Repository pre-commit hooks (lint, typecheck, clang-format, ktlint) — passed

PR Checklist

  • Code compiles and runs on iOS
  • Code compiles and runs on Android
  • Updated documentation/README if applicable
  • Ran example app to verify changes
  • E2E tests are passing
  • Required E2E tests have been added (if applicable)

@eszlamczyk eszlamczyk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @huytdps13400 for your pull request!

While this change could work, I believe there is a cleaner way to do it. Since tables are part of the strict GFM specification, gating them behind their own dedicated tables flag threaded through the whole stack is unnecessary - and it leaves the mode half-done, since strikethrough and task lists are still parsed unconditionally, so flavor="commonmark" isn't really CommonMark.

Instead of putting a flavor-derived value inside the Md4cFlags struct, I'd rather thread the information about the flavor being GFM through to the parser and resolve the extension flags there - not just tables, but the whole GFM set (strikethrough, task lists) in one place:

std::shared_ptr<MarkdownASTNode> MD4CParser::parse(
    const std::string &markdown, const Md4cFlags &md4cFlags, bool isGFM) {
  unsigned flags = MD_FLAG_NOHTML | MD_FLAG_SPOILERS; 
  if (isGFM) {
    flags |= MD_FLAG_TABLES | MD_FLAG_STRIKETHROUGH | MD_FLAG_TASKLISTS;
  }
  // ...

On the JS side this stays what you already have, just as its own signal rather than member of md4cFlags, e.g. isGFM: flavor === 'github'. On iOS and Android you might need to therad this a bit, confirm that you need or reuse already existing infrastructure.

When you're done please tag me here and I'll re-review. Please address my other review comments as well.

@huytdps13400

Copy link
Copy Markdown
Contributor Author

@eszlamczyk Addressed the review in 9da8a83. I removed the dedicated tables member from Md4cFlags and now pass a separate isGFM signal from flavor === "github" through codegen, Android/iOS render and measurement paths, and the parser bridges. The core parser enables tables, strikethrough, and task lists together only for GFM. Native parser regressions cover all three extensions in CommonMark mode, and I removed the Maestro flow plus Playground changes. Verification: TypeScript, lint, all 19 JS tests, Android parser native build, Android test APK compilation, Kotlin lint, and a focused C++ parser harness all pass.

@eszlamczyk eszlamczyk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM - the core change is exactly the shape I wanted: a single isGFM signal gating the whole GFM extension set (tables + strikethrough + task lists) in the parser, driven off flavor === 'github' at the RN layer, with no tables member left in Md4cFlags. Threading through the RN iOS and Android paths (including measurement and streaming) is thorough.

One small nitpick, which I'll take care of myself so we don't hold this up: the standalone native packages (enriched-markdown-ios, android-enriched-markdown) don't render GFM yet - there's no table/task-list/strikethrough renderer in them - so exposing an isGFM toggle on their public parse API (and documenting it in the iOS README) advertises something they can't do. I'll drop the isGFM parameter from those two packages and just pin CommonMark at their bridge call, and remove the GFM mention from the iOS README. That keeps GFM entirely a concern of the RN layer until the dedicated iOS GFM rendering work lands. Thanks for the solid rework here!

@eszlamczyk
eszlamczyk merged commit db0ec76 into software-mansion:main Aug 31, 2026
11 checks passed
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.

EnrichedMarkdownText renders nothing when content contains | (spoiler syntax cannot be disabled)

2 participants