fix(glass): memory leak fixes, BLE hook extraction, FlashList, and profiling tools#7586
fix(glass): memory leak fixes, BLE hook extraction, FlashList, and profiling tools#7586roseonlineownz-lab wants to merge 1 commit into
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: e8cbb162fb
ℹ️ 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".
| console.log('Rotated photo', rotated); | ||
| setPhotos((p) => { | ||
| const next = [...p, { data: rotated, timestamp }]; | ||
| return next.length > MAX_PHOTOS ? next.slice(next.length - MAX_PHOTOS) : next; |
There was a problem hiding this comment.
Once the gallery reaches MAX_PHOTOS, this cap keeps photos.length at 100, but the background InvalidateSync logic still uses a monotonically increasing processed count and only calls agent.addPhoto when processedPhotos.current.length > processed. After the 101st photo, the length is no longer greater than processed, so every new captured image is skipped by the agent even though it remains visible in the grid. This affects sessions that capture more than 100 photos; the UI continues updating, but agent processing silently stops.
Useful? React with 👍 / 👎.
|
|
||
| const { subscribed, error } = useBleCharacteristic(device, { |
There was a problem hiding this comment.
In the web Bluetooth flow checked in useDevice.ts, requestDevice only grants the custom OMI service in optionalServices, so subscribing here to the SIG battery service will fail with a service-access error on devices selected through that flow. This means the newly added battery display never receives notifications unless 0000180f-0000-1000-8000-00805f9b34fb/battery_service is also requested when pairing.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR addresses memory leaks and BLE subscription lifecycle issues in
Confidence Score: 3/5Three functional defects need addressing before merging: the O(n²) buffer concatenation in the photo chunk pipeline, a null-dereference in textToSpeech when unmount races with an in-flight TTS response, and a missing initial read in useBatteryLevel that leaves the battery UI blank. The core memory fixes (GATT disconnect, AudioContext close, photos cap, stopNotifications on unmount) are correct and well-structured. However, onChunk replaces one memory issue with a quadratic-allocation pattern that generates substantial GC pressure for every photo received. The stopAudio() call introduced on unmount also opens a null-dereference window inside textToSpeech for any response that resolves during navigation. And useBatteryLevel subscribes to notifications only, meaning the battery display stays hidden until the peripheral sends an unprompted update. DeviceView.tsx (chunk buffer concatenation), openai.ts (AudioContext lifetime), and useBatteryLevel.ts (missing initial characteristic read) need fixes before this is merged. Important Files Changed
Sequence DiagramsequenceDiagram
participant Main
participant DeviceView
participant useBleCharacteristic
participant useBatteryLevel
participant openai
participant GATT as BLE GATT
Main->>DeviceView: mount (device prop)
DeviceView->>useBleCharacteristic: subscribe(PHOTO_CHAR)
useBleCharacteristic->>GATT: getPrimaryService → getCharacteristic → startNotifications
GATT-->>useBleCharacteristic: characteristicvaluechanged events
useBleCharacteristic-->>DeviceView: onPhotoNotification(e)
DeviceView->>useBatteryLevel: subscribe(BATTERY_LEVEL_CHAR)
useBatteryLevel->>useBleCharacteristic: subscribe(BATTERY_CHAR)
useBleCharacteristic->>GATT: startNotifications (Battery)
GATT-->>useBatteryLevel: level notifications
DeviceView->>openai: textToSpeech(text)
openai->>openai: startAudio() → AudioContext
openai-->>DeviceView: audio plays
Note over DeviceView: Unmount
DeviceView->>openai: stopAudio() → AudioContext.close()
DeviceView->>useBleCharacteristic: cleanup (removeEventListener + stopNotifications)
useBleCharacteristic->>GATT: stopNotifications(PHOTO_CHAR)
useBleCharacteristic->>GATT: stopNotifications(BATTERY_CHAR)
DeviceView->>GATT: disconnect()
Reviews (1): Last reviewed commit: "perf(glass): replace FlatList with Flash..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| s.previousChunk = id; | ||
| } | ||
| } | ||
| s.buffer = new Uint8Array([...s.buffer, ...data]); |
There was a problem hiding this comment.
onChunk: Spreading two Uint8Array objects into a plain JS array and back into a Uint8Array on every BLE packet means the total allocation per photo is O(n²) in the number of chunks. A 100 KB JPEG arriving in 512-byte BLE packets (~200 chunks) will trigger ~200 spread operations whose cumulative cost grows quadratically, generating significant GC pressure and negating much of the memory work this PR tries to do. Use a typed array copy with Uint8Array.set() instead.
| s.buffer = new Uint8Array([...s.buffer, ...data]); | |
| const merged = new Uint8Array(s.buffer.length + data.length); | |
| merged.set(s.buffer, 0); | |
| merged.set(data, s.buffer.length); | |
| s.buffer = merged; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (!audioContext) startAudio(); | ||
| const audioBuffer = await audioContext!.decodeAudioData(response.data); | ||
|
|
||
| // Create an audio source | ||
| const source = audioContext.createBufferSource(); | ||
| const source = audioContext!.createBufferSource(); | ||
| source.buffer = audioBuffer; | ||
| source.connect(audioContext.destination); | ||
| source.start(); // Play the audio immediately |
There was a problem hiding this comment.
audioContext null-dereference after stopAudio() race: This PR adds a DeviceView unmount handler that calls stopAudio(), which sets audioContext = null. If a textToSpeech call is in flight when DeviceView unmounts, stopAudio() nulls audioContext between the synchronous startAudio() check and the await decodeAudioData call. The ! assertions on audioContext!.decodeAudioData and audioContext!.createBufferSource() will throw a TypeError swallowed by the surrounding try/catch, causing TTS to silently fail. Capture a local reference before the first await to fix this.
| export function useBatteryLevel(device: BluetoothRemoteGATTServer): UseBatteryLevelResult { | ||
| const [level, setLevel] = React.useState(-1); | ||
|
|
||
| const onBatteryNotification = React.useCallback((e: Event) => { | ||
| const value = (e.target as BluetoothRemoteGATTCharacteristic).value; | ||
| if (value && value.byteLength > 0) { | ||
| setLevel(value.getUint8(0)); | ||
| } | ||
| }, []); | ||
|
|
||
| const { subscribed, error } = useBleCharacteristic(device, { | ||
| serviceUUID: BATTERY_SERVICE_UUID, | ||
| characteristicUUID: BATTERY_LEVEL_CHAR_UUID, | ||
| onCharacteristicChanged: onBatteryNotification, | ||
| }); | ||
|
|
||
| return { level, subscribed, error }; | ||
| } |
There was a problem hiding this comment.
-1 until a notification fires: Per the BLE specification, Battery Level (0x2A19) is a readable + notifiable characteristic. Most peripherals only push a notification when the level changes — they do not send a notification on subscription. Without an initial readValue() call, level will remain -1 indefinitely, so the battery display in DeviceView (batteryLevel >= 0) will never render. An initial read inside a useEffect after subscription succeeds would fix this.
| if (growingSnapshots > recent.length * 0.3) { | ||
| const totalGrowth = snapshot.jsHeapUsed - (baselineRef.current?.jsHeapUsed ?? snapshot.jsHeapUsed); | ||
| if (totalGrowth > leakThresholdMb) { | ||
| setLeakDetected(true); | ||
| onLeakDetectedRef.current?.(snapshot, historyRef.current); | ||
| } | ||
| } |
There was a problem hiding this comment.
leakDetected is set to true but never reset: Once a sustained-growth window is detected, the flag stays true for the lifetime of the component — even if GC runs and memory levels out. The overlay will show the leak warning indefinitely, making it hard to know when the issue resolves. The flag should be cleared when the growth condition is no longer met.
| if (growingSnapshots > recent.length * 0.3) { | |
| const totalGrowth = snapshot.jsHeapUsed - (baselineRef.current?.jsHeapUsed ?? snapshot.jsHeapUsed); | |
| if (totalGrowth > leakThresholdMb) { | |
| setLeakDetected(true); | |
| onLeakDetectedRef.current?.(snapshot, historyRef.current); | |
| } | |
| } | |
| if (growingSnapshots > recent.length * 0.3) { | |
| const totalGrowth = snapshot.jsHeapUsed - (baselineRef.current?.jsHeapUsed ?? snapshot.jsHeapUsed); | |
| if (totalGrowth > leakThresholdMb) { | |
| setLeakDetected(true); | |
| onLeakDetectedRef.current?.(snapshot, historyRef.current); | |
| } else { | |
| setLeakDetected(false); | |
| } | |
| } else { | |
| setLeakDetected(false); | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ofiling tools - useBleCharacteristic: reusable hook for BLE subscribe/unsubscribe lifecycle with callbackRef and race-condition protection - useBatteryLevel: second consumer validating hook reusability via standard SIG battery service - usePhotos: BLE subscription cleanup, race condition fix, photos capped at MAX_PHOTOS=100 - useDevice: GATT disconnect + ongattserverdisconnected cleanup on unmount - openai: removed module-scope side effects, added stopAudio(), idempotent startAudio() - DeviceView: sync.stop() + stopAudio() on unmount, dead import removed - ScrollView+map replaced with FlashList numColumns=3 virtualized rendering - useMemoryProfiler: periodic JS heap snapshots with leak detection - MemoryProfilerOverlay: DEV-only floating memory overlay - MEMORY_PROFILING.md: profiling workflow documentation - OmiConnection: BLE subscription lifecycle improvements
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
omiGlass memory leak fixes: BLE subscription cleanup, GATT disconnect on unmount, FlashList virtualization, plus reusable useBleCharacteristic hook. Approve only — mobile/RN area.
2 participants
Summary
Comprehensive memory leak fixes, BLE subscription lifecycle improvements, and performance upgrades for omiGlass — squashed into one clean commit.
Changes (12 files, +899 / -157)
Memory Leak Fixes
usePhotos: BLE subscription cleanup (removeEventListener+stopNotifications), race condition fix withcancelledflag, photos capped atMAX_PHOTOS=100useDevice: GATT disconnect +ongattserverdisconnectedcleanup on unmountopenai: removed module-scope side effects, addedstopAudio(), idempotentstartAudio()DeviceView:sync.stop()+stopAudio()on unmount, dead import removedBLE Hook Extraction
useBleCharacteristic.ts(NEW): Reusable hook encapsulating BLE characteristic subscribe/unsubscribe lifecycle withcallbackRefpattern and race-condition protectionuseBatteryLevel.ts(NEW): Second consumer validating hook reusability via standard SIG Battery Service (0x180F)DeviceView: RefactoredusePhotosto useuseBleCharacteristicPerformance
ScrollView+.map()with FlashList (@shopify/flash-list) usingnumColumns={3}for virtualized off-screen view recyclingphoto.timestampProfiling Tools
useMemoryProfiler.ts(NEW): Periodic JS heap snapshots with leak detectionMemoryProfilerOverlay.tsx(NEW): DEV-only floating memory overlayMEMORY_PROFILING.md(NEW): Profiling workflow documentationOmiConnection: BLE subscription lifecycle improvements