Merge v1 to main #89
Merge v1 to main #89
Conversation
…ift-Enterprise to version 3.3.0
*SDK version migrated from 3.2.1 to 3.3 in both Android and iOS. *All the submodules are now pointing to the main branch. *Create an error for all unimplemented functions since we are not releasing p2p in v1.0.0
- Add collection change listener tests - Add document change listener tests - Add custom bug fix tests - Update CblReactnativeModule.kt for Android - Update CblReactNativeEngine.tsx - Update iOS Swift bridge code - Update test navigation sections - Update cblite-js tests
- Add collection-change-listener test component - Add custom-bug-fix test component - Add document-change-listener test component - Update CblReactNativeEngine with latest changes - Update Android module with latest changes - Resolve merge conflicts in Podfile.lock
- Implement collection_GetFullName method in CblReactnativeModule for both Android and iOS. - Update CblReactNativeEngine to include collection_GetFullName functionality. - Modify various components to utilize the new fullName method for collections. - Expand tsconfig.build.json to exclude additional directories.
- Add listenerToken_Remove method to CblReactNativeEngine - Update Android native module with listener token removal - Update iOS native module with listener token removal - Update listener test files to use new token-based removal - Add listener token plan documentation
- Update Android CblReactnativeModule.kt - Update iOS CblReactnative.swift - Remove listener-token-plan.md (no longer needed)
New remove listener api
chore: update submodule reference for cblite-js-tests
# Conflicts: # expo-example/app/tests/replicator-listeners-new.tsx # expo-example/cblite-js-tests # expo-example/hooks/useTestsNavigationSections.tsx # src/cblite-js
…d ios cbl-js-swift
chore: update submodules to support_col_db_property branch
…d ios cbl-js-swift
…for cbl-js-kotlin
New replicator config
fix: correct Kotlin import paths and enhance logging methods in Swift
- Update version to 1.0.0-beta.1 - Update submodule pointers to RN-v1 branches - Fix TypeScript compatibility in blob.ts
Summary of ChangesHello @jayant-dhingra-cb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request marks a major version update to Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring for listener management, new logging capabilities via a LogSinks API, and updates dependencies. The listener management has been centralized, which is a great improvement for maintainability. However, there are a few critical issues that need to be addressed, including a typo in a ProGuard file that could break release builds, and unsafe type casting in the new logging functionality that could lead to runtime crashes. Additionally, the use of GlobalScope for coroutines should be replaced with a lifecycle-aware scope to prevent potential memory leaks.
|
|
||
| # Keep our React Native module | ||
| -keep class com.cblreactnative.** { *; } | ||
| -keep class cbl.js.kotiln.** { *; } |
There was a problem hiding this comment.
There is a typo in the package name. It should be cbl.js.kotlin instead of cbl.js.kotiln. This will cause ProGuard to not keep the necessary classes, leading to crashes in release builds due to obfuscation.
-keep class cbl.js.kotlin.** { *; }
| val intLevel = level?.toInt() | ||
|
|
||
| // Convert ReadableMap? to Map<String, Any>? | ||
| val configMap = config?.toHashMap()?.mapValues { it.value as Any } |
There was a problem hiding this comment.
The unsafe cast it.value as Any can lead to a TypeCastException at runtime if a value in the ReadableMap is null. The toHashMap() method on ReadableMap can produce a map with nullable values (HashMap<String, Any?>). Casting a null value to a non-nullable type Any will cause a crash.
To prevent this, you should safely handle potential null values before passing the map to LogSinksManager.setFileSink.
| val configMap = config?.toHashMap()?.mapValues { it.value as Any } | |
| val configMap = config?.toHashMap()?.filterValues { it != null } as? Map<String, Any> |
| scopeName: String, | ||
| promise: Promise | ||
| ) { | ||
| GlobalScope.launch(Dispatchers.IO) { |
There was a problem hiding this comment.
Using GlobalScope to launch coroutines is discouraged as it can lead to memory leaks. Coroutines launched in GlobalScope are not tied to any component's lifecycle and will not be automatically cancelled. This can cause them to continue running even after the component that launched them is destroyed.
A better approach is to create a CoroutineScope that is tied to the lifecycle of this CblReactnativeModule. You can create a scope with a SupervisorJob and cancel it in the onCatalystInstanceDestroy method of the module.
This comment also applies to other uses of GlobalScope in this file, such as in listenerToken_Remove, logsinks_SetConsole, logsinks_SetFile, and logsinks_SetCustom.
| } | ||
| } else { | ||
| val errorMsg = "No listener found for token $changeListenerToken" | ||
| android.util.Log.e("CblReactnative", "::KOTLIN DEBUG:: listenerToken_Remove: $errorMsg") |
There was a problem hiding this comment.
Using Log.e for debugging messages that are not critical errors can clutter the error logs and make it harder to identify actual problems. For a 'listener not found' scenario, which is an expected client-side condition, Log.w (warning) or Log.d (debug) would be more appropriate. Additionally, it's good practice to ensure such debug logs are stripped from production builds.
| android.util.Log.e("CblReactnative", "::KOTLIN DEBUG:: listenerToken_Remove: $errorMsg") | |
| android.util.Log.w("CblReactnative", "::KOTLIN DEBUG:: listenerToken_Remove: $errorMsg") |
| // Test 3: Test error handling with a fake collection | ||
| results.push(''); | ||
| results.push('=== TEST 3: Error Handling ==='); | ||
| results.push('Testing with valid collection (should succeed)'); | ||
| try { | ||
| const testFullName = await collection.fullName(); | ||
| results.push(`✅ No error thrown for valid collection`); | ||
| results.push(` Got: "${testFullName}"`); | ||
| } catch (error: any) { | ||
| results.push(`❌ Unexpected error: ${error.message}`); | ||
| results.push(` Code: ${error.code || 'N/A'}`); | ||
| } |
There was a problem hiding this comment.
The test case description for 'Test 3' says it will test error handling with a fake collection, but the implementation uses the valid collection object passed into the function. This means the error path is not actually being tested. To properly test error handling, you should attempt to call .fullName() on an invalid or destroyed collection object and assert that it throws the expected error.
- Fix ProGuard typo: cbl.js.kotiln -> cbl.js.kotlin - Fix nullable value handling in LogSinksManager.setFileSink - Change Log.e to Log.w for non-critical listener debug message - Update test case comment for clarity
Fixed an issue where passing an empty encryption key would cause a callback error. Empty encryption keys are now properly handled on both iOS and Android platforms to remove database encryption as intended. - iOS: Removed validation that rejected empty strings, convert empty strings to nil before passing to DatabaseManager - Android: Convert empty strings to null before passing to DatabaseManager This prevents the "callback was already invoked" error and allows users to remove encryption by providing an empty key value.
Temporarily disabled these test navigation items in the expo-example app.
Remove unused commented code for connecting to Sync Gateway from collection change listener test.
Changes for all the code for React Native version 1