[ENG-40] Identify node obsidian #105
Conversation
|
@trangdoan982 is attempting to deploy a commit to the Discourse Graphs Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces a new discourse context feature within the Obsidian plugin. A React component and its associated view class manage the display of file metadata and discourse node information. Enhancements allow for asynchronous creation of discourse nodes via a dedicated modal. The plugin now registers the discourse context view, provides a toggle functionality through both a ribbon icon and a hotkey, and updates its lifecycle management accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as DiscourseGraphPlugin
participant W as WorkspaceLeaf
participant V as DiscourseContextView
U->>P: Toggle view command (ribbon/hotkey)
P->>W: Check for existing discourse context leaf
alt Leaf exists
P->>W: Detach leaf
else Leaf missing
P->>W: Create new leaf on right
W->>V: Set view state to discourse context view
V-->>P: Render DiscourseContext component
Note over P,V: Active file updated after timeout
end
sequenceDiagram
participant U as User
participant M as NodeTypeModal
participant V as Vault
participant E as Editor
U->>M: Choose discourse node suggestion
M->>M: Call async createDiscourseNode()
M->>V: Create file with generated node frontmatter
V-->>M: Return new TFile
M->>E: Replace selected text with node name
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/obsidian/src/index.ts (1)
34-59: Consider replacing setTimeout with an event-driven approach
This toggle method is logically sound, detaching the existing leaf if found and creating one if not. The 50mssetTimeoutensures the new leaf is ready before setting its active file. However, if large files or slow operations occur, a simple time delay can cause race conditions. Consider an event-driven approach (e.g., hooking into Obsidian’s internal signals) instead of using a hard-coded delay.apps/obsidian/src/components/NodeTypeModal.tsx (2)
28-44: Sanitize filenames to avoid vault errors
The logic for creating new node files is correct. However, if titles include forbidden filesystem characters, it could trigger errors or unintended folder creation. Consider sanitizing the filename:async createDiscourseNode( nodeType: DiscourseNode, title: string, ): Promise<TFile> { const instanceId = `${nodeType.id}-${Date.now()}`; const frontmatter = `--- nodeTypeId: ${nodeType.id} nodeInstanceId: ${instanceId} --- `; - const filename = `${title}.md`; + const sanitizedTitle = title.replace(/[\/\\?%*:|"<>]/g, "-"); + const filename = `${sanitizedTitle}.md`; const file = await this.app.vault.create(filename, frontmatter); return file; }
46-61: Handle potential errors when creating new nodes
createDiscourseNodeis an async operation but the code does not handle rejections. It may be beneficial to wrap this call in a try/catch and notify the user with aNoticeif something goes wrong (e.g., insufficient vault permissions or invalid filename).apps/obsidian/src/components/DiscourseContextView.tsx (1)
50-72: Display node type details
Showing the node type name and extracted content gives a concise snapshot of the discourse node. Inline styling is acceptable, but consider using a unified styling approach (CSS or Obsidian theming) for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/obsidian/src/components/DiscourseContextView.tsx(1 hunks)apps/obsidian/src/components/NodeTypeModal.tsx(3 hunks)apps/obsidian/src/index.ts(2 hunks)apps/obsidian/src/utils/registerCommands.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/obsidian/src/index.ts (1)
apps/obsidian/src/components/DiscourseContextView.tsx (2) (2)
VIEW_TYPE_DISCOURSE_CONTEXT(6-6)DiscourseContextView(83-145)
apps/obsidian/src/components/NodeTypeModal.tsx (1)
apps/obsidian/src/types.ts (1) (1)
DiscourseNode(1-7)
🔇 Additional comments (13)
apps/obsidian/src/utils/registerCommands.ts (1)
55-63: Add command for toggling discourse context successfully
This command properly registers a hotkey and references thetoggleDiscourseContextViewmethod. It appears well-integrated into the overall plugin flow, allowing users to quickly reveal or hide the discourse context view.apps/obsidian/src/index.ts (3)
1-2: New imports for enhanced features
ImportingNoticeandWorkspaceLeaflooks consistent for future usage of Obsidian's APIs.
5-8: Import DiscourseContextView modules
Bringing inDiscourseContextViewand its associated view type is essential for registering the custom view.
23-32: Register new view and ribbon icon
Registering the view viathis.registerViewand adding a ribbon icon to toggle it significantly enhances user discoverability. The approach is appropriate and follows Obsidian’s plugin conventions.apps/obsidian/src/components/NodeTypeModal.tsx (1)
1-1: Import TFile for promise-based file creation
IncludingTFilecorrectly declares the return type fromcreateDiscourseNode.apps/obsidian/src/components/DiscourseContextView.tsx (8)
1-2: Core React and Obsidian imports established
These imports correctly set up theItemViewsubclass along with React’screateRoot.
3-4: Index and utility imports
Pulling inDiscourseGraphPluginandgetDiscourseNodeFormatExpressionis crucial for linking context logic and node formatting.
6-6: Unique view type definition
DefiningVIEW_TYPE_DISCOURSE_CONTEXThelps keep code consistent, ensuring the plugin registers and references the correct view.
8-11: Interface for context props
Clear typing: theDiscourseContextPropsinterface ensures that theDiscourseContextcomponent receives everything it needs.
13-22: Extract content from file title
The helper functionextractContentFromTitleis a neat way to parse the active file’s name based on the node type’s format. Returning the full title if no match is found is a safe fallback.
24-47: Context-based rendering checks
Each condition (no file, no metadata, missing frontmatter, unknown node type) is handled gracefully with user-facing messages. This approach clearly communicates the status of the discourse node setup.
75-81: High-level “Discourse Context” heading
Wrapping the rendered content is logical, keeping the entire context UI in a clear container with an identifiable heading.
83-145: Full-featured ItemView for discourse context
The class-based view links seamlessly with Obsidian. UsingonOpenandonCloseto mount/unmount the React component avoids memory leaks. Implementingfile-openevents to dynamically updateactiveFileensures behavior is automatically in sync with the user’s context.
trangdoan982
force-pushed
the
eng-40/identify-node-obsidian
branch
from
294298a to
037d577
Compare
trangdoan982
changed the title
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
A few small changes.
I think this will be a good time to step back and think more about the schema and architecture: https://linear.app/discourse-graphs/issue/ENG-142/v0-schema-and-architecture-planning
We can discuss this more on Monday.
But for now, I think we can keep the implementation presented here.
I also noticed possible improvements on the getDiscourseNodeFormatExpression() / regex implementations (cross plugin and normalize if possible). But we can tackle that later. I've created one of the tasks: https://linear.app/discourse-graphs/issue/ENG-141/updates-to-getdiscoursenodeformatexpression
2 participants
Summary by CodeRabbit