fix: detect Shift+Enter on macOS terminals without Kitty protocol support#2969
fix: detect Shift+Enter on macOS terminals without Kitty protocol support#2969gtardif wants to merge 1 commit into
Conversation
…port macOS Terminal.app (and other legacy terminals) don't support the Kitty keyboard protocol, so Shift+Enter sends the same byte as plain Enter. This makes it impossible to distinguish the two at the terminal protocol level. Use the macOS CoreGraphics API (CGEventSourceFlagsState) to query the system-level keyboard modifier state when a plain Enter key event is received. If Shift is physically held, treat it as a newline insertion instead of submitting the message. This approach works universally across all macOS terminal emulators without requiring any terminal configuration. For terminals that do support the Kitty protocol (VSCode, iTerm2 3.5+, Ghostty), the existing shift+enter detection continues to work via the protocol, and the CoreGraphics fallback is never reached.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR introduces macOS CoreGraphics-based Shift key detection to handle Shift+Enter in terminals that don't support the Kitty keyboard protocol. The build tag split (darwin / !darwin) is correct and exhaustive. The CGo constants (kCGEventSourceStateCombinedSessionState, kCGEventFlagMaskShift) are appropriate. Kitty-capable terminals (iTerm2, Ghostty, VSCode) are not affected since they send shift+enter as a distinct key event.
Two LIKELY issues found in the added code — see inline comments.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
3 finding(s): 1 HIGH (LIKELY), 1 MEDIUM (LIKELY), 1 LOW (LIKELY). See inline comments for details.
| @@ -0,0 +1,23 @@ | |||
| //go:build darwin | |||
There was a problem hiding this comment.
[HIGH] CGo dependency breaks darwin cross-compilation from Linux CI
modifiers_darwin.go introduces a CGo dependency (#cgo LDFLAGS: -framework CoreGraphics + import "C"), which requires a native darwin C toolchain at build time. The project's task cross command builds darwin/amd64 and darwin/arm64 targets — if that task runs on a Linux CI host (typical for cross-platform builds), the darwin CGo target will fail to compile because:
- A darwin cross-compiler (e.g. osxcross) is not a standard Go CI setup, and
- Forcing
CGO_ENABLED=0for darwin would also break the build since theimport "C"cannot be excluded.
Impact: CI pipelines doing cross-compilation for darwin will fail. Docker's own multi-platform build (task cross) is likely affected.
Suggestion: Consider adding a build tag guard or a //go:build ignore variant so that the CGo implementation is only compiled natively on a darwin host, and document that darwin binaries must be built natively. Alternatively, restructure to allow CGO_ENABLED=0 fallback (where IsShiftPressed() returns false, falling back to the ctrl+j path).
| // protocol, so Shift+Enter sends the same byte as Enter. Use the macOS | ||
| // CoreGraphics API to check if Shift is physically held, and treat it | ||
| // as a newline insertion. | ||
| if msg.String() == "enter" && termfeatures.IsShiftPressed() { |
There was a problem hiding this comment.
[MEDIUM] Timing race: Shift key may be released before IsShiftPressed() is polled
CGEventSourceFlagsState is called at the moment bubbletea dispatches the key event to the Update function — not at the exact moment the physical key was pressed. If the user releases Shift before the Go event loop processes the message (possible under goroutine scheduling delays or system load), IsShiftPressed() returns false, and the Enter is treated as a plain Enter — submitting the message instead of inserting a newline.
Impact: Intermittent false negatives: the user presses Shift+Enter intending to insert a newline, but the message is submitted unexpectedly. This is a regression compared to terminals that use the Kitty protocol (where the key event carries shift modifier flags, immune to timing).
Suggestion: The PR description acknowledges this is a best-effort approach, but this limitation should be documented (e.g., a code comment or a note in the PR). Consider whether the timing window is acceptable in practice (bubbletea dispatch is typically sub-millisecond), and if so, document it explicitly to set expectations.
| // CoreGraphics API to check if Shift is physically held, and treat it | ||
| // as a newline insertion. | ||
| if msg.String() == "enter" && termfeatures.IsShiftPressed() { | ||
| e.textarea.InsertString("\n") |
There was a problem hiding this comment.
[LOW] InsertString("\n") vs InsertNewline may behave differently
The Kitty-protocol path inserts a newline by passing the key event through e.textarea.Update(msg), which dispatches the InsertNewline key binding internally. The new macOS path directly calls e.textarea.InsertString("\n").
For the current textarea implementation these are likely equivalent, but if the bubbletea textarea widget ever adds smart-indentation, auto-pairing, or other InsertNewline-specific logic, the two paths will diverge silently.
Suggestion: If a direct InsertNewline API is available on the textarea, prefer calling it for consistency with the Kitty-protocol path.
macOS Terminal.app (and other legacy terminals) don't support the Kitty keyboard protocol, so Shift+Enter sends the same byte as plain Enter. This makes it impossible to distinguish the two at the terminal protocol level.
Use the macOS CoreGraphics API (CGEventSourceFlagsState) to query the system-level keyboard modifier state when a plain Enter key event is received. If Shift is physically held, treat it as a newline insertion instead of submitting the message.
This approach works universally across all macOS terminal emulators without requiring any terminal configuration. For terminals that do support the Kitty protocol (VSCode, iTerm2 3.5+, Ghostty), the existing shift+enter detection continues to work via the protocol, and the CoreGraphics fallback is never reached.