From 7768f499e6f6d26f5977dfb7f9f0eca9d36eac09 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Thu, 30 Jul 2026 10:00:11 +0700 Subject: [PATCH 1/2] A document read as a document, and boxes you can tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **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 `
` 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 `

` a picture. A `` 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 `
`/`
`, `` 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) Claude-Session: https://claude.ai/code/session_01BAgEj6aqwdPgmZEq5BapSu --- CLAUDE.md | 47 +- Docs/Development.md | 11 +- Docs/Markdown.md | 142 +++ Package.resolved | 20 +- Package.swift | 14 +- README.md | 12 +- .../Workspace/Editor/EditorThemeBridge.swift | 18 + .../Models/LanguageServerCatalog.swift | 18 + Sources/Workspace/Support/MarkdownHTML.swift | 75 +- .../Workspace/Support/MarkdownHTMLText.swift | 375 ++++---- .../Workspace/Support/MarkdownInline.swift | 311 ++++++ .../Workspace/Support/MarkdownParser.swift | 612 ++++++++++++ Sources/Workspace/Support/RemoteImage.swift | 123 ++- Sources/Workspace/Views/DiffView.swift | 40 +- Sources/Workspace/Views/MarkdownPreview.swift | 891 ++++++++++-------- .../Views/PullRequestDetailView.swift | 45 + Sources/Workspace/Views/ViewerView.swift | 23 +- TODO.md | 240 ++--- 18 files changed, 2289 insertions(+), 728 deletions(-) create mode 100644 Docs/Markdown.md create mode 100644 Sources/Workspace/Support/MarkdownInline.swift create mode 100644 Sources/Workspace/Support/MarkdownParser.swift diff --git a/CLAUDE.md b/CLAUDE.md index fcaafe6..bc86461 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,21 +84,38 @@ The app is one window with three panes, and the mental model that drives the cod language server join. `TreeSitterHighlighter` survives for the diff and for markdown snippets, which live in no editor. Don't reach for an `NSTextView` API here: the package's `TextView` is not one. -- **Markdown** (`Views/MarkdownPreview.swift`) — a hand-rolled block renderer used - for `.md` files, PR descriptions and comments. A fenced block is coloured by - `Models/MarkdownCodeHighlighter.swift`, which runs the editor's - `TreeSitterHighlighter` over the snippet and takes its colours from the - current palette — the fence's language word replaces the file name the editor - detects from, and the result is cached per (language, palette, text). A - ` ```mermaid ` fence goes to `Views/MermaidDiagramView.swift`. - Both hosts render a comment as Markdown *with HTML in it*, and a bot leans on - that hard, so `Support/MarkdownHTMLText.swift` handles the tags a comment - actually uses: `` is dropped, `
`/`` and - `
` become blocks the parser recurses into, and every inline tag is - rewritten as the Markdown that says the same thing before the line is - classified. It is not an HTML parser — an unknown tag is dropped and its text - kept. `MarkdownText.Block` is `indirect` because of those containers, so a new - case has to be answered in `MarkdownHTML` too. +- **Markdown** — the parse and the renderer are separate on purpose, and only + the renderer is ours. `Support/MarkdownParser.swift` walks + [swift-markdown](https://github.com/swiftlang/swift-markdown)'s tree (cmark-gfm, + which is what both hosts render a comment with) into `MarkdownText.Block`, and + `Views/MarkdownPreview.swift` draws those blocks for `.md` files, PR + descriptions and comments alike. Read `Docs/Markdown.md` before changing any of + it: it records why the parser was replaced and why a library that renders too + was the wrong trade. + - The blocks carry **Markdown strings**, which the views read back with + `AttributedString(markdown:)` — so everything the parser writes into one goes + through `Support/MarkdownInline.swift`, which escapes plain text and is also + where the four things cmark does not answer live: a bare URL, `:tada:`, + `#123` and `@name`. That is one scan over `Text` nodes, not four passes. + - HTML arrives **raw and flat**: cmark splits `
` into three siblings — + the open tag, the inner Markdown parsed properly, the close tag — so + `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 + `
` is read only in its plain shape. + - Comments are dropped **from the tree, not from the text**, because every + node carries a 1-based source line and that is what a tickable checkbox + writes back to (`MarkdownTask.toggling`). Cutting lines out beforehand would + move every line number in the document. + - A fenced block is coloured by `Models/MarkdownCodeHighlighter.swift`, which + runs the editor's `TreeSitterHighlighter` over the snippet and takes its + colours from the current palette — the fence's language word replaces the + file name the editor detects from, and the result is cached per (language, + palette, text). A ` ```mermaid ` fence goes to `Views/MermaidDiagramView.swift`. + - `MarkdownText.Block` is `indirect` because a quote, a `
` section and + a list item all hold blocks, so a new case has to be answered in + `Support/MarkdownHTML.swift` too — that is the PDF, and it walks the same + blocks on purpose so the page and the file cannot drift. - **Diagrams** (`Views/DiagramWebView.swift`) — mermaid fences and `.drawio` files are both drawn by the real JavaScript library in a `WKWebView`, and both go through this one representable: it loads a page from `Resources/` as a diff --git a/Docs/Development.md b/Docs/Development.md index d00cb06..9f5de4e 100644 --- a/Docs/Development.md +++ b/Docs/Development.md @@ -97,6 +97,7 @@ Nothing is bundled — a server is used if it is on your `PATH`. | C / C++ / Objective-C | `clangd` | | Ruby, PHP, Dart, Lua, Kotlin | `ruby-lsp`, `intelephense`, `dart`, `lua-language-server`, `kotlin-lsp` (JetBrains') | | JSON, YAML, HTML, CSS, Bash | the matching `vscode-*-language-server` / `yaml-language-server` / `bash-language-server` | +| Markdown | `marksman` — link and heading completion, go-to-definition between documents, and a diagnostic for a link that points nowhere | That table is only the default list: **Settings → Language Servers** installs the missing ones, corrects a command, and adds a server for any language that is not @@ -109,8 +110,8 @@ your global npm packages or your gems, and that folder goes on the front of the repository wants one; **Settings → Language Servers** has the switch and a **Remove Downloaded** button. Only the npm-published servers are fetched this way — the ones that arrive with a toolchain (`sourcekit-lsp`, `clangd`, `dart`) -or as a platform binary (`rust-analyzer`, `kotlin-lsp`, `ruby-lsp`) keep their -Install button. +or as a platform binary (`rust-analyzer`, `kotlin-lsp`, `ruby-lsp`, `marksman`) +keep their Install button. Servers are **started while the dashboard is on screen**, not when a file is opened. Selecting a repository lists its files, works out which languages are @@ -163,9 +164,11 @@ Sources/Workspace/ Support/ViewerMotion.swift how long a screen takes to arrive, in one place Support/PullRequestReference.swift finding a `#123` written in a commit message Support/FileOperations.swift what the Files tab does to disk: create, copy, move, rename, trash + Support/MarkdownParser.swift cmark's block tree walked into the preview's blocks; ticking a box + Support/MarkdownInline.swift one scan over the text: escaping, bare URLs, :emoji:, #123, @name Support/MarkdownPDF.swift the preview as a PDF: measure, render, cut into sheets Support/MarkdownHTML.swift the same document as a printable page, diagrams and all - Support/MarkdownHTMLText.swift the HTML in a comment: `
`, `
`, `&`, hidden comments + Support/MarkdownHTMLText.swift the HTML in a comment: `
`, `
`, `
`, `&`, hidden comments Support/StreamingShellProcess.swift a command that stays up, read a line at a time Support/JSONValue.swift JSON of a shape only known at runtime (tool inputs) Support/RemoteImage.swift pictures Markdown points at: download once, draw, fall back @@ -241,7 +244,7 @@ Sources/Workspace/ ChatInputField.swift the growing text box the comment boxes are built on ChatCompletionList.swift the @ list, and reading the token under the caret MentionTextBox.swift a comment box where @ names a person on the pull request - MarkdownPreview.swift the Markdown renderer: blocks, tables, code chips + MarkdownPreview.swift the Markdown renderer: blocks, lists, tables, code chips, the outline — see Docs/Markdown.md DiagramWebView.swift the web view both diagram renderers are driven in MermaidDiagramView.swift a ```mermaid fence, drawn in the Markdown column DrawioPreview.swift a .drawio file, drawn in the whole pane diff --git a/Docs/Markdown.md b/Docs/Markdown.md new file mode 100644 index 0000000..9ea0601 --- /dev/null +++ b/Docs/Markdown.md @@ -0,0 +1,142 @@ +# Markdown: swift-markdown + +**Done.** This was written as a plan and is kept as the reasoning — every stage +at the bottom has landed. What follows is why the parser is what it is; the walk +itself is `Support/MarkdownParser.swift`. + +Markdown is rendered three times over in this app — a `.md` file in the viewer, a +pull request's description, and every comment on it — and all three go through one +type, `MarkdownText` in `Views/MarkdownPreview.swift`. Its parser *was* +hand-rolled and **line-by-line**: each source line was classified on its own +(`parse(_:depth:)`), which is where every remaining gap came from. A paragraph +hard-wrapped at 80 columns became one `.paragraph` 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; a fence inside a list item was hoisted out of +it. None of these were small bugs with small fixes — they were all the same +missing thing, which is a block tree. + +**The decision: replace the parser, keep the renderer.** +[swiftlang/swift-markdown](https://github.com/swiftlang/swift-markdown) is a +parser and nothing else — an immutable, thread-safe markup tree over +[swift-cmark](https://github.com/swiftlang/swift-cmark), the same cmark-gfm both +hosts use. So `MarkdownText.Block`, every view under it, `MarkdownPDF`, +`MarkdownCodeHighlighter`, the palettes, the mermaid and draw.io fences and the +mention chips all stay. Only `blocks(in:relativeTo:)` and `parse(_:depth:)` +change: a `MarkupWalker` builds the same blocks the views already draw. + +This was measured rather than assumed — every claim below was run against +swift-markdown 0.8.0 before being written down. + +## What the parser answers for free + +Each of these was a line in `TODO.md`, and each one is a node in the tree rather +than code of ours: + +| Today | What the tree gives | +| --- | --- | +| Hard-wrapped paragraphs come apart | **one** `Paragraph` holding `SoftBreak`s | +| A continuation line under a bullet becomes its own paragraph | it stays inside the `ListItem`'s paragraph | +| Nested checklists lose their indent | a nested `UnorderedList` *inside* the item — structure, not an `indent:` we carry | +| `1. [ ] thing` is not a checklist | `OrderedList` → `ListItem checkbox: [ ]` | +| Setext headings render as paragraph + rule | `Heading level: 1` | +| Rules only match `---` and `***` | `___`, `----` and `- - -` all give `ThematicBreak` | +| Indented code blocks are prose | `CodeBlock language: none` | +| Reference-style links stay literal | resolved to a `Link` with its destination; the `[ref]: …` line disappears | +| Table alignment is dropped | `columnAlignments: |l|c|r|` | +| A fence inside a list item is hoisted out | nested, as written | +| Checklists are not clickable | every node carries a **source range** (line:column) | +| No outline for long documents | `Heading` nodes, each with a range | + +The source ranges are the interesting one. A tickable checkbox has never been +about drawing a toggle — it is about knowing *which* `[ ]` in the file to flip, +and the tree says so exactly. For a `.md` file the write-back is `OpenDocument`; +for a pull request comment it is the edit path that already exists (see #19). + +## What it does not answer + +Worth writing down so it is not investigated twice. The package attaches exactly +three cmark extensions — `table`, `strikethrough`, `tasklist` — and no others: + +- **Bare URLs are still not links.** No autolink extension, so + `https://example.com` stays `Text`. Ours to do. +- **Footnotes are worse than missing.** `[^1]: note` is read as a *reference link + definition*, so `text[^1]` comes out as a link pointing at `note`. Adopting the + library does not fix footnotes, and this mangling must be caught rather than + passed through. +- **`:tada:`, `#123` and `@name`** are not Markdown at all. They are an inline + scan over `Text` nodes — and so is autolinking a bare URL, so that is one piece + of work rather than four. `CommitMessageText` already navigates `#123`. +- **HTML arrives raw.** `
`, `
` and `` come through as + `HTMLBlock` with the literal string in it, and `` as `InlineHTML`. So + `Support/MarkdownHTMLText.swift` stays exactly as load-bearing as it was. A + `
` *does* arrive whole in one `HTMLBlock`, which turned out to be enough + to read the plain shape — ``, `" }.joined() + case .table(let headers, let rows, let alignments): + let head = headers.enumerated() + .map { "\(inline($0.element))" } + .joined() let body = rows.map { row in - "" + row.map { "" }.joined() + "" + "" + row.enumerated() + .map { "\(inline($0.element))" } + .joined() + "" }.joined(separator: "\n") return """
`, `` — into the same headers and + rows a `|…|` table gives. Anything past that (a `colspan`, an `align=` on a + cell) wants [SwiftSoup](https://github.com/scinfu/SwiftSoup) rather than more + of the tag scanner. +- **A bare `- [ ]` is still not a box**, because cmark-gfm wants text after the + brackets. That stops being a bug worth fixing: both hosts *are* cmark-gfm, so + matching it is fidelity. The `TODO.md` line should be struck rather than done. +- Everything interactive or visual — the copy button on a fence, click-to-zoom on + an image, the checkbox's own control — is renderer work either way. The parser + only makes the checkbox's *effect* possible. + +## Why not the library that renders too + +[swift-markdown-ui](https://github.com/gonzalezreal/swift-markdown-ui) would draw +it as well, with themes, block styles and a code-highlighter hook. It is the +wrong trade here: raw HTML is not rendered at all, which is a straight regression +for the bot comments both hosts serve; its checkboxes are read-only; and mermaid, +draw.io, the palettes and the mention chips would all have to be re-plumbed +through its theming to arrive back where we already are. `Down` and `Ink` render +to `NSAttributedString` or HTML, meaning a `WKWebView` or the loss of the SwiftUI +blocks — the wrong direction for a column that hosts live diagram views. + +## The stages + +Each landed on its own and left the app working. + +1. ✅ **The tree, behind the same door.** The two packages are in `Package.swift`, + pinned exactly at 0.8.0. `MarkdownText.blocks(in:relativeTo:links:)` calls + `MarkdownParser`, and `parse(_:depth:)` is gone. This is where the whole first + table arrived at once. +2. ✅ **Lists became nested.** `Block` no longer carries `bullet(indent:)`, + `numbered(indent:)` and `task(done:)` as flat rows; there is one `.list` case + holding a `MarkdownList`, whose items hold blocks of their own. Answered in + `MarkdownListView`, and in `MarkdownHTML` — which writes the marker and the + item's column out by hand rather than as `
    `, so the page and the PDF + cannot drift. +3. ✅ **HTML against a flat stream.** `MarkdownHTMLText.events(in:)` reads one raw + block into `open`/`close`/`summary`/`table`/`markdown`, and `ContainerStack` + in the parser keeps the stack. Loose HTML — a README's + `

    ` — is handed back as Markdown and re-read as a + document of its own, which is what makes it a picture. +4. ✅ **One inline pass.** `MarkdownInline.decorated(_:links:inLink:)`, over `Text` + nodes only, doing the escaping in the same walk. The footnote hazard is caught + in both directions: a definition cmark left as a paragraph is lifted to the + end of the document, and one it ate as a link reference definition is drawn as + the marker it was rather than as a link to the note's words. +5. ✅ **Ticking a box.** `MarkdownList.Item.line` and `MarkdownTask.toggling`. For + a `.md` file the tick lands in `OpenDocument` as an unsaved edit — ⌘S writes + it, the way every other edit in the app is written. For a PR it goes back + through `updateDescription` / `updateComment`, from what the host holds rather + than from what is drawn. +6. ✅ **The rest of the list.** Table alignment, the outline behind a button in + the corner, the copy button on hover, and a picture that opens at full size. + +## What it cost, and what it did not + +- **The parse is cached exactly as it was.** `MarkdownText` is drawn on every pass + of a pull request view, so `cachedBlocks` and `cachedInlineText` both survived — + the `MarkdownLinks` of the document joined the key, since the same `#123` is + another repository's pull request. A tree costs 2–7 ms on this repository's own + documents, and it is built once per (text, folder, repository). +- **Comments are dropped from the tree, not from the text.** `strippingComments` + is gone. It cut whole lines out before parsing, which moved every line number + after them — and a source range is the one thing every write-back depends on. +- **The blocks still carry Markdown strings**, so the views, `MarkdownPDF`, + `MarkdownCodeHighlighter`, the palettes, the mermaid and draw.io fences all + stayed. The price is that everything the parser writes into a block has to be + escaped for the second read, which is why `MarkdownInline` owns both jobs. +- Both packages are ordinary SwiftPM dependencies with no asset catalogue and + nothing fetched at runtime, so the offline rule in `CLAUDE.md` holds. diff --git a/Package.resolved b/Package.resolved index c5dcaeb..88a57e9 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "238ea5e32729f9074cf8bf102b537b6f1abe8407e272f2c2cbe007282717556c", + "originHash" : "6af3bb23156d57dab34dbeae805bee5287cdf3aaaa96e7855f2a3b975cfc52f9", "pins" : [ { "identity" : "codeeditsourceeditor", @@ -55,6 +55,15 @@ "version" : "2.1.1" } }, + { + "identity" : "swift-cmark", + "kind" : "remoteSourceControl", + "location" : "https://github.com/swiftlang/swift-cmark.git", + "state" : { + "revision" : "924936d0427cb25a61169739a7660230bffa6ea6", + "version" : "0.8.0" + } + }, { "identity" : "swift-collections", "kind" : "remoteSourceControl", @@ -64,6 +73,15 @@ "version" : "1.6.0" } }, + { + "identity" : "swift-markdown", + "kind" : "remoteSourceControl", + "location" : "https://github.com/swiftlang/swift-markdown.git", + "state" : { + "revision" : "3c6f9523da3a1ec2fd829673e472d95b8097a3b8", + "version" : "0.8.0" + } + }, { "identity" : "swiftlintplugin", "kind" : "remoteSourceControl", diff --git a/Package.swift b/Package.swift index 7733b55..fcbb20b 100644 --- a/Package.swift +++ b/Package.swift @@ -41,6 +41,17 @@ let package = Package( // internal layout can be rearranged in a patch release and the compiler // is the only thing that would notice — and only if the names change. .package(url: "https://github.com/CodeEditApp/CodeEditTextView.git", exact: "0.12.1"), + // The Markdown parser, and only a parser: an immutable markup tree over + // swift-cmark — the same cmark-gfm both hosts render a comment with, so + // the preview reads a document the way the host that served it does. + // Nothing of the renderer comes from here; see Docs/Markdown.md for why + // a library that draws as well was the wrong trade. + .package(url: "https://github.com/swiftlang/swift-markdown.git", exact: "0.8.0"), + // The C parser under it. Named here rather than left transitive for the + // same reason CodeEditTextView is: the grammar of the document is what + // decides how every `.md` file, PR description and comment comes out, so + // the version that decides it should be one this manifest states. + .package(url: "https://github.com/swiftlang/swift-cmark.git", exact: "0.8.0"), // The terminal engine. Upstream ghostty ships no reusable framework, so // this package supplies libghostty as a prebuilt universal // (arm64 + x86_64) xcframework that SwiftPM downloads and @@ -56,7 +67,8 @@ let package = Package( .product(name: "GhosttyKit", package: "libghostty-spm"), .product(name: "CodeEditLanguages", package: "CodeEditLanguages"), .product(name: "CodeEditSourceEditor", package: "CodeEditSourceEditor"), - .product(name: "CodeEditTextView", package: "CodeEditTextView") + .product(name: "CodeEditTextView", package: "CodeEditTextView"), + .product(name: "Markdown", package: "swift-markdown") ], path: "Sources/Workspace", // The diagram renderers' HTML hosts with their bundled scripts, and diff --git a/README.md b/README.md index 38ee09b..d7700be 100644 --- a/README.md +++ b/README.md @@ -127,7 +127,11 @@ missing ones, so you never have to work out which command it wanted. | Feature | What it does | | --- | --- | -| Markdown | Rendered, not raw — tables, task lists, code chips, and the pictures a README keeps beside itself — and saved as PDF with ⇧⌘E. | +| Markdown | Rendered, not raw, by the same parser both hosts use — nested lists, tables with their alignment, quotes, footnotes, `:tada:`, code chips, and the pictures a README keeps beside itself. Saved as PDF with ⇧⌘E. | +| Tick a box | Checkboxes in a `.md` file, a PR description or a comment are real: click one and it is written back. | +| Jump around | An outline button on any document with headings worth jumping between. | +| Copy a block | A fenced block copies whole on hover, without selecting around it. | +| Open a picture | Click a screenshot to see it at full size instead of squinting at the capped one. | | Mermaid | ` ```mermaid ` fences drawn as diagrams. | | draw.io | `.drawio` files drawn as the diagram, with pages, zoom and layers. | | PDF | Read page by page, with a floating page and zoom bar. | @@ -202,6 +206,9 @@ Workspace is mostly other people's excellent work, joined together: SwiftPM by [Lakr233](https://github.com/Lakr233/libghostty-spm). - **[tree-sitter](https://github.com/tree-sitter/tree-sitter)** by Max Brunsfeld and every grammar author — the syntax colours everywhere in the app. +- **[swift-markdown](https://github.com/swiftlang/swift-markdown)** by Apple and + the Swift community — the Markdown parser, over cmark-gfm, which is what both + hosts render a comment with. - **[mermaid](https://github.com/mermaid-js/mermaid)** by Knut Sveidqvist and contributors — the diagrams in Markdown. - **[draw.io](https://github.com/jgraph/drawio)** by JGraph — the `.drawio` viewer. @@ -248,7 +255,8 @@ Scripts/make-icon.sh # rebuild Resources/AppIcon.icns from Assets/icon.png Background reading lives in [`Docs/`](Docs/) — [Development.md](Docs/Development.md) for how the project is built and released, [LSP.md](Docs/LSP.md) for the language -server layer, [Terminal.md](Docs/Terminal.md) for the terminal. +server layer, [Terminal.md](Docs/Terminal.md) for the terminal, and +[Markdown.md](Docs/Markdown.md) for how the Markdown renderer is put together. Found a bug or want something? [Open an issue](https://github.com/necessarylion/workspace/issues) — screenshots diff --git a/Sources/Workspace/Editor/EditorThemeBridge.swift b/Sources/Workspace/Editor/EditorThemeBridge.swift index a15f46d..cf68f9a 100644 --- a/Sources/Workspace/Editor/EditorThemeBridge.swift +++ b/Sources/Workspace/Editor/EditorThemeBridge.swift @@ -20,6 +20,24 @@ extension SyntaxTheme { /// and ``MarkdownCodeHighlighter`` keep colouring at full detail through /// ``TreeSitterHighlighter``. The editor is now the coarsest view of a file /// in the app, not the finest. + /// + /// **Markdown is the extreme of that**, and worth writing down so it is not + /// investigated twice: its grammar captures nothing *but* names on the + /// unrecognised list — `text.title`, `text.literal`, `text.uri`, + /// `text.reference`, `punctuation.special`, `punctuation.delimiter`, + /// `string.escape` — so every range is dropped and a `.md` file in the editor + /// paints in one flat colour. It is not broken, it is this list; the preview + /// (⌘-toggled, and the way a `.md` file is meant to be read here) colours the + /// same file properly because it goes through ``TreeSitterHighlighter``. + /// + /// Fixing it would mean rewriting the vendored markdown queries onto the + /// names the package knows, which loses the bold heading and the italic + /// emphasis — no slot carries them — or giving the editor a + /// `highlightProviders` of our own. There is also a second break under it, + /// so neither is a one-line change: the vendored injection query names the + /// inline grammar `markdown_inline`, `TreeSitterLanguage`'s raw value is + /// `markdownInline`, and the layer is dropped for want of the match, so + /// `**bold**` and `*italic*` are not even parsed today. var editorTheme: EditorTheme { EditorTheme( text: attribute(for: nil), diff --git a/Sources/Workspace/Models/LanguageServerCatalog.swift b/Sources/Workspace/Models/LanguageServerCatalog.swift index 8fe0b13..2b24b67 100644 --- a/Sources/Workspace/Models/LanguageServerCatalog.swift +++ b/Sources/Workspace/Models/LanguageServerCatalog.swift @@ -446,6 +446,24 @@ final class LanguageServerCatalog { languageID: "css", installCommand: "npm install -g vscode-langservers-extracted" ), + .init( + // The one markdown server worth starting in an editor: it answers + // about the *document*, not the prose. Heading and link completion, + // go-to-definition across `[](other.md)` and `[[wiki]]` links, + // references on a heading, and a diagnostic for a link that points + // nowhere — which is the thing a docs folder actually gets wrong. + // + // A single self-contained binary with no runtime behind it, so it + // starts in the time gopls does; the grammar checkers that also call + // themselves markdown servers (`ltex-ls`, `harper-ls`) do a + // different job and the JVM one is far slower than anything else + // here. + language: TreeSitterLanguage.markdown.rawValue, + executable: "marksman", + command: "marksman server", + languageID: "markdown", + installCommand: "brew install marksman" + ), .init( language: TreeSitterLanguage.bash.rawValue, executable: "bash-language-server", diff --git a/Sources/Workspace/Support/MarkdownHTML.swift b/Sources/Workspace/Support/MarkdownHTML.swift index db217b1..7388b4d 100644 --- a/Sources/Workspace/Support/MarkdownHTML.swift +++ b/Sources/Workspace/Support/MarkdownHTML.swift @@ -42,12 +42,8 @@ enum MarkdownHTML { return "\(inline(text))" case .paragraph(let text): return "

    \(inline(text))

    " - case .bullet(let indent, let text): - return item(marker: indent > 0 ? "◦" : "•", text: inline(text), indent: indent) - case .numbered(let indent, let number, let text): - return item(marker: "\(escape(number)).", text: inline(text), indent: indent) - case .task(let done, let text): - return item(marker: done ? "☑" : "☐", text: inline(text), indent: 0) + case .list(let list): + return html(for: list, depth: 0) case .quote(let alert, let blocks): let head = alert.map { "

    \(escape($0.title))

    " } ?? "" return "
    \(head)\(blocks.map(html(for:)).joined(separator: "\n"))
    " @@ -81,10 +77,14 @@ enum MarkdownHTML { // stylesheet's `max-width: 100%` still keeps it inside the sheet. let style = width.map { " style=\"width: \(Int($0))px\"" } ?? "" return "

    \"\(escape(alt))\"

    " - case .table(let headers, let rows): - let head = headers.map { "
\(inline($0))
\(inline($0))
@@ -94,10 +94,43 @@ enum MarkdownHTML { """ case .rule: return "
" - case .spacer: - // Blank lines are what separates the blocks, and the spacing is in - // the stylesheet. - return "" + } + } + + /// One list, drawn the way the preview draws it: the marker in a column of + /// its own, and everything nested under an item indented by that column. + /// + /// Written out by hand rather than as `
` read whole. Cells hold Markdown. + case table(headers: [String], rows: [[String]]) + /// Everything that was not one of the above, rewritten as the Markdown + /// that says the same thing — for the parser to read as a document of + /// its own. This is what turns a README's + /// `

` into a picture. + case markdown(String) } - /// The first container in `text` that is not nested inside another one. + /// Reads one raw HTML block into the events above. /// - /// Only tags of the same name are counted while looking for the close, so a - /// `
` inside a `
` — the shape every CodeRabbit review - /// has — does not end the section early; the recursion picks it up from the - /// inner text. A container that is never closed takes the rest of the text, - /// which is the reading a browser gives it too. - static func container(in text: String) -> Container? { - guard text.contains("<") else { return nil } - let fenced = fences(in: text) - var index = text.startIndex - var open: HTMLTag? - var innerStart = text.startIndex - var depth = 0 + /// A comment goes nowhere: `` is dropped here rather than cut out + /// of the source beforehand, which is what keeps every source range in the + /// document pointing at the line it was written on — and a tickable + /// checkbox is a source range. + static func events(in rawHTML: String) -> [Event] { + var events: [Event] = [] + var loose = "" + var index = rawHTML.startIndex - while index < text.endIndex { - if let fence = fenced.first(where: { $0.contains(index) }) { - index = fence.upperBound + /// Whatever plain HTML has piled up since the last structural tag. + func flush() { + defer { loose = "" } + let text = markdown(from: loose) + guard !text.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return } + events.append(.markdown(text)) + } + + while index < rawHTML.endIndex { + // A comment, which the reader was never meant to see. + if rawHTML[index] == "<", rawHTML[index...].hasPrefix("", range: after..` out of a `
`, handing back its text and the - /// rest of the section. The two are rarely on lines of their own — - /// `…
` on one line is the usual shape — so - /// what is left either side is stitched back together. - static func summary(in text: Substring) -> (summary: String, body: String)? { - var index = text.startIndex - var opening: HTMLTag? + /// The text between an opening tag and the one that closes it, counting + /// tags of the same name so a nested one does not end it early, plus the + /// index just past the close. A tag that is never closed takes the rest, + /// which is the reading a browser gives it too. + private static func contents( + of name: String, + after opening: HTMLTag, + in text: String + ) -> (inner: String, end: String.Index) { + var index = opening.range.upperBound + var depth = 1 while index < text.endIndex { - guard text[index] == "<", let tag = tag(at: index, in: text) else { + guard text[index] == "<", let found = tag(at: index, in: text[...]) else { index = text.index(after: index) continue } - index = tag.range.upperBound - // A section of its own starts before this one ever said what it - // was: the summary further down belongs to that one, not to us. - if opening == nil, !tag.isClosing, tag.name == "details" { return nil } - guard tag.name == "summary" else { continue } - if let start = opening, tag.isClosing { - return ( - summary: String(text[start.range.upperBound..` read into the same headers and rows a `|…|` table gives. + /// + /// The header is whichever row is written with `
`; a table with none is + /// read with its first row as the header, because a table drawn with no + /// header band at all is not a shape the preview has. + private static func table(in html: String) -> (headers: [String], rows: [[String]])? { + var rows: [[String]] = [] + var headerRow: Int? + var index = html.startIndex + + while index < html.endIndex { + guard html[index] == "<", let found = tag(at: index, in: html[...]) else { + index = html.index(after: index) + continue + } + switch found.name { + case "tr" where !found.isClosing: + rows.append([]) + index = found.range.upperBound + case "th", "td": + guard !found.isClosing else { + index = found.range.upperBound + continue + } + if rows.isEmpty { rows.append([]) } + if found.name == "th", headerRow == nil { headerRow = rows.count - 1 } + let (inner, end) = contents(of: found.name, after: found, in: html) + rows[rows.count - 1].append( + markdown(from: inner).trimmingCharacters(in: .whitespacesAndNewlines) ) + index = end + default: + index = found.range.upperBound } - if !tag.isClosing { opening = tag } } - return nil + + rows.removeAll { $0.isEmpty } + guard !rows.isEmpty else { return nil } + let headers = rows.remove(at: headerRow ?? 0) + let width = max(headers.count, rows.map(\.count).max() ?? 0) + return ( + headers: headers + Array(repeating: "", count: width - headers.count), + rows: rows.map { $0 + Array(repeating: "", count: width - $0.count) } + ) } // MARK: - Inline - /// One line of HTML rewritten as the Markdown that says the same thing, so - /// the inline parser downstream needs to know nothing about tags. A tag with - /// no Markdown spelling — `
`, ``, a stray `

` — is dropped and - /// its text kept. + /// What one `InlineHTML` node — a tag in the middle of a sentence — turns + /// into. + enum Inline { + /// The Markdown that says the same thing, `**` for `` and so on. + case markdown(String) + /// An ``, which is a block of its own in the preview rather than + /// something a line of text can hold. + case image(source: String, alt: String, width: CGFloat?) + /// A tag with no meaning here: dropped, its text kept. + case nothing + } + + /// One inline tag, read. `links` carries the addresses of the `` tags + /// still open, so the closing tag writes the address of the one it belongs + /// to; the caller keeps it across a paragraph's nodes. + static func inline(_ rawHTML: String, links: inout [String]) -> Inline { + // Bookkeeping a bot hides mid-sentence. + if rawHTML.hasPrefix("` goes first, so nothing - // downstream has to know it was ever there. - let parsed = parse(MarkdownHTMLText.strippingComments(text), depth: 0) + static func blocks(in text: String, relativeTo base: URL? = nil, links: MarkdownLinks = .none) -> [Block] { + let parsed = MarkdownParser.blocks(in: text, links: links) guard let base else { return parsed } return resolvingImages(parsed, relativeTo: base) } + /// The headings of a document, for the outline: the level, the words with + /// their styling taken off, and the index of the block they came from — + /// which is what ``headingID(_:)`` names and the scroll view scrolls to. + @MainActor + static func outline(of blocks: [Block]) -> [(index: Int, level: Int, title: String)] { + blocks.enumerated().compactMap { index, block in + guard case .heading(let level, let text) = block else { return nil } + let plain = (try? AttributedString( + markdown: text, + options: .init(interpretedSyntax: .inlineOnlyPreservingWhitespace) + )).map { String($0.characters) } ?? text + let title = plain.trimmingCharacters(in: .whitespaces) + return title.isEmpty ? nil : (index, level, title) + } + } + /// Turns `![](assets/preview.jpeg)` into the file it means. /// /// A README is written to be read beside its own folder, and the pictures in @@ -565,7 +742,8 @@ struct MarkdownText: View { case .image(let address, let alt, let width): guard let file = localFile(for: address, relativeTo: base) else { return block } return .image(url: file.absoluteString, alt: alt, width: width) - // A picture inside a quote or a folded section is still a picture. + // A picture inside a quote, a folded section or a list item is + // still a picture. case .quote(let alert, let nested): return .quote(alert, resolvingImages(nested, relativeTo: base)) case .disclosure(let summary, let isOpen, let nested): @@ -574,6 +752,13 @@ struct MarkdownText: View { isOpen: isOpen, blocks: resolvingImages(nested, relativeTo: base) ) + case .list(var list): + list.items = list.items.map { item in + var resolved = item + resolved.blocks = resolvingImages(item.blocks, relativeTo: base) + return resolved + } + return .list(list) default: return block } @@ -633,323 +818,283 @@ struct MarkdownText: View { switch block { case .mermaid: true case .quote(_, let nested), .disclosure(_, _, let nested): containsDiagram(nested) + case .list(let list): list.items.contains { containsDiagram($0.blocks) } default: false } } } +} - /// Peels off the `

` and `
` sections, outermost first, - /// and hands what is between them to the line parser. A section's own - /// contents come back through here, which is what lets a review nested four - /// deep read as four foldable sections rather than as its tags. - /// - /// `depth` is only a backstop against text that nests without end; real - /// output from a bot bottoms out around five. - private static func parse(_ text: String, depth: Int) -> [Block] { - guard depth < 12, let container = MarkdownHTMLText.container(in: text) else { - return tidied(plainBlocks(in: text, depth: depth)) - } - var result = plainBlocks(in: String(container.before), depth: depth) - - switch container.tag { - case .details: - let split = MarkdownHTMLText.summary(in: container.inner) - let summary = MarkdownHTMLText.markdown(from: split?.summary ?? "") - .trimmingCharacters(in: .whitespacesAndNewlines) - result.append(.disclosure( - // A section with no `` still needs something to click. - summary: summary.isEmpty ? "Details" : summary, - isOpen: container.isOpen, - blocks: parse(split?.body ?? String(container.inner), depth: depth + 1) - )) - case .blockquote: - result.append(.quote(nil, parse(String(container.inner), depth: depth + 1))) - } +// MARK: - Environment + +/// How deep in a list the text being drawn sits. It travels in the environment +/// rather than as a parameter because a sub-list is drawn by a nested +/// `MarkdownText` that was handed blocks, not a depth — and the same is true of +/// a list inside a quote or a `
` section. +private struct MarkdownListDepthKey: EnvironmentKey { + static let defaultValue = 0 +} + +/// Where a `#123` and an `@name` in the Markdown being drawn point. Nothing by +/// default: a release note has no repository behind it, and a `#123` that +/// cannot be resolved is better left as the words it was written as. +private struct MarkdownLinksKey: EnvironmentKey { + static let defaultValue = MarkdownLinks.none +} + +/// What ticking a checkbox does, and what it writes to. +/// +/// `perform` takes the 1-based source line the item was written on and the state +/// it should end up in. `target` is what those lines belong to — a file's path, +/// a comment's id — and it is the whole of this type's equality. +/// +/// That equality is the point of the type existing. A closure is never equal to +/// another closure, so an action rebuilt on each pass reads as a change to +/// everything under it: a page of pull request comments would rebuild all of its +/// text on any redraw at all, which is exactly how it came to stutter under a +/// scroll. Two actions writing to the same place are the same action, whatever +/// closure was made this time round. +struct MarkdownTaskToggle: Equatable { + let target: String + let perform: (Int, Bool) -> Void + + static func == (lhs: Self, rhs: Self) -> Bool { lhs.target == rhs.target } +} - result += parse(String(container.after), depth: depth) - return tidied(result) +/// Nothing by default: a release note and a `
` section from a bot have +/// checkboxes nobody can write back to, and a box that cannot be ticked should +/// not look as though it can. +struct MarkdownTaskToggleKey: EnvironmentKey { + // `nil` is as shared-mutable as this gets, but the type it is nil *of* + // holds a closure, and a closure is not `Sendable`. The environment is read + // on the main actor and nowhere else. + nonisolated(unsafe) static let defaultValue: MarkdownTaskToggle? = nil +} + +extension EnvironmentValues { + var markdownListDepth: Int { + get { self[MarkdownListDepthKey.self] } + set { self[MarkdownListDepthKey.self] = newValue } } - /// A run of blank lines is one paragraph break, and the blank lines a - /// section opens and closes with are nothing at all — the stack's own - /// spacing is what holds the blocks apart. - /// - /// This matters here in a way it does not in a hand-written document: HTML - /// needs a blank line either side of a tag for the Markdown inside it to be - /// read as Markdown, so a bot writing `
` sections leaves two or - /// three, and each one used to push the next section further down. - private static func tidied(_ blocks: [Block]) -> [Block] { - var result: [Block] = [] - for block in blocks { - guard case .spacer = block else { - result.append(block) - continue - } - if let last = result.last, case .spacer = last { continue } - if result.isEmpty { continue } - result.append(block) - } - if let last = result.last, case .spacer = last { result.removeLast() } - return result + var markdownLinks: MarkdownLinks { + get { self[MarkdownLinksKey.self] } + set { self[MarkdownLinksKey.self] = newValue } } - private static func plainBlocks(in text: String, depth: Int) -> [Block] { - var result: [Block] = [] - var codeBuffer: [String] = [] - var inCode = false - /// The word after the opening ``` — "swift", "mermaid", or nothing. - var codeLanguage = "" - var tableBuffer: [[String]] = [] - var quoteBuffer: [String] = [] - - /// A quote is parsed as a document of its own rather than line by line: - /// it can hold anything, a `
` section included, and a bot's - /// alert is several paragraphs behind one bar. - func flushQuote() { - guard !quoteBuffer.isEmpty else { return } - var lines = quoteBuffer - quoteBuffer.removeAll() - var alert: Alert? - if let first = lines.firstIndex(where: { !$0.trimmingCharacters(in: .whitespaces).isEmpty }), - let marked = Alert(marker: lines[first].trimmingCharacters(in: .whitespaces)) { - alert = marked - lines.remove(at: first) - } - result.append(.quote(alert, parse(lines.joined(separator: "\n"), depth: depth + 1))) - } + var markdownTaskToggle: MarkdownTaskToggle? { + get { self[MarkdownTaskToggleKey.self] } + set { self[MarkdownTaskToggleKey.self] = newValue } + } +} - /// A fence is a diagram when it says so and has something in it; - /// everything else stays a code block. - func flushCode() { - let body = codeBuffer.joined(separator: "\n") - let language = codeLanguage - codeBuffer.removeAll() - codeLanguage = "" - let hasContent = !body.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty - if language.lowercased() == "mermaid", hasContent { - result.append(.mermaid(body)) - } else { - result.append(.code(language: language, text: body)) - } - } +/// Names a block so the outline can scroll to it, and leaves it alone otherwise. +private struct HeadingAnchor: ViewModifier { + let id: String? - func flushTable() { - guard !tableBuffer.isEmpty else { return } - defer { tableBuffer.removeAll() } - // A lone |…| line is not a table, just a paragraph with pipes. - guard tableBuffer.count >= 2 else { - result.append(.paragraph(tableBuffer[0].joined(separator: " | "))) - return - } - let headers = tableBuffer[0] - var rows = Array(tableBuffer.dropFirst()) - // Drop the |---|---| separator row if present. - if let first = rows.first, first.allSatisfy({ cell in - !cell.isEmpty && cell.allSatisfy { "-: ".contains($0) } - }) { - rows.removeFirst() - } - // Pad short rows so every GridRow has the same number of cells. - rows = rows.map { row in - row + Array(repeating: "", count: max(0, headers.count - row.count)) - } - result.append(.table(headers: headers, rows: rows)) + func body(content: Content) -> some View { + if let id { + content.id(id) + } else { + content } + } +} - for rawLine in text.split(separator: "\n", omittingEmptySubsequences: false) { - // Whatever HTML the line still carries becomes the Markdown that - // says the same thing, so everything below reads one syntax. - var line = inCode ? String(rawLine) : MarkdownHTMLText.markdown(from: String(rawLine)) - var trimmed = line.trimmingCharacters(in: .whitespaces) - - // The quote comes off before the pictures are pulled out: its lines - // are re-parsed as a document, and one that went through here first - // would have its pictures hoisted out of the quote and left after it. - if !inCode, trimmed.hasPrefix(">") { - flushTable() - var rest = Substring(trimmed).dropFirst() - if rest.hasPrefix(" ") { rest = rest.dropFirst() } - quoteBuffer.append(String(rest)) - continue - } - flushQuote() - - // Pictures come out of the line before anything else looks at it, so - // an image sitting on a bullet or at the end of a sentence still - // becomes a picture and the words around it still read as words. - // `AttributedString` would otherwise swallow the whole `![…](…)` - // and draw nothing at all. - var images: [Picture] = [] - if !inCode, line.contains("![") { - let split = splitImages(from: line) - line = split.text - trimmed = line.trimmingCharacters(in: .whitespaces) - images = split.images - } - defer { - for image in images { - result.append(.image(url: image.url, alt: image.alt, width: image.width)) - } - } - // A line that was nothing but pictures leaves no text behind, and an - // empty line here would only add a gap above them. - let isImageOnly = !images.isEmpty && trimmed.isEmpty +// MARK: - Lists - if !inCode, trimmed.hasPrefix("|"), trimmed.dropFirst().contains("|") { - tableBuffer.append(tableCells(trimmed)) - continue - } - flushTable() +/// One list, drawn as its items — each of which is a document of its own. +/// +/// The nesting is real: a sub-list, a second paragraph and a fenced block under +/// a bullet are blocks of the item, so they are drawn by a nested `MarkdownText` +/// inside the item's own column. That is what lines a wrapped line up under the +/// text rather than under the bullet, and what indents a sub-list by exactly the +/// width of its parent's marker. +private struct MarkdownListView: View { + let list: MarkdownList + /// Read here rather than in `MarkdownText`, so that every paragraph in the + /// document does not take a dependency on two values only a list uses. + @Environment(\.markdownListDepth) private var depth + @Environment(\.markdownTaskToggle) private var onToggle - if trimmed.hasPrefix("```") { - if inCode { - flushCode() - } else { - codeLanguage = String(trimmed.dropFirst(3)).trimmingCharacters(in: .whitespaces) + var body: some View { + VStack(alignment: .leading, spacing: 6) { + ForEach(Array(list.items.enumerated()), id: \.offset) { index, item in + HStack(alignment: .firstTextBaseline, spacing: 8) { + marker(for: item, at: index) + content(of: item) + // No strikethrough on a ticked item: the box already + // says it is done, and struck-through text is the + // harder to read the more there is of it. + .foregroundStyle(item.isDone == true ? .secondary : .primary) } - inCode.toggle() - continue - } - - if inCode { - codeBuffer.append(line) - continue + .environment(\.markdownListDepth, depth + 1) } + } + } - // Two spaces (or one tab) of leading indentation = one list level. - let leading = line.prefix { $0 == " " || $0 == "\t" } - let indent = leading.reduce(0) { $0 + ($1 == "\t" ? 2 : 1) } / 2 - - if isImageOnly { - // The pictures are all this line had; `defer` appends them. - } else if trimmed.isEmpty { - result.append(.spacer) - } else if trimmed == "---" || trimmed == "***" { - result.append(.rule) - } else if trimmed.hasPrefix("#") { - let level = trimmed.prefix(while: { $0 == "#" }).count - let title = trimmed.dropFirst(level).trimmingCharacters(in: .whitespaces) - result.append(.heading(level: min(level, 6), text: title)) - } else if trimmed.hasPrefix("- ") || trimmed.hasPrefix("* ") || trimmed.hasPrefix("+ ") { - let rest = String(trimmed.dropFirst(2)) - // Trimmed: a bot writes its checkbox id in a comment between the - // box and the label, and taking the comment out leaves the gap. - if rest.hasPrefix("[ ] ") { - result.append(.task(done: false, text: rest.dropFirst(4).trimmingCharacters(in: .whitespaces))) - } else if rest.hasPrefix("[x] ") || rest.hasPrefix("[X] ") { - result.append(.task(done: true, text: rest.dropFirst(4).trimmingCharacters(in: .whitespaces))) - } else { - result.append(.bullet(indent: indent, text: rest)) - } - } else if let ordered = orderedItem(trimmed) { - result.append(.numbered(indent: indent, number: ordered.number, text: ordered.text)) - } else { - result.append(.paragraph(trimmed)) - } + /// Nearly every item is one paragraph, and that one is drawn as the line it + /// is. The nested `MarkdownText` is what carries a sub-list, a second + /// paragraph or a fence — and it is a whole stack, a `ForEach` and a round + /// of environment reads, so a long checklist is worth not paying it for. + @ViewBuilder + private func content(of item: MarkdownList.Item) -> some View { + if item.blocks.count == 1, case .paragraph(let text) = item.blocks[0] { + MarkdownText.inline(text) + } else { + MarkdownText(blocks: item.blocks) } + } - flushQuote() - flushTable() - // An unterminated fence — the file is still being typed, most likely. - if inCode, !codeBuffer.isEmpty { - flushCode() + @ViewBuilder + private func marker(for item: MarkdownList.Item, at index: Int) -> some View { + if let isDone = item.isDone { + MarkdownCheckbox( + isDone: isDone, + // A box with no source line behind it — one that came out of a + // bot's HTML — is drawn but not offered. + onToggle: item.line.flatMap { line in + onToggle.map { toggle in { toggle.perform(line, !isDone) } } + } + ) + } else if list.isOrdered { + Text("\(list.start + index).") + .foregroundStyle(.secondary) + .monospacedDigit() + } else { + Text(Self.bullet(at: depth)) + .foregroundStyle(.secondary) } - return result } - /// One picture pulled out of a line. - private struct Picture { - var url: String - var alt: String - var width: CGFloat? + /// A different mark at each level, the way a printed list distinguishes + /// them — the indent alone is easy to lose in a long item. + private static func bullet(at depth: Int) -> String { + switch depth { + case 0: "•" + case 1: "◦" + default: "▪" + } } +} - /// Pulls every `![alt](url)` out of one line, handing back what is left of - /// the line and the pictures in the order they appeared. - /// - /// Bitbucket writes an attribute list after the image — - /// `![](…png){: data-layout='center' }` — which is not Markdown any parser - /// here knows; it is swallowed along with the image rather than left behind - /// as stray braces in the middle of a sentence. `MarkdownHTMLText` writes - /// the same list to carry the one attribute that *is* read: the width an - /// `` asked for. - private static func splitImages(from line: String) -> (text: String, images: [Picture]) { - var text = "" - var images: [Picture] = [] - var index = line.startIndex - - while index < line.endIndex { - guard line[index] == "!", - let bracket = line.index(index, offsetBy: 1, limitedBy: line.endIndex), - bracket < line.endIndex, line[bracket] == "[", - let altEnd = line[bracket...].firstIndex(of: "]"), - let open = line.index(altEnd, offsetBy: 1, limitedBy: line.endIndex), - open < line.endIndex, line[open] == "(", - // A closing paren inside the address would end it early, but - // an address with one in it is rare enough not to trade the - // simplicity for. - let close = line[open...].firstIndex(of: ")") - else { - text.append(line[index]) - index = line.index(after: index) - continue - } +/// The box itself. A button when there is somewhere to write the answer, and +/// otherwise the picture of one. +private struct MarkdownCheckbox: View { + let isDone: Bool + let onToggle: (() -> Void)? - let alt = String(line[line.index(after: bracket).. CGFloat? { - guard let key = attributes.range(of: "width=") else { return nil } - let digits = attributes[key.upperBound...].prefix { $0.isNumber } - guard let number = Int(digits), number > 0 else { return nil } - return CGFloat(number) +// MARK: - Code + +/// A fenced block, with the one thing people do to a fenced block on it. +/// +/// Everything in the preview is selectable, but a code block is the one thing +/// that is copied *whole* — and selecting exactly it, with no line of the prose +/// either side, is fiddly in a wall of text. +private struct MarkdownCodeBlock: View { + let code: String + let text: Text + + var body: some View { + ScrollView(.horizontal) { + // Coloured by the editor's own tree-sitter setup when the fence + // names a language we have a grammar for, plain when it does not. + text + .font(.system(.callout, design: .monospaced)) + .frame(maxWidth: .infinity, alignment: .leading) + .padding(10) + // Room for the button, so the last words of a long first line + // do not run under it. + .padding(.trailing, 28) + } + .background(.quaternary.opacity(0.4), in: RoundedRectangle(cornerRadius: 6)) + // The fill alone is faint against a dark viewer; the outline is + // what actually marks where the block starts and stops. + .overlay(RoundedRectangle(cornerRadius: 6).stroke(.quaternary, lineWidth: 1)) + // The hover lives in the overlay, not here. Scrolling drags the whole + // document under a pointer that never moved, so every block the pointer + // crosses reports a hover — and with the state on this view, each of + // those rebuilt the fence and its coloured text. In there it rebuilds + // one button. + .overlay { MarkdownCopyButton(code: code) } } +} + +/// The copy button, and the hover that reveals it. +/// +/// Everything in the preview is selectable, but a code block is the one thing +/// that is copied *whole* — and selecting exactly it, with no line of the prose +/// either side, is fiddly in a wall of text. +private struct MarkdownCopyButton: View { + let code: String - /// "| a | b |" → ["a", "b"]. - private static func tableCells(_ line: String) -> [String] { - var inner = Substring(line) - if inner.hasPrefix("|") { inner = inner.dropFirst() } - if inner.hasSuffix("|") { inner = inner.dropLast() } - return inner - .split(separator: "|", omittingEmptySubsequences: false) - .map { $0.trimmingCharacters(in: .whitespaces) } + @State private var isHovered = false + @State private var hasCopied = false + + var body: some View { + // Shown on hover only: a page of fenced examples otherwise wears a row + // of buttons nobody asked for. It stays up while it says "Copied", so + // the word is readable after the pointer has moved on. + button + .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topTrailing) + .contentShape(Rectangle()) + // Nothing here takes a click except the button, so a drag that + // starts on the code still selects it. + .allowsHitTesting(isHovered || hasCopied) + .onHover { isHovered = $0 } } - /// "3. text" or "3) text" → ("3", "text"); nil when not an ordered item. - private static func orderedItem(_ line: String) -> (number: String, text: String)? { - let digits = line.prefix(while: \.isNumber) - guard !digits.isEmpty, digits.count <= 4 else { return nil } - let rest = line.dropFirst(digits.count) - guard rest.hasPrefix(". ") || rest.hasPrefix(") ") else { return nil } - return (String(digits), rest.dropFirst(2).trimmingCharacters(in: .whitespaces)) + @ViewBuilder + private var button: some View { + if isHovered || hasCopied { + Button { + NSPasteboard.general.clearContents() + NSPasteboard.general.setString(code, forType: .string) + hasCopied = true + Task { + try? await Task.sleep(for: .seconds(1.5)) + hasCopied = false + } + } label: { + Image(systemName: hasCopied ? "checkmark" : "doc.on.doc") + .font(.caption) + .foregroundStyle(hasCopied ? Color.green : Color.secondary) + .padding(5) + .background(.background.opacity(0.8), in: RoundedRectangle(cornerRadius: 5)) + } + .buttonStyle(.plain) + .pointerCursor() + .help(hasCopied ? "Copied" : "Copy this block") + .padding(5) + .transition(.opacity) + } } } diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 28ea1e5..ce171c9 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -95,6 +95,10 @@ struct PullRequestDetailView: View { // transaction is the one that ended the pull request, so a build tick, // a keystroke or a comment landing still redraws with nothing animated. .animation(ViewerMotion.contentChange, value: isOpen) + // Set once for the whole page: the description, every comment and every + // reply nested under one are all this repository's Markdown, so a + // `#123` in any of them is this repository's pull request. + .environment(\.markdownLinks, MarkdownLinks(remote: project.remote)) // A half-written description belongs to the pull request it was opened // on, and this one view is reused as the viewer moves between them. .onChange(of: item.id) { @@ -682,6 +686,10 @@ struct PullRequestDetailView: View { .transition(ViewerMotion.contentArrival) } else if !pr.body.isEmpty { MarkdownText(text: pr.body) + .environment( + \.markdownTaskToggle, + MarkdownTaskToggle(target: "pr-\(pr.number)-description", perform: toggleDescriptionTask) + ) .font(.callout) .transition(ViewerMotion.contentArrival) } @@ -729,6 +737,19 @@ struct PullRequestDetailView: View { if saved { descriptionDraft = nil } } + /// Ticking a box in the description. There is no half of a description to + /// save, so the whole of it is written back — from what the host holds + /// rather than from what is drawn, for the same reason the edit box opens + /// on that: replacing a Bitbucket mention with the name beside it would + /// post the name as words and lose the mention. + private func toggleDescriptionTask(line: Int, isDone: Bool) { + Task { + let source = await store.editableDescription(project: project, pr: pr) + guard let updated = MarkdownTask.toggling(line: line, to: isDone, in: source) else { return } + _ = await store.updateDescription(updated, on: item, project: project, pr: pr) + } + } + private var diffTab: some View { // The spinner and the diff share the pane, so the one gives way to the // other rather than being cut to it. Keyed to `isLoading` alone: a diff @@ -2355,6 +2376,26 @@ struct CommentBubble: View { /// Sends the rewritten text. The box is drawn only when this is given. var onEditSubmitted: ((String) async -> Void)? + /// Ticking a box in a comment: the flip is made on the Markdown the *host* + /// holds — which on Bitbucket is not quite what is on screen, see + /// ``PullRequestComment/rawBody`` — and the whole comment is posted back. + /// The line numbers agree because a mention put back in its account-id form + /// is a replacement within a line, not a line of its own. + /// + /// Nothing at all for a comment nobody here may edit, which is most of them: + /// a bot's review, and everyone else's words. + private var taskToggle: MarkdownTaskToggle? { + guard let onEditSubmitted else { return nil } + return MarkdownTaskToggle(target: "comment-\(comment.id)") { line, isDone in + guard let updated = MarkdownTask.toggling( + line: line, + to: isDone, + in: comment.editableBody + ) else { return } + Task { await onEditSubmitted(updated) } + } + } + var body: some View { VStack(alignment: .leading, spacing: 5) { HStack(spacing: 6) { @@ -2416,6 +2457,10 @@ struct CommentBubble: View { .foregroundStyle(.tertiary) } else { MarkdownText(text: comment.body) + // A checklist in a comment is only tickable by whoever + // could edit the comment anyway — the tick *is* an edit, + // and it goes back through the same path the box does. + .environment(\.markdownTaskToggle, taskToggle) .font(.callout) } diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index fff2cbc..c3b5cf7 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -376,8 +376,27 @@ struct ViewerView: View { case .text: if document.isMarkdown && store.markdownPreview { // With the file's own address: a README's pictures are - // paths beside it, and this is what they are beside. - MarkdownPreview(text: document.text, baseURL: document.url) + // paths beside it, and this is what they are beside. And + // with the repository it belongs to, so a `#123` in it is + // that repository's pull request. + MarkdownPreview( + text: document.text, + baseURL: document.url, + links: MarkdownLinks(remote: store.selectedProject?.remote), + onToggleTask: MarkdownTaskToggle(target: document.url.path) { line, isDone in + // The edit lands in the document, not on disk: a + // tick is an edit like any other here, so the tab + // marks it unsaved and ⌘S writes it. Auto-saving + // from a preview would be the one write in the app + // nobody asked for. + guard let updated = MarkdownTask.toggling( + line: line, + to: isDone, + in: document.text + ) else { return } + document.text = updated + } + ) } else if document.isDrawio && store.drawioPreview { DrawioPreview(xml: document.text) } else { diff --git a/TODO.md b/TODO.md index 8d4f327..bf22f54 100644 --- a/TODO.md +++ b/TODO.md @@ -1,105 +1,74 @@ -[x] Resolved comments should be collapse and show resolved status -[x] Resolve / unresolve button on a comment thread in PR detail -[x] When PR detail is opened, build status should be refresh in every 10s -[x] Full refresh button in PR details. and instead of copy PR link in tile, replace with open in browser. - ## Still missing in PR details Roughly in order of how much each one hurts. Everything below was checked against `PullRequestDetailView.swift`, `PullRequestSidebar.swift` and the `PullRequest*` models — none of it exists yet, on either host. -[ ] Mergeable / conflict state. Nothing reads `mergeable` or - `mergeStateStatus`, so Merge is always offered at full strength and a - conflict is only discovered when the host refuses. The sync badge answers a - different question ("behind the target"), not "will this merge". -[ ] Delete a comment. Reply, edit and resolve are all there; delete is not, so - a mistyped comment has to be finished on the web. -[ ] Edit the title. The description has a full Markdown editor, but the title — - the thing every list shows — can only be changed in the browser. -[ ] Draft ↔ Ready for review. `pr.isDraft` is drawn as a badge and never - toggled, so marking a PR ready needs the host. -[ ] Delete the source branch on merge. The merge sheet says outright "nothing is - deleted"; there is no option to, which is the normal end of a merge. -[ ] Remove a reviewer, and re-request a review. `addReviewers` is the only - write; somebody added by mistake stays, and a stale approval cannot be - asked for again. -[ ] Labels. Not in the model, not in the sidebar, not editable — and on most - repos they are how a PR is triaged. -[ ] Assignees. Same: `PullRequestReviewer.swift` only mentions the GitHub - assignees endpoint as a source of *candidates*, never as a field to show or - set. -[ ] Reactions on comments (👍/🎉/…). Both hosts have them; nothing here reads - or posts one, so a comment can only be answered with another comment. -[ ] Linked issues ("Closes #123"). Not parsed, not shown, and not opened — - unlike `#123` in a commit message, which already navigates. -[ ] Per-file "viewed" checkbox in the diff. `DiffFileList` selects a file but - keeps no read/unread state, so a long review loses its place on every - reload. -[ ] Suggested changes (` ```suggestion ` blocks) with an Apply button. They - render as plain fenced code today. -[ ] Reopen a closed PR. The reject sheet says it "can be reopened on GitHub" — - which is exactly where you have to go to do it. Once `state != .open` the - whole action bar disappears. -[ ] Unresolved-thread count in the summary bar. It says "N comments"; what a - reviewer actually wants before merging is how many threads are still open. -[ ] Change the target branch. Fixed at whatever the PR was opened against. +- [ ] Mergeable / conflict state. Nothing reads `mergeable` or + `mergeStateStatus`, so Merge is always offered at full strength and a + conflict is only discovered when the host refuses. The sync badge answers a + different question ("behind the target"), not "will this merge". +- [ ] Delete a comment. Reply, edit and resolve are all there; delete is not, so + a mistyped comment has to be finished on the web. +- [ ] Edit the title. The description has a full Markdown editor, but the title — + the thing every list shows — can only be changed in the browser. +- [ ] Draft ↔ Ready for review. `pr.isDraft` is drawn as a badge and never + toggled, so marking a PR ready needs the host. +- [ ] Delete the source branch on merge. The merge sheet says outright "nothing is + deleted"; there is no option to, which is the normal end of a merge. +- [ ] Remove a reviewer, and re-request a review. `addReviewers` is the only + write; somebody added by mistake stays, and a stale approval cannot be + asked for again. +- [ ] Labels. Not in the model, not in the sidebar, not editable — and on most + repos they are how a PR is triaged. +- [ ] Assignees. Same: `PullRequestReviewer.swift` only mentions the GitHub + assignees endpoint as a source of *candidates*, never as a field to show or + set. +- [ ] Reactions on comments (👍/🎉/…). Both hosts have them; nothing here reads + or posts one, so a comment can only be answered with another comment. +- [ ] Linked issues ("Closes #123"). Not parsed, not shown, and not opened — + unlike `#123` in a commit message, which already navigates. +- [ ] Per-file "viewed" checkbox in the diff. `DiffFileList` selects a file but + keeps no read/unread state, so a long review loses its place on every + reload. +- [ ] Suggested changes (` ```suggestion ` blocks) with an Apply button. They + render as plain fenced code today. +- [ ] Reopen a closed PR. The reject sheet says it "can be reopened on GitHub" — + which is exactly where you have to go to do it. Once `state != .open` the + whole action bar disappears. +- [ ] Unresolved-thread count in the summary bar. It says "N comments"; what a + reviewer actually wants before merging is how many threads are still open. +- [ ] Change the target branch. Fixed at whatever the PR was opened against. ## Still missing in Markdown preview -Checked against `Views/MarkdownPreview.swift` (the block parser and renderer) -and `Support/MarkdownHTMLText.swift`. Everything here affects `.md` files, PR -descriptions and comments alike — they all go through `MarkdownText`. - -[ ] Checklists don't work properly. Four separate things: - - **Not clickable.** `.task` draws a plain `Image(systemName:)`, so a box - can be read but never ticked — a TODO.md or a PR checklist has to be - edited by hand, or on the host. - - **Nested checklists lose their indent.** `.bullet` and `.numbered` both - carry `indent:`; `.task` does not (`MarkdownPreview.swift:173`), so - ` - [ ] sub` is drawn flush left, level with its parent. - - **A box with no text after it is not a box.** The test is - `rest.hasPrefix("[ ] ")` (line 837), so a bare `- [ ]` falls through to a - bullet and shows the literal brackets. - - **Ordered checklists are not recognised.** `1. [ ] thing` goes to - `orderedItem` and prints `[ ]` as text. -[ ] Hard-wrapped paragraphs come apart. Every source line becomes its own - `.paragraph` block (line 847), and the stack puts 8pt between blocks — so a - paragraph wrapped at 80 columns is drawn as a loose ladder of lines instead - of one flowing block. This is the biggest visual gap for real `.md` files. -[ ] Multi-line list items, same cause: a continuation line under a bullet - becomes a paragraph of its own and loses both the indent and the bullet. -[ ] Indented code blocks (four spaces). Only ``` fences are recognised, so - older READMEs render their code as ordinary prose. -[ ] Setext headings. `Title` over `=====` or `-----` — the `---` is matched as - a horizontal rule first (line 827), so it comes out paragraph + rule. -[ ] Horizontal rules only match `---` and `***` exactly. `___`, `----` and - `- - -` are all valid and all render as text. -[ ] Bare URLs are not links. Both hosts autolink `https://…` written on its - own; here only `[text](url)` and `` become links. (Inferred from what - `AttributedString(markdown:)` does — worth a quick check before fixing.) -[ ] Reference-style links. `[text][ref]` with `[ref]: https://…` further down: - the link stays literal and the definition line renders as a paragraph. -[ ] Footnotes (`[^1]`). Not parsed at all. -[ ] Table alignment. The `|:---:|` row is dropped (line 747) and every cell is - leading-aligned, so numeric columns cannot be right-aligned. -[ ] HTML tables. `MarkdownHTMLText` knows `
` and `
` as - containers and nothing else, so a bot's `` collapses into a run of - text with its structure gone. -[ ] Emoji shortcodes (`:tada:`, `:warning:`). Both hosts render them; here they - stay as the colon-name. -[ ] `#123` and `@name` are not links in rendered Markdown. `CommitMessageText` - already navigates `#123` in a commit subject — a PR description and a - comment, where they are far more common, do not. -[ ] A code fence or a nested list inside a list item is hoisted out of it. The - parser is line-by-line with no block nesting below quotes and `
`. -[ ] Copy button on a code block. Everything is selectable, but a fenced block - is the one thing people copy whole. -[ ] Images cannot be opened. They are capped at 420pt tall and there is no - click-to-zoom, so a wide screenshot in a PR description can only be - squinted at. -[ ] No outline for long documents. Headings are drawn but not anchored, so a - big README has no way to jump around it. +The list that used to be here was one bug written out sixteen ways: the parser +read the document a line at a time and there was no block tree. It has been +replaced by [swift-markdown](https://github.com/swiftlang/swift-markdown) — +cmark-gfm, which is what both hosts render a comment with — so 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 inside a list item are all +answered by the tree rather than by code of ours. Bare URLs, `:tada:`, `#123`, +`@name` and footnotes are one scan over the text on top of it, checkboxes tick +and write back, fenced blocks have a copy button, pictures open at full size and +a long document has an outline. See `Docs/Markdown.md` for the reasoning and +`Support/MarkdownParser.swift` for the walk. + +Struck rather than done: **a bare `- [ ]` is not a box**. cmark-gfm wants text +after the brackets, and both hosts *are* cmark-gfm — so matching it is fidelity, +not a bug. + +What is left, and both need a real HTML parser rather than the tag scanner in +`Support/MarkdownHTMLText.swift`: + +- [ ] HTML tables are read, but only the plain shape — `
`, ``, `` was renumbered — and `` 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 `
`, + ``. A `colspan`, a `rowspan` or an `align=` on a cell is dropped, and a + table with a blank line inside it is split by cmark before we ever see it. + [SwiftSoup](https://github.com/scinfu/SwiftSoup) is the tool if that ever + matters. +- [ ] `` inside a table cell is drawn as its alt text. A picture is a block + of its own here, and a cell has nowhere to put one. ## Missing elsewhere in the app @@ -109,59 +78,58 @@ not against the README. ### Git -[ ] No branch switcher. Only two branches can ever be checked out: the - repository's default (the dashboard button) and a PR's source branch. There - is no list of local or remote branches, and no create, rename or delete. - `git for-each-ref` appears exactly once, in `DefaultBranch.swift:114`, and - only to guess which branch is the default. -[ ] No stash. Nothing to park half-done work on — which compounds the one - above, since switching branches is what stashing is usually for. -[ ] No blame. No way to ask who wrote a line, or why. -[ ] No history for a single file. The dashboard lists the repository's recent - commits; there is no log for the file you are reading. -[ ] No amend, revert or cherry-pick. Commit and push are there, so fixing the - commit you just made means dropping to the terminal. -[ ] No tags. Not listed, not created — so nothing in the app says what shipped. -[ ] No diff between two arbitrary refs. A PR's diff and a commit's patch are - the only two; `main...my-branch` cannot be asked for. -[ ] No submodule awareness at all. +- [ ] No branch switcher. Only two branches can ever be checked out: the + repository's default (the dashboard button) and a PR's source branch. There + is no list of local or remote branches, and no create, rename or delete. + `git for-each-ref` appears exactly once, in `DefaultBranch.swift:114`, and + only to guess which branch is the default. +- [ ] No stash. Nothing to park half-done work on — which compounds the one + above, since switching branches is what stashing is usually for. +- [ ] No blame. No way to ask who wrote a line, or why. +- [ ] No history for a single file. The dashboard lists the repository's recent + commits; there is no log for the file you are reading. +- [ ] No amend, revert or cherry-pick. Commit and push are there, so fixing the + commit you just made means dropping to the terminal. +- [ ] No tags. Not listed, not created — so nothing in the app says what shipped. +- [ ] No diff between two arbitrary refs. A PR's diff and a commit's patch are + the only two; `main...my-branch` cannot be asked for. +- [ ] No submodule awareness at all. ### The PR board -[ ] **You cannot create a pull request.** Read, review, comment, approve, merge - and reject are all there; opening one is not. The README's shortcut table - lists ⌃⌘P "Create PR with Claude Code" and nothing in the code implements - it — there is no such `ShortcutAction` case and no handler. -[ ] The board has no filter or search. `PullRequestTable.swift` sorts and never - filters — no by-author, no "needs my review", no title search. -[ ] No cross-repo review inbox. Each repository's PRs are their own list, so - there is no single view of everything waiting on you. +- [ ] **You cannot create a pull request.** Read, review, comment, approve, merge + and reject are all there; opening one is not. The README's shortcut table + lists ⌃⌘P "Create PR with Claude Code" and nothing in the code implements + it — there is no such `ShortcutAction` case and no handler. +- [ ] The board has no filter or search. `PullRequestTable.swift` sorts and never + filters — no by-author, no "needs my review", no title search. +- [ ] No cross-repo review inbox. Each repository's PRs are their own list, so + there is no single view of everything waiting on you. ### Editor and language servers -`LanguageService` covers diagnostics, completions, hover, document symbols and -go-to-definition. The rest of the protocol is unimplemented: +`LanguageService` covers diagnostics, completions, hover, document symbols and go-to-definition. The rest of the protocol is unimplemented: -[ ] Rename symbol (`textDocument/rename`). -[ ] Find references (`textDocument/references`). -[ ] Code actions / quick fixes. A diagnostic can be read but never acted on, - which is the single most-used thing an LSP offers after completion. -[ ] Format document (`textDocument/formatting`), and so no format-on-save. -[ ] Signature help — completions are there, parameter hints are not. -[ ] Workspace symbol search. ⌘P finds files; nothing finds a symbol. -[ ] Inlay hints and code lens. +- [ ] Rename symbol (`textDocument/rename`). +- [ ] Find references (`textDocument/references`). +- [ ] Code actions / quick fixes. A diagnostic can be read but never acted on, + which is the single most-used thing an LSP offers after completion. +- [ ] Format document (`textDocument/formatting`), and so no format-on-save. +- [ ] Signature help — completions are there, parameter hints are not. +- [ ] Workspace symbol search. ⌘P finds files; nothing finds a symbol. +- [ ] Inlay hints and code lens. ### Search and diffs -[ ] No replace across files. `FileSearch.swift` finds and never writes. -[ ] No diff options — no ignore-whitespace, no context-line count, no way to - hide whitespace-only changes. A reformatting commit is unreadable. +- [ ] No replace across files. `FileSearch.swift` finds and never writes. +- [ ] No diff options — no ignore-whitespace, no context-line count, no way to + hide whitespace-only changes. A reformatting commit is unreadable. ### Shortcuts -[ ] ⌘N and ⇧⌘N are not rebindable. They are `.keyboardShortcut` literals in - `WorkspaceApp.swift:45` and `:50` instead of `ShortcutAction` cases, which - is exactly what CLAUDE.md says not to do — and it makes the README's "every - shortcut the app binds is rebindable" untrue. -[ ] README shortcut table needs a pass: ⌘, and ⇧⌘O are each listed twice, and - ⌃⌘P documents a command that does not exist. +- [ ] ⌘N and ⇧⌘N are not rebindable. They are `.keyboardShortcut` literals in + `WorkspaceApp.swift:45` and `:50` instead of `ShortcutAction` cases, which + is exactly what CLAUDE.md says not to do — and it makes the README's "every + shortcut the app binds is rebindable" untrue. +- [ ] README shortcut table needs a pass: ⌘, and ⇧⌘O are each listed twice, and + ⌃⌘P documents a command that does not exist. From 34a2634dc7ce84a85899d493826f27f25a65373e Mon Sep 17 00:00:00 2001 From: necessarylion Date: Thu, 30 Jul 2026 10:17:52 +0700 Subject: [PATCH 2/2] The review's findings, and one of them was a crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **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 `
A
` is read" rather than "`
` 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) Claude-Session: https://claude.ai/code/session_01BAgEj6aqwdPgmZEq5BapSu --- CLAUDE.md | 2 +- Docs/Markdown.md | 2 +- .../Workspace/Editor/EditorThemeBridge.swift | 8 ++++--- .../Workspace/Support/MarkdownHTMLText.swift | 13 +++++++++-- .../Workspace/Support/MarkdownInline.swift | 2 +- .../Workspace/Support/MarkdownParser.swift | 10 +++++++- Sources/Workspace/Support/RemoteImage.swift | 6 +++++ Sources/Workspace/Views/MarkdownPreview.swift | 16 ++++++++++++- .../Views/PullRequestDetailView.swift | 23 +++++++++++++------ Sources/Workspace/Views/ViewerView.swift | 13 +++++++++-- 10 files changed, 76 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index bc86461..d877053 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,7 +102,7 @@ The app is one window with three panes, and the mental model that drives the cod `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 - `
` is read only in its plain shape. + only the plain shape of a `
` is read. - Comments are dropped **from the tree, not from the text**, because every node carries a 1-based source line and that is what a tickable checkbox writes back to (`MarkdownTask.toggling`). Cutting lines out beforehand would diff --git a/Docs/Markdown.md b/Docs/Markdown.md index 9ea0601..713db95 100644 --- a/Docs/Markdown.md +++ b/Docs/Markdown.md @@ -42,7 +42,7 @@ than code of ours: | Rules only match `---` and `***` | `___`, `----` and `- - -` all give `ThematicBreak` | | Indented code blocks are prose | `CodeBlock language: none` | | Reference-style links stay literal | resolved to a `Link` with its destination; the `[ref]: …` line disappears | -| Table alignment is dropped | `columnAlignments: |l|c|r|` | +| Table alignment is dropped | `columnAlignments: \|l\|c\|r\|` | | A fence inside a list item is hoisted out | nested, as written | | Checklists are not clickable | every node carries a **source range** (line:column) | | No outline for long documents | `Heading` nodes, each with a range | diff --git a/Sources/Workspace/Editor/EditorThemeBridge.swift b/Sources/Workspace/Editor/EditorThemeBridge.swift index cf68f9a..7477231 100644 --- a/Sources/Workspace/Editor/EditorThemeBridge.swift +++ b/Sources/Workspace/Editor/EditorThemeBridge.swift @@ -26,9 +26,11 @@ extension SyntaxTheme { /// unrecognised list — `text.title`, `text.literal`, `text.uri`, /// `text.reference`, `punctuation.special`, `punctuation.delimiter`, /// `string.escape` — so every range is dropped and a `.md` file in the editor - /// paints in one flat colour. It is not broken, it is this list; the preview - /// (⌘-toggled, and the way a `.md` file is meant to be read here) colours the - /// same file properly because it goes through ``TreeSitterHighlighter``. + /// paints in one flat colour. It is not broken, it is this list; and the + /// preview (⌘-toggled, and the way a `.md` file is meant to be read here) + /// does not go through any of this — `MarkdownParser` reads the document and + /// `MarkdownText` draws it, with ``TreeSitterHighlighter`` reaching only as + /// far as the snippet inside a fence. /// /// Fixing it would mean rewriting the vendored markdown queries onto the /// names the package knows, which loses the bold heading and the italic diff --git a/Sources/Workspace/Support/MarkdownHTMLText.swift b/Sources/Workspace/Support/MarkdownHTMLText.swift index af45aea..ab57f55 100644 --- a/Sources/Workspace/Support/MarkdownHTMLText.swift +++ b/Sources/Workspace/Support/MarkdownHTMLText.swift @@ -191,9 +191,18 @@ enum MarkdownHTMLText { } } + // An empty `` carries nothing and would draw as a blank + // stripe — but dropping one renumbers every row after it, so where the + // header sits is corrected in the same breath. Read out of the old + // numbering afterwards it is off by the empties above it, and a table + // whose only rows are an empty one and a header takes the array out of + // range altogether. + let named = headerRow ?? 0 + guard named < rows.count else { return nil } + let position = named - rows[.." index = end continue diff --git a/Sources/Workspace/Support/MarkdownParser.swift b/Sources/Workspace/Support/MarkdownParser.swift index e47d8f9..1266992 100644 --- a/Sources/Workspace/Support/MarkdownParser.swift +++ b/Sources/Workspace/Support/MarkdownParser.swift @@ -179,7 +179,15 @@ private struct Builder { // which is what lets a tick be written back to the right // `[ ]`, and the reason comments are dropped from the tree // rather than cut out of the text beforehand. - line: item.range?.lowerBound.line, + // + // Nothing at all below the top level, and that is not a + // nicety: `depth` counts the loose HTML re-read as a + // document of its own, and cmark numbers *that* document + // from its own first line. A box in there would name a line + // in the snippet and a tick would be written over whatever + // happens to sit on that line of the real one. Undrawable + // is the right answer; corrupting the document is not. + line: depth == 0 ? item.range?.lowerBound.line : nil, blocks: blocks(of: item) ) ) diff --git a/Sources/Workspace/Support/RemoteImage.swift b/Sources/Workspace/Support/RemoteImage.swift index e75b3f2..2af6c8e 100644 --- a/Sources/Workspace/Support/RemoteImage.swift +++ b/Sources/Workspace/Support/RemoteImage.swift @@ -165,6 +165,12 @@ struct MarkdownImage: View { .overlay(RoundedRectangle(cornerRadius: 6).stroke(.quaternary, lineWidth: 1)) .contentShape(RoundedRectangle(cornerRadius: 6)) .accessibilityLabel(alt.isEmpty ? "Image" : alt) + // `onTapGesture` is a pointer and nothing else: without these + // the picture is announced as a picture and there is no way to + // open it but a mouse. + .accessibilityAddTraits(.isButton) + .accessibilityHint("Opens this picture at full size") + .accessibilityAction { isZoomed = true } // A wide screenshot in a pull request is capped at 420pt tall // and can only be squinted at, so a click opens it at its own // size rather than handing it to the browser. The browser is diff --git a/Sources/Workspace/Views/MarkdownPreview.swift b/Sources/Workspace/Views/MarkdownPreview.swift index 979c62a..d6b147a 100644 --- a/Sources/Workspace/Views/MarkdownPreview.swift +++ b/Sources/Workspace/Views/MarkdownPreview.swift @@ -856,9 +856,23 @@ private struct MarkdownLinksKey: EnvironmentKey { /// closure was made this time round. struct MarkdownTaskToggle: Equatable { let target: String + /// The text this action was built against. + /// + /// It is here because `target` alone is *too* stable. An action that reads + /// its source when it runs would need nothing else — but a comment's does + /// not: the words it flips are a `String` captured when the closure was + /// made. Equal on `target` alone, the first action would be kept for as + /// long as the comment kept its id, so a second tick would be computed + /// against the body from before the first one and posted back over it. + /// Comparing the content is what retires the stale closure, and it costs + /// nothing in the ordinary case: the same string, so `==` answers on the + /// pointer. + let content: String let perform: (Int, Bool) -> Void - static func == (lhs: Self, rhs: Self) -> Bool { lhs.target == rhs.target } + static func == (lhs: Self, rhs: Self) -> Bool { + lhs.target == rhs.target && lhs.content == rhs.content + } } /// Nothing by default: a release note and a `
` section from a bot have diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index ce171c9..8f7d34e 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -688,7 +688,16 @@ struct PullRequestDetailView: View { MarkdownText(text: pr.body) .environment( \.markdownTaskToggle, - MarkdownTaskToggle(target: "pr-\(pr.number)-description", perform: toggleDescriptionTask) + MarkdownTaskToggle( + target: "pr-\(pr.number)-description", + // The action below asks the host for the + // description when it runs, so nothing + // stale can be captured — but the body is + // still what it was built for, and saying + // so keeps every one of these honest. + content: pr.body, + perform: toggleDescriptionTask + ) ) .font(.callout) .transition(ViewerMotion.contentArrival) @@ -2386,12 +2395,12 @@ struct CommentBubble: View { /// a bot's review, and everyone else's words. private var taskToggle: MarkdownTaskToggle? { guard let onEditSubmitted else { return nil } - return MarkdownTaskToggle(target: "comment-\(comment.id)") { line, isDone in - guard let updated = MarkdownTask.toggling( - line: line, - to: isDone, - in: comment.editableBody - ) else { return } + let body = comment.editableBody + // `content` is that same body, so that ticking a second box after the + // conversation has come back from the host builds its answer on what + // the host now holds rather than on what this closure captured. + return MarkdownTaskToggle(target: "comment-\(comment.id)", content: body) { line, isDone in + guard let updated = MarkdownTask.toggling(line: line, to: isDone, in: body) else { return } Task { await onEditSubmitted(updated) } } } diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index c3b5cf7..23b1c04 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -382,8 +382,17 @@ struct ViewerView: View { MarkdownPreview( text: document.text, baseURL: document.url, - links: MarkdownLinks(remote: store.selectedProject?.remote), - onToggleTask: MarkdownTaskToggle(target: document.url.path) { line, isDone in + // The repository the file is *in*, not the one selected + // in the sidebar: with more than one open, a `#123` in + // a file from another checkout would otherwise point at + // the selected repository's pull request. + links: MarkdownLinks( + remote: (store.project(containing: document.url) ?? store.selectedProject)?.remote + ), + onToggleTask: MarkdownTaskToggle( + target: document.url.path, + content: document.text + ) { line, isDone in // The edit lands in the document, not on disk: a // tick is an edit like any other here, so the tab // marks it unsaved and ⌘S writes it. Auto-saving