fix(iOS): missing listeners enabled check#108
Merged
gladiuscode merged 2 commits intomainfrom Apr 2, 2026
Conversation
3 tasks
📝 WalkthroughWalkthroughDelegates native orientation and lock operations to Changes
Sequence DiagramsequenceDiagram
participant App as App/Listener
participant EventEmitter as EventEmitter.ts
participant NativeDir as NativeOrientationDirector
participant EventMgr as EventManager.swift
participant ObjCDir as OrientationDirector.mm
App->>EventEmitter: register device/interface/lock listener
EventEmitter->>NativeDir: addDeviceOrientationListener / onInterfaceOrientationChanged / onLockChanged
NativeDir->>ObjCDir: enableOrientationSensors() (when first listener)
ObjCDir->>ObjCDir: sensor triggers -> prepare params
ObjCDir->>EventMgr: emitDeviceOrientationChangedWithParams(params)
Note right of EventMgr: delegate method renamed -> emitDeviceOrientationChanged
EventMgr->>EventEmitter: forward event to JS listeners
EventEmitter->>App: invoke callbacks
App->>EventEmitter: remove listener (last)
EventEmitter->>NativeDir: disableOrientationSensors()
NativeDir->>ObjCDir: disableOrientationSensors()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ios/implementation/EventManager.swift (1)
22-22: SwiftLint: Prefer dictionary literal syntax.Static analysis flags direct use of
Dictionary(dictionaryLiteral:). Use dictionary literal syntax for cleaner code.🔧 Proposed fix
-let params = Dictionary(dictionaryLiteral: ("orientation", value)) +let params: NSDictionary = ["orientation": value]Apply similarly for
lockedparameter.Also applies to: 35-35, 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/implementation/EventManager.swift` at line 22, Replace the use of Dictionary(dictionaryLiteral:) with Swift dictionary literal syntax: locate the places in EventManager.swift where params is created using Dictionary(dictionaryLiteral:) (the lines creating params for "orientation" and for "locked", and the similar occurrence around the third instance) and change them to use the ["key": value] literal form so the variables (params) are constructed with standard dictionary literal syntax instead of Dictionary(dictionaryLiteral:).src/EventEmitter.ts (1)
36-60: Minor fragility in proxy trap, but acceptable for this fix.The proxy intercepts the
getfor'remove'rather than the actual invocation. If code accesses.removewithout calling it (e.g.,const fn = subscription.remove;), the count decrements prematurely. However, this pattern appears to be pre-existing and the current fix is appropriately scoped.🔧 Optional: Safer approach using apply trap
private static createDeviceOrientationListenerProxy( listener: EventSubscription ) { - const handler: ProxyHandler<EventSubscription> = { - get(target, propertyKey, receiver) { - if (propertyKey === 'remove') { - disableOrientationSensorsIfLastListener(); - } - return Reflect.get(target, propertyKey, receiver); - }, - }; - - return new Proxy(listener, handler); + const originalRemove = listener.remove.bind(listener); + listener.remove = () => { + disableOrientationSensorsIfLastListener(); + originalRemove(); + }; + return listener; function disableOrientationSensorsIfLastListener() { // ... existing logic } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventEmitter.ts` around lines 36 - 60, The proxy currently decrements listener counts when the 'remove' property is accessed in the get trap (handler.get), which can run prematurely if code reads subscription.remove without calling it; change the Proxy behavior to decrement only when the remove function is actually invoked by intercepting calls (e.g., wrap the target.remove function or use an apply trap) so that disableOrientationSensorsIfLastListener() is executed on invocation, not property access; locate the Proxy for EventSubscription (handler: ProxyHandler<EventSubscription> / return new Proxy(listener, handler)), the get trap, the disableOrientationSensorsIfLastListener function, and ensure EventEmitter.listenerCount and NativeOrientationDirector.disableOrientationSensors are updated only when remove is called.ios/OrientationDirector.mm (1)
50-55: Exception handling correctly addresses thestd::bad_function_callcrash.The
try/catch (std::exception&)blocks appropriately catch the C++ exception (std::bad_function_callinherits fromstd::exception) that occurs when TurboModule event emitters are invoked without listeners in the new architecture.Consider adding a more descriptive comment explaining the specific exception being caught:
📝 Clarify exception handling intent
-(void)emitDeviceOrientationChangedWithParams:(NSDictionary*)params { try { [self emitOnDeviceOrientationChanged:params]; } catch (std::exception &e) { - // Ignore if no listeners + // Catches std::bad_function_call thrown when no JS listeners are registered } }Also applies to: 58-63, 67-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/OrientationDirector.mm` around lines 50 - 55, Update the catch blocks to clarify what specific exception is being handled: in emitDeviceOrientationChangedWithParams (and the other similar methods with try/catch at the next two blocks) change the comment inside the catch(std::exception &e) to explicitly mention std::bad_function_call coming from TurboModule event emitters invoked without listeners and note that it’s intentionally ignored; keep the existing catch signature and behavior but replace the generic "// Ignore if no listeners" with a brief explanatory comment referencing std::bad_function_call and TurboModule emitters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/implementation/EventManager.swift`:
- Line 22: Replace the use of Dictionary(dictionaryLiteral:) with Swift
dictionary literal syntax: locate the places in EventManager.swift where params
is created using Dictionary(dictionaryLiteral:) (the lines creating params for
"orientation" and for "locked", and the similar occurrence around the third
instance) and change them to use the ["key": value] literal form so the
variables (params) are constructed with standard dictionary literal syntax
instead of Dictionary(dictionaryLiteral:).
In `@ios/OrientationDirector.mm`:
- Around line 50-55: Update the catch blocks to clarify what specific exception
is being handled: in emitDeviceOrientationChangedWithParams (and the other
similar methods with try/catch at the next two blocks) change the comment inside
the catch(std::exception &e) to explicitly mention std::bad_function_call coming
from TurboModule event emitters invoked without listeners and note that it’s
intentionally ignored; keep the existing catch signature and behavior but
replace the generic "// Ignore if no listeners" with a brief explanatory comment
referencing std::bad_function_call and TurboModule emitters.
In `@src/EventEmitter.ts`:
- Around line 36-60: The proxy currently decrements listener counts when the
'remove' property is accessed in the get trap (handler.get), which can run
prematurely if code reads subscription.remove without calling it; change the
Proxy behavior to decrement only when the remove function is actually invoked by
intercepting calls (e.g., wrap the target.remove function or use an apply trap)
so that disableOrientationSensorsIfLastListener() is executed on invocation, not
property access; locate the Proxy for EventSubscription (handler:
ProxyHandler<EventSubscription> / return new Proxy(listener, handler)), the get
trap, the disableOrientationSensorsIfLastListener function, and ensure
EventEmitter.listenerCount and
NativeOrientationDirector.disableOrientationSensors are updated only when remove
is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78457401-c794-4241-92e4-96c6b8213c32
📒 Files selected for processing (5)
ios/OrientationDirector.mmios/implementation/EventManager.swiftsrc/EventEmitter.tssrc/RNOrientationDirector.tssrc/module.ts
💤 Files with no reviewable changes (1)
- src/module.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ios/implementation/EventManager.swift (1)
13-28: Use standard dictionary literals for all payload builders.
Dictionary(dictionaryLiteral:)is verbose and non-idiomatic Swift. Lines 18, 27, and 36 use this form to build single-key payloads; replace with plain literals like["orientation": value]and["locked": value]respectively. This removes compiler-protocol initializer usage and improves readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/implementation/EventManager.swift` around lines 13 - 28, The payloads are built using the verbose Dictionary(dictionaryLiteral:) initializer; update the payload creation in sendDeviceOrientationDidChange and sendInterfaceOrientationDidChange (and any similar builders producing single-key payloads like the one using "locked") to use Swift dictionary literals instead (e.g. ["orientation": value] or ["locked": value]) and pass that as the NSDictionary to emitDeviceOrientationChanged / emitInterfaceOrientationChanged so the code is idiomatic and simpler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/OrientationDirector.mm`:
- Around line 48-69: Add an explicit listener flag (e.g., a bool member like
listenerEnabled or hasOrientationListeners) and use it to guard calls to
emitOnDeviceOrientationChanged:, emitOnInterfaceOrientationChanged:, and
emitOnLockChanged: instead of unconditionally catching all std::exception;
update enableOrientationSensors and disableOrientationSensors to set/clear that
flag (matching Android's pattern). In each emit*WithParams method check the flag
first and only wrap the actual emit call in a minimal try/catch intended for
rare startup races (do not use the flag as a substitute for broad exception
swallowing), so unrelated emitter failures are not hidden.
---
Nitpick comments:
In `@ios/implementation/EventManager.swift`:
- Around line 13-28: The payloads are built using the verbose
Dictionary(dictionaryLiteral:) initializer; update the payload creation in
sendDeviceOrientationDidChange and sendInterfaceOrientationDidChange (and any
similar builders producing single-key payloads like the one using "locked") to
use Swift dictionary literals instead (e.g. ["orientation": value] or ["locked":
value]) and pass that as the NSDictionary to emitDeviceOrientationChanged /
emitInterfaceOrientationChanged so the code is idiomatic and simpler.
🪄 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: db412c98-fe6d-4180-88b3-7bd54822a230
📒 Files selected for processing (3)
ios/OrientationDirector.mmios/implementation/EventManager.swiftsrc/EventEmitter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/EventEmitter.ts
gladiuscode
deleted the
fix/107-ios-release-crash-on-launch-stdbad_function_call-after-upgrading-to-v300-new-architecture-hermes
branch
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
1 participant
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.
Summary by CodeRabbit
Bug Fixes
Refactor