feat(plugin): report broken plugin with GitHub issue pre-fill#110
Merged
feat(plugin): report broken plugin with GitHub issue pre-fill#110
Conversation
…ill (task 16)
Plugins now expose a "Report broken plugin" item in the kebab menu of
the Plugin Store row. Clicking it opens the user's default browser at
a pre-filled GitHub issue on the plugin's repository, with diagnostic
metadata (plugin name + version, Vortex version, OS, optional URL
under test, last 50 log lines) inlined into the issue body.
Domain
- `PluginInfo` gains `repository_url: Option<String>` (parsed from a
new `[plugin].repository` key in `plugin.toml`).
- New `UrlOpener` driven port with platform-native `SystemUrlOpener`
(xdg-open / open / cmd start), `http(s)://` only by validation so
the OS launcher never receives `javascript:` / `file://` payloads.
- `build_report_broken_url` is std-only: RFC 3986 unreserved-set
percent encoder, last 50 log lines kept, GitHub-only repository
hosts, accepts `.git` suffix, rejects malformed URLs with
`DomainError::ValidationError`.
Application
- `ReportBrokenPluginCommand` handler returns `AppError::Validation`
when the manifest carries no `repository_url` and `AppError::NotFound`
when the plugin is not loaded; the URL is opened off the async
runtime via `tokio::task::spawn_blocking`.
Adapters
- `manifest.rs` parses `[plugin].repository` (optional, backward
compatible — plugins without the field still install).
- New Tauri IPC `plugin_report_broken(pluginName, logLines?, testedUrl?)
-> string` returns the issue URL so the UI can fall back to clipboard
copy if the launcher fails.
Frontend
- `PluginStoreRow` renders an extra menu item when `onReportBroken` is
wired (hidden otherwise), and the parent `PluginsView` invokes the
IPC through `useTauriMutation` with success / error toasts.
- i18n (en/fr): `plugins.action.reportBroken`,
`plugins.toast.reportBroken{Success,Error}`.
Coverage
- 11 new Rust tests (URL builder edge cases + handler) and 2 new
Vitest cases for the row's menu entry. cargo test --lib reports
909 passed, npx vitest run reports 582 passed.
github-actions
Bot
added
documentation
|
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:
📝 WalkthroughWalkthroughParses optional Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as React UI
participant Tauri as Tauri IPC
participant Bus as CommandBus
participant Domain as Domain Logic
participant Opener as SystemUrlOpener
participant Browser as Browser
User->>UI: click "Report Broken"
UI->>Tauri: plugin_report_broken(name, log_lines?, tested_url?)
Tauri->>Bus: handle_report_broken_plugin(command)
Bus->>Domain: resolve manifest (loaded or find_installed_manifest)
Domain-->>Bus: PluginInfo (with repository_url)
Bus->>Domain: build_report_broken_url(repository_url, metadata)
Domain-->>Bus: encoded GitHub issue URL
Bus->>Opener: open_url(issue_url)
Opener->>Opener: validate http(s) and authority (strict)
Opener->>Browser: spawn OS open command (xdg-open/open/Windows)
Browser-->>User: open GitHub new issue form
Opener-->>Bus: success/error (logged)
Bus-->>Tauri: return issue URL or error
Tauri-->>UI: show toast / copy URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR implements the "Report broken plugin" feature (task 16): a new kebab-menu action in the Plugin Store that builds a pre-filled GitHub issue URL with diagnostic metadata and opens it via a platform-native Confidence Score: 5/5Safe to merge; only P2 findings remain and no security issues were identified. All findings are P2 (a single UX edge-case around log-line length in the generated URL). The security surface (URL validation, symlink containment, scheme restriction) is well-hardened and well-tested. The previous P1 about unconditional src-tauri/src/domain/model/plugin.rs —
|
| Filename | Overview |
|---|---|
| src-tauri/src/domain/model/plugin.rs | Adds repository_url to PluginInfo, build_report_broken_url, parse_github_owner_repo, and percent_encode. Logic is solid; per-line log length is unbounded which can produce URLs exceeding GitHub/browser limits. |
| src-tauri/src/adapters/driven/filesystem/url_opener.rs | New SystemUrlOpener with validate_http_url (IPv6, port, authority checks) and platform-specific launchers. Windows discards exit-status intentionally and with a comment explaining why; well-tested. |
| src-tauri/src/application/commands/report_broken_plugin.rs | Handler falls back to find_installed_manifest for unloaded plugins, always returns URL for clipboard fallback, and surfaced launcher failures are logged but non-fatal. Well-tested with mocks. |
| src/views/PluginsView.tsx | Adds canReportBroken guard (GitHub prefix check), reportBrokenMutation with clipboard fallback and toast handling, and conditionally passes onReportBroken to rows — addressing the concern from the previous review thread. |
| src/views/PluginsView/PluginStoreRow.tsx | Adds onReportBroken prop (optional); menu item only renders when prop is provided and plugin is installed. Clean implementation. |
| src-tauri/src/adapters/driven/plugin/extism_loader.rs | Adds find_installed_manifest with name sanitisation (rejects /, \, ..) and symlink-containment check via canonicalize. Silently returns Ok(None) for unreadable manifests, which is correct for the broken-plugin use-case. |
| src-tauri/src/adapters/driven/plugin/manifest.rs | Adds parse_manifest_metadata (no .wasm requirement) and repository deserialization via Option<String> in RawPluginSection. Backward-compatible. |
| src/types/plugin-store.ts | Adds repository: string field; consistent with backend DTO which always serializes as a (possibly empty) string. |
Sequence Diagram
sequenceDiagram
participant UI as PluginsView (React)
participant Row as PluginStoreRow
participant IPC as Tauri IPC
participant Bus as CommandBus
participant Loader as PluginLoader
participant Builder as build_report_broken_url
participant Opener as SystemUrlOpener
UI->>Row: onReportBroken(name) [only if canReportBroken]
Row->>IPC: plugin_report_broken(pluginName, logLines?, testedUrl?)
IPC->>Bus: handle_report_broken_plugin(cmd)
Bus->>Loader: list_loaded()
alt plugin found in loaded list
Loader-->>Bus: PluginInfo
else not loaded (e.g. broken)
Bus->>Loader: find_installed_manifest(name)
Loader-->>Bus: PluginInfo (from plugin.toml)
end
Bus->>Builder: build_report_broken_url(repo_url, meta, logs)
Builder-->>Bus: issue_url (percent-encoded)
Bus->>Opener: open_url(issue_url) [spawn_blocking]
Opener-->>Bus: Ok / Err (logged, non-fatal)
Bus-->>IPC: Ok(issue_url)
IPC-->>UI: issue_url
UI->>UI: clipboard.writeText(url) [best-effort]
UI->>UI: toast.success / toast.error
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src-tauri/src/domain/model/plugin.rs
Line: 951-1006
Comment:
**No per-line length cap for log lines in URL body**
`MAX_LOG_LINES` bounds the *number* of lines included but not their individual length. A single line containing a stack trace, serialised JSON, or base64 payload could be hundreds of kilobytes; 50 such lines would produce a URL well above GitHub's ~8 KB practical limit (acknowledged in the comment) and likely above browser URL limits (~2 MB). The URL would either be silently truncated by GitHub or rejected by the browser entirely, making the pre-filled issue arrive empty.
Consider adding a per-line character limit (e.g. 500 chars) before the percent-encode step:
```rust
const MAX_LOG_LINE_CHARS: usize = 500;
for line in &log_lines[start..] {
let truncated = &line[..line.len().min(MAX_LOG_LINE_CHARS)];
body.push_str(truncated);
body.push('\n');
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "fix: validate port and IPv6 bracket tail..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/filesystem/url_opener.rs (1)
76-83: Add rationale comment for Windows status handling.
run_launcheron Windows intentionally ignores exit status; adding the same explanatory comment used insrc-tauri/src/adapters/driven/filesystem/file_opener.rswould prevent future “fixes” that reintroduce false failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/filesystem/url_opener.rs` around lines 76 - 83, The Windows implementation of run_launcher currently discards the process exit status but lacks the explanatory rationale; update the #[cfg(target_os = "windows")] fn run_launcher(program: &str, args: &[std::ffi::OsString]) to include a concise comment explaining that on Windows we intentionally do not treat non-zero/exit codes as failures (mirroring the rationale in file_opener.rs) so future maintainers won't convert the status into a DomainError—place the comment immediately above or inside run_launcher near the Command::new(...) call and keep the behavior unchanged.
🤖 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/report_broken_plugin.rs`:
- Around line 53-56: The current code aborts on opener.open_url errors so
issue_url never gets returned; change the behavior in the block that spawns the
blocking call (the use of url_for_browser, opener.open_url, and the map_err ->
AppError::Plugin path) to make launcher failures non-fatal and always return the
generated URL plus an open-status indicator. Concretely, replace the fallible
map_err/?? chain with code that captures the Result from opener.open_url (or the
JoinHandle error), maps it to a boolean or a small enum (e.g., opened: bool or
ReportLaunchStatus), and then return a structured outcome containing issue_url
and that status instead of erroring out via AppError::Plugin. Ensure the
function signature/result type is adjusted accordingly so callers receive both
the URL (issue_url) and whether opener.open_url succeeded.
- Around line 27-34: The current lookup uses self.plugin_loader().list_loaded()
and returns AppError::NotFound if the plugin isn't loaded, which prevents
reporting installed-but-not-loaded plugins; change the resolution to first try
finding the plugin in installed manifests (e.g., a list_installed() or the
repository of installed manifests) by matching cmd.plugin_name, and only fall
back to list_loaded() if necessary, so that a manifest for an installed plugin
(even if failed to load) is returned instead of raising AppError::NotFound;
update references around manifest, plugin_loader(), list_loaded(),
cmd.plugin_name, and the NotFound error to reflect the new search order.
In `@src/views/PluginsView.tsx`:
- Around line 101-116: The report flow currently sends only pluginName and
ignores the mutation result URL; update the call sites that invoke
reportBrokenMutation.mutate (the UI path that only passes pluginName) to include
logLines and testedUrl collected from the component state/console output so the
backend gets prefilled diagnostics, and modify the reportBrokenMutation handlers
to use the returned value (the mutation success payload/URL from
reportBrokenMutation) — onSuccess should receive and open/use that URL and also
expose it to the UI as a fallback (e.g., show a copy-to-clipboard/link button)
when window.open fails; look for the reportBrokenMutation definition and the
mutate invocation(s) to implement these changes.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/filesystem/url_opener.rs`:
- Around line 76-83: The Windows implementation of run_launcher currently
discards the process exit status but lacks the explanatory rationale; update the
#[cfg(target_os = "windows")] fn run_launcher(program: &str, args:
&[std::ffi::OsString]) to include a concise comment explaining that on Windows
we intentionally do not treat non-zero/exit codes as failures (mirroring the
rationale in file_opener.rs) so future maintainers won't convert the status into
a DomainError—place the comment immediately above or inside run_launcher near
the Command::new(...) call and keep the behavior unchanged.
🪄 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: 01bb8fab-ca6a-4b90-ade7-34f47b924651
📒 Files selected for processing (17)
CHANGELOG.mdsrc-tauri/src/adapters/driven/filesystem/mod.rssrc-tauri/src/adapters/driven/filesystem/url_opener.rssrc-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/command_bus.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/report_broken_plugin.rssrc-tauri/src/domain/model/plugin.rssrc-tauri/src/domain/ports/driven/mod.rssrc-tauri/src/domain/ports/driven/url_opener.rssrc-tauri/src/lib.rssrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/views/PluginsView.tsxsrc/views/PluginsView/PluginStoreRow.tsxsrc/views/PluginsView/__tests__/PluginStoreRow.test.tsx
Comment on lines
+101
to
+116
| const reportBrokenMutation = useTauriMutation< | ||
| string, | ||
| { pluginName: string; logLines?: string[]; testedUrl?: string } | ||
| >("plugin_report_broken", { | ||
| onSuccess: (_url, variables) => { | ||
| toast.success(t("plugins.toast.reportBrokenSuccess", { name: variables.pluginName })); | ||
| }, | ||
| onError: (error, variables) => { | ||
| toast.error( | ||
| t("plugins.toast.reportBrokenError", { | ||
| name: variables.pluginName, | ||
| reason: error.message, | ||
| }), | ||
| ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Report flow drops diagnostics and does not use the returned issue URL.
Line 232 only sends pluginName, so logLines/testedUrl are never populated from the UI path. Also, Line 105 ignores the returned URL, so this component currently has no manual fallback path (e.g., copy URL) when opening behavior is unreliable.
This undercuts the intended “pre-filled diagnostic issue + fallback URL” workflow.
Also applies to: 232-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/PluginsView.tsx` around lines 101 - 116, The report flow currently
sends only pluginName and ignores the mutation result URL; update the call sites
that invoke reportBrokenMutation.mutate (the UI path that only passes
pluginName) to include logLines and testedUrl collected from the component
state/console output so the backend gets prefilled diagnostics, and modify the
reportBrokenMutation handlers to use the returned value (the mutation success
payload/URL from reportBrokenMutation) — onSuccess should receive and open/use
that URL and also expose it to the UI as a fallback (e.g., show a
copy-to-clipboard/link button) when window.open fails; look for the
reportBrokenMutation definition and the mutate invocation(s) to implement these
changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 17 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/PluginsView.tsx">
<violation number="1" location="src/views/PluginsView.tsx:232">
P2: The report-broken action only sends `pluginName`, so generated GitHub issues miss optional diagnostics (`testedUrl`, recent log lines) that the command supports.</violation>
<violation number="2" location="src/views/PluginsView.tsx:232">
P2: Only pass `onReportBroken` for plugins that actually expose a repository URL; otherwise the menu advertises an action that immediately fails for unsupported manifests.</violation>
</file>
<file name="src-tauri/src/adapters/driven/filesystem/url_opener.rs">
<violation number="1" location="src-tauri/src/adapters/driven/filesystem/url_opener.rs:39">
P1: Windows URL launching uses `cmd /C start` with an unescaped URL, so GitHub issue URLs containing `&`/`%` can be misparsed by `cmd` and fail to open correctly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| onEnable={(name) => enableMutation.mutate({ name })} | ||
| onUninstall={(name) => uninstallMutation.mutate({ name })} | ||
| onConfigure={(name) => setConfigPluginName(name)} | ||
| onReportBroken={(name) => reportBrokenMutation.mutate({ pluginName: name })} |
There was a problem hiding this comment.
P2: The report-broken action only sends pluginName, so generated GitHub issues miss optional diagnostics (testedUrl, recent log lines) that the command supports.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/views/PluginsView.tsx, line 232:
<comment>The report-broken action only sends `pluginName`, so generated GitHub issues miss optional diagnostics (`testedUrl`, recent log lines) that the command supports.</comment>
<file context>
@@ -213,6 +229,7 @@ export function PluginsView() {
onEnable={(name) => enableMutation.mutate({ name })}
onUninstall={(name) => uninstallMutation.mutate({ name })}
onConfigure={(name) => setConfigPluginName(name)}
+ onReportBroken={(name) => reportBrokenMutation.mutate({ pluginName: name })}
hasConfig={hasConfig(entry.name)}
isInstalling={isInstalling(entry.name)}
</file context>
- Frontend gates the menu item on `entry.repository` so plugins without a GitHub manifest no longer surface a button that immediately fails. - Handler falls back to `find_installed_manifest` when `list_loaded` doesn't have the plugin — broken (failed-to-load) plugins are the whole point of the action and must be reportable. - Launcher failure is now non-fatal: the URL is always returned so the frontend can offer a clipboard fallback when the OS browser is unavailable. - Frontend writes the returned URL to the clipboard on success and surfaces a dedicated toast acknowledging the copy. - Windows launcher swaps `cmd /C start` for `rundll32 url.dll,FileProtocolHandler` so URLs containing `&` / `%` reach the shell-execute call intact, plus a comment explaining why the exit status is intentionally ignored.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/views/PluginsView/__tests__/PluginStoreRow.test.tsx (1)
99-111: Consider extracting a tiny helper to open the kebab menu.The same interaction is repeated multiple times; a local helper would reduce duplication and improve readability.
Refactor sketch
+async function openActionsMenu(user: ReturnType<typeof userEvent.setup>) { + await user.click( + screen.getByRole("button", { name: /(more actions|plus d['\u2019]actions)/i }), + ); +} ... - await user.click( - screen.getByRole("button", { name: /(more actions|plus d['\u2019]actions)/i }), - ); + await openActionsMenu(user); ... - await user.click(screen.getByRole("button", { name: /(more actions|plus d['’]actions)/i })); + await openActionsMenu(user);Also applies to: 134-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PluginsView/__tests__/PluginStoreRow.test.tsx` around lines 99 - 111, Extract a small helper (e.g., openKebabMenu) to encapsulate the repeated interaction that opens the kebab menu: it should accept the userEvent instance (user) and call user.click(screen.getByRole("button", { name: /(more actions|plus d['\u2019]actions)/i })); then replace the duplicated calls in PluginStoreRow.test.tsx (the tests that use user.click(...getByRole button...) around the onDisable and uninstall assertions and the similar block at lines 134-156) with await openKebabMenu(user) to reduce duplication and improve readability while keeping the rest of each test unchanged.
🤖 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/driven/filesystem/url_opener.rs`:
- Around line 56-64: The current validate_http_url function only checks the
prefix and lets malformed scheme-only URLs (like "https://") through; update
validate_http_url to parse the string with url::Url::parse, ensure the scheme is
"http" or "https" and that Url::host() (or authority) is Some, returning Ok(())
only if parse succeeds and a host exists, otherwise return
DomainError::ValidationError with the offending URL in the message; reference
the validate_http_url function and DomainError type when making this change.
In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs`:
- Around line 259-264: The code builds dir = self.plugins_dir.join(name) and
then reads parse_manifest(&dir), but dir can be a symlink escaping the plugins
root; update the flow in the function that uses dir (reference: plugins_dir,
name, dir, parse_manifest) to canonicalize both the plugins_dir and the
candidate dir (using std::fs::canonicalize), verify the canonicalized dir
starts_with the canonicalized plugins_dir root, and if not return Ok(None); only
after this containment check proceed with is_dir and parse_manifest to prevent
symlink escape.
---
Nitpick comments:
In `@src/views/PluginsView/__tests__/PluginStoreRow.test.tsx`:
- Around line 99-111: Extract a small helper (e.g., openKebabMenu) to
encapsulate the repeated interaction that opens the kebab menu: it should accept
the userEvent instance (user) and call user.click(screen.getByRole("button", {
name: /(more actions|plus d['\u2019]actions)/i })); then replace the duplicated
calls in PluginStoreRow.test.tsx (the tests that use user.click(...getByRole
button...) around the onDisable and uninstall assertions and the similar block
at lines 134-156) with await openKebabMenu(user) to reduce duplication and
improve readability while keeping the rest of each test unchanged.
🪄 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: d53b5168-d9f7-49cb-91cd-2b8252be2433
📒 Files selected for processing (11)
src-tauri/src/adapters/driven/filesystem/url_opener.rssrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/application/commands/report_broken_plugin.rssrc-tauri/src/domain/ports/driven/plugin_loader.rssrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/types/plugin-store.tssrc/views/PluginsView.tsxsrc/views/PluginsView/__tests__/PluginStoreRow.test.tsxsrc/views/PluginsView/__tests__/groupByCategory.test.tssrc/views/PluginsView/__tests__/usePluginStore.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/views/PluginsView/tests/groupByCategory.test.ts
- src/views/PluginsView/tests/usePluginStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/i18n/locales/en.json
- src/i18n/locales/fr.json
- src/views/PluginsView.tsx
There was a problem hiding this comment.
2 issues found across 11 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/PluginsView.tsx">
<violation number="1" location="src/views/PluginsView.tsx:111">
P2: Guard `navigator.clipboard` before calling `.writeText`; otherwise this can throw synchronously and break the success flow.</violation>
</file>
<file name="src-tauri/src/adapters/driven/plugin/extism_loader.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/extism_loader.rs:263">
P2: Using `parse_manifest` here incorrectly treats installed plugins with broken/missing `.wasm` as “not installed”, so the broken-plugin report flow can fail for exactly the failure mode it should support.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- `validate_http_url` rejects scheme-only and missing-authority shapes (`https://`, `https:///foo`, `https://?x`) and any whitespace, so garbage URLs fail at validation instead of bubbling up as a useless launcher error. - `find_installed_manifest` now uses a metadata-only manifest parser that doesn't require `<plugin>.wasm` on disk — the broken-plugin flow needs to surface plugins whose binary is missing or corrupted. - The same path canonicalizes both `plugins_dir` and the candidate plugin directory and verifies containment before reading anything, so a hostile symlink can't make the loader read a manifest from outside the plugins root. - Frontend guards `navigator.clipboard` before calling `.writeText`: the API is undefined in non-secure contexts and falling back to the no-copy toast is preferable to a synchronous throw.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/plugin/extism_loader.rs (1)
251-298: Add unit tests forfind_installed_manifestmethod.This method contains several critical security and error-handling paths that should be covered by tests:
- Valid plugin lookup returning
Some(PluginInfo)- Invalid name validation (empty, contains
/,\,..) returningValidationError- Non-existent directory returning
Ok(None)- Symlink escape attempt returning
ValidationError- Missing/corrupt manifest returning
Ok(None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs` around lines 251 - 298, Add unit tests exercising find_installed_manifest on the key paths: create a temp plugins_dir and test (1) valid plugin returning Some(PluginInfo) by writing a correct manifest metadata file and asserting the returned PluginInfo matches; (2) invalid name inputs ("", names containing '/', '\\', or "..") by calling find_installed_manifest and asserting a DomainError::ValidationError is returned; (3) non-existent plugin directory returns Ok(None); (4) symlink escape by creating plugins_dir/<name> as a symlink to a directory outside the canonical plugins_dir and assert DomainError::ValidationError is returned; and (5) missing/corrupt manifest by placing a plugin directory without a valid manifest (or with corrupted metadata) and asserting it yields Ok(None); use the find_installed_manifest method, parse_manifest_metadata behavior (metadata-only parsing), tempdir utilities and canonicalize semantics to set up each scenario and ensure cleanup.
🤖 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/driven/filesystem/url_opener.rs`:
- Around line 56-79: The validate_http_url function currently only checks simple
prefixes and leading chars, but still permits hostless authorities like
"https://:443/path" or "https://@/x"; update validate_http_url to extract the
authority portion (take rest up to the first '/', '?' or '#') and reject if that
authority is empty or begins with ':' or '@', or ends with '@' (e.g., ":443" or
"@/x" or "user@/x"), and also preserve existing whitespace checks and error type
DomainError::ValidationError; update the error message in the same
ValidationError path to reference the original url string.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs`:
- Around line 251-298: Add unit tests exercising find_installed_manifest on the
key paths: create a temp plugins_dir and test (1) valid plugin returning
Some(PluginInfo) by writing a correct manifest metadata file and asserting the
returned PluginInfo matches; (2) invalid name inputs ("", names containing '/',
'\\', or "..") by calling find_installed_manifest and asserting a
DomainError::ValidationError is returned; (3) non-existent plugin directory
returns Ok(None); (4) symlink escape by creating plugins_dir/<name> as a symlink
to a directory outside the canonical plugins_dir and assert
DomainError::ValidationError is returned; and (5) missing/corrupt manifest by
placing a plugin directory without a valid manifest (or with corrupted metadata)
and asserting it yields Ok(None); use the find_installed_manifest method,
parse_manifest_metadata behavior (metadata-only parsing), tempdir utilities and
canonicalize semantics to set up each scenario and ensure cleanup.
🪄 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: 6e4815d6-9464-4ea5-a036-9cddf8ae9efc
📒 Files selected for processing (4)
src-tauri/src/adapters/driven/filesystem/url_opener.rssrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/manifest.rssrc/views/PluginsView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/PluginsView.tsx
- src-tauri/src/adapters/driven/plugin/manifest.rs
There was a problem hiding this comment.
1 issue found across 4 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-tauri/src/adapters/driven/filesystem/url_opener.rs">
<violation number="1" location="src-tauri/src/adapters/driven/filesystem/url_opener.rs:68">
P3: Reject URLs with an empty authority host before launching. Cases like `https://:443/path` and `https://@/x` currently pass this check even though HTTP(S) URLs require a non-empty host.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Forms like `https://:443/path` and `https://user@/x` survived the prefix / leading-char check even though they carry no usable host. Strip userinfo, then look at `host[:port]`, with an extra arm for IPv6 literals so `[]` and an unclosed `[` get rejected too. Tests cover the new rejection paths plus a few authorities (userinfo, IPv6) that must still pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/filesystem/url_opener.rs`:
- Around line 85-94: The current host_missing check only ensures a host string
exists but doesn't reject malformed authorities (e.g., non-numeric ports or
stray chars after an IPv6 `]`), so update the validation around host_port,
host_missing and the IPv6 branch: when host_port starts with '[' require a
matching ']' and ensure any characters following the ']' are either empty or a
single ':' followed by a non-empty all-digit port; when not IPv6, split on the
last ':' and ensure the portion after the last ':' (if present) is all digits (a
valid port) and the host portion is non-empty; if any of these checks fail,
treat the authority as missing/invalid (set host_missing = true) so malformed
inputs like `:abc` or `[::1]oops` are rejected early.
🪄 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: 6fdc318f-ecd5-44c5-9bd4-b626673705e6
📒 Files selected for processing (1)
src-tauri/src/adapters/driven/filesystem/url_opener.rs
`https://example.com:abc/x` and `https://[::1]oops/x` slipped past the empty-host check because the validator only looked at "is the host present". Parse the authority into `(host, port)` instead: require digits-only port when present, and only accept `:port` or nothing after the IPv6 closing bracket. Tests cover the new rejections.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/filesystem/url_opener.rs (1)
114-119: Consider enforcing valid TCP port range in validation.On Line 115, digit-only validation still allows out-of-range ports (for example,
:999999). Parsing asu16keeps failures in validation instead of deferring to launcher/browser behavior.🔧 Suggested patch
- if let Some(p) = port - && (p.is_empty() || !p.bytes().all(|b| b.is_ascii_digit())) + if let Some(p) = port + && p.parse::<u16>().is_err() { return Err(DomainError::ValidationError(format!( - "http(s) URL has non-numeric port: '{url}'" + "http(s) URL has invalid port: '{url}'" ))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/filesystem/url_opener.rs` around lines 114 - 119, The current validation only checks that the port string contains digits which allows out-of-range values like ":999999"; change the check on the extracted port (variable port) to attempt parsing it as u16 (e.g., using str::parse::<u16>()) and return DomainError::ValidationError with the same message when parsing fails so only valid TCP ports 0–65535 are accepted; update the branch where port is inspected (the block that now checks p.is_empty() || !p.bytes().all(|b| b.is_ascii_digit())) to instead reject empty strings and any port that fails parse::<u16>(), keeping the same error message format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/adapters/driven/filesystem/url_opener.rs`:
- Around line 114-119: The current validation only checks that the port string
contains digits which allows out-of-range values like ":999999"; change the
check on the extracted port (variable port) to attempt parsing it as u16 (e.g.,
using str::parse::<u16>()) and return DomainError::ValidationError with the same
message when parsing fails so only valid TCP ports 0–65535 are accepted; update
the branch where port is inspected (the block that now checks p.is_empty() ||
!p.bytes().all(|b| b.is_ascii_digit())) to instead reject empty strings and any
port that fails parse::<u16>(), keeping the same error message format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d75864d9-da5d-460e-853b-c59c8a823025
📒 Files selected for processing (1)
src-tauri/src/adapters/driven/filesystem/url_opener.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
• Plugin Store rows now expose "Report broken plugin" menu action
• Opens pre-filled GitHub issue with diagnostic metadata (Vortex version, OS, plugin version, optional tested URL, last 50 log lines)
• New
UrlOpenerdomain port with platform-native implementation (xdg-open on Linux, open on macOS, cmd start on Windows)• RFC 3986 percent encoder in domain layer to safely build GitHub URLs without external dependencies
• Plugin manifest gains optional
[plugin].repositoryfield (backward compatible) for GitHub repository reference• 11 new Rust tests for URL builder and command handler, 2 new Vitest tests for row menu
• All tests pass: 909 cargo, 582 vitest
Type
feat
Summary by cubic
Adds a “Report broken plugin” action in the Plugin Store that opens a pre-filled GitHub issue with diagnostics. Implements Task 16 and tightens http(s) URL validation (now rejects empty/missing authority, whitespace, malformed IPv6, and non-numeric ports); always returns the issue URL so the UI can fall back to clipboard.
UrlOpener(xdg-open/open/rundll32 url.dll,FileProtocolHandler), http(s) only with strict validation (rejects missing/empty authority, whitespace, malformed IPv6 bracket tails, and non-numeric ports)..gitsuffix accepted.[plugin].repositoryparsed intorepository_url; action renders only when present.find_installed_manifestand uses a metadata-only parser when the plugin isn’t loaded or.wasmis missing; canonicalizes paths and enforces plugins-dir containment to prevent symlink escapes.plugin_report_broken(pluginName, logLines?, testedUrl?) -> string; always returns the issue URL for clipboard fallback; tests cover URL builder, handler (including launcher failures and authority/IPv6/port validation), and menu rendering.Written for commit cd0fe04. Summary will update on new commits.
Summary by CodeRabbit
New Features
i18n
Tests
Documentation