Conversation
Summary of ChangesHello, 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 significantly advances the Flocon project's multiplatform strategy by integrating WebAssembly (WasmJs) support across its core modules. The changes enable the framework to compile and run on WasmJs, laying the groundwork for web-based applications while maintaining its modular and plugin-based architecture. This expansion broadens the reach of Flocon's standardized solutions for cross-cutting concerns like networking and data management to a new platform. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly expands the Flocon Kotlin Multiplatform project by adding WebAssembly (WasmJs) support across various modules, including database, datastores, deeplinks, flocon core, and network components. This involved updating Gradle build configurations to include the wasmJs target and providing platform-specific implementations or stubs for WasmJs. A new FloconProject Overview markdown file was also added. The review identified several issues: a critical recursive definition of Dispatchers.IO in Android and JVM source sets leading to StackOverflowError, incorrect stubbing of time functions (currentTimeMillis, currentTimeNanos) in WasmJs, an unused constructor parameter in FloconRoom3DatabaseProviderImpl.wasmJs.kt, commented-out imports in FloconDatastorePreference.kt, and a potential bug in decodeNetworkBody in ktor-interceptor for WasmJs where charset information from headers is ignored.
| public actual val Dispatchers.IO: CoroutineDispatcher get() = Dispatchers.IO | ||
|
|
||
| public actual val IO: CoroutineDispatcher get() = Dispatchers.IO |
There was a problem hiding this comment.
The actual implementations for Dispatchers.IO and IO are recursive. Because this file is in the kotlinx.coroutines package, Dispatchers.IO on the right-hand side of the getter refers to the property being defined, not the original dispatcher from the coroutines library. This will cause a StackOverflowError at runtime. This is a critical issue that needs to be addressed. A possible solution is to not define these actuals in the kotlinx.coroutines package to avoid shadowing.
| public actual val Dispatchers.IO: CoroutineDispatcher get() = Dispatchers.IO | ||
|
|
||
| public actual val IO: CoroutineDispatcher get() = Dispatchers.IO |
There was a problem hiding this comment.
The actual implementations for Dispatchers.IO and IO are recursive. Because this file is in the kotlinx.coroutines package, Dispatchers.IO on the right-hand side of the getter refers to the property being defined, not the original dispatcher from the coroutines library. This will cause a StackOverflowError at runtime. This is a critical issue that needs to be addressed. A possible solution is to not define these actuals in the kotlinx.coroutines package to avoid shadowing.
| actual fun currentTimeMillis(): Long = 0L // Stub for now to ensure compilation | ||
|
|
||
| actual fun currentTimeNanos(): Long = 0L // Stub for now to ensure compilation |
There was a problem hiding this comment.
The functions currentTimeMillis and currentTimeNanos are stubbed to return 0L. This is incorrect and can lead to subtle bugs related to timing, caching, or logging. For WasmJS, you can get the current time from the JavaScript environment.
| actual fun currentTimeMillis(): Long = 0L // Stub for now to ensure compilation | |
| actual fun currentTimeNanos(): Long = 0L // Stub for now to ensure compilation | |
| actual fun currentTimeMillis(): Long = kotlin.js.Date.now().toLong() | |
| actual fun currentTimeNanos(): Long = (js("performance.now()") as Double * 1_000_000).toLong() |
| @OptIn(markerClass = [FloconMarker::class]) | ||
| internal actual class FloconRoom3DatabaseProviderImpl actual constructor( | ||
| private val context: FloconContext, | ||
| paths: List<String> |
| //import androidx.datastore.core.DataStore | ||
| //import androidx.datastore.preferences.core.Preferences | ||
| //import androidx.datastore.preferences.core.booleanPreferencesKey | ||
| //import androidx.datastore.preferences.core.doublePreferencesKey | ||
| //import androidx.datastore.preferences.core.edit | ||
| //import androidx.datastore.preferences.core.floatPreferencesKey | ||
| //import androidx.datastore.preferences.core.intPreferencesKey | ||
| //import androidx.datastore.preferences.core.longPreferencesKey | ||
| //import androidx.datastore.preferences.core.stringPreferencesKey |
| internal actual fun decodeNetworkBody( | ||
| bytes: ByteArray, | ||
| headers: Map<String, String> | ||
| ): String = bytes.decodeToString() |
There was a problem hiding this comment.
The decodeNetworkBody function currently ignores the headers parameter, which may contain important charset information in the Content-Type header. This can lead to incorrect decoding of the response body if it's not UTF-8. The headers should be parsed to extract the charset for a more robust implementation.
No description provided.