Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/renderer/lib/ui/kbd.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ function KbdGroup({ className, ...props }: React.ComponentProps<'div'>) {
return (
<kbd
data-slot="kbd-group"
className={cn('inline-flex items-center gap-1', className)}
className={cn(
'inline-flex items-center gap-0 [&>[data-slot=kbd]]:min-w-0 [&>[data-slot=kbd]]:rounded-none [&>[data-slot=kbd]]:px-0 [&>[data-slot=kbd]:first-child]:rounded-l-sm [&>[data-slot=kbd]:first-child]:pl-1 [&>[data-slot=kbd]:last-child]:rounded-r-sm [&>[data-slot=kbd]:last-child]:pr-1',
className
)}
{...props}
/>
);
Expand Down
31 changes: 24 additions & 7 deletions src/renderer/lib/ui/shortcut-hint.tsx
Original file line number Diff line number Diff line change
@@ -1,30 +1,47 @@
import { formatForDisplay } from '@tanstack/react-hotkeys';
import {
detectPlatform,
KEY_DISPLAY_SYMBOLS,
MAC_MODIFIER_SYMBOLS,
parseHotkey,
STANDARD_MODIFIER_LABELS,
} from '@tanstack/react-hotkeys';
import React from 'react';
import { useAppSettingsKey } from '@renderer/features/settings/use-app-settings-key';
import {
getEffectiveHotkey,
type ShortcutSettingsKey,
} from '@renderer/lib/hooks/useKeyboardShortcuts';
import { cn } from '@renderer/utils/utils';
import { Kbd, KbdGroup } from './kbd';

interface ShortcutHintProps {
settingsKey: ShortcutSettingsKey;
className?: string;
}

const PLATFORM = detectPlatform();
const IS_MAC = PLATFORM === 'mac';

export const ShortcutHint: React.FC<ShortcutHintProps> = ({ settingsKey, className }) => {
const { value: keyboard } = useAppSettingsKey('keyboard');
const hotkey = getEffectiveHotkey(settingsKey, keyboard);
const display = hotkey ? formatForDisplay(hotkey) : '';

if (!display) return null;
if (!hotkey) return null;

const parsed = parseHotkey(hotkey, PLATFORM);

return (
<span className={`flex items-center gap-1 text-xs text-muted-foreground ${className ?? ''}`}>
<span className={cn('flex items-center gap-1 text-xs text-muted-foreground', className)}>
<KbdGroup>
{display.split('+').map((key, _) => (
<Kbd key={key}>{key.trim()}</Kbd>
))}
{parsed.modifiers.map((modifier, idx) => {
const glyph = IS_MAC
? MAC_MODIFIER_SYMBOLS[modifier]
: STANDARD_MODIFIER_LABELS[modifier];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 glyph has no fallback when a modifier is absent from MAC_MODIFIER_SYMBOLS or STANDARD_MODIFIER_LABELS. If parseHotkey ever surfaces an unexpected modifier name (e.g. 'Mod' before it is resolved, or a future library addition), glyph is undefined and an empty <Kbd> renders in the group. Adding ?? modifier as a last-resort fallback — mirroring what KEY_DISPLAY_SYMBOLS[parsed.key] ?? parsed.key already does for the main key — keeps the output legible instead of blank.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/ui/shortcut-hint.tsx
Line: 37-39

Comment:
`glyph` has no fallback when a modifier is absent from `MAC_MODIFIER_SYMBOLS` or `STANDARD_MODIFIER_LABELS`. If `parseHotkey` ever surfaces an unexpected modifier name (e.g. `'Mod'` before it is resolved, or a future library addition), `glyph` is `undefined` and an empty `<Kbd>` renders in the group. Adding `?? modifier` as a last-resort fallback — mirroring what `KEY_DISPLAY_SYMBOLS[parsed.key] ?? parsed.key` already does for the main key — keeps the output legible instead of blank.

How can I resolve this? If you propose a fix, please make it concise.

return (
<Kbd key={idx}>{IS_MAC ? <span className="translate-y-px">{glyph}</span> : glyph}</Kbd>
);
})}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Using the array index as the React key for modifier elements is fragile — if modifiers ever reorder, React will diff them incorrectly and potentially keep stale DOM. Each modifier string is already unique within the array (a hotkey can't have duplicate modifiers), so modifier itself is a stable, meaningful key.

Suggested change
{parsed.modifiers.map((modifier, idx) => {
const glyph = IS_MAC
? MAC_MODIFIER_SYMBOLS[modifier]
: STANDARD_MODIFIER_LABELS[modifier];
return (
<Kbd key={idx}>{IS_MAC ? <span className="translate-y-px">{glyph}</span> : glyph}</Kbd>
);
})}
{parsed.modifiers.map((modifier) => {
const glyph = IS_MAC
? MAC_MODIFIER_SYMBOLS[modifier]
: STANDARD_MODIFIER_LABELS[modifier];
return (
<Kbd key={modifier}>{IS_MAC ? <span className="translate-y-px">{glyph}</span> : glyph}</Kbd>
);
})}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/ui/shortcut-hint.tsx
Line: 36-43

Comment:
Using the array index as the React `key` for modifier elements is fragile — if modifiers ever reorder, React will diff them incorrectly and potentially keep stale DOM. Each modifier string is already unique within the array (a hotkey can't have duplicate modifiers), so `modifier` itself is a stable, meaningful key.

```suggestion
        {parsed.modifiers.map((modifier) => {
          const glyph = IS_MAC
            ? MAC_MODIFIER_SYMBOLS[modifier]
            : STANDARD_MODIFIER_LABELS[modifier];
          return (
            <Kbd key={modifier}>{IS_MAC ? <span className="translate-y-px">{glyph}</span> : glyph}</Kbd>
          );
        })}
```

How can I resolve this? If you propose a fix, please make it concise.

<Kbd>{KEY_DISPLAY_SYMBOLS[parsed.key] ?? parsed.key}</Kbd>
</KbdGroup>
</span>
);
Expand Down
Loading