-
Notifications
You must be signed in to change notification settings - Fork 3
[ENG-1079] Cmd + Alt + Enter to open file to new leaf #572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRefactors the TldrawViewComponent file opening logic to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (2)
16-16: Unused import:TLKeyboardEventInfoThis type is imported but never used. The keyboard handling at lines 116-146 uses the native DOM
KeyboardEventtype, not tldraw'sTLKeyboardEventInfo.- TLKeyboardEventInfo,
148-148: Consider adding the listener to the container instead ofwindow.Attaching to
windowwith capture phase could intercept this shortcut globally, even when focus is elsewhere. Binding tocontainer(line 114) would scope the shortcut to the tldraw editor only.- window.addEventListener("keydown", handleKeyDown, true); + container.addEventListener("keydown", handleKeyDown, true); return () => { - window.removeEventListener("keydown", handleKeyDown, true); + container.removeEventListener("keydown", handleKeyDown, true); };apps/obsidian/src/components/canvas/utils/openFileUtils.ts (1)
15-20: Unused parameter:pluginThe
pluginparameter is declared but never used in the function body. Consider removing it to simplify the API.export const resolveDiscourseNodeFile = async ( shape: DiscourseNodeShape, canvasFile: TFile, app: App, - plugin: DiscourseGraphPlugin, ): Promise<TFile | null> => {Note: This would require updating the call sites in
TldrawViewComponent.tsx(lines 133-138 and 318-323).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx(7 hunks)apps/obsidian/src/components/canvas/utils/openFileUtils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/obsidian/src/components/canvas/TldrawViewComponent.tsxapps/obsidian/src/components/canvas/utils/openFileUtils.ts
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI
Files:
apps/obsidian/src/components/canvas/TldrawViewComponent.tsxapps/obsidian/src/components/canvas/utils/openFileUtils.ts
🧬 Code graph analysis (2)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (2)
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
DiscourseNodeShape(25-37)apps/obsidian/src/components/canvas/utils/openFileUtils.ts (4)
resolveDiscourseNodeFile(15-68)openFileInNewLeaf(99-106)openFileInNewTab(90-97)openFileInSidebar(70-88)
apps/obsidian/src/components/canvas/utils/openFileUtils.ts (3)
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
DiscourseNodeShape(25-37)apps/obsidian/src/components/canvas/stores/assetStore.ts (2)
extractBlockRefId(58-71)resolveLinkedTFileByBlockRef(77-112)apps/obsidian/src/components/canvas/utils/toastUtils.ts (1)
showToast(4-23)
🔇 Additional comments (5)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (3)
109-153: Cross-platform keyboard shortcut consideration.The shortcut uses
e.metaKeywhich maps to the Command key on macOS but the Windows key on Windows/Linux. Users on non-Mac platforms typically expectCtrl+Alt+Enterrather thanWin+Alt+Enter.Consider adding
ctrlKeysupport for Windows/Linux users:if ( e.key === "Enter" && - e.metaKey && + (e.metaKey || e.ctrlKey) && e.altKey && - !e.shiftKey && - !e.ctrlKey + !e.shiftKey ) {Alternatively, if Mac-only behavior is intentional, please verify this aligns with the product requirements and document it accordingly.
273-274: Good simplification of the event handler guard.The early return for non-pointer events cleans up the control flow.
317-334: Clean refactoring to use centralized utility functions.The async file resolution and opening logic is now properly delegated to the shared utilities, improving consistency and reducing duplication.
apps/obsidian/src/components/canvas/utils/openFileUtils.ts (2)
10-68: Well-structured utility with comprehensive error handling.The function properly encapsulates block reference resolution with clear toast messages for each failure mode. The JSDoc comment accurately describes the behavior.
99-106: LGTM!Clean implementation following the same pattern as
openFileInNewTab. Using"split"for a new leaf is the correct Obsidian workspace API.
https://www.loom.com/share/28cc2d9a82aa4cc495504f780066479a
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.