feat(android): rewrite sensor logic to improve reliability#103
feat(android): rewrite sensor logic to improve reliability#103gladiuscode merged 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitched Android orientation handling to use Rotation Vector + OrientationEventListener; OrientationSensorsEventListener now reports device and face Orientation enums via a device-orientation callback, OrientationDirectorModuleImpl consumes device-orientation events, and two orientation conversion helpers were removed from Utils. Changes
Sequence Diagram(s)sequenceDiagram
actor RotationVectorSensor
participant SensorListener as OrientationSensorsEventListener
participant Module as OrientationDirectorModuleImpl
participant JS as React/JS
RotationVectorSensor->>SensorListener: SensorEvent(rotation vector)
SensorListener->>SensorListener: getRotationMatrixFromVector()\ncompute face Z / map angles -> Device Orientation
alt face orientation determined
SensorListener-->>Module: notifyFaceOrientationChanged(Orientation)
end
SensorListener-->>Module: notifyDeviceOrientationChanged(Orientation)
Module->>Module: onDeviceOrientationChanged(Orientation)\nreturn if UNKNOWN or unchanged
Module-->>JS: emit orientation change event
Module->>SensorListener: enable()/disable() (manage lifecycle)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/orientationdirector/implementation/OrientationDirectorModuleImpl.kt (1)
154-172:⚠️ Potential issue | 🟠 MajorDon’t finalize initial orientation on flat states (FACE_UP/FACE_DOWN).
Line 168 marks initial orientation as computed for any non-
UNKNOWNvalue. BecauseOrientationSensorsEventListeneremitsFACE_UP/FACE_DOWNvia the same callback (OrientationSensorsEventListener.ktLine 108-Line 111), the module can disable sensors while still lacking a stable portrait/landscape orientation.Proposed fix
private fun onDeviceOrientationChanged(deviceOrientation: Orientation) { if (deviceOrientation == Orientation.UNKNOWN) return - if (lastDeviceOrientation == deviceOrientation) return mEventManager.sendDeviceOrientationDidChange(deviceOrientation.ordinal) lastDeviceOrientation = deviceOrientation checkInterfaceOrientation() // NOTE(2.init): This is needed to disable sensors if they were needed just for the initial // device computation. - if (didComputeInitialDeviceOrientation) return + val isFlatOrientation = + deviceOrientation == Orientation.FACE_UP || deviceOrientation == Orientation.FACE_DOWN + if (didComputeInitialDeviceOrientation || isFlatOrientation) return didComputeInitialDeviceOrientation = true if (!areOrientationSensorsEnabled) { mOrientationSensorsEventListener.disable() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationDirectorModuleImpl.kt` around lines 154 - 172, The onDeviceOrientationChanged function currently marks didComputeInitialDeviceOrientation true for any non-UNKNOWN deviceOrientation, causing FACE_UP/FACE_DOWN to finalize the initial state and potentially disable sensors; change the logic in onDeviceOrientationChanged to ignore FACE_UP and FACE_DOWN (i.e., only set didComputeInitialDeviceOrientation = true and disable sensors when deviceOrientation is one of the stable orientations like PORTRAIT, PORTRAIT_UPSIDE_DOWN, LANDSCAPE_LEFT, LANDSCAPE_RIGHT), and keep lastDeviceOrientation and the early-return behavior intact so sensors aren’t disabled prematurely.
🧹 Nitpick comments (1)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt (1)
113-113: Replace empty override block to satisfy static analysis.Line 113 is currently an empty function block and is flagged by detekt (
EmptyFunctionBlock). Use an expression body instead.Proposed fix
- override fun onAccuracyChanged(p0: Sensor?, p1: Int) {} + override fun onAccuracyChanged(sensor: Sensor?, accuracy: Int) = Unit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt` at line 113, The override onAccuracyChanged in OrientationSensorsEventListener currently has an empty function block which triggers detekt's EmptyFunctionBlock; change it to an expression body (returning Unit) instead of an empty block so the method stays explicit but non-empty — update the declaration of onAccuracyChanged(p0: Sensor?, p1: Int) to use an expression body (e.g., = Unit) to satisfy static analysis without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationDirectorModuleImpl.kt`:
- Around line 154-172: The onDeviceOrientationChanged function currently marks
didComputeInitialDeviceOrientation true for any non-UNKNOWN deviceOrientation,
causing FACE_UP/FACE_DOWN to finalize the initial state and potentially disable
sensors; change the logic in onDeviceOrientationChanged to ignore FACE_UP and
FACE_DOWN (i.e., only set didComputeInitialDeviceOrientation = true and disable
sensors when deviceOrientation is one of the stable orientations like PORTRAIT,
PORTRAIT_UPSIDE_DOWN, LANDSCAPE_LEFT, LANDSCAPE_RIGHT), and keep
lastDeviceOrientation and the early-return behavior intact so sensors aren’t
disabled prematurely.
---
Nitpick comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Line 113: The override onAccuracyChanged in OrientationSensorsEventListener
currently has an empty function block which triggers detekt's
EmptyFunctionBlock; change it to an expression body (returning Unit) instead of
an empty block so the method stays explicit but non-empty — update the
declaration of onAccuracyChanged(p0: Sensor?, p1: Int) to use an expression body
(e.g., = Unit) to satisfy static analysis without changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54fbfb88-cef8-4a95-aefd-81ac92b2d270
📒 Files selected for processing (3)
android/src/main/java/com/orientationdirector/implementation/OrientationDirectorModuleImpl.ktandroid/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.ktandroid/src/main/java/com/orientationdirector/implementation/Utils.kt
💤 Files with no reviewable changes (1)
- android/src/main/java/com/orientationdirector/implementation/Utils.kt
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt (2)
34-54:⚠️ Potential issue | 🟠 MajorReset the cached orientations when the listener is re-enabled.
lastComputedDeviceOrientationandlastComputedFaceOrientationsurvivedisable(). Re-enabling in the same pose then suppresses the first callback, so a fresh subscriber can stay stale until the next real rotation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt` around lines 34 - 54, The cached orientation fields lastComputedDeviceOrientation and lastComputedFaceOrientation are left intact across disable()/enable() which can suppress the first callback after re-enabling; clear/reset these caches when (re)enabling the listener so subscribers get an immediate fresh callback. In the OrientationSensorsEventListener.enable() method (before registering the sensor or right at start of the method) set lastComputedDeviceOrientation and lastComputedFaceOrientation back to their initial/neutral state (e.g., null or the same sentinel used on creation) so the first sensor event is treated as new; you may also clear them in disable() if you prefer to keep state teardown there, but ensure at least enable() resets them.
38-42:⚠️ Potential issue | 🟡 Minor
SENSOR_DELAY_UIis in the wrong slot here.The fourth argument
maxReportLatencyUsmust be an explicit microsecond value (e.g.,0or a specific duration), not a sensor delay preset constant. PassingSensorManager.SENSOR_DELAY_UI(which equals2) means the max report latency is only 2 microseconds, effectively disabling sensor event batching. The sampling rate remainsSENSOR_DELAY_NORMALfrom the third argument, but the batching configuration is incorrect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt` around lines 38 - 42, The registerListener call is passing SensorManager.SENSOR_DELAY_UI as the fourth argument (maxReportLatencyUs), which is incorrect; update the mSensorManager.registerListener(this, mRotationSensor, SensorManager.SENSOR_DELAY_NORMAL, SensorManager.SENSOR_DELAY_UI) invocation to pass an explicit microsecond latency value (e.g., 0L for no batching or a specific microsecond duration) instead of SENSOR_DELAY_UI so maxReportLatencyUs is set correctly; ensure you reference the same mSensorManager.registerListener call and mRotationSensor and keep the sampling rate as SensorManager.SENSOR_DELAY_NORMAL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Around line 56-72: The method onOrientationChanged currently uses
lastComputedDeviceOrientation both as the "flat/unknown" flag and the dedupe
key, causing missed transitions; introduce a separate
lastEmittedDeviceOrientation (or similar) and use that for the dedupe check
instead of lastComputedDeviceOrientation. In onOrientationChanged, when
angleDegrees == ORIENTATION_UNKNOWN keep lastComputedDeviceOrientation =
Orientation.UNKNOWN (to represent the computed state) but do not update
lastEmittedDeviceOrientation or call notifyDeviceOrientationChanged; compute
currentDeviceOrientation from angleDegrees, compare it against
lastEmittedDeviceOrientation (not lastComputedDeviceOrientation), and only when
different call notifyDeviceOrientationChanged and then set
lastEmittedDeviceOrientation = currentDeviceOrientation (and optionally update
lastComputedDeviceOrientation too). Ensure you update references in the class to
use lastEmittedDeviceOrientation for all dedupe comparisons.
---
Outside diff comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Around line 34-54: The cached orientation fields lastComputedDeviceOrientation
and lastComputedFaceOrientation are left intact across disable()/enable() which
can suppress the first callback after re-enabling; clear/reset these caches when
(re)enabling the listener so subscribers get an immediate fresh callback. In the
OrientationSensorsEventListener.enable() method (before registering the sensor
or right at start of the method) set lastComputedDeviceOrientation and
lastComputedFaceOrientation back to their initial/neutral state (e.g., null or
the same sentinel used on creation) so the first sensor event is treated as new;
you may also clear them in disable() if you prefer to keep state teardown there,
but ensure at least enable() resets them.
- Around line 38-42: The registerListener call is passing
SensorManager.SENSOR_DELAY_UI as the fourth argument (maxReportLatencyUs), which
is incorrect; update the mSensorManager.registerListener(this, mRotationSensor,
SensorManager.SENSOR_DELAY_NORMAL, SensorManager.SENSOR_DELAY_UI) invocation to
pass an explicit microsecond latency value (e.g., 0L for no batching or a
specific microsecond duration) instead of SENSOR_DELAY_UI so maxReportLatencyUs
is set correctly; ensure you reference the same mSensorManager.registerListener
call and mRotationSensor and keep the sampling rate as
SensorManager.SENSOR_DELAY_NORMAL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c564161c-73d2-4a29-955d-1fe7f3186b19
📒 Files selected for processing (1)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt (2)
8-8: Unused import.
android.util.Logis imported but not used in this file.🧹 Suggested removal
-import android.util.Log🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt` at line 8, Remove the unused import `android.util.Log` from OrientationSensorsEventListener.kt; open the file containing the OrientationSensorsEventListener class (or related top-level functions) and delete the `import android.util.Log` line, then run a quick build or lint to confirm no remaining references to Log remain.
82-85: Face orientation transition to UNKNOWN is not emitted.When
lastComputedDeviceOrientation != UNKNOWN(device is no longer flat),lastComputedFaceOrientationis reset toUNKNOWNsilently without invoking the callback. If consumers track face orientation state, they won't be notified when the device transitions away fromFACE_UP/FACE_DOWN.If this is intentional (face orientation only meaningful when device is flat), consider documenting this behavior. Otherwise, you may want to emit
Orientation.UNKNOWNfor face orientation before the early return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt` around lines 82 - 85, When lastComputedDeviceOrientation != Orientation.UNKNOWN the code silently sets lastComputedFaceOrientation = Orientation.UNKNOWN and returns; update this to also emit the face-orientation change (i.e., call the face-orientation callback/notification) with Orientation.UNKNOWN before the early return so consumers are notified, or alternatively add a clear comment/docstring near lastComputedDeviceOrientation and lastComputedFaceOrientation explaining that face orientation updates are suppressed when the device is not flat; locate the logic around lastComputedDeviceOrientation / lastComputedFaceOrientation in OrientationSensorsEventListener.kt and either invoke the existing face-orientation callback with Orientation.UNKNOWN before returning or add the documentation explaining the intentional 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
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Around line 95-99: The face-down detection logic in
OrientationSensorsEventListener.kt is using absPitch and comparing it to
FACE_DOWN_LIMIT (155°) which can never be exceeded because
SensorManager.getOrientation() returns pitch in [-90, +90], so absPitch >
FACE_DOWN_LIMIT is always false; update the logic that computes
currentFaceOrientation to check the signed pitch (e.g., pitch <
-FACE_DOWN_PITCH_THRESHOLD) instead of absPitch or reduce FACE_DOWN_LIMIT to a
realistic threshold within 0–90 and use absPitch > NEW_LIMIT, and keep the
existing roll check (absRoll) as a secondary condition — update references to
absPitch, FACE_DOWN_LIMIT and currentFaceOrientation accordingly so face-down
states are correctly detected.
---
Nitpick comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Line 8: Remove the unused import `android.util.Log` from
OrientationSensorsEventListener.kt; open the file containing the
OrientationSensorsEventListener class (or related top-level functions) and
delete the `import android.util.Log` line, then run a quick build or lint to
confirm no remaining references to Log remain.
- Around line 82-85: When lastComputedDeviceOrientation != Orientation.UNKNOWN
the code silently sets lastComputedFaceOrientation = Orientation.UNKNOWN and
returns; update this to also emit the face-orientation change (i.e., call the
face-orientation callback/notification) with Orientation.UNKNOWN before the
early return so consumers are notified, or alternatively add a clear
comment/docstring near lastComputedDeviceOrientation and
lastComputedFaceOrientation explaining that face orientation updates are
suppressed when the device is not flat; locate the logic around
lastComputedDeviceOrientation / lastComputedFaceOrientation in
OrientationSensorsEventListener.kt and either invoke the existing
face-orientation callback with Orientation.UNKNOWN before returning or add the
documentation explaining the intentional behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74589bf1-aa8e-438b-8694-d3d65b7e3654
📒 Files selected for processing (1)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt (1)
17-17: Prune the leftover orientation-angle computation.
currentFaceOrientationonly depends onrotationMatrix[8]. TheorientationAnglesfield and theSensorManager.getOrientation(...)call are dead work on every rotation-vector sample now.🧹 Suggested cleanup
private val rotationMatrix = FloatArray(9) - private val orientationAngles = FloatArray(3) SensorManager.getRotationMatrixFromVector(rotationMatrix, event.values) - SensorManager.getOrientation(rotationMatrix, orientationAngles)Also applies to: 87-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt` at line 17, Remove the unused orientationAngles array and the dead call to SensorManager.getOrientation(...) (and any assignments to orientationAngles) since currentFaceOrientation only uses rotationMatrix[8]; locate the orientationAngles declaration and usages in OrientationSensorsEventListener (including lines around onSensorChanged where SensorManager.getOrientation is called) and delete them, then replace any logic that relied on orientationAngles with direct reads from rotationMatrix[8] (e.g., use rotationMatrix[8] when computing currentFaceOrientation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Around line 104-111: notifyDeviceOrientationChanged and
notifyFaceOrientationChanged overwrite caches after invoking the callback, which
lets a synchronous disable() call in the callback reset the caches only to have
the old value written back; to fix, capture the current cache value (e.g., prev
= lastComputedDeviceOrientation / lastComputedFaceOrientation) before calling
onDeviceOrientationChangedCallback, invoke the callback, then only assign
lastComputedDeviceOrientation = deviceOrientation (or
lastComputedFaceOrientation = faceOrientation) if the cache still equals prev
(meaning the callback did not call disable() or otherwise change it); reference
functions: notifyDeviceOrientationChanged, notifyFaceOrientationChanged,
onDeviceOrientationChangedCallback, lastComputedDeviceOrientation,
lastComputedFaceOrientation, and disable().
---
Nitpick comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Line 17: Remove the unused orientationAngles array and the dead call to
SensorManager.getOrientation(...) (and any assignments to orientationAngles)
since currentFaceOrientation only uses rotationMatrix[8]; locate the
orientationAngles declaration and usages in OrientationSensorsEventListener
(including lines around onSensorChanged where SensorManager.getOrientation is
called) and delete them, then replace any logic that relied on orientationAngles
with direct reads from rotationMatrix[8] (e.g., use rotationMatrix[8] when
computing currentFaceOrientation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e799a2d-eb58-4ab7-9e15-a7ae85bb4d2f
📒 Files selected for processing (1)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt (1)
79-82: Silent reset of face orientation when device orientation becomes known.When the device transitions from flat to tilted,
lastComputedFaceOrientationis reset toUNKNOWNwithout notifying consumers. This means if a consumer previously receivedFACE_UP, they won't receive an explicit notification that face orientation is no longer valid.This is likely acceptable since:
- The handler in
OrientationDirectorModuleImplfilters outUNKNOWNvalues- A non-unknown device orientation semantically supersedes face orientation
Just flagging for awareness in case consumers need to track face orientation end events.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt` around lines 79 - 82, The code silently sets lastComputedFaceOrientation = Orientation.UNKNOWN when lastComputedDeviceOrientation becomes known; instead dispatch an explicit face-orientation update so consumers are notified of the transition. Modify the block that currently sets lastComputedFaceOrientation (look for lastComputedDeviceOrientation, lastComputedFaceOrientation, and Orientation.UNKNOWN in OrientationSensorsEventListener.kt) to call the same notification/emission path used elsewhere (the method or callback used by OrientationDirectorModuleImpl to receive orientation updates) with Orientation.UNKNOWN before changing the internal field so consumers receive the “face orientation ended” event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt`:
- Around line 79-82: The code silently sets lastComputedFaceOrientation =
Orientation.UNKNOWN when lastComputedDeviceOrientation becomes known; instead
dispatch an explicit face-orientation update so consumers are notified of the
transition. Modify the block that currently sets lastComputedFaceOrientation
(look for lastComputedDeviceOrientation, lastComputedFaceOrientation, and
Orientation.UNKNOWN in OrientationSensorsEventListener.kt) to call the same
notification/emission path used elsewhere (the method or callback used by
OrientationDirectorModuleImpl to receive orientation updates) with
Orientation.UNKNOWN before changing the internal field so consumers receive the
“face orientation ended” event.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f253b495-ac85-480e-b91c-a140cb2b7ab6
📒 Files selected for processing (1)
android/src/main/java/com/orientationdirector/implementation/OrientationSensorsEventListener.kt
Summary by CodeRabbit
New Features
Bug Fixes
Refactor