Conversation
|
@maucher 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. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 848-856: The current early-return logic uses
resolvedRecordedShortcutIgnoringConflicts(...) and then falls back to returning
the original shortcut when rejected, which lets invalid .showHideAllWindows
bindings slip through; update the fallback to also drop rejected
.showHideAllWindows the same way storedShortcutForPersistence(...) does by
returning nil for actions that use numbered digit matching or are
.showHideAllWindows when the resolvedRecordedShortcutIgnoringConflicts(...) did
not accept the shortcut (i.e., change the final return to check
action.usesNumberedDigitMatching || action == .showHideAllWindows and return nil
in that case).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f4a1858-041e-4181-9849-c5e3f9dbcec0
📒 Files selected for processing (1)
Sources/KeyboardShortcutSettingsFileStore.swift
Greptile SummaryThis PR fixes a launch crash ( Confidence Score: 5/5Safe to merge — targeted fix with no behavioral regression. The early-return for No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[parseShortcutBindingValue called during dispatch_once init] --> B{action == .showHideAllWindows?}
B -- Yes, NEW --> C[return shortcut immediately\nno store re-entry]
B -- No --> D[resolvedRecordedShortcutIgnoringConflicts]
D -- accepted --> E[return normalized shortcut]
D -- rejected --> F{usesNumberedDigitMatching?}
F -- true --> G[return nil, discard binding]
F -- false --> H[return raw shortcut]
B -- Yes, BEFORE FIX --> X[normalizedSystemWideHotkeyShortcutResult\nreservedSystemWideHotkeyShortcuts\nshortcut for action\noverride for action\nCmuxSettingsFileStore.shared]
X --> Z[recursive dispatch_once trap]
Reviews (2): Last reviewed commit: "fix: avoid recursive dispatch_once deadl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…tive shortcuts During CmuxSettingsFileStore.shared initialization, parseShortcutBindingValue called action.normalizedRecordedShortcut(), which called conflictingAction(), which accessed KeyboardShortcutSettings.settingsFileStore — the very singleton whose dispatch_once was still on the stack. This caused a recursive lock trap: BUG IN CLIENT OF LIBDISPATCH: trying to lock recursively Fix: accept the raw shortcut during settings file parsing; skip all conflict checks (including showHideAllWindows system-wide hotkey conflicts) which would re-enter the not-yet-initialized store. The Settings UI validates with full conflict detection post-init. Also trims comments from parsed shortcut values. Fixes manaflow-ai#3412 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
492418a to
91416c8
Compare
|
@greptileai review |
|
The fix from this PR has been superseded by the upstream implementation in |
1 participant
Summary
EXC_BREAKPOINT / SIGTRAP) that affects any user with an activeshortcuts.bindingsentry in~/.config/cmux/settings.json.parseShortcutBindingValuecalledaction.normalizedRecordedShortcut()duringCmuxSettingsFileStore.sharedinitialization, which internally calledconflictingAction(), which accessedKeyboardShortcutSettings.settingsFileStore— the very singleton whosedispatch_oncewas still on the stack — causing a recursive lock trap (BUG IN CLIENT OF LIBDISPATCH: trying to lock recursively).action.resolvedRecordedShortcutIgnoringConflicts(), which retains format validation (numbered digits, system-wide hotkey modifier) but skips the conflict check. Skipping conflicts during file parsing is also semantically correct — the file defines the overrides, so checking against the not-yet-initialized store is meaningless. The interactive conflict check in the Settings UI is unaffected.Fixes #3412
Testing
shortcuts.bindingsentry insettings.jsonon macOS 15.7.5.settings.json.shortcutsblock unblocks the nightly without the fix.Demo Video
No UI change — crash fix only.
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Fixes a launch crash when
~/.config/cmux/settings.jsonhas activeshortcuts.bindingsby skipping conflict checks during startup parsing to avoid a recursivedispatch_oncedeadlock. Accepts the raw.showHideAllWindowsshortcut, validates format, and trims inline comments; the Settings UI still checks conflicts after init. Fixes #3412.action.normalizedRecordedShortcut(...)withaction.resolvedRecordedShortcutIgnoringConflicts(...)and special-cased.showHideAllWindowsto avoid re-enteringKeyboardShortcutSettings.settingsFileStoreduringCmuxSettingsFileStoreinit.parseShortcutBindingValueso entries with trailing comments parse correctly.Written for commit 91416c8. Summary will update on new commits.
Summary by CodeRabbit