Add Writing / Shortcuts / Apps panes to redesigned Settings#366
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @State private var isRecordingKeybind = false | ||
| @State private var isRecordingFullAcceptKeybind = false |
There was a problem hiding this comment.
isRecordingKeybind and isRecordingFullAcceptKeybind are independent, so both can be true simultaneously. When that happens, both KeyRecorderView instances call NSEvent.addLocalMonitorForEvents. Local monitors fire in LIFO order, so whichever "Change" button was clicked second will capture the next key — even if the user intended it for the first row. The first KeyRecorderView then stays in "Press a key…" state until a further key is pressed, silently recording a second shortcut the user didn't intend. Setting the other flag to false (or using a single activeRecorder enum) when one starts recording would prevent this.
| private struct KeybindRow: View { | ||
| let label: String | ||
| let keyCode: CGKeyCode | ||
| let modifiers: ShortcutModifierMask | ||
| let defaultKeyCode: CGKeyCode | ||
| @Binding var isRecording: Bool | ||
| let onRecord: (CGKeyCode, ShortcutModifierMask, String) -> Void | ||
| let onReset: () -> Void | ||
| let onClear: () -> Void | ||
| let clearHelp: String |
There was a problem hiding this comment.
clearHelp parameter
clearHelp: String is declared in KeybindRow's stored properties and passed by both call sites, but it is never referenced anywhere in body. The PR description notes tooltip removal happened in #362, so this was almost certainly meant to appear as .help(clearHelp) on the Clear button. As-is, the parameter is dead weight and the Clear button is left with no help text for VoiceOver or on-hover display.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Re-opened after base branch deletion. Rebased onto current main (includes tooltip removal #362), cotabbyHelp calls stripped. Build/lint clean locally.
Greptile Summary
This PR promotes the Writing, Shortcuts, and Apps settings panes from placeholder stubs to fully implemented views, completing three legs of the redesigned Settings window. Each pane is lifted from the corresponding section of the legacy
SettingsViewso behavior is preserved, andSettingsContainerViewsimply swaps the threePlaceholderPaneViewcases for the real implementations.WritingPaneViewandAppsPaneVieware clean lifts: word-count picker, profile fields, language/rules editors, and the disabled-app list (with NSOpenPanel picker) all match the legacy code with no behavioral changes.ShortcutsPaneViewintroduces two independent@Staterecording booleans. If both "Change" buttons are activated without completing the first recording, bothKeyRecorderViewinstances installNSEventlocal monitors simultaneously; LIFO ordering means the second-installed monitor captures the next key, potentially recording the wrong binding. Additionally, theKeybindRow.clearHelpparameter is accepted at every call site but never rendered anywhere in the view.Confidence Score: 3/5
Safe to merge for most users, but the shortcut recorder can silently assign a key to the wrong binding if both Change buttons are tapped without completing the first recording.
The Writing and Apps panes are faithful, straightforward lifts with nothing new to break. The Shortcuts pane has two independent recording-state booleans with no mutual-exclusion logic; when both are true at once, the two NSEvent local monitors compete in LIFO order, and the user's intended key gets recorded by whichever row most recently started recording — not necessarily the one they were targeting. The first recorder then silently stays open until another key is pressed. This is a present, reproducible mis-assignment of a persisted user setting rather than a hypothetical edge case.
Cotabby/UI/Settings/Panes/ShortcutsPaneView.swift — specifically the dual isRecording state and the unused clearHelp parameter in KeybindRow.
Important Files Changed
clearHelpparameter.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD SCV[SettingsContainerView\ndetailPane switch] --> WPV[WritingPaneView\nLength · Name · Languages · Rules] SCV --> SPV[ShortcutsPaneView\nAccept Word · Accept Entire] SCV --> APV[AppsPaneView\nDisabled App Rules] SPV --> KR1[KeybindRow\nAccept Word] SPV --> KR2[KeybindRow\nAccept Entire Suggestion] KR1 -->|isRecording=true| KRV1[KeyRecorderView\nNSEvent monitor 1] KR2 -->|isRecording=true| KRV2[KeyRecorderView\nNSEvent monitor 2] KRV1 -.->|LIFO: fires after monitor 2| CONFLICT((⚠️ Both active\nLIFO conflict)) KRV2 -.->|LIFO: fires first| CONFLICT APV --> FP[NSOpenPanel\nApp File Picker] APV --> RL[Disabled Rules List\nwith remove button]Reviews (1): Last reviewed commit: "Add Writing / Shortcuts / Apps panes to ..." | Re-trigger Greptile