Add gallery video bug reporting and recorded video integrity checks#2507
Closed
Add gallery video bug reporting and recorded video integrity checks#2507
Conversation
- ASG: RecordedVideoIntegrityChecker, MediaCaptureService, AsgCameraServer, AsgClientServiceManager updates - Mobile: MantleBridge, bug report incident and gallery playback reporting modules with tests - Refine feedback settings and AwesomeGalleryViewer integration Made-with: Cursor
📝 WalkthroughWalkthroughIntroduces asynchronous post-processing and integrity verification for recorded videos, adds a RecordedVideoIntegrityChecker, expands server sync to skip pending/active captures, implements gallery video playback bug reporting with dedupe, and adds mobile-native test-audio injection and related plumbing. Changes
Sequence Diagram(s)sequenceDiagram
actor Recorder as Recording Process
participant MCS as MediaCaptureService
participant Executor as Integrity<br/>Executor
participant Checker as RecordedVideo<br/>IntegrityChecker
participant Listener as MediaCapture<br/>Listener
Recorder->>MCS: stopRecording()
MCS->>MCS: add captureId to pending set
MCS->>Executor: submit verify(filePath)
Executor->>Checker: verify(filePath)
alt Integrity Check Passes
Checker-->>Executor: true
Executor->>MCS: callback on main thread
MCS->>Listener: onVideoRecordingStopped()
MCS->>MCS: sendGalleryStatusUpdate()
MCS->>MCS: uploadVideo()
MCS->>MCS: remove captureId from pending
else Integrity Check Fails
Checker-->>Executor: false
Executor->>MCS: callback on main thread
MCS->>MCS: delete corrupted file
MCS->>Listener: onMediaError(...)
MCS->>MCS: sendGalleryStatusUpdate()
MCS->>MCS: remove captureId from pending
else Exception During Check
Checker-->>Executor: throws
Executor->>MCS: callback on main thread
MCS->>Listener: onMediaError(...)
MCS->>MCS: sendGalleryStatusUpdate()
MCS->>MCS: clear pending captureId
end
sequenceDiagram
actor User as Gallery User
participant Gallery as Gallery<br/>Component
participant VideoReport as submitGallery<br/>VideoPlaybackBugReport
participant Dedup as Deduplication<br/>Registry
participant BugService as buildBugReport<br/>FeedbackDataForBug
participant Submit as submitBugIncident
participant Server as Backend Server
User->>Gallery: Play video (error)
Gallery->>VideoReport: submitGalleryVideoPlaybackBugReport(photo, error, isActive)
VideoReport->>VideoReport: serialize error
VideoReport->>VideoReport: compute dedupe key
VideoReport->>Dedup: check dedupe registry
alt Dedupe Hit (within window)
Dedup-->>VideoReport: skip reporting
else Not a dupe
Dedup-->>VideoReport: proceed
VideoReport->>BugService: buildBugReportFeedbackDataForBug(...)
BugService-->>VideoReport: feedbackData
VideoReport->>Submit: submitBugIncident(feedbackData, screenshots?)
Submit->>Server: createIncident(...)
Server-->>Submit: incidentId
Submit->>Server: upload logs & screenshots
Submit-->>VideoReport: {ok:true, incidentId}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Member
Author
|
@codex review |
![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: 9c20c97f02
ℹ️ 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".
Comment on lines
+2961
to
+2964
| videoIntegrityExecutor.shutdown(); | ||
| try { | ||
| if (!videoIntegrityExecutor.awaitTermination(3, TimeUnit.SECONDS)) { | ||
| videoIntegrityExecutor.shutdownNow(); |
There was a problem hiding this comment.
When cleanup() runs while a recording is still active, it calls stopVideoRecording(...) (which completes asynchronously via CameraNeo) and then immediately shuts down videoIntegrityExecutor. If onRecordingStopped arrives afterward, videoIntegrityExecutor.execute(...) in that callback can throw RejectedExecutionException, so the integrity path never runs and listener/gallery updates are skipped (and this can crash the callback thread). Please defer executor shutdown until outstanding stop callbacks are drained or guard execute(...) against rejection.
Useful? React with 👍 / 👎.
Comment on lines
+863
to
+865
| File bad = new File(filePath); | ||
| if (bad.exists() && !bad.delete()) { | ||
| Log.w(TAG, "Could not delete failed video file: " + filePath); |
There was a problem hiding this comment.
On integrity failure this branch deletes only the MP4 file, but the capture folder's sidecar files (for example imu.json, written before onRecordingStopped) are left behind. That leaves a partial VID_* capture that sync can still emit, and the mobile capture pipeline may treat the remaining sidecar as the capture's primary file, producing broken gallery entries/download attempts. The failure cleanup should remove the whole capture directory (or at least auxiliary files) rather than just base.mp4.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
mobile/src/services/bugReport/bugReportIncident.ts (1)
102-112: Consider consolidating repeated store access calls.Multiple sequential calls to
useGlassesStore.getState()could be consolidated into a single call for marginal efficiency and cleaner code:♻️ Suggested refactor
- const glassesConnected = useGlassesStore.getState().connected - const deviceModel = useGlassesStore.getState().deviceModel - const glassesBluetoothName = useGlassesStore.getState().bluetoothName - const buildNumber = useGlassesStore.getState().buildNumber - const glassesFwVersion = useGlassesStore.getState().fwVersion - const appVersion = useGlassesStore.getState().appVersion - const serialNumber = useGlassesStore.getState().serialNumber - const androidVersion = useGlassesStore.getState().androidVersion - const glassesWifiConnected = useGlassesStore.getState().wifiConnected - const glassesWifiSsid = useGlassesStore.getState().wifiSsid - const glassesBatteryLevel = useGlassesStore.getState().batteryLevel + const glassesState = useGlassesStore.getState() + const { + connected: glassesConnected, + deviceModel, + bluetoothName: glassesBluetoothName, + buildNumber, + fwVersion: glassesFwVersion, + appVersion, + serialNumber, + androidVersion, + wifiConnected: glassesWifiConnected, + wifiSsid: glassesWifiSsid, + batteryLevel: glassesBatteryLevel, + } = glassesState🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/src/services/bugReport/bugReportIncident.ts` around lines 102 - 112, The code repeatedly calls useGlassesStore.getState() for each value (glassesConnected, deviceModel, glassesBluetoothName, buildNumber, glassesFwVersion, appVersion, serialNumber, androidVersion, glassesWifiConnected, glassesWifiSsid, glassesBatteryLevel); replace these repeated calls by a single const state = useGlassesStore.getState() and then read state.connected, state.deviceModel, state.bluetoothName, state.buildNumber, state.fwVersion, state.appVersion, state.serialNumber, state.androidVersion, state.wifiConnected, state.wifiSsid, and state.batteryLevel to reduce redundant calls and simplify the code.mobile/src/services/bugReport/galleryVideoPlaybackBugReportCore.test.ts (1)
1-50: Good test coverage for dedupe and serialization logic.The tests appropriately cover the core helpers. Consider adding test coverage for
uriSchemeFromPlaybackUrlwhich is also exported from the core module but not tested here.📝 Suggested tests for uriSchemeFromPlaybackUrl
describe("uriSchemeFromPlaybackUrl", () => { it("returns file for file:// URLs", () => { expect(uriSchemeFromPlaybackUrl("file:///path/to/video.mp4")).toBe("file") }) it("returns file for absolute paths", () => { expect(uriSchemeFromPlaybackUrl("/var/mobile/video.mp4")).toBe("file") }) it("returns https for https URLs", () => { expect(uriSchemeFromPlaybackUrl("https://example.com/video.mp4")).toBe("https") }) it("returns http for http URLs", () => { expect(uriSchemeFromPlaybackUrl("http://example.com/video.mp4")).toBe("http") }) it("returns other for unknown schemes", () => { expect(uriSchemeFromPlaybackUrl("content://media/video")).toBe("other") }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/src/services/bugReport/galleryVideoPlaybackBugReportCore.test.ts` around lines 1 - 50, Add unit tests for the exported function uriSchemeFromPlaybackUrl: create a new describe block "uriSchemeFromPlaybackUrl" in the same test file and add cases checking that file:// URLs and absolute paths return "file", https:// returns "https", http:// returns "http", and unknown schemes (e.g., content://) return "other"; reference the function name uriSchemeFromPlaybackUrl so the tests import/consume that export from galleryVideoPlaybackBugReportCore and assert expected return values for each input.mobile/src/app/miniapps/settings/feedback.tsx (1)
150-246: Consider extracting shared system info gathering logic.The feature request branch duplicates significant logic that also exists in
buildBugReportFeedbackDataForBug: network info fetching (lines 163-173), location fetching (lines 175-201), and system info assembly. This could lead to drift over time if one path is updated but not the other.Consider extracting shared helpers (e.g.,
getNetworkInfoForFeedback(),getLocationInfoForFeedback(),buildSystemInfoForFeedback()) that both paths can use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/src/app/miniapps/settings/feedback.tsx` around lines 150 - 246, This code duplicates network/location/system-info assembly also used by buildBugReportFeedbackDataForBug; extract the shared logic into three helpers (e.g., getNetworkInfoForFeedback() that wraps NetInfo.fetch and returns {type,isConnected,isInternetReachable}, getLocationInfoForFeedback() that handles Location.getForegroundPermissionsAsync, getLastKnownPositionAsync and reverseGeocodeAsync and returns {location,locationPlace}, and buildSystemInfoForFeedback(params) that composes the systemInfo object), then replace the inline blocks in this function (where networkInfo is built from NetInfo.fetch, the try/catch Location logic, and the systemInfo assembly used to build feedbackData) to call those helpers and reuse them from buildBugReportFeedbackDataForBug; ensure helper signatures accept needed context (apps, Constants.deviceName, env vars, glasses state) and preserve all optional fields and error handling.asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.java (1)
303-309: Rename the new fields to the repo’s member convention.
videoCaptureIdsPendingIntegrityCheckandvideoIntegrityExecutorare new member fields, so they should follow the existingmCamelCaserule. As per coding guidelines,asg_client/**/*.java: "Member variables must use mCamelCase prefix convention (e.g.,mWebSocketClient)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.java` around lines 303 - 309, The two new member fields in MediaCaptureService—videoCaptureIdsPendingIntegrityCheck and videoIntegrityExecutor—do not follow the repo mCamelCase member naming convention; rename them to mVideoCaptureIdsPendingIntegrityCheck and mVideoIntegrityExecutor (or similar m-prefixed names consistent with other members) and update all references/usages in MediaCaptureService (and any inner classes or lambdas) to the new identifiers, including initializations and any places that add/remove elements or submit tasks to the executor, ensuring imports and visibility remain correct.asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/RecordedVideoIntegrityChecker.java (1)
27-31: Consider adding explicit null and blank path validation for clarity.While the current code already handles null and blank paths (via
isFile()andcanRead()returning false at line 29), explicitly checking these cases before the File constructor improves readability and provides a more specific error message:Suggested improvement
public static boolean verify(String absolutePath) { + if (absolutePath == null || absolutePath.isBlank()) { + Log.w(TAG, "Missing video path for integrity check"); + return false; + } File file = new File(absolutePath); if (!file.isFile() || !file.canRead()) { Log.w(TAG, "Not a readable file: " + absolutePath); return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/RecordedVideoIntegrityChecker.java` around lines 27 - 31, Add explicit null/blank path validation at the start of RecordedVideoIntegrityChecker.verify(String absolutePath): check if absolutePath is null or trimmed().isEmpty(), log a clear warning via TAG (e.g., "Empty or null path: " + absolutePath) and return false before constructing new File(absolutePath), so subsequent isFile()/canRead() checks run only for valid input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.java`:
- Around line 851-889: Wrap the call to videoIntegrityExecutor.execute(...) in a
try-catch that catches RejectedExecutionException (and Throwable as a fallback);
if execution is rejected, immediately remove the captureId from
videoCaptureIdsPendingIntegrityCheck, call
mMediaCaptureListener.onMediaError(pendingRequestId, "Video integrity check
could not be scheduled", MediaUploadQueueManager.MEDIA_TYPE_VIDEO) if the
listener is present, and call sendGalleryStatusUpdate() so the UI/state is not
left pending; otherwise proceed with the existing runnable as-is. Ensure you
reference the same symbols: videoIntegrityExecutor.execute,
videoCaptureIdsPendingIntegrityCheck, mMediaCaptureListener, onMediaError,
pendingRequestId, MediaUploadQueueManager.MEDIA_TYPE_VIDEO, and
sendGalleryStatusUpdate.
In
`@asg_client/app/src/main/java/com/mentra/asg_client/io/server/services/AsgCameraServer.java`:
- Around line 1194-1198: The current isActiveRecording(...) logic treats
integrity-pending captures as active (it returns true when fileCaptureId is in
activeRecordingProvider.getPendingVideoIntegrityCaptureIds()), which causes
serveSync() to skip them and advance the sync watermark (last_sync) so they
never get re-sent after validation; update the code so that either
isActiveRecording(...) does not consider pending integrity captures as "active"
for sync eligibility (remove or change the getPendingVideoIntegrityCaptureIds()
check) or change serveSync() to avoid advancing the sync watermark for files
skipped due to pending integrity; alternatively implement/consult a separate
"ready" timestamp (e.g., captureReadyTimestamp or use lastValidated time) and
only include files whose ready timestamp <= sync watermark when deciding which
files to include and what to advance. Ensure references: isActiveRecording(...),
serveSync(), activeRecordingProvider.getPendingVideoIntegrityCaptureIds(),
activeRecordingProvider.getActiveRecordingCaptureId(), fileCaptureId, and
last_sync are updated consistently so pending captures do not permanently get
excluded from future incremental syncs.
In `@mobile/src/bridge/MantleBridge.ts`:
- Around line 5-6: The MantleBridge export is untyped; add a strict type
annotation to the MantleBridge constant (e.g., annotate MantleBridge:
MantleBridgeInterface) so consumers get compile-time checks. If a MantleBridge
interface exists, import and apply it to the MantleBridge symbol; otherwise
define a minimal interface matching the expected bridge methods/events and use
that type on MantleBridge before exporting. Ensure the name MantleBridge is used
exactly when annotating so the stub conforms to the project’s interface
expectations.
---
Nitpick comments:
In
`@asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.java`:
- Around line 303-309: The two new member fields in
MediaCaptureService—videoCaptureIdsPendingIntegrityCheck and
videoIntegrityExecutor—do not follow the repo mCamelCase member naming
convention; rename them to mVideoCaptureIdsPendingIntegrityCheck and
mVideoIntegrityExecutor (or similar m-prefixed names consistent with other
members) and update all references/usages in MediaCaptureService (and any inner
classes or lambdas) to the new identifiers, including initializations and any
places that add/remove elements or submit tasks to the executor, ensuring
imports and visibility remain correct.
In
`@asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/RecordedVideoIntegrityChecker.java`:
- Around line 27-31: Add explicit null/blank path validation at the start of
RecordedVideoIntegrityChecker.verify(String absolutePath): check if absolutePath
is null or trimmed().isEmpty(), log a clear warning via TAG (e.g., "Empty or
null path: " + absolutePath) and return false before constructing new
File(absolutePath), so subsequent isFile()/canRead() checks run only for valid
input.
In `@mobile/src/app/miniapps/settings/feedback.tsx`:
- Around line 150-246: This code duplicates network/location/system-info
assembly also used by buildBugReportFeedbackDataForBug; extract the shared logic
into three helpers (e.g., getNetworkInfoForFeedback() that wraps NetInfo.fetch
and returns {type,isConnected,isInternetReachable}, getLocationInfoForFeedback()
that handles Location.getForegroundPermissionsAsync, getLastKnownPositionAsync
and reverseGeocodeAsync and returns {location,locationPlace}, and
buildSystemInfoForFeedback(params) that composes the systemInfo object), then
replace the inline blocks in this function (where networkInfo is built from
NetInfo.fetch, the try/catch Location logic, and the systemInfo assembly used to
build feedbackData) to call those helpers and reuse them from
buildBugReportFeedbackDataForBug; ensure helper signatures accept needed context
(apps, Constants.deviceName, env vars, glasses state) and preserve all optional
fields and error handling.
In `@mobile/src/services/bugReport/bugReportIncident.ts`:
- Around line 102-112: The code repeatedly calls useGlassesStore.getState() for
each value (glassesConnected, deviceModel, glassesBluetoothName, buildNumber,
glassesFwVersion, appVersion, serialNumber, androidVersion,
glassesWifiConnected, glassesWifiSsid, glassesBatteryLevel); replace these
repeated calls by a single const state = useGlassesStore.getState() and then
read state.connected, state.deviceModel, state.bluetoothName, state.buildNumber,
state.fwVersion, state.appVersion, state.serialNumber, state.androidVersion,
state.wifiConnected, state.wifiSsid, and state.batteryLevel to reduce redundant
calls and simplify the code.
In `@mobile/src/services/bugReport/galleryVideoPlaybackBugReportCore.test.ts`:
- Around line 1-50: Add unit tests for the exported function
uriSchemeFromPlaybackUrl: create a new describe block "uriSchemeFromPlaybackUrl"
in the same test file and add cases checking that file:// URLs and absolute
paths return "file", https:// returns "https", http:// returns "http", and
unknown schemes (e.g., content://) return "other"; reference the function name
uriSchemeFromPlaybackUrl so the tests import/consume that export from
galleryVideoPlaybackBugReportCore and assert expected return values for each
input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4966165d-e8d0-4400-81c4-e4c2656648ac
📒 Files selected for processing (13)
asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.javaasg_client/app/src/main/java/com/mentra/asg_client/io/media/core/RecordedVideoIntegrityChecker.javaasg_client/app/src/main/java/com/mentra/asg_client/io/server/services/AsgCameraServer.javaasg_client/app/src/main/java/com/mentra/asg_client/service/legacy/managers/AsgClientServiceManager.javamobile/src/app/miniapps/settings/feedback.tsxmobile/src/bridge/MantleBridge.tsmobile/src/components/glasses/Gallery/AwesomeGalleryViewer.tsxmobile/src/services/bugReport/bugReportIncident.test.tsmobile/src/services/bugReport/bugReportIncident.tsmobile/src/services/bugReport/galleryVideoPlaybackBugReport.integration.test.tsmobile/src/services/bugReport/galleryVideoPlaybackBugReport.tsmobile/src/services/bugReport/galleryVideoPlaybackBugReportCore.test.tsmobile/src/services/bugReport/galleryVideoPlaybackBugReportCore.ts
Comment on lines
+851
to
+889
| videoIntegrityExecutor.execute(() -> { | ||
| try { | ||
| final boolean ok = RecordedVideoIntegrityChecker.verify(filePath); | ||
| mainHandler.post(() -> { | ||
| videoCaptureIdsPendingIntegrityCheck.remove(captureId); | ||
| if (ok) { | ||
| if (mMediaCaptureListener != null) { | ||
| mMediaCaptureListener.onVideoRecordingStopped(pendingRequestId, filePath); | ||
| } | ||
| sendGalleryStatusUpdate(); | ||
| uploadVideo(filePath, pendingRequestId); | ||
| } else { | ||
| File bad = new File(filePath); | ||
| if (bad.exists() && !bad.delete()) { | ||
| Log.w(TAG, "Could not delete failed video file: " + filePath); | ||
| } | ||
| if (mMediaCaptureListener != null) { | ||
| mMediaCaptureListener.onMediaError( | ||
| pendingRequestId, | ||
| "Video file failed integrity check and was removed", | ||
| MediaUploadQueueManager.MEDIA_TYPE_VIDEO); | ||
| } | ||
| sendGalleryStatusUpdate(); | ||
| } | ||
| }); | ||
| } catch (Throwable t) { | ||
| Log.e(TAG, "Unexpected error during video integrity check", t); | ||
| mainHandler.post(() -> { | ||
| videoCaptureIdsPendingIntegrityCheck.remove(captureId); | ||
| if (mMediaCaptureListener != null) { | ||
| mMediaCaptureListener.onMediaError( | ||
| pendingRequestId, | ||
| "Video integrity check error: " + t.getMessage(), | ||
| MediaUploadQueueManager.MEDIA_TYPE_VIDEO); | ||
| } | ||
| sendGalleryStatusUpdate(); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.java | head -100Repository: Mentra-Community/MentraOS
Length of output: 5103
🏁 Script executed:
wc -l asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.javaRepository: Mentra-Community/MentraOS
Length of output: 163
🏁 Script executed:
# Search for cleanup() method and onRecordingStopped() method
rg -n "void cleanup\(\)|void onRecordingStopped\(" asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.javaRepository: Mentra-Community/MentraOS
Length of output: 187
🏁 Script executed:
# Search for videoIntegrityExecutor initialization and shutdown
rg -n "videoIntegrityExecutor" asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.javaRepository: Mentra-Community/MentraOS
Length of output: 499
🏁 Script executed:
sed -n '2931,2980p' asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.javaRepository: Mentra-Community/MentraOS
Length of output: 1771
🏁 Script executed:
sed -n '827,870p' asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.javaRepository: Mentra-Community/MentraOS
Length of output: 2656
🏁 Script executed:
# Check context around line 305 for executor initialization
sed -n '305,320p' asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.javaRepository: Mentra-Community/MentraOS
Length of output: 712
🏁 Script executed:
# Check if there are any synchronization mechanisms or lifecycle management
rg -n "synchronized|volatile|AtomicBoolean|lock|mutex" asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.java | head -30Repository: Mentra-Community/MentraOS
Length of output: 1210
Wrap executor submission in try-catch to handle shutdown race condition.
cleanup() shuts down videoIntegrityExecutor, but onRecordingStopped() can still be called afterward due to asynchronous callback delivery. If the executor is shut down, execute() throws RejectedExecutionException before the pending capture ID is cleared or the listener is notified, leaving the recording stranded in the "pending" state and potentially hanging the UI.
Proposed fix
- videoIntegrityExecutor.execute(() -> {
- try {
- final boolean ok = RecordedVideoIntegrityChecker.verify(filePath);
- mainHandler.post(() -> {
- videoCaptureIdsPendingIntegrityCheck.remove(captureId);
- if (ok) {
- if (mMediaCaptureListener != null) {
- mMediaCaptureListener.onVideoRecordingStopped(pendingRequestId, filePath);
- }
- sendGalleryStatusUpdate();
- uploadVideo(filePath, pendingRequestId);
- } else {
- File bad = new File(filePath);
- if (bad.exists() && !bad.delete()) {
- Log.w(TAG, "Could not delete failed video file: " + filePath);
- }
- if (mMediaCaptureListener != null) {
- mMediaCaptureListener.onMediaError(
- pendingRequestId,
- "Video file failed integrity check and was removed",
- MediaUploadQueueManager.MEDIA_TYPE_VIDEO);
- }
- sendGalleryStatusUpdate();
- }
- });
- } catch (Throwable t) {
- Log.e(TAG, "Unexpected error during video integrity check", t);
- mainHandler.post(() -> {
- videoCaptureIdsPendingIntegrityCheck.remove(captureId);
- if (mMediaCaptureListener != null) {
- mMediaCaptureListener.onMediaError(
- pendingRequestId,
- "Video integrity check error: " + t.getMessage(),
- MediaUploadQueueManager.MEDIA_TYPE_VIDEO);
- }
- sendGalleryStatusUpdate();
- });
- }
- });
+ try {
+ videoIntegrityExecutor.execute(() -> {
+ try {
+ final boolean ok = RecordedVideoIntegrityChecker.verify(filePath);
+ mainHandler.post(() -> {
+ videoCaptureIdsPendingIntegrityCheck.remove(captureId);
+ if (ok) {
+ if (mMediaCaptureListener != null) {
+ mMediaCaptureListener.onVideoRecordingStopped(pendingRequestId, filePath);
+ }
+ sendGalleryStatusUpdate();
+ uploadVideo(filePath, pendingRequestId);
+ } else {
+ File bad = new File(filePath);
+ if (bad.exists() && !bad.delete()) {
+ Log.w(TAG, "Could not delete failed video file: " + filePath);
+ }
+ if (mMediaCaptureListener != null) {
+ mMediaCaptureListener.onMediaError(
+ pendingRequestId,
+ "Video file failed integrity check and was removed",
+ MediaUploadQueueManager.MEDIA_TYPE_VIDEO);
+ }
+ sendGalleryStatusUpdate();
+ }
+ });
+ } catch (Throwable t) {
+ Log.e(TAG, "Unexpected error during video integrity check", t);
+ mainHandler.post(() -> {
+ videoCaptureIdsPendingIntegrityCheck.remove(captureId);
+ if (mMediaCaptureListener != null) {
+ mMediaCaptureListener.onMediaError(
+ pendingRequestId,
+ "Video integrity check error: " + t.getMessage(),
+ MediaUploadQueueManager.MEDIA_TYPE_VIDEO);
+ }
+ sendGalleryStatusUpdate();
+ });
+ }
+ });
+ } catch (java.util.concurrent.RejectedExecutionException e) {
+ videoCaptureIdsPendingIntegrityCheck.remove(captureId);
+ Log.w(TAG, "Integrity check rejected after shutdown: " + filePath, e);
+ if (mMediaCaptureListener != null) {
+ mMediaCaptureListener.onMediaError(
+ pendingRequestId,
+ "Video integrity check skipped during shutdown",
+ MediaUploadQueueManager.MEDIA_TYPE_VIDEO);
+ }
+ sendGalleryStatusUpdate();
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@asg_client/app/src/main/java/com/mentra/asg_client/io/media/core/MediaCaptureService.java`
around lines 851 - 889, Wrap the call to videoIntegrityExecutor.execute(...) in
a try-catch that catches RejectedExecutionException (and Throwable as a
fallback); if execution is rejected, immediately remove the captureId from
videoCaptureIdsPendingIntegrityCheck, call
mMediaCaptureListener.onMediaError(pendingRequestId, "Video integrity check
could not be scheduled", MediaUploadQueueManager.MEDIA_TYPE_VIDEO) if the
listener is present, and call sendGalleryStatusUpdate() so the UI/state is not
left pending; otherwise proceed with the existing runnable as-is. Ensure you
reference the same symbols: videoIntegrityExecutor.execute,
videoCaptureIdsPendingIntegrityCheck, mMediaCaptureListener, onMediaError,
pendingRequestId, MediaUploadQueueManager.MEDIA_TYPE_VIDEO, and
sendGalleryStatusUpdate.
Comment on lines
+1194
to
+1198
| String activeCaptureId = activeRecordingProvider.getActiveRecordingCaptureId(); | ||
| if (activeCaptureId != null && activeCaptureId.equals(fileCaptureId)) { | ||
| return true; | ||
| } | ||
| return activeRecordingProvider.getPendingVideoIntegrityCaptureIds().contains(fileCaptureId); |
There was a problem hiding this comment.
This can permanently hide validated videos from incremental sync.
After isActiveRecording(...) starts returning true for integrity-pending captures, serveSync() skips those files while still letting the client advance last_sync. Once validation finishes, the file's lastModified is unchanged, so later syncs never include it. Please keep skipped captures out of the sync watermark, or track a separate "ready" timestamp for sync eligibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@asg_client/app/src/main/java/com/mentra/asg_client/io/server/services/AsgCameraServer.java`
around lines 1194 - 1198, The current isActiveRecording(...) logic treats
integrity-pending captures as active (it returns true when fileCaptureId is in
activeRecordingProvider.getPendingVideoIntegrityCaptureIds()), which causes
serveSync() to skip them and advance the sync watermark (last_sync) so they
never get re-sent after validation; update the code so that either
isActiveRecording(...) does not consider pending integrity captures as "active"
for sync eligibility (remove or change the getPendingVideoIntegrityCaptureIds()
check) or change serveSync() to avoid advancing the sync watermark for files
skipped due to pending integrity; alternatively implement/consult a separate
"ready" timestamp (e.g., captureReadyTimestamp or use lastValidated time) and
only include files whose ready timestamp <= sync watermark when deciding which
files to include and what to advance. Ensure references: isActiveRecording(...),
serveSync(), activeRecordingProvider.getPendingVideoIntegrityCaptureIds(),
activeRecordingProvider.getActiveRecordingCaptureId(), fileCaptureId, and
last_sync are updated consistently so pending captures do not permanently get
excluded from future incremental syncs.
Comment on lines
+5
to
+6
| const MantleBridge = {} | ||
| export default MantleBridge |
There was a problem hiding this comment.
Missing type annotation violates strict typing guideline.
The MantleBridge constant lacks a type annotation. Per coding guidelines, TypeScript code should use strict typing with interfaces. Without a defined type, TypeScript cannot verify that code importing this stub is compatible with the actual bridge interface (if one exists).
🔧 Suggested fix to add type annotation
If a MantleBridge interface exists elsewhere, import and use it:
+import type {MantleBridge as IMantleBridge} from './MantleBridge.types'
+
-const MantleBridge = {}
+const MantleBridge: IMantleBridge = {} as IMantleBridge
export default MantleBridgeOtherwise, define an explicit type for the stub:
-const MantleBridge = {}
+const MantleBridge: Record<string, never> = {}
export default MantleBridgeAs per coding guidelines: "Use strict typing with interfaces for message types in TypeScript".
📝 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.
Suggested change
| const MantleBridge = {} | |
| export default MantleBridge | |
| const MantleBridge: Record<string, never> = {} | |
| export default MantleBridge |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/src/bridge/MantleBridge.ts` around lines 5 - 6, The MantleBridge
export is untyped; add a strict type annotation to the MantleBridge constant
(e.g., annotate MantleBridge: MantleBridgeInterface) so consumers get
compile-time checks. If a MantleBridge interface exists, import and apply it to
the MantleBridge symbol; otherwise define a minimal interface matching the
expected bridge methods/events and use that type on MantleBridge before
exporting. Ensure the name MantleBridge is used exactly when annotating so the
stub conforms to the project’s interface expectations.
added 2 commits
April 15, 2026 16:13
Deploying mentra-live-ota-site with
|
| Latest commit: |
0a19c54
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://927b771f.mentra-live-ota-site.pages.dev |
| Branch Preview URL: | https://mentra-live-logging.mentra-live-ota-site.pages.dev |
Deploying mentra-store-dev with
|
| Latest commit: |
0a19c54
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ada4ec59.augmentos-appstore-2.pages.dev |
| Branch Preview URL: | https://mentra-live-logging.augmentos-appstore-2.pages.dev |
Deploying dev-augmentos-console with
|
| Latest commit: |
0a19c54
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://27f4d6a2.dev-augmentos-console.pages.dev |
| Branch Preview URL: | https://mentra-live-logging.dev-augmentos-console.pages.dev |
This reverts commit 5ca5238.
Deploying prod-augmentos-account with
|
| Latest commit: |
0a19c54
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4ce19bdb.augmentos-e84.pages.dev |
| Branch Preview URL: | https://mentra-live-logging.augmentos-e84.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mobile/src/app/miniapps/settings/feedback.tsx (2)
125-150:⚠️ Potential issue | 🟠 MajorWrap the submit flow in
try/catch/finally.These awaits can throw before the explicit
!ok/is_error()checks run. If that happens,isSubmittingnever resets and the screen stays disabled with the spinner showing.💡 Suggested structure
const handleSubmitFeedback = async () => { setIsSubmitting(true) - - // existing submission logic - ... - - setIsSubmitting(false) + try { + // existing submission logic + ... + } catch (error) { + console.error('Error submitting feedback:', error) + showAlert(translate('common:error'), translate('feedback:errorSendingFeedback'), [ + {text: translate('common:ok')}, + ]) + } finally { + setIsSubmitting(false) + } }As per coding guidelines, "Use try/catch with meaningful error messages in TypeScript".
Also applies to: 151-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/src/app/miniapps/settings/feedback.tsx` around lines 125 - 150, The submit flow around buildBugReportFeedbackDataForBug and submitBugIncident should be wrapped in a try/catch/finally so thrown exceptions don't leave the UI stuck; move the awaits for buildBugReportFeedbackDataForBug and submitBugIncident into a try block, handle errors in catch by logging the error and calling showAlert (similar to the existing !submitRes.ok path) and ensure setIsSubmitting(false) is always called in finally; reference the existing functions/variables buildBugReportFeedbackDataForBug, submitBugIncident, setIsSubmitting, showAlert and translate to implement the error handling and cleanup.
355-379:⚠️ Potential issue | 🟡 MinorApple private relay users now get the email field twice.
The generic email input above already binds to
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/src/app/miniapps/settings/feedback.tsx` around lines 355 - 379, The UI renders a duplicate email input when isApplePrivateRelay is true because the conditional block reuses the same email state (email, setEmail); remove the duplicate TextInput (and its surrounding View if desired) inside the isApplePrivateRelay conditional and keep only the informational label/Pressable (or replace the TextInput with non-editable info) so there is a single email control bound to email; update references in the block (isApplePrivateRelay, email, setEmail, TextInput, Pressable, Icon, translate) accordingly.
♻️ Duplicate comments (1)
asg_client/app/src/main/java/com/mentra/asg_client/io/server/services/AsgCameraServer.java (1)
1203-1207:⚠️ Potential issue | 🟠 MajorPending-integrity files can still be permanently skipped in incremental sync.
Because pending-integrity captures are treated as “active” here,
serveSync()skips them at Line 1269-1271 while clients can still advancelast_syncusing the response timestamp (Line 1458). When integrity validation later completes without updating file mtime, those files no longer satisfy thelastModified > lastSyncTimefilter and never get sent.Please decouple pending-integrity gating from sync watermark progression (e.g., separate “ready” timestamp/watermark strategy or avoid advancing watermark past skipped-pending captures).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asg_client/app/src/main/java/com/mentra/asg_client/io/server/services/AsgCameraServer.java` around lines 1203 - 1207, The current check in AsgCameraServer that treats pending-integrity captures the same as active ones (using activeRecordingProvider.getActiveRecordingCaptureId() and getPendingVideoIntegrityCaptureIds()) causes serveSync to skip pending files while still allowing last_sync to advance; change the gating so pending-integrity captures are NOT treated as “active” for watermark progression: keep getActiveRecordingCaptureId() as the sole determinant of active captures, add a separate isPendingVideoIntegrityCapture(captureId) predicate and use it only to skip serving the file, and when advancing last_sync in serveSync compute the new watermark from only served/ready files (or maintain a separate “ready” timestamp/watermark) so the response timestamp never advances past skipped pending captures. Ensure any helper method that currently mixes active+pending semantics (the snippet returning true for pending IDs) is split into distinct checks used appropriately.
🧹 Nitpick comments (1)
mobile/modules/core/android/src/main/java/com/mentra/core/CoreModule.kt (1)
515-605: Code duplication with TestAudioReceiver.kt.The WAV parsing and stereo-to-mono conversion logic is nearly identical between this method and
TestAudioReceiver.injectAudioFromFile(). Consider extracting this into a shared utility class (e.g.,WavAudioInjector) to avoid maintaining two copies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/modules/core/android/src/main/java/com/mentra/core/CoreModule.kt` around lines 515 - 605, The injectTestAudioFromFile function duplicates WAV parsing and stereo-to-mono logic found in TestAudioReceiver.injectAudioFromFile(); extract the shared parsing and chunking logic into a new utility (e.g., class WavAudioInjector with methods parseWav(filePath): ParsedWav and nextMonoChunk(parsedWav, chunkSize): ByteArray?) and replace the inline logic in CoreModule.injectTestAudioFromFile and TestAudioReceiver.injectAudioFromFile to call WavAudioInjector.parseWav to validate header/sample format, locate "data" chunk and produce pcmData, then use a shared chunking/stereo-to-mono helper to produce mono chunks before calling coreManager?.handlePcm; ensure WavAudioInjector validates bitsPerSample/sampleRate/numChannels and exposes chunkDelay/mono conversion so both callers reuse identical behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mobile/modules/core/android/src/main/java/com/mentra/core/CoreModule.kt`:
- Around line 552-563: The code finds the "data" chunk but ignores its parsed
size; update the logic so when chunkId == "data" you compute the PCM start as
dataStart = dataOffset + 8 and the PCM end as dataEnd = min(dataStart +
chunkSize, wavData.size) and then set pcmData = wavData.sliceArray(dataStart
until dataEnd); use dataOffset, chunkSize, chunkId, pcmData, wavData and
byteBuffer to locate and bound the slice and keep the existing loop/break
behavior to stop after finding the data chunk.
- Line 600: The duration calculation uses a hardcoded divisor of 32 which
assumes mono audio; update the map entry that returns "durationMs" (where
pcmData.size is used) to account for numChannels by replacing the hardcoded 32
with 32 * numChannels (i.e., "durationMs" to (pcmData.size / (32 *
numChannels))) so stereo/interleaved samples are calculated correctly using the
existing numChannels value in CoreModule.kt.
In
`@mobile/modules/core/android/src/main/java/com/mentra/core/testing/TestAudioReceiver.kt`:
- Around line 88-100: The loop in TestAudioReceiver.kt finds the "data" chunk
but then ignores chunkSize and slices pcmData to the EOF; update the pcmData
extraction to use the parsed chunkSize (and ensure bounds safety) by replacing
the final sliceArray call to sliceArray(dataOffset until minOf(dataOffset +
chunkSize, wavData.size)), keeping the existing variables (dataOffset,
chunkSize, chunkId, byteBuffer, wavData) and preserving the break when "data" is
found; also add a guard if dataOffset + chunkSize > wavData.size to avoid
out-of-bounds and handle that error case appropriately.
- Around line 56-59: The FileInputStream in TestAudioReceiver.kt is opened and
closed manually which leaks if readBytes() throws; replace the manual
open/read/close with Kotlin's use extension so the stream is closed
automatically (e.g., obtain the stream with java.io.FileInputStream or
file.inputStream() and call use to read into wavData), updating the wavData
assignment to come from the lambda result and removing the explicit close call.
- Around line 9-18: The comment on TestAudioReceiver claims it's "only active in
debug builds" but the receiver is registered unconditionally in the manifest;
either update the docstring to remove the misleading claim or enforce it by
adding a runtime check in TestAudioReceiver.onReceive() that returns immediately
when BuildConfig.DEBUG is false (and optionally log a warning). Locate the
TestAudioReceiver class and its onReceive method and implement the
BuildConfig.DEBUG guard (or revise the header comment) so the behavior matches
the documentation.
In `@mobile/src/app/miniapps/settings/feedback.tsx`:
- Around line 132-133: The code currently logs the full feedbackData object and
the REST URL; remove the console.log that prints JSON.stringify(feedbackData)
and instead log only non-sensitive, minimal info (e.g., "Feedback submitted"
with a boolean or an id) or a redacted summary by explicitly picking safe
fields; keep or log useSettingsStore.getState().getRestUrl() if needed but avoid
pairing it with any user/device identifiers; apply the same change to the other
occurrence where feedbackData is logged (the second console.log at the noted
location).
- Around line 332-352: The UI that allowed changing feedbackType was removed,
leaving feedbackType initialized to 'bug' and never updated so the
feature-request path (sendFeedback(...) and the app-rating prompt) is
unreachable; reintroduce a control (e.g., segmented control, buttons, or
Pressables) that updates the feedbackType state via its setter (setFeedbackType)
so users can select 'feature' (or other kinds), or change the initial state to a
neutral value and handle selection elsewhere, and ensure the code paths in
sendFeedback(...) that check feedbackType can be exercised when the user selects
'feature'.
---
Outside diff comments:
In `@mobile/src/app/miniapps/settings/feedback.tsx`:
- Around line 125-150: The submit flow around buildBugReportFeedbackDataForBug
and submitBugIncident should be wrapped in a try/catch/finally so thrown
exceptions don't leave the UI stuck; move the awaits for
buildBugReportFeedbackDataForBug and submitBugIncident into a try block, handle
errors in catch by logging the error and calling showAlert (similar to the
existing !submitRes.ok path) and ensure setIsSubmitting(false) is always called
in finally; reference the existing functions/variables
buildBugReportFeedbackDataForBug, submitBugIncident, setIsSubmitting, showAlert
and translate to implement the error handling and cleanup.
- Around line 355-379: The UI renders a duplicate email input when
isApplePrivateRelay is true because the conditional block reuses the same email
state (email, setEmail); remove the duplicate TextInput (and its surrounding
View if desired) inside the isApplePrivateRelay conditional and keep only the
informational label/Pressable (or replace the TextInput with non-editable info)
so there is a single email control bound to email; update references in the
block (isApplePrivateRelay, email, setEmail, TextInput, Pressable, Icon,
translate) accordingly.
---
Duplicate comments:
In
`@asg_client/app/src/main/java/com/mentra/asg_client/io/server/services/AsgCameraServer.java`:
- Around line 1203-1207: The current check in AsgCameraServer that treats
pending-integrity captures the same as active ones (using
activeRecordingProvider.getActiveRecordingCaptureId() and
getPendingVideoIntegrityCaptureIds()) causes serveSync to skip pending files
while still allowing last_sync to advance; change the gating so
pending-integrity captures are NOT treated as “active” for watermark
progression: keep getActiveRecordingCaptureId() as the sole determinant of
active captures, add a separate isPendingVideoIntegrityCapture(captureId)
predicate and use it only to skip serving the file, and when advancing last_sync
in serveSync compute the new watermark from only served/ready files (or maintain
a separate “ready” timestamp/watermark) so the response timestamp never advances
past skipped pending captures. Ensure any helper method that currently mixes
active+pending semantics (the snippet returning true for pending IDs) is split
into distinct checks used appropriately.
---
Nitpick comments:
In `@mobile/modules/core/android/src/main/java/com/mentra/core/CoreModule.kt`:
- Around line 515-605: The injectTestAudioFromFile function duplicates WAV
parsing and stereo-to-mono logic found in
TestAudioReceiver.injectAudioFromFile(); extract the shared parsing and chunking
logic into a new utility (e.g., class WavAudioInjector with methods
parseWav(filePath): ParsedWav and nextMonoChunk(parsedWav, chunkSize):
ByteArray?) and replace the inline logic in CoreModule.injectTestAudioFromFile
and TestAudioReceiver.injectAudioFromFile to call WavAudioInjector.parseWav to
validate header/sample format, locate "data" chunk and produce pcmData, then use
a shared chunking/stereo-to-mono helper to produce mono chunks before calling
coreManager?.handlePcm; ensure WavAudioInjector validates
bitsPerSample/sampleRate/numChannels and exposes chunkDelay/mono conversion so
both callers reuse identical behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae5d1fc3-cabb-445f-b0ea-bd916efb39ca
📒 Files selected for processing (7)
asg_client/app/src/main/java/com/mentra/asg_client/io/server/services/AsgCameraServer.javamobile/modules/core/android/src/main/AndroidManifest.xmlmobile/modules/core/android/src/main/java/com/mentra/core/CoreModule.ktmobile/modules/core/android/src/main/java/com/mentra/core/testing/TestAudioReceiver.ktmobile/modules/core/src/CoreModule.tsmobile/src/app/miniapps/settings/feedback.tsxmobile/src/components/mirror/GlassesDisplayMirror.tsx
✅ Files skipped from review due to trivial changes (1)
- mobile/src/components/mirror/GlassesDisplayMirror.tsx
Comment on lines
+552
to
+563
| var dataOffset = 12 | ||
| while (dataOffset < wavData.size - 8) { | ||
| val chunkId = String(wavData.sliceArray(dataOffset until dataOffset + 4)) | ||
| val chunkSize = byteBuffer.getInt(dataOffset + 4) | ||
| if (chunkId == "data") { | ||
| dataOffset += 8 | ||
| break | ||
| } | ||
| dataOffset += 8 + chunkSize | ||
| } | ||
|
|
||
| val pcmData = wavData.sliceArray(dataOffset until wavData.size) |
There was a problem hiding this comment.
Data chunk size from header is ignored.
The code parses chunkSize from the WAV data chunk header (Line 555) but doesn't use it to limit pcmData. Instead, it reads from dataOffset to end-of-file (Line 563), which may include trailing metadata or padding in some WAV files. Consider using the parsed chunk size:
Proposed fix
+ var dataChunkSize = 0
var dataOffset = 12
while (dataOffset < wavData.size - 8) {
val chunkId = String(wavData.sliceArray(dataOffset until dataOffset + 4))
val chunkSize = byteBuffer.getInt(dataOffset + 4)
if (chunkId == "data") {
+ dataChunkSize = chunkSize
dataOffset += 8
break
}
dataOffset += 8 + chunkSize
}
- val pcmData = wavData.sliceArray(dataOffset until wavData.size)
+ val dataEnd = minOf(dataOffset + dataChunkSize, wavData.size)
+ val pcmData = wavData.sliceArray(dataOffset until dataEnd)📝 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.
Suggested change
| var dataOffset = 12 | |
| while (dataOffset < wavData.size - 8) { | |
| val chunkId = String(wavData.sliceArray(dataOffset until dataOffset + 4)) | |
| val chunkSize = byteBuffer.getInt(dataOffset + 4) | |
| if (chunkId == "data") { | |
| dataOffset += 8 | |
| break | |
| } | |
| dataOffset += 8 + chunkSize | |
| } | |
| val pcmData = wavData.sliceArray(dataOffset until wavData.size) | |
| var dataChunkSize = 0 | |
| var dataOffset = 12 | |
| while (dataOffset < wavData.size - 8) { | |
| val chunkId = String(wavData.sliceArray(dataOffset until dataOffset + 4)) | |
| val chunkSize = byteBuffer.getInt(dataOffset + 4) | |
| if (chunkId == "data") { | |
| dataChunkSize = chunkSize | |
| dataOffset += 8 | |
| break | |
| } | |
| dataOffset += 8 + chunkSize | |
| } | |
| val dataEnd = minOf(dataOffset + dataChunkSize, wavData.size) | |
| val pcmData = wavData.sliceArray(dataOffset until dataEnd) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/modules/core/android/src/main/java/com/mentra/core/CoreModule.kt`
around lines 552 - 563, The code finds the "data" chunk but ignores its parsed
size; update the logic so when chunkId == "data" you compute the PCM start as
dataStart = dataOffset + 8 and the PCM end as dataEnd = min(dataStart +
chunkSize, wavData.size) and then set pcmData = wavData.sliceArray(dataStart
until dataEnd); use dataOffset, chunkSize, chunkId, pcmData, wavData and
byteBuffer to locate and bound the slice and keep the existing loop/break
behavior to stop after finding the data chunk.
| android.util.Log.d("CoreModule", "Test audio injection complete: $chunkCount chunks sent") | ||
| }.start() | ||
|
|
||
| mapOf("success" to true, "pcmBytes" to pcmData.size, "durationMs" to (pcmData.size / 32)) |
There was a problem hiding this comment.
Duration calculation incorrect for stereo files.
The duration formula pcmData.size / 32 assumes mono input. For stereo WAV files, pcmData contains interleaved L/R samples (4 bytes per sample pair at 16-bit), so the divisor should be 64, not 32. Since the code already knows numChannels, use it:
Proposed fix
- mapOf("success" to true, "pcmBytes" to pcmData.size, "durationMs" to (pcmData.size / 32))
+ val bytesPerMsStereoAdjusted = 32 * numChannels // 32 for mono, 64 for stereo
+ mapOf("success" to true, "pcmBytes" to pcmData.size, "durationMs" to (pcmData.size / bytesPerMsStereoAdjusted))📝 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.
Suggested change
| mapOf("success" to true, "pcmBytes" to pcmData.size, "durationMs" to (pcmData.size / 32)) | |
| val bytesPerMsStereoAdjusted = 32 * numChannels // 32 for mono, 64 for stereo | |
| mapOf("success" to true, "pcmBytes" to pcmData.size, "durationMs" to (pcmData.size / bytesPerMsStereoAdjusted)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/modules/core/android/src/main/java/com/mentra/core/CoreModule.kt` at
line 600, The duration calculation uses a hardcoded divisor of 32 which assumes
mono audio; update the map entry that returns "durationMs" (where pcmData.size
is used) to account for numChannels by replacing the hardcoded 32 with 32 *
numChannels (i.e., "durationMs" to (pcmData.size / (32 * numChannels))) so
stereo/interleaved samples are calculated correctly using the existing
numChannels value in CoreModule.kt.
Comment on lines
+9
to
+18
| /** | ||
| * BroadcastReceiver for E2E testing that allows injecting test audio via ADB. | ||
| * | ||
| * Usage from command line: | ||
| * adb shell am broadcast -a com.mentra.TEST_INJECT_AUDIO \ | ||
| * --es filePath "/sdcard/Download/mentra-test/hello-world.wav" \ | ||
| * -n com.mentra.mentra/com.mentra.core.testing.TestAudioReceiver | ||
| * | ||
| * This receiver is only active in debug builds. | ||
| */ |
There was a problem hiding this comment.
Misleading comment: "only active in debug builds" is not enforced.
The docstring claims the receiver is "only active in debug builds," but there's no mechanism to enforce this. The manifest registers it unconditionally. Either:
- Update the comment to reflect reality, or
- Add a
BuildConfig.DEBUGcheck inonReceive()to enforce the constraint
Option 2: Add debug check
override fun onReceive(context: Context, intent: Intent) {
+ if (!com.mentra.core.BuildConfig.DEBUG) {
+ Log.w(TAG, "TestAudioReceiver is disabled in release builds")
+ return
+ }
+
if (intent.action != ACTION_INJECT_AUDIO) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/modules/core/android/src/main/java/com/mentra/core/testing/TestAudioReceiver.kt`
around lines 9 - 18, The comment on TestAudioReceiver claims it's "only active
in debug builds" but the receiver is registered unconditionally in the manifest;
either update the docstring to remove the misleading claim or enforce it by
adding a runtime check in TestAudioReceiver.onReceive() that returns immediately
when BuildConfig.DEBUG is false (and optionally log a warning). Locate the
TestAudioReceiver class and its onReceive method and implement the
BuildConfig.DEBUG guard (or revise the header comment) so the behavior matches
the documentation.
Comment on lines
+88
to
+100
| // Find data chunk | ||
| var dataOffset = 12 | ||
| while (dataOffset < wavData.size - 8) { | ||
| val chunkId = String(wavData.sliceArray(dataOffset until dataOffset + 4)) | ||
| val chunkSize = byteBuffer.getInt(dataOffset + 4) | ||
| if (chunkId == "data") { | ||
| dataOffset += 8 | ||
| break | ||
| } | ||
| dataOffset += 8 + chunkSize | ||
| } | ||
|
|
||
| val pcmData = wavData.sliceArray(dataOffset until wavData.size) |
There was a problem hiding this comment.
Data chunk size from header is ignored (same issue as CoreModule.kt).
The parsed chunkSize at Line 92 is not used to limit the PCM data extraction. Line 100 reads from dataOffset to end-of-file, potentially including non-audio data. Apply the same fix suggested for CoreModule.kt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/modules/core/android/src/main/java/com/mentra/core/testing/TestAudioReceiver.kt`
around lines 88 - 100, The loop in TestAudioReceiver.kt finds the "data" chunk
but then ignores chunkSize and slices pcmData to the EOF; update the pcmData
extraction to use the parsed chunkSize (and ensure bounds safety) by replacing
the final sliceArray call to sliceArray(dataOffset until minOf(dataOffset +
chunkSize, wavData.size)), keeping the existing variables (dataOffset,
chunkSize, chunkId, byteBuffer, wavData) and preserving the break when "data" is
found; also add a guard if dataOffset + chunkSize > wavData.size to avoid
out-of-bounds and handle that error case appropriately.
Comment on lines
+132
to
+133
| console.log("Feedback submitted:", JSON.stringify(feedbackData, null, 2)) | ||
| console.log("Phone backend URL (incident creation):", useSettingsStore.getState().getRestUrl()) |
There was a problem hiding this comment.
Don't log the full feedback payload.
feedbackData contains user/device metadata such as contact email, location, serial number, and Wi‑Fi SSID. Dumping the whole object to client logs is a privacy/compliance risk.
Also applies to: 248-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/src/app/miniapps/settings/feedback.tsx` around lines 132 - 133, The
code currently logs the full feedbackData object and the REST URL; remove the
console.log that prints JSON.stringify(feedbackData) and instead log only
non-sensitive, minimal info (e.g., "Feedback submitted" with a boolean or an id)
or a redacted summary by explicitly picking safe fields; keep or log
useSettingsStore.getState().getRestUrl() if needed but avoid pairing it with any
user/device identifiers; apply the same change to the other occurrence where
feedbackData is logged (the second console.log at the noted location).
Comment on lines
+332
to
352
| <View className="flex-row items-center mb-2 gap-1.5"> | ||
| <Text className="text-sm font-semibold text-foreground">{translate("feedback:emailOptional")}</Text> | ||
| <Pressable | ||
| hitSlop={10} | ||
| onPress={() => | ||
| showAlert(translate("feedback:emailOptional"), translate("feedback:emailInfoMessage"), [ | ||
| {text: translate("common:ok")}, | ||
| ]) | ||
| }> | ||
| <Icon name="info" size={16} color={theme.colors.muted_foreground} /> | ||
| </Pressable> | ||
| </View> | ||
| <TextInput | ||
| className="bg-background border border-border rounded-xl p-4 text-base text-foreground" | ||
| value={email} | ||
| onChangeText={setEmail} | ||
| placeholder={translate("feedback:email")} | ||
| placeholderTextColor={theme.colors.muted_foreground} | ||
| keyboardType="email-address" | ||
| autoCapitalize="none" | ||
| /> |
There was a problem hiding this comment.
Feature feedback is no longer reachable from this screen.
This block appears to have replaced the only UI that changed feedbackType, but feedbackType is still initialized to 'bug' on Line 25 and never updated afterward. That makes the feature-request branch, sendFeedback(...), and the app-rating prompt dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/src/app/miniapps/settings/feedback.tsx` around lines 332 - 352, The UI
that allowed changing feedbackType was removed, leaving feedbackType initialized
to 'bug' and never updated so the feature-request path (sendFeedback(...) and
the app-rating prompt) is unreachable; reintroduce a control (e.g., segmented
control, buttons, or Pressables) that updates the feedbackType state via its
setter (setFeedbackType) so users can select 'feature' (or other kinds), or
change the initial state to a neutral value and handle selection elsewhere, and
ensure the code paths in sendFeedback(...) that check feedbackType can be
exercised when the user selects 'feature'.
Contributor
|
I resolved the conflict and mistakenly pushed an unrelated commit that I then reverted. |
Member
|
@PhilippeFerreiraDeSousa has branched off your branch and is incorporating it into a bigger pr, so no need for this smaller og pr nice work though this is structured very well |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Made-with: Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Improvements