Skip to content

Conversation

Rohit3523
Copy link
Collaborator

@Rohit3523 Rohit3523 commented Oct 9, 2025

Proposed changes

Issue(s)

https://rocketchat.atlassian.net/browse/COMM-43

How to test or reproduce

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Summary by CodeRabbit

  • Chores
    • Upgraded the secure storage library to the latest major version to maintain compatibility and overall app stability.
    • Improves consistency of shared storage between the main app and any extensions, reducing potential discrepancies.
    • No changes to features or user interface; behavior should remain the same.
    • This update is preventive and keeps the app aligned with upstream improvements without altering workflows.

@Rohit3523 Rohit3523 temporarily deployed to approve_e2e_testing October 9, 2025 21:39 — with GitHub Actions Inactive
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

Walkthrough

Dependency for react-native-mmkv-storage is bumped to ^12.0.0. A patch adjusts internal native logic: initializes a local serviceName in getSecureKey, assigns serviceName from AppGroup in newSearchDictionary, and comments out a prior dispatch_sync(main) block. No public API or method signatures changed.

Changes

Cohort / File(s) Summary
Dependency bump
package.json
Update react-native-mmkv-storage from ^0.11.2 to ^12.0.0.
Native storage patch
patches/react-native-mmkv-storage+12.0.0.patch
Initialize local serviceName in getSecureKey; propagate serviceName from AppGroup in newSearchDictionary; comment out prior dispatch_sync on main queue; add comments/formatting. No exported API changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App
  participant MMKV as MMKVStorage (Native)
  participant Keychain
  participant AppGroup as AppGroup Service

  App->>MMKV: getSecureKey()
  Note over MMKV: Initialize local serviceName
  MMKV->>AppGroup: newSearchDictionary(AppGroup)
  AppGroup-->>MMKV: serviceName (shared for app/extensions)
  MMKV->>Keychain: Query using serviceName
  Keychain-->>MMKV: Secure key value
  MMKV-->>App: Return key
  Note over MMKV: Main-thread dispatch removed (commented-out)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A whisker twitch, a version hop—
I stash my keys where carrots drop.
One serviceName for all my burrows,
No main-thread dash, just gentle furrows.
Patch applied, I thump with glee—
Secure and swift, from warren to tree. 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The title explicitly states the primary change—updating the react-native-mmkv-storage dependency to version 12.0.0—which directly reflects the modifications made in the package.json and the patch file. It is concise, clear, and conveys the main intent of the pull request without extraneous detail.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mmkv-storage-update

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@diegolmello diegolmello temporarily deployed to approve_e2e_testing October 14, 2025 11:54 — with GitHub Actions Inactive
@diegolmello diegolmello temporarily deployed to experimental_ios_build October 14, 2025 11:58 — with GitHub Actions Inactive
@diegolmello diegolmello temporarily deployed to experimental_android_build October 14, 2025 11:58 — with GitHub Actions Inactive
@diegolmello diegolmello marked this pull request as ready for review October 14, 2025 16:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
patches/react-native-mmkv-storage+12.0.0.patch (1)

32-35: Verify serviceName initialization works correctly with app extensions.

The addition of serviceName initialization from AppGroup before the nil check looks reasonable for sharing the keychain service between the main app and extensions. However, please ensure:

  1. The AppGroup key is properly configured in Info.plist for both the main app and all extensions
  2. Test keychain operations work correctly in:
    • Main app context
    • Share extension context
    • Notification service extension context
  3. Verify fallback to bundleIdentifier works when AppGroup is not configured

Consider adding a log statement or assertion to help debug if AppGroup is missing when extensions are expected:

 - (NSMutableDictionary *)newSearchDictionary:(NSString *)identifier {
     NSMutableDictionary *searchDictionary = [[NSMutableDictionary alloc] init];
 
     // this value is shared by main app and extensions, so, is the best to be used here
     serviceName = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"AppGroup"];
     
     if(serviceName == nil){
+        // Log when falling back to bundle identifier (may indicate missing AppGroup configuration)
         serviceName = [[NSBundle mainBundle] bundleIdentifier];
     }
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d3b9ea6 and 808e558.

⛔ Files ignored due to path filters (2)
  • ios/Podfile.lock is excluded by !**/*.lock
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (2)
  • package.json (1 hunks)
  • patches/react-native-mmkv-storage+12.0.0.patch (2 hunks)
🔇 Additional comments (1)
package.json (1)

116-116: Verify migration path and test react-native-mmkv-storage v12.0.0 upgrade

The v12.0.0 release notes lack a breaking-changes summary. Review the full changelog (ammarahm-ed/react-native-mmkv-storage@v0.11.0...v12.0.0) for API changes, then:

  • Validate secure-storage behavior when the device is locked
  • Confirm compatibility with app extensions and AppGroup sharing
  • Document all required migration steps for consumers

Comment on lines +17 to +24
+ // dispatch_sync(dispatch_get_main_queue(), ^{
+ // int readAttempts = 0;
+ // // See: https://github.com/ammarahm-ed/react-native-mmkv-storage/issues/195
+ // while (![[UIApplication sharedApplication] isProtectedDataAvailable] && readAttempts++ < 100) {
+ // // sleep 25ms before another attempt
+ // usleep(25000);
+ // }
+ // });
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Critical: Verify that commenting out the protected data availability check doesn't break device lock scenarios.

This change comments out a synchronous check on the main queue that was verifying protected data availability with retry logic (referenced issue #195). This code was specifically handling scenarios when the device is locked or protected data is unavailable.

Removing this without proper verification could cause:

  • Keychain access failures when the device is locked
  • Data loss or corruption if operations proceed without protected data being available
  • Silent failures in background or extension contexts

Please verify:

  1. Why was this dispatch_sync causing issues in version 12.0.0? (deadlock, threading issues?)
  2. Does the new version handle protected data availability internally?
  3. Has this been tested with:
    • Device locked scenarios
    • Background app refresh
    • App extensions
    • Cold start when device is locked

If this change was needed to fix a deadlock or crash, document the reasoning in the PR description and ensure alternative protection mechanisms are in place.


🏁 Script executed:

#!/bin/bash
# Inspect iOS code in react-native-mmkv-storage for protected data handling changes
rg -n --type=objc 'isProtectedDataAvailable' -C3 node_modules/react-native-mmkv-storage/ios
rg -n --type=objc 'dispatch_get_main_queue' -C3 node_modules/react-native-mmkv-storage/ios

Length of output: 165


🏁 Script executed:

#!/bin/bash
# Display the patch to confirm file path and context
cat patches/react-native-mmkv-storage+12.0.0.patch

Length of output: 1873


Critical: Reinstate or justify removal of isProtectedDataAvailable retry logic. Deleting the dispatch_sync loop awaiting UIApplication.sharedApplication.isProtectedDataAvailable removes keychain-read protection when the device is locked, risking failures in locked, background, and extension contexts; restore the retry or document an alternative and validate behavior during device lock, background fetch, app extensions, and cold start.

🤖 Prompt for AI Agents
In patches/react-native-mmkv-storage+12.0.0.patch around lines 17–24, the
dispatch_sync loop that retried until
UIApplication.sharedApplication.isProtectedDataAvailable was removed; reinstate
that retry logic (dispatch_sync on main queue, loop with a readAttempts counter,
poll isProtectedDataAvailable and sleep ~25ms per attempt up to ~100 attempts)
so keychain/read operations wait for protected data when device is locked, or if
you intentionally remove it, add a clear comment and documented alternative
(e.g., defer reads until
application:didFinishLaunchingWithOptions:/protectedDataAvailable notification
or use SecItemCopyMatching with kSecAttrAccessibleWhenUnlockedThisDeviceOnly
strategy), add unit/integration tests validating behavior during
locked/background/extension/cold-start scenarios, and ensure logging/metrics
capture any failures after max attempts.

@diegolmello diegolmello had a problem deploying to upload_experimental_android October 14, 2025 16:48 — with GitHub Actions Error
Copy link

Android Build Available

Rocket.Chat Experimental 4.66.0.107565

Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNR5kZhUzv11hvurHi5iGF7ZLZoDNVjd-Z6QBG2KFu11Bqzm_enVX2objoO0G1CVUQs-wJZ-UoG_S8Nuusg5

Copy link

iOS Build Available

Rocket.Chat Experimental 4.66.0.107567

@Rohit3523 Rohit3523 changed the title chore: update react-native-mmkv-storage chore: update react-native-mmkv-storage to v12.0.0 Oct 15, 2025
@Rohit3523 Rohit3523 requested a deployment to approve_e2e_testing October 15, 2025 14:19 — with GitHub Actions Waiting
@Rohit3523 Rohit3523 requested a deployment to experimental_ios_build October 15, 2025 14:22 — with GitHub Actions Waiting
@Rohit3523 Rohit3523 requested a deployment to experimental_android_build October 15, 2025 14:22 — with GitHub Actions Waiting
@Rohit3523 Rohit3523 requested a deployment to official_android_build October 15, 2025 14:22 — with GitHub Actions Waiting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants