Repository navigation
ENG-1734 Add Roam semantic search to Advanced Node Search - #1186
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)
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
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:
Roam's semantic scores are not comparable across hits, so sorting by them reordered results away from what Roam returned. Record each provider's returned position as `rank` and sort semantic entries by it; MiniSearch entries still sort by score and stay below semantic ones. Also dedupe semantic uids when combining the two sources, and degrade to an empty list when the MiniSearch fallback itself throws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both providers already return results in relevance order, and combining them appends MiniSearch after semantic, so relevance sorting only has to reverse for ascending. Sorting by score re-derived that order from Roam scores that are not comparable across hits, and the uid tie-break broke ties in the process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Semantic results were never capped: MAX_RESULTS only applied inside the MiniSearch helper, so the semantic and combined paths could return far more. Move the constant down to the search types and slice every path. The docked panel persists results on every change, including the empty list published while an async search is in flight. If it unmounted in that window the cached empty list was treated as a hit on restore, so a query with matches stayed at zero results. Only reuse a non-empty cache. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Search became async with semantic search, but contentState only knew about isIndexLoading, so "No matches" rendered for the whole semanticSearch round trip. Track an in-flight flag and use it in the dialog and the docked panel. Also route the recovery path's runMiniSearch() through the same guard the other call sites use, so a throw there cannot become an unhandled rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-search-for-sync-enabled-graphs
The cap lives in searchDiscourseNodes, not in the combine helper, so the combine test could not prove it. Add searchDiscourseNodes coverage for the cap, the semantic-only threshold, the sparse combine, both fallbacks, and the blank query, and correct the combine test to state what it checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@mdroidian you're so right! bug fixed. The sorting function was the problem cuz Roam semantic search doesn't return a score value -> all the semantic results are actually ranked lower because the value defaults to 0 when there's no score. I fixed this bug: now Relevance is the default order, which will simply return the original order that was given by Roam semantic. If the search has to use both semantic + miniSearch, the ranking would prioritize the order from semantic search first. here's the screenshot of the same test case on graph discourse-graph: before fix: roam semantic search: after fix: |
The wrapper existed only to give the cap a single place to apply. One function with a single exit does the same thing without the indirection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Entire-Checkpoint: 01M35P984XJ2TWP81W573WTT9H
This branch was previously deployed
2 participants
https://www.loom.com/share/acfb9a8fcae24dea892a655974210c52
Reviewer brief
Result: Advanced Node Search runs Roam semantic search when the graph has it enabled, filters hits to discourse nodes, and renders them in the order Roam returned. MiniSearch stays the path for graphs without semantic search, and backfills when semantic returns fewer than five discourse nodes.
Review focus:
.semanticSearchreturns no score. A raw hit is{type, uid, topUids}and nothing else, verified againstdiscourse-graphs.score: item.score ?? 0therefore gave every semantic result a score of 0, every score comparison tied, and the sharedcomparison || a.uid.localeCompare(b.uid)tie-break decided the final order. Semantic results were ranked alphabetically by uid.The fix is that relevance sorting no longer derives order from score. Each provider already returns its own results in relevance order, so
sortSearchResultspreserves that order and only reverses it for ascending. Two consequences worth a careful look:filteredResultspreserves Roam's order,miniSearch.searchreturns score-descending, and combining concatenates semantic before MiniSearch.discourseNodeSearchTypes.test.tspins it.scoreandsourceare retained onScoredSearchResultbut no longer read for ordering. They are kept for diagnostics and possible rank fusion.Risk and follow-up:
getDiscourseNodes()and aresultsByUidmap are rebuilt per debounced keystroke, andsearchDiscourseNodesWithMiniSearchbuilds a second identical map. Memoizable, not addressed here.Verification
Unit tests (50 passing)
pnpm --filter roam test,check-types, andlintall pass.apps/roam/src/utils/__tests__/discourseNodeSearchTypes.test.tspins the ordering contract:zzzandaaastay in returned orderascinverts the full listsortSearchResultsdoes not reorder its argumentMAX_RESULTSLive proof: ordering in the Advanced Node Search UI
Driven through Playwright against a real graph, with
.semanticSearchstubbed to return a deliberately scrambled order so provider order, score order, and alphabetical order are all distinct. The UI matched provider order and neither of the others.Query
transparency and legitimacyagainst ten purpose-built claims. The only two titles containing the query words are highlighted as keyword matches and rank last, below five semantically relevant claims containing neither word.Real-graph ranking, before and after
Run on
discourse-graphs, which has semantic search enabled. 24 of 50 page hits were discourse nodes.Query
transparency and legitimacy:Across three queries the old sort displaced results by a mean of 5.9 to 6.8 positions. The node Roam ranks first moved from rank 3 to rank 1 on two of them.
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: not assessed.Standards check
$dg-pr-adherence-checkagainst the final diff and PR metadata.Local delegated full review
Findings addressed in this PR: results are now capped at
MAX_RESULTSon every path, the docked panel no longer caches the empty result list published while a search is in flight, the async search renders the indexing state instead of "No matches", and the recovery path routes through the same MiniSearch guard as the other call sites. Remaining findings are listed under Risk and follow-up.