feat: add attachment previews and file type icons#153
Open
IntoTheBlueAt wants to merge 4 commits into
Open
Conversation
Add image onError fallback UI, 10MB size limit for text file preview, and wire up AttachmentKind::File for unknown extensions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults 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 attachment support across backend and frontend: Rust Tauri commands to list/read attachments and updated watcher events; frontend types, services, context, folder-tree, lists, icons, and a FilePreview component to list, select, and preview images, PDFs, text, and generic files. ChangesAttachment Support Integration
Sequence DiagramsequenceDiagram
participant User as User
participant Frontend as Frontend (React)
participant Context as NotesContext
participant Services as Services (notes/files)
participant Backend as Tauri (Rust)
participant FS as Filesystem
User->>Frontend: click attachment
Frontend->>Context: selectAttachment(attachment)
Context->>Frontend: selectedAttachment updated
Frontend->>Services: readTextFile(path) [if text]
Services->>Backend: invoke("read_text_attachment", path)
Backend->>FS: canonicalize & validate path, check ext & size, read file
FS-->>Backend: file contents / error
Backend-->>Services: return contents or error
Services-->>Frontend: deliver text or error
Frontend-->>User: render FilePreview (image/pdf/text/file)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/services/files.ts (1)
21-23: 💤 Low valueConsider naming the helper to match its actual contract.
readTextFilereads as a generic "any text file" helper, but it invokesread_text_attachment, which only succeeds for paths inside the notes folder with a specific extension allowlist. Renaming toreadTextAttachmentwould prevent future callers from misusing it for non-attachment paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/files.ts` around lines 21 - 23, The helper readTextFile is misleading because it always calls invoke("read_text_attachment") which only works for attachment paths in the notes folder; rename the function to readTextAttachment to match its contract (change function name readTextFile to readTextAttachment and update all callers/imports accordingly) and keep the invoke("read_text_attachment", { path }) call unchanged so callers cannot mistakenly use it for arbitrary text files.src-tauri/src/lib.rs (2)
2029-2072: 💤 Low value
read_text_attachmentLGTM.Canonicalization on both
folder_rootandfile_pathplus astarts_withcontainment check is the right way to defeat traversal/symlink escapes. The extension allowlist matches theTextarm ofattachment_kind_from_extension, and the 10 MB cap is a sensible guardrail before reading into memory.One small suggestion: since the allowlist must stay in sync with the
Textarm inattachment_kind_from_extension, consider extracting it into a sharedconst TEXT_EXTENSIONS: &[&str]to avoid drift over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/lib.rs` around lines 2029 - 2072, Extract the text-file extension allowlist into a shared constant (e.g. const TEXT_EXTENSIONS: &[&str]) and replace the hard-coded list in read_text_attachment (function read_text_attachment) with a lookup against that constant; then update attachment_kind_from_extension to use the same TEXT_EXTENSIONS constant for its Text arm so both places share the single source of truth, ensuring the constant is exported/visible to both modules if needed and preserving the current case-insensitive comparison (to_ascii_lowercase or compare case-insensitively).
695-704: ⚡ Quick win
attachment_kind_from_extensionalways returnsSome, makingOptionmisleading and overly broad in the watcher.The
_ => Some(AttachmentKind::File)arm means this function returnsNoneonly when the extension can't be matched against the lowercase string conversion (which never fails). As a result:
- The
?on line 726 inattachment_from_abs_pathis effectively unreachable.- In the file watcher (line 2338-2343),
is_attachmentbecomestruefor any file in the notes folder with any extension (e.g..DS_Store,.swp, editor lock files, partial download tempfiles), spammingfile-changeevents withchanged_pathsand triggering frontendlistAttachments()refetches. The 500 ms debounce reduces but doesn't eliminate this.Either change the return type to
AttachmentKind(and gate "noise" extensions elsewhere), or restrict the catch-all to a known list of "generic" extensions worth surfacing as attachments.♻️ Suggested approach
-fn attachment_kind_from_extension(ext: &str) -> Option<AttachmentKind> { - match ext.to_ascii_lowercase().as_str() { - "png" | "jpg" | "jpeg" | "gif" | "webp" | "bmp" | "svg" => Some(AttachmentKind::Image), - "pdf" => Some(AttachmentKind::Pdf), - "txt" | "text" | "log" | "csv" | "tsv" | "json" | "xml" | "yaml" | "yml" => { - Some(AttachmentKind::Text) - } - _ => Some(AttachmentKind::File), - } -} +fn attachment_kind_from_extension(ext: &str) -> AttachmentKind { + match ext.to_ascii_lowercase().as_str() { + "png" | "jpg" | "jpeg" | "gif" | "webp" | "bmp" | "svg" => AttachmentKind::Image, + "pdf" => AttachmentKind::Pdf, + "txt" | "text" | "log" | "csv" | "tsv" | "json" | "xml" | "yaml" | "yml" => { + AttachmentKind::Text + } + _ => AttachmentKind::File, + } +}Then in the watcher, gate by an explicit "should treat as attachment" rule (e.g. ignore dotfiles or a small set of known-noise extensions) rather than relying on
Option.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/lib.rs` around lines 695 - 704, attachment_kind_from_extension currently always returns Some(AttachmentKind) because of the catch-all arm, which makes Option misleading and causes attachment_from_abs_path's use of ? to be unreachable and the watcher to treat every file as an attachment; change attachment_kind_from_extension to return None for unknown/noise extensions (or change its signature to return AttachmentKind if you intend every extension to be treated) and explicitly exclude noise patterns (dotfiles like .DS_Store, swap/temp files like *.swp, partial download suffixes) before mapping to AttachmentKind; update the watcher logic that sets is_attachment (and attachment_from_abs_path usage) to rely on the Option returned (treat None as not an attachment) or to call a new should_treat_as_attachment predicate that filters dotfiles and a small whitelist of generic extensions.src/components/preview/FilePreview.tsx (1)
80-87: ⚡ Quick winUse
navigator.clipboard.writeTextinstead of thecopy_to_clipboardTauri command.
attachment.pathis already available in the frontend, so a backend round-trip viainvoke("copy_to_clipboard", …)is unnecessary here.♻️ Suggested change
const handleCopyPath = useCallback(async () => { try { - await invoke("copy_to_clipboard", { text: attachment.path }); + await navigator.clipboard.writeText(attachment.path); toast.success("Copied filepath"); } catch { toast.error("Failed to copy filepath"); } }, [attachment.path]);If you remove the only remaining
invoke()call, the import on line 2 can be reduced toconvertFileSrconly.Based on learnings from PR
#103, prefernavigator.clipboard.writeText()in frontend components when the data to copy is already available on the frontend; reserve thecopy_to_clipboardTauri command for cases where the browser Clipboard API is unavailable or when copying data that originates from Rust-side operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/preview/FilePreview.tsx` around lines 80 - 87, The handleCopyPath function uses the Tauri invoke("copy_to_clipboard", ...) for data already available in the frontend; replace the invoke call with navigator.clipboard.writeText(attachment.path) inside the try block and keep the same toast.success/toast.error behavior, update the dependency array to still include attachment.path, and remove the now-unused import of invoke (leaving convertFileSrc import only) to clean up imports in FilePreview.tsx.src/context/NotesContext.tsx (1)
670-675: ⚡ Quick winWiden the
listenpayload generic instead of casting.The payload is now used for both notes and attachments, so add
changed_pathsto the generic to remove the inlineascast and keep the field statically typed.♻️ Proposed change
- listen<{ changed_ids: string[] }>("file-change", (event) => { + listen<{ changed_ids: string[]; changed_paths?: string[] }>("file-change", (event) => { // Don't process if effect was cleaned up if (isCancelled) return; const changedIds = event.payload.changed_ids || []; - const changedPaths = (event.payload as { changed_paths?: string[] }).changed_paths || []; + const changedPaths = event.payload.changed_paths || [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/context/NotesContext.tsx` around lines 670 - 675, The listen callback currently types the payload as { changed_ids: string[] } and then casts event.payload to access changed_paths; update the generic on listen to include changed_paths (e.g. listen<{ changed_ids: string[]; changed_paths?: string[] }>) so event.payload.changed_paths is statically typed and remove the inline cast; keep existing fallback/default behavior for empty arrays (changedIds and changedPaths) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/preview/FilePreview.tsx`:
- Around line 188-198: In FilePreview.tsx change the icon used in the
unknown-file-type fallback so it reflects non-image files: in the JSX branch
that checks attachment.kind === "file" (the block that renders the "No preview
is available for this file type." text and the Reveal File Button, and uses
handleReveal), replace the ImageIcon with FileIcon (which is already imported
elsewhere in this PR) so the visual matches the file-type fallback.
---
Nitpick comments:
In `@src-tauri/src/lib.rs`:
- Around line 2029-2072: Extract the text-file extension allowlist into a shared
constant (e.g. const TEXT_EXTENSIONS: &[&str]) and replace the hard-coded list
in read_text_attachment (function read_text_attachment) with a lookup against
that constant; then update attachment_kind_from_extension to use the same
TEXT_EXTENSIONS constant for its Text arm so both places share the single source
of truth, ensuring the constant is exported/visible to both modules if needed
and preserving the current case-insensitive comparison (to_ascii_lowercase or
compare case-insensitively).
- Around line 695-704: attachment_kind_from_extension currently always returns
Some(AttachmentKind) because of the catch-all arm, which makes Option misleading
and causes attachment_from_abs_path's use of ? to be unreachable and the watcher
to treat every file as an attachment; change attachment_kind_from_extension to
return None for unknown/noise extensions (or change its signature to return
AttachmentKind if you intend every extension to be treated) and explicitly
exclude noise patterns (dotfiles like .DS_Store, swap/temp files like *.swp,
partial download suffixes) before mapping to AttachmentKind; update the watcher
logic that sets is_attachment (and attachment_from_abs_path usage) to rely on
the Option returned (treat None as not an attachment) or to call a new
should_treat_as_attachment predicate that filters dotfiles and a small whitelist
of generic extensions.
In `@src/components/preview/FilePreview.tsx`:
- Around line 80-87: The handleCopyPath function uses the Tauri
invoke("copy_to_clipboard", ...) for data already available in the frontend;
replace the invoke call with navigator.clipboard.writeText(attachment.path)
inside the try block and keep the same toast.success/toast.error behavior,
update the dependency array to still include attachment.path, and remove the
now-unused import of invoke (leaving convertFileSrc import only) to clean up
imports in FilePreview.tsx.
In `@src/context/NotesContext.tsx`:
- Around line 670-675: The listen callback currently types the payload as {
changed_ids: string[] } and then casts event.payload to access changed_paths;
update the generic on listen to include changed_paths (e.g. listen<{
changed_ids: string[]; changed_paths?: string[] }>) so
event.payload.changed_paths is statically typed and remove the inline cast; keep
existing fallback/default behavior for empty arrays (changedIds and
changedPaths) unchanged.
In `@src/services/files.ts`:
- Around line 21-23: The helper readTextFile is misleading because it always
calls invoke("read_text_attachment") which only works for attachment paths in
the notes folder; rename the function to readTextAttachment to match its
contract (change function name readTextFile to readTextAttachment and update all
callers/imports accordingly) and keep the invoke("read_text_attachment", { path
}) call unchanged so callers cannot mistakenly use it for arbitrary text files.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aeec2bf2-f51d-40f2-af08-a1eaf5a8e4b6
📒 Files selected for processing (15)
src-tauri/src/lib.rssrc-tauri/tauri.conf.jsonsrc/App.tsxsrc/components/icons/index.tsxsrc/components/layout/Sidebar.tsxsrc/components/notes/FileTypeIcon.tsxsrc/components/notes/FolderTreeView.tsxsrc/components/notes/NoteList.tsxsrc/components/preview/FilePreview.tsxsrc/components/ui/index.tsxsrc/context/NotesContext.tsxsrc/lib/folderTree.tssrc/services/files.tssrc/services/notes.tssrc/types/note.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/preview/FilePreview.tsx`:
- Around line 61-63: Replace the toast that interpolates the raw error with a
generic, user-safe message while keeping the detailed error in console.error; in
FilePreview.tsx update the error handling block that calls console.error("Failed
to read text file:", error), toast.error(`Failed to read file: ${error}`), and
setTextContent("") so that console.error still logs the full error but
toast.error shows a stable message like "Failed to read file. Please try again."
and then call setTextContent("") as before.
- Around line 55-73: loadText can complete out-of-order and overwrite the active
preview; guard against stale results by tagging each load with a generation
token (e.g., a ref counter) or AbortController and only apply results when the
token matches the latest. Concretely: add a requestIdRef and increment it before
calling loadText (or inside useEffect), capture the current id inside loadText,
and before calling setTextContent / setImageError / setIsLoadingText ensure the
captured id === requestIdRef.current (or check !signal.aborted if using
AbortController) so only the most recent readTextFile result updates state;
update references to loadText, setTextContent, useEffect, and
attachment.modified accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20093149-db59-46fd-b509-8e319156c9f7
📒 Files selected for processing (1)
src/components/preview/FilePreview.tsx
… error toast Prevent out-of-order async loads from overwriting the active preview by tagging each load with a generation counter. Also replace the raw error interpolation in the toast with a generic user-safe message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.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
Closes #151
Changes
lib.rs): new Tauri commands for reading text attachments, opening file previews, importing files; attachment scanning integrated into existing folder/note listingFilePreview,FileTypeIconcomponents; updates toFolderTreeView,NoteList,NotesContext,AppAttachmentMetadata,AttachmentKindadded tonote.tsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI