[ENG-968] Obsidian - Open node functionalities #524
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR refactors file-opening behavior in the Obsidian canvas system. It extracts file-opening logic into utility functions, implements keyboard-modifier-based routing (Shift+Click for sidebar, Cmd/Ctrl+Click for new tab), and adds a hover-triggered UI button on canvas nodes to open linked files in the sidebar. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TldrawView as TldrawViewComponent
participant Utils as openFileUtils
participant Obsidian as Obsidian App
User->>TldrawView: Click with Shift key
activate TldrawView
TldrawView->>TldrawView: Debounce & compute openInNewTab = false
TldrawView->>Utils: openFileInSidebar(app, file)
activate Utils
Utils->>Obsidian: Get right leaf / vertical split
Utils->>Obsidian: Open file in leaf
Utils->>Obsidian: Set leaf active
Utils-->>TldrawView: Promise resolved
deactivate Utils
deactivate TldrawView
User->>TldrawView: Click with Cmd/Ctrl key
activate TldrawView
TldrawView->>TldrawView: Debounce & compute openInNewTab = true
TldrawView->>Utils: openFileInNewTab(app, file)
activate Utils
Utils->>Obsidian: Get tab leaf
Utils->>Obsidian: Open file in leaf
Utils->>Obsidian: Set leaf active
Utils-->>TldrawView: Promise resolved
deactivate Utils
deactivate TldrawView
sequenceDiagram
actor User
participant NodeShape as DiscourseNodeShape
participant Utils as openFileUtils
participant Toast as Toast Feedback
User->>NodeShape: Hover over shape
activate NodeShape
NodeShape->>NodeShape: Set isHovered = true
NodeShape->>NodeShape: Render "Open in sidebar" button
deactivate NodeShape
User->>NodeShape: Click "Open in sidebar" button
activate NodeShape
NodeShape->>NodeShape: Call handleOpenInSidebar()
alt src or cache exists
NodeShape->>Utils: openFileInSidebar(app, file)
activate Utils
Utils-->>NodeShape: File opened
deactivate Utils
NodeShape->>Toast: Show success message
else error
NodeShape->>Toast: Show error message
end
deactivate NodeShape
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 (3)
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (3)
129-135: Consider a more descriptive debug label.The first parameter of
useValueis a debug label. While "is hovered" is clear, it could be more specific to this shape, such asdiscourse-node-${shape.id}-hoveredfor easier debugging when multiple shapes are tracked.
240-278: Consider using Tailwind utilities for all styling.The button mixes inline styles with Tailwind classes, which reduces consistency and makes the styles harder to maintain. Consider moving inline styles to Tailwind utilities or a CSS module.
Example refactor using Tailwind utilities:
- className="absolute left-1 top-1 z-10 flex items-center justify-center rounded bg-white/90 p-1 shadow-sm transition-all duration-200 hover:bg-white" - style={{ - width: "24px", - height: "24px", - border: "1px solid rgba(0, 0, 0, 0.1)", - pointerEvents: "auto", - cursor: "pointer", - }} + className="absolute left-1 top-1 z-10 flex h-6 w-6 cursor-pointer items-center justify-center rounded border border-black/10 bg-white/90 p-1 shadow-sm transition-all duration-200 hover:bg-white" + style={{ pointerEvents: "auto" }} title="Open in sidebar"Note: Keep
pointerEvents: "auto"in inline styles if it's needed to override tldraw's canvas event handling.
263-276: Consider extracting the SVG icon to a constant.The inline SVG could be extracted to the
~/iconsmodule (similar toWHITE_LOGO_SVG) for better reusability and maintainability.Example:
In
~/icons.tsor similar:export const SIDEBAR_ICON_SVG = `<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><rect x="3" y="3" width="18" height="18" rx="2" ry="2"/><line x1="15" y1="3" x2="15" y2="21"/></svg>`;Then use
dangerouslySetInnerHTMLor a proper SVG component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx(3 hunks)apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx(4 hunks)apps/obsidian/src/components/canvas/utils/openFileUtils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Prefertypeoverinterface
Use explicit return types for functions
Avoidanytypes when possible
Files:
apps/obsidian/src/components/canvas/utils/openFileUtils.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsxapps/obsidian/src/components/canvas/TldrawViewComponent.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants
Files:
apps/obsidian/src/components/canvas/utils/openFileUtils.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsxapps/obsidian/src/components/canvas/TldrawViewComponent.tsx
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
Use the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for all code in the Obsidian plugin
Files:
apps/obsidian/src/components/canvas/utils/openFileUtils.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsxapps/obsidian/src/components/canvas/TldrawViewComponent.tsx
🔇 Additional comments (3)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (1)
203-281: LGTM! Clean refactoring of file-opening behavior.The changes successfully extract file-opening logic into utility functions and add support for modifier keys:
- Shift+Click opens in sidebar
- Cmd/Ctrl+Click (metaKey) opens in new tab
The debounce logic, error handling, and user feedback are all preserved. When both modifiers are pressed, metaKey takes precedence, which is reasonable behavior.
apps/obsidian/src/components/canvas/utils/openFileUtils.ts (2)
3-18: Verify the fallback behavior for empty right sidebar.The current logic uses
getRightLeaf(false), which returnsnullif the right sidebar has no leaves. In that case, it falls back to creating a vertical split (line 14), which may not be in the right sidebar.Consider whether the intended behavior is:
- Always open in the right sidebar (even if empty) → use
getRightLeaf(true)or checkrightSplitexistence- Only use right sidebar if it already has leaves → current implementation is correct
If the goal is to always use the right sidebar when available:
export const openFileInSidebar = async (app: App, file: TFile): Promise<void> => { const rightSplit = app.workspace.rightSplit; - const rightLeaf = app.workspace.getRightLeaf(false); + const rightLeaf = app.workspace.getRightLeaf(true); if (rightLeaf) { if (rightSplit && rightSplit.collapsed) { rightSplit.expand(); } await rightLeaf.openFile(file); app.workspace.setActiveLeaf(rightLeaf); - } else { - const leaf = app.workspace.getLeaf("split", "vertical"); - await leaf.openFile(file); - app.workspace.setActiveLeaf(leaf); } };Alternatively, verify the current behavior matches the expected UX through testing.
20-24: LGTM! Clean implementation.The function correctly opens a file in a new tab and activates it. The explicit return type and arrow function follow the coding guidelines.
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
Outdated
Show resolved
Hide resolved
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I couldn't get Ctrl click to open in a new tab, I think it might have to do with the onClick being called to open in sidebar.
| <button | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| void handleOpenInSidebar(); |
There was a problem hiding this comment.
It looks like we might be calling handleOpenInSidebar() in two places? Here an onMount pointer_up?
There was a problem hiding this comment.
you have to do Cmd + Click to open in a new tab to open file in a new tab
There was a problem hiding this comment.
- cmd+click works for me it opens up a new tab
- Button click opens up in sidebar
From my investigation it is the case that both places are handling different cases, in onMount we are handling shift and cmd click on the node shape and here we are handling the button click
trangdoan982
force-pushed
the
eng-968-obsidian-open-node-button-in-sidebar-for-canvas-nodes
branch
from
1afe53b to
3f65cb2
Compare
|
@sid597 can you help me review this PR since Michael is away this week? |
|
@trangdoan982 Yeah I will review it, first need to figure out how to setup and test dg plugin in obsidian |
trangdoan982
deleted the
eng-968-obsidian-open-node-button-in-sidebar-for-canvas-nodes
branch
https://www.loom.com/share/059b956c0ef84c59aa5668d6ae7dfb76
Summary by CodeRabbit