Fix ElevenLabs voice discovery and TTS pickers#4151
Merged
Conversation
|
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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
SpicyMarinara
marked this pull request as ready for review
July 26, 2026 20:55
coderabbitai
Bot
added
feature
![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 (3)
packages/client/src/components/panels/settings/TTSConfigCard.tsx (2)
171-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider handling object-shaped
detailpayloads.ElevenLabs returns
detailas an object ({ status, message }) — as your own regression mock inscripts/regressions/tts-source-persistence.regression.ts(lines 33-36) demonstrates. If that payload ever reaches the client unflattened, the string check silently discards it and the subject is left with only the generic message. A trivial specimen extension:♻️ Suggested extraction of nested detail
- const detail = typeof payload?.detail === "string" ? payload.detail.trim() : ""; + const rawDetail = payload?.detail; + const detail = + typeof rawDetail === "string" + ? rawDetail.trim() + : rawDetail && typeof rawDetail === "object" && typeof (rawDetail as { message?: unknown }).message === "string" + ? (rawDetail as { message: string }).message.trim() + : "";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/components/panels/settings/TTSConfigCard.tsx` around lines 171 - 181, Update getTtsRequestErrorMessage to handle object-shaped payload.detail values, extracting a meaningful nested message such as detail.message when it is a string, while preserving the existing string-detail and fallback behavior. Ensure ApiError messages combine the generic message with the extracted detail when available.
465-486: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThrottle the capture-phase scroll repositioning.
updatePositionis bound toscrollwithcapture: true, so every scroll event anywhere in the document performs a layout read and an unconditionalsetPosition— a fresh object each time, so React re-renders the portal on each frame's worth of events. On a settings panel nested in scrollable containers this is measurable jank. An rAF gate (or an equality bail-out beforesetPosition) suffices.⚡ Suggested rAF-gated scheduler
updatePosition(); const frame = window.requestAnimationFrame(updatePosition); - window.addEventListener("resize", updatePosition); - window.addEventListener("scroll", updatePosition, true); + let scheduled = 0; + const schedulePosition = () => { + if (scheduled) return; + scheduled = window.requestAnimationFrame(() => { + scheduled = 0; + updatePosition(); + }); + }; + window.addEventListener("resize", schedulePosition); + window.addEventListener("scroll", schedulePosition, true); return () => { window.cancelAnimationFrame(frame); - window.removeEventListener("resize", updatePosition); - window.removeEventListener("scroll", updatePosition, true); + if (scheduled) window.cancelAnimationFrame(scheduled); + window.removeEventListener("resize", schedulePosition); + window.removeEventListener("scroll", schedulePosition, true); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/components/panels/settings/TTSConfigCard.tsx` around lines 465 - 486, Throttle the capture-phase scroll repositioning around updatePosition by scheduling at most one layout update per animation frame, rather than running it for every scroll event. Preserve immediate initial positioning and resize behavior, cancel any pending frame during cleanup, and ensure the scroll listener uses the gated scheduler while updatePosition remains the actual measurement function.scripts/regressions/tts-source-persistence.regression.ts (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions thrown inside the request handler degrade into timeouts.
When
assert.equalthrows here, no response is ever written. The client then waits outAbortSignal.timeout(10_000)infetchElevenLabsVoiceOptions, and becausefetchAllElevenLabsVoiceOptionswraps both calls inPromise.allSettled, the failure is swallowed into a partial result. A precise diagnostic becomes a ten-second riddle. Prefer answering with a 500 carrying the mismatch and asserting on the client side.🧪 Suggested handler-safe failure reporting
- assert.equal(apiKey, "test-elevenlabs-key"); - assert.equal(url.pathname, "/v2/voices"); - assert.equal(url.searchParams.get("page_size"), "100"); + if (apiKey !== "test-elevenlabs-key" || url.pathname !== "/v2/voices" || url.searchParams.get("page_size") !== "100") { + response.writeHead(500, { "Content-Type": "application/json" }); + response.end(JSON.stringify({ detail: `unexpected request: ${apiKey} ${url.pathname}${url.search}` })); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/regressions/tts-source-persistence.regression.ts` around lines 48 - 50, Update the request handler in the regression test to catch validation failures and send an HTTP 500 response containing the mismatch details instead of allowing assertions to escape and leave the request unresolved. Move the corresponding assertions to the client-side test flow around fetchAllElevenLabsVoiceOptions or fetchElevenLabsVoiceOptions, preserving validation of apiKey, pathname, and page_size while ensuring failures surface immediately rather than being swallowed by Promise.allSettled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/core-flows.e2e.ts`:
- Line 4200: Make disposal of “ElevenLabs Voice Picker Character” unconditional
in the test flow surrounding the assertions before the existing
page.request.delete call. Move the deletion into a finally block or equivalent
afterEach cleanup so it runs whether assertions pass or throw, while preserving
the current character identifier and delete request.
In `@packages/client/src/components/panels/settings/TTSConfigCard.tsx`:
- Around line 546-601: Implement focus restoration for the settings selector by
attaching triggerRef to the trigger button and using closePanel() in the Escape
handler and both option onClick handlers, so closing or selecting returns focus
to the trigger. Use closePanel(false) in the outside-pointer handler to preserve
its existing non-focus-restoring behavior; defer arrow-key and Enter navigation
changes.
In `@packages/server/src/routes/tts.routes.ts`:
- Around line 717-733: Update fetchAllElevenLabsVoiceOptions so that when both
provider requests reject, the thrown error preserves every collected message
instead of only errors[0]. Join the errors with "; " or use an AggregateError,
while retaining the existing fallback message when no error details are
available.
---
Nitpick comments:
In `@packages/client/src/components/panels/settings/TTSConfigCard.tsx`:
- Around line 171-181: Update getTtsRequestErrorMessage to handle object-shaped
payload.detail values, extracting a meaningful nested message such as
detail.message when it is a string, while preserving the existing string-detail
and fallback behavior. Ensure ApiError messages combine the generic message with
the extracted detail when available.
- Around line 465-486: Throttle the capture-phase scroll repositioning around
updatePosition by scheduling at most one layout update per animation frame,
rather than running it for every scroll event. Preserve immediate initial
positioning and resize behavior, cancel any pending frame during cleanup, and
ensure the scroll listener uses the gated scheduler while updatePosition remains
the actual measurement function.
In `@scripts/regressions/tts-source-persistence.regression.ts`:
- Around line 48-50: Update the request handler in the regression test to catch
validation failures and send an HTTP 500 response containing the mismatch
details instead of allowing assertions to escape and leave the request
unresolved. Move the corresponding assertions to the client-side test flow
around fetchAllElevenLabsVoiceOptions or fetchElevenLabsVoiceOptions, preserving
validation of apiKey, pathname, and page_size while ensuring failures surface
immediately rather than being swallowed by Promise.allSettled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1306d37e-830b-482b-af35-3df65744e9d7
📒 Files selected for processing (5)
e2e/core-flows.e2e.tspackages/client/src/components/panels/settings/TTSConfigCard.tsxpackages/client/src/localization/locales/en.jsonpackages/server/src/routes/tts.routes.tsscripts/regressions/tts-source-persistence.regression.ts
| await refreshButton.click(); | ||
| await expect.poll(() => saveCount).toBeGreaterThan(0); | ||
| await expect.poll(() => voiceFetchCount).toBeGreaterThan(fetchesBeforeRefresh); | ||
| await page.request.delete(`/api/characters/${character.id}`); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The specimen is only disposed of on a successful trial.
Every assertion between line 4108 and 4199 can throw, and each such throw abandons "ElevenLabs Voice Picker Character" in the database. Subsequent tests that enumerate characters — and reruns of this very test — then operate on a contaminated sample. Bind the disposal to a finally or an afterEach hook so cleanup is unconditional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/core-flows.e2e.ts` at line 4200, Make disposal of “ElevenLabs Voice
Picker Character” unconditional in the test flow surrounding the assertions
before the existing page.request.delete call. Move the deletion into a finally
block or equivalent afterEach cleanup so it runs whether assertions pass or
throw, while preserving the current character identifier and delete request.
Comment on lines
+546
to
+601
| <div | ||
| id={listboxId} | ||
| role="listbox" | ||
| aria-label={ariaLabel} | ||
| data-testid={testId} | ||
| className="min-h-0 overflow-x-hidden overflow-y-scroll pr-1 [scrollbar-color:var(--primary)_var(--secondary)] [scrollbar-gutter:stable] [&::-webkit-scrollbar-thumb]:rounded-full [&::-webkit-scrollbar-thumb]:bg-[var(--primary)] [&::-webkit-scrollbar-track]:bg-[var(--secondary)] [&::-webkit-scrollbar]:w-2" | ||
| > | ||
| <span className="min-w-0 flex-1 truncate">{formatVoiceOptionLabel(option)}</span> | ||
| {option.id === value && <Check size="0.75rem" className="shrink-0" />} | ||
| </button> | ||
| ))} | ||
| {filteredOptions.length === 0 && ( | ||
| <p className="px-2.5 py-3 text-center text-xs text-[var(--muted-foreground)]"> | ||
| {localizeUi("ui.panels.ttsconfigcard.noMatchingVoices")} | ||
| </p> | ||
| )} | ||
| </div> | ||
| </div> | ||
| )} | ||
| <button | ||
| type="button" | ||
| role="option" | ||
| aria-selected={!value} | ||
| onClick={() => { | ||
| onChange(""); | ||
| setOpen(false); | ||
| setSearch(""); | ||
| }} | ||
| className={cn( | ||
| "flex w-full min-w-0 items-center gap-2 rounded-lg px-2.5 py-2 text-left text-xs hover:bg-[var(--secondary)]", | ||
| !value && "bg-[var(--primary)]/10 text-[var(--primary)]", | ||
| )} | ||
| > | ||
| <span className="flex h-6 w-6 shrink-0 items-center justify-center rounded-md bg-[var(--secondary)] text-[var(--primary)]"> | ||
| {optionKind === "character" ? <UserRound size="0.75rem" /> : <Volume2 size="0.75rem" />} | ||
| </span> | ||
| <span className="min-w-0 flex-1 truncate">{placeholder}</span> | ||
| {!value && <Check size="0.75rem" className="shrink-0" />} | ||
| </button> | ||
| {filteredOptions.map((option) => ( | ||
| <button | ||
| key={option.id} | ||
| type="button" | ||
| role="option" | ||
| aria-selected={option.id === value} | ||
| aria-disabled={option.disabled || undefined} | ||
| disabled={option.disabled} | ||
| title={option.label} | ||
| onClick={() => { | ||
| onChange(option.id); | ||
| setOpen(false); | ||
| setSearch(""); | ||
| }} | ||
| className={cn( | ||
| "flex w-full min-w-0 items-center gap-2 rounded-lg px-2.5 py-2 text-left text-xs hover:bg-[var(--secondary)] disabled:cursor-not-allowed disabled:opacity-40", | ||
| option.id === value && "bg-[var(--primary)]/10 text-[var(--primary)]", | ||
| )} | ||
| > | ||
| <span className="flex h-6 w-6 shrink-0 items-center justify-center rounded-md bg-[var(--secondary)] text-[var(--primary)]"> | ||
| {optionKind === "character" ? <UserRound size="0.75rem" /> : <Volume2 size="0.75rem" />} | ||
| </span> | ||
| <span className="min-w-0 flex-1 truncate">{option.label}</span> | ||
| {option.id === value && <Check size="0.75rem" className="shrink-0" />} | ||
| </button> | ||
| ))} | ||
| {filteredOptions.length === 0 && ( | ||
| <p className="px-2.5 py-3 text-center text-xs text-[var(--muted-foreground)]">{emptyText}</p> | ||
| )} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keyboard subjects are left stranded: no focus return, no arrow navigation.
Two deficiencies in the specimen's nervous system:
- On Escape (line 431-434) and on selection, focus is never returned to the trigger. Since the panel lives in a
document.bodyportal, the vanished focus falls back to<body>and the tab order restarts at the document head — a jarring regression versus the native<select>this replaces. - The listbox offers no ArrowUp/ArrowDown/Enter traversal;
role="listbox"advertises a keyboard contract the implementation does not honor.
The first is trivial to remedy and worth doing now; the second can be deferred.
♿ Minimal focus restoration
+ const triggerRef = useRef<HTMLButtonElement>(null);
+ const closePanel = (restoreFocus = true) => {
+ setOpen(false);
+ setSearch("");
+ if (restoreFocus) triggerRef.current?.focus();
+ };Then use closePanel() in the Escape handler and in the option onClick handlers, closePanel(false) for the outside-pointer handler, and attach ref={triggerRef} to the trigger button.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client/src/components/panels/settings/TTSConfigCard.tsx` around
lines 546 - 601, Implement focus restoration for the settings selector by
attaching triggerRef to the trigger button and using closePanel() in the Escape
handler and both option onClick handlers, so closing or selecting returns focus
to the trigger. Use closePanel(false) in the outside-pointer handler to preserve
its existing non-focus-restoring behavior; defer arrow-key and Enter navigation
changes.
Comment on lines
+717
to
+733
| export async function fetchAllElevenLabsVoiceOptions(baseUrl: string, apiKey: string): Promise<VoiceOption[]> { | ||
| const results = await Promise.allSettled([ | ||
| fetchElevenLabsVoiceOptions(baseUrl, apiKey), | ||
| fetchElevenLabsVoiceOptions(baseUrl, apiKey, { voice_type: "saved" }), | ||
| ]); | ||
| const successfulResults = results.filter( | ||
| (result): result is PromiseFulfilledResult<VoiceOption[]> => result.status === "fulfilled", | ||
| ); | ||
| if (successfulResults.length === 0) { | ||
| const errors = results | ||
| .filter((result): result is PromiseRejectedResult => result.status === "rejected") | ||
| .map((result) => (result.reason instanceof Error ? result.reason.message : String(result.reason))); | ||
| throw new Error(errors[0] || "ElevenLabs voice discovery failed"); | ||
| } | ||
| return mergeVoiceOptions(successfulResults.flatMap((result) => result.value)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not discard the second provider failure.
When both requests reject, errors contains both messages but errors[0] is thrown. The resulting 502 hides useful provider diagnostics, especially for the saved-voice query. Preserve all messages with errors.join("; ") or an AggregateError.
Suggested fix
- throw new Error(errors[0] || "ElevenLabs voice discovery failed");
+ throw new Error(
+ errors.length > 0
+ ? `ElevenLabs voice discovery failed: ${errors.join("; ")}`
+ : "ElevenLabs voice discovery failed",
+ );📝 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
| export async function fetchAllElevenLabsVoiceOptions(baseUrl: string, apiKey: string): Promise<VoiceOption[]> { | |
| const results = await Promise.allSettled([ | |
| fetchElevenLabsVoiceOptions(baseUrl, apiKey), | |
| fetchElevenLabsVoiceOptions(baseUrl, apiKey, { voice_type: "saved" }), | |
| ]); | |
| const successfulResults = results.filter( | |
| (result): result is PromiseFulfilledResult<VoiceOption[]> => result.status === "fulfilled", | |
| ); | |
| if (successfulResults.length === 0) { | |
| const errors = results | |
| .filter((result): result is PromiseRejectedResult => result.status === "rejected") | |
| .map((result) => (result.reason instanceof Error ? result.reason.message : String(result.reason))); | |
| throw new Error(errors[0] || "ElevenLabs voice discovery failed"); | |
| } | |
| return mergeVoiceOptions(successfulResults.flatMap((result) => result.value)); | |
| } | |
| export async function fetchAllElevenLabsVoiceOptions(baseUrl: string, apiKey: string): Promise<VoiceOption[]> { | |
| const results = await Promise.allSettled([ | |
| fetchElevenLabsVoiceOptions(baseUrl, apiKey), | |
| fetchElevenLabsVoiceOptions(baseUrl, apiKey, { voice_type: "saved" }), | |
| ]); | |
| const successfulResults = results.filter( | |
| (result): result is PromiseFulfilledResult<VoiceOption[]> => result.status === "fulfilled", | |
| ); | |
| if (successfulResults.length === 0) { | |
| const errors = results | |
| .filter((result): result is PromiseRejectedResult => result.status === "rejected") | |
| .map((result) => (result.reason instanceof Error ? result.reason.message : String(result.reason))); | |
| throw new Error( | |
| errors.length > 0 | |
| ? `ElevenLabs voice discovery failed: ${errors.join("; ")}` | |
| : "ElevenLabs voice discovery failed", | |
| ); | |
| } | |
| return mergeVoiceOptions(successfulResults.flatMap((result) => result.value)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/routes/tts.routes.ts` around lines 717 - 733, Update
fetchAllElevenLabsVoiceOptions so that when both provider requests reject, the
thrown error preserves every collected message instead of only errors[0]. Join
the errors with "; " or use an AggregateError, while retaining the existing
fallback message when no error details are available.
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.
Linked issue
Closes #4150
Why this change
ElevenLabs preview failures could expose compressed response bytes as unreadable text. Account voice discovery could also appear to load only defaults, and the per-character assignment controls were inconsistent and cramped for larger libraries.
Root cause
What changed
/v2/voicespage and merge the unfiltered catalog withvoice_type=saved, deduplicating overlapping entries.Automated validation run
pnpm checkpnpm regression:ttspnpm exec playwright test -c playwright.config.ts -g "ElevenLabs keeps models visible" --project=desktop-chromiumgit diff --checkOne unrelated existing lint warning remains in
packages/client/src/components/noodle/NoodleHome.tsxfor a missinglocalizeUihook dependency.Manual verification
Documentation and release impact
No user documentation or release metadata changes are required. The new picker copy is localized through the canonical English catalog, with normal fallback behavior for partial community locales.
Summary by CodeRabbit
New Features
Bug Fixes