feat(link): wire willReuseExisting in MediaGrabberDialog (#136)#138
Merged
Conversation
Add read-only IPC query `package_find_by_external_id` so the playlist
banner accurately reports "Reusing playlist package {name}" when a
playlist URL is re-resolved, instead of always falling back to "Will
create …".
Backend (CQRS read side):
- `PackageReadRepository::find_package_by_external_id` (port)
- SQLite impl filters on existing `idx_packages_external_id` UNIQUE
index, mirrors `find_package_by_id` SQL pattern
- New `FindPackageByExternalIdQuery` handler maps result to a minimal
`PackageSummaryDto { packageId, packageName }` (camelCase)
- Tauri IPC: `package_find_by_external_id` registered in lib.rs
Frontend:
- `PackageSummary` type + `usePackageByExternalId(externalId?)` hook
with `enabled: !!externalId` short-circuit
- `MediaGrabberDialog` calls the hook with `canonicalPlaylistKey(...)`
when `metadata.isPlaylist`, derives `willReuseExisting`, and prefers
the real `existingPackage.packageName` for the banner title
- Loading state defaults to "Will create" so the banner is never
blocked by the round-trip (AC #3)
Tests: 7 new Rust (DTO + handler + SQLite), 6 new TypeScript (hook +
dialog branches). cargo test 1337 passing, vitest 650 passing,
clippy + tsc + oxlint clean.
Closes #136
github-actions
Bot
added
documentation
📝 WalkthroughWalkthroughThis PR implements a read-side query system to detect whether a playlist package already exists by external ID, enabling the MediaGrabberDialog to display accurate reuse status upfront. It spans a complete vertical slice: SQLite lookup, CQRS query handler, Tauri IPC command, React hook, and component integration with corresponding tests. Changes
Sequence DiagramsequenceDiagram
participant React as Frontend<br/>(React Component)
participant Hook as usePackageByExternalId<br/>(Hook)
participant TauriAPI as Tauri IPC<br/>(Command Bus)
participant QueryBus as Query Bus<br/>(Handler)
participant Repo as SQLite Repo<br/>(Adapter)
participant DB as SQLite DB
React->>Hook: Call usePackageByExternalId(externalId)
alt externalId is defined
Hook->>TauriAPI: invoke("package_find_by_external_id", { externalId })
TauriAPI->>QueryBus: route to handler
QueryBus->>Repo: find_package_by_external_id(external_id)
Repo->>DB: SELECT * FROM packages WHERE external_id = ?
alt Match found
DB-->>Repo: PackageView
Repo-->>QueryBus: Ok(Some(PackageView))
QueryBus-->>TauriAPI: PackageSummaryDto { packageId, packageName }
TauriAPI-->>Hook: Ok(Some(PackageSummary))
else No match
DB-->>Repo: (empty)
Repo-->>QueryBus: Ok(None)
QueryBus-->>TauriAPI: Ok(None)
TauriAPI-->>Hook: Ok(null)
end
Hook-->>React: Return query result (data)
else externalId is undefined
Hook-->>React: Query disabled, return undefined
end
React->>React: willReuseExisting = !!existingPackage
React->>React: Render banner with reuse status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?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 (1)
src-tauri/src/application/test_support.rs (1)
676-683: ⚡ Quick winPreserve deterministic mapping semantics in
insert_with_external_id.At Line 679, inserting into
external_id_indexunconditionally overwrites prior mappings. The SQLite adapter resolves deterministically by earliest(created_at, id)ordering, so this fake can return a different package in duplicate-fixture scenarios and mask parity bugs in handler tests.♻️ Suggested patch
pub(crate) fn insert_with_external_id(&self, view: PackageView, external_id: &str) { let pkg_id = view.id.clone(); self.packages.lock().unwrap().push(view); - self.external_id_index - .lock() - .unwrap() - .insert(external_id.to_string(), pkg_id); + self.external_id_index + .lock() + .unwrap() + .entry(external_id.to_string()) + .or_insert(pkg_id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/test_support.rs` around lines 676 - 683, insert_with_external_id currently always overwrites any existing external_id -> pkg id mapping; change it to preserve the deterministic earliest (created_at, id) semantics: when inserting into external_id_index in insert_with_external_id, check if an entry already exists for external_id, and if so look up the existing PackageView by that pkg id from self.packages to compare (created_at, id) with the new view; only replace the mapping when the new view is strictly earlier by (created_at, id) ordering, otherwise keep the existing mapping; refer to insert_with_external_id, PackageView, self.packages, and external_id_index to locate and implement this logic.
🤖 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/hooks/usePackageByExternalId.ts`:
- Around line 4-13: The hook usePackageByExternalId sends a payload key
externalId that doesn't match the Tauri command parameter external_id; update
the object passed to useTauriQuery (the second argument of useTauriQuery in
usePackageByExternalId) to use the snake_case key external_id when externalId is
present so the Tauri invoke payload matches the Rust parameter name (leave
queryKey and enabled logic unchanged).
---
Nitpick comments:
In `@src-tauri/src/application/test_support.rs`:
- Around line 676-683: insert_with_external_id currently always overwrites any
existing external_id -> pkg id mapping; change it to preserve the deterministic
earliest (created_at, id) semantics: when inserting into external_id_index in
insert_with_external_id, check if an entry already exists for external_id, and
if so look up the existing PackageView by that pkg id from self.packages to
compare (created_at, id) with the new view; only replace the mapping when the
new view is strictly earlier by (created_at, id) ordering, otherwise keep the
existing mapping; refer to insert_with_external_id, PackageView, self.packages,
and external_id_index to locate and implement this logic.
🪄 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: 63881bd0-3ddd-4f79-966e-6461cabd6728
📒 Files selected for processing (15)
CHANGELOG.mdsrc-tauri/src/adapters/driven/sqlite/package_read_repo.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/queries/find_package_by_external_id.rssrc-tauri/src/application/queries/mod.rssrc-tauri/src/application/read_models/mod.rssrc-tauri/src/application/read_models/package_summary.rssrc-tauri/src/application/test_support.rssrc-tauri/src/domain/ports/driven/package_read_repository.rssrc-tauri/src/lib.rssrc/hooks/__tests__/usePackageByExternalId.test.tsxsrc/hooks/usePackageByExternalId.tssrc/types/package.tssrc/views/LinkGrabberView/MediaGrabberDialog/MediaGrabberDialog.tsxsrc/views/LinkGrabberView/__tests__/MediaGrabberDialog.test.tsx
Comment on lines
+4
to
+13
| export function usePackageByExternalId(externalId: string | undefined) { | ||
| return useTauriQuery<PackageSummary | null>( | ||
| "package_find_by_external_id", | ||
| externalId ? { externalId } : undefined, | ||
| { | ||
| queryKey: ["package_find_by_external_id", { externalId }], | ||
| enabled: !!externalId, | ||
| staleTime: 30_000, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'export function useTauriQuery|function useTauriQuery|const useTauriQuery|invoke\(' srcRepository: mpiton/vortex
Length of output: 519
🏁 Script executed:
# Get full implementation of useTauriQuery
cat -n src/api/hooks.ts | head -30Repository: mpiton/vortex
Length of output: 1512
🏁 Script executed:
# Find the Tauri command definition
rg -n 'package_find_by_external_id' src-tauri/src/ -A 5 -B 2Repository: mpiton/vortex
Length of output: 2077
🏁 Script executed:
# Check tauriInvoke to see how args are handled
rg -n 'export.*tauriInvoke|function tauriInvoke' src/api/ -A 10Repository: mpiton/vortex
Length of output: 567
🏁 Script executed:
# Look for other useTauriQuery calls to see the pattern of how args are structured
rg -n 'useTauriQuery' src/hooks/ -A 5 | head -50Repository: mpiton/vortex
Length of output: 1617
🏁 Script executed:
# Check if there are other IPC commands with underscores and how they're called
rg -n 'invoke\(' src/ -B 2 -A 1 | grep -E '(invoke|_|Id|id)' | head -30Repository: mpiton/vortex
Length of output: 39
🏁 Script executed:
# Find the download_detail Tauri command definition to see parameter naming
rg -n 'download_detail' src-tauri/src/adapters/driving/tauri_ipc.rs -B 2 -A 5 | head -30Repository: mpiton/vortex
Length of output: 1425
🏁 Script executed:
# Check if there's any serialization configuration or naming strategy in the Tauri setup
rg -n 'serde|serialize|rename' src-tauri/src/adapters/driving/tauri_ipc.rs | head -20Repository: mpiton/vortex
Length of output: 1011
🏁 Script executed:
# Check how Tauri invoke handles parameter naming with a concrete example
# Look for how other commands with underscored parameters are called
rg -n 'history_delete_entry|history_export|link_resolve|package_delete|package_set_password' src/hooks/ -A 3Repository: mpiton/vortex
Length of output: 39
🏁 Script executed:
# Double-check the exact signature of package_find_by_external_id
cat -n src-tauri/src/adapters/driving/tauri_ipc.rs | sed -n '3197,3210p'Repository: mpiton/vortex
Length of output: 579
🏁 Script executed:
# Check Tauri's invoke behavior and if there's any middleware for case conversion
# Search for any tauri configuration or plugins that might do case conversion
rg -n 'invoke|camelCase|snake_case|rename' src-tauri/src-tauri.conf.json src-tauri/Cargo.toml 2>/dev/null | head -20Repository: mpiton/vortex
Length of output: 39
🏁 Script executed:
# Search for examples of how other hooks call commands with underscored parameters
rg -n 'useTauriQuery|useTauriMutation' src/hooks/ -A 4 | grep -E '(useTauri|_|[a-z]+Id)' | head -40Repository: mpiton/vortex
Length of output: 1728
Ensure the payload key matches the Tauri command parameter name.
The hook sends { externalId } but the Tauri command in src-tauri/src/adapters/driving/tauri_ipc.rs:3200 expects the parameter external_id. Tauri's invoke forwards the object verbatim, and Rust deserialization will fail to map camelCase externalId to snake_case external_id, causing the query to error.
Required fix
- externalId ? { externalId } : undefined,
+ externalId ? { external_id: externalId } : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/usePackageByExternalId.ts` around lines 4 - 13, The hook
usePackageByExternalId sends a payload key externalId that doesn't match the
Tauri command parameter external_id; update the object passed to useTauriQuery
(the second argument of useTauriQuery in usePackageByExternalId) to use the
snake_case key external_id when externalId is present so the Tauri invoke
payload matches the Rust parameter name (leave queryKey and enabled logic
unchanged).
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
When a playlist URL is re-resolved in the Link Grabber, the
MediaGrabberDialognow predicts whether the app will reuse an existing package or create a new one. This makes thePlaylistPackageBannershow "Reusing playlist package {name}" instead of always falling back to "Will create …". Closes #136.Why
The backend
link_group_playlistscommand already implements the reuse logic (viaPackageRepository::find_by_external_id), but the frontend dialog has no way to know the outcome upfront. Adding a read-only query surfaces the prediction before the dialog is confirmed, making the UI more accurate and less surprising to users.Changes
Backend (CQRS read side):
PackageReadRepositorytrait: addfind_package_by_external_id(external_id) -> Result<Option<PackageView>, DomainError>idx_packages_external_idUNIQUE index, mirrorsfind_package_by_idSQL patternFindPackageByExternalIdQueryhandler inapplication/queries/maps result to minimalPackageSummaryDto { packageId, packageName }(camelCase for IPC)package_find_by_external_idregistered inlib.rsFrontend:
PackageSummary { packageId: string; packageName: string }insrc/types/package.tsusePackageByExternalId(externalId?)wrapsuseTauriQuerywithenabled: !!externalIdso no IPC call is made for non-playlist URLsMediaGrabberDialogcalls the hook withcanonicalPlaylistKey(link.originalUrl)whenmetadata.isPlaylist, deriveswillReuseExisting = !!existingPackage, and passes both the flag and the real package name toPlaylistPackageBannerTesting
InMemoryPackageReadRepomock, and SQLite adapter round-trippingexternal_idmatches and missesRelated Issues
MediaGrabberDialog: wirewillReuseExistingfrom apackage_find_by_external_idquery #136Notes for Reviewer
The
PackageReadRepository::find_package_by_external_idmethod reuses the same SQL column and index already wired for the write-sidePackageRepository::find_by_external_id, so there is no new schema burden. The hook'senabledguard means non-playlist dialogs incur zero IPC overhead — the query only fires when there is actually a playlist key to look up.Checklist
Summary by CodeRabbit