feat: complete Phase 2 plugin system (config validation + dev inspector)#127
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a pure validateConfigValue() function that validates config values against their PluginConfigSchemaField definitions, covering boolean, string, number (with min/max), enum (with options membership), and range (with min/max) types. Includes 15 TDD test cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ror display Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request extends the plugin system with support for enum and range configuration field types, introduces a validateConfigValue API to validate config changes before persisting, tracks per-plugin load timings during execution, and adds a development-only plugin runtime inspector panel to display runtime status and diagnostics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/pluginScanner.ts`:
- Around line 11-22: Remove the local redeclaration of PluginConfigSchemaField
in pluginScanner.ts and import the canonical type from the plugin API types
module (the exported PluginConfigSchemaField in
packages/plugin-api/src/types.ts); replace all local references/usages with the
imported symbol (e.g., update any function signatures or variables that used the
local interface to use the imported PluginConfigSchemaField) so the scanner uses
the single source of truth and will follow future schema changes.
In `@apps/desktop/src/renderer/pages/settings/sections/PluginsSection.tsx`:
- Around line 445-449: The inspector toggle button (className inspectorToggle,
onClick calling setOpen) is missing accessibility attributes; update the button
in PluginsSection to include aria-expanded={open} and add an aria-controls
attribute pointing to the id of the collapsible region (give the controlled
region a unique id, e.g., inspector-panel or similar) so screen readers can
detect state and relation; make the same change for the other toggle instance at
the 463-464 area and ensure the region element has the matching id and
appropriate role (region or toolbar) as needed.
- Around line 591-601: The current code only validates when a schema field
exists, allowing unknown keys to be written; update the logic around
BUILT_IN_CONFIG_SCHEMAS and plugins lookup so missing schema entries are treated
as invalid: after computing const schema = BUILT_IN_CONFIG_SCHEMAS[pluginId] ??
plugins.find(p => p.id === pluginId)?.configSchema and const field =
schema?.[key], if field is undefined log a clear warning referencing pluginId
and key (similar to the existing console.warn) and return early instead of
calling pluginConfig.set; preserve the existing validateConfigValue(path) flow
when field exists.
In `@apps/desktop/src/renderer/stores/pluginRuntimeStore.ts`:
- Around line 88-95: The timing logic only records successful scanned plugin
loads via loadPluginFromSource(sp.code, sp.id) and skips failures and init.js
entries, so update the code around the manifest handling to always push a
timings entry for each scanned plugin (use sp.id and sp.name) regardless of
manifest truthiness and also record failures with an error or status field;
additionally, ensure the same change is applied to the other loading branch
referenced (the block around lines 104-124) so init.js loads and slow failures
are represented in the timings array and the inspector/pluginCount stays
consistent.
In `@docs/plans/2026-03-11-phase2-completion-implementation.md`:
- Around line 220-251: The numeric branches in the validator (the switch cases
'number' and 'range') must reject non-finite values and the 'range' branch must
also enforce the declared step; update the 'number' case to call
Number.isFinite(value) (return invalid with a clear reason if not finite) in
addition to the existing typeof check, and update the 'range' case to similarly
require Number.isFinite(value) and then, if field.step is provided, verify that
(value - (field.min ?? 0)) is an integer multiple of field.step within a small
epsilon tolerance (reject if not), while keeping the existing min/max checks and
error messages; refer to the switch cases 'number' and 'range' and the field
properties field.min, field.max, and field.step when making the changes.
- Around line 22-38: Replace the local PluginConfigSchemaField interface in
pluginScanner.ts with the shared type from the canonical package: remove the
local interface declaration and add an import like `import type {
PluginConfigSchemaField } from '@readied/plugin-api'` (or re-export that type
from a single source used across the repo), then update any local usages to
reference that imported symbol to ensure enum/range fields stay in sync.
- Around line 305-323: handleConfigChange currently aborts silently on invalid
input after calling validateConfigValue, causing the UI save to appear to fail;
change handleConfigChange to return the validation result instead of just doing
console.warn + return so the caller can show inline/toast feedback.
Specifically, in the handleConfigChange callback (and where validateConfigValue
is referenced), when result.valid is false return or resolve an object like {
valid: false, reason: result.reason } (or throw a specific ValidationError)
instead of just returning void, and only call window.readied.pluginConfig.set
and setConfigValues when result.valid is true; ensure the function signature and
any callers can handle the returned validation result.
- Line 13: Update the heading level for "Task 1: Fix pluginScanner Type Bug"
from "###" to "##" so the document doesn't jump from top-level to H3 and violate
markdownlint MD001; also verify and adjust the subsequent task headings (e.g.,
any other "### Task ..." headings) to be the same "##" level to keep a
consistent heading hierarchy.
- Around line 487-503: The inspector toggle button lacks disclosure
semantics—update the button (the element using
className={styles.inspectorToggle} and the open state controlled by open and
setOpen) to include aria-expanded={open} and aria-controls pointing to the
expandable content container's id; add a stable id (e.g., generate a unique
string or use a constant) to the developer panel container (the element that
renders the expandable content and currently shows errors via
styles.inspectorErrorBadge) and ensure the button's aria-controls value matches
that id so screen readers can map the toggle to the panel.
In `@packages/plugin-api/src/validation.ts`:
- Around line 127-137: The range validator currently ignores field.step; update
the 'range' case in the validation logic (in
packages/plugin-api/src/validation.ts) to, after min/max checks, if field.step
is a positive number compute base = field.min ?? 0 and require that (value -
base) is an integer multiple of step (use a small epsilon to handle
floating-point rounding) and return valid:false with a clear reason if it is
not; also guard against non-positive step values by treating them as "no step"
or returning an explicit validation error depending on your schema rules.
- Around line 106-137: The number and range validators (in the 'case "number"'
and 'case "range"' branches of validate logic) must explicitly reject non-finite
numeric values; add a check using Number.isFinite(value) after the typeof check
and return invalid if it fails, and ensure the error message mentions non-finite
values (e.g., NaN/Infinity). Also review PluginConfigSchemaField.step and decide
whether to enforce step alignment (if so, add validation that (value -
(field.min || 0)) % field.step === 0 or equivalent); otherwise document that
step is not validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aeeee292-4232-4c9f-90a3-14fbc4b35f45
📒 Files selected for processing (9)
apps/desktop/src/main/pluginScanner.tsapps/desktop/src/renderer/pages/settings/sections/PluginsSection.tsxapps/desktop/src/renderer/pages/settings/sections/Section.module.cssapps/desktop/src/renderer/stores/pluginRuntimeStore.tsdocs/plans/2026-03-11-phase2-completion-design.mddocs/plans/2026-03-11-phase2-completion-implementation.mdpackages/plugin-api/src/index.tspackages/plugin-api/src/validation.tspackages/plugin-api/tests/validation.test.ts
Comment on lines
+11
to
+22
| export interface PluginConfigSchemaField { | ||
| type: 'string' | 'number' | 'boolean'; | ||
| type: 'string' | 'number' | 'boolean' | 'enum' | 'range'; | ||
| default: unknown; | ||
| description?: string; | ||
| /** For 'enum' type: available options */ | ||
| options?: Array<{ value: string; label: string }>; | ||
| /** For 'range' type: minimum value */ | ||
| min?: number; | ||
| /** For 'range' type: maximum value */ | ||
| max?: number; | ||
| /** For 'range' type: step increment */ | ||
| step?: number; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Stop re-declaring PluginConfigSchemaField in the scanner.
This PR fixes the current drift, but the next field added to packages/plugin-api/src/types.ts will silently diverge again for the same reason. Import the canonical schema type here instead of maintaining a second definition.
As per coding guidelines, "Main process code. Check IPC handler patterns and sync service consistency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/pluginScanner.ts` around lines 11 - 22, Remove the
local redeclaration of PluginConfigSchemaField in pluginScanner.ts and import
the canonical type from the plugin API types module (the exported
PluginConfigSchemaField in packages/plugin-api/src/types.ts); replace all local
references/usages with the imported symbol (e.g., update any function signatures
or variables that used the local interface to use the imported
PluginConfigSchemaField) so the scanner uses the single source of truth and will
follow future schema changes.
Comment on lines
+445
to
+449
| <button | ||
| type="button" | ||
| className={styles.inspectorToggle} | ||
| onClick={() => setOpen(prev => !prev)} | ||
| > |
There was a problem hiding this comment.
Expose the inspector’s expanded state to assistive tech.
The toggle is a collapsible control, but it does not set aria-expanded or link to the controlled region. Screen-reader users cannot tell whether the panel is open.
Suggested accessibility fix
function PluginInspector() {
const [open, setOpen] = useState(false);
+ const contentId = 'plugin-inspector-content';
@@
<button
type="button"
className={styles.inspectorToggle}
onClick={() => setOpen(prev => !prev)}
+ aria-expanded={open}
+ aria-controls={contentId}
>
@@
{open && (
- <div className={styles.inspectorContent}>
+ <div id={contentId} className={styles.inspectorContent}>Also applies to: 463-464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/pages/settings/sections/PluginsSection.tsx` around
lines 445 - 449, The inspector toggle button (className inspectorToggle, onClick
calling setOpen) is missing accessibility attributes; update the button in
PluginsSection to include aria-expanded={open} and add an aria-controls
attribute pointing to the id of the collapsible region (give the controlled
region a unique id, e.g., inspector-panel or similar) so screen readers can
detect state and relation; make the same change for the other toggle instance at
the 463-464 area and ensure the region element has the matching id and
appropriate role (region or toolbar) as needed.
Comment on lines
+591
to
+601
| // Find the schema field for validation | ||
| const schema = BUILT_IN_CONFIG_SCHEMAS[pluginId] ?? plugins.find(p => p.id === pluginId)?.configSchema; | ||
| const field = schema?.[key]; | ||
|
|
||
| if (field) { | ||
| const result = validateConfigValue(field, value); | ||
| if (!result.valid) { | ||
| console.warn(`[plugin:${pluginId}] Invalid config value for "${key}": ${result.reason}`); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Treat missing schema fields as invalid instead of falling through.
If schema lookup fails, this still writes the value via pluginConfig.set(). That keeps the new validation path as a best-effort check rather than a hard guard against unsupported keys/values.
Suggested guard
const schema = BUILT_IN_CONFIG_SCHEMAS[pluginId] ?? plugins.find(p => p.id === pluginId)?.configSchema;
const field = schema?.[key];
- if (field) {
- const result = validateConfigValue(field, value);
- if (!result.valid) {
- console.warn(`[plugin:${pluginId}] Invalid config value for "${key}": ${result.reason}`);
- return;
- }
+ if (!field) {
+ console.warn(`[plugin:${pluginId}] Unknown config key "${key}"`);
+ return;
+ }
+
+ const result = validateConfigValue(field, value);
+ if (!result.valid) {
+ console.warn(`[plugin:${pluginId}] Invalid config value for "${key}": ${result.reason}`);
+ return;
}📝 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.
Suggested change
| // Find the schema field for validation | |
| const schema = BUILT_IN_CONFIG_SCHEMAS[pluginId] ?? plugins.find(p => p.id === pluginId)?.configSchema; | |
| const field = schema?.[key]; | |
| if (field) { | |
| const result = validateConfigValue(field, value); | |
| if (!result.valid) { | |
| console.warn(`[plugin:${pluginId}] Invalid config value for "${key}": ${result.reason}`); | |
| return; | |
| } | |
| } | |
| // Find the schema field for validation | |
| const schema = BUILT_IN_CONFIG_SCHEMAS[pluginId] ?? plugins.find(p => p.id === pluginId)?.configSchema; | |
| const field = schema?.[key]; | |
| if (!field) { | |
| console.warn(`[plugin:${pluginId}] Unknown config key "${key}"`); | |
| return; | |
| } | |
| const result = validateConfigValue(field, value); | |
| if (!result.valid) { | |
| console.warn(`[plugin:${pluginId}] Invalid config value for "${key}": ${result.reason}`); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/pages/settings/sections/PluginsSection.tsx` around
lines 591 - 601, The current code only validates when a schema field exists,
allowing unknown keys to be written; update the logic around
BUILT_IN_CONFIG_SCHEMAS and plugins lookup so missing schema entries are treated
as invalid: after computing const schema = BUILT_IN_CONFIG_SCHEMAS[pluginId] ??
plugins.find(p => p.id === pluginId)?.configSchema and const field =
schema?.[key], if field is undefined log a clear warning referencing pluginId
and key (similar to the existing console.warn) and return early instead of
calling pluginConfig.set; preserve the existing validateConfigValue(path) flow
when field exists.
Comment on lines
+88
to
95
| const start = performance.now(); | ||
| const manifest = loadPluginFromSource(sp.code, sp.id); | ||
| const elapsed = performance.now() - start; | ||
|
|
||
| if (manifest) { | ||
| plugins.push(manifest); | ||
| timings.push({ pluginId: sp.id, pluginName: sp.name, loadTimeMs: elapsed }); | ||
| } else { |
There was a problem hiding this comment.
Timing capture currently misses failed loads and init.js.
timings is only populated for successfully loaded scanned plugins. Slow failures and a loaded init.js script never appear, so the new inspector can disagree with pluginCount and hide the cases developers most need for debugging.
Suggested shape
for (const sp of scanned) {
const enabled = stateMap.get(sp.id) ?? true;
if (!enabled) continue;
const start = performance.now();
const manifest = loadPluginFromSource(sp.code, sp.id);
const elapsed = performance.now() - start;
+ timings.push({ pluginId: sp.id, pluginName: sp.name, loadTimeMs: elapsed });
if (manifest) {
plugins.push(manifest);
- timings.push({ pluginId: sp.id, pluginName: sp.name, loadTimeMs: elapsed });
} else {
errors.push({
pluginId: sp.id,
pluginName: sp.name,
reason: 'Failed to load plugin code',
@@
if (initCode) {
+ const start = performance.now();
const initManifest = loadInitScript(initCode);
+ const elapsed = performance.now() - start;
+ timings.push({ pluginId: 'user-init', pluginName: 'init.js', loadTimeMs: elapsed });
if (initManifest) {
const enabled = stateMap.get(initManifest.id) ?? true;
if (enabled) {
plugins.push(initManifest);Also applies to: 104-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/stores/pluginRuntimeStore.ts` around lines 88 - 95,
The timing logic only records successful scanned plugin loads via
loadPluginFromSource(sp.code, sp.id) and skips failures and init.js entries, so
update the code around the manifest handling to always push a timings entry for
each scanned plugin (use sp.id and sp.name) regardless of manifest truthiness
and also record failures with an error or status field; additionally, ensure the
same change is applied to the other loading branch referenced (the block around
lines 104-124) so init.js loads and slow failures are represented in the timings
array and the inspector/pluginCount stays consistent.
|
|
||
| --- | ||
|
|
||
| ### Task 1: Fix pluginScanner Type Bug |
There was a problem hiding this comment.
Fix the heading hierarchy here.
This jumps from # to ###, which trips markdownlint MD001. Make this ## Task 1 (and keep the later task headings at the same level).
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-11-phase2-completion-implementation.md` at line 13, Update
the heading level for "Task 1: Fix pluginScanner Type Bug" from "###" to "##" so
the document doesn't jump from top-level to H3 and violate markdownlint MD001;
also verify and adjust the subsequent task headings (e.g., any other "### Task
..." headings) to be the same "##" level to keep a consistent heading hierarchy.
Comment on lines
+220
to
+251
| case 'number': | ||
| if (typeof value !== 'number') { | ||
| return { valid: false, reason: `Expected number, got ${typeof value}` }; | ||
| } | ||
| if (field.min !== undefined && value < field.min) { | ||
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | ||
| } | ||
| if (field.max !== undefined && value > field.max) { | ||
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | ||
| } | ||
| return { valid: true }; | ||
|
|
||
| case 'enum': | ||
| if (!field.options || field.options.length === 0) { | ||
| return { valid: false, reason: 'Enum field has no options defined' }; | ||
| } | ||
| if (!field.options.some(opt => opt.value === value)) { | ||
| return { valid: false, reason: `Value "${value}" is not a valid option` }; | ||
| } | ||
| return { valid: true }; | ||
|
|
||
| case 'range': | ||
| if (typeof value !== 'number') { | ||
| return { valid: false, reason: `Expected number, got ${typeof value}` }; | ||
| } | ||
| if (field.min !== undefined && value < field.min) { | ||
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | ||
| } | ||
| if (field.max !== undefined && value > field.max) { | ||
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | ||
| } | ||
| return { valid: true }; |
There was a problem hiding this comment.
Tighten numeric validation before publishing this API.
The proposed checks accept NaN/Infinity, and the range branch ignores step. That means callers can still persist values the schema cannot represent correctly. Please validate finiteness for both number and range, and enforce the declared step for range fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-11-phase2-completion-implementation.md` around lines 220 -
251, The numeric branches in the validator (the switch cases 'number' and
'range') must reject non-finite values and the 'range' branch must also enforce
the declared step; update the 'number' case to call Number.isFinite(value)
(return invalid with a clear reason if not finite) in addition to the existing
typeof check, and update the 'range' case to similarly require
Number.isFinite(value) and then, if field.step is provided, verify that (value -
(field.min ?? 0)) is an integer multiple of field.step within a small epsilon
tolerance (reject if not), while keeping the existing min/max checks and error
messages; refer to the switch cases 'number' and 'range' and the field
properties field.min, field.max, and field.step when making the changes.
Comment on lines
+305
to
+323
| const handleConfigChange = useCallback(async (pluginId: string, key: string, value: unknown) => { | ||
| // Find the schema field for validation | ||
| const schema = BUILT_IN_CONFIG_SCHEMAS[pluginId] ?? plugins.find(p => p.id === pluginId)?.configSchema; | ||
| const field = schema?.[key]; | ||
|
|
||
| if (field) { | ||
| const result = validateConfigValue(field, value); | ||
| if (!result.valid) { | ||
| console.warn(`[plugin:${pluginId}] Invalid config value for "${key}": ${result.reason}`); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| await window.readied.pluginConfig.set(pluginId, key, value); | ||
| setConfigValues(prev => ({ | ||
| ...prev, | ||
| [pluginId]: { ...prev[pluginId], [key]: value }, | ||
| })); | ||
| }, [plugins]); |
There was a problem hiding this comment.
Don’t silently discard invalid edits in the settings UI.
Returning after console.warn() makes the save fail invisibly for the user. The caller should get a validation result it can surface inline/toast, otherwise the UI path for invalid plugin config changes is confusing and looks broken.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-11-phase2-completion-implementation.md` around lines 305 -
323, handleConfigChange currently aborts silently on invalid input after calling
validateConfigValue, causing the UI save to appear to fail; change
handleConfigChange to return the validation result instead of just doing
console.warn + return so the caller can show inline/toast feedback.
Specifically, in the handleConfigChange callback (and where validateConfigValue
is referenced), when result.valid is false return or resolve an object like {
valid: false, reason: result.reason } (or throw a specific ValidationError)
instead of just returning void, and only call window.readied.pluginConfig.set
and setConfigValues when result.valid is true; ensure the function signature and
any callers can handle the returned validation result.
Comment on lines
+487
to
+503
| <button | ||
| type="button" | ||
| className={styles.inspectorToggle} | ||
| onClick={() => setOpen(prev => !prev)} | ||
| > | ||
| <ChevronDown | ||
| size={14} | ||
| style={{ transform: open ? 'rotate(0deg)' : 'rotate(-90deg)', transition: 'transform 0.15s' }} | ||
| /> | ||
| <span>Developer</span> | ||
| {errors.length > 0 && ( | ||
| <span className={styles.inspectorErrorBadge}> | ||
| <AlertTriangle size={12} /> | ||
| {errors.length} | ||
| </span> | ||
| )} | ||
| </button> |
There was a problem hiding this comment.
Add disclosure semantics to the inspector toggle.
This button controls expandable content, but it does not expose its state to assistive tech. Please add aria-expanded and wire aria-controls to the content container so the dev panel remains navigable with screen readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-11-phase2-completion-implementation.md` around lines 487 -
503, The inspector toggle button lacks disclosure semantics—update the button
(the element using className={styles.inspectorToggle} and the open state
controlled by open and setOpen) to include aria-expanded={open} and
aria-controls pointing to the expandable content container's id; add a stable id
(e.g., generate a unique string or use a constant) to the developer panel
container (the element that renders the expandable content and currently shows
errors via styles.inspectorErrorBadge) and ensure the button's aria-controls
value matches that id so screen readers can map the toggle to the panel.
Comment on lines
+106
to
+137
| case 'number': | ||
| if (typeof value !== 'number') { | ||
| return { valid: false, reason: `Expected number, got ${typeof value}` }; | ||
| } | ||
| if (field.min !== undefined && value < field.min) { | ||
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | ||
| } | ||
| if (field.max !== undefined && value > field.max) { | ||
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | ||
| } | ||
| return { valid: true }; | ||
|
|
||
| case 'enum': | ||
| if (!field.options || field.options.length === 0) { | ||
| return { valid: false, reason: 'Enum field has no options defined' }; | ||
| } | ||
| if (!field.options.some((opt) => opt.value === value)) { | ||
| return { valid: false, reason: `Value "${String(value)}" is not a valid option` }; | ||
| } | ||
| return { valid: true }; | ||
|
|
||
| case 'range': | ||
| if (typeof value !== 'number') { | ||
| return { valid: false, reason: `Expected number for range, got ${typeof value}` }; | ||
| } | ||
| if (field.min !== undefined && value < field.min) { | ||
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | ||
| } | ||
| if (field.max !== undefined && value > field.max) { | ||
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | ||
| } | ||
| return { valid: true }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "validation.ts" | head -20Repository: tomymaritano/readide
Length of output: 103
🏁 Script executed:
cat -n packages/plugin-api/src/validation.ts | head -150Repository: tomymaritano/readide
Length of output: 5907
🏁 Script executed:
# Find schema type definitions to understand field structure
fd -t f -e ts -e d.ts | xargs grep -l "min.*max.*step" | head -10Repository: tomymaritano/readide
Length of output: 46
🏁 Script executed:
# Search for range and number field definitions
rg "type.*range|type.*number" packages/plugin-api/src/ -A 3 -B 1Repository: tomymaritano/readide
Length of output: 1224
🏁 Script executed:
cat -n packages/plugin-api/src/types.tsRepository: tomymaritano/readide
Length of output: 6117
🏁 Script executed:
# Search for step field definition in types
rg "step" packages/plugin-api/src/types.ts -B 2 -A 2Repository: tomymaritano/readide
Length of output: 182
🏁 Script executed:
# Check if step is used or validated anywhere in the codebase
rg "\.step" packages/plugin-api/src/ -B 2 -A 2Repository: tomymaritano/readide
Length of output: 46
🏁 Script executed:
# Look for any range validation or step-related logic elsewhere
rg "step|range" packages/plugin-api/src/ --type ts -B 1 -A 1Repository: tomymaritano/readide
Length of output: 1776
🏁 Script executed:
# Test JavaScript behavior with NaN and Infinity
node << 'EOF'
// Verify JavaScript number semantics
console.log("typeof NaN:", typeof NaN);
console.log("typeof Infinity:", typeof Infinity);
console.log("typeof -Infinity:", typeof -Infinity);
console.log("NaN < 10:", NaN < 10);
console.log("NaN > 5:", NaN > 5);
console.log("Infinity < 100:", Infinity < 100);
console.log("Number.isFinite(NaN):", Number.isFinite(NaN));
console.log("Number.isFinite(Infinity):", Number.isFinite(Infinity));
console.log("Number.isFinite(5):", Number.isFinite(5));
EOFRepository: tomymaritano/readide
Length of output: 270
Add finite number validation for number and range types.
In JavaScript, NaN, Infinity, and -Infinity all satisfy typeof value === 'number'. Additionally, NaN fails all relational comparisons (<, >), so it bypasses the min/max checks entirely. The validator must reject non-finite values using Number.isFinite().
Note: The step field is defined in PluginConfigSchemaField but not validated. Consider whether step-based validation should also be enforced.
Suggested fix
case 'number':
- if (typeof value !== 'number') {
- return { valid: false, reason: `Expected number, got ${typeof value}` };
+ if (typeof value !== 'number' || !Number.isFinite(value)) {
+ return { valid: false, reason: `Expected finite number, got ${String(value)}` };
}
if (field.min !== undefined && value < field.min) {
return { valid: false, reason: `Value ${value} is below minimum ${field.min}` };
}
if (field.max !== undefined && value > field.max) {
return { valid: false, reason: `Value ${value} is above maximum ${field.max}` };
}
return { valid: true };
case 'range':
- if (typeof value !== 'number') {
- return { valid: false, reason: `Expected number for range, got ${typeof value}` };
+ if (typeof value !== 'number' || !Number.isFinite(value)) {
+ return { valid: false, reason: `Expected finite number for range, got ${String(value)}` };
}
if (field.min !== undefined && value < field.min) {
return { valid: false, reason: `Value ${value} is below minimum ${field.min}` };
}
if (field.max !== undefined && value > field.max) {
return { valid: false, reason: `Value ${value} is above maximum ${field.max}` };
}
return { valid: true };📝 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.
Suggested change
| case 'number': | |
| if (typeof value !== 'number') { | |
| return { valid: false, reason: `Expected number, got ${typeof value}` }; | |
| } | |
| if (field.min !== undefined && value < field.min) { | |
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | |
| } | |
| if (field.max !== undefined && value > field.max) { | |
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | |
| } | |
| return { valid: true }; | |
| case 'enum': | |
| if (!field.options || field.options.length === 0) { | |
| return { valid: false, reason: 'Enum field has no options defined' }; | |
| } | |
| if (!field.options.some((opt) => opt.value === value)) { | |
| return { valid: false, reason: `Value "${String(value)}" is not a valid option` }; | |
| } | |
| return { valid: true }; | |
| case 'range': | |
| if (typeof value !== 'number') { | |
| return { valid: false, reason: `Expected number for range, got ${typeof value}` }; | |
| } | |
| if (field.min !== undefined && value < field.min) { | |
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | |
| } | |
| if (field.max !== undefined && value > field.max) { | |
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | |
| } | |
| return { valid: true }; | |
| case 'number': | |
| if (typeof value !== 'number' || !Number.isFinite(value)) { | |
| return { valid: false, reason: `Expected finite number, got ${String(value)}` }; | |
| } | |
| if (field.min !== undefined && value < field.min) { | |
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | |
| } | |
| if (field.max !== undefined && value > field.max) { | |
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | |
| } | |
| return { valid: true }; | |
| case 'enum': | |
| if (!field.options || field.options.length === 0) { | |
| return { valid: false, reason: 'Enum field has no options defined' }; | |
| } | |
| if (!field.options.some((opt) => opt.value === value)) { | |
| return { valid: false, reason: `Value "${String(value)}" is not a valid option` }; | |
| } | |
| return { valid: true }; | |
| case 'range': | |
| if (typeof value !== 'number' || !Number.isFinite(value)) { | |
| return { valid: false, reason: `Expected finite number for range, got ${String(value)}` }; | |
| } | |
| if (field.min !== undefined && value < field.min) { | |
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | |
| } | |
| if (field.max !== undefined && value > field.max) { | |
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | |
| } | |
| return { valid: true }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/plugin-api/src/validation.ts` around lines 106 - 137, The number and
range validators (in the 'case "number"' and 'case "range"' branches of validate
logic) must explicitly reject non-finite numeric values; add a check using
Number.isFinite(value) after the typeof check and return invalid if it fails,
and ensure the error message mentions non-finite values (e.g., NaN/Infinity).
Also review PluginConfigSchemaField.step and decide whether to enforce step
alignment (if so, add validation that (value - (field.min || 0)) % field.step
=== 0 or equivalent); otherwise document that step is not validated.
Comment on lines
+127
to
+137
| case 'range': | ||
| if (typeof value !== 'number') { | ||
| return { valid: false, reason: `Expected number for range, got ${typeof value}` }; | ||
| } | ||
| if (field.min !== undefined && value < field.min) { | ||
| return { valid: false, reason: `Value ${value} is below minimum ${field.min}` }; | ||
| } | ||
| if (field.max !== undefined && value > field.max) { | ||
| return { valid: false, reason: `Value ${value} is above maximum ${field.max}` }; | ||
| } | ||
| return { valid: true }; |
There was a problem hiding this comment.
range.step is currently ignored.
A schema like { min: 0, max: 10, step: 5 } still accepts 3, so callers can persist values the slider control would never generate. That weakens the new “validate before persist” guarantee for range fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/plugin-api/src/validation.ts` around lines 127 - 137, The range
validator currently ignores field.step; update the 'range' case in the
validation logic (in packages/plugin-api/src/validation.ts) to, after min/max
checks, if field.step is a positive number compute base = field.min ?? 0 and
require that (value - base) is an integer multiple of step (use a small epsilon
to handle floating-point rounding) and return valid:false with a clear reason if
it is not; also guard against non-positive step values by treating them as "no
step" or returning an explicit validation error depending on your schema rules.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Import canonical PluginConfigSchemaField from plugin-api instead of redeclaring - Add aria-expanded/aria-controls to Plugin Inspector toggle for accessibility - Treat missing config schema fields as invalid (reject unknown keys) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pletion # Conflicts: # packages/storage-sqlite/src/repositories/SQLiteNoteRepository.ts
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
enumandrangetypes toPluginConfigSchemaFieldin pluginScanner.ts, aligning with the canonical plugin-api typevalidateConfigValue()function in plugin-api with 15 tests covering all 5 field types (boolean, string, number, enum, range) with bounds checking. Wired into PluginsSection to reject invalid values before persistingimport.meta.env.DEVis truepluginRuntimeStorenow tracksperformance.now()timing for each plugin loadTest plan
pnpm --filter @readied/plugin-api test— 168 tests pass (15 new validation tests)pnpm --filter @readied/desktop typecheck— all 3 phases clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation