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 modernizes the Android implementation of Rive-CMP by migrating from the legacy RiveAnimationView to the native Jetpack Compose RiveUI API. This transition enhances performance, aligns with current best practices, and resolves critical native C++ rendering issues, particularly those encountered with LazyColumn recycling. Highlights
Changelog
Activity
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 migrates the Android implementation to the modern Rive Compose API, aligning the library with current best practices, improving performance with caching, and resolving critical native crashes. However, this migration introduces a potential security vulnerability related to how remote Rive files are fetched. The review also focuses on refining the new API surface, improving error handling, and ensuring consistency across the common and platform-specific code to make the API less error-prone and more maintainable.
| RiveUI( | ||
| file = actualFile, | ||
| modifier = modifier, | ||
| artboard = artboard, | ||
| stateMachineName = stateMachineName, | ||
| viewModelInstance = vmi, | ||
| fit = fit.toAndroidFit(), | ||
| alignment = alignment.toAndroidAlignment() | ||
| ) |
There was a problem hiding this comment.
There are a couple of issues with parameter handling here:
-
The
autoPlayparameter is not passed to theRiveUIcomposable. This means the caller's input is ignored, and the animation will useRiveUI's defaultautoplayvalue (true). You should pass theautoPlayparameter through. -
The
overlayparameter is a remnant from the old API and is not used by the newRiveUIcomposable. It should be removed from allCustomRiveAnimationfunction signatures (including theexpectdeclarations in common code) to avoid confusion.
RiveUI(
file = actualFile,
modifier = modifier,
artboard = artboard,
stateMachineName = stateMachineName,
viewModelInstance = vmi,
autoplay = autoPlay,
fit = fit.toAndroidFit(),
alignment = alignment.toAndroidAlignment()
)| actual fun setNumberInput(stateMachineName: String, name: String, value: Float) { | ||
| animationViewRef?.setNumberState( | ||
| stateMachineName = stateMachineName, | ||
| inputName = name, | ||
| value = value | ||
| ) | ||
| viewModelInstanceRef?.setNumber(name, value) | ||
| } |
There was a problem hiding this comment.
The stateMachineName parameter is unused here, and also in setBooleanInput and setTriggerInput. The ViewModelInstance is already scoped to a specific state machine at creation time, making this parameter redundant in the Android implementation.
To create a consistent and clear API, this parameter should be removed from these methods in the common expect class RiveComposition and all its actual implementations.
| actual fun setNumberInput(stateMachineName: String, name: String, value: Float) { | |
| animationViewRef?.setNumberState( | |
| stateMachineName = stateMachineName, | |
| inputName = name, | |
| value = value | |
| ) | |
| viewModelInstanceRef?.setNumber(name, value) | |
| } | |
| actual fun setNumberInput(name: String, value: Float) { | |
| viewModelInstanceRef?.setNumber(name, value) | |
| } |
| if (value == null) { | ||
| value = withContext(Dispatchers.IO) { | ||
| try { | ||
| val downloaded = URL(url).readBytes() |
There was a problem hiding this comment.
The CustomRiveAnimationInternal function uses URL(url).readBytes() to fetch animation data from a provided URL. This URL is passed directly from the public CustomRiveAnimation composable without any validation or sanitization. An attacker could provide a malicious URL using schemes like file:// to read sensitive local files from the application's data directory or assets, or use http:// to perform requests to internal network resources (SSRF). While this is a client-side library, the lack of URL validation can lead to local file inclusion or unauthorized network requests if the application allows user-controlled URLs to be passed to this component.
| } | ||
|
|
||
| @ExperimentalRiveCmpApi | ||
| private val riveByteCache = LruCache<String, ByteArray>(20) |
There was a problem hiding this comment.
The cache size 20 is a magic number. It's better to define it as a named constant to improve readability and make it easier to change in the future. Please define a constant for this value and use it here.
| private val riveByteCache = LruCache<String, ByteArray>(20) | |
| private val riveByteCache = LruCache<String, ByteArray>(RIVE_BYTE_CACHE_SIZE) |
| } catch (e: Exception) { | ||
| null | ||
| } |
There was a problem hiding this comment.
Catching a generic Exception is too broad and can hide unexpected runtime errors. It's better to catch more specific exceptions, like java.io.IOException, to handle network or file-related errors explicitly. Additionally, logging the exception would be very helpful for debugging cases where an animation fails to load.
} catch (e: java.io.IOException) {
// Consider logging the exception for easier debugging, e.g., using android.util.Log
null
}| else -> { | ||
| // Wait while loading or handle failure silently | ||
| } |
There was a problem hiding this comment.
Silently ignoring loading failures makes it very difficult to diagnose problems, such as a corrupt Rive file or other loading errors. While you might not want to show an error UI, you should at least log the failure case. This will be invaluable for debugging.
else -> {
if (result is Result.Failure) {
// Log the error to help with debugging, e.g., using android.util.Log
}
// Wait while loading or handle failure silently
}
2 participants
Description
This PR migrates the Android implementation of
Rive-CMPfrom the legacy imperativeRiveAnimationViewto the modern Jetpack Compose native RiveUI API provided byapp.rive:rive-android:10.5.0.This migration aligns the library with the official Rive Android Compose API documentation. These changes modernize the rendering pipeline, deprecate old state machine inputs in favor of ViewModels, and resolve several native C++
libEGLcontext crashes related toLazyColumnrecycling.Changes Made
AndroidViewwrapper aroundRiveAnimationViewwith the new Compose native RiveUI composable.withContext(Dispatchers.IO)and pass them toRiveFileSource.Bytes.#trigger()method is deprecated in favor of View Models.android.util.LruCacheto cache downloaded Rive byte arrays. This drastically reduces network overhead and lag when scrolling through lists of animations.Modifier.graphicsLayer { alpha = ... }instead of conditionalif (visible)blocks during swipe gestures. This prevents Compose from continuously creating/destroying the native OpenGL Contexts and completely eliminates thestd::__ndk1::mutex::lock()+12 (SIGSEGV)runtime crashes during rapid recompositions.