-
Notifications
You must be signed in to change notification settings - Fork 29
[WIP] Replace unmaintained packages #750
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
|
example/src/hooks.tsx
Outdated
| address, | ||
| save: async (address: string) => { | ||
| await EncryptedStorage.setItem('xmtp.address', address) | ||
| SecureStore.setItem('xmtp.address', address) |
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.
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) |
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.
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.
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 testin mobile CI workflows.react-native-encrypted-storage,react-native-fs,react-native-quick-crypto, andreact-native-sqlite-storagewithexpo-secure-store,expo-file-system,expo-crypto, andexpo-sqlite; update hooks and test helpers to use Expo APIsyarn testto Android and iOS CI workflows in test-android.yml and test-ios.ymlexpo-sqlitewithenableFTS: trueanduseSQLCipher: truein app.jsonnpx pod-installfrom quick start in README.md📍Where to Start
Start with the updated Expo-based utilities in
hooksand the new filesystem helpers: reviewuseSavedAddressandgetDbEncryptionKeyin 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
save: (address: string) => voidandclear: () => void, but the implementations areasyncfunctions returningPromise<void>. This contract mismatch can cause unhandled promise rejections if callers treat them as synchronous (as the types suggest). For example, ifSecureStore.setItemAsyncorrefetchrejects, the returned promise will reject, but a caller expectingvoidwill notcatchit, leading to unhandled rejection. This violates contract parity and makes error handling inconsistent. [ Out of scope ]useQueryare silently hidden: the hook returns onlyaddresswithout exposingstatus,error, or any fallback handling. If the storage call fails (e.g., due to the incorrectSecureStore.getItemusage 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 ]SecureStore.getItem('xmtp.address'), butexpo-secure-storeprovidesgetItemAsync.SecureStore.getItemis undefined and will throwTypeError: SecureStore.getItem is not a functionwhen the query runs (both initial load and onrefetch). This causes theuseQueryto error and preventsaddressfrom ever resolving. [ Already posted ]SecureStore.getItemis called instead of the correct async APISecureStore.getItemAsync, and it is not awaited. In Expo SecureStore, the method isgetItemAsync(key)and returns a Promise. CallingSecureStore.getItem(key)will throw at runtime (TypeError: SecureStore.getItem is not a function). Even if a similarly named function existed, failing toawaitthe Promise would cause downstream logic to treat a Promise as a string, breaking control flow. [ Already posted ]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 usingexpo-random. Ifcryptois undefined or lacksgetRandomValues, this will throw at runtime when generating the key. [ Low confidence ]SecureStore.getItemis not awaited and the wrong method is used,resultis not a string and downstream logic attempts to convert it withhexStringToUint8Array(result). Passing a non-string (or a Promise) intohexStringToUint8Arraywill causehexString.lengthto throw a runtime error, or produce incorrect behavior if coerced. [ Already posted ]ensureFileUriblindly prefixes withfile://when the path does not start withfile://. If a valid non-file URI (e.g.,content://on Android) is passed, this produces an invalid URI likefile://content://..., causingexpo-file-systemoperations to fail. The function should detect and preserve other URI schemes or validate input. [ Low confidence ]ensureFileUriblindly prependsfile://to any non-file://input, corrupting non-file URIs. For example, inputs likecontent://...,asset://...,http://..., ordata:...will becomefile://content://...which is an invalid URI and will likely causeFileSystem.getInfoAsyncto throw. This yields false negatives infileExistsand masks real errors. [ Already posted ]/dir/My File (final).txt) are returned asfile:////dir/My File (final).txt, which is not a strictly valid URI and may be rejected by some consumers. This can causegetInfoAsyncto throw or misinterpret the path. [ Low confidence ]ensureFileUridoes not handle empty or whitespace-only paths. For an empty string, it returns"file://", which is not a valid file URI and will likely causeFileSystem.getInfoAsyncto throw. The caller then returnsfalse, masking input errors and potentially hiding bugs upstream. [ Low confidence ]path.startsWith('file://')is case-sensitive, but URI schemes are case-insensitive. Inputs likeFILE://...orFile://...(which are valid forms of thefilescheme) will be incorrectly treated as non-file and rewritten tofile://FILE://..., producing an invalid URI. [ Low confidence ]"foo/bar"becomes"file://foo/bar", which is parsed as hostfooand path/bar, not a local file path. This likely breaksgetInfoAsyncand causes false negatives instead of either resolving relative to a known base or rejecting the input. [ Low confidence ]"C:\\foo\\bar"becomes"file://C:\\foo\\bar", which is not a valid file URI (wrong separators and missing third slashfile:///C:/...). This will likely fail in any URI consumer and cause false negatives. [ Low confidence ]"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 ]getFileSize, the code returnsinfo.sizewheninfo.existsis true, butinfo.sizecan beundefineddepending on platform or file type. Returningundefinedfrom a function declared to returnPromise<number>leads to downstream numeric operations producingNaN(e.g., when adding tofileSizeTotal). A safe fallback (e.g., returning0whensizeis not a number) is needed. [ Low confidence ]Bufferin React Native/Expo (Buffer.from(fileContent, 'base64')andBuffer.from(digest)), butBufferis not guaranteed to be defined in Expo without a polyfill. In environments lacking a globalBuffer, this will throwReferenceError: Buffer is not defined, breaking file digest calculation. [ Low confidence ]calculateFileDigestusesExpoCrypto.digestwith aBufferinput (buffer), but Expo'sexpo-cryptoAPI most commonly exposesdigestStringAsyncfor hashing strings and may not provide adigestmethod that accepts a NodeBuffer(or may expect anArrayBuffer). IfExpoCrypto.digestis undefined or rejects the input type at runtime, this will throw (TypeError/invalid input), breaking digest calculation and attachment verification. [ Low confidence ]ExpoCrypto.digestreturns a hex string (as someexpo-cryptoAPIs do), the codereturn 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
const DOCUMENT_DIRECTORY = documentDirectoryPath()callsensureDocumentDirectory()at module import time. IfFileSystem.documentDirectoryis 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'sFileSystem.documentDirectorybeing present. [ Low confidence ]states[0]without verifying thatstatescontains at least one element. IfClient.inboxStatesForInboxIds('local', [inboxId])returns an empty array,states[0]will beundefinedand accessing.installationswill throw at runtime before any assertion is reached. Add an explicit length check (or assert) before dereferencingstates[0]. [ Test / Mock code ]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 ]dropLocalDatabaseConnection()anddeleteLocalDatabase()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 ]states[0]without verifying thatstatescontains at least one element. IfClient.inboxStatesForInboxIds('local', [inboxId])returns an empty array,states[0]will beundefinedand accessing.installationswill throw at runtime. Add an explicit length check (or assert) before dereferencingstates[0]. [ Test / Mock code ]dropLocalDatabaseConnection()anddeleteLocalDatabase()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 ]dbDirectory:${basePath}_11``. Earlier code ensures directories only for indices 0..10. IfClient.createrequires 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 ]joinDocumentPath(...), which callsdocumentDirectoryPath()and ultimatelyensureDocumentDirectory()that throws ifFileSystem.documentDirectoryis unavailable. In environments where ExpoFileSystem.documentDirectoryis undefined (e.g., non-Expo environments or incorrect initialization), this test will crash before creating directories. Consider guarding for availability or skipping the test whendocumentDirectoryis not provided. [ Low confidence ]adaptEthersWalletToSignerusesdebugLoginsidesignMessage, butdebugLogis not defined anywhere. WhenalixSigner.signMessage(...)is invoked here, it will throw aReferenceErrorat runtime before attempting to sign. This occurs for both sign operations in this test. Fix by either definingdebugLog, removing the calls, or guarding them. [ Low confidence ]joinDocumentPath(...)which depends on ExpoFileSystem.documentDirectory. IfdocumentDirectoryis undefined in the runtime environment,ensureDocumentDirectory()throws, causing the test to fail before any client creation. This environment dependency was introduced by switching fromreact-native-fspaths to Expo helpers. Guard the test or ensure the environment providesdocumentDirectory. [ Low confidence ]adaptEthersWalletToSignerusesdebugLoginsidesignMessage, butdebugLogis not defined. Duringalix3.revokeInstallations(adaptEthersWalletToSigner(alixWallet), ...), the SDK will callsignMessageon the provided signer, causing aReferenceErrorat runtime. DefinedebugLog, remove the calls, or guard them. [ Low confidence ]boGroup?.send('hey')without verifying thatboGroupexists. IffindConversation(group.id)returnsundefineddue to sync timing or propagation delays, the send is silently skipped, but subsequent assertions assume the message was sent and expectmessages.length === 3. This leads to a confusing failure later rather than an immediate, clear error when the conversation is not found. [ Out of scope ]alixandalix2remain 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 ]crypto.getRandomValuesis used to create the encryption key for archives and databases. In some React Native environments,cryptois 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 ]toUri(path)fromfileSystemHelperswhich prefixesfile://to whatever string is passed. Whenpathcomes fromjoinDocumentPath(...), it is an absolute path starting with/. This produces a malformed URI likefile:////...(four slashes) instead of the canonicalfile:///.... Passing such a URI toexpo-file-systemmethods likegetInfoAsyncandmakeDirectoryAsynccan 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 callingensureDirectory(dbPath). [ Low confidence ]example/src/tests/conversationTests.ts — 0 comments posted, 3 evaluated, 3 filtered
joinDocumentPath(...)introduces a hard dependency onexpo-file-system'sdocumentDirectory. IfFileSystem.documentDirectoryis 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 usedRNFS.DocumentDirectoryPath, which may be available in non-Expo RN environments. This change narrows compatibility without a guard or fallback. [ Test / Mock code ]conversationTests: usingpathExists(dbDirPath)and thenensureDirectory(dbDirPath)withdbDirPathbuilt byjoinDocumentPath(...)will producefile:////...URIs under the hood, potentially causingexpo-file-systemto misinterpret the path. This risks false negatives on existence checks and failure to create the directory. [ Low confidence ]pathExistsreturnsinfo.existswithout checkinginfo.isDirectory, andensureDirectoryonly checks!info.existsbefore 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 passingdbDirectoryto 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
pathExistsreturnsinfo.existswithout distinguishing between files and directories. The tests usepathExiststo decide whether to create a directory. If a file exists at the target path,pathExistswill returntrueand 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 andisDirectory, or handle the directory-vs-file distinction at the call site. [ Low confidence ]ensureDirectorydoes not verify that an existing path is a directory. If a file exists atpath,FileSystem.getInfoAsyncwill returnexists: true, and the function will skipmakeDirectoryAsync. Callers that expectpathto be a directory (e.g., passing it asdbDirectory) may subsequently fail at runtime because they are actually pointing at a file. The function should checkinfo.isDirectoryand 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
pathExists(dbDirPath)reliably indicates that a directory exists atdbDirPath. IfpathExistsreturnstruefor any filesystem entry (file or directory), this can cause the code to skipensureDirectory(dbDirPath)when a file exists at that path. Subsequent operations that expect a directory fordbDirectorymay 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 callensureDirectoryunconditionally. IfpathExistsdoes not distinguish directories, it should be replaced with a function that checksisDirectoryor the code should fetchFileSystem.getInfoAsyncand verifyinfo.isDirectory === true. [ Low confidence ]create,build, andbuild with inboxId) and assumebuildis always faster thancreateandbuild with inboxIdis faster thanbuild. 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
dm2is obtained viaawait alix2.conversations.findConversation(dm.id), which can returnundefined. The code then usesdm2!(non-null assertion) atnew ConsentRecord(dm2!.id, 'conversation_id', 'allowed'). Ifdm2isundefineddue to timing or sync delays, accessingdm2.idwill throw a runtimeTypeError. Guard againstundefinedbefore dereferencing or assert/await until the conversation is present. [ Out of scope ]dm2can beundefinedfromfindConversation. The code usesdm2!again atawait alix2.preferences.conversationConsentState(dm2!.id). Ifdm2isundefined, accessing.idwill throw. Add a guard or ensure the conversation exists before dereferencing. [ Out of scope ]alix2Groupis obtained viaawait alix2.conversations.findConversation(alixGroup.id), which can returnundefined. The code then usesalix2Group!atawait alix2Group!.updateConsent('denied'). Ifalix2Groupisundefineddue to timing or sync delays, this will throw a runtimeTypeError. Guard forundefinedor ensure the group conversation exists before dereferencing. [ Out of scope ]dmis created byawait alix2.conversations.newConversation(bo.inboxId). IfnewConversationreturnsundefinedornulldue to an error or precondition, the non-null assertiondm!atawait dm!.updateConsent('denied')will cause a runtime failure. Add a guard to verifydmis defined before invoking methods. [ Out of scope ]alix.preferences.streamPreferenceUpdates(...)but later callsalix.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 ]