Skip to content

Fix TextBox IME replacement range crash #5346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lawrencecchen wants to merge 3 commits into
base: main
Choose a base branch
from
+62 −1
Open

Fix TextBox IME replacement range crash #5346

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f12db4
Add TextBox stale IME range regression test
lawrencecchen Jun 4, 2026
1c81399
Clamp TextBox IME replacement ranges
lawrencecchen Jun 4, 2026
57e092f
Normalize TextBox marked selection range
lawrencecchen Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion Sources/TextBoxInput.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3636,14 +3636,19 @@ final class TextBoxInputTextView: NSTextView {
}

override func insertText(_ insertString: Any, replacementRange: NSRange) {
let replacementRange = sanitizedTextStorageReplacementRange(replacementRange)
queueAutomaticAttachmentFileCleanup(in: replacementRange)
super.insertText(insertString, replacementRange: replacementRange)
flushAutomaticAttachmentFileCleanup()
onMarkedTextStateChanged(hasMarkedText())
}

override func setMarkedText(_ string: Any, selectedRange: NSRange, replacementRange: NSRange) {
super.setMarkedText(string, selectedRange: selectedRange, replacementRange: replacementRange)
super.setMarkedText(
string,
selectedRange: Self.sanitizedRange(selectedRange, upperBound: Self.textInputStringLength(string)),

@greptile-apps greptile-apps Bot Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 selectedRange in setMarkedText is not covered by a regression test

selectedRange here is sanitized against the incoming marked-text string length (textInputStringLength(string)), which is the correct upper bound per the NSTextInputClient spec (the selection lives within the new marked string, not the text storage). However, there is no test that drives a stale or out-of-bounds selectedRange through setMarkedText to prove this branch works. The existing test only exercises insertText. A stale selectedRange from a multi-step IME composition sequence (e.g. selecting a different candidate after the replacement range was already consumed) would silently fall into this path and a future refactor could break it without a test to catch it.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

replacementRange: sanitizedTextStorageReplacementRange(replacementRange)
Comment on lines +3648 to +3651

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize selectedRange == NSNotFound in the marked-text path.

Line 3649 still lets NSNotFound pass through unchanged. In Sources/GhosttyNSView+IMEComposition.swift:11-23, this repo already treats that case as “caret at end of marked text” when the marked buffer is non-empty; keeping NSNotFound here leaves super.setMarkedText with an invalid marked-selection range on the same IME path this patch is hardening.

Suggested fix 
-        super.setMarkedText(
-            string,
-            selectedRange: Self.sanitizedRange(selectedRange, upperBound: Self.textInputStringLength(string)),
-            replacementRange: sanitizedTextStorageReplacementRange(replacementRange)
-        )
+        super.setMarkedText(
+            string,
+            selectedRange: Self.sanitizedMarkedSelectionRange(
+                selectedRange,
+                upperBound: Self.textInputStringLength(string)
+            ),
+            replacementRange: sanitizedTextStorageReplacementRange(replacementRange)
+        )
         onMarkedTextStateChanged(hasMarkedText())
     }

@@
-    private static func sanitizedRange(_ range: NSRange, upperBound: Int) -> NSRange {
-        guard range.location != NSNotFound else { return range }
+    private static func sanitizedMarkedSelectionRange(_ range: NSRange, upperBound: Int) -> NSRange {
         let upperBound = max(0, upperBound)
+        guard range.location != NSNotFound else {
+            return upperBound > 0
+                ? NSRange(location: upperBound, length: 0)
+                : NSRange(location: NSNotFound, length: 0)
+        }
         let location = min(max(0, range.location), upperBound)
         let length = min(max(0, range.length), upperBound - location)
         return NSRange(location: location, length: length)
     }

Also applies to: 3670-3675

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/TextBoxInput.swift` around lines 3647 - 3650, The call to
super.setMarkedText is passing NSNotFound through as the selectedRange which can
produce an invalid marked-selection; update the selectedRange passed into
Self.sanitizedRange in the setMarkedText path so that if selectedRange ==
NSNotFound it is normalized to the caret-at-end index (use
Self.textInputStringLength(string) as the upperBound) when the marked buffer is
non-empty (mirroring the logic in GhosttyNSView+IMEComposition.swift); apply the
same normalization to the second occurrence of setMarkedText around the
3670–3675 region so both call sites use the sanitized/normalized selectedRange
before calling super.setMarkedText (refer to methods: setMarkedText,
Self.sanitizedRange, Self.textInputStringLength,
sanitizedTextStorageReplacementRange).

)
onMarkedTextStateChanged(hasMarkedText())
}

Expand All @@ -3657,6 +3662,27 @@ final class TextBoxInputTextView: NSTextView {
flushAutomaticAttachmentFileCleanup()
}

private func sanitizedTextStorageReplacementRange(_ range: NSRange) -> NSRange {
guard range.location != NSNotFound else { return range }
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
return Self.sanitizedRange(range, upperBound: attributedString().length)
}

private static func sanitizedRange(_ range: NSRange, upperBound: Int) -> NSRange {
guard range.location != NSNotFound else { return range }
let upperBound = max(0, upperBound)
let location = min(max(0, range.location), upperBound)
let length = min(max(0, range.length), upperBound - location)
return NSRange(location: location, length: length)
}

private static func textInputStringLength(_ string: Any) -> Int {
if let attributed = string as? NSAttributedString {
return attributed.length
}
let plain = (string as? String) ?? String(describing: string)
return (plain as NSString).length
}
Comment on lines +3687 to +3693

@greptile-apps greptile-apps Bot Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 For an unrecognised input type, String(describing:) returns a debug description (e.g. "<NSFoo: 0x…>"), so the resulting length is the character count of that label rather than any meaningful text length. Since this function is the defensive guard against malformed IME input, returning 0 for the unexpected branch is safer — it collapses the selection to an empty range rather than clamping against an arbitrary number.

Suggested change
private static func textInputStringLength(_ string: Any) -> Int {
if let attributed = string as? NSAttributedString {
return attributed.length
}
let plain = (string as? String) ?? String(describing: string)
return (plain as NSString).length
}
private static func textInputStringLength(_ string: Any) -> Int {
if let attributed = string as? NSAttributedString {
return attributed.length
}
if let plain = string as? String {
return (plain as NSString).length
}
return 0
}


override func copy(_ sender: Any?) {
if copySelectedAttachments(to: .general) {
return
Expand Down
12 changes: 12 additions & 0 deletions cmuxTests/TextBoxMentionCompletionTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,18 @@ struct TextBoxMentionCompletionTests {
))
}

@Test
func testTextBoxInsertTextClampsStaleIMERangeAtUTF16End() {
let textView = TextBoxInputTextView(frame: NSRect(x: 0, y: 0, width: 320, height: 30))
textView.string = "日本語"
textView.setSelectedRange(NSRange(location: 3, length: 0))

textView.insertText("、", replacementRange: NSRange(location: 3, length: 1))

#expect(textView.string == "日本語、")
#expect(textView.selectedRange() == NSRange(location: 4, length: 0))
}

@Test
func testTextBoxPublishesCommittedIMETextBeforeClearingMarkedState() {
var text = ""
Expand Down
Loading