-
Couldn't load subscription status.
- Fork 28
D14z client prerelease updates #736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
Claude finished @cameronvoell's task —— View job Pull Request Review ✅SummaryThis PR adds support for an optional Code Quality ✅
Potential Issues
|
| if (jsonOptions.has("dbDirectory")) jsonOptions.get("dbDirectory").asString else null, | ||
| if (jsonOptions.has("historySyncUrl")) jsonOptions.get("historySyncUrl").asString else null, | ||
| if (jsonOptions.has("customLocalUrl")) jsonOptions.get("customLocalUrl").asString else null, | ||
| if (jsonOptions.has("dbDirectory")) stringOrNull(jsonOptions.get("dbDirectory").asString) else null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsonOptions.has(key) returns true even if the value is JsonNull. Calling asString on such values throws IllegalStateException. This can crash when dbDirectory, historySyncUrl, customLocalUrl, appVersion, or gatewayUrl are present but explicitly null in the JSON.
Consider guarding these reads with an isJsonNull check (e.g., only call asString when the value exists and is not JsonNull), and otherwise treat them as null so they flow through stringOrNull(...) or default to null.
- if (jsonOptions.has("dbDirectory")) stringOrNull(jsonOptions.get("dbDirectory").asString) else null,
- if (jsonOptions.has("historySyncUrl")) stringOrNull(jsonOptions.get("historySyncUrl").asString) else null,
- if (jsonOptions.has("customLocalUrl")) stringOrNull(jsonOptions.get("customLocalUrl").asString) else null,
+ if (jsonOptions.has("dbDirectory") && !jsonOptions.get("dbDirectory").isJsonNull) stringOrNull(jsonOptions.get("dbDirectory").asString) else null,
+ if (jsonOptions.has("historySyncUrl") && !jsonOptions.get("historySyncUrl").isJsonNull) stringOrNull(jsonOptions.get("historySyncUrl").asString) else null,
+ if (jsonOptions.has("customLocalUrl") && !jsonOptions.get("customLocalUrl").isJsonNull) stringOrNull(jsonOptions.get("customLocalUrl").asString) else null,
if (jsonOptions.has("deviceSyncEnabled")) jsonOptions.get("deviceSyncEnabled").asBoolean else true,
if (jsonOptions.has("debugEventsEnabled")) jsonOptions.get("debugEventsEnabled").asBoolean else false,
- if (jsonOptions.has("appVersion")) stringOrNull(jsonOptions.get("appVersion").asString) else null,
- if (jsonOptions.has("gatewayUrl")) stringOrNull(jsonOptions.get("gatewayUrl").asString) else null,
+ if (jsonOptions.has("appVersion") && !jsonOptions.get("appVersion").isJsonNull) stringOrNull(jsonOptions.get("appVersion").asString) else null,
+ if (jsonOptions.has("gatewayUrl") && !jsonOptions.get("gatewayUrl").isJsonNull) stringOrNull(jsonOptions.get("gatewayUrl").asString) else null,
)🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
6d1265e to
ab41c70
Compare
Add
gatewayUrlsupport across JS, Android, and iOS client creation paths and update XMTP dependencies for D14z client prerelease in index.ts and native modulesThis pull request introduces a
gatewayUrlparameter to the JS APIs and native client configuration on Android and iOS, and updates XMTP dependencies to prerelease versions. It extends auth parameter parsing to normalize empty strings and propagatesgatewayUrlintoClientOptions.Apifor environment-specific clients.gatewayUrlviaAuthParamsinindex.tsandClientfactories inClient.tsgatewayUrlto native client configuration on Android inXMTPModule.ktandAuthParamsWrapperinAuthParamsWrapper.ktgatewayUrlto native client configuration on iOS inXMTPModule.swiftandAuthParamsWrapperinAuthParamsWrapper.swiftnull/nilin Android and iOS wrappers4.6.0-dev.c78f91finbuild.gradleand XMTP iOS pod to4.6.0-dev.4a8ee5dinXMTPReactNative.podspecwith example lock updates inPodfile.lockgatewayUrlusage and client rebuild inclientTests.ts5.1.0-devinpackage.json📍Where to Start
Start with the JS entry points that introduce
gatewayUrlinindex.createRandom,index.create,index.build, andindex.ffiCreateClientinindex.ts, then follow propagation throughClientfactories inClient.tsinto AndroidXMTPModule.apiEnvironmentsand iOSXMTPModule.createApiClient.📊 Macroscope summarized ab41c70. 6 files reviewed, 23 issues evaluated, 22 issues filtered, 0 comments posted
🗂️ Filtered Issues
android/src/main/java/expo/modules/xmtpreactnativesdk/XMTPModule.kt — 0 comments posted, 4 evaluated, 3 filtered
preAuthenticateToInboxCallbackDeferredbeing overwritten without synchronization or per-instance scoping. IfclientOptions(...)is called multiple times withhasPreAuthenticateToInboxCallback == truebefore prior callback flows complete,preAuthenticateToInboxCallbackDeferredis reassigned, causing any ongoingawait()in the previously created callback to now wait on the new deferred. The old deferred becomes orphaned and will never be completed, and the first flow can deadlock or wait on the wrong completion signal. This violates at-most-once callback consumption, atomicity, and no-leak/no-double-apply guarantees. [ Low confidence ]preAuthenticateToInboxCallbackDeferredwhenhasPreAuthenticateToInboxCallbackis false: The function setspreAuthenticateToInboxCallbackDeferredonly whenhasPreAuthenticateToInboxCallback == trueand never clears it when false. If a previous call left a non-null deferred, a subsequent call without a callback leaves the stale deferred in memory and accessible, violating the 'no leaks' requirement and potentially causing confusion or incorrect behavior if other parts of the module read it. [ Invalidated by documentation search ]AuthParamsWrapper.authParamsFromJson(authParams)can causeclientOptions(...)to fail catastrophically without a graceful outcome. IfauthParamsis malformed JSON or missing required fields (e.g.,environment),authParamsFromJsoncan throw, andclientOptionsdoes not catch this. This violates the requirement that all inputs (including empty or invalid) must reach a defined terminal state with a visible outcome. In addition, the failure can leave earlier side effects (see related issues) in an inconsistent state. [ Invalidated by documentation search ]android/src/main/java/expo/modules/xmtpreactnativesdk/wrappers/AuthParamsWrapper.kt — 0 comments posted, 4 evaluated, 4 filtered
JsonParser.parseString(authParams).asJsonObjectassumesauthParamsis valid JSON and a JSON object. IfauthParamsis malformed JSON or a JSON array/primitive,asJsonObjectwill throw at runtime (e.g.,IllegalStateException). There is no guard or graceful fallback, so any bad input crashes the call path. [ Low confidence ]jsonOptions.get("environment").asStringdereferences theenvironmentmember without checking existence or nullability. If theenvironmentkey is missing or isJsonNull/non-string, this will throw at runtime (e.g.,NullPointerExceptionorIllegalStateException). There is no fallback for mandatoryenvironment. [ Low confidence ]jsonOptions.has(key)to decide whether to readasString, buthas(key)returns true even when the value isJsonNull. In such cases,jsonOptions.get(key).asStringwill throwIllegalStateException. This affectsdbDirectory,historySyncUrl,customLocalUrl,appVersion, andgatewayUrl. [ Invalidated by documentation search ]deviceSyncEnabledanddebugEventsEnabledreadasBooleanwhen the key exists. If the value isJsonNullor a non-boolean type,asBooleanwill throw.JsonObject.has(key)returns true for keys withJsonNull, so explicit nulls cause runtime exceptions. There is no type/nullable guard beforeasBoolean. [ Invalidated by documentation search ]example/src/tests/clientTests.ts — 0 comments posted, 4 evaluated, 4 filtered
RNFS.DocumentDirectoryPath + '/xmtp_db'if it doesn't exist and leaves it in place. Subsequent test runs may operate on residual state, potentially affecting reproducibility or causing false positives/negatives. To preserve invariants across runs, consider cleaning up the directory after the test or using a uniquely-scoped temporary directory. [ Code style ]Configmay throw before the intended validation. The code checksif (!Config.GATEWAY_URL)to validate the environment variable, but if thereact-native-configmodule isn't properly initialized orConfigisundefinedat runtime, accessingConfig.GATEWAY_URLwill throw aTypeError(cannot read properties of undefined) before reaching the guard, causing the test to crash. Add a preceding check (e.g.,if (!Config || !Config.GATEWAY_URL)) to ensure a defined object before property access. [ Test / Mock code ]gatewayUrl: The test configures the client using a customgatewayUrl(options.gatewayUrl), butClient.getOrCreateInboxIdonly acceptsidentityandenvand does not take or usegatewayUrl. If the XMTP stack uses thegatewayUrlto route requests, callinggetOrCreateInboxIdwithout it may hit a different gateway than the one used to create the client, potentially yielding a different inboxId and causing theinboxIds should matchassertion to fail. This breaks parity between client creation/build paths and this lookup path. [ Test / Mock code ]Client.getOrCreateInboxIdis called with a hardcoded'dev'value (env: 'dev' as XMTPEnvironment) instead of reusingoptions.env. Ifoptions.envdiffers from'dev'(now or in future refactors), the test will derive the inboxId from a different environment than the one used to create the client, potentially causing the subsequent equality assertion to fail and violating contract parity across code paths. [ Test / Mock code ]ios/Wrappers/AuthParamsWrapper.swift — 0 comments posted, 2 evaluated, 2 filtered
environmentis not normalized usingstringOrNillike other string fields, so an empty string value from JSON (e.g.,"environment":"") will be preserved as""instead of defaulting to"dev"ornil. This creates an inconsistency in normalization compared todbDirectory,historySyncUrl,customLocalUrl,appVersion, andgatewayUrl, and can propagate an invalid/empty environment value through the system, violating the canonicalization applied to other fields. [ Low confidence ]deviceSyncEnabledanddebugEventsEnabledusingas? Boolcan silently misinterpret valid but non-boolean JSON inputs (e.g.,0/1numbers or string values like"false"). In such cases, the code defaults totruefordeviceSyncEnabledandfalsefordebugEventsEnabled, potentially enabling device sync when it was intended to be disabled or vice versa. Since inputs are unconstrained andJSONSerializationmay yieldNSNumberfor0/1, this creates a runtime misconfiguration risk without error reporting or normalization. [ Low confidence ]src/index.ts — 0 comments posted, 5 evaluated, 5 filtered
hasPreAuthenticateToInboxCallbackis optional and forwarded directly toXMTPModule.create/XMTPModule.createRandom. If the native layer expects a strict boolean, passingundefinedcan cause runtime errors or unintended behavior. Consider defaulting tofalseor explicitly mappingundefinedto a well-defined value per API contract. [ Invalidated by documentation search ]chainIdandblockNumberare only checked withtypeof === 'number', which allowsNaN,Infinity, and-Infinityto pass through intosignerParams. These values are not valid chain IDs or block numbers and can cause downstream runtime failures in the nativeXMTPModule.createhandler or any numeric validation expecting finite positive integers. Consider validating withNumber.isFinite(...)and domain checks (e.g.,> 0) before serialization. [ Low confidence ]hasPreAuthenticateToInboxCallbackis optional and forwarded directly toXMTPModule.createin thecreate(...)path. Ifundefinedis passed where a strict boolean is expected on the native side, it can cause runtime errors or misbehavior. Normalize to a boolean or explicitly handle absence. [ Invalidated by documentation search ]inboxIdis optional and passed directly toXMTPModule.buildwithout normalization or guarding. If the native bridge expects a non-null string, passingundefinedcan lead to runtime exceptions or misinterpretation (e.g., the string 'undefined'). Consider explicitly passingnullor omitting the parameter per native contract, or validate and throw/reject early wheninboxIdis absent. [ Invalidated by documentation search ]Clientusesoptions.env, whileAuthParamsdefinesenvironment. This asymmetry can lead to runtime confusion or misconfiguration if a caller constructs options according toAuthParamsand expectsenvironmentto be used;Clientwill readenvinstead, potentially yieldingundefinedand misconfiguring the environment. This issue is pre-existing and not introduced by thegatewayUrlchange, but it presents a runtime contract mismatch risk. [ Out of scope ]src/lib/Client.ts — 0 comments posted, 4 evaluated, 4 filtered
XMTPModule.createRandomnow forwardsoptions.gatewayUrlwithout any guard or normalization. Ifoptionsis present butoptions.gatewayUrlisundefined,null, or an invalid type/value for the native bridge, this can cause a runtime failure inside the module or yield misconfigured client behavior. There is no fallback or default value. Since this is an external-effect call (creating a client), the lack of validation means an invalid or missinggatewayUrlcan lead to errors after side-effects begin, with no defined terminal outcome. [ Invalidated by documentation search ]XMTPModule.buildnow forwardsoptions.gatewayUrlas a new trailing argument (options.appVersion, options.gatewayUrl). BecausegatewayUrlis optional in the options object, it can beundefinedat runtime. IfXMTPModule.buildexpects a non-empty string and uses it to construct a URL or perform network configuration, passingundefinedis a reachable input that can cause malformed configuration or runtime failures (e.g., URL parsing errors, HTTP client misconfiguration) without any guard or fallback. There is no validation or defaulting ofoptions.gatewayUrlbefore passing it to the lower-level builder. [ Invalidated by documentation search ]options.gatewayUrl) toXMTPModule.buildcan cause runtime misbinding if the callee relies on argument count to select behavior (e.g., overload-like branching onarguments.length) or expects a specific arity. In JavaScript, extra arguments are generally ignored, but native/bridged modules or code that inspectsarguments.lengthor positional offsets could misinterpret the parameters. Without visible confirmation thatXMTPModule.buildaccepts the new parameter at that position, there is a risk of mismatched contract at runtime. [ Low confidence ]XMTPModule.ffiCreateClientnow includes a new trailing argumentoptions.gatewayUrl. If the native bridge or FFI binding expects a fixed arity and does not tolerate extra arguments, this can cause a runtime error or misinterpretation of parameters on the native side. JavaScript functions ignore extra arguments, but many native module bridges (e.g., React Native/FFI) map all passed arguments by position; an unexpected extra argument can break the call at runtime. [ Low confidence ]