[ENG-299] Trigger setting for DG summoning menu #162
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce a configurable trigger string for the Node Search Menu, replacing the previously hardcoded "@" trigger. A new component enables users to set this trigger in the settings UI. The event listener logic and menu activation now dynamically use the user-defined trigger string, supporting multi-character triggers and updating relevant event dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI
participant ExtensionAPI
participant Listener
participant NodeSearchMenu
User->>SettingsUI: Enters new trigger string
SettingsUI->>ExtensionAPI: Save trigger to settings
Note over SettingsUI,ExtensionAPI: User refreshes page
User->>Listener: Types in textarea
Listener->>ExtensionAPI: Fetch trigger string
Listener->>Listener: Detect trigger in textarea input
alt Trigger detected
Listener->>NodeSearchMenu: Open menu with triggerText
end
Possibly related PRs
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
529-560: Well-implemented settings component.The NodeSearchMenuTriggerSetting component:
- Correctly manages state and persists changes to extension settings
- Properly sanitizes input by escaping special characters and trimming whitespace
- Has appropriate validation with maxLength
- Uses a sensible default placeholder
One suggestion for improvement:
Consider adding validation feedback when users enter invalid characters or empty strings. You could display a helper text that explains the restrictions and provides immediate feedback.
return ( <InputGroup value={nodeSearchTrigger} onChange={handleNodeSearchTriggerChange} placeholder="@" maxLength={5} + intent={nodeSearchTrigger ? "none" : "warning"} + helperText={!nodeSearchTrigger ? "Trigger cannot be empty" : undefined} /> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx(6 hunks)apps/roam/src/components/settings/HomePersonalSettings.tsx(2 hunks)apps/roam/src/index.ts(2 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
renderDiscourseNodeSearchMenu(508-527)
apps/roam/src/components/settings/HomePersonalSettings.tsx (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
NodeSearchMenuTriggerSetting(529-560)
🔇 Additional comments (9)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
192-238: Well-implemented dynamic trigger detection logic.The refactoring improves flexibility by replacing the hardcoded "@" trigger with a configurable setting. The implementation correctly handles:
- Checking if the trigger is at a valid position (start of line, after space, or after newline)
- Verifying the cursor position is immediately after the trigger
- Passing the appropriate positioning information to the search menu component
This approach enables multi-character triggers and improves user customization.
apps/roam/src/index.ts (2)
110-110: Correctly updated event listener type.Changing from "keydown" to "input" aligns with the modified trigger detection logic that now responds to input changes rather than specific keystrokes.
141-141: Event cleanup properly updated.The event removal correctly matches the new event type, ensuring proper cleanup on extension unload.
apps/roam/src/components/settings/HomePersonalSettings.tsx (2)
15-15: Import added for new component.The import is correctly placed with related imports.
32-40: Well-integrated settings UI component.The new trigger setting is logically placed near the related "Personal Node Menu Trigger" setting with appropriate labeling and description. The UI implementation follows the existing pattern for consistency.
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (4)
15-15: Added required imports.The additional imports for InputGroup and OnloadArgs support the new functionality.
Also applies to: 23-23
33-33: Props interface and component signature properly updated.The triggerText prop has been added to both the Props interface and component parameters, maintaining type safety.
Also applies to: 57-57
243-245: Robust regex pattern construction.The code now dynamically builds the regex pattern from the triggerText, properly escaping special characters to prevent regex syntax errors with custom triggers.
257-257: Updated dependency array in useCallback.The dependency array correctly includes the new triggerText prop to ensure the callback is regenerated when the trigger changes.
| target.classList.contains("rm-block-input") | ||
| ) { | ||
| const textarea = target as HTMLTextAreaElement; | ||
| const location = window.roamAlphaAPI.ui.getFocusedBlock(); |
There was a problem hiding this comment.
Ensure this is only called when customTrigger the evt.key. Right now this is being called on every keystroke.
There was a problem hiding this comment.
i think it needs to check every keystroke because we're trying to match a pattern (the key could be multiple chars, similar to smartblock) instead of listening to just 1 key.
There was a problem hiding this comment.
Can you walk me through the reason we need to call window.roamAlphaAPI.ui.getFocusedBlock() on every keystroke?
There was a problem hiding this comment.
ahh i see what you mean. yes good point. I moved getFocusedBlock() check to the last step right before rendering the search menu to optimize
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Couple nits, feel free to edit them now or defer.
trangdoan982
merged commit 59926c8
into
eng-235-create-dg-summoning-menu
* [ENG-259] Create Observer and UI pattern (#138) * create event listener for @ key * fix ui * address PR comments * cur progress * current progress * tested yayyy * add the checkbox UI * fix focus issue * rm logs * address PR comments * address PR comments * [ENG-261] Query function for DG summoning menu (#150) * cur progress * current progress * rm logs * address PR comments * address PR comments * query works * address PR comments * address PR comments * sm change * [ENG-299] Trigger setting for DG summoning menu (#162) * setting menu done * trigger works correctly * clean up * address PR comment * optimize getFocusedBlock() * address nit comments * fix bug (#166) * address PR comments
Context:
Testing:
https://www.loom.com/share/9f23f6b7dab8401c97918f3fd2f5bd8b
Summary by CodeRabbit
New Features
Refactor
Chores