Skip to content

ENG-1730: Add node type filter dropdown to advanced node search#1054

Open
trangdoan982 wants to merge 8 commits into
mainfrom
eng-1730-add-filter-dropdown-menu
Open

ENG-1730: Add node type filter dropdown to advanced node search#1054
trangdoan982 wants to merge 8 commits into
mainfrom
eng-1730-add-filter-dropdown-menu

Conversation

@trangdoan982
Copy link
Copy Markdown
Member

@trangdoan982 trangdoan982 commented May 18, 2026 •
edited
Loading

https://www.loom.com/share/0f58be24091140faac67a1b74bc3ee06

Summary

  • Add a Filter by type control to the Advanced Node Search dialog toolbar with a Blueprint popover (checkboxes, color dots, Select all, per-type Only, and in-dropdown type search when there are more than 7 types).
  • Wire filter state into MiniSearch via typeFilter so keyword results respect the selected discourse node types; empty selection means all types (no filter).
  • Extract shared helpers (discourseNodeTypeFilter) and useCloseOnClickOutside for reliable close behavior inside the dialog.

Test plan

  • Open Advanced Node Search and confirm the filter button appears in the toolbar.
  • With no types selected (all types), search returns results across types; partial selection limits results.
  • Verify active filter styling (purple trigger + count pill) when a subset of types is selected.
  • Exercise Select all, individual toggles, and Only on a row.
  • With 8+ node types, confirm Filter types… search narrows the list.
  • Click outside the popover and press Escape to close; reopen and confirm state resets when the dialog closes.

Made with Cursor

@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 18, 2026

ENG-1730

@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app Bot commented May 18, 2026

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@supabase
Copy link
Copy Markdown

supabase Bot commented May 18, 2026

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

graphite-app[bot]
graphite-app Bot reviewed May 18, 2026
View reviewed changes
Comment thread apps/roam/src/components/DiscourseNodeTypeFilter.tsx Outdated
@trangdoan982 trangdoan982 force-pushed the eng-1730-add-filter-dropdown-menu branch from a029682 to dbbd6d2 Compare May 21, 2026 04:22
@trangdoan982
Copy link
Copy Markdown
Member Author

trangdoan982 commented May 21, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

chatgpt-codex-connector Bot commented May 21, 2026

To use Codex here, create a Codex account and connect to github.

@trangdoan982 trangdoan982 force-pushed the eng-1729-create-initial-search-with-preview branch from 88d19e0 to ae0cea7 Compare May 22, 2026 20:08
Base automatically changed from eng-1729-create-initial-search-with-preview to main May 22, 2026 20:21
@trangdoan982 trangdoan982 force-pushed the eng-1730-add-filter-dropdown-menu branch from 45ac2a1 to 67a2895 Compare May 22, 2026 20:27
trangdoan982 and others added 7 commits May 22, 2026 19:00
@trangdoan982
basic filter
a49e3a7
@trangdoan982 @cursoragent
Address PR review feedback for node type filter.
5e02f5a 
Fix indeterminate checkbox sizing, use Blueprint controls, improve Escape handling when the filter popover is open, disable the trigger while types load, colocate click-outside handling, and add unit tests for filter state helpers.

Co-authored-by: Cursor <cursoragent@cursor.com>
@trangdoan982 @cursoragent
Fix eslint no-floating-promises in filter unit tests.
44e9fa0 
Co-authored-by: Cursor <cursoragent@cursor.com>
@trangdoan982
remove test
7e4ff3f
@trangdoan982
cleanup styling
d4fa67e
@trangdoan982
final clean
3d46b2f
@trangdoan982
lint fix
deb8ae8
@trangdoan982 trangdoan982 force-pushed the eng-1730-add-filter-dropdown-menu branch from 67a2895 to deb8ae8 Compare May 22, 2026 23:13
graphite-app[bot]
graphite-app Bot reviewed May 22, 2026
View reviewed changes
Comment thread apps/roam/src/components/AdvancedNodeSearchDialog/DiscourseNodeTypeFilter.tsx
@trangdoan982 trangdoan982 requested a review from mdroidian May 23, 2026 17:48
mdroidian
mdroidian approved these changes May 24, 2026
View reviewed changes
Comment thread apps/roam/src/components/AdvancedNodeSearchDialog/DiscourseNodeTypeFilter.tsx
onSelectOnly: () => void;
onToggle: () => void;
}): React.ReactElement => (
<div className="group flex items-center gap-2 px-3 py-1.5 hover:bg-gray-900/[0.04]">
Copy link
Copy Markdown
Member

@mdroidian mdroidian May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd double check hover:bg-gray-900/[0.04], I'm not sure these type of dynamic classes are supported in Roam

Comment thread apps/roam/src/components/AdvancedNodeSearchDialog/DiscourseNodeTypeFilter.tsx
onChange={onToggle}
/>
<Button
className="!px-2 !py-1 opacity-0 transition-opacity group-focus-within:opacity-100 group-hover:opacity-100"
Copy link
Copy Markdown
Member

@mdroidian mdroidian May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe ! is supported in Roam

Comment thread apps/roam/src/components/AdvancedNodeSearchDialog/DiscourseNodeTypeFilter.tsx
[onSelectedIdsChange, selectedIdSet, selectedIds],
);

const handleOnly = useCallback(
Copy link
Copy Markdown
Member

@mdroidian mdroidian May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need useCallback? Actually why does this need a separate function at all?

Comment thread apps/roam/src/components/AdvancedNodeSearchDialog/DiscourseNodeTypeFilter.tsx
}
>
{activeFilterCount > 0 && (
<span
Copy link
Copy Markdown
Member

@mdroidian mdroidian May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use something like the filter-keep icon so that the icon itself doesn't move and cause jank when filter is on/off

Comment thread apps/roam/src/utils/discourseNodeTypeFilter.ts
@@ -0,0 +1,61 @@
import { type DiscourseNode } from "~/utils/getDiscourseNodes";

/** Advanced search: empty `selectedTypeIds` means all types; partial list is an active filter. */
Copy link
Copy Markdown
Member

@mdroidian mdroidian May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit ambiguous because “empty” can read like an action rather than a state.

Maybe something like

/* Advanced search: when selectedTypeIds has no values, show all node types; otherwise, filter to the selected types. */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@mdroidian mdroidian mdroidian approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@trangdoan982 @mdroidian