Feature/add thumbnail support#133
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR introduces metadata exchange functionality to the file sharing system, enabling clients to fetch file metadata (name, size, thumbnail, MIME type) via a separate ALPN protocol channel, and adds platform-specific video thumbnail extraction capabilities with MIME-type-based routing across Windows, macOS, and Linux. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
🔥 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-app/src/hooks/useSender.ts (1)
51-63:⚠️ Potential issue | 🟠 MajorReset description when selection is cleared to prevent stale metadata reuse.
With persistent
fileDescriptionadded, clearing a selected file currently leaves the old description in state, so a later share can inherit unintended text.🧹 Proposed fix (outside this changed hunk)
const clearSelectedPath = () => { setSelectedPath(null) setPathType(null) + setFileDescription('') }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 321b976b-8648-4dce-a030-04d6ca75ea75
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
sendme/src/core/receive.rssendme/src/core/send.rssendme/src/core/types.rssendme/src/lib.rssrc-tauri/Cargo.tomlsrc-tauri/src/commands.rssrc-tauri/src/features/mod.rssrc-tauri/src/features/thumbnail/image.rssrc-tauri/src/features/thumbnail/mime.rssrc-tauri/src/features/thumbnail/mod.rssrc-tauri/src/features/thumbnail/video.rssrc-tauri/src/main.backup.final.rssrc-tauri/src/platform/mod.rssrc-tauri/src/platform/video_thumbnail/linux.rssrc-tauri/src/platform/video_thumbnail/macos.rssrc-tauri/src/platform/video_thumbnail/mod.rssrc-tauri/src/platform/video_thumbnail/windows.rssrc-tauri/src/utils/base64.rsweb-app/src/components/receiver/Receiver.tsxweb-app/src/components/receiver/TicketInput.tsxweb-app/src/components/sender/Sender.tsxweb-app/src/components/sender/ShareActionCard.tsxweb-app/src/hooks/useReceiver.tsweb-app/src/hooks/useSender.tsweb-app/src/lib/tauri.tsweb-app/src/locales/en/common.jsonweb-app/src/locales/zh-CN/common.jsonweb-app/src/store/sender-store.tsweb-app/src/types/receiver.tsweb-app/src/types/sender.tsweb-app/src/types/transfer.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
web-app/src/hooks/useReceiver.ts (1)
144-194:⚠️ Potential issue | 🟠 MajorInvalidate in-flight preview requests on receive/reset to prevent stale overwrites.
The sequence guard is good, but it is not invalidated when transitioning into receive/reset flows. A pending metadata request can still resolve and overwrite cleared preview state.
Suggested fix
useEffect(() => { + const seq = ++previewRequestSeqRef.current + if (isReceiving) { + setIsPreviewLoading(false) return } const trimmed = ticket.trim() if (!trimmed) { setPreviewMetadata(null) setIsPreviewLoading(false) return } - const seq = ++previewRequestSeqRef.current setIsPreviewLoading(true) @@ const handleReceive = async () => { if (!ticket.trim()) return try { setIsReceiving(true) @@ + previewRequestSeqRef.current += 1 setPreviewMetadata(null) + setIsPreviewLoading(false) folderOpenTriggeredRef.current = false @@ const resetForNewTransfer = async () => { + previewRequestSeqRef.current += 1 setIsReceiving(false) @@ setPreviewMetadata(null) setIsPreviewLoading(false)Also applies to: 398-399, 414-425
src-tauri/src/commands.rs (2)
93-100:⚠️ Potential issue | 🟠 MajorOffload metadata prep from async path and stop coercing size failures to
0.Line 99 currently masks metadata size failures, and the directory walk + thumbnail generation run synchronously in the async command path.
🛠️ Proposed fix
- let size = get_total_size(&path).unwrap_or(0); - let thumbnail = generate_thumbnail(&path); + let path_for_metadata = path.clone(); + let (size_opt, thumbnail) = tokio::task::spawn_blocking(move || { + (get_total_size(&path_for_metadata), generate_thumbnail(&path_for_metadata)) + }) + .await + .map_err(|e| format!("Failed to prepare metadata: {e}"))?; + let size = size_opt + .ok_or_else(|| format!("Failed to calculate size for {}", path.display()))?;#!/bin/bash # Verify whether blocking metadata calls and silent fallback are still present. rg -n --type=rust -C2 'get_total_size\(&path\)\.unwrap_or\(0\)|generate_thumbnail\(&path\)|spawn_blocking' src-tauri/src/commands.rs
120-126:⚠️ Potential issue | 🟠 MajorDo not log raw user description text.
Line 125 and Line 174 still emit
descriptioncontent directly, which can leak sensitive user-provided text.🔒 Safer logging pattern
tracing::info!( path = %path.display(), file_name = %metadata.file_name, size = metadata.size, has_thumbnail = metadata.thumbnail.is_some(), - description = ?metadata.description, + has_description = metadata.description.as_ref().map(|d| !d.is_empty()).unwrap_or(false), "share metadata prepared" ); @@ tracing::info!( file_name = %metadata.file_name, size = metadata.size, has_thumbnail = metadata.thumbnail.is_some(), - description = ?metadata.description, + has_description = metadata.description.as_ref().map(|d| !d.is_empty()).unwrap_or(false), "fetch_ticket_metadata succeeded" );Also applies to: 170-175
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a3e9135-c349-403a-a866-cab92176085e
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src-tauri/Cargo.tomlsrc-tauri/src/commands.rsweb-app/src/hooks/useReceiver.tsweb-app/src/hooks/useSender.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src-tauri/src/commands.rs (1)
92-99:⚠️ Potential issue | 🟠 MajorMove metadata preparation off the async command path, and fail instead of publishing
0bytes.
get_total_size()walks the filesystem andgenerate_thumbnail()does synchronous media work. Running both inline here can stall the Tauri runtime, andunwrap_or(0)turns size failures into incorrect preview metadata. Prefer doing this work inspawn_blockingbefore the state mutex is held.Suggested fix
- let size = get_total_size(&path).unwrap_or(0); - let thumbnail = generate_thumbnail(&path); + let (size, thumbnail) = tokio::task::spawn_blocking({ + let path = path.clone(); + move || { + let size = get_total_size(&path) + .ok_or_else(|| format!("Failed to calculate size for {}", path.display()))?; + Ok::<_, String>((size, generate_thumbnail(&path))) + } + }) + .await + .map_err(|e| e.to_string())??;web-app/src/hooks/useSender.ts (1)
30-30: 🛠️ Refactor suggestion | 🟠 MajorKeep
handleFileSelect’s exported signature aligned with the returned function.The implementation still accepts an optional
pathTypeand returns aPromise. Narrowing the public type to(path: string) => voidhides that capability and forces an extracheck_path_typeround-trip when the caller already knows the type.Suggested fix
- handleFileSelect: (path: string) => void + handleFileSelect: ( + path: string, + pathType?: 'file' | 'directory' + ) => Promise<void>sendme/src/core/send.rs (1)
59-70:⚠️ Potential issue | 🟡 MinorReject invalid metadata request markers.
The handler reads one byte but never checks its value, so any client currently receives metadata without speaking the intended protocol.
Suggested fix
let mut req = [0u8; 1]; tokio::time::timeout(Duration::from_secs(10), recv_stream.read_exact(&mut req)) .await .map_err(|_| { AcceptError::from_err(std::io::Error::new( ErrorKind::TimedOut, "metadata request read timeout", )) })? .map_err(AcceptError::from_err)?; + if req[0] != 1 { + return Err(AcceptError::from_err(std::io::Error::new( + ErrorKind::InvalidData, + format!("invalid metadata request marker: {}", req[0]), + ))); + } tracing::debug!("metadata request marker received");sendme/src/core/receive.rs (1)
351-362:⚠️ Potential issue | 🟡 MinorPlan a real third retry for direct-only tickets.
fetch_metadatasays it retries up to 3 times, butattempt_planonly has two default attempts. Tickets without relay addresses therefore lose the last retry entirely.Suggested fix
- let mut attempt_plan: Vec<(usize, &'static str, iroh::EndpointAddr)> = - vec![(1, "default", addr.clone()), (2, "default", addr.clone())]; + let mut attempt_plan: Vec<(usize, &'static str, iroh::EndpointAddr)> = vec![ + (1, "default", addr.clone()), + (2, "default", addr.clone()), + (3, "default", addr.clone()), + ]; @@ - if !relay_only_addr.addrs.is_empty() { - attempt_plan.push((3, "relay-only", relay_only_addr)); + if !relay_only_addr.addrs.is_empty() { + attempt_plan[2] = (3, "relay-only", relay_only_addr); }web-app/src/hooks/useReceiver.ts (1)
143-156:⚠️ Potential issue | 🟠 MajorInvalidate in-flight preview requests when the ticket is cleared or receive starts.
previewRequestSeqRefonly advances when a non-empty, non-receiving fetch begins. Once the timeout has fired, clearing the ticket, startinghandleReceive, or resetting the transfer leaves the old sequence active, so a stale response can still repopulatepreviewMetadataafter the UI has been cleared. Please bump the sequence and clearisPreviewLoadingon those transitions.Also applies to: 167-191, 389-397, 412-422
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 544d0835-d230-4bb8-ad9d-13cae0b43eb2
📒 Files selected for processing (11)
sendme/src/core/receive.rssendme/src/core/send.rssendme/src/core/types.rssrc-tauri/src/commands.rsweb-app/src/components/receiver/TicketInput.tsxweb-app/src/components/sender/Sender.tsxweb-app/src/hooks/useReceiver.tsweb-app/src/hooks/useSender.tsweb-app/src/store/sender-store.tsweb-app/src/types/sender.tsweb-app/src/types/transfer.ts
💤 Files with no reviewable changes (2)
- web-app/src/types/sender.ts
- web-app/src/components/sender/Sender.tsx
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-app/src/components/sender/DragDrop.tsx (1)
35-39: 🧹 Nitpick | 🔵 TrivialConsider memoizing
checkPathTypeto avoid effect re-runs.The
checkPathTypefunction is included in the dependency array, but since it's recreated on each render ofuseDragDrop, this effect may run more often than intended. If this causes issues, consider wrappingcheckPathTypeinuseCallbackwithin the hook.
♻️ Duplicate comments (1)
src-tauri/src/commands.rs (1)
305-323:⚠️ Potential issue | 🟠 MajorDon't return partial directory sizes as success.
WalkDirandentry.metadata()failures are skipped here, so unreadable descendants silently shrink thesizethat gets sent inFileMetadata. Please fail fast on the first traversal/stat error instead of returning a partial total.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f853b7a8-28d0-44bc-963f-76ee47ae2a0c
📒 Files selected for processing (21)
sendme/src/core/receive.rssendme/src/core/send.rssrc-tauri/Cargo.tomlsrc-tauri/src/commands.rssrc-tauri/src/features/thumbnail/image.rssrc-tauri/src/features/thumbnail/mod.rssrc-tauri/src/features/thumbnail/video.rssrc-tauri/src/lib.rssrc-tauri/src/platform/video_thumbnail/linux.rssrc-tauri/src/platform/video_thumbnail/macos.rssrc-tauri/src/platform/video_thumbnail/mod.rssrc-tauri/src/platform/video_thumbnail/windows.rsweb-app/src/components/icons.tsxweb-app/src/components/receiver/TicketInput.tsxweb-app/src/components/sender/DragDrop.tsxweb-app/src/components/ui/menu.tsxweb-app/src/hooks/useDragDrop.tsweb-app/src/hooks/useReceiver.tsweb-app/src/hooks/useSender.tsweb-app/src/locales/en/common.jsonweb-app/src/types/transfer.ts
…ct and merge tool functions
2 participants
Description
This PR implements File Preview metadata fetching through a dedicated metadata protocol channel, without increasing ticket length.
To keep tickets compact, preview metadata (thumbnail/description/mime) is not encoded into the ticket itself.
Instead, the receiver parses the existing ticket for address resolution, then opens a metadata-specific connection to fetch length-prefixed JSON metadata.
metadata Transport (Core)
sendme/metadata/1) for preview metadata exchange.[4-byte length prefix] [JSON payload]Tauri Commend Integration
fetch_ticket_metadatacommand now provides a direct metadata-only fetch path for frontend preview UIFile Preview UX (Web)
Testing
Checklist
npm run lintbefore raising this PRnpm run formatbefore raising this PR