-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: Add shortcut hint to auto-approve dropdown #8849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Review SummaryI've reviewed the PR and found 2 minor issues to address:
The implementation looks good overall. The shortcut hint feature is well-integrated and uses proper i18n patterns. |
| } | ||
|
|
||
| export const ShortcutHint: React.FC<ShortcutHintProps> = ({ translationKey }) => { | ||
| const { t } = useAppTranslation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useAppTranslation hook is imported but never used. The component only uses the Trans component directly with the translationKey prop. Consider removing this unused import to keep the code clean.
| export function usePlatform(): Platform { | ||
| const [platform, setPlatform] = useState<Platform>("unknown") | ||
|
|
||
| useEffect(() => { | ||
| const userAgent = window.navigator.userAgent.toLowerCase() | ||
| if (userAgent.includes("mac")) { | ||
| setPlatform("mac") | ||
| } else if (userAgent.includes("win")) { | ||
| setPlatform("windows") | ||
| } else if (userAgent.includes("linux")) { | ||
| setPlatform("linux") | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hook initializes platform state as "unknown" and updates it in useEffect, causing the component to render twice (first with "unknown", then with the actual platform). While this works in VSCode webviews, consider using useMemo with direct platform detection to avoid the initial render with incorrect state, especially if this hook is reused in SSR contexts later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review complete. Found 2 minor issues that should be addressed before merging.
This PR adds a visual hint for the keyboard shortcut to the auto-approve dropdown menu.
Important
Adds a keyboard shortcut hint to the auto-approve dropdown using a new
ShortcutHintcomponent andusePlatformhook.ShortcutHintcomponent to display keyboard shortcut inAutoApproveDropdown.tsx.usePlatformhook to determine platform-specific shortcuts ("⌘⌥A" for Mac, "Ctrl+Alt+A" for Windows/Linux).toggleShortcutkey tochat.jsonfor shortcut hint translation.ShortcutHint.tsxfor rendering shortcut hints.usePlatform.tsfor detecting user platform.This description was created by
for f37a5eb. You can customize this summary. It will automatically update as commits are pushed.