cmux-next: keyboard copy mode (Toggle Copy Mode, ⇧⌘M) - #16450
Conversation
Port the old app's vim-style copy mode to the cmux-next terminal. The key table moves to a pure CmuxNextCopyMode target with its tests; the terminal view drives Ghostty's keyboard-copy API (cursor, selection, viewport, bounded clipboard copy), draws the cursor box and the "vim" badge, and takes every key except Command chords until Esc, q, or a copy. `/` opens the same find prompt as ⌘F through a new `find` host action. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Review fixes for the copy mode port: - Resolve h/j/k/l and the other command keys through the ASCII-capable layout, as the old app did, so they work under Korean or Russian input. - Discard an unfinished IME composition on entering copy mode. - The action test now targets a tab shown in a window, which is the only way the handler resolves a terminal. The live-surface suite reports itself skipped where ghostty_surface_new cannot succeed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView.swift:197">
P2: The cursor box remains stale when cell metrics change without a frame resize, such as a font-size or backing-scale change during copy mode. Resynchronize it after every successful `updateSurfaceSize()` path, ideally from that method itself.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift:150">
P2: `resolve` bypasses the modifier guard for uppercase `Y` and `G`, so non-Command chords such as Option+Shift can copy or jump instead of being swallowed. Apply the same empty-or-Shift guard used by `action`.</violation>
<violation number="2" location="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift:211">
P2: Caps Lock plus Shift is incorrectly classified as uppercase, so Shift+V/Y/G/N can run uppercase commands instead of their lowercase behavior. Check the raw Caps Lock flag before treating Shift as uppercase.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Handlers/TerminalHandlers.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Handlers/TerminalHandlers.swift:88">
P3: The refusal message exposes an internal C API name: `ghosttyRejected("keyboard_copy_cursor_set")` renders as "Ghostty rejected keyboard_copy_cursor_set", but that is the function name `ghostty_surface_keyboard_copy_cursor_set`, not a config key or action the user can touch. Every other `ghosttyRejected` call in this file passes a user-facing Ghostty binding action (e.g. `scroll_page_up`). When entering copy mode fails here (unsupported or not-yet-ready surface), the toast gives the user nothing actionable. Use a refusal that says copy mode isn't available on that surface, or names the config that enables it.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeAction.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeAction.swift:22">
P2: TerminalSurfaceView+CopyMode's `performCopyMode` runs `.scrollToTop`/`.scrollToBottom` through `moveCopyModeCursor(.home/.end)`, i.e. `ghostty_surface_keyboard_selection_move` with `GHOSTTY_KEYBOARD_SELECTION_MOVE_HOME/END` — a cursor move, not a viewport/scrollback scroll. If Ghostty's HOME/END moves are screen-relative (as in Ghostty's own copy mode, where scroll-to-top/bottom are separate scroll actions), `gg`/`G` will only reach the visible screen edges and never the ends of a long scrollback, contradicting this doc. The available scroll kinds are only LINES/PAGES/HALF_PAGES/PROMPTS. Verify the move semantics against ghostty.h; if screen-relative, add a scroll-to-absolute path or document the viewport-relative behavior.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView+Keyboard.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView+Keyboard.swift:83">
P3: `handleCopyModeKeyUp` swallows a key-up whenever its key-code is in `copyModeConsumedKeyUps`, and that set is only drained by the matching key-up. The pairing breaks when copy mode is toggled off mid-hold: a key-down consumed by copy mode then gets repeat key-downs routed to Ghostty (copy mode is nil, so `handleCopyModeKeyDown` returns false) while its release is still swallowed, so Ghostty receives `GHOSTTY_ACTION_REPEAT` presses with no press and no release and the terminal can emit phantom repeated characters. Likewise, a key-up that is only genuinely missed (e.g., window lost between down and up) leaves the key-code in the set past `exitCopyMode`, so the next normal-mode press of that key sends press to Ghostty but has its release swallowed, leaving a stuck/auto-repeating key until the same key is pressed again. Bound the swallow to keys actually taken in the current session or drain the set when leaving copy mode (keeping only the release of the key that triggered the exit).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| public override func setFrameSize(_ newSize: NSSize) { | ||
| super.setFrameSize(newSize) | ||
| updateSurfaceSize() | ||
| syncCopyModeCursor() |
There was a problem hiding this comment.
P2: The cursor box remains stale when cell metrics change without a frame resize, such as a font-size or backing-scale change during copy mode. Resynchronize it after every successful updateSurfaceSize() path, ideally from that method itself.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView.swift, line 197:
<comment>The cursor box remains stale when cell metrics change without a frame resize, such as a font-size or backing-scale change during copy mode. Resynchronize it after every successful `updateSurfaceSize()` path, ideally from that method itself.</comment>
<file context>
@@ -188,6 +194,7 @@ public final class TerminalSurfaceView: NSView {
public override func setFrameSize(_ newSize: NSSize) {
super.setFrameSize(newSize)
updateSurfaceSize()
+ syncCopyModeCursor()
}
</file context>
| chars = first | ||
| } | ||
| lowercased = chars.lowercased() | ||
| if modifiers == [.shift] { |
There was a problem hiding this comment.
P2: Caps Lock plus Shift is incorrectly classified as uppercase, so Shift+V/Y/G/N can run uppercase commands instead of their lowercase behavior. Check the raw Caps Lock flag before treating Shift as uppercase.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift, line 211:
<comment>Caps Lock plus Shift is incorrectly classified as uppercase, so Shift+V/Y/G/N can run uppercase commands instead of their lowercase behavior. Check the raw Caps Lock flag before treating Shift as uppercase.</comment>
<file context>
@@ -0,0 +1,222 @@
+ chars = first
+ }
+ lowercased = chars.lowercased()
+ if modifiers == [.shift] {
+ isUppercase = true
+ } else if raw.contains(.capsLock) {
</file context>
| if !hasSelection, lower == "y", key.isUppercase { return perform(.copyLineAndExit) } | ||
| if lower == "g", key.isUppercase { return perform(hasSelection ? .adjustSelection(.end) : .scrollToBottom) } |
There was a problem hiding this comment.
P2: resolve bypasses the modifier guard for uppercase Y and G, so non-Command chords such as Option+Shift can copy or jump instead of being swallowed. Apply the same empty-or-Shift guard used by action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift, line 150:
<comment>`resolve` bypasses the modifier guard for uppercase `Y` and `G`, so non-Command chords such as Option+Shift can copy or jump instead of being swallowed. Apply the same empty-or-Shift guard used by `action`.</comment>
<file context>
@@ -0,0 +1,222 @@
+ return .consume
+ }
+ }
+ if !hasSelection, lower == "y", key.isUppercase { return perform(.copyLineAndExit) }
+ if lower == "g", key.isUppercase { return perform(hasSelection ? .adjustSelection(.end) : .scrollToBottom) }
+ if !hasSelection, lower == "y", mods.isEmpty {
</file context>
| if !hasSelection, lower == "y", key.isUppercase { return perform(.copyLineAndExit) } | |
| if lower == "g", key.isUppercase { return perform(hasSelection ? .adjustSelection(.end) : .scrollToBottom) } | |
| if !hasSelection, lower == "y", key.isUppercase, mods.isEmpty || mods == [.shift] { return perform(.copyLineAndExit) } | |
| if lower == "g", key.isUppercase, mods.isEmpty || mods == [.shift] { return perform(hasSelection ? .adjustSelection(.end) : .scrollToBottom) } |
| case scrollPage(Int) | ||
| /// Scrolls the viewport by signed half pages (Ctrl-U, Ctrl-D). | ||
| case scrollHalfPage(Int) | ||
| /// Moves to the top-left cell of the scrollback (`gg`, Home). |
There was a problem hiding this comment.
P2: TerminalSurfaceView+CopyMode's performCopyMode runs .scrollToTop/.scrollToBottom through moveCopyModeCursor(.home/.end), i.e. ghostty_surface_keyboard_selection_move with GHOSTTY_KEYBOARD_SELECTION_MOVE_HOME/END — a cursor move, not a viewport/scrollback scroll. If Ghostty's HOME/END moves are screen-relative (as in Ghostty's own copy mode, where scroll-to-top/bottom are separate scroll actions), gg/G will only reach the visible screen edges and never the ends of a long scrollback, contradicting this doc. The available scroll kinds are only LINES/PAGES/HALF_PAGES/PROMPTS. Verify the move semantics against ghostty.h; if screen-relative, add a scroll-to-absolute path or document the viewport-relative behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeAction.swift, line 22:
<comment>TerminalSurfaceView+CopyMode's `performCopyMode` runs `.scrollToTop`/`.scrollToBottom` through `moveCopyModeCursor(.home/.end)`, i.e. `ghostty_surface_keyboard_selection_move` with `GHOSTTY_KEYBOARD_SELECTION_MOVE_HOME/END` — a cursor move, not a viewport/scrollback scroll. If Ghostty's HOME/END moves are screen-relative (as in Ghostty's own copy mode, where scroll-to-top/bottom are separate scroll actions), `gg`/`G` will only reach the visible screen edges and never the ends of a long scrollback, contradicting this doc. The available scroll kinds are only LINES/PAGES/HALF_PAGES/PROMPTS. Verify the move semantics against ghostty.h; if screen-relative, add a scroll-to-absolute path or document the viewport-relative behavior.</comment>
<file context>
@@ -0,0 +1,59 @@
+ case scrollPage(Int)
+ /// Scrolls the viewport by signed half pages (Ctrl-U, Ctrl-D).
+ case scrollHalfPage(Int)
+ /// Moves to the top-left cell of the scrollback (`gg`, Home).
+ case scrollToTop
+ /// Moves to the bottom-right cell (`G`, End).
</file context>
| private static func bindCopyMode(_ registry: ActionRegistry, _ ctx: AppActionContext) { | ||
| registry.bind("toggleTerminalCopyMode", invoke: { invocation in | ||
| guard let entry = ctx.terminal(invocation) else { return } | ||
| if !entry.session.surfaceView.toggleCopyMode() { ctx.refuse(RefusalStrings.ghosttyRejected("keyboard_copy_cursor_set")) } |
There was a problem hiding this comment.
P3: The refusal message exposes an internal C API name: ghosttyRejected("keyboard_copy_cursor_set") renders as "Ghostty rejected keyboard_copy_cursor_set", but that is the function name ghostty_surface_keyboard_copy_cursor_set, not a config key or action the user can touch. Every other ghosttyRejected call in this file passes a user-facing Ghostty binding action (e.g. scroll_page_up). When entering copy mode fails here (unsupported or not-yet-ready surface), the toast gives the user nothing actionable. Use a refusal that says copy mode isn't available on that surface, or names the config that enables it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextApp/Handlers/TerminalHandlers.swift, line 88:
<comment>The refusal message exposes an internal C API name: `ghosttyRejected("keyboard_copy_cursor_set")` renders as "Ghostty rejected keyboard_copy_cursor_set", but that is the function name `ghostty_surface_keyboard_copy_cursor_set`, not a config key or action the user can touch. Every other `ghosttyRejected` call in this file passes a user-facing Ghostty binding action (e.g. `scroll_page_up`). When entering copy mode fails here (unsupported or not-yet-ready surface), the toast gives the user nothing actionable. Use a refusal that says copy mode isn't available on that surface, or names the config that enables it.</comment>
<file context>
@@ -79,8 +80,16 @@ enum TerminalHandlers {
+ private static func bindCopyMode(_ registry: ActionRegistry, _ ctx: AppActionContext) {
+ registry.bind("toggleTerminalCopyMode", invoke: { invocation in
+ guard let entry = ctx.terminal(invocation) else { return }
+ if !entry.session.surfaceView.toggleCopyMode() { ctx.refuse(RefusalStrings.ghosttyRejected("keyboard_copy_cursor_set")) }
+ })
+ }
</file context>
| } | ||
|
|
||
| public override func keyUp(with event: NSEvent) { | ||
| if handleCopyModeKeyUp(event) { return } |
There was a problem hiding this comment.
P3: handleCopyModeKeyUp swallows a key-up whenever its key-code is in copyModeConsumedKeyUps, and that set is only drained by the matching key-up. The pairing breaks when copy mode is toggled off mid-hold: a key-down consumed by copy mode then gets repeat key-downs routed to Ghostty (copy mode is nil, so handleCopyModeKeyDown returns false) while its release is still swallowed, so Ghostty receives GHOSTTY_ACTION_REPEAT presses with no press and no release and the terminal can emit phantom repeated characters. Likewise, a key-up that is only genuinely missed (e.g., window lost between down and up) leaves the key-code in the set past exitCopyMode, so the next normal-mode press of that key sends press to Ghostty but has its release swallowed, leaving a stuck/auto-repeating key until the same key is pressed again. Bound the swallow to keys actually taken in the current session or drain the set when leaving copy mode (keeping only the release of the key that triggered the exit).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView+Keyboard.swift, line 83:
<comment>`handleCopyModeKeyUp` swallows a key-up whenever its key-code is in `copyModeConsumedKeyUps`, and that set is only drained by the matching key-up. The pairing breaks when copy mode is toggled off mid-hold: a key-down consumed by copy mode then gets repeat key-downs routed to Ghostty (copy mode is nil, so `handleCopyModeKeyDown` returns false) while its release is still swallowed, so Ghostty receives `GHOSTTY_ACTION_REPEAT` presses with no press and no release and the terminal can emit phantom repeated characters. Likewise, a key-up that is only genuinely missed (e.g., window lost between down and up) leaves the key-code in the set past `exitCopyMode`, so the next normal-mode press of that key sends press to Ghostty but has its release swallowed, leaving a stuck/auto-repeating key until the same key is pressed again. Bound the swallow to keys actually taken in the current session or drain the set when leaving copy mode (keeping only the release of the key that triggered the exit).</comment>
<file context>
@@ -77,6 +80,7 @@ extension TerminalSurfaceView {
}
public override func keyUp(with event: NSEvent) {
+ if handleCopyModeKeyUp(event) { return }
sendKey(GHOSTTY_ACTION_RELEASE, event: event)
}
</file context>
There was a problem hiding this comment.
3 existing issues remain and 4 new issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView+CopyMode.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView+CopyMode.swift:130">
P2: Counted `gg`/`G` commands lose their count here: `3gg` and `3G` always perform a single absolute jump. Preserve the count in these actions and implement the corresponding counted jump semantics.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalCopyModeBadge.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalCopyModeBadge.swift:41">
P3: The badge is exposed as its own accessibility element (`setAccessibilityElement(true)` + `setAccessibilityRole(.staticText)` + label "vim") while its two subviews remain separate accessibility elements: the `NSImageView` and the "vim" `NSTextField`. VoiceOver will announce the badge's "vim" label, the image, and the label's "vim" text again, producing duplicated or garbled output. Mark the subviews as non-elements so the badge reads as a single "vim" static text.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeAction.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeAction.swift:58">
P3: The `consume` case documentation says it only covers pending-state updates, but `CopyModeKeys.resolve` returns `.consume` for swallowed ordinary keys too (after `state.reset()`). Clarify that `consume` means the key was taken with nothing to execute, including unrecognized keys, so the case isn't mistaken for a promise that pending state changed.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift:85">
P2: The `^` command does not work on the standard macOS layout. Handle the unshifted `6` representation of Shift+6, just as the adjacent `$` handling handles `4`.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
Re-trigger cubic
| case .jumpToPrompt(let delta): | ||
| copyModeScroll(GHOSTTY_KEYBOARD_COPY_SCROLL_PROMPTS, delta * count, surface: surface) | ||
| case .scrollToTop: | ||
| moveCopyModeCursor(.home, count: 1, surface: surface) |
There was a problem hiding this comment.
P2: Counted gg/G commands lose their count here: 3gg and 3G always perform a single absolute jump. Preserve the count in these actions and implement the corresponding counted jump semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalSurfaceView+CopyMode.swift, line 130:
<comment>Counted `gg`/`G` commands lose their count here: `3gg` and `3G` always perform a single absolute jump. Preserve the count in these actions and implement the corresponding counted jump semantics.</comment>
<file context>
@@ -0,0 +1,236 @@
+ case .jumpToPrompt(let delta):
+ copyModeScroll(GHOSTTY_KEYBOARD_COPY_SCROLL_PROMPTS, delta * count, surface: surface)
+ case .scrollToTop:
+ moveCopyModeCursor(.home, count: 1, surface: surface)
+ case .scrollToBottom:
+ moveCopyModeCursor(.end, count: 1, surface: surface)
</file context>
| case "g": | ||
| guard key.isUppercase else { return nil } | ||
| return hasSelection ? .adjustSelection(.end) : .scrollToBottom | ||
| case "0", "^": |
There was a problem hiding this comment.
P2: The ^ command does not work on the standard macOS layout. Handle the unshifted 6 representation of Shift+6, just as the adjacent $ handling handles 4.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeKeys.swift, line 85:
<comment>The `^` command does not work on the standard macOS layout. Handle the unshifted `6` representation of Shift+6, just as the adjacent `$` handling handles `4`.</comment>
<file context>
@@ -0,0 +1,222 @@
+ case "g":
+ guard key.isUppercase else { return nil }
+ return hasSelection ? .adjustSelection(.end) : .scrollToBottom
+ case "0", "^":
+ return .adjustSelection(.beginningOfLine)
+ case "$", "4":
</file context>
| setAccessibilityElement(true) | ||
| setAccessibilityRole(.staticText) | ||
| setAccessibilityLabel(Self.text) |
There was a problem hiding this comment.
P3: The badge is exposed as its own accessibility element (setAccessibilityElement(true) + setAccessibilityRole(.staticText) + label "vim") while its two subviews remain separate accessibility elements: the NSImageView and the "vim" NSTextField. VoiceOver will announce the badge's "vim" label, the image, and the label's "vim" text again, producing duplicated or garbled output. Mark the subviews as non-elements so the badge reads as a single "vim" static text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextTerminal/TerminalCopyModeBadge.swift, line 41:
<comment>The badge is exposed as its own accessibility element (`setAccessibilityElement(true)` + `setAccessibilityRole(.staticText)` + label "vim") while its two subviews remain separate accessibility elements: the `NSImageView` and the "vim" `NSTextField`. VoiceOver will announce the badge's "vim" label, the image, and the label's "vim" text again, producing duplicated or garbled output. Mark the subviews as non-elements so the badge reads as a single "vim" static text.</comment>
<file context>
@@ -0,0 +1,52 @@
+ label.topAnchor.constraint(equalTo: topAnchor, constant: 6),
+ label.bottomAnchor.constraint(equalTo: bottomAnchor, constant: -6),
+ ])
+ setAccessibilityElement(true)
+ setAccessibilityRole(.staticText)
+ setAccessibilityLabel(Self.text)
</file context>
| setAccessibilityElement(true) | |
| setAccessibilityRole(.staticText) | |
| setAccessibilityLabel(Self.text) | |
| icon.setAccessibilityElement(false) | |
| label.setAccessibilityElement(false) | |
| setAccessibilityElement(true) | |
| setAccessibilityRole(.staticText) | |
| setAccessibilityLabel(Self.text) |
| /// update pending state (a count prefix, the first `g` or `y`). | ||
| public enum CopyModeResolution: Equatable, Sendable { | ||
| case perform(CopyModeAction, count: Int) | ||
| case consume |
There was a problem hiding this comment.
P3: The consume case documentation says it only covers pending-state updates, but CopyModeKeys.resolve returns .consume for swallowed ordinary keys too (after state.reset()). Clarify that consume means the key was taken with nothing to execute, including unrecognized keys, so the case isn't mistaken for a promise that pending state changed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextCopyMode/CopyModeAction.swift, line 58:
<comment>The `consume` case documentation says it only covers pending-state updates, but `CopyModeKeys.resolve` returns `.consume` for swallowed ordinary keys too (after `state.reset()`). Clarify that `consume` means the key was taken with nothing to execute, including unrecognized keys, so the case isn't mistaken for a promise that pending state changed.</comment>
<file context>
@@ -0,0 +1,59 @@
+/// update pending state (a count prefix, the first `g` or `y`).
+public enum CopyModeResolution: Equatable, Sendable {
+ case perform(CopyModeAction, count: Int)
+ case consume
+}
</file context>
The suite's .enabled trait named the suite itself, which the Suite macro
cannot resolve ("circular reference resolving attached macro 'Suite'").
The probe now lives in CopyModeLiveSurface. The action test also requires
the window to show the pane before it acts.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent subagent review (read-only, no build): Head 3686ccb: fix first. The key resolver is a faithful port. Findings:
Fixes in b904219:
Re-review of b904219: ship. The provider matches the old one. It has no cost outside copy mode. CI then found a compile error the review missed: the suite's trait named the suite itself ( Still open:
|
There was a problem hiding this comment.
1 issue found across 1 file (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. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/TerminalCopyModeTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/TerminalCopyModeTests.swift:102">
P3: Every failure inside `make()` collapses into `nil`: a `BridgeTreeFixture.tree()` decode error returns nil exactly like a missing tab or an unavailable surface, so `try #require(CopyModeLiveSurface.make())` in `terminal()` only reports "returned nil" instead of the underlying JSON/decoding error that the old `try BridgeTreeFixture.tree()` propagated, and a fixture failure disables the whole suite under the misleading "needs a live Ghostty surface" reason. Make `make()` throw for fixture/service failures and let `available()` use `try? make()` so only surface availability drives the skip and test failures surface the real cause.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| enum CopyModeLiveSurface { | ||
| static func make() -> (services: AppServices, tab: TabModel, view: TerminalSurfaceView)? { | ||
| let services = ActionBindingCoverageTests.boundServices() | ||
| guard let tree = try? BridgeTreeFixture.tree() else { return nil } |
There was a problem hiding this comment.
P3: Every failure inside make() collapses into nil: a BridgeTreeFixture.tree() decode error returns nil exactly like a missing tab or an unavailable surface, so try #require(CopyModeLiveSurface.make()) in terminal() only reports "returned nil" instead of the underlying JSON/decoding error that the old try BridgeTreeFixture.tree() propagated, and a fixture failure disables the whole suite under the misleading "needs a live Ghostty surface" reason. Make make() throw for fixture/service failures and let available() use try? make() so only surface availability drives the skip and test failures surface the real cause.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/TerminalCopyModeTests.swift, line 102:
<comment>Every failure inside `make()` collapses into `nil`: a `BridgeTreeFixture.tree()` decode error returns nil exactly like a missing tab or an unavailable surface, so `try #require(CopyModeLiveSurface.make())` in `terminal()` only reports "returned nil" instead of the underlying JSON/decoding error that the old `try BridgeTreeFixture.tree()` propagated, and a fixture failure disables the whole suite under the misleading "needs a live Ghostty surface" reason. Make `make()` throw for fixture/service failures and let `available()` use `try? make()` so only surface availability drives the skip and test failures surface the real cause.</comment>
<file context>
@@ -98,3 +92,20 @@ struct TerminalCopyModeTests {
+enum CopyModeLiveSurface {
+ static func make() -> (services: AppServices, tab: TabModel, view: TerminalSurfaceView)? {
+ let services = ActionBindingCoverageTests.boundServices()
+ guard let tree = try? BridgeTreeFixture.tree() else { return nil }
+ services.daemon.store.apply(snapshot: tree)
+ guard let tab = services.daemon.store.workspaces.first?.screens.first?.panes.first?.tabs.first else { return nil }
</file context>
package-conventions-lint rejects an all-static public enum. CopyModeKeys is now a struct built with the ASCII layout lookup it needs, and action and resolve are instance methods. Count clamping and the shortcut bypass stay static. No behavior change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent subagent review of b904219..f816959 (read-only): ship.
CI at f816959 is all green:
Merging per the coordinator: parity items approved; feat-cmux-next is dogfooded as a whole. |
1 participant
Part of the cmux-next parity work listed in #16174 (comment): #16174 (comment)
Problem
Toggle Copy Mode (⇧⌘M) was registered as unavailable in cmux-next ("needs a keyboard copy mode in the cmux-next terminal"). The old app has a vim-style copy mode over the scrollback.
Behavior
Same keys as the old app:
While copy mode is on, a box marks the cursor cell and a "vim" pill sits in the terminal's top-right corner, as in the old app.
How
CmuxNextCopyMode(new, pure, tested): the key table and count/gg/yystate, ported from the oldCmuxTerminalCore/CopyModeresolver. It also has the cursor box geometry.TerminalSurfaceView+CopyMode: drives Ghostty's keyboard-copy API from the cmux fork (ghostty_surface_keyboard_copy_*,keyboard_selection_move,copy_selection_to_clipboard_bounded; the pinned ghostty already exports them). The old app used the same calls. Ghostty owns the cursor, selection and viewport, so the old Swift cursor and visual-line models are not ported.keyDowngives copy mode the key first, andkeyUpswallows the matching release. The cursor box resyncs on every key, scrollbar update and resize.TerminalHostAction.findroutes to the registry'sfindaction, so/uses the same path as ⌘F.toggleTerminalCopyModeis bound inTerminalHandlers. ThecopyModeUnportedrefusal and its strings are removed.inventory.md: the CmuxTerminalCore row said copy mode would be deleted in favor of "cmux-tui copy mode", but cmux-tui has none. The row now points at the new code.Not ported: the old soft-wrap re-join after copy (Ghostty's clipboard formatter unwraps soft wraps itself) and the old rendered-frame demand that kept the cursor box in sync on every frame. Here it resyncs on keys, scrollbar updates and resizes.
Tests
CmuxNextCopyModeTests: the old resolver suite's cases (non-ASCII layouts, Caps Lock, line bounds, pendingg/y,V/Yvariants), plus counts, clamping, scroll keys, prompts, search, exit, the Command bypass and the cursor box geometry.TerminalCopyModeTests(app): the action is bound and toggles copy mode on its terminal. Plain keys stay in copy mode,qand Esc exit, Esc's key-up is swallowed, and Command chords pass through and reset the count./routes tofind.Localization
One new string,
terminal.copyMode.indicator("vim", the old app's indicator text, in every locale the terminal catalog has). One string removed,handlers.refusal.copyModeUnported. A failed entry reuses the existingghosttyRejectedrefusal.Evidence
This is a UI change. cmux-next has no CI screenshot path yet (session cc-next-ci-media is building one), so there are no screenshots here yet; they will be added from the CI tour once it exists.
Credit: the original copy mode is by Lawrence Chen (#792), with follow-ups by Austin Wang. Both are staff, so there is no Co-authored-by line.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds vim-style keyboard copy mode (⇧⌘M) to the cmux-next terminal, replacing the "unavailable" refusal and restoring parity with the old app's copy mode over the scrollback.
Behavior
/opens the same find prompt as ⌘F,{/}jump between prompts, Esc or q exits.Implementation
CmuxNextCopyModetarget holds the key table (a struct carrying its layout lookup) and cursor-box geometry, ported from the old resolver, with its own test suite.TerminalSurfaceView+CopyModedrives Ghostty's keyboard-copy API, which owns the cursor, selection, and viewport, so the old Swift cursor/selection models weren't ported./routes through a newfindhost action, reusing the ⌘F prompt; live-surface tests self-skip where no Ghostty surface can be created, and the binding test acts through a windowed tab.terminal.copyMode.indicatorstring; removes thecopyModeUnportedrefusal.Written for commit f816959. Summary will update on new commits.