Add JSON preset import with validation and UI#29
Conversation
📝 WalkthroughWalkthroughAdded JSON preset file import functionality to a web audio application. Users can now select a Changes
Sequence DiagramsequenceDiagram
participant User
participant FileInput as File Input
participant Parser as JSON Parser
participant Validator as Parameter Validator
participant ConfirmDialog as Confirmation Dialog
participant FormCtrl as Form Controller
participant API as Audio API
participant Output as Output Renderer
User->>FileInput: Select .json file
FileInput->>Parser: Read & parse JSON
Parser->>Validator: Validate parameters
alt Validation Failed
Validator-->>User: Show error status
else Validation Passed
alt Overwrite Confirmation Needed
Validator->>ConfirmDialog: Prompt user
ConfirmDialog-->>User: Confirm action
User-->>ConfirmDialog: Accept/Reject
end
alt User Accepted
Validator->>FormCtrl: Apply normalized values
FormCtrl->>FormCtrl: Update form controls
opt API Present
FormCtrl->>API: applyParams()
end
FormCtrl->>Output: Refresh state
Output->>Output: renderOutput()
Output->>Output: updateTapTempoOut()
Output->>Output: refreshLoopPoints()
Output-->>User: Show success status
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84374b90a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| orbit: (v) => expectNumberRange(v, 0, 1), | ||
| offsetSamples: (v) => expectNumberRange(v, -500, 500), | ||
| tempoBpm: (v) => expectNumberRange(v, TAP_MIN_BPM, TAP_MAX_BPM), | ||
| noteDivision: (v) => expectNumber(v), |
There was a problem hiding this comment.
noteDivision is currently accepted as any finite number, but this control is a <select> with a fixed value set (0.25, 0.5, 0.75, 1, 1.5, 2, 3). When a JSON preset includes another numeric value (for example 1.25), normalizePresetJson accepts it, applyNormalizedPresetValues assigns it to the select, and the browser leaves the select with no matching option (value === ''), so setParam sends Number('') (0) instead of the imported division. This causes presets to be applied incorrectly while still reporting success.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to import custom presets from JSON files. It adds a new file input and button to the UI, along with comprehensive validation logic for preset parameters and a mechanism to apply these values to the DSP. A review comment suggests tightening the validation for the noteDivision parameter to ensure it only accepts values corresponding to the available UI options.
| orbit: (v) => expectNumberRange(v, 0, 1), | ||
| offsetSamples: (v) => expectNumberRange(v, -500, 500), | ||
| tempoBpm: (v) => expectNumberRange(v, TAP_MIN_BPM, TAP_MAX_BPM), | ||
| noteDivision: (v) => expectNumber(v), |
There was a problem hiding this comment.
The noteDivision parameter should be validated against the specific set of allowed values defined in the UI (e.g., 0.25, 0.5, 0.75, 1, 1.5, 2, 3). Using expectNumber is too permissive and could allow values that don't match any <option> in the select element, leading to an inconsistent UI state or invalid values being sent to the DSP.
| noteDivision: (v) => expectNumber(v), | |
| noteDivision: (v) => expectOneOf(v, [0.25, 0.5, 0.75, 1, 1.5, 2, 3]), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
targets/wasm/js/demo.js (1)
960-962: Prevent concurrent preset imports from repeated button clicks.Lines 960-962 can trigger multiple overlapping async imports. Disable/re-enable the button around
handlePresetJsonImport()to avoid racey status/UI updates.🛡️ Suggested fix
-els.applyPresetJsonBtn?.addEventListener('click', () => { - handlePresetJsonImport(); -}); +els.applyPresetJsonBtn?.addEventListener('click', async () => { + if (!els.applyPresetJsonBtn || els.applyPresetJsonBtn.disabled) return; + els.applyPresetJsonBtn.disabled = true; + try { + await handlePresetJsonImport(); + } finally { + els.applyPresetJsonBtn.disabled = false; + } +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/wasm/js/demo.js` around lines 960 - 962, Wrap the click handler for els.applyPresetJsonBtn so it disables the button immediately on click, awaits handlePresetJsonImport(), and re-enables the button in a finally block to prevent concurrent imports; use a try { await handlePresetJsonImport(); } finally { els.applyPresetJsonBtn.disabled = false; } pattern (set disabled = true before calling) to ensure the button is always restored even on error, referencing els.applyPresetJsonBtn and handlePresetJsonImport.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@targets/wasm/js/demo.js`:
- Line 570: The noteDivision getter currently uses expectNumber(v) and allows
any finite number; change it to validate against the engine/UI's allowed
subdivisions instead: in the noteDivision handler (symbol: noteDivision) check
that the parsed value is one of the supported subdivision constants/array (e.g.,
SUPPORTED_SUBDIVISIONS or DEFAULT_SUBDIVISION) and only accept it if present,
otherwise fallback to a safe default or null so the <select> stays in sync;
replace expectNumber(v) with this whitelist check (or a helper like expectOneOf)
and ensure downstream code still receives the validated value.
- Around line 589-593: The expectNumber helper currently coerces with
Number(value) which accepts non-number JSON (e.g., null, true, ""); change it to
perform strict type checking: verify typeof value === "number" and
Number.isFinite(value) and return the numeric value only in that case, otherwise
return null. Update the expectNumber function to avoid any coercion and rely on
Number.isFinite on the original value so only true numeric JSON values pass
validation.
In `@targets/wasm/js/index.html`:
- Line 245: The paragraph used for preset import feedback (element id
"presetImportStatus") lacks live-region semantics so screen readers won't
reliably announce dynamic updates; update the <p id="presetImportStatus">
element by adding appropriate ARIA attributes such as role="status",
aria-live="polite", and aria-atomic="true" (or equivalent) to ensure
success/error messages are announced to assistive technologies whenever the text
content is changed.
---
Nitpick comments:
In `@targets/wasm/js/demo.js`:
- Around line 960-962: Wrap the click handler for els.applyPresetJsonBtn so it
disables the button immediately on click, awaits handlePresetJsonImport(), and
re-enables the button in a finally block to prevent concurrent imports; use a
try { await handlePresetJsonImport(); } finally {
els.applyPresetJsonBtn.disabled = false; } pattern (set disabled = true before
calling) to ensure the button is always restored even on error, referencing
els.applyPresetJsonBtn and handlePresetJsonImport.
🪄 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: d059008c-c5e4-4f01-9f08-ab661cb3ed7c
📒 Files selected for processing (2)
targets/wasm/js/demo.jstargets/wasm/js/index.html
| orbit: (v) => expectNumberRange(v, 0, 1), | ||
| offsetSamples: (v) => expectNumberRange(v, -500, 500), | ||
| tempoBpm: (v) => expectNumberRange(v, TAP_MIN_BPM, TAP_MAX_BPM), | ||
| noteDivision: (v) => expectNumber(v), |
There was a problem hiding this comment.
Constrain noteDivision to supported subdivision values.
Line 570 accepts any finite number, but the UI/engine only supports a fixed subdivision set. Invalid imported values can desync the <select> state and apply unintended runtime values.
🧭 Suggested fix
- noteDivision: (v) => expectNumber(v),
+ noteDivision: (v) => expectOneOf(v, [0.25, 0.5, 0.75, 1, 1.5, 2, 3]),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/wasm/js/demo.js` at line 570, The noteDivision getter currently uses
expectNumber(v) and allows any finite number; change it to validate against the
engine/UI's allowed subdivisions instead: in the noteDivision handler (symbol:
noteDivision) check that the parsed value is one of the supported subdivision
constants/array (e.g., SUPPORTED_SUBDIVISIONS or DEFAULT_SUBDIVISION) and only
accept it if present, otherwise fallback to a safe default or null so the
<select> stays in sync; replace expectNumber(v) with this whitelist check (or a
helper like expectOneOf) and ensure downstream code still receives the validated
value.
| function expectNumber(value) { | ||
| const n = Number(value); | ||
| if (!Number.isFinite(n)) return null; | ||
| return n; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript (ECMAScript), what are the exact results of Number(null), Number(""), and Number(true), and does Number(value) perform type coercion for non-number inputs?
💡 Result:
Number(null) returns 0, Number("") returns 0, and Number(true) returns 1. Yes, Number(value) performs type coercion for non-number inputs by invoking the ToNumber abstract operation from the ECMAScript specification, which defines specific conversions for different types including null, booleans, and strings.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/Number
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/number
- 4: https://tc39.es/ecma262/pr/3770/multipage/numbers-and-dates.html
- 5: https://tc39.es/ecma262/2026/multipage/abstract-operations.html
- 6: https://stackoverflow.com/questions/7820683/convert-boolean-result-into-number-integer
🏁 Script executed:
find . -name "demo.js" -type fRepository: ovelhaaa/orbit-echo
Length of output: 88
🏁 Script executed:
sed -n '585,600p' ./targets/wasm/js/demo.jsRepository: ovelhaaa/orbit-echo
Length of output: 462
🏁 Script executed:
rg "expectNumber" ./targets/wasm/js/demo.js -A 2 -B 2Repository: ovelhaaa/orbit-echo
Length of output: 1155
🏁 Script executed:
rg "presetValidators" ./targets/wasm/js/demo.js -A 5 -B 5Repository: ovelhaaa/orbit-echo
Length of output: 797
🏁 Script executed:
rg "presetValidators\[key\]" ./targets/wasm/js/demo.js -B 20 | head -40Repository: ovelhaaa/orbit-echo
Length of output: 830
🏁 Script executed:
sed -n '589,593p' ./targets/wasm/js/demo.js | cat -nRepository: ovelhaaa/orbit-echo
Length of output: 209
Enforce strict type checking for JSON numeric values instead of relying on type coercion.
The Number() coercion at line 590 converts non-number JSON types (null, true, "") to valid numbers (0, 1, 0 respectively), allowing type mismatches to pass validation. For example, {"orbit": null} would be accepted as orbit=0 instead of being rejected. This undermines type safety in preset validation.
Suggested fix
function expectNumber(value) {
- const n = Number(value);
- if (!Number.isFinite(n)) return null;
- return n;
+ if (typeof value !== 'number' || !Number.isFinite(value)) return null;
+ return value;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/wasm/js/demo.js` around lines 589 - 593, The expectNumber helper
currently coerces with Number(value) which accepts non-number JSON (e.g., null,
true, ""); change it to perform strict type checking: verify typeof value ===
"number" and Number.isFinite(value) and return the numeric value only in that
case, otherwise return null. Update the expectNumber function to avoid any
coercion and rely on Number.isFinite on the original value so only true numeric
JSON values pass validation.
| <input id="presetJsonFile" type="file" accept=".json,application/json" aria-label="Arquivo de preset JSON" /> | ||
| <button id="applyPresetJsonBtn" type="button">Aplicar preset JSON</button> | ||
| </div> | ||
| <p id="presetImportStatus" class="hint">Carregue um arquivo .json para aplicar parâmetros customizados.</p> |
There was a problem hiding this comment.
Add live-region semantics for preset import feedback.
Line 245 is dynamically updated, but without role="status"/aria-live, screen readers may not announce success/error feedback.
♿ Suggested fix
- <p id="presetImportStatus" class="hint">Carregue um arquivo .json para aplicar parâmetros customizados.</p>
+ <p
+ id="presetImportStatus"
+ class="hint"
+ role="status"
+ aria-live="polite"
+ aria-atomic="true"
+ >
+ Carregue um arquivo .json para aplicar parâmetros customizados.
+ </p>📝 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.
| <p id="presetImportStatus" class="hint">Carregue um arquivo .json para aplicar parâmetros customizados.</p> | |
| <p | |
| id="presetImportStatus" | |
| class="hint" | |
| role="status" | |
| aria-live="polite" | |
| aria-atomic="true" | |
| > | |
| Carregue um arquivo .json para aplicar parâmetros customizados. | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/wasm/js/index.html` at line 245, The paragraph used for preset import
feedback (element id "presetImportStatus") lacks live-region semantics so screen
readers won't reliably announce dynamic updates; update the <p
id="presetImportStatus"> element by adding appropriate ARIA attributes such as
role="status", aria-live="polite", and aria-atomic="true" (or equivalent) to
ensure success/error messages are announced to assistive technologies whenever
the text content is changed.
1 participant
Motivation
Description
PRESET_SCHEMA_VERSIONand centralizedPARAM_KEYSlist and used it inbindRealtimeParamUpdatesto drive realtime updates.targets/wasm/js/demo.jsincludingpresetValidators,normalizePresetJson,hasPresetOverwrite,applyNormalizedPresetValues,readFileAsText, andhandlePresetJsonImportwith localized status messages viasetPresetImportStatus.targets/wasm/js/index.htmlfor selecting a.jsonpreset file and anAplicar preset JSONbutton plus apresetImportStatushint paragraph, and wired the button tohandlePresetJsonImport.applyParams()and UI refreshes when successful.Testing
Codex Task
Summary by CodeRabbit