Open
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:
📝 WalkthroughWalkthroughRemoves legacy architecture gating and ObjC pathways; converges to new-architecture TurboModule/event-emitter pattern; centralizes event delegation via delegate interfaces and a SharedOrientationDirectorImpl singleton; simplifies Android Gradle config; removes multiple bridging headers, legacy tests, and ObjC plugin updaters. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,238,255,0.5)
participant JS as JavaScript
participant E as EventEmitter (TS)
end
rect rgba(200,255,220,0.5)
participant TM as TurboModule (OrientationDirector)
participant EM as EventManager / Delegate
end
rect rgba(255,230,230,0.5)
participant NI as Native Impl (OrientationDirectorImpl / Shared)
end
JS->>E: subscribe onDevice/onInterface/onLock
E->>TM: register emitter (proxy increments android counter)
NI->>EM: orientation or lock state changes
EM->>TM: delegate.emitOnDevice/Interface/LockChanged(params)
TM->>E: deliver typed event to JS subscribers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.h`:
- Around line 6-10: Add a public Objective-C class method to OrientationDirector
to bridge the Swift implementation so generated ObjC code can call it: declare a
class method
+(UIInterfaceOrientationMask)getSupportedInterfaceOrientationsForWindow in
OrientationDirector (header) and implement it to return the Swift-backed value
(call the Swift singleton's supportedInterfaceOrientations, e.g.
SharedOrientationDirectorImpl or OrientationDirectorImpl shared instance). This
exposes OrientationDirector.getSupportedInterfaceOrientationsForWindow for the
code generated by plugin/src/withRNOrientationAppDelegate.ts.
In `@react-native-orientation-director.podspec`:
- Line 17: The podspec currently marks all iOS headers private which hides
OrientationDirector.h and breaks consumers; update the podspec to explicitly
expose OrientationDirector.h as a public header (e.g., add a public header entry
for "OrientationDirector.h" or use s.public_header_files to include
"ios/OrientationDirector.h") and restrict s.private_header_files to only
generated/internal headers (e.g., generated or internal subpaths) so the
plugin-imported symbol OrientationDirector.h remains on the public surface while
keeping other headers private; ensure references to OrientationDirector.h in
bridging headers/AppDelegate will resolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ca8a307-6f52-48e0-ac91-66c6227edb51
⛔ Files ignored due to path filters (1)
example/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
android/build.gradleandroid/gradle.propertiesandroid/src/main/AndroidManifest.xmlandroid/src/main/AndroidManifestNew.xmlandroid/src/main/java/com/orientationdirector/OrientationDirectorModule.ktandroid/src/main/java/com/orientationdirector/OrientationDirectorPackage.ktandroid/src/oldarch/OrientationDirectorModule.ktexample/ios/OrientationDirectorExample-Bridging-Header.hexample/ios/OrientationDirectorExample.xcodeproj/project.pbxprojexample/ios/OrientationDirectorExample/AppDelegate.swiftios/OrientationDirector.hios/OrientationDirector.mmios/SharedOrientationDirectorImpl.swiftios/react-native-orientation-director-Bridging-Header.hpackage.jsonreact-native-orientation-director.podspec
💤 Files with no reviewable changes (5)
- example/ios/OrientationDirectorExample-Bridging-Header.h
- ios/react-native-orientation-director-Bridging-Header.h
- example/ios/OrientationDirectorExample.xcodeproj/project.pbxproj
- android/src/main/AndroidManifestNew.xml
- android/src/oldarch/OrientationDirectorModule.kt
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/implementation/EventManager.swift (1)
13-36:⚠️ Potential issue | 🔴 CriticalUpdate all call sites or restore the old external argument label.
Lines 13 and 26 rename the external argument label from
orientationValuetovalue, butios/implementation/OrientationDirectorImpl.swiftstill calls the methods withorientationValue:. This breaks the build.Minimal fix
- func sendDeviceOrientationDidChange(value: Int) { + func sendDeviceOrientationDidChange(orientationValue value: Int) { @@ - func sendInterfaceOrientationDidChange(value: Int) { + func sendInterfaceOrientationDidChange(orientationValue value: Int) {Call sites at
ios/implementation/OrientationDirectorImpl.swift:179and206need to be updated if you proceed with the signature change.🤖 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 - 36, The external parameter label for sendDeviceOrientationDidChange(_: ) and sendInterfaceOrientationDidChange(_: ) was changed from orientationValue to value, breaking callers; fix by restoring the original external label or updating all callers to the new label — locate the functions sendDeviceOrientationDidChange(value: Int) and sendInterfaceOrientationDidChange(value: Int) and either change their signatures back to sendDeviceOrientationDidChange(orientationValue: Int) / sendInterfaceOrientationDidChange(orientationValue: Int) or update the call sites (e.g., in OrientationDirectorImpl where emit calls use orientationValue:) to call with the new label value: instead of orientationValue: so the method signatures and all callers match.src/EventEmitter.ts (1)
39-63:⚠️ Potential issue | 🟠 MajorDefer sensor cleanup until
remove()is actually invoked.Line 41 updates
androidListenerCountwhenremoveis read, not when the subscription is removed. Extractingsubscription.removeor callingremove()twice will undercount and can disable sensors while listeners are still active.🩹 Proposed fix
private static createDeviceOrientationListenerProxy( listener: EventSubscription ) { + let removed = false; const handler: ProxyHandler<EventSubscription> = { get(target, propertyKey, receiver) { - if (propertyKey === 'remove') { - disableOrientationSensorsIfLastListener(); + if (propertyKey !== 'remove') { + return Reflect.get(target, propertyKey, receiver); } - return Reflect.get(target, propertyKey, receiver); + + return () => { + if (removed) { + return; + } + + removed = true; + listener.remove(); + disableOrientationSensorsIfLastListener(); + }; }, }; @@ function disableOrientationSensorsIfLastListener() { - if (EventEmitter.androidListenerCount === 1) { - EventEmitter.androidListenerCount = 0; - Module.disableOrientationSensors(); - return; - } - if (EventEmitter.androidListenerCount === 0) { return; } EventEmitter.androidListenerCount--; - return; + if (EventEmitter.androidListenerCount === 0) { + Module.disableOrientationSensors(); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventEmitter.ts` around lines 39 - 63, The bug is that the Proxy handler currently calls disableOrientationSensorsIfLastListener when the 'remove' property is read, decrementing EventEmitter.androidListenerCount too early; instead, when creating the Proxy for an EventSubscription, wrap the actual remove method so the cleanup runs only when remove() is invoked (and only once): grab the original remove function from the listener (e.g., origRemove = listener.remove), replace listener.remove with a wrapper that checks a called flag, invokes origRemove(), and then calls disableOrientationSensorsIfLastListener() to decrement EventEmitter.androidListenerCount and call Module.disableOrientationSensors() if it was the last listener; keep the Proxy.get handler from triggering side effects on property access and ensure the wrapper is idempotent so multiple remove() calls won't undercount.
🧹 Nitpick comments (2)
ios/implementation/OrientationDirectorImpl.swift (1)
11-11: LGTM! Consider removing redundant@objcannotations.The change to
@objcMembersis appropriate for the new architecture migration. Since@objcMembersautomatically exposes all public members to Objective-C, the individual@objcannotations onsupportedInterfaceOrientations,init(),setEventManager(delegate:),getInterfaceOrientation(),getDeviceOrientation(),getIsLocked(),lockTo(jsValue:),unlock(), andresetSupportedInterfaceOrientations()are now redundant and could be removed for cleaner code.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/implementation/OrientationDirectorImpl.swift` at line 11, Remove the redundant per-member `@objc` annotations now that the class is marked with `@objcMembers`: delete the individual `@objc` attributes on supportedInterfaceOrientations, init(), setEventManager(delegate:), getInterfaceOrientation(), getDeviceOrientation(), getIsLocked(), lockTo(jsValue:), unlock(), and resetSupportedInterfaceOrientations() within the OrientationDirectorImpl class so members rely on `@objcMembers` for Objective-C exposure and keep signatures unchanged.android/src/main/java/com/orientationdirector/implementation/EventManager.kt (1)
4-6: Unused imports should be removed.
ReactApplicationContextandDeviceEventManagerModuleare no longer used after the refactoring to the delegate pattern. While this doesn't affect functionality, removing them improves code clarity.🧹 Remove unused imports
package com.orientationdirector.implementation import com.facebook.react.bridge.Arguments -import com.facebook.react.bridge.ReactApplicationContext import com.facebook.react.bridge.WritableMap -import com.facebook.react.modules.core.DeviceEventManagerModule🤖 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/EventManager.kt` around lines 4 - 6, Remove the unused imports in EventManager.kt: delete the imports for ReactApplicationContext and DeviceEventManagerModule since the class was refactored to a delegate pattern and those symbols are no longer referenced; keep the WritableMap import if still used by methods in EventManager.
🤖 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/build.gradle`:
- Around line 2-7: The ext.OrientationDirector map currently sets
compileSdkVersion and targetSdkVersion to 36 which is below the plugin
requirement; update the values in ext.OrientationDirector (keys
compileSdkVersion and targetSdkVersion) to at least 50 (e.g., 50) so the build
uses SDK 50+ and the plugin can run correctly.
In `@ios/OrientationDirector.mm`:
- Around line 119-127: The current stubs hide unsupported behavior; change
-(NSNumber *)isAutoRotationEnabled to return nil (not @(NO)) so JS can detect
"unsupported" vs a real false, and update -disableOrientationSensors and
-enableOrientationSensors to surface an explicit unsupported error (e.g., log
and raise an NSException or use the React Native bridge error mechanism) instead
of silently no-op; modify the implementations of isAutoRotationEnabled,
disableOrientationSensors, and enableOrientationSensors to reflect this behavior
so callers can distinguish unsupported-on-iOS from valid states/actions.
In `@README.md`:
- Around line 123-131: Remove the Objective-C import line `#import
"OrientationDirector.h"` from the Swift code block and replace it with the Swift
style import or omit it if the plugin already adds `import OrientationDirector`;
ensure the Swift snippet around the
application(_:supportedInterfaceOrientationsFor:) implementation uses `import
OrientationDirector` (or no manual import) and keeps the return of
SharedOrientationDirectorImpl.shared.supportedInterfaceOrientations unchanged.
---
Outside diff comments:
In `@ios/implementation/EventManager.swift`:
- Around line 13-36: The external parameter label for
sendDeviceOrientationDidChange(_: ) and sendInterfaceOrientationDidChange(_: )
was changed from orientationValue to value, breaking callers; fix by restoring
the original external label or updating all callers to the new label — locate
the functions sendDeviceOrientationDidChange(value: Int) and
sendInterfaceOrientationDidChange(value: Int) and either change their signatures
back to sendDeviceOrientationDidChange(orientationValue: Int) /
sendInterfaceOrientationDidChange(orientationValue: Int) or update the call
sites (e.g., in OrientationDirectorImpl where emit calls use orientationValue:)
to call with the new label value: instead of orientationValue: so the method
signatures and all callers match.
In `@src/EventEmitter.ts`:
- Around line 39-63: The bug is that the Proxy handler currently calls
disableOrientationSensorsIfLastListener when the 'remove' property is read,
decrementing EventEmitter.androidListenerCount too early; instead, when creating
the Proxy for an EventSubscription, wrap the actual remove method so the cleanup
runs only when remove() is invoked (and only once): grab the original remove
function from the listener (e.g., origRemove = listener.remove), replace
listener.remove with a wrapper that checks a called flag, invokes origRemove(),
and then calls disableOrientationSensorsIfLastListener() to decrement
EventEmitter.androidListenerCount and call Module.disableOrientationSensors() if
it was the last listener; keep the Proxy.get handler from triggering side
effects on property access and ensure the wrapper is idempotent so multiple
remove() calls won't undercount.
---
Nitpick comments:
In
`@android/src/main/java/com/orientationdirector/implementation/EventManager.kt`:
- Around line 4-6: Remove the unused imports in EventManager.kt: delete the
imports for ReactApplicationContext and DeviceEventManagerModule since the class
was refactored to a delegate pattern and those symbols are no longer referenced;
keep the WritableMap import if still used by methods in EventManager.
In `@ios/implementation/OrientationDirectorImpl.swift`:
- Line 11: Remove the redundant per-member `@objc` annotations now that the class
is marked with `@objcMembers`: delete the individual `@objc` attributes on
supportedInterfaceOrientations, init(), setEventManager(delegate:),
getInterfaceOrientation(), getDeviceOrientation(), getIsLocked(),
lockTo(jsValue:), unlock(), and resetSupportedInterfaceOrientations() within the
OrientationDirectorImpl class so members rely on `@objcMembers` for Objective-C
exposure and keep signatures unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cde1d342-2a26-44b7-b7ec-0216f0aeed1c
⛔ Files ignored due to path filters (3)
example/ios/Podfile.lockis excluded by!**/*.lockplugin/__tests__/__snapshots__/withRNOrientationAppDelegate.spec.ts.snapis excluded by!**/*.snapplugin/__tests__/__snapshots__/withRNOrientationBridgingHeader.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (26)
.github/workflows/pr.yml.github/workflows/release.ymlREADME.mdandroid/build.gradleandroid/gradle.propertiesandroid/src/main/java/com/orientationdirector/OrientationDirectorModule.ktandroid/src/main/java/com/orientationdirector/implementation/EventManager.ktandroid/src/main/java/com/orientationdirector/implementation/EventManagerDelegate.ktandroid/src/main/java/com/orientationdirector/implementation/OrientationDirectorModuleImpl.ktandroid/src/test/java/com/orientationdirector/implementation/OrientationDirectorModuleImplTest.ktandroid/src/test/java/com/orientationdirector/implementation/UtilsTest.ktexample/ios/Podfileios/OrientationDirector.hios/OrientationDirector.mmios/implementation/EventManager.swiftios/implementation/OrientationDirectorImpl.swiftplugin/__tests__/fixtures/AppDelegate.mmplugin/__tests__/fixtures/Bridging-Header.hplugin/__tests__/withRNOrientationAppDelegate.spec.tsplugin/__tests__/withRNOrientationBridgingHeader.spec.tsplugin/src/custom-mod/withBridgingHeader.tsplugin/src/index.tsplugin/src/withRNOrientationAppDelegate.tsplugin/src/withRNOrientationBridgingHeader.tssrc/EventEmitter.tssrc/NativeOrientationDirector.ts
💤 Files with no reviewable changes (12)
- android/src/test/java/com/orientationdirector/implementation/UtilsTest.kt
- plugin/src/index.ts
- .github/workflows/release.yml
- plugin/tests/fixtures/AppDelegate.mm
- android/gradle.properties
- plugin/tests/withRNOrientationAppDelegate.spec.ts
- .github/workflows/pr.yml
- plugin/tests/withRNOrientationBridgingHeader.spec.ts
- plugin/src/withRNOrientationBridgingHeader.ts
- plugin/src/custom-mod/withBridgingHeader.ts
- plugin/tests/fixtures/Bridging-Header.h
- android/src/test/java/com/orientationdirector/implementation/OrientationDirectorModuleImplTest.kt
Owner
Author
|
I have yet to test everything out properly, so I'll do that and then merge this to finally say goodbye to the old arch. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 121: Remove the leading word "Then," from the sentence that introduces
implementing supportedInterfaceOrientationsFor in AppDelegate.swift so the
instruction reads directly (e.g., "In your AppDelegate.swift file, implement the
supportedInterfaceOrientationsFor method as follows:"); update the README.md
line that currently starts with "Then," to avoid implying a prior step and
ensure the sentence starts cleanly and flows with the surrounding instructions.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (2)
9-9: Optional: Consider using Oxford comma for clarity.The list "Kotlin, Swift and Typescript" could use an Oxford comma: "Kotlin, Swift, and Typescript" for improved readability and consistency with standard English style guides.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 9, Update the sentence "Written in Kotlin, Swift and Typescript." to include the Oxford comma by changing it to "Written in Kotlin, Swift, and Typescript." to improve readability and consistency; locate and edit that exact line in README.md.
123-131: Optional: Consider adding class context hint.While the instruction makes it clear this goes "In your AppDelegate.swift file," the code snippet could be slightly more explicit that the method should be added inside the AppDelegate class. This is purely for additional clarity.
📖 Optional documentation enhancement
```swift import OrientationDirector +// Add this method to your AppDelegate class: func application(_ application: UIApplication, supportedInterfaceOrientationsFor window: UIWindow?) -> UIInterfaceOrientationMask { return SharedOrientationDirectorImpl.shared.supportedInterfaceOrientations }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 123 - 131, Add a brief comment clarifying that the
method belongs inside your AppDelegate class: insert a one-line note above the
application(_:supportedInterfaceOrientationsFor:) method indicating "Add this
method to your AppDelegate class" so readers know the context; ensure the
snippet still imports OrientationDirector and returns
SharedOrientationDirectorImpl.shared.supportedInterfaceOrientations as shown.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@README.md:
- Line 9: Update the sentence "Written in Kotlin, Swift and Typescript." to
include the Oxford comma by changing it to "Written in Kotlin, Swift, and
Typescript." to improve readability and consistency; locate and edit that exact
line in README.md.- Around line 123-131: Add a brief comment clarifying that the method belongs
inside your AppDelegate class: insert a one-line note above the
application(_:supportedInterfaceOrientationsFor:) method indicating "Add this
method to your AppDelegate class" so readers know the context; ensure the
snippet still imports OrientationDirector and returns
SharedOrientationDirectorImpl.shared.supportedInterfaceOrientations as shown.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `1d28c668-f148-4c2c-8366-29e5b734fb24` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 70bcc0b3a020104b8593ca3bc58e60b9aab10b63 and a8e4ce21ec1a81ed27883749b036671fda41a658. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ios/OrientationDirector.mm (1)
111-119:⚠️ Potential issue | 🟠 MajorDon’t present unsupported sensor APIs as successful iOS behavior.
Line 111 returning
@(NO)and Lines 116/119 being silent no-ops make JS treat unsupported features as valid false/success paths. Please surface unsupported explicitly (e.g., nullable return + warning/error path) so callers can distinguish capability vs state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/OrientationDirector.mm` around lines 111 - 119, The current implementations of isAutoRotationEnabled, disableOrientationSensors, and enableOrientationSensors silently pretend the APIs are supported; instead make unsupported behavior explicit by returning nil from isAutoRotationEnabled (use a nullable NSNumber) and have disableOrientationSensors/enableOrientationSensors signal failure (e.g., call the module logger/warn method or invoke the JS-facing error callback) so callers can distinguish "unsupported" from a valid false/success; update the method signatures to nullable where applicable and ensure any JS bridge path checks for nil/error rather than assuming a boolean or success.
🧹 Nitpick comments (2)
src/module.ts (1)
1-1: Document the breaking API change tied to this cleanup.Line 1 reflects the emitter cleanup, and this file now only default-exports
OrientationDirectorModule(noModuleEventEmitternamed export). Please add a migration note/changelog entry so downstream users withimport { ModuleEventEmitter } ...don’t get surprised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.ts` at line 1, Update the project changelog/release notes to document the breaking API change where src/module.ts now only default-exports OrientationDirectorModule and no longer exports ModuleEventEmitter; add a migration note instructing downstream users to replace imports like "import { ModuleEventEmitter } from '...'" with the new pattern (import OrientationDirectorModule as default and use the new internal emitter API or recommended alternative), include the version bump that introduces this change and a short example of the before/after import to make migration straightforward.ios/implementation/EventManager.swift (1)
18-37: Use dictionary literals instead ofDictionary(dictionaryLiteral:).Lines 18, 27, and 36 use the compiler-protocol initializer, which SwiftLint flags. Replace with dictionary literals for clarity and compliance.
Proposed refactor
- let params = Dictionary(dictionaryLiteral: ("orientation", value)) - delegate.emitOnDeviceOrientationDidChange(params: params as NSDictionary) + let params: NSDictionary = ["orientation": value] + delegate.emitOnDeviceOrientationDidChange(params: params) - let params = Dictionary(dictionaryLiteral: ("orientation", value)) - delegate.emitOnInterfaceOrientationDidChange(params: params as NSDictionary) + let params: NSDictionary = ["orientation": value] + delegate.emitOnInterfaceOrientationDidChange(params: params) - let params = Dictionary(dictionaryLiteral: ("locked", value)) - delegate.emitOnLockChanged(params: params as NSDictionary) + let params: NSDictionary = ["locked": value] + delegate.emitOnLockChanged(params: params)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/implementation/EventManager.swift` around lines 18 - 37, Replace the usage of Dictionary(dictionaryLiteral:) with Swift dictionary literals in the orientation/lock event senders: update sendDeviceOrientationDidChange (the call to delegate.emitOnDeviceOrientationDidChange), sendInterfaceOrientationDidChange (delegate.emitOnInterfaceOrientationDidChange) and sendLockDidChange (delegate.emitOnLockChanged) to construct params using a literal (e.g. ["orientation": value] or ["locked": value]) instead of Dictionary(dictionaryLiteral:), keeping the same keys and types and passing the result as an NSDictionary to the delegate.
🤖 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 80-83: The isLocked accessor reads _director state off the main
thread while writes are dispatched to the main queue, causing a race; change
-(NSNumber *)isLocked to synchronously fetch the value on the main thread (e.g.
use dispatch_sync to dispatch_get_main_queue() or performSelectorOnMainThread:)
and call [_director getIsLocked] inside that main-thread block, then wrap the
retrieved BOOL in an NSNumber before returning so reads are thread-safe with the
existing main-queue writes.
---
Duplicate comments:
In `@ios/OrientationDirector.mm`:
- Around line 111-119: The current implementations of isAutoRotationEnabled,
disableOrientationSensors, and enableOrientationSensors silently pretend the
APIs are supported; instead make unsupported behavior explicit by returning nil
from isAutoRotationEnabled (use a nullable NSNumber) and have
disableOrientationSensors/enableOrientationSensors signal failure (e.g., call
the module logger/warn method or invoke the JS-facing error callback) so callers
can distinguish "unsupported" from a valid false/success; update the method
signatures to nullable where applicable and ensure any JS bridge path checks for
nil/error rather than assuming a boolean or success.
---
Nitpick comments:
In `@ios/implementation/EventManager.swift`:
- Around line 18-37: Replace the usage of Dictionary(dictionaryLiteral:) with
Swift dictionary literals in the orientation/lock event senders: update
sendDeviceOrientationDidChange (the call to
delegate.emitOnDeviceOrientationDidChange), sendInterfaceOrientationDidChange
(delegate.emitOnInterfaceOrientationDidChange) and sendLockDidChange
(delegate.emitOnLockChanged) to construct params using a literal (e.g.
["orientation": value] or ["locked": value]) instead of
Dictionary(dictionaryLiteral:), keeping the same keys and types and passing the
result as an NSDictionary to the delegate.
In `@src/module.ts`:
- Line 1: Update the project changelog/release notes to document the breaking
API change where src/module.ts now only default-exports
OrientationDirectorModule and no longer exports ModuleEventEmitter; add a
migration note instructing downstream users to replace imports like "import {
ModuleEventEmitter } from '...'" with the new pattern (import
OrientationDirectorModule as default and use the new internal emitter API or
recommended alternative), include the version bump that introduces this change
and a short example of the before/after import to make migration
straightforward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3a08c9b-c2f5-4a98-ab8c-1712eaf2a82f
📒 Files selected for processing (4)
ios/OrientationDirector.mmios/implementation/EventManager.swiftios/implementation/OrientationDirectorImpl.swiftsrc/module.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/implementation/OrientationDirectorImpl.swift
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
Refactor
Documentation
Chores
Tests