feat(plugin): add dynamic config UI with schema-driven form#109
Conversation
Plugins declaring a `[config]` block in `plugin.toml` now expose their
schema at runtime. Backend extends `PluginManifest` with
`PluginConfigSchema`, adds the `PluginConfigStore` port (SQLite-backed
via the new `plugin_configs` table), persists user values across
restarts, and surfaces them through new `plugin_config_get` /
`plugin_config_update` IPC commands. The `ExtismPluginLoader` exposes
`get_manifest` and `set_runtime_config` so the loader's in-memory
plugin_configs map is kept in sync with the persisted store, ensuring
`get_config(key)` calls inside WASM plugins observe the latest value
without a reload.
Frontend ships a `PluginConfigField` dispatcher (string → input,
boolean → switch, integer/float → numeric input with bounds, url →
url input, enum / string-with-options → select) and a
`PluginConfigDialog` that loads the schema, validates every field
against type / min / max / regex on the UI side and persists changed
values sequentially. The Plugins view fetches each installed plugin's
schema once and only renders the *Configure* button when the manifest
declares fields.
Validation runs both UI-side (optimistic feedback) and backend-side
(authoritative — `UpdatePluginConfigCommand` re-validates against the
schema before persisting), matching the existing `update_config`
pattern.
i18n: `plugins.action.configure`, `plugins.config.{title,description,
loading,error,noFields,toast.{saveSuccess,validationFailed}}` (en/fr).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds dynamic per-plugin runtime configuration: schema extraction from manifests, typed validation, SQLite-backed persisted store with migrations/repo, CQRS commands/queries and Tauri IPC, frontend modal editor and field components, and startup replay of persisted configs into the plugin loader. Changes
Sequence DiagramsequenceDiagram
participant FE as Frontend UI
participant IPC as Tauri IPC
participant QB as QueryBus
participant PL as PluginLoader
participant CS as PluginConfigStore
participant DB as SQLite
participant WASM as WASM Plugin
FE->>IPC: plugin_config_get(name)
IPC->>QB: GetPluginConfigQuery(name)
QB->>PL: get_manifest(name)
PL-->>QB: PluginManifest + schema
QB->>CS: get_values(name)
CS->>DB: SELECT * FROM plugin_configs
DB-->>CS: persisted values
CS-->>QB: values map
QB->>QB: merge schema + defaults + persisted
QB-->>IPC: PluginConfigView
IPC-->>FE: schema & values
FE->>IPC: plugin_config_update(name,key,value)
IPC->>QB: UpdatePluginConfigCommand(name,key,value)
QB->>PL: get_manifest(name)
PL-->>QB: PluginManifest + schema
QB->>QB: validate(key,value)
QB->>CS: set_value(name,key,value)
CS->>DB: INSERT/UPDATE plugin_configs
DB-->>CS: success
CS-->>QB: Ok
QB->>PL: set_runtime_config(name,key,value)
PL->>WASM: update shared host config
WASM-->>PL: Ok
PL-->>QB: Ok
QB-->>IPC: Ok
IPC-->>FE: success
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 adds a complete schema-driven plugin configuration system: Confidence Score: 4/5Safe to merge with low risk; two previously-flagged P1s (escaped No new P0/P1 issues found in this review pass. The P1 ceiling comes from the unresolved src-tauri/src/domain/model/plugin.rs (regex anchor stripping) and src-tauri/src/application/commands/uninstall_plugin.rs (missing delete_all call)
|
| Filename | Overview |
|---|---|
| src-tauri/src/domain/model/plugin.rs | Adds ConfigField, ConfigFieldType, PluginConfigSchema, a hand-rolled JSON array length counter, and a minimal regex engine; the $ end-anchor stripping in match_regex does not handle escaped \$ (flagged in previous review and still present) |
| src-tauri/src/application/commands/update_plugin_config.rs | Validates value against schema, sets runtime first, persists, rolls back runtime on persist failure; runtime-only inconsistency when there is no prior value and persist fails is documented and minor |
| src-tauri/src/application/queries/get_plugin_config.rs | Joins schema with persisted values, prunes stale entries via schema validation, fills manifest defaults; clean and well-tested |
| src-tauri/src/application/commands/uninstall_plugin.rs | Never calls PluginConfigStore::delete_all on uninstall; config rows for the uninstalled plugin remain indefinitely (flagged in previous review and still unaddressed) |
| src-tauri/src/adapters/driven/sqlite/plugin_config_repo.rs | SQLite repo with upsert, per-plugin isolation, delete_all; well-tested with in-process tests |
| src/views/PluginsView/PluginConfigDialog.tsx | Validate function now properly uses Number.isFinite and checks array element counts; draft-seeding race fixed; sequential saves with single post-loop invalidation is correct |
| src/views/PluginsView.tsx | Batch-fetches all installed plugins' configs to show/hide Configure button; query key includes installedNames so any status change triggers a full refetch; misleading cache-sharing comment (flagged previously) |
| src-tauri/src/lib.rs | Startup replay inserts all persisted values raw; relies on per-plugin load-time pruning to discard stale values (documented in code comment); unresolved concern from previous review |
Sequence Diagram
sequenceDiagram
participant UI as PluginConfigDialog
participant IPC as Tauri IPC
participant QB as QueryBus
participant CB as CommandBus
participant Loader as ExtismPluginLoader
participant Store as SqlitePluginConfigRepo
UI->>IPC: plugin_config_get(name)
IPC->>QB: handle_get_plugin_config
QB->>Loader: get_manifest(name)
QB->>Store: get_values(name)
QB-->>UI: PluginConfigView {fields, values}
UI->>IPC: plugin_config_update(name, key, value)
IPC->>CB: handle_update_plugin_config
CB->>Loader: get_manifest(name) — validate schema
CB->>Store: get_values(name) — capture prior value
CB->>Loader: set_runtime_config(name, key, value)
CB->>Store: set_value(name, key, value)
alt persist fails
CB->>Loader: set_runtime_config(name, key, prior) — rollback
CB-->>UI: Err(AppError)
else persist ok
CB-->>UI: Ok(())
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/views/PluginsView.tsx
Line: 128-149
Comment:
**Batch config refetch on every plugin status change**
The query key `["plugin_config_get_all", installedNames]` is an array that is rebuilt whenever `entries` or `locallyDisabled` changes. Any plugin install, uninstall, enable, or disable mutates `installedNames`, invalidates the whole cache entry, and triggers a fresh `Promise.all` call for every installed plugin — including the ones whose schemas haven't changed. For users with many plugins this causes unnecessary IPC traffic on every status change.
Individual queries per plugin (keyed by `["plugin_config_get", { name }]` — the same key `PluginConfigDialog` uses) would let TanStack Query serve cached responses for unchanged plugins and only fetch the newly installed one.
How can I resolve this? If you propose a fix, please make it concise.Reviews (8): Last reviewed commit: "fix(plugin): validate persisted configs ..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/views/PluginsView/__tests__/PluginConfigField.test.tsx (1)
20-127: Cover thefloatandarraybranches too.This suite exercises most of the new dispatcher, but the PR also introduces
fieldType: "float"andfieldType: "array". Those paths currently have no regression coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PluginsView/__tests__/PluginConfigField.test.tsx` around lines 20 - 127, Add two tests to PluginConfigField.test.tsx that exercise the new "float" and "array" branches: for fieldType: "float" (use makeField({ fieldType: "float" })) render PluginConfigField and assert the labeled control is an HTMLInputElement with type "number" and an appropriate step attribute (e.g., "any" or a numeric step) to allow decimals; for fieldType: "array" (use makeField({ fieldType: "array" })) render PluginConfigField with an array value and assert the component renders the expected array control (e.g., a textarea or list of inputs) and that the displayed value matches the provided array (serialized/visible), using the same patterns as the existing tests (screen.getByLabelText / getByText) to locate controls.src-tauri/src/domain/model/plugin.rs (1)
353-360: Array validation only checks brackets, not JSON validity.The validation accepts strings like
"[not, valid, json{"as long as they start with[and end with]. If strict JSON array validation is needed, consider parsing withserde_json.This is acceptable if the consuming code (plugins) handles malformed arrays gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/model/plugin.rs` around lines 353 - 360, The Array branch in ConfigFieldType::Array currently only checks leading/trailing brackets on value, which allows malformed JSON arrays; update the validation in the ConfigFieldType::Array match arm (where value.trim() is used) to parse the string with serde_json::from_str::<serde_json::Value>(trimmed) (or from_str::<Vec<_>> if you want to enforce array type) and return a DomainError::ValidationError including the serde_json parse error if parsing fails, instead of only checking starts_with/ends_with.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 21: The changelog entry mixes French and English—replace all French terms
with English equivalents and ensure consistent wording: e.g., change "UI
dynamique" to "Dynamic UI", "plugins déclarant" to "plugins declaring", "input
texte" to "text input", and translate other fragments (keep all symbol names
unchanged: ConfigField / ConfigFieldType / PluginConfigSchema,
PluginConfigStore, SqlitePluginConfigRepo, migration
m20260425_000005_create_plugin_configs, adapters/driven/plugin/manifest.rs,
UpdatePluginConfigCommand, GetPluginConfigQuery,
PluginLoader.get_manifest()/set_runtime_config(), ExtismPluginLoader,
PluginRegistry, SharedHostResources::plugin_configs, lib.rs,
PluginConfigField.tsx, PluginConfigDialog.tsx, PluginsView, IPC commands
plugin_config_get/plugin_config_update, and i18n keys plugins.action.configure
and plugins.config.*) so the entire paragraph is in English and reads naturally.
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Around line 166-189: The code currently copies entry.default into ConfigField
without validating it; update the block that handles entry.default (and where
options/min/max/regex are applied) to validate the encoded default produced by
encode_config_default(default)? against the declared field_type and constraints:
ensure its type matches field_type, that it is one of the encoded options if
entry.options is present, that numeric values respect entry.min and entry.max,
and that string values match entry.regex; if validation fails return an error
(or propagate one) instead of inserting into schema. Implement this either by
calling an existing validator (e.g. validate_config_value or similar) or by
adding a validate_default helper used before field.with_default(...), and keep
using ConfigField, encode_config_default, entry.options, entry.min/entry.max,
entry.regex, and schema.insert to locate the changes.
In
`@src-tauri/src/adapters/driven/sqlite/migrations/m20260425_000005_create_plugin_configs.rs`:
- Around line 11-14: The migration uses Table::create().if_not_exists() in the
up() for PluginConfigs::Table which masks unexpected pre-existing tables; remove
the if_not_exists() call so the create will error loudly if the table already
exists, and make the down() symmetric by adding an if_exists() guard to the
Table::drop() for PluginConfigs::Table so rollback is safe but not silent.
Locate the up() function and the Table::create() invocation for
PluginConfigs::Table and delete the if_not_exists() call, then locate the down()
function and add the corresponding if_exists() option to the Table::drop() call
for PluginConfigs::Table.
In `@src-tauri/src/application/commands/update_plugin_config.rs`:
- Around line 39-45: The code currently calls store.set_value(...) before
plugin_loader().set_runtime_config(...), which can leave persisted state ahead
of the live plugin; change the sequence or add atomic behavior: either (A)
implement a transactional adapter method on the store/loader that applies
runtime and persistent changes together, or (B) capture the previous stored
value via store.get_value(&cmd.plugin_name, &cmd.key), call
plugin_loader().set_runtime_config(&cmd.plugin_name, &cmd.key, &cmd.value)
first, and only on success call store.set_value(...); if the subsequent
store.set_value fails roll back the runtime by calling
plugin_loader().set_runtime_config(..., &previous_value) and map errors to
AppError::Domain so update_plugin_config returns a consistent state.
In `@src-tauri/src/application/queries/get_plugin_config.rs`:
- Around line 29-46: The handler currently trusts get_values(&query.plugin_name)
and only fills missing keys with field.default_value(), causing stale/invalid
persisted values to be returned; after fetching values, revalidate each
persisted entry against manifest.config_schema() (for each (key, field) in
manifest.config_schema().fields()) and if a value is missing or fails validation
(enum/regex/type constraints) replace it with field.default_value() when present
or remove/normalize it to a safe default, then pass the cleaned map into
values_to_view(...) and PluginConfigView::new(...) so the UI always sees
schema-valid values rather than stale invalid persisted ones.
In `@src-tauri/src/domain/model/plugin.rs`:
- Around line 320-325: The integer branch in ConfigFieldType::Integer parses
value into parsed: i64 and then casts to f64 before calling
self.check_numeric_bounds(parsed as f64), which can lose precision for large
integers; fix by avoiding the f64 cast—either add/use an integer-aware bounds
check (e.g., a new method check_numeric_bounds_i64 or extend
check_numeric_bounds to accept i64) that compares parsed directly against
integer min/max, or convert the stored min/max/max to i64 when they represent
integral bounds and then perform i64 comparisons; update the
ConfigFieldType::Integer branch to call this integer-safe check (referencing
parsed and check_numeric_bounds/check_numeric_bounds_i64) so large integers are
validated without precision loss.
In `@src/views/PluginsView/PluginConfigDialog.tsx`:
- Around line 39-45: The float validator in PluginConfigDialog (inside the
branch checking field.fieldType === "float") accepts empty strings because
Number("") === 0; update the validation to treat empty/whitespace-only input as
invalid before converting to Number: check that value (or value.trim()) is
non-empty and return "Must be a number" if it is empty, then proceed to parse
into n and perform the existing Number.isNaN and min/max checks (refer to the
variables field.fieldType, value, and n in that function).
- Around line 103-145: The useEffect that resets draft when `data` changes can
overwrite the in-memory draft mid-save because each `updateMutation.mutateAsync`
triggers `queryClient.invalidateQueries`; to fix, guard that effect so it does
nothing while saves are in flight: update the effect that resets `draft` (the
one reading `data`) to return early if `updateMutation.isPending` (or
`updateMutation.isLoading`) is true (and add
`updateMutation`/`updateMutation.isPending` to the effect deps), so
`handleSave`'s sequential `updateMutation.mutateAsync` calls won't have the
draft clobbered by a refetch.
In `@src/views/PluginsView/PluginConfigField.tsx`:
- Around line 46-55: The error element currently sets aria-describedby on itself
and the computed describedBy is never applied to the input control; remove
aria-describedby from the <p> error element (keep its id like
`${labelId}-error`) and pass the describedBy variable into the renderControl
call so the actual form control components (Switch, SelectTrigger, Input)
receive aria-describedby={describedBy}; update the renderControl
implementation/call sites to accept the describedBy prop and add
aria-describedby to those components so screen readers reference the error from
the input.
---
Nitpick comments:
In `@src-tauri/src/domain/model/plugin.rs`:
- Around line 353-360: The Array branch in ConfigFieldType::Array currently only
checks leading/trailing brackets on value, which allows malformed JSON arrays;
update the validation in the ConfigFieldType::Array match arm (where
value.trim() is used) to parse the string with
serde_json::from_str::<serde_json::Value>(trimmed) (or from_str::<Vec<_>> if you
want to enforce array type) and return a DomainError::ValidationError including
the serde_json parse error if parsing fails, instead of only checking
starts_with/ends_with.
In `@src/views/PluginsView/__tests__/PluginConfigField.test.tsx`:
- Around line 20-127: Add two tests to PluginConfigField.test.tsx that exercise
the new "float" and "array" branches: for fieldType: "float" (use makeField({
fieldType: "float" })) render PluginConfigField and assert the labeled control
is an HTMLInputElement with type "number" and an appropriate step attribute
(e.g., "any" or a numeric step) to allow decimals; for fieldType: "array" (use
makeField({ fieldType: "array" })) render PluginConfigField with an array value
and assert the component renders the expected array control (e.g., a textarea or
list of inputs) and that the displayed value matches the provided array
(serialized/visible), using the same patterns as the existing tests
(screen.getByLabelText / getByText) to locate controls.
🪄 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: f11eff2a-66fb-4d4f-b758-bdd0b9ef29b1
📒 Files selected for processing (32)
CHANGELOG.mdsrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driven/plugin/registry.rssrc-tauri/src/adapters/driven/sqlite/entities/mod.rssrc-tauri/src/adapters/driven/sqlite/entities/plugin_config.rssrc-tauri/src/adapters/driven/sqlite/migrations/m20260425_000005_create_plugin_configs.rssrc-tauri/src/adapters/driven/sqlite/migrations/mod.rssrc-tauri/src/adapters/driven/sqlite/mod.rssrc-tauri/src/adapters/driven/sqlite/plugin_config_repo.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/command_bus.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/update_plugin_config.rssrc-tauri/src/application/queries/get_plugin_config.rssrc-tauri/src/application/queries/mod.rssrc-tauri/src/application/query_bus.rssrc-tauri/src/application/read_models/mod.rssrc-tauri/src/application/read_models/plugin_config_view.rssrc-tauri/src/domain/model/plugin.rssrc-tauri/src/domain/ports/driven/mod.rssrc-tauri/src/domain/ports/driven/plugin_config_store.rssrc-tauri/src/domain/ports/driven/plugin_loader.rssrc-tauri/src/lib.rssrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/types/plugin-config.tssrc/views/PluginsView.tsxsrc/views/PluginsView/PluginConfigDialog.tsxsrc/views/PluginsView/PluginConfigField.tsxsrc/views/PluginsView/PluginStoreRow.tsxsrc/views/PluginsView/__tests__/PluginConfigField.test.tsx
There was a problem hiding this comment.
6 issues found across 32 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-tauri/src/application/queries/get_plugin_config.rs">
<violation number="1" location="src-tauri/src/application/queries/get_plugin_config.rs:76">
P3: This test module duplicates large shared query-test scaffolding instead of reusing `application/test_support.rs`, increasing maintenance cost and drift risk.</violation>
</file>
<file name="src/views/PluginsView.tsx">
<violation number="1" location="src/views/PluginsView.tsx:122">
P2: The config query key is tied to filtered UI results, so typing in search or changing category triggers unnecessary refetches and cache churn.</violation>
</file>
<file name="src/types/plugin-config.ts">
<violation number="1" location="src/types/plugin-config.ts:8">
P2: `array` is declared as a field type, but UI rendering/validation does not handle it, so array config values are treated as unvalidated plain text.</violation>
</file>
<file name="src-tauri/src/domain/model/plugin.rs">
<violation number="1" location="src-tauri/src/domain/model/plugin.rs:355">
P2: Array field validation is too weak: it accepts invalid JSON arrays by only checking `[` and `]`.</violation>
<violation number="2" location="src-tauri/src/domain/model/plugin.rs:576">
P2: Malformed character classes are not rejected, so invalid regex patterns can still match and pass validation.</violation>
</file>
<file name="src/views/PluginsView/PluginConfigDialog.tsx">
<violation number="1" location="src/views/PluginsView/PluginConfigDialog.tsx:34">
P2: Reject empty input before numeric coercion in float validation; otherwise an empty field is treated as `0` in the UI and can pass client-side checks incorrectly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| | "float" | ||
| | "url" | ||
| | "enum" | ||
| | "array"; |
There was a problem hiding this comment.
P2: array is declared as a field type, but UI rendering/validation does not handle it, so array config values are treated as unvalidated plain text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/types/plugin-config.ts, line 8:
<comment>`array` is declared as a field type, but UI rendering/validation does not handle it, so array config values are treated as unvalidated plain text.</comment>
<file context>
@@ -0,0 +1,24 @@
+ | "float"
+ | "url"
+ | "enum"
+ | "array";
+
+export interface PluginConfigField {
</file context>
| PluginLoader, PluginReadRepository, StatsRepository, | ||
| }; | ||
|
|
||
| struct FakeArchiveExtractor; |
There was a problem hiding this comment.
P3: This test module duplicates large shared query-test scaffolding instead of reusing application/test_support.rs, increasing maintenance cost and drift risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/src/application/queries/get_plugin_config.rs, line 76:
<comment>This test module duplicates large shared query-test scaffolding instead of reusing `application/test_support.rs`, increasing maintenance cost and drift risk.</comment>
<file context>
@@ -0,0 +1,350 @@
+ PluginLoader, PluginReadRepository, StatsRepository,
+ };
+
+ struct FakeArchiveExtractor;
+ impl ArchiveExtractor for FakeArchiveExtractor {
+ fn detect_format(
</file context>
- Translate task 15 changelog entry from FR to EN - Validate plugin manifest defaults against their declared schema - Drop if_not_exists on plugin_configs migration; add if_exists on rollback - Apply runtime config first, persist second, roll back runtime on persist failure - Drop persisted config values that no longer match the schema before returning - Use i64 bounds for integer config validation (avoid f64 precision loss) - Validate JSON array structure (balanced brackets, escaped strings) instead of just '[' and ']' - Reject unterminated regex character classes by returning a never-matching empty class - Reject empty input on the float validator instead of treating it as 0 - Skip schema-reset effect while a save is in flight to avoid clobbering the draft - Validate JSON arrays on the frontend before submitting - Move aria-describedby from the error <p> to the input control (correct a11y semantics) - Key the per-plugin config query off the unfiltered installed list so search/category churn doesn't refetch
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/domain/model/plugin.rs (1)
532-593: Consider adding recursion depth limit.The recursive
regex_match_fromfunction could stack overflow on deeply nested patterns with many*/+quantifiers. While plugin config regexes are typically simple, a malicious manifest could trigger this.💡 Optional: Add depth parameter
-fn regex_match_from(pat: &[char], val: &[char], start: usize, anchor_end: bool) -> bool { +fn regex_match_from(pat: &[char], val: &[char], start: usize, anchor_end: bool) -> bool { + regex_match_from_depth(pat, val, start, anchor_end, 0) +} + +fn regex_match_from_depth(pat: &[char], val: &[char], start: usize, anchor_end: bool, depth: usize) -> bool { + if depth > 100 { + return false; // Bail on deeply nested patterns + } // ... rest of implementation, passing depth + 1 to recursive calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/model/plugin.rs` around lines 532 - 593, The recursive regex_match_from can overflow on deeply nested/ambiguous patterns; add a recursion depth parameter (e.g., depth: usize) with a sensible max constant (e.g., MAX_REGEX_RECURSION_DEPTH) and check at the start of regex_match_from to return false (or an error sentinel) when depth >= MAX_REGEX_RECURSION_DEPTH, and then pass depth + 1 on every recursive call (calls inside the '*', '+', '?' branches and any other recursive invocation); update any callers to start with depth = 0 and adjust unit tests/uses accordingly so the function bounds recursion safely.
🤖 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/update_plugin_config.rs`:
- Around line 55-62: When store.set_value(&cmd.plugin_name, &cmd.key,
&cmd.value) fails we currently only roll back to a previous value if Some(prev)
exists, leaving the new value in the runtime when persistence failed; change the
error path so that if previous is None you remove the runtime config entry
instead of leaving it set by calling the PluginLoader removal API (e.g.
plugin_loader().remove_runtime_config(&cmd.plugin_name, &cmd.key)) before
returning Err(AppError::Domain(e)); if PluginLoader does not expose a removal
method, add one or document this as a known limitation and ensure the code
attempts removal where supported.
---
Nitpick comments:
In `@src-tauri/src/domain/model/plugin.rs`:
- Around line 532-593: The recursive regex_match_from can overflow on deeply
nested/ambiguous patterns; add a recursion depth parameter (e.g., depth: usize)
with a sensible max constant (e.g., MAX_REGEX_RECURSION_DEPTH) and check at the
start of regex_match_from to return false (or an error sentinel) when depth >=
MAX_REGEX_RECURSION_DEPTH, and then pass depth + 1 on every recursive call
(calls inside the '*', '+', '?' branches and any other recursive invocation);
update any callers to start with depth = 0 and adjust unit tests/uses
accordingly so the function bounds recursion safely.
🪄 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: 2bc9a08f-887b-4634-a01b-41a28a3e04f0
📒 Files selected for processing (9)
CHANGELOG.mdsrc-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driven/sqlite/migrations/m20260425_000005_create_plugin_configs.rssrc-tauri/src/application/commands/update_plugin_config.rssrc-tauri/src/application/queries/get_plugin_config.rssrc-tauri/src/domain/model/plugin.rssrc/views/PluginsView.tsxsrc/views/PluginsView/PluginConfigDialog.tsxsrc/views/PluginsView/PluginConfigField.tsx
✅ Files skipped from review due to trivial changes (1)
- src-tauri/src/adapters/driven/plugin/manifest.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src-tauri/src/adapters/driven/sqlite/migrations/m20260425_000005_create_plugin_configs.rs
- src-tauri/src/application/queries/get_plugin_config.rs
- CHANGELOG.md
| if let Err(e) = store.set_value(&cmd.plugin_name, &cmd.key, &cmd.value) { | ||
| if let Some(prev) = previous { | ||
| let _ = self | ||
| .plugin_loader() | ||
| .set_runtime_config(&cmd.plugin_name, &cmd.key, &prev); | ||
| } | ||
| return Err(AppError::Domain(e)); | ||
| } |
There was a problem hiding this comment.
Incomplete rollback when no previous value exists.
If this is the first time a key is being set (no previous value) and persistence fails, the runtime will have the new value while disk does not. Consider removing the runtime config entry in this case.
🔧 Proposed fix
if let Err(e) = store.set_value(&cmd.plugin_name, &cmd.key, &cmd.value) {
if let Some(prev) = previous {
let _ = self
.plugin_loader()
.set_runtime_config(&cmd.plugin_name, &cmd.key, &prev);
+ } else {
+ // No previous value existed; ideally remove the key from runtime.
+ // If PluginLoader doesn't support removal, this is a known limitation.
}
return Err(AppError::Domain(e));
}Note: This requires PluginLoader to support removing a runtime config key, which may not be implemented. If not, document this as a known limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/application/commands/update_plugin_config.rs` around lines 55 -
62, When store.set_value(&cmd.plugin_name, &cmd.key, &cmd.value) fails we
currently only roll back to a previous value if Some(prev) exists, leaving the
new value in the runtime when persistence failed; change the error path so that
if previous is None you remove the runtime config entry instead of leaving it
set by calling the PluginLoader removal API (e.g.
plugin_loader().remove_runtime_config(&cmd.plugin_name, &cmd.key)) before
returning Err(AppError::Domain(e)); if PluginLoader does not expose a removal
method, add one or document this as a known limitation and ensure the code
attempts removal where supported.
There was a problem hiding this comment.
3 issues found across 9 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/PluginConfigDialog.tsx">
<violation number="1" location="src/views/PluginsView/PluginConfigDialog.tsx:115">
P2: Including `updateMutation.isPending` in this reset effect causes draft values to be overwritten when a mutation settles, which can discard user edits after a failed save.</violation>
</file>
<file name="src-tauri/src/application/commands/update_plugin_config.rs">
<violation number="1" location="src-tauri/src/application/commands/update_plugin_config.rs:43">
P2: Rollback only uses persisted values, so a failed save can leave runtime config mutated when the prior effective value came from manifest defaults.</violation>
</file>
<file name="src-tauri/src/domain/model/plugin.rs">
<violation number="1" location="src-tauri/src/domain/model/plugin.rs:324">
P2: Integer bound checks truncate `min`/`max` to `i64`, which can incorrectly accept out-of-range values when bounds are fractional.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Use ceil/floor on integer bound checks so fractional min/max (e.g. min=1.5) reject the integer 1 instead of widening the range via i64 truncation - Roll back runtime config to the manifest default when no persisted value exists, so a failed save no longer leaves the runtime mutated past the prior effective value - Replace the isPending-based draft reset with a one-shot ref keyed on pluginName, so a failed save no longer overwrites the user's edits when the mutation settles; collapse the per-iteration invalidate into a single invalidate after the save loop
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src-tauri/src/domain/model/plugin.rs (1)
801-819: Add a fractional-bound regression case here.This test still only covers integer
min/max, so it won't catch theceil/floorbehavior that motivated the latest fix. A case likemin = 1.5rejecting"1"and accepting"2"would lock that in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/model/plugin.rs` around lines 801 - 819, Extend the existing test_config_field_validate_integer_parses_and_checks_bounds test to include a fractional-bound regression case: create a ConfigField::new(ConfigFieldType::Integer) with with_min(1.5) (and optionally with_max(2.5)), then assert that f.validate("1") returns a ValidationError (rejected), f.validate("2") is Ok (accepted), and if you add max 2.5 assert f.validate("3") returns a ValidationError while "2" remains Ok; use the same f.validate(...) and DomainError::ValidationError pattern used in the existing assertions.
🤖 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/domain/model/plugin.rs`:
- Around line 371-377: The Array branch in ConfigFieldType::Array currently
trusts is_json_array_structure and accepts malformed arrays and never enforces
min/max element counts; update the validation in the ConfigFieldType::Array
handling (the match arm using is_json_array_structure and returning
DomainError::ValidationError) to parse `value` with a strict JSON parser (e.g.,
serde_json::from_str::<Vec<...>>) and reject parse errors, then apply the
field's min/max element constraints to the parsed Vec length and return a
ValidationError with a clear message if out of range; ensure the same rigorous
parsing-and-count-check logic is applied to the other Array handling locations
noted (lines ~451-499).
- Around line 501-649: The current mini-regex in
match_regex/regex_match_from/parse_atom/Atom silently treats
group/alternation/counted-quantifier syntax as literals; update parse_atom (or
add a validate_pattern function called from match_regex) to detect and reject
unsupported tokens like '(', ')', '|', '{', '}', and other constructs you don't
implement (returning a clear malformed result/false or an explicit validation
error) so manifests using real regex features fail early, or alternatively move
matching out of this domain layer into a higher layer that can depend on a full
regex engine and replace calls to match_regex with that engine there.
---
Nitpick comments:
In `@src-tauri/src/domain/model/plugin.rs`:
- Around line 801-819: Extend the existing
test_config_field_validate_integer_parses_and_checks_bounds test to include a
fractional-bound regression case: create a
ConfigField::new(ConfigFieldType::Integer) with with_min(1.5) (and optionally
with_max(2.5)), then assert that f.validate("1") returns a ValidationError
(rejected), f.validate("2") is Ok (accepted), and if you add max 2.5 assert
f.validate("3") returns a ValidationError while "2" remains Ok; use the same
f.validate(...) and DomainError::ValidationError pattern used in the existing
assertions.
🪄 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: 93e2050a-ba55-40d3-afc0-047aa6321984
📒 Files selected for processing (3)
src-tauri/src/application/commands/update_plugin_config.rssrc-tauri/src/domain/model/plugin.rssrc/views/PluginsView/PluginConfigDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/PluginsView/PluginConfigDialog.tsx
- src-tauri/src/application/commands/update_plugin_config.rs
- Replace the structural array validator with a strict recursive-descent JSON parser so [1 2], [1,], [tru] now reject; apply min/max to the array element count - Reject regex patterns with unsupported features (alternation, groups, counted quantifiers) at manifest parse time and from validate(), so plugin authors get a clear error instead of a silent match failure - Add tests covering the new array parser, array count bounds, regex feature detector, and the manifest parser rejecting unsupported regex patterns
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/domain/model/plugin.rs`:
- Around line 392-399: The current check using unsupported_regex_feature()
misses malformed patterns (e.g., unclosed '[' or trailing '\'), so update the
regex-handling in the code path that sees self.regex (inside the block using
unsupported_regex_feature and match_regex in plugin.rs) to attempt a syntax
validation by compiling the pattern (e.g., try Regex::new(pattern)) and if
compilation fails return a DomainError::ValidationError that includes the
compilation error message; alternatively perform the same compile-and-fail-fast
validation during manifest parsing in build_config_schema (adapter driven plugin
manifest parsing) so invalid manifests are rejected up-front rather than later
producing generic “does not match regex” errors.
- Around line 595-665: parse_string currently accepts any escaped char and
control characters; update parse_string to validate JSON string escapes: allow
only the standard escapes (\" \\ \/ \b \f \n \r \t) and \u followed by exactly
four hex digits, and reject raw control characters (U+0000..U+001F); also ensure
it fails if EOF is reached before the closing quote. For parse_number, enforce
JSON numeric syntax: disallow leading zeros for integers (only "0" is valid),
require at least one digit in integer and fractional/exponent parts as already
done, and reject a leading plus sign; keep using the same position updates and
return semantics. Use the existing function names parse_string and parse_number
to locate where to add these validations.
- Around line 344-349: The ConfigFieldType::Float branch currently parses floats
but allows NaN/inf through; after parsing the f64 in the ConfigFieldType::Float
match arm (the parsed variable), explicitly reject non-finite values by checking
parsed.is_finite() and returning a DomainError::ValidationError with a clear
message (e.g. "expected finite float, got '{value}'") if it is not finite,
before calling self.check_numeric_bounds(parsed)?; this uses the existing
DomainError::ValidationError and check_numeric_bounds symbols to locate where to
add the is_finite() guard.
🪄 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: 45db62fb-ac57-4c09-bf96-ce777090c9ef
📒 Files selected for processing (2)
src-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/domain/model/plugin.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/adapters/driven/plugin/manifest.rs
There was a problem hiding this comment.
3 issues 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="src-tauri/src/domain/model/plugin.rs">
<violation number="1" location="src-tauri/src/domain/model/plugin.rs:602">
P2: String escape handling is too permissive, so malformed JSON arrays can pass validation as valid.</violation>
<violation number="2" location="src-tauri/src/domain/model/plugin.rs:623">
P2: Number parsing allows leading-zero integers, which makes malformed JSON pass array validation.</violation>
<violation number="3" location="src-tauri/src/domain/model/plugin.rs:675">
P2: Add regex syntax validation during schema construction; the current unsupported-feature scan allows malformed patterns (such as an unclosed `[` or trailing `\`) to be accepted and then fail all runtime matches.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- JSON parser now rejects invalid string escapes (\q etc.) and unescaped control characters per RFC 8259 - JSON parser now rejects leading-zero integers ([01], [001], [-01]) per RFC 8259 - Reject NaN, +/-Infinity in float validation: f64::from_str accepts them and NaN compares false against any bound, silently bypassing min/max - Reject malformed regex patterns (unclosed [, trailing \) at manifest parse time and from validate(), so plugin authors get a clear error instead of silent never-matches - Frontend float validator uses Number.isFinite to reject 'Infinity' inline, matching the backend's rejection - Tests for invalid escapes, leading zeros, NaN/inf rejection, malformed-regex detection, and the manifest parser rejecting malformed patterns
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/views/PluginsView/PluginConfigDialog.tsx (2)
32-37: Integer validation accepts values outside safe integer range.The regex
/^-?\d+$/accepts arbitrarily large integers, butNumber(value)loses precision for integers outside ±2⁵³. This can cause the bounds check to behave incorrectly for very large values. The backend usesi64::parse()which has a different range.This is unlikely to matter for typical plugin configs, but worth noting for completeness.
♻️ Optional: Add BigInt check for precision safety
if (field.fieldType === "integer") { if (!/^-?\d+$/.test(value)) return "Must be an integer"; const n = Number(value); + // Check if value exceeds safe integer range + if (!Number.isSafeInteger(n)) return "Integer too large"; if (field.min !== null && n < field.min) return `Min ${field.min}`; if (field.max !== null && n > field.max) return `Max ${field.max}`; return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PluginsView/PluginConfigDialog.tsx` around lines 32 - 37, The integer validation currently uses Number(value) which loses precision for values outside the safe integer range; change the check to parse with BigInt to preserve precision: try to construct BigInt(value) (catch and return "Must be an integer" on error), then compare the BigInt against bounds by converting field.min/field.max to BigInt (or if those can be null leave comparisons conditional) instead of using Number(value); optionally also clamp/validate against the backend i64 range by comparing to BigInt("-9223372036854775808") and BigInt("9223372036854775807") so the behavior matches i64::parse(), all inside the same block that tests field.fieldType === "integer" and using value, field.min, field.max.
59-67: Frontend array validation is more lenient than backend.The frontend uses
JSON.parse()which accepts payloads like[01](leading zeros) and["\q"](invalid escapes) that the backend's strict RFC 8259 parser rejects. This mismatch means the UI shows no error, but the backend returns a validation failure toast.Consider using stricter validation or accepting the backend as authoritative (current behavior is acceptable since backend validates).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PluginsView/PluginConfigDialog.tsx` around lines 59 - 67, The current client-side array validation in PluginConfigDialog.tsx (inside the block checking field.fieldType === "array") uses JSON.parse which accepts non-RFC8259 payloads (see JSON.parse usage and parsed variable); replace this with a stricter RFC 8259-compliant check or remove the eager parse and defer to backend validation: either integrate a strict JSON parser/validator library that enforces RFC8259 for the value before accepting it, or change the validation path to only check for non-empty input and let the backend return the authoritative error (update the error messages accordingly).
🤖 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/views/PluginsView/PluginConfigDialog.tsx`:
- Around line 32-37: The integer validation currently uses Number(value) which
loses precision for values outside the safe integer range; change the check to
parse with BigInt to preserve precision: try to construct BigInt(value) (catch
and return "Must be an integer" on error), then compare the BigInt against
bounds by converting field.min/field.max to BigInt (or if those can be null
leave comparisons conditional) instead of using Number(value); optionally also
clamp/validate against the backend i64 range by comparing to
BigInt("-9223372036854775808") and BigInt("9223372036854775807") so the behavior
matches i64::parse(), all inside the same block that tests field.fieldType ===
"integer" and using value, field.min, field.max.
- Around line 59-67: The current client-side array validation in
PluginConfigDialog.tsx (inside the block checking field.fieldType === "array")
uses JSON.parse which accepts non-RFC8259 payloads (see JSON.parse usage and
parsed variable); replace this with a stricter RFC 8259-compliant check or
remove the eager parse and defer to backend validation: either integrate a
strict JSON parser/validator library that enforces RFC8259 for the value before
accepting it, or change the validation path to only check for non-empty input
and let the backend return the authoritative error (update the error messages
accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1218f64e-eb11-4bc0-b2af-4f23d317f1ca
📒 Files selected for processing (3)
src-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/domain/model/plugin.rssrc/views/PluginsView/PluginConfigDialog.tsx
Backend already bounds array length, but the dialog only validated the JSON shape, so a schema declaring min=1 still let the user save [] and surfaced a generic backend toast instead of an inline field error.
… replay Replay now runs after plugin loading so each persisted (key, value) is checked against the current schema before being inserted into the runtime map. Entries that no longer match (e.g. after a plugin update narrowed an enum, tightened a regex, or dropped a key) are skipped with a warning, so WASM plugins never observe a stale schema-invalid value via get_config.
| let (anchor_end, body) = if body.last() == Some(&'$') { | ||
| (true, &body[..body.len() - 1]) | ||
| } else { | ||
| (false, body) | ||
| }; |
There was a problem hiding this comment.
End-anchor stripping does not check for an escaped
\$
body.last() == Some(&'$') strips the $ as a positional anchor without verifying that the preceding character is not a backslash. A pattern like ^[A-Z]+\$ ends with the two characters \ and $. The anchor-end check fires, anchor_end becomes true, and the body shrinks to [A-Z]+\ — meaning the backslash becomes a dangling escape, causing match_regex to return incorrect results for any value.
let (anchor_end, body) = {
let escaped = body.len() >= 2 && body[body.len() - 2] == '\\';
if !escaped && body.last() == Some(&'$') {
(true, &body[..body.len() - 1])
} else {
(false, body)
}
};Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/domain/model/plugin.rs
Line: 788-792
Comment:
**End-anchor stripping does not check for an escaped `\$`**
`body.last() == Some(&'$')` strips the `$` as a positional anchor without verifying that the preceding character is not a backslash. A pattern like `^[A-Z]+\$` ends with the two characters `\` and `$`. The anchor-end check fires, `anchor_end` becomes `true`, and the body shrinks to `[A-Z]+\` — meaning the backslash becomes a dangling escape, causing `match_regex` to return incorrect results for any value.
```rust
let (anchor_end, body) = {
let escaped = body.len() >= 2 && body[body.len() - 2] == '\\';
if !escaped && body.last() == Some(&'$') {
(true, &body[..body.len() - 1])
} else {
(false, body)
}
};
```
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 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/lib.rs">
<violation number="1" location="src-tauri/src/lib.rs:203">
P2: Skipping replay for not-yet-loaded plugins drops their persisted overrides for later runtime loads, so those plugins fall back to manifest defaults in the current session.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Move the schema-validation pass from lib.rs into build_host_functions so it runs every time a plugin loads (startup or hot-load via the file watcher). Replay in lib.rs is back to inserting raw values, so plugins that load later in the session keep their persisted overrides instead of falling back to manifest defaults.
Summary
• Plugin config schema parsing from
plugin.tomlwith dual validation (UI + backend)• SQLite persistence via new
plugin_configstable andPluginConfigStoreport• CQRS handlers:
GetPluginConfigQuery(schema + values) andUpdatePluginConfigCommand(validate + persist)• TypeScript React form dispatcher rendering all field types (string, integer, float, boolean, url, enum, array)
• Tauri IPC:
plugin_config_get(name)andplugin_config_update(name, key, value)commands• Runtime config sync: persisted values replayed to
SharedHostResources::plugin_configsat startup• i18n support (en/fr) with 11 new translation keys
Type
feat
Summary by cubic
Adds a schema‑driven plugin configuration UI powered by
[config]inplugin.toml. Values are validated in the UI and backend, stored in SQLite, exposed via IPC, applied live to running plugins, and validated at plugin load to avoid stale values; fulfills task 15.New Features
[config]schema with defaults and constraints (string, integer, float, boolean, url, enum, array withmin/max/regex/options); new domain types (ConfigField,ConfigFieldType,PluginConfigSchema).plugin_configs(SQLite) andPluginConfigStore; replay at startup; live sync via loaderset_runtime_config;get_manifestexposes schema.GetPluginConfigQuery,UpdatePluginConfigCommand, and Tauriplugin_config_get(name)/plugin_config_update(name, key, value).PluginConfigDialogwith dynamic fields and client‑side validation; “Configure” only shows when a plugin declares fields; i18n (en/fr).Bug Fixes
get_config.min/maxand RFC‑8259 checks; regex checks reject unsupported features and malformed patterns at parse and validate.aria-describedby; stable query key; single invalidate after save; float validator usesNumber.isFinite.Written for commit 6f56f48. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests