Conversation
WalkthroughThis PR enhances null-safety checks in Android, updates C++ lambda capture semantics, adds platform-specific header compilation for iOS, refactors iOS Prisma module for new React Native architecture support, and adjusts the build script for migrations copying. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d89adffc-66b7-4720-a069-b88a786e04fb
📒 Files selected for processing (5)
android/src/main/java/com/prisma/PrismaModule.javacopy-migrations.shcpp/QueryEngineHostObject.hcpp/react-native-prisma.cppios/Prisma.mm
| if (jsContextPointer == 0) { | ||
| throw new RuntimeException("JSI runtime pointer is null. Make sure Hermes is enabled and the runtime is initialized before calling install()."); | ||
| } | ||
| CallInvokerHolderImpl jsCallInvokerHolder; | ||
| if (context.getJSCallInvokerHolder() != null) { | ||
| jsCallInvokerHolder = (CallInvokerHolderImpl) context.getJSCallInvokerHolder(); | ||
| } else if (context.getCatalystInstance() != null) { | ||
| jsCallInvokerHolder = (CallInvokerHolderImpl) context.getCatalystInstance().getJSCallInvokerHolder(); | ||
| } else { | ||
| throw new RuntimeException("JSCallInvokerHolder is not available yet."); | ||
| } |
There was a problem hiding this comment.
Null-safety improvements are good, but the fallback path may still NPE.
The null checks for jsContextPointer and getJSCallInvokerHolder() are correct. However, on line 132, context.getCatalystInstance().getJSCallInvokerHolder() may also return null even when getCatalystInstance() is non-null, leading to an NPE during the cast.
🛠️ Proposed fix to add null check
if (context.getJSCallInvokerHolder() != null) {
jsCallInvokerHolder = (CallInvokerHolderImpl) context.getJSCallInvokerHolder();
} else if (context.getCatalystInstance() != null) {
- jsCallInvokerHolder = (CallInvokerHolderImpl) context.getCatalystInstance().getJSCallInvokerHolder();
+ var holder = context.getCatalystInstance().getJSCallInvokerHolder();
+ if (holder == null) {
+ throw new RuntimeException("JSCallInvokerHolder from CatalystInstance is null.");
+ }
+ jsCallInvokerHolder = (CallInvokerHolderImpl) holder;
} else {
throw new RuntimeException("JSCallInvokerHolder is not available yet.");
}📝 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.
| if (jsContextPointer == 0) { | |
| throw new RuntimeException("JSI runtime pointer is null. Make sure Hermes is enabled and the runtime is initialized before calling install()."); | |
| } | |
| CallInvokerHolderImpl jsCallInvokerHolder; | |
| if (context.getJSCallInvokerHolder() != null) { | |
| jsCallInvokerHolder = (CallInvokerHolderImpl) context.getJSCallInvokerHolder(); | |
| } else if (context.getCatalystInstance() != null) { | |
| jsCallInvokerHolder = (CallInvokerHolderImpl) context.getCatalystInstance().getJSCallInvokerHolder(); | |
| } else { | |
| throw new RuntimeException("JSCallInvokerHolder is not available yet."); | |
| } | |
| if (jsContextPointer == 0) { | |
| throw new RuntimeException("JSI runtime pointer is null. Make sure Hermes is enabled and the runtime is initialized before calling install()."); | |
| } | |
| CallInvokerHolderImpl jsCallInvokerHolder; | |
| if (context.getJSCallInvokerHolder() != null) { | |
| jsCallInvokerHolder = (CallInvokerHolderImpl) context.getJSCallInvokerHolder(); | |
| } else if (context.getCatalystInstance() != null) { | |
| var holder = context.getCatalystInstance().getJSCallInvokerHolder(); | |
| if (holder == null) { | |
| throw new RuntimeException("JSCallInvokerHolder from CatalystInstance is null."); | |
| } | |
| jsCallInvokerHolder = (CallInvokerHolderImpl) holder; | |
| } else { | |
| throw new RuntimeException("JSCallInvokerHolder is not available yet."); | |
| } |
| rm -rf "$MIGRATIONS_TARGET/migrations" | ||
| mkdir "$MIGRATIONS_TARGET/migrations" | ||
| cp -r ${SRCROOT}/../migrations ${MIGRATIONS_TARGET} | ||
| cp -r ${SRCROOT}/../prisma/migrations "${MIGRATIONS_TARGET}/migrations" |
There was a problem hiding this comment.
Quote the source path to prevent word splitting.
If SRCROOT contains spaces, this command will fail. Quote the variable expansion for robustness.
🛠️ Proposed fix
-cp -r ${SRCROOT}/../prisma/migrations "${MIGRATIONS_TARGET}/migrations"
+cp -r "${SRCROOT}/../prisma/migrations" "${MIGRATIONS_TARGET}/migrations"📝 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.
| cp -r ${SRCROOT}/../prisma/migrations "${MIGRATIONS_TARGET}/migrations" | |
| cp -r "${SRCROOT}/../prisma/migrations" "${MIGRATIONS_TARGET}/migrations" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 8-8: Double quote to prevent globbing and word splitting.
(SC2086)
| #include <TargetConditionals.h> | ||
| #if TARGET_OS_SIMULATOR | ||
| #include "../engines/ios/QueryEngine.xcframework/ios-arm64_x86_64-simulator/Headers/query_engine.h" | ||
| #else | ||
| #include "../engines/ios/QueryEngine.xcframework/ios-arm64/Headers/query_engine.h" | ||
| #endif |
There was a problem hiding this comment.
Missing platform guard will break non-Apple builds.
<TargetConditionals.h> is an Apple-only header. This code compiles on Android where this header doesn't exist, causing a build failure. Additionally, cpp/react-native-prisma.cpp:5 still uses #include "query_engine.h" directly, creating inconsistency.
Wrap the Apple-specific conditional in a platform check:
🐛 Proposed fix to add platform guard
+#if defined(__APPLE__)
`#include` <TargetConditionals.h>
`#if` TARGET_OS_SIMULATOR
`#include` "../engines/ios/QueryEngine.xcframework/ios-arm64_x86_64-simulator/Headers/query_engine.h"
`#else`
`#include` "../engines/ios/QueryEngine.xcframework/ios-arm64/Headers/query_engine.h"
`#endif`
+#else
+#include "query_engine.h"
+#endif📝 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.
| #include <TargetConditionals.h> | |
| #if TARGET_OS_SIMULATOR | |
| #include "../engines/ios/QueryEngine.xcframework/ios-arm64_x86_64-simulator/Headers/query_engine.h" | |
| #else | |
| #include "../engines/ios/QueryEngine.xcframework/ios-arm64/Headers/query_engine.h" | |
| #endif | |
| `#if` defined(__APPLE__) | |
| `#include` <TargetConditionals.h> | |
| `#if` TARGET_OS_SIMULATOR | |
| `#include` "../engines/ios/QueryEngine.xcframework/ios-arm64_x86_64-simulator/Headers/query_engine.h" | |
| `#else` | |
| `#include` "../engines/ios/QueryEngine.xcframework/ios-arm64/Headers/query_engine.h" | |
| `#endif` | |
| `#else` | |
| `#include` "query_engine.h" | |
| `#endif` |
🧰 Tools
🪛 Clang (14.0.6)
[error] 5-5: 'TargetConditionals.h' file not found
(clang-diagnostic-error)
| if (bridge && [bridge respondsToSelector:@selector(runtimeExecutor)]) { | ||
| facebook::react::RuntimeExecutor executor = RCTRuntimeExecutorFromBridge(bridge); | ||
| dispatch_semaphore_t sema = dispatch_semaphore_create(0); | ||
| executor([&](facebook::jsi::Runtime &runtime) { | ||
| InstallInRuntime(runtime, bridge.jsCallInvoker); | ||
| *okPtr = YES; | ||
| dispatch_semaphore_signal(sema); | ||
| }); | ||
| dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER); | ||
| return @(ok); | ||
| } |
There was a problem hiding this comment.
Potential deadlock if InstallInRuntime throws.
If InstallInRuntime throws an exception inside the executor block, dispatch_semaphore_signal is never called, causing dispatch_semaphore_wait to block indefinitely.
🐛 Proposed fix using try-catch
if (bridge && [bridge respondsToSelector:`@selector`(runtimeExecutor)]) {
facebook::react::RuntimeExecutor executor = RCTRuntimeExecutorFromBridge(bridge);
dispatch_semaphore_t sema = dispatch_semaphore_create(0);
+ __block NSException *caughtException = nil;
executor([&](facebook::jsi::Runtime &runtime) {
- InstallInRuntime(runtime, bridge.jsCallInvoker);
- *okPtr = YES;
+ `@try` {
+ InstallInRuntime(runtime, bridge.jsCallInvoker);
+ *okPtr = YES;
+ } `@catch` (NSException *e) {
+ caughtException = e;
+ }
dispatch_semaphore_signal(sema);
});
dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
+ if (caughtException) {
+ `@throw` caughtException;
+ }
return @(ok);
}Note: C++ exceptions from JSI may need different handling (e.g., std::exception_ptr). Consider using a C++ try-catch block instead if JSI throws C++ exceptions.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores