feat(clipboard): support image attachments in note content#1162
Open
feat(clipboard): support image attachments in note content#1162
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds destination-aware clipboard resolution including image embedding and local-file import for Changes
Sequence Diagram(s)sequenceDiagram
participant TemplateEngine as TemplateEngine
participant Formatter as CompleteFormatter
participant Utility as resolveClipboardForNoteContent
participant App as App/Clipboard
participant FileManager as FileManager
participant Vault as Vault
TemplateEngine->>Formatter: setDestinationFile(file) / setDestinationSourcePath(path)
Formatter->>Formatter: store destination context\nenter formattingFileContent scope
Formatter->>Utility: resolveClipboardForNoteContent(app, destinationPath)
Utility->>App: navigator.clipboard.read() / readText()
alt Clipboard image or local-file path found
App-->>Utility: image Blob or fetched bytes
Utility->>FileManager: getAvailablePathForAttachment(suggestedName, destinationPath)
FileManager-->>Utility: availableVaultPath
Utility->>Vault: createBinary(vaultPath, bytes)
Vault-->>Utility: created TFile
Utility->>FileManager: generateMarkdownLink(attachment, destinationPath)
FileManager-->>Formatter: embed string (![[...]])
else No image / fallback text
App-->>Utility: clipboard text (or error -> "")
Utility-->>Formatter: plain text
end
Formatter->>Formatter: replace {{clipboard}} with embed or text
Formatter-->>TemplateEngine: formatted content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Deploying quickadd with
|
| Latest commit: |
1393634
|
| Status: | 🚫 Build failed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/formatters/completeFormatter.clipboard.test.ts (1)
233-267: Minor inconsistency: missingexistsSyncassertion.The PNG test at line 217-219 verifies
existsSyncwas called with the correct path, but this JPEG test omits that assertion. Consider adding it for consistency:expect(result).toBe("![[Assets/pasted-image.png]]"); + expect(app.vault.createBinary).toHaveBeenCalledOnce(); + // Optionally verify existsSync was called: + // expect(existsSync).toHaveBeenCalledWith("/Users/christian/Library/Caches/Clop/images/85074.jpeg"); expect(app.fileManager.getAvailablePathForAttachment).toHaveBeenCalledWith(Not blocking since the test still validates the core behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/formatters/completeFormatter.clipboard.test.ts` around lines 233 - 267, Add the missing assertion that verifies fs.existsSync was called with the JPEG path in this test; locate the mock fs returned by setWindowRequire and assert that the existsSync mock (the function returned for id "fs") was invoked with "/Users/christian/Library/Caches/Clop/images/85074.jpeg" after calling CompleteFormatter.formatFileContent (the same pattern used in the PNG test), so the test consistently checks existence checks alongside readFile and getAvailablePathForAttachment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/formatters/completeFormatter.clipboard.test.ts`:
- Around line 233-267: Add the missing assertion that verifies fs.existsSync was
called with the JPEG path in this test; locate the mock fs returned by
setWindowRequire and assert that the existsSync mock (the function returned for
id "fs") was invoked with
"/Users/christian/Library/Caches/Clop/images/85074.jpeg" after calling
CompleteFormatter.formatFileContent (the same pattern used in the PNG test), so
the test consistently checks existence checks alongside readFile and
getAvailablePathForAttachment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19cf611f-9fe3-4a7f-97f5-c2f9ca8cd3fb
📒 Files selected for processing (3)
src/formatters/completeFormatter.clipboard.test.tssrc/formatters/formatter.tssrc/utilityObsidian.ts
✅ Files skipped from review due to trivial changes (1)
- src/formatters/formatter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utilityObsidian.ts
chhoumann
commented
Apr 7, 2026
src/utilityObsidian.ts
Outdated
| const imagePath = parseClipboardLocalImagePath(clipboardText); | ||
| if (!imagePath) return null; | ||
|
|
||
| const req = |
Owner
Author
There was a problem hiding this comment.
Need to find alternative approach
…tter
- Replace window.require('fs') with fetch('file://...') for reading
local clipboard image paths. This removes the Node.js fs dependency
and uses a standard web API that works in Electron.
- Simplify completeFormatter clipboard handling: replace depth counter
(clipboardContentScopeDepth) with a simple boolean flag
(formattingFileContent), remove withClipboardContentScope wrapper.
- Update tests to mock fetch instead of window.require.
Addresses review comments on PR #1162.
Co-Authored-By: Christian <christian@bagerbach.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/formatters/completeFormatter.ts`:
- Around line 415-432: The clipboard cache (resolvedClipboardContent) can
persist across formatting runs and return stale clipboard data; ensure the cache
is cleared at the end of a single formatting flow by resetting
this.resolvedClipboardContent = undefined (or null) when the flow completes (for
example in the finalization path of formatFileContent or in the same place that
calls clearDestinationContext()), or scope the cache to a per-run token and
invalidate it when the run finishes so getClipboardContent always reads fresh
clipboard content for each formatFileContent invocation.
In `@src/utilityObsidian.ts`:
- Around line 831-836: absolutePathToFileUrl currently interpolates raw paths
into a file:// string which breaks on characters like #, ?, or %; update
absolutePathToFileUrl to percent-encode the path by building a URL object and
assigning the normalized forward-slash path to URL.pathname (e.g. compute
forwardSlashPath and prefix a leading "/" if needed, then set url.pathname =
that value) and return the URL string (url.toString() or url.href) so the
produced file:// URL is properly encoded before any fetch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8439a5d-771c-4326-8e0f-c37d33b84d5e
📒 Files selected for processing (3)
src/formatters/completeFormatter.clipboard.test.tssrc/formatters/completeFormatter.tssrc/utilityObsidian.ts
Comment on lines
415
to
+432
| protected async getClipboardContent(): Promise<string> { | ||
| if (this.formattingFileContent && this.destinationSourcePath !== null) { | ||
| if ( | ||
| this.resolvedClipboardContent && | ||
| this.resolvedClipboardContent.destinationSourcePath === | ||
| this.destinationSourcePath | ||
| ) { | ||
| return this.resolvedClipboardContent.value; | ||
| } | ||
|
|
||
| const resolvedContent = await resolveClipboardForNoteContent( | ||
| this.app, | ||
| this.destinationSourcePath, | ||
| ); | ||
| this.resolvedClipboardContent = { | ||
| destinationSourcePath: this.destinationSourcePath, | ||
| value: resolvedContent, | ||
| }; |
There was a problem hiding this comment.
Scope the clipboard cache to a single formatting flow.
resolvedClipboardContent is only cleared when the destination changes or clearDestinationContext() is called. In the provided src/engine/TemplateEngine.ts snippets, the destination context is set before formatFileContent() but never cleared afterward, so a later run against the same note can reuse stale clipboard text/embed instead of rereading the clipboard. Please reset this cache at the end of the active flow, or clear the destination context from the callers’ finally blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/formatters/completeFormatter.ts` around lines 415 - 432, The clipboard
cache (resolvedClipboardContent) can persist across formatting runs and return
stale clipboard data; ensure the cache is cleared at the end of a single
formatting flow by resetting this.resolvedClipboardContent = undefined (or null)
when the flow completes (for example in the finalization path of
formatFileContent or in the same place that calls clearDestinationContext()), or
scope the cache to a per-run token and invalidate it when the run finishes so
getClipboardContent always reads fresh clipboard content for each
formatFileContent invocation.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Encode each path segment with encodeURIComponent to handle special characters like # and ? that would otherwise be interpreted as URL fragment/query delimiters. Co-Authored-By: Christian <christian@bagerbach.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add clipboard image support to note-content
{{clipboard}}rendering.This keeps
{{clipboard}}text-based by default, but when QuickAdd is formatting note content with a destination note context it now detects clipboard images, saves them as Obsidian attachments, and inserts embeds. The resolver also handles desktop clipboard payloads that arrive as local image-file paths, which was necessary to support the live Obsidian clipboard behavior reproduced from issue #1161.Changes addressing review comments
Alternative approach for local image file reading (
src/utilityObsidian.ts):window.require("fs")withfetch("file://...")for reading local clipboard image pathsuint8ArrayToArrayBufferhelper (no longer needed —fetchreturnsArrayBufferdirectly)absolutePathToFileUrlhelper for cross-platform path→URL conversionfsdependency, verbose type casting, and fragilewindow.requiredetectionSimplified clipboard handling (
src/formatters/completeFormatter.ts):clipboardContentScopeDepthcounter with a simpleformattingFileContentboolean flagwithClipboardContentScopewrapper methodformatFileContentnow directly sets/unsets the flag in a try/finally blockresolvedClipboardContenttype (removed null union ondestinationSourcePath)Tests (
src/formatters/completeFormatter.clipboard.test.ts):setWindowRequirehelper withmockFetchForFilethat mocksglobalThis.fetchfetchcalls withfile://URLsFixes #1161
Review & Testing Checklist for Human
fetch("file://...")works in Obsidian's Electron runtime — test with a clipboard manager (e.g. Clop) that puts local image paths in the clipboard. Iffetchis blocked by Electron security policy, the function gracefully returnsnulland falls back to plain text, but the image import feature would be degraded.{{clipboard}}, verify the attachment is created and embed inserted.formatFileNamewith{{clipboard}}still returns plain text (not image embeds) even when clipboard contains an image.Notes
formattingFileContentboolean replaces the depth counter. NestedformatFileContentcalls are not expected in practice, so a boolean suffices. If nesting is ever needed, it can be upgraded back to a counter.Link to Devin session: https://app.devin.ai/sessions/d2e18cb56396423ca07600dfd2facff9
Requested by: @chhoumann
Summary by CodeRabbit
New Features
Tests