perf: Optimize real-time demo #15
Conversation
WalkthroughThe changes in this pull request involve modifications across multiple files, focusing on enhancing OAuth authentication, workspace management, and voice selection in a React application. The Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
cspell.json (1)
7-13: Consider adding more OAuth-related termsSince the PR implements OAuth authentication functionality, consider adding these common OAuth-related terms to prevent false positives in spell checking:
"words": [ "packagejson", "pkce", "PKCE", "preinstall", + "oauth", + "authz", + "authn", "undici" ],examples/realtime-console/src/net.ts (4)
78-78: Remove console.log statements from production codeThe
console.logstatement on line 78 may clutter the console output and is generally not recommended in production code. Consider removing it or replacing it with a proper logging mechanism if needed for debugging.Apply this diff:
- console.log(`load ${page_index} page bots`, bots);
58-59: Maintain consistent pagination variable names across functionsIn
fetchAllWorkspaces, you usepage_num, while infetchAllBots, you usepage_index. For consistency and readability, consider using the same variable names across your functions.Apply this diff to rename
page_indextopage_numinfetchAllBots:export const fetchAllBots = async ( api: CozeAPI, workspaceId: string, ): Promise<BotOption[]> => { - let page_index = 1; + let page_num = 1; const page_size = 20; let allBots: BotOption[] = []; let hasMore = true; try { while (hasMore) { const response = await api.bots.list({ space_id: workspaceId, page_size, - page_index, + page_num, }); const bots: BotOption[] = response.space_bots.map(bot => ({ value: bot.bot_id, label: bot.bot_name, avatar: bot.icon_url, })); allBots = [...allBots, ...bots]; - console.log(`load ${page_index} page bots`, bots); + console.log(`load ${page_num} page bots`, bots); hasMore = response.space_bots.length === page_size; - page_index++; + page_num++; } return allBots; } catch (error) { console.error('get bots error:', error); throw error; } };Also applies to: 24-25
91-91: Specify return type forfetchAllVoicesfunctionTo enhance type safety and maintain consistency, consider specifying the return type as
Promise<VoiceOption[]>for thefetchAllVoicesfunction.Apply this diff:
export const fetchAllVoices = async (api: CozeAPI) + : Promise<VoiceOption[]> => {
95-104: Simplify voice list processing by mapping directlyCurrently, you filter and separate
customVoicesandsystemVoicesand then merge them back together. You can simplify this by mapping over the entireresponse.voice_listand, if necessary, sort the voices afterward.Apply this diff:
// Merge and format voice list - const customVoices = response.voice_list.filter( - voice => !voice.is_system_voice, - ); - const systemVoices = response.voice_list.filter( - voice => voice.is_system_voice, - ); - const formattedVoices = [...customVoices, ...systemVoices].map(voice => ({ + const formattedVoices = response.voice_list.map(voice => ({ value: voice.voice_id, preview_url: voice.preview_audio, name: voice.name, language_name: voice.language_name, is_system_voice: voice.is_system_voice, label: `${voice.name} (${voice.language_name})`, })); + // Optionally sort voices if needed + // formattedVoices.sort((a, b) => Number(a.is_system_voice) - Number(b.is_system_voice));examples/realtime-console/src/Settings.tsx (3)
265-294: Enhance error feedback in the loadData functionIn the
loadDatafunction, when an error occurs, a generic message is displayed:'Failed to load ' + objectName. Providing more specific error details can help users understand and resolve issues quicker. Consider appending the error message to the notification, ensuring that sensitive information is not exposed.Update the error handling as follows:
} catch (error) { - message.error('Failed to load ' + objectName); + message.error(`Failed to load ${objectName}: ${error.message || error}`); console.error(error); } finally {
Line range hint
452-483: Use state values instead of localStorage in the settings menuIn the settings dropdown menu, you are retrieving values directly from
localStorageto display the current settings. Since these values are already stored in the component's state or form fields, consider using those instead to ensure consistency and reactiveness when the state changes.Update the menu items to use state or form values:
<Menu.Item key="1"> <Text ellipsis style={{ maxWidth: 200 }} title={accessToken}> access_token: {accessToken || 'Not set'} </Text> </Menu.Item> <Menu.Item key="2"> <Text ellipsis style={{ maxWidth: 200 }} - title={form.getFieldValue('workspaceId')} + title={selectedWorkspace} > - workspace_id: {localStorage.getItem('workspaceId') || 'Not set'} + workspace_id: {selectedWorkspace || 'Not set'} </Text> </Menu.Item>
132-144: Prevent multiple audio instances in the VoiceSelect componentIn the
VoiceSelectcomponent, each click on the play icon may create a new audio instance ifhandlePreviewis called multiple times rapidly. This could lead to overlapping audio playback. Consider disabling the play button while the audio is loading or playing to prevent this issue.You can modify the
handlePreviewfunction to manage playback state:const [isPlaying, setIsPlaying] = useState(false); const handlePreview = (previewUrl: string, e: React.MouseEvent) => { e.stopPropagation(); + if (isPlaying) return; if (audioPlayer.src === previewUrl) { if (audioPlayer.paused) { audioPlayer.play(); + setIsPlaying(true); } else { audioPlayer.pause(); + setIsPlaying(false); } } else { audioPlayer.src = previewUrl; audioPlayer.play(); + setIsPlaying(true); } }; + audioPlayer.onended = () => setIsPlaying(false); + audioPlayer.onpause = () => setIsPlaying(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cspell.json(1 hunks)examples/realtime-console/src/Settings.tsx(8 hunks)examples/realtime-console/src/net.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
examples/realtime-console/src/Settings.tsx (1)
270-298: Add retry mechanism for failed API calls.The loadData function should implement a retry mechanism to handle temporary network issues or API failures.
Consider implementing a retry mechanism:
const loadData = async ( api: CozeAPI, form: FormInstance<any>, objectName: string, dataLocalKey: string, setLoading: (loading: boolean) => void, setData: (data: any[]) => void, fetchData: (api: CozeAPI, id?: string) => Promise<any[]>, parentId?: string, + maxRetries: number = 3, + retryDelay: number = 1000, ): Promise<void> => { setLoading(true); + let retries = 0; + + const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); + + const attemptFetch = async (): Promise<any[]> => { + try { + return await fetchData(api, parentId); + } catch (error) { + if (retries < maxRetries) { + retries++; + await delay(retryDelay * retries); + return attemptFetch(); + } + throw error; + } + }; try { - const data = await fetchData(api, parentId); + const data = await attemptFetch(); // if no data in localStorage, set the first data as default const storedData = localStorage.getItem(dataLocalKey); if (!storedData && data.length > 0) { const firstData = data[0].value; localStorage.setItem(dataLocalKey, firstData); form.setFieldsValue({ [dataLocalKey]: firstData }); } setData(data); } catch (error) { message.error('Failed to load ' + objectName); console.error(error); } finally { setLoading(false); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
examples/realtime-console/src/Settings.tsx(8 hunks)packages/coze-js/src/resources/workspaces/workspaces.ts(1 hunks)
🔇 Additional comments (1)
packages/coze-js/src/resources/workspaces/workspaces.ts (1)
24-24: Address the TODO comment regarding response format.
The TODO comment indicates a known issue with the response format. This needs to be investigated and properly documented.
Let's analyze the API response format:
Would you like me to:
- Create an issue to track the response format investigation?
- Add comprehensive response validation?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/coze-js/src/resources/workspaces/workspaces.ts (1)
26-26: Remove console.log statement.Console.log statements should not be committed to production code. Consider using proper logging mechanisms if debugging information is needed.
- console.log('response', response);examples/realtime-console/src/Settings.tsx (3)
211-278: Consider enhancing token security.While the token management implementation is solid, storing tokens in localStorage makes them vulnerable to XSS attacks. Consider:
- Using secure httpOnly cookies (requires backend changes)
- Implementing token encryption before storage
- Adding additional validation for token format
280-309: Consider implementing retry logic for loadData.The loadData function handles errors well, but for better reliability, consider:
- Implementing retry logic for transient failures
- Adding timeout handling
419-423: Improve error message clarity.The error message combines multiple fields in a single message. Consider separating the validation messages for better user experience:
- message.error( - 'Access Token, Workspace, Bot, and Base URL are required!', - ); + const missingFields = []; + if (!accessToken) missingFields.push('Access Token'); + if (!workspaceId) missingFields.push('Workspace'); + if (!botId) missingFields.push('Bot'); + message.error(`Please provide: ${missingFields.join(', ')}`);examples/realtime-console/src/use-coze-api.tsx (2)
70-70: Removeconsole.logstatement from production codeThe
console.logstatement on line 70 is likely intended for debugging purposes. It's advisable to remove such statements from production code or replace them with a proper logging mechanism if necessary.Apply this diff to remove the debug statement:
- console.log(`load ${pageIndex} page bots`, bots);
45-81: Refactor pagination logic to reduce code duplicationThe pagination logic in
fetchAllBotsandfetchAllWorkspacesis similar. Consider extracting a reusable helper function to improve maintainability and reduce code duplication.For example, you could create a generic function to handle paginated API requests:
const fetchAllItems = async <T>( fetchPage: (pageIndex: number, pageSize: number) => Promise<T[]>, pageSize: number = 20 ): Promise<T[]> => { let pageIndex = 1; let allItems: T[] = []; let hasMore = true; try { while (hasMore) { const items = await fetchPage(pageIndex, pageSize); allItems = [...allItems, ...items]; hasMore = items.length === pageSize; pageIndex++; } return allItems; } catch (error) { throw error; } };You can then refactor
fetchAllBotsandfetchAllWorkspacesto use this helper function.Also applies to: 110-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cspell.json(1 hunks)examples/realtime-console/src/Settings.tsx(10 hunks)examples/realtime-console/src/use-coze-api.tsx(1 hunks)packages/coze-js/src/resources/workspaces/workspaces.ts(1 hunks)packages/coze-js/test/resources/workspaces.spec.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/coze-js/test/resources/workspaces.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- cspell.json
🔇 Additional comments (5)
packages/coze-js/src/resources/workspaces/workspaces.ts (2)
15-18: LGTM! Good type safety improvement.
The explicit Promise return type enhances type safety and makes the API contract clearer.
27-27:
Previous error handling concerns are still valid.
The current implementation still lacks proper error handling and type validation as mentioned in the previous review. The response processing could fail silently or return invalid data.
Let's verify the response format across the codebase:
#!/bin/bash
# Description: Check response handling patterns in workspace-related code
# Look for potential inconsistencies in workspace response handling
# Search for workspace response processing patterns
rg -A 5 'safeJsonParse.*workspace'
# Search for OpenSpaceData type usage to verify consistent handling
ast-grep --pattern 'interface OpenSpaceData {
$$$
}'examples/realtime-console/src/Settings.tsx (3)
117-191: LGTM! Well-implemented VoiceSelect component with audio preview.
The component demonstrates good practices:
- Proper audio resource cleanup
- Error handling for audio playback
- Accessibility attributes for the play button
- Correct state management for play/pause
75-115: LGTM! Well-structured BotSelect component.
Good implementation with:
- Helpful empty state message
- Avatar handling with fallback icon
- Proper disabled state management
564-569: Skip: Tooltip language inconsistency.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
examples/realtime-console/src/Settings.tsx (1)
422-426: Improve validation message consistency.The error message doesn't accurately reflect the required fields, and the validation could be handled by the form's built-in validation.
-if (!accessToken || !workspaceId || !botId) { - message.error( - 'Access Token, Workspace, Bot, and Base URL are required!', - ); - return; -} +const requiredFields = ['accessToken', 'workspaceId', 'botId', 'baseURL']; +const missingFields = requiredFields.filter(field => !values[field]); +if (missingFields.length > 0) { + message.error( + `The following fields are required: ${missingFields.join(', ')}`, + ); + return; +}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (9)
examples/realtime-console/src/ConsoleFooter.tsx (3)
67-67: Consider enhancing the error message with specific configuration requirements.While the message guides users to the Settings, it could be more helpful by indicating which configurations are required (e.g., access token, bot ID).
- message.error('Please click Settings to set configuration first'); + message.error('Please click Settings to configure your access token and bot ID first');
94-95: Add input validation before JSON parsing.Consider validating the input string before attempting to parse it to provide more specific error messages and prevent unnecessary try-catch overhead.
const handleSendMessage = (values: { eventData: string }) => { if (!clientRef.current) { message.error('Please click Settings to set configuration first'); return; } + if (!values.eventData?.trim()) { + message.error('Message cannot be empty'); + return; + } try {
107-107: Standardize optional chaining usage.The function uses optional chaining inconsistently. Consider standardizing its usage throughout the function for better code consistency.
- if (!clientRef?.current) { + if (!clientRef.current) { message.error('Please click Settings to set configuration first'); return; } try { - clientRef?.current?.enableAudioPropertiesReport({ interval: 1000 }); + clientRef.current.enableAudioPropertiesReport({ interval: 1000 });examples/realtime-console/src/RealtimeConsole.tsx (3)
67-67: Consider enhancing feedback for auto-configuration process.While the change from error message to console log aligns with the new automatic authorization flow, consider adding user feedback for cases where auto-configuration is in progress or fails.
Consider this enhancement:
- console.log('no access token or bot id, auto config'); + console.log('no access token or bot id, auto config'); + message.loading({ + content: 'Attempting automatic configuration...', + key: 'autoConfig' + });
Line range hint
71-82: Consider adding retry mechanism for client initialization.The client initialization could benefit from a retry mechanism when using automatic configuration, especially since the token might not be immediately available.
Consider adding a retry mechanism:
const client = new RealtimeClient({ accessToken: accessToken.trim(), botId: botId.trim(), voiceId: voiceId.trim(), - // conversationId: '1234567890', // Optional + retryAttempts: 3, // Add retry attempts + retryDelay: 1000, // Add delay between retries debug: true, baseURL: baseURL.trim(), allowPersonalAccessTokenInBrowser: true, audioMutedDefault: !isMicrophoneOn, });
Line range hint
126-159: Consider optimizing event merging logic.The
mergeEventfunction could be optimized to handle more event types and reduce unnecessary iterations.Consider this optimization:
const mergeEvent = (prevEvents: any[], event: any) => { - if (prevEvents.length === 0) { + const lastEvent = prevEvents[prevEvents.length - 1]; + if (!lastEvent) { return null; } - const lastEvent = prevEvents[prevEvents.length - 1]; if ( lastEvent.event === 'conversation.message.delta' && event.event === 'conversation.message.delta' ) { return { time: lastEvent.time, event: lastEvent.event, data: { ...lastEvent.data, data: { ...lastEvent.data.data, content: lastEvent.data.data.content + event.data.data.content, }, }, }; } return null; };examples/realtime-console/src/Settings.tsx (1)
371-377: Enhance form validation logicThe form validation could be improved by:
- Using form-level validation rules instead of manual checks
- Adding proper type checking for values
- Implementing more descriptive error messages
- const { workspaceId, botId, voiceId } = values; - if (!accessToken || !workspaceId || !botId) { - message.error( - 'Access Token, Workspace, Bot, and Base URL are required!', - ); - return; - } + const { workspaceId, botId, voiceId, baseURL } = values; + + // Validate required fields using form rules + form.setFields([ + { + name: 'accessToken', + errors: !accessToken ? ['Access Token is required'] : [], + }, + { + name: 'workspaceId', + errors: !workspaceId ? ['Please select a workspace'] : [], + }, + { + name: 'botId', + errors: !botId ? ['Please select a bot'] : [], + }, + { + name: 'baseURL', + errors: !baseURL ? ['Base URL is required'] : + !/^https?:\/\/.+/.test(baseURL) ? ['Invalid URL format'] : [], + }, + ]);examples/realtime-console/src/use-coze-api.tsx (1)
74-74: Consider removing or adjusting the debug log statementThe
console.logstatement used for debugging may not be suitable for production code as it can clutter the console and potentially reveal sensitive information. Consider using a logging library with appropriate log levels or remove the statement before deploying.examples/realtime-console/src/VoiceSelect.tsx (1)
30-30: Remove debuggingconsole.logstatementThe
console.log('voice xx', voice);appears to be a leftover from debugging. It's best practice to remove such statements in production code to keep the console output clean.Apply this diff to remove the debugging statement:
- console.log('voice xx', voice);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
examples/realtime-console/src/ConsoleFooter.tsx(4 hunks)examples/realtime-console/src/RealtimeConsole.tsx(1 hunks)examples/realtime-console/src/Settings.tsx(10 hunks)examples/realtime-console/src/VoiceSelect.tsx(1 hunks)examples/realtime-console/src/use-coze-api.tsx(1 hunks)
🔇 Additional comments (2)
examples/realtime-console/src/ConsoleFooter.tsx (1)
Line range hint 67-121: Overall improvement in user guidance with room for enhancement.
The consistent update of error messages across all functions improves user guidance by directing them to the Settings configuration. However, consider implementing the suggested improvements to enhance:
- Error message specificity
- Permission handling
- Input validation
- Code consistency
- Error handling organization
These changes would further improve the robustness and maintainability of the component while maintaining its enhanced user experience.
examples/realtime-console/src/RealtimeConsole.tsx (1)
Line range hint 64-82: Verify error handling consistency with auto-configuration flow.
The initialization logic needs to be verified against the new automatic authorization flow mentioned in the PR objectives.
Let's verify the auto-configuration implementation:
✅ Verification successful
Let me gather more information about the auto-configuration implementation.
Let me check one more aspect to understand the OAuth flow implementation better.
Error handling is consistent with auto-configuration flow
The implementation correctly handles the auto-configuration scenario:
- When access token/bot ID is missing, it logs and returns early with appropriate user feedback
- OAuth flow is properly implemented with PKCE for secure token exchange
- Comprehensive error handling exists for token refresh, exchange, and validation
- User is appropriately guided to configure settings when required through consistent error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for auto-configuration implementation and related error handling
# Test 1: Look for auto-configuration implementation
echo "Checking for auto-configuration implementation..."
rg -l "auto.*config" --type ts --type tsx
# Test 2: Check error handling patterns across the codebase
echo "Checking error handling patterns..."
ast-grep --pattern 'message.error($$$)'
Length of output: 3467
Script:
#!/bin/bash
# Search for auto-configuration related code in RealtimeConsole.tsx and Settings.tsx
echo "Checking RealtimeConsole.tsx implementation..."
rg "auto.*config|!accessToken.*return" examples/realtime-console/src/RealtimeConsole.tsx -A 5
echo -e "\nChecking Settings.tsx implementation..."
rg "auto.*config|initializeOAuth|exchangeCodeForToken" examples/realtime-console/src/Settings.tsx -A 5
Length of output: 945
Script:
#!/bin/bash
# Check for OAuth flow implementation and configuration handling
echo "Checking OAuth implementation..."
rg "getPKCEOAuthToken|initializeOAuth" examples/realtime-console/src/Settings.tsx -B 5 -A 10
echo -e "\nChecking configuration state management..."
rg "setToken|setAccessToken|setBotId" examples/realtime-console/src/Settings.tsx -B 2 -A 2
Length of output: 2466
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
common/config/subspaces/default/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
examples/realtime-console/package.json(2 hunks)examples/realtime-console/src/RealtimeConsole.tsx(4 hunks)packages/realtime-api/src/index.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/realtime-console/src/RealtimeConsole.tsx
- packages/realtime-api/src/index.ts
🔇 Additional comments (1)
examples/realtime-console/package.json (1)
139-139: LGTM! Surge dependency is correctly configured.
The surge package is appropriately added as a devDependency since it's only needed for deployment operations.
3 participants
Summary by CodeRabbit
New Features
VoiceSelectcomponent for selecting and cloning voice options.RealtimeClientwith options for suppressing stationary and non-stationary noise during audio interactions.Bug Fixes
Documentation
Tests
EngineClientandRealtimeClient.