refactor(plugins): load bundled plugins from resource dir (closes #280)#285
Merged
Conversation
Pre-1.5.1, `ensure_bundled_plugins` copied every bundled plugin (`manifest.toml` + `plugin.wasm`) from the installer's resource tree into `<app-data>/plugins/<id>/` at every boot. That gave us one uniform install root for both bundled and sideloaded plugins at the cost of ~150 KB duplicated per id per install, ~10 ms of boot-time IO, and one bad user-facing surprise: anybody who went folder spelunking found `web-radio` sitting in two places (`%LocalAppData%\WaveFlow\plugins\` AND `%AppData%\Roaming\app.waveflow\waveflow\plugins\`) and rightly asked "why?" (issue #280). This change moves bundled plugins to resolve directly from `BaseDirectory::Resource` and eliminates the copy entirely. ## What moves - `BUNDLED_PLUGINS` + `is_bundled_plugin` shift from `crates/app/src/state.rs` into `waveflow_core::plugin`. They're a static fact about which ids are first-party, and core needs to know to route path resolution. The desktop's existing callsite (`commands::plugins::manifest_to_info` for the `bundled: bool` badge + `uninstall_plugin`'s refusal gate) re-imports from core unchanged. - `PluginPaths` gains an optional `bundled_root: Option<PathBuf>` field + a `with_bundled_root` builder. Path-resolution methods (`plugin_dir`, `manifest_path`, `wasm_path`, `assets_dir`) route bundled ids under `bundled_root` when present, sideloaded ids under `plugins_root`. `state_dir` always targets `data_root` regardless of bundling — user state stays in the writable app-data tree so a bundled-plugin upgrade can't lose it. - `AppPaths::from_handle` resolves `BaseDirectory::Resource / plugins` once at startup + stores it as `bundled_plugins_dir`. Failure (dev builds, broken installs) is logged at WARN + recorded as `None` so bundled resolution falls back to the writable tree — pre-1.5.1 behaviour, kept as a safety net. - `AppPaths::plugin_paths()` injects `bundled_plugins_dir` into the `PluginPaths` it returns. Every existing callsite that pulls `PluginPaths` (the wasmtime runtime, every command in `commands/plugins.rs`) gets the new routing transparently. ## What's removed - `ensure_bundled_plugins(handle, paths)` — the copy that was the whole reason `<app-data>/plugins/web-radio/` ever existed. - Its `BaseDirectory` + `Manager` imports from `state.rs` (now resolved in `paths.rs`). ## What's added - `cleanup_bundled_plugin_leftovers(paths)` — one-shot cleanup that drops any subdirectory of `<app-data>/plugins/` whose name is in `BUNDLED_PLUGINS`. Handles the 1.5.0 → 1.5.1 transition for users whose disk already carries the pre-refactor copies. Idempotent: a 1.5.1 fresh install finds nothing to remove. - `walk_install_root(root)` helper — pulled out of `list_installed_plugins` so the same parse + dir-id-pin logic serves both the bundled walk and the sideloaded walk. - `list_installed_plugins` now walks BOTH the bundled tree (when resolvable) AND the sideloaded tree, with bundled winning on any id collision (defence-in-depth — post-cleanup collisions shouldn't happen, but a stray sideloaded copy of a reserved id shouldn't load the wrong .wasm). ## Tests Four new unit tests in `core::plugin::tests`: - `bundled_plugin_routes_to_bundled_root` pins the bundled-id -> resource-path routing and confirms `state_dir` still hits the writable tree. - `sideloaded_plugin_stays_in_plugins_root_even_with_bundled_set` confirms non-bundled ids ignore the resource root. - `bundled_plugin_falls_back_to_plugins_root_without_bundled_set` covers the `bundled_root: None` fallback for tests + dev builds. - `is_bundled_plugin_only_matches_known_ids` checks case- sensitivity + the empty-string edge. All 40 `plugin::` unit tests + 84 desktop unit tests pass; full workspace `cargo check --all-targets` is warning-free. ## Disk footprint `%LocalAppData%\WaveFlow\plugins\web-radio\` (resource dir, 143 KB): the installer's read-only copy, source of truth, unchanged. `%AppData%\Roaming\app.waveflow\waveflow\plugins\web-radio\` (was the writable copy): no longer created. Existing copies from a prior 1.5.0 install are removed at boot by the cleanup pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDéplace ChangesRoutage des plugins bundled et refactor du scan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
InstaZDLL
added
scope: backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/crates/app/src/paths.rs`:
- Around line 64-66: The bundled_plugins_dir assignment in the
handle.path().resolve() match block does not validate that the resolved
directory actually exists on disk. After obtaining the path from resolve(), you
must verify the directory exists using standard file system checks (exists() and
is_dir() methods) before assigning it to Some(path). If the directory does not
exist, set bundled_plugins_dir to None instead so that the documented fallback
mechanism to the root plugins directory can be activated. This ensures that
missing or improperly packaged bundled plugin directories do not break plugin
loading.
In `@src-tauri/crates/app/src/state.rs`:
- Around line 125-126: The cleanup_bundled_plugin_leftovers function is being
called unconditionally at line 125, which can delete the compatibility fallback
directory when no valid bundled root is available. Check if a valid bundled root
path exists before calling cleanup_bundled_plugin_leftovers. Add a condition to
verify that the bundled plugin root is available and valid before proceeding
with the cleanup operation in the deletion loop (around lines 381-389) to
prevent removing the fallback plugin directory like web-radio that should be
preserved for compatibility.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fbe087b4-d58d-4343-b7c3-4ea352ae165c
📒 Files selected for processing (4)
src-tauri/crates/app/src/commands/plugins.rssrc-tauri/crates/app/src/paths.rssrc-tauri/crates/app/src/state.rssrc-tauri/crates/core/src/plugin/mod.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.
Draft pour test manuel — refactor de l'architecture plugins bundled pour éliminer la duplication signalée dans #280.
Closes #280
Le bug
Avant 1.5.1,
ensure_bundled_pluginscopiait chaque plugin bundled deBaseDirectory::Resourcevers `/plugins//` à chaque boot. Résultat : sur Windows on retrouvait `web-radio` dans DEUX dossiers :Exactement 143 KB dupliqués (manifest.toml 1,6 KB + plugin.wasm 142 KB), pour rien fonctionnellement — le runtime n'a JAMAIS lu depuis Roaming après la première copie.
Le fix
Le runtime résout les plugins bundled directement depuis le resource dir. Plus de copy au boot.
Changes
core/plugin/mod.rsBUNDLED_PLUGINS+is_bundled_pluginmovés depuisapp/state.rs.PluginPathsgagnebundled_root: Option<PathBuf>+with_bundled_rootbuilder. Path resolution route bundled ids versbundled_root, sideloaded versplugins_root.state_dirreste toujours dans la writable tree.app/paths.rsAppPaths::from_handlerésoutBaseDirectory::Resource / pluginsau startup, stocke commebundled_plugins_dir: Option<PathBuf>. Failure logged WARN + recorded commeNone(dev builds, broken installs) — fallback safe.app/state.rsensure_bundled_pluginssupprimé.cleanup_bundled_plugin_leftoversajouté : drop tout subdir de<app-data>/plugins/dont le nom est dansBUNDLED_PLUGINS. Idempotent — 1.5.1 fresh install ne trouve rien à remove.app/commands/plugins.rslist_installed_pluginswalke les DEUX roots (bundled + sideloaded) avec bundled wins on collision. Helperwalk_install_rootextrait. Re-importis_bundled_plugindepuis core.Compatibility
<app-data>/plugins/reste vide jusqu'à ce que l'user sideload qqch.<app-data>/plugin-data/web-radio/) intact.BaseDirectory::Resource / pluginsn'est pas résolvable, fallback vers `/plugins/` — comportement pre-refactor preservé.Tests
4 nouveaux tests unitaires dans
core::plugin::tests:bundled_plugin_routes_to_bundled_root: bundled id → resource path.state_dirreste writable.sideloaded_plugin_stays_in_plugins_root_even_with_bundled_set: ids non-bundled ignorent le resource root.bundled_plugin_falls_back_to_plugins_root_without_bundled_set: fallback dev/test sans bundled_root.is_bundled_plugin_only_matches_known_ids: case-sensitive + empty edge.40 tests
plugin::+ 84 tests desktop unit passent.cargo check --all-targetsworkspace : zéro warning.Test plan
bun run tauri dev) : Web Radio loading + listing OK<app-data>/plugins/reste vide après boot fresh/usr/lib/WaveFlow/plugins/)Risks
BaseDirectory::Resourceest Tauri-géré donc devrait marcher cross-OS, mais on n'a pas testé en CI sur Linux .deb / .rpm yet. À tester manuellement.<resource>/plugins/<id>/plugin.wasmau lieu de<app-data>/plugins/.... Le file open + checkis_file()est unchanged dansruntime.rs:283-287. Sur Linux /usr/lib/ est world-readable donc OK ; sur macOS .app bundle Resources/ pareil.Summary by CodeRabbit
Améliorations
Chores