[ENG-235] Create DG summoning menu #169
Conversation
* 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
* cur progress * current progress * rm logs * address PR comments * address PR comments * query works * address PR comments * address PR comments * sm change
* setting menu done * trigger works correctly * clean up * address PR comment * optimize getFocusedBlock() * address nit comments
|
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 📝 WalkthroughWalkthroughThis change introduces a configurable discourse node search menu in Roam Research. It adds a new React component for the search menu, integrates a trigger listener for activating the menu based on user-defined text, updates event listener management, and provides a settings UI for configuring the trigger string. Utility and observer logic are also extended to support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Textarea
participant DiscourseNodeSearchTriggerListener
participant NodeSearchMenu
participant RoamAPI
User->>Textarea: Types trigger string
Textarea-->>DiscourseNodeSearchTriggerListener: "input" event
DiscourseNodeSearchTriggerListener->>DiscourseNodeSearchMenu: Render menu near cursor
NodeSearchMenu->>RoamAPI: Fetch discourse node types and search results
RoamAPI-->>NodeSearchMenu: Return filtered node results
User->>NodeSearchMenu: Navigate/select node (keyboard/mouse)
NodeSearchMenu->>RoamAPI: Insert page reference at trigger position
NodeSearchMenu->>Textarea: Update block text and restore focus
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: 2
🧹 Nitpick comments (5)
apps/roam/src/utils/formatUtils.ts (1)
199-201: Looks good, but consider adding a JSDoc comment.The
escapeCljStringfunction correctly handles escaping backslashes and double quotes while converting to lowercase. This utility function will be useful for the DG summoning menu.Consider adding a JSDoc comment explaining the purpose and usage of this function:
+/** + * Escapes a string for use in Clojure/ClojureScript by escaping backslashes and double quotes, + * and converting to lowercase. + * @param str - The string to escape + * @returns The escaped string in lowercase + */ export const escapeCljString = (str: string) => { return str.replace(/\\/g, "\\\\").replace(/"/g, '\\"').toLowerCase(); };apps/roam/src/index.ts (1)
101-106: Good refactoring to named properties for easier access to listeners.Changing from array-based to named property access improves code readability and maintainability. The inclusion of the new
discourseNodeSearchTriggerListeneris well-structured.You might want to add a TypeScript interface for the listeners object structure to further improve code clarity.
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
193-241: Well-implemented event listener with proper validation.The implementation follows good practices with multiple validation steps:
- Checks for existing menu to prevent duplicates
- Validates the target is a Roam block textarea
- Ensures the trigger is valid and positioned correctly
- Confirms cursor position is right after the trigger
- Double-checks Roam's API state before rendering
Consider improving these aspects:
- The empty
onClosecallback could be a potential issue:renderDiscourseNodeSearchMenu({ - onClose: () => {}, + onClose: () => { + // Perform cleanup if needed when the menu is closed + }, textarea: textarea, triggerPosition: lastTriggerPos, triggerText: customTrigger, });
- Add error handling for the Roam API call:
- const isEditingBlock = !!window.roamAlphaAPI.ui.getFocusedBlock(); + let isEditingBlock = false; + try { + isEditingBlock = !!window.roamAlphaAPI.ui.getFocusedBlock(); + } catch (error) { + console.error("Failed to check focused block:", error); + }
- Consider adding a debug log for when the trigger is detected but conditions aren't met, which could help with troubleshooting.
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (2)
355-357: Global mutation during render risks mismatched active indices
currentGlobalIndexis a mutable outer variable incremented inside the JSX map.
Because it relies on side-effects during render it can:• Desynchronise if the render tree changes
• Break when React switches to concurrent renderingA safer, side-effect-free pattern:
-const isActive = currentGlobalIndex === activeIndex; +const globalIndex = allItems.findIndex( + (i) => i.item.uid === item.uid && i.typeIndex === typeIndex && i.itemIndex === itemIndex, +); +const isActive = globalIndex === activeIndex;Or compute
isActiveby comparing object identities without any external counter.Also applies to: 474-476
550-555: Trigger length capped at 5 characters may be too restrictive
maxLength={5}prevents users from choosing longer, mnemonic triggers (e.g. “/dgnode”).
Consider increasing or removing this limit to improve configurability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx(1 hunks)apps/roam/src/components/settings/HomePersonalSettings.tsx(2 hunks)apps/roam/src/index.ts(2 hunks)apps/roam/src/utils/formatUtils.ts(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/roam/src/components/settings/HomePersonalSettings.tsx (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
NodeSearchMenuTriggerSetting(526-557)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
renderDiscourseNodeSearchMenu(505-524)
🪛 Biome (1.9.4)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx
[error] 134-134: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (8)
apps/roam/src/components/settings/HomePersonalSettings.tsx (2)
15-15: LGTM - Import for the new NodeSearchMenuTriggerSetting component.The import statement correctly brings in the NodeSearchMenuTriggerSetting component from the DiscourseNodeSearchMenu module.
32-40: Good UI addition for the new Node Search Menu Trigger.The new labeled section follows the same pattern as other settings in this component, with a clear label, helpful description (noting the refresh requirement), and the appropriate component.
The addition integrates well with existing settings and provides users with a clear way to configure the new DG summoning menu trigger.
apps/roam/src/index.ts (2)
110-110: LGTM - Registration of the new input event listener.Properly adds the event listener for the new discourse node search trigger functionality.
141-141: LGTM - Proper cleanup of the event listener on unload.Ensures the input event listener is correctly removed during extension unload, preventing memory leaks.
apps/roam/src/utils/initializeObserversAndListeners.ts (4)
37-37: LGTM - Import for the render function.The import statement correctly brings in the renderDiscourseNodeSearchMenu function from the DiscourseNodeSearchMenu component.
44-51: Good update to the return type for more structured listeners.Changing from an array of EventListener to a named object with specific listener properties improves type safety and code clarity.
189-192: LGTM - Retrieving the custom trigger setting.Correctly retrieves the user-configured trigger string from the extension settings.
251-256: LGTM - Updated listener object structure.The new structure with named properties is more maintainable than the previous array approach.
Summary by CodeRabbit