UI updates while testing #122
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds GDrive UI error/disabled styling and renames connector ID to Changes
Sequence Diagram(s)sequenceDiagram
participant UI as VoiceChat (UI)
participant JS as VmWebrtc (JS wrapper)
participant Native as VmWebrtc Native (iOS)
participant OpenAI as OpenAI WebRTC Endpoint
UI->>JS: call emitVoiceSessionStatus("Connecting to OpenAI...")
JS->>Native: invoke native emitVoiceSessionStatus(status)
Note right of Native: Native emits onVoiceSessionStatus events\nat multiple lifecycle milestones
Native->>JS: onVoiceSessionStatus event(payload)
JS->>UI: propagate event to JS listeners
UI->>UI: update UI state (isConnecting / retry / error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
|
OSSF Scorecard (PR vs base)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/settings/GDriveConnectorConfigCore.tsx (1)
368-389: Remove test/debug code from production.The
handleDisconnectLongPresscallback andgenerateSecureRandomTokenhelper appear to be test/debug functionality that allows injecting random access tokens via long-press. This bypasses the normal OAuth flow and could be a security risk if users discover it.Recommendation: Remove this code or protect it behind a development-only flag (e.g.,
__DEV__).🔎 Proposed fix to remove debug code
- const handleDisconnectLongPress = useCallback(async () => { - try { - const randomToken = await generateSecureRandomToken(); - await saveGDriveAccessToken(randomToken); - Alert.alert("access token randomized"); - } catch { - Alert.alert("Error", "Failed to randomize access token."); - } - }, []); - - // Helper to generate a secure random "ya29." token (with 32-byte hex string) - async function generateSecureRandomToken(): Promise<string> { - // Use expo-random for cryptographically secure PRNG in React Native/Expo - // Generates 32 random bytes and encodes as hex - const { getRandomBytesAsync } = await import("expo-random"); - const bytes = await getRandomBytesAsync(32); - // Convert bytes to hex string - const hex = Array.from(bytes) - .map((b) => b.toString(16).padStart(2, "0")) - .join(""); - return `ya29.${hex}`; - }And remove the long press handler:
<ButtonCard icon="🧹" title="Disconnect" note="Disconnects and clears stored auth tokens." variant="destructive" disabled={!hasAuthTokens || isLoading} onPress={handleDisconnect} - onLongPress={handleDisconnectLongPress} - suppressOnPressAfterLongPress />
🧹 Nitpick comments (2)
components/settings/GDriveConnectorConfigCore.tsx (2)
694-709: Consider consolidating duplicate icon styles.The
statusIconErrorstyle (line 706) is identical tostatusIcon(line 705). You could reuse the existingstatusIconstyle instead of creating a duplicate.🔎 Proposed consolidation
Remove the duplicate style definition and reuse
statusIcon:- <Text style={styles.statusIconError}>⚠️</Text> + <Text style={styles.statusIcon}>⚠️</Text>Then remove line 706:
statusIcon: { fontSize: 16, marginRight: 8 }, - statusIconError: { fontSize: 16, marginRight: 8 },
244-613: Consider extracting sub-components to improve maintainability.The
GDriveConnectorConfigCorecomponent spans ~370 lines with multiple concerns (status display, connection actions, advanced settings). While the code is well-organized, extracting sub-components likeStatusBanner,ConnectionActions, andAdvancedSettingswould improve readability and testability.Based on coding guidelines: "avoid making components too large - better refactor them into smaller reusable components."
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/settings/GDriveConnectorConfigCore.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: When creating Expo React Native UI, avoid making components too large - better refactor them into smaller reusable components
Use Expo libraries instead of Node.js built-ins - useexpo-file-systeminstead of Node.jsfsmodule for file operations
Use Expo fetch instead of standard fetch for HTTP requests
Never use Node.jscryptomodule - instead useexpo-cryptopackage withCrypto.digestStringAsync()for cryptographic operations
Files:
components/settings/GDriveConnectorConfigCore.tsx
🔇 Additional comments (3)
components/settings/GDriveConnectorConfigCore.tsx (3)
210-216: LGTM! Improved disabled state handling.The disabled-first styling logic correctly prioritizes the disabled state over variant styles, ensuring consistent visual feedback when the button is disabled.
457-462: LGTM! Better visual distinction for configuration errors.The new error banner with warning icon and red styling provides clearer feedback when the client ID is missing, improving the user experience.
732-735: LGTM! Explicit disabled state styling.The explicit background color and full opacity for disabled cards provides better visual clarity compared to opacity-only approaches.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
modules/vm-webrtc/src/VmWebrtc.types.ts (1)
152-155: Consider using a newline separator instead of a space.When concatenating
groupDescriptionwithtoolkit.description, a single space may produce awkward or unclear text boundaries. A newline or double-newline would provide clearer separation consistent with the prompt additions below.🔎 Suggested change
// Start with group description if provided let description = groupDescription - ? `${groupDescription} ${toolkit.description}` + ? `${groupDescription}\n\n${toolkit.description}` : toolkit.description;modules/vm-webrtc/src/ToolkitManager.ts (1)
562-627: LGTM on the group description threading with one optional nit.The group-level prompt loading logic is well implemented with appropriate error handling via try-catch blocks. The description composition follows the same pattern as
exportToolDefinitionin VmWebrtc.types.ts.Optional nit: Lines 595-598 and 611-613 use
console.warnwhile the rest of the file uses the structuredlog.warn. Consider usinglog.warnfor consistency with the file's logging pattern, which would make these warnings appear in structured logs alongside other toolkit manager events.🔎 Example change for consistency
} catch (error) { // If loading fails, just continue without group prompt - console.warn( - `Failed to load group prompt addition for MCP tool ${groupPromptKey}:`, - error, + log.warn( + "[ToolkitManager] Failed to load group prompt addition for MCP tool", + {}, + { + promptKey: groupPromptKey, + error: error instanceof Error ? error.message : String(error), + }, ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/VoiceChat.tsxcomponents/settings/ConfigureToolsSheet.tsxcomponents/settings/ConnectorsConfig.tsxcomponents/settings/ToolList.tsxcomponents/settings/connectorOptions.tsmodules/vm-webrtc/index.tsmodules/vm-webrtc/ios/OpenAIWebRTCClient.swiftmodules/vm-webrtc/ios/VmWebrtcModule.swiftmodules/vm-webrtc/ios/WebRtcClientHelpers.swiftmodules/vm-webrtc/src/ToolkitManager.tsmodules/vm-webrtc/src/VmWebrtc.types.tsmodules/vm-webrtc/src/VmWebrtcModule.tsmodules/vm-webrtc/toolkits/toolkitGroups.json
✅ Files skipped from review due to trivial changes (1)
- modules/vm-webrtc/ios/WebRtcClientHelpers.swift
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: When creating Expo React Native UI, avoid making components too large - better refactor them into smaller reusable components
Use Expo libraries instead of Node.js built-ins - useexpo-file-systeminstead of Node.jsfsmodule for file operations
Use Expo fetch instead of standard fetch for HTTP requests
Never use Node.jscryptomodule - instead useexpo-cryptopackage withCrypto.digestStringAsync()for cryptographic operations
Files:
modules/vm-webrtc/index.tsmodules/vm-webrtc/src/VmWebrtcModule.tsmodules/vm-webrtc/src/VmWebrtc.types.tscomponents/settings/ToolList.tsxcomponents/settings/connectorOptions.tsapp/VoiceChat.tsxcomponents/settings/ConfigureToolsSheet.tsxcomponents/settings/ConnectorsConfig.tsxmodules/vm-webrtc/src/ToolkitManager.ts
**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
Swift objects like AVAudioPlayerDelegate should have serialized access when called from multiple threads - protect shared state with a serial DispatchQueue or ensure all method calls dispatch to the main queue
Files:
modules/vm-webrtc/ios/OpenAIWebRTCClient.swiftmodules/vm-webrtc/ios/VmWebrtcModule.swift
🧬 Code graph analysis (5)
modules/vm-webrtc/src/VmWebrtcModule.ts (3)
modules/vm-webrtc/index.ts (1)
emitVoiceSessionStatus(9-9)lib/logger.ts (1)
log(257-338)modules/vm-webrtc/ios/NativeLogger.swift (1)
log(26-68)
modules/vm-webrtc/src/VmWebrtc.types.ts (1)
lib/toolPrompts.ts (1)
loadToolPromptAddition(15-16)
app/VoiceChat.tsx (2)
modules/vm-webrtc/index.ts (1)
emitVoiceSessionStatus(9-9)modules/vm-webrtc/src/VmWebrtcModule.ts (1)
emitVoiceSessionStatus(207-218)
components/settings/ConfigureToolsSheet.tsx (1)
lib/toolPrompts.ts (2)
loadToolPromptAddition(15-16)saveToolPromptAddition(18-73)
modules/vm-webrtc/src/ToolkitManager.ts (2)
modules/vm-webrtc/src/VmWebrtc.types.ts (1)
exportToolDefinition(136-195)lib/toolPrompts.ts (1)
loadToolPromptAddition(15-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: iOS Simulator Build
🔇 Additional comments (22)
components/settings/connectorOptions.ts (1)
3-3: LGTM! Clean connector ID rename.The rename from
"gdrive"to"google_drive"is consistent with the broader PR changes inConnectorsConfig.tsxandtoolkitGroups.json.Also applies to: 30-30
components/settings/ConnectorsConfig.tsx (1)
51-52: LGTM!The conditional check correctly updated to match the new
"google_drive"connector ID.components/settings/ToolList.tsx (2)
15-15: LGTM!Clean implementation of the optional
onCustomizeGroupPromptprop with proper TypeScript typing.Also applies to: 24-24
51-71: LGTM! Well-structured conditional UI block.The group prompt customization button is properly:
- Conditionally rendered only when the callback is provided
- Accessible with appropriate role and label
- Styled consistently with the existing tool buttons pattern
modules/vm-webrtc/src/VmWebrtc.types.ts (2)
157-171: LGTM! Robust group-level prompt loading.The error-tolerant approach with
console.warnis appropriate here—failing to load a prompt addition should not break tool definition export. The_group_.{group}key pattern aligns with the UI implementation inConfigureToolsSheet.tsx.
173-187: LGTM!Tool-specific prompt loading follows the same resilient pattern as group prompts.
components/settings/ConfigureToolsSheet.tsx (4)
36-36: LGTM!Clean state addition to track group vs tool prompt mode.
172-186: LGTM! Well-designed group prompt flow.The pseudo-tool pattern effectively reuses the existing
ConfigurePromptModalinfrastructure for group-level prompts. UsingrequestAnimationFrameto defer modal opening after sheet close ensures smooth UI transitions.
193-198: LGTM! Clean key generation logic.The
getPromptKeyhelper centralizes the key format distinction between group (_group_.{group}) and tool ({group}.{name}) prompts, consistent with the loading logic inVmWebrtc.types.ts.
272-276: LGTM!Dynamic title properly reflects whether the user is customizing a group or individual tool prompt.
modules/vm-webrtc/index.ts (1)
9-9: LGTM!Clean re-export of
emitVoiceSessionStatusto expose the new voice session status API.modules/vm-webrtc/toolkits/toolkitGroups.json (1)
5-5: LGTM! Useful group descriptions added.The descriptions provide clear, concise summaries of each toolkit group's capabilities. These will enhance the group-level prompt composition feature.
Also applies to: 134-134, 236-236, 278-278, 421-421, 445-445, 469-469, 493-493, 512-512
modules/vm-webrtc/ios/VmWebrtcModule.swift (1)
343-349: LGTM!Clean implementation of
emitVoiceSessionStatusthat emits theonVoiceSessionStatusevent to JavaScript. The function correctly matches the event declared in theEvents()block (line 91) and the TypeScript typeVoiceSessionStatusEventPayload.modules/vm-webrtc/src/VmWebrtcModule.ts (2)
57-57: LGTM!The interface declaration for
emitVoiceSessionStatusis consistent with the existing method signatures in the module.
207-218: LGTM!The wrapper implementation follows the established pattern correctly:
- Module availability check with appropriate error
- Debug logging with the status update payload
- Delegation to native module
Clean and consistent with other exported functions like
muteUnmuteOutgoingAudio.app/VoiceChat.tsx (2)
24-24: LGTM!Import correctly added alongside other vm-webrtc exports.
369-372: LGTM!Moving state resets before the try block ensures the UI updates immediately when the user presses the start button, providing better feedback responsiveness.
modules/vm-webrtc/ios/OpenAIWebRTCClient.swift (2)
496-501: LGTM!Good placement of status emissions throughout the connection flow. The instrumentation provides clear progress feedback at each milestone without affecting the existing control flow or error handling.
561-565: LGTM!The status emissions follow a consistent pattern and are strategically placed at meaningful connection milestones:
- Audio session setup
- Peer connection establishment
- ICE gathering
- SDP exchange
- Connection finalization
- Final connected state
The
emitModuleEventmethod already handles the case where no emitter is configured (logs and returns), so these calls are safe.Also applies to: 578-582, 592-596, 617-621, 630-634, 642-646
modules/vm-webrtc/src/ToolkitManager.ts (3)
242-250: LGTM!The static toolkit export now correctly threads through the group description, aligning with the signature update in
exportToolDefinition.
684-703: LGTM!The group description lookup and threading through to
loadRemoteToolkitDefinitionsis clean. The optional chaining ongroup?.descriptionproperly handles the case where the group might not be found.
837-852: LGTM!The
groupDescriptionparameter is consistently threaded through the entire remote toolkit loading chain:
convertToolsForCachefetchAndCacheRemoteToolkitDefinitionsscheduleRemoteToolkitRefreshloadRemoteToolkitDefinitionsAll call sites are updated to pass the parameter correctly, and the optional nature (
groupDescription?: string) maintains backward compatibility.Also applies to: 855-892, 919-984, 986-1064
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.