-
Notifications
You must be signed in to change notification settings - Fork 59.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add voice audio preview button in tts-config option 为 TTS 设置选项添加了语音预览按钮 #5651
base: main
Are you sure you want to change the base?
Conversation
@Dakai is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
app/components/tts-config.tsx (4)
Line range hint
69-69
: Avoid assignment within arrow function expressionsAssigning values within an arrow function expression can be confusing and is flagged by static analysis tools. Use a block body instead.
Apply this diff to refactor the code:
- (config) => (config.enable = e.currentTarget.checked), + (config) => { + config.enable = e.currentTarget.checked; + },
Line range hint
94-97
: Avoid assignment within arrow function expressionsFor better readability and to adhere to coding standards, avoid using assignments within arrow function expressions.
Apply this diff:
- (config) => - (config.engine = TTSConfigValidator.engine( - e.currentTarget.value, - )), + (config) => { + config.engine = TTSConfigValidator.engine( + e.currentTarget.value, + ); + },
Line range hint
115-118
: Avoid assignment within arrow function expressionsUsing assignments inside arrow function expressions can be misleading. Use a block body to make the assignment explicit.
Apply this diff:
- (config) => - (config.model = TTSConfigValidator.model( - e.currentTarget.value, - )), + (config) => { + config.model = TTSConfigValidator.model( + e.currentTarget.value, + ); + },
147-148
: Externalize hardcoded preview text for localizationThe hardcoded preview text may not be appropriate for all users or locales. Consider moving it to a localized string to support internationalization.
Apply this diff:
- "NextChat,Unleash your imagination, experience the future of AI conversation.", + Locale.Settings.TTS.PreviewText,And add
PreviewText
to your localization files:// In Locale.Settings.TTS PreviewText: "NextChat, unleash your imagination, experience the future of AI conversation.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/components/tts-config.tsx (2 hunks)
🧰 Additional context used
🪛 Biome
app/components/tts-config.tsx
[error] 157-159: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Your build has completed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/components/tts-config.tsx (1)
Line range hint
22-195
: Consider performance optimizationsThe TTSConfigList component has grown in complexity with the addition of speech playback functionality. While the implementation is sound, consider the following optimizations to improve performance:
- Memoize the IconButton and Select components using React.memo to prevent unnecessary re-renders.
- Extract the openaiSpeech function to a custom hook or a utility function outside the component to improve readability and potentially reduce re-renders.
- Use the useCallback hook for functions passed as props to child components to maintain referential equality across renders.
Here's an example of how you might implement these optimizations:
import React, { useState, useCallback, useMemo } from "react"; // ... other imports ... const MemoizedIconButton = React.memo(IconButton); const MemoizedSelect = React.memo(Select); export function TTSConfigList(props: { ttsConfig: TTSConfig; updateConfig: (updater: (config: TTSConfig) => void) => void; }) { // ... existing state variables ... const openaiSpeech = useCallback(async (text: string) => { // ... existing openaiSpeech implementation ... }, [props.ttsConfig]); const handleVoiceChange = useCallback((e: React.ChangeEvent<HTMLSelectElement>) => { props.updateConfig((config) => { config.voice = TTSConfigValidator.voice(e.currentTarget.value); }); }, [props.updateConfig]); const voiceOptions = useMemo(() => DEFAULT_TTS_VOICES.map((v, i) => ( <option value={v} key={i}>{v}</option> )) , []); // ... rest of the component ... }These optimizations can help improve the component's performance, especially as it grows in complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/components/tts-config.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/components/tts-config.tsx (2)
2-2
: LGTM: New imports and state variables are well-structuredThe new imports and state variables are appropriately added to support the new speech playback functionality. The use of
useState
for managingspeechLoading
andspeechStatus
is a good practice for handling component-level state.Also applies to: 14-19, 26-27
154-169
: LGTM: Improved voice selection UI and handlerThe voice selection UI has been improved by grouping the IconButton and Select component. The onChange handler for the Select component has been updated to address a previous review comment about assignments in arrow functions. These changes enhance both the user experience and code quality.
很实用的功能, 交互没问题 @Dogtiti |
Very practical function, no problem in interaction @Dogtiti |
tts以及stt相关功能可能会暂停一下 |
tts and stt related functions may be suspended for a while |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/components/tts-config.tsx (2)
131-165
: LGTM: Enhanced UI with speech playback controlThe addition of the IconButton for speech playback control and the reorganization of the voice selection dropdown improve the user interface and enhance interaction. The layout changes are well-structured and improve the organization of related controls.
Consider adding an aria-label to the IconButton for better accessibility:
<IconButton aria={Locale.Chat.Actions.Speech} + aria-label={speechStatus ? "Stop speech" : "Start speech"} icon={speechStatus ? <SpeakStopIcon /> : <SpeakIcon />} text={ speechLoading ? "Loading..." : speechStatus ? Locale.Chat.Actions.Stop : Locale.Chat.Actions.Speech } onClick={() => { openaiSpeech( "NextChat,Unleash your imagination, experience the future of AI conversation.", ); }} />
This change will improve the accessibility of the button for screen reader users.
151-157
: Simplify onChange handler for voice selectionThe onChange handler for the voice selection dropdown can be simplified for better readability. Consider the following change:
onChange={(e) => { - props.updateConfig((config) => { - config.voice = TTSConfigValidator.voice( - e.currentTarget.value, - ); - }); + props.updateConfig((config) => ({ + ...config, + voice: TTSConfigValidator.voice(e.currentTarget.value), + })); }}This change maintains the same functionality while improving code clarity and adhering to immutability principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/components/tts-config.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/components/tts-config.tsx (2)
2-2
: LGTM: New imports and ttsPlayer initializationThe new imports and the creation of a single
ttsPlayer
instance outside the component are appropriate for the added speech synthesis functionality. This approach ensures efficient resource usage by avoiding recreation of the player on each render.Also applies to: 7-11, 14-19, 21-21
26-27
: LGTM: New state variables for speech synthesisThe addition of
speechLoading
andspeechStatus
state variables is appropriate for managing the loading and playback status of the speech synthesis feature. The naming is clear and descriptive, enhancing code readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/components/tts-config.tsx (1)
22-22
: Consider scoping the ttsPlayer instance to the component.Creating a global ttsPlayer instance could lead to issues if multiple instances of the component are rendered. Consider moving this into the component or managing it through React Context.
-const ttsPlayer = createTTSPlayer(); +function TTSConfigList(props: { + ttsConfig: TTSConfig; + updateConfig: (updater: (config: TTSConfig) => void) => void; +}) { + const ttsPlayer = React.useMemo(() => createTTSPlayer(), []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/components/tts-config.tsx (2 hunks)
🔇 Additional comments (1)
app/components/tts-config.tsx (1)
166-181
: LGTM! Voice selection implementation is clean and follows best practices.The code incorporates previous review suggestions and handles voice selection appropriately.
<div style={{ display: "flex", gap: "10px" }}> | ||
<IconButton | ||
aria={Locale.Chat.Actions.Speech} | ||
icon={speechStatus ? <SpeakStopIcon /> : <SpeakIcon />} | ||
text={ | ||
speechLoading | ||
? "Loading..." | ||
: speechStatus | ||
? Locale.Chat.Actions.Stop | ||
: Locale.Chat.Actions.Speech | ||
} | ||
onClick={() => { | ||
openaiSpeech( | ||
"NextChat,Unleash your imagination, experience the future of AI conversation.", | ||
); | ||
}} | ||
/> | ||
|
||
<Select | ||
value={props.ttsConfig.voice} | ||
onChange={(e) => { | ||
props.updateConfig((config) => { | ||
config.voice = TTSConfigValidator.voice( | ||
e.currentTarget.value, | ||
)), | ||
); | ||
}} | ||
> | ||
{DEFAULT_TTS_VOICES.map((v, i) => ( | ||
<option value={v} key={i}> | ||
{v} | ||
</option> | ||
))} | ||
</Select> | ||
); | ||
}); | ||
}} | ||
> | ||
{DEFAULT_TTS_VOICES.map((v, i) => ( | ||
<option value={v} key={i}> | ||
{v} | ||
</option> | ||
))} | ||
</Select> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Several UI improvements recommended.
- The preview text should be localized
- Consider moving inline styles to CSS classes
- The loading state could benefit from a loading spinner component
-<div style={{ display: "flex", gap: "10px" }}>
+<div className="tts-voice-controls">
-text={
- speechLoading
- ? "Loading..."
- : speechStatus
- ? Locale.Chat.Actions.Stop
- : Locale.Chat.Actions.Speech
-}
+text={
+ speechLoading
+ ? Locale.Chat.Actions.Loading
+ : speechStatus
+ ? Locale.Chat.Actions.Stop
+ : Locale.Chat.Actions.Speech
+}
-"NextChat,Unleash your imagination, experience the future of AI conversation."
+Locale.Settings.TTS.PreviewText
Add this CSS to your stylesheet:
.tts-voice-controls {
display: flex;
gap: 10px;
align-items: center;
}
if (typeof (error as Error).message === "string") { | ||
showToast((error as Error).message); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error type checking.
The error type assertion could be improved using type guards for better type safety.
-if (typeof (error as Error).message === "string") {
+if (error instanceof Error) {
showToast(error.message);
+} else {
+ showToast("An unexpected error occurred");
}
📝 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.
if (typeof (error as Error).message === "string") { | |
showToast((error as Error).message); | |
} | |
if (error instanceof Error) { | |
showToast(error.message); | |
} else { | |
showToast("An unexpected error occurred"); | |
} |
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
在 TTS 配置选项中添加了一个语音预览按钮,允许用户在应用设置中试听语音效果。
📝 补充信息 | Additional Information
Summary by CodeRabbit
New Features
Bug Fixes