various: add a generic, nix compatible plugin system - #573
Conversation
A plugin is an external executable Kopuz spawns and talks to over JSON-RPC 2.0 on stdio (newline-delimited JSON). It provides a music source: library, search, playlists, covers, sign-in and playable audio. Nothing upstream is provider-specific — name, icon, accent, capabilities, auth steps and stream URLs are all runtime data from the manifest or the handshake. The pipe is the capability: no port to allocate, no nonce to check, and process lifetime equals connection lifetime, so crash handling is plain child supervision (kill_on_drop, ping every 20s, restart with backoff, give up after 5 in 5 minutes; in-flight calls resolve as errors rather than hanging). Audio is the one exception — the player only consumes a URL, so a plugin serves its own bytes and returns that URL from resolve_stream. One MusicService::Plugin unit variant carries every plugin, so the ~20 existing match sites take one arm once instead of growing per plugin; identity rides on MusicServer::plugin_id and item ids are namespaced "<plugin_id>/<ref>" so two plugins cannot collide in the DB. crates/plugin-example is a working reference plugin serving a folder of local files, and docs/plugins.md is the protocol spec for third parties.
|
@temidaradev voila |
📝 WalkthroughWalkthroughAdds a generic external-plugin system with manifest discovery, JSON-RPC process supervision, plugin-backed persisted sources, media operations, settings authentication UI, a reference plugin, localization, tests, and documentation. ChangesPlugin source contracts and persistence
Sequence Diagram(s)sequenceDiagram
participant Settings
participant PluginRegistry
participant PluginClient
participant PluginProcess
participant PluginSource
Settings->>PluginRegistry: select plugin and begin authentication
PluginRegistry->>PluginClient: connect(plugin_id)
PluginClient->>PluginProcess: initialize
PluginProcess-->>PluginClient: capabilities and auth prompt
PluginClient-->>Settings: authentication state
Settings->>PluginClient: auth_submit
PluginClient-->>Settings: authentication completion
PluginSource->>PluginClient: media operation
PluginClient->>PluginProcess: JSON-RPC request
PluginProcess-->>PluginClient: plugin result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
im confused why does the pr title say varios it says i typed in various lol |
temidaradev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
crates/db/tests/config.rs (1)
37-61: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise a non-null plugin_id in the round-trip test.
All new values are
None, so this test does not verify the plugin column’s actual persistence path. Add a plugin server withplugin_id: Some(...)and assert it survivessave_config/load_configin both saved and active-server records.🤖 Prompt for 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. In `@crates/db/tests/config.rs` around lines 37 - 61, Update the round-trip test’s server fixtures to use a plugin server with plugin_id: Some(...) instead of None, then assert the same plugin_id is preserved after save_config/load_config in both the saved-server entry and the active server record.crates/server/src/plugin/wire.rs (1)
257-273: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWire casing is inconsistent:
ErrorKind/LogLevelaresnake_case,AuthPrompt(andCapabilities' enums) are PascalCase.
AuthPrompthas norename_all, so it goes over the wire as{"OpenUrl":{…}}/"Done"while sibling enums useopen_url-style names. Since this freezes as protocol v1, aligning now is cheap; later it is a breaking change for every installed plugin. Whichever way you settle it, make suredocs/plugins.mdstates the exact JSON shape per variant.🤖 Prompt for 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. In `@crates/server/src/plugin/wire.rs` around lines 257 - 273, Align AuthPrompt serialization with the established snake_case wire naming used by ErrorKind, LogLevel, and Capabilities enums by adding the appropriate serde variant renaming. Update docs/plugins.md to specify the exact JSON representation for every AuthPrompt variant, including payload field shapes, and keep the protocol-v1 format consistent across implementations.crates/server/src/server_ops.rs (1)
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment is now stale for
Plugin.The comment still says an access token "is always required," but the code below now defaults it to empty for
MusicService::Plugin(line 41). Worth updating so the doc matches the new per-service exception.📝 Proposed doc update
/// Build connection params from app config for the active server, or /// `None` when a field the active service requires is missing. An access - /// token is always required; Jellyfin/Subsonic/Custom additionally require - /// a `user_id` (YouTube Music authenticates by cookie only and Spotify by - /// OAuth token only, so a missing user_id is fine for both). Centralizing this stops every UI call site from + /// token is required for every built-in service; Jellyfin/Subsonic/Custom + /// additionally require a `user_id` (YouTube Music authenticates by cookie + /// only and Spotify by OAuth token only, so a missing user_id is fine for + /// both). A plugin service owns its own credentials and requires neither, + /// but must name the plugin backing it via `plugin_id`. Centralizing this stops every UI call site from /// coercing an absent user_id into `""` and firing a malformed /// authenticated request that silently fails.🤖 Prompt for 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. In `@crates/server/src/server_ops.rs` around lines 24 - 30, Update the doc comment describing connection parameter requirements to include the new MusicService::Plugin exception: Plugin may use an empty access token, while the existing access-token requirement remains for other services. Keep the user_id requirements and other service-specific behavior documented accurately.crates/components/src/plugin_auth_popup.rs (1)
24-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
valuesoutlives the prompt it was collected for.The signal is never cleared between steps, so a multi-step wizard (e.g.
Form→Form) submits the previous step's keys — including secrets — alongside the new ones. Clearing on prompt change keeps each submission scoped to the fields the plugin actually asked for.Also worth noting: the fields are labelled only via
placeholder, which disappears on input and isn't reliably announced; a reallabel(oraria-label) per field would be better.Also applies to: 48-67
🤖 Prompt for 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. In `@crates/components/src/plugin_auth_popup.rs` at line 24, Reset the values signal whenever the requested prompt/step changes so each submission from the plugin-auth popup contains only fields for the current prompt, including removing prior secrets. Update the generated input fields in the affected field-rendering block to provide a persistent accessible label, using a real label or aria-label instead of relying only on placeholder text.crates/plugin-example/src/main.rs (1)
204-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo size cap or timeout on the byte-server request line.
read_lineinhandle_requesthas no maximum length and no timeout; a stalled or malformed local connection would hang the spawned task indefinitely / could growrequestunbounded. Low real-world risk given loopback-only binding, but cheap to harden since other plugin authors may copy this pattern.🛡️ Proposed fix: bound the read with a timeout
- let mut request = String::new(); - BufReader::new(&mut stream).read_line(&mut request).await?; + let mut request = String::new(); + tokio::time::timeout( + std::time::Duration::from_secs(5), + BufReader::new(&mut stream).read_line(&mut request), + ) + .await + .map_err(|_| std::io::Error::new(std::io::ErrorKind::TimedOut, "request line timed out"))??;🤖 Prompt for 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. In `@crates/plugin-example/src/main.rs` around lines 204 - 243, Harden the request-line read in handle_request by enforcing a small maximum request size and applying a read timeout around BufReader::read_line. Reject or close the connection when the timeout expires, the line exceeds the limit, or the read is otherwise malformed, while preserving the existing routing and 404/200 response behavior for valid requests.docs/plugins.md (1)
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd languages to the fenced code blocks.
The two fences are missing language identifiers, triggering Markdown lint warnings. Use
textfor the directory tree andshellfor the environment-variable example.Also applies to: 49-51
🤖 Prompt for 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. In `@docs/plugins.md` around lines 37 - 42, Update the fenced code blocks in the plugins documentation to include language identifiers: use text for the directory-tree block shown near the plugins example and shell for the environment-variable example. Apply the same change to both referenced fences without altering their contents.Source: Linters/SAST tools
🤖 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 `@crates/hooks/src/source_switch.rs`:
- Around line 72-75: Validate the Plugin source identity in the activation flow
before writing the active snapshot: when saved.service is MusicService::Plugin,
require saved.plugin_id to be present and reject or return an error if it is
missing. Update the logic around is_plugin and the activation path near Lines
97-104 so incomplete Plugin sources are never activated and the failure is
surfaced to the caller.
In `@crates/i18n/locales/es.ftl`:
- Around line 514-529: Translate the plugin block values, preserving every
Fluent key and placeholder, in crates/i18n/locales/es.ftl lines 514-529
(Spanish), crates/i18n/locales/fil.ftl lines 507-522 (Filipino),
crates/i18n/locales/fr.ftl lines 514-529 (French), crates/i18n/locales/gr.ftl
lines 513-528 (Greek), crates/i18n/locales/tr.ftl lines 513-528 (Turkish),
crates/i18n/locales/uk.ftl lines 513-528 (Ukrainian),
crates/i18n/locales/vi-VN.ftl lines 507-522 (Vietnamese), and
crates/i18n/locales/zh-CN.ftl lines 513-528 (Simplified Chinese), keeping all
locales in parity with en.ftl.
In `@crates/i18n/locales/he.ftl`:
- Around line 514-528: Localize the plugin UI keys from plugins through
continue_action in every affected locale: crates/i18n/locales/he.ftl lines
514-528, crates/i18n/locales/hu.ftl lines 514-528, crates/i18n/locales/id.ftl
lines 508-522, crates/i18n/locales/it.ftl lines 507-521,
crates/i18n/locales/ja.ftl lines 520-534, crates/i18n/locales/ko.ftl lines
514-528, crates/i18n/locales/ml.ftl lines 508-522, crates/i18n/locales/nl.ftl
lines 508-522, crates/i18n/locales/pl.ftl lines 514-528, and
crates/i18n/locales/pt-BR.ftl lines 513-527. Provide natural translations
matching each locale, preserve the Fluent placeholders { $version } and { $id },
and keep all keys in parity with en.ftl using only referenced keys.
In `@crates/kopuz/src/main.rs`:
- Around line 609-623: Move the plugin teardown block using shutdown_all() out
of the CloseRequested handling and into the actual application shutdown path,
such as the Event::LoopDestroyed branch, so tray-mode window hides do not
terminate live plugins. Update shutdown() to enforce a bounded grace/kill
deadline before returning, ensuring the spawned-thread join cannot block the UI
indefinitely on a wedged plugin.
In `@crates/pages/src/settings_actions.rs`:
- Around line 534-574: Update finish_step to apply auth_begin/auth_submit
results only when auth_state still represents the same active flow, matching the
captured plugin_id (and flow identity as needed). Ignore stale results after
plugin_auth_cancel or when another plugin wizard has replaced the state,
including Done results so they cannot clear or nudge a newer/cancelled flow.
In `@crates/server/src/plugin/client.rs`:
- Around line 885-891: Update i18n_locale so it uses the user's selected UI
language from the existing configuration or initialization state, rather than
reading LANG from the process environment. Ensure the returned value matches
InitializeParams::locale and reflects config.language when the user changes the
language, while preserving the English fallback when no language is configured.
- Around line 686-691: Update the reader-task error handling around the Err(e)
branch and its corresponding termination path so an overlong line or read
failure fails all pending requests and forces the plugin child to restart,
rather than only returning after logging. Ensure the child process and stdin are
closed or terminated and the existing restart mechanism is triggered
immediately, preserving normal handling for successful reads.
- Around line 718-746: The data_token redaction contract is not enforced before
plugin output is logged. Update emit_plugin_log and stderr_task to receive or
access the handshake data_token, scrub every occurrence from log.message and
stderr lines before tracing them, and preserve the existing level and target
metadata while emitting the sanitized text.
- Around line 402-417: Update start_locked and next_backoff so the
exhausted/give-up result is distinguished from the initial “no delay” case,
preventing spawn_and_handshake from running after the restart budget is
exhausted while still allowing the first start. Correct the attempt-window
boundary in next_backoff so it permits exactly MAX_RESTARTS restarts, not six
when the limit is five, and preserve the existing backoff behavior for allowed
attempts.
In `@crates/server/src/plugin/mod.rs`:
- Around line 109-127: Refactor PluginManager::client so the global
inner.clients lock is held only while retrieving or updating a per-plugin
connection slot, not during PluginClient::connect(plugin_id).await. Add
per-plugin synchronization such as Arc<tokio::sync::Mutex<Option<PluginClient>>>
to deduplicate concurrent connections for the same plugin while allowing
different plugin IDs to connect concurrently; preserve exhausted-client removal,
manifest lookup, handshake recording, and client reuse behavior.
In `@crates/server/src/source/plugin.rs`:
- Around line 679-695: Cap pagination in fetch_playlist_entries by tracking the
number of fetched pages and stopping with an appropriate SourceError once the
configured maximum is reached. Increment the counter for each fetch, preserve
normal cursor termination and accumulation, and ensure a plugin returning
endless cursors cannot grow all or loop indefinitely.
- Around line 249-257: Update web_url to retain the owner returned by
plugin::split_item_id and validate it matches self.plugin_id before fetching the
URL template or constructing the URL; return None for mismatched namespaces,
consistent with strip().
---
Nitpick comments:
In `@crates/components/src/plugin_auth_popup.rs`:
- Line 24: Reset the values signal whenever the requested prompt/step changes so
each submission from the plugin-auth popup contains only fields for the current
prompt, including removing prior secrets. Update the generated input fields in
the affected field-rendering block to provide a persistent accessible label,
using a real label or aria-label instead of relying only on placeholder text.
In `@crates/db/tests/config.rs`:
- Around line 37-61: Update the round-trip test’s server fixtures to use a
plugin server with plugin_id: Some(...) instead of None, then assert the same
plugin_id is preserved after save_config/load_config in both the saved-server
entry and the active server record.
In `@crates/plugin-example/src/main.rs`:
- Around line 204-243: Harden the request-line read in handle_request by
enforcing a small maximum request size and applying a read timeout around
BufReader::read_line. Reject or close the connection when the timeout expires,
the line exceeds the limit, or the read is otherwise malformed, while preserving
the existing routing and 404/200 response behavior for valid requests.
In `@crates/server/src/plugin/wire.rs`:
- Around line 257-273: Align AuthPrompt serialization with the established
snake_case wire naming used by ErrorKind, LogLevel, and Capabilities enums by
adding the appropriate serde variant renaming. Update docs/plugins.md to specify
the exact JSON representation for every AuthPrompt variant, including payload
field shapes, and keep the protocol-v1 format consistent across implementations.
In `@crates/server/src/server_ops.rs`:
- Around line 24-30: Update the doc comment describing connection parameter
requirements to include the new MusicService::Plugin exception: Plugin may use
an empty access token, while the existing access-token requirement remains for
other services. Keep the user_id requirements and other service-specific
behavior documented accurately.
In `@docs/plugins.md`:
- Around line 37-42: Update the fenced code blocks in the plugins documentation
to include language identifiers: use text for the directory-tree block shown
near the plugins example and shell for the environment-variable example. Apply
the same change to both referenced fences without altering their contents.
🪄 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 Plus
Run ID: bba76f37-0c3d-4bd1-bc77-a5348a08eb71
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
Cargo.tomlcrates/components/src/lib.rscrates/components/src/plugin_auth_popup.rscrates/components/src/settings_items.rscrates/components/src/settings_popups.rscrates/components/src/source_switcher.rscrates/config/src/lib.rscrates/config/src/source.rscrates/config/src/views.rscrates/db/.sqlx/query-22cf3686ea40ee51b0a233fa98772afd5719a8bb9ddddfdb7697be0e9885f9a6.jsoncrates/db/.sqlx/query-314745d92b22cb77a5c4258eec5fd209516910cfa3ba9206feb68a2a3ccf8005.jsoncrates/db/.sqlx/query-8fd8eedc0b2f8b57614ea3d8b51f5e03a1084e911fd39c62647b0227814d46c4.jsoncrates/db/.sqlx/query-a21179b43b7e71626075a18ae898f49884c35284ff8f1930c370770709f0393d.jsoncrates/db/.sqlx/query-bf94c9daf8aab10c82f27a50aff9fac1888f4453a5117f6f14b5dc1f83b17cc0.jsoncrates/db/.sqlx/query-f2d42fa809c2bdb26840c7f8ef3e9b49505a455ca7d75468086091ef4ca322e2.jsoncrates/db/migrations/20260728000000_server_plugin_id.sqlcrates/db/src/backend/cfg_store.rscrates/db/src/backend/migrations.rscrates/db/src/backend/rows.rscrates/db/src/backend/writes.rscrates/db/tests/config.rscrates/db/tests/persistence.rscrates/hooks/src/playback_ref.rscrates/hooks/src/source_switch.rscrates/hooks/src/use_sync_task.rscrates/i18n/locales/ar.ftlcrates/i18n/locales/de.ftlcrates/i18n/locales/en.ftlcrates/i18n/locales/es.ftlcrates/i18n/locales/fil.ftlcrates/i18n/locales/fr.ftlcrates/i18n/locales/gr.ftlcrates/i18n/locales/he.ftlcrates/i18n/locales/hu.ftlcrates/i18n/locales/id.ftlcrates/i18n/locales/it.ftlcrates/i18n/locales/ja.ftlcrates/i18n/locales/ko.ftlcrates/i18n/locales/ml.ftlcrates/i18n/locales/nl.ftlcrates/i18n/locales/pl.ftlcrates/i18n/locales/pt-BR.ftlcrates/i18n/locales/pt-PT.ftlcrates/i18n/locales/ro.ftlcrates/i18n/locales/ru.ftlcrates/i18n/locales/sv.ftlcrates/i18n/locales/ta.ftlcrates/i18n/locales/tok-SP.ftlcrates/i18n/locales/tok.ftlcrates/i18n/locales/tr.ftlcrates/i18n/locales/uk.ftlcrates/i18n/locales/vi-VN.ftlcrates/i18n/locales/zh-CN.ftlcrates/kopuz/src/main.rscrates/pages/src/server/mod.rscrates/pages/src/settings.rscrates/pages/src/settings_actions.rscrates/plugin-example/Cargo.tomlcrates/plugin-example/src/main.rscrates/reader/src/models.rscrates/server/Cargo.tomlcrates/server/src/lib.rscrates/server/src/plugin/client.rscrates/server/src/plugin/manifest.rscrates/server/src/plugin/mod.rscrates/server/src/plugin/wire.rscrates/server/src/provider.rscrates/server/src/server_ops.rscrates/server/src/source.rscrates/server/src/source/plugin.rscrates/server/src/source/types.rscrates/server/tests/plugin_roundtrip.rsdocs/plugins.md
💤 Files with no reviewable changes (2)
- crates/db/.sqlx/query-8fd8eedc0b2f8b57614ea3d8b51f5e03a1084e911fd39c62647b0227814d46c4.json
- crates/db/.sqlx/query-bf94c9daf8aab10c82f27a50aff9fac1888f4453a5117f6f14b5dc1f83b17cc0.json
| // A plugin holds its own credentials, so there is nothing for Kopuz | ||
| // to have stored; whether it is signed in is the plugin's answer to | ||
| // `validate`, not a token in our database. | ||
| let is_plugin = saved.service == MusicService::Plugin && saved.plugin_id.is_some(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject Plugin sources without a valid plugin ID before activation.
When saved.service == MusicService::Plugin but saved.plugin_id is missing, Line [75] makes is_plugin false, yet Lines [99-102] still activate the incomplete server and Line [104] returns false. The caller then reaches its Plugin branch but has no ID to authenticate, leaving the user on an unusable source with no error path. Validate the required plugin identity before writing the active snapshot and surface the failure to the caller.
Also applies to: 97-104
🤖 Prompt for 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.
In `@crates/hooks/src/source_switch.rs` around lines 72 - 75, Validate the Plugin
source identity in the activation flow before writing the active snapshot: when
saved.service is MusicService::Plugin, require saved.plugin_id to be present and
reject or return an error if it is missing. Update the logic around is_plugin
and the activation path near Lines 97-104 so incomplete Plugin sources are never
activated and the failure is surfaced to the caller.
|
|
||
| # Plugin sources (generic — a plugin supplies its own name and prompts). | ||
| plugins = Plugins | ||
| plugins_none = No plugins installed. Drop a plugin folder into the directory above, then rescan. | ||
| plugin_rescan = Rescan plugins | ||
| plugin_protocol = protocol { $version } | ||
| plugin_sign_in = Sign in | ||
| plugin_not_found = Plugin { $id } is not installed | ||
| plugin_pick_one = Pick a plugin from the list | ||
| plugin_no_setup = This plugin collects whatever it needs during sign-in. | ||
| plugin_connecting = Connecting to the plugin… | ||
| plugin_open_sign_in = Open sign-in page | ||
| plugin_signed_in = Signed in. | ||
| plugin_working = Working… | ||
| plugin_retry = Try again | ||
| continue_action = Continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the plugin strings in every supplied locale.
The new plugin keys exist everywhere, but their values are copied in English, so localized users will see English plugin-management and sign-in UI. As per coding guidelines, crates/i18n/locales/*.ftl must add every new Fluent key to all locales while keeping parity with en.ftl.
crates/i18n/locales/es.ftl#L514-L529: translate the plugin block into Spanish.crates/i18n/locales/fil.ftl#L507-L522: translate the plugin block into Filipino.crates/i18n/locales/fr.ftl#L514-L529: translate the plugin block into French.crates/i18n/locales/gr.ftl#L513-L528: translate the plugin block into Greek.crates/i18n/locales/tr.ftl#L513-L528: translate the plugin block into Turkish.crates/i18n/locales/uk.ftl#L513-L528: translate the plugin block into Ukrainian.crates/i18n/locales/vi-VN.ftl#L507-L522: translate the plugin block into Vietnamese.crates/i18n/locales/zh-CN.ftl#L513-L528: translate the plugin block into Simplified Chinese.
📍 Affects 8 files
crates/i18n/locales/es.ftl#L514-L529(this comment)crates/i18n/locales/fil.ftl#L507-L522crates/i18n/locales/fr.ftl#L514-L529crates/i18n/locales/gr.ftl#L513-L528crates/i18n/locales/tr.ftl#L513-L528crates/i18n/locales/uk.ftl#L513-L528crates/i18n/locales/vi-VN.ftl#L507-L522crates/i18n/locales/zh-CN.ftl#L513-L528
🤖 Prompt for 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.
In `@crates/i18n/locales/es.ftl` around lines 514 - 529, Translate the plugin
block values, preserving every Fluent key and placeholder, in
crates/i18n/locales/es.ftl lines 514-529 (Spanish), crates/i18n/locales/fil.ftl
lines 507-522 (Filipino), crates/i18n/locales/fr.ftl lines 514-529 (French),
crates/i18n/locales/gr.ftl lines 513-528 (Greek), crates/i18n/locales/tr.ftl
lines 513-528 (Turkish), crates/i18n/locales/uk.ftl lines 513-528 (Ukrainian),
crates/i18n/locales/vi-VN.ftl lines 507-522 (Vietnamese), and
crates/i18n/locales/zh-CN.ftl lines 513-528 (Simplified Chinese), keeping all
locales in parity with en.ftl.
Source: Coding guidelines
| # Plugin sources (generic — a plugin supplies its own name and prompts). | ||
| plugins = Plugins | ||
| plugins_none = No plugins installed. Drop a plugin folder into the directory above, then rescan. | ||
| plugin_rescan = Rescan plugins | ||
| plugin_protocol = protocol { $version } | ||
| plugin_sign_in = Sign in | ||
| plugin_not_found = Plugin { $id } is not installed | ||
| plugin_pick_one = Pick a plugin from the list | ||
| plugin_no_setup = This plugin collects whatever it needs during sign-in. | ||
| plugin_connecting = Connecting to the plugin… | ||
| plugin_open_sign_in = Open sign-in page | ||
| plugin_signed_in = Signed in. | ||
| plugin_working = Working… | ||
| plugin_retry = Try again | ||
| continue_action = Continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the plugin UI strings in every affected locale.
The new plugin blocks are English in all reviewed non-English locales, leaving the discovery and authentication flows untranslated.
crates/i18n/locales/he.ftl#L514-L528: provide Hebrew translations.crates/i18n/locales/hu.ftl#L514-L528: provide Hungarian translations.crates/i18n/locales/id.ftl#L508-L522: provide Indonesian translations.crates/i18n/locales/it.ftl#L507-L521: provide Italian translations.crates/i18n/locales/ja.ftl#L520-L534: provide Japanese translations.crates/i18n/locales/ko.ftl#L514-L528: provide Korean translations.crates/i18n/locales/ml.ftl#L508-L522: provide Malayalam translations.crates/i18n/locales/nl.ftl#L508-L522: provide Dutch translations.crates/i18n/locales/pl.ftl#L514-L528: provide Polish translations.crates/i18n/locales/pt-BR.ftl#L513-L527: provide Brazilian Portuguese translations.
Preserve the Fluent placeholders { $version } and { $id }.
As per coding guidelines, crates/i18n/locales/*.ftl: add every new Fluent key to all locales, keeping parity with en.ftl and only using keys that are actually referenced.
📍 Affects 10 files
crates/i18n/locales/he.ftl#L514-L528(this comment)crates/i18n/locales/hu.ftl#L514-L528crates/i18n/locales/id.ftl#L508-L522crates/i18n/locales/it.ftl#L507-L521crates/i18n/locales/ja.ftl#L520-L534crates/i18n/locales/ko.ftl#L514-L528crates/i18n/locales/ml.ftl#L508-L522crates/i18n/locales/nl.ftl#L508-L522crates/i18n/locales/pl.ftl#L514-L528crates/i18n/locales/pt-BR.ftl#L513-L527
🤖 Prompt for 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.
In `@crates/i18n/locales/he.ftl` around lines 514 - 528, Localize the plugin UI
keys from plugins through continue_action in every affected locale:
crates/i18n/locales/he.ftl lines 514-528, crates/i18n/locales/hu.ftl lines
514-528, crates/i18n/locales/id.ftl lines 508-522, crates/i18n/locales/it.ftl
lines 507-521, crates/i18n/locales/ja.ftl lines 520-534,
crates/i18n/locales/ko.ftl lines 514-528, crates/i18n/locales/ml.ftl lines
508-522, crates/i18n/locales/nl.ftl lines 508-522, crates/i18n/locales/pl.ftl
lines 514-528, and crates/i18n/locales/pt-BR.ftl lines 513-527. Provide natural
translations matching each locale, preserve the Fluent placeholders { $version }
and { $id }, and keep all keys in parity with en.ftl using only referenced keys.
Source: Coding guidelines
| // tao calls process::exit() straight after this, which runs no | ||
| // destructors — so `kill_on_drop` never fires and a plugin child | ||
| // would be reparented and left running. Same fresh-thread reason | ||
| // as the flush above: block_on cannot run inside dioxus's runtime. | ||
| let _ = std::thread::spawn(|| { | ||
| let Ok(rt) = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| else { | ||
| return; | ||
| }; | ||
| rt.block_on(::server::plugin::shutdown_all()); | ||
| }) | ||
| .join(); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'mod.rs' crates/server/src/plugin --exec cat -n
rg -nP -C4 'fn shutdown(_all)?\s*\(' crates/server/src/pluginRepository: Kopuz-org/kopuz
Length of output: 11843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== main.rs relevant lines =="
sed -n '580,635p' crates/kopuz/src/main.rs | cat -n -v
sed -n '1060,1105p' crates/kopuz/src/main.rs | cat -n -v
echo
echo "== shutdown implementation =="
sed -n '230,265p' crates/server/src/plugin/client.rs | cat -n
echo
echo "== event handling symbols =="
rg -n -C5 'CloseRequested|RequestClose|LoopDestroyed|WindowCloseBehaviour|WindowHides|minimize_to_tray|shutdown_all' crates/kopuz/src/main.rs crates/server/src/pluginRepository: Kopuz-org/kopuz
Length of output: 16634
🌐 Web query:
dioxus desktop tao Event LoopDestroyed CloseRequested WindowHides documentation
💡 Result:
In Dioxus desktop applications, the event loop and window lifecycle are managed by the tao crate [1][2]. Understanding how these events interact is central to managing window visibility and application exit behavior. Key Events and Definitions: - CloseRequested: Emitted by the tao event loop when a user clicks the window close button [3][4]. - Destroyed: Emitted when a window is actually destroyed (removed from the OS) [5][3][4]. On Windows and Linux, this typically occurs when the Window object is dropped; on macOS, it can also be triggered by the dock "Quit" item [5][4]. - LoopDestroyed: Emitted when the entire event loop shuts down, signaling that the application is terminating [1][4][2]. Window Close Behavior: Dioxus provides the WindowCloseBehaviour enum to control how a window responds to a CloseRequested event [6][7]. - WindowCloses (Default): When the close button is clicked, the window is closed and destroyed [8][7]. If the application is configured to exit on the last window close (the default setting), the entire application will terminate once the last window is destroyed [8][7]. - WindowHides: When the close button is clicked, the window is hidden (set_visible(false)) rather than destroyed [8][7]. This keeps the application alive, which is useful if you want to perform background tasks or intercept the close event for custom logic (e.g., a confirmation dialog) [8][9]. To change this behavior, you can use the DesktopService associated with your window: window.set_close_behavior(WindowCloseBehaviour::WindowHides); Because Dioxus does not currently have a native "prevent_close" API, you can implement custom close logic by setting the behavior to WindowHides, listening for the CloseRequested event via a wry event handler, and then performing your desired actions (such as showing a confirmation modal) before manually calling close or leaving it hidden [9].
Citations:
- 1: https://docs.rs/tao/latest/tao/
- 2: https://docs.rs/tao/latest/src/tao/lib.rs.html
- 3: https://docs.rs/tao/latest/tao/event/enum.WindowEvent.html
- 4: https://docs.rs/tao/latest/src/tao/event.rs.html
- 5: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/enum.WindowEvent.html
- 6: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/struct.DesktopService.html
- 7: https://github.com/DioxusLabs/dioxus/blob/cece12b5/packages/desktop/src/config.rs
- 8: https://github.com/DioxusLabs/dioxus/blob/main/packages/desktop/src/app.rs
- 9: How to intercept closing the window, but keep it visible? DioxusLabs/dioxus#5552
🌐 Web query:
Dioxus desktop use_wry_event_handler LoopDestroyed CloseRequested WindowHides
💡 Result:
In Dioxus desktop applications, use_wry_event_handler is a hook that allows you to register a closure to intercept low-level Wry (and underlying Winit) events [1][2]. These events include window lifecycle notifications like CloseRequested, Destroyed, and LoopDestroyed [3][4]. Key Event Distinctions: - CloseRequested: This event is triggered when the user initiates a request to close the window (e.g., clicking the 'X' button) [3][5]. In Dioxus, the application handles this based on the configured WindowCloseBehaviour [6]. If set to WindowCloseBehaviour::WindowHides, the window will hide rather than be destroyed [7][6]. If set to WindowCloseBehaviour::WindowCloses (default), it proceeds to close the window [6]. - Destroyed: This event indicates that the window has been fully destroyed and its resources cleaned up [3][5]. It is important to note that this event often requires the window to be explicitly dropped or closed to trigger [8]. - LoopDestroyed: This is a global event signaling that the entire application event loop is terminating [4][8]. It is generally considered the safest place to perform final application-wide cleanup, as it is guaranteed to be the last event received [8]. Practical Usage: You can use use_wry_event_handler to listen for these specific events by pattern matching on the event passed to your handler [4][2]. Since use_wry_event_handler uses a hook to manage the lifecycle of the listener, it automatically cleans up (removes the handler) when the component is unmounted [2]. Example Concept: use_wry_event_handler(|event, _| { if let Event::WindowEvent { event: window_event,.. } = event { match window_event { WindowEvent::CloseRequested => println!("Close requested"), WindowEvent::Destroyed => println!("Window destroyed"), _ => {} } } else if let Event::LoopDestroyed = event { println!("App loop destroyed"); } }); If you are implementing custom close behavior, ensure you are not creating a loop where you attempt to exit the event loop before the window is fully destroyed, as this can lead to unresponsive windows on some platforms [9]. The WindowCloseBehaviour::WindowHides setting is the standard Dioxus approach for scenarios where you wish to keep the process running after a window is "closed" [7][6].
Citations:
- 1: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/fn.use_wry_event_handler.html
- 2: https://docs.rs/dioxus-desktop/latest/src/dioxus_desktop/hooks.rs.html
- 3: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/enum.WindowEvent.html
- 4: https://github.com/DioxusLabs/dioxus/blob/672dc1f4/packages/desktop/src/launch.rs
- 5: https://docs.rs/winit/latest/winit/event/enum.WindowEvent.html
- 6: https://github.com/DioxusLabs/dioxus/blob/main/packages/desktop/src/app.rs
- 7: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/struct.DesktopService.html
- 8: Window Destroyed Event Not Sent on X11 rust-windowing/winit#1486
- 9: Window does not close under specific conditions. rust-windowing/winit#3673
🌐 Web query:
tao close_requested handler WindowHides close requested event docs
💡 Result:
In the Tao Rust crate, CloseRequested is a variant of the WindowEvent enum [1]. It is emitted when the operating system sends a request to close a window, such as when a user clicks the close button [1][2]. To handle this event, you match the Event::WindowEvent and specifically check for WindowEvent::CloseRequested within your event loop [3][4]. The typical pattern for handling this is to set the control flow to ControlFlow::Exit, which terminates the event loop [3][5][4]. Example usage: event_loop.run(move |event, _, control_flow| { match event { Event::WindowEvent { event: WindowEvent::CloseRequested,.. } => *control_flow = ControlFlow::Exit, _ =>, } }); In multi-window applications, the WindowEvent contains a WindowId member [5][4]. You should compare this ID against the ID of your specific window (obtained via Window::id) to ensure you are closing the correct window [5][4].
Citations:
- 1: https://docs.rs/tao/latest/tao/event/enum.WindowEvent.html
- 2: https://docs.rs/tao/latest/src/tao/event.rs.html
- 3: https://docs.rs/tao/latest/tao/window/struct.Window.html
- 4: https://docs.rs/tao
- 5: https://docs.rs/tao/latest/src/tao/lib.rs.html
🌐 Web query:
Dioxus Desktop 0.39 CloseRequested WindowHides documentation
💡 Result:
In Dioxus Desktop, the behavior of a window when a user attempts to close it is controlled by the WindowCloseBehaviour enum [1]. When the CloseRequested event is triggered, Dioxus checks the configured close behavior to decide whether to actually destroy the window or merely hide it [2][3]. To change this behavior, you can use the DesktopContext to update the close behavior for a specific window [4][5]. How to use WindowCloseBehaviour::WindowHides: 1. Access the DesktopContext: You can typically access this through hooks like use_window [6]. 2. Set the behavior: Call the set_close_behavior method on the desktop context, passing WindowCloseBehaviour::WindowHides as the argument [4][5]. Example: let desktop = dioxus_desktop::use_window; desktop.set_close_behavior(dioxus_desktop::WindowCloseBehaviour::WindowHides); When set to WindowHides, the window will simply become invisible (via window.set_visible(false)) instead of closing when the user clicks the close button [2][5][3]. If you want to configure this globally at app startup, you can use the with_close_behaviour method on the desktop configuration [7][8].
Citations:
- 1: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/enum.WindowCloseBehaviour.html
- 2: https://github.com/DioxusLabs/dioxus/blob/main/packages/desktop/src/app.rs
- 3: https://github.com/DioxusLabs/dioxus/blob/1766af46/packages/desktop/src/app.rs
- 4: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/struct.DesktopService.html
- 5: https://github.com/DioxusLabs/dioxus/blob/672dc1f4/packages/desktop/src/desktop_context.rs
- 6: https://docs.rs/dioxus-desktop/latest/dioxus_desktop/all.html
- 7: https://github.com/DioxusLabs/dioxus/blob/cece12b5/packages/desktop/src/config.rs
- 8: https://docs.rs/dioxus-desktop/latest/x86_64-pc-windows-msvc/src/dioxus_desktop/config.rs.html
Gate shutdown_all() on actual app quit, not on every close request.
When tray mode is enabled, the close button sets WindowCloseBehaviour::WindowHides, but this CloseRequested handler still drains clients and calls shutdown(), killing live plugin children while the app stays running. Restrict plugin teardown to the app shutdown path such as Event::LoopDestroyed. Also make shutdown() return after a bounded grace/kill deadline so a wedged plugin cannot hang the UI thread via this join().
🤖 Prompt for 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.
In `@crates/kopuz/src/main.rs` around lines 609 - 623, Move the plugin teardown
block using shutdown_all() out of the CloseRequested handling and into the
actual application shutdown path, such as the Event::LoopDestroyed branch, so
tray-mode window hides do not terminate live plugins. Update shutdown() to
enforce a bounded grace/kill deadline before returning, ensuring the
spawned-thread join cannot block the UI indefinitely on a wedged plugin.
| pub fn plugin_auth_cancel(mut auth_state: Signal<Option<PluginAuthState>>) { | ||
| let Some(state) = auth_state.take() else { | ||
| return; | ||
| }; | ||
| spawn(async move { | ||
| if let Some(client) = ::server::registry().connected(&state.plugin_id).await { | ||
| client.notify( | ||
| ::server::plugin::wire::method::AUTH_CANCEL, | ||
| serde_json::json!({}), | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /// Apply one wizard result: close on success, surface the message on failure, | ||
| /// keep going otherwise. | ||
| fn finish_step( | ||
| mut auth_state: Signal<Option<PluginAuthState>>, | ||
| mut error: Signal<Option<String>>, | ||
| plugin_id: String, | ||
| plugin_name: String, | ||
| prompt: Result<::server::plugin::wire::AuthPrompt, String>, | ||
| ) { | ||
| use ::server::plugin::wire::AuthPrompt; | ||
|
|
||
| if let Ok(AuthPrompt::Done) = prompt { | ||
| auth_state.set(None); | ||
| error.set(None); | ||
| hooks::use_sync_task::nudge(); | ||
| return; | ||
| } | ||
| // A transport failure is shown the same way the plugin's own `Failed` | ||
| // would be — the user cannot act on the distinction. | ||
| let prompt = prompt.unwrap_or_else(|message| AuthPrompt::Failed { message }); | ||
| auth_state.set(Some(PluginAuthState { | ||
| plugin_id, | ||
| plugin_name, | ||
| prompt, | ||
| busy: false, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A cancelled wizard is resurrected by the in-flight RPC.
The cancel button is never disabled, so it can fire while busy. plugin_auth_cancel takes the state to None, but the still-pending auth_begin/auth_submit future then lands in finish_step, which unconditionally writes Some(..) — the popup reappears for a flow the user abandoned (and, if the plugin answers Done, nudge() runs for a cancelled sign-in). Same hazard if the user starts a wizard for a different plugin while one is in flight.
Gate the write on the state still belonging to this flow.
🐛 Proposed fix: ignore results for a superseded flow
fn finish_step(
mut auth_state: Signal<Option<PluginAuthState>>,
mut error: Signal<Option<String>>,
plugin_id: String,
plugin_name: String,
prompt: Result<::server::plugin::wire::AuthPrompt, String>,
) {
use ::server::plugin::wire::AuthPrompt;
+ // The user may have cancelled (or started another plugin's wizard) while
+ // this call was in flight; that result is no longer ours to apply.
+ if auth_state.peek().as_ref().map(|s| s.plugin_id.as_str()) != Some(plugin_id.as_str()) {
+ return;
+ }
if let Ok(AuthPrompt::Done) = prompt {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn plugin_auth_cancel(mut auth_state: Signal<Option<PluginAuthState>>) { | |
| let Some(state) = auth_state.take() else { | |
| return; | |
| }; | |
| spawn(async move { | |
| if let Some(client) = ::server::registry().connected(&state.plugin_id).await { | |
| client.notify( | |
| ::server::plugin::wire::method::AUTH_CANCEL, | |
| serde_json::json!({}), | |
| ); | |
| } | |
| }); | |
| } | |
| /// Apply one wizard result: close on success, surface the message on failure, | |
| /// keep going otherwise. | |
| fn finish_step( | |
| mut auth_state: Signal<Option<PluginAuthState>>, | |
| mut error: Signal<Option<String>>, | |
| plugin_id: String, | |
| plugin_name: String, | |
| prompt: Result<::server::plugin::wire::AuthPrompt, String>, | |
| ) { | |
| use ::server::plugin::wire::AuthPrompt; | |
| if let Ok(AuthPrompt::Done) = prompt { | |
| auth_state.set(None); | |
| error.set(None); | |
| hooks::use_sync_task::nudge(); | |
| return; | |
| } | |
| // A transport failure is shown the same way the plugin's own `Failed` | |
| // would be — the user cannot act on the distinction. | |
| let prompt = prompt.unwrap_or_else(|message| AuthPrompt::Failed { message }); | |
| auth_state.set(Some(PluginAuthState { | |
| plugin_id, | |
| plugin_name, | |
| prompt, | |
| busy: false, | |
| })); | |
| } | |
| pub fn plugin_auth_cancel(mut auth_state: Signal<Option<PluginAuthState>>) { | |
| let Some(state) = auth_state.take() else { | |
| return; | |
| }; | |
| spawn(async move { | |
| if let Some(client) = ::server::registry().connected(&state.plugin_id).await { | |
| client.notify( | |
| ::server::plugin::wire::method::AUTH_CANCEL, | |
| serde_json::json!({}), | |
| ); | |
| } | |
| }); | |
| } | |
| /// Apply one wizard result: close on success, surface the message on failure, | |
| /// keep going otherwise. | |
| fn finish_step( | |
| mut auth_state: Signal<Option<PluginAuthState>>, | |
| mut error: Signal<Option<String>>, | |
| plugin_id: String, | |
| plugin_name: String, | |
| prompt: Result<::server::plugin::wire::AuthPrompt, String>, | |
| ) { | |
| use ::server::plugin::wire::AuthPrompt; | |
| // The user may have cancelled (or started another plugin's wizard) while | |
| // this call was in flight; that result is no longer ours to apply. | |
| if auth_state.peek().as_ref().map(|s| s.plugin_id.as_str()) != Some(plugin_id.as_str()) { | |
| return; | |
| } | |
| if let Ok(AuthPrompt::Done) = prompt { | |
| auth_state.set(None); | |
| error.set(None); | |
| hooks::use_sync_task::nudge(); | |
| return; | |
| } | |
| // A transport failure is shown the same way the plugin's own `Failed` | |
| // would be — the user cannot act on the distinction. | |
| let prompt = prompt.unwrap_or_else(|message| AuthPrompt::Failed { message }); | |
| auth_state.set(Some(PluginAuthState { | |
| plugin_id, | |
| plugin_name, | |
| prompt, | |
| busy: false, | |
| })); | |
| } |
🤖 Prompt for 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.
In `@crates/pages/src/settings_actions.rs` around lines 534 - 574, Update
finish_step to apply auth_begin/auth_submit results only when auth_state still
represents the same active flow, matching the captured plugin_id (and flow
identity as needed). Ignore stale results after plugin_auth_cancel or when
another plugin wizard has replaced the state, including Done results so they
cannot clear or nudge a newer/cancelled flow.
| fn emit_plugin_log(id: &str, log: &wire::LogParams) { | ||
| let target = log.target.as_deref().unwrap_or(""); | ||
| match log.level { | ||
| wire::LogLevel::Trace => { | ||
| tracing::trace!(target: "plugin", plugin = %id, module = target, "{}", log.message); | ||
| } | ||
| wire::LogLevel::Debug => { | ||
| tracing::debug!(target: "plugin", plugin = %id, module = target, "{}", log.message); | ||
| } | ||
| wire::LogLevel::Info => { | ||
| tracing::info!(target: "plugin", plugin = %id, module = target, "{}", log.message); | ||
| } | ||
| wire::LogLevel::Warn => { | ||
| tracing::warn!(target: "plugin", plugin = %id, module = target, "{}", log.message); | ||
| } | ||
| wire::LogLevel::Error => { | ||
| tracing::error!(target: "plugin", plugin = %id, module = target, "{}", log.message); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async fn stderr_task(id: String, stderr: tokio::process::ChildStderr) { | ||
| let mut lines = LineReader::new(stderr); | ||
| while let Ok(Some(line)) = lines.next_line().await { | ||
| if !line.trim().is_empty() { | ||
| tracing::debug!(target: "plugin", plugin = %id, "{}", line.trim_end()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
data_token redaction is documented but not implemented.
wire.rs says the token is held "only so the host can redact it from logs", yet plugin log params and stderr lines are re-emitted verbatim here. A plugin that logs the stream URL it just handed back leaks its per-process token into Kopuz's logs. Either scrub the handshake's data_token from log.message/stderr before emitting, or drop the claim from the protocol docs.
🤖 Prompt for 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.
In `@crates/server/src/plugin/client.rs` around lines 718 - 746, The data_token
redaction contract is not enforced before plugin output is logged. Update
emit_plugin_log and stderr_task to receive or access the handshake data_token,
scrub every occurrence from log.message and stderr lines before tracing them,
and preserve the existing level and target metadata while emitting the sanitized
text.
| fn i18n_locale() -> String { | ||
| std::env::var("LANG") | ||
| .ok() | ||
| .and_then(|l| l.split('.').next().map(str::to_owned)) | ||
| .filter(|l| !l.is_empty()) | ||
| .unwrap_or_else(|| "en".to_string()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
locale comes from LANG, not the user's selected UI language.
InitializeParams::locale is documented as "language tag of the running UI", but this reads the process environment — which is commonly unset on Windows/macOS app bundles, and diverges from config.language whenever the user picks a language in settings. Plugin-authored strings will be localized against the wrong tag.
🤖 Prompt for 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.
In `@crates/server/src/plugin/client.rs` around lines 885 - 891, Update
i18n_locale so it uses the user's selected UI language from the existing
configuration or initialization state, rather than reading LANG from the process
environment. Ensure the returned value matches InitializeParams::locale and
reflects config.language when the user changes the language, while preserving
the English fallback when no language is configured.
| pub async fn client(&self, plugin_id: &str) -> Result<PluginClient, SourceError> { | ||
| let mut clients = self.inner.clients.lock().await; | ||
| if let Some(existing) = clients.get(plugin_id) { | ||
| if !existing.is_exhausted() { | ||
| return Ok(existing.clone()); | ||
| } | ||
| // Past its restart budget: drop it so an explicit reconnect can | ||
| // start from a clean slate. | ||
| clients.remove(plugin_id); | ||
| } | ||
|
|
||
| let manifest = self.manifest(plugin_id).ok_or_else(|| { | ||
| SourceError::Backend(format!("no plugin named {plugin_id} is installed")) | ||
| })?; | ||
| let client = PluginClient::connect(manifest).await?; | ||
| self.remember_handshake(plugin_id, &client); | ||
| clients.insert(plugin_id.to_string(), client.clone()); | ||
| Ok(client) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
One global mutex over the whole client map serializes unrelated plugins across a full spawn+handshake.
client() holds inner.clients for the entire PluginClient::connect().await, which covers process spawn, restart backoff sleep, and a handshake bounded by CALL_TIMEOUT (60s in client.rs). While plugin A is connecting, client(), connected() and disconnect() for every other plugin block behind it, so a slow or hanging plugin stalls unrelated source access (and any UI path awaiting it).
Consider keying the dedup per plugin — e.g. store a per-id Arc<tokio::sync::Mutex<Option<PluginClient>>> (or a shared connect future) fetched under a briefly-held map lock, then release the map lock before awaiting the connect.
🤖 Prompt for 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.
In `@crates/server/src/plugin/mod.rs` around lines 109 - 127, Refactor
PluginManager::client so the global inner.clients lock is held only while
retrieving or updating a per-plugin connection slot, not during
PluginClient::connect(plugin_id).await. Add per-plugin synchronization such as
Arc<tokio::sync::Mutex<Option<PluginClient>>> to deduplicate concurrent
connections for the same plugin while allowing different plugin IDs to connect
concurrently; preserve exhausted-client removal, manifest lookup, handshake
recording, and client reuse behavior.
| fn web_url(&self, track: &reader::Track) -> Option<String> { | ||
| let item_id = track.id.key(); | ||
| let (_, item_ref) = plugin::split_item_id(&item_id)?; | ||
| Some( | ||
| plugin::registry() | ||
| .cached_web_url_template(&self.plugin_id)? | ||
| .replace("{id}", item_ref), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
web_url doesn't validate namespace ownership, unlike strip().
split_item_id returns (owner, item_ref) but owner is discarded. If a track from a different plugin's namespace is ever passed in, this silently builds a URL from this plugin's template using the other plugin's ref, instead of returning None the way strip() rejects mismatched ownership everywhere else in this file.
🔧 Proposed fix
fn web_url(&self, track: &reader::Track) -> Option<String> {
let item_id = track.id.key();
- let (_, item_ref) = plugin::split_item_id(&item_id)?;
+ let (owner, item_ref) = plugin::split_item_id(&item_id)?;
+ if owner != self.plugin_id {
+ return None;
+ }
Some(
plugin::registry()
.cached_web_url_template(&self.plugin_id)?
.replace("{id}", item_ref),
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn web_url(&self, track: &reader::Track) -> Option<String> { | |
| let item_id = track.id.key(); | |
| let (_, item_ref) = plugin::split_item_id(&item_id)?; | |
| Some( | |
| plugin::registry() | |
| .cached_web_url_template(&self.plugin_id)? | |
| .replace("{id}", item_ref), | |
| ) | |
| } | |
| fn web_url(&self, track: &reader::Track) -> Option<String> { | |
| let item_id = track.id.key(); | |
| let (owner, item_ref) = plugin::split_item_id(&item_id)?; | |
| if owner != self.plugin_id { | |
| return None; | |
| } | |
| Some( | |
| plugin::registry() | |
| .cached_web_url_template(&self.plugin_id)? | |
| .replace("{id}", item_ref), | |
| ) | |
| } |
🤖 Prompt for 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.
In `@crates/server/src/source/plugin.rs` around lines 249 - 257, Update web_url to
retain the owner returned by plugin::split_item_id and validate it matches
self.plugin_id before fetching the URL template or constructing the URL; return
None for mismatched namespaces, consistent with strip().
| async fn fetch_playlist_entries( | ||
| &self, | ||
| playlist_id: &str, | ||
| ) -> Result<Vec<reader::Track>, SourceError> { | ||
| let mut all = Vec::new(); | ||
| let mut cursor = None; | ||
| loop { | ||
| let page = self | ||
| .fetch_playlist_entries_page(playlist_id, cursor) | ||
| .await?; | ||
| all.extend(page.tracks); | ||
| match page.next { | ||
| Some(next) => cursor = Some(next), | ||
| None => return Ok(all), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded pagination loop against an external plugin process.
fetch_playlist_entries loops on page.next with no cap on the number of pages. A buggy or misbehaving plugin that keeps returning a fresh cursor would make this loop indefinitely, growing all without bound and never returning — a real external-call hazard since a plugin is an arbitrary user-installed executable, not a trusted in-process backend.
🛡️ Proposed fix: cap the number of pages
async fn fetch_playlist_entries(
&self,
playlist_id: &str,
) -> Result<Vec<reader::Track>, SourceError> {
let mut all = Vec::new();
let mut cursor = None;
- loop {
+ // A misbehaving plugin could paginate forever; this is a generous but
+ // finite ceiling so a single bad plugin can't hang the sync/UI thread.
+ const MAX_PAGES: usize = 10_000;
+ for _ in 0..MAX_PAGES {
let page = self
.fetch_playlist_entries_page(playlist_id, cursor)
.await?;
all.extend(page.tracks);
match page.next {
Some(next) => cursor = Some(next),
None => return Ok(all),
}
}
+ Err(SourceError::Backend(format!(
+ "plugin {} paginated past the safety cap fetching playlist {playlist_id}",
+ self.plugin_id
+ )))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn fetch_playlist_entries( | |
| &self, | |
| playlist_id: &str, | |
| ) -> Result<Vec<reader::Track>, SourceError> { | |
| let mut all = Vec::new(); | |
| let mut cursor = None; | |
| loop { | |
| let page = self | |
| .fetch_playlist_entries_page(playlist_id, cursor) | |
| .await?; | |
| all.extend(page.tracks); | |
| match page.next { | |
| Some(next) => cursor = Some(next), | |
| None => return Ok(all), | |
| } | |
| } | |
| } | |
| async fn fetch_playlist_entries( | |
| &self, | |
| playlist_id: &str, | |
| ) -> Result<Vec<reader::Track>, SourceError> { | |
| let mut all = Vec::new(); | |
| let mut cursor = None; | |
| // A misbehaving plugin could paginate forever; this is a generous but | |
| // finite ceiling so a single bad plugin can't hang the sync/UI thread. | |
| const MAX_PAGES: usize = 10_000; | |
| for _ in 0..MAX_PAGES { | |
| let page = self | |
| .fetch_playlist_entries_page(playlist_id, cursor) | |
| .await?; | |
| all.extend(page.tracks); | |
| match page.next { | |
| Some(next) => cursor = Some(next), | |
| None => return Ok(all), | |
| } | |
| } | |
| Err(SourceError::Backend(format!( | |
| "plugin {} paginated past the safety cap fetching playlist {playlist_id}", | |
| self.plugin_id | |
| ))) | |
| } |
🤖 Prompt for 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.
In `@crates/server/src/source/plugin.rs` around lines 679 - 695, Cap pagination in
fetch_playlist_entries by tracking the number of fetched pages and stopping with
an appropriate SourceError once the configured maximum is reached. Increment the
counter for each fetch, preserve normal cursor termination and accumulation, and
ensure a plugin returning endless cursors cannot grow all or loop indefinitely.
|
Reviewing 5k+ lines will be really kirkful |
|
isnt lua the better option for plugin system? |
|
could be, i think this depends more on whether you want to add a lua interop to kopuz, thats more your decision ofc |
|
I think lua would fit better for plugin system |
|
i'll explore that |
2 participants
So this adds a plugin system, which in the future can be extended with a marketplace and such on the site, and it is mainly meant for adding new music servers (like a spotify one im making, separate to the officially supported one btw)
You can find more docu in:
docs/plugins.mdSanity Checking
rules.
contribution guidelines, or this pull request did not use AI assistance. (I used AI assisted coding, I watched everything, I am an experienced developer that writes real enterprise code ok)
Style and Consistency
style.
cargo fmt --all --checkorcargo fmt --allas appropriate.cargo clippy --workspace --all-targets -- -D warnings, orexplained why it could not be run.
this change depends on them.
Testing
Tested on platform(s):
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwin