Fix TextBox IME replacement range crash#5346
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
ChangesRange Sanitization for IME Text Input
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ 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 |
Greptile SummaryThis PR adds defensive UTF-16 bounds clamping to
Confidence Score: 5/5Safe to merge — the change is a narrow defensive clamp on two well-understood NSTextView override sites with no effect on non-IME input paths. The fix is a tightly scoped bounds guard: three private helpers with no external surface, applied only at the two NSTextInputClient entry points that the Sentry crash implicated. The sanitization logic is straightforward and correct — NSNotFound ranges are passed through or defaulted per the NSTextInputClient spec, and all arithmetic correctly constrains both location and length. Two regression tests verify the primary crash scenario and the NSNotFound selectedRange fallback. No concurrency, state, or lifecycle changes are involved. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant IME as IME (e.g. Japanese)
participant TV as TextBoxInputTextView
participant SH as sanitizedTextStorageReplacementRange
participant SM as sanitizedMarkedTextSelectionRange
participant NS as NSTextView (super)
IME->>TV: insertText(string, staleRange)
TV->>SH: clamp(staleRange, upperBound: attributedString().length)
SH-->>TV: safeRange
TV->>TV: queueAutomaticAttachmentFileCleanup(safeRange)
TV->>NS: super.insertText(string, safeRange)
NS-->>TV: text inserted
TV->>TV: flushAutomaticAttachmentFileCleanup()
TV->>TV: onMarkedTextStateChanged()
IME->>TV: setMarkedText(string, selectedRange, replacementRange)
TV->>TV: textInputStringLength(string) → markedLen
TV->>SM: clamp(selectedRange, markedTextLength: markedLen)
SM-->>TV: safeSelectedRange
TV->>SH: clamp(replacementRange, upperBound: attributedString().length)
SH-->>TV: safeReplacementRange
TV->>NS: super.setMarkedText(string, safeSelectedRange, safeReplacementRange)
NS-->>TV: marked text set
TV->>TV: onMarkedTextStateChanged()
Reviews (2): Last reviewed commit: "Normalize TextBox marked selection range" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| super.setMarkedText(string, selectedRange: selectedRange, replacementRange: replacementRange) | ||
| super.setMarkedText( | ||
| string, | ||
| selectedRange: Self.sanitizedRange(selectedRange, upperBound: Self.textInputStringLength(string)), |
There was a problem hiding this comment.
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!
| 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 | ||
| } |
There was a problem hiding this comment.
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.
| 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 | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TextBoxInput.swift`:
- Around line 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).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 581f600f-de5d-40e2-a075-392152f421f3
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/TextBoxMentionCompletionTests.swift
| super.setMarkedText( | ||
| string, | ||
| selectedRange: Self.sanitizedRange(selectedRange, upperBound: Self.textInputStringLength(string)), | ||
| replacementRange: sanitizedTextStorageReplacementRange(replacementRange) |
There was a problem hiding this comment.
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).
![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 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Testing
Issues
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized defensive bounds checks on text input paths; regression test added; no auth or data-layer changes.
Overview
Fixes a crash when IME (e.g. Japanese input) passes replacement or marked-text ranges that extend past the current UTF-16 text storage—such as a stale range at the end of
日本語.TextBoxInputTextViewnow clampsreplacementRangeandselectedRangeto valid bounds before callinginsertText/setMarkedText, using the attributed string length (or the incoming marked string length for selection). Attachment cleanup and marked-text callbacks use the same clamped ranges.A unit test covers inserting with an out-of-range replacement at the UTF-16 end boundary.
Reviewed by Cursor Bugbot for commit 1c81399. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Clamp and normalize IME replacement and selection ranges in
TextBoxInputTextViewto valid UTF-16 bounds to prevent NSRange crashes and ensure correct caret placement. Fixes Sentry CMUXTERM-MACOS-VD0.insertText/setMarkedTextreplacementRangeto the current text length; sanitizeselectedRangewithin marked text, defaulting to the end whenNSNotFound.selectedRange.Written for commit 57e092f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests