[ENG-299] Trigger for DG summoning menu #160
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe changes introduce a customizable trigger key for the Discourse Node Search Menu, allowing users to configure which key combination opens the search. This includes exporting utility functions for key normalization, adding a settings UI for the trigger, updating event listeners to use the dynamic trigger, and adjusting logic for inserting the trigger character and rendering the menu. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI as NodeSearchMenuTriggerComponent
participant ExtensionAPI
participant RoamBlock as Roam Block Textarea
participant Listener as Trigger Listener
participant Menu as NodeSearchMenu
User->>SettingsUI: Open settings, configure trigger key
SettingsUI->>ExtensionAPI: Save trigger key combo
User->>RoamBlock: Focus and press configured trigger key
RoamBlock->>Listener: Keydown event
Listener->>ExtensionAPI: Retrieve trigger key combo
Listener->>Listener: Check if key combo matches
alt Combo matches
Listener->>RoamBlock: Insert trigger if needed
Listener->>Menu: Render NodeSearchMenu at cursor
end
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/roam/src/components/settings/HomePersonalSettings.tsx (1)
32-41: Nice UX consistency – minor copy nitGreat to see the new trigger setting mirroring the existing “Personal Node Menu Trigger”.
If you want absolute wording parity with the sibling setting, consider dropping “Customize” so both descriptions start with a verb (“Override …” / “Customize …”) or both with “Override …”. Totally optional.apps/roam/src/components/DiscourseNodeMenu.tsx (1)
199-228: Utilities are now exported – consider relocating to avoid future tanglingMaking
isMac,MODIFIER_BIT_MASKS,ALIASES, andnormalizeKeyCombopublic is fine for now, but this component file is already large and heavily UI-oriented. As more features start depending on these helpers, moving them (plusgetModifiersFromCombo) into a small~/utils/keyboard.ts(or similar) would
• keep concerns separated,
• prevent accidental circular-import chains, and
• speed up unit-testing such pure helpers.apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
526-626: Trigger component duplicates existing logic – extract a shared helper
NodeSearchMenuTriggerComponentis nearly a copy ofNodeMenuTriggerComponentwith a small special-char map difference. Duplicating ~100 lines makes maintenance harder (bug-fixes & behaviour tweaks must be applied twice).Consider:
- Create a generic
<KeyComboInput>that accepts props
•settingKey(e.g."discourse-node-search-trigger"), and
•defaultCombo(e.g.{ modifiers: 0, key: "@" }),
then reuse it for both Personal-Menu and Search-Menu triggers.- Move
specialCharMapinto a shared keyboard-helper module for reuse.No immediate functional issue, just a maintainability win.
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
252-253: Remove leftover console debugging statements
console.loglines will spam the browser console in production and leak internal data. Please remove them or guard them behind a"development"flag.- console.log("evt.key", evt.key); - console.log("personal trigger", personalTrigger, nodeSearchTriggerCombo);
247-249: Variable naming is misleading
personalTrigger/personalModifiersactually refer to the “node-search” trigger, not the “personal” node-menu trigger. Consider renaming tonodeSearchTrigger/nodeSearchModifiersfor clarity and future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/DiscourseNodeMenu.tsx(2 hunks)apps/roam/src/components/DiscourseNodeSearchMenu.tsx(5 hunks)apps/roam/src/components/settings/HomePersonalSettings.tsx(2 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/roam/src/components/settings/HomePersonalSettings.tsx (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
NodeSearchMenuTriggerComponent(526-626)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
apps/roam/src/components/DiscourseNodeMenu.tsx (2)
getModifiersFromCombo(230-238)normalizeKeyCombo(222-228)
🔇 Additional comments (1)
apps/roam/src/components/settings/HomePersonalSettings.tsx (1)
5-6: Import looks good
NodeSearchMenuTriggerComponentis correctly referenced from the Search-menu module and passed through the same alias path convention used elsewhere. No concerns here.
| const textAfterTrigger = textarea.value.substring( | ||
| triggerPosition, | ||
| textarea.selectionStart, | ||
| ); | ||
| const match = atTriggerRegex.exec(textBeforeCursor); | ||
| if (match) { | ||
| debouncedSearchTerm(match[1]); | ||
| } else { | ||
| onClose(); | ||
| return; | ||
|
|
||
| if (textAfterTrigger.length > 0) { | ||
| debouncedSearchTerm(textAfterTrigger); | ||
| } | ||
| }, [textarea, onClose, debouncedSearchTerm, triggerPosition]); | ||
| }, [textarea, debouncedSearchTerm, triggerPosition]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Empty query leaves menu open – UI feels “stuck”
handleTextAreaInput no longer closes the menu when the user backspaces past the trigger.
That means the pop-over lingers with an empty result list, which can feel broken.
- if (textAfterTrigger.length > 0) {
- debouncedSearchTerm(textAfterTrigger);
- }
+ if (textAfterTrigger.length > 0) {
+ debouncedSearchTerm(textAfterTrigger);
+ } else {
+ onClose(); // hide menu when no query is present
+ }Alternatively, keep it open but call debouncedSearchTerm("") so the UI refreshes to an explicit “No matches” state.
📝 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.
| const textAfterTrigger = textarea.value.substring( | |
| triggerPosition, | |
| textarea.selectionStart, | |
| ); | |
| const match = atTriggerRegex.exec(textBeforeCursor); | |
| if (match) { | |
| debouncedSearchTerm(match[1]); | |
| } else { | |
| onClose(); | |
| return; | |
| if (textAfterTrigger.length > 0) { | |
| debouncedSearchTerm(textAfterTrigger); | |
| } | |
| }, [textarea, onClose, debouncedSearchTerm, triggerPosition]); | |
| }, [textarea, debouncedSearchTerm, triggerPosition]); | |
| const textAfterTrigger = textarea.value.substring( | |
| triggerPosition, | |
| textarea.selectionStart, | |
| ); | |
| if (textAfterTrigger.length > 0) { | |
| debouncedSearchTerm(textAfterTrigger); | |
| } else { | |
| onClose(); // hide menu when no query is present | |
| } | |
| }, [textarea, debouncedSearchTerm, triggerPosition]); |
🤖 Prompt for AI Agents
In apps/roam/src/components/DiscourseNodeSearchMenu.tsx around lines 246 to 254,
the current code does not close the menu or refresh the UI when the query is
empty after backspacing past the trigger, causing the menu to remain open with
no results. Modify the logic so that if textAfterTrigger is empty, either close
the menu explicitly or call debouncedSearchTerm with an empty string to refresh
the UI and show a "No matches" state, ensuring the menu does not feel stuck.
| if (!evt.isComposing && evt.key !== triggerChar) { | ||
| const text = textarea.value; | ||
| const newText = | ||
| text.slice(0, cursorPos) + triggerChar + text.slice(cursorPos); | ||
|
|
||
| const blockUid = getUids(textarea).blockUid; | ||
| if (blockUid) { | ||
| updateBlock({ uid: blockUid, text: newText }); | ||
| } | ||
| triggerPosition = cursorPos; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Asynchronous write may race with UI state and cursor management
updateBlock is asynchronous but the surrounding function is not async, and its returned promise is ignored.
• If the Roam API lags, renderDiscourseNodeSearchMenu can pop up before the block text actually includes the trigger character, causing a mismatch between what the menu thinks is in the block and the real DOM.
• The textarea value is never updated locally, so the user keeps seeing the pre-update text until Roam re-renders, which feels glitchy, and the caret position will jump.
- updateBlock({ uid: blockUid, text: newText });
+ await updateBlock({ uid: blockUid, text: newText });
+ // Keep the local textarea in-sync and restore cursor
+ textarea.value = newText;
+ textarea.setSelectionRange(triggerPosition + 1, triggerPosition + 1);Turning handleNodeSearchRender into an async function (and awaiting it from the listener) prevents the race and preserves UX.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/roam/src/utils/initializeObserversAndListeners.ts around lines 190 to
200, the updateBlock call is asynchronous but not awaited, causing potential
race conditions with UI updates and cursor management. To fix this, make the
surrounding function async and await the updateBlock call to ensure the block
text is updated before continuing. Also, update the textarea value locally to
reflect the new text immediately and manage the caret position to prevent jumps,
preserving a smooth user experience.
| if (evt.key === personalTrigger) { | ||
| triggerMatched = | ||
| (!personalModifiers.includes("ctrl") || evt.ctrlKey) && | ||
| (!personalModifiers.includes("shift") || evt.shiftKey) && | ||
| (!personalModifiers.includes("alt") || evt.altKey) && | ||
| (!personalModifiers.includes("meta") || evt.metaKey); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trigger matching is fragile – make it case-insensitive and reuse normalizeKeyCombo
evt.key differs between "d" and "D" depending on Shift, so Shift-modified combos can fail to match even when modifiers are correct. Compare keys after normalisation:
- if (evt.key === personalTrigger) {
+ if (evt.key.toLowerCase() === personalTrigger.toLowerCase()) {Even better, call the already-exported normalizeKeyCombo helper and compare the fully-normalised combo objects to avoid hand-rolled logic.
📝 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.
| if (evt.key === personalTrigger) { | |
| triggerMatched = | |
| (!personalModifiers.includes("ctrl") || evt.ctrlKey) && | |
| (!personalModifiers.includes("shift") || evt.shiftKey) && | |
| (!personalModifiers.includes("alt") || evt.altKey) && | |
| (!personalModifiers.includes("meta") || evt.metaKey); | |
| } | |
| if (evt.key.toLowerCase() === personalTrigger.toLowerCase()) { | |
| triggerMatched = | |
| (!personalModifiers.includes("ctrl") || evt.ctrlKey) && | |
| (!personalModifiers.includes("shift") || evt.shiftKey) && | |
| (!personalModifiers.includes("alt") || evt.altKey) && | |
| (!personalModifiers.includes("meta") || evt.metaKey); | |
| } |
🤖 Prompt for AI Agents
In apps/roam/src/utils/initializeObserversAndListeners.ts around lines 254 to
260, the current trigger matching logic compares evt.key directly, which is
case-sensitive and can fail for Shift-modified keys. To fix this, replace the
manual key and modifier checks by using the existing normalizeKeyCombo helper to
normalize both the event key combo and the personalTrigger combo, then compare
these normalized objects for equality. This ensures case-insensitive and
consistent matching without hand-rolled logic.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes