feat: migrate react-native-mmkv-storage to react-native-mmkv #6744
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughDeleted legacy react-native-mmkv-storage mocks and integrated react-native-mmkv with native MMKVBridge/TurboModule readers, added cross-platform SecureStorage/SecureKeystore modules, implemented MMKV migration flows, updated app initialization and preferences, and adjusted native build/project configs for MMKV integration. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Migrator as migrateFromOldMMKV()
participant Turbo as NativeMMKVReader (TurboModule)
participant Old as Legacy MMKV
participant New as react-native-mmkv / MMKVBridge
App->>Migrator: call migrateFromOldMMKV()
Migrator->>Turbo: readAndDecryptMMKV(instanceId)
Turbo->>Old: open instance (with/without password) and read keys
Old-->>Turbo: key-value map
Turbo-->>Migrator: decrypted data
Migrator->>New: write keys by type (string/number/bool)
New-->>Migrator: ack per-key
Migrator->>App: mark WORKSPACE_MIGRATION_COMPLETED
sequenceDiagram
participant JS as JS getSecureKey()
participant Native as SecureStorage (native)
participant Store as Keychain/SharedPrefs
JS->>Native: getSecureKey(hexAlias)
Native->>Store: fetch stored Base64(iv+ciphertext)
alt stored value exists
Native->>Native: Base64 decode, split IV, decrypt with AES-GCM
Native-->>JS: plaintext or null
else missing
Native-->>JS: null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
patches/react-native-webview+13.15.0.patch (2)
55-60: Handle client certificate failure path and nulls inonReceivedClientCertRequestThe new client‑cert handling is on the right track, but there are two important gaps:
- If
KeyChain.getPrivateKey/getCertificateChainfails or returns null,doInBackgroundreturnsnullandonPostExecutedoes nothing. That means neitherrequest.proceed,request.cancel, norrequest.ignoreis ever called, which can leave the TLS handshake hanging or failing in undefined ways.privKeyorcertChaincan individually be null; you should guard against that before callingrequest.proceed.Consider something along these lines:
- protected void onPostExecute(SslStuff sslStuff) { - if (sslStuff != null) { - request.proceed(sslStuff.privKey, sslStuff.certChain); - } - } + protected void onPostExecute(SslStuff sslStuff) { + if (sslStuff != null && sslStuff.privKey != null && sslStuff.certChain != null) { + request.proceed(sslStuff.privKey, sslStuff.certChain); + } else { + request.cancel(); // or super.onReceivedClientCertRequest(view, request); + } + }Also, logging the exception in
doInBackground(instead of silently returning null) would greatly help diagnosing cert/KeyChain issues.Also applies to: 67-122
40-46: Fix critical typo and resolve missing ReactContext methodTwo blocking issues in the patch:
Critical typo in
node_modules/react-native-webview/android/src/oldarch/com/reactnativecommunity/webview/RNCWebViewModule.javaline ~43:mRNCWxebViewModuleImplshould bemRNCWebViewModuleImpl. This causes a compile error.Missing method: Line 43 in
RNCWebView.javacallsthis.getReactApplicationContext(), butRNCWebViewextendsWebViewwhich doesn't provide this method. Either addgetReactApplicationContext()toRNCWebViewor passReactContextvia constructor/field.Missing error handling in
RNCWebViewClient.onReceivedClientCertRequest(): WhenKeyChainoperations fail andsslStuffis null, the code does nothing inonPostExecute(). Addrequest.cancel()orrequest.ignore()to properly reject the cert request on failure.ios/RocketChatRN.xcodeproj/project.pbxproj (1)
274-291: SecureStorage.m / MMKVBridge.mm appear multiple times in Sources; likely duplicate‑symbol linker errorsRight now the same physical files seem to be wired into certain targets more than once:
- Multiple PBXFileReference/PBXBuildFile entries for
SecureStorage.mandMMKVBridge.mm(e.g.,66C2701F… SecureStorage.m,9B215A42… SecureStorage.m, and their various… in Sourcesbuild files).- In
13B07F871A680F5B00A75B9A /* Sources */(RocketChatRN), both66C270212E… /* SecureStorage.m in Sources */andA2C6E2DD38… /* SecureStorage.m in Sources */are included.- In
7AAB3E14257E6A6E00707CF6 /* Sources */(Rocket.Chat), you also have both66C270202E…and79D8C97F8C…entries forSecureStorage.m.- Similar duplication pattern exists for
MMKVBridge.mmacross PBXBuildFile entries and multiple Sources phases.If these entries all point at the same .m/.mm files (which they appear to), Xcode will compile them twice for a given target and you’ll hit duplicate symbol errors at link time.
Recommendation:
- For each target, ensure at most one
… in Sourcesentry per actual file (one BuildFile per FileReference per target).- Prefer a single canonical FileReference for
SecureStorage.{h,m}andMMKVBridge.mmunder the main group hierarchy, and drop the extra “Recovered References”/duplicate references.Also applies to: 360-361, 587-590, 607-608, 612-613, 690-692, 709-710, 1143-1145, 1188-1193, 2146-2196, 2382-2418, 2421-2471
🧹 Nitpick comments (26)
app/lib/methods/helpers/getSecureKey.ts (2)
5-11: AlignISecureStoragetyping with native module behaviorThe TS interface models
setSecureKeyasPromise<void>, but the Android native implementation currently resolves a boolean (promise.resolve(true)). That’s harmless here (you ignore the return value), but it creates a subtle mismatch between JS typings and the native API and can confuse future callers.Consider either:
- Updating
ISecureStorage.setSecureKeyto something likePromise<boolean | void>orPromise<unknown>, or- Normalizing the native side to resolve
null/voidfor symmetry.Not urgent, but worth aligning to keep the bridge contract explicit.
43-55: MakegetAppGroupPathresilient whenSecureStorageis missingRight now you only optional‑chain the method (
SecureStorage.getAppGroupPath?.()), so ifSecureStorageitself is undefined (e.g., certain test environments), you’ll hit a TypeError that is then caught and logged.You can avoid even that by optional‑chaining the module as well:
- const appGroupPath = await SecureStorage.getAppGroupPath?.(); + const appGroupPath = await SecureStorage?.getAppGroupPath?.();This keeps behavior the same (returns
nullwhen the module or method is absent) but avoids spurious errors in logs.app/lib/methods/helpers/sslPinning.ts (1)
37-44: Add consistent null guard for hostname inremoveCertificate.For consistency with the defensive coding added to
persistCertificate(lines 29-32), consider adding a null guard when callingextractHostname(server)on line 42.Apply this diff:
const removeCertificate = (server: string) => { const certificate = UserPreferences.getString(`${CERTIFICATE_KEY}-${server}`); if (certificate) { UserPreferences.removeItem(certificate); } - UserPreferences.removeItem(extractHostname(server)); + const hostname = extractHostname(server); + if (hostname) { + UserPreferences.removeItem(hostname); + } UserPreferences.removeItem(`${CERTIFICATE_KEY}-${server}`); };app/views/MediaAutoDownloadView/index.tsx (1)
38-42: Explicit MediaDownloadOption casts are fine; consider tightening types upstreamThese casts are safe given the
useUserPreferences<MediaDownloadOption>usage and help the compiler, but if we see more of these it may be worth adjustingListPicker(oruseUserPreferences) prop/return types so the union flows through without needingasat call sites.ios/Podfile (1)
69-92: Post_install MMKV/webview tweaks are reasonable; consider minor guardsThe additional
HEADER_SEARCH_PATHSforreact-native-webviewandFORCE_POSIX=1defines forreact-native-mmkvand the app targets are a pragmatic way to make the MMKV C++ API available. Two small considerations:
- You might want to guard the extra header path with an
include?check similar to the FORCE_POSIX block to avoid duplicates if post_install runs multiple times.- Since this touches Xcode build settings for multiple targets, please verify both Debug/Release builds on CI to ensure there are no unexpected warnings or conflicts.
patches/react-native-mmkv+3.3.3.patch (1)
785-811: MMKV now always uses the real native instance; ensure tests/mocks are alignedThis patch removes the
isTest() ? createMockMMKV() : ...branch and disablescreateMMKV.mock.ts, forcingMMKVto always callcreateMMKV(configuration). That’s consistent with the goal of exercising real MMKV in E2E, but it means:
- Unit tests and any non‑RN environments must rely entirely on the new Jest manual mock for
react-native-mmkv; otherwise,createMMKVwill hit native APIs and fail in Node.- The
isTestimport inMMKV.tsis now unused and could be dropped in a follow‑up patch to keep the diff minimal.Please confirm the full Jest suite and E2E runs pass with this behavior before merging.
android/app/src/main/java/chat/rocket/reactnative/storage/SecureKeystore.java (1)
37-257: SecureKeystore port looks correct; a couple of crypto/compatibility details to double-checkThis class closely mirrors the original
react-native-mmkv-storagekeystore helper and theuseKeystore()branching (Keystore path for API ≥ M, SharedPreferences/EncryptedSharedPreferences for older devices) is consistent.Two things worth verifying/tidying up:
EncryptedSharedPreferences on older APIs
In the constructor,EncryptedSharedPreferencesis used when!useKeystore()(API < M) and any failure falls back to standardSharedPreferences. Givenandroidx.security:security-crypto’s API level expectations, please confirm this behaves as intended on your actual minSdk (or that, in practice, the branch is never hit because minSdk ≥ 23).SecretKeySpec algorithm string
new SecretKeySpec(..., Constants.AES_ALGORITHM)currently passes the full transformation string ("AES/ECB/PKCS5Padding") as the algorithm name. The usual pattern is a bare"AES"algorithm identifier; using the transformation here can be brittle across providers. Consider introducing a separateAES_KEY_ALGORITHM = "AES"(or similar) inConstantsand using that forSecretKeySpec, while keepingAES_ALGORITHMforCipher.getInstance(...).Neither point is strictly blocking, but they’re easier to adjust now than after this becomes widely deployed.
__mocks__/react-native-mmkv.js (1)
3-93: MMKV mock behavior looks correct; consider minor test-friendly tweaksThe mock captures the shared-per-id storage and listener API well and should be sufficient for most tests. Two small, optional improvements you might consider later:
- In
notifyListeners, iterate over a shallow copy ([...this.listeners]) to avoid edge cases where a listener removes itself or others while iterating.- If you ever need suite-level isolation, exposing a small internal reset helper (e.g., clearing
storageInstances) can help keep Jest tests independent without relying onclearAllin app code.app/index.tsx (1)
42-42: Good placement for migration; consider gating to avoid repeated workRunning
await migrateFromOldMMKV()at the start ofinitensures MMKV data is migrated before any local settings or notifications logic runs, which is the right ordering. Given the migration reads and rewrites the full MMKV payload, you may want to short‑circuit it after the first successful run (e.g., using the existingWORKSPACE_MIGRATION_COMPLETEDflag) so upgraded users don’t pay the migration cost on every cold start.Also applies to: 129-155
ios/SSLPinning.mm (1)
19-40: Consider cachingMMKVBridge/SecureStorageinstead of recreating each call
+getMMKVInstanceconstructs a newSecureStorageandMMKVBridgeevery time it is called (from bothrunChallengeandxxx_updateSecureStreamOptions). That’s likely more work than needed during connection setup.You could cache a single shared instance, e.g.:
+ (MMKVBridge *)getMMKVInstance { static MMKVBridge *sharedInstance = nil; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ SecureStorage *secureStorage = [[SecureStorage alloc] init]; NSString *hexKey = [self stringToHex:@"com.MMKV.default"]; NSString *key = [secureStorage getSecureKey:hexKey]; NSString *appGroupPath = nil; NSString *suite = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"AppGroup"]; if (suite) { NSURL *directory = [[NSFileManager defaultManager] containerURLForSecurityApplicationGroupIdentifier:suite]; appGroupPath = directory.path; } NSString *mmkvPath = appGroupPath ? [appGroupPath stringByAppendingPathComponent:@"mmkv"] : nil; NSData *cryptKey = (key.length > 0) ? [key dataUsingEncoding:NSUTF8StringEncoding] : nil; sharedInstance = [[MMKVBridge alloc] initWithID:@"default" cryptKey:cryptKey rootPath:mmkvPath]; }); return sharedInstance; }ios/Shared/RocketChat/MMKVBridge.h (1)
12-27: Optionally mark the custom initializer as designated and disable plaininitThe bridge interface looks clean and the nullability annotations make sense. To prevent accidental use of
[MMKVBridge init]without required parameters, you might make the custom initializer designated and markinitunavailable.For example:
@interface MMKVBridge : NSObject - (instancetype)initWithID:(NSString *)mmapID cryptKey:(nullable NSData *)cryptKey rootPath:(nullable NSString *)rootPath; + +- (instancetype)init NS_UNAVAILABLE; + + (instancetype)new NS_UNAVAILABLE;(Optionally add
NS_DESIGNATED_INITIALIZERtoinitWithID:cryptKey:rootPath:.)android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboPackage.java (1)
19-44: TurboModule wiring looks correct; consider minor robustness tweaks.The package correctly registers
MMKVReaderand exposes it as a TurboModule. Two small nits you may want to adjust:
- To be null‑safe, prefer
NativeMMKVReaderSpec.NAME.equals(name)instead ofname.equals(...)ingetModule, in casenameis ever null.- In
ReactModuleInfo,hasConstantsis set totrue, but this module appears to expose only methods (nogetConstants()override). Unless you plan to add constants, setting this tofalsewould more accurately reflect the module’s surface.app/lib/methods/migrateMMKVStorage.ts (1)
8-9: Align migration result/metrics with actual behavior and clean up logging.A few small inconsistencies here that are worth tightening up:
errorsis accumulated but never returned or inspected by callers; it only drives logging.- The function returns a detailed object only in the “no data” case, but
voidon the successful migration path, which makes the API awkward if anyone ever wants to use the metrics.- The log message
"Step 2: Migrating..."doesn’t match the preceding comment (// Step 3).Consider one of these options:
- Make the function fire‑and‑forget:
- Remove the structured return object and the
errorsarray, leaving only logs and the preference flag, or- Commit to a structured result:
- Track
keysFound,keysMigrated, anderrorsfor all paths and return a consistent result type from the function.For example:
- const allKeys = Object.keys(oldData); + const allKeys = Object.keys(oldData); + const keysFound = allKeys.length; + let keysMigrated = 0; @@ - if (allKeys.length === 0) { + if (keysFound === 0) { @@ - for (const key of allKeys) { + for (const key of allKeys) { @@ - targetStorage.set(key, value); + targetStorage.set(key, value); + keysMigrated += 1; @@ - console.log('=== Migration Complete ==='); + console.log('=== Migration Complete ===', { keysFound, keysMigrated, errors }); + return { + success: errors.length === 0, + keysFound, + keysMigrated, + errors, + data: oldData + };And align the comment/log:
- // Step 3: Migrate data - console.log('Step 2: Migrating data to new MMKV...'); + // Step 2: Migrate data + console.log('Step 2: Migrating data to new MMKV...');Also applies to: 21-32, 41-58, 60-65
android/app/src/main/java/chat/rocket/reactnative/storage/NativeMMKVReaderSpec.java (1)
9-30: TurboModule spec surface is clear and consistent with the JS side.The
NativeMMKVReaderSpecdefinition (module name, constructor, and three Promise‑based@ReactMethods) looks correct for exposingreadAndDecryptMMKV,listMMKVFiles, andgetStoragePathto JS. As a small improvement, you may want to add brief JavaDoc comments describing the expected promise payloads (e.g., shape ofoldDataand path format) to keep the native/TS contract self‑documented.android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java (2)
71-85: Type probing logic can silently coerce unknown types to booleanfalseFor each key you:
- try
decodeString,- then
decodeIntwithInteger.MIN_VALUEsentinel,- else unconditionally
decodeBool(key, false)and store the result.Any value that is not a string or int (e.g. a stored double, bytes, or an unsupported type) will end up as
falsein the result map, which can be misleading during migration.Consider:
- Checking
mmkv.containsKey(key)(if available) before defaulting tofalse, and/or- Skipping keys that can’t be decoded into the supported types instead of coercing them to boolean, or
- Explicitly handling all known types you expect from the old storage.
Optionally, you can also remove
stringCount,intCount, andboolCountsince they’re unused, or surface them as debug metrics if they’re meant for logging.Also applies to: 99-125
73-76: Minor: avoid repeatedMMKV.initializeand confirmtoHexmatches upstreamCalling
MMKV.initialize(reactContext)on everyreadAndDecryptMMKVinvocation is redundant; it can typically be done once at app startup or cached behind a static guard.Also, since
toHexis meant to be “same as react-native-mmkv-storage”, please double‑check against the original implementation to ensure alias derivation is byte‑for‑byte identical across platforms (critical for finding existing keys).Also applies to: 139-146
ios/SecureStorage.m (2)
73-92: Fix SecItemCopyMatching usage and release the returned CF object
searchKeychainCopyMatching::
- Requests
kSecReturnData, soresultis aCFDataRef, not a dictionary.- Casts
resulttoNSDictionary *and then passes it toinitWithData:, which expectsNSData *– works at runtime only because the dynamic type is actually data, but is type‑unsafe.- Does not check
OSStatusand never releasesresult, leaking the returned data.A safer pattern:
- (NSString *)searchKeychainCopyMatching:(NSString *)identifier { NSMutableDictionary *searchDictionary = [self newSearchDictionary:identifier]; searchDictionary[(id)kSecMatchLimit] = (id)kSecMatchLimitOne; searchDictionary[(id)kSecReturnData] = (id)kCFBooleanTrue; CFTypeRef result = NULL; OSStatus status = SecItemCopyMatching((CFDictionaryRef)searchDictionary, &result); if (status != errSecSuccess || result == NULL) { if (result != NULL) { CFRelease(result); } return nil; } NSData *data = (__bridge_transfer NSData *)result; return [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding]; }This:
- Uses the correct data type,
- Checks
status,- Uses
__bridge_transferto avoid a leak.
22-51: Minor: exceptions are swallowed without logging
setSecureKey:...andgetSecureKey:catchNSExceptionand either do nothing or returnNULL, giving you no visibility into underlying keychain issues.Consider at least logging the exception (e.g.,
NSLogor a centralized logger) so failures in secure storage can be diagnosed in the field.Also applies to: 127-139
android/app/src/main/java/chat/rocket/reactnative/storage/Constants.java (1)
8-29: Constants look consistent with original library usageCentralizing these MMKV/keystore constants is helpful and aligns with the upstream
react-native-mmkv-storageimplementation. No functional issues spotted here.If you later revisit the encryption design, consider migrating away from ECB modes (
AES/ECB/PKCS5Padding) toward an AEAD mode, but that’s out of scope for this migration.ios/SecureStorage.h (1)
1-4: Minor: duplicateRCTBridgeModuleimport and header availabilityYou import
<React/RCTBridgeModule.h>twice; the duplicate import is harmless but can be removed for clarity.Static analysis is reporting
'React/RCTBridgeModule.h' file not found; this is usually a header‑search‑path or Pods configuration issue rather than a problem with this header itself. Just confirm that the React headers are available in your iOS build environment.ios/MMKVMigration.mm (2)
87-107: Value-type handling is limited;NSNumberbranch is currently unreachable
readInstanceData:withPassword:atPath:only ever populatesdatawith:
NSStringvalues fromstringForKey:, orNSDatavalues fromdataForKey:.You never insert
NSNumbervalues intodata, so in+migratetheNSNumbercase:} else if ([value isKindOfClass:[NSNumber class]]) { [newMMKV setString:[value stringValue] forKey:key]; migratedCount++; }is effectively unreachable.
Depending on how the old storage was used:
- If you genuinely only stored strings and binary blobs, you can remove the
NSNumbercase to simplify the code.- If numeric/bool values were stored in MMKV, you may want to extend
readInstanceDatato read them via typed APIs (e.g.,boolForKey:,intForKey:) and then preserve those types on write, instead of relying solely on string/data.Also applies to: 149-185
63-82: Behavior note: merging multiple instances into one is last-writer-winsThe migration flattens all discovered MMKV instances into a single
allDatadictionary and then writes everything into a new"default"MMKV:[allData addEntriesFromDictionary:instanceData]; ... for (NSString *key in allData) { id value = allData[key]; ... }If the same key exists in multiple instances, the value from the last processed instance silently overrides earlier ones.
If this is intentional, consider adding a brief comment explaining the decision. If not, you might want to:
- Track the source instance for each key, or
- Define explicit precedence rules (e.g., prefer
mmkvIDStoreoverdefault) and enforce them when merging.Also applies to: 87-107, 125-139
android/app/src/main/java/chat/rocket/reactnative/SecureStorage.java (1)
24-31: Secure storage implementation looks solid; a couple of minor robustness tweaks possibleThe overall design (AndroidKeyStore AES/GCM per-alias key, random IV, IV||ciphertext Base64 in SharedPreferences) is sound and appropriate for the intended use; the static-analysis hints about 3DES/ECB don’t apply here since you’re explicitly using
AES/GCM/NoPadding.Two small, optional hardening ideas:
- In
getSecureKeyInternal, you assume a 12‑byte IV; you could derive the IV length fromcombined.length(e.g., store IV length or usecipher.getBlockSize()at encryption time) to be more future‑proof.- In
getSecureKeyInternal, you currently swallow all exceptions and returnnull; consider logging the alias or a more specific message to help debug corrupted entries (you already log ingetSecureKey/setSecureKey, so even just reusing that style here would help).Nothing blocking; this is good to ship as is.
Also applies to: 65-101, 104-161
patches/react-native-webview+13.15.0.patch (2)
67-83: Global staticcertificateAliaslimits flexibility (OK if you only ever need one)
certificateAliasis static onRNCWebViewClient, so a single alias is shared by all WebViews. That matches your current API (a singlesetCertificateAliasstatic across managers/modules), but it makes per‑WebView or per‑host aliases impossible without refactoring.If in future you need multiple aliases (e.g., multiple servers with different client certs), you’ll likely want to move this alias down to instance level on
RNCWebViewClientand plumb it from JS per-WebView instead of globally.
1-34: Adding.projectundernode_modules/react-native-webview/androidis IDE-specificThe new Eclipse
.projectmetadata undernode_modules/react-native-webview/androidis harmless, but it’s tooling-specific and unusual to carry in a long‑lived patch. If you don’t rely on Eclipse/Buildship, consider dropping it to keep the patch minimal.ios/RocketChatRN.xcodeproj/project.pbxproj (1)
1868-1877: One Bugsnag “Upload source maps” phase is now effectively a no‑opFor the Rocket.ChatRN target you still have two “Upload source maps to Bugsnag” phases:
- One with a full script using
bugsnag-react-native-xcode.sh.- Another (
7AAE9EB3…) whoseshellScriptis now just a blank newline.The empty phase is harmless but does nothing and adds noise to the build. Consider either:
- Removing that build phase entirely, or
- Restoring a real script if it’s meant to perform work.
Also applies to: 1954-1964
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (48)
__mocks__/react-native-mmkv-storage.js(0 hunks)__mocks__/react-native-mmkv.js(1 hunks)android/app/build.gradle(1 hunks)android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt(3 hunks)android/app/src/main/java/chat/rocket/reactnative/SecureStorage.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/SecureStoragePackage.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java(2 hunks)android/app/src/main/java/chat/rocket/reactnative/storage/Constants.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboPackage.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/storage/NativeMMKVReaderSpec.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/storage/SecureKeystore.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/storage/Storage.java(1 hunks)android/app/src/main/jni/MainApplicationModuleProvider.cpp(1 hunks)app/containers/AudioPlayer/PlaybackSpeed.tsx(1 hunks)app/index.tsx(2 hunks)app/lib/helpers/ensureServersInDatabase.ts(1 hunks)app/lib/methods/helpers/getSecureKey.ts(1 hunks)app/lib/methods/helpers/sslPinning.ts(3 hunks)app/lib/methods/migrateMMKVStorage.ts(1 hunks)app/lib/methods/userPreferences.ts(1 hunks)app/lib/native/NativeMMKVReader.android.ts(1 hunks)app/lib/native/NativeMMKVReader.ts(1 hunks)app/sagas/init.js(1 hunks)app/views/AccessibilityAndAppearanceView/index.tsx(1 hunks)app/views/MediaAutoDownloadView/index.tsx(1 hunks)ios/AppDelegate.swift(1 hunks)ios/MMKVMigration.h(1 hunks)ios/MMKVMigration.mm(1 hunks)ios/NotificationService/NotificationService-Bridging-Header.h(1 hunks)ios/Podfile(2 hunks)ios/RocketChatRN-Bridging-Header.h(1 hunks)ios/RocketChatRN.xcodeproj/project.pbxproj(30 hunks)ios/SSLPinning.mm(3 hunks)ios/SSLPinning/SSLPinning.swift(2 hunks)ios/SecureStorage.h(1 hunks)ios/SecureStorage.m(1 hunks)ios/Shared/RocketChat/ClientSSL.swift(1 hunks)ios/Shared/RocketChat/MMKV.swift(1 hunks)ios/Shared/RocketChat/MMKVBridge.h(1 hunks)ios/Shared/RocketChat/MMKVBridge.mm(1 hunks)ios/Shared/RocketChat/Storage.swift(1 hunks)ios/Watch/WatchConnection.swift(1 hunks)metro.config.js(1 hunks)package.json(1 hunks)patches/react-native-mmkv+3.3.3.patch(1 hunks)patches/react-native-mmkv-storage+12.0.0.patch(0 hunks)patches/react-native-webview+13.15.0.patch(3 hunks)
💤 Files with no reviewable changes (2)
- mocks/react-native-mmkv-storage.js
- patches/react-native-mmkv-storage+12.0.0.patch
🧰 Additional context used
🧬 Code graph analysis (23)
app/containers/AudioPlayer/PlaybackSpeed.tsx (1)
app/containers/AudioPlayer/constants.ts (1)
AVAILABLE_SPEEDS(1-1)
app/views/MediaAutoDownloadView/index.tsx (1)
app/lib/constants/mediaAutoDownload.ts (1)
MediaDownloadOption(1-1)
android/app/src/main/java/chat/rocket/reactnative/storage/SecureKeystore.java (2)
android/app/src/main/java/chat/rocket/reactnative/storage/Constants.java (1)
Constants(8-29)android/app/src/main/java/chat/rocket/reactnative/storage/Storage.java (1)
Storage(15-35)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (2)
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboPackage.java (1)
MMKVReaderTurboPackage(15-45)android/app/src/main/java/chat/rocket/reactnative/SecureStoragePackage.java (1)
SecureStoragePackage(12-24)
app/lib/methods/helpers/getSecureKey.ts (2)
android/app/src/main/java/chat/rocket/reactnative/SecureStorage.java (1)
SecureStorage(24-162)app/lib/methods/appGroup.ts (1)
appGroupPath(5-5)
app/index.tsx (1)
app/lib/methods/migrateMMKVStorage.ts (1)
migrateFromOldMMKV(7-65)
app/sagas/init.js (2)
app/lib/methods/userPreferencesMethods.ts (1)
getSortPreferences(6-8)app/actions/sortPreferences.ts (1)
setAllPreferences(16-21)
app/lib/helpers/ensureServersInDatabase.ts (2)
app/lib/constants/keys.ts (1)
TOKEN_KEY(25-25)app/sagas/init.js (1)
serversDB(39-39)
ios/Shared/RocketChat/MMKVBridge.h (1)
app/lib/methods/userPreferences.ts (1)
setString(32-38)
app/lib/methods/migrateMMKVStorage.ts (1)
__mocks__/react-native-mmkv.js (1)
MMKV(11-94)
ios/Watch/WatchConnection.swift (1)
ios/Shared/RocketChat/MMKV.swift (1)
build(4-20)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
android/app/src/main/java/chat/rocket/reactnative/SecureStorage.java (1)
SecureStorage(24-162)
app/lib/methods/helpers/sslPinning.ts (1)
app/definitions/ICertificate.ts (1)
ICertificate(1-4)
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboPackage.java (1)
android/app/src/main/java/chat/rocket/reactnative/storage/NativeMMKVReaderSpec.java (1)
NativeMMKVReaderSpec(9-30)
app/lib/methods/userPreferences.ts (4)
__mocks__/react-native-mmkv.js (1)
MMKV(11-94)metro.config.js (1)
config(10-19)app/lib/methods/appGroup.ts (1)
appGroupPath(5-5)app/lib/methods/helpers/getSecureKey.ts (1)
getAppGroupPath(43-55)
ios/Shared/RocketChat/Storage.swift (1)
ios/Shared/RocketChat/MMKV.swift (1)
build(4-20)
ios/Shared/RocketChat/MMKV.swift (1)
ios/Shared/Extensions/FileManager+Extensions.swift (1)
groupDir(4-12)
ios/AppDelegate.swift (1)
ios/SSLPinning/SSLPinning.swift (1)
migrate(24-43)
__mocks__/react-native-mmkv.js (1)
metro.config.js (1)
config(10-19)
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java (2)
__mocks__/react-native-mmkv.js (1)
MMKV(11-94)android/app/src/main/java/chat/rocket/reactnative/storage/NativeMMKVReaderSpec.java (1)
NativeMMKVReaderSpec(9-30)
ios/MMKVMigration.h (1)
ios/SSLPinning/SSLPinning.swift (1)
migrate(24-43)
android/app/src/main/java/chat/rocket/reactnative/SecureStorage.java (1)
app/lib/methods/helpers/getSecureKey.ts (2)
getSecureKey(22-31)setSecureKey(33-41)
ios/SSLPinning/SSLPinning.swift (1)
ios/Shared/RocketChat/MMKV.swift (1)
build(4-20)
🪛 ast-grep (0.40.0)
android/app/src/main/java/chat/rocket/reactnative/SecureStorage.java
[warning] 75-75: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 83-83: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 104-104: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 109-112: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyGenerator.getInstance(
KeyProperties.KEY_ALGORITHM_AES,
KEYSTORE_PROVIDER
)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 128-128: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 75-75: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 83-83: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 104-104: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 109-112: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyGenerator.getInstance(
KeyProperties.KEY_ALGORITHM_AES,
KEYSTORE_PROVIDER
)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 128-128: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🪛 Clang (14.0.6)
ios/NotificationService/NotificationService-Bridging-Header.h
[error] 5-5: 'SecureStorage.h' file not found
(clang-diagnostic-error)
ios/Shared/RocketChat/MMKVBridge.h
[error] 8-8: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
ios/MMKVMigration.h
[error] 8-8: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
ios/SecureStorage.h
[error] 1-1: 'React/RCTBridgeModule.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (34)
app/lib/methods/helpers/getSecureKey.ts (1)
13-41: Confirm alias hex‑encoding is compatible with existing stored keysYou’re now normalizing aliases via
stringToHexbefore callingSecureStorage.getSecureKey/setSecureKey. This is a good way to keep aliases filesystem/KeyStore‑friendly, but it changes the effective alias string seen by the native layer.Please double‑check:
- There is no existing data that was stored using raw aliases against
SecureStorage(Android or iOS). If there is, you’ll need a migration step that looks up old aliases and re‑stores them under the hex‑encoded form.- All new call sites of
SecureStoragego through this helper so alias handling stays consistent across the app and over time.If everything using secure keys is new in this PR, this is fine; otherwise, a small migration helper may be necessary.
app/lib/methods/helpers/sslPinning.ts (3)
69-69: Review comment is incorrect.The native
SSLPinning.setCertificate()method is synchronous, not asynchronous. The implementation is declared as a regular function (@objc func setCertificate) and exported viaRCT_EXTERN_METHOD(notRCT_EXTERN_ASYNC_METHOD), which indicates it returns immediately without a promise. The method performs file I/O and MMKV storage synchronously, so awaiting it at line 69 would be incorrect.Likely an incorrect or invalid review comment.
22-35: Disregard the original review comment—persistCertificateshould not beasync.The original concern was based on an incorrect assumption. Verification shows that
UserPreferences.setMap()andUserPreferences.setString()both returnvoidand are synchronous methods—they did not become async in the react-native-mmkv migration. SincepersistCertificateonly calls these synchronous methods and has no async operations, it correctly remains non-async.The actual issue is the opposite: lines 68 and 87 are unnecessarily awaiting
persistCertificate. Theseawaitstatements should be removed since the function is synchronous.Likely an incorrect or invalid review comment.
83-91: Verify external consumers handle the new async API signature.The
setCertificatemethod (line 83) is nowasyncand exported as a public API (line 101). No internal call sites were found in the codebase, indicating this is likely consumed by external packages or applications. Ensure all external consumers ofRCSSLPinning.setCertificate()are updated to properly await the returned promise.Additionally, line 88 calls the native
SSLPinning?.setCertificate()without awaiting it, consistent with the same concern at line 69—verify whether this should be awaited as well.app/views/AccessibilityAndAppearanceView/index.tsx (1)
115-115: The guard on line 115 is justified, but there's a codebase-wide consistency issue that needs addressing.The
useUserPreferenceshook explicitly returns[T | undefined, setter], which means the nullish coalescing on line 115 is a valid defensive coding pattern.However, verification reveals an inconsistency: the same hook with identical default value (
'TOAST') is used inToast.tsxline 31 but directly compared without a guard on line 45 (if (alertDisplayType === 'DIALOG')). Additionally, inAccessibilityAndAppearanceView, boolean preferences from the hook (lines 27-28 without defaults, line 32 with default) are passed toSwitchcomponents without guards, despite the hook's type signature allowing undefined.Either:
- Apply the guard pattern consistently across all uses of
useUserPreferencesto match line 115's defensive approach, or- Verify whether the hook actually returns undefined in practice despite the permissive type signature, and update the types accordingly
app/lib/methods/userPreferences.ts (5)
8-15: LGTM: Storage singleton pattern is appropriate.The lazy initialization pattern ensures the MMKV instance is created once and reused across the application.
32-38: LGTM: Setter and utility methods are well-implemented.All methods include appropriate error handling with console logging, and the MMKV API is used correctly throughout.
Also applies to: 48-54, 65-71, 73-79, 90-120
195-206: LGTM: App group path helper is correctly implemented.The synchronous access to
NativeModules.AppGroup.pathmatches the pattern used inapp/lib/methods/appGroup.tsand includes appropriate error handling.
123-175: MMKV type system behavior verified—no issues found.The mock implementation confirms MMKV 3.0.0 guarantees type-safe behavior: each typed getter (
getString,getBoolean,getNumber) returnsundefinedfor values that don't match its type. The hook's sequential type-checking logic correctly leverages this guarantee, ensuring no type conflicts across the fallback chain.
177-193: Code pattern is appropriate — no changes needed.The defensive type assertion with optional chaining on line 182 is justified. While
Mode.MULTI_PROCESSexists in the enum, multi-process behavior depends on platform/native setup (e.g. Android supports multi-process MMKV; on iOS you must configure App Groups / shared container) and there have been historical codegen/runtime issues in some versions. The current pattern safely handles this variability across platforms and library versions.app/containers/AudioPlayer/PlaybackSpeed.tsx (1)
15-18: Casting playbackSpeed for indexOf is a reasonable TS fixGiven
useUserPreferences<number>and numericAVAILABLE_SPEEDS, this cast just satisfies the type checker and does not alter runtime behavior; the cycling logic remains correct.metro.config.js (1)
15-18: E2E mock resolution tied explicitly to RUNNING_E2E_TESTS='true'The stricter check matches the
e2e:startscript and keeps.mock.tsresolution opt‑in for E2E only, which is helpful given the MMKV mock being disabled via patch-package.ios/Podfile (1)
11-11: MMKV Pod entry looks correct; ensure CocoaPods integration stays greenAdding
pod 'react-native-mmkv', path: '../node_modules/react-native-mmkv', modular_headers: trueis consistent with a local RN pod setup alongsideuse_frameworks! :linkage => :static. Please confirmpod install(and CI iOS builds) succeed with this and that any oldreact-native-mmkv-storagepod entries are fully removed.app/lib/native/NativeMMKVReader.ts (1)
1-22: NativeMMKVReader TS fallback interface looks solidThe interface cleanly describes the native module surface and the
declare const MMKVReader: MMKVReaderModule | nullpattern is appropriate as a type-only fallback with platform-specific implementations provided elsewhere. No changes needed here.ios/Shared/RocketChat/Storage.swift (1)
9-15: MMKVBridge swap-in looks straightforwardSwitching to
MMKVBridge.build()here is a clean drop-in replacement as long asMMKVBridgestill exposesprivateKey(for:)with the previous semantics. No additional changes needed in this file.ios/Watch/WatchConnection.swift (1)
6-8: Watch MMKVBridge usage aligns with main app; just ensure shared configUsing
MMKVBridge.build()in the watch extension is consistent with the main app and should keep user/token data in sync via the app group path, providedbuild()uses the same group identifier for both targets. Worth double‑checking that the watch extension links the MMKVBridge module and shares the same app group value.Also applies to: 43-47
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
29-32: Android MMKV initialization and TurboModule wiring look correctRegistering
MMKVReaderTurboPackage()ingetPackages()and callingMMKV.initialize(this)inonCreate()should be enough to make the new storage + reader available before JS runs. If you ever need to align the storage path with iOS (e.g., for debugging/mirroring), consider passing an explicit rootDir toMMKV.initialize, but it’s not required for correctness.Also applies to: 37-43, 56-63
ios/MMKVMigration.h (1)
1-18: Minimal migration interface looks fine; check toolchain if Foundation import errorsThe
MMKVMigrationinterface is straightforward and conventional (+migrateon anNSObjectsubclass with<Foundation/Foundation.h>imported). If you’re actually seeing the “Foundation/Foundation.hfile not found” error outside of static analysis, it’s almost certainly an SDK/header search path issue rather than a problem with this header’s contents.ios/Shared/RocketChat/ClientSSL.swift (1)
3-28: MMKVBridge extension correctly preserves legacy ClientSSL read behaviorUpdating the extension to target
MMKVBridgewhile leavingclientSSL(for:)’s internals unchanged keeps the “read old layout (RC_CERTIFICATE_KEY/host)” logic intact, now routed through the bridge. That matches its usage inSSLPinning.migrate()and cleanly separates legacy reads from the newssl_pinning_*key scheme.ios/SSLPinning/SSLPinning.swift (1)
8-22: SSLPinning’s move to MMKVBridge and new key scheme is coherentUsing
MMKVBridge.build()and rewriting certificates intossl_pinning_certificate/ssl_pinning_passwordviasetData/setStringgives you a clear separation between:
- reading legacy MMKV data through
mmkv.clientSSL(for:)duringmigrate(), and- using
getCertificateagainst the newssl_pinning_*keys afterwards.That migration path looks consistent, and the new read/write keys in
setCertificateandgetCertificateline up correctly.Also applies to: 24-55
android/app/src/main/jni/MainApplicationModuleProvider.cpp (1)
26-28: LGTM!The MMKVReader TurboModule registration follows the established pattern and correctly integrates with the Java TurboModule infrastructure.
app/sagas/init.js (1)
27-31: LGTM!The migration check correctly guards against data inconsistencies by ensuring
ensureServersInDatabaseruns only after the workspace migration is completed. The saga pattern usage is correct.android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
62-71: LGTM!The SecureStorage integration is correct. The code properly handles the case where
getSecureKeyInternalreturns null, and MMKV initialization accepts null passwords gracefully.ios/RocketChatRN-Bridging-Header.h (1)
5-7: LGTM!The bridging header imports correctly reference the new local implementations needed for MMKV migration, secure storage, and the MMKVBridge infrastructure.
ios/AppDelegate.swift (2)
20-22: LGTM!The migration is correctly placed at the very beginning of application launch, before any other initialization. The comment clearly explains the critical ordering requirement.
32-33: LGTM!The deferred Firebase and Bugsnag initialization ensures MMKV migration completes before any analytics or crash reporting starts. This is the correct ordering to maintain data integrity, even though it means migration crashes won't be reported.
ios/NotificationService/NotificationService-Bridging-Header.h (1)
5-6: Files exist, but build configuration requires manual verification in Xcode.The header files referenced in the bridging header do exist:
SecureStorage.hatios/SecureStorage.hMMKVBridge.hatios/Shared/RocketChat/MMKVBridge.hThe relative path for MMKVBridge is correct. However,
SecureStorage.his imported as a bare name, which means it depends on correct header search paths in the Xcode build configuration. Verify in Xcode that:
- NotificationService target has
ios/directory in its header search paths- Both headers are in the target membership
The static analysis error may be a false positive if build paths are properly configured.
app/lib/helpers/ensureServersInDatabase.ts (1)
7-43: <!-- [scratchpad_start]
[task verify_server_record_fields_complete]
[observations]
- ensureServersInDatabase called from app/sagas/init.js after workspace migration
- Creates stub records with only id and name
- selectServer.ts creates/updates records with version field
- version field is used extensively: compareServerVersion calls, state.server.version in Redux
- compareServerVersion used for feature detection throughout app
- autoLock accessed in ScreenLockConfigView
- lastLocalAuthenticatedSession accessed in localAuthentication.ts
- All schema fields are optional (isOptional: true)
[analysis]- ensureServersInDatabase intentionally creates minimal stubs during migration/recovery
- selectServer populates version when connecting to server
- Risk: if stub records are accessed before selectServer populates version, compareServerVersion could fail
- review comment is valid - unclear if null version will cause runtime issues
- Function appears to be for recovery/migration, but version field dependency unclear
[pending]- Cannot definitively determine if null version is safe without running tests
- compareServerVersion implementation not fully examined
- No clear guarantee stub records won't be accessed with null version
[conclusion]- Review comment raises valid concern about field initialization
- Verification shows version field is critical and used extensively
- Cannot confirm if current implementation is safe or if it's an oversight
- Developer should verify behavior with null/undefined version in compareServerVersion
Based on my investigation, I have verified the concern raised in the review comment.
Key findings:
Function Purpose:
ensureServersInDatabaseis a migration/recovery function called fromapp/sagas/init.jsafter workspace migration completes. It creates stub records for server URLs found in user preferences.Current Implementation: The function only initializes
id(viasanitizedRaw) andnamefields, leaving all other server record fields as null/undefined.Server Schema Fields: The Server model has 17 optional fields including
version,autoLock,autoLockTime,lastLocalAuthenticatedSession,uniqueID,enterpriseModules, etc.Version Field Dependency: The
versionfield is critical and extensively used throughout the app:
- Used in
compareServerVersioncalls for feature detection- Accessed from Redux state in many components
- Set by
selectServer.tswhen properly connecting to a serverPattern Comparison: In
app/sagas/selectServer.ts, server record creation includesversion(and optionallysupportedVersions/supportedVersionsUpdatedAt), which differs fromensureServersInDatabase.Risk Assessment: If stub records created by
ensureServersInDatabaseare accessed before being properly updated byselectServer, the missingversionfield could cause runtime issues incompareServerVersioncalls.
Consider initializing the
versionfield with a default value (e.g., empty string or a safe default version) when creating server records.This ensures server records are consistently initialized and prevents potential null/undefined errors when version-dependent code accesses these stub records. Verify whether other fields like
autoLock,uniqueID, orenterpriseModulesalso require defaults for safety.android/app/src/main/java/chat/rocket/reactnative/SecureStoragePackage.java (1)
12-24: SecureStorage ReactPackage wiring looks correct
SecureStoragePackageexposes a single native module and no view managers, matching the intended usage pattern. Nothing blocking here.app/lib/native/NativeMMKVReader.android.ts (1)
1-20: TurboModule spec and export forMMKVReaderlook goodThe
Specinterface matches the described native API, andTurboModuleRegistry.getEnforcing('MMKVReader')is an appropriate way to expose the module on Android.ios/Shared/RocketChat/MMKV.swift (1)
3-20: MMKVBridge builder matches cross‑platform storage layout.The
build()helper cleanly derives the app group path, creates<groupDir>/mmkv, and instantiatesMMKVBridge(id: "default", cryptKey: …, rootPath: …), which lines up with the TS side usingnew MMKV({ id: 'default' })and the sharedmmkvdirectory name. This should keep iOS storage aligned with the new MMKV layout without changing the existing token/private key accessors.ios/SecureStorage.m (1)
5-5: I'll verify the Objective-C @implementation syntax claim:
@implementationsyntax is invalid and will not compileThe implementation doesn't need to repeat the superclass name. While you can use a trailing colon followed by the parent class name in the @implementation section as was done in the @interface section, this is optional and typically not done.
Remove the superclass declaration from the
@implementationline:-@implementation SecureStorage : NSObject +@implementation SecureStoragepatches/react-native-webview+13.15.0.patch (1)
128-152: Alias plumbing from JS → module → client looks consistentThe new static
setCertificateAliashelpers onRNCWebViewModuleImpl,RNCWebViewModule(new/old arch), and the managers correctly forward the alias down toRNCWebViewClient.setCertificateAlias. This wiring should make the JS‑side API straightforward.No issues from a wiring perspective here.
Also applies to: 172-206
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
2542-2551: MMKV header paths andFORCE_POSIXdefines look reasonable; just confirm they match Pod layoutYou’ve:
- Added
FORCE_POSIX=1toGCC_PREPROCESSOR_DEFINITIONSfor the main app and NotificationService targets.- Updated
HEADER_SEARCH_PATHSfor RocketChatRN + Rocket.Chat to include$(SRCROOT)/../node_modules/react-native-mmkv/MMKV/Core/**.- Added simple
HEADER_SEARCH_PATHS = ("$(inherited)", "$(SRCROOT)", );for NotificationService.All of this is consistent with an MMKV migration, assuming your Pod/headers for
react-native-mmkvare indeed undernode_modules/react-native-mmkv/MMKV/Core/**and thatFORCE_POSIXis required by your MMKV integration.I’d just double‑check:
- That the MMKV Pod actually exposes headers at this path for all configurations.
- That
FORCE_POSIXis intentional and documented somewhere (so future maintainers know why it’s there).Otherwise these changes look fine.
Also applies to: 2608-2617, 3127-3136, 3191-3200, 3023-3031, 3076-3084
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/helpers/ensureServersInDatabase.ts (1)
7-44: Consider adding comprehensive error handling and logging.The function performs multiple database operations without any error handling. While the database write transaction provides some safety, adding explicit error handling would improve:
- Debuggability: Log which step failed during migration
- User experience: Provide meaningful error context to calling code
- Monitoring: Track migration success rates
Consider wrapping critical sections in try-catch blocks with appropriate logging, especially since this is a migration helper that runs during app initialization.
Example structure:
const ensureServersInDatabase = async (): Promise<void> => { try { const prefix = `${TOKEN_KEY}-`; const keys = userPreferences.getAllKeys(); // ... rest of logic } catch (error) { console.error('Failed to ensure servers in database:', error); // Optionally: track analytics, show user message, etc. throw error; // Re-throw to signal failure to caller } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/lib/helpers/ensureServersInDatabase.ts(1 hunks)app/sagas/init.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/sagas/init.js
🔇 Additional comments (1)
app/lib/helpers/ensureServersInDatabase.ts (1)
36-39: The review comment is incorrect.The pattern of setting
_rawwithsanitizedRaw({ id: ... }, schema)and then directly assigning model fields likerecord.nameis the correct and recommended WatermelonDB approach. The code at lines 36–39 follows this standard pattern correctly and requires no changes.Likely an incorrect or invalid review comment.
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
android/app/build.gradle(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java(2 hunks)android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java (1)
android/app/src/main/java/chat/rocket/reactnative/storage/NativeMMKVReaderSpec.java (1)
NativeMMKVReaderSpec(9-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (4)
android/app/build.gradle (1)
156-156: Excellent: already using stable androidx.security:security-crypto:1.1.0.The dependency has been correctly updated to the stable release version instead of the alpha version flagged in the previous review. This is a security-sensitive library, so using the stable version is the right choice.
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java (3)
3-3: Import added correctly.The
java.io.Fileimport that was flagged in the previous review is now present, resolving the compilation issue.
28-43: LGTM!The
getStoragePathmethod correctly retrieves directory paths and existence status with appropriate error handling.
45-71: LGTM!The
listMMKVFilesmethod includes proper defensive checks (directory existence, null array) and comprehensive error handling.
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
OtavioStasiak
deployed
to
experimental_ios_build
GitHub Actions
Active
OtavioStasiak
deployed
to
experimental_android_build
GitHub Actions
Active
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
package.json (1)
108-108: ✓ Version pinned correctly; past review concern resolved.The exact version pin
"3.3.3"(no caret) successfully addresses the prior review concern about semver-compatible upgrades bypassing the patch file. React Native 0.79.4 exceeds the minimum requirement of 0.74+ for react-native-mmkv V3, and the migration is well-scoped.
🧹 Nitpick comments (3)
ios/MMKVMigration.mm (2)
28-152: Validate migration root path and avoid completely silent failuresThe overall control flow in
+migratelooks solid (idempotent flag, app group lookup, app-group → Documents fallback, and the newgroupURLnil guard). Two points to double‑check:
- The
mmkvPathyou derive here is also used asrootPathwhen creating the newMMKVBridgefor"default". If other code in the app uses a different root path for the runtime MMKV instance (e.g., a separate helper or default path), you could be writing migrated data to a location the app never reads. It’s worth confirming that the samerootPathresolution is used everywhere.- The top‑level
@try/@catchswallows allNSExceptions without any logging or diagnostics. If something goes wrong (filesystem permission, MMKV init issue, etc.), the app will just silently skip migration and keep retrying on every launch. Even a minimal log/metric in debug or a single error log guarded for release would make this much easier to diagnose in the field.
154-193: Confirm that value-type coverage matches what the old storage actually used
readInstanceData:withPassword:atPath:only attemptsstringForKey:and thendataForKey:; later, in+migrate, anyNSNumbervalues you encounter are written back as strings. This is fine ifreact-native-mmkv-storagealways stored app-visible values as strings or binary blobs, but if it ever persisted booleans/ints/floats as native numeric types:
- Those keys might not be captured here (depending on how
MMKVBridgeexposes typed getters), or- They may come back as raw
NSDataand be written into the new store in a way that no longer matches how the JS side reads them (e.g., previously via numeric APIs).Please confirm that this string/data‑only read and string (for
NSNumber)/data writeback preserves behavior for all keys that matter. If the previous layer used typed numeric/bool APIs, you may want to extend the migration to explicitly handle those types so you don’t silently change how preferences or counters are represented.android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java (1)
89-89: MMKV.initialize is idempotent and safe for repeated calls — no issue, but consider a minor optimization.MMKV.initialize() is generally safe to call multiple times with the same parameters, as it sets a static rootDir and invokes the native initializer. Since
readAndDecryptMMKVis a one-time legacy migration utility and no call sites exist elsewhere in the codebase, repeated initialization is not a practical concern.However, for cleaner code, consider moving
MMKV.initialize(reactContext)outside this method to avoid redundant initialization—initialize it once during app startup (e.g., in Application.onCreate or a dedicated initialization method) rather than per-invocation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java(1 hunks)app/lib/helpers/ensureServersInDatabase.ts(1 hunks)ios/MMKVMigration.mm(1 hunks)ios/RocketChatRN.xcodeproj/project.pbxproj(24 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java (2)
android/app/src/main/java/chat/rocket/reactnative/storage/NativeMMKVReaderSpec.java (1)
NativeMMKVReaderSpec(9-30)app/lib/methods/helpers/getSecureKey.ts (1)
getSecureKey(22-31)
app/lib/helpers/ensureServersInDatabase.ts (2)
app/lib/constants/keys.ts (1)
TOKEN_KEY(25-25)app/sagas/init.js (1)
serversDB(39-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (10)
ios/MMKVMigration.mm (1)
13-24:toHexhelper looks correct and nil-safeThe UTF‑8 → hex conversion loop and the early return for
nilare both sound for the aliasing use case; no changes needed here.app/lib/helpers/ensureServersInDatabase.ts (1)
33-48: Error handling and transaction flow look good.The past critical issue regarding migration flag placement and error handling has been properly addressed. The flag is now set inside the try block after the database write succeeds, and errors are logged and rethrown for proper propagation to the caller.
ios/RocketChatRN.xcodeproj/project.pbxproj (2)
1963-1963: Bugsnag source map upload script has been emptied.The "Upload source maps to Bugsnag" build phase script was replaced with an empty script. This will prevent source maps from being uploaded, degrading crash report symbolication in production.
If this is intentional during development, consider restoring it before release. If the migration requires a different approach for Bugsnag integration, ensure source map uploads are configured accordingly.
2541-2550: Build settings correctly configured for react-native-mmkv.The
FORCE_POSIX=1preprocessor definition and updatedHEADER_SEARCH_PATHSpointing toreact-native-mmkv/MMKV/Core/**are correctly configured for the MMKV migration. These changes are consistently applied across Debug/Release configurations for RocketChatRN, Rocket.Chat, and NotificationService targets.android/app/src/main/java/chat/rocket/reactnative/storage/MMKVReaderTurboModule.java (6)
1-21: LGTM! Well-structured module initialization.The imports, class structure, and constructor follow TurboModule best practices correctly. The missing
Fileimport issue from the previous review has been properly addressed.
28-43: LGTM! Clean diagnostic helper.The method provides useful path information for debugging migration issues, with appropriate error handling.
45-71: LGTM! Robust file listing implementation.The defensive checks for directory existence and null files array prevent potential NPEs. The detailed file information will be helpful for migration diagnostics.
116-138: Type detection improvements look good; confirm edge case handling is sufficient.The updates address the previous review concerns well:
✅ Line 131 adds
containsKeycheck before boolean decode (prevents false-positive boolean detection)
✅ Line 136 logs decode errors instead of silently swallowing themHowever, the fundamental limitations remain:
- Line 125-126: If a key actually stores
Integer.MIN_VALUE, it will be misdetected and skipped- Line 132: A boolean key storing
falsewill returnfalse(correct), but if the key is missing entirely, it would also returnfalse—though thecontainsKeyguard on line 131 mitigates thisThe documentation (lines 76-80) acknowledges these are acceptable trade-offs for one-time legacy migration. Please confirm:
- The migrated data is not expected to contain
Integer.MIN_VALUEas a stored integer- If type misdetection occurs for edge cases, the migration can be re-run or manually corrected
- This is truly read-only and one-time (not used in production data paths)
149-160: LGTM! Hex conversion correctly fixed.The formatting now correctly handles signed bytes with
(b & 0xff)masking and%02xpadding, addressing the previous review issue. The implementation matches the TypeScript equivalent ingetSecureKey.ts.
92-98: No explicit cleanup is needed—code follows correct MMKV and SecureKeystore lifecycle patterns.SecureKeystore is a utility class that wraps Android Keystore System and manages encryption keys via SharedPreferences. Neither Android Keystore nor SharedPreferences require explicit disposal.
MMKV best practice is to initialize once in Application.onCreate and keep instances open for the app lifetime. Don't call close() unless you are certain that no code will access that MMKV ID again, as close() marks the instance unusable. The code correctly avoids calling
close()on the MMKV instance, which aligns with library guidance for typical app usage.
OtavioStasiak
requested a deployment
to
upload_experimental_android
GitHub Actions
Waiting
|
Android Build Available Rocket.Chat Experimental 4.67.0.107723 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQXUmbZHIh67oKi1CJ7DgTd7k268gpUYE7g_2GVgNKwpdtbIMutJS0thjZzYDoOMhgmqOuL2TJqlwu94ORI |
Proposed changes
This PR migrates our storage implementation from react-native-mmkv-storage to react-native-mmkv.
The new library is more actively maintained and provides improved stability and performance.
This migration may also resolve issues previously reported on iOS, such as unexpected user logouts.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1064
How to test or reproduce
New Installation
Default user
Server with SSL Certificate
E2EE
Notifications (This case must be repeated in Default user, SSL Server and E2EE)
Upgrading the app
For the upgrade process, all tests performed in New Installation must be repeated, but under different scenarios:
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.