feat(desktop): complete keybinding customization rollout#443
Open
velga111 wants to merge 27 commits into
Open
Conversation
Make SearchView command shortcuts configurable via persisted keybindings. Add settings UI and tests for shortcut routing and validation. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Assisted-by: OpenAI Codex
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces configurable search keybindings across the desktop app. It adds a new keyboard shortcut system for search window actions (like maximize, toggle pin, reopen last session), integrates those bindings into keyboard routing and settings persistence, updates the global shortcut UI to prevent conflicts, and provides search-specific shortcut editing with validation and persistence. ChangesSearch keybindings end-to-end integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
area:frontend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@apps/desktop/src/config/searchKeybindings.ts`:
- Around line 130-133: normalizeSearchKeybindings currently accepts any
normalized string (via normalizeLocalShortcutString) including modifierless
single-letter keys like "H", allowing invalid persisted payloads; update the
normalization step to enforce the shortcut policy (require at least one modifier
or allow only function keys when modifierless) before writing to
normalized[definition.id]. Specifically, after calling
normalizeLocalShortcutString(candidate) validate the returned shortcut with the
policy (e.g., check for presence of Ctrl/Alt/Meta/Shift modifiers or that the
key is a function key) and only assign to normalized[definition.id] when it
passes; otherwise skip (or clear) the entry so invalid persisted shortcuts
cannot bypass UI validation. Ensure you use the existing helpers
(normalizeLocalShortcutString and the normalizeSearchKeybindings function) and
centralize the policy check so all persisted normalization follows the same
rule.
In `@apps/desktop/src/stores/settings.ts`:
- Around line 187-199: The code currently sets
settings.value.lastClosedSessionId and settings.value.lastActiveSessionId
directly from Number(value) and only checks Number.isNaN; change this to
validate that the persisted value is a positive integer (>0) before applying:
trim the input, ensure it matches /^\d+$/ (no decimals/whitespace/signs),
convert to a Number and accept only if Number.isInteger(parsed) && parsed > 0,
otherwise set the field to null; factor this into a small helper (e.g.,
validateSessionId(value)) and reuse it for settings.value.lastClosedSessionId,
settings.value.lastActiveSessionId and the other similar assignments mentioned
so all persisted session IDs are sanitized consistently.
In `@apps/desktop/src/utils/shortcuts.ts`:
- Around line 154-161: The loop that parses tokens currently overwrites
non-modifier keys (variables: parts, SUPPORTED_CAPTURE_MODIFIERS, modifierSet,
key) allowing malformed multi-key shortcuts to be silently rewritten; update the
parsing in the function that normalizes shortcuts so that when iterating parts,
if you encounter a non-modifier token and key is already set, the function fails
normalization (e.g., return an error/invalid result or throw) instead of
assigning the new token—this rejects multi-key inputs rather than keeping the
last token. Ensure the failure path is used by whatever callers expect
(normalize/parseShortcut) and add a test for two non-modifier tokens to verify
rejection.
In `@apps/desktop/src/views/SearchView/composables/searchInteraction.ts`:
- Around line 776-804: The onSearchKeybindingAction handler currently swallows
any actionId not explicitly matched, causing declared shortcuts to appear
broken; update the switch in onSearchKeybindingAction to either (A) add explicit
cases for the remaining declared search keybinding IDs and implement their
behavior (or delegate to existing helpers like controller.focusSearchInput,
openHistoryDialog, startNewSession, etc.), or (B) change the default branch to
not silently consume the action—return false (or propagate/forward the action to
a higher-level handler) and optionally log a debug/warn with the actionId—so
unhandled actions are not swallowed.
In `@apps/desktop/src/views/SearchView/composables/useSearchRequest.ts`:
- Around line 436-447: The persistence calls to
settingsStore.updateLastClosedSessionId(null) can throw and should not abort
reopen flow; wrap both calls (the one after successfully opening the session
where currentSessionId.value === settingsStore.lastClosedSessionId, and the one
in the catch block where sessionId === settingsStore.lastClosedSessionId) in
their own try/catch that swallows or logs errors so failures to write metadata
do not propagate out of reopenLastClosedSession (refer to
settingsStore.updateLastClosedSessionId, currentSessionId, sessionId,
lastOpenError and reopenLastClosedSession in useSearchRequest.ts).
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Around line 152-155: The function formatSearchShortcutForSettings currently
returns the hardcoded Chinese string '无' for empty shortcuts; change it to
return a localized string using the app's i18n translator instead (e.g., call
the same i18n key used elsewhere in settings like t('settings.noShortcut') or
the existing settings copy key). Update formatSearchShortcutForSettings to
obtain the translator (e.g., via useI18n() or the project's i18n helper) and
return t(...) when normalizedShortcut is falsy so the fallback respects the
active locale.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f43909e-e466-498f-9eaa-82de665e7cce
📒 Files selected for processing (25)
.prettierignoreapps/desktop/src/components/CustomSelect.vueapps/desktop/src/components/appIconMap.tsapps/desktop/src/components/ui/select/SelectContent.vueapps/desktop/src/config/searchKeybindings.tsapps/desktop/src/database/schema.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/services/EventService/types.tsapps/desktop/src/stores/settings.tsapps/desktop/src/utils/font.tsapps/desktop/src/utils/shortcuts.tsapps/desktop/src/views/SearchView/composables/interaction/useSearchKeyboardRouter.tsapps/desktop/src/views/SearchView/composables/searchInteraction.tsapps/desktop/src/views/SearchView/composables/useSearchRequest.tsapps/desktop/src/views/SearchView/index.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/tests/SettingsView/general-language.test.tsapps/desktop/tests/composables/SearchView/searchInteraction.test.tsapps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.tsapps/desktop/tests/composables/SearchView/useSearchRequest.test.tsapps/desktop/tests/config/searchKeybindings.test.tsapps/desktop/tests/stores/settings-keybindings.test.tsapps/desktop/tests/utils/shortcuts.test.tsapps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.tseslint.config.js
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.22.0)
apps/desktop/src/utils/shortcuts.ts
[ERROR] 318-318: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (19)
apps/desktop/src/components/CustomSelect.vue (1)
11-67: LGTM!Also applies to: 68-71, 93-96, 105-106, 116-126, 127-150, 154-156, 162-164
apps/desktop/src/components/ui/select/SelectContent.vue (1)
11-17: LGTM!Also applies to: 21-22, 31-32, 35-36, 40-48, 52-55
apps/desktop/src/components/appIconMap.ts (1)
42-43: LGTM!Also applies to: 86-87
apps/desktop/tests/SettingsView/general-language.test.ts (1)
169-172: LGTM!apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts (1)
7-47: LGTM!Also applies to: 105-126, 188-189, 191-474
apps/desktop/src/utils/font.ts (1)
19-19: LGTM!Also applies to: 161-163, 177-179, 184-186
.prettierignore (1)
8-8: LGTM!eslint.config.js (1)
63-66: LGTM!apps/desktop/src/database/schema.ts (1)
29-29: LGTM!apps/desktop/src/services/EventService/types.ts (1)
14-14: LGTM!Also applies to: 75-79, 93-93
apps/desktop/tests/config/searchKeybindings.test.ts (1)
13-80: LGTM!apps/desktop/tests/stores/settings-keybindings.test.ts (1)
55-166: LGTM!apps/desktop/tests/utils/shortcuts.test.ts (1)
25-153: LGTM!apps/desktop/src/i18n/messages.ts (1)
80-128: LGTM!Also applies to: 855-915
apps/desktop/src/views/SearchView/composables/interaction/useSearchKeyboardRouter.ts (1)
1-8: LGTM!Also applies to: 15-17, 25-25, 35-37, 49-56, 63-63, 86-98, 108-108, 118-119, 132-137, 144-144, 171-174, 182-185, 225-243, 279-306
apps/desktop/tests/composables/SearchView/searchInteraction.test.ts (1)
2-3: LGTM!Also applies to: 6-6, 12-12, 36-38, 297-474
apps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.ts (1)
3-3: LGTM!Also applies to: 14-57, 60-62, 103-483
apps/desktop/src/views/SearchView/index.vue (1)
119-119: LGTM!Also applies to: 237-237, 513-513, 541-541, 817-846
apps/desktop/tests/composables/SearchView/useSearchRequest.test.ts (1)
18-18: LGTM!Also applies to: 36-45, 54-54, 70-72, 149-153, 352-352, 357-403, 405-443
coderabbitai
Bot
added
area:ci
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/tests/composables/SearchView/searchInteraction.test.ts (1)
297-477: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a keydown-handler test for the new settings action.
This suite doesn’t yet assert the new
'search.settings.open'branch increateSearchKeydownHandler(Line 804 inapps/desktop/src/views/SearchView/composables/searchInteraction.ts). Add aCtrl+,/Cmd+,case that verifiesopenSettingsWindowis called and the event is consumed.🤖 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 `@apps/desktop/tests/composables/SearchView/searchInteraction.test.ts` around lines 297 - 477, Add a test to cover the new "search.settings.open" branch in createSearchKeydownHandler by asserting openSettingsWindow is invoked and the event is consumed; create a handler via createSearchKeydownHandler (reuse createControllerStub) with openSettingsWindow mocked (vi.fn().mockResolvedValue(undefined)), dispatch a KeyboardEvent('keydown', { key: ',', ctrlKey: true, cancelable: true }) and await the handler, then expect openSettingsWindow toHaveBeenCalledTimes(1) and event.defaultPrevented toBe(true); also add a separate case for Cmd (metaKey: true) if you want both modifier behaviours covered.
🤖 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.
Outside diff comments:
In `@apps/desktop/tests/composables/SearchView/searchInteraction.test.ts`:
- Around line 297-477: Add a test to cover the new "search.settings.open" branch
in createSearchKeydownHandler by asserting openSettingsWindow is invoked and the
event is consumed; create a handler via createSearchKeydownHandler (reuse
createControllerStub) with openSettingsWindow mocked
(vi.fn().mockResolvedValue(undefined)), dispatch a KeyboardEvent('keydown', {
key: ',', ctrlKey: true, cancelable: true }) and await the handler, then expect
openSettingsWindow toHaveBeenCalledTimes(1) and event.defaultPrevented
toBe(true); also add a separate case for Cmd (metaKey: true) if you want both
modifier behaviours covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51ca1c1b-0722-4638-891f-de5819401f32
📒 Files selected for processing (11)
apps/desktop/src/config/searchKeybindings.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/utils/shortcuts.tsapps/desktop/src/views/SearchView/composables/searchInteraction.tsapps/desktop/src/views/SearchView/index.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/tests/composables/SearchView/searchInteraction.test.tsapps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.tsapps/desktop/tests/config/searchKeybindings.test.tsapps/desktop/tests/stores/settings-keybindings.test.tsapps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: CodeQL (rust)
🔇 Additional comments (13)
apps/desktop/tests/stores/settings-keybindings.test.ts (1)
71-74: LGTM!apps/desktop/src/config/searchKeybindings.ts (1)
16-17: LGTM!Also applies to: 89-96, 143-151
apps/desktop/src/i18n/messages.ts (1)
104-105: LGTM!Also applies to: 119-120, 885-886, 903-904
apps/desktop/src/utils/shortcuts.ts (1)
40-40: LGTM!apps/desktop/tests/config/searchKeybindings.test.ts (1)
46-47: LGTM!Also applies to: 57-58, 65-66
apps/desktop/src/views/SearchView/composables/searchInteraction.ts (1)
139-140: LGTM!Also applies to: 677-678, 804-806
apps/desktop/src/views/SearchView/index.vue (1)
544-545: LGTM!apps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.ts (1)
189-198: LGTM!apps/desktop/src/views/SettingsView/components/General/index.vue (1)
222-222: LGTM!apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts (4)
19-19: LGTM!
223-224: LGTM!
231-236: LGTM!
273-273: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts (1)
9-20: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winBuild the mocked search keybindings from the shared defaults.
This fixture duplicates the production map, so any change to the real keybinding defaults has to be mirrored manually here and these restore/default assertions can silently test the wrong contract. Seed
searchKeybindingsfromcreateDefaultSearchKeybindings()and override only the bindings a given test cares about.🤖 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 `@apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts` around lines 9 - 20, The test fixture createGeneralSettingsMock duplicates production keybinding defaults; replace the hardcoded searchKeybindings with a seeded map from the shared helper createDefaultSearchKeybindings() and only override entries needed for specific tests. Update createGeneralSettingsMock to call createDefaultSearchKeybindings(), spread that result into the searchKeybindings value, and apply per-test overrides (e.g., for 'search.history.open' etc.) so the test uses the canonical defaults and only customizes what each test requires.apps/desktop/src/views/SettingsView/components/General/index.vue (1)
532-542:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserved-shortcut validation now blocks a supported reopen binding.
This unconditional check rejects every reserved chord, but the search keyboard router still supports
search.session.reopenLastClosedwithMod+Up. BecauseresetSearchShortcut()also flows throughsaveSearchShortcut(), users can no longer save or restore that action from Settings.🛠️ Proposed fix
- if (isReservedLocalShortcut(normalizedShortcut)) { + const allowsReservedShortcut = + actionId === 'search.session.reopenLastClosed' && + normalizedShortcut === 'Mod+Up'; + + if (isReservedLocalShortcut(normalizedShortcut) && !allowsReservedShortcut) { reportSearchShortcutError( actionId, 'settings.general.searchShortcuts.errors.reserved' ); updateSearchShortcutDisplay(🤖 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 `@apps/desktop/src/views/SettingsView/components/General/index.vue` around lines 532 - 542, The reserved-shortcut check in the save path (isReservedLocalShortcut) is too strict and blocks the supported reopen binding (search.session.reopenLastClosed with Mod+Up); update the logic in the block around isReservedLocalShortcut to allow that supported binding instead of unconditionally rejecting it — either skip the reserved validation when actionId === 'search.session.reopenLastClosed' (or when resetSearchShortcut/saveSearchShortcut flow needs to restore it) or replace the check with a router-aware allowance (e.g., call a helper that returns true if the shortcut is allowed for this action by the search keyboard router) before calling reportSearchShortcutError/updateSearchShortcutDisplay/formatSearchShortcutForSettings so users can save and restore the reopenLastClosed binding.
🤖 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 `@apps/desktop/src/views/SearchView/composables/useSearchRequest.ts`:
- Around line 428-443: The catch block in reopenLastClosedSession clears
settingsStore.lastClosedSessionId on any error, losing the only retry candidate;
change it so clearLastClosedSessionIdSafely is NOT called for generic/transient
errors. Specifically, remove the await clearLastClosedSessionIdSafely() from the
catch in reopenLastClosedSession(), and only clear the stored id when you can
deterministically detect the session is gone (e.g., catch and check for a
specific SessionNotFoundError or an error code/message indicating the session is
missing) — otherwise rethrow the error so the user can retry; keep the existing
success-path clearing logic that checks currentSessionId.value === sessionId
after openSession returns.
In `@apps/desktop/src/views/SearchView/index.vue`:
- Around line 818-822: In handleReopenLastClosedSession: before the early return
that preserves the draft (the branch that checks queryText.value.trim() ||
attachments.value.length > 0), ensure you call hideAllPopups() so any open popup
surfaces are dismissed, then await controller.focusSearchInput() and return;
this mirrors the existing search.input.focus path behavior and keeps the draft
while hiding popups.
---
Outside diff comments:
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Around line 532-542: The reserved-shortcut check in the save path
(isReservedLocalShortcut) is too strict and blocks the supported reopen binding
(search.session.reopenLastClosed with Mod+Up); update the logic in the block
around isReservedLocalShortcut to allow that supported binding instead of
unconditionally rejecting it — either skip the reserved validation when actionId
=== 'search.session.reopenLastClosed' (or when
resetSearchShortcut/saveSearchShortcut flow needs to restore it) or replace the
check with a router-aware allowance (e.g., call a helper that returns true if
the shortcut is allowed for this action by the search keyboard router) before
calling
reportSearchShortcutError/updateSearchShortcutDisplay/formatSearchShortcutForSettings
so users can save and restore the reopenLastClosed binding.
In `@apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts`:
- Around line 9-20: The test fixture createGeneralSettingsMock duplicates
production keybinding defaults; replace the hardcoded searchKeybindings with a
seeded map from the shared helper createDefaultSearchKeybindings() and only
override entries needed for specific tests. Update createGeneralSettingsMock to
call createDefaultSearchKeybindings(), spread that result into the
searchKeybindings value, and apply per-test overrides (e.g., for
'search.history.open' etc.) so the test uses the canonical defaults and only
customizes what each test requires.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff5f7c16-e57d-44e4-814a-8451f0559450
📒 Files selected for processing (12)
apps/desktop/src/config/searchKeybindings.tsapps/desktop/src/utils/shortcuts.tsapps/desktop/src/views/SearchView/composables/interaction/useSearchKeyboardRouter.tsapps/desktop/src/views/SearchView/composables/useSearchRequest.tsapps/desktop/src/views/SearchView/index.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/tests/composables/SearchView/searchInteraction.test.tsapps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.tsapps/desktop/tests/composables/SearchView/useSearchRequest.test.tsapps/desktop/tests/config/searchKeybindings.test.tsapps/desktop/tests/utils/shortcuts.test.tsapps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: Rust Checks
- GitHub Check: Frontend Tests
🔇 Additional comments (6)
apps/desktop/src/config/searchKeybindings.ts (1)
5-5: LGTM!Also applies to: 62-62, 152-154
apps/desktop/src/utils/shortcuts.ts (1)
17-31: LGTM!Also applies to: 173-177
apps/desktop/tests/config/searchKeybindings.test.ts (1)
42-42: LGTM!Also applies to: 56-57, 64-64, 67-67, 80-80
apps/desktop/tests/utils/shortcuts.test.ts (1)
36-36: LGTM!Also applies to: 43-46, 73-73, 134-139
apps/desktop/src/views/SearchView/composables/interaction/useSearchKeyboardRouter.ts (1)
86-89: LGTM!Also applies to: 284-286, 296-298
apps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.ts (1)
165-175: LGTM!
Contributor
Author
|
Review feedback has been addressed and pushed in \2c44d9dd\. Key updates:
Ready for re-review. @coderabbitai review |
|
(ﾐ･ω･ﾐ) ✅ Action performedReview finished.
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/desktop/src/config/searchKeybindings.ts`:
- Around line 141-147: hasShortcutConflict is comparing the incoming shortcut
against normalized[definition.id] which still contains defaults for actions not
yet processed, so valid multi-action remaps (like swaps) are flagged as
conflicts; fix by using a deterministic two-pass approach: iterate
SEARCH_KEYBINDING_DEFINITIONS in a stable order and build a map/set of final
shortcuts applying overrides from normalized when an action has already been
assigned (first-wins), then in hasShortcutConflict compare the candidate
shortcut only against that final-assigned set (and skip comparing against the
actionId itself); reference SEARCH_KEYBINDING_DEFINITIONS and normalized and
update hasShortcutConflict to consult the computed final assignment rather than
raw normalized defaults.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 75d5236c-af8c-477e-8bf8-e89ac044219d
📒 Files selected for processing (7)
apps/desktop/src/config/searchKeybindings.tsapps/desktop/src/views/SearchView/composables/interaction/useSearchKeyboardRouter.tsapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/tests/composables/SearchView/searchInteraction.test.tsapps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.tsapps/desktop/tests/config/searchKeybindings.test.tsapps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
🔇 Additional comments (7)
apps/desktop/src/views/SettingsView/components/General/index.vue (2)
508-614: LGTM!
639-666: LGTM!apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts (1)
380-402: LGTM!apps/desktop/tests/config/searchKeybindings.test.ts (1)
86-100: LGTM!apps/desktop/src/views/SearchView/composables/interaction/useSearchKeyboardRouter.ts (1)
175-182: LGTM!apps/desktop/tests/composables/SearchView/searchInteraction.test.ts (1)
416-424: LGTM!Also applies to: 468-472
apps/desktop/tests/composables/SearchView/useSearchKeyboardRouter.test.ts (1)
275-285: LGTM!
Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/desktop/src/config/searchKeybindings.ts`:
- Around line 182-185: The code normalizes definition.defaultShortcut into
fallback for availability checks but then stores the un-normalized
definition.defaultShortcut into result, creating a mismatch between
usedShortcuts and stored values; update the assignment in the block that uses
normalizeLocalShortcutString so that result[definition.id] = fallback (or
alternatively normalize all defaults at module initialization) and ensure
usedShortcuts, result, and any later conflict checks consistently use
normalizeLocalShortcutString.
- Line 187: The current assignment at result[definition.id] =
definition.allowDisable ? null : definition.defaultShortcut can produce
duplicate bindings because it doesn't consult or update usedShortcuts; change
the logic in the searchKeybindings generation code so that when a desired
shortcut is taken you treat an unavailable fallback as irreconcilable and set
result[definition.id] = null (and do not add the default to usedShortcuts), or
if you decide to honor allowDisable: false instead, add
definition.defaultShortcut to usedShortcuts when assigning it so subsequent
iterations see it as used; update the branch that references
definition.allowDisable, result, definition.defaultShortcut and usedShortcuts
accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a92a2e4-d49e-408c-b631-f8f1c11993a3
📒 Files selected for processing (1)
apps/desktop/src/config/searchKeybindings.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Frontend Tests
- GitHub Check: Frontend Quality
- GitHub Check: Rust Checks
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (rust)
🔇 Additional comments (3)
apps/desktop/src/config/searchKeybindings.ts (3)
1-31: LGTM!
33-98: LGTM!
100-125: LGTM!
Co-Authored-By: Claude <[email protected]>
…stomization # Conflicts: # apps/desktop/src/components/CustomSelect.vue # apps/desktop/src/database/schema.ts # apps/desktop/src/services/EventService/types.ts # apps/desktop/src/stores/settings.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.prettierignore:
- Line 68: The ignore entry "apps/desktop/.vite-cache/" is redundant because the
existing pattern ".vite-cache/" already covers it; remove the specific
"apps/desktop/.vite-cache/" line from .prettierignore so only the generic
".vite-cache/" pattern remains, leaving no behavioral change but reducing
duplication.
In `@apps/desktop/src/components/CustomSelect.vue`:
- Around line 12-13: The import of useAttrs is duplicated; replace the two
separate Vue imports by consolidating computed, ref and useAttrs into a single
import from 'vue' (i.e., merge the separate import lines so the single import
includes computed, ref, and useAttrs) to remove the duplicate import statement.
In `@apps/desktop/src/stores/settings.ts`:
- Around line 80-92: The Promise.allSettled call over
GENERAL_SETTING_DEFINITIONS currently ignores rejected results and silently
swallows failures from setSetting/serializeSetting/persistDefaultIfMissing;
change this to inspect the returned results after awaiting Promise.allSettled,
collect any entries with status === "rejected" (use the associated
definition.key or index from GENERAL_SETTING_DEFINITIONS/settingRows to identify
which setting failed), and then surface the failure by logging the error and
rethrowing or aggregating and throwing a single Error so callers see the write
failure (alternatively use Promise.all if you want fail-fast behavior); ensure
you reference setSetting, serializeSetting and persistDefaultIfMissing when
mapping failures to keys so the error message clearly identifies which setting
write failed.
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Line 333: The conditional that checks for native shortcut registration errors
uses mojibake Chinese strings ('宸叉敞鍐?' / '鏃犳晥'); update the check in the
SettingsView General component so errorStr is matched against the proper Chinese
fallbacks used elsewhere (e.g., '已注册' and '无效') instead of the garbled
strings—locate the if that references errorStr (the one currently testing for
'already registered' || '宸叉敞鍐?') and replace the mojibake literals with the
correct '已注册' and '无效' matches.
In
`@apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue`:
- Around line 570-572: The template's `@click` handler is split across lines
causing a Prettier violation; collapse the handler into a single line so the
element uses `@click`="handleSearchShortcutIconAction(row.id, row.shortcutAction)"
(reference the handleSearchShortcutIconAction call and the row.id /
row.shortcutAction args) and ensure there are no extra line breaks or trailing
spaces around the attribute.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bfa8d44c-f3c5-4f3f-9b41-572197c1f375
📒 Files selected for processing (10)
.prettierignoreapps/desktop/src/components/CustomSelect.vueapps/desktop/src/components/appIconMap.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/stores/setting/index.tsapps/desktop/src/stores/settings.tsapps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/tests/stores/setting.test.tseslint.config.js
💤 Files with no reviewable changes (2)
- eslint.config.js
- apps/desktop/tests/stores/setting.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: Rust Checks
- GitHub Check: Frontend Tests
- GitHub Check: CodeQL (rust)
🧰 Additional context used
🪛 ESLint
apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue
[error] 570-572: Replace ⏎····································handleSearchShortcutIconAction(row.id,·row.shortcutAction)⏎································ with handleSearchShortcutIconAction(row.id,·row.shortcutAction)
(prettier/prettier)
🪛 GitHub Check: Frontend Quality
apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue
[failure] 570-570:
Replace ⏎····································handleSearchShortcutIconAction(row.id,·row.shortcutAction)⏎································ with handleSearchShortcutIconAction(row.id,·row.shortcutAction)
🔇 Additional comments (7)
.prettierignore (2)
65-68: AI summary inconsistency.The AI summary states "
.vite-cache/[was added] to the ignore patterns list," but.vite-cache/already exists at line 8. The actual change is adding the comment section andapps/desktop/.vite-cache/(along withapps/desktop/data/).
67-67: apps/desktop/data/ ignore looks like standard runtime-data hygiene
.prettierignorealready labels it as “Local app runtime data and Vite cache”, and the repo also ignoresapps/desktop/data/in.gitignoreandeslint.config.js(no repo code references found). If PR scope is strictly keybinding-only, confirm whether this ignore-only maintenance should be split out.apps/desktop/src/components/CustomSelect.vue (1)
17-18: LGTM!Also applies to: 33-39, 44-46, 63-74, 97-100, 103-161
apps/desktop/src/components/appIconMap.ts (1)
22-22: LGTM!Also applies to: 43-43, 66-67, 71-71, 89-89
apps/desktop/src/views/SettingsView/components/General/index.vue (1)
23-27: LGTM!Also applies to: 96-106, 120-159, 170-172, 186-188, 221-229, 564-564
apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue (2)
1-72: LGTM!Also applies to: 119-238, 240-347, 372-503, 505-569, 573-585
74-117: LGTM!
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 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 @.prettierignore:
- Line 68: The ignore entry "apps/desktop/.vite-cache/" is redundant because the
existing pattern ".vite-cache/" already covers it; remove the specific
"apps/desktop/.vite-cache/" line from .prettierignore so only the generic
".vite-cache/" pattern remains, leaving no behavioral change but reducing
duplication.
In `@apps/desktop/src/components/CustomSelect.vue`:
- Around line 12-13: The import of useAttrs is duplicated; replace the two
separate Vue imports by consolidating computed, ref and useAttrs into a single
import from 'vue' (i.e., merge the separate import lines so the single import
includes computed, ref, and useAttrs) to remove the duplicate import statement.
In `@apps/desktop/src/stores/settings.ts`:
- Around line 80-92: The Promise.allSettled call over
GENERAL_SETTING_DEFINITIONS currently ignores rejected results and silently
swallows failures from setSetting/serializeSetting/persistDefaultIfMissing;
change this to inspect the returned results after awaiting Promise.allSettled,
collect any entries with status === "rejected" (use the associated
definition.key or index from GENERAL_SETTING_DEFINITIONS/settingRows to identify
which setting failed), and then surface the failure by logging the error and
rethrowing or aggregating and throwing a single Error so callers see the write
failure (alternatively use Promise.all if you want fail-fast behavior); ensure
you reference setSetting, serializeSetting and persistDefaultIfMissing when
mapping failures to keys so the error message clearly identifies which setting
write failed.
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Line 333: The conditional that checks for native shortcut registration errors
uses mojibake Chinese strings ('宸叉敞鍐?' / '鏃犳晥'); update the check in the
SettingsView General component so errorStr is matched against the proper Chinese
fallbacks used elsewhere (e.g., '已注册' and '无效') instead of the garbled
strings—locate the if that references errorStr (the one currently testing for
'already registered' || '宸叉敞鍐?') and replace the mojibake literals with the
correct '已注册' and '无效' matches.
In
`@apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue`:
- Around line 570-572: The template's `@click` handler is split across lines
causing a Prettier violation; collapse the handler into a single line so the
element uses `@click`="handleSearchShortcutIconAction(row.id, row.shortcutAction)"
(reference the handleSearchShortcutIconAction call and the row.id /
row.shortcutAction args) and ensure there are no extra line breaks or trailing
spaces around the attribute.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bfa8d44c-f3c5-4f3f-9b41-572197c1f375
📒 Files selected for processing (10)
.prettierignoreapps/desktop/src/components/CustomSelect.vueapps/desktop/src/components/appIconMap.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/stores/setting/index.tsapps/desktop/src/stores/settings.tsapps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/tests/stores/setting.test.tseslint.config.js
💤 Files with no reviewable changes (2)
- eslint.config.js
- apps/desktop/tests/stores/setting.test.ts
📜 Review details
🔇 Additional comments (7)
.prettierignore (2)
65-68: AI summary inconsistency.The AI summary states "
.vite-cache/[was added] to the ignore patterns list," but.vite-cache/already exists at line 8. The actual change is adding the comment section andapps/desktop/.vite-cache/(along withapps/desktop/data/).
67-67: apps/desktop/data/ ignore looks like standard runtime-data hygiene
.prettierignorealready labels it as “Local app runtime data and Vite cache”, and the repo also ignoresapps/desktop/data/in.gitignoreandeslint.config.js(no repo code references found). If PR scope is strictly keybinding-only, confirm whether this ignore-only maintenance should be split out.apps/desktop/src/components/CustomSelect.vue (1)
17-18: LGTM!Also applies to: 33-39, 44-46, 63-74, 97-100, 103-161
apps/desktop/src/components/appIconMap.ts (1)
22-22: LGTM!Also applies to: 43-43, 66-67, 71-71, 89-89
apps/desktop/src/views/SettingsView/components/General/index.vue (1)
23-27: LGTM!Also applies to: 96-106, 120-159, 170-172, 186-188, 221-229, 564-564
apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue (2)
1-72: LGTM!Also applies to: 119-238, 240-347, 372-503, 505-569, 573-585
74-117: LGTM!
🛑 Comments failed to post (5)
.prettierignore (1)
68-68: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Redundant ignore pattern—already covered by line 8.
The pattern
.vite-cache/at line 8 matches.vite-cache/directories at any depth (gitignore glob semantics), soapps/desktop/.vite-cache/is unnecessary.♻️ Simplify by removing the redundant entry
# Local app runtime data and Vite cache apps/desktop/data/ -apps/desktop/.vite-cache/📝 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.# Local app runtime data and Vite cache apps/desktop/data/🤖 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 @.prettierignore at line 68, The ignore entry "apps/desktop/.vite-cache/" is redundant because the existing pattern ".vite-cache/" already covers it; remove the specific "apps/desktop/.vite-cache/" line from .prettierignore so only the generic ".vite-cache/" pattern remains, leaving no behavioral change but reducing duplication.apps/desktop/src/components/CustomSelect.vue (1)
12-13: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consolidate duplicate Vue imports.
useAttrsis imported separately on line 13, but it should be merged with the existing Vue import on line 12.♻️ Suggested fix
- import { computed, ref } from 'vue'; - import { useAttrs } from 'vue'; + import { computed, ref, useAttrs } from 'vue';📝 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.import { computed, ref, useAttrs } from 'vue';🤖 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 `@apps/desktop/src/components/CustomSelect.vue` around lines 12 - 13, The import of useAttrs is duplicated; replace the two separate Vue imports by consolidating computed, ref and useAttrs into a single import from 'vue' (i.e., merge the separate import lines so the single import includes computed, ref, and useAttrs) to remove the duplicate import statement.apps/desktop/src/stores/settings.ts (1)
80-92:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle failed rewrite/default persistence writes instead of swallowing them.
Line [80] uses
Promise.allSettled(...)without checking rejected results, so failedsetSetting(...)calls are silently ignored. That leaves normalized/default values unapplied in storage and can repeatedly reload stale/corrupt persisted data on next startup.Suggested fix
- await Promise.allSettled( + const persistenceResults = await Promise.allSettled( GENERAL_SETTING_DEFINITIONS.map((definition, index) => { const persistedValue = settingRows[index] ?? null; if (definition.shouldRewritePersisted?.(persistedValue)) { return setSetting({ key: definition.key, value: serializeSetting(definition.key), }); } return persistDefaultIfMissing(definition.key, persistedValue); }) ); + const failedWrites = persistenceResults.filter( + (result): result is PromiseRejectedResult => result.status === 'rejected' + ); + if (failedWrites.length > 0) { + throw new Error( + `Failed to persist ${failedWrites.length} general setting rewrite/default value(s)` + ); + }🤖 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 `@apps/desktop/src/stores/settings.ts` around lines 80 - 92, The Promise.allSettled call over GENERAL_SETTING_DEFINITIONS currently ignores rejected results and silently swallows failures from setSetting/serializeSetting/persistDefaultIfMissing; change this to inspect the returned results after awaiting Promise.allSettled, collect any entries with status === "rejected" (use the associated definition.key or index from GENERAL_SETTING_DEFINITIONS/settingRows to identify which setting failed), and then surface the failure by logging the error and rethrowing or aggregating and throwing a single Error so callers see the write failure (alternatively use Promise.all if you want fail-fast behavior); ensure you reference setSetting, serializeSetting and persistDefaultIfMissing when mapping failures to keys so the error message clearly identifies which setting write failed.apps/desktop/src/views/SettingsView/components/General/index.vue (1)
333-333:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify what Chinese strings the native API actually returns for these error conditions # and check for other potential mojibake in the codebase echo "=== Searching for similar mojibake patterns in codebase ===" rg -n '宸叠敞|鏃犳晥|宸叉敞鍐' --type ts --type vue echo "" echo "=== Looking for correct Chinese error strings in native/backend code ===" rg -n '已注册|无效' --type rust --type ts echo "" echo "=== Check if there are locale files with these error messages ===" fd -e json -e ts messages | xargs rg -l 'registered|invalid' | head -5Repository: TouchAI-org/TouchAI
Length of output: 4334
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="apps/desktop/src/views/SettingsView/components/General/index.vue" echo "=== Dump lines around 333 in ${FILE} ===" nl -ba "$FILE" | sed -n '320,360p' echo "" echo "=== Dump lines around 343 in ${FILE} ===" nl -ba "$FILE" | sed -n '338,350p' echo "" echo "=== Search for mojibake substrings using globs (no --type vue) ===" rg -n '宸叠敞|鏃犳晥|宸叉敞鍐' -g '*.ts' -g '*.vue' || true echo "" echo "=== Search for the exact suspected substrings ===" rg -n "宸叠敞鍐\\?|宸叉敞鍐\\?|鏃犳晥" -g '*.ts' -g '*.vue' || trueRepository: TouchAI-org/TouchAI
Length of output: 222
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="apps/desktop/src/views/SettingsView/components/General/index.vue" echo "=== Dump lines around 333 in ${FILE} (with line numbers) ===" awk 'NR>=320 && NR<=360 {printf "%d\t%s\n", NR, $0}' "$FILE" echo "" echo "=== Dump lines around 343 in ${FILE} (with line numbers) ===" awk 'NR>=338 && NR<=350 {printf "%d\t%s\n", NR, $0}' "$FILE" echo "" echo "=== Search for mojibake substrings using globs ===" rg -n '宸叠敞|鏃犳晥|宸叉敞鍐' -g '*.ts' -g '*.vue' || true echo "" echo "=== Search for the exact suspected substrings ===" rg -n "宸叠敞鍐\\?|宸叉敞鍐\\?|鏃犳晥" -g '*.ts' -g '*.vue' || trueRepository: TouchAI-org/TouchAI
Length of output: 3657
🏁 Script executed:
#!/bin/bash set -euo pipefail # Where error strings might originate (tauri/native side) BASE="apps/desktop/src-tauri" echo "=== Search native/tauri for mojibake substrings ===" rg -n "宸叉敞鍐\\?|鏃犳晥" "$BASE" || true echo "" echo "=== Search native/tauri for likely intended Chinese strings ===" rg -n "已注册|无效" "$BASE" || true echo "" echo "=== Search desktop SettingsView for where errorStr is built ===" FILE="apps/desktop/src/views/SettingsView/components/General/index.vue" rg -n "errorStr" "$FILE" echo "" echo "=== Show surrounding context for errorStr assignment (first occurrences) ===" # Print a handful of nearby lines for the first few errorStr mentions rg -n "errorStr" "$FILE" | head -n 20 | awk -F: '{print $1}' | sort -n | uniq | head -n 5 | while read -r line; do start=$((line-15)); end=$((line+15)); echo "--- $FILE:${line} ---" sed -n "${start},${end}p" "$FILE" doneRepository: TouchAI-org/TouchAI
Length of output: 8189
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Search for registerGlobalShortcut implementation ===" rg -n "registerGlobalShortcut" apps/desktop/src apps/desktop/src-tauri || true echo "" echo "=== Search for shortcut.registerGlobalShortcut usage ===" rg -n "shortcut\.registerGlobalShortcut" apps/desktop/src || true echo "" echo "=== Search for 'already registered' / 'invalid' in tauri shortcut code ===" rg -n "already registered|invalid" apps/desktop/src-tauri || true echo "" echo "=== Search for tauri command names related to shortcut ===" rg -n "shortcut" apps/desktop/src-tauri/src | head -n 80 || trueRepository: TouchAI-org/TouchAI
Length of output: 13960
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== apps/desktop/src/services/NativeService/shortcut.ts ===" FILE="apps/desktop/src/services/NativeService/shortcut.ts" wc -l "$FILE" sed -n '1,200p' "$FILE" echo "" echo "=== apps/desktop/src-tauri/src/core/system/shortcut.rs (core bits) ===" FILE2="apps/desktop/src-tauri/src/core/system/shortcut.rs" wc -l "$FILE2" # show around register_global_shortcut and parse_shortcut rg -n "register_global_shortcut|map_err\\(|Failed to register shortcut|parse_shortcut" "$FILE2" # print a focused range around likely locations sed -n '1,140p' "$FILE2" sed -n '140,260p' "$FILE2"Repository: TouchAI-org/TouchAI
Length of output: 7265
🌐 Web query:
tauri-plugin-global-shortcut error message "already registered" "invalid"💡 Result:
In Tauri’s plugin-global-shortcut, the error strings you mention typically come from two different failure modes: 1) “already registered” (often shown as “Hotkey already registered.”) This means the OS already has that global shortcut reserved (by another application or by your own app already registering it). Tauri cannot “hijack” an already-taken global hotkey, so your handler won’t trigger when the shortcut is already taken by another app. [1][2] What to do: - Pick a more unique shortcut to avoid collisions. [1] - Before registering, check whether it’s registered (by your application) using isRegistered; if it’s false, you can try registering again (but if another app has it, register will still not trigger your handler). [1] - If you’re re-registering within the same app lifecycle, call unregister/unregisterAll appropriately, but note that unregister_all behavior around “none registered” has had issues. [2] 2) “invalid” (e.g., “Uncaught (in promise) invalid args … missing required key shortcut”) This is usually an argument/format problem rather than an OS-collision problem: the plugin expects a shortcut string in the form “modifiers+key” (e.g., CmdOrControl+Q). If the call payload is wrong or missing the shortcut field, you can get “invalid args … missing required key shortcut”. [3][1] What to do: - Ensure you pass the shortcut strings as the plugin’s register expects (string or string[]), and that each item is a valid accelerator like “CommandOrControl+Shift+C”. [1][4] - If you receive “invalid accelerator” / parsing-related errors, ensure the key names match what Tauri accepts; invalid key names can cause parse failures. (Tauri has also had patches to avoid panics and return proper errors for invalid shortcuts.) [5] Quick checklist (most common root causes) - Are you trying to register the exact same shortcut while it’s still registered? -> collision, “Hotkey already registered.” [2][1] - Is another app likely using it already? -> collision, handler won’t fire. [1] - Are you passing malformed JS (e.g., wrong shape or missing shortcut string)? -> “invalid args … missing required key shortcut”. [3] - Are you using an unsupported key name/format? -> parsing/invalid accelerator. [6][5] If you paste the exact full error text and the JS call site (the register / unregister code plus the exact shortcut string), I can map it to which of the two categories above it is and suggest the precise fix.
Citations:
- 1: https://v2.tauri.app/reference/javascript/global-shortcut/
- 2: tauri-apps/tauri#9806
- 3: tauri-apps/plugins-workspace#1558
- 4: https://github.com/tauri-apps/tauri-plugin-global-shortcut
- 5: https://www.github.com/tauri-apps/tauri/commit/b41b57ebb27befd366db5befaafb6043c18fdfef
- 6: tauri-apps/tauri#7121
Fix mojibake shortcut error matching in SettingsView
apps/desktop/src/views/SettingsView/components/General/index.vuematches Chinese fallbacks'宸叉敞鍐?'and'鏃犳晥'for the native shortcut registration error, but these strings don’t exist in the native codebase and should be aligned with the Chinese fallbacks already used elsewhere ('已注册'/'无效').🐛 Suggested fix
- if (errorStr.includes('already registered') || errorStr.includes('宸叉敞鍐?')) { + if (errorStr.includes('already registered') || errorStr.includes('已注册')) {- if (errorStr.includes('invalid') || errorStr.includes('鏃犳晥')) { + if (errorStr.includes('invalid') || errorStr.includes('无效')) {🤖 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 `@apps/desktop/src/views/SettingsView/components/General/index.vue` at line 333, The conditional that checks for native shortcut registration errors uses mojibake Chinese strings ('宸叉敞鍐?' / '鏃犳晥'); update the check in the SettingsView General component so errorStr is matched against the proper Chinese fallbacks used elsewhere (e.g., '已注册' and '无效') instead of the garbled strings—locate the if that references errorStr (the one currently testing for 'already registered' || '宸叉敞鍐?') and replace the mojibake literals with the correct '已注册' and '无效' matches.apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue (1)
570-572:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Prettier formatting violation.
The
@clickhandler should be on a single line per the project's Prettier configuration.🧹 Suggested fix
`@click`=" - handleSearchShortcutIconAction(row.id, row.shortcutAction) - " + `@click`="handleSearchShortcutIconAction(row.id, row.shortcutAction)"🧰 Tools
🪛 ESLint
[error] 570-572: Replace
⏎····································handleSearchShortcutIconAction(row.id,·row.shortcutAction)⏎································withhandleSearchShortcutIconAction(row.id,·row.shortcutAction)(prettier/prettier)
🪛 GitHub Check: Frontend Quality
[failure] 570-570:
Replace⏎····································handleSearchShortcutIconAction(row.id,·row.shortcutAction)⏎································withhandleSearchShortcutIconAction(row.id,·row.shortcutAction)🤖 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 `@apps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vue` around lines 570 - 572, The template's `@click` handler is split across lines causing a Prettier violation; collapse the handler into a single line so the element uses `@click`="handleSearchShortcutIconAction(row.id, row.shortcutAction)" (reference the handleSearchShortcutIconAction call and the row.id / row.shortcutAction args) and ensure there are no extra line breaks or trailing spaces around the attribute.Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/views/SettingsView/components/General/index.vue (1)
23-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize search bindings to the current platform before checking global conflicts.
This comparison uses raw
searchKeybindingsvalues, but the search-shortcut editor already treats those bindings as local syntax and converts them withtoCurrentPlatformShortcut()before comparing them tosettings.value.globalShortcut. Without the same conversion here, a binding likeCommandOrCtrl+Spacecan slip past this check while still colliding withCtrl+Spaceon the active platform, so the global shortcut UI can persist an overlap that the search shortcut UI rejects.Suggested fix
-import { normalizeLocalShortcutString } from '`@/utils/shortcuts`'; +import { + normalizeLocalShortcutString, + toCurrentPlatformShortcut, +} from '`@/utils/shortcuts`'; function findGlobalShortcutSearchConflict(shortcut: string): boolean { const normalizedShortcut = normalizeLocalShortcutString(shortcut); if (!normalizedShortcut) { return false; } return Object.values(settings.value.searchKeybindings).some((searchShortcut) => { - const normalizedSearchShortcut = normalizeLocalShortcutString(searchShortcut); + const normalizedSearchShortcut = searchShortcut + ? normalizeLocalShortcutString(toCurrentPlatformShortcut(searchShortcut)) + : null; return normalizedSearchShortcut === normalizedShortcut; }); }Also applies to: 96-106
🤖 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 `@apps/desktop/src/views/SettingsView/components/General/index.vue` at line 23, The code compares raw searchKeybindings to settings.value.globalShortcut without normalizing to the current platform, allowing values like "CommandOrCtrl+Space" to bypass conflict checks; update the comparison to convert each search binding using normalizeLocalShortcutString (the same normalization used by the search-shortcut editor) before checking against settings.value.globalShortcut so conflicts are detected consistently (apply the same normalization in both places where searchKeybindings are checked).
🤖 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.
Outside diff comments:
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Line 23: The code compares raw searchKeybindings to
settings.value.globalShortcut without normalizing to the current platform,
allowing values like "CommandOrCtrl+Space" to bypass conflict checks; update the
comparison to convert each search binding using normalizeLocalShortcutString
(the same normalization used by the search-shortcut editor) before checking
against settings.value.globalShortcut so conflicts are detected consistently
(apply the same normalization in both places where searchKeybindings are
checked).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b43cbef-8739-4da8-9544-5fd5d88a72c1
📒 Files selected for processing (4)
apps/desktop/src/components/CustomSelect.vueapps/desktop/src/stores/settings.tsapps/desktop/src/views/SettingsView/components/General/SearchShortcutSettings.vueapps/desktop/src/views/SettingsView/components/General/index.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Frontend Tests
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
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
Related issue or RFC
Related: #145 - make model-switching and other shortcuts configurable.
Also references #40 for SearchView shortcut behavior.
The review feedback about persisted
last_closed_session_id/last_active_session_idstate has been addressed by removing that persisted recovery contract. The remaining reopen-last-closed-session behavior is scoped to the active SearchView runtime state.AI assistance disclosure
pnpm test:pr.AgentService, tool execution, provider/runtime data, release workflows, and desktop startup/settings paths. The session restore persistence work called out in review has been removed from this PR.Testing evidence
TDD: not fully confirmed for the whole branch; tests were added and the font-loader regression was fixed after a failing local test run.
Additional local validation after review fixes:
Risk notes
AgentService, runtime, MCP, or schema impact: touchesAgentServiceexecution/provider/session paths and built-in tool runtime/web-fetch behavior.Screenshots or recordings
No updated screenshots for these review fixes.
Checklist
[WIP]or similar title prefixes.AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.pnpm test:prfor this code PR, or this is a docs-only change.pnpm test:coverage:rustor relied on CI coverage evidence.pnpm test:e2elocally or documented why CI is the first valid proof.