ENG-1738: Render advanced search results as sidebar block#1071
ENG-1738: Render advanced search results as sidebar block#1071trangdoan982 wants to merge 15 commits into
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d392a6af2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
This is quite different than the functionality I was expecting.
On Apr 23 and May 7th, we discussed replicating the same functionality as the Roam native search which is to render the search as a component in the sidebar. But it looks like this did not make it into the scope doc/ticket. Upon looking at the scope doc/ticket, I can see where the confusion could have arose: "The active result opens in the main panel or side bar accordingly". This is quite ambiguous.
I believe this will be quite confusing to users based on the wording and it will leave them with multiple backlinks to clean up. I don't see the benefit of creating a page/blocks in this case as search is generally ephemeral.
That being said, there is a similar pattern in query builder called "Share To" where a user can insert search results as references to a given page, which may be something users request.
But for this use case let's change it to a rendered component or bring it up in the next Roam meeting / All Hands for clarification.
trangdoan982
force-pushed
the
eng-1738-platform-roam
branch
from
d392a6a to
0f66bc2
Compare
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ca5ed69b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
The biggest blocking change is the issue we discussed in our 1:1 of the sidebar hijacking the last window instead of creating it's own window to use/persist. We mentioned that bringing this (expected way to load custom windows in sidebar) up in Roam dev slack would be a good idea and suggesting a custom sidebar window type.
| useEffect(() => { | ||
| if (!debouncedSearchTerm) { | ||
| setResults([]); | ||
| return; | ||
| } | ||
|
|
||
| if (isIndexLoading || indexError || !miniSearchRef.current) { | ||
| if (!isDockedQuery) setResults([]); | ||
| return; | ||
| } | ||
|
|
||
| const scoredHits = searchIndexedNodes({ | ||
| miniSearch: miniSearchRef.current, | ||
| allResults: allResultsRef.current, | ||
| searchTerm: debouncedSearchTerm, | ||
| typeFilter: selectedNodeTypeIds.length ? selectedNodeTypeIds : undefined, | ||
| }); | ||
|
|
||
| setResults(sortSearchResults({ hits: scoredHits, sort })); | ||
| }, [ | ||
| debouncedSearchTerm, | ||
| indexError, | ||
| isDockedQuery, | ||
| isIndexLoading, | ||
| selectedNodeTypeIds, | ||
| sort, | ||
| ]); |
There was a problem hiding this comment.
Suggestion: derive search results during render instead of syncing with useEffect
Both AdvancedSearchDialog and AdvancedSearchSidebarPanel use an effect to run searchIndexedNodes → sortSearchResults → setResults whenever the debounced query, sort, filters, or index readiness change. That pattern causes an extra render on every search update (render → effect → setState → render).
Since search is synchronous, debounced, and capped at 50 results, this looks like derived data rather than a side effect. Consider replacing results state + the search useEffect with a useMemo (or inline derivation):
const results = useMemo(() => {
if (!debouncedSearchTerm) return [];
if (isIndexLoading || indexError || !miniSearchRef.current) {
// Sidebar only: keep docked placeholder while index loads
return isDockedQuery ? dockedResults : [];
}
const scoredHits = searchIndexedNodes({ /* ... */ });
return sortSearchResults({ hits: scoredHits, sort });
}, [debouncedSearchTerm, isIndexLoading, indexError, selectedNodeTypeIds, sort, /* sidebar: isDockedQuery, dockedResults */]);Benefits
- One render per input change instead of two
- Search logic lives in one place; no effect/state drift
- Same behavior, simpler mental model (“results = f(query, index, filters, sort)”)
Keep as useEffect
- Async index build (
buildSearchIndex) — that’s a real side effect - Debounce timer — still belongs in an effect (or
useDebouncedValueif you add one)
Optional follow-up: store the built index in state (searchIndex) instead of refs so the memo depends on real reactive data rather than isIndexLoading as a proxy for “refs are populated.” Could also extract a shared useAdvancedNodeSearchResults hook used by both dialog and sidebar.
Switch advanced search sidebar behavior to create a single summary block with wikilink children, and wire Option+Enter/footer action to open that block in the right sidebar. This aligns the flow with Roam's native sidebar result rendering while keeping the search dialog focused on result-list interaction. Co-authored-by: Cursor <cursoragent@cursor.com>
trangdoan982
force-pushed
the
eng-1738-platform-roam
branch
from
0ca5ed6 to
d241625
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (!debouncedSearchTerm) return; | ||
|
|
||
| onPersistState({ | ||
| query: debouncedSearchTerm, | ||
| results, | ||
| selectedNodeTypeIds, | ||
| sort, | ||
| windowId, | ||
| dgSearchId, | ||
| }); | ||
| }, [ | ||
| debouncedSearchTerm, | ||
| dgSearchId, | ||
| onPersistState, | ||
| results, | ||
| selectedNodeTypeIds, | ||
| sort, | ||
| windowId, | ||
| ]); |
There was a problem hiding this comment.
State persistence bug: When debouncedSearchTerm becomes empty (user clears search), the useEffect guard if (!debouncedSearchTerm) return; prevents state from being persisted. This leaves stale query/results in localStorage. If the sidebar window is closed and reopened, it will display outdated search results.
useEffect(() => {
// Remove the guard or persist empty state explicitly
onPersistState({
query: debouncedSearchTerm,
results,
selectedNodeTypeIds,
sort,
windowId,
dgSearchId,
});
}, [
debouncedSearchTerm,
dgSearchId,
onPersistState,
results,
selectedNodeTypeIds,
sort,
windowId,
]);| useEffect(() => { | |
| if (!debouncedSearchTerm) return; | |
| onPersistState({ | |
| query: debouncedSearchTerm, | |
| results, | |
| selectedNodeTypeIds, | |
| sort, | |
| windowId, | |
| dgSearchId, | |
| }); | |
| }, [ | |
| debouncedSearchTerm, | |
| dgSearchId, | |
| onPersistState, | |
| results, | |
| selectedNodeTypeIds, | |
| sort, | |
| windowId, | |
| ]); | |
| useEffect(() => { | |
| onPersistState({ | |
| query: debouncedSearchTerm, | |
| results, | |
| selectedNodeTypeIds, | |
| sort, | |
| windowId, | |
| dgSearchId, | |
| }); | |
| }, [ | |
| debouncedSearchTerm, | |
| dgSearchId, | |
| onPersistState, | |
| results, | |
| selectedNodeTypeIds, | |
| sort, | |
| windowId, | |
| ]); | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
2 participants
https://www.loom.com/share/08c356bb7cd841868b34bcea39cbab3b
Summary
Option + Entershortcut to open a dedicated sidebar results block instead of opening individual pagesAdvanced search results: \"{query}\") with child wikilink rows for each currently displayed resultTest plan
DG: Open Node Search, search for a query, pressOption + Enter, and confirm a single right-sidebar block opensOpen search sidebarin footer triggers the same behavior asOption + EnterMade with Cursor