Make compaction optional #123
Conversation
resets the conversation
📝 WalkthroughWalkthroughA new boolean flag Changes
Sequence DiagramsequenceDiagram
actor User
participant Configure as ConfigureContextWindow
participant App as app/index.tsx
participant DevSettings as developerSettings
participant Voice as VoiceChat
participant Bridge as VmWebrtcModule
participant Client as OpenAIWebRTCClient
participant Handler as WebRTCEventHandler
rect rgb(220,235,245)
App->>DevSettings: loadCompactionDisabled()
DevSettings-->>App: boolean
App->>Configure: render(disableCompaction)
end
rect rgb(220,245,220)
User->>Configure: toggle Switch
Configure->>App: onDisableCompactionChange(value)
App->>DevSettings: saveCompactionDisabled(value)
App->>Voice: render(disableCompaction)
end
rect rgb(245,235,210)
Voice->>Bridge: openConnection(customConnectionOptions{disableCompaction})
Bridge->>Client: openConnection(..., disableCompaction: Bool?)
Client->>Handler: configureDisableCompaction(disabled: Bool)
Handler->>Handler: set flag
end
rect rgb(245,220,220)
Handler->>Handler: compactConversationItems()
alt disableCompaction == true
Handler->>Handler: log & skip compaction
else
Handler->>Handler: perform compaction
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.swift📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)modules/vm-webrtc/ios/WebRTCEventHandler.swift (1)
⏰ 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)
🔇 Additional comments (3)
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 |
|
OSSF Scorecard (PR vs base)
|
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @app/VoiceChat.tsx:
- Line 418: The useCallback for handleStartVoiceSession closes over the
disableCompaction prop but disableCompaction is not included in its dependency
array, causing stale reads; update the dependency array for
handleStartVoiceSession to include disableCompaction so the callback re-creates
when the prop changes (locate the useCallback that defines
handleStartVoiceSession and add disableCompaction to its dependency list).
In @modules/vm-webrtc/ios/WebRTCEventHandler.swift:
- Around line 2473-2481: When compaction is disabled the early return in
compactConversationItems leaves the compactionInProgress flag set and blocks
future compactions; before returning from the guard branch in
compactConversationItems, reset compactionInProgress to false (perform the
mutation on the same conversationQueue.sync context as the read) so callers that
set compactionInProgress (e.g., where compactionInProgress = true is set around
lines 1667 and 1800) are not left in a permanently locked state; keep the
existing log call and then set compactionInProgress = false before exiting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/VoiceChat.tsxapp/index.tsxcomponents/settings/ConfigureContextWindow.tsxlib/developerSettings.tsmodules/vm-webrtc/ios/OpenAIWebRTCClient.swiftmodules/vm-webrtc/ios/VmWebrtcModule.swiftmodules/vm-webrtc/ios/WebRTCEventHandler.swiftmodules/vm-webrtc/src/VmWebrtc.types.ts
🧰 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/src/VmWebrtc.types.tslib/developerSettings.tscomponents/settings/ConfigureContextWindow.tsxapp/VoiceChat.tsxapp/index.tsx
**/*.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/VmWebrtcModule.swiftmodules/vm-webrtc/ios/WebRTCEventHandler.swiftmodules/vm-webrtc/ios/OpenAIWebRTCClient.swift
🧠 Learnings (1)
📚 Learning: 2025-11-26T07:57:56.704Z
Learnt from: CR
Repo: vibemachine-labs/arty PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T07:57:56.704Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : When creating Expo React Native UI, avoid making components too large - better refactor them into smaller reusable components
Applied to files:
components/settings/ConfigureContextWindow.tsx
🧬 Code graph analysis (4)
lib/developerSettings.ts (1)
lib/logger.ts (2)
error(308-310)warn(305-307)
app/index.tsx (1)
lib/developerSettings.ts (2)
loadCompactionDisabled(68-79)saveCompactionDisabled(81-90)
modules/vm-webrtc/ios/WebRTCEventHandler.swift (3)
modules/vm-webrtc/ios/NativeLogger.swift (1)
log(26-68)modules/vm-webrtc/ios/ConversationStateMachine.swift (1)
log(910-933)modules/vm-webrtc/ios/VmWebrtcLogging.swift (1)
logAttributes(34-41)
modules/vm-webrtc/ios/OpenAIWebRTCClient.swift (1)
modules/vm-webrtc/ios/WebRTCEventHandler.swift (2)
configureConversationTurnLimit(2071-2094)configureDisableCompaction(2096-2109)
⏰ 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 (19)
modules/vm-webrtc/src/VmWebrtc.types.ts (1)
224-224: LGTM!The optional
disableCompactionfield is correctly added to theOpenAIConnectionOptionsinterface with a clear comment explaining its purpose.components/settings/ConfigureContextWindow.tsx (3)
2-2: LGTM!The new props are correctly added and destructured following the existing pattern.
Also applies to: 11-11, 14-14, 22-22, 25-25
87-87: Good UX decision to disable the slider.Disabling the Max Conversation Turns slider when compaction is disabled makes logical sense since the turns limit won't be enforced.
91-106: LGTM!The new Disable Compaction section follows the existing UI patterns and provides clear user feedback about the feature's behavior.
app/VoiceChat.tsx (1)
53-53: LGTM!The new
disableCompactionprop is correctly added to the component interface and destructured.Also applies to: 66-66
modules/vm-webrtc/ios/VmWebrtcModule.swift (2)
38-39: LGTM!The optional
disableCompactionfield is correctly added to theOpenAIConnectionOptionsstruct following the existing pattern for optional fields.
226-226: LGTM!The
disableCompactionoption is correctly passed through to the WebRTC client'sopenConnectionmethod.lib/developerSettings.ts (3)
5-5: LGTM!The constant follows the existing naming convention for developer settings keys.
68-79: LGTM!The
loadCompactionDisabledfunction correctly implements the same pattern as existing settings loaders with appropriate default behavior (compaction enabled) and error handling.
81-90: LGTM!The
saveCompactionDisabledfunction follows the existing pattern with consistent error handling.modules/vm-webrtc/ios/OpenAIWebRTCClient.swift (3)
91-91: LGTM! Property declaration is correct.The
disableCompactionproperty follows the same pattern as other configuration flags in the class, with an appropriate default offalse(compaction enabled).
438-438: LGTM! Function signature extension is consistent.The
disableCompaction: Bool?parameter is appropriately optional and follows the same pattern as other configuration parameters in the function signature.
487-492: LGTM! Configuration is properly stored and propagated.The
disableCompactionvalue is correctly defaulted tofalsewhen not provided and properly passed to the event handler configuration. Thread safety is maintained through the@MainActorannotation onopenConnection.modules/vm-webrtc/ios/WebRTCEventHandler.swift (2)
163-163: LGTM! Property declaration is correct.The
disableCompactionproperty is appropriately placed with other conversation tracking configuration and defaults tofalse(compaction enabled).
2096-2109: LGTM! Configuration function is properly implemented.The
configureDisableCompactionfunction correctly usesconversationQueuefor thread-safe access and follows the same pattern as other configuration functions in the class.app/index.tsx (4)
32-35: LGTM! Imports are correct.The persistence functions
loadCompactionDisabledandsaveCompactionDisabledare properly imported from the developer settings module.
125-125: LGTM! State initialization and preference loading are correct.The
disableCompactionstate is properly initialized with a default offalse(compaction enabled) and loaded from persistent storage during component mount, following the same pattern as other preference values.Also applies to: 204-222
364-372: LGTM! Change handler is properly implemented.The
handleDisableCompactionChangecallback correctly updates both local state and persisted storage, following the same pattern as other preference handlers in the component.
521-521: LGTM! Props are correctly propagated to child components.The
disableCompactionpreference is properly threaded through toVoiceChat(for connection configuration) andConfigureContextWindow(for UI control), with the change handler callback correctly wired to propagate updates.Also applies to: 583-586
| audioSpeed: voiceSpeed, | ||
| maxConversationTurns, | ||
| retentionRatio, | ||
| disableCompaction, |
There was a problem hiding this comment.
Missing dependency in useCallback.
The disableCompaction prop is used inside handleStartVoiceSession but is missing from its dependency array (lines 537-550). This creates a stale closure that won't reflect updates to the disableCompaction value.
🔎 Proposed fix
Add disableCompaction to the dependency array:
}, [
audioOutput,
baseConnectionOptions,
hasMicPermission,
isConnecting,
isSessionActive,
mainPromptAddition,
selectedVoice,
selectedVadMode,
voiceSpeed,
maxConversationTurns,
retentionRatio,
+ disableCompaction,
selectedLanguage,
]);📝 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.
| disableCompaction, | |
| }, [ | |
| audioOutput, | |
| baseConnectionOptions, | |
| hasMicPermission, | |
| isConnecting, | |
| isSessionActive, | |
| mainPromptAddition, | |
| selectedVoice, | |
| selectedVadMode, | |
| voiceSpeed, | |
| maxConversationTurns, | |
| retentionRatio, | |
| disableCompaction, | |
| selectedLanguage, | |
| ]); |
🤖 Prompt for AI Agents
In @app/VoiceChat.tsx at line 418, The useCallback for handleStartVoiceSession
closes over the disableCompaction prop but disableCompaction is not included in
its dependency array, causing stale reads; update the dependency array for
handleStartVoiceSession to include disableCompaction so the callback re-creates
when the prop changes (locate the useCallback that defines
handleStartVoiceSession and add disableCompaction to its dependency list).
the tool configuration screen
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @components/ui/BottomSheet.tsx:
- Around line 118-123: The ScrollView isn't expanding because the scrollView
style uses flexGrow: 0; update the BottomSheet component's scrollView style (and
where ScrollView is rendered) to use flex: 1 instead of flexGrow: 0 so the
ScrollView fills remaining space and allows proper scrolling; keep scrollContent
as-is (flexGrow: 1) to let inner content size correctly.
- Around line 68-75: The ScrollView inside BottomSheet is using
styles.scrollView with flexGrow: 0 which prevents it from filling the available
space; change styles.scrollView to use flex: 1 and change styles.scrollContent
to use flexGrow: 1 so the ScrollView expands within the bottom sheet and content
can scroll correctly (update the style definitions referenced by ScrollView and
contentContainerStyle), and remove the redundant explicit boolean props
showsVerticalScrollIndicator={true} and bounces={true} from the ScrollView
element.
🧹 Nitpick comments (1)
components/ui/BottomSheet.tsx (1)
71-72: Consider removing redundant boolean props.The
showsVerticalScrollIndicatorandbouncesprops are explicitly set totrue, which are their default values on iOS. While harmless, omitting them would reduce visual noise.🔎 Proposed refactor
<ScrollView style={styles.scrollView} contentContainerStyle={styles.scrollContent} - showsVerticalScrollIndicator={true} - bounces={true} > {children} </ScrollView>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/ui/BottomSheet.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/ui/BottomSheet.tsx
⏰ 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 (2)
components/ui/BottomSheet.tsx (2)
63-67: LGTM - Clear conditional rendering.The reformatted title rendering with explicit parentheses and null return improves code clarity without changing functionality.
97-97: LGTM - Appropriate maxHeight constraint.Setting
maxHeightto 85% of screen height is a good UX practice for bottom sheets, ensuring the backdrop remains partially visible and the modal context is clear.
| <ScrollView | ||
| style={styles.scrollView} | ||
| contentContainerStyle={styles.scrollContent} | ||
| showsVerticalScrollIndicator={true} | ||
| bounces={true} | ||
| > | ||
| {children} | ||
| </ScrollView> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the BottomSheet.tsx file to see the actual implementation
cat -n components/ui/BottomSheet.tsxRepository: vibemachine-labs/arty
Length of output: 3760
🏁 Script executed:
# Search for other ScrollView implementations to compare styling patterns
rg -n -A 5 -B 2 '<ScrollView' --type=tsx --type=ts -g '!node_modules' -g '!*.test.*'Repository: vibemachine-labs/arty
Length of output: 92
🏁 Script executed:
# Search for ScrollView in React Native TypeScript files without type flag
rg -n -A 5 '<ScrollView' -g '*.tsx' -g '*.ts' | head -100Repository: vibemachine-labs/arty
Length of output: 5312
🏁 Script executed:
# Also check if there are any Expo ScrollView imports or usages
rg 'ScrollView' -g '*.tsx' -g '*.ts' | grep -E '(import|from|expo)' | head -20Repository: vibemachine-labs/arty
Length of output: 181
Consider using flex: 1 instead of flexGrow: 0 on the ScrollView.
The ScrollView styling uses flexGrow: 0, which prevents it from filling the available space in the bottom sheet container. This is unusual and could prevent proper scroll detection when content exceeds the maxHeight constraint.
The standard React Native pattern for a ScrollView in a constrained container is flex: 1:
Suggested fix
scrollView: {
flex: 1,
},
scrollContent: {
flexGrow: 1,
},Additionally, the explicit boolean props showsVerticalScrollIndicator={true} and bounces={true} are redundant defaults and can be removed.
🤖 Prompt for AI Agents
In @components/ui/BottomSheet.tsx around lines 68 - 75, The ScrollView inside
BottomSheet is using styles.scrollView with flexGrow: 0 which prevents it from
filling the available space; change styles.scrollView to use flex: 1 and change
styles.scrollContent to use flexGrow: 1 so the ScrollView expands within the
bottom sheet and content can scroll correctly (update the style definitions
referenced by ScrollView and contentContainerStyle), and remove the redundant
explicit boolean props showsVerticalScrollIndicator={true} and bounces={true}
from the ScrollView element.
| scrollView: { | ||
| flexGrow: 0, | ||
| }, | ||
| scrollContent: { | ||
| flexGrow: 1, | ||
| }, |
There was a problem hiding this comment.
Replace flexGrow: 0 with flex: 1 on the ScrollView style.
The scrollView style uses flexGrow: 0, which prevents the ScrollView from expanding to fill the available space in the bottom sheet. This contradicts the typical pattern for scrollable containers and may cause the scrolling functionality to fail.
🔎 Proposed fix
scrollView: {
- flexGrow: 0,
+ flex: 1,
},
scrollContent: {
flexGrow: 1,
},This allows the ScrollView to fill the remaining space after the header and handle, enabling proper scroll detection when content overflows.
📝 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.
| scrollView: { | |
| flexGrow: 0, | |
| }, | |
| scrollContent: { | |
| flexGrow: 1, | |
| }, | |
| scrollView: { | |
| flex: 1, | |
| }, | |
| scrollContent: { | |
| flexGrow: 1, | |
| }, |
🤖 Prompt for AI Agents
In @components/ui/BottomSheet.tsx around lines 118 - 123, The ScrollView isn't
expanding because the scrollView style uses flexGrow: 0; update the BottomSheet
component's scrollView style (and where ScrollView is rendered) to use flex: 1
instead of flexGrow: 0 so the ScrollView fills remaining space and allows proper
scrolling; keep scrollContent as-is (flexGrow: 1) to let inner content size
correctly.
Add ability to disable compaction since it currently has a bug that resets the conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.