feat(ui): implement Link Grabber View with paste zone and link resolution - #19
Merged
Conversation
…tion Add full Link Grabber view replacing the placeholder: paste zone with URL extraction (http/https/ftp/magnet), drag & drop with container file detection (.dlc/.ccf/.rsdf), backend link_resolve IPC command via plugin infrastructure, resolved links display with status badges, filtering (all/online/offline/media), package grouping (none/hostname/extension/type), batch actions (start selected, start all online, clear), and clipboard monitoring toggle. Backend: ResolveLinksCommand + handler with URL validation, batch limit (500), hostname-based media detection, and sanitized error messages. Frontend: 8 components (LinkGrabberView, PasteZone, FilterBar, PackageGrouping, ActionsBar, LinkRow, ResolvedLinksSection, types) with 36 tests. Rust: 20 unit tests for resolve_links handler.
…test Radix Select uses hasPointerCapture, setPointerCapture, releasePointerCapture, and scrollIntoView which jsdom doesn't implement. Add beforeAll mocks to prevent uncaught exceptions during test runs.
|
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:
📝 WalkthroughWalkthroughAdds a new link-resolution backend command ( Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Frontend as React Frontend
participant TauriIPC as Tauri IPC
participant CommandBus as CommandBus
participant PluginLoader as PluginLoader
participant HTTPClient as HTTP Client
User->>Frontend: Paste/Drop URLs
Frontend->>Frontend: extractUrls & filter allowed schemes
Frontend->>TauriIPC: link_resolve(urls)
TauriIPC->>CommandBus: handle_resolve_links(command)
CommandBus->>CommandBus: enforce MAX_URLS, generate UUIDs
loop per URL
CommandBus->>PluginLoader: resolve_url(url)
PluginLoader-->>CommandBus: module_name / None
alt magnet
CommandBus-->>CommandBus: mark online, set resolved_url=original
else normal URL
CommandBus->>HTTPClient: HEAD request
HTTPClient-->>CommandBus: status + headers / error
CommandBus->>CommandBus: determine status, filename, size, media flag
end
end
CommandBus-->>TauriIPC: Vec<ResolvedLinkDto>
TauriIPC-->>Frontend: resolved links
Frontend->>Frontend: update state, render FilterBar/Grouping/Actions
Frontend-->>User: display grouped/filtered links and actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Greptile SummaryThis PR introduces a full Link Grabber UI view with 8 frontend components, a Rust
Confidence Score: 4/5Mostly solid, but two P1 correctness issues need fixing before merge: container files are silently discarded and magnet URLs always error in the backend despite being advertised as supported. Two P1 findings where advertised features produce no visible output (container files) or always-failing results (magnet URLs). The rest of the implementation — Rust URL validation, media detection, component decomposition, and the bulk of the test coverage — is clean and well-structured. Addressing the two P1s is straightforward and should not require architectural changes. src/views/LinkGrabberView/LinkGrabberView.tsx (container URL filter), src/views/LinkGrabberView/PasteZone.tsx (magnet URL extraction vs backend support) Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant PZ as PasteZone
participant LGV as LinkGrabberView
participant IPC as Tauri IPC (link_resolve)
participant BE as CommandBus / resolve_links.rs
participant HTTP as HTTP Client
U->>PZ: Paste text / drop file
PZ->>PZ: extractUrls() — regex for http/https/ftp/magnet
alt Container file dropped (.dlc/.ccf/…)
PZ->>LGV: onPasteUrls(["container:file.dlc"])
LGV->>LGV: handlePasteUrls — filter removes container:// ⚠️ silently discarded
else Text with URLs
PZ->>LGV: onPasteUrls([...urls])
LGV->>LGV: handlePasteUrls — keep http/https/ftp/magnet
LGV->>IPC: invoke("link_resolve", { urls })
IPC->>BE: ResolveLinksCommand { urls }
loop for each URL (sequential)
BE->>BE: is_allowed_scheme() — rejects magnet: ⚠️
BE->>HTTP: HEAD request
HTTP-->>BE: 200 OK / non-2xx / error
BE->>BE: extract filename + content-length
end
BE-->>IPC: Vec<ResolvedLinkDto>
IPC-->>LGV: ResolvedLink[]
LGV->>LGV: setResolvedLinks + setSelectedLinkIds([])
end
LGV->>U: Render FilterBar / PackageGrouping / ActionsBar / ResolvedLinksSection
Reviews (1): Last reviewed commit: "fix(ui): mock missing jsdom APIs for Rad..." | Re-trigger Greptile |
![greptile-apps[bot]](https://cdn.github.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+38
to
+48
| const validUrls = urls.filter( | ||
| (u) => | ||
| u.startsWith("http://") || | ||
| u.startsWith("https://") || | ||
| u.startsWith("ftp://") || | ||
| u.startsWith("magnet:?"), | ||
| ); | ||
| if (validUrls.length > 0) { | ||
| resolveLinks({ urls: validUrls }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
PasteZone.handleDrop generates container:${f.name} URIs for .dlc/.ccf/.rsdf/.metalink files and passes them to onPasteUrls. This filter rejects every URI that doesn't begin with http://, https://, ftp://, or magnet:?, so validUrls is empty, resolveLinks is never called, and the user receives no feedback that their container files were ignored. The PasteZone test even asserts that onPasteUrls is called with ["container:links.dlc"], confirming the contract exists — but the integration breaks here.
Until container processing is implemented on the backend, the user should at least receive a notification explaining why the drop was ignored.
Suggested change
| const validUrls = urls.filter( | |
| (u) => | |
| u.startsWith("http://") || | |
| u.startsWith("https://") || | |
| u.startsWith("ftp://") || | |
| u.startsWith("magnet:?"), | |
| ); | |
| if (validUrls.length > 0) { | |
| resolveLinks({ urls: validUrls }); | |
| } | |
| }; | |
| const handlePasteUrls = (urls: string[]) => { | |
| const containerUrls = urls.filter((u) => u.startsWith("container:")); | |
| const validUrls = urls.filter( | |
| (u) => | |
| u.startsWith("http://") || | |
| u.startsWith("https://") || | |
| u.startsWith("ftp://") || | |
| u.startsWith("magnet:?"), | |
| ); | |
| if (containerUrls.length > 0) { | |
| // TODO(task-22): forward container files to dedicated parser | |
| console.warn("Container files are not yet supported:", containerUrls); | |
| } | |
| if (validUrls.length > 0) { | |
| resolveLinks({ urls: validUrls }); | |
| } | |
| }; |
Comment on lines
+9
to
+12
| export function extractUrls(text: string): string[] { | ||
| const matches = text.match(/(https?:\/\/[^\s]+|ftp:\/\/[^\s]+|magnet:\?[^\s]+)/gi); | ||
| return matches ?? []; | ||
| } |
There was a problem hiding this comment.
The regex captures magnet:?… URIs and the parent's filter in handlePasteUrls deliberately passes them through (u.startsWith("magnet:?")). However is_allowed_scheme in resolve_links.rs only permits http://, https://, and ftp://, so every magnet link reaches the backend and returns status: "error" with error_message: "URL scheme not allowed". The feature is advertised in the PR description ("URL regex for http/https/ftp/magnet") but produces broken results end-to-end.
Either add magnet: to the backend's allowed schemes (if download-engine support exists), or remove magnet:? from the frontend regex until backend support lands, to avoid confusing users with an always-failing entry.
Comment on lines
+78
to
+82
| onCheckedChange={async (enabled) => { | ||
| await tauriInvoke("command_toggle_clipboard_monitoring", { | ||
| enabled, | ||
| }); | ||
| }} |
There was a problem hiding this comment.
command_toggle_clipboard_monitoring is not registered in lib.rs's invoke_handler! (intentionally deferred to task 22). When the switch is toggled, tauriInvoke(…) will reject, producing an unhandled promise rejection. While React will not crash, this surfaces as a console error and leaves the switch in an inconsistent visual state until the controlled value from the store snaps it back.
Wrapping the call in a no-op catch until task 22 lands keeps the toggle visually correct and the console clean:
Suggested change
| onCheckedChange={async (enabled) => { | |
| await tauriInvoke("command_toggle_clipboard_monitoring", { | |
| enabled, | |
| }); | |
| }} | |
| onCheckedChange={async (enabled) => { | |
| try { | |
| await tauriInvoke("command_toggle_clipboard_monitoring", { | |
| enabled, | |
| }); | |
| } catch { | |
| // TODO(task-22): command not yet implemented | |
| } | |
| }} |
Comment on lines
+10
to
+11
| const matches = text.match(/(https?:\/\/[^\s]+|ftp:\/\/[^\s]+|magnet:\?[^\s]+)/gi); | ||
| return matches ?? []; |
There was a problem hiding this comment.
[^\s]+ greedily consumes every non-whitespace character, so a URL ending a sentence will include trailing punctuation — e.g., "Download from https://example.com/file.zip." yields "https://example.com/file.zip." and "(https://example.com)" yields "https://example.com)". These malformed URLs will either fail the backend's scheme check or return an incorrect result.
A common fix is to strip common trailing punctuation characters after matching:
Suggested change
| const matches = text.match(/(https?:\/\/[^\s]+|ftp:\/\/[^\s]+|magnet:\?[^\s]+)/gi); | |
| return matches ?? []; | |
| const matches = text.match(/(https?:\/\/[^\s<>"]+|ftp:\/\/[^\s<>"]+|magnet:\?[^\s<>"]+)/gi); | |
| return (matches ?? []).map((u) => u.replace(/[.,;:!?)]+$/, "")); |
Comment on lines
+48
to
+126
| for url in &cmd.urls { | ||
| let id = Uuid::new_v4().to_string(); | ||
|
|
||
| if !is_allowed_scheme(url) { | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: None, | ||
| filename: None, | ||
| size_bytes: None, | ||
| status: "error".to_string(), | ||
| error_message: Some("URL scheme not allowed".to_string()), | ||
| module_name: "core-http".to_string(), | ||
| is_media: false, | ||
| media_type: None, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| let plugin_info = self.plugin_loader().resolve_url(url); | ||
| let module_name = match &plugin_info { | ||
| Ok(Some(info)) => info.name().to_string(), | ||
| _ => "core-http".to_string(), | ||
| }; | ||
|
|
||
| let is_media = is_media_url(url); | ||
| let media_type = if is_media { | ||
| detect_media_type(url) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| match self.http_client().head(url) { | ||
| Ok(response) if response.is_success() => { | ||
| let filename = extract_filename_from_url(url); | ||
| let size = extract_content_length(&response); | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: Some(url.clone()), | ||
| filename, | ||
| size_bytes: size, | ||
| status: "online".to_string(), | ||
| error_message: None, | ||
| module_name, | ||
| is_media, | ||
| media_type, | ||
| }); | ||
| } | ||
| Ok(_) => { | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: None, | ||
| filename: None, | ||
| size_bytes: None, | ||
| status: "offline".to_string(), | ||
| error_message: None, | ||
| module_name, | ||
| is_media, | ||
| media_type, | ||
| }); | ||
| } | ||
| Err(e) => { | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: None, | ||
| filename: None, | ||
| size_bytes: None, | ||
| status: "error".to_string(), | ||
| error_message: Some(e.to_string()), | ||
| module_name, | ||
| is_media, | ||
| media_type, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The loop issues one synchronous http_client().head(url) call per URL before moving to the next. For the allowed maximum of 500 URLs, each with a typical 200–500 ms round-trip time, this can block the Tauri async task for several minutes. Consider spawning tokio::task::spawn_blocking per request and collecting with futures::future::join_all, or use a semaphore-bounded parallel approach to cap concurrency without blocking the runtime.
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx (1)
1-3: Reset sharedinvokemock per test for isolation.The suite reuses a module-level mocked
invokewithout resetting between tests. AddbeforeEachto avoid cross-test coupling.Suggested fix
-import { describe, it, expect, vi } from "vitest"; +import { beforeEach, describe, it, expect, vi } from "vitest"; ... describe("LinkGrabberView", () => { + beforeEach(() => { + mockInvoke.mockReset(); + mockInvoke.mockResolvedValue([]); + });Also applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx` around lines 1 - 3, The tests in LinkGrabberView.test.tsx reuse a module-level mocked "invoke" which causes cross-test coupling; add a beforeEach block (e.g., beforeEach(() => { ... })) that resets the mock state (use vi.resetAllMocks() or vi.clearAllMocks() and re-mock invoke if needed) so the module-level invoke mock is cleared between tests and each test starts with a fresh mock.src/views/LinkGrabberView/__tests__/PasteZone.test.tsx (1)
71-73: Avoid class-based structural selector for the drop zone test.Lines 71–73 depend on a Tailwind class (
rounded-lg) and DOM shape, which is fragile. Prefer a stable test identifier.Suggested fix
- const dropZone = screen - .getByRole("textbox") - .closest("div.rounded-lg") as HTMLElement; + const dropZone = screen.getByTestId("paste-drop-zone");You’d also need to expose the test id in
src/views/LinkGrabberView/PasteZone.tsx, e.g.:<div data-testid="paste-drop-zone" ...>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/__tests__/PasteZone.test.tsx` around lines 71 - 73, Update the PasteZone component to add a stable test id (e.g., data-testid="paste-drop-zone") on the root drop-zone element in src/views/LinkGrabberView/PasteZone.tsx, then change the test in PasteZone.test.tsx to query the element by that test id (use screen.getByTestId("paste-drop-zone")) instead of using closest("div.rounded-lg"); reference the PasteZone component and the test's variable dropZone to locate where to add the attribute and update the selector.src/views/LinkGrabberView/PasteZone.tsx (1)
9-11: Strip trailing punctuation from extracted URLs.
[^\s]+also captures closing)/]/.from surrounding prose, so valid pasted links can be forwarded with extra characters and fail resolution.Possible fix
export function extractUrls(text: string): string[] { const matches = text.match(/(https?:\/\/[^\s]+|ftp:\/\/[^\s]+|magnet:\?[^\s]+)/gi); - return matches ?? []; + return (matches ?? []).map((url) => url.replace(/[),.\]}!?]+$/, "")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/PasteZone.tsx` around lines 9 - 11, extractUrls currently uses [^\s]+ and can include trailing punctuation like ).,]} which breaks link resolution; update the extractUrls implementation to post-process each regex match to strip trailing punctuation characters (e.g., .,;:>"') and closing brackets ) ] } > while preserving balanced parentheses: for a trailing ')' only remove it if the URL contains fewer '(' than ')' (i.e., unbalanced), otherwise keep it; apply this trimming to each match before returning the array so the function returns clean URLs.
🤖 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 300-303: The map_err closure in the link resolution call currently
logs the error then replaces every error with the generic "Failed to resolve
links" string; change it to preserve and return safe, user-facing validation
messages while still sanitizing internal errors: inside the map_err closure (the
one that calls tracing::error and currently returns "Failed to resolve
links".to_string()) match or inspect the incoming error `e` and if it is a known
validation/user-facing error (e.g., a URL batch limit or sanitization validation
error) return e.to_string() unmodified, otherwise log the full error with
tracing::error and return the generic "Failed to resolve links" message;
alternatively extract this logic into a small helper like
is_user_facing_error(_) or format_user_error(_) and use that from the same
closure so internal errors remain sanitized but validation messages are passed
through.
In `@src-tauri/src/application/commands/resolve_links.rs`:
- Around line 111-123: The ResolvedLinkDto construction in the error arm
currently assigns e.to_string() to error_message (in resolve_links.rs); instead
replace that raw backend error with a small user-safe message enum/string (e.g.,
"network error", "not found", "unsupported media", or "unknown error") by
matching on the error type returned by the resolver (or mapping known error
variants), set that safe message into error_message, and write the full e (e.g.,
with debug/trace) to backend logs instead of exposing it to the renderer; ensure
ResolvedLinkDto.error_message remains Option<String> and that the match covers
common error variants produced by the resolver functions used here.
- Around line 48-125: The loop in resolve_links.rs performs synchronous, serial
network calls via HttpClient::head inside the Tauri command handler (iterating
cmd.urls), which can block the command thread; change this to run HTTP checks on
a blocking/thread-pool or use bounded async concurrency with timeouts so slow
hosts don't stall the command: spawn tasks on a blocking pool (or use
tokio::spawn_blocking or a bounded FuturesUnordered with a semaphore) for each
URL check, call HttpClient::head from those worker tasks (or switch to an async
http client and await with timeout), collect results back into the same
ResolvedLinkDto structure, and ensure accesses to plugin_loader() and
http_client() are thread-safe (clone handles or call them inside the worker) and
that errors/timeouts are mapped to the same "offline"/"error" statuses.
- Around line 132-134: The current is_allowed_scheme rejects magnet: URIs
causing UI-accepted magnet links to always error; update is_allowed_scheme to
also accept "magnet:" (i.e., allow url.starts_with("magnet:")), and add a
magnet-specific resolution path in this module: detect magnet links during
resolution and route them to a new helper (e.g., resolve_magnet_link) that
returns the appropriate LinkResolution/Result or clear error for now, so magnets
are accepted by the validator and handled explicitly instead of being rejected.
In `@src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx`:
- Around line 45-55: The test currently clicks the combobox trigger but never
verifies the onModeChange callback; update the test in PackageGrouping.test.tsx
to actually change the selected value and assert onModeChange was called: after
obtaining trigger via screen.getByRole("combobox") (and using
userEvent.setup()), simulate a real selection change (e.g., open the combobox
then navigate and choose an option via keyboard with user.keyboard("ArrowDown
Enter") or locate an option by role/text and user.click it) and then
expect(onModeChange).toHaveBeenCalled() (or
.toHaveBeenCalledWith(expectedMode)); keep references to the existing symbols
userEvent.setup, trigger (combobox), and onModeChange to locate the code to
modify.
In `@src/views/LinkGrabberView/LinkGrabberView.tsx`:
- Around line 37-48: The paste handler handlePasteUrls is filtering out
container entries like "container:{filename}" (for .dlc/.ccf/.rsdf/.metalink)
and thus discarding the container-file flow; update handlePasteUrls to detect
and preserve container entries (e.g., strings starting with "container:") and
either pass them through to resolveLinks (resolveLinks({ urls: validUrls,
containers: [...] })) or call a dedicated resolver (e.g., resolveContainers or
resolveLinks with a flag) so container files are not dropped; ensure you
reference handlePasteUrls and resolveLinks (or add resolveContainers) when
adding this branch so the .dlc/.ccf/.rsdf/.metalink flow reaches the backend.
- Around line 19-21: The toggle is only reading clipboardMonitoringEnabled from
useSettingsStore but onCheckedChange only calls
command_toggle_clipboard_monitoring (which isn't implemented yet), so the UI
will snap back; fix by performing an optimistic update to the settings store
when the switch is toggled: in the onCheckedChange handler (the Switch control
around lines 75-83), immediately update the store's clipboard monitoring flag
via the useSettingsStore updater (flip config.clipboardMonitoring) so the switch
reflects the new state, then call command_toggle_clipboard_monitoring and, if it
rejects, revert the store change and surface an error; alternatively, if you
prefer to block until the backend exists, disable the Switch when
command_toggle_clipboard_monitoring is not available instead of relying on the
command—apply this change to the same onCheckedChange/checked usage that
references clipboardMonitoringEnabled.
In `@src/views/LinkGrabberView/LinkRow.tsx`:
- Around line 46-50: The TooltipTrigger currently wraps a non-focusable <p> in
LinkRow, preventing keyboard users from opening the tooltip; replace the <p>
used as the TooltipTrigger child with a keyboard-focusable element (e.g., a
<button type="button"> or a <span role="button" tabIndex={0}>) while preserving
the existing classes ("min-w-0 flex-1 truncate text-sm") and visual styling, so
TooltipTrigger (component TooltipTrigger) remains asChild but the trigger is
keyboard-focusable and screen-reader accessible.
In `@src/views/LinkGrabberView/PackageGrouping.tsx`:
- Around line 18-21: The visible label element ("Group Into Packages:") is not
associated with the combobox trigger (SelectTrigger), reducing accessibility;
update the markup so the label is programmatically connected to the
SelectTrigger used by the Select component: give SelectTrigger a unique id (or
aria-labelledby) and set the label's htmlFor (or create a matching id referenced
by aria-labelledby) so the label text is announced for the combobox; locate the
elements around the Select, SelectTrigger, and SelectValue (use symbols mode,
onModeChange, GroupingMode) and add the id/aria attribute pair to link them.
---
Nitpick comments:
In `@src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx`:
- Around line 1-3: The tests in LinkGrabberView.test.tsx reuse a module-level
mocked "invoke" which causes cross-test coupling; add a beforeEach block (e.g.,
beforeEach(() => { ... })) that resets the mock state (use vi.resetAllMocks() or
vi.clearAllMocks() and re-mock invoke if needed) so the module-level invoke mock
is cleared between tests and each test starts with a fresh mock.
In `@src/views/LinkGrabberView/__tests__/PasteZone.test.tsx`:
- Around line 71-73: Update the PasteZone component to add a stable test id
(e.g., data-testid="paste-drop-zone") on the root drop-zone element in
src/views/LinkGrabberView/PasteZone.tsx, then change the test in
PasteZone.test.tsx to query the element by that test id (use
screen.getByTestId("paste-drop-zone")) instead of using
closest("div.rounded-lg"); reference the PasteZone component and the test's
variable dropZone to locate where to add the attribute and update the selector.
In `@src/views/LinkGrabberView/PasteZone.tsx`:
- Around line 9-11: extractUrls currently uses [^\s]+ and can include trailing
punctuation like ).,]} which breaks link resolution; update the extractUrls
implementation to post-process each regex match to strip trailing punctuation
characters (e.g., .,;:>"') and closing brackets ) ] } > while preserving
balanced parentheses: for a trailing ')' only remove it if the URL contains
fewer '(' than ')' (i.e., unbalanced), otherwise keep it; apply this trimming to
each match before returning the array so the function returns clean URLs.
🪄 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: 8acadcc3-beb7-4821-9319-3e54fbceac16
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
src-tauri/Cargo.tomlsrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/resolve_links.rssrc-tauri/src/lib.rssrc/components/ui/select.tsxsrc/components/ui/switch.tsxsrc/views/LinkGrabberView.tsxsrc/views/LinkGrabberView/ActionsBar.tsxsrc/views/LinkGrabberView/FilterBar.tsxsrc/views/LinkGrabberView/LinkGrabberView.tsxsrc/views/LinkGrabberView/LinkRow.tsxsrc/views/LinkGrabberView/PackageGrouping.tsxsrc/views/LinkGrabberView/PasteZone.tsxsrc/views/LinkGrabberView/ResolvedLinksSection.tsxsrc/views/LinkGrabberView/__tests__/ActionsBar.test.tsxsrc/views/LinkGrabberView/__tests__/FilterBar.test.tsxsrc/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsxsrc/views/LinkGrabberView/__tests__/PackageGrouping.test.tsxsrc/views/LinkGrabberView/__tests__/PasteZone.test.tsxsrc/views/LinkGrabberView/__tests__/ResolvedLinksSection.test.tsxsrc/views/LinkGrabberView/index.tssrc/views/LinkGrabberView/types.ts
💤 Files with no reviewable changes (1)
- src/views/LinkGrabberView.tsx
Comment on lines
+48
to
+125
| for url in &cmd.urls { | ||
| let id = Uuid::new_v4().to_string(); | ||
|
|
||
| if !is_allowed_scheme(url) { | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: None, | ||
| filename: None, | ||
| size_bytes: None, | ||
| status: "error".to_string(), | ||
| error_message: Some("URL scheme not allowed".to_string()), | ||
| module_name: "core-http".to_string(), | ||
| is_media: false, | ||
| media_type: None, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| let plugin_info = self.plugin_loader().resolve_url(url); | ||
| let module_name = match &plugin_info { | ||
| Ok(Some(info)) => info.name().to_string(), | ||
| _ => "core-http".to_string(), | ||
| }; | ||
|
|
||
| let is_media = is_media_url(url); | ||
| let media_type = if is_media { | ||
| detect_media_type(url) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| match self.http_client().head(url) { | ||
| Ok(response) if response.is_success() => { | ||
| let filename = extract_filename_from_url(url); | ||
| let size = extract_content_length(&response); | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: Some(url.clone()), | ||
| filename, | ||
| size_bytes: size, | ||
| status: "online".to_string(), | ||
| error_message: None, | ||
| module_name, | ||
| is_media, | ||
| media_type, | ||
| }); | ||
| } | ||
| Ok(_) => { | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: None, | ||
| filename: None, | ||
| size_bytes: None, | ||
| status: "offline".to_string(), | ||
| error_message: None, | ||
| module_name, | ||
| is_media, | ||
| media_type, | ||
| }); | ||
| } | ||
| Err(e) => { | ||
| results.push(ResolvedLinkDto { | ||
| id, | ||
| original_url: url.clone(), | ||
| resolved_url: None, | ||
| filename: None, | ||
| size_bytes: None, | ||
| status: "error".to_string(), | ||
| error_message: Some(e.to_string()), | ||
| module_name, | ||
| is_media, | ||
| media_type, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This does serial blocking network checks on the command path.
HttpClient::head is synchronous, and this loop performs up to 500 calls one by one. A slow batch will stall the Tauri command for a long time and can make the resolve flow feel hung. Move this work onto a blocking pool or use bounded concurrency with timeouts so one bad host does not monopolize the whole request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/application/commands/resolve_links.rs` around lines 48 - 125,
The loop in resolve_links.rs performs synchronous, serial network calls via
HttpClient::head inside the Tauri command handler (iterating cmd.urls), which
can block the command thread; change this to run HTTP checks on a
blocking/thread-pool or use bounded async concurrency with timeouts so slow
hosts don't stall the command: spawn tasks on a blocking pool (or use
tokio::spawn_blocking or a bounded FuturesUnordered with a semaphore) for each
URL check, call HttpClient::head from those worker tasks (or switch to an async
http client and await with timeout), collect results back into the same
ResolvedLinkDto structure, and ensure accesses to plugin_loader() and
http_client() are thread-safe (clone handles or call them inside the worker) and
that errors/timeouts are mapped to the same "offline"/"error" statuses.
![cubic-dev-ai[bot]](https://cdn.github.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 24 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/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx">
<violation number="1" location="src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx:45">
P2: This test is named as if it verifies `onModeChange`, but it never asserts the callback was called.</violation>
</file>
<file name="src-tauri/src/application/commands/resolve_links.rs">
<violation number="1" location="src-tauri/src/application/commands/resolve_links.rs:133">
P1: Backend scheme validation omits `magnet:?`, so magnet links accepted by the new UI are always rejected during resolution.</violation>
</file>
<file name="src/views/LinkGrabberView/PasteZone.tsx">
<violation number="1" location="src/views/LinkGrabberView/PasteZone.tsx:10">
P2: This URL matcher captures trailing punctuation (for example `)` or `.` at sentence boundaries), which produces malformed URLs sent for resolution.</violation>
<violation number="2" location="src/views/LinkGrabberView/PasteZone.tsx:50">
P2: Container-file drops emit `container:*` values that are filtered out by `handlePasteUrls`, so this path never resolves anything.</violation>
</file>
<file name="src/views/LinkGrabberView/ResolvedLinksSection.tsx">
<violation number="1" location="src/views/LinkGrabberView/ResolvedLinksSection.tsx:17">
P2: Handle empty hostnames when grouping by hostname; valid non-hostname URLs can currently create unnamed groups.</violation>
</file>
<file name="src/views/LinkGrabberView/LinkGrabberView.tsx">
<violation number="1" location="src/views/LinkGrabberView/LinkGrabberView.tsx:38">
P2: This client-side URL filter is overly strict and silently drops extracted inputs (e.g., uppercase schemes/container drop values).</violation>
<violation number="2" location="src/views/LinkGrabberView/LinkGrabberView.tsx:79">
P1: This toggle calls a non-existent IPC command, so switching it will fail at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
|
|
||
| if (containerFiles.length > 0) { | ||
| const containerUrls = containerFiles.map((f) => `container:${f.name}`); |
There was a problem hiding this comment.
P2: Container-file drops emit container:* values that are filtered out by handlePasteUrls, so this path never resolves anything.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/views/LinkGrabberView/PasteZone.tsx, line 50:
<comment>Container-file drops emit `container:*` values that are filtered out by `handlePasteUrls`, so this path never resolves anything.</comment>
<file context>
@@ -0,0 +1,91 @@
+ );
+
+ if (containerFiles.length > 0) {
+ const containerUrls = containerFiles.map((f) => `container:${f.name}`);
+ onPasteUrls(containerUrls);
+ return;
</file context>
- Accept magnet: URIs in backend scheme validation and resolve as "online" - Sanitize error messages in ResolvedLinkDto (generic user-facing message) - Preserve validation errors (batch limit) through IPC, sanitize internal ones - Strip trailing punctuation from extracted URLs in PasteZone - Preserve container: entries in handlePasteUrls URL filter - Disable clipboard monitoring Switch until backend command exists (task 22) - Fix LinkRow tooltip accessibility (focusable span instead of p) - Associate PackageGrouping label with combobox via aria-labelledby - Handle empty hostnames in groupLinks hostname grouping - Add data-testid to PasteZone drop zone for stable test selectors - Reset invoke mock between tests in LinkGrabberView.test.tsx - Verify onModeChange callback in PackageGrouping test - Add TODO for async URL resolution concurrency
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src-tauri/src/application/commands/resolve_links.rs (1)
48-51:⚠️ Potential issue | 🟠 Major
handle_resolve_linksstill does serial blocking I/O on the async command path.Line 99 executes sync
headin the per-URL loop, so slow hosts can stall the entire IPC call; move to bounded concurrency with timeout (or blocking pool workers).#!/bin/bash # Verify synchronous head API and its serial use inside async handler. rg -n 'fn head\(&self, url: &str\)' src-tauri/src/domain/ports/driven/http_client.rs rg -n 'for url in &cmd.urls|http_client\(\)\.head\(' src-tauri/src/application/commands/resolve_links.rsAlso applies to: 99-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 48 - 51, The handler handle_resolve_links is performing synchronous blocking I/O inside the per-URL loop by calling http_client.head(...) serially which stalls the async command; change it to run head calls with bounded concurrency and timeouts — either make HttpClient::head asynchronous or call the sync head inside tokio::task::spawn_blocking and wrap each spawned task with tokio::time::timeout, control parallelism with a tokio::sync::Semaphore (or tokio::task::spawn_pool) and collect results via join handles, and apply the same pattern for the loop over cmd.urls so slow hosts cannot block the entire IPC handler.src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx (1)
53-61:⚠️ Potential issue | 🟠 MajorCallback assertion is still non-deterministic in this test.
Line 45 says callback wiring is being tested, but Line 58-61 allows a passing path without asserting
onModeChange, so regressions can slip through.Proposed deterministic test change
it("should call onModeChange when select value changes", async () => { const user = userEvent.setup(); const onModeChange = vi.fn(); render(<PackageGrouping mode="none" onModeChange={onModeChange} />); const trigger = screen.getByRole("combobox"); await user.click(trigger); - - // Try to find an option in the portal - const option = await screen.findByText("By Hostname").catch(() => null); - if (option) { - await user.click(option); - expect(onModeChange).toHaveBeenCalledWith("hostname"); - } else { - // Radix Select portal doesn't render in jsdom — verify trigger is at least interactive - expect(trigger).toHaveAttribute("aria-expanded", "true"); - } + await user.keyboard("{ArrowDown}{Enter}"); + expect(onModeChange).toHaveBeenCalledWith("hostname"); });#!/bin/bash # Verify whether fallback logic still exists (non-deterministic assertion path). rg -n 'findByText\("By Hostname"\).*catch\(\(\) => null\)|aria-expanded' src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx` around lines 53 - 61, The test currently lets a missing Radix Select portal skip the callback assertion by using screen.findByText("By Hostname").catch(() => null) and an else branch that only checks trigger aria-expanded; update PackageGrouping.test.tsx to make the onModeChange assertion deterministic: either remove the .catch so await screen.findByText("By Hostname") will fail the test if the option isn't present and then await user.click(option) followed by expect(onModeChange).toHaveBeenCalledWith("hostname"), or keep the conditional but make the else branch explicitly fail (throw or use fail("Select option not found; test cannot verify onModeChange")) so the test cannot pass without verifying onModeChange; reference onModeChange, trigger, user.click, and screen.findByText("By Hostname") when making the change.
🧹 Nitpick comments (2)
src/views/LinkGrabberView/ResolvedLinksSection.tsx (1)
45-50: Consider mutable accumulation to avoid repeated object/array copies.Current grouping rebuilds objects/arrays on every item. A push-based accumulator is simpler and scales better.
Refactor sketch
- return links.reduce<Record<string, ResolvedLink[]>>((acc, link) => { - const key = getGroupKey(link, mode); - return { - ...acc, - [key]: [...(acc[key] ?? []), link], - }; - }, {}); + return links.reduce<Record<string, ResolvedLink[]>>((acc, link) => { + const key = getGroupKey(link, mode); + (acc[key] ??= []).push(link); + return acc; + }, {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/ResolvedLinksSection.tsx` around lines 45 - 50, The current links.reduce in ResolvedLinksSection.tsx rebuilds the accumulator and arrays for every item causing unnecessary allocations; change it to a mutable accumulator pattern by initializing acc as an empty Record<string, ResolvedLink[]> and, for each link (using links.reduce or links.forEach), compute const key = getGroupKey(link, mode), ensure acc[key] is initialized (acc[key] = acc[key] || []), then push the link into acc[key] and return acc — keep the function name/usage (getGroupKey, links.reduce or replace with links.forEach) and the accumulator type Record<string, ResolvedLink[]> the same.src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx (1)
147-149: Adddata-testidto ActionsBar's Clear button for robust test selection.The test selects the ActionsBar Clear button by index because both PasteZone and ActionsBar render buttons with the identical accessible name
"Clear". This breaks if component order changes or another Clear button is introduced. Adddata-testid="actions-bar-clear"to the destructive Clear button in ActionsBar.tsx (line 33) and update the test to usescreen.getByTestId("actions-bar-clear").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx` around lines 147 - 149, Add a stable test selector to the destructive "Clear" button in the ActionsBar component: in ActionsBar.tsx, add data-testid="actions-bar-clear" to the Clear button element (the destructive Clear rendered by the ActionsBar component) and then update the test in LinkGrabberView.test.tsx to replace the brittle index-based selection with screen.getByTestId("actions-bar-clear") and click that element.
🤖 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/application/commands/resolve_links.rs`:
- Line 131: The log currently emits the raw `url` variable in the
`tracing::debug!` call inside the link resolution flow (see usage of `url` and
the log at the end of `resolve_links`), which can leak credentials/PII; instead
parse the URL (e.g., with `url::Url::parse`) and produce a redacted host/path
fingerprint (for example combine host + path and hash or redact
query/credentials) and log that fingerprint as `url_fingerprint` along with
`error = %e` in the `tracing::debug!` call; update the logging site that
currently uses `url = url` to use the new redacted value and ensure
queries/username/password are not included.
In `@src/views/LinkGrabberView/ResolvedLinksSection.tsx`:
- Line 24: The extension extraction currently uses
link.filename?.split(".").pop()?.toUpperCase() ?? "UNKNOWN", which returns the
whole filename (e.g., "README") when there's no dot; change the logic in
ResolvedLinksSection.tsx to first check that link.filename contains a dot (or
that split(".") yields length > 1) before using pop(), e.g., compute parts =
link.filename?.split(".") and only use parts.pop() when parts.length > 1, then
return ext?.toUpperCase() ?? "UNKNOWN" so files without extensions are
classified as "UNKNOWN".
---
Duplicate comments:
In `@src-tauri/src/application/commands/resolve_links.rs`:
- Around line 48-51: The handler handle_resolve_links is performing synchronous
blocking I/O inside the per-URL loop by calling http_client.head(...) serially
which stalls the async command; change it to run head calls with bounded
concurrency and timeouts — either make HttpClient::head asynchronous or call the
sync head inside tokio::task::spawn_blocking and wrap each spawned task with
tokio::time::timeout, control parallelism with a tokio::sync::Semaphore (or
tokio::task::spawn_pool) and collect results via join handles, and apply the
same pattern for the loop over cmd.urls so slow hosts cannot block the entire
IPC handler.
In `@src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx`:
- Around line 53-61: The test currently lets a missing Radix Select portal skip
the callback assertion by using screen.findByText("By Hostname").catch(() =>
null) and an else branch that only checks trigger aria-expanded; update
PackageGrouping.test.tsx to make the onModeChange assertion deterministic:
either remove the .catch so await screen.findByText("By Hostname") will fail the
test if the option isn't present and then await user.click(option) followed by
expect(onModeChange).toHaveBeenCalledWith("hostname"), or keep the conditional
but make the else branch explicitly fail (throw or use fail("Select option not
found; test cannot verify onModeChange")) so the test cannot pass without
verifying onModeChange; reference onModeChange, trigger, user.click, and
screen.findByText("By Hostname") when making the change.
---
Nitpick comments:
In `@src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx`:
- Around line 147-149: Add a stable test selector to the destructive "Clear"
button in the ActionsBar component: in ActionsBar.tsx, add
data-testid="actions-bar-clear" to the Clear button element (the destructive
Clear rendered by the ActionsBar component) and then update the test in
LinkGrabberView.test.tsx to replace the brittle index-based selection with
screen.getByTestId("actions-bar-clear") and click that element.
In `@src/views/LinkGrabberView/ResolvedLinksSection.tsx`:
- Around line 45-50: The current links.reduce in ResolvedLinksSection.tsx
rebuilds the accumulator and arrays for every item causing unnecessary
allocations; change it to a mutable accumulator pattern by initializing acc as
an empty Record<string, ResolvedLink[]> and, for each link (using links.reduce
or links.forEach), compute const key = getGroupKey(link, mode), ensure acc[key]
is initialized (acc[key] = acc[key] || []), then push the link into acc[key] and
return acc — keep the function name/usage (getGroupKey, links.reduce or replace
with links.forEach) and the accumulator type Record<string, ResolvedLink[]> the
same.
🪄 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: deff58d1-a9e0-43bc-8dd8-14c1bcc7e32f
📒 Files selected for processing (10)
src-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/resolve_links.rssrc/views/LinkGrabberView/LinkGrabberView.tsxsrc/views/LinkGrabberView/LinkRow.tsxsrc/views/LinkGrabberView/PackageGrouping.tsxsrc/views/LinkGrabberView/PasteZone.tsxsrc/views/LinkGrabberView/ResolvedLinksSection.tsxsrc/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsxsrc/views/LinkGrabberView/__tests__/PackageGrouping.test.tsxsrc/views/LinkGrabberView/__tests__/PasteZone.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/views/LinkGrabberView/PackageGrouping.tsx
- src/views/LinkGrabberView/LinkRow.tsx
- src/views/LinkGrabberView/tests/PasteZone.test.tsx
- src/views/LinkGrabberView/PasteZone.tsx
- src/views/LinkGrabberView/LinkGrabberView.tsx
There was a problem hiding this comment.
4 issues found across 10 files (changes from recent commits).
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/views/LinkGrabberView/LinkRow.tsx">
<violation number="1" location="src/views/LinkGrabberView/LinkRow.tsx:48">
P2: `role="button"` is applied to a non-interactive element without button handlers, which creates misleading accessibility semantics.</violation>
</file>
<file name="src/views/LinkGrabberView/PasteZone.tsx">
<violation number="1" location="src/views/LinkGrabberView/PasteZone.tsx:13">
P2: The trailing-punctuation cleanup is over-aggressive: it removes `]`, which breaks valid bracketed IPv6 URLs.</violation>
</file>
<file name="src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx">
<violation number="1" location="src/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx:54">
P2: This test can pass without verifying `onModeChange`, so regressions in select option rendering/selection may go undetected.</violation>
</file>
<file name="src/views/LinkGrabberView/LinkGrabberView.tsx">
<violation number="1" location="src/views/LinkGrabberView/LinkGrabberView.tsx:43">
P2: `container:` links are now accepted by the UI filter, but `link_resolve` rejects that scheme, so container drops are routed into a guaranteed error path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Redact raw URLs from tracing logs to avoid leaking credentials/PII - Fix extension grouping for files without dots (README → UNKNOWN) - Remove misleading role="button" from tooltip trigger span - Exclude ] from trailing punctuation strip to preserve IPv6 URLs - Remove container: from URL filter (needs dedicated backend command) - Strengthen PackageGrouping test assertion
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src-tauri/src/application/commands/resolve_links.rs (1)
48-51:⚠️ Potential issue | 🟠 MajorMove blocking HEAD checks off the async command path.
This still performs per-URL network checks serially in the command handler, so slow hosts can stall the entire resolve request. The TODO is accurate, but this is still a high-impact latency/reliability risk for larger batches.
#!/bin/bash set -euo pipefail # Verify current command flow is sequential and uses direct head() calls rg -n -C3 'async fn handle_resolve_links|for url in &cmd\.urls|http_client\(\)\.head\(' src-tauri/src/application/commands/resolve_links.rs # Verify no bounded concurrency primitives are used in this handler rg -n -C2 'spawn_blocking|Semaphore|FuturesUnordered|buffer_unordered' src-tauri/src/application/commands/resolve_links.rs # Inspect head() definitions/signatures in repo to confirm blocking vs async API shape rg -n -C2 'fn\s+head\s*\(|async\s+fn\s+head\s*\('Also applies to: 99-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 48 - 51, The resolve_links handler is doing blocking per-URL HEAD checks serially (see async fn handle_resolve_links, iterating over cmd.urls and calling http_client().head()), which can stall the async command; move the blocking/network work off the async path by spawning bounded parallel tasks: wrap the head() calls in tokio::task::spawn_blocking (or ensure an async non-blocking client) and run them with a concurrency limiter (tokio::sync::Semaphore or FuturesUnordered with buffer_unordered) so multiple URL checks proceed in parallel but are bounded, collect results and then return them from handle_resolve_links; update references to http_client().head() usage accordingly.
🤖 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/application/commands/resolve_links.rs`:
- Around line 152-157: The is_allowed_scheme function is doing case-sensitive
prefix checks and will reject valid URLs with uppercase schemes; fix it by
performing the scheme comparison case-insensitively—e.g., convert the input url
(or at least its scheme prefix) to ASCII lowercase before calling starts_with
for "http://", "https://", "ftp://", and "magnet:". Update is_allowed_scheme to
use to_ascii_lowercase() on the portion checked so uppercase schemes like
"HTTPS://" are accepted.
---
Duplicate comments:
In `@src-tauri/src/application/commands/resolve_links.rs`:
- Around line 48-51: The resolve_links handler is doing blocking per-URL HEAD
checks serially (see async fn handle_resolve_links, iterating over cmd.urls and
calling http_client().head()), which can stall the async command; move the
blocking/network work off the async path by spawning bounded parallel tasks:
wrap the head() calls in tokio::task::spawn_blocking (or ensure an async
non-blocking client) and run them with a concurrency limiter
(tokio::sync::Semaphore or FuturesUnordered with buffer_unordered) so multiple
URL checks proceed in parallel but are bounded, collect results and then return
them from handle_resolve_links; update references to http_client().head() usage
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 358bdcf5-ff5b-4c12-bd8f-3f5b2cef6100
📒 Files selected for processing (6)
src-tauri/src/application/commands/resolve_links.rssrc/views/LinkGrabberView/LinkGrabberView.tsxsrc/views/LinkGrabberView/LinkRow.tsxsrc/views/LinkGrabberView/PasteZone.tsxsrc/views/LinkGrabberView/ResolvedLinksSection.tsxsrc/views/LinkGrabberView/__tests__/PackageGrouping.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/views/LinkGrabberView/tests/PackageGrouping.test.tsx
- src/views/LinkGrabberView/LinkRow.tsx
- src/views/LinkGrabberView/PasteZone.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src-tauri/src/application/commands/resolve_links.rs (1)
48-51:⚠️ Potential issue | 🟠 MajorSerial blocking
HEADcalls still run on the command path.Line 99 performs synchronous
HttpClient::headinside a per-URL loop, so one slow host can delay the full response (up to 500 URLs). The TODO on Lines 48–51 is accurate, but this is still a user-visible latency/reliability risk in production.Also applies to: 99-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 48 - 51, The loop over cmd.urls currently calls HttpClient::head synchronously per URL (in the for url in &cmd.urls loop), causing serial blocking on the command path; fix it by offloading each blocking HttpClient::head call into tokio::task::spawn_blocking and constrain concurrent tasks with a tokio::sync::Semaphore (or tokio::task::JoinSet) to a bounded parallelism (e.g., N permits) and then await all spawned tasks and collect results before returning; update the code paths that read cmd.urls and call HttpClient::head to use the spawned tasks and semaphore so a slow host cannot block the whole operation.
🧹 Nitpick comments (1)
src-tauri/src/application/commands/resolve_links.rs (1)
328-348: Add uppercase scheme regression tests to lock the recent fix.Given the case-insensitive scheme update, please add assertions for uppercase/mixed-case inputs (e.g.,
HTTPS://...,MAGNET:?...) so this behavior does not regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 328 - 348, Add tests that assert is_allowed_scheme accepts uppercase and mixed-case schemes to prevent regressions of the case-insensitive scheme handling; specifically, add test functions similar to test_is_allowed_scheme_accepts_https and test_is_allowed_scheme_accepts_magnet but using inputs like "HTTPS://example.com/file.zip" and "MAGNET:?xt=urn:btih:..." (and optionally a mixed-case variant like "HtTpS://...") referencing the is_allowed_scheme function so the behavior remains validated.
🤖 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/application/commands/resolve_links.rs`:
- Around line 70-84: The magnet-scheme check is case-sensitive: change the check
in resolve_links.rs that currently does url.starts_with("magnet:") to a
case-insensitive comparison (e.g.,
url.to_ascii_lowercase().starts_with("magnet:") or compare a lowercased scheme)
so inputs like "MAGNET:..." are handled by the magnet branch and produce a
ResolvedLinkDto with module_name "magnet"; apply the same case-insensitive logic
to the related scheme allowlisting logic elsewhere in the file (the scheme
validation block) so magnet scheme detection is consistent across resolve_links
handling.
---
Duplicate comments:
In `@src-tauri/src/application/commands/resolve_links.rs`:
- Around line 48-51: The loop over cmd.urls currently calls HttpClient::head
synchronously per URL (in the for url in &cmd.urls loop), causing serial
blocking on the command path; fix it by offloading each blocking
HttpClient::head call into tokio::task::spawn_blocking and constrain concurrent
tasks with a tokio::sync::Semaphore (or tokio::task::JoinSet) to a bounded
parallelism (e.g., N permits) and then await all spawned tasks and collect
results before returning; update the code paths that read cmd.urls and call
HttpClient::head to use the spawned tasks and semaphore so a slow host cannot
block the whole operation.
---
Nitpick comments:
In `@src-tauri/src/application/commands/resolve_links.rs`:
- Around line 328-348: Add tests that assert is_allowed_scheme accepts uppercase
and mixed-case schemes to prevent regressions of the case-insensitive scheme
handling; specifically, add test functions similar to
test_is_allowed_scheme_accepts_https and test_is_allowed_scheme_accepts_magnet
but using inputs like "HTTPS://example.com/file.zip" and
"MAGNET:?xt=urn:btih:..." (and optionally a mixed-case variant like
"HtTpS://...") referencing the is_allowed_scheme function so the behavior
remains validated.
🪄 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: 54f75e62-a674-4af9-af19-2367b24744d5
📒 Files selected for processing (1)
src-tauri/src/application/commands/resolve_links.rs
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/application/commands/resolve_links.rs">
<violation number="1" location="src-tauri/src/application/commands/resolve_links.rs:153">
P2: Scheme validation is case-insensitive now, but magnet handling remains case-sensitive, so uppercase `MAGNET:` links can be misclassified as offline/error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src-tauri/src/application/commands/resolve_links.rs (5)
86-90: Plugin resolution errors are silently swallowed.When
plugin_loader().resolve_url(url)returnsErr(_), the code silently defaults to"core-http". Consider logging plugin resolution failures at debug level to aid troubleshooting.🔍 Proposed logging
let plugin_info = self.plugin_loader().resolve_url(url); let module_name = match &plugin_info { Ok(Some(info)) => info.name().to_string(), + Err(e) => { + tracing::debug!(error = %e, "plugin resolution failed, using core-http"); + "core-http".to_string() + } _ => "core-http".to_string(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 86 - 90, plugin resolution failures are being swallowed: when calling plugin_loader().resolve_url(url) (the plugin_info match that sets module_name and falls back to "core-http") you should log the Err case at debug level instead of silently ignoring it; change the match on plugin_info to handle Err(e) by calling the appropriate logger.debug (or process_logger.debug) with context about the url and error before falling back to "core-http" so failures are visible while preserving the existing fallback behavior.
51-52: Multipleto_lowercase()allocations per URL in the loop.Each URL iteration calls
to_lowercase()multiple times (lines 70, 153, 200, 217). For 500 URLs, this creates thousands of temporary string allocations. Consider computing the lowercase URL once per iteration.♻️ Reduce allocations
for url in &cmd.urls { let id = Uuid::new_v4().to_string(); + let url_lower = url.to_lowercase(); - if !is_allowed_scheme(url) { + if !is_allowed_scheme_lower(&url_lower) { // ... } - if url.to_lowercase().starts_with("magnet:") { + if url_lower.starts_with("magnet:") { // ... } // ... pass url_lower to is_media_url and detect_media_type🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 51 - 52, In the for loop over cmd.urls (the loop that creates id with Uuid::new_v4()), avoid calling url.to_lowercase() multiple times by computing a single lowercase string at the top of each iteration (e.g., let lower_url = url.to_lowercase()) and reuse lower_url whenever a lowercase form is needed in functions like the domain/host checks and lookups currently calling to_lowercase() (the occurrences you flagged). Replace each repeated to_lowercase() call with a reference to this single lower_url (or lower_url.as_str()) to eliminate the redundant allocations per URL.
24-25: Consider using an enum forstatusto enforce valid values at compile time.The
statusfield is aStringbut only accepts"checking" | "online" | "offline" | "error". Using an enum with#[serde(rename_all = "lowercase")]would catch invalid statuses at compile time rather than runtime.♻️ Optional improvement
+#[derive(Debug, Clone, Serialize)] +#[serde(rename_all = "lowercase")] +pub enum LinkStatus { + Checking, + Online, + Offline, + Error, +} + #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct ResolvedLinkDto { // ... - /// "checking" | "online" | "offline" | "error" - pub status: String, + pub status: LinkStatus, // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 24 - 25, Replace the String status field with a dedicated enum (e.g., enum LinkStatus { Checking, Online, Offline, Error }) and derive/annotate Serialize and Deserialize with #[serde(rename_all = "lowercase")] so JSON uses "checking"|"online"|"offline"|"error"; change the struct's pub status: String to pub status: LinkStatus and update any constructors/serializers/usages in resolve_links.rs to construct or pattern-match the LinkStatus variants instead of raw strings.
188-197:extract_hostdoesn't handle URLs with userinfo and has misleading variable name.The variable
lower_urlisn't lowercased. More importantly, URLs with userinfo (https://user:pass@host.com/) will incorrectly include the credentials in the extracted host.♻️ Proposed fix
fn extract_host(url: &str) -> &str { - let lower_url = url; - let after_scheme = lower_url + let after_scheme = url .strip_prefix("https://") - .or_else(|| lower_url.strip_prefix("http://")) - .or_else(|| lower_url.strip_prefix("ftp://")) - .unwrap_or(lower_url); - let host_and_port = after_scheme.split('/').next().unwrap_or(""); + .or_else(|| url.strip_prefix("http://")) + .or_else(|| url.strip_prefix("ftp://")) + .unwrap_or(url); + // Strip userinfo if present (user:pass@host) + let after_userinfo = after_scheme + .split('@') + .last() + .unwrap_or(after_scheme); + let host_and_port = after_userinfo.split('/').next().unwrap_or(""); host_and_port.split(':').next().unwrap_or("") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 188 - 197, The current extract_host function incorrectly keeps userinfo (user:pass@) and the variable lower_url is misleading (it isn't lowercased); replace the manual prefix/split logic (variables lower_url, after_scheme, host_and_port) with proper URL parsing using url::Url: attempt Url::parse(url).host_str().map(|s| s.to_string()).unwrap_or_else(|| { try parsing with "http://"+url as fallback; "".to_string() }), change extract_host's return type to String and update its callers accordingly so you return the actual host without userinfo.
199-236:is_media_urlanddetect_media_typeduplicate the media hosts list.Both functions maintain similar host lists. Consider extracting a shared constant to avoid drift between them.
♻️ DRY improvement
+const VIDEO_HOSTS: &[&str] = &[ + "youtube.com", + "youtu.be", + "vimeo.com", + "dailymotion.com", + "twitch.tv", + "tiktok.com", +]; +const AUDIO_HOSTS: &[&str] = &["soundcloud.com"]; + +fn host_matches(host: &str, patterns: &[&str]) -> bool { + patterns.iter().any(|&h| host == h || host.ends_with(&format!(".{h}"))) +} + fn is_media_url(url: &str) -> bool { let lower = url.to_lowercase(); let host = extract_host(&lower); - let media_hosts = [...]; - media_hosts.iter().any(|&h| host == h || host.ends_with(&format!(".{h}"))) + host_matches(host, VIDEO_HOSTS) || host_matches(host, AUDIO_HOSTS) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/resolve_links.rs` around lines 199 - 236, Extract the duplicated host arrays into one or two shared constants (e.g., const MEDIA_HOSTS: &[&str] = &["youtube.com","youtu.be","vimeo.com","dailymotion.com","twitch.tv","tiktok.com","soundcloud.com"] and optionally const MEDIA_VIDEO_HOSTS that excludes "soundcloud.com"), then update is_media_url and detect_media_type to use those constants instead of inline arrays; keep detect_media_type's special-case for "soundcloud.com" (return "audio") and otherwise check MEDIA_VIDEO_HOSTS (or filter MEDIA_HOSTS) to return "video". Ensure both functions call extract_host(&lower) as before and reuse the new constants to avoid drift.
🤖 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/application/commands/resolve_links.rs`:
- Around line 86-90: plugin resolution failures are being swallowed: when
calling plugin_loader().resolve_url(url) (the plugin_info match that sets
module_name and falls back to "core-http") you should log the Err case at debug
level instead of silently ignoring it; change the match on plugin_info to handle
Err(e) by calling the appropriate logger.debug (or process_logger.debug) with
context about the url and error before falling back to "core-http" so failures
are visible while preserving the existing fallback behavior.
- Around line 51-52: In the for loop over cmd.urls (the loop that creates id
with Uuid::new_v4()), avoid calling url.to_lowercase() multiple times by
computing a single lowercase string at the top of each iteration (e.g., let
lower_url = url.to_lowercase()) and reuse lower_url whenever a lowercase form is
needed in functions like the domain/host checks and lookups currently calling
to_lowercase() (the occurrences you flagged). Replace each repeated
to_lowercase() call with a reference to this single lower_url (or
lower_url.as_str()) to eliminate the redundant allocations per URL.
- Around line 24-25: Replace the String status field with a dedicated enum
(e.g., enum LinkStatus { Checking, Online, Offline, Error }) and derive/annotate
Serialize and Deserialize with #[serde(rename_all = "lowercase")] so JSON uses
"checking"|"online"|"offline"|"error"; change the struct's pub status: String to
pub status: LinkStatus and update any constructors/serializers/usages in
resolve_links.rs to construct or pattern-match the LinkStatus variants instead
of raw strings.
- Around line 188-197: The current extract_host function incorrectly keeps
userinfo (user:pass@) and the variable lower_url is misleading (it isn't
lowercased); replace the manual prefix/split logic (variables lower_url,
after_scheme, host_and_port) with proper URL parsing using url::Url: attempt
Url::parse(url).host_str().map(|s| s.to_string()).unwrap_or_else(|| { try
parsing with "http://"+url as fallback; "".to_string() }), change extract_host's
return type to String and update its callers accordingly so you return the
actual host without userinfo.
- Around line 199-236: Extract the duplicated host arrays into one or two shared
constants (e.g., const MEDIA_HOSTS: &[&str] =
&["youtube.com","youtu.be","vimeo.com","dailymotion.com","twitch.tv","tiktok.com","soundcloud.com"]
and optionally const MEDIA_VIDEO_HOSTS that excludes "soundcloud.com"), then
update is_media_url and detect_media_type to use those constants instead of
inline arrays; keep detect_media_type's special-case for "soundcloud.com"
(return "audio") and otherwise check MEDIA_VIDEO_HOSTS (or filter MEDIA_HOSTS)
to return "video". Ensure both functions call extract_host(&lower) as before and
reuse the new constants to avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cdb50fd-0fe6-482b-9b1d-d4a99808d0c7
📒 Files selected for processing (1)
src-tauri/src/application/commands/resolve_links.rs
This was referenced Apr 11, 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
link_resolveIPC command via plugin infrastructure with URL validation (scheme check), batch limit (500), hostname-based media detection, and sanitized error messagesChanges
Backend (Rust — 4 files):
resolve_links.rs— ResolvedLinkDto + CommandBus handler with URL validation, batch limit, media detectioncommands/mod.rs— ResolveLinksCommand structtauri_ipc.rs— link_resolve IPC function with error sanitizationlib.rs— command registrationFrontend (TypeScript — 17 files):
src/views/LinkGrabberView/Testing
Spec coverage
Implements task 20 from specs/01-mvp/20-link-grabber-view.md
Summary by cubic
Build the full Link Grabber view with paste/drag‑and‑drop, link resolution, filtering, grouping, selection, and batch actions. Adds a new
link_resolveIPC with URL validation (http/https/ftp/magnet), media detection, metadata, and a 500‑URL cap, implementing MVP task 20.New Features
link_resolveyet.link_resolve: validates schemes, detects media by hostname, extracts filename/size, returnsResolvedLinkDto. Clipboard monitoring toggle shown but disabled (backend handled in task 22).Bug Fixes
magnet:URIs and mark them as online; make scheme and magnet handling case‑insensitive.aria-labelledbyfor grouping select, handle empty hostnames and no‑extension files as “UNKNOWN”. Redact raw URLs from logs.Written for commit a23d340. Summary will update on new commits.
Summary by CodeRabbit