Fix TextBox IME and input-source handling#5340
Conversation
|
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:
📝 WalkthroughWalkthroughMarked-text (IME composition) state is propagated from the AppKit text view through the representable coordinator into SwiftUI, gating placeholder rendering, external text synchronization, and submit/send enablement; keyboard shortcuts use a layout-aware command-character translator. ChangesIME-aware text box and keyboard shortcuts
Sequence Diagram(s)sequenceDiagram
participant TextBoxInputTextView
participant Coordinator
participant TextBoxInputView
TextBoxInputTextView->>Coordinator: onMarkedTextStateChanged(hasMarkedText)
Coordinator->>TextBoxInputView: onMarkedTextStateChanged(Bool)
TextBoxInputView->>Coordinator: queuePendingMarkedTextStateSync(from:)
Coordinator->>TextBoxInputTextView: applyPendingMarkedTextStateSyncIfNeeded() / recalculateHeight()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (15 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 |
![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: 57c762c8f9
ℹ️ 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 let event = makeKeyDownEvent( | ||
| key: "c", | ||
| modifiers: [.command], | ||
| keyCode: UInt16(kVK_ANSI_B), | ||
| windowNumber: 0 |
There was a problem hiding this comment.
In this test, key: "c" does not control what textBoxCommandShortcutKey(for:) returns: that helper first calls KeyboardLayout.character, which consults the machine's current TIS input source for event.keyCode. On a normal US/QWERTY CI host, kVK_ANSI_B translates to "b", so the expectation on line 155 fails regardless of the synthetic event's characters; the Y/Z test below has the same issue. Please mock/inject the layout translation or use a key code that matches the active layout.
Useful? React with 👍 / 👎.
lawrencecchen
force-pushed
the
fix-textbox-ime-input-sources
branch
from
57c762c to
e75f972
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lawrencecchen
force-pushed
the
fix-textbox-ime-input-sources
branch
from
e75f972 to
3297644
Compare
![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 `@Sources/TextBoxInput.swift`:
- Line 3142: The binding `@Binding` var hasMarkedText is being written directly
during makeNSView/updateNSView which leaves a stale true value when
dismantleNSView preserves bindings; instead add a one-shot
"pendingMarkedTextSyncForNextLayout" flag (mirroring
pendingAttachmentUploadStateForNextLayout) and set that flag when rebuilding
representables in dismantleNSView or where appropriate, then on the next layout
pass (in the same place where pendingAttachmentUploadStateForNextLayout is
handled) read the new NSTextView.hasMarkedText() and update hasMarkedText once
and clear the pending flag; update references in makeNSView/updateNSView and the
layout sync handler to use pendingMarkedTextSyncForNextLayout rather than
directly assigning hasMarkedText so the fresh NSTextView publishes its real
initial state.
- Line 2657: The send button and submit path must be disabled/guarded while IME
composition is active: use the existing `@State` private var hasMarkedText to
prevent enabling or invoking submit() during marked text. Update the Send
button's binding/disabled logic to include hasMarkedText (e.g., disable when
hasMarkedText is true or when text.isEmpty) and add a guard at the start of
submit() to return early if hasMarkedText is true; ensure any UI paths that call
submit() (button action, shortcut handlers) check hasMarkedText before clearing
or sending text. Also audit related locations referencing text (e.g., where the
button enablement and submit invocation occur) and apply the same hasMarkedText
guard to avoid discarding IME composition.
🪄 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: 8045b780-41a5-44e2-9c91-26abab416cac
📒 Files selected for processing (3)
Sources/TextBoxInput.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/TextBoxMentionCompletionTests.swift
Greptile SummaryThis PR fixes two TextBox regressions: IME marked-text during composition is now preserved (external SwiftUI text sync, placeholder display, and send/submit are all gated on
Confidence Score: 5/5Safe to merge; all new production paths are tested and the AppKit override ordering is sound. The IME gating logic is pure and fully unit-tested; the Coordinator's pending/delivered state machine handles the live-cancels-queued edge case correctly, and the publishTextViewContent idempotency check prevents double onContentChanged calls. The only concern is a style-level file-size issue that pre-dates this PR. Sources/TextBoxInput.swift — already far over the package-boundary line threshold and grows further with this change. Important Files Changed
Reviews (3): Last reviewed commit: "fix: cancel stale textbox marked sync" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let onInsertFileURLs: ([URL], TextBoxInputTextView) -> Bool | ||
| let onChooseFiles: () -> Void | ||
| let onContentChanged: () -> Void | ||
| var onMarkedTextStateChanged: (Bool) -> Void = { _ in } |
There was a problem hiding this comment.
var breaks the let-closure convention for callback properties
Every other callback on TextBoxInputView (onContentChanged, onTextViewCreated, onTextViewMovedToWindow, onTextViewDismantled, etc.) is declared let, making the contract clear: these closures are fixed at construction time. Declaring onMarkedTextStateChanged as var just to supply a default no-op weakens that contract without a compelling reason — makeCoordinator can wire an always-present closure instead, or the default can be expressed with an Optional unwrapped at call sites. As written, any code that sees a TextBoxInputView value can silently reassign the handler.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
| @@ -2625,6 +2655,7 @@ struct TextBoxInputContainer: View { | |||
|
|
|||
| @State private var textViewHeight: CGFloat = 0 | |||
| @State private var hasPendingAttachmentUpload = false | |||
There was a problem hiding this comment.
@State hasMarkedText introduces split SwiftUI/AppKit lifecycle ownership
@State private var hasMarkedText is a mirror of NSTextView.hasMarkedText() maintained through three separate AppKit entrypoints (setMarkedText, unmarkText, insertText) and two delegate callbacks (textViewDidChange, textViewDidChangeSelection). This is the flag/side-channel pattern flagged by the architectural rethink rule: AppKit owns the marked-text invariant, and SwiftUI now owns a shadow copy that must be kept in sync across five call sites.
The sync guard in updateNSView (shouldSynchronizeExternalTextToTextBox) already reads textView.hasMarkedText() directly — the real-time AppKit state — so the shadow @State flag is only needed for placeholder visibility. If any of the five notification paths is missed in a future refactor, the shadow can drift and the placeholder will flicker or show during active composition.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/TextBoxInput.swift (2)
2695-2696:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock button-driven submit while IME composition is active.
hasMarkedTextis now tracked, but the Send button still enables fromtext/attachmentsalone andsubmit()clears the field without checking composition state. Clicking Send mid-IME can still discard the active marked text this PR is trying to preserve.Suggested fix
- let canSend = !hasPendingAttachmentUpload + let canSend = !hasPendingAttachmentUpload + && !hasMarkedText && (!text.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty || !attachments.isEmpty) ... private func submit() { + guard textViewReference.textView?.hasMarkedText() != true else { + NSSound.beep() + return + } guard textViewReference.textView?.hasPendingAttachmentUploadPlaceholder() != true else { NSSound.beep() return }Also applies to: 2817-2835
🤖 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 2695 - 2696, The Send button enabling logic and submit flow must respect IME composition: update the canSend computation (used when rendering the Send button) to also require hasMarkedText == false in addition to the existing hasPendingAttachmentUpload/text/attachments checks (modify the variable named canSend), and update the submit() method to no-op or defer clearing `text` while hasMarkedText is true (check the `hasMarkedText` flag at the start of submit() and return early or postpone clearing any marked text until composition ends). Apply the same change to the other submit-related block referenced around the 2817-2835 region so both button state and submit clearing consistently honor `hasMarkedText`.
3210-3213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQueue a one-shot marked-text sync after rebuilding the representable.
Only pending-upload state gets a post-construction sync. If the old view is torn down while
TextBoxInputContainer.hasMarkedText == true, the replacementNSTextViewnever publishes its initialhasMarkedText() == false, so the placeholder can stay hidden until the user edits again.Suggested fix
final class Coordinator: NSObject, NSTextViewDelegate { var parent: TextBoxInputView private var pendingAttachmentUploadStateForNextLayout: Bool? + private var pendingMarkedTextStateForNextLayout: Bool? init(parent: TextBoxInputView) { self.parent = parent } /// Captures pending-upload state once after representable construction restores AppKit storage. func queuePendingAttachmentUploadStateSync(from textView: TextBoxInputTextView) { pendingAttachmentUploadStateForNextLayout = textView.hasPendingAttachmentUploadPlaceholder() } + + func queuePendingMarkedTextStateSync(from textView: TextBoxInputTextView) { + pendingMarkedTextStateForNextLayout = textView.hasMarkedText() + } ... onTextViewCreated(textView) context.coordinator.queuePendingAttachmentUploadStateSync(from: textView) + context.coordinator.queuePendingMarkedTextStateSync(from: textView) return scrollView ... if let textBoxView = textView as? TextBoxInputTextView { textBoxView.recenterSingleLineTextContainer() applyPendingAttachmentUploadStateSyncIfNeeded() + applyPendingMarkedTextStateSyncIfNeeded() } ... private func applyPendingAttachmentUploadStateSyncIfNeeded() { guard let hasPendingUpload = pendingAttachmentUploadStateForNextLayout else { return } pendingAttachmentUploadStateForNextLayout = nil guard parent.hasPendingAttachmentUpload != hasPendingUpload else { return } parent.hasPendingAttachmentUpload = hasPendingUpload } + + private func applyPendingMarkedTextStateSyncIfNeeded() { + guard let hasMarkedText = pendingMarkedTextStateForNextLayout else { return } + pendingMarkedTextStateForNextLayout = nil + parent.onMarkedTextStateChanged(hasMarkedText) + }Also applies to: 3281-3379
🤖 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 3210 - 3213, After rebuilding the representable, also enqueue a one-shot marked-text sync so the new NSTextView publishes its initial hasMarkedText() state; specifically, right after calling updateTextView(...) and onTextViewCreated(...) (near where context.coordinator.queuePendingAttachmentUploadStateSync(from: textView) is called) add a coordinator method call that synchronizes TextBoxInputContainer.hasMarkedText to the new textView’s hasMarkedText() once (so the placeholder visibility is corrected when the old view was torn down with hasMarkedText == true).
🤖 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.
Duplicate comments:
In `@Sources/TextBoxInput.swift`:
- Around line 2695-2696: The Send button enabling logic and submit flow must
respect IME composition: update the canSend computation (used when rendering the
Send button) to also require hasMarkedText == false in addition to the existing
hasPendingAttachmentUpload/text/attachments checks (modify the variable named
canSend), and update the submit() method to no-op or defer clearing `text` while
hasMarkedText is true (check the `hasMarkedText` flag at the start of submit()
and return early or postpone clearing any marked text until composition ends).
Apply the same change to the other submit-related block referenced around the
2817-2835 region so both button state and submit clearing consistently honor
`hasMarkedText`.
- Around line 3210-3213: After rebuilding the representable, also enqueue a
one-shot marked-text sync so the new NSTextView publishes its initial
hasMarkedText() state; specifically, right after calling updateTextView(...) and
onTextViewCreated(...) (near where
context.coordinator.queuePendingAttachmentUploadStateSync(from: textView) is
called) add a coordinator method call that synchronizes
TextBoxInputContainer.hasMarkedText to the new textView’s hasMarkedText() once
(so the placeholder visibility is corrected when the old view was torn down with
hasMarkedText == true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: de1c6d55-814e-4d12-8400-8856db808e2b
📒 Files selected for processing (2)
Sources/TextBoxInput.swiftcmuxTests/TextBoxMentionCompletionTests.swift
lawrencecchen
force-pushed
the
fix-textbox-ime-input-sources
branch
from
3297644 to
0c171e0
Compare
lawrencecchen
force-pushed
the
fix-textbox-ime-input-sources
branch
from
0c171e0 to
8f71676
Compare
lawrencecchen
force-pushed
the
fix-textbox-ime-input-sources
branch
from
8f71676 to
15ea581
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 95ec20f. Configure here.
![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 (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
lawrencecchen
force-pushed
the
fix-textbox-ime-input-sources
branch
from
df3df3e to
7904df1
Compare
lawrencecchen
dismissed
coderabbitai[bot]’s stale review
Dismissed stale CodeRabbit review: both actionable comments were addressed, later bot checks are green, and current Codex review is clean.
1 participant
Summary
Verification
./scripts/reload.sh --tag tbime4xcodebuild testnot run because cmux tests are prohibited on the local MacRegression structure
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches prompt input, submit gating, and SwiftUI–AppKit text sync—user-visible paths where IME or shortcut bugs could still slip through despite new tests.
Overview
Improves TextBox behavior during IME composition and on non-US keyboard layouts by tracking AppKit marked text in SwiftUI and routing edit shortcuts through layout-translated command characters.
While composition is active, the UI hides the placeholder, disables send/submit, and skips external
textsync so SwiftUI updates do not wipe in-progress input; committed text is published when marked text clears, with a one-shot marked-text sync on representable creation that live updates can cancel. Command+C/X/V/Z (and related edit shortcuts) now key offtextBoxCommandShortcutKeyinstead of fixed ANSI key codes.Pure helpers (
shouldSynchronizeExternalTextToTextBox, placeholder/submit gating, etc.) centralize the rules, and tests cover IME sync, submit gating, coordinator publish ordering, and translated shortcuts.Reviewed by Cursor Bugbot for commit 7904df1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes TextBox IME composition and non‑US keyboard Command shortcuts to prevent text loss, accidental sends, and misrouted actions. Hides the placeholder during composition, blocks submit, syncs AppKit marked text into SwiftUI, and cancels stale initial marked‑state sync on first layout.
hasMarkedTextacross insert/set/unmark and selection; publish committed IME text before clearing; queue a one‑shot marked‑text sync on creation and cancel it if a live update arrives.submit()).textBoxCommandShortcutKeyfor copy/cut/paste and undo.Written for commit 7904df1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests