Repository navigation
A document read as a document, and boxes you can tick - #21
Conversation
**Every gap left in the Markdown preview was the same gap.** The parser was hand-rolled and line-by-line — each source line classified on its own — so a paragraph hard-wrapped at 80 columns became one block per line and was drawn as a ladder, a continuation line under a bullet lost its bullet, `Title` over `=====` came out as a paragraph and a rule, and a fence inside a list item was hoisted out of it. Sixteen lines in `TODO.md`, one missing thing: a block tree. So the parser is replaced and the renderer is kept. [swift-markdown](https://github.com/swiftlang/swift-markdown) is a parser and nothing else — an immutable tree over swift-cmark, the same cmark-gfm both hosts render a comment with — so the app now reads a document the way the host that served it does. `Support/MarkdownParser.swift` is the whole of the walk from its nodes to ours; `Docs/Markdown.md` records why a library that renders too was the wrong trade. The tree answers, for free: hard-wrapped paragraphs, multi-line list items, nested and ordered checklists, setext headings, every spelling of a horizontal rule (`___`, `----`, `- - -`), indented code blocks, reference-style links, table alignment, and a fence or a sub-list inside a list item. **Three things it does not answer**, and each has its place: *Bare URLs, `:tada:`, `#123` and `@name` are not Markdown at all.* They are a scan over the `Text` nodes, which makes them one piece of work rather than four — `Support/MarkdownInline.swift`. It does the escaping in the same pass, and that is why the two live together: the blocks carry Markdown *strings*, which the views read back with `AttributedString(markdown:)`, so a `*` the author escaped has to be written back escaped or it comes out as emphasis the second time round. *HTML arrives raw and flat.* cmark splits `<details>` into three siblings — the open tag, the inner Markdown parsed properly, the close tag — so there is nothing to recurse into. `MarkdownHTMLText` reads one raw block into events and `ContainerStack` keeps the stack. Loose HTML is handed back as Markdown and re-read as a document of its own, which is what finally makes a README's `<p align="center"><img …></p>` a picture. A `<table>` arrives whole in one block, so the plain shape is read into a real table. *Footnotes are worse than missing.* A definition whose note is one word is a valid link reference definition, so cmark eats the line and turns `[^1]` into a link pointing at the note's words. Both halves are caught: a definition left as a paragraph is lifted to the end of the document, and a mangled reference is drawn as the marker it was. **Comments are dropped from the tree, not from the text.** `strippingComments` cut whole lines out before parsing, which moved every line number after them — and a source range is the one thing a tickable checkbox depends on. A tick is a write at the item's own line: through `OpenDocument` for a `.md` file, where it lands as an unsaved edit like any other, and through the comment and description edit paths for a pull request, from what the host holds rather than from what is drawn. The rest of the list with it: table alignment now reaches the cell, a fenced block has a copy button, a picture opens at full size instead of being squinted at under its 420pt cap, and a long document has an outline. ## Scrolling A pull request draws its comments in a `LazyVStack`, so a comment scrolled out of sight is thrown away and built again on the way back — which is why the cost of one comment shows up as a stutter rather than as load time. Measured, not guessed: **Every paragraph was laid out twice.** `CodeChipBackdrop` puts a second copy of the text behind the first so a `code` span can have a rounded fill, and it went behind every line in the document. Most prose has no span at all, and that half of the work was never visible. Now it is attached only where there is a chip to draw. This one predates the branch. **A name on every block.** The outline needs a heading to answer to a name; it was putting one on all of them, in every `MarkdownText` on the page — the same handful of names over and over. An explicit `.id` is structural identity, so SwiftUI tore views down and built them again as the indices moved. **A closure rebuilt every pass.** A closure is never equal to another closure, so an action written into the environment on each draw read as a change to everything beneath it, and defeated the check that lets an unchanged comment be skipped whole. `MarkdownTaskToggle` is equal by what it writes to, and is not set at all for a comment nobody here may edit — which is most of them. **Hover on a fence rebuilt the fence.** Scrolling drags the document under a pointer that never moved, so every block it crosses reports a hover. The state now sits in the overlay, where it rebuilds one button. Plus: an item of one paragraph is drawn as the line it is rather than as a nested `MarkdownText`, and the caches are sized for a page rather than for a view — 2500 lines and 500 documents, against 600 and 200. ## Also here Three changes that were already in the tree and are not part of the above: `marksman` joins the language server catalogue; `EditorThemeBridge` gains a note on why a `.md` file paints flat in the editor and what fixing it would cost; and a diff now folds away the files it deletes, which is the one file nobody reads. Built and run. The parser was checked against the whole of `Docs/`, `README.md` and `TODO.md`, and against the shapes a review bot writes — nested `<details>`/`<blockquote>`, `<kbd>` chips, HTML tables, badge links, Bitbucket's `{: width=140 }`. `TODO.md` is rewritten in the same pass, and not only because those sixteen lines are done: its items were written `[ ] thing` with no `-`, which is not a list in Markdown at all. The old parser made it *look* like one; cmark reads it the way GitHub does, as one paragraph. They are real list items now, so the file renders as a checklist here and on the host, and its boxes tick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAgEj6aqwdPgmZEq5BapSu
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change migrates Markdown parsing to swift-markdown, adds nested list and aligned table rendering, introduces inline Markdown rewriting, supports checklist persistence and heading outlines, improves image interaction, registers Marksman, updates documentation, and preserves folded deleted files across diff reloads. ChangesMarkdown migration
Diff deletion folding
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant ViewerView
participant MarkdownPreview
participant MarkdownTask
participant DocumentStore
ViewerView->>MarkdownPreview: provide task toggle callback
MarkdownPreview->>MarkdownTask: toggle checkbox on source line
MarkdownTask-->>ViewerView: return updated Markdown
ViewerView->>DocumentStore: persist updated text
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
Package.swift (1)
44-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDeclare a product for direct
swift-cmarkuse.
swift-cmark0.8.0 exists, but it exposescmark-gfmandcmark-gfm-extensionsproducts rather than an implicit package product. This target already importsMarkdowninstead, so the direct package is not unused; to keep the grammar-version pin without relying on a future upstream product change, drop the raw declaration and add a localMarkdownSwiftCmarkproduct/package wrapper that exposes the neededcmark-gfm*product.🤖 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 `@Package.swift` around lines 44 - 54, Update the Package.swift dependency declarations around swift-markdown and swift-cmark to remove the raw direct swift-cmark package entry and add the local MarkdownSwiftCmark product/package wrapper exposing the cmark-gfm and cmark-gfm-extensions products. Preserve the exact 0.8.0 grammar-version pin and keep the existing Markdown dependency usage unchanged.Sources/Workspace/Views/DiffView.swift (1)
120-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid rebuilding the flattened diff twice.
foldDeletions()mutatescollapsedFiles, while the revision handler immediately callsrebuild(collapsed:)and the separatecollapsedFilesobserver callsrebuild()again. On large diffs with new deletions, this can perform two full flattening passes. Coalesce these updates or defer the state write until after the rebuild.Also applies to: 233-234
🤖 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 `@Sources/Workspace/Views/DiffView.swift` at line 120, Update the change handlers in DiffView around foldDeletions(), the diff.revision observer, and the collapsedFiles observer so a revision update that changes collapsedFiles triggers only one rebuild() pass. Coalesce the state mutation and rebuild, or defer the collapsedFiles write until after the explicit rebuild, while preserving initial rebuilding and deletion-folding behavior.
🤖 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 `@CLAUDE.md`:
- Line 105: Update the wording in the table description so “read only” becomes
the hyphenated “read-only” when modifying “shape”; leave the rest of the
statement unchanged.
In `@Docs/Markdown.md`:
- Line 45: Update the table example in Markdown.md so each pipe character in the
columnAlignments value is escaped, preserving the row as exactly two cells and
preventing markdownlint MD056.
In `@Sources/Workspace/Editor/EditorThemeBridge.swift`:
- Around line 24-31: Revise the Markdown explanation in EditorThemeBridge.swift
to state that normal Markdown preview coloring comes through MarkdownText and
the parser/AttributedString path, while TreeSitterHighlighter applies only to
fenced code snippets. Remove the claim that TreeSitterHighlighter processes the
same full .md file, preserving the existing explanation of the unrecognised
token list.
In `@Sources/Workspace/Support/MarkdownHTMLText.swift`:
- Around line 163-202: Update table(in:) to track the header row by value rather
than its pre-filter index. When processing the first header cell, retain the
corresponding row value or otherwise identify it after empty rows are removed,
then remove that header row safely while preserving the default first-row
behavior when no header exists.
In `@Sources/Workspace/Support/MarkdownInline.swift`:
- Around line 35-43: Update the www. autolink construction in the inline parsing
branch to pass its generated destination through destination(_:) before
interpolation, while preserving the existing escaping and https:// prefix
behavior.
In `@Sources/Workspace/Support/MarkdownParser.swift`:
- Around line 242-247: Thread an isNested (or equivalent) flag through the
blocks(of:) builder and its nested HTML re-parse path, setting it for
Document(parsing: text) content. In list(_:isOrdered:start:), omit the source
line when building Markdown.List.Item while this flag is set, while preserving
line ranges for top-level document lists.
In `@Sources/Workspace/Support/RemoteImage.swift`:
- Around line 164-195: The zoom interaction in the image view’s onTapGesture is
pointer-only and lacks button semantics. Add an accessibility action that sets
isZoomed to true and mark the image with the button accessibility trait,
preserving the existing click behavior and sheet presentation.
In `@Sources/Workspace/Views/MarkdownPreview.swift`:
- Around line 857-862: Document on MarkdownTaskToggle that perform must read
mutable source data when invoked because equality compares only target and
ignores captured values. In Sources/Workspace/Views/PullRequestDetailView.swift
lines 2387-2397, update the action construction to read or re-fetch the comment
body inside the perform closure rather than capturing comment.editableBody when
constructing the action.
In `@Sources/Workspace/Views/ViewerView.swift`:
- Around line 382-399: Update the MarkdownPreview link configuration to resolve
the project containing document.url via store.project(containing:) instead of
store.selectedProject, and pass that project’s remote to MarkdownLinks. Keep the
existing preview and task-toggle behavior unchanged.
---
Nitpick comments:
In `@Package.swift`:
- Around line 44-54: Update the Package.swift dependency declarations around
swift-markdown and swift-cmark to remove the raw direct swift-cmark package
entry and add the local MarkdownSwiftCmark product/package wrapper exposing the
cmark-gfm and cmark-gfm-extensions products. Preserve the exact 0.8.0
grammar-version pin and keep the existing Markdown dependency usage unchanged.
In `@Sources/Workspace/Views/DiffView.swift`:
- Line 120: Update the change handlers in DiffView around foldDeletions(), the
diff.revision observer, and the collapsedFiles observer so a revision update
that changes collapsedFiles triggers only one rebuild() pass. Coalesce the state
mutation and rebuild, or defer the collapsedFiles write until after the explicit
rebuild, while preserving initial rebuilding and deletion-folding 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 076d4eeb-ca4c-4a1d-9d0d-2e6b95a3f851
⛔ Files ignored due to path filters (1)
Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (17)
CLAUDE.mdDocs/Development.mdDocs/Markdown.mdPackage.swiftREADME.mdSources/Workspace/Editor/EditorThemeBridge.swiftSources/Workspace/Models/LanguageServerCatalog.swiftSources/Workspace/Support/MarkdownHTML.swiftSources/Workspace/Support/MarkdownHTMLText.swiftSources/Workspace/Support/MarkdownInline.swiftSources/Workspace/Support/MarkdownParser.swiftSources/Workspace/Support/RemoteImage.swiftSources/Workspace/Views/DiffView.swiftSources/Workspace/Views/MarkdownPreview.swiftSources/Workspace/Views/PullRequestDetailView.swiftSources/Workspace/Views/ViewerView.swiftTODO.md
| `Support/MarkdownHTMLText.swift` reads one raw block into events and | ||
| `ContainerStack` in the parser keeps the stack. It is not an HTML parser: an | ||
| unknown tag is dropped and its text kept, `<!-- … -->` goes nowhere, and | ||
| `<table>` is read only in its plain shape. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate “read-only”.
Line 105 should use “read-only” when modifying “shape”; this is also reported by the grammar checker.
Suggested wording
- `<table>` is read only in its plain shape.
+ `<table>` is read-only in its plain shape.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `<table>` is read only in its plain shape. | |
| `<table>` is read-only in its plain shape. |
🧰 Tools
🪛 LanguageTool
[grammar] ~105-~105: Use a hyphen to join words.
Context: ... goes nowhere, and <table> is read only in its plain shape. - Comments ar...
(QB_NEW_EN_HYPHEN)
🤖 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 `@CLAUDE.md` at line 105, Update the wording in the table description so “read
only” becomes the hyphenated “read-only” when modifying “shape”; leave the rest
of the statement unchanged.
Source: Linters/SAST tools
**An HTML table with an empty row took the app down.** `table(in:)` recorded the header as an *index* while scanning and then dropped the empty rows out from under it, so every row after an empty `<tr></tr>` was renumbered — and `<tr></tr><tr><th>A</th></tr>` left one row with the header pointing at the second. `Index out of range`, reproduced before fixing and after. The header's place is now corrected in the same breath as the rows are dropped. **A tick inside HTML wrote to the wrong line.** Loose HTML is re-read as a document of its own, and cmark numbers that document from *its* first line — so a checkbox in there named a line in the snippet and a tick would have been written over whatever sat on that line of the real document. `depth` already counted the re-read; the item's line is now only recorded at the top level. A box in there is drawn and not offered, which is the right answer. **A second tick could discard the first.** `MarkdownTaskToggle` compares on `target` so that a rebuilt action reads as unchanged — but a comment's action captures its body when the closure is made, so the kept closure would compute the second tick against the words from before the first. The body is part of equality now, which retires the stale closure and costs nothing in the ordinary case: the same string, so `==` answers on the pointer. Smaller, all of them real: - A `.md` file takes its `#123` from the repository it is *in* (`project(containing:)`), not from whichever is selected in the sidebar. - A bare `www.` address goes through `destination(_:)` like every other, so one with a bracket in it cannot break out of the link. - The picture says it is a button and answers an accessibility action, rather than being openable by pointer alone. - `Docs/Markdown.md` escapes the pipes inside `columnAlignments: \|l\|c\|r\|`, which were splitting that row into six cells. - `EditorThemeBridge` no longer claims the preview colours a `.md` file through `TreeSitterHighlighter`; that reaches only as far as the snippet in a fence. - `CLAUDE.md` says "only the plain shape of a `<table>` is read" rather than "`<table>` is read only in its plain shape" — the linter wanted "read-only", which means the opposite of what the sentence does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAgEj6aqwdPgmZEq5BapSu
|
All nine addressed in 34a2634. Each was checked against the code first; three were more than cosmetic. Confirmed as a crash — Confirmed, and it could have corrupted a document — the nested re-parse. Confirmed — the Applied as suggested: One taken differently. The On the two failing pre-merge checks: the docstring coverage number counts the parser's small private helpers, most of which sit under a doc comment that explains the whole method — I would rather not add noise to reach a percentage. The title is deliberate; it follows the house style of the last several PRs here. |
Main arrived as the squash of #21, whose content this branch was already built on, so the only conflict was in the bubble: the same lines this branch moved out of `body` and into `folding`, which is what the merge keeps — plus the `taskToggle` the merge had written down twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Every gap left in the Markdown preview was the same gap. The parser was
hand-rolled and line-by-line — each source line classified on its own — so a
paragraph hard-wrapped at 80 columns became one block per line and was drawn as
a ladder, a continuation line under a bullet lost its bullet,
Titleover=====came out as a paragraph and a rule, and a fence inside a list item washoisted out of it. Sixteen lines in
TODO.md, one missing thing: a block tree.So the parser is replaced and the renderer is kept.
swift-markdown is a parser and
nothing else — an immutable tree over swift-cmark, the same cmark-gfm both hosts
render a comment with — so the app now reads a document the way the host that
served it does.
Support/MarkdownParser.swiftis the whole of the walk from itsnodes to ours;
Docs/Markdown.mdrecords why a library that renders too was thewrong trade.
The tree answers, for free: hard-wrapped paragraphs, multi-line list items,
nested and ordered checklists, setext headings, every spelling of a horizontal
rule (
___,----,- - -), indented code blocks, reference-style links,table alignment, and a fence or a sub-list inside a list item.
Three things it does not answer, and each has its place:
Bare URLs,
:tada:,#123and@nameare not Markdown at all. They are ascan over the
Textnodes, which makes them one piece of work rather than four—
Support/MarkdownInline.swift. It does the escaping in the same pass, andthat is why the two live together: the blocks carry Markdown strings, which
the views read back with
AttributedString(markdown:), so a*the authorescaped has to be written back escaped or it comes out as emphasis the second
time round.
HTML arrives raw and flat. cmark splits
<details>into three siblings — theopen tag, the inner Markdown parsed properly, the close tag — so there is
nothing to recurse into.
MarkdownHTMLTextreads one raw block into events andContainerStackkeeps the stack. Loose HTML is handed back as Markdown andre-read as a document of its own, which is what finally makes a README's
<p align="center"><img …></p>a picture. A<table>arrives whole in oneblock, so the plain shape is read into a real table.
Footnotes are worse than missing. A definition whose note is one word is a
valid link reference definition, so cmark eats the line and turns
[^1]into alink pointing at the note's words. Both halves are caught: a definition left as
a paragraph is lifted to the end of the document, and a mangled reference is
drawn as the marker it was.
Comments are dropped from the tree, not from the text.
strippingCommentscut whole lines out before parsing, which moved every line number after them —
and a source range is the one thing a tickable checkbox depends on. A tick is a
write at the item's own line: through
OpenDocumentfor a.mdfile, where itlands as an unsaved edit like any other, and through the comment and description
edit paths for a pull request, from what the host holds rather than from what is
drawn.
The rest of the list with it: table alignment now reaches the cell, a fenced
block has a copy button, a picture opens at full size instead of being squinted
at under its 420pt cap, and a long document has an outline.
Scrolling
A pull request draws its comments in a
LazyVStack, so a comment scrolled outof sight is thrown away and built again on the way back — which is why the cost
of one comment shows up as a stutter rather than as load time. Measured, not
guessed:
Every paragraph was laid out twice.
CodeChipBackdropputs a second copy ofthe text behind the first so a
codespan can have a rounded fill, and it wentbehind every line in the document. Most prose has no span at all, and that half
of the work was never visible. Now it is attached only where there is a chip to
draw. This one predates the branch.
A name on every block. The outline needs a heading to answer to a name; it
was putting one on all of them, in every
MarkdownTexton the page — the samehandful of names over and over. An explicit
.idis structural identity, soSwiftUI tore views down and built them again as the indices moved.
A closure rebuilt every pass. A closure is never equal to another closure, so
an action written into the environment on each draw read as a change to
everything beneath it, and defeated the check that lets an unchanged comment be
skipped whole.
MarkdownTaskToggleis equal by what it writes to, and is notset at all for a comment nobody here may edit — which is most of them.
Hover on a fence rebuilt the fence. Scrolling drags the document under a
pointer that never moved, so every block it crosses reports a hover. The state
now sits in the overlay, where it rebuilds one button.
Plus: an item of one paragraph is drawn as the line it is rather than as a
nested
MarkdownText, and the caches are sized for a page rather than for aview — 2500 lines and 500 documents, against 600 and 200.
Also here
Three changes that were already in the tree and are not part of the above:
marksmanjoins the language server catalogue;EditorThemeBridgegains a noteon why a
.mdfile paints flat in the editor and what fixing it would cost; anda diff now folds away the files it deletes, which is the one file nobody reads.
Built and run. The parser was checked against the whole of
Docs/,README.mdand
TODO.md, and against the shapes a review bot writes — nested<details>/<blockquote>,<kbd>chips, HTML tables, badge links, Bitbucket's{: width=140 }.TODO.mdis rewritten in the same pass, and not only because those sixteenlines are done: its items were written
[ ] thingwith no-, which is not alist in Markdown at all. The old parser made it look like one; cmark reads it
the way GitHub does, as one paragraph. They are real list items now, so the file
renders as a checklist here and on the host, and its boxes tick.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BAgEj6aqwdPgmZEq5BapSu
Summary by CodeRabbit