feat(settings): improve shortcut editor ux#267
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds searchable sidebar for hotkey commands, reset-all confirmation dialog, and refactored capture lifecycle to preserve partial override state during conflict resolution. Updates binding display logic and UI refinements throughout the editor, plus comprehensive test coverage and i18n strings. ChangesHotkey Editor Search and Reset Confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR improves the shortcut editor UX with three main additions: a live search bar in the sidebar, a confirmation dialog before resetting all shortcuts, and per-conflict "Overwrite" buttons replacing the previous single "Overwrite All" action. Explicit unassigned bindings (empty string) are now surfaced in the UI and correctly round-tripped through import/export.
Confidence Score: 3/5Safe to merge if single-conflict overwrite is the dominant path; multi-conflict partial-overwrite-then-cancel will silently drop an existing binding. The per-conflict overwrite path writes unbinds to the store immediately and incrementally. If a user resolves one of two conflicts then presses Escape, the first binding is permanently lost even though the new assignment was never saved. All other changes — search, reset confirmation, i18n, unassigned display, export coverage — are solid and well-tested. src/features/settings/components/hotkey-editor.tsx — specifically the overwriteConflictingHotkey function and how stopCapture interacts with partial store mutations when multiple conflicts are present. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User presses key combo] --> B{Conflicts detected?}
B -- No --> C[canSaveCapture = true\nSave button enabled]
B -- Yes --> D[Show per-conflict Overwrite buttons]
D --> E{Multiple conflicts?}
E -- Yes --> F[Also show Overwrite All button]
E -- No --> G[Only per-item Overwrite]
F --> H[overwriteAllConflictingHotkeys\nUnbind all, save binding, stopCapture]
G --> I[overwriteConflictingHotkey key]
D --> I
I --> J[unbindHotkeyBinding key\ncompute remainingConflicts]
J --> K{remainingConflicts == 0?}
K -- Yes --> L[setHotkeyBinding + stopCapture]
K -- No --> M[Store updated, re-render\ncaptureConflicts shrinks by 1\n⚠️ partial unbind NOT rolled back on Escape]
M --> D
C --> N[User clicks Save]
N --> O[setHotkeyBinding + stopCapture]
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/features/settings/components/hotkey-editor-reset-dialog.test.tsx (1)
39-65: ⚡ Quick winReplace fixed retry loops with
waitFor/findBy*to reduce CI flakiness.
waitForText/waitForBodyTextuse only 10 zero-delay ticks; async UI updates can exceed this, causing nondeterministic failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/settings/components/hotkey-editor-reset-dialog.test.tsx` around lines 39 - 65, Replace the custom retry loops in waitForText and waitForBodyText with React Testing Library utilities to avoid flaky timing; specifically remove the 10-iteration zero-delay loops in the functions and instead use waitFor (or the appropriate findBy* queries) to await the DOM updates, e.g. use waitFor(() => expect(screen.getByText(...)).toBeInTheDocument()) or screen.findByText for waitForText and waitFor(() => expect(document.body).toHaveTextContent(...)) or screen.findByText scoped to body for waitForBodyText; update any tests that call waitForText/waitForBodyText to use the new waitFor/findBy* behavior so asynchronous UI updates are awaited reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/settings/components/hotkey-editor-reset-dialog.test.tsx`:
- Around line 1-7: The test currently uses vite-plus/test plus manual DOM setup
(createRoot) and brittle polling helpers; migrate it to Vitest + jsdom +
`@testing-library/react` by replacing imports from 'vite-plus/test' with vitest's
test helpers and using `@testing-library/react`'s render, screen, fireEvent (or
userEvent) and waitFor/findBy* for assertions; remove manual createRoot and the
custom getButton/click/keyDown/polling loops and instead render <HotkeyEditor>
wrapped with <TooltipProvider> and, if necessary, mock useSettingsStore via
vi.mock to control state, then use screen.findByText/screen.getByRole and
waitFor to simulate keyboard events and assert dialog open/close and text
changes (reference identifiers: HotkeyEditor, TooltipProvider, useSettingsStore,
any test helpers like getButton/getDialog used in the diff).
In `@src/features/settings/components/hotkey-editor-sections.ts`:
- Around line 58-67: bindings currently maps raw shortcut strings so empty
bindings ('') never match the translated "Unassigned" label; update the search
to treat empty keys as the UI label by mapping item.keys (or bindings) such that
when a key === '' you replace it with the translated/unassigned label
(lowercased) before comparing, and ensure both the item.keys.some(...) check and
the bindings.some(...) comparisons compare against normalizedBindingQuery using
the same transformed value; reference symbols: bindings, item.keys, hotkeys,
normalizedBindingQuery, normalizedQuery, itemLabel, sectionLabel.
In `@src/features/settings/components/hotkey-editor.tsx`:
- Around line 665-668: selectSearchResult currently stops capture and sets only
setSelectedKey(item.keys[0]), which leaves the UI scoped to the previous
section; update selectSearchResult to also sync the active section by calling
the state updater that controls the UI scope (e.g., setActiveLayer or
setActiveSection) with the item's section/layer (item.section or item.layer)
right after stopCapture, and make the same change in the other selection helper
at the other occurrence so the keyboard overlay, command list and details panel
all focus the chosen search result.
---
Nitpick comments:
In `@src/features/settings/components/hotkey-editor-reset-dialog.test.tsx`:
- Around line 39-65: Replace the custom retry loops in waitForText and
waitForBodyText with React Testing Library utilities to avoid flaky timing;
specifically remove the 10-iteration zero-delay loops in the functions and
instead use waitFor (or the appropriate findBy* queries) to await the DOM
updates, e.g. use waitFor(() =>
expect(screen.getByText(...)).toBeInTheDocument()) or screen.findByText for
waitForText and waitFor(() => expect(document.body).toHaveTextContent(...)) or
screen.findByText scoped to body for waitForBodyText; update any tests that call
waitForText/waitForBodyText to use the new waitFor/findBy* behavior so
asynchronous UI updates are awaited reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1835b62b-0678-48ed-95bf-05d10859b2a8
📒 Files selected for processing (6)
src/config/hotkeys.test.tssrc/features/settings/components/hotkey-editor-reset-dialog.test.tsxsrc/features/settings/components/hotkey-editor-search.test.tssrc/features/settings/components/hotkey-editor-sections.tssrc/features/settings/components/hotkey-editor.tsxsrc/i18n/locales/partials/projects.json
Comment on lines
+1
to
+7
| import { createElement } from 'react' | ||
| import * as DialogPrimitive from '@radix-ui/react-dialog' | ||
| import { createRoot, type Root } from 'react-dom/client' | ||
| import { afterEach, beforeEach, describe, expect, it } from 'vite-plus/test' | ||
| import { HotkeyEditor } from './hotkey-editor' | ||
| import { TooltipProvider } from '@/components/ui/tooltip' | ||
| import { useSettingsStore } from '../stores/settings-store' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/features/settings/components/hotkey-editor-reset-dialog.test.tsx"
echo "---- File exists and basic stats ----"
ls -l "$FILE" || true
wc -l "$FILE" || true
echo
echo "---- Imports section (first ~80 lines) ----"
sed -n '1,120p' "$FILE"
echo
echo "---- Check for forbidden patterns in this file ----"
rg -n "console\." "$FILE" || true
rg -n "\bany\b" "$FILE" || true
echo
echo "---- Check for `@testing-library/react` usage ----"
rg -n "`@testing-library/react`" "$FILE" || true
rg -n "user-event" "$FILE" || true
echo
echo "---- Check for vite-plus/test usage ----"
rg -n "vite-plus/test" "$FILE" || true
echo
echo "---- Inspect polling helpers (grep waitForText/waitForBodyText) ----"
rg -n "waitFor(Text|BodyText)|setTimeout\\(0\\)|attempt" "$FILE" || true
echo
echo "---- Inspect for manual query/click/keyDown/changeInput helpers ----"
rg -n "getButton|click\\(|keyDown\\(|changeInput\\(|document\\.querySelector|querySelectorAll" "$FILE" || trueRepository: walterlow/freecut
Length of output: 6299
Migrate this hotkey-editor-reset-dialog test to the required Vitest + jsdom + @testing-library/react`` stack.
- The test uses
vite-plus/testand manual DOM/event plumbing (createRoot+getButton/click/keyDownandwaitForText/waitForBodyText) instead of@testing-library/react-based testing (render,screen,findBy*/waitFor). - The custom polling helpers use fixed 10-attempt loops with
setTimeout(0), which is brittle and can cause CI flakiness.
Suggested direction (migration shape)
-import { createElement } from 'react'
-import { createRoot, type Root } from 'react-dom/client'
-import { afterEach, beforeEach, describe, expect, it } from 'vite-plus/test'
+import { describe, it, beforeEach, afterEach, expect } from 'vitest'
+import { render, screen, waitFor } from '`@testing-library/react`'
+import userEvent from '`@testing-library/user-event`'
...
-beforeEach(async () => {
- ...
- root = createRoot(container)
- root.render(...)
- await waitForText('Reset All')
-})
+beforeEach(async () => {
+ ...
+ render(
+ <TooltipProvider>
+ <DialogPrimitive.Root open>
+ <HotkeyEditor />
+ </DialogPrimitive.Root>
+ </TooltipProvider>,
+ )
+ await screen.findByRole('button', { name: 'Reset All' })
+})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/settings/components/hotkey-editor-reset-dialog.test.tsx` around
lines 1 - 7, The test currently uses vite-plus/test plus manual DOM setup
(createRoot) and brittle polling helpers; migrate it to Vitest + jsdom +
`@testing-library/react` by replacing imports from 'vite-plus/test' with vitest's
test helpers and using `@testing-library/react`'s render, screen, fireEvent (or
userEvent) and waitFor/findBy* for assertions; remove manual createRoot and the
custom getButton/click/keyDown/polling loops and instead render <HotkeyEditor>
wrapped with <TooltipProvider> and, if necessary, mock useSettingsStore via
vi.mock to control state, then use screen.findByText/screen.getByRole and
waitFor to simulate keyboard events and assert dialog open/close and text
changes (reference identifiers: HotkeyEditor, TooltipProvider, useSettingsStore,
any test helpers like getButton/getDialog used in the diff).
Comment on lines
+58
to
+67
| const bindings = item.keys.map((key) => hotkeys[key].toLowerCase()) | ||
|
|
||
| return ( | ||
| itemLabel.includes(normalizedQuery) || | ||
| sectionLabel.includes(normalizedQuery) || | ||
| item.keys.some((key) => key.toLowerCase().includes(normalizedQuery)) || | ||
| bindings.some( | ||
| (binding) => | ||
| binding.includes(normalizedQuery) || | ||
| (normalizedBindingQuery.length > 0 && binding === normalizedBindingQuery), |
Contributor
There was a problem hiding this comment.
Make explicit unassigned bindings searchable.
bindings only contains raw shortcut strings, so commands with '' bindings can never match the translated “Unassigned” label shown in the UI. A search for unassigned shortcuts will always return no results.
Suggested fix
.filter((item) => {
const itemLabel = translate(item.labelKey).toLowerCase()
- const bindings = item.keys.map((key) => hotkeys[key].toLowerCase())
+ const rawBindings = item.keys.map((key) => hotkeys[key].toLowerCase())
+ const displayBindings = item.keys.map((key) =>
+ getHotkeyBindingDisplayLabel(
+ hotkeys[key],
+ translate('projects.settings.hotkeys.unassigned'),
+ ).toLowerCase(),
+ )
return (
itemLabel.includes(normalizedQuery) ||
sectionLabel.includes(normalizedQuery) ||
item.keys.some((key) => key.toLowerCase().includes(normalizedQuery)) ||
- bindings.some(
+ rawBindings.some(
(binding) =>
binding.includes(normalizedQuery) ||
(normalizedBindingQuery.length > 0 && binding === normalizedBindingQuery),
- )
+ ) ||
+ displayBindings.some((binding) => binding.includes(normalizedQuery))
)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bindings = item.keys.map((key) => hotkeys[key].toLowerCase()) | |
| return ( | |
| itemLabel.includes(normalizedQuery) || | |
| sectionLabel.includes(normalizedQuery) || | |
| item.keys.some((key) => key.toLowerCase().includes(normalizedQuery)) || | |
| bindings.some( | |
| (binding) => | |
| binding.includes(normalizedQuery) || | |
| (normalizedBindingQuery.length > 0 && binding === normalizedBindingQuery), | |
| const rawBindings = item.keys.map((key) => hotkeys[key].toLowerCase()) | |
| const displayBindings = item.keys.map((key) => | |
| getHotkeyBindingDisplayLabel( | |
| hotkeys[key], | |
| translate('projects.settings.hotkeys.unassigned'), | |
| ).toLowerCase(), | |
| ) | |
| return ( | |
| itemLabel.includes(normalizedQuery) || | |
| sectionLabel.includes(normalizedQuery) || | |
| item.keys.some((key) => key.toLowerCase().includes(normalizedQuery)) || | |
| rawBindings.some( | |
| (binding) => | |
| binding.includes(normalizedQuery) || | |
| (normalizedBindingQuery.length > 0 && binding === normalizedBindingQuery), | |
| ) || | |
| displayBindings.some((binding) => binding.includes(normalizedQuery)) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/settings/components/hotkey-editor-sections.ts` around lines 58 -
67, bindings currently maps raw shortcut strings so empty bindings ('') never
match the translated "Unassigned" label; update the search to treat empty keys
as the UI label by mapping item.keys (or bindings) such that when a key === ''
you replace it with the translated/unassigned label (lowercased) before
comparing, and ensure both the item.keys.some(...) check and the
bindings.some(...) comparisons compare against normalizedBindingQuery using the
same transformed value; reference symbols: bindings, item.keys, hotkeys,
normalizedBindingQuery, normalizedQuery, itemLabel, sectionLabel.
Comment on lines
+665
to
+668
| const selectSearchResult = (item: HotkeyEditorItem) => { | ||
| stopCapture() | ||
| setSelectedKey(item.keys[0]!) | ||
| } |
Contributor
There was a problem hiding this comment.
Sync the active section when selecting a search result.
This only updates selectedKey. If another activeLayer was already selected, the details panel jumps to the found command but the keyboard overlay and command list stay scoped to the old section, so the selected result is not actually focused.
Suggested fix
- const selectSearchResult = (item: HotkeyEditorItem) => {
+ const selectSearchResult = (section: HotkeyEditorSection, item: HotkeyEditorItem) => {
stopCapture()
+ setActiveLayer(section)
setSelectedKey(item.keys[0]!)
}- onClick={() => selectSearchResult(item)}
+ onClick={() => selectSearchResult(section, item)}Also applies to: 793-794
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/settings/components/hotkey-editor.tsx` around lines 665 - 668,
selectSearchResult currently stops capture and sets only
setSelectedKey(item.keys[0]), which leaves the UI scoped to the previous
section; update selectSearchResult to also sync the active section by calling
the state updater that controls the UI scope (e.g., setActiveLayer or
setActiveSection) with the item's section/layer (item.section or item.layer)
right after stopCapture, and make the same change in the other selection helper
at the other occurrence so the keyboard overlay, command list and details panel
all focus the chosen search result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
Improvements