feat: add Leader Key mode and Workspace Tags#1854
Open
xiaocang wants to merge 1 commit intomanaflow-ai:mainfrom
Open
feat: add Leader Key mode and Workspace Tags#1854xiaocang wants to merge 1 commit intomanaflow-ai:mainfrom
xiaocang wants to merge 1 commit intomanaflow-ai:mainfrom
Conversation
|
@xiaocang is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a tmux-style leader-key feature: persistent settings and recorder UI, two-step leader handling in AppDelegate with timeout/ESC, TabManager leader-mode state, workspace tagging (model, persistence, v2 RPC), pane-focus helpers, keyboard-shortcut addition, localized strings, and a new Swift source registered in the Xcode project. Changes
Sequence DiagramsequenceDiagram
participant User
participant Monitor as NSEventMonitor
participant App as AppDelegate
participant TM as TabManager
participant UI as Settings/ContentView
participant TC as TerminalController
participant WS as Workspace
User->>Monitor: press leader key (Ctrl+B)
Monitor->>App: handleKeyDown (leader detected)
App->>TM: set isLeaderModeActive = true
App->>App: start timeout (DispatchWorkItem)
TM->>UI: publish leader-mode state (show LEADER indicator)
User->>Monitor: press second key
Monitor->>App: handleKeyDown (waitingForSecondKey)
App->>App: resolve LeaderAction from key
alt action == workspace.set_tag
App->>UI: show tag prompt
UI->>App: submit tag
App->>TC: v2 workspace.set_tag RPC
TC->>WS: ws.setTag(tag)
WS->>TM: publish tag change
TM->>UI: update displayTitle
else navigation/split/focus/tab-select/copy-mode
App->>WS: perform action (split/focus/tab/select/copy-mode)
WS->>TM: publish state changes
TM->>UI: update views
end
alt ESC or timeout
App->>App: cancel leader mode
App->>TM: set isLeaderModeActive = false
TM->>UI: hide LEADER indicator
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
xiaocang
force-pushed
the
feat/leader-key-workspace-tags
branch
from
350b477 to
a78557c
Compare
Contributor
Greptile SummaryThis PR adds two complementary features to cmux: a tmux-style Leader Key mode (a configurable prefix chord — default Ctrl+B — that activates a one-shot command mode for split, focus, tab, and workspace actions) and Workspace Tags (short labels displayed as Key observations:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate as AppDelegate (event monitor)
participant TabManager
participant Workspace
participant ContentView
User->>AppDelegate: Press leader key (Ctrl+B)
AppDelegate->>AppDelegate: leaderKeyState = .waitingForSecondKey
AppDelegate->>TabManager: isLeaderModeActive = true
TabManager-->>ContentView: objectWillChange → show LEADER indicator
AppDelegate->>AppDelegate: startLeaderKeyTimer(timeout)
alt Timeout fires
AppDelegate->>AppDelegate: cancelLeaderMode()
AppDelegate->>TabManager: isLeaderModeActive = false
TabManager-->>ContentView: objectWillChange → hide LEADER indicator
else User presses sub-key (e.g. ",")
User->>AppDelegate: Key event
AppDelegate->>AppDelegate: cancelLeaderMode() → hide indicator
AppDelegate->>AppDelegate: executeLeaderAction(event)
AppDelegate->>Workspace: setTag(input) via beginSetWorkspaceTagFlow()
Workspace->>Workspace: tag = trimmed value (@Published fires)
Workspace-->>ContentView: objectWillChange → displayTitle updated
else User presses ESC
User->>AppDelegate: ESC key
AppDelegate->>AppDelegate: cancelLeaderMode()
AppDelegate->>TabManager: isLeaderModeActive = false
TabManager-->>ContentView: objectWillChange → hide LEADER indicator
else Double-press leader key
User->>AppDelegate: Press leader key again
AppDelegate->>AppDelegate: cancelLeaderMode()
AppDelegate-->>User: return false (pass key to terminal)
end
Last reviewed commit: "feat: add Leader Key..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
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/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:3594">
P2: workspace.set_tag clears tags when the tag parameter is omitted or invalid; missing tag should be rejected to avoid accidental tag deletion.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:8920">
P2: Leader-key handling ignores routing-context sync failure and can consume keystrokes in non-main windows.</violation>
<violation number="2" location="Sources/AppDelegate.swift:10510">
P2: Leader key mode is cleared on the current tabManager rather than the one that was activated, so switching windows between key presses can leave the original manager stuck in leader mode.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
10935-10939:⚠️ Potential issue | 🟠 MajorKeep VoiceOver in sync with the visible tagged title.
The row now renders
tab.displayTitle, butaccessibilityTitlestill announcestab.titleat Line 11518. If tags are being used to distinguish similar workspaces, screen-reader users won't hear the same identifier that sighted users see.♿ Suggested follow-up
private var accessibilityTitle: String { - String(localized: "accessibility.workspacePosition", defaultValue: "\(tab.title), workspace \(index + 1) of \(accessibilityWorkspaceCount)") + String(localized: "accessibility.workspacePosition", defaultValue: "\(tab.displayTitle), workspace \(index + 1) of \(accessibilityWorkspaceCount)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10935 - 10939, The visible row renders Text(tab.displayTitle) but the accessibility announcement still uses tab.title; update the accessibility modifier so screen readers announce the same identifier by replacing usages of accessibilityTitle (or .accessibilityLabel/.accessibilityValue if used) that reference tab.title with tab.displayTitle for the same view (look for the accessibilityTitle call near the Text(tab.displayTitle) and change the argument to tab.displayTitle).
🧹 Nitpick comments (3)
Sources/cmuxApp.swift (1)
3849-3849: Keep Workspace Tags out of the leader-key bucket.
workspaceTagsEnabledis persisted throughLeaderKeySettingsand surfaced directly under the “Leader Key” section, even though tags are an independent feature. Giving tags their own settings type/key space and section/header will keep future settings work cleaner and avoid implying a dependency on leader mode.Also applies to: 5343-5355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` at line 3849, The workspaceTagsEnabled AppStorage is wrongly housed in LeaderKeySettings; create a separate settings namespace (e.g., WorkspaceTagsSettings) with its own key constant (workspaceTagsEnabledKey) and default (workspaceTagsEnabledDefault), replace occurrences of LeaderKeySettings.workspaceTagsEnabledKey/Default and the `@AppStorage` binding for workspaceTagsEnabled to use WorkspaceTagsSettings, and move the UI and any references (including the other occurrences around the block that previously used LeaderKeySettings at lines ~5343-5355) out of the “Leader Key” section into a new “Workspace Tags” settings section/header so tags are independently persisted and displayed.Sources/TerminalController.swift (1)
3291-3304: Consider centralizing workspace payload encoding.This adds another workspace field to
workspace.list, whileworkspace.currentandsystem.treestill build separate workspace payloads. A shared encoder would reduce field drift the next time workspace metadata changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3291 - 3304, Extract the workspace payload construction into a single helper (e.g., a method like encodeWorkspacePayload or Workspace.payloadDictionary) and replace the inline map over tabManager.tabs in TerminalController (the block that builds workspaces with keys "id","ref","index","title","selected","pinned","listening_ports","remote","current_directory","custom_color","tag") so the same helper is used by workspace.list, workspace.current and system.tree; ensure the helper accepts the Workspace instance (and index/selected state when needed) and reuses v2Ref, v2OrNull and ws.remoteStatusPayload to produce a consistent dictionary output across all callers.Sources/Workspace.swift (1)
8018-8064: Prefer split-tree order overallPaneIdsfor pane navigation.These helpers call the "ordered pane list" but index
bonsplitController.allPaneIds. This file already derives visual pane order fromtreeSnapshot(), so reusing that here avoids depending on Bonsplit's ordering contract and keeps leader-pane navigation aligned with the visible layout. Please confirm Bonsplit'sallPaneIdsordering contract before leaning on it here.💡 Proposed fix
+ private func orderedPaneIdsForNavigation() -> [PaneID] { + let panesById = Dictionary( + uniqueKeysWithValues: bonsplitController.allPaneIds.map { ($0.id.uuidString, $0) } + ) + return SidebarBranchOrdering + .orderedPaneIds(tree: bonsplitController.treeSnapshot()) + .compactMap { panesById[$0] } + } + /// Cycle focus to the previous pane. func focusPreviousPane() { - let paneIds = bonsplitController.allPaneIds + let paneIds = orderedPaneIdsForNavigation() `#if` DEBUG dlog("pane.cyclePrev count=\(paneIds.count) focusedId=\(bonsplitController.focusedPaneId.map { "\($0)" } ?? "nil")") `#endif` @@ /// Cycle focus to the next pane (tmux prefix+o behavior). func focusNextPane() { - let paneIds = bonsplitController.allPaneIds + let paneIds = orderedPaneIdsForNavigation() `#if` DEBUG dlog("pane.cycleNext count=\(paneIds.count) focusedId=\(bonsplitController.focusedPaneId.map { "\($0)" } ?? "nil")") `#endif` @@ /// Focus a pane by its index in the ordered pane list. func focusPaneByIndex(_ index: Int) { - let paneIds = bonsplitController.allPaneIds + let paneIds = orderedPaneIdsForNavigation() `#if` DEBUG dlog("pane.focusByIndex index=\(index) count=\(paneIds.count) focusedId=\(bonsplitController.focusedPaneId.map { "\($0)" } ?? "nil")") `#endif`
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 57501-57534: The copy for settings.app.workspaceTags.subtitleOn
incorrectly references the "sidebar" UI; update the string value for both
settings.app.workspaceTags.subtitleOn and settings.app.workspaceTags.subtitleOff
(and their ja localizations) to reflect the actual UI surfaces ("workspace
switcher" and "tab bar") and the correct behavior (enabled vs disabled), e.g.,
change the English and Japanese "value" texts to say tags are shown in the
workspace switcher and tab bar when on, and that display/assignment are disabled
when off.
In `@Sources/AppDelegate.swift`:
- Around line 2030-2032: Currently leaderKeyState and leaderKeyWorkItem are
global to the app which causes leader mode to stick to the wrong window; change
the leader session to be window-scoped by storing the originating TabManager (or
window identifier) when entering leader mode and use that stored owner to
cancel/query state and timeouts. Specifically: replace the global
leaderKeyState/leaderKeyWorkItem semantics with a struct or tuple that pairs the
state and workItem with an owner reference (the TabManager instance that armed
the prefix), set that owner in the code path that arms leader mode, and update
all places that clear or check leader state (places interacting with
leaderKeyState, leaderKeyWorkItem, and where current tabManager is
cleared/queried) to operate only if the stored owner matches the TabManager for
the current window; ensure cancellation of the DispatchWorkItem is performed
against the stored workItem and that comparisons guard against window switches
to avoid cross-window leakage.
- Around line 8918-8949: The leader-mode code currently runs immediately after
calling synchronizeShortcutRoutingContext(event:) and can arm leader mode even
when no main terminal/window context was resolved or when modal UI
(close-confirmation, sheets, or the command palette) is active; move the entire
LeaderKeySettings.isEnabled block (including references to leaderKeyState,
matchShortcut(event:), tabManager?.isLeaderModeActive, startLeaderKeyTimer(),
cancelLeaderMode(), executeLeaderAction(event:)) so it executes only after the
existing guards that check for a resolved main terminal window and after the
command-palette/ modal-sheet/close-confirmation checks, and add an explicit
bail-out if synchronizeShortcutRoutingContext(event:) did not produce a
main-window context (do not reorder shortcut handling when the command palette
is visible).
In `@Sources/cmuxApp.swift`:
- Around line 6317-6337: The recording currently accepts any non-empty
characters; change the validation before assigning self.currentKey to reject
non-printable or whitespace and multi-character/IME strings by ensuring the
chosen recorded string is a single grapheme (e.g. recorded.count == 1 or use
recorded.index(after:) to verify a single cluster), that its first Character is
not whitespace (first.isWhitespace == false), and that its unicodeScalars do not
contain control characters (use CharacterSet.controlCharacters and
CharacterSet.whitespaces to filter). Apply this check where recorded is computed
(using shifted/unshifted) and only call self.currentKey = recorded,
self.onKeyRecorded?(recorded) and self.stopRecording() when the validation
passes; alternatively explicitly map any special keys you want to allow instead
of the general acceptance.
- Around line 6287-6294: The displayString(for:) function is returning an
escaped quote ("\\\"") which shows a backslash in the recorder button; change
the handling for the quote case in displayString(for key: String) so it returns
the actual quote character ("\"") rather than the escaped form, ensuring the
recorded key displays the glyph the user pressed; verify the "\"" case in
displayString(for:) is updated accordingly.
- Around line 5315-5324: The displayed timeout text uses a hard-coded format
"%.1fs"; replace this with a localized format string and use that when
formatting leaderKeyTimeout. Locate the Text(...) inside SettingsCardRow (the
Text currently using String(format: "%.1fs", leaderKeyTimeout)) and change it to
first fetch a localized format via String(localized:
"settings.leaderKey.timeout.format", defaultValue: "%.1fs") (or similar key),
then call String(format: localizedFormat, leaderKeyTimeout) so the suffix is
localizable; keep the .font(.system(.body, design: .monospaced)) and
.frame(width: 40, alignment: .trailing) as-is.
In `@Sources/ContentView.swift`:
- Around line 8214-8227: The leader-mode badge is currently inside the
ScrollView content and can scroll away; move the UI that checks
tabManager.isLeaderModeActive (the HStack/Text block that shows
"leader.mode.indicator") out of the scrollable content and render it in a
non-scrolling overlay/top layer of ContentView (e.g., place it in a parent
ZStack or as a top overlay/scrim) so it stays pinned to the view while leader
mode is active; keep the same styling (font, padding, background, cornerRadius)
and condition (tabManager.isLeaderModeActive) but relocate the HStack/Text from
the ScrollView body to the view’s overlay/top container so it remains visible
even when the scroll content moves.
- Around line 6059-6060: The workspaceDisplayName(_:) function is only for
presentation and is currently seeding the rename flow with a tag-decorated
title; update the rename flow so it seeds its input from the workspace's
raw/custom title (e.g., workspace.customTitle or workspace.title) instead of
calling workspaceDisplayName(_:), and keep workspaceDisplayName(_:) unchanged
for display-only use; adjust applyRenameFlow to read and persist the raw
customTitle when creating/updating the rename draft so tags aren't embedded into
customTitle.
In `@Sources/LeaderKeySettings.swift`:
- Around line 31-33: The default for Workspace Tags is wrong: change the static
constant workspaceTagsEnabledDefault in LeaderKeySettings
(LeaderKeySettings.swift) from true to false so that users without a persisted
value get tags off by default; update any related usages that rely on this
constant (e.g., settings initialization or feature-gate checks that read
workspaceTagsEnabledDefault) to continue to use this constant for the fallback
value.
In `@Sources/TerminalController.swift`:
- Around line 3594-3596: The code currently treats any non-string "tag" as nil
because v2String(params, "tag") returns nil, which causes accidental clears;
change the handling so that if params contains the "tag" key and its value is
neither a String nor NSNull, you return an invalid_params error instead of
proceeding. Concretely: check params for presence of "tag" (e.g., params["tag"]
!= nil), if the value is NSNull treat that as an explicit clear, if it is a
String use v2String/trim to compute effectiveTag (empty string or trimmed-empty
-> clear), and if it is any other type return invalid_params; keep using tagRaw,
tag, and effectiveTag variables but add the presence/type guard before computing
them.
In `@Sources/Workspace.swift`:
- Line 4966: The tag property needs normalization to a single-line value so
multi-line or tab-containing tags (e.g. "foo\nbar" or "foo\tbar") don’t break
layouts; update the setter flow (the existing setTag(_:) implementation) to trim
leading/trailing whitespace and replace any internal newlines, tabs, and runs of
whitespace with a single space (or otherwise collapse internal whitespace to one
space) before assigning to the `@Published` var tag and before computing
displayTitle; ensure you update both the setTag(_:) method and any other places
that assign tag (the other block around lines 5823-5840) so all inputs are
normalized consistently.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 10935-10939: The visible row renders Text(tab.displayTitle) but
the accessibility announcement still uses tab.title; update the accessibility
modifier so screen readers announce the same identifier by replacing usages of
accessibilityTitle (or .accessibilityLabel/.accessibilityValue if used) that
reference tab.title with tab.displayTitle for the same view (look for the
accessibilityTitle call near the Text(tab.displayTitle) and change the argument
to tab.displayTitle).
---
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Line 3849: The workspaceTagsEnabled AppStorage is wrongly housed in
LeaderKeySettings; create a separate settings namespace (e.g.,
WorkspaceTagsSettings) with its own key constant (workspaceTagsEnabledKey) and
default (workspaceTagsEnabledDefault), replace occurrences of
LeaderKeySettings.workspaceTagsEnabledKey/Default and the `@AppStorage` binding
for workspaceTagsEnabled to use WorkspaceTagsSettings, and move the UI and any
references (including the other occurrences around the block that previously
used LeaderKeySettings at lines ~5343-5355) out of the “Leader Key” section into
a new “Workspace Tags” settings section/header so tags are independently
persisted and displayed.
In `@Sources/TerminalController.swift`:
- Around line 3291-3304: Extract the workspace payload construction into a
single helper (e.g., a method like encodeWorkspacePayload or
Workspace.payloadDictionary) and replace the inline map over tabManager.tabs in
TerminalController (the block that builds workspaces with keys
"id","ref","index","title","selected","pinned","listening_ports","remote","current_directory","custom_color","tag")
so the same helper is used by workspace.list, workspace.current and system.tree;
ensure the helper accepts the Workspace instance (and index/selected state when
needed) and reuses v2Ref, v2OrNull and ws.remoteStatusPayload to produce a
consistent dictionary output across all callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a32d81c9-213c-4fab-a357-1a2912d9adf6
📒 Files selected for processing (11)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/LeaderKeySettings.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swift
There was a problem hiding this comment.
2 issues found across 4 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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:8923">
P2: Leader-key routing guard now exits the entire custom shortcut handler for non-main-window events, unintentionally bypassing all later app-level shortcuts.</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:3594">
P2: `workspace.set_tag` cannot accept empty-string tags for clearing because `v2String` converts empty input to `nil`, causing the new required-tag guard to reject valid clear requests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
4966-4966:⚠️ Potential issue | 🟡 MinorNormalize workspace tags to a single-line invariant.
setTag(_:)still preserves embedded tabs/newlines, so an API value like"foo\nbar"will render directly intodisplayTitleand can break the tab/switcher layout. Also, becausetagis publicly writable, callers can bypass the 20-character/whitespace rules entirely.💡 Proposed fix
- `@Published` var tag: String? + `@Published` private(set) var tag: String? ... func setTag(_ newTag: String?) { - let trimmed = newTag?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let capped = String(trimmed.prefix(20)) - tag = capped.isEmpty ? nil : capped + let normalized = newTag? + .components(separatedBy: .whitespacesAndNewlines) + .filter { !$0.isEmpty } + .joined(separator: " ") ?? "" + let capped = String(normalized.prefix(20)) + let nextTag = capped.isEmpty ? nil : capped + guard tag != nextTag else { return } + tag = nextTag }Also applies to: 5823-5826
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` at line 4966, The tag property allows embedded newlines/tabs and is publicly writable so callers can bypass length/whitespace rules; change `@Published` var tag to `@Published` private(set) var tag and ensure all external mutations go through setTag(_:) (or a new normalizedTag setter) which normalizes input by replacing all whitespace sequences (including tabs/newlines) with a single space, trimming ends, and truncating to 20 characters before assigning and updating displayTitle; also audit and replace direct assignments to tag (including the locations referenced around the 5823-5826 region) so they call the normalization setter instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 3594-3598: The code treats an empty string as a missing parameter
because v2String(params, "tag") returns nil for "" and causes workspace.set_tag
to error instead of clearing the tag; change the logic to detect the presence of
the "tag" parameter explicitly (e.g., check params contains "tag" or use a
helper that returns an optional distinguishing absent vs present-empty) so that
when "tag" is present but empty you proceed (set effectiveTag = nil) and only
return .err("invalid_params") when the parameter is actually missing or
malformed; update the block around v2String, tagRaw, and effectiveTag to
implement this presence-vs-empty distinction.
In `@Sources/Workspace.swift`:
- Around line 5829-5840: The tagged display string in the displayTitle computed
property is currently assembled from a literal ("[\(tag)] \(base)"); change this
to use a localized format string (e.g. via String(localized:
"workspace.displayName.tagged", defaultValue: "[%@] %@")) and pass tag and base
as format arguments so the entire tagged format is localizable; update the code
paths in the displayTitle property that check
LeaderKeySettings.workspaceTagsEnabled and use the new localized format, and add
the corresponding "workspace.displayName.tagged" entry to
Resources/Localizable.xcstrings with the appropriate default value.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Line 4966: The tag property allows embedded newlines/tabs and is publicly
writable so callers can bypass length/whitespace rules; change `@Published` var
tag to `@Published` private(set) var tag and ensure all external mutations go
through setTag(_:) (or a new normalizedTag setter) which normalizes input by
replacing all whitespace sequences (including tabs/newlines) with a single
space, trimming ends, and truncating to 20 characters before assigning and
updating displayTitle; also audit and replace direct assignments to tag
(including the locations referenced around the 5823-5826 region) so they call
the normalization setter instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c207e78-3358-458b-9299-0be08c8db2b4
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/LeaderKeySettings.swiftSources/TerminalController.swiftSources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/LeaderKeySettings.swift
- Sources/AppDelegate.swift
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
Sources/cmuxApp.swift (2)
5315-5324:⚠️ Potential issue | 🟡 MinorLocalize the timeout value text.
"%.1fs"is still user-visible copy, so the suffix remains hard-coded English instead of going through the string catalog.🌐 Proposed fix
HStack(spacing: 8) { + let timeoutFormat = String( + localized: "settings.leaderKey.timeout.format", + defaultValue: "%.1fs" + ) Slider(value: $leaderKeyTimeout, in: 0.2...3.0, step: 0.1) .frame(width: 120) - Text(String(format: "%.1fs", leaderKeyTimeout)) + Text(String(format: timeoutFormat, leaderKeyTimeout)) .font(.system(.body, design: .monospaced)) .frame(width: 40, alignment: .trailing) }As per coding guidelines: "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 5315 - 5324, Replace the hard-coded format string used in the Text view inside SettingsCardRow with a localized format string: fetch the format via String(localized: "settings.leaderKey.timeout.value", defaultValue: "%.1fs") and then use that localized format with leaderKeyTimeout (e.g., via String(format: localizedFormat, leaderKeyTimeout)); keep the Text, .font and .frame usage unchanged so only the displayed timeout value suffix goes through localization.
6317-6339:⚠️ Potential issue | 🟠 MajorFunction/navigation keys still slip past this validation.
Repro: click a row and press ↑ or Page Down. Those keys arrive as single non-whitespace scalars, so this guard still accepts them and stores an unusable binding. Please explicitly reject special/function keys before assigning
currentKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6317 - 6339, The guard still accepts navigation/function keys because they deliver single non-whitespace scalars; update the logic that sets recorded/currentKey to explicitly reject those by checking the NSEvent keyCode and function flags: after computing recorded (and before assigning currentKey) add a check against a small set of known navigation keyCodes (up/down/left/right, pageUp/pageDown, home/end) and also reject events where modifierFlags.contains(.function) or whose keyCode matches F-keys; if any of those match, return nil so recorded/currentKey is not set. Use the existing event and recorded identifiers and ensure the new check runs before the final guard that assigns currentKey.Sources/ContentView.swift (1)
8216-8229:⚠️ Potential issue | 🟠 MajorPin the leader indicator outside the scrollable stack.
This badge still lives inside the
ScrollViewcontent, so it can disappear while leader mode remains armed. Move it into one of the sidebar's top overlays so the active-state indicator stays visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 8216 - 8229, The leader-mode badge is currently inside the ScrollView and can scroll away; move the HStack that checks tabManager.isLeaderModeActive (the Text showing localized "leader.mode.indicator") out of the ScrollView content and place it in the sidebar's fixed top overlay (e.g., attach it with .overlay or a VStack at the sidebar root) so it remains pinned while the list scrolls; keep the same styling (font, padding, background, cornerRadius) and preserve the conditional use of tabManager.isLeaderModeActive to show/hide the badge.Sources/Workspace.swift (1)
5832-5844:⚠️ Potential issue | 🟡 MinorLocalize the tagged title format.
displayTitleis user-facing text, so the tagged branch should not be assembled from a bare literal. Please move the full format intoResources/Localizable.xcstringsas well.🌐 Suggested change
- return "[\(tag)] \(base)" + let format = String(localized: "workspace.displayTitle.tagged", defaultValue: "[%@] %@") + return String(format: format, tag, base)As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI (labels, buttons, menus, dialogs, tooltips, error messages)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 5832 - 5844, The tagged branch in displayTitle currently builds a UI string from a literal; change it to use a localized format string instead: add a localized entry (e.g. "workspace.displayName.tagged" with default like "[%@] %@") to Resources/Localizable.xcstrings and replace the return "[\(tag)] \(base)" with a call that formats String(localized: "workspace.displayName.tagged", defaultValue: "[%@] %@") (or use String(localized:...) with String(format:) as appropriate) while still gating on LeaderKeySettings.workspaceTagsEnabled and the existing tag/base values.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
4966-4966: Restricttagwrites to the normalizing setter.
setTag(_:)now owns whitespace collapsing and the 20-character cap, buttagis still writable from anywhere in the module. A direct assignment can bypass those invariants and then flow into UI/persistence unchanged.♻️ Suggested change
- `@Published` var tag: String? + `@Published` private(set) var tag: String?Also applies to: 5823-5829
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` at line 4966, The `@Published` var tag: String? is publicly writable and can be assigned directly, bypassing the normalization logic in setTag(_:); change the visibility so writes go only through the normalizing setter—e.g. make the published storage privately writable (private(set) or private) and expose a read-only tag plus keep the existing setTag(_:) as the sole mutator (or route setTag to update the private published storage). Also apply the same visibility change to the other tag-like published properties referenced by setTag (the similar declarations near the other occurrence) so all assignments must flow through the normalization routine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 6255-6365: The startRecording() can leave multiple buttons with
active NSEvent monitors; make LeaderKeyRecorderNSButton enforce a single active
recorder by adding a static weak var currentRecorder: LeaderKeyRecorderNSButton?
and in startRecording() check if currentRecorder exists and != self then call
its stopRecording() before proceeding, then set currentRecorder = self; in
stopRecording() clear currentRecorder if currentRecorder === self; keep using
eventMonitor and NotificationCenter removal as already implemented so the
previous monitor/observer are reliably removed.
In `@Sources/ContentView.swift`:
- Line 10937: TabItemView currently renders tab.displayTitle but
accessibilityTitle and the Equatable == implementation still use tab.title, so
VoiceOver can't distinguish workspaces that differ only by tags; fix by
computing a resolvedDisplayTitle (using tab.displayTitle) in the parent and pass
it into TabItemView as a parameter, use that same resolvedDisplayTitle for both
the visible Text and the accessibilityLabel (accessibilityTitle), and include
resolvedDisplayTitle in TabItemView’s Equatable conformance (the == function) so
.equatable() correctly skips body updates; do not add new
EnvironmentObject/ObservedObject/Binding properties or remove .equatable() from
the ForEach.
In `@Sources/Workspace.swift`:
- Around line 8035-8067: The focusPreviousPane, focusNextPane and
focusPaneByIndex functions currently use bonsplitController.allPaneIds but must
use the visual/tree order; replace the paneIds assignment in each with the
tree-ordered list (use SidebarBranchOrdering.orderedPaneIds(tree:) or the
existing helper sidebarOrderedPanelIds()) so navigation uses the same visual
ordering as the sidebar; keep the rest of each method (guard, currentId
selection, index logic, and switchFocusToPane calls) unchanged.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 5315-5324: Replace the hard-coded format string used in the Text
view inside SettingsCardRow with a localized format string: fetch the format via
String(localized: "settings.leaderKey.timeout.value", defaultValue: "%.1fs") and
then use that localized format with leaderKeyTimeout (e.g., via String(format:
localizedFormat, leaderKeyTimeout)); keep the Text, .font and .frame usage
unchanged so only the displayed timeout value suffix goes through localization.
- Around line 6317-6339: The guard still accepts navigation/function keys
because they deliver single non-whitespace scalars; update the logic that sets
recorded/currentKey to explicitly reject those by checking the NSEvent keyCode
and function flags: after computing recorded (and before assigning currentKey)
add a check against a small set of known navigation keyCodes
(up/down/left/right, pageUp/pageDown, home/end) and also reject events where
modifierFlags.contains(.function) or whose keyCode matches F-keys; if any of
those match, return nil so recorded/currentKey is not set. Use the existing
event and recorded identifiers and ensure the new check runs before the final
guard that assigns currentKey.
In `@Sources/ContentView.swift`:
- Around line 8216-8229: The leader-mode badge is currently inside the
ScrollView and can scroll away; move the HStack that checks
tabManager.isLeaderModeActive (the Text showing localized
"leader.mode.indicator") out of the ScrollView content and place it in the
sidebar's fixed top overlay (e.g., attach it with .overlay or a VStack at the
sidebar root) so it remains pinned while the list scrolls; keep the same styling
(font, padding, background, cornerRadius) and preserve the conditional use of
tabManager.isLeaderModeActive to show/hide the badge.
In `@Sources/Workspace.swift`:
- Around line 5832-5844: The tagged branch in displayTitle currently builds a UI
string from a literal; change it to use a localized format string instead: add a
localized entry (e.g. "workspace.displayName.tagged" with default like "[%@]
%@") to Resources/Localizable.xcstrings and replace the return "[\(tag)]
\(base)" with a call that formats String(localized:
"workspace.displayName.tagged", defaultValue: "[%@] %@") (or use
String(localized:...) with String(format:) as appropriate) while still gating on
LeaderKeySettings.workspaceTagsEnabled and the existing tag/base values.
---
Nitpick comments:
In `@Sources/Workspace.swift`:
- Line 4966: The `@Published` var tag: String? is publicly writable and can be
assigned directly, bypassing the normalization logic in setTag(_:); change the
visibility so writes go only through the normalizing setter—e.g. make the
published storage privately writable (private(set) or private) and expose a
read-only tag plus keep the existing setTag(_:) as the sole mutator (or route
setTag to update the private published storage). Also apply the same visibility
change to the other tag-like published properties referenced by setTag (the
similar declarations near the other occurrence) so all assignments must flow
through the normalization routine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf9fa50d-7cba-4082-b2aa-560534051d7d
📒 Files selected for processing (5)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/TerminalController.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AppDelegate.swift
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (2)
8933-8954:⚠️ Potential issue | 🔴 CriticalScope second-key dispatch to
leaderKeyOwner.Line 8934 re-syncs to whatever window produced the second key, and Lines 10642-10690 then execute against the current
tabManager. That means a prefix armed in window A can still close or mutate window B if the user switches windows before the next key.leaderKeyOwneris currently only used when clearing the indicator, not when deciding where the action is allowed to run.🔧 Minimal guard
if LeaderKeySettings.isEnabled, leaderKeyState == .waitingForSecondKey { - if synchronizeShortcutRoutingContext(event: event) { + if synchronizeShortcutRoutingContext(event: event), + let leaderKeyOwner, + tabManager === leaderKeyOwner { cancelLeaderMode() let leaderShortcut = KeyboardShortcutSettings.shortcut(for: .leaderKey) // Double-press leader key: send the raw key through to terminal if matchShortcut(event: event, shortcut: leaderShortcut) { return false } // ESC cancels leader mode if event.keyCode == 53 { return true } // Try to dispatch the second key if executeLeaderAction(event: event) { return true } // Unrecognized second key: beep and consume NSSound.beep() return true } else { cancelLeaderMode() } }Also applies to: 10533-10539, 10642-10690
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8933 - 8954, The second-key handling currently re-syncs routing to the window that produced the second key and then runs matchShortcut/executeLeaderAction against whichever tabManager is current; change this so the second-key dispatch is scoped to the original leaderKeyOwner: use leaderKeyOwner as the routing target (or bail/cancel if leaderKeyOwner is nil) instead of accepting a different window from synchronizeShortcutRoutingContext(event:), and ensure matchShortcut(event:, shortcut:) and executeLeaderAction(event:) are invoked against leaderKeyOwner's context/tabManager rather than the current global tabManager; keep cancelLeaderMode() and indicator clearing behavior unchanged but add the guard so a prefix armed in one window cannot close/mutate another window if focus changes before the second key.
8997-9013:⚠️ Potential issue | 🟠 MajorLeader arming still bypasses the command-palette guard.
Line 9002 runs before palette state is computed at Line 9016, so the leader chord can still arm while the command palette search field is focused. Once that happens, the next key is consumed by the armed-session path instead of going to the palette input. Move leader arming behind the palette visibility check, or explicitly bail/cancel when the target window has an effective palette. Based on learnings, when the command palette is visible, shortcut handling must not be reordered to bypass its guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8997 - 9013, Leader arming executes before the command-palette guard, allowing the leader chord to arm while the palette input is focused; move the leader-arming block (the if guarded by LeaderKeySettings.isEnabled && leaderKeyState == .inactive that calls synchronizeShortcutRoutingContext(event:), matchShortcut(event:shortcut:), sets leaderKeyState/leaderKeyOwner/tabManager?.isLeaderModeActive and calls startLeaderKeyTimer()) to after the command-palette visibility/focus check (the code that computes whether the palette is visible or the palette input is focused), or add an early bail that checks the target window/tabManager for an active/effective command palette and returns false; ensure you reference LeaderKeySettings, leaderKeyState, synchronizeShortcutRoutingContext(event:), matchShortcut(event:shortcut:), KeyboardShortcutSettings.shortcut(for: .leaderKey), leaderKeyOwner, tabManager?.isLeaderModeActive and startLeaderKeyTimer() when applying the change so leader arming cannot occur while the palette is visible.
🧹 Nitpick comments (2)
Sources/TerminalController.swift (1)
3302-3303: Consider centralizing workspace response encoding.
workspace.list,workspace.current, andv2TreeWorkspaceNodeall hand-build overlapping workspace dictionaries. Addingtaghere makes payload drift easier; a shared helper would keep future fields aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3302 - 3303, Multiple places (workspace.list, workspace.current, v2TreeWorkspaceNode) manually build similar workspace dictionaries causing drift; create a single helper (e.g., a function like encodeWorkspaceV2 or WorkspaceV2Encoder) that accepts a Workspace model and returns the canonical dictionary/json with fields including id, name, is_default, custom_color, tag, etc., then replace the per-site dictionary construction in workspace.list, workspace.current and v2TreeWorkspaceNode to call that helper so all payloads stay consistent.Sources/Workspace.swift (1)
4966-4966: Protect tag invariants by restricting direct writes.
tagis externally mutable, so normalization/length limits insetTag(_:)can be bypassed. Consider makingtagread-only outsideWorkspaceand forcing updates throughsetTag(_:).♻️ Suggested change
- `@Published` var tag: String? + `@Published` private(set) var tag: String?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` at line 4966, Make the tag property read-only outside Workspace by changing its setter visibility and force all updates through the existing normalization method: change the `@Published` tag declaration to have a restricted setter (e.g., `@Published` private(set) var tag: String?) inside the Workspace type, keep the public func setTag(_:) as the sole mutator that enforces normalization/length limits, and update any external code that previously wrote to tag to call setTag(_:) instead. Ensure observers still receive published updates when setTag(_:) assigns the normalized value.
🤖 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/AppDelegateShortcutRoutingTests.swift`:
- Around line 3233-3260: The helper withTemporaryLeaderWorkspaceTagSettings
currently saves/restores enabled, workspaceTagsEnabled, and the setWorkspaceTag
action but not the persisted leader-key binding; update it to also save and
restore the UserDefaults entry that stores the leader chord (the persisted
leader-key binding used by LeaderKeySettings) and, before calling body(), force
that defaults entry to the default chord (Ctrl+B) so tests synthesize the
expected leader key; ensure you reference LeaderKeySettings when reading/setting
the leader chord defaults key and restore the original value in the defer block
alongside the other savedValues.
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 238-239: The test currently hardcodes "[api] Project" which will
break under non-English locales; instead obtain the expected tagged title via
the same localization path used in production (e.g. call the localization format
and format it with the tag and workspace title) or force the test locale to
English before asserting; update the assertion to compute expected =
String(format: NSLocalizedString("workspace.tagged_title_format", comment: ""),
"[api]", "Project") (or call the production helper that builds tagged titles)
and then XCTAssertEqual(workspace.displayTitle, expected) while keeping the
UserDefaults change to LeaderKeySettings.workspaceTagsEnabledKey.
In `@Sources/cmuxApp.swift`:
- Around line 6327-6335: The recorder currently checks unshifted.first to decide
whether to prefer unshifted.lowercased(), which causes Shift+5 to record "5"
instead of the shifted "%" — change the branching to inspect the actual produced
character (use shifted or characters) when deciding; e.g. replace the check on
unshifted.first with a check on shifted.first (or characters.first if that
symbol exists) so that if the produced/shifted first char is a letter/number you
still use unshifted.lowercased(), otherwise prefer shifted when non-empty,
falling back to unshifted — update the logic around the variables shifted,
unshifted, and recorded accordingly.
In `@Sources/Workspace.swift`:
- Around line 8036-8043: visuallyOrderedPaneIds() currently uses compactMap to
convert orderedStrings from SidebarBranchOrdering.orderedPaneIds(tree:) into
PaneID instances by matching bonsplitController.allPaneIds, which can drop
entries when an ID doesn't match and return a partial ordering; change the logic
to map orderedStrings to PaneID? (preserving nils) or otherwise build a mapped
array that only becomes the return value if mapped.count ==
bonsplitController.allPaneIds.count, and otherwise return
bonsplitController.allPaneIds as the safe fallback; ensure you reference
visuallyOrderedPaneIds(), SidebarBranchOrdering.orderedPaneIds(tree:), and
bonsplitController.allPaneIds when updating the mapping and fallback check.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 8933-8954: The second-key handling currently re-syncs routing to
the window that produced the second key and then runs
matchShortcut/executeLeaderAction against whichever tabManager is current;
change this so the second-key dispatch is scoped to the original leaderKeyOwner:
use leaderKeyOwner as the routing target (or bail/cancel if leaderKeyOwner is
nil) instead of accepting a different window from
synchronizeShortcutRoutingContext(event:), and ensure matchShortcut(event:,
shortcut:) and executeLeaderAction(event:) are invoked against leaderKeyOwner's
context/tabManager rather than the current global tabManager; keep
cancelLeaderMode() and indicator clearing behavior unchanged but add the guard
so a prefix armed in one window cannot close/mutate another window if focus
changes before the second key.
- Around line 8997-9013: Leader arming executes before the command-palette
guard, allowing the leader chord to arm while the palette input is focused; move
the leader-arming block (the if guarded by LeaderKeySettings.isEnabled &&
leaderKeyState == .inactive that calls
synchronizeShortcutRoutingContext(event:), matchShortcut(event:shortcut:), sets
leaderKeyState/leaderKeyOwner/tabManager?.isLeaderModeActive and calls
startLeaderKeyTimer()) to after the command-palette visibility/focus check (the
code that computes whether the palette is visible or the palette input is
focused), or add an early bail that checks the target window/tabManager for an
active/effective command palette and returns false; ensure you reference
LeaderKeySettings, leaderKeyState, synchronizeShortcutRoutingContext(event:),
matchShortcut(event:shortcut:), KeyboardShortcutSettings.shortcut(for:
.leaderKey), leaderKeyOwner, tabManager?.isLeaderModeActive and
startLeaderKeyTimer() when applying the change so leader arming cannot occur
while the palette is visible.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 3302-3303: Multiple places (workspace.list, workspace.current,
v2TreeWorkspaceNode) manually build similar workspace dictionaries causing
drift; create a single helper (e.g., a function like encodeWorkspaceV2 or
WorkspaceV2Encoder) that accepts a Workspace model and returns the canonical
dictionary/json with fields including id, name, is_default, custom_color, tag,
etc., then replace the per-site dictionary construction in workspace.list,
workspace.current and v2TreeWorkspaceNode to call that helper so all payloads
stay consistent.
In `@Sources/Workspace.swift`:
- Line 4966: Make the tag property read-only outside Workspace by changing its
setter visibility and force all updates through the existing normalization
method: change the `@Published` tag declaration to have a restricted setter (e.g.,
`@Published` private(set) var tag: String?) inside the Workspace type, keep the
public func setTag(_:) as the sole mutator that enforces normalization/length
limits, and update any external code that previously wrote to tag to call
setTag(_:) instead. Ensure observers still receive published updates when
setTag(_:) assigns the normalized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc139ed1-e19c-4441-80b3-3e2290c5370a
📒 Files selected for processing (10)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftcmuxTests/WorkspaceUnitTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Resources/Localizable.xcstrings
- Sources/TabManager.swift
- Sources/ContentView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
4966-4966: Keeptagwrites funneled throughsetTag(_:).
setTag(_:)is now the invariant-enforcing path for whitespace collapse and the 20-character cap. Making the published propertyprivate(set)would prevent future call sites from bypassing that normalization.♻️ Proposed refinement
- `@Published` var tag: String? + `@Published` private(set) var tag: String?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` at line 4966, Make the published property tag read-only from outside by changing its declaration to private(set) so all external mutations must go through setTag(_:), which enforces whitespace collapse and the 20-character cap; update any call sites that currently write to tag to call setTag(_:) instead, ensuring normalization logic in setTag(_:) remains the single invariant-enforcing path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 5301-5310: The Toggle in SettingsCardRow currently has an empty
visible label (Toggle("", isOn: $leaderKeyEnabled)) and the
LeaderKeyRecorderButton doesn't expose the action context to VoiceOver; update
the Toggle to include an explicit accessibilityLabel (e.g., use the localized
"settings.leaderKey.enabled" string) and set accessibilityValue/state
appropriately, and update LeaderKeyRecorderButton usages to pass action.label
through as the button's accessibilityLabel (or accessibilityValue) so VoiceOver
announces the setting/action; apply the same changes at the other occurrences
mentioned (lines ~5344-5353 and ~6218-6252) referencing SettingsCardRow,
Toggle("", isOn: $leaderKeyEnabled) and LeaderKeyRecorderButton and ensure the
accessibility properties use the localized action.label.
- Around line 6315-6356: The recorder currently accepts keys even when
Command/Control/Option modifiers are held, collapsing chords like Cmd+1 or
Ctrl+B to plain characters; update the NSEvent.addLocalMonitorForEvents block
(the eventMonitor closure) to reject events that include .command, .control, or
.option in event.modifierFlags (allow Shift for symbol detection) by returning
nil before computing recorded, so the recorder does not store a plain character
for modifier chords; keep existing behavior for Shift-only and non-modifier
keys, continue to call stopRecording(), set currentKey and call onKeyRecorded?
only for modifier-free (or Shift-only when appropriate) single printable
characters.
---
Nitpick comments:
In `@Sources/Workspace.swift`:
- Line 4966: Make the published property tag read-only from outside by changing
its declaration to private(set) so all external mutations must go through
setTag(_:), which enforces whitespace collapse and the 20-character cap; update
any call sites that currently write to tag to call setTag(_:) instead, ensuring
normalization logic in setTag(_:) remains the single invariant-enforcing path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93085532-c101-418d-a3ac-671856d9bf3c
📒 Files selected for processing (4)
Sources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/WorkspaceUnitTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/WorkspaceUnitTests.swift
ctrlShiftBryan
added a commit
to ctrlShiftBryan/cmux
that referenced
this pull request
Mar 21, 2026
Continuation of c24a5c4 TURN 1: Found and analyzed open PR manaflow-ai#1854 by xiaocang implementing tmux-style leader key mode with configurable timeout, visual indicator, and workspace/pane management actions (~1900 additions). OVERALL: Identified that PR manaflow-ai#1854 is the existing pull request implementing the requested feature with leader key mode and workspace tags, currently awaiting human review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ctrlShiftBryan
added a commit
to ctrlShiftBryan/cmux
that referenced
this pull request
Mar 21, 2026
Continuation of 2411b06 TURN 1: User asked for a URL and was provided a link to a GitHub pull request (manaflow-ai#1854). OVERALL: Shared a reference to a GitHub pull request URL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ctrlShiftBryan
added a commit
to ctrlShiftBryan/cmux
that referenced
this pull request
Mar 26, 2026
Continuation of 008ae36 TURN 1: User asks about keybinding customization in cmux; Claude explains the Settings UI supports 28 customizable actions with single-key combos. TURN 2: User asks about chord/sequence keybindings like tmux; Claude explains cmux UI doesn't support this but Ghostty config does via key sequences. TURN 3: User asks if anyone requested this feature; Claude finds multiple GitHub issues requesting leader-key/chord support, with manaflow-ai#1827 being most relevant. TURN 4: User asks if anyone submitted a PR; Claude finds PR manaflow-ai#1854 by xiaocang adding tmux-style leader key mode with customizable actions and visual indicator. TURN 5: User asks for PR URL; Claude provides manaflow-ai#1854. TURN 6: User asks Claude to open it in browser; Claude opens the URL. TURN 7: User asks how to test the PR instead of waiting for merge; Claude provides instructions for building from source and checking out the PR branch. TURN 8: User asks Claude to perform checkout and initialization for staging use; Claude checkouts PR manaflow-ai#1854, attempts setup but hits Xcode version issues (needs 17+ for macOS 26 SDK). TURN 9: User wants to install latest Xcode beta; Claude identifies Xcode 26.0 Beta 4 is latest and asks user to run installation command interactively. TURN 10: User confirms Xcode installed; Claude verifies installation and asks to switch developer tools with sudo. TURN 11: User confirms sudo command complete; Claude verifies Xcode 26.4 active, runs setup and build, resolves zig linker issues with macOS 26 SDK by using Xcode 16.4 for zig builds. TURN 12: Background build task completes; Claude acknowledges successful build and confirms cmux STAGING is running with leader key feature. TURN 13: User asks what the leader key is; Claude confirms Ctrl+B default and explains sub-key bindings (n/p for workspaces, numbers for jumps, etc.) and customization options. TURN 14: User asks how to start the app since they closed it; Claude confirms it's back up and running. OVERALL: User explored cmux keybinding customization, discovered PR manaflow-ai#1854 adding tmux-style leader key chording, and with Claude's help built and tested the PR branch in staging mode by installing Xcode 26 beta and resolving zig SDK compatibility issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
xiaocang
force-pushed
the
feat/leader-key-workspace-tags
branch
from
f151a89 to
a0987b0
Compare
xiaocang
force-pushed
the
feat/leader-key-workspace-tags
branch
2 times, most recently
from
7625f8c to
55234e3
Compare
Implements tmux-style Leader Key mode that intercepts a configurable key chord (default: Ctrl+B) to activate a one-shot command mode for workspace navigation and management. Leader Key features: - Configurable leader key with timeout (default 1500ms) - Visual indicator when leader mode is active - Key bindings: n/p (next/prev workspace), number keys (jump to workspace), c (create), x (close), , (rename), d (detach/split), q (quit mode) Workspace Tags features: - Assign short tags/labels to workspaces for quick identification - Tags shown in workspace switcher and tab bar - Persistent across sessions via SessionPersistence - Reactive updates across all windows via objectWillChange Also adds LeaderKeySettings.swift with settings model, updates KeyboardShortcutSettings for leader key configuration, wires up routing in AppDelegate to handle leader key intercept before normal shortcut processing. All user-facing strings localized in English and Japanese. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
xiaocang
force-pushed
the
feat/leader-key-workspace-tags
branch
from
55234e3 to
d22b224
Compare
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
1 participant
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.
Implements tmux-style Leader Key mode that intercepts a configurable key chord (default: Ctrl+B) to activate a one-shot command mode for workspace navigation and management.
Leader Key features:
Workspace Tags features:
Also adds LeaderKeySettings.swift with settings model, updates KeyboardShortcutSettings for leader key configuration, wires up routing in AppDelegate to handle leader key intercept before normal shortcut processing.
All user-facing strings localized in English and Japanese.
Summary
Testing
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Adds tmux‑style Leader Key mode and Workspace Tags to speed up navigation and label workspaces, plus copy‑mode stability fixes. Both features are configurable in Settings and off by default.
New Features
Ctrl+B): arms only outside Settings/alerts/command palette; ESC cancels; double‑press passes through; window‑scoped; “LEADER” sidebar indicator; configurable timeout (0.2–3.0s) and per‑action sub‑keys via a simple recorder; addedleaderKeyinKeyboardShortcutSettings.SessionPersistence; set via leader action or v2 APIworkspace.set_tag; v2 workspace payloads includetag.Bug Fixes
workspace.set_tagreturns the stored tag and errors when tags are disabled; matching honors shifted symbols and keyboard layout fallbacks.displayTitlefor tagged names.Written for commit d22b224. Summary will update on new commits.
Summary by CodeRabbit
New Features
UI
Persistence & Sync
Localization
Tests