Conversation
jayant-dhingra-cb
commented
Apr 7, 2026
jayant-dhingra-cb
commented
- Migrate iOS native bridge to Turbo Modules (RCTCblModules + per-area Swift modules: DB, collection, doc, query, replicator, etc.).
- Keep legacy implementation under legacy_* for reference / gradual cutover.
- JS: Turbo specs + CblReactNativeEngine updates aligned with new native surface.
Adds NativeCbl* TurboModule spec files for Collection, Database, Document, Engine, Logging, Query, Replicator, and Scope. Updates package.json and podspec for TurboModule compatibility, and includes the migration plan. Made-with: Cursor
- Replace monolithic CblReactnative with RCTCblModules and Swift modules - Add per-domain modules (database, collection, document, query, etc.) - Preserve legacy implementation under legacy_* files; wrap JS engine - Update bridging header for new module surface Made-with: Cursor
Remove the migration plan from version control and list it in .gitignore so it can stay local. Made-with: Cursor
![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 completes the migration to Turbo Modules by refactoring the monolithic bridge into domain-specific modules and updating the React Native engine to use the new architecture. My feedback includes suggestions to improve readability in error handling and complex ternary operations, using Swift conventions for unused parameters, and clarifying the 'not found' case for document retrieval.
| let (isIdxError, indexData) = DataAdapter.shared.adaptIndexToArrayAny( | ||
| dict: indexDict, reject: reject | ||
| ) | ||
| if isError || isIdxNameError || isIdxError { return } |
There was a problem hiding this comment.
The condition isError || isIdxNameError || isIdxError is correct, but for better readability and maintainability, consider using a guard statement or a more descriptive error handling approach.
| scopeName: args.scopeName, | ||
| databaseName: args.databaseName | ||
| ) else { | ||
| resolve(NSDictionary()) |
There was a problem hiding this comment.
Returning an empty NSDictionary() when a document is not found might be ambiguous for the JS layer. Consider returning nil or a specific error object to explicitly handle the 'not found' case.
| let domainsArray = domains.isEmpty ? nil : domains | ||
| let tokenValue = token.isEmpty ? nil : token | ||
| let callback: ((LogLevel, LogDomain, String) -> Void)? = | ||
| (intLevel != nil && tokenValue != nil) ? |
There was a problem hiding this comment.
The ternary operator here is quite complex. Consider refactoring this into a separate helper method to improve readability.
| // replicatorId is present in the TypeScript spec but intentionally unused — | ||
| // matches legacy behaviour at CblReactnative.swift line 1622 |
There was a problem hiding this comment.
If the replicatorId is intentionally unused, it should be prefixed with an underscore (e.g., _replicatorId) to adhere to Swift conventions and avoid compiler warnings about unused parameters.
References
- Unused parameters should be prefixed with an underscore in Swift.
There was a problem hiding this comment.
Pull request overview
This PR advances the iOS native bridge migration to React Native Turbo Modules by introducing per-domain TurboModule specs on the JS side and new iOS adapter + Swift module implementations, while retaining the legacy bridge code under legacy_* for reference and incremental cutover.
Changes:
- Added JS TurboModule spec files (
NativeCbl*) and updatedCblReactNativeEngineto call the new TurboModule surface. - Added iOS TurboModule adapters (
RCTCblModules.{h,mm}) and domain-specific Swift implementations (Database/Collection/Document/Query/Replicator/Scope/Logging/Engine). - Updated packaging/config (
codegenConfig, podspec) to support codegen + new-arch module setup; archived legacy bridge sources.
Reviewed changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/NativeCblDatabase.ts | TurboModule spec for database operations |
| src/NativeCblCollection.ts | TurboModule spec for collection ops + events |
| src/NativeCblDocument.ts | TurboModule spec for document CRUD + blobs |
| src/NativeCblQuery.ts | TurboModule spec for query ops + events |
| src/NativeCblReplicator.ts | TurboModule spec for replicator ops + events |
| src/NativeCblScope.ts | TurboModule spec for scopes |
| src/NativeCblLogging.ts | TurboModule spec for legacy logging + LogSinks |
| src/NativeCblEngine.ts | TurboModule spec for engine utilities (path + listener removal) |
| src/CblReactNativeEngine.tsx | Rewired engine to use the new TurboModules + global event emitter |
| src/legacy_CblReactNativeEngine.tsx | Legacy JS bridge retained (commented out) |
| ios/RCTCblModules.h | Declares iOS TurboModule adapter classes and which emit events |
| ios/RCTCblModules.mm | Implements iOS TurboModule adapters and forwards to Swift modules |
| ios/*.swift (new modules) | New per-domain Swift implementations + shared queue/token store |
| ios/legacy_CblReactnative.* | Legacy iOS bridge files retained but disabled |
| ios/CblReactnative-Bridging-Header.h | Trimmed bridging header imports |
| cbl-reactnative.podspec | Switched to unconditional install_modules_dependencies |
| package.json | Added codegenConfig and updated library type; adjusted published files |
| .npmignore / .gitignore | Minor ignore list updates |
Comments suppressed due to low confidence (1)
src/CblReactNativeEngine.tsx:1408
replicator_AddDocumentChangeListener/replicator_RemoveChangeListeneruse achangeListenerToken + '_doc'key in_emitterSubscriptions, but nothing ever stores a subscription under that key (the only stored doc subscription uses_eventReplicatorDocumentChange). As a result, the_docguard is ineffective and the_docremoval path never runs, which can leave subscriptions around unexpectedly. Either store/remove the document-change subscription consistently under the same key, or remove the unused_doclogic and (if desired) tear down the_eventReplicatorDocumentChangesubscription when the last document listener is removed.
//need to track the listener callback for later use due to how React Native events work
if (
this._replicatorDocumentChangeListeners.has(args.changeListenerToken) ||
this._emitterSubscriptions.has(args.changeListenerToken + '_doc')
) {
throw new Error(
'ERROR: changeListenerToken already exists and is registered to listen to document callbacks, cannot add a new one'
);
}
// Set up document change listener if not already done
if (!this._isReplicatorDocumentChangeEventSetup) {
const docSubscription = this._eventEmitter.addListener(
this._eventReplicatorDocumentChange,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(results: any) => {
this.debugLog(
`::DEBUG:: Received event ${this._eventReplicatorDocumentChange}`
);
const token = results.token as string;
const data = results?.documents;
const error = results?.error;
if (token === undefined || token === null || token.length === 0) {
this.debugLog(
'::ERROR:: No token to resolve back to proper callback for Replicator Document Change'
);
throw new Error(
'ERROR: No token to resolve back to proper callback'
);
}
const callback = this._replicatorDocumentChangeListeners.get(token);
if (callback !== undefined) {
callback(data, error);
} else {
this.debugLog(
`Error: Could not find callback method for document change token: ${token}.`
);
throw new Error(
`Error: Could not find callback method for document change token: ${token}.`
);
}
}
);
this._emitterSubscriptions.set(
this._eventReplicatorDocumentChange,
docSubscription
);
this._isReplicatorDocumentChangeEventSetup = true;
}
return new Promise((resolve, reject) => {
NativeCblReplicator.replicator_AddDocumentChangeListener(
args.changeListenerToken,
args.replicatorId
).then(
() => {
this._replicatorDocumentChangeListeners.set(
args.changeListenerToken,
lcb
);
this.debugLog(
`::DEBUG:: replicator_AddDocumentChangeListener added successfully with token: ${args.changeListenerToken}`
);
resolve();
},
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(error: any) => {
this._replicatorDocumentChangeListeners.delete(
args.changeListenerToken
);
reject(error);
}
);
});
}
replicator_RemoveChangeListener(
args: ReplicationChangeListenerArgs
): Promise<void> {
if (this._replicatorDocumentChangeListeners.has(args.changeListenerToken)) {
this._replicatorDocumentChangeListeners.delete(args.changeListenerToken);
// Remove any subscription with the doc suffix
if (this._emitterSubscriptions.has(args.changeListenerToken + '_doc')) {
this._emitterSubscriptions
.get(args.changeListenerToken + '_doc')
?.remove();
this._emitterSubscriptions.delete(args.changeListenerToken + '_doc');
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct ChangeListenerRecord { | ||
| let nativeListenerToken: ListenerToken | ||
| let listenerType: ChangeListenerType | ||
| } | ||
|
|
||
| enum ChangeListenerType: String { | ||
| case collection | ||
| case collectionDocument | ||
| case query | ||
| case replicator | ||
| case replicatorDocument | ||
| } | ||
|
|
||
| public class ListenerTokenStore { | ||
|
|
||
| public static let shared: ListenerTokenStore = ListenerTokenStore() | ||
| private init() {} | ||
|
|
||
| private let queue = DispatchQueue( | ||
| label: "com.cblite.ListenerTokenStore", | ||
| attributes: .concurrent | ||
| ) | ||
| private var store: [String: ChangeListenerRecord] = [:] | ||
|
|
||
| public func add(token: String, record: ChangeListenerRecord) { | ||
| queue.async(flags: .barrier) { | ||
| self.store[token] = record | ||
| } | ||
| } | ||
|
|
||
| public func remove(token: String) -> ChangeListenerRecord? { | ||
| var removed: ChangeListenerRecord? |
There was a problem hiding this comment.
ListenerTokenStore declares public methods that accept/return ChangeListenerRecord, but ChangeListenerRecord (and ChangeListenerType) are not public. Swift does not allow a public API to reference an internal type, so this will fail to compile. Make the record/enum public (or at least public within the module) or reduce the visibility of ListenerTokenStore/its methods to internal/fileprivate so access levels are consistent.
| s.dependency "ReactCommon/turbomodule/core" | ||
| end | ||
| end | ||
| install_modules_dependencies(s) |
There was a problem hiding this comment.
install_modules_dependencies is only available in React Native >= 0.71 (as noted in the removed comment). Calling it unconditionally will raise an error during pod install on older RN versions / older CocoaPods scripts. Either restore the respond_to?(:install_modules_dependencies, true) guard with the legacy dependency branch, or explicitly document/enforce the minimum supported RN version so consumers can’t end up with a broken install.
| install_modules_dependencies(s) | |
| if respond_to?(:install_modules_dependencies, true) | |
| install_modules_dependencies(s) | |
| else | |
| raise "cbl-reactnative requires React Native >= 0.71 for CocoaPods integration (`install_modules_dependencies` is unavailable in this environment)." | |
| end |
pasin
left a comment
pasin
left a comment
There was a problem hiding this comment.
I have review CblCollectionModule.swift.
As this is a very big PR, I cannot review them all at once due to other priorities. I will provide my feedback for the other later.
| let (isError, args) = DataAdapter.shared.adaptCollectionArgs( | ||
| name: name as NSString, collectionName: collectionName as NSString, | ||
| scopeName: scopeName as NSString, reject: reject | ||
| ) |
There was a problem hiding this comment.
I thought Swift will bridge String to NSString automatically. It might be good to double check this. But then another question is that why adaptCollectionArgs needs NSString from the first place (It can just take String).
** If calling objective-c needs to convert it to NSString, it can be done at the very. end instead of converting them from the start. This would allow Swift code use its native time until the point that it needs to be converted. Also most of Swift types can be bridged to Objective-C automatically without explicitly casting.
There was a problem hiding this comment.
For consideration:
-
I understand that adaptCollectionArgs() will validate or do pre-condition check on the argument. It might be better to make the function name more specific such as validate() or preconditionCheck().
-
I'm not sure if it's useful to repackage the parameter into an arugment object. You can make the validation function returns just a boolean so you can do like this:
guard DataAdapter.shared.adaptCollectionArgs(databaseName: name, collectionName: collectionName, scopeName: scopeName, reject: reject) else { return }
- For an alternative approach, you can have generic validation function like below. I personally like something like this as when I read the code, I know like away what will be validated even though it might feel a bit more repetitive codes.
guard Preconditions.notEmpty(name, reject, field: "name") else { return }
guard Preconditions.notEmpty(collectionName, reject, field: "collectionName") else { return }
guard Preconditions.notEmpty(scopeName, reject, field: "scopeName") else { return }
Note: field parameter is for generating an error message so that it can include the name of the parameter.
| } | ||
|
|
||
| public func collection_CreateCollection( | ||
| collectionName: String, name: String, scopeName: String, |
There was a problem hiding this comment.
Have name argument between collectionName and scopeName is confusing to me. I understand that it's the database name.
I would expect the database name will be the first argument like this (This applies to the other functions as well):
collection_CreateCollection(databaseName: String, collectionName: String, scopeName: String, ...)
| "\(args.collectionName)> in database <\(args.databaseName)>", nil) | ||
| return | ||
| } | ||
| let dict = DataAdapter.shared.adaptCollectionToNSDictionary( |
There was a problem hiding this comment.
I check the returned dict and the format is like
[
"name": "collection-name",
"scope": [
"name": "scope-name"
"databaseName": "database-name"
]
]
My opinion is that the databaseName shouldn't be with the scope. It's not conceptually correct. For CBL, a scope is just a namespace of collections.
I would expect something like the following :
[
"database": "database-name",
"scope": "scope-name",
"collection": "collection-name"
]
There was a problem hiding this comment.
Also I don't see much benefit of having a function (on another class) to return the result value as it seems to be something very specific to each API.
Doing something like this feels more clear to me when reading the code :
let result = [
"database": collection.database.name,
"scope": collection.scope.name,
"collection": "collection.name"
]
resolve(result)
| let (isError, args) = DataAdapter.shared.adaptCollectionArgs( | ||
| name: name as NSString, collectionName: collectionName as NSString, | ||
| scopeName: scopeName as NSString, reject: reject | ||
| ) |
There was a problem hiding this comment.
if validation is failed, it should return first instead of moving to the next step.
| name: name as NSString, collectionName: collectionName as NSString, | ||
| scopeName: scopeName as NSString, reject: reject | ||
| ) | ||
| let (isIdxNameError, idxName) = DataAdapter.shared.adaptNonEmptyString( |
There was a problem hiding this comment.
Seems like this is what I mentioned earlier.
| } | ||
|
|
||
| public func collection_CreateIndex( | ||
| indexName: String, index: Any, collectionName: String, |
There was a problem hiding this comment.
If index needs to be dictionary, can it be explicitly defined here?
| ) { | ||
| backgroundQueue.async { | ||
| guard let record = ListenerTokenStore.shared.remove(token: changeListenerToken) else { | ||
| reject("LISTENER_ERROR", |
There was a problem hiding this comment.
This can be just no-ops without returning an error.
