Obsidian: Hello World #83
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
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 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive setup for an Obsidian plugin called Discourse Graph. The changes span multiple files across the project, establishing a new Obsidian plugin with React integration, build scripts, development configurations, and documentation. The implementation includes TypeScript configurations, environment setup, React components for settings and modals, and utility functions for registering plugin commands. Changes
Sequence DiagramsequenceDiagram
participant User
participant ObsidianApp
participant DiscourseGraphPlugin
participant SettingsTab
participant Commands
User->>ObsidianApp: Install Plugin
ObsidianApp->>DiscourseGraphPlugin: onload()
DiscourseGraphPlugin->>SettingsTab: Create Settings
DiscourseGraphPlugin->>Commands: Register Commands
User->>Commands: Trigger Command
Commands->>DiscourseGraphPlugin: Execute Action
DiscourseGraphPlugin->>ObsidianApp: Perform Task
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
| @@ -0,0 +1,29 @@ | |||
| { | |||
| "name": "@discourse-graphs/obsidian", | |||
There was a problem hiding this comment.
can't just use obsidian because the npm obsidian package is in our dependencies sad face. will probably namespace all of the plugins this way
| @@ -0,0 +1,27 @@ | |||
| import { compile } from "./compile"; | |||
There was a problem hiding this comment.
will generalize all of the scripts/ to work for all apps (as best we can) later
There was a problem hiding this comment.
| }, | ||
| }, | ||
| { | ||
| name: "combineStyles", |
There was a problem hiding this comment.
this is temp.
need to include css in entryPoints: https://roamresearch.com/#/app/discourse-graphs/page/P2IyJVpf9
also tailwind: https://roamresearch.com/#/app/discourse-graphs/page/Y-LXnxfPh
| @@ -0,0 +1,3 @@ | |||
| .modal-content { | |||
| @@ -0,0 +1,18 @@ | |||
| { | |||
| "extends": "@repo/typescript-config/react-library.json", | |||
There was a problem hiding this comment.
needs to be migrated as much as possible to the @repo/typescript-config
https://roamresearch.com/#/app/discourse-graphs/page/s6veJ_Zvr
|
@coderabbitai full review |
✅ Actions performedFull review 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: 9
🧹 Nitpick comments (20)
apps/obsidian/tsconfig.json (1)
17-17: Consider expanding TypeScript file patterns.The current include pattern
["**/*.ts"]might be too restrictive. Since this is a React project, you might want to include.tsxfiles as well.- "include": ["**/*.ts"] + "include": ["**/*.ts", "**/*.tsx"]apps/obsidian/scripts/compile.ts (2)
47-64: Handle non-existing or locked directories gracefully.
ThereadDirfunction logs an error if the directory doesn't exist but continues execution. If there are permission or lock issues, the function returns an empty array. Consider handling such errors more formally or rethrowing them to indicate a failure status, especially in production builds.
70-70: Consider consolidating environment variable checks.
You’re loading environment variables globally usingdotenv.config()(line 8) and also referencing environment variables inargs. It might be beneficial to unify your environment checks and defaults in one place to reduce potential confusion or re-initialization issues.apps/obsidian/src/components/AppContext.tsx (1)
10-18: Enhance clarity with a dedicated props interface.
Creating a dedicated interface, e.g.,interface ContextProviderProps { app: App; children: React.ReactNode; }, may improve type consistency and readability.apps/obsidian/scripts/build.ts (2)
3-8: Validate or log theNODE_ENVsetting.
You explicitly setNODE_ENV = "production"when it’s not defined, but you do not log or validate that the final value aligns with your expectations. Consider adding a brief log or check to confirm environment correctness and possibly warn ifNODE_ENVwas already set differently.
9-16: Surface esbuild warnings and partial failures.
This block indicates a console message for compile failures and terminates the process upon error, but warnings from esbuild may go unnoticed. Consider capturing and logging warnings as well to ensure no relevant issues slip by in production.apps/obsidian/scripts/dev.ts (1)
19-26: Enhance error handling in main functionThe error handling could be more informative by including error details in the console output.
const main = async () => { try { await dev(); } catch (error) { - console.error(error); + console.error('Development build failed:', error instanceof Error ? error.message : String(error)); process.exit(1); } };apps/obsidian/src/index.ts (1)
24-26: Add type assertion for loaded dataThe
loadData()method returnsany, which could lead to runtime errors if the loaded data doesn't match theSettingstype.async loadSettings() { - this.settings = Object.assign({}, DEFAULT_SETTINGS, await this.loadData()); + const loadedData = await this.loadData() as Partial<Settings>; + this.settings = { ...DEFAULT_SETTINGS, ...loadedData }; }apps/obsidian/src/components/SampleModal.tsx (2)
6-7: Remove empty className propsEmpty className props don't serve any purpose and should be removed.
- <div className=""> + <div> <h2 className="">SO RED</h2> <p>Lorum ipsum dolor sit amet.</p> <button - className="" + className="clickable-icon" onClick={() => {Also applies to: 10-10
21-24: Remove unnecessary constructorThe constructor doesn't add any functionality beyond the parent constructor.
- constructor(app: App) { - super(app); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 22-24: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
apps/obsidian/src/components/Settings.tsx (2)
9-11: Improve error message for missing contextThe current error message is not descriptive enough. Consider providing more context about what went wrong.
if (!app) { - return <div>An error occured</div>; + return <div>Error: Obsidian app context not found. This component must be used within an AppContext.Provider.</div>; }
45-53: Add debounce to settings onChange handlerThe current implementation saves settings on every keystroke, which could lead to performance issues with rapid typing.
+ import { debounce } from "obsidian"; + addText((text) => text .setPlaceholder("Enter your secret") .setValue(this.plugin.settings.mySetting) - .onChange(async (value) => { + .onChange(debounce(async (value) => { this.plugin.settings.mySetting = value; await this.plugin.saveSettings(); - }), + }, 500)), );apps/obsidian/src/utils/registerCommands.ts (1)
10-12: Add error handling for modal creation.Both modal commands should include try-catch blocks to handle potential initialization errors.
callback: () => { + try { new SampleModal(plugin.app).open(); + } catch (error) { + console.error('Failed to open sample modal:', error); + } },Also applies to: 36-38
apps/obsidian/.env.example (1)
1-1: Enhance environment variable documentation.Consider adding:
- Platform-specific path examples (Windows, macOS, Linux)
- Instructions for finding the Obsidian plugins folder
- A note about copying this file to
.env-# OBSIDIAN_PLUGIN_PATH="path/to/your/obsidian/plugins/folder" +# Copy this file to .env and update the path to your Obsidian plugins folder +# Examples: +# Windows: OBSIDIAN_PLUGIN_PATH="C:/Users/USERNAME/AppData/Roaming/obsidian/plugins" +# macOS: OBSIDIAN_PLUGIN_PATH="/Users/USERNAME/Library/Application Support/obsidian/plugins" +# Linux: OBSIDIAN_PLUGIN_PATH="/home/USERNAME/.config/obsidian/plugins"apps/obsidian/manifest.json (1)
1-10: Add recommended manifest fields.Consider adding these recommended fields to enhance plugin discoverability and maintenance:
fundingUrl: For sponsorship/donation linksrepository: Link to the GitHub repositorykeywords: For better search visibility{ "id": "@discourse-graph/obsidian", "name": "Discourse Graph", "version": "0.1.0", "minAppVersion": "1.7.0", "description": "Discourse Graph Plugin for Obsidian", "author": "Discourse Graphs", "authorUrl": "https://discoursegraphs.com", - "isDesktopOnly": false + "isDesktopOnly": false, + "fundingUrl": "https://github.com/sponsors/DiscourseGraphs", + "repository": "https://github.com/DiscourseGraphs/discourse-graph", + "keywords": ["discourse", "graph", "knowledge-management"] }apps/obsidian/README.md (2)
1-5: Enhance documentation with additional sections.While the overview is good, consider adding essential sections:
- Installation instructions
- Basic usage guide
- Development setup
- Contributing guidelines
3-3: Consider breaking down the long sentence for better readability.The description is contained in a single long sentence. Consider breaking it down into multiple sentences for clarity.
-The Discourse Graph extension enables Roam users to seamlessly add additional semantic structure to their notes, including specified page types and link types that model scientific discourse, to enable more complex and structured knowledge synthesis work, such as a complex interdisciplinary literature review, and enhanced collaboration with others on this work. +The Discourse Graph extension enables Roam users to seamlessly add semantic structure to their notes. Users can specify page types and link types that model scientific discourse. This structured approach enables complex knowledge synthesis work, such as interdisciplinary literature reviews, and enhances collaboration.apps/obsidian/package.json (1)
15-23: Consider updating TypeScript-related dependencies.The TypeScript and related packages are using older versions. Consider updating to more recent versions for better compatibility and features.
"devDependencies": { - "@types/node": "^16.11.6", + "@types/node": "^20.10.0", - "@typescript-eslint/eslint-plugin": "5.29.0", - "@typescript-eslint/parser": "5.29.0", + "@typescript-eslint/eslint-plugin": "^6.13.0", + "@typescript-eslint/parser": "^6.13.0", "builtin-modules": "3.3.0", "esbuild": "0.17.3", "obsidian": "^1.7.2", - "tslib": "2.4.0", + "tslib": "^2.6.2", "tsx": "^4.19.2", - "typescript": "4.7.4" + "typescript": "^5.3.0"README.md (2)
72-75: Enhance Obsidian setup instructions.Consider adding more details to the Obsidian setup:
- Specify where to find the plugins folder on different operating systems
- Add steps to enable the plugin in Obsidian after installation
- Include troubleshooting steps
12-12: Strengthen the wording.Consider using "accelerates" instead of "speeds up" for stronger wording.
-Only changes in code are rebuilt, which speeds up development. +Only changes in code are rebuilt, which accelerates development.🧰 Tools
🪛 LanguageTool
[style] ~12-~12: Consider using a different verb to strengthen your wording.
Context: ...Only changes in code are rebuilt, which speeds up development. - Parallel processing:...(SPEED_UP_ACCELERATE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
README.md(2 hunks)apps/obsidian/.env.example(1 hunks)apps/obsidian/README.md(1 hunks)apps/obsidian/manifest.json(1 hunks)apps/obsidian/package.json(1 hunks)apps/obsidian/scripts/build.ts(1 hunks)apps/obsidian/scripts/compile.ts(1 hunks)apps/obsidian/scripts/dev.ts(1 hunks)apps/obsidian/src/components/AppContext.tsx(1 hunks)apps/obsidian/src/components/SampleModal.tsx(1 hunks)apps/obsidian/src/components/Settings.tsx(1 hunks)apps/obsidian/src/index.ts(1 hunks)apps/obsidian/src/styles/styles.css(1 hunks)apps/obsidian/src/utils/registerCommands.ts(1 hunks)apps/obsidian/tsconfig.json(1 hunks)apps/roam/tsconfig.json(0 hunks)packages/typescript-config/base.json(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/roam/tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
apps/obsidian/src/components/SampleModal.tsx
[error] 22-24: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🪛 LanguageTool
README.md
[style] ~12-~12: Consider using a different verb to strengthen your wording.
Context: ...Only changes in code are rebuilt, which speeds up development. - Parallel processing:...
(SPEED_UP_ACCELERATE)
🔇 Additional comments (11)
apps/obsidian/tsconfig.json (2)
2-2: Configuration already follows the recommended base.The configuration already extends
@repo/typescript-config/react-library.jsonas suggested in the past review comment.
3-16: Verify compiler options against the base configuration.Some of these compiler options might be redundant if they're already defined in
@repo/typescript-config/react-library.json. Consider removing duplicate options to maintain a cleaner configuration.✅ Verification successful
The compiler options are appropriately configured for Obsidian's requirements
While some options overlap with the base configuration, the differences in values (e.g., ES6 vs ES2022, ESNext vs NodeNext) and Obsidian-specific options (e.g., inlineSourceMap, inlineSources) appear intentional and necessary for Obsidian's environment. These aren't redundant configurations but rather purposeful overrides.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the base TypeScript configuration to identify duplicate options # Find and display the base configuration file echo "Searching for the base configuration file..." fd -t f "react-library.json" -x cat {}Length of output: 275
Script:
#!/bin/bash # Find and display the base.json configuration file echo "Searching for base.json..." fd -t f "base.json" -x cat {}Length of output: 681
packages/typescript-config/base.json (2)
7-7: LGTM: Standardizing module import behavior.Moving
allowSyntheticDefaultImportsto the base configuration standardizes how default imports are handled across all packages. This is a good practice for maintaining consistency.
4-6: Verify that all packages follow the expected directory structure.The paths mapping assumes all packages using this base config have a
srcdirectory. Please ensure this is the intended behavior and all packages follow this structure.✅ Verification successful
The paths mapping configuration is safe and flexible ✅
The current setup works well because:
- Packages that need src-based imports (like ui) can override the paths
- Configuration packages that work from root directory (like tailwind-config) include "." making the src mapping non-impactful
- Other packages don't extend the TypeScript configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all packages have a src directory echo "Checking for src directories in all packages..." fd --type d '^src$' packages/ --exec echo "Found src directory in: {//}" # List all package.json files to compare against echo -e "\nFound packages:" fd package.json packages/ --exec echo "{//}"Length of output: 411
Script:
#!/bin/bash # Check which packages extend the base typescript config echo "Checking for tsconfig.json files and their content:" fd tsconfig.json packages/ --exec sh -c 'echo "\nFile: {}"; cat {}'Length of output: 598
apps/obsidian/scripts/compile.ts (2)
110-121: Incorporate CSS in your build entry points.
A TODO was mentioned regarding including CSS inentryPointsand using Tailwind. If desired, you can bundle styles with esbuild directly by specifying.cssfiles as entry points or using a plugin. This manual approach is fine, but bundling could simplify the pipeline.builder({ absWorkingDir: process.cwd(), - entryPoints: [path.join(srcRoot, entryTs)], + entryPoints: [ + path.join(srcRoot, entryTs), + // Include CSS entry if needed: + // path.join(stylesDir, "main.css"), + ], outdir, ...
138-164: Avoid self-mirroring or recursive copies.
Whenmirroroverlaps with youroutdir, the mirror plugin copies compiled files from the output to the same output path, potentially causing repetition or overwriting. Consider detecting ifmirror === outdiror ifmirroris withinoutdirto prevent unintended side effects.apps/obsidian/src/components/AppContext.tsx (1)
4-5: Provide fallback or error handling for undefined context.
WhenuseApp()returnsundefined, any consumers may cause a runtime error if they assume a validAppis always present. Consider returning a default object or throwing a clear error ifAppContextis not provided.apps/obsidian/src/index.ts (1)
22-22: Implement cleanup in onunloadThe
onunloadmethod is empty. Consider implementing cleanup logic for registered commands and settings.✅ Verification successful
Empty onunload() is acceptable
The current implementation doesn't require explicit cleanup as:
- Commands are automatically unregistered by Obsidian's plugin system
- Settings tab is automatically cleaned up
- No event listeners or persistent resources are found that would require manual cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any resources that need cleanup rg -l "registerEvent|addCommand|addSettingTab" apps/obsidian/src/Length of output: 138
Script:
#!/bin/bash # Check the actual registration calls and their context echo "=== Content of registerCommands.ts ===" cat apps/obsidian/src/utils/registerCommands.ts echo -e "\n=== Relevant sections from index.ts ===" rg -A 5 -B 5 "registerEvent|addCommand|addSettingTab" apps/obsidian/src/index.tsLength of output: 2201
apps/obsidian/manifest.json (1)
2-2: Review the plugin ID format.The current ID uses an npm-style scoped package name (
@discourse-graph/obsidian). Consider using a simpler, reverse-domain format as recommended by Obsidian (e.g.,com.discoursegraphs.obsidian).turbo.json (1)
18-18: LGTM!The addition of
OBSIDIAN_PLUGIN_PATHtopassThroughEnvis appropriate for Obsidian plugin development.README.md (1)
5-16: LGTM!The Turborepo section is well-written and provides clear benefits of using the build system.
🧰 Tools
🪛 LanguageTool
[style] ~12-~12: Consider using a different verb to strengthen your wording.
Context: ...Only changes in code are rebuilt, which speeds up development. - Parallel processing:...(SPEED_UP_ACCELERATE)
| const stylesDir = path.join(root, "src", "styles"); | ||
| const outputStylesFile = path.join(outdir, "styles.css"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gracefully handle missing styles directory.
This script expects a styles directory to exist. If the directory is absent, an error occurs when reading style files. Consider adding a guard check or fallback behavior when stylesDir is missing to avoid build-time exceptions.
handle SIGINT and SIGTERM signals Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Release Notes: Discourse Graphs Obsidian Plugin v0.1.0
New Features
Documentation
Development
Environment
.env.examplewith plugin path configuration