ENG-1734 Add Roam semantic search to Advanced Node Search#1186
ENG-1734 Add Roam semantic search to Advanced Node Search#1186trangdoan982 wants to merge 11 commits into
Conversation
Wire Advanced Node Search to Roam semantic search when enabled, with discourse-note filtering and MiniSearch fallback when fewer than five semantic results remain. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-authored-by: Cursor <cursoragent@cursor.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Return frozen docked results synchronously and avoid persisting empty results that clobber the sidebar registry on mount. Co-authored-by: Cursor <cursoragent@cursor.com>
Use main-branch inline docked query check, remove dockedSearchSnapshot helper and Roam tests, and apply sort in useMemo so sort changes do not re-run async search. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Extract shared search hit types and remove DiscourseSearchHit adapter conversions in the Advanced Node Search hook. Co-authored-by: Cursor <cursoragent@cursor.com>
Use searchDiscourseNodes for the semantic-plus-miniSearch orchestrator, searchDiscourseNodesWithMiniSearch for MiniSearch, and ScoredSearchResult instead of hit-based names. Co-authored-by: Cursor <cursoragent@cursor.com>
Sort runs in useMemo only; the search effect does not depend on sort config. Co-authored-by: Cursor <cursoragent@cursor.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop temporary search logging, clear stale results when docking or starting a new search, and keep semantic hits first when sorting by relevance. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed open review feedback in 7acc534:
Also merged latest |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughRefactors discourse node search into a shared type system ( ChangesSemantic + MiniSearch Discourse Node Search
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Hook as useAdvancedNodeSearchResults
participant Search as searchDiscourseNodes
participant Semantic as runRoamSemanticSearch
participant Mini as runMiniSearch
Hook->>Search: query, nodeTypes, resultsByUid, runMiniSearch
Search->>Search: check semantic search enabled
alt semantic disabled or errors
Search->>Mini: runMiniSearch()
Mini-->>Search: ScoredSearchResult[]
else semantic enabled
Search->>Semantic: query
Semantic-->>Search: filteredResults
alt sufficient semantic results
Search-->>Hook: semantic ScoredSearchResult[]
else insufficient results
Search->>Mini: runMiniSearch()
Mini-->>Search: ScoredSearchResult[]
Search->>Search: combineSemanticAndMiniSearchResults
end
end
Search-->>Hook: ScoredSearchResult[]
Hook->>Hook: sortSearchResults(scoredResults, sort)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/roam/src/components/AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.tsOops! Something went wrong! :( ESLint: 8.57.1 YAMLException: Cannot read config file: /apps/roam/eslint.config.mjs 5 | export default [ apps/roam/src/components/AdvancedNodeSearchDialog/utils.tsOops! Something went wrong! :( ESLint: 8.57.1 YAMLException: Cannot read config file: /apps/roam/eslint.config.mjs 5 | export default [ apps/roam/src/utils/discourseNodeSearchProviders.tsOops! Something went wrong! :( ESLint: 8.57.1 YAMLException: Cannot read config file: /apps/roam/eslint.config.mjs 5 | export default [
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: 2
🧹 Nitpick comments (2)
apps/roam/src/utils/discourseNodeSearchTypes.ts (1)
39-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented fallback limitation and inconsistent placeholder defaults.
When
uidisn't found inresultsByUid, the placeholderSearchResultsilently dropsexcerpt,createdAt, andlastModified(empty strings) but setsauthorNameto"Unknown"instead of"". This inconsistency could surface oddly in the UI (e.g., "Unknown" author next to blank dates), and the fact that this fallback path exists at all (and why real fields are unavailable) isn't documented anywhere.As per coding guidelines,
**/*.{ts,tsx,js,jsx,md,mdx}files should "Document limitations, known bugs, or edge cases where behavior may not align with expectations."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/utils/discourseNodeSearchTypes.ts` around lines 39 - 73, The semantic fallback in toScoredSearchResultFromSemantic creates a partial SearchResult when resultsByUid has no match, but its placeholder defaults are inconsistent and the limitation is undocumented. Update the fallback in toScoredSearchResultFromSemantic so the placeholder fields use a consistent convention, and add a brief comment/doc note near this path explaining that the data is incomplete because no indexed result was found and which fields are intentionally unavailable.Source: Coding guidelines
apps/roam/src/components/AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.ts (1)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
excludeDefaultNodesinstead of duplicating the backedBy filter.
getDiscourseNodes.tsalready exportsexcludeDefaultNodesfor exactly this "drop default-backed nodes" check. Inliningnode.backedBy === "user"here duplicates that logic and risks drifting if the definition of "default" changes.♻️ Reuse existing utility
-import getDiscourseNodes from "~/utils/getDiscourseNodes"; +import getDiscourseNodes, { excludeDefaultNodes } from "~/utils/getDiscourseNodes"; ... const discourseNodes = getDiscourseNodes().filter( - (node) => - node.backedBy === "user" && - (!typeFilter || typeFilter.includes(node.type)), + (node) => + excludeDefaultNodes(node) && + (!typeFilter || typeFilter.includes(node.type)), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/components/AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.ts` around lines 71 - 75, The discourse node filtering in useAdvancedNodeSearchResults duplicates the default-node check by inlining node.backedBy === "user". Update the discourseNodes computation to reuse the existing excludeDefaultNodes helper exported from getDiscourseNodes.ts, then keep the typeFilter condition on top of that shared utility so the filtering logic stays consistent and centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/roam/src/utils/discourseNodeSearchTypes.ts`:
- Around line 20-37: The combineSemanticAndMiniSearchResults helper currently
seeds seenUids from semantic but does not deduplicate semantic entries
themselves, so repeated uid values can still appear in the returned list. Update
combineSemanticAndMiniSearchResults to filter the semantic array with the same
seenUids logic used for miniSearch before building combined, keeping only the
first ScoredSearchResult per uid.
In `@apps/roam/src/utils/searchDiscourseNodes.ts`:
- Around line 21-69: The MiniSearch fallback in searchDiscourseNodes is not
protected, so a thrown error from runMiniSearch() can still reject the search
update. Update searchDiscourseNodes to invoke runMiniSearch through a safe
wrapper in both the isRoamSemanticSearchEnabled() disabled branch and the
catch/combine fallback path, and return an empty array if MiniSearch fails so
the function always degrades cleanly.
---
Nitpick comments:
In
`@apps/roam/src/components/AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.ts`:
- Around line 71-75: The discourse node filtering in
useAdvancedNodeSearchResults duplicates the default-node check by inlining
node.backedBy === "user". Update the discourseNodes computation to reuse the
existing excludeDefaultNodes helper exported from getDiscourseNodes.ts, then
keep the typeFilter condition on top of that shared utility so the filtering
logic stays consistent and centralized.
In `@apps/roam/src/utils/discourseNodeSearchTypes.ts`:
- Around line 39-73: The semantic fallback in toScoredSearchResultFromSemantic
creates a partial SearchResult when resultsByUid has no match, but its
placeholder defaults are inconsistent and the limitation is undocumented. Update
the fallback in toScoredSearchResultFromSemantic so the placeholder fields use a
consistent convention, and add a brief comment/doc note near this path
explaining that the data is incomplete because no indexed result was found and
which fields are intentionally unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b498032-11de-469b-b587-52f3e638f6c8
📒 Files selected for processing (5)
apps/roam/src/components/AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.tsapps/roam/src/components/AdvancedNodeSearchDialog/utils.tsapps/roam/src/utils/discourseNodeSearchProviders.tsapps/roam/src/utils/discourseNodeSearchTypes.tsapps/roam/src/utils/searchDiscourseNodes.ts
| export const combineSemanticAndMiniSearchResults = ({ | ||
| semantic, | ||
| miniSearch, | ||
| }: { | ||
| semantic: ScoredSearchResult[]; | ||
| miniSearch: ScoredSearchResult[]; | ||
| }): ScoredSearchResult[] => { | ||
| const seenUids = new Set(semantic.map((entry) => entry.result.uid)); | ||
| const combined = [...semantic]; | ||
|
|
||
| miniSearch.forEach((entry) => { | ||
| if (seenUids.has(entry.result.uid)) return; | ||
| seenUids.add(entry.result.uid); | ||
| combined.push(entry); | ||
| }); | ||
|
|
||
| return combined; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline apps/roam/src/utils/discourseNodeSearchTypes.ts --view expanded || true
printf '\n== File contents ==\n'
cat -n apps/roam/src/utils/discourseNodeSearchTypes.ts
printf '\n== Search usages ==\n'
rg -n "combineSemanticAndMiniSearchResults|ScoredSearchResult|semantic.*result.uid|miniSearch" apps/roam/src -SRepository: DiscourseGraphs/discourse-graph
Length of output: 8447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== searchDiscourseNodes ==\n'
cat -n apps/roam/src/utils/searchDiscourseNodes.ts
printf '\n== Advanced search semantic path ==\n'
sed -n '250,320p' apps/roam/src/components/AdvancedNodeSearchDialog/utils.ts
printf '\n== useAdvancedNodeSearchResults ==\n'
sed -n '1,140p' apps/roam/src/components/AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.ts
printf '\n== semantic provider result shaping ==\n'
sed -n '330,420p' apps/roam/src/utils/discourseNodeSearchProviders.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 9516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== runRoamSemanticSearch definition ==\n'
rg -n "export .*runRoamSemanticSearch|const runRoamSemanticSearch|function runRoamSemanticSearch" apps/roam/src/utils/discourseNodeSearchProviders.ts apps/roam/src -S
printf '\n== relevant provider section ==\n'
sed -n '480,620p' apps/roam/src/utils/discourseNodeSearchProviders.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 3341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,320p' apps/roam/src/utils/discourseNodeSearchProviders.ts
printf '\n== filterResultsToDiscourseNodeTitles ==\n'
rg -n "filterResultsToDiscourseNodeTitles" apps/roam/src/utils/discourseNodeSearchProviders.ts apps/roam/src -n -A 80 -B 20Repository: DiscourseGraphs/discourse-graph
Length of output: 46659
🌐 Web query:
Roam Alpha API semanticSearch returns duplicate page hits unique uid contract
💡 Result:
The Roam Alpha API provides a semanticSearch functionality, which is separate from the standard Datalog-based q query API [1][2][3]. If you are encountering duplicate page hits when using this function, it is generally not a violation of the underlying UID contract, as Roam's database architecture relies on unique UIDs for all entities (blocks and pages) [3]. Technical considerations regarding this behavior include: 1. Data Normalization and Duplication: The semantic search index may occasionally include multiple references or "hits" that resolve to the same underlying entity UID due to the nature of how content is chunked or indexed by the semantic engine [4]. If your application logic expects a one-to-one mapping between search hits and unique entities, you should implement client-side deduplication using the unique uid (or block-uid) field present in the result object [1][5][3]. 2. Querying Alternatives: If semanticSearch is returning redundant data that complicates your integration, you can use the more deterministic Datalog query interface (roamAlphaAPI.q) [2][6][3]. Datalog queries allow you to use the distinct keyword or explicitly group results by UID, ensuring that you only retrieve a single, unique record for each entity regardless of how many blocks or page references match your criteria [3]. 3. API Contracts: There is no formal "unique hit" contract for the semantic search API that guarantees a distinct list of results [1]. The API is intended to provide relevant search context, which may include multiple fragments of a single page if they meet the semantic threshold. Always rely on the uid property as the primary key for identifying and de-duplicating entities within your application [5][3]. If you are building an extension or integration, verify that you are checking window.roamAlphaAPI.data.semanticSearchEnabled() before invocation to ensure the environment is correctly initialized [1].
Citations:
- 1: [codex] Add semantic search Roam API types RoamJS/roamjs-components#61
- 2: https://registry.npmjs.org/%40roam-research%2Froam-api-sdk
- 3: https://github.com/2b3pro/roam-research-mcp/blob/main/Roam_Research_Datalog_Cheatsheet.md
- 4: https://github.com/MaskyS/roam-embeddings-search
- 5: https://github.com/forrestchang/roam-research-cli
- 6: https://registry.npmjs.org/@roam-research/roam-api-sdk
Dedup semantic hits before combining The Roam semantic path still passes repeated uids through, so duplicate semantic hits can appear twice in the final list. Dedup semantic with the same seenUids pass used for miniSearch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/roam/src/utils/discourseNodeSearchTypes.ts` around lines 20 - 37, The
combineSemanticAndMiniSearchResults helper currently seeds seenUids from
semantic but does not deduplicate semantic entries themselves, so repeated uid
values can still appear in the returned list. Update
combineSemanticAndMiniSearchResults to filter the semantic array with the same
seenUids logic used for miniSearch before building combined, keeping only the
first ScoredSearchResult per uid.
| export const searchDiscourseNodes = async ({ | ||
| nodeTypes, | ||
| query, | ||
| resultsByUid, | ||
| runMiniSearch, | ||
| }: { | ||
| nodeTypes: DiscourseNode[]; | ||
| query: string; | ||
| resultsByUid: Map<string, SearchResult>; | ||
| runMiniSearch: () => ScoredSearchResult[]; | ||
| }): Promise<ScoredSearchResult[]> => { | ||
| const trimmedQuery = query.trim(); | ||
| if (!trimmedQuery) return []; | ||
|
|
||
| if (!isRoamSemanticSearchEnabled()) { | ||
| return runMiniSearch(); | ||
| } | ||
|
|
||
| try { | ||
| const providerResult = await runRoamSemanticSearch({ | ||
| nodeTypes, | ||
| query: trimmedQuery, | ||
| }); | ||
| const semanticResults = providerResult.filteredResults.map((item) => | ||
| toScoredSearchResultFromSemantic({ | ||
| uid: item.uid, | ||
| title: item.text, | ||
| type: item.type, | ||
| nodeTypeLabel: item.nodeTypeLabel, | ||
| score: item.score ?? 0, | ||
| resultsByUid, | ||
| }), | ||
| ); | ||
|
|
||
| if ( | ||
| providerResult.filteredResultCount >= | ||
| SEMANTIC_SEARCH_MIN_DISCOURSE_RESULTS | ||
| ) { | ||
| return semanticResults; | ||
| } | ||
|
|
||
| return combineSemanticAndMiniSearchResults({ | ||
| semantic: semanticResults, | ||
| miniSearch: runMiniSearch(), | ||
| }); | ||
| } catch { | ||
| return runMiniSearch(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant search utilities and call sites.
git ls-files 'apps/roam/src/**/*search*' 'apps/roam/src/**/*Search*' 'apps/roam/src/**/*mini*' | sed -n '1,200p'
printf '\n--- searchDiscourseNodes.ts ---\n'
cat -n apps/roam/src/utils/searchDiscourseNodes.ts | sed -n '1,220p'
printf '\n--- useAdvancedNodeSearchResults references ---\n'
rg -n "useAdvancedNodeSearchResults|searchDiscourseNodes|runMiniSearch|MiniSearch" apps/roam/src -A 3 -B 3Repository: DiscourseGraphs/discourse-graph
Length of output: 29073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- AdvancedNodeSearchDialog/utils.ts (search helper) ---\n'
cat -n apps/roam/src/components/AdvancedNodeSearchDialog/utils.ts | sed -n '240,340p'
printf '\n--- AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.ts (fallback path) ---\n'
cat -n apps/roam/src/components/AdvancedNodeSearchDialog/useAdvancedNodeSearchResults.ts | sed -n '70,110p'
printf '\n--- MiniSearch search-error handling in the repo ---\n'
rg -n "try \\{|catch \\{|miniSearch\\.search\\(|search\\(MiniSearch\\.wildcard|searchTerm" apps/roam/src/components/AdvancedNodeSearchDialog apps/roam/src/utils -A 3 -B 3Repository: DiscourseGraphs/discourse-graph
Length of output: 50387
🌐 Web query:
MiniSearch search() throws invalid query syntax malformed query exception documentation
💡 Result:
MiniSearch does not explicitly throw a "malformed query" exception or a generic "invalid query syntax" error in its official documentation or API. Instead, errors encountered during the search process typically manifest as JavaScript runtime exceptions—such as TypeErrors—triggered when the search() method receives unexpected input types or structures [1][2]. Common causes for these runtime errors include: 1. Wildcard Symbol Misalignment: If you use MiniSearch.wildcard, errors can occur if there is a mismatch in the MiniSearch instance or package version importing the symbol versus the instance performing the search [1][2]. This often happens in environments like Next.js or edge runtime setups where multiple instances of the library might be active, causing the MiniSearch.wildcard check to fail and the engine to incorrectly process the input as a different type of query [1][2]. 2. Incorrect Query Expression Tree: When passing an object as a query (an expression tree) rather than a string, the structure must strictly adhere to the Query type, which expects queries (an array of subqueries) and a combineWith operator [3][4][5]. Providing a malformed object that lacks these required properties or passes invalid types to the queries field will lead to runtime errors when the internal executeQuery method attempts to map or iterate over them [6][1][3]. If you are experiencing search-time errors, verify the following: - Ensure the input is either a plain string or a correctly formatted query expression tree object (containing a queries array and a combineWith field) [4][5]. - If using MiniSearch.wildcard, ensure it is being imported from the same package bundle that initialized the MiniSearch instance [1][2]. - Check your environment for potential multiple-instance issues (e.g., singleton patterns or conflicting dependency versions) if running in complex frameworks like Next.js [1]. For further debugging, inspect the stack trace to see if the error occurs within executeQuery or executeQuerySpec, as these are the primary internal methods where query processing occurs [6][3].
Citations:
- 1: [BUG] global wildcard symbol is not safe. lucaong/minisearch#307
- 2: using
MiniSearch.wildcardthrows an error lucaong/react-minisearch#42 - 3: https://cdn.jsdelivr.net/npm/minisearch@7.1.2/src/MiniSearch.ts
- 4: https://github.com/lucaong/minisearch/blob/master/src/MiniSearch.ts
- 5: Combining and / or for search terms lucaong/minisearch#100
- 6: https://github.com/lucaong/minisearch/blob/2e2b3b791858abf665c842800069728c8e3666c1/src/MiniSearch.ts
Guard the MiniSearch fallback too
runMiniSearch() is called directly in the disabled-search branch and again in the fallback path. If that helper ever throws, the recovery path throws as well and the search result update can still reject; wrap it in a safe fallback so this degrades to [] instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/roam/src/utils/searchDiscourseNodes.ts` around lines 21 - 69, The
MiniSearch fallback in searchDiscourseNodes is not protected, so a thrown error
from runMiniSearch() can still reject the search update. Update
searchDiscourseNodes to invoke runMiniSearch through a safe wrapper in both the
isRoamSemanticSearchEnabled() disabled branch and the catch/combine fallback
path, and return an empty array if MiniSearch fails so the function always
degrades cleanly.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I think the semantic result ordering needs another pass.
Right now we take Roam semantic search results, filter them to discourse-node pages, then sort semantic results by score in sortSearchResults.
I think we should preserve Roam’s returned order for semantic results. A clean fix would be to store a semanticRank/providerRank when mapping providerResult.filteredResults, then for relevance sorting:
- semantic results stay before MiniSearch results
- semantic-vs-semantic uses Roam return order
- MiniSearch-vs-MiniSearch uses MiniSearch score
- combined sparse fallback still dedupes by
uid
That would better match the user expectation that Roam semantic search’s own ordering is the relevance ordering.
| comparison = compareNumbers(aEntry.score, bEntry.score, sort.direction); | ||
| break; | ||
| case "alphabetical": | ||
| comparison = compareStrings( |
There was a problem hiding this comment.
Looks like this is doing some odd scoring even when roam's semantic search returns properly scored results:
Ours:
Roam's:
2 participants
https://www.loom.com/share/acfb9a8fcae24dea892a655974210c52
Summary
discourseNodeSemanticSearchorchestrator that runs Roam semantic search whensemanticSearchEnabled()is true, filters to discourse notes, and falls back to MiniSearch when fewer than five post-filter results remain or the API errorsTest plan
pnpm --filter roam testpnpm --filter roam check-typespnpm --filter roam lint->> I wasn't able to test this because semantic search always returns more than enough results
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Refactor