support new arch, Expo 53+#59
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:
WalkthroughThis PR adds a synchronous native Prisma execution entry point, updates JavaScript proxy routing and type contracts, patches the installed React Native runtime at install time, and changes platform-specific installation and header-resolution logic. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/react-native-prisma.cpp`:
- Around line 203-206: The query response path in the Prisma JSI bridge can
crash when prisma_query returns a null response without setting error_ptr.
Update the response handling in react-native-prisma.cpp near the code that
creates the JSI string to mirror the transaction handler null-response guard:
check response for null before calling jsi::String::createFromUtf8(rt,
response), and return a proper error/empty result path instead of converting a
null pointer.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e65979e-48b5-4fa6-9ea0-49510fe14253
📒 Files selected for processing (2)
cpp/react-native-prisma.cppsrc/index.ts
| if (error_ptr == nullptr) { | ||
| auto js_response = jsi::String::createFromUtf8(rt, response); | ||
| free_engine_string(response); | ||
| return js_response; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard null query responses before creating a JSI string.
If prisma_query returns nullptr without setting error_ptr, createFromUtf8(rt, response) can crash. Mirror the transaction handlers’ null-response guard before conversion.
Proposed fix
if (error_ptr == nullptr) {
+ if (response == nullptr) {
+ throw std::runtime_error("prisma engine did not return a query response");
+ }
auto js_response = jsi::String::createFromUtf8(rt, response);
free_engine_string(response);
return js_response;📝 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 (error_ptr == nullptr) { | |
| auto js_response = jsi::String::createFromUtf8(rt, response); | |
| free_engine_string(response); | |
| return js_response; | |
| if (error_ptr == nullptr) { | |
| if (response == nullptr) { | |
| throw std::runtime_error("prisma engine did not return a query response"); | |
| } | |
| auto js_response = jsi::String::createFromUtf8(rt, response); | |
| free_engine_string(response); | |
| return js_response; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/react-native-prisma.cpp` around lines 203 - 206, The query response path
in the Prisma JSI bridge can crash when prisma_query returns a null response
without setting error_ptr. Update the response handling in
react-native-prisma.cpp near the code that creates the JSI string to mirror the
transaction handler null-response guard: check response for null before calling
jsi::String::createFromUtf8(rt, response), and return a proper error/empty
result path instead of converting a null pointer.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/patch-prisma-runtime.cjs`:
- Around line 19-22: The runtime patch in patch-prisma-runtime.cjs can end up
writing code that references __prismaReactNativeFastRead without having inserted
the helper first. Update the patch flow around the source.replace logic to
verify the marker/helper was actually injected before any later replacements or
writeFileSync output, and treat a missing "use strict"; match as an unsupported
runtime shape that should fail early instead of emitting a broken runtime.
- Around line 72-77: The postinstall patch in patchPrismaRuntime currently only
warns when the Prisma React Native runtime patch fails, which lets installation
continue with an unpatched runtime. Update the catch path in patchPrismaRuntime
to treat failures as non-recoverable: propagate the error, rethrow it, or set a
non-zero exit code after logging, so unsupported runtime shapes, permission
issues, and write failures cause installation to fail. Keep the existing error
context from the catch block, but ensure the failure is surfaced instead of
silently succeeding.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78c68b34-b6f8-4f23-941a-6449eacd90a8
📒 Files selected for processing (2)
package.jsonscripts/patch-prisma-runtime.cjs
| source = source.replace( | ||
| '"use strict";', | ||
| `"use strict";var ${marker}=e=>{try{let t=e?.query?.arguments??{};if(t.distinct!=null)return!1;if(e?.action==="findUnique"||e?.action==="findFirst")return!0;return e?.action==="findMany"&&typeof t.take==="number"&&Number.isFinite(t.take)&&Math.abs(t.take)<=100}catch{return!1}};` | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify the helper insertion before writing the patched runtime.
If "use strict"; is absent but the later string matches still succeed, Line 53 writes a runtime that calls an undefined __prismaReactNativeFastRead. Treat a missed helper insertion as an unsupported runtime shape.
Proposed fix
- source = source.replace(
- '"use strict";',
- `"use strict";var ${marker}=e=>{try{let t=e?.query?.arguments??{};if(t.distinct!=null)return!1;if(e?.action==="findUnique"||e?.action==="findFirst")return!0;return e?.action==="findMany"&&typeof t.take==="number"&&Number.isFinite(t.take)&&Math.abs(t.take)<=100}catch{return!1}};`
- );
+ const strictHeader = '"use strict";';
+ if (!source.includes(strictHeader)) {
+ throw new Error(
+ `Unsupported `@prisma/client` react-native strict header shape: ${runtimePath}`
+ );
+ }
+
+ source = source.replace(
+ strictHeader,
+ `${strictHeader}var ${marker}=e=>{try{let t=e?.query?.arguments??{};if(t.distinct!=null)return!1;if(e?.action==="findUnique"||e?.action==="findFirst")return!0;return e?.action==="findMany"&&typeof t.take==="number"&&Number.isFinite(t.take)&&Math.abs(t.take)<=100}catch{return!1}};`
+ );📝 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.
| source = source.replace( | |
| '"use strict";', | |
| `"use strict";var ${marker}=e=>{try{let t=e?.query?.arguments??{};if(t.distinct!=null)return!1;if(e?.action==="findUnique"||e?.action==="findFirst")return!0;return e?.action==="findMany"&&typeof t.take==="number"&&Number.isFinite(t.take)&&Math.abs(t.take)<=100}catch{return!1}};` | |
| ); | |
| const strictHeader = '"use strict";'; | |
| if (!source.includes(strictHeader)) { | |
| throw new Error( | |
| `Unsupported `@prisma/client` react-native strict header shape: ${runtimePath}` | |
| ); | |
| } | |
| source = source.replace( | |
| strictHeader, | |
| `${strictHeader}var ${marker}=e=>{try{let t=e?.query?.arguments??{};if(t.distinct!=null)return!1;if(e?.action==="findUnique"||e?.action==="findFirst")return!0;return e?.action==="findMany"&&typeof t.take==="number"&&Number.isFinite(t.take)&&Math.abs(t.take)<=100}catch{return!1}};` | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/patch-prisma-runtime.cjs` around lines 19 - 22, The runtime patch in
patch-prisma-runtime.cjs can end up writing code that references
__prismaReactNativeFastRead without having inserted the helper first. Update the
patch flow around the source.replace logic to verify the marker/helper was
actually injected before any later replacements or writeFileSync output, and
treat a missing "use strict"; match as an unsupported runtime shape that should
fail early instead of emitting a broken runtime.
| } catch (error) { | ||
| console.warn( | ||
| `Could not patch Prisma React Native runtime: ${ | ||
| error instanceof Error ? error.message : String(error) | ||
| }` | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently succeed when the runtime patch fails.
This postinstall hook is the only automated path that applies the fast-read runtime changes. Unsupported runtime shapes, permission errors, or write failures should fail installation or set a non-zero exit code; otherwise consumers can run with an unpatched Prisma runtime without noticing.
Proposed fix
} catch (error) {
- console.warn(
+ const message =
+ error instanceof Error ? error.message : String(error);
+
+ console.error(
`Could not patch Prisma React Native runtime: ${
- error instanceof Error ? error.message : String(error)
+ message
}`
);
+ process.exitCode = 1;
}📝 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.
| } catch (error) { | |
| console.warn( | |
| `Could not patch Prisma React Native runtime: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }` | |
| ); | |
| } catch (error) { | |
| const message = | |
| error instanceof Error ? error.message : String(error); | |
| console.error( | |
| `Could not patch Prisma React Native runtime: ${message}` | |
| ); | |
| process.exitCode = 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/patch-prisma-runtime.cjs` around lines 72 - 77, The postinstall patch
in patchPrismaRuntime currently only warns when the Prisma React Native runtime
patch fails, which lets installation continue with an unpatched runtime. Update
the catch path in patchPrismaRuntime to treat failures as non-recoverable:
propagate the error, rethrow it, or set a non-zero exit code after logging, so
unsupported runtime shapes, permission issues, and write failures cause
installation to fail. Keep the existing error context from the catch block, but
ensure the failure is surfaced instead of silently succeeding.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores