Add context menu functionality. AI-assisted, tested manually.#1
Add context menu functionality. AI-assisted, tested manually.#1roleohibachi wants to merge 1 commit intoethanolivertroy:masterfrom
Conversation
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds context menu functionality to convert files to Markdown using Markitdown directly from the Obsidian file explorer. Users can right-click on supported file types (PDF, DOCX, PPTX, etc.) and select "Convert to Markdown with Markitdown" to initiate conversion.
Key Changes
- Adds a file menu event handler that displays a context menu option for supported file types
- Implements inline conversion logic that handles file selection, output path determination, and file opening
- Supports the same file types as the existing modal-based conversion workflow
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Refresh the vault to see the new file | ||
| await this.app.vault.adapter.exists(outputPath); | ||
|
|
||
| new Notice(`File converted and saved to ${outputPath}`); |
There was a problem hiding this comment.
The error message displays the full system file path which may expose sensitive information about the user's file system structure. Consider using only the relative path within the vault (as computed on line 162) for the success message to be consistent with Obsidian's typical UX and avoid exposing absolute paths.
| new Notice(`File converted and saved to ${outputPath}`); | |
| new Notice(`File converted and saved to ${relativePath}`); |
| const supportedExtensions = ['pdf', 'docx', 'pptx', 'xlsx', 'xls', | ||
| 'html', 'htm', 'txt', 'csv', 'json', 'xml', | ||
| 'jpg', 'jpeg', 'png', 'gif', 'wav', 'mp3', 'zip']; |
There was a problem hiding this comment.
The list of supported extensions is duplicated here and in the file input accept attribute at line 314. Consider extracting this to a constant at the class level (e.g., SUPPORTED_EXTENSIONS) to maintain a single source of truth and make future updates easier.
| try { | ||
| // Get vault path | ||
| let vaultPath = ''; | ||
| if (this.app.vault.adapter instanceof FileSystemAdapter) { | ||
| vaultPath = this.app.vault.adapter.getBasePath(); | ||
| } | ||
|
|
||
| if (!vaultPath) { | ||
| new Notice('Could not determine vault path. This plugin requires a local vault.'); | ||
| return; | ||
| } | ||
|
|
||
| // Get file path | ||
| const filePath = path.join(vaultPath, file.path); | ||
|
|
||
| // Determine output path | ||
| let outputFolder = this.settings.outputPath || ''; | ||
| if (!outputFolder) { | ||
| outputFolder = path.join(vaultPath, 'markitdown-output'); | ||
| if (!fs.existsSync(outputFolder)) { | ||
| fs.mkdirSync(outputFolder, { recursive: true }); | ||
| } | ||
| } else { | ||
| outputFolder = path.join(vaultPath, outputFolder); | ||
| if (!fs.existsSync(outputFolder)) { | ||
| fs.mkdirSync(outputFolder, { recursive: true }); | ||
| } | ||
| } | ||
|
|
||
| // Create output filename | ||
| const baseName = path.basename(file.path, path.extname(file.path)); | ||
| const outputPath = path.join(outputFolder, `${baseName}.md`); | ||
|
|
||
| new Notice('Converting file...'); | ||
|
|
||
| // Convert the file | ||
| await this.convertFile(filePath, outputPath); | ||
|
|
||
| // Refresh the vault to see the new file | ||
| await this.app.vault.adapter.exists(outputPath); | ||
|
|
||
| new Notice(`File converted and saved to ${outputPath}`); | ||
|
|
||
| // Try to open the converted file | ||
| const relativePath = path.relative(vaultPath, outputPath).replace(/\\/g, '/'); | ||
| const existingFile = this.app.vault.getAbstractFileByPath(relativePath); | ||
| if (existingFile instanceof TFile) { | ||
| this.app.workspace.getLeaf().openFile(existingFile); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error during conversion:', error); | ||
| new Notice(`Error: ${error.message}`); | ||
| } |
There was a problem hiding this comment.
This conversion logic (lines 118-170) largely duplicates the logic in MarkitdownFileModal (lines 329-388). Consider extracting the shared conversion logic into a reusable method on the plugin class to reduce duplication and improve maintainability. For example, create a method like async performConversion(file: TFile, vaultPath: string) that handles the output folder creation, path resolution, conversion, and file opening logic.
| // Register file menu event handler for context menu | ||
| this.registerEvent( | ||
| this.app.workspace.on('file-menu', (menu, file) => { | ||
| // Only show for files, not folders | ||
| if (!(file instanceof TFile)) return; | ||
|
|
||
| // Get file extension | ||
| const extension = file.extension.toLowerCase(); | ||
|
|
||
| // List of supported extensions | ||
| const supportedExtensions = ['pdf', 'docx', 'pptx', 'xlsx', 'xls', | ||
| 'html', 'htm', 'txt', 'csv', 'json', 'xml', | ||
| 'jpg', 'jpeg', 'png', 'gif', 'wav', 'mp3', 'zip']; | ||
|
|
||
| // Only show for supported file types | ||
| if (!supportedExtensions.includes(extension)) return; | ||
|
|
||
| menu.addItem((item) => { | ||
| item | ||
| .setTitle('Convert to Markdown with Markitdown') | ||
| .setIcon('file-text') | ||
| .onClick(async () => { | ||
| if (!this.markitdownInstalled) { | ||
| new MarkitdownSetupModal(this.app, this).open(); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Get vault path | ||
| let vaultPath = ''; | ||
| if (this.app.vault.adapter instanceof FileSystemAdapter) { | ||
| vaultPath = this.app.vault.adapter.getBasePath(); | ||
| } | ||
|
|
||
| if (!vaultPath) { | ||
| new Notice('Could not determine vault path. This plugin requires a local vault.'); | ||
| return; | ||
| } | ||
|
|
||
| // Get file path | ||
| const filePath = path.join(vaultPath, file.path); | ||
|
|
||
| // Determine output path | ||
| let outputFolder = this.settings.outputPath || ''; | ||
| if (!outputFolder) { | ||
| outputFolder = path.join(vaultPath, 'markitdown-output'); | ||
| if (!fs.existsSync(outputFolder)) { | ||
| fs.mkdirSync(outputFolder, { recursive: true }); | ||
| } | ||
| } else { | ||
| outputFolder = path.join(vaultPath, outputFolder); | ||
| if (!fs.existsSync(outputFolder)) { | ||
| fs.mkdirSync(outputFolder, { recursive: true }); | ||
| } | ||
| } | ||
|
|
||
| // Create output filename | ||
| const baseName = path.basename(file.path, path.extname(file.path)); | ||
| const outputPath = path.join(outputFolder, `${baseName}.md`); | ||
|
|
||
| new Notice('Converting file...'); | ||
|
|
||
| // Convert the file | ||
| await this.convertFile(filePath, outputPath); | ||
|
|
||
| // Refresh the vault to see the new file | ||
| await this.app.vault.adapter.exists(outputPath); | ||
|
|
||
| new Notice(`File converted and saved to ${outputPath}`); | ||
|
|
||
| // Try to open the converted file | ||
| const relativePath = path.relative(vaultPath, outputPath).replace(/\\/g, '/'); | ||
| const existingFile = this.app.vault.getAbstractFileByPath(relativePath); | ||
| if (existingFile instanceof TFile) { | ||
| this.app.workspace.getLeaf().openFile(existingFile); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error during conversion:', error); | ||
| new Notice(`Error: ${error.message}`); | ||
| } | ||
| }); | ||
| }); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Inconsistent indentation: this section uses spaces for indentation while the rest of the file appears to use tabs (based on the surrounding code at lines 65-88). This creates a mixed indentation style. Consider using tabs to match the existing code style.
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a file-menu context action to MarkitdownPlugin that appears for supported file types, validates installation, computes and creates an output folder, invokes convertFile to produce a Markdown file, refreshes vault visibility, and attempts to open the generated .md file. Errors are reported via Notice. Changes
Sequence DiagramsequenceDiagram
actor User
participant Menu as Context Menu
participant Plugin as MarkitdownPlugin
participant FS as FileSystemAdapter
participant Vault as Vault
participant Convert as convertFile
participant Leaf as Editor Leaf
User->>Menu: Right-click supported file
Menu->>Plugin: onFileMenu callback
alt Markitdown not installed
Plugin->>Plugin: Show setup modal
else Markitdown installed
Plugin->>FS: Get vault base path
FS-->>Plugin: base path
Plugin->>Vault: Ensure output folder exists
Vault-->>Plugin: folder ready
Plugin->>Convert: convertFile(inputPath, outputPath)
Convert-->>Plugin: conversion result
Plugin->>Vault: Refresh file visibility
Plugin->>Leaf: Open converted .md
end
opt Error occurs
Plugin->>User: Show Notice with error message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@main.ts`:
- Line 154: The convertFile implementation currently interpolates filePath into
a shell command (using exec) which allows command injection; update convertFile
to avoid shell interpretation by using child_process.spawn (or spawnSync) with
an argument array and shell: false, passing filePath and any flags as separate
arguments, pipe spawn.stdout into the output stream for outputPath, handle
spawn.stderr and exit code to resolve/reject the Promise, and remove any use of
exec or string interpolation of filePath; alternatively, validate/sanitize
filePath before use to ensure it contains no metacharacters (refer to
convertFile, filePath, outputPath, and the external tool name markitdown in your
changes).
- Around line 167-169: The catch blocks that log errors and create Notices
(e.g., the one with console.error('Error during conversion:', error) and new
Notice(`Error: ${error.message}`)) assume caught values are Error instances;
change them to safely extract a message by using a type guard/normalizer (e.g.,
const msg = error instanceof Error ? error.message : String(error)) and use msg
for both console.error and new Notice; apply the same change to the other
similar catch blocks mentioned (the ones that currently access error.message at
the other two locations).
♻️ Duplicate comments (5)
main.ts (5)
101-103: Extract supported extensions to a shared constant.The supported extensions list is duplicated here and in the file input
acceptattribute at line 314. Extract to a class-level constant (e.g.,SUPPORTED_EXTENSIONS) to maintain a single source of truth.♻️ Suggested refactor
export default class MarkitdownPlugin extends Plugin { settings: MarkitdownSettings; pythonInstalled: boolean = false; markitdownInstalled: boolean = false; + static readonly SUPPORTED_EXTENSIONS = ['pdf', 'docx', 'pptx', 'xlsx', 'xls', + 'html', 'htm', 'txt', 'csv', 'json', 'xml', + 'jpg', 'jpeg', 'png', 'gif', 'wav', 'mp3', 'zip'];Then use
MarkitdownPlugin.SUPPORTED_EXTENSIONSin both locations.
91-174: Inconsistent indentation style.This section uses spaces while the rest of the file uses tabs. Align with the existing code style by using tabs for indentation.
118-170: Extract shared conversion logic to reduce duplication.This conversion workflow (vault path resolution, output folder creation, conversion, file opening) is largely duplicated in
MarkitdownFileModal(lines 329-388). Extract into a reusable method likeasync performConversion(file: TFile | string, outputFolder?: string).
159-159: Avoid exposing absolute file system paths to users.The Notice displays the full system path which may reveal sensitive directory structure. Use the relative vault path instead for consistency with Obsidian's UX.
Suggested fix
- new Notice(`File converted and saved to ${outputPath}`); + new Notice(`File converted and saved to ${relativePath}`);Note: Move
relativePathcomputation (line 162) before this Notice.
156-157:exists()does not refresh the vault index.
adapter.exists()only checks file existence on disk; it doesn't trigger Obsidian's vault indexing. The newly created file may not be immediately available viagetAbstractFileByPath()on line 163.Suggested fix
Use Obsidian's vault methods to ensure the file is indexed:
- // Refresh the vault to see the new file - await this.app.vault.adapter.exists(outputPath); + // Ensure the vault indexes the new file + // Option 1: Read the file through vault API to trigger indexing + const relativePath = path.relative(vaultPath, outputPath).replace(/\\/g, '/'); + + // Small delay to allow file system events to propagate + await new Promise(resolve => setTimeout(resolve, 100));Alternatively, consider writing the output file using
this.app.vault.create()instead of letting the external process write directly, which would automatically index the file.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
main.ts
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| new Notice('Converting file...'); | ||
|
|
||
| // Convert the file | ||
| await this.convertFile(filePath, outputPath); |
There was a problem hiding this comment.
Potential command injection vulnerability in file paths.
The convertFile method (lines 227-255) passes filePath directly into shell commands using string interpolation. If a filename contains shell metacharacters (e.g., $(cmd), backticks, or semicolons), arbitrary commands could be executed.
🔒 Recommended mitigation
Use spawn with an argument array instead of exec with string interpolation to avoid shell interpretation:
// In convertFile method, replace exec with spawn
import { spawn } from 'child_process';
async convertFile(filePath: string, outputPath: string): Promise<string> {
return new Promise((resolve, reject) => {
const outputStream = fs.createWriteStream(outputPath);
const proc = spawn('markitdown', [filePath], { shell: false });
proc.stdout.pipe(outputStream);
proc.stderr.on('data', (data) => { /* handle stderr */ });
proc.on('close', (code) => {
if (code === 0) resolve('');
else reject(new Error(`markitdown exited with code ${code}`));
});
});
}Alternatively, validate/sanitize file paths before use.
🤖 Prompt for AI Agents
In `@main.ts` at line 154, The convertFile implementation currently interpolates
filePath into a shell command (using exec) which allows command injection;
update convertFile to avoid shell interpretation by using child_process.spawn
(or spawnSync) with an argument array and shell: false, passing filePath and any
flags as separate arguments, pipe spawn.stdout into the output stream for
outputPath, handle spawn.stderr and exit code to resolve/reject the Promise, and
remove any use of exec or string interpolation of filePath; alternatively,
validate/sanitize filePath before use to ensure it contains no metacharacters
(refer to convertFile, filePath, outputPath, and the external tool name
markitdown in your changes).
| } catch (error) { | ||
| console.error('Error during conversion:', error); | ||
| new Notice(`Error: ${error.message}`); |
There was a problem hiding this comment.
Handle unknown error type safely.
In TypeScript strict mode, caught errors are typed as unknown. Accessing error.message directly may fail if the error is not an Error instance.
Suggested fix
} catch (error) {
console.error('Error during conversion:', error);
- new Notice(`Error: ${error.message}`);
+ const message = error instanceof Error ? error.message : String(error);
+ new Notice(`Error: ${message}`);
}This pattern should also be applied at lines 391 and 598.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('Error during conversion:', error); | |
| new Notice(`Error: ${error.message}`); | |
| } catch (error) { | |
| console.error('Error during conversion:', error); | |
| const message = error instanceof Error ? error.message : String(error); | |
| new Notice(`Error: ${message}`); | |
| } |
🤖 Prompt for AI Agents
In `@main.ts` around lines 167 - 169, The catch blocks that log errors and create
Notices (e.g., the one with console.error('Error during conversion:', error) and
new Notice(`Error: ${error.message}`)) assume caught values are Error instances;
change them to safely extract a message by using a type guard/normalizer (e.g.,
const msg = error instanceof Error ? error.message : String(error)) and use msg
for both console.error and new Notice; apply the same change to the other
similar catch blocks mentioned (the ones that currently access error.message at
the other two locations).
Greptile SummaryThis PR adds a context menu option to convert supported file types to Markdown using the Markitdown tool. When users right-click on files with supported extensions (PDF, DOCX, PPTX, XLSX, images, audio, etc.), a "Convert to Markdown with Markitdown" option appears. Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ContextMenu as Context Menu
participant Plugin as MarkitdownPlugin
participant Vault as Obsidian Vault
participant FS as File System
participant Markitdown as Markitdown CLI
User->>ContextMenu: Right-click on file
ContextMenu->>Plugin: Check file extension
Plugin->>Plugin: Validate against supportedExtensions
alt File type supported
Plugin->>ContextMenu: Add "Convert to Markdown" menu item
ContextMenu->>User: Display menu option
User->>Plugin: Click "Convert to Markdown"
Plugin->>Plugin: Check markitdownInstalled
alt Markitdown not installed
Plugin->>User: Show MarkitdownSetupModal
else Markitdown installed
Plugin->>Vault: Get vault base path
Plugin->>FS: Determine output folder path
Plugin->>FS: Create output folder if needed
Plugin->>User: Show "Converting file..." notice
Plugin->>Markitdown: Convert file to markdown
Markitdown-->>Plugin: Conversion result
Plugin->>Vault: Check if output file exists (refresh)
Plugin->>User: Show success notice
Plugin->>Vault: Get converted file
Plugin->>User: Open converted file in editor
end
else File type not supported
Plugin->>ContextMenu: Don't add menu item
end
|
| // Register file menu event handler for context menu | ||
| this.registerEvent( |
There was a problem hiding this comment.
style: inconsistent indentation - file uses tabs but these lines use spaces
| // Register file menu event handler for context menu | |
| this.registerEvent( | |
| // Register file menu event handler for context menu | |
| this.registerEvent( |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: main.ts
Line: 91:92
Comment:
**style:** inconsistent indentation - file uses tabs but these lines use spaces
```suggestion
// Register file menu event handler for context menu
this.registerEvent(
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main.ts`:
- Around line 147-150: The outputPath construction uses only the basename
(baseName) which causes collisions when different source files share the same
name; change the logic in main.ts around baseName/outputPath to preserve the
source file's relative directory (e.g., compute a relativePath from the input
root using file.path and include it under outputFolder) or, if preserving
structure isn't desired, append a unique suffix (timestamp or increment) to
baseName before joining with outputFolder; update any creation of directories
accordingly so the directory tree exists before writing.
♻️ Duplicate comments (1)
main.ts (1)
100-103: Deduplicate supported extensions.The list here is duplicated elsewhere (file input accept list). Consider extracting a shared constant to keep updates in sync.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
main.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Create output filename | ||
| const baseName = path.basename(file.path, path.extname(file.path)); | ||
| const outputPath = path.join(outputFolder, `${baseName}.md`); | ||
|
|
There was a problem hiding this comment.
Avoid overwriting conversions for same base filename.
Line 148 builds outputPath solely from the basename, so two files with the same name in different folders will overwrite each other in the output folder. Preserve relative directories (or add a suffix) to avoid silent collisions.
🔧 Proposed fix (preserve relative folder structure)
- const baseName = path.basename(file.path, path.extname(file.path));
- const outputPath = path.join(outputFolder, `${baseName}.md`);
+ const baseName = path.basename(file.path, path.extname(file.path));
+ const relDir = path.dirname(file.path);
+ const outputDir = path.join(outputFolder, relDir);
+ if (!fs.existsSync(outputDir)) {
+ fs.mkdirSync(outputDir, { recursive: true });
+ }
+ const outputPath = path.join(outputDir, `${baseName}.md`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create output filename | |
| const baseName = path.basename(file.path, path.extname(file.path)); | |
| const outputPath = path.join(outputFolder, `${baseName}.md`); | |
| // Create output filename | |
| const baseName = path.basename(file.path, path.extname(file.path)); | |
| const relDir = path.dirname(file.path); | |
| const outputDir = path.join(outputFolder, relDir); | |
| if (!fs.existsSync(outputDir)) { | |
| fs.mkdirSync(outputDir, { recursive: true }); | |
| } | |
| const outputPath = path.join(outputDir, `${baseName}.md`); | |
🤖 Prompt for AI Agents
In `@main.ts` around lines 147 - 150, The outputPath construction uses only the
basename (baseName) which causes collisions when different source files share
the same name; change the logic in main.ts around baseName/outputPath to
preserve the source file's relative directory (e.g., compute a relativePath from
the input root using file.path and include it under outputFolder) or, if
preserving structure isn't desired, append a unique suffix (timestamp or
increment) to baseName before joining with outputFolder; update any creation of
directories accordingly so the directory tree exists before writing.
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Closing — a major rewrite of the plugin is underway using the new Obsidian CLI. The new version will include context menu functionality. Thanks for the contribution! |
Ground-up rewrite addressing critical security vulnerability (shell injection via exec() with string interpolation) and all open GitHub issues (#3, #4, #6, #8, PR #1). Security: - Replace all exec() calls with spawn() + argument arrays (shell: false) - Ship bundled Python wrapper scripts called via argparse - Zero string interpolation of user input into shell commands - Set PYTHONUTF8=1 for Unicode/diacritics support (#4) Architecture: - Decompose monolithic main.ts (735 lines) into 12 focused modules - Separate converter, modals, settings, and utilities into src/ - Bundled Python scripts in python/ directory New features: - Image extraction from PDFs/EPUBs to subfolder (#6) - Plugin argument configuration via key-value UI (#8) - Right-click context menu conversion (PR #1) - Guided setup wizard with streaming pip output (#3) - Batch progress bar for folder conversions Dependencies: - TypeScript 4.7 -> 5.4, @types/node 16 -> 20, target ES2020 - Enable strict mode
This PR implements a context menu option for convertible filetypes.
First, add some files to your vault that can be converted. I used .pptx.
Second, Obsidian settings > Files and links > Detect all file extensions. This allows the pptx files to be visible in the vault.
Then, right click the file and choose "Convert to Markdown with Markitdown".
I wrote very little of this code myself (thanks, AI!) but tested it as above.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.