PR_26171_BETA_081-message-playback-through-tts-engine#40
Conversation
![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: 2122063eff
ℹ️ 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".
| const DEFAULT_TTS_EMOTION_SETTINGS = Object.freeze([ | ||
| Object.freeze({ active: true, emotion: "calm", emotionLabel: "Calm", pitch: 1, rate: 1, ssmlLikePreset: "normal", volume: 1 }), | ||
| Object.freeze({ active: true, emotion: "urgent", emotionLabel: "Urgent", pitch: 1.08, rate: 1.15, ssmlLikePreset: "normal", volume: 1 }), | ||
| Object.freeze({ active: true, emotion: "whisper", emotionLabel: "Whisper", pitch: 0.95, rate: 0.9, ssmlLikePreset: "normal", volume: 0.55 }), | ||
| Object.freeze({ active: true, emotion: "angry", emotionLabel: "Angry", pitch: 0.98, rate: 1.1, ssmlLikePreset: "normal", volume: 1 }), |
There was a problem hiding this comment.
When there are no active API TTS profiles, activeTtsProfileOptions() falls back to DEFAULT_TTS_PROFILE; however the fallback Emotion Settings added here only cover Calm/Urgent/Whisper/Angry while the seeded active emotions also include Excited, Sad, and Mysterious. In that no-active-profile fallback path, any message or part using one of the omitted seeded emotions now fails in speakTarget() with a missing Emotion Setting error instead of playing with the emotion values as it did before. Include all seeded emotions or derive these fallback settings from state.emotionProfiles.
Useful? React with 👍 / 👎.
| const setting = settings.find((candidate) => ( | ||
| emotionSettingKey(candidate.emotion) === selectedEmotionKey | ||
| || emotionSettingKey(candidate.emotionLabel) === selectedEmotionKey | ||
| )); |
There was a problem hiding this comment.
If two active Emotion Profiles have distinct names that normalize to the same slug, such as High Energy and High-Energy, the API permits both names but this lookup can return the first setting for either profile because it checks the slugified candidate.emotion before the exact label. Playback then applies the wrong pitch/rate/volume for whichever profile appears later in the settings list. Prefer matching by a stable emotion profile key, or at least check the exact emotionLabel before the slug.
Useful? React with 👍 / 👎.
| const emotionSetting = selectedEmotionSettingForProfile(profile, target.emotionProfile); | ||
| if (!emotionSetting.ok) { | ||
| return { message: emotionSetting.message, ok: false }; |
There was a problem hiding this comment.
When a message or part has its own TTS profile selected in the row, selectedSpeechTarget() stores that profile and testSelectedSpeech() passes target.profile to speakTarget(), but this new readiness check validates the separate Inspector dropdown profile instead. With per-profile Emotion Settings, Test Speech can be disabled by an unrelated profile or shown as ready and then fail on click. Check target.profile || selectedTtsProfile() here so readiness validates the same profile that will actually speak.
Useful? React with 👍 / 👎.
| const emotionSetting = selectedEmotionSettingForProfile(profile, target.emotionProfile); | ||
| if (!emotionSetting.ok) { | ||
| return visiblePlaybackError(emotionSetting.message); | ||
| } |
There was a problem hiding this comment.
When Play Message is used on a multi-part message and a later part lacks a matching Emotion Setting, earlier loop iterations have already called speakTarget() and queued audio before this new guard returns the error. That means the user hears a partial message even though playback is reported as blocked for a missing setting. Resolve settings for every active part before calling the TTS registry, or otherwise stop/avoid queuing earlier parts when any part cannot be played.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation