fix(voice-design): validator-safe instruct builder (plan-05, closes #114 #115) - #141
Conversation
plan-05 (option A — frontend guard). The engine validator is whitelist-strict by design; the #114/#115 failures came from useTTS.js merging the free-text instruct field with the category dropdowns, producing unsupported items (#115) or two items in one category (#114). - voiceInstruct.js buildDesignInstruct(vdStates, freeText): dropdowns win their category; free-text accepted only as a known tag in an open category; unknown/duplicate items are dropped and returned so the UI can warn. Derives TAG_TO_CATEGORY from CATEGORIES (single source of truth). - useTTS.js design mode uses it instead of the raw merge; toasts dropped items. Engine validator (_resolve_instruct) untouched — whitelist contract preserved, no vendored-engine change. Tests (TDD, vitest): voiceInstruct.test.js (6). Full frontend suite 72 passed; typecheck + build green. Closes #114, #115. Addresses #132. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a frontend whitelist-safe builder ChangesVoice Design Validator Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
| Filename | Overview |
|---|---|
| frontend/src/utils/voiceInstruct.js | New validator-safe instruct builder: derives TAG_TO_CATEGORY from CATEGORIES at module load, enforces one tag per category with dropdowns winning, and cleanly buckets free-text into unsupported vs duplicates. Logic is correct and edge cases (null/empty, casing, full-width comma, unknown dropdown drift) are all handled. |
| frontend/src/hooks/useTTS.js | Design-mode instruct assembly replaced with buildDesignInstruct; separate toasts for unsupported vs duplicate items address the misleading-message concern from the prior review. vdStates correctly included in useCallback dependency array. |
| frontend/src/utils/voiceInstruct.test.js | 7 vitest tests covering the full contract: one-per-category from dropdowns, free-text prose → unsupported, dropdown outranks typed tag → duplicates, valid free-text in open category, casing/full-width-comma normalization, Auto/empty, and CATEGORIES↔dropdown drift. Good coverage of all the root-cause scenarios. |
Sequence Diagram
sequenceDiagram
participant UI as User (Design Mode)
participant Hook as useTTS.js
participant Builder as buildDesignInstruct()
participant Toast as react-hot-toast
participant API as generateSpeech()
participant Engine as Engine Validator<br/>(_resolve_instruct)
UI->>Hook: handleGenerate()
Hook->>Builder: buildDesignInstruct(vdStates, instruct)
Note over Builder: 1. Dropdowns win their category<br/>2. Free-text: valid+open → accept<br/> unknown → unsupported[]<br/> valid+taken → duplicates[]
Builder-->>Hook: "{ instruct, unsupported[], duplicates[] }"
alt unsupported not empty
Hook->>Toast: Ignored unsupported instruct: ...
end
alt duplicates not empty
Hook->>Toast: Ignored (category already set): ...
end
Hook->>API: "POST formData (instruct = validated string)"
API->>Engine: "instruct = one valid tag per category"
Engine-->>API: valid
API-->>Hook: audio response
Hook-->>UI: plays audio + loads history
Reviews (2): Last reviewed commit: "fix(voice-design): split unsupported vs ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (dropped.length) { | ||
| toast(`Ignored instruct not in the supported set: ${dropped.join(', ')}`, { icon: '⚠️' }); | ||
| } |
There was a problem hiding this comment.
dropped conflates two distinct cases: (1) genuinely unsupported prose, and (2) a recognized tag whose category was already claimed by a dropdown. The shared toast message "not in the supported set" is factually wrong for case 2 — high pitch is in the supported set; it was simply outranked by the dropdown's low pitch. A user seeing this toast may incorrectly conclude the tag itself is invalid and start troubleshooting the wrong thing.
Consider splitting the return value of buildDesignInstruct (or the toast call) into two arrays — one for truly unknown items and one for category-duplicate items — so each gets an accurate message.
There was a problem hiding this comment.
Fixed — split the return into unsupported (free-text prose) and duplicates (valid tag outranked by a dropdown), and the UI now shows an accurate toast per bucket. A valid-but-outranked tag like high pitch no longer reads as "unsupported."
| // Dropdowns first — they win their category (not tracked as "dropped"). | ||
| for (const v of Object.values(vdStates || {})) add(v, null); |
There was a problem hiding this comment.
Dropdown values are passed as add(v, null), so if any dropdown option is not found in TAG_TO_CATEGORY (e.g., if CATEGORIES and the actual dropdown options drift apart), the value is quietly dropped — no toast, no console warning, no entry in dropped. The resulting finalInstruct will just be missing that category with no indication to the user or developer. Adding at least a console.warn for the null-dropped case would make silent desync easier to catch in development.
There was a problem hiding this comment.
Fixed — an unknown dropdown value (CATEGORIES↔option-list drift) now triggers a console.warn instead of being silently dropped, so the desync is visible in dev. Added a test asserting it's excluded from the instruct and not mislabeled as unsupported free-text.
…ropdown drift (Greptile #141) - buildDesignInstruct now returns { instruct, unsupported, duplicates }: `unsupported` = free-text prose (not a known tag, #115); `duplicates` = a valid tag whose category was already set (e.g. dropdown low pitch outranks a typed high pitch, #114). useTTS shows an accurate toast per bucket instead of calling a valid-but-outranked tag "unsupported". - console.warn when a *dropdown* value isn't in CATEGORIES (option-list ↔ whitelist drift) instead of silently dropping it. Tests updated + 1 added (7/7); typecheck + build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bring the branch up to date with main and resolve 5 conflicts as feature-unions so nothing shipped since #133 was opened regresses: - useTTS.js: take main's #141 validator-safe instruct (buildDesignInstruct); #133 held only the stale pre-#141 dedup logic. - dub_pipeline.py: UNION — keep #133's download-task cancel cleanup AND main's plan-04 logging + structured failure event (build_failure). - dubSlice.ts / useDubWorkflow.js: UNION — keep both #133's dub download-progress state (setDubPrepProgress / setDubCurrentSegId) and main's pipeline-error-transparency state (setDubFailure). - bootstrap.rs: take main's shipped plan-03 network-resilience cascade (#140/#142); #133's region-based mirror approach was the superseded alternative for the same concern. get_effective_region stays live (shared via config.rs, used by tools.rs). Verified: frontend typecheck + build clean; 90 backend tests pass (dub / failure / timing / onboarding / personalities), 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
plan-05 — Closes #114, #115. Addresses #132.
Voice Design Synthesize failed with "Unsupported instruct items" (#115) or "conflicting instruct items within the same category" (#114).
Root cause (investigated, not guessed)
The engine validator (
omnivoice/models/omnivoice.py::_resolve_instruct) is whitelist-strict by design — it maps tags to canonical form the engine consumes, so free-text can't be passed through. The presets already emit valid tags. The failures came from the frontend assembly inuseTTS.js: it merged the free-text instruct field with the category dropdowns —Fix — option A (frontend guard, your call)
frontend/src/utils/voiceInstruct.jsbuildDesignInstruct(vdStates, freeText): dropdowns win their category; free-text accepted only as a known tag in an open category; unknown/duplicate items are dropped and returned so the UI can warn.TAG_TO_CATEGORYis derived fromCATEGORIES(single source of truth).useTTS.jsdesign mode uses it instead of the raw merge; toasts the dropped items so the user sees what was ignored.Engine validator untouched — the whitelist contract is preserved, no vendored-engine change, no audio-quality risk.
Tests (TDD, vitest — Constitution V)
voiceInstruct.test.js(6): one-per-category, prose dropped (#115), category-duplicate dropped (#114), valid free-text accepted, casing/full-width-comma, Auto/empty. Full frontend suite: 72 passed; typecheck + build green.Notes
Spec/plan/tasks in
specs/005-voice-design-validator/.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation