[ENG-1052] Enable node tags via command key #554
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR adds a hotkey-triggered node tag suggestion feature to the Obsidian plugin. It introduces a configurable hotkey setting in the UI, extends the Settings schema, and registers a CodeMirror editor hotkey handler that opens a new NodeTagSuggestPopover component to suggest and insert node tags. Changes
Sequence DiagramsequenceDiagram
actor User
participant MarkdownView as MarkdownView<br/>(Editor)
participant CodeMirror as CodeMirror<br/>keydown handler
participant Plugin as DiscourseGraphPlugin
participant Popover as NodeTagSuggestPopover
participant DOM
User->>MarkdownView: Presses configured hotkey
MarkdownView->>CodeMirror: Triggers keydown event
CodeMirror->>Plugin: setupNodeTagHotkey handler
Plugin->>Popover: open(editor, nodeTypes)
Popover->>Popover: Filter nodeTypes<br/>and create items
Popover->>Popover: Compute cursor position
Popover->>DOM: Create & append popover element
DOM-->>User: Display tag suggestions
User->>Popover: Navigate & select tag
alt Keyboard/Mouse selection
Popover->>MarkdownView: Insert selected tag at cursor
MarkdownView-->>User: Tag inserted
end
Popover->>Popover: close()
Popover->>DOM: Remove popover & event listeners
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
🧹 Nitpick comments (6)
apps/obsidian/src/components/GeneralSettings.tsx (1)
157-159: Hotkey state and save behavior are sound; confirm semantics for empty valueThe single-character guard in
handleNodeTagHotkeyChangeplus assigningplugin.settings.nodeTagHotkey = nodeTagHotkey || "\\"on save is internally consistent and keeps the hotkey always defined. One implication is that clearing the field with Backspace will be saved as the default"\\", not “no hotkey”, so there’s currently no way to disable the shortcut via this UI. If that’s intentional (“empty means use default”), you’re good; if you ever want a “no hotkey” option, you’d need an explicit toggle or a distinct sentinel value.Also applies to: 185-191, 198-198
apps/obsidian/src/index.ts (2)
1-1: File-wide eslint disable is broader than necessaryDisabling
@typescript-eslint/no-unsafe-assignmentfor the whole file can hide unrelated unsafe assignments over time. If feasible, consider constraining this to the specific lines that actually need it (e.g., aroundloadData()), so the rest of the file stays fully checked.
192-225: Hotkey extension setup looks solid; consider an IME-friendly guardRegistering a
keydownhandler viaEditorView.domEventHandlersthat readsthis.settings.nodeTagHotkeyon each event is a good way to keep the shortcut dynamic after settings changes. ThepreventDefault/stopPropagation+ returningtruecorrectly claims the event before openingNodeTagSuggestPopoverfor the activeMarkdownView. As a small robustness tweak, you might optionally early‑return withif (event.isComposing) return false;to avoid interfering with IME composition for non‑Latin input.apps/obsidian/src/components/NodeTagSuggestModal.tsx (3)
173-181: Consider using event delegation for item interactions.Individual event listeners are attached to each item element. While these are cleaned up when the popover is removed from the DOM, using event delegation on the container would be more efficient and follows best practices.
Consider attaching a single event listener to the container and using event delegation:
itemsContainer.addEventListener("mousedown", (e) => { const target = (e.target as HTMLElement).closest(".node-tag-item"); if (target) { e.preventDefault(); e.stopPropagation(); const index = parseInt(target.dataset.index || "0", 10); const item = this.items[index]; if (item) this.selectItem(item); } });Similarly for
mouseenterevents.
261-269: Remove redundant closest check.The check on line 265 is redundant with the
containscheck on line 264. Ifthis.popover.contains(e.target)is false, the target is already outside the popover.Apply this diff to simplify:
this.clickOutsideHandler = (e: MouseEvent) => { - if ( - this.popover && - !this.popover.contains(e.target as Node) && - !(e.target as HTMLElement).closest(".node-tag-suggest-popover") - ) { + if (this.popover && !this.popover.contains(e.target as Node)) { this.close(); } };
76-99: Consider adding accessibility attributes.The popover lacks ARIA attributes for screen reader support. While keyboard navigation is implemented, adding semantic roles and states would improve accessibility.
Consider adding:
role="listbox"on the popover containerrole="option"on each item elementaria-selected="true"on the selected itemaria-activedescendanton the popover pointing to the selected item's IDExample for the popover:
popover.setAttribute("role", "listbox"); popover.setAttribute("aria-label", "Node tag suggestions");Also applies to: 117-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/obsidian/package.json(2 hunks)apps/obsidian/src/components/GeneralSettings.tsx(3 hunks)apps/obsidian/src/components/NodeTagSuggestModal.tsx(1 hunks)apps/obsidian/src/constants.ts(1 hunks)apps/obsidian/src/index.ts(4 hunks)apps/obsidian/src/types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/obsidian/src/index.tsapps/obsidian/src/components/GeneralSettings.tsx
🔇 Additional comments (5)
apps/obsidian/package.json (1)
19-21: New dependencies for editor hotkey support look fineThe added entries align with the new CodeMirror-based hotkey handling and date utilities used by the node-tag popover; nothing concerning here. Please just ensure the workspace lockfile is updated and builds/tests run cleanly with these versions.
Also applies to: 41-41
apps/obsidian/src/types.ts (1)
28-37: Settings wiring fornodeTagHotkeylooks consistentAdding
nodeTagHotkey: stringcleanly aligns the type withDEFAULT_SETTINGSandGeneralSettings, so settings objects will always carry a defined hotkey.apps/obsidian/src/constants.ts (1)
47-72: DefaultnodeTagHotkeyvalue is coherent with settings loading and UIUsing
"\\"as the default matches the GeneralSettings placeholder/description and ensures older configs pick up the new field viaDEFAULT_SETTINGS+hasNewFields.apps/obsidian/src/components/GeneralSettings.tsx (1)
277-305: Key capture logic correctly enforces single-character hotkeysThe
onKeyDownhandler cleanly restricts the setting to printable single characters (plus Backspace to clear) and prevents default insertion when a character hotkey is pressed, which keeps the input and state in sync. Just be aware that modifier combos (e.g. Cmd/Ctrl/Alt + key) will still record the underlying character, not the chord, which seems fine given the “single key” design.apps/obsidian/src/components/NodeTagSuggestModal.tsx (1)
9-336: Well-structured implementation with clean lifecycle management.The class demonstrates good separation of concerns with clear method responsibilities, proper event handler lifecycle management (setup/teardown), and appropriate viewport-aware positioning logic. The public API is simple and fits well with the hotkey integration pattern.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Approved to not block, but I strongly recommend we understand and/or fix this issue:
// eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
| @@ -0,0 +1,335 @@ | |||
| import { Editor } from "obsidian"; | |||
There was a problem hiding this comment.
Looks like we are mostly creating this from scratch? Are there any existing obsidian components we can tie into without having to re-invent the wheel, cover all edge cases, etc? List items, popovers, etc, etc.
There was a problem hiding this comment.
as far as i know there's no official API to create popovers at tooltip position. The popover API they have are the one that takes over the whole screen
apps/obsidian/package.json
Outdated
| "author": "", | ||
| "license": "Apache-2.0", | ||
| "devDependencies": { | ||
| "@codemirror/state": "^6.5.2", |
There was a problem hiding this comment.
Where is this being used?
trangdoan982
force-pushed
the
eng-1052-enable-node-tags-via-hot-key
branch
from
337a28c to
618f4fa
Compare
https://www.loom.com/share/0c61409741e94d818c4ad3c87aa6f1e8
Summary by CodeRabbit
Release Notes