[ENG-404] Fix multiple things about node search menu #347
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughReplaces the checkbox filter UI with a Menu-based filter panel in DiscourseNodeSearchMenu, adds per-type "Only" actions and bulk-select logic, changes Popover behavior and menu sizing, enables multiline result items and keyboard/active states, and tweaks focus propagation. Also changes nanoid import style in generateUid. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as DiscourseNodeSearchMenu
participant P as Popover/Menu
participant F as Type Filter MenuItem
participant E as Editor/Textarea
U->>C: Open search menu
C->>P: Render menu (usePortal, viewport bounds, width=400)
U->>F: Click type item / "Only" / "Select all"
F-->>C: handleTypeCheckChange / handleSelectOnly / handleToggleAll
C->>C: Update selected typeIds / isAllSelected
C->>P: Keep popover open (shouldDismissPopover=false)
C->>E: refocusTextarea (preserve editor focus)
U->>P: Select result item
P-->>C: onSelect -> C calls waitForBlock (promise ignored with void)
C->>E: refocusTextarea (if needed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 (3)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (3)
364-372: Harden refocus logic against unmounted/stale textarea and reuse it elsewhereGood centralization. Add a containment check to guard against cases where the textarea has unmounted before the timeout fires.
Apply this diff within the helper:
- setTimeout(() => { - if (!textarea) return; - textarea.focus(); - const cursorPos = textarea.selectionStart; - textarea.setSelectionRange(cursorPos, cursorPos); - }, 0); + setTimeout(() => { + const el = textarea; + if (!el || !document.body.contains(el)) return; + const cursorPos = el.selectionStart; + el.focus(); + el.setSelectionRange(cursorPos, cursorPos); + }, 0);Also consider reusing this helper in toggleFilterMenu instead of duplicating the focus+selection snippet there to keep behavior consistent.
445-445: Avoid duplicated inline width; factor a single constantThe inline width appears twice. Prefer a shared constant or CSS class to keep styles DRY.
- style={{ width: "400px" }} + style={{ width: MENU_WIDTH }}Outside this range, add near the top of the file:
const MENU_WIDTH = 400;
455-455: Same width duplication here—reuse the constantMirror the change here to use the same MENU_WIDTH constant.
- style={{ width: "400px" }} + style={{ width: MENU_WIDTH }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/DiscourseNodeSearchMenu.tsx
🧬 Code Graph Analysis (1)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (2)
apps/obsidian/src/types.ts (1)
DiscourseNode(3-11)apps/roam/src/utils/types.ts (1)
Result(42-46)
🔇 Additional comments (6)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (6)
16-17: Switch import is appropriate for Blueprint v3Importing Switch from @blueprintjs/core aligns with the new “Select All” control usage.
62-62: Opening the filter panel by default is a sensible UX choiceDefaulting to open improves discoverability of the new filtering controls.
69-76: Memoized typeIds/isAllSelected logic looks correctThe derivation of typeIds and the all-selected check are correct and efficient for the current state shape.
411-417: Bulk toggle implementation is correctSetting all checkedTypes based on current typeIds is straightforward and correct. Refocusing is consistent with the focus policy.
427-427: enforceFocus={false} may have a11y implications—please verify interaction modelDisabling focus enforcement is consistent with keeping the editor cursor stable, but it can degrade keyboard/screen-reader flows. Please manually verify:
- Tabbing does not unexpectedly trap or lose focus.
- Escape still closes the popover reliably when the menu is open.
- Clicking outside behaves as intended and doesn’t cause cursor jumps.
If issues arise, consider keeping enforceFocus but selectively suppress focus on specific controls via onMouseDown preventDefault, while allowing keyboard focus.
540-540: Enabling multiline MenuItem text is a solid UX improvementThis prevents truncation for longer labels and matches the widened menu.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I couldn't get it to work.
https://www.loom.com/share/082393dbf75242eaadc0a4eadae9d921
Also, in the ticket it was mentioned "Changing the pattern to be "Opt in" vs "Opt out" where default is "all" if none are selected." I believe we should still do this. Currently it takes multiple clicks just to filter a single node type, which is likely the most often action in this instance.
Personally I find the checkboxes quite busy. I still would prefer multi-select, but I'm happy to defer to @jsmorabito It is really quite busy now. If we keep the checkboxes, maybe they should go down the left hand side or something.
|
Any questions regarding this one? What is left before re-review? |
|
no this is still wip. I will ping you when it's ready for re-review |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (7)
59-59: Make menu width responsive instead of fixed 400px.A hard width can overflow on smaller windows. Prefer a max width with a viewport cap.
Apply:
- const MENU_WIDTH = 400; + const MENU_MAX_WIDTH = 400;And replace both inline styles:
- style={{ width: MENU_WIDTH }} + style={{ width: "min(92vw, 400px)" }}
208-208: Potential cursor race; capture selection earlier.
textarea.selectionStartis read after async waits. If the user types meanwhile, suffix slicing may shift. Capture the selection before awaiting.- void waitForBlock(blockUid, textarea.value).then(() => { + const selectionStartAtInvoke = textarea.selectionStart; + void waitForBlock(blockUid, textarea.value).then(() => { onClose(); setTimeout(() => { const originalText = getTextByBlockUid(blockUid); - const prefix = originalText.substring(0, triggerPosition); - const suffix = originalText.substring(textarea.selectionStart); + const prefix = originalText.substring(0, triggerPosition); + const suffix = originalText.substring(selectionStartAtInvoke);
373-378: Bulk toggle: LGTM with a micro‑nit.Implementation is correct. Optional: use
discourseTypesdirectly to avoid relying ontypeIdsdependency.- setCheckedTypes(Object.fromEntries(typeIds.map((id) => [id, checked]))); + setCheckedTypes(Object.fromEntries(discourseTypes.map((t) => [t.type, checked])));
451-451: Duplicate inline width styles.Avoid repeating the same style in two sibling containers.
- style={{ width: MENU_WIDTH }} + // outer container sets width onceRemove the inner duplicate or convert both to the responsive width suggestion above.
Also applies to: 460-460
467-473: Filter button: add aria-label and consider toggled state.Add
aria-pressedto reflect panel visibility and an explicit aria label.<Button icon="filter" minimal small title="Filter by type" + aria-label="Filter by type" + aria-pressed={isFilterMenuVisible}
475-494: Select all: keep focus stable and avoid double guards.You already guard with
onMouseDownon the Button—good. No need for bothstopPropagationandpreventDefaulton click;preventDefaulton mousedown is sufficient.- onClick={(e) => { - e.stopPropagation(); - e.preventDefault(); - handleToggleAll(!isAllSelected); - }} + onClick={() => handleToggleAll(!isAllSelected)}
517-517: Result items: consider preserving editor focus on mouse clicks too.Keyboard is handled; to keep the editor focused on mouse selection as well, mirror the
onMouseDownprevention used elsewhere.<MenuItem key={item.uid} text={item.text} multiline data-active={isActive} active={isActive} shouldDismissPopover={false} + onMouseDown={(e) => e.preventDefault()} onClick={() => onSelect(item)} />Also applies to: 521-521
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/obsidian/src/utils/generateUid.ts(1 hunks)apps/roam/src/components/DiscourseNodeSearchMenu.tsx(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/DiscourseNodeSearchMenu.tsx
🔇 Additional comments (7)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (6)
15-16: Intent import: OK.Usage aligns with BlueprintJS; no issues.
68-76: Derived selection state: LGTM.
typeIdsandisAllSelectedare correct and handle empty states.
366-372: Single-type toggle: LGTM.Functional update prevents stale state; no issues.
380-388: Select “Only”: LGTM.Clear and efficient.
495-499: Scrollable type list: LGTM.Menu within a capped container avoids tall popovers; good choice.
431-437: Popover portal/overflow — manual UI sanity‑check requiredusePortal={true} with preventOverflow: { boundariesElement: "viewport" } is present in apps/roam/src/components/DiscourseNodeSearchMenu.tsx (≈lines 420–460); repo search found no other Popover configs.
- Manually verify in small windows and near top/bottom edges for clipping or jumping.
- Manually verify inside the right sidebar and in references panes to ensure the portal positioning behaves correctly.
apps/obsidian/src/utils/generateUid.ts (1)
1-1: Don't change the import to a named export yet — repo currently resolves nanoid@2.xapps/roam/package.json pins "nanoid": "2.0.4" and package-lock.json shows node_modules/nanoid@2.0.4 at top level; switching to import { nanoid } from "nanoid" will break consumers that resolve v2. Files importing nanoid in this repo (update all if you upgrade):
- apps/obsidian/src/utils/generateUid.ts
- apps/roam/src/utils/fireQuery.ts
- apps/roam/src/components/Export.tsx
- apps/roam/src/components/canvas/useRoamStore.ts
- apps/roam/src/components/DiscourseContextOverlay.tsx
- apps/roam/src/components/DiscourseContext.tsx
- apps/roam/src/components/ExportGithub.tsx
- apps/roam/scripts/compile.ts
Options:
- Upgrade/pin nanoid to an ESM version (v3+/v4) across the monorepo (update package.json(s) and reinstall), then replace imports with: import { nanoid } from "nanoid"; across the files above.
- Or keep the existing default imports until you standardize/upgrade nanoid.
Likely an incorrect or invalid review comment.
| const renderTypeItem = useCallback( | ||
| (item: DiscourseNode) => { | ||
| const isSelected = !!checkedTypes[item.type]; | ||
| return ( | ||
| <MenuItem | ||
| key={item.type} | ||
| className="group" | ||
| text={item.text} | ||
| icon={isSelected ? "tick" : "blank"} | ||
| shouldDismissPopover={false} | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| handleTypeCheckChange(item.type); | ||
| }} | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| labelElement={ | ||
| <Button | ||
| minimal | ||
| small | ||
| className="opacity-0 transition-opacity group-hover:opacity-100" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| handleSelectOnly(item); | ||
| }} | ||
| > | ||
| Only | ||
| </Button> | ||
| } | ||
| /> | ||
| ); | ||
| }, | ||
| [textarea], | ||
| [checkedTypes, handleTypeCheckChange, handleSelectOnly], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restore editor focus for “Only” button; improve a11y.
Clicking “Only” can steal focus from the editor. Also add an accessible label.
labelElement={
<Button
minimal
small
className="opacity-0 transition-opacity group-hover:opacity-100"
+ aria-label={`Select only ${item.text}`}
+ onMouseDown={(e) => e.preventDefault()}
onClick={(e) => {
e.stopPropagation();
handleSelectOnly(item);
}}
>
Only
</Button>
}Also consider revealing the button on keyboard focus:
- className="opacity-0 transition-opacity group-hover:opacity-100"
+ className="opacity-0 transition-opacity group-hover:opacity-100 group-focus-within:opacity-100"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const renderTypeItem = useCallback( | |
| (item: DiscourseNode) => { | |
| const isSelected = !!checkedTypes[item.type]; | |
| return ( | |
| <MenuItem | |
| key={item.type} | |
| className="group" | |
| text={item.text} | |
| icon={isSelected ? "tick" : "blank"} | |
| shouldDismissPopover={false} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| handleTypeCheckChange(item.type); | |
| }} | |
| onMouseDown={(e) => e.preventDefault()} | |
| labelElement={ | |
| <Button | |
| minimal | |
| small | |
| className="opacity-0 transition-opacity group-hover:opacity-100" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| handleSelectOnly(item); | |
| }} | |
| > | |
| Only | |
| </Button> | |
| } | |
| /> | |
| ); | |
| }, | |
| [textarea], | |
| [checkedTypes, handleTypeCheckChange, handleSelectOnly], | |
| ); | |
| const renderTypeItem = useCallback( | |
| (item: DiscourseNode) => { | |
| const isSelected = !!checkedTypes[item.type]; | |
| return ( | |
| <MenuItem | |
| key={item.type} | |
| className="group" | |
| text={item.text} | |
| icon={isSelected ? "tick" : "blank"} | |
| shouldDismissPopover={false} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| handleTypeCheckChange(item.type); | |
| }} | |
| onMouseDown={(e) => e.preventDefault()} | |
| labelElement={ | |
| <Button | |
| minimal | |
| small | |
| className="opacity-0 transition-opacity group-hover:opacity-100 group-focus-within:opacity-100" | |
| aria-label={`Select only ${item.text}`} | |
| onMouseDown={(e) => e.preventDefault()} | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| handleSelectOnly(item); | |
| }} | |
| > | |
| Only | |
| </Button> | |
| } | |
| /> | |
| ); | |
| }, | |
| [checkedTypes, handleTypeCheckChange, handleSelectOnly], | |
| ); |
🤖 Prompt for AI Agents
In apps/roam/src/components/DiscourseNodeSearchMenu.tsx around lines 390 to 423,
the "Only" button currently steals editor focus and lacks an accessible label
and keyboard-visible styling; update the button so it does not steal focus by
preventing default on mouseDown (i.e., call e.preventDefault() on onMouseDown),
add a clear aria-label (e.g., `Only ${item.text}`) for screen readers, ensure
keyboard users can both see and operate it by making it visible on focus (add
focus-visible or focus styles to reveal the button) and handle keyboard
activation (handle Enter/Space in onKeyDown to call handleSelectOnly), and keep
stopping propagation in click handlers so the menu behavior is unchanged.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I don't think this is blocking, but to considered for future UX.
There is a few pixels on the top, bottom, and right of "Only" that click the menu item rather than "Only".
A css fix would be to move the padding vertical to the middle element and extend the "Only" button to the edges, then recenter "Only" and ✔ vertically.
trangdoan982
deleted the
eng-404-node-summoning-menu-multi-select-filter-ui
branch
* fix multiple things about node menu * address PR comments * multi-select as it is * fix the UI, still has position bug * fix finally * address PR comment
https://www.loom.com/share/b8cf4939d1f549f0a792b88a286b9212
Note:
Summary by CodeRabbit
New Features
Improvements
Chores