Repository navigation
Conversation
|
@lejahmie is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
Author
|
@codex review |
@lejahmie I have started the AI code review. It will take a few minutes to complete. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds explicit right-Option state tracking and UI-test hooks in the view layer, centralizes modifier translation/consumption (including side-specific Option detection), consolidates key dispatch via Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant View as GhosttyNSView
participant Translator as ModifierTranslator
participant Sender as GhosttyInputSender
participant Core as TerminalCore
rect rgba(200,200,255,0.5)
User->>View: press/release key (incl. left/right Option)
end
View->>View: flagsChanged(with: event) -> update rightOptionModifierDown / fallback toggle
View->>Translator: translatedModifierFlags(from: flags, fallback:)
Translator->>Translator: consumed = consumedModsFromFlags(flags, sourceEvent: event)
View->>Sender: sendGhosttyKey(keyEvent, translatedMods, consumed)
Sender->>Core: deliver ghostty_input_key_s to terminal core
Core-->>User: terminal receives input / characters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Contributor
Greptile SummaryThis PR fixes right-Option (
Confidence Score: 5/5Safe to merge — production logic is correct; all remaining findings are P2 style and test-coverage improvements The core right-Option detection and modifier-propagation logic is sound: NX_DEVICERALTKEYMASK is the established macOS API for this purpose, the toggle heuristic correctly handles press/release edges under normal event delivery, and translatedModifierFlags() removes duplication without altering behaviour. No P0/P1 defects were found. The three P2 issues are non-blocking. cmuxTests/CJKIMEInputTests.swift — the literal-character test does not cover the rightOptionModifierDown fallback path it implies, and the delete test is missing consumed_mods and text assertions Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
FC[flagsChanged event] --> KC{keyCode?}
KC -->|kVK_RightOption| RO_BIT{hasRightOptionBit?}
KC -->|kVK_Option| LO_BIT{hasRightOptionBit?}
KC -->|other| OD3{optionDown?}
RO_BIT -->|yes| SET_TRUE[rightOptionModifierDown = true]
RO_BIT -->|no| OD1{optionDown?}
OD1 -->|no| SET_FALSE1[rightOptionModifierDown = false]
OD1 -->|yes| TOGGLE[rightOptionModifierDown.toggle]
LO_BIT -->|yes| SET_TRUE2[rightOptionModifierDown = true]
LO_BIT -->|no| OD2{optionDown?}
OD2 -->|no| SET_FALSE2[rightOptionModifierDown = false]
OD2 -->|yes| NOOP[no change]
OD3 -->|no| SET_FALSE3[rightOptionModifierDown = false]
OD3 -->|yes| NOOP2[no change]
SET_TRUE --> MODS
SET_FALSE1 --> MODS
TOGGLE --> MODS
SET_TRUE2 --> MODS
SET_FALSE2 --> MODS
NOOP --> MODS
SET_FALSE3 --> MODS
NOOP2 --> MODS
MODS[modsFromEvent / isRightOptionActive] --> IRA{detection path?}
IRA -->|NX_DEVICERALTKEYMASK bit set| ALT_RIGHT[GHOSTTY_MODS_ALT_RIGHT]
IRA -->|keyCode == kVK_RightOption| ALT_RIGHT
IRA -->|keyCode == kVK_Option| ALT_ONLY[GHOSTTY_MODS_ALT only]
IRA -->|fallback: rightOptionModifierDown| ALT_RIGHT
IRA -->|else| ALT_ONLY
Reviews (1): Last reviewed commit: "Add support for right Option modifier in..." | 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
6171-6200:⚠️ Potential issue | 🟡 MinorRecord the fallback right-Option state before checking
surface.Line 6172 exits before
rightOptionModifierDownis updated. SinceensureSurfaceReadyForInput()can create or reattach the surface on the followingkeyDown, the first right-Option chord after startup or reparent can still lose its side information on layouts that only report generic.option.Suggested fix
override func flagsChanged(with event: NSEvent) { - guard let surface = surface else { - super.flagsChanged(with: event) - return - } - let optionDown = event.modifierFlags.contains(.option) let hasRightOptionBit = event.modifierFlags.contains(Self.rightOptionModifierFlag) switch Int(event.keyCode) { case Int(kVK_RightOption): if hasRightOptionBit { @@ if !optionDown { rightOptionModifierDown = false } } + + guard let surface = surface else { + super.flagsChanged(with: event) + return + } var keyEvent = ghostty_input_key_s()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 6171 - 6200, Compute and store the inferred right-Option state before early-returning when surface is nil: in flagsChanged(with:) evaluate optionDown and hasRightOptionBit and derive the fallback right-side value (the same inference logic you use later for Int(kVK_RightOption)/Int(kVK_Option)/default) into a local variable and assign it to rightOptionModifierDown before the guard that checks surface so the first right-Option chord after startup/reparent retains side information; keep the existing per-key handling after the guard (and still call super.flagsChanged(with:) when returning), and ensure this change works with ensureSurfaceReadyForInput() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CJKIMEInputTests.swift`:
- Around line 1293-1330: The test sets a sticky override via
hostedView.surfaceView.debugSetRightOptionModifierDownForUITest(true) but resets
it later in the function, which can be skipped on early returns and leak state;
wrap the reset in a defer immediately after enabling the override (i.e., call
defer { hostedView.surfaceView.debugSetRightOptionModifierDownForUITest(false)
}) so the override is always cleared even on failures, and keep the existing
debugGhosttySurfaceKeyEventObserver and capturedPress logic unchanged.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6235-6292: consumedModsFromFlags currently only sets
GHOSTTY_MODS_ALT when flags contains .option, which can drop
GHOSTTY_MODS_ALT_RIGHT; change it to always compute rightOptionActive (using
isRightOptionActive) and set GHOSTTY_MODS_ALT_RIGHT when rightOptionActive is
true, but only set GHOSTTY_MODS_ALT when flags.contains(.option) (keep existing
behavior for ALT bit); also update isRightOptionActive(event:flags:) to treat a
right-option keyUp correctly by checking event.type (return false for keyUp when
flags no longer contain .option and Int(event.keyCode) == kVK_RightOption) so a
right-option release doesn't leave rightOptionModifierDown implied.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6171-6200: Compute and store the inferred right-Option state
before early-returning when surface is nil: in flagsChanged(with:) evaluate
optionDown and hasRightOptionBit and derive the fallback right-side value (the
same inference logic you use later for
Int(kVK_RightOption)/Int(kVK_Option)/default) into a local variable and assign
it to rightOptionModifierDown before the guard that checks surface so the first
right-Option chord after startup/reparent retains side information; keep the
existing per-key handling after the guard (and still call
super.flagsChanged(with:) when returning), and ensure this change works with
ensureSurfaceReadyForInput() behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b16e956-1729-4f20-9340-87d6d0ddb139
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
…s for non-printing delete behavior
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
6235-6294:⚠️ Potential issue | 🟠 Major
ALT_RIGHTstill falls out of the translated path.Line 6290 can preserve
rightOptionModifierFlageven when.optionis cleared, but Line 6264 returns before that bit is checked. That meansconsumedModsFromFlags(..., sourceEvent:)can still emitNONEfor right-Option literal input, and Line 6266 still makes akVK_RightOptionkeyUp look right-active if left Option remains held.💡 Proposed fix
private func isRightOptionActive(event: NSEvent, flags: NSEvent.ModifierFlags) -> Bool { - if event.type == .keyUp, - Int(event.keyCode) == Int(kVK_RightOption), - !flags.contains(.option) { + if event.type == .keyUp, + Int(event.keyCode) == Int(kVK_RightOption) { return false } - guard flags.contains(.option) else { return false } if flags.contains(Self.rightOptionModifierFlag) { return true } + guard flags.contains(.option) else { return false } if Int(event.keyCode) == Int(kVK_RightOption) { return true } if Int(event.keyCode) == Int(kVK_Option) { return false } return rightOptionModifierDown }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 6235 - 6294, consumedModsFromFlags and isRightOptionActive currently let the absence of .option short-circuit detection of a physically active right-Option, causing GHOSTTY_MODS_ALT_RIGHT to be lost; update isRightOptionActive (used by consumedModsFromFlags) to remove the early guard "guard flags.contains(.option) else { return false }" and instead only keep the existing keyUp special-case (the if checking event.type == .keyUp && keyCode == kVK_RightOption && !flags.contains(.option) return false), then evaluate flags.contains(Self.rightOptionModifierFlag), Int(event.keyCode) == kVK_RightOption, Int(event.keyCode) == kVK_Option (return false), and finally fallback to rightOptionModifierDown so that GHOSTTY_MODS_ALT_RIGHT can be emitted even when .option is not present in flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6235-6294: consumedModsFromFlags and isRightOptionActive currently
let the absence of .option short-circuit detection of a physically active
right-Option, causing GHOSTTY_MODS_ALT_RIGHT to be lost; update
isRightOptionActive (used by consumedModsFromFlags) to remove the early guard
"guard flags.contains(.option) else { return false }" and instead only keep the
existing keyUp special-case (the if checking event.type == .keyUp && keyCode ==
kVK_RightOption && !flags.contains(.option) return false), then evaluate
flags.contains(Self.rightOptionModifierFlag), Int(event.keyCode) ==
kVK_RightOption, Int(event.keyCode) == kVK_Option (return false), and finally
fallback to rightOptionModifierDown so that GHOSTTY_MODS_ALT_RIGHT can be
emitted even when .option is not present in flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1df96d5e-f844-4abb-9a6c-09211d0a64e8
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/CJKIMEInputTests.swift (1)
1298-1300: Consider renamingrightOptionOnlyfor clarity.The variable is assigned
[.option]without the device-specific right flag. The "right-Option" semantics come fromdebugSetRightOptionModifierDownForUITest(true)which sets the internal fallback state. This mirrors the real-world scenario where macOS doesn't reliably report right-Option in modifier flags for some layouts/OS paths.Renaming to
optionOnlyor adding a brief comment would reduce confusion:-let rightOptionOnly = NSEvent.ModifierFlags([.option]) +// .option only; right-Option semantics injected via debugSetRightOptionModifierDownForUITest +let optionOnly = NSEvent.ModifierFlags([.option]) hostedView.debugSetRightOptionModifierDownForUITest(true) defer { hostedView.debugSetRightOptionModifierDownForUITest(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CJKIMEInputTests.swift` around lines 1298 - 1300, The variable name rightOptionOnly is misleading because it is constructed as NSEvent.ModifierFlags([.option]) (not the device-specific right flag) while the "right option" behavior is produced by calling hostedView.debugSetRightOptionModifierDownForUITest(true); rename rightOptionOnly to optionOnly (or similar) and/or add a one-line comment near the declaration explaining that the flags use .option and that debugSetRightOptionModifierDownForUITest(true) provides the right-Option semantics; update any references to rightOptionOnly accordingly and keep the defer call to debugSetRightOptionModifierDownForUITest(false) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/CJKIMEInputTests.swift`:
- Around line 1298-1300: The variable name rightOptionOnly is misleading because
it is constructed as NSEvent.ModifierFlags([.option]) (not the device-specific
right flag) while the "right option" behavior is produced by calling
hostedView.debugSetRightOptionModifierDownForUITest(true); rename
rightOptionOnly to optionOnly (or similar) and/or add a one-line comment near
the declaration explaining that the flags use .option and that
debugSetRightOptionModifierDownForUITest(true) provides the right-Option
semantics; update any references to rightOptionOnly accordingly and keep the
defer call to debugSetRightOptionModifierDownForUITest(false) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73677fd4-ae80-467d-8b89-47a611a7bd3f
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CJKIMEInputTests.swift`:
- Around line 1325-1335: The test currently asserts only capturedPress.mods;
also assert that capturedPress.consumed_mods contains the GHOSTTY_MODS_ALT_RIGHT
bit so printable right-Option input preserves the AltRight consumption. Locate
the assertions around capturedPress.mods in CJKIMEInputTests (the test method
using capturedPress) and add an XCTAssertEqual or XCTAssertTrue checking
(capturedPress.consumed_mods.rawValue & GHOSTTY_MODS_ALT_RIGHT.rawValue) ==
GHOSTTY_MODS_ALT_RIGHT.rawValue to ensure consumed_mods carries the AltRight
bit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b1c855d-4d24-45f1-a07e-35ea33baaa88
📒 Files selected for processing (1)
cmuxTests/CJKIMEInputTests.swift
Author
|
All automated PR review feedback adressed. Manual tests done, unit tests pass. |
![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).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:6191">
P2: Right‑Option fallback can get stuck true when right Option is released while left Option remains held, because the one‑shot toggle is never re‑armed or cleared on that release. This can misclassify later left‑Option shortcuts as right‑Option.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
6178-6211:⚠️ Potential issue | 🟠 MajorRight-Option fallback still desynchronizes side state.
Line 6191 only handles the ambiguous
kVK_RightOptiontransition once, so releasing right Option while left Option stays down never clearsrightOptionModifierDown. Line 6280 then returns before checkingrightOptionModifierFlag, so a translated right-only flag set can still loseALT_RIGHT. That leavesmods/consumed_modswrong in exactly the side-specific cases this PR is fixing.🛠️ Proposed fix
switch Int(event.keyCode) { case Int(kVK_RightOption): if hasRightOptionBit { inferredRightOptionDown = true inferredFallbackToggleArmed = true } else if !optionDown { inferredRightOptionDown = false inferredFallbackToggleArmed = true } else if inferredFallbackToggleArmed { - inferredRightOptionDown.toggle() + inferredRightOptionDown = true inferredFallbackToggleArmed = false + } else { + inferredRightOptionDown = false + inferredFallbackToggleArmed = true } case Int(kVK_Option): if hasRightOptionBit { inferredRightOptionDown = true inferredFallbackToggleArmed = true @@ private func isRightOptionActive(event: NSEvent, flags: NSEvent.ModifierFlags) -> Bool { + if flags.contains(Self.rightOptionModifierFlag) { return true } if event.type == .keyUp, Int(event.keyCode) == Int(kVK_RightOption), !flags.contains(.option) { return false } guard flags.contains(.option) else { return false } - if flags.contains(Self.rightOptionModifierFlag) { return true } - if Int(event.keyCode) == Int(kVK_RightOption) { return true } + if Int(event.keyCode) == Int(kVK_RightOption) { return rightOptionModifierDown } if Int(event.keyCode) == Int(kVK_Option) { return false } return rightOptionModifierDown }Also applies to: 6274-6313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 6178 - 6211, The right-option fallback handling leaves rightOptionModifierDown set when releasing the physical right-Option key while the left Option remains pressed; update the switch logic around Int(kVK_RightOption) and the default branch so that whenever optionDown is false (no Option bits), inferredRightOptionDown is explicitly set to false and inferredFallbackToggleArmed set to true — and ensure this state update occurs before any early return that checks modifier flags later. Concretely, adjust the kVK_RightOption case to also clear rightOptionModifierDown when hasRightOptionBit is false and optionDown is false (even if previously armed), and make the default branch mirror that same clearing behavior so rightOptionModifierDown/rightOptionFallbackToggleArmed cannot desynchronize; keep all changes confined to the variables rightOptionModifierDown, rightOptionFallbackToggleArmed, inferredRightOptionDown, and inferredFallbackToggleArmed to preserve the later modifier-flag logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6178-6211: The right-option fallback handling leaves
rightOptionModifierDown set when releasing the physical right-Option key while
the left Option remains pressed; update the switch logic around
Int(kVK_RightOption) and the default branch so that whenever optionDown is false
(no Option bits), inferredRightOptionDown is explicitly set to false and
inferredFallbackToggleArmed set to true — and ensure this state update occurs
before any early return that checks modifier flags later. Concretely, adjust the
kVK_RightOption case to also clear rightOptionModifierDown when
hasRightOptionBit is false and optionDown is false (even if previously armed),
and make the default branch mirror that same clearing behavior so
rightOptionModifierDown/rightOptionFallbackToggleArmed cannot desynchronize;
keep all changes confined to the variables rightOptionModifierDown,
rightOptionFallbackToggleArmed, inferredRightOptionDown, and
inferredFallbackToggleArmed to preserve the later modifier-flag logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44bcee52-bab0-4d4f-98d2-625f721c8443
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
6288-6313:⚠️ Potential issue | 🟠 Major
consumed_modscan still missALT_RIGHTwhen translation strips.option.Line 6290 currently checks right-option activity against the translated
flagsvalue. If Ghostty translation returns right-side intent without.option,isRightOptionActive(...)can evaluate false and dropGHOSTTY_MODS_ALT_RIGHTinconsumed_mods.Suggested fix
private func consumedModsFromFlags( _ flags: NSEvent.ModifierFlags, sourceEvent: NSEvent? = nil ) -> ghostty_input_mods_e { var mods = GHOSTTY_MODS_NONE.rawValue @@ let rightOptionActive: Bool if let sourceEvent { - rightOptionActive = isRightOptionActive(event: sourceEvent, flags: flags) + // Use raw source-event flags for side detection; translation flags may + // intentionally drop `.option` while preserving right-side intent. + rightOptionActive = isRightOptionActive(event: sourceEvent, flags: sourceEvent.modifierFlags) } else { rightOptionActive = flags.contains(Self.rightOptionModifierFlag) || rightOptionModifierDown } @@ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 6288 - 6313, The consumed_mods path can drop GHOSTTY_MODS_ALT_RIGHT because isRightOptionActive(...) checks the translated flags parameter (which may have .option stripped) instead of the event's original modifier flags; update the logic to use the NSEvent's original modifier flags when testing for presence of .option so right-side ALT intent isn't lost. Concretely, in isRightOptionActive(event:flags:) replace checks that test flags.contains(.option) with event.modifierFlags.contains(.option) (or change the call to pass event.modifierFlags), and keep the other checks for Self.rightOptionModifierFlag, kVK_RightOption/kVK_Option keyCode, and rightOptionModifierDown so GHOSTTY_MODS_ALT_RIGHT is set reliably when computing consumed_mods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6288-6313: The consumed_mods path can drop GHOSTTY_MODS_ALT_RIGHT
because isRightOptionActive(...) checks the translated flags parameter (which
may have .option stripped) instead of the event's original modifier flags;
update the logic to use the NSEvent's original modifier flags when testing for
presence of .option so right-side ALT intent isn't lost. Concretely, in
isRightOptionActive(event:flags:) replace checks that test
flags.contains(.option) with event.modifierFlags.contains(.option) (or change
the call to pass event.modifierFlags), and keep the other checks for
Self.rightOptionModifierFlag, kVK_RightOption/kVK_Option keyCode, and
rightOptionModifierDown so GHOSTTY_MODS_ALT_RIGHT is set reliably when computing
consumed_mods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8764374-baf1-4287-9e40-38034892137f
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/CJKIMEInputTests.swift
Svanmark
approved these changes
Mar 31, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
7159-7169:⚠️ Potential issue | 🟠 MajorRight-Option keyUp still reports
ALT_RIGHTwhen left Option stays down.
Line 7167re-marks anykVK_RightOptionevent as right-side active, so afterflagsChanged(with:)clearsrightOptionModifierDownfor the⌥R up while ⌥L remains heldfallback case,modsFromEvent(_:)still emitsALT_RIGHTon the release event. That leaves the side-specific modifier state wrong for the exact transition this patch is trying to fix.Suggested fix
private func isRightOptionActive(event: NSEvent, flags: NSEvent.ModifierFlags) -> Bool { - if event.type == .keyUp, - Int(event.keyCode) == Int(kVK_RightOption), - !flags.contains(.option) { + if event.type == .keyUp, + Int(event.keyCode) == Int(kVK_RightOption) { return false } guard flags.contains(.option) else { return false } if flags.contains(Self.rightOptionModifierFlag) { return true } - if Int(event.keyCode) == Int(kVK_RightOption) { return true } if Int(event.keyCode) == Int(kVK_Option) { return false } return rightOptionModifierDown }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 7159 - 7169, The isRightOptionActive(_:flags:) helper wrongly treats any event with event.keyCode == kVK_RightOption as right-side active even on keyUp when the left Option remains held; update the logic so that for keyUp events with keyCode == kVK_RightOption you do not return true unconditionally but instead consult the rightOptionModifierDown state (or explicitly return false when rightOptionModifierDown is false) before marking right-side active. In short, inside isRightOptionActive check event.type == .keyUp && Int(event.keyCode) == Int(kVK_RightOption) and return rightOptionModifierDown (or false) rather than falling through to the unconditional `if Int(event.keyCode) == Int(kVK_RightOption) { return true }` branch so the release of ⌥R while ⌥L is held does not emit ALT_RIGHT.
🧹 Nitpick comments (2)
cmuxTests/CJKIMEInputTests.swift (2)
1833-1839: Minor: Character doesn't match key code for Option+D.Line 1834 uses
characters: "˚"withkeyCode: kVK_ANSI_D, but "˚" is produced by Option+K on a US keyboard, not Option+D (which produces "∂" as used in line 1751). This doesn't affect test correctness since the test validates modifier flags, but for consistency:Suggested fix
let sent = hostedView.debugSendSyntheticKeyPressAndReleaseForUITest( - characters: "˚", + characters: "∂", charactersIgnoringModifiers: "d", keyCode: UInt16(kVK_ANSI_D), modifierFlags: [.option] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CJKIMEInputTests.swift` around lines 1833 - 1839, The test uses hostedView.debugSendSyntheticKeyPressAndReleaseForUITest with characters: "˚" but keyCode: UInt16(kVK_ANSI_D) and modifierFlags: [.option]; update the characters argument to match the character produced by Option+D on a US keyboard (use "∂") so the characters value is consistent with the keyCode kVK_ANSI_D and modifierFlags in the call to debugSendSyntheticKeyPressAndReleaseForUITest.
1736-1738: Misleading variable name:rightOptionOnlycontains no right-Option indicator.The variable
rightOptionOnlyis set to[.option]without the device-specific right-Option bit. The right-Option state is actually injected viadebugSetRightOptionModifierDownForUITest(true). Consider renaming for clarity:Suggested rename
- let rightOptionOnly = NSEvent.ModifierFlags([.option]) - hostedView.debugSetRightOptionModifierDownForUITest(true) + let optionFlagsWithoutDeviceBit = NSEvent.ModifierFlags([.option]) + hostedView.debugSetRightOptionModifierDownForUITest(true)Then update the reference at line 1754:
- modifierFlags: rightOptionOnly + modifierFlags: optionFlagsWithoutDeviceBit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CJKIMEInputTests.swift` around lines 1736 - 1738, Rename the misleading variable rightOptionOnly (an NSEvent.ModifierFlags([.option]) value) to something that does not imply device-specific right-Option (e.g., optionFlagOnly or optionModifier) and update its usage; note the right-Option device bit is actually set by hostedView.debugSetRightOptionModifierDownForUITest(true), so leave that call as-is but change the variable name wherever referenced (search for rightOptionOnly and replace with the new name) to make intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 7159-7169: The isRightOptionActive(_:flags:) helper wrongly treats
any event with event.keyCode == kVK_RightOption as right-side active even on
keyUp when the left Option remains held; update the logic so that for keyUp
events with keyCode == kVK_RightOption you do not return true unconditionally
but instead consult the rightOptionModifierDown state (or explicitly return
false when rightOptionModifierDown is false) before marking right-side active.
In short, inside isRightOptionActive check event.type == .keyUp &&
Int(event.keyCode) == Int(kVK_RightOption) and return rightOptionModifierDown
(or false) rather than falling through to the unconditional `if
Int(event.keyCode) == Int(kVK_RightOption) { return true }` branch so the
release of ⌥R while ⌥L is held does not emit ALT_RIGHT.
---
Nitpick comments:
In `@cmuxTests/CJKIMEInputTests.swift`:
- Around line 1833-1839: The test uses
hostedView.debugSendSyntheticKeyPressAndReleaseForUITest with characters: "˚"
but keyCode: UInt16(kVK_ANSI_D) and modifierFlags: [.option]; update the
characters argument to match the character produced by Option+D on a US keyboard
(use "∂") so the characters value is consistent with the keyCode kVK_ANSI_D and
modifierFlags in the call to debugSendSyntheticKeyPressAndReleaseForUITest.
- Around line 1736-1738: Rename the misleading variable rightOptionOnly (an
NSEvent.ModifierFlags([.option]) value) to something that does not imply
device-specific right-Option (e.g., optionFlagOnly or optionModifier) and update
its usage; note the right-Option device bit is actually set by
hostedView.debugSetRightOptionModifierDownForUITest(true), so leave that call
as-is but change the variable name wherever referenced (search for
rightOptionOnly and replace with the new name) to make intent clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a319927-e1a7-4ee7-81cc-471383098d93
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
|
Is there anything missing here? This is currently preventing me to fully migrate from ghostty to cmux. Thanks for your work! |
Author
I have been waiting for weeks, and have tried to reach out to the maintainers for weeks now. I guess they are busy people. 😢 |
Author
@JannikWempe fuck it, built my own from scratch.... you are welcome to beta test it :-D |
|
Bumping this because #2369 makes it tricky to use cmux for most things. This PR would be quite helpful. I'm game if there's anything I can do to help getting it merged. |
Collaborator
|
Shipped on main, including side-specific Option handling and regression coverage: 558d6b9. |
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
5 participants
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
right|left|true|false/unset behaves correctly.
layouts/OS paths where raw right-Option modifier bits are unreliable.
behavior (Option+Delete and right-Option literal input).
Why:
Fix: #2369
to produce layout text (∂) instead of Meta (^[d) under macos-option-as-alt = right.
Testing
fix-right-option-alt
26.2, ISO International keyboard):
Review Trigger (Copy/Paste as PR comment)
@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review
Checklist
equivalent)
Summary by cubic
Fixes bug 2369 by reliably handling the right Option key on macOS so
macos-option-as-alt=rightworks and Meta combos behave (e.g., ⌥R+d → ^[d). Adds side-aware detection, a robust fallback, unified key dispatch, and debug hooks to simulate right‑Option release while left is held.Bug Fixes
NX_DEVICERALTKEYMASK; fallback infers usingflagsChangedandkVK_RightOptionwith a one‑shot toggle.ALT_RIGHTin rawmods; include it inconsumed_modsonly for literal input. Non‑printing keys (e.g., Option+Delete) consume none. Left Option never setsALT_RIGHT.Refactors
translatedModifierFlags(...),consumedModsFromFlags(...), andisRightOptionActive(...); propagateAltRightinto AppKit flags.sendGhosttyKey.GhosttyNSView/GhosttySurfaceScrollViewto set right‑Option state and simulateflagsChanged.Written for commit dd5ae09. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Debug