feat(ui): implement Media Grabber dialog for media download options - #20
Merged
Conversation
Add MediaGrabberDialog modal in LinkGrabberView that opens when a media link badge is clicked. Allows selecting video quality, container format, audio-only mode, subtitle languages, and playlist items before download. Components: QualitySelector (radio grid), AudioOnlySection (switch+formats), SubtitleSelector (multi-checkbox), PlaylistSection (scroll list+select all), SizeEstimate (bitrate-based calc), MediaPreview (thumbnail+fallback). Install shadcn/ui: dialog, card, skeleton. Add MediaMetadata types in src/types/media.ts. Integrate via onMediaClick prop chain from LinkRow through ResolvedLinksSection to LinkGrabberView. Fixes from adversarial review: type-safe mutation (no cast), centralized formatBytes import, keyboard-accessible quality cards (role=radio), accessible media button (Button vs Badge), empty playlist guard, error state with retry button. 27 new tests, 242 total, 0 failures.
📝 WalkthroughWalkthroughAdded a Link Grabber media feature: UI components (Card/Dialog/Skeleton), MediaPreview, TypeScript media types, a Media Grabber modal with metadata-driven quality/format/subtitle/playlist controls, Tauri query/mutation wiring (metadata + download_start), and comprehensive tests for dialog and size estimation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LinkRow
participant LinkGrabberView
participant MediaGrabberDialog
participant useMediaMetadata
participant TauriBackend as Tauri Backend
User->>LinkRow: Click media button
LinkRow->>LinkGrabberView: onMediaClick(link)
LinkGrabberView->>LinkGrabberView: set selectedMediaLink, open dialog
LinkGrabberView->>MediaGrabberDialog: render with link & open=true
MediaGrabberDialog->>useMediaMetadata: useMediaMetadata(url, enabled)
useMediaMetadata->>TauriBackend: invoke("command_get_media_metadata", {url})
TauriBackend-->>useMediaMetadata: return MediaMetadata
useMediaMetadata-->>MediaGrabberDialog: metadata loaded
MediaGrabberDialog->>User: render options (quality/format/subtitles/playlist)
User->>MediaGrabberDialog: choose options, click Download
MediaGrabberDialog->>LinkGrabberView: onConfirm(options)
LinkGrabberView->>TauriBackend: invoke("download_start", { url, mediaOptions })
TauriBackend-->>LinkGrabberView: download initiated
LinkGrabberView->>MediaGrabberDialog: onOpenChange(false)
MediaGrabberDialog->>User: close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/views/LinkGrabberView/__tests__/SizeEstimate.test.tsx (1)
12-16: Add an assertion for the audio/video descriptor in the audio-only case.This suite already validates size math; adding the descriptor check will guard against regressions in the UI text branch.
✅ Test addition
it("should calculate correct size for audio_only", () => { render(<SizeEstimate quality="audio_only" format="m4a" duration={600} />); // 192 kbps * 1000 * 600 / 8 = 14_400_000 bytes ≈ 13.73 MB expect(screen.getByText(/Estimated Size: 13\.73 MB/)).toBeInTheDocument(); + expect(screen.getByText(/10m audio/)).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/__tests__/SizeEstimate.test.tsx` around lines 12 - 16, The test for SizeEstimate ("should calculate correct size for audio_only") only asserts the numeric size; add an assertion to also verify the audio/video descriptor is rendered for the audio-only case. In the same it block that renders <SizeEstimate quality="audio_only" format="m4a" duration={600} />, add an expect on the descriptor text (e.g. expect(screen.getByText(/audio only/i)).toBeInTheDocument() or a similar regex matching the UI string) so the test checks both size math and the "audio only" label from the SizeEstimate component.src/views/LinkGrabberView/MediaGrabberDialog/SubtitleSelector.tsx (1)
27-33: Use a deduped update path for subtitle codes.Appending directly can produce duplicate entries; using a
Setkeeps payload stable.Proposed refactor
<Checkbox id={`subtitle-${lang.code}`} checked={selected.includes(lang.code)} onCheckedChange={(checked) => { - if (checked) { - onSelect([...selected, lang.code]); - } else { - onSelect(selected.filter((c) => c !== lang.code)); - } + const next = new Set(selected); + if (checked === true) next.add(lang.code); + else next.delete(lang.code); + onSelect([...next]); }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/MediaGrabberDialog/SubtitleSelector.tsx` around lines 27 - 33, The onCheckedChange handler in SubtitleSelector.tsx may append duplicate lang.code values; replace the current append logic with a deduping update using a Set: inside the onCheckedChange callback, create a Set from the existing selected array, call set.add(lang.code) when checked and set.delete(lang.code) when unchecked, then call onSelect with Array.from(theSet) to produce a stable, deduped payload; keep the existing removal via delete rather than filter to ensure symmetric handling.src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx (1)
38-48: Expose selected audio format semantically for accessibility.Current selection is visual-only; add ARIA state (or a radio/toggle-group primitive) so screen readers can detect the active format.
Lightweight improvement
{audioFormats.map((fmt) => ( <Button key={fmt} variant={selectedFormat === fmt ? "default" : "outline"} size="sm" onClick={() => onSelectFormat(fmt)} + aria-pressed={selectedFormat === fmt} className="uppercase" > {fmt} </Button> ))}🤖 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 38 - 48, The audio format buttons in AudioOnlySection.tsx are only visually differentiated; make the selection semantic by exposing ARIA state: when rendering the list (audioFormats.map) set an accessible role (e.g., role="radiogroup" on the wrapper div) and on each Button include role="radio" and an aria-checked={selectedFormat === fmt} (or aria-pressed for toggle semantics) and ensure keyboard focus/activation calls onSelectFormat(fmt); update the Button props (keyed by fmt) to include these attributes so screen readers announce the active format.src/views/LinkGrabberView/LinkGrabberView.tsx (1)
142-149: Consider clearingselectedMediaLinkwhen dialog closes.Keeping
selectedMediaLinkset whileopen=falsekeeps the dialog mounted and can retain stale UI state across reopen cycles.Proposed refactor
{selectedMediaLink && ( <MediaGrabberDialog link={selectedMediaLink} open={mediaGrabberOpen} - onOpenChange={setMediaGrabberOpen} + onOpenChange={(open) => { + setMediaGrabberOpen(open); + if (!open) setSelectedMediaLink(null); + }} onConfirm={handleMediaGrabberConfirm} /> )}🤖 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 142 - 149, The dialog is kept mounted while selectedMediaLink remains truthy even after mediaGrabberOpen is set false, causing stale UI state; update the onOpenChange flow (or add a useEffect watching mediaGrabberOpen) so that when the MediaGrabberDialog closes (mediaGrabberOpen becomes false) you also clear selectedMediaLink (set it to null/undefined) — modify the setter usage around setMediaGrabberOpen or the handler that controls opening (where MediaGrabberDialog is rendered) to call the existing selectedMediaLink state setter and clear it on close; references: selectedMediaLink, mediaGrabberOpen, setMediaGrabberOpen, MediaGrabberDialog, handleMediaGrabberConfirm.src/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx (1)
241-289: Add a regression test for reopen/link-change state reset.Please add a test that changes selection values, closes/reopens the dialog (or swaps
link), then assertsDownloadsends values valid for the new metadata. This will protect against stale payload regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx` around lines 241 - 289, Add a regression test in MediaGrabberDialog.test.tsx that verifies state resets when the dialog is reopened or the `link` prop changes: render via the existing renderDialog helper, use mockInvoke to return initial mockMetadata, interact with controls (e.g., toggle the audio switch and change quality/subtitles), close the dialog by calling the component's close path (click Cancel or trigger onOpenChange), then update the mockInvoke to return a different metadata payload (e.g., mockMetadataAlt) and reopen or re-render the dialog with the new `link`; finally click the "Download" button and assert onConfirm was called with the fresh options matching the new metadata (reference symbols: renderDialog, mockInvoke, mockMetadata, onConfirm, onOpenChange, "Download" button, switch control).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 124: Update the changelog entry to use the correct accessible
terminology: change "badge" to "button" in the line referencing the integration
so it reads something like "Integration in LinkGrabberView via clickable media
button in LinkRow" (ensure the entry mentions LinkGrabberView and LinkRow and
that it's a media button to match the shipped implementation).
In `@src/components/MediaPreview.tsx`:
- Around line 9-23: The MediaPreview component currently keeps imgError true
after a failed load and never retries when the thumbnail prop changes; add a
useEffect that watches thumbnail and calls setImgError(false) to reset the error
state whenever a new thumbnail is provided so the <img> (handled in the
component where imgError, setImgError, thumbnail, title are defined) will
attempt to load the new URL.
In `@src/views/LinkGrabberView/LinkGrabberView.tsx`:
- Around line 42-45: The frontend is sending mediaOptions with the
download_start mutation (useTauriMutation startMediaDownload) but the backend
download_start handler and StartDownloadCommand struct don't accept it, so
mediaOptions get dropped; update the backend command signature and
StartDownloadCommand to include a mediaOptions (e.g., MediaGrabberOptions)
field, deserialize it, and propagate those options into the download flow, and
ensure the frontend type for startMediaDownload matches the updated backend
shape so the serialized payload includes url and mediaOptions.
In `@src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx`:
- Around line 77-83: The playlist UI renders even when metadata.playlistItems is
an empty array; update the render guard in MediaGrabberDialog so it checks both
metadata.isPlaylist and that metadata.playlistItems.length > 0 before returning
the <PlaylistSection> component (the block involving PlaylistSection,
selectedPlaylistItems and setSelectedPlaylistItems), ensuring the
PlaylistSection only mounts when there are actual items.
- Around line 34-57: The component initializes local option state
(qualitySelection, formatSelection, audioOnly, audioFormat, selectedSubtitles,
selectedPlaylistItems) with hardcoded defaults and never synchronizes or resets
them when metadata from useMediaMetadata(link.originalUrl, open) arrives or when
the dialog is reopened/link changes, causing stale/invalid onConfirm payloads;
update the component to derive initial selections from metadata when it becomes
available (and fallback to safe defaults), reset those states when open or
link.originalUrl changes (watching open and link.originalUrl in a useEffect),
ensure selections are constrained to
metadata.availableQualities/availableFormats/subtitles/playlistItems if present,
and use metadata-aware values when building the payload in handleConfirm so
qualitySelection/formatSelection/audioFormat/subtitle and playlist state always
reflect the current metadata.
In `@src/views/LinkGrabberView/MediaGrabberDialog/PlaylistSection.tsx`:
- Around line 47-63: The Checkbox rendered in PlaylistSection lacks an
accessible name; update the Checkbox component (the one using
checked={selectedItems.includes(item.id)} and onCheckedChange handlers) to
include an explicit aria-label or aria-labelledby that uniquely identifies the
item (for example use aria-label={`#${idx + 1} ${item.title}`} or reference a
span/p element by id to use aria-labelledby), so screen readers announce the
playlist index and title for each item (use item.id/item.title and idx to
construct the label).
In `@src/views/LinkGrabberView/MediaGrabberDialog/QualitySelector.tsx`:
- Around line 58-60: The "Container Format" text is rendered as a <label>
without an associated form control (inside QualitySelector where formats.map
renders options), so replace the orphan label with a non-label text element
(e.g., span or div) and link the option group semantically by wrapping the
options container (the div currently with className="flex gap-2") in an
ARIA/grouping element: either use a <fieldset> with a <legend> or give the
container role="group" and an id and set aria-labelledby to that id; update the
text node "Container Format" and the container attributes accordingly to
maintain accessibility.
In `@src/views/LinkGrabberView/MediaGrabberDialog/SizeEstimate.tsx`:
- Around line 29-30: The label always prints "video" even for audio-only items;
update the JSX in SizeEstimate.tsx to render "audio" when the selection is
audio-only by checking the relevant flag/format (e.g., inspect the format
variable or an audio_only prop) and otherwise render "video". Locate the
expression that outputs "{quality} {format.toUpperCase()} • {Math.round(duration
/ 60)}m video" and replace the trailing literal "video" with a conditional that
chooses "audio" when format (or audio_only) indicates audio-only and "video"
otherwise so the descriptor reflects the actual media type.
---
Nitpick comments:
In `@src/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx`:
- Around line 241-289: Add a regression test in MediaGrabberDialog.test.tsx that
verifies state resets when the dialog is reopened or the `link` prop changes:
render via the existing renderDialog helper, use mockInvoke to return initial
mockMetadata, interact with controls (e.g., toggle the audio switch and change
quality/subtitles), close the dialog by calling the component's close path
(click Cancel or trigger onOpenChange), then update the mockInvoke to return a
different metadata payload (e.g., mockMetadataAlt) and reopen or re-render the
dialog with the new `link`; finally click the "Download" button and assert
onConfirm was called with the fresh options matching the new metadata (reference
symbols: renderDialog, mockInvoke, mockMetadata, onConfirm, onOpenChange,
"Download" button, switch control).
In `@src/views/LinkGrabberView/__tests__/SizeEstimate.test.tsx`:
- Around line 12-16: The test for SizeEstimate ("should calculate correct size
for audio_only") only asserts the numeric size; add an assertion to also verify
the audio/video descriptor is rendered for the audio-only case. In the same it
block that renders <SizeEstimate quality="audio_only" format="m4a"
duration={600} />, add an expect on the descriptor text (e.g.
expect(screen.getByText(/audio only/i)).toBeInTheDocument() or a similar regex
matching the UI string) so the test checks both size math and the "audio only"
label from the SizeEstimate component.
In `@src/views/LinkGrabberView/LinkGrabberView.tsx`:
- Around line 142-149: The dialog is kept mounted while selectedMediaLink
remains truthy even after mediaGrabberOpen is set false, causing stale UI state;
update the onOpenChange flow (or add a useEffect watching mediaGrabberOpen) so
that when the MediaGrabberDialog closes (mediaGrabberOpen becomes false) you
also clear selectedMediaLink (set it to null/undefined) — modify the setter
usage around setMediaGrabberOpen or the handler that controls opening (where
MediaGrabberDialog is rendered) to call the existing selectedMediaLink state
setter and clear it on close; references: selectedMediaLink, mediaGrabberOpen,
setMediaGrabberOpen, MediaGrabberDialog, handleMediaGrabberConfirm.
In `@src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx`:
- Around line 38-48: The audio format buttons in AudioOnlySection.tsx are only
visually differentiated; make the selection semantic by exposing ARIA state:
when rendering the list (audioFormats.map) set an accessible role (e.g.,
role="radiogroup" on the wrapper div) and on each Button include role="radio"
and an aria-checked={selectedFormat === fmt} (or aria-pressed for toggle
semantics) and ensure keyboard focus/activation calls onSelectFormat(fmt);
update the Button props (keyed by fmt) to include these attributes so screen
readers announce the active format.
In `@src/views/LinkGrabberView/MediaGrabberDialog/SubtitleSelector.tsx`:
- Around line 27-33: The onCheckedChange handler in SubtitleSelector.tsx may
append duplicate lang.code values; replace the current append logic with a
deduping update using a Set: inside the onCheckedChange callback, create a Set
from the existing selected array, call set.add(lang.code) when checked and
set.delete(lang.code) when unchecked, then call onSelect with Array.from(theSet)
to produce a stable, deduped payload; keep the existing removal via delete
rather than filter to ensure symmetric handling.
🪄 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: dccdd68d-3419-4da0-b2f5-80301ed5fbfe
📒 Files selected for processing (20)
CHANGELOG.mdsrc/components/MediaPreview.tsxsrc/components/__tests__/MediaPreview.test.tsxsrc/components/ui/card.tsxsrc/components/ui/dialog.tsxsrc/components/ui/skeleton.tsxsrc/types/media.tssrc/views/LinkGrabberView/LinkGrabberView.tsxsrc/views/LinkGrabberView/LinkRow.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/PlaylistSection.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/QualitySelector.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/SizeEstimate.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/SubtitleSelector.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/index.tssrc/views/LinkGrabberView/MediaGrabberDialog/useMediaMetadata.tssrc/views/LinkGrabberView/ResolvedLinksSection.tsxsrc/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsxsrc/views/LinkGrabberView/__tests__/SizeEstimate.test.tsx
Comment on lines
+42
to
+45
| const { mutate: startMediaDownload } = useTauriMutation< | ||
| unknown, | ||
| { url: string; mediaOptions: MediaGrabberOptions } | ||
| >("download_start"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the Tauri command implementation(s)
rg -n --glob '*.rs' 'download_start|command_get_media_metadata|#\[tauri::command\]' -C3
# 2) Inspect structs/types used by download_start for request args
rg -n --glob '*.rs' 'struct .*Download.*(Args|Request|Command)|media_options|mediaOptions|playlist|subtitles|audio' -C4
# 3) Check serde rename strategy to confirm frontend key casing compatibility
rg -n --glob '*.rs' 'serde\(rename_all|rename\s*=|Deserialize|Serialize' -C3Repository: mpiton/vortex
Length of output: 32372
🏁 Script executed:
sed -n '36,52p' src-tauri/src/adapters/driving/tauri_ipc.rsRepository: mpiton/vortex
Length of output: 466
🏁 Script executed:
sed -n '26,31p' src-tauri/src/application/commands/mod.rsRepository: mpiton/vortex
Length of output: 209
Backend download_start command does not accept mediaOptions parameter.
The Tauri command signature at src-tauri/src/adapters/driving/tauri_ipc.rs:36-40 accepts only url and destination. The StartDownloadCommand struct has no field to carry media options. Any mediaOptions sent from the frontend will be silently dropped during deserialization, preventing the feature from functioning end-to-end. Update the backend signature and command struct to accept and handle media options before shipping this feature.
🤖 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 42 - 45, The
frontend is sending mediaOptions with the download_start mutation
(useTauriMutation startMediaDownload) but the backend download_start handler and
StartDownloadCommand struct don't accept it, so mediaOptions get dropped; update
the backend command signature and StartDownloadCommand to include a mediaOptions
(e.g., MediaGrabberOptions) field, deserialize it, and propagate those options
into the download flow, and ensure the frontend type for startMediaDownload
matches the updated backend shape so the serialized payload includes url and
mediaOptions.
Greptile SummaryThis PR adds a
Confidence Score: 4/5Two P1 logic bugs should be fixed before merging — state leaking across media links and an unconditional default quality that may not exist in backend-provided qualities. The new feature is well-structured and thoroughly tested, but the dialog state persistence bug means a user who opens multiple media links in one session will have their previous selections silently applied to subsequent downloads. The hardcoded quality default is a related issue that can produce invalid backend requests. Both are straightforward to fix and don't affect any other part of the codebase. src/views/LinkGrabberView/LinkGrabberView.tsx (missing key prop) and src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx (hardcoded quality/format defaults) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant LinkGrabberView
participant MediaGrabberDialog
participant useMediaMetadata
participant TauriBackend
User->>LinkGrabberView: Click media badge on LinkRow
LinkGrabberView->>LinkGrabberView: setSelectedMediaLink(link)
LinkGrabberView->>LinkGrabberView: setMediaGrabberOpen(true)
LinkGrabberView->>MediaGrabberDialog: render(link, open=true)
MediaGrabberDialog->>useMediaMetadata: fetch(url, enabled=true)
useMediaMetadata->>TauriBackend: invoke command_get_media_metadata
TauriBackend-->>useMediaMetadata: MediaMetadata
useMediaMetadata-->>MediaGrabberDialog: data, isLoading, isError
MediaGrabberDialog-->>User: Show quality/format/subtitle/playlist options
User->>MediaGrabberDialog: Click Download
MediaGrabberDialog->>LinkGrabberView: onConfirm(MediaGrabberOptions)
LinkGrabberView->>TauriBackend: startMediaDownload(url, mediaOptions)
MediaGrabberDialog->>LinkGrabberView: onOpenChange(false)
Reviews (1): Last reviewed commit: "feat(ui): implement Media Grabber dialog..." | Re-trigger Greptile |
Comment on lines
+34
to
+35
| const [qualitySelection, setQualitySelection] = useState("1080p"); | ||
| const [formatSelection, setFormatSelection] = useState("mp4"); |
There was a problem hiding this comment.
qualitySelection is initialised to "1080p" unconditionally, but the backend may return qualities that don't include "1080p" (e.g. a 480p-capped stream). When that happens no quality card renders with the selected ring style, and clicking Download silently sends quality: "1080p" — an option the backend never listed. The same issue affects the "mp4" default for formatSelection.
After metadata arrives, the selections should fall back to the first available option when the current default is absent:
// After the useMediaMetadata call
useEffect(() => {
if (!metadata) return;
if (!metadata.availableQualities.find((q) => q.quality === qualitySelection)) {
setQualitySelection(metadata.availableQualities[0]?.quality ?? qualitySelection);
}
if (!metadata.availableFormats.includes(formatSelection)) {
setFormatSelection(metadata.availableFormats[0] ?? formatSelection);
}
}, [metadata]);
Comment on lines
+142
to
+149
| {selectedMediaLink && ( | ||
| <MediaGrabberDialog | ||
| link={selectedMediaLink} | ||
| open={mediaGrabberOpen} | ||
| onOpenChange={setMediaGrabberOpen} | ||
| onConfirm={handleMediaGrabberConfirm} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
MediaGrabberDialog is never unmounted after first creation — selectedMediaLink is only ever set, never cleared. When the user opens link A (selects 720p), closes, then opens link B, React merely re-renders the existing dialog instance with new props; all useState values (qualitySelection, formatSelection, audioOnly, audioFormat, selectedSubtitles, selectedPlaylistItems) carry over from the previous session and silently apply to the new download.
The fix is to add a key prop derived from the link identity on MediaGrabberDialog in LinkGrabberView.tsx. React will then remount (and fully reset) the dialog whenever a different media link is selected.
Comment on lines
+9
to
+17
| const BITRATE_MAP: Record<string, number> = { | ||
| "360p": 500, | ||
| "480p": 1000, | ||
| "720p": 2500, | ||
| "1080p": 5000, | ||
| "1440p": 8000, | ||
| "4k": 15000, | ||
| audio_only: 192, | ||
| }; |
There was a problem hiding this comment.
QualityOption.bitrateKbps from the backend is already the real per-stream bitrate, yet SizeEstimate discards it and uses a static BITRATE_MAP. The map also misses any custom quality label the backend might return (e.g. "2160p" vs "4k"), silently falling back to the 2500 kbps default.
Passing the actual selected QualityOption.bitrateKbps down to this component would make the estimate accurate without any additional fetch cost.
There was a problem hiding this comment.
7 issues found across 20 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/components/MediaPreview.tsx">
<violation number="1" location="src/components/MediaPreview.tsx:9">
P2: Reset `imgError` when `thumbnail` changes; otherwise a single failed image load keeps this component stuck in fallback mode for subsequent thumbnails.</violation>
</file>
<file name="src/views/LinkGrabberView/MediaGrabberDialog/PlaylistSection.tsx">
<violation number="1" location="src/views/LinkGrabberView/MediaGrabberDialog/PlaylistSection.tsx:47">
P2: Add an accessible name to each playlist checkbox so screen readers can identify which item is being toggled.</violation>
</file>
<file name="src/views/LinkGrabberView/MediaGrabberDialog/QualitySelector.tsx">
<violation number="1" location="src/views/LinkGrabberView/MediaGrabberDialog/QualitySelector.tsx:32">
P2: All quality radios are tabbable; only the selected radio should be in the tab order for accessible radiogroup behavior.</violation>
</file>
<file name="src/views/LinkGrabberView/MediaGrabberDialog/SizeEstimate.tsx">
<violation number="1" location="src/views/LinkGrabberView/MediaGrabberDialog/SizeEstimate.tsx:20">
P2: Estimated size is derived from a hardcoded quality map instead of the selected stream’s actual `bitrateKbps`, which can produce significantly incorrect size estimates.</violation>
<violation number="2" location="src/views/LinkGrabberView/MediaGrabberDialog/SizeEstimate.tsx:29">
P3: Audio-only estimates are mislabeled as `video`, which shows incorrect download context in the summary text.</violation>
</file>
<file name="src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx">
<violation number="1" location="src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx:34">
P1: Default selections are hard-coded to specific formats/quality and can send invalid options for media that doesn't provide those values.</violation>
<violation number="2" location="src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx:38">
P2: Dialog selection state is not reset across link changes/reopens, so stale subtitles/playlist selections can be submitted for a different media item.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Reset imgError on thumbnail change in MediaPreview (useEffect) - Reset dialog state (quality/format/subs/playlist) on link change/reopen - Derive initial selections from metadata instead of hardcoded defaults - Clear selectedMediaLink when dialog closes (unmount stale state) - Guard PlaylistSection on playlistItems.length > 0 - SizeEstimate: use actual bitrateKbps from qualities, show "audio" label - QualitySelector: proper tab order (only selected radio in tab order), orphan label replaced with span+role=group for container format - AudioOnlySection: aria-pressed on format buttons, role=group wrapper - PlaylistSection: aria-label on checkboxes for screen readers - SubtitleSelector: Set-based dedup for language codes - CHANGELOG: "badge" → "button" to match shipped implementation - Add regression test for state reset on dialog reopen - Add test for actual bitrate from qualities prop
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx (1)
50-65:⚠️ Potential issue | 🟠 MajorReset video/audio format selections on reopen, and avoid unconditional re-initialization on metadata updates.
Lines 50-55 reset only part of dialog state.
qualitySelection,formatSelection, andaudioFormatcan carry over across reopen/link-change flows unlessmetadatachanges identity; and Lines 57-65 currently force-reset values whenever metadata updates.Suggested adjustment
useEffect(() => { if (!open) return; setAudioOnly(false); setSelectedSubtitles([]); setSelectedPlaylistItems([]); + setQualitySelection("1080p"); + setFormatSelection("mp4"); + setAudioFormat("m4a"); }, [open, link.originalUrl]); useEffect(() => { - if (!metadata) return; - const firstQuality = metadata.availableQualities[0]?.quality ?? "1080p"; - const firstFormat = metadata.availableFormats[0] ?? "mp4"; - const firstAudioFormat = metadata.availableAudioFormats[0] ?? "m4a"; - setQualitySelection(firstQuality); - setFormatSelection(firstFormat); - setAudioFormat(firstAudioFormat); - }, [metadata]); + if (!open || !metadata) return; + setQualitySelection((prev) => + metadata.availableQualities.some((q) => q.quality === prev) + ? prev + : (metadata.availableQualities[0]?.quality ?? "1080p"), + ); + setFormatSelection((prev) => + metadata.availableFormats.includes(prev) + ? prev + : (metadata.availableFormats[0] ?? "mp4"), + ); + setAudioFormat((prev) => + metadata.availableAudioFormats.includes(prev) + ? prev + : (metadata.availableAudioFormats[0] ?? "m4a"), + ); + }, [open, metadata]);🤖 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 50 - 65, The open-effect currently resets only some dialog state (setAudioOnly, setSelectedSubtitles, setSelectedPlaylistItems) but leaves qualitySelection/formatSelection/audioFormat to leak across reopens, while the metadata-effect unconditionally overwrites those selections on every metadata change; update the logic so that when the dialog opens (useEffect depending on open and link.originalUrl) you also reset qualitySelection, formatSelection and audioFormat to sensible defaults derived from metadata if available (or null/undefined if metadata not yet present), and change the metadata-effect (useEffect depending on metadata) to only initialize qualitySelection/formatSelection/audioFormat on first metadata arrival or when metadata identity truly changes (e.g., check for previous metadata or only set if selection values are currently undefined), rather than always resetting them. Ensure you reference the existing setters (setQualitySelection, setFormatSelection, setAudioFormat) and keep other resets (setAudioOnly, setSelectedSubtitles, setSelectedPlaylistItems) intact.
🧹 Nitpick comments (1)
src/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx (1)
368-452: Add a true close→reopen test path (same link) to match the scenario name.This test currently validates link-change reset while staying open. Consider adding a same-link
open: true → false → trueassertion so reopen-specific state reset is explicitly covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx` around lines 368 - 452, The test currently covers resetting when the link changes but not the close→reopen path; add a same-link close/reopen flow for MediaGrabberDialog to assert reopen-specific state reset: after the initial render and toggling the audio-only switch (user.click on getByRole("switch")), rerender the component with the same mockMediaLink but open={false} (to simulate close), then rerender again with open={true} (reopen) and ensure mockInvoke resolves the metadata for that link; wait for the title to appear, click the "Download" button, and assert onConfirm was called with audioOnly: false and the expected quality/format/subtitles/playlistItems—use the same symbols MediaGrabberDialog, mockMediaLink, mockInvoke, rerender, and onConfirm to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsx`:
- Around line 50-65: The open-effect currently resets only some dialog state
(setAudioOnly, setSelectedSubtitles, setSelectedPlaylistItems) but leaves
qualitySelection/formatSelection/audioFormat to leak across reopens, while the
metadata-effect unconditionally overwrites those selections on every metadata
change; update the logic so that when the dialog opens (useEffect depending on
open and link.originalUrl) you also reset qualitySelection, formatSelection and
audioFormat to sensible defaults derived from metadata if available (or
null/undefined if metadata not yet present), and change the metadata-effect
(useEffect depending on metadata) to only initialize
qualitySelection/formatSelection/audioFormat on first metadata arrival or when
metadata identity truly changes (e.g., check for previous metadata or only set
if selection values are currently undefined), rather than always resetting them.
Ensure you reference the existing setters (setQualitySelection,
setFormatSelection, setAudioFormat) and keep other resets (setAudioOnly,
setSelectedSubtitles, setSelectedPlaylistItems) intact.
---
Nitpick comments:
In `@src/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx`:
- Around line 368-452: The test currently covers resetting when the link changes
but not the close→reopen path; add a same-link close/reopen flow for
MediaGrabberDialog to assert reopen-specific state reset: after the initial
render and toggling the audio-only switch (user.click on getByRole("switch")),
rerender the component with the same mockMediaLink but open={false} (to simulate
close), then rerender again with open={true} (reopen) and ensure mockInvoke
resolves the metadata for that link; wait for the title to appear, click the
"Download" button, and assert onConfirm was called with audioOnly: false and the
expected quality/format/subtitles/playlistItems—use the same symbols
MediaGrabberDialog, mockMediaLink, mockInvoke, rerender, and onConfirm to locate
and implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea0be61e-c3ba-4f0c-bf45-642838bf06d5
📒 Files selected for processing (11)
CHANGELOG.mdsrc/components/MediaPreview.tsxsrc/views/LinkGrabberView/LinkGrabberView.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/PlaylistSection.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/QualitySelector.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/SizeEstimate.tsxsrc/views/LinkGrabberView/MediaGrabberDialog/SubtitleSelector.tsxsrc/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsxsrc/views/LinkGrabberView/__tests__/SizeEstimate.test.tsx
✅ Files skipped from review due to trivial changes (1)
- src/views/LinkGrabberView/tests/SizeEstimate.test.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/components/MediaPreview.tsx
- src/views/LinkGrabberView/LinkGrabberView.tsx
- src/views/LinkGrabberView/MediaGrabberDialog/AudioOnlySection.tsx
- src/views/LinkGrabberView/MediaGrabberDialog/SubtitleSelector.tsx
- src/views/LinkGrabberView/MediaGrabberDialog/SizeEstimate.tsx
- src/views/LinkGrabberView/MediaGrabberDialog/PlaylistSection.tsx
- src/views/LinkGrabberView/MediaGrabberDialog/QualitySelector.tsx
- CHANGELOG.md
This was referenced Apr 16, 2026
This was referenced Apr 30, 2026
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
MediaGrabberDialogmodal that opens when clicking a media link badge in Link GrabberuseMediaMetadatahook fetching metadata via Tauri IPConMediaClickprop chainAdversarial review fixes
startMediaDownloadmutation (noascast)formatBytesimport (removed duplicate from LinkRow)role="radio",tabIndex,onKeyDown)allSelectedwhenitems.length === 0)queryKeykept (required byuseTauriQuerytype signature)Test plan
tsc --noEmit)Summary by cubic
Adds a Media Grabber dialog to configure media downloads (quality, format, audio-only, subtitles, playlist items) with preview and more accurate size estimates. Opens from the media button in Link Grabber and starts downloads with the chosen options. Implements Linear Task 21.
New Features
useMediaMetadata; addsDialog,Card,Skeletonfromshadcn/ui.Bug Fixes
role="group"+aria-pressedon format buttons; labeled playlist checkboxes; media button replaces badge.formatBytes; type-safe download mutation.Written for commit 51c6ce9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests