Repository navigation
Replace markdown pane with STTextView - #2973
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR replaces the Changes
Sequence DiagramsequenceDiagram
participant MarkdownPanelView
participant MarkdownPanelTextSurface
participant MarkdownPanelAttributedRenderer
participant MarkdownPanelTheme
participant MarkdownPanelSTTextContainerView as Container View
participant STTextView
MarkdownPanelView->>MarkdownPanelTextSurface: markdown, ghosttyConfig, isFocused
MarkdownPanelTextSurface->>MarkdownPanelAttributedRenderer: render markdown content
MarkdownPanelAttributedRenderer-->>MarkdownPanelTextSurface: attributed string
MarkdownPanelTextSurface->>MarkdownPanelTheme: create theme from ghosttyConfig
MarkdownPanelTheme-->>MarkdownPanelTextSurface: theme object
MarkdownPanelTextSurface->>MarkdownPanelSTTextContainerView: configure with string & theme
MarkdownPanelSTTextContainerView->>STTextView: set attributed text
MarkdownPanelSTTextContainerView->>STTextView: apply theme styles
MarkdownPanelSTTextContainerView->>STTextView: set focus state
STTextView-->>Container View: display rendered markdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48cc411913
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| guard level > 0, | ||
| source.length > level, | ||
| CharacterSet.whitespaces.contains(UnicodeScalar(source.character(at: level))!) else { |
There was a problem hiding this comment.
Avoid force-unwrapping UnicodeScalar(...) here: a line like #😀 (hash followed by an emoji, i.e. a surrogate pair) makes UnicodeScalar(source.character(at: level)) return nil, which crashes the renderer while processing otherwise valid markdown text. This path runs for every line in attributedString(markdown:theme:), so a single such line can take down the markdown panel.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| enum MarkdownPanelAttributedRenderer { | ||
| private static let markdownLinkRegex = makeRegex(#"\[([^\]]+)\]\(([^)\s]+)\)"#) |
There was a problem hiding this comment.
The markdown link regex stops the destination at the first ) and rejects many legal link forms, so links such as [wiki](https://en.wikipedia.org/wiki/Function_(mathematics)) are parsed as truncated URLs and cmd-click opens the wrong target (or no target). Since link activation now depends on this parser, this introduces a functional regression for common markdown links.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Sources/Panels/MarkdownPanelView.swift (1)
636-659: Avoid force-unwrapping aUnicodeScalarbuilt from a UTF‑16 code unit.
UnicodeScalar(source.character(at: level))!will crash if the character at that index is a surrogate half. While unlikely for a character following#in a heading, the cheaper and safer form isCharacterSet.whitespaces.contains(Unicode.Scalar(source.character(at: level)) ?? Unicode.Scalar(0)), or simply test against0x20/0x09directly. Low severity — included for defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/MarkdownPanelView.swift` around lines 636 - 659, In headingMetadata(in:) avoid force-unwrapping UnicodeScalar built from UTF-16 by replacing CharacterSet.whitespaces.contains(UnicodeScalar(source.character(at: level))!) with a safe check; e.g. create a safe scalar using let scalar = Unicode.Scalar(source.character(at: level)) ?? Unicode.Scalar(0) and call CharacterSet.whitespaces.contains(scalar), or alternatively compare the code unit directly (e.g. let ch = source.character(at: level); guard ch == 0x20 || ch == 0x09 else { return nil }) to eliminate the force unwrap in headingMetadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/MarkdownPanelView.swift`:
- Around line 523-531: The current pipe heuristic uses lineText.contains("|")
(in the block that calls attributed.addAttributes with theme.codeFont and
theme.textColor over lineContentRange) and thus misclassifies prose with single
pipes or operators as tables; update the check to require a more reliable table
pattern (e.g., at least two '|' characters and a following separator row like
/^\s*\|.*\|\s*$/ plus a subsequent line matching a separator such as /^\s*\|?[-:
]+\|[-: |]*$/) before applying table styling, or disable this heuristic entirely
and only apply theme.codeFont when a full table parse is confirmed. Ensure the
change touches the same conditional that currently uses lineText.contains("|")
and still uses attributed.addAttributes with lineContentRange when the stricter
condition is met.
- Around line 216-225: The current link-activation flow calls
NSWorkspace.shared.open(url) without validating the URL scheme; restrict which
schemes are allowed before opening by extracting url.scheme (lowercased) and
allowlisting only "http", "https", "mailto" (and "file" only if you intend to
support local files), and return true without calling
NSWorkspace.shared.open(url) for any other schemes; update the block around
MarkdownPanelLinkActivationPolicy.url(for:) / shouldOpenLink (referencing
MarkdownPanelSTTextView and NSWorkspace.shared.open(url)) to perform this scheme
check and early-return when the scheme is not allowed.
- Around line 179-200: The restoreScrollPosition(previousRatio:) call happens
immediately after setting textView.attributedText while STTextView lays out
asynchronously, so use a deferred restore once layout is finished: after setting
textView.attributedText in the contentChanged branch, request/ensure layout with
textView.textLayoutManager?.ensureLayout(for:) or observe
NSTextLayoutManagerDelegate callbacks (or as a simpler fallback dispatch to the
next run loop with DispatchQueue.main.async) and then call
restoreScrollPosition(previousRatio:); keep currentMarkdown assignment after
successful restore and leave the existing theme-only branch unchanged.
---
Nitpick comments:
In `@Sources/Panels/MarkdownPanelView.swift`:
- Around line 636-659: In headingMetadata(in:) avoid force-unwrapping
UnicodeScalar built from UTF-16 by replacing
CharacterSet.whitespaces.contains(UnicodeScalar(source.character(at: level))!)
with a safe check; e.g. create a safe scalar using let scalar =
Unicode.Scalar(source.character(at: level)) ?? Unicode.Scalar(0) and call
CharacterSet.whitespaces.contains(scalar), or alternatively compare the code
unit directly (e.g. let ch = source.character(at: level); guard ch == 0x20 || ch
== 0x09 else { return nil }) to eliminate the force unwrap in headingMetadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87775ecb-8fb3-4e9c-a227-80b645ae6f5b
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojGhosttyTabs.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedSources/Panels/MarkdownPanelView.swiftcmuxTests/MarkdownPanelTextViewTests.swift
| let previousOrigin = scrollView.contentView.bounds.origin | ||
| let previousViewportHeight = scrollView.contentView.bounds.height | ||
| let previousDocumentHeight = textView.frame.height | ||
| let previousScrollRatio = markdownScrollRatio( | ||
| originY: previousOrigin.y, | ||
| documentHeight: previousDocumentHeight, | ||
| viewportHeight: previousViewportHeight | ||
| ) | ||
|
|
||
| MarkdownPanelEditorConfiguration.apply(theme: theme, to: textView, scrollView: scrollView) | ||
|
|
||
| if themeChanged || contentChanged { | ||
| textView.attributedText = MarkdownPanelAttributedRenderer.attributedString(markdown: markdown, theme: theme) | ||
| } | ||
|
|
||
| if contentChanged { | ||
| restoreScrollPosition(previousRatio: previousScrollRatio) | ||
| currentMarkdown = markdown | ||
| } else if themeChanged { | ||
| scrollView.contentView.scroll(to: previousOrigin) | ||
| scrollView.reflectScrolledClipView(scrollView.contentView) | ||
| } |
There was a problem hiding this comment.
Scroll-position restore may jump after content updates.
restoreScrollPosition(previousRatio:) reads textView.frame.height immediately after textView.attributedText = .... STTextView lays out text asynchronously via NSTextLayoutManager, so frame.height often still reflects the previous document. The computed scrollableHeight and restored y will be based on stale geometry, causing a visible jump (typically to the top) on content updates.
Consider deferring the restore until after layout completes (e.g., using textView.textLayoutManager?.ensureLayout(for:) plus NSTextLayoutManagerDelegate, or dispatching to the next run loop and reapplying), or anchoring to a text location rather than a ratio.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/MarkdownPanelView.swift` around lines 179 - 200, The
restoreScrollPosition(previousRatio:) call happens immediately after setting
textView.attributedText while STTextView lays out asynchronously, so use a
deferred restore once layout is finished: after setting textView.attributedText
in the contentChanged branch, request/ensure layout with
textView.textLayoutManager?.ensureLayout(for:) or observe
NSTextLayoutManagerDelegate callbacks (or as a simpler fallback dispatch to the
next run loop with DispatchQueue.main.async) and then call
restoreScrollPosition(previousRatio:); keep currentMarkdown assignment after
successful restore and leave the existing theme-only branch unchanged.
| guard let url = MarkdownPanelLinkActivationPolicy.url(for: link) else { | ||
| return true | ||
| } | ||
| guard MarkdownPanelLinkActivationPolicy.shouldOpenLink( | ||
| modifierFlags: (textView as? MarkdownPanelSTTextView)?.lastMouseDownModifierFlags ?? [] | ||
| ) else { | ||
| return true | ||
| } | ||
| NSWorkspace.shared.open(url) | ||
| return true |
There was a problem hiding this comment.
Restrict URL schemes before opening.
Markdown content is read from arbitrary files on disk and may contain javascript:, file://, or other unexpected schemes. NSWorkspace.shared.open(url) will happily dispatch those to registered handlers. Consider allowlisting to http, https, mailto (and perhaps file only for intentional cases) before calling open.
Proposed guard
- NSWorkspace.shared.open(url)
- return true
+ let allowedSchemes: Set<String> = ["http", "https", "mailto"]
+ if let scheme = url.scheme?.lowercased(), allowedSchemes.contains(scheme) {
+ NSWorkspace.shared.open(url)
+ }
+ return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/MarkdownPanelView.swift` around lines 216 - 225, The current
link-activation flow calls NSWorkspace.shared.open(url) without validating the
URL scheme; restrict which schemes are allowed before opening by extracting
url.scheme (lowercased) and allowlisting only "http", "https", "mailto" (and
"file" only if you intend to support local files), and return true without
calling NSWorkspace.shared.open(url) for any other schemes; update the block
around MarkdownPanelLinkActivationPolicy.url(for:) / shouldOpenLink (referencing
MarkdownPanelSTTextView and NSWorkspace.shared.open(url)) to perform this scheme
check and early-return when the scheme is not allowed.
| } else if lineText.contains("|") { | ||
| attributed.addAttributes( | ||
| [ | ||
| .font: theme.codeFont, | ||
| .foregroundColor: theme.textColor | ||
| ], | ||
| range: lineContentRange | ||
| ) | ||
| } |
There was a problem hiding this comment.
Pipe-heuristic for tables over-matches prose.
lineText.contains("|") styles any prose line containing a pipe (e.g., use \| to escape, shell examples, if a || b) as a table, applying codeFont and removing paragraph styling. Consider requiring at least two pipes and a separator row (e.g., |---|---|) before treating a line as a table, or skip table styling entirely until a proper parser is used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/MarkdownPanelView.swift` around lines 523 - 531, The current
pipe heuristic uses lineText.contains("|") (in the block that calls
attributed.addAttributes with theme.codeFont and theme.textColor over
lineContentRange) and thus misclassifies prose with single pipes or operators as
tables; update the check to require a more reliable table pattern (e.g., at
least two '|' characters and a following separator row like /^\s*\|.*\|\s*$/
plus a subsequent line matching a separator such as /^\s*\|?[-: ]+\|[-: |]*$/)
before applying table styling, or disable this heuristic entirely and only apply
theme.codeFont when a full table parse is confirmed. Ensure the change touches
the same conditional that currently uses lineText.contains("|") and still uses
attributed.addAttributes with lineContentRange when the stricter condition is
met.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/MarkdownPanelView.swift">
<violation number="1" location="Sources/Panels/MarkdownPanelView.swift:644">
P2: Force-unwrapping `UnicodeScalar(source.character(at: level))` can crash on surrogate code units. Use a safe unwrap instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| guard level > 0, | ||
| source.length > level, | ||
| CharacterSet.whitespaces.contains(UnicodeScalar(source.character(at: level))!) else { |
There was a problem hiding this comment.
P2: Force-unwrapping UnicodeScalar(source.character(at: level)) can crash on surrogate code units. Use a safe unwrap instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/MarkdownPanelView.swift, line 644:
<comment>Force-unwrapping `UnicodeScalar(source.character(at: level))` can crash on surrogate code units. Use a safe unwrap instead.</comment>
<file context>
@@ -290,6 +108,631 @@ struct MarkdownPanelView: View {
+ }
+ guard level > 0,
+ source.length > level,
+ CharacterSet.whitespaces.contains(UnicodeScalar(source.character(at: level))!) else {
+ return nil
+ }
</file context>
| CharacterSet.whitespaces.contains(UnicodeScalar(source.character(at: level))!) else { | |
| let scalar = UnicodeScalar(source.character(at: level)), | |
| CharacterSet.whitespaces.contains(scalar) else { |
Greptile SummaryReplaces the MarkdownUI dependency with a custom
Confidence Score: 4/5Safe to merge after fixing the force-unwrap crash in headingMetadata; all other findings are P2 style/quality improvements. One P1 crash exists in headingMetadata where a non-BMP character immediately after heading markers will hit a nil force-unwrap. The remaining issues (cursor color mismatch, overly-broad table heuristic) are P2 and do not block correctness on typical markdown inputs. The overall architecture is sound, scroll preservation and cmd-click link handling are well-structured, and the unit tests cover the key behavioral contracts. Sources/Panels/MarkdownPanelView.swift — headingMetadata force-unwrap (line ~672) and table heuristic (line ~523) Important Files Changed
Sequence DiagramsequenceDiagram
participant SwiftUI as SwiftUI (MarkdownPanelView)
participant Surface as MarkdownPanelTextSurface
participant Container as MarkdownPanelSTTextContainerView
participant Renderer as MarkdownPanelAttributedRenderer
participant Theme as MarkdownPanelTheme
participant TextView as MarkdownPanelSTTextView
SwiftUI->>Surface: updateNSView(markdown, config, isFocused)
Surface->>Container: update(markdown, ghosttyConfig, isFocused)
Container->>Theme: init(config) → cacheKey
Container->>Container: detect themeChanged / contentChanged / focusChanged
alt themeChanged or contentChanged
Container->>Renderer: attributedString(markdown, theme)
Renderer-->>Container: NSAttributedString
Container->>TextView: attributedText = …
end
alt themeChanged
Container->>TextView: apply(theme, textView, scrollView)
end
alt contentChanged
Container->>Container: restoreScrollPosition(previousRatio)
end
alt isFocused && focusChanged
Container->>TextView: makeFirstResponder
end
TextView-->>SwiftUI: rendered view
Note over TextView: On mouseDown
TextView->>TextView: capture lastMouseDownModifierFlags
TextView->>Container: clickedOnLink(link, location)
Container->>Container: check .command modifier
alt cmd held
Container->>Container: NSWorkspace.open(url)
end
TextView->>TextView: clear lastMouseDownModifierFlags
Reviews (1): Last reviewed commit: "Replace markdown pane with STTextView" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let match = orderedListRegex.firstMatch(in: lineText, range: fullRange) { | ||
| return match.range | ||
| } | ||
| return nil |
There was a problem hiding this comment.
source.character(at: level) returns a unichar (UInt16). For any character outside the Basic Multilingual Plane (emoji, certain CJK characters), NSString represents it as a UTF-16 surrogate pair. If the character immediately after the # markers happens to be a leading surrogate (0xD800–0xDBFF) — for example ##😀 No space — UnicodeScalar(surrogate) returns nil and the ! crashes the process.
The guard already correctly rejects ##emoji (no space) by the whitespace check failing, but the crash fires before the check can return false. Replace with an optional-safe path:
| return nil | |
| let scalar = UnicodeScalar(UInt32(source.character(at: level))) | |
| guard level > 0, | |
| source.length > level, | |
| let scalar, | |
| CharacterSet.whitespaces.contains(scalar) else { |
| textView.font = theme.font | ||
| textView.textColor = theme.textColor | ||
| textView.backgroundColor = theme.editorBackgroundColor | ||
| textView.insertionPointColor = theme.linkColor |
There was a problem hiding this comment.
insertionPointColor uses link color instead of cursor color
GhosttyConfig.cursorColor is captured in the theme's cacheKey (and thus tracked for invalidation), but it's never surfaced as a theme field. Instead the insertion-point color is set to linkColor, which is a semantic mismatch. For a read-only view the cursor is rarely visible, but if the view ever receives focus the user will see the link color as the caret.
| textView.insertionPointColor = theme.linkColor | |
| textView.insertionPointColor = theme.cursorColor |
(Requires adding let cursorColor: NSColor to MarkdownPanelTheme and setting it to config.cursorColor in init.)
| } else if lineText.contains("|") { | ||
| attributed.addAttributes( | ||
| [ | ||
| .font: theme.codeFont, | ||
| .foregroundColor: theme.textColor | ||
| ], | ||
| range: lineContentRange | ||
| ) |
There was a problem hiding this comment.
Any line that contains a literal | is rendered in codeFont. This catches genuine GFM table rows but also sentences like "use A | B to toggle…" or shell commands in prose. CommonMark/GFM table rows must have at least two |-delimited cells or start/end with |, so a tighter check would reduce false positives:
| } else if lineText.contains("|") { | |
| attributed.addAttributes( | |
| [ | |
| .font: theme.codeFont, | |
| .foregroundColor: theme.textColor | |
| ], | |
| range: lineContentRange | |
| ) | |
| } else if lineText.hasPrefix("|") || (lineText.contains("|") && lineText.filter({ $0 == "|" }).count >= 2) { |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Replaces the markdown pane implementation with STTextView.
What changed:
Verification:
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-sttext-unit test -only-testing:cmuxTests/MarkdownPanelTextViewTests./scripts/reload.sh --tag sttextSummary by cubic
Replaces the markdown pane with
STTextViewfor native text rendering while keeping the Ghostty look and feel. Adds line numbers and cmd-click to open links, and removesMarkdownUI.New Features
STTextViewmarkdown surface with Ghostty font, size, and colors.Dependencies
MarkdownUI→STTextView(addsCoreTextSwiftandSTTextKitPlus).Package.resolved.Written for commit 48cc411. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests