Skip to content

fix(renderer): improve shortcut hint rendering#1949

Open
janburzinski wants to merge 2 commits intogeneralaction:mainfrom
janburzinski:emdash/go-back-popover-yywpi
Open

fix(renderer): improve shortcut hint rendering#1949
janburzinski wants to merge 2 commits intogeneralaction:mainfrom
janburzinski:emdash/go-back-popover-yywpi

Conversation

@janburzinski
Copy link
Copy Markdown
Collaborator

summary

adjust go back/forward

image

@janburzinski janburzinski marked this pull request as ready for review May 9, 2026 18:36
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 9, 2026

Greptile Summary

This PR replaces the fragile formatForDisplay + string-split approach in ShortcutHint with structured parseHotkey, rendering modifiers and the main key as individual Kbd elements with proper platform-aware symbols. KbdGroup is also updated to visually merge adjacent keys into a seamless pill.

  • shortcut-hint.tsx: Switches from splitting a display string on + to using parseHotkey from @tanstack/react-hotkeys; modifiers are resolved via MAC_MODIFIER_SYMBOLS/STANDARD_MODIFIER_LABELS with ?? modifier fallbacks, and the main key via KEY_DISPLAY_SYMBOLS[parsed.key] ?? parsed.key.
  • kbd.tsx: KbdGroup drops gap-1 in favour of gap-0 with child-combinator selectors that strip border-radius and horizontal padding from interior keys, restoring them only on the first and last child to create a unified look.

Confidence Score: 5/5

Safe to merge — changes are purely presentational with no logic regressions.

Both files touch only UI rendering: parseHotkey replaces a fragile string-split, fallbacks are present for unknown modifiers, and the KbdGroup CSS change is additive. No data flow, state management, or API contracts are touched.

No files require special attention.

Important Files Changed

Filename Overview
src/renderer/lib/ui/shortcut-hint.tsx Replaces string-splitting via formatForDisplay with structured parseHotkey — modifiers and main key are rendered as separate Kbd elements with platform-aware symbols and proper fallbacks.
src/renderer/lib/ui/kbd.tsx KbdGroup now fuses child Kbd items into a seamless pill by zeroing the gap and selectively applying rounded corners and padding only to the first/last child.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ShortcutHint] --> B{hotkey?}
    B -- no --> C[return null]
    B -- yes --> D[parseHotkey hotkey PLATFORM]
    D --> E[parsed.modifiers + parsed.key]
    E --> F{IS_MAC?}
    F -- yes --> G[MAC_MODIFIER_SYMBOLS glyph wrap in translate-y-px span]
    F -- no --> H[STANDARD_MODIFIER_LABELS glyph render plain text]
    G --> I[KbdGroup]
    H --> I
    E --> J[KEY_DISPLAY_SYMBOLS parsed.key]
    J --> I
    I --> K[CSS: gap-0, first-child rounded-l pl-1 last-child rounded-r pr-1]
Loading

Reviews (2): Last reviewed commit: "fix(renderer): address shortcut hint rev..." | Re-trigger Greptile

Comment thread src/renderer/lib/ui/shortcut-hint.tsx Outdated
Comment on lines +36 to +43
{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>
);
})}
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.

Comment thread src/renderer/lib/ui/shortcut-hint.tsx Outdated
Comment on lines +37 to +39
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.

@janburzinski
Copy link
Copy Markdown
Collaborator Author

@greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant