Show tooltip with shortcut for chat, remove icon from Dock#320
Conversation
gsabran
commented
Dec 16, 2025
|
@codex PTAL |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the user experience by adding a chat tooltip that displays the keyboard shortcut for opening chat, and improves the app's UI presence by removing the dock icon (making it accessible only via the menu bar). The default keyboard shortcuts are updated from ⌘ I to ⌘ L for consistency.
Key changes:
- Added a chat tooltip that appears after 1 second when no code completion is active, showing the shortcut to open chat
- Changed default keyboard shortcuts from
⌘ I/⌘ ⇧ Ito⌘ L/⌘ ⇧ L - Hid the dock icon by setting the app's activation policy to
.accessory
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SourceEditorObserver.swift | Removed debug print statement for cleaner logs |
| SettingsService.swift | Added display property to KeyboardShortcut for formatting shortcuts and updated default shortcuts from "i" to "l" |
| KeyboardShortcutsSettingsView.swift | Refactored display logic to use the new shared display property and updated KeyBindingInputView to accept defaultValue parameter |
| AboutSettingsView.swift | Added build type indicator ("DEBUG") for debug builds to help identify the app configuration |
| CodeCompletionWindow.swift | Refactored positioning logic to support both code completion and chat tooltip, extracting common code into shared methods |
| CodeCompletionViewModel.swift | Added chat tooltip state management including visibility logic with 1-second delay, and moved xcodeObservation initialization to init |
| CodeCompletionView.swift | Added UI rendering for the chat tooltip with fade-in animation and trailing alignment |
| App.swift | Set app activation policy to .accessory to hide the dock icon |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.modifiers = modifiers | ||
| } | ||
|
|
||
| /// A string representation of the keyboard shortcut (e.g. "⌘ L"). |
There was a problem hiding this comment.
The documentation example "⌘ L" is now accurate since the default shortcut was changed from "i" to "l". However, to avoid confusion, consider updating the example to use a generic key like "⌘ K" since "⌘ L" is now actually one of the default shortcuts in this codebase. This would make it clearer that it's just an example format.
| /// A string representation of the keyboard shortcut (e.g. "⌘ L"). | |
| /// A string representation of the keyboard shortcut (e.g. "⌘ K"). |
| borderWidth: 0.5) | ||
| .onChange(of: keyboardShortcut) { newValue in | ||
| inputShortcut = NSAttributedString(string: newValue?.display ?? "") | ||
| inputShortcut = NSAttributedString(string: (newValue ?? defaultValue).display) |
There was a problem hiding this comment.
When the delete key is pressed to clear the shortcut, the inputShortcut is first set to an empty string in the onKeyDown handler (line 133), but then when keyboardShortcut is set to nil (line 134), this onChange handler will immediately override it with the defaultValue.display (line 145). This means the user will see the default shortcut displayed instead of an empty field, which is confusing UX. The old code had the same behavior (it used an empty string), but now with the refactored code that always shows the default when nil, this creates an inconsistency. Consider checking if the new value is nil in the onChange handler and preserving the empty string in that case.
| inputShortcut = NSAttributedString(string: (newValue ?? defaultValue).display) | |
| if let shortcut = newValue { | |
| inputShortcut = NSAttributedString(string: shortcut.display) | |
| } |
| if completion != nil { | ||
| screenShotEditorIfNeeded() | ||
| } else if showChatTooltip, oldValue != nil { | ||
| // When changed, hide the tooltip instead of showing having a jaggy scrolling |
There was a problem hiding this comment.
The phrase "instead of showing having a jaggy scrolling" is grammatically incorrect. It should be "instead of having jaggy scrolling" or "instead of showing a jaggy scrolling effect".
| // When changed, hide the tooltip instead of showing having a jaggy scrolling | |
| // When changed, hide the tooltip instead of having jaggy scrolling |
542b0c9 to
7579f5f
Compare