-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add live query listeners and fix dates conversion #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add live query listeners and fix dates conversion #55
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @DominikZajac, 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!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR introduces the capability to add and remove live query change listeners specifically for the Android platform. This allows the React Native application to react in real-time to changes in the results of a Couchbase Lite query. Additionally, it includes a fix for how date parameters are handled when passed to queries via the DataAdapter
, ensuring correct parsing of various ISO 8601 date formats.
Highlights
- Live Query Listeners (Android): Adds native methods (
query_AddChangeListener
andquery_RemoveChangeListener
) to the Android module (CblReactnativeModule.kt
) to enable subscribing to and unsubscribing from live query changes. Changes are sent back to React Native via events. - Date Parameter Conversion Fix: Corrects the
DataAdapter.kt
to properly parse date strings provided as query parameters. It now correctly accesses the date value from the nested map and uses a new helper function (parseIsoDate
) that attempts to parse the date string using multiple common ISO 8601 formats. - Dependency Update: Updates the
cbl-reactnative
version in theexpo-example
's iOSPodfile.lock
from 0.2.3 to 0.5.0.
Changelog
Click here to see the changelog
- android/src/main/java/com/cblreactnative/CblReactnativeModule.kt
- Added
queryChangeListeners
mutable map to store ListenerToken instances for active query listeners. - Implemented
query_AddChangeListener
method to create a Couchbase Lite query, set parameters, add a change listener, store the listener token, and send query results/errors back via React Native events. - Implemented
query_RemoveChangeListener
method to find and remove a query listener by its token.
- Added
- android/src/main/java/com/cblreactnative/DataAdapter.kt
- Imported
java.util.TimeZone
. - Modified
readableMapToParameters
to correctly retrieve the date string from the nested map (nestedMap.getString("value")
) instead of the outer map (map.getString("value")
). - Modified
readableMapToParameters
to use the newparseIsoDate
helper function for date parsing. - Added a new case 'value' in
readableMapToParameters
to handle generic parameter values. - Added
parseIsoDate
private helper function to attempt parsing a date string using multiple common ISO 8601 formats.
- Imported
- expo-example/ios/Podfile.lock
- Updated
cbl-reactnative
dependency version from 0.2.3 to 0.5.0. - Updated the checksum for
cbl-reactnative
.
- Updated
- expo-example/ios/expoexample.xcodeproj/project.pbxproj
- Added quotes around the
PRODUCT_NAME
value in the project settings.
- Added quotes around the
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Data flows like streams,
Queries listen, soft gleams,
Dates parsed just right.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces live query listeners for Android and refines date conversion logic in the DataAdapter
. The addition of live query functionality is a significant enhancement. The date parsing improvements in DataAdapter
also increase robustness.
Overall, the changes are well-intentioned. However, there are a few areas, particularly concerning coroutine scope management and thread safety in the Android module, and date parsing locale, that should be addressed to ensure stability and correctness. The data adapter changes for parameters also warrant a closer look for handling complex types.
Summary of Findings
- Coroutine Scope Management: The use of
GlobalScope.launch
inCblReactnativeModule.kt
(lines 938, 990) is a high-severity concern as it can lead to resource leaks or unexpected behavior in Android/React Native modules. A lifecycle-awareCoroutineScope
should be used instead. - Thread Safety of Listeners Map: The
queryChangeListeners
map inCblReactnativeModule.kt
(line 32) is aMutableMap
accessed from IO dispatcher coroutines. This could lead to concurrency issues. Using a thread-safe map implementation likejava.util.Collections.synchronizedMap
orConcurrentHashMap
is recommended. - JSON Array Construction: In
CblReactnativeModule.kt
(lines 959-964), a JSON array string is constructed manually. Usingorg.json.JSONArray
would be more robust and align with best practices. - Date Parsing Locale: In
DataAdapter.kt
(line 580),parseIsoDate
usesLocale.getDefault()
withSimpleDateFormat
. For fixed-format ISO date parsing,Locale.US
orLocale.ROOT
is generally safer to ensure consistency. - Handling Dynamic Types in Query Parameters: In
DataAdapter.kt
(line 366), the handling of generic"value"
types inreadableMapToParameters
might not be fully robust for complexReadableType.Map
orReadableType.Array
when passed toqueryParameters.setValue()
. Explicit conversion to Couchbase LiteDictionary
orArray
might be necessary. - Code Style: Indentation (Low Severity - Not Commented): The
DataAdapter.kt
file has inconsistent indentation in thereadableMapToParameters
method (lines 344-374). This was not commented on due to review settings. - Optimization: Listener Removal (Low Severity - Not Commented): In
CblReactnativeModule.kt
(lines 993-995), fetching and removing a listener fromqueryChangeListeners
could be a single map operation. This was not commented on due to review settings.
Merge Readiness
The pull request introduces valuable features. However, due to the high-severity issue related to GlobalScope
usage and other medium-severity concerns regarding thread safety, JSON handling, and date parsing, I recommend that these changes be addressed before merging. Addressing these points will significantly improve the stability and robustness of the Android module. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from authorized team members after addressing the feedback.
name: String, | ||
promise: Promise | ||
) { | ||
GlobalScope.launch(Dispatchers.IO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using GlobalScope.launch
is generally discouraged in Android components like React Native modules. Coroutines launched in GlobalScope
are not tied to any specific lifecycle, meaning they can continue to run even after the module or activity is destroyed, potentially leading to memory leaks or crashes (e.g., trying to access a destroyed context or update UI).
Could you consider using a custom CoroutineScope
that is tied to the lifecycle of this ReactContextBaseJavaModule
? For example, you could create a scope like private val moduleScope = CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate)
(or Dispatchers.Default
if Main
is not always needed for launch) and then cancel this scope in a lifecycle method like onCatalystInstanceDestroy()
.
This would ensure that any ongoing work is cleaned up when the module is no longer in use.
changeListenerToken: String, | ||
promise: Promise | ||
) { | ||
GlobalScope.launch(Dispatchers.IO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment on line 938, using GlobalScope.launch
here presents risks of memory leaks or unexpected behavior if the module is destroyed while this coroutine is active.
It's recommended to use a lifecycle-aware CoroutineScope
for background tasks within this module. This ensures that operations are cancelled when the module instance is no longer valid.
@@ -29,6 +29,7 @@ class CblReactnativeModule(reactContext: ReactApplicationContext) : | |||
|
|||
// Property to hold the context | |||
private val context: ReactApplicationContext = reactContext | |||
private val queryChangeListeners: MutableMap<String, ListenerToken> = mutableMapOf() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The queryChangeListeners
map is a MutableMap
. While individual operations on a java.util.Collections.synchronizedMap
are thread-safe, compound operations or iteration would still require external synchronization. Given that listeners are added and removed from coroutines running on Dispatchers.IO
(see lines 938, 990), and these methods can be called from JavaScript, there's a potential for concurrent modifications if not handled carefully by React Native's threading model for module method invocations.
Consider using java.util.concurrent.ConcurrentHashMap
for better concurrent performance and safety, or ensure all accesses and modifications to this map are properly synchronized if complex operations are needed. For simple put/remove/get, a synchronized map wrapper could be sufficient.
Would java.util.Collections.synchronizedMap(mutableMapOf())
or ConcurrentHashMap
be more appropriate here to prevent potential race conditions?
private val queryChangeListeners: MutableMap<String, ListenerToken> = mutableMapOf() | |
private val queryChangeListeners: MutableMap<String, ListenerToken> = java.util.Collections.synchronizedMap(mutableMapOf()) |
val resultList = mutableListOf<String>() | ||
for (result in results) { | ||
resultList.add(result.toJSON()) | ||
} | ||
val jsonArray = "[" + resultList.joinToString(",") + "]" | ||
resultMap.putString("data", jsonArray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually constructing JSON strings (e.g., "[" + resultList.joinToString(",") + "]"
) can be error-prone, especially if the content of result.toJSON()
isn't strictly controlled or could contain characters that break the structure. While Result.toJSON()
from the Couchbase Lite SDK should produce valid JSON strings for each result, using a dedicated JSON library like org.json.JSONArray
and org.json.JSONObject
to build the final JSON array is generally more robust and less susceptible to formatting issues.
Have you considered using org.json.JSONArray
to build the data
string? This would also handle any necessary escaping automatically.
val dataJsonArray = org.json.JSONArray()
for (result in results) {
// Assuming result.toJSON() returns a String that is a valid JSON object.
// org.json.JSONObject can parse this string.
try {
dataJsonArray.put(org.json.JSONObject(result.toJSON()))
} catch (e: org.json.JSONException) {
// Log error or handle appropriately if result.toJSON() is not a valid JSON object string
// For example, if it could be a primitive JSON value string:
// dataJsonArray.put(result.toJSON());
}
}
resultMap.putString("data", dataJsonArray.toString())
ReadableType.Boolean -> queryParameters.setBoolean(key, value.asBoolean()) | ||
ReadableType.Number -> queryParameters.setDouble(key, value.asDouble()) | ||
ReadableType.String -> queryParameters.setString(key, value.asString()) | ||
else -> queryParameters.setValue(key, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the new "value"
type handling within readableMapToParameters
, the else
case for value.type
(where value
is a Dynamic
) calls queryParameters.setValue(key, value)
. While Couchbase Lite's Parameters.setValue
is flexible, directly passing a Dynamic
object might not be fully robust for all underlying ReadableType
s, especially complex types like ReadableMap
or ReadableArray
.
Does queryParameters.setValue(key, value)
correctly handle cases where value.type
is ReadableType.Map
or ReadableType.Array
? It might be safer to explicitly convert these to HashMap
/ArrayList
(or even better, CBL Dictionary
/Array
) before setting. For instance:
when (value.type) {
// ... other cases
ReadableType.Map -> queryParameters.setDictionary(key, Dictionary(toMap(value.asMap()))) // toMap handles nested blobs
ReadableType.Array -> queryParameters.setArray(key, Array(value.asArray().toArrayList())) // Elements might need deeper conversion
ReadableType.Null -> queryParameters.setValue(key, null)
else -> queryParameters.setValue(key, value.asNative()) // asNative() might be safer for other Dynamic types
}
This ensures that the types passed to queryParameters
are what Couchbase Lite expects. Could you verify the current behavior with nested maps/arrays passed as generic 'value' types?
) | ||
for (format in formats) { | ||
try { | ||
val sdf = SimpleDateFormat(format, Locale.getDefault()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Locale.getDefault()
with SimpleDateFormat
can sometimes lead to unexpected parsing behavior if the default locale has date/time formatting conventions that conflict with the ISO 8601 standard, even though ISO 8601 is designed to be locale-independent. For fixed-format parsing like ISO dates, it's generally safer to use a specific, non-varying locale like Locale.US
or Locale.ROOT
(if available and appropriate for API level) to ensure consistent parsing across all devices.
Would it be more robust to use Locale.US
here?
val sdf = SimpleDateFormat(format, Locale.getDefault()) | |
val sdf = SimpleDateFormat(format, Locale.US) |
This PR add query change listeners on android and fixes the params data adapter.