feat(plugin): integrate vortex-mod-vimeo for end-to-end download#83
Merged
feat(plugin): integrate vortex-mod-vimeo for end-to-end download#83
Conversation
- Add extract_links/get_media_variants exports to PluginLoader trait with default NotFound implementations so existing loaders are unaffected - Wire command_get_media_metadata to prefer the Vimeo plugin path when resolve_url returns vortex-mod-vimeo; fall back to yt-dlp for all others - Parse [config] section in plugin.toml into config_defaults on PluginManifest and seed defaults into SharedHostResources so default_quality = 720p is honoured by the sandbox host functions at runtime - Filter and deduplicate video variants by height, sort descending; select first returned variant as the dialog default quality - MediaGrabberDialog respects metadata.defaultQuality with a fallback chain; AudioOnlySection forces audio-only mode when no video qualities are present - Unit tests: manifest config-defaults extraction, host-function seeding, Vimeo video metadata parsing, SoundCloud track/playlist parsing, frontend MediaGrabberDialog regression path (22 vitest tests green) Closes [MPI-100](/MPI/issues/MPI-100)
|
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:
📝 WalkthroughWalkthroughPlugin manifests now expose stringified config defaults which are seeded into the shared host config map during host initialization. Two plugin-facing loader methods ( Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (LinkGrabberView)
participant IPC as Tauri IPC
participant Loader as PluginLoader
participant Reg as PluginRegistry
participant Plugin as Extism Plugin
participant YTD as yt-dlp Fallback
FE->>IPC: command_get_media_metadata(url)
IPC->>Loader: extract_links(url)
Loader->>Reg: resolve plugin for url
Reg->>Plugin: call_plugin("extract_links", url)
Plugin-->>Reg: links / metadata JSON
Reg-->>Loader: plugin result
alt plugin returned metadata
Loader-->>IPC: parsed metadata (artist, default_quality)
else plugin missing/failed
IPC->>YTD: parse_ytdlp_json(url)
YTD-->>IPC: metadata
end
IPC-->>FE: MediaMetadataDto
sequenceDiagram
participant FE as Frontend (MediaGrabberDialog)
participant IPC as Tauri IPC
participant Loader as PluginLoader
participant Reg as PluginRegistry
participant Plugin as Extism Plugin
FE->>FE: detect mediaType=audio && missing qualities
FE->>FE: set requiresAudioOnly=true, disable quality selector
FE->>IPC: download_media_start(url, playlistItems?)
IPC->>Loader: extract_links(url) [if plugin handles playlists]
Loader->>Reg: resolve plugin for url
Reg->>Plugin: invoke extract_links
Plugin-->>Reg: playlist items
Reg-->>Loader: items
Loader-->>IPC: resolved targets
IPC->>IPC: start_media_download_for_url each target
IPC-->>FE: MediaDownloadResultDto { download_ids: [...] }
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)
Comment |
Greptile SummaryThis PR wires
Confidence Score: 4/5Safe to merge after resolving the SoundCloud track title P1; remaining findings are P2 style/polish items. One P1 logic issue: SoundCloud single-track metadata parser can return collection title instead of per-track title. All other findings are P2. Core Vimeo wiring, quality sorting, config-defaults seeding, and batch-download rollback are well-implemented and tested. src-tauri/src/adapters/driving/tauri_ipc.rs — specifically parse_soundcloud_metadata around line 1414.
|
| Filename | Overview |
|---|---|
| src-tauri/src/adapters/driving/tauri_ipc.rs | Core IPC layer wiring Vimeo/SoundCloud plugin paths; contains a P1 logic issue where extract_links.title shadows the per-track title for SoundCloud single-track responses, and two P2 concerns. |
| src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx | New audio-only component; works correctly but uses three hardcoded English strings instead of i18n t() calls. |
| src-tauri/src/adapters/driven/plugin/manifest.rs | Parses [config] defaults from plugin.toml; logic correct, well-tested. Minor: RawConfigEntry silently ignores unknown keys. |
| src-tauri/src/adapters/driven/plugin/capabilities.rs | Seeds config_defaults into SharedHostResources using or_insert_with; well-tested. |
| src-tauri/src/adapters/driven/plugin/extism_loader.rs | Adds extract_links/get_media_variants via call_url_plugin_function helper; clean with proper function_exists guard. |
| src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx | Respects metadata.defaultQuality with correct fallback chain; requiresAudioOnly and effectiveAudioOnly composition are correct. |
| src/types/media.ts | Adds artist, defaultQuality, downloadIds fields; clean. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx
Line: 34-40
Comment:
**Hardcoded English strings — missing i18n**
`"Audio Only"` (line 34) and `"Audio Format"` (line 40) are literal English strings. The rest of the dialog layer uses `useTranslation` + `t(...)` for every user-visible string. This component should follow the same pattern to keep the UI translatable.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx
Line: 56-58
Comment:
**Hardcoded format hint string also needs i18n**
The helper text "Popular formats: MP3 (wide compatibility), M4A (iTunes), OPUS (quality)" is also a hard-coded English string not going through `t(...)`, inconsistent with the rest of the view layer.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src-tauri/src/adapters/driving/tauri_ipc.rs
Line: 1350-1373
Comment:
**`Adaptive` variants with no `height` are silently discarded, yielding an empty quality list**
The `Adaptive` match arm pushes a quality entry only when `variant.height` is `Some` and non-zero. HLS/DASH manifests typically omit a fixed height, so an `Adaptive`-only plugin response produces empty `available_qualities` while `available_formats` is populated. A brief comment clarifying the contract would resolve the earlier thread concern.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src-tauri/src/adapters/driving/tauri_ipc.rs
Line: 1413-1415
Comment:
**SoundCloud track: `extract_links.title` shadows `track.title` for per-track title**
For a `"track"` response, `extract_links.title` is the top-level (collection/album) title, while `track.title` is the individual track title. Returning `extract_links.title` when present will use the collection name for a track fetched as part of a set, producing the wrong download filename.
```suggestion
title: track.title.clone(),
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src-tauri/src/adapters/driven/plugin/manifest.rs
Line: 37-40
Comment:
**`RawConfigEntry` accepts unknown fields silently**
A typo like `defaut = "720p"` would be silently swallowed, leaving the default unset. Adding a test that asserts `default` is captured even when other sibling keys exist would make the contract explicit.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src-tauri/src/adapters/driving/tauri_ipc.rs
Line: 1492-1494
Comment:
**Plugin-name string literal duplicated in batch-download path**
`"vortex-mod-soundcloud"` appears again here. Extracting it to a named constant shared with any other occurrence would reduce future maintenance risk.
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "fix: address PR review comments" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (4)
src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx (1)
30-33: Consider matching label cursor style to disabled state.When the switch is disabled,
cursor-pointeron the label is slightly misleading. A conditional class (e.g.,cursor-not-allowed) would better reflect interactivity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx` around lines 30 - 33, The label element for htmlFor="audio-only" in the AudioOnlySection component always uses "cursor-pointer"; update its className to be conditional based on the switch's disabled state (the prop/state that controls interactivity in AudioOnlySection) so it uses "cursor-not-allowed" when disabled and "cursor-pointer" when enabled, ensuring the label's cursor reflects interactivity.src-tauri/src/adapters/driven/plugin/manifest.rs (1)
143-153: Add direct tests for remainingencode_config_defaultbranches.
Integer,Float,Datetime, andTablebranches are implemented but not asserted yet. Adding focused unit tests would harden this serializer contract and prevent subtle regressions.Also applies to: 234-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/manifest.rs` around lines 143 - 153, Add focused unit tests for encode_config_default that exercise the remaining branches: construct toml::Value::Integer, toml::Value::Float, toml::Value::Datetime, and toml::Value::Table (and optionally Array) inputs, call encode_config_default, and assert the Result is Ok with the expected string (use i.to_string(), f.to_string(), dt.to_string(), and serde_json::to_string for Table/Array) and also assert the error mapping for invalid serialization where applicable; locate the function by name encode_config_default and add tests in the same module (or a tests module) to ensure those branches are covered.src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx (1)
66-80: Consider extracting the duplicated condition.The
shouldForceAudioOnlycondition (lines 66-69) duplicates therequiresAudioOnlycomputation (lines 51-54). SincerequiresAudioOnlyis already derived state, the effect could reference it directly.💡 Optional simplification
useEffect(() => { if (!metadata) return; - const shouldForceAudioOnly = - link.mediaType === "audio" && - metadata.availableQualities.length === 0 && - metadata.availableAudioFormats.length > 0; const firstQuality = metadata.defaultQuality ?? metadata.availableQualities[0]?.quality ?? "1080p"; const firstFormat = metadata.availableFormats[0] ?? "mp4"; const firstAudioFormat = metadata.availableAudioFormats[0] ?? - (shouldForceAudioOnly ? "mp3" : "m4a"); - setAudioOnly(shouldForceAudioOnly); + (requiresAudioOnly ? "mp3" : "m4a"); + setAudioOnly(requiresAudioOnly); setQualitySelection(firstQuality); setFormatSelection(firstFormat); setAudioFormat(firstAudioFormat); - }, [link.mediaType, metadata]); + }, [link.mediaType, metadata, requiresAudioOnly]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx` around lines 66 - 80, The effect is recomputing the same audio-only condition; replace the local shouldForceAudioOnly with the existing requiresAudioOnly variable (use requiresAudioOnly when setting audioOnly and when computing firstAudioFormat fallback), remove the duplicated boolean expression, and update the effect dependency array to include requiresAudioOnly (and keep metadata/link.mediaType as needed) so the effect runs when that derived state changes.src/views/LinkGrabberView/LinkGrabberView.tsx (1)
49-64: Consider typing the mutation return value for future extensibility.The backend now returns
MediaDownloadResultDto { download_ids: Vec<u64> }, but the mutation is typed asuseTauriMutation<unknown, ...>. While the return value is currently unused (only toast + navigate on success), typing it would enable future features like tracking batch downloads.💡 Optional improvement
+interface MediaDownloadResult { + downloadIds: number[]; +} + -const { mutate: startMediaDownload } = useTauriMutation< - unknown, +const { mutate: startMediaDownload } = useTauriMutation< + MediaDownloadResult, { url: string; quality: string; format: string; audioOnly: boolean; title?: string; playlistItems: string[]; } >("download_media_start", {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/LinkGrabberView.tsx` around lines 49 - 64, The mutation return type is currently unknown; update the useTauriMutation call in LinkGrabberView (the const { mutate: startMediaDownload } = useTauriMutation<...>("download_media_start", ...)) to use the backend DTO type (e.g., MediaDownloadResultDto with download_ids: number[]) instead of unknown, and either import that DTO type from your shared/backend types or declare a local interface matching the backend shape so future code can access the download_ids from startMediaDownload's result.
🤖 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-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 759-775: The batch loop over batch_targets calls
start_media_download_for_url and aborts on the first Err, leaving earlier
downloads started; change the loop in the block that constructs download_ids to
instead handle each start_media_download_for_url(await) with match so you
collect successful download IDs and failed item errors (e.g., collect Vec<Uuid>
download_ids and Vec<String> errors), then return a DTO that conveys partial
success (or return Ok when there are some download_ids and include errors via a
new field or log them) or, alternatively, on error cancel already-started
downloads by calling whatever cancellation API exists for the returned IDs;
update MediaDownloadResultDto (or create a new result type) and the loop around
start_media_download_for_url/state.command_bus/state.plugin_loader to reflect
this behavior.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Around line 143-153: Add focused unit tests for encode_config_default that
exercise the remaining branches: construct toml::Value::Integer,
toml::Value::Float, toml::Value::Datetime, and toml::Value::Table (and
optionally Array) inputs, call encode_config_default, and assert the Result is
Ok with the expected string (use i.to_string(), f.to_string(), dt.to_string(),
and serde_json::to_string for Table/Array) and also assert the error mapping for
invalid serialization where applicable; locate the function by name
encode_config_default and add tests in the same module (or a tests module) to
ensure those branches are covered.
In `@src/views/LinkGrabberView/LinkGrabberView.tsx`:
- Around line 49-64: The mutation return type is currently unknown; update the
useTauriMutation call in LinkGrabberView (the const { mutate: startMediaDownload
} = useTauriMutation<...>("download_media_start", ...)) to use the backend DTO
type (e.g., MediaDownloadResultDto with download_ids: number[]) instead of
unknown, and either import that DTO type from your shared/backend types or
declare a local interface matching the backend shape so future code can access
the download_ids from startMediaDownload's result.
In `@src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx`:
- Around line 30-33: The label element for htmlFor="audio-only" in the
AudioOnlySection component always uses "cursor-pointer"; update its className to
be conditional based on the switch's disabled state (the prop/state that
controls interactivity in AudioOnlySection) so it uses "cursor-not-allowed" when
disabled and "cursor-pointer" when enabled, ensuring the label's cursor reflects
interactivity.
In `@src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx`:
- Around line 66-80: The effect is recomputing the same audio-only condition;
replace the local shouldForceAudioOnly with the existing requiresAudioOnly
variable (use requiresAudioOnly when setting audioOnly and when computing
firstAudioFormat fallback), remove the duplicated boolean expression, and update
the effect dependency array to include requiresAudioOnly (and keep
metadata/link.mediaType as needed) so the effect runs when that derived state
changes.
🪄 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: 05aa8da9-7863-4720-ae10-ddcd07b737b1
📒 Files selected for processing (16)
src-tauri/src/adapters/driven/plugin/capabilities.rssrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driven/plugin/registry.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/command_bus.rssrc-tauri/src/application/commands/resolve_links.rssrc-tauri/src/domain/model/plugin.rssrc-tauri/src/domain/ports/driven/plugin_loader.rssrc-tauri/src/domain/ports/driven/tests.rssrc/components/MediaPreview.tsxsrc/types/media.tssrc/views/LinkGrabberView/LinkGrabberView.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsxsrc/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/adapters/driving/tauri_ipc.rs">
<violation number="1" location="src-tauri/src/adapters/driving/tauri_ipc.rs:1314">
P2: `Adaptive` variants are silently discarded. If a Vimeo video only provides HLS/DASH adaptive streams (no progressive `video` variants), both `available_qualities` and `available_formats` will be empty. Because `media_type` is still `"video"`, the `requiresAudioOnly` guard in `MediaGrabberDialog` evaluates to `false`, rendering a blank quality selector with no actionable options. Either handle adaptive variants here (e.g., surface them as selectable qualities) or propagate a signal so the frontend can show an appropriate fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
6 tasks
- Fix E0382 compile error in start_media_downloads: compute soundcloud track title before moving target.url into start_media_download_for_url - Pick default_quality from sorted qualities list so UI top entry matches the default (Greptile P2 — previously captured first variant pre-sort) - Log a warning when a plugin returns an Adaptive variant instead of silently dropping it (Greptile P2 — aids future debugging of HLS/DASH) - Update parse_plugin_video_metadata test to assert the highest quality (1080p) as default, matching the new ordering
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src-tauri/src/adapters/driving/tauri_ipc.rs (1)
759-774:⚠️ Potential issue | 🟠 MajorBatch download still fails non-atomically on first error.
Line 763–773 can start earlier downloads, then Line 772 (
await?) aborts on a later failure. This leaves partially-started background downloads while returning an error for the whole request.Suggested direction
if let Some(targets) = batch_targets { let mut download_ids = Vec::with_capacity(targets.len()); + let mut failures = Vec::new(); for target in targets { let title = Some(soundcloud_track_download_title(&target)); - let id = start_media_download_for_url( + match start_media_download_for_url( state.command_bus.clone(), state.plugin_loader.clone(), target.url, quality.clone(), format.clone(), true, title, ) - .await?; - download_ids.push(id); + .await { + Ok(id) => download_ids.push(id), + Err(err) => failures.push(err), + } } + if download_ids.is_empty() && !failures.is_empty() { + return Err(failures.join("; ")); + } + // optionally log `failures` for partial success visibility return Ok(MediaDownloadResultDto { download_ids }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driving/tauri_ipc.rs` around lines 759 - 774, The loop in handling batch_targets starts downloads one-by-one and immediately awaits start_media_download_for_url, so a later await? failure leaves earlier downloads running; fix by initiating all start requests without awaiting (e.g., collect futures for start_media_download_for_url for each target using soundcloud_track_download_title), then await them together with a fallible join (tokio::try_join! / futures::future::try_join_all) so the whole batch fails atomically; if you must spawn background downloads, record started download_ids and on any subsequent failure call the cancellation/stop path for each started id to roll back partial starts.
🤖 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-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 1135-1141: The code currently bubbles up errors from the plugin
metadata extraction (the tokio::task::spawn_blocking call that invokes
load_plugin_media_metadata with url_clone), which prevents the yt-dlp fallback;
change the error handling so plugin extraction failures are logged (or recorded)
but do not cause an early Err return—treat plugin errors as a None metadata
result and proceed to the fallback path instead; specifically, inside the block
around tokio::task::spawn_blocking(...).await, replace the two .map_err(...)?
chains that propagate errors with handling that logs the error (including
context: plugin loader and url_clone) and returns Ok(None) so the surrounding if
let Some(metadata) = ... can continue without aborting the whole flow.
---
Duplicate comments:
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 759-774: The loop in handling batch_targets starts downloads
one-by-one and immediately awaits start_media_download_for_url, so a later
await? failure leaves earlier downloads running; fix by initiating all start
requests without awaiting (e.g., collect futures for
start_media_download_for_url for each target using
soundcloud_track_download_title), then await them together with a fallible join
(tokio::try_join! / futures::future::try_join_all) so the whole batch fails
atomically; if you must spawn background downloads, record started download_ids
and on any subsequent failure call the cancellation/stop path for each started
id to roll back partial starts.
🪄 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: 099564f7-f10e-45dd-a854-fed89dca5b10
📒 Files selected for processing (1)
src-tauri/src/adapters/driving/tauri_ipc.rs
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx (1)
64-76: Consider addinglink.mediaTypeto the effect dependencies.The effect depends on
requiresAudioOnly, which is derived fromlink.mediaType. Iflinkchanges butmetadatadoesn't immediately update (e.g., cached), the audio-only state might not sync correctly. Addinglink.mediaTypeexplicitly would make the dependency clearer.However, this is likely a minor edge case since link changes typically trigger new metadata fetches.
💡 Suggested change
- }, [metadata, requiresAudioOnly]); + }, [metadata, requiresAudioOnly, link.mediaType]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx` around lines 64 - 76, The useEffect that initializes selection state (the effect using metadata, requiresAudioOnly, and calling setAudioOnly, setQualitySelection, setFormatSelection, setAudioFormat) should also include link.mediaType in its dependency array so changes to the link's media type update audioOnly immediately; update the dependency list for the useEffect to include link?.mediaType (or link.mediaType) alongside metadata and requiresAudioOnly to ensure the effect re-runs when the link's media type changes.src-tauri/src/adapters/driving/tauri_ipc.rs (2)
1016-1031: Consider cleaning up the temp file on subsequent errors.If
download_to_filesucceeds but a later step fails (e.g.,unique_destinationor the file move/copy), the temp file atfile_info.pathis left behind. While not critical, orphaned files in the temp directory could accumulate over time.💡 Suggested approach
Consider wrapping the post-download operations in a scope that removes the temp file on early return:
// After download_to_file succeeds, ensure cleanup on failure let cleanup_on_error = |path: &std::path::Path| { if let Err(e) = std::fs::remove_file(path) { tracing::debug!(path = %path.display(), error = %e, "temp file cleanup failed"); } }; // Then in error paths after line 1031: // cleanup_on_error(&file_info.path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driving/tauri_ipc.rs` around lines 1016 - 1031, After download_to_file succeeds, ensure the temporary file at file_info.path is removed on any subsequent error (e.g., failures from unique_destination or file move/copy); implement a small cleanup closure or RAII guard right after plugin_loader.download_to_file returns that calls std::fs::remove_file(&file_info.path) on error paths (logging failures with tracing::debug), and invoke that cleanup before returning Err in all post-download failure branches so the temp_dir/vortex-downloads files don't accumulate.
1502-1514: Edge case: non-matching selection IDs produce a user-unfriendly error.When
selected_item_idsis non-empty but none of the IDs match any track in the playlist, the error "no SoundCloud tracks matched the selected playlist items" is returned. This could confuse users if track IDs have changed since metadata was fetched (e.g., playlist was modified).Consider logging the mismatched IDs for debugging or providing a more informative error message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driving/tauri_ipc.rs` around lines 1502 - 1514, The error returned when no tracks match the selection should include which selected IDs were not found to aid debugging; compute the set of available track IDs from extract_links.tracks (e.g., collect track.id into available_ids), compute the difference missing_ids = selected_ids - available_ids, and if selected_item_ids was non-empty return or log an error that includes the missing_ids (or a friendlier message saying "selected items not found: [...]") instead of the generic message; update the block around selected_item_ids/selected_ids/tracks to include this missing-id-aware error and preferably log the missing IDs via the existing logger before returning the Err.
🤖 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-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 1016-1031: After download_to_file succeeds, ensure the temporary
file at file_info.path is removed on any subsequent error (e.g., failures from
unique_destination or file move/copy); implement a small cleanup closure or RAII
guard right after plugin_loader.download_to_file returns that calls
std::fs::remove_file(&file_info.path) on error paths (logging failures with
tracing::debug), and invoke that cleanup before returning Err in all
post-download failure branches so the temp_dir/vortex-downloads files don't
accumulate.
- Around line 1502-1514: The error returned when no tracks match the selection
should include which selected IDs were not found to aid debugging; compute the
set of available track IDs from extract_links.tracks (e.g., collect track.id
into available_ids), compute the difference missing_ids = selected_ids -
available_ids, and if selected_item_ids was non-empty return or log an error
that includes the missing_ids (or a friendlier message saying "selected items
not found: [...]") instead of the generic message; update the block around
selected_item_ids/selected_ids/tracks to include this missing-id-aware error and
preferably log the missing IDs via the existing logger before returning the Err.
In `@src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx`:
- Around line 64-76: The useEffect that initializes selection state (the effect
using metadata, requiresAudioOnly, and calling setAudioOnly,
setQualitySelection, setFormatSelection, setAudioFormat) should also include
link.mediaType in its dependency array so changes to the link's media type
update audioOnly immediately; update the dependency list for the useEffect to
include link?.mediaType (or link.mediaType) alongside metadata and
requiresAudioOnly to ensure the effect re-runs when the link's media type
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c13102c-2ef8-4186-af10-f8e8ce6c1074
📒 Files selected for processing (6)
src-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc/types/media.tssrc/views/LinkGrabberView/LinkGrabberView.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/types/media.ts
- src/views/LinkGrabberView/LinkGrabberView.tsx
- src-tauri/src/adapters/driven/plugin/manifest.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 1060-1074: AdaptiveStreamOnly handling currently calls
dirs::download_dir() (via unique_destination) instead of using the
app-configured download directory, causing adaptive streams to land in the
system default; update resolve_media_stream (or the callsite) to accept the
configured download directory from AppState and use that path when creating the
destination (instead of dirs::download_dir()), or alternatively change
resolve_media_stream to return StreamResolution::LocalFile with the temp path
and let the caller (which has AppState and invokes StartDownloadCommand) perform
the final placement using the configured download directory and
unique_destination; refer to functions/types resolve_media_stream,
AdaptiveStreamOnly, StreamResolution::LocalFile, StartDownloadCommand, and
unique_destination to locate the relevant code to change.
🪄 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: fc5dbc38-e513-4108-8c0a-ff36ec0e523b
📒 Files selected for processing (1)
src-tauri/src/adapters/driving/tauri_ipc.rs
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
vortex-mod-vimeoplugin (v1.1.0) end-to-end: URL detection viacan_handle, metadata viaextract_links/get_media_variants, quality ladder, and download path — all following the same hexagonal contract as the YouTube integration (PRs feat(download): YouTube 1080p+ via download_to_file DASH fallback #72, fix(download): race-free completion + final progress + source_hostname #73, refactor(ui): globalize sonner for consistent error feedback (#74) #76, fix(link): filter unsupported YouTube qualities from media metadata #79)[config]defaults fromplugin.tomlintoPluginManifest.config_defaults()and seeds them intoSharedHostResourcessodefault_quality = 720pis active in the sandbox at runtime; first-returned variant is selected as dialog defaultAudioOnlySectionforces audio-only mode when no video variants exist;MediaGrabberDialogrespectsmetadata.defaultQualitywith a fallback chainCloses MPI-100
Test plan
cargo fmt --all --check— green (verified locally)npm run typecheck— green (verified locally)npx vitest run .../MediaGrabberDialog.test.tsx— 22/22 green (verified locally)cargo test --workspace— delegated to CI (host lackslibwebkit2gtk-4.1-dev; requires the Linux deps from CI step)Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Summary by cubic
Integrates the
vortex-mod-vimeoplugin end-to-end with plugin-first metadata and variant lookup, and improves dialog defaults and audio-only UX. Adds safe-batch SoundCloud playlist downloads with rollback and seeds[config]defaults; merged downloads now honor the configured download directory. Closes MPI-100.New Features
can_handle+extract_links/get_media_variants, de-dupes/sorts qualities, and completes downloads with an Adaptive-merge fallback.[config]inplugin.tomlintoPluginManifest.config_defaults()and seed intoSharedHostResources(e.g.,default_quality = 720p).metadata.defaultQuality, disables and forces audio-only when no video variants exist, shows artist as a subtitle, and saves as “Artist - Title” (defaults to mp3 when audio-only).PluginLoaderaddsextract_links/get_media_variants; registry exposesfunction_exists;download_media_startacceptsplaylistItemsand returns{ downloadIds }; SoundCloud playlists batch-download with rollback on failure.download_dirif set.Bug Fixes
default_qualityfrom the sorted ladder so the UI default matches the top option.Written for commit c935cce. Summary will update on new commits.