[ENG-748] Change the way discourse node search query works #498
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR refactors DiscourseNodeSearchMenu to introduce local fuzzy filtering with per-type node result caching. It modifies API signatures (waitForBlock to accept object parameters), precomputes and caches node results on initial load, and implements cached fuzzy filtering for subsequent searches. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu as DiscourseNodeSearchMenu
participant Cache as allNodes State
participant Filter as filterNodesLocally
participant API as Roam API
Note over Menu: Component Mount
Menu->>API: Fetch all node types (pre-compute)
API-->>Cache: Store results per type
Note over Menu: User types query
User->>Menu: Search input
Menu->>Cache: Retrieve cached nodes
Menu->>Filter: Apply fuzzy filter
Filter-->>Menu: Filtered results
Menu->>User: Display filtered results
Note over Menu: User selects node
User->>Menu: Click/Enter selection
Menu->>API: waitForBlock({uid, text, ...})
API-->>Menu: Block focused/updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes This refactoring involves multiple interconnected logic changes (caching mechanism, fuzzy filtering, data access patterns) within a single file, requiring verification of state management correctness, caching integrity, and compatibility of the API signature changes throughout the component. Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (2)
153-173: Synchronous prefetch of all types can block UI on large graphs.This loops q() over all types in one tick, freezing the UI. Make it async and yield between queries.
Apply this diff:
- const fetchNodeTypes = () => { + const fetchNodeTypes = async () => { setIsLoading(true); @@ - const allNodesCache: Record<string, Result[]> = {}; - allNodeTypes.forEach((type) => { - allNodesCache[type.type] = searchNodesForType(type); - }); - setAllNodes(allNodesCache); + const allNodesCache: Record<string, Result[]> = {}; + for (const type of allNodeTypes) { + allNodesCache[type.type] = searchNodesForType(type); + // Yield to event loop to keep UI responsive + await new Promise((r) => setTimeout(r, 0)); + } + setAllNodes(allNodesCache);
611-614: Trigger is double-escaped; store raw, escape only at match time.Escaping on save plus escaping on use will break triggers like “+” and “\”. Save raw; build a safe regex later (already done in handleTextAreaInput).
Apply this diff:
- const trigger = value - .replace(/"/g, "") - .replace(/\\/g, "\\\\") - .replace(/\+/g, "\\+") - .trim(); + const trigger = value;
🧹 Nitpick comments (5)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (5)
37-47: waitForBlock API change looks good; consider tunable delay/backoff.Signature refactor is clean. Optionally add a delayMs/backoff to reduce tight polling and make timeouts clearer.
Also applies to: 54-62
106-136: Harden regex string embedding in Datalog.If regex.source contains newlines, the query string can break. Escape newlines in addition to quotes and backslashes.
Apply this diff:
- const regexPattern = regex.source - .replace(/\\/g, "\\\\") - .replace(/"/g, '\\"'); + const regexPattern = regex.source + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"') + .replace(/\n/g, "\\n");
138-150: Local fuzzy filter OK; consider capping results for UX/perf.For large node sets, cap per-type results (e.g., slice to top N) to avoid huge menus and layout thrash.
186-197: Filtering every type each keystroke: add cheap guard or cap.Minor perf nit: if you decide not to show “all results” by default, gate filtering until searchTerm length ≥ 2, or cap per-type results to reduce work.
581-581: React 18: replace deprecated ReactDOM.render/unmount.Use createRoot to avoid deprecated APIs.
Apply this diff:
- // eslint-disable-next-line react/no-deprecated - ReactDOM.render( + const root = (ReactDOM as any).createRoot + ? (require("react-dom/client").createRoot as (e: Element) => any)(parent) + : null; + const renderFn = root ? root.render.bind(root) : ReactDOM.render; + renderFn( @@ - // eslint-disable-next-line react/no-deprecated - ReactDOM.unmountComponentAtNode(parent); + if (root && root.unmount) root.unmount(); + else ReactDOM.unmountComponentAtNode(parent); parent.remove();Additionally, add (outside this hunk) at top-level imports:
// If you can modify imports: import { createRoot } from "react-dom/client";Also applies to: 587-588
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx(11 hunks)
🔇 Additional comments (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (1)
28-28: Import fuzzy via namespace for CommonJS
CommonJS-onlyfuzzylacks a default export in ESM; switch to:-import fuzzy from "fuzzy"; +import * as fuzzy from "fuzzy";or enable
esModuleInterop/allowSyntheticDefaultImports: truein your TypeScript config.
trangdoan982
force-pushed
the
eng-748-node-summoning-menu-not-showing-all-results
branch
from
ebc4520 to
36a2d9c
Compare
1 participant
https://www.loom.com/share/8706a4b84aa94e69b0cce35bffc76654
Summary by CodeRabbit