Skip to content

Conversation

@bennycode
Copy link
Member

@bennycode bennycode commented Oct 24, 2025

Replace unmaintained React Native storage/crypto packages with Expo equivalents and add unit test runs to Android and iOS CI workflows

Switch the example app to Expo storage, crypto, and filesystem modules and run yarn test in mobile CI workflows.

  • Replace react-native-encrypted-storage, react-native-fs, react-native-quick-crypto, and react-native-sqlite-storage with expo-secure-store, expo-file-system, expo-crypto, and expo-sqlite; update hooks and test helpers to use Expo APIs
  • Add yarn test to Android and iOS CI workflows in test-android.yml and test-ios.yml
  • Configure expo-sqlite with enableFTS: true and useSQLCipher: true in app.json
  • Remove npx pod-install from quick start in README.md

📍Where to Start

Start with the updated Expo-based utilities in hooks and the new filesystem helpers: review useSavedAddress and getDbEncryptionKey in hooks.tsx, then review path and directory utilities in fileSystemHelpers.ts.


📊 Macroscope summarized 746a4d9. 6 files reviewed, 46 issues evaluated, 45 issues filtered, 0 comments posted

🗂️ Filtered Issues

example/src/hooks.tsx — 0 comments posted, 19 evaluated, 18 filtered
  • line 760: The hook's returned function types declare save: (address: string) => void and clear: () => void, but the implementations are async functions returning Promise<void>. This contract mismatch can cause unhandled promise rejections if callers treat them as synchronous (as the types suggest). For example, if SecureStore.setItemAsync or refetch rejects, the returned promise will reject, but a caller expecting void will not catch it, leading to unhandled rejection. This violates contract parity and makes error handling inconsistent. [ Out of scope ]
  • line 763: Errors from the useQuery are silently hidden: the hook returns only address without exposing status, error, or any fallback handling. If the storage call fails (e.g., due to the incorrect SecureStore.getItem usage or other runtime errors), consumers cannot distinguish between "no saved address" and a failed query. This silent failure path violates the requirement to provide a defined and visible outcome for error inputs. [ Low confidence ]
  • line 765: The query function calls SecureStore.getItem('xmtp.address'), but expo-secure-store provides getItemAsync. SecureStore.getItem is undefined and will throw TypeError: SecureStore.getItem is not a function when the query runs (both initial load and on refetch). This causes the useQuery to error and prevents address from ever resolving. [ Already posted ]
  • line 787: SecureStore.getItem is called instead of the correct async API SecureStore.getItemAsync, and it is not awaited. In Expo SecureStore, the method is getItemAsync(key) and returns a Promise. Calling SecureStore.getItem(key) will throw at runtime (TypeError: SecureStore.getItem is not a function). Even if a similarly named function existed, failing to await the Promise would cause downstream logic to treat a Promise as a string, breaking control flow. [ Already posted ]
  • line 796: crypto.getRandomValues(randomBytes) may not be available in the React Native/Expo runtime without an explicit polyfill (e.g., react-native-get-random-values) or using expo-random. If crypto is undefined or lacks getRandomValues, this will throw at runtime when generating the key. [ Low confidence ]
  • line 806: Because the result of SecureStore.getItem is not awaited and the wrong method is used, result is not a string and downstream logic attempts to convert it with hexStringToUint8Array(result). Passing a non-string (or a Promise) into hexStringToUint8Array will cause hexString.length to throw a runtime error, or produce incorrect behavior if coerced. [ Already posted ]
  • line 834: The helper ensureFileUri blindly prefixes with file:// when the path does not start with file://. If a valid non-file URI (e.g., content:// on Android) is passed, this produces an invalid URI like file://content://..., causing expo-file-system operations to fail. The function should detect and preserve other URI schemes or validate input. [ Low confidence ]
  • line 835: ensureFileUri blindly prepends file:// to any non-file:// input, corrupting non-file URIs. For example, inputs like content://..., asset://..., http://..., or data:... will become file://content://... which is an invalid URI and will likely cause FileSystem.getInfoAsync to throw. This yields false negatives in fileExists and masks real errors. [ Already posted ]
  • line 835: No percent-encoding of path segments is performed. Inputs containing spaces or reserved characters (e.g., /dir/My File (final).txt) are returned as file:////dir/My File (final).txt, which is not a strictly valid URI and may be rejected by some consumers. This can cause getInfoAsync to throw or misinterpret the path. [ Low confidence ]
  • line 835: ensureFileUri does not handle empty or whitespace-only paths. For an empty string, it returns "file://", which is not a valid file URI and will likely cause FileSystem.getInfoAsync to throw. The caller then returns false, masking input errors and potentially hiding bugs upstream. [ Low confidence ]
  • line 835: The check path.startsWith('file://') is case-sensitive, but URI schemes are case-insensitive. Inputs like FILE://... or File://... (which are valid forms of the file scheme) will be incorrectly treated as non-file and rewritten to file://FILE://..., producing an invalid URI. [ Low confidence ]
  • line 835: Relative paths are turned into authority-based URIs. For example, "foo/bar" becomes "file://foo/bar", which is parsed as host foo and path /bar, not a local file path. This likely breaks getInfoAsync and causes false negatives instead of either resolving relative to a known base or rejecting the input. [ Low confidence ]
  • line 835: Windows paths are not normalized. An input like "C:\\foo\\bar" becomes "file://C:\\foo\\bar", which is not a valid file URI (wrong separators and missing third slash file:///C:/...). This will likely fail in any URI consumer and cause false negatives. [ Low confidence ]
  • line 835: The function does not recognize the alternate but still-seen form "file:/path" (single slash after scheme). Such inputs will be rewritten to "file://file:/path", producing an invalid URI rather than either accepting or normalizing them. [ Already posted ]
  • line 853: In getFileSize, the code returns info.size when info.exists is true, but info.size can be undefined depending on platform or file type. Returning undefined from a function declared to return Promise<number> leads to downstream numeric operations producing NaN (e.g., when adding to fileSizeTotal). A safe fallback (e.g., returning 0 when size is not a number) is needed. [ Low confidence ]
  • line 870: The code uses Buffer in React Native/Expo (Buffer.from(fileContent, 'base64') and Buffer.from(digest)), but Buffer is not guaranteed to be defined in Expo without a polyfill. In environments lacking a global Buffer, this will throw ReferenceError: Buffer is not defined, breaking file digest calculation. [ Low confidence ]
  • line 873: The function calculateFileDigest uses ExpoCrypto.digest with a Buffer input (buffer), but Expo's expo-crypto API most commonly exposes digestStringAsync for hashing strings and may not provide a digest method that accepts a Node Buffer (or may expect an ArrayBuffer). If ExpoCrypto.digest is undefined or rejects the input type at runtime, this will throw (TypeError/invalid input), breaking digest calculation and attachment verification. [ Low confidence ]
  • line 877: Potential incorrect hex conversion: If ExpoCrypto.digest returns a hex string (as some expo-crypto APIs do), the code return Buffer.from(digest).toString('hex') will return the hex of the UTF-8 bytes of the hex string, yielding an incorrect digest. This would cause content digest verification to fail even for matching files. [ Low confidence ]
example/src/tests/clientTests.ts — 0 comments posted, 15 evaluated, 15 filtered
  • line 24: Top-level initialization const DOCUMENT_DIRECTORY = documentDirectoryPath() calls ensureDocumentDirectory() at module import time. If FileSystem.documentDirectory is unavailable in the runtime (e.g., non-Expo React Native environment), ensureDocumentDirectory() throws, causing the entire test module to crash during import. This makes every test in the module fail before any test logic runs and introduces a hard runtime dependency on Expo's FileSystem.documentDirectory being present. [ Low confidence ]
  • line 98: Possible out-of-bounds access on states[0] without verifying that states contains at least one element. If Client.inboxStatesForInboxIds('local', [inboxId]) returns an empty array, states[0] will be undefined and accessing .installations will throw at runtime before any assertion is reached. Add an explicit length check (or assert) before dereferencing states[0]. [ Test / Mock code ]
  • line 98: Misleading assertion message: assert(states[0].installations.length === 2, 'should equal 5 installations'). The assertion verifies length equals 2, but the error message claims it should equal 5. This contradiction will mislead investigation if the test fails. [ Test / Mock code ]
  • line 127: Resource cleanup is not ensured on failure paths. Multiple asynchronous operations and assertions occur before calling dropLocalDatabaseConnection() and deleteLocalDatabase() for both clients. If any await or assertion throws earlier, these cleanups will not execute, potentially leaving database connections/files open. Wrap the critical section in a try/finally to guarantee cleanup on all exit paths. [ Test / Mock code ]
  • line 167: Possible out-of-bounds access on states[0] without verifying that states contains at least one element. If Client.inboxStatesForInboxIds('local', [inboxId]) returns an empty array, states[0] will be undefined and accessing .installations will throw at runtime. Add an explicit length check (or assert) before dereferencing states[0]. [ Test / Mock code ]
  • line 188: Resource cleanup is not ensured on failure paths. Multiple asynchronous operations and assertions occur before calling dropLocalDatabaseConnection() and deleteLocalDatabase() for both clients. If any await or assertion throws earlier, these cleanups will not execute, potentially leaving database connections/files open. Use a try/finally to guarantee cleanup. [ Test / Mock code ]
  • line 253: In the installation limit test, the directory for the 11th client is used without prior creation: dbDirectory: ${basePath}_11``. Earlier code ensures directories only for indices 0..10. If Client.create requires the directory to exist (consistent with prior explicit directory creation logic), this can throw at runtime when creating `sixthNow`. It also risks leaving inconsistent state and makes cleanup assumptions fragile. [ Out of scope ]
  • line 928: The test relies on joinDocumentPath(...), which calls documentDirectoryPath() and ultimately ensureDocumentDirectory() that throws if FileSystem.documentDirectory is unavailable. In environments where Expo FileSystem.documentDirectory is undefined (e.g., non-Expo environments or incorrect initialization), this test will crash before creating directories. Consider guarding for availability or skipping the test when documentDirectory is not provided. [ Low confidence ]
  • line 970: The signer created by adaptEthersWalletToSigner uses debugLog inside signMessage, but debugLog is not defined anywhere. When alixSigner.signMessage(...) is invoked here, it will throw a ReferenceError at runtime before attempting to sign. This occurs for both sign operations in this test. Fix by either defining debugLog, removing the calls, or guarding them. [ Low confidence ]
  • line 1055: The test uses joinDocumentPath(...) which depends on Expo FileSystem.documentDirectory. If documentDirectory is undefined in the runtime environment, ensureDocumentDirectory() throws, causing the test to fail before any client creation. This environment dependency was introduced by switching from react-native-fs paths to Expo helpers. Guard the test or ensure the environment provides documentDirectory. [ Low confidence ]
  • line 1094: The signer created by adaptEthersWalletToSigner uses debugLog inside signMessage, but debugLog is not defined. During alix3.revokeInstallations(adaptEthersWalletToSigner(alixWallet), ...), the SDK will call signMessage on the provided signer, causing a ReferenceError at runtime. Define debugLog, remove the calls, or guard them. [ Low confidence ]
  • line 1195: The archive test uses optional chaining boGroup?.send('hey') without verifying that boGroup exists. If findConversation(group.id) returns undefined due to sync timing or propagation delays, the send is silently skipped, but subsequent assertions assume the message was sent and expect messages.length === 3. This leads to a confusing failure later rather than an immediate, clear error when the conversation is not found. [ Out of scope ]
  • line 1212: The archive test does not clean up created clients or delete local databases/directories at the end of the test. alix and alix2 remain with active databases and potentially open connections. This can leak resources and leave residual state that may affect subsequent tests or retries. [ Out of scope ]
  • line 1288: crypto.getRandomValues is used to create the encryption key for archives and databases. In some React Native environments, crypto is not defined unless a polyfill (e.g., react-native-get-random-values) is installed and initialized. If unavailable, this will throw at runtime during test setup. There is no guard or fallback. This can cause test runs to crash in environments lacking the polyfill. [ Test / Mock code ]
  • line 1292: ensureDirectory/pathExists rely on toUri(path) from fileSystemHelpers which prefixes file:// to whatever string is passed. When path comes from joinDocumentPath(...), it is an absolute path starting with /. This produces a malformed URI like file:////... (four slashes) instead of the canonical file:///.... Passing such a URI to expo-file-system methods like getInfoAsync and makeDirectoryAsync can lead to failures or false negatives, causing directory existence checks to fail and directories not to be created as expected. This regression was introduced by replacing RNFS paths with the new helpers. The faulty usage occurs when calling ensureDirectory(dbPath). [ Low confidence ]
example/src/tests/conversationTests.ts — 0 comments posted, 3 evaluated, 3 filtered
  • line 150: Using joinDocumentPath(...) introduces a hard dependency on expo-file-system's documentDirectory. If FileSystem.documentDirectory is unavailable (e.g., running tests in a non-Expo environment or on platforms where the directory is not initialized), documentDirectoryPath() throws, causing the entire test to fail during setup. Previously, the code used RNFS.DocumentDirectoryPath, which may be available in non-Expo RN environments. This change narrows compatibility without a guard or fallback. [ Test / Mock code ]
  • line 153: Same malformed URI risk as above when checking/creating database directories in conversationTests: using pathExists(dbDirPath) and then ensureDirectory(dbDirPath) with dbDirPath built by joinDocumentPath(...) will produce file:////... URIs under the hood, potentially causing expo-file-system to misinterpret the path. This risks false negatives on existence checks and failure to create the directory. [ Low confidence ]
  • line 156: The existence and directory creation checks assume that a path existing means a directory exists. pathExists returns info.exists without checking info.isDirectory, and ensureDirectory only checks !info.exists before creating. If a regular file exists at the desired path, the code will incorrectly treat it as a directory and skip creation, leading to later failures when passing dbDirectory to the SDK or writing archives. This should explicitly validate that the path is a directory and either create it or throw a clear error if a non-directory exists at that path. [ Low confidence ]
example/src/tests/fileSystemHelpers.ts — 0 comments posted, 2 evaluated, 2 filtered
  • line 29: pathExists returns info.exists without distinguishing between files and directories. The tests use pathExists to decide whether to create a directory. If a file exists at the target path, pathExists will return true and the code will skip directory creation, leaving a file where a directory is expected. Subsequent operations using that path as a directory may fail. Consider returning both existence and isDirectory, or handle the directory-vs-file distinction at the call site. [ Low confidence ]
  • line 34: ensureDirectory does not verify that an existing path is a directory. If a file exists at path, FileSystem.getInfoAsync will return exists: true, and the function will skip makeDirectoryAsync. Callers that expect path to be a directory (e.g., passing it as dbDirectory) may subsequently fail at runtime because they are actually pointing at a file. The function should check info.isDirectory and either create the directory (if possible) or throw an error when a non-directory path exists. [ Low confidence ]
example/src/tests/groupPerformanceTests.ts — 0 comments posted, 2 evaluated, 2 filtered
  • line 39: The code assumes that pathExists(dbDirPath) reliably indicates that a directory exists at dbDirPath. If pathExists returns true for any filesystem entry (file or directory), this can cause the code to skip ensureDirectory(dbDirPath) when a file exists at that path. Subsequent operations that expect a directory for dbDirectory may fail at runtime (e.g., when the client attempts to create files within that path). To avoid this, either check that the path is specifically a directory before skipping creation, or call ensureDirectory unconditionally. If pathExists does not distinguish directories, it should be replaced with a function that checks isDirectory or the code should fetch FileSystem.getInfoAsync and verify info.isDirectory === true. [ Low confidence ]
  • line 96: The performance assertions compare measured durations across separate runs (create, build, and build with inboxId) and assume build is always faster than create and build with inboxId is faster than build. These measurements are susceptible to environmental fluctuations (JIT warm-up, I/O variability, device load), making the test potentially flaky. A temporary slowdown or GC pause could cause test failures unrelated to actual regressions. Consider adding tolerances, warming up operations, averaging multiple runs, or converting these to non-failing metrics reporting rather than hard assertions. [ Test / Mock code ]
example/src/tests/historySyncTests.ts — 0 comments posted, 5 evaluated, 5 filtered
  • line 85: Possible undefined dereference: dm2 is obtained via await alix2.conversations.findConversation(dm.id), which can return undefined. The code then uses dm2! (non-null assertion) at new ConsentRecord(dm2!.id, 'conversation_id', 'allowed'). If dm2 is undefined due to timing or sync delays, accessing dm2.id will throw a runtime TypeError. Guard against undefined before dereferencing or assert/await until the conversation is present. [ Out of scope ]
  • line 88: Possible undefined dereference: dm2 can be undefined from findConversation. The code uses dm2! again at await alix2.preferences.conversationConsentState(dm2!.id). If dm2 is undefined, accessing .id will throw. Add a guard or ensure the conversation exists before dereferencing. [ Out of scope ]
  • line 147: Possible undefined dereference: alix2Group is obtained via await alix2.conversations.findConversation(alixGroup.id), which can return undefined. The code then uses alix2Group! at await alix2Group!.updateConsent('denied'). If alix2Group is undefined due to timing or sync delays, this will throw a runtime TypeError. Guard for undefined or ensure the group conversation exists before dereferencing. [ Out of scope ]
  • line 149: Possible undefined dereference: dm is created by await alix2.conversations.newConversation(bo.inboxId). If newConversation returns undefined or null due to an error or precondition, the non-null assertion dm! at await dm!.updateConsent('denied') will cause a runtime failure. Add a guard to verify dm is defined before invoking methods. [ Out of scope ]
  • line 217: Incorrect cleanup API: The test starts a preferences stream via alix.preferences.streamPreferenceUpdates(...) but later calls alix.preferences.cancelStreamConsent(). This mismatched cancellation likely leaves the preference updates stream running, causing resource/callback leaks and violating single paired cleanup. Use the corresponding cancellation method for preference updates (e.g., cancelStreamPreferenceUpdates() or the API appropriate to the started stream). [ Out of scope ]

@bennycode bennycode requested a review from a team as a code owner October 24, 2025 09:39
@changeset-bot
Copy link

changeset-bot bot commented Oct 24, 2025

⚠️ No Changeset found

Latest commit: 746a4d9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@bennycode bennycode requested a review from a team as a code owner October 24, 2025 09:40
address,
save: async (address: string) => {
await EncryptedStorage.setItem('xmtp.address', address)
SecureStore.setItem('xmtp.address', address)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the fetcher in useQuery and the save logic in save call non-existent synchronous methods (SecureStore.getItem/setItem) and don’t await them, leading to undefined-method TypeErrors and race conditions (e.g. refetch() before storage). Update to await SecureStore.getItemAsync(...) in your query fetcher and await SecureStore.setItemAsync(...) in save to ensure the values exist and are persisted before proceeding.

-      SecureStore.setItem('xmtp.address', address)
+      await SecureStore.setItemAsync('xmtp.address', address)

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.


const result = await EncryptedStorage.getItem(key)
if ((result && clear === true) || !result) {
const result = SecureStore.getItem(key)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SecureStore calls in getDbEncryptionKey are never awaited: both the retrieval and storage methods return promises, so the function returns before persistence completes, swallows storage errors, mis-evaluates the stored-key condition (a Promise is always truthy) and even passes a Promise into hexStringToUint8Array, causing runtime errors. Consider using await SecureStore.getItemAsync(...) and await SecureStore.setItemAsync(...) so persistence finishes before returning, errors propagate, and you operate on the resolved string.

-    const result = SecureStore.getItem(key)
+    const result = await SecureStore.getItem(key)

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.

@bennycode bennycode changed the title Replace unmaintained packages [WIP] Replace unmaintained packages Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants