Skip to content

fix(mdxish): don't serialise GFM tables -> JSX because of line breaks - #1577

Merged
eaglethrost merged 4 commits into
nextfrom
dimas/cx-3773-line-breaks-and-bare-block-markers-in-a-table-cell-silently
Aug 5, 2026
Merged

fix(mdxish): don't serialise GFM tables -> JSX because of line breaks#1577
eaglethrost merged 4 commits into
nextfrom
dimas/cx-3773-line-breaks-and-bare-block-markers-in-a-table-cell-silently

Conversation

@eaglethrost

@eaglethrost eaglethrost commented Aug 4, 2026

Copy link
Copy Markdown
Contributor
🎫 Resolve CX-3773

🎯 What does this PR do?

  • Stops line breaks in a table cell from silently promoting the whole GFM table to JSX <Table> on save (customer request)
  • Line breaks are representable inline in a pipe cell, so the serializer now normalizes them instead: break nodes and newlines inside text become <br />

🧪 QA tips

Link this branch into the app locally (npm run build && npm link here, then npm link @readme/markdown in the app repo) and test in the mdxish editor:

  • Start from a plain pipe table, press shift+enter (or paste multi-line text) inside a cell, save → the doc should stay a pipe table with <br /> in the cell, not turn into <Table>:

    | Field       | Required                               |
    | :---------- | :------------------------------------- |
    | postal_code | Required for US<br />Ignored elsewhere |
  • Re-save the doc a couple of times → output is stable, no drift back to <Table>.

  • Sanity-check that real flow content in a cell still converts to <Table> on save: use the editor UI to insert a bulleted list (with text in the items), multiple paragraphs, or a code block inside a cell → the saved doc should contain <Table>, e.g.:

📸 Screenshot or Loom

Before vs After:

Screen.Recording.2026-08-04.at.9.39.03.pm.mov

@eaglethrost

Copy link
Copy Markdown
Contributor Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 4, 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 Plus

Run ID: d0dae30d-bcc5-41c4-9f20-9087e98f4d04

📥 Commits

Reviewing files that changed from the base of the PR and between f87985c and d0d5a08.

📒 Files selected for processing (3)
  • __tests__/lib/mdxish/mdxishMdastToMd.test.ts
  • __tests__/transformers/mdxish-tables-to-jsx.test.ts
  • processor/transform/mdxish/tables/gfm-cell-normalization.ts
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • readmeio/ai (manual)
  • readmeio/gitto (manual)
  • readmeio/markdown (manual)
  • readmeio/readme (manual)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/transformers/mdxish-tables-to-jsx.test.ts

Walkthrough

The change adds reusable GFM table-cell normalization helpers and separates cell classification from table conversion. GFM-safe cells convert breaks and embedded newlines to <br /> and preserve GFM table output. Unsupported cells flatten breaks and promote tables to JSX. Tests cover breaks, paragraphs, inline-code newlines, list markers, authored <br />, AST normalization, and serialization stability.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

__tests__/lib/mdxish/mdxishMdastToMd.test.ts

Oops! Something went wrong! :(

ESLint: 8.57.1

Error: Error while loading rule '@vitest/unbound-method': You have used a rule which requires type information, but don't have parserOptions set to generate type information for this file. See https://tseslint.com/typed-linting for enabling linting with type information.
Parser: /node_modules/@typescript-eslint/parser/dist/index.js
Occurred while linting /tests/lib/mdxish/mdxishMdastToMd.test.ts
at throwError (/node_modules/@typescript-eslint/utils/dist/eslint-utils/getParserServices.js:40:11)
at getParserServices (/node_modules/@typescript-eslint/utils/dist/eslint-utils/getParserServices.js:29:9)
at create (/node_modules/@typescript-eslint/eslint-plugin/dist/rules/unbound-method.js:138:55)
at Object.create (/node_modules/@typescript-eslint/utils/dist/eslint-utils/RuleCreator.js:39:20)
at create (/node_modules/@vitest/eslint-plugin/dist/index.cjs:6355:35)
at Object.create (/node_modules/@typescript-eslint/utils/dist/eslint-utils/RuleCreator.js:39:20)
at createRuleListeners (/node_modules/eslint/lib/linter/linter.js:895:21)
at /node_modules/eslint/lib/linter/linter.js:1066:110
at Array.forEach ()
at runRules (/node_modules/eslint/lib/linter/linter.js:1003:34)

__tests__/transformers/mdxish-tables-to-jsx.test.ts

Oops! Something went wrong! :(

ESLint: 8.57.1

Error: Error while loading rule '@vitest/unbound-method': You have used a rule which requires type information, but don't have parserOptions set to generate type information for this file. See https://tseslint.com/typed-linting for enabling linting with type information.
Parser: /node_modules/@typescript-eslint/parser/dist/index.js
Occurred while linting /tests/transformers/mdxish-tables-to-jsx.test.ts
at throwError (/node_modules/@typescript-eslint/utils/dist/eslint-utils/getParserServices.js:40:11)
at getParserServices (/node_modules/@typescript-eslint/utils/dist/eslint-utils/getParserServices.js:29:9)
at create (/node_modules/@typescript-eslint/eslint-plugin/dist/rules/unbound-method.js:138:55)
at Object.create (/node_modules/@typescript-eslint/utils/dist/eslint-utils/RuleCreator.js:39:20)
at create (/node_modules/@vitest/eslint-plugin/dist/index.cjs:6355:35)
at Object.create (/node_modules/@typescript-eslint/utils/dist/eslint-utils/RuleCreator.js:39:20)
at createRuleListeners (/node_modules/eslint/lib/linter/linter.js:895:21)
at /node_modules/eslint/lib/linter/linter.js:1066:110
at Array.forEach ()
at runRules (/node_modules/eslint/lib/linter/linter.js:1003:34)


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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
processor/transform/mdxish/tables/mdxish-tables-to-jsx.ts (1)

85-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Rename the JSX-promotion flag.

canCellBeGfm also rejects newlines in inlineCode and raw html. In those cases, hasFlowContent is true even though the cell has no flow child. Rename it to requiresJsxTable.

As per coding guidelines: name code according to its behavior.

Proposed refactor
-      let hasFlowContent = false;
+      let requiresJsxTable = false;
...
-        hasFlowContent = true;
+        requiresJsxTable = true;
...
-      if (!hasFlowContent) {
+      if (!requiresJsxTable) {
🤖 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 `@processor/transform/mdxish/tables/mdxish-tables-to-jsx.ts` around lines 85 -
100, Rename the hasFlowContent flag to requiresJsxTable throughout the table
transformation logic, including its declaration and assignments, so the name
reflects that the condition determines whether JSX promotion is required rather
than whether flow content exists. Preserve the existing canCellBeGfm check and
branching behavior.

Source: Coding guidelines

__tests__/transformers/mdxish-tables-to-jsx.test.ts (1)

100-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add direct recursion coverage.

The current cases verify direct cell children. They do not exercise recursive normalizeNodes behavior. Add a focused normalizeCellForGfm AST test for breaks inside nested emphasis and strong, an escape node, and leading, trailing, and consecutive newlines. Keep the existing serialization tests.

Based on learnings: mdxishTables runs only in the MDXish pipeline, so equivalent RMDX coverage is not applicable. As per coding guidelines: unit-test dense logic directly and cover emphasis variants, escapes, nesting, whitespace, and formatting differences.

🤖 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 `@__tests__/transformers/mdxish-tables-to-jsx.test.ts` around lines 100 - 188,
Add focused unit coverage for normalizeCellForGfm that directly passes AST nodes
containing breaks nested within emphasis and strong, including escape nodes and
leading, trailing, and consecutive newline cases. Assert the normalized
recursive AST preserves the expected formatting and whitespace behavior, while
retaining the existing round-trip serialization tests unchanged.

Sources: Coding guidelines, Learnings

🤖 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 `@processor/transform/mdxish/tables/gfm-cell-normalization.ts`:
- Around line 26-30: Update isBareListMarker to accept only a single empty list
item whose checked state is null, preserving task-list state and multiple-item
counts through the existing JSX path. Add CX-3773 regression coverage in the
transformer and MDXish serializer suites for unchecked/checked task items and
multi-item empty lists, including the cited ticket.

---

Nitpick comments:
In `@__tests__/transformers/mdxish-tables-to-jsx.test.ts`:
- Around line 100-188: Add focused unit coverage for normalizeCellForGfm that
directly passes AST nodes containing breaks nested within emphasis and strong,
including escape nodes and leading, trailing, and consecutive newline cases.
Assert the normalized recursive AST preserves the expected formatting and
whitespace behavior, while retaining the existing round-trip serialization tests
unchanged.

In `@processor/transform/mdxish/tables/mdxish-tables-to-jsx.ts`:
- Around line 85-100: Rename the hasFlowContent flag to requiresJsxTable
throughout the table transformation logic, including its declaration and
assignments, so the name reflects that the condition determines whether JSX
promotion is required rather than whether flow content exists. Preserve the
existing canCellBeGfm check and branching behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2108cc21-e317-499b-9152-3fd7e33e6b4f

📥 Commits

Reviewing files that changed from the base of the PR and between d088998 and 90fced3.

📒 Files selected for processing (4)
  • __tests__/lib/mdxish/mdxishMdastToMd.test.ts
  • __tests__/transformers/mdxish-tables-to-jsx.test.ts
  • processor/transform/mdxish/tables/gfm-cell-normalization.ts
  • processor/transform/mdxish/tables/mdxish-tables-to-jsx.ts
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • readmeio/ai (manual)
  • readmeio/gitto (manual)
  • readmeio/markdown (manual)
  • readmeio/readme (manual)

Comment thread processor/transform/mdxish/tables/gfm-cell-normalization.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@__tests__/lib/mdxish/mdxishMdastToMd.test.ts`:
- Around line 1118-1141: Strengthen the tests around mdxishMdastToMd by
asserting serialized JSX content rather than only checking for <Table>. Verify
that the empty task item preserves its checked state, add a checked: true case,
and assert that the multiple-empty-item case retains both items. Include a
CX-3773 regression reference in the test descriptions or nearby test
documentation.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 12d6e845-6b8a-4121-839c-bab063f24d7e

📥 Commits

Reviewing files that changed from the base of the PR and between 90fced3 and f117fba.

📒 Files selected for processing (4)
  • __tests__/lib/mdxish/mdxishMdastToMd.test.ts
  • __tests__/transformers/mdxish-tables-to-jsx.test.ts
  • processor/transform/mdxish/tables/gfm-cell-normalization.ts
  • processor/transform/mdxish/tables/mdxish-tables-to-jsx.ts
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • readmeio/ai (manual)
  • readmeio/gitto (manual)
  • readmeio/markdown (manual)
  • readmeio/readme (manual)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/transformers/mdxish-tables-to-jsx.test.ts
  • processor/transform/mdxish/tables/mdxish-tables-to-jsx.ts

Comment thread __tests__/lib/mdxish/mdxishMdastToMd.test.ts Outdated
@eaglethrost
eaglethrost requested a review from kevinports August 4, 2026 11:11
@eaglethrost eaglethrost changed the title fix(mdxish): don't serialise GFM tables -> JSX because of line breaks fix(mdxish): don't serialise GFM tables -> JSX because of line breaks & bare list Aug 4, 2026
@eaglethrost eaglethrost changed the title fix(mdxish): don't serialise GFM tables -> JSX because of line breaks & bare list fix(mdxish): don't serialise GFM tables -> JSX because of line breaks Aug 4, 2026
@eaglethrost
eaglethrost merged commit 1ef7f10 into next Aug 5, 2026
8 checks passed
@eaglethrost
eaglethrost deleted the dimas/cx-3773-line-breaks-and-bare-block-markers-in-a-table-cell-silently branch August 5, 2026 03:39
rafegoldberg pushed a commit that referenced this pull request Aug 5, 2026
## Version 14.14.2
### 🛠 Fixes & Updates

* **mdxish:** don't serialise GFM tables -> JSX because of line breaks ([#1577](#1577)) ([1ef7f10](1ef7f10))

<!--SKIP CI-->
@rafegoldberg

Copy link
Copy Markdown
Collaborator

This PR was released!

🚀 Changes included in v14.14.2

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants