Enhance native Markdown support - #28
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docmostly/Features/Editor/NativeEditorMarkdownParser.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorMarkdownParser.swift:16">
P2: Escaped reference syntax is expanded into visible link destination text. Skip a `[` (and `![`) when preceded by an unescaped backslash so `\[label]` remains literal Markdown during import.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| static func blocks(from markdown: String) -> [NativeEditorBlock] { | ||
| let markdown = removingLeadingYAMLFrontMatter(from: markdown) | ||
| let markdown = resolvingReferenceStyleLinks( |
There was a problem hiding this comment.
P2: Escaped reference syntax is expanded into visible link destination text. Skip a [ (and ![) when preceded by an unescaped backslash so \[label] remains literal Markdown during import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/NativeEditorMarkdownParser.swift, line 16:
<comment>Escaped reference syntax is expanded into visible link destination text. Skip a `[` (and `![`) when preceded by an unescaped backslash so `\[label]` remains literal Markdown during import.</comment>
<file context>
@@ -13,7 +13,9 @@ enum NativeEditorMarkdownParser {
static func blocks(from markdown: String) -> [NativeEditorBlock] {
- let markdown = removingLeadingYAMLFrontMatter(from: markdown)
+ let markdown = resolvingReferenceStyleLinks(
+ in: removingLeadingYAMLFrontMatter(from: markdown)
+ )
</file context>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0444922b9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| } | ||
|
|
||
| if let commonMarkBlock = commonMarkBlock(in: lines, startingAt: index) { |
There was a problem hiding this comment.
When this new CommonMark block check runs before listItemBlock, any line with four leading spaces is consumed as an indented code block. The exporter still emits two spaces per list indent, so an indentLevel == 2 list item round-trips as - Grandchild and is imported as code instead of a nested list item, corrupting documents with lists deeper than one level. Defer indented-code parsing until after list detection, or exclude indented list markers from the code-block path.
Useful? React with 👍 / 👎.
| resolvedFence = openingFence | ||
| return line | ||
| } | ||
| return replacingReferenceStyleLinks(in: line, definitions: definitions) |
There was a problem hiding this comment.
Because every non-fenced line is passed through replacingReferenceStyleLinks whenever a definition exists, lines that will later be parsed as indented code are still rewritten. For example, a four-space code line containing [Guide][docs] is converted to [Guide](...) before indentedCodeBlock builds the code block, so imported code content is mutated; skip indented code ranges here or resolve references after block classification.
Useful? React with 👍 / 👎.
| let startsOrderedList = startsWithOrderedListMarker(text) | ||
| let isExactMarker = exactMarkers.contains(text) | ||
| guard startsFixedSyntax || startsFence || startsOrderedList || isExactMarker else { return text } | ||
| return "\\\(text)" |
There was a problem hiding this comment.
When paragraph text starts with an ordered-list marker, this branch emits a leading escape before the digit, e.g. \1. item. Markdown backslash escapes only apply to punctuation, and the importer’s unescaper also leaves a slash before 1, so exporting and re-importing a literal 1. item paragraph changes the saved text to include the backslash. Escape the period instead, or otherwise make this ordered-list escape symmetric.
Useful? React with 👍 / 👎.
| private static func escapingMarkdownBlockPrefix(_ line: Substring) -> String { | ||
| let text = String(line) | ||
| let fixedPrefixes = ["# ", "## ", "### ", "#### ", "##### ", "###### ", "> ", "- ", "+ "] | ||
| let exactMarkers = ["---", "***", "___"] |
There was a problem hiding this comment.
This only treats exact ---, ***, and ___ as block-leading syntax, but the new setext parser accepts any non-empty line made entirely of - or = as a heading underline. A paragraph with a hard break before ---- or ==== therefore exports unchanged and re-imports as a heading, losing the second paragraph line. Escape all setext underline-only lines, not just the exact divider markers.
Useful? React with 👍 / 👎.
| let isImage = line[index] == "!" && line.index(after: index) < line.endIndex && | ||
| line[line.index(after: index)] == "[" | ||
| let openLabel = isImage ? line.index(after: index) : index | ||
| if line[openLabel] == "[", |
There was a problem hiding this comment.
This matcher does not check whether the opening [ is escaped, so a literal reference such as \[Guide][docs] is still rewritten when a [docs]: ... definition exists. The later inline parser keeps the escaped bracket literal, but the content has already been mutated to \[Guide](...), changing user text that explicitly opted out of link parsing. Ignore escaped opening brackets before resolving reference-style links.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0444922b9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| } | ||
|
|
||
| if let commonMarkBlock = commonMarkBlock(in: lines, startingAt: index) { |
There was a problem hiding this comment.
When this new CommonMark block check runs before listItemBlock, any line with four leading spaces is consumed as an indented code block. The exporter still emits two spaces per list indent, so an indentLevel == 2 list item round-trips as - Grandchild and is imported as code instead of a nested list item, corrupting documents with lists deeper than one level. Defer indented-code parsing until after list detection, or exclude indented list markers from the code-block path.
Useful? React with 👍 / 👎.
| resolvedFence = openingFence | ||
| return line | ||
| } | ||
| return replacingReferenceStyleLinks(in: line, definitions: definitions) |
There was a problem hiding this comment.
Because every non-fenced line is passed through replacingReferenceStyleLinks whenever a definition exists, lines that will later be parsed as indented code are still rewritten. For example, a four-space code line containing [Guide][docs] is converted to [Guide](...) before indentedCodeBlock builds the code block, so imported code content is mutated; skip indented code ranges here or resolve references after block classification.
Useful? React with 👍 / 👎.
| let startsOrderedList = startsWithOrderedListMarker(text) | ||
| let isExactMarker = exactMarkers.contains(text) | ||
| guard startsFixedSyntax || startsFence || startsOrderedList || isExactMarker else { return text } | ||
| return "\\\(text)" |
There was a problem hiding this comment.
When paragraph text starts with an ordered-list marker, this branch emits a leading escape before the digit, e.g. \1. item. Markdown backslash escapes only apply to punctuation, and the importer’s unescaper also leaves a slash before 1, so exporting and re-importing a literal 1. item paragraph changes the saved text to include the backslash. Escape the period instead, or otherwise make this ordered-list escape symmetric.
Useful? React with 👍 / 👎.
| private static func escapingMarkdownBlockPrefix(_ line: Substring) -> String { | ||
| let text = String(line) | ||
| let fixedPrefixes = ["# ", "## ", "### ", "#### ", "##### ", "###### ", "> ", "- ", "+ "] | ||
| let exactMarkers = ["---", "***", "___"] |
There was a problem hiding this comment.
This only treats exact ---, ***, and ___ as block-leading syntax, but the new setext parser accepts any non-empty line made entirely of - or = as a heading underline. A paragraph with a hard break before ---- or ==== therefore exports unchanged and re-imports as a heading, losing the second paragraph line. Escape all setext underline-only lines, not just the exact divider markers.
Useful? React with 👍 / 👎.
| let isImage = line[index] == "!" && line.index(after: index) < line.endIndex && | ||
| line[line.index(after: index)] == "[" | ||
| let openLabel = isImage ? line.index(after: index) : index | ||
| if line[openLabel] == "[", |
There was a problem hiding this comment.
This matcher does not check whether the opening [ is escaped, so a literal reference such as \[Guide][docs] is still rewritten when a [docs]: ... definition exists. The later inline parser keeps the escaped bracket literal, but the content has already been mutated to \[Guide](...), changing user text that explicitly opted out of link parsing. Ignore escaped opening brackets before resolving reference-style links.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0444922b9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| } | ||
|
|
||
| if let commonMarkBlock = commonMarkBlock(in: lines, startingAt: index) { |
There was a problem hiding this comment.
When this new CommonMark block check runs before listItemBlock, any line with four leading spaces is consumed as an indented code block. The exporter still emits two spaces per list indent, so an indentLevel == 2 list item round-trips as - Grandchild and is imported as code instead of a nested list item, corrupting documents with lists deeper than one level. Defer indented-code parsing until after list detection, or exclude indented list markers from the code-block path.
Useful? React with 👍 / 👎.
| resolvedFence = openingFence | ||
| return line | ||
| } | ||
| return replacingReferenceStyleLinks(in: line, definitions: definitions) |
There was a problem hiding this comment.
Because every non-fenced line is passed through replacingReferenceStyleLinks whenever a definition exists, lines that will later be parsed as indented code are still rewritten. For example, a four-space code line containing [Guide][docs] is converted to [Guide](...) before indentedCodeBlock builds the code block, so imported code content is mutated; skip indented code ranges here or resolve references after block classification.
Useful? React with 👍 / 👎.
| let startsOrderedList = startsWithOrderedListMarker(text) | ||
| let isExactMarker = exactMarkers.contains(text) | ||
| guard startsFixedSyntax || startsFence || startsOrderedList || isExactMarker else { return text } | ||
| return "\\\(text)" |
There was a problem hiding this comment.
When paragraph text starts with an ordered-list marker, this branch emits a leading escape before the digit, e.g. \1. item. Markdown backslash escapes only apply to punctuation, and the importer’s unescaper also leaves a slash before 1, so exporting and re-importing a literal 1. item paragraph changes the saved text to include the backslash. Escape the period instead, or otherwise make this ordered-list escape symmetric.
Useful? React with 👍 / 👎.
| private static func escapingMarkdownBlockPrefix(_ line: Substring) -> String { | ||
| let text = String(line) | ||
| let fixedPrefixes = ["# ", "## ", "### ", "#### ", "##### ", "###### ", "> ", "- ", "+ "] | ||
| let exactMarkers = ["---", "***", "___"] |
There was a problem hiding this comment.
This only treats exact ---, ***, and ___ as block-leading syntax, but the new setext parser accepts any non-empty line made entirely of - or = as a heading underline. A paragraph with a hard break before ---- or ==== therefore exports unchanged and re-imports as a heading, losing the second paragraph line. Escape all setext underline-only lines, not just the exact divider markers.
Useful? React with 👍 / 👎.
| let isImage = line[index] == "!" && line.index(after: index) < line.endIndex && | ||
| line[line.index(after: index)] == "[" | ||
| let openLabel = isImage ? line.index(after: index) : index | ||
| if line[openLabel] == "[", |
There was a problem hiding this comment.
This matcher does not check whether the opening [ is escaped, so a literal reference such as \[Guide][docs] is still rewritten when a [docs]: ... definition exists. The later inline parser keeps the escaped bracket literal, but the content has already been mutated to \[Guide](...), changing user text that explicitly opted out of link parsing. Ignore escaped opening brackets before resolving reference-style links.
Useful? React with 👍 / 👎.
|
| Filename | Overview |
|---|---|
| docmostly/Features/Editor/NativeEditorMarkdownParser+Escaping.swift | New file adding block-level and inline escape/unescape helpers; ordered-list escaping uses a \N. pattern that isn't a valid CommonMark escape sequence, breaking the round-trip for paragraphs starting with ordered-list notation. |
| docmostly/Features/Editor/NativeEditorMarkdownParser+References.swift | New file resolving full, collapsed, and shortcut reference-style links/images; the definition parser uses an unescaped bracket search which can mishandle labels containing \]. |
| docmostly/Features/Editor/NativeEditorMarkdownParser+CommonMarkBlocks.swift | New file adding setext heading and indented code block parsing; logic looks correct, including tab expansion and trailing-blank-line trimming for code blocks. |
| docmostly/Features/Editor/NativeEditorMarkdownParser+FrontMatter.swift | Refactored removingLeadingYAMLFrontMatter into its own extension file with no functional changes. |
| docmostly/Features/Editor/NativeEditorMarkdownParser+InlineMarks.swift | Updated inline mark parsing to respect escape sequences when locating delimiters and to unescape plain-text spans; the link-label escaping helper is removed and consolidated into the new Escaping extension. |
| docmostly/Features/Editor/NativeEditorBlockKind.swift | Adds per-level heading fonts for levels 3–6 and a stronglyEmphasizedEditorFont variant using .heavy weight for bold-within-heading rendering. |
| docmostly/Features/Editor/NativeEditorRichBlockPreviewView.swift | Adds previewText computed property on the View to apply heavy-weight font to strongly-emphasized runs inside headings; view logic would be more testable in a view model per project guidelines. |
| docmostly/Features/Editor/NativeEditorTextInputView+iOS.swift | Threads NativeEditorBlockKind through renderedText/applyInlinePresentationFonts to apply .heavy UIFont weight for bold runs inside headings; adds a headingTextStyle private extension covering all 6 heading levels. |
| docmostly/Features/Editor/NativeEditorTextInputView+macOS.swift | Mirrors the iOS heading-font changes for macOS using NSFont and NSFontManager; parallel with iOS implementation and looks correct. |
| docmostlyTests/Editor/NativeEditorMarkdownParityTests.swift | New parity test file; the parameterized literalMarkdownSyntaxRoundTripsAsParagraph test includes "1. literal ordered list" which will fail at runtime due to the ordered-list escaping bug. |
| docmostlyTests/Editor/NativeEditorHeadingMarkdownTests.swift | Adds a bold-heading round-trip test through ProseMirror; test structure and assertions look correct. |
| docmostly/Features/Editor/NativeEditorMarkdownParser.swift | Wires commonMarkBlock and resolvingReferenceStyleLinks into the parsing pipeline; the default export case now uses escapedBlockLeadingMarkdown, which introduces the ordered-list escaping bug. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["blocks(from: markdown)"] --> B["removingLeadingYAMLFrontMatter"]
B --> C["resolvingReferenceStyleLinks"]
C --> D{"Per-line loop"}
D -->|fenced code| E["fencedCodeBlock"]
D -->|setext / indented| F["commonMarkBlock (NEW)"]
F --> F1["setextHeadingBlock"]
F --> F2["indentedCodeBlock"]
D -->|images| G["appendImageMarkdownBlocks..."]
D -->|ATX heading / list / quote| H["richBlock"]
D -->|table| I["tableBlock"]
D -->|blockquote| J["blockquoteBlock"]
D -->|list item| K["listItemBlock"]
D -->|paragraph| L["paragraphBlock"]
M["markdown(from: blocks)"] --> N{"Per-block switch"}
N -->|heading| O["## text"]
N -->|list / quote / code / divider / table| P["structured prefix"]
N -->|paragraph / other| Q["escapedBlockLeadingMarkdown (NEW)"]
Q --> Q1["escapedMarkdownPlainText inline"]
Q --> Q2["escapingMarkdownBlockPrefix - ordered-list branch bug"]
style Q2 fill:#ffcccc,stroke:#cc0000
Comments Outside Diff (2)
-
docmostly/Features/Editor/NativeEditorMarkdownParser+Escaping.swift, line 227-235 (link)Ordered-list escape corrupts round-trip text
escapingMarkdownBlockPrefixprepends\to ordered-list lines, producing e.g.\1. literal ordered list. However\1is not a valid CommonMark escape sequence — digits are not ASCII punctuation — sounescapedMarkdownPlainTextdoes not consume the backslash and the re-imported paragraph text becomes\1. literal ordered listinstead of1. literal ordered list. The parameterized parity test for"1. literal ordered list"would fail at runtime. The fix is to escape the.after the digits (e.g. produce1\. text) since.IS inmarkdownEscapableCharactersandunescapedMarkdownPlainTextwill correctly strip the backslash on re-import.Prompt To Fix With AI
This is a comment left during a code review. Path: docmostly/Features/Editor/NativeEditorMarkdownParser+Escaping.swift Line: 227-235 Comment: **Ordered-list escape corrupts round-trip text** `escapingMarkdownBlockPrefix` prepends `\` to ordered-list lines, producing e.g. `\1. literal ordered list`. However `\1` is not a valid CommonMark escape sequence — digits are not ASCII punctuation — so `unescapedMarkdownPlainText` does not consume the backslash and the re-imported paragraph text becomes `\1. literal ordered list` instead of `1. literal ordered list`. The parameterized parity test for `"1. literal ordered list"` would fail at runtime. The fix is to escape the `.` after the digits (e.g. produce `1\. text`) since `.` IS in `markdownEscapableCharacters` and `unescapedMarkdownPlainText` will correctly strip the backslash on re-import. How can I resolve this? If you propose a fix, please make it concise.
-
docmostly/Features/Editor/NativeEditorRichBlockPreviewView.swift, line 684-695 (link)View logic in a computed property rather than a view model
The AGENTS.md guide says "Place view logic into view models or similar, so it can be tested."
previewTextperforms non-trivialAttributedStringmutation — iterating runs and applying per-range fonts based on block kind — that would be straightforward to unit-test in isolation. As a computed property directly on theViewstruct it bypasses that guidance. Moving this transform into a dedicated helper (e.g. a static function onNativeEditorBlockor a view model) would make it independently testable.Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review. Path: docmostly/Features/Editor/NativeEditorRichBlockPreviewView.swift Line: 684-695 Comment: **View logic in a computed property rather than a view model** The AGENTS.md guide says "Place view logic into view models or similar, so it can be tested." `previewText` performs non-trivial `AttributedString` mutation — iterating runs and applying per-range fonts based on block kind — that would be straightforward to unit-test in isolation. As a computed property directly on the `View` struct it bypasses that guidance. Moving this transform into a dedicated helper (e.g. a static function on `NativeEditorBlock` or a view model) would make it independently testable. **Context Used:** AGENTS.md ([source](https://app.greptile.com/chefski/github/Chefski/docmostly/-/custom-context?memory=b96280a5-77a8-422c-b334-bf960d63b0fc)) How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
docmostly/Features/Editor/NativeEditorMarkdownParser+Escaping.swift:227-235
**Ordered-list escape corrupts round-trip text**
`escapingMarkdownBlockPrefix` prepends `\` to ordered-list lines, producing e.g. `\1. literal ordered list`. However `\1` is not a valid CommonMark escape sequence — digits are not ASCII punctuation — so `unescapedMarkdownPlainText` does not consume the backslash and the re-imported paragraph text becomes `\1. literal ordered list` instead of `1. literal ordered list`. The parameterized parity test for `"1. literal ordered list"` would fail at runtime. The fix is to escape the `.` after the digits (e.g. produce `1\. text`) since `.` IS in `markdownEscapableCharacters` and `unescapedMarkdownPlainText` will correctly strip the backslash on re-import.
### Issue 2 of 3
docmostly/Features/Editor/NativeEditorMarkdownParser+References.swift:57-61
**Unescaped bracket search in reference-definition parser**
`referenceDefinition` uses `trimmed.firstIndex(of: "]")` to locate the end of the link label, which does not respect `\]` escapes inside the label. A label like `[foo\]bar]` would split at the `\]` position, extracting `foo\` as the label and leaving `bar]` to fail the colon check, silently rejecting a valid definition. The `closingReferenceBracket` helper used elsewhere already handles escapes correctly; it could be reused here.
### Issue 3 of 3
docmostly/Features/Editor/NativeEditorRichBlockPreviewView.swift:684-695
**View logic in a computed property rather than a view model**
The AGENTS.md guide says "Place view logic into view models or similar, so it can be tested." `previewText` performs non-trivial `AttributedString` mutation — iterating runs and applying per-range fonts based on block kind — that would be straightforward to unit-test in isolation. As a computed property directly on the `View` struct it bypasses that guidance. Moving this transform into a dedicated helper (e.g. a static function on `NativeEditorBlock` or a view model) would make it independently testable.
Reviews (1): Last reviewed commit: "feat: enhance native markdown support" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let trimmed = line.dropFirst(leadingSpaces) | ||
| guard trimmed.first == "[", | ||
| let closeLabel = trimmed.firstIndex(of: "]") else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
referenceDefinition uses trimmed.firstIndex(of: "]") to locate the end of the link label, which does not respect \] escapes inside the label. A label like [foo\]bar] would split at the \] position, extracting foo\ as the label and leaving bar] to fail the colon check, silently rejecting a valid definition. The closingReferenceBracket helper used elsewhere already handles escapes correctly; it could be reused here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docmostly/Features/Editor/NativeEditorMarkdownParser+References.swift
Line: 57-61
Comment:
**Unescaped bracket search in reference-definition parser**
`referenceDefinition` uses `trimmed.firstIndex(of: "]")` to locate the end of the link label, which does not respect `\]` escapes inside the label. A label like `[foo\]bar]` would split at the `\]` position, extracting `foo\` as the label and leaving `bar]` to fail the colon check, silently rejecting a valid definition. The `closingReferenceBracket` helper used elsewhere already handles escapes correctly; it could be reused here.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Summary
Why
The native editor supported the most common Markdown constructs, but heading typography and several CommonMark-compatible forms did not match the Docmost web editor. Literal Markdown characters could also be interpreted as formatting during a round trip.
Impact
Imported Markdown now retains more of its intended document structure, headings display consistently as bold headings, and literal syntax is exported safely. This closes several practical parity gaps without introducing a third-party dependency.
Full arbitrary nested CommonMark structures and unrestricted HTML remain outside the scope of this change.
Review fixes
Validation
All current PR checks pass: