Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Free Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces a new Quick Switcher feature for quickly navigating to bookmarked Roam pages via keyboard shortcuts. The implementation includes a modal dialog component for searching and opening bookmarks, a settings panel for creating and managing bookmarks with custom keyboard shortcuts, core initialization and controller logic, utility functions for shortcut normalization and URL parsing, type definitions, test coverage, and integration into the extension's main entry point. Additionally, a To Do section is added to the README documenting planned UI improvements. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const normalizedToken = token.toLowerCase().trim(); | ||
| if (!normalizedToken) { | ||
| return ""; | ||
| } | ||
| if (normalizedToken === " ") { | ||
| return "space"; | ||
| } |
There was a problem hiding this comment.
🟡 Space key in shortcuts is silently dropped due to .trim() before space check
The normalizeKeyToken function calls .toLowerCase().trim() on the token before checking if it equals " " (space). Since .trim() removes whitespace, a single space " " becomes "", which triggers the early return at line 29 (if (!normalizedToken) return ""). The normalizedToken === " " check at line 32 is therefore unreachable.
Root Cause and Impact
When a KeyboardEvent fires for the Space key, event.key is " ". This flows through normalizeKeyboardEventKey → normalizeKeyToken({ token: " " }), where .trim() converts it to "", and the function returns empty string. As a result, keyboardEventToShortcut returns null for any shortcut involving Space.
Since the shortcut capture UI (onShortcutKeyDown in QuickSwitcherSettings.tsx:89) also uses keyboardEventToShortcut, users cannot even record a Space-based shortcut. If a stored shortcut containing "space" were loaded from settings (e.g., "ctrl+space"), it would parse correctly via normalizeShortcut (since the string token "space" survives .trim()), but the keyboard event would never match it — making the shortcut permanently non-functional.
| const normalizedToken = token.toLowerCase().trim(); | |
| if (!normalizedToken) { | |
| return ""; | |
| } | |
| if (normalizedToken === " ") { | |
| return "space"; | |
| } | |
| const lower = token.toLowerCase(); | |
| if (lower === " " || lower.trim() === "space" || lower.trim() === "spacebar") { | |
| return "space"; | |
| } | |
| const normalizedToken = lower.trim(); | |
| if (!normalizedToken) { | |
| return ""; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const onDocumentKeyDown = (event: KeyboardEvent): void => { | ||
| if (isDialogOpen && event.key === "Escape") { | ||
| event.preventDefault(); | ||
| closeDialog(); | ||
| return; | ||
| } | ||
|
|
||
| if (isEditableTarget({ target: event.target })) { | ||
| return; | ||
| } | ||
|
|
||
| const shortcut = keyboardEventToShortcut({ event }); | ||
| if (!shortcut) { | ||
| return; | ||
| } | ||
|
|
||
| const bookmark = bookmarks.find((entry) => entry.shortcut === shortcut); | ||
| if (!bookmark) { | ||
| return; | ||
| } | ||
|
|
||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| void openBookmark({ bookmark }); | ||
| }; |
There was a problem hiding this comment.
🔴 Bookmark keyboard shortcuts fire while the Quick Switcher dialog is open
The onDocumentKeyDown handler only guards against Escape when the dialog is open (isDialogOpen && event.key === "Escape"). For all other keys, it falls through to the isEditableTarget check. If the dialog is open and focus is on a non-editable element (e.g., a MenuItem, the dialog overlay, or the dialog body), the bookmark shortcut matching proceeds and navigates away from the current page while the dialog remains open.
Detailed Explanation
When the dialog opens, focus is set on the search input via a setTimeout in QuickSwitcherDialog.tsx:52. However, the user can move focus to non-editable elements by clicking on menu items, the dialog body, or using Tab. In that state:
isDialogOpenistrue, butevent.keyis not"Escape"→ line 181 doesn't matchisEditableTarget({ target: event.target })returnsfalse→ line 187 doesn't bail outkeyboardEventToShortcutproduces a shortcut string → line 196 finds a matching bookmarkopenBookmarknavigates to the page while the dialog is still rendered
The fix is to return early from the handler whenever isDialogOpen is true (for any key, not just Escape), or to add if (isDialogOpen) return; after the Escape check.
| const onDocumentKeyDown = (event: KeyboardEvent): void => { | |
| if (isDialogOpen && event.key === "Escape") { | |
| event.preventDefault(); | |
| closeDialog(); | |
| return; | |
| } | |
| if (isEditableTarget({ target: event.target })) { | |
| return; | |
| } | |
| const shortcut = keyboardEventToShortcut({ event }); | |
| if (!shortcut) { | |
| return; | |
| } | |
| const bookmark = bookmarks.find((entry) => entry.shortcut === shortcut); | |
| if (!bookmark) { | |
| return; | |
| } | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| void openBookmark({ bookmark }); | |
| }; | |
| const onDocumentKeyDown = (event: KeyboardEvent): void => { | |
| if (isDialogOpen) { | |
| if (event.key === "Escape") { | |
| event.preventDefault(); | |
| closeDialog(); | |
| } | |
| return; | |
| } | |
| if (isEditableTarget({ target: event.target })) { | |
| return; | |
| } | |
| const shortcut = keyboardEventToShortcut({ event }); | |
| if (!shortcut) { | |
| return; | |
| } | |
| const bookmark = bookmarks.find((entry) => entry.shortcut === shortcut); | |
| if (!bookmark) { | |
| return; | |
| } | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| void openBookmark({ bookmark }); | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Uh oh!
There was an error while loading. Please reload this page.