Skip to content

[ENG-2181] Advanced node search filters menu UX update - #1354

Open
jsmorabito wants to merge 1 commit into
mainfrom
jsmorabito/eng-2181-advanced-node-search-filters-menu-ux-update
Open

[ENG-2181] Advanced node search filters menu UX update#1354
jsmorabito wants to merge 1 commit into
mainfrom
jsmorabito/eng-2181-advanced-node-search-filters-menu-ux-update

Conversation

@jsmorabito

@jsmorabito jsmorabito commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Node type filter checkboxes in the Obsidian node search now start unchecked instead of all-checked-by-default; checking one filters results to it directly.
  • Removed the per-row Only button (and its hover-reveal styling) — checking a single box now does the same thing.
  • Simplified discourseNodeTypeFilter.ts: hasActiveTypeFilter is now a plain selectedTypeIds.length > 0, and toPanelSelectedIds / fromPanelSelectedIds are deleted — they existed only to translate an empty selection into an "all checked" display, which no longer applies.

Linear: https://linear.app/discourse-graphs/issue/ENG-2181/advanced-node-search-filters-menu-ux-update

Video

https://www.loom.com/share/8d0c7af989684c49b5c44d7003b66efe

Test plan

  • pnpm run check-types — no new errors (pre-existing unrelated tldraw/canvas type errors only)
  • pnpm exec eslint on both changed files — clean
  • Manually tested in dev vault via Open node search → filter by type menu
  • Before/after screenshots attached
  • Loom walkthrough linked

Self-review

Checked against STYLE_GUIDE.md:

  • Explicit return types, named-object params where >2 args, no any — all hold.
  • No stray comments left from the removed "Only" button; top-of-file doc comment updated to describe the new checkbox semantics.
  • One gap: discourseNodeTypeFilter.ts has no unit tests (none existed before this change either). Flagging rather than adding tests unasked, since that'd be scope beyond this ticket — happy to add a follow-up if wanted.

Scope check

  • Ran $scope-check against ENG-2181 and the final diff.
  • Scope beyond Done When: Not documented — ENG-2181 has no written Done When section, only a linked Loom video (from DES-286) I don't have the ability to watch. The diff matches the plain-language request this PR was built from: default-unchecked filter checkboxes, direct per-type filtering on check, and removal of the "Only" button. No other behavior was changed.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Improvements
    • Simplified node-type filtering: selecting any node types now activates the filter, while clearing all selections removes it.
    • Filter counts and activity indicators now directly reflect the selected node types.
    • Removed the “Only” action from individual node-type filter options.

Nodes types now start unchecked; checking one filters to it directly,
so the panel no longer needs to translate an empty selection into an
all-checked display or offer a separate Only shortcut.
@linear-code

linear-code Bot commented Aug 26, 2026

Copy link
Copy Markdown

ENG-2181

@vercel

vercel Bot commented Aug 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
discourse-graph Skipped Skipped Aug 26, 2026 2:11pm

Request Review

@supabase

supabase Bot commented Aug 26, 2026

Copy link
Copy Markdown

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 ↗︎.

@jsmorabito

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a2268997-835c-4162-8e7f-ba9fd4dc055d

📥 Commits

Reviewing files that changed from the base of the PR and between f007436 and e6497f3.

📒 Files selected for processing (2)
  • apps/obsidian/src/components/NodeTypeFilterMenu.tsx
  • apps/obsidian/src/utils/discourseNodeTypeFilter.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The node-type filter now uses selected node-type IDs directly. Any non-empty selection is active. The menu no longer provides an “Only” action or converts IDs between menu and panel representations.

Changes

Node-type filter behavior

Layer / File(s) Summary
Filter activity semantics
apps/obsidian/src/utils/discourseNodeTypeFilter.ts
hasActiveTypeFilter now treats every non-empty selection as active. The panel-selection conversion helpers were removed.
Direct menu selection handling
apps/obsidian/src/components/NodeTypeFilterMenu.tsx
Node-type rows now provide only toggle controls. The menu passes selected IDs directly to the panel and uses their length for the active count.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to e6497

This localized update changes node-type filters to start unchecked and removes the redundant Only action. No actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: the advanced node search filter menu UX update.
Description check ✅ Passed The description includes the required Scope check section and documents the scope status, summary, testing, and known test gap. The scope-check command remains unchecked, but the section is otherwise …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes the required Scope check section and documents the scope status, summary, testing, and known test gap. The scope-check command remains unchecked, but the section is otherwise complete and relevant.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@trangdoan982

trangdoan982 commented Aug 26, 2026

Copy link
Copy Markdown
Member

I'm not sure if this is the desired design. When we made this proposal for the Node Summoning Menu in Roam, I remember being asked specifically for the ability to make it easy to filter "all but one node", "all", or "only one." Not sure if there's any user context that's changed since then? But if it has, and we agreed on this, the code looks fine to me.

@jsmorabito

Copy link
Copy Markdown
Collaborator Author

I'm not sure if this is the desired design. When we made this proposal for the Node Summoning Menu in Roam, I remember being asked specifically for the ability to make it easy to filter "all but one node", "all", or "only one." Not sure if there's any user context that's changed since then? But if it has, and we agreed on this, the code looks fine to me.

I haven't done a design pass on the node summoning menu's filter menu so I wont comment on its implementation, but in the context of this particular filter menu for advanced node search I agree with Michaels sentiment here https://linear.app/discourse-graphs/issue/ENG-2181/advanced-node-search-filters-menu-ux-update that it's more straightforward to reverse the behavior of filters.

I don't think I misinterpreted @mdroidian from that video, but tagging so he can double check my loom 😁

Copy link
Copy Markdown
Member

i'll defer to MG to review then

@mdroidian

Copy link
Copy Markdown
Member

Let's continue this discussion in Linear.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants