allow configuring key prefix for tab and workspace changes#1819
Open
gmmeyer wants to merge 3 commits intomanaflow-ai:mainfrom
Open
allow configuring key prefix for tab and workspace changes#1819gmmeyer wants to merge 3 commits intomanaflow-ai:mainfrom
gmmeyer wants to merge 3 commits intomanaflow-ai:mainfrom
Conversation
|
@gmmeyer is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
1 similar comment
|
To use Codex here, create a Codex account and connect to github. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable digit-shortcut modifiers: persisted settings, a modifier-recording UI, runtime propagation via a change notification, shortcut matching updated to use configurable flags, localization updates, and unit tests validating behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as ModifierRecorderRow
participant NSWrap as ModifierRecorderNSButton
participant Defaults as UserDefaults
participant AppDel as AppDelegate
participant Shortcuts as Shortcut Matcher
User->>UI: Click "Record"
UI->>NSWrap: begin capture (show "Press modifier…")
User->>NSWrap: Press modifier keys
NSWrap->>NSWrap: update displayed symbol
User->>NSWrap: Release keys or press Escape
NSWrap->>Defaults: write stored modifier Int
Defaults->>AppDel: post DigitShortcutModifierSettings.didChangeNotification
AppDel->>AppDel: refresh cached workspace/surface flags
User->>Shortcuts: Press <modifier>+5
Shortcuts->>AppDel: request cached flags / compare event flags
AppDel-->>Shortcuts: return match result
Shortcuts->>User: Trigger workspace/surface selection if matched
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
@gmmeyer I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 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/KeyboardShortcutSettings.swift">
<violation number="1" location="Sources/KeyboardShortcutSettings.swift:302">
P1: Unchecked negative Int to UInt conversion from UserDefaults can cause runtime crash. Change `raw != 0` checks to `raw > 0` to ensure only non-negative values are converted.</violation>
</file>
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
9491-9515:⚠️ Potential issue | 🟠 MajorShift-inclusive digit modifiers won't work here.
These branches parse
Int(chars)directly, butcharscomes fromcharactersIgnoringModifiers, which preserves Shift-transformed symbols. If a user configures modifier flags that includeShift, thenShift+1becomes"!", and the shortcut never matches. Extract the digit from the unshifted key instead of parsingcharsdirectly, or validate the modifier settings to prevent.shiftfrom being stored.💡 Minimal fix shape
- if flags == workspaceDigitFlags, - let manager = tabManager, - let num = Int(chars), + let digit = resolvedDigitShortcutNumber(for: event) + + if flags == workspaceDigitFlags, + let manager = tabManager, + let num = digit, let targetIndex = WorkspaceShortcutMapper.workspaceIndex(forCommandDigit: num, workspaceCount: manager.tabs.count) { manager.selectTab(at: targetIndex) return true } - if flags == surfaceDigitFlags { - if let num = Int(chars), num >= 1 && num <= 9 { + if flags == surfaceDigitFlags, let num = digit { if num == 9 { tabManager?.selectLastSurface() } else { tabManager?.selectSurface(at: num - 1) } return true - } }
resolvedDigitShortcutNumber(for:)should derive1...9from the unshifted key (via keyCode or layout-aware lookup), not directly fromcharactersIgnoringModifiers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 9491 - 9515, The digit parsing currently uses charactersIgnoringModifiers (chars) so Shift-modified keys like "!" won't parse; change the branches guarded by workspaceDigitFlags and surfaceDigitFlags to call a helper (resolvedDigitShortcutNumber(for:)) that derives the 1...9 digit from the unshifted key (use the event's keyCode or a layout-aware unshifted character lookup rather than charactersIgnoringModifiers) and return an Int 1–9 or nil; then feed that Int into WorkspaceShortcutMapper.workspaceIndex(forCommandDigit:) and tabManager.selectTab(at:)/tabManager.selectSurface(at:)/tabManager.selectLastSurface() as appropriate, leaving the existing flag checks (workspaceDigitFlags, surfaceDigitFlags) intact.
🧹 Nitpick comments (1)
Sources/KeyboardShortcutSettings.swift (1)
319-326: Modifier key display names should be localized.The
displayName(for:)helper returns hardcoded English strings ("Control", "Option", "Shift", "Command"). If this method is used in user-facing UI, these strings should be localized per the project's i18n requirements.Suggested localization
static func displayName(for flags: NSEvent.ModifierFlags) -> String { var parts: [String] = [] - if flags.contains(.control) { parts.append("Control") } - if flags.contains(.option) { parts.append("Option") } - if flags.contains(.shift) { parts.append("Shift") } - if flags.contains(.command) { parts.append("Command") } + if flags.contains(.control) { parts.append(String(localized: "modifier.control", defaultValue: "Control")) } + if flags.contains(.option) { parts.append(String(localized: "modifier.option", defaultValue: "Option")) } + if flags.contains(.shift) { parts.append(String(localized: "modifier.shift", defaultValue: "Shift")) } + if flags.contains(.command) { parts.append(String(localized: "modifier.command", defaultValue: "Command")) } return parts.joined(separator: "+") }As per coding guidelines: "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 319 - 326, The displayName(for:) helper currently returns hardcoded English modifier names; update it to use localized user-facing strings instead. Replace the literal "Control", "Option", "Shift", and "Command" values with localized lookups using String(localized: "key.control", defaultValue: "Control"), String(localized: "key.option", defaultValue: "Option"), String(localized: "key.shift", defaultValue: "Shift"), and String(localized: "key.command", defaultValue: "Command") respectively so the ModifierFlags display (in displayName(for:)) is i18n-compliant; keep the same join logic on parts.joined(separator: "+").
🤖 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 57406-57412: Update the Japanese translations for the keys
settings.shortcuts.showHints.subtitleOff and
settings.shortcuts.showHints.subtitleOn to match the new generic English
phrasing ("Modifier-hold" / "workspace or surface modifier key") or mark them as
needs_review; locate the entries for those string keys in
Resources/Localizable.xcstrings and either replace the specific key mentions
("CmdまたはCtrl", "Cmd（サイドバー/タイトルバー）またはCtrl/Cmd（ペインタブ）") with a generic Japanese
equivalent for "modifier-hold"/"workspace or surface modifier key" or change
their "state" to "needs_review" so translators can address them.
In `@Sources/cmuxApp.swift`:
- Around line 5283-5305: The current UI allows settingsWorkspaceDigitModifier
and settingsSurfaceDigitModifier to be set to the same flags, which causes
ambiguous duplicate actions; update the onChange handlers for the
ModifierRecorderRow instances (the ones bound to settingsWorkspaceDigitModifier
and settingsSurfaceDigitModifier) to detect when the new flags equal the other
modifier and prevent/save a resolved state: either reject the change by
restoring the previous value and surface a UX message/alert, or implement
deterministic precedence by automatically clearing or changing the other
modifier before persisting and still posting
DigitShortcutModifierSettings.didChangeNotification; ensure the check uses the
same equality logic used for the warning text and that the change is atomic so
callers always receive a non-ambiguous configuration.
- Around line 5271-5274: The UI copy claims hints show on the "workspace or
surface modifier key" but the implementation
(ShortcutHintModifierPolicy.shouldShowHints) only shows pills on Command-hold;
update the localized subtitle strings used with showShortcutHintsOnCommandHold
to explicitly say "Command (⌘) hold" when on and "Modifier-hold shortcut hint
pills are hidden" when off, or alternatively adjust the copy to match the actual
behavior, referencing the showShortcutHintsOnCommandHold boolean and
ShortcutHintModifierPolicy.shouldShowHints so the text and logic remain
consistent.
In `@Sources/ContentView.swift`:
- Around line 10706-10710: The code converts a possibly-negative Int
(workspaceDigitModifierStored) to UInt which can trap at runtime; update the
guard to ensure the stored value is positive (e.g., check
workspaceDigitModifierStored > 0) before performing UInt(...) and falling back
to DigitShortcutModifierSettings.defaultWorkspaceFlags otherwise, so
symbolString(for:) receives a safe ModifierFlags; apply the same fix pattern to
the identical conversions in KeyboardShortcutSettings.swift's eventModifiers()
and flagsFor() functions.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 9491-9515: The digit parsing currently uses
charactersIgnoringModifiers (chars) so Shift-modified keys like "!" won't parse;
change the branches guarded by workspaceDigitFlags and surfaceDigitFlags to call
a helper (resolvedDigitShortcutNumber(for:)) that derives the 1...9 digit from
the unshifted key (use the event's keyCode or a layout-aware unshifted character
lookup rather than charactersIgnoringModifiers) and return an Int 1–9 or nil;
then feed that Int into WorkspaceShortcutMapper.workspaceIndex(forCommandDigit:)
and
tabManager.selectTab(at:)/tabManager.selectSurface(at:)/tabManager.selectLastSurface()
as appropriate, leaving the existing flag checks (workspaceDigitFlags,
surfaceDigitFlags) intact.
---
Nitpick comments:
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 319-326: The displayName(for:) helper currently returns hardcoded
English modifier names; update it to use localized user-facing strings instead.
Replace the literal "Control", "Option", "Shift", and "Command" values with
localized lookups using String(localized: "key.control", defaultValue:
"Control"), String(localized: "key.option", defaultValue: "Option"),
String(localized: "key.shift", defaultValue: "Shift"), and String(localized:
"key.command", defaultValue: "Command") respectively so the ModifierFlags
display (in displayName(for:)) is i18n-compliant; keep the same join logic on
parts.joined(separator: "+").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f28ea14-753b-49dc-bf86-ecb4577f1bda
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swift
Comment on lines
+5271
to
+5274
| String(localized: "settings.shortcuts.showHints", defaultValue: "Show Modifier-Hold Shortcut Hints"), | ||
| subtitle: showShortcutHintsOnCommandHold | ||
| ? String(localized: "settings.shortcuts.showHints.subtitleOn", defaultValue: "Holding Cmd (sidebar/titlebar) or Ctrl/Cmd (pane tabs) shows shortcut hint pills.") | ||
| : String(localized: "settings.shortcuts.showHints.subtitleOff", defaultValue: "Holding Cmd or Ctrl keeps shortcut hint pills hidden.") | ||
| ? String(localized: "settings.shortcuts.showHints.subtitleOn", defaultValue: "Holding the workspace or surface modifier key shows shortcut hint pills.") | ||
| : String(localized: "settings.shortcuts.showHints.subtitleOff", defaultValue: "Modifier-hold shortcut hint pills are hidden.") |
There was a problem hiding this comment.
This hint-copy is ahead of the implementation.
Unless ShortcutHintModifierPolicy also changed elsewhere, the app still shows these pills on Command-hold only, so “workspace or surface modifier key” is misleading for Control/Option/Shift configs.
Based on learnings: ShortcutHintModifierPolicy.shouldShowHints is Command-only; Control-hold must not show sidebar/titlebar shortcut hints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 5271 - 5274, The UI copy claims hints
show on the "workspace or surface modifier key" but the implementation
(ShortcutHintModifierPolicy.shouldShowHints) only shows pills on Command-hold;
update the localized subtitle strings used with showShortcutHintsOnCommandHold
to explicitly say "Command (⌘) hold" when on and "Modifier-hold shortcut hint
pills are hidden" when off, or alternatively adjust the copy to match the actual
behavior, referencing the showShortcutHintsOnCommandHold boolean and
ShortcutHintModifierPolicy.shouldShowHints so the text and logic remain
consistent.
Comment on lines
+5283
to
+5305
| ModifierRecorderRow( | ||
| label: String(localized: "settings.shortcuts.workspaceDigitModifier", defaultValue: "Workspace 1-9 Modifier"), | ||
| subtitle: String(localized: "settings.shortcuts.workspaceDigitModifier.subtitle", defaultValue: "Modifier key(s) for jumping to workspace by number."), | ||
| storedFlags: $settingsWorkspaceDigitModifier, | ||
| defaultFlags: DigitShortcutModifierSettings.defaultWorkspaceFlags | ||
| ) | ||
| .onChange(of: settingsWorkspaceDigitModifier) { _ in | ||
| NotificationCenter.default.post(name: DigitShortcutModifierSettings.didChangeNotification, object: nil) | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| ModifierRecorderRow( | ||
| label: String(localized: "settings.shortcuts.surfaceDigitModifier", defaultValue: "Surface 1-9 Modifier"), | ||
| subtitle: settingsSurfaceDigitModifier == settingsWorkspaceDigitModifier | ||
| ? String(localized: "settings.shortcuts.digitModifierConflict", defaultValue: "Warning: same modifier as workspace shortcuts. Both will fire on the same key.") | ||
| : String(localized: "settings.shortcuts.surfaceDigitModifier.subtitle", defaultValue: "Modifier key(s) for jumping to surface by number."), | ||
| storedFlags: $settingsSurfaceDigitModifier, | ||
| defaultFlags: DigitShortcutModifierSettings.defaultSurfaceFlags | ||
| ) | ||
| .onChange(of: settingsSurfaceDigitModifier) { _ in | ||
| NotificationCenter.default.post(name: DigitShortcutModifierSettings.didChangeNotification, object: nil) | ||
| } |
There was a problem hiding this comment.
Don’t allow a workspace/surface modifier collision.
The warning text already says both shortcut families fire on the same key. Persisting and broadcasting that configuration leaves digit navigation ambiguous, because one keypress drives two independent actions. Reject duplicate modifier sets here, or define a hard precedence instead of only warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 5283 - 5305, The current UI allows
settingsWorkspaceDigitModifier and settingsSurfaceDigitModifier to be set to the
same flags, which causes ambiguous duplicate actions; update the onChange
handlers for the ModifierRecorderRow instances (the ones bound to
settingsWorkspaceDigitModifier and settingsSurfaceDigitModifier) to detect when
the new flags equal the other modifier and prevent/save a resolved state: either
reject the change by restoring the previous value and surface a UX
message/alert, or implement deterministic precedence by automatically clearing
or changing the other modifier before persisting and still posting
DigitShortcutModifierSettings.didChangeNotification; ensure the check uses the
same equality logic used for the warning text and that the change is atomic so
callers always receive a non-ambiguous configuration.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmuxTests/ShortcutAndCommandPaletteTests.swift (1)
532-539: Consider renaming this test to reflect both modifiers.Line [532] currently implies Command-only behavior, but the test now validates both workspace and surface default modifiers.
✏️ Suggested rename
- func testCommandHintDefaultsToEnabledWhenSettingMissing() { + func testShortcutHintsDefaultToEnabledForWorkspaceAndSurfaceModifiersWhenSettingMissing() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/ShortcutAndCommandPaletteTests.swift` around lines 532 - 539, Rename the test function testCommandHintDefaultsToEnabledWhenSettingMissing to reflect that it verifies both command and control modifiers; update the function name (e.g., testCommandAndControlHintDefaultsToEnabledWhenSettingMissing or testHintDefaultsToEnabledForCommandAndControlWhenSettingMissing) and update any references to the test name, keeping the body that calls ShortcutHintModifierPolicy.shouldShowHints(for: [.command], defaults: defaults) and shouldShowHints(for: [.control], defaults: defaults) and the reference to ShortcutHintDebugSettings.showHintsOnCommandHoldKey unchanged.cmuxTests/DigitShortcutModifierTests.swift (1)
15-24: Extract duplicatedwithDefaultsfixture setup into one shared helper.Both test classes duplicate the same isolated
UserDefaultssetup/teardown logic. Consolidating it will reduce drift risk and keep future fixture changes in one place.♻️ Suggested refactor
+private func withIsolatedDefaults( + suitePrefix: String, + file: StaticString = `#filePath`, + line: UInt = `#line`, + _ body: (UserDefaults) -> Void +) { + let suiteName = "\(suitePrefix)-\(UUID().uuidString)" + guard let defaults = UserDefaults(suiteName: suiteName) else { + XCTFail("Failed to create defaults suite", file: file, line: line) + return + } + defaults.removePersistentDomain(forName: suiteName) + defer { defaults.removePersistentDomain(forName: suiteName) } + body(defaults) +}Then call it from each class (or directly in tests) instead of maintaining two copies.
Also applies to: 170-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/DigitShortcutModifierTests.swift` around lines 15 - 24, Duplicate isolated UserDefaults setup/teardown logic is present; extract the withDefaults helper into a single shared test utility and have both test classes call it. Create one shared helper function (e.g., withDefaults) that takes a closure (UserDefaults) -> Void, generates a unique suiteName via UUID, creates and cleans the UserDefaults suite with removePersistentDomain before and after calling the closure, and replace the duplicated implementations in both test files to call this shared helper instead of redefining the suite creation/teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/DigitShortcutModifierTests.swift`:
- Around line 15-24: Duplicate isolated UserDefaults setup/teardown logic is
present; extract the withDefaults helper into a single shared test utility and
have both test classes call it. Create one shared helper function (e.g.,
withDefaults) that takes a closure (UserDefaults) -> Void, generates a unique
suiteName via UUID, creates and cleans the UserDefaults suite with
removePersistentDomain before and after calling the closure, and replace the
duplicated implementations in both test files to call this shared helper instead
of redefining the suite creation/teardown.
In `@cmuxTests/ShortcutAndCommandPaletteTests.swift`:
- Around line 532-539: Rename the test function
testCommandHintDefaultsToEnabledWhenSettingMissing to reflect that it verifies
both command and control modifiers; update the function name (e.g.,
testCommandAndControlHintDefaultsToEnabledWhenSettingMissing or
testHintDefaultsToEnabledForCommandAndControlWhenSettingMissing) and update any
references to the test name, keeping the body that calls
ShortcutHintModifierPolicy.shouldShowHints(for: [.command], defaults: defaults)
and shouldShowHints(for: [.control], defaults: defaults) and the reference to
ShortcutHintDebugSettings.showHintsOnCommandHoldKey unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c4212e1-414f-49b2-90b2-9be6deaddcf9
📒 Files selected for processing (3)
GhosttyTabs.xcodeproj/project.pbxprojcmuxTests/DigitShortcutModifierTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
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.
Summary
The default tab modifer is control, which is also the default modifier for changing desktops in macos. I'd like to be able to change it. So I made both configurable, you can now configure tab and workspace modifier keys
it should resolve this issue: #645
manaflow-ai/bonsplit#32
Testing
I verified it all works manually, however it doesn't fully work to show the tabs up top, that will require a bonesplit change. I am going to submit a PR here.
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Screen.Recording.2026-03-19.at.12.43.39.PM.mov
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Make the 1–9 shortcuts configurable so users can choose the modifier key(s) for switching workspaces and surfaces. Replaces the hardcoded Cmd/Ctrl behavior, updates hints and menus to match, and applies changes live.
Written for commit 2b0a2e7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Localization
Tests