fix(plugin): #115 fall back to registry repository for report_broken - #118
Conversation
Plugin report_broken IPC errored on every official plugin because the runtime requires `[plugin].repository` in `plugin.toml` but none of the shipped manifests carried it. The handler now falls back to the local plugin store cache (`PluginStoreEntry.repository`) when the manifest is missing the field, so already-installed plugins keep working until they upgrade. The validation message points at the actual TOML field rather than the internal Rust struct name. Bumps registry to the new plugin versions: - vortex-mod-youtube 1.2.3 → 1.2.4 - vortex-mod-vimeo 1.3.1 → 1.3.2 - vortex-mod-soundcloud 1.2.1 → 1.2.2 - vortex-mod-gallery 1.0.0 → 1.0.1 Refs #115.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request implements a fallback mechanism for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src-tauri/src/application/commands/report_broken_plugin.rs (1)
116-120: Normalize cachedrepositoryvalues before use.At Line 116-120, trimming whitespace avoids avoidable validation failures from slightly malformed cache data.
♻️ Suggested tweak
- let repo = entry.get("repository")?.as_str()?; - if repo.is_empty() { + let repo = entry.get("repository")?.as_str()?.trim(); + if repo.is_empty() { None } else { Some(repo.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/report_broken_plugin.rs` around lines 116 - 120, Normalize the cached repository string before validation: when retrieving the value from entry.get("repository")?.as_str()? assign or use a trimmed version (e.g., call .trim() on the &str), then check trimmed.is_empty() and return Some(trimmed.to_string()) when non-empty; update the repo handling in report_broken_plugin logic to use the trimmed value so leading/trailing whitespace in cached "repository" entries doesn't cause false empty checks.src-tauri/src/adapters/driving/tauri_ipc.rs (1)
626-626: Keep a trace when cache-path resolution fails.At Line 626,
.ok()silently discards the error. Logging it (debug/warn) would make fallback behavior easier to diagnose without changing runtime semantics.♻️ Suggested tweak
- let store_cache_path = store_cache_path().ok(); + let store_cache_path = match store_cache_path() { + Ok(path) => Some(path), + Err(error) => { + tracing::debug!( + error = %error, + "plugin_report_broken: store cache path unavailable; continuing without cache fallback" + ); + None + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driving/tauri_ipc.rs` at line 626, The call to store_cache_path() currently swallows errors with .ok(); change it to capture the Result, log the Err before falling back, and keep the same semantics (None on error). Specifically, replace the .ok() usage around the store_cache_path() call so you match on its Result (or use .map_err(|e| { debug!/warn!(...), None })) and log the error via your crate logger (e.g., debug! or warn!) while still assigning None to the variable store_cache_path on failure; reference the store_cache_path() function and the store_cache_path variable to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Line 626: The call to store_cache_path() currently swallows errors with .ok();
change it to capture the Result, log the Err before falling back, and keep the
same semantics (None on error). Specifically, replace the .ok() usage around the
store_cache_path() call so you match on its Result (or use .map_err(|e| {
debug!/warn!(...), None })) and log the error via your crate logger (e.g.,
debug! or warn!) while still assigning None to the variable store_cache_path on
failure; reference the store_cache_path() function and the store_cache_path
variable to locate the change.
In `@src-tauri/src/application/commands/report_broken_plugin.rs`:
- Around line 116-120: Normalize the cached repository string before validation:
when retrieving the value from entry.get("repository")?.as_str()? assign or use
a trimmed version (e.g., call .trim() on the &str), then check
trimmed.is_empty() and return Some(trimmed.to_string()) when non-empty; update
the repo handling in report_broken_plugin logic to use the trimmed value so
leading/trailing whitespace in cached "repository" entries doesn't cause false
empty checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3d7c3b4-3ed6-4b9a-ae32-0d048a6ad444
📒 Files selected for processing (5)
CHANGELOG.mdregistry/registry.tomlsrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/report_broken_plugin.rs
Plugin-side change abandoned — Path B (registry fallback in the report_broken_plugin handler) suffices for the four official plugins listed in the registry. Reverting the registry bump keeps this PR focused on the runtime fix and avoids coupling it to plugin releases.
Greptile SummaryFixes Confidence Score: 4/5Safe to merge; only a P2 doc-comment typo present, no logic or correctness issues found. The fallback logic is correct — JSON keys src-tauri/src/application/commands/mod.rs (doc comment filename mismatch)
|
| Filename | Overview |
|---|---|
| src-tauri/src/application/commands/report_broken_plugin.rs | Adds registry-cache fallback for missing manifest repository field; JSON key access is correct (both name and repository are single words, unaffected by camelCase rename); two new tests cover the happy path and the empty-cache edge case; logic is clean. |
| src-tauri/src/application/commands/mod.rs | Adds store_cache_path: Option<PathBuf> to ReportBrokenPluginCommand; doc comment names the file registry-cache.json instead of the actual plugin-registry-cache.json. |
| src-tauri/src/adapters/driving/tauri_ipc.rs | Wires store_cache_path().ok() into the command; path derivation is consistent with every other store command that uses the same helper; silently degrades to None when config dir is unavailable. |
| CHANGELOG.md | Accurate entry under [Unreleased] / Fixed describing the manifest-missing-repository bug and its cache-fallback fix. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[plugin_report_broken IPC] --> B[store_cache_path.ok]
B --> C[handle_report_broken_plugin]
C --> D{list_loaded finds plugin?}
D -- yes --> E[Use loaded PluginInfo as manifest]
D -- no --> F{find_installed_manifest?}
F -- Some --> E
F -- None --> G[AppError::NotFound]
E --> H{manifest.repository_url is Some?}
H -- yes --> K[build_report_broken_url]
H -- no --> I{store_cache_path is Some?}
I -- None --> J[AppError::Validation]
I -- Some --> L[read_repository_from_cache]
L --> M{entry found in cache?}
M -- yes --> K
M -- no --> J
K --> N[opener.open_url]
N --> O[Return issue URL]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src-tauri/src/application/commands/mod.rs
Line: 199-202
Comment:
**Incorrect filename in doc comment**
The doc comment says `` `registry-cache.json` `` but the actual file written by `store_cache_path()` is `plugin-registry-cache.json`. A reader following this comment to locate the cache on disk would look for the wrong file.
```suggestion
/// Local path to the plugin store cache (`plugin-registry-cache.json`). When
/// the plugin's manifest does not surface a `repository` field, the
/// handler falls back to this cache so plugins installed before the
/// field was required keep working.
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "revert: drop registry version bump for #..." | Re-trigger Greptile
| /// Local path to the plugin store cache (`registry-cache.json`). When | ||
| /// the plugin's manifest does not surface a `repository` field, the | ||
| /// handler falls back to this cache so plugins installed before the | ||
| /// field was required keep working. |
There was a problem hiding this comment.
The doc comment says `registry-cache.json` but the actual file written by store_cache_path() is plugin-registry-cache.json. A reader following this comment to locate the cache on disk would look for the wrong file.
| /// Local path to the plugin store cache (`registry-cache.json`). When | |
| /// the plugin's manifest does not surface a `repository` field, the | |
| /// handler falls back to this cache so plugins installed before the | |
| /// field was required keep working. | |
| /// Local path to the plugin store cache (`plugin-registry-cache.json`). When | |
| /// the plugin's manifest does not surface a `repository` field, the | |
| /// handler falls back to this cache so plugins installed before the | |
| /// field was required keep working. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/application/commands/mod.rs
Line: 199-202
Comment:
**Incorrect filename in doc comment**
The doc comment says `` `registry-cache.json` `` but the actual file written by `store_cache_path()` is `plugin-registry-cache.json`. A reader following this comment to locate the cache on disk would look for the wrong file.
```suggestion
/// Local path to the plugin store cache (`plugin-registry-cache.json`). When
/// the plugin's manifest does not surface a `repository` field, the
/// handler falls back to this cache so plugins installed before the
/// field was required keep working.
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
1 issue found across 2 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="registry/registry.toml">
<violation number="1">
P1: This change rolls back multiple official plugin versions in the registry, so clients will resolve and download older plugin releases instead of the latest published ones.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Doc comment: `registry-cache.json` → `plugin-registry-cache.json` (greptile P2 on commands/mod.rs). - Trim cached `repository` value before the empty-check so leading or trailing whitespace in cache entries doesn't trip the fallback (coderabbit nitpick on report_broken_plugin.rs). - Log a debug trace when `store_cache_path()` fails instead of silently dropping the error with `.ok()`, so the fallback path is diagnosable (coderabbit nitpick on tauri_ipc.rs).
|
@cubic-dev-ai re. registry rollback flag (P1): the version revert in 9618c1d is intentional. The four official plugin manifests in their respective repos do not declare |
@mpiton That makes total sense. I suggested checking the registry bump because typically fixing a "missing field" bug involves updating the source manifests, but your runtime fallback approach is a much cleaner way to fix this for official plugins without forcing a cascade of new plugin releases. Since The logic in I've noted that:
This is a solid middle-ground fix. Learn more about how I handle learnings | Configure my review settings |
Closes #115.
Summary
plugin_report_brokenerrored on every official plugin (4/4) because theirplugin.tomlmanifests do not declare arepositoryfield, while the handler inapplication/commands/report_broken_plugin.rsrequiresmanifest.repository_url()to beSometo build the GitHub issue URL.This PR fixes the runtime side only:
repository, the handler now readsPluginStoreEntry.repositoryfrom the local plugin store cache (plugin-registry-cache.json, populated byplugin_store_refreshfromregistry.toml). The four official plugins are listed in the registry with theirrepositoryURL, so the feature works immediately without re-publishing any plugin.[plugin].repositoryinplugin.toml) instead of the internal Rust struct fieldrepository_url.Plugin-side updates (adding
repositoryto each manifest) were considered but dropped — the registry already carries the data, so duplicating it in every manifest is redundant for officially-listed plugins. Third-party / sideloaded plugins can still shiprepositoryin their manifest for the standard path; that field is not introduced here, only relied on when present.Changes
application/commands/mod.rs:ReportBrokenPluginCommandgains an optionalstore_cache_path: Option<PathBuf>.application/commands/report_broken_plugin.rs: cache fallback + improved validation message + 2 new unit tests.adapters/driving/tauri_ipc.rs: passstore_cache_path()through to the command.CHANGELOG.md: entry under[Unreleased]/ Fixed.Test plan
cargo test --lib— 1012 passed, 4 ignored (15 inreport_broken, was 13)cargo clippy --workspace -- -D warnings— cleancargo fmt --check— cleannpx vitest run— 582 passednpm run lint— 0 warnings, 0 errorstauri-pilot ipc plugin_store_refreshthentauri-pilot ipc plugin_report_broken --args '{"pluginName":"vortex-mod-youtube"}'to confirm the GitHub issue URL is returned.Summary by CodeRabbit
Bug Fixes