Skip to content

Fix Zhuyin candidate-window arrow navigation (#3691), with guardrails - #3850

Closed
austinywang wants to merge 5 commits into
mainfrom
issue-3691-zhuyin-candidate-arrows
Closed

austinywang wants to merge 5 commits into
mainfrom
issue-3691-zhuyin-candidate-arrows

Conversation

@austinywang

@austinywang austinywang commented May 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #3691 for the narrow Apple Zhuyin path: com.apple.inputmethod.TCIM.Zhuyin, active marked text, plain Down/Up only.

The previous pushed attempt stopped the shell leak but did not show the candidate menu. That path asked private AppKit candidate-panel selectors to present candidates; Austin dogfooded it and Down still did nothing. This revision removes that non-working presenter path. The current flow first gives the original Down/Up to AppKit text input; if exact plain Apple Zhuyin Down reports moveDown:, produces no committed text, and leaves marked text unchanged, it replays a plain Space key event into the active NSTextInputContext to trigger Zhuyin's candidate-list expansion. Up remains consume-only for navigating an already-open list.

This replaces PR #3694, which was reverted by #3849 after broad IME/window-level routing broke arrows globally for IME users. This PR does not recreate the reverted framework: no window reroute, no keyUp suppression set, no broad IME predicate, and no PageUp/PageDown/Home/End/Space suppression case list. McBopomofo/OpenVanilla remain out of scope.

#3691 Guardrails Confirmation

Citing the mandatory "Guardrails for the next fix attempt" section from #3691:

  1. Only TCIM.Zhuyin: satisfied. The source predicate is an exact case-insensitive compare against com.apple.inputmethod.TCIM.Zhuyin.
  2. No no-marked-text suppression: satisfied. The routing predicate starts with hasMarkedText() == true; Zhuyin outside composition falls through to Ghostty.
  3. Only Down/Up: satisfied. Suppression is limited to kVK_DownArrow and kVK_UpArrow. Candidate expansion is narrower: Down only, after unchanged moveDown: handling.
  4. No window-level reroute: satisfied. No AppDelegate.performKeyEquivalent changes; the hook lives in the focused GhosttyNSView.keyDown path.
  5. No keyUp suppression changes: satisfied. keyUp is untouched and no Set<UInt16> lifecycle was added.
  6. All four gates: satisfied. Marked text, exact Apple Zhuyin source, empty normalized modifiers, and Down/Up are all required before terminal forwarding is suppressed.
  7. Regression guards: satisfied. Commit 1 adds the required Korean, Japanese, Pinyin, Cangjie, Latin/ABC, Zhuyin-outside-composition, and Zhuyin marked-text tests.
  8. Neighboring suites listed below. Focused marked-selection suite is green after this revision; the neighboring CJK input selectors were run earlier and are listed under Verification.
  9. Two commits: satisfied. Commit 1 (216da0338) is test-only. Commit 2 (2c9695e3) lands the fix.
  10. Manual repro capture included. Current-main repro is below. Latest post-fix reload/manual dogfood is intentionally left to Austin per the latest request not to reload locally.

Implementation note: the issue guardrails are satisfied. The later implementation sketch said not to synthesize Space; dogfood proved the private candidate-panel path did not show Apple Zhuyin's menu. The Space replay here is therefore intentionally narrower than #3694: it is only an input-context event after exact Down + Apple Zhuyin + active marked text + unchanged moveDown: no-op, and it does not add any of #3694's broad state/routing machinery.

Manual Repro Capture

Pre-fix tagged current-main build:

  • Ran ./scripts/reload.sh --tag repro-3691-main --launch from post-revert base behavior.
  • App launched as cmux DEV repro-3691-main.
  • In a terminal pane running cat -v, selected com.apple.inputmethod.TCIM.Zhuyin.
  • Pressed a Zhuyin composition key, then Space.
  • Pressed Down: no candidate navigation was visible and cat -v printed ^[[B.
  • Pressed Up: cat -v printed ^[[A.

Latest branch dogfood before 2c9695e3: Down no longer visibly leaked to the shell, but the expected Zhuyin candidate menu still did not appear. This revision replaces that failed presenter path with input-context candidate expansion.

Regression-Guard Test Diff

+func testKoreanInputSourceArrowKeysAlwaysReachShell()
+func testJapaneseInputSourceArrowKeysAlwaysReachShell()
+func testSimplifiedChinesePinyinArrowKeysAlwaysReachShell()
+func testCangjieArrowKeysAlwaysReachShell()
+func testNonIMELayoutArrowKeysAlwaysReachShell()
+func testZhuyinArrowKeysOutsideCompositionReachShell()

Each forwarding guard synthesizes Left/Right/Up/Down/PageUp/PageDown/Home/End/Space with empty modifiers and asserts the key reaches Ghostty.

Zhuyin-Specific Test Diff

+func testZhuyinDownArrowDuringCompositionOpensCandidates()
+func testZhuyinUpArrowDuringCompositionMovesCandidateSelection()
+func testZhuyinCandidateArrowCommitFromInterpretKeyEventsReachesShell()
+func testZhuyinModifiedCandidateArrowsDuringCompositionReachShell()

The positive helper asserts the original arrow goes through AppKit text input, composition remains active, no bare Up/Down reaches Ghostty, and only plain Down asks the input context to expand candidates via a plain Space event. Modified arrows and synchronous committed candidate text do not use the expansion fallback.

Verification

Commit 1 red proof:

  • ./scripts/test-unit.sh test -only-testing:cmuxTests/CJKIMEMarkedSelectionTests
  • Result on commit 1: exactly the two Zhuyin marked-text arrow tests fail; non-Zhuyin regression guards pass.

Post-fix focused proof:

  • ./scripts/test-unit.sh test -only-testing:cmuxTests/CJKIMEMarkedSelectionTests
  • Result after 2c9695e3: 20 tests, 0 failures.

Passing CJKIMEMarkedSelectionTests names:

  • testAttributedSubstringReturnsMarkedTextSegment
  • testCangjieArrowKeysAlwaysReachShell
  • testDoesNotSuppressCommittedIMEInsertText
  • testDoesNotSuppressNormalTerminalKeyWhenIMEDidNothing
  • testJapaneseInputSourceArrowKeysAlwaysReachShell
  • testKeyDownDoesNotForwardWhenZhuyinStartsMarkedText
  • testKoreanInputSourceArrowKeysAlwaysReachShell
  • testNonIMELayoutArrowKeysAlwaysReachShell
  • testSelectedRangeReturnsEmptyRangeAfterCompositionEnds
  • testSelectedRangeReturnsEmptyRangeWithoutSelectionOrMarkedText
  • testSelectedRangeTracksMarkedTextSelection
  • testSimplifiedChinesePinyinArrowKeysAlwaysReachShell
  • testSuppressesTerminalForwardingWhenZhuyinMarkedTextChanges
  • testSuppressesTerminalForwardingWhenZhuyinStartsMarkedText
  • testTraditionalChineseZhuyinMarkedTextSelectionAndSubstring
  • testZhuyinArrowKeysOutsideCompositionReachShell
  • testZhuyinCandidateArrowCommitFromInterpretKeyEventsReachesShell
  • testZhuyinDownArrowDuringCompositionOpensCandidates
  • testZhuyinModifiedCandidateArrowsDuringCompositionReachShell
  • testZhuyinUpArrowDuringCompositionMovesCandidateSelection

Neighboring file run:

  • ./scripts/test-unit.sh test with the XCTestCase selectors from cmuxTests/CJKIMEInputTests.swift was run earlier on this branch.
  • Result: IME-focused classes passed; unrelated existing key-equivalent failures were outside this PR's path.

Static checks:

  • git diff --check passed.
  • Reverted-symbol scan passed: no isInputMethodSource, shouldRouteTextInputKeyEquivalentToKeyDown, shouldKeepIMECompositionCommandInsideTextInput, shouldOpenZhuyinCandidatesWithSyntheticSpace, imeSuppressedKeyUpKeyCodes, zhuyinCandidateOpenRequested, debugTextInputEventHandler, shouldAllowDeferredNumpadIMEFallback, shouldRememberZhuyinCandidateInteraction, or isTraditionalZhuyinInputSource.

Retest Plan for yoonkeee@gmail.com

After CI/nightly is available, send the build to yoonkeee@gmail.com (ko-KR, M4 Pro, macOS 26.4) and ask them to verify Korean 2-Set Hangul with Left/Right/Up/Down/PageUp/PageDown/Home/End/Space in an active terminal session. Expected result: all keys still reach the shell when not composing.

Additional manual dogfood for this branch:

  • Korean 2-Set arrows work.
  • Japanese Hiragana arrows work.
  • Simplified Pinyin arrows work.
  • Latin/ABC arrows work.
  • Zhuyin arrows outside composition work.
  • Zhuyin Down during composition opens the candidate window.
  • Zhuyin Up during composition navigates candidates.

Note

Medium Risk
Touches the core keyDown/IME event path and synthesizes an extra input-context event, so regressions could affect key handling during composition, though the behavior is tightly gated to Apple Zhuyin + marked text + plain Up/Down and is covered by new tests.

Overview
Fixes the narrow Apple Traditional Zhuyin IME case where plain Up/Down arrows during active marked text should control the candidate UI instead of leaking to the terminal.

GhosttyTerminalView.keyDown now detects this gated scenario, routes the original arrow event through interpretKeyEvents, observes the resulting doCommand selector, and if Down results in no composition/commit change, sends a synthetic Space event to the NSTextInputContext to request candidate-list expansion (with a DEBUG hook for tests).

Adds helper predicates in GhosttyNSView+IMEComposition and expands CJKIMEMarkedSelectionTests with regression guards ensuring navigation keys still reach the shell for other input sources, plus Zhuyin-specific tests for arrow suppression/expansion behavior, modified-arrow passthrough, and commit-on-interpret handling.

Reviewed by Cursor Bugbot for commit 22da525. Bugbot is set up for automated code reviews on this repo. Configure here.

Add the issue #3691 regression matrix before changing production code so the first commit demonstrates the current post-revert behavior. Korean, Japanese, Pinyin, Cangjie, Latin/ABC, and Zhuyin without marked text all continue to forward navigation keys, while Zhuyin Up/Down during marked text still leak into Ghostty.

Constraint: Issue #3691 guardrails require a two-commit structure with failing Zhuyin tests first

Constraint: Production code is intentionally untouched in this commit

Confidence: high

Scope-risk: narrow

Directive: Do not broaden these tests into generic IME suppression; they are guarding against the reverted #3694 shape

Tested: ./scripts/test-unit.sh test -only-testing:cmuxTests/CJKIMEMarkedSelectionTests (expected failure: only testZhuyinDownArrowDuringCompositionOpensCandidates and testZhuyinUpArrowDuringCompositionMovesCandidateSelection)

Not-tested: Manual IME repro on tagged dev build not yet captured in this commit
@vercel

vercel Bot commented May 11, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 12, 2026 0:34am
cmux-staging Ready Ready Preview, Comment May 12, 2026 0:34am

@coderabbitai

coderabbitai Bot commented May 11, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Routes Up/Down arrow events to AppKit's IME handler only when Apple Traditional Zhuyin is active and marked text exists; integrates into keyDown, adds a test-only NSTextInputContext.handleEvent swizzle, and expands tests covering multiple IMEs to prevent regressions.

Changes

Zhuyin Arrow-Key Candidate Window Routing

Layer / File(s) Summary
Input Source Detection & Carbon Constants
Sources/GhosttyNSView+IMEComposition.swift
Imports Carbon.HIToolbox; adds appleTraditionalZhuyinInputSourceId and isAppleTraditionalZhuyinInputSource(_:) for case-insensitive Traditional Zhuyin detection.
Zhuyin Arrow Routing Decision Logic
Sources/GhosttyNSView+IMEComposition.swift
Adds shouldRouteKeyToZhuyinCandidateInsteadOfTerminal(event:inputSourceId:) that returns true only when: hasMarkedText()==true, input source is Traditional Zhuyin, masked device-independent modifiers are empty, and keyCode is Up or Down.
Terminal View keyDown Integration
Sources/GhosttyTerminalView.swift
When marked text exists and key is Up/Down, compute optional zhuyinCandidateInputSourceId; if routing predicate is true, call inputContext?.handleEvent(...), call syncPreedit(clearIfNeeded:), and return early (skip interpret/forward).
NSTextInputContext Swizzle (tests)
cmuxTests/CJKIMEInputTests.swift
Adds test-only swizzle: cjkIMETextInputContextHandleEventSwizzled guard, cjkIMETextInputContextHandleEventHook, swizzled cmuxUnitTest_handleEvent(_:), and installCJKIMETextInputContextHandleEventSwizzle() to observe/consume IME handleEvent calls.
Regression & Feature Tests
cmuxTests/CJKIMEMarkedSelectionTests.swift
Imports Carbon in tests; extend keyEvent helper to accept modifierFlags; add ForwardedKeyCase/terminalNavigationKeyCases, assertPlainNavigationKeysReachShell (verifies navigation keys reach Ghostty for non-Zhuyin sources), assertZhuyinCandidateArrowDoesNotReachShell (verifies Zhuyin Up/Down during composition are handled by NSTextInputContext), assertModifiedZhuyinCandidateArrowReachesShell, and new test cases for Korean, Japanese, Simplified Chinese Pinyin, Cangjie, non-IME layouts, and Zhuyin composition scenarios.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant GhosttyNSView
  participant NSTextInputContext
  participant Shell
  User->>GhosttyNSView: keyDown (Up/Down)
  GhosttyNSView->>GhosttyNSView: shouldRouteKeyToZhuyinCandidateInsteadOfTerminal?
  alt predicate true
    GhosttyNSView->>NSTextInputContext: handleEvent(event)
    Note right of NSTextInputContext: IME handles candidate navigation
    GhosttyNSView->>GhosttyNSView: syncPreedit(clearIfNeeded:)
    GhosttyNSView-->>User: return (do not forward to Shell)
  else predicate false
    GhosttyNSView->>GhosttyNSView: interpretKeyEvents / forward
    GhosttyNSView->>Shell: forwarded key bytes
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • manaflow-ai/cmux#790: Prior changes to Ghostty keyDown routing; touches related code paths.
  • manaflow-ai/cmux#3836: Prior attempt touching the same IME routing/predicate logic in GhosttyNSView+IMEComposition.
  • manaflow-ai/cmux#3694: Reverted PR that implemented a broader IME routing change; closely related in intent and code area.

Poem

🐰
Up and down through Zhuyin air,
Candidates hop with careful care,
Marked and matched and modifiers clear,
Only Zhuyin arrows linger near,
A rabbit nods — the bytes stay fair.

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (14 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix Zhuyin candidate-window arrow navigation (#3691), with guardrails' clearly summarizes the main change: a targeted fix for Zhuyin arrow key navigation that adheres to specified guardrails from the linked issue.
Linked Issues check ✅ Passed The PR implementation satisfies all objectives from issue #3691: it restores Zhuyin candidate navigation by routing Up/Down arrows to NSTextInputContext during composition, maintains narrow scope with exact source matching and composition-gated behavior, provides comprehensive regression tests across Korean/Japanese/Pinyin/Cangjie/ABC layouts, follows two-commit structure, and documents manual repro and user retest plan.
Out of Scope Changes check ✅ Passed All changes are in scope and directly support the stated objectives: GhosttyNSView+IMEComposition.swift adds Zhuyin-specific routing logic, GhosttyTerminalView.swift implements the arrow-routing decision in keyDown, and test files add comprehensive regression guards and Zhuyin candidate-window coverage. No unrelated functionality was altered.
Cmux Swift Actor Isolation ✅ Passed No Swift 6 actor isolation issues. New methods are pure functions in NSView subclass (implicitly MainActor), no shared mutable state, no background access to UI-bound stores introduced.
Cmux Swift Blocking Runtime ✅ Passed No blocking/timing primitives detected. Production code adds only synchronous IME routing with AppKit handler calls and preedit sync.
Cmux No Hacky Sleeps ✅ Passed All PR changes are in Swift files (.swift). The rule applies only to TypeScript, JavaScript, shell, and non-Swift runtime scripts. Swift timing is covered by swift-blocking-runtime.md.
Cmux Swift Concurrency ✅ Passed No legacy async patterns introduced. The PR adds only synchronous Swift code for Zhuyin IME routing with no DispatchQueue.global(), Combine app state, completion handlers, or fire-and-forget Tasks.
Cmux Swift @Concurrent ✅ Passed PR contains only synchronous Swift code. Added methods and modifications are all synchronous functions with no async/await or @concurrent annotations. No concurrency violations found.
Cmux Swift File And Package Boundaries ✅ Passed 95-line extension file with focused responsibility. 32-line addition to oversized file (under 250 threshold). Tests in cmuxTests/. No violations found.
Cmux Swift Logging ✅ Passed PR introduces no logging violations. No print/debugPrint/dump/NSLog statements added. No Logger constants. No secrets/sensitive data. New code only handles key routing logic.
Cmux Swiftui State Layout ✅ Passed PR does not violate SwiftUI state layout rules. Changes are in AppKit views (GhosttyNSView) and test files only. No new @Observable, @Published, or other SwiftUI state constructs added.
Cmux Architecture Rethink ✅ Passed No timing repairs, new state, duplicate entrypoints, or split lifecycle. Single decision path via shouldRouteKeyToZhuyinCandidateInsteadOfTerminal with standard AppKit callback.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR adds no new standalone windows. Source code changes are IME key routing in existing methods. Test files use standard test-only fixtures allowed by the rule.
Description check ✅ Passed PR description is comprehensive and follows the template structure with Summary, Testing, Checklist sections, plus extensive implementation context and verification details.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-3691-zhuyin-candidate-arrows

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented May 11, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes #3691 by intercepting plain Down/Up arrow keys during active Apple Zhuyin composition (com.apple.inputmethod.TCIM.Zhuyin) and routing them through AppKit text input instead of forwarding to the terminal; if Down reaches AppKit but leaves composition state unchanged, a synthetic plain Space event is replayed via NSTextInputContext.handleEvent to trigger Zhuyin's candidate-list expansion.

  • New Zhuyin routing branch in keyDown: six-gate predicate (marked text, exact Zhuyin source ID, no significant modifiers, Down/Up only) routes the arrow through interpretKeyEvents([event]) and captures the resulting command selector via a new commandObservedDuringTextInterpretation side channel; a separate predicate gates the Space replay on moveDown: + unchanged composition state.
  • requestZhuyinCandidateExpansion: synthesizes a Space key event and delivers it to NSTextInputContext.handleEvent; falls back to interpretKeyEvents if the input context is nil and exposes a #if DEBUG hook for unit testing.
  • Regression test suite: 14 new tests guard non-Zhuyin sources (Korean, Japanese, Pinyin, Cangjie, Latin/ABC, Zhuyin-outside-composition) and cover Zhuyin-specific paths including committed-text pass-through, modified arrows, and candidate-expansion gating.

Confidence Score: 5/5

Safe to merge; the Zhuyin routing is tightly scoped to a single input source in active composition, all other IME paths and the normal terminal forwarding path are unchanged.

The change adds six independent guard conditions before any key is diverted, none of which affect non-Zhuyin input sources or keys outside Down/Up during active composition. The regression test suite explicitly covers Korean, Japanese, Pinyin, Cangjie, and Latin/ABC sources to confirm they are unaffected. The two minor issues found do not affect correctness under normal use.

The keyDown branch in GhosttyTerminalView.swift around the new handledByZhuyinCandidateContext block is the most structurally complex addition and worth a close read during review.

Important Files Changed

Filename Overview
Sources/GhosttyNSView+IMEComposition.swift Adds two narrowly-gated helpers for Zhuyin candidate routing; appleTraditionalZhuyinInputSourceId correctly uses a private static let. Both predicates are pure and easy to unit-test.
Sources/GhosttyTerminalView.swift Introduces the Zhuyin keyDown branch: captures commandObservedDuringTextInterpretation as a per-event side channel, routes Down/Up through AppKit text input, and replays a synthetic Space to NSTextInputContext when composition is unchanged. Two minor issues: the side channel records only the last selector, and the synthetic Space inherits isARepeat from the trigger event.
cmuxTests/CJKIMEMarkedSelectionTests.swift Adds comprehensive regression guards for Korean, Japanese, Pinyin, Cangjie, and Latin input sources, plus four Zhuyin-specific tests covering Down/Up suppression, committed-text pass-through, modified arrows, and candidate-expansion gating. Test structure is thorough and well-scoped.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[keyDown: Down or Up event] --> B{markedText active?}
    B -- No --> C[Normal path: interpretKeyEvents with translationEvent]
    B -- Yes --> D{Exact Apple Zhuyin source\nplain modifiers\nDown or Up?}
    D -- No --> C
    D -- Yes --> E[interpretKeyEvents with original event\ncapture commandSelector via doCommand]
    E --> F{accumulatedText non-empty?\ni.e. IME committed text}
    F -- Yes --> G[Fall through to normal\naccumulated-text send path]
    F -- No --> H{shouldExpandZhuyinCandidates?\nDown + moveDown: + unchanged composition}
    H -- Yes --> I[requestZhuyinCandidateExpansion\nNSTextInputContext.handleEvent Space]
    H -- No --> J[syncPreedit + early return\nkey consumed by AppKit]
    I --> J
    C --> K[keyboard layout change check]
    K --> L[syncPreedit]
    L --> M{shouldSuppressGhosttyKeyForwarding?}
    M -- Yes --> N[return: IME consumed event]
    M -- No --> O[Send key event to Ghostty terminal]
Loading

Reviews (10): Last reviewed commit: "Prevent wrong-event Zhuyin expansion fal..." | Re-trigger Greptile

Comment on lines +5 to +7
private var appleTraditionalZhuyinInputSourceId: String {
"com.apple.inputmethod.TCIM.Zhuyin"
}

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 The appleTraditionalZhuyinInputSourceId property is a computed var, so it allocates a fresh String on every access. isAppleTraditionalZhuyinInputSource calls it on each invocation, and the value never changes. Promoting this to a private static let makes the intent — a compile-time constant — explicit and avoids the repeated allocation.

Suggested change
private var appleTraditionalZhuyinInputSourceId: String {
"com.apple.inputmethod.TCIM.Zhuyin"
}
private static let appleTraditionalZhuyinInputSourceId = "com.apple.inputmethod.TCIM.Zhuyin"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 737a272 by changing the Zhuyin source identifier to a private static let.

— Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/GhosttyTerminalView.swift`:
- Around line 7517-7520: The call to
shouldRouteKeyToZhuyinCandidateInsteadOfTerminal currently passes
translationEvent (whose modifiers may be rewritten by
ghostty_surface_key_translation_mods) causing modified arrows to be
misclassified; change the call to pass the original key event (the unmodified
event variable in scope) instead of translationEvent so the Zhuyin “plain arrow”
gate uses original modifiers; keep the rest of the logic and only swap the
argument to the original event to avoid suppressing legitimate forwarded keys.
🪄 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: a18ce62f-417d-4d80-bf97-5242f427b647

📥 Commits

Reviewing files that changed from the base of the PR and between d9e27e0 and a7c982f.

📒 Files selected for processing (3)
  • Sources/GhosttyNSView+IMEComposition.swift
  • Sources/GhosttyTerminalView.swift
  • cmuxTests/CJKIMEMarkedSelectionTests.swift

Comment thread Sources/GhosttyTerminalView.swift Outdated
Comment thread Sources/GhosttyTerminalView.swift Outdated
@austinywang
austinywang force-pushed the issue-3691-zhuyin-candidate-arrows branch from a7c982f to efd9507 Compare May 11, 2026 09:31
@austinywang
austinywang force-pushed the issue-3691-zhuyin-candidate-arrows branch from efd9507 to 737a272 Compare May 11, 2026 09:38

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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`:
- Around line 85-97: The test's terminalNavigationKeyCases uses empty text which
causes the production keyDown early-return (via ghostty_surface_key) and skips
Zhuyin routing; update ForwardedKeyCase entries in terminalNavigationKeyCases so
their text fields contain the real NSEvent function-key Unicode scalars for
navigation keys (i.e., use the NSEvent.charactersIgnoringModifiers values rather
than ""), for example set Up to the function-key scalar (e.g., "\u{F700}") and
similarly populate Left, Right, Down, PageUp, PageDown, Home, End with their
corresponding function-key scalars while keeping Space as " ", so the synthetic
events exercise the same keyDown path that reaches the Zhuyin routing logic.
🪄 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: 23d15c1d-9a1e-41db-890a-658a34f83f8a

📥 Commits

Reviewing files that changed from the base of the PR and between a7c982f and efd9507.

📒 Files selected for processing (4)
  • Sources/GhosttyNSView+IMEComposition.swift
  • Sources/GhosttyTerminalView.swift
  • cmuxTests/CJKIMEInputTests.swift
  • cmuxTests/CJKIMEMarkedSelectionTests.swift

Comment thread cmuxTests/CJKIMEMarkedSelectionTests.swift
Comment thread Sources/GhosttyTerminalView.swift Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmuxTests/CJKIMEMarkedSelectionTests.swift (1)

59-97: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add .numericPad modifier to arrow key fixtures only.

Arrow keys (Left, Right, Up, Down) should include .numericPad in their synthetic events per AppKit behavior. However, Home/End/PageUp/PageDown do not carry .numericPad per Apple's official API—only arrow keys are distinguished. The current fixtures omit .numericPad for all navigation keys, so tests will not catch regressions in the production "normalized modifier flags" logic that strips .numericPad.

Apply the fix to arrow keys only:

  • Left, Right, Up, Down: add modifiers: [.numericPad]
  • PageUp, PageDown, Home, End: keep as-is (no .numericPad)
  • Space: keep as-is
  • Lines 142-145, 212-216: add modifiers: [.numericPad] when calling keyEvent() for arrow keys
  • Lines 294-299: change modifiers: [.shift] to modifiers: [.shift, .numericPad] for shifted arrow tests
🤖 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 `@cmuxTests/CJKIMEMarkedSelectionTests.swift` around lines 59 - 97, The
arrow-key fixtures need the .numericPad modifier added so synthetic events match
AppKit: update the terminalNavigationKeyCases entries for "Left", "Right", "Up",
and "Down" (the ForwardedKeyCase instances used by terminalNavigationKeyCases)
to generate key events with modifiers: [.numericPad] when calling keyEvent(...),
leave "PageUp", "PageDown", "Home", "End", and "Space" unchanged, and also
update any tests that call keyEvent(...) for shifted arrow key scenarios (the
shifted-arrow cases) to use modifiers: [.shift, .numericPad] instead of just
[.shift]; locate changes around usages of keyEvent(...) and the
terminalNavigationKeyCases array.
🤖 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.

Outside diff comments:
In `@cmuxTests/CJKIMEMarkedSelectionTests.swift`:
- Around line 59-97: The arrow-key fixtures need the .numericPad modifier added
so synthetic events match AppKit: update the terminalNavigationKeyCases entries
for "Left", "Right", "Up", and "Down" (the ForwardedKeyCase instances used by
terminalNavigationKeyCases) to generate key events with modifiers: [.numericPad]
when calling keyEvent(...), leave "PageUp", "PageDown", "Home", "End", and
"Space" unchanged, and also update any tests that call keyEvent(...) for shifted
arrow key scenarios (the shifted-arrow cases) to use modifiers: [.shift,
.numericPad] instead of just [.shift]; locate changes around usages of
keyEvent(...) and the terminalNavigationKeyCases array.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2a51012b-2439-442c-a59d-a83c1ec74330

📥 Commits

Reviewing files that changed from the base of the PR and between 737a272 and c5833bc.

📒 Files selected for processing (4)
  • Sources/GhosttyNSView+IMEComposition.swift
  • Sources/GhosttyTerminalView.swift
  • cmuxTests/CJKIMEInputTests.swift
  • cmuxTests/CJKIMEMarkedSelectionTests.swift

Comment thread cmuxTests/CJKIMEMarkedSelectionTests.swift
Comment thread cmuxTests/CJKIMEMarkedSelectionTests.swift
Apple Zhuyin reports plain Down during marked text as moveDown: without changing composition, so simply consuming the arrow or asking AppKit's app-candidate panel does not show the IME candidate menu. Keep the #3691 intercept exact, then replay the IME candidate expansion key into NSTextInputContext only for that unchanged Down case.

This also keeps the Zhuyin candidate-arrow test cleanup symmetrical by restoring the debug expansion handler after the commit-from-interpretKeyEvents regression case.

Constraint: #3691 scope is exact Apple Zhuyin, active marked text, plain Down/Up only; Up remains consume-only for candidate navigation

Rejected: private AppKit candidate presentation selectors | dogfood still showed no Zhuyin candidate menu

Rejected: broad IME routing/keyUp state/window reroute | reverted in #3849 after breaking other IMEs

Confidence: medium

Scope-risk: narrow

Directive: Do not widen this beyond Apple Zhuyin marked-text Down without per-IME shell-forwarding regression guards and manual IME dogfood

Tested: git diff --check

Tested: ./scripts/test-unit.sh test -only-testing:cmuxTests/CJKIMEMarkedSelectionTests

Tested: ./scripts/reload.sh --tag issue-3691-zhuyin-candidate-arrows

Not-tested: latest local manual IME dogfood; user/maintainer should verify Apple Zhuyin Down opens the candidate window in the tagged app
@austinywang
austinywang force-pushed the issue-3691-zhuyin-candidate-arrows branch from 2c9695e to 18bf468 Compare May 11, 2026 22:06
@austinywang

Copy link
Copy Markdown
Contributor Author

Addressed the latest Cursor Bugbot finding in amended commit 18bf468ec: testZhuyinCandidateArrowCommitFromInterpretKeyEventsReachesShell now saves/restores GhosttyNSView.debugZhuyinCandidateExpansionEventHandler, matching the cleanup pattern in the surrounding helpers.\n\nPreserved the required two-commit shape by amending the implementation commit rather than adding a third commit.\n\nLocal validation after the patch:\n- git diff --check\n- ./scripts/reload.sh --tag issue-3691-zhuyin-candidate-arrows\n\n*— Codex*

The PR branch was behind main after CI review. Merging origin/main brings the current app, test, and workflow changes into this branch before final CI validation, without intentionally changing the Zhuyin candidate-arrow implementation beyond the clean auto-merge.

Constraint: iterate-pr workflow requires validating the PR against the latest base branch

Confidence: medium

Scope-risk: moderate

Directive: Treat this as a base-sync merge only; do not use it to infer new Zhuyin behavior changes

Tested: git diff --check HEAD^..HEAD

Not-tested: Local unit/UI tests skipped per user instruction; post-push CI will validate
Synthetic navigation events already use AppKit function-key characters. Add the matching numericPad modifier for arrow keys only so the Zhuyin regression tests exercise the same modifier normalization path as real arrow-key NSEvents.

Constraint: CodeRabbit review noted arrow keys carry numericPad while PageUp, PageDown, Home, End, and Space do not

Confidence: high

Scope-risk: narrow

Directive: Keep these fixtures aligned with real NSEvent shape; do not add numericPad to non-arrow navigation keys without AppKit evidence

Tested: git diff --check

Not-tested: Local unit/UI tests skipped per user instruction; CI will validate after push

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c7c66ce. Configure here.

Comment thread Sources/GhosttyTerminalView.swift
The candidate-expansion path should only send the synthetic Space event to AppKit. If NSEvent cannot create that event, return false instead of replaying the original Down arrow and accidentally asking the IME to navigate twice.

Constraint: Cursor Bugbot flagged the original-event fallback on the current PR head

Rejected: Keep the original Down fallback | it silently changes expansion failure into a second navigation event

Confidence: high

Scope-risk: narrow

Directive: Candidate expansion must fail closed; do not substitute terminal or arrow events for the synthetic Space request

Tested: git diff --check

Not-tested: Local unit/UI tests skipped per user instruction; CI will validate after push
@austinywang

Copy link
Copy Markdown
Contributor Author

Current head 22da5256e resolves the remaining stale review-summary findings I can verify:

  • Sources/GhosttyTerminalView.swift now passes the original event into shouldRouteKeyToZhuyinCandidateInsteadOfTerminal at the routing gate, so rewritten translationEvent modifiers are not used for the plain-arrow check.
  • cmuxTests/CJKIMEMarkedSelectionTests.swift now gives navigation-key test events their AppKit function-key Unicode scalars (U+F700..U+F72D) instead of empty text.
  • zhuyinCandidateExpansionEvent now returns nil on synthetic Space event creation failure, and requestZhuyinCandidateExpansion fails closed instead of replaying the original Down arrow.

The high-priority entries still reported by the feedback helper are old CodeRabbit summary comments without current inline unresolved threads; the corresponding inline findings are resolved/outdated on the latest head.

@austinywang

Copy link
Copy Markdown
Contributor Author

Closing — issue is already fixed. See #3691.

@austinywang
austinywang deleted the issue-3691-zhuyin-candidate-arrows branch May 12, 2026 03:14

This branch was successfully deployed

1 active deployment
Preview – cmux — 22da5256 Deployed May 12, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chinese IME (Zhuyin/Bopomofo): arrow keys cannot navigate candidate window in terminal

1 participant