Repository navigation
Restore Zhuyin IME candidate marked-text handling - #3574
Merged
Merged
Conversation
Zhuyin candidate UI asks the NSTextInputClient about the active marked range and text while composition is in progress. Dictation support had redirected selectedRange and substring queries to terminal selection snapshots, which left active IME preedit without its own selection/content answers. Keep terminal-selection behavior for non-IME accessibility and dictation, but let active marked text own selection and substring responses. Constraint: Preserve dictation/accessibility terminal-selection fallback when no marked text is active Rejected: Special-case Traditional Chinese input source IDs | the bug is in the generic marked-text client contract Confidence: medium Scope-risk: narrow Directive: Do not route active marked-text selectedRange or attributedSubstring through terminal selection snapshots Tested: ./scripts/reload.sh --tag issue-3571-zhuyin-ime-candidate Not-tested: Manual Zhuyin candidate window dogfood; local XCTest per project testing policy
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds explicit IME composition selection tracking and clamping helpers; updates NSTextInputClient paths in GhosttyTerminalView to prefer and maintain a marked-text selection during composition; introduces GhosttyNSView helpers for normalizing/clamping marked ranges; adds unit tests and Xcode project entries. ChangesIME Composition Selection Management
Sequence Diagram(s)sequenceDiagram
participant App as App (Event loop)
participant View as GhosttyNSView / GhosttyTerminalView
participant IME as macOS IME (NSTextInputClient)
participant Core as Terminal Core (input forwarding)
App->>View: keyDown(event)
View->>IME: query markedText / markedSelectedRange (capture before)
IME-->>View: markedText / composition state
View->>View: update markedSelectedRange (normalize/clamp)
alt composing exists
View->>View: attributedSubstring(forProposedRange) → composing substring
View->>Core: suppress or forward accumulated text based on IME handling
Core-->>View: acknowledgement
else no composition
View->>Core: forward key event / insertText
Core-->>View: acknowledgement
end
View->>IME: unmarkText() when composition ends
IME-->>View: cleared state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 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 |
Contributor
Greptile SummaryThis PR restores correct Zhuyin/CJK IME behavior by tracking the active marked-text selection in a new
Confidence Score: 5/5Safe to merge — targeted NSTextInputClient corrections with no timing dependencies and clear state ownership throughout. All three No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[keyDown event received] --> B[snapshot markedStateBefore]
B --> C[interpretKeyEvents]
C --> D{keyboard layout changed?}
D -- Yes --> E[syncPreedit + return]
D -- No --> F[syncPreedit]
F --> G{accumulatedText non-empty?}
G -- Yes --> H[forward committed text to Ghostty]
G -- No --> I{IME state changed?}
I -- Yes --> J[suppress Ghostty key forwarding]
I -- No --> K{active or had marked text?}
K -- Yes --> L[forward key with composing=true]
K -- No --> M[forward key normally]
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmuxTests/CJKIMEInputTests.swift`:
- Around line 367-440: The file exceeds the Swift file-length budget because the
new tests (testSelectedRangeReturnsEmptyRangeWithoutSelectionOrMarkedText,
testSelectedRangeTracksMarkedTextSelection,
testSelectedRangeReturnsEmptyRangeAfterCompositionEnds,
testAttributedSubstringReturnsMarkedTextSegment,
testTraditionalChineseZhuyinMarkedTextSelectionAndSubstring) appended to the
large cmuxTests/CJKIMEInputTests.swift push it over the limit; fix by moving
these test functions (which reference GhosttyNSView and its selectedRange(),
setMarkedText(...), unmarkText(), and
attributedSubstring(forProposedRange:actualRange:)) into a new test file (e.g.,
cmuxTests/CJKIMEMarkedSelectionTests.swift) and ensure the new file has the
appropriate import/test class scaffolding and test target so CI file-length
check passes.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 12828-12853: Extract the two IME helper methods
normalizedMarkedSelectionRange(_:) and clampedMarkedTextRange(_:) from
GhosttyTerminalView.swift into a new Swift file by creating an extension on
GhosttyTerminalView (e.g., GhosttyNSView+IMEComposition) and paste the methods
verbatim (keeping their signatures, private access, and any references to
markedText); remove the original implementations from GhosttyTerminalView.swift
so the file shrinks under CI budget, ensure the new file imports
Foundation/AppKit as needed and is included in the same target so compilation
and behavior remain unchanged.
🪄 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: f5986c26-e58e-4e48-8a06-a60401c50ab1
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
The Zhuyin marked-text regression tests and helper logic were valid, but they pushed already-large files over the review budget. Move the marked-text range helper into a focused GhosttyNSView extension and keep the new IME coverage in a dedicated test file so the behavior stays covered without expanding the overloaded CJK test file. Constraint: CodeRabbit file budget review requires both GhosttyTerminalView.swift and CJKIMEInputTests.swift to stay under their configured limits Rejected: Leave the helpers inline | it keeps the behavioral fix correct but fails the repository review budget Confidence: high Scope-risk: narrow Directive: Keep active marked-text range helpers near IME composition code instead of re-expanding GhosttyTerminalView.swift Tested: ./scripts/reload.sh --tag issue-3571-zhuyin-ime-candidate Not-tested: Local XCTest per project testing policy
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 `@cmuxTests/CJKIMEMarkedSelectionTests.swift`:
- Line 10: The CJKIMEMarkedSelectionTests class is missing an explicit deinit
which triggers the required_deinit SwiftLint rule; add a deinit { } to the final
class CJKIMEMarkedSelectionTests to satisfy the linter (place the deinit inside
the class body, keeping it empty if no teardown is needed) so CI linting no
longer fails.
🪄 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: ccc34947-c2ac-4531-9b39-6839c8a7d88b
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojSources/GhosttyNSView+IMEComposition.swiftSources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swiftcmuxTests/CJKIMEMarkedSelectionTests.swift
The dedicated Zhuyin marked-selection test class needs the repo's explicit deinit convention even though it has no teardown work. Add the empty deinit so review and lint gates stay focused on the IME behavior rather than class-shape warnings. Constraint: SwiftLint required_deinit applies to XCTest classes in this target Rejected: Suppress the lint rule locally | the existing test style expects explicit deinit declarations Confidence: high Scope-risk: narrow Tested: ./scripts/reload.sh --tag issue-3571-zhuyin-ime-candidate Not-tested: Local XCTest per project testing policy
Zhuyin can update marked text during interpretKeyEvents without committing insertText. Treat marked-text or marked-selection changes as AppKit consuming the key so Ghostty does not receive a second key event while the IME candidate flow owns composition state. Constraint: NSTextInputClient marked-text transitions are synchronous AppKit ownership boundaries Rejected: Suppress only Traditional Chinese source IDs | the invariant applies to any IME that mutates marked text without committing text Confidence: high Scope-risk: narrow Directive: Do not forward Ghostty key events after interpretKeyEvents mutates marked text unless insertText committed payload exists Tested: git diff --check Not-tested: local xcodebuild/tests unavailable by repo and user policy; CI will validate after push
Bring the PR branch onto the current main tip so CI validates the IME candidate fix against the merge target. Constraint: origin/main advanced from v0.64.2 to v0.64.3 while this PR was open Confidence: high Scope-risk: narrow Tested: merge completed by git Not-tested: CI pending after push
After merging current main, GhosttyTerminalView gained upstream lines and the IME bridge was close to the enforced large-file budget. Compact the new assertion and range normalization call without changing the marked-text ownership behavior. Constraint: CodeRabbit/CI enforce line budgets on large Swift files Rejected: Move more NSTextInputClient logic out of the main file | larger migration than needed for this review round Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: local xcodebuild/tests unavailable by repo and user policy; CI will validate after push
2 tasks
This was referenced May 7, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Notes
Fixes #3571
Note
Medium Risk
Changes
NSTextInputClientIME composition behavior and key event forwarding, which can regress text input for CJK/other input methods if edge cases are missed. Coverage is improved with new Zhuyin/CJK regression tests, reducing but not eliminating risk.Overview
Fixes IME marked-text handling in
GhosttyNSViewduring composition by tracking the active marked-text selection, clamping IME range queries to the preedit buffer, and servingattributedSubstringfrom marked text when present.Updates
keyDownto suppress forwarding a Ghostty key event wheninterpretKeyEventsonly mutates IME composition state (marked text/selection) without committing text, preventing duplicate input (notably for Zhuyin).Adds
CJKIMEMarkedSelectionTeststo cover marked selection/substrings and Zhuyin keyDown forwarding behavior, and removes the now-redundantselectedRangetest fromCJKIMEInputTests.Reviewed by Cursor Bugbot for commit 50cb0ff. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Restores correct marked-text handling during IME composition so Zhuyin/CJK candidate windows get the right selection and substrings, and suppresses duplicate terminal key forwarding when composition changes without commit. Keeps terminal-selection fallback for dictation and accessibility. Fixes #3571.
Bug Fixes
GhosttyNSView’sNSTextInputClient,selectedRange()returns the active marked-text selection during composition; falls back to terminal selection only when no marked text exists.attributedSubstring(forProposedRange:)serves substrings from active marked text; ranges are clamped and state resets onunmarkText().interpretKeyEventsmutates marked text or its selection unlessinsertTextcommitted text.Refactors
GhosttyNSView+IMEComposition.swift.CJKIMEMarkedSelectionTests.swift; trimmedCJKIMEInputTests.swiftand small compactions/lint fixes to satisfy file-budget rules.Written for commit 50cb0ff. Summary will update on new commits.
Summary by CodeRabbit