[ENG-261] Query function for DG summoning menu #150
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe changes refactor the DiscourseNodeSearchMenu component to dynamically fetch and filter discourse node types, implement asynchronous loading and debounced search, and update UI and event handling accordingly. Additionally, a utility function for escaping strings is introduced, and a minor formatting change is made in an observer utility file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeSearchMenu
participant RoamAPI
User->>NodeSearchMenu: Opens search menu
NodeSearchMenu->>RoamAPI: Fetch discourse node types (async)
RoamAPI-->>NodeSearchMenu: Return discourse node types
NodeSearchMenu->>User: Display loading indicator, then show types
User->>NodeSearchMenu: Types search term (debounced)
NodeSearchMenu->>RoamAPI: Query nodes by regex for each type
RoamAPI-->>NodeSearchMenu: Return matching nodes
NodeSearchMenu->>User: Display grouped search results
User->>NodeSearchMenu: Selects a result
NodeSearchMenu->>RoamAPI: Insert node reference, manage cursor/focus
NodeSearchMenu->>User: Close menu
Possibly related PRs
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/index.ts (1)
141-144: Consider defensive removal inunloadIf another script removes the listener earlier,
removeEventListeneris still safe, but you might guard with an existence check for symmetry with the registration block (minor).apps/roam/src/utils/initializeObserversAndListeners.ts (1)
229-234: Minor: listeners array → object mismatch
observersis still an array butlistenersis now an object. To keep the API symmetrical you may want to plural-rename tolistenerMapor similar.apps/roam/src/components/DiscourseNodeSearchMenu.tsx (3)
70-84: Potential O(N²) spread in reduceUsing spread inside every
.reduce()iteration reallocates and copies the accumulator:const initialCheckedTypes = allNodeTypes.reduce( (acc, type) => ({ ...acc, [type.type]: true }), {}, );For ~hundreds of discourse types this can be noticeably slower.
-const initialCheckedTypes = allNodeTypes.reduce( - (acc, type) => ({ ...acc, [type.type]: true }), - {}, -); +const initialCheckedTypes: Record<string, boolean> = {}; +allNodeTypes.forEach((t) => { + initialCheckedTypes[t.type] = true; +});
291-296: Remove redundant!!cast
textarea.closest(...)returns an element | null, which is already truthy/falsy:-const listeningEl = !!textarea.closest(".rm-reference-item") - ? textarea.parentElement - : textarea; +const listeningEl = textarea.closest(".rm-reference-item") + ? textarea.parentElement + : textarea;🧰 Tools
🪛 Biome (1.9.4)
[error] 291-291: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
214-219: Analytics event fires before block update completes
posthog.captureis executed synchronously after schedulingupdateBlock. If the update fails the analytics becomes misleading. Capture inside thethenofupdateBlockor useawait.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx(1 hunks)apps/roam/src/index.ts(2 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
renderDiscourseNodeSearchMenu(440-459)
🪛 Biome (1.9.4)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx
[error] 291-291: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 109-109: 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)
[error] 115-115: 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 (4)
apps/roam/src/index.ts (1)
101-110: ✅ Correctly integrates new search-trigger listenerNice job updating the destructuring and registering the new
discourseNodeSearchTriggerListener. The listener is added and later removed, so no memory-leak concerns here.apps/roam/src/utils/initializeObserversAndListeners.ts (2)
37-38: Import looks goodNew import is scoped and tree-shake-able.
45-50: Good shift from array → named objectReturning a named object (
{ pageActionListener, … }) increases clarity and prevents parameter order bugs.apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
255-265: Suppress horizontal-arrow propagation only when neededPreventing default on
ArrowLeft/ArrowRightglobally can block users from normal cursor movement in the textarea while the menu is open. Consider handling those keys only when you use them for navigation.
trangdoan982
changed the base branch from
main
to
eng-235-create-dg-summoning-menu
mdroidian
changed the base branch from
eng-235-create-dg-summoning-menu
to
eng-259-observer
trangdoan982
changed the base branch from
eng-259-observer
to
eng-235-create-dg-summoning-menu
trangdoan982
force-pushed
the
eng-261-menu-query
branch
from
62af5c5 to
2f815cc
Compare
trangdoan982
force-pushed
the
eng-261-menu-query
branch
from
0b03459 to
0063494
Compare
|
@coderabbitai review |
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
129-132: O(n²) spread insidereduce– convert to mutable pushStatic analysis correctly flags this pattern; each spread copies the accumulator.
-const initialSearchResults = allNodeTypes.reduce( - (acc, type) => ({ ...acc, [type.type]: [] }), - {}, -); +const initialSearchResults: Record<string, Result[]> = {}; +allNodeTypes.forEach((t) => { + initialSearchResults[t.type] = []; +});This brings the complexity down to O(n).
🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: 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 ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (3)
apps/roam/src/utils/formatUtils.ts (1)
198-201: Avoid forcing lowercase & broaden escaping scope
escapeCljStringsilently lower-cases every input, which may unintentionally break case-sensitive searches (includes?is case-sensitive) and alter user supplied text.
Additionally, only back-slashes and double-quotes are escaped – single quotes, back-ticks and new-lines may also appear in user input and break the Clojure string literal.-export const escapeCljString = (str: string) => { - return str.replace(/\\/g, "\\\\").replace(/"/g, '\\"').toLowerCase(); -}; +export const escapeCljString = (str: string) => { + // Escape \" \\ \n and leave the original casing intact + return str + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"') + .replace(/\n/g, "\\n"); +};If a lower-case comparison is required, convert both operands at the call site instead of mutating the original string.
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (2)
65-75: Use browser-compatible timer type
useRef<NodeJS.Timeout>pulls the Node typings; in a browser build this often resolves tonumber, causing type-mismatch noise. PreferReturnType<typeof setTimeout>.-const searchTimeoutRef = useRef<NodeJS.Timeout | null>(null); +const searchTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
469-481: Side-effectfulcurrentGlobalIndexmakes render order brittleRelying on a mutable counter mutated during render couples the visual list to the evaluation order. Use the already-computed
allItemsarray to deriveisActiveinstead:{searchResults[type.type]?.map((item, localIdx) => { - currentGlobalIndex++; - const isActive = currentGlobalIndex === activeIndex; + const globalIdx = allItems.findIndex((i) => i.item.uid === item.uid); + const isActive = globalIdx === activeIndex;This removes the hidden dependency and works even if render order ever changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx(7 hunks)apps/roam/src/utils/formatUtils.ts(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/roam/src/utils/initializeObserversAndListeners.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx
[error] 130-130: 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)
* [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
https://www.loom.com/share/b76845e3d8934501b6a89fae687e5d95
Summary by CodeRabbit